mbox series

[net-next,v2,0/5] net: phy: add Lynx PCS MDIO module

Message ID 20200621110005.23306-1-ioana.ciornei@nxp.com
Headers show
Series net: phy: add Lynx PCS MDIO module | expand

Message

Ioana Ciornei June 21, 2020, 11 a.m. UTC
Add support for the Lynx PCS as a separate module in drivers/net/phy/.
The advantage of this structure is that multiple ethernet or switch
drivers used on NXP hardware (ENETC, Felix DSA switch etc) can share the
same implementation of PCS configuration and runtime management.

The PCS is represented as an mdio_device and the callbacks exported are
highly tied with PHYLINK and can't be used without it.

The first 3 patches add some missing pieces in PHYLINK and the locked
mdiobus write accessor. Next, the Lynx PCS MDIO module is added as a
standalone module. The majority of the code is extracted from the Felix
DSA driver. The last patch makes the necessary changes in the Felix
driver in order to use the new common PCS implementation.

At the moment, USXGMII (only with in-band AN and speeds up to 2500),
SGMII, QSGMII (with and without in-band AN) and 2500Base-X (only w/o
in-band AN) are supported by the Lynx PCS MDIO module since these were
also supported by Felix and no functional change is intended at this
time.

Changes in v2:
 * got rid of the mdio_lynx_pcs structure and directly exported the
 functions without the need of an indirection
 * made the necessary adjustments for this in the Felix DSA driver
 * solved the broken allmodconfig build test by making the module
 tristate instead of bool
 * fixed a memory leakage in the Felix driver (the pcs structure was
 allocated twice)

At this moment in time, I do not feel like a major restructuring is
needed (ie export directly a phylink_pcs_ops from the Lynx
module). I feel like this would limit consumers (MAC drivers) to use
all or nothing, with no option of doing any MDIO reads/writes of their
own (not part of the common code). Also, there is already a precedent of
a PCS module (mdio-xpcs.c, the model of which I have followed) and
without also changing that (which I am not comfortable doing) there is
no point of changing this one.

Ioana Ciornei (4):
  net: phylink: consider QSGMII interface mode in
    phylink_mii_c22_pcs_get_state
  net: mdiobus: add clause 45 mdiobus write accessor
  net: phy: add Lynx PCS MDIO module
  net: dsa: felix: use the Lynx PCS helpers

Russell King (1):
  net: phylink: add interface to configure clause 22 PCS PHY

 MAINTAINERS                            |   7 +
 drivers/net/dsa/ocelot/Kconfig         |   1 +
 drivers/net/dsa/ocelot/felix.c         |   5 +
 drivers/net/dsa/ocelot/felix.h         |   7 +-
 drivers/net/dsa/ocelot/felix_vsc9959.c | 371 ++-----------------------
 drivers/net/phy/Kconfig                |   6 +
 drivers/net/phy/Makefile               |   1 +
 drivers/net/phy/mdio-lynx-pcs.c        | 337 ++++++++++++++++++++++
 drivers/net/phy/phylink.c              |  38 +++
 include/linux/fsl/enetc_mdio.h         |  21 --
 include/linux/mdio-lynx-pcs.h          |  25 ++
 include/linux/mdio.h                   |   6 +
 include/linux/phylink.h                |   3 +
 13 files changed, 461 insertions(+), 367 deletions(-)
 create mode 100644 drivers/net/phy/mdio-lynx-pcs.c
 create mode 100644 include/linux/mdio-lynx-pcs.h

Comments

