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. > > >> 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 -- 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