| Message ID | 20191007080339.57209-1-manu@freebsd.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: Set status to disable for MMC3 | expand |
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 >
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> [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>
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> [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
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"; }; };
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(+)