> -----Original Message----- > From: Florian Fainelli [mailto:f.fainelli@xxxxxxxxx] > Sent: Tuesday, March 27, 2018 1:45 AM > To: Vicenţiu Galanopulo <vicentiu.galanopulo@xxxxxxx>; > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; > mark.rutland@xxxxxxx; davem@xxxxxxxxxxxxx; marcel@xxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx > Cc: Madalin-cristian Bucur <madalin.bucur@xxxxxxx>; Alexandru Marginean > <alexandru.marginean@xxxxxxx> > 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@xxxxxxx> > > --- > > 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 ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
