EDMA oftree entry for AM335x

EDMA oftree entry for AM335x

[Robert Schwebel]

Hi,

since 5e863c561, I get this warning for an AM335x board:

  omap_hwmod: tptc0 using broken dt data from edma
  omap_hwmod: tptc1 using broken dt data from edma
  omap_hwmod: tptc2 using broken dt data from edma

It seems like the code in arch/arm/mach-omap2/omap_hwmod.c expects the oftree
entry to look differently than what we currently have in
arch/arm/boot/dts/am33xx.dtsi:

  edma: edma at 49000000 {
          compatible = "ti,edma3";
          ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";    <==========
          reg =   <0x49000000 0x10000>,
                  <0x44e10f90 0x40>;
          interrupts = <12 13 14>;
          #dma-cells = <1>;
  };

The patch description says the device should be split into more devices instead
of specifying several ti,hwmods entries, but unfortunately the Documentation
still suggests the above format in Documentation/devicetree/bindings/dma/ti-edma.txt.

How would a correct entry look like?

Thanks,
rsc
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

EDMA oftree entry for AM335x

[Tony Lindgren]

Hi,

Adding Peter to Cc as well.

* Robert Schwebel <r.schwebel@pengutronix.de> [150503 23:44]:
> Hi,
> 
> since 5e863c561, I get this warning for an AM335x board:
> 
>   omap_hwmod: tptc0 using broken dt data from edma
>   omap_hwmod: tptc1 using broken dt data from edma
>   omap_hwmod: tptc2 using broken dt data from edma
> 
> It seems like the code in arch/arm/mach-omap2/omap_hwmod.c expects the oftree
> entry to look differently than what we currently have in
> arch/arm/boot/dts/am33xx.dtsi:
> 
>   edma: edma at 49000000 {
>           compatible = "ti,edma3";
>           ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";    <==========
>           reg =   <0x49000000 0x10000>,
>                   <0x44e10f90 0x40>;
>           interrupts = <12 13 14>;
>           #dma-cells = <1>;
>   };
> 
> The patch description says the device should be split into more devices instead
> of specifying several ti,hwmods entries, but unfortunately the Documentation
> still suggests the above format in Documentation/devicetree/bindings/dma/ti-edma.txt.
> 
> How would a correct entry look like?

Each tptc instance has it's own sysconfig register. This means each
one of them can be clocked and idled independently. They should be
treated as separate device instances. I believe the interrupts all
belong to the tpcc, which should be also treated as a separate
device instance.

In order to get rid of the ti,hwmods property, we want to have a
1-1-1 mapping of the compatible property, ti,hwmod property, and
the device entry.

Looking at the TRM "Table 2-1. L3 Memory Map", these all are on
the L3, so they should be like this for now (assuming the
interrupts all belong to tpcc):

ocp {
	...

	edma: tpcc at 49000000 {
		compatible = "ti,edma3";
		ti,hwmods = "tpcc"; 
		reg = <0x49000000 0x10000>;
		interrupts = <12 13 14>;
		#dma-cells = <1>;
	};

	tptc0: tptc at 49800000 {
		compatible = "ti,tptc3";
		ti,hwmods = "tptc0";
		reg = <0x49800000 0x10000>;
	};

	tptc1: tptc at 49900000 {
		compatible = "ti,tptc3";
		ti,hwmods = "tptc1";
		reg = <0x49900000 0x10000>;
	};

	tptc2: tptc at 49a00000 {
		compatible = "ti,tptc3";
		ti,hwmods = "tptc2";
		reg = <0x49a00000 0x10000>;
	};
	...
};

Then eventually we can deprecate the ti,hwmods property and just
leave it out.

Changing thing this way means each tptc instance should register
itself separately with tpcc when probed. Other than that, I think
that's pretty much the only changes needed here to fix up things.

Regards,

Tony

EDMA oftree entry for AM335x

[Robert Schwebel]

Hi Tony,

On Mon, May 04, 2015 at 07:11:03AM -0700, Tony Lindgren wrote:
> Adding Peter to Cc as well.

You didn't :)
 
> > The patch description says the device should be split into more devices instead
> > of specifying several ti,hwmods entries, but unfortunately the Documentation
> > still suggests the above format in Documentation/devicetree/bindings/dma/ti-edma.txt.
> > 
> > How would a correct entry look like?
> 
> Each tptc instance has it's own sysconfig register. This means each
> one of them can be clocked and idled independently. They should be
> treated as separate device instances. I believe the interrupts all
> belong to the tpcc, which should be also treated as a separate
> device instance.
> 
> In order to get rid of the ti,hwmods property, we want to have a
> 1-1-1 mapping of the compatible property, ti,hwmod property, and
> the device entry.
> 
> Looking at the TRM "Table 2-1. L3 Memory Map", these all are on
> the L3, so they should be like this for now (assuming the
> interrupts all belong to tpcc):
> 
> ocp {
> 	...
> 
> 	edma: tpcc at 49000000 {
> 		compatible = "ti,edma3";
> 		ti,hwmods = "tpcc"; 
> 		reg = <0x49000000 0x10000>;
> 		interrupts = <12 13 14>;
> 		#dma-cells = <1>;
> 	};
> 
> 	tptc0: tptc at 49800000 {
> 		compatible = "ti,tptc3";
> 		ti,hwmods = "tptc0";
> 		reg = <0x49800000 0x10000>;
> 	};
> 
> 	tptc1: tptc at 49900000 {
> 		compatible = "ti,tptc3";
> 		ti,hwmods = "tptc1";
> 		reg = <0x49900000 0x10000>;
> 	};
> 
> 	tptc2: tptc at 49a00000 {
> 		compatible = "ti,tptc3";
> 		ti,hwmods = "tptc2";
> 		reg = <0x49a00000 0x10000>;
> 	};
> 	...
> };

