Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Florinel Iordache]

> On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> > +static void setup_supported_linkmode(struct phy_device *bpphy) {
> > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> 
> I'm not sure it is a good idea to completely take over phydev->priv like this, in
> what is just helper code. What if the PHY driver needs memory of its own? There
> are a few examples of this already in other PHY drivers. Could a KR PHY contain
> a temperature sensor? Could it contain statistics counters which need
> accumulating?
> 
>         Andrew

Backplane KR driver allocates memory for structure backplane_phy_info
which is saved in phydev->priv. After all this is the purpose of priv
according to its description in phy.h: <<private data pointer For use
by PHYs to maintain extra state>>. Here the priv is used to maintain
extra state needed for backplane. This way the backplane specific data
becomes available for all PHY callbacks (defined in struct phy_driver)
that receive a pointer to phy_device structure. This initial version
doesn't include accumulating statistics counters but we have in plan
to add these in future versions. The counters will be kept in specific
structures as members of the main backplane data mentioned above
and entire support will be integrated with ethtool.

Florin.

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Andrew Lunn]

On Wed, Apr 01, 2020 at 01:35:36PM +0000, Florinel Iordache wrote:
> > On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> > > +static void setup_supported_linkmode(struct phy_device *bpphy) {
> > > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> > 
> > I'm not sure it is a good idea to completely take over phydev->priv like this, in
> > what is just helper code. What if the PHY driver needs memory of its own? There
> > are a few examples of this already in other PHY drivers. Could a KR PHY contain
> > a temperature sensor? Could it contain statistics counters which need
> > accumulating?
> > 
> >         Andrew
> 
> Backplane KR driver allocates memory for structure backplane_phy_info
> which is saved in phydev->priv. After all this is the purpose of priv
> according to its description in phy.h: <<private data pointer For use
> by PHYs to maintain extra state>>. Here the priv is used to maintain
> extra state needed for backplane. This way the backplane specific data
> becomes available for all PHY callbacks (defined in struct phy_driver)
> that receive a pointer to phy_device structure. This initial version
> doesn't include accumulating statistics counters but we have in plan
> to add these in future versions. The counters will be kept in specific
> structures as members of the main backplane data mentioned above
> and entire support will be integrated with ethtool.

Hi Florinel

And what about hwmon, or anything else which a driver needs memory
for?

As far as i see it, we have two bodies of code here. There is a set of
helpers which implement most of the backplane functionality. And then
there is an example driver for your hardware. In the future we expect
other drivers to be added for other vendors hardware.

phydev->priv is for the driver. helpers should not assume they have
complete control over it.

Anyway, this may be a mute point. Lets first solve the problem of how
a PCS is represented.

  Andrew

RE: [EXT] Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Florinel Iordache]

> On Wed, Apr 01, 2020 at 01:35:36PM +0000, Florinel Iordache wrote:
> > > On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> > > > +static void setup_supported_linkmode(struct phy_device *bpphy) {
> > > > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> > >
> > > I'm not sure it is a good idea to completely take over phydev->priv
> > > like this, in what is just helper code. What if the PHY driver needs
> > > memory of its own? There are a few examples of this already in other
> > > PHY drivers. Could a KR PHY contain a temperature sensor? Could it
> > > contain statistics counters which need accumulating?
> > >
> > >         Andrew
> >
> > Backplane KR driver allocates memory for structure backplane_phy_info
> > which is saved in phydev->priv. After all this is the purpose of priv
> > according to its description in phy.h: <<private data pointer For use
> > by PHYs to maintain extra state>>. Here the priv is used to maintain
> > extra state needed for backplane. This way the backplane specific data
> > becomes available for all PHY callbacks (defined in struct phy_driver)
> > that receive a pointer to phy_device structure. This initial version
> > doesn't include accumulating statistics counters but we have in plan
> > to add these in future versions. The counters will be kept in specific
> > structures as members of the main backplane data mentioned above and
> > entire support will be integrated with ethtool.
> 
> Hi Florinel
> 
> And what about hwmon, or anything else which a driver needs memory for?
> 
> As far as i see it, we have two bodies of code here. There is a set of helpers
> which implement most of the backplane functionality. And then there is an
> example driver for your hardware. In the future we expect other drivers to be
> added for other vendors hardware.
> 
> phydev->priv is for the driver. helpers should not assume they have
> complete control over it.
> 
> Anyway, this may be a mute point. Lets first solve the problem of how a PCS is
> represented.
> 
>   Andrew

Hi Andrew,

Backplane driver was designed as a generic backplane driver over
the PHY Abstraction Layer, containing standard implementations over
which several specific devices can be added.
(please see the diagram existent in chapter: Ethernet Backplane support
architecture, in the Documentation/networking/backplane.rst)
So according to this design, the backplane driver can use the priv pointer
provided by the layer below which is the PHY to be used as extra state
by the next upper layer which is generic backplane. On the same concept
we can provide another priv pointer in backplane main structure which is
equivalent to phy_device to be used by the specific backplane drivers on
the upper layer.
Actually for this reason, in v2 which I am currently working, I already
renamed the structure 'backplane_phy_info' to 'backplane_device'

Thank you,
Florin.

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Florinel Iordache]

