[PATCH 1/3] net: smc91x: isolate u16 writes alignment workaround

[PATCH 1/3] net: smc91x: isolate u16 writes alignment workaround

[Robert Jarzmik]

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 <robert.jarzmik@free.fr>
---
 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)) & 0xffff;
 		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) - 0xa0000000)
 #define SMC_inl(a, r)		inl((a) + (r) - 0xa0000000)
 #define SMC_outb(v, a, r)	outb(v, (a) + (r) - 0xa0000000)
-#define SMC_outw(v, a, r)	outw(v, (a) + (r) - 0xa0000000)
+#define _SMC_outw(v, a, r)	outw(v, (a) + (r) - 0xa0000000)
+#define SMC_outw(lp, v, a, r)	_SMC_outw((v), (a), (r))
 #define SMC_outl(v, a, r)	outl(v, (a) + (r) - 0xa0000000)
 #define SMC_insl(a, r, p, l)	insl((a) + (r) - 0xa0000000, p, l)
 #define SMC_outsl(a, r, p, l)	outsl((a) + (r) - 0xa0000000, 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, r)	writew(_swapw(v), (a) + (r))
+#define _SMC_outw(v, a, r)	writew(_swapw(v), (a) + (r))
+#define SMC_outw(lp, v, a, r)	_SMC_outw((v), (a), (r))
 #define SMC_insw(a, r, p, l)	mcf_insw(a + r, p, l)
 #define SMC_outsw(a, r, p, l)	mcf_outsw(a + r, p, l)
 
@@ -200,7 +210,8 @@ static inline void mcf_outsw(void *a, unsigned char *p, int l)
 #define SMC_inw(a, r)		ioread16((a) + (r))
 #define SMC_inl(a, r)		ioread32((a) + (r))
 #define SMC_outb(v, a, r)	iowrite8(v, (a) + (r))
-#define SMC_outw(v, a, r)	iowrite16(v, (a) + (r))
+#define _SMC_outw(v, a, r)	iowrite16(v, (a) + (r))
+#define SMC_outw(lp, v, a, r)	_SMC_outw((v), (a), (r))
 #define SMC_outl(v, a, r)	iowrite32(v, (a) + (r))
 #define SMC_insw(a, r, p, l)	ioread16_rep((a) + (r), p, l)
 #define SMC_outsw(a, r, p, l)	iowrite16_rep((a) + (r), p, l)
@@ -262,6 +273,8 @@ struct smc_local {
 
 	/* the low address lines on some platforms aren't connected... */
 	int	io_shift;
+	/* on some platforms a u16 write must be 4-bytes aligned */
+	bool	half_word_align4;
 
 	struct smc91x_platdata cfg;
 };
