netdev - Re: [net-next PATCH 11/15] eth: fbnic: Enable Ethernet link setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bffafcca-3b02-4f76-929b-fc5e30284c2b@lunn.ch>
Date: Mon, 22 Apr 2024 17:52:08 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: netdev@...r.kernel.org, Alexander Duyck <alexanderduyck@...com>,
	kuba@...nel.org, davem@...emloft.net, pabeni@...hat.com
Subject: Re: [net-next PATCH 11/15] eth: fbnic: Enable Ethernet link setup

On Sun, Apr 21, 2024 at 04:21:57PM -0700, Alexander Duyck wrote:
> On Fri, Apr 5, 2024 at 2:51 PM Andrew Lunn <andrew@...n.ch> wrote:
> >
> > > +#define FBNIC_CSR_START_PCS          0x10000 /* CSR section delimiter */
> > > +#define FBNIC_PCS_CONTROL1_0         0x10000         /* 0x40000 */
> > > +#define FBNIC_PCS_CONTROL1_RESET             CSR_BIT(15)
> > > +#define FBNIC_PCS_CONTROL1_LOOPBACK          CSR_BIT(14)
> > > +#define FBNIC_PCS_CONTROL1_SPEED_SELECT_ALWAYS       CSR_BIT(13)
> > > +#define FBNIC_PCS_CONTROL1_SPEED_ALWAYS              CSR_BIT(6)
> >
> > This appears to be PCS control register 1, define in 45.2.3.1. Since
> > this is a standard register, please add it to mdio.h.
> 
> Actually all these bits are essentially there in the forms of:
> MDIO_CTRL1_RESET, MDIO_PCS_CTRL1_LOOPBACK, and MDIO_CTRL1_SPEEDSELEXT.
> I will base the driver on these values.

Great, thanks.

> > > +#define FBNIC_PCS_VENDOR_VL_INTVL_0  0x10202         /* 0x40808 */
> >
> > Could you explain how these registers map to 802.3 clause 45? Would
> > that be 3.1002? That would however put it in the reserved range 3.812
> > through 3.1799. The vendor range is 3.32768 through 3.65535.
> 
> So from what I can tell the vendor specific registers are mapped into
> the middle of the range starting at register 512 instead of starting
> at 32768.

802.3, clause 1.4.512:

  reserved: A key word indicating an object (bit, register, connector
  pin, encoding, interface signal, enumeration, etc.) to be defined
  only by this standard. A reserved object shall not be used for any
  user- defined purpose such as a user- or device-specific function;
  and such use of a reserved object shall render the implementation
  noncompliant with this standard.

It is surprising how many vendors like to make their devices
noncompliant by not following simple things like this. Anyway, nothing
you can do. Please put _VEND_ in the #define names to make it clear
these are vendor registers, even if they are not in the vendor space.

> > > +#define FBNIC_CSR_START_RSFEC                0x10800 /* CSR section delimiter */
> > > +#define FBNIC_RSFEC_CONTROL(n)\
> > > +                             (0x10800 + 8 * (n))     /* 0x42000 + 32*n */
> > > +#define FBNIC_RSFEC_CONTROL_AM16_COPY_DIS    CSR_BIT(3)
> > > +#define FBNIC_RSFEC_CONTROL_KP_ENABLE                CSR_BIT(8)
> > > +#define FBNIC_RSFEC_CONTROL_TC_PAD_ALTER     CSR_BIT(10)
> > > +#define FBNIC_RSFEC_MAX_LANES                        4
> > > +#define FBNIC_RSFEC_CCW_LO(n) \
> > > +                             (0x10802 + 8 * (n))     /* 0x42008 + 32*n */
> > > +#define FBNIC_RSFEC_CCW_HI(n) \
> > > +                             (0x10803 + 8 * (n))     /* 0x4200c + 32*n */
> >
> > Is this Corrected Code Words Lower/Upper? 1.202 and 1.203?
> 
> Yes and no, this is 3.802 and 3.803 which I assume is more or less the
> same thing but the PCS variant.

Have you figure out how to map the 802.3 register number to the value
you need here? 0x42008 + 32*n? Ideally we should list the registers in
the common header file with there 802.3 defined value. Your driver can
them massage the value to what you need for your memory mapped device.

If you really want to go the whole hog, you might be able to extend
mdio-regmap.c to support memory mapped C45 registers, so your driver
can then use mdiodev_c45_read()/mdiodev_c45_write(). We have a few PCS
drivers for licensed IP which appear on both MDIO busses and memory
mapped. mdio-regmap.c hides way the access details.

> I will try to see what I can do. I will try to sort out what is device
> device specific such as our register layout versus what is shared such
> as PCS register layouts and such.

That would be great.

	Andrew