Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver

[Stefan Agner <stefan@agner.ch>]

On 22.05.2018 15:34, Dmitry Osipenko wrote:
> On 22.05.2018 15:19, Stefan Agner wrote:
>> [review sent to my first patch sent off-ml, moving to ml thread]
>>
>> On 21.05.2018 16:05, Dmitry Osipenko wrote:
>>> Hello Stefan,
>>>
>>> I don't have expertise to review the actual NAND-related driver logic, so I only
>>> reviewed the basics. The driver code looks good to me, though I've couple minor
>>> comments.
>>>
>>> On 21.05.2018 03:16, Stefan Agner wrote:
>>>> Add support for the NAND flash controller found on NVIDIA
>>>> Tegra 2 SoCs. This implementation does not make use of the
>>>> command queue feature. Regular operations/data transfers are
>>>> done in PIO mode. Page read/writes with hardware ECC make
>>>> use of the DMA for data transfer.
>>>>
>>>> Signed-off-by: Lucas Stach <dev@lynxeye.de>
>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>> ---

<snip>

>>>> +static int tegra_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>>>> +				uint8_t *buf, int oob_required, int page)
>>>> +{
>>>> +	struct tegra_nand *nand = to_tegra_nand(mtd);
>>>> +	u32 value, addrs;
>>>> +
>>>> +	writel(NAND_CMD_READ0, nand->regs + CMD_1);
>>>> +	writel(NAND_CMD_READSTART, nand->regs + CMD_2);
>>>> +
>>>> +	addrs = tegra_nand_fill_address(mtd, chip, page);
>>>> +
>>>> +	value = readl(nand->regs + CFG);
>>>> +	value |= CFG_HW_ECC | CFG_ERR_COR;
>>>> +	writel(value, nand->regs + CFG);
>>>> +
>>>> +	writel(mtd->writesize - 1, nand->regs + DMA_CFG_A);
>>>> +	writel(nand->data_dma, nand->regs + DATA_PTR);
>>>> +
>>>> +	if (oob_required) {
>>>> +		writel(mtd_ooblayout_count_freebytes(mtd) - 1,
>>>> +		       nand->regs + DMA_CFG_B);
>>>> +		writel(nand->oob_dma, nand->regs + TAG_PTR);
>>>> +	} else {
>>>> +		writel(0, nand->regs + DMA_CFG_B);
>>>> +		writel(0, nand->regs + TAG_PTR);
>>>> +	}
>>>> +
>>>> +	value = DMA_CTRL_GO | DMA_CTRL_IN | DMA_CTRL_PERF_EN |
>>>> +		DMA_CTRL_REUSE | DMA_CTRL_IE_DONE | DMA_CTRL_IS_DONE |
>>>> +		DMA_CTRL_BURST_8 | DMA_CTRL_EN_A;
>>>
>>> Wouldn't be more efficient to set DMA burst to 16 words? The writesize seems
>>> always aligned to at least 64 words.
>>>
>>
>> Hm, haven't tested 16 words, 8 was the setting Lucas used.
>>
>> Are you sure this is only about write size? Not sure, but isn't the ECC
>> area also DMA'd? On Colibri we use RS with t=8, hence 144 bytes parity,
>> so this would be properly aligned non the less...
>>
> 
> I don't know, that's up to you to figure out. Is RS stands for Reed-Solomon and
> t=8 is the max number of redundant words?
> 

RS = Reed-Solomon
t=8 maximum symbol errors

Will do some testing and check whether it fails.

--
Stefan