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

[Yixun Lan]

HI Boris


On 07/19/18 16:39, Boris Brezillon wrote:
> Hi Yixun,
> 
> On Thu, 19 Jul 2018 16:13:47 +0800
> Yixun Lan  wrote:
> 
> 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?
> 
> Well, making buffers DMA coherent might be expensive, especially if you
> access them a lot (unless you have a coherency unit and the cache is
> kept enabled).
> 
> Regarding the "why is devm_kzalloc() is not DMA-safe?" question, I'd
> recommend that you read this discussion [1].
> 
great, thanks for the info.

we fixed this in patch v2

>> +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(..)
> 
> Yes, that's what I meant.
> 
>>
>> 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.
> 
> Hm, that shouldn't be the case. Maybe you can add traces to find out
> who is setting mtd->name to this value.
> 
will trace this, then get back to you
>>
  
> 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 >;
>> };
> 
> Yep, that's for 2 single-die chips.
> 
>>
>> or even
>> nand@0 {
>>   reg = < 0 2 >;
>> };
>>
>> nand@1 {
> 
> nand@3 {
> 
>>   reg = < 3 4 >;
>> };
> 
> And this is describing 2 dual-die chips.
> 
>>
>> 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..
> 
> Yes, reg should contain an array of controller-side CS lines used to
> select the chip (or a specific die in a chip, the index in the reg
> table being the id of the die).
>
much clear about this, thanks

Yixun

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

[Yixun Lan]

HI Boris


On 07/19/18 16:39, Boris Brezillon wrote:
> Hi Yixun,
> 
> On Thu, 19 Jul 2018 16:13:47 +0800
> Yixun Lan  wrote:
> 
> 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?
> 
> Well, making buffers DMA coherent might be expensive, especially if you
> access them a lot (unless you have a coherency unit and the cache is
> kept enabled).
> 
> Regarding the "why is devm_kzalloc() is not DMA-safe?" question, I'd
> recommend that you read this discussion [1].
> 
great, thanks for the info.

we fixed this in patch v2

>> +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(..)
> 
> Yes, that's what I meant.
> 
>>
>> 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.
> 
> Hm, that shouldn't be the case. Maybe you can add traces to find out
> who is setting mtd->name to this value.
> 
will trace this, then get back to you
>>
  
> 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 >;
>> };
> 
> Yep, that's for 2 single-die chips.
> 
>>
>> or even
>> nand@0 {
>>   reg = < 0 2 >;
>> };
>>
>> nand@1 {
> 
> nand@3 {
> 
>>   reg = < 3 4 >;
>> };
> 
> And this is describing 2 dual-die chips.
> 
>>
>> 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..
> 
> Yes, reg should contain an array of controller-side CS lines used to
> select the chip (or a specific die in a chip, the index in the reg
> table being the id of the die).
>
much clear about this, thanks

Yixun

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

[Boris Brezillon]

Hi Yixun,

On Thu, 19 Jul 2018 16:13:47 +0800
Yixun Lan  wrote:

> >>> 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?

Well, making buffers DMA coherent might be expensive, especially if you
access them a lot (unless you have a coherency unit and the cache is
kept enabled).

Regarding the "why is devm_kzalloc() is not DMA-safe?" question, I'd
recommend that you read this discussion [1].

>  +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(..)

Yes, that's what I meant.

> 
> 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.

Hm, that shouldn't be the case. Maybe you can add traces to find out
who is setting mtd->name to this value.

> 
> >>  
> >>> 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 >;
> };

Yep, that's for 2 single-die chips.

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

nand@3 {

>   reg = < 3 4 >;
> };

And this is describing 2 dual-die chips.

> 
> 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..

Yes, reg should contain an array of controller-side CS lines used to
select the chip (or a specific die in a chip, the index in the reg
table being the id of the die).

Regards,

Boris

[1]http://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma

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

[Boris Brezillon]

Hi Yixun,

On Thu, 19 Jul 2018 16:13:47 +0800
Yixun Lan  wrote:

> >>> 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?

Well, making buffers DMA coherent might be expensive, especially if you
access them a lot (unless you have a coherency unit and the cache is
kept enabled).

Regarding the "why is devm_kzalloc() is not DMA-safe?" question, I'd
recommend that you read this discussion [1].

>  +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(..)

Yes, that's what I meant.

> 
> 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.

Hm, that shouldn't be the case. Maybe you can add traces to find out
who is setting mtd->name to this value.

> 
> >>  
> >>> 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 >;
> };

Yep, that's for 2 single-die chips.

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

nand@3 {

>   reg = < 3 4 >;
> };

And this is describing 2 dual-die chips.

> 
> 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..

Yes, reg should contain an array of controller-side CS lines used to
select the chip (or a specific die in a chip, the index in the reg
table being the id of the die).

Regards,

Boris

[1]http://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma

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  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_DC0x18
 +#define NFC_REG_ADR   0x1c
 +#define NFC_REG_DL0x20
 +#define NFC_REG_DH0x24
 +#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 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 

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  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_DC0x18
 +#define NFC_REG_ADR   0x1c
 +#define NFC_REG_DL0x20
 +#define NFC_REG_DH0x24
 +#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 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 

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

[Boris Brezillon]

Hi Yixun,

On Wed, 18 Jul 2018 17:38:56 +0800
Yixun Lan  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_DC0x18
> >> +#define NFC_REG_ADR   0x1c
> >> +#define NFC_REG_DL0x20
> >> +#define NFC_REG_DH0x24
> >> +#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. 

> >> +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.

> >> +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 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.

> >> +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].

> >> +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 = 

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

[Boris Brezillon]

Hi Yixun,

On Wed, 18 Jul 2018 17:38:56 +0800
Yixun Lan  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_DC0x18
> >> +#define NFC_REG_ADR   0x1c
> >> +#define NFC_REG_DL0x20
> >> +#define NFC_REG_DH0x24
> >> +#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. 

> >> +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.

> >> +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 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.

> >> +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].

> >> +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 = 

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

[Yixun Lan]

Hi Roris

thanks for all your suggestions!

It actually takes us some time to digest all your comments ;-)
and get back to you on these questions.


On 06/25/18 03:38, Boris Brezillon wrote:
> 
> 
> Hi Yixun,
> 
> On Wed, 13 Jun 2018 16:13:14 +
> Yixun Lan  wrote:
> 
>> From: Liang Yang 
>>
>> Add initial support for the Amlogic NAND flash controller which found
>> in the Meson-GXBB/GXL/AXG SoCs.
>>
>> Singed-off-by: Liang Yang 
>> Signed-off-by: Yixun Lan 
>> ---
>>  drivers/mtd/nand/raw/Kconfig  |8 +
>>  drivers/mtd/nand/raw/Makefile |3 +
>>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>>  3 files changed, 1433 insertions(+)
>>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c
> 
> Can you run checkpatch.pl --strict and fix the coding style issues?
> 
sure, we will be more cautious about this

>>
>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> index 19a2b283fbbe..b3c17a3ca8f4 100644
>> --- a/drivers/mtd/nand/raw/Kconfig
>> +++ b/drivers/mtd/nand/raw/Kconfig
>> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
>>Enables support for NAND controller on MTK SoCs.
>>This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>>  
>> +config MTD_NAND_MESON
>> +tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
>> +depends on ARCH_MESON || COMPILE_TEST
>> +select COMMON_CLK_REGMAP_MESON
>> +select MFD_SYSCON
>> +help
>> +  Enables support for NAND controller on Amlogic's Meson SoCs.
>> +
>>  endif # MTD_NAND
>> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
>> index 165b7ef9e9a1..cdf6162f38c3 100644
>> --- a/drivers/mtd/nand/raw/Makefile
>> +++ b/drivers/mtd/nand/raw/Makefile
>> @@ -1,5 +1,7 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  
>> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson
> 
> Please don't do that. If you need to expose common regs, put them
> in include/linux/soc/meson/. I'm also not sure why you need to access
> the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
> provider whose driver would be placed in drivers/clk and which would use
> the mmc syscon. This way the same clk driver could be used for both
> MMC and NAND clk indifferently, and the NAND driver would be much
> simpler.
> 

this is already addressed in another thread, as we will model it as a
standard clock driver.

so this cflags can be dropped.

>> +
>>  obj-$(CONFIG_MTD_NAND)  += nand.o
>>  obj-$(CONFIG_MTD_NAND_ECC)  += nand_ecc.o
>>  obj-$(CONFIG_MTD_NAND_BCH)  += nand_bch.o
>> @@ -56,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_HISI504) += 
>> hisi504_nand.o
>>  obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/
>>  obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o
>>  obj-$(CONFIG_MTD_NAND_MTK)  += mtk_ecc.o mtk_nand.o
>> +obj-$(CONFIG_MTD_NAND_MESON)+= meson_nand.o
>>  
>>  nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
>>  nand-objs += nand_amd.o
>> diff --git a/drivers/mtd/nand/raw/meson_nand.c 
>> b/drivers/mtd/nand/raw/meson_nand.c
>> new file mode 100644
>> index ..28abc3684772
>> --- /dev/null
>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>> @@ -0,0 +1,1422 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson Nand Flash Controller Driver
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Liang Yang 
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include "clk-regmap.h"
>> +
>> +#define NFC_REG_CMD 0x00
>> +#define NFC_REG_CFG 0x04
>> +#define NFC_REG_DADR0x08
>> +#define NFC_REG_IADR0x0c
>> +#define NFC_REG_BUF 0x10
>> +#define NFC_REG_INFO0x14
>> +#define NFC_REG_DC  0x18
>> +#define NFC_REG_ADR 0x1c
>> +#define NFC_REG_DL  0x20
>> +#define NFC_REG_DH  0x24
>> +#define NFC_REG_CADR0x28
>> +#define NFC_REG_SADR0x2c
>> +#define NFC_REG_PINS0x30
>> +#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.


>> +
>> +#define NFC_CMD_DRD (0x8 << 14)
>> +#define NFC_CMD_IDLE(0xc << 14)
>> +#define NFC_CMD_DWR (0x4 << 14)
>> +#define NFC_CMD_CLE (0x5 << 14)
>> +#define NFC_CMD_ALE (0x6 << 14)
>> +#define NFC_CMD_ADL ((0 << 16) | (3 << 20))
>> +#define NFC_CMD_ADH ((1 << 16) | (3 << 20))
>> +#define NFC_CMD_AIL ((2 << 16) | (3 << 20))
>> +#define NFC_CMD_AIH ((3 << 16) | (3 << 20))
>> +#define NFC_CMD_SEED((8 << 16) | (3 << 20))
>> +#define NFC_CMD_M2N   

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

[Yixun Lan]

Hi Roris

thanks for all your suggestions!

It actually takes us some time to digest all your comments ;-)
and get back to you on these questions.


