[PATCH 1/2] atmel_nand: add missing include of linux/dmaengine.h

[PATCH 1/2] atmel_nand: add missing include of linux/dmaengine.h

[Hans-Christian Egtvedt]

Including linux/dmaengine.h fixes the missing definition of the enum
dma_ctrl_flags type used in atmel_nand_dma_op function.

Signed-off-by: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>
---
 drivers/mtd/nand/atmel_nand.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 950646a..380912f 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -30,6 +30,7 @@
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 
+#include <linux/dmaengine.h>
 #include <linux/gpio.h>
 #include <linux/io.h>
 
-- 
1.7.1

[PATCH 2/2] mtd: add switch to support NAND flash on big endian bus

[Hans-Christian Egtvedt]

This patch adds a new kconfig symbol to the MTD NAND system, MTD_NAND_BE_BUS.
This symbol is used by the nand base to properly convert the data read from the
bus into the format the CPU expects.

The patch fixes 16-bit NAND flash support on big endian architectures (at least
AVR32) with NAND flash hooked up to a big endian external bus.

Signed-off-by: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>
---
 drivers/mtd/nand/Kconfig     |    8 ++++++++
 drivers/mtd/nand/nand_base.c |    8 ++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index edec457..bb3a54a 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -22,6 +22,14 @@ menuconfig MTD_NAND
 
 if MTD_NAND
 