I tried this, with the patch below, but it doesn't work:

----------8<---------- Output with 4.1-rc1 vanilla ----------8<----------

Loading ARM Linux zImage '/mnt/mmc0.1//boot/zImage'
Loading devicetree from '/mnt/mmc0.1//boot/am335x-afi-gf.dtb'
commandline: console=ttyO2,115200n8     root=/dev/mmcblk0p2 rootfstype=ext4 rootwait loglevel=6
[    0.000000] Linux version 4.1.0-rc1 (rsc@callisto) (gcc version 4.8.3 20131111 (prerelease) (OSELAS.Toolchain-2013.12.2 linaro-4.8-2013.11) ) #9 PREEMPT Mon May 4 22:24:41 CEST 2015
[    0.000000] earlycon: no match for ttyO2,115200n8
[    0.000000] Kernel command line: console=ttyO2,115200n8     root=/dev/mmcblk0p2 rootfstype=ext4 rootwait loglevel=6
[    0.000000] Virtual kernel memory layout:
[    0.000000]     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
[    0.000000]     fixmap  : 0xffc00000 - 0xfff00000   (3072 kB)
[    0.000000]     vmalloc : 0xd0800000 - 0xff000000   ( 744 MB)
[    0.000000]     lowmem  : 0xc0000000 - 0xd0000000   ( 256 MB)
[    0.000000]     pkmap   : 0xbfe00000 - 0xc0000000   (   2 MB)
[    0.000000]     modules : 0xbf000000 - 0xbfe00000   (  14 MB)
[    0.000000]       .text : 0xc0008000 - 0xc0674ed4   (6580 kB)
[    0.000000]       .init : 0xc0675000 - 0xc06b2000   ( 244 kB)
[    0.000000]       .data : 0xc06b2000 - 0xc06fd3e8   ( 301 kB)
[    0.000000]        .bss : 0xc0700000 - 0xc073a5d8   ( 234 kB)
[    0.145038] omap_hwmod: tptc0 using broken dt data from edma
[    0.145247] omap_hwmod: tptc1 using broken dt data from edma
[    0.145425] omap_hwmod: tptc2 using broken dt data from edma
[    0.151051] omap_hwmod: debugss: _wait_target_disable failed
[    0.323079] SCSI subsystem initialized
[    0.356218] NFS: Registering the id_resolver key type
[    0.356339] Key type id_resolver registered
[    0.356354] Key type id_legacy registered
[    0.518782] 47401300.usb-phy supply vcc not found, using dummy regulator
[    0.532356] 47401b00.usb-phy supply vcc not found, using dummy regulator
[    0.554370] Key type dns_resolver registered
[    0.559333] omap_voltage_late_init: Voltage driver support not added
[    0.775905] mmc0: host does not support reading read-only switch, assuming write-enable
[    1.140875] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_wait_vrise (80, <SessEnd), retry #3, port1 0008010c
[    1.561977] random: systemd urandom read with 17 bits of entropy available

Welcome to PTXdist / af inventions-GF!

<userspace starts>

----------8<---------- Output with patch ----------8<----------

[    0.000000] Linux version 4.1.0-rc1+ (rsc@callisto) (gcc version 4.8.3 20131111 (prerelease) (OSELAS.Toolchain-2013.12.2 linaro-4.8-2013.11) ) #10 PREEMPT Mon May 4 23:03:27 CEST 2015
[    0.000000] earlycon: no match for ttyO2,115200n8
[    0.000000] Kernel command line: console=ttyO2,115200n8     root=/dev/mmcblk0p2 rootfstype=ext4 rootwait loglevel=6
[    0.000000] Virtual kernel memory layout:
[    0.000000]     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
[    0.000000]     fixmap  : 0xffc00000 - 0xfff00000   (3072 kB)
[    0.000000]     vmalloc : 0xd0800000 - 0xff000000   ( 744 MB)
[    0.000000]     lowmem  : 0xc0000000 - 0xd0000000   ( 256 MB)
[    0.000000]     pkmap   : 0xbfe00000 - 0xc0000000   (   2 MB)
[    0.000000]     modules : 0xbf000000 - 0xbfe00000   (  14 MB)
[    0.000000]       .text : 0xc0008000 - 0xc0674ed4   (6580 kB)
[    0.000000]       .init : 0xc0675000 - 0xc06b2000   ( 244 kB)
[    0.000000]       .data : 0xc06b2000 - 0xc06fd3e8   ( 301 kB)
[    0.000000]        .bss : 0xc0700000 - 0xc073a5d8   ( 234 kB)
[    0.151101] omap_hwmod: debugss: _wait_target_disable failed
[    0.278517] edma 49000000.tpcc: failed to get DT data
[    0.278670] edma: probe of 49000000.tpcc failed with error -12
[    0.312864] SCSI subsystem initialized
[    0.345860] NFS: Registering the id_resolver key type
[    0.345977] Key type id_resolver registered
[    0.345992] Key type id_legacy registered
[    0.493001] spi spi1.0: not using DMA for McSPI
[    0.498211] spi spi1.1: not using DMA for McSPI
[    0.511199] 47401300.usb-phy supply vcc not found, using dummy regulator
[    0.524715] 47401b00.usb-phy supply vcc not found, using dummy regulator
[    0.546556] Key type dns_resolver registered
[    0.551522] omap_voltage_late_init: Voltage driver support not added
[    0.644175] omap_hsmmc 48060000.mmc: unable to obtain RX DMA engine channel 3473832464
[    1.133193] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_wait_vrise (80, <SessEnd), retry #3, port1 0008010c

<hangs>

----------8<---------- patch ----------8<----------

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 21fcc44..34eb437 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -156,15 +156,32 @@
 			reg = <0x48200000 0x1000>;
 		};
 
