[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[f.fainelli@gmail.com (Florian Fainelli)]

On 04/19/2017 10:35 AM, Rafa? Mi?ecki wrote:
> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>> On 04/02/2017 02:25 PM, Rafa? Mi?ecki wrote:
>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>> Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
>>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>
>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>> attached.
>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>> yet).
>>>>>
>>>>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>> ---
>>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> @@ -320,6 +320,13 @@
>>>>>          };
>>>>>      };
>>>>>
>>>>> +    mdio at 18003000 {
>>>>> +        compatible = "brcm,iproc-mdio";
>>>>> +        reg = <0x18003000 0x8>;
>>>>> +        #size-cells = <1>;
>>>>> +        #address-cells = <0>;
>>>>> +    };
>>>>
>>>> This looks fine, but usually the block should be enabled on a per-board
>>>> basis, such that there should be a status = "disabled" property here by
>>>> default.
>>>
>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>>> it's
>>> for stuff that is always present on every SoC family board: rng, nand,
>>> spi to
>>> name few.
>>>
>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>> controller so it's enabled by default. Not every board has SPI flash, so
>>> it's
>>> disabled by default.
>>>
>>> It's there and it make sense to me. Is that OK or not?
>>
>> Even though there are devices that are always enabled on a given SoC,
>> because the board designs are always consistent does not necessarily
>> make them good candidates to be enabled at the .dtsi level. This is
>> particularly true when there are external connections to blocks (SPI,
>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>> default is safer as a starting point to begin with.
> 
> In case of Northstar there is USB 3.0 PHY connected *internally* to this
> MDIO.
> I don't think any board manufacturer is able to rip SoC out of the MDIO
> or the
> USB 3.0 PHY.

OK, then can you still resubmit a proper patch that a) puts that
information in the commit message, and b) also adds a proper label to
the mdio node such that it can later on be referenced by label in
board-level DTS files? By that I mean:

mdio: mdio at 18003000 {

Thank you

> 
> 
>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>> MDIO bus
>>> just devices may differ and should not be enabled by default.
>>
>> In which case, the only difference, for you would be to do to, at the
>> board-level DTS:
>>
>> &mdio {
>>     status = "okay";
>>
>>     phy at 0 {
>>         reg = <0>;
>>         ...
>>     };
>> };
>>
>> versus:
>>
>> &mdio {
>>     phy at 0 {
>>         reg = <0>;
>>         ...
>>     };
>> };
>>
>> I think we can afford putting the mdio node's status property in each
>> board-level DTS and make it clear that way that it is enabled because
>> there are child nodes enabled?
> 
> This will be a pretty big effort because every Northstar device I know
> has USB
> 3.0 PHY in the SoC.

Adding a one liner is a "pretty big effort", for sure.

> 
> 
>> NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
>> devices because it relies on child nodes being declared, but you would
>> still get the driver to be probed and enabled, which is a waste of
>> resources at best.
> 
> Right, but DT role is to describe device/board and not really care if
> operating
> system handles that efficiently.


-- 
Florian