[PATCH v2 0/7] SH Mobile sh_flctl driver brush up

[PATCH v2 0/7] SH Mobile sh_flctl driver brush up

[Bastian Hecht]

This patch series brushs up the sh_flctl.c a bit to make it work with
the mackerel board. The hardware has changed and this needed to be
reflected in the NAND host driver.
On the road some minor enhancements have been incorporated.

Bastian Hecht (7):
  mtd: sh_flctl: Expand FLCMNCR register bit field
  mtd: sh_flctl: Reorder empty_fifo() calls
  mtd: sh_flctl: Expand the READID command to 8 bytes
  mtd: sh_flctl: Implement NAND_CMD_RNDOUT command
  mtd: sh_flctl: Use cached register value for FLCMNCR
  mtd: sh_flctl: Add FLHOLDCR register
  ARM: mach-shmobile: mackerel: Add the flash controller flctl

 arch/arm/mach-shmobile/board-mackerel.c |   71 +++++++++++++++++++++++++++++++
 arch/arm/mach-shmobile/clock-sh7372.c   |    4 +-
 drivers/mtd/nand/sh_flctl.c             |   48 +++++++++++++--------
 include/linux/mtd/sh_flctl.h            |   37 ++++++++++++++++
 4 files changed, 141 insertions(+), 19 deletions(-)

-- 
1.7.5.4

[PATCH v2 1/7] mtd: sh_flctl: Expand FLCMNCR register bit field

[Bastian Hecht]

Add support for a new hardware generation. The meaning of some bits
of the FLCMNCR register changed, so some new defines are added
parallel to the existing ones to keep backward compatibility.

The defines allow to choose an appropriate clocking scheme.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
changelog: code stays the same, just the description is slightly corrected.

 include/linux/mtd/sh_flctl.h |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
index 9cf4c4c..e66c393 100644
--- a/include/linux/mtd/sh_flctl.h
+++ b/include/linux/mtd/sh_flctl.h
@@ -67,6 +67,30 @@
 #define	CE0_ENABLE	(0x1 << 3)	/* Chip Enable 0 */
 #define	TYPESEL_SET	(0x1 << 0)
 
+/*
+ * Clock settings using the PULSEx registers from FLCMNCR
+ *
+ * Some hardware uses bits called PULSEx instead of FCKSEL_E and QTSEL_E
+ * to control the clock divider used between the High-Speed Peripheral Clock
+ * and the FLCTL internal clock. If so, use CLK_8_BIT_xxx for connecting 8 bit
+ * and CLK_16_BIT_xxx for connecting 16 bit bus bandwith NAND chips. For the 16
+ * bit version the divider is seperate for the pulse width of high and low
+ * signals.
+ */
+#define PULSE3	(0x1 << 27)
+#define PULSE2	(0x1 << 17)
+#define PULSE1	(0x1 << 15)
+#define PULSE0	(0x1 << 9)
+#define CLK_8_BIT_0_5HP			PULSE1
+#define CLK_8_BIT_1HP			0x0
+#define CLK_8_BIT_1_5HP			(PULSE1 | PULSE2)
+#define CLK_8_BIT_2HP			PULSE0
+#define CLK_8_BIT_3HP			(PULSE0 | PULSE1 | PULSE2)
+#define CLK_8_BIT_4HP			(PULSE0 | PULSE2)
+#define CLK_16_BIT_6HP_LOW_2HP_HIGH	PULSE0
+#define CLK_16_BIT_9HP_LOW_3HP_HIGH	(PULSE0 | PULSE1 | PULSE2)
+#define CLK_16_BIT_12HP_LOW_4HP_HIGH	(PULSE0 | PULSE2)
+
 /* FLCMDCR control bits */
 #define ADRCNT2_E	(0x1 << 31)	/* 5byte address enable */
 #define ADRMD_E		(0x1 << 26)	/* Sector address access */
-- 
1.7.5.4

Re: [PATCH v2 1/7] mtd: sh_flctl: Expand FLCMNCR register bit field

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Saturday 11 February 2012 12:44:59 Bastian Hecht wrote:
> Add support for a new hardware generation. The meaning of some bits
> of the FLCMNCR register changed, so some new defines are added
> parallel to the existing ones to keep backward compatibility.
> 
> The defines allow to choose an appropriate clocking scheme.
> 
> Signed-off-by: Bastian Hecht <hechtb@gmail.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> changelog: code stays the same, just the description is slightly corrected.
> 
>  include/linux/mtd/sh_flctl.h |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
> index 9cf4c4c..e66c393 100644
> --- a/include/linux/mtd/sh_flctl.h
> +++ b/include/linux/mtd/sh_flctl.h
> @@ -67,6 +67,30 @@
>  #define	CE0_ENABLE	(0x1 << 3)	/* Chip Enable 0 */
>  #define	TYPESEL_SET	(0x1 << 0)
> 
> +/*
> + * Clock settings using the PULSEx registers from FLCMNCR
> + *
> + * Some hardware uses bits called PULSEx instead of FCKSEL_E and QTSEL_E
> + * to control the clock divider used between the High-Speed Peripheral
> Clock + * and the FLCTL internal clock. If so, use CLK_8_BIT_xxx for
> connecting 8 bit + * and CLK_16_BIT_xxx for connecting 16 bit bus bandwith
> NAND chips. For the 16 + * bit version the divider is seperate for the
> pulse width of high and low + * signals.
> + */
> +#define PULSE3	(0x1 << 27)
> +#define PULSE2	(0x1 << 17)
> +#define PULSE1	(0x1 << 15)
> +#define PULSE0	(0x1 << 9)
> +#define CLK_8_BIT_0_5HP			PULSE1
> +#define CLK_8_BIT_1HP			0x0
> +#define CLK_8_BIT_1_5HP			(PULSE1 | PULSE2)
> +#define CLK_8_BIT_2HP			PULSE0
> +#define CLK_8_BIT_3HP			(PULSE0 | PULSE1 | PULSE2)
> +#define CLK_8_BIT_4HP			(PULSE0 | PULSE2)
> +#define CLK_16_BIT_6HP_LOW_2HP_HIGH	PULSE0
> +#define CLK_16_BIT_9HP_LOW_3HP_HIGH	(PULSE0 | PULSE1 | PULSE2)
> +#define CLK_16_BIT_12HP_LOW_4HP_HIGH	(PULSE0 | PULSE2)
> +

This is nitpicking, but you could shorten the names to something like 
CLK_16B_12L_4P (at the expense of readability though). Feel free to ignore 
this.

>  /* FLCMDCR control bits */
>  #define ADRCNT2_E	(0x1 << 31)	/* 5byte address enable */
>  #define ADRMD_E		(0x1 << 26)	/* Sector address access */
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/7] mtd: sh_flctl: Expand FLCMNCR register bit field

[Bastian Hecht]

Hello Laurent,

thanks for reviewing this patch.