On 06/25/18 03:38, Boris Brezillon wrote:
> 
> 
> Hi Yixun,
> 
> On Wed, 13 Jun 2018 16:13:14 +
> Yixun Lan  wrote:
> 
>> From: Liang Yang 
>>
>> Add initial support for the Amlogic NAND flash controller which found
>> in the Meson-GXBB/GXL/AXG SoCs.
>>
>> Singed-off-by: Liang Yang 
>> Signed-off-by: Yixun Lan 
>> ---
>>  drivers/mtd/nand/raw/Kconfig  |8 +
>>  drivers/mtd/nand/raw/Makefile |3 +
>>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>>  3 files changed, 1433 insertions(+)
>>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c
> 
> Can you run checkpatch.pl --strict and fix the coding style issues?
> 
sure, we will be more cautious about this

>>
>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> index 19a2b283fbbe..b3c17a3ca8f4 100644
>> --- a/drivers/mtd/nand/raw/Kconfig
>> +++ b/drivers/mtd/nand/raw/Kconfig
>> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
>>Enables support for NAND controller on MTK SoCs.
>>This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>>  
>> +config MTD_NAND_MESON
>> +tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
>> +depends on ARCH_MESON || COMPILE_TEST
>> +select COMMON_CLK_REGMAP_MESON
>> +select MFD_SYSCON
>> +help
>> +  Enables support for NAND controller on Amlogic's Meson SoCs.
>> +
>>  endif # MTD_NAND
>> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
>> index 165b7ef9e9a1..cdf6162f38c3 100644
>> --- a/drivers/mtd/nand/raw/Makefile
>> +++ b/drivers/mtd/nand/raw/Makefile
>> @@ -1,5 +1,7 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  
>> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson
> 
> Please don't do that. If you need to expose common regs, put them
> in include/linux/soc/meson/. I'm also not sure why you need to access
> the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
> provider whose driver would be placed in drivers/clk and which would use
> the mmc syscon. This way the same clk driver could be used for both
> MMC and NAND clk indifferently, and the NAND driver would be much
> simpler.
> 

this is already addressed in another thread, as we will model it as a
standard clock driver.

so this cflags can be dropped.

>> +
>>  obj-$(CONFIG_MTD_NAND)  += nand.o
>>  obj-$(CONFIG_MTD_NAND_ECC)  += nand_ecc.o
>>  obj-$(CONFIG_MTD_NAND_BCH)  += nand_bch.o
>> @@ -56,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_HISI504) += 
>> hisi504_nand.o
>>  obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/
>>  obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o
>>  obj-$(CONFIG_MTD_NAND_MTK)  += mtk_ecc.o mtk_nand.o
>> +obj-$(CONFIG_MTD_NAND_MESON)+= meson_nand.o
>>  
>>  nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
>>  nand-objs += nand_amd.o
>> diff --git a/drivers/mtd/nand/raw/meson_nand.c 
>> b/drivers/mtd/nand/raw/meson_nand.c
>> new file mode 100644
>> index ..28abc3684772
>> --- /dev/null
>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>> @@ -0,0 +1,1422 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson Nand Flash Controller Driver
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Liang Yang 
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include "clk-regmap.h"
>> +
>> +#define NFC_REG_CMD 0x00
>> +#define NFC_REG_CFG 0x04
>> +#define NFC_REG_DADR0x08
>> +#define NFC_REG_IADR0x0c
>> +#define NFC_REG_BUF 0x10
>> +#define NFC_REG_INFO0x14
>> +#define NFC_REG_DC  0x18
>> +#define NFC_REG_ADR 0x1c
>> +#define NFC_REG_DL  0x20
>> +#define NFC_REG_DH  0x24
>> +#define NFC_REG_CADR0x28
>> +#define NFC_REG_SADR0x2c
>> +#define NFC_REG_PINS0x30
>> +#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.


>> +
>> +#define NFC_CMD_DRD (0x8 << 14)
>> +#define NFC_CMD_IDLE(0xc << 14)
>> +#define NFC_CMD_DWR (0x4 << 14)
>> +#define NFC_CMD_CLE (0x5 << 14)
>> +#define NFC_CMD_ALE (0x6 << 14)
>> +#define NFC_CMD_ADL ((0 << 16) | (3 << 20))
>> +#define NFC_CMD_ADH ((1 << 16) | (3 << 20))
>> +#define NFC_CMD_AIL ((2 << 16) | (3 << 20))
>> +#define NFC_CMD_AIH ((3 << 16) | (3 << 20))
>> +#define NFC_CMD_SEED((8 << 16) | (3 << 20))
>> +#define NFC_CMD_M2N   

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

[Yixun Lan]

HI Kevin

On 06/29/18 07:45, Kevin Hilman wrote:
> Hi Miquel,
> 
> Miquel Raynal  writes:
> 
>> On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman 
>> wrote:
>>
>>> Hi Boris,
>>>
>>> Boris Brezillon  writes:
>>>
 Hi Yixun,

 On Wed, 13 Jun 2018 16:13:14 +
 Yixun Lan  wrote:
  
> From: Liang Yang 
>
> Add initial support for the Amlogic NAND flash controller which found
> in the Meson-GXBB/GXL/AXG SoCs.
>
> Singed-off-by: Liang Yang 
> Signed-off-by: Yixun Lan 
> ---
>  drivers/mtd/nand/raw/Kconfig  |8 +
>  drivers/mtd/nand/raw/Makefile |3 +
>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>  3 files changed, 1433 insertions(+)
>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c  

 Can you run checkpatch.pl --strict and fix the coding style issues?
  
>
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 19a2b283fbbe..b3c17a3ca8f4 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
> Enables support for NAND controller on MTK SoCs.
> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>  
> +config MTD_NAND_MESON
> + tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
> + depends on ARCH_MESON || COMPILE_TEST
> + select COMMON_CLK_REGMAP_MESON
> + select MFD_SYSCON
> + help
> +   Enables support for NAND controller on Amlogic's Meson SoCs.
> +
>  endif # MTD_NAND
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 165b7ef9e9a1..cdf6162f38c3 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson  

 Please don't do that. If you need to expose common regs, put them
 in include/linux/soc/meson/. I'm also not sure why you need to access
 the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
 provider whose driver would be placed in drivers/clk and which would use
 the mmc syscon. This way the same clk driver could be used for both
 MMC and NAND clk indifferently, and the NAND driver would be much
 simpler.  
