| Message ID | 20230113211151.2314874-1-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: gta04: fix excess dma channel usage | expand |
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
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
Revisiting it again... Am Sun, 22 Jan 2023 10:08:52 +0100 schrieb Andreas Kemnade <andreas@kemnade.info>: > 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 RAM + SCM on, no issue, cross-checked, force-enabling mcspi brings more activity, so this an indication thet mcspi is off. > CM_IDLEST3_CORE 00000000 > CM_FCLKEN1_CORE 00000000 same here with force-enabling mcspi, brings in some bits set. > CM_FCLKEN3_CORE 00000002 see comments below. > CM_CLKSTST_CORE 00000003 > CM_IDLEST_CKGEN 00000209 this becomes 1 without this patch. > 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*. > it does not look like, but it is, but how... There is something below my radar. Regards, Andreas
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>;