1. 20 Jun, 2017 16 commits
    • Masahiro Yamada's avatar
      mtd: nand: denali: avoid magic numbers and rename for clarification · 0d3a966d
      Masahiro Yamada authored
      Introduce some macros and helpers to avoid magic numbers and
      rename macros/functions for clarification.
      
      - We see '| 2' in several places.  This means Data Cycle in MAP11 mode.
        The Denali User's Guide says bit[1:0] of MAP11 is like follows:
      
        b'00 = Command Cycle
        b'01 = Address Cycle
        b'10 = Data Cycle
      
        So, this commit added DENALI_MAP11_{CMD,ADDR,DATA} macros.
      
      - We see 'denali->flash_mem + 0x10' in several places, but 0x10 is a
        magic number.  Actually, this accesses the data port of the Host
        Data/Command Interface.  So, this commit added DENALI_HOST_DATA.
        On the other hand, 'denali->flash_mem' gets access to the address
        port, so DENALI_HOST_ADDR was also added.
      
      - We see 'index_addr(denali, cmd, 0x1)' in denali_erase(), but 0x1
        is a magic number.  0x1 means the erase operation.  Replace 0x1
        with DENALI_ERASE.
      
      - Rename index_addr() to denali_host_write() for clarification
      
      - Denali User's Guide says MAP{00,01,10,11} for access mode.  Match
        the macros with terminology in the IP document.
      
      - Rename struct members as follows:
        flash_bank   -> active_bank    (currently selected bank)
        flash_reg    -> reg            (base address of registers)
        flash_mem    -> host           (base address of host interface)
        devnum       -> devs_per_cs    (devices connected in parallel)
        bbtskipbytes -> oob_skip_bytes (number of bytes to skip in OOB)
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      0d3a966d
    • Masahiro Yamada's avatar
      mtd: nand: denali: enable bad block table scan · 777f2d49
      Masahiro Yamada authored
      Now this driver is ready to remove NAND_SKIP_BBTSCAN.
      
      The BBT descriptors in denali.c are equivalent to the ones in
      nand_bbt.c.  There is no need to duplicate the equivalent structures.
      The with-oob decriptors do not work for this driver anyway.
      
      The bbt_pattern (offs = 8) and the version (veroffs = 12) area
      overlaps the ECC area.  Set NAND_BBT_NO_OOB flag to use the no_oob
      variant of the BBT descriptors.
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      777f2d49
    • Masahiro Yamada's avatar
      mtd: nand: denali: use non-managed kmalloc() for DMA buffer · 7d370b2c
      Masahiro Yamada authored
      As Russell and Lars stated in the discussion [1], using
      devm_k*alloc() with DMA is not a good idea.
      
      Let's use kmalloc (not kzalloc because no need for zero-out).
      Also, allocate the buffer as late as possible because it must be
      freed for any error that follows.
      
      [1] https://lkml.org/lkml/2017/3/8/693Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Cc: Russell King <rmk+kernel@armlinux.org.uk>
      Cc: Lars-Peter Clausen <lars@metafoo.de>
      Acked-by: default avatarRobin Murphy <robin.murphy@arm.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      7d370b2c
    • Masahiro Yamada's avatar
      mtd: nand: denali: skip driver internal bounce buffer when possible · 997cde2a
      Masahiro Yamada authored
      For ecc->read_page() and ecc->write_page(), it is possible to call
      dma_map_single() against the given buffer.  This bypasses the driver
      internal bounce buffer and save the memcpy().
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      997cde2a
    • Masahiro Yamada's avatar
      mtd: nand: denali: support hardware-assisted erased page detection · 57a4d8b5
      Masahiro Yamada authored
      Recent versions of this IP support automatic erased page detection.
      If an erased page is detected on reads, the controller does not set
      INTR__ECC_UNCOR_ERR, but INTR__ERASED_PAGE.
      
      The detection of erased pages is based on the number of zeros in a
      page; if the number of zeros is less than the value in the field
      ERASED_THRESHOLD, the page is assumed as erased.
      
      Please note ERASED_THRESHOLD specifies the number of zeros in a _page_
      instead of an ECC chunk.  Moreover, the controller does not provide a
      way to know the actual number of bitflips.
      
      Actually, an erased page (all 0xff) is not an ECC correctable pattern
      on the Denali ECC engine.  In other words, there may be overlap between
      the following two:
      
      [1] a bit pattern reachable from a valid payload + ECC pattern within
          ecc.strength bitflips
      [2] a bit pattern reachable from an erased state (all 0xff) within
          ecc.strength bitflips
      
      So, this feature may intercept ECC correctable patterns, then replace
      [1] with [2].
      
      After all, this feature can work safely only when ECC_THRESHOLD == 1,
      i.e. detect erased pages without any bitflips.  This should be the
      case most of the time.  If there is a bitflip or more, the driver will
      fallback to the software method by using nand_check_erased_ecc_chunk().
      
      Strangely enough, the driver still has to fill the buffer with 0xff
      in case of INTR__ERASED_PAGE because the ECC correction engine has
      already manipulated the data in the buffer before it judges erased
      pages.
      Signed-off-by: default avatarMasahiro Yamada <yamada.masahiro@socionext.com>
      Signed-off-by: default avatarBoris Brezillon <boris.brezillon@free-electrons.com>
      57a4d8b5
    • 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/97Signed-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. 13 Jun, 2017 1 commit
  3. 10 Jun, 2017 11 commits
  4. 01 Jun, 2017 12 commits