Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

Hi Andrew / Heiner / Vladimir,

On 8/6/19 2:09 PM, Tao Ren wrote:
> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
> example, on Facebook CMM BMC platform), mainly because genphy functions
> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
> needs to be handled differently.
> 
> This patch enables 1000Base-X support for BCM54616S by customizing 3
> driver callbacks:
> 
>   - probe: probe callback detects PHY's operation mode based on
>     INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>     Control register.
> 
>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>     negotiation in 1000Base-X mode.
> 
>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>     callback which manually set link speed and duplex mode in 1000Base-X
>     mode.
> 
> Signed-off-by: Tao Ren <taoren@fb.com>

I customized config_aneg function for BCM54616S 1000Base-X mode and link-down issue is also fixed: the patch is tested on Facebook CMM and Minipack BMC and everything looks normal. Please kindly review when you have bandwidth and let me know if you have further suggestions.

BTW, I would be happy to help if we decide to add a set of genphy functions for clause 37, although that may mean I need more help/guidance from you :-)


Cheers,

Tao

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Heiner Kallweit]

On 06.08.2019 23:42, Tao Ren wrote:
> Hi Andrew / Heiner / Vladimir,
> 
> On 8/6/19 2:09 PM, Tao Ren wrote:
>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>> example, on Facebook CMM BMC platform), mainly because genphy functions
>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>> needs to be handled differently.
>>
>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>> driver callbacks:
>>
>>   - probe: probe callback detects PHY's operation mode based on
>>     INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>     Control register.
>>
>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>>     negotiation in 1000Base-X mode.
>>
>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>     callback which manually set link speed and duplex mode in 1000Base-X
>>     mode.
>>
>> Signed-off-by: Tao Ren <taoren@fb.com>
> 
> I customized config_aneg function for BCM54616S 1000Base-X mode and link-down issue is also fixed: the patch is tested on Facebook CMM and Minipack BMC and everything looks normal. Please kindly review when you have bandwidth and let me know if you have further suggestions.
> 
> BTW, I would be happy to help if we decide to add a set of genphy functions for clause 37, although that may mean I need more help/guidance from you :-)

You want to have standard clause 37 aneg and this should be generic in phylib.
I hacked together a first version that is compile-tested only:
https://patchwork.ozlabs.org/patch/1143631/
It supports fixed mode too.

It doesn't support half duplex mode because phylib doesn't know 1000BaseX HD yet.
Not sure whether half duplex mode is used at all in reality.

You could test the new core functions in your own config_aneg and read_status
callback implementations.

> 
> 
> Cheers,
> 
> Tao
> 
Heiner

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

Hi Heiner,

On 8/7/19 12:18 PM, Heiner Kallweit wrote:
> On 06.08.2019 23:42, Tao Ren wrote:
>> Hi Andrew / Heiner / Vladimir,
>>
>> On 8/6/19 2:09 PM, Tao Ren wrote:
>>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>>> example, on Facebook CMM BMC platform), mainly because genphy functions
>>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>>> needs to be handled differently.
>>>
>>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>>> driver callbacks:
>>>
>>>   - probe: probe callback detects PHY's operation mode based on
>>>     INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>>     Control register.
>>>
>>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>>>     negotiation in 1000Base-X mode.
>>>
>>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>>     callback which manually set link speed and duplex mode in 1000Base-X
>>>     mode.
>>>
>>> Signed-off-by: Tao Ren <taoren@fb.com>
>>
>> I customized config_aneg function for BCM54616S 1000Base-X mode and link-down issue is also fixed: the patch is tested on Facebook CMM and Minipack BMC and everything looks normal. Please kindly review when you have bandwidth and let me know if you have further suggestions.
>>
>> BTW, I would be happy to help if we decide to add a set of genphy functions for clause 37, although that may mean I need more help/guidance from you :-)
> 
> You want to have standard clause 37 aneg and this should be generic in phylib.
> I hacked together a first version that is compile-tested only:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_1143631_&d=DwICaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=iYElT7HC77pRZ3byVvW8ng&m=ZJArOJvHqNkqvs1x8l9HjfxjCN8e5xJpPz2YViBuKRA&s=EskpfBQtu9IBVeb96dv-sz76xIz4tJK5-lD4-qdIyWI&e= 
> It supports fixed mode too.
> 
> It doesn't support half duplex mode because phylib doesn't know 1000BaseX HD yet.
> Not sure whether half duplex mode is used at all in reality.
> 
> You could test the new core functions in your own config_aneg and read_status
> callback implementations.

