[PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips

[PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips

[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 GPMC as a data bus.

The actual mapping between the GPMC address space and OMAP2+
address space is made using the GPMC DT "ranges" property.
But also a explicit call to gpmc_cs_request() is needed.

So, this patch allows an ethernet chip to be defined as an
GPMC child node an its chip-select memory address be requested.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Jon,

This patch assumes that we have solved somehow the issue that a
call to request_irq() is needed before before using a GPIO as an
IRQ and this is no longer the case when using from Device Trees.

Anyway, this is independent as how we solve this, whether is
using Jan's patch [1], adding a .request function pointer to
irq_chip as suggested by Stephen [2], or any other approach.

[1]: https://patchwork.kernel.org/patch/2009331/
[2]: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85592.html

 arch/arm/mach-omap2/gpmc.c |   45 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 4fe9ee7..d1bf48b 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -29,6 +29,7 @@
 #include <linux/of.h>
 #include <linux/of_mtd.h>
 #include <linux/of_device.h>
+#include <linux/of_address.h>
 #include <linux/mtd/nand.h>
 
 #include <linux/platform_data/mtd-nand-omap2.h>
@@ -1296,6 +1297,42 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev,
 }
 #endif
 
+static int gpmc_probe_ethernet_child(struct platform_device *pdev,
+				     struct device_node *child)
+{
+	int ret, cs;
+	unsigned long base;
+	struct resource res;
+	struct platform_device *of_dev;
+
+	if (of_property_read_u32(child, "reg", &cs) < 0) {
+		dev_err(&pdev->dev, "%s has no 'reg' property\n",
+			child->full_name);
+		return -ENODEV;
+	}
+
+	if (of_address_to_resource(child, 0, &res)) {
+		dev_err(&pdev->dev, "%s has malformed 'reg' property\n",
+			child->full_name);
+		return -ENODEV;
+	}
+
+	ret = gpmc_cs_request(cs, resource_size(&res), &base);
+	if (IS_ERR_VALUE(ret)) {
+		dev_err(&pdev->dev, "cannot request GPMC CS %d\n", cs);
+		return ret;
+	}
+
+	of_dev = of_platform_device_create(child, NULL, &pdev->dev);
+	if (!of_dev) {
+		dev_err(&pdev->dev, "cannot create platform device for %s\n",
+			child->full_name);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int gpmc_probe_dt(struct platform_device *pdev)
 {
 	int ret;
@@ -1326,6 +1363,14 @@ static int gpmc_probe_dt(struct platform_device *pdev)
 			return ret;
 		}
 	}
+
+	for_each_node_by_name(child, "ethernet") {
+		ret = gpmc_probe_ethernet_child(pdev, child);
+		if (ret < 0) {
+			of_node_put(child);
+			return ret;
+		}
+	}
 	return 0;
 }
 #else
-- 
1.7.7.6

Re: [PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips

[Jon Hunter]

On 03/10/2013 12:18 PM, 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 GPMC as a data bus.
> 
> The actual mapping between the GPMC address space and OMAP2+
> address space is made using the GPMC DT "ranges" property.
> But also a explicit call to gpmc_cs_request() is needed.

One problem with gpmc_cs_request() is that it will map the chip-select
to any physical location in the 1GB address space for the gpmc
controller. So in other words, it will ignore the ranges property
altogether. If you look at my code for NOR, I have added a
gpmc_cs_remap() function to remap the cs to the location as specified by
the device-tree.

Ideally we should change gpmc_cs_request() so we can pass the desire
base address that we want to map the gpmc cs too. I had started out that
way but it made the code some what messy and so I opted to create a
gpmc_cs_remap() function instead. The goal will be to get rid of
gpmc_cs_remap() once DT migration is completed and we can change
gpmc_cs_request() to map the cs to a specific address (see my FIXME
comment).

Your code probably works today because the cs is setup by the bootloader
and so when you request the cs in the kernel the mapping is not changed
from the bootloader settings. However, if the mapping in DT (ranges
property) is different from that setup by the bootloader then the kernel
would probably crash because the kernel would not remap it as expected.

> So, this patch allows an ethernet chip to be defined as an
> GPMC child node an its chip-select memory address be requested.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
> 
> Jon,
> 
> This patch assumes that we have solved somehow the issue that a
> call to request_irq() is needed before before using a GPIO as an
> IRQ and this is no longer the case when using from Device Trees.
> 
> Anyway, this is independent as how we solve this, whether is
> using Jan's patch [1], adding a .request function pointer to
> irq_chip as suggested by Stephen [2], or any other approach.
> 
> [1]: https://patchwork.kernel.org/patch/2009331/
> [2]: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85592.html
> 
>  arch/arm/mach-omap2/gpmc.c |   45 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 45 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 4fe9ee7..d1bf48b 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -29,6 +29,7 @@
>  #include <linux/of.h>
>  #include <linux/of_mtd.h>
>  #include <linux/of_device.h>
> +#include <linux/of_address.h>
>  #include <linux/mtd/nand.h>
>  
>  #include <linux/platform_data/mtd-nand-omap2.h>
> @@ -1296,6 +1297,42 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev,
>  }
>  #endif
>  
> +static int gpmc_probe_ethernet_child(struct platform_device *pdev,
> +				     struct device_node *child)
> +{
> +	int ret, cs;
> +	unsigned long base;
> +	struct resource res;
> +	struct platform_device *of_dev;
> +
> +	if (of_property_read_u32(child, "reg", &cs) < 0) {
> +		dev_err(&pdev->dev, "%s has no 'reg' property\n",
> +			child->full_name);
> +		return -ENODEV;
> +	}
> +
> +	if (of_address_to_resource(child, 0, &res)) {
> +		dev_err(&pdev->dev, "%s has malformed 'reg' property\n",
> +			child->full_name);
> +		return -ENODEV;
> +	}
> +
> +	ret = gpmc_cs_request(cs, resource_size(&res), &base);
> +	if (IS_ERR_VALUE(ret)) {
> +		dev_err(&pdev->dev, "cannot request GPMC CS %d\n", cs);
> +		return ret;
> +	}
> +
> +	of_dev = of_platform_device_create(child, NULL, &pdev->dev);
> +	if (!of_dev) {
> +		dev_err(&pdev->dev, "cannot create platform device for %s\n",
> +			child->full_name);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}

So this function does not setup the cs at all and so that means you are
dependent on having the bootloader configure the cs. I would really like
to get away from that sort of dependency. In fact I am wondering if we
could make the gpmc_probe_nor() function a gpmc_probe_generic() that we
can use for both nor and ethernet as we are doing very similar things
(if we add the timings and gpmc settings to the ethernet binding).

Also I think we need to add some DT binding documentation for this as well.

Cheers
Jon

Re: [PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips

[Javier Martinez Canillas]

On 03/11/2013 06:13 PM, Jon Hunter wrote:
> 

Hi Jon, thanks a lot for your feedback.

> On 03/10/2013 12:18 PM, 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 GPMC as a data bus.
>> 
>> The actual mapping between the GPMC address space and OMAP2+
>> address space is made using the GPMC DT "ranges" property.
>> But also a explicit call to gpmc_cs_request() is needed.
> 
> One problem with gpmc_cs_request() is that it will map the chip-select
> to any physical location in the 1GB address space for the gpmc
> controller. So in other words, it will ignore the ranges property
> altogether. If you look at my code for NOR, I have added a
> gpmc_cs_remap() function to remap the cs to the location as specified by
> the device-tree.
>

I see, thanks for pointing this out.

> Ideally we should change gpmc_cs_request() so we can pass the desire
> base address that we want to map the gpmc cs too. I had started out that
> way but it made the code some what messy and so I opted to create a
> gpmc_cs_remap() function instead. The goal will be to get rid of
> gpmc_cs_remap() once DT migration is completed and we can change
> gpmc_cs_request() to map the cs to a specific address (see my FIXME
> comment).
> 

By looking at gpmc_probe_onenand_child() and gpmc_probe_nand_child() I see that
these functions just allocates platform data and call gpmc_onenand_init() and
gpmc_nand_init() accordingly. So if I understood right these functions have the
same issue and need to call gpmc_cs_remap() too in order to map to the location
specified on the DT.

> Your code probably works today because the cs is setup by the bootloader
> and so when you request the cs in the kernel the mapping is not changed
> from the bootloader settings. However, if the mapping in DT (ranges
> property) is different from that setup by the bootloader then the kernel
> would probably crash because the kernel would not remap it as expected.
> 
>> So, this patch allows an ethernet chip to be defined as an
>> GPMC child node an its chip-select memory address be requested.
>> 
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>> 
>> Jon,
>> 
>> This patch assumes that we have solved somehow the issue that a
>> call to request_irq() is needed before before using a GPIO as an
>> IRQ and this is no longer the case when using from Device Trees.
>> 
>> Anyway, this is independent as how we solve this, whether is
>> using Jan's patch [1], adding a .request function pointer to
>> irq_chip as suggested by Stephen [2], or any other approach.
>> 
>> [1]: https://patchwork.kernel.org/patch/2009331/
>> [2]: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85592.html
>> 
>>  arch/arm/mach-omap2/gpmc.c |   45 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 45 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>> index 4fe9ee7..d1bf48b 100644
>> --- a/arch/arm/mach-omap2/gpmc.c
>> +++ b/arch/arm/mach-omap2/gpmc.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/of.h>
>>  #include <linux/of_mtd.h>
>>  #include <linux/of_device.h>
>> +#include <linux/of_address.h>
>>  #include <linux/mtd/nand.h>
>>  
>>  #include <linux/platform_data/mtd-nand-omap2.h>
>> @@ -1296,6 +1297,42 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev,
>>  }
>>  #endif
>>  
>> +static int gpmc_probe_ethernet_child(struct platform_device *pdev,
>> +				     struct device_node *child)
>> +{
>> +	int ret, cs;
>> +	unsigned long base;
>> +	struct resource res;
>> +	struct platform_device *of_dev;
>> +
>> +	if (of_property_read_u32(child, "reg", &cs) < 0) {
>> +		dev_err(&pdev->dev, "%s has no 'reg' property\n",
>> +			child->full_name);
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (of_address_to_resource(child, 0, &res)) {
>> +		dev_err(&pdev->dev, "%s has malformed 'reg' property\n",
>> +			child->full_name);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = gpmc_cs_request(cs, resource_size(&res), &base);
>> +	if (IS_ERR_VALUE(ret)) {
>> +		dev_err(&pdev->dev, "cannot request GPMC CS %d\n", cs);
>> +		return ret;
>> +	}
>> +
>> +	of_dev = of_platform_device_create(child, NULL, &pdev->dev);
>> +	if (!of_dev) {
>> +		dev_err(&pdev->dev, "cannot create platform device for %s\n",
>> +			child->full_name);
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> So this function does not setup the cs at all and so that means you are
> dependent on having the bootloader configure the cs. I would really like
> to get away from that sort of dependency. In fact I am wondering if we
> could make the gpmc_probe_nor() function a gpmc_probe_generic() that we
> can use for both nor and ethernet as we are doing very similar things
> (if we add the timings and gpmc settings to the ethernet binding).
>

Yes, I also thought about a gmpc_probe_generic() for all GMPC child nodes since
the chip-select setup is the same for all of them.

The problem I saw was that gpmc_probe_{onenand,nand}_child() were just wrappers
around gpmc_{onenand,nand}_init() and since the init functions call
gpmc_cs_request(), I couldn't have a generic probe that call gpmc_cs_request()
for all childs.

But since we should probably have to change this to call gpmc_cs_remap() besides
gpmc_cs_request(), I think is better to not use gpmc_{onenand,nand}_init() at
all and make this somehow generic.

Actually, since the mapping (and the IORESOURCE_MEM struct resource allocation)
is made by the DT core using the "ranges" property. I wonder if we could add
some callback function (e.g: .range_request() ) that can be set by memory
controllers that want to take an action (such as calling gpmc_cs_request() and
gpmc_cs_remap() ) once each element in the "ranges" vector is processed by the
DT core.

> Also I think we need to add some DT binding documentation for this as well.
>

+1

> Cheers
> Jon
> 

Best regards,
Javier

Re: [PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips

[Jon Hunter]

On 03/11/2013 12:57 PM, Javier Martinez Canillas wrote:
> On 03/11/2013 06:13 PM, Jon Hunter wrote:
>>
> 
> Hi Jon, thanks a lot for your feedback.
> 
>> On 03/10/2013 12:18 PM, 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 GPMC as a data bus.
>>>
>>> The actual mapping between the GPMC address space and OMAP2+
>>> address space is made using the GPMC DT "ranges" property.
>>> But also a explicit call to gpmc_cs_request() is needed.
>>
>> One problem with gpmc_cs_request() is that it will map the chip-select
>> to any physical location in the 1GB address space for the gpmc
>> controller. So in other words, it will ignore the ranges property
>> altogether. If you look at my code for NOR, I have added a
>> gpmc_cs_remap() function to remap the cs to the location as specified by
>> the device-tree.
>>
> 
> I see, thanks for pointing this out.
> 
>> Ideally we should change gpmc_cs_request() so we can pass the desire
>> base address that we want to map the gpmc cs too. I had started out that
>> way but it made the code some what messy and so I opted to create a
>> gpmc_cs_remap() function instead. The goal will be to get rid of
>> gpmc_cs_remap() once DT migration is completed and we can change
>> gpmc_cs_request() to map the cs to a specific address (see my FIXME
>> comment).
>>
> 
> By looking at gpmc_probe_onenand_child() and gpmc_probe_nand_child() I see that
> these functions just allocates platform data and call gpmc_onenand_init() and
> gpmc_nand_init() accordingly. So if I understood right these functions have the
> same issue and need to call gpmc_cs_remap() too in order to map to the location
> specified on the DT.

Ideally they should but it is not critical.

So today for NAND and ONENAND the ranges property is completely ignored
(I just came to realise this recently). However, this works because the
address mapped by gpmc_cs_request() is passed to the NAND/ONENAND
drivers via the platform data. However, NOR (and your ethernet patch) we
can't pass via platform data and therefore we must remap.

This needs to be fixed, but it is not critical in terms that it won't
crash. However, I fear your ethernet patch could :-o

>> Your code probably works today because the cs is setup by the bootloader
>> and so when you request the cs in the kernel the mapping is not changed
>> from the bootloader settings. However, if the mapping in DT (ranges
>> property) is different from that setup by the bootloader then the kernel
>> would probably crash because the kernel would not remap it as expected.
>>
>>> So, this patch allows an ethernet chip to be defined as an
>>> GPMC child node an its chip-select memory address be requested.
>>>
>>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>> ---
>>>
>>> Jon,
>>>
>>> This patch assumes that we have solved somehow the issue that a
>>> call to request_irq() is needed before before using a GPIO as an
>>> IRQ and this is no longer the case when using from Device Trees.
>>>
>>> Anyway, this is independent as how we solve this, whether is
>>> using Jan's patch [1], adding a .request function pointer to
>>> irq_chip as suggested by Stephen [2], or any other approach.
>>>
>>> [1]: https://patchwork.kernel.org/patch/2009331/
>>> [2]: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85592.html
>>>
>>>  arch/arm/mach-omap2/gpmc.c |   45 ++++++++++++++++++++++++++++++++++++++++++++
>>>  1 files changed, 45 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>>> index 4fe9ee7..d1bf48b 100644
>>> --- a/arch/arm/mach-omap2/gpmc.c
>>> +++ b/arch/arm/mach-omap2/gpmc.c
>>> @@ -29,6 +29,7 @@
>>>  #include <linux/of.h>
>>>  #include <linux/of_mtd.h>
>>>  #include <linux/of_device.h>
>>> +#include <linux/of_address.h>
>>>  #include <linux/mtd/nand.h>
>>>  
>>>  #include <linux/platform_data/mtd-nand-omap2.h>
>>> @@ -1296,6 +1297,42 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev,
>>>  }
>>>  #endif
>>>  
>>> +static int gpmc_probe_ethernet_child(struct platform_device *pdev,
>>> +				     struct device_node *child)
>>> +{
>>> +	int ret, cs;
>>> +	unsigned long base;
>>> +	struct resource res;
>>> +	struct platform_device *of_dev;
>>> +
>>> +	if (of_property_read_u32(child, "reg", &cs) < 0) {
>>> +		dev_err(&pdev->dev, "%s has no 'reg' property\n",
>>> +			child->full_name);
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	if (of_address_to_resource(child, 0, &res)) {
>>> +		dev_err(&pdev->dev, "%s has malformed 'reg' property\n",
>>> +			child->full_name);
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	ret = gpmc_cs_request(cs, resource_size(&res), &base);
>>> +	if (IS_ERR_VALUE(ret)) {
>>> +		dev_err(&pdev->dev, "cannot request GPMC CS %d\n", cs);
>>> +		return ret;
>>> +	}
>>> +
>>> +	of_dev = of_platform_device_create(child, NULL, &pdev->dev);
>>> +	if (!of_dev) {
>>> +		dev_err(&pdev->dev, "cannot create platform device for %s\n",
>>> +			child->full_name);
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>
>> So this function does not setup the cs at all and so that means you are
>> dependent on having the bootloader configure the cs. I would really like
>> to get away from that sort of dependency. In fact I am wondering if we
>> could make the gpmc_probe_nor() function a gpmc_probe_generic() that we
>> can use for both nor and ethernet as we are doing very similar things
>> (if we add the timings and gpmc settings to the ethernet binding).
>>
> 
> Yes, I also thought about a gmpc_probe_generic() for all GMPC child nodes since
> the chip-select setup is the same for all of them.
> 
> The problem I saw was that gpmc_probe_{onenand,nand}_child() were just wrappers
> around gpmc_{onenand,nand}_init() and since the init functions call
> gpmc_cs_request(), I couldn't have a generic probe that call gpmc_cs_request()
> for all childs.

Yes I was thinking about leaving nand and onenand the way they were but
using probe_generic() for nor and ethernet.

> But since we should probably have to change this to call gpmc_cs_remap() besides
> gpmc_cs_request(), I think is better to not use gpmc_{onenand,nand}_init() at
> all and make this somehow generic.

Yes but may be we could do this longer term and just get ethernet
working for now.

> Actually, since the mapping (and the IORESOURCE_MEM struct resource allocation)
> is made by the DT core using the "ranges" property. I wonder if we could add
> some callback function (e.g: .range_request() ) that can be set by memory
> controllers that want to take an action (such as calling gpmc_cs_request() and
> gpmc_cs_remap() ) once each element in the "ranges" vector is processed by the
> DT core.

Possibly but again I think that should be look at longer term. I think
you are on the right path. Care to see if you can make gpmc_probe_nor
into gpmc_probe_generic and make this work for ethernet too? Leave nand
and onenand as-is for now.

>> Also I think we need to add some DT binding documentation for this as well.
>>
> 
> +1

Thanks
Jon

Re: [PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips

[Javier Martinez Canillas]

On 03/11/2013 07:11 PM, Jon Hunter wrote:
> 
> On 03/11/2013 12:57 PM, Javier Martinez Canillas wrote:
>> On 03/11/2013 06:13 PM, Jon Hunter wrote:
>>>
>> 
>> Hi Jon, thanks a lot for your feedback.
>> 
>>> On 03/10/2013 12:18 PM, 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 GPMC as a data bus.
>>>>
>>>> The actual mapping between the GPMC address space and OMAP2+
>>>> address space is made using the GPMC DT "ranges" property.
>>>> But also a explicit call to gpmc_cs_request() is needed.
>>>
>>> One problem with gpmc_cs_request() is that it will map the chip-select
>>> to any physical location in the 1GB address space for the gpmc
>>> controller. So in other words, it will ignore the ranges property
>>> altogether. If you look at my code for NOR, I have added a
>>> gpmc_cs_remap() function to remap the cs to the location as specified by
>>> the device-tree.
>>>
>> 
>> I see, thanks for pointing this out.
>> 
>>> Ideally we should change gpmc_cs_request() so we can pass the desire
>>> base address that we want to map the gpmc cs too. I had started out that
>>> way but it made the code some what messy and so I opted to create a
>>> gpmc_cs_remap() function instead. The goal will be to get rid of
>>> gpmc_cs_remap() once DT migration is completed and we can change
>>> gpmc_cs_request() to map the cs to a specific address (see my FIXME
>>> comment).
>>>
>> 
>> By looking at gpmc_probe_onenand_child() and gpmc_probe_nand_child() I see that
>> these functions just allocates platform data and call gpmc_onenand_init() and
>> gpmc_nand_init() accordingly. So if I understood right these functions have the
>> same issue and need to call gpmc_cs_remap() too in order to map to the location
>> specified on the DT.
> 
> Ideally they should but it is not critical.
> 
> So today for NAND and ONENAND the ranges property is completely ignored
> (I just came to realise this recently). However, this works because the
> address mapped by gpmc_cs_request() is passed to the NAND/ONENAND
> drivers via the platform data. However, NOR (and your ethernet patch) we
> can't pass via platform data and therefore we must remap.
> 
> This needs to be fixed, but it is not critical in terms that it won't
> crash. However, I fear your ethernet patch could :-o
>
>>> Your code probably works today because the cs is setup by the bootloader
>>> and so when you request the cs in the kernel the mapping is not changed
>>> from the bootloader settings. However, if the mapping in DT (ranges
>>> property) is different from that setup by the bootloader then the kernel
>>> would probably crash because the kernel would not remap it as expected.
>>>
>>>> So, this patch allows an ethernet chip to be defined as an
>>>> GPMC child node an its chip-select memory address be requested.
>>>>
>>>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>>> ---
>>>>
>>>> Jon,
>>>>
>>>> This patch assumes that we have solved somehow the issue that a
>>>> call to request_irq() is needed before before using a GPIO as an
>>>> IRQ and this is no longer the case when using from Device Trees.
>>>>
>>>> Anyway, this is independent as how we solve this, whether is
>>>> using Jan's patch [1], adding a .request function pointer to
>>>> irq_chip as suggested by Stephen [2], or any other approach.
>>>>
>>>> [1]: https://patchwork.kernel.org/patch/2009331/
>>>> [2]: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85592.html
>>>>
>>>>  arch/arm/mach-omap2/gpmc.c |   45 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 files changed, 45 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>>>> index 4fe9ee7..d1bf48b 100644
>>>> --- a/arch/arm/mach-omap2/gpmc.c
>>>> +++ b/arch/arm/mach-omap2/gpmc.c
>>>> @@ -29,6 +29,7 @@
>>>>  #include <linux/of.h>
>>>>  #include <linux/of_mtd.h>
>>>>  #include <linux/of_device.h>
>>>> +#include <linux/of_address.h>
>>>>  #include <linux/mtd/nand.h>
>>>>  
>>>>  #include <linux/platform_data/mtd-nand-omap2.h>
>>>> @@ -1296,6 +1297,42 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev,
>>>>  }
>>>>  #endif
>>>>  
>>>> +static int gpmc_probe_ethernet_child(struct platform_device *pdev,
>>>> +				     struct device_node *child)
>>>> +{
>>>> +	int ret, cs;
>>>> +	unsigned long base;
>>>> +	struct resource res;
>>>> +	struct platform_device *of_dev;
>>>> +
>>>> +	if (of_property_read_u32(child, "reg", &cs) < 0) {
>>>> +		dev_err(&pdev->dev, "%s has no 'reg' property\n",
>>>> +			child->full_name);
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	if (of_address_to_resource(child, 0, &res)) {
>>>> +		dev_err(&pdev->dev, "%s has malformed 'reg' property\n",
>>>> +			child->full_name);
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	ret = gpmc_cs_request(cs, resource_size(&res), &base);
>>>> +	if (IS_ERR_VALUE(ret)) {
>>>> +		dev_err(&pdev->dev, "cannot request GPMC CS %d\n", cs);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	of_dev = of_platform_device_create(child, NULL, &pdev->dev);
>>>> +	if (!of_dev) {
>>>> +		dev_err(&pdev->dev, "cannot create platform device for %s\n",
>>>> +			child->full_name);
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> So this function does not setup the cs at all and so that means you are
>>> dependent on having the bootloader configure the cs. I would really like
>>> to get away from that sort of dependency. In fact I am wondering if we
>>> could make the gpmc_probe_nor() function a gpmc_probe_generic() that we
>>> can use for both nor and ethernet as we are doing very similar things
>>> (if we add the timings and gpmc settings to the ethernet binding).
>>>
>> 
>> Yes, I also thought about a gmpc_probe_generic() for all GMPC child nodes since
>> the chip-select setup is the same for all of them.
>> 
>> The problem I saw was that gpmc_probe_{onenand,nand}_child() were just wrappers
>> around gpmc_{onenand,nand}_init() and since the init functions call
>> gpmc_cs_request(), I couldn't have a generic probe that call gpmc_cs_request()
>> for all childs.
> 
> Yes I was thinking about leaving nand and onenand the way they were but
> using probe_generic() for nor and ethernet.
> 
>> But since we should probably have to change this to call gpmc_cs_remap() besides
>> gpmc_cs_request(), I think is better to not use gpmc_{onenand,nand}_init() at
>> all and make this somehow generic.
> 
> Yes but may be we could do this longer term and just get ethernet
> working for now.
> 
>> Actually, since the mapping (and the IORESOURCE_MEM struct resource allocation)
>> is made by the DT core using the "ranges" property. I wonder if we could add
>> some callback function (e.g: .range_request() ) that can be set by memory
>> controllers that want to take an action (such as calling gpmc_cs_request() and
>> gpmc_cs_remap() ) once each element in the "ranges" vector is processed by the
>> DT core.
> 
> Possibly but again I think that should be look at longer term. I think
> you are on the right path. Care to see if you can make gpmc_probe_nor
> into gpmc_probe_generic and make this work for ethernet too? Leave nand
> and onenand as-is for now.
> 

Yes, you are right, nand and onenand can be fixed later.

I'll do what you suggest later this week and post the patches.

>>> Also I think we need to add some DT binding documentation for this as well.
>>>
>> 
>> +1
> 
> Thanks
> Jon
> 

Thanks a lot and best regards,
Jaiver

Re: [PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips

[Russell King - ARM Linux]

On Sun, Mar 10, 2013 at 06:18:22PM +0100, Javier Martinez Canillas wrote:
> +static int gpmc_probe_ethernet_child(struct platform_device *pdev,
> +				     struct device_node *child)
> +{
> +	int ret, cs;
> +	unsigned long base;
> +	struct resource res;
> +	struct platform_device *of_dev;
> +
> +	if (of_property_read_u32(child, "reg", &cs) < 0) {
> +		dev_err(&pdev->dev, "%s has no 'reg' property\n",
> +			child->full_name);
> +		return -ENODEV;
> +	}
> +
> +	if (of_address_to_resource(child, 0, &res)) {
> +		dev_err(&pdev->dev, "%s has malformed 'reg' property\n",
> +			child->full_name);
> +		return -ENODEV;
> +	}
> +
> +	ret = gpmc_cs_request(cs, resource_size(&res), &base);
> +	if (IS_ERR_VALUE(ret)) {

NAK.  ret < 0 is the correct way to test here.  Don't use IS_ERR_VALUE
unless you have a _very_ good reason to.  That's a good bit of advice
for everything in linux/err.h.  Don't use *anything* from there without
a damned good reason.