> On 3/26/2020 6:07 PM, Andrew Lunn wrote:
> >> +static u32 le_ioread32(void __iomem *reg) {
> >> +    return ioread32(reg);
> >> +}
> >> +
> >> +static void le_iowrite32(u32 value, void __iomem *reg) {
> >> +    iowrite32(value, reg);
> >> +}
> >> +
> >> +static u32 be_ioread32(void __iomem *reg) {
> >> +    return ioread32be(reg);
> >> +}
> >> +
> >> +static void be_iowrite32(u32 value, void __iomem *reg) {
> >> +    iowrite32be(value, reg);
> >> +}
> >
> > This is very surprising to me. I've not got my head around the
> > structure of this code yet, but i'm surprised to see memory mapped
> > access functions in generic code.
> 
> This abstraction makes no sense whatsoever, you already have
> io{read,write}32{be,} to deal with the correct endian, and you can use the
> standard Device Tree properties 'big-endian', 'little-endian', 'native-endian' to
> decide which of those of to use. If you need to introduce a wrapper or indirect
> function calls to select the correct I/O accessor, that is fine of course.
> --
> Florian

Hi Florian,
I need these wrappers in generic code in order to automatically assign the proper
I/O accessor in the following structure according to endianness specified in DT.

/* Endianness specific memory I/O */
struct mem_io {
	u32 (*read32)(void __iomem *addr);
	void (*write32)(u32 value, void __iomem *addr);
};

And then the usage is straightforward in device specific code:
io.read32(&reg_base->tcsr3) ...
io.write32((io.read32(&reg_base->tcsr1) ... 

without the need to check endianness at each call and select which read/write function to use.
This is done in order to reduce the overall number of LOC (lines of code).

Florin.

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Russell King - ARM Linux admin]

On Wed, Apr 01, 2020 at 02:01:25PM +0000, Florinel Iordache wrote:
> > On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> > > Add support for backplane kr generic driver including link training
> > > (ieee802.3ap/ba) and fixed equalization algorithm
> > >
> > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com>
> > > +/* Read AN Link Status */
> > > +static int is_an_link_up(struct phy_device *bpphy) {
> > > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> > > +     int ret, val = 0;
> > > +
> > > +     mutex_lock(&bp_phy->bpphy_lock);
> > > +
> > > +     /* Read twice because Link_Status is LL (Latched Low) bit */
> > > +     val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy-
> > >bp_dev.mdio.an_status);
> > > +     val = phy_read_mmd(bpphy, MDIO_MMD_AN,
> > > + bp_phy->bp_dev.mdio.an_status);
> > 
> > Why not just
> > 
> > val = phy_read_mmd(bpphy, MDIO_MMD_AN, MDIO_CTRL1);
> > 
> > Or is your hardware not actually conformant to the standard?
> > 
> > There has also been a lot of discussion of reading the status twice is correct or
> > not. Don't you care the link has briefly gone down and up again?
> > 
> >         Andrew
> 
> This could be changed to use directly the MDIO_STAT1 in order to read
> AN status (and use MDIO_CTRL1 for writing the control register) but this
> is more flexible and more readable since we defined the structure
> kr_mdio_info that contains all registers offsets required by backplane
> driver like: LT(link training) registers, AN registers, PMD registers.
> So we wanted to put all these together to be clear that all these
> offsets are essential for backplane driver and they can be setup
> automatically by calling the function: backplane_setup_mdio_c45.
> 
> + void backplane_setup_mdio_c45(struct backplane_kr_info *bpkr)
> + /* KX/KR AN registers: IEEE802.3 Clause 45 (MMD 7) */
> + bpkr->mdio.an_control = MDIO_CTRL1;
> + bpkr->mdio.an_status = MDIO_STAT1;
> + bpkr->mdio.an_ad_ability_0 = MDIO_PMA_EXTABLE_10GBKR;
> + bpkr->mdio.an_ad_ability_1 = MDIO_PMA_EXTABLE_10GBKR + 1;
> + bpkr->mdio.an_lp_base_page_ability_1 = MDIO_PMA_EXTABLE_10GBKR + 4;

Where they are IEEE 802.3 standard registers, just use the standard
definitions, do not indirect.

> This approach is more flexible because it lets open the possibility for
> extension on other non-standard devices (devices non-compliant with
> clause 45) to still use this driver for backplane operation.

That's an entirely false argument.  If something is going to be
non-standard, why do you think that the only thing they'll do is
have non-standard register offsets?  Wouldn't they also have
non-standard register contents as well - and if they do, your
"flexible" model will no longer work there.

This seems to me to be a classic case of over-design.

We have seem some PHYs with multiple different PHY blocks within the
clause 45 space, but these are merely at offsets and follow the
standard IEEE 802.3 register sets at various offsets.  The minimum
that would be required in that case would be to carry a single register
offset - but there is no point until we encounter a PHY that actually
requires that for this support.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Andrew Lunn]