Thank you very much for the help! I'm planning to add these functions but I haven't started yet because I'm still going through clause 37 :-)

Let me apply your patch and run some test on my platform. Will share you results tomorrow.


Cheers,

Tao

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

Hi Heiner,

On 8/7/19 9:24 PM, Tao Ren wrote:
> Hi Heiner,
> 
> On 8/7/19 12:18 PM, Heiner Kallweit wrote:
>> On 06.08.2019 23:42, Tao Ren wrote:
>>> Hi Andrew / Heiner / Vladimir,
>>>
>>> On 8/6/19 2:09 PM, Tao Ren wrote:
>>>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>>>> example, on Facebook CMM BMC platform), mainly because genphy functions
>>>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>>>> needs to be handled differently.
>>>>
>>>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>>>> driver callbacks:
>>>>
>>>>   - probe: probe callback detects PHY's operation mode based on
>>>>     INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>>>     Control register.
>>>>
>>>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>>>>     negotiation in 1000Base-X mode.
>>>>
>>>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>>>     callback which manually set link speed and duplex mode in 1000Base-X
>>>>     mode.
>>>>
>>>> Signed-off-by: Tao Ren <taoren@fb.com>
>>>
>>> I customized config_aneg function for BCM54616S 1000Base-X mode and link-down issue is also fixed: the patch is tested on Facebook CMM and Minipack BMC and everything looks normal. Please kindly review when you have bandwidth and let me know if you have further suggestions.
>>>
>>> BTW, I would be happy to help if we decide to add a set of genphy functions for clause 37, although that may mean I need more help/guidance from you :-)
>>
>> You want to have standard clause 37 aneg and this should be generic in phylib.
>> I hacked together a first version that is compile-tested only:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_1143631_&d=DwICaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=iYElT7HC77pRZ3byVvW8ng&m=ZJArOJvHqNkqvs1x8l9HjfxjCN8e5xJpPz2YViBuKRA&s=EskpfBQtu9IBVeb96dv-sz76xIz4tJK5-lD4-qdIyWI&e= 
>> It supports fixed mode too.
>>
>> It doesn't support half duplex mode because phylib doesn't know 1000BaseX HD yet.
>> Not sure whether half duplex mode is used at all in reality.
>>
>> You could test the new core functions in your own config_aneg and read_status
>> callback implementations.
> 
> Thank you very much for the help! I'm planning to add these functions but I haven't started yet because I'm still going through clause 37 :-)
> 
> Let me apply your patch and run some test on my platform. Will share you results tomorrow.

The patch "net: phy: add support for clause 37 auto-negotiation" works on my CMM platform, with just 1 minor change in phy.h (I guess it's typo?). Thanks again for the help!

-int genphy_c37_aneg_done(struct phy_device *phydev);
+int genphy_c37_config_aneg(struct phy_device *phydev);

BTW, shall I send out my patch v5 now (based on your patch)? Or I should wait till your patch is included in net-next and then send out my patch?


Cheers,

Tao

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Heiner Kallweit]

