Messages in this thread | | | Date | Mon, 6 May 2024 12:25:36 -0700 | Subject | Re: [PATCH v3 2/3] net: phy: bcm54811: Add LRE registers definitions | From | Florian Fainelli <> |
| |
On 5/6/24 07:40, Kamil Horák - 2N wrote: > Add the definitions of LRE registers for Broadcom BCM5481x PHY
Missing Signed-off-by tag.
> --- > 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 */ > + > +/* 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 */
So previous register had definitions ordered from MSB to LSB...
> + > +/* LRE status register. */ > +#define LRESR_ERCAP 0x0001 /* Ext-reg capability */
and here we have LSB to MSB, seems like you chose MSB to LSB, so we should stick to that.
> +#define LRESR_JCD 0x0002 /* Jabber detected */ > +#define LRESR_LSTATUS 0x0004 /* Link status */ > +#define LRESR_LDSABILITY 0x0008 /* Can do LDS */
Comment should be LDS auto-negotiation capable, other comments are fine.
> +#define LRESR_8023 0x0010 /* Has IEEE 802.3 Support */ > +#define LRESR_LDSCOMPLETE 0x0020 /* LDS Auto-negotiation complete */ > +#define LRESR_MFPS 0x0040 /* Can suppress Management Frames Preamble */ > +#define LRESR_RESV 0x0080 /* Unused... */ > +#define LRESR_ESTATEN 0x0100 /* Extended Status in R15 */ > +#define LRESR_10_1PAIR 0x0200 /* Can do 10Mbps 1 Pair */ > +#define LRESR_10_2PAIR 0x0400 /* Can do 10Mbps 2 Pairs */ > +#define LRESR_100_2PAIR 0x0800 /* Can do 100Mbps 2 Pairs */ > +#define LRESR_100_4PAIR 0x1000 /* Can do 100Mbps 4 Pairs */ > +#define LRESR_100_1PAIR 0x2000 /* Can do 100Mbps 1 Pair */ > + > +/* LDS Auto-Negotiation Advertised Ability. */ > +#define LREANAA_PAUSE_ASYM 0x8000 /* Can pause asymmetrically */ > +#define LREANAA_PAUSE 0x4000 /* Can pause */ > +#define LREANAA_100_1PAIR 0x0020 /* Can do 100Mbps 1 Pair */ > +#define LREANAA_100_4PAIR 0x0010 /* Can do 100Mbps 4 Pair */ > +#define LREANAA_100_2PAIR 0x0008 /* Can do 100Mbps 2 Pair */ > +#define LREANAA_10_2PAIR 0x0004 /* Can do 10Mbps 2 Pair */ > +#define LREANAA_10_1PAIR 0x0002 /* Can do 10Mbps 1 Pair */ > + > +#define LRE_ADVERTISE_FULL (LREANAA_100_1PAIR | LREANAA_100_4PAIR | \ > + LREANAA_100_2PAIR | LREANAA_10_2PAIR | \ > + LREANAA_10_1PAIR) > + > +#define LRE_ADVERTISE_ALL LRE_ADVERTISE_FULL > + > +/* LDS Link Partner Ability. */ > +#define LRELPA_PAUSE_ASYM 0x8000 /* Supports asymmetric pause */ > +#define LRELPA_PAUSE 0x4000 /* Supports pause capability */ > +#define LRELPA_100_1PAIR 0x0020 /* 100Mbps 1 Pair capable */ > +#define LRELPA_100_4PAIR 0x0010 /* 100Mbps 4 Pair capable */ > +#define LRELPA_100_2PAIR 0x0008 /* 100Mbps 2 Pair capable */ > +#define LRELPA_10_2PAIR 0x0004 /* 10Mbps 2 Pair capable */ > +#define LRELPA_10_1PAIR 0x0002 /* 10Mbps 1 Pair capable */ > + > +/* LDS Expansion register. */ > +#define LDSE_DOWNGRADE 0x8000 /* Can do LDS Speed Downgrade */ > +#define LDSE_MASTER 0x4000 /* Master / Slave */ > +#define LDSE_PAIRS_MASK 0x3000 /* Pair Count Mask */
Please define LDSE_PAIRS_SHIFT and make the LDSE_%dPAIRS left shift by 12.
> +#define LDSE_4PAIRS 0x2000 /* 4 Pairs Connection */ > +#define LDSE_2PAIRS 0x1000 /* 2 Pairs Connection */ > +#define LDSE_1PAIR 0x0000 /* 1 Pair Connection */ > +#define LDSE_CABLEN_MASK 0x0FFF /* Cable Length Mask */ > > /* BCM54810 Registers */ > #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL (MII_BCM54XX_EXP_SEL_ER + 0x90) > #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN (1 << 0) > #define BCM54810_SHD_CLK_CTL 0x3 > #define BCM54810_SHD_CLK_CTL_GTXCLK_EN (1 << 9) > +#define BCM54810_SHD_SCR3_TRDDAPD 0x0100
Unrelated change?
> + > +/* BCM54811 Registers */ > +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL (MII_BCM54XX_EXP_SEL_ER + 0x9A) > +/* Access Control Override Enable */ > +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN BIT(15) > +/* Access Control Override Value */ > +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL BIT(14) > +/* Access Control Value */ > +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_VAL BIT(13)
This register is not documented in my datasheet, but register 0x0E is, I am fine with using that one though.
> > /* BCM54612E Registers */ > #define BCM54612E_EXP_SPARE0 (MII_BCM54XX_EXP_SEL_ETC + 0x34) > -#define BCM54612E_LED4_CLK125OUT_EN (1 << 1) > +#define BCM54612E_LED4_CLK125OUT_EN BIT(1)
Unrelated change, and one that is arguably inconsistent with the definitions you are adding, I would stick with the style you used, since there are precedents for that in brcmphy.h. -- Florian
[unhandled content-type:application/pkcs7-signature] | |