Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <9d8250a9-8e00-4233-8bd9-9e49bd15fac5@quicinc.com>
Date:   Mon, 11 Dec 2023 14:10:54 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net v3] net: stmmac: update Rx clk divider for 10M SGMII
Content-Language: en-US
To:     Bjorn Andersson <andersson@kernel.org>
CC:     Vinod Koul <vkoul@kernel.org>,
        Bhupesh Sharma <bhupesh.sharma@linaro.org>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Jose Abreu <joabreu@synopsys.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        <netdev@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <kernel@quicinc.com>,
        Andrew Halaney <ahalaney@redhat.com>
References: <20231208062502.13124-1-quic_snehshah@quicinc.com>
 <5luxwdjyzkg5o6w27mqixggr65ebosnn53vaqrbtsclfudet4v@kse23pgyj7ld>
From:   Sneh Shah <quic_snehshah@quicinc.com>
In-Reply-To: <5luxwdjyzkg5o6w27mqixggr65ebosnn53vaqrbtsclfudet4v@kse23pgyj7ld>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 12/9/2023 9:54 AM, Bjorn Andersson wrote:
> On Fri, Dec 08, 2023 at 11:55:02AM +0530, Sneh Shah wrote:
>> SGMII 10MBPS mode needs RX clock divider to avoid drops in Rx.
>> Update configure SGMII function with rx clk divider programming.
> 
> Are you trying saying that the RX clock is completely wrong in 10MBps
> mode? Or is the RX clock good, but without some division of some other
> clock signal you loose some packets now and then?

Without the divider, Rx clock is completely wrong. All the packets will dropped.  
> 
> Please write your commit message such that it describe the actual
> problem you're having. This will help others to know if this fix is
> applicable to some issue they are seeing on their hardware, now and in
> the future.
> 
Rx clock divider is must for 10M SGMII to work for my hardware. Hence, It was mentioned that SGMII needs Rx clock divider for 10MBPS to work.
Will update the commit message to emphasize on how rx clock is completely wrong without this.
>>
>> Fixes: 463120c31c58 ("net: stmmac: dwmac-qcom-ethqos: add support for SGMII")
>> Signed-off-by: Sneh Shah <quic_snehshah@quicinc.com>
>> ---
>> v3 changelog:
>> - Added comment to explain why MAC needs to be reconfigured for SGMII
>> v2 changelog:
>> - Use FIELD_PREP to prepare bifield values in place of GENMASK
>> - Add fixes tag
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> index d3bf42d0fceb..ab2245995bc6 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> @@ -34,6 +34,7 @@
>>  #define RGMII_CONFIG_LOOPBACK_EN		BIT(2)
>>  #define RGMII_CONFIG_PROG_SWAP			BIT(1)
>>  #define RGMII_CONFIG_DDR_MODE			BIT(0)
>> +#define RGMII_CONFIG_SGMII_CLK_DVDR		GENMASK(18, 10)
> 
> This new bitfield overlaps with existing fields, is it the same
> register? Did the fields move? Is it applicable to all versions?
> 
It is same register but bitfield is moved for new SGMII based HW version. Not applicable to older version. few bitfields of older HW version's RGMII register are repurposed in the new hw version for SGMII config.

> What will existing writes to RGMII_IO_MACRO_CONFIG do to this new
> layout? What will the new write do to hardware with the existing field
> layout?
> 
we have two functions to configure MAC IOMACRO interface.
1. ethqos_configure_sgmii( this used this new bifield) 2. ethqos_configure_rgmii ( this used old layout)
Old HW version uses ethqos_configure_rgmii only. New HW version used ethqos_configure_sgmii only.

Old HW version is not going to use this new layout. New HW version is not going to use old layout configuration.

>>  
>>  /* SDCC_HC_REG_DLL_CONFIG fields */
>>  #define SDCC_DLL_CONFIG_DLL_RST			BIT(30)
>> @@ -598,6 +599,9 @@ static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos)
>>  	return 0;
>>  }
>>  
>> +/* On interface toggle MAC registetrs gets reset.
>> + * Configure MAC block for SGMII on ethernet phy link up
>> + */
>>  static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos)
>>  {
>>  	int val;
>> @@ -617,6 +621,9 @@ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos)
>>  	case SPEED_10:
>>  		val |= ETHQOS_MAC_CTRL_PORT_SEL;
>>  		val &= ~ETHQOS_MAC_CTRL_SPEED_MODE;
>> +		rgmii_updatel(ethqos, RGMII_CONFIG_SGMII_CLK_DVDR,
>> +			      FIELD_PREP(RGMII_CONFIG_SGMII_CLK_DVDR, 0x31),
> 
> Is this just a magic constant, or does 0x31 of some convenient unit?
> Could we give it name/define?
> 
It is just a constant based on programming guide of HW.  Will add a #define for it.
> Regards,
> Bjorn
> 
>> +			      RGMII_IO_MACRO_CONFIG);
>>  		break;
>>  	}
>>  
>> -- 
>> 2.17.1
>>
>>