diff mbox series

[PATCHv2] drivers: bus: simple-pm-bus: Fix compatibility with simple-bus for auxdata

Message ID 20210118073340.62141-1-tony@atomide.com
State Accepted
Commit eda080eabf5b9555e4d574ba035b0cb8aa42f052
Headers show
Series [PATCHv2] drivers: bus: simple-pm-bus: Fix compatibility with simple-bus for auxdata | expand

Commit Message

Tony Lindgren Jan. 18, 2021, 7:33 a.m. UTC
After converting am335x to probe devices with simple-pm-bus I noticed
that we are not passing auxdata for of_platform_populate() like we do
with simple-bus.

While device tree using SoCs should no longer need platform data, there
are still quite a few drivers that still need it as can be seen with
git grep OF_DEV_AUXDATA. We want to have simple-pm-bus be usable as a
replacement for simple-bus also for cases where OF_DEV_AUXDATA is still
needed.

Let's fix the issue by passing auxdata as platform data to simple-pm-bus.
That way the SoCs needing this can pass the auxdata with OF_DEV_AUXDATA.
And let's pass the auxdata for omaps to fix the issue for am335x.

As an alternative solution, adding simple-pm-bus handling directly to
drivers/of/platform.c was considered, but we would still need simple-pm-bus
device driver. So passing auxdata as platform data seems like the simplest
solution.

Fixes: 5a230524f879 ("ARM: dts: Use simple-pm-bus for genpd for am3 l4_wkup")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
Changes since v1: Updated description, added devicetree list to Cc
---
 arch/arm/mach-omap2/pdata-quirks.c | 1 +
 drivers/bus/simple-pm-bus.c        | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Rob Herring Jan. 19, 2021, 2:51 p.m. UTC | #1
On Mon, Jan 18, 2021 at 2:41 AM Tony Lindgren <tony@atomide.com> wrote:
>
> * Arnd Bergmann <arnd@kernel.org> [210118 08:30]:
> > On Mon, Jan 18, 2021 at 8:33 AM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > After converting am335x to probe devices with simple-pm-bus I noticed
> > > that we are not passing auxdata for of_platform_populate() like we do
> > > with simple-bus.
> > >
> > > While device tree using SoCs should no longer need platform data, there
> > > are still quite a few drivers that still need it as can be seen with
> > > git grep OF_DEV_AUXDATA. We want to have simple-pm-bus be usable as a
> > > replacement for simple-bus also for cases where OF_DEV_AUXDATA is still
> > > needed.
> > >
> > > Let's fix the issue by passing auxdata as platform data to simple-pm-bus.
> > > That way the SoCs needing this can pass the auxdata with OF_DEV_AUXDATA.
> > > And let's pass the auxdata for omaps to fix the issue for am335x.
> > >
> > > As an alternative solution, adding simple-pm-bus handling directly to
> > > drivers/of/platform.c was considered, but we would still need simple-pm-bus
> > > device driver. So passing auxdata as platform data seems like the simplest
> > > solution.
> > >
> > > Fixes: 5a230524f879 ("ARM: dts: Use simple-pm-bus for genpd for am3 l4_wkup")
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > ---
> > > Changes since v1: Updated description, added devicetree list to Cc
> >
> > This looks fine to me for now
> >
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> Thanks for the review.
>
> > But I think we should take the time to discuss how to phase out auxdata
> > over time. There are still a number of users, but it's not that many in the
> > end. For some of them I see a clear solution, for other ones I do not:
>
> Yes agreed we should remove the auxdata use.
>
> > omap2: I'll leave these for Tony to comment
>
> The three hardest ones to update (because of PM dependencies):
>
> - PRM power managment interrupts that also pinctrl driver uses

I haven't looked at it, but can't one driver go find the other node
and the interrupts it needs? There's nothing wrong with a driver
looking outside 'its node' for information.

Rob
Tony Lindgren Jan. 19, 2021, 3:03 p.m. UTC | #2
* Rob Herring <robh+dt@kernel.org> [210119 14:51]:
> On Mon, Jan 18, 2021 at 2:41 AM Tony Lindgren <tony@atomide.com> wrote:
> > - PRM power managment interrupts that also pinctrl driver uses
> 
> I haven't looked at it, but can't one driver go find the other node
> and the interrupts it needs? There's nothing wrong with a driver
> looking outside 'its node' for information.

Yes sure once there are interrupt nodes for it :) It should eventually
be a chained irqchip or something like that. FYI, the current stuff is
the code in mach-omap2/prm_common.c.

Regards,

Tony
Rob Herring Jan. 19, 2021, 3:09 p.m. UTC | #3
+Linus W

On Mon, Jan 18, 2021 at 2:30 AM Arnd Bergmann <arnd@kernel.org> wrote:
>

