* [PATCH] net: sh_eth alignment fix for sh7724 using NET_IP_ALIGN
@ 2009-12-14 10:34 Magnus Damm
2009-12-15 5:57 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Magnus Damm @ 2009-12-14 10:34 UTC (permalink / raw)
To: netdev; +Cc: Magnus Damm, lethal, davem, linux-sh
From: Magnus Damm <damm@opensource•se>
Fix sh_eth for sh7724 by adding NET_IP_ALIGN support.
Without this patch the receive data is misaligned.
Signed-off-by: Magnus Damm <damm@opensource•se>
---
Tested on Ecovec24. Perhaps for the sh-2.6 tree?
drivers/net/sh_eth.c | 8 ++++++++
1 file changed, 8 insertions(+)
--- 0001/drivers/net/sh_eth.c
+++ work/drivers/net/sh_eth.c 2009-12-14 15:27:25.000000000 +0900
@@ -84,6 +84,10 @@ static struct sh_eth_cpu_data sh_eth_my_
.mpr = 1,
.tpauser = 1,
.hw_swap = 1,
+#if NET_IP_ALIGN == 2
+ .rpadir = 1,
+ .rpadir_value = 0x00020000,
+#endif
};
#elif defined(CONFIG_CPU_SUBTYPE_SH7763)
@@ -501,6 +505,8 @@ static int sh_eth_ring_init(struct net_d
*/
mdp->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ :
(((ndev->mtu + 26 + 7) & ~7) + 2 + 16));
+ if (mdp->cd->rpadir)
+ mdp->rx_buf_sz += NET_IP_ALIGN;
/* Allocate RX and TX skb rings */
mdp->rx_skbuff = kmalloc(sizeof(*mdp->rx_skbuff) * RX_RING_SIZE,
@@ -715,6 +721,8 @@ static int sh_eth_rx(struct net_device *
pkt_len + 2);
skb = mdp->rx_skbuff[entry];
mdp->rx_skbuff[entry] = NULL;
+ if (mdp->cd->rpadir)
+ skb_reserve(skb, NET_IP_ALIGN);
skb_put(skb, pkt_len);
skb->protocol = eth_type_trans(skb, ndev);
netif_rx(skb);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sh_eth alignment fix for sh7724 using NET_IP_ALIGN
2009-12-14 10:34 [PATCH] net: sh_eth alignment fix for sh7724 using NET_IP_ALIGN Magnus Damm
@ 2009-12-15 5:57 ` David Miller
2009-12-15 7:13 ` Magnus Damm
0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2009-12-15 5:57 UTC (permalink / raw)
To: magnus.damm; +Cc: netdev, lethal, linux-sh
From: Magnus Damm <magnus.damm@gmail•com>
Date: Mon, 14 Dec 2009 19:34:43 +0900
> From: Magnus Damm <damm@opensource•se>
>
> Fix sh_eth for sh7724 by adding NET_IP_ALIGN support.
> Without this patch the receive data is misaligned.
>
> Signed-off-by: Magnus Damm <damm@opensource•se>
How does this interact with sh_eth_set_receive_align() and
why isn't all of the skb_reserve() logic confined to one
place?
The sh7763 sh_eth_my_cpu_data sets rpadir unconditionally,
yet your NET_IP_ALIGN behavior is controlled by the new
setting of .rpadir for sh7724, is this ok or is it going
to break sh7763 or cause it to misbehave?
Could you possibly generalize this so that you don't
need the ifdefs and it doesn't matter what the NET_IP_ALIGN
value actually is? That is making this code more confusing
than it needs to be.
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sh_eth alignment fix for sh7724 using NET_IP_ALIGN
2009-12-15 5:57 ` David Miller
@ 2009-12-15 7:13 ` Magnus Damm
2009-12-15 7:15 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Magnus Damm @ 2009-12-15 7:13 UTC (permalink / raw)
To: David Miller; +Cc: netdev, lethal, linux-sh
On Tue, Dec 15, 2009 at 2:57 PM, David Miller <davem@davemloft•net> wrote:
> From: Magnus Damm <magnus.damm@gmail•com>
> Date: Mon, 14 Dec 2009 19:34:43 +0900
>
>> From: Magnus Damm <damm@opensource•se>
>>
>> Fix sh_eth for sh7724 by adding NET_IP_ALIGN support.
>> Without this patch the receive data is misaligned.
>>
>> Signed-off-by: Magnus Damm <damm@opensource•se>
>
> How does this interact with sh_eth_set_receive_align() and
> why isn't all of the skb_reserve() logic confined to one
> place?
It looks like the driver is written to support a few different
processors, and the alignment requirement varies from processor to
processor. I assume that the mdp->rx_buf_sz calculation takes care of
the alignment, but it all looks pretty messy to me.
Some processors support RPADIR and some do not. The processors that do
support RPADIR can tie in NET_IP_ALIGN support easily. The sh7763
hardware should support RPADIR, but only a partial software
implementation seems to be in place without this patch. And I can't
unfortunately test sh7763 support since I only have sh7724 hardware
available.
That aside, unifying the alignment code and doing a major cleanup
would be nice, yes.
> The sh7763 sh_eth_my_cpu_data sets rpadir unconditionally,
> yet your NET_IP_ALIGN behavior is controlled by the new
> setting of .rpadir for sh7724, is this ok or is it going
> to break sh7763 or cause it to misbehave?
Good point. It may be better to remove the .rpadir setting from sh7763
since it looks unused and broken at this point. OTOH, if RPADIR on
sh7763 does what I suspect then this patch will actually unbreak
sh7763 using the skb_reserve() hunk.
> Could you possibly generalize this so that you don't
> need the ifdefs and it doesn't matter what the NET_IP_ALIGN
> value actually is? That is making this code more confusing
> than it needs to be.
Sorry about the confusion, but the #ifdef was actually intentional. =)
I can't find any description of the RPADIR register in the sh7724 data
sheet, but testing shows that the buffers are adjusted properly. So
since I don't know the register layout I prefer to optimize for the
known working combination of NET_IP_ALIGN == 2 and simply disable
.rpadir usage for other NET_IP_ALIGN cases.
Thanks!
/ magnus
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sh_eth alignment fix for sh7724 using NET_IP_ALIGN
2009-12-15 7:13 ` Magnus Damm
@ 2009-12-15 7:15 ` David Miller
2009-12-15 7:22 ` Magnus Damm
0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2009-12-15 7:15 UTC (permalink / raw)
To: magnus.damm; +Cc: netdev, lethal, linux-sh
From: Magnus Damm <magnus.damm@gmail•com>
Date: Tue, 15 Dec 2009 16:13:47 +0900
> Sorry about the confusion, but the #ifdef was actually intentional. =)
> I can't find any description of the RPADIR register in the sh7724 data
> sheet, but testing shows that the buffers are adjusted properly. So
> since I don't know the register layout I prefer to optimize for the
> known working combination of NET_IP_ALIGN == 2 and simply disable
> .rpadir usage for other NET_IP_ALIGN cases.
NET_IP_ALIGN is never != 2 on SUPERH
SH_ETH depends on SUPERH
It's a fantasy ifdef, of sorts. :-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sh_eth alignment fix for sh7724 using NET_IP_ALIGN
2009-12-15 7:15 ` David Miller
@ 2009-12-15 7:22 ` Magnus Damm
2009-12-15 7:28 ` Magnus Damm
2009-12-15 7:34 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Magnus Damm @ 2009-12-15 7:22 UTC (permalink / raw)
To: David Miller; +Cc: netdev, lethal, linux-sh
On Tue, Dec 15, 2009 at 4:15 PM, David Miller <davem@davemloft•net> wrote:
> NET_IP_ALIGN is never != 2 on SUPERH
>
> SH_ETH depends on SUPERH
>
> It's a fantasy ifdef, of sorts. :-)
Sure, but _maybe_ someone will put the sh_eth hardware block together
with MIPS or ARM in the future. I guess it makes more sense to just
use the NET_IP_ALIGN value blindly and not care so much about what is
known working and what is not...?
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sh_eth alignment fix for sh7724 using NET_IP_ALIGN
2009-12-15 7:22 ` Magnus Damm
@ 2009-12-15 7:28 ` Magnus Damm
2009-12-15 7:34 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: Magnus Damm @ 2009-12-15 7:28 UTC (permalink / raw)
To: David Miller; +Cc: netdev, lethal, linux-sh
On Tue, Dec 15, 2009 at 4:22 PM, Magnus Damm <magnus.damm@gmail•com> wrote:
> On Tue, Dec 15, 2009 at 4:15 PM, David Miller <davem@davemloft•net> wrote:
>> NET_IP_ALIGN is never != 2 on SUPERH
>>
>> SH_ETH depends on SUPERH
>>
>> It's a fantasy ifdef, of sorts. :-)
>
> Sure, but _maybe_ someone will put the sh_eth hardware block together
> with MIPS or ARM in the future. I guess it makes more sense to just
> use the NET_IP_ALIGN value blindly and not care so much about what is
> known working and what is not...?
Well, it's in the sh7724 specific portion of the code and I guess
that's SUPERH. =)
/ magnus
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sh_eth alignment fix for sh7724 using NET_IP_ALIGN
2009-12-15 7:22 ` Magnus Damm
2009-12-15 7:28 ` Magnus Damm
@ 2009-12-15 7:34 ` David Miller
2009-12-15 7:36 ` Magnus Damm
1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2009-12-15 7:34 UTC (permalink / raw)
To: magnus.damm; +Cc: netdev, lethal, linux-sh
From: Magnus Damm <magnus.damm@gmail•com>
Date: Tue, 15 Dec 2009 16:22:11 +0900
> On Tue, Dec 15, 2009 at 4:15 PM, David Miller <davem@davemloft•net> wrote:
>> NET_IP_ALIGN is never != 2 on SUPERH
>>
>> SH_ETH depends on SUPERH
>>
>> It's a fantasy ifdef, of sorts. :-)
>
> Sure, but _maybe_ someone will put the sh_eth hardware block together
> with MIPS or ARM in the future.
MIPS and ARM use the default NET_IP_ALIGN of 2 as well.
Next?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sh_eth alignment fix for sh7724 using NET_IP_ALIGN
2009-12-15 7:34 ` David Miller
@ 2009-12-15 7:36 ` Magnus Damm
0 siblings, 0 replies; 8+ messages in thread
From: Magnus Damm @ 2009-12-15 7:36 UTC (permalink / raw)
To: David Miller; +Cc: netdev, lethal, linux-sh
On Tue, Dec 15, 2009 at 4:34 PM, David Miller <davem@davemloft•net> wrote:
> From: Magnus Damm <magnus.damm@gmail•com>
> Date: Tue, 15 Dec 2009 16:22:11 +0900
>
>> On Tue, Dec 15, 2009 at 4:15 PM, David Miller <davem@davemloft•net> wrote:
>>> NET_IP_ALIGN is never != 2 on SUPERH
>>>
>>> SH_ETH depends on SUPERH
>>>
>>> It's a fantasy ifdef, of sorts. :-)
>>
>> Sure, but _maybe_ someone will put the sh_eth hardware block together
>> with MIPS or ARM in the future.
>
> MIPS and ARM use the default NET_IP_ALIGN of 2 as well.
>
> Next?
I'll just get rid of the #ifdef and make a V2. Thanks! =)
/ magnus
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-12-15 7:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-12-14 10:34 [PATCH] net: sh_eth alignment fix for sh7724 using NET_IP_ALIGN Magnus Damm
2009-12-15 5:57 ` David Miller
2009-12-15 7:13 ` Magnus Damm
2009-12-15 7:15 ` David Miller
2009-12-15 7:22 ` Magnus Damm
2009-12-15 7:28 ` Magnus Damm
2009-12-15 7:34 ` David Miller
2009-12-15 7:36 ` Magnus Damm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox