[PATCH] mtd: nand: pxa3xx-nand: prevent DFI bus lockup on removal

[PATCH] mtd: nand: pxa3xx-nand: prevent DFI bus lockup on removal

[Robert Jarzmik]

After the conversion of pxa architecture to common clock framework, the
NAND clock can be disabled on driver exit.

In this case, it happens that if the driver used the NAND and set the
DFI arbitration bit, the next access to a static memory controller area,
such as an ethernet card, will stall the system bus, and the core will
be stalled forever.

This is especially true on pxa31x SoCs, where the NDCR was augmented
with a new bit to prevent this lockups by giving full ownership of the
DFI arbiter to the SMC, in change SCr#6.

Fix this by clearing the DFI arbritration bit in driver exit. This
effectively prevents a lockup on zylonite when removing pxa3xx-nand
module, and using ethernet afterwards.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

---
Ezequiel, this deserves a test on Armada, as the bit has another meaning
there I think.
---
 drivers/mtd/nand/pxa3xx_nand.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index f287ce9e225a..087d769f92fd 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -78,6 +78,7 @@
 #define NDCR_NAND_MODE   	(0x0)
 #define NDCR_CLR_PG_CNT		(0x1 << 20)
 #define NDCR_STOP_ON_UNCOR	(0x1 << 19)
+#define NDCR_ARB_CNTL		(0x1 << 19)
 #define NDCR_RD_ID_CNT_MASK	(0x7 << 16)
 #define NDCR_RD_ID_CNT(x)	(((x) << 16) & NDCR_RD_ID_CNT_MASK)
 
@@ -1322,7 +1323,8 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 	}
 
 	/* Set an initial chunk size */
-	info->reg_ndcr = ndcr & ~NDCR_INT_MASK;
+	info->reg_ndcr = ndcr &
+		~(NDCR_INT_MASK | NDCR_ND_ARB_EN | NDCR_ARB_CNTL);
 	info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
 	info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
 	return 0;
@@ -1569,6 +1571,7 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
 	pxa3xx_flash_ids[1].name = NULL;
 	def = pxa3xx_flash_ids;
 KEEP_CONFIG:
+	info->reg_ndcr |= (pdata->enable_arbiter) ? NDCR_ND_ARB_EN : 0;
 	if (info->reg_ndcr & NDCR_DWIDTH_M)
 		chip->options |= NAND_BUSWIDTH_16;
 
@@ -1784,6 +1787,8 @@ static int pxa3xx_nand_remove(struct platform_device *pdev)
 		free_irq(irq, info);
 	pxa3xx_nand_free_buff(info);
 
+	nand_writel(info, NDCR,
+		    (nand_readl(info, NDCR) & ~NDCR_ND_ARB_EN) | NDCR_ARB_CNTL);
 	clk_disable_unprepare(info->clk);
 
 	for (cs = 0; cs < pdata->num_cs; cs++)
-- 
2.1.4

Re: [PATCH] mtd: nand: pxa3xx-nand: prevent DFI bus lockup on removal

[Ezequiel Garcia]

On 23 Aug 09:05 PM, Robert Jarzmik wrote:
> After the conversion of pxa architecture to common clock framework, the
> NAND clock can be disabled on driver exit.
> 
> In this case, it happens that if the driver used the NAND and set the
> DFI arbitration bit, the next access to a static memory controller area,
> such as an ethernet card, will stall the system bus, and the core will
> be stalled forever.
> 
> This is especially true on pxa31x SoCs, where the NDCR was augmented
> with a new bit to prevent this lockups by giving full ownership of the
> DFI arbiter to the SMC, in change SCr#6.
> 
> Fix this by clearing the DFI arbritration bit in driver exit. This
> effectively prevents a lockup on zylonite when removing pxa3xx-nand
> module, and using ethernet afterwards.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> 
> ---
> Ezequiel, this deserves a test on Armada, as the bit has another meaning
> there I think.

Right, on NFCv2 (Armada 370/XP) the bit 19 of NDCR has another meaning:
"Stop on uncorrectable error".

The bit is kept cleared during while the controller is used, so it should
be fine to keep it cleared for PXA usage as well.

BTW, the bit 12 (ND_ARB_EN on PXA) is marked as reserved on NFCv2.

> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index f287ce9e225a..087d769f92fd 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -78,6 +78,7 @@
>  #define NDCR_NAND_MODE   	(0x0)
>  #define NDCR_CLR_PG_CNT		(0x1 << 20)
>  #define NDCR_STOP_ON_UNCOR	(0x1 << 19)
> +#define NDCR_ARB_CNTL		(0x1 << 19)

Should we worry about having two definitions for the same bit?
Would it be too ugly to mix the two meaning? Something like this:

/* This bit has two different meanings on NFCv1 and NFCv2 */
#define NDCR_STOP_ON_UNCOR_ARB_CNTL (0x1 << 19)

>  #define NDCR_RD_ID_CNT_MASK	(0x7 << 16)
>  #define NDCR_RD_ID_CNT(x)	(((x) << 16) & NDCR_RD_ID_CNT_MASK)
>  
> @@ -1322,7 +1323,8 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
>  	}
>  
>  	/* Set an initial chunk size */
> -	info->reg_ndcr = ndcr & ~NDCR_INT_MASK;
> +	info->reg_ndcr = ndcr &
> +		~(NDCR_INT_MASK | NDCR_ND_ARB_EN | NDCR_ARB_CNTL);
>  	info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
>  	info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
>  	return 0;
> @@ -1569,6 +1571,7 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
>  	pxa3xx_flash_ids[1].name = NULL;
>  	def = pxa3xx_flash_ids;
>  KEEP_CONFIG:
> +	info->reg_ndcr |= (pdata->enable_arbiter) ? NDCR_ND_ARB_EN : 0;
>  	if (info->reg_ndcr & NDCR_DWIDTH_M)
>  		chip->options |= NAND_BUSWIDTH_16;
>  
> @@ -1784,6 +1787,8 @@ static int pxa3xx_nand_remove(struct platform_device *pdev)
>  		free_irq(irq, info);
>  	pxa3xx_nand_free_buff(info);
>  

I think a comment here explaining how this disables DFI arbitration and
how clearing it grants DFI access to SMC only.

While here, we might want to document how the whole arbiter applies to
PXA only, since the DFI bus is shared there.

> +	nand_writel(info, NDCR,
> +		    (nand_readl(info, NDCR) & ~NDCR_ND_ARB_EN) | NDCR_ARB_CNTL);

>  	clk_disable_unprepare(info->clk);
>  
>  	for (cs = 0; cs < pdata->num_cs; cs++)
> -- 
> 2.1.4
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Thanks!
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] mtd: nand: pxa3xx-nand: prevent DFI bus lockup on removal

[Robert Jarzmik]

Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> writes:

> Should we worry about having two definitions for the same bit?
> Would it be too ugly to mix the two meaning? Something like this:
>
> /* This bit has two different meanings on NFCv1 and NFCv2 */
> #define NDCR_STOP_ON_UNCOR_ARB_CNTL (0x1 << 19)
I don't find that very pretty, but if you want I can put that in the patch
instead.

>> @@ -1784,6 +1787,8 @@ static int pxa3xx_nand_remove(struct platform_device *pdev)
>>  		free_irq(irq, info);
>>  	pxa3xx_nand_free_buff(info);
>>  
>
> I think a comment here explaining how this disables DFI arbitration and
> how clearing it grants DFI access to SMC only.
>
> While here, we might want to document how the whole arbiter applies to
> PXA only, since the DFI bus is shared there.
Ok, for v2. I take it that DFI bus is not shared on Armada, lucky you. Maybe
something like :

/*
 * In the pxa3xx case, the DFI bus is shared between the SMC and NFC. In order
 * to prevent a lockup of the system bus, the DFI bus arbitration is granted
 * to SMC upon driver removal. This is done by setting the x_ARB_CNTL bit,
 * which also prevents the NAND to have access to the bus anymore.
 */

Cheers.

-- 
Robert

Re: [PATCH] mtd: nand: pxa3xx-nand: prevent DFI bus lockup on removal

[Ezequiel Garcia]

Robert,

On 24 August 2015 at 15:24, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> writes:
>
>> Should we worry about having two definitions for the same bit?
>> Would it be too ugly to mix the two meaning? Something like this:
>>
>> /* This bit has two different meanings on NFCv1 and NFCv2 */
>> #define NDCR_STOP_ON_UNCOR_ARB_CNTL (0x1 << 19)
> I don't find that very pretty, but if you want I can put that in the patch
> instead.
>

Yeah, it's far from pretty.

OK, another idea. How about this:

#define NFCV2_NDCR_STOP_ON_UNCOR (0x1 << 19)
#define NFCV1_NDCR_ARB_CNTL      (0x1 << 19)

Or maybe I'm just bike-shedding here? I figured it's important
to document the bit has different meanings in different
hardware versions.

Anyway, just submit a v2 with whatever you feel it's less ugly.

>>> @@ -1784,6 +1787,8 @@ static int pxa3xx_nand_remove(struct platform_device *pdev)
>>>              free_irq(irq, info);
>>>      pxa3xx_nand_free_buff(info);
>>>
>>
>> I think a comment here explaining how this disables DFI arbitration and
>> how clearing it grants DFI access to SMC only.
>>
>> While here, we might want to document how the whole arbiter applies to
>> PXA only, since the DFI bus is shared there.
> Ok, for v2. I take it that DFI bus is not shared on Armada, lucky you.

Right, seems not shared.

> Maybe
> something like :
>
> /*
>  * In the pxa3xx case, the DFI bus is shared between the SMC and NFC. In order
>  * to prevent a lockup of the system bus, the DFI bus arbitration is granted
>  * to SMC upon driver removal. This is done by setting the x_ARB_CNTL bit,
>  * which also prevents the NAND to have access to the bus anymore.
>  */
>

That'll work. Feel free to send a v2.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] mtd: nand: pxa3xx-nand: prevent DFI bus lockup on removal

[Robert Jarzmik]

Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> writes:

> Robert,
>
> On 24 August 2015 at 15:24, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> writes:
>>
>>> Should we worry about having two definitions for the same bit?
>>> Would it be too ugly to mix the two meaning? Something like this:
>>>
>>> /* This bit has two different meanings on NFCv1 and NFCv2 */
>>> #define NDCR_STOP_ON_UNCOR_ARB_CNTL (0x1 << 19)
>> I don't find that very pretty, but if you want I can put that in the patch
>> instead.
>>
>
> Yeah, it's far from pretty.
>
> OK, another idea. How about this:
>
> #define NFCV2_NDCR_STOP_ON_UNCOR (0x1 << 19)
> #define NFCV1_NDCR_ARB_CNTL      (0x1 << 19)
This one looks much more prettier, I'll take it.

> That'll work. Feel free to send a v2.
Okay, I'm on my way.

Cheers.

-- 
Robert