ARM: dts: gta04: fix excess dma channel usage

  From: "H. Nikolaus Schaller" <hns@goldelico.com>

OMAP processors support 32 channels but there is no check or
inspect this except booting a device and looking at dmesg reports
of not available channels.

Recently some more subsystems with DMA (aes1+2) were added filling
the list of dma channels beyond the limit of 32 (even if other
parameters indicate 96 or 128 channels). This leads to random
subsystem failures i(e.g. mcbsp for audio) after boot or boot
messages that DMA can not be initialized.

Another symptom is that

/sys/kernel/debug/dmaengine/summary

has 32 entries and does not show all required channels.

Fix by disabling unused (on the GTA04 hardware) mcspi1...4.
Each SPI channel allocates 4 DMA channels rapidly filling
the available ones.

Disabling unused SPI modules on the OMAP3 SoC may also save
some energy (has not been checked).

Fixes: c312f066314e ("ARM: dts: omap3: Migrate AES from hwmods to sysc-omap2")
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
[re-enabled aes2, improved commit subject line]
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
  

On Fri, Jan 13, 2023 at 3:12 PM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> From: "H. Nikolaus Schaller" <hns@goldelico.com>
>
> OMAP processors support 32 channels but there is no check or
> inspect this except booting a device and looking at dmesg reports
> of not available channels.
>
> Recently some more subsystems with DMA (aes1+2) were added filling
> the list of dma channels beyond the limit of 32 (even if other
> parameters indicate 96 or 128 channels). This leads to random
> subsystem failures i(e.g. mcbsp for audio) after boot or boot
> messages that DMA can not be initialized.
>
> Another symptom is that
>
> /sys/kernel/debug/dmaengine/summary
>
> has 32 entries and does not show all required channels.
>
> Fix by disabling unused (on the GTA04 hardware) mcspi1...4.
> Each SPI channel allocates 4 DMA channels rapidly filling
> the available ones.
>
> Disabling unused SPI modules on the OMAP3 SoC may also save
> some energy (has not been checked).

Tony,

Would it make sense to make this default in the omap3.dtsi file and
enable them in the individual boards that need it?

From what I can tell the following use mcspi1:

logicpd-som-lv.dtsi
logicpd-torpedo-som.dtsi
omap3-evm-common.dtsi
omap3-ldp.dts
omap3-n900.dts
omap3-pandora-common.dtsi
omap3-tao3530.dtsi

The following use mcspi2:
omap3-lilly-a83x.dtsi

mscpi3 is used on:
omap3-tao3530.dtsi

and mcspi4:
omap3-n900.dts

In theory that would save a bunch of boards duplicating the disabled
status if they were to all follow suit.

I was looking into doing something like that for the mmc drivers on
various OMAP3 boards while disabling it on the omap3.dtsi. It seems
like some drivers are disabled by default (dss, ssi, mcbsp)  while
others are enabled by default (i2c, spi, mmc, usb_otg_hs, gpmc,
usbhshost, and a bunch of timers).  Disabling some of these also might
help speed up boot times if less devices need to enumerate.  I am
willing to do some of that work if the idea makes sense.

