Re: [PATCH v5 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris, Thank you very much for the review comments and your time... On 7/5/2020 2:48 pm, Boris Brezillon wrote: > On Thu, 7 May 2020 14:38:52 +0800 > "Ramuthevar, Vadivel MuruganX" > wrote: > >> Hi Boris, >> >>Thank you very much for the review comments and your time... >> >> On 7/5/2020 2:27 pm, Boris Brezillon wrote: >>> On Thu, 7 May 2020 14:13:42 +0800 >>> "Ramuthevar, Vadivel MuruganX" >>> wrote: >>> Hi Boris, Thank you very much for the review comments and your time... On 7/5/2020 1:28 pm, Boris Brezillon wrote: > On Thu, 7 May 2020 08:15:37 +0800 > "Ramuthevar,Vadivel MuruganX" > wrote: > >> + reg = readl(ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); >> + writel(reg | EBU_ADDR_MASK(5) | EBU_ADDR_SEL_REGEN, >> + ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); > Seriously, did you really think I would not notice what you're doing > here? Yes , I know that you have very good understanding about this. You're reading the previous value which either contains a default > mapping or has the mapping set by the bootloader, and write it back to > the register along with a new mask and the REGEN bit set (which > BTW is wrong since you don't mask out other fields before updating > them). There is no other field get overwritten This confirms that this Core -> FPI address translation exists > and has to be set properly, so please stop lying about that. Sorry, there is no SW translation, as I have mentioned that it's optional only, for safer side , reading and writing the default values. >>> Then write EBU_ADDR_SEL_REGEN and we'll if see that works. I suspect it >>> won't. >> You mean, without reading just writing EBU_ADDR_SEL_REGEN bit alone in >> EBU_ADDR_SELx , as you said it won't work because it overwrites 0x174 >> with 0x0 values so BASE is lost. > Which confirms that this mapping has to be defined. Sure, Noted. >> either we can leave it or read & write with ORed | EBU_ADDR_SEL_REGEN > None of this is acceptable IMO. You have to build the value based on the > address translation described in the DT. Why are you so reluctant to > this approach? Agreed!, will derive the values(0x174/0x17C) to be written into these registers based on the chip select (CS0/CS1) Address_sel0_register: 0xE0F0_0020 Address_sel1_register: 0xE0F0_0024 Bits : 31...12|11...8| 7..4 |3..2| 1 | 0 flags: BASE |--| MASK | -- | MRME | REGEN BASE : 0x17400 /0x17C00 to be written into 31:12 based on the chip selection MASK: 5: bits 26:22 to included address comparison MRME: Memory Region Memory Enable REGEN: Memory Region Access Enable As you have suggested to get the above base values from DT and update in driver, will do that. Thanks! Regards Vadivel >> Please correct me if anything is wrong, Thanks! >>> The memory region to enabled that's my concern so written the same register values. >>> I don't buy that, sorry. >>> This will not be impact other fields, so please see below for reference The EBU Address Select Registers EBU_ADDR_SEL_0 to EBU_ADDSEL3 establish and control memory regions for external accesses. Reset Value: 1741H >>> See, as suspected the reset value is exactly what you expect. >> Yes , that's the reason said being optional. > Then it's not optional. It just works because you use the default > va
Re: [PATCH v5 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Thu, 7 May 2020 14:38:52 +0800 "Ramuthevar, Vadivel MuruganX" wrote: > Hi Boris, > >Thank you very much for the review comments and your time... > > On 7/5/2020 2:27 pm, Boris Brezillon wrote: > > On Thu, 7 May 2020 14:13:42 +0800 > > "Ramuthevar, Vadivel MuruganX" > > wrote: > > > >> Hi Boris, > >> > >> Thank you very much for the review comments and your time... > >> > >> On 7/5/2020 1:28 pm, Boris Brezillon wrote: > >>> On Thu, 7 May 2020 08:15:37 +0800 > >>> "Ramuthevar,Vadivel MuruganX" > >>> wrote: > >>> > +reg = readl(ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); > +writel(reg | EBU_ADDR_MASK(5) | EBU_ADDR_SEL_REGEN, > + ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); > >>> > >>> Seriously, did you really think I would not notice what you're doing > >>> here? > >> Yes , I know that you have very good understanding about this. > >>You're reading the previous value which either contains a default > >>> mapping or has the mapping set by the bootloader, and write it back to > >>> the register along with a new mask and the REGEN bit set (which > >>> BTW is wrong since you don't mask out other fields before updating > >>> them). > >> There is no other field get overwritten > >>This confirms that this Core -> FPI address translation exists > >>> and has to be set properly, so please stop lying about that. > >> > >> Sorry, there is no SW translation, as I have mentioned that it's > >> optional only, for safer side , reading and writing the default values. > > > > Then write EBU_ADDR_SEL_REGEN and we'll if see that works. I suspect it > > won't. > > You mean, without reading just writing EBU_ADDR_SEL_REGEN bit alone in > EBU_ADDR_SELx , as you said it won't work because it overwrites 0x174 > with 0x0 values so BASE is lost. Which confirms that this mapping has to be defined. > either we can leave it or read & write with ORed | EBU_ADDR_SEL_REGEN None of this is acceptable IMO. You have to build the value based on the address translation described in the DT. Why are you so reluctant to this approach? > > Please correct me if anything is wrong, Thanks! > > > >> The memory region to enabled that's my concern so written the same > >> register values. > > > > I don't buy that, sorry. > > > >> > >> This will not be impact other fields, so please see below for reference > >> > >> The EBU Address Select Registers EBU_ADDR_SEL_0 to EBU_ADDSEL3 establish > >> and control memory regions for external accesses. > >> > >> Reset Value: 1741H > > > > See, as suspected the reset value is exactly what you expect. > > Yes , that's the reason said being optional. Then it's not optional. It just works because you use the default value.
Re: [PATCH v5 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris, Thank you very much for the review comments and your time... On 7/5/2020 2:27 pm, Boris Brezillon wrote: On Thu, 7 May 2020 14:13:42 +0800 "Ramuthevar, Vadivel MuruganX" wrote: Hi Boris, Thank you very much for the review comments and your time... On 7/5/2020 1:28 pm, Boris Brezillon wrote: On Thu, 7 May 2020 08:15:37 +0800 "Ramuthevar,Vadivel MuruganX" wrote: + reg = readl(ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); + writel(reg | EBU_ADDR_MASK(5) | EBU_ADDR_SEL_REGEN, + ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); Seriously, did you really think I would not notice what you're doing here? Yes , I know that you have very good understanding about this. You're reading the previous value which either contains a default mapping or has the mapping set by the bootloader, and write it back to the register along with a new mask and the REGEN bit set (which BTW is wrong since you don't mask out other fields before updating them). There is no other field get overwritten This confirms that this Core -> FPI address translation exists and has to be set properly, so please stop lying about that. Sorry, there is no SW translation, as I have mentioned that it's optional only, for safer side , reading and writing the default values. Then write EBU_ADDR_SEL_REGEN and we'll if see that works. I suspect it won't. You mean, without reading just writing EBU_ADDR_SEL_REGEN bit alone in EBU_ADDR_SELx , as you said it won't work because it overwrites 0x174 with 0x0 values so BASE is lost. either we can leave it or read & write with ORed | EBU_ADDR_SEL_REGEN Please correct me if anything is wrong, Thanks! The memory region to enabled that's my concern so written the same register values. I don't buy that, sorry. This will not be impact other fields, so please see below for reference The EBU Address Select Registers EBU_ADDR_SEL_0 to EBU_ADDSEL3 establish and control memory regions for external accesses. Reset Value: 1741H See, as suspected the reset value is exactly what you expect. Yes , that's the reason said being optional. Regards Vadivel
Re: [PATCH v5 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Thu, 7 May 2020 14:13:42 +0800 "Ramuthevar, Vadivel MuruganX" wrote: > Hi Boris, > > Thank you very much for the review comments and your time... > > On 7/5/2020 1:28 pm, Boris Brezillon wrote: > > On Thu, 7 May 2020 08:15:37 +0800 > > "Ramuthevar,Vadivel MuruganX" > > wrote: > > > >> + reg = readl(ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); > >> + writel(reg | EBU_ADDR_MASK(5) | EBU_ADDR_SEL_REGEN, > >> + ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); > > > > Seriously, did you really think I would not notice what you're doing > > here? > Yes , I know that you have very good understanding about this. > You're reading the previous value which either contains a default > > mapping or has the mapping set by the bootloader, and write it back to > > the register along with a new mask and the REGEN bit set (which > > BTW is wrong since you don't mask out other fields before updating > > them). > There is no other field get overwritten > This confirms that this Core -> FPI address translation exists > > and has to be set properly, so please stop lying about that. > > Sorry, there is no SW translation, as I have mentioned that it's > optional only, for safer side , reading and writing the default values. Then write EBU_ADDR_SEL_REGEN and we'll if see that works. I suspect it won't. > The memory region to enabled that's my concern so written the same > register values. I don't buy that, sorry. > > This will not be impact other fields, so please see below for reference > > The EBU Address Select Registers EBU_ADDR_SEL_0 to EBU_ADDSEL3 establish > and control memory regions for external accesses. > > Reset Value: 1741H See, as suspected the reset value is exactly what you expect.
Re: [PATCH v5 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris, Thank you very much for the review comments and your time... On 7/5/2020 1:28 pm, Boris Brezillon wrote: On Thu, 7 May 2020 08:15:37 +0800 "Ramuthevar,Vadivel MuruganX" wrote: + reg = readl(ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); + writel(reg | EBU_ADDR_MASK(5) | EBU_ADDR_SEL_REGEN, + ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); Seriously, did you really think I would not notice what you're doing here? Yes , I know that you have very good understanding about this. You're reading the previous value which either contains a default mapping or has the mapping set by the bootloader, and write it back to the register along with a new mask and the REGEN bit set (which BTW is wrong since you don't mask out other fields before updating them). There is no other field get overwritten This confirms that this Core -> FPI address translation exists and has to be set properly, so please stop lying about that. Sorry, there is no SW translation, as I have mentioned that it's optional only, for safer side , reading and writing the default values. The memory region to enabled that's my concern so written the same register values. This will not be impact other fields, so please see below for reference The EBU Address Select Registers EBU_ADDR_SEL_0 to EBU_ADDSEL3 establish and control memory regions for external accesses. Reset Value: 1741H Regards Vadivel
Re: [PATCH v5 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi, On 7/5/2020 8:22 am, Randy Dunlap wrote: On 5/6/20 5:15 PM, Ramuthevar,Vadivel MuruganX wrote: diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig index a80a46bb5b8b..a026bec28f39 100644 --- a/drivers/mtd/nand/raw/Kconfig +++ b/drivers/mtd/nand/raw/Kconfig @@ -457,6 +457,14 @@ config MTD_NAND_CADENCE Enable the driver for NAND flash on platforms using a Cadence NAND controller. +config MTD_NAND_INTEL_LGM + tristate "Support for NAND controller on Intel LGM SoC" + depends on OF || COMPILE_TEST + depends on HAS_IOMEM + help + Enables support for NAND Flash chips on Intel's LGM SoC. + NAND flash interfaced through the External Bus Unit. Please use one tab + 2 spaces for indentation in the line above. Thank you for the review comments, will update in the next patch-set. Regards Vadivel + comment "Misc" config MTD_SM_COMMON
Re: [PATCH v5 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Thu, 7 May 2020 08:15:37 +0800 "Ramuthevar,Vadivel MuruganX" wrote: > + reg = readl(ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); > + writel(reg | EBU_ADDR_MASK(5) | EBU_ADDR_SEL_REGEN, > +ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num)); Seriously, did you really think I would not notice what you're doing here? You're reading the previous value which either contains a default mapping or has the mapping set by the bootloader, and write it back to the register along with a new mask and the REGEN bit set (which BTW is wrong since you don't mask out other fields before updating them). This confirms that this Core -> FPI address translation exists and has to be set properly, so please stop lying about that.
Re: [PATCH v5 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On 5/6/20 5:15 PM, Ramuthevar,Vadivel MuruganX wrote: > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index a80a46bb5b8b..a026bec28f39 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -457,6 +457,14 @@ config MTD_NAND_CADENCE > Enable the driver for NAND flash on platforms using a Cadence NAND > controller. > > +config MTD_NAND_INTEL_LGM > + tristate "Support for NAND controller on Intel LGM SoC" > + depends on OF || COMPILE_TEST > + depends on HAS_IOMEM > + help > + Enables support for NAND Flash chips on Intel's LGM SoC. > + NAND flash interfaced through the External Bus Unit. Please use one tab + 2 spaces for indentation in the line above. > + > comment "Misc" > > config MTD_SM_COMMON -- ~Randy
[PATCH v5 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
From: Ramuthevar Vadivel Murugan This patch adds the new IP of Nand Flash Controller(NFC) support on Intel's Lightning Mountain(LGM) SoC. DMA is used for burst data transfer operation, also DMA HW supports aligned 32bit memory address and aligned data access by default. DMA burst of 8 supported. Data register used to support the read/write operation from/to device. NAND controller driver implements ->exec_op() to replace legacy hooks, these specific call-back method to execute NAND operations. Signed-off-by: Ramuthevar Vadivel Murugan --- drivers/mtd/nand/raw/Kconfig | 8 + drivers/mtd/nand/raw/Makefile| 1 + drivers/mtd/nand/raw/intel-nand-controller.c | 741 +++ 3 files changed, 750 insertions(+) create mode 100644 drivers/mtd/nand/raw/intel-nand-controller.c diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig index a80a46bb5b8b..a026bec28f39 100644 --- a/drivers/mtd/nand/raw/Kconfig +++ b/drivers/mtd/nand/raw/Kconfig @@ -457,6 +457,14 @@ config MTD_NAND_CADENCE Enable the driver for NAND flash on platforms using a Cadence NAND controller. +config MTD_NAND_INTEL_LGM + tristate "Support for NAND controller on Intel LGM SoC" + depends on OF || COMPILE_TEST + depends on HAS_IOMEM + help + Enables support for NAND Flash chips on Intel's LGM SoC. + NAND flash interfaced through the External Bus Unit. + comment "Misc" config MTD_SM_COMMON diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile index 2d136b158fb7..bfc8fe4d2cb0 100644 --- a/drivers/mtd/nand/raw/Makefile +++ b/drivers/mtd/nand/raw/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_TEGRA) += tegra_nand.o obj-$(CONFIG_MTD_NAND_STM32_FMC2) += stm32_fmc2_nand.o obj-$(CONFIG_MTD_NAND_MESON) += meson_nand.o obj-$(CONFIG_MTD_NAND_CADENCE) += cadence-nand-controller.o +obj-$(CONFIG_MTD_NAND_INTEL_LGM) += intel-nand-controller.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/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c new file mode 100644 index ..e9720271ce2a --- /dev/null +++ b/drivers/mtd/nand/raw/intel-nand-controller.c @@ -0,0 +1,741 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Copyright (c) 2020 Intel Corporation. */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define EBU_CLC0x000 +#define EBU_CLC_RST0xu + +#define EBU_ADDR_SEL(n)(0x20 + (n) * 4) +/* 5 bits 26:22 included for comparison in the ADDR_SELx */ +#define EBU_ADDR_MASK(x) ((x) << 4) +#define EBU_ADDR_SEL_REGEN 0x1 + +#define EBU_BUSCON(n) (0x60 + (n) * 4) +#define EBU_BUSCON_CMULT_V40x1 +#define EBU_BUSCON_RECOVC(n) ((n) << 2) +#define EBU_BUSCON_HOLDC(n)((n) << 4) +#define EBU_BUSCON_WAITRDC(n) ((n) << 6) +#define EBU_BUSCON_WAITWRC(n) ((n) << 8) +#define EBU_BUSCON_BCGEN_CS0x0 +#define EBU_BUSCON_SETUP_ENBIT(22) +#define EBU_BUSCON_ALEC0xC000 + +#define EBU_CON0x0B0 +#define EBU_CON_NANDM_EN BIT(0) +#define EBU_CON_NANDM_DIS 0x0 +#define EBU_CON_CSMUX_E_EN BIT(1) +#define EBU_CON_ALE_P_LOW BIT(2) +#define EBU_CON_CLE_P_LOW BIT(3) +#define EBU_CON_CS_P_LOW BIT(4) +#define EBU_CON_SE_P_LOW BIT(5) +#define EBU_CON_WP_P_LOW BIT(6) +#define EBU_CON_PRE_P_LOW BIT(7) +#define EBU_CON_IN_CS_S(n) ((n) << 8) +#define EBU_CON_OUT_CS_S(n)((n) << 10) +#define EBU_CON_LAT_EN_CS_P((0x3D) << 18) + +#define EBU_WAIT 0x0B4 +#define EBU_WAIT_RDBY BIT(0) +#define EBU_WAIT_WR_C BIT(3) + +#define HSNAND_CTL10x110 +#define HSNAND_CTL1_ADDR_SHIFT 24 + +#define HSNAND_CTL20x114 +#define HSNAND_CTL2_ADDR_SHIFT 8 +#define HSNAND_CTL2_CYC_N_V5 (0x2 << 16) + +#define HSNAND_INT_MSK_CTL 0x124 +#define HSNAND_INT_MSK_CTL_WR_CBIT(4) + +#define HSNAND_INT_STA 0x128 +#define HSNAND_INT_STA_WR_CBIT(4) + +#define HSNAND_CTL 0x130 +#define HSNAND_CTL_ENABLE_ECC BIT(0) +#define HSNAND_CTL_GO BIT(2) +#define HSNAND_CTL_CE_SEL_CS(n)BIT(3 + (n)) +#define HSNAND_CTL_RW_READ 0x0 +#define HSNAND_CTL_RW_WRITEBIT(10) +#define HSNAND_CTL_ECC_OFF_V8THBIT(11) +#define HSNAND_CTL_CKFF_EN 0x0 +#define HSNAND_CTL_MSG_EN BIT(17) + +#define HSNAND_PARA0 0x13c +#define HSNAND_PARA0_PAGE_V81920x3 +#define HSNAND_PARA0_PIB_V256 (0x3 << 4) +#define HSNAND_PARA0_BYP_EN_NP 0x0 +#define HSNAND_PARA0_BYP_DEC_NP0x0 +#define HSNAND_PARA0_TYPE_ONFI BIT(18) +#define
