Skip to content
  1. Jun 20, 2017
    • Masahiro Yamada's avatar
      mtd: nand: denali: fix raw and oob accessors for syndrome page layout · 26d266e1
      Masahiro Yamada authored
      
      
      The Denali IP adopts the syndrome page layout; payload and ECC are
      interleaved, with BBM area always placed at the beginning of OOB.
      
      The figure below shows the page organization for ecc->steps == 2:
      
        |----------------|    |-----------|
        |                |    |           |
        |                |    |           |
        |    Payload0    |    |           |
        |                |    |           |
        |                |    |           |
        |                |    |           |
        |----------------|    |  in-band  |
        |      ECC0      |    |   area    |
        |----------------|    |           |
        |                |    |           |
        |                |    |           |
        |    Payload1    |    |           |
        |                |    |           |
        |                |    |           |
        |----------------|    |-----------|
        |      BBM       |    |           |
        |----------------|    |           |
        |Payload1 (cont.)|    |           |
        |----------------|    |out-of-band|
        |      ECC1      |    |    area   |
        |----------------|    |           |
        |    OOB free    |    |           |
        |----------------|    |-----------|
      
      The current raw / oob accessors do not take that into consideration,
      so in-band and out-of-band data are transferred as stored in the
      device.  In the case above,
      
        in-band:      Payload0 + ECC0 + Payload1(partial)
        out-of-band:  BBM + Payload1(cont.) + ECC1 + OOB-free
      
      This is wrong.  As the comment block of struct nand_ecc_ctrl says,
      driver callbacks must hide the specific layout used by the hardware
      and always return contiguous in-band and out-of-band data.
      
      The current implementation is completely screwed-up, so read/write
      callbacks must be re-worked.
      
      Also, it is reasonable to support PIO transfer in case DMA may not
      work for some reasons.  Actually, the Data DMA may not be equipped
      depending on the configuration of the RTL.  This can be checked by
      reading the bit 4 of the FEATURES register.  Even if the controller
      has the DMA support, dma_set_mask() and dma_map_single() could fail.
      In either case, the driver can fall back to the PIO transfer.  Slower
      access would be better than giving up.
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      26d266e1
    • Masahiro Yamada's avatar
      mtd: nand: denali: use flag instead of register macro for direction · 96a376bd
      Masahiro Yamada authored
      
      
      It is not a good idea to re-use macros that represent a specific
      register bit field for the transfer direction.
      
      It is true that bit 8 indicates the direction for the MAP10 pipeline
      operation and the data DMA operation, but this is not valid across
      the IP.
      
      Use a simple flag (write: 1, read: 0) for the direction.
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      96a376bd
    • Masahiro Yamada's avatar
      mtd: nand: denali: merge struct nand_buf into struct denali_nand_info · 00fc615f
      Masahiro Yamada authored
      
      
      Now struct nand_buf has only two members, so I see no reason for the
      separation.
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      00fc615f
    • Masahiro Yamada's avatar
      mtd: nand: denali: propagate page to helpers via function argument · 2291cb89
      Masahiro Yamada authored
      
      
      This driver stores the currently addressed page into denali->page,
      which is later read out by helper functions.  While I am tackling on
      this driver, I often missed to insert "denali->page = page;" where
      needed.  This makes page_read/write callbacks to get access to a
      wrong page, which is a bug hard to figure out.
      
      Instead, I'd rather pass the page via function argument because the
      compiler's prototype checks will help to detect bugs.
      
      For the same reason, propagate dma_addr to the DMA helpers instead
      of denali->buf.dma_buf .
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      2291cb89
    • Masahiro Yamada's avatar
      mtd: nand: denali: use interrupt instead of polling for bank reset · d49f5790
      Masahiro Yamada authored
      
      
      The current bank reset implementation polls the INTR_STATUS register
      until interested bits are set.  This is not good because:
      
      - polling simply wastes time-slice of the thread
      
      - The while() loop may continue eternally if no bit is set, for
        example, due to the controller problem.  The denali_wait_for_irq()
        uses wait_for_completion_timeout(), which is safer.
      
      We can use interrupt by moving the denali_reset_bank() call below
      the interrupt setup.
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      d49f5790
    • Masahiro Yamada's avatar
      mtd: nand: denali: fix bank reset function to detect the number of chips · f486287d
      Masahiro Yamada authored
      
      
      The nand_scan_ident() iterates over maxchips, and calls nand_reset()
      for each.  This driver currently passes the maximum number of banks
      (=chip selects) supported by the controller as maxchips.  So, maxchips
      is typically 4 or 8.  Usually, less number of NAND chips are connected
      to the controller.
      
      This can be a problem for ONFi devices.  Now, this driver implements
      ->setup_data_interface() hook, so nand_setup_data_interface() issues
      Set Features (0xEF) command, which waits until the chip returns R/B#
      response.  If no chip there, we know it never happens, but the driver
      still ends up with waiting for a long time.  It will finally bail-out
      with timeout error and the driver will work with existing chips, but
      unnecessary wait will give a bad user experience.
      
      The denali_nand_reset() polls the INTR__RST_COMP and INTR__TIME_OUT
      bits, but they are always set even if not NAND chip is connected to
      that bank.  To know the chip existence, INTR__INT_ACT bit must be
      checked; this flag is set only when R/B# is toggled.  Since the Reset
      (0xFF) command toggles the R/B# pin, this can be used to know the
      actual number of chips, and update denali->max_banks.
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      f486287d
    • Masahiro Yamada's avatar
      mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc · fa6134e5
      Masahiro Yamada authored
      The NAND_CMD_SET_FEATURES support is missing from denali_cmdfunc().
      We also see /* TODO: Read OOB data */ comment.
      
      It would be possible to add more commands along with the current
      implementation, but having ->cmd_ctrl() seems a better approach from
      the discussion with Boris [1].
      
      Rely on the default ->cmdfunc() from the framework and implement the
      driver's own ->cmd_ctrl().
      
      This transition also fixes NAND_CMD_STATUS and NAND_CMD_PARAM handling.
      NAND_CMD_STATUS was just faked by the register read, so the only valid
      bit was the WP bit.  NAND_CMD_PARAM was completely broken; not only the
      command sent on the bus was NAND_CMD_STATUS instead of NAND_CMD_PARAM,
      but also the driver was only reading 8 bytes, while the parameter page
      contains several hundreds of bytes.
      
      Also add ->write_byte(), which is needed for write direction commands,
      ->read/write_buf(16), which will be used some commits later.
      ->read_word() is not used for now, but the core may call it in the
      future.
      
      Now, this driver can drop nand_onfi_get_set_features_notsupp().
      
      [1] https://lkml.org/lkml/2017/3/15/97
      
      
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      fa6134e5
    • Masahiro Yamada's avatar
      mtd: nand: denali: rework interrupt handling · c19e31d0
      Masahiro Yamada authored
      
      
      Simplify the interrupt handling and fix issues:
      
      - The register field view of INTR_EN / INTR_STATUS is different
        among IP versions.  The global macro DENALI_IRQ_ALL is hard-coded
        for Intel platforms.  The interrupt mask should be determined at
        run-time depending on the running platform.
      
      - wait_for_irq() loops do {} while() until interested flags are
        asserted.  The logic can be simplified.
      
      - The spin_lock() guard seems too complex (and suspicious in a race
        condition if wait_for_completion_timeout() bails out by timeout).
      
      - denali->complete is reused again and again, but reinit_completion()
        is missing.  Add it.
      
      Re-work the code to make it more robust and easier to handle.
      
      While we are here, also rename the jump label "failed_req_irq" to
      more appropriate "disable_irq".
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      c19e31d0
    • Masahiro Yamada's avatar
      mtd: nand: denali: handle timing parameters by setup_data_interface() · 1bb88666
      Masahiro Yamada authored
      Handling timing parameters in a driver's own way should be avoided
      because it duplicates efforts of drivers/mtd/nand/nand_timings.c
      Besides, this driver hard-codes Intel specific parameters such as
      CLK_X=5, CLK_MULTI=4.  Taking a certain device (Samsung K9WAG08U1A)
      into account by get_samsung_nand_para() is weird as well.
      
      Now, the core framework provides .setup_data_interface() hook, which
      handles timing parameters in a generic manner.
      
      While I am working on this, I found even more issues in the current
      code, so fixed the following as well:
      
      - In recent IP versions, WE_2_RE and TWHR2 share the same register.
        Likewise for ADDR_2_DATA and TCWAW, CS_SETUP_CNT and TWB.  When
        updating one, the other must be masked.  Otherwise, the other will
        be set to 0, then timing settings will be broken.
      
      - The recent IP release expanded the ADDR_2_DATA to 7-bit wide.
        This register is related to tADL.  As commit 74a332e7
      
       ("mtd:
        nand: timings: Fix tADL_min for ONFI 4.0 chips") addressed, the
        ONFi 4.0 increased the minimum of tADL to 400 nsec.  This may not
        fit in the 6-bit ADDR_2_DATA in older versions.  Check the IP
        revision and handle this correctly, otherwise the register value
        would wrap around.
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      1bb88666
    • Masahiro Yamada's avatar
      mtd: nand: denali: remove unneeded find_valid_banks() · 959e9f2a
      Masahiro Yamada authored
      
      
      The function find_valid_banks() issues the Read ID (0x90) command,
      then compares the first byte (Manufacturer ID) of each bank with
      the one of bank0.
      
      This is equivalent to what nand_scan_ident() does.  The number of
      chips is detected there, so this is unneeded.
      
      What is worse for find_valid_banks() is that, if multiple chips are
      connected to INTEL_CE4100 platform, it crashes the kernel by BUG().
      This is what we should avoid.  This function is just harmful and
      unneeded.
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      959e9f2a
    • Masahiro Yamada's avatar
      mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS · b21ff825
      Masahiro Yamada authored
      
      
      The denali_cmdfunc() actually does nothing valuable for
      NAND_CMD_{PAGEPROG,READ0,SEQIN}.
      
      For NAND_CMD_{READ0,SEQIN}, it copies "page" to "denali->page", then
      denali_read_page(_raw) compares them just for the sanity check.
      (Inconsistently, this check is missing from denali_write_page(_raw).)
      
      The Denali controller is equipped with high level read/write interface,
      so let's skip unneeded call of cmdfunc().
      
      If NAND_ECC_CUSTOM_PAGE_ACCESS is set, nand_write_page() will not
      call ->waitfunc hook.  So, ->write_page(_raw) hooks should directly
      return -EIO on failure.  The error handling of page writes will be
      much simpler.
      
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      b21ff825
  2. Jun 13, 2017
  3. Jun 10, 2017
  4. Jun 01, 2017