adam
>
> Fixes: c312f066314e ("ARM: dts: omap3: Migrate AES from hwmods to sysc-omap2")
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> [re-enabled aes2, improved commit subject line]
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  arch/arm/boot/dts/omap3-gta04.dtsi | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> index 87e0ab1bbe95..e0be0fb23f80 100644
> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> @@ -612,6 +612,22 @@ &i2c3 {
>         clock-frequency = <100000>;
>  };
>
> +&mcspi1 {
> +       status = "disabled";
> +};
> +
> +&mcspi2 {
> +       status = "disabled";
> +};
> +
> +&mcspi3 {
> +       status = "disabled";
> +};
> +
> +&mcspi4 {
> +       status = "disabled";
> +};
> +
>  &usb_otg_hs {
>         interface-type = <0>;
>         usb-phy = <&usb2_phy>;
> --
> 2.30.2
>
  

Hi,

* Adam Ford <aford173@gmail.com> [230116 14:16]:
> Would it make sense to make this default in the omap3.dtsi file and
> enable them in the individual boards that need it?

In general disabling the unused devices by default for omaps will break
the power management. The disabled devices are completely ignored by the
kernel, and the devices are left to whatever the bootloader state might
be.

For SoCs using firmware to manage devices it's a bit different story
however. The firmware can still idle disabled devices based on a
late_initcall for example, even if the kernel knows nothing about the
disabled devices.

Regards,

Tony
  

Hi,

> Am 16.01.2023 um 15:51 schrieb Tony Lindgren <tony@atomide.com>:
> 
> Hi,
> 
> * Adam Ford <aford173@gmail.com> [230116 14:16]:
>> Would it make sense to make this default in the omap3.dtsi file and
>> enable them in the individual boards that need it?
> 
> In general disabling the unused devices by default for omaps will break
> the power management. The disabled devices are completely ignored by the
> kernel, and the devices are left to whatever the bootloader state might
> be.

Yes, indeed.

> For SoCs using firmware to manage devices it's a bit different story
> however. The firmware can still idle disabled devices based on a
> late_initcall for example, even if the kernel knows nothing about the
> disabled devices.

But how can we then handle all devices being "okay" by default and
eating up more dma channels than are available?

We can't put all under power management AND dma by default.

Or can dma channel usage be postponed until the device is really used?

BR,
Nikolaus
  

On Mon, 16 Jan 2023 16:51:57 +0200
Tony Lindgren <tony@atomide.com> wrote:

> Hi,
> 
> * Adam Ford <aford173@gmail.com> [230116 14:16]:
> > Would it make sense to make this default in the omap3.dtsi file and
> > enable them in the individual boards that need it?  
> 
> In general disabling the unused devices by default for omaps will break
> the power management. The disabled devices are completely ignored by the
> kernel, and the devices are left to whatever the bootloader state might
> be.
> 
hmm, shouldn't ti-sysc keep things disabled in most cases? It is still a bit
known because there is no status = "disabled" in the target-module@xxx node.

Regards,
Andreas
  

* Andreas Kemnade <andreas@kemnade.info> [230116 16:39]:
> On Mon, 16 Jan 2023 16:51:57 +0200
> Tony Lindgren <tony@atomide.com> wrote:
> 
> > Hi,
> > 
> > * Adam Ford <aford173@gmail.com> [230116 14:16]:
> > > Would it make sense to make this default in the omap3.dtsi file and
> > > enable them in the individual boards that need it?  
> > 
> > In general disabling the unused devices by default for omaps will break
> > the power management. The disabled devices are completely ignored by the
> > kernel, and the devices are left to whatever the bootloader state might
> > be.
> > 
> hmm, shouldn't ti-sysc keep things disabled in most cases? It is still a bit
> known because there is no status = "disabled" in the target-module@xxx node.

Oh right, if the child device is tagged disabled (instead of the the parent
ti-sysc tagged disabled) the module should get idled just fine as long as the
module related quirks are implemented for ti-sysc.c.

But still, I'd rather not start tagging devices disabled by default and then
re-enabling everywhere since we never needed it before. It just adds a lot
of pointless status tinkering, see commit 12afc0cf8121 ("ARM: dts: Drop
pointless status changing for am3 musb").

So considering things, IMO it's best to set only the child device with
status disabled, and set it at the board specific dts file in this case.

Also note that the dma channels could be freed with /delete-property/ at the
board specific dts file even for devices that are usable if not really
needed.

Regards,

Tony
  

* H. Nikolaus Schaller <hns@goldelico.com> [230116 15:29]:
> Hi,
> 
> > Am 16.01.2023 um 15:51 schrieb Tony Lindgren <tony@atomide.com>:
> > 
> > Hi,
> > 
> > * Adam Ford <aford173@gmail.com> [230116 14:16]:
> >> Would it make sense to make this default in the omap3.dtsi file and
> >> enable them in the individual boards that need it?
> > 
> > In general disabling the unused devices by default for omaps will break
> > the power management. The disabled devices are completely ignored by the
> > kernel, and the devices are left to whatever the bootloader state might
> > be.
> 
> Yes, indeed.

See my further clarification based on what Adam commented too, I was
thinking status = "disabled" at the ti-sysc parent level.

> > For SoCs using firmware to manage devices it's a bit different story
> > however. The firmware can still idle disabled devices based on a
> > late_initcall for example, even if the kernel knows nothing about the
> > disabled devices.
> 
> But how can we then handle all devices being "okay" by default and
> eating up more dma channels than are available?

1. Set the child device (not the ti-sysc node) with status = "disabled"
   in the board specific file as needed

2. Use /delete-property/ for dma channels in the board specific file
   if the device is in use but does not need dma

3. Or if this is a generic problem, we could disable dma by default for
   some devices

> We can't put all under power management AND dma by default.
> 
> Or can dma channel usage be postponed until the device is really used?

Sure.

Regards,

Tony
  

On Mon, Jan 16, 2023 at 10:56 AM Tony Lindgren <tony@atomide.com> wrote:
>
> * Andreas Kemnade <andreas@kemnade.info> [230116 16:39]:
> > On Mon, 16 Jan 2023 16:51:57 +0200
> > Tony Lindgren <tony@atomide.com> wrote:
> >
> > > Hi,
> > >
> > > * Adam Ford <aford173@gmail.com> [230116 14:16]:
> > > > Would it make sense to make this default in the omap3.dtsi file and
> > > > enable them in the individual boards that need it?
> > >
> > > In general disabling the unused devices by default for omaps will break
> > > the power management. The disabled devices are completely ignored by the
> > > kernel, and the devices are left to whatever the bootloader state might
> > > be.
> > >
> > hmm, shouldn't ti-sysc keep things disabled in most cases? It is still a bit
> > known because there is no status = "disabled" in the target-module@xxx node.
>
> Oh right, if the child device is tagged disabled (instead of the the parent
> ti-sysc tagged disabled) the module should get idled just fine as long as the
> module related quirks are implemented for ti-sysc.c.
>
> But still, I'd rather not start tagging devices disabled by default and then
> re-enabling everywhere since we never needed it before. It just adds a lot
> of pointless status tinkering, see commit 12afc0cf8121 ("ARM: dts: Drop
> pointless status changing for am3 musb").
>
> So considering things, IMO it's best to set only the child device with
> status disabled, and set it at the board specific dts file in this case.

Doesn't this imply the target-module stuff needs to be implemented for
the drivers?  It looks like a lot of the omap3 drivers are still using
hwmods although some have target-modules. In this case, the mcspi
drivers that Andreas is disabling don't appear to have target-module
stuff configured.

adam

>
> Also note that the dma channels could be freed with /delete-property/ at the
> board specific dts file even for devices that are usable if not really
> needed.
>
> Regards,
>
> Tony
>
  

* Adam Ford <aford173@gmail.com> [230116 17:00]:
> Doesn't this imply the target-module stuff needs to be implemented for
> the drivers?  It looks like a lot of the omap3 drivers are still using
> hwmods although some have target-modules. In this case, the mcspi
> drivers that Andreas is disabling don't appear to have target-module
> stuff configured.

Sorry I don't remember if omap_device.c ignores status disabled or not.
But in any case, it should be trivial to update omap3.dtsi to configure
some of the devices like mcspi to probe with device tree data and ti-sysc
as needed.

Regards,

Tony
  

* Tony Lindgren <tony@atomide.com> [230116 17:33]:
> * Adam Ford <aford173@gmail.com> [230116 17:00]:
> > Doesn't this imply the target-module stuff needs to be implemented for
> > the drivers?  It looks like a lot of the omap3 drivers are still using
> > hwmods although some have target-modules. In this case, the mcspi
> > drivers that Andreas is disabling don't appear to have target-module
> > stuff configured.
> 
> Sorry I don't remember if omap_device.c ignores status disabled or not.
> But in any case, it should be trivial to update omap3.dtsi to configure
> some of the devices like mcspi to probe with device tree data and ti-sysc
> as needed.

So as long as gta04 power management still behaves with this patch it
should good to go.

Just to note, if mcspi is at some point is updated to probe with ti-sysc,
the disabled flag needs to be kept at the mcspi child node, not at the
sysc target module level to idle the unused devices.

Regards,

Tony
  

On Thu, 19 Jan 2023 09:30:20 +0200
Tony Lindgren <tony@atomide.com> wrote:

> * Tony Lindgren <tony@atomide.com> [230116 17:33]:
> > * Adam Ford <aford173@gmail.com> [230116 17:00]:  
> > > Doesn't this imply the target-module stuff needs to be implemented for
> > > the drivers?  It looks like a lot of the omap3 drivers are still using
> > > hwmods although some have target-modules. In this case, the mcspi
> > > drivers that Andreas is disabling don't appear to have target-module
> > > stuff configured.  
> > 
> > Sorry I don't remember if omap_device.c ignores status disabled or not.
> > But in any case, it should be trivial to update omap3.dtsi to configure
> > some of the devices like mcspi to probe with device tree data and ti-sysc
> > as needed.  
> 
> So as long as gta04 power management still behaves with this patch it
> should good to go.
> 
# sleep 10 ; /usr/local/bin/idledump
     CM_IDLEST1_CORE 00000042
     CM_IDLEST3_CORE 00000000
     CM_FCLKEN1_CORE 00000000
     CM_FCLKEN3_CORE 00000002
     CM_CLKSTST_CORE 00000003
     CM_IDLEST_CKGEN 00000209
    CM_IDLEST2_CKGEN 00000000
       CM_FCLKEN_DSS 00000000
       CM_IDLEST_DSS 00000000
       CM_FCLKEN_CAM 00000000
       CM_IDLEST_CAM 00000000
       CM_FCLKEN_PER 00000000
       CM_IDLEST_PER 00000000


FCLKEN3_CORE becomes 0 after unbinding the bandgap sensor.

but...
# cat /sys/kernel/debug/pm_debug/time 
usbhost_pwrdm (ON),OFF:830267486567,RET:0,INA:0,ON:12202880865
sgx_pwrdm (INA),OFF:0,RET:0,INA:841224365234,ON:1245971680
core_pwrdm (ON),OFF:0,RET:0,INA:0,ON:842470336914
per_pwrdm (ON),OFF:520406799328,RET:30043365464,INA:0,ON:292020111087

hmmm.... 

but does not look like anything related to mcspi*.

Regards,
Andreas
  

* Andreas Kemnade <andreas@kemnade.info> [230113 23:12]:
> From: "H. Nikolaus Schaller" <hns@goldelico.com>
> 
> OMAP processors support 32 channels but there is no check or
> inspect this except booting a device and looking at dmesg reports
> of not available channels.
> 
> Recently some more subsystems with DMA (aes1+2) were added filling
> the list of dma channels beyond the limit of 32 (even if other
> parameters indicate 96 or 128 channels). This leads to random
> subsystem failures i(e.g. mcbsp for audio) after boot or boot
> messages that DMA can not be initialized.
> 
> Another symptom is that
> 
> /sys/kernel/debug/dmaengine/summary
> 
> has 32 entries and does not show all required channels.
> 
> Fix by disabling unused (on the GTA04 hardware) mcspi1...4.
> Each SPI channel allocates 4 DMA channels rapidly filling
> the available ones.
> 
> Disabling unused SPI modules on the OMAP3 SoC may also save
> some energy (has not been checked).

Applying this into omap-for-v6.4/dt based on what we discussed
in this thread earlier.

Thanks,

Tony