[PATCH v9 2/2] mtd: nand: Add support for Arasan NAND Flash Controller
Miquel Raynal
miquel.raynal at free-electrons.com
Sat Feb 3 07:05:04 PST 2018
Hi Naga,
-> Boris, there is one question for you below (about NVDDR).
> > > +static inline struct anfc_nand_chip *to_anfc_nand(struct nand_chip
> > > *nand) +{
> > > + return container_of(nand, struct anfc_nand_chip, chip); }
> > > +
> > > +static inline struct anfc *to_anfc(struct nand_hw_control *ctrl) {
> > > + return container_of(ctrl, struct anfc, controller); }
> > > +
> > > +static u8 anfc_page(u32 pagesize)
> > > +{
> > > + switch (pagesize) {
> > > + case 512:
> > > + return REG_PAGE_SIZE_512;
> > > + case 1024:
> > > + return REG_PAGE_SIZE_1K;
> > > + case 2048:
> > > + return REG_PAGE_SIZE_2K;
> > > + case 4096:
> > > + return REG_PAGE_SIZE_4K;
> > > + case 8192:
> > > + return REG_PAGE_SIZE_8K;
> > > + case 16384:
> >
> > Maybe you can use macros like SZ_512, SZ_1K, SZ_2K, etc, see
> > include/linux/sizes.h
> These are not really page size values.
> In CMD_REG[25:23] we need to specify page size as below
> 000: 512-byte Page size.
> 001: 2KB Page size.
> 010: 4KB Page size.
> 011: 8KB Page size.
> 100: 16KB Page size, hence for naming conventions we used the above macros.
> Means, REG_PAGE_SIZE_512 = 0.
I meant for the 'case' statement, not the return value.
ie. s/case 2048:/case SZ_2K:/
> >
> > > + return REG_PAGE_SIZE_16K;
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static inline void anfc_enable_intrs(struct anfc *nfc, u32 val) {
> > > + writel(val, nfc->base + INTR_STS_EN_OFST);
> > > + writel(val, nfc->base + INTR_SIG_EN_OFST); }
> > > +
> > > +static inline void anfc_config_ecc(struct anfc *nfc, int on) {
> > > + u32 val;
> > > +
> > > + val = readl(nfc->base + CMD_OFST);
> > > + if (on)
> > > + val |= ECC_ENABLE;
> > > + else
> > > + val &= ~ECC_ENABLE;
> > > + writel(val, nfc->base + CMD_OFST);
> > > +}
> > > +
> > > +static inline void anfc_config_dma(struct anfc *nfc, int on) {
> > > + u32 val;
> > > +
> > > + val = readl(nfc->base + CMD_OFST);
> > > + val &= ~DMA_EN_MASK;
> > > + if (on)
> > > + val |= DMA_ENABLE << DMA_EN_SHIFT;
> > > + writel(val, nfc->base + CMD_OFST);
> > > +}
> >
> > Nitpick: both anfc_config_ecc/dma() functions do similar actions but the syntax
> > is different.
> Yes, we can make it generic, But to identify whether we are configuring ecc or dma, these > two functions are added.
Sure, I am not discussing the two functions, but just to achieve the
same goal (put an enable bit or wipe it out) either you do
if (enable)
enable;
else
disable;
while right after you do:
val = disable;
if (enable)
val = enable;
I don't care which one you will chose but maybe you could use only one
style.
> > > +}
> > > +
> > > +static int anfc_read_page_hwecc(struct mtd_info *mtd,
> > > + struct nand_chip *chip, uint8_t *buf,
> > > + int oob_required, int page)
> > > +{
> > > + u32 val;
> > > + struct anfc *nfc = to_anfc(chip->controller);
> > > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > + u8 *ecc_code = chip->buffers->ecccode;
> > > + u8 *p = buf;
> > > + int eccsize = chip->ecc.size;
> > > + int eccbytes = chip->ecc.bytes;
> > > + int eccsteps = chip->ecc.steps;
> > > + int stat = 0, i;
> > > +
> > > + anfc_set_eccsparecmd(nfc, achip, NAND_CMD_RNDOUT,
> > > NAND_CMD_RNDOUTSTART);
> > > + anfc_config_ecc(nfc, 1);
> > > +
> > > + chip->read_buf(mtd, buf, mtd->writesize);
> > > +
> > > + val = readl(nfc->base + ECC_ERR_CNT_OFST);
> > > + val = ((val & PAGE_ERR_CNT_MASK) >> 8);
> >
> > If I understand this correctly, there is no point doing the upper two lines out of
> > the if statement?
> Yes, I will move this to if statement.
> >
> > > + if (achip->bch) {
> > > + mtd->ecc_stats.corrected += val;
> >
> > This is strange that there is no handling of a failing situation when using BCH
> > algorithm. You should probably add some logic here that either increments
> > ecc_stats.corrected or ecc_stats.failed like it is done below.
> Yes, in case of BCH, upto 24 bit errors it will correct. After that it won't even detect the errors.
> Hence when BCH we are not updating errors.
That's weird... Could you please double check this assumption? I find
weird that you have no way to distinguish a "there was no bitflips"
with a "there are too much bitflips so I can't even correct them".
>
> >
> > > + } else {
> > > + val = readl(nfc->base + ECC_ERR_CNT_1BIT_OFST);
> > > + mtd->ecc_stats.corrected += val;
> > > + val = readl(nfc->base + ECC_ERR_CNT_2BIT_OFST);
> > > + mtd->ecc_stats.failed += val;
> > > + /* Clear ecc error count register 1Bit, 2Bit */
> > > + writel(0x0, nfc->base + ECC_ERR_CNT_1BIT_OFST);
> > > + writel(0x0, nfc->base + ECC_ERR_CNT_2BIT_OFST);
> > > + }
> > > +
> > > + if (oob_required)
> > > + chip->ecc.read_oob(mtd, chip, page);
> > > +
> > > + anfc_config_ecc(nfc, 0);
> > > +
> >
> > Some comments would be welcome from here to the end, that is not entirely
> > clear to me what you try to achieve here.
> >
> > > + if (val) {
> >
> > When using Hamming, val != 0 means there is an uncorrectable error, while
> > when using BCH, it may just mean there was some corrected bitflips. I think you
> > should enter this statement only if Hamming or BCH failed to recover bitflips?
> Yes, I will update.
> >
> > > + chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> > > + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> >
> > If OOB area was required, you are most likely overwriting OOB bytes that were
> > maybe corrected by the ECC engine before it was turned off, by bytes retried
> > with a raw read (potentially with bitflips).
> This code is added to handle below case.
> When a page was erased, ECC won't update for erased page.
> And when we do an empty page read, ECC will be updated and there is mismatch between ECC data
> At the time erasing and reading, causing ECC errors.
> Hence we are reading entire OOB and updating the correct ECC value for all 0xff's case.
> This we have seen when using UBIFS, sometimes read happens on erased page.
I see what you try to achieve but actually I don't think this is the
right way. Instead, you should, upon error (val != 0), use the core
helper nand_check_erased_ecc_chunk(). This way you check if the page is
good or not depending on the strength used and can discriminate a bad
written page (almost full of 0xff) and a clean erased page without ECC
bytes for which the ECC calculation failed.
> >
> > > + mtd_ooblayout_get_eccbytes(mtd, ecc_code,
> > > chip->oob_poi, 0,
> > > + chip->ecc.total);
> > > + for (i = 0 ; eccsteps; eccsteps--, i += eccbytes,
> > > + p += eccsize) {
> > > + stat = nand_check_erased_ecc_chunk(p,
> > > chip->ecc.size, &ecc_code[i], eccbytes, NULL, 0, chip->ecc.strength);
> >
> > Do you actually check the entire OOB area here ?
> >
> > Can't you just do the check on chip->oob_poi?
>
> Above comments answers this.
> >
> > > + }
> > > + if (stat < 0)
> >
> > Please check this, but I think you should increment ecc_stats.failed here.
> Yes, I will update.
> >
> > > + stat = 0;
> > > + else
> > > + mtd->ecc_stats.corrected += stat;
> > > + return stat;
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > Can you run nandbiterrs test (from mtd-utils) with both Hamming and BCH and
> > check it fails after the right number of bitflips?
> Ok, I will run.
> >
> > > +
> > > +static int anfc_write_page_hwecc(struct mtd_info *mtd,
> > > + struct nand_chip *chip, const
> > > uint8_t *buf,
> > > + int oob_required, int page)
> > > +{
> > > + int ret;
> > > + struct anfc *nfc = to_anfc(chip->controller);
> > > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > + u8 *ecc_calc = chip->buffers->ecccalc;
> > > +
> > > + anfc_set_eccsparecmd(nfc, achip, NAND_CMD_RNDIN, 0);
> > > + anfc_config_ecc(nfc, 1);
> > > +
> > > + chip->write_buf(mtd, buf, mtd->writesize);
> > > +
> > > + if (oob_required) {
> > > + chip->waitfunc(mtd, chip);
> > > + chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> > > + chip->read_buf(mtd, ecc_calc, mtd->oobsize);
> > > + ret = mtd_ooblayout_set_eccbytes(mtd, ecc_calc,
> > > chip->oob_poi,
> > > + 0, chip->ecc.total);
> > > + if (ret)
> > > + return ret;
> > > + chip->ecc.write_oob(mtd, chip, page);
> > > + }
> > > + anfc_config_ecc(nfc, 0);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static u8 anfc_read_byte(struct mtd_info *mtd) {
> > > + struct nand_chip *chip = mtd_to_nand(mtd);
> > > + struct anfc *nfc = to_anfc(chip->controller);
> > > +
> > > + if (nfc->curr_cmd == NAND_CMD_STATUS)
> > > + return readl(nfc->base + FLASH_STS_OFST);
> > > + else
> > > + return nfc->buf[nfc->bufshift++];
> > > +}
> > > +
> > > +static int anfc_extra_init(struct anfc *nfc, struct anfc_nand_chip
> > > *achip) +{
> > > + int mode, err;
> > > + unsigned int feature[2];
> > > + u32 inftimeval;
> > > + struct nand_chip *chip = &achip->chip;
> > > + struct mtd_info *mtd = nand_to_mtd(chip);
> > > + bool change_sdr_clk = false;
> > > +
> > > + if (!chip->onfi_version ||
> > > + !(le16_to_cpu(chip->onfi_params.opt_cmd)
> > > + & ONFI_OPT_CMD_SET_GET_FEATURES))
> > > + return -EINVAL;
> >
> > I think this check is already done by the core.
> See my comments below.
> >
> > > +
> > > + /*
> > > + * If the controller is already in the same mode as flash
> > > device
> > > + * then no need to change the timing mode again.
> > > + */
> > > + if (readl(nfc->base + DATA_INTERFACE_OFST) ==
> > > achip->inftimeval)
> > > + return 0;
> > > +
> > > + memset(feature, 0, NVDDR_MODE_PACKET_SIZE);
> > > + /* Get nvddr timing modes */
> > > + mode = onfi_get_sync_timing_mode(chip) & 0xff;
> > > + if (!mode) {
> > > + mode = fls(onfi_get_async_timing_mode(chip)) - 1;
> > > + inftimeval = mode;
> > > + if (mode >= 2 && mode <= 5)
> > > + change_sdr_clk = true;
> > > + } else {
> > > + mode = fls(mode) - 1;
> > > + inftimeval = NVDDR_MODE | (mode <<
> > > NVDDR_TIMING_MODE_SHIFT);
> > > + mode |= ONFI_DATA_INTERFACE_NVDDR;
> > > + }
> > > + feature[0] = mode;
> > > + chip->select_chip(mtd, achip->csnum);
> > > + err = chip->onfi_set_features(mtd, chip,
> > > ONFI_FEATURE_ADDR_TIMING_MODE,
> > > + (uint8_t *)feature);
> > > + chip->select_chip(mtd, -1);
> > > + if (err)
> > > + return err;
> >
> > You should forget all the NAND chip related code here, the core already handles
> > it, and then calls ->setup_data_interface() to tune timings on the controller side
> > only.
> Since core doesn't have support for NVDDR,
> enum nand_data_interface_type {
> NAND_SDR_IFACE,
> };
> we added this logic in setup_data_interface. To achieve the same
> So what we are doing here is,
> When core calls setup_data_interface, the flash changes its operating mode to SDR.
> And when flash supports NVDDR, we are changing the
> Flash operating mode to NVDDR.
Not sure this is actually supported by the core. Maybe Boris could give
us some clues?
> Also the setup_data_interface call will happen during probe time, and at this time we don't have
> Parameter page info, hence I added
I sent a patch series to fix that. Let's keep it for now but soon this
will have to be removed.
> if (!chip->onfi_version ||
> !(le16_to_cpu(chip->onfi_params.opt_cmd)
> & ONFI_OPT_CMD_SET_GET_FEATURES))
> return -EINVAL;
> >
> > > + /*
> > > + * SDR timing modes 2-5 will not work for the arasan nand
> > > when
> > > + * freq > 90 MHz, so reduce the freq in SDR modes 2-5 to <
> > > 90Mhz
> > > + */
> > > + if (change_sdr_clk) {
> > > + clk_disable_unprepare(nfc->clk_sys);
> > > + err = clk_set_rate(nfc->clk_sys,
> > > SDR_MODE_DEFLT_FREQ);
> > > + if (err) {
> > > + dev_err(nfc->dev, "Can't set the clock
> > > rate\n");
> > > + return err;
> > > + }
> > > + err = clk_prepare_enable(nfc->clk_sys);
> > > + if (err) {
> > > + dev_err(nfc->dev, "Unable to enable sys
> > > clock.\n");
> > > + clk_disable_unprepare(nfc->clk_sys);
> > > + return err;
> > > + }
> >
> > You do this twice as it is already handled in ->setup_data_interface().
> Yes, we added it twice, once when core sends SDR info
> And the second, because there is a HW bug in controller, i.e SDR timing modes 2-5 will not work, when freq > 90MHz.
> Hence when Flash operates at SDR modes 2-5, we are changing the clock rate to below 90MHz
1/ If you do it twice because of a HW bug: please add a comment.
2/ What if there are several NAND chips using different timing
modes? You probably should handle this in ->select_chip() then.
> >
> > > + }
> > > + achip->inftimeval = inftimeval;
> > > + if (mode & ONFI_DATA_INTERFACE_NVDDR)
> > > + achip->spktsize = NVDDR_MODE_PACKET_SIZE;
> > > + return 0;
> > > +}
> >
> > I don't think this should be in a separate function and instead should be in -
> > >setup_data_interface().
> Yes, we can do it in single function, but as mentioned in comments, we added this extra_init().
> >
> > > +
> > > +static int anfc_ecc_init(struct mtd_info *mtd,
> > > + struct nand_ecc_ctrl *ecc)
> > > +{
> > > + u32 ecc_addr;
> > > + unsigned int bchmode, steps;
> > > + struct nand_chip *chip = mtd_to_nand(mtd);
> > > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > +
> > > + ecc->mode = NAND_ECC_HW;
> > > + ecc->read_page = anfc_read_page_hwecc;
> > > + ecc->write_page = anfc_write_page_hwecc;
> > > + ecc->write_oob = anfc_write_oob;
> > > + mtd_set_ooblayout(mtd, &anfc_ooblayout_ops);
> > > +
> > > + steps = mtd->writesize / chip->ecc_step_ds;
> > > +
> > > + switch (chip->ecc_strength_ds) {
> > > + case 12:
> > > + bchmode = 0x1;
> > > + break;
> > > + case 8:
> > > + bchmode = 0x2;
> > > + break;
> > > + case 4:
> > > + bchmode = 0x3;
> > > + break;
> > > + case 24:
> > > + bchmode = 0x4;
> > > + break;
> > > + default:
> > > + bchmode = 0x0;
> > > + }
> > > +
> > > + if (!bchmode)
> > > + ecc->total = 3 * steps;
> > > + else
> > > + ecc->total =
> > > + DIV_ROUND_UP(fls(8 * chip->ecc_step_ds) *
> > > + chip->ecc_strength_ds * steps, 8);
> > > +
> > > + ecc->strength = chip->ecc_strength_ds;
> > > + ecc->size = chip->ecc_step_ds;
> > > + ecc->bytes = ecc->total / steps;
> > > + ecc->steps = steps;
> > > + achip->bchmode = bchmode;
> > > + achip->bch = achip->bchmode;
> > > + ecc_addr = mtd->writesize + (mtd->oobsize - ecc->total);
> > > +
> > > + achip->eccval = ecc_addr | (ecc->total << ECC_SIZE_SHIFT) |
> > > + (achip->bch << BCH_EN_SHIFT);
> > > +
> > > + if (chip->ecc_step_ds >= 1024)
> > > + achip->pktsize = 1024;
> > > + else
> > > + achip->pktsize = 512;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void anfc_cmd_function(struct mtd_info *mtd,
> > > + unsigned int cmd, int column, int
> > > page_addr) +{
> > > + struct nand_chip *chip = mtd_to_nand(mtd);
> > > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > + struct anfc *nfc = to_anfc(chip->controller);
> > > + bool wait = false;
> > > + u32 addrcycles, prog;
> > > +
> > > + nfc->bufshift = 0;
> > > + nfc->curr_cmd = cmd;
> > > +
> > > + if (page_addr == -1)
> > > + page_addr = 0;
> > > + if (column == -1)
> > > + column = 0;
> > > +
> > > + switch (cmd) {
> > > + case NAND_CMD_RESET:
> > > + anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 0);
> > > + prog = PROG_RST;
> > > + wait = true;
> > > + break;
> > > + case NAND_CMD_SEQIN:
> > > + addrcycles = achip->raddr_cycles +
> > > achip->caddr_cycles;
> > > + anfc_prepare_cmd(nfc, cmd, NAND_CMD_PAGEPROG, 1,
> > > + mtd->writesize, addrcycles);
> > > + anfc_setpagecoladdr(nfc, page_addr, column);
> > > + break;
> > > + case NAND_CMD_READOOB:
> > > + column += mtd->writesize;
> > > + case NAND_CMD_READ0:
> > > + case NAND_CMD_READ1:
> > > + addrcycles = achip->raddr_cycles +
> > > achip->caddr_cycles;
> > > + anfc_prepare_cmd(nfc, NAND_CMD_READ0,
> > > NAND_CMD_READSTART, 1,
> > > + mtd->writesize, addrcycles);
> > > + anfc_setpagecoladdr(nfc, page_addr, column);
> > > + break;
> > > + case NAND_CMD_RNDOUT:
> > > + anfc_prepare_cmd(nfc, cmd, NAND_CMD_RNDOUTSTART, 1,
> > > + mtd->writesize, 2);
> > > + anfc_setpagecoladdr(nfc, page_addr, column);
> > > + break;
> > > + case NAND_CMD_PARAM:
> > > + anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
> > > + anfc_setpagecoladdr(nfc, page_addr, column);
> > > + anfc_rw_buf_pio(mtd, nfc->buf,
> > > + (4 * sizeof(struct
> > > nand_onfi_params)),
> > > + 1, PROG_RDPARAM);
> > > + break;
> > > + case NAND_CMD_READID:
> > > + anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
> > > + anfc_setpagecoladdr(nfc, page_addr, column);
> > > + anfc_rw_buf_pio(mtd, nfc->buf, ONFI_ID_LEN, 1,
> > > PROG_RDID);
> > > + break;
> > > + case NAND_CMD_ERASE1:
> > > + addrcycles = achip->raddr_cycles;
> > > + prog = PROG_ERASE;
> > > + anfc_prepare_cmd(nfc, cmd, NAND_CMD_ERASE2, 0, 0,
> > > addrcycles);
> > > + column = page_addr & 0xffff;
> > > + page_addr = (page_addr >> PG_ADDR_SHIFT) & 0xffff;
> > > + anfc_setpagecoladdr(nfc, page_addr, column);
> > > + wait = true;
> > > + break;
> > > + case NAND_CMD_STATUS:
> > > + anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 0);
> > > + anfc_setpktszcnt(nfc, achip->spktsize / 4, 1);
> > > + anfc_setpagecoladdr(nfc, page_addr, column);
> > > + prog = PROG_STATUS;
> > > + wait = true;
> > > + break;
> > > + case NAND_CMD_GET_FEATURES:
> > > + anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
> > > + anfc_setpagecoladdr(nfc, page_addr, column);
> > > + anfc_rw_buf_pio(mtd, nfc->buf, achip->spktsize, 1,
> > > + PROG_GET_FEATURE);
> > > + break;
> > > + case NAND_CMD_SET_FEATURES:
> > > + anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
> > > + anfc_setpagecoladdr(nfc, page_addr, column);
> > > + break;
> > > + default:
> > > + return;
> > > + }
> > > +
> > > + if (wait) {
> > > + anfc_enable_intrs(nfc, XFER_COMPLETE);
> > > + writel(prog, nfc->base + PROG_OFST);
> > > + anfc_wait_for_event(nfc);
> > > + }
> > > + if (nfc->curr_cmd == NAND_CMD_STATUS)
> > > + nfc->status = readl(nfc->base + FLASH_STS_OFST); }
> > > +
> > > +static void anfc_select_chip(struct mtd_info *mtd, int num) {
> > > + u32 val;
> > > + struct nand_chip *chip = mtd_to_nand(mtd);
> > > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > + struct anfc *nfc = to_anfc(chip->controller);
> > > +
> > > + if (num == -1)
> > > + return;
> > > +
> > > + val = readl(nfc->base + MEM_ADDR2_OFST);
> > > + val &= (val & ~(CS_MASK | BCH_MODE_MASK));
> > > + val |= (achip->csnum << CS_SHIFT) | (achip->bchmode <<
> > > BCH_MODE_SHIFT);
> > > + writel(val, nfc->base + MEM_ADDR2_OFST);
> > > + nfc->csnum = achip->csnum;
> > > + writel(achip->eccval, nfc->base + ECC_OFST);
> > > + writel(achip->inftimeval, nfc->base + DATA_INTERFACE_OFST); }
> > > +
> > > +static irqreturn_t anfc_irq_handler(int irq, void *ptr) {
> > > + struct anfc *nfc = ptr;
> > > + u32 status;
> > > +
> > > + status = readl(nfc->base + INTR_STS_OFST);
> > > + if (status & EVENT_MASK) {
> > > + complete(&nfc->event);
> > > + writel((status & EVENT_MASK), nfc->base +
> > > INTR_STS_OFST);
> > > + writel(0, nfc->base + INTR_STS_EN_OFST);
> > > + writel(0, nfc->base + INTR_SIG_EN_OFST);
> > > + return IRQ_HANDLED;
> > > + }
> > > +
> > > + return IRQ_NONE;
> > > +}
> > > +
> > > +static int anfc_onfi_set_features(struct mtd_info *mtd, struct
> > > nand_chip *chip,
> > > + int addr, uint8_t
> > > *subfeature_param) +{
> > > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > + int status;
> > > +
> > > + if (!chip->onfi_version)
> > > + return -EINVAL;
> > > +
> > > + if (!(le16_to_cpu(chip->onfi_params.opt_cmd) &
> > > + ONFI_OPT_CMD_SET_GET_FEATURES))
> > > + return -EINVAL;
> > > +
> > > + chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
> > > + anfc_rw_buf_pio(mtd, subfeature_param, achip->spktsize,
> > > + 0, PROG_SET_FEATURE);
> > > + status = chip->waitfunc(mtd, chip);
> > > + if (status & NAND_STATUS_FAIL)
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
> >
> > Are you sure this function is needed? If yes can you add a comment to explain
> > why? Otherwise I think the core already handles that kind of logic.
> We added this to set/get features of on-die ECC. At the time of adding this, core doesn’t have support
> For this. I will remove it now, since micron on-die ecc driver is there.
> >
> > > +
> > > +static int anfc_setup_data_interface(struct mtd_info *mtd, int
> > > csline,
> > > + const struct
> > > nand_data_interface *conf) +{
> > > + struct nand_chip *chip = mtd_to_nand(mtd);
> > > + struct anfc *nfc = to_anfc(chip->controller);
> > > + int err;
> > > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > +
> > > + if (csline == NAND_DATA_IFACE_CHECK_ONLY)
> > > + return 0;
> > > +
> > > + clk_disable_unprepare(nfc->clk_sys);
> > > + err = clk_set_rate(nfc->clk_sys, SDR_MODE_DEFLT_FREQ);
> > > + if (err) {
> > > + dev_err(nfc->dev, "Can't set the clock rate\n");
> > > + return err;
> > > + }
> > > + err = clk_prepare_enable(nfc->clk_sys);
> > > + if (err) {
> > > + dev_err(nfc->dev, "Unable to enable sys clock.\n");
> > > + clk_disable_unprepare(nfc->clk_sys);
> > > + return err;
> > > + }
> > > + achip->inftimeval = 0;
> > > + anfc_extra_init(nfc, achip);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int anfc_nand_chip_init(struct anfc *nfc,
> > > + struct anfc_nand_chip *anand_chip,
> > > + struct device_node *np)
> > > +{
> > > + struct nand_chip *chip = &anand_chip->chip;
> > > + struct mtd_info *mtd = nand_to_mtd(chip);
> > > + int ret;
> > > +
> > > + ret = of_property_read_u32(np, "reg", &anand_chip->csnum);
> > > + if (ret) {
> > > + dev_err(nfc->dev, "can't get chip-select\n");
> > > + return -ENXIO;
> > > + }
> > > +
> > > + mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL,
> > > "arasan_nand.%d",
> > > + anand_chip->csnum);
> > > + mtd->dev.parent = nfc->dev;
> > > +
> > > + chip->cmdfunc = anfc_cmd_function;
> > > + chip->chip_delay = 30;
> > > + chip->controller = &nfc->controller;
> > > + chip->read_buf = anfc_read_buf;
> > > + chip->write_buf = anfc_write_buf;
> > > + chip->read_byte = anfc_read_byte;
> > > + chip->options = NAND_BUSWIDTH_AUTO |
> > NAND_NO_SUBPAGE_WRITE;
> > > + chip->bbt_options = NAND_BBT_USE_FLASH;
> > > + chip->select_chip = anfc_select_chip;
> > > + chip->onfi_set_features = anfc_onfi_set_features;
> > > + chip->setup_data_interface = anfc_setup_data_interface;
> > > + nand_set_flash_node(chip, np);
> > > +
> > > + anand_chip->spktsize = SDR_MODE_PACKET_SIZE;
> > > + ret = nand_scan_ident(mtd, 1, NULL);
> > > + if (ret) {
> > > + dev_err(nfc->dev, "nand_scan_ident for NAND
> > > failed\n");
> > > + return ret;
> > > + }
> > > + if (chip->onfi_version) {
> > > + anand_chip->raddr_cycles =
> > > chip->onfi_params.addr_cycles & 0xf;
> > > + anand_chip->caddr_cycles =
> > > + (chip->onfi_params.addr_cycles >> 4)
> > > & 0xf;
> > > + } else {
> > > + /* For non-ONFI devices, configuring the address
> > > cyles as 5 */
> > > + anand_chip->raddr_cycles = 3;
> > > + anand_chip->caddr_cycles = 2;
> > > + }
> >
> > I think you should remove this block and instead decide how many cycles you
> > will need depending on "chip->options & NAND_ROW_ADDR_3".
> If chip supports ONFI, then we are reading addr cycles from parameter page, if not
> We are setting with default values.
> I am not checking the sizes here, instead reading it from parameter page.
> I didn't get your comment, can you please brief it?
> I saw the patch https://patchwork.kernel.org/patch/9950377/.
> These are based on sizes, and cycles are getting updated using " NAND_ROW_ADDR_3"
> But the case here is I am setting some default values if not ONFI.
I suppose this is already handled by the core. You should probably just
choose the number of address cycles depending on the NAND_ROW_ADDR_3
flag presence in chip->options. Logic should be in the core and shared
across all NAND controllers anyway.
> >
> > > +
> > > + ret = anfc_ecc_init(mtd, &chip->ecc);
> > > + if (ret)
> > > + return ret;
> > > +
> >
> > Shouldn't the controller set mtd->name here if empty?
> Yes, driver is not setting the name if empty.
> I will add this in next version of patch.
> >
> > > + ret = nand_scan_tail(mtd);
> > > + if (ret) {
> > > + dev_err(nfc->dev, "nand_scan_tail for NAND
> > > failed\n");
> > > + return ret;
> > > + }
> > > +
> > > + return mtd_device_register(mtd, NULL, 0); }
> > > +
> > > +static int anfc_probe(struct platform_device *pdev) {
> > > + struct anfc *nfc;
> > > + struct anfc_nand_chip *anand_chip;
> > > + struct device_node *np = pdev->dev.of_node, *child;
> > > + struct resource *res;
> > > + int err;
> > > +
> > > + nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
> > > + if (!nfc)
> > > + return -ENOMEM;
> > > +
> > > + init_waitqueue_head(&nfc->controller.wq);
> > > + INIT_LIST_HEAD(&nfc->chips);
> > > + init_completion(&nfc->event);
> > > + nfc->dev = &pdev->dev;
> > > + platform_set_drvdata(pdev, nfc);
> > > + nfc->csnum = -1;
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + nfc->base = devm_ioremap_resource(&pdev->dev, res);
> > > + if (IS_ERR(nfc->base))
> > > + return PTR_ERR(nfc->base);
> > > + nfc->dma = of_property_read_bool(pdev->dev.of_node,
> > > + "arasan,has-mdma");
> > > + nfc->irq = platform_get_irq(pdev, 0);
> > > + if (nfc->irq < 0) {
> > > + dev_err(&pdev->dev, "platform_get_irq failed\n");
> > > + return -ENXIO;
> > > + }
> > > + dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
> > > + err = devm_request_irq(&pdev->dev, nfc->irq,
> > > anfc_irq_handler,
> > > + 0, "arasannfc", nfc);
> > > + if (err)
> > > + return err;
> > > + nfc->clk_sys = devm_clk_get(&pdev->dev, "sys");
> > > + if (IS_ERR(nfc->clk_sys)) {
> > > + dev_err(&pdev->dev, "sys clock not found.\n");
> > > + return PTR_ERR(nfc->clk_sys);
> > > + }
> > > +
> > > + nfc->clk_flash = devm_clk_get(&pdev->dev, "flash");
> > > + if (IS_ERR(nfc->clk_flash)) {
> > > + dev_err(&pdev->dev, "flash clock not found.\n");
> > > + return PTR_ERR(nfc->clk_flash);
> > > + }
> > > +
> > > + err = clk_prepare_enable(nfc->clk_sys);
> > > + if (err) {
> > > + dev_err(&pdev->dev, "Unable to enable sys clock.\n");
> > > + return err;
> > > + }
> > > +
> > > + err = clk_prepare_enable(nfc->clk_flash);
> > > + if (err) {
> > > + dev_err(&pdev->dev, "Unable to enable flash
> > > clock.\n");
> > > + goto clk_dis_sys;
> > > + }
> > > +
> > > + for_each_available_child_of_node(np, child) {
> > > + anand_chip = devm_kzalloc(&pdev->dev,
> > > sizeof(*anand_chip),
> > > + GFP_KERNEL);
> > > + if (!anand_chip) {
> > > + of_node_put(child);
> > > + err = -ENOMEM;
> > > + goto nandchip_clean_up;
> > > + }
> > > +
> > > + err = anfc_nand_chip_init(nfc, anand_chip, child);
> > > + if (err) {
> > > + devm_kfree(&pdev->dev, anand_chip);
> > > + continue;
> > > + }
> > > +
> > > + list_add_tail(&anand_chip->node, &nfc->chips);
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +nandchip_clean_up:
> > > + list_for_each_entry(anand_chip, &nfc->chips, node)
> > > + nand_release(nand_to_mtd(&anand_chip->chip));
> > > + clk_disable_unprepare(nfc->clk_flash);
> > > +clk_dis_sys:
> > > + clk_disable_unprepare(nfc->clk_sys);
> > > +
> > > + return err;
> > > +}
> > > +
> > > +static int anfc_remove(struct platform_device *pdev) {
> > > + struct anfc *nfc = platform_get_drvdata(pdev);
> > > + struct anfc_nand_chip *anand_chip;
> > > +
> > > + list_for_each_entry(anand_chip, &nfc->chips, node)
> > > + nand_release(nand_to_mtd(&anand_chip->chip));
> > > +
> > > + clk_disable_unprepare(nfc->clk_sys);
> > > + clk_disable_unprepare(nfc->clk_flash);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id anfc_ids[] = {
> > > + { .compatible = "arasan,nfc-v3p10" },
> > > + { .compatible = "xlnx,zynqmp-nand" },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, anfc_ids);
> > > +
> > > +static struct platform_driver anfc_driver = {
> > > + .driver = {
> > > + .name = DRIVER_NAME,
> > > + .of_match_table = anfc_ids,
> > > + },
> > > + .probe = anfc_probe,
> > > + .remove = anfc_remove,
> > > +};
> > > +module_platform_driver(anfc_driver);
> > > +
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Xilinx, Inc");
> > > +MODULE_DESCRIPTION("Arasan NAND Flash Controller Driver");
> >
> >
> >
> > --
> > Miquel Raynal, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com
>
> I will restructure the driver to sync with exec_op().
> Thanks again for reviewing the driver.
>
> Thanks,
> Naga Sureshkumar Relli.
Thanks for your work,
Miquèl
--
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
More information about the linux-mtd
mailing list