> > > +     val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy-
> > >bp_dev.mdio.an_status);
> > > +     val = phy_read_mmd(bpphy, MDIO_MMD_AN,
> > > + bp_phy->bp_dev.mdio.an_status);
> > 
> > Why not just
> > 
> > val = phy_read_mmd(bpphy, MDIO_MMD_AN, MDIO_CTRL1);
> > 
> > Or is your hardware not actually conformant to the standard?
> > 
> > There has also been a lot of discussion of reading the status twice is correct or
> > not. Don't you care the link has briefly gone down and up again?
> > 
> >         Andrew
> 
> This could be changed to use directly the MDIO_STAT1 in order to read
> AN status (and use MDIO_CTRL1 for writing the control register) but this
> is more flexible and more readable

Less readale. MDIO_STAT1 is well known. What does
bp_dev.mdio.an_status mean?

In general, core code should be KISS, and assume everything follows
the standard. We don't want to scatter workarounds for non-conformant
hardware in core code. Such workarounds should be in the drivers where
they are hidden away.

> + bpkr->mdio.an_control = MDIO_CTRL1;
> + bpkr->mdio.an_status = MDIO_STAT1;
> + bpkr->mdio.an_ad_ability_0 = MDIO_PMA_EXTABLE_10GBKR;
> + bpkr->mdio.an_ad_ability_1 = MDIO_PMA_EXTABLE_10GBKR + 1;
> + bpkr->mdio.an_lp_base_page_ability_1 = MDIO_PMA_EXTABLE_10GBKR + 4;

Please drop this, and use the #defines directly.

       Andrew

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Florinel Iordache]

> On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> > Add support for backplane kr generic driver including link training
> > (ieee802.3ap/ba) and fixed equalization algorithm
> >
> > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com>
> > +/* Read AN Link Status */
> > +static int is_an_link_up(struct phy_device *bpphy) {
> > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> > +     int ret, val = 0;
> > +
> > +     mutex_lock(&bp_phy->bpphy_lock);
> > +
> > +     /* Read twice because Link_Status is LL (Latched Low) bit */
> > +     val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy-
> >bp_dev.mdio.an_status);
> > +     val = phy_read_mmd(bpphy, MDIO_MMD_AN,
> > + bp_phy->bp_dev.mdio.an_status);
> 
> Why not just
> 
> val = phy_read_mmd(bpphy, MDIO_MMD_AN, MDIO_CTRL1);
> 
> Or is your hardware not actually conformant to the standard?
> 
> There has also been a lot of discussion of reading the status twice is correct or
> not. Don't you care the link has briefly gone down and up again?
> 
>         Andrew

This could be changed to use directly the MDIO_STAT1 in order to read
AN status (and use MDIO_CTRL1 for writing the control register) but this
is more flexible and more readable since we defined the structure
kr_mdio_info that contains all registers offsets required by backplane
driver like: LT(link training) registers, AN registers, PMD registers.
So we wanted to put all these together to be clear that all these
offsets are essential for backplane driver and they can be setup
automatically by calling the function: backplane_setup_mdio_c45.

+ void backplane_setup_mdio_c45(struct backplane_kr_info *bpkr)
+ /* KX/KR AN registers: IEEE802.3 Clause 45 (MMD 7) */
+ bpkr->mdio.an_control = MDIO_CTRL1;
+ bpkr->mdio.an_status = MDIO_STAT1;
+ bpkr->mdio.an_ad_ability_0 = MDIO_PMA_EXTABLE_10GBKR;
+ bpkr->mdio.an_ad_ability_1 = MDIO_PMA_EXTABLE_10GBKR + 1;
+ bpkr->mdio.an_lp_base_page_ability_1 = MDIO_PMA_EXTABLE_10GBKR + 4;

