Re: [PATCH v5 3/3] arm: dts: ti: Add device tree support for PRU-ICSS on AM335x

[Parvathi Pudi]

Hi,

> Parvathi Pudi <parvathi@xxxxxxxxxxx> writes:
> 
>> Hi,
>>
>>> Parvathi Pudi <parvathi@xxxxxxxxxxx> writes:
>>> 
>>>> From: Roger Quadros <rogerq@xxxxxx>
>>>>
>>>> The TI Sitara AM335x ICE-V2 consists of single PRU-ICSS instance,
>>>> This patch adds the new device tree overlay file in-order to enable
>>>> PRU-ICSS instance, along with makefile changes.
>>>>
>>>> PRU-ICSS instance consists of two PRU cores along with various
>>>> peripherals such as the Interrupt Controller (PRU_INTC), the Industrial
>>>> Ethernet Peripheral(IEP), the Real Time Media Independent Interface
>>>> controller (MII_RT), and the Enhanced Capture (eCAP) event module.
>>>>
>>>> am33xx-l4.dtsi - Adds IEP and eCAP peripheral as child nodes
>>>> of the PRUSS subsystem node.
>>>>
>>>> am335x-icev2-prueth.dtso - Adds PRU-ICSS instance node along with PRU
>>>> eth port information and corresponding port configuration. It includes
>>>> interrupt mapping for packet reception, HW timestamp collection, and PRU
>>>> Ethernet ports in MII mode,
>>>>
>>>> GPIO configuration, boot strapping along with delay configuration for
>>>> individual PRU Ethernet port and other required nodes.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
>>>> Signed-off-by: Andrew F. Davis <afd@xxxxxx>
>>>> Signed-off-by: Murali Karicheri <m-karicheri2@xxxxxx>
>>>> Signed-off-by: Basharath Hussain Khaja <basharath@xxxxxxxxxxx>
>>>> Signed-off-by: Parvathi Pudi <parvathi@xxxxxxxxxxx>
>>> 
>>> [...]
>>> 
>>>> +/*
>>>> + * Disable CPSW switch node and
>>>> + * MDIO configuration to prevent
>>>> + * conflict with PRU-ICSS
>>>> + */
>>>> +&mac_sw {
>>>> +        status = "disable";
>>>> +};
>>>> +
>>>> +&davinci_mdio_sw {
>>>> +        status = "disable";
>>>> +};
>>> 
>>> I think you need s/disable/disabled/?  (note the trailing 'd').  Without
>>> that, I don't think you're disabling these nodes, so I'm curious how it
>>> is not conflicting with the PRU-ICSS.
>>> 
>>> Kevin
>>
>> Thank you for pointing out this typo.
>>
>> We checked the kernel code as to why this did not create any issue in our
>> testing.  We found that the device availability check goes through
>> of_device_is_available(), which only treats "ok" or "okay" as enabled.
>> Anything else is effectively treated as not enabled.
>>
>> So even though "disable" isn't the usual DT value, it still prevents the
>> node from being probed since it doesn't match "ok"/"okay".
> 
> Yes, but since your node is in an overlay, if a previous .dts[i] had set
> this to "ok", then your overlay would not disable it, which would not be
> expected behavior.
> 
>> We will update the value to "disabled" in the next version, since that is
>> the standard.

Sorry for the delayed response.

In our setup, there is no previous .dts where we set this to "ok". We are working
with a combined DTB (base + overlay applied). When we de-compile the final DTB
back to DTS, we do see that the status property is set to "disable" for the node.
At runtime, the kernel’s of_device_is_available() only treats "ok"/"okay" as enabled,
so "disable" results in the node not being probed, which is why we did not observe
any functional issue during testing. 

Are you referring to anything beyond the final merged DT result?

Thanks and Regards,
Parvathi.