-		edma: edma at 49000000 {
+		edma: tpcc at 49000000 {
 			compatible = "ti,edma3";
-			ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
-			reg =	<0x49000000 0x10000>,
-				<0x44e10f90 0x40>;
+			ti,hwmods = "tpcc";
+			reg = <0x49000000 0x10000>;
 			interrupts = <12 13 14>;
 			#dma-cells = <1>;
 		};
 
+		tptc0: tptc at 49800000 {
+			compatible = "ti,tptc3";
+			ti,hwmods = "tptc0";
+			reg = <0x49800000 0x10000>;
+		};
+
+		tptc1: tptc at 49900000 {
+			compatible = "ti,tptc3";
+			ti,hwmods = "tptc1";
+			reg = <0x49900000 0x10000>;
+		};
+
+		tptc2: tptc at 49a00000 {
+			compatible = "ti,tptc3";
+			ti,hwmods = "tptc2";
+			reg = <0x49a00000 0x10000>;
+		};
+
 		gpio0: gpio at 44e07000 {
 			compatible = "ti,omap4-gpio";
 			ti,hwmods = "gpio1";

rsc
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

EDMA oftree entry for AM335x

[Tony Lindgren]

* Robert Schwebel <r.schwebel@pengutronix.de> [150504 14:13]:
> Hi Tony,
> 
> On Mon, May 04, 2015 at 07:11:03AM -0700, Tony Lindgren wrote:
> > Adding Peter to Cc as well.
> 
> You didn't :)

Trying again :)
  
> > > The patch description says the device should be split into more devices instead
> > > of specifying several ti,hwmods entries, but unfortunately the Documentation
> > > still suggests the above format in Documentation/devicetree/bindings/dma/ti-edma.txt.
> > > 
> > > How would a correct entry look like?
> > 
> > Each tptc instance has it's own sysconfig register. This means each
> > one of them can be clocked and idled independently. They should be
> > treated as separate device instances. I believe the interrupts all
> > belong to the tpcc, which should be also treated as a separate
> > device instance.
> > 
> > In order to get rid of the ti,hwmods property, we want to have a
> > 1-1-1 mapping of the compatible property, ti,hwmod property, and
> > the device entry.
> > 
> > Looking at the TRM "Table 2-1. L3 Memory Map", these all are on
> > the L3, so they should be like this for now (assuming the
> > interrupts all belong to tpcc):
> > 
> > ocp {
> > 	...
> > 
> > 	edma: tpcc at 49000000 {
> > 		compatible = "ti,edma3";
> > 		ti,hwmods = "tpcc"; 
> > 		reg = <0x49000000 0x10000>;
> > 		interrupts = <12 13 14>;
> > 		#dma-cells = <1>;
> > 	};
> > 
> > 	tptc0: tptc at 49800000 {
> > 		compatible = "ti,tptc3";
> > 		ti,hwmods = "tptc0";
> > 		reg = <0x49800000 0x10000>;
> > 	};
> > 
> > 	tptc1: tptc at 49900000 {
> > 		compatible = "ti,tptc3";
> > 		ti,hwmods = "tptc1";
> > 		reg = <0x49900000 0x10000>;
> > 	};
> > 
> > 	tptc2: tptc at 49a00000 {
> > 		compatible = "ti,tptc3";
> > 		ti,hwmods = "tptc2";
> > 		reg = <0x49a00000 0x10000>;
> > 	};
> > 	...
> > };
> 
> I tried this, with the patch below, but it doesn't work:

With the above the tptc instances are being probed as separate
device entries that do not register with tpcc without the
related driver changes. I don't think we even parse the
compatible = "ti,tptc3" property currently.

Regards,

Tony
 
> ----------8<---------- Output with 4.1-rc1 vanilla ----------8<----------
> 
> Loading ARM Linux zImage '/mnt/mmc0.1//boot/zImage'
> Loading devicetree from '/mnt/mmc0.1//boot/am335x-afi-gf.dtb'
> commandline: console=ttyO2,115200n8     root=/dev/mmcblk0p2 rootfstype=ext4 rootwait loglevel=6
> [    0.000000] Linux version 4.1.0-rc1 (rsc at callisto) (gcc version 4.8.3 20131111 (prerelease) (OSELAS.Toolchain-2013.12.2 linaro-4.8-2013.11) ) #9 PREEMPT Mon May 4 22:24:41 CEST 2015
> [    0.000000] earlycon: no match for ttyO2,115200n8
> [    0.000000] Kernel command line: console=ttyO2,115200n8     root=/dev/mmcblk0p2 rootfstype=ext4 rootwait loglevel=6
> [    0.000000] Virtual kernel memory layout:
> [    0.000000]     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
> [    0.000000]     fixmap  : 0xffc00000 - 0xfff00000   (3072 kB)
> [    0.000000]     vmalloc : 0xd0800000 - 0xff000000   ( 744 MB)
> [    0.000000]     lowmem  : 0xc0000000 - 0xd0000000   ( 256 MB)
> [    0.000000]     pkmap   : 0xbfe00000 - 0xc0000000   (   2 MB)
> [    0.000000]     modules : 0xbf000000 - 0xbfe00000   (  14 MB)
> [    0.000000]       .text : 0xc0008000 - 0xc0674ed4   (6580 kB)
> [    0.000000]       .init : 0xc0675000 - 0xc06b2000   ( 244 kB)
> [    0.000000]       .data : 0xc06b2000 - 0xc06fd3e8   ( 301 kB)
> [    0.000000]        .bss : 0xc0700000 - 0xc073a5d8   ( 234 kB)
> [    0.145038] omap_hwmod: tptc0 using broken dt data from edma
> [    0.145247] omap_hwmod: tptc1 using broken dt data from edma
> [    0.145425] omap_hwmod: tptc2 using broken dt data from edma
> [    0.151051] omap_hwmod: debugss: _wait_target_disable failed
> [    0.323079] SCSI subsystem initialized
> [    0.356218] NFS: Registering the id_resolver key type
> [    0.356339] Key type id_resolver registered
> [    0.356354] Key type id_legacy registered
> [    0.518782] 47401300.usb-phy supply vcc not found, using dummy regulator
> [    0.532356] 47401b00.usb-phy supply vcc not found, using dummy regulator
> [    0.554370] Key type dns_resolver registered
> [    0.559333] omap_voltage_late_init: Voltage driver support not added
> [    0.775905] mmc0: host does not support reading read-only switch, assuming write-enable
> [    1.140875] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_wait_vrise (80, <SessEnd), retry #3, port1 0008010c
> [    1.561977] random: systemd urandom read with 17 bits of entropy available
> 
> Welcome to PTXdist / af inventions-GF!
> 
> <userspace starts>
> 
> ----------8<---------- Output with patch ----------8<----------
> 
> [    0.000000] Linux version 4.1.0-rc1+ (rsc at callisto) (gcc version 4.8.3 20131111 (prerelease) (OSELAS.Toolchain-2013.12.2 linaro-4.8-2013.11) ) #10 PREEMPT Mon May 4 23:03:27 CEST 2015
> [    0.000000] earlycon: no match for ttyO2,115200n8
> [    0.000000] Kernel command line: console=ttyO2,115200n8     root=/dev/mmcblk0p2 rootfstype=ext4 rootwait loglevel=6
> [    0.000000] Virtual kernel memory layout:
> [    0.000000]     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
> [    0.000000]     fixmap  : 0xffc00000 - 0xfff00000   (3072 kB)
> [    0.000000]     vmalloc : 0xd0800000 - 0xff000000   ( 744 MB)
> [    0.000000]     lowmem  : 0xc0000000 - 0xd0000000   ( 256 MB)
> [    0.000000]     pkmap   : 0xbfe00000 - 0xc0000000   (   2 MB)
> [    0.000000]     modules : 0xbf000000 - 0xbfe00000   (  14 MB)
> [    0.000000]       .text : 0xc0008000 - 0xc0674ed4   (6580 kB)
> [    0.000000]       .init : 0xc0675000 - 0xc06b2000   ( 244 kB)
> [    0.000000]       .data : 0xc06b2000 - 0xc06fd3e8   ( 301 kB)
> [    0.000000]        .bss : 0xc0700000 - 0xc073a5d8   ( 234 kB)
> [    0.151101] omap_hwmod: debugss: _wait_target_disable failed
> [    0.278517] edma 49000000.tpcc: failed to get DT data
> [    0.278670] edma: probe of 49000000.tpcc failed with error -12
> [    0.312864] SCSI subsystem initialized
> [    0.345860] NFS: Registering the id_resolver key type
> [    0.345977] Key type id_resolver registered
> [    0.345992] Key type id_legacy registered
> [    0.493001] spi spi1.0: not using DMA for McSPI
> [    0.498211] spi spi1.1: not using DMA for McSPI
> [    0.511199] 47401300.usb-phy supply vcc not found, using dummy regulator
> [    0.524715] 47401b00.usb-phy supply vcc not found, using dummy regulator
> [    0.546556] Key type dns_resolver registered
> [    0.551522] omap_voltage_late_init: Voltage driver support not added
> [    0.644175] omap_hsmmc 48060000.mmc: unable to obtain RX DMA engine channel 3473832464
> [    1.133193] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_wait_vrise (80, <SessEnd), retry #3, port1 0008010c
> 
> <hangs>
> 
> ----------8<---------- patch ----------8<----------
> 
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index 21fcc44..34eb437 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -156,15 +156,32 @@
>  			reg = <0x48200000 0x1000>;
>  		};
>  
> -		edma: edma at 49000000 {
> +		edma: tpcc at 49000000 {
>  			compatible = "ti,edma3";
> -			ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
> -			reg =	<0x49000000 0x10000>,
> -				<0x44e10f90 0x40>;
> +			ti,hwmods = "tpcc";
> +			reg = <0x49000000 0x10000>;
>  			interrupts = <12 13 14>;
>  			#dma-cells = <1>;
>  		};
>  
> +		tptc0: tptc at 49800000 {
> +			compatible = "ti,tptc3";
> +			ti,hwmods = "tptc0";
> +			reg = <0x49800000 0x10000>;
> +		};
> +
> +		tptc1: tptc at 49900000 {
> +			compatible = "ti,tptc3";
> +			ti,hwmods = "tptc1";
> +			reg = <0x49900000 0x10000>;
> +		};
> +
> +		tptc2: tptc at 49a00000 {
> +			compatible = "ti,tptc3";
> +			ti,hwmods = "tptc2";
> +			reg = <0x49a00000 0x10000>;
> +		};
> +
>  		gpio0: gpio at 44e07000 {
>  			compatible = "ti,omap4-gpio";
>  			ti,hwmods = "gpio1";
> 
> rsc
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

EDMA oftree entry for AM335x

[Robert Schwebel]

On Mon, May 04, 2015 at 02:24:57PM -0700, Tony Lindgren wrote:
> > I tried this, with the patch below, but it doesn't work:
> 
> With the above the tptc instances are being probed as separate
> device entries that do not register with tpcc without the
> related driver changes. I don't think we even parse the
> compatible = "ti,tptc3" property currently.

How useful is it to let the driver warn about wrong devicetrees then,
when fixing the devicetrees results in a kernel not supporting it...?

Shouldn't these sort of patches being upstreamed only together with the
full set of changes?

However, thanks for the infos, I'll stay with the warnings for now.

rsc
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

EDMA oftree entry for AM335x

[Tony Lindgren]

* Robert Schwebel <r.schwebel@pengutronix.de> [150504 14:34]:
> On Mon, May 04, 2015 at 02:24:57PM -0700, Tony Lindgren wrote:
> > > I tried this, with the patch below, but it doesn't work:
> > 
> > With the above the tptc instances are being probed as separate
> > device entries that do not register with tpcc without the
> > related driver changes. I don't think we even parse the
> > compatible = "ti,tptc3" property currently.
> 
> How useful is it to let the driver warn about wrong devicetrees then,
> when fixing the devicetrees results in a kernel not supporting it...?

It's there so we can NAK new drivers if somebody tries to sneak
in more stuff like that. Of course it should be fixed for the
existing ones, that's the EDMA and McBSP AFAIK.

> Shouldn't these sort of patches being upstreamed only together with the
> full set of changes?

Ideally these sort of patches never got merged. The right way to fix
this is to first fix the driver, only then do the dts changes.
 
> However, thanks for the infos, I'll stay with the warnings for now.

Yes I figured :)

Regards,

Tony

EDMA oftree entry for AM335x

[Peter Ujfalusi]

On 05/04/2015 05:11 PM, Tony Lindgren wrote:
> Hi,
> 
> Adding Peter to Cc as well.
> 
> * Robert Schwebel <r.schwebel@pengutronix.de> [150503 23:44]:
>> Hi,
>>
>> since 5e863c561, I get this warning for an AM335x board:
>>
>>   omap_hwmod: tptc0 using broken dt data from edma
>>   omap_hwmod: tptc1 using broken dt data from edma
>>   omap_hwmod: tptc2 using broken dt data from edma
>>
>> It seems like the code in arch/arm/mach-omap2/omap_hwmod.c expects the oftree
>> entry to look differently than what we currently have in
>> arch/arm/boot/dts/am33xx.dtsi:
>>
>>   edma: edma at 49000000 {
>>           compatible = "ti,edma3";
>>           ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";    <==========
>>           reg =   <0x49000000 0x10000>,
>>                   <0x44e10f90 0x40>;
>>           interrupts = <12 13 14>;
>>           #dma-cells = <1>;
>>   };
>>
>> The patch description says the device should be split into more devices instead
>> of specifying several ti,hwmods entries, but unfortunately the Documentation
>> still suggests the above format in Documentation/devicetree/bindings/dma/ti-edma.txt.
>>
>> How would a correct entry look like?
> 
> Each tptc instance has it's own sysconfig register. This means each
> one of them can be clocked and idled independently. They should be
> treated as separate device instances. I believe the interrupts all
> belong to the tpcc, which should be also treated as a separate
> device instance.
> 
> In order to get rid of the ti,hwmods property, we want to have a
> 1-1-1 mapping of the compatible property, ti,hwmod property, and
> the device entry.
> 
> Looking at the TRM "Table 2-1. L3 Memory Map", these all are on
> the L3, so they should be like this for now (assuming the
> interrupts all belong to tpcc):
> 
> ocp {
> 	...
> 
> 	edma: tpcc at 49000000 {
> 		compatible = "ti,edma3";
> 		ti,hwmods = "tpcc"; 
> 		reg = <0x49000000 0x10000>;
> 		interrupts = <12 13 14>;
> 		#dma-cells = <1>;
> 	};
> 
> 	tptc0: tptc at 49800000 {
> 		compatible = "ti,tptc3";
> 		ti,hwmods = "tptc0";
> 		reg = <0x49800000 0x10000>;
> 	};
> 
> 	tptc1: tptc at 49900000 {
> 		compatible = "ti,tptc3";
> 		ti,hwmods = "tptc1";
> 		reg = <0x49900000 0x10000>;
> 	};
> 
> 	tptc2: tptc at 49a00000 {
> 		compatible = "ti,tptc3";
> 		ti,hwmods = "tptc2";
> 		reg = <0x49a00000 0x10000>;
> 	};
> 	...
> };

It is not that simple ;)
eDMA consists of Channel Controller(s) and Transfer Controllers. In AM335x we
have 1 CC and 3 TC. TC is mostly a box for us, we do not need to configure
anything within so the eDMA driver stack just does that: do not touch anything
TC related in TC address space, we only deal with the CC.
But in order to be able to move data around we need both CC and the TCs
enabled and this is why we have the multiple hwmod. with pm_runtime we manage
both CC and TCs at the same time.
But from the SW point of view the TC does not really exists. As I said we only
touch CC registers, paRAM entries.