This approach is more flexible because it lets open the possibility for
extension on other non-standard devices (devices non-compliant with
clause 45) to still use this driver for backplane operation.
These non-standard devices will have just to define their particular
registers offsets in structure kr_mdio_info and then the rest of the driver
can be used without other modifications.

Florin.

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Joe Perches]

On Fri, 2020-03-27 at 15:22 +0100, Andrew Lunn wrote:
> > +/* Backplane custom logging */
> > +#define BPDEV_LOG(name) \
> > +	char log_buffer[LOG_BUFFER_SIZE]; \
> > +	va_list args; va_start(args, msg); \
> > +	vsnprintf(log_buffer, LOG_BUFFER_SIZE - 1, msg, args); \
> > +	if (!bpphy->attached_dev) \
> > +		dev_##name(&bpphy->mdio.dev, log_buffer); \
> > +	else \
> > +		dev_##name(&bpphy->mdio.dev, "%s: %s", \
> > +			netdev_name(bpphy->attached_dev), log_buffer); \
> > +	va_end(args)

This could also use %pV instead of an intermediate buffer.

It's also bad form to use macros with required
external variables.

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Florian Fainelli]

On 3/26/2020 6:07 PM, Andrew Lunn wrote:
>> +static u32 le_ioread32(void __iomem *reg)
>> +{
>> +	return ioread32(reg);
>> +}
>> +
>> +static void le_iowrite32(u32 value, void __iomem *reg)
>> +{
>> +	iowrite32(value, reg);
>> +}
>> +
>> +static u32 be_ioread32(void __iomem *reg)
>> +{
>> +	return ioread32be(reg);
>> +}
>> +
>> +static void be_iowrite32(u32 value, void __iomem *reg)
>> +{
>> +	iowrite32be(value, reg);
>> +}
> 
> This is very surprising to me. I've not got my head around the
> structure of this code yet, but i'm surprised to see memory mapped
> access functions in generic code.

This abstraction makes no sense whatsoever, you already have
io{read,write}32{be,} to deal with the correct endian, and you can use
the standard Device Tree properties 'big-endian', 'little-endian',
'native-endian' to decide which of those of to use. If you need to
introduce a wrapper or indirect function calls to select the correct I/O
accessor, that is fine of course.
-- 
Florian

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Andrew Lunn]

On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> +static void setup_supported_linkmode(struct phy_device *bpphy)
> +{
> +	struct backplane_phy_info *bp_phy = bpphy->priv;

I'm not sure it is a good idea to completely take over phydev->priv
like this, in what is just helper code. What if the PHY driver needs
memory of its own? There are a few examples of this already in other
PHY drivers. Could a KR PHY contain a temperature sensor? Could it
contain statistics counters which need accumulating?

	Andrew

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Andrew Lunn]

