[Patch net-next 03/11] net:fec: use multiqueue interface to allocate Ethernet device

[Patch net-next 03/11] net:fec: use multiqueue interface to allocate Ethernet device

[Florian Fainelli]

On 09/03/2014 01:38 PM, Frank Li wrote:
> From: Fugang Duan <B38611@freescale.com>
> 
> Since i.MX6SX enet-AVB IP support multi queues, so use multi queues
> interface to allocate and set up an Ethernet device.
> 
> Signed-off-by: Fugang Duan <B38611@freescale.com>
> Signed-off-by: Frank Li <Frank.Li@freescale.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  9 ++++++
>  drivers/net/ethernet/freescale/fec_main.c | 49 ++++++++++++++++++++++++++++++-
>  2 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 635772b..f77ed6f 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -233,6 +233,13 @@ struct bufdesc_ex {
>  /* This device has up to three irqs on some platforms */
>  #define FEC_IRQ_NUM		3
>  
> +/* Maximum number of queues supported
> + * ENET with AVB IP can support up to 3 independent tx queues and rx queues.
> + * User can point the queue number that is less than or equal to 3.
> + */
> +#define FEC_ENET_MAX_TX_QS	3
> +#define FEC_ENET_MAX_RX_QS	3
> +
>  /* The number of Tx and Rx buffers.  These are allocated from the page
>   * pool.  The code may assume these are power of two, so it it best
>   * to keep them that size.
> @@ -278,6 +285,8 @@ struct fec_enet_private {
>  
>  	bool ptp_clk_on;
>  	struct mutex ptp_clk_mutex;
> +	int num_tx_queues;
> +	int num_rx_queues;

unsigned int.

>  
>  	/* The saved address of a sent-in-place packet/buffer, for skfree(). */
>  	unsigned char *tx_bounce[TX_RING_SIZE];
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index ee9f04f..00fcadd 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2573,6 +2573,39 @@ static void fec_reset_phy(struct platform_device *pdev)
>  #endif /* CONFIG_OF */
>  
>  static int
> +fec_enet_get_queue_num(struct platform_device *pdev, int *num_tx, int *num_rx)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	int err;
> +
> +	if (!np || !of_device_is_available(np))
> +		return -ENODEV;

This effectively breaks non-DT platform probing, is that intentional?
How about assining *num_tx and *num_rx to 1, and if you find the
property, use it?

> +
> +	/* parse the num of tx and rx queues */
> +	err = of_property_read_u32(np, "fsl,num_tx_queues", num_tx);
> +	err |= of_property_read_u32(np, "fsl,num_rx_queues", num_rx);
> +	if (err) {
> +		*num_tx = 1;
> +		*num_rx = 1;
> +		return 0;
> +	}
> +
> +	if (*num_tx < 1 || *num_tx > FEC_ENET_MAX_TX_QS) {
> +		dev_err(&pdev->dev, "num_tx(=%d) greater than MAX_TX_QS(=%d)\n",
> +			*num_tx, FEC_ENET_MAX_TX_QS);
> +		return -EINVAL;
> +	}

You are dealing with two error conditions here, *num_tx == 0 and *num_tx
> FEC_ENET_MAX_TX_QS, so the message should not be "greater than"

> +
> +	if (*num_rx < 1 || *num_rx > FEC_ENET_MAX_RX_QS) {
> +		dev_err(&pdev->dev, "num_rx(=%d) greater than MAX_RX_QS(=%d)\n",
> +			*num_rx, FEC_ENET_MAX_RX_QS);
> +		return -EINVAL;
> +	}

Same here.

> +
> +	return 0;
> +}
> +
> +static int
>  fec_probe(struct platform_device *pdev)
>  {
>  	struct fec_enet_private *fep;
> @@ -2583,13 +2616,23 @@ fec_probe(struct platform_device *pdev)
>  	const struct of_device_id *of_id;
>  	static int dev_id;
>  	struct device_node *np = pdev->dev.of_node, *phy_node;
> +	int num_tx_qs = 1;
> +	int num_rx_qs = 1;
>  
>  	of_id = of_match_device(fec_dt_ids, &pdev->dev);
>  	if (of_id)
>  		pdev->id_entry = of_id->data;
>  
> +	if (pdev->id_entry &&
> +	    (pdev->id_entry->driver_data & FEC_QUIRK_HAS_AVB)) {
> +		ret = fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
> +		if (ret)
> +			return ret;
> +	}

This should be made a void function in order no to break non-DT probing
as mentioned before.

> +
>  	/* Init network device */
> -	ndev = alloc_etherdev(sizeof(struct fec_enet_private));
> +	ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private),
> +				  num_tx_qs, num_rx_qs);
>  	if (!ndev)
>  		return -ENOMEM;
>  
> @@ -2598,6 +2641,10 @@ fec_probe(struct platform_device *pdev)
>  	/* setup board info structure */
>  	fep = netdev_priv(ndev);
>  
> +	fep->num_rx_queues = num_rx_qs;
> +	fep->num_tx_queues = num_tx_qs;
> +	netif_set_real_num_rx_queues(ndev, num_rx_qs);