And yes, both TC and CCs can generate interrupts.

So it is not straight forward to separate the TC from the CC driver (edma3).


> 
> Then eventually we can deprecate the ti,hwmods property and just
> leave it out.
> 
> Changing thing this way means each tptc instance should register
> itself separately with tpcc when probed. Other than that, I think
> that's pretty much the only changes needed here to fix up things.
> 
> Regards,
> 
> Tony
> 


-- 
P?ter

EDMA oftree entry for AM335x

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [150505 06:54]:
> On 05/04/2015 05:11 PM, Tony Lindgren wrote:
> > Hi,
> > 
> > Adding Peter to Cc as well.
> > 
> > * Robert Schwebel <r.schwebel@pengutronix.de> [150503 23:44]:
> >> Hi,
> >>
> >> since 5e863c561, I get this warning for an AM335x board:
> >>
> >>   omap_hwmod: tptc0 using broken dt data from edma
> >>   omap_hwmod: tptc1 using broken dt data from edma
> >>   omap_hwmod: tptc2 using broken dt data from edma
> >>
> >> It seems like the code in arch/arm/mach-omap2/omap_hwmod.c expects the oftree
> >> entry to look differently than what we currently have in
> >> arch/arm/boot/dts/am33xx.dtsi:
> >>
> >>   edma: edma at 49000000 {
> >>           compatible = "ti,edma3";
> >>           ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";    <==========
> >>           reg =   <0x49000000 0x10000>,
> >>                   <0x44e10f90 0x40>;
> >>           interrupts = <12 13 14>;
> >>           #dma-cells = <1>;
> >>   };
> >>
> >> The patch description says the device should be split into more devices instead
> >> of specifying several ti,hwmods entries, but unfortunately the Documentation
> >> still suggests the above format in Documentation/devicetree/bindings/dma/ti-edma.txt.
> >>
> >> How would a correct entry look like?
> > 
> > Each tptc instance has it's own sysconfig register. This means each
> > one of them can be clocked and idled independently. They should be
> > treated as separate device instances. I believe the interrupts all
> > belong to the tpcc, which should be also treated as a separate
> > device instance.
> > 
> > In order to get rid of the ti,hwmods property, we want to have a
> > 1-1-1 mapping of the compatible property, ti,hwmod property, and
> > the device entry.
> > 
> > Looking at the TRM "Table 2-1. L3 Memory Map", these all are on
> > the L3, so they should be like this for now (assuming the
> > interrupts all belong to tpcc):
> > 
> > ocp {
> > 	...
> > 
> > 	edma: tpcc at 49000000 {
> > 		compatible = "ti,edma3";
> > 		ti,hwmods = "tpcc"; 
> > 		reg = <0x49000000 0x10000>;
> > 		interrupts = <12 13 14>;
> > 		#dma-cells = <1>;
> > 	};
> > 
> > 	tptc0: tptc at 49800000 {
> > 		compatible = "ti,tptc3";
> > 		ti,hwmods = "tptc0";
> > 		reg = <0x49800000 0x10000>;
> > 	};
> > 
> > 	tptc1: tptc at 49900000 {
> > 		compatible = "ti,tptc3";
> > 		ti,hwmods = "tptc1";
> > 		reg = <0x49900000 0x10000>;
> > 	};
> > 
> > 	tptc2: tptc at 49a00000 {
> > 		compatible = "ti,tptc3";
> > 		ti,hwmods = "tptc2";
> > 		reg = <0x49a00000 0x10000>;
> > 	};
> > 	...
> > };
> 
> It is not that simple ;)
> eDMA consists of Channel Controller(s) and Transfer Controllers. In AM335x we
> have 1 CC and 3 TC. TC is mostly a box for us, we do not need to configure
> anything within so the eDMA driver stack just does that: do not touch anything
> TC related in TC address space, we only deal with the CC.
> But in order to be able to move data around we need both CC and the TCs
> enabled and this is why we have the multiple hwmod. with pm_runtime we manage
> both CC and TCs at the same time.
> But from the SW point of view the TC does not really exists. As I said we only
> touch CC registers, paRAM entries.

