diff mbox

[3/3] net: stmmac: Use AVB mode by default

Message ID 20170321151211.31841-3-thierry.reding@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Thierry Reding March 21, 2017, 3:12 p.m. UTC 
From: Thierry Reding <treding@nvidia.com>

Prior to the recent multi-queue changes the driver would configure the
queues to use the AVB mode, but the mode then got switched to DCB. The
hardware still works fine in DCB mode, but my testing capabilities are
limited, so it's safer to revert to the prior setting anyway.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/linux/stmmac.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Joao Pinto March 21, 2017, 3:23 p.m. UTC | #1 
Às 3:12 PM de 3/21/2017, Thierry Reding escreveu:
> From: Thierry Reding <treding@nvidia.com>
> 
> Prior to the recent multi-queue changes the driver would configure the
> queues to use the AVB mode, but the mode then got switched to DCB. The
> hardware still works fine in DCB mode, but my testing capabilities are
> limited, so it's safer to revert to the prior setting anyway.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  include/linux/stmmac.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index be47b859e954..8349a5c1537b 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -56,8 +56,8 @@
>  #define MTL_RX_ALGORITHM_WSP	0x5
>  
>  /* RX/TX Queue Mode */
> -#define MTL_QUEUE_DCB		0x0
> -#define MTL_QUEUE_AVB		0x1
> +#define MTL_QUEUE_AVB		0x0
> +#define MTL_QUEUE_DCB		0x1
>  
>  /* The MDC clock could be set higher than the IEEE 802.3
>   * specified frequency limit 0f 2.5 MHz, by programming a clock divider
> 

Thierry, I don't understand this patch. It will have 0 impact.

In stmmac_platform configuration, 0 impact:

		if (of_property_read_bool(q_node, "snps,dcb-algorithm"))
			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
		else if (of_property_read_bool(q_node, "snps,avb-algorithm"))
			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
		else
**			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;

In dwmac4_core, 0 impact:

	value &= GMAC_RX_QUEUE_CLEAR(queue);
	if (mode == MTL_QUEUE_AVB)
		value |= GMAC_RX_AV_QUEUE_ENABLE(queue);
	else if (mode == MTL_QUEUE_DCB)
		value |= GMAC_RX_DCB_QUEUE_ENABLE(queue);

I think you should set the default mode in (**).

Thanks.
Thierry Reding March 21, 2017, 4:42 p.m. UTC | #2 
On Tue, Mar 21, 2017 at 03:23:00PM +0000, Joao Pinto wrote:
> Às 3:12 PM de 3/21/2017, Thierry Reding escreveu:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Prior to the recent multi-queue changes the driver would configure the
> > queues to use the AVB mode, but the mode then got switched to DCB. The
> > hardware still works fine in DCB mode, but my testing capabilities are
> > limited, so it's safer to revert to the prior setting anyway.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  include/linux/stmmac.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> > index be47b859e954..8349a5c1537b 100644
> > --- a/include/linux/stmmac.h
> > +++ b/include/linux/stmmac.h
> > @@ -56,8 +56,8 @@
> >  #define MTL_RX_ALGORITHM_WSP	0x5
> >  
> >  /* RX/TX Queue Mode */
> > -#define MTL_QUEUE_DCB		0x0
> > -#define MTL_QUEUE_AVB		0x1
> > +#define MTL_QUEUE_AVB		0x0
> > +#define MTL_QUEUE_DCB		0x1
> >  
> >  /* The MDC clock could be set higher than the IEEE 802.3
> >   * specified frequency limit 0f 2.5 MHz, by programming a clock divider
> > 
> 
> Thierry, I don't understand this patch. It will have 0 impact.
> 
> In stmmac_platform configuration, 0 impact:
> 
> 		if (of_property_read_bool(q_node, "snps,dcb-algorithm"))
> 			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
> 		else if (of_property_read_bool(q_node, "snps,avb-algorithm"))
> 			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
> 		else
> **			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
> 
> In dwmac4_core, 0 impact:
> 
> 	value &= GMAC_RX_QUEUE_CLEAR(queue);
> 	if (mode == MTL_QUEUE_AVB)
> 		value |= GMAC_RX_AV_QUEUE_ENABLE(queue);
> 	else if (mode == MTL_QUEUE_DCB)
> 		value |= GMAC_RX_DCB_QUEUE_ENABLE(queue);
> 
> I think you should set the default mode in (**).

That was my initial attempt, but then I realized that for old DTBs,
stmmac_mtl_setup() will already exit prematurely because of the missing
snps,mtl-{rx,tx}-config properties. It's pretty much for the same reason
as the separate assignment of the default {rx,tx}_queues_to_use. In this
case it's somewhat more obfuscated, though. Changing AVB to be mode 0
means that plat->rx_queues_cfg[].mode_to_use will contain AVB as default
because plat is devm_kzalloc()'ed.

Effectively this change makes all queues use AVB by default unless they
are configured using the new device tree bindings.

Thierry
Joao Pinto March 21, 2017, 4:50 p.m. UTC | #3 
Às 4:42 PM de 3/21/2017, Thierry Reding escreveu:
> On Tue, Mar 21, 2017 at 03:23:00PM +0000, Joao Pinto wrote:
>> Às 3:12 PM de 3/21/2017, Thierry Reding escreveu:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Prior to the recent multi-queue changes the driver would configure the
>>> queues to use the AVB mode, but the mode then got switched to DCB. The
>>> hardware still works fine in DCB mode, but my testing capabilities are
>>> limited, so it's safer to revert to the prior setting anyway.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  include/linux/stmmac.h | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
>>> index be47b859e954..8349a5c1537b 100644
>>> --- a/include/linux/stmmac.h
>>> +++ b/include/linux/stmmac.h
>>> @@ -56,8 +56,8 @@
>>>  #define MTL_RX_ALGORITHM_WSP	0x5
>>>  
>>>  /* RX/TX Queue Mode */
>>> -#define MTL_QUEUE_DCB		0x0
>>> -#define MTL_QUEUE_AVB		0x1
>>> +#define MTL_QUEUE_AVB		0x0
>>> +#define MTL_QUEUE_DCB		0x1
>>>  
>>>  /* The MDC clock could be set higher than the IEEE 802.3
>>>   * specified frequency limit 0f 2.5 MHz, by programming a clock divider
>>>
>>
>> Thierry, I don't understand this patch. It will have 0 impact.
>>
>> In stmmac_platform configuration, 0 impact:
>>
>> 		if (of_property_read_bool(q_node, "snps,dcb-algorithm"))
>> 			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
>> 		else if (of_property_read_bool(q_node, "snps,avb-algorithm"))
>> 			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
>> 		else
>> **			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
>>
>> In dwmac4_core, 0 impact:
>>
>> 	value &= GMAC_RX_QUEUE_CLEAR(queue);
>> 	if (mode == MTL_QUEUE_AVB)
>> 		value |= GMAC_RX_AV_QUEUE_ENABLE(queue);
>> 	else if (mode == MTL_QUEUE_DCB)
>> 		value |= GMAC_RX_DCB_QUEUE_ENABLE(queue);
>>
>> I think you should set the default mode in (**).
> 
> That was my initial attempt, but then I realized that for old DTBs,
> stmmac_mtl_setup() will already exit prematurely because of the missing
> snps,mtl-{rx,tx}-config properties. It's pretty much for the same reason
> as the separate assignment of the default {rx,tx}_queues_to_use. In this
> case it's somewhat more obfuscated, though. Changing AVB to be mode 0
> means that plat->rx_queues_cfg[].mode_to_use will contain AVB as default
> because plat is devm_kzalloc()'ed.
> 
> Effectively this change makes all queues use AVB by default unless they
> are configured using the new device tree bindings.

Yes I keep forgeting that :), but you are assuming that
plat->rx_queues_cfg[queue].mode_to_use is 0 by default, which might not be the
case, but I agree with you that this is the simpler approach. Let's see what
others have to say.
> 
> Thierry
>
Joao Pinto March 21, 2017, 4:52 p.m. UTC | #4 
Às 4:50 PM de 3/21/2017, Joao Pinto escreveu:
> Às 4:42 PM de 3/21/2017, Thierry Reding escreveu:
>> On Tue, Mar 21, 2017 at 03:23:00PM +0000, Joao Pinto wrote:
>>> Às 3:12 PM de 3/21/2017, Thierry Reding escreveu:
>>>> From: Thierry Reding <treding@nvidia.com>
>>>>
>>>> Prior to the recent multi-queue changes the driver would configure the
>>>> queues to use the AVB mode, but the mode then got switched to DCB. The
>>>> hardware still works fine in DCB mode, but my testing capabilities are
>>>> limited, so it's safer to revert to the prior setting anyway.
>>>>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>> ---
>>>>  include/linux/stmmac.h | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
>>>> index be47b859e954..8349a5c1537b 100644
>>>> --- a/include/linux/stmmac.h
>>>> +++ b/include/linux/stmmac.h
>>>> @@ -56,8 +56,8 @@
>>>>  #define MTL_RX_ALGORITHM_WSP	0x5
>>>>  
>>>>  /* RX/TX Queue Mode */
>>>> -#define MTL_QUEUE_DCB		0x0
>>>> -#define MTL_QUEUE_AVB		0x1
>>>> +#define MTL_QUEUE_AVB		0x0
>>>> +#define MTL_QUEUE_DCB		0x1
>>>>  
>>>>  /* The MDC clock could be set higher than the IEEE 802.3
>>>>   * specified frequency limit 0f 2.5 MHz, by programming a clock divider
>>>>
>>>
>>> Thierry, I don't understand this patch. It will have 0 impact.
>>>
>>> In stmmac_platform configuration, 0 impact:
>>>
>>> 		if (of_property_read_bool(q_node, "snps,dcb-algorithm"))
>>> 			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
>>> 		else if (of_property_read_bool(q_node, "snps,avb-algorithm"))
>>> 			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
>>> 		else
>>> **			plat->rx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
>>>
>>> In dwmac4_core, 0 impact:
>>>
>>> 	value &= GMAC_RX_QUEUE_CLEAR(queue);
>>> 	if (mode == MTL_QUEUE_AVB)
>>> 		value |= GMAC_RX_AV_QUEUE_ENABLE(queue);
>>> 	else if (mode == MTL_QUEUE_DCB)
>>> 		value |= GMAC_RX_DCB_QUEUE_ENABLE(queue);
>>>
>>> I think you should set the default mode in (**).
>>
>> That was my initial attempt, but then I realized that for old DTBs,
>> stmmac_mtl_setup() will already exit prematurely because of the missing
>> snps,mtl-{rx,tx}-config properties. It's pretty much for the same reason
>> as the separate assignment of the default {rx,tx}_queues_to_use. In this
>> case it's somewhat more obfuscated, though. Changing AVB to be mode 0
>> means that plat->rx_queues_cfg[].mode_to_use will contain AVB as default
>> because plat is devm_kzalloc()'ed.
>>
>> Effectively this change makes all queues use AVB by default unless they
>> are configured using the new device tree bindings.
> 
> Yes I keep forgeting that :), but you are assuming that
> plat->rx_queues_cfg[queue].mode_to_use is 0 by default, which might not be the
> case, but I agree with you that this is the simpler approach. Let's see what
> others have to say.

Forget what I said, yes devm_kzalloc() in plat guarantees this. I need a cup of
coffee :).

Acked-By: Joao Pinto <jpinto@synopsys.com>
>>
>> Thierry
>>
>
David Miller March 22, 2017, 7:15 p.m. UTC | #5 
From: Thierry Reding <thierry.reding@gmail.com>
Date: Tue, 21 Mar 2017 16:12:11 +0100

> From: Thierry Reding <treding@nvidia.com>
> 
> Prior to the recent multi-queue changes the driver would configure the
> queues to use the AVB mode, but the mode then got switched to DCB. The
> hardware still works fine in DCB mode, but my testing capabilities are
> limited, so it's safer to revert to the prior setting anyway.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Applied to net-next
diff mbox

Patch

diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index be47b859e954..8349a5c1537b 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -56,8 +56,8 @@ 
 #define MTL_RX_ALGORITHM_WSP	0x5
 
 /* RX/TX Queue Mode */
-#define MTL_QUEUE_DCB		0x0
-#define MTL_QUEUE_AVB		0x1
+#define MTL_QUEUE_AVB		0x0
+#define MTL_QUEUE_DCB		0x1
 
 /* The MDC clock could be set higher than the IEEE 802.3
  * specified frequency limit 0f 2.5 MHz, by programming a clock divider