@@ -420,12 +433,13 @@ smc_pxa_dma_insw(void __iomem *ioaddr, struct smc_local *lp, int reg, int dma,
  * Any 16-bit access is performed with two 8-bit accesses if the hardware
  * can't do it directly. Most registers are 16-bit so those are mandatory.
  */
-#define SMC_outw(x, ioaddr, reg)					\
+#define _SMC_outw(x, ioaddr, reg)					\
 	do {								\
 		unsigned int __val16 = (x);				\
 		SMC_outb( __val16, ioaddr, reg );			\
 		SMC_outb( __val16 >> 8, ioaddr, reg + (1 << SMC_IO_SHIFT));\
 	} while (0)
+#define SMC_outw(lp, v, a, r)	_SMC_outw((v), (a), (r))
 #define SMC_inw(ioaddr, reg)						\
 	({								\
 		unsigned int __val16;					\
@@ -882,7 +896,7 @@ static const char * chip_ids[ 16 ] =  {
 		else if (SMC_8BIT(lp))				\
 			SMC_outb(x, ioaddr, PN_REG(lp));		\
 		else							\
-			SMC_outw(x, ioaddr, PN_REG(lp));		\
+			SMC_outw(lp, x, ioaddr, PN_REG(lp));		\
 	} while (0)
 
 #define SMC_GET_AR(lp)						\
@@ -910,7 +924,7 @@ static const char * chip_ids[ 16 ] =  {
 			int __mask;					\
 			local_irq_save(__flags);			\
 			__mask = SMC_inw(ioaddr, INT_REG(lp)) & ~0xff; \
-			SMC_outw(__mask | (x), ioaddr, INT_REG(lp));	\
+			SMC_outw(lp, __mask | (x), ioaddr, INT_REG(lp)); \
 			local_irq_restore(__flags);			\
 		}							\
 	} while (0)
@@ -924,7 +938,7 @@ static const char * chip_ids[ 16 ] =  {
 		if (SMC_8BIT(lp))					\
 			SMC_outb(x, ioaddr, IM_REG(lp));		\
 		else							\
-			SMC_outw((x) << 8, ioaddr, INT_REG(lp));	\
+			SMC_outw(lp, (x) << 8, ioaddr, INT_REG(lp));	\
 	} while (0)
 
 #define SMC_CURRENT_BANK(lp)	SMC_inw(ioaddr, BANK_SELECT)
@@ -934,22 +948,22 @@ static const char * chip_ids[ 16 ] =  {
 		if (SMC_MUST_ALIGN_WRITE(lp))				\
 			SMC_outl((x)<<16, ioaddr, 12<<SMC_IO_SHIFT);	\
 		else							\
-			SMC_outw(x, ioaddr, BANK_SELECT);		\
+			SMC_outw(lp, x, ioaddr, BANK_SELECT);		\
 	} while (0)
 
 #define SMC_GET_BASE(lp)		SMC_inw(ioaddr, BASE_REG(lp))
 
-#define SMC_SET_BASE(lp, x)		SMC_outw(x, ioaddr, BASE_REG(lp))
+#define SMC_SET_BASE(lp, x)		SMC_outw(lp, x, ioaddr, BASE_REG(lp))
 
 #define SMC_GET_CONFIG(lp)	SMC_inw(ioaddr, CONFIG_REG(lp))
 
-#define SMC_SET_CONFIG(lp, x)	SMC_outw(x, ioaddr, CONFIG_REG(lp))
+#define SMC_SET_CONFIG(lp, x)	SMC_outw(lp, x, ioaddr, CONFIG_REG(lp))
 
 #define SMC_GET_COUNTER(lp)	SMC_inw(ioaddr, COUNTER_REG(lp))
 
 #define SMC_GET_CTL(lp)		SMC_inw(ioaddr, CTL_REG(lp))
 
-#define SMC_SET_CTL(lp, x)		SMC_outw(x, ioaddr, CTL_REG(lp))
+#define SMC_SET_CTL(lp, x)		SMC_outw(lp, x, ioaddr, CTL_REG(lp))
 
 #define SMC_GET_MII(lp)		SMC_inw(ioaddr, MII_REG(lp))
 
@@ -960,18 +974,18 @@ static const char * chip_ids[ 16 ] =  {
 		if (SMC_MUST_ALIGN_WRITE(lp))				\
 			SMC_outl((x)<<16, ioaddr, SMC_REG(lp, 8, 1));	\
 		else							\
-			SMC_outw(x, ioaddr, GP_REG(lp));		\
+			SMC_outw(lp, x, ioaddr, GP_REG(lp));		\
 	} while (0)
 
-#define SMC_SET_MII(lp, x)		SMC_outw(x, ioaddr, MII_REG(lp))
+#define SMC_SET_MII(lp, x)		SMC_outw(lp, x, ioaddr, MII_REG(lp))
 
 #define SMC_GET_MIR(lp)		SMC_inw(ioaddr, MIR_REG(lp))
 
-#define SMC_SET_MIR(lp, x)		SMC_outw(x, ioaddr, MIR_REG(lp))
+#define SMC_SET_MIR(lp, x)		SMC_outw(lp, x, ioaddr, MIR_REG(lp))
 
 #define SMC_GET_MMU_CMD(lp)	SMC_inw(ioaddr, MMU_CMD_REG(lp))
 
-#define SMC_SET_MMU_CMD(lp, x)	SMC_outw(x, ioaddr, MMU_CMD_REG(lp))
+#define SMC_SET_MMU_CMD(lp, x)	SMC_outw(lp, x, ioaddr, MMU_CMD_REG(lp))
 
 #define SMC_GET_FIFO(lp)		SMC_inw(ioaddr, FIFO_REG(lp))
 
@@ -982,14 +996,14 @@ static const char * chip_ids[ 16 ] =  {
 		if (SMC_MUST_ALIGN_WRITE(lp))				\
 			SMC_outl((x)<<16, ioaddr, SMC_REG(lp, 4, 2));	\
 		else							\
-			SMC_outw(x, ioaddr, PTR_REG(lp));		\
+			SMC_outw(lp, x, ioaddr, PTR_REG(lp));		\
 	} while (0)
 
 #define SMC_GET_EPH_STATUS(lp)	SMC_inw(ioaddr, EPH_STATUS_REG(lp))
 
 #define SMC_GET_RCR(lp)		SMC_inw(ioaddr, RCR_REG(lp))
 
-#define SMC_SET_RCR(lp, x)		SMC_outw(x, ioaddr, RCR_REG(lp))
+#define SMC_SET_RCR(lp, x)		SMC_outw(lp, x, ioaddr, RCR_REG(lp))
 
 #define SMC_GET_REV(lp)		SMC_inw(ioaddr, REV_REG(lp))
 
@@ -1000,12 +1014,12 @@ static const char * chip_ids[ 16 ] =  {
 		if (SMC_MUST_ALIGN_WRITE(lp))				\
 			SMC_outl((x)<<16, ioaddr, SMC_REG(lp, 8, 0));	\
 		else							\
-			SMC_outw(x, ioaddr, RPC_REG(lp));		\
+			SMC_outw(lp, x, ioaddr, RPC_REG(lp));		\
 	} while (0)
 
 #define SMC_GET_TCR(lp)		SMC_inw(ioaddr, TCR_REG(lp))
 
-#define SMC_SET_TCR(lp, x)		SMC_outw(x, ioaddr, TCR_REG(lp))
+#define SMC_SET_TCR(lp, x)		SMC_outw(lp, x, ioaddr, TCR_REG(lp))
 
 #ifndef SMC_GET_MAC_ADDR
 #define SMC_GET_MAC_ADDR(lp, addr)					\
@@ -1022,18 +1036,18 @@ static const char * chip_ids[ 16 ] =  {
 
 #define SMC_SET_MAC_ADDR(lp, addr)					\
 	do {								\
-		SMC_outw(addr[0]|(addr[1] << 8), ioaddr, ADDR0_REG(lp)); \
-		SMC_outw(addr[2]|(addr[3] << 8), ioaddr, ADDR1_REG(lp)); \
-		SMC_outw(addr[4]|(addr[5] << 8), ioaddr, ADDR2_REG(lp)); \
+		SMC_outw(lp, addr[0]|(addr[1] << 8), ioaddr, ADDR0_REG(lp)); \
+		SMC_outw(lp, addr[2]|(addr[3] << 8), ioaddr, ADDR1_REG(lp)); \
+		SMC_outw(lp, addr[4]|(addr[5] << 8), ioaddr, ADDR2_REG(lp)); \
 	} while (0)
 
 #define SMC_SET_MCAST(lp, x)						\
 	do {								\
 		const unsigned char *mt = (x);				\
-		SMC_outw(mt[0] | (mt[1] << 8), ioaddr, MCAST_REG1(lp)); \
-		SMC_outw(mt[2] | (mt[3] << 8), ioaddr, MCAST_REG2(lp)); \
-		SMC_outw(mt[4] | (mt[5] << 8), ioaddr, MCAST_REG3(lp)); \
-		SMC_outw(mt[6] | (mt[7] << 8), ioaddr, MCAST_REG4(lp)); \
+		SMC_outw(lp, mt[0] | (mt[1] << 8), ioaddr, MCAST_REG1(lp)); \
+		SMC_outw(lp, mt[2] | (mt[3] << 8), ioaddr, MCAST_REG2(lp)); \
+		SMC_outw(lp, mt[4] | (mt[5] << 8), ioaddr, MCAST_REG3(lp)); \
+		SMC_outw(lp, mt[6] | (mt[7] << 8), ioaddr, MCAST_REG4(lp)); \
 	} while (0)
 
 #define SMC_PUT_PKT_HDR(lp, status, length)				\
@@ -1042,8 +1056,8 @@ static const char * chip_ids[ 16 ] =  {
 			SMC_outl((status) | (length)<<16, ioaddr,	\
 				 DATA_REG(lp));			\
 		else {							\
-			SMC_outw(status, ioaddr, DATA_REG(lp));	\
-			SMC_outw(length, ioaddr, DATA_REG(lp));	\
+			SMC_outw(lp, status, ioaddr, DATA_REG(lp));	\
+			SMC_outw(lp, length, ioaddr, DATA_REG(lp));	\
 		}							\
 	} while (0)
 
-- 
2.1.4

[PATCH 2/3] net: smc91x: take into account half-word workaround

[Robert Jarzmik]

For device-tree builds, platforms such as mainstone, idp and stargate2
must have their u16 writes all aligned on 32 bit boundaries. This is
already enabled in platform data builds, and this patch adds it to
device-tree builds.

Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
---
 drivers/net/ethernet/smsc/smc91x.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smc91x.c b/drivers/net/ethernet/smsc/smc91x.c
index b1f74e06d98e..b081d5ca88ce 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -2323,6 +2323,8 @@ static int smc_drv_probe(struct platform_device *pdev)
 		if (!device_property_read_u32(&pdev->dev, "reg-shift",
 					      &val))
 			lp->io_shift = val;
+		lp->half_word_align4 =
+			device_property_read_bool(&pdev->dev, "reg-u16-align4");
 	}
 #endif
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Robert Jarzmik]

Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
which must be aligned on 32 bits addresses.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
index 3fed3c124411..224965b7453c 100644
--- a/Documentation/devicetree/bindings/net/smsc911x.txt
+++ b/Documentation/devicetree/bindings/net/smsc911x.txt
@@ -13,6 +13,8 @@ Optional properties:
 - reg-io-width : Specify the size (in bytes) of the IO accesses that
   should be performed on the device.  Valid value for SMSC LAN is
   2 or 4.  If it's omitted or invalid, the size would be 2.
+- reg-u16-align4 : Boolean, put in place the workaround the force all
+  		   u16 writes to be 32 bits aligned
 - smsc,irq-active-high : Indicates the IRQ polarity is active-high
 - smsc,irq-push-pull : Indicates the IRQ type is push-pull
 - smsc,force-internal-phy : Forces SMSC LAN controller to use
-- 
2.1.4

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Jeremy Linton]

