[PATCH/RFC net-next 0/5] ravb: updates

[PATCH/RFC net-next 0/5] ravb: updates

[Simon Horman]

Hi Sergei,

this series is composed of otherwise unrelated RAVB patches from the R-Car
BSP v3.6.2 which at a first pass seem worth considering for upstream.

I would value your feedback on these patches so they can either proceed
into net-next or remain local to the BSP.

Thanks!

Kazuya Mizuguchi (4):
  ravb: correct ptp does failure after suspend and resume
  ravb: do not write 1 to reserved bits
  ravb: remove undocumented processing
  ravb: remove tx buffer addr 4byte alilgnment restriction for R-Car
    Gen3

Masaru Nagai (1):
  ravb: fix inconsistent lock state at enabling tx timestamp

 drivers/net/ethernet/renesas/ravb.h      |  23 +++-
 drivers/net/ethernet/renesas/ravb_main.c | 192 ++++++++++++++++---------------
 drivers/net/ethernet/renesas/ravb_ptp.c  |   2 +-
 3 files changed, 117 insertions(+), 100 deletions(-)

-- 
2.11.0

[PATCH/RFC net-next 1/5] ravb: fix inconsistent lock state at enabling tx timestamp

[Simon Horman]

From: Masaru Nagai <masaru.nagai.vx@renesas.com>