>>>
>>> [...]
>>>
> +
> + return 0;
> +}
> +
> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS];
> +
> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = SD_EMMC_CLOCK,
> + .mask = 0x3,
> + .shift = 6,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "sd_emmc_c_nand_clk_mux",
> + .ops = _regmap_mux_ops,
> + .parent_names = sd_emmc_ext_clk0_parent_names,
> + .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap sd_emmc_c_ext_clk0_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = SD_EMMC_CLOCK,
> + .shift = 0,
> + .width = 6,
> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "sd_emmc_c_nand_clk_div",
> + .ops = _regmap_divider_ops,
> + .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
> +{
> + struct clk_regmap *mux = _emmc_c_ext_clk0_sel;
> + struct clk_regmap *div = _emmc_c_ext_clk0_div;
> + struct clk *clk;
> + int i, ret;
> +
> + /* request core clock */
> + nfc->core_clk = devm_clk_get(nfc->dev, "core");
> + if (IS_ERR(nfc->core_clk)) {
> + dev_err(nfc->dev, "failed to get core clk\n");
> + return PTR_ERR(nfc->core_clk);
> + }
> +
> + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
> + regmap_update_bits(nfc->reg_clk, 0,
> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
> +
> + /* get the mux parents */
> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> + char name[16];
> +
> + snprintf(name, sizeof(name), "clkin%d", i);
> + clk = devm_clk_get(nfc->dev, name);
> + if (IS_ERR(clk)) {
> + if (clk != ERR_PTR(-EPROBE_DEFER))
> + dev_err(nfc->dev, "Missing clock %s\n", name);
> + return PTR_ERR(clk);
> + }
> 

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

[Yixun Lan]

HI Kevin

On 06/29/18 07:45, Kevin Hilman wrote:
> Hi Miquel,
> 
> Miquel Raynal  writes:
> 
>> On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman 
>> wrote:
>>
>>> Hi Boris,
>>>
>>> Boris Brezillon  writes:
>>>
 Hi Yixun,

 On Wed, 13 Jun 2018 16:13:14 +
 Yixun Lan  wrote:
  
> From: Liang Yang 
>
> Add initial support for the Amlogic NAND flash controller which found
> in the Meson-GXBB/GXL/AXG SoCs.
>
> Singed-off-by: Liang Yang 
> Signed-off-by: Yixun Lan 
> ---
>  drivers/mtd/nand/raw/Kconfig  |8 +
>  drivers/mtd/nand/raw/Makefile |3 +
>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>  3 files changed, 1433 insertions(+)
>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c  

 Can you run checkpatch.pl --strict and fix the coding style issues?
  
>
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 19a2b283fbbe..b3c17a3ca8f4 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
> Enables support for NAND controller on MTK SoCs.
> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>  
> +config MTD_NAND_MESON
> + tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
> + depends on ARCH_MESON || COMPILE_TEST
> + select COMMON_CLK_REGMAP_MESON
> + select MFD_SYSCON
> + help
> +   Enables support for NAND controller on Amlogic's Meson SoCs.
> +
>  endif # MTD_NAND
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 165b7ef9e9a1..cdf6162f38c3 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson  

 Please don't do that. If you need to expose common regs, put them
 in include/linux/soc/meson/. I'm also not sure why you need to access
 the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
 provider whose driver would be placed in drivers/clk and which would use
 the mmc syscon. This way the same clk driver could be used for both
 MMC and NAND clk indifferently, and the NAND driver would be much
 simpler.  
>>>
>>> [...]
>>>
> +
> + return 0;
> +}
> +
> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS];
> +
> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = SD_EMMC_CLOCK,
> + .mask = 0x3,
> + .shift = 6,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "sd_emmc_c_nand_clk_mux",
> + .ops = _regmap_mux_ops,
> + .parent_names = sd_emmc_ext_clk0_parent_names,
> + .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap sd_emmc_c_ext_clk0_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = SD_EMMC_CLOCK,
> + .shift = 0,
> + .width = 6,
> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "sd_emmc_c_nand_clk_div",
> + .ops = _regmap_divider_ops,
> + .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
> +{
> + struct clk_regmap *mux = _emmc_c_ext_clk0_sel;
> + struct clk_regmap *div = _emmc_c_ext_clk0_div;
> + struct clk *clk;
> + int i, ret;
> +
> + /* request core clock */
> + nfc->core_clk = devm_clk_get(nfc->dev, "core");
> + if (IS_ERR(nfc->core_clk)) {
> + dev_err(nfc->dev, "failed to get core clk\n");
> + return PTR_ERR(nfc->core_clk);
> + }
> +
> + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
> + regmap_update_bits(nfc->reg_clk, 0,
> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
> +
> + /* get the mux parents */
> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> + char name[16];
> +
> + snprintf(name, sizeof(name), "clkin%d", i);
> + clk = devm_clk_get(nfc->dev, name);
> + if (IS_ERR(clk)) {
> + if (clk != ERR_PTR(-EPROBE_DEFER))
> + dev_err(nfc->dev, "Missing clock %s\n", name);
> + return PTR_ERR(clk);
> + }
> 

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

[Neil Armstrong]

On 29/06/2018 01:45, Kevin Hilman wrote:
> Hi Miquel,
> 
> Miquel Raynal  writes:
> 
>> On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman 
>> wrote:
>>
>>> Hi Boris,
>>>
>>> Boris Brezillon  writes:
>>>
 Hi Yixun,

 On Wed, 13 Jun 2018 16:13:14 +
 Yixun Lan  wrote:
  
> From: Liang Yang 
>
> Add initial support for the Amlogic NAND flash controller which found
> in the Meson-GXBB/GXL/AXG SoCs.
>
> Singed-off-by: Liang Yang 
> Signed-off-by: Yixun Lan 
> ---
>  drivers/mtd/nand/raw/Kconfig  |8 +
>  drivers/mtd/nand/raw/Makefile |3 +
>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>  3 files changed, 1433 insertions(+)
>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c  

 Can you run checkpatch.pl --strict and fix the coding style issues?
  
>
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 19a2b283fbbe..b3c17a3ca8f4 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
> Enables support for NAND controller on MTK SoCs.
> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>  
> +config MTD_NAND_MESON
> + tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
> + depends on ARCH_MESON || COMPILE_TEST
> + select COMMON_CLK_REGMAP_MESON
> + select MFD_SYSCON
> + help
> +   Enables support for NAND controller on Amlogic's Meson SoCs.
> +
>  endif # MTD_NAND
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 165b7ef9e9a1..cdf6162f38c3 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson  

 Please don't do that. If you need to expose common regs, put them
 in include/linux/soc/meson/. I'm also not sure why you need to access
 the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
 provider whose driver would be placed in drivers/clk and which would use
 the mmc syscon. This way the same clk driver could be used for both
 MMC and NAND clk indifferently, and the NAND driver would be much
 simpler.  
>>>
>>> [...]
>>>
> +
> + return 0;
> +}
> +
> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS];
> +
> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = SD_EMMC_CLOCK,
> + .mask = 0x3,
> + .shift = 6,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "sd_emmc_c_nand_clk_mux",
> + .ops = _regmap_mux_ops,
> + .parent_names = sd_emmc_ext_clk0_parent_names,
> + .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap sd_emmc_c_ext_clk0_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = SD_EMMC_CLOCK,
> + .shift = 0,
> + .width = 6,
> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "sd_emmc_c_nand_clk_div",
> + .ops = _regmap_divider_ops,
> + .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
> +{
> + struct clk_regmap *mux = _emmc_c_ext_clk0_sel;
> + struct clk_regmap *div = _emmc_c_ext_clk0_div;
> + struct clk *clk;
> + int i, ret;
> +
> + /* request core clock */
> + nfc->core_clk = devm_clk_get(nfc->dev, "core");
> + if (IS_ERR(nfc->core_clk)) {
> + dev_err(nfc->dev, "failed to get core clk\n");
> + return PTR_ERR(nfc->core_clk);
> + }
> +
> + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
> + regmap_update_bits(nfc->reg_clk, 0,
> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
> +
> + /* get the mux parents */
> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> + char name[16];
> +
> + snprintf(name, sizeof(name), "clkin%d", i);
> + clk = devm_clk_get(nfc->dev, name);
> + if (IS_ERR(clk)) {
> + if (clk != ERR_PTR(-EPROBE_DEFER))
> + dev_err(nfc->dev, "Missing clock %s\n", name);
> + return PTR_ERR(clk);
> + }
> +
> 

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

[Neil Armstrong]

On 29/06/2018 01:45, Kevin Hilman wrote:
> Hi Miquel,
> 
> Miquel Raynal  writes:
> 
>> On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman 
>> wrote:
>>
>>> Hi Boris,
>>>
>>> Boris Brezillon  writes:
>>>
 Hi Yixun,

 On Wed, 13 Jun 2018 16:13:14 +
 Yixun Lan  wrote:
  
> From: Liang Yang 
>
> Add initial support for the Amlogic NAND flash controller which found
> in the Meson-GXBB/GXL/AXG SoCs.
>
> Singed-off-by: Liang Yang 
> Signed-off-by: Yixun Lan 
> ---
>  drivers/mtd/nand/raw/Kconfig  |8 +
>  drivers/mtd/nand/raw/Makefile |3 +
>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>  3 files changed, 1433 insertions(+)
>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c  

 Can you run checkpatch.pl --strict and fix the coding style issues?
  
>
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 19a2b283fbbe..b3c17a3ca8f4 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
> Enables support for NAND controller on MTK SoCs.
> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>  
> +config MTD_NAND_MESON
> + tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
> + depends on ARCH_MESON || COMPILE_TEST
> + select COMMON_CLK_REGMAP_MESON
> + select MFD_SYSCON
> + help
> +   Enables support for NAND controller on Amlogic's Meson SoCs.
> +
>  endif # MTD_NAND
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 165b7ef9e9a1..cdf6162f38c3 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson  

 Please don't do that. If you need to expose common regs, put them
 in include/linux/soc/meson/. I'm also not sure why you need to access
 the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
 provider whose driver would be placed in drivers/clk and which would use
 the mmc syscon. This way the same clk driver could be used for both
 MMC and NAND clk indifferently, and the NAND driver would be much
 simpler.  
>>>
>>> [...]
>>>
> +
> + return 0;
> +}
> +
> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS];
> +
> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = SD_EMMC_CLOCK,
> + .mask = 0x3,
> + .shift = 6,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "sd_emmc_c_nand_clk_mux",
> + .ops = _regmap_mux_ops,
> + .parent_names = sd_emmc_ext_clk0_parent_names,
> + .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap sd_emmc_c_ext_clk0_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = SD_EMMC_CLOCK,
> + .shift = 0,
> + .width = 6,
> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "sd_emmc_c_nand_clk_div",
> + .ops = _regmap_divider_ops,
> + .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
> +{
> + struct clk_regmap *mux = _emmc_c_ext_clk0_sel;
> + struct clk_regmap *div = _emmc_c_ext_clk0_div;
> + struct clk *clk;
> + int i, ret;
> +
> + /* request core clock */
> + nfc->core_clk = devm_clk_get(nfc->dev, "core");
> + if (IS_ERR(nfc->core_clk)) {
> + dev_err(nfc->dev, "failed to get core clk\n");
> + return PTR_ERR(nfc->core_clk);
> + }
> +
> + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
> + regmap_update_bits(nfc->reg_clk, 0,
> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
> +
> + /* get the mux parents */
> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> + char name[16];
> +
> + snprintf(name, sizeof(name), "clkin%d", i);
> + clk = devm_clk_get(nfc->dev, name);
> + if (IS_ERR(clk)) {
> + if (clk != ERR_PTR(-EPROBE_DEFER))
> + dev_err(nfc->dev, "Missing clock %s\n", name);
> + return PTR_ERR(clk);
> + }
> +
> 

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

[Kevin Hilman]

Hi Miquel,

Miquel Raynal  writes:

> On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman 
> wrote:
>
>> Hi Boris,
>> 
>> Boris Brezillon  writes:
>> 
>> > Hi Yixun,
>> >
>> > On Wed, 13 Jun 2018 16:13:14 +
>> > Yixun Lan  wrote:
>> >  
>> >> From: Liang Yang 
>> >> 
>> >> Add initial support for the Amlogic NAND flash controller which found
>> >> in the Meson-GXBB/GXL/AXG SoCs.
>> >> 
>> >> Singed-off-by: Liang Yang 
>> >> Signed-off-by: Yixun Lan 
>> >> ---
>> >>  drivers/mtd/nand/raw/Kconfig  |8 +
>> >>  drivers/mtd/nand/raw/Makefile |3 +
>> >>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>> >>  3 files changed, 1433 insertions(+)
>> >>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c  
>> >
>> > Can you run checkpatch.pl --strict and fix the coding style issues?
>> >  
>> >> 
>> >> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> >> index 19a2b283fbbe..b3c17a3ca8f4 100644
>> >> --- a/drivers/mtd/nand/raw/Kconfig
>> >> +++ b/drivers/mtd/nand/raw/Kconfig
>> >> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
>> >> Enables support for NAND controller on MTK SoCs.
>> >> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>> >>  
>> >> +config MTD_NAND_MESON
>> >> + tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
>> >> + depends on ARCH_MESON || COMPILE_TEST
>> >> + select COMMON_CLK_REGMAP_MESON
>> >> + select MFD_SYSCON
>> >> + help
>> >> +   Enables support for NAND controller on Amlogic's Meson SoCs.
>> >> +
>> >>  endif # MTD_NAND
>> >> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
>> >> index 165b7ef9e9a1..cdf6162f38c3 100644
>> >> --- a/drivers/mtd/nand/raw/Makefile
>> >> +++ b/drivers/mtd/nand/raw/Makefile
>> >> @@ -1,5 +1,7 @@
>> >>  # SPDX-License-Identifier: GPL-2.0
>> >>  
>> >> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson  
>> >
>> > Please don't do that. If you need to expose common regs, put them
>> > in include/linux/soc/meson/. I'm also not sure why you need to access
>> > the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
>> > provider whose driver would be placed in drivers/clk and which would use
>> > the mmc syscon. This way the same clk driver could be used for both
>> > MMC and NAND clk indifferently, and the NAND driver would be much
>> > simpler.  
>> 
>> [...]
>> 
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS];
>> >> +
>> >> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = {
>> >> + .data = &(struct clk_regmap_mux_data){
>> >> + .offset = SD_EMMC_CLOCK,
>> >> + .mask = 0x3,
>> >> + .shift = 6,
>> >> + },
>> >> + .hw.init = &(struct clk_init_data) {
>> >> + .name = "sd_emmc_c_nand_clk_mux",
>> >> + .ops = _regmap_mux_ops,
>> >> + .parent_names = sd_emmc_ext_clk0_parent_names,
>> >> + .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
>> >> + .flags = CLK_SET_RATE_PARENT,
>> >> + },
>> >> +};
>> >> +
>> >> +static struct clk_regmap sd_emmc_c_ext_clk0_div = {
>> >> + .data = &(struct clk_regmap_div_data){
>> >> + .offset = SD_EMMC_CLOCK,
>> >> + .shift = 0,
>> >> + .width = 6,
>> >> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
>> >> + },
>> >> + .hw.init = &(struct clk_init_data) {
>> >> + .name = "sd_emmc_c_nand_clk_div",
>> >> + .ops = _regmap_divider_ops,
>> >> + .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" },
>> >> + .num_parents = 1,
>> >> + .flags = CLK_SET_RATE_PARENT,
>> >> + },
>> >> +};
>> >> +
>> >> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
>> >> +{
>> >> + struct clk_regmap *mux = _emmc_c_ext_clk0_sel;
>> >> + struct clk_regmap *div = _emmc_c_ext_clk0_div;
>> >> + struct clk *clk;
>> >> + int i, ret;
>> >> +
>> >> + /* request core clock */
>> >> + nfc->core_clk = devm_clk_get(nfc->dev, "core");
>> >> + if (IS_ERR(nfc->core_clk)) {
>> >> + dev_err(nfc->dev, "failed to get core clk\n");
>> >> + return PTR_ERR(nfc->core_clk);
>> >> + }
>> >> +
>> >> + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
>> >> + regmap_update_bits(nfc->reg_clk, 0,
>> >> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
>> >> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
>> >> +
>> >> + /* get the mux parents */
>> >> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
>> >> + char name[16];
>> >> +
>> >> + snprintf(name, sizeof(name), "clkin%d", i);
>> >> + clk = devm_clk_get(nfc->dev, name);
>> >> + if (IS_ERR(clk)) {
>> >> + if (clk != ERR_PTR(-EPROBE_DEFER))
>> >> + dev_err(nfc->dev, "Missing clock %s\n", name);
>> >> + return PTR_ERR(clk);
>> >> + }
>> >> +
>> >> + sd_emmc_ext_clk0_parent_names[i] = 

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

[Kevin Hilman]

Hi Miquel,

Miquel Raynal  writes:

> On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman 
> wrote:
>
>> Hi Boris,
>> 
>> Boris Brezillon  writes:
>> 
>> > Hi Yixun,
>> >
>> > On Wed, 13 Jun 2018 16:13:14 +
>> > Yixun Lan  wrote:
>> >  
>> >> From: Liang Yang 
>> >> 
>> >> Add initial support for the Amlogic NAND flash controller which found
>> >> in the Meson-GXBB/GXL/AXG SoCs.
>> >> 
>> >> Singed-off-by: Liang Yang 
>> >> Signed-off-by: Yixun Lan 
>> >> ---
>> >>  drivers/mtd/nand/raw/Kconfig  |8 +
>> >>  drivers/mtd/nand/raw/Makefile |3 +
>> >>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>> >>  3 files changed, 1433 insertions(+)
>> >>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c  
>> >
>> > Can you run checkpatch.pl --strict and fix the coding style issues?
>> >  
>> >> 
>> >> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> >> index 19a2b283fbbe..b3c17a3ca8f4 100644
>> >> --- a/drivers/mtd/nand/raw/Kconfig
>> >> +++ b/drivers/mtd/nand/raw/Kconfig
>> >> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
>> >> Enables support for NAND controller on MTK SoCs.
>> >> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>> >>  
>> >> +config MTD_NAND_MESON
>> >> + tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
>> >> + depends on ARCH_MESON || COMPILE_TEST
>> >> + select COMMON_CLK_REGMAP_MESON
>> >> + select MFD_SYSCON
>> >> + help
>> >> +   Enables support for NAND controller on Amlogic's Meson SoCs.
>> >> +
>> >>  endif # MTD_NAND
>> >> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
>> >> index 165b7ef9e9a1..cdf6162f38c3 100644
>> >> --- a/drivers/mtd/nand/raw/Makefile
>> >> +++ b/drivers/mtd/nand/raw/Makefile
>> >> @@ -1,5 +1,7 @@
>> >>  # SPDX-License-Identifier: GPL-2.0
>> >>  
>> >> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson  
>> >
>> > Please don't do that. If you need to expose common regs, put them
>> > in include/linux/soc/meson/. I'm also not sure why you need to access
>> > the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
>> > provider whose driver would be placed in drivers/clk and which would use
>> > the mmc syscon. This way the same clk driver could be used for both
>> > MMC and NAND clk indifferently, and the NAND driver would be much
>> > simpler.  
>> 
>> [...]
>> 
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS];
>> >> +
>> >> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = {
>> >> + .data = &(struct clk_regmap_mux_data){
>> >> + .offset = SD_EMMC_CLOCK,
>> >> + .mask = 0x3,
>> >> + .shift = 6,
>> >> + },
>> >> + .hw.init = &(struct clk_init_data) {
>> >> + .name = "sd_emmc_c_nand_clk_mux",
>> >> + .ops = _regmap_mux_ops,
>> >> + .parent_names = sd_emmc_ext_clk0_parent_names,
>> >> + .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
>> >> + .flags = CLK_SET_RATE_PARENT,
>> >> + },
>> >> +};
>> >> +
>> >> +static struct clk_regmap sd_emmc_c_ext_clk0_div = {
>> >> + .data = &(struct clk_regmap_div_data){
>> >> + .offset = SD_EMMC_CLOCK,
>> >> + .shift = 0,
>> >> + .width = 6,
>> >> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
>> >> + },
>> >> + .hw.init = &(struct clk_init_data) {
>> >> + .name = "sd_emmc_c_nand_clk_div",
>> >> + .ops = _regmap_divider_ops,
>> >> + .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" },
>> >> + .num_parents = 1,
>> >> + .flags = CLK_SET_RATE_PARENT,
>> >> + },
>> >> +};
>> >> +
>> >> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
>> >> +{
>> >> + struct clk_regmap *mux = _emmc_c_ext_clk0_sel;
>> >> + struct clk_regmap *div = _emmc_c_ext_clk0_div;
>> >> + struct clk *clk;
>> >> + int i, ret;
>> >> +
>> >> + /* request core clock */
>> >> + nfc->core_clk = devm_clk_get(nfc->dev, "core");
>> >> + if (IS_ERR(nfc->core_clk)) {
>> >> + dev_err(nfc->dev, "failed to get core clk\n");
>> >> + return PTR_ERR(nfc->core_clk);
>> >> + }
>> >> +
>> >> + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
>> >> + regmap_update_bits(nfc->reg_clk, 0,
>> >> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
>> >> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
>> >> +
>> >> + /* get the mux parents */
>> >> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
>> >> + char name[16];
>> >> +
>> >> + snprintf(name, sizeof(name), "clkin%d", i);
>> >> + clk = devm_clk_get(nfc->dev, name);
>> >> + if (IS_ERR(clk)) {
>> >> + if (clk != ERR_PTR(-EPROBE_DEFER))
>> >> + dev_err(nfc->dev, "Missing clock %s\n", name);
>> >> + return PTR_ERR(clk);
>> >> + }
>> >> +
>> >> + sd_emmc_ext_clk0_parent_names[i] = 

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

[Miquel Raynal]

Hi Kevin,

On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman 
wrote:

> Hi Boris,
> 
> Boris Brezillon  writes:
> 
> > Hi Yixun,
> >
> > On Wed, 13 Jun 2018 16:13:14 +
> > Yixun Lan  wrote:
> >  
> >> From: Liang Yang 
> >> 
> >> Add initial support for the Amlogic NAND flash controller which found
> >> in the Meson-GXBB/GXL/AXG SoCs.
> >> 
> >> Singed-off-by: Liang Yang 
> >> Signed-off-by: Yixun Lan 
> >> ---
> >>  drivers/mtd/nand/raw/Kconfig  |8 +
> >>  drivers/mtd/nand/raw/Makefile |3 +
> >>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
> >>  3 files changed, 1433 insertions(+)
> >>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c  
> >
> > Can you run checkpatch.pl --strict and fix the coding style issues?
> >  
> >> 
> >> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> >> index 19a2b283fbbe..b3c17a3ca8f4 100644
> >> --- a/drivers/mtd/nand/raw/Kconfig
> >> +++ b/drivers/mtd/nand/raw/Kconfig
> >> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
> >>  Enables support for NAND controller on MTK SoCs.
> >>  This controller is found on mt27xx, mt81xx, mt65xx SoCs.
> >>  
> >> +config MTD_NAND_MESON
> >> +  tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
> >> +  depends on ARCH_MESON || COMPILE_TEST
> >> +  select COMMON_CLK_REGMAP_MESON
> >> +  select MFD_SYSCON
> >> +  help
> >> +Enables support for NAND controller on Amlogic's Meson SoCs.
> >> +
> >>  endif # MTD_NAND
> >> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> >> index 165b7ef9e9a1..cdf6162f38c3 100644
> >> --- a/drivers/mtd/nand/raw/Makefile
> >> +++ b/drivers/mtd/nand/raw/Makefile
> >> @@ -1,5 +1,7 @@
> >>  # SPDX-License-Identifier: GPL-2.0
> >>  
> >> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson  
> >
> > Please don't do that. If you need to expose common regs, put them
> > in include/linux/soc/meson/. I'm also not sure why you need to access
> > the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
> > provider whose driver would be placed in drivers/clk and which would use
> > the mmc syscon. This way the same clk driver could be used for both
> > MMC and NAND clk indifferently, and the NAND driver would be much
> > simpler.  
> 
> [...]
> 
> >> +
> >> +  return 0;
> >> +}
> >> +
> >> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS];
> >> +
> >> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = {
> >> +  .data = &(struct clk_regmap_mux_data){
> >> +  .offset = SD_EMMC_CLOCK,
> >> +  .mask = 0x3,
> >> +  .shift = 6,
> >> +  },
> >> +  .hw.init = &(struct clk_init_data) {
> >> +  .name = "sd_emmc_c_nand_clk_mux",
> >> +  .ops = _regmap_mux_ops,
> >> +  .parent_names = sd_emmc_ext_clk0_parent_names,
> >> +  .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
> >> +  .flags = CLK_SET_RATE_PARENT,
> >> +  },
> >> +};
> >> +
> >> +static struct clk_regmap sd_emmc_c_ext_clk0_div = {
> >> +  .data = &(struct clk_regmap_div_data){
> >> +  .offset = SD_EMMC_CLOCK,
> >> +  .shift = 0,
> >> +  .width = 6,
> >> +  .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> >> +  },
> >> +  .hw.init = &(struct clk_init_data) {
> >> +  .name = "sd_emmc_c_nand_clk_div",
> >> +  .ops = _regmap_divider_ops,
> >> +  .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" },
> >> +  .num_parents = 1,
> >> +  .flags = CLK_SET_RATE_PARENT,
> >> +  },
> >> +};
> >> +
> >> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
> >> +{
> >> +  struct clk_regmap *mux = _emmc_c_ext_clk0_sel;
> >> +  struct clk_regmap *div = _emmc_c_ext_clk0_div;
> >> +  struct clk *clk;
> >> +  int i, ret;
> >> +
> >> +  /* request core clock */
> >> +  nfc->core_clk = devm_clk_get(nfc->dev, "core");
> >> +  if (IS_ERR(nfc->core_clk)) {
> >> +  dev_err(nfc->dev, "failed to get core clk\n");
> >> +  return PTR_ERR(nfc->core_clk);
> >> +  }
> >> +
> >> +  /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
> >> +  regmap_update_bits(nfc->reg_clk, 0,
> >> +  CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
> >> +  CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
> >> +
> >> +  /* get the mux parents */
> >> +  for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> >> +  char name[16];
> >> +
> >> +  snprintf(name, sizeof(name), "clkin%d", i);
> >> +  clk = devm_clk_get(nfc->dev, name);
> >> +  if (IS_ERR(clk)) {
> >> +  if (clk != ERR_PTR(-EPROBE_DEFER))
> >> +  dev_err(nfc->dev, "Missing clock %s\n", name);
> >> +  return PTR_ERR(clk);
> >> +  }
> >> +
> >> +  sd_emmc_ext_clk0_parent_names[i] = __clk_get_name(clk);
> >> +  }
> >> +
> >> +  mux->map = nfc->reg_clk;
> >> +  clk = 

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

[Miquel Raynal]

Hi Kevin,

On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman 
wrote:

> Hi Boris,
> 
> Boris Brezillon  writes:
> 
> > Hi Yixun,
> >
> > On Wed, 13 Jun 2018 16:13:14 +
> > Yixun Lan  wrote:
> >  
> >> From: Liang Yang 
> >> 
> >> Add initial support for the Amlogic NAND flash controller which found
> >> in the Meson-GXBB/GXL/AXG SoCs.
> >> 
> >> Singed-off-by: Liang Yang 
> >> Signed-off-by: Yixun Lan 
> >> ---
> >>  drivers/mtd/nand/raw/Kconfig  |8 +
> >>  drivers/mtd/nand/raw/Makefile |3 +
> >>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
> >>  3 files changed, 1433 insertions(+)
> >>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c  
> >
> > Can you run checkpatch.pl --strict and fix the coding style issues?
> >  
> >> 
> >> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> >> index 19a2b283fbbe..b3c17a3ca8f4 100644
> >> --- a/drivers/mtd/nand/raw/Kconfig
> >> +++ b/drivers/mtd/nand/raw/Kconfig
> >> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
> >>  Enables support for NAND controller on MTK SoCs.
> >>  This controller is found on mt27xx, mt81xx, mt65xx SoCs.
> >>  
> >> +config MTD_NAND_MESON
> >> +  tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
> >> +  depends on ARCH_MESON || COMPILE_TEST
> >> +  select COMMON_CLK_REGMAP_MESON
> >> +  select MFD_SYSCON
> >> +  help
> >> +Enables support for NAND controller on Amlogic's Meson SoCs.
> >> +
> >>  endif # MTD_NAND
> >> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> >> index 165b7ef9e9a1..cdf6162f38c3 100644
> >> --- a/drivers/mtd/nand/raw/Makefile
> >> +++ b/drivers/mtd/nand/raw/Makefile
> >> @@ -1,5 +1,7 @@
> >>  # SPDX-License-Identifier: GPL-2.0
> >>  
> >> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson  
> >
> > Please don't do that. If you need to expose common regs, put them
> > in include/linux/soc/meson/. I'm also not sure why you need to access
> > the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
> > provider whose driver would be placed in drivers/clk and which would use
> > the mmc syscon. This way the same clk driver could be used for both
> > MMC and NAND clk indifferently, and the NAND driver would be much
> > simpler.  
> 
> [...]
> 
> >> +
> >> +  return 0;
> >> +}
> >> +
> >> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS];
> >> +
> >> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = {
> >> +  .data = &(struct clk_regmap_mux_data){
> >> +  .offset = SD_EMMC_CLOCK,
> >> +  .mask = 0x3,
> >> +  .shift = 6,
> >> +  },
> >> +  .hw.init = &(struct clk_init_data) {
> >> +  .name = "sd_emmc_c_nand_clk_mux",
> >> +  .ops = _regmap_mux_ops,
> >> +  .parent_names = sd_emmc_ext_clk0_parent_names,
> >> +  .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
> >> +  .flags = CLK_SET_RATE_PARENT,
> >> +  },
> >> +};
> >> +
> >> +static struct clk_regmap sd_emmc_c_ext_clk0_div = {
> >> +  .data = &(struct clk_regmap_div_data){
> >> +  .offset = SD_EMMC_CLOCK,
> >> +  .shift = 0,
> >> +  .width = 6,
> >> +  .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> >> +  },
> >> +  .hw.init = &(struct clk_init_data) {
> >> +  .name = "sd_emmc_c_nand_clk_div",
> >> +  .ops = _regmap_divider_ops,
> >> +  .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" },
> >> +  .num_parents = 1,
> >> +  .flags = CLK_SET_RATE_PARENT,
> >> +  },
> >> +};
> >> +
> >> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
> >> +{
> >> +  struct clk_regmap *mux = _emmc_c_ext_clk0_sel;
> >> +  struct clk_regmap *div = _emmc_c_ext_clk0_div;
> >> +  struct clk *clk;
> >> +  int i, ret;
> >> +
> >> +  /* request core clock */
> >> +  nfc->core_clk = devm_clk_get(nfc->dev, "core");
> >> +  if (IS_ERR(nfc->core_clk)) {
> >> +  dev_err(nfc->dev, "failed to get core clk\n");
> >> +  return PTR_ERR(nfc->core_clk);
> >> +  }
> >> +
> >> +  /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
> >> +  regmap_update_bits(nfc->reg_clk, 0,
> >> +  CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
> >> +  CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
> >> +
> >> +  /* get the mux parents */
> >> +  for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> >> +  char name[16];
> >> +
> >> +  snprintf(name, sizeof(name), "clkin%d", i);
> >> +  clk = devm_clk_get(nfc->dev, name);
> >> +  if (IS_ERR(clk)) {
> >> +  if (clk != ERR_PTR(-EPROBE_DEFER))
> >> +  dev_err(nfc->dev, "Missing clock %s\n", name);
> >> +  return PTR_ERR(clk);
> >> +  }
> >> +
> >> +  sd_emmc_ext_clk0_parent_names[i] = __clk_get_name(clk);
> >> +  }
> >> +
> >> +  mux->map = nfc->reg_clk;
> >> +  clk = 

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

[Kevin Hilman]

Hi Boris,

Boris Brezillon  writes:

> Hi Yixun,
>
> On Wed, 13 Jun 2018 16:13:14 +
> Yixun Lan  wrote:
>
>> From: Liang Yang 
>> 
>> Add initial support for the Amlogic NAND flash controller which found
>> in the Meson-GXBB/GXL/AXG SoCs.
>> 
>> Singed-off-by: Liang Yang 
>> Signed-off-by: Yixun Lan 
>> ---
>>  drivers/mtd/nand/raw/Kconfig  |8 +
>>  drivers/mtd/nand/raw/Makefile |3 +
>>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>>  3 files changed, 1433 insertions(+)
>>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c
>
> Can you run checkpatch.pl --strict and fix the coding style issues?
>
>> 
>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> index 19a2b283fbbe..b3c17a3ca8f4 100644
>> --- a/drivers/mtd/nand/raw/Kconfig
>> +++ b/drivers/mtd/nand/raw/Kconfig
>> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
>>Enables support for NAND controller on MTK SoCs.
>>This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>>  
>> +config MTD_NAND_MESON
>> +tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
>> +depends on ARCH_MESON || COMPILE_TEST
>> +select COMMON_CLK_REGMAP_MESON
>> +select MFD_SYSCON
>> +help
>> +  Enables support for NAND controller on Amlogic's Meson SoCs.
>> +
>>  endif # MTD_NAND
>> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
>> index 165b7ef9e9a1..cdf6162f38c3 100644
>> --- a/drivers/mtd/nand/raw/Makefile
>> +++ b/drivers/mtd/nand/raw/Makefile
>> @@ -1,5 +1,7 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  
>> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson
>
> Please don't do that. If you need to expose common regs, put them
> in include/linux/soc/meson/. I'm also not sure why you need to access
> the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
> provider whose driver would be placed in drivers/clk and which would use
> the mmc syscon. This way the same clk driver could be used for both
> MMC and NAND clk indifferently, and the NAND driver would be much
> simpler.

[...]

>> +
>> +return 0;
>> +}
>> +
>> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS];
>> +
>> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = {
>> +.data = &(struct clk_regmap_mux_data){
>> +.offset = SD_EMMC_CLOCK,
>> +.mask = 0x3,
>> +.shift = 6,
>> +},
>> +.hw.init = &(struct clk_init_data) {
>> +.name = "sd_emmc_c_nand_clk_mux",
>> +.ops = _regmap_mux_ops,
>> +.parent_names = sd_emmc_ext_clk0_parent_names,
>> +.num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
>> +.flags = CLK_SET_RATE_PARENT,
>> +},
>> +};
>> +
>> +static struct clk_regmap sd_emmc_c_ext_clk0_div = {
>> +.data = &(struct clk_regmap_div_data){
>> +.offset = SD_EMMC_CLOCK,
>> +.shift = 0,
>> +.width = 6,
>> +.flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
>> +},
>> +.hw.init = &(struct clk_init_data) {
>> +.name = "sd_emmc_c_nand_clk_div",
>> +.ops = _regmap_divider_ops,
>> +.parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" },
>> +.num_parents = 1,
>> +.flags = CLK_SET_RATE_PARENT,
>> +},
>> +};
>> +
>> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
>> +{
>> +struct clk_regmap *mux = _emmc_c_ext_clk0_sel;
>> +struct clk_regmap *div = _emmc_c_ext_clk0_div;
>> +struct clk *clk;
>> +int i, ret;
>> +
>> +/* request core clock */
>> +nfc->core_clk = devm_clk_get(nfc->dev, "core");
>> +if (IS_ERR(nfc->core_clk)) {
>> +dev_err(nfc->dev, "failed to get core clk\n");
>> +return PTR_ERR(nfc->core_clk);
>> +}
>> +
>> +/* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
>> +regmap_update_bits(nfc->reg_clk, 0,
>> +CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
>> +CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
>> +
>> +/* get the mux parents */
>> +for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
>> +char name[16];
>> +
>> +snprintf(name, sizeof(name), "clkin%d", i);
>> +clk = devm_clk_get(nfc->dev, name);
>> +if (IS_ERR(clk)) {
>> +if (clk != ERR_PTR(-EPROBE_DEFER))
>> +dev_err(nfc->dev, "Missing clock %s\n", name);
>> +return PTR_ERR(clk);
>> +}
>> +
>> +sd_emmc_ext_clk0_parent_names[i] = __clk_get_name(clk);
>> +}
>> +
>> +mux->map = nfc->reg_clk;
>> +clk = devm_clk_register(nfc->dev, >hw);
>> +if (WARN_ON(IS_ERR(clk)))
>> +return PTR_ERR(clk);
>> +
>> +div->map = nfc->reg_clk;
>> +nfc->device_clk = devm_clk_register(nfc->dev, >hw);
>> +if (WARN_ON(IS_ERR(nfc->device_clk)))

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

