[PATCH 0/3] ARM: OMAP2+: Add GPMC DT support for Ethernet child

[PATCH 0/3] ARM: OMAP2+: Add GPMC DT support for Ethernet child

[Javier Martinez Canillas]

Hello Jon,

As discussed before [1], this patch-set adds DT support for ethernet
controllers connected to a TI GPMC by making gpmc_probe_nor_child()
a generic function and reusing it to initialize "ethernet" child
devices nodes too. It also adds documentation about the DT binding.

The patch-set is based on top of your omap-gpmc-for-v3.10 branch [2]
and is composed of the following patches:

[PATCH 1/3] ARM: OMAP2+: return -ENODEV if GPMC child device
[PATCH 2/3] ARM: OMAP2+: rename gpmc_probe_nor_child() to gpmc_probe_generic_child()
[PATCH 3/3] ARM: OMAP2+: Add GPMC DT support for Ethernet child

This was tested on an TI OMAP3 DM3735 based board (IGEPv2)

Thanks a lot and best regards,
Javier

[1]: https://patchwork.kernel.org/patch/2245111/
[2]: https://github.com/jonhunter/linux/tree/omap-gpmc-for-v3.10

[PATCH 1/3] ARM: OMAP2+: return -ENODEV if GPMC child device creation fails

[Javier Martinez Canillas]

gpmc_probe_nor_child() calls of_platform_device_create() to create a
platform device for the NOR child. If this function fails the value
of ret is returned to the caller but this value is zero since it was
assigned the return of a previous call to gpmc_cs_program_settings()
that had to succeed or otherwise gpmc_probe_nor_child() would have
returned before.

