Re: [PATCH v2 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Hauke,

 Thank you very much for the review comments and your time...

On 18/4/2020 1:05 am, Hauke Mehrtens wrote:
> On 4/17/20 10:21 AM, Ramuthevar, Vadivel MuruganX wrote:
> > From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>
> Thanks for adding me in CC, I am using my private mail it took me too
> long to configure Outlook.
>
> I compared the register description of the LGM with the VRX200, the EBU
> and NAND block are looking very similar. I think the VRX200 also
> supports HW ECC and DMA, nobody implimented it in the upstream driver.
>
> I think replacing the old XWAY dirver with this one is a good approach.
Thank you for confirmation to replace the xway_nand.c since you are the 
owner for that driver.
> Then we can add feature flags to activate the extra features only on the
> SoCs which support them.
>
> On older SoCs the EBU (NAND) and the PCI (not express) IP core are
> sharing some parts like an endianess switch, this is causeing some more
> problems.
>
> > 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 <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>
> .....
>
> > +config MTD_NAND_INTEL_LGM
> > +	tristate "Support for NAND controller on Intel LGM SoC"
> > +	depends on X86
> Compile test should also work for other archs
Noted, will add it.
> > +	help
> > +	  Enables support for NAND Flash chips on Intel's LGM SoC.
> > +          NAND flash interfaced through the External Bus Unit.
> > +
> >   comment "Misc"
> .....
>
> > +
> > +#define LGM_CLC			0x000
> > +#define LGM_CLC_RST		0x00000000u
> > +
> > +#define LGM_NAND_ECC_OFFSET	0x008
> It is confusing that this is not a register but a different definition.
> Please move it somewheer else.
Noted, will move it.
> > +#define LGM_ADDR_SEL(n)		(0x20 + (n) * 4)
> > +#define LGM_ADDR_MASK		(5 << 4)
> > +#define LGM_ADDR_SEL_REGEN	0x1
> > +
> > +#define LGM_BUSCON(n)		(0x60 + (n) * 4)
> > +#define LGM_BUSCON_CMULT_V4	0x1
> > +#define LGM_BUSCON_RECOVC(n)	((n) << 2)
> > +#define LGM_BUSCON_HOLDC(n)	((n) << 4)
> > +#define LGM_BUSCON_WAITRDC(n)	((n) << 6)
> > +#define LGM_BUSCON_WAITWRC(n)	((n) << 8)
> > +#define LGM_BUSCON_BCGEN_CS	0x0
> > +#define LGM_BUSCON_SETUP_EN	BIT(22)
> > +#define LGM_BUSCON_ALEC		0xC000
> > +
> > +#define NAND_CON		0x0B0
> > +#define NAND_CON_NANDM_EN	BIT(0)
> > +#define NAND_CON_NANDM_DIS	0x0
> > +#define NAND_CON_CSMUX_E_EN	BIT(1)
> > +#define NAND_CON_ALE_P_LOW	BIT(2)
> > +#define NAND_CON_CLE_P_LOW	BIT(3)
> > +#define NAND_CON_CS_P_LOW	BIT(4)
> > +#define NAND_CON_SE_P_LOW	BIT(5)
> > +#define NAND_CON_WP_P_LOW	BIT(6)
> > +#define NAND_CON_PRE_P_LOW	BIT(7)
> > +#define NAND_CON_IN_CS_S(n)	((n) << 8)
> > +#define NAND_CON_OUT_CS_S(n)	((n) << 10)
> > +#define NAND_CON_LAT_EN_CS_P	((0x3D) << 18)
> > +
> > +#define NAND_WAIT		0x0B4
> > +#define NAND_WAIT_RDBY		BIT(0)
> > +#define NAND_WAIT_WR_C		BIT(3)
> The registers with the LGM_ prefix, NAND_CON and NAND_WAIT are from the
> EBU (EBU_N) register block. I would prefer if the have the same prefix.
> You should use a different prefix for the register definitions below
> this comment, which are from the NAND Controller (HSNAND).
Sure, will use as you have suggested
> > +#define NAND_CTL1		0x110
> > +#define NAND_CTL1_ADDR_3_SHIFT	24
> > +
> > +#define NAND_CTL2		0x114
> > +#define NAND_CTL2_ADDR_5_SHIFT	8
> > +#define NAND_CTL2_CYC_N_V5	(0x2 << 16)
> > +
> > +#define NAND_INT_MSK_CTL	0x124
> > +#define NAND_INT_MSK_CTL_WR_C	BIT(4)
> > +
> > +#define NAND_INT_STA		0x128
> > +#define NAND_INT_STA_WR_C	BIT(4)
> > +
> > +#define NAND_CTL		0x130
> > +#define NAND_CTL_MODE_ECC	0x1
> > +#define NAND_CTL_GO		BIT(2)
> > +#define NAND_CTL_CE_SEL_CS(n)	BIT(3 + (n))
> > +#define NAND_CTL_RW_READ	0x0
> > +#define NAND_CTL_RW_WRITE	BIT(10)
> > +#define NAND_CTL_ECC_OFF_V8TH	BIT(11)
> > +#define NAND_CTL_CKFF_EN	0x0
> > +#define NAND_CTL_MSG_EN		BIT(17)
> Till here I find the same registers you use also in the VRX200
> description. I haven't checked all the bits. Danube only has the
> registers from the EBU, I haven't checked the SoCs in between.
Okay, I don't have other SoC's datasheet to check, otherwise would have 
checked it.
> > +#define NAND_PARA0		0x13c
> > +#define NAND_PARA0_PAGE_V8192	0x3
> > +#define NAND_PARA0_PIB_V256	(0x3 << 4)
> > +#define NAND_PARA0_BYP_EN_NP	0x0
> > +#define NAND_PARA0_BYP_DEC_NP	0x0
> > +#define NAND_PARA0_TYPE_ONFI	BIT(18)
> > +#define NAND_PARA0_ADEP_EN	BIT(21)
> > +
> > +#define NAND_CMSG_0		0x150
> > +#define NAND_CMSG_1		0x154
> > +
> > +#define NAND_WRITE_CMD		(NAND_CON_CS_P_LOW | NAND_CON_CLE_P_LOW)
> > +#define NAND_WRITE_ADDR		(NAND_CON_CS_P_LOW | NAND_CON_ALE_P_LOW)
> > +#define NAND_WRITE_DATA		NAND_CON_CS_P_LOW
> > +#define NAND_READ_DATA		NAND_CON_CS_P_LOW
> > +
> > +#define NAND_CHIP_NO_SELECTION	-1
> > +#define NAND_CHIP_SELECTION	0x0
> > +
> ....
> > +static int lgm_nand_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct lgm_nand_host *lgm_host;
> > +	struct nand_chip *nand;
> > +	phys_addr_t nandaddr_pa;
> > +	struct mtd_info *mtd;
> > +	struct resource *res;
> > +	int ret;
> > +	u32 cs;
> > +
> > +	lgm_host = devm_kzalloc(dev, sizeof(*lgm_host), GFP_KERNEL);
> > +	if (!lgm_host)
> > +		return -ENOMEM;
> > +
> > +	lgm_host->dev = dev;
> > +	nand_controller_init(&lgm_host->controller);
> > +
> > +	lgm_host->lgm_va =
> > +	devm_platform_ioremap_resource_byname(pdev, "lgmnand");
> > +	if (IS_ERR(lgm_host->lgm_va))
> > +		return PTR_ERR(lgm_host->lgm_va);
> lgm_va is named ebu_n in the register description. Could you rename this
> variable to ebu. I think the _va postfix is not needed as it should be
> clear that this is a virtual addresss.
Noted, will rename it.
> > +	lgm_host->hsnand_va =
> > +	devm_platform_ioremap_resource_byname(pdev, "hsnand");
> > +	if (IS_ERR(lgm_host->hsnand_va))
> > +		return PTR_ERR(lgm_host->hsnand_va);
> > +
> > +	ret = device_property_read_u32(dev, "nand,cs", &cs);
> > +	if (ret) {
> > +		dev_err(dev, "failed to get chip select: %d\n", ret);
> > +		return ret;
> > +	}
> The cs should be validated, ifsome uses cs == 4 there will be a problem.
> LGM supports CS 0 to 3 like the VRX200, danube for example only 0 and 1.
Even LGM also supports 0 or 1
> > +	lgm_host->cs = cs;
> > +
> > +	lgm_host->cs_name = devm_kasprintf(dev, GFP_KERNEL, "nand_cs%d", cs);
> cs_name is only used locally in this function you can also use some
> memory on the stack to generate the name.
>
> > +	if (IS_ERR(lgm_host->cs_name)) {
> > +		ret = PTR_ERR(lgm_host->cs_name);
> > +		dev_err(dev, "failed to get chip select name: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);
> This will only support one chip select at a time on the SoC. It is not
> possible to configure the DTS in a way to talk to two NAND chips on
> different chip selects. I think this is an uncommon scenario and I do
> not know if other NAND drivers in Linux support this feature.

Yes, you are Right, LGM supports only one nand chip at a time.

> > +	lgm_host->nandaddr_va = res;
> > +	nandaddr_pa = res->start;
> > +	if (IS_ERR(lgm_host->nandaddr_va))
> > +		return PTR_ERR(lgm_host->nandaddr_va);
> > +
> > +	lgm_host->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(lgm_host->clk)) {
> > +		ret = PTR_ERR(lgm_host->clk);
> > +		dev_err(dev, "failed to get clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(lgm_host->clk);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +	lgm_host->clk_rate = clk_get_rate(lgm_host->clk);
> > +
> > +	ret = lgm_dma_init(dev, lgm_host);
> > +	if (ret)
> > +		goto disable_clk;
> > +
> > +	writel(lower_32_bits(nandaddr_pa) | LGM_ADDR_SEL_REGEN | LGM_ADDR_MASK,
> > +	       lgm_host->lgm_va + LGM_ADDR_SEL(cs));
> > +
> > +	writel(LGM_BUSCON_CMULT_V4 | LGM_BUSCON_RECOVC(1) |
> > +	       LGM_BUSCON_HOLDC(1) | LGM_BUSCON_WAITRDC(2) |
> > +	       LGM_BUSCON_WAITWRC(2) | LGM_BUSCON_BCGEN_CS | LGM_BUSCON_ALEC |
> > +	       LGM_BUSCON_SETUP_EN, lgm_host->lgm_va + LGM_BUSCON(cs));
> > +
> > +	/*
> > +	 * NAND physical address selection is based on the chip select
> > +	 * and written to ADDR_SEL register to get Memory Region Base address.
> > +	 * FPI Bus addresses are compared to this base address in conjunction
> > +	 * with the mask control. Driver need to program this field!
> > +	 * Address: 0x17400 if chip select is CS_0
> > +	 * Address: 0x17C00 if chip select is CS_1
> > +	 * Refer the Intel LGM SoC datasheet.
> Could you please name the section name in the document, to make it
> easier to find it, the number will probably change over time.

okay, Noted

Regards
Vadivel
> > +	 */
> > +	writel(0x17400051, lgm_host->lgm_va + LGM_ADDR_SEL(0));
> > +	writel(0x17C00051, lgm_host->lgm_va + LGM_ADDR_SEL(cs));
> Please do not use magic values here. For example you set here the
> LGM_ADDR_SEL_REGEN bit a mask and a base.
Noted, will update with MACRO
> > +	nand_set_flash_node(&lgm_host->chip, dev->of_node);
> > +	mtd = nand_to_mtd(&lgm_host->chip);
> > +	mtd->dev.parent = dev;
> > +	lgm_host->dev = dev;
> > +
> > +	platform_set_drvdata(pdev, lgm_host);
> > +	nand_set_controller_data(&lgm_host->chip, lgm_host);
> > +
> > +	nand = &lgm_host->chip;
> > +	nand->controller = &lgm_host->controller;
> > +	nand->controller->ops = &lgm_nand_controller_ops;
> > +
> > +	/* Scan to find existence of the device */
> > +	ret = nand_scan(&lgm_host->chip, 1);
> > +	if (ret)
> > +		goto exit_dma;
> > +
> > +	ret = mtd_device_register(mtd, NULL, 0);
> > +	if (ret)
> > +		goto clean_nand;
> > +
> > +	return 0;
> > +
> > +clean_nand:
> > +	nand_cleanup(&lgm_host->chip);
> > +exit_dma:
> > +	lgm_dma_exit(lgm_host);
> > +disable_clk:
> > +	clk_disable_unprepare(lgm_host->clk);
> > +
> > +	return ret;
> > +}
> Hauke