Re: [U-Boot] [PATCH 1/6] nand: denali: add Denali NAND driver for SPL

2014-07-10 Thread Masahiro Yamada
Hi Marek,


On Fri, 4 Jul 2014 16:34:11 +0200
Marek Vasut ma...@denx.de wrote:

 
  +static void read_data_from_flash_mem(uint8_t *buf, int len)
  +{
  +   int i;
  +   uint32_t *buf32;
  +
  +   /* transfer the data from the flash */
  +   buf32 = (uint32_t *)buf;
  +   for (i = 0; i  len / 4; i++)
  +   *buf32++ = readl(denali_flash_mem + INDEX_DATA_REG);
 
 Won't this trigger unaligned access if $buf is not aligned to 4-byte boundary 
 ?


Thanks for catching this.

Actually, it would seldom happen because the aligned load address is generally 
given.

But I will add a sanity check here in v2, just in case.



  +
  +   /* setup the pipeline command */
  +   index_addr(cmd, 0x2000 | page_count);
 
 Magic value 0x2000 should be fixed here.


Actually, this part is the same as that of the normal Denali driver
by Chin Liang See
http://patchwork.ozlabs.org/patch/357717/

Moreover, it is the same as in Linux Kernel.

I understand what you mean, but I guess we can excuse here.




  +   cmd = MODE_01 | addr;
  +   writel(cmd, denali_flash_mem + INDEX_CTRL_REG);
 
 Somehow, eventually, this should be migrated to struct based register 
 access 
 instead of such offset computation. If you feel like doing it, that'd be nice 
 ;-)

Dito.
I am following the coding style of the normal Denali driver and the Linux one.

We might perhaps do that migration as the separated task in future, but not now.



Best Regards
Masahiro Yamada

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/6] nand: denali: add Denali NAND driver for SPL

2014-07-10 Thread Marek Vasut
On Thursday, July 10, 2014 at 12:06:20 PM, Masahiro Yamada wrote:
 Hi Marek,

Hi!

 On Fri, 4 Jul 2014 16:34:11 +0200
 
 Marek Vasut ma...@denx.de wrote:
   +static void read_data_from_flash_mem(uint8_t *buf, int len)
   +{
   + int i;
   + uint32_t *buf32;
   +
   + /* transfer the data from the flash */
   + buf32 = (uint32_t *)buf;
   + for (i = 0; i  len / 4; i++)
   + *buf32++ = readl(denali_flash_mem + INDEX_DATA_REG);
  
  Won't this trigger unaligned access if $buf is not aligned to 4-byte
  boundary ?
 
 Thanks for catching this.
 
 Actually, it would seldom happen because the aligned load address is
 generally given.
 
 But I will add a sanity check here in v2, just in case.

Or just use put_unaligned() ?

   +
   + /* setup the pipeline command */
   + index_addr(cmd, 0x2000 | page_count);
  
  Magic value 0x2000 should be fixed here.
 
 Actually, this part is the same as that of the normal Denali driver
 by Chin Liang See
 http://patchwork.ozlabs.org/patch/357717/
 
 Moreover, it is the same as in Linux Kernel.
 
 I understand what you mean, but I guess we can excuse here.

OK, I see. Is there any chance to fix this in Linux maybe eventually?

[...]

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/6] nand: denali: add Denali NAND driver for SPL

2014-07-10 Thread Masahiro Yamada
Hi Marek,



On Thu, 10 Jul 2014 12:10:25 +0200
Marek Vasut ma...@denx.de wrote:

 
  On Fri, 4 Jul 2014 16:34:11 +0200
  
  Marek Vasut ma...@denx.de wrote:
+static void read_data_from_flash_mem(uint8_t *buf, int len)
+{
+   int i;
+   uint32_t *buf32;
+
+   /* transfer the data from the flash */
+   buf32 = (uint32_t *)buf;
+   for (i = 0; i  len / 4; i++)
+   *buf32++ = readl(denali_flash_mem + INDEX_DATA_REG);
   
   Won't this trigger unaligned access if $buf is not aligned to 4-byte
   boundary ?
  
  Thanks for catching this.
  
  Actually, it would seldom happen because the aligned load address is
  generally given.
  
  But I will add a sanity check here in v2, just in case.
 
 Or just use put_unaligned() ?


Oh, year!  put_unaligned() would be better.
Thanks!


+
+   /* setup the pipeline command */
+   index_addr(cmd, 0x2000 | page_count);
   
   Magic value 0x2000 should be fixed here.
  
  Actually, this part is the same as that of the normal Denali driver
  by Chin Liang See
  http://patchwork.ozlabs.org/patch/357717/
  
  Moreover, it is the same as in Linux Kernel.
  
  I understand what you mean, but I guess we can excuse here.
 
 OK, I see. Is there any chance to fix this in Linux maybe eventually?
 

OK.
I will send a patch to linux-mtd ML
to use  #define PIPELINE_ACCESS2000






Best Regards
Masahiro Yamada

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/6] nand: denali: add Denali NAND driver for SPL

