[PATCH v3 1/3] mtd: nand: add asm9260 NFC driver
Boris Brezillon
boris.brezillon at free-electrons.com
Thu Jan 1 13:03:46 PST 2015
On Thu, 01 Jan 2015 21:18:44 +0100
Oleksij Rempel <linux at rempel-privat.de> wrote:
> Am 31.12.2014 um 17:48 schrieb Boris Brezillon:
> > Hi Oleksij,
> >
> > You should really add a cover letter containing a changelog (updated at
> > each new version of your cover letter) so that reviewers can easily
> > identify what has changed.
> >
> > While you're at it, can you add your mtd test results to the cover
> > letter ?
>
> ok.
>
> [snip]
>
> >> +/**
> >> + * We can't read less then 32 bits on HW_FIFO_DATA. So, to make
> >> + * read_byte and read_word happy, we use sort of cached 32bit read.
> >> + * Note: expected values for size should be 1 or 2 bytes.
> >> + */
> >> +static u32 asm9260_nand_read_cached(struct mtd_info *mtd, int size)
> >> +{
> >> + struct asm9260_nand_priv *priv = mtd_to_priv(mtd);
> >> + u8 tmp;
> >> +
> >> + if ((priv->read_cache_cnt <= 0) || (priv->read_cache_cnt > 4)) {
> >
> > ^ how can this ever happen ?
> >
> >> + asm9260_nand_cmd_comp(mtd, 0);
> >> + priv->read_cache = ioread32(priv->base + HW_FIFO_DATA);
> >> + priv->read_cache_cnt = 4;
> >> + }
> >> +
> >> + tmp = priv->read_cache >> (8 * (4 - priv->read_cache_cnt));
> >> + priv->read_cache_cnt -= size;
> >> +
> >> + return tmp;
> >> +}
> >> +
> >> +static u8 asm9260_nand_read_byte(struct mtd_info *mtd)
> >> +{
> >> + return 0xff & asm9260_nand_read_cached(mtd, 1);
> >
> > Maybe this mask operation could be done in asm9260_nand_read_cached, so
> > that you won't have to bother in read_byte/read_word functions.
>
> it is same as "return (u8)asm9260_nand_read_cached(mtd, 1);", just makes
> sure compiler do not complain.
Absolutely, I didn't notice the return type of this function (thought
it was returning an u32).
Does it complain when you directly return asm9260_nand_read_cached
result ?
>
> >> +}
> >> +
> >> +static u16 asm9260_nand_read_word(struct mtd_info *mtd)
> >> +{
> >> + return 0xffff & asm9260_nand_read_cached(mtd, 2);
> >
> > You'd better always use read_word, cause if you call read_byte once
> > then read_word twice, you'll end up with a wrong value after the second
> > read_word (3 bytes consumed, which means there's only 1 remaining byte
> > and you're asking for 2).
>
> nand_base use both functions. how can i use only one? For example:
> chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
>
> /* Read entire ID string */
> for (i = 0; i < 8; i++)
> id_data[i] = chip->read_byte(mtd);
>
> this wont work with read_word.
What I meant is that if you start using read_byte after launching a
specific command you should not mix read_byte and read_word, because
otherwise you might end up with erroneous values.
I'm not sure this can really happen (is there any code in nand core
mixing read_byte and read_word ?), but my point is that you should
handle that specific case (only one remaining byte in read_cache while
2 are requested) in asm9260_nand_read_cached...
>
>
> >> +}
> >> +
> >> +static void asm9260_nand_read_buf(struct mtd_info *mtd, u8 *buf, int len)
> >> +{
> >> + struct asm9260_nand_priv *priv = mtd_to_priv(mtd);
> >> + dma_addr_t dma_addr;
> >> + int dma_ok = 0;
> >> +
> >> + if (len & 0x3) {
> >> + dev_err(priv->dev, "Unsupported length (%x)\n", len);
> >> + return;
> >> + }
> >> +
> >> + /*
> >> + * I hate you UBI for your all vmalloc. Be slow as hell with PIO.
> >> + * ~ with love from ZeroCopy ~
> >> + */
> >> + if (!is_vmalloc_addr(buf)) {
> >> + dma_addr = asm9260_nand_dma_set(mtd, buf, DMA_FROM_DEVICE, len);
> >> + dma_ok = !(dma_mapping_error(priv->dev, dma_addr));
> >> + }
> >> + asm9260_nand_cmd_comp(mtd, dma_ok);
> >> +
> >> + if (dma_ok) {
> >> + dma_sync_single_for_cpu(priv->dev, dma_addr, len,
> >> + DMA_FROM_DEVICE);
> >> + dma_unmap_single(priv->dev, dma_addr, len, DMA_FROM_DEVICE);
> >> + return;
> >> + }
> >> +
> >> + /* fall back to pio mode */
> >> + len >>= 2;
> >> + ioread32_rep(priv->base + HW_FIFO_DATA, buf, len);
> >
> > Hm, what if the buf is not aligned on 32bit, or len is not a multiple
> > of 4 ?
>
> I can read only buf == page size. All page sizes suported by this
> controller are aligned. See "if (len & 0x3) {", and take a look at
> asm9260_nand_read_cached - we can read only 32bit.
But read_buf is not only used to read full pages (see [1]), and, AFAIR,
there's nothing preventing mtd users from passing a non 32 bit aligned
buf...
>
>
> >> +}
> >> +
> >> +static void asm9260_nand_write_buf(struct mtd_info *mtd,
> >> + const u8 *buf, int len)
> >> +{
> >> + struct asm9260_nand_priv *priv = mtd_to_priv(mtd);
> >> + dma_addr_t dma_addr;
> >> + int dma_ok = 0;
> >> +
> >> + if (len & 0x3) {
> >> + dev_err(priv->dev, "Unsupported length (%x)\n", len);
> >> + return;
> >> + }
> >> +
> >> + if (!is_vmalloc_addr(buf)) {
> >> + dma_addr = asm9260_nand_dma_set(mtd,
> >> + (void *)buf, DMA_TO_DEVICE, len);
> >> + dma_ok = !(dma_mapping_error(priv->dev, dma_addr));
> >> + }
> >> +
> >> + if (dma_ok)
> >> + dma_sync_single_for_device(priv->dev, dma_addr, len,
> >> + DMA_TO_DEVICE);
> >> + asm9260_nand_cmd_comp(mtd, dma_ok);
> >> +
> >> + if (dma_ok) {
> >> + dma_unmap_single(priv->dev, dma_addr, len, DMA_TO_DEVICE);
> >> + return;
> >> + }
> >> +
> >> + /* fall back to pio mode */
> >> + len >>= 2;
> >> + iowrite32_rep(priv->base + HW_FIFO_DATA, buf, len);
> >
> > Ditto
> >
> >> +}
> >> +
> >> +static int asm9260_nand_write_page_raw(struct mtd_info *mtd,
> >> + struct nand_chip *chip, const u8 *buf,
> >> + int oob_required)
> >> +{
> >> + struct asm9260_nand_priv *priv = mtd_to_priv(mtd);
> >> +
> >> + chip->write_buf(mtd, buf, mtd->writesize);
> >> + if (oob_required)
> >> + chip->ecc.write_oob(mtd, chip, priv->page_cache &
> >> + chip->pagemask);
> >
> > Can't you just call
> > chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> >
> > And if it works, then you can just drop this raw function since the
> > default one does the exact same thing.
>
> There 3 variants:
> - allocate dma == page + oob; and copy each buffer
> - use pio and read out oob even if it was not requested
> - use dma_map_single, avoid buffer copying, and request oob if needed.
>
> i use last variant even if it meane sending a command to request oob.
Because the controller cannot read in-band and out-of-band data in a
single command ?
>
> >
> >> + return 0;
> >> +}
> >> +
> >> +static int asm9260_nand_write_page_hwecc(struct mtd_info *mtd,
> >> + struct nand_chip *chip, const u8 *buf,
> >> + int oob_required)
> >> +{
> >> + struct asm9260_nand_priv *priv = mtd_to_priv(mtd);
> >> +
> >> + asm9260_reg_rmw(priv, HW_CTRL, BM_CTRL_ECC_EN, 0);
> >> + chip->ecc.write_page_raw(mtd, chip, buf, oob_required);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static unsigned int asm9260_nand_count_ecc(struct asm9260_nand_priv *priv)
> >> +{
> >> + u32 tmp, i, count, maxcount = 0;
> >> +
> >> + /* FIXME: this layout was tested only on 2048byte NAND.
> >> + * NANDs with bigger page size should use more registers. */
> >
> > Yep, you should really fix that, or at least reject NAND with a
> > different page size until you've fixed that.
>
> ok.
>
> >> + tmp = ioread32(priv->base + HW_ECC_ERR_CNT);
> >> + for (i = 0; i < 4; i++) {
> >> + count = 0x1f & (tmp >> (5 * i));
> >> + maxcount = max_t(unsigned int, maxcount, count);
> >> + }
> >> +
> >> + return count;
> >> +}
> >> +
> >> +static int asm9260_nand_read_page_hwecc(struct mtd_info *mtd,
> >> + struct nand_chip *chip, u8 *buf,
> >> + int oob_required, int page)
> >> +{
> >> + struct asm9260_nand_priv *priv = mtd_to_priv(mtd);
> >> + u8 *temp_ptr;
> >> + u32 status, max_bitflips = 0;
> >> +
> >> + temp_ptr = buf;
> >> +
> >> + /* enable ecc */
> >> + asm9260_reg_rmw(priv, HW_CTRL, BM_CTRL_ECC_EN, 0);
> >> + chip->read_buf(mtd, temp_ptr, mtd->writesize);
> >> +
> >> + status = ioread32(priv->base + HW_ECC_CTRL);
> >> +
> >> + if (status & BM_ECC_ERR_UNC) {
> >> + u32 ecc_err;
> >> +
> >> + ecc_err = ioread32(priv->base + HW_ECC_ERR_CNT);
> >> + /* check if it is erased page (all_DATA_OOB == 0xff) */
> >> + /* FIXME: should be tested if it is a bullet proof solution.
> >> + * if not, use is_buf_blank. */
> >
> > you'd rather use is_buf_blank if you're unsure.
>
> ok, can is_buf_blank be moved to nand_base?
I'll let Brian answer that one.
>
> >
> >> + if (ecc_err != 0x8421)
> >> + mtd->ecc_stats.failed++;
> >> +
> >> + } else if (status & BM_ECC_ERR_CORRECT) {
> >> + max_bitflips = asm9260_nand_count_ecc(priv);
> >
> > max_bitflips should contain the maximum number of bitflips in all
> > ECC blocks of a given page, here you just returning the number of
> > bitflips in the last ECC block.
>
> no. see asm9260_nand_count_ecc.
My bad, you're right (I thought the ECC step loop was done here).
>
> > The following should do the trick:
> >
> > int bitflips = asm9260_nand_count_ecc(priv);
> >
> > if (bitflips > max_bitflips)
> > max_bitflips = bitflips;
> >
> > mtd->ecc_stats.corrected += bitflips;
> >
> >> + mtd->ecc_stats.corrected += max_bitflips;
> >> + }
> >> +
> >> + if (oob_required)
> >> + chip->ecc.read_oob(mtd, chip, page);
> >> +
> >> + return max_bitflips;
> >> +}
> >> +
> >> +static irqreturn_t asm9260_nand_irq(int irq, void *device_info)
> >> +{
> >> + struct asm9260_nand_priv *priv = device_info;
> >> + struct nand_chip *nand = &priv->nand;
> >> + u32 status;
> >> +
> >> + status = ioread32(priv->base + HW_INT_STATUS);
> >> + if (!status)
> >> + return IRQ_NONE;
> >> +
> >> + iowrite32(0, priv->base + HW_INT_MASK);
> >> + iowrite32(0, priv->base + HW_INT_STATUS);
> >> + priv->irq_done = 1;
> >
> > You should test the flags before deciding the irq is actually done...
>
> ok. if fixed it by changing mem_mask logic.
Still, checking the status register is a good practice.
>
> >
> >> + wake_up(&nand->controller->wq);
> >
> > and if the condition is not met, don't wake up the waitqueue.
> >
> >> +
> >> + return IRQ_HANDLED;
> >> +}
> >> +
> >> +static void __init asm9260_nand_init_chip(struct nand_chip *nand_chip)
> >> +{
> >> + nand_chip->select_chip = asm9260_select_chip;
> >> + nand_chip->cmdfunc = asm9260_nand_command_lp;
> >
> > You seems to only support large pages NANDs (> 512 bytes), maybe you
> > should check if the discovered chip has large pages, and reject the NAND
> > otherwise.
>
> ok.
>
> >> + nand_chip->read_byte = asm9260_nand_read_byte;
> >> + nand_chip->read_word = asm9260_nand_read_word;
> >> + nand_chip->read_buf = asm9260_nand_read_buf;
> >> + nand_chip->write_buf = asm9260_nand_write_buf;
> >> +
> >> + nand_chip->dev_ready = asm9260_nand_dev_ready;
> >> + nand_chip->chip_delay = 100;
> >> + nand_chip->options |= NAND_NO_SUBPAGE_WRITE;
> >> +
> >> + nand_chip->ecc.write_page = asm9260_nand_write_page_hwecc;
> >> + nand_chip->ecc.write_page_raw = asm9260_nand_write_page_raw;
> >> + nand_chip->ecc.read_page = asm9260_nand_read_page_hwecc;
> >> +}
> >> +
> >> +static int __init asm9260_nand_cached_config(struct asm9260_nand_priv *priv)
> >> +{
> >> + struct nand_chip *nand = &priv->nand;
> >> + struct mtd_info *mtd = &priv->mtd;
> >> + u32 addr_cycles, col_cycles, pages_per_block;
> >> +
> >> + pages_per_block = mtd->erasesize / mtd->writesize;
> >> + /* max 256P, min 32P */
> >> + if (pages_per_block & ~(0x000001e0)) {
> >> + dev_err(priv->dev, "Unsupported erasesize 0x%x\n",
> >> + mtd->erasesize);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* max 32K, min 256. */
> >> + if (mtd->writesize & ~(0x0000ff00)) {
> >> + dev_err(priv->dev, "Unsupported writesize 0x%x\n",
> >> + mtd->erasesize);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + col_cycles = 2;
> >> + addr_cycles = col_cycles +
> >> + (((mtd->size >> mtd->writesize) > 65536) ? 3 : 2);
> >> +
> >> + priv->ctrl_cache = addr_cycles << BM_CTRL_ADDR_CYCLE1_S
> >> + | BM_CTRL_PAGE_SIZE(mtd->writesize) << BM_CTRL_PAGE_SIZE_S
> >> + | BM_CTRL_BLOCK_SIZE(pages_per_block) << BM_CTRL_BLOCK_SIZE_S
> >> + | BM_CTRL_INT_EN
> >> + | addr_cycles << BM_CTRL_ADDR_CYCLE0_S;
> >> +
> >> + iowrite32(BM_ECC_CAPn(nand->ecc.strength) << BM_ECC_CAP_S,
> >> + priv->base + HW_ECC_CTRL);
> >> +
> >> + iowrite32(mtd->writesize + priv->spare_size,
> >> + priv->base + HW_ECC_OFFSET);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static unsigned long __init clk_get_cyc_from_ns(struct clk *clk,
> >> + unsigned long ns)
> >> +{
> >> + unsigned int cycle;
> >> +
> >> + cycle = NSEC_PER_SEC / clk_get_rate(clk);
> >> + return DIV_ROUND_CLOSEST(ns, cycle);
> >> +}
> >> +
> >> +static void __init asm9260_nand_timing_config(struct asm9260_nand_priv *priv)
> >> +{
> >> + struct nand_chip *nand = &priv->nand;
> >> + const struct nand_sdr_timings *time;
> >> + u32 twhr, trhw, trwh, trwp, tadl, tccs, tsync, trr, twb;
> >> + int mode;
> >> +
> >
> > Maybe you should add support for ONFI timing mode retrieval with
> > onfi_get_async_timing_mode.
>
> instead of nand->onfi_timing_mode_default?
No you should check both: first try to retrieve the onfi timing mode
with the onfi_get_async_timing_mode function and it it returns an error
use onfi_timing_mode_default.
>
> >> + mode = nand->onfi_timing_mode_default;
> >> + dev_info(priv->dev, "ONFI timing mode: %i\n", mode);
> >> +
> >> + time = onfi_async_timing_mode_to_sdr_timings(mode);
> >> + if (IS_ERR_OR_NULL(time)) {
> >> + dev_err(priv->dev, "Can't get onfi_timing_mode: %i\n", mode);
> >> + return;
> >> + }
> >> +
> >> + trwh = clk_get_cyc_from_ns(priv->clk, time->tWH_min / 1000);
> >> + trwp = clk_get_cyc_from_ns(priv->clk, time->tWP_min / 1000);
> >> +
> >> + iowrite32((trwh << 4) | (trwp), priv->base + HW_TIMING_ASYN);
> >> +
> >> + twhr = clk_get_cyc_from_ns(priv->clk, time->tWHR_min / 1000);
> >> + trhw = clk_get_cyc_from_ns(priv->clk, time->tRHW_min / 1000);
> >> + tadl = clk_get_cyc_from_ns(priv->clk, time->tADL_min / 1000);
> >> + /* tCCS - change read/write collumn. Time between last cmd and data. */
> >> + tccs = clk_get_cyc_from_ns(priv->clk,
> >> + (time->tCLR_min + time->tCLH_min + time->tRC_min)
> >> + / 1000);
> >> +
> >> + iowrite32((twhr << 24) | (trhw << 16)
> >> + | (tadl << 8) | (tccs), priv->base + HW_TIM_SEQ_0);
> >> +
> >> + trr = clk_get_cyc_from_ns(priv->clk, time->tRR_min / 1000);
> >> + tsync = 0; /* ??? */
> >> + twb = clk_get_cyc_from_ns(priv->clk, time->tWB_max / 1000);
> >> + iowrite32((tsync << 16) | (trr << 9) | (twb),
> >> + priv->base + HW_TIM_SEQ_1);
> >> +}
> >> +
> >> +static int __init asm9260_nand_ecc_conf(struct asm9260_nand_priv *priv)
> >> +{
> >> + struct device_node *np = priv->dev->of_node;
> >> + struct nand_chip *nand = &priv->nand;
> >> + struct mtd_info *mtd = &priv->mtd;
> >> + struct nand_ecclayout *ecc_layout = &priv->ecc_layout;
> >> + int i, ecc_strength;
> >> +
> >> + nand->ecc.mode = of_get_nand_ecc_mode(np);
> >> + switch (nand->ecc.mode) {
> >> + case NAND_ECC_SOFT:
> >> + case NAND_ECC_SOFT_BCH:
> >> + dev_info(priv->dev, "Using soft ECC %i\n", nand->ecc.mode);
> >> + /* nand_base will find needed settings */
> >> + return 0;
> >> + case NAND_ECC_HW:
> >> + default:
> >> + dev_info(priv->dev, "Using default NAND_ECC_HW\n");
> >> + nand->ecc.mode = NAND_ECC_HW;
> >> + break;
> >> + }
> >> +
> >> + ecc_strength = of_get_nand_ecc_strength(np);
> >> + nand->ecc.size = ASM9260_ECC_STEP;
> >> + nand->ecc.steps = mtd->writesize / nand->ecc.size;
> >> +
> >> + if (ecc_strength < nand->ecc_strength_ds) {
> >> + int ds_corr;
> >> +
> >> + /* Let's check if ONFI can help us. */
> >> + if (nand->ecc_strength_ds <= 0) {
> >
> > Actually this is not necessarily filled by ONFI parameters (it can be
> > statically defined in the nand_ids table).
>
> Should i sepcify it by dev_err or enough to say it in comment?
Keep your error message, just change its content.
>
> >> + /* No ONFI and no DT - it is bad. */
> >> + dev_err(priv->dev,
> >> + "nand-ecc-strength is not set by DT or ONFI. Please set nand-ecc-strength in DT or add chip quirk in nand_ids.c.\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + ds_corr = (mtd->writesize * nand->ecc_strength_ds) /
> >> + nand->ecc_step_ds;
> >> + ecc_strength = ds_corr / nand->ecc.steps;
> >> + dev_info(priv->dev, "ONFI:nand-ecc-strength = %i\n",
> >> + ecc_strength);
> >> + } else
> >> + dev_info(priv->dev, "DT:nand-ecc-strength = %i\n",
> >> + ecc_strength);
> >> +
> >> + if (ecc_strength == 0 || ecc_strength > ASM9260_ECC_MAX_BIT) {
> >> + dev_err(priv->dev,
> >> + "Not supported ecc_strength value: %i\n",
> >> + ecc_strength);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (ecc_strength & 0x1) {
> >> + ecc_strength++;
> >> + dev_info(priv->dev,
> >> + "Only even ecc_strength value is supported. Recalculating: %i\n",
> >> + ecc_strength);
> >> + }
> >> +
> >> + /* FIXME: do we have max or min page size? */
> >> +
> >> + /* 13 - the smallest integer for 512 (ASM9260_ECC_STEP). Div to 8bit. */
> >> + nand->ecc.bytes = DIV_ROUND_CLOSEST(ecc_strength * 13, 8);
> >> +
> >> + ecc_layout->eccbytes = nand->ecc.bytes * nand->ecc.steps;
> >> + nand->ecc.layout = ecc_layout;
> >> + nand->ecc.strength = ecc_strength;
> >> +
> >> + priv->spare_size = mtd->oobsize - ecc_layout->eccbytes;
> >> +
> >> + ecc_layout->oobfree[0].offset = 2;
> >> + ecc_layout->oobfree[0].length = priv->spare_size - 2;
> >> +
> >> + /* FIXME: can we use same layout as SW_ECC? */
> >
> > It depends on what your controller is capable of. If you can define the
> > offset at which you write the ECC bytes, then yes you can reuse the
> > same kind of layout used in SW_ECC.
> >
> >> + for (i = 0; i < ecc_layout->eccbytes; i++)
> >> + ecc_layout->eccpos[i] = priv->spare_size + i;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int __init asm9260_nand_get_dt_clks(struct asm9260_nand_priv *priv)
> >> +{
> >> + int clk_idx = 0, err;
> >> +
> >> + priv->clk = devm_clk_get(priv->dev, "sys");
> >> + if (IS_ERR(priv->clk))
> >> + goto out_err;
> >> +
> >> + /* configure AHB clock */
> >> + clk_idx = 1;
> >> + priv->clk_ahb = devm_clk_get(priv->dev, "ahb");
> >> + if (IS_ERR(priv->clk_ahb))
> >> + goto out_err;
> >> +
> >> + err = clk_prepare_enable(priv->clk_ahb);
> >> + if (err)
> >> + dev_err(priv->dev, "Failed to enable ahb_clk!\n");
> >> +
> >> + err = clk_set_rate(priv->clk, clk_get_rate(priv->clk_ahb));
> >> + if (err) {
> >> + clk_disable_unprepare(priv->clk_ahb);
> >> + dev_err(priv->dev, "Failed to set rate!\n");
> >> + }
> >> +
> >> + err = clk_prepare_enable(priv->clk);
> >> + if (err) {
> >> + clk_disable_unprepare(priv->clk_ahb);
> >> + dev_err(priv->dev, "Failed to enable clk!\n");
> >> + }
> >> +
> >> + return 0;
> >> +out_err:
> >> + dev_err(priv->dev, "%s: Failed to get clk (%i)\n", __func__, clk_idx);
> >> + return 1;
> >> +}
> >> +
> >> +static int __init asm9260_nand_probe(struct platform_device *pdev)
> >> +{
> >> + struct asm9260_nand_priv *priv;
> >> + struct nand_chip *nand;
> >> + struct mtd_info *mtd;
> >> + struct device_node *np = pdev->dev.of_node;
> >> + struct resource *r;
> >> + int ret, i;
> >> + unsigned int irq;
> >> + u32 val;
> >> +
> >> + priv = devm_kzalloc(&pdev->dev, sizeof(struct asm9260_nand_priv),
> >> + GFP_KERNEL);
> >> + if (!priv)
> >> + return -ENOMEM;
> >> +
> >> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> + priv->base = devm_ioremap_resource(&pdev->dev, r);
> >> + if (!priv->base) {
> >> + dev_err(&pdev->dev, "Unable to map resource!\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + priv->dev = &pdev->dev;
> >> + nand = &priv->nand;
> >> + nand->priv = priv;
> >> +
> >> + platform_set_drvdata(pdev, priv);
> >> + mtd = &priv->mtd;
> >> + mtd->priv = nand;
> >> + mtd->owner = THIS_MODULE;
> >> + mtd->name = dev_name(&pdev->dev);
> >> +
> >> + if (asm9260_nand_get_dt_clks(priv))
> >> + return -ENODEV;
> >> +
> >> + irq = platform_get_irq(pdev, 0);
> >> + if (!irq)
> >> + return -ENODEV;
> >> +
> >> + iowrite32(0, priv->base + HW_INT_MASK);
> >> + ret = devm_request_irq(priv->dev, irq, asm9260_nand_irq,
> >> + IRQF_ONESHOT | IRQF_SHARED,
> >> + dev_name(&pdev->dev), priv);
> >> +
> >> + asm9260_nand_init_chip(nand);
> >> +
> >> + ret = of_property_read_u32(np, "nand-max-chips", &val);
> >> + if (ret)
> >> + val = 1;
> >> +
> >> + if (val > ASM9260_MAX_CHIPS) {
> >> + dev_err(&pdev->dev, "Unsupported nand-max-chips value: %i\n",
> >> + val);
> >> + return -ENODEV;
> >> + }
> >> +
> >> + for (i = 0; i < val; i++)
> >> + priv->mem_mask |= BM_CTRL_MEM0_RDY << i;
> >
> > If you want to support multiple NAND chips, then I recommend to rework
> > the DT representation and to define a asm9260_nand_controller struct:
> >
> > struct asm9260_nand_controller {
> > struct nand_hw_control base;
> >
> > /* asm9260 related stuff */
> > };
> >
> > Take a look [2] and [3].
>
>
> need to tak clother look. Right now i don't understand meaning of struct
> nand_hw_control.
It is there to represent an NAND flash controller, and is mainly useful
to lock access to the controller so that only one chip can be accessed
through the controller at a given time.
>
> >
> >> +
> >> + ret = nand_scan_ident(mtd, val, NULL);
> >> + if (ret) {
> >> + dev_err(&pdev->dev, "scan_ident filed!\n");
> >> + return ret;
> >> + }
> >> +
> >> + if (of_get_nand_on_flash_bbt(np)) {
> >> + dev_info(&pdev->dev, "Use On Flash BBT\n");
> >> + nand->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB_BBM
> >> + | NAND_BBT_LASTBLOCK;
> >> + }
> >> +
> >> + asm9260_nand_timing_config(priv);
> >> + asm9260_nand_ecc_conf(priv);
> >> + ret = asm9260_nand_cached_config(priv);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* second phase scan */
> >> + if (nand_scan_tail(mtd)) {
> >> + dev_err(&pdev->dev, "scan_tail filed!\n");
> >> + return -ENXIO;
> >> + }
> >> +
> >> +
> >> + ret = mtd_device_parse_register(mtd, NULL,
> >> + &(struct mtd_part_parser_data) {
> >> + .of_node = pdev->dev.of_node,
> >> + },
> >> + NULL, 0);
> >
> > Ergh, can you please define a local variable to replace this ugly
> > mtd_part_parser_data definition ?
>
> why?
>
Because I find it hard to read, but maybe I'm the only one to think
that way.
How about the following syntax:
struct mtd_part_parser_data ppdata = {
.of_node = pdev->dev.of_node,
};
[...]
ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
Regards,
Boris
[1]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L3052
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
More information about the linux-mtd
mailing list