[Kevin Hilman]

Hi Boris,

Boris Brezillon  writes:

> Hi Yixun,
>
> On Wed, 13 Jun 2018 16:13:14 +
> Yixun Lan  wrote:
>
>> From: Liang Yang 
>> 
>> Add initial support for the Amlogic NAND flash controller which found
>> in the Meson-GXBB/GXL/AXG SoCs.
>> 
>> Singed-off-by: Liang Yang 
>> Signed-off-by: Yixun Lan 
>> ---
>>  drivers/mtd/nand/raw/Kconfig  |8 +
>>  drivers/mtd/nand/raw/Makefile |3 +
>>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>>  3 files changed, 1433 insertions(+)
>>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c
>
> Can you run checkpatch.pl --strict and fix the coding style issues?
>
>> 
>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> index 19a2b283fbbe..b3c17a3ca8f4 100644
>> --- a/drivers/mtd/nand/raw/Kconfig
>> +++ b/drivers/mtd/nand/raw/Kconfig
>> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
>>Enables support for NAND controller on MTK SoCs.
>>This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>>  
>> +config MTD_NAND_MESON
>> +tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
>> +depends on ARCH_MESON || COMPILE_TEST
>> +select COMMON_CLK_REGMAP_MESON
>> +select MFD_SYSCON
>> +help
>> +  Enables support for NAND controller on Amlogic's Meson SoCs.
>> +
>>  endif # MTD_NAND
>> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
>> index 165b7ef9e9a1..cdf6162f38c3 100644
>> --- a/drivers/mtd/nand/raw/Makefile
>> +++ b/drivers/mtd/nand/raw/Makefile
>> @@ -1,5 +1,7 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  
>> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson
>
> Please don't do that. If you need to expose common regs, put them
> in include/linux/soc/meson/. I'm also not sure why you need to access
> the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
> provider whose driver would be placed in drivers/clk and which would use
> the mmc syscon. This way the same clk driver could be used for both
> MMC and NAND clk indifferently, and the NAND driver would be much
> simpler.