How about netif_set_real_num_tx_queues? This is probably useless anyway
because you already allocated this network device with the correct
(supposedly) number of TX and RX queues coming from the platform
configuration data.

> +
>  #if !defined(CONFIG_M5272)
>  	/* default enable pause frame auto negotiation */
>  	if (pdev->id_entry &&
> 

[Patch net-next 04/11] net:fec: add multiqueue support

[Florian Fainelli]

On 09/03/2014 01:38 PM, Frank Li wrote:
> From: Fugang Duan <B38611@freescale.com>
> 
> Add multiqueue support, which is compatible with previous single queue
> driver for FEC/ENET IPs.
> 
> By default, the tx/rx queue number is 1, user can config the queue number
> at DTS file like this:
> 	fsl,num_tx_queues=<3>;
> 	fsl,num_rx_queues=<3>;
> 
> Ethernet multiqueue mechanism can improve performance in SMP system.
> For single hw queue, multiqueue can balance cpu loading.
> For multi hw queues, multiple cores can process network packets in parallel,
> and refer the article for the detail advantage for multiqueue:
> http://vger.kernel.org/~davem/davem_nyc09.pdf

You should probably split that patch in a minimum of two, or more
different parts.

Two parts:

- add TX multiqueue support
- add RX multiqueue support

More parts:

- prepare for TX multiqueue (introduce queue structure, definitions)
- transition TX path to use queue 0
- add support for the two other TX queues
- prepare for RX multiqueue (introduce queue structure, definitions)
- prepare for multiqueue, hardcode RX path to use queue 0
- add support for the two other RX queues

The number of changes are extremely hard to review...
--
Florian

[Patch net-next 04/11] net:fec: add multiqueue support

[Zhi Li]

On Wed, Sep 3, 2014 at 4:27 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 09/03/2014 01:38 PM, Frank Li wrote:
>> From: Fugang Duan <B38611@freescale.com>
>>
>> Add multiqueue support, which is compatible with previous single queue
>> driver for FEC/ENET IPs.
>>
>> By default, the tx/rx queue number is 1, user can config the queue number
>> at DTS file like this:
>>       fsl,num_tx_queues=<3>;
>>       fsl,num_rx_queues=<3>;
>>
>> Ethernet multiqueue mechanism can improve performance in SMP system.
>> For single hw queue, multiqueue can balance cpu loading.
>> For multi hw queues, multiple cores can process network packets in parallel,
>> and refer the article for the detail advantage for multiqueue:
>> http://vger.kernel.org/~davem/davem_nyc09.pdf
>
> You should probably split that patch in a minimum of two, or more
> different parts.
>
> Two parts:
>
> - add TX multiqueue support
> - add RX multiqueue support
>

I understood your concern.
We also need consider logical integration.

Separate this patch just reduce changed lines in a patch.
Most of this patch change is from fep-> to txq-> or rxq->

best regards
Frank Li

> More parts:
>
> - prepare for TX multiqueue (introduce queue structure, definitions)
> - transition TX path to use queue 0
> - add support for the two other TX queues
> - prepare for RX multiqueue (introduce queue structure, definitions)
> - prepare for multiqueue, hardcode RX path to use queue 0
> - add support for the two other RX queues
>
> The number of changes are extremely hard to review...
> --
> Florian

[Patch net-next 04/11] net:fec: add multiqueue support

[Florian Fainelli]

2014-09-03 20:41 GMT-07:00 Zhi Li <lznuaa@gmail.com>:
> On Wed, Sep 3, 2014 at 4:27 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 09/03/2014 01:38 PM, Frank Li wrote:
>>> From: Fugang Duan <B38611@freescale.com>
>>>
>>> Add multiqueue support, which is compatible with previous single queue
>>> driver for FEC/ENET IPs.
>>>
>>> By default, the tx/rx queue number is 1, user can config the queue number
>>> at DTS file like this:
>>>       fsl,num_tx_queues=<3>;
>>>       fsl,num_rx_queues=<3>;
>>>
>>> Ethernet multiqueue mechanism can improve performance in SMP system.
>>> For single hw queue, multiqueue can balance cpu loading.
>>> For multi hw queues, multiple cores can process network packets in parallel,
>>> and refer the article for the detail advantage for multiqueue:
>>> http://vger.kernel.org/~davem/davem_nyc09.pdf
>>
>> You should probably split that patch in a minimum of two, or more
>> different parts.
>>
>> Two parts:
>>
>> - add TX multiqueue support
>> - add RX multiqueue support
>>
>
> I understood your concern.
> We also need consider logical integration.

Sure, hence the reason why I suggest you make it happen in different
parts, what you can do is make sure that you parse "fsl,num_tx_queues"
and "fsl,num_rx_queues" as one of the last patches in this patch
series. That way, we achieve a logical breakdown of the changes and
only support and enable multiqueue when the driver is ready.

>
> Separate this patch just reduce changed lines in a patch.

That is exactly my point, right now there is too much churn to make a
reviewer able to spot potential problems.

> Most of this patch change is from fep-> to txq-> or rxq->
>
> best regards
> Frank Li
>
>> More parts:
>>
>> - prepare for TX multiqueue (introduce queue structure, definitions)
>> - transition TX path to use queue 0
>> - add support for the two other TX queues
>> - prepare for RX multiqueue (introduce queue structure, definitions)
>> - prepare for multiqueue, hardcode RX path to use queue 0
>> - add support for the two other RX queues
>>
>> The number of changes are extremely hard to review...
>> --
>> Florian



-- 
Florian

[Patch net-next 04/11] net:fec: add multiqueue support

[Zhi Li]

On Wed, Sep 3, 2014 at 11:09 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 2014-09-03 20:41 GMT-07:00 Zhi Li <lznuaa@gmail.com>:
>> On Wed, Sep 3, 2014 at 4:27 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 09/03/2014 01:38 PM, Frank Li wrote:
>>>> From: Fugang Duan <B38611@freescale.com>
>>>>
>>>> Add multiqueue support, which is compatible with previous single queue
>>>> driver for FEC/ENET IPs.
>>>>
>>>> By default, the tx/rx queue number is 1, user can config the queue number
>>>> at DTS file like this:
>>>>       fsl,num_tx_queues=<3>;
>>>>       fsl,num_rx_queues=<3>;
>>>>
>>>> Ethernet multiqueue mechanism can improve performance in SMP system.
>>>> For single hw queue, multiqueue can balance cpu loading.
>>>> For multi hw queues, multiple cores can process network packets in parallel,
>>>> and refer the article for the detail advantage for multiqueue:
>>>> http://vger.kernel.org/~davem/davem_nyc09.pdf
>>>
>>> You should probably split that patch in a minimum of two, or more
>>> different parts.
>>>
>>> Two parts:
>>>
>>> - add TX multiqueue support
>>> - add RX multiqueue support
>>>
>>
>> I understood your concern.
>> We also need consider logical integration.
>
> Sure, hence the reason why I suggest you make it happen in different
> parts, what you can do is make sure that you parse "fsl,num_tx_queues"
> and "fsl,num_rx_queues" as one of the last patches in this patch
> series. That way, we achieve a logical breakdown of the changes and
> only support and enable multiqueue when the driver is ready.

I can remove this part to "net:fec: add enet-avb IP support", last
patch just change DT file,
which come into difference maintainer.

But RX and TX is difficult to separate because RX and TX share some
function, such as
fec_enet_get_nextdesc

If I change TX part,  fec_enet_get_nextdesc will add a parameter. If
that, all RX part need be
changed.

>
>>
>> Separate this patch just reduce changed lines in a patch.
>
> That is exactly my point, right now there is too much churn to make a
> reviewer able to spot potential problems.
>
>> Most of this patch change is from fep-> to txq-> or rxq->
>>
>> best regards
>> Frank Li
>>
>>> More parts:
>>>
>>> - prepare for TX multiqueue (introduce queue structure, definitions)
>>> - transition TX path to use queue 0
>>> - add support for the two other TX queues
>>> - prepare for RX multiqueue (introduce queue structure, definitions)
>>> - prepare for multiqueue, hardcode RX path to use queue 0
>>> - add support for the two other RX queues
>>>
>>> The number of changes are extremely hard to review...
>>> --
>>> Florian
>
>
>
> --
> Florian

[Patch net-next 08/11] net:fec: change FEC alignment to 64 bytes for ARM platform

[David Laight]

From: Frank Li
> From: Fugang Duan <B38611@freescale.com>
> 
> Since enet-avb has 64 bytes alignment limitation for rx DMA transfer.
> The previous enet IP for ARM platform has 16 bytes alignment for tx
> DMA transfer.

Do you mean rx or tx here? or both??

And can we beat up the hardware designers to stop these restrictions
on rx (in particular) ethernet buffer alignments?
A device isn't suitable for ethernet unless is can write the destination
mac address to a 4n+2 boundary.

	David

> 64 is the an integral number of 16, so change alignment
> to 64 bytes for all ARM platform, which don't impact the performance
> of previous platform.
> 
> Signed-off-by: Fugang Duan <B38611@freescale.com>
> Signed-off-by: Frank Li <Frank.Li@freescale.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index a232245..b388e29f 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -65,7 +65,7 @@
>  static void set_multicast_list(struct net_device *ndev);
> 
>  #if defined(CONFIG_ARM)
> -#define FEC_ALIGNMENT	0xf
> +#define FEC_ALIGNMENT	0x3f
>  #else
>  #define FEC_ALIGNMENT	0x3
>  #endif
> --
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Patch net-next 08/11] net:fec: change FEC alignment to 64 bytes for ARM platform