Hi Robert,

On 10/03/2016 04:05 AM, Robert Jarzmik wrote:
> Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
> which must be aligned on 32 bits addresses.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
>  1 file changed, 2 insertions(+)

I think this might be the wrong doc file. I think you want the 
smsc-lan91c111.txt file.


Thanks,

>
> diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
> index 3fed3c124411..224965b7453c 100644
> --- a/Documentation/devicetree/bindings/net/smsc911x.txt
> +++ b/Documentation/devicetree/bindings/net/smsc911x.txt
> @@ -13,6 +13,8 @@ Optional properties:
>  - reg-io-width : Specify the size (in bytes) of the IO accesses that
>    should be performed on the device.  Valid value for SMSC LAN is
>    2 or 4.  If it's omitted or invalid, the size would be 2.
> +- reg-u16-align4 : Boolean, put in place the workaround the force all
> +  		   u16 writes to be 32 bits aligned
>  - smsc,irq-active-high : Indicates the IRQ polarity is active-high
>  - smsc,irq-push-pull : Indicates the IRQ type is push-pull
>  - smsc,force-internal-phy : Forces SMSC LAN controller to use
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Robert Jarzmik]

Jeremy Linton <jeremy.linton-5wv7dgnIgG8@public.gmane.org> writes:

> Hi Robert,
>
> On 10/03/2016 04:05 AM, Robert Jarzmik wrote:
>> Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
>> which must be aligned on 32 bits addresses.
>>
>> Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>
> I think this might be the wrong doc file. I think you want the
> smsc-lan91c111.txt file.
Ah yes, thanks for spoting that.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Mark Rutland]