On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> Add support for backplane kr generic driver including link training
> (ieee802.3ap/ba) and fixed equalization algorithm
> 
> Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com>
> ---
>  drivers/net/phy/Kconfig                   |    2 +
>  drivers/net/phy/Makefile                  |    1 +
>  drivers/net/phy/backplane/Kconfig         |   20 +
>  drivers/net/phy/backplane/Makefile        |    9 +
>  drivers/net/phy/backplane/backplane.c     | 1538 +++++++++++++++++++++++++++
>  drivers/net/phy/backplane/backplane.h     |  262 +++++
>  drivers/net/phy/backplane/eq_fixed.c      |   83 ++
>  drivers/net/phy/backplane/equalization.h  |  282 +++++
>  drivers/net/phy/backplane/link_training.c | 1604 +++++++++++++++++++++++++++++
>  drivers/net/phy/backplane/link_training.h |   34 +
>  10 files changed, 3835 insertions(+)
>  create mode 100644 drivers/net/phy/backplane/Kconfig
>  create mode 100644 drivers/net/phy/backplane/Makefile
>  create mode 100644 drivers/net/phy/backplane/backplane.c
>  create mode 100644 drivers/net/phy/backplane/backplane.h
>  create mode 100644 drivers/net/phy/backplane/eq_fixed.c
>  create mode 100644 drivers/net/phy/backplane/equalization.h
>  create mode 100644 drivers/net/phy/backplane/link_training.c
>  create mode 100644 drivers/net/phy/backplane/link_training.h
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index cc7f1df..abab4e5 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -523,6 +523,8 @@ config XILINX_GMII2RGMII
>  	  the Reduced Gigabit Media Independent Interface(RGMII) between
>  	  Ethernet physical media devices and the Gigabit Ethernet controller.
>  
> +source "drivers/net/phy/backplane/Kconfig"
> +
>  endif # PHYLIB
>  
>  config MICREL_KS8995MA
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 70774ab..0b867fb 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -101,3 +101,4 @@ obj-$(CONFIG_STE10XP)		+= ste10Xp.o
>  obj-$(CONFIG_TERANETICS_PHY)	+= teranetics.o
>  obj-$(CONFIG_VITESSE_PHY)	+= vitesse.o
>  obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
> +obj-$(CONFIG_ETH_BACKPLANE)	+= backplane/
> diff --git a/drivers/net/phy/backplane/Kconfig b/drivers/net/phy/backplane/Kconfig
> new file mode 100644
> index 0000000..9ec54b5
> --- /dev/null
> +++ b/drivers/net/phy/backplane/Kconfig
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +config ETH_BACKPLANE
> +	tristate "Ethernet Backplane support"
> +	depends on OF_MDIO
> +	help
> +	  This module provides driver support for Ethernet Operation over
> +	  Electrical Backplanes. It includes Backplane generic
> +	  driver including support for Link Training (IEEE802.3ap/ba).
> +	  Based on the link quality, a signal equalization is required.
> +	  The standard specifies that a start-up algorithm should be in place
> +	  in order to get the link up.
> +
> +config ETH_BACKPLANE_FIXED
> +	tristate "Fixed: No Equalization algorithm"
> +	depends on ETH_BACKPLANE
> +	help
> +	  This module provides a driver to setup fixed user configurable
> +	  coefficient values for backplanes equalization. This means
> +	  No Equalization algorithm is used to adapt the initial coefficients
> +	  initially set by the user.
> \ No newline at end of file
> diff --git a/drivers/net/phy/backplane/Makefile b/drivers/net/phy/backplane/Makefile
> new file mode 100644
> index 0000000..ded6f2d
> --- /dev/null
> +++ b/drivers/net/phy/backplane/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +#
> +# Makefile for Ethernet Backplane driver
> +#
> +
> +obj-$(CONFIG_ETH_BACKPLANE) += eth_backplane.o
> +obj-$(CONFIG_ETH_BACKPLANE_FIXED) += eq_fixed.o
> +
> +eth_backplane-objs	:= backplane.o link_training.o
> diff --git a/drivers/net/phy/backplane/backplane.c b/drivers/net/phy/backplane/backplane.c
> new file mode 100644
> index 0000000..1b580bc
> --- /dev/null
> +++ b/drivers/net/phy/backplane/backplane.c
> @@ -0,0 +1,1538 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/* Backplane driver
> + *
> + * Copyright 2015 Freescale Semiconductor, Inc.
> + * Copyright 2018-2020 NXP
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mii.h>
> +#include <linux/mdio.h>
> +#include <linux/ethtool.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_net.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/timer.h>
> +#include <linux/delay.h>
> +#include <linux/workqueue.h>
> +#include <linux/netdevice.h>
> +#include <linux/list.h>
> +
> +#include "backplane.h"
> +#include "link_training.h"
> +
> +/* KR timeouts in milliseconds */
> +#define KR_TIMEOUT_1				100
> +#define KR_TIMEOUT_2				1000
> +#define KR_DENY_RT_INTERVAL			3000
> +#define KR_LT_TIMEOUT				500
> +
> +/* KR timings in interations */
> +#define KR_AN_WAIT_ITERATIONS			5
> +#define KR_TRAIN_STEP_ITERATIONS		2
> +#define CDR_LOCK_RETRY_COUNT			3
> +
> +/* AN status register (Clause 45) (MMD 7): MDIO_STAT1 */
> +#define AN_LINK_UP_MASK				0x04
> +
> +/* Logging buffer size */
> +#define LOG_BUFFER_SIZE				200
> +
> +/* Backplane custom logging */
> +#define BPDEV_LOG(name) \
> +	char log_buffer[LOG_BUFFER_SIZE]; \
> +	va_list args; va_start(args, msg); \
> +	vsnprintf(log_buffer, LOG_BUFFER_SIZE - 1, msg, args); \
> +	if (!bpphy->attached_dev) \
> +		dev_##name(&bpphy->mdio.dev, log_buffer); \
> +	else \
> +		dev_##name(&bpphy->mdio.dev, "%s: %s", \
> +			netdev_name(bpphy->attached_dev), log_buffer); \
> +	va_end(args)
> +
> +/* Backplane features */
> +__ETHTOOL_DECLARE_LINK_MODE_MASK(backplane_features) __ro_after_init;
> +EXPORT_SYMBOL(backplane_features);
> +
> +const int backplane_common_features_array[] = {
> +	ETHTOOL_LINK_MODE_Backplane_BIT,
> +	ETHTOOL_LINK_MODE_Autoneg_BIT,
> +	ETHTOOL_LINK_MODE_MII_BIT,
> +};
> +
> +const int backplane_protocol_features_array[] = {
> +	ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
> +	ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT,
> +};
> +
> +/* map string key to pointer data */
> +struct spmap_node {
> +	struct list_head entry;
> +	const char *key;
> +	void *pdata;
> +};
> +
> +/* registered equalization algorithms info */
> +static LIST_HEAD(eqalg_list);
> +
> +/* lanes attached to an equalization algorithm */
> +static LIST_HEAD(lnalg_list);
> +
> +/* Backplane mutex between all KR PHY threads */
> +static struct mutex backplane_lock;
> +
> +static int get_backplane_speed(phy_interface_t bp_mode)
> +{
> +	switch (bp_mode) {
> +	case PHY_INTERFACE_MODE_10GKR:
> +		return SPEED_10000;
> +	case PHY_INTERFACE_MODE_40GKR4:
> +		return SPEED_40000;
> +	default:
> +		pr_err("%s: Unsupported backplane phy interface\n",
> +		       BACKPLANE_DRIVER_NAME);
> +		return SPEED_UNKNOWN;
> +	}
> +	return SPEED_UNKNOWN;
> +}
> +
> +static enum ethtool_link_mode_bit_indices
> +	get_backplane_supported_mode(phy_interface_t bp_mode)
> +{
> +	switch (bp_mode) {
> +	case PHY_INTERFACE_MODE_10GKR:
> +		return ETHTOOL_LINK_MODE_10000baseKR_Full_BIT;
> +	case PHY_INTERFACE_MODE_40GKR4:
> +		return ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT;
> +	default:
> +		pr_err("%s: Unsupported backplane phy interface\n",
> +		       BACKPLANE_DRIVER_NAME);
> +		return ETHTOOL_LINK_MODE_Backplane_BIT;
> +	}
> +	return ETHTOOL_LINK_MODE_Backplane_BIT;
> +}
> +
> +static int spmap_add(struct list_head *list, const char *key, void *pdata)
> +{
> +	struct spmap_node *node;
> +
> +	/* create a new entry with desired key */
> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	if (!node)
> +		return -ENOMEM;
> +
> +	node->key = key;
> +	node->pdata = pdata;
> +
> +	list_add(&node->entry, list);
> +
> +	return 0;
> +}
> +
> +static const struct equalization_algorithm *eq_find(const char *key)
> +{
> +	struct spmap_node *eqalg, *eqalg_tmp;
> +
> +	if (!key)
> +		return NULL;
> +
> +	/* search desired single key */
> +	list_for_each_entry_safe(eqalg, eqalg_tmp, &eqalg_list, entry) {
> +		if (strcmp(eqalg->key, key) == 0)
> +			return (struct equalization_algorithm *)eqalg->pdata;
> +	}
> +	return NULL;
> +}
> +
> +static void backplane_features_init(void)
> +{
> +	linkmode_set_bit_array(backplane_common_features_array,
> +			       ARRAY_SIZE(backplane_common_features_array),
> +			       backplane_features);
> +
> +	linkmode_set_bit_array(backplane_protocol_features_array,
> +			       ARRAY_SIZE(backplane_protocol_features_array),
> +			       backplane_features);
> +}
> +
> +static u32 le_ioread32(void __iomem *reg)
> +{
> +	return ioread32(reg);
> +}
> +
> +static void le_iowrite32(u32 value, void __iomem *reg)
> +{
> +	iowrite32(value, reg);
> +}
> +
> +static u32 be_ioread32(void __iomem *reg)
> +{
> +	return ioread32be(reg);
> +}
> +
> +static void be_iowrite32(u32 value, void __iomem *reg)
> +{
> +	iowrite32be(value, reg);
> +}
> +
> +static void training_status_init(struct training_status *trst)
> +{
> +	trst->done_training = false;
> +	trst->remote_tx_complete = false;
> +	trst->remote_tx_running = false;
> +	trst->sent_init = false;
> +	trst->lp_rx_ready = 0;
> +	trst->local_tx_running = false;
> +}
> +
> +static void init_krln(struct kr_lane_info *krln, bool revert_default)
> +{
> +	if (revert_default)
> +		backplane_default_kr_lane(krln);
> +
> +	training_status_init(&krln->trst);
> +	krln->state = DETECTING_LP;
> +	krln->an_acquired = false;
> +
> +	krln->ld_update = 0;
> +	krln->prev_ld_update = 0;
> +	krln->ld_last_nonhold_update = 0;
> +	krln->lp_status = 0;
> +	krln->lp_last_change_status = 0;
> +	krln->last_lp_update_status[C_M1] = 0;
> +	krln->last_lp_update_status[C_Z0] = 0;
> +	krln->last_lp_update_status[C_P1] = 0;
> +	krln->ld_status = 0;
> +	krln->move_back_prev = false;
> +	krln->move_back_cnt = 0;
> +	krln->move_back_lp_status = 0;
> +
> +	lt_init_ld(krln);
> +}
> +
> +static void setup_supported_linkmode(struct phy_device *bpphy)
> +{
> +	struct backplane_phy_info *bp_phy = bpphy->priv;
> +	int i;
> +
> +	/* Clear all supported backplane protocols features
> +	 * and setup only the currently configured protocol
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(backplane_protocol_features_array); i++)
> +		linkmode_clear_bit(backplane_protocol_features_array[i],
> +				   bpphy->supported);
> +
> +	linkmode_set_bit(get_backplane_supported_mode(bp_phy->bp_mode),
> +			 bpphy->supported);
> +}
> +
> +/* Read AN Link Status */
> +static int is_an_link_up(struct phy_device *bpphy)
> +{
> +	struct backplane_phy_info *bp_phy = bpphy->priv;
> +	int ret, val = 0;
> +
> +	mutex_lock(&bp_phy->bpphy_lock);
> +
> +	/* Read twice because Link_Status is LL (Latched Low) bit */
> +	val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy->bp_dev.mdio.an_status);
> +	val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy->bp_dev.mdio.an_status);

