Re: [PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips
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
---
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
#include
#include
+#include
#include
#include
@@ -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);
>>
Re: [PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips
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
>>> ---
>>>
>>> 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
>>> #include
>>> #include
>>> +#include
>>> #include
>>>
>>> #include
>>> @@ -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
Re: [PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips
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
>> ---
>>
>> 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
>> #include
>> #include
>> +#include
>> #include
>>
>> #include
>> @@ -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
Re: [PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips
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
> ---
>
> 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
> #include
> #include
> +#include
> #include
>
> #include
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] ARM: OMAP: gpmc: request CS address space for ethernet chips
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
---
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
#include
#include
+#include
#include
#include
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