On Mon, Oct 03, 2016 at 11:05:53AM +0200, Robert Jarzmik wrote:
> Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
> which must be aligned on 32 bits addresses.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
> index 3fed3c124411..224965b7453c 100644
> --- a/Documentation/devicetree/bindings/net/smsc911x.txt
> +++ b/Documentation/devicetree/bindings/net/smsc911x.txt
> @@ -13,6 +13,8 @@ Optional properties:
>  - reg-io-width : Specify the size (in bytes) of the IO accesses that
>    should be performed on the device.  Valid value for SMSC LAN is
>    2 or 4.  If it's omitted or invalid, the size would be 2.
> +- reg-u16-align4 : Boolean, put in place the workaround the force all
> +  		   u16 writes to be 32 bits aligned

This property name and description is confusing.

How exactly does this differ from having reg-io-width = <4>, which is
documented immediately above?

Thanks,
Mark.

>  - smsc,irq-active-high : Indicates the IRQ polarity is active-high
>  - smsc,irq-push-pull : Indicates the IRQ type is push-pull
>  - smsc,force-internal-phy : Forces SMSC LAN controller to use
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Russell King - ARM Linux]

On Mon, Oct 03, 2016 at 04:46:25PM +0100, Mark Rutland wrote:
> On Mon, Oct 03, 2016 at 11:05:53AM +0200, Robert Jarzmik wrote:
> > Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
> > which must be aligned on 32 bits addresses.
> > 
> > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> > ---
> >  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
> > index 3fed3c124411..224965b7453c 100644
> > --- a/Documentation/devicetree/bindings/net/smsc911x.txt
> > +++ b/Documentation/devicetree/bindings/net/smsc911x.txt
> > @@ -13,6 +13,8 @@ Optional properties:
> >  - reg-io-width : Specify the size (in bytes) of the IO accesses that
> >    should be performed on the device.  Valid value for SMSC LAN is
> >    2 or 4.  If it's omitted or invalid, the size would be 2.
> > +- reg-u16-align4 : Boolean, put in place the workaround the force all
> > +  		   u16 writes to be 32 bits aligned
> 
> This property name and description is confusing.
> 
> How exactly does this differ from having reg-io-width = <4>, which is
> documented immediately above?

