Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
HI Boris On 07/19/18 16:39, Boris Brezillon wrote: > Hi Yixun, > > On Thu, 19 Jul 2018 16:13:47 +0800 > Yixun Lan wrote: > > You're doing DMA on those buffers, and devm_kzalloc() is not > DMA-friendly (returned buffers are not aligned on a cache line). Also, > you don't have to allocate your own buffers because the core already > allocate them (chip->data_buf, chip->oob_poi). All you need to do is > set the NAND_USE_BOUNCE_BUFFER flag in chip->options to make sure > you're always passed a DMA-able buffer. > thanks for the suggestion, we've migrated to use the dmam_alloc_coherent() API >>> >>> kzalloc() should be just fine, no need to alloc a DMA coherent region. >>> >> >> we're a little bit confused here, isn't devm_kzalloc (previously we are >> using) a variant of kzalloc? and since the NAND controller is doing DMA >> here, using DMA coherent API is more proper way? > > Well, making buffers DMA coherent might be expensive, especially if you > access them a lot (unless you have a coherency unit and the cache is > kept enabled). > > Regarding the "why is devm_kzalloc() is not DMA-safe?" question, I'd > recommend that you read this discussion [1]. > great, thanks for the info. we fixed this in patch v2 >> +mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL, >> + "%s:nand", dev_name(dev)); >> +if (!mtd->name) { >> +dev_err(nfc->dev, "Failed to allocate mtd->name\n"); >> +return -ENOMEM; >> +} > > You set the name after nand_scan_ident() and make it conditional (only > if ->name == NULL) so that the label property defined in the DT takes > precedence over the default name. >> for setting mtd->name conditional, do you mean doing something like this? >> >> if (!mtd->name) >> mtd->name = devm_kasprintf(..) > > Yes, that's what I meant. > >> >> but we found mtd->name = "ffe07800.nfc" after function >> nand_scan_ident(), which is same value as dev_name(dev).. >> and there is no cs information encoded there. > > Hm, that shouldn't be the case. Maybe you can add traces to find out > who is setting mtd->name to this value. > will trace this, then get back to you >> > Also, I recommend suffixing this name > with the CS id, just in case you ever need to support connecting several > chips to the same controller. > we actually didn't get the point here, cs is about chip selection with multiple nand chip? and how to get this information? >>> >>> Well, you currently seem to only support one chip per controller, but I >>> guess the IP can handle several CS lines. So my recommendation is about >>> choosing a name so that you can later easily add support for multiple >>> chips without breaking setups where mtdparts is used. >>> >>> To sum-up, assuming your NAND chip is always connected to CS0 (on the >>> controller side), I'd suggest doing: >>> >> yes, this is exactly how the hardware connected. >>> mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL, >>>"%s:nand.%d", dev_name(dev), cs_id); >>> >>> where cs_id is the value you extracted from the reg property of the >>> NAND node. >>> >> Ok, you right. >> current, the NAND chip is only use one CS (which CE0) for now, what's in >> the DT is >> >> nand@0 { >> reg = < 0 >; >> .. >> }; >> >> so for the multiple chips it would something like this in DT? >> >> nand@0 { >> reg = < 0 >; >> }; >> >> nand@1 { >> reg = < 1 >; >> }; > > Yep, that's for 2 single-die chips. > >> >> or even >> nand@0 { >> reg = < 0 2 >; >> }; >> >> nand@1 { > > nand@3 { > >> reg = < 3 4 >; >> }; > > And this is describing 2 dual-die chips. > >> >> do we need to encode all the cs information here? not sure if we >> understand this correctly, but could send out the patch for review.. > > Yes, reg should contain an array of controller-side CS lines used to > select the chip (or a specific die in a chip, the index in the reg > table being the id of the die). > much clear about this, thanks Yixun
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Yixun, On Thu, 19 Jul 2018 16:13:47 +0800 Yixun Lan wrote: > >>> You're doing DMA on those buffers, and devm_kzalloc() is not > >>> DMA-friendly (returned buffers are not aligned on a cache line). Also, > >>> you don't have to allocate your own buffers because the core already > >>> allocate them (chip->data_buf, chip->oob_poi). All you need to do is > >>> set the NAND_USE_BOUNCE_BUFFER flag in chip->options to make sure > >>> you're always passed a DMA-able buffer. > >>> > >> > >> thanks for the suggestion, we've migrated to use the > >> dmam_alloc_coherent() API > > > > kzalloc() should be just fine, no need to alloc a DMA coherent region. > > > > we're a little bit confused here, isn't devm_kzalloc (previously we are > using) a variant of kzalloc? and since the NAND controller is doing DMA > here, using DMA coherent API is more proper way? Well, making buffers DMA coherent might be expensive, especially if you access them a lot (unless you have a coherency unit and the cache is kept enabled). Regarding the "why is devm_kzalloc() is not DMA-safe?" question, I'd recommend that you read this discussion [1]. > +mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL, > + "%s:nand", dev_name(dev)); > +if (!mtd->name) { > +dev_err(nfc->dev, "Failed to allocate mtd->name\n"); > +return -ENOMEM; > +} > >>> > >>> You set the name after nand_scan_ident() and make it conditional (only > >>> if ->name == NULL) so that the label property defined in the DT takes > >>> precedence over the default name. > >> > for setting mtd->name conditional, do you mean doing something like this? > > if (!mtd->name) > mtd->name = devm_kasprintf(..) Yes, that's what I meant. > > but we found mtd->name = "ffe07800.nfc" after function > nand_scan_ident(), which is same value as dev_name(dev).. > and there is no cs information encoded there. Hm, that shouldn't be the case. Maybe you can add traces to find out who is setting mtd->name to this value. > > >> > >>> Also, I recommend suffixing this name > >>> with the CS id, just in case you ever need to support connecting several > >>> chips to the same controller. > >>> > >> > >> we actually didn't get the point here, cs is about chip selection with > >> multiple nand chip? and how to get this information? > > > > Well, you currently seem to only support one chip per controller, but I > > guess the IP can handle several CS lines. So my recommendation is about > > choosing a name so that you can later easily add support for multiple > > chips without breaking setups where mtdparts is used. > > > > To sum-up, assuming your NAND chip is always connected to CS0 (on the > > controller side), I'd suggest doing: > > > yes, this is exactly how the hardware connected. > > mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL, > >"%s:nand.%d", dev_name(dev), cs_id); > > > > where cs_id is the value you extracted from the reg property of the > > NAND node. > > > Ok, you right. > current, the NAND chip is only use one CS (which CE0) for now, what's in > the DT is > > nand@0 { > reg = < 0 >; > .. > }; > > so for the multiple chips it would something like this in DT? > > nand@0 { > reg = < 0 >; > }; > > nand@1 { > reg = < 1 >; > }; Yep, that's for 2 single-die chips. > > or even > nand@0 { > reg = < 0 2 >; > }; > > nand@1 { nand@3 { > reg = < 3 4 >; > }; And this is describing 2 dual-die chips. > > do we need to encode all the cs information here? not sure if we > understand this correctly, but could send out the patch for review.. Yes, reg should contain an array of controller-side CS lines used to select the chip (or a specific die in a chip, the index in the reg table being the id of the die). Regards, Boris [1]http://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
HI Boris: thanks for the quick response. On 07/19/18 03:08, Boris Brezillon wrote: > Hi Yixun, > > On Wed, 18 Jul 2018 17:38:56 +0800 > Yixun Lan wrote: > + +#define NFC_REG_CMD 0x00 +#define NFC_REG_CFG 0x04 +#define NFC_REG_DADR 0x08 +#define NFC_REG_IADR 0x0c +#define NFC_REG_BUF 0x10 +#define NFC_REG_INFO 0x14 +#define NFC_REG_DC0x18 +#define NFC_REG_ADR 0x1c +#define NFC_REG_DL0x20 +#define NFC_REG_DH0x24 +#define NFC_REG_CADR 0x28 +#define NFC_REG_SADR 0x2c +#define NFC_REG_PINS 0x30 +#define NFC_REG_VER 0x38 + >>> >>> Can you put the reg offsets next to their field definitions? >>> >> actually, we would prefer to put all the CMD definition below the reg >> offset, so it will better reflect what's it belong to. > > Just to be clear, I meant something like: > > #define NFC_CMD 0x00 > #define NFC_CMD_DRD (0x8 << 14) > #define NFC_CMD_IDLE (0xc << 14) > ... > > #define NFC_CFG 0x04 > #define NFC_CFG_XXX xxx > ... > > I find it easier to guess which register the fields are attached to when > it's defined like that, but I won't block the driver for such a tiny > detail. > yes, this is exactly what I mean +static void meson_nfc_cmd_ctrl(struct mtd_info *mtd, + int cmd, unsigned int ctrl) >>> >>> ->cmd_ctrl() has recently been deprecated in favor of ->exec_op(). You >>> can have a look at the marvell, v610 or fsmc drivers if you want to >>> have an idea of how ->exec_op() should be implemented. Miquel and I are >>> also here to help if you have any questions. >>> >> >> follow your suggestion, we have implemented the exec_op() interface, >> we'd really appreciate if you can help to review this .. > > Sure, just send a v2 and we'll review it. > > + +static void meson_nfc_cmd_m2n(struct meson_nfc *nfc, int raw) >>> >>> n2m -> nand2mem ? >>> >> yes, it is > > Then please use nand2mem, it's clearer. we end at dropping the n2m function. by converting them into static void meson_nfc_cmd_access( struct meson_nfc *nfc, struct mtd_info *mtd, int raw, bool dir) > +static int meson_nfc_wait_dma_finish(struct meson_nfc *nfc) +{ + meson_nfc_cmd_idle(nfc, 0); + meson_nfc_cmd_idle(nfc, 0); >>> >>> Two calls to cmd_idle(), is this expected or a copy&paste error? If >>> that's expected it definitely deserves a comment explaining why? >>> >> >> yes, it is intentional >> >> we will put these comments into the function. >> /* >> * The Nand flash controller is designed as two stages pipleline - >> * a) fetch and b) excute. >> * So, there might be cases when the driver see command queue is >> empty, >> * but the Nand flash controller still has two commands buffered, >> * one is fetched into NFC request queue (ready to run), and another >> * is actively executing. >> */ >> > > So pushing 2 "IDLE" commands guarantees that the pipeline is emptied, > right? The comment looks incomplete, you should explain what those > meson_nfc_cmd_idle() are for. > thanks the meson_nfc_cmd_idle() function itself is quite straightforward, and we feel explain that inserting 2 "IDLE" commands to drain out the pipeline is enough. +static int meson_nfc_ecc_init(struct device *dev, struct mtd_info *mtd) +{ + struct nand_chip *nand = mtd_to_nand(mtd); + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); + struct meson_nfc *nfc = nand_get_controller_data(nand); + struct meson_nand_ecc *meson_ecc = nfc->data->ecc; + int num = nfc->data->ecc_num; + int nsectors, i, bytes; + + /* support only ecc hw mode */ + if (nand->ecc.mode != NAND_ECC_HW) { >>> >>> Given that you support raw accesses, I'm pretty sure you can support >>> ECC_NONE, ECC_SOFT and ECC_ON_DIE with zero effort. >>> >> >> is this a block for this driver to be accepted by upstream? > > Nope. > >> otherwise we'd like to implement this feature later in separate patch. >> > > That's fine. > + nsectors = mtd->writesize / nand->ecc.size; + bytes =(meson_chip->user_mode == NFC_USER2_OOB_BYTES) ? nsectors * 2 : 16; + if (mtd->oobsize < (nand->ecc.bytes * nsectors + bytes)) + return -EINVAL; >>> >>> It's probably worth looking at what is being proposed here [2] for the >>> ECC config selection logic. >>> >> >> sure, we'd happy to adopt the ECC config helper function, and seems it >> is possible ;-) >> >> sounds the proposed ECC config patch is still under review, and we >> would like to adjust our code once it'
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Yixun, On Wed, 18 Jul 2018 17:38:56 +0800 Yixun Lan wrote: > >> + > >> +#define NFC_REG_CMD 0x00 > >> +#define NFC_REG_CFG 0x04 > >> +#define NFC_REG_DADR 0x08 > >> +#define NFC_REG_IADR 0x0c > >> +#define NFC_REG_BUF 0x10 > >> +#define NFC_REG_INFO 0x14 > >> +#define NFC_REG_DC0x18 > >> +#define NFC_REG_ADR 0x1c > >> +#define NFC_REG_DL0x20 > >> +#define NFC_REG_DH0x24 > >> +#define NFC_REG_CADR 0x28 > >> +#define NFC_REG_SADR 0x2c > >> +#define NFC_REG_PINS 0x30 > >> +#define NFC_REG_VER 0x38 > >> + > > > > Can you put the reg offsets next to their field definitions? > > > actually, we would prefer to put all the CMD definition below the reg > offset, so it will better reflect what's it belong to. Just to be clear, I meant something like: #define NFC_CMD 0x00 #define NFC_CMD_DRD (0x8 << 14) #define NFC_CMD_IDLE(0xc << 14) ... #define NFC_CFG 0x04 #define NFC_CFG_XXX xxx ... I find it easier to guess which register the fields are attached to when it's defined like that, but I won't block the driver for such a tiny detail. > >> +static void meson_nfc_cmd_ctrl(struct mtd_info *mtd, > >> + int cmd, unsigned int ctrl) > > > > ->cmd_ctrl() has recently been deprecated in favor of ->exec_op(). You > > can have a look at the marvell, v610 or fsmc drivers if you want to > > have an idea of how ->exec_op() should be implemented. Miquel and I are > > also here to help if you have any questions. > > > > follow your suggestion, we have implemented the exec_op() interface, > we'd really appreciate if you can help to review this .. Sure, just send a v2 and we'll review it. > >> + > >> +static void meson_nfc_cmd_m2n(struct meson_nfc *nfc, int raw) > > > > n2m -> nand2mem ? > > > yes, it is Then please use nand2mem, it's clearer. > >> +static int meson_nfc_wait_dma_finish(struct meson_nfc *nfc) > >> +{ > >> + meson_nfc_cmd_idle(nfc, 0); > >> + meson_nfc_cmd_idle(nfc, 0); > > > > Two calls to cmd_idle(), is this expected or a copy&paste error? If > > that's expected it definitely deserves a comment explaining why? > > > > yes, it is intentional > > we will put these comments into the function. > /* > * The Nand flash controller is designed as two stages pipleline - > * a) fetch and b) excute. > * So, there might be cases when the driver see command queue is > empty, > * but the Nand flash controller still has two commands buffered, > * one is fetched into NFC request queue (ready to run), and another > * is actively executing. > */ > So pushing 2 "IDLE" commands guarantees that the pipeline is emptied, right? The comment looks incomplete, you should explain what those meson_nfc_cmd_idle() are for. > >> +static int meson_nfc_ecc_init(struct device *dev, struct mtd_info *mtd) > >> +{ > >> + struct nand_chip *nand = mtd_to_nand(mtd); > >> + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > >> + struct meson_nfc *nfc = nand_get_controller_data(nand); > >> + struct meson_nand_ecc *meson_ecc = nfc->data->ecc; > >> + int num = nfc->data->ecc_num; > >> + int nsectors, i, bytes; > >> + > >> + /* support only ecc hw mode */ > >> + if (nand->ecc.mode != NAND_ECC_HW) { > > > > Given that you support raw accesses, I'm pretty sure you can support > > ECC_NONE, ECC_SOFT and ECC_ON_DIE with zero effort. > > > > is this a block for this driver to be accepted by upstream? Nope. > otherwise we'd like to implement this feature later in separate patch. > That's fine. > >> + nsectors = mtd->writesize / nand->ecc.size; > >> + bytes =(meson_chip->user_mode == NFC_USER2_OOB_BYTES) ? nsectors * 2 : > >> 16; > >> + if (mtd->oobsize < (nand->ecc.bytes * nsectors + bytes)) > >> + return -EINVAL; > > > > It's probably worth looking at what is being proposed here [2] for the > > ECC config selection logic. > > > > sure, we'd happy to adopt the ECC config helper function, and seems it > is possible ;-) > > sounds the proposed ECC config patch is still under review, and we > would like to adjust our code once it's ready (probably we will still > keep this version in patch v2, then address it in v3 later) It's been merged, and should be available in the nand/next branch [1]. > >> +static int meson_nfc_buffer_init(struct mtd_info *mtd) > >> +{ > >> + struct nand_chip *nand = mtd_to_nand(mtd); > >> + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > >> + struct meson_nfc *nfc = nand_get_controller_data(nand); > >> + struct device *dev = nfc->dev; > >> + int info_bytes, page_bytes; > >> + int nsectors; > >> + > >> + nsectors = mtd
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Roris thanks for all your suggestions! It actually takes us some time to digest all your comments ;-) and get back to you on these questions. On 06/25/18 03:38, Boris Brezillon wrote: > > > Hi Yixun, > > On Wed, 13 Jun 2018 16:13:14 + > Yixun Lan wrote: > >> From: Liang Yang >> >> Add initial support for the Amlogic NAND flash controller which found >> in the Meson-GXBB/GXL/AXG SoCs. >> >> Singed-off-by: Liang Yang >> Signed-off-by: Yixun Lan >> --- >> drivers/mtd/nand/raw/Kconfig |8 + >> drivers/mtd/nand/raw/Makefile |3 + >> drivers/mtd/nand/raw/meson_nand.c | 1422 + >> 3 files changed, 1433 insertions(+) >> create mode 100644 drivers/mtd/nand/raw/meson_nand.c > > Can you run checkpatch.pl --strict and fix the coding style issues? > sure, we will be more cautious about this >> >> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig >> index 19a2b283fbbe..b3c17a3ca8f4 100644 >> --- a/drivers/mtd/nand/raw/Kconfig >> +++ b/drivers/mtd/nand/raw/Kconfig >> @@ -534,4 +534,12 @@ config MTD_NAND_MTK >>Enables support for NAND controller on MTK SoCs. >>This controller is found on mt27xx, mt81xx, mt65xx SoCs. >> >> +config MTD_NAND_MESON >> +tristate "Support for NAND flash controller on Amlogic's Meson SoCs" >> +depends on ARCH_MESON || COMPILE_TEST >> +select COMMON_CLK_REGMAP_MESON >> +select MFD_SYSCON >> +help >> + Enables support for NAND controller on Amlogic's Meson SoCs. >> + >> endif # MTD_NAND >> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile >> index 165b7ef9e9a1..cdf6162f38c3 100644 >> --- a/drivers/mtd/nand/raw/Makefile >> +++ b/drivers/mtd/nand/raw/Makefile >> @@ -1,5 +1,7 @@ >> # SPDX-License-Identifier: GPL-2.0 >> >> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson > > Please don't do that. If you need to expose common regs, put them > in include/linux/soc/meson/. I'm also not sure why you need to access > the clk regs directly. Why can't you expose the MMC/NAND clk as a clk > provider whose driver would be placed in drivers/clk and which would use > the mmc syscon. This way the same clk driver could be used for both > MMC and NAND clk indifferently, and the NAND driver would be much > simpler. > this is already addressed in another thread, as we will model it as a standard clock driver. so this cflags can be dropped. >> + >> obj-$(CONFIG_MTD_NAND) += nand.o >> obj-$(CONFIG_MTD_NAND_ECC) += nand_ecc.o >> obj-$(CONFIG_MTD_NAND_BCH) += nand_bch.o >> @@ -56,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_HISI504) += >> hisi504_nand.o >> obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/ >> obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o >> obj-$(CONFIG_MTD_NAND_MTK) += mtk_ecc.o mtk_nand.o >> +obj-$(CONFIG_MTD_NAND_MESON)+= meson_nand.o >> >> nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o >> nand-objs += nand_amd.o >> diff --git a/drivers/mtd/nand/raw/meson_nand.c >> b/drivers/mtd/nand/raw/meson_nand.c >> new file mode 100644 >> index ..28abc3684772 >> --- /dev/null >> +++ b/drivers/mtd/nand/raw/meson_nand.c >> @@ -0,0 +1,1422 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Amlogic Meson Nand Flash Controller Driver >> + * >> + * Copyright (c) 2018 Amlogic, inc. >> + * Author: Liang Yang >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include "clk-regmap.h" >> + >> +#define NFC_REG_CMD 0x00 >> +#define NFC_REG_CFG 0x04 >> +#define NFC_REG_DADR0x08 >> +#define NFC_REG_IADR0x0c >> +#define NFC_REG_BUF 0x10 >> +#define NFC_REG_INFO0x14 >> +#define NFC_REG_DC 0x18 >> +#define NFC_REG_ADR 0x1c >> +#define NFC_REG_DL 0x20 >> +#define NFC_REG_DH 0x24 >> +#define NFC_REG_CADR0x28 >> +#define NFC_REG_SADR0x2c >> +#define NFC_REG_PINS0x30 >> +#define NFC_REG_VER 0x38 >> + > > Can you put the reg offsets next to their field definitions? > actually, we would prefer to put all the CMD definition below the reg offset, so it will better reflect what's it belong to. >> + >> +#define NFC_CMD_DRD (0x8 << 14) >> +#define NFC_CMD_IDLE(0xc << 14) >> +#define NFC_CMD_DWR (0x4 << 14) >> +#define NFC_CMD_CLE (0x5 << 14) >> +#define NFC_CMD_ALE (0x6 << 14) >> +#define NFC_CMD_ADL ((0 << 16) | (3 << 20)) >> +#define NFC_CMD_ADH ((1 << 16) | (3 << 20)) >> +#define NFC_CMD_AIL ((2 << 16) | (3 << 20)) >> +#define NFC_CMD_AIH ((3 << 16) | (3 << 20)) >> +#define NFC_CMD_SEED((8 << 16) | (3 << 20)) >> +#define NFC_CMD_M2N
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
HI Kevin On 06/29/18 07:45, Kevin Hilman wrote: > Hi Miquel, > > Miquel Raynal writes: > >> On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman >> wrote: >> >>> Hi Boris, >>> >>> Boris Brezillon writes: >>> Hi Yixun, On Wed, 13 Jun 2018 16:13:14 + Yixun Lan wrote: > From: Liang Yang > > Add initial support for the Amlogic NAND flash controller which found > in the Meson-GXBB/GXL/AXG SoCs. > > Singed-off-by: Liang Yang > Signed-off-by: Yixun Lan > --- > drivers/mtd/nand/raw/Kconfig |8 + > drivers/mtd/nand/raw/Makefile |3 + > drivers/mtd/nand/raw/meson_nand.c | 1422 + > 3 files changed, 1433 insertions(+) > create mode 100644 drivers/mtd/nand/raw/meson_nand.c Can you run checkpatch.pl --strict and fix the coding style issues? > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index 19a2b283fbbe..b3c17a3ca8f4 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -534,4 +534,12 @@ config MTD_NAND_MTK > Enables support for NAND controller on MTK SoCs. > This controller is found on mt27xx, mt81xx, mt65xx SoCs. > > +config MTD_NAND_MESON > + tristate "Support for NAND flash controller on Amlogic's Meson SoCs" > + depends on ARCH_MESON || COMPILE_TEST > + select COMMON_CLK_REGMAP_MESON > + select MFD_SYSCON > + help > + Enables support for NAND controller on Amlogic's Meson SoCs. > + > endif # MTD_NAND > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > index 165b7ef9e9a1..cdf6162f38c3 100644 > --- a/drivers/mtd/nand/raw/Makefile > +++ b/drivers/mtd/nand/raw/Makefile > @@ -1,5 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > > +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson Please don't do that. If you need to expose common regs, put them in include/linux/soc/meson/. I'm also not sure why you need to access the clk regs directly. Why can't you expose the MMC/NAND clk as a clk provider whose driver would be placed in drivers/clk and which would use the mmc syscon. This way the same clk driver could be used for both MMC and NAND clk indifferently, and the NAND driver would be much simpler. >>> >>> [...] >>> > + > + return 0; > +} > + > +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS]; > + > +static struct clk_regmap sd_emmc_c_ext_clk0_sel = { > + .data = &(struct clk_regmap_mux_data){ > + .offset = SD_EMMC_CLOCK, > + .mask = 0x3, > + .shift = 6, > + }, > + .hw.init = &(struct clk_init_data) { > + .name = "sd_emmc_c_nand_clk_mux", > + .ops = &clk_regmap_mux_ops, > + .parent_names = sd_emmc_ext_clk0_parent_names, > + .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names), > + .flags = CLK_SET_RATE_PARENT, > + }, > +}; > + > +static struct clk_regmap sd_emmc_c_ext_clk0_div = { > + .data = &(struct clk_regmap_div_data){ > + .offset = SD_EMMC_CLOCK, > + .shift = 0, > + .width = 6, > + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, > + }, > + .hw.init = &(struct clk_init_data) { > + .name = "sd_emmc_c_nand_clk_div", > + .ops = &clk_regmap_divider_ops, > + .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + }, > +}; > + > +static int meson_nfc_clk_init(struct meson_nfc *nfc) > +{ > + struct clk_regmap *mux = &sd_emmc_c_ext_clk0_sel; > + struct clk_regmap *div = &sd_emmc_c_ext_clk0_div; > + struct clk *clk; > + int i, ret; > + > + /* request core clock */ > + nfc->core_clk = devm_clk_get(nfc->dev, "core"); > + if (IS_ERR(nfc->core_clk)) { > + dev_err(nfc->dev, "failed to get core clk\n"); > + return PTR_ERR(nfc->core_clk); > + } > + > + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */ > + regmap_update_bits(nfc->reg_clk, 0, > + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK, > + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK); > + > + /* get the mux parents */ > + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { > + char name[16]; > + > + snprintf(name, sizeof(name), "clkin%d", i); > + clk = devm_clk_get(nfc->dev, name); > + if (IS_ERR(clk)) { > + if (clk != ERR_PTR(-EPROBE_DEFER)) > + dev_err(nfc->dev, "Missing clock %s\n", name); > + return PTR_ERR(clk); > +
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
On 29/06/2018 01:45, Kevin Hilman wrote: > Hi Miquel, > > Miquel Raynal writes: > >> On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman >> wrote: >> >>> Hi Boris, >>> >>> Boris Brezillon writes: >>> Hi Yixun, On Wed, 13 Jun 2018 16:13:14 + Yixun Lan wrote: > From: Liang Yang > > Add initial support for the Amlogic NAND flash controller which found > in the Meson-GXBB/GXL/AXG SoCs. > > Singed-off-by: Liang Yang > Signed-off-by: Yixun Lan > --- > drivers/mtd/nand/raw/Kconfig |8 + > drivers/mtd/nand/raw/Makefile |3 + > drivers/mtd/nand/raw/meson_nand.c | 1422 + > 3 files changed, 1433 insertions(+) > create mode 100644 drivers/mtd/nand/raw/meson_nand.c Can you run checkpatch.pl --strict and fix the coding style issues? > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index 19a2b283fbbe..b3c17a3ca8f4 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -534,4 +534,12 @@ config MTD_NAND_MTK > Enables support for NAND controller on MTK SoCs. > This controller is found on mt27xx, mt81xx, mt65xx SoCs. > > +config MTD_NAND_MESON > + tristate "Support for NAND flash controller on Amlogic's Meson SoCs" > + depends on ARCH_MESON || COMPILE_TEST > + select COMMON_CLK_REGMAP_MESON > + select MFD_SYSCON > + help > + Enables support for NAND controller on Amlogic's Meson SoCs. > + > endif # MTD_NAND > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > index 165b7ef9e9a1..cdf6162f38c3 100644 > --- a/drivers/mtd/nand/raw/Makefile > +++ b/drivers/mtd/nand/raw/Makefile > @@ -1,5 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > > +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson Please don't do that. If you need to expose common regs, put them in include/linux/soc/meson/. I'm also not sure why you need to access the clk regs directly. Why can't you expose the MMC/NAND clk as a clk provider whose driver would be placed in drivers/clk and which would use the mmc syscon. This way the same clk driver could be used for both MMC and NAND clk indifferently, and the NAND driver would be much simpler. >>> >>> [...] >>> > + > + return 0; > +} > + > +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS]; > + > +static struct clk_regmap sd_emmc_c_ext_clk0_sel = { > + .data = &(struct clk_regmap_mux_data){ > + .offset = SD_EMMC_CLOCK, > + .mask = 0x3, > + .shift = 6, > + }, > + .hw.init = &(struct clk_init_data) { > + .name = "sd_emmc_c_nand_clk_mux", > + .ops = &clk_regmap_mux_ops, > + .parent_names = sd_emmc_ext_clk0_parent_names, > + .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names), > + .flags = CLK_SET_RATE_PARENT, > + }, > +}; > + > +static struct clk_regmap sd_emmc_c_ext_clk0_div = { > + .data = &(struct clk_regmap_div_data){ > + .offset = SD_EMMC_CLOCK, > + .shift = 0, > + .width = 6, > + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, > + }, > + .hw.init = &(struct clk_init_data) { > + .name = "sd_emmc_c_nand_clk_div", > + .ops = &clk_regmap_divider_ops, > + .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + }, > +}; > + > +static int meson_nfc_clk_init(struct meson_nfc *nfc) > +{ > + struct clk_regmap *mux = &sd_emmc_c_ext_clk0_sel; > + struct clk_regmap *div = &sd_emmc_c_ext_clk0_div; > + struct clk *clk; > + int i, ret; > + > + /* request core clock */ > + nfc->core_clk = devm_clk_get(nfc->dev, "core"); > + if (IS_ERR(nfc->core_clk)) { > + dev_err(nfc->dev, "failed to get core clk\n"); > + return PTR_ERR(nfc->core_clk); > + } > + > + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */ > + regmap_update_bits(nfc->reg_clk, 0, > + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK, > + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK); > + > + /* get the mux parents */ > + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { > + char name[16]; > + > + snprintf(name, sizeof(name), "clkin%d", i); > + clk = devm_clk_get(nfc->dev, name); > + if (IS_ERR(clk)) { > + if (clk != ERR_PTR(-EPROBE_DEFER)) > + dev_err(nfc->dev, "Missing clock %s\n", name); > + return PTR_ERR(clk); > + } >
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Miquel, Miquel Raynal writes: > On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman > wrote: > >> Hi Boris, >> >> Boris Brezillon writes: >> >> > Hi Yixun, >> > >> > On Wed, 13 Jun 2018 16:13:14 + >> > Yixun Lan wrote: >> > >> >> From: Liang Yang >> >> >> >> Add initial support for the Amlogic NAND flash controller which found >> >> in the Meson-GXBB/GXL/AXG SoCs. >> >> >> >> Singed-off-by: Liang Yang >> >> Signed-off-by: Yixun Lan >> >> --- >> >> drivers/mtd/nand/raw/Kconfig |8 + >> >> drivers/mtd/nand/raw/Makefile |3 + >> >> drivers/mtd/nand/raw/meson_nand.c | 1422 + >> >> 3 files changed, 1433 insertions(+) >> >> create mode 100644 drivers/mtd/nand/raw/meson_nand.c >> > >> > Can you run checkpatch.pl --strict and fix the coding style issues? >> > >> >> >> >> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig >> >> index 19a2b283fbbe..b3c17a3ca8f4 100644 >> >> --- a/drivers/mtd/nand/raw/Kconfig >> >> +++ b/drivers/mtd/nand/raw/Kconfig >> >> @@ -534,4 +534,12 @@ config MTD_NAND_MTK >> >> Enables support for NAND controller on MTK SoCs. >> >> This controller is found on mt27xx, mt81xx, mt65xx SoCs. >> >> >> >> +config MTD_NAND_MESON >> >> + tristate "Support for NAND flash controller on Amlogic's Meson SoCs" >> >> + depends on ARCH_MESON || COMPILE_TEST >> >> + select COMMON_CLK_REGMAP_MESON >> >> + select MFD_SYSCON >> >> + help >> >> + Enables support for NAND controller on Amlogic's Meson SoCs. >> >> + >> >> endif # MTD_NAND >> >> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile >> >> index 165b7ef9e9a1..cdf6162f38c3 100644 >> >> --- a/drivers/mtd/nand/raw/Makefile >> >> +++ b/drivers/mtd/nand/raw/Makefile >> >> @@ -1,5 +1,7 @@ >> >> # SPDX-License-Identifier: GPL-2.0 >> >> >> >> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson >> > >> > Please don't do that. If you need to expose common regs, put them >> > in include/linux/soc/meson/. I'm also not sure why you need to access >> > the clk regs directly. Why can't you expose the MMC/NAND clk as a clk >> > provider whose driver would be placed in drivers/clk and which would use >> > the mmc syscon. This way the same clk driver could be used for both >> > MMC and NAND clk indifferently, and the NAND driver would be much >> > simpler. >> >> [...] >> >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS]; >> >> + >> >> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = { >> >> + .data = &(struct clk_regmap_mux_data){ >> >> + .offset = SD_EMMC_CLOCK, >> >> + .mask = 0x3, >> >> + .shift = 6, >> >> + }, >> >> + .hw.init = &(struct clk_init_data) { >> >> + .name = "sd_emmc_c_nand_clk_mux", >> >> + .ops = &clk_regmap_mux_ops, >> >> + .parent_names = sd_emmc_ext_clk0_parent_names, >> >> + .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names), >> >> + .flags = CLK_SET_RATE_PARENT, >> >> + }, >> >> +}; >> >> + >> >> +static struct clk_regmap sd_emmc_c_ext_clk0_div = { >> >> + .data = &(struct clk_regmap_div_data){ >> >> + .offset = SD_EMMC_CLOCK, >> >> + .shift = 0, >> >> + .width = 6, >> >> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, >> >> + }, >> >> + .hw.init = &(struct clk_init_data) { >> >> + .name = "sd_emmc_c_nand_clk_div", >> >> + .ops = &clk_regmap_divider_ops, >> >> + .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" }, >> >> + .num_parents = 1, >> >> + .flags = CLK_SET_RATE_PARENT, >> >> + }, >> >> +}; >> >> + >> >> +static int meson_nfc_clk_init(struct meson_nfc *nfc) >> >> +{ >> >> + struct clk_regmap *mux = &sd_emmc_c_ext_clk0_sel; >> >> + struct clk_regmap *div = &sd_emmc_c_ext_clk0_div; >> >> + struct clk *clk; >> >> + int i, ret; >> >> + >> >> + /* request core clock */ >> >> + nfc->core_clk = devm_clk_get(nfc->dev, "core"); >> >> + if (IS_ERR(nfc->core_clk)) { >> >> + dev_err(nfc->dev, "failed to get core clk\n"); >> >> + return PTR_ERR(nfc->core_clk); >> >> + } >> >> + >> >> + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */ >> >> + regmap_update_bits(nfc->reg_clk, 0, >> >> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK, >> >> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK); >> >> + >> >> + /* get the mux parents */ >> >> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { >> >> + char name[16]; >> >> + >> >> + snprintf(name, sizeof(name), "clkin%d", i); >> >> + clk = devm_clk_get(nfc->dev, name); >> >> + if (IS_ERR(clk)) { >> >> + if (clk != ERR_PTR(-EPROBE_DEFER)) >> >> + dev_err(nfc->dev, "Missing clock %s\n", name); >> >> + return PTR_ERR(clk); >> >> + } >> >> + >> >> + sd_emmc_ext_clk0_parent_names
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Kevin, On Wed, 27 Jun 2018 16:33:43 -0700, Kevin Hilman wrote: > Hi Boris, > > Boris Brezillon writes: > > > Hi Yixun, > > > > On Wed, 13 Jun 2018 16:13:14 + > > Yixun Lan wrote: > > > >> From: Liang Yang > >> > >> Add initial support for the Amlogic NAND flash controller which found > >> in the Meson-GXBB/GXL/AXG SoCs. > >> > >> Singed-off-by: Liang Yang > >> Signed-off-by: Yixun Lan > >> --- > >> drivers/mtd/nand/raw/Kconfig |8 + > >> drivers/mtd/nand/raw/Makefile |3 + > >> drivers/mtd/nand/raw/meson_nand.c | 1422 + > >> 3 files changed, 1433 insertions(+) > >> create mode 100644 drivers/mtd/nand/raw/meson_nand.c > > > > Can you run checkpatch.pl --strict and fix the coding style issues? > > > >> > >> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > >> index 19a2b283fbbe..b3c17a3ca8f4 100644 > >> --- a/drivers/mtd/nand/raw/Kconfig > >> +++ b/drivers/mtd/nand/raw/Kconfig > >> @@ -534,4 +534,12 @@ config MTD_NAND_MTK > >> Enables support for NAND controller on MTK SoCs. > >> This controller is found on mt27xx, mt81xx, mt65xx SoCs. > >> > >> +config MTD_NAND_MESON > >> + tristate "Support for NAND flash controller on Amlogic's Meson SoCs" > >> + depends on ARCH_MESON || COMPILE_TEST > >> + select COMMON_CLK_REGMAP_MESON > >> + select MFD_SYSCON > >> + help > >> +Enables support for NAND controller on Amlogic's Meson SoCs. > >> + > >> endif # MTD_NAND > >> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > >> index 165b7ef9e9a1..cdf6162f38c3 100644 > >> --- a/drivers/mtd/nand/raw/Makefile > >> +++ b/drivers/mtd/nand/raw/Makefile > >> @@ -1,5 +1,7 @@ > >> # SPDX-License-Identifier: GPL-2.0 > >> > >> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson > > > > Please don't do that. If you need to expose common regs, put them > > in include/linux/soc/meson/. I'm also not sure why you need to access > > the clk regs directly. Why can't you expose the MMC/NAND clk as a clk > > provider whose driver would be placed in drivers/clk and which would use > > the mmc syscon. This way the same clk driver could be used for both > > MMC and NAND clk indifferently, and the NAND driver would be much > > simpler. > > [...] > > >> + > >> + return 0; > >> +} > >> + > >> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS]; > >> + > >> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = { > >> + .data = &(struct clk_regmap_mux_data){ > >> + .offset = SD_EMMC_CLOCK, > >> + .mask = 0x3, > >> + .shift = 6, > >> + }, > >> + .hw.init = &(struct clk_init_data) { > >> + .name = "sd_emmc_c_nand_clk_mux", > >> + .ops = &clk_regmap_mux_ops, > >> + .parent_names = sd_emmc_ext_clk0_parent_names, > >> + .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names), > >> + .flags = CLK_SET_RATE_PARENT, > >> + }, > >> +}; > >> + > >> +static struct clk_regmap sd_emmc_c_ext_clk0_div = { > >> + .data = &(struct clk_regmap_div_data){ > >> + .offset = SD_EMMC_CLOCK, > >> + .shift = 0, > >> + .width = 6, > >> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, > >> + }, > >> + .hw.init = &(struct clk_init_data) { > >> + .name = "sd_emmc_c_nand_clk_div", > >> + .ops = &clk_regmap_divider_ops, > >> + .parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" }, > >> + .num_parents = 1, > >> + .flags = CLK_SET_RATE_PARENT, > >> + }, > >> +}; > >> + > >> +static int meson_nfc_clk_init(struct meson_nfc *nfc) > >> +{ > >> + struct clk_regmap *mux = &sd_emmc_c_ext_clk0_sel; > >> + struct clk_regmap *div = &sd_emmc_c_ext_clk0_div; > >> + struct clk *clk; > >> + int i, ret; > >> + > >> + /* request core clock */ > >> + nfc->core_clk = devm_clk_get(nfc->dev, "core"); > >> + if (IS_ERR(nfc->core_clk)) { > >> + dev_err(nfc->dev, "failed to get core clk\n"); > >> + return PTR_ERR(nfc->core_clk); > >> + } > >> + > >> + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */ > >> + regmap_update_bits(nfc->reg_clk, 0, > >> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK, > >> + CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK); > >> + > >> + /* get the mux parents */ > >> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { > >> + char name[16]; > >> + > >> + snprintf(name, sizeof(name), "clkin%d", i); > >> + clk = devm_clk_get(nfc->dev, name); > >> + if (IS_ERR(clk)) { > >> + if (clk != ERR_PTR(-EPROBE_DEFER)) > >> + dev_err(nfc->dev, "Missing clock %s\n", name); > >> + return PTR_ERR(clk); > >> + } > >> + > >> + sd_emmc_ext_clk0_parent_names[i] = __clk_get_name(clk); > >> + } > >> + > >> + mux->map = nfc->reg_clk; > >> + clk = devm_clk_re
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Boris, Boris Brezillon writes: > Hi Yixun, > > On Wed, 13 Jun 2018 16:13:14 + > Yixun Lan wrote: > >> From: Liang Yang >> >> Add initial support for the Amlogic NAND flash controller which found >> in the Meson-GXBB/GXL/AXG SoCs. >> >> Singed-off-by: Liang Yang >> Signed-off-by: Yixun Lan >> --- >> drivers/mtd/nand/raw/Kconfig |8 + >> drivers/mtd/nand/raw/Makefile |3 + >> drivers/mtd/nand/raw/meson_nand.c | 1422 + >> 3 files changed, 1433 insertions(+) >> create mode 100644 drivers/mtd/nand/raw/meson_nand.c > > Can you run checkpatch.pl --strict and fix the coding style issues? > >> >> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig >> index 19a2b283fbbe..b3c17a3ca8f4 100644 >> --- a/drivers/mtd/nand/raw/Kconfig >> +++ b/drivers/mtd/nand/raw/Kconfig >> @@ -534,4 +534,12 @@ config MTD_NAND_MTK >>Enables support for NAND controller on MTK SoCs. >>This controller is found on mt27xx, mt81xx, mt65xx SoCs. >> >> +config MTD_NAND_MESON >> +tristate "Support for NAND flash controller on Amlogic's Meson SoCs" >> +depends on ARCH_MESON || COMPILE_TEST >> +select COMMON_CLK_REGMAP_MESON >> +select MFD_SYSCON >> +help >> + Enables support for NAND controller on Amlogic's Meson SoCs. >> + >> endif # MTD_NAND >> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile >> index 165b7ef9e9a1..cdf6162f38c3 100644 >> --- a/drivers/mtd/nand/raw/Makefile >> +++ b/drivers/mtd/nand/raw/Makefile >> @@ -1,5 +1,7 @@ >> # SPDX-License-Identifier: GPL-2.0 >> >> +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson > > Please don't do that. If you need to expose common regs, put them > in include/linux/soc/meson/. I'm also not sure why you need to access > the clk regs directly. Why can't you expose the MMC/NAND clk as a clk > provider whose driver would be placed in drivers/clk and which would use > the mmc syscon. This way the same clk driver could be used for both > MMC and NAND clk indifferently, and the NAND driver would be much > simpler. [...] >> + >> +return 0; >> +} >> + >> +static const char * sd_emmc_ext_clk0_parent_names[MUX_CLK_NUM_PARENTS]; >> + >> +static struct clk_regmap sd_emmc_c_ext_clk0_sel = { >> +.data = &(struct clk_regmap_mux_data){ >> +.offset = SD_EMMC_CLOCK, >> +.mask = 0x3, >> +.shift = 6, >> +}, >> +.hw.init = &(struct clk_init_data) { >> +.name = "sd_emmc_c_nand_clk_mux", >> +.ops = &clk_regmap_mux_ops, >> +.parent_names = sd_emmc_ext_clk0_parent_names, >> +.num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names), >> +.flags = CLK_SET_RATE_PARENT, >> +}, >> +}; >> + >> +static struct clk_regmap sd_emmc_c_ext_clk0_div = { >> +.data = &(struct clk_regmap_div_data){ >> +.offset = SD_EMMC_CLOCK, >> +.shift = 0, >> +.width = 6, >> +.flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, >> +}, >> +.hw.init = &(struct clk_init_data) { >> +.name = "sd_emmc_c_nand_clk_div", >> +.ops = &clk_regmap_divider_ops, >> +.parent_names = (const char *[]){ "sd_emmc_c_nand_clk_mux" }, >> +.num_parents = 1, >> +.flags = CLK_SET_RATE_PARENT, >> +}, >> +}; >> + >> +static int meson_nfc_clk_init(struct meson_nfc *nfc) >> +{ >> +struct clk_regmap *mux = &sd_emmc_c_ext_clk0_sel; >> +struct clk_regmap *div = &sd_emmc_c_ext_clk0_div; >> +struct clk *clk; >> +int i, ret; >> + >> +/* request core clock */ >> +nfc->core_clk = devm_clk_get(nfc->dev, "core"); >> +if (IS_ERR(nfc->core_clk)) { >> +dev_err(nfc->dev, "failed to get core clk\n"); >> +return PTR_ERR(nfc->core_clk); >> +} >> + >> +/* init SD_EMMC_CLOCK to sane defaults w/min clock rate */ >> +regmap_update_bits(nfc->reg_clk, 0, >> +CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK, >> +CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK); >> + >> +/* get the mux parents */ >> +for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { >> +char name[16]; >> + >> +snprintf(name, sizeof(name), "clkin%d", i); >> +clk = devm_clk_get(nfc->dev, name); >> +if (IS_ERR(clk)) { >> +if (clk != ERR_PTR(-EPROBE_DEFER)) >> +dev_err(nfc->dev, "Missing clock %s\n", name); >> +return PTR_ERR(clk); >> +} >> + >> +sd_emmc_ext_clk0_parent_names[i] = __clk_get_name(clk); >> +} >> + >> +mux->map = nfc->reg_clk; >> +clk = devm_clk_register(nfc->dev, &mux->hw); >> +if (WARN_ON(IS_ERR(clk))) >> +return PTR_ERR(clk); >> + >> +div->map = nfc->reg_clk; >> +nfc->device_clk = devm_clk_register(nfc->dev, &div->hw); >> +if (WARN_ON(IS_
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Yixun, On Wed, 13 Jun 2018 16:13:14 + Yixun Lan wrote: > From: Liang Yang > > Add initial support for the Amlogic NAND flash controller which found > in the Meson-GXBB/GXL/AXG SoCs. > > Singed-off-by: Liang Yang > Signed-off-by: Yixun Lan > --- > drivers/mtd/nand/raw/Kconfig |8 + > drivers/mtd/nand/raw/Makefile |3 + > drivers/mtd/nand/raw/meson_nand.c | 1422 + > 3 files changed, 1433 insertions(+) > create mode 100644 drivers/mtd/nand/raw/meson_nand.c Can you run checkpatch.pl --strict and fix the coding style issues? > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index 19a2b283fbbe..b3c17a3ca8f4 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -534,4 +534,12 @@ config MTD_NAND_MTK > Enables support for NAND controller on MTK SoCs. > This controller is found on mt27xx, mt81xx, mt65xx SoCs. > > +config MTD_NAND_MESON > + tristate "Support for NAND flash controller on Amlogic's Meson SoCs" > + depends on ARCH_MESON || COMPILE_TEST > + select COMMON_CLK_REGMAP_MESON > + select MFD_SYSCON > + help > + Enables support for NAND controller on Amlogic's Meson SoCs. > + > endif # MTD_NAND > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > index 165b7ef9e9a1..cdf6162f38c3 100644 > --- a/drivers/mtd/nand/raw/Makefile > +++ b/drivers/mtd/nand/raw/Makefile > @@ -1,5 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > > +ccflags-$(CONFIG_MTD_NAND_MESON) += -I$(srctree)/drivers/clk/meson Please don't do that. If you need to expose common regs, put them in include/linux/soc/meson/. I'm also not sure why you need to access the clk regs directly. Why can't you expose the MMC/NAND clk as a clk provider whose driver would be placed in drivers/clk and which would use the mmc syscon. This way the same clk driver could be used for both MMC and NAND clk indifferently, and the NAND driver would be much simpler. > + > obj-$(CONFIG_MTD_NAND) += nand.o > obj-$(CONFIG_MTD_NAND_ECC) += nand_ecc.o > obj-$(CONFIG_MTD_NAND_BCH) += nand_bch.o > @@ -56,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_HISI504) += > hisi504_nand.o > obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/ > obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o > obj-$(CONFIG_MTD_NAND_MTK) += mtk_ecc.o mtk_nand.o > +obj-$(CONFIG_MTD_NAND_MESON) += meson_nand.o > > nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o > nand-objs += nand_amd.o > diff --git a/drivers/mtd/nand/raw/meson_nand.c > b/drivers/mtd/nand/raw/meson_nand.c > new file mode 100644 > index ..28abc3684772 > --- /dev/null > +++ b/drivers/mtd/nand/raw/meson_nand.c > @@ -0,0 +1,1422 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Amlogic Meson Nand Flash Controller Driver > + * > + * Copyright (c) 2018 Amlogic, inc. > + * Author: Liang Yang > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "clk-regmap.h" > + > +#define NFC_REG_CMD 0x00 > +#define NFC_REG_CFG 0x04 > +#define NFC_REG_DADR 0x08 > +#define NFC_REG_IADR 0x0c > +#define NFC_REG_BUF 0x10 > +#define NFC_REG_INFO 0x14 > +#define NFC_REG_DC 0x18 > +#define NFC_REG_ADR 0x1c > +#define NFC_REG_DL 0x20 > +#define NFC_REG_DH 0x24 > +#define NFC_REG_CADR 0x28 > +#define NFC_REG_SADR 0x2c > +#define NFC_REG_PINS 0x30 > +#define NFC_REG_VER 0x38 > + Can you put the reg offsets next to their field definitions? > + > +#define NFC_CMD_DRD (0x8 << 14) > +#define NFC_CMD_IDLE (0xc << 14) > +#define NFC_CMD_DWR (0x4 << 14) > +#define NFC_CMD_CLE (0x5 << 14) > +#define NFC_CMD_ALE (0x6 << 14) > +#define NFC_CMD_ADL ((0 << 16) | (3 << 20)) > +#define NFC_CMD_ADH ((1 << 16) | (3 << 20)) > +#define NFC_CMD_AIL ((2 << 16) | (3 << 20)) > +#define NFC_CMD_AIH ((3 << 16) | (3 << 20)) > +#define NFC_CMD_SEED ((8 << 16) | (3 << 20)) > +#define NFC_CMD_M2N ((0 << 17) | (2 << 20)) > +#define NFC_CMD_N2M ((1 << 17) | (2 << 20)) > +#define NFC_CMD_RB (1 << 20) > +#define NFC_CMD_IO6 ((0xb << 10) | (1 << 18)) > + > +#define NFC_RB_USED (1 << 23) > +#define NFC_LARGE_PAGE (1 << 22) > +#define NFC_RW_OPS (2 << 20) > + > +#define NAND_TWB_TIME_CYCLE 10 > + > +#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages)\ > + ( \ > + (cmd_dir) | \ > + ((ran) << 19) | \ > +
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Liang, Thank you for the patch! Yet something to improve: [auto build test ERROR on mtd/nand/next] [also build test ERROR on v4.17 next-20180613] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Yixun-Lan/mtd-rawnand-meson-add-Amlogic-NAND-driver-support/20180613-161917 base: git://git.infradead.org/linux-mtd.git nand/next config: i386-allmodconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): >> drivers/mtd/nand/raw/meson_nand.c:21:10: fatal error: clk-regmap.h: No such >> file or directory #include "clk-regmap.h" ^~ compilation terminated. vim +21 drivers/mtd/nand/raw/meson_nand.c > 21 #include "clk-regmap.h" 22 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Hi Liang, Thank you for the patch! Yet something to improve: [auto build test ERROR on mtd/nand/next] [also build test ERROR on v4.17 next-20180613] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Yixun-Lan/mtd-rawnand-meson-add-Amlogic-NAND-driver-support/20180613-161917 base: git://git.infradead.org/linux-mtd.git nand/next config: sparc64-allyesconfig (attached as .config) compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=sparc64 All error/warnings (new ones prefixed by >>): In file included from drivers/mtd/nand/raw/meson_nand.c:21:0: >> drivers/clk/meson/clk-regmap.h:22:16: error: field 'hw' has incomplete type struct clk_hw hw; ^~ >> drivers/mtd/nand/raw/meson_nand.c:951:2: error: field name not in record or >> union initializer .hw.init = &(struct clk_init_data) { ^ drivers/mtd/nand/raw/meson_nand.c:951:2: note: (near initialization for 'sd_emmc_c_ext_clk0_sel') >> drivers/mtd/nand/raw/meson_nand.c:952:4: error: 'struct clk_init_data' has >> no member named 'name' .name = "sd_emmc_c_nand_clk_mux", ^~~~ >> drivers/mtd/nand/raw/meson_nand.c:952:11: warning: excess elements in struct >> initializer .name = "sd_emmc_c_nand_clk_mux", ^~~~ drivers/mtd/nand/raw/meson_nand.c:952:11: note: (near initialization for '(anonymous)') >> drivers/mtd/nand/raw/meson_nand.c:953:4: error: 'struct clk_init_data' has >> no member named 'ops' .ops = &clk_regmap_mux_ops, ^~~ drivers/mtd/nand/raw/meson_nand.c:953:10: warning: excess elements in struct initializer .ops = &clk_regmap_mux_ops, ^ drivers/mtd/nand/raw/meson_nand.c:953:10: note: (near initialization for '(anonymous)') >> drivers/mtd/nand/raw/meson_nand.c:954:4: error: 'struct clk_init_data' has >> no member named 'parent_names' .parent_names = sd_emmc_ext_clk0_parent_names, ^~~~ drivers/mtd/nand/raw/meson_nand.c:954:19: warning: excess elements in struct initializer .parent_names = sd_emmc_ext_clk0_parent_names, ^ drivers/mtd/nand/raw/meson_nand.c:954:19: note: (near initialization for '(anonymous)') >> drivers/mtd/nand/raw/meson_nand.c:955:4: error: 'struct clk_init_data' has >> no member named 'num_parents' .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names), ^~~ In file included from include/linux/list.h:9:0, from include/linux/kobject.h:19, from include/linux/device.h:16, from include/linux/platform_device.h:14, from drivers/mtd/nand/raw/meson_nand.c:9: >> include/linux/kernel.h:71:25: warning: excess elements in struct initializer #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) ^ >> drivers/mtd/nand/raw/meson_nand.c:955:18: note: in expansion of macro >> 'ARRAY_SIZE' .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names), ^~ include/linux/kernel.h:71:25: note: (near initialization for '(anonymous)') #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) ^ >> drivers/mtd/nand/raw/meson_nand.c:955:18: note: in expansion of macro >> 'ARRAY_SIZE' .num_parents = ARRAY_SIZE(sd_emmc_ext_clk0_parent_names), ^~ >> drivers/mtd/nand/raw/meson_nand.c:956:4: error: 'struct clk_init_data' has >> no member named 'flags' .flags = CLK_SET_RATE_PARENT, ^ >> drivers/mtd/nand/raw/meson_nand.c:956:12: error: 'CLK_SET_RATE_PARENT' >> undeclared here (not in a function); did you mean 'DL_STATE_DORMANT'? .flags = CLK_SET_RATE_PARENT, ^~~ DL_STATE_DORMANT drivers/mtd/nand/raw/meson_nand.c:956:12: warning: excess elements in struct initializer drivers/mtd/nand/raw/meson_nand.c:956:12: note: (near initialization for '(anonymous)') >> drivers/mtd/nand/raw/meson_nand.c:951:37: error: invalid use of undefined >> type 'struct clk_init_data' .hw.init = &(struct clk_init_data) { ^ >> drivers/mtd/nand/raw/meson_nand.c:965:12: error: 'CLK_DIVIDER_ROUND_CLOSEST' >> undeclared here (not in a function); did you mean 'DIV_ROUND_CLOSEST'? .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, ^ DIV_ROUND_CLOSEST >> drivers/mtd/nand/raw/meson_nand.c:965:40: error: 'CLK_DIVIDER_ONE_BASED' >> undeclared here (not in a functi