RE: [PATCH net-next 3/3] net: phy: Enable C45 PHYs with vendor specific address space
> > 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.
>
> I must clearly be missing something, but why are you introducing all these
> conditionals instead of updating the existing code to be able to operate
> against
> an arbitrary dev-addr value, and then just making sure the first thing you do
> is
> fetch that property from Device Tree? There is no way someone is going to be
> testing with your specific use case in the future (except yourselves) so
> unless you
> make supporting an arbitrary "dev-addr" value become part of how the code
> works, this is going to be breaking badly.
>
Hi Florian,
My intention was to have this patch as "plugin" and modify the existing kernel
API little to none. I was thinking that with a #ifdef, ideally, all changes
could be part of a CONFIG kernel option.
Updating the existing code, instead of the conditionals, might run into just
that, and the change could propagate across multiple modules. This is from my
first RFC patch, review by Andrew:
of_mdiobus_register(), when it loops over the children, looks for
the new property. If found, it passed dev-id to
of_mdiobus_register_phy().
That passes it to get_phy_device(). I think get_phy_device() can
then set the ID in c45_ids, before passing it to get_phy_id().
get_phy_c45_ids() will first look at devices in package and can add
further devices to c45_ids. It will then probe both those found, and
the static
one you added.
Andrew
[Vicenţiu Galanopulo]
Just to make sure I understand. Do you want me to change the signature
of all of_mdiobus_register_phy(), get_phy_device(), get_phy_id() and
get_phy_c45_ids() and include the dev_addr parameter obtained from the
device tree? (a propagation of this parameter across all functions
all the way to
get_phy_c45_devs_in_pkg?) This will impact xgbe-mdio.c, fixed_phy.c
because get_phy_device() is used in these files.
The "catch" is to transport the dev-addr value from of_mdio.c (location of the
loop of the PHY device tree node which reads all PHY node properties) to
phy_device.c (this is where you can get the PHY ID).
My understanding from Andrew's comment is that the key here is the c45_ids, and
that these could be filled in of_mdio.c, first, with the IDs from dev-addr (he
called them "static" as they are queried directly by using the value of
dev-addr) and afterwards, in phy_device.c (following the lookup loop - in a
"dynamic" way).
There's nothing more to this patch than some functionality from phy_device.c
ported to of_mdio.c, to enable the extraction of the PHY IDs.
I guess the code redundancy could be reduced (between of_mdio.c and
phy_device.c) and maybe you or Andrew could comment on this if you would like
to go with this patch approach.
Not sure I understand your comment about the specific use case and the breaking
badly part.
Right now I'm able to test because I have access to a PHY with dev-addr = 0x1e.
But the whole mechanism in this patch starts to work the moment you set
in the device tree. If you don't set that, nothing happens. If you
set it to a bogus value, no PHY ID will be found at that address. Besides that,
the PHY ID extraction code is the same as what is currently working in
phy_device.c.
80-90% of the patch is based on what already exists in phy_device.c and
of_mdio.c. Where is the breaking badly part supposed to happen?
> And please, can you keep me copied for next submissions?
Yes, the "to" list was pretty long and I somehow missed you. Sorry.
Vicentiu
RE: [PATCH net-next 3/3] net: phy: Enable C45 PHYs with vendor specific address space
> > 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.
>
> I must clearly be missing something, but why are you introducing all these
> conditionals instead of updating the existing code to be able to operate
> against
> an arbitrary dev-addr value, and then just making sure the first thing you do
> is
> fetch that property from Device Tree? There is no way someone is going to be
> testing with your specific use case in the future (except yourselves) so
> unless you
> make supporting an arbitrary "dev-addr" value become part of how the code
> works, this is going to be breaking badly.
>
Hi Florian,
My intention was to have this patch as "plugin" and modify the existing kernel
API little to none. I was thinking that with a #ifdef, ideally, all changes
could be part of a CONFIG kernel option.
Updating the existing code, instead of the conditionals, might run into just
that, and the change could propagate across multiple modules. This is from my
first RFC patch, review by Andrew:
of_mdiobus_register(), when it loops over the children, looks for
the new property. If found, it passed dev-id to
of_mdiobus_register_phy().
That passes it to get_phy_device(). I think get_phy_device() can
then set the ID in c45_ids, before passing it to get_phy_id().
get_phy_c45_ids() will first look at devices in package and can add
further devices to c45_ids. It will then probe both those found, and
the static
one you added.
Andrew
[Vicenţiu Galanopulo]
Just to make sure I understand. Do you want me to change the signature
of all of_mdiobus_register_phy(), get_phy_device(), get_phy_id() and
get_phy_c45_ids() and include the dev_addr parameter obtained from the
device tree? (a propagation of this parameter across all functions
all the way to
get_phy_c45_devs_in_pkg?) This will impact xgbe-mdio.c, fixed_phy.c
because get_phy_device() is used in these files.
The "catch" is to transport the dev-addr value from of_mdio.c (location of the
loop of the PHY device tree node which reads all PHY node properties) to
phy_device.c (this is where you can get the PHY ID).
My understanding from Andrew's comment is that the key here is the c45_ids, and
that these could be filled in of_mdio.c, first, with the IDs from dev-addr (he
called them "static" as they are queried directly by using the value of
dev-addr) and afterwards, in phy_device.c (following the lookup loop - in a
"dynamic" way).
There's nothing more to this patch than some functionality from phy_device.c
ported to of_mdio.c, to enable the extraction of the PHY IDs.
I guess the code redundancy could be reduced (between of_mdio.c and
phy_device.c) and maybe you or Andrew could comment on this if you would like
to go with this patch approach.
Not sure I understand your comment about the specific use case and the breaking
badly part.
Right now I'm able to test because I have access to a PHY with dev-addr = 0x1e.
But the whole mechanism in this patch starts to work the moment you set
in the device tree. If you don't set that, nothing happens. If you
set it to a bogus value, no PHY ID will be found at that address. Besides that,
the PHY ID extraction code is the same as what is currently working in
phy_device.c.
80-90% of the patch is based on what already exists in phy_device.c and
of_mdio.c. Where is the breaking badly part supposed to happen?
> And please, can you keep me copied for next submissions?
Yes, the "to" list was pretty long and I somehow missed you. Sorry.
Vicentiu
RE: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
> -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>;
> net...@vger.kernel.org; linux-kernel@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
> -Original Message-
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: Tuesday, March 27, 2018 1:45 AM
> To: Vicenţiu Galanopulo ;
> net...@vger.kernel.org; linux-kernel@vger.kernel.org; robh...@kernel.org;
> mark.rutl...@arm.com; da...@davemloft.net; mar...@holtmann.org;
> devicet...@vger.kernel.org
> Cc: Madalin-cristian Bucur ; Alexandru Marginean
>
> 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
> > ---
>
> 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 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
RE: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
> -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: net...@vger.kernel.org; linux-kernel@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
> -Original Message- > From: Rob Herring [mailto:r...@kernel.org] > Sent: Tuesday, March 27, 2018 1:25 AM > To: Vicenţiu Galanopulo > Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; > mark.rutl...@arm.com; da...@davemloft.net; mar...@holtmann.org; > devicet...@vger.kernel.org; Madalin-cristian Bucur ; > Alexandru Marginean > 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 > > --- > > 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