Please note that the binding doc for smsc,lan91c111.txt is slightly wrong
on two counts:

1) compatible property:

compatible = "smsc,lan91c111";

vs the code:

static const struct of_device_id smc91x_match[] = {
        { .compatible = "smsc,lan91c94", },
        { .compatible = "smsc,lan91c111", },
        {},
};
MODULE_DEVICE_TABLE(of, smc91x_match);

So the binding document needs to mention that smsc,lan91c94 is a valid
compatible for this device.

2) reg-io-width property:

- reg-io-width : Mask of sizes (in bytes) of the IO accesses that
  are supported on the device.  Valid value for SMSC LAN91c111 are
  1, 2 or 4.  If it's omitted or invalid, the size would be 2 meaning
  16-bit access only.

The SMC requires at least one of byte or 16-bit access sizes, with
32-bit access sizes being optional on top.  So, the legal values here
are: 1, 2, 3, 5, 6, and 7.  4 is illegal, and has never been supported
by the driver.

Note that the driver will always use byte accesses if '1' is specified
and emulate 16-bit accesses.  If '2' is specified, the driver will
always use 16-bit accesses, and emulate byte accesses for the 8-bit
registers using a read-modify-write scheme.  If '3' is specified, the
driver will use both 16-bit and byte accesses as appropriate for the
register being accessed with no emulation.  Byte or 16-bit access are
required for non-data register access.

Including 32-bit accesses on top of this allows the packet transfer
(iow, data register accesses) to use 32-bit access instructions, which
is a performance boost.

Moreover, look at the property name vs the binding description.  It's
property name says it's a width, but the description says it's a mask
of sizes - these really aren't the same thing.  Once you start
specifying these other legal masks, it makes a nonsense of the "width"
part of the name.  It's too late to try and fix this now though.

The binding document really needs to get fixed - I'll try to cook up a
patch during this week to correct these points, but it probably needs
coordination if others are going to be changing this as well.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Mark Rutland]

On Mon, Oct 03, 2016 at 05:09:13PM +0100, Russell King - ARM Linux wrote:
> Please note that the binding doc for smsc,lan91c111.txt is slightly wrong
> on two counts:
> 
> 1) compatible property:
> 
> compatible = "smsc,lan91c111";
> 
> vs the code:
> 
> static const struct of_device_id smc91x_match[] = {
>         { .compatible = "smsc,lan91c94", },
>         { .compatible = "smsc,lan91c111", },
>         {},
> };
> MODULE_DEVICE_TABLE(of, smc91x_match);
> 
> So the binding document needs to mention that smsc,lan91c94 is a valid
> compatible for this device.

