Re: [PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: Add support for SGMII mode

[Siddharth Vadapalli <s-vadapalli@ti.com>]

Hello Russell,

On 21-03-2023 21:08, Russell King (Oracle) wrote:
> On Tue, Mar 21, 2023 at 07:04:50PM +0530, Siddharth Vadapalli wrote:
>> Hello Russell,
>>
>> On 21-03-2023 16:59, Russell King (Oracle) wrote:
>>> On Tue, Mar 21, 2023 at 04:49:56PM +0530, Siddharth Vadapalli wrote:
>>>> Add support for configuring the CPSW Ethernet Switch in SGMII mode.
>>>>
>>>> Depending on the SoC, allow selecting SGMII mode as a supported interface,
>>>> based on the compatible used.
>>>>
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>> ---
>>>>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 11 ++++++++++-
>>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> index cba8db14e160..d2ca1f2035f4 100644
>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> @@ -76,6 +76,7 @@
>>>>  #define AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2       0x31C
>>>>  
>>>>  #define AM65_CPSW_SGMII_CONTROL_REG		0x010
>>>> +#define AM65_CPSW_SGMII_MR_ADV_ABILITY_REG	0x018
>>>>  #define AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE	BIT(0)
>>>
>>> Isn't this misplaced? Shouldn't AM65_CPSW_SGMII_MR_ADV_ABILITY_REG come
>>> after AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, rather than splitting that
>>> from its register offset definition?
>>
>> Thank you for reviewing the patch. The registers are as follows:
>> CONTROL_REG offset 0x10
>> STATUS_REG offset  0x14
>> MR_ADV_REG offset  0x18
>>
>> Since the STATUS_REG is not used in the driver, its offset is omitted.
>> The next register is the MR_ADV_REG, which I placed after the
>> CONTROL_REG. I grouped the register offsets together, to represent the
>> order in which the registers are placed. Due to this, the
>> MR_ADV_ABILITY_REG offset is placed after the CONTROL_REG offset define.
>>
>> Please let me know if I should move it after the CONTROL_MR_AN_ENABLE
>> define instead.
> 
> Well, it's up to you - whether you wish to group the register offsets
> separately from the bit definitions for those registers, or whether
> you wish to describe the register offset and its associated bit
> definitions in one group before moving on to the next register.
> 
>>> If the advertisement register is at 0x18, and the lower 16 bits is the
>>> advertisement, are the link partner advertisement found in the upper
>>> 16 bits?
>>
>> The MR_LP_ADV_ABILITY_REG is at offset 0x020, which is the the register
>> corresponding to the Link Partner advertised value. Also, the
>> AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE Bit is in the CONTROL_REG. The CPSW
>> Hardware specification describes the process of configuring the CPSW MAC
>> for SGMII mode as follows:
>> 1. Write 0x1 (ADVERTISE_SGMII) to the MR_ADV_ABILITY_REG register.
>> 2. Enable auto-negotiation in the CONTROL_REG by setting the
>> AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE bit.
> 
> Good to hear that there is a link partner register.
> 
>>>>  #define AM65_CPSW_CTL_VLAN_AWARE		BIT(1)
>>>> @@ -1496,9 +1497,14 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
>>>>  	struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
>>>>  	struct am65_cpsw_common *common = port->common;
>>>>  
>>>> -	if (common->pdata.extra_modes & BIT(state->interface))
>>>> +	if (common->pdata.extra_modes & BIT(state->interface)) {
>>>> +		if (state->interface == PHY_INTERFACE_MODE_SGMII)
>>>> +			writel(ADVERTISE_SGMII,
>>>> +			       port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
>>>> +
>>>
>>> I think we can do better with this, by implementing proper PCS support.
>>>
>>> It seems manufacturers tend to use bought-in IP for this, so have a
>>> look at drivers/net/pcs/ to see whether any of those (or the one in
>>> the Mediatek patch set on netdev that has recently been applied) will
>>> idrive your hardware.
>>>
>>> However, given the definition of AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
>>> I suspect you won't find a compatible implementation.
>>
>> I have tested with an SGMII Ethernet PHY in the standard SGMII MAC2PHY
>> configuration. I am not sure if PCS support will be required or not. I
>> hope that the information shared above by me regarding the CPSW
>> Hardware's specification for configuring it in SGMII mode will help
>> determine what the right approach might be. Please let me know whether
>> the current implementation is acceptable or PCS support is necessary.
> 
> Nevertheless, this SGMII block is a PCS, and if you're going to want to
> support inband mode (e.g. to read the SGMII word from the PHY), or if
> someone ever wants to use 1000base-X, you're going to need to implement
> this properly as a PCS.
> 
> That said, it can be converted later, so isn't a blocking sisue.

Thank you for clarifying. I will work on converting it to PCS in a
future series.

Regards,
Siddharth.