Message-ID: <9b9ba199-8379-0840-b99a-d729f8ad33e1@ti.com>
Date:   Tue, 21 Mar 2023 19:04:50 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
CC:     <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <rogerq@kernel.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <srk@ti.com>,
        <s-vadapalli@ti.com>
Subject: Re: [PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: Add support
 for SGMII mode
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
References: <20230321111958.2800005-1-s-vadapalli@ti.com>
 <20230321111958.2800005-3-s-vadapalli@ti.com>
 <ZBmVGu2vf1ADmEuN@shell.armlinux.org.uk>
Content-Language: en-US
From:   Siddharth Vadapalli <s-vadapalli@ti.com>
In-Reply-To: <ZBmVGu2vf1ADmEuN@shell.armlinux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

> 
> 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.

> 
>>  #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.

Regards,
Siddharth.