2014-07-10 Thread Marek Vasut
On Thursday, July 10, 2014 at 12:52:19 PM, Masahiro Yamada wrote:
 Hi Marek,

Hi!

[...]

  Or just use put_unaligned() ?
 
 Oh, year!  put_unaligned() would be better.
 Thanks!
 
 +
 + /* setup the pipeline command */
 + index_addr(cmd, 0x2000 | page_count);

Magic value 0x2000 should be fixed here.
   
   Actually, this part is the same as that of the normal Denali driver
   by Chin Liang See
   http://patchwork.ozlabs.org/patch/357717/
   
   Moreover, it is the same as in Linux Kernel.
   
   I understand what you mean, but I guess we can excuse here.
  
  OK, I see. Is there any chance to fix this in Linux maybe eventually?
 
 OK.
 I will send a patch to linux-mtd ML
 to use  #define PIPELINE_ACCESS2000

Ack on both, thank you !

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/6] nand: denali: add Denali NAND driver for SPL

2014-07-10 Thread Masahiro Yamada
Hi Marek,


On Thu, 10 Jul 2014 13:30:38 +0200
Marek Vasut ma...@denx.de wrote:

  +
  +   /* setup the pipeline command */
  +   index_addr(cmd, 0x2000 | page_count);
 
 Magic value 0x2000 should be fixed here.

Actually, this part is the same as that of the normal Denali driver
by Chin Liang See
http://patchwork.ozlabs.org/patch/357717/

Moreover, it is the same as in Linux Kernel.

I understand what you mean, but I guess we can excuse here.
   
   OK, I see. Is there any chance to fix this in Linux maybe eventually?
  
  OK.
  I will send a patch to linux-mtd ML
  to use  #define PIPELINE_ACCESS2000
 
 Ack on both, thank you !


Done.

http://patchwork.ozlabs.org/patch/368933/



Best Regards
Masahiro Yamada

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/6] nand: denali: add Denali NAND driver for SPL

2014-07-04 Thread Marek Vasut
On Friday, July 04, 2014 at 12:19:13 PM, Masahiro Yamada wrote:
 The SPL-mode driver for Denali(Cadence) NAND Flash Memory Controller IP.
 
 This driver requires two CONFIG macros:
  - CONFIG_NAND_DENALI
  Define to enable this driver.
  - CONFIG_SYS_NAND_BAD_BLOCK_POS
  Specify bad block mark position in the oob space. Typically 0.
 
 Signed-off-by: Masahiro Yamada yamad...@jp.panasonic.com
 Cc: Chin Liang See cl...@altera.com
 Cc: Scott Wood scottw...@freescale.com

[...]

 +static void read_data_from_flash_mem(uint8_t *buf, int len)
 +{
 + int i;
 + uint32_t *buf32;
 +
 + /* transfer the data from the flash */
 + buf32 = (uint32_t *)buf;
 + for (i = 0; i  len / 4; i++)
 + *buf32++ = readl(denali_flash_mem + INDEX_DATA_REG);

Won't this trigger unaligned access if $buf is not aligned to 4-byte boundary ?

 +}
 +
 +int denali_send_pipeline_cmd(int page, int ecc_en, int access_type)
 +{
 + uint32_t addr, cmd;
 + static uint32_t page_count = 1;
 +
 + writel(ecc_en, denali_flash_reg + ECC_ENABLE);
 +
 + /* clear all bits of intr_status. */
 + writel(0x, denali_flash_reg + INTR_STATUS(flash_bank));
 +
 + addr = BANK(flash_bank) | page;
 +
 + /* setup the acccess type */
 + cmd = MODE_10 | addr;
 + index_addr(cmd, access_type);
 +
 + /* setup the pipeline command */
 + index_addr(cmd, 0x2000 | page_count);

Magic value 0x2000 should be fixed here.

 + cmd = MODE_01 | addr;
 + writel(cmd, denali_flash_mem + INDEX_CTRL_REG);

Somehow, eventually, this should be migrated to struct based register access 
instead of such offset computation. If you feel like doing it, that'd be nice 
;-)

 + return wait_for_irq(INTR_STATUS__LOAD_COMP);
 +}
[...]

Thanks!
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot