* Re: [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program
2017-03-03 21:16 ` [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program Kamal Dasu
@ 2017-03-10 13:22 ` Marek Vasut
2017-03-10 17:46 ` Florian Fainelli
2017-03-15 14:01 ` Boris Brezillon
2017-03-15 19:55 ` Boris Brezillon
2017-04-11 19:41 ` Boris Brezillon
2 siblings, 2 replies; 10+ messages in thread
From: Marek Vasut @ 2017-03-10 13:22 UTC (permalink / raw)
To: Kamal Dasu, linux-mtd
Cc: f.fainelli, bcm-kernel-feedback-list, dwmw2, computersforpeace,
boris.brezillon, richard, cyrille.pitchen
On 03/03/2017 10:16 PM, Kamal Dasu wrote:
> On brcmnand controller v6.x and v7.x, the #WP pin is controlled through
> the NAND_WP bit in CS_SELECT register.
>
> The driver currently assumes that toggling the #WP pin is
> instantaneously enabling/disabling write-protection, but it actually
> takes some time to propagate the new state to the internal NAND chip
> logic. This behavior is sometime causing data corruptions when an
> erase/program operation is executed before write-protection has really
> been disabled.
>
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
> drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index 42ebd73..7419c5c 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
> #define BRCMNAND_MIN_BLOCKSIZE (8 * 1024)
> #define BRCMNAND_MIN_DEVSIZE (4ULL * 1024 * 1024)
>
> +#define NAND_CTRL_RDY (INTFC_CTLR_READY | INTFC_FLASH_READY)
> +#define NAND_POLL_STATUS_TIMEOUT_MS 100
> +
> /* Controller feature flags */
> enum {
> BRCMNAND_HAS_1K_SECTORS = BIT(0),
> @@ -765,6 +768,31 @@ enum {
> CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30),
> };
>
> +static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
> + u32 mask, u32 expected_val,
> + unsigned long timeout_ms)
> +{
> + unsigned long limit;
> + u32 val;
> +
> + if (!timeout_ms)
> + timeout_ms = NAND_POLL_STATUS_TIMEOUT_MS;
> +
> + limit = jiffies + msecs_to_jiffies(timeout_ms);
> + do {
> + val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> + if ((val & mask) == expected_val)
> + return 0;
> +
> + cpu_relax();
> + } while (time_after(limit, jiffies));
> +
> + dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
> + expected_val, val & mask);
> +
> + return -ETIMEDOUT;
> +}
> +
> static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
> {
> u32 val = en ? CS_SELECT_NAND_WP : 0;
> @@ -1024,12 +1052,39 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
>
> if ((ctrl->features & BRCMNAND_HAS_WP) && wp_on == 1) {
> static int old_wp = -1;
Unrelated to this patch, but this static variable should be moved to
driver's private data instead.
> + int ret;
>
> if (old_wp != wp) {
> dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off");
> old_wp = wp;
> }
> +
> + /*
> + * make sure ctrl/flash ready before and after
> + * changing state of #WP pin
> + */
Shouldn't the brcmnand_set_wp() do this ?
> + ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY |
> + NAND_STATUS_READY,
> + NAND_CTRL_RDY |
> + NAND_STATUS_READY, 0);
> + if (ret)
> + return;
> +
> brcmnand_set_wp(ctrl, wp);
> + chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> + /* NAND_STATUS_WP 0x00 = protected, 0x80 = not protected */
> + ret = bcmnand_ctrl_poll_status(ctrl,
> + NAND_CTRL_RDY |
> + NAND_STATUS_READY |
> + NAND_STATUS_WP,
> + NAND_CTRL_RDY |
> + NAND_STATUS_READY |
> + (wp ? 0 : NAND_STATUS_WP), 0);
> +
> + if (ret)
> + dev_err_ratelimited(&host->pdev->dev,
> + "nand #WP expected %s\n",
> + wp ? "on" : "off");
> }
> }
>
> @@ -1157,15 +1212,15 @@ static irqreturn_t brcmnand_dma_irq(int irq, void *data)
> static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
> {
> struct brcmnand_controller *ctrl = host->ctrl;
> - u32 intfc;
> + int ret;
>
> dev_dbg(ctrl->dev, "send native cmd %d addr_lo 0x%x\n", cmd,
> brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS));
> BUG_ON(ctrl->cmd_pending != 0);
> ctrl->cmd_pending = cmd;
>
> - intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> - WARN_ON(!(intfc & INTFC_CTLR_READY));
> + ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> + WARN_ON(ret);
>
> mb(); /* flush previous writes */
> brcmnand_write_reg(ctrl, BRCMNAND_CMD_START,
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program
2017-03-10 13:22 ` Marek Vasut
@ 2017-03-10 17:46 ` Florian Fainelli
2017-03-10 18:37 ` Marek Vasut
2017-03-15 14:01 ` Boris Brezillon
1 sibling, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2017-03-10 17:46 UTC (permalink / raw)
To: Marek Vasut, Kamal Dasu, linux-mtd
Cc: f.fainelli, bcm-kernel-feedback-list, dwmw2, computersforpeace,
boris.brezillon, richard, cyrille.pitchen
On 03/10/2017 05:22 AM, Marek Vasut wrote:
> On 03/03/2017 10:16 PM, Kamal Dasu wrote:
>> On brcmnand controller v6.x and v7.x, the #WP pin is controlled through
>> the NAND_WP bit in CS_SELECT register.
>>
>> The driver currently assumes that toggling the #WP pin is
>> instantaneously enabling/disabling write-protection, but it actually
>> takes some time to propagate the new state to the internal NAND chip
>> logic. This behavior is sometime causing data corruptions when an
>> erase/program operation is executed before write-protection has really
>> been disabled.
>>
>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>> ---
>> drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++--
>> 1 file changed, 58 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
>> index 42ebd73..7419c5c 100644
>> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
>> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
>> #define BRCMNAND_MIN_BLOCKSIZE (8 * 1024)
>> #define BRCMNAND_MIN_DEVSIZE (4ULL * 1024 * 1024)
>>
>> +#define NAND_CTRL_RDY (INTFC_CTLR_READY | INTFC_FLASH_READY)
>> +#define NAND_POLL_STATUS_TIMEOUT_MS 100
>> +
>> /* Controller feature flags */
>> enum {
>> BRCMNAND_HAS_1K_SECTORS = BIT(0),
>> @@ -765,6 +768,31 @@ enum {
>> CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30),
>> };
>>
>> +static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
>> + u32 mask, u32 expected_val,
>> + unsigned long timeout_ms)
>> +{
>> + unsigned long limit;
>> + u32 val;
>> +
>> + if (!timeout_ms)
>> + timeout_ms = NAND_POLL_STATUS_TIMEOUT_MS;
>> +
>> + limit = jiffies + msecs_to_jiffies(timeout_ms);
>> + do {
>> + val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
>> + if ((val & mask) == expected_val)
>> + return 0;
>> +
>> + cpu_relax();
>> + } while (time_after(limit, jiffies));
>> +
>> + dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
>> + expected_val, val & mask);
>> +
>> + return -ETIMEDOUT;
>> +}
>> +
>> static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
>> {
>> u32 val = en ? CS_SELECT_NAND_WP : 0;
>> @@ -1024,12 +1052,39 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
>>
>> if ((ctrl->features & BRCMNAND_HAS_WP) && wp_on == 1) {
>> static int old_wp = -1;
>
> Unrelated to this patch, but this static variable should be moved to
> driver's private data instead.
Does that mean you are okay with this patch as-is as a fix which can be
backported and code refactoring can be submitted as follow up patches?
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program
2017-03-10 17:46 ` Florian Fainelli
@ 2017-03-10 18:37 ` Marek Vasut
0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2017-03-10 18:37 UTC (permalink / raw)
To: Florian Fainelli, Kamal Dasu, linux-mtd
Cc: bcm-kernel-feedback-list, dwmw2, computersforpeace,
boris.brezillon, richard, cyrille.pitchen
On 03/10/2017 06:46 PM, Florian Fainelli wrote:
> On 03/10/2017 05:22 AM, Marek Vasut wrote:
>> On 03/03/2017 10:16 PM, Kamal Dasu wrote:
>>> On brcmnand controller v6.x and v7.x, the #WP pin is controlled through
>>> the NAND_WP bit in CS_SELECT register.
>>>
>>> The driver currently assumes that toggling the #WP pin is
>>> instantaneously enabling/disabling write-protection, but it actually
>>> takes some time to propagate the new state to the internal NAND chip
>>> logic. This behavior is sometime causing data corruptions when an
>>> erase/program operation is executed before write-protection has really
>>> been disabled.
>>>
>>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
>>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>>> ---
>>> drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 58 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
>>> index 42ebd73..7419c5c 100644
>>> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
>>> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
>>> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
>>> #define BRCMNAND_MIN_BLOCKSIZE (8 * 1024)
>>> #define BRCMNAND_MIN_DEVSIZE (4ULL * 1024 * 1024)
>>>
>>> +#define NAND_CTRL_RDY (INTFC_CTLR_READY | INTFC_FLASH_READY)
>>> +#define NAND_POLL_STATUS_TIMEOUT_MS 100
>>> +
>>> /* Controller feature flags */
>>> enum {
>>> BRCMNAND_HAS_1K_SECTORS = BIT(0),
>>> @@ -765,6 +768,31 @@ enum {
>>> CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30),
>>> };
>>>
>>> +static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
>>> + u32 mask, u32 expected_val,
>>> + unsigned long timeout_ms)
>>> +{
>>> + unsigned long limit;
>>> + u32 val;
>>> +
>>> + if (!timeout_ms)
>>> + timeout_ms = NAND_POLL_STATUS_TIMEOUT_MS;
>>> +
>>> + limit = jiffies + msecs_to_jiffies(timeout_ms);
>>> + do {
>>> + val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
>>> + if ((val & mask) == expected_val)
>>> + return 0;
>>> +
>>> + cpu_relax();
>>> + } while (time_after(limit, jiffies));
>>> +
>>> + dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
>>> + expected_val, val & mask);
>>> +
>>> + return -ETIMEDOUT;
>>> +}
>>> +
>>> static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
>>> {
>>> u32 val = en ? CS_SELECT_NAND_WP : 0;
>>> @@ -1024,12 +1052,39 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
>>>
>>> if ((ctrl->features & BRCMNAND_HAS_WP) && wp_on == 1) {
>>> static int old_wp = -1;
>>
>> Unrelated to this patch, but this static variable should be moved to
>> driver's private data instead.
>
> Does that mean you are okay with this patch as-is as a fix which can be
> backported and code refactoring can be submitted as follow up patches?
>
No, this means the static variable thing above should be fixed in a
separate patch, that is all I mean.
It's up to Boris to decide about this patch as he does NAND .
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program
2017-03-10 13:22 ` Marek Vasut
2017-03-10 17:46 ` Florian Fainelli
@ 2017-03-15 14:01 ` Boris Brezillon
2017-03-15 16:26 ` Florian Fainelli
1 sibling, 1 reply; 10+ messages in thread
From: Boris Brezillon @ 2017-03-15 14:01 UTC (permalink / raw)
To: Marek Vasut
Cc: Kamal Dasu, linux-mtd, f.fainelli, bcm-kernel-feedback-list,
dwmw2, computersforpeace, richard, cyrille.pitchen
On Fri, 10 Mar 2017 14:22:39 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:
> On 03/03/2017 10:16 PM, Kamal Dasu wrote:
> > On brcmnand controller v6.x and v7.x, the #WP pin is controlled through
> > the NAND_WP bit in CS_SELECT register.
> >
> > The driver currently assumes that toggling the #WP pin is
> > instantaneously enabling/disabling write-protection, but it actually
> > takes some time to propagate the new state to the internal NAND chip
> > logic. This behavior is sometime causing data corruptions when an
> > erase/program operation is executed before write-protection has really
> > been disabled.
> >
> > Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> > ---
> > drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++--
> > 1 file changed, 58 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> > index 42ebd73..7419c5c 100644
> > --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> > @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
> > #define BRCMNAND_MIN_BLOCKSIZE (8 * 1024)
> > #define BRCMNAND_MIN_DEVSIZE (4ULL * 1024 * 1024)
> >
> > +#define NAND_CTRL_RDY (INTFC_CTLR_READY | INTFC_FLASH_READY)
> > +#define NAND_POLL_STATUS_TIMEOUT_MS 100
> > +
> > /* Controller feature flags */
> > enum {
> > BRCMNAND_HAS_1K_SECTORS = BIT(0),
> > @@ -765,6 +768,31 @@ enum {
> > CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30),
> > };
> >
> > +static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
> > + u32 mask, u32 expected_val,
> > + unsigned long timeout_ms)
> > +{
> > + unsigned long limit;
> > + u32 val;
> > +
> > + if (!timeout_ms)
> > + timeout_ms = NAND_POLL_STATUS_TIMEOUT_MS;
> > +
> > + limit = jiffies + msecs_to_jiffies(timeout_ms);
> > + do {
> > + val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> > + if ((val & mask) == expected_val)
> > + return 0;
> > +
> > + cpu_relax();
> > + } while (time_after(limit, jiffies));
> > +
> > + dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
> > + expected_val, val & mask);
> > +
> > + return -ETIMEDOUT;
> > +}
> > +
> > static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
> > {
> > u32 val = en ? CS_SELECT_NAND_WP : 0;
> > @@ -1024,12 +1052,39 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
> >
> > if ((ctrl->features & BRCMNAND_HAS_WP) && wp_on == 1) {
> > static int old_wp = -1;
>
> Unrelated to this patch, but this static variable should be moved to
> driver's private data instead.
>
> > + int ret;
> >
> > if (old_wp != wp) {
> > dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off");
> > old_wp = wp;
> > }
> > +
> > + /*
> > + * make sure ctrl/flash ready before and after
> > + * changing state of #WP pin
> > + */
>
> Shouldn't the brcmnand_set_wp() do this ?
Hm, AFAIU, brcmnand_set_wp() is only controlling the WP pin from the
controller side, so maybe we should rename the function
brcmnand_ctrl_set_wp() to clarify that.
>
> > + ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY |
> > + NAND_STATUS_READY,
> > + NAND_CTRL_RDY |
> > + NAND_STATUS_READY, 0);
> > + if (ret)
> > + return;
> > +
> > brcmnand_set_wp(ctrl, wp);
> > + chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> > + /* NAND_STATUS_WP 0x00 = protected, 0x80 = not protected */
> > + ret = bcmnand_ctrl_poll_status(ctrl,
> > + NAND_CTRL_RDY |
> > + NAND_STATUS_READY |
> > + NAND_STATUS_WP,
> > + NAND_CTRL_RDY |
> > + NAND_STATUS_READY |
> > + (wp ? 0 : NAND_STATUS_WP), 0);
> > +
> > + if (ret)
> > + dev_err_ratelimited(&host->pdev->dev,
> > + "nand #WP expected %s\n",
> > + wp ? "on" : "off");
> > }
> > }
> >
> > @@ -1157,15 +1212,15 @@ static irqreturn_t brcmnand_dma_irq(int irq, void *data)
> > static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
> > {
> > struct brcmnand_controller *ctrl = host->ctrl;
> > - u32 intfc;
> > + int ret;
> >
> > dev_dbg(ctrl->dev, "send native cmd %d addr_lo 0x%x\n", cmd,
> > brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS));
> > BUG_ON(ctrl->cmd_pending != 0);
> > ctrl->cmd_pending = cmd;
> >
> > - intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> > - WARN_ON(!(intfc & INTFC_CTLR_READY));
> > + ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> > + WARN_ON(ret);
> >
> > mb(); /* flush previous writes */
> > brcmnand_write_reg(ctrl, BRCMNAND_CMD_START,
> >
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program
2017-03-15 14:01 ` Boris Brezillon
@ 2017-03-15 16:26 ` Florian Fainelli
2017-03-16 22:33 ` Marek Vasut
0 siblings, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2017-03-15 16:26 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut
Cc: Kamal Dasu, linux-mtd, f.fainelli, bcm-kernel-feedback-list,
dwmw2, computersforpeace, richard, cyrille.pitchen
On 03/15/2017 07:01 AM, Boris Brezillon wrote:
> On Fri, 10 Mar 2017 14:22:39 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
>
>> On 03/03/2017 10:16 PM, Kamal Dasu wrote:
>>> On brcmnand controller v6.x and v7.x, the #WP pin is controlled through
>>> the NAND_WP bit in CS_SELECT register.
>>>
>>> The driver currently assumes that toggling the #WP pin is
>>> instantaneously enabling/disabling write-protection, but it actually
>>> takes some time to propagate the new state to the internal NAND chip
>>> logic. This behavior is sometime causing data corruptions when an
>>> erase/program operation is executed before write-protection has really
>>> been disabled.
>>>
>>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
>>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>>> ---
>>> drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 58 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
>>> index 42ebd73..7419c5c 100644
>>> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
>>> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
>>> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
>>> #define BRCMNAND_MIN_BLOCKSIZE (8 * 1024)
>>> #define BRCMNAND_MIN_DEVSIZE (4ULL * 1024 * 1024)
>>>
>>> +#define NAND_CTRL_RDY (INTFC_CTLR_READY | INTFC_FLASH_READY)
>>> +#define NAND_POLL_STATUS_TIMEOUT_MS 100
>>> +
>>> /* Controller feature flags */
>>> enum {
>>> BRCMNAND_HAS_1K_SECTORS = BIT(0),
>>> @@ -765,6 +768,31 @@ enum {
>>> CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30),
>>> };
>>>
>>> +static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
>>> + u32 mask, u32 expected_val,
>>> + unsigned long timeout_ms)
>>> +{
>>> + unsigned long limit;
>>> + u32 val;
>>> +
>>> + if (!timeout_ms)
>>> + timeout_ms = NAND_POLL_STATUS_TIMEOUT_MS;
>>> +
>>> + limit = jiffies + msecs_to_jiffies(timeout_ms);
>>> + do {
>>> + val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
>>> + if ((val & mask) == expected_val)
>>> + return 0;
>>> +
>>> + cpu_relax();
>>> + } while (time_after(limit, jiffies));
>>> +
>>> + dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
>>> + expected_val, val & mask);
>>> +
>>> + return -ETIMEDOUT;
>>> +}
>>> +
>>> static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
>>> {
>>> u32 val = en ? CS_SELECT_NAND_WP : 0;
>>> @@ -1024,12 +1052,39 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
>>>
>>> if ((ctrl->features & BRCMNAND_HAS_WP) && wp_on == 1) {
>>> static int old_wp = -1;
>>
>> Unrelated to this patch, but this static variable should be moved to
>> driver's private data instead.
>>
>>> + int ret;
>>>
>>> if (old_wp != wp) {
>>> dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off");
>>> old_wp = wp;
>>> }
>>> +
>>> + /*
>>> + * make sure ctrl/flash ready before and after
>>> + * changing state of #WP pin
>>> + */
>>
>> Shouldn't the brcmnand_set_wp() do this ?
>
> Hm, AFAIU, brcmnand_set_wp() is only controlling the WP pin from the
> controller side, so maybe we should rename the function
> brcmnand_ctrl_set_wp() to clarify that.
While I don't object that this change should be made, we are already at
version 6 here, and this is a bugfix, so what else needs to be done to
get this included?
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program
2017-03-15 16:26 ` Florian Fainelli
@ 2017-03-16 22:33 ` Marek Vasut
0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2017-03-16 22:33 UTC (permalink / raw)
To: Florian Fainelli, Boris Brezillon
Cc: Kamal Dasu, linux-mtd, f.fainelli, bcm-kernel-feedback-list,
dwmw2, computersforpeace, richard, cyrille.pitchen
On 03/15/2017 05:26 PM, Florian Fainelli wrote:
>
>
> On 03/15/2017 07:01 AM, Boris Brezillon wrote:
>> On Fri, 10 Mar 2017 14:22:39 +0100
>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>>> On 03/03/2017 10:16 PM, Kamal Dasu wrote:
>>>> On brcmnand controller v6.x and v7.x, the #WP pin is controlled through
>>>> the NAND_WP bit in CS_SELECT register.
>>>>
>>>> The driver currently assumes that toggling the #WP pin is
>>>> instantaneously enabling/disabling write-protection, but it actually
>>>> takes some time to propagate the new state to the internal NAND chip
>>>> logic. This behavior is sometime causing data corruptions when an
>>>> erase/program operation is executed before write-protection has really
>>>> been disabled.
>>>>
>>>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
>>>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>>>> ---
>>>> drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 58 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
>>>> index 42ebd73..7419c5c 100644
>>>> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
>>>> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
>>>> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
>>>> #define BRCMNAND_MIN_BLOCKSIZE (8 * 1024)
>>>> #define BRCMNAND_MIN_DEVSIZE (4ULL * 1024 * 1024)
>>>>
>>>> +#define NAND_CTRL_RDY (INTFC_CTLR_READY | INTFC_FLASH_READY)
>>>> +#define NAND_POLL_STATUS_TIMEOUT_MS 100
>>>> +
>>>> /* Controller feature flags */
>>>> enum {
>>>> BRCMNAND_HAS_1K_SECTORS = BIT(0),
>>>> @@ -765,6 +768,31 @@ enum {
>>>> CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30),
>>>> };
>>>>
>>>> +static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
>>>> + u32 mask, u32 expected_val,
>>>> + unsigned long timeout_ms)
>>>> +{
>>>> + unsigned long limit;
>>>> + u32 val;
>>>> +
>>>> + if (!timeout_ms)
>>>> + timeout_ms = NAND_POLL_STATUS_TIMEOUT_MS;
>>>> +
>>>> + limit = jiffies + msecs_to_jiffies(timeout_ms);
>>>> + do {
>>>> + val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
>>>> + if ((val & mask) == expected_val)
>>>> + return 0;
>>>> +
>>>> + cpu_relax();
>>>> + } while (time_after(limit, jiffies));
>>>> +
>>>> + dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
>>>> + expected_val, val & mask);
>>>> +
>>>> + return -ETIMEDOUT;
>>>> +}
>>>> +
>>>> static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
>>>> {
>>>> u32 val = en ? CS_SELECT_NAND_WP : 0;
>>>> @@ -1024,12 +1052,39 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
>>>>
>>>> if ((ctrl->features & BRCMNAND_HAS_WP) && wp_on == 1) {
>>>> static int old_wp = -1;
>>>
>>> Unrelated to this patch, but this static variable should be moved to
>>> driver's private data instead.
>>>
>>>> + int ret;
>>>>
>>>> if (old_wp != wp) {
>>>> dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off");
>>>> old_wp = wp;
>>>> }
>>>> +
>>>> + /*
>>>> + * make sure ctrl/flash ready before and after
>>>> + * changing state of #WP pin
>>>> + */
>>>
>>> Shouldn't the brcmnand_set_wp() do this ?
>>
>> Hm, AFAIU, brcmnand_set_wp() is only controlling the WP pin from the
>> controller side, so maybe we should rename the function
>> brcmnand_ctrl_set_wp() to clarify that.
>
> While I don't object that this change should be made, we are already at
> version 6 here, and this is a bugfix, so what else needs to be done to
> get this included?
>
Follow the maintainers' advice , just like everyone else , just like we
did it ever since ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program
2017-03-03 21:16 ` [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program Kamal Dasu
2017-03-10 13:22 ` Marek Vasut
@ 2017-03-15 19:55 ` Boris Brezillon
2017-04-11 19:41 ` Boris Brezillon
2 siblings, 0 replies; 10+ messages in thread
From: Boris Brezillon @ 2017-03-15 19:55 UTC (permalink / raw)
To: Kamal Dasu, computersforpeace
Cc: linux-mtd, f.fainelli, bcm-kernel-feedback-list, dwmw2,
marek.vasut, richard, cyrille.pitchen
On Fri, 3 Mar 2017 16:16:53 -0500
Kamal Dasu <kdasu.kdev@gmail.com> wrote:
> On brcmnand controller v6.x and v7.x, the #WP pin is controlled through
> the NAND_WP bit in CS_SELECT register.
>
> The driver currently assumes that toggling the #WP pin is
> instantaneously enabling/disabling write-protection, but it actually
> takes some time to propagate the new state to the internal NAND chip
> logic. This behavior is sometime causing data corruptions when an
> erase/program operation is executed before write-protection has really
> been disabled.
>
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Brian, can you queue this patch for 4.11-rc3?
Thanks,
Boris
> ---
> drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index 42ebd73..7419c5c 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
> #define BRCMNAND_MIN_BLOCKSIZE (8 * 1024)
> #define BRCMNAND_MIN_DEVSIZE (4ULL * 1024 * 1024)
>
> +#define NAND_CTRL_RDY (INTFC_CTLR_READY | INTFC_FLASH_READY)
> +#define NAND_POLL_STATUS_TIMEOUT_MS 100
> +
> /* Controller feature flags */
> enum {
> BRCMNAND_HAS_1K_SECTORS = BIT(0),
> @@ -765,6 +768,31 @@ enum {
> CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30),
> };
>
> +static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
> + u32 mask, u32 expected_val,
> + unsigned long timeout_ms)
> +{
> + unsigned long limit;
> + u32 val;
> +
> + if (!timeout_ms)
> + timeout_ms = NAND_POLL_STATUS_TIMEOUT_MS;
> +
> + limit = jiffies + msecs_to_jiffies(timeout_ms);
> + do {
> + val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> + if ((val & mask) == expected_val)
> + return 0;
> +
> + cpu_relax();
> + } while (time_after(limit, jiffies));
> +
> + dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
> + expected_val, val & mask);
> +
> + return -ETIMEDOUT;
> +}
> +
> static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
> {
> u32 val = en ? CS_SELECT_NAND_WP : 0;
> @@ -1024,12 +1052,39 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
>
> if ((ctrl->features & BRCMNAND_HAS_WP) && wp_on == 1) {
> static int old_wp = -1;
> + int ret;
>
> if (old_wp != wp) {
> dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off");
> old_wp = wp;
> }
> +
> + /*
> + * make sure ctrl/flash ready before and after
> + * changing state of #WP pin
> + */
> + ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY |
> + NAND_STATUS_READY,
> + NAND_CTRL_RDY |
> + NAND_STATUS_READY, 0);
> + if (ret)
> + return;
> +
> brcmnand_set_wp(ctrl, wp);
> + chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> + /* NAND_STATUS_WP 0x00 = protected, 0x80 = not protected */
> + ret = bcmnand_ctrl_poll_status(ctrl,
> + NAND_CTRL_RDY |
> + NAND_STATUS_READY |
> + NAND_STATUS_WP,
> + NAND_CTRL_RDY |
> + NAND_STATUS_READY |
> + (wp ? 0 : NAND_STATUS_WP), 0);
> +
> + if (ret)
> + dev_err_ratelimited(&host->pdev->dev,
> + "nand #WP expected %s\n",
> + wp ? "on" : "off");
> }
> }
>
> @@ -1157,15 +1212,15 @@ static irqreturn_t brcmnand_dma_irq(int irq, void *data)
> static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
> {
> struct brcmnand_controller *ctrl = host->ctrl;
> - u32 intfc;
> + int ret;
>
> dev_dbg(ctrl->dev, "send native cmd %d addr_lo 0x%x\n", cmd,
> brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS));
> BUG_ON(ctrl->cmd_pending != 0);
> ctrl->cmd_pending = cmd;
>
> - intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> - WARN_ON(!(intfc & INTFC_CTLR_READY));
> + ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> + WARN_ON(ret);
>
> mb(); /* flush previous writes */
> brcmnand_write_reg(ctrl, BRCMNAND_CMD_START,
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program
2017-03-03 21:16 ` [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program Kamal Dasu
2017-03-10 13:22 ` Marek Vasut
2017-03-15 19:55 ` Boris Brezillon
@ 2017-04-11 19:41 ` Boris Brezillon
2 siblings, 0 replies; 10+ messages in thread
From: Boris Brezillon @ 2017-04-11 19:41 UTC (permalink / raw)
To: Kamal Dasu
Cc: linux-mtd, f.fainelli, bcm-kernel-feedback-list, dwmw2,
computersforpeace, marek.vasut, richard, cyrille.pitchen
On Fri, 3 Mar 2017 16:16:53 -0500
Kamal Dasu <kdasu.kdev@gmail.com> wrote:
> On brcmnand controller v6.x and v7.x, the #WP pin is controlled through
> the NAND_WP bit in CS_SELECT register.
>
> The driver currently assumes that toggling the #WP pin is
> instantaneously enabling/disabling write-protection, but it actually
> takes some time to propagate the new state to the internal NAND chip
> logic. This behavior is sometime causing data corruptions when an
> erase/program operation is executed before write-protection has really
> been disabled.
Applied.
Thanks,
Boris
>
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
> drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index 42ebd73..7419c5c 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
> #define BRCMNAND_MIN_BLOCKSIZE (8 * 1024)
> #define BRCMNAND_MIN_DEVSIZE (4ULL * 1024 * 1024)
>
> +#define NAND_CTRL_RDY (INTFC_CTLR_READY | INTFC_FLASH_READY)
> +#define NAND_POLL_STATUS_TIMEOUT_MS 100
> +
> /* Controller feature flags */
> enum {
> BRCMNAND_HAS_1K_SECTORS = BIT(0),
> @@ -765,6 +768,31 @@ enum {
> CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30),
> };
>
> +static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
> + u32 mask, u32 expected_val,
> + unsigned long timeout_ms)
> +{
> + unsigned long limit;
> + u32 val;
> +
> + if (!timeout_ms)
> + timeout_ms = NAND_POLL_STATUS_TIMEOUT_MS;
> +
> + limit = jiffies + msecs_to_jiffies(timeout_ms);
> + do {
> + val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> + if ((val & mask) == expected_val)
> + return 0;
> +
> + cpu_relax();
> + } while (time_after(limit, jiffies));
> +
> + dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
> + expected_val, val & mask);
> +
> + return -ETIMEDOUT;
> +}
> +
> static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
> {
> u32 val = en ? CS_SELECT_NAND_WP : 0;
> @@ -1024,12 +1052,39 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
>
> if ((ctrl->features & BRCMNAND_HAS_WP) && wp_on == 1) {
> static int old_wp = -1;
> + int ret;
>
> if (old_wp != wp) {
> dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off");
> old_wp = wp;
> }
> +
> + /*
> + * make sure ctrl/flash ready before and after
> + * changing state of #WP pin
> + */
> + ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY |
> + NAND_STATUS_READY,
> + NAND_CTRL_RDY |
> + NAND_STATUS_READY, 0);
> + if (ret)
> + return;
> +
> brcmnand_set_wp(ctrl, wp);
> + chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> + /* NAND_STATUS_WP 0x00 = protected, 0x80 = not protected */
> + ret = bcmnand_ctrl_poll_status(ctrl,
> + NAND_CTRL_RDY |
> + NAND_STATUS_READY |
> + NAND_STATUS_WP,
> + NAND_CTRL_RDY |
> + NAND_STATUS_READY |
> + (wp ? 0 : NAND_STATUS_WP), 0);
> +
> + if (ret)
> + dev_err_ratelimited(&host->pdev->dev,
> + "nand #WP expected %s\n",
> + wp ? "on" : "off");
> }
> }
>
> @@ -1157,15 +1212,15 @@ static irqreturn_t brcmnand_dma_irq(int irq, void *data)
> static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
> {
> struct brcmnand_controller *ctrl = host->ctrl;
> - u32 intfc;
> + int ret;
>
> dev_dbg(ctrl->dev, "send native cmd %d addr_lo 0x%x\n", cmd,
> brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS));
> BUG_ON(ctrl->cmd_pending != 0);
> ctrl->cmd_pending = cmd;
>
> - intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> - WARN_ON(!(intfc & INTFC_CTLR_READY));
> + ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> + WARN_ON(ret);
>
> mb(); /* flush previous writes */
> brcmnand_write_reg(ctrl, BRCMNAND_CMD_START,
^ permalink raw reply [flat|nested] 10+ messages in thread