This means that if of_platform_device_create() fails, 0 will be returned
to the caller instead of an appropriate error code.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/mach-omap2/gpmc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index d594e1d..8799aed 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1505,6 +1505,7 @@ static int gpmc_probe_nor_child(struct platform_device *pdev,
 		return 0;
 
 	dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
+	ret = -ENODEV;
 
 err:
 	gpmc_cs_free(cs);
-- 
1.7.7.6

Re: [PATCH 1/3] ARM: OMAP2+: return -ENODEV if GPMC child device creation fails

[Jon Hunter]

On 03/14/2013 10:09 AM, Javier Martinez Canillas wrote:
> gpmc_probe_nor_child() calls of_platform_device_create() to create a
> platform device for the NOR child. If this function fails the value
> of ret is returned to the caller but this value is zero since it was
> assigned the return of a previous call to gpmc_cs_program_settings()
> that had to succeed or otherwise gpmc_probe_nor_child() would have
> returned before.
> 
> This means that if of_platform_device_create() fails, 0 will be returned
> to the caller instead of an appropriate error code.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  arch/arm/mach-omap2/gpmc.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index d594e1d..8799aed 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1505,6 +1505,7 @@ static int gpmc_probe_nor_child(struct platform_device *pdev,
>  		return 0;
>  
>  	dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
> +	ret = -ENODEV;
>  
>  err:
>  	gpmc_cs_free(cs);

Good catch! Thanks for the fix, I will pick this up.

Cheers
Jon

[PATCH 2/3] ARM: OMAP2+: rename gpmc_probe_nor_child() to gpmc_probe_generic_child()

[Javier Martinez Canillas]

The gpmc_probe_nor_child() function is used in the GPMC driver to
configure the GPMC for a NOR child device node.

But this function is quite generic and all the NOR specific configuration
is made by the driver of the actual NOR flash memory used.

Other Pseudo-SRAM devices such as ethernet controllers need a similar
setup so by making this function generic it can be used for those too.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/mach-omap2/gpmc.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 8799aed..898b44d 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1439,14 +1439,14 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev,
 #endif
 
 /**
- * gpmc_probe_nor_child - configures the gpmc for a nor device
+ * gpmc_probe_generic_child - configures the gpmc for a child device
  * @pdev:	pointer to gpmc platform device
- * @child:	pointer to device-tree node for nor device
+ * @child:	pointer to device-tree node for child device
  *
- * Allocates and configures a GPMC chip-select for a NOR flash device.
+ * Allocates and configures a GPMC chip-select for a child device.
  * Returns 0 on success and appropriate negative error code on failure.
  */
-static int gpmc_probe_nor_child(struct platform_device *pdev,
+static int gpmc_probe_generic_child(struct platform_device *pdev,
 				struct device_node *child)
 {
 	struct gpmc_settings gpmc_s;
@@ -1552,7 +1552,7 @@ static int gpmc_probe_dt(struct platform_device *pdev)
 	}
 
 	for_each_node_by_name(child, "nor") {
-		ret = gpmc_probe_nor_child(pdev, child);
+		ret = gpmc_probe_generic_child(pdev, child);
 		if (ret < 0) {
 			of_node_put(child);
 			return ret;
-- 
1.7.7.6

Re: [PATCH 2/3] ARM: OMAP2+: rename gpmc_probe_nor_child() to gpmc_probe_generic_child()

[Jon Hunter]

On 03/14/2013 10:09 AM, Javier Martinez Canillas wrote:
> The gpmc_probe_nor_child() function is used in the GPMC driver to
> configure the GPMC for a NOR child device node.
> 
> But this function is quite generic and all the NOR specific configuration
> is made by the driver of the actual NOR flash memory used.
> 
> Other Pseudo-SRAM devices such as ethernet controllers need a similar
> setup so by making this function generic it can be used for those too.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  arch/arm/mach-omap2/gpmc.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 8799aed..898b44d 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1439,14 +1439,14 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev,
>  #endif
>  
>  /**
> - * gpmc_probe_nor_child - configures the gpmc for a nor device
> + * gpmc_probe_generic_child - configures the gpmc for a child device
>   * @pdev:	pointer to gpmc platform device
> - * @child:	pointer to device-tree node for nor device
> + * @child:	pointer to device-tree node for child device
>   *
> - * Allocates and configures a GPMC chip-select for a NOR flash device.
> + * Allocates and configures a GPMC chip-select for a child device.
>   * Returns 0 on success and appropriate negative error code on failure.
>   */
> -static int gpmc_probe_nor_child(struct platform_device *pdev,
> +static int gpmc_probe_generic_child(struct platform_device *pdev,
>  				struct device_node *child)
>  {
>  	struct gpmc_settings gpmc_s;
> @@ -1552,7 +1552,7 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>  	}
>  
>  	for_each_node_by_name(child, "nor") {
> -		ret = gpmc_probe_nor_child(pdev, child);
> +		ret = gpmc_probe_generic_child(pdev, child);
>  		if (ret < 0) {
>  			of_node_put(child);
>  			return ret;
> 

Thanks, looks good. I will pick this up too.

Cheers
Jon

[PATCH 3/3] ARM: OMAP2+: Add GPMC DT support for Ethernet child nodes

[Javier Martinez Canillas]

Besides being used to interface with external memory devices,
the General-Purpose Memory Controller can be used to connect
Pseudo-SRAM devices such as ethernet controllers to OMAP2+
processors using the TI GPMC as a data bus.

This patch allows an ethernet chip to be defined as an GPMC
child device node.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 Documentation/devicetree/bindings/net/gpmc-eth.txt |   90 ++++++++++++++++++++
 arch/arm/mach-omap2/gpmc.c                         |    8 ++
 2 files changed, 98 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/gpmc-eth.txt

diff --git a/Documentation/devicetree/bindings/net/gpmc-eth.txt b/Documentation/devicetree/bindings/net/gpmc-eth.txt
new file mode 100644
index 0000000..c45363c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/gpmc-eth.txt
@@ -0,0 +1,90 @@
+Device tree bindings for Ethernet chip connected to TI GPMC
+
+Besides being used to interface with external memory devices, the
+General-Purpose Memory Controller can be used to connect Pseudo-SRAM devices
+such as ethernet controllers to processors using the TI GPMC as a data bus.
+
+Ethernet controllers connected to TI GPMC are represented as child nodes of
+the GPMC controller with an "ethernet" name.
+
+All timing relevant properties as well as generic GPMC child properties are
+explained in a separate documents. Please refer to
+Documentation/devicetree/bindings/bus/ti-gpmc.txt
+
+Required properties:
+- bank-width: 		Address width of the device in bytes. GPMC supports 8-bit
+			and 16-bit devices and so must be either 1 or 2 bytes.
+- compatible:		Compatible string property for the ethernet child device.
+- gpmc,cs-on:		Chip-select assertion time
+- gpmc,cs-rd-off:	Chip-select de-assertion time for reads
+- gpmc,cs-wr-off:	Chip-select de-assertion time for writes
+- gpmc,oe-on:		Output-enable assertion time
+- gpmc,oe-off		Output-enable de-assertion time
+- gpmc,we-on:		Write-enable assertion time
+- gpmc,we-off:		Write-enable de-assertion time
+- gpmc,access:		Start cycle to first data capture (read access)
+- gpmc,rd-cycle:	Total read cycle time
+- gpmc,wr-cycle:	Total write cycle time
+- reg:			Chip-select, base address (relative to chip-select)
+			and size of the memory mapped for the device.
+			Note that base address will be typically 0 as this
+			is the start of the chip-select.
+
+Optional properties:
+- gpmc,XXX		Additional GPMC timings and settings parameters. See
+			Documentation/devicetree/bindings/bus/ti-gpmc.txt
+
+Optional properties for partiton table parsing:
+- #address-cells: should be set to 1
+- #size-cells: should be set to 1
+
+Example:
+
+gpmc: gpmc@6e000000 {
+	compatible = "ti,omap3430-gpmc";
+	ti,hwmods = "gpmc";
+	reg = <0x6e000000 0x1000>;
+	interrupts = <20>;
+	gpmc,num-cs = <8>;
+	gpmc,num-waitpins = <4>;
+	#address-cells = <2>;
+	#size-cells = <1>;
+
+	ranges = <5 0 0x2c000000 0x1000000>;
+
+	ethernet@5,0 {
+		compatible = "smsc,lan9221", "smsc,lan9115";
+		reg = <5 0 0xff>;
+		bank-width = <2>;
+
+		gpmc,mux-add-data;
+		gpmc,cs-on = <0>;
+		gpmc,cs-rd-off = <186>;
+		gpmc,cs-wr-off = <186>;
+		gpmc,adv-on = <12>;
+		gpmc,adv-rd-off = <48>;
+		gpmc,adv-wr-off = <48>;
+		gpmc,oe-on = <54>;
+		gpmc,oe-off = <168>;
+		gpmc,we-on = <54>;
+		gpmc,we-off = <168>;
+		gpmc,rd-cycle = <186>;
+		gpmc,wr-cycle = <186>;
+		gpmc,access = <114>;
+		gpmc,page-burst-access = <6>;
+		gpmc,bus-turnaround = <12>;
+		gpmc,cycle2cycle-delay = <18>;
+		gpmc,wr-data-mux-bus = <90>;
+		gpmc,wr-access = <186>;
+		gpmc,cycle2cycle-samecsen;
+		gpmc,cycle2cycle-diffcsen;
+
+		interrupt-parent = <&gpio6>;
+		interrupts = <16>;
+		vmmc-supply = <&vddvario>;
+		vmmc_aux-supply = <&vdd33a>;
+		reg-io-width = <2>;
+
+		smsc,save-mac-address;
+	};
+};
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 898b44d..2bc276b 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1559,6 +1559,14 @@ static int gpmc_probe_dt(struct platform_device *pdev)
 		}
 	}
 
+	for_each_node_by_name(child, "ethernet") {
+		ret = gpmc_probe_generic_child(pdev, child);
+		if (ret < 0) {
+			of_node_put(child);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 #else
-- 
1.7.7.6

Re: [PATCH 3/3] ARM: OMAP2+: Add GPMC DT support for Ethernet child nodes

[Jon Hunter]

Hi Javier,

On 03/14/2013 10:09 AM, Javier Martinez Canillas wrote:
> Besides being used to interface with external memory devices,
> the General-Purpose Memory Controller can be used to connect
> Pseudo-SRAM devices such as ethernet controllers to OMAP2+
> processors using the TI GPMC as a data bus.
> 
> This patch allows an ethernet chip to be defined as an GPMC
> child device node.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  Documentation/devicetree/bindings/net/gpmc-eth.txt |   90 ++++++++++++++++++++
>  arch/arm/mach-omap2/gpmc.c                         |    8 ++
>  2 files changed, 98 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/gpmc-eth.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/gpmc-eth.txt b/Documentation/devicetree/bindings/net/gpmc-eth.txt
> new file mode 100644
> index 0000000..c45363c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/gpmc-eth.txt
> @@ -0,0 +1,90 @@
> +Device tree bindings for Ethernet chip connected to TI GPMC
> +
> +Besides being used to interface with external memory devices, the
> +General-Purpose Memory Controller can be used to connect Pseudo-SRAM devices
> +such as ethernet controllers to processors using the TI GPMC as a data bus.
> +
> +Ethernet controllers connected to TI GPMC are represented as child nodes of
> +the GPMC controller with an "ethernet" name.
> +
> +All timing relevant properties as well as generic GPMC child properties are
> +explained in a separate documents. Please refer to
> +Documentation/devicetree/bindings/bus/ti-gpmc.txt
> +
> +Required properties:
> +- bank-width: 		Address width of the device in bytes. GPMC supports 8-bit
> +			and 16-bit devices and so must be either 1 or 2 bytes.

I am wondering if we should use reg-io-width here. The smsc driver is
using this to determine the width of the device. And so I am wondering
if this could cause problems.

Obviously this complicates gpmc_probe_generic_child() a little, but may
be would could pass the name of the width property to
gpmc_probe_generic_child() as well. What do you think?

> +- compatible:		Compatible string property for the ethernet child device.
> +- gpmc,cs-on:		Chip-select assertion time
> +- gpmc,cs-rd-off:	Chip-select de-assertion time for reads
> +- gpmc,cs-wr-off:	Chip-select de-assertion time for writes
> +- gpmc,oe-on:		Output-enable assertion time
> +- gpmc,oe-off		Output-enable de-assertion time
> +- gpmc,we-on:		Write-enable assertion time
> +- gpmc,we-off:		Write-enable de-assertion time
> +- gpmc,access:		Start cycle to first data capture (read access)
> +- gpmc,rd-cycle:	Total read cycle time
> +- gpmc,wr-cycle:	Total write cycle time
> +- reg:			Chip-select, base address (relative to chip-select)
> +			and size of the memory mapped for the device.
> +			Note that base address will be typically 0 as this
> +			is the start of the chip-select.
> +
> +Optional properties:
> +- gpmc,XXX		Additional GPMC timings and settings parameters. See
> +			Documentation/devicetree/bindings/bus/ti-gpmc.txt
> +
> +Optional properties for partiton table parsing:
> +- #address-cells: should be set to 1
> +- #size-cells: should be set to 1
> +
> +Example:
> +
> +gpmc: gpmc@6e000000 {
> +	compatible = "ti,omap3430-gpmc";
> +	ti,hwmods = "gpmc";
> +	reg = <0x6e000000 0x1000>;
> +	interrupts = <20>;
> +	gpmc,num-cs = <8>;
> +	gpmc,num-waitpins = <4>;
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +
> +	ranges = <5 0 0x2c000000 0x1000000>;
> +
> +	ethernet@5,0 {
> +		compatible = "smsc,lan9221", "smsc,lan9115";
> +		reg = <5 0 0xff>;
> +		bank-width = <2>;
> +
> +		gpmc,mux-add-data;
> +		gpmc,cs-on = <0>;
> +		gpmc,cs-rd-off = <186>;
> +		gpmc,cs-wr-off = <186>;
> +		gpmc,adv-on = <12>;
> +		gpmc,adv-rd-off = <48>;
> +		gpmc,adv-wr-off = <48>;
> +		gpmc,oe-on = <54>;
> +		gpmc,oe-off = <168>;
> +		gpmc,we-on = <54>;
> +		gpmc,we-off = <168>;
> +		gpmc,rd-cycle = <186>;
> +		gpmc,wr-cycle = <186>;
> +		gpmc,access = <114>;
> +		gpmc,page-burst-access = <6>;
> +		gpmc,bus-turnaround = <12>;
> +		gpmc,cycle2cycle-delay = <18>;
> +		gpmc,wr-data-mux-bus = <90>;
> +		gpmc,wr-access = <186>;
> +		gpmc,cycle2cycle-samecsen;
> +		gpmc,cycle2cycle-diffcsen;
> +
> +		interrupt-parent = <&gpio6>;
> +		interrupts = <16>;
> +		vmmc-supply = <&vddvario>;
> +		vmmc_aux-supply = <&vdd33a>;
> +		reg-io-width = <2>;
> +
> +		smsc,save-mac-address;
> +	};
> +};
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 898b44d..2bc276b 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1559,6 +1559,14 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>  		}
>  	}
>  
> +	for_each_node_by_name(child, "ethernet") {
> +		ret = gpmc_probe_generic_child(pdev, child);
> +		if (ret < 0) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +	}
> +
>  	return 0;
>  }
>  #else

Otherwise looks good to me. Thanks!

Jon

Re: [PATCH 3/3] ARM: OMAP2+: Add GPMC DT support for Ethernet child nodes

[Javier Martinez Canillas]

On Thu, Mar 14, 2013 at 4:48 PM, Jon Hunter <jon-hunter@ti.com> wrote:
> Hi Javier,
>
> On 03/14/2013 10:09 AM, Javier Martinez Canillas wrote:
>> Besides being used to interface with external memory devices,
>> the General-Purpose Memory Controller can be used to connect
>> Pseudo-SRAM devices such as ethernet controllers to OMAP2+
>> processors using the TI GPMC as a data bus.
>>
>> This patch allows an ethernet chip to be defined as an GPMC
>> child device node.
>>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>>  Documentation/devicetree/bindings/net/gpmc-eth.txt |   90 ++++++++++++++++++++
>>  arch/arm/mach-omap2/gpmc.c                         |    8 ++
>>  2 files changed, 98 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/net/gpmc-eth.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/gpmc-eth.txt b/Documentation/devicetree/bindings/net/gpmc-eth.txt
>> new file mode 100644
>> index 0000000..c45363c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/gpmc-eth.txt
>> @@ -0,0 +1,90 @@
>> +Device tree bindings for Ethernet chip connected to TI GPMC
>> +
>> +Besides being used to interface with external memory devices, the
>> +General-Purpose Memory Controller can be used to connect Pseudo-SRAM devices
>> +such as ethernet controllers to processors using the TI GPMC as a data bus.
>> +
>> +Ethernet controllers connected to TI GPMC are represented as child nodes of
>> +the GPMC controller with an "ethernet" name.
>> +
>> +All timing relevant properties as well as generic GPMC child properties are
>> +explained in a separate documents. Please refer to
>> +Documentation/devicetree/bindings/bus/ti-gpmc.txt
>> +
>> +Required properties:
>> +- bank-width:                Address width of the device in bytes. GPMC supports 8-bit
>> +                     and 16-bit devices and so must be either 1 or 2 bytes.
>
> I am wondering if we should use reg-io-width here. The smsc driver is
> using this to determine the width of the device. And so I am wondering
> if this could cause problems.
>
> Obviously this complicates gpmc_probe_generic_child() a little, but may
> be would could pass the name of the width property to
> gpmc_probe_generic_child() as well. What do you think?
>

Hi Jon,

Well now I'm confused. I thought that both were needed since a
combination of bank-width 16-bit / reg-io-width 32-bit is also
possible (in fact that is how I'm testing on my IGEPv2) and we have
talked about this before [1].

By reading both the OMAP3 TRM and the smsc LAN9221 data-sheet, it
seems that the reg-io-width is not about the data bus address width
but the CPU address width. The smsc data-sheet says:

"The simple, yet highly functional host bus interface provides a
glue-less connection to most common 16-bit microprocessors and
microcontrollers as well as 32-bit microprocessors with a 16-bit
external bus"

By looking at the smsc911x driver
(drivers/net/ethernet/smsc/smsc911x.c) I see that if you use
reg-io-width = <4> (SMSC911X_USE_32BIT) then the smsc911x hardware
registers can be read in one operation and if you use reg-io-width =
<2> (SMSC911X_USE_16BIT) then you need two operations to read the
register:

static inline u32 __smsc911x_reg_read(struct smsc911x_data *pdata, u32 reg)
{
	if (pdata->config.flags & SMSC911X_USE_32BIT)
		return readl(pdata->ioaddr + reg);

	if (pdata->config.flags & SMSC911X_USE_16BIT)
		return ((readw(pdata->ioaddr + reg) & 0xFFFF) |
			((readw(pdata->ioaddr + reg + 2) & 0xFFFF) << 16));

	BUG();
	return 0;
}

Also, by reading at the OMAP3 TRM, I understand that even when the
GPMC has a maximum 16-bit address width, it support devices that has
32-bit registers by doing some sort of access adaptation.

So, as far as I understand the "bank-width" is to configure the GPMC
if is going to use a 8-bit or 16-bit width access and the
"reg-io-width" is how the smsc911x driver is going to read its
registers. So, I think we need both properties and there is no need to
change gpmc_probe_generic_child() neither pass the child name to it.

But to be honest I can be wrong here since data-sheet and technical
reference manuals can be quite confusing sometimes :-)

Thanks a lot for your feedback and best regards,
Javier

[1]: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85597.html

Re: [PATCH 3/3] ARM: OMAP2+: Add GPMC DT support for Ethernet child nodes

[Javier Martinez Canillas]

On Thu, Mar 14, 2013 at 5:37 PM, Javier Martinez Canillas
<martinez.javier@gmail.com> wrote:
> On Thu, Mar 14, 2013 at 4:48 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>> Hi Javier,
>>
>> On 03/14/2013 10:09 AM, Javier Martinez Canillas wrote:
>>> Besides being used to interface with external memory devices,
>>> the General-Purpose Memory Controller can be used to connect
>>> Pseudo-SRAM devices such as ethernet controllers to OMAP2+
>>> processors using the TI GPMC as a data bus.
>>>
>>> This patch allows an ethernet chip to be defined as an GPMC
>>> child device node.
>>>
>>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>> ---
>>>  Documentation/devicetree/bindings/net/gpmc-eth.txt |   90 ++++++++++++++++++++
>>>  arch/arm/mach-omap2/gpmc.c                         |    8 ++
>>>  2 files changed, 98 insertions(+), 0 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/net/gpmc-eth.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/gpmc-eth.txt b/Documentation/devicetree/bindings/net/gpmc-eth.txt
>>> new file mode 100644
>>> index 0000000..c45363c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/gpmc-eth.txt
>>> @@ -0,0 +1,90 @@
>>> +Device tree bindings for Ethernet chip connected to TI GPMC
>>> +
>>> +Besides being used to interface with external memory devices, the
>>> +General-Purpose Memory Controller can be used to connect Pseudo-SRAM devices
>>> +such as ethernet controllers to processors using the TI GPMC as a data bus.
>>> +
>>> +Ethernet controllers connected to TI GPMC are represented as child nodes of
>>> +the GPMC controller with an "ethernet" name.
>>> +
>>> +All timing relevant properties as well as generic GPMC child properties are
>>> +explained in a separate documents. Please refer to
>>> +Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>> +
>>> +Required properties:
>>> +- bank-width:                Address width of the device in bytes. GPMC supports 8-bit
>>> +                     and 16-bit devices and so must be either 1 or 2 bytes.
>>
>> I am wondering if we should use reg-io-width here. The smsc driver is
>> using this to determine the width of the device. And so I am wondering
>> if this could cause problems.
>>
>> Obviously this complicates gpmc_probe_generic_child() a little, but may
>> be would could pass the name of the width property to
>> gpmc_probe_generic_child() as well. What do you think?
>>
>
> Hi Jon,
>
> Well now I'm confused. I thought that both were needed since a
> combination of bank-width 16-bit / reg-io-width 32-bit is also
> possible (in fact that is how I'm testing on my IGEPv2) and we have
> talked about this before [1].
>
> By reading both the OMAP3 TRM and the smsc LAN9221 data-sheet, it
> seems that the reg-io-width is not about the data bus address width
> but the CPU address width. The smsc data-sheet says:
>

Hi Jon,

By the way, here is the smsc LAN9221 data-sheet if you don't have it
and want to take it a look:

http://www.smsc.com/Downloads/SMSC/Downloads_Public/Data_Sheets/9221.pdf

Thanks a lot and best regards,
Javier

Re: [PATCH 3/3] ARM: OMAP2+: Add GPMC DT support for Ethernet child nodes

[Jon Hunter]

On 03/14/2013 11:37 AM, Javier Martinez Canillas wrote:
> On Thu, Mar 14, 2013 at 4:48 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>> Hi Javier,
>>
>> On 03/14/2013 10:09 AM, Javier Martinez Canillas wrote:
>>> Besides being used to interface with external memory devices,
>>> the General-Purpose Memory Controller can be used to connect
>>> Pseudo-SRAM devices such as ethernet controllers to OMAP2+
>>> processors using the TI GPMC as a data bus.
>>>
>>> This patch allows an ethernet chip to be defined as an GPMC
>>> child device node.
>>>
>>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>> ---
>>>  Documentation/devicetree/bindings/net/gpmc-eth.txt |   90 ++++++++++++++++++++
>>>  arch/arm/mach-omap2/gpmc.c                         |    8 ++
>>>  2 files changed, 98 insertions(+), 0 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/net/gpmc-eth.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/gpmc-eth.txt b/Documentation/devicetree/bindings/net/gpmc-eth.txt
>>> new file mode 100644
>>> index 0000000..c45363c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/gpmc-eth.txt
>>> @@ -0,0 +1,90 @@
>>> +Device tree bindings for Ethernet chip connected to TI GPMC
>>> +
>>> +Besides being used to interface with external memory devices, the
>>> +General-Purpose Memory Controller can be used to connect Pseudo-SRAM devices
>>> +such as ethernet controllers to processors using the TI GPMC as a data bus.
>>> +
>>> +Ethernet controllers connected to TI GPMC are represented as child nodes of
>>> +the GPMC controller with an "ethernet" name.
>>> +
>>> +All timing relevant properties as well as generic GPMC child properties are
>>> +explained in a separate documents. Please refer to
>>> +Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>> +
>>> +Required properties:
>>> +- bank-width:                Address width of the device in bytes. GPMC supports 8-bit
>>> +                     and 16-bit devices and so must be either 1 or 2 bytes.
>>
>> I am wondering if we should use reg-io-width here. The smsc driver is
>> using this to determine the width of the device. And so I am wondering
>> if this could cause problems.
>>
>> Obviously this complicates gpmc_probe_generic_child() a little, but may
>> be would could pass the name of the width property to
>> gpmc_probe_generic_child() as well. What do you think?
>>
> 
> Hi Jon,
> 
> Well now I'm confused. I thought that both were needed since a
> combination of bank-width 16-bit / reg-io-width 32-bit is also
> possible (in fact that is how I'm testing on my IGEPv2) and we have
> talked about this before [1].

Yes you are right. Sorry about that ...

> By reading both the OMAP3 TRM and the smsc LAN9221 data-sheet, it
> seems that the reg-io-width is not about the data bus address width
> but the CPU address width. The smsc data-sheet says:
> 
> "The simple, yet highly functional host bus interface provides a
> glue-less connection to most common 16-bit microprocessors and
> microcontrollers as well as 32-bit microprocessors with a 16-bit
> external bus"
> 
> By looking at the smsc911x driver
> (drivers/net/ethernet/smsc/smsc911x.c) I see that if you use
> reg-io-width = <4> (SMSC911X_USE_32BIT) then the smsc911x hardware
> registers can be read in one operation and if you use reg-io-width =
> <2> (SMSC911X_USE_16BIT) then you need two operations to read the
> register:
> 
> static inline u32 __smsc911x_reg_read(struct smsc911x_data *pdata, u32 reg)
> {
> 	if (pdata->config.flags & SMSC911X_USE_32BIT)
> 		return readl(pdata->ioaddr + reg);
> 
> 	if (pdata->config.flags & SMSC911X_USE_16BIT)
> 		return ((readw(pdata->ioaddr + reg) & 0xFFFF) |
> 			((readw(pdata->ioaddr + reg + 2) & 0xFFFF) << 16));
> 
> 	BUG();
> 	return 0;
> }

Looking at the above, then I don't see any issue with this then.

> Also, by reading at the OMAP3 TRM, I understand that even when the
> GPMC has a maximum 16-bit address width, it support devices that has
> 32-bit registers by doing some sort of access adaptation.

Yes I believe that is correct.

> So, as far as I understand the "bank-width" is to configure the GPMC
> if is going to use a 8-bit or 16-bit width access and the
> "reg-io-width" is how the smsc911x driver is going to read its
> registers. So, I think we need both properties and there is no need to
> change gpmc_probe_generic_child() neither pass the child name to it.
> 
> But to be honest I can be wrong here since data-sheet and technical
> reference manuals can be quite confusing sometimes :-)

Ok, so do you think that we should add some documentation to the
gpmc-eth.txt so say that set "reg-io-width = <4>;" for OMAP regardless
of bank-width?

Cheers
Jon

Re: [PATCH 3/3] ARM: OMAP2+: Add GPMC DT support for Ethernet child nodes

[Javier Martinez Canillas]

On Thu, Mar 14, 2013 at 7:49 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 03/14/2013 11:37 AM, Javier Martinez Canillas wrote:
>> On Thu, Mar 14, 2013 at 4:48 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>>> Hi Javier,
>>>
>>> On 03/14/2013 10:09 AM, Javier Martinez Canillas wrote:
>>>> Besides being used to interface with external memory devices,
>>>> the General-Purpose Memory Controller can be used to connect
>>>> Pseudo-SRAM devices such as ethernet controllers to OMAP2+
>>>> processors using the TI GPMC as a data bus.
>>>>
>>>> This patch allows an ethernet chip to be defined as an GPMC
>>>> child device node.
>>>>
>>>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>>> ---
>>>>  Documentation/devicetree/bindings/net/gpmc-eth.txt |   90 ++++++++++++++++++++
>>>>  arch/arm/mach-omap2/gpmc.c                         |    8 ++
>>>>  2 files changed, 98 insertions(+), 0 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/net/gpmc-eth.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/gpmc-eth.txt b/Documentation/devicetree/bindings/net/gpmc-eth.txt
>>>> new file mode 100644
>>>> index 0000000..c45363c
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/gpmc-eth.txt
>>>> @@ -0,0 +1,90 @@
>>>> +Device tree bindings for Ethernet chip connected to TI GPMC
>>>> +
>>>> +Besides being used to interface with external memory devices, the
>>>> +General-Purpose Memory Controller can be used to connect Pseudo-SRAM devices
>>>> +such as ethernet controllers to processors using the TI GPMC as a data bus.
>>>> +
>>>> +Ethernet controllers connected to TI GPMC are represented as child nodes of
>>>> +the GPMC controller with an "ethernet" name.
>>>> +
>>>> +All timing relevant properties as well as generic GPMC child properties are
>>>> +explained in a separate documents. Please refer to
>>>> +Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>> +
>>>> +Required properties:
>>>> +- bank-width:                Address width of the device in bytes. GPMC supports 8-bit
>>>> +                     and 16-bit devices and so must be either 1 or 2 bytes.
>>>
>>> I am wondering if we should use reg-io-width here. The smsc driver is
>>> using this to determine the width of the device. And so I am wondering
>>> if this could cause problems.
>>>
>>> Obviously this complicates gpmc_probe_generic_child() a little, but may
>>> be would could pass the name of the width property to
>>> gpmc_probe_generic_child() as well. What do you think?
>>>
>>
>> Hi Jon,
>>
>> Well now I'm confused. I thought that both were needed since a
>> combination of bank-width 16-bit / reg-io-width 32-bit is also
>> possible (in fact that is how I'm testing on my IGEPv2) and we have
>> talked about this before [1].
>
> Yes you are right. Sorry about that ...
>
>> By reading both the OMAP3 TRM and the smsc LAN9221 data-sheet, it
>> seems that the reg-io-width is not about the data bus address width
>> but the CPU address width. The smsc data-sheet says:
>>
>> "The simple, yet highly functional host bus interface provides a
>> glue-less connection to most common 16-bit microprocessors and
>> microcontrollers as well as 32-bit microprocessors with a 16-bit
>> external bus"
>>
>> By looking at the smsc911x driver
>> (drivers/net/ethernet/smsc/smsc911x.c) I see that if you use
>> reg-io-width = <4> (SMSC911X_USE_32BIT) then the smsc911x hardware
>> registers can be read in one operation and if you use reg-io-width =
>> <2> (SMSC911X_USE_16BIT) then you need two operations to read the
>> register:
>>
>> static inline u32 __smsc911x_reg_read(struct smsc911x_data *pdata, u32 reg)
>> {
>>       if (pdata->config.flags & SMSC911X_USE_32BIT)
>>               return readl(pdata->ioaddr + reg);
>>
>>       if (pdata->config.flags & SMSC911X_USE_16BIT)
>>               return ((readw(pdata->ioaddr + reg) & 0xFFFF) |
>>                       ((readw(pdata->ioaddr + reg + 2) & 0xFFFF) << 16));
>>
>>       BUG();
>>       return 0;
>> }
>
> Looking at the above, then I don't see any issue with this then.
>
>> Also, by reading at the OMAP3 TRM, I understand that even when the
>> GPMC has a maximum 16-bit address width, it support devices that has
>> 32-bit registers by doing some sort of access adaptation.
>
> Yes I believe that is correct.
>
>> So, as far as I understand the "bank-width" is to configure the GPMC
>> if is going to use a 8-bit or 16-bit width access and the
>> "reg-io-width" is how the smsc911x driver is going to read its
>> registers. So, I think we need both properties and there is no need to
>> change gpmc_probe_generic_child() neither pass the child name to it.
>>
>> But to be honest I can be wrong here since data-sheet and technical
>> reference manuals can be quite confusing sometimes :-)
>
> Ok, so do you think that we should add some documentation to the
> gpmc-eth.txt so say that set "reg-io-width = <4>;" for OMAP regardless
> of bank-width?

Yes, I think we can improve the documentation. I'll send a v2 with
this delta patch squashed:

diff --git a/Documentation/devicetree/bindings/net/gpmc-eth.txt
b/Documentation/devicetree/bindings/net/gpmc-eth.txt
index c45363c..711d28d 100644
--- a/Documentation/devicetree/bindings/net/gpmc-eth.txt
+++ b/Documentation/devicetree/bindings/net/gpmc-eth.txt
@@ -11,6 +11,17 @@ All timing relevant properties as well as generic
GPMC child properties are
 explained in a separate documents. Please refer to
 Documentation/devicetree/bindings/bus/ti-gpmc.txt

+For the properties relevant to the ethernet controller connected to the GPMC
+refer to the binding documentation of the device. For example, the
documentation
+for the SMSC 911x is Documentation/devicetree/bindings/net/smsc911x.txt
+
+Child nodes need to specify the GPMC bus address width using the "bank-width"
+property but is possible that an ethernet controller also has a property to
+specify the I/O registers address width. Even when the GPMC has a
maximum 16-bit
+address width, it supports devices with 32-bit word registers.
+For example with an SMSC LAN911x/912x controller connected to the TI GPMC on an
+OMAP2+ board, "bank-width = <2>;" and "reg-io-width = <4>;".
+
 Required properties:
 - bank-width:          Address width of the device in bytes. GPMC
supports 8-bit
                        and 16-bit devices and so must be either 1 or 2 bytes.

>
> Cheers
> Jon

Best regards,
Javier