There's probably a real reason for each TC to have their own clkctrl
register. Do you have any info on how they are split?
 
> And yes, both TC and CCs can generate interrupts.

Do the TCs have a shared interrupt that could be handled by
each TC instance?
 
> So it is not straight forward to separate the TC from the CC driver (edma3).

It seems pm runtime for these four separate modules needs to be
done at the CC driver level if it can't be done separately for
each instance.

Regards,

Tony

EDMA oftree entry for AM335x

[Peter Ujfalusi]

On 05/05/2015 05:58 PM, Tony Lindgren wrote:
>> It is not that simple ;)
>> eDMA consists of Channel Controller(s) and Transfer Controllers. In AM335x we
>> have 1 CC and 3 TC. TC is mostly a box for us, we do not need to configure
>> anything within so the eDMA driver stack just does that: do not touch anything
>> TC related in TC address space, we only deal with the CC.
>> But in order to be able to move data around we need both CC and the TCs
>> enabled and this is why we have the multiple hwmod. with pm_runtime we manage
>> both CC and TCs at the same time.
>> But from the SW point of view the TC does not really exists. As I said we only
>> touch CC registers, paRAM entries.
> 
> There's probably a real reason for each TC to have their own clkctrl
> register. Do you have any info on how they are split?

In different SoC we can have different number of CCs (OMAP-L138 has two) and
each CC can have different number of TCs associated with. In AM335x we have
one CC with 3 TC, in OMAP-L138 we have two CC, CC0 has 2 TC, CC1 has 1 TC. The
TCs are used to set priorities, you can assign the priorities to the TCs and
you can choose to assign the transfer (channel) to a give TC.