On 08.08.2019 23:47, Tao Ren wrote:
> Hi Heiner,
> 
> On 8/7/19 9:24 PM, Tao Ren wrote:
>> Hi Heiner,
>>
>> On 8/7/19 12:18 PM, Heiner Kallweit wrote:
>>> On 06.08.2019 23:42, Tao Ren wrote:
>>>> Hi Andrew / Heiner / Vladimir,
>>>>
>>>> On 8/6/19 2:09 PM, Tao Ren wrote:
>>>>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>>>>> example, on Facebook CMM BMC platform), mainly because genphy functions
>>>>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>>>>> needs to be handled differently.
>>>>>
>>>>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>>>>> driver callbacks:
>>>>>
>>>>>   - probe: probe callback detects PHY's operation mode based on
>>>>>     INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>>>>     Control register.
>>>>>
>>>>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>>>>>     negotiation in 1000Base-X mode.
>>>>>
>>>>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>>>>     callback which manually set link speed and duplex mode in 1000Base-X
>>>>>     mode.
>>>>>
>>>>> Signed-off-by: Tao Ren <taoren@fb.com>
>>>>
>>>> I customized config_aneg function for BCM54616S 1000Base-X mode and link-down issue is also fixed: the patch is tested on Facebook CMM and Minipack BMC and everything looks normal. Please kindly review when you have bandwidth and let me know if you have further suggestions.
>>>>
>>>> BTW, I would be happy to help if we decide to add a set of genphy functions for clause 37, although that may mean I need more help/guidance from you :-)
>>>
>>> You want to have standard clause 37 aneg and this should be generic in phylib.
>>> I hacked together a first version that is compile-tested only:
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_1143631_&d=DwICaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=iYElT7HC77pRZ3byVvW8ng&m=ZJArOJvHqNkqvs1x8l9HjfxjCN8e5xJpPz2YViBuKRA&s=EskpfBQtu9IBVeb96dv-sz76xIz4tJK5-lD4-qdIyWI&e= 
>>> It supports fixed mode too.
>>>
>>> It doesn't support half duplex mode because phylib doesn't know 1000BaseX HD yet.
>>> Not sure whether half duplex mode is used at all in reality.
>>>
>>> You could test the new core functions in your own config_aneg and read_status
>>> callback implementations.
>>
>> Thank you very much for the help! I'm planning to add these functions but I haven't started yet because I'm still going through clause 37 :-)
>>
>> Let me apply your patch and run some test on my platform. Will share you results tomorrow.
> 
> The patch "net: phy: add support for clause 37 auto-negotiation" works on my CMM platform, with just 1 minor change in phy.h (I guess it's typo?). Thanks again for the help!
> 
> -int genphy_c37_aneg_done(struct phy_device *phydev);
> +int genphy_c37_config_aneg(struct phy_device *phydev);
> 
Indeed, this was a typo. Thanks.

> BTW, shall I send out my patch v5 now (based on your patch)? Or I should wait till your patch is included in net-next and then send out my patch?
> 
Adding new functions to the core is typically only acceptable if in the
same patch series a user of the new functions is added. Therefore it's
best if you include my patch in your series (just remove the RFC tag and
set the From: properly).

> 
> Cheers,
> 
> Tao
> 
Heiner

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 8/8/19 3:11 PM, Heiner Kallweit wrote:
> On 08.08.2019 23:47, Tao Ren wrote:
>> Hi Heiner,
>>
>> On 8/7/19 9:24 PM, Tao Ren wrote:
>>> Hi Heiner,
>>>
>>> On 8/7/19 12:18 PM, Heiner Kallweit wrote:
>>>> On 06.08.2019 23:42, Tao Ren wrote:
>>>>> Hi Andrew / Heiner / Vladimir,
>>>>>
>>>>> On 8/6/19 2:09 PM, Tao Ren wrote:
>>>>>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>>>>>> example, on Facebook CMM BMC platform), mainly because genphy functions
>>>>>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>>>>>> needs to be handled differently.
>>>>>>
>>>>>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>>>>>> driver callbacks:
>>>>>>
>>>>>>   - probe: probe callback detects PHY's operation mode based on
>>>>>>     INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>>>>>     Control register.
>>>>>>
>>>>>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>>>>>>     negotiation in 1000Base-X mode.
>>>>>>
>>>>>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>>>>>     callback which manually set link speed and duplex mode in 1000Base-X
>>>>>>     mode.
>>>>>>
>>>>>> Signed-off-by: Tao Ren <taoren@fb.com>
>>>>>
>>>>> I customized config_aneg function for BCM54616S 1000Base-X mode and link-down issue is also fixed: the patch is tested on Facebook CMM and Minipack BMC and everything looks normal. Please kindly review when you have bandwidth and let me know if you have further suggestions.
>>>>>
>>>>> BTW, I would be happy to help if we decide to add a set of genphy functions for clause 37, although that may mean I need more help/guidance from you :-)
>>>>
>>>> You want to have standard clause 37 aneg and this should be generic in phylib.
>>>> I hacked together a first version that is compile-tested only:
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_1143631_&d=DwICaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=iYElT7HC77pRZ3byVvW8ng&m=ZJArOJvHqNkqvs1x8l9HjfxjCN8e5xJpPz2YViBuKRA&s=EskpfBQtu9IBVeb96dv-sz76xIz4tJK5-lD4-qdIyWI&e= 
>>>> It supports fixed mode too.
>>>>
>>>> It doesn't support half duplex mode because phylib doesn't know 1000BaseX HD yet.
>>>> Not sure whether half duplex mode is used at all in reality.
>>>>
>>>> You could test the new core functions in your own config_aneg and read_status
>>>> callback implementations.
>>>
>>> Thank you very much for the help! I'm planning to add these functions but I haven't started yet because I'm still going through clause 37 :-)
>>>
>>> Let me apply your patch and run some test on my platform. Will share you results tomorrow.
>>
>> The patch "net: phy: add support for clause 37 auto-negotiation" works on my CMM platform, with just 1 minor change in phy.h (I guess it's typo?). Thanks again for the help!
>>
>> -int genphy_c37_aneg_done(struct phy_device *phydev);
>> +int genphy_c37_config_aneg(struct phy_device *phydev);
>>
> Indeed, this was a typo. Thanks.
> 
>> BTW, shall I send out my patch v5 now (based on your patch)? Or I should wait till your patch is included in net-next and then send out my patch?
>>
> Adding new functions to the core is typically only acceptable if in the
> same patch series a user of the new functions is added. Therefore it's
> best if you include my patch in your series (just remove the RFC tag and
> set the From: properly).

Got it. Let me play with it (especially "From:" property) and will send out patch series soon.


Cheers,

Tao

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Heiner Kallweit]