[fugang.duan at freescale.com]

From: David Laight <David.Laight@ACULAB.COM> Sent: Thursday, September 04, 2014 4:51 PM
>To: Li Frank-B20596; Duan Fugang-B38611; davem at davemloft.net;
>netdev at vger.kernel.org; lznuaa at gmail.com
>Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org;
>devicetree at vger.kernel.org; Duan Fugang-B38611
>Subject: RE: [Patch net-next 08/11] net:fec: change FEC alignment to 64
>bytes for ARM platform
>
>From: Frank Li
>> From: Fugang Duan <B38611@freescale.com>
>>
>> Since enet-avb has 64 bytes alignment limitation for rx DMA transfer.
>> The previous enet IP for ARM platform has 16 bytes alignment for tx
>> DMA transfer.
>
>Do you mean rx or tx here? or both??
>
>And can we beat up the hardware designers to stop these restrictions on rx
>(in particular) ethernet buffer alignments?
>A device isn't suitable for ethernet unless is can write the destination
>mac address to a 4n+2 boundary.
>
>	David
>
Hi, David,

For previous enet IP there has 16 bytes data buffer alignment limitation for tx & rx DMA transfer.
For imx6sx enet-avb IP, there has 64 bytes data buffer alignment limitation for rx DMA transfer, byte alignment for tx data
Buffer for DMA transfer.

