diff mbox series

ARM: dts: gta04: fix excess dma channel usage

Message ID 20230113211151.2314874-1-andreas@kemnade.info
State New
Headers show
Series ARM: dts: gta04: fix excess dma channel usage | expand

Commit Message

Andreas Kemnade Jan. 13, 2023, 9:11 p.m. UTC
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(+)

Comments

Adam Ford Jan. 16, 2023, 2:16 p.m. UTC | #1
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?
Tony Lindgren Jan. 16, 2023, 2:51 p.m. UTC | #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
Tony Lindgren Jan. 16, 2023, 4:56 p.m. UTC | #3
* 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
Adam Ford Jan. 16, 2023, 5 p.m. UTC | #4
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
>
Tony Lindgren Jan. 16, 2023, 5:08 p.m. UTC | #5
* 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 Jan. 19, 2023, 7:30 a.m. UTC | #6
* 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
Andreas Kemnade Jan. 22, 2023, 9:08 a.m. UTC | #7
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
diff mbox series

Patch

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>;