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

[Florian Fainelli <florian.fainelli@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 7065 bytes --]

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


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]