Re: [PATCH v3 1/1] mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants.

[Chris Packham]

Hi Miquel,

On 09/10/17 19:19, Miquel RAYNAL wrote:
> Hello Kalyan,
> 
> On Mon, 9 Oct 2017 02:31:30 +0000
> Kalyan Kinthada <Kalyan.Kinthada@xxxxxxxxxxxxxxxxxxx> wrote:
> 
>> On 05/10/17 20:41, Miquel RAYNAL wrote:
>>> Hello Kalyan,
>>>
>>> On Thu, 28 Sep 2017 13:57:56 +1300
>>> Kalyan Kinthada <kalyan.kinthada@xxxxxxxxxxxxxxxxxxx> wrote:
>>>   
>>>> When the arbitration between NOR and NAND flash is enabled
>>>> the <FORCE_CSX> field bit[21] in the Data Flash Control Register
>>>> needs to be set to 1 according to guidleine GL-5830741 mentioned
>>> Typo: guideline ^
>> I will correct this typo in the next patch version.
>>>> in Marvell Errata document MV-S501377-00, Rev. D.
>>> Thanks for that, I checked it.
>>>   
>>>> This commit sets the FORCE_CSX bit to 1 for all
>>>> ARMADA370 variants as the arbitration is always enabled by default.
>>>> This change does not apply for pxa3xx variants because the
>>>> FORCE_CSX bit does not exist/reserved on the NFCv1.
>>> Sorry to bother you again but I am reworking the pxa3xx_nand driver
>>> and so I would like to deeply understand why this is needed because
>>> I will have to integrate it in my work too.
>>>
>>> So please tell me what is your setup, do you really use NAND/NOR
>>> arbitration? Do you have not-Don't Care CS NAND chips? I have some
>>> doubts because even if the spec precises in the NAND controller
>>> chapter that arbitration is always enabled by default, the bit 27
>>> (NfArbiterEn) in the SoC Device Multiplex Register at offset
>>> 0x00018208 is actually at 0 by default (disabled). Did you enable
>>> this bit manually ? I checked with devmem on my setup and this bit
>>> was unset.
>> Yes, my setup use NAND/NOR arbitration and use a Don't
>> care CS Nand Chip. The bit 27 at offset 0x00018208 is set to 1
>> by default in my case. I did not manually set the bit 27 to 1.
> 
> Actually if you use Don't Care CS NAND chips you should not need the
> force CS bit, as it is mentioned in the guidelines you pointed, this
> bit is only useful for *not* don't care CS NAND chips (and the name
> "force CS" is pretty straight forward too!).

It's less straight forward than you think. It's FORCE_CSX (the X is 
important) which is defined as "force chip select false on busy". What 
it means is that the NF_CSn is de-asserted if the interface is 
arbitrated away from the NAND flash controller. I think that means you 
won't end up with the chip-select for the dev-bus/NOR firing immediately 
when that device is arbitrated.

> Otherwise what bootloader and kernel do you use to have this
> bit set by default? You may also want to check if this bit is set by
> your bootloader by stopping it before it loads Linux and check manually
> the value of this bit with 'md' (on U-Boot).
> 
> Thank you,
> Miquèl
> 
>>
>> Please let me now if you have any other question or do you want me
>> to send the updated patch version with the typo fixed.
>>
>> Thank You,
>> Kalyan
>>
>>>
>>> Thank you for your time,
>>> Miquèl
>>>   
>>>> The NDCR_ND_MODE conflicts with the new NDCR_FORCE_CSX but is
>>>> unused so remove it along with NDCR_NAND_MODE.
>>>>
>>>> Signed-off-by: Kalyan Kinthada
>>>> <kalyan.kinthada@xxxxxxxxxxxxxxxxxxx> ---
>>>
>>>   
>>>>    drivers/mtd/nand/pxa3xx_nand.c | 9 +++++++--
>>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/pxa3xx_nand.c
>>>> b/drivers/mtd/nand/pxa3xx_nand.c index 85cff68643e0..a6a7a5af7bed
>>>> 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c
>>>> +++ b/drivers/mtd/nand/pxa3xx_nand.c
>>>> @@ -67,8 +67,7 @@
>>>>    #define NDCR_DWIDTH_M		(0x1 << 26)
>>>>    #define NDCR_PAGE_SZ		(0x1 << 24)
>>>>    #define NDCR_NCSX		(0x1 << 23)
>>>> -#define NDCR_ND_MODE		(0x3 << 21)
>>>> -#define NDCR_NAND_MODE   	(0x0)
>>>> +#define NDCR_FORCE_CSX		(0x1 << 21)
>>>>    #define NDCR_CLR_PG_CNT		(0x1 << 20)
>>>>    #define NFCV1_NDCR_ARB_CNTL	(0x1 << 19)
>>>>    #define NFCV2_NDCR_STOP_ON_UNCOR	(0x1 << 19)
>>>> @@ -1464,6 +1463,9 @@ static int pxa3xx_nand_config_ident(struct
>>>> pxa3xx_nand_info *info) info->chunk_size = PAGE_CHUNK_SIZE;
>>>>    	info->reg_ndcr = 0x0; /* enable all interrupts */
>>>>    	info->reg_ndcr |= (pdata->enable_arbiter) ?
>>>> NDCR_ND_ARB_EN : 0;
>>>> +	/* Set FORCE_CSX bit for all ARMADA370 Variants. Ref#:
>>>> GL-5830741 */
>>>> +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
>>>> +		info->reg_ndcr |= NDCR_FORCE_CSX;
>>>>    	info->reg_ndcr |= NDCR_RD_ID_CNT(READ_ID_BYTES);
>>>>    	info->reg_ndcr |= NDCR_SPARE_EN;
>>>>    
>>>> @@ -1498,6 +1500,9 @@ static void pxa3xx_nand_detect_config(struct
>>>> pxa3xx_nand_info *info) info->reg_ndcr = ndcr &
>>>>    		~(NDCR_INT_MASK | NDCR_ND_ARB_EN |
>>>> NFCV1_NDCR_ARB_CNTL); info->reg_ndcr |= (pdata->enable_arbiter) ?
>>>> NDCR_ND_ARB_EN : 0;
>>>> +	/* Set FORCE_CSX bit for all ARMADA370 Variants. Ref#:
>>>> GL-5830741 */
>>>> +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
>>>> +		info->reg_ndcr |= NDCR_FORCE_CSX;
>>>>    	info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
>>>>    	info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
>>>>    }
>>>
>>>   
> 
> 
>