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