Why not just 

val = phy_read_mmd(bpphy, MDIO_MMD_AN, MDIO_CTRL1);

Or is your hardware not actually conformant to the standard?

There has also been a lot of discussion of reading the status twice is
correct or not. Don't you care the link has briefly gone down and up
again?

	Andrew

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Andrew Lunn]

> +/* Read AN Link Status */
> +static int is_an_link_up(struct phy_device *bpphy)
> +{
> +	struct backplane_phy_info *bp_phy = bpphy->priv;
> +	int ret, val = 0;
> +
> +	mutex_lock(&bp_phy->bpphy_lock);
> +
> +	/* Read twice because Link_Status is LL (Latched Low) bit */
> +	val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy->bp_dev.mdio.an_status);
> +	val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy->bp_dev.mdio.an_status);
> +
> +	mutex_unlock(&bp_phy->bpphy_lock);

How does this mutex interact with phydev->lock? It appears both are
trying to do the same thing, serialise access to the PHY hardware.

	 Andrew

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Andrew Lunn]

> +/* Backplane custom logging */
> +#define BPDEV_LOG(name) \
> +	char log_buffer[LOG_BUFFER_SIZE]; \
> +	va_list args; va_start(args, msg); \
> +	vsnprintf(log_buffer, LOG_BUFFER_SIZE - 1, msg, args); \
> +	if (!bpphy->attached_dev) \
> +		dev_##name(&bpphy->mdio.dev, log_buffer); \
> +	else \
> +		dev_##name(&bpphy->mdio.dev, "%s: %s", \
> +			netdev_name(bpphy->attached_dev), log_buffer); \
> +	va_end(args)

> +void bpdev_err(struct phy_device *bpphy, char *msg, ...)
> +{
> +	BPDEV_LOG(err);
> +}
> +EXPORT_SYMBOL(bpdev_err);
> +
> +void bpdev_warn(struct phy_device *bpphy, char *msg, ...)
> +{
> +	BPDEV_LOG(warn);
> +}
> +EXPORT_SYMBOL(bpdev_warn);
> +
> +void bpdev_info(struct phy_device *bpphy, char *msg, ...)
> +{
> +	BPDEV_LOG(info);
> +}
> +EXPORT_SYMBOL(bpdev_info);
> +
> +void bpdev_dbg(struct phy_device *bpphy, char *msg, ...)
> +{
> +	BPDEV_LOG(dbg);
> +}
> +EXPORT_SYMBOL(bpdev_dbg);

You are currently modelling this as a phydev. So please just use
phydev_err(), phydev_info(), phydev_dbg() etc.

Also, if you look at other PHY code, struct phy_device * is nearly
always called phydev. Please try to be consistent with the existing
code base.

     Andrew

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Andrew Lunn]

> +static u32 le_ioread32(void __iomem *reg)
> +{
> +	return ioread32(reg);
> +}
> +
> +static void le_iowrite32(u32 value, void __iomem *reg)
> +{
> +	iowrite32(value, reg);
> +}
> +
> +static u32 be_ioread32(void __iomem *reg)
> +{
> +	return ioread32be(reg);
> +}
> +
> +static void be_iowrite32(u32 value, void __iomem *reg)
> +{
> +	iowrite32be(value, reg);
> +}

