Message-ID: <c3c155131582acf447ed0a370bf4e7701c76b425.camel@gmail.com>
Date: Mon, 22 Apr 2024 11:59:13 -0700
From: Alexander H Duyck <alexander.duyck@...il.com>
To: Andrew Lunn <andrew@...n.ch>
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 Mon, 2024-04-22 at 17:52 +0200, Andrew Lunn wrote:
> 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.

Yeah, I am not sure how much of this is the synthesis of the IP versus
the mapping functionality of our device in terms of how the registers
got ordered. I'm thinking if nothing else there may be a need to break
this up into logical "pages".

>From what I can tell the layout is something like:
CSR Range	Register Block
==================================
   0 -  511	PCS Channel 0 (within spec)
 512 - 1023	PCS Channel 0 Vendor Registers
1024 - 1535	PCS Channel 1 (within spec)
1536 - 2043	PCS Channel 1 Vendor Registers

I said we weren't using channel 1 registers but after looking back
through the code and starting re-factoring I believe I was thinking of
channels 2 and 3 which would be used for 100-R4. Basically channel 1 is
used in the 50-R2 and 100-R2 use cases.

As far as the vendor registers themselves most of this block of
registers is all about the virtual lane alignment markers. As such I
may want to export the values to a shared header file since they should
be common as per the spec, but the means of programming them would be
vendor specific.

> > > > +#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.

Similarly for the RSFEC portion things seem to have been broken out
into 4 blocks w/ multiple sets of registers. The first 6 are laid out
in the same order as the spec, but they are starting at an offset of
(2048 + 8 * (n)) instead of 800. So I suppose the big question would be
how to convert the standard addressing scheme into something that would
get us to the right page for the right interface.

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

The big issue as I see it is the fact that we have multiple copies of
things that are interleaved together. So for example with the RSFEC we
have 4 blocks of 8 registers that are all interleaved with the first 6
matching the layout, but the last 2 being something different.

Since I am not accessing these via MDIO I am not sure what the expected
layout should be in terms of deciding what should be a device, channel,
or register address and how that would map to a page.

Powered by blists - more mailing lists