[net] net: stmmac: update Rx clk divider for 10M SGMII

  SGMII 10MBPS mode needs RX clock divider to avoid drops in Rx.
Update configure SGMII function with rx clk divider programming.

Signed-off-by: Sneh Shah <quic_snehshah@quicinc.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 3 +++
 1 file changed, 3 insertions(+)
  

On 24-11-23, 10:38, Sneh Shah wrote:
> SGMII 10MBPS mode needs RX clock divider to avoid drops in Rx.
> Update configure SGMII function with rx clk divider programming.

Acked-by: Vinod Koul <vkoul@kernel.org>
  

Hi Sneh,

>----------------------------------------------------------------------
>SGMII 10MBPS mode needs RX clock divider to avoid drops in Rx.
>Update configure SGMII function with rx clk divider programming.
>
[Suman] You need to add the Fixes tag as well.
>Signed-off-by: Sneh Shah <quic_snehshah@quicinc.com>
>---
> drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 3 +++
> 1 file changed, 3 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..f8c42e91a624 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)
>
> /* SDCC_HC_REG_DLL_CONFIG fields */
> #define SDCC_DLL_CONFIG_DLL_RST			BIT(30)
>@@ -617,6 +618,8 @@ 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, BIT(10) |
>+			      GENMASK(15, 14), RGMII_IO_MACRO_CONFIG);
> 		break;
> 	}
>
>--
>2.17.1
>
  

On Fri, Nov 24, 2023 at 10:38:18AM +0530, Sneh Shah wrote:
>  #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)

So you're saying here that this is a 9 bit field...

> @@ -617,6 +618,8 @@ 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, BIT(10) |
> +			      GENMASK(15, 14), RGMII_IO_MACRO_CONFIG);

... and then you use GENMASK(15,14) | BIT(10) here to set bits in that
bitfield. If there are multiple bitfields, then these should be defined
separately and the mask built up.

I suspect that they aren't, and you're using this to generate a _value_
that has bits 5, 4, and 0 set for something that really takes a _value_.
So, FIELD_PREP(RGMII_CONFIG_SGMII_CLK_DVDR, 0x31) or
FIELD_PREP(RGMII_CONFIG_SGMII_CLK_DVDR, 49) would be entirely correct
here.

The next concern I have is that you're only doing this for SPEED_10.
If it needs to be programmed for SPEED_10 to work, and not any of the
other speeds, isn't this something that can be done at initialisation
time? If it has to be done depending on the speed, then don't you need
to do this for each speed with an appropriate value?
  

You are right here for GENMASK(15,14) | BIT(10). I am using this to create a field value.I will switch to FIELD_PREP as that seems like a better way to do this.

This field programming is required only for 10M speed in for SGMII mode. other speeds are agnostic to this field. Hence we are programming it always when SGMII link comes up in 10M mode. init driver data for ethqos is common for sgmii and rgmii. As this fix is specific to SGMII we can't add this to init driver data.

On 11/24/2023 2:42 PM, Russell King (Oracle) wrote:
> On Fri, Nov 24, 2023 at 10:38:18AM +0530, Sneh Shah wrote:
>>  #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)
> 
> So you're saying here that this is a 9 bit field...
> 
>> @@ -617,6 +618,8 @@ 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, BIT(10) |
>> +			      GENMASK(15, 14), RGMII_IO_MACRO_CONFIG);
> 
> ... and then you use GENMASK(15,14) | BIT(10) here to set bits in that
> bitfield. If there are multiple bitfields, then these should be defined
> separately and the mask built up.
> 
> I suspect that they aren't, and you're using this to generate a _value_
> that has bits 5, 4, and 0 set for something that really takes a _value_.
> So, FIELD_PREP(RGMII_CONFIG_SGMII_CLK_DVDR, 0x31) or
> FIELD_PREP(RGMII_CONFIG_SGMII_CLK_DVDR, 49) would be entirely correct
> here.
> 
> The next concern I have is that you're only doing this for SPEED_10.
> If it needs to be programmed for SPEED_10 to work, and not any of the
> other speeds, isn't this something that can be done at initialisation
> time? If it has to be done depending on the speed, then don't you need
> to do this for each speed with an appropriate value?
>
  

Please reply _inline_ rather than at the top of the message, just like
every other email that is sent in the Linux community. It is actually
the _Internet_ standard way of replying, before people like Microsoft
encouraged your broken style.

Also wrapping the text of your message makes it easier.