I think rx data buffer alignment limitation don't introduce performance drop, is not complex for sw implemention. Anyway,
We can request IC designer to remove the limitation for future chips.

Thanks,
Andy

>> 64 is the an integral number of 16, so change alignment to 64 bytes
>> for all ARM platform, which don't impact the performance of previous
>> platform.
>>
>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>> Signed-off-by: Frank Li <Frank.Li@freescale.com>
>> ---
>>  drivers/net/ethernet/freescale/fec_main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index a232245..b388e29f 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -65,7 +65,7 @@
>>  static void set_multicast_list(struct net_device *ndev);
>>
>>  #if defined(CONFIG_ARM)
>> -#define FEC_ALIGNMENT	0xf
>> +#define FEC_ALIGNMENT	0x3f
>>  #else
>>  #define FEC_ALIGNMENT	0x3
>>  #endif
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo at vger.kernel.org More majordomo info
>> at  http://vger.kernel.org/majordomo-info.html

[Patch net-next 08/11] net:fec: change FEC alignment to 64 bytes for ARM platform

[David Laight]

From: fugang.duan@freescale.com
> >From: Frank Li
> >> From: Fugang Duan <B38611@freescale.com>
> >>
> >> Since enet-avb has 64 bytes alignment limitation for rx DMA transfer.
> >> The previous enet IP for ARM platform has 16 bytes alignment for tx
> >> DMA transfer.
> >
> >Do you mean rx or tx here? or both??
> >
> >And can we beat up the hardware designers to stop these restrictions on rx
> >(in particular) ethernet buffer alignments?
> >A device isn't suitable for ethernet unless is can write the destination
> >mac address to a 4n+2 boundary.
> >
> >	David
> >
> Hi, David,
> 
> For previous enet IP there has 16 bytes data buffer alignment limitation for tx & rx DMA transfer.
> For imx6sx enet-avb IP, there has 64 bytes data buffer alignment limitation for rx DMA transfer, byte
> alignment for tx data
> Buffer for DMA transfer.
> 
> I think rx data buffer alignment limitation don't introduce performance drop, is not complex for sw
> implemention. Anyway,
> We can request IC designer to remove the limitation for future chips.