+config MTD_NAND_BE_BUS
+	bool "NAND device on big endian bus"
+	default n
+	help
+	  This option will assume data read from the bus is in big endian
+	  format. This is vital when reading command values from the bus, as
+	  only the lower 8 bit are in use then.
+
 config MTD_NAND_VERIFY_WRITE
 	bool "Verify NAND page writes"
 	help
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index c54a4cb..bbb0c1d 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -180,7 +180,11 @@ static uint8_t nand_read_byte(struct mtd_info *mtd)
 static uint8_t nand_read_byte16(struct mtd_info *mtd)
 {
 	struct nand_chip *chip = mtd->priv;
+#if CONFIG_MTD_NAND_BE_BUS
+	return (uint8_t) be16_to_cpu(readw(chip->IO_ADDR_R));
+#else
 	return (uint8_t) cpu_to_le16(readw(chip->IO_ADDR_R));
+#endif
 }
 
 /**
@@ -364,7 +368,11 @@ static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)
 	if (chip->options & NAND_BUSWIDTH_16) {
 		chip->cmdfunc(mtd, NAND_CMD_READOOB, chip->badblockpos & 0xFE,
 			      page);
+#if CONFIG_MTD_NAND_BE_BUS
+		bad = be16_to_cpu(chip->read_word(mtd));
+#else
 		bad = cpu_to_le16(chip->read_word(mtd));
+#endif
 		if (chip->badblockpos & 0x1)
 			bad >>= 8;
 		else
-- 
1.7.1

Re: [PATCH 2/2] mtd: add switch to support NAND flash on big endian bus

[Ricard Wanderlof]

On Wed, 13 Apr 2011, Hans-Christian Egtvedt wrote:

> This patch adds a new kconfig symbol to the MTD NAND system, MTD_NAND_BE_BUS.
> This symbol is used by the nand base to properly convert the data read from the
> bus into the format the CPU expects.
>
> The patch fixes 16-bit NAND flash support on big endian architectures (at least
> AVR32) with NAND flash hooked up to a big endian external bus.
>
> Signed-off-by: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>
> ---
> drivers/mtd/nand/Kconfig     |    8 ++++++++
> drivers/mtd/nand/nand_base.c |    8 ++++++++
> 2 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index edec457..bb3a54a 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -22,6 +22,14 @@ menuconfig MTD_NAND
>
> if MTD_NAND
>
> +config MTD_NAND_BE_BUS
> +	bool "NAND device on big endian bus"
> +	default n
> +	help
> +	  This option will assume data read from the bus is in big endian
> +	  format. This is vital when reading command values from the bus, as
> +	  only the lower 8 bit are in use then.
> +
> config MTD_NAND_VERIFY_WRITE
> 	bool "Verify NAND page writes"
> 	help
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index c54a4cb..bbb0c1d 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -180,7 +180,11 @@ static uint8_t nand_read_byte(struct mtd_info *mtd)
> static uint8_t nand_read_byte16(struct mtd_info *mtd)
> {
> 	struct nand_chip *chip = mtd->priv;
> +#if CONFIG_MTD_NAND_BE_BUS
> +	return (uint8_t) be16_to_cpu(readw(chip->IO_ADDR_R));
> +#else
> 	return (uint8_t) cpu_to_le16(readw(chip->IO_ADDR_R));
> +#endif }

Wouldn't one expect a certain symmetry here, i.e. if one function is
cpu_to_le16, the other one would be called cpu_to_be16 ? I guess in most 
cases cpu_to_le16 would be the same as le16_to_cpu, etc, but it still 
looks as if someone made a mistake.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 2/2] mtd: add switch to support NAND flash on big endian bus

[Artem Bityutskiy]

On Wed, 2011-04-13 at 15:55 +0200, Hans-Christian Egtvedt wrote:
> This patch adds a new kconfig symbol to the MTD NAND system, MTD_NAND_BE_BUS.
> This symbol is used by the nand base to properly convert the data read from the
> bus into the format the CPU expects.
> 
> The patch fixes 16-bit NAND flash support on big endian architectures (at least
> AVR32) with NAND flash hooked up to a big endian external bus.
> 
> Signed-off-by: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>

These kind of config options are bad and lead to confusion. You should
try hard to auto-detect the endianness, or take this information from
the boot-loader via the device tree.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/2] atmel_nand: add missing include of linux/dmaengine.h

[Artem Bityutskiy]

On Wed, 2011-04-13 at 15:55 +0200, Hans-Christian Egtvedt wrote:
> Including linux/dmaengine.h fixes the missing definition of the enum
> dma_ctrl_flags type used in atmel_nand_dma_op function.
> 
> Signed-off-by: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>

I do not see how it is a problem. C does not support strict types
checking anyway. This all is just 'int'.

IMHO, the enums you invented only make code less readable for C
programmers.

I'd suggest to turn all you enums into anonymous enums and stop using
them as types. That would only make things more readable and easier to
follow.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/2] atmel_nand: add missing include of linux/dmaengine.h

[Artem Bityutskiy]

On Thu, 2011-04-14 at 16:39 +0300, Artem Bityutskiy wrote:
> On Wed, 2011-04-13 at 15:55 +0200, Hans-Christian Egtvedt wrote:
> > Including linux/dmaengine.h fixes the missing definition of the enum
> > dma_ctrl_flags type used in atmel_nand_dma_op function.
> > 
> > Signed-off-by: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>
> 
> I do not see how it is a problem. C does not support strict types
> checking anyway. This all is just 'int'.

Sorry, I was too fast. Pushed to l2-mtd-2.6.git.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2/2] mtd: add switch to support NAND flash on big endian bus

[Hans-Christian Egtvedt]

On Thu, 2011-04-14 at 15:35 +0300, Artem Bityutskiy wrote:
> On Wed, 2011-04-13 at 15:55 +0200, Hans-Christian Egtvedt wrote:
> > This patch adds a new kconfig symbol to the MTD NAND system, MTD_NAND_BE_BUS.
> > This symbol is used by the nand base to properly convert the data read from the
> > bus into the format the CPU expects.
> > 
> > The patch fixes 16-bit NAND flash support on big endian architectures (at least
> > AVR32) with NAND flash hooked up to a big endian external bus.
> > 
> > Signed-off-by: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>
> 
> These kind of config options are bad and lead to confusion. You should
> try hard to auto-detect the endianness, or take this information from
> the boot-loader via the device tree.

AVR32 doesn't support the device tree.

For AVR32 assuming big endian wiring on the EBI is sane, as the
architecture is big-endian only. I don't know how other big endian
architectures behaves, and I am puzzled that I am the only observer of
this...

-- 
Hans-Christian Egtvedt

Re: [PATCH 2/2] mtd: add switch to support NAND flash on big endian bus

[Hans-Christian Egtvedt]

On Wed, 2011-04-13 at 16:11 +0200, Ricard Wanderlof wrote:
> On Wed, 13 Apr 2011, Hans-Christian Egtvedt wrote:
> 
> > This patch adds a new kconfig symbol to the MTD NAND system, MTD_NAND_BE_BUS.
> > This symbol is used by the nand base to properly convert the data read from the
> > bus into the format the CPU expects.
> >
> > The patch fixes 16-bit NAND flash support on big endian architectures (at least
> > AVR32) with NAND flash hooked up to a big endian external bus.
> >
> > Signed-off-by: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>
> > ---
> > drivers/mtd/nand/Kconfig     |    8 ++++++++
> > drivers/mtd/nand/nand_base.c |    8 ++++++++
> > 2 files changed, 16 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> > index edec457..bb3a54a 100644
> > --- a/drivers/mtd/nand/Kconfig
> > +++ b/drivers/mtd/nand/Kconfig
> > @@ -22,6 +22,14 @@ menuconfig MTD_NAND
> >
> > if MTD_NAND
> >
> > +config MTD_NAND_BE_BUS
> > +	bool "NAND device on big endian bus"
> > +	default n
> > +	help
> > +	  This option will assume data read from the bus is in big endian
> > +	  format. This is vital when reading command values from the bus, as
> > +	  only the lower 8 bit are in use then.
> > +
> > config MTD_NAND_VERIFY_WRITE
> > 	bool "Verify NAND page writes"
> > 	help
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index c54a4cb..bbb0c1d 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -180,7 +180,11 @@ static uint8_t nand_read_byte(struct mtd_info *mtd)
> > static uint8_t nand_read_byte16(struct mtd_info *mtd)
> > {
> > 	struct nand_chip *chip = mtd->priv;
> > +#if CONFIG_MTD_NAND_BE_BUS
> > +	return (uint8_t) be16_to_cpu(readw(chip->IO_ADDR_R));
> > +#else
> > 	return (uint8_t) cpu_to_le16(readw(chip->IO_ADDR_R));
> > +#endif }
> 
> Wouldn't one expect a certain symmetry here, i.e. if one function is
> cpu_to_le16, the other one would be called cpu_to_be16 ? I guess in most 
> cases cpu_to_le16 would be the same as le16_to_cpu, etc, but it still 
> looks as if someone made a mistake.

My other option for this patch is to remove the endianness conversion
all together, it looks a bit weird, and AFAICT it indicates that the
external NAND device is wired in a certain way to the bus interface.

Perhaps someone with knowledge about this specific piece of code can
shed some light on this?

-- 
Hans-Christian Egtvedt