Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy translation

[Florian Fainelli <f.fainelli@gmail.com>]

On 2/6/19 2:29 PM, Florian Fainelli wrote:
> On 2/6/19 1:57 PM, Christian Lamparter wrote:
>> On Tuesday, February 5, 2019 11:29:36 PM CET Florian Fainelli wrote:
>>> On 2/5/19 2:12 PM, Christian Lamparter wrote:
>>>> On Tuesday, February 5, 2019 10:29:34 PM CET Andrew Lunn wrote:
>>>>>> For now, I added the DT binding update to the patch as well.
>>>>>> But if this is indeed the way to go, it'll get a separate patch.
>>>>>
>>>>> Hi Christian 
>>>>>
>>>>> You need to be careful with the DT binding. You need to keep backwards
>>>>> compatible with it. An old DT blob needs to keep working. I don't
>>>>> think this is true with this change.
>>>>
>>>> Do you mean because of the 
>>>>
>>>> -               switch0@0 {
>>>> +               switch@10 {
>>>>                         compatible = "qca,qca8337";
>>>>                         #address-cells = <1>;
>>>>                         #size-cells = <0>;
>>>>  
>>>> -                       reg = <0>;
>>>> +                       reg = <0x10>;
>>>>
>>>> change?
>>>>
>>>> or because I removed the phy-handles?>
>>>> The reg = <0x10>; will be necessary regardless. Because this
>>>> is really a bug in the existing binding example and if it is
>>>> copied it will prevent the qca8k driver from loading. 
>>>> This is due to a resource conflict, because there will be 
>>>> already a "phy_port1: phy@0" registered at reg = <0>;
>>>> So this never worked would have worked.
>>>
>>> That part is fine, it is the removal of the phy-handle properties that
>>> is possibly a problem, but in hindsight, I do not believe it will be a
>>> compatibility issue. Lack of "phy-handle" property within the core DSA
>>> layer means: utilize the switch's internal MDIO bus (ds->slave_mii_bus)
>>> instance, which you are not removing, you are just changing how the PHYs
>>> map to port numbers.
>>>
>> Ok, thanks. 
>>
>> I think I'm almost ready for v2. I have fully addressed the compatibility
>> issue by forking off the qca8k_switch_ops depending on whenever a phy-handle
>> property on one of the ports was found or not. If there was no phy-handle the
>> driver adds the slave-bus accessors to the ops which tells DSA to allocate
>> the slave bus and allows the phys can be enumerated. If the phy-handles are
>> found the driver will not have the accessors and DSA will not setup a
>> redundant/fake bus and this prevents the second/double/duplicated discovery
>> and enumeration of the same PHYs again.
> 
> The logic you have sounds a little too broad since it stops as soon as
> one port is found with a 'phy-handle' property and assumes that the
> parent MDIO bus from which qca8k itself is a child device, is the MDIO
> bus to be used. There are possibly 3 cases:
> 
> 1) All ports using internal/build-in PHYs. In that case, you can either
> not specify a 'phy-handle' property and DSA assumes that they are part
> of the switch's internal MDIO bus. You can also specify a 'phy-handle'
> property that references the internal MDIO bus, although then we also
> expect qca8k to register its internal MDIO bus (ala mv88e6xxx)
> 
> 2) Some ports using internal PHYs, some using external PHYs. Similar
> situation again, ports may, or may not specify a 'phy-handle' property,
> so without a 'phy-handle' property that means the port connects to an
> internal PHY, with a 'phy-handle' it could connect to either internal
> PHY or external PHY
> 
> 3) All ports using external PHYs, in that case, we must have a
> 'phy-handle' for each port to specify where and how they connect to
> their external PHYs.
> 
> With respect to your patch, what I would do is register QCA8k's internal
> MDIO bus as a proper mdio bus and use ds->slave_mii_bus as a storage for
> that bus, such that tell the DSA layer: look, here is the internal MDIO
> bus, would you ever find a port that needs to use a PHY in there.
> 
> Then you can still scan each enabled port device, and for each of them,
> populate ds->phys_mii_mask, thus telling DSA exacly which ports are
> using an internal PHY because that would be the ports that do not have a
> 'phy-handle' property. Ports that have a 'phy-handle' property.

Forgot to finish my sentence here. This should read:

Ports that have a 'phy-handle' property will either point to the QCA8k's
internal MDIO bus or an external one, but that will be transparently be
handled during PHY device creation.

Thanks!
-- 
Florian