Re: [RFC v2 PATCH] m68knommu: added dm9000 support
From: Angelo Dureghello
Date: Thu Jan 06 2011 - 07:39:35 EST
thanks for the review,
- sure, i agree the sw byte swap could be related to a BIG_ENDIAN config option only.
- about HW, can be replaced with something like "dm9000 to cpu bus wiring", i will be more clear in my next patch version.
- i will remove blank lines
- readsX and writesX was completely missing for m68knommu, i have seen that the most appropriate file where to create them was probably "linux/arch/m68k/include/asm/io_no.h" but any comment is appreciated.
- i am adding m68k guys list from now.
thanks
angelo
On 06/01/2011 00:51, Arnaud Lacombe wrote:
> Hi,
>
> [disclamer: I have no power whatsoever on the dm9k driver, and the
> only dm9000 chip I've access to belongs to hardware whose parent
> company is now defunct.]
>
> Btw, Linux 68k folks is missing from the CC list.
>
> On Wed, Jan 5, 2011 at 1:03 PM, Angelo Dureghello <angelo70@xxxxxxxxx> wrote:
>> This patch allows to use the dm9000 network chip with a m68knommu
>> big-endian cpu. From the HW point of view,
> what hardware ?
>
>> the cpu data bus connected to
>> the dm9000 chip should be hardware-byte-swapped, crossing the bytes
>> wires (D0:7 to D24:31, etc.). In anyway, has been also added an option
>> to swap the bytes in the driver, if some cpu has been wired straight
>> D0:D31 to dm9000.
>>
>> Signed-off-by: Angelo Dureghello <angelo70@xxxxxxxxx>
>> ---
>>
>> --- linux/drivers/net/Kconfig.orig 2011-01-05 17:11:37.992376124 +0100
>> +++ linux/drivers/net/Kconfig 2011-01-04 22:33:14.132301872 +0100
>> @@ -960,7 +960,7 @@ config TI_DAVINCI_EMAC
>>
>> config DM9000
>> tristate "DM9000 support"
>> - depends on ARM || BLACKFIN || MIPS
>> + depends on COLDFIRE || ARM || BLACKFIN || MIPS
>> select CRC32
>> select MII
>> ---help---
>> @@ -986,6 +986,14 @@ config DM9000_FORCE_SIMPLE_PHY_POLL
>> costly MII PHY reads. Note, this will not work if the chip is
>> operating with an external PHY.
>>
>> +config DM9000_32BIT_SW_SWAP
>> + bool "Software byte swap for 32 bit data bus"
>> + depends on DM9000 && COLDFIRE
>> + ---help---
>> + This configuration allows to swap data bytes from the dm9000
>> + driver itself, when the big endian cpu is wired straight to
>> + the dm9000 32 bit data bus.
>> +
> I'm not sure COLDFIRE is the best dependency. AFAICS, it should
> depends on endianness, and potentially board specific settings.
>
>> config ENC28J60
>> tristate "ENC28J60 support"
>> depends on EXPERIMENTAL && SPI && NET_ETHERNET
>> @@ -3347,4 +3355,3 @@ config VMXNET3
>> module will be called vmxnet3.
>>
>> endif # NETDEVICES
>> -
>>
> useless whitespace change ...
>
>> --- linux/drivers/net/dm9000.c.orig 2010-12-30 23:19:39.747836070 +0100
>> +++ linux/drivers/net/dm9000.c 2011-01-05 16:30:48.636116500 +0100
>> [...]
>> @@ -981,7 +1032,11 @@ dm9000_rx(struct net_device *dev)
>> ior(db, DM9000_MRCMDX); /* Dummy read */
>>
>> /* Get most updated data */
>> - rxbyte = readb(db->io_data);
>> +#ifdef CONFIG_DM9000_32BIT_SW_SWAP
>> + rxbyte = (u8)readl(db->io_data);
>> +#else
>> + rxbyte = readb(db->io_data);
>> +#endif
>>
>> /* Status check: this byte must be 0 or 1 */
>> if (rxbyte & DM9000_PKT_ERR) {
>> @@ -996,8 +1051,13 @@ dm9000_rx(struct net_device *dev)
>>
>> /* A packet ready now & Get status/length */
>> GoodPacket = true;
>> - writeb(DM9000_MRCMD, db->io_addr);
>>
>> +#ifdef CONFIG_DM9000_32BIT_SW_SWAP
>> + writel(DM9000_MRCMD, db->io_addr);
>> +#else
>> + writeb(DM9000_MRCMD, db->io_addr);
>> +#endif
>> +
> All these #ifdef make the driver quite horrible to read.
>
>> (db->inblk)(db->io_data, &rxhdr, sizeof(rxhdr));
>>
>> RxLen = le16_to_cpu(rxhdr.RxLen);
>> @@ -1077,7 +1137,7 @@ static irqreturn_t dm9000_interrupt(int
>> unsigned long flags;
>> u8 reg_save;
>>
>> - dm9000_dbg(db, 3, "entering %s\n", __func__);
>> + //dm9000_dbg(db, 3, "entering %s\n", __func__);
>>
> C++ comment ... Why do you comment this ?
>
>> /* Disable all interrupts */
>> iow(db, DM9000_IMR, IMR_PAR);
>> @@ -1100,7 +1164,7 @@ static irqreturn_t dm9000_interrupt(int
>> /* Received the coming packet */
>> if (int_status & ISR_PRS)
>> dm9000_rx(dev);
>> -
>> +
> whitespace ...
>
>> @@ -1233,11 +1301,15 @@ dm9000_phy_read(struct net_device *dev,
>> int ret;
>>
>> mutex_lock(&db->addr_lock);
>> -
>> +
> whitespace ...
>
>> @@ -1713,4 +1811,3 @@ MODULE_AUTHOR("Sascha Hauer, Ben Dooks")
>> MODULE_DESCRIPTION("Davicom DM9000 network driver");
>> MODULE_LICENSE("GPL");
>> MODULE_ALIAS("platform:dm9000");
>> -
> whitespace ...
>
>> --- linux/arch/m68k/include/asm/io_no.h.orig 2011-01-05 16:53:55.904905038 +0100
>> +++ linux/arch/m68k/include/asm/io_no.h 2011-01-04 23:45:08.893049554 +0100
>> @@ -47,6 +47,91 @@ static inline unsigned int _swapl(volati
>> #define writew(b,addr) (void)((*(volatile unsigned short *) (addr)) = (b))
>> #define writel(b,addr) (void)((*(volatile unsigned int *) (addr)) = (b))
>>
>> +static inline void writesb (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned char *p = (unsigned char*) data;
>> +
>> + while (count--) writeb(*p++, reg);
>> +}
>> +
>> +static inline void writesbsw (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned char *p = (unsigned char *) data;
>> +
>> + while (count--) writel((int)(*p++), reg);
>> +}
>> +
>> +static inline void writesw (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned short *p = (unsigned short*) data;
>> +
>> + while (count--) writew(*p++, reg);
>> +}
>> +
>> +static inline void writeswsw (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned short *p = (unsigned short *) data;
>> +
>> + while (count--) writel((int)(_swapw(*p++)), reg);
>> +}
>> +
>> +static inline void writesl (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned long *p = (unsigned long*) data;
>> +
>> + while (count--) writel(*p++, reg);
>> +}
>> +
>> +static inline void writeslsw (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned long *p = (unsigned long *) data;
>> +
>> + while (count--) writel((int)(_swapl(*p++)), reg);
>> +}
>> +
>> +static inline void readsb (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned char *p = (unsigned char *) data;
>> +
>> + while (count--) *p++ = readb(reg);
>> +}
>> +
>> +static inline void readsbsw (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned char *p = (unsigned char *) data;
>> +
>> + while (count--) *p++ = (unsigned char)readl(reg);
>> +}
>> +
>> +static inline void readsw (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned short *p = (unsigned short *) data;
>> +
>> + while (count--) *p++ = readb(reg);
>> +}
>> +
>> +static inline void readswsw (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned short *p = (unsigned short *) data;
>> +
>> + while (count--) *p++ = _swapw((unsigned short)readw(reg));
>> +}
>> +
>> +static inline void readsl (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned long *p = (unsigned long *) data;
>> +
>> + while (count--) *p++ = readb(reg);
>> +}
>> +
>> +static inline void readslsw (void __iomem *reg, void *data, int count)
>> +{
>> + unsigned long *p = (unsigned long *) data;
>> +
>> + while (count--) *p++ = _swapl(readl(reg));
>> +}
>> +
>> +
>> #define __raw_readb readb
>> #define __raw_readw readw
>> #define __raw_readl readl
>> @@ -180,4 +265,3 @@ extern void iounmap(void *addr);
>> #endif /* __KERNEL__ */
>>
>> #endif /* _M68KNOMMU_IO_H */
> Could not this be moved to a separate patch ? That way arch specific
> changes are separated from the network arch-indep changes.
>
> - Arnaud
>
>> -
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/