>> And yes, both TC and CCs can generate interrupts.
> 
> Do the TCs have a shared interrupt that could be handled by
> each TC instance?

Separate IRQ lines to the ARM core, but we do not handle them right now.

>> So it is not straight forward to separate the TC from the CC driver (edma3).
> 
> It seems pm runtime for these four separate modules needs to be
> done at the CC driver level if it can't be done separately for
> each instance.

The current eDMA stack is not really flexible on this IMHO.
I think the way forward would be something like this in DT:

edma_cc: edma_cc at 49000000 {
	compatible = "ti,edma3-cc";
	ti,hwmods = "tpcc"
	reg =	<0x49000000 0x10000>,
		<0x44e10f90 0x40>;
	interrupts = <12 13 14>;
	#dma-cells = <1>;

	edma3_tc0: edma3_tc0 at 49800000 {
		compatible = "ti,edma3-tc";
		ti,hwmods = "tptc0"
	};

	edma3_tc1: edma3_tc1 at 49900000 {
		compatible = "ti,edma3-tc";
		ti,hwmods = "tptc1"
	};

	edma3_tc2: edma3_tc2 at 49a00000 {
		compatible = "ti,edma3-tc";
		ti,hwmods = "tptc2"
	};
};

The driver for ti,edma3-tc would be pretty minimal, doing only pm_runtime only
and from the CC driver we could just set the runtime status for the the TC
which we are about to submit work and decrement the runtime when the work is
done. If not work is needed for the TC it can be shot down.

