Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Yixun Lan]

HI Boris:

thanks for the quick response.

On 07/19/18 03:08, Boris Brezillon wrote:
> Hi Yixun,
> 
> On Wed, 18 Jul 2018 17:38:56 +0800
> Yixun Lan <yixun.lan@xxxxxxxxxxx> wrote:
> 
>>>> +
>>>> +#define NFC_REG_CMD		0x00
>>>> +#define NFC_REG_CFG		0x04
>>>> +#define NFC_REG_DADR		0x08
>>>> +#define NFC_REG_IADR		0x0c
>>>> +#define NFC_REG_BUF		0x10
>>>> +#define NFC_REG_INFO		0x14
>>>> +#define NFC_REG_DC		0x18
>>>> +#define NFC_REG_ADR		0x1c
>>>> +#define NFC_REG_DL		0x20
>>>> +#define NFC_REG_DH		0x24
>>>> +#define NFC_REG_CADR		0x28
>>>> +#define NFC_REG_SADR		0x2c
>>>> +#define NFC_REG_PINS		0x30
>>>> +#define NFC_REG_VER		0x38
>>>> +  
>>>
>>> Can you put the reg offsets next to their field definitions?
>>>   
>> actually, we would prefer to put all the CMD definition below the reg
>> offset, so it will better reflect what's it belong to.
> 
> Just to be clear, I meant something like:
> 
> #define NFC_CMD				0x00
> #define NFC_CMD_DRD			(0x8 << 14)
> #define NFC_CMD_IDLE			(0xc << 14)
> ...
> 
> #define NFC_CFG				0x04
> #define NFC_CFG_XXX			xxx
> ...
> 
> I find it easier to guess which register the fields are attached to when
> it's defined like that, but I won't block the driver for such a tiny
> detail. 
> 
yes, this is exactly what I mean

>>>> +static void meson_nfc_cmd_ctrl(struct mtd_info *mtd,
>>>> +					int cmd, unsigned int ctrl)  
>>>   
>>> ->cmd_ctrl() has recently been deprecated in favor of ->exec_op(). You  
>>> can have a look at the marvell, v610 or fsmc drivers if you want to
>>> have an idea of how ->exec_op() should be implemented. Miquel and I are
>>> also here to help if you have any questions.
>>>   
>>
>> follow your suggestion, we have implemented the exec_op() interface,
>> we'd really appreciate if you can help to review this ..
> 
> Sure, just send a v2 and we'll review it.
> 
> 
>>>> +
>>>> +static void meson_nfc_cmd_m2n(struct meson_nfc *nfc, int raw)  
>>>
>>> n2m -> nand2mem ?
>>>   
>> yes, it is
> 
> Then please use nand2mem, it's clearer.
we end at dropping the n2m function. by converting them into

static void
meson_nfc_cmd_access(
struct meson_nfc *nfc,
struct mtd_info *mtd, int raw, bool dir)