[...]

>> +
>> +return 0;
>> +}
>> +
>> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS];
>> +
>> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = {
>> +.data = &(struct clk_regmap_mux_data){
>> +.offset = SD_EMMC_CLOCK,
>> +.mask = 0x3,
>> +.shift = 6,
>> +},
>> +.hw.init = &(struct clk_init_data) {
>> +.name = "sd_emmc_c_nand_clk_mux",
>> +.ops = _regmap_mux_ops,
>> +.parent_names = sd_emmc_ext_clk0_parent_names,
>> +.num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
>> +.flags = CLK_SET_RATE_PARENT,
>> +},
>> +};
>> +
>> +static struct clk_regmap sd_emmc_c_ext_clk0_div = {
>> +.data = &(struct clk_regmap_div_data){
>> +.offset = SD_EMMC_CLOCK,
>> +.shift = 0,
>> +.width = 6,
>> +.flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
>> +},
>> +.hw.init = &(struct clk_init_data) {
>> +.name = "sd_emmc_c_nand_clk_div",
>> +.ops = _regmap_divider_ops,
>> +.parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" },
>> +.num_parents = 1,
>> +.flags = CLK_SET_RATE_PARENT,
>> +},
>> +};
>> +
>> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
>> +{
>> +struct clk_regmap *mux = _emmc_c_ext_clk0_sel;
>> +struct clk_regmap *div = _emmc_c_ext_clk0_div;
>> +struct clk *clk;
>> +int i, ret;
>> +
>> +/* request core clock */
>> +nfc->core_clk = devm_clk_get(nfc->dev, "core");
>> +if (IS_ERR(nfc->core_clk)) {
>> +dev_err(nfc->dev, "failed to get core clk\n");
>> +return PTR_ERR(nfc->core_clk);
>> +}
>> +
>> +/* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
>> +regmap_update_bits(nfc->reg_clk, 0,
>> +CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
>> +CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
>> +
>> +/* get the mux parents */
>> +for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
>> +char name[16];
>> +
>> +snprintf(name, sizeof(name), "clkin%d", i);
>> +clk = devm_clk_get(nfc->dev, name);
>> +if (IS_ERR(clk)) {
>> +if (clk != ERR_PTR(-EPROBE_DEFER))
>> +dev_err(nfc->dev, "Missing clock %s\n", name);
>> +return PTR_ERR(clk);
>> +}
>> +
>> +sd_emmc_ext_clk0_parent_names[i] = __clk_get_name(clk);
>> +}
>> +
>> +mux->map = nfc->reg_clk;
>> +clk = devm_clk_register(nfc->dev, >hw);
>> +if (WARN_ON(IS_ERR(clk)))
>> +return PTR_ERR(clk);
>> +
>> +div->map = nfc->reg_clk;
>> +nfc->device_clk = devm_clk_register(nfc->dev, >hw);
>> +if (WARN_ON(IS_ERR(nfc->device_clk)))

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