> On Mon, Jan 18, 2021 at 8:33 AM Tony Lindgren <tony@atomide.com> wrote:

> >

> > After converting am335x to probe devices with simple-pm-bus I noticed

> > that we are not passing auxdata for of_platform_populate() like we do

> > with simple-bus.

> >

> > While device tree using SoCs should no longer need platform data, there

> > are still quite a few drivers that still need it as can be seen with

> > git grep OF_DEV_AUXDATA. We want to have simple-pm-bus be usable as a

> > replacement for simple-bus also for cases where OF_DEV_AUXDATA is still

> > needed.

> >

> > Let's fix the issue by passing auxdata as platform data to simple-pm-bus.

> > That way the SoCs needing this can pass the auxdata with OF_DEV_AUXDATA.

> > And let's pass the auxdata for omaps to fix the issue for am335x.

> >

> > As an alternative solution, adding simple-pm-bus handling directly to

> > drivers/of/platform.c was considered, but we would still need simple-pm-bus

> > device driver. So passing auxdata as platform data seems like the simplest

> > solution.

> >

> > Fixes: 5a230524f879 ("ARM: dts: Use simple-pm-bus for genpd for am3 l4_wkup")

> > Signed-off-by: Tony Lindgren <tony@atomide.com>

> > ---

> > Changes since v1: Updated description, added devicetree list to Cc

>

> This looks fine to me for now

>

> Acked-by: Arnd Bergmann <arnd@arndb.de>

>

> But I think we should take the time to discuss how to phase out auxdata

> over time. There are still a number of users, but it's not that many in the

> end. For some of them I see a clear solution, for other ones I do not:


Thanks for summarizing.

> lpc32xx: Used only for pl080 DMA data with the old method, needs to

>     be converted to use the proper DT binding that was added a few years

>     ago.

>

> kirkwood: I don't see what this does at all, as there is no pdata, and

>     there is no clkdev lookup for "mvebu-audio"


Probably nothing. I reached that conclusion on u300 too. Clocks got
added in DT and someone forgot to remove auxdata. Granted, it's pretty
non-obvious what the purpose is if there is no platform_data.

>

> orion: similar to kirkwood, these seem to have been added for

>     clkdev lookup, but the orion_clkdev_init() function seems to

>     not be called for the orion5x_dt variant.

>

> omap2: I'll leave these for Tony to comment

>

> spear3xx: pl022 and pl080 should just use the normal DT

>    binding, see lpc32xx.

>

> u300: platform is scheduled for removal

>

> integrator_ap: pl010_set_mctrl() needs a callback to

>     integrator_uart_set_mctrl(). I see no good alternative, but

>     a workaround might be to call into syscon directly from the

>     driver on versatile machines. For all I can tell, pl010 is only

>     used on versatile and ep93xx, so that would not harm a

>     commonly used driver.


That was my conclusion.

> versatile/integrator_cp: similar problem but for mmci, which is

>     used more widely. Used for card detection, which could

>     theoretically be implemented with a fake gpio driver, but that

>     might be excessive.

>

> mips/pic32: used for setting up DMA for sdhci, could be done

>    in a platform-specific sdhci front-end.

>

> arm-cci: used to pass cci address after ioremap(), avoiding

>    this would revert e9c112c94b01 ("perf/arm-cci: Untangle

>    global cci_ctrl_base").


Create a regmap and then secondary drivers needing register access can
lookup the regmap? Or just ioremap it twice... I'll take a closer look
at this one.

Rob
diff mbox series

Patch

diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
--- a/arch/arm/mach-omap2/pdata-quirks.c
+++ b/arch/arm/mach-omap2/pdata-quirks.c
@@ -522,6 +522,7 @@  static struct of_dev_auxdata omap_auxdata_lookup[] = {
 		       &dra7_ipu1_dsp_iommu_pdata),
 #endif
 	/* Common auxdata */
+	OF_DEV_AUXDATA("simple-pm-bus", 0, NULL, omap_auxdata_lookup),
 	OF_DEV_AUXDATA("ti,sysc", 0, NULL, &ti_sysc_pdata),
 	OF_DEV_AUXDATA("pinctrl-single", 0, NULL, &pcs_pdata),
 	OF_DEV_AUXDATA("ti,omap-prm-inst", 0, NULL, &ti_prm_pdata),
diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -16,6 +16,7 @@ 
 
 static int simple_pm_bus_probe(struct platform_device *pdev)
 {
+	const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
 	struct device_node *np = pdev->dev.of_node;
 
 	dev_dbg(&pdev->dev, "%s\n", __func__);
@@ -23,7 +24,7 @@  static int simple_pm_bus_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 
 	if (np)
-		of_platform_populate(np, NULL, NULL, &pdev->dev);
+		of_platform_populate(np, NULL, lookup, &pdev->dev);
 
 	return 0;
 }