But this would need significant amount of work which can be done when we get
rid of the legacy (arch/arm/common/edma.c) and move to dmaengine only mode.

-- 
P?ter

EDMA oftree entry for AM335x

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [150506 04:23]:
> On 05/05/2015 05:58 PM, Tony Lindgren wrote:
> >> It is not that simple ;)
> >> eDMA consists of Channel Controller(s) and Transfer Controllers. In AM335x we
> >> have 1 CC and 3 TC. TC is mostly a box for us, we do not need to configure
> >> anything within so the eDMA driver stack just does that: do not touch anything
> >> TC related in TC address space, we only deal with the CC.
> >> But in order to be able to move data around we need both CC and the TCs
> >> enabled and this is why we have the multiple hwmod. with pm_runtime we manage
> >> both CC and TCs at the same time.
> >> But from the SW point of view the TC does not really exists. As I said we only
> >> touch CC registers, paRAM entries.
> > 
> > There's probably a real reason for each TC to have their own clkctrl
> > register. Do you have any info on how they are split?
> 
> In different SoC we can have different number of CCs (OMAP-L138 has two) and
> each CC can have different number of TCs associated with. In AM335x we have
> one CC with 3 TC, in OMAP-L138 we have two CC, CC0 has 2 TC, CC1 has 1 TC. The
> TCs are used to set priorities, you can assign the priorities to the TCs and
> you can choose to assign the transfer (channel) to a give TC.

OK
 
> >> And yes, both TC and CCs can generate interrupts.
> > 
> > Do the TCs have a shared interrupt that could be handled by
> > each TC instance?
> 
> Separate IRQ lines to the ARM core, but we do not handle them right now.

OK
 
> >> So it is not straight forward to separate the TC from the CC driver (edma3).
> > 
> > It seems pm runtime for these four separate modules needs to be
> > done at the CC driver level if it can't be done separately for
> > each instance.
> 
> The current eDMA stack is not really flexible on this IMHO.
> I think the way forward would be something like this in DT:
> 
> edma_cc: edma_cc at 49000000 {
> 	compatible = "ti,edma3-cc";
> 	ti,hwmods = "tpcc"
> 	reg =	<0x49000000 0x10000>,
> 		<0x44e10f90 0x40>;
> 	interrupts = <12 13 14>;
> 	#dma-cells = <1>;
> 
> 	edma3_tc0: edma3_tc0 at 49800000 {
> 		compatible = "ti,edma3-tc";
> 		ti,hwmods = "tptc0"
> 	};
> 
> 	edma3_tc1: edma3_tc1 at 49900000 {
> 		compatible = "ti,edma3-tc";
> 		ti,hwmods = "tptc1"
> 	};
> 
> 	edma3_tc2: edma3_tc2 at 49a00000 {
> 		compatible = "ti,edma3-tc";
> 		ti,hwmods = "tptc2"
> 	};
> };

Yeah that would be an improvment and remove the blockers for further
hwmod work. It would still be better to have the TC probe separately
and register with CC rather than combining separate elements in DT
in a way that does not represent the hardare. There's a 7MB reserved
block inbetween there..
 
> The driver for ti,edma3-tc would be pretty minimal, doing only pm_runtime only
> and from the CC driver we could just set the runtime status for the the TC
> which we are about to submit work and decrement the runtime when the work is
> done. If not work is needed for the TC it can be shot down.

OK
 
> But this would need significant amount of work which can be done when we get
> rid of the legacy (arch/arm/common/edma.c) and move to dmaengine only mode.

That may never happen considering that davinci is using it too.. It's
best to not count on that happening anytime soon at least.

Regards,

Tony

EDMA oftree entry for AM335x

[Robert Schwebel]

On Wed, May 06, 2015 at 07:39:52AM -0700, Tony Lindgren wrote:
> > In different SoC we can have different number of CCs (OMAP-L138 has two) and
> > each CC can have different number of TCs associated with. In AM335x we have
> > one CC with 3 TC, in OMAP-L138 we have two CC, CC0 has 2 TC, CC1 has 1 TC. The
> > TCs are used to set priorities, you can assign the priorities to the TCs and
> > you can choose to assign the transfer (channel) to a give TC.
> 
> OK
>  
> > >> And yes, both TC and CCs can generate interrupts.
> > > 
> > > Do the TCs have a shared interrupt that could be handled by
> > > each TC instance?
> > 
> > Separate IRQ lines to the ARM core, but we do not handle them right now.
> 
> OK
>  
> > >> So it is not straight forward to separate the TC from the CC driver (edma3).
> > > 
> > > It seems pm runtime for these four separate modules needs to be
> > > done at the CC driver level if it can't be done separately for
> > > each instance.
> > 
> > The current eDMA stack is not really flexible on this IMHO.
> > I think the way forward would be something like this in DT:
> > 
> > edma_cc: edma_cc at 49000000 {
> > 	compatible = "ti,edma3-cc";
> > 	ti,hwmods = "tpcc"
> > 	reg =	<0x49000000 0x10000>,
> > 		<0x44e10f90 0x40>;
> > 	interrupts = <12 13 14>;
> > 	#dma-cells = <1>;
> > 
> > 	edma3_tc0: edma3_tc0 at 49800000 {
> > 		compatible = "ti,edma3-tc";
> > 		ti,hwmods = "tptc0"
> > 	};
> > 
> > 	edma3_tc1: edma3_tc1 at 49900000 {
> > 		compatible = "ti,edma3-tc";
> > 		ti,hwmods = "tptc1"
> > 	};
> > 
> > 	edma3_tc2: edma3_tc2 at 49a00000 {
> > 		compatible = "ti,edma3-tc";
> > 		ti,hwmods = "tptc2"
> > 	};
> > };
> 
> Yeah that would be an improvment and remove the blockers for further
> hwmod work. It would still be better to have the TC probe separately
> and register with CC rather than combining separate elements in DT
> in a way that does not represent the hardare. There's a 7MB reserved
> block inbetween there..
>  
> > The driver for ti,edma3-tc would be pretty minimal, doing only pm_runtime only
> > and from the CC driver we could just set the runtime status for the the TC
> > which we are about to submit work and decrement the runtime when the work is
> > done. If not work is needed for the TC it can be shot down.
> 
> OK
>  
> > But this would need significant amount of work which can be done when we get
> > rid of the legacy (arch/arm/common/edma.c) and move to dmaengine only mode.
> 
> That may never happen considering that davinci is using it too.. It's
> best to not count on that happening anytime soon at least.