On Mon, Nov 27, 2023 at 11:25:34AM +0530, Sneh Shah wrote:
> On 11/24/2023 2:42 PM, Russell King (Oracle) wrote:
> > On Fri, Nov 24, 2023 at 10:38:18AM +0530, Sneh Shah wrote:
> >>  #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)
> > 
> > So you're saying here that this is a 9 bit field...
> > 
> >> @@ -617,6 +618,8 @@ 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, BIT(10) |
> >> +			      GENMASK(15, 14), RGMII_IO_MACRO_CONFIG);
> > 
> > ... and then you use GENMASK(15,14) | BIT(10) here to set bits in that
> > bitfield. If there are multiple bitfields, then these should be defined
> > separately and the mask built up.
> > 
> > I suspect that they aren't, and you're using this to generate a _value_
> > that has bits 5, 4, and 0 set for something that really takes a _value_.
> > So, FIELD_PREP(RGMII_CONFIG_SGMII_CLK_DVDR, 0x31) or
> > FIELD_PREP(RGMII_CONFIG_SGMII_CLK_DVDR, 49) would be entirely correct
> > here.
> 
> You are right here for GENMASK(15,14) | BIT(10). I am using this to create a field value.I will switch to FIELD_PREP as that seems like a better way to do this.

So this is a "nice" example of taking the use of GENMASK() and BIT() to
an inappropriate case.

> > The next concern I have is that you're only doing this for SPEED_10.
> > If it needs to be programmed for SPEED_10 to work, and not any of the
> > other speeds, isn't this something that can be done at initialisation
> > time? If it has to be done depending on the speed, then don't you need
> > to do this for each speed with an appropriate value?
> 
> This field programming is required only for 10M speed in for SGMII mode. other speeds are agnostic to this field. Hence we are programming it always when SGMII link comes up in 10M mode. init driver data for ethqos is common for sgmii and rgmii. As this fix is specific to SGMII we can't add this to init driver data.

I wasn't referring to adding it to driver data. I was asking whether it
could be done in the initialisation path.
  

On 11/27/2023 2:09 PM, Russell King (Oracle) wrote:
> Please reply _inline_ rather than at the top of the message, just like
> every other email that is sent in the Linux community. It is actually
> the _Internet_ standard way of replying, before people like Microsoft
> encouraged your broken style.
> 
> Also wrapping the text of your message makes it easier.

Noted. Going forward will make sure to reply inline.

> 
> On Mon, Nov 27, 2023 at 11:25:34AM +0530, Sneh Shah wrote:
>> On 11/24/2023 2:42 PM, Russell King (Oracle) wrote:
>>> On Fri, Nov 24, 2023 at 10:38:18AM +0530, Sneh Shah wrote:
>>>>  #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)
>>>
>>> So you're saying here that this is a 9 bit field...
>>>
>>>> @@ -617,6 +618,8 @@ 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, BIT(10) |
>>>> +			      GENMASK(15, 14), RGMII_IO_MACRO_CONFIG);
>>>
>>> ... and then you use GENMASK(15,14) | BIT(10) here to set bits in that
>>> bitfield. If there are multiple bitfields, then these should be defined
>>> separately and the mask built up.
>>>
>>> I suspect that they aren't, and you're using this to generate a _value_
>>> that has bits 5, 4, and 0 set for something that really takes a _value_.
>>> So, FIELD_PREP(RGMII_CONFIG_SGMII_CLK_DVDR, 0x31) or
>>> FIELD_PREP(RGMII_CONFIG_SGMII_CLK_DVDR, 49) would be entirely correct
>>> here.
>>
>> You are right here for GENMASK(15,14) | BIT(10). I am using this to create a field value.I will switch to FIELD_PREP as that seems like a better way to do this.
> 
> So this is a "nice" example of taking the use of GENMASK() and BIT() to
> an inappropriate case.
> 
>>> The next concern I have is that you're only doing this for SPEED_10.
>>> If it needs to be programmed for SPEED_10 to work, and not any of the
>>> other speeds, isn't this something that can be done at initialisation
>>> time? If it has to be done depending on the speed, then don't you need
>>> to do this for each speed with an appropriate value?
>>
>> This field programming is required only for 10M speed in for SGMII mode. other speeds are agnostic to this field. Hence we are programming it always when SGMII link comes up in 10M mode. init driver data for ethqos is common for sgmii and rgmii. As this fix is specific to SGMII we can't add this to init driver data.
> 
> I wasn't referring to adding it to driver data. I was asking whether it
> could be done in the initialisation path.
> 
No, IOMACRO block is configured post phylink up regardless of RGMII or SGMII mode. We are not updating them at driver initialization time itself.
  