[Boris Brezillon]

Hi Yixun,

On Wed, 13 Jun 2018 16:13:14 +
Yixun Lan  wrote:

> From: Liang Yang 
> 
> Add initial support for the Amlogic NAND flash controller which found
> in the Meson-GXBB/GXL/AXG SoCs.
> 
> Singed-off-by: Liang Yang 
> Signed-off-by: Yixun Lan 
> ---
>  drivers/mtd/nand/raw/Kconfig  |8 +
>  drivers/mtd/nand/raw/Makefile |3 +
>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>  3 files changed, 1433 insertions(+)
>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c

Can you run checkpatch.pl --strict and fix the coding style issues?

> 
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 19a2b283fbbe..b3c17a3ca8f4 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
> Enables support for NAND controller on MTK SoCs.
> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>  
> +config MTD_NAND_MESON
> + tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
> + depends on ARCH_MESON || COMPILE_TEST
> + select COMMON_CLK_REGMAP_MESON
> + select MFD_SYSCON
> + help
> +   Enables support for NAND controller on Amlogic's Meson SoCs.
> +
>  endif # MTD_NAND
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 165b7ef9e9a1..cdf6162f38c3 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson

Please don't do that. If you need to expose common regs, put them
in include/linux/soc/meson/. I'm also not sure why you need to access
the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
provider whose driver would be placed in drivers/clk and which would use
the mmc syscon. This way the same clk driver could be used for both
MMC and NAND clk indifferently, and the NAND driver would be much
simpler.

> +
>  obj-$(CONFIG_MTD_NAND)   += nand.o
>  obj-$(CONFIG_MTD_NAND_ECC)   += nand_ecc.o
>  obj-$(CONFIG_MTD_NAND_BCH)   += nand_bch.o
> @@ -56,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_HISI504)  += 
> hisi504_nand.o
>  obj-$(CONFIG_MTD_NAND_BRCMNAND)  += brcmnand/
>  obj-$(CONFIG_MTD_NAND_QCOM)  += qcom_nandc.o
>  obj-$(CONFIG_MTD_NAND_MTK)   += mtk_ecc.o mtk_nand.o
> +obj-$(CONFIG_MTD_NAND_MESON) += meson_nand.o
>  
>  nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
>  nand-objs += nand_amd.o
> diff --git a/drivers/mtd/nand/raw/meson_nand.c 
> b/drivers/mtd/nand/raw/meson_nand.c
> new file mode 100644
> index ..28abc3684772
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -0,0 +1,1422 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson Nand Flash Controller Driver
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Liang Yang 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "clk-regmap.h"
> +
> +#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?

> +
> +#define NFC_CMD_DRD  (0x8 << 14)
> +#define NFC_CMD_IDLE (0xc << 14)
> +#define NFC_CMD_DWR  (0x4 << 14)
> +#define NFC_CMD_CLE  (0x5 << 14)
> +#define NFC_CMD_ALE  (0x6 << 14)
> +#define NFC_CMD_ADL  ((0 << 16) | (3 << 20))
> +#define NFC_CMD_ADH  ((1 << 16) | (3 << 20))
> +#define NFC_CMD_AIL  ((2 << 16) | (3 << 20))
> +#define NFC_CMD_AIH  ((3 << 16) | (3 << 20))
> +#define NFC_CMD_SEED ((8 << 16) | (3 << 20))
> +#define NFC_CMD_M2N  ((0 << 17) | (2 << 20))
> +#define NFC_CMD_N2M  ((1 << 17) | (2 << 20))
> +#define NFC_CMD_RB   (1 << 20)
> +#define NFC_CMD_IO6  ((0xb << 10) | (1 << 18))
> +
> +#define NFC_RB_USED  (1 << 23)
> +#define NFC_LARGE_PAGE   (1 << 22)
> +#define NFC_RW_OPS   (2 << 20)
> +
> +#define NAND_TWB_TIME_CYCLE  10
> +
> +#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages)\
> + (   \
> + (cmd_dir)   |   \
> + ((ran) << 19)   |   \
> + 

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

[Boris Brezillon]

Hi Yixun,

On Wed, 13 Jun 2018 16:13:14 +
Yixun Lan  wrote:

> From: Liang Yang 
> 
> Add initial support for the Amlogic NAND flash controller which found
> in the Meson-GXBB/GXL/AXG SoCs.
> 
> Singed-off-by: Liang Yang 
> Signed-off-by: Yixun Lan 
> ---
>  drivers/mtd/nand/raw/Kconfig  |8 +
>  drivers/mtd/nand/raw/Makefile |3 +
>  drivers/mtd/nand/raw/meson_nand.c | 1422 +
>  3 files changed, 1433 insertions(+)
>  create mode 100644 drivers/mtd/nand/raw/meson_nand.c

Can you run checkpatch.pl --strict and fix the coding style issues?

> 
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 19a2b283fbbe..b3c17a3ca8f4 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -534,4 +534,12 @@ config MTD_NAND_MTK
> Enables support for NAND controller on MTK SoCs.
> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>  
> +config MTD_NAND_MESON
> + tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
> + depends on ARCH_MESON || COMPILE_TEST
> + select COMMON_CLK_REGMAP_MESON
> + select MFD_SYSCON
> + help
> +   Enables support for NAND controller on Amlogic's Meson SoCs.
> +
>  endif # MTD_NAND
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 165b7ef9e9a1..cdf6162f38c3 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson

Please don't do that. If you need to expose common regs, put them
in include/linux/soc/meson/. I'm also not sure why you need to access
the clk regs directly. Why can't you expose the MMC/NAND clk as a clk
provider whose driver would be placed in drivers/clk and which would use
the mmc syscon. This way the same clk driver could be used for both
MMC and NAND clk indifferently, and the NAND driver would be much
simpler.

> +
>  obj-$(CONFIG_MTD_NAND)   += nand.o
>  obj-$(CONFIG_MTD_NAND_ECC)   += nand_ecc.o
>  obj-$(CONFIG_MTD_NAND_BCH)   += nand_bch.o
> @@ -56,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_HISI504)  += 
> hisi504_nand.o
>  obj-$(CONFIG_MTD_NAND_BRCMNAND)  += brcmnand/
>  obj-$(CONFIG_MTD_NAND_QCOM)  += qcom_nandc.o
>  obj-$(CONFIG_MTD_NAND_MTK)   += mtk_ecc.o mtk_nand.o
> +obj-$(CONFIG_MTD_NAND_MESON) += meson_nand.o
>  
>  nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
>  nand-objs += nand_amd.o
> diff --git a/drivers/mtd/nand/raw/meson_nand.c 
> b/drivers/mtd/nand/raw/meson_nand.c
> new file mode 100644
> index ..28abc3684772
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -0,0 +1,1422 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson Nand Flash Controller Driver
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Liang Yang 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "clk-regmap.h"
> +
> +#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?