The 'problem' is that you need the IP header to be 32bit aligned.
If the dma buffer has to be aligned then the code either has to do an expensive
misaligned copy of the entire frame (at some point the entire frame is almost
certainly all copied into an aligned buffer), or take the hit of misaligned
memory accesses (which might have to be emulated with byte transfers).

The rx buffer can be aligned provided the hardware skips (or writes junk to)
the first two bytes.

	David

[Patch net-next 08/11] net:fec: change FEC alignment to 64 bytes for ARM platform

[Zhi Li]

On Thu, Sep 4, 2014 at 4:18 AM, David Laight <David.Laight@aculab.com> wrote:
> From: fugang.duan at freescale.com
>> >From: Frank Li
>> >> From: Fugang Duan <B38611@freescale.com>
>> >>
>> >> Since enet-avb has 64 bytes alignment limitation for rx DMA transfer.
>> >> The previous enet IP for ARM platform has 16 bytes alignment for tx
>> >> DMA transfer.
>> >
>> >Do you mean rx or tx here? or both??
>> >
>> >And can we beat up the hardware designers to stop these restrictions on rx
>> >(in particular) ethernet buffer alignments?
>> >A device isn't suitable for ethernet unless is can write the destination
>> >mac address to a 4n+2 boundary.
>> >
>> >     David
>> >
>> Hi, David,
>>
>> For previous enet IP there has 16 bytes data buffer alignment limitation for tx & rx DMA transfer.
>> For imx6sx enet-avb IP, there has 64 bytes data buffer alignment limitation for rx DMA transfer, byte
>> alignment for tx data
>> Buffer for DMA transfer.
>>
>> I think rx data buffer alignment limitation don't introduce performance drop, is not complex for sw
>> implemention. Anyway,
>> We can request IC designer to remove the limitation for future chips.
>
> The 'problem' is that you need the IP header to be 32bit aligned.
> If the dma buffer has to be aligned then the code either has to do an expensive
> misaligned copy of the entire frame (at some point the entire frame is almost
> certainly all copied into an aligned buffer), or take the hit of misaligned
> memory accesses (which might have to be emulated with byte transfers).
>
> The rx buffer can be aligned provided the hardware skips (or writes junk to)
> the first two bytes.

ENET IP have feature to add additional 2 byte when save data to memory.

RX FIFO Shift-16
When this field is set, the actual frame data starts at bit 16 of the
first word read from the RX FIFO aligning
the Ethernet payload on a 32-bit boundary.
NOTE: This function only affects the FIFO storage and has no influence
on the statistics, which use the
actual length of the frame received.
0 Disabled.
1 Instructs the MAC to write two additional bytes in front of each
frame received into the RX FIFO.

i.MX6SX,
          Remove TX alignment requirement.
          But RX and DMA BD address is 64byte aligment.


best regards
Frank Li



>
>         David
>
>
>