Re: [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S

[Florian Fainelli <f.fainelli@gmail.com>]

On 2/12/2021 5:45 PM, Robert Hancock wrote:
> On Fri, 2021-02-12 at 17:26 -0800, Florian Fainelli wrote:
>>
>> On 2/12/2021 4:28 PM, 'Robert Hancock' via BCM-KERNEL-FEEDBACK-LIST,PDL
>> wrote:
>>> The default configuration for the BCM54616S PHY may not match the desired
>>> mode when using 1000BaseX or SGMII interface modes, such as when it is on
>>> an SFP module. Add code to explicitly set the correct mode using
>>> programming sequences provided by Bel-Fuse:
>>>
>>> https://urldefense.com/v3/__https://www.belfuse.com/resources/datasheets/powersolutions/ds-bps-sfp-1gbt-05-series.pdf__;!!IOGos0k!20FhZqRHEiz2-qFJ7J8XC4xX2qG-ajZ17Ma1W-VwDgwdQZeIhHEpWKlNldWW8DyFaQo$ 
>>> https://urldefense.com/v3/__https://www.belfuse.com/resources/datasheets/powersolutions/ds-bps-sfp-1gbt-06-series.pdf__;!!IOGos0k!20FhZqRHEiz2-qFJ7J8XC4xX2qG-ajZ17Ma1W-VwDgwdQZeIhHEpWKlNldWW58K3fY4$ 
>>>
>>> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
>>> ---
>>>  drivers/net/phy/broadcom.c | 83 ++++++++++++++++++++++++++++++++------
>>>  include/linux/brcmphy.h    |  4 ++
>>>  2 files changed, 75 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>>> index 0472b3470c59..78542580f2b2 100644
>>> --- a/drivers/net/phy/broadcom.c
>>> +++ b/drivers/net/phy/broadcom.c
>>> @@ -64,6 +64,63 @@ static int bcm54612e_config_init(struct phy_device
>>> *phydev)
>>>  	return 0;
>>>  }
>>>  
>>> +static int bcm54616s_config_init(struct phy_device *phydev)
>>> +{
>>> +	int rc, val;
>>> +
>>> +	if (phydev->interface == PHY_INTERFACE_MODE_SGMII ||
>>> +	    phydev->interface == PHY_INTERFACE_MODE_1000BASEX) {
>>
>> Can you reverse the condition so as to save a level of identation?
> 
> Can do.
> 
>>
>>> +		/* Ensure proper interface mode is selected. */
>>> +		/* Disable RGMII mode */
>>> +		val = bcm54xx_auxctl_read(phydev,
>>> MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
>>> +		if (val < 0)
>>> +			return val;
>>> +		val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_EN;
>>> +		rc = bcm54xx_auxctl_write(phydev,
>>> MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
>>> +					  val);
>>> +		if (rc < 0)
>>> +			return rc;
>>
>> I don't think this write is making it through since you are not setting
>> MII_BCM54XX_AUXCTL_MISC_WREN in val, I know this is an annoying detail,
>> and we could probably fold that to be within bcm54xx_auxctl_write()
>> directly, similarly to what bcm_phy_write_shadow() does.
> 
> Ah, indeed. I assume that is specific to the MII_BCM54XX_AUXCTL_SHDWSEL_MISC
> register? I suppose bcm54xx_auxctl_write could add that automatically for
> writes to that register. Not sure if that is too much magic for that function
> or not..

Upon closer look it's a bit more subtle than that, as we need to apply
the write enable bit only when targeting the "misc" shadow register, a
million ways to die in the west :)
-- 
Florian