Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris and Miquel, understand. i will move helpers into nfc driver to avoid some errors when sending the patch. On 2018/11/15 21:09, Boris Brezillon wrote: On Thu, 15 Nov 2018 14:04:00 +0100 Miquel Raynal wrote: Hi Liang, Liang Yang wrote on Thu, 15 Nov 2018 19:25:07 +0800: Hi Boris, I have implemented dma access base on these helpers you provided below. we prepare to send v7 version now, so when will these helpers be pushed? Thanks for your work. You can send the v7 so we will have a look at the overall driver; but since we raised the DMA buffers issue we had a discussion with Boris about how to handle them and I think we are going to adopt the same solution as Wolfram in the I2C subsystem: manual flagging. Sadly, this is probably the best we can do to ensure proper DMA support. There is nothing set is stone yet but I started a small rework to handle MTD operations differently (and add a DMA_SAFE flag), you can have a look there [1]. Don't base your work on it for now as it is just a preliminary version, subject to big changes. In order to not block the driver, I'd suggest that you move the helper I proposed directly into your driver and prefix them with 'meson_'. .
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris and Miquel, understand. i will move helpers into nfc driver to avoid some errors when sending the patch. On 2018/11/15 21:09, Boris Brezillon wrote: On Thu, 15 Nov 2018 14:04:00 +0100 Miquel Raynal wrote: Hi Liang, Liang Yang wrote on Thu, 15 Nov 2018 19:25:07 +0800: Hi Boris, I have implemented dma access base on these helpers you provided below. we prepare to send v7 version now, so when will these helpers be pushed? Thanks for your work. You can send the v7 so we will have a look at the overall driver; but since we raised the DMA buffers issue we had a discussion with Boris about how to handle them and I think we are going to adopt the same solution as Wolfram in the I2C subsystem: manual flagging. Sadly, this is probably the best we can do to ensure proper DMA support. There is nothing set is stone yet but I started a small rework to handle MTD operations differently (and add a DMA_SAFE flag), you can have a look there [1]. Don't base your work on it for now as it is just a preliminary version, subject to big changes. In order to not block the driver, I'd suggest that you move the helper I proposed directly into your driver and prefix them with 'meson_'. .
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Thu, 15 Nov 2018 14:04:00 +0100 Miquel Raynal wrote: > Hi Liang, > > Liang Yang wrote on Thu, 15 Nov 2018 19:25:07 > +0800: > > > Hi Boris, > > > > I have implemented dma access base on these helpers you provided below. > > we prepare to send v7 version now, so when will these helpers be pushed? > > Thanks for your work. You can send the v7 so we will have a look at the > overall driver; but since we raised the DMA buffers issue we had a > discussion with Boris about how to handle them and I think we are going > to adopt the same solution as Wolfram in the I2C subsystem: manual > flagging. Sadly, this is probably the best we can do to ensure proper > DMA support. > > There is nothing set is stone yet but I started a small rework to > handle MTD operations differently (and add a DMA_SAFE flag), you can > have a look there [1]. Don't base your work on it for now as it is just > a preliminary version, subject to big changes. In order to not block the driver, I'd suggest that you move the helper I proposed directly into your driver and prefix them with 'meson_'.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Thu, 15 Nov 2018 14:04:00 +0100 Miquel Raynal wrote: > Hi Liang, > > Liang Yang wrote on Thu, 15 Nov 2018 19:25:07 > +0800: > > > Hi Boris, > > > > I have implemented dma access base on these helpers you provided below. > > we prepare to send v7 version now, so when will these helpers be pushed? > > Thanks for your work. You can send the v7 so we will have a look at the > overall driver; but since we raised the DMA buffers issue we had a > discussion with Boris about how to handle them and I think we are going > to adopt the same solution as Wolfram in the I2C subsystem: manual > flagging. Sadly, this is probably the best we can do to ensure proper > DMA support. > > There is nothing set is stone yet but I started a small rework to > handle MTD operations differently (and add a DMA_SAFE flag), you can > have a look there [1]. Don't base your work on it for now as it is just > a preliminary version, subject to big changes. In order to not block the driver, I'd suggest that you move the helper I proposed directly into your driver and prefix them with 'meson_'.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Liang, Liang Yang wrote on Thu, 15 Nov 2018 19:25:07 +0800: > Hi Boris, > > I have implemented dma access base on these helpers you provided below. > we prepare to send v7 version now, so when will these helpers be pushed? Thanks for your work. You can send the v7 so we will have a look at the overall driver; but since we raised the DMA buffers issue we had a discussion with Boris about how to handle them and I think we are going to adopt the same solution as Wolfram in the I2C subsystem: manual flagging. Sadly, this is probably the best we can do to ensure proper DMA support. There is nothing set is stone yet but I started a small rework to handle MTD operations differently (and add a DMA_SAFE flag), you can have a look there [1]. Don't base your work on it for now as it is just a preliminary version, subject to big changes. [1] https://github.com/miquelraynal/linux/commits/dma-safe-buffers Thanks, Miquèl
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Liang, Liang Yang wrote on Thu, 15 Nov 2018 19:25:07 +0800: > Hi Boris, > > I have implemented dma access base on these helpers you provided below. > we prepare to send v7 version now, so when will these helpers be pushed? Thanks for your work. You can send the v7 so we will have a look at the overall driver; but since we raised the DMA buffers issue we had a discussion with Boris about how to handle them and I think we are going to adopt the same solution as Wolfram in the I2C subsystem: manual flagging. Sadly, this is probably the best we can do to ensure proper DMA support. There is nothing set is stone yet but I started a small rework to handle MTD operations differently (and add a DMA_SAFE flag), you can have a look there [1]. Don't base your work on it for now as it is just a preliminary version, subject to big changes. [1] https://github.com/miquelraynal/linux/commits/dma-safe-buffers Thanks, Miquèl
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris,
I have implemented dma access base on these helpers you provided below.
we prepare to send v7 version now, so when will these helpers be pushed?
On 2018/11/13 1:45, Boris Brezillon wrote:
On Mon, 12 Nov 2018 17:54:16 +0100
Boris Brezillon wrote:
+Wolfram to give some inputs on the DMA issue.
On Mon, 12 Nov 2018 17:13:51 +0100
Miquel Raynal wrote:
Hello,
Boris Brezillon wrote on Tue, 6 Nov 2018
11:22:06 +0100:
On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang wrote:
On 2018/11/6 17:28, Boris Brezillon wrote:
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ u32 cmd;
+
+ cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+ meson_nfc_drain_cmd(nfc);
You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
nand cycle to read one byte and covers the 1st byte every time reading.
i think nfc controller is faster than nand cycle, but really it is not
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
ok, i will try dma here.
We should probably expose the bounce buf handling as generic helpers at
the rawnand level:
void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
return NULL;
if (virt_addr_valid(instr->data.in) &&
!object_is_on_stack(instr->data.buf.in))
return instr->data.buf.in;
return kzalloc(instr->data.len, GFP_KERNEL);
}
void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
WARN_ON(!buf))
return;
if (buf == instr->data.buf.in)
return;
memcpy(instr->data.buf.in, buf, instr->data.len);
kfree(buf);
}
const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
return NULL;
if (virt_addr_valid(instr->data.out) &&
!object_is_on_stack(instr->data.buf.out))
return instr->data.buf.out;
return kmemdup(instr->data.buf.out, GFP_KERNEL);
}
void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
WARN_ON(!buf))
return;
if (buf != instr->data.buf.out)
kfree(buf);
}
Not that I am against such function, but maybe they should come with
comments stating that there is no reliable way to find if a buffer is
DMA-able at runtime and these are just sanity checks (ie. required, but
probably not enough).
It's not 100% reliable, but it should cover most cases. Note that the
NAND framework already uses virt_addr_valid() to decide when to use its
internal bounce buffer, so this should be fixed too if we want a fully
reliable solution.
This is my understanding of Wolfram's recent talk
at ELCE [1].
Yes, you're right, but the NAND framework does not provide any guarantee
on the buf passed to ->exec_op() simply because the MTD layer does not
provide such a guarantee. Reworking that to match how the i2c framework
handles it is possible (with a flag set when the buffer is known to be
DMA-safe), but it requires rewriting all MTD users if we want to keep
decent perfs (the amount of data transfered to a flash is an order of
magnitude bigger than what you usually receive/send from/to an I2C
device). Also, I'm not even sure the DMA_SAFE flag covers all weird
cases like the "DMA engine embedded in the controller is not able to
access the whole physical memory range" one.
I forgot that this problem was handled at dma_map time (a bounce
buffer is allocated if needed, and this decision is based on
dev->dma_mask).
So ideally we should have
something that checks if a pointer is DMA-safe at the device level and
then at the arch level.
A temporary solution would be to add a hook at the nand_controller
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris,
I have implemented dma access base on these helpers you provided below.
we prepare to send v7 version now, so when will these helpers be pushed?
On 2018/11/13 1:45, Boris Brezillon wrote:
On Mon, 12 Nov 2018 17:54:16 +0100
Boris Brezillon wrote:
+Wolfram to give some inputs on the DMA issue.
On Mon, 12 Nov 2018 17:13:51 +0100
Miquel Raynal wrote:
Hello,
Boris Brezillon wrote on Tue, 6 Nov 2018
11:22:06 +0100:
On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang wrote:
On 2018/11/6 17:28, Boris Brezillon wrote:
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ u32 cmd;
+
+ cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+ meson_nfc_drain_cmd(nfc);
You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
nand cycle to read one byte and covers the 1st byte every time reading.
i think nfc controller is faster than nand cycle, but really it is not
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
ok, i will try dma here.
We should probably expose the bounce buf handling as generic helpers at
the rawnand level:
void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
return NULL;
if (virt_addr_valid(instr->data.in) &&
!object_is_on_stack(instr->data.buf.in))
return instr->data.buf.in;
return kzalloc(instr->data.len, GFP_KERNEL);
}
void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
WARN_ON(!buf))
return;
if (buf == instr->data.buf.in)
return;
memcpy(instr->data.buf.in, buf, instr->data.len);
kfree(buf);
}
const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
return NULL;
if (virt_addr_valid(instr->data.out) &&
!object_is_on_stack(instr->data.buf.out))
return instr->data.buf.out;
return kmemdup(instr->data.buf.out, GFP_KERNEL);
}
void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
WARN_ON(!buf))
return;
if (buf != instr->data.buf.out)
kfree(buf);
}
Not that I am against such function, but maybe they should come with
comments stating that there is no reliable way to find if a buffer is
DMA-able at runtime and these are just sanity checks (ie. required, but
probably not enough).
It's not 100% reliable, but it should cover most cases. Note that the
NAND framework already uses virt_addr_valid() to decide when to use its
internal bounce buffer, so this should be fixed too if we want a fully
reliable solution.
This is my understanding of Wolfram's recent talk
at ELCE [1].
Yes, you're right, but the NAND framework does not provide any guarantee
on the buf passed to ->exec_op() simply because the MTD layer does not
provide such a guarantee. Reworking that to match how the i2c framework
handles it is possible (with a flag set when the buffer is known to be
DMA-safe), but it requires rewriting all MTD users if we want to keep
decent perfs (the amount of data transfered to a flash is an order of
magnitude bigger than what you usually receive/send from/to an I2C
device). Also, I'm not even sure the DMA_SAFE flag covers all weird
cases like the "DMA engine embedded in the controller is not able to
access the whole physical memory range" one.
I forgot that this problem was handled at dma_map time (a bounce
buffer is allocated if needed, and this decision is based on
dev->dma_mask).
So ideally we should have
something that checks if a pointer is DMA-safe at the device level and
then at the arch level.
A temporary solution would be to add a hook at the nand_controller
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Mon, 12 Nov 2018 17:54:16 +0100
Boris Brezillon wrote:
> +Wolfram to give some inputs on the DMA issue.
>
> On Mon, 12 Nov 2018 17:13:51 +0100
> Miquel Raynal wrote:
>
> > Hello,
> >
> > Boris Brezillon wrote on Tue, 6 Nov 2018
> > 11:22:06 +0100:
> >
> > > On Tue, 6 Nov 2018 18:00:37 +0800
> > > Liang Yang wrote:
> > >
> > > > On 2018/11/6 17:28, Boris Brezillon wrote:
> > > > > On Tue, 6 Nov 2018 17:08:00 +0800
> > > > > Liang Yang wrote:
> > > > >
> > > > >> On 2018/11/5 23:53, Boris Brezillon wrote:
> > > > >>> On Fri, 2 Nov 2018 00:42:21 +0800
> > > > >>> Jianxin Pan wrote:
> > > > >>>
> > > > +
> > > > +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> > > > +{
> > > > + struct nand_chip *nand = mtd_to_nand(mtd);
> > > > + struct meson_nfc *nfc = nand_get_controller_data(nand);
> > > > + u32 cmd;
> > > > +
> > > > + cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> > > > + writel(cmd, nfc->reg_base + NFC_REG_CMD);
> > > > +
> > > > + meson_nfc_drain_cmd(nfc);
> > > > >>>
> > > > >>> You probably don't want to drain the FIFO every time you read a
> > > > >>> byte on
> > > > >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > > > >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> > > > >>> possible and only sync when the user explicitly requests it or when
> > > > >>> the INPUT/READ FIFO is full.
> > > > >>>
> > > > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only
> > > > >> one
> > > > >> nand cycle to read one byte and covers the 1st byte every time
> > > > >> reading.
> > > > >> i think nfc controller is faster than nand cycle, but really it is
> > > > >> not
> > > > >> high efficiency when reading so many bytes once.
> > > > >> Or use dma command here like read_page and read_page_raw.
> > > > >
> > > > > Yep, that's also an alternative, though you'll have to make sure the
> > > > > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > > > > buffer when that's not the case.
> > > > >
> > > > ok, i will try dma here.
> > >
> > > We should probably expose the bounce buf handling as generic helpers at
> > > the rawnand level:
> > >
> > > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> > > {
> > > void *buf;
> > >
> > > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> > > return NULL;
> > >
> > > if (virt_addr_valid(instr->data.in) &&
> > > !object_is_on_stack(instr->data.buf.in))
> > > return instr->data.buf.in;
> > >
> > > return kzalloc(instr->data.len, GFP_KERNEL);
> > > }
> > >
> > > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> > > void *buf)
> > > {
> > > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> > > WARN_ON(!buf))
> > > return;
> > >
> > > if (buf == instr->data.buf.in)
> > > return;
> > >
> > > memcpy(instr->data.buf.in, buf, instr->data.len);
> > > kfree(buf);
> > > }
> > >
> > > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> > > {
> > > void *buf;
> > >
> > > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> > > return NULL;
> > >
> > > if (virt_addr_valid(instr->data.out) &&
> > > !object_is_on_stack(instr->data.buf.out))
> > > return instr->data.buf.out;
> > >
> > > return kmemdup(instr->data.buf.out, GFP_KERNEL);
> > > }
> > >
> > > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> > > void *buf)
> > > {
> > > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> > > WARN_ON(!buf))
> > > return;
> > >
> > > if (buf != instr->data.buf.out)
> > > kfree(buf);
> > > }
> >
> > Not that I am against such function, but maybe they should come with
> > comments stating that there is no reliable way to find if a buffer is
> > DMA-able at runtime and these are just sanity checks (ie. required, but
> > probably not enough).
>
> It's not 100% reliable, but it should cover most cases. Note that the
> NAND framework already uses virt_addr_valid() to decide when to use its
> internal bounce buffer, so this should be fixed too if we want a fully
> reliable solution.
>
> > This is my understanding of Wolfram's recent talk
> > at ELCE [1].
>
> Yes, you're right, but the NAND framework does not provide any guarantee
> on the buf passed to ->exec_op() simply because the MTD layer does not
> provide such a guarantee. Reworking that to match how the i2c framework
> handles it is possible (with a flag set when the buffer is known to be
> DMA-safe), but it requires rewriting all MTD users if we want to keep
> decent perfs (the amount of data transfered to a flash is an order of
> magnitude bigger than what you usually receive/send
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Mon, 12 Nov 2018 17:54:16 +0100
Boris Brezillon wrote:
> +Wolfram to give some inputs on the DMA issue.
>
> On Mon, 12 Nov 2018 17:13:51 +0100
> Miquel Raynal wrote:
>
> > Hello,
> >
> > Boris Brezillon wrote on Tue, 6 Nov 2018
> > 11:22:06 +0100:
> >
> > > On Tue, 6 Nov 2018 18:00:37 +0800
> > > Liang Yang wrote:
> > >
> > > > On 2018/11/6 17:28, Boris Brezillon wrote:
> > > > > On Tue, 6 Nov 2018 17:08:00 +0800
> > > > > Liang Yang wrote:
> > > > >
> > > > >> On 2018/11/5 23:53, Boris Brezillon wrote:
> > > > >>> On Fri, 2 Nov 2018 00:42:21 +0800
> > > > >>> Jianxin Pan wrote:
> > > > >>>
> > > > +
> > > > +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> > > > +{
> > > > + struct nand_chip *nand = mtd_to_nand(mtd);
> > > > + struct meson_nfc *nfc = nand_get_controller_data(nand);
> > > > + u32 cmd;
> > > > +
> > > > + cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> > > > + writel(cmd, nfc->reg_base + NFC_REG_CMD);
> > > > +
> > > > + meson_nfc_drain_cmd(nfc);
> > > > >>>
> > > > >>> You probably don't want to drain the FIFO every time you read a
> > > > >>> byte on
> > > > >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > > > >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> > > > >>> possible and only sync when the user explicitly requests it or when
> > > > >>> the INPUT/READ FIFO is full.
> > > > >>>
> > > > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only
> > > > >> one
> > > > >> nand cycle to read one byte and covers the 1st byte every time
> > > > >> reading.
> > > > >> i think nfc controller is faster than nand cycle, but really it is
> > > > >> not
> > > > >> high efficiency when reading so many bytes once.
> > > > >> Or use dma command here like read_page and read_page_raw.
> > > > >
> > > > > Yep, that's also an alternative, though you'll have to make sure the
> > > > > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > > > > buffer when that's not the case.
> > > > >
> > > > ok, i will try dma here.
> > >
> > > We should probably expose the bounce buf handling as generic helpers at
> > > the rawnand level:
> > >
> > > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> > > {
> > > void *buf;
> > >
> > > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> > > return NULL;
> > >
> > > if (virt_addr_valid(instr->data.in) &&
> > > !object_is_on_stack(instr->data.buf.in))
> > > return instr->data.buf.in;
> > >
> > > return kzalloc(instr->data.len, GFP_KERNEL);
> > > }
> > >
> > > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> > > void *buf)
> > > {
> > > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> > > WARN_ON(!buf))
> > > return;
> > >
> > > if (buf == instr->data.buf.in)
> > > return;
> > >
> > > memcpy(instr->data.buf.in, buf, instr->data.len);
> > > kfree(buf);
> > > }
> > >
> > > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> > > {
> > > void *buf;
> > >
> > > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> > > return NULL;
> > >
> > > if (virt_addr_valid(instr->data.out) &&
> > > !object_is_on_stack(instr->data.buf.out))
> > > return instr->data.buf.out;
> > >
> > > return kmemdup(instr->data.buf.out, GFP_KERNEL);
> > > }
> > >
> > > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> > > void *buf)
> > > {
> > > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> > > WARN_ON(!buf))
> > > return;
> > >
> > > if (buf != instr->data.buf.out)
> > > kfree(buf);
> > > }
> >
> > Not that I am against such function, but maybe they should come with
> > comments stating that there is no reliable way to find if a buffer is
> > DMA-able at runtime and these are just sanity checks (ie. required, but
> > probably not enough).
>
> It's not 100% reliable, but it should cover most cases. Note that the
> NAND framework already uses virt_addr_valid() to decide when to use its
> internal bounce buffer, so this should be fixed too if we want a fully
> reliable solution.
>
> > This is my understanding of Wolfram's recent talk
> > at ELCE [1].
>
> Yes, you're right, but the NAND framework does not provide any guarantee
> on the buf passed to ->exec_op() simply because the MTD layer does not
> provide such a guarantee. Reworking that to match how the i2c framework
> handles it is possible (with a flag set when the buffer is known to be
> DMA-safe), but it requires rewriting all MTD users if we want to keep
> decent perfs (the amount of data transfered to a flash is an order of
> magnitude bigger than what you usually receive/send
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
+Wolfram to give some inputs on the DMA issue.
On Mon, 12 Nov 2018 17:13:51 +0100
Miquel Raynal wrote:
> Hello,
>
> Boris Brezillon wrote on Tue, 6 Nov 2018
> 11:22:06 +0100:
>
> > On Tue, 6 Nov 2018 18:00:37 +0800
> > Liang Yang wrote:
> >
> > > On 2018/11/6 17:28, Boris Brezillon wrote:
> > > > On Tue, 6 Nov 2018 17:08:00 +0800
> > > > Liang Yang wrote:
> > > >
> > > >> On 2018/11/5 23:53, Boris Brezillon wrote:
> > > >>> On Fri, 2 Nov 2018 00:42:21 +0800
> > > >>> Jianxin Pan wrote:
> > > >>>
> > > +
> > > +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> > > +{
> > > +struct nand_chip *nand = mtd_to_nand(mtd);
> > > +struct meson_nfc *nfc = nand_get_controller_data(nand);
> > > +u32 cmd;
> > > +
> > > +cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> > > +writel(cmd, nfc->reg_base + NFC_REG_CMD);
> > > +
> > > +meson_nfc_drain_cmd(nfc);
> > > >>>
> > > >>> You probably don't want to drain the FIFO every time you read a byte
> > > >>> on
> > > >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > > >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> > > >>> possible and only sync when the user explicitly requests it or when
> > > >>> the INPUT/READ FIFO is full.
> > > >>>
> > > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> > > >> nand cycle to read one byte and covers the 1st byte every time reading.
> > > >> i think nfc controller is faster than nand cycle, but really it is not
> > > >> high efficiency when reading so many bytes once.
> > > >> Or use dma command here like read_page and read_page_raw.
> > > >
> > > > Yep, that's also an alternative, though you'll have to make sure the
> > > > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > > > buffer when that's not the case.
> > > >
> > > ok, i will try dma here.
> >
> > We should probably expose the bounce buf handling as generic helpers at
> > the rawnand level:
> >
> > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> > {
> > void *buf;
> >
> > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> > return NULL;
> >
> > if (virt_addr_valid(instr->data.in) &&
> > !object_is_on_stack(instr->data.buf.in))
> > return instr->data.buf.in;
> >
> > return kzalloc(instr->data.len, GFP_KERNEL);
> > }
> >
> > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> > void *buf)
> > {
> > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> > WARN_ON(!buf))
> > return;
> >
> > if (buf == instr->data.buf.in)
> > return;
> >
> > memcpy(instr->data.buf.in, buf, instr->data.len);
> > kfree(buf);
> > }
> >
> > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> > {
> > void *buf;
> >
> > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> > return NULL;
> >
> > if (virt_addr_valid(instr->data.out) &&
> > !object_is_on_stack(instr->data.buf.out))
> > return instr->data.buf.out;
> >
> > return kmemdup(instr->data.buf.out, GFP_KERNEL);
> > }
> >
> > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> > void *buf)
> > {
> > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> > WARN_ON(!buf))
> > return;
> >
> > if (buf != instr->data.buf.out)
> > kfree(buf);
> > }
>
> Not that I am against such function, but maybe they should come with
> comments stating that there is no reliable way to find if a buffer is
> DMA-able at runtime and these are just sanity checks (ie. required, but
> probably not enough).
It's not 100% reliable, but it should cover most cases. Note that the
NAND framework already uses virt_addr_valid() to decide when to use its
internal bounce buffer, so this should be fixed too if we want a fully
reliable solution.
> This is my understanding of Wolfram's recent talk
> at ELCE [1].
Yes, you're right, but the NAND framework does not provide any guarantee
on the buf passed to ->exec_op() simply because the MTD layer does not
provide such a guarantee. Reworking that to match how the i2c framework
handles it is possible (with a flag set when the buffer is known to be
DMA-safe), but it requires rewriting all MTD users if we want to keep
decent perfs (the amount of data transfered to a flash is an order of
magnitude bigger than what you usually receive/send from/to an I2C
device). Also, I'm not even sure the DMA_SAFE flag covers all weird
cases like the "DMA engine embedded in the controller is not able to
access the whole physical memory range" one. So ideally we should have
something that checks if a pointer is DMA-safe at the device level and
then at the arch
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
+Wolfram to give some inputs on the DMA issue.
On Mon, 12 Nov 2018 17:13:51 +0100
Miquel Raynal wrote:
> Hello,
>
> Boris Brezillon wrote on Tue, 6 Nov 2018
> 11:22:06 +0100:
>
> > On Tue, 6 Nov 2018 18:00:37 +0800
> > Liang Yang wrote:
> >
> > > On 2018/11/6 17:28, Boris Brezillon wrote:
> > > > On Tue, 6 Nov 2018 17:08:00 +0800
> > > > Liang Yang wrote:
> > > >
> > > >> On 2018/11/5 23:53, Boris Brezillon wrote:
> > > >>> On Fri, 2 Nov 2018 00:42:21 +0800
> > > >>> Jianxin Pan wrote:
> > > >>>
> > > +
> > > +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> > > +{
> > > +struct nand_chip *nand = mtd_to_nand(mtd);
> > > +struct meson_nfc *nfc = nand_get_controller_data(nand);
> > > +u32 cmd;
> > > +
> > > +cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> > > +writel(cmd, nfc->reg_base + NFC_REG_CMD);
> > > +
> > > +meson_nfc_drain_cmd(nfc);
> > > >>>
> > > >>> You probably don't want to drain the FIFO every time you read a byte
> > > >>> on
> > > >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > > >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> > > >>> possible and only sync when the user explicitly requests it or when
> > > >>> the INPUT/READ FIFO is full.
> > > >>>
> > > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> > > >> nand cycle to read one byte and covers the 1st byte every time reading.
> > > >> i think nfc controller is faster than nand cycle, but really it is not
> > > >> high efficiency when reading so many bytes once.
> > > >> Or use dma command here like read_page and read_page_raw.
> > > >
> > > > Yep, that's also an alternative, though you'll have to make sure the
> > > > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > > > buffer when that's not the case.
> > > >
> > > ok, i will try dma here.
> >
> > We should probably expose the bounce buf handling as generic helpers at
> > the rawnand level:
> >
> > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> > {
> > void *buf;
> >
> > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> > return NULL;
> >
> > if (virt_addr_valid(instr->data.in) &&
> > !object_is_on_stack(instr->data.buf.in))
> > return instr->data.buf.in;
> >
> > return kzalloc(instr->data.len, GFP_KERNEL);
> > }
> >
> > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> > void *buf)
> > {
> > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> > WARN_ON(!buf))
> > return;
> >
> > if (buf == instr->data.buf.in)
> > return;
> >
> > memcpy(instr->data.buf.in, buf, instr->data.len);
> > kfree(buf);
> > }
> >
> > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> > {
> > void *buf;
> >
> > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> > return NULL;
> >
> > if (virt_addr_valid(instr->data.out) &&
> > !object_is_on_stack(instr->data.buf.out))
> > return instr->data.buf.out;
> >
> > return kmemdup(instr->data.buf.out, GFP_KERNEL);
> > }
> >
> > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> > void *buf)
> > {
> > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> > WARN_ON(!buf))
> > return;
> >
> > if (buf != instr->data.buf.out)
> > kfree(buf);
> > }
>
> Not that I am against such function, but maybe they should come with
> comments stating that there is no reliable way to find if a buffer is
> DMA-able at runtime and these are just sanity checks (ie. required, but
> probably not enough).
It's not 100% reliable, but it should cover most cases. Note that the
NAND framework already uses virt_addr_valid() to decide when to use its
internal bounce buffer, so this should be fixed too if we want a fully
reliable solution.
> This is my understanding of Wolfram's recent talk
> at ELCE [1].
Yes, you're right, but the NAND framework does not provide any guarantee
on the buf passed to ->exec_op() simply because the MTD layer does not
provide such a guarantee. Reworking that to match how the i2c framework
handles it is possible (with a flag set when the buffer is known to be
DMA-safe), but it requires rewriting all MTD users if we want to keep
decent perfs (the amount of data transfered to a flash is an order of
magnitude bigger than what you usually receive/send from/to an I2C
device). Also, I'm not even sure the DMA_SAFE flag covers all weird
cases like the "DMA engine embedded in the controller is not able to
access the whole physical memory range" one. So ideally we should have
something that checks if a pointer is DMA-safe at the device level and
then at the arch
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hello,
Boris Brezillon wrote on Tue, 6 Nov 2018
11:22:06 +0100:
> On Tue, 6 Nov 2018 18:00:37 +0800
> Liang Yang wrote:
>
> > On 2018/11/6 17:28, Boris Brezillon wrote:
> > > On Tue, 6 Nov 2018 17:08:00 +0800
> > > Liang Yang wrote:
> > >
> > >> On 2018/11/5 23:53, Boris Brezillon wrote:
> > >>> On Fri, 2 Nov 2018 00:42:21 +0800
> > >>> Jianxin Pan wrote:
> > >>>
> > +
> > +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> > +{
> > + struct nand_chip *nand = mtd_to_nand(mtd);
> > + struct meson_nfc *nfc = nand_get_controller_data(nand);
> > + u32 cmd;
> > +
> > + cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> > + writel(cmd, nfc->reg_base + NFC_REG_CMD);
> > +
> > + meson_nfc_drain_cmd(nfc);
> > >>>
> > >>> You probably don't want to drain the FIFO every time you read a byte on
> > >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> > >>> possible and only sync when the user explicitly requests it or when
> > >>> the INPUT/READ FIFO is full.
> > >>>
> > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> > >> nand cycle to read one byte and covers the 1st byte every time reading.
> > >> i think nfc controller is faster than nand cycle, but really it is not
> > >> high efficiency when reading so many bytes once.
> > >> Or use dma command here like read_page and read_page_raw.
> > >
> > > Yep, that's also an alternative, though you'll have to make sure the
> > > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > > buffer when that's not the case.
> > >
> > ok, i will try dma here.
>
> We should probably expose the bounce buf handling as generic helpers at
> the rawnand level:
>
> void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> {
> void *buf;
>
> if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> return NULL;
>
> if (virt_addr_valid(instr->data.in) &&
> !object_is_on_stack(instr->data.buf.in))
> return instr->data.buf.in;
>
> return kzalloc(instr->data.len, GFP_KERNEL);
> }
>
> void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> void *buf)
> {
> if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> WARN_ON(!buf))
> return;
>
> if (buf == instr->data.buf.in)
> return;
>
> memcpy(instr->data.buf.in, buf, instr->data.len);
> kfree(buf);
> }
>
> const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> {
> void *buf;
>
> if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> return NULL;
>
> if (virt_addr_valid(instr->data.out) &&
> !object_is_on_stack(instr->data.buf.out))
> return instr->data.buf.out;
>
> return kmemdup(instr->data.buf.out, GFP_KERNEL);
> }
>
> void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> void *buf)
> {
> if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> WARN_ON(!buf))
> return;
>
> if (buf != instr->data.buf.out)
> kfree(buf);
> }
Not that I am against such function, but maybe they should come with
comments stating that there is no reliable way to find if a buffer is
DMA-able at runtime and these are just sanity checks (ie. required, but
probably not enough). This is my understanding of Wolfram's recent talk
at ELCE [1]. I suppose using the CONFIG_DMA_API_DEBUG option could help
more reliably to find such issues.
[1] https://www.youtube.com/watch?v=JDwaMClvV-s
Thanks,
Miquèl
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hello,
Boris Brezillon wrote on Tue, 6 Nov 2018
11:22:06 +0100:
> On Tue, 6 Nov 2018 18:00:37 +0800
> Liang Yang wrote:
>
> > On 2018/11/6 17:28, Boris Brezillon wrote:
> > > On Tue, 6 Nov 2018 17:08:00 +0800
> > > Liang Yang wrote:
> > >
> > >> On 2018/11/5 23:53, Boris Brezillon wrote:
> > >>> On Fri, 2 Nov 2018 00:42:21 +0800
> > >>> Jianxin Pan wrote:
> > >>>
> > +
> > +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> > +{
> > + struct nand_chip *nand = mtd_to_nand(mtd);
> > + struct meson_nfc *nfc = nand_get_controller_data(nand);
> > + u32 cmd;
> > +
> > + cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> > + writel(cmd, nfc->reg_base + NFC_REG_CMD);
> > +
> > + meson_nfc_drain_cmd(nfc);
> > >>>
> > >>> You probably don't want to drain the FIFO every time you read a byte on
> > >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> > >>> possible and only sync when the user explicitly requests it or when
> > >>> the INPUT/READ FIFO is full.
> > >>>
> > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> > >> nand cycle to read one byte and covers the 1st byte every time reading.
> > >> i think nfc controller is faster than nand cycle, but really it is not
> > >> high efficiency when reading so many bytes once.
> > >> Or use dma command here like read_page and read_page_raw.
> > >
> > > Yep, that's also an alternative, though you'll have to make sure the
> > > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > > buffer when that's not the case.
> > >
> > ok, i will try dma here.
>
> We should probably expose the bounce buf handling as generic helpers at
> the rawnand level:
>
> void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> {
> void *buf;
>
> if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> return NULL;
>
> if (virt_addr_valid(instr->data.in) &&
> !object_is_on_stack(instr->data.buf.in))
> return instr->data.buf.in;
>
> return kzalloc(instr->data.len, GFP_KERNEL);
> }
>
> void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> void *buf)
> {
> if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> WARN_ON(!buf))
> return;
>
> if (buf == instr->data.buf.in)
> return;
>
> memcpy(instr->data.buf.in, buf, instr->data.len);
> kfree(buf);
> }
>
> const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> {
> void *buf;
>
> if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> return NULL;
>
> if (virt_addr_valid(instr->data.out) &&
> !object_is_on_stack(instr->data.buf.out))
> return instr->data.buf.out;
>
> return kmemdup(instr->data.buf.out, GFP_KERNEL);
> }
>
> void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> void *buf)
> {
> if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> WARN_ON(!buf))
> return;
>
> if (buf != instr->data.buf.out)
> kfree(buf);
> }
Not that I am against such function, but maybe they should come with
comments stating that there is no reliable way to find if a buffer is
DMA-able at runtime and these are just sanity checks (ie. required, but
probably not enough). This is my understanding of Wolfram's recent talk
at ELCE [1]. I suppose using the CONFIG_DMA_API_DEBUG option could help
more reliably to find such issues.
[1] https://www.youtube.com/watch?v=JDwaMClvV-s
Thanks,
Miquèl
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/11/7 0:16, Boris Brezillon wrote:
On Tue, 6 Nov 2018 19:08:27 +0800
Liang Yang wrote:
On 2018/11/6 18:22, Boris Brezillon wrote:
On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang wrote:
On 2018/11/6 17:28, Boris Brezillon wrote:
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ u32 cmd;
+
+ cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+ meson_nfc_drain_cmd(nfc);
You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
nand cycle to read one byte and covers the 1st byte every time reading.
i think nfc controller is faster than nand cycle, but really it is not
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
ok, i will try dma here.
We should probably expose the bounce buf handling as generic helpers at
the rawnand level:
void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
return NULL;
if (virt_addr_valid(instr->data.in) &&
!object_is_on_stack(instr->data.buf.in))
return instr->data.buf.in;
return kzalloc(instr->data.len, GFP_KERNEL);
}
void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
WARN_ON(!buf))
return;
if (buf == instr->data.buf.in)
return;
memcpy(instr->data.buf.in, buf, instr->data.len);
kfree(buf);
}
const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
return NULL;
if (virt_addr_valid(instr->data.out) &&
!object_is_on_stack(instr->data.buf.out))
return instr->data.buf.out;
return kmemdup(instr->data.buf.out, GFP_KERNEL);
}
void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
WARN_ON(!buf))
return;
if (buf != instr->data.buf.out)
kfree(buf);
}
that is more convenient.
i will use meson_chip->databuf as the bounce mid-buffer now.
It won't work: the bounce buffer is allocated after the detection, and
the detection code is calling ->exec_op().
Just add a new patch to you series adding these helpers to nand_base.c.
when using these helpers, i finally find that i still need a *info
buffer* which is used to get status and ecc counter. even i don't need
to check *info buffer*, it is still needed. i just malloc *info_buffer*
in driver now. and then i think some dedicated dma may need a minimum
size(such as 4 bytes). it is luckly that meson nfc is not limited about
the minimum size and i tested it.
so what is your suggestion about it?
.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/11/7 0:16, Boris Brezillon wrote:
On Tue, 6 Nov 2018 19:08:27 +0800
Liang Yang wrote:
On 2018/11/6 18:22, Boris Brezillon wrote:
On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang wrote:
On 2018/11/6 17:28, Boris Brezillon wrote:
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ u32 cmd;
+
+ cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+ meson_nfc_drain_cmd(nfc);
You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
nand cycle to read one byte and covers the 1st byte every time reading.
i think nfc controller is faster than nand cycle, but really it is not
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
ok, i will try dma here.
We should probably expose the bounce buf handling as generic helpers at
the rawnand level:
void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
return NULL;
if (virt_addr_valid(instr->data.in) &&
!object_is_on_stack(instr->data.buf.in))
return instr->data.buf.in;
return kzalloc(instr->data.len, GFP_KERNEL);
}
void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
WARN_ON(!buf))
return;
if (buf == instr->data.buf.in)
return;
memcpy(instr->data.buf.in, buf, instr->data.len);
kfree(buf);
}
const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
return NULL;
if (virt_addr_valid(instr->data.out) &&
!object_is_on_stack(instr->data.buf.out))
return instr->data.buf.out;
return kmemdup(instr->data.buf.out, GFP_KERNEL);
}
void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
WARN_ON(!buf))
return;
if (buf != instr->data.buf.out)
kfree(buf);
}
that is more convenient.
i will use meson_chip->databuf as the bounce mid-buffer now.
It won't work: the bounce buffer is allocated after the detection, and
the detection code is calling ->exec_op().
Just add a new patch to you series adding these helpers to nand_base.c.
when using these helpers, i finally find that i still need a *info
buffer* which is used to get status and ecc counter. even i don't need
to check *info buffer*, it is still needed. i just malloc *info_buffer*
in driver now. and then i think some dedicated dma may need a minimum
size(such as 4 bytes). it is luckly that meson nfc is not limited about
the minimum size and i tested it.
so what is your suggestion about it?
.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/11/7 0:16, Boris Brezillon wrote:
On Tue, 6 Nov 2018 19:08:27 +0800
Liang Yang wrote:
On 2018/11/6 18:22, Boris Brezillon wrote:
On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang wrote:
On 2018/11/6 17:28, Boris Brezillon wrote:
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ u32 cmd;
+
+ cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+ meson_nfc_drain_cmd(nfc);
You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
nand cycle to read one byte and covers the 1st byte every time reading.
i think nfc controller is faster than nand cycle, but really it is not
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
ok, i will try dma here.
We should probably expose the bounce buf handling as generic helpers at
the rawnand level:
void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
return NULL;
if (virt_addr_valid(instr->data.in) &&
!object_is_on_stack(instr->data.buf.in))
return instr->data.buf.in;
return kzalloc(instr->data.len, GFP_KERNEL);
}
void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
WARN_ON(!buf))
return;
if (buf == instr->data.buf.in)
return;
memcpy(instr->data.buf.in, buf, instr->data.len);
kfree(buf);
}
const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
return NULL;
if (virt_addr_valid(instr->data.out) &&
!object_is_on_stack(instr->data.buf.out))
return instr->data.buf.out;
return kmemdup(instr->data.buf.out, GFP_KERNEL);
}
void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
WARN_ON(!buf))
return;
if (buf != instr->data.buf.out)
kfree(buf);
}
that is more convenient.
i will use meson_chip->databuf as the bounce mid-buffer now.
It won't work: the bounce buffer is allocated after the detection, and
the detection code is calling ->exec_op().
Just add a new patch to you series adding these helpers to nand_base.c.
ok
.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/11/7 0:16, Boris Brezillon wrote:
On Tue, 6 Nov 2018 19:08:27 +0800
Liang Yang wrote:
On 2018/11/6 18:22, Boris Brezillon wrote:
On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang wrote:
On 2018/11/6 17:28, Boris Brezillon wrote:
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ u32 cmd;
+
+ cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+ meson_nfc_drain_cmd(nfc);
You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
nand cycle to read one byte and covers the 1st byte every time reading.
i think nfc controller is faster than nand cycle, but really it is not
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
ok, i will try dma here.
We should probably expose the bounce buf handling as generic helpers at
the rawnand level:
void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
return NULL;
if (virt_addr_valid(instr->data.in) &&
!object_is_on_stack(instr->data.buf.in))
return instr->data.buf.in;
return kzalloc(instr->data.len, GFP_KERNEL);
}
void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
WARN_ON(!buf))
return;
if (buf == instr->data.buf.in)
return;
memcpy(instr->data.buf.in, buf, instr->data.len);
kfree(buf);
}
const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
return NULL;
if (virt_addr_valid(instr->data.out) &&
!object_is_on_stack(instr->data.buf.out))
return instr->data.buf.out;
return kmemdup(instr->data.buf.out, GFP_KERNEL);
}
void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
WARN_ON(!buf))
return;
if (buf != instr->data.buf.out)
kfree(buf);
}
that is more convenient.
i will use meson_chip->databuf as the bounce mid-buffer now.
It won't work: the bounce buffer is allocated after the detection, and
the detection code is calling ->exec_op().
Just add a new patch to you series adding these helpers to nand_base.c.
ok
.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Tue, 6 Nov 2018 19:08:27 +0800
Liang Yang wrote:
> On 2018/11/6 18:22, Boris Brezillon wrote:
> > On Tue, 6 Nov 2018 18:00:37 +0800
> > Liang Yang wrote:
> >
> >> On 2018/11/6 17:28, Boris Brezillon wrote:
> >>> On Tue, 6 Nov 2018 17:08:00 +0800
> >>> Liang Yang wrote:
> >>>
> On 2018/11/5 23:53, Boris Brezillon wrote:
> > On Fri, 2 Nov 2018 00:42:21 +0800
> > Jianxin Pan wrote:
> >
> >> +
> >> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> >> +{
> >> + struct nand_chip *nand = mtd_to_nand(mtd);
> >> + struct meson_nfc *nfc = nand_get_controller_data(nand);
> >> + u32 cmd;
> >> +
> >> + cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> >> + writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +
> >> + meson_nfc_drain_cmd(nfc);
> >
> > You probably don't want to drain the FIFO every time you read a byte on
> > the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > FIFO, right? If that's the case, you should queue as much DRD cmd as
> > possible and only sync when the user explicitly requests it or when
> > the INPUT/READ FIFO is full.
> >
> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> nand cycle to read one byte and covers the 1st byte every time reading.
> i think nfc controller is faster than nand cycle, but really it is not
> high efficiency when reading so many bytes once.
> Or use dma command here like read_page and read_page_raw.
> >>>
> >>> Yep, that's also an alternative, though you'll have to make sure the
> >>> buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> >>> buffer when that's not the case.
> >>>
> >> ok, i will try dma here.
> >
> > We should probably expose the bounce buf handling as generic helpers at
> > the rawnand level:
> >
> > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> > {
> > void *buf;
> >
> > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> > return NULL;
> >
> > if (virt_addr_valid(instr->data.in) &&
> > !object_is_on_stack(instr->data.buf.in))
> > return instr->data.buf.in;
> >
> > return kzalloc(instr->data.len, GFP_KERNEL);
> > }
> >
> > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> > void *buf)
> > {
> > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> > WARN_ON(!buf))
> > return;
> >
> > if (buf == instr->data.buf.in)
> > return;
> >
> > memcpy(instr->data.buf.in, buf, instr->data.len);
> > kfree(buf);
> > }
> >
> > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> > {
> > void *buf;
> >
> > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> > return NULL;
> >
> > if (virt_addr_valid(instr->data.out) &&
> > !object_is_on_stack(instr->data.buf.out))
> > return instr->data.buf.out;
> >
> > return kmemdup(instr->data.buf.out, GFP_KERNEL);
> > }
> >
> > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> > void *buf)
> > {
> > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> > WARN_ON(!buf))
> > return;
> >
> > if (buf != instr->data.buf.out)
> > kfree(buf);
> > }
> >
>
> that is more convenient.
> i will use meson_chip->databuf as the bounce mid-buffer now.
It won't work: the bounce buffer is allocated after the detection, and
the detection code is calling ->exec_op().
Just add a new patch to you series adding these helpers to nand_base.c.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Tue, 6 Nov 2018 19:08:27 +0800
Liang Yang wrote:
> On 2018/11/6 18:22, Boris Brezillon wrote:
> > On Tue, 6 Nov 2018 18:00:37 +0800
> > Liang Yang wrote:
> >
> >> On 2018/11/6 17:28, Boris Brezillon wrote:
> >>> On Tue, 6 Nov 2018 17:08:00 +0800
> >>> Liang Yang wrote:
> >>>
> On 2018/11/5 23:53, Boris Brezillon wrote:
> > On Fri, 2 Nov 2018 00:42:21 +0800
> > Jianxin Pan wrote:
> >
> >> +
> >> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> >> +{
> >> + struct nand_chip *nand = mtd_to_nand(mtd);
> >> + struct meson_nfc *nfc = nand_get_controller_data(nand);
> >> + u32 cmd;
> >> +
> >> + cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> >> + writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +
> >> + meson_nfc_drain_cmd(nfc);
> >
> > You probably don't want to drain the FIFO every time you read a byte on
> > the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > FIFO, right? If that's the case, you should queue as much DRD cmd as
> > possible and only sync when the user explicitly requests it or when
> > the INPUT/READ FIFO is full.
> >
> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> nand cycle to read one byte and covers the 1st byte every time reading.
> i think nfc controller is faster than nand cycle, but really it is not
> high efficiency when reading so many bytes once.
> Or use dma command here like read_page and read_page_raw.
> >>>
> >>> Yep, that's also an alternative, though you'll have to make sure the
> >>> buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> >>> buffer when that's not the case.
> >>>
> >> ok, i will try dma here.
> >
> > We should probably expose the bounce buf handling as generic helpers at
> > the rawnand level:
> >
> > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> > {
> > void *buf;
> >
> > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> > return NULL;
> >
> > if (virt_addr_valid(instr->data.in) &&
> > !object_is_on_stack(instr->data.buf.in))
> > return instr->data.buf.in;
> >
> > return kzalloc(instr->data.len, GFP_KERNEL);
> > }
> >
> > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> > void *buf)
> > {
> > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> > WARN_ON(!buf))
> > return;
> >
> > if (buf == instr->data.buf.in)
> > return;
> >
> > memcpy(instr->data.buf.in, buf, instr->data.len);
> > kfree(buf);
> > }
> >
> > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> > {
> > void *buf;
> >
> > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> > return NULL;
> >
> > if (virt_addr_valid(instr->data.out) &&
> > !object_is_on_stack(instr->data.buf.out))
> > return instr->data.buf.out;
> >
> > return kmemdup(instr->data.buf.out, GFP_KERNEL);
> > }
> >
> > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> > void *buf)
> > {
> > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> > WARN_ON(!buf))
> > return;
> >
> > if (buf != instr->data.buf.out)
> > kfree(buf);
> > }
> >
>
> that is more convenient.
> i will use meson_chip->databuf as the bounce mid-buffer now.
It won't work: the bounce buffer is allocated after the detection, and
the detection code is calling ->exec_op().
Just add a new patch to you series adding these helpers to nand_base.c.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/11/6 18:22, Boris Brezillon wrote:
On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang wrote:
On 2018/11/6 17:28, Boris Brezillon wrote:
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ u32 cmd;
+
+ cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+ meson_nfc_drain_cmd(nfc);
You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
nand cycle to read one byte and covers the 1st byte every time reading.
i think nfc controller is faster than nand cycle, but really it is not
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
ok, i will try dma here.
We should probably expose the bounce buf handling as generic helpers at
the rawnand level:
void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
return NULL;
if (virt_addr_valid(instr->data.in) &&
!object_is_on_stack(instr->data.buf.in))
return instr->data.buf.in;
return kzalloc(instr->data.len, GFP_KERNEL);
}
void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
WARN_ON(!buf))
return;
if (buf == instr->data.buf.in)
return;
memcpy(instr->data.buf.in, buf, instr->data.len);
kfree(buf);
}
const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
return NULL;
if (virt_addr_valid(instr->data.out) &&
!object_is_on_stack(instr->data.buf.out))
return instr->data.buf.out;
return kmemdup(instr->data.buf.out, GFP_KERNEL);
}
void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
WARN_ON(!buf))
return;
if (buf != instr->data.buf.out)
kfree(buf);
}
that is more convenient.
i will use meson_chip->databuf as the bounce mid-buffer now.
Isn't the controller engine able to wait on the data transfer to be
complete before sending the next instruction in the CMD FIFO pipe?
I'm pretty sure it's able to do that, which would make
meson_nfc_wait_dma_finish() useless, and all you'd have to do is wait
for the CMD FIFO to be empty (assuming it guarantees the last command
has been executed).
Maybe the nfc design is difference. dedicated nfc dma engine is
concatenated with the command fifo, there is no other status to check
whether dma is done.
No, I mean that internally a "DMA-transfer" instruction probably
forces the NAND controller to wait for the DMA transfer to be finished
before dequeuing/executing the next instruction. So, it should be safe
to queue the PROG and WAIT_RB instructions and just rely on the "FIFO
empty" event to consider the operation as finished. Then, all you'll
have to do is check the status reg to determine whether the
write operation succeeded or not.
em, i got the point. indeed, until dma is checked done, nfc will execute
next command in the command queue. so i will consider it.
.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/11/6 18:22, Boris Brezillon wrote:
On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang wrote:
On 2018/11/6 17:28, Boris Brezillon wrote:
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ u32 cmd;
+
+ cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+ meson_nfc_drain_cmd(nfc);
You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
nand cycle to read one byte and covers the 1st byte every time reading.
i think nfc controller is faster than nand cycle, but really it is not
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
ok, i will try dma here.
We should probably expose the bounce buf handling as generic helpers at
the rawnand level:
void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
return NULL;
if (virt_addr_valid(instr->data.in) &&
!object_is_on_stack(instr->data.buf.in))
return instr->data.buf.in;
return kzalloc(instr->data.len, GFP_KERNEL);
}
void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
WARN_ON(!buf))
return;
if (buf == instr->data.buf.in)
return;
memcpy(instr->data.buf.in, buf, instr->data.len);
kfree(buf);
}
const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
return NULL;
if (virt_addr_valid(instr->data.out) &&
!object_is_on_stack(instr->data.buf.out))
return instr->data.buf.out;
return kmemdup(instr->data.buf.out, GFP_KERNEL);
}
void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
WARN_ON(!buf))
return;
if (buf != instr->data.buf.out)
kfree(buf);
}
that is more convenient.
i will use meson_chip->databuf as the bounce mid-buffer now.
Isn't the controller engine able to wait on the data transfer to be
complete before sending the next instruction in the CMD FIFO pipe?
I'm pretty sure it's able to do that, which would make
meson_nfc_wait_dma_finish() useless, and all you'd have to do is wait
for the CMD FIFO to be empty (assuming it guarantees the last command
has been executed).
Maybe the nfc design is difference. dedicated nfc dma engine is
concatenated with the command fifo, there is no other status to check
whether dma is done.
No, I mean that internally a "DMA-transfer" instruction probably
forces the NAND controller to wait for the DMA transfer to be finished
before dequeuing/executing the next instruction. So, it should be safe
to queue the PROG and WAIT_RB instructions and just rely on the "FIFO
empty" event to consider the operation as finished. Then, all you'll
have to do is check the status reg to determine whether the
write operation succeeded or not.
em, i got the point. indeed, until dma is checked done, nfc will execute
next command in the command queue. so i will consider it.
.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang wrote:
> On 2018/11/6 17:28, Boris Brezillon wrote:
> > On Tue, 6 Nov 2018 17:08:00 +0800
> > Liang Yang wrote:
> >
> >> On 2018/11/5 23:53, Boris Brezillon wrote:
> >>> On Fri, 2 Nov 2018 00:42:21 +0800
> >>> Jianxin Pan wrote:
> >>>
> +
> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> +{
> +struct nand_chip *nand = mtd_to_nand(mtd);
> +struct meson_nfc *nfc = nand_get_controller_data(nand);
> +u32 cmd;
> +
> +cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> +writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +
> +meson_nfc_drain_cmd(nfc);
> >>>
> >>> You probably don't want to drain the FIFO every time you read a byte on
> >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> >>> possible and only sync when the user explicitly requests it or when
> >>> the INPUT/READ FIFO is full.
> >>>
> >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> >> nand cycle to read one byte and covers the 1st byte every time reading.
> >> i think nfc controller is faster than nand cycle, but really it is not
> >> high efficiency when reading so many bytes once.
> >> Or use dma command here like read_page and read_page_raw.
> >
> > Yep, that's also an alternative, though you'll have to make sure the
> > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > buffer when that's not the case.
> >
> ok, i will try dma here.
We should probably expose the bounce buf handling as generic helpers at
the rawnand level:
void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
return NULL;
if (virt_addr_valid(instr->data.in) &&
!object_is_on_stack(instr->data.buf.in))
return instr->data.buf.in;
return kzalloc(instr->data.len, GFP_KERNEL);
}
void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
WARN_ON(!buf))
return;
if (buf == instr->data.buf.in)
return;
memcpy(instr->data.buf.in, buf, instr->data.len);
kfree(buf);
}
const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
return NULL;
if (virt_addr_valid(instr->data.out) &&
!object_is_on_stack(instr->data.buf.out))
return instr->data.buf.out;
return kmemdup(instr->data.buf.out, GFP_KERNEL);
}
void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
WARN_ON(!buf))
return;
if (buf != instr->data.buf.out)
kfree(buf);
}
> >
> > Isn't the controller engine able to wait on the data transfer to be
> > complete before sending the next instruction in the CMD FIFO pipe?
> > I'm pretty sure it's able to do that, which would make
> > meson_nfc_wait_dma_finish() useless, and all you'd have to do is wait
> > for the CMD FIFO to be empty (assuming it guarantees the last command
> > has been executed).
> > Maybe the nfc design is difference. dedicated nfc dma engine is
> concatenated with the command fifo, there is no other status to check
> whether dma is done.
No, I mean that internally a "DMA-transfer" instruction probably
forces the NAND controller to wait for the DMA transfer to be finished
before dequeuing/executing the next instruction. So, it should be safe
to queue the PROG and WAIT_RB instructions and just rely on the "FIFO
empty" event to consider the operation as finished. Then, all you'll
have to do is check the status reg to determine whether the
write operation succeeded or not.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang wrote:
> On 2018/11/6 17:28, Boris Brezillon wrote:
> > On Tue, 6 Nov 2018 17:08:00 +0800
> > Liang Yang wrote:
> >
> >> On 2018/11/5 23:53, Boris Brezillon wrote:
> >>> On Fri, 2 Nov 2018 00:42:21 +0800
> >>> Jianxin Pan wrote:
> >>>
> +
> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> +{
> +struct nand_chip *nand = mtd_to_nand(mtd);
> +struct meson_nfc *nfc = nand_get_controller_data(nand);
> +u32 cmd;
> +
> +cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> +writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +
> +meson_nfc_drain_cmd(nfc);
> >>>
> >>> You probably don't want to drain the FIFO every time you read a byte on
> >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> >>> possible and only sync when the user explicitly requests it or when
> >>> the INPUT/READ FIFO is full.
> >>>
> >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> >> nand cycle to read one byte and covers the 1st byte every time reading.
> >> i think nfc controller is faster than nand cycle, but really it is not
> >> high efficiency when reading so many bytes once.
> >> Or use dma command here like read_page and read_page_raw.
> >
> > Yep, that's also an alternative, though you'll have to make sure the
> > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > buffer when that's not the case.
> >
> ok, i will try dma here.
We should probably expose the bounce buf handling as generic helpers at
the rawnand level:
void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
return NULL;
if (virt_addr_valid(instr->data.in) &&
!object_is_on_stack(instr->data.buf.in))
return instr->data.buf.in;
return kzalloc(instr->data.len, GFP_KERNEL);
}
void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
WARN_ON(!buf))
return;
if (buf == instr->data.buf.in)
return;
memcpy(instr->data.buf.in, buf, instr->data.len);
kfree(buf);
}
const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
void *buf;
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
return NULL;
if (virt_addr_valid(instr->data.out) &&
!object_is_on_stack(instr->data.buf.out))
return instr->data.buf.out;
return kmemdup(instr->data.buf.out, GFP_KERNEL);
}
void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
void *buf)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
WARN_ON(!buf))
return;
if (buf != instr->data.buf.out)
kfree(buf);
}
> >
> > Isn't the controller engine able to wait on the data transfer to be
> > complete before sending the next instruction in the CMD FIFO pipe?
> > I'm pretty sure it's able to do that, which would make
> > meson_nfc_wait_dma_finish() useless, and all you'd have to do is wait
> > for the CMD FIFO to be empty (assuming it guarantees the last command
> > has been executed).
> > Maybe the nfc design is difference. dedicated nfc dma engine is
> concatenated with the command fifo, there is no other status to check
> whether dma is done.
No, I mean that internally a "DMA-transfer" instruction probably
forces the NAND controller to wait for the DMA transfer to be finished
before dequeuing/executing the next instruction. So, it should be safe
to queue the PROG and WAIT_RB instructions and just rely on the "FIFO
empty" event to consider the operation as finished. Then, all you'll
have to do is check the status reg to determine whether the
write operation succeeded or not.
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/11/6 17:28, Boris Brezillon wrote:
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ u32 cmd;
+
+ cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+ meson_nfc_drain_cmd(nfc);
You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
nand cycle to read one byte and covers the 1st byte every time reading.
i think nfc controller is faster than nand cycle, but really it is not
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
ok, i will try dma here.
+
+ meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
As for the read_byte() implementation, I don't think you should force a
sync here.
ok, it can send 30 bytes (command fifo size subtract 2 idle command )
once with a sync.
Okay, still better than syncing after each transmitted byte.
Or use dma command.
I guess that's the best option.
ok, i will try dma here.
+}
+
+static void meson_nfc_write_buf(struct mtd_info *mtd,
+ const u8 *buf, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++)
+ meson_nfc_write_byte(mtd, buf[i]);
+}
+
+static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
+ int page, bool in)
+{
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ const struct nand_sdr_timings *sdr =
+ nand_get_sdr_timings(>data_interface);
+ int cs = nfc->param.chip_select;
+ int i, cmd0, cmd_num;
+ int ret = 0;
+
+ cmd0 = in ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
+ cmd_num = sizeof(struct nand_rw_cmd) / sizeof(int);
+ if (!in)
+ cmd_num--;
+
+ nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
+ for (i = 0; i < MAX_CYCLE_COLUMN_ADDRS; i++)
+ nfc->cmdfifo.rw.col[i] = cs | NFC_CMD_ALE | 0;
+
+ for (i = 0; i < MAX_CYCLE_ROW_ADDRS; i++)
+ nfc->cmdfifo.rw.row[i] = cs | NFC_CMD_ALE | ROW_ADDER(page, i);
+
+ nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
Having a fixed size array for the column and row address cycles does
not sound like a good idea to me (depending on the NAND chip you
connect, the number of cycles for the row and column differ), which
makes me realize the nand_rw_cmd struct is not such a good thing...
em , i will fix it by adding the size of the column and row address.
is that ok?
+
+ for (i = 0; i < cmd_num; i++)
+ writel(nfc->cmdfifo.cmd[i], nfc->reg_base + NFC_REG_CMD);
... why not write directly to the CMD reg?
it seems that too many writel(xxx, nfc->reg_base + NFC_REG_CMD) in one
function; so I want to cache all the commands and write it in a loop.
Not sure why it makes a difference since you'll end up writing to
NFC_REG_CMD anyway.
BTW, you can probably use the writel_relaxed() instead of writel() when
writing to the CMD FIFO.
ok.
+
+ if (in)
+ meson_nfc_queue_rb(nfc, sdr->tR_max);
+ else
+ meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
+
+ return ret;
+}
+
+static int meson_nfc_write_page_sub(struct nand_chip *nand, const u8 *buf,
+ int page, int raw)
+{
+ struct mtd_info *mtd = nand_to_mtd(nand);
+ const struct nand_sdr_timings *sdr =
+ nand_get_sdr_timings(>data_interface);
+ struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ dma_addr_t daddr, iaddr;
+ u32 cmd;
+ int ret;
+
+ daddr = dma_map_single(nfc->dev, (void *)meson_chip->data_buf,
+ mtd->writesize + mtd->oobsize,
+ DMA_TO_DEVICE);
+ ret = dma_mapping_error(nfc->dev, daddr);
+ if (ret) {
+ dev_err(nfc->dev, "dma mapping error\n");
+ goto err;
+ }
+
+ iaddr = dma_map_single(nfc->dev, (void *)meson_chip->info_buf,
+ nand->ecc.steps * PER_INFO_BYTE,
+ DMA_TO_DEVICE);
+ ret =
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/11/6 17:28, Boris Brezillon wrote:
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ u32 cmd;
+
+ cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+ meson_nfc_drain_cmd(nfc);
You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
nand cycle to read one byte and covers the 1st byte every time reading.
i think nfc controller is faster than nand cycle, but really it is not
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
ok, i will try dma here.
+
+ meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
As for the read_byte() implementation, I don't think you should force a
sync here.
ok, it can send 30 bytes (command fifo size subtract 2 idle command )
once with a sync.
Okay, still better than syncing after each transmitted byte.
Or use dma command.
I guess that's the best option.
ok, i will try dma here.
+}
+
+static void meson_nfc_write_buf(struct mtd_info *mtd,
+ const u8 *buf, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++)
+ meson_nfc_write_byte(mtd, buf[i]);
+}
+
+static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
+ int page, bool in)
+{
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ const struct nand_sdr_timings *sdr =
+ nand_get_sdr_timings(>data_interface);
+ int cs = nfc->param.chip_select;
+ int i, cmd0, cmd_num;
+ int ret = 0;
+
+ cmd0 = in ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
+ cmd_num = sizeof(struct nand_rw_cmd) / sizeof(int);
+ if (!in)
+ cmd_num--;
+
+ nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
+ for (i = 0; i < MAX_CYCLE_COLUMN_ADDRS; i++)
+ nfc->cmdfifo.rw.col[i] = cs | NFC_CMD_ALE | 0;
+
+ for (i = 0; i < MAX_CYCLE_ROW_ADDRS; i++)
+ nfc->cmdfifo.rw.row[i] = cs | NFC_CMD_ALE | ROW_ADDER(page, i);
+
+ nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
Having a fixed size array for the column and row address cycles does
not sound like a good idea to me (depending on the NAND chip you
connect, the number of cycles for the row and column differ), which
makes me realize the nand_rw_cmd struct is not such a good thing...
em , i will fix it by adding the size of the column and row address.
is that ok?
+
+ for (i = 0; i < cmd_num; i++)
+ writel(nfc->cmdfifo.cmd[i], nfc->reg_base + NFC_REG_CMD);
... why not write directly to the CMD reg?
it seems that too many writel(xxx, nfc->reg_base + NFC_REG_CMD) in one
function; so I want to cache all the commands and write it in a loop.
Not sure why it makes a difference since you'll end up writing to
NFC_REG_CMD anyway.
BTW, you can probably use the writel_relaxed() instead of writel() when
writing to the CMD FIFO.
ok.
+
+ if (in)
+ meson_nfc_queue_rb(nfc, sdr->tR_max);
+ else
+ meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
+
+ return ret;
+}
+
+static int meson_nfc_write_page_sub(struct nand_chip *nand, const u8 *buf,
+ int page, int raw)
+{
+ struct mtd_info *mtd = nand_to_mtd(nand);
+ const struct nand_sdr_timings *sdr =
+ nand_get_sdr_timings(>data_interface);
+ struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ dma_addr_t daddr, iaddr;
+ u32 cmd;
+ int ret;
+
+ daddr = dma_map_single(nfc->dev, (void *)meson_chip->data_buf,
+ mtd->writesize + mtd->oobsize,
+ DMA_TO_DEVICE);
+ ret = dma_mapping_error(nfc->dev, daddr);
+ if (ret) {
+ dev_err(nfc->dev, "dma mapping error\n");
+ goto err;
+ }
+
+ iaddr = dma_map_single(nfc->dev, (void *)meson_chip->info_buf,
+ nand->ecc.steps * PER_INFO_BYTE,
+ DMA_TO_DEVICE);
+ ret =
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
> On 2018/11/5 23:53, Boris Brezillon wrote:
> > On Fri, 2 Nov 2018 00:42:21 +0800
> > Jianxin Pan wrote:
> >
> >> +#define NFC_REG_CMD 0x00
> >> +#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_RBBIT(20)
> >> +#define NFC_CMD_IO6 ((0xb << 10) | (1 << 18))
> >> +#define NFC_CMD_SCRAMBLER_ENABLE BIT(19)
> >> +#define NFC_CMD_RB_INTBIT(14)
> >> +
> >> +#define NFC_CMD_GET_SIZE(x) (((x) >> 22) & GENMASK(4, 0))
> >> +#define NFC_CMD_RB_TIMEOUT0x18
> >
> > Where does this value come from? Is it the typical timeout value,
> > and if it is, what's the value in milli/microseconds?
> >
> it is about 5.2ms when one nand cycle is 20ns and calculate it with the
> max erase time of toshiba slc flash.i think it should be taken from the
> sdr_timings.
Yes, it should. Or just pick the maximum value, since it's just a
timeout anyway and shouldn't expire if everything works as expected.
> >> +
> >> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> >> +{
> >> + struct nand_chip *nand = mtd_to_nand(mtd);
> >> + struct meson_nfc *nfc = nand_get_controller_data(nand);
> >> + u32 cmd;
> >> +
> >> + cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> >> + writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +
> >> + meson_nfc_drain_cmd(nfc);
> >
> > You probably don't want to drain the FIFO every time you read a byte on
> > the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > FIFO, right? If that's the case, you should queue as much DRD cmd as
> > possible and only sync when the user explicitly requests it or when
> > the INPUT/READ FIFO is full.
> >
> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> nand cycle to read one byte and covers the 1st byte every time reading.
> i think nfc controller is faster than nand cycle, but really it is not
> high efficiency when reading so many bytes once.
> Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
> >> +
> >> + meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
> >
> > As for the read_byte() implementation, I don't think you should force a
> > sync here.
> >
> ok, it can send 30 bytes (command fifo size subtract 2 idle command )
> once with a sync.
Okay, still better than syncing after each transmitted byte.
> Or use dma command.
I guess that's the best option.
>
> >> +}
> >> +
> >> +static void meson_nfc_write_buf(struct mtd_info *mtd,
> >> + const u8 *buf, int len)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < len; i++)
> >> + meson_nfc_write_byte(mtd, buf[i]);
> >> +}
> >> +
> >> +static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
> >> + int page, bool in)
> >> +{
> >> + struct meson_nfc *nfc = nand_get_controller_data(nand);
> >> + const struct nand_sdr_timings *sdr =
> >> + nand_get_sdr_timings(>data_interface);
> >> + int cs = nfc->param.chip_select;
> >> + int i, cmd0, cmd_num;
> >> + int ret = 0;
> >> +
> >> + cmd0 = in ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
> >> + cmd_num = sizeof(struct nand_rw_cmd) / sizeof(int);
> >> + if (!in)
> >> + cmd_num--;
> >> +
> >> + nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
> >> + for (i = 0; i < MAX_CYCLE_COLUMN_ADDRS; i++)
> >> + nfc->cmdfifo.rw.col[i] = cs | NFC_CMD_ALE | 0;
> >> +
> >> + for (i = 0; i < MAX_CYCLE_ROW_ADDRS; i++)
> >> + nfc->cmdfifo.rw.row[i] = cs | NFC_CMD_ALE | ROW_ADDER(page, i);
> >> +
> >> + nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
> >
> > Having a fixed size array for the column and row address cycles does
> > not sound like a good idea to me (depending on the NAND chip you
> > connect, the number of cycles for the row and column differ), which
> > makes me realize the nand_rw_cmd struct is not such a good thing...
> >
> em , i will fix it by adding the size of the column and row address.
> is that ok?
>
> >> +
> >> + for (i = 0; i < cmd_num; i++)
> >> +
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang wrote:
> On 2018/11/5 23:53, Boris Brezillon wrote:
> > On Fri, 2 Nov 2018 00:42:21 +0800
> > Jianxin Pan wrote:
> >
> >> +#define NFC_REG_CMD 0x00
> >> +#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_RBBIT(20)
> >> +#define NFC_CMD_IO6 ((0xb << 10) | (1 << 18))
> >> +#define NFC_CMD_SCRAMBLER_ENABLE BIT(19)
> >> +#define NFC_CMD_RB_INTBIT(14)
> >> +
> >> +#define NFC_CMD_GET_SIZE(x) (((x) >> 22) & GENMASK(4, 0))
> >> +#define NFC_CMD_RB_TIMEOUT0x18
> >
> > Where does this value come from? Is it the typical timeout value,
> > and if it is, what's the value in milli/microseconds?
> >
> it is about 5.2ms when one nand cycle is 20ns and calculate it with the
> max erase time of toshiba slc flash.i think it should be taken from the
> sdr_timings.
Yes, it should. Or just pick the maximum value, since it's just a
timeout anyway and shouldn't expire if everything works as expected.
> >> +
> >> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> >> +{
> >> + struct nand_chip *nand = mtd_to_nand(mtd);
> >> + struct meson_nfc *nfc = nand_get_controller_data(nand);
> >> + u32 cmd;
> >> +
> >> + cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> >> + writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +
> >> + meson_nfc_drain_cmd(nfc);
> >
> > You probably don't want to drain the FIFO every time you read a byte on
> > the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > FIFO, right? If that's the case, you should queue as much DRD cmd as
> > possible and only sync when the user explicitly requests it or when
> > the INPUT/READ FIFO is full.
> >
> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> nand cycle to read one byte and covers the 1st byte every time reading.
> i think nfc controller is faster than nand cycle, but really it is not
> high efficiency when reading so many bytes once.
> Or use dma command here like read_page and read_page_raw.
Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.
> >> +
> >> + meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
> >
> > As for the read_byte() implementation, I don't think you should force a
> > sync here.
> >
> ok, it can send 30 bytes (command fifo size subtract 2 idle command )
> once with a sync.
Okay, still better than syncing after each transmitted byte.
> Or use dma command.
I guess that's the best option.
>
> >> +}
> >> +
> >> +static void meson_nfc_write_buf(struct mtd_info *mtd,
> >> + const u8 *buf, int len)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < len; i++)
> >> + meson_nfc_write_byte(mtd, buf[i]);
> >> +}
> >> +
> >> +static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
> >> + int page, bool in)
> >> +{
> >> + struct meson_nfc *nfc = nand_get_controller_data(nand);
> >> + const struct nand_sdr_timings *sdr =
> >> + nand_get_sdr_timings(>data_interface);
> >> + int cs = nfc->param.chip_select;
> >> + int i, cmd0, cmd_num;
> >> + int ret = 0;
> >> +
> >> + cmd0 = in ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
> >> + cmd_num = sizeof(struct nand_rw_cmd) / sizeof(int);
> >> + if (!in)
> >> + cmd_num--;
> >> +
> >> + nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
> >> + for (i = 0; i < MAX_CYCLE_COLUMN_ADDRS; i++)
> >> + nfc->cmdfifo.rw.col[i] = cs | NFC_CMD_ALE | 0;
> >> +
> >> + for (i = 0; i < MAX_CYCLE_ROW_ADDRS; i++)
> >> + nfc->cmdfifo.rw.row[i] = cs | NFC_CMD_ALE | ROW_ADDER(page, i);
> >> +
> >> + nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
> >
> > Having a fixed size array for the column and row address cycles does
> > not sound like a good idea to me (depending on the NAND chip you
> > connect, the number of cycles for the row and column differ), which
> > makes me realize the nand_rw_cmd struct is not such a good thing...
> >
> em , i will fix it by adding the size of the column and row address.
> is that ok?
>
> >> +
> >> + for (i = 0; i < cmd_num; i++)
> >> +
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+#define NFC_REG_CMD0x00
+#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 BIT(20)
+#define NFC_CMD_IO6((0xb << 10) | (1 << 18))
+#define NFC_CMD_SCRAMBLER_ENABLE BIT(19)
+#define NFC_CMD_RB_INT BIT(14)
+
+#define NFC_CMD_GET_SIZE(x)(((x) >> 22) & GENMASK(4, 0))
+#define NFC_CMD_RB_TIMEOUT 0x18
Where does this value come from? Is it the typical timeout value,
and if it is, what's the value in milli/microseconds?
it is about 5.2ms when one nand cycle is 20ns and calculate it with the
max erase time of toshiba slc flash.i think it should be taken from the
sdr_timings.
+
+#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_RB_IRQ_EN BIT(21)
+#define NFC_INT_MASK (3 << 20)
+
+#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 DMA_DIR(dir) ((dir) ? NFC_CMD_N2M : NFC_CMD_M2N)
+
+#define ECC_CHECK_RETURN_FF(-1)
+
+#define NAND_CE0 (0xe << 10)
+#define NAND_CE1 (0xd << 10)
+
+#define DMA_BUSY_TIMEOUT 0x10
+#define CMD_FIFO_EMPTY_TIMEOUT 1000
+
+#define MAX_CE_NUM 2
+
+/* eMMC clock register, misc control */
+#define SD_EMMC_CLOCK 0x00
+#define CLK_ALWAYS_ON BIT(28)
+#define CLK_SELECT_NANDBIT(31)
+#define CLK_DIV_MASK GENMASK(5, 0)
+
+#define NFC_CLK_CYCLE 6
+
+/* nand flash controller delay 3 ns */
+#define NFC_DEFAULT_DELAY 3000
+
+#define MAX_ECC_INDEX 10
+
+#define MUX_CLK_NUM_PARENTS2
+
+#define ROW_ADDER(page, index) (((page) >> (8 * (index))) & 0xff)
+#define MAX_CYCLE_ROW_ADDRS3
+#define MAX_CYCLE_COLUMN_ADDRS 2
+#define DIRREAD1
+#define DIRWRITE 0
+
+#define ECC_PARITY_BCH8_512B 14
+
+#define PER_INFO_BYTE 8
+#define ECC_GET_PROTECTED_OOB_BYTE(x, y) \
+ ((x) >> (8 * (y)) & GENMASK(7, 0))
(le64_to_cpu(x) >> (8 * (y)) & GENMASK(7, 0))
+
+#define ECC_SET_PROTECTED_OOB_BYTE(x, y, z)\
+ { \
+ (x) &= (~((__le64)(0xff) << (8 * (y; \
It's probably better to memset(0) the info_buf so that you can drop
this masking step.
ok.
+ (x) |= ((__le64)(z) << (8 * (y)));\
|= cpu_to_le64((u64)z << (8 * (y)));
+ }
+
+#define ECC_COMPLETEBIT(31)
+#define ECC_ERR_CNT(x) (((x) >> 24) & GENMASK(5, 0))
+#define ECC_ZERO_CNT(x)(((x) >> 16) & GENMASK(5, 0))
+
+struct meson_nfc_nand_chip {
+ struct list_head node;
+ struct nand_chip nand;
+ int clk_rate;
+ int level1_divider;
+ int bus_timing;
+ int twb;
+ int tadl;
+
+ int bch_mode;
+ u8 *data_buf;
+ __le64 *info_buf;
+ int nsels;
+ u8 sels[0];
+};
Please use
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/11/5 23:53, Boris Brezillon wrote:
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
+#define NFC_REG_CMD0x00
+#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 BIT(20)
+#define NFC_CMD_IO6((0xb << 10) | (1 << 18))
+#define NFC_CMD_SCRAMBLER_ENABLE BIT(19)
+#define NFC_CMD_RB_INT BIT(14)
+
+#define NFC_CMD_GET_SIZE(x)(((x) >> 22) & GENMASK(4, 0))
+#define NFC_CMD_RB_TIMEOUT 0x18
Where does this value come from? Is it the typical timeout value,
and if it is, what's the value in milli/microseconds?
it is about 5.2ms when one nand cycle is 20ns and calculate it with the
max erase time of toshiba slc flash.i think it should be taken from the
sdr_timings.
+
+#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_RB_IRQ_EN BIT(21)
+#define NFC_INT_MASK (3 << 20)
+
+#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 DMA_DIR(dir) ((dir) ? NFC_CMD_N2M : NFC_CMD_M2N)
+
+#define ECC_CHECK_RETURN_FF(-1)
+
+#define NAND_CE0 (0xe << 10)
+#define NAND_CE1 (0xd << 10)
+
+#define DMA_BUSY_TIMEOUT 0x10
+#define CMD_FIFO_EMPTY_TIMEOUT 1000
+
+#define MAX_CE_NUM 2
+
+/* eMMC clock register, misc control */
+#define SD_EMMC_CLOCK 0x00
+#define CLK_ALWAYS_ON BIT(28)
+#define CLK_SELECT_NANDBIT(31)
+#define CLK_DIV_MASK GENMASK(5, 0)
+
+#define NFC_CLK_CYCLE 6
+
+/* nand flash controller delay 3 ns */
+#define NFC_DEFAULT_DELAY 3000
+
+#define MAX_ECC_INDEX 10
+
+#define MUX_CLK_NUM_PARENTS2
+
+#define ROW_ADDER(page, index) (((page) >> (8 * (index))) & 0xff)
+#define MAX_CYCLE_ROW_ADDRS3
+#define MAX_CYCLE_COLUMN_ADDRS 2
+#define DIRREAD1
+#define DIRWRITE 0
+
+#define ECC_PARITY_BCH8_512B 14
+
+#define PER_INFO_BYTE 8
+#define ECC_GET_PROTECTED_OOB_BYTE(x, y) \
+ ((x) >> (8 * (y)) & GENMASK(7, 0))
(le64_to_cpu(x) >> (8 * (y)) & GENMASK(7, 0))
+
+#define ECC_SET_PROTECTED_OOB_BYTE(x, y, z)\
+ { \
+ (x) &= (~((__le64)(0xff) << (8 * (y; \
It's probably better to memset(0) the info_buf so that you can drop
this masking step.
ok.
+ (x) |= ((__le64)(z) << (8 * (y)));\
|= cpu_to_le64((u64)z << (8 * (y)));
+ }
+
+#define ECC_COMPLETEBIT(31)
+#define ECC_ERR_CNT(x) (((x) >> 24) & GENMASK(5, 0))
+#define ECC_ZERO_CNT(x)(((x) >> 16) & GENMASK(5, 0))
+
+struct meson_nfc_nand_chip {
+ struct list_head node;
+ struct nand_chip nand;
+ int clk_rate;
+ int level1_divider;
+ int bus_timing;
+ int twb;
+ int tadl;
+
+ int bch_mode;
+ u8 *data_buf;
+ __le64 *info_buf;
+ int nsels;
+ u8 sels[0];
+};
Please use
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
> +#define NFC_REG_CMD 0x00
> +#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 BIT(20)
> +#define NFC_CMD_IO6 ((0xb << 10) | (1 << 18))
> +#define NFC_CMD_SCRAMBLER_ENABLE BIT(19)
> +#define NFC_CMD_RB_INT BIT(14)
> +
> +#define NFC_CMD_GET_SIZE(x) (((x) >> 22) & GENMASK(4, 0))
> +#define NFC_CMD_RB_TIMEOUT 0x18
Where does this value come from? Is it the typical timeout value,
and if it is, what's the value in milli/microseconds?
> +
> +#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
> +
> +#define NFC_RB_IRQ_ENBIT(21)
> +#define NFC_INT_MASK (3 << 20)
> +
> +#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 DMA_DIR(dir) ((dir) ? NFC_CMD_N2M : NFC_CMD_M2N)
> +
> +#define ECC_CHECK_RETURN_FF (-1)
> +
> +#define NAND_CE0 (0xe << 10)
> +#define NAND_CE1 (0xd << 10)
> +
> +#define DMA_BUSY_TIMEOUT 0x10
> +#define CMD_FIFO_EMPTY_TIMEOUT 1000
> +
> +#define MAX_CE_NUM 2
> +
> +/* eMMC clock register, misc control */
> +#define SD_EMMC_CLOCK0x00
> +#define CLK_ALWAYS_ONBIT(28)
> +#define CLK_SELECT_NAND BIT(31)
> +#define CLK_DIV_MASK GENMASK(5, 0)
> +
> +#define NFC_CLK_CYCLE6
> +
> +/* nand flash controller delay 3 ns */
> +#define NFC_DEFAULT_DELAY3000
> +
> +#define MAX_ECC_INDEX10
> +
> +#define MUX_CLK_NUM_PARENTS 2
> +
> +#define ROW_ADDER(page, index) (((page) >> (8 * (index))) & 0xff)
> +#define MAX_CYCLE_ROW_ADDRS 3
> +#define MAX_CYCLE_COLUMN_ADDRS 2
> +#define DIRREAD 1
> +#define DIRWRITE 0
> +
> +#define ECC_PARITY_BCH8_512B 14
> +
> +#define PER_INFO_BYTE8
> +#define ECC_GET_PROTECTED_OOB_BYTE(x, y) \
> + ((x) >> (8 * (y)) & GENMASK(7, 0))
(le64_to_cpu(x) >> (8 * (y)) & GENMASK(7, 0))
> +
> +#define ECC_SET_PROTECTED_OOB_BYTE(x, y, z) \
> + { \
> + (x) &= (~((__le64)(0xff) << (8 * (y;\
It's probably better to memset(0) the info_buf so that you can drop
this masking step.
> + (x) |= ((__le64)(z) << (8 * (y))); \
|= cpu_to_le64((u64)z << (8 * (y)));
> + }
> +
> +#define ECC_COMPLETEBIT(31)
> +#define ECC_ERR_CNT(x) (((x) >> 24) & GENMASK(5, 0))
> +#define ECC_ZERO_CNT(x) (((x) >> 16) & GENMASK(5, 0))
> +
> +struct meson_nfc_nand_chip {
> + struct list_head node;
> + struct nand_chip nand;
> + int clk_rate;
> + int level1_divider;
> + int bus_timing;
> + int twb;
> + int tadl;
> +
> + int bch_mode;
> + u8 *data_buf;
> + __le64 *info_buf;
> + int nsels;
> + u8 sels[0];
> +};
Please use unsigned integers where having a negative value is not
possible. I'm pretty sure
Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan wrote:
> +#define NFC_REG_CMD 0x00
> +#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 BIT(20)
> +#define NFC_CMD_IO6 ((0xb << 10) | (1 << 18))
> +#define NFC_CMD_SCRAMBLER_ENABLE BIT(19)
> +#define NFC_CMD_RB_INT BIT(14)
> +
> +#define NFC_CMD_GET_SIZE(x) (((x) >> 22) & GENMASK(4, 0))
> +#define NFC_CMD_RB_TIMEOUT 0x18
Where does this value come from? Is it the typical timeout value,
and if it is, what's the value in milli/microseconds?
> +
> +#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
> +
> +#define NFC_RB_IRQ_ENBIT(21)
> +#define NFC_INT_MASK (3 << 20)
> +
> +#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 DMA_DIR(dir) ((dir) ? NFC_CMD_N2M : NFC_CMD_M2N)
> +
> +#define ECC_CHECK_RETURN_FF (-1)
> +
> +#define NAND_CE0 (0xe << 10)
> +#define NAND_CE1 (0xd << 10)
> +
> +#define DMA_BUSY_TIMEOUT 0x10
> +#define CMD_FIFO_EMPTY_TIMEOUT 1000
> +
> +#define MAX_CE_NUM 2
> +
> +/* eMMC clock register, misc control */
> +#define SD_EMMC_CLOCK0x00
> +#define CLK_ALWAYS_ONBIT(28)
> +#define CLK_SELECT_NAND BIT(31)
> +#define CLK_DIV_MASK GENMASK(5, 0)
> +
> +#define NFC_CLK_CYCLE6
> +
> +/* nand flash controller delay 3 ns */
> +#define NFC_DEFAULT_DELAY3000
> +
> +#define MAX_ECC_INDEX10
> +
> +#define MUX_CLK_NUM_PARENTS 2
> +
> +#define ROW_ADDER(page, index) (((page) >> (8 * (index))) & 0xff)
> +#define MAX_CYCLE_ROW_ADDRS 3
> +#define MAX_CYCLE_COLUMN_ADDRS 2
> +#define DIRREAD 1
> +#define DIRWRITE 0
> +
> +#define ECC_PARITY_BCH8_512B 14
> +
> +#define PER_INFO_BYTE8
> +#define ECC_GET_PROTECTED_OOB_BYTE(x, y) \
> + ((x) >> (8 * (y)) & GENMASK(7, 0))
(le64_to_cpu(x) >> (8 * (y)) & GENMASK(7, 0))
> +
> +#define ECC_SET_PROTECTED_OOB_BYTE(x, y, z) \
> + { \
> + (x) &= (~((__le64)(0xff) << (8 * (y;\
It's probably better to memset(0) the info_buf so that you can drop
this masking step.
> + (x) |= ((__le64)(z) << (8 * (y))); \
|= cpu_to_le64((u64)z << (8 * (y)));
> + }
> +
> +#define ECC_COMPLETEBIT(31)
> +#define ECC_ERR_CNT(x) (((x) >> 24) & GENMASK(5, 0))
> +#define ECC_ZERO_CNT(x) (((x) >> 16) & GENMASK(5, 0))
> +
> +struct meson_nfc_nand_chip {
> + struct list_head node;
> + struct nand_chip nand;
> + int clk_rate;
> + int level1_divider;
> + int bus_timing;
> + int twb;
> + int tadl;
> +
> + int bch_mode;
> + u8 *data_buf;
> + __le64 *info_buf;
> + int nsels;
> + u8 sels[0];
> +};
Please use unsigned integers where having a negative value is not
possible. I'm pretty sure
[PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
From: Liang Yang Add initial support for the Amlogic NAND flash controller which found in the Meson-GXBB/GXL/AXG SoCs. Signed-off-by: Liang Yang Signed-off-by: Yixun Lan Signed-off-by: Jianxin Pan --- drivers/mtd/nand/raw/Kconfig | 10 + drivers/mtd/nand/raw/Makefile |1 + drivers/mtd/nand/raw/meson_nand.c | 1360 + 3 files changed, 1371 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 c7efc31..223b041 100644 --- a/drivers/mtd/nand/raw/Kconfig +++ b/drivers/mtd/nand/raw/Kconfig @@ -541,4 +541,14 @@ config MTD_NAND_TEGRA is supported. Extra OOB bytes when using HW ECC are currently not supported. +config MTD_NAND_MESON + tristate "Support for NAND controller on Amlogic's Meson SoCs" + depends on ARCH_MESON || COMPILE_TEST + depends on COMMON_CLK_AMLOGIC + select COMMON_CLK_REGMAP_MESON + select MFD_SYSCON + help + Enables support for NAND controller on Amlogic's Meson SoCs. + This controller is found on Meson GXBB, GXL, AXG SoCs. + endif # MTD_NAND diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile index 57159b3..a2cc2fe 100644 --- a/drivers/mtd/nand/raw/Makefile +++ b/drivers/mtd/nand/raw/Makefile @@ -56,6 +56,7 @@ 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_TEGRA) += tegra_nand.o +obj-$(CONFIG_MTD_NAND_MESON) += meson_nand.o nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o nand-objs += nand_onfi.o diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c new file mode 100644 index 000..e196c0d --- /dev/null +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -0,0 +1,1360 @@ +// 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 + +#define NFC_REG_CMD0x00 +#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 BIT(20) +#define NFC_CMD_IO6((0xb << 10) | (1 << 18)) +#define NFC_CMD_SCRAMBLER_ENABLE BIT(19) +#define NFC_CMD_RB_INT BIT(14) + +#define NFC_CMD_GET_SIZE(x)(((x) >> 22) & GENMASK(4, 0)) +#define NFC_CMD_RB_TIMEOUT 0x18 + +#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_RB_IRQ_EN BIT(21) +#define NFC_INT_MASK (3 << 20) + +#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 DMA_DIR(dir) ((dir) ? NFC_CMD_N2M : NFC_CMD_M2N) + +#define ECC_CHECK_RETURN_FF(-1) + +#define NAND_CE0 (0xe << 10) +#define NAND_CE1 (0xd << 10) + +#define
[PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
From: Liang Yang Add initial support for the Amlogic NAND flash controller which found in the Meson-GXBB/GXL/AXG SoCs. Signed-off-by: Liang Yang Signed-off-by: Yixun Lan Signed-off-by: Jianxin Pan --- drivers/mtd/nand/raw/Kconfig | 10 + drivers/mtd/nand/raw/Makefile |1 + drivers/mtd/nand/raw/meson_nand.c | 1360 + 3 files changed, 1371 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 c7efc31..223b041 100644 --- a/drivers/mtd/nand/raw/Kconfig +++ b/drivers/mtd/nand/raw/Kconfig @@ -541,4 +541,14 @@ config MTD_NAND_TEGRA is supported. Extra OOB bytes when using HW ECC are currently not supported. +config MTD_NAND_MESON + tristate "Support for NAND controller on Amlogic's Meson SoCs" + depends on ARCH_MESON || COMPILE_TEST + depends on COMMON_CLK_AMLOGIC + select COMMON_CLK_REGMAP_MESON + select MFD_SYSCON + help + Enables support for NAND controller on Amlogic's Meson SoCs. + This controller is found on Meson GXBB, GXL, AXG SoCs. + endif # MTD_NAND diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile index 57159b3..a2cc2fe 100644 --- a/drivers/mtd/nand/raw/Makefile +++ b/drivers/mtd/nand/raw/Makefile @@ -56,6 +56,7 @@ 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_TEGRA) += tegra_nand.o +obj-$(CONFIG_MTD_NAND_MESON) += meson_nand.o nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o nand-objs += nand_onfi.o diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c new file mode 100644 index 000..e196c0d --- /dev/null +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -0,0 +1,1360 @@ +// 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 + +#define NFC_REG_CMD0x00 +#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 BIT(20) +#define NFC_CMD_IO6((0xb << 10) | (1 << 18)) +#define NFC_CMD_SCRAMBLER_ENABLE BIT(19) +#define NFC_CMD_RB_INT BIT(14) + +#define NFC_CMD_GET_SIZE(x)(((x) >> 22) & GENMASK(4, 0)) +#define NFC_CMD_RB_TIMEOUT 0x18 + +#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_RB_IRQ_EN BIT(21) +#define NFC_INT_MASK (3 << 20) + +#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 DMA_DIR(dir) ((dir) ? NFC_CMD_N2M : NFC_CMD_M2N) + +#define ECC_CHECK_RETURN_FF(-1) + +#define NAND_CE0 (0xe << 10) +#define NAND_CE1 (0xd << 10) + +#define
