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@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@xxxxxxxxxxxxxxx>
>>>> 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@xxxxxxxxxxxxxxx>
>>>> ---
>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
