[PATCH V4 0/4] add the GPMI controller driver for IMX23/IMX28

[PATCH V4 0/4] add the GPMI controller driver for IMX23/IMX28

[Huang Shijie]

The general-purpose media interface(GPMI) controller is a flexible interface
to up to several NAND flashs.

The Bose Ray-Choudhury Hocquenghem(BCH) module is a hardware ECC accelerator.

With the help of BCH, the GPMI controller can choose to do the hardware ECC or
not.

This driver is based the Shawn Guo's DMA patches for IMX23/IMX28,
please refer to :
	http://git.infradead.org/users/vkoul/slave-dma.git/commit/a580b8c5429a624d120cd603e1498bf676e2b4da

v3 --> v4:
	[0] use the nand_ids{} as the nand database, drop my own database.
	[1] remove the patch for DMA enginer, Shawn will submit his own version.
	[2] use the platform_id to distinguish different Archs.
	[3] fix the strange coding style.
	[4] others.

v2 --> v3:
	[0] merge the imx23 and imx28 into one file(including the header file).
	[1] remove the unuse registers in the headers.
	[2] fix DMA bugs
	[3] add bus width field to nand_attr{}
	[4] others

v1 --> v2:
	[0] merge the common files into the gpmi-nfc-main.c
	[1] change the code to get the clock.
	[2] remove the timing in the nand_device_info{}
	[3] fix DMA errors
	[4] add the nand_device_info.[ch] to generic code
	[5] use the chip->onfi_version for the ONFI nand
	[6] useless init
	[7] others


Huang Shijie (4):
  ARM: add GPMI support for imx23/imx28
  MTD : add the common code for GPMI controller driver
  MTD: add support for imx23 and imx28
  MTD : add GPMI driver in the config and Makefile

 arch/arm/mach-mxs/Kconfig                       |    2 +
 arch/arm/mach-mxs/clock-mx23.c                  |    3 +
 arch/arm/mach-mxs/clock-mx28.c                  |    3 +
 arch/arm/mach-mxs/devices-mx23.h                |    3 +
 arch/arm/mach-mxs/devices-mx28.h                |    3 +
 arch/arm/mach-mxs/devices/Kconfig               |    3 +
 arch/arm/mach-mxs/devices/Makefile              |    1 +
 arch/arm/mach-mxs/devices/platform-gpmi.c       |  136 ++
 arch/arm/mach-mxs/include/mach/devices-common.h |    4 +
 arch/arm/mach-mxs/include/mach/gpmi-nfc.h       |   64 +
 arch/arm/mach-mxs/mach-mx23evk.c                |   37 +
 arch/arm/mach-mxs/mach-mx28evk.c                |   37 +
 drivers/mtd/nand/Kconfig                        |   10 +
 drivers/mtd/nand/Makefile                       |    1 +
 drivers/mtd/nand/gpmi-nfc/Makefile              |    5 +
 drivers/mtd/nand/gpmi-nfc/bch-mx23-mx28.h       |   88 +
 drivers/mtd/nand/gpmi-nfc/gpmi-mx23-mx28.h      |  163 ++
 drivers/mtd/nand/gpmi-nfc/gpmi-nfc-main.c       | 2453 +++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nfc/gpmi-nfc.h            |  551 +++++
 drivers/mtd/nand/gpmi-nfc/hal-mx23-mx28.c       |  556 +++++
 drivers/mtd/nand/gpmi-nfc/rom-mx23.c            |  298 +++
 drivers/mtd/nand/gpmi-nfc/rom-mx28.c            |   66 +
 22 files changed, 4487 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-mxs/devices/platform-gpmi.c
 create mode 100644 arch/arm/mach-mxs/include/mach/gpmi-nfc.h
 create mode 100644 drivers/mtd/nand/gpmi-nfc/Makefile
 create mode 100644 drivers/mtd/nand/gpmi-nfc/bch-mx23-mx28.h
 create mode 100644 drivers/mtd/nand/gpmi-nfc/gpmi-mx23-mx28.h
 create mode 100644 drivers/mtd/nand/gpmi-nfc/gpmi-nfc-main.c
 create mode 100644 drivers/mtd/nand/gpmi-nfc/gpmi-nfc.h
 create mode 100644 drivers/mtd/nand/gpmi-nfc/hal-mx23-mx28.c
 create mode 100644 drivers/mtd/nand/gpmi-nfc/rom-mx23.c
 create mode 100644 drivers/mtd/nand/gpmi-nfc/rom-mx28.c

[PATCH V4 0/4] add the GPMI controller driver for IMX23/IMX28

[Huang Shijie]

Hi:

Does some one have any comments about this driver?

Thanks

Huang Shijie

