"Mark A. Greer" <mgreer@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxxxxxxx>
>> > ---
>> > 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.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