2012/2/18 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> Thanks for the patch.
>
> On Saturday 11 February 2012 12:44:59 Bastian Hecht wrote:
>> Add support for a new hardware generation. The meaning of some bits
>> of the FLCMNCR register changed, so some new defines are added
>> parallel to the existing ones to keep backward compatibility.
>>
>> The defines allow to choose an appropriate clocking scheme.
>>
>> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> ---
>> changelog: code stays the same, just the description is slightly corrected.
>>
>>  include/linux/mtd/sh_flctl.h |   24 ++++++++++++++++++++++++
>>  1 files changed, 24 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
>> index 9cf4c4c..e66c393 100644
>> --- a/include/linux/mtd/sh_flctl.h
>> +++ b/include/linux/mtd/sh_flctl.h
>> @@ -67,6 +67,30 @@
>>  #define      CE0_ENABLE      (0x1 << 3)      /* Chip Enable 0 */
>>  #define      TYPESEL_SET     (0x1 << 0)
>>
>> +/*
>> + * Clock settings using the PULSEx registers from FLCMNCR
>> + *
>> + * Some hardware uses bits called PULSEx instead of FCKSEL_E and QTSEL_E
>> + * to control the clock divider used between the High-Speed Peripheral
>> Clock + * and the FLCTL internal clock. If so, use CLK_8_BIT_xxx for
>> connecting 8 bit + * and CLK_16_BIT_xxx for connecting 16 bit bus bandwith
>> NAND chips. For the 16 + * bit version the divider is seperate for the
>> pulse width of high and low + * signals.
>> + */
>> +#define PULSE3       (0x1 << 27)
>> +#define PULSE2       (0x1 << 17)
>> +#define PULSE1       (0x1 << 15)
>> +#define PULSE0       (0x1 << 9)
>> +#define CLK_8_BIT_0_5HP                      PULSE1
>> +#define CLK_8_BIT_1HP                        0x0
>> +#define CLK_8_BIT_1_5HP                      (PULSE1 | PULSE2)
>> +#define CLK_8_BIT_2HP                        PULSE0
>> +#define CLK_8_BIT_3HP                        (PULSE0 | PULSE1 | PULSE2)
>> +#define CLK_8_BIT_4HP                        (PULSE0 | PULSE2)
>> +#define CLK_16_BIT_6HP_LOW_2HP_HIGH  PULSE0
>> +#define CLK_16_BIT_9HP_LOW_3HP_HIGH  (PULSE0 | PULSE1 | PULSE2)
>> +#define CLK_16_BIT_12HP_LOW_4HP_HIGH (PULSE0 | PULSE2)
>> +
>
> This is nitpicking, but you could shorten the names to something like
> CLK_16B_12L_4P (at the expense of readability though). Feel free to ignore
> this.

Well yeah - I felt a bit unsure about the verbosity too, so I'll take
your suggestion, thanks.

>>  /* FLCMDCR control bits */
>>  #define ADRCNT2_E    (0x1 << 31)     /* 5byte address enable */
>>  #define ADRMD_E              (0x1 << 26)     /* Sector address access */
> --
> Regards,
>
> Laurent Pinchart

thanks,

 Bastian

[PATCH v2 2/7] mtd: sh_flctl: Reorder empty_fifo() calls

[Bastian Hecht]

Reorders the calls to make it a bit shorter and match the calling
procedure displayed in the datasheet.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
changelog: exactly same as patch v1.

 drivers/mtd/nand/sh_flctl.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 93b1f74..8c97367 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -525,7 +525,6 @@ static void flctl_cmdfunc(struct mtd_info *mtd, unsigned int command,
 			execmd_read_page_sector(mtd, page_addr);
 			break;
 		}
-		empty_fifo(flctl);
 		if (flctl->page_size)
 			set_cmd_regs(mtd, command, (NAND_CMD_READSTART << 8)
 				| command);
@@ -547,7 +546,6 @@ static void flctl_cmdfunc(struct mtd_info *mtd, unsigned int command,
 			break;
 		}
 