On 06.08.2019 23:09, Tao Ren wrote:
> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
> example, on Facebook CMM BMC platform), mainly because genphy functions
> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
> needs to be handled differently.
> 
> This patch enables 1000Base-X support for BCM54616S by customizing 3
> driver callbacks:
> 
>   - probe: probe callback detects PHY's operation mode based on
>     INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>     Control register.
> 
>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>     negotiation in 1000Base-X mode.
> 
>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>     callback which manually set link speed and duplex mode in 1000Base-X
>     mode.
> 
> Signed-off-by: Tao Ren <taoren@fb.com>
> ---
>  Changes in v4:
>   - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
>     1000Base-X mode.
>  Changes in v3:
>   - rename bcm5482_read_status to bcm54xx_read_status so the callback can
>     be shared by BCM5482 and BCM54616S.
>  Changes in v2:
>   - Auto-detect PHY operation mode instead of passing DT node.
>   - move PHY mode auto-detect logic from config_init to probe callback.
>   - only set speed (not including duplex) in read_status callback.
>   - update patch description with more background to avoid confusion.
>   - patch #1 in the series ("net: phy: broadcom: set features explicitly
>     for BCM54616") is dropped: the fix should go to get_features callback
>     which may potentially depend on this patch.
> 
>  drivers/net/phy/broadcom.c | 62 ++++++++++++++++++++++++++++++++++----
>  include/linux/brcmphy.h    | 10 ++++--
>  2 files changed, 64 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 937d0059e8ac..bf61ed8451e5 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
>  		/*
>  		 * Select 1000BASE-X register set (primary SerDes)
>  		 */
> -		reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
> -		bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
> -				     reg | BCM5482_SHD_MODE_1000BX);
> +		reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
> +		bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
> +				     reg | BCM54XX_SHD_MODE_1000BX);
>  
>  		/*
>  		 * LED1=ACTIVITYLED, LED3=LINKSPD[2]
> @@ -409,7 +409,7 @@ static int bcm5482_config_init(struct phy_device *phydev)
>  	return err;
>  }
>  
> -static int bcm5482_read_status(struct phy_device *phydev)
> +static int bcm54xx_read_status(struct phy_device *phydev)
>  {
>  	int err;
>  
> @@ -451,12 +451,60 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
>  	return ret;
>  }
>  
> +static int bcm54616s_probe(struct phy_device *phydev)
> +{
> +	int val, intf_sel;
> +
> +	val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
> +	if (val < 0)
> +		return val;
> +
> +	/* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
> +	 * is 01b.
> +	 */
> +	intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
> +	if (intf_sel == 1) {
> +		val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
> +		if (val < 0)
> +			return val;
> +
> +		/* Bit 0 of the SerDes 100-FX Control register, when set
> +		 * to 1, sets the MII/RGMII -> 100BASE-FX configuration.
> +		 * When this bit is set to 0, it sets the GMII/RGMII ->
> +		 * 1000BASE-X configuration.
> +		 */
> +		if (!(val & BCM54616S_100FX_MODE))
> +			phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bcm54616s_config_aneg_1000bx(struct phy_device *phydev)
> +{
> +	int err;
> +	int adv = 0;
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +			      phydev->supported))
> +		adv |= ADVERTISE_1000XFULL;
> +
> +	err = phy_modify_changed(phydev, MII_ADVERTISE, 0, adv);

