RE: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up

["Vicenţiu Galanopulo" <vicentiu.galanopulo@nxp.com>]

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Tuesday, March 27, 2018 1:45 AM
> To: Vicenţiu Galanopulo <vicentiu.galanopulo@nxp.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; robh+dt@kernel.org;
> mark.rutland@arm.com; davem@davemloft.net; marcel@holtmann.org;
> devicetree@vger.kernel.org
> Cc: Madalin-cristian Bucur <madalin.bucur@nxp.com>; Alexandru Marginean
> <alexandru.marginean@nxp.com>
> Subject: Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr
> and dev-addr code check-up
> 
> On 03/23/2018 08:05 AM, Vicentiu Galanopulo wrote:
> > Reason for this patch is that the Inphi PHY has a vendor specific
> > address space for accessing the
> > C45 MDIO registers - starting from 0x1e.
> >
> > A search of the dev-addr property is done in of_mdiobus_register.
> > If the property is found in the PHY node,
> > of_mdiobus_register_static_phy is called. This is a wrapper function
> > for of_mdiobus_register_phy which finds the device in package based on
> > dev-addr and fills devices_addrs:
> > devices_addrs is a new field added to phy_c45_device_ids.
> > This new field will store the dev-addr property on the same index
> > where the device in package has been found.
> > In order to have dev-addr in get_phy_c45_ids(), mdio_c45_ids is passed
> > from of_mdio.c to phy_device.c as an external variable.
> > In get_phy_device a copy of the mdio_c45_ids is done over the local
> > c45_ids (wich are empty). After the copying, the c45_ids will also
> > contain the static device found from dev-addr.
> > Having dev-addr stored in devices_addrs, in get_phy_c45_ids(), when
> > probing the identifiers, dev-addr can be extracted from devices_addrs
> > and probed if devices_addrs[current_identifier] is not 0.
> > This way changing the kernel API is avoided completely.
> >
> > As a plus to this patch, num_ids in get_phy_c45_ids, has the value 8
> > (ARRAY_SIZE(c45_ids->device_ids)),
> > but the u32 *devs can store 32 devices in the bitfield.
> > If a device is stored in *devs, in bits 32 to 9, it will not be found.
> > This is the reason for changing in phy.h, the size of device_ids
> > array.
> >
> > Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
> > ---
> 
> Correct me if I am completely misunderstanding  the problem, but have you
> considered doing the following:
> 
> - if all you need to is to replace instances of loops that do:
> 
>         if (phydev->is_c45) {
>                 for (i = 1; i < num_ids; i++) {
>                         if (!(phydev->c45_ids.devices_in_package & (1 <<
> i)))
>                                 continue;
> 
> with one that starts at dev-addr, as specified by Device Tree, then I suspect
> there is an easier way to do what you want rather than your fairly intrusive
> patch to do that:
> 


Sorry for the lengthy comment and sorry if this is redundant, I'm just trying to explain 
best that I can my patch: 
The loop goes through the devices_in_packages, and where it finds a bit that is set, it
continues to get the PHY device ID.
But, to have devices_in_package with those bits set, a previous querying of the 
MDIO_DEVS2 and MDIO_DEVS1 is necessary. And in the MDIO bus query,
dev-addr, from the device tree,  is part of the whole register address:
reg_addr = MII_ADDR_C45 | dev_addr << 16 | MDIO_DEVS2; 

Andrew suggested in his first comment that the device tree lookup could be done
in of_mdiobus_register(),  probably because of the loop which already checks the
<reg> property of the PHY.
My understanding of his comments was that I could propagate, as a parameter,
dev-addr, down the hierarchy call of_mdiobus_register_phy()-> get_phy_device()->
get_phy_id()->get_phy_c45_ids()->get_phy_c45_devs_in_pkg(dev_addr param existed here).
This is where the loop you're mentioning needed some altering, because the loop index is
actually the dev_addr parameter from get_phy_c45_devs_in_pkg(), and it's from 1 to 7 (num_ids = 8)

This worked for the other PHYs which had dev-addr in this range, but it doesn't work for the INPHI,
which has dev_add = 30 (0x1e).
After I did the extension of the  device_ids from 8 to 32 to match 
the devs (u32 *devs = &c45_ids->devices_in_package;)  in get_phy_c45_ids() :
 -	u32 device_ids[8];
 +	u32 device_ids[32];

I realized that dev_addr for other PHY vendors could be larger than 31 (just a coincidence that 
for INPHI is 30 and it fits), and that dev-addr should be a separate parameter, that still 
has to match the bit position from *devs (&c45_ids->devices_in_package) 

So, I didn't had to change the loop to start from dev-addr, just to let it check if the bit is set in *devs (as before), 
and if that bit corresponds to the INPHI PHY (dev-addr has been set in the PHY device tree node), use
dev-addr in getting the PHY ID.

The loop start index has to remain the same because other PHY vendors have dev-addr 1 to 7, and 
PHY discovering succeeds.

For other issues that I had with the above solution (plus Andrew's comment about the SMP), I uploaded 
a v3... probably slightly before you made this comment. Please have look at it, and paste the below 
comments if they still apply. I was not able to match them with my latest patch...

Thanks,
Vicentiu

> - patch of_mdiobus_register_phy() to lookup both the c45 compatible string as
> well as fetch the "dev-addr" property
> 
> - provide a PHY Device Tree node that has its OUI as a compatible string (see
> of_get_phy_id() for details), or if it has a specified 'dev-addr'
> property, use that in lieu of the default get_phy_device() logic
> 
> - pass both to phy_device_create() and eventually introduce a helper function
> that lets you populate the c45_ids structure
> 
> Then for each function that does the loop above, as long as you have a phydev
> reference, you can have phydev->dev_addr = 0x1e be where to start from, if it is
> 0, then start at 1 (like it currently is). If you don't have a phy device reference,
> which would be get_phy_c45_ids() then just make sure you don't call that
> function :)
> 
> >  struct phy_c45_device_ids {
> >  	u32 devices_in_package;
> > -	u32 device_ids[8];
> > +	u32 device_ids[32];
> > +	u32 devices_addrs[32];
> >  };
> 
> This looks like a fix in itself, so it is worth a separate patch.
> --
> Florian