[   58.490829] =================================
[   58.495205] [ INFO: inconsistent lock state ]
[   58.499583] 4.9.0-yocto-standard-00007-g2ef7caf #57 Not tainted
[   58.505529] ---------------------------------
[   58.509904] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
[   58.515939] swapper/0/0 [HC1[1]:SC1[1]:HE0:SE0] takes:
[   58.521099]  (&(&list->lock)->rlock#2){?.-...}, at: [<ffff00000899f474>] skb_queue_tail+0x2c/0x68
{HARDIRQ-ON-W} state was registered at:
[   58.533654]   [   58.535155] [<ffff000008127e94>] mark_lock+0x1c4/0x718
[   58.540318]   [   58.541814] [<ffff000008129068>] __lock_acquire+0x660/0x1890
[   58.547501]   [   58.548997] [<ffff00000812a840>] lock_acquire+0xd0/0x290
[   58.554334]   [   58.555834] [<ffff000008acfd28>] _raw_spin_lock_bh+0x50/0x90
[   58.561520]   [   58.563018] [<ffff000008a31908>] first_packet_length+0x40/0x2b0
[   58.568965]   [   58.570461] [<ffff000008a31bd0>] udp_ioctl+0x58/0x120
[   58.575535]   [   58.577032] [<ffff000008a41df8>] inet_ioctl+0x58/0x128
[   58.582194]   [   58.583691] [<ffff0000089938e0>] sock_do_ioctl+0x40/0x88
[   58.589028]   [   58.590523] [<ffff000008994824>] sock_ioctl+0x284/0x350
[   58.595773]   [   58.597271] [<ffff000008295b88>] do_vfs_ioctl+0xb0/0x7c0
[   58.602607]   [   58.604103] [<ffff00000829632c>] SyS_ioctl+0x94/0xa8
[   58.609090]   [   58.610588] [<ffff00000808374c>] __sys_trace_return+0x0/0x4
[   58.616187] irq event stamp: 335205
[   58.619690] hardirqs last  enabled at (335204): [<ffff00000808180c>] __do_softirq+0xdc/0x5c4
[   58.628168] hardirqs last disabled at (335205): [<ffff000008082f70>] el1_irq+0x70/0x12c
[   58.636211] softirqs last  enabled at (335202): [<ffff0000080d7168>] _local_bh_enable+0x28/0x50
[   58.644950] softirqs last disabled at (335203): [<ffff0000080d76e4>] irq_exit+0xd4/0x100
[   58.653076]
[   58.653076] other info that might help us debug this:
[   58.659632]  Possible unsafe locking scenario:
[   58.659632]
[   58.665577]        CPU0
[   58.668031]        ----
[   58.670484]   lock(&(&list->lock)->rlock#2);
[   58.674799]   <Interrupt>
[   58.677427]     lock(&(&list->lock)->rlock#2);
[   58.681916]
[   58.681916]  *** DEADLOCK ***
[   58.681916]
[   58.687863] 1 lock held by swapper/0/0:
[   58.691713]  #0:  (&(&priv->lock)->rlock){-.-...}, at: [<ffff0000087981b0>] ravb_multi_interrupt+0x28/0x98
[   58.701456]
[   58.701456] stack backtrace:
[   58.705833] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-yocto-standard-00007-g2ef7caf #57
[   58.714396] Hardware name: Renesas Salvator-X board based on r8a7796 (DT)
[   58.721214] Call trace:
[   58.723672] [<ffff00000808a478>] dump_backtrace+0x0/0x1d8
[   58.729095] [<ffff00000808a674>] show_stack+0x24/0x30
[   58.734170] [<ffff000008490858>] dump_stack+0xb0/0xe8
[   58.740285] [<ffff0000081eb668>] print_usage_bug.part.24+0x264/0x27c
[   58.747697] [<ffff000008127e20>] mark_lock+0x150/0x718
[   58.753892] [<ffff000008129618>] __lock_acquire+0xc10/0x1890
[   58.760602] [<ffff00000812a840>] lock_acquire+0xd0/0x290
[   58.766956] [<ffff000008acfe50>] _raw_spin_lock_irqsave+0x58/0x98
[   58.774089] [<ffff00000899f474>] skb_queue_tail+0x2c/0x68
[   58.780518] [<ffff0000089a1678>] sock_queue_err_skb+0xc8/0x138
[   58.787364] [<ffff0000089a1774>] __skb_complete_tx_timestamp+0x8c/0xb8
[   58.794888] [<ffff0000089a7168>] __skb_tstamp_tx+0xd8/0x130
[   58.801437] [<ffff0000089a71f0>] skb_tstamp_tx+0x30/0x40
[   58.807723] [<ffff000008798144>] ravb_timestamp_interrupt+0x164/0x1a8
[   58.815144] [<ffff000008798210>] ravb_multi_interrupt+0x88/0x98
[   58.822043] [<ffff00000813add4>] __handle_irq_event_percpu+0x94/0x418
[   58.829464] [<ffff00000813b180>] handle_irq_event_percpu+0x28/0x60
[   58.836622] [<ffff00000813b208>] handle_irq_event+0x50/0x80
[   58.843166] [<ffff00000813f0f4>] handle_fasteoi_irq+0xdc/0x1e0
[   58.849968] [<ffff000008139cac>] generic_handle_irq+0x34/0x50
[   58.856681] [<ffff00000813a41c>] __handle_domain_irq+0x8c/0x100
[   58.863568] [<ffff000008081570>] gic_handle_irq+0x60/0xb8
[   58.869930] Exception stack(0xffff80063b0f9de0 to 0xffff80063b0f9f10)
[   58.877348] 9de0: ffff80063b0f9e10 0001000000000000 ffff80063b0f9f40 ffff000008081810
[   58.886159] 9e00: 0000000060000145 ffff000008082f70 ffff000009194b00 0000000000190f2c
[   58.894961] 9e20: 0000800632171000 000000000000000a 0000000000000000 000000000003a4d0
[   58.903767] 9e40: 0000000000000016 0000000000000023 ffff0000091952f8 0000000000000000
[   58.912568] 9e60: 0000000000000040 0000000000000000 0000000034d5d91d 0000000000000000
[   58.921363] 9e80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[   58.930133] 9ea0: 0000000000000000 ffff000009180000 ffff0000080d76e4 0000000000000052
[   58.938897] 9ec0: ffff000008d70000 0000000000000008 0000000000000000 0000000000000001
[   58.947660] 9ee0: ffff80063a428000 ffff000009185000 ffff000009180000 ffff80063b0f9f40
[   58.956430] 9f00: ffff00000808180c ffff80063b0f9f40
[   58.962253] [<ffff000008082fb4>] el1_irq+0xb4/0x12c
[   58.968096] [<ffff0000080d76e4>] irq_exit+0xd4/0x100
[   58.974025] [<ffff00000813a420>] __handle_domain_irq+0x90/0x100
[   58.980916] [<ffff000008081570>] gic_handle_irq+0x60/0xb8
[   58.987281] Exception stack(0xffff000009183d20 to 0xffff000009183e50)
[   58.994708] 3d20: ffff000009194b00 0000000000190f2b 0000800632171000 8c6318c6318c6320
[   59.003554] 3d40: 0000000000000000 000000000003a4d0 0000000000000016 000000000000002a
[   59.012416] 3d60: ffff0000091952f8 0000000000000000 0000000000001000 0000000000000000
[   59.021279] 3d80: 0000000034d5d91d 0000000000000000 0000000000000000 0000000000000000
[   59.030111] 3da0: 0000000000000000 0000000000000000 0000000000000000 0000000d9e3b53c4
[   59.038913] 3dc0: ffff800638fb1800 0000000000000001 ffff00000925ad40 0000000000000004
[   59.047726] 3de0: 0000000d9e0899ee 0000000000000001 ffff000008e3cc90 ffff00000918b000
[   59.056521] 3e00: ffff00000918b000 ffff000009183e50 ffff0000088d0acc ffff000009183e50
[   59.065298] 3e20: ffff0000088d0ad0 0000000060000145 0000000000000001 ffff0000088d0b70
[   59.074068] 3e40: ffffffffffffffff ffff0000088d0acc
[   59.079878] [<ffff000008082fb4>] el1_irq+0xb4/0x12c
[   59.085696] [<ffff0000088d0ad0>] cpuidle_enter_state+0x130/0x408
[   59.092656] [<ffff0000088d0e1c>] cpuidle_enter+0x34/0x48
[   59.098909] [<ffff00000811ff50>] call_cpuidle+0x40/0x70
[   59.105070] [<ffff00000812026c>] cpu_startup_entry+0x144/0x1f0
[   59.111845] [<ffff000008ac7f98>] rest_init+0x150/0x160
[   59.117925] [<ffff000008ec0b54>] start_kernel+0x38c/0x3a0
[   59.124261] [<ffff000008ec01d8>] __primary_switched+0x5c/0x64

Fixes: f51bdc236b6c ("ravb: Add dma queue interrupt support")
Signed-off-by: Masaru Nagai <masaru.nagai.vx@renesas.com>
Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/net/ethernet/renesas/ravb_main.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 68f122140966..b311b1ac1286 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -481,7 +481,7 @@ static int ravb_dmac_init(struct net_device *ndev)
 	/* Receive FIFO full error, descriptor empty */
 	ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
 	/* Frame transmitted, timestamp FIFO updated */
-	ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
+	ravb_write(ndev, TIC_FTE0 | TIC_FTE1, TIC);
 
 	/* Setting the control will start the AVB-DMAC process. */
 	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION);
@@ -793,18 +793,6 @@ static bool ravb_queue_interrupt(struct net_device *ndev, int q)
 	return false;
 }
 
-static bool ravb_timestamp_interrupt(struct net_device *ndev)
-{
-	u32 tis = ravb_read(ndev, TIS);
-
-	if (tis & TIS_TFUF) {
-		ravb_write(ndev, ~TIS_TFUF, TIS);
-		ravb_get_tx_tstamp(ndev);
-		return true;
-	}
-	return false;
-}
-
 static irqreturn_t ravb_interrupt(int irq, void *dev_id)
 {
 	struct net_device *ndev = dev_id;
@@ -817,13 +805,9 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id)
 	iss = ravb_read(ndev, ISS);
 
 	/* Received and transmitted interrupts */
-	if (iss & (ISS_FRS | ISS_FTS | ISS_TFUS)) {
+	if (iss & (ISS_FRS | ISS_FTS)) {
 		int q;
 
-		/* Timestamp updated */
-		if (ravb_timestamp_interrupt(ndev))
-			result = IRQ_HANDLED;
-
 		/* Network control and best effort queue RX/TX */
 		for (q = RAVB_NC; q >= RAVB_BE; q--) {
 			if (ravb_queue_interrupt(ndev, q))
@@ -854,7 +838,7 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id)
 	return result;
 }
 
-/* Timestamp/Error/gPTP interrupt handler */
+/* Error/gPTP interrupt handler */
 static irqreturn_t ravb_multi_interrupt(int irq, void *dev_id)
 {
 	struct net_device *ndev = dev_id;
@@ -866,10 +850,6 @@ static irqreturn_t ravb_multi_interrupt(int irq, void *dev_id)
 	/* Get interrupt status */
 	iss = ravb_read(ndev, ISS);
 
-	/* Timestamp updated */
-	if ((iss & ISS_TFUS) && ravb_timestamp_interrupt(ndev))
-		result = IRQ_HANDLED;
-
 	/* Error status summary */
 	if (iss & ISS_ES) {
 		ravb_error_interrupt(ndev);
@@ -939,6 +919,10 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 		}
 		/* Processing TX Descriptor Ring */
 		if (tis & mask) {
+			/* Timestamp updated */
+			if (q == RAVB_NC)
+				ravb_get_tx_tstamp(ndev);
+
 			spin_lock_irqsave(&priv->lock, flags);
 			/* Clear TX interrupt */
 			ravb_write(ndev, ~mask, TIS);
-- 
2.11.0

Re: [PATCH/RFC net-next 1/5] ravb: fix inconsistent lock state at enabling tx timestamp

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

On Tue, Apr 17, 2018 at 10:50:26AM +0200, Simon Horman wrote:
> From: Masaru Nagai <masaru.nagai.vx@renesas.com>
> 
> [   58.490829] =================================
> [   58.495205] [ INFO: inconsistent lock state ]
> [   58.499583] 4.9.0-yocto-standard-00007-g2ef7caf #57 Not tainted
> [   58.505529] ---------------------------------
> [   58.509904] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
> [   58.515939] swapper/0/0 [HC1[1]:SC1[1]:HE0:SE0] takes:
> [   58.521099]  (&(&list->lock)->rlock#2){?.-...}, at: [<ffff00000899f474>] skb_queue_tail+0x2c/0x68
> {HARDIRQ-ON-W} state was registered at:

Maybe add a short text to the log to describe the approach of this fix?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH/RFC net-next 1/5] ravb: fix inconsistent lock state at enabling tx timestamp

[Geert Uytterhoeven]

Hi Simon,

On Tue, Apr 17, 2018 at 10:50 AM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> From: Masaru Nagai <masaru.nagai.vx@renesas.com>
>
> [   58.490829] =================================
> [   58.495205] [ INFO: inconsistent lock state ]
> [   58.499583] 4.9.0-yocto-standard-00007-g2ef7caf #57 Not tainted

Can this be triggered on contemporary kernels, too?

> [   58.505529] ---------------------------------
> [   58.509904] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC net-next 1/5] ravb: fix inconsistent lock state at enabling tx timestamp

[David Miller]

From: Simon Horman <horms+renesas@verge.net.au>
Date: Tue, 17 Apr 2018 10:50:26 +0200

> From: Masaru Nagai <masaru.nagai.vx@renesas.com>
> 
> [   58.490829] =================================
> [   58.495205] [ INFO: inconsistent lock state ]
> [   58.499583] 4.9.0-yocto-standard-00007-g2ef7caf #57 Not tainted
 ...
> Fixes: f51bdc236b6c ("ravb: Add dma queue interrupt support")
> Signed-off-by: Masaru Nagai <masaru.nagai.vx@renesas.com>
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

This really needs more than the lockdep dump in the commit message, explaining
what the problem is and how it was corrected.

Are the wrong interrupt types enabled?  Are they handled improperly?
It definitely isn't clear from just reading the patch.

[PATCH/RFC net-next 2/5] ravb: correct ptp does failure after suspend and resume

[Simon Horman]

From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>

This patch fixes the problem that ptp4l command does not work after
suspend and resume.
Add the initial setting in ravb_suspend() and ravb_resume(),
because ptp does not work.

Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/net/ethernet/renesas/ravb_main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index b311b1ac1286..dbde3d11458b 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2295,6 +2295,9 @@ static int __maybe_unused ravb_suspend(struct device *dev)
 	else
 		ret = ravb_close(ndev);
 
+	if (priv->chip_id != RCAR_GEN2)
+		ravb_ptp_stop(ndev);
+
 	return ret;
 }
 
@@ -2302,6 +2305,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct ravb_private *priv = netdev_priv(ndev);
+	struct platform_device *pdev = priv->pdev;
 	int ret = 0;
 
 	/* If WoL is enabled set reset mode to rearm the WoL logic */
@@ -2330,6 +2334,9 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	/* Restore descriptor base address table */
 	ravb_write(ndev, priv->desc_bat_dma, DBAT);
 
+	if (priv->chip_id != RCAR_GEN2)
+		ravb_ptp_init(ndev, pdev);
+
 	if (netif_running(ndev)) {
 		if (priv->wol_enabled) {
 			ret = ravb_wol_restore(ndev);
-- 
2.11.0

Re: [PATCH/RFC net-next 2/5] ravb: correct ptp does failure after suspend and resume

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]


> @@ -2302,6 +2305,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
>  {
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct ravb_private *priv = netdev_priv(ndev);
> +	struct platform_device *pdev = priv->pdev;

Minor nit: I'd save this line...

> +	if (priv->chip_id != RCAR_GEN2)
> +		ravb_ptp_init(ndev, pdev);

... and use ravb_ptp_init(ndev, priv->pdev); here.

But well, maybe just bike-shedding...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH/RFC net-next 2/5] ravb: correct ptp does failure after suspend and resume

[Sergei Shtylyov]

On 04/17/2018 01:05 PM, Wolfram Sang wrote:

>> @@ -2302,6 +2305,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
>>  {
>>  	struct net_device *ndev = dev_get_drvdata(dev);
>>  	struct ravb_private *priv = netdev_priv(ndev);
>> +	struct platform_device *pdev = priv->pdev;

   Could infer 'pdev' from 'dev' (avoiding the dereference)...

> Minor nit: I'd save this line...
> 
>> +	if (priv->chip_id != RCAR_GEN2)
>> +		ravb_ptp_init(ndev, pdev);
> 
> ... and use ravb_ptp_init(ndev, priv->pdev); here.

   Agreed, no dire need for the new variable used only once.

MBR, Sergei

Re: [PATCH/RFC net-next 2/5] ravb: correct ptp does failure after suspend and resume

[Sergei Shtylyov]

Hello!

   s/failure/fail/ in the subject.

On 04/17/2018 11:50 AM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> 
> This patch fixes the problem that ptp4l command does not work after
> suspend and resume.
> Add the initial setting in ravb_suspend() and ravb_resume(),
> because ptp does not work.
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
[...]

   Other than the earlier comments, this seems good:

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH/RFC net-next 3/5] ravb: do not write 1 to reserved bits

[Simon Horman]

From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>

This patch corrects writing 1 to reserved bits.
The write value should be 0.

Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/net/ethernet/renesas/ravb.h      | 12 ++++++++++++
 drivers/net/ethernet/renesas/ravb_main.c |  9 +++++----
 drivers/net/ethernet/renesas/ravb_ptp.c  |  2 +-
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index b81f4faf7b10..57eea4a77826 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -433,6 +433,8 @@ enum EIS_BIT {
 	EIS_QFS		= 0x00010000,
 };
 
+#define EIS_RESERVED_BITS	(u32)(GENMASK(31, 17) | GENMASK(15, 11))
+
 /* RIC0 */
 enum RIC0_BIT {
 	RIC0_FRE0	= 0x00000001,
@@ -477,6 +479,8 @@ enum RIS0_BIT {
 	RIS0_FRF17	= 0x00020000,
 };
 
+#define RIS0_RESERVED_BITS	(u32)GENMASK(31, 18)
+
 /* RIC1 */
 enum RIC1_BIT {
 	RIC1_RFWE	= 0x80000000,
@@ -533,6 +537,8 @@ enum RIS2_BIT {
 	RIS2_RFFF	= 0x80000000,
 };
 
+#define RIS2_RESERVED_BITS	(u32)GENMASK_ULL(30, 18)
+
 /* TIC */
 enum TIC_BIT {
 	TIC_FTE0	= 0x00000001,	/* Undocumented? */
@@ -549,6 +555,10 @@ enum TIS_BIT {
 	TIS_TFWF	= 0x00000200,
 };
 
+#define TIS_RESERVED_BITS	(u32)(GENMASK_ULL(31, 20) | \
+				      GENMASK_ULL(15, 12) | \
+				      GENMASK_ULL(7, 4))
+
 /* ISS */
 enum ISS_BIT {
 	ISS_FRS		= 0x00000001,	/* Undocumented? */
@@ -622,6 +632,8 @@ enum GIS_BIT {
 	GIS_PTMF	= 0x00000004,
 };
 
+#define GIS_RESERVED_BITS	(u32)GENMASK(15, 10)
+
 /* GIE (R-Car Gen3 only) */
 enum GIE_BIT {
 	GIE_PTCS	= 0x00000001,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index dbde3d11458b..736ca2f76a35 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -742,10 +742,11 @@ static void ravb_error_interrupt(struct net_device *ndev)
 	u32 eis, ris2;
 
 	eis = ravb_read(ndev, EIS);
-	ravb_write(ndev, ~EIS_QFS, EIS);
+	ravb_write(ndev, ~(EIS_QFS | EIS_RESERVED_BITS), EIS);
 	if (eis & EIS_QFS) {
 		ris2 = ravb_read(ndev, RIS2);
-		ravb_write(ndev, ~(RIS2_QFF0 | RIS2_RFFF), RIS2);
+		ravb_write(ndev, ~(RIS2_QFF0 | RIS2_RFFF | RIS2_RESERVED_BITS),
+			   RIS2);
 
 		/* Receive Descriptor Empty int */
 		if (ris2 & RIS2_QFF0)
@@ -913,7 +914,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 		/* Processing RX Descriptor Ring */
 		if (ris0 & mask) {
 			/* Clear RX interrupt */
-			ravb_write(ndev, ~mask, RIS0);
+			ravb_write(ndev, ~(mask | RIS0_RESERVED_BITS), RIS0);
 			if (ravb_rx(ndev, &quota, q))
 				goto out;
 		}
@@ -925,7 +926,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 
 			spin_lock_irqsave(&priv->lock, flags);
 			/* Clear TX interrupt */
-			ravb_write(ndev, ~mask, TIS);
+			ravb_write(ndev, ~(mask | TIS_RESERVED_BITS), TIS);
 			ravb_tx_free(ndev, q, true);
 			netif_wake_subqueue(ndev, q);
 			mmiowb();
diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index eede70ec37f8..ba3017ca5577 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
@@ -319,7 +319,7 @@ void ravb_ptp_interrupt(struct net_device *ndev)
 		}
 	}
 
-	ravb_write(ndev, ~gis, GIS);
+	ravb_write(ndev, ~(gis | GIS_RESERVED_BITS), GIS);
 }
 
 void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
-- 
2.11.0

Re: [PATCH/RFC net-next 3/5] ravb: do not write 1 to reserved bits

[David Miller]

From: Simon Horman <horms+renesas@verge.net.au>
Date: Tue, 17 Apr 2018 10:50:28 +0200

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> 
> This patch corrects writing 1 to reserved bits.
> The write value should be 0.
> 
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

How are we ending up in situations where the driver is trying to write
non-zero values to those fields in the first place?

The places creating those values should be making sure that the
reserved bits are never set.

If you mask out the reserved bits in the register writing locations,
this just hides bugs.

Re: [PATCH/RFC net-next 3/5] ravb: do not write 1 to reserved bits

[Sergei Shtylyov]

Hello!

On 04/17/2018 05:15 PM, David Miller wrote:

>> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>
>> This patch corrects writing 1 to reserved bits.
>> The write value should be 0.
>>
>> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> 
> How are we ending up in situations where the driver is trying to write
> non-zero values to those fields in the first place?

   The brain damaged AVB core design is to blame here. You have to write 0 to
clear the set bits and, at the same time, you can't write 1 to the reserved
bits... :-/

> The places creating those values should be making sure that the
> reserved bits are never set.

   That's basically what this patch is doing.

> If you mask out the reserved bits in the register writing locations,
> this just hides bugs.

   There are no *other* locations in some cases... 
   And I don't think that forcing the reserved bits to 1 after a register is
read would look better. :-(

MBR, Sergei

Re: [PATCH/RFC net-next 3/5] ravb: do not write 1 to reserved bits

[Sergei Shtylyov]

On 04/17/2018 11:50 AM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> 
> This patch corrects writing 1 to reserved bits.
> The write value should be 0.
> 
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>  drivers/net/ethernet/renesas/ravb.h      | 12 ++++++++++++
>  drivers/net/ethernet/renesas/ravb_main.c |  9 +++++----
>  drivers/net/ethernet/renesas/ravb_ptp.c  |  2 +-
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index b81f4faf7b10..57eea4a77826 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -433,6 +433,8 @@ enum EIS_BIT {
>  	EIS_QFS		= 0x00010000,
>  };
>  
> +#define EIS_RESERVED_BITS	(u32)(GENMASK(31, 17) | GENMASK(15, 11))
> +
>  /* RIC0 */
>  enum RIC0_BIT {
>  	RIC0_FRE0	= 0x00000001,
> @@ -477,6 +479,8 @@ enum RIS0_BIT {
>  	RIS0_FRF17	= 0x00020000,
>  };
>  
> +#define RIS0_RESERVED_BITS	(u32)GENMASK(31, 18)
> +
>  /* RIC1 */
>  enum RIC1_BIT {
>  	RIC1_RFWE	= 0x80000000,
> @@ -533,6 +537,8 @@ enum RIS2_BIT {
>  	RIS2_RFFF	= 0x80000000,
>  };
>  
> +#define RIS2_RESERVED_BITS	(u32)GENMASK_ULL(30, 18)
> +
>  /* TIC */
>  enum TIC_BIT {
>  	TIC_FTE0	= 0x00000001,	/* Undocumented? */
> @@ -549,6 +555,10 @@ enum TIS_BIT {
>  	TIS_TFWF	= 0x00000200,
>  };
>  
> +#define TIS_RESERVED_BITS	(u32)(GENMASK_ULL(31, 20) | \
> +				      GENMASK_ULL(15, 12) | \
> +				      GENMASK_ULL(7, 4))
> +
>  /* ISS */
>  enum ISS_BIT {
>  	ISS_FRS		= 0x00000001,	/* Undocumented? */
> @@ -622,6 +632,8 @@ enum GIS_BIT {
>  	GIS_PTMF	= 0x00000004,
>  };
>  
> +#define GIS_RESERVED_BITS	(u32)GENMASK(15, 10)
> +
>  /* GIE (R-Car Gen3 only) */
>  enum GIE_BIT {
>  	GIE_PTCS	= 0x00000001,
[...]

   Perhaps we can do what the MUSB driver does: declare the writing-zero-clears
masks (inside *enum*!), and then set them before writing th register... 

MBR, Sergei

[PATCH/RFC net-next 4/5] ravb: remove undocumented processing

[Simon Horman]

From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>

Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/net/ethernet/renesas/ravb.h      |  5 -----
 drivers/net/ethernet/renesas/ravb_main.c | 15 ---------------
 2 files changed, 20 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 57eea4a77826..fcd04dbc7dde 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -197,15 +197,11 @@ enum ravb_reg {
 	MAHR	= 0x05c0,
 	MALR	= 0x05c8,
 	TROCR	= 0x0700,	/* Undocumented? */
-	CDCR	= 0x0708,	/* Undocumented? */
-	LCCR	= 0x0710,	/* Undocumented? */
 	CEFCR	= 0x0740,
 	FRECR	= 0x0748,
 	TSFRCR	= 0x0750,
 	TLFRCR	= 0x0758,
 	RFCR	= 0x0760,
-	CERCR	= 0x0768,	/* Undocumented? */
-	CEECR	= 0x0770,	/* Undocumented? */
 	MAFCR	= 0x0778,
 };
 
@@ -223,7 +219,6 @@ enum CCC_BIT {
 	CCC_CSEL_HPB	= 0x00010000,
 	CCC_CSEL_ETH_TX	= 0x00020000,
 	CCC_CSEL_GMII_REF = 0x00030000,
-	CCC_BOC		= 0x00100000,	/* Undocumented? */
 	CCC_LBME	= 0x01000000,
 };
 
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 736ca2f76a35..88056dd912ed 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -451,12 +451,6 @@ static int ravb_dmac_init(struct net_device *ndev)
 	ravb_ring_format(ndev, RAVB_BE);
 	ravb_ring_format(ndev, RAVB_NC);
 
-#if defined(__LITTLE_ENDIAN)
-	ravb_modify(ndev, CCC, CCC_BOC, 0);
-#else
-	ravb_modify(ndev, CCC, CCC_BOC, CCC_BOC);
-#endif
-
 	/* Set AVB RX */
 	ravb_write(ndev,
 		   RCR_EFFS | RCR_ENCF | RCR_ETS0 | RCR_ESF | 0x18000000, RCR);
@@ -1660,15 +1654,6 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
 
 	nstats->tx_dropped += ravb_read(ndev, TROCR);
 	ravb_write(ndev, 0, TROCR);	/* (write clear) */
-	nstats->collisions += ravb_read(ndev, CDCR);
-	ravb_write(ndev, 0, CDCR);	/* (write clear) */
-	nstats->tx_carrier_errors += ravb_read(ndev, LCCR);
-	ravb_write(ndev, 0, LCCR);	/* (write clear) */
-
-	nstats->tx_carrier_errors += ravb_read(ndev, CERCR);
-	ravb_write(ndev, 0, CERCR);	/* (write clear) */
-	nstats->tx_carrier_errors += ravb_read(ndev, CEECR);
-	ravb_write(ndev, 0, CEECR);	/* (write clear) */
 
 	nstats->rx_packets = stats0->rx_packets + stats1->rx_packets;
 	nstats->tx_packets = stats0->tx_packets + stats1->tx_packets;
-- 
2.11.0

Re: [PATCH/RFC net-next 4/5] ravb: remove undocumented processing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

On Tue, Apr 17, 2018 at 10:50:29AM +0200, Simon Horman wrote:
> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> 
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  5 -----
>  drivers/net/ethernet/renesas/ravb_main.c | 15 ---------------
>  2 files changed, 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 57eea4a77826..fcd04dbc7dde 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -197,15 +197,11 @@ enum ravb_reg {
>  	MAHR	= 0x05c0,
>  	MALR	= 0x05c8,
>  	TROCR	= 0x0700,	/* Undocumented? */

Why not this, too? Maybe some background info from HW team for the
commit message would be nice to have...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH/RFC net-next 4/5] ravb: remove undocumented processing

[David Miller]

From: Simon Horman <horms+renesas@verge.net.au>
Date: Tue, 17 Apr 2018 10:50:29 +0200

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> 
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

Why?  What was wrong with it?

Need more text and explanations in these commit messages please.

Re: [PATCH/RFC net-next 4/5] ravb: remove undocumented processing

[Sergei Shtylyov]

On 04/17/2018 11:50 AM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>

   How about the description (or 2)?

> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  5 -----
>  drivers/net/ethernet/renesas/ravb_main.c | 15 ---------------
>  2 files changed, 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 57eea4a77826..fcd04dbc7dde 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -197,15 +197,11 @@ enum ravb_reg {
>  	MAHR	= 0x05c0,
>  	MALR	= 0x05c8,
>  	TROCR	= 0x0700,	/* Undocumented? */
> -	CDCR	= 0x0708,	/* Undocumented? */
> -	LCCR	= 0x0710,	/* Undocumented? */
>  	CEFCR	= 0x0740,
>  	FRECR	= 0x0748,
>  	TSFRCR	= 0x0750,
>  	TLFRCR	= 0x0758,
>  	RFCR	= 0x0760,
> -	CERCR	= 0x0768,	/* Undocumented? */
> -	CEECR	= 0x0770,	/* Undocumented? */
>  	MAFCR	= 0x0778,
>  };
>  

   I think this is a material of patch #1... 

> @@ -223,7 +219,6 @@ enum CCC_BIT {
>  	CCC_CSEL_HPB	= 0x00010000,
>  	CCC_CSEL_ETH_TX	= 0x00020000,
>  	CCC_CSEL_GMII_REF = 0x00030000,
> -	CCC_BOC		= 0x00100000,	/* Undocumented? */
>  	CCC_LBME	= 0x01000000,

   ... and this -- of patch #2. Or vice versa. :-)

[...]

MBR, Sergei

[PATCH/RFC net-next 5/5] ravb: remove tx buffer addr 4byte alilgnment restriction for R-Car Gen3

[Simon Horman]

From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>

This patch sets from two descriptor to one descriptor because R-Car Gen3
does not have the 4 bytes alignment restriction of the transmission buffer.

Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/net/ethernet/renesas/ravb.h      |   6 +-
 drivers/net/ethernet/renesas/ravb_main.c | 131 +++++++++++++++++++------------
 2 files changed, 85 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index fcd04dbc7dde..3d0985305c26 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -964,7 +964,10 @@ enum RAVB_QUEUE {
 #define RX_QUEUE_OFFSET	4
 #define NUM_RX_QUEUE	2
 #define NUM_TX_QUEUE	2
-#define NUM_TX_DESC	2	/* TX descriptors per packet */
+
+/* TX descriptors per packet */
+#define NUM_TX_DESC_GEN2	2
+#define NUM_TX_DESC_GEN3	1
 
 struct ravb_tstamp_skb {
 	struct list_head list;
@@ -1043,6 +1046,7 @@ struct ravb_private {
 	unsigned no_avb_link:1;
 	unsigned avb_link_active_low:1;
 	unsigned wol_enabled:1;
+	int num_tx_desc;	/* TX descriptors per packet */
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 88056dd912ed..f137b62d5b52 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -189,12 +189,13 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
 	int free_num = 0;
 	int entry;
 	u32 size;
+	int num_tx_desc = priv->num_tx_desc;
 
 	for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) {
 		bool txed;
 
 		entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] *
-					     NUM_TX_DESC);
+					     num_tx_desc);
 		desc = &priv->tx_ring[q][entry];
 		txed = desc->die_dt == DT_FEMPTY;
 		if (free_txed_only && !txed)
@@ -203,12 +204,12 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
 		dma_rmb();
 		size = le16_to_cpu(desc->ds_tagl) & TX_DS;
 		/* Free the original skb. */
-		if (priv->tx_skb[q][entry / NUM_TX_DESC]) {
+		if (priv->tx_skb[q][entry / num_tx_desc]) {
 			dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
 					 size, DMA_TO_DEVICE);
 			/* Last packet descriptor? */
-			if (entry % NUM_TX_DESC == NUM_TX_DESC - 1) {
-				entry /= NUM_TX_DESC;
+			if (entry % num_tx_desc == num_tx_desc - 1) {
+				entry /= num_tx_desc;
 				dev_kfree_skb_any(priv->tx_skb[q][entry]);
 				priv->tx_skb[q][entry] = NULL;
 				if (txed)
@@ -229,6 +230,7 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 	struct ravb_private *priv = netdev_priv(ndev);
 	int ring_size;
 	int i;
+	int num_tx_desc = priv->num_tx_desc;
 
 	if (priv->rx_ring[q]) {
 		for (i = 0; i < priv->num_rx_ring[q]; i++) {
@@ -252,7 +254,7 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 		ravb_tx_free(ndev, q, false);
 
 		ring_size = sizeof(struct ravb_tx_desc) *
-			    (priv->num_tx_ring[q] * NUM_TX_DESC + 1);
+			    (priv->num_tx_ring[q] * num_tx_desc + 1);
 		dma_free_coherent(ndev->dev.parent, ring_size, priv->tx_ring[q],
 				  priv->tx_desc_dma[q]);
 		priv->tx_ring[q] = NULL;
@@ -284,9 +286,10 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 	struct ravb_ex_rx_desc *rx_desc;
 	struct ravb_tx_desc *tx_desc;
 	struct ravb_desc *desc;
+	int num_tx_desc = priv->num_tx_desc;
 	int rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
 	int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] *
-			   NUM_TX_DESC;
+			   num_tx_desc;
 	dma_addr_t dma_addr;
 	int i;
 
@@ -321,8 +324,10 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 	for (i = 0, tx_desc = priv->tx_ring[q]; i < priv->num_tx_ring[q];
 	     i++, tx_desc++) {
 		tx_desc->die_dt = DT_EEMPTY;
-		tx_desc++;
-		tx_desc->die_dt = DT_EEMPTY;
+		if (num_tx_desc >= 2) {
+			tx_desc++;
+			tx_desc->die_dt = DT_EEMPTY;
+		}
 	}
 	tx_desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
 	tx_desc->die_dt = DT_LINKFIX; /* type */
@@ -345,6 +350,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 	struct sk_buff *skb;
 	int ring_size;
 	int i;
+	int num_tx_desc = priv->num_tx_desc;
 
 	priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) +
 		ETH_HLEN + VLAN_HLEN;
@@ -383,7 +389,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 
 	/* Allocate all TX descriptors. */
 	ring_size = sizeof(struct ravb_tx_desc) *
-		    (priv->num_tx_ring[q] * NUM_TX_DESC + 1);
+		    (priv->num_tx_ring[q] * num_tx_desc + 1);
 	priv->tx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
 					      &priv->tx_desc_dma[q],
 					      GFP_KERNEL);
@@ -1533,10 +1539,11 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	void *buffer;
 	u32 entry;
 	u32 len;
+	int num_tx_desc = priv->num_tx_desc;
 
 	spin_lock_irqsave(&priv->lock, flags);
 	if (priv->cur_tx[q] - priv->dirty_tx[q] > (priv->num_tx_ring[q] - 1) *
-	    NUM_TX_DESC) {
+	    num_tx_desc) {
 		netif_err(priv, tx_queued, ndev,
 			  "still transmitting with the full ring!\n");
 		netif_stop_subqueue(ndev, q);
@@ -1547,41 +1554,55 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	if (skb_put_padto(skb, ETH_ZLEN))
 		goto exit;
 
-	entry = priv->cur_tx[q] % (priv->num_tx_ring[q] * NUM_TX_DESC);
-	priv->tx_skb[q][entry / NUM_TX_DESC] = skb;
-
-	buffer = PTR_ALIGN(priv->tx_align[q], DPTR_ALIGN) +
-		 entry / NUM_TX_DESC * DPTR_ALIGN;
-	len = PTR_ALIGN(skb->data, DPTR_ALIGN) - skb->data;
-	/* Zero length DMA descriptors are problematic as they seem to
-	 * terminate DMA transfers. Avoid them by simply using a length of
-	 * DPTR_ALIGN (4) when skb data is aligned to DPTR_ALIGN.
-	 *
-	 * As skb is guaranteed to have at least ETH_ZLEN (60) bytes of
-	 * data by the call to skb_put_padto() above this is safe with
-	 * respect to both the length of the first DMA descriptor (len)
-	 * overflowing the available data and the length of the second DMA
-	 * descriptor (skb->len - len) being negative.
-	 */
-	if (len == 0)
-		len = DPTR_ALIGN;
+	entry = priv->cur_tx[q] % (priv->num_tx_ring[q] * num_tx_desc);
+	priv->tx_skb[q][entry / num_tx_desc] = skb;
+
+	if (num_tx_desc >= 2) {
+		buffer = PTR_ALIGN(priv->tx_align[q], DPTR_ALIGN) +
+			 entry / num_tx_desc * DPTR_ALIGN;
+		len = PTR_ALIGN(skb->data, DPTR_ALIGN) - skb->data;
+
+		/* Zero length DMA descriptors are problematic as they seem
+		 * to terminate DMA transfers. Avoid them by simply using a
+		 * length of DPTR_ALIGN (4) when skb data is aligned to
+		 * DPTR_ALIGN.
+		 *
+		 * As skb is guaranteed to have at least ETH_ZLEN (60)
+		 * bytes of data by the call to skb_put_padto() above this
+		 * is safe with respect to both the length of the first DMA
+		 * descriptor (len) overflowing the available data and the
+		 * length of the second DMA descriptor (skb->len - len)
+		 * being negative.
+		 */
+		if (len == 0)
+			len = DPTR_ALIGN;
 
-	memcpy(buffer, skb->data, len);
-	dma_addr = dma_map_single(ndev->dev.parent, buffer, len, DMA_TO_DEVICE);
-	if (dma_mapping_error(ndev->dev.parent, dma_addr))
-		goto drop;
+		memcpy(buffer, skb->data, len);
+		dma_addr = dma_map_single(ndev->dev.parent, buffer, len,
+					  DMA_TO_DEVICE);
+		if (dma_mapping_error(ndev->dev.parent, dma_addr))
+			goto drop;
 
-	desc = &priv->tx_ring[q][entry];
-	desc->ds_tagl = cpu_to_le16(len);
-	desc->dptr = cpu_to_le32(dma_addr);
+		desc = &priv->tx_ring[q][entry];
+		desc->ds_tagl = cpu_to_le16(len);
+		desc->dptr = cpu_to_le32(dma_addr);
 
-	buffer = skb->data + len;
-	len = skb->len - len;
-	dma_addr = dma_map_single(ndev->dev.parent, buffer, len, DMA_TO_DEVICE);
-	if (dma_mapping_error(ndev->dev.parent, dma_addr))
-		goto unmap;
+		buffer = skb->data + len;
+		len = skb->len - len;
+		dma_addr = dma_map_single(ndev->dev.parent, buffer, len,
+					  DMA_TO_DEVICE);
+		if (dma_mapping_error(ndev->dev.parent, dma_addr))
+			goto unmap;
 
-	desc++;
+		desc++;
+	} else {
+		desc = &priv->tx_ring[q][entry];
+		len = skb->len;
+		dma_addr = dma_map_single(ndev->dev.parent, skb->data, skb->len,
+					  DMA_TO_DEVICE);
+		if (dma_mapping_error(ndev->dev.parent, dma_addr))
+			goto drop;
+	}
 	desc->ds_tagl = cpu_to_le16(len);
 	desc->dptr = cpu_to_le32(dma_addr);
 
@@ -1589,9 +1610,11 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	if (q == RAVB_NC) {
 		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
 		if (!ts_skb) {
-			desc--;
-			dma_unmap_single(ndev->dev.parent, dma_addr, len,
-					 DMA_TO_DEVICE);
+			if (num_tx_desc >= 2) {
+				desc--;
+				dma_unmap_single(ndev->dev.parent, dma_addr,
+						 len, DMA_TO_DEVICE);
+			}
 			goto unmap;
 		}
 		ts_skb->skb = skb;
@@ -1608,15 +1631,18 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	skb_tx_timestamp(skb);
 	/* Descriptor type must be set after all the above writes */
 	dma_wmb();
-	desc->die_dt = DT_FEND;
-	desc--;
-	desc->die_dt = DT_FSTART;
-
+	if (num_tx_desc > 1) {
+		desc->die_dt = DT_FEND;
+		desc--;
+		desc->die_dt = DT_FSTART;
+	} else {
+		desc->die_dt = DT_FSINGLE;
+	}
 	ravb_modify(ndev, TCCR, TCCR_TSRQ0 << q, TCCR_TSRQ0 << q);
 
-	priv->cur_tx[q] += NUM_TX_DESC;
+	priv->cur_tx[q] += num_tx_desc;
 	if (priv->cur_tx[q] - priv->dirty_tx[q] >
-	    (priv->num_tx_ring[q] - 1) * NUM_TX_DESC &&
+	    (priv->num_tx_ring[q] - 1) * num_tx_desc &&
 	    !ravb_tx_free(ndev, q, true))
 		netif_stop_subqueue(ndev, q);
 
@@ -1630,7 +1656,7 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 			 le16_to_cpu(desc->ds_tagl), DMA_TO_DEVICE);
 drop:
 	dev_kfree_skb_any(skb);
-	priv->tx_skb[q][entry / NUM_TX_DESC] = NULL;
+	priv->tx_skb[q][entry / num_tx_desc] = NULL;
 	goto exit;
 }
 
@@ -2106,6 +2132,9 @@ static int ravb_probe(struct platform_device *pdev)
 	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
 	ndev->min_mtu = ETH_MIN_MTU;
 
+	priv->num_tx_desc = (chip_id == RCAR_GEN2) ?
+		NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3;
+
 	/* Set function */
 	ndev->netdev_ops = &ravb_netdev_ops;
 	ndev->ethtool_ops = &ravb_ethtool_ops;
-- 
2.11.0

Re: [PATCH/RFC net-next 5/5] ravb: remove tx buffer addr 4byte alilgnment restriction for R-Car Gen3

[Sergei Shtylyov]

Hello!

On 4/17/2018 11:50 AM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> 
> This patch sets from two descriptor to one descriptor because R-Car Gen3
> does not have the 4 bytes alignment restriction of the transmission buffer.
> 
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index fcd04dbc7dde..3d0985305c26 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -964,7 +964,10 @@ enum RAVB_QUEUE {
>  #define RX_QUEUE_OFFSET	4
>  #define NUM_RX_QUEUE	2
>  #define NUM_TX_QUEUE	2
> -#define NUM_TX_DESC	2	/* TX descriptors per packet */
> +
> +/* TX descriptors per packet */
> +#define NUM_TX_DESC_GEN2	2
> +#define NUM_TX_DESC_GEN3	1

    We hanrdly need these...

>  
>  struct ravb_tstamp_skb {
>  	struct list_head list;
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 88056dd912ed..f137b62d5b52 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -189,12 +189,13 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
>  	int free_num = 0;
>  	int entry;
>  	u32 size;
> +	int num_tx_desc = priv->num_tx_desc;

    Please keep sorting the declarations by length -- that's DaveM's pet peeve 
(and indeed looks prettier. :-)

[...]
> @@ -229,6 +230,7 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	int ring_size;
>  	int i;
> +	int num_tx_desc = priv->num_tx_desc;

    Here as well.

>  
>  	if (priv->rx_ring[q]) {
>  	
>  		for (i = 0; i < priv->num_rx_ring[q]; i++) {
[...]
> @@ -321,8 +324,10 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>   	for (i = 0, tx_desc = priv->tx_ring[q]; i < priv->num_tx_ring[q];
>   	     i++, tx_desc++) {
>   		tx_desc->die_dt = DT_EEMPTY;
> -		tx_desc++;
> -		tx_desc->die_dt = DT_EEMPTY;
> +		if (num_tx_desc >= 2) {

    > 1, please.
    Strictly speaking, this only works when num_tx_desc == 2, however that's 
my fault...

> +			tx_desc++;
> +			tx_desc->die_dt = DT_EEMPTY;
> +		}
>   	}
>   	tx_desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
>   	tx_desc->die_dt = DT_LINKFIX; /* type */
> @@ -345,6 +350,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>   	struct sk_buff *skb;
>   	int ring_size;
>   	int i;
> +	int num_tx_desc = priv->num_tx_desc;

    Again, please keep the descarations sorted by length.

>   
>   	priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) +
>   		ETH_HLEN + VLAN_HLEN;
[...]
> @@ -1533,10 +1539,11 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>   	void *buffer;
>   	u32 entry;
>   	u32 len;
> +	int num_tx_desc = priv->num_tx_desc;

    Here as well...

[...]
> @@ -1547,41 +1554,55 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
[...]
> +	entry = priv->cur_tx[q] % (priv->num_tx_ring[q] * num_tx_desc);
> +	priv->tx_skb[q][entry / num_tx_desc] = skb;
> +
> +	if (num_tx_desc >= 2) {

    > 1, please.

[...]
> @@ -1589,9 +1610,11 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>   	if (q == RAVB_NC) {
>   		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
>   		if (!ts_skb) {
> -			desc--;
> -			dma_unmap_single(ndev->dev.parent, dma_addr, len,
> -					 DMA_TO_DEVICE);
> +			if (num_tx_desc >= 2) {

    Likewise.

> +				desc--;
> +				dma_unmap_single(ndev->dev.parent, dma_addr,
> +						 len, DMA_TO_DEVICE);
> +			}
>   			goto unmap;
>   		}
>   		ts_skb->skb = skb;
[...]
> @@ -2106,6 +2132,9 @@ static int ravb_probe(struct platform_device *pdev)
>   	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
>   	ndev->min_mtu = ETH_MIN_MTU;
>   
> +	priv->num_tx_desc = (chip_id == RCAR_GEN2) ?

    Parens not needed.

> +		NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3;

    Just 2 : 1;

[...]

    However... I'm not seeing this patch disabling memory allocation for 
priv->tx_align[] and reducing the memory pressure is one of 2 reasons for this 
patch, isn't it?

MBR, Sergei