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

[Andrew Lunn]

> If this is a rare case and something future devices should get right,
> then I'm more inclined to use 'dev-addr' rather than extending reg.

Hi Rob

The sample size is a bit small at the moment to know how rare it is.
I think we have 6 C45 devices so far, and two get this wrong. C45 is
mostly used for PHY device with > 1Gbps. There are not so many of
these yet.

I would also prefer to use 'dev-addr'.

  Andrew

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

[Rob Herring]

On Tue, Mar 27, 2018 at 9:24 AM, Andrew Lunn  wrote:
>> > This is a 2nd MDIO address, right? Can't you just append this to reg 
>> > property?
>
> Hi Rob
>
> It is a sub address.
>
> There are two different MDIO addressing schemes. Clause 22 allowed for
> 32 different addresses on an MDIO bus. Clause 45 extended that. You
> have the existing 32 addresses for a package. However, within a
> package, you can have 32 devices.

Sounds similar to functions in PCI land (which are part of reg address).

> You are supposed to be able to look are registers inside the package,
> and it will tell you which devices in the packages are in use. You can
> then look at those devices and figure out what they are using ID
> registers.
>
> However some vendors get this wrong, they don't fill in the devices in
> package information. So the generic probe code never finds them. We
> need to pass it a hint, go looking at this specific device in the
> package.

If this is a rare case and something future devices should get right,
then I'm more inclined to use 'dev-addr' rather than extending reg.

> You can mix Clause 22 and Clause 45 on the same bus. Does DT allow two
> different reg formats to be used at once? Can we have some reg
> properties with a single value, and some with two values? I thought
> #address-cells = <1> means there should be a single address in reg.

