Re: [PATCH 4/4] mtd: rawnand: meson: only initialize the RB completion once
Hi Martin, On 2019/4/19 3:44, Martin Blumenstingl wrote: Hi Liang, On Mon, Apr 15, 2019 at 8:04 AM Liang Yang wrote: On 2019/4/12 6:00, Martin Blumenstingl wrote: Documentation/scheduler/completion.txt states: Calling init_completion() on the same completion object twice is most likely a bug as it re-initializes the queue to an empty queue and enqueued tasks could get "lost" - use reinit_completion() in that case, but be aware of other races. Initialize nfc->completion in meson_nfc_probe using init_completion and change the call in meson_nfc_queue_rb to reinit_completion so the logic matches what the documentation suggests. Signed-off-by: Martin Blumenstingl --- drivers/mtd/nand/raw/meson_nand.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c index 57cc4bd3f665..ea57ddcec41e 100644 --- a/drivers/mtd/nand/raw/meson_nand.c +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -400,7 +400,7 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) cfg |= NFC_RB_IRQ_EN; writel(cfg, nfc->reg_base + NFC_REG_CFG); - init_completion(>completion); + reinit_completion(>completion); Tested-by:Liang Yang Acked-by: Liang Yang thank you for reviewing and testing my patches! [...] Tested-by:Liang Yang Acked-by: Liang Yang please consider the following note for future code-reviews: most maintainers take the patch from patchwork and apply it to their git tree. however, patchwork is not smart enough to detect when the same Tested-by/Acked-by is sent multiple times. this results in the same Tested-by/Acked-by being listed multiple times in the final commit: [0] what I do instead is to reply with one set of Tested-by/Acked-by (below the author's Signed-off-by) which is then valid for the whole patch. There's no problem to have Tested-by and Acked-by at the same time, the issue only shows up if you send Acked-by (or any other tag) for the same patch multiple times. Thanks. Well, I known about it now. Have a great day! Regards, Martin [0] https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/commit/?h=nand/next=39e01956e2f70ff9f0e97db1a69c9847aa1d5d8b .
Re: [PATCH 4/4] mtd: rawnand: meson: only initialize the RB completion once
On 2019/4/12 6:00, Martin Blumenstingl wrote: Documentation/scheduler/completion.txt states: Calling init_completion() on the same completion object twice is most likely a bug as it re-initializes the queue to an empty queue and enqueued tasks could get "lost" - use reinit_completion() in that case, but be aware of other races. Initialize nfc->completion in meson_nfc_probe using init_completion and change the call in meson_nfc_queue_rb to reinit_completion so the logic matches what the documentation suggests. Signed-off-by: Martin Blumenstingl --- drivers/mtd/nand/raw/meson_nand.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c index 57cc4bd3f665..ea57ddcec41e 100644 --- a/drivers/mtd/nand/raw/meson_nand.c +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -400,7 +400,7 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) cfg |= NFC_RB_IRQ_EN; writel(cfg, nfc->reg_base + NFC_REG_CFG); - init_completion(>completion); + reinit_completion(>completion); Tested-by:Liang Yang Acked-by: Liang Yang /* use the max erase time as the maximum clock for waiting R/B */ cmd = NFC_CMD_RB | NFC_CMD_RB_INT @@ -1380,6 +1380,7 @@ static int meson_nfc_probe(struct platform_device *pdev) nand_controller_init(>controller); INIT_LIST_HEAD(>chips); + init_completion(>completion); Tested-by:Liang Yang Acked-by: Liang Yang nfc->dev = dev;
Re: [PATCH 3/4] mtd: rawnand: meson: use a void pointer for meson_nfc_dma_buffer_setup
On 2019/4/12 6:00, Martin Blumenstingl wrote:
This simplifies the code because it gets rid of the casts to an
u8-pointer when passing "info_buf" from struct meson_nfc_nand_chip.
Also it gets rid of the cast of the u8 databuf pointer to a void
pointer.
The logic inside meson_nfc_dma_buffer_setup() doesn't care about the
pointer types themselves because it only passes them to dma_map_single
which accepts a void pointer.
No functional changes.
Signed-off-by: Martin Blumenstingl
---
drivers/mtd/nand/raw/meson_nand.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index 9a6023638101..57cc4bd3f665 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -470,15 +470,15 @@ static int meson_nfc_ecc_correct(struct nand_chip *nand,
u32 *bitflips,
return ret;
}
-static int meson_nfc_dma_buffer_setup(struct nand_chip *nand, u8 *databuf,
- int datalen, u8 *infobuf, int infolen,
+static int meson_nfc_dma_buffer_setup(struct nand_chip *nand, void *databuf,
+ int datalen, void *infobuf, int infolen,
enum dma_data_direction dir)
Tested-by:Liang Yang
Acked-by: Liang Yang
{
struct meson_nfc *nfc = nand_get_controller_data(nand);
u32 cmd;
int ret = 0;
- nfc->daddr = dma_map_single(nfc->dev, (void *)databuf, datalen, dir);
+ nfc->daddr = dma_map_single(nfc->dev, databuf, datalen, dir);
ret = dma_mapping_error(nfc->dev, nfc->daddr);
if (ret) {
dev_err(nfc->dev, "DMA mapping error\n");
@@ -645,7 +645,7 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
return ret;
ret = meson_nfc_dma_buffer_setup(nand, meson_chip->data_buf,
-data_len, (u8 *)meson_chip->info_buf,
+data_len, meson_chip->info_buf,
info_len, DMA_TO_DEVICE);
Tested-by:Liang Yang
Acked-by: Liang Yang
if (ret)
return ret;
@@ -729,7 +729,7 @@ static int meson_nfc_read_page_sub(struct nand_chip *nand,
return ret;
ret = meson_nfc_dma_buffer_setup(nand, meson_chip->data_buf,
-data_len, (u8 *)meson_chip->info_buf,
+data_len, meson_chip->info_buf,
Tested-by:Liang Yang
Acked-by: Liang Yang
info_len, DMA_FROM_DEVICE);
if (ret)
return ret;
Re: [PATCH 2/4] mtd: rawnand: meson: use of_property_count_elems_of_size helper
On 2019/4/12 6:00, Martin Blumenstingl wrote:
Use the of_property_count_elems_of_size() helper instead of open-coding
it's logic. As a bonus this will now error out if the "reg" property
values use an incorrect size (anything other than sizeof(u32)).
Signed-off-by: Martin Blumenstingl
---
drivers/mtd/nand/raw/meson_nand.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index c1a6af57dab5..9a6023638101 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -1233,10 +1233,7 @@ meson_nfc_nand_chip_init(struct device *dev,
int ret, i;
u32 tmp, nsels;
- if (!of_get_property(np, "reg", ))
- return -EINVAL;
-
- nsels /= sizeof(u32);
+ nsels = of_property_count_elems_of_size(np, "reg", sizeof(u32));
Tested-by:Liang Yang
Acked-by: Liang Yang
if (!nsels || nsels > MAX_CE_NUM) {
dev_err(dev, "invalid register property size\n");
return -EINVAL;
Re: [PATCH 1/4] mtd: rawnand: meson: use struct_size macro
Hello Martin and Miquel, On 2019/4/12 6:00, Martin Blumenstingl wrote: Use the recently introduced struct_size macro instead of open-coding it's logic. No functional changes. Signed-off-by: Martin Blumenstingl --- drivers/mtd/nand/raw/meson_nand.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c index cb0b03e36a35..c1a6af57dab5 100644 --- a/drivers/mtd/nand/raw/meson_nand.c +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -1242,8 +1242,7 @@ meson_nfc_nand_chip_init(struct device *dev, return -EINVAL; } - meson_chip = devm_kzalloc(dev, - sizeof(*meson_chip) + (nsels * sizeof(u8)), + meson_chip = devm_kzalloc(dev, struct_size(meson_chip, sels, nsels), GFP_KERNEL); Tested-by:Liang Yang Acked-by: Liang Yang if (!meson_chip) return -ENOMEM;
Re: 32-bit Amlogic (ARM) SoC: kernel BUG in kfree()
Hi Martin, On 2019/4/11 1:54, Martin Blumenstingl wrote: Hi Liang, On Wed, Apr 10, 2019 at 1:08 PM Liang Yang wrote: Hi Martin, On 2019/4/5 12:30, Martin Blumenstingl wrote: Hi Liang, On Fri, Mar 29, 2019 at 8:44 AM Liang Yang wrote: Hi Martin, On 2019/3/29 2:03, Martin Blumenstingl wrote: Hi Liang, [..] I don't think it is caused by a different NAND type, but i have followed the some test on my GXL platform. we can see the result from the attachment. By the way, i don't find any information about this on meson NFC datasheet, so i will ask our VLSI. Martin, May you reproduce it with the new patch on meson8b platform ? I need a more clear and easier compared log like gxl.txt. Thanks. your gxl.txt is great, finally I can also compare my own results with something that works for you! in my results (see attachment) the "DATA_IN [256 B, force 8-bit]" instructions result in a different info buffer output. does this make any sense to you? I have asked our VLSI designer for explanation or simulation result by an e-mail. Thanks. do you have any update on this? Sorry. I haven't got reply from VLSI designer yet. We tried to improve priority yesterday, but i still can't estimate the time. There is no document or change list showing the difference between m8/b and gxl/axg serial chips. Now it seems that we can't use command NFC_CMD_N2M on nand initialization for m8/b chips and use *read byte from NFC fifo register* instead. thank you for the status update! I am trying to understand your suggestion not to use NFC_CMD_N2M: the documentation (public S922X datasheet from Hardkernel: [0]) states that P_NAND_BUF (NFC_REG_BUF in the meson_nand driver) can hold up to four bytes of data. is this the "read byte from NFC FIFO register" you mentioned? You are right.take the early meson NFC driver V2 on previous mail as a reference. Before I spend time changing the code to use the FIFO register I would like to wait for an answer from your VLSI designer. Setting the "correct" info buffer length for NFC_CMD_N2M on the 32-bit SoCs seems like an easier solution compared to switching to the FIFO register. Keeping NFC_CMD_N2M on the 32-bit SoCs also allows us to have only one code-path for 32 and 64 bit SoCs, meaning we don't have to maintain two separate code-paths for basically the same functionality (assuming that NFC_CMD_N2M is not completely broken on the 32-bit SoCs, we just don't know how to use it yet). All right. I am also waiting for the answer. Regards Martin [0] https://dn.odroid.com/S922X/ODROID-N2/Datasheet/S922X_Public_Datasheet_V0.2.pdf .
Re: 32-bit Amlogic (ARM) SoC: kernel BUG in kfree()
Hi Martin, On 2019/4/5 12:30, Martin Blumenstingl wrote: Hi Liang, On Fri, Mar 29, 2019 at 8:44 AM Liang Yang wrote: Hi Martin, On 2019/3/29 2:03, Martin Blumenstingl wrote: Hi Liang, [..] I don't think it is caused by a different NAND type, but i have followed the some test on my GXL platform. we can see the result from the attachment. By the way, i don't find any information about this on meson NFC datasheet, so i will ask our VLSI. Martin, May you reproduce it with the new patch on meson8b platform ? I need a more clear and easier compared log like gxl.txt. Thanks. your gxl.txt is great, finally I can also compare my own results with something that works for you! in my results (see attachment) the "DATA_IN [256 B, force 8-bit]" instructions result in a different info buffer output. does this make any sense to you? I have asked our VLSI designer for explanation or simulation result by an e-mail. Thanks. do you have any update on this? Sorry. I haven't got reply from VLSI designer yet. We tried to improve priority yesterday, but i still can't estimate the time. There is no document or change list showing the difference between m8/b and gxl/axg serial chips. Now it seems that we can't use command NFC_CMD_N2M on nand initialization for m8/b chips and use *read byte from NFC fifo register* instead. Martin .
Re: 32-bit Amlogic (ARM) SoC: kernel BUG in kfree()
Hi Martin, On 2019/3/29 2:03, Martin Blumenstingl wrote: Hi Liang, [..] I don't think it is caused by a different NAND type, but i have followed the some test on my GXL platform. we can see the result from the attachment. By the way, i don't find any information about this on meson NFC datasheet, so i will ask our VLSI. Martin, May you reproduce it with the new patch on meson8b platform ? I need a more clear and easier compared log like gxl.txt. Thanks. your gxl.txt is great, finally I can also compare my own results with something that works for you! in my results (see attachment) the "DATA_IN [256 B, force 8-bit]" instructions result in a different info buffer output. does this make any sense to you? I have asked our VLSI designer for explanation or simulation result by an e-mail. Thanks.
Re: 32-bit Amlogic (ARM) SoC: kernel BUG in kfree()
Hi Martin,
Thanks a lot.
On 2019/3/26 2:31, Martin Blumenstingl wrote:
Hi Liang,
On Mon, Mar 25, 2019 at 11:03 AM Liang Yang wrote:
Hi Martin,
On 2019/3/23 5:07, Martin Blumenstingl wrote:
Hi Matthew,
On Thu, Mar 21, 2019 at 10:44 PM Matthew Wilcox wrote:
On Thu, Mar 21, 2019 at 09:17:34PM +0100, Martin Blumenstingl wrote:
Hello,
I am experiencing the following crash:
[ cut here ]
kernel BUG at mm/slub.c:3950!
if (unlikely(!PageSlab(page))) {
BUG_ON(!PageCompound(page));
You called kfree() on the address of a page which wasn't allocated by slab.
I have traced this crash to the kfree() in meson_nfc_read_buf().
my observation is as follows:
- meson_nfc_read_buf() is called 7 times without any crash, the
kzalloc() call returns 0xe9e6c600 (virtual address) / 0x29e6c600
(physical address)
- the eight time meson_nfc_read_buf() is called kzalloc() call returns
0xee39a38b (virtual address) / 0x2e39a38b (physical address) and the
final kfree() crashes
- changing the size in the kzalloc() call from PER_INFO_BYTE (= 8) to
PAGE_SIZE works around that crash
I suspect you're doing something which corrupts memory. Overrunning
the end of your allocation or something similar. Have you tried KASAN
or even the various slab debugging (eg redzones)?
KASAN is not available on 32-bit ARM. there was some progress last
year [0] but it didn't make it into mainline. I tried to make the
patches apply again and got it to compile (and my kernel is still
booting) but I have no idea if it's still working. for anyone
interested, my patches are here: [1] (I consider this a HACK because I
don't know anything about the code which is being touched in the
patches, I only made it compile)
SLAB debugging (redzones) were a great hint, thank you very much for
that Matthew! I enabled:
CONFIG_SLUB_DEBUG=y
CONFIG_SLUB_DEBUG_ON=y
and with that I now get "BUG kmalloc-64 (Not tainted): Redzone
overwritten" (a larger kernel log extract is attached).
I'm starting to wonder if the NAND controller (hardware) writes more
than 8 bytes.
some context: the "info" buffer allocated in meson_nfc_read_buf is
then passed to the NAND controller IP (after using dma_map_single).
Liang, how does the NAND controller know that it only has to send
PER_INFO_BYTE (= 8) bytes when called from meson_nfc_read_buf? all
other callers of meson_nfc_dma_buffer_setup (which passes the info
buffer to the hardware) are using (nand->ecc.steps * PER_INFO_BYTE)
bytes?
NFC_CMD_N2M and CMDRWGEN are different commands. CMDRWGEN needs to set
the ecc page size (1KB or 512B) and Pages(2, 4, 8, ...), so
PER_INFO_BYTE(= 8) bytes for each ecc page.
I have never used NFC_CMD_N2M to transfer data before, because it is
very low efficient. And I do a experiment with the attachment and find
on overwritten on my meson axg platform.
Martin, I would appreciate it very much if you would try the attachment
on your meson m8b platform.
thank you for your debug patch! on my board 2 * PER_INFO_BYTE is not enough.
I took the idea from your patch and adapted it so I could print a
buffer with 256 bytes (which seems to be "big enough" for my board).
it only needs PER_INFO_BYTE (= 8) bytes, because NFC_CMD_N2M don't set
*Pages*, that is not like CMDRWGEN which needs Pages*PER_INFO_BYTE (= 8)
bytes when setting *Pages* parameter. I have been thinking that
NFC_CMD_N2M only occupis PER_INFO_BYTE (= 8) bytes. And i have tried to
not set the info address, the machine would crash.
see the attached, modified patch
in the output I see that sometimes the first 32 bytes are not touched
by the controller, but everything beyond 32 bytes is modified in the
info buffer.
it really makes sense that the controller sometimes fills the space
beyond the first 8 bytes. However i expect the controller should only
take the first 8 bytes when using NFC_CMD_N2M.
I also tried to increase the buffer size to 512, but that didn't make
a difference (I never saw any info buffer modification beyond 256
bytes).
also I just noticed that I didn't give you much details on my NAND chip yet.
from Amlogic vendor u-boot on Meson8m2 (all my Meson8b boards have
eMMC flash, but I believe the NAND controller on Meson8 to GXBB is
identical):
m8m2_n200_v1#amlnf chipinfo
flash info
name:B revision 20nm NAND 8GiB H27UCG8T2B, id:ad de 94 eb 74 44 0 0
pagesize:0x4000, blocksize:0x40, oobsize:0x500, chipsize:0x2000,
option:0x8, T_REA:16, T_RHOH:15
hw controller info
chip_num:1, onfi_mode:0, page_shift:14, block_shift:22, option:0xc2
ecc_unit:1024, ecc_bytes:70, ecc_steps:16, ecc_max:40
bch_mode:5, user_mode:2, oobavail:32, oobtail:64384
I don't think it is caused by a different NAND type, but i have followed
the some test on my GXL platform. we can see the result from the
attachment. By the way, i don't find any information about this on meson
NFC datasheet, so i will ask ou
Re: 32-bit Amlogic (ARM) SoC: kernel BUG in kfree()
Hi Martin,
On 2019/3/23 5:07, Martin Blumenstingl wrote:
Hi Matthew,
On Thu, Mar 21, 2019 at 10:44 PM Matthew Wilcox wrote:
On Thu, Mar 21, 2019 at 09:17:34PM +0100, Martin Blumenstingl wrote:
Hello,
I am experiencing the following crash:
[ cut here ]
kernel BUG at mm/slub.c:3950!
if (unlikely(!PageSlab(page))) {
BUG_ON(!PageCompound(page));
You called kfree() on the address of a page which wasn't allocated by slab.
I have traced this crash to the kfree() in meson_nfc_read_buf().
my observation is as follows:
- meson_nfc_read_buf() is called 7 times without any crash, the
kzalloc() call returns 0xe9e6c600 (virtual address) / 0x29e6c600
(physical address)
- the eight time meson_nfc_read_buf() is called kzalloc() call returns
0xee39a38b (virtual address) / 0x2e39a38b (physical address) and the
final kfree() crashes
- changing the size in the kzalloc() call from PER_INFO_BYTE (= 8) to
PAGE_SIZE works around that crash
I suspect you're doing something which corrupts memory. Overrunning
the end of your allocation or something similar. Have you tried KASAN
or even the various slab debugging (eg redzones)?
KASAN is not available on 32-bit ARM. there was some progress last
year [0] but it didn't make it into mainline. I tried to make the
patches apply again and got it to compile (and my kernel is still
booting) but I have no idea if it's still working. for anyone
interested, my patches are here: [1] (I consider this a HACK because I
don't know anything about the code which is being touched in the
patches, I only made it compile)
SLAB debugging (redzones) were a great hint, thank you very much for
that Matthew! I enabled:
CONFIG_SLUB_DEBUG=y
CONFIG_SLUB_DEBUG_ON=y
and with that I now get "BUG kmalloc-64 (Not tainted): Redzone
overwritten" (a larger kernel log extract is attached).
I'm starting to wonder if the NAND controller (hardware) writes more
than 8 bytes.
some context: the "info" buffer allocated in meson_nfc_read_buf is
then passed to the NAND controller IP (after using dma_map_single).
Liang, how does the NAND controller know that it only has to send
PER_INFO_BYTE (= 8) bytes when called from meson_nfc_read_buf? all
other callers of meson_nfc_dma_buffer_setup (which passes the info
buffer to the hardware) are using (nand->ecc.steps * PER_INFO_BYTE)
bytes?
NFC_CMD_N2M and CMDRWGEN are different commands. CMDRWGEN needs to set
the ecc page size (1KB or 512B) and Pages(2, 4, 8, ...), so
PER_INFO_BYTE(= 8) bytes for each ecc page.
I have never used NFC_CMD_N2M to transfer data before, because it is
very low efficient. And I do a experiment with the attachment and find
on overwritten on my meson axg platform.
Martin, I would appreciate it very much if you would try the attachment
on your meson m8b platform.
Regards
Martin
[0] https://lore.kernel.org/patchwork/cover/913212/
[1] https://github.com/xdarklight/linux/tree/arm-kasan-hack-v5.1-rc1
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
old mode 100644
new mode 100755
index e858d58..905ef39
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -527,11 +527,12 @@ static void meson_nfc_dma_buffer_release(struct nand_chip
*nand,
static int meson_nfc_read_buf(struct nand_chip *nand, u8 *buf, int len)
{
struct meson_nfc *nfc = nand_get_controller_data(nand);
- int ret = 0;
+ int ret = 0, i;
u32 cmd;
u8 *info;
- info = kzalloc(PER_INFO_BYTE, GFP_KERNEL);
+ info = kzalloc(2 * PER_INFO_BYTE, GFP_KERNEL);
+ memset(info, 0xFD, 2 * PER_INFO_BYTE);
ret = meson_nfc_dma_buffer_setup(nand, buf, len, info,
PER_INFO_BYTE, DMA_FROM_DEVICE);
if (ret)
@@ -543,6 +544,12 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8
*buf, int len)
meson_nfc_drain_cmd(nfc);
meson_nfc_wait_cmd_finish(nfc, 1000);
meson_nfc_dma_buffer_release(nand, len, PER_INFO_BYTE, DMA_FROM_DEVICE);
+
+ for (i = 0; i < 2 * PER_INFO_BYTE; i++){
+ printk("0x%x ", info[i]);
+ }
+ printk("\n");
+
kfree(info);
return ret;
[PATCH 1/1] mtd: rawnand: meson: set oob layout ops
Specify the oob layout operation to avoid no oob scheme defined for
some nand flash.
Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash
controller")
Signed-off-by: Liang Yang
---
drivers/mtd/nand/raw/meson_nand.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index 3e8aa71..a2154a2 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -1183,6 +1183,8 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
return -EINVAL;
}
+ mtd_set_ooblayout(mtd, _ooblayout_ops);
+
ret = meson_nand_bch_mode(nand);
if (ret)
return -EINVAL;
--
1.9.1
Re: [RFC PATCH nand-next 0/2] meson-nand: support for older SoCs
Hi Martin, On 2019/3/21 4:48, Martin Blumenstingl wrote: Hi Liang, On Wed, Mar 20, 2019 at 4:32 AM Liang Yang wrote: Hi Martin, Thanks for your time. On 2019/3/20 4:27, Martin Blumenstingl wrote: Hello Liang, On Sat, Mar 16, 2019 at 11:55 AM Martin Blumenstingl wrote: [...] Allocating a fixed size info buffer before nand_scan_ident and attach it on the struct meson_nfc; Or considering not use dma for reading data less than 8 bytes. Both can reduce kmalloc and kfree calling. Thanks. both suggestions sound reasonable. however, I will search for the root cause of the unaligned address first before changing the Meson NFC driver. That is good. And i will implement one of both mentioned above soon. [...] [2.227374] [ cut here ] [2.231968] WARNING: CPU: 1 PID: 1 at drivers/mtd/nand/raw/nand_base.c:5503 nand_scan_with_ids+0x1718/0x171c [2.241760] No oob scheme defined for oobsize 1280 ... (the "No oob scheme defined for oobsize 1280" message is expected) miss mtd_set_ooblayout(mtd, _ooblayout_ops) on function meson_nand_attach_chip. thank you for the suggestion. I didn't have time to test this on my board yet but I'll let you know about my results during the weekend. Does the missing mtd_set_ooblayout() call also affect GXL or AXG boards? Yes. I deleted it unintentionally. Regards Martin .
Re: [RFC PATCH nand-next 0/2] meson-nand: support for older SoCs
Hi Martin, Thanks for your time. On 2019/3/20 4:27, Martin Blumenstingl wrote: Hello Liang, On Sat, Mar 16, 2019 at 11:55 AM Martin Blumenstingl wrote: [...] Martin, Now i am not sure whether NFC driver leads to kernel panic when calling kmem_cache_alloc_trace. thank you for confirming that it works for you on GXL I'm not sure that this is a NFC driver problem. after enabling CONFIG_SLAB_FREELIST_HARDENED in my kernel config the crash moves. it's now crashing in slub.c's kfree() at BUG_ON(!PageCompound(page)); I added some debug prints in meson_nfc_read_buf() to get some details about the info buffer before the crash, format is: meson_nfc_read_buf during my first test three different addresses are used: - meson_nfc_read_buf e9e6c640 0x29e6c640 (works fine) - meson_nfc_read_buf e9e6c680 0x29e6c680 (works fine) - meson_nfc_read_buf ee39a34b 0x2e39a34b (crashes during kfree) so I tried playing around with the allocation size (see the attached patch) and changed it to: kzalloc(PER_INFO_BYTE + 64, GFP_KERNEL) this results in the following addresses being used: - meson_nfc_read_buf e9ea4280 0x29ea4280 (works fine) - meson_nfc_read_buf e9ea4300 0x29ea4300 (works fine) (there is no crash anymore) Liang, are there any special requirements on the "info address" like the alignment? It must be 4 bytes alignment. i have met it previously when debugging NFC driver on AXG platform, but it is not specified on spec. Now i am confused that how to get the no aligned address "xe39a34b" when using kmalloc; i think it should return the aligned address. doesn't it? also do you know why the PER_INFO_BYTE buffer is allocated dynamically in meson_nfc_read_buf() instead of allocating it at initialization? I'm not saying that it should be changed! I'm curious because there's per-meson_nfc_nand_chip info and data buffers which are allocated at initialization time. NAND scan or initialization is divided into three stages: nand_scan_ident->nand_attach->nand_scan_tail. info and data buffer which depend on the result of nand_scan_ident are allocated on nand_attach; so nand_scan_ident can not use the info buffer on meson_nfc_nand_chip. Allocating a fixed size info buffer before nand_scan_ident and attach it on the struct meson_nfc; Or considering not use dma for reading data less than 8 bytes. Both can reduce kmalloc and kfree calling. Thanks. meson_nfc_read_buf debug log with PER_INFO_BYTE allocation: [2.032914] meson_nfc_read_buf e9e6c640 0x29e6c640 [2.033005] meson_nfc_dma_buffer_setup 0x29e6c640 [2.037717] meson_nfc_read_buf: about to kfree info [2.042535] meson_nfc_read_buf: kfree'd info [2.046794] meson_nfc_read_buf e9e6c640 0x29e6c640 [2.051552] meson_nfc_dma_buffer_setup 0x29e6c640 [2.056261] meson_nfc_read_buf: about to kfree info [2.061086] meson_nfc_read_buf: kfree'd info [2.065356] meson_nfc_read_buf e9e6c680 0x29e6c680 [2.070102] meson_nfc_dma_buffer_setup 0x29e6c680 [2.074810] meson_nfc_read_buf: about to kfree info [2.079635] meson_nfc_read_buf: kfree'd info [2.083978] meson_nfc_read_buf e9e6c640 0x29e6c640 [2.088684] meson_nfc_dma_buffer_setup 0x29e6c640 [2.093334] meson_nfc_read_buf: about to kfree info [2.098199] meson_nfc_read_buf: kfree'd info [2.102446] meson_nfc_read_buf e9e6c640 0x29e6c640 [2.107208] meson_nfc_dma_buffer_setup 0x29e6c640 [2.111883] meson_nfc_read_buf: about to kfree info [2.116765] meson_nfc_read_buf: kfree'd info [2.120996] meson_nfc_read_buf e9e6c640 0x29e6c640 [2.125762] meson_nfc_dma_buffer_setup 0x29e6c640 [2.130433] meson_nfc_read_buf: about to kfree info [2.135294] meson_nfc_read_buf: kfree'd info [2.139545] Could not find a valid ONFI parameter page, trying bit-wise majority to recover it [2.148173] ONFI parameter recovery failed, aborting [2.153058] meson_nfc_read_buf e9e6c680 0x29e6c680 [2.157831] meson_nfc_dma_buffer_setup 0x29e6c680 [2.162527] meson_nfc_read_buf: about to kfree info [2.167369] meson_nfc_read_buf: kfree'd info [2.171611] meson_nfc_read_buf ee39a34b 0x2e39a34b [2.176383] meson_nfc_dma_buffer_setup 0x2e39a34b [2.181076] meson_nfc_read_buf: about to kfree info [2.185932] [ cut here ] [2.190503] kernel BUG at mm/slub.c:3950! [2.194491] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM ... meson_nfc_read_buf debug log with PER_INFO_BYTE+64 allocation: [2.033019] meson_nfc_read_buf e9ea4280 0x29ea4280 [2.033112] meson_nfc_dma_buffer_setup 0x29ea4280 [2.037847] meson_nfc_read_buf: about to kfree info [2.042642] meson_nfc_read_buf: kfree'd info [2.046909] meson_nfc_read_buf e9ea4280 0x29ea4280 [2.051659] meson_nfc_dma_buffer_setup 0x29ea4280 [2.056374] meson_nfc_read_buf: about to kfree info [2.061192] meson_nfc_read_buf: kfree'd info [2.065461] meson_nfc_read_buf e9ea4280 0x29ea4280 [2.070208] meson_nfc_dma_buffer_setup 0x29ea4280 [
Re: [RFC PATCH nand-next 0/2] meson-nand: support for older SoCs
Hi Martin and Miquel, On 2019/3/7 21:09, Miquel Raynal wrote: Hello, Martin Blumenstingl wrote on Tue, 5 Mar 2019 23:12:51 +0100: Hi Liang, On Mon, Mar 4, 2019 at 5:55 AM Liang Yang wrote: Hello Martin, On 2019/3/2 2:29, Martin Blumenstingl wrote: Hi Liang, I am trying to add support for older SoCs to the meson-nand driver. Back when the driver was in development I used an early revision (of your driver) and did some modifications to make it work on older SoCs. Now that the driver is upstream I wanted to give it another try and make a real patch out of it. Unfortunately it's not working anymore. As far as I know the NFC IP block revision on GXL is similar (or even the same?) as on all older SoCs. As far as I can tell only the clock setup is different on the older SoCs (which have a dedicated NAND clock): - we don't need the "amlogic,mmc-syscon" property on the older SoCs because we don't need to setup any muxing (common clock framework will do everything for us) - "rx" and "tx" clocks don't exist - I could not find any other differences between Meson8, Meson8b, Meson8m2, GXBB and GXL That is right. the serials NFC is almost the same except: 1) The clock control and source that M8-serials are not share with EMMC. 2) The base register address 3) DMA encryption option which we don't care on NFC driver. great, thank you for confirming this! In this series I'm sending two patches which add support for the older SoCs. Unfortunately these patches are currently not working for me (hence the "RFC" prefix). I get a (strange) crash which is triggered by the kzalloc() in meson_nfc_read_buf() - see below for more details. Can you please help me on this one? I'd like to know whether: - the meson-nand driver works for you on GXL or AXG on linux-next? (I was running these patches on top of next-20190301 on my M8S board which uses a 32-bit Meson8m2 SoC. I don't have any board using a GXL SoC which also has NAND) Yes, it works on AXG platform using a MXIC slc nand flash(MX30LF4G); but i an not sure it runs the same flow with yours. because i see the print "Counld not find a valid ONFI parameter page, " in yours. i will try to reproduce it on AXG(i don't have a M8 platform now). I'm looking forward to hear about the test results on your AXG boards for reference: my board has a SK Hynix H27UCG8T2B (ID bytes: 0xad 0xde 0x94 0xeb 0x74 0x44, 20nm MLC) I have another board (where I haven't tested the NFC driver yet) with a SK Hynix H27UCG8T2E (ID bytes: 0xad 0xde 0x14 0xa7 0x42 0x4a, 1Ynm MLC). if it helps with your analysis I can test on that board as well Liang, you just have to fake the output of the ONFI page detection and you will probably run into this error which will then be easy to reproduce. i don't reproduce it by using a SK Hynix nand flash H27UCG8T2E on gxl platform. it runs well. [..] [0.977127] loop: module loaded [0.998625] Could not find a valid ONFI parameter page, trying bit-wise majority to recover it [1.001619] ONFI parameter recovery failed, aborting [1.006684] Could not find valid JEDEC parameter page; aborting [1.012391] nand: device found, Manufacturer ID: 0xad, Chip ID: 0xde [1.018660] nand: Hynix NAND 8GiB 3,3V 8-bit [1.022885] nand: 8192 MiB, MLC, erase size: 4096 KiB, page size: 16384, OOB size: 1664 [1.047033] Bad block table not found for chip 0 [1.054950] Bad block table not found for chip 0 [1.054970] Scanning device for bad blocks [1.522664] random: fast init done [4.893731] Bad eraseblock 1985 at 0x0001f07fc000 [5.020637] Bad block table written to 0x0001ffc0, version 0x01 [5.028258] Bad block table written to 0x0001ff80, version 0x01 [5.029905] 5 fixed-partitions partitions found on MTD device d0074800.nfc [5.035714] Creating 5 MTD partitions on "d0074800.nfc": [..] Martin, Now i am not sure whether NFC driver leads to kernel panic when calling kmem_cache_alloc_trace. .
Re: [RFC PATCH nand-next 0/2] meson-nand: support for older SoCs
Hi Martin, On 2019/3/7 21:09, Miquel Raynal wrote: Hello, Martin Blumenstingl wrote on Tue, 5 Mar 2019 23:12:51 +0100: Hi Liang, On Mon, Mar 4, 2019 at 5:55 AM Liang Yang wrote: Hello Martin, On 2019/3/2 2:29, Martin Blumenstingl wrote: Hi Liang, I am trying to add support for older SoCs to the meson-nand driver. Back when the driver was in development I used an early revision (of your driver) and did some modifications to make it work on older SoCs. Now that the driver is upstream I wanted to give it another try and make a real patch out of it. Unfortunately it's not working anymore. As far as I know the NFC IP block revision on GXL is similar (or even the same?) as on all older SoCs. As far as I can tell only the clock setup is different on the older SoCs (which have a dedicated NAND clock): - we don't need the "amlogic,mmc-syscon" property on the older SoCs because we don't need to setup any muxing (common clock framework will do everything for us) - "rx" and "tx" clocks don't exist - I could not find any other differences between Meson8, Meson8b, Meson8m2, GXBB and GXL That is right. the serials NFC is almost the same except: 1) The clock control and source that M8-serials are not share with EMMC. 2) The base register address 3) DMA encryption option which we don't care on NFC driver. great, thank you for confirming this! In this series I'm sending two patches which add support for the older SoCs. Unfortunately these patches are currently not working for me (hence the "RFC" prefix). I get a (strange) crash which is triggered by the kzalloc() in meson_nfc_read_buf() - see below for more details. Can you please help me on this one? I'd like to know whether: - the meson-nand driver works for you on GXL or AXG on linux-next? (I was running these patches on top of next-20190301 on my M8S board which uses a 32-bit Meson8m2 SoC. I don't have any board using a GXL SoC which also has NAND) Yes, it works on AXG platform using a MXIC slc nand flash(MX30LF4G); but i an not sure it runs the same flow with yours. because i see the print "Counld not find a valid ONFI parameter page, " in yours. i will try to reproduce it on AXG(i don't have a M8 platform now). I'm looking forward to hear about the test results on your AXG boards for reference: my board has a SK Hynix H27UCG8T2B (ID bytes: 0xad 0xde 0x94 0xeb 0x74 0x44, 20nm MLC) I have another board (where I haven't tested the NFC driver yet) with a SK Hynix H27UCG8T2E (ID bytes: 0xad 0xde 0x14 0xa7 0x42 0x4a, 1Ynm MLC). if it helps with your analysis I can test on that board as well Liang, you just have to fake the output of the ONFI page detection and you will probably run into this error which will then be easy to reproduce. I have tested it on AXG platform; I find MX30LF4G also enter this flow , but it doesn't crash. log as follow: [1.018056] Could not find a valid ONFI parameter page, trying bit-wise majority to recover it [1.021057] ONFI parameter recovery failed, aborting [1.025966] nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xdc [1.032237] nand: Macronix NAND 512MiB 3,3V 8-bit [1.036889] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64 [1.045741] Bad block table not found for chip 0 [1.050077] Bad block table not found for chip 0 [1.053538] Scanning device for bad blocks [1.069094] Bad eraseblock 20 at 0x0028 [1.071074] Bad eraseblock 24 at 0x0030 [1.127494] random: fast init done [1.348754] Bad eraseblock 519 at 0x040e [1.632819] Bad eraseblock 1028 at 0x0808 [2.405420] Bad eraseblock 2411 at 0x12d6 [3.349276] Bad block table written to 0x1ffe, version 0x01 [3.350967] Bad block table written to 0x1ffc, version 0x01 [3.356429] 5 fixed-partitions partitions found on MTD device ffe07800.nfc [3.362925] Creating 5 MTD partitions on "ffe07800.nfc": [3.368188] 0x-0x0020 : "boot" [3.373970] 0x0020-0x0060 : "env" [3.378564] 0x0060-0x0100 : "system" [3.383511] 0x0100-0x0400 : "rootfs" [3.388525] 0x0400-0x0c00 : "media" I am looking forward to a Hynix nand flash to test on GXL platform, and there should be something different from MXIC flash on ONFI page detection. I will update the result asap net week. Do you have another type of nand flash to test on M8 platform ? Thanks, Miquèl .
Re: [RFC PATCH nand-next 0/2] meson-nand: support for older SoCs
Hello Martin, On 2019/3/2 2:29, Martin Blumenstingl wrote: Hi Liang, I am trying to add support for older SoCs to the meson-nand driver. Back when the driver was in development I used an early revision (of your driver) and did some modifications to make it work on older SoCs. Now that the driver is upstream I wanted to give it another try and make a real patch out of it. Unfortunately it's not working anymore. As far as I know the NFC IP block revision on GXL is similar (or even the same?) as on all older SoCs. As far as I can tell only the clock setup is different on the older SoCs (which have a dedicated NAND clock): - we don't need the "amlogic,mmc-syscon" property on the older SoCs because we don't need to setup any muxing (common clock framework will do everything for us) - "rx" and "tx" clocks don't exist - I could not find any other differences between Meson8, Meson8b, Meson8m2, GXBB and GXL That is right. the serials NFC is almost the same except: 1) The clock control and source that M8-serials are not share with EMMC. 2) The base register address 3) DMA encryption option which we don't care on NFC driver. In this series I'm sending two patches which add support for the older SoCs. Unfortunately these patches are currently not working for me (hence the "RFC" prefix). I get a (strange) crash which is triggered by the kzalloc() in meson_nfc_read_buf() - see below for more details. Can you please help me on this one? I'd like to know whether: - the meson-nand driver works for you on GXL or AXG on linux-next? (I was running these patches on top of next-20190301 on my M8S board which uses a 32-bit Meson8m2 SoC. I don't have any board using a GXL SoC which also has NAND) Yes, it works on AXG platform using a MXIC slc nand flash(MX30LF4G); but i an not sure it runs the same flow with yours. because i see the print "Counld not find a valid ONFI parameter page, " in yours. i will try to reproduce it on AXG(i don't have a M8 platform now). - you see any issue with my patches? (maybe I missed more differences between GXL and the older SoCs) i think it is ok now. kernel log extract: [...] Could not find a valid ONFI parameter page, trying bit-wise majority to recover it ONFI parameter recovery failed, aborting Unable to handle kernel paging request at virtual address 8011 pgd = (ptrval) [8011] *pgd= Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.0.0-rc8-next-20190301-00053-g50ac6f7757e2 #4145 Hardware name: Amlogic Meson platform PC is at kmem_cache_alloc_trace+0xc8/0x268 LR is at kmem_cache_alloc_trace+0x2c/0x268 pc : []lr : []psr: 6013 sp : c02adc58 ip : e9e7a440 fp : 4ee2 r10: 8011 r9 : e000 r8 : c110918c r7 : 0008 r6 : c08967c0 r5 : 0dc0 r4 : c0201e40 r3 : c109dd30 r2 : r1 : 4ee2 r0 : Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 0020404a DAC: 0051 Process swapper/0 (pid: 1, stack limit = 0x(ptrval)) Stack: (0xc02adc58 to 0xc02ae000) dc40: e9d84048 0003 dc60: e9e7a440 c02add18 0028 e9e68680 c02adcf0 0003 e9d84048 0002 dc80: e9e7a440 c08967c0 c1108cb4 c02adcdc 10624dd3 c02adce4 10624dd3 0005 dca0: e9e7a440 c0f5c310 0028 c0f09998 0005 e9d84048 0005 c02add57 dcc0: c1108c88 e9e7a440 c02adcf0 e9d84428 e9d843b0 c0882258 00ff 4000 dce0: c02adce8 c02adcf0 0003 0090 dd00: 0001 0001 c02adcdf 0190 0002 0005 dd20: c02add57 0001 a10a1ef7 c1108c88 c1180250 e9d843c0 dd40: 0001 00de c088d114 c0f08db4 00d843c0 a10a1ef7 dd60: 0015 e9d84048 c1108c88 c088d470 e9d84048 c0b4 6013 dd80: c0eebc9c 00ad c0da01ac e9e7a48c c0cc6d50 c12122cc a10a1ef7 dda0: e9e7a440 e9e7a440 e9d84040 c0eebc9c e987f410 eafd6748 e9d84048 c1108c88 ddc0: e9e7a48c c0895c70 e9871f00 e9e7a440 c04eef60 eafd64bc dde0: e9e7a534 0001 a10a1ef7 e987f410 de00: c1180728 c1180728 c1071854 c07fd388 de20: c120da38 e987f410 c120da3c c07fb410 e987f410 c1180728 de40: c1180728 c07fb910 c1071834 c10004a8 c07fb65c c10004a8 c0a917b4 de60: c0da1914 e987f410 c1180728 c07fb910 c1071834 c10004a8 de80: c1071854 c07fb908 c1180728 e987f410 c07fb968 e98b8eb4 c1108c88 dea0: c1180728 c07f97d4 c1175260 c029a958 e98b8eb4 a10a1ef7 c029a96c c1180728 dec0: e9e1fa80 c1175260 c07fa844 c0f09c64 c1108c88 e000 c1180728 dee0: c1108c88 e000 c103b788 c07fc494 c11c2ca0 c1108c88 e000 c0302f1c df00: ebfffd96 c0346f90 c0fab8a0 c0f2ad00 0006 0006 c0e9e26c df20: c1108c88 c0eb11b0 c0e9e2e0 c11d1300 ebfffd84 ebfffd89 a10a1ef7
Re: [PATCH 2/2] mtd: rawnand: meson: fix a potential memory leak in meson_nfc_read_buf
Hello Martin,
Thank you very much.
On 2019/3/2 1:38, Martin Blumenstingl wrote:
meson_nfc_dma_buffer_setup() is called with the "info" buffer which is
allocated a few lines before using kzalloc(). If
meson_nfc_dma_buffer_setup() fails we need to free the allocated "info"
buffer instead of only freeing it upon success.
Fixes: 8fae856c53500a ("mtd: rawnand: meson: add support for Amlogic NAND flash
controller")
Signed-off-by: Martin Blumenstingl
---
drivers/mtd/nand/raw/meson_nand.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index a1d8506b61c7..38db4fd61459 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -534,7 +534,7 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8
*buf, int len)
ret = meson_nfc_dma_buffer_setup(nand, buf, len, info,
PER_INFO_BYTE, DMA_FROM_DEVICE);
if (ret)
- return ret;
+ goto out;
Looks good to me.
Acked-by: Liang Yang
cmd = NFC_CMD_N2M | (len & GENMASK(5, 0));
writel(cmd, nfc->reg_base + NFC_REG_CMD);
@@ -542,6 +542,8 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8
*buf, int len)
meson_nfc_drain_cmd(nfc);
meson_nfc_wait_cmd_finish(nfc, 1000);
meson_nfc_dma_buffer_release(nand, len, PER_INFO_BYTE, DMA_FROM_DEVICE);
+
+out:
Looks good to me.
Acked-by: Liang Yang
kfree(info);
return ret;
Re: [PATCH 1/2] mtd: rawnand: meson: add missing ENOMEM check in meson_nfc_read_buf()
Hello Martin,
On 2019/3/2 1:38, Martin Blumenstingl wrote:
kzalloc() can return NULL if memory could not be allocated. Check the
return value of the kzalloc() call in meson_nfc_read_buf() to make it
consistent with other memory allocations within the meson_nand driver.
Fixes: 8fae856c53500a ("mtd: rawnand: meson: add support for Amlogic NAND flash
controller")
Signed-off-by: Martin Blumenstingl
---
drivers/mtd/nand/raw/meson_nand.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index 3e8aa71407b5..a1d8506b61c7 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -528,6 +528,9 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8
*buf, int len)
u8 *info;
info = kzalloc(PER_INFO_BYTE, GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
Thank you very much. it is really good to me.
Acked-by: Liang Yang
ret = meson_nfc_dma_buffer_setup(nand, buf, len, info,
PER_INFO_BYTE, DMA_FROM_DEVICE);
if (ret)
Re: [PATCH] mtd: rawnand: meson: Fix linking error on 32-bit platforms
Hi Miquel, Nathan,
On 2019/1/30 23:27, Nathan Chancellor wrote:
On Wed, Jan 30, 2019 at 10:32:20AM +0100, Miquel Raynal wrote:
Hi Liang, Nathan,
Liang Yang wrote on Wed, 30 Jan 2019 17:26:39
+0800:
Hi Nathan,
On 2019/1/30 5:46, Nathan Chancellor wrote:
On arm little endian allyesconfig:
ld.lld: error: undefined symbol: __aeabi_uldivmod
>>> referenced by meson_nand.c
>>> mtd/nand/raw/meson_nand.o:(meson_nfc_setup_data_interface) in archive
drivers/built-in.a
The dividend tBERS_max is u64, meaning we need to use DIV_ROUND_UP_ULL
(which wraps do_div) to prevent the compiler from emitting
__aebi_uldivmod.
Fixes: 2d570b34b41a ("mtd: rawnand: meson: add support for Amlogic NAND flash
controller")
Signed-off-by: Nathan Chancellor
---
drivers/mtd/nand/raw/meson_nand.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index e858d58d97b0..6f12a96195d1 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -1116,8 +1116,8 @@ int meson_nfc_setup_data_interface(struct nand_chip
*nand, int csline,
div * NFC_CLK_CYCLE);
meson_chip->tadl = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tADL_min),
div * NFC_CLK_CYCLE);
- tbers_clocks = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tBERS_max),
- div * NFC_CLK_CYCLE);
+ tbers_clocks = DIV_ROUND_UP_ULL(PSEC_TO_NSEC(timings->tBERS_max),
+ div * NFC_CLK_CYCLE);
Looks good to me:
Acked-by: Liang Yang
This is nand/next material, so if you don't mind I would like to squash
this fix into the original commit inserting the driver.
Thanks,
Miquèl
Hi Miquel,
That is totally fine by me, thanks for the quick response!
Nathan
it is ok with me.
.
Re: [PATCH][next] mtd: rawnand: meson: fix missing assignment of ret on a call to meson_chip_buffer_init
Hi Miquel, Colin,
On 2019/1/30 17:34, Colin Ian King wrote:
On 30/01/2019 09:32, Miquel Raynal wrote:
Hi Liang, Colin,
Liang Yang wrote on Wed, 30 Jan 2019 17:26:49
+0800:
Hi Colin,
On 2019/1/29 18:57, Colin King wrote:
From: Colin Ian King
The call to meson_chip_buffer_init is not assigning ret, however, ret
is being checked for failure. Fix this by adding in the missing assignment.
Fixes: 2d570b34b41a ("mtd: rawnand: meson: add support for Amlogic NAND flash
controller")
Signed-off-by: Colin Ian King
---
drivers/mtd/nand/raw/meson_nand.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index e858d58d97b0..b9c543d1054c 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -1206,7 +1206,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
dev_err(nfc->dev, "16bits bus width not supported");
return -EINVAL;
}
- meson_chip_buffer_init(nand);
+ ret = meson_chip_buffer_init(nand);
Looks good to me:
Acked-by: Liang Yang
This is nand/next material, so if you don't mind I would like to squash
the two fixes you sent into the original commit inserting the driver.
Sure, ok with me, squashing them makes sense.
Colin
ok.
Thanks,
Miquèl
.
Re: [PATCH] mtd: rawnand: meson: Fix linking error on 32-bit platforms
Hi Nathan,
On 2019/1/30 5:46, Nathan Chancellor wrote:
On arm little endian allyesconfig:
ld.lld: error: undefined symbol: __aeabi_uldivmod
>>> referenced by meson_nand.c
>>> mtd/nand/raw/meson_nand.o:(meson_nfc_setup_data_interface) in archive
drivers/built-in.a
The dividend tBERS_max is u64, meaning we need to use DIV_ROUND_UP_ULL
(which wraps do_div) to prevent the compiler from emitting
__aebi_uldivmod.
Fixes: 2d570b34b41a ("mtd: rawnand: meson: add support for Amlogic NAND flash
controller")
Signed-off-by: Nathan Chancellor
---
drivers/mtd/nand/raw/meson_nand.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index e858d58d97b0..6f12a96195d1 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -1116,8 +1116,8 @@ int meson_nfc_setup_data_interface(struct nand_chip
*nand, int csline,
div * NFC_CLK_CYCLE);
meson_chip->tadl = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tADL_min),
div * NFC_CLK_CYCLE);
- tbers_clocks = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tBERS_max),
- div * NFC_CLK_CYCLE);
+ tbers_clocks = DIV_ROUND_UP_ULL(PSEC_TO_NSEC(timings->tBERS_max),
+ div * NFC_CLK_CYCLE);
Looks good to me:
Acked-by: Liang Yang
meson_chip->tbers_max = ilog2(tbers_clocks);
if (!is_power_of_2(tbers_clocks))
meson_chip->tbers_max++;
Re: [PATCH][next] mtd: rawnand: meson: fix missing assignment of ret on a call to meson_chip_buffer_init
Hi Colin,
On 2019/1/29 18:57, Colin King wrote:
From: Colin Ian King
The call to meson_chip_buffer_init is not assigning ret, however, ret
is being checked for failure. Fix this by adding in the missing assignment.
Fixes: 2d570b34b41a ("mtd: rawnand: meson: add support for Amlogic NAND flash
controller")
Signed-off-by: Colin Ian King
---
drivers/mtd/nand/raw/meson_nand.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index e858d58d97b0..b9c543d1054c 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -1206,7 +1206,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
dev_err(nfc->dev, "16bits bus width not supported");
return -EINVAL;
}
- meson_chip_buffer_init(nand);
+ ret = meson_chip_buffer_init(nand);
Looks good to me:
Acked-by: Liang Yang
if (ret)
return -ENOMEM;
Re: [PATCH][next] mtd: rawnand: meson:: make several functions static
Hi Colin,
On 2019/1/29 20:44, Colin King wrote:
From: Colin Ian King
There are several functions that are local to the source and do
not need to be in global scope, so make them static.
Cleans up sparse warnings.
Signed-off-by: Colin Ian King
---
drivers/mtd/nand/raw/meson_nand.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index b9c543d1054c..9557bd94dcd2 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -829,14 +829,14 @@ static int meson_nfc_read_oob(struct nand_chip *nand, int
page)
return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
}
-bool meson_nfc_is_buffer_dma_safe(const void *buffer)
+static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
{
if (virt_addr_valid(buffer) && (!object_is_on_stack(buffer)))
return true;
return false;
}
-void *
+static void *
meson_nand_op_get_dma_safe_input_buf(const struct nand_op_instr *instr)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
@@ -848,7 +848,7 @@ meson_nand_op_get_dma_safe_input_buf(const struct
nand_op_instr *instr)
return kzalloc(instr->ctx.data.len, GFP_KERNEL);
}
-void
+static void
meson_nand_op_put_dma_safe_input_buf(const struct nand_op_instr *instr,
void *buf)
{
@@ -863,7 +863,7 @@ meson_nand_op_put_dma_safe_input_buf(const struct
nand_op_instr *instr,
kfree(buf);
}
-void *
+static void *
meson_nand_op_get_dma_safe_output_buf(const struct nand_op_instr *instr)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
@@ -876,7 +876,7 @@ meson_nand_op_get_dma_safe_output_buf(const struct
nand_op_instr *instr)
instr->ctx.data.len, GFP_KERNEL);
}
-void
+static void
meson_nand_op_put_dma_safe_output_buf(const struct nand_op_instr *instr,
const void *buf)
{
Looks good to me:
Acked-by: Liang Yang
Re: [PATCH] mtd: rawnand: meson: Fix linking error on 32-bit platforms
Hello Nathan,
On 2019/1/30 5:46, Nathan Chancellor wrote:
On arm little endian allyesconfig:
ld.lld: error: undefined symbol: __aeabi_uldivmod
>>> referenced by meson_nand.c
>>> mtd/nand/raw/meson_nand.o:(meson_nfc_setup_data_interface) in archive
drivers/built-in.a
The dividend tBERS_max is u64, meaning we need to use DIV_ROUND_UP_ULL
(which wraps do_div) to prevent the compiler from emitting
__aebi_uldivmod.
ok. thanks for your time.
Fixes: 2d570b34b41a ("mtd: rawnand: meson: add support for Amlogic NAND flash
controller")
Signed-off-by: Nathan Chancellor
---
drivers/mtd/nand/raw/meson_nand.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index e858d58d97b0..6f12a96195d1 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -1116,8 +1116,8 @@ int meson_nfc_setup_data_interface(struct nand_chip
*nand, int csline,
div * NFC_CLK_CYCLE);
meson_chip->tadl = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tADL_min),
div * NFC_CLK_CYCLE);
- tbers_clocks = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tBERS_max),
- div * NFC_CLK_CYCLE);
+ tbers_clocks = DIV_ROUND_UP_ULL(PSEC_TO_NSEC(timings->tBERS_max),
+ div * NFC_CLK_CYCLE);
ok.
meson_chip->tbers_max = ilog2(tbers_clocks);
if (!is_power_of_2(tbers_clocks))
meson_chip->tbers_max++;
Re: [PATCH][next] mtd: rawnand: meson: fix missing assignment of ret on a call to meson_chip_buffer_init
Hello Colin,
On 2019/1/29 18:57, Colin King wrote:
From: Colin Ian King
The call to meson_chip_buffer_init is not assigning ret, however, ret
is being checked for failure. Fix this by adding in the missing assignment.
ok. thanks for your time.
Fixes: 2d570b34b41a ("mtd: rawnand: meson: add support for Amlogic NAND flash
controller")
Signed-off-by: Colin Ian King
---
drivers/mtd/nand/raw/meson_nand.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index e858d58d97b0..b9c543d1054c 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -1206,7 +1206,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
dev_err(nfc->dev, "16bits bus width not supported");
return -EINVAL;
}
- meson_chip_buffer_init(nand);
+ ret = meson_chip_buffer_init(nand); >if (ret)
return -ENOMEM;
Re: [PATCH][next] mtd: rawnand: meson:: make several functions static
Hello Colin,
On 2019/1/29 20:44, Colin King wrote:
From: Colin Ian King
There are several functions that are local to the source and do
not need to be in global scope, so make them static.
Cleans up sparse warnings.
ok. thanks
Signed-off-by: Colin Ian King
---
drivers/mtd/nand/raw/meson_nand.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c
b/drivers/mtd/nand/raw/meson_nand.c
index b9c543d1054c..9557bd94dcd2 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -829,14 +829,14 @@ static int meson_nfc_read_oob(struct nand_chip *nand, int
page)
return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
}
-bool meson_nfc_is_buffer_dma_safe(const void *buffer)
+static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
{
if (virt_addr_valid(buffer) && (!object_is_on_stack(buffer)))
return true;
return false;
}
-void *
+static void *
meson_nand_op_get_dma_safe_input_buf(const struct nand_op_instr *instr)
{
if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
@@ -848,7 +848,7 @@ meson_nand_op_get_dma_safe_input_buf(const struct
nand_op_instr *instr)
return kzalloc(instr->ctx.data.len, GFP_KERNEL);
}
-void
+static void
meson_nand_op_put_dma_safe_input_buf(const struct nand_op_instr *instr,
void *buf)
{
@@ -863,7 +863,7 @@ meson_nand_op_put_dma_safe_input_buf(const struct
nand_op_instr *instr,
kfree(buf);
}
-void *
+static void *
meson_nand_op_get_dma_safe_output_buf(const struct nand_op_instr *instr)
{
if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
@@ -876,7 +876,7 @@ meson_nand_op_get_dma_safe_output_buf(const struct
nand_op_instr *instr)
instr->ctx.data.len, GFP_KERNEL);
}
-void
+static void
meson_nand_op_put_dma_safe_output_buf(const struct nand_op_instr *instr,
const void *buf)
{
Re: [PATCH] MAINTAINERS: Add entry for Amlogic NAND controller driver
Hi Miquel， On 2019/1/21 16:21, Miquel Raynal wrote: Hi Jianxin, Jianxin Pan wrote on Mon, 21 Jan 2019 12:18:18 +0800: Hi Miquel, On 2019/1/20 23:06, Miquel Raynal wrote: Hi Jianxin, Jianxin Pan wrote on Sun, 20 Jan 2019 01:02:35 +0800: Add entry for Amlogic NAND controller driver and its bindings[0]. [0] https://lore.kernel.org/lkml/1547566684-57472-1-git-send-email-jianxin@amlogic.com/ Signed-off-by: Liang Yang Signed-off-by: Jianxin Pan If you are the author of the patch your Signed-off-by should come first. OK. Also, why is Liang the Maintainer? Why not you? Yangliang is Amlogic maintainer for NAND controller. And all patches about amlogic NAND controller are from him. Maybe but he never wrote to the ML, the maintainer may be someone else than the original author, I just want someone to review patches and help to maintain the driver. Yangliang are you willing to do that? Of course.it is my pleasure and responsibility. Thanks, Miquèl .
Re: [PATCH RESEND v8 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Martin,
On 2018/12/23 1:07, Martin Blumenstingl wrote:
Hi Jianxin, Hi Liang,
On Fri, Dec 21, 2018 at 12:45 PM Jianxin Pan wrote:
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 | 1468 +
3 files changed, 1479 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 1a55d3e..d05ff20 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
I believe that "depends on COMMON_CLK_AMLOGIC" and "select
COMMON_CLK_REGMAP_MESON" are not necessary:
the driver should build fine without them because it's only
interfacing with the common clock framework.
the common clock framework is enabled by ARCH_MESON and for the
COMPILE_TEST case the common clock framework provides stub
implementations inside the headers.
+ select MFD_SYSCON
+ help
+ Enables support for NAND controller on Amlogic's Meson SoCs.
+ This controller is found on Meson GXBB, GXL, AXG SoCs.
you are explicitly mentioning GXBB here but you don't add a "GXBB" compatible.
I suggest to shorten this sentence ("This controller is found on Meson
SoCs.") because this driver can also support the 32-bit
Meson8/Meson8b/Meson8m2 SoCs with minor adjustments.
we only have tested on Meson GXL and AXG platform, but it should support
GXBB and Meson8/Meson8b/Meson8m2 and the differences between these
controllers are only the base address of register and some SD_EMMC_CLOCK
control bits.
i think "This controller is found on Meson SoCs." is ok.
Regards
Martin
.
Re: [PATCH v7 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Miquel, On 2018/12/11 17:07, Miquel Raynal wrote: Hi Liang, Liang Yang wrote on Tue, 11 Dec 2018 16:36:47 +0800: Hi Miquel, Thanks for your quickly reply. On 2018/12/11 15:54, Miquel Raynal wrote: Hi Liang, Liang Yang wrote on Tue, 11 Dec 2018 09:56:25 +0800: Hi Miquel, On 2018/12/10 22:50, Miquel Raynal wrote: Hi Liang, Liang Yang wrote on Mon, 10 Dec 2018 20:12:39 +0800: >>>> On 2018/12/10 19:38, Boris Brezillon wrote: On Mon, 10 Dec 2018 19:23:46 +0800 Liang Yang wrote: >>>>>> + mtd->ecc_stats.failed++; + continue; + } + mtd->ecc_stats.corrected += ECC_ERR_CNT(*info); + bitflips = max_t(u32, bitflips, ECC_ERR_CNT(*info)); + } Are you sure you handle correctly empty pages with bf? >> if scramble is enable, i would say yes here. when scramble is disabled, i am considering how to use the helper nand_check_erased_ecc_chunk, but it seems that i can't get the ecc bytes which is caculated by ecc engine.by the way, nfc dma doesn't send out the ecc parity bytes. Even if the ECC engine is disabled? >> No. When ECC engine is disabled, it can read the ecc parity bytes ; but there is another problem that i need to consider how code struct looks better when reading error with ecc opened and then try to raw read. Is there a good idea? When reading with ECC enabled, in case of uncorrectable error you must re-read without ECC, then check if the page is empty or not with the core helpers (nand_check_erased_*()). Is this what you meant? >> yes. when uncorrectable ECC error, i need firstly read out the ECC bytes without ECC engine and then use the helper nand_check_erased_ecc_chunk to check if blank page. Of course, the precondition is without scrambler, or the bland page can be detected by meson NFC. A suppose you meant "blank page"? If yes, then you don't need the helper to check for only-0xFF pages. If the controller tells you if the page was blank, then just check for that bit. i think not. we need to return back the previous problem that how i can get the bitflips of one blank page. i think i need the helper. You are right, I suppose the "blank page" flag is only triggered if there is no bitflip. In this case you can assume there are no bitflips. Otherwise the controller will trigger an uncorrectable error event and you will have to re-read the page without ECC and check for bitflips with the helper. yes, that is right. i will do the "re-read" process. Thanks, Miquèl .
Re: [PATCH v7 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris, On 2018/12/11 16:39, Boris Brezillon wrote: On Tue, 11 Dec 2018 09:56:25 +0800 Liang Yang wrote: Hi Miquel, On 2018/12/10 22:50, Miquel Raynal wrote: Hi Liang, Liang Yang wrote on Mon, 10 Dec 2018 20:12:39 +0800: On 2018/12/10 19:38, Boris Brezillon wrote: On Mon, 10 Dec 2018 19:23:46 +0800 Liang Yang wrote: + mtd->ecc_stats.failed++; + continue; + } + mtd->ecc_stats.corrected += ECC_ERR_CNT(*info); + bitflips = max_t(u32, bitflips, ECC_ERR_CNT(*info)); + } Are you sure you handle correctly empty pages with bf? >> if scramble is enable, i would say yes here. when scramble is disabled, i am considering how to use the helper nand_check_erased_ecc_chunk, but it seems that i can't get the ecc bytes which is caculated by ecc engine.by the way, nfc dma doesn't send out the ecc parity bytes. Even if the ECC engine is disabled? No. When ECC engine is disabled, it can read the ecc parity bytes ; but there is another problem that i need to consider how code struct looks better when reading error with ecc opened and then try to raw read. Is there a good idea? When reading with ECC enabled, in case of uncorrectable error you must re-read without ECC, then check if the page is empty or not with the core helpers (nand_check_erased_*()). Is this what you meant? yes. when uncorrectable ECC error, i need firstly read out the ECC bytes without ECC engine and then use the helper nand_check_erased_ecc_chunk to check if blank page. Of course, the precondition is without scrambler, or the bland page can be detected by meson NFC. Yep, raw accessors should disable both the scrambler and the ECC engine (see what's done in sunxi_nand.c). i see sunxi_nfc_hw_ecc_read_chunk and it will re-read the data for bitflips with scrambler off when ECC failed. also we can do the same implementation and it seems to be the only answer. .
Re: [PATCH v7 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Miquel, Thanks for your quickly reply. On 2018/12/11 15:54, Miquel Raynal wrote: Hi Liang, Liang Yang wrote on Tue, 11 Dec 2018 09:56:25 +0800: Hi Miquel, On 2018/12/10 22:50, Miquel Raynal wrote: Hi Liang, Liang Yang wrote on Mon, 10 Dec 2018 20:12:39 +0800: On 2018/12/10 19:38, Boris Brezillon wrote: On Mon, 10 Dec 2018 19:23:46 +0800 Liang Yang wrote: >>>>>> + mtd->ecc_stats.failed++; + continue; + } + mtd->ecc_stats.corrected += ECC_ERR_CNT(*info); + bitflips = max_t(u32, bitflips, ECC_ERR_CNT(*info)); + } Are you sure you handle correctly empty pages with bf? >> if scramble is enable, i would say yes here. when scramble is disabled, i am considering how to use the helper nand_check_erased_ecc_chunk, but it seems that i can't get the ecc bytes which is caculated by ecc engine.by the way, nfc dma doesn't send out the ecc parity bytes. Even if the ECC engine is disabled? >> No. When ECC engine is disabled, it can read the ecc parity bytes ; but there is another problem that i need to consider how code struct looks better when reading error with ecc opened and then try to raw read. Is there a good idea? When reading with ECC enabled, in case of uncorrectable error you must re-read without ECC, then check if the page is empty or not with the core helpers (nand_check_erased_*()). Is this what you meant? yes. when uncorrectable ECC error, i need firstly read out the ECC bytes without ECC engine and then use the helper nand_check_erased_ecc_chunk to check if blank page. Of course, the precondition is without scrambler, or the bland page can be detected by meson NFC. A suppose you meant "blank page"? If yes, then you don't need the helper to check for only-0xFF pages. If the controller tells you if the page was blank, then just check for that bit. i think not. we need to return back the previous problem that how i can get the bitflips of one blank page. i think i need the helper. Thanks, Miquèl .
Re: [PATCH v7 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Miquel, On 2018/12/10 22:50, Miquel Raynal wrote: Hi Liang, Liang Yang wrote on Mon, 10 Dec 2018 20:12:39 +0800: On 2018/12/10 19:38, Boris Brezillon wrote: On Mon, 10 Dec 2018 19:23:46 +0800 Liang Yang wrote: + mtd->ecc_stats.failed++; + continue; + } + mtd->ecc_stats.corrected += ECC_ERR_CNT(*info); + bitflips = max_t(u32, bitflips, ECC_ERR_CNT(*info)); + } Are you sure you handle correctly empty pages with bf? >> if scramble is enable, i would say yes here. when scramble is disabled, i am considering how to use the helper nand_check_erased_ecc_chunk, but it seems that i can't get the ecc bytes which is caculated by ecc engine.by the way, nfc dma doesn't send out the ecc parity bytes. Even if the ECC engine is disabled? No. When ECC engine is disabled, it can read the ecc parity bytes ; but there is another problem that i need to consider how code struct looks better when reading error with ecc opened and then try to raw read. Is there a good idea? When reading with ECC enabled, in case of uncorrectable error you must re-read without ECC, then check if the page is empty or not with the core helpers (nand_check_erased_*()). Is this what you meant? yes. when uncorrectable ECC error, i need firstly read out the ECC bytes without ECC engine and then use the helper nand_check_erased_ecc_chunk to check if blank page. Of course, the precondition is without scrambler, or the bland page can be detected by meson NFC. Thanks, Miquèl .
Re: [PATCH v7 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/12/10 19:38, Boris Brezillon wrote:
On Mon, 10 Dec 2018 19:23:46 +0800
Liang Yang wrote:
+ mtd->ecc_stats.failed++;
+ continue;
+ }
+ mtd->ecc_stats.corrected += ECC_ERR_CNT(*info);
+ bitflips = max_t(u32, bitflips, ECC_ERR_CNT(*info));
+ }
Are you sure you handle correctly empty pages with bf?
if scramble is enable, i would say yes here.
when scramble is disabled, i am considering how to use the helper
nand_check_erased_ecc_chunk, but it seems that i can't get the ecc
bytes which is caculated by ecc engine.by the way, nfc dma doesn't send
out the ecc parity bytes.
Even if the ECC engine is disabled?
No.
When ECC engine is disabled, it can read the ecc parity bytes ; but
there is another problem that i need to consider how code struct looks
better when reading error with ecc opened and then try to raw read.
Is there a good idea?
so i would suggest using scramble.
No, please don't force people to use the scrambler.
+
+const void *
+meson_nand_op_get_dma_safe_output_buf(const struct nand_op_instr *instr)
+{
+ if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
+ return NULL;
+
+ if (virt_addr_valid(instr->ctx.data.buf.out) &&
+ !object_is_on_stack(instr->ctx.data.buf.out))
Can you please create helpers for that? I guess it will help removing
these checks once the core will have a DMA-safe approach.
I will use below definition:
#define BUFFER_IS_DMA_SAFE(x) \
(virt_addr_valid((x)) && (!object_is_on_stack((x
Is it ok?
Please define a function, not a macro.
ok
.
Re: [PATCH v7 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/12/7 17:24, Miquel Raynal wrote: Hi Jianxin, Looks good to me overall, a few comments inline. Jianxin Pan wrote on Sat, 17 Nov 2018 00:40:38 +0800: 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 | 1417 + 3 files changed, 1428 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..c566636 --- /dev/null +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -0,0 +1,1417 @@ +// 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_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)) +
Re: [PATCH v7 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Miquel, Appreciate your time. I will follow the nand/next and rework some next week as soon as possible. On 2018/12/7 17:42, Miquel Raynal wrote: Hi Jianxin, Miquel Raynal wrote on Fri, 7 Dec 2018 10:24:56 +0100: Hi Jianxin, Looks good to me overall, a few comments inline. Jianxin Pan wrote on Sat, 17 Nov 2018 00:40:38 +0800: 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 | 1417 + 3 files changed, 1428 insertions(+) create mode 100644 drivers/mtd/nand/raw/meson_nand.c I forgot to mention, Boris has done more cleanup which breaks your patches, please have a look at the following commits in the nand/next branch, they will force you to do some light rework to get the driver building (especially, you should not export the ->select_chip hook anymore): 7a08dbaedd36 mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops f2abfeb2078b mtd: rawnand: Move the ->exec_op() method to nand_controller_ops 7d6c37e90cf9 mtd: rawnand: Deprecate the ->select_chip() hook 1770022ffa85 mtd: rawnand: ams-delta: Stop implementing ->select_chip() 653c57c7da08 mtd: rawnand: vf610: Stop implementing ->select_chip() 2ace451cae22 mtd: rawnand: tegra: Stop implementing ->select_chip() b25251414f6e mtd: rawnand: marvell: Stop implementing ->select_chip() 550b9fc4e3af mtd: rawnand: fsmc: Stop implementing ->select_chip() 02b4a52604a4 mtd: rawnand: Make ->select_chip() optional when ->exec_op() is implemented ae2294b10b0f mtd: rawnand: Pass the CS line to be selected in struct nand_operation 1d0178593d14 mtd: rawnand: Add nand_[de]select_target() helpers Thanks, Miquèl .
Re: [PATCH v7 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Miquel, Appreciate your time. I will follow the nand/next and rework some next week as soon as possible. On 2018/12/7 17:42, Miquel Raynal wrote: Hi Jianxin, Miquel Raynal wrote on Fri, 7 Dec 2018 10:24:56 +0100: Hi Jianxin, Looks good to me overall, a few comments inline. Jianxin Pan wrote on Sat, 17 Nov 2018 00:40:38 +0800: 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 | 1417 + 3 files changed, 1428 insertions(+) create mode 100644 drivers/mtd/nand/raw/meson_nand.c I forgot to mention, Boris has done more cleanup which breaks your patches, please have a look at the following commits in the nand/next branch, they will force you to do some light rework to get the driver building (especially, you should not export the ->select_chip hook anymore): 7a08dbaedd36 mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops f2abfeb2078b mtd: rawnand: Move the ->exec_op() method to nand_controller_ops 7d6c37e90cf9 mtd: rawnand: Deprecate the ->select_chip() hook 1770022ffa85 mtd: rawnand: ams-delta: Stop implementing ->select_chip() 653c57c7da08 mtd: rawnand: vf610: Stop implementing ->select_chip() 2ace451cae22 mtd: rawnand: tegra: Stop implementing ->select_chip() b25251414f6e mtd: rawnand: marvell: Stop implementing ->select_chip() 550b9fc4e3af mtd: rawnand: fsmc: Stop implementing ->select_chip() 02b4a52604a4 mtd: rawnand: Make ->select_chip() optional when ->exec_op() is implemented ae2294b10b0f mtd: rawnand: Pass the CS line to be selected in struct nand_operation 1d0178593d14 mtd: rawnand: Add nand_[de]select_target() helpers Thanks, Miquèl .
Re: [PATCH v7 0/2] mtd: rawnand: meson: add Amlogic NAND driver support
Hi Boris and Miquel, How about the v7 patch? On 2018/11/17 0:40, Jianxin Pan wrote: These two patches try to add initial NAND driver support for Amlogic Meson SoCs, current it has been tested on GXL(p212) and AXG(s400) platform. Changes since V6 at [7] - use timings->tBERS_max as the maximum time out, delete NFC_CMD_RB_TIMEOUT - fix nand_rw_cmd and support small block flash and which row address less than 3 - fix coding style - replace readl/writel_* with readl/writel_relaxed* - delete ECC_SET_PROTECTED_OOB_BYTE and ECC_GET_PROTECTED_OOB_BYTE - implement dma access for read_buf and write_buf, more efficient. - delete waiting dma finish in write process and let NAND_CMD_PAGEPROG and RB command go on queuing - add waiting the completed flag of last ecc page be set, for more strict
Re: [PATCH v7 0/2] mtd: rawnand: meson: add Amlogic NAND driver support
Hi Boris and Miquel, How about the v7 patch? On 2018/11/17 0:40, Jianxin Pan wrote: These two patches try to add initial NAND driver support for Amlogic Meson SoCs, current it has been tested on GXL(p212) and AXG(s400) platform. Changes since V6 at [7] - use timings->tBERS_max as the maximum time out, delete NFC_CMD_RB_TIMEOUT - fix nand_rw_cmd and support small block flash and which row address less than 3 - fix coding style - replace readl/writel_* with readl/writel_relaxed* - delete ECC_SET_PROTECTED_OOB_BYTE and ECC_GET_PROTECTED_OOB_BYTE - implement dma access for read_buf and write_buf, more efficient. - delete waiting dma finish in write process and let NAND_CMD_PAGEPROG and RB command go on queuing - add waiting the completed flag of last ecc page be set, for more strict
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
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
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
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 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 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,
+ nan
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,
+ nan
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 v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/19 16:42, Boris Brezillon wrote:
On Fri, 19 Oct 2018 16:29:40 +0800
Liang Yang wrote:
On 2018/10/19 4:50, Boris Brezillon wrote:
On Thu, 18 Oct 2018 13:09:05 +0800
Jianxin Pan wrote:
+static int meson_nfc_buffer_init(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ static int max_page_bytes, max_info_bytes;
+ int page_bytes, info_bytes;
+ int nsectors;
+
+ nsectors = mtd->writesize / nand->ecc.size;
+ page_bytes = mtd->writesize + mtd->oobsize;
+ info_bytes = nsectors * PER_INFO_BYTE;
+
+ if (nfc->data_buf && nfc->info_buf) {
+ if (max_page_bytes < page_bytes)
+ meson_nfc_free_buffer(nfc);
+ else
+ return 0;
+ }
+
+ max_page_bytes = max_t(int, max_page_bytes, page_bytes);
+ max_info_bytes = max_t(int, max_info_bytes, info_bytes);
+
+ nfc->data_buf = kmalloc(max_page_bytes, GFP_KERNEL);
Is there a good reason for not using chip->data_buf and allocating a
new buffer here?
when calling read_oob or write_oob, i need a mid-buffer which is used in
meson_nfc_prase_data_oob(). i.e. after reading a page data into
nfc->data_buf, I will format it and transfer to chip->data_buf.
+ if (!nfc->data_buf)
+ return -ENOMEM;
+
+ nfc->info_buf = kmalloc(max_info_bytes, GFP_KERNEL);
+ if (!nfc->info_buf) {
+ kfree(nfc->data_buf);
+ return -ENOMEM;
+ }
I'd recommend moving this info_buf in the priv chip struct, otherwise
you'll have to protect nfc->info_buf with a lock to prevent an already
register chip from using this pointer while you're reallocating the
buffer. Also, I think you have a memleak here.
ok, i will move it in the chip struct .
but how memleak happens even i have called meson_nfc_free_buffer before
and free data_buf if info_buf alloc faied.
You're right, I missed the call to meson_nfc_free_buffer() when
max_page_bytes < page_bytes. Still, this part is racy, just like the
nfc->data_buf replacement is if you have several NAND chips. I'd
recommend moving those bufs to the priv chip struct.
ok. i will move data_duf and info_buf to priv chip struct.
+
+ return 0;
+}
.
.
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/19 16:42, Boris Brezillon wrote:
On Fri, 19 Oct 2018 16:29:40 +0800
Liang Yang wrote:
On 2018/10/19 4:50, Boris Brezillon wrote:
On Thu, 18 Oct 2018 13:09:05 +0800
Jianxin Pan wrote:
+static int meson_nfc_buffer_init(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ static int max_page_bytes, max_info_bytes;
+ int page_bytes, info_bytes;
+ int nsectors;
+
+ nsectors = mtd->writesize / nand->ecc.size;
+ page_bytes = mtd->writesize + mtd->oobsize;
+ info_bytes = nsectors * PER_INFO_BYTE;
+
+ if (nfc->data_buf && nfc->info_buf) {
+ if (max_page_bytes < page_bytes)
+ meson_nfc_free_buffer(nfc);
+ else
+ return 0;
+ }
+
+ max_page_bytes = max_t(int, max_page_bytes, page_bytes);
+ max_info_bytes = max_t(int, max_info_bytes, info_bytes);
+
+ nfc->data_buf = kmalloc(max_page_bytes, GFP_KERNEL);
Is there a good reason for not using chip->data_buf and allocating a
new buffer here?
when calling read_oob or write_oob, i need a mid-buffer which is used in
meson_nfc_prase_data_oob(). i.e. after reading a page data into
nfc->data_buf, I will format it and transfer to chip->data_buf.
+ if (!nfc->data_buf)
+ return -ENOMEM;
+
+ nfc->info_buf = kmalloc(max_info_bytes, GFP_KERNEL);
+ if (!nfc->info_buf) {
+ kfree(nfc->data_buf);
+ return -ENOMEM;
+ }
I'd recommend moving this info_buf in the priv chip struct, otherwise
you'll have to protect nfc->info_buf with a lock to prevent an already
register chip from using this pointer while you're reallocating the
buffer. Also, I think you have a memleak here.
ok, i will move it in the chip struct .
but how memleak happens even i have called meson_nfc_free_buffer before
and free data_buf if info_buf alloc faied.
You're right, I missed the call to meson_nfc_free_buffer() when
max_page_bytes < page_bytes. Still, this part is racy, just like the
nfc->data_buf replacement is if you have several NAND chips. I'd
recommend moving those bufs to the priv chip struct.
ok. i will move data_duf and info_buf to priv chip struct.
+
+ return 0;
+}
.
.
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/19 16:10, Boris Brezillon wrote:
On Fri, 19 Oct 2018 15:29:05 +0800
Liang Yang wrote:
How about defining that the HW returns an array of __le64 instead and then
define the following macros which you can use after converting in the
CPU endianness
#define ECC_GET_PROTECTED_OOB_BYTE(x, y)(((x) >> (8 * (1 + y)) &
GENMASK(7, 0))
#define ECC_COMPLETEBIT(31)
#define ECC_ERR_CNT(x) (((x) >> 24) & GENMASK(5, 0))
(I'm not entirely sure the field positions are correct, but I'll let you
check that).
ok. i think it should be:
#define ECC_GET_PROTECTED_OOB_BYTE(x, y)(((x) >> (8 * y) &
GENMASK(7, 0))
if x represents the u64 and y represents the index of the u64.
Absolutely.
+
+#define PER_INFO_BYTE (sizeof(struct meson_nfc_info_format))
+
+struct meson_nfc_nand_chip {
+ struct list_head node;
+ struct nand_chip nand;
+ bool is_scramble;
I think I already mentioned the NAND_NEED_SCRAMBLING flag []. Please
drop this field and test (chip->flags & NAND_NEED_SCRAMBLING) instead.
em, i use NAND_NEED_SCRAMBLING and is_scramble is set:
static int meson_nand_attach_chip(struct nand_chip *nand)
{
..
meson_chip->is_scramble =
(nand->options & NAND_NEED_SCRAMBLING) ? 1 : 0;
..
}
Why do you need to add a new field then? Just test
nand->options & NAND_NEED_SCRAMBLING directly or provide a helper
function that does that.
ok, i will fix it.
.
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/19 16:10, Boris Brezillon wrote:
On Fri, 19 Oct 2018 15:29:05 +0800
Liang Yang wrote:
How about defining that the HW returns an array of __le64 instead and then
define the following macros which you can use after converting in the
CPU endianness
#define ECC_GET_PROTECTED_OOB_BYTE(x, y)(((x) >> (8 * (1 + y)) &
GENMASK(7, 0))
#define ECC_COMPLETEBIT(31)
#define ECC_ERR_CNT(x) (((x) >> 24) & GENMASK(5, 0))
(I'm not entirely sure the field positions are correct, but I'll let you
check that).
ok. i think it should be:
#define ECC_GET_PROTECTED_OOB_BYTE(x, y)(((x) >> (8 * y) &
GENMASK(7, 0))
if x represents the u64 and y represents the index of the u64.
Absolutely.
+
+#define PER_INFO_BYTE (sizeof(struct meson_nfc_info_format))
+
+struct meson_nfc_nand_chip {
+ struct list_head node;
+ struct nand_chip nand;
+ bool is_scramble;
I think I already mentioned the NAND_NEED_SCRAMBLING flag []. Please
drop this field and test (chip->flags & NAND_NEED_SCRAMBLING) instead.
em, i use NAND_NEED_SCRAMBLING and is_scramble is set:
static int meson_nand_attach_chip(struct nand_chip *nand)
{
..
meson_chip->is_scramble =
(nand->options & NAND_NEED_SCRAMBLING) ? 1 : 0;
..
}
Why do you need to add a new field then? Just test
nand->options & NAND_NEED_SCRAMBLING directly or provide a helper
function that does that.
ok, i will fix it.
.
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/19 4:50, Boris Brezillon wrote:
On Thu, 18 Oct 2018 13:09:05 +0800
Jianxin Pan wrote:
+static int meson_nfc_buffer_init(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ static int max_page_bytes, max_info_bytes;
+ int page_bytes, info_bytes;
+ int nsectors;
+
+ nsectors = mtd->writesize / nand->ecc.size;
+ page_bytes = mtd->writesize + mtd->oobsize;
+ info_bytes = nsectors * PER_INFO_BYTE;
+
+ if (nfc->data_buf && nfc->info_buf) {
+ if (max_page_bytes < page_bytes)
+ meson_nfc_free_buffer(nfc);
+ else
+ return 0;
+ }
+
+ max_page_bytes = max_t(int, max_page_bytes, page_bytes);
+ max_info_bytes = max_t(int, max_info_bytes, info_bytes);
+
+ nfc->data_buf = kmalloc(max_page_bytes, GFP_KERNEL);
Is there a good reason for not using chip->data_buf and allocating a
new buffer here?
when calling read_oob or write_oob, i need a mid-buffer which is used in
meson_nfc_prase_data_oob(). i.e. after reading a page data into
nfc->data_buf, I will format it and transfer to chip->data_buf.
+ if (!nfc->data_buf)
+ return -ENOMEM;
+
+ nfc->info_buf = kmalloc(max_info_bytes, GFP_KERNEL);
+ if (!nfc->info_buf) {
+ kfree(nfc->data_buf);
+ return -ENOMEM;
+ }
I'd recommend moving this info_buf in the priv chip struct, otherwise
you'll have to protect nfc->info_buf with a lock to prevent an already
register chip from using this pointer while you're reallocating the
buffer. Also, I think you have a memleak here.
ok, i will move it in the chip struct .
but how memleak happens even i have called meson_nfc_free_buffer before
and free data_buf if info_buf alloc faied.
+
+ return 0;
+}
.
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/19 4:50, Boris Brezillon wrote:
On Thu, 18 Oct 2018 13:09:05 +0800
Jianxin Pan wrote:
+static int meson_nfc_buffer_init(struct mtd_info *mtd)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ static int max_page_bytes, max_info_bytes;
+ int page_bytes, info_bytes;
+ int nsectors;
+
+ nsectors = mtd->writesize / nand->ecc.size;
+ page_bytes = mtd->writesize + mtd->oobsize;
+ info_bytes = nsectors * PER_INFO_BYTE;
+
+ if (nfc->data_buf && nfc->info_buf) {
+ if (max_page_bytes < page_bytes)
+ meson_nfc_free_buffer(nfc);
+ else
+ return 0;
+ }
+
+ max_page_bytes = max_t(int, max_page_bytes, page_bytes);
+ max_info_bytes = max_t(int, max_info_bytes, info_bytes);
+
+ nfc->data_buf = kmalloc(max_page_bytes, GFP_KERNEL);
Is there a good reason for not using chip->data_buf and allocating a
new buffer here?
when calling read_oob or write_oob, i need a mid-buffer which is used in
meson_nfc_prase_data_oob(). i.e. after reading a page data into
nfc->data_buf, I will format it and transfer to chip->data_buf.
+ if (!nfc->data_buf)
+ return -ENOMEM;
+
+ nfc->info_buf = kmalloc(max_info_bytes, GFP_KERNEL);
+ if (!nfc->info_buf) {
+ kfree(nfc->data_buf);
+ return -ENOMEM;
+ }
I'd recommend moving this info_buf in the priv chip struct, otherwise
you'll have to protect nfc->info_buf with a lock to prevent an already
register chip from using this pointer while you're reallocating the
buffer. Also, I think you have a memleak here.
ok, i will move it in the chip struct .
but how memleak happens even i have called meson_nfc_free_buffer before
and free data_buf if info_buf alloc faied.
+
+ return 0;
+}
.
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/19 4:39, Boris Brezillon wrote:
On Thu, 18 Oct 2018 13:09:05 +0800
Jianxin Pan wrote:
+static int meson_nfc_calc_set_timing(struct meson_nfc *nfc,
+const struct nand_sdr_timings *timings)
+{
+ struct nand_timing *timing = >timing;
+ int div, bt_min, bt_max, bus_timing;
+ int ret;
+
+ div = DIV_ROUND_UP((timings->tRC_min / 1000), NFC_CLK_CYCLE);
+ ret = clk_set_rate(nfc->device_clk, 10 / div);
+ if (ret) {
+ dev_err(nfc->dev, "failed to set nand clock rate\n");
+ return ret;
+ }
+
+ timing->twb = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tWB_max),
+ div * NFC_CLK_CYCLE);
+ timing->tadl = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tADL_min),
+ div * NFC_CLK_CYCLE);
+ timing->twhr = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tWHR_min),
+ div * NFC_CLK_CYCLE);
+
+ bt_min = (timings->tREA_max + NFC_DEFAULT_DELAY) / div;
+ bt_max = (NFC_DEFAULT_DELAY + timings->tRHOH_min
+ + timings->tRC_min / 2) / div;
+
+ bt_min = DIV_ROUND_UP(bt_min, 1000);
+ bt_max = DIV_ROUND_UP(bt_max, 1000);
+
+ if (bt_max < bt_min)
+ return -EINVAL;
+
+ bus_timing = (bt_min + bt_max) / 2 + 1;
+
+ writel((1 << 21), nfc->reg_base + NFC_REG_CFG);
+ writel((NFC_CLK_CYCLE - 1) | (bus_timing << 5),
+ nfc->reg_base + NFC_REG_CFG);
+
+ writel((1 << 31), nfc->reg_base + NFC_REG_CMD);
+
+ return 0;
+}
+
+static int
+meson_nfc_setup_data_interface(struct nand_chip *nand, int csline,
+ const struct nand_data_interface *conf)
+{
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ const struct nand_sdr_timings *timings;
+
+ timings = nand_get_sdr_timings(conf);
+ if (IS_ERR(timings))
+ return -ENOTSUPP;
+
+ if (csline == NAND_DATA_IFACE_CHECK_ONLY)
+ return 0;
Hm, before saying you supporting the requested timing, you should make
sure they are actually supported. I'd recommend splitting this in 2
steps:
1/ calc timings
2/ store the timings in the chip priv struct so that they can be
applied next time ->select_chip() is called.
ok, i will try to split.
+
+ meson_nfc_calc_set_timing(nfc, timings);
> You should not set the timing from ->setup_data_interface(), just
calculate them, make sure they are supported and store the state in the
private chip struct. Applying those timings should be done in
->select_chip(), so that you can support 2 chips with different timings.
em, i will fix it.
+ return 0;
+}
.
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/19 4:39, Boris Brezillon wrote:
On Thu, 18 Oct 2018 13:09:05 +0800
Jianxin Pan wrote:
+static int meson_nfc_calc_set_timing(struct meson_nfc *nfc,
+const struct nand_sdr_timings *timings)
+{
+ struct nand_timing *timing = >timing;
+ int div, bt_min, bt_max, bus_timing;
+ int ret;
+
+ div = DIV_ROUND_UP((timings->tRC_min / 1000), NFC_CLK_CYCLE);
+ ret = clk_set_rate(nfc->device_clk, 10 / div);
+ if (ret) {
+ dev_err(nfc->dev, "failed to set nand clock rate\n");
+ return ret;
+ }
+
+ timing->twb = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tWB_max),
+ div * NFC_CLK_CYCLE);
+ timing->tadl = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tADL_min),
+ div * NFC_CLK_CYCLE);
+ timing->twhr = DIV_ROUND_UP(PSEC_TO_NSEC(timings->tWHR_min),
+ div * NFC_CLK_CYCLE);
+
+ bt_min = (timings->tREA_max + NFC_DEFAULT_DELAY) / div;
+ bt_max = (NFC_DEFAULT_DELAY + timings->tRHOH_min
+ + timings->tRC_min / 2) / div;
+
+ bt_min = DIV_ROUND_UP(bt_min, 1000);
+ bt_max = DIV_ROUND_UP(bt_max, 1000);
+
+ if (bt_max < bt_min)
+ return -EINVAL;
+
+ bus_timing = (bt_min + bt_max) / 2 + 1;
+
+ writel((1 << 21), nfc->reg_base + NFC_REG_CFG);
+ writel((NFC_CLK_CYCLE - 1) | (bus_timing << 5),
+ nfc->reg_base + NFC_REG_CFG);
+
+ writel((1 << 31), nfc->reg_base + NFC_REG_CMD);
+
+ return 0;
+}
+
+static int
+meson_nfc_setup_data_interface(struct nand_chip *nand, int csline,
+ const struct nand_data_interface *conf)
+{
+ struct meson_nfc *nfc = nand_get_controller_data(nand);
+ const struct nand_sdr_timings *timings;
+
+ timings = nand_get_sdr_timings(conf);
+ if (IS_ERR(timings))
+ return -ENOTSUPP;
+
+ if (csline == NAND_DATA_IFACE_CHECK_ONLY)
+ return 0;
Hm, before saying you supporting the requested timing, you should make
sure they are actually supported. I'd recommend splitting this in 2
steps:
1/ calc timings
2/ store the timings in the chip priv struct so that they can be
applied next time ->select_chip() is called.
ok, i will try to split.
+
+ meson_nfc_calc_set_timing(nfc, timings);
> You should not set the timing from ->setup_data_interface(), just
calculate them, make sure they are supported and store the state in the
private chip struct. Applying those timings should be done in
->select_chip(), so that you can support 2 chips with different timings.
em, i will fix it.
+ return 0;
+}
.
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/19 3:33, Boris Brezillon wrote: On Thu, 18 Oct 2018 13:09:05 +0800 Jianxin Pan wrote: 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 | 1370 + 3 files changed, 1381 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..bcaac53 --- /dev/null +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -0,0 +1,1370 @@ +// 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_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)
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/19 3:33, Boris Brezillon wrote: On Thu, 18 Oct 2018 13:09:05 +0800 Jianxin Pan wrote: 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 | 1370 + 3 files changed, 1381 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..bcaac53 --- /dev/null +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -0,0 +1,1370 @@ +// 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_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)
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/18 22:24, Boris Brezillon wrote:
On Thu, 18 Oct 2018 13:09:05 +0800
Jianxin Pan wrote:
+static int meson_nfc_exec_op(struct nand_chip *chip,
+const struct nand_operation *op, bool check_only)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct meson_nfc *nfc = nand_get_controller_data(chip);
+ const struct nand_op_instr *instr = NULL;
+ int ret = 0, cmd;
+ unsigned int op_id;
+ int i;
+
+ for (op_id = 0; op_id < op->ninstrs; op_id++) {
+ instr = >instrs[op_id];
+ switch (instr->type) {
+ case NAND_OP_CMD_INSTR:
+ if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
+ meson_nfc_cmd_idle(nfc, nfc->timing.twb);
Hm, I don't want drivers to base their decisions on the opcode value.
There's a ->delay_ns field in the instruction object, can't you use
that one instead? Also, I don't understand why this is only needed for
the STATUS command. It should normally be applied to all instructions.
em, it should be applied to all instructions.
i will fix it and use instr->delay_ns instead.
+ cmd = nfc->param.chip_select | NFC_CMD_CLE;
+ cmd |= instr->ctx.cmd.opcode & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
+ meson_nfc_cmd_idle(nfc, nfc->timing.twhr);
+ break;
+
+ case NAND_OP_ADDR_INSTR:
+ for (i = 0; i < instr->ctx.addr.naddrs; i++) {
+ cmd = nfc->param.chip_select | NFC_CMD_ALE;
+ cmd |= instr->ctx.addr.addrs[i] & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ }
+ break;
+
+ case NAND_OP_DATA_IN_INSTR:
+ meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_DATA_OUT_INSTR:
+ meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_WAITRDY_INSTR:
+ meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms);
+ break;
+ }
+ }
+ return ret;
+}
.
Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 2018/10/18 22:24, Boris Brezillon wrote:
On Thu, 18 Oct 2018 13:09:05 +0800
Jianxin Pan wrote:
+static int meson_nfc_exec_op(struct nand_chip *chip,
+const struct nand_operation *op, bool check_only)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct meson_nfc *nfc = nand_get_controller_data(chip);
+ const struct nand_op_instr *instr = NULL;
+ int ret = 0, cmd;
+ unsigned int op_id;
+ int i;
+
+ for (op_id = 0; op_id < op->ninstrs; op_id++) {
+ instr = >instrs[op_id];
+ switch (instr->type) {
+ case NAND_OP_CMD_INSTR:
+ if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
+ meson_nfc_cmd_idle(nfc, nfc->timing.twb);
Hm, I don't want drivers to base their decisions on the opcode value.
There's a ->delay_ns field in the instruction object, can't you use
that one instead? Also, I don't understand why this is only needed for
the STATUS command. It should normally be applied to all instructions.
em, it should be applied to all instructions.
i will fix it and use instr->delay_ns instead.
+ cmd = nfc->param.chip_select | NFC_CMD_CLE;
+ cmd |= instr->ctx.cmd.opcode & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
+ meson_nfc_cmd_idle(nfc, nfc->timing.twhr);
+ break;
+
+ case NAND_OP_ADDR_INSTR:
+ for (i = 0; i < instr->ctx.addr.naddrs; i++) {
+ cmd = nfc->param.chip_select | NFC_CMD_ALE;
+ cmd |= instr->ctx.addr.addrs[i] & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ }
+ break;
+
+ case NAND_OP_DATA_IN_INSTR:
+ meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_DATA_OUT_INSTR:
+ meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_WAITRDY_INSTR:
+ meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms);
+ break;
+ }
+ }
+ return ret;
+}
.
Re: [PATCH v4 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver
On 9/28/2018 2:16 AM, Rob Herring wrote:
On Thu, Sep 20, 2018 at 04:50:48PM +0800, Jianxin Pan wrote:
From: Liang Yang
Add Amlogic NAND controller dt-bindings for Meson SoC,
Current this driver support GXBB/GXL/AXG platform.
Signed-off-by: Liang Yang
Signed-off-by: Yixun Lan
---
.../devicetree/bindings/mtd/amlogic,meson-nand.txt | 60 ++
1 file changed, 60 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
new file mode 100644
index 000..803df2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
@@ -0,0 +1,60 @@
+Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
+
+This file documents the properties in addition to those available in
+the MTD NAND bindings.
+
+Required properties:
+- compatible : contains one of:
+ - "amlogic,meson-gxl-nfc"
+ - "amlogic,meson-axg-nfc"
+- clocks :
+ A list of phandle + clock-specifier pairs for the clocks listed
+ in clock-names.
+
+- clock-names: Should contain the following:
+ "core" - NFC module gate clock
+ "device" - device clock from eMMC sub clock controller
+
+- amlogic,mmc-syscon : Required for NAND clocks, it's shared with SD/eMMC
+ controller port C
+
+Optional children nodes:
+Children nodes represent the available nand chips.
+
+Other properties:
+see Documentation/devicetree/bindings/mtd/nand.txt for generic bindings.
+
+Example demonstrate on AXG SoC:
+
+ sd_emmc_c_clkc: mmc@7000 {
+ compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
+ reg = <0x0 0x7000 0x0 0x800>;
+ status = "okay";
+ };
+
+ nand: nfc@7800 {
nand-controller@7800
ok, i will fix it.
+ compatible = "amlogic,meson-axg-nfc";
+ reg = <0x0 0x7800 0x0 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = ;
+ status = "disabled";
+
+ clocks = < CLKID_SD_EMMC_C>,
+ <_emmc_c_clkc CLKID_MMC_DIV>;
+ clock-names = "core", "device";
+ amlogic,mmc-syscon = <_emmc_c_clkc>;
+
+ status = "okay";
Don't show status in examples, plus you have it twice.
ok, i will fix it.
+
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+
+ nand@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ nand-on-flash-bbt;
+ };
+ };
--
1.9.1
.
Re: [PATCH v4 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver
On 9/28/2018 2:16 AM, Rob Herring wrote:
On Thu, Sep 20, 2018 at 04:50:48PM +0800, Jianxin Pan wrote:
From: Liang Yang
Add Amlogic NAND controller dt-bindings for Meson SoC,
Current this driver support GXBB/GXL/AXG platform.
Signed-off-by: Liang Yang
Signed-off-by: Yixun Lan
---
.../devicetree/bindings/mtd/amlogic,meson-nand.txt | 60 ++
1 file changed, 60 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
new file mode 100644
index 000..803df2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
@@ -0,0 +1,60 @@
+Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
+
+This file documents the properties in addition to those available in
+the MTD NAND bindings.
+
+Required properties:
+- compatible : contains one of:
+ - "amlogic,meson-gxl-nfc"
+ - "amlogic,meson-axg-nfc"
+- clocks :
+ A list of phandle + clock-specifier pairs for the clocks listed
+ in clock-names.
+
+- clock-names: Should contain the following:
+ "core" - NFC module gate clock
+ "device" - device clock from eMMC sub clock controller
+
+- amlogic,mmc-syscon : Required for NAND clocks, it's shared with SD/eMMC
+ controller port C
+
+Optional children nodes:
+Children nodes represent the available nand chips.
+
+Other properties:
+see Documentation/devicetree/bindings/mtd/nand.txt for generic bindings.
+
+Example demonstrate on AXG SoC:
+
+ sd_emmc_c_clkc: mmc@7000 {
+ compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
+ reg = <0x0 0x7000 0x0 0x800>;
+ status = "okay";
+ };
+
+ nand: nfc@7800 {
nand-controller@7800
ok, i will fix it.
+ compatible = "amlogic,meson-axg-nfc";
+ reg = <0x0 0x7800 0x0 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = ;
+ status = "disabled";
+
+ clocks = < CLKID_SD_EMMC_C>,
+ <_emmc_c_clkc CLKID_MMC_DIV>;
+ clock-names = "core", "device";
+ amlogic,mmc-syscon = <_emmc_c_clkc>;
+
+ status = "okay";
Don't show status in examples, plus you have it twice.
ok, i will fix it.
+
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+
+ nand@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ nand-on-flash-bbt;
+ };
+ };
--
1.9.1
.
Re: [PATCH v4 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 9/27/2018 5:12 PM, Martin Blumenstingl wrote:
Hello Liang,
On Thu, Sep 27, 2018 at 10:19 AM Liang Yang wrote:
Hello Martin,
On 9/22/2018 11:32 PM, Martin Blumenstingl wrote:
Hello,
On Thu, Sep 20, 2018 at 10:51 AM Jianxin Pan wrote:
[snip]
+static int meson_nfc_clk_init(struct meson_nfc *nfc)
+{
+ int ret;
+
+ /* request core clock */
+ nfc->core_clk = devm_clk_get(nfc->dev, "core");
+ if (IS_ERR(nfc->core_clk)) {
+ dev_err(nfc->dev, "failed to get core clk\n");
+ return PTR_ERR(nfc->core_clk);
+ }
+
+ nfc->device_clk = devm_clk_get(nfc->dev, "device");
+ if (IS_ERR(nfc->device_clk)) {
+ dev_err(nfc->dev, "failed to get device clk\n");
+ return PTR_ERR(nfc->device_clk);
+ }
+
+ nfc->phase_tx = devm_clk_get(nfc->dev, "tx");
+ if (IS_ERR(nfc->phase_tx)) {
+ dev_err(nfc->dev, "failed to get tx clk\n");
+ return PTR_ERR(nfc->phase_tx);
+ }
+
+ nfc->phase_rx = devm_clk_get(nfc->dev, "rx");
+ if (IS_ERR(nfc->phase_rx)) {
+ dev_err(nfc->dev, "failed to get rx clk\n");
+ return PTR_ERR(nfc->phase_rx);
+ }
neither the "rx" nor the "tx" clock are documented in the dt-bindings patch
ok, i will add them later.
+ /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
+ regmap_update_bits(nfc->reg_clk, 0,
+ CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
+ CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
clk_set_rate also works for clocks that are not enabled yet (except if
they have the flag CLK_SET_RATE_UNGATE)
this should help you to remove CLK_DIV_MASK here
if not set clk_div_mask here, the value 0x00 of divider means nand clock
off, even read/write nand register is forbidden.
ah, now I see the pattern here.
Jerome has written a "sclk-div" driver (which is currently only used
for the audio clocks on AXG). based on reading the code it seems that
switching the driver of the divider clock to sclk-div would allow you
to remove setting CLK_DIV_MASK here:
- the "hi" parameter in struct meson_sclk_div_data is optional ->
then the sclk-div clock won't try to change the duty cycle
- sclk_div_init reads the divider at initialization time - if it's 0
it takes the maximum possible divider value
- sclk_div_enable (which you're going to call anyways, through
clk_prepare_enable) will then set the divider to the greatest possible
value
I read the code and it makes sense.
I try ro add mmc_clkc_regmap_divider_ops in clk-regmap.c and implement
the initial value when enable call. like this:
+static int mmc_clkc_regmap_div_enable(struct clk_hw *hw)
+{
+ struct clk_regmap *clk = to_clk_regmap(hw);
+ struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
+ unsigned int val;
+
+ regmap_read(clk->map, div->offset, );
+ val &= clk_div_mask(div->width);
+ if (!val)
+ regmap_update_bits(clk->map, div->offset,
+ clk_div_mask(div->width) << div->shift,
+ clk_div_mask(div->width));
+ return 0;
+}
it works.
is CLK_SELECT_NAND a bit that switches the clock output from the sdmmc
controller to the NAND controller?
if so: can this be modeled as a mux clock?
it seems to like a gate. 1: nand is selected; 0: emmc is selected.
Is it suitable for making it as a mux clock?
my understanding of a gate is:
- register value X = OFF, value Y = ON
but in your case it's:
- 0 = eMMC is ON but NAND is OFF
- 1 = eMMC is OFF but NAND is ON
(so both values mean "on", just for different contexts)
I believe you need to set this value for eMMC as well:
what if the bootloader (or hardware defaults, etc.) incorrectly sets
the value to 1 but the Linux .dts is configured to use eMMC?
en , we need set 0 for emmc as well.
the public S905 datasheet doesn't mention CLK_ALWAYS_ON at bit 28 but
uses bit 24 instead. the description from the datasheet:
Cfg_always_on:
1: Keep clock always on
0: Clock on/off controlled by activities.
Any APB3 access or descriptor execution will turn clock on.
Recommended value: 0
can you please explain what CLK_ALWAYS_ON does and why it has to be 1?
em , it is the same as bit 24 in S905 datasheet, only moves to bit 28.
it means keeping internal clock on whether nand wroks or not.
it doesn't have to be 1; i have tried 0 successfully on AXG platform.
my preference would be to use the recommended value from the datasheet
unless there's a good argument why it has to be different
indeed, i will adopt the recommended value.
Regards
Martin
.
Re: [PATCH v4 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 9/27/2018 5:12 PM, Martin Blumenstingl wrote:
Hello Liang,
On Thu, Sep 27, 2018 at 10:19 AM Liang Yang wrote:
Hello Martin,
On 9/22/2018 11:32 PM, Martin Blumenstingl wrote:
Hello,
On Thu, Sep 20, 2018 at 10:51 AM Jianxin Pan wrote:
[snip]
+static int meson_nfc_clk_init(struct meson_nfc *nfc)
+{
+ int ret;
+
+ /* request core clock */
+ nfc->core_clk = devm_clk_get(nfc->dev, "core");
+ if (IS_ERR(nfc->core_clk)) {
+ dev_err(nfc->dev, "failed to get core clk\n");
+ return PTR_ERR(nfc->core_clk);
+ }
+
+ nfc->device_clk = devm_clk_get(nfc->dev, "device");
+ if (IS_ERR(nfc->device_clk)) {
+ dev_err(nfc->dev, "failed to get device clk\n");
+ return PTR_ERR(nfc->device_clk);
+ }
+
+ nfc->phase_tx = devm_clk_get(nfc->dev, "tx");
+ if (IS_ERR(nfc->phase_tx)) {
+ dev_err(nfc->dev, "failed to get tx clk\n");
+ return PTR_ERR(nfc->phase_tx);
+ }
+
+ nfc->phase_rx = devm_clk_get(nfc->dev, "rx");
+ if (IS_ERR(nfc->phase_rx)) {
+ dev_err(nfc->dev, "failed to get rx clk\n");
+ return PTR_ERR(nfc->phase_rx);
+ }
neither the "rx" nor the "tx" clock are documented in the dt-bindings patch
ok, i will add them later.
+ /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
+ regmap_update_bits(nfc->reg_clk, 0,
+ CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
+ CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
clk_set_rate also works for clocks that are not enabled yet (except if
they have the flag CLK_SET_RATE_UNGATE)
this should help you to remove CLK_DIV_MASK here
if not set clk_div_mask here, the value 0x00 of divider means nand clock
off, even read/write nand register is forbidden.
ah, now I see the pattern here.
Jerome has written a "sclk-div" driver (which is currently only used
for the audio clocks on AXG). based on reading the code it seems that
switching the driver of the divider clock to sclk-div would allow you
to remove setting CLK_DIV_MASK here:
- the "hi" parameter in struct meson_sclk_div_data is optional ->
then the sclk-div clock won't try to change the duty cycle
- sclk_div_init reads the divider at initialization time - if it's 0
it takes the maximum possible divider value
- sclk_div_enable (which you're going to call anyways, through
clk_prepare_enable) will then set the divider to the greatest possible
value
I read the code and it makes sense.
I try ro add mmc_clkc_regmap_divider_ops in clk-regmap.c and implement
the initial value when enable call. like this:
+static int mmc_clkc_regmap_div_enable(struct clk_hw *hw)
+{
+ struct clk_regmap *clk = to_clk_regmap(hw);
+ struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
+ unsigned int val;
+
+ regmap_read(clk->map, div->offset, );
+ val &= clk_div_mask(div->width);
+ if (!val)
+ regmap_update_bits(clk->map, div->offset,
+ clk_div_mask(div->width) << div->shift,
+ clk_div_mask(div->width));
+ return 0;
+}
it works.
is CLK_SELECT_NAND a bit that switches the clock output from the sdmmc
controller to the NAND controller?
if so: can this be modeled as a mux clock?
it seems to like a gate. 1: nand is selected; 0: emmc is selected.
Is it suitable for making it as a mux clock?
my understanding of a gate is:
- register value X = OFF, value Y = ON
but in your case it's:
- 0 = eMMC is ON but NAND is OFF
- 1 = eMMC is OFF but NAND is ON
(so both values mean "on", just for different contexts)
I believe you need to set this value for eMMC as well:
what if the bootloader (or hardware defaults, etc.) incorrectly sets
the value to 1 but the Linux .dts is configured to use eMMC?
en , we need set 0 for emmc as well.
the public S905 datasheet doesn't mention CLK_ALWAYS_ON at bit 28 but
uses bit 24 instead. the description from the datasheet:
Cfg_always_on:
1: Keep clock always on
0: Clock on/off controlled by activities.
Any APB3 access or descriptor execution will turn clock on.
Recommended value: 0
can you please explain what CLK_ALWAYS_ON does and why it has to be 1?
em , it is the same as bit 24 in S905 datasheet, only moves to bit 28.
it means keeping internal clock on whether nand wroks or not.
it doesn't have to be 1; i have tried 0 successfully on AXG platform.
my preference would be to use the recommended value from the datasheet
unless there's a good argument why it has to be different
indeed, i will adopt the recommended value.
Regards
Martin
.
Re: [PATCH v4 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hello Martin,
On 9/22/2018 11:32 PM, Martin Blumenstingl wrote:
Hello,
On Thu, Sep 20, 2018 at 10:51 AM Jianxin Pan wrote:
[snip]
+static int meson_nfc_clk_init(struct meson_nfc *nfc)
+{
+ int ret;
+
+ /* request core clock */
+ nfc->core_clk = devm_clk_get(nfc->dev, "core");
+ if (IS_ERR(nfc->core_clk)) {
+ dev_err(nfc->dev, "failed to get core clk\n");
+ return PTR_ERR(nfc->core_clk);
+ }
+
+ nfc->device_clk = devm_clk_get(nfc->dev, "device");
+ if (IS_ERR(nfc->device_clk)) {
+ dev_err(nfc->dev, "failed to get device clk\n");
+ return PTR_ERR(nfc->device_clk);
+ }
+
+ nfc->phase_tx = devm_clk_get(nfc->dev, "tx");
+ if (IS_ERR(nfc->phase_tx)) {
+ dev_err(nfc->dev, "failed to get tx clk\n");
+ return PTR_ERR(nfc->phase_tx);
+ }
+
+ nfc->phase_rx = devm_clk_get(nfc->dev, "rx");
+ if (IS_ERR(nfc->phase_rx)) {
+ dev_err(nfc->dev, "failed to get rx clk\n");
+ return PTR_ERR(nfc->phase_rx);
+ }
neither the "rx" nor the "tx" clock are documented in the dt-bindings patch
ok, i will add them later.
+ /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
+ regmap_update_bits(nfc->reg_clk, 0,
+ CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
+ CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
clk_set_rate also works for clocks that are not enabled yet (except if
they have the flag CLK_SET_RATE_UNGATE)
this should help you to remove CLK_DIV_MASK here
if not set clk_div_mask here, the value 0x00 of divider means nand clock
off, even read/write nand register is forbidden.
is CLK_SELECT_NAND a bit that switches the clock output from the sdmmc
controller to the NAND controller?
if so: can this be modeled as a mux clock?
it seems to like a gate. 1: nand is selected; 0: emmc is selected.
Is it suitable for making it as a mux clock?
the public S905 datasheet doesn't mention CLK_ALWAYS_ON at bit 28 but
uses bit 24 instead. the description from the datasheet:
Cfg_always_on:
1: Keep clock always on
0: Clock on/off controlled by activities.
Any APB3 access or descriptor execution will turn clock on.
Recommended value: 0
can you please explain what CLK_ALWAYS_ON does and why it has to be 1?
em , it is the same as bit 24 in S905 datasheet, only moves to bit 28.
it means keeping internal clock on whether nand wroks or not.
it doesn't have to be 1; i have tried 0 successfully on AXG platform.
Regards
Martin
.
Re: [PATCH v4 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hello Martin,
On 9/22/2018 11:32 PM, Martin Blumenstingl wrote:
Hello,
On Thu, Sep 20, 2018 at 10:51 AM Jianxin Pan wrote:
[snip]
+static int meson_nfc_clk_init(struct meson_nfc *nfc)
+{
+ int ret;
+
+ /* request core clock */
+ nfc->core_clk = devm_clk_get(nfc->dev, "core");
+ if (IS_ERR(nfc->core_clk)) {
+ dev_err(nfc->dev, "failed to get core clk\n");
+ return PTR_ERR(nfc->core_clk);
+ }
+
+ nfc->device_clk = devm_clk_get(nfc->dev, "device");
+ if (IS_ERR(nfc->device_clk)) {
+ dev_err(nfc->dev, "failed to get device clk\n");
+ return PTR_ERR(nfc->device_clk);
+ }
+
+ nfc->phase_tx = devm_clk_get(nfc->dev, "tx");
+ if (IS_ERR(nfc->phase_tx)) {
+ dev_err(nfc->dev, "failed to get tx clk\n");
+ return PTR_ERR(nfc->phase_tx);
+ }
+
+ nfc->phase_rx = devm_clk_get(nfc->dev, "rx");
+ if (IS_ERR(nfc->phase_rx)) {
+ dev_err(nfc->dev, "failed to get rx clk\n");
+ return PTR_ERR(nfc->phase_rx);
+ }
neither the "rx" nor the "tx" clock are documented in the dt-bindings patch
ok, i will add them later.
+ /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
+ regmap_update_bits(nfc->reg_clk, 0,
+ CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
+ CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
clk_set_rate also works for clocks that are not enabled yet (except if
they have the flag CLK_SET_RATE_UNGATE)
this should help you to remove CLK_DIV_MASK here
if not set clk_div_mask here, the value 0x00 of divider means nand clock
off, even read/write nand register is forbidden.
is CLK_SELECT_NAND a bit that switches the clock output from the sdmmc
controller to the NAND controller?
if so: can this be modeled as a mux clock?
it seems to like a gate. 1: nand is selected; 0: emmc is selected.
Is it suitable for making it as a mux clock?
the public S905 datasheet doesn't mention CLK_ALWAYS_ON at bit 28 but
uses bit 24 instead. the description from the datasheet:
Cfg_always_on:
1: Keep clock always on
0: Clock on/off controlled by activities.
Any APB3 access or descriptor execution will turn clock on.
Recommended value: 0
can you please explain what CLK_ALWAYS_ON does and why it has to be 1?
em , it is the same as bit 24 in S905 datasheet, only moves to bit 28.
it means keeping internal clock on whether nand wroks or not.
it doesn't have to be 1; i have tried 0 successfully on AXG platform.
Regards
Martin
.
Re: [RFC PATCH v3 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver
Hi boric,
Thanks for your quick reply.
On 9/7/2018 8:19 PM, Boris Brezillon wrote:
On Fri, 7 Sep 2018 18:57:10 +0800
Jianxin Pan wrote:
From: Liang Yang
Add Amlogic NAND controller dt-bindings for Meson SoC,
Current this driver support GXBB/GXL/AXG platform.
Signed-off-by: Liang Yang
Signed-off-by: Yixun Lan
---
.../devicetree/bindings/mtd/amlogic,meson-nand.txt | 91 ++
1 file changed, 91 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
new file mode 100644
index 000..655a778
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
@@ -0,0 +1,91 @@
+Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
+
+This file documents the properties in addition to those available in
+the MTD NAND bindings.
+
+Required properties:
+- compatible : contains one of:
+ - "amlogic,meson-gxl-nfc"
+ - "amlogic,meson-axg-nfc"
+- clocks :
+ A list of phandle + clock-specifier pairs for the clocks listed
+ in clock-names.
+
+- clock-names: Should contain the following:
+ "core" - NFC module gate clock
+ "device" - device clock from eMMC sub clock controller
+
+- pins : Select pins which NFC need.
+- nand_pins: Detail NAND pins information.
You mean pinctrl-names and pinctrl-0, right? Not sure it's necessary to
document that, but if you do, please use the correct DT prop names.
I find no documentation for that in other xx_nand.txt; I will consider
to remove it.
+- amlogic,mmc-syscon : Required for NAND clocks, it's shared with SD/eMMC
+ controller port C
+
+Optional children nodes:
+Children nodes represent the available nand chips.
+
+
+
One too many blank lines here.
ok, i will remove it.
+Other properties:
+see Documentation/devicetree/bindings/mtd/nand.txt for generic bindings.
+
+Example demonstrate on AXG SoC:
+
+ sd_emmc_c_clkc: mmc@7000 {
+ compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
+ reg = <0x0 0x7000 0x0 0x800>;
+ status = "okay";
+ };
+
+ nand: nfc@7800 {
+ compatible = "amlogic,meson-axg-nfc";
+ reg = <0x0 0x7800 0x0 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = ;
+ status = "disabled";
+
+ clocks = < CLKID_SD_EMMC_C>,
+ <_emmc_c_clkc CLKID_MMC_DIV>;
+ clock-names = "core", "device";
+ amlogic,mmc-syscon = <_emmc_c_clkc>;
+
+ status = "okay";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+
+ nand@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ nand-on-flash-bbt;
+ nand-ecc-mode = "hw";
+ nand-ecc-strength = <8>;
+ nand-ecc-step-size = <1024>;
Drop nand-ecc- props. I guess you have a sensible default value and I
prefer when ECC requirements are directly extracted during chip
detection. Defining that in the DT is a bad habit. The only one that
could make sense (assuming you support it) is nand-ecc-maximize.
ok, i will drop them.
we adopt auto detection during init stage, it works too.
+
+ amlogic,nand-enable-scrambler;
Please drop this property (it's not longer documented).
em, we should have removed it when nfc driver never use it.
+
+ partition@0 {
+ label = "boot";
+ reg = <0x 0x0020>;
+ read-only;
+ };
+ partition@20 {
+ label = "env";
+ reg = <0x0020 0x0040>;
+ };
+ partition@60 {
+ label = "system";
+ reg = <0x0060 0x00a0>;
+ };
+ partition@100 {
+ label = "rootfs";
+ reg = <0x0100 0x0300>;
+ };
+ partition@400 {
+ label = "media";
+ reg = <0x0400 0x800>;
+ };
No need to define the partitions in your example, especially since they
should be placed in a partitions subnode with a "fixed-partitions"
compat.
ok, i will remove it.
+ };
+ };
.
Re: [RFC PATCH v3 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver
Hi boric,
Thanks for your quick reply.
On 9/7/2018 8:19 PM, Boris Brezillon wrote:
On Fri, 7 Sep 2018 18:57:10 +0800
Jianxin Pan wrote:
From: Liang Yang
Add Amlogic NAND controller dt-bindings for Meson SoC,
Current this driver support GXBB/GXL/AXG platform.
Signed-off-by: Liang Yang
Signed-off-by: Yixun Lan
---
.../devicetree/bindings/mtd/amlogic,meson-nand.txt | 91 ++
1 file changed, 91 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
new file mode 100644
index 000..655a778
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
@@ -0,0 +1,91 @@
+Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
+
+This file documents the properties in addition to those available in
+the MTD NAND bindings.
+
+Required properties:
+- compatible : contains one of:
+ - "amlogic,meson-gxl-nfc"
+ - "amlogic,meson-axg-nfc"
+- clocks :
+ A list of phandle + clock-specifier pairs for the clocks listed
+ in clock-names.
+
+- clock-names: Should contain the following:
+ "core" - NFC module gate clock
+ "device" - device clock from eMMC sub clock controller
+
+- pins : Select pins which NFC need.
+- nand_pins: Detail NAND pins information.
You mean pinctrl-names and pinctrl-0, right? Not sure it's necessary to
document that, but if you do, please use the correct DT prop names.
I find no documentation for that in other xx_nand.txt; I will consider
to remove it.
+- amlogic,mmc-syscon : Required for NAND clocks, it's shared with SD/eMMC
+ controller port C
+
+Optional children nodes:
+Children nodes represent the available nand chips.
+
+
+
One too many blank lines here.
ok, i will remove it.
+Other properties:
+see Documentation/devicetree/bindings/mtd/nand.txt for generic bindings.
+
+Example demonstrate on AXG SoC:
+
+ sd_emmc_c_clkc: mmc@7000 {
+ compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
+ reg = <0x0 0x7000 0x0 0x800>;
+ status = "okay";
+ };
+
+ nand: nfc@7800 {
+ compatible = "amlogic,meson-axg-nfc";
+ reg = <0x0 0x7800 0x0 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = ;
+ status = "disabled";
+
+ clocks = < CLKID_SD_EMMC_C>,
+ <_emmc_c_clkc CLKID_MMC_DIV>;
+ clock-names = "core", "device";
+ amlogic,mmc-syscon = <_emmc_c_clkc>;
+
+ status = "okay";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+
+ nand@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ nand-on-flash-bbt;
+ nand-ecc-mode = "hw";
+ nand-ecc-strength = <8>;
+ nand-ecc-step-size = <1024>;
Drop nand-ecc- props. I guess you have a sensible default value and I
prefer when ECC requirements are directly extracted during chip
detection. Defining that in the DT is a bad habit. The only one that
could make sense (assuming you support it) is nand-ecc-maximize.
ok, i will drop them.
we adopt auto detection during init stage, it works too.
+
+ amlogic,nand-enable-scrambler;
Please drop this property (it's not longer documented).
em, we should have removed it when nfc driver never use it.
+
+ partition@0 {
+ label = "boot";
+ reg = <0x 0x0020>;
+ read-only;
+ };
+ partition@20 {
+ label = "env";
+ reg = <0x0020 0x0040>;
+ };
+ partition@60 {
+ label = "system";
+ reg = <0x0060 0x00a0>;
+ };
+ partition@100 {
+ label = "rootfs";
+ reg = <0x0100 0x0300>;
+ };
+ partition@400 {
+ label = "media";
+ reg = <0x0400 0x800>;
+ };
No need to define the partitions in your example, especially since they
should be placed in a partitions subnode with a "fixed-partitions"
compat.
ok, i will remove it.
+ };
+ };
.
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 8/29/2018 6:08 PM, Liang Yang wrote: On 8/28/2018 9:26 PM, Boris Brezillon wrote: On Tue, 28 Aug 2018 21:21:48 +0800 Liang Yang wrote: Hi Boris, On 8/24/2018 8:48 PM, Boris Brezillon wrote: On Wed, 22 Aug 2018 22:08:42 +0800 Liang Yang wrote: You have to wait tWB, that's for sure. we have a maximum 32 commands fifo. when command is written into NFC_REG_CMD, it doesn't mean that command is executing right now, maybe it is buffering on the queue.Assume one ERASE operation, when 2nd command(0xd0) is written into NFC_REG_CMD and then come into NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be wrong because 0xd0 may not being executed. it is unusual unless buffering two many command. You should flush the queue and wait for it to empty at the end of ->exec_op(). so it seems that i still need to use nand_soft_waitrdy or wait cmd is executed somewhere. Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also, NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's probably what you should use for tWB. em, I can wait for RB by reading the status from register now. but when calling nand_soft_waitrdy, i really met a problem. One *jiffies* is about 4ms. When programming, it pass 1ms to instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one *jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at the tail of 4ms interval, it may only wait 100us and next jiffies arrive. Is it correct? Hm, no. If you initialize the time you compare to (using time_before() or time_after()) correctly it should not happen. Anyway, I keep thinking this is not how it should be done. Did you try NFC_CMD_RB? Did you ask HW designers what it was created for? I am using NFC_CMD_RB and checking with irq. it is ok now. there are two usages for NFC_CMD_RB. One reads the data status continuously by hardware after sending 0x70 command; the other checks the r/b IO status continuously.both can send irq when r/b is ready. .
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 8/29/2018 6:08 PM, Liang Yang wrote: On 8/28/2018 9:26 PM, Boris Brezillon wrote: On Tue, 28 Aug 2018 21:21:48 +0800 Liang Yang wrote: Hi Boris, On 8/24/2018 8:48 PM, Boris Brezillon wrote: On Wed, 22 Aug 2018 22:08:42 +0800 Liang Yang wrote: You have to wait tWB, that's for sure. we have a maximum 32 commands fifo. when command is written into NFC_REG_CMD, it doesn't mean that command is executing right now, maybe it is buffering on the queue.Assume one ERASE operation, when 2nd command(0xd0) is written into NFC_REG_CMD and then come into NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be wrong because 0xd0 may not being executed. it is unusual unless buffering two many command. You should flush the queue and wait for it to empty at the end of ->exec_op(). so it seems that i still need to use nand_soft_waitrdy or wait cmd is executed somewhere. Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also, NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's probably what you should use for tWB. em, I can wait for RB by reading the status from register now. but when calling nand_soft_waitrdy, i really met a problem. One *jiffies* is about 4ms. When programming, it pass 1ms to instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one *jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at the tail of 4ms interval, it may only wait 100us and next jiffies arrive. Is it correct? Hm, no. If you initialize the time you compare to (using time_before() or time_after()) correctly it should not happen. Anyway, I keep thinking this is not how it should be done. Did you try NFC_CMD_RB? Did you ask HW designers what it was created for? I am using NFC_CMD_RB and checking with irq. it is ok now. there are two usages for NFC_CMD_RB. One reads the data status continuously by hardware after sending 0x70 command; the other checks the r/b IO status continuously.both can send irq when r/b is ready. .
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 8/28/2018 9:26 PM, Boris Brezillon wrote: On Tue, 28 Aug 2018 21:21:48 +0800 Liang Yang wrote: Hi Boris, On 8/24/2018 8:48 PM, Boris Brezillon wrote: On Wed, 22 Aug 2018 22:08:42 +0800 Liang Yang wrote: You have to wait tWB, that's for sure. we have a maximum 32 commands fifo. when command is written into NFC_REG_CMD, it doesn't mean that command is executing right now, maybe it is buffering on the queue.Assume one ERASE operation, when 2nd command(0xd0) is written into NFC_REG_CMD and then come into NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be wrong because 0xd0 may not being executed. it is unusual unless buffering two many command. You should flush the queue and wait for it to empty at the end of ->exec_op(). so it seems that i still need to use nand_soft_waitrdy or wait cmd is executed somewhere. Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also, NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's probably what you should use for tWB. em, I can wait for RB by reading the status from register now. but when calling nand_soft_waitrdy, i really met a problem. One *jiffies* is about 4ms. When programming, it pass 1ms to instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one *jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at the tail of 4ms interval, it may only wait 100us and next jiffies arrive. Is it correct? Hm, no. If you initialize the time you compare to (using time_before() or time_after()) correctly it should not happen. Anyway, I keep thinking this is not how it should be done. Did you try NFC_CMD_RB? Did you ask HW designers what it was created for? I am using NFC_CMD_RB and checking with irq. it is ok now. .
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 8/28/2018 9:26 PM, Boris Brezillon wrote: On Tue, 28 Aug 2018 21:21:48 +0800 Liang Yang wrote: Hi Boris, On 8/24/2018 8:48 PM, Boris Brezillon wrote: On Wed, 22 Aug 2018 22:08:42 +0800 Liang Yang wrote: You have to wait tWB, that's for sure. we have a maximum 32 commands fifo. when command is written into NFC_REG_CMD, it doesn't mean that command is executing right now, maybe it is buffering on the queue.Assume one ERASE operation, when 2nd command(0xd0) is written into NFC_REG_CMD and then come into NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be wrong because 0xd0 may not being executed. it is unusual unless buffering two many command. You should flush the queue and wait for it to empty at the end of ->exec_op(). so it seems that i still need to use nand_soft_waitrdy or wait cmd is executed somewhere. Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also, NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's probably what you should use for tWB. em, I can wait for RB by reading the status from register now. but when calling nand_soft_waitrdy, i really met a problem. One *jiffies* is about 4ms. When programming, it pass 1ms to instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one *jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at the tail of 4ms interval, it may only wait 100us and next jiffies arrive. Is it correct? Hm, no. If you initialize the time you compare to (using time_before() or time_after()) correctly it should not happen. Anyway, I keep thinking this is not how it should be done. Did you try NFC_CMD_RB? Did you ask HW designers what it was created for? I am using NFC_CMD_RB and checking with irq. it is ok now. .
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris, On 8/24/2018 8:48 PM, Boris Brezillon wrote: On Wed, 22 Aug 2018 22:08:42 +0800 Liang Yang wrote: You have to wait tWB, that's for sure. we have a maximum 32 commands fifo. when command is written into NFC_REG_CMD, it doesn't mean that command is executing right now, maybe it is buffering on the queue.Assume one ERASE operation, when 2nd command(0xd0) is written into NFC_REG_CMD and then come into NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be wrong because 0xd0 may not being executed. it is unusual unless buffering two many command. You should flush the queue and wait for it to empty at the end of ->exec_op(). so it seems that i still need to use nand_soft_waitrdy or wait cmd is executed somewhere. Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also, NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's probably what you should use for tWB. em, I can wait for RB by reading the status from register now. but when calling nand_soft_waitrdy, i really met a problem. One *jiffies* is about 4ms. When programming, it pass 1ms to instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one *jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at the tail of 4ms interval, it may only wait 100us and next jiffies arrive. Is it correct?
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris, On 8/24/2018 8:48 PM, Boris Brezillon wrote: On Wed, 22 Aug 2018 22:08:42 +0800 Liang Yang wrote: You have to wait tWB, that's for sure. we have a maximum 32 commands fifo. when command is written into NFC_REG_CMD, it doesn't mean that command is executing right now, maybe it is buffering on the queue.Assume one ERASE operation, when 2nd command(0xd0) is written into NFC_REG_CMD and then come into NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be wrong because 0xd0 may not being executed. it is unusual unless buffering two many command. You should flush the queue and wait for it to empty at the end of ->exec_op(). so it seems that i still need to use nand_soft_waitrdy or wait cmd is executed somewhere. Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also, NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's probably what you should use for tWB. em, I can wait for RB by reading the status from register now. but when calling nand_soft_waitrdy, i really met a problem. One *jiffies* is about 4ms. When programming, it pass 1ms to instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one *jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at the tail of 4ms interval, it may only wait 100us and next jiffies arrive. Is it correct?
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris,
There is a question below. please see my comments.
Thanks.
On 8/17/2018 9:56 PM, Boris Brezillon wrote:
On Fri, 17 Aug 2018 21:03:59 +0800
Liang Yang wrote:
Hi Boris,
On 2018/8/2 5:50, Boris Brezillon wrote:
Hi Yixun,
On Thu, 19 Jul 2018 17:46:12 +0800
Yixun Lan wrote:
I haven't finished reviewing the driver yet (I'll try to do that later
this week), but I already pointed a few things to fix/improve.
+
+static int meson_nfc_exec_op(struct nand_chip *chip,
+const struct nand_operation *op, bool check_only)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct meson_nfc *nfc = nand_get_controller_data(chip);
+ const struct nand_op_instr *instr = NULL;
+ int ret = 0, cmd;
+ unsigned int op_id;
+ int i;
+
+ for (op_id = 0; op_id < op->ninstrs; op_id++) {
+ instr = >instrs[op_id];
+ switch (instr->type) {
+ case NAND_OP_CMD_INSTR:
+ cmd = nfc->param.chip_select | NFC_CMD_CLE;
+ cmd |= instr->ctx.cmd.opcode & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
+ meson_nfc_drain_cmd(nfc);
I don't know exactly how the NAND controller works, but it's usually
+ break;
+
+ case NAND_OP_ADDR_INSTR:
+ for (i = 0; i < instr->ctx.addr.naddrs; i++) {
+ cmd = nfc->param.chip_select | NFC_CMD_ALE;
+ cmd |= instr->ctx.addr.addrs[i] & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ }
+ break;
+
+ case NAND_OP_DATA_IN_INSTR:
+ meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_DATA_OUT_INSTR:
+ meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_WAITRDY_INSTR:
+ mdelay(instr->ctx.waitrdy.timeout_ms);
+ ret = nand_soft_waitrdy(chip,
+ instr->ctx.waitrdy.timeout_ms);
Hm, i'd be surprised if the controller does not have a way to optimize
waits on R/B transitions.
When i delete the delay here, erasing operation will be failed.
Does it mean NFC send 0x70 to nand device when rb is busy(low)?
I was not even talking about the delay, but yes, mdelay() seems way too
big. Remember that it's a timeout, and you usually don't have to wait
that much. You can do ndelay(instr->ctx.delay_ns) before calling
nand_soft_waitrdy() to make sure tWB is enforced.
Anyway, that's not what I was initially referring to. What I meant is
that nand_soft_waitrdy() should be replaced by native R/B pin or status
polling wait logic so that the CPU is released while waiting for a R/B
transition.
If so, i will ask our NFC designer for comfirmation or grasping the waveform.
You have to wait tWB, that's for sure.
we have a maximum 32 commands fifo. when command is written into
NFC_REG_CMD, it doesn't mean that command is executing right now, maybe
it is buffering on the queue.Assume one ERASE operation, when 2nd
command(0xd0) is written into NFC_REG_CMD and then come into
NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be
wrong because 0xd0 may not being executed. it is unusual unless
buffering two many command.
so it seems that i still need to use nand_soft_waitrdy or wait cmd is
executed somewhere.
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris,
There is a question below. please see my comments.
Thanks.
On 8/17/2018 9:56 PM, Boris Brezillon wrote:
On Fri, 17 Aug 2018 21:03:59 +0800
Liang Yang wrote:
Hi Boris,
On 2018/8/2 5:50, Boris Brezillon wrote:
Hi Yixun,
On Thu, 19 Jul 2018 17:46:12 +0800
Yixun Lan wrote:
I haven't finished reviewing the driver yet (I'll try to do that later
this week), but I already pointed a few things to fix/improve.
+
+static int meson_nfc_exec_op(struct nand_chip *chip,
+const struct nand_operation *op, bool check_only)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct meson_nfc *nfc = nand_get_controller_data(chip);
+ const struct nand_op_instr *instr = NULL;
+ int ret = 0, cmd;
+ unsigned int op_id;
+ int i;
+
+ for (op_id = 0; op_id < op->ninstrs; op_id++) {
+ instr = >instrs[op_id];
+ switch (instr->type) {
+ case NAND_OP_CMD_INSTR:
+ cmd = nfc->param.chip_select | NFC_CMD_CLE;
+ cmd |= instr->ctx.cmd.opcode & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
+ meson_nfc_drain_cmd(nfc);
I don't know exactly how the NAND controller works, but it's usually
+ break;
+
+ case NAND_OP_ADDR_INSTR:
+ for (i = 0; i < instr->ctx.addr.naddrs; i++) {
+ cmd = nfc->param.chip_select | NFC_CMD_ALE;
+ cmd |= instr->ctx.addr.addrs[i] & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ }
+ break;
+
+ case NAND_OP_DATA_IN_INSTR:
+ meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_DATA_OUT_INSTR:
+ meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_WAITRDY_INSTR:
+ mdelay(instr->ctx.waitrdy.timeout_ms);
+ ret = nand_soft_waitrdy(chip,
+ instr->ctx.waitrdy.timeout_ms);
Hm, i'd be surprised if the controller does not have a way to optimize
waits on R/B transitions.
When i delete the delay here, erasing operation will be failed.
Does it mean NFC send 0x70 to nand device when rb is busy(low)?
I was not even talking about the delay, but yes, mdelay() seems way too
big. Remember that it's a timeout, and you usually don't have to wait
that much. You can do ndelay(instr->ctx.delay_ns) before calling
nand_soft_waitrdy() to make sure tWB is enforced.
Anyway, that's not what I was initially referring to. What I meant is
that nand_soft_waitrdy() should be replaced by native R/B pin or status
polling wait logic so that the CPU is released while waiting for a R/B
transition.
If so, i will ask our NFC designer for comfirmation or grasping the waveform.
You have to wait tWB, that's for sure.
we have a maximum 32 commands fifo. when command is written into
NFC_REG_CMD, it doesn't mean that command is executing right now, maybe
it is buffering on the queue.Assume one ERASE operation, when 2nd
command(0xd0) is written into NFC_REG_CMD and then come into
NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be
wrong because 0xd0 may not being executed. it is unusual unless
buffering two many command.
so it seems that i still need to use nand_soft_waitrdy or wait cmd is
executed somewhere.
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris,
On 8/17/2018 9:56 PM, Boris Brezillon wrote:
On Fri, 17 Aug 2018 21:03:59 +0800
Liang Yang wrote:
Hi Boris,
On 2018/8/2 5:50, Boris Brezillon wrote:
Hi Yixun,
On Thu, 19 Jul 2018 17:46:12 +0800
Yixun Lan wrote:
I haven't finished reviewing the driver yet (I'll try to do that later
this week), but I already pointed a few things to fix/improve.
+
+static int meson_nfc_exec_op(struct nand_chip *chip,
+const struct nand_operation *op, bool check_only)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct meson_nfc *nfc = nand_get_controller_data(chip);
+ const struct nand_op_instr *instr = NULL;
+ int ret = 0, cmd;
+ unsigned int op_id;
+ int i;
+
+ for (op_id = 0; op_id < op->ninstrs; op_id++) {
+ instr = >instrs[op_id];
+ switch (instr->type) {
+ case NAND_OP_CMD_INSTR:
+ cmd = nfc->param.chip_select | NFC_CMD_CLE;
+ cmd |= instr->ctx.cmd.opcode & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
This is not necessarily TWB you have to wait after a CMD cycle. It can
be tWHR. And you should definitely not hardcode the value, since,
AFAIR, it depends on the selected SDR timings. Probably something you
should calculate in ->setup_data_interface().
Indeed. TWB is not necessarily. And tWHR will be promised by NFC.
so I will delete it.
Are you sure the engine always applies a tWHR delay, even when tWB is
expected? tWB should be applied everytime you are about to wait for a
R/B transition. tWHR is about switching IO pins from input to output on
the NAND chip side.
it seems work well even do not add tWHR, but software needs to promise
tWHR, also the same as TWB. I will check the code and add them.
+ meson_nfc_drain_cmd(nfc);
I don't know exactly how the NAND controller works, but it's usually
not a good idea to execute the operation right away, especially if you
have address/cmd/data cycles following this cmd and those can be
packed in the same controller operation.
it doesn't need meson_nfc_drain_cmd(nfc) here. i will delete it next version
What's the CMD queue depth? I think you'll have to ensure the requested
op fits in the CMD FIFO and split things in several sub-ops if it does
not.
there are maximum 32 commands.
I think it should be enough to promise ONE maximum number of
operations(cmd - addr - dma - 2 ilde commands).
+ break;
+
+ case NAND_OP_ADDR_INSTR:
+ for (i = 0; i < instr->ctx.addr.naddrs; i++) {
+ cmd = nfc->param.chip_select | NFC_CMD_ALE;
+ cmd |= instr->ctx.addr.addrs[i] & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ }
+ break;
+
+ case NAND_OP_DATA_IN_INSTR:
+ meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_DATA_OUT_INSTR:
+ meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
+ instr->ctx.data.len);
Well, I'm not entirely sure what happens when you call
read/write_buf(), but it seems you're doing that one byte at a time,
and that sounds not so efficient given the operation you do for each
byte read/written. Don't you have a way to tell the engine that you
want to read/write X bytes?
As i known, there is no way to read/write X bytes once.
Okay, then maybe you can queue several byte read/write reqs before
flushing the queue (meson_nfc_drain_cmd() +
meson_nfc_wait_cmd_finish()).
+ break;
+
+ case NAND_OP_WAITRDY_INSTR:
+ mdelay(instr->ctx.waitrdy.timeout_ms);
+ ret = nand_soft_waitrdy(chip,
+ instr->ctx.waitrdy.timeout_ms);
Hm, i'd be surprised if the controller does not have a way to optimize
waits on R/B transitions.
When i delete the delay here, erasing operation will be failed.
Does it mean NFC send 0x70 to nand device when rb is busy(low)?
I was not even talking about the delay, but yes, mdelay() seems way too
big. Remember that it's a timeout, and you usually don't have to wait
that much. You can do ndelay(instr->ctx.delay_ns) before calling
nand_soft_waitrdy() to make sure tWB is enforced.
Anyway, that's not what I was initially referring to. What I meant is
that nand_soft_waitrdy() should be replaced by native R/B pin or status
polling wait logic so that the CPU is released while waiting for a R/B
transition.
If so, i will ask our NFC designer for c
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris,
On 8/17/2018 9:56 PM, Boris Brezillon wrote:
On Fri, 17 Aug 2018 21:03:59 +0800
Liang Yang wrote:
Hi Boris,
On 2018/8/2 5:50, Boris Brezillon wrote:
Hi Yixun,
On Thu, 19 Jul 2018 17:46:12 +0800
Yixun Lan wrote:
I haven't finished reviewing the driver yet (I'll try to do that later
this week), but I already pointed a few things to fix/improve.
+
+static int meson_nfc_exec_op(struct nand_chip *chip,
+const struct nand_operation *op, bool check_only)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct meson_nfc *nfc = nand_get_controller_data(chip);
+ const struct nand_op_instr *instr = NULL;
+ int ret = 0, cmd;
+ unsigned int op_id;
+ int i;
+
+ for (op_id = 0; op_id < op->ninstrs; op_id++) {
+ instr = >instrs[op_id];
+ switch (instr->type) {
+ case NAND_OP_CMD_INSTR:
+ cmd = nfc->param.chip_select | NFC_CMD_CLE;
+ cmd |= instr->ctx.cmd.opcode & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
This is not necessarily TWB you have to wait after a CMD cycle. It can
be tWHR. And you should definitely not hardcode the value, since,
AFAIR, it depends on the selected SDR timings. Probably something you
should calculate in ->setup_data_interface().
Indeed. TWB is not necessarily. And tWHR will be promised by NFC.
so I will delete it.
Are you sure the engine always applies a tWHR delay, even when tWB is
expected? tWB should be applied everytime you are about to wait for a
R/B transition. tWHR is about switching IO pins from input to output on
the NAND chip side.
it seems work well even do not add tWHR, but software needs to promise
tWHR, also the same as TWB. I will check the code and add them.
+ meson_nfc_drain_cmd(nfc);
I don't know exactly how the NAND controller works, but it's usually
not a good idea to execute the operation right away, especially if you
have address/cmd/data cycles following this cmd and those can be
packed in the same controller operation.
it doesn't need meson_nfc_drain_cmd(nfc) here. i will delete it next version
What's the CMD queue depth? I think you'll have to ensure the requested
op fits in the CMD FIFO and split things in several sub-ops if it does
not.
there are maximum 32 commands.
I think it should be enough to promise ONE maximum number of
operations(cmd - addr - dma - 2 ilde commands).
+ break;
+
+ case NAND_OP_ADDR_INSTR:
+ for (i = 0; i < instr->ctx.addr.naddrs; i++) {
+ cmd = nfc->param.chip_select | NFC_CMD_ALE;
+ cmd |= instr->ctx.addr.addrs[i] & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ }
+ break;
+
+ case NAND_OP_DATA_IN_INSTR:
+ meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_DATA_OUT_INSTR:
+ meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
+ instr->ctx.data.len);
Well, I'm not entirely sure what happens when you call
read/write_buf(), but it seems you're doing that one byte at a time,
and that sounds not so efficient given the operation you do for each
byte read/written. Don't you have a way to tell the engine that you
want to read/write X bytes?
As i known, there is no way to read/write X bytes once.
Okay, then maybe you can queue several byte read/write reqs before
flushing the queue (meson_nfc_drain_cmd() +
meson_nfc_wait_cmd_finish()).
+ break;
+
+ case NAND_OP_WAITRDY_INSTR:
+ mdelay(instr->ctx.waitrdy.timeout_ms);
+ ret = nand_soft_waitrdy(chip,
+ instr->ctx.waitrdy.timeout_ms);
Hm, i'd be surprised if the controller does not have a way to optimize
waits on R/B transitions.
When i delete the delay here, erasing operation will be failed.
Does it mean NFC send 0x70 to nand device when rb is busy(low)?
I was not even talking about the delay, but yes, mdelay() seems way too
big. Remember that it's a timeout, and you usually don't have to wait
that much. You can do ndelay(instr->ctx.delay_ns) before calling
nand_soft_waitrdy() to make sure tWB is enforced.
Anyway, that's not what I was initially referring to. What I meant is
that nand_soft_waitrdy() should be replaced by native R/B pin or status
polling wait logic so that the CPU is released while waiting for a R/B
transition.
If so, i will ask our NFC designer for c
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris,
On 2018/8/2 5:50, Boris Brezillon wrote:
Hi Yixun,
On Thu, 19 Jul 2018 17:46:12 +0800
Yixun Lan wrote:
I haven't finished reviewing the driver yet (I'll try to do that later
this week), but I already pointed a few things to fix/improve.
+
+static int meson_nfc_exec_op(struct nand_chip *chip,
+const struct nand_operation *op, bool check_only)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct meson_nfc *nfc = nand_get_controller_data(chip);
+ const struct nand_op_instr *instr = NULL;
+ int ret = 0, cmd;
+ unsigned int op_id;
+ int i;
+
+ for (op_id = 0; op_id < op->ninstrs; op_id++) {
+ instr = >instrs[op_id];
+ switch (instr->type) {
+ case NAND_OP_CMD_INSTR:
+ cmd = nfc->param.chip_select | NFC_CMD_CLE;
+ cmd |= instr->ctx.cmd.opcode & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
This is not necessarily TWB you have to wait after a CMD cycle. It can
be tWHR. And you should definitely not hardcode the value, since,
AFAIR, it depends on the selected SDR timings. Probably something you
should calculate in ->setup_data_interface().
Indeed. TWB is not necessarily. And tWHR will be promised by NFC.
so I will delete it.
+ meson_nfc_drain_cmd(nfc);
I don't know exactly how the NAND controller works, but it's usually
not a good idea to execute the operation right away, especially if you
have address/cmd/data cycles following this cmd and those can be
packed in the same controller operation.
it doesn't need meson_nfc_drain_cmd(nfc) here. i will delete it next version
+ break;
+
+ case NAND_OP_ADDR_INSTR:
+ for (i = 0; i < instr->ctx.addr.naddrs; i++) {
+ cmd = nfc->param.chip_select | NFC_CMD_ALE;
+ cmd |= instr->ctx.addr.addrs[i] & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ }
+ break;
+
+ case NAND_OP_DATA_IN_INSTR:
+ meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_DATA_OUT_INSTR:
+ meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
+ instr->ctx.data.len);
Well, I'm not entirely sure what happens when you call
read/write_buf(), but it seems you're doing that one byte at a time,
and that sounds not so efficient given the operation you do for each
byte read/written. Don't you have a way to tell the engine that you
want to read/write X bytes?
As i known, there is no way to read/write X bytes once.
+ break;
+
+ case NAND_OP_WAITRDY_INSTR:
+ mdelay(instr->ctx.waitrdy.timeout_ms);
+ ret = nand_soft_waitrdy(chip,
+ instr->ctx.waitrdy.timeout_ms);
Hm, i'd be surprised if the controller does not have a way to optimize
waits on R/B transitions.
When i delete the delay here, erasing operation will be failed.
Does it mean NFC send 0x70 to nand device when rb is busy(low)?
If so, i will ask our NFC designer for comfirmation or grasping the waveform.
+ break;
+ }
+ }
+ return ret;
+}
+
+static int meson_ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ int free_oob;
+
+ if (section >= chip->ecc.steps)
+ return -ERANGE;
+
+ free_oob = (section + 1) * 2;
+ oobregion->offset = section * chip->ecc.bytes + free_oob;
Hm, this offset calculation looks weird. Are you sure it's correct?
I'd bet on something like:
oobregion->offset = 2 + (section * (chip->ecc.bytes + 4));
Each ecc page have 2 user bytes. Assume one 2KB+64B page size nand
flash using ECC8/1KB which ecc parity bytes is 14B.
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
| | | | | | | not |
|1KB |2B| 14B | 1KB |2B| 14B | used | (layout on nand)
|_ _ _ _ _ _ _|_ |_ _ _ | _ _ _ _ _ _ |_ |_ _ _|_ _ _ _|
(2KB + 64B)
when reading from nand, I will format the page as follow:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
| | | | | | | not |
|1KB |1KB |2B| 14B |2B| 14B | used |(layout on ddr)
|_ _ _ _ _ _ _|_ _ _ _ _ _ _|_ |_ _ _|_ |_ _ _ |_ _ _ _|
(2KB + 64B)
So i get "oobregion->offset = section * chip->ecc.bytes +
Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris,
On 2018/8/2 5:50, Boris Brezillon wrote:
Hi Yixun,
On Thu, 19 Jul 2018 17:46:12 +0800
Yixun Lan wrote:
I haven't finished reviewing the driver yet (I'll try to do that later
this week), but I already pointed a few things to fix/improve.
+
+static int meson_nfc_exec_op(struct nand_chip *chip,
+const struct nand_operation *op, bool check_only)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct meson_nfc *nfc = nand_get_controller_data(chip);
+ const struct nand_op_instr *instr = NULL;
+ int ret = 0, cmd;
+ unsigned int op_id;
+ int i;
+
+ for (op_id = 0; op_id < op->ninstrs; op_id++) {
+ instr = >instrs[op_id];
+ switch (instr->type) {
+ case NAND_OP_CMD_INSTR:
+ cmd = nfc->param.chip_select | NFC_CMD_CLE;
+ cmd |= instr->ctx.cmd.opcode & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
This is not necessarily TWB you have to wait after a CMD cycle. It can
be tWHR. And you should definitely not hardcode the value, since,
AFAIR, it depends on the selected SDR timings. Probably something you
should calculate in ->setup_data_interface().
Indeed. TWB is not necessarily. And tWHR will be promised by NFC.
so I will delete it.
+ meson_nfc_drain_cmd(nfc);
I don't know exactly how the NAND controller works, but it's usually
not a good idea to execute the operation right away, especially if you
have address/cmd/data cycles following this cmd and those can be
packed in the same controller operation.
it doesn't need meson_nfc_drain_cmd(nfc) here. i will delete it next version
+ break;
+
+ case NAND_OP_ADDR_INSTR:
+ for (i = 0; i < instr->ctx.addr.naddrs; i++) {
+ cmd = nfc->param.chip_select | NFC_CMD_ALE;
+ cmd |= instr->ctx.addr.addrs[i] & 0xff;
+ writel(cmd, nfc->reg_base + NFC_REG_CMD);
+ }
+ break;
+
+ case NAND_OP_DATA_IN_INSTR:
+ meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_DATA_OUT_INSTR:
+ meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
+ instr->ctx.data.len);
Well, I'm not entirely sure what happens when you call
read/write_buf(), but it seems you're doing that one byte at a time,
and that sounds not so efficient given the operation you do for each
byte read/written. Don't you have a way to tell the engine that you
want to read/write X bytes?
As i known, there is no way to read/write X bytes once.
+ break;
+
+ case NAND_OP_WAITRDY_INSTR:
+ mdelay(instr->ctx.waitrdy.timeout_ms);
+ ret = nand_soft_waitrdy(chip,
+ instr->ctx.waitrdy.timeout_ms);
Hm, i'd be surprised if the controller does not have a way to optimize
waits on R/B transitions.
When i delete the delay here, erasing operation will be failed.
Does it mean NFC send 0x70 to nand device when rb is busy(low)?
If so, i will ask our NFC designer for comfirmation or grasping the waveform.
+ break;
+ }
+ }
+ return ret;
+}
+
+static int meson_ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ int free_oob;
+
+ if (section >= chip->ecc.steps)
+ return -ERANGE;
+
+ free_oob = (section + 1) * 2;
+ oobregion->offset = section * chip->ecc.bytes + free_oob;
Hm, this offset calculation looks weird. Are you sure it's correct?
I'd bet on something like:
oobregion->offset = 2 + (section * (chip->ecc.bytes + 4));
Each ecc page have 2 user bytes. Assume one 2KB+64B page size nand
flash using ECC8/1KB which ecc parity bytes is 14B.
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
| | | | | | | not |
|1KB |2B| 14B | 1KB |2B| 14B | used | (layout on nand)
|_ _ _ _ _ _ _|_ |_ _ _ | _ _ _ _ _ _ |_ |_ _ _|_ _ _ _|
(2KB + 64B)
when reading from nand, I will format the page as follow:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
| | | | | | | not |
|1KB |1KB |2B| 14B |2B| 14B | used |(layout on ddr)
|_ _ _ _ _ _ _|_ _ _ _ _ _ _|_ |_ _ _|_ |_ _ _ |_ _ _ _|
(2KB + 64B)
So i get "oobregion->offset = section * chip->ecc.bytes +
Re: change strip_cache_size freeze the whole raid
Do we need to consider the chunk size when we adjust the value of Striped_Cache_Szie for the MD-RAID5 array? Liang - Original Message - From: "Justin Piszcz" <[EMAIL PROTECTED]> To: "kyle" <[EMAIL PROTECTED]> Cc: ; Sent: Monday, January 22, 2007 5:18 AM Subject: Re: change strip_cache_size freeze the whole raid On Mon, 22 Jan 2007, kyle wrote: Hi, Yesterday I tried to increase the value of strip_cache_size to see if I can get better performance or not. I increase the value from 2048 to something like 16384. After I did that, the raid5 freeze. Any proccess read / write to it stucked at D state. I tried to change it back to 2048, read strip_cache_active, cat /proc/mdstat, mdadm stop, etc. All didn't return back. I even cannot shutdown the machine. Finally I need to press the reset button in order to get back my control. Kernel is 2.6.17.8 x86-64, running at AMD Athlon3000+, 2GB Ram, 8 x Seagate 8200.10 250GB HDD, nvidia chipset. cat /proc/mdstat (after reboot): Personalities : [raid1] [raid5] [raid4] md1 : active raid1 hdc2[1] hda2[0] 6144768 blocks [2/2] [UU] md2 : active raid5 sdf1[7] sde1[6] sdd1[5] sdc1[4] sdb1[3] sda1[2] hdc4[1] hda4[0] 1664893440 blocks level 5, 512k chunk, algorithm 2 [8/8] [] md0 : active raid1 hdc1[1] hda1[0] 104320 blocks [2/2] [UU] Kyle - To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Yes, I noticed this bug too, if you change it too many times or change it at the 'wrong' time, it hangs up when you echo numbr > /proc/stripe_cache_size. Basically don't run it more than once and don't run it at the 'wrong' time and it works. Not sure where the bug lies, but yeah I've seen that on 3 different machines! Justin. - To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: change strip_cache_size freeze the whole raid
Do we need to consider the chunk size when we adjust the value of Striped_Cache_Szie for the MD-RAID5 array? Liang - Original Message - From: Justin Piszcz [EMAIL PROTECTED] To: kyle [EMAIL PROTECTED] Cc: linux-raid@vger.kernel.org; linux-kernel@vger.kernel.org Sent: Monday, January 22, 2007 5:18 AM Subject: Re: change strip_cache_size freeze the whole raid On Mon, 22 Jan 2007, kyle wrote: Hi, Yesterday I tried to increase the value of strip_cache_size to see if I can get better performance or not. I increase the value from 2048 to something like 16384. After I did that, the raid5 freeze. Any proccess read / write to it stucked at D state. I tried to change it back to 2048, read strip_cache_active, cat /proc/mdstat, mdadm stop, etc. All didn't return back. I even cannot shutdown the machine. Finally I need to press the reset button in order to get back my control. Kernel is 2.6.17.8 x86-64, running at AMD Athlon3000+, 2GB Ram, 8 x Seagate 8200.10 250GB HDD, nvidia chipset. cat /proc/mdstat (after reboot): Personalities : [raid1] [raid5] [raid4] md1 : active raid1 hdc2[1] hda2[0] 6144768 blocks [2/2] [UU] md2 : active raid5 sdf1[7] sde1[6] sdd1[5] sdc1[4] sdb1[3] sda1[2] hdc4[1] hda4[0] 1664893440 blocks level 5, 512k chunk, algorithm 2 [8/8] [] md0 : active raid1 hdc1[1] hda1[0] 104320 blocks [2/2] [UU] Kyle - To unsubscribe from this list: send the line unsubscribe linux-raid in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Yes, I noticed this bug too, if you change it too many times or change it at the 'wrong' time, it hangs up when you echo numbr /proc/stripe_cache_size. Basically don't run it more than once and don't run it at the 'wrong' time and it works. Not sure where the bug lies, but yeah I've seen that on 3 different machines! Justin. - To unsubscribe from this list: send the line unsubscribe linux-raid in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