On Mon, Nov 27, 2023 at 03:17:20PM +0530, Sneh Shah wrote:
> On 11/27/2023 2:09 PM, Russell King (Oracle) wrote:
> > On Mon, Nov 27, 2023 at 11:25:34AM +0530, Sneh Shah wrote:
> >> On 11/24/2023 2:42 PM, Russell King (Oracle) wrote:
> >>> The next concern I have is that you're only doing this for SPEED_10.
> >>> If it needs to be programmed for SPEED_10 to work, and not any of the
> >>> other speeds, isn't this something that can be done at initialisation
> >>> time? If it has to be done depending on the speed, then don't you need
> >>> to do this for each speed with an appropriate value?
> >>
> >> This field programming is required only for 10M speed in for SGMII mode. other speeds are agnostic to this field. Hence we are programming it always when SGMII link comes up in 10M mode. init driver data for ethqos is common for sgmii and rgmii. As this fix is specific to SGMII we can't add this to init driver data.
> > 
> > I wasn't referring to adding it to driver data. I was asking whether it
> > could be done in the initialisation path.
> > 
> No, IOMACRO block is configured post phylink up regardless of RGMII or SGMII mode. We are not updating them at driver initialization time itself.

What reason (in terms of the hardware) requires you to do this every
time you select 10M speed? Does the hardware change the value in the
register?
  

On 11/27/2023 3:35 PM, Russell King (Oracle) wrote:
> On Mon, Nov 27, 2023 at 03:17:20PM +0530, Sneh Shah wrote:
>> On 11/27/2023 2:09 PM, Russell King (Oracle) wrote:
>>> On Mon, Nov 27, 2023 at 11:25:34AM +0530, Sneh Shah wrote:
>>>> On 11/24/2023 2:42 PM, Russell King (Oracle) wrote:
>>>>> The next concern I have is that you're only doing this for SPEED_10.
>>>>> If it needs to be programmed for SPEED_10 to work, and not any of the
>>>>> other speeds, isn't this something that can be done at initialisation
>>>>> time? If it has to be done depending on the speed, then don't you need
>>>>> to do this for each speed with an appropriate value?
>>>>
>>>> This field programming is required only for 10M speed in for SGMII mode. other speeds are agnostic to this field. Hence we are programming it always when SGMII link comes up in 10M mode. init driver data for ethqos is common for sgmii and rgmii. As this fix is specific to SGMII we can't add this to init driver data.
>>>
>>> I wasn't referring to adding it to driver data. I was asking whether it
>>> could be done in the initialisation path.
>>>
>> No, IOMACRO block is configured post phylink up regardless of RGMII or SGMII mode. We are not updating them at driver initialization time itself.
> 
> What reason (in terms of the hardware) requires you to do this every
> time you select 10M speed? Does the hardware change the value in the
> register?
> 
Yes, the hardware changes the value in register every time the interface is toggled. That is the reason we have ethqos_configure_sgmii function to configure registers whenever there is link activity.
  

On Wed, Nov 29, 2023 at 04:56:53PM +0530, Sneh Shah wrote:
> 
> 
> On 11/27/2023 3:35 PM, Russell King (Oracle) wrote:
> > On Mon, Nov 27, 2023 at 03:17:20PM +0530, Sneh Shah wrote:
> >> On 11/27/2023 2:09 PM, Russell King (Oracle) wrote:
> >>> On Mon, Nov 27, 2023 at 11:25:34AM +0530, Sneh Shah wrote:
> >>>> On 11/24/2023 2:42 PM, Russell King (Oracle) wrote:
> >>>>> The next concern I have is that you're only doing this for SPEED_10.
> >>>>> If it needs to be programmed for SPEED_10 to work, and not any of the
> >>>>> other speeds, isn't this something that can be done at initialisation
> >>>>> time? If it has to be done depending on the speed, then don't you need
> >>>>> to do this for each speed with an appropriate value?
> >>>>
> >>>> This field programming is required only for 10M speed in for SGMII mode. other speeds are agnostic to this field. Hence we are programming it always when SGMII link comes up in 10M mode. init driver data for ethqos is common for sgmii and rgmii. As this fix is specific to SGMII we can't add this to init driver data.
> >>>
> >>> I wasn't referring to adding it to driver data. I was asking whether it
> >>> could be done in the initialisation path.
> >>>
> >> No, IOMACRO block is configured post phylink up regardless of RGMII or SGMII mode. We are not updating them at driver initialization time itself.
> > 
> > What reason (in terms of the hardware) requires you to do this every
> > time you select 10M speed? Does the hardware change the value in the
> > register?
> > 
> Yes, the hardware changes the value in register every time the interface is toggled. That is the reason we have ethqos_configure_sgmii function to configure registers whenever there is link activity.

That is sufficient reason to write it each time - and it would be good
to mention this in a comment above the write in
ethqos_configure_sgmii().

Thanks.