> The general-purpose media interface(GPMI) controller is a flexible interface
> to up to several NAND flashs.
>
> The Bose Ray-Choudhury Hocquenghem(BCH) module is a hardware ECC accelerator.
>
> With the help of BCH, the GPMI controller can choose to do the hardware ECC or
> not.
>
> This driver is based the Shawn Guo's DMA patches for IMX23/IMX28,
> please refer to :
> 	http://git.infradead.org/users/vkoul/slave-dma.git/commit/a580b8c5429a624d120cd603e1498bf676e2b4da
>
> v3 --> v4:
> 	[0] use the nand_ids{} as the nand database, drop my own database.
> 	[1] remove the patch for DMA enginer, Shawn will submit his own version.
> 	[2] use the platform_id to distinguish different Archs.
> 	[3] fix the strange coding style.
> 	[4] others.
>
> v2 --> v3:
> 	[0] merge the imx23 and imx28 into one file(including the header file).
> 	[1] remove the unuse registers in the headers.
> 	[2] fix DMA bugs
> 	[3] add bus width field to nand_attr{}
> 	[4] others
>
> v1 --> v2:
> 	[0] merge the common files into the gpmi-nfc-main.c
> 	[1] change the code to get the clock.
> 	[2] remove the timing in the nand_device_info{}
> 	[3] fix DMA errors
> 	[4] add the nand_device_info.[ch] to generic code
> 	[5] use the chip->onfi_version for the ONFI nand
> 	[6] useless init
> 	[7] others
>
>
> Huang Shijie (4):
>   ARM: add GPMI support for imx23/imx28
>   MTD : add the common code for GPMI controller driver
>   MTD: add support for imx23 and imx28
>   MTD : add GPMI driver in the config and Makefile
>
>  arch/arm/mach-mxs/Kconfig                       |    2 +
>  arch/arm/mach-mxs/clock-mx23.c                  |    3 +
>  arch/arm/mach-mxs/clock-mx28.c                  |    3 +
>  arch/arm/mach-mxs/devices-mx23.h                |    3 +
>  arch/arm/mach-mxs/devices-mx28.h                |    3 +
>  arch/arm/mach-mxs/devices/Kconfig               |    3 +
>  arch/arm/mach-mxs/devices/Makefile              |    1 +
>  arch/arm/mach-mxs/devices/platform-gpmi.c       |  136 ++
>  arch/arm/mach-mxs/include/mach/devices-common.h |    4 +
>  arch/arm/mach-mxs/include/mach/gpmi-nfc.h       |   64 +
>  arch/arm/mach-mxs/mach-mx23evk.c                |   37 +
>  arch/arm/mach-mxs/mach-mx28evk.c                |   37 +
>  drivers/mtd/nand/Kconfig                        |   10 +
>  drivers/mtd/nand/Makefile                       |    1 +
>  drivers/mtd/nand/gpmi-nfc/Makefile              |    5 +
>  drivers/mtd/nand/gpmi-nfc/bch-mx23-mx28.h       |   88 +
>  drivers/mtd/nand/gpmi-nfc/gpmi-mx23-mx28.h      |  163 ++
>  drivers/mtd/nand/gpmi-nfc/gpmi-nfc-main.c       | 2453 +++++++++++++++++++++++
>  drivers/mtd/nand/gpmi-nfc/gpmi-nfc.h            |  551 +++++
>  drivers/mtd/nand/gpmi-nfc/hal-mx23-mx28.c       |  556 +++++
>  drivers/mtd/nand/gpmi-nfc/rom-mx23.c            |  298 +++
>  drivers/mtd/nand/gpmi-nfc/rom-mx28.c            |   66 +
>  22 files changed, 4487 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-mxs/devices/platform-gpmi.c
>  create mode 100644 arch/arm/mach-mxs/include/mach/gpmi-nfc.h
>  create mode 100644 drivers/mtd/nand/gpmi-nfc/Makefile
>  create mode 100644 drivers/mtd/nand/gpmi-nfc/bch-mx23-mx28.h
>  create mode 100644 drivers/mtd/nand/gpmi-nfc/gpmi-mx23-mx28.h
>  create mode 100644 drivers/mtd/nand/gpmi-nfc/gpmi-nfc-main.c
>  create mode 100644 drivers/mtd/nand/gpmi-nfc/gpmi-nfc.h
>  create mode 100644 drivers/mtd/nand/gpmi-nfc/hal-mx23-mx28.c
>  create mode 100644 drivers/mtd/nand/gpmi-nfc/rom-mx23.c
>  create mode 100644 drivers/mtd/nand/gpmi-nfc/rom-mx28.c
>

[PATCH V4 0/4] add the GPMI controller driver for IMX23/IMX28

[Lothar Waßmann]

Hi,

Huang Shijie writes:
> Does some one have any comments about this driver?
> 
I'm still not happy with the rom-helper code that IMO does not belong
in this driver.
You could enable CONFIG_MTD_CMDLINE_PARTS and add something like:
"mtdparts=gpmi-nfc-main:2m(gpmi-nfc-0-boot)ro,-(gpmi-nfc-general-use)"
to the kernel cmdline to achieve the same without any special code
inside the chip driver.

Also, I would integrate the code from the hal-*.c files into the main
file and remove all the function hooks, since the functions are the
same for all the current variants anyway. You might hook the 'begin()'
and 'is_ready()' functions which are the only ones that are different
in the current variants so that the distinction can be made once upon
initialization rather than on every function call:
+static int mx23_is_ready(struct gpmi_nfc_data *this, unsigned chip)
+{
+	struct resources  *resources = &this->resources;
+	uint32_t          mask;
+	uint32_t          reg;
+
+	mask = MX23_BM_GPMI_DEBUG_READY0 << chip;
+	reg = __raw_readl(resources->gpmi_regs + HW_GPMI_DEBUG);
+
+	return !!(reg & mask);
+}
+
+static void mx28_begin(struct gpmi_nfc_data *this)
+{
+	struct resources                 *resources = &this->resources;
{...]
+}
+
+static int mx28_is_ready(struct gpmi_nfc_data *this, unsigned chip)
+{
+	struct resources  *resources = &this->resources;
+	uint32_t          mask;
+	uint32_t          reg;
+
+	mask = MX28_BF_GPMI_STAT_READY_BUSY(1 << chip);
+	reg = __raw_readl(resources->gpmi_regs + HW_GPMI_STAT);
+
+	return !!(reg & mask);
+}
[...]
+static int gpmi_nfc_probe(struct platform_device *pdev)
[...]
+	if (CPU_IS_MX23()) {
+		this->is_ready = mx23_is_ready;
+	} else if (CPU_IS_MX28()) {
+		this->is_ready = mx28_is_ready;
+		this->begin = mx28_begin;
+	}
Since the begin() function is a NOP for i.MX23, the function pointer
could be left unassigned and the function only be called if the
pointer is not NULL or an empty function could be assigned.

Further I wouldn't name the macro for distinguishing the different
controller variants CPU_IS_... but something like GPMI_IS


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH V4 0/4] add the GPMI controller driver for IMX23/IMX28

[Huang Shijie]

Hi Lothar:
> Hi,
>
> Huang Shijie writes:
>> Does some one have any comments about this driver?
>>
> I'm still not happy with the rom-helper code that IMO does not belong
> in this driver.
Could you tell me which part of the rom-helper code is not belong the 
driver?


> You could enable CONFIG_MTD_CMDLINE_PARTS and add something like:
> "mtdparts=gpmi-nfc-main:2m(gpmi-nfc-0-boot)ro,-(gpmi-nfc-general-use)"
> to the kernel cmdline to achieve the same without any special code
> inside the chip driver.
>
The original code does have the command-line-partition parsing code,

I removed it. I think I did a wrong thing.

I found the code is needed yesterday. The android system needs a different
partition layout. I had to revert the code back.



> Also, I would integrate the code from the hal-*.c files into the main
> file and remove all the function hooks, since the functions are the
thanks for your advice.

This driver will serve for many platforms, not only the imx23 and imx28.
I am merging the imx508 code to the driver.
I feel lucky that i did not merge all the code into the main file, which 
will
make the code mess.

But still thanks for your advice.

> same for all the current variants anyway. You might hook the 'begin()'
> and 'is_ready()' functions which are the only ones that are different
> in the current variants so that the distinction can be made once upon
> initialization rather than on every function call:
> +static int mx23_is_ready(struct gpmi_nfc_data *this, unsigned chip)
> +{
> +	struct resources  *resources =&this->resources;
> +	uint32_t          mask;
> +	uint32_t          reg;
> +
> +	mask = MX23_BM_GPMI_DEBUG_READY0<<  chip;
> +	reg = __raw_readl(resources->gpmi_regs + HW_GPMI_DEBUG);
> +
> +	return !!(reg&  mask);
> +}
> +
> +static void mx28_begin(struct gpmi_nfc_data *this)
> +{
> +	struct resources                 *resources =&this->resources;
> {...]
> +}
> +
> +static int mx28_is_ready(struct gpmi_nfc_data *this, unsigned chip)
> +{
> +	struct resources  *resources =&this->resources;
> +	uint32_t          mask;
> +	uint32_t          reg;
> +
> +	mask = MX28_BF_GPMI_STAT_READY_BUSY(1<<  chip);
> +	reg = __raw_readl(resources->gpmi_regs + HW_GPMI_STAT);
> +
> +	return !!(reg&  mask);
> +}
> [...]
> +static int gpmi_nfc_probe(struct platform_device *pdev)
> [...]
> +	if (CPU_IS_MX23()) {
> +		this->is_ready = mx23_is_ready;
> +	} else if (CPU_IS_MX28()) {
> +		this->is_ready = mx28_is_ready;
> +		this->begin = mx28_begin;
> +	}
> Since the begin() function is a NOP for i.MX23, the function pointer
> could be left unassigned and the function only be called if the
> pointer is not NULL or an empty function could be assigned.
>
> Further I wouldn't name the macro for distinguishing the different
> controller variants CPU_IS_... but something like GPMI_IS
ok. This is a small change. I will do it in the next version. thanks

Huang Shijie
>
> Lothar Wa?mann

[PATCH V4 0/4] add the GPMI controller driver for IMX23/IMX28

[Lothar Waßmann]

Hi,

Huang Shijie writes:
> Hi Lothar:
> > Hi,
> >
> > Huang Shijie writes:
> >> Does some one have any comments about this driver?
> >>
> > I'm still not happy with the rom-helper code that IMO does not belong
> > in this driver.
> Could you tell me which part of the rom-helper code is not belong the 
> driver?
> 
All of it. I don't see a point in having it, when all that it does can
be achieved with standard functions already.

> > Also, I would integrate the code from the hal-*.c files into the main
> > file and remove all the function hooks, since the functions are the
> thanks for your advice.
> 
> This driver will serve for many platforms, not only the imx23 and imx28.
> I am merging the imx508 code to the driver.
> I feel lucky that i did not merge all the code into the main file, which 
> will
> make the code mess.
> 
You would probably end up with some more functions that are
implemented for the different variants. They could be prefixed with
the SoC name and selected in the probe() function depending on the
platform id. I don't see why this should get messy.

If the imx508 driver is so much different from the other variants,
that merging it gets too messy, it's probably better to not merge it
with the other variants anyway.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH V4 0/4] add the GPMI controller driver for IMX23/IMX28

[Huang Shijie]

Hi Lothar:

> Hi,
>
> Huang Shijie writes:
>> Hi Lothar:
>>> Hi,
>>>
>>> Huang Shijie writes:
>>>> Does some one have any comments about this driver?
>>>>
>>> I'm still not happy with the rom-helper code that IMO does not belong
>>> in this driver.
>> Could you tell me which part of the rom-helper code is not belong the
>> driver?
>>
> All of it. I don't see a point in having it, when all that it does can
> be achieved with standard functions already.
>
I am afraid most of the rom-help code can not be removed, the  reasons are:
[1] imx23 does not need the swap-block-mark feature. The swap-block-mark 
feature is used
    by the BCH hardware ecc-correction module. But the imx28 does need 
the swap-block-mark feature.
    Why? the firmwares in both chips are a little different. The 
rom-help code is used to distinguish
    this, and _DO_ some necessary initializations. After the 
initialization, the NAND_SCAN will works.

[2] In the NAND boot mode, we also need to check the swap-block-mark to 
do the right ECC read pages.



BUT the default partition layout code really can be removed. It somehow 
can be regards as not belonged to
the driver.

thanks.
>>> Also, I would integrate the code from the hal-*.c files into the main
>>> file and remove all the function hooks, since the functions are the
>> thanks for your advice.
>>
>> This driver will serve for many platforms, not only the imx23 and imx28.
>> I am merging the imx508 code to the driver.
>> I feel lucky that i did not merge all the code into the main file, which
>> will
>> make the code mess.
>>
> You would probably end up with some more functions that are
> implemented for the different variants. They could be prefixed with
> the SoC name and selected in the probe() function depending on the
> platform id. I don't see why this should get messy.
>
I really do not like to package all the code into one file which makes 
the file bigger and bigger.
Frankly speaking, I regard it as a bad idea. I think it's messy.

imx508 will support DDR mode for ONFI NAND and TOGGLE nand which need to 
do lot of TIMING initialization for both mode.
It is a long initialization code. Keep the code in separate file is to 
make the code tidy. Do you think it's a
grace method to keep it in one file? I do not think so. :)

yes, I can use the PREFIXED name for different variants. But I think 
that will create more code.
The current *hal.c only contains the different implementation for variants.


> If the imx508 driver is so much different from the other variants,
> that merging it gets too messy, it's probably better to not merge it
> with the other variants anyway.
>
It just needs a separate hal-508.c file.
> Lothar Wa?mann

Thanks a lot.

Best Regards
Huang Shijie