[PATCH] ARM: dts: Set status to disable for MMC3

[PATCH] ARM: dts: Set status to disable for MMC3

[Emmanuel Vadot]

Commit 5b63fb90adb95 ("ARM: dts: Fix incomplete dts data for am3 and am4 mmc")
fixed the mmc instances on the l3 interconnect but removed the disabled status.
Fix this and let boards properly define it if it have it.

Fixes: 5b63fb90adb95 ("ARM: dts: Fix incomplete dts data for am3 and am4 mmc")
Signed-off-by: Emmanuel Vadot <manu@freebsd.org>
---
 arch/arm/boot/dts/am33xx.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index fb6b8aa12cc5..b3a1fd9e39fa 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -260,6 +260,7 @@
 				ti,needs-special-reset;
 				interrupts = <29>;
 				reg = <0x0 0x1000>;
+				status = "disabled";
 			};
 		};
 
-- 
2.22.0

Re: [PATCH] ARM: dts: Set status to disable for MMC3

[Tony Lindgren]

Hi,

* Emmanuel Vadot <manu@freebsd.org> [191007 08:04]:
> Commit 5b63fb90adb95 ("ARM: dts: Fix incomplete dts data for am3 and am4 mmc")
> fixed the mmc instances on the l3 interconnect but removed the disabled status.
> Fix this and let boards properly define it if it have it.

The dts default is "okay", and should be fine for all the
internal devices even if not pinned out on the board. This
way the devices get properly idled during boot, and we
avoid repeating status = "enabled" over and over again in
the board specific dts files.

Then the board specific dts files might want to configure
devices with status = "disabled" if really needed. But this
should be only done for devices that Linux must not use,
such as crypto acclerators on secure devices if claimed by
the secure mode.

So if this fixes something, it's almost certainly a sign
of something else being broken?

Regards,

Tony


> Fixes: 5b63fb90adb95 ("ARM: dts: Fix incomplete dts data for am3 and am4 mmc")
> Signed-off-by: Emmanuel Vadot <manu@freebsd.org>
> ---
>  arch/arm/boot/dts/am33xx.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index fb6b8aa12cc5..b3a1fd9e39fa 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -260,6 +260,7 @@
>  				ti,needs-special-reset;
>  				interrupts = <29>;
>  				reg = <0x0 0x1000>;
> +				status = "disabled";
>  			};
>  		};
>  
> -- 
> 2.22.0
> 

Re: [PATCH] ARM: dts: Set status to disable for MMC3

[Emmanuel Vadot]

On Mon, 7 Oct 2019 09:16:34 -0700
Tony Lindgren <tony@atomide.com> wrote:

> Hi,
> 
> * Emmanuel Vadot <manu@freebsd.org> [191007 08:04]:
> > Commit 5b63fb90adb95 ("ARM: dts: Fix incomplete dts data for am3 and am4 mmc")
> > fixed the mmc instances on the l3 interconnect but removed the disabled status.
> > Fix this and let boards properly define it if it have it.
> 
> The dts default is "okay", and should be fine for all the
> internal devices even if not pinned out on the board. This
> way the devices get properly idled during boot, and we
> avoid repeating status = "enabled" over and over again in
> the board specific dts files.

 That is not correct, if a status != "disabled" then pinmuxing will be
configured for this device and if multiple devices share the same pin
then you have a problem. Note that I have (almost) no knowledge on Ti
SoC but I doubt that this is not the case on them.
 Also every other boards that I work with use the standard of setting
every node to disabled in the dtsi and let the board enable them at
will. Is there something different happening in the TI world ?

> Then the board specific dts files might want to configure
> devices with status = "disabled" if really needed. But this
> should be only done for devices that Linux must not use,
> such as crypto acclerators on secure devices if claimed by
> the secure mode.
> 
> So if this fixes something, it's almost certainly a sign
> of something else being broken?

 In this case it's FreeBSD being  because (I think) we have bad support
for the clocks for this module so we panic when we read from it as the
module isn't clocked. And since I find it wrong to have device enabled
while it isn't present I've sent this patch.

 Cheers,

> Regards,
> 
> Tony
> 
> 
> > Fixes: 5b63fb90adb95 ("ARM: dts: Fix incomplete dts data for am3 and am4 mmc")
> > Signed-off-by: Emmanuel Vadot <manu@freebsd.org>
> > ---
> >  arch/arm/boot/dts/am33xx.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> > index fb6b8aa12cc5..b3a1fd9e39fa 100644
> > --- a/arch/arm/boot/dts/am33xx.dtsi
> > +++ b/arch/arm/boot/dts/am33xx.dtsi
> > @@ -260,6 +260,7 @@
> >  				ti,needs-special-reset;
> >  				interrupts = <29>;
> >  				reg = <0x0 0x1000>;
> > +				status = "disabled";
> >  			};
> >  		};
> >  
> > -- 
> > 2.22.0
> > 


-- 
Emmanuel Vadot <manu@bidouilliste.com>

Re: [PATCH] ARM: dts: Set status to disable for MMC3

