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

[Chris Packham]

Hi Miquel,

On 27/09/17 22:16, Miquel RAYNAL wrote:
> On Wed, 27 Sep 2017 08:53:18 +0200
> Miquel RAYNAL <miquel.raynal@xxxxxxxxxxxxxxxxxx> wrote:
> 
>> Hello Kalyan,
>>
>> On Wed, 27 Sep 2017 13:55:16 +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.
> 
> I forgot to ask you what is this guideline ? Is this a Marvell
> document ? I could not find it. The functional spec clearly states:
> "When NOR/NAND hardware arbiter enabled, this bit must be set" but as
> you mention it in the commit log and in the code it is worth knowing
> what your are referring to.

The guideline number is from a Marvell Errata document MV-S501377-00, 
although this specific change is a guideline not an erratum. You'll need 
access to Marvell's extranet (with appropriate NDAs etc) to retrieve it. 
We can mention the document reference in the commit message for v3.

> Also, could you test if this bit introduces a regression or a speed
> penalty when using for instance only one NAND chip (so not using
> the arbiter even if it is enabled) ?
> 
> To do so you may use the flash_speed tool from the mtd-utils package:
> http://lists.infradead.org/pipermail/linux-mtd/2017-August/076477.html

We haven't run flash_speed (yet) but anecdotally we've seen no 
noticeable performance difference.

> 
> Thank you,
> Miquèl
> 
>>>
>>> This commit sets the FORCE_CSX bit to 1 for all
>>> ARMADA370 variants as the arbitration is always enabled by
>>> default.
>>
>> Maybe you could mention here that this does not apply for pxa3xx
>> variant as this bit does not exist/is reserved on the NFCv1.
>>
>>>
>>> Signed-off-by: Kalyan Kinthada <kalyan.kinthada@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>>   drivers/mtd/nand/pxa3xx_nand.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/mtd/nand/pxa3xx_nand.c
>>> b/drivers/mtd/nand/pxa3xx_nand.c index 85cff68643e0..b2753eea567c
>>> 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c
>>> +++ b/drivers/mtd/nand/pxa3xx_nand.c
>>> @@ -68,6 +68,7 @@
>>>   #define NDCR_PAGE_SZ		(0x1 << 24)
>>>   #define NDCR_NCSX		(0x1 << 23)
>>>   #define NDCR_ND_MODE		(0x3 << 21)
>>> +#define NDCR_FORCE_CSX		(0x1 << 21)
>>>   #define NDCR_NAND_MODE   	(0x0)
>>
>> The three lines above seems to have the same purpose.
>> I had a look to the specs (pxa/370/375/xp/380/390), and I could
>> not find any reference to a valid bit 22 in NDCR (it is always
>> reserved). Maybe you could delete NDCR_ND_MODE and
>> NDCR_NAND_MODE then ?
>>
>>>   #define NDCR_CLR_PG_CNT		(0x1 << 20)
>>>   #define NFCV1_NDCR_ARB_CNTL	(0x1 << 19)
>>> @@ -1464,6 +1465,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 ARMADAGL-5830741370 Variants.
>>> Ref#: GL-5830741*/
>>
>> You miss a space between "GL-5830741" and "*/".
>>
>>> +	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 +1502,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*/
>>
>> Same here.
>>
>>> +	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);
>>>   }
>>
>>
>> Thanks,
>> Miquèl
>>
> 
> 
>