So my current understanding is that we have the situation that the
kernel warns about the oftree being wrong, but isn't able to handle an
oftree that would be right.

Shouldn't the warning being added when the kernel driver supports that
new mechanism?

I had a warning free mainline kernel without patches on my customer
hardware before, so this smells a bit like a regression ... :-/

rsc
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

EDMA oftree entry for AM335x

[Tony Lindgren]

* Robert Schwebel <r.schwebel@pengutronix.de> [150608 04:01]:
> On Wed, May 06, 2015 at 07:39:52AM -0700, Tony Lindgren wrote:
> > 
> > That may never happen considering that davinci is using it too.. It's
> > best to not count on that happening anytime soon at least.
> 
> So my current understanding is that we have the situation that the
> kernel warns about the oftree being wrong, but isn't able to handle an
> oftree that would be right.
>
> Shouldn't the warning being added when the kernel driver supports that
> new mechanism?

This particular driver is broken and it needs to be fixed properly.
 
> I had a warning free mainline kernel without patches on my customer
> hardware before, so this smells a bit like a regression ... :-/

Yes sorry I'm not patching away the warning as it does not fix the
driver. Peter, I assume you are busy fixing it?

Regards,

Tony

EDMA oftree entry for AM335x

[Peter Ujfalusi]

On 06/10/2015 08:10 AM, Tony Lindgren wrote:
> * Robert Schwebel <r.schwebel@pengutronix.de> [150608 04:01]:
>> On Wed, May 06, 2015 at 07:39:52AM -0700, Tony Lindgren wrote:
>>>
>>> That may never happen considering that davinci is using it too.. It's
>>> best to not count on that happening anytime soon at least.
>>
>> So my current understanding is that we have the situation that the
>> kernel warns about the oftree being wrong, but isn't able to handle an
>> oftree that would be right.
>>
>> Shouldn't the warning being added when the kernel driver supports that
>> new mechanism?
> 
> This particular driver is broken and it needs to be fixed properly.
>  
>> I had a warning free mainline kernel without patches on my customer
>> hardware before, so this smells a bit like a regression ... :-/
> 
> Yes sorry I'm not patching away the warning as it does not fix the
> driver. Peter, I assume you are busy fixing it?

Well, I was not.
But I'm now ;)

-- 
P?ter

EDMA oftree entry for AM335x

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [150610 04:18]:
> On 06/10/2015 08:10 AM, Tony Lindgren wrote:
> > * Robert Schwebel <r.schwebel@pengutronix.de> [150608 04:01]:
> >> On Wed, May 06, 2015 at 07:39:52AM -0700, Tony Lindgren wrote:
> >>>
> >>> That may never happen considering that davinci is using it too.. It's
> >>> best to not count on that happening anytime soon at least.
> >>
> >> So my current understanding is that we have the situation that the
> >> kernel warns about the oftree being wrong, but isn't able to handle an
> >> oftree that would be right.
> >>
> >> Shouldn't the warning being added when the kernel driver supports that
> >> new mechanism?
> > 
> > This particular driver is broken and it needs to be fixed properly.
> >  
> >> I had a warning free mainline kernel without patches on my customer
> >> hardware before, so this smells a bit like a regression ... :-/
> > 
> > Yes sorry I'm not patching away the warning as it does not fix the
> > driver. Peter, I assume you are busy fixing it?
> 
> Well, I was not.
> But I'm now ;)

OK good to hear!

Tony

EDMA oftree entry for AM335x

[Peter Ujfalusi]

On 06/11/2015 05:13 PM, Tony Lindgren wrote:
>> Well, I was not.
>> But I'm now ;)
> 
> OK good to hear!

I have a working version now, but...
Due to the scale of the needed changes we would need different compatible
string for the single hwmod version.
The Linux driver itself works fine if I don't change the compatible
(ti,edma3). Boots fine with the old and new DTS file, but the edma sections
are not compatible anymore IMO.

I don't want to rush in a new binding for it since I want to make sure that
the new binding will be usable when I got the chance to rewrite the edma stack
(get rid of the code from arch/arm/common/edma.c)

We have the xbar support embedded in the current edma driver - not the same
crossbar as we have in dra7, it is a bit more problematic (very problematic).
Currently we do not have support for more than one TPCC via DT, legacy boot on
SoC with more than one TPCC is also going to be interesting.

Since we will need new bindings for the edma when we fix the multiple hwmods I
think we should allow us some time to think about it a bit and come up with
something which will work in the future and it is nice to look at it at the
same time ;)

And I will be away for one week...
If you want I can send an RFC/DO NOT MERGE series with the code I have
tomorrow in case you want to see it, but I think it will look quite a bit
different at the end.

-- 
P?ter