#address-cells is how many cells (aka u32) it takes to store an
address, not how many addresses you have. The number of addresses is
(sizeof(reg) / 4) / (#address-cells + #size-cells). So yes, you can
have different number of addresses for each device, but you can't have
different sizes of addresses (e.g. 32-bit and 64-bit) in one bus. But
I think in this case it would logically be 1 address with 2 cells
because it is the port and device together that make up the address.
If you did that, you'd have to define how to express a clause 22
device with 2 cells. You could either set a high bit in the first cell
to indicate clause 45 address or use an illegal device address in the
2nd cell.

However, the MDIO core would need to handle 2 address cells. If more
than 1 address cell is an error currently, that causes a compatibility
problem with new dtb and older kernels (but could be addressed with
stable updates).

Rob

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

[Andrew Lunn]

> > This is a 2nd MDIO address, right? Can't you just append this to reg 
> > property?

Hi Rob

It is a sub address.

There are two different MDIO addressing schemes. Clause 22 allowed for
32 different addresses on an MDIO bus. Clause 45 extended that. You
have the existing 32 addresses for a package. However, within a
package, you can have 32 devices.

You are supposed to be able to look are registers inside the package,
and it will tell you which devices in the packages are in use. You can
then look at those devices and figure out what they are using ID
registers.

However some vendors get this wrong, they don't fill in the devices in
package information. So the generic probe code never finds them. We
need to pass it a hint, go looking at this specific device in the
package.

You can mix Clause 22 and Clause 45 on the same bus. Does DT allow two
different reg formats to be used at once? Can we have some reg
properties with a single value, and some with two values? I thought
#address-cells = <1> means there should be a single address in reg.

Andrew

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

[Vicenţiu Galanopulo]

> -Original Message-
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: Tuesday, March 27, 2018 1:45 AM
> To: Vicenţiu Galanopulo <vicentiu.galanop...@nxp.com>;
> netdev@vger.kernel.org; linux-ker...@vger.kernel.org; robh...@kernel.org;
> mark.rutl...@arm.com; da...@davemloft.net; mar...@holtmann.org;
> devicet...@vger.kernel.org
> Cc: Madalin-cristian Bucur <madalin.bu...@nxp.com>; Alexandru Marginean
> <alexandru.margin...@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.galanop...@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
 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 = _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 (_ids->devices_in_package) 

So, I didn't had to change the loop to start from dev-addr, just to 

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

[Vicenţiu Galanopulo]

> -Original Message-
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: Tuesday, March 27, 2018 1:25 AM
> To: Vicenţiu Galanopulo <vicentiu.galanop...@nxp.com>
> Cc: netdev@vger.kernel.org; linux-ker...@vger.kernel.org;
> mark.rutl...@arm.com; da...@davemloft.net; mar...@holtmann.org;
> devicet...@vger.kernel.org; Madalin-cristian Bucur <madalin.bu...@nxp.com>;
> Alexandru Marginean <alexandru.margin...@nxp.com>
> Subject: Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr
> and dev-addr code check-up
> 
> On Fri, Mar 23, 2018 at 10:05:22AM -0500, 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.galanop...@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/net/phy.txt |  6 ++
> 
> Please split bindings to separate patch.

 
Thanks Rob for your comments. I was considering doing that after I reach a more 
stable, non-RFC version of the patch. I also added a v3, before
your comments, I think... so in the next one, v4, I will split the binding to a 
separate patch.


> 
> >  drivers/net/phy/phy_device.c  | 22 +-
> >  drivers/of/of_mdio.c  | 98 
> > ++-
> >  include/linux/phy.h   |  5 +-
> >  4 files changed, 125 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/phy.txt
> > b/Documentation/devicetree/bindings/net/phy.txt
> > index d2169a5..82692e2 100644
> > --- a/Documentation/devicetree/bindings/net/phy.txt
> > +++ b/Documentation/devicetree/bindings/net/phy.txt
> > @@ -61,6 +61,11 @@ Optional Properties:
> >  - reset-deassert-us: Delay after the reset was deasserted in microseconds.
> >If this property is missing the delay will be skipped.
> >
> > +- dev-addr: If set, it indicates the device address of the PHY to be
> > +used
> > +  when accessing the C45 PHY registers over MDIO. It is used for
> > +vendor specific
> > +  register space addresses that do no conform to standard address for
> > +the MDIO
> > +  registers (e.g. MMD30)
> 
> This is a 2nd MDIO address, right? Can't you just append this to reg property?
> 
> Rob

Yes, it is a 2nd MDIO address which is coming from the PHY vendor. This address 
cannot be obtained by querying the MDIO bus, it's specified in the PHY 
datasheet. The current kernel implementation was relying on the fact that this 
address is in the decimal 0 to 7 range. That worked for the PHYs which already 
have a kernel driver, but for the new Inphi PHY, which I'm trying to add, it 
didn't.  
If I were to append the dev-addr address to the reg property,  I would have to 
fit two 32bit variable into a single one... in my opinion the code is clearer 
having the two addresses as distinct variables And so far, the comments 
from Andrew or Florian didn't go in this direction..  

Vicentiu

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

[Florian Fainelli]

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

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:

- 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

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

[Rob Herring]

On Fri, Mar 23, 2018 at 10:05:22AM -0500, 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 
> ---
>  Documentation/devicetree/bindings/net/phy.txt |  6 ++

Please split bindings to separate patch.

>  drivers/net/phy/phy_device.c  | 22 +-
>  drivers/of/of_mdio.c  | 98 
> ++-
>  include/linux/phy.h   |  5 +-
>  4 files changed, 125 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/phy.txt 
> b/Documentation/devicetree/bindings/net/phy.txt
> index d2169a5..82692e2 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -61,6 +61,11 @@ Optional Properties:
>  - reset-deassert-us: Delay after the reset was deasserted in microseconds.
>If this property is missing the delay will be skipped.
>  
> +- dev-addr: If set, it indicates the device address of the PHY to be used
> +  when accessing the C45 PHY registers over MDIO. It is used for vendor 
> specific
> +  register space addresses that do no conform to standard address for the 
> MDIO
> +  registers (e.g. MMD30)

This is a 2nd MDIO address, right? Can't you just append this to reg 
property?

Rob

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

[Andrew Lunn]

> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -24,6 +24,8 @@
>  
>  #define DEFAULT_GPIO_RESET_DELAY 10  /* in microseconds */
>  
> +struct phy_c45_device_ids mdio_c45_ids = {0};

You do know that Linux is multi-threaded. It could be probing two MDIO
busses at once.

Andrew