> +
> +#define NFC_CMD_DRD  (0x8 << 14)
> +#define NFC_CMD_IDLE (0xc << 14)
> +#define NFC_CMD_DWR  (0x4 << 14)
> +#define NFC_CMD_CLE  (0x5 << 14)
> +#define NFC_CMD_ALE  (0x6 << 14)
> +#define NFC_CMD_ADL  ((0 << 16) | (3 << 20))
> +#define NFC_CMD_ADH  ((1 << 16) | (3 << 20))
> +#define NFC_CMD_AIL  ((2 << 16) | (3 << 20))
> +#define NFC_CMD_AIH  ((3 << 16) | (3 << 20))
> +#define NFC_CMD_SEED ((8 << 16) | (3 << 20))
> +#define NFC_CMD_M2N  ((0 << 17) | (2 << 20))
> +#define NFC_CMD_N2M  ((1 << 17) | (2 << 20))
> +#define NFC_CMD_RB   (1 << 20)
> +#define NFC_CMD_IO6  ((0xb << 10) | (1 << 18))
> +
> +#define NFC_RB_USED  (1 << 23)
> +#define NFC_LARGE_PAGE   (1 << 22)
> +#define NFC_RW_OPS   (2 << 20)
> +
> +#define NAND_TWB_TIME_CYCLE  10
> +
> +#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages)\
> + (   \
> + (cmd_dir)   |   \
> + ((ran) << 19)   |   \
> + 

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

[kbuild test robot]

Hi Liang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mtd/nand/next]
[also build test ERROR on v4.17 next-20180613]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Yixun-Lan/mtd-rawnand-meson-add-Amlogic-NAND-driver-support/20180613-161917
base:   git://git.infradead.org/linux-mtd.git nand/next
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

>> drivers/mtd/nand/raw/meson_nand.c:21:10: fatal error: clk-regmap.h: No such 
>> file or directory
#include "clk-regmap.h"
 ^~
   compilation terminated.

vim +21 drivers/mtd/nand/raw/meson_nand.c

  > 21  #include "clk-regmap.h"
22  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

[kbuild test robot]

Hi Liang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mtd/nand/next]
[also build test ERROR on v4.17 next-20180613]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Yixun-Lan/mtd-rawnand-meson-add-Amlogic-NAND-driver-support/20180613-161917
base:   git://git.infradead.org/linux-mtd.git nand/next
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

>> drivers/mtd/nand/raw/meson_nand.c:21:10: fatal error: clk-regmap.h: No such 
>> file or directory
#include "clk-regmap.h"
 ^~
   compilation terminated.

vim +21 drivers/mtd/nand/raw/meson_nand.c

  > 21  #include "clk-regmap.h"
22  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

[kbuild test robot]

Hi Liang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mtd/nand/next]
[also build test ERROR on v4.17 next-20180613]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Yixun-Lan/mtd-rawnand-meson-add-Amlogic-NAND-driver-support/20180613-161917
base:   git://git.infradead.org/linux-mtd.git nand/next
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=sparc64 

All error/warnings (new ones prefixed by >>):

   In file included from drivers/mtd/nand/raw/meson_nand.c:21:0:
>> drivers/clk/meson/clk-regmap.h:22:16: error: field 'hw' has incomplete type
 struct clk_hw hw;
   ^~
>> drivers/mtd/nand/raw/meson_nand.c:951:2: error: field name not in record or 
>> union initializer
 .hw.init = &(struct clk_init_data) {
 ^
   drivers/mtd/nand/raw/meson_nand.c:951:2: note: (near initialization for 
'sd_emmc_c_ext_clk0_sel')
>> drivers/mtd/nand/raw/meson_nand.c:952:4: error: 'struct clk_init_data' has 
>> no member named 'name'
  .name = "sd_emmc_c_nand_clk_mux",
   ^~~~
>> drivers/mtd/nand/raw/meson_nand.c:952:11: warning: excess elements in struct 
>> initializer
  .name = "sd_emmc_c_nand_clk_mux",
  ^~~~
   drivers/mtd/nand/raw/meson_nand.c:952:11: note: (near initialization for 
'(anonymous)')
>> drivers/mtd/nand/raw/meson_nand.c:953:4: error: 'struct clk_init_data' has 
>> no member named 'ops'
  .ops = _regmap_mux_ops,
   ^~~
   drivers/mtd/nand/raw/meson_nand.c:953:10: warning: excess elements in struct 
initializer
  .ops = _regmap_mux_ops,
 ^
   drivers/mtd/nand/raw/meson_nand.c:953:10: note: (near initialization for 
'(anonymous)')
>> drivers/mtd/nand/raw/meson_nand.c:954:4: error: 'struct clk_init_data' has 
>> no member named 'parent_names'
  .parent_names = sd_emmc_ext_clk0_parent_names,
   ^~~~
   drivers/mtd/nand/raw/meson_nand.c:954:19: warning: excess elements in struct 
initializer
  .parent_names = sd_emmc_ext_clk0_parent_names,
  ^
   drivers/mtd/nand/raw/meson_nand.c:954:19: note: (near initialization for 
'(anonymous)')
>> drivers/mtd/nand/raw/meson_nand.c:955:4: error: 'struct clk_init_data' has 
>> no member named 'num_parents'
  .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
   ^~~
   In file included from include/linux/list.h:9:0,
from include/linux/kobject.h:19,
from include/linux/device.h:16,
from include/linux/platform_device.h:14,
from drivers/mtd/nand/raw/meson_nand.c:9:
>> include/linux/kernel.h:71:25: warning: excess elements in struct initializer
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))
^
>> drivers/mtd/nand/raw/meson_nand.c:955:18: note: in expansion of macro 
>> 'ARRAY_SIZE'
  .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
 ^~
   include/linux/kernel.h:71:25: note: (near initialization for '(anonymous)')
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))
^
>> drivers/mtd/nand/raw/meson_nand.c:955:18: note: in expansion of macro 
>> 'ARRAY_SIZE'
  .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
 ^~
>> drivers/mtd/nand/raw/meson_nand.c:956:4: error: 'struct clk_init_data' has 
>> no member named 'flags'
  .flags = CLK_SET_RATE_PARENT,
   ^
>> drivers/mtd/nand/raw/meson_nand.c:956:12: error: 'CLK_SET_RATE_PARENT' 
>> undeclared here (not in a function); did you mean 'DL_STATE_DORMANT'?
  .flags = CLK_SET_RATE_PARENT,
   ^~~
   DL_STATE_DORMANT
   drivers/mtd/nand/raw/meson_nand.c:956:12: warning: excess elements in struct 
initializer
   drivers/mtd/nand/raw/meson_nand.c:956:12: note: (near initialization for 
'(anonymous)')
>> drivers/mtd/nand/raw/meson_nand.c:951:37: error: invalid use of undefined 
>> type 'struct clk_init_data'
 .hw.init = &(struct clk_init_data) {
^
>> drivers/mtd/nand/raw/meson_nand.c:965:12: error: 'CLK_DIVIDER_ROUND_CLOSEST' 
>> undeclared here (not in a function); did you mean 'DIV_ROUND_CLOSEST'?
  .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
   ^
   DIV_ROUND_CLOSEST
>> drivers/mtd/nand/raw/meson_nand.c:965:40: error: 'CLK_DIVIDER_ONE_BASED' 
>> undeclared here (not in a function); 

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

[kbuild test robot]

Hi Liang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mtd/nand/next]
[also build test ERROR on v4.17 next-20180613]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Yixun-Lan/mtd-rawnand-meson-add-Amlogic-NAND-driver-support/20180613-161917
base:   git://git.infradead.org/linux-mtd.git nand/next
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=sparc64 

All error/warnings (new ones prefixed by >>):

   In file included from drivers/mtd/nand/raw/meson_nand.c:21:0:
>> drivers/clk/meson/clk-regmap.h:22:16: error: field 'hw' has incomplete type
 struct clk_hw hw;
   ^~
>> drivers/mtd/nand/raw/meson_nand.c:951:2: error: field name not in record or 
>> union initializer
 .hw.init = &(struct clk_init_data) {
 ^
   drivers/mtd/nand/raw/meson_nand.c:951:2: note: (near initialization for 
'sd_emmc_c_ext_clk0_sel')
>> drivers/mtd/nand/raw/meson_nand.c:952:4: error: 'struct clk_init_data' has 
>> no member named 'name'
  .name = "sd_emmc_c_nand_clk_mux",
   ^~~~
>> drivers/mtd/nand/raw/meson_nand.c:952:11: warning: excess elements in struct 
>> initializer
  .name = "sd_emmc_c_nand_clk_mux",
  ^~~~
   drivers/mtd/nand/raw/meson_nand.c:952:11: note: (near initialization for 
'(anonymous)')
>> drivers/mtd/nand/raw/meson_nand.c:953:4: error: 'struct clk_init_data' has 
>> no member named 'ops'
  .ops = _regmap_mux_ops,
   ^~~
   drivers/mtd/nand/raw/meson_nand.c:953:10: warning: excess elements in struct 
initializer
  .ops = _regmap_mux_ops,
 ^
   drivers/mtd/nand/raw/meson_nand.c:953:10: note: (near initialization for 
'(anonymous)')
>> drivers/mtd/nand/raw/meson_nand.c:954:4: error: 'struct clk_init_data' has 
>> no member named 'parent_names'
  .parent_names = sd_emmc_ext_clk0_parent_names,
   ^~~~
   drivers/mtd/nand/raw/meson_nand.c:954:19: warning: excess elements in struct 
initializer
  .parent_names = sd_emmc_ext_clk0_parent_names,
  ^
   drivers/mtd/nand/raw/meson_nand.c:954:19: note: (near initialization for 
'(anonymous)')
>> drivers/mtd/nand/raw/meson_nand.c:955:4: error: 'struct clk_init_data' has 
>> no member named 'num_parents'
  .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
   ^~~
   In file included from include/linux/list.h:9:0,
from include/linux/kobject.h:19,
from include/linux/device.h:16,
from include/linux/platform_device.h:14,
from drivers/mtd/nand/raw/meson_nand.c:9:
>> include/linux/kernel.h:71:25: warning: excess elements in struct initializer
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))
^
>> drivers/mtd/nand/raw/meson_nand.c:955:18: note: in expansion of macro 
>> 'ARRAY_SIZE'
  .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
 ^~
   include/linux/kernel.h:71:25: note: (near initialization for '(anonymous)')
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))
^
>> drivers/mtd/nand/raw/meson_nand.c:955:18: note: in expansion of macro 
>> 'ARRAY_SIZE'
  .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names),
 ^~