Yes, it should.

> 2) reg-io-width property:
> 
> - reg-io-width : Mask of sizes (in bytes) of the IO accesses that
>   are supported on the device.  Valid value for SMSC LAN91c111 are
>   1, 2 or 4.  If it's omitted or invalid, the size would be 2 meaning
>   16-bit access only.
 
> Moreover, look at the property name vs the binding description.  It's
> property name says it's a width, but the description says it's a mask
> of sizes - these really aren't the same thing.  Once you start
> specifying these other legal masks, it makes a nonsense of the "width"
> part of the name.  It's too late to try and fix this now though.

Indeed, as-is this is nonsense. :(

The best we can do here is to add a big fat notice regarding the
misnaming; adding a new property is only giong to cause more confusion.

> The binding document really needs to get fixed - I'll try to cook up a
> patch during this week to correct these points, but it probably needs
> coordination if others are going to be changing this as well.

Thanks for handling both of these.

Given the historical rate of change of the binding document, I suspect
the stuff for pxa platforms is going to be the only potential conflict.

Either all of that can go via the DT tree (independent of any new code),
or we can ack the whole lot and it can all go via the net tree in one
go.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Rob Herring]

On Mon, Oct 03, 2016 at 05:42:29PM +0100, Mark Rutland wrote:
> On Mon, Oct 03, 2016 at 05:09:13PM +0100, Russell King - ARM Linux wrote:
> > Please note that the binding doc for smsc,lan91c111.txt is slightly wrong
> > on two counts:
> > 
> > 1) compatible property:
> > 
> > compatible = "smsc,lan91c111";
> > 
> > vs the code:
> > 
> > static const struct of_device_id smc91x_match[] = {
> >         { .compatible = "smsc,lan91c94", },
> >         { .compatible = "smsc,lan91c111", },
> >         {},
> > };
> > MODULE_DEVICE_TABLE(of, smc91x_match);
> > 
> > So the binding document needs to mention that smsc,lan91c94 is a valid
> > compatible for this device.
> 
> Yes, it should.
> 
> > 2) reg-io-width property:
> > 
> > - reg-io-width : Mask of sizes (in bytes) of the IO accesses that
> >   are supported on the device.  Valid value for SMSC LAN91c111 are
> >   1, 2 or 4.  If it's omitted or invalid, the size would be 2 meaning
> >   16-bit access only.
>  
> > Moreover, look at the property name vs the binding description.  It's
> > property name says it's a width, but the description says it's a mask
> > of sizes - these really aren't the same thing.  Once you start
> > specifying these other legal masks, it makes a nonsense of the "width"
> > part of the name.  It's too late to try and fix this now though.
> 
> Indeed, as-is this is nonsense. :(
> 
> The best we can do here is to add a big fat notice regarding the
> misnaming; adding a new property is only giong to cause more confusion.

Just fix the text here removing the mask part. This is a common property 
and not a mask. The rest saying 1, 2, 4 being valid is correct. There 
are no occurences using this as a mask in kernel dts files either.

> 
> > The binding document really needs to get fixed - I'll try to cook up a
> > patch during this week to correct these points, but it probably needs
> > coordination if others are going to be changing this as well.
> 
> Thanks for handling both of these.
> 
> Given the historical rate of change of the binding document, I suspect
> the stuff for pxa platforms is going to be the only potential conflict.
> 
> Either all of that can go via the DT tree (independent of any new code),
> or we can ack the whole lot and it can all go via the net tree in one
> go.
> 
> Thanks,
> Mark.

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Robert Jarzmik]

Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:

> On Mon, Oct 03, 2016 at 11:05:53AM +0200, Robert Jarzmik wrote:
>> Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
>> which must be aligned on 32 bits addresses.
>> 
>> Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
>> index 3fed3c124411..224965b7453c 100644
>> --- a/Documentation/devicetree/bindings/net/smsc911x.txt
>> +++ b/Documentation/devicetree/bindings/net/smsc911x.txt
>> @@ -13,6 +13,8 @@ Optional properties:
>>  - reg-io-width : Specify the size (in bytes) of the IO accesses that
>>    should be performed on the device.  Valid value for SMSC LAN is
>>    2 or 4.  If it's omitted or invalid, the size would be 2.
>> +- reg-u16-align4 : Boolean, put in place the workaround the force all
>> +  		   u16 writes to be 32 bits aligned
>
> This property name and description is confusing.
>
> How exactly does this differ from having reg-io-width = <4>, which is
> documented immediately above?

reg-io-width specifies the IO size, ie. how many data lines are physically
connected from the system bus to the lan adapter.

reg-u16-align4 tells that a specific hardware doesn't support 16 bit writes not
being 32 bits aligned, or said differently that a "store" 16 bits wide on an
address of the format 4*n + 2 deserves a special handling in the driver, while a
store 16 bits wide on an address of the format 4*n can follow the simple casual
case.

I'm pretty open to any name you might suggest, these 3 hardwares I know of are
really crazy, you can see them in patch 1/3, in the _SMC_outw_align4() function
...

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Mark Rutland]

On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
> Mark Rutland <mark.rutland@arm.com> writes:
> 
> > On Mon, Oct 03, 2016 at 11:05:53AM +0200, Robert Jarzmik wrote:
> >> Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
> >> which must be aligned on 32 bits addresses.
> >> 
> >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> >> ---
> >>  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
> >> index 3fed3c124411..224965b7453c 100644
> >> --- a/Documentation/devicetree/bindings/net/smsc911x.txt
> >> +++ b/Documentation/devicetree/bindings/net/smsc911x.txt
> >> @@ -13,6 +13,8 @@ Optional properties:
> >>  - reg-io-width : Specify the size (in bytes) of the IO accesses that
> >>    should be performed on the device.  Valid value for SMSC LAN is
> >>    2 or 4.  If it's omitted or invalid, the size would be 2.
> >> +- reg-u16-align4 : Boolean, put in place the workaround the force all
> >> +  		   u16 writes to be 32 bits aligned
> >
> > This property name and description is confusing.
> >
> > How exactly does this differ from having reg-io-width = <4>, which is
> > documented immediately above?
> 
> reg-io-width specifies the IO size, ie. how many data lines are physically
> connected from the system bus to the lan adapter.
> 
> reg-u16-align4 tells that a specific hardware doesn't support 16 bit writes not
> being 32 bits aligned, or said differently that a "store" 16 bits wide on an
> address of the format 4*n + 2 deserves a special handling in the driver, while a
> store 16 bits wide on an address of the format 4*n can follow the simple casual
> case.

If I've understood correctly, effectively the low 2 address lines to the
device are hard-wired to zero, e.g. a 16-bit access to 4*n + 2 would go
to 4*n + 0 on the device? Or is the failure case distinct from that?

Do we have other platforms where similar is true? e.g. u8 accesses
requiring 16-bit alignment?

Thanks,
Mark.

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Robert Jarzmik]

Mark Rutland <mark.rutland@arm.com> writes:

> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
>> Mark Rutland <mark.rutland@arm.com> writes:
>> 
>> reg-u16-align4 tells that a specific hardware doesn't support 16 bit writes not
>> being 32 bits aligned, or said differently that a "store" 16 bits wide on an
>> address of the format 4*n + 2 deserves a special handling in the driver, while a
>> store 16 bits wide on an address of the format 4*n can follow the simple casual
>> case.
>
> If I've understood correctly, effectively the low 2 address lines to the
> device are hard-wired to zero, e.g. a 16-bit access to 4*n + 2 would go
> to 4*n + 0 on the device? Or is the failure case distinct from that?
It is distinct.

The "awful truth" is that an FPGA lies between the system bus and the
smc91c111. And this FPGA cannot handle correctly the 4*n + 2 u16 writes.

> Do we have other platforms where similar is true? e.g. u8 accesses
> requiring 16-bit alignment?

Not really, ie. not with a alignement requirement.

