On 04/19/2017 11:10 AM, Rafał Miłecki wrote:
> 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@xxxxxxxxxx>
>>>>>>>
>>>>>>> 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@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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@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@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@0 {
>>>> reg = <0>;
>>>> ...
>>>> };
>>>> };
>>>>
>>>> versus:
>>>>
>>>> &mdio {
>>>> phy@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@10 {
> compatible = "brcm,ns-ax-usb3-phy";
> reg = <0x10>;
> usb3-dmp-syscon = <&usb3_dmp>;
> #phy-cells = <0>;
> };
> };
Ah no, I would have just done the following in the per-board DTS:
&mdio {
status = "okay";
};
&usb3_phy {
status = "okay";
};
&xhci {
status = "okay";
};
Something like that.
>
> usb3_dmp: syscon@18105000 {
> reg = <0x18105000 0x1000>;
> };
>
> So I clearly missed something important. Did you want to have USB 3.0 PHY
> defined in the dtsi file?
Yes, I think it does make sense to have it defined in the .dtsi file
because it's internal to the SoC, however it should probably be marked
disabled by default, unless a board enables its xHCI controller, does
that make sense?
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