>> drivers/mtd/nand/raw/meson_nand.c:956:4: error: 'struct clk_init_data' has 
>> no member named 'flags'
  .flags = CLK_SET_RATE_PARENT,
   ^
>> drivers/mtd/nand/raw/meson_nand.c:956:12: error: 'CLK_SET_RATE_PARENT' 
>> undeclared here (not in a function); did you mean 'DL_STATE_DORMANT'?
  .flags = CLK_SET_RATE_PARENT,
   ^~~
   DL_STATE_DORMANT
   drivers/mtd/nand/raw/meson_nand.c:956:12: warning: excess elements in struct 
initializer
   drivers/mtd/nand/raw/meson_nand.c:956:12: note: (near initialization for 
'(anonymous)')
>> drivers/mtd/nand/raw/meson_nand.c:951:37: error: invalid use of undefined 
>> type 'struct clk_init_data'
 .hw.init = &(struct clk_init_data) {
^
>> drivers/mtd/nand/raw/meson_nand.c:965:12: error: 'CLK_DIVIDER_ROUND_CLOSEST' 
>> undeclared here (not in a function); did you mean 'DIV_ROUND_CLOSEST'?
  .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
   ^
   DIV_ROUND_CLOSEST
>> drivers/mtd/nand/raw/meson_nand.c:965:40: error: 'CLK_DIVIDER_ONE_BASED' 
>> undeclared here (not in a function); 

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

[Yixun Lan]

From: Liang Yang 

Add initial support for the Amlogic NAND flash controller which found
in the Meson-GXBB/GXL/AXG SoCs.

Singed-off-by: Liang Yang 
Signed-off-by: Yixun Lan 
---
 drivers/mtd/nand/raw/Kconfig  |8 +
 drivers/mtd/nand/raw/Makefile |3 +
 drivers/mtd/nand/raw/meson_nand.c | 1422 +
 3 files changed, 1433 insertions(+)
 create mode 100644 drivers/mtd/nand/raw/meson_nand.c

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index 19a2b283fbbe..b3c17a3ca8f4 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -534,4 +534,12 @@ config MTD_NAND_MTK
  Enables support for NAND controller on MTK SoCs.
  This controller is found on mt27xx, mt81xx, mt65xx SoCs.
 
+config MTD_NAND_MESON
+   tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
+   depends on ARCH_MESON || COMPILE_TEST
+   select COMMON_CLK_REGMAP_MESON
+   select MFD_SYSCON
+   help
+ Enables support for NAND controller on Amlogic's Meson SoCs.
+
 endif # MTD_NAND
diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 165b7ef9e9a1..cdf6162f38c3 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
+ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson
+
 obj-$(CONFIG_MTD_NAND) += nand.o
 obj-$(CONFIG_MTD_NAND_ECC) += nand_ecc.o
 obj-$(CONFIG_MTD_NAND_BCH) += nand_bch.o
@@ -56,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_HISI504)+= 
hisi504_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)+= brcmnand/
 obj-$(CONFIG_MTD_NAND_QCOM)+= qcom_nandc.o
 obj-$(CONFIG_MTD_NAND_MTK) += mtk_ecc.o mtk_nand.o
+obj-$(CONFIG_MTD_NAND_MESON)   += meson_nand.o
 
 nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
 nand-objs += nand_amd.o
diff --git a/drivers/mtd/nand/raw/meson_nand.c 
b/drivers/mtd/nand/raw/meson_nand.c
new file mode 100644
index ..28abc3684772
--- /dev/null
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -0,0 +1,1422 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson Nand Flash Controller Driver
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Liang Yang 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "clk-regmap.h"
+
+#define NFC_REG_CMD0x00
+#define NFC_REG_CFG0x04
+#define NFC_REG_DADR   0x08
+#define NFC_REG_IADR   0x0c
+#define NFC_REG_BUF0x10
+#define NFC_REG_INFO   0x14
+#define NFC_REG_DC 0x18
+#define NFC_REG_ADR0x1c
+#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_VER0x38
+
+
+#define NFC_CMD_DRD(0x8 << 14)
+#define NFC_CMD_IDLE   (0xc << 14)
+#define NFC_CMD_DWR(0x4 << 14)
+#define NFC_CMD_CLE(0x5 << 14)
+#define NFC_CMD_ALE(0x6 << 14)
+#define NFC_CMD_ADL((0 << 16) | (3 << 20))
+#define NFC_CMD_ADH((1 << 16) | (3 << 20))
+#define NFC_CMD_AIL((2 << 16) | (3 << 20))
+#define NFC_CMD_AIH((3 << 16) | (3 << 20))
+#define NFC_CMD_SEED   ((8 << 16) | (3 << 20))
+#define NFC_CMD_M2N((0 << 17) | (2 << 20))
+#define NFC_CMD_N2M((1 << 17) | (2 << 20))
+#define NFC_CMD_RB (1 << 20)
+#define NFC_CMD_IO6((0xb << 10) | (1 << 18))
+
+#define NFC_RB_USED(1 << 23)
+#define NFC_LARGE_PAGE (1 << 22)
+#define NFC_RW_OPS (2 << 20)
+
+#define NAND_TWB_TIME_CYCLE10
+
+#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages)  \
+   (   \
+   (cmd_dir)   |   \
+   ((ran) << 19)   |   \
+   ((bch) << 14)   |   \
+   ((short_mode) << 13)|   \
+   (((page_size) & 0x7f) << 6) |   \
+   ((pages) & 0x3f)\
+   )
+
+#define GENCMDDADDRL(adl, addr)((adl) | ((addr) & 0x))
+#define GENCMDDADDRH(adh, addr)((adh) | (((addr) >> 16) & 
0x))
+#define GENCMDIADDRL(ail, addr)((ail) | ((addr) & 0x))
+#define GENCMDIADDRH(aih, addr)((aih) | (((addr) >> 16) & 
0x))
+
+#define RB_STA(x)  (1 << (26 + x))
+
+#define ECC_CHECK_RETURN_FF(-1)
+
+#define NAND_CE0   (0xe << 10)
+#define NAND_CE1   

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

[Yixun Lan]

From: Liang Yang 

Add initial support for the Amlogic NAND flash controller which found
in the Meson-GXBB/GXL/AXG SoCs.

Singed-off-by: Liang Yang 
Signed-off-by: Yixun Lan 
---
 drivers/mtd/nand/raw/Kconfig  |8 +
 drivers/mtd/nand/raw/Makefile |3 +
 drivers/mtd/nand/raw/meson_nand.c | 1422 +
 3 files changed, 1433 insertions(+)
 create mode 100644 drivers/mtd/nand/raw/meson_nand.c

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index 19a2b283fbbe..b3c17a3ca8f4 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -534,4 +534,12 @@ config MTD_NAND_MTK
  Enables support for NAND controller on MTK SoCs.
  This controller is found on mt27xx, mt81xx, mt65xx SoCs.
 
+config MTD_NAND_MESON
+   tristate "Support for NAND flash controller on Amlogic's Meson SoCs"
+   depends on ARCH_MESON || COMPILE_TEST
+   select COMMON_CLK_REGMAP_MESON
+   select MFD_SYSCON
+   help
+ Enables support for NAND controller on Amlogic's Meson SoCs.
+
 endif # MTD_NAND
diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 165b7ef9e9a1..cdf6162f38c3 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
+ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson
+
 obj-$(CONFIG_MTD_NAND) += nand.o
 obj-$(CONFIG_MTD_NAND_ECC) += nand_ecc.o
 obj-$(CONFIG_MTD_NAND_BCH) += nand_bch.o
@@ -56,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_HISI504)+= 
hisi504_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)+= brcmnand/
 obj-$(CONFIG_MTD_NAND_QCOM)+= qcom_nandc.o
 obj-$(CONFIG_MTD_NAND_MTK) += mtk_ecc.o mtk_nand.o
+obj-$(CONFIG_MTD_NAND_MESON)   += meson_nand.o
 
 nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
 nand-objs += nand_amd.o
diff --git a/drivers/mtd/nand/raw/meson_nand.c 
b/drivers/mtd/nand/raw/meson_nand.c
new file mode 100644
index ..28abc3684772
--- /dev/null
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -0,0 +1,1422 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson Nand Flash Controller Driver
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Liang Yang 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "clk-regmap.h"
+
+#define NFC_REG_CMD0x00
+#define NFC_REG_CFG0x04
+#define NFC_REG_DADR   0x08
+#define NFC_REG_IADR   0x0c
+#define NFC_REG_BUF0x10
+#define NFC_REG_INFO   0x14
+#define NFC_REG_DC 0x18
+#define NFC_REG_ADR0x1c
+#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_VER0x38
+
+
+#define NFC_CMD_DRD(0x8 << 14)
+#define NFC_CMD_IDLE   (0xc << 14)
+#define NFC_CMD_DWR(0x4 << 14)
+#define NFC_CMD_CLE(0x5 << 14)
+#define NFC_CMD_ALE(0x6 << 14)
+#define NFC_CMD_ADL((0 << 16) | (3 << 20))
+#define NFC_CMD_ADH((1 << 16) | (3 << 20))
+#define NFC_CMD_AIL((2 << 16) | (3 << 20))
+#define NFC_CMD_AIH((3 << 16) | (3 << 20))
+#define NFC_CMD_SEED   ((8 << 16) | (3 << 20))
+#define NFC_CMD_M2N((0 << 17) | (2 << 20))
+#define NFC_CMD_N2M((1 << 17) | (2 << 20))
+#define NFC_CMD_RB (1 << 20)
+#define NFC_CMD_IO6((0xb << 10) | (1 << 18))
+
+#define NFC_RB_USED(1 << 23)
+#define NFC_LARGE_PAGE (1 << 22)
+#define NFC_RW_OPS (2 << 20)
+
+#define NAND_TWB_TIME_CYCLE10
+
+#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages)  \
+   (   \
+   (cmd_dir)   |   \
+   ((ran) << 19)   |   \
+   ((bch) << 14)   |   \
+   ((short_mode) << 13)|   \
+   (((page_size) & 0x7f) << 6) |   \
+   ((pages) & 0x3f)\
+   )
+
+#define GENCMDDADDRL(adl, addr)((adl) | ((addr) & 0x))
+#define GENCMDDADDRH(adh, addr)((adh) | (((addr) >> 16) & 
0x))
+#define GENCMDIADDRL(ail, addr)((ail) | ((addr) & 0x))
+#define GENCMDIADDRH(aih, addr)((aih) | (((addr) >> 16) & 
0x))
+
+#define RB_STA(x)  (1 << (26 + x))
+
+#define ECC_CHECK_RETURN_FF(-1)
+
+#define NAND_CE0   (0xe << 10)
+#define NAND_CE1