> 
>>>> +static int meson_nfc_wait_dma_finish(struct meson_nfc *nfc)
>>>> +{
>>>> +	meson_nfc_cmd_idle(nfc, 0);
>>>> +	meson_nfc_cmd_idle(nfc, 0);  
>>>
>>> Two calls to cmd_idle(), is this expected or a copy&paste error? If
>>> that's expected it definitely deserves a comment explaining why?
>>>   
>>
>> yes, it is intentional
>>
>> we will put these comments into the function.
>> 	/*
>>          * The Nand flash controller is designed as two stages pipleline -
>>          *  a) fetch and b) excute.
>>          * So, there might be cases when the driver see command queue is
>> empty,
>>          * but the Nand flash controller still has two commands buffered,
>>          * one is fetched into NFC request queue (ready to run), and another
>>          * is actively executing.
>>          */
>>
> 
> So pushing 2 "IDLE" commands guarantees that the pipeline is emptied,
> right? The comment looks incomplete, you should explain what those
> meson_nfc_cmd_idle() are for.
> 
thanks

the meson_nfc_cmd_idle() function itself is quite straightforward, and
we feel explain that inserting 2 "IDLE" commands to drain out the
pipeline is enough.

>>>> +static int meson_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
>>>> +{
>>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>>>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>> +	struct meson_nand_ecc *meson_ecc = nfc->data->ecc;
>>>> +	int num = nfc->data->ecc_num;
>>>> +	int nsectors, i, bytes;
>>>> +
>>>> +	/* support only ecc hw mode */
>>>> +	if (nand->ecc.mode != NAND_ECC_HW) {  
>>>
>>> Given that you support raw accesses, I'm pretty sure you can support
>>> ECC_NONE, ECC_SOFT and ECC_ON_DIE with zero effort.
>>>   
>>
>> is this a block for this driver to be accepted by upstream?
> 
> Nope.
> 
>> otherwise we'd like to implement this feature later in separate patch.
>>
> 
> That's fine.
> 
>>>> +	nsectors = mtd->writesize / nand->ecc.size;
>>>> +	bytes =(meson_chip->user_mode == NFC_USER2_OOB_BYTES) ? nsectors * 2 : 16;
>>>> +	if (mtd->oobsize < (nand->ecc.bytes * nsectors + bytes))
>>>> +		return -EINVAL;  
>>>
>>> It's probably worth looking at what is being proposed here [2] for the
>>> ECC config selection logic.
>>>   
>>
>> sure, we'd happy to adopt the ECC config helper function, and seems it
>> is possible ;-)
>>
>> sounds the proposed ECC config patch is still under review, and we
>> would like to adjust our code once it's ready (probably we will still
>> keep this version in patch v2, then address it in v3 later)
> 
> It's been merged, and should be available in the nand/next branch [1].
> 
em... I'd leave this to Liang Yang to implement this, so it's not fixed
in next PATCH v2, but will address this in v3.

thanks

>>>> +static int meson_nfc_buffer_init(struct mtd_info *mtd)
>>>> +{
>>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>>>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>> +	struct device *dev = nfc->dev;
>>>> +	int info_bytes, page_bytes;
>>>> +	int nsectors;
>>>> +
>>>> +	nsectors = mtd->writesize / nand->ecc.size;
>>>> +	info_bytes = nsectors * PER_INFO_BYTE;
>>>> +	page_bytes = mtd->writesize + mtd->oobsize;
>>>> +
>>>> +	if ((meson_chip->data_buf) && (meson_chip->info_buf))
>>>> +		return 0;
>>>> +
>>>> +	meson_chip->data_buf = devm_kzalloc(dev, page_bytes, GFP_KERNEL);
>>>> +	if (!meson_chip->data_buf)
>>>> +		return  -ENOMEM;
>>>> +
>>>> +	meson_chip->info_buf = devm_kzalloc(dev, info_bytes, GFP_KERNEL);
>>>> +	if (!meson_chip->info_buf)
>>>> +		return  -ENOMEM;  
>>>
>>> You're doing DMA on those buffers, and devm_kzalloc() is not
>>> DMA-friendly (returned buffers are not aligned on a cache line). Also,
>>> you don't have to allocate your own buffers because the core already
>>> allocate them (chip->data_buf, chip->oob_poi). All you need to do is
>>> set the NAND_USE_BOUNCE_BUFFER flag in chip->options to make sure
>>> you're always passed a DMA-able buffer.
>>>   
>>
>> thanks for the suggestion, we've migrated to use the
>> dmam_alloc_coherent() API
> 
> kzalloc() should be just fine, no need to alloc a DMA coherent region. 
> 

we're a little bit confused here, isn't devm_kzalloc (previously we are
using) a variant of kzalloc? and since the NAND controller is doing DMA
here, using DMA coherent API is more proper way?

> 
>>
>>>> +	nand->setup_data_interface = meson_nfc_setup_data_interface;
>>>> +
>>>> +	nand->chip_delay = 200;  
>>>
>>> This should not be needed if you implement ->exec_op() and  
>>> ->setup_data_interface().  
>>>   
>> will drop this
>>
>>>> +	nand->ecc.mode = NAND_ECC_HW;
>>>> +
>>>> +	nand->ecc.write_page_raw = meson_nfc_write_page_raw;
>>>> +	nand->ecc.write_page = meson_nfc_write_page_hwecc;
>>>> +	nand->ecc.write_oob_raw = nand_write_oob_std;
>>>> +	nand->ecc.write_oob = nand_write_oob_std;
>>>> +
>>>> +	nand->ecc.read_page_raw = meson_nfc_read_page_raw;
>>>> +	nand->ecc.read_page = meson_nfc_read_page_hwecc;
>>>> +	nand->ecc.read_oob_raw = meson_nfc_read_oob_raw;
>>>> +	nand->ecc.read_oob = meson_nfc_read_oob;
>>>> +
>>>> +	mtd = nand_to_mtd(nand);
>>>> +	mtd->owner = THIS_MODULE;
>>>> +	mtd->dev.parent = dev;
>>>> +	mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL,
>>>> +				   "%s:nand", dev_name(dev));
>>>> +	if (!mtd->name) {
>>>> +		dev_err(nfc->dev, "Failed to allocate mtd->name\n");
>>>> +		return -ENOMEM;
>>>> +	}  
>>>
>>> You set the name after nand_scan_ident() and make it conditional (only
>>> if ->name == NULL) so that the label property defined in the DT takes
>>> precedence over the default name.  
>>
for setting mtd->name conditional, do you mean doing something like this?

if (!mtd->name)
	mtd->name = devm_kasprintf(..)

but we found mtd->name = "ffe07800.nfc" after function
nand_scan_ident(), which is same value as dev_name(dev)..
and there is no cs information encoded there.


>> we can do this, but as second consideration, we'd prefer simply to drop
>> this customization of mtd->name here (we didn't understand your next cs
>> id suggestion).
> 
> No, you really should set a well-known name, so that people can pass
> mtdparts on the kernel command line.
> 
ok

>>
>>> Also, I recommend suffixing this name
>>> with the CS id, just in case you ever need to support connecting several
>>> chips to the same controller. 
>>>   
>>
>> we actually didn't get the point here, cs is about chip selection with
>> multiple nand chip? and how to get this information?
> 
> Well, you currently seem to only support one chip per controller, but I
> guess the IP can handle several CS lines. So my recommendation is about
> choosing a name so that you can later easily add support for multiple
> chips without breaking setups where mtdparts is used.
> 
> To sum-up, assuming your NAND chip is always connected to CS0 (on the
> controller side), I'd suggest doing:
> 
yes, this is exactly how the hardware connected.
> 	mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL,
> 				   "%s:nand.%d", dev_name(dev), cs_id);
> 
> where cs_id is the value you extracted from the reg property of the
> NAND node.
> 
Ok, you right.
current, the NAND chip is only use one CS (which CE0) for now, what's in
the DT is

nand@0 {
 reg = < 0 >;
 ..
};

so for the multiple chips it would something like this in DT?

nand@0 {
  reg = < 0 >;
};

nand@1 {
  reg = < 1 >;
};

or even
nand@0 {
  reg = < 0 2 >;
};

nand@1 {
  reg = < 3 4 >;
};

do we need to encode all the cs information here? not sure if we
understand this correctly, but could send out the patch for review..


> Regards,
> 
> Boris
> 
> [1]http://git.infradead.org/linux-mtd.git/shortlog/refs/heads/nand/next
> 
> .
>