The "0" parameter is wrong, it must be ADVERTISE_1000XFULL.
First you reset the bit, and then you set it or not.

> +	if (err > 0)
> +		err = genphy_restart_aneg(phydev);
> +
> +	return err;
> +}
> +
>  static int bcm54616s_config_aneg(struct phy_device *phydev)
>  {
>  	int ret;
>  
>  	/* Aneg firsly. */
> -	ret = genphy_config_aneg(phydev);
> +	if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
> +		ret = bcm54616s_config_aneg_1000bx(phydev);
> +	else
> +		ret = genphy_config_aneg(phydev);
>  
>  	/* Then we can set up the delay. */
>  	bcm54xx_config_clock_delay(phydev);
> @@ -655,6 +703,8 @@ static struct phy_driver broadcom_drivers[] = {
>  	.config_aneg	= bcm54616s_config_aneg,
>  	.ack_interrupt	= bcm_phy_ack_intr,
>  	.config_intr	= bcm_phy_config_intr,
> +	.read_status	= bcm54xx_read_status,

If you use aneg, you should also read what was negotiated.
But this function reads neither negotiated duplex mode nor
pause settings.

> +	.probe		= bcm54616s_probe,
>  }, {
>  	.phy_id		= PHY_ID_BCM5464,
>  	.phy_id_mask	= 0xfffffff0,
> @@ -689,7 +739,7 @@ static struct phy_driver broadcom_drivers[] = {
>  	.name		= "Broadcom BCM5482",
>  	/* PHY_GBIT_FEATURES */
>  	.config_init	= bcm5482_config_init,
> -	.read_status	= bcm5482_read_status,
> +	.read_status	= bcm54xx_read_status,
>  	.ack_interrupt	= bcm_phy_ack_intr,
>  	.config_intr	= bcm_phy_config_intr,
>  }, {
> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
> index 6db2d9a6e503..b475e7f20d28 100644
> --- a/include/linux/brcmphy.h
> +++ b/include/linux/brcmphy.h
> @@ -200,9 +200,15 @@
>  #define BCM5482_SHD_SSD		0x14	/* 10100: Secondary SerDes control */
>  #define BCM5482_SHD_SSD_LEDM	0x0008	/* SSD LED Mode enable */
>  #define BCM5482_SHD_SSD_EN	0x0001	/* SSD enable */
> -#define BCM5482_SHD_MODE	0x1f	/* 11111: Mode Control Register */
> -#define BCM5482_SHD_MODE_1000BX	0x0001	/* Enable 1000BASE-X registers */
>  
> +/* 10011: SerDes 100-FX Control Register */
> +#define BCM54616S_SHD_100FX_CTRL	0x13
> +#define	BCM54616S_100FX_MODE		BIT(0)	/* 100-FX SerDes Enable */
> +
> +/* 11111: Mode Control Register */
> +#define BCM54XX_SHD_MODE		0x1f
> +#define BCM54XX_SHD_INTF_SEL_MASK	GENMASK(2, 1)	/* INTERF_SEL[1:0] */
> +#define BCM54XX_SHD_MODE_1000BX		BIT(0)	/* Enable 1000-X registers */
>  
>  /*
>   * EXPANSION SHADOW ACCESS REGISTERS.  (PHY REG 0x15, 0x16, and 0x17)
> 

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 8/6/19 3:00 PM, Heiner Kallweit wrote:
> On 06.08.2019 23:09, Tao Ren wrote:
>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>> example, on Facebook CMM BMC platform), mainly because genphy functions
>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>> needs to be handled differently.
>>
>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>> driver callbacks:
>>
>>   - probe: probe callback detects PHY's operation mode based on
>>     INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>     Control register.
>>
>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>>     negotiation in 1000Base-X mode.
>>
>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>     callback which manually set link speed and duplex mode in 1000Base-X
>>     mode.
>>
>> Signed-off-by: Tao Ren <taoren@fb.com>
>> ---
>>  Changes in v4:
>>   - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
>>     1000Base-X mode.
>>  Changes in v3:
>>   - rename bcm5482_read_status to bcm54xx_read_status so the callback can
>>     be shared by BCM5482 and BCM54616S.
>>  Changes in v2:
>>   - Auto-detect PHY operation mode instead of passing DT node.
>>   - move PHY mode auto-detect logic from config_init to probe callback.
>>   - only set speed (not including duplex) in read_status callback.
>>   - update patch description with more background to avoid confusion.
>>   - patch #1 in the series ("net: phy: broadcom: set features explicitly
>>     for BCM54616") is dropped: the fix should go to get_features callback
>>     which may potentially depend on this patch.
>>
>>  drivers/net/phy/broadcom.c | 62 ++++++++++++++++++++++++++++++++++----
>>  include/linux/brcmphy.h    | 10 ++++--
>>  2 files changed, 64 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>> index 937d0059e8ac..bf61ed8451e5 100644
>> --- a/drivers/net/phy/broadcom.c
>> +++ b/drivers/net/phy/broadcom.c
>> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
>>  		/*
>>  		 * Select 1000BASE-X register set (primary SerDes)
>>  		 */
>> -		reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
>> -		bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
>> -				     reg | BCM5482_SHD_MODE_1000BX);
>> +		reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +		bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
>> +				     reg | BCM54XX_SHD_MODE_1000BX);
>>  
>>  		/*
>>  		 * LED1=ACTIVITYLED, LED3=LINKSPD[2]
>> @@ -409,7 +409,7 @@ static int bcm5482_config_init(struct phy_device *phydev)
>>  	return err;
>>  }
>>  
>> -static int bcm5482_read_status(struct phy_device *phydev)
>> +static int bcm54xx_read_status(struct phy_device *phydev)
>>  {
>>  	int err;
>>  
>> @@ -451,12 +451,60 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
>>  	return ret;
>>  }
>>  
>> +static int bcm54616s_probe(struct phy_device *phydev)
>> +{
>> +	int val, intf_sel;
>> +
>> +	val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	/* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
>> +	 * is 01b.
>> +	 */
>> +	intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
>> +	if (intf_sel == 1) {
>> +		val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
>> +		if (val < 0)
>> +			return val;
>> +
>> +		/* Bit 0 of the SerDes 100-FX Control register, when set
>> +		 * to 1, sets the MII/RGMII -> 100BASE-FX configuration.
>> +		 * When this bit is set to 0, it sets the GMII/RGMII ->
>> +		 * 1000BASE-X configuration.
>> +		 */
>> +		if (!(val & BCM54616S_100FX_MODE))
>> +			phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int bcm54616s_config_aneg_1000bx(struct phy_device *phydev)
>> +{
>> +	int err;
>> +	int adv = 0;
>> +
>> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
>> +			      phydev->supported))
>> +		adv |= ADVERTISE_1000XFULL;
>> +
>> +	err = phy_modify_changed(phydev, MII_ADVERTISE, 0, adv);
> 
> The "0" parameter is wrong, it must be ADVERTISE_1000XFULL.
> First you reset the bit, and then you set it or not.

Got it. Will fix it in patch v5. Thanks.

>> +	if (err > 0)
>> +		err = genphy_restart_aneg(phydev);
>> +
>> +	return err;
>> +}
>> +
>>  static int bcm54616s_config_aneg(struct phy_device *phydev)
>>  {
>>  	int ret;
>>  
>>  	/* Aneg firsly. */
>> -	ret = genphy_config_aneg(phydev);
>> +	if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
>> +		ret = bcm54616s_config_aneg_1000bx(phydev);
>> +	else
>> +		ret = genphy_config_aneg(phydev);
>>  
>>  	/* Then we can set up the delay. */
>>  	bcm54xx_config_clock_delay(phydev);
>> @@ -655,6 +703,8 @@ static struct phy_driver broadcom_drivers[] = {
>>  	.config_aneg	= bcm54616s_config_aneg,
>>  	.ack_interrupt	= bcm_phy_ack_intr,
>>  	.config_intr	= bcm_phy_config_intr,
>> +	.read_status	= bcm54xx_read_status,
> 
> If you use aneg, you should also read what was negotiated.
> But this function reads neither negotiated duplex mode nor
> pause settings.

Let me see how to fix it.. Will come back soon..


Thanks,

Tao