[PATCH 2/2] arm: omap3: am35x: Disable hlt when using Davinci EMAC
Igor Grinberg
grinberg at compulab.co.il
Tue May 15 08:42:09 EDT 2012
On 05/15/12 00:32, Kevin Hilman wrote:
> "Mark A. Greer" <mgreer at animalcreek.com> writes:
>
>> On Mon, May 14, 2012 at 11:20:58AM +0300, Igor Grinberg wrote:
>>> Hi Mark,
>>
>> Hi Igor.
>>
>>> Thanks for the great work!
>>>
>>> On 05/12/12 00:12, Mark A. Greer wrote:
>>>> From: "Mark A. Greer" <mgreer at animalcreek.com>
>>>>
>>>> The am35x family of SoCs has a Davinci EMAC ethernet
>>>> controller on-chip. Unfortunately, the EMAC is unable
>>>> to wake the PRCM when there is network activity which
>>>> leads to a hung or extremely slow system when the MPU
>>>> has executed a 'wfi' instruction (because of pm_idle
>>>> or CPUidle). To prevent this, add hooks to the EMAC
>>>> pm_runtime suspend/resume calls so that hlt is disabled
>>>> whenever the EMAC is in use.
>>>>
>>>> Signed-off-by: Mark A. Greer <mgreer at animalcreek.com>
>>>> ---
>>>> arch/arm/mach-omap2/am35xx-emac.c | 44 +++++++++++++++++++++++++++++----
>>>> arch/arm/mach-omap2/am35xx-emac.h | 16 +++++++++---
>>>> arch/arm/mach-omap2/board-am3517evm.c | 3 ++-
>>>> arch/arm/mach-omap2/board-cm-t3517.c | 3 ++-
>>>> 4 files changed, 56 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/am35xx-emac.c b/arch/arm/mach-omap2/am35xx-emac.c
>>>> index 3bb5cb3..22ff968 100644
>>>> --- a/arch/arm/mach-omap2/am35xx-emac.c
>>>> +++ b/arch/arm/mach-omap2/am35xx-emac.c
>>>> @@ -23,6 +23,37 @@
>>>> #include "control.h"
>>>> #include "am35xx-emac.h"
>>>>
>>>> +/*
>>>> + * Default pm_lats for the am35x.
>>>> + * The net effect of using am35xx_emac_pm_lats[] is that
>>>> + * pm_idle or CPUidle won't be called while the emac
>>>> + * interface is open. This is required because the
>>>> + * EMAC can't wake up PRCM so if the MPU is executing
>>>> + * a 'wfi' instruction (e.g., from pm_idle or CPUidle),
>>>> + * it won't break out of it due to emac activity.
>>>> + */
>>>> +static int am35xx_emac_deactivate_func(struct omap_device *od)
>>>> +{
>>>> + enable_hlt();
>>>> + return omap_device_idle_hwmods(od);
>>>> +}
>>>> +
>>>> +static int am35xx_emac_activate_func(struct omap_device *od)
>>>> +{
>>>> + disable_hlt();
>>>> + return omap_device_enable_hwmods(od);
>>>> +}
>>>> +
>>>> +struct omap_device_pm_latency am35xx_emac_pm_lats[] = {
>>>> + {
>>>> + .deactivate_func = am35xx_emac_deactivate_func,
>>>> + .activate_func = am35xx_emac_activate_func,
>>>> + .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
>>>> + },
>>>> +};
>>>> +
>>>> +int am35xx_emac_pm_lats_size = ARRAY_SIZE(am35xx_emac_pm_lats);
>>>> +
>>>> static void am35xx_enable_emac_int(void)
>>>> {
>>>> u32 regval;
>>>> @@ -61,12 +92,13 @@ static struct emac_platform_data am35xx_emac_pdata = {
>>>> static struct mdio_platform_data am35xx_mdio_pdata;
>>>>
>>>> static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
>>>> - void *pdata, int pdata_len)
>>>> + void *pdata, int pdata_len,
>>>> + struct omap_device_pm_latency *pm_lats, int pm_lats_size)
>>>> {
>>>> struct platform_device *pdev;
>>>>
>>>> pdev = omap_device_build(oh->class->name, 0, oh, pdata, pdata_len,
>>>> - NULL, 0, false);
>>>> + pm_lats, pm_lats_size, false);
>>>> if (IS_ERR(pdev)) {
>>>> WARN(1, "Can't build omap_device for %s:%s.\n",
>>>> oh->class->name, oh->name);
>>>> @@ -76,7 +108,8 @@ static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
>>>> return 0;
>>>> }
>>>>
>>>> -void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
>>>> +void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
>>>> + struct omap_device_pm_latency *pm_lats, int pm_lats_size)
>>>> {
>>>> struct omap_hwmod *oh;
>>>> u32 regval;
>>>> @@ -91,7 +124,7 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
>>>> am35xx_mdio_pdata.bus_freq = mdio_bus_freq;
>>>>
>>>> ret = omap_davinci_emac_dev_init(oh, &am35xx_mdio_pdata,
>>>> - sizeof(am35xx_mdio_pdata));
>>>> + sizeof(am35xx_mdio_pdata), NULL, 0);
>>>> if (ret) {
>>>> pr_err("Could not build davinci_mdio hwmod device\n");
>>>> return;
>>>> @@ -106,7 +139,8 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
>>>> am35xx_emac_pdata.rmii_en = rmii_en;
>>>>
>>>> ret = omap_davinci_emac_dev_init(oh, &am35xx_emac_pdata,
>>>> - sizeof(am35xx_emac_pdata));
>>>> + sizeof(am35xx_emac_pdata),
>>>> + pm_lats, pm_lats_size);
>>>> if (ret) {
>>>> pr_err("Could not build davinci_emac hwmod device\n");
>>>> return;
>>>> diff --git a/arch/arm/mach-omap2/am35xx-emac.h b/arch/arm/mach-omap2/am35xx-emac.h
>>>> index 15c6f9c..7c23808 100644
>>>> --- a/arch/arm/mach-omap2/am35xx-emac.h
>>>> +++ b/arch/arm/mach-omap2/am35xx-emac.h
>>>> @@ -6,10 +6,20 @@
>>>> * published by the Free Software Foundation.
>>>> */
>>>>
>>>> +#include <plat/omap_device.h>
>>>> +
>>>> #define AM35XX_DEFAULT_MDIO_FREQUENCY 1000000
>>>>
>>>> -#if defined(CONFIG_TI_DAVINCI_EMAC) || defined(CONFIG_TI_DAVINCI_EMAC_MODULE)
>>>> -void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en);
>>>> +#if IS_ENABLED(CONFIG_TI_DAVINCI_EMAC)
>>>> +extern struct omap_device_pm_latency am35xx_emac_pm_lats[];
>>>> +extern int am35xx_emac_pm_lats_size;
>>>> +
>>>> +void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
>>>> + struct omap_device_pm_latency *pm_lats, int pm_lats_size);
>>>> #else
>>>> -static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en) {}
>>>> +#define am35xx_emac_pm_lats NULL
>>>> +#define am35xx_emac_pm_lats_size 0
>>>> +
>>>> +static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
>>>> + struct omap_device_pm_latency *pm_lats, int pm_lats_size) {}
>>>> #endif
>>>> diff --git a/arch/arm/mach-omap2/board-am3517evm.c b/arch/arm/mach-omap2/board-am3517evm.c
>>>> index 3645285..fe3a304 100644
>>>> --- a/arch/arm/mach-omap2/board-am3517evm.c
>>>> +++ b/arch/arm/mach-omap2/board-am3517evm.c
>>>> @@ -385,7 +385,8 @@ static void __init am3517_evm_init(void)
>>>> i2c_register_board_info(1, am3517evm_i2c1_boardinfo,
>>>> ARRAY_SIZE(am3517evm_i2c1_boardinfo));
>>>> /*Ethernet*/
>>>> - am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1);
>>>> + am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1,
>>>> + am35xx_emac_pm_lats, am35xx_emac_pm_lats_size);
>>>
>>> Why is it essential for board file to pass these pm structures?
>>> Is it something board specific?
>>> Can't you just use them inside the am35xx-emac.c file?
>>
>> Great question. I went back & forth on this mayself (I have 3 different
>> versions sitting in my repo :). I ended up passing the pm structures in
>> case there is a variant that comes along that doesn't need this fixup (or
>> a board that wants to do this plus additional things).
>>
>> The variants that I have are:
>>
>> 1) Leave am35xx_emac_init() interface the way it is and automatically
>> add the necessary things. The problem is lack of flexibility if & when
>> some other variant comes along.
>>
>> 2) Add pm_lats & pm_lats_size to am35xx_emac_init() but if they're NULL
>> or 0, then use the defaults that are the same as what's in this patch.
>> More flexible but doesn't easily allow a board file to NULL out the pm
>> structure (have to make up and pass NULL version).
>>
>> 3) Pass the pm struct via am35xx_emac_init() and just make the default
>> available for use. This is what I submitted since it was the most flexible.
>>
>> Which one do you prefer or do you have something else in mind?
>>
>
> For now, I suggest you stick with (1) until we have a reason to make it
> more flexible. AFAIK, all usages of this IP in OMAP-derivative parts
> have this same limitation.
I agree with Kevin - (1) should be used.
There is no need to complicate things in favor of something that might
or might not exist some day, because even if it will exist some day,
we probably anyway will see a better solution to this and will change
the existing one.
--
Regards,
Igor.
More information about the linux-arm-kernel
mailing list