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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 22 May 2018 16:28:37 +0200
From:   Stefan Agner <stefan@...er.ch>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     boris.brezillon@...tlin.com, dwmw2@...radead.org,
        computersforpeace@...il.com, marek.vasut@...il.com,
        robh+dt@...nel.org, mark.rutland@....com, thierry.reding@...il.com,
        mturquette@...libre.com, sboyd@...nel.org, dev@...xeye.de,
        miquel.raynal@...tlin.com, richard@....at, marcel@...wiler.com,
        krzk@...nel.org, benjamin.lindqvist@...ian.se,
        jonathanh@...dia.com, pdeschrijver@...dia.com, pgaikwad@...dia.com,
        mirza.krak@...il.com, linux-mtd@...ts.infradead.org,
        linux-tegra@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash
 controller driver

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@...xeye.de>
>>>> Signed-off-by: Stefan Agner <stefan@...er.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