[Tony Lindgren]

* Emmanuel Vadot <manu@bidouilliste.com> [191007 16:39]:
> On Mon, 7 Oct 2019 09:16:34 -0700
> Tony Lindgren <tony@atomide.com> wrote:
> 
> > Hi,
> > 
> > * Emmanuel Vadot <manu@freebsd.org> [191007 08:04]:
> > > Commit 5b63fb90adb95 ("ARM: dts: Fix incomplete dts data for am3 and am4 mmc")
> > > fixed the mmc instances on the l3 interconnect but removed the disabled status.
> > > Fix this and let boards properly define it if it have it.
> > 
> > The dts default is "okay", and should be fine for all the
> > internal devices even if not pinned out on the board. This
> > way the devices get properly idled during boot, and we
> > avoid repeating status = "enabled" over and over again in
> > the board specific dts files.
> 
>  That is not correct, if a status != "disabled" then pinmuxing will be
> configured for this device and if multiple devices share the same pin
> then you have a problem. Note that I have (almost) no knowledge on Ti
> SoC but I doubt that this is not the case on them.

Hmm well, that should not be needed. The pinmux configuration is always
done in a board specific dts file.

>  Also every other boards that I work with use the standard of setting
> every node to disabled in the dtsi and let the board enable them at
> will. Is there something different happening in the TI world ?

There should be no need to do that for SoC internal devices, the
the default status = "okay" should be just fine. Setting the
status = "disabled" for SoC internal devices and then enabling them
again for tens of board specific dts files just generates tons of
pointless extra churn for the board specific configuration.

> > Then the board specific dts files might want to configure
> > devices with status = "disabled" if really needed. But this
> > should be only done for devices that Linux must not use,
> > such as crypto acclerators on secure devices if claimed by
> > the secure mode.
> > 
> > So if this fixes something, it's almost certainly a sign
> > of something else being broken?
> 
>  In this case it's FreeBSD being  because (I think) we have bad support
> for the clocks for this module so we panic when we read from it as the
> module isn't clocked. And since I find it wrong to have device enabled
> while it isn't present I've sent this patch.

Thanks for clarifying what happens. OK, sounds like FreeBSD might be
missing clock handling for some devices then.

What Linux does is probe the internal devices and then idle the
unused ones as bootloaders often leave many things enabled. Otherwise
the SoC power management won't work properly because device clocks
will block deeper SoC idle states.

Regards,

Tony

> > > Fixes: 5b63fb90adb95 ("ARM: dts: Fix incomplete dts data for am3 and am4 mmc")
> > > Signed-off-by: Emmanuel Vadot <manu@freebsd.org>
> > > ---
> > >  arch/arm/boot/dts/am33xx.dtsi | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> > > index fb6b8aa12cc5..b3a1fd9e39fa 100644
> > > --- a/arch/arm/boot/dts/am33xx.dtsi
> > > +++ b/arch/arm/boot/dts/am33xx.dtsi
> > > @@ -260,6 +260,7 @@
> > >  				ti,needs-special-reset;
> > >  				interrupts = <29>;
> > >  				reg = <0x0 0x1000>;
> > > +				status = "disabled";
> > >  			};
> > >  		};
> > >  
> > > -- 
> > > 2.22.0
> > > 
> 
> 
> -- 
> Emmanuel Vadot <manu@bidouilliste.com>

Re: [PATCH] ARM: dts: Set status to disable for MMC3

[Emmanuel Vadot]

On Mon, 7 Oct 2019 09:58:59 -0700
Tony Lindgren <tony@atomide.com> wrote:

> * Emmanuel Vadot <manu@bidouilliste.com> [191007 16:39]:
> > On Mon, 7 Oct 2019 09:16:34 -0700
> > Tony Lindgren <tony@atomide.com> wrote:
> > 
> > > Hi,
> > > 
> > > * Emmanuel Vadot <manu@freebsd.org> [191007 08:04]:
> > > > Commit 5b63fb90adb95 ("ARM: dts: Fix incomplete dts data for am3 and am4 mmc")
> > > > fixed the mmc instances on the l3 interconnect but removed the disabled status.
> > > > Fix this and let boards properly define it if it have it.
> > > 
> > > The dts default is "okay", and should be fine for all the
> > > internal devices even if not pinned out on the board. This
> > > way the devices get properly idled during boot, and we
> > > avoid repeating status = "enabled" over and over again in
> > > the board specific dts files.
> > 
> >  That is not correct, if a status != "disabled" then pinmuxing will be
> > configured for this device and if multiple devices share the same pin
> > then you have a problem. Note that I have (almost) no knowledge on Ti
> > SoC but I doubt that this is not the case on them.
> 
> Hmm well, that should not be needed. The pinmux configuration is always
> done in a board specific dts file.

 For TI it seems to be that way, but clearly not for other brand.