This is very surprising to me. I've not got my head around the
structure of this code yet, but i'm surprised to see memory mapped
access functions in generic code.

       Andrew

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Joe Perches]

On Thu, 2020-03-26 at 12:07 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Thu, 26 Mar 2020 11:55:17 -0700
> 
> > On Thu, 2020-03-26 at 11:53 -0700, David Miller wrote:
> >> From: Florinel Iordache <florinel.iordache@nxp.com>
> >> Date: Thu, 26 Mar 2020 15:51:19 +0200
> >> 
> >> > +static void kr_reset_master_lane(struct kr_lane_info *krln)
> >> > +{
> >> > +     struct phy_device *bpphy = krln->bpphy;
> >> > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> >> > +     const struct lane_io_ops *lane_ops = krln->bp_phy->bp_dev.lane_ops;
> >> 
> >> Please use reverse christmas tree ordering for local variables.
> > 
> > How (any why) do you suggest the first 2 entries here
> > should be ordered?
> 
> You have to sometimes put assignments into the code body rather than
> the declarations in situations like this.

No "why" reply given.

An option is not using reverse christmas tree to both
avoid ordering
constraints and reduce overall line count.

I think this is your own personal taste rather than an
actual valuable addition for subsystem maintenance.

And if this a serious requirement for the subsystem
you oversee, you should add it to something like a
maintainer-entry-profile with a "P:" line in MAINTAINERS.

https://www.kernel.org/doc/html/latest/maintainer/maintainer-entry-profile.html

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Thu, 26 Mar 2020 11:55:17 -0700

> On Thu, 2020-03-26 at 11:53 -0700, David Miller wrote:
>> From: Florinel Iordache <florinel.iordache@nxp.com>
>> Date: Thu, 26 Mar 2020 15:51:19 +0200
>> 
>> > +static void kr_reset_master_lane(struct kr_lane_info *krln)
>> > +{
>> > +     struct phy_device *bpphy = krln->bpphy;
>> > +     struct backplane_phy_info *bp_phy = bpphy->priv;
>> > +     const struct lane_io_ops *lane_ops = krln->bp_phy->bp_dev.lane_ops;
>> 
>> Please use reverse christmas tree ordering for local variables.
> 
> How (any why) do you suggest the first 2 entries here
> should be ordered?

You have to sometimes put assignments into the code body rather than
the declarations in situations like this.

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Joe Perches]

On Thu, 2020-03-26 at 11:53 -0700, David Miller wrote:
> From: Florinel Iordache <florinel.iordache@nxp.com>
> Date: Thu, 26 Mar 2020 15:51:19 +0200
> 
> > +static void kr_reset_master_lane(struct kr_lane_info *krln)
> > +{
> > +     struct phy_device *bpphy = krln->bpphy;
> > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> > +     const struct lane_io_ops *lane_ops = krln->bp_phy->bp_dev.lane_ops;
> 
> Please use reverse christmas tree ordering for local variables.

How (any why) do you suggest the first 2 entries here
should be ordered?

Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[David Miller]

From: Florinel Iordache <florinel.iordache@nxp.com>
Date: Thu, 26 Mar 2020 15:51:19 +0200

> +static void kr_reset_master_lane(struct kr_lane_info *krln)
> +{
> +	struct phy_device *bpphy = krln->bpphy;
> +	struct backplane_phy_info *bp_phy = bpphy->priv;
> +	const struct lane_io_ops *lane_ops = krln->bp_phy->bp_dev.lane_ops;

Please use reverse christmas tree ordering for local variables.

Please audit your entire submission for this issue.

Thank you.