Russell King (Oracle) June 21, 2020, 12:32 p.m. UTC | #1
On Sun, Jun 21, 2020 at 02:00:00PM +0300, Ioana Ciornei wrote:
> Add support for the Lynx PCS as a separate module in drivers/net/phy/.
> The advantage of this structure is that multiple ethernet or switch
> drivers used on NXP hardware (ENETC, Felix DSA switch etc) can share the
> same implementation of PCS configuration and runtime management.
> 
> The PCS is represented as an mdio_device and the callbacks exported are
> highly tied with PHYLINK and can't be used without it.
> 
> The first 3 patches add some missing pieces in PHYLINK and the locked
> mdiobus write accessor. Next, the Lynx PCS MDIO module is added as a
> standalone module. The majority of the code is extracted from the Felix
> DSA driver. The last patch makes the necessary changes in the Felix
> driver in order to use the new common PCS implementation.
> 
> At the moment, USXGMII (only with in-band AN and speeds up to 2500),
> SGMII, QSGMII (with and without in-band AN) and 2500Base-X (only w/o
> in-band AN) are supported by the Lynx PCS MDIO module since these were
> also supported by Felix and no functional change is intended at this
> time.
> 
> Changes in v2:
>  * got rid of the mdio_lynx_pcs structure and directly exported the
>  functions without the need of an indirection
>  * made the necessary adjustments for this in the Felix DSA driver
>  * solved the broken allmodconfig build test by making the module
>  tristate instead of bool
>  * fixed a memory leakage in the Felix driver (the pcs structure was
>  allocated twice)
> 
> At this moment in time, I do not feel like a major restructuring is
> needed (ie export directly a phylink_pcs_ops from the Lynx
> module). I feel like this would limit consumers (MAC drivers) to use
> all or nothing, with no option of doing any MDIO reads/writes of their
> own (not part of the common code). Also, there is already a precedent of
> a PCS module (mdio-xpcs.c, the model of which I have followed) and
> without also changing that (which I am not comfortable doing) there is
> no point of changing this one.

Please don't write off my suggestion to use phylink_pcs_ops so lightly.
I _need_ people to move over to it, so that the phylink code can be
cleaned up - or we're going to end up with phylink gradually turning
into an unmaintainable mess.  Having one way to do stuff is always
better than having multiple different backward compatible ways.

So, I /really/ want to push the phylink_pcs_ops forward, and get rid
of the ability to use the old "bolt everything into phylink_mac_ops"
approach.
Andrew Lunn June 21, 2020, 3:51 p.m. UTC | #2
Hi Ioana

I will be submitting a patchset soon which does as Russell requested,
moving drivers into subdirectories.

As part of that, i rename mdio-xpcs to pcs-xpcs, and change its
Kconfig symbol to PCS_XPCS. It would be nice if you could follow this
new naming, so all i need to do is a move.

Thanks
	Andrew
Andrew Lunn June 21, 2020, 3:56 p.m. UTC | #3
> Also, there is already a precedent of a PCS module (mdio-xpcs.c, the
> model of which I have followed) and without also changing that
> (which I am not comfortable doing) there is no point of changing
> this one.

I don't give this much value. You often need a couple of
implementation before you can see what the right structure should
be. And then you refactor. Jose is pretty active, and will probably
help refactor his driver if we ask him.

     Andrew
Ioana Ciornei June 21, 2020, 7:21 p.m. UTC | #4
> Subject: Re: [PATCH net-next v2 0/5] net: phy: add Lynx PCS MDIO module
> 
> Hi Ioana
> 
> I will be submitting a patchset soon which does as Russell requested, moving
> drivers into subdirectories.
> 
> As part of that, i rename mdio-xpcs to pcs-xpcs, and change its Kconfig symbol
> to PCS_XPCS. It would be nice if you could follow this new naming, so all i need
> to do is a move.
> 
> Thanks
> 	Andrew

Sure, I will change the file to be named pcs-lynx.c and the Kconfig accordingly.
Should I wait for you to send the patch moving xpcs to another folder?

Ioana
Andrew Lunn June 21, 2020, 8:46 p.m. UTC | #5
> Sure, I will change the file to be named pcs-lynx.c and the Kconfig accordingly.
> Should I wait for you to send the patch moving xpcs to another folder?

Please send it whenever things are agreed with Russell.

It will be a few days before i move stuff around. I have another
patchset making all the phy code C=1 W=1 clean which i would like to
get merged first.

    Andrew