> >  Also every other boards that I work with use the standard of setting
> > every node to disabled in the dtsi and let the board enable them at
> > will. Is there something different happening in the TI world ?
> 
> There should be no need to do that for SoC internal devices, the
> the default status = "okay" should be just fine. Setting the
> status = "disabled" for SoC internal devices and then enabling them
> again for tens of board specific dts files just generates tons of
> pointless extra churn for the board specific configuration.

 Setting the status = "okay" means that you can use the device. What's
the point of enabling a device if you can't use it ? Or worse can't
probe it like i2c or spi ?
 Is the plan for TI dts to have every (or almost) device tree node
enabled even if the device isn't usable on the board ?

> > > Then the board specific dts files might want to configure
> > > devices with status = "disabled" if really needed. But this
> > > should be only done for devices that Linux must not use,
> > > such as crypto acclerators on secure devices if claimed by
> > > the secure mode.
> > > 
> > > So if this fixes something, it's almost certainly a sign
> > > of something else being broken?
> > 
> >  In this case it's FreeBSD being  because (I think) we have bad support
> > for the clocks for this module so we panic when we read from it as the
> > module isn't clocked. And since I find it wrong to have device enabled
> > while it isn't present I've sent this patch.
> 
> Thanks for clarifying what happens. OK, sounds like FreeBSD might be
> missing clock handling for some devices then.
> 
> What Linux does is probe the internal devices and then idle the
> unused ones as bootloaders often leave many things enabled. Otherwise
> the SoC power management won't work properly because device clocks
> will block deeper SoC idle states.

 I can understand stand but then the bootload should be fixed to not
enable devices that aren't enabled in the DTS if it does that.

> Regards,
> 
> Tony
> 
> > > > Fixes: 5b63fb90adb95 ("ARM: dts: Fix incomplete dts data for am3 and am4 mmc")
> > > > Signed-off-by: Emmanuel Vadot <manu@freebsd.org>
> > > > ---
> > > >  arch/arm/boot/dts/am33xx.dtsi | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> > > > index fb6b8aa12cc5..b3a1fd9e39fa 100644
> > > > --- a/arch/arm/boot/dts/am33xx.dtsi
> > > > +++ b/arch/arm/boot/dts/am33xx.dtsi
> > > > @@ -260,6 +260,7 @@
> > > >  				ti,needs-special-reset;
> > > >  				interrupts = <29>;
> > > >  				reg = <0x0 0x1000>;
> > > > +				status = "disabled";
> > > >  			};
> > > >  		};
> > > >  
> > > > -- 
> > > > 2.22.0
> > > > 
> > 
> > 
> > -- 
> > Emmanuel Vadot <manu@bidouilliste.com>


-- 
Emmanuel Vadot <manu@bidouilliste.com>

Re: [PATCH] ARM: dts: Set status to disable for MMC3

[Tony Lindgren]

* Emmanuel Vadot <manu@bidouilliste.com> [191007 17:30]:
> On Mon, 7 Oct 2019 09:58:59 -0700
> Tony Lindgren <tony@atomide.com> wrote:
> > There should be no need to do that for SoC internal devices, the
> > the default status = "okay" should be just fine. Setting the
> > status = "disabled" for SoC internal devices and then enabling them
> > again for tens of board specific dts files just generates tons of
> > pointless extra churn for the board specific configuration.
> 
>  Setting the status = "okay" means that you can use the device. What's
> the point of enabling a device if you can't use it ? Or worse can't
> probe it like i2c or spi ?

The main reasons for not setting SoC internal devices with
status = "disabled" are:

1. The SoC internal device is there for sure and also accessible
   for the kernel

2. Setting an SoC internal device with status = "disabled" makes
   the kernel completely ignore it and it cannot be idled for PM
   if left on from the bootloader

3. We avoid tens of lines of pointess repetive status = "okay"
   tinkering for multiple devices per each board specic dts file
   making them more readable and easier to maintain

>  Is the plan for TI dts to have every (or almost) device tree node
> enabled even if the device isn't usable on the board ?

This has always been the case with omap3, 4, and 5. I don't
know when folks started tagging am33xx SoC internal devices as
disabled but that should have never been necessary.

Few years back I suggested some patches for status = "incomplete"
to deal with cases where the device is there but not pinned
out or otherwise not complete. But so far we've gotten away
without that, so probably not needed.

> > >  In this case it's FreeBSD being  because (I think) we have bad support
> > > for the clocks for this module so we panic when we read from it as the
> > > module isn't clocked. And since I find it wrong to have device enabled
> > > while it isn't present I've sent this patch.
> > 
> > Thanks for clarifying what happens. OK, sounds like FreeBSD might be
> > missing clock handling for some devices then.
> > 
> > What Linux does is probe the internal devices and then idle the
> > unused ones as bootloaders often leave many things enabled. Otherwise
> > the SoC power management won't work properly because device clocks
> > will block deeper SoC idle states.
> 
>  I can understand stand but then the bootload should be fixed to not
> enable devices that aren't enabled in the DTS if it does that.

This is not doable as in many cases the bootloader cannot be
updated as it can be signed for phones like n900 and droid4.

Regards,

Tony