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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Boris,

    Thank you  very much for review comments and your time...
On 25/4/2020 12:36 am, Boris Brezillon wrote:
> On Fri, 24 Apr 2020 00:21:13 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
>
>> +
>> +#define EBU_CLC			0x000
>> +#define EBU_CLC_RST		0x00000000u
>> +
>> +#define	EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
> 	   ^ Please replace those tabs by a single space.
Noted.
>> +#define	EBU_ADDR_MASK		(5 << 4)
>> +#define	EBU_ADDR_SEL_REGEN	0x1
>> +
>> +#define EBU_BUSCON(n)		(0x60 + (n) * 4)
>> +#define EBU_BUSCON_CMULT_V4	0x1
>> +#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_CS	0x0
>> +#define EBU_BUSCON_SETUP_EN	BIT(22)
>> +#define EBU_BUSCON_ALEC		0xC000
>> +
>> +#define EBU_CON			0x0B0
>> +#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	EBU_MEM_BASE_CS_0	0x17400
>> +#define	EBU_MEM_BASE_CS_1	0x17C00
> If I refer to v2, that's actually 0x17400000 and 0x17C00000.
> And I suspect those addresses come from the physical addresses of the
> EBU range attached to those CS lines, which can be extracted
> from the resource passed to the driver. But maybe I'm wrong.
Agreed!, will update.
>> +#define	EBU_MEM_OFFSET		0x051
> Sorry but that's too vague. What is this mask/offset encoding?
it's not offset, wrongly mentioned name as offset, bit field values.
will correct it.
>> +
>> +#define HSNAND_CTL1		0x110
>> +#define HSNAND_CTL1_ADDR_SHIFT	24
>> +
>> +#define HSNAND_CTL2		0x114
>> +#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_C	BIT(4)
>> +
>> +#define HSNAND_INT_STA		0x128
>> +#define HSNAND_INT_STA_WR_C	BIT(4)
>> +
>> +#define HSNAND_CTL		0x130
>> +#define HSNAND_CTL_MODE_ECC	0x1
> #define HSNAND_CTL_ENABLE_ECC	BIT(0)
Noted, will update
>> +#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_WRITE	BIT(10)
>> +#define HSNAND_CTL_ECC_OFF_V8TH	BIT(11)
>> +#define HSNAND_CTL_CKFF_EN	0x0
>> +#define HSNAND_CTL_MSG_EN	BIT(17)
>> +
>> +#define NAND_PARA0		0x13c
> Why are some of those registers prefixed NAND and others HSNAND? I
> guess HS stands for High-Speed.
To differentiate the common HSNAND registers between MIPS and X86 based SoC's
>> +#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_ALE_OFFS		BIT(2)
>> +#define NAND_CLE_OFFS		BIT(3)
>> +#define NAND_CS_OFFS		BIT(4)
>> +
>> +#define NAND_WRITE_CMD		(NAND_CS_OFFS | NAND_CLE_OFFS)
>> +#define NAND_WRITE_ADDR		(NAND_CS_OFFS | NAND_ALE_OFFS)
> Those macros are no longer used, you can drop them.
Noted, will drop them
>> +#define NAND_WRITE_DATA		EBU_CON_CS_P_LOW
>> +#define NAND_READ_DATA		EBU_CON_CS_P_LOW
> And those should not be used, please use NAND_CS_OFFS instead.
Sure, Noted
>> +
>> +#define NAND_ECC_OFFSET		0x008
>> +
>> +struct ebu_nand_controller {
>> +	struct nand_controller	controller;
> Can you drop the tabs and use one space between the type and field name?
Noted
>> +	struct nand_chip	chip;
> Can you add a comment explaining that this should be reworked if
> someone needs to support more than one chip.
Sure, will add comments
>> +	void __iomem		*ebu_addr;
>> +	void __iomem		*nand_addr;
> Can we get rid of those _addr and rename those fields so they describe
> more precisely what the MMIO range is about:
>
> 	void __iomem *ebu;
> 	void __iomem *hsnand;
>
Noted
>> +	void __iomem		*chip_addr;
> This field should be part of a specialized nand_chip object (see what's
> done here [1]). More generally, you should move anything that's
> chip-specific to this driver-specific nand_chip object.
I supposed to add like nand_cs, will change it.
>> +	struct clk		*clk;
>> +	unsigned long		clk_rate;
>> +	u32			cs;
>> +	u32			nd_para0;
> The cs and nd_para0 fields should be attached to the ebu_nand_chip
> object too (see [1]). While you move that, please add a comment saying
> that you currently only support single-CE chips.
Yes, you are correct, will do that
>> +	struct device		*dev;
>> +	struct dma_chan		*dma_tx;
>> +	struct dma_chan		*dma_rx;
>> +	struct completion	dma_access_complete;
> Maybe
>
> 	struct {
> 		struct dma_chan *tx;
> 		struct dma_chan *rx;
> 		struct completion complete;
> 	} dma;
Ok, Noted
>> +	const char *cs_name;
> You no longer use this field.
Noted, will drop it.
>> +};
>> +
>> +static inline struct ebu_nand_controller *nand_to_ebu(struct nand_chip *chip)
>> +{
>> +	return container_of(chip, struct ebu_nand_controller, chip);
>> +}
>> +
>> +static u8 ebu_nand_readb(struct nand_chip *chip, unsigned int op)
>> +{
>> +	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
>> +	void __iomem *nand_wait = ebu_host->ebu_addr + EBU_WAIT;
>> +	u32 stat;
>> +	int ret;
>> +	u8 val;
>> +
>> +	val = readb(ebu_host->chip_addr + op);
> No need for op here, just pass NAND_CS_OFFS directly, and you can drop
> the op argument.
Noted, will update.
>> +
>> +	ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
>> +				 20, 1000);
>> +	if (ret)
>> +		dev_warn(ebu_host->dev,
>> +			 "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
>> +			 nand_wait, readl(nand_wait));
> Maybe dev_warn_ratelimited().
Not required, might be used for network drivers.
>> +
>> +	return val;
>> +}
>> +
>> +static void ebu_nand_writeb(struct nand_chip *chip, int op, u8 value)
> 							^u32 offset,
>
>> +{
>> +	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
>> +	void __iomem *nand_wait = ebu_host->ebu_addr + EBU_WAIT;
>> +	u32 stat;
>> +	int ret;
>> +
>> +	writeb(value, ebu_host->chip_addr + op);
>> +
>> +	ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
>> +				 20, 1000);
>> +	if (ret)
>> +		dev_warn(ebu_host->dev,
>> +			 "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
>> +			 nand_wait, readl(nand_wait));
>> +}
>> +
>> +static void ebu_read_buf(struct nand_chip *chip, u_char *buf, int len)
> 						      ^void *	   ^unsigned int
>
> Please update all of those.
Sure, will update all.
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < len; i++)
>> +		buf[i] = ebu_nand_readb(chip, NAND_READ_DATA);
>> +}
>> +
>> +static void ebu_write_buf(struct nand_chip *chip, const u_char *buf, int len)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < len; i++)
>> +		ebu_nand_writeb(chip, NAND_WRITE_DATA, buf[i]);
> 				      ^NAND_CS_OFFS
Noted.
>> +}
>> +
>> +static void ebu_unselect_chip(struct nand_chip *chip)
>> +{
>> +	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
>> +	void __iomem *nand_con = ebu_host->ebu_addr + EBU_CON;
>> +	int val;
> 	^u32
>
>> +
>> +	val = readl(nand_con);
>> +	writel(val & ~EBU_CON_NANDM_EN, nand_con);
> And I suspect you could just do:
>
> 	writel(0, ebu_host->ebu + EBU_CON);
>
> here.
>
> BTW, maybe we don't have to unselect the chip and can instead do this
> 'disable NAND module' in a runtime PM hook when the NAND has been
> unused for some time, but let's keep that for later.
Sure, Noted.
>> +}
> ...
>
>> +static int ebu_nand_exec_op(struct nand_chip *chip,
>> +			    const struct nand_operation *op, bool check_only)
>> +{
>> +	struct ebu_nand_controller *ctrl = nand_to_ebu(chip);
>> +	const struct nand_op_instr *instr = NULL;
>> +	unsigned int op_id;
>> +	int i, time_out, ret = 0;
>> +	u32 stat;
>> +
>> +	ebu_select_chip(chip);
>> +
>> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
>> +		instr = &op->instrs[op_id];
>> +
>> +		switch (instr->type) {
>> +		case NAND_OP_CMD_INSTR:
>> +			ebu_nand_writeb(chip, NAND_CLE_OFFS,
> Hm, are you sure you shouldn't add NAND_CS_OFFS here? I'd expect
> the CS to be asserted when the CLE/ALE pin is. Or maybe NAND_CS_OFFS
> is not about CS pin assertion/de-assertion, in which case this should
> be documented (and the name should be changed accordingly).
before doing the read/write asserting the chip by chip_select function, let me double check
as you said, will update accordingly
>> +					instr->ctx.cmd.opcode);
>> +			break;
>> +
>> +		case NAND_OP_ADDR_INSTR:
>> +			for (i = 0; i < instr->ctx.addr.naddrs; i++)
>> +				ebu_nand_writeb(chip, NAND_ALE_OFFS,
>> +						instr->ctx.addr.addrs[i]);
>> +			break;
>> +
>> +		case NAND_OP_DATA_IN_INSTR:
>> +			ebu_read_buf(chip, instr->ctx.data.buf.in,
>> +				     instr->ctx.data.len);
>> +			break;
>> +
>> +		case NAND_OP_DATA_OUT_INSTR:
>> +			ebu_write_buf(chip, instr->ctx.data.buf.out,
>> +				      instr->ctx.data.len);
>> +			break;
>> +
>> +		case NAND_OP_WAITRDY_INSTR:
>> +			time_out = instr->ctx.waitrdy.timeout_ms * 1000;
>> +			ret = readl_poll_timeout(ctrl->ebu_addr + EBU_WAIT,
>> +						 stat, stat & EBU_WAIT_RDBY,
>> +						 20, time_out);
>> +			break;
>> +		}
>> +	}
> Yay! I really like this new version of exec_op().
Thank you Boris!
>> +
>> +	ebu_unselect_chip(chip);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct nand_controller_ops ebu_nand_controller_ops = {
>> +	.attach_chip = ebu_nand_attach_chip,
>> +	.exec_op = ebu_nand_exec_op,
>> +};
>> +
>> +static void ebu_dma_cleanup(struct ebu_nand_controller *ebu_host)
>> +{
>> +	if (ebu_host->dma_rx) {
>> +		dma_release_channel(ebu_host->dma_rx);
>> +		ebu_host->dma_rx = NULL;
> You don't need to reset the value here.
Noted.
>> +	}
>> +
>> +	if (ebu_host->dma_tx) {
>> +		dma_release_channel(ebu_host->dma_tx);
>> +		ebu_host->dma_tx = NULL;
>> +	}
>> +}
>> +
>> +static int ebu_nand_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct ebu_nand_controller *ebu_host;
>> +	struct nand_chip *nand;
>> +	phys_addr_t nandaddr_pa;
>> +	struct mtd_info *mtd;
>> +	struct resource *res;
>> +	int ret;
>> +	u32 cs;
>> +
>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
>> +	if (!ebu_host)
>> +		return -ENOMEM;
>> +
>> +	ebu_host->dev = dev;
>> +	nand_controller_init(&ebu_host->controller);
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
>> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(ebu_host->ebu_addr))
>> +		return PTR_ERR(ebu_host->ebu_addr);
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
>> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(ebu_host->nand_addr))
>> +		return PTR_ERR(ebu_host->nand_addr);
>> +
>> +	ret = device_property_read_u32(dev, "nand,cs", &cs);
> CS ids should be encoded in the reg property (see [1]).
Ok, will check.
>> +	if (ret) {
>> +		dev_err(dev, "failed to get chip select: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ebu_host->cs = cs;
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand_cs0");
> 	resname = devm_kasprintf("nand-cs%d", cs);
> 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, resname);
I have kept same-way in my earlier patches , will update.
>> +	ebu_host->chip_addr = devm_ioremap_resource(&pdev->dev, res);
>> +	nandaddr_pa = res->start;
>> +	if (IS_ERR(ebu_host->chip_addr))
>> +		return PTR_ERR(ebu_host->chip_addr);
>> +
>> +	ebu_host->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(ebu_host->clk)) {
>> +		ret = PTR_ERR(ebu_host->clk);
>> +		dev_err(dev, "failed to get clock: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(ebu_host->clk);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable clock: %d\n", ret);
>> +		return ret;
>> +	}
>> +	ebu_host->clk_rate = clk_get_rate(ebu_host->clk);
>> +
>> +	ebu_host->dma_tx = dma_request_chan(dev, "tx");
>> +	if (IS_ERR(ebu_host->dma_tx)) {
>> +		ret = PTR_ERR(ebu_host->dma_tx);
>> +		dev_err(dev, "DMA tx channel request fail!.\n");
>> +		goto exit_dma;
>> +	}
>> +
>> +	ebu_host->dma_rx = dma_request_chan(dev, "rx");
>> +	if (IS_ERR(ebu_host->dma_rx)) {
>> +		ret = PTR_ERR(ebu_host->dma_rx);
>> +		dev_err(dev, "DMA tx channel request fail!.\n");
>> +		goto exit_dma;
>> +	}
>> +
>> + writel(lower_32_bits(nandaddr_pa) | EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
>> +	       ebu_host->ebu_addr + EBU_ADDR_SEL(cs));
>> +
>> +	writel(EBU_BUSCON_CMULT_V4 | EBU_BUSCON_RECOVC(1) |
>> +	       EBU_BUSCON_HOLDC(1) | EBU_BUSCON_WAITRDC(2) |
>> +	       EBU_BUSCON_WAITWRC(2) | EBU_BUSCON_BCGEN_CS | EBU_BUSCON_ALEC |
>> +	       EBU_BUSCON_SETUP_EN, ebu_host->ebu_addr + EBU_BUSCON(cs));
> Again, can you try to implement ->setup_data_interface()?
Sure, will implement .
>> +
>> +	/*
>> +	 * 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
>> +	 */
>> +	writel(EBU_MEM_BASE_CS_0 + EBU_MEM_OFFSET,
>> +	       ebu_host->ebu_addr + EBU_ADDR_SEL(0));
>> +	writel(EBU_MEM_BASE_CS_1 + EBU_MEM_OFFSET,
>> +	       ebu_host->ebu_addr + EBU_ADDR_SEL(cs));
> See my comments on those values. I feel like the mapping should be
> created based on information we gather from somewhere else.
Yes, of-course will update the proper way.
>> +	nand_set_flash_node(&ebu_host->chip, dev->of_node);
>> +	mtd = nand_to_mtd(&ebu_host->chip);
>> +	mtd->dev.parent = dev;
>> +	ebu_host->dev = dev;
>> +
>> +	platform_set_drvdata(pdev, ebu_host);
>> +	nand_set_controller_data(&ebu_host->chip, ebu_host);
>> +
>> +	nand = &ebu_host->chip;
>> +	nand->controller = &ebu_host->controller;
>> +	nand->controller->ops = &ebu_nand_controller_ops;
>> +
>> +	/* Scan to find existence of the device */
>> +	ret = nand_scan(&ebu_host->chip, 1);
>> +	if (ret)
>> +		goto exit_dma;
>> +
>> +	ret = mtd_device_register(mtd, NULL, 0);
>> +	if (ret)
>> +		goto clean_nand;
>> +
>> +	return 0;
>> +
>> +clean_nand:
> err_cleanup_nand:
Noted.
>> +	nand_cleanup(&ebu_host->chip);
>> +exit_dma:
> err_cleanup_dma:
Noted.
>> +	ebu_dma_cleanup(ebu_host);
>> +	clk_disable_unprepare(ebu_host->clk);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ebu_nand_remove(struct platform_device *pdev)
>> +{
>> +	struct ebu_nand_controller *ebu_host = platform_get_drvdata(pdev);
>> +
>> +	mtd_device_unregister(nand_to_mtd(&ebu_host->chip));
> Check the return value of this function please.

sure, will check and update.
Regards
Vadivel



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux