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

[Javier Martinez Canillas]

On 04/17/2013 08:33 PM, Jon Hunter wrote:
> 
> On 04/17/2013 09:07 AM, Javier Martinez Canillas wrote:
>> On 04/17/2013 03:48 PM, Jon Hunter wrote:
>>>
>>> On 04/17/2013 07:05 AM, Javier Martinez Canillas wrote:
>>>
>>> ...
>>>
>>>> Yes, in fact I just realized that for_each_node_by_name() expand to:
>>>>
>>>> #define for_each_node_by_name(dn, name) \
>>>>         for (dn = of_find_node_by_name(NULL, name); dn; \
>>>>              dn = of_find_node_by_name(dn, name))
>>>>
>>>> which means that it will search for a node with "name" on the complete
>>>> DeviceTree and this is wrong. It should only search on GPMC childs.
>>>
>>> Good catch. I guess we could have flash & ethernet devices connected to
>>> other interfaces such as SPI.
>>>
>> 
>> Yes, in fact this is the case for omap4-sdp.dts which has an ethernet controller
>> connected through SPI:
>> 
>> &mcspi1 {
>>         eth at 0 {
>>                 compatible = "ks8851";
>>                 spi-max-frequency = <24000000>;
>>                 reg = <0>;
>>                 interrupt-parent = <&gpio2>;
>>                 interrupts = <2>; /* gpio line 34 */
>>                 vdd-supply = <&vdd_eth>;
>>         };
>> };
>> 
>> it just didn't fail because the device node name is "eth" and not "ethernet".
> 
> Yes we got lucky here or unlucky as this would have tripped us up
> earlier ;-)
> 

indeed :)

>> which makes me wonder if is OK to rely on a device node name or we should use
>> compatible properties instead such as "ti,gpmc-{eth,nand,nor,onenand}" and do
>> something like:
>> 
>> for_each_compatible_node(pdev->dev.of_node, NULL, "ti,gpmc-eth") and so on.
> 
> Yes I was wondering about that too. However, it does seem simpler to
> just search through the child devices and that would be an easier fix.
> 

agreed

>>>> Could you please test the following patch? If it works for you I'll add a proper
>>>> description and submit it as a PATCH.
>>>>
>>>> Thanks a lot and best regards,
>>>> Javier
>>>>
>>>> From d8dab9ae9a0284f17553875c2fddd806d9f6ab2e Mon Sep 17 00:00:00 2001
>>>> From: Javier Martinez Canillas <javier.martinez at collabora.co.uk>
>>>> Date: Wed, 17 Apr 2013 13:50:30 +0200
>>>> Subject: [PATCH] ARM: OMAP2+: only search for GPMC DT child nodes on
>>>>  probe
>>>>
>>>> Signed-off-by: Javier Martinez Canillas <javier.martinez at collabora.co.uk>
>>>> ---
>>>>  arch/arm/mach-omap2/gpmc.c |   51 ++++++++++++++++++++-----------------------
>>>>  1 files changed, 24 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>>>> index ed946df..58e2415 100644
>>>> --- a/arch/arm/mach-omap2/gpmc.c
>>>> +++ b/arch/arm/mach-omap2/gpmc.c
>>>> @@ -1520,35 +1520,32 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>>>>  		return ret;
>>>>  	}
>>>>
>>>> -	for_each_node_by_name(child, "nand") {
>>>> -		ret = gpmc_probe_nand_child(pdev, child);
>>>> -		if (ret < 0) {
>>>> -			of_node_put(child);
>>>> -			return ret;
>>>> -		}
>>>> -	}
>>>> -
>>>> -	for_each_node_by_name(child, "onenand") {
>>>> -		ret = gpmc_probe_onenand_child(pdev, child);
>>>> -		if (ret < 0) {
>>>> -			of_node_put(child);
>>>> -			return ret;
>>>> -		}
>>>> -	}
>>>> +	for_each_child_of_node(pdev->dev.of_node, child) {
>>>> +		if (child->name) {
>>>
>>> Minor nit ... how about ...
>>>
>>> +		if (!child->name)
>>> 			continue;
>>>
>> 
>> Yes, much better. I just cooked a quick patch so Lars could test it ;-)
>>  		
>>>> +			if (of_node_cmp(child->name, "nand") == 0) {
>>>> +				ret = gpmc_probe_nand_child(pdev, child);
>>>> +				if (ret < 0) {
>>>> +					of_node_put(child);
>>>> +					return ret;
>>>> +				}
>>>> +			}
>>>>
>>>> -	for_each_node_by_name(child, "nor") {
>>>> -		ret = gpmc_probe_generic_child(pdev, child);
>>>> -		if (ret < 0) {
>>>> -			of_node_put(child);
>>>> -			return ret;
>>>> -		}
>>>> -	}
>>>> +			if (of_node_cmp(child->name, "onenand") == 0) {
>>>> +				ret = gpmc_probe_onenand_child(pdev, child);
>>>> +				if (ret < 0) {
>>>> +					of_node_put(child);
>>>> +					return ret;
>>>> +				}
>>>> +			}
>>>>
>>>> -	for_each_node_by_name(child, "ethernet") {
>>>> -		ret = gpmc_probe_generic_child(pdev, child);
>>>> -		if (ret < 0) {
>>>> -			of_node_put(child);
>>>> -			return ret;
>>>> +			if (of_node_cmp(child->name, "ethernet") == 0 ||
>>>> +			    of_node_cmp(child->name, "nor") == 0) {
>>>> +				ret = gpmc_probe_generic_child(pdev, child);
>>>> +				if (ret < 0) {
>>>> +					of_node_put(child);
>>>> +					return ret;
>>>> +				}
>>>> +			}
>>>>  		}
>>>>  	}
>>>
>>> I am also wondering if the probe should fail if one of its children
>>> fails. The panic that Lars reported occurred because the nand
>>> successfully probed and so the nand device was registered, but because
>>> the ethernet device probe failed, the gpmc probe fails, and then when
>>> the nand device is probed by the mtd driver the kernel panics. I wonder
>>> if it would be better to WARN on child devices that fail to probe but
>>> not return error from the gpmc probe.
>>>
>> 
>> I wonder the same. Probably is better to just WARN so a user could at lest use
>> some of the peripherals connected to the GPMC. This is specially important for
>> flash memory devices since the rootfs could have to be mounted from there.
>> 
>> Do you want me to split the child node search and the WARN_ON() replacement in
>> two different patchs or should I send both changes in one patch?
> 
> Probably best to make these two separate patches as I think that will be
> clearer.
>

OK, I had already sent it as one patch as "[PATCH] ARM: OMAP2+: only search for
GPMC DT child nodes on probe" but I'll split it and send as two different patches.

> Cheers
> Jon
> 

Thanks a lot and best regards,
Javier