But there are of course these ones are handled by reg-io-width and the
SMC_USE_xxx_BITS flags as far as I understand it. These cases are when a
platform declares SMC91X_USE_16BIT or SMC91X_USE_32BIT, but not SMC91X_USE_8BIT,
which would make me think of :
 - CONFIG_SH_SH4202_MICRODEV,
 - CONFIG_M32R
 - several omap1 boards
 - 1 sa1100 board
 - several MMP and realview boards

With all these platforms, each u8 access is replaced with a u16 access and a
mask / shift + mask.

Cheers.

-- 
Robert

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Robert Jarzmik]

Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Mark Rutland <mark.rutland@arm.com> writes:
>
>> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
>>> Mark Rutland <mark.rutland@arm.com> writes:
>>> 
>>> reg-u16-align4 tells that a specific hardware doesn't support 16 bit writes not
>>> being 32 bits aligned, or said differently that a "store" 16 bits wide on an
>>> address of the format 4*n + 2 deserves a special handling in the driver, while a
>>> store 16 bits wide on an address of the format 4*n can follow the simple casual
>>> case.
>>
>> If I've understood correctly, effectively the low 2 address lines to the
>> device are hard-wired to zero, e.g. a 16-bit access to 4*n + 2 would go
>> to 4*n + 0 on the device? Or is the failure case distinct from that?
> It is distinct.
>
> The "awful truth" is that an FPGA lies between the system bus and the
> smc91c111. And this FPGA cannot handle correctly the 4*n + 2 u16 writes.
>
>> Do we have other platforms where similar is true? e.g. u8 accesses
>> requiring 16-bit alignment?
>
> Not really, ie. not with a alignement requirement.
>
> But there are of course these ones are handled by reg-io-width and the
> SMC_USE_xxx_BITS flags as far as I understand it. These cases are when a
> platform declares SMC91X_USE_16BIT or SMC91X_USE_32BIT, but not SMC91X_USE_8BIT,
> which would make me think of :
>  - CONFIG_SH_SH4202_MICRODEV,
>  - CONFIG_M32R
>  - several omap1 boards
>  - 1 sa1100 board
>  - several MMP and realview boards
>
> With all these platforms, each u8 access is replaced with a u16 access and a
> mask / shift + mask.

Or so what should I call this entry if reg-u16-align4 is not a good candidate ?

Cheers.

-- 
Robert

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Rob Herring]

On Thu, Oct 06, 2016 at 08:47:13AM +0200, Robert Jarzmik wrote:
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
> 
> > Mark Rutland <mark.rutland@arm.com> writes:
> >
> >> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
> >>> Mark Rutland <mark.rutland@arm.com> writes:
> >>> 
> >>> reg-u16-align4 tells that a specific hardware doesn't support 16 bit writes not
> >>> being 32 bits aligned, or said differently that a "store" 16 bits wide on an
> >>> address of the format 4*n + 2 deserves a special handling in the driver, while a
> >>> store 16 bits wide on an address of the format 4*n can follow the simple casual
> >>> case.
> >>
> >> If I've understood correctly, effectively the low 2 address lines to the
> >> device are hard-wired to zero, e.g. a 16-bit access to 4*n + 2 would go
> >> to 4*n + 0 on the device? Or is the failure case distinct from that?
> > It is distinct.
> >
> > The "awful truth" is that an FPGA lies between the system bus and the
> > smc91c111. And this FPGA cannot handle correctly the 4*n + 2 u16 writes.
> >
> >> Do we have other platforms where similar is true? e.g. u8 accesses
> >> requiring 16-bit alignment?
> >
> > Not really, ie. not with a alignement requirement.
> >
> > But there are of course these ones are handled by reg-io-width and the
> > SMC_USE_xxx_BITS flags as far as I understand it. These cases are when a
> > platform declares SMC91X_USE_16BIT or SMC91X_USE_32BIT, but not SMC91X_USE_8BIT,
> > which would make me think of :
> >  - CONFIG_SH_SH4202_MICRODEV,
> >  - CONFIG_M32R
> >  - several omap1 boards
> >  - 1 sa1100 board
> >  - several MMP and realview boards
> >
> > With all these platforms, each u8 access is replaced with a u16 access and a
> > mask / shift + mask.
> 
> Or so what should I call this entry if reg-u16-align4 is not a good candidate ?

This seems broken in a very platform specific way, so perhaps something 
named based on the platform.

Rob