Re: [PATCH v2 1/3] net: smc91x: isolate u16 writes alignment workaround
Robert Jarzmik writes: > diff --git a/drivers/net/ethernet/smsc/smc91x.h > b/drivers/net/ethernet/smsc/smc91x.h > index ea8465467469..dff165ed106d 100644 > --- a/drivers/net/ethernet/smsc/smc91x.h > +++ b/drivers/net/ethernet/smsc/smc91x.h And there is also the specific case of ARCH=MN10300, where I didn't see : #include In which SMC_outw() is also defined ... sic. So this is also to be fixed, thanks to kbuild test robot. -- Robert
Re: [PATCH v2 1/3] net: smc91x: isolate u16 writes alignment workaround
Sorry David, I just noticed you weren't in the "To:" of this serie, but I won't forget you for the v3 I need to release anyway (https://lkml.org/lkml/2016/10/15/104). Robert Jarzmik writes: > + lp->half_word_align4 = > + machine_is_mainstone() || machine_is_stargate2() || > + machine_is_pxa_idp(); Bah this one is not good enough. First, machine_is_*() is not defined if CONFIG_ARM=n, and this part is not under a #ifdef CONFIG_ARM. Moreover, I think it is a good occasion to go further, and : - enhance smc91x_platdata and add a pxa_u16_align4 boolean - transform this statement into : lp->half_word_align4 = lp->cfg.pxa_u16_align4 This will remove the machine_*() calls from the smc91x driver, which looks a good move, doesn't it ? Cheers. -- Robert
[PATCH v2 1/3] net: smc91x: isolate u16 writes alignment workaround
Writes to u16 has a special handling on 3 PXA platforms, where the
hardware wiring forces these writes to be u32 aligned.
This patch isolates this handling for PXA platforms as before, but
enables this "workaround" to be set up dynamically, which will be the
case in device-tree build types.
This patch was tested on 2 PXA platforms : mainstone, which relies on
the workaround, and lubbock, which doesn't.
Signed-off-by: Robert Jarzmik
---
drivers/net/ethernet/smsc/smc91x.c | 6 ++-
drivers/net/ethernet/smsc/smc91x.h | 78 +-
2 files changed, 48 insertions(+), 36 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smc91x.c
b/drivers/net/ethernet/smsc/smc91x.c
index 9b4780f87863..5658c2b28ec8 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -602,7 +602,8 @@ static void smc_hardware_send_pkt(unsigned long data)
SMC_PUSH_DATA(lp, buf, len & ~1);
/* Send final ctl word with the last byte if there is one */
- SMC_outw(((len & 1) ? (0x2000 | buf[len-1]) : 0), ioaddr, DATA_REG(lp));
+ SMC_outw(lp, ((len & 1) ? (0x2000 | buf[len-1]) : 0), ioaddr,
+DATA_REG(lp));
/*
* If THROTTLE_TX_PKTS is set, we stop the queue here. This will
@@ -2282,6 +2283,9 @@ static int smc_drv_probe(struct platform_device *pdev)
goto out_free_netdev;
}
}
+ lp->half_word_align4 =
+ machine_is_mainstone() || machine_is_stargate2() ||
+ machine_is_pxa_idp();
#if IS_BUILTIN(CONFIG_OF)
match = of_match_device(of_match_ptr(smc91x_match), &pdev->dev);
diff --git a/drivers/net/ethernet/smsc/smc91x.h
b/drivers/net/ethernet/smsc/smc91x.h
index ea8465467469..dff165ed106d 100644
--- a/drivers/net/ethernet/smsc/smc91x.h
+++ b/drivers/net/ethernet/smsc/smc91x.h
@@ -86,11 +86,11 @@
#define SMC_inl(a, r) readl((a) + (r))
#define SMC_outb(v, a, r) writeb(v, (a) + (r))
-#define SMC_outw(v, a, r) \
+#define SMC_outw(lp, v, a, r) \
do {\
unsigned int __v = v, __smc_r = r; \
if (SMC_16BIT(lp)) \
- __SMC_outw(__v, a, __smc_r);\
+ __SMC_outw(lp, __v, a, __smc_r);\
else if (SMC_8BIT(lp)) \
SMC_outw_b(__v, a, __smc_r);\
else\
@@ -107,10 +107,10 @@
#define SMC_IRQ_FLAGS (-1)/* from resource */
/* We actually can't write halfwords properly if not word aligned */
-static inline void __SMC_outw(u16 val, void __iomem *ioaddr, int reg)
+static inline void _SMC_outw_align4(u16 val, void __iomem *ioaddr, int reg,
+ bool use_align4_workaround)
{
- if ((machine_is_mainstone() || machine_is_stargate2() ||
-machine_is_pxa_idp()) && reg & 2) {
+ if (use_align4_workaround) {
unsigned int v = val << 16;
v |= readl(ioaddr + (reg & ~2)) & 0x;
writel(v, ioaddr + (reg & ~2));
@@ -119,6 +119,12 @@ static inline void __SMC_outw(u16 val, void __iomem
*ioaddr, int reg)
}
}
+#define __SMC_outw(lp, v, a, r)
\
+ _SMC_outw_align4((v), (a), (r), \
+IS_BUILTIN(CONFIG_ARCH_PXA) && ((r) & 2) &&\
+lp->half_word_align4)
+
+
#elif defined(CONFIG_SH_SH4202_MICRODEV)
#define SMC_CAN_USE_8BIT 0
@@ -129,7 +135,7 @@ static inline void __SMC_outw(u16 val, void __iomem
*ioaddr, int reg)
#define SMC_inw(a, r) inw((a) + (r) - 0xa000)
#define SMC_inl(a, r) inl((a) + (r) - 0xa000)
#define SMC_outb(v, a, r) outb(v, (a) + (r) - 0xa000)
-#define SMC_outw(v, a, r) outw(v, (a) + (r) - 0xa000)
+#define SMC_outw(lp, v, a, r) outw(v, (a) + (r) - 0xa000)
#define SMC_outl(v, a, r) outl(v, (a) + (r) - 0xa000)
#define SMC_insl(a, r, p, l) insl((a) + (r) - 0xa000, p, l)
#define SMC_outsl(a, r, p, l) outsl((a) + (r) - 0xa000, p, l)
@@ -147,7 +153,7 @@ static inline void __SMC_outw(u16 val, void __iomem
*ioaddr, int reg)
#define SMC_inb(a, r) inb(((u32)a) + (r))
#define SMC_inw(a, r) inw(((u32)a) + (r))
#define SMC_outb(v, a, r) outb(v, ((u32)a) + (r))
-#define SMC_outw(v, a, r) outw(v, ((u32)a) + (r))
+#define SMC_outw(lp, v, a, r) outw(v, ((u32)a) + (r))
#define SMC_insw(a, r, p, l) insw(((u32)a) + (r), p, l)
#define SMC_outsw(a, r, p, l) outsw(((u32)a) + (r), p, l)
@@ -175,7 +181,7 @@ static
Re: [PATCH v2 1/3] net: smc91x: isolate u16 writes alignment workaround
David Miller writes: > From: Robert Jarzmik > Date: Sun, 9 Oct 2016 22:33:45 +0200 > >> Writes to u16 has a special handling on 3 PXA platforms, where the >> hardware wiring forces these writes to be u32 aligned. >> >> This patch isolates this handling for PXA platforms as before, but >> enables this "workaround" to be set up dynamically, which will be the >> case in device-tree build types. >> >> This patch was tested on 2 PXA platforms : mainstone, which relies on >> the workaround, and lubbock, which doesn't. >> >> Signed-off-by: Robert Jarzmik > > Please resubmit this patch series: > > 1) Respun against net-next, these don't currently apply cleanly there. > > 2) With a proper "[PATCH 0/3] ..." posting explaining at a high level >what this patch series does, how it does it, and why it does it >that way. Sure, let me retest it after the rebase, and I'll post again. Cheers. -- Robert
Re: [PATCH v2 1/3] net: smc91x: isolate u16 writes alignment workaround
From: Robert Jarzmik Date: Sun, 9 Oct 2016 22:33:45 +0200 > Writes to u16 has a special handling on 3 PXA platforms, where the > hardware wiring forces these writes to be u32 aligned. > > This patch isolates this handling for PXA platforms as before, but > enables this "workaround" to be set up dynamically, which will be the > case in device-tree build types. > > This patch was tested on 2 PXA platforms : mainstone, which relies on > the workaround, and lubbock, which doesn't. > > Signed-off-by: Robert Jarzmik Please resubmit this patch series: 1) Respun against net-next, these don't currently apply cleanly there. 2) With a proper "[PATCH 0/3] ..." posting explaining at a high level what this patch series does, how it does it, and why it does it that way. Thanks.
Re: [PATCH v2 1/3] net: smc91x: isolate u16 writes alignment workaround
Andy Shevchenko writes: >> +#define SMC_outw(lp, v, a, r) \ >> + _SMC_outw_align4((v), (a), (r), \ >> +IS_BUILTIN(CONFIG_ARCH_PXA) && ((r) & 2) &&\ >> +lp->half_word_align4) > > Hmm... Isn't enough to have just (r) & 2 && lp->half_word_align4 ? It wouldn't be equivalent to what we had before. The point of the previous code was to compile out as much as possible of this test. Therefore, at compilation time for omap1 boards, the compiler would evaluate the test to 0, and never leave the workaround code compiled. So it would be enough, but worse performance wise and not equivalent for non-pxa boards, hence this test. Cheers. -- Robert
Re: [PATCH v2 1/3] net: smc91x: isolate u16 writes alignment workaround
On Sun, Oct 9, 2016 at 11:33 PM, Robert Jarzmik wrote:
> Writes to u16 has a special handling on 3 PXA platforms, where the
> hardware wiring forces these writes to be u32 aligned.
>
> This patch isolates this handling for PXA platforms as before, but
> enables this "workaround" to be set up dynamically, which will be the
> case in device-tree build types.
>
> This patch was tested on 2 PXA platforms : mainstone, which relies on
> the workaround, and lubbock, which doesn't.
> @@ -2276,6 +2277,9 @@ static int smc_drv_probe(struct platform_device *pdev)
> memcpy(&lp->cfg, pd, sizeof(lp->cfg));
> lp->io_shift = SMC91X_IO_SHIFT(lp->cfg.flags);
> }
> + lp->half_word_align4 =
> + machine_is_mainstone() || machine_is_stargate2() ||
> + machine_is_pxa_idp();
> /* We actually can't write halfwords properly if not word aligned */
> -static inline void SMC_outw(u16 val, void __iomem *ioaddr, int reg)
> +static inline void _SMC_outw_align4(u16 val, void __iomem *ioaddr, int reg,
> + bool use_align4_workaround)
> {
> - if ((machine_is_mainstone() || machine_is_stargate2() ||
> -machine_is_pxa_idp()) && reg & 2) {
> + if (use_align4_workaround) {
> unsigned int v = val << 16;
> v |= readl(ioaddr + (reg & ~2)) & 0x;
> writel(v, ioaddr + (reg & ~2));
> +#define SMC_outw(lp, v, a, r) \
> + _SMC_outw_align4((v), (a), (r), \
> +IS_BUILTIN(CONFIG_ARCH_PXA) && ((r) & 2) &&\
> +lp->half_word_align4)
Hmm... Isn't enough to have just (r) & 2 && lp->half_word_align4 ?
--
With Best Regards,
Andy Shevchenko
[PATCH v2 1/3] net: smc91x: isolate u16 writes alignment workaround
Writes to u16 has a special handling on 3 PXA platforms, where the
hardware wiring forces these writes to be u32 aligned.
This patch isolates this handling for PXA platforms as before, but
enables this "workaround" to be set up dynamically, which will be the
case in device-tree build types.
This patch was tested on 2 PXA platforms : mainstone, which relies on
the workaround, and lubbock, which doesn't.
Signed-off-by: Robert Jarzmik
---
drivers/net/ethernet/smsc/smc91x.c | 6 ++-
drivers/net/ethernet/smsc/smc91x.h | 80 ++
2 files changed, 52 insertions(+), 34 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smc91x.c
b/drivers/net/ethernet/smsc/smc91x.c
index 77ad2a3f59db..b1f74e06d98e 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -602,7 +602,8 @@ static void smc_hardware_send_pkt(unsigned long data)
SMC_PUSH_DATA(lp, buf, len & ~1);
/* Send final ctl word with the last byte if there is one */
- SMC_outw(((len & 1) ? (0x2000 | buf[len-1]) : 0), ioaddr, DATA_REG(lp));
+ SMC_outw(lp, ((len & 1) ? (0x2000 | buf[len-1]) : 0), ioaddr,
+DATA_REG(lp));
/*
* If THROTTLE_TX_PKTS is set, we stop the queue here. This will
@@ -2276,6 +2277,9 @@ static int smc_drv_probe(struct platform_device *pdev)
memcpy(&lp->cfg, pd, sizeof(lp->cfg));
lp->io_shift = SMC91X_IO_SHIFT(lp->cfg.flags);
}
+ lp->half_word_align4 =
+ machine_is_mainstone() || machine_is_stargate2() ||
+ machine_is_pxa_idp();
#if IS_BUILTIN(CONFIG_OF)
match = of_match_device(of_match_ptr(smc91x_match), &pdev->dev);
diff --git a/drivers/net/ethernet/smsc/smc91x.h
b/drivers/net/ethernet/smsc/smc91x.h
index 1a55c7976df0..2b7752db8635 100644
--- a/drivers/net/ethernet/smsc/smc91x.h
+++ b/drivers/net/ethernet/smsc/smc91x.h
@@ -66,10 +66,10 @@
#define SMC_IRQ_FLAGS (-1)/* from resource */
/* We actually can't write halfwords properly if not word aligned */
-static inline void SMC_outw(u16 val, void __iomem *ioaddr, int reg)
+static inline void _SMC_outw_align4(u16 val, void __iomem *ioaddr, int reg,
+ bool use_align4_workaround)
{
- if ((machine_is_mainstone() || machine_is_stargate2() ||
-machine_is_pxa_idp()) && reg & 2) {
+ if (use_align4_workaround) {
unsigned int v = val << 16;
v |= readl(ioaddr + (reg & ~2)) & 0x;
writel(v, ioaddr + (reg & ~2));
@@ -78,6 +78,12 @@ static inline void SMC_outw(u16 val, void __iomem *ioaddr,
int reg)
}
}
+#define SMC_outw(lp, v, a, r) \
+ _SMC_outw_align4((v), (a), (r), \
+IS_BUILTIN(CONFIG_ARCH_PXA) && ((r) & 2) &&\
+lp->half_word_align4)
+
+
#elif defined(CONFIG_SH_SH4202_MICRODEV)
#define SMC_CAN_USE_8BIT 0
@@ -88,7 +94,8 @@ static inline void SMC_outw(u16 val, void __iomem *ioaddr,
int reg)
#define SMC_inw(a, r) inw((a) + (r) - 0xa000)
#define SMC_inl(a, r) inl((a) + (r) - 0xa000)
#define SMC_outb(v, a, r) outb(v, (a) + (r) - 0xa000)
-#define SMC_outw(v, a, r) outw(v, (a) + (r) - 0xa000)
+#define _SMC_outw(v, a, r) outw(v, (a) + (r) - 0xa000)
+#define SMC_outw(lp, v, a, r) _SMC_outw((v), (a), (r))
#define SMC_outl(v, a, r) outl(v, (a) + (r) - 0xa000)
#define SMC_insl(a, r, p, l) insl((a) + (r) - 0xa000, p, l)
#define SMC_outsl(a, r, p, l) outsl((a) + (r) - 0xa000, p, l)
@@ -106,7 +113,8 @@ static inline void SMC_outw(u16 val, void __iomem *ioaddr,
int reg)
#define SMC_inb(a, r) inb(((u32)a) + (r))
#define SMC_inw(a, r) inw(((u32)a) + (r))
#define SMC_outb(v, a, r) outb(v, ((u32)a) + (r))
-#define SMC_outw(v, a, r) outw(v, ((u32)a) + (r))
+#define _SMC_outw(v, a, r) outw(v, ((u32)a) + (r))
+#define SMC_outw(lp, v, a, r) _SMC_outw((v), (a), (r))
#define SMC_insw(a, r, p, l) insw(((u32)a) + (r), p, l)
#define SMC_outsw(a, r, p, l) outsw(((u32)a) + (r), p, l)
@@ -134,7 +142,8 @@ static inline void SMC_outw(u16 val, void __iomem *ioaddr,
int reg)
#define SMC_inw(a, r) readw((a) + (r))
#define SMC_inl(a, r) readl((a) + (r))
#define SMC_outb(v, a, r) writeb(v, (a) + (r))
-#define SMC_outw(v, a, r) writew(v, (a) + (r))
+#define _SMC_outw(v, a, r) writew(v, (a) + (r))
+#define SMC_outw(lp, v, a, r) _SMC_outw((v), (a), (r))
#define SMC_outl(v, a, r) writel(v, (a) + (r))
#define SMC_insw(a, r, p, l)readsw((a) + (r), p, l)
#define SMC_outsw(a, r, p, l) writesw((a) + (r), p, l)
@@ -166,7 +175,8 @@ static inline void mcf_outsw(void *a, unsigned char *p, int
l)
}
#define SMC_inw(a, r) _swapw(readw((a) + (r)))
-#define SMC_outw(v, a