-		empty_fifo(flctl);
 		if (flctl->page_size) {
 			set_cmd_regs(mtd, command, (NAND_CMD_READSTART << 8)
 				| NAND_CMD_READ0);
@@ -560,7 +558,6 @@ static void flctl_cmdfunc(struct mtd_info *mtd, unsigned int command,
 		goto read_normal_exit;
 
 	case NAND_CMD_READID:
-		empty_fifo(flctl);
 		set_cmd_regs(mtd, command, command);
 		set_addr(mtd, 0, 0);
 
@@ -654,6 +651,7 @@ static void flctl_cmdfunc(struct mtd_info *mtd, unsigned int command,
 
 read_normal_exit:
 	writel(flctl->read_bytes, FLDTCNTR(flctl));	/* set read size */
+	empty_fifo(flctl);
 	start_translation(flctl);
 	read_fiforeg(flctl, flctl->read_bytes, 0);
 	wait_completion(flctl);
-- 
1.7.5.4

Re: [PATCH v2 2/7] mtd: sh_flctl: Reorder empty_fifo() calls

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Saturday 11 February 2012 12:45:00 Bastian Hecht wrote:
> Reorders the calls to make it a bit shorter and match the calling
> procedure displayed in the datasheet.

Could you point me to the datasheet section in which the order is documented ?

> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
> ---
> changelog: exactly same as patch v1.
> 
>  drivers/mtd/nand/sh_flctl.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index 93b1f74..8c97367 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -525,7 +525,6 @@ static void flctl_cmdfunc(struct mtd_info *mtd, unsigned
> int command, execmd_read_page_sector(mtd, page_addr);
>  			break;
>  		}
> -		empty_fifo(flctl);
>  		if (flctl->page_size)
>  			set_cmd_regs(mtd, command, (NAND_CMD_READSTART << 8)
> 
>  				| command);
> 
> @@ -547,7 +546,6 @@ static void flctl_cmdfunc(struct mtd_info *mtd, unsigned
> int command, break;
>  		}
> 
> -		empty_fifo(flctl);
>  		if (flctl->page_size) {
>  			set_cmd_regs(mtd, command, (NAND_CMD_READSTART << 8)
> 
>  				| NAND_CMD_READ0);
> 
> @@ -560,7 +558,6 @@ static void flctl_cmdfunc(struct mtd_info *mtd, unsigned
> int command, goto read_normal_exit;
> 
>  	case NAND_CMD_READID:
> -		empty_fifo(flctl);

This case doesn't end up in the read_normal_exit label. The empty_fifo() call 
will thus be completely removed. Is that intentional ?

>  		set_cmd_regs(mtd, command, command);
>  		set_addr(mtd, 0, 0);
> 
> @@ -654,6 +651,7 @@ static void flctl_cmdfunc(struct mtd_info *mtd, unsigned
> int command,
> 
>  read_normal_exit:
>  	writel(flctl->read_bytes, FLDTCNTR(flctl));	/* set read size */
> +	empty_fifo(flctl);
>  	start_translation(flctl);
>  	read_fiforeg(flctl, flctl->read_bytes, 0);
>  	wait_completion(flctl);
-- 
Regards,

Laurent Pinchart

[PATCH v2 3/7] mtd: sh_flctl: Expand the READID command to 8 bytes

[Bastian Hecht]

The nand base code wants to read out 8 bytes in the READID command.
Reflect this in the driver code.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
changelog: exactly same as patch v1.

 drivers/mtd/nand/sh_flctl.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 8c97367..407acb5 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -320,6 +320,7 @@ static void set_cmd_regs(struct mtd_info *mtd, uint32_t cmd, uint32_t flcmcdr_va
 		break;
 	case NAND_CMD_READID:
 		flcmncr_val &= ~SNAND_E;
+		flcmdcr_val |= CDSRC_E;
 		addr_len_bytes = ADRCNT_1;
 		break;
 	case NAND_CMD_STATUS:
@@ -559,12 +560,18 @@ static void flctl_cmdfunc(struct mtd_info *mtd, unsigned int command,
 
 	case NAND_CMD_READID:
 		set_cmd_regs(mtd, command, command);
-		set_addr(mtd, 0, 0);
 
-		flctl->read_bytes = 4;
+		/* READID is always performed using an 8-bit bus */
+		if (flctl->chip.options & NAND_BUSWIDTH_16)
+			column <<= 1;
+		set_addr(mtd, column, 0);
+
+		flctl->read_bytes = 8;
 		writel(flctl->read_bytes, FLDTCNTR(flctl)); /* set read size */
+		empty_fifo(flctl);
 		start_translation(flctl);
-		read_datareg(flctl, 0);	/* read and end */
+		read_fiforeg(flctl, flctl->read_bytes, 0);
+		wait_completion(flctl);
 		break;
 
 	case NAND_CMD_ERASE1:
-- 
1.7.5.4

Re: [PATCH v2 3/7] mtd: sh_flctl: Expand the READID command to 8 bytes

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Saturday 11 February 2012 12:45:01 Bastian Hecht wrote:
> The nand base code wants to read out 8 bytes in the READID command.
> Reflect this in the driver code.
> 
> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
> ---
> changelog: exactly same as patch v1.
> 
>  drivers/mtd/nand/sh_flctl.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index 8c97367..407acb5 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -320,6 +320,7 @@ static void set_cmd_regs(struct mtd_info *mtd, uint32_t
> cmd, uint32_t flcmcdr_va break;
>  	case NAND_CMD_READID:
>  		flcmncr_val &= ~SNAND_E;
> +		flcmdcr_val |= CDSRC_E;

this bit is marked as reserved in my SH7372 documentation, and should be set 
to 0. Won't this break compatibility with the SH7372 ?

>  		addr_len_bytes = ADRCNT_1;
>  		break;
>  	case NAND_CMD_STATUS:
> @@ -559,12 +560,18 @@ static void flctl_cmdfunc(struct mtd_info *mtd,
> unsigned int command,
> 
>  	case NAND_CMD_READID:
>  		set_cmd_regs(mtd, command, command);
> -		set_addr(mtd, 0, 0);
> 
> -		flctl->read_bytes = 4;
> +		/* READID is always performed using an 8-bit bus */
> +		if (flctl->chip.options & NAND_BUSWIDTH_16)
> +			column <<= 1;
> +		set_addr(mtd, column, 0);
> +
> +		flctl->read_bytes = 8;
>  		writel(flctl->read_bytes, FLDTCNTR(flctl)); /* set read size */
> +		empty_fifo(flctl);
>  		start_translation(flctl);
> -		read_datareg(flctl, 0);	/* read and end */
> +		read_fiforeg(flctl, flctl->read_bytes, 0);

Not saying it's a bad thing, but why do you switch from read_datareg() to 
read_fiforeg() ?

> +		wait_completion(flctl);
>  		break;
> 
>  	case NAND_CMD_ERASE1:
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/7] mtd: sh_flctl: Expand the READID command to 8 bytes

[Bastian Hecht]

Hello Laurent,

2012/2/18 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> Thanks for the patch.
>
> On Saturday 11 February 2012 12:45:01 Bastian Hecht wrote:
>> The nand base code wants to read out 8 bytes in the READID command.
>> Reflect this in the driver code.
>>
>> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>> ---
>> changelog: exactly same as patch v1.
>>
>>  drivers/mtd/nand/sh_flctl.c |   13 ++++++++++---
>>  1 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
>> index 8c97367..407acb5 100644
>> --- a/drivers/mtd/nand/sh_flctl.c
>> +++ b/drivers/mtd/nand/sh_flctl.c
>> @@ -320,6 +320,7 @@ static void set_cmd_regs(struct mtd_info *mtd, uint32_t
>> cmd, uint32_t flcmcdr_va break;
>>       case NAND_CMD_READID:
>>               flcmncr_val &= ~SNAND_E;
>> +             flcmdcr_val |= CDSRC_E;
>
> this bit is marked as reserved in my SH7372 documentation, and should be set
> to 0. Won't this break compatibility with the SH7372 ?

It's flcm*D*cr not flcmncr. Took me some time to separate these
beautiful abbreviations too ;)
It's to tell the flctl to save the read in the fifo instead of a register.

>>               addr_len_bytes = ADRCNT_1;
>>               break;
>>       case NAND_CMD_STATUS:
>> @@ -559,12 +560,18 @@ static void flctl_cmdfunc(struct mtd_info *mtd,
>> unsigned int command,
>>
>>       case NAND_CMD_READID:
>>               set_cmd_regs(mtd, command, command);
>> -             set_addr(mtd, 0, 0);
>>
>> -             flctl->read_bytes = 4;
>> +             /* READID is always performed using an 8-bit bus */
>> +             if (flctl->chip.options & NAND_BUSWIDTH_16)
>> +                     column <<= 1;
>> +             set_addr(mtd, column, 0);
>> +
>> +             flctl->read_bytes = 8;
>>               writel(flctl->read_bytes, FLDTCNTR(flctl)); /* set read size */
>> +             empty_fifo(flctl);
>>               start_translation(flctl);
>> -             read_datareg(flctl, 0); /* read and end */
>> +             read_fiforeg(flctl, flctl->read_bytes, 0);
>
> Not saying it's a bad thing, but why do you switch from read_datareg() to
> read_fiforeg() ?

We can only read 4 bytes from a register read, while we need 8 bytes
for the ID. The extra 4 bytes are used to determine the correct NAND
chip size in nand_base.c.

>> +             wait_completion(flctl);
>>               break;
>>
>>       case NAND_CMD_ERASE1:
> --
> Regards,
>
> Laurent Pinchart

thanks,

 Bastian

[PATCH v2 4/7] mtd: sh_flctl: Implement NAND_CMD_RNDOUT command

[Bastian Hecht]

Implements the command to seek and read in pages.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
changelog: exactly same as patch v1.

 drivers/mtd/nand/sh_flctl.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 407acb5..5c3e71f 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -303,6 +303,7 @@ static void set_cmd_regs(struct mtd_info *mtd, uint32_t cmd, uint32_t flcmcdr_va
 		break;
 	case NAND_CMD_READ0:
 	case NAND_CMD_READOOB:
+	case NAND_CMD_RNDOUT:
 		addr_len_bytes = flctl->rw_ADRCNT;
 		flcmdcr_val |= CDSRC_E;
 		if (flctl->chip.options & NAND_BUSWIDTH_16)
@@ -558,6 +559,18 @@ static void flctl_cmdfunc(struct mtd_info *mtd, unsigned int command,
 		flctl->read_bytes = mtd->oobsize;
 		goto read_normal_exit;
 
+	case NAND_CMD_RNDOUT:
+		if (flctl->page_size)
+			set_cmd_regs(mtd, command, (NAND_CMD_RNDOUTSTART << 8)
+				| command);
+		else
+			set_cmd_regs(mtd, command, command);
+
+		set_addr(mtd, column, 0);
+
+		flctl->read_bytes = mtd->writesize + mtd->oobsize - column;
+		goto read_normal_exit;
+
 	case NAND_CMD_READID:
 		set_cmd_regs(mtd, command, command);
 
-- 
1.7.5.4

[PATCH v2 5/7] mtd: sh_flctl: Use cached register value for FLCMNCR

[Bastian Hecht]

Instead of reading out the register, use a cached value. This will
make way for a proper runtime power management implementation.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
changelog: I delayed the rtpm patch until the clock code is fixed for
board mackerel, so that no "udelay(1);" is needed. Instead I cleaned
up the code to use a cached register value. The rtpm code will benefit
from it.

 drivers/mtd/nand/sh_flctl.c  |   15 +++------------
 include/linux/mtd/sh_flctl.h |    1 +
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 5c3e71f..1af41fd 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -681,16 +681,13 @@ read_normal_exit:
 static void flctl_select_chip(struct mtd_info *mtd, int chipnr)
 {
 	struct sh_flctl *flctl = mtd_to_flctl(mtd);
-	uint32_t flcmncr_val = readl(FLCMNCR(flctl));
 
 	switch (chipnr) {
 	case -1:
-		flcmncr_val &= ~CE0_ENABLE;
-		writel(flcmncr_val, FLCMNCR(flctl));
+		writel(flctl->flcmncr_val, FLCMNCR(flctl));
 		break;
 	case 0:
-		flcmncr_val |= CE0_ENABLE;
-		writel(flcmncr_val, FLCMNCR(flctl));
+		writel(flctl->flcmncr_val | CE0_ENABLE, FLCMNCR(flctl));
 		break;
 	default:
 		BUG();
@@ -748,11 +745,6 @@ static int flctl_verify_buf(struct mtd_info *mtd, const u_char *buf, int len)
 	return 0;
 }
 
-static void flctl_register_init(struct sh_flctl *flctl, unsigned long val)
-{
-	writel(val, FLCMNCR(flctl));
-}
-
 static int flctl_chip_init_tail(struct mtd_info *mtd)
 {
 	struct sh_flctl *flctl = mtd_to_flctl(mtd);
@@ -851,10 +843,9 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 	nand = &flctl->chip;
 	flctl_mtd->priv = nand;
 	flctl->pdev = pdev;
+	flctl->flcmncr_val = pdata->flcmncr_val;
 	flctl->hwecc = pdata->has_hwecc;
 
-	flctl_register_init(flctl, pdata->flcmncr_val);
-
 	nand->options = NAND_NO_AUTOINCR;
 
 	/* Set address of hardware control function */
diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
index e66c393..107fd8a 100644
--- a/include/linux/mtd/sh_flctl.h
+++ b/include/linux/mtd/sh_flctl.h
@@ -132,6 +132,7 @@ struct sh_flctl {
 	int	erase1_page_addr;	/* page_addr in ERASE1 cmd */
 	uint32_t erase_ADRCNT;		/* bits of FLCMDCR in ERASE1 cmd */
 	uint32_t rw_ADRCNT;	/* bits of FLCMDCR in READ WRITE cmd */
+	uint32_t flcmncr_val;	/* base value of FLCMNCR */
 
 	int	hwecc_cant_correct[4];
 
-- 
1.7.5.4

Re: [PATCH v2 5/7] mtd: sh_flctl: Use cached register value for FLCMNCR

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Saturday 11 February 2012 12:45:03 Bastian Hecht wrote:
> Instead of reading out the register, use a cached value. This will
> make way for a proper runtime power management implementation.
> 
> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
> ---
> changelog: I delayed the rtpm patch until the clock code is fixed for
> board mackerel, so that no "udelay(1);" is needed. Instead I cleaned
> up the code to use a cached register value. The rtpm code will benefit
> from it.
> 
>  drivers/mtd/nand/sh_flctl.c  |   15 +++------------
>  include/linux/mtd/sh_flctl.h |    1 +
>  2 files changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index 5c3e71f..1af41fd 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -681,16 +681,13 @@ read_normal_exit:
>  static void flctl_select_chip(struct mtd_info *mtd, int chipnr)
>  {
>  	struct sh_flctl *flctl = mtd_to_flctl(mtd);
> -	uint32_t flcmncr_val = readl(FLCMNCR(flctl));
> 
>  	switch (chipnr) {
>  	case -1:
> -		flcmncr_val &= ~CE0_ENABLE;
> -		writel(flcmncr_val, FLCMNCR(flctl));
> +		writel(flctl->flcmncr_val, FLCMNCR(flctl));
>  		break;
>  	case 0:
> -		flcmncr_val |= CE0_ENABLE;
> -		writel(flcmncr_val, FLCMNCR(flctl));
> +		writel(flctl->flcmncr_val | CE0_ENABLE, FLCMNCR(flctl));
>  		break;
>  	default:
>  		BUG();

Shouldn't you use the cached value in set_cmd_regs() as well ?

> @@ -748,11 +745,6 @@ static int flctl_verify_buf(struct mtd_info *mtd, const
> u_char *buf, int len) return 0;
>  }
> 
> -static void flctl_register_init(struct sh_flctl *flctl, unsigned long val)
> -{
> -	writel(val, FLCMNCR(flctl));
> -}
> -
>  static int flctl_chip_init_tail(struct mtd_info *mtd)
>  {
>  	struct sh_flctl *flctl = mtd_to_flctl(mtd);
> @@ -851,10 +843,9 @@ static int __devinit flctl_probe(struct platform_device
> *pdev) nand = &flctl->chip;
>  	flctl_mtd->priv = nand;
>  	flctl->pdev = pdev;
> +	flctl->flcmncr_val = pdata->flcmncr_val;
>  	flctl->hwecc = pdata->has_hwecc;
> 
> -	flctl_register_init(flctl, pdata->flcmncr_val);
> -
>  	nand->options = NAND_NO_AUTOINCR;
> 
>  	/* Set address of hardware control function */
> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
> index e66c393..107fd8a 100644
> --- a/include/linux/mtd/sh_flctl.h
> +++ b/include/linux/mtd/sh_flctl.h
> @@ -132,6 +132,7 @@ struct sh_flctl {
>  	int	erase1_page_addr;	/* page_addr in ERASE1 cmd */
>  	uint32_t erase_ADRCNT;		/* bits of FLCMDCR in ERASE1 cmd */
>  	uint32_t rw_ADRCNT;	/* bits of FLCMDCR in READ WRITE cmd */
> +	uint32_t flcmncr_val;	/* base value of FLCMNCR */
> 
>  	int	hwecc_cant_correct[4];
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/7] mtd: sh_flctl: Use cached register value for FLCMNCR

[Bastian Hecht]

Hello Laurent,

2012/2/18 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> Thanks for the patch.
>
> On Saturday 11 February 2012 12:45:03 Bastian Hecht wrote:
>> Instead of reading out the register, use a cached value. This will
>> make way for a proper runtime power management implementation.
>>
>> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>> ---
>> changelog: I delayed the rtpm patch until the clock code is fixed for
>> board mackerel, so that no "udelay(1);" is needed. Instead I cleaned
>> up the code to use a cached register value. The rtpm code will benefit
>> from it.
>>
>>  drivers/mtd/nand/sh_flctl.c  |   15 +++------------
>>  include/linux/mtd/sh_flctl.h |    1 +
>>  2 files changed, 4 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
>> index 5c3e71f..1af41fd 100644
>> --- a/drivers/mtd/nand/sh_flctl.c
>> +++ b/drivers/mtd/nand/sh_flctl.c
>> @@ -681,16 +681,13 @@ read_normal_exit:
>>  static void flctl_select_chip(struct mtd_info *mtd, int chipnr)
>>  {
>>       struct sh_flctl *flctl = mtd_to_flctl(mtd);
>> -     uint32_t flcmncr_val = readl(FLCMNCR(flctl));
>>
>>       switch (chipnr) {
>>       case -1:
>> -             flcmncr_val &= ~CE0_ENABLE;
>> -             writel(flcmncr_val, FLCMNCR(flctl));
>> +             writel(flctl->flcmncr_val, FLCMNCR(flctl));
>>               break;
>>       case 0:
>> -             flcmncr_val |= CE0_ENABLE;
>> -             writel(flcmncr_val, FLCMNCR(flctl));
>> +             writel(flctl->flcmncr_val | CE0_ENABLE, FLCMNCR(flctl));
>>               break;
>>       default:
>>               BUG();
>
> Shouldn't you use the cached value in set_cmd_regs() as well ?

Yes that's true. I agree that I should use it on all occasions.

>> @@ -748,11 +745,6 @@ static int flctl_verify_buf(struct mtd_info *mtd, const
>> u_char *buf, int len) return 0;
>>  }
>>
>> -static void flctl_register_init(struct sh_flctl *flctl, unsigned long val)
>> -{
>> -     writel(val, FLCMNCR(flctl));
>> -}
>> -
>>  static int flctl_chip_init_tail(struct mtd_info *mtd)
>>  {
>>       struct sh_flctl *flctl = mtd_to_flctl(mtd);
>> @@ -851,10 +843,9 @@ static int __devinit flctl_probe(struct platform_device
>> *pdev) nand = &flctl->chip;
>>       flctl_mtd->priv = nand;
>>       flctl->pdev = pdev;
>> +     flctl->flcmncr_val = pdata->flcmncr_val;
>>       flctl->hwecc = pdata->has_hwecc;
>>
>> -     flctl_register_init(flctl, pdata->flcmncr_val);
>> -
>>       nand->options = NAND_NO_AUTOINCR;
>>
>>       /* Set address of hardware control function */
>> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
>> index e66c393..107fd8a 100644
>> --- a/include/linux/mtd/sh_flctl.h
>> +++ b/include/linux/mtd/sh_flctl.h
>> @@ -132,6 +132,7 @@ struct sh_flctl {
>>       int     erase1_page_addr;       /* page_addr in ERASE1 cmd */
>>       uint32_t erase_ADRCNT;          /* bits of FLCMDCR in ERASE1 cmd */
>>       uint32_t rw_ADRCNT;     /* bits of FLCMDCR in READ WRITE cmd */
>> +     uint32_t flcmncr_val;   /* base value of FLCMNCR */
>>
>>       int     hwecc_cant_correct[4];
> --
> Regards,
>
> Laurent Pinchart

thanks,

 Bastian

[PATCH v2 6/7] mtd: sh_flctl: Add FLHOLDCR register

[Bastian Hecht]

Add a register used in new FLCTL hardware and a feature flag for it.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
changelog: the write to the register has been moved due to patch 5.

 drivers/mtd/nand/sh_flctl.c  |    3 +++
 include/linux/mtd/sh_flctl.h |   12 ++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 1af41fd..40dda26 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -688,6 +688,8 @@ static void flctl_select_chip(struct mtd_info *mtd, int chipnr)
 		break;
 	case 0:
 		writel(flctl->flcmncr_val | CE0_ENABLE, FLCMNCR(flctl));
+		if (flctl->holden)
+			writel(HOLDEN, FLHOLDCR(flctl));
 		break;
 	default:
 		BUG();
@@ -845,6 +847,7 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 	flctl->pdev = pdev;
 	flctl->flcmncr_val = pdata->flcmncr_val;
 	flctl->hwecc = pdata->has_hwecc;
+	flctl->holden = pdata->use_holden;
 
 	nand->options = NAND_NO_AUTOINCR;
 
diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
index 107fd8a..6046443 100644
--- a/include/linux/mtd/sh_flctl.h
+++ b/include/linux/mtd/sh_flctl.h
@@ -38,6 +38,7 @@
 #define FLDTFIFO(f)		(f->reg + 0x24)
 #define FLECFIFO(f)		(f->reg + 0x28)
 #define FLTRCR(f)		(f->reg + 0x2C)
+#define FLHOLDCR(f)		(f->reg + 0x38)
 #define	FL4ECCRESULT0(f)	(f->reg + 0x80)
 #define	FL4ECCRESULT1(f)	(f->reg + 0x84)
 #define	FL4ECCRESULT2(f)	(f->reg + 0x88)
@@ -109,6 +110,15 @@
 #define TRSTRT		(0x1 << 0)	/* translation start */
 #define TREND		(0x1 << 1)	/* translation end */
 
+/*
+ * FLHOLDCR control bits
+ *
+ * HOLDEN: Bus Occupancy Enable (inverted)
+ * Enable this bit when the external bus might be used in between transfers.
+ * If not set and the bus gets used by other modules, a deadlock occurs.
+ */
+#define HOLDEN		(0x1 << 0)
+
 /* FL4ECCCR control bits */
 #define	_4ECCFA		(0x1 << 2)	/* 4 symbols correct fault */
 #define	_4ECCEND	(0x1 << 1)	/* 4 symbols end */
@@ -138,6 +148,7 @@ struct sh_flctl {
 
 	unsigned page_size:1;	/* NAND page size (0 = 512, 1 = 2048) */
 	unsigned hwecc:1;	/* Hardware ECC (0 = disabled, 1 = enabled) */
+	unsigned holden:1;	/* Hardware has FLHOLDCR and HOLDEN is set */
 };
 
 struct sh_flctl_platform_data {
@@ -146,6 +157,7 @@ struct sh_flctl_platform_data {
 	unsigned long		flcmncr_val;
 
 	unsigned has_hwecc:1;
+	unsigned use_holden:1;
 };
 
 static inline struct sh_flctl *mtd_to_flctl(struct mtd_info *mtdinfo)
-- 
1.7.5.4

Re: [PATCH v2 6/7] mtd: sh_flctl: Add FLHOLDCR register

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Saturday 11 February 2012 12:45:04 Bastian Hecht wrote:
> Add a register used in new FLCTL hardware and a feature flag for it.
> 
> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
> ---
> changelog: the write to the register has been moved due to patch 5.
> 
>  drivers/mtd/nand/sh_flctl.c  |    3 +++
>  include/linux/mtd/sh_flctl.h |   12 ++++++++++++
>  2 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index 1af41fd..40dda26 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -688,6 +688,8 @@ static void flctl_select_chip(struct mtd_info *mtd, int
> chipnr) break;
>  	case 0:
>  		writel(flctl->flcmncr_val | CE0_ENABLE, FLCMNCR(flctl));
> +		if (flctl->holden)
> +			writel(HOLDEN, FLHOLDCR(flctl));

Can't this be done at probe time (maybe in flctl_chip_init_tail()) ? You could 
then get rid of the flctl->holden field and use platform data directly.

>  		break;
>  	default:
>  		BUG();
> @@ -845,6 +847,7 @@ static int __devinit flctl_probe(struct platform_device
> *pdev) flctl->pdev = pdev;
>  	flctl->flcmncr_val = pdata->flcmncr_val;
>  	flctl->hwecc = pdata->has_hwecc;
> +	flctl->holden = pdata->use_holden;
> 
>  	nand->options = NAND_NO_AUTOINCR;
> 
> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
> index 107fd8a..6046443 100644
> --- a/include/linux/mtd/sh_flctl.h
> +++ b/include/linux/mtd/sh_flctl.h
> @@ -38,6 +38,7 @@
>  #define FLDTFIFO(f)		(f->reg + 0x24)
>  #define FLECFIFO(f)		(f->reg + 0x28)
>  #define FLTRCR(f)		(f->reg + 0x2C)
> +#define FLHOLDCR(f)		(f->reg + 0x38)
>  #define	FL4ECCRESULT0(f)	(f->reg + 0x80)
>  #define	FL4ECCRESULT1(f)	(f->reg + 0x84)
>  #define	FL4ECCRESULT2(f)	(f->reg + 0x88)
> @@ -109,6 +110,15 @@
>  #define TRSTRT		(0x1 << 0)	/* translation start */
>  #define TREND		(0x1 << 1)	/* translation end */
> 
> +/*
> + * FLHOLDCR control bits
> + *
> + * HOLDEN: Bus Occupancy Enable (inverted)
> + * Enable this bit when the external bus might be used in between
> transfers. + * If not set and the bus gets used by other modules, a
> deadlock occurs. + */
> +#define HOLDEN		(0x1 << 0)
> +
>  /* FL4ECCCR control bits */
>  #define	_4ECCFA		(0x1 << 2)	/* 4 symbols correct fault */
>  #define	_4ECCEND	(0x1 << 1)	/* 4 symbols end */
> @@ -138,6 +148,7 @@ struct sh_flctl {
> 
>  	unsigned page_size:1;	/* NAND page size (0 = 512, 1 = 2048) */
>  	unsigned hwecc:1;	/* Hardware ECC (0 = disabled, 1 = enabled) */
> +	unsigned holden:1;	/* Hardware has FLHOLDCR and HOLDEN is set */
>  };
> 
>  struct sh_flctl_platform_data {
> @@ -146,6 +157,7 @@ struct sh_flctl_platform_data {
>  	unsigned long		flcmncr_val;
> 
>  	unsigned has_hwecc:1;
> +	unsigned use_holden:1;
>  };
> 
>  static inline struct sh_flctl *mtd_to_flctl(struct mtd_info *mtdinfo)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 6/7] mtd: sh_flctl: Add FLHOLDCR register

[Bastian Hecht]

Hello Laurent,

2012/2/18 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> Thanks for the patch.
>
> On Saturday 11 February 2012 12:45:04 Bastian Hecht wrote:
>> Add a register used in new FLCTL hardware and a feature flag for it.
>>
>> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>> ---
>> changelog: the write to the register has been moved due to patch 5.
>>
>>  drivers/mtd/nand/sh_flctl.c  |    3 +++
>>  include/linux/mtd/sh_flctl.h |   12 ++++++++++++
>>  2 files changed, 15 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
>> index 1af41fd..40dda26 100644
>> --- a/drivers/mtd/nand/sh_flctl.c
>> +++ b/drivers/mtd/nand/sh_flctl.c
>> @@ -688,6 +688,8 @@ static void flctl_select_chip(struct mtd_info *mtd, int
>> chipnr) break;
>>       case 0:
>>               writel(flctl->flcmncr_val | CE0_ENABLE, FLCMNCR(flctl));
>> +             if (flctl->holden)
>> +                     writel(HOLDEN, FLHOLDCR(flctl));
>
> Can't this be done at probe time (maybe in flctl_chip_init_tail()) ? You could
> then get rid of the flctl->holden field and use platform data directly.

I need this information for runtime pm. I reworked this patch series
to be without rtpm, but from the next patch on (that's only waiting to
be posted), I'll want to have it like this.
In fact this is a design question:
I can either have a resume/runtime_resume callback whose only job is
to set FLHOLDCR. or I integrate this into select_chip. The first
variation saves 1 register write in case of no rtpm activated - the
second variation makes the code a bit slimmer.

What do you think?
1. Leave it like this and save the callbacks in the future
2. Rework it to be set at probe time() and postpone the design decision?
3. Rework it to be set at probe time() and implement callbacks in the
upcoming patches?

>>               break;
>>       default:
>>               BUG();
>> @@ -845,6 +847,7 @@ static int __devinit flctl_probe(struct platform_device
>> *pdev) flctl->pdev = pdev;
>>       flctl->flcmncr_val = pdata->flcmncr_val;
>>       flctl->hwecc = pdata->has_hwecc;
>> +     flctl->holden = pdata->use_holden;
>>
>>       nand->options = NAND_NO_AUTOINCR;
>>
>> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
>> index 107fd8a..6046443 100644
>> --- a/include/linux/mtd/sh_flctl.h
>> +++ b/include/linux/mtd/sh_flctl.h
>> @@ -38,6 +38,7 @@
>>  #define FLDTFIFO(f)          (f->reg + 0x24)
>>  #define FLECFIFO(f)          (f->reg + 0x28)
>>  #define FLTRCR(f)            (f->reg + 0x2C)
>> +#define FLHOLDCR(f)          (f->reg + 0x38)
>>  #define      FL4ECCRESULT0(f)        (f->reg + 0x80)
>>  #define      FL4ECCRESULT1(f)        (f->reg + 0x84)
>>  #define      FL4ECCRESULT2(f)        (f->reg + 0x88)
>> @@ -109,6 +110,15 @@
>>  #define TRSTRT               (0x1 << 0)      /* translation start */
>>  #define TREND                (0x1 << 1)      /* translation end */
>>
>> +/*
>> + * FLHOLDCR control bits
>> + *
>> + * HOLDEN: Bus Occupancy Enable (inverted)
>> + * Enable this bit when the external bus might be used in between
>> transfers. + * If not set and the bus gets used by other modules, a
>> deadlock occurs. + */
>> +#define HOLDEN               (0x1 << 0)
>> +
>>  /* FL4ECCCR control bits */
>>  #define      _4ECCFA         (0x1 << 2)      /* 4 symbols correct fault */
>>  #define      _4ECCEND        (0x1 << 1)      /* 4 symbols end */
>> @@ -138,6 +148,7 @@ struct sh_flctl {
>>
>>       unsigned page_size:1;   /* NAND page size (0 = 512, 1 = 2048) */
>>       unsigned hwecc:1;       /* Hardware ECC (0 = disabled, 1 = enabled) */
>> +     unsigned holden:1;      /* Hardware has FLHOLDCR and HOLDEN is set */
>>  };
>>
>>  struct sh_flctl_platform_data {
>> @@ -146,6 +157,7 @@ struct sh_flctl_platform_data {
>>       unsigned long           flcmncr_val;
>>
>>       unsigned has_hwecc:1;
>> +     unsigned use_holden:1;
>>  };
>>
>>  static inline struct sh_flctl *mtd_to_flctl(struct mtd_info *mtdinfo)
>
> --
> Regards,
>
> Laurent Pinchart

thanks for all the reviews!

 Bastian

[PATCH v2 7/7] ARM: mach-shmobile: mackerel: Add the flash controller flctl

[Bastian Hecht]

Add board and clock setup code for the SH Mobile flctl controller.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
changelog: stays exactly the same as v1.

 arch/arm/mach-shmobile/board-mackerel.c |   71 +++++++++++++++++++++++++++++++
 arch/arm/mach-shmobile/clock-sh7372.c   |    4 +-
 2 files changed, 74 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index 9c5e598..136b3cb 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -39,6 +39,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/physmap.h>
+#include <linux/mtd/sh_flctl.h>
 #include <linux/pm_clock.h>
 #include <linux/smsc911x.h>
 #include <linux/sh_intc.h>
@@ -994,6 +995,50 @@ static struct platform_device fsi_ak4643_device = {
 	.name		= "sh_fsi2_a_ak4643",
 };
 
+/* FLCTL */
+static struct mtd_partition nand_partition_info[] = {
+	{
+		.name	= "system",
+		.offset	= 0,
+		.size	= 128 * 1024 * 1024,
+	},
+	{
+		.name	= "userdata",
+		.offset	= MTDPART_OFS_APPEND,
+		.size	= 256 * 1024 * 1024,
+	},
+	{
+		.name	= "cache",
+		.offset	= MTDPART_OFS_APPEND,
+		.size	= 128 * 1024 * 1024,
+	},
+};
+
+static struct resource nand_flash_resources[] = {
+	[0] = {
+		.start	= 0xe6a30000,
+		.end	= 0xe6a3009b,
+		.flags	= IORESOURCE_MEM,
+	}
+};
+
+static struct sh_flctl_platform_data nand_flash_data = {
+	.parts		= nand_partition_info,
+	.nr_parts	= ARRAY_SIZE(nand_partition_info),
+	.flcmncr_val	= CLK_16_BIT_12HP_LOW_4HP_HIGH | TYPESEL_SET
+			| SHBUSSEL | SEL_16BIT | SNAND_E,
+	.use_holden	= 1,
+};
+
+static struct platform_device nand_flash_device = {
+	.name		= "sh_flctl",
+	.resource	= nand_flash_resources,
+	.num_resources	= ARRAY_SIZE(nand_flash_resources),
+	.dev		= {
+		.platform_data = &nand_flash_data,
+	},
+};
+
 /*
  * The card detect pin of the top SD/MMC slot (CN7) is active low and is
  * connected to GPIO A22 of SH7372 (GPIO_PORT41).
@@ -1306,6 +1351,7 @@ static struct platform_device *mackerel_devices[] __initdata = {
 	&fsi_device,
 	&fsi_ak4643_device,
 	&fsi_hdmi_device,
+	&nand_flash_device,
 	&sdhi0_device,
 #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
 	&sdhi1_device,
@@ -1558,6 +1604,30 @@ static void __init mackerel_init(void)
 	gpio_request(GPIO_FN_MMCCMD0, NULL);
 	gpio_request(GPIO_FN_MMCCLK0, NULL);
 
+	/* FLCTL */
+	gpio_request(GPIO_FN_D0_NAF0, NULL);
+	gpio_request(GPIO_FN_D1_NAF1, NULL);
+	gpio_request(GPIO_FN_D2_NAF2, NULL);
+	gpio_request(GPIO_FN_D3_NAF3, NULL);
+	gpio_request(GPIO_FN_D4_NAF4, NULL);
+	gpio_request(GPIO_FN_D5_NAF5, NULL);
+	gpio_request(GPIO_FN_D6_NAF6, NULL);
+	gpio_request(GPIO_FN_D7_NAF7, NULL);
+	gpio_request(GPIO_FN_D8_NAF8, NULL);
+	gpio_request(GPIO_FN_D9_NAF9, NULL);
+	gpio_request(GPIO_FN_D10_NAF10, NULL);
+	gpio_request(GPIO_FN_D11_NAF11, NULL);
+	gpio_request(GPIO_FN_D12_NAF12, NULL);
+	gpio_request(GPIO_FN_D13_NAF13, NULL);
+	gpio_request(GPIO_FN_D14_NAF14, NULL);
+	gpio_request(GPIO_FN_D15_NAF15, NULL);
+	gpio_request(GPIO_FN_FCE0, NULL);
+	gpio_request(GPIO_FN_WE0_FWE, NULL);
+	gpio_request(GPIO_FN_FRB, NULL);
+	gpio_request(GPIO_FN_A4_FOE, NULL);
+	gpio_request(GPIO_FN_A5_FCDE, NULL);
+	gpio_request(GPIO_FN_RD_FSC, NULL);
+
 	/* enable GPS module (GT-720F) */
 	gpio_request(GPIO_FN_SCIFA2_TXD1, NULL);
 	gpio_request(GPIO_FN_SCIFA2_RXD1, NULL);
@@ -1602,6 +1672,7 @@ static void __init mackerel_init(void)
 	sh7372_add_device_to_domain(&sh7372_a4mp, &fsi_device);
 	sh7372_add_device_to_domain(&sh7372_a3sp, &usbhs0_device);
 	sh7372_add_device_to_domain(&sh7372_a3sp, &usbhs1_device);
+	sh7372_add_device_to_domain(&sh7372_a3sp, &nand_flash_device);
 	sh7372_add_device_to_domain(&sh7372_a3sp, &sh_mmcif_device);
 	sh7372_add_device_to_domain(&sh7372_a3sp, &sdhi0_device);
 #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
diff --git a/arch/arm/mach-shmobile/clock-sh7372.c b/arch/arm/mach-shmobile/clock-sh7372.c
index 995a9c3..99dd283 100644
--- a/arch/arm/mach-shmobile/clock-sh7372.c
+++ b/arch/arm/mach-shmobile/clock-sh7372.c
@@ -511,7 +511,7 @@ enum { MSTP001, MSTP000,
        MSTP223,
        MSTP218, MSTP217, MSTP216, MSTP214, MSTP208, MSTP207,
        MSTP206, MSTP205, MSTP204, MSTP203, MSTP202, MSTP201, MSTP200,
-       MSTP328, MSTP323, MSTP322, MSTP314, MSTP313, MSTP312,
+	MSTP328, MSTP323, MSTP322, MSTP315, MSTP314, MSTP313, MSTP312,
        MSTP423, MSTP415, MSTP413, MSTP411, MSTP410, MSTP407, MSTP406,
        MSTP405, MSTP404, MSTP403, MSTP400,
        MSTP_NR };
@@ -553,6 +553,7 @@ static struct clk mstp_clks[MSTP_NR] = {
 	[MSTP328] = MSTP(&div6_clks[DIV6_SPU], SMSTPCR3, 28, 0), /* FSI2 */
 	[MSTP323] = MSTP(&div6_clks[DIV6_SUB], SMSTPCR3, 23, 0), /* IIC1 */
 	[MSTP322] = MSTP(&div6_clks[DIV6_SUB], SMSTPCR3, 22, 0), /* USB0 */
+	[MSTP315] = MSTP(&div4_clks[DIV4_HP], SMSTPCR3, 15, 0), /* FLCTL*/
 	[MSTP314] = MSTP(&div4_clks[DIV4_HP], SMSTPCR3, 14, 0), /* SDHI0 */
 	[MSTP313] = MSTP(&div4_clks[DIV4_HP], SMSTPCR3, 13, 0), /* SDHI1 */
 	[MSTP312] = MSTP(&div4_clks[DIV4_HP], SMSTPCR3, 12, 0), /* MMC */
@@ -653,6 +654,7 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_DEV_ID("r8a66597_hcd.0", &mstp_clks[MSTP322]), /* USB0 */
 	CLKDEV_DEV_ID("r8a66597_udc.0", &mstp_clks[MSTP322]), /* USB0 */
 	CLKDEV_DEV_ID("renesas_usbhs.0", &mstp_clks[MSTP322]), /* USB0 */
+	CLKDEV_DEV_ID("sh_flctl.0", &mstp_clks[MSTP315]), /* FLCTL */
 	CLKDEV_DEV_ID("sh_mobile_sdhi.0", &mstp_clks[MSTP314]), /* SDHI0 */
 	CLKDEV_DEV_ID("sh_mobile_sdhi.1", &mstp_clks[MSTP313]), /* SDHI1 */
 	CLKDEV_DEV_ID("sh_mmcif.0", &mstp_clks[MSTP312]), /* MMC */
-- 
1.7.5.4

Re: [PATCH v2 0/7] SH Mobile sh_flctl driver brush up

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

On Sat, 2012-02-11 at 12:44 +0100, Bastian Hecht wrote:
> This patch series brushs up the sh_flctl.c a bit to make it work with
> the mackerel board. The hardware has changed and this needed to be
> reflected in the NAND host driver.
> On the road some minor enhancements have been incorporated.

I cannot even compile-test this patch because I get the following error:


sh4-linux-gcc: error: command line option '-m4-nofpu' is not supported
by this configuration


with the cross-compiler downloaded from here:

http://kernel.org/pub/tools/crosstool/files/bin/x86_64/4.6.0/x86_64-gcc-4.6.0-nolibc_sh4-linux.tar.bz2

Could you please describe how could I compile-test it?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 0/7] SH Mobile sh_flctl driver brush up

[Simon Horman]

On Mon, Feb 13, 2012 at 03:41:55PM +0200, Artem Bityutskiy wrote:
> On Sat, 2012-02-11 at 12:44 +0100, Bastian Hecht wrote:
> > This patch series brushs up the sh_flctl.c a bit to make it work with
> > the mackerel board. The hardware has changed and this needed to be
> > reflected in the NAND host driver.
> > On the road some minor enhancements have been incorporated.
> 
> I cannot even compile-test this patch because I get the following error:
> 
> 
> sh4-linux-gcc: error: command line option '-m4-nofpu' is not supported
> by this configuration
> 
> 
> with the cross-compiler downloaded from here:
> 
> http://kernel.org/pub/tools/crosstool/files/bin/x86_64/4.6.0/x86_64-gcc-4.6.0-nolibc_sh4-linux.tar.bz2
> 
> Could you please describe how could I compile-test it?

You may find it easier to build a kernel for mackerel using
arch arm and defconfig_mackerel.

Personally I have had good experience using Code Sourcery (now part of
Mentor Graphics?) G++ lite to compile both arm and sh kernels.

Re: [PATCH v2 0/7] SH Mobile sh_flctl driver brush up

[Bastian Hecht]

Hello Artem,

2012/2/14 Simon Horman <horms@verge.net.au>:
> On Mon, Feb 13, 2012 at 03:41:55PM +0200, Artem Bityutskiy wrote:
>> On Sat, 2012-02-11 at 12:44 +0100, Bastian Hecht wrote:
>> > This patch series brushs up the sh_flctl.c a bit to make it work with
>> > the mackerel board. The hardware has changed and this needed to be
>> > reflected in the NAND host driver.
>> > On the road some minor enhancements have been incorporated.
>>
>> I cannot even compile-test this patch because I get the following error:
>>
>>
>> sh4-linux-gcc: error: command line option '-m4-nofpu' is not supported
>> by this configuration
>>
>>
>> with the cross-compiler downloaded from here:
>>
>> http://kernel.org/pub/tools/crosstool/files/bin/x86_64/4.6.0/x86_64-gcc-4.6.0-nolibc_sh4-linux.tar.bz2
>>
>> Could you please describe how could I compile-test it?
>
> You may find it easier to build a kernel for mackerel using
> arch arm and defconfig_mackerel.
>
> Personally I have had good experience using Code Sourcery (now part of
> Mentor Graphics?) G++ lite to compile both arm and sh kernels.
>
>

I as well use a Code Sourcery toolchain. arm-none-linux-gnueabi-gcc -v
states "gcc version 4.4.1 (Sourcery G++ Lite 2010q1-202)". It took me
quite a bit to find a link to the toolchain as these gpl toolchains
from Code Sourcery almost seem to be hidden. But here it is if you
want to give it a try:

http://www.codesourcery.com/sgpp/lite/arm/portal/package6488/public/arm-none-linux-gnueabi/arm-2010q1-202-arm-none-linux-gnueabi-i686-pc-linux-gnu.tar.bz2

I very appreciate that you take a look at the patches - I really hope
them to be merged.

thanks,

 Bastian

Re: [PATCH v2 0/7] SH Mobile sh_flctl driver brush up

[Artem Bityutskiy]

On Tue, 2012-02-14 at 11:58 +0100, Bastian Hecht wrote:
> Hello Artem,
> 
> 2012/2/14 Simon Horman <horms@verge.net.au>:
> > On Mon, Feb 13, 2012 at 03:41:55PM +0200, Artem Bityutskiy wrote:
> >> On Sat, 2012-02-11 at 12:44 +0100, Bastian Hecht wrote:
> >> > This patch series brushs up the sh_flctl.c a bit to make it work with
> >> > the mackerel board. The hardware has changed and this needed to be
> >> > reflected in the NAND host driver.
> >> > On the road some minor enhancements have been incorporated.
> >>
> >> I cannot even compile-test this patch because I get the following error:
> >>
> >>
> >> sh4-linux-gcc: error: command line option '-m4-nofpu' is not supported
> >> by this configuration
> >>
> >>
> >> with the cross-compiler downloaded from here:
> >>
> >> http://kernel.org/pub/tools/crosstool/files/bin/x86_64/4.6.0/x86_64-gcc-4.6.0-nolibc_sh4-linux.tar.bz2
> >>
> >> Could you please describe how could I compile-test it?
> >
> > You may find it easier to build a kernel for mackerel using
> > arch arm and defconfig_mackerel.
> >
> > Personally I have had good experience using Code Sourcery (now part of
> > Mentor Graphics?) G++ lite to compile both arm and sh kernels.
> >
> >
> 
> I as well use a Code Sourcery toolchain. arm-none-linux-gnueabi-gcc -v
> states "gcc version 4.4.1 (Sourcery G++ Lite 2010q1-202)". It took me
> quite a bit to find a link to the toolchain as these gpl toolchains
> from Code Sourcery almost seem to be hidden. But here it is if you
> want to give it a try:

I am a bit confused. We have the following in drivers/mtd/nand/Kconfig

config MTD_NAND_SH_FLCTL
        tristate "Support for NAND on Renesas SuperH FLCTL"
        depends on SUPERH || ARCH_SHMOBILE
        help
          Several Renesas SuperH CPU has FLCTL. This option enables support
          for NAND Flash using FLCTL.

SUPERH and ARCH_SHMOBILE are SH platforms - how can I compile-test your
patch-set with an arm cross-compiler?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v2 0/7] SH Mobile sh_flctl driver brush up

[Laurent Pinchart]

Hi Artem,

On Friday 09 March 2012 14:11:52 Artem Bityutskiy wrote:
> On Tue, 2012-02-14 at 11:58 +0100, Bastian Hecht wrote:
> > 2012/2/14 Simon Horman <horms@verge.net.au>:
> > > On Mon, Feb 13, 2012 at 03:41:55PM +0200, Artem Bityutskiy wrote:
> > >> On Sat, 2012-02-11 at 12:44 +0100, Bastian Hecht wrote:
> > >> > This patch series brushs up the sh_flctl.c a bit to make it work with
> > >> > the mackerel board. The hardware has changed and this needed to be
> > >> > reflected in the NAND host driver.
> > >> > On the road some minor enhancements have been incorporated.
> > >> 
> > >> I cannot even compile-test this patch because I get the following
> > >> error:
> > >> 
> > >> 
> > >> sh4-linux-gcc: error: command line option '-m4-nofpu' is not supported
> > >> by this configuration
> > >> 
> > >> 
> > >> with the cross-compiler downloaded from here:
> > >> 
> > >> http://kernel.org/pub/tools/crosstool/files/bin/x86_64/4.6.0/x86_64-gcc
> > >> -4.6.0-nolibc_sh4-linux.tar.bz2
> > >> 
> > >> Could you please describe how could I compile-test it?
> > > 
> > > You may find it easier to build a kernel for mackerel using
> > > arch arm and defconfig_mackerel.
> > > 
> > > Personally I have had good experience using Code Sourcery (now part of
> > > Mentor Graphics?) G++ lite to compile both arm and sh kernels.
> > 
> > I as well use a Code Sourcery toolchain. arm-none-linux-gnueabi-gcc -v
> > states "gcc version 4.4.1 (Sourcery G++ Lite 2010q1-202)". It took me
> > quite a bit to find a link to the toolchain as these gpl toolchains
> > from Code Sourcery almost seem to be hidden. But here it is if you
> 
> > want to give it a try:
> I am a bit confused. We have the following in drivers/mtd/nand/Kconfig
> 
> config MTD_NAND_SH_FLCTL
>         tristate "Support for NAND on Renesas SuperH FLCTL"
>         depends on SUPERH || ARCH_SHMOBILE
>         help
>           Several Renesas SuperH CPU has FLCTL. This option enables support
>           for NAND Flash using FLCTL.
> 
> SUPERH and ARCH_SHMOBILE are SH platforms - how can I compile-test your
> patch-set with an arm cross-compiler?

ARCH_SHMOBILE is an ARM platform (defined in arch/arm/Kconfig).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/7] SH Mobile sh_flctl driver brush up

[Artem Bityutskiy]

On Fri, 2012-03-09 at 13:14 +0100, Laurent Pinchart wrote:
> ARCH_SHMOBILE is an ARM platform (defined in arch/arm/Kconfig).

Hmm, ok, thanks. So you say that

http://www.renesas.eu/products/soc/assp/mobile/sh_mobile/mt1/index.jsp

saying "CPU core: sh4a" actually means ARM? I must be very confused :-)

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v2 0/7] SH Mobile sh_flctl driver brush up

[Laurent Pinchart]

On Friday 09 March 2012 14:37:12 Artem Bityutskiy wrote:
> On Fri, 2012-03-09 at 13:14 +0100, Laurent Pinchart wrote:
> > ARCH_SHMOBILE is an ARM platform (defined in arch/arm/Kconfig).
> 
> Hmm, ok, thanks. So you say that
> 
> http://www.renesas.eu/products/soc/assp/mobile/sh_mobile/mt1/index.jsp
> 
> saying "CPU core: sh4a" actually means ARM? I must be very confused :-)

Don't trust marketing materiel ;-)

I'm not sure about that exact chip, but the SH-Mobile architecture supported 
by the Linux kernel (and renamed to R-Mobile for recent Renesas products) is 
ARM-based, with an SH-based multimedia coprocessor in most (all ?) models.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/7] SH Mobile sh_flctl driver brush up

[Magnus Damm]

On Fri, Mar 9, 2012 at 9:37 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Fri, 2012-03-09 at 13:14 +0100, Laurent Pinchart wrote:
>> ARCH_SHMOBILE is an ARM platform (defined in arch/arm/Kconfig).
>
> Hmm, ok, thanks. So you say that
>
> http://www.renesas.eu/products/soc/assp/mobile/sh_mobile/mt1/index.jsp
>
> saying "CPU core: sh4a" actually means ARM? I must be very confused :-)

Certain product lines of SoCs from Renesas have both ARM and SH in
them. The ones with only SH are supported under arch/sh and the ARM
based ones under arch/arm/mach-shmobile. Both ARM and SH variants can
be found in anything from vending machines to cars and cellphones.

Regardless of CPU archtecture, endian and availability of MMU they do
share a lot of I/O device hardware IP. The mtd driver for FLCTL is one
such case, if I recall correctly I have seen it in at least 10
different SoCs. Other popular areas that have shared ARM and SH
drivers include mmc, input, network, fbdev, v4l2, i2c and spi.

I believe Bastian is using a sh7372 based Mackerel board which is
fully supported upstream under arch/arm/mach-shmobile. Hope this
helps!

Cheers,

/ magnus