Re: [PATCH v3 2/3] net: phy: bcm54811: Add LRE registers definitions

[Andrew Lunn]

On Mon, May 06, 2024 at 04:40:14PM +0200, Kamil Horák - 2N wrote:
> Add the definitions of LRE registers for Broadcom BCM5481x PHY
> ---
>  include/linux/brcmphy.h | 91 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
> index 1394ba302367..9c0b78c1b6fb 100644
> --- a/include/linux/brcmphy.h
> +++ b/include/linux/brcmphy.h
> @@ -270,16 +270,105 @@
>  #define BCM5482_SSD_SGMII_SLAVE		0x15	/* SGMII Slave Register */
>  #define BCM5482_SSD_SGMII_SLAVE_EN	0x0002	/* Slave mode enable */
>  #define BCM5482_SSD_SGMII_SLAVE_AD	0x0001	/* Slave auto-detection */
> +#define BCM5482_SSD_SGMII_SLAVE_AD	0x0001	/* Slave auto-detection */
> +
> +/* BroadR-Reach LRE Registers. */
> +#define MII_BCM54XX_LRECR		0x00	/* LRE Control Register                    */
> +#define MII_BCM54XX_LRESR		0x01	/* LRE Status Register                     */
> +#define MII_BCM54XX_LREPHYSID1		0x02	/* LRE PHYS ID 1                           */
> +#define MII_BCM54XX_LREPHYSID2		0x03	/* LRE PHYS ID 2                           */
> +#define MII_BCM54XX_LREANAA		0x04	/* LDS Auto-Negotiation Advertised Ability */
> +#define MII_BCM54XX_LREANAC		0x05	/* LDS Auto-Negotiation Advertised Control */
> +#define MII_BCM54XX_LREANPT		0x06	/* LDS Ability Next Page Transmit          */
> +#define MII_BCM54XX_LRELPA		0x07	/* LDS Link Partner Ability                */
> +#define MII_BCM54XX_LRELPNPM		0x08	/* LDS Link Partner Next Page Message      */
> +#define MII_BCM54XX_LRELPNPC		0x09	/* LDS Link Partner Next Page Control      */
> +#define MII_BCM54XX_LRELDSE		0x0a	/* LDS Expansion Register                  */
> +#define MII_BCM54XX_LREES		0x0f	/* LRE Extended Status                     */

Please look at these side by side to standard C22 registers. Which are
different? Is LRECR actually different to BMCR that it needs it own
define? Is LRESR different enought to BMSR that it needs its own
define? Does the ID registers use a different format?

> +/* LRE control register. */
> +#define LRECR_RESET			0x8000	/* Reset to default state      */
> +#define LRECR_LOOPBACK			0x4000	/* Internal Loopback           */
> +#define LRECR_LDSRES			0x2000	/* Restart LDS Process         */
> +#define LRECR_LDSEN			0x1000	/* LDS Enable                  */
> +#define LRECR_PDOWN			0x0800	/* Enable low power state      */
> +#define LRECR_ISOLATE			0x0400	/* Isolate data paths from MII */
> +#define LRECR_SPEED100			0x0200	/* Select 100 Mbps             */
> +#define LRECR_SPEED10			0x0000	/* Select 10 Mbps              */
> +#define LRECR_4PAIRS			0x0020	/* Select 4 Pairs              */
> +#define LRECR_2PAIRS			0x0010	/* Select 2 Pairs              */
> +#define LRECR_1PAIR			0x0000	/* Select 1 Pair               */
> +#define LRECR_MASTER			0x0008	/* Force Master when LDS disabled */
> +#define LRECR_SLAVE			0x0000	/* Force Slave when LDS disabled  */

Reverse the order of these, and then compare them to:

/* Basic mode control register. */
#define BMCR_SPEED1000          0x0040  /* MSB of Speed (1000)         */
#define BMCR_CTST               0x0080  /* Collision test              */
#define BMCR_FULLDPLX           0x0100  /* Full duplex                 */
#define BMCR_ANRESTART          0x0200  /* Auto negotiation restart    */
#define BMCR_ISOLATE            0x0400  /* Isolate data paths from MII */
#define BMCR_PDOWN              0x0800  /* Enable low power state      */
#define BMCR_ANENABLE           0x1000  /* Enable auto negotiation     */
#define BMCR_SPEED100           0x2000  /* Select 100Mbps              */
#define BMCR_LOOPBACK           0x4000  /* TXD loopback bits           */
#define BMCR_RESET              0x8000  /* Reset to default state      */

Drop any which are the same, and just add defined for those which are
different.


> +
> +/* LRE status register. */
> +#define LRESR_ERCAP			0x0001	/* Ext-reg capability          */
> +#define LRESR_JCD			0x0002	/* Jabber detected             */
> +#define LRESR_LSTATUS			0x0004	/* Link status                 */
> +#define LRESR_LDSABILITY		0x0008	/* Can do LDS                  */

What does LDS mean? In BMSR this bit is about auto-neg. How does LDS
differ from auto-neg?

Ideally, where the functionality is the same, please use the existing
definitions. It makes it easier for somebody who knows C22 to look at
the code and understand it. And it makes it easier to spot where the
differences actually are.

	Andrew