Patchwork [2/4] mtd: bcm47xxnflash: add dev_ready and fill chip_delay

login
register
mail settings
Submitter Rafał Miłecki
Date Aug. 19, 2014, 7:14 a.m.
Message ID <1408432456-27280-2-git-send-email-zajec5@gmail.com>
Download mbox | patch
Permalink /patch/381256/
State Accepted
Commit 5282a3acbfa5295f331696e603a9fd6be3bd4094
Headers show

Comments

Rafał Miłecki - Aug. 19, 2014, 7:14 a.m.
Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Brian Norris - Sept. 18, 2014, 6:27 a.m.
On Tue, Aug 19, 2014 at 09:14:14AM +0200, Rafał Miłecki wrote:
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>  drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c
> index dc204f3..1ea5e77 100644
> --- a/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c
> +++ b/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c
> @@ -174,6 +174,14 @@ static void bcm47xxnflash_ops_bcm4706_select_chip(struct mtd_info *mtd,
>  	return;
>  }
>  
> +static int bcm47xxnflash_ops_bcm4706_dev_ready(struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand_chip = (struct nand_chip *)mtd->priv;
> +	struct bcm47xxnflash *b47n = (struct bcm47xxnflash *)nand_chip->priv;
> +
> +	return !!(bcma_cc_read32(b47n->cc, BCMA_CC_NFLASH_CTL) & NCTL_READY);

BTW, you don't actually need the double negation, since
chip->dev_ready() is always used as a bool anyway. But no matter.

> +}
> +
>  /*
>   * Default nand_command and nand_command_lp don't match BCM4706 hardware layout.
>   * For example, reading chip id is performed in a non-standard way.
> @@ -341,6 +349,7 @@ static void bcm47xxnflash_ops_bcm4706_write_buf(struct mtd_info *mtd,
>  
>  int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
>  {
> +	struct nand_chip *nand_chip = (struct nand_chip *)&b47n->nand_chip;
>  	int err;
>  	u32 freq;
>  	u16 clock;
> @@ -351,10 +360,13 @@ int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
>  	u32 val;
>  
>  	b47n->nand_chip.select_chip = bcm47xxnflash_ops_bcm4706_select_chip;
> +	nand_chip->dev_ready = bcm47xxnflash_ops_bcm4706_dev_ready;
>  	b47n->nand_chip.cmdfunc = bcm47xxnflash_ops_bcm4706_cmdfunc;
>  	b47n->nand_chip.read_byte = bcm47xxnflash_ops_bcm4706_read_byte;
>  	b47n->nand_chip.read_buf = bcm47xxnflash_ops_bcm4706_read_buf;
>  	b47n->nand_chip.write_buf = bcm47xxnflash_ops_bcm4706_write_buf;
> +
> +	nand_chip->chip_delay = 50;

I'm curious, did you have a good reason to use something other than the
default provided by nand_base?

>  	b47n->nand_chip.bbt_options = NAND_BBT_USE_FLASH;
>  	b47n->nand_chip.ecc.mode = NAND_ECC_NONE; /* TODO: implement ECC */
>  

Brian
Rafał Miłecki - Sept. 18, 2014, 6:43 a.m.
On 18 September 2014 08:27, Brian Norris <computersforpeace@gmail.com> wrote:
> On Tue, Aug 19, 2014 at 09:14:14AM +0200, Rafał Miłecki wrote:
>> +
>> +     nand_chip->chip_delay = 50;
>
> I'm curious, did you have a good reason to use something other than the
> default provided by nand_base?

I simply followed what Broadcom used in their ugly code (don't look to
deep into that file, or you'll go crazy):
http://svn.dd-wrt.com/browser/src/linux/universal/linux-3.8/drivers/mtd/bcm947xx/nand/brcmnand_47xx.c?rev=22249#L2240

They set "dev_ready" as well as "chip_delay" (to the 50), so the only
case where they use this delay is
NAND_CMD_STATUS_ERROR
NAND_CMD_STATUS_ERROR[0-3]

Do you think we should try to stay with the default value?
Brian Norris - Sept. 18, 2014, 6:54 a.m.
On Wed, Sep 17, 2014 at 11:43 PM, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 18 September 2014 08:27, Brian Norris <computersforpeace@gmail.com> wrote:
>> On Tue, Aug 19, 2014 at 09:14:14AM +0200, Rafał Miłecki wrote:
>>> +
>>> +     nand_chip->chip_delay = 50;
>>
>> I'm curious, did you have a good reason to use something other than the
>> default provided by nand_base?
>
> I simply followed what Broadcom used in their ugly code (don't look to
> deep into that file, or you'll go crazy):
> http://svn.dd-wrt.com/browser/src/linux/universal/linux-3.8/drivers/mtd/bcm947xx/nand/brcmnand_47xx.c?rev=22249#L2240
>
> They set "dev_ready" as well as "chip_delay" (to the 50), so the only
> case where they use this delay is
> NAND_CMD_STATUS_ERROR
> NAND_CMD_STATUS_ERROR[0-3]
>
> Do you think we should try to stay with the default value?

No, it's fine as-is.

Brian

Patch

diff --git a/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c
index dc204f3..1ea5e77 100644
--- a/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c
+++ b/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c
@@ -174,6 +174,14 @@  static void bcm47xxnflash_ops_bcm4706_select_chip(struct mtd_info *mtd,
 	return;
 }
 
+static int bcm47xxnflash_ops_bcm4706_dev_ready(struct mtd_info *mtd)
+{
+	struct nand_chip *nand_chip = (struct nand_chip *)mtd->priv;
+	struct bcm47xxnflash *b47n = (struct bcm47xxnflash *)nand_chip->priv;
+
+	return !!(bcma_cc_read32(b47n->cc, BCMA_CC_NFLASH_CTL) & NCTL_READY);
+}
+
 /*
  * Default nand_command and nand_command_lp don't match BCM4706 hardware layout.
  * For example, reading chip id is performed in a non-standard way.
@@ -341,6 +349,7 @@  static void bcm47xxnflash_ops_bcm4706_write_buf(struct mtd_info *mtd,
 
 int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
 {
+	struct nand_chip *nand_chip = (struct nand_chip *)&b47n->nand_chip;
 	int err;
 	u32 freq;
 	u16 clock;
@@ -351,10 +360,13 @@  int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
 	u32 val;
 
 	b47n->nand_chip.select_chip = bcm47xxnflash_ops_bcm4706_select_chip;
+	nand_chip->dev_ready = bcm47xxnflash_ops_bcm4706_dev_ready;
 	b47n->nand_chip.cmdfunc = bcm47xxnflash_ops_bcm4706_cmdfunc;
 	b47n->nand_chip.read_byte = bcm47xxnflash_ops_bcm4706_read_byte;
 	b47n->nand_chip.read_buf = bcm47xxnflash_ops_bcm4706_read_buf;
 	b47n->nand_chip.write_buf = bcm47xxnflash_ops_bcm4706_write_buf;
+
+	nand_chip->chip_delay = 50;
 	b47n->nand_chip.bbt_options = NAND_BBT_USE_FLASH;
 	b47n->nand_chip.ecc.mode = NAND_ECC_NONE; /* TODO: implement ECC */