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

[Rafał Miłecki]

On 04/19/2017 07:52 PM, Florian Fainelli wrote:
> 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 at 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 at 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.

Sorry, we got a misunderstanding here.

I thought you meant adding something like this for every device:

&mdio {
	status = "okay";

	usb3_phy: usb-phy at 10 {
		compatible = "brcm,ns-ax-usb3-phy";
		reg = <0x10>;
		usb3-dmp-syscon = <&usb3_dmp>;
		#phy-cells = <0>;
	};
};

usb3_dmp: syscon at 18105000 {
	reg = <0x18105000 0x1000>;
};

So I clearly missed something important. Did you want to have USB 3.0 PHY
defined in the dtsi file?