Message ID | 57DFE444.3000003@sigmadesigns.com |
---|---|
State | Superseded |
Headers | show |
On 19/09/2016 15:12, Marc Gonzalez wrote: > Tests done to validate this driver: > mtd_stresstest dev=1 count=500000 # time modprobe mtd_stresstest dev=1 count=500000 [ 5872.077247] [ 5872.078768] ================================================= [ 5872.084628] mtd_stresstest: MTD device: 1 [ 5872.088707] mtd_stresstest: MTD device size 536870912, eraseblock size 131072, page size 2048, count of eraseblocks 4096, pages per eraseblock 64, OOB size 64 [ 5872.104813] mtd_test: scanning for bad eraseblocks [ 5872.113166] mtd_test: scanned 4096 eraseblocks, 0 are bad [ 5872.118624] mtd_stresstest: doing operations [ 5872.122941] mtd_stresstest: 0 operations done [ 5883.636505] mtd_stresstest: 1024 operations done [ 5894.626418] mtd_stresstest: 2048 operations done [ 5904.911443] mtd_stresstest: 3072 operations done ... [10740.794754] mtd_stresstest: 499712 operations done [10743.412941] mtd_stresstest: finished, 500000 operations done [10743.418765] ================================================= real 81m11.354s user 0m0.000s sys 51m46.666s > mtd_nandbiterrs dev=1 # modprobe mtd_nandbiterrs dev=1 [11713.669415] [11713.670936] ================================================== [11713.677186] mtd_nandbiterrs: MTD device: 1 [11713.681442] mtd_nandbiterrs: MTD device size 536870912, eraseblock=131072, page=2048, oob=64 [11713.690045] mtd_nandbiterrs: Device uses 1 subpages of 2048 bytes [11713.696249] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0 [11713.703241] mtd_nandbiterrs: incremental biterrors test [11713.708608] mtd_nandbiterrs: write_page [11713.712783] mtd_nandbiterrs: rewrite page [11713.717164] mtd_nandbiterrs: read_page [11713.721166] mtd_nandbiterrs: verify_page [11713.725180] mtd_nandbiterrs: Successfully corrected 0 bit errors per subpage [11713.732360] mtd_nandbiterrs: Inserted biterror @ 0/5 [11713.737403] mtd_nandbiterrs: rewrite page [11713.741760] mtd_nandbiterrs: read_page [11713.745749] mtd_nandbiterrs: verify_page [11713.749805] mtd_nandbiterrs: Successfully corrected 1 bit errors per subpage [11713.756947] mtd_nandbiterrs: Inserted biterror @ 0/2 [11713.761990] mtd_nandbiterrs: rewrite page [11713.766340] mtd_nandbiterrs: read_page [11713.770331] mtd_nandbiterrs: verify_page [11713.774382] mtd_nandbiterrs: Successfully corrected 2 bit errors per subpage [11713.781524] mtd_nandbiterrs: Inserted biterror @ 0/0 [11713.786565] mtd_nandbiterrs: rewrite page [11713.790917] mtd_nandbiterrs: read_page [11713.794881] mtd_nandbiterrs: verify_page [11713.798887] mtd_nandbiterrs: Successfully corrected 3 bit errors per subpage [11713.805985] mtd_nandbiterrs: Inserted biterror @ 1/7 [11713.810985] mtd_nandbiterrs: rewrite page [11713.815299] mtd_nandbiterrs: read_page [11713.819240] mtd_nandbiterrs: verify_page [11713.823253] mtd_nandbiterrs: Successfully corrected 4 bit errors per subpage [11713.830351] mtd_nandbiterrs: Inserted biterror @ 1/5 [11713.835350] mtd_nandbiterrs: rewrite page [11713.839654] mtd_nandbiterrs: read_page [11713.843601] mtd_nandbiterrs: verify_page [11713.847608] mtd_nandbiterrs: Successfully corrected 5 bit errors per subpage [11713.854705] mtd_nandbiterrs: Inserted biterror @ 1/2 [11713.859700] mtd_nandbiterrs: rewrite page [11713.864018] mtd_nandbiterrs: read_page [11713.867961] mtd_nandbiterrs: verify_page [11713.871972] mtd_nandbiterrs: Successfully corrected 6 bit errors per subpage [11713.879071] mtd_nandbiterrs: Inserted biterror @ 1/0 [11713.884069] mtd_nandbiterrs: rewrite page [11713.888375] mtd_nandbiterrs: read_page [11713.892321] mtd_nandbiterrs: verify_page [11713.896327] mtd_nandbiterrs: Successfully corrected 7 bit errors per subpage [11713.903424] mtd_nandbiterrs: Inserted biterror @ 2/6 [11713.908421] mtd_nandbiterrs: rewrite page [11713.912731] mtd_nandbiterrs: read_page [11713.916671] mtd_nandbiterrs: verify_page [11713.920681] mtd_nandbiterrs: Successfully corrected 8 bit errors per subpage [11713.927788] mtd_nandbiterrs: Inserted biterror @ 2/5 [11713.932787] mtd_nandbiterrs: rewrite page [11713.937092] mtd_nandbiterrs: read_page [11713.941038] mtd_nandbiterrs: verify_page [11713.945046] mtd_nandbiterrs: Successfully corrected 9 bit errors per subpage [11713.952143] mtd_nandbiterrs: Inserted biterror @ 2/2 [11713.957138] mtd_nandbiterrs: rewrite page [11713.961448] mtd_nandbiterrs: read_page [11713.965388] mtd_nandbiterrs: verify_page [11713.969402] mtd_nandbiterrs: Successfully corrected 10 bit errors per subpage [11713.976584] mtd_nandbiterrs: Inserted biterror @ 2/0 [11713.981582] mtd_nandbiterrs: rewrite page [11713.985886] mtd_nandbiterrs: read_page [11713.989832] mtd_nandbiterrs: verify_page [11713.993837] mtd_nandbiterrs: Successfully corrected 11 bit errors per subpage [11714.001022] mtd_nandbiterrs: Inserted biterror @ 3/7 [11714.006017] mtd_nandbiterrs: rewrite page [11714.010352] mtd_nandbiterrs: read_page [11714.014351] mtd_nandbiterrs: verify_page [11714.018371] mtd_nandbiterrs: Successfully corrected 12 bit errors per subpage [11714.025554] mtd_nandbiterrs: Inserted biterror @ 3/6 [11714.030557] mtd_nandbiterrs: rewrite page [11714.034867] mtd_nandbiterrs: read_page [11714.038817] mtd_nandbiterrs: verify_page [11714.042827] mtd_nandbiterrs: Successfully corrected 13 bit errors per subpage [11714.050012] mtd_nandbiterrs: Inserted biterror @ 3/5 [11714.055009] mtd_nandbiterrs: rewrite page [11714.059361] mtd_nandbiterrs: read_page [11714.063301] mtd_nandbiterrs: verify_page [11714.067312] mtd_nandbiterrs: Successfully corrected 14 bit errors per subpage [11714.074496] mtd_nandbiterrs: Inserted biterror @ 3/2 [11714.079495] mtd_nandbiterrs: rewrite page [11714.083801] mtd_nandbiterrs: read_page [11714.093403] mtd_nandbiterrs: error: read failed at 0x0 [11714.098578] mtd_nandbiterrs: After 15 biterrors per subpage, read reported error -74 [11714.106779] mtd_nandbiterrs: finished successfully. [11714.111691] ================================================== > ubiattach -m 1 -d 3 && runtests.sh /dev/ubi3 # time ./runtests.sh /dev/ubi3 Running mkvol_basic /dev/ubi3 Running mkvol_bad /dev/ubi3 [ 2319.279975] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request [ 2319.286844] Volume creation request dump: [ 2319.291238] vol_id -2 [ 2319.294023] alignment 1 [ 2319.296649] bytes 509427712 [ 2319.299956] vol_type 3 [ 2319.302498] name_len 22 [ 2319.305216] 1st 16 characters of name: mkvol_bad:test_m [ 2319.310672] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request [ 2319.317571] Volume creation request dump: [ 2319.321662] vol_id 128 [ 2319.324440] alignment 1 [ 2319.327042] bytes 509427712 [ 2319.330341] vol_type 3 [ 2319.332882] name_len 22 [ 2319.335594] 1st 16 characters of name: mkvol_bad:test_m [ 2319.341023] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request [ 2319.347871] Volume creation request dump: [ 2319.351912] vol_id 0 [ 2319.354466] alignment 0 [ 2319.357024] bytes 509427712 [ 2319.360277] vol_type 3 [ 2319.362825] name_len 22 [ 2319.365464] 1st 16 characters of name: mkvol_bad:test_m [ 2319.370837] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request [ 2319.377677] Volume creation request dump: [ 2319.381716] vol_id 0 [ 2319.384270] alignment -1 [ 2319.386910] bytes 509427712 [ 2319.390163] vol_type 3 [ 2319.392704] name_len 22 [ 2319.395343] 1st 16 characters of name: mkvol_bad:test_m [ 2319.400713] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request [ 2319.407554] Volume creation request dump: [ 2319.411593] vol_id 0 [ 2319.414148] alignment 126977 [ 2319.417139] bytes 509427712 [ 2319.420393] vol_type 3 [ 2319.422934] name_len 22 [ 2319.425573] 1st 16 characters of name: mkvol_bad:test_m [ 2319.430944] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request [ 2319.437783] Volume creation request dump: [ 2319.441822] vol_id 0 [ 2319.444377] alignment 2049 [ 2319.447193] bytes 509427712 [ 2319.450446] vol_type 3 [ 2319.452987] name_len 22 [ 2319.455627] 1st 16 characters of name: mkvol_bad:test_m [ 2319.460997] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request [ 2319.467843] Volume creation request dump: [ 2319.471883] vol_id 0 [ 2319.474438] alignment 1 [ 2319.476992] bytes -1 [ 2319.479620] vol_type 3 [ 2319.482174] name_len 22 [ 2319.484811] 1st 16 characters of name: mkvol_bad:test_m [ 2319.490188] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request [ 2319.497026] Volume creation request dump: [ 2319.501066] vol_id 0 [ 2319.503624] alignment 1 [ 2319.506164] bytes 0 [ 2319.508718] vol_type 3 [ 2319.511273] name_len 22 [ 2319.513912] 1st 16 characters of name: mkvol_bad:test_m [ 2319.519288] ubi3 error: ubi_create_volume [ubi]: not enough PEBs, only 4012 available [ 2319.527183] ubi3 error: ubi_create_volume [ubi]: cannot create volume 0, error -28 [ 2319.534830] ubi3 error: ubi_create_volume [ubi]: not enough PEBs, only 4012 available [ 2319.542721] ubi3 error: ubi_create_volume [ubi]: cannot create volume 0, error -28 [ 2319.550365] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request [ 2319.557205] Volume creation request dump: [ 2319.561244] vol_id 0 [ 2319.563800] alignment 1 [ 2319.566345] bytes 126976 [ 2319.569337] vol_type 7 [ 2319.571891] name_len 22 [ 2319.574531] 1st 16 characters of name: mkvol_bad:test_m [ 2319.688559] ubi3 error: ubi_create_volume [ubi]: volume 0 already exists [ 2319.695328] ubi3 error: ubi_create_volume [ubi]: cannot create volume 0, error -17 [ 2319.702987] ubi3 error: ubi_create_volume [ubi]: volume "mkvol_bad:test_mkvol()" exists (ID 0) [ 2319.711676] ubi3 error: ubi_create_volume [ubi]: cannot create volume 1, error -17 [ 2319.936067] ubi3 error: ubi_create_volume [ubi]: volume "mkvol_bad:test_mkvol()" exists (ID 0) [ 2319.944757] ubi3 error: ubi_create_volume [ubi]: cannot create volume 1, error -17 [ 2347.540770] ubi3 error: ubi_unregister_volume_notifier [ubi]: cannot open device 3, volume 128, error -22 [ 2347.550452] ubi3 error: ubi_unregister_volume_notifier [ubi]: cannot open device 3, volume -1, error -22 [ 2347.560458] ubi3 error: ubi_unregister_volume_notifier [ubi]: cannot open device 3, volume 128, error -22 [ 2347.570244] ubi3 error: ubi_unregister_volume_notifier [ubi]: cannot open device 3, volume 0, error -19 [ 2347.798164] ubi3 error: ubi_unregister_volume_notifier [ubi]: cannot open device 3, volume 0, error -19 Running mkvol_paral /dev/ubi3 Running rsvol /dev/ubi3 Running io_basic /dev/ubi3 Running io_read /dev/ubi3 Running io_update /dev/ubi3 Running io_paral /dev/ubi3 Running volrefcnt /dev/ubi3 SUCCESS real 40m16.730s user 11m33.653s sys 20m57.187s NB: Richard also suggested running a decent filesystem load using bonnie++
Hi Marc, On Mon, 19 Sep 2016 15:12:36 +0200 Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote: > This driver supports the NAND Flash controller embedded in recent > Tango chips, such as SMP8758 and SMP8759. > Please send another patch to document the DT bindings, and Cc the DT maintainers. > Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> > --- > Tests done to validate this driver: > mtd_stresstest dev=1 count=500000 > mtd_nandbiterrs dev=1 > ubiattach -m 1 -d 3 && runtests.sh /dev/ubi3 > --- > drivers/mtd/nand/Kconfig | 6 + > drivers/mtd/nand/Makefile | 1 + > drivers/mtd/nand/tango_nand.c | 555 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 562 insertions(+) > create mode 100644 drivers/mtd/nand/tango_nand.c > > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig > index f05e0e9eb2f7..22eb5457c9f7 100644 > --- a/drivers/mtd/nand/Kconfig > +++ b/drivers/mtd/nand/Kconfig > @@ -205,6 +205,12 @@ config MTD_NAND_S3C2410_CLKSTOP > when the is NAND chip selected or released, but will save > approximately 5mA of power when there is nothing happening. > > +config MTD_NAND_TANGO > + tristate "NAND Flash support for Tango chips" > + depends on ARCH_TANGO > + help > + Enables the NAND Flash controller on Tango chips. > + > config MTD_NAND_DISKONCHIP > tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND reimplementation)" > depends on HAS_IOMEM > diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile > index f55335373f7c..647f727223ef 100644 > --- a/drivers/mtd/nand/Makefile > +++ b/drivers/mtd/nand/Makefile > @@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_DENALI_DT) += denali_dt.o > obj-$(CONFIG_MTD_NAND_AU1550) += au1550nd.o > obj-$(CONFIG_MTD_NAND_BF5XX) += bf5xx_nand.o > obj-$(CONFIG_MTD_NAND_S3C2410) += s3c2410.o > +obj-$(CONFIG_MTD_NAND_TANGO) += tango_nand.o > obj-$(CONFIG_MTD_NAND_DAVINCI) += davinci_nand.o > obj-$(CONFIG_MTD_NAND_DISKONCHIP) += diskonchip.o > obj-$(CONFIG_MTD_NAND_DOCG4) += docg4.o > diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > new file mode 100644 > index 000000000000..ec18cad546fb > --- /dev/null > +++ b/drivers/mtd/nand/tango_nand.c > @@ -0,0 +1,555 @@ > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/clk.h> > +#include <linux/iopoll.h> > +#include <linux/module.h> > +#include <linux/mtd/nand.h> > +#include <linux/dmaengine.h> > +#include <linux/dma-mapping.h> > +#include <linux/platform_device.h> > + > +/* Offsets relative to chip->base */ > +#define PBUS_CMD 0 > +#define PBUS_ADDR 4 > +#define PBUS_DATA 8 > + > +/* Offsets relative to reg_base */ > +#define NFC_STATUS 0x00 > +#define NFC_FLASH_CMD 0x04 > +#define NFC_DEVICE_CFG 0x08 > +#define NFC_TIMING1 0x0c > +#define NFC_TIMING2 0x10 > +#define NFC_XFER_CFG 0x14 > +#define NFC_PKT_0_CFG 0x18 > +#define NFC_PKT_N_CFG 0x1c > +#define NFC_BB_CFG 0x20 > +#define NFC_ADDR_PAGE 0x24 > +#define NFC_ADDR_OFFSET 0x28 > +#define NFC_XFER_STATUS 0x2c > + > +/* NFC_STATUS values */ > +#define CMD_READY BIT(31) > + > +/* NFC_FLASH_CMD values */ > +#define NFC_READ 1 > +#define NFC_WRITE 2 > + > +/* NFC_XFER_STATUS values */ > +#define PAGE_IS_EMPTY BIT(16) > + > +/* Offsets relative to mem_base */ > +#define METADATA 0x000 > +#define ERROR_REPORT 0x1c0 > + > +/* > + * Error reports are split in two bytes: > + * byte 0 for the first packet in a page (PKT_0) > + * byte 1 for other packets in a page (PKT_N, for N > 0) > + * ERR_COUNT_PKT_N is the max error count over all but the first packet. > + */ > +#define DECODE_OK_PKT_0(v) (v & BIT(7)) > +#define DECODE_OK_PKT_N(v) (v & BIT(15)) > +#define ERR_COUNT_PKT_0(v) ((v >> 0) & 0x3f) > +#define ERR_COUNT_PKT_N(v) ((v >> 8) & 0x3f) > + > +/* Offsets relative to pbus_base */ > +#define PBUS_CS_CTRL 0x83c > +#define PBUS_PAD_MODE 0x8f0 > + > +/* PBUS_CS_CTRL values */ > +#define PBUS_IORDY BIT(31) > + > +/* > + * PBUS_PAD_MODE values > + * In raw mode, the driver communicates directly with the NAND chips. > + * In NFC mode, the NAND Flash controller manages the communication. > + * We use NFC mode for read and write; raw mode for everything else. > + */ > +#define MODE_RAW 0 > +#define MODE_NFC BIT(31) > + > +#define METADATA_SIZE 4 > +#define BBM_SIZE 6 > +#define FIELD_ORDER 15 > + > +#define MAX_CS 4 > + > +struct tango_nfc { > + struct nand_hw_control hw; > + void __iomem *reg_base; > + void __iomem *mem_base; > + void __iomem *pbus_base; > + struct tango_chip *chips[MAX_CS]; > + struct dma_chan *chan; > +}; > + > +#define to_tango_nfc(ptr) container_of(ptr, struct tango_nfc, hw) > + > +struct tango_chip { > + struct nand_chip chip; > + void __iomem *base; I'm still not convinced this is needed (->base can be calculated where needed based on the chip->cs and nfc->reg_base values), but if you want to keep it, let's keep it. > + u32 timing1; > + u32 timing2; > + u32 xfer_cfg; > + u32 pkt_0_cfg; > + u32 pkt_n_cfg; > + u32 bb_cfg; > + int cs; > +}; > + > +#define to_tango_chip(ptr) container_of(ptr, struct tango_chip, chip) > + > +#define XFER_CFG(cs, page_count, steps, metadata_size) \ > + ((cs) << 24 | (page_count) << 16 | (steps) << 8 | (metadata_size) << 0) > + > +#define PKT_CFG(size, strength) ((size) << 16 | (strength) << 0) > + > +#define BB_CFG(bb_offset, bb_size) ((bb_offset) << 16 | (bb_size) << 0) > + > +#define TIMING(t0, t1, t2, t3) (t0 << 24 | t1 << 16 | t2 << 8 | t3 << 0) > + > +static void tango_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl) > +{ > + struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd)); > + > + if (ctrl & NAND_CLE) > + writeb_relaxed(dat, chip->base + PBUS_CMD); > + > + if (ctrl & NAND_ALE) > + writeb_relaxed(dat, chip->base + PBUS_ADDR); > +} > + > +static int tango_dev_ready(struct mtd_info *mtd) > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + struct tango_nfc *nfc = to_tango_nfc(chip->controller); > + > + return readl_relaxed(nfc->pbus_base + PBUS_CS_CTRL) & PBUS_IORDY; > +} > + > +static uint8_t tango_read_byte(struct mtd_info *mtd) > +{ > + struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd)); > + > + return readb_relaxed(chip->base + PBUS_DATA); > +} > + > +static void tango_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) > +{ > + struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd)); > + > + ioread8_rep(chip->base + PBUS_DATA, buf, len); > +} > + > +static void tango_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) > +{ > + struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd)); > + > + iowrite8_rep(chip->base + PBUS_DATA, buf, len); > +} > + > +static void tango_select_chip(struct mtd_info *mtd, int idx) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + struct tango_nfc *nfc = to_tango_nfc(nand->controller); > + struct tango_chip *chip = to_tango_chip(nand); > + > + if (idx < 0) { > + chip->base = NULL; > + return; > + } > + > + chip->base = nfc->pbus_base + (chip->cs * 256); You support only one CS per chip, so this is something you can configure at init time. When I asked you to remove the chip->base field, I had multi-CS chips in mind, but given this implementation, there's no need to set/reset the chip->base field each time ->select_chip() is called. > + > + writel_relaxed(chip->timing1, nfc->reg_base + NFC_TIMING1); > + writel_relaxed(chip->timing2, nfc->reg_base + NFC_TIMING2); > + writel_relaxed(chip->xfer_cfg, nfc->reg_base + NFC_XFER_CFG); > + writel_relaxed(chip->pkt_0_cfg, nfc->reg_base + NFC_PKT_0_CFG); > + writel_relaxed(chip->pkt_n_cfg, nfc->reg_base + NFC_PKT_N_CFG); > + writel_relaxed(chip->bb_cfg, nfc->reg_base + NFC_BB_CFG); Nit: can you avoid these weird alignments. Use a single space after the comma. > +} > + > +static int decode_error_report(struct tango_nfc *nfc) > +{ > + u32 status, res; > + > + status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS); > + if (status & PAGE_IS_EMPTY) > + return 0; > + > + res = readl_relaxed(nfc->mem_base + ERROR_REPORT); > + > + if (DECODE_OK_PKT_0(res) && DECODE_OK_PKT_N(res)) > + return max(ERR_COUNT_PKT_0(res), ERR_COUNT_PKT_N(res)); > + > + return -EBADMSG; > +} > + > +static void tango_dma_callback(void *arg) > +{ > + complete(arg); > +} > + > +static int do_dma(struct tango_nfc *nfc, int dir, int cmd, > + const void *buf, int len, int page) > +{ > + struct dma_async_tx_descriptor *desc; > + struct scatterlist sg; > + struct completion tx_done; > + int err = -EIO; > + u32 res, val; > + > + sg_init_one(&sg, buf, len); > + if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1) > + goto leave; Return -EIO directly instead of creating this 'leave' label. > + > + desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT); Put DMA_PREP_INTERRUPT on a second line to avoid >80 char lines. > + if (!desc) > + goto dma_unmap; > + > + desc->callback = tango_dma_callback; > + desc->callback_param = &tx_done; > + init_completion(&tx_done); > + > + writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE); > + > + writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE); > + writel_relaxed( 0, nfc->reg_base + NFC_ADDR_OFFSET); > + writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD); > + > + dmaengine_submit(desc); > + dma_async_issue_pending(nfc->chan); > + > + res = wait_for_completion_timeout(&tx_done, HZ); > + if (res > 0) { > + void __iomem *addr = nfc->reg_base + NFC_STATUS; > + err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000); Why do you need this local variable? err = readl_poll_timeout(nfc->reg_base + NFC_STATUS, val, val & CMD_READY, 0, 1000); > + } > + > + writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE); Add an extra blank line here. > +dma_unmap: > + dma_unmap_sg(nfc->chan->device->dev, &sg, 1, dir); > +leave: > + return err; > +} > + > +static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip, > + uint8_t *buf, int oob_required, int page) > +{ > + struct tango_nfc *nfc = to_tango_nfc(chip->controller); > + int err, res, len = mtd->writesize; > + > + err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page); > + if (err) > + return err; > + > + if (oob_required) > + chip->ecc.read_oob(mtd, chip, page); > + > + res = decode_error_report(nfc); > + if (res >= 0) > + return res; > + > + chip->ecc.read_oob(mtd, chip, page); There's no different in your case, but it should be read in raw mode. How about calling ecc.read_oob_raw() so that you're safe even if you want to differentiate the read_oob() and read_oob_raw() cases at some point (IIUC, the METADATA section is ECC protected). > + res = nand_check_erased_ecc_chunk(buf, len, chip->oob_poi, mtd->oobsize, > + NULL, 0, chip->ecc.strength); You should check each ECC packet/chunk independently, because ECC strength is referring to an ECC packet not a full page. > + if (res < 0) > + mtd->ecc_stats.failed++; > + > + return res; > +} > + > +static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip, > + const uint8_t *buf, int oob_required, int page) > +{ > + struct tango_nfc *nfc = to_tango_nfc(chip->controller); > + int err, len = mtd->writesize; > + > + writel_relaxed(0xffffffff, nfc->mem_base + METADATA); > + err = do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, buf, len, page); > + if (err) > + return err; > + > + if (oob_required) > + chip->ecc.write_oob(mtd, chip, page); > + > + return 0; > +} > + > +enum { OP_SKIP, OP_READ, OP_WRITE }; > + > +static void raw_aux(struct mtd_info *mtd, u8 **buf, int len, int op, int *lim) Nit: please pass struct nand_chip *chip in first argument, and extract the mtd using nand_to_mtd(). I know it's just a detail, but I'd like to avoid passing mtd pointers, especially in internal functions. > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + int pos = mtd->writesize - *lim; > + > + if (op == OP_SKIP) > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, pos + len, -1); Okay, we already discussed that one, and I already showed that this was not working: NAND_CMD_RNDOUT is only valid for read accesses. And I told you I'd like to avoid the const to non-const cast in write accessors. > + else if (op == OP_READ) > + tango_read_buf(mtd, *buf, len); > + else if (op == OP_WRITE) > + tango_write_buf(mtd, *buf, len); > + > + *lim -= len; > + *buf += len; > +} I'm not sure factorizing this code is really important, but since you insist, how about doing the following instead: enum tango_raw_op { OP_READ, OP_WRITE }; union tango_raw_buffer { void *in; const void *out; }; struct tango_raw_access { enum tango_raw_op op; union tango_raw_buffer *buf; int pos; }; static void raw_aux(struct nand_chip *chip, struct tango_raw_access *info, union tango_raw_buffer *buf, int len) { struct mtd_info *mtd = nand_to_mtd(chip); if (!buf) { if (info->op == OP_READ) cmd = NAND_CMD_RNDOUT; else cmd = NAND_CMD_SEQIN; chip->cmdfunc(mtd, cmd, info->pos + len, -1); } else { if (info->op == OP_READ) tango_read_buf(mtd, buf->out, len); else tango_write_buf(mtd, buf->in, len); buf->in += len; } info->pos += len; } > + > +static int raw_access(struct mtd_info *mtd, uint8_t *buf, uint8_t *oob, int op) > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + int pkt_size = chip->ecc.size; > + int ecc_size = chip->ecc.bytes; > + int buf_op = buf ? op : OP_SKIP; > + int oob_op = oob ? op : OP_SKIP; > + int rem, lim = mtd->writesize; > + u8 *oob_orig = oob; > + > + oob += BBM_SIZE; > + raw_aux(mtd, &oob, METADATA_SIZE, oob_op, &lim); > + > + while (lim > pkt_size) > + { > + raw_aux(mtd, &buf, pkt_size, buf_op, &lim); > + raw_aux(mtd, &oob, ecc_size, oob_op, &lim); > + } > + > + rem = pkt_size - lim; > + raw_aux(mtd, &buf, lim, buf_op, &lim); > + raw_aux(mtd, &oob_orig, BBM_SIZE, oob_op, &lim); > + raw_aux(mtd, &buf, rem, buf_op, &lim); > + raw_aux(mtd, &oob, ecc_size, oob_op, &lim); > + > + return 0; > +} With the above changes this gives: static int raw_access(struct nand_chip *chip, union tango_raw_buffer *buf, union tango_raw_buffer *oob, int op) { struct mtd_info *mtd = nand_to_mtd(chip); int pkt_size = chip->ecc.size; int ecc_size = chip->ecc.bytes; int steps = 0, rem = 0; struct tango_raw_access info = { .op = op, .pos = 0, }; union tango_raw_buffer oob_orig; if (oob) { oob->in = chip->oob_poi + BBM_SIZE; oob_orig.in = chip->oob_poi; } raw_aux(chip, &info, oob, METADATA_SIZE); while (info.pos + pkt_size + ecc_size <= mtd->writesize) { raw_aux(chip, &info, buf, pkt_size); raw_aux(mtd, &info, oob, ecc_size); steps++; } if (steps < chip->ecc.steps) rem = pkt_size - (mtd->writesize - info.pos); raw_aux(mtd, &info, buf, mtd->writesize - info.pos); } raw_aux(mtd, &info, oob ? &oob_orig : NULL, BBM_SIZE); raw_aux(mtd, &info, buf, rem); rem = mtd->writesize + mtd->oobsize - info.pos; raw_aux(mtd, &info, oob, rem); return 0; } > + > +static int tango_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > + uint8_t *buf, int oob_required, int page) > +{ > + return raw_access(mtd, buf, chip->oob_poi, OP_READ); > +} static int tango_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int oob_required, int page) { union tango_raw_buffer data_buf = { .in = buf } union tango_raw_buffer oob_buf; return raw_access(mtd, data_buf, oob_required ? oob_buf : NULL, OP_READ); } > + > +static int tango_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > + const uint8_t *buf, int oob_required, int page) > +{ > + return raw_access(mtd, (void *)buf, chip->oob_poi, OP_WRITE); > +} > + > +static int tango_read_oob(struct mtd_info *mtd, struct nand_chip *chip, int page) > +{ > + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); > + return raw_access(mtd, NULL, chip->oob_poi, OP_READ); > +} > + > +static int tango_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page) > +{ > + chip->pagebuf = -1; > + memset(chip->buffers->databuf, 0xff, mtd->writesize); > + chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0, page); > + raw_access(mtd, chip->buffers->databuf, chip->oob_poi, OP_WRITE); > + chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1); > + chip->waitfunc(mtd, chip); > + return 0; > +} > + > +static int oob_ecc(struct mtd_info *mtd, int idx, struct mtd_oob_region *res) > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + struct nand_ecc_ctrl *ecc = &chip->ecc; > + > + if (idx >= ecc->steps) > + return -ERANGE; > + > + res->offset = BBM_SIZE + METADATA_SIZE + ecc->bytes * idx; > + res->length = ecc->bytes; > + > + return 0; > +} > + > +static int oob_free(struct mtd_info *mtd, int idx, struct mtd_oob_region *res) > +{ > + return -ERANGE; /* no free space in spare area */ > +} > + > +static const struct mtd_ooblayout_ops tango_nand_ooblayout_ops = { > + .ecc = oob_ecc, > + .free = oob_free, > +}; > + > +static u32 to_ticks(int kHz, int ps) > +{ > + return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC); > +} > + > +static int set_timings(struct tango_chip *p, int kHz) > +{ > + u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr; > + const struct nand_sdr_timings *sdr; > + int mode = onfi_get_async_timing_mode(&p->chip); > + > + if (mode & ONFI_TIMING_MODE_UNKNOWN) > + return -ENODEV; > + > + sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1); We recently introduced a generic interface to automate nand timings configuration [1]. Can you make use of it (the patches are in my nand/next branch [2], you can rebase your patch series on top of this branch). > + > + Trdy = to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max); > + Textw = to_ticks(kHz, sdr->tWB_max); > + Twc = to_ticks(kHz, sdr->tWC_min); > + Twpw = to_ticks(kHz, sdr->tWC_min - sdr->tWP_min); > + > + Tacc = to_ticks(kHz, sdr->tREA_max); > + Thold = to_ticks(kHz, sdr->tREH_min); > + Trpw = to_ticks(kHz, sdr->tRC_min - sdr->tREH_min); > + Textr = to_ticks(kHz, sdr->tRHZ_max); Can you please be consistent in your indentation? You're using spaces before '=' almost everywhere except in a few places (like here). > + > + p->timing1 = TIMING(Trdy, Textw, Twc, Twpw); > + p->timing2 = TIMING(Tacc, Thold, Trpw, Textr); > + > + return 0; > +} > + > +static int chip_init(struct device *dev, struct device_node *np, int kHz) > +{ > + int err; > + u32 cs, ecc_bits; > + struct nand_ecc_ctrl *ecc; > + struct tango_nfc *nfc = dev_get_drvdata(dev); > + struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); > + if (!p) > + return -ENOMEM; > + > + err = of_property_read_u32_index(np, "reg", 0, &cs); > + if (err) > + return err; > + Can you please check that you only have a single value in reg? Just to make sure the user doesn't try to define a multi-CS chip. > + if (cs >= MAX_CS) > + return -ERANGE; > + > + p->chip.read_byte = tango_read_byte; > + p->chip.read_buf = tango_read_buf; > + p->chip.select_chip = tango_select_chip; > + p->chip.cmd_ctrl = tango_cmd_ctrl; > + p->chip.dev_ready = tango_dev_ready; > + p->chip.options = NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE; > + p->chip.controller = &nfc->hw; Again, be consistent in you coding style: use spaces. > + > + ecc = &p->chip.ecc; > + ecc->mode = NAND_ECC_HW; > + ecc->algo = NAND_ECC_BCH; > + ecc->read_page_raw = tango_read_page_raw; > + ecc->write_page_raw = tango_write_page_raw; > + ecc->read_page = tango_read_page; > + ecc->write_page = tango_write_page; > + ecc->read_oob = tango_read_oob; > + ecc->write_oob = tango_write_oob; Can you move this part after nand_scan_ident(). I'd also suggest to support software ECC (it's just taking a few more lines), but that's up to you. > + > + nand_set_flash_node(&p->chip, np); > + mtd_set_ooblayout(&p->chip.mtd, &tango_nand_ooblayout_ops); > + err = nand_scan_ident(&p->chip.mtd, 1, NULL); > + if (err) > + return err; > + > + ecc_bits = ecc->strength * FIELD_ORDER; > + p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, BITS_PER_BYTE); > + set_timings(p, kHz); You won't need that one if you implement the chip->setup_data_interface() method. > + > + err = nand_scan_tail(&p->chip.mtd); > + if (err) > + return err; > + > + err = mtd_device_register(&p->chip.mtd, NULL, 0); > + if (err) > + return err; > + > + nfc->chips[cs] = p; > + > + p->xfer_cfg = XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE); > + p->pkt_0_cfg = PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength); > + p->pkt_n_cfg = PKT_CFG(ecc->size, ecc->strength); > + p->bb_cfg = BB_CFG(p->chip.mtd.writesize, BBM_SIZE); This should go before nand_scan_tail(), because, as soon as you call mtd_device_register(), someone might use your NAND device. > + p->cs = cs; And this one should go before nand_scan_ident(). > + > + return 0; > +} > + > +static int tango_nand_probe(struct platform_device *pdev) > +{ > + int kHz; > + struct clk *clk; > + struct resource *res; > + struct tango_nfc *nfc; > + struct device_node *np; > + > + nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL); > + if (!nfc) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + nfc->reg_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(nfc->reg_base)) > + return PTR_ERR(nfc->reg_base); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + nfc->mem_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(nfc->mem_base)) > + return PTR_ERR(nfc->mem_base); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 2); > + nfc->pbus_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(nfc->pbus_base)) > + return PTR_ERR(nfc->pbus_base); > + > + nfc->chan = dma_request_chan(&pdev->dev, "nfc_sbox"); > + if (IS_ERR(nfc->chan)) > + return PTR_ERR(nfc->chan); > + > + clk = clk_get(&pdev->dev, NULL); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + > + platform_set_drvdata(pdev, nfc); > + nand_hw_control_init(&nfc->hw); > + kHz = clk_get_rate(clk) / 1000; > + > + for_each_child_of_node(pdev->dev.of_node, np) { > + int err = chip_init(&pdev->dev, np, kHz); > + if (err) > + return err; You should unregister all NAND/MTD devices before returning an error. > + } > + > + /* TODO: tweak (?) Peripheral Bus setup (timings and CS config) */ > + > + return 0; > +} > + > +static int tango_nand_remove(struct platform_device *pdev) > +{ > + int cs; > + struct tango_nfc *nfc = platform_get_drvdata(pdev); > + > + dma_release_channel(nfc->chan); > + for (cs = 0; cs < MAX_CS; ++cs) > + if (nfc->chips[cs] != NULL) > + nand_release(&nfc->chips[cs]->chip.mtd); > + > + return 0; > +} > + > +static const struct of_device_id tango_nand_ids[] = { > + { .compatible = "sigma,smp8758-nand" }, > + { /* sentinel */ } > +}; > + > +static struct platform_driver tango_nand_driver = { > + .probe = tango_nand_probe, > + .remove = tango_nand_remove, > + .driver = { > + .name = "tango-nand", > + .of_match_table = tango_nand_ids, > + }, > +}; > + > +module_platform_driver(tango_nand_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Sigma Designs"); > +MODULE_DESCRIPTION("Tango4 NAND Flash controller driver"); [1]http://www.spinics.net/lists/arm-kernel/msg530498.html [2]https://github.com/linux-nand/linux/tree/nand/next
On 19/09/2016 19:06, Boris Brezillon wrote: > Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote: > >> This driver supports the NAND Flash controller embedded in recent >> Tango chips, such as SMP8758 and SMP8759. > > Please send another patch to document the DT bindings, and Cc the DT > maintainers. OK. >> +struct tango_chip { >> + struct nand_chip chip; >> + void __iomem *base; > > I'm still not convinced this is needed (->base can be calculated where > needed based on the chip->cs and nfc->reg_base values), but if you want > to keep it, let's keep it. I thought it was bad form to duplicate the code computing chip->base, especially if I need to update that function. >> +static void tango_select_chip(struct mtd_info *mtd, int idx) >> +{ >> + struct nand_chip *nand = mtd_to_nand(mtd); >> + struct tango_nfc *nfc = to_tango_nfc(nand->controller); >> + struct tango_chip *chip = to_tango_chip(nand); >> + >> + if (idx < 0) { >> + chip->base = NULL; >> + return; >> + } >> + >> + chip->base = nfc->pbus_base + (chip->cs * 256); > > You support only one CS per chip, so this is something you can > configure at init time. > > When I asked you to remove the chip->base field, I had multi-CS chips > in mind, but given this implementation, there's no need to set/reset > the chip->base field each time ->select_chip() is called. OK. I'll set chip->cs and chip->base in the probe function. Just to be sure, I'll ask internally whether we want to support multi-CS chips right now, or if this can be added later on. >> + writel_relaxed(chip->timing1, nfc->reg_base + NFC_TIMING1); >> + writel_relaxed(chip->timing2, nfc->reg_base + NFC_TIMING2); >> + writel_relaxed(chip->xfer_cfg, nfc->reg_base + NFC_XFER_CFG); >> + writel_relaxed(chip->pkt_0_cfg, nfc->reg_base + NFC_PKT_0_CFG); >> + writel_relaxed(chip->pkt_n_cfg, nfc->reg_base + NFC_PKT_N_CFG); >> + writel_relaxed(chip->bb_cfg, nfc->reg_base + NFC_BB_CFG); > > Nit: can you avoid these weird alignments. Use a single space after the > comma. I was trying to highlight the fact that all these writes are within the reg_base area. I'll do as you ask, since you are the gatekeeper. >> +static int do_dma(struct tango_nfc *nfc, int dir, int cmd, >> + const void *buf, int len, int page) >> +{ >> + struct dma_async_tx_descriptor *desc; >> + struct scatterlist sg; >> + struct completion tx_done; >> + int err = -EIO; >> + u32 res, val; >> + >> + sg_init_one(&sg, buf, len); >> + if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1) >> + goto leave; > > Return -EIO directly instead of creating this 'leave' label. OK. I wasn't sure it was good form to make the first test special, especially if someone adds a new alloc before this one later on. But I don't care either way. >> + >> + desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT); > > Put DMA_PREP_INTERRUPT on a second line to avoid >80 char lines. I was told some maintainers don't enforce the >80 char line rule. Am I to understand you're not one of them? :-) >> + if (!desc) >> + goto dma_unmap; >> + >> + desc->callback = tango_dma_callback; >> + desc->callback_param = &tx_done; >> + init_completion(&tx_done); >> + >> + writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE); >> + >> + writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE); >> + writel_relaxed( 0, nfc->reg_base + NFC_ADDR_OFFSET); >> + writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD); >> + >> + dmaengine_submit(desc); >> + dma_async_issue_pending(nfc->chan); >> + >> + res = wait_for_completion_timeout(&tx_done, HZ); >> + if (res > 0) { >> + void __iomem *addr = nfc->reg_base + NFC_STATUS; >> + err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000); > > Why do you need this local variable? > > err = readl_poll_timeout(nfc->reg_base + NFC_STATUS, > val, val & CMD_READY, 0, 1000); I find it hard to read when function arguments are split over multiple lines. So if there is a hard "80 char" limit, I prefer using temp variables for "long" arguments. >> + } >> + >> + writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE); > > Add an extra blank line here. OK. >> +static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip, >> + uint8_t *buf, int oob_required, int page) >> +{ >> + struct tango_nfc *nfc = to_tango_nfc(chip->controller); >> + int err, res, len = mtd->writesize; >> + >> + err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page); >> + if (err) >> + return err; >> + >> + if (oob_required) >> + chip->ecc.read_oob(mtd, chip, page); >> + >> + res = decode_error_report(nfc); >> + if (res >= 0) >> + return res; >> + >> + chip->ecc.read_oob(mtd, chip, page); > > There's no different in your case, but it should be read in raw mode. > How about calling ecc.read_oob_raw() so that you're safe even if you > want to differentiate the read_oob() and read_oob_raw() cases at some > point (IIUC, the METADATA section is ECC protected). OK. >> + res = nand_check_erased_ecc_chunk(buf, len, chip->oob_poi, mtd->oobsize, >> + NULL, 0, chip->ecc.strength); > > You should check each ECC packet/chunk independently, because ECC > strength is referring to an ECC packet not a full page. Because of our weird physical layout, accessing data chunks is really awkward. We are willing to sacrifice a few bad pages by using a sub-optimal check (allowing only "strength" bitflips over the entire page, instead of over individual packets). >> +static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip, >> + const uint8_t *buf, int oob_required, int page) >> +{ >> + struct tango_nfc *nfc = to_tango_nfc(chip->controller); >> + int err, len = mtd->writesize; >> + >> + writel_relaxed(0xffffffff, nfc->mem_base + METADATA); >> + err = do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, buf, len, page); >> + if (err) >> + return err; >> + >> + if (oob_required) >> + chip->ecc.write_oob(mtd, chip, page); I suppose we should use write_oob_raw here? >> +enum { OP_SKIP, OP_READ, OP_WRITE }; >> + >> +static void raw_aux(struct mtd_info *mtd, u8 **buf, int len, int op, int *lim) > > Nit: please pass struct nand_chip *chip in first argument, and extract > the mtd using nand_to_mtd(). I know it's just a detail, but I'd like to > avoid passing mtd pointers, especially in internal functions. OK. >> +{ >> + struct nand_chip *chip = mtd_to_nand(mtd); >> + int pos = mtd->writesize - *lim; >> + >> + if (op == OP_SKIP) >> + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, pos + len, -1); > > Okay, we already discussed that one, and I already showed that this was > not working: NAND_CMD_RNDOUT is only valid for read accesses. > > And I told you I'd like to avoid the const to non-const cast in write > accessors. Note: I don't use SKIP for writes, therefore there's no need to implement that code. >> + else if (op == OP_READ) >> + tango_read_buf(mtd, *buf, len); >> + else if (op == OP_WRITE) >> + tango_write_buf(mtd, *buf, len); >> + >> + *lim -= len; >> + *buf += len; >> +} > > I'm not sure factorizing this code is really important, but since you > insist, how about doing the following instead: If the code is not factorized, then it must be duplicated for read/write_raw and read/write_oob, I think (i.e. 4 times). > enum tango_raw_op { OP_READ, OP_WRITE }; > > union tango_raw_buffer { > void *in; > const void *out; > }; > > struct tango_raw_access { > enum tango_raw_op op; > union tango_raw_buffer *buf; > int pos; > }; > > static void raw_aux(struct nand_chip *chip, > struct tango_raw_access *info, > union tango_raw_buffer *buf, int len) > { > struct mtd_info *mtd = nand_to_mtd(chip); > > if (!buf) { > if (info->op == OP_READ) > cmd = NAND_CMD_RNDOUT; > else > cmd = NAND_CMD_SEQIN; > > chip->cmdfunc(mtd, cmd, info->pos + len, -1); > } else { > if (info->op == OP_READ) > tango_read_buf(mtd, buf->out, len); > else > tango_write_buf(mtd, buf->in, len); > > buf->in += len; > } > > info->pos += len; > } I'll take a closer look tomorrow. >> +static int raw_access(struct mtd_info *mtd, uint8_t *buf, uint8_t *oob, int op) >> +{ >> + struct nand_chip *chip = mtd_to_nand(mtd); >> + int pkt_size = chip->ecc.size; >> + int ecc_size = chip->ecc.bytes; >> + int buf_op = buf ? op : OP_SKIP; >> + int oob_op = oob ? op : OP_SKIP; >> + int rem, lim = mtd->writesize; >> + u8 *oob_orig = oob; >> + >> + oob += BBM_SIZE; >> + raw_aux(mtd, &oob, METADATA_SIZE, oob_op, &lim); >> + >> + while (lim > pkt_size) >> + { >> + raw_aux(mtd, &buf, pkt_size, buf_op, &lim); >> + raw_aux(mtd, &oob, ecc_size, oob_op, &lim); >> + } >> + >> + rem = pkt_size - lim; >> + raw_aux(mtd, &buf, lim, buf_op, &lim); >> + raw_aux(mtd, &oob_orig, BBM_SIZE, oob_op, &lim); >> + raw_aux(mtd, &buf, rem, buf_op, &lim); >> + raw_aux(mtd, &oob, ecc_size, oob_op, &lim); >> + >> + return 0; >> +} > > With the above changes this gives: > > static int raw_access(struct nand_chip *chip, > union tango_raw_buffer *buf, > union tango_raw_buffer *oob, > int op) > { > struct mtd_info *mtd = nand_to_mtd(chip); > int pkt_size = chip->ecc.size; > int ecc_size = chip->ecc.bytes; > int steps = 0, rem = 0; > struct tango_raw_access info = { > .op = op, > .pos = 0, > }; > union tango_raw_buffer oob_orig; > > if (oob) { > oob->in = chip->oob_poi + BBM_SIZE; > oob_orig.in = chip->oob_poi; > } > > raw_aux(chip, &info, oob, METADATA_SIZE); > > while (info.pos + pkt_size + ecc_size <= mtd->writesize) { > raw_aux(chip, &info, buf, pkt_size); > raw_aux(mtd, &info, oob, ecc_size); > steps++; > } > > if (steps < chip->ecc.steps) > rem = pkt_size - (mtd->writesize - info.pos); > raw_aux(mtd, &info, buf, mtd->writesize - info.pos); > } I don't think we need the 'steps' variable (I didn't need one). > raw_aux(mtd, &info, oob ? &oob_orig : NULL, BBM_SIZE); > raw_aux(mtd, &info, buf, rem); > > rem = mtd->writesize + mtd->oobsize - info.pos; > raw_aux(mtd, &info, oob, rem); > > return 0; > } I'll take a closer look tomorrow. >> +static u32 to_ticks(int kHz, int ps) >> +{ >> + return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC); >> +} >> + >> +static int set_timings(struct tango_chip *p, int kHz) >> +{ >> + u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr; >> + const struct nand_sdr_timings *sdr; >> + int mode = onfi_get_async_timing_mode(&p->chip); >> + >> + if (mode & ONFI_TIMING_MODE_UNKNOWN) >> + return -ENODEV; >> + >> + sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1); > > We recently introduced a generic interface to automate nand timings > configuration [1]. > Can you make use of it (the patches are in my nand/next branch [2], you > can rebase your patch series on top of this branch). I'll take a look. I still need to have this driver working on 4.7 for the time being... >> + >> + Trdy = to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max); >> + Textw = to_ticks(kHz, sdr->tWB_max); >> + Twc = to_ticks(kHz, sdr->tWC_min); >> + Twpw = to_ticks(kHz, sdr->tWC_min - sdr->tWP_min); >> + >> + Tacc = to_ticks(kHz, sdr->tREA_max); >> + Thold = to_ticks(kHz, sdr->tREH_min); >> + Trpw = to_ticks(kHz, sdr->tRC_min - sdr->tREH_min); >> + Textr = to_ticks(kHz, sdr->tRHZ_max); > > Can you please be consistent in your indentation? > You're using spaces before '=' almost everywhere except in a few > places (like here). When initializing fields of a struct, most code I've seen uses tabs to align the equal signs. I thought it should be the same when assigning the fields of a struct, or assigning several variables of the same nature (such as above). Are you saying you prefer Trdy = to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max); Textw = to_ticks(kHz, sdr->tWB_max); Twc = to_ticks(kHz, sdr->tWC_min); Thus the to_ticks calls are not aligned, and it's less obvious that the first argument is always the same. >> +static int chip_init(struct device *dev, struct device_node *np, int kHz) >> +{ >> + int err; >> + u32 cs, ecc_bits; >> + struct nand_ecc_ctrl *ecc; >> + struct tango_nfc *nfc = dev_get_drvdata(dev); >> + struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); >> + if (!p) >> + return -ENOMEM; >> + >> + err = of_property_read_u32_index(np, "reg", 0, &cs); >> + if (err) >> + return err; >> + > > Can you please check that you only have a single value in reg? Just to > make sure the user doesn't try to define a multi-CS chip. OK. >> + if (cs >= MAX_CS) >> + return -ERANGE; >> + >> + p->chip.read_byte = tango_read_byte; >> + p->chip.read_buf = tango_read_buf; >> + p->chip.select_chip = tango_select_chip; >> + p->chip.cmd_ctrl = tango_cmd_ctrl; >> + p->chip.dev_ready = tango_dev_ready; >> + p->chip.options = NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE; >> + p->chip.controller = &nfc->hw; > > Again, be consistent in you coding style: use spaces. OK. I'll take another look at the sunxi driver. It makes no sense to me that we should tab-align when using struct initialization with named fields, but space-align when using assignment. >> + >> + ecc = &p->chip.ecc; >> + ecc->mode = NAND_ECC_HW; >> + ecc->algo = NAND_ECC_BCH; >> + ecc->read_page_raw = tango_read_page_raw; >> + ecc->write_page_raw = tango_write_page_raw; >> + ecc->read_page = tango_read_page; >> + ecc->write_page = tango_write_page; >> + ecc->read_oob = tango_read_oob; >> + ecc->write_oob = tango_write_oob; > > Can you move this part after nand_scan_ident(). I'd also suggest to > support software ECC (it's just taking a few more lines), but that's up > to you. OK. What does it mean to support SW ECC? >> + >> + nand_set_flash_node(&p->chip, np); >> + mtd_set_ooblayout(&p->chip.mtd, &tango_nand_ooblayout_ops); >> + err = nand_scan_ident(&p->chip.mtd, 1, NULL); >> + if (err) >> + return err; >> + >> + ecc_bits = ecc->strength * FIELD_ORDER; >> + p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, BITS_PER_BYTE); >> + set_timings(p, kHz); > > You won't need that one if you implement the > chip->setup_data_interface() method. I'll take a closer look. >> + p->xfer_cfg = XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE); >> + p->pkt_0_cfg = PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength); >> + p->pkt_n_cfg = PKT_CFG(ecc->size, ecc->strength); >> + p->bb_cfg = BB_CFG(p->chip.mtd.writesize, BBM_SIZE); > > This should go before nand_scan_tail(), because, as soon as you call > mtd_device_register(), someone might use your NAND device. > >> + p->cs = cs; > > And this one should go before nand_scan_ident(). OK to both. >> + for_each_child_of_node(pdev->dev.of_node, np) { >> + int err = chip_init(&pdev->dev, np, kHz); >> + if (err) >> + return err; > > You should unregister all NAND/MTD devices before returning an error. Good catch. I was used to devm_* wrappers for everything in some frameworks. I'll call tango_nand_remove on error. Thanks for the detailed review. Regards.
On Tue, 20 Sep 2016 00:37:06 +0200 Mason <slash.tmp@free.fr> wrote: > On 19/09/2016 19:06, Boris Brezillon wrote: > > > Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote: > > > >> This driver supports the NAND Flash controller embedded in recent > >> Tango chips, such as SMP8758 and SMP8759. > > > > Please send another patch to document the DT bindings, and Cc the DT > > maintainers. > > OK. > > > >> +struct tango_chip { > >> + struct nand_chip chip; > >> + void __iomem *base; > > > > I'm still not convinced this is needed (->base can be calculated where > > needed based on the chip->cs and nfc->reg_base values), but if you want > > to keep it, let's keep it. > > I thought it was bad form to duplicate the code computing > chip->base, especially if I need to update that function. I simple macro would avoid the code duplication: #define TANGO_CHIP_REG(nfc, cs, reg) \ ((nfc)->pbus_base + ((cs) *256) + reg) > > > >> +static void tango_select_chip(struct mtd_info *mtd, int idx) > >> +{ > >> + struct nand_chip *nand = mtd_to_nand(mtd); > >> + struct tango_nfc *nfc = to_tango_nfc(nand->controller); > >> + struct tango_chip *chip = to_tango_chip(nand); > >> + > >> + if (idx < 0) { > >> + chip->base = NULL; > >> + return; > >> + } > >> + > >> + chip->base = nfc->pbus_base + (chip->cs * 256); > > > > You support only one CS per chip, so this is something you can > > configure at init time. > > > > When I asked you to remove the chip->base field, I had multi-CS chips > > in mind, but given this implementation, there's no need to set/reset > > the chip->base field each time ->select_chip() is called. > > OK. I'll set chip->cs and chip->base in the probe function. > Just to be sure, I'll ask internally whether we want to > support multi-CS chips right now, or if this can be added > later on. > > > >> + writel_relaxed(chip->timing1, nfc->reg_base + NFC_TIMING1); > >> + writel_relaxed(chip->timing2, nfc->reg_base + NFC_TIMING2); > >> + writel_relaxed(chip->xfer_cfg, nfc->reg_base + NFC_XFER_CFG); > >> + writel_relaxed(chip->pkt_0_cfg, nfc->reg_base + NFC_PKT_0_CFG); > >> + writel_relaxed(chip->pkt_n_cfg, nfc->reg_base + NFC_PKT_N_CFG); > >> + writel_relaxed(chip->bb_cfg, nfc->reg_base + NFC_BB_CFG); > > > > Nit: can you avoid these weird alignments. Use a single space after the > > comma. > > I was trying to highlight the fact that all these writes are > within the reg_base area. I'll do as you ask, since you are > the gatekeeper. Is that really less clear without the tabs? > > > >> +static int do_dma(struct tango_nfc *nfc, int dir, int cmd, > >> + const void *buf, int len, int page) > >> +{ > >> + struct dma_async_tx_descriptor *desc; > >> + struct scatterlist sg; > >> + struct completion tx_done; > >> + int err = -EIO; > >> + u32 res, val; > >> + > >> + sg_init_one(&sg, buf, len); > >> + if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1) > >> + goto leave; > > > > Return -EIO directly instead of creating this 'leave' label. > > OK. I wasn't sure it was good form to make the first test > special, especially if someone adds a new alloc before this > one later on. But I don't care either way. If one adds an allocation, it's likely to be put after this call, and if it's not, we'll patch the code and create a new exit patch. > > > >> + > >> + desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT); > > > > Put DMA_PREP_INTERRUPT on a second line to avoid >80 char lines. > > I was told some maintainers don't enforce the >80 char line rule. > Am I to understand you're not one of them? :-) Yes, I'm one of them :P. > > > >> + if (!desc) > >> + goto dma_unmap; > >> + > >> + desc->callback = tango_dma_callback; > >> + desc->callback_param = &tx_done; > >> + init_completion(&tx_done); > >> + > >> + writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE); > >> + > >> + writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE); > >> + writel_relaxed( 0, nfc->reg_base + NFC_ADDR_OFFSET); > >> + writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD); > >> + > >> + dmaengine_submit(desc); > >> + dma_async_issue_pending(nfc->chan); > >> + > >> + res = wait_for_completion_timeout(&tx_done, HZ); > >> + if (res > 0) { > >> + void __iomem *addr = nfc->reg_base + NFC_STATUS; > >> + err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000); > > > > Why do you need this local variable? > > > > err = readl_poll_timeout(nfc->reg_base + NFC_STATUS, > > val, val & CMD_READY, 0, 1000); > > I find it hard to read when function arguments are split > over multiple lines. So if there is a hard "80 char" limit, > I prefer using temp variables for "long" arguments. Really? Is that really hard to read? > > > >> + } > >> + > >> + writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE); > > > > Add an extra blank line here. > > OK. > > > >> +static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip, > >> + uint8_t *buf, int oob_required, int page) > >> +{ > >> + struct tango_nfc *nfc = to_tango_nfc(chip->controller); > >> + int err, res, len = mtd->writesize; > >> + > >> + err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page); > >> + if (err) > >> + return err; > >> + > >> + if (oob_required) > >> + chip->ecc.read_oob(mtd, chip, page); > >> + > >> + res = decode_error_report(nfc); > >> + if (res >= 0) > >> + return res; > >> + > >> + chip->ecc.read_oob(mtd, chip, page); > > > > There's no different in your case, but it should be read in raw mode. > > How about calling ecc.read_oob_raw() so that you're safe even if you > > want to differentiate the read_oob() and read_oob_raw() cases at some > > point (IIUC, the METADATA section is ECC protected). > > OK. > > > >> + res = nand_check_erased_ecc_chunk(buf, len, chip->oob_poi, mtd->oobsize, > >> + NULL, 0, chip->ecc.strength); > > > > You should check each ECC packet/chunk independently, because ECC > > strength is referring to an ECC packet not a full page. > > Because of our weird physical layout, accessing data chunks is > really awkward. We are willing to sacrifice a few bad pages > by using a sub-optimal check (allowing only "strength" bitflips > over the entire page, instead of over individual packets). Hm, it doesn't look so complicated to me: bitflips = 0; for (i = 0; i < chip->ecc.steps; i++) { void *data = buf + (i * chip->ecc.size); void *oob = chip->oob_poi + (i * chip->ecc.bytes); void *extra_oob = NULL; int extra_oob_len = 0; if (!i) { extra_oob = chip->oob_poi; extra_oob_len = METADATA_SIZE + BBM_SIZE; } else if (i == chip->ecc.steps) { extra_oob_len = mtd->oobsize - METADATA_SIZE - BBM_SIZE - (chip->ecc.steps * chip->ecc.bytes); extra_oob_len = chip->oob_poi + mtd->oobsize - extra_oob_len; } res = nand_check_erased_ecc_chunk(data, chip->ecc.size, oob, chip->ecc.bytes, extra_oob, extra_oob_len, chip->ecc.strength); if (res >= 0) bitflips = max(res, bitflips); else mtd->ecc_stats.failed++; } > > > >> +static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip, > >> + const uint8_t *buf, int oob_required, int page) > >> +{ > >> + struct tango_nfc *nfc = to_tango_nfc(chip->controller); > >> + int err, len = mtd->writesize; > >> + > >> + writel_relaxed(0xffffffff, nfc->mem_base + METADATA); > >> + err = do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, buf, len, page); > >> + if (err) > >> + return err; > >> + > >> + if (oob_required) > >> + chip->ecc.write_oob(mtd, chip, page); > > I suppose we should use write_oob_raw here? Oops, I didn't spot that one in my review. You said your engine sends the PAGEPROG command, which means the page has already been programmed when do_dma() returns, right? In that case, you can't call ->write_oob() or ->write_oob_raw(), because you're not allowed to program a page twice. Don't you have a way to program OOB along with data (extra registers you can use to pass your OOB data)? Anyway, since you don't expose free OOB sections, I think you can ignore the oob_required parameter. Just add a comment explaining why. > > > >> +enum { OP_SKIP, OP_READ, OP_WRITE }; > >> + > >> +static void raw_aux(struct mtd_info *mtd, u8 **buf, int len, int op, int *lim) > > > > Nit: please pass struct nand_chip *chip in first argument, and extract > > the mtd using nand_to_mtd(). I know it's just a detail, but I'd like to > > avoid passing mtd pointers, especially in internal functions. > > OK. > > > >> +{ > >> + struct nand_chip *chip = mtd_to_nand(mtd); > >> + int pos = mtd->writesize - *lim; > >> + > >> + if (op == OP_SKIP) > >> + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, pos + len, -1); > > > > Okay, we already discussed that one, and I already showed that this was > > not working: NAND_CMD_RNDOUT is only valid for read accesses. > > > > And I told you I'd like to avoid the const to non-const cast in write > > accessors. > > Note: I don't use SKIP for writes, therefore there's no need > to implement that code. Except you still have to cast the buf pointer in tango_write_page_raw(). > > > >> + else if (op == OP_READ) > >> + tango_read_buf(mtd, *buf, len); > >> + else if (op == OP_WRITE) > >> + tango_write_buf(mtd, *buf, len); > >> + > >> + *lim -= len; > >> + *buf += len; > >> +} > > > > I'm not sure factorizing this code is really important, but since you > > insist, how about doing the following instead: > > If the code is not factorized, then it must be duplicated for > read/write_raw and read/write_oob, I think (i.e. 4 times). Only twice: read_oob and read_page_raw can still be factorized, same goes for write_oob and write_page_raw. Anyway, I fear I won't convince you, so please make sure you remove this const to non-const cast. > > > enum tango_raw_op { OP_READ, OP_WRITE }; > > > > union tango_raw_buffer { > > void *in; > > const void *out; > > }; > > > > struct tango_raw_access { > > enum tango_raw_op op; > > union tango_raw_buffer *buf; > > int pos; > > }; > > > > static void raw_aux(struct nand_chip *chip, > > struct tango_raw_access *info, > > union tango_raw_buffer *buf, int len) > > { > > struct mtd_info *mtd = nand_to_mtd(chip); > > > > if (!buf) { > > if (info->op == OP_READ) > > cmd = NAND_CMD_RNDOUT; > > else > > cmd = NAND_CMD_SEQIN; > > > > chip->cmdfunc(mtd, cmd, info->pos + len, -1); > > } else { > > if (info->op == OP_READ) > > tango_read_buf(mtd, buf->out, len); > > else > > tango_write_buf(mtd, buf->in, len); > > > > buf->in += len; > > } > > > > info->pos += len; > > } > > I'll take a closer look tomorrow. > > > >> +static int raw_access(struct mtd_info *mtd, uint8_t *buf, uint8_t *oob, int op) > >> +{ > >> + struct nand_chip *chip = mtd_to_nand(mtd); > >> + int pkt_size = chip->ecc.size; > >> + int ecc_size = chip->ecc.bytes; > >> + int buf_op = buf ? op : OP_SKIP; > >> + int oob_op = oob ? op : OP_SKIP; > >> + int rem, lim = mtd->writesize; > >> + u8 *oob_orig = oob; > >> + > >> + oob += BBM_SIZE; > >> + raw_aux(mtd, &oob, METADATA_SIZE, oob_op, &lim); > >> + > >> + while (lim > pkt_size) > >> + { > >> + raw_aux(mtd, &buf, pkt_size, buf_op, &lim); > >> + raw_aux(mtd, &oob, ecc_size, oob_op, &lim); > >> + } > >> + > >> + rem = pkt_size - lim; > >> + raw_aux(mtd, &buf, lim, buf_op, &lim); > >> + raw_aux(mtd, &oob_orig, BBM_SIZE, oob_op, &lim); > >> + raw_aux(mtd, &buf, rem, buf_op, &lim); > >> + raw_aux(mtd, &oob, ecc_size, oob_op, &lim); > >> + > >> + return 0; > >> +} > > > > With the above changes this gives: > > > > static int raw_access(struct nand_chip *chip, > > union tango_raw_buffer *buf, > > union tango_raw_buffer *oob, > > int op) > > { > > struct mtd_info *mtd = nand_to_mtd(chip); > > int pkt_size = chip->ecc.size; > > int ecc_size = chip->ecc.bytes; > > int steps = 0, rem = 0; > > struct tango_raw_access info = { > > .op = op, > > .pos = 0, > > }; > > union tango_raw_buffer oob_orig; > > > > if (oob) { > > oob->in = chip->oob_poi + BBM_SIZE; > > oob_orig.in = chip->oob_poi; > > } > > > > raw_aux(chip, &info, oob, METADATA_SIZE); > > > > while (info.pos + pkt_size + ecc_size <= mtd->writesize) { > > raw_aux(chip, &info, buf, pkt_size); > > raw_aux(mtd, &info, oob, ecc_size); > > steps++; > > } > > > > if (steps < chip->ecc.steps) > > rem = pkt_size - (mtd->writesize - info.pos); > > raw_aux(mtd, &info, buf, mtd->writesize - info.pos); > > } > > I don't think we need the 'steps' variable (I didn't need one). > > > raw_aux(mtd, &info, oob ? &oob_orig : NULL, BBM_SIZE); > > raw_aux(mtd, &info, buf, rem); > > > > rem = mtd->writesize + mtd->oobsize - info.pos; > > raw_aux(mtd, &info, oob, rem); > > > > return 0; > > } > > I'll take a closer look tomorrow. > > > >> +static u32 to_ticks(int kHz, int ps) > >> +{ > >> + return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC); > >> +} > >> + > >> +static int set_timings(struct tango_chip *p, int kHz) > >> +{ > >> + u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr; > >> + const struct nand_sdr_timings *sdr; > >> + int mode = onfi_get_async_timing_mode(&p->chip); > >> + > >> + if (mode & ONFI_TIMING_MODE_UNKNOWN) > >> + return -ENODEV; > >> + > >> + sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1); > > > > We recently introduced a generic interface to automate nand timings > > configuration [1]. > > Can you make use of it (the patches are in my nand/next branch [2], you > > can rebase your patch series on top of this branch). > > I'll take a look. I still need to have this driver working > on 4.7 for the time being... And? You're trying to mainline the thing. What did you expect? I'm asking you to use an infrastructure we introduced recently (will be part of 4.9), the fact that you need to make the driver work on 4.7 is not a good reason to not use it (you can either backport the series, or keep 2 versions of your driver). > > > >> + > >> + Trdy = to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max); > >> + Textw = to_ticks(kHz, sdr->tWB_max); > >> + Twc = to_ticks(kHz, sdr->tWC_min); > >> + Twpw = to_ticks(kHz, sdr->tWC_min - sdr->tWP_min); > >> + > >> + Tacc = to_ticks(kHz, sdr->tREA_max); > >> + Thold = to_ticks(kHz, sdr->tREH_min); > >> + Trpw = to_ticks(kHz, sdr->tRC_min - sdr->tREH_min); > >> + Textr = to_ticks(kHz, sdr->tRHZ_max); > > > > Can you please be consistent in your indentation? > > You're using spaces before '=' almost everywhere except in a few > > places (like here). > > When initializing fields of a struct, most code I've seen > uses tabs to align the equal signs. I thought it should be > the same when assigning the fields of a struct, or assigning > several variables of the same nature (such as above). > > Are you saying you prefer > > Trdy = to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max); > Textw = to_ticks(kHz, sdr->tWB_max); > Twc = to_ticks(kHz, sdr->tWC_min); Yes, I prefer that one. > > Thus the to_ticks calls are not aligned, and it's less obvious > that the first argument is always the same. And? What's the point of making it obvious? Does it help understanding the code? I prefer when the coding is consistent, and here, it's not consistent with the rest of the file. > > > >> +static int chip_init(struct device *dev, struct device_node *np, int kHz) > >> +{ > >> + int err; > >> + u32 cs, ecc_bits; > >> + struct nand_ecc_ctrl *ecc; > >> + struct tango_nfc *nfc = dev_get_drvdata(dev); > >> + struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); > >> + if (!p) > >> + return -ENOMEM; > >> + > >> + err = of_property_read_u32_index(np, "reg", 0, &cs); > >> + if (err) > >> + return err; > >> + > > > > Can you please check that you only have a single value in reg? Just to > > make sure the user doesn't try to define a multi-CS chip. > > OK. > > > >> + if (cs >= MAX_CS) > >> + return -ERANGE; > >> + > >> + p->chip.read_byte = tango_read_byte; > >> + p->chip.read_buf = tango_read_buf; > >> + p->chip.select_chip = tango_select_chip; > >> + p->chip.cmd_ctrl = tango_cmd_ctrl; > >> + p->chip.dev_ready = tango_dev_ready; > >> + p->chip.options = NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE; > >> + p->chip.controller = &nfc->hw; > > > > Again, be consistent in you coding style: use spaces. > > OK. I'll take another look at the sunxi driver. > > It makes no sense to me that we should tab-align when using > struct initialization with named fields, but space-align > when using assignment. I never asked to tab-align struct field assignments. Actually, I don't like it either. > > > >> + > >> + ecc = &p->chip.ecc; > >> + ecc->mode = NAND_ECC_HW; > >> + ecc->algo = NAND_ECC_BCH; > >> + ecc->read_page_raw = tango_read_page_raw; > >> + ecc->write_page_raw = tango_write_page_raw; > >> + ecc->read_page = tango_read_page; > >> + ecc->write_page = tango_write_page; > >> + ecc->read_oob = tango_read_oob; > >> + ecc->write_oob = tango_write_oob; > > > > Can you move this part after nand_scan_ident(). I'd also suggest to > > support software ECC (it's just taking a few more lines), but that's up > > to you. > > OK. What does it mean to support SW ECC? I means that, if someone wants to use your NAND controller with software ECC or ECC at all he can. Regards, Boris
On Tue, 2016-09-20 at 09:24 +0200, Boris Brezillon wrote: > > >> + writel_relaxed(chip->timing1, nfc->reg_base + NFC_TIMING1); > > >> + writel_relaxed(chip->timing2, nfc->reg_base + NFC_TIMING2); > > >> + writel_relaxed(chip->xfer_cfg, nfc->reg_base + NFC_XFER_CFG); > > >> + writel_relaxed(chip->pkt_0_cfg, nfc->reg_base + NFC_PKT_0_CFG); > > >> + writel_relaxed(chip->pkt_n_cfg, nfc->reg_base + NFC_PKT_N_CFG); > > >> + writel_relaxed(chip->bb_cfg, nfc->reg_base + NFC_BB_CFG); > > > > > > Nit: can you avoid these weird alignments. Use a single space after the > > > comma. > > > > I was trying to highlight the fact that all these writes are > > within the reg_base area. I'll do as you ask, since you are > > the gatekeeper. > > Is that really less clear without the tabs? Visually the aligned stuff looks nicer, and some kernel maintainers prefer this style. Other maintainers avoid it because it causes more churn. Indeed, imaging you need to add another field with longer name, or one of the existing fields is renamed - then you need to re-align, your patches become larger. So my suggestion is to just follow maintainers' preferences.
On 20.09.2016 09:39, Artem Bityutskiy wrote: > On Tue, 2016-09-20 at 09:24 +0200, Boris Brezillon wrote: >>>>> + writel_relaxed(chip->timing1, nfc->reg_base + NFC_TIMING1); >>>>> + writel_relaxed(chip->timing2, nfc->reg_base + NFC_TIMING2); >>>>> + writel_relaxed(chip->xfer_cfg, nfc->reg_base + NFC_XFER_CFG); >>>>> + writel_relaxed(chip->pkt_0_cfg, nfc->reg_base + NFC_PKT_0_CFG); >>>>> + writel_relaxed(chip->pkt_n_cfg, nfc->reg_base + NFC_PKT_N_CFG); >>>>> + writel_relaxed(chip->bb_cfg, nfc->reg_base + NFC_BB_CFG); >>>> >>>> Nit: can you avoid these weird alignments. Use a single space after the >>>> comma. >>> >>> I was trying to highlight the fact that all these writes are >>> within the reg_base area. I'll do as you ask, since you are >>> the gatekeeper. >> >> Is that really less clear without the tabs? > > Visually the aligned stuff looks nicer, and some kernel maintainers > prefer this style. Other maintainers avoid it because it causes more > churn. Indeed, imaging you need to add another field with longer name, > or one of the existing fields is renamed - then you need to re-align, > your patches become larger. FWIW, I don't like tab aligned stuff for the simple fact because git grep "foo =" does not find them. Of course I could use regex magic but this does not make things easier. :-) Thanks, //richard
On 19/09/2016 15:12, Marc Gonzalez wrote: > +static int chip_init(struct device *dev, struct device_node *np, int kHz) > +{ > + int err; > + u32 cs, ecc_bits; > + struct nand_ecc_ctrl *ecc; > + struct tango_nfc *nfc = dev_get_drvdata(dev); > + struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); As pointed out by Boris on IRC, every chip needs a pointer back to the parent device. p->chip.mtd.dev.parent = dev; http://lxr.free-electrons.com/source/drivers/mtd/nand/sunxi_nand.c#L1729
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index f05e0e9eb2f7..22eb5457c9f7 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -205,6 +205,12 @@ config MTD_NAND_S3C2410_CLKSTOP when the is NAND chip selected or released, but will save approximately 5mA of power when there is nothing happening. +config MTD_NAND_TANGO + tristate "NAND Flash support for Tango chips" + depends on ARCH_TANGO + help + Enables the NAND Flash controller on Tango chips. + config MTD_NAND_DISKONCHIP tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND reimplementation)" depends on HAS_IOMEM diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile index f55335373f7c..647f727223ef 100644 --- a/drivers/mtd/nand/Makefile +++ b/drivers/mtd/nand/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_DENALI_DT) += denali_dt.o obj-$(CONFIG_MTD_NAND_AU1550) += au1550nd.o obj-$(CONFIG_MTD_NAND_BF5XX) += bf5xx_nand.o obj-$(CONFIG_MTD_NAND_S3C2410) += s3c2410.o +obj-$(CONFIG_MTD_NAND_TANGO) += tango_nand.o obj-$(CONFIG_MTD_NAND_DAVINCI) += davinci_nand.o obj-$(CONFIG_MTD_NAND_DISKONCHIP) += diskonchip.o obj-$(CONFIG_MTD_NAND_DOCG4) += docg4.o diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c new file mode 100644 index 000000000000..ec18cad546fb --- /dev/null +++ b/drivers/mtd/nand/tango_nand.c @@ -0,0 +1,555 @@ +#include <linux/io.h> +#include <linux/of.h> +#include <linux/clk.h> +#include <linux/iopoll.h> +#include <linux/module.h> +#include <linux/mtd/nand.h> +#include <linux/dmaengine.h> +#include <linux/dma-mapping.h> +#include <linux/platform_device.h> + +/* Offsets relative to chip->base */ +#define PBUS_CMD 0 +#define PBUS_ADDR 4 +#define PBUS_DATA 8 + +/* Offsets relative to reg_base */ +#define NFC_STATUS 0x00 +#define NFC_FLASH_CMD 0x04 +#define NFC_DEVICE_CFG 0x08 +#define NFC_TIMING1 0x0c +#define NFC_TIMING2 0x10 +#define NFC_XFER_CFG 0x14 +#define NFC_PKT_0_CFG 0x18 +#define NFC_PKT_N_CFG 0x1c +#define NFC_BB_CFG 0x20 +#define NFC_ADDR_PAGE 0x24 +#define NFC_ADDR_OFFSET 0x28 +#define NFC_XFER_STATUS 0x2c + +/* NFC_STATUS values */ +#define CMD_READY BIT(31) + +/* NFC_FLASH_CMD values */ +#define NFC_READ 1 +#define NFC_WRITE 2 + +/* NFC_XFER_STATUS values */ +#define PAGE_IS_EMPTY BIT(16) + +/* Offsets relative to mem_base */ +#define METADATA 0x000 +#define ERROR_REPORT 0x1c0 + +/* + * Error reports are split in two bytes: + * byte 0 for the first packet in a page (PKT_0) + * byte 1 for other packets in a page (PKT_N, for N > 0) + * ERR_COUNT_PKT_N is the max error count over all but the first packet. + */ +#define DECODE_OK_PKT_0(v) (v & BIT(7)) +#define DECODE_OK_PKT_N(v) (v & BIT(15)) +#define ERR_COUNT_PKT_0(v) ((v >> 0) & 0x3f) +#define ERR_COUNT_PKT_N(v) ((v >> 8) & 0x3f) + +/* Offsets relative to pbus_base */ +#define PBUS_CS_CTRL 0x83c +#define PBUS_PAD_MODE 0x8f0 + +/* PBUS_CS_CTRL values */ +#define PBUS_IORDY BIT(31) + +/* + * PBUS_PAD_MODE values + * In raw mode, the driver communicates directly with the NAND chips. + * In NFC mode, the NAND Flash controller manages the communication. + * We use NFC mode for read and write; raw mode for everything else. + */ +#define MODE_RAW 0 +#define MODE_NFC BIT(31) + +#define METADATA_SIZE 4 +#define BBM_SIZE 6 +#define FIELD_ORDER 15 + +#define MAX_CS 4 + +struct tango_nfc { + struct nand_hw_control hw; + void __iomem *reg_base; + void __iomem *mem_base; + void __iomem *pbus_base; + struct tango_chip *chips[MAX_CS]; + struct dma_chan *chan; +}; + +#define to_tango_nfc(ptr) container_of(ptr, struct tango_nfc, hw) + +struct tango_chip { + struct nand_chip chip; + void __iomem *base; + u32 timing1; + u32 timing2; + u32 xfer_cfg; + u32 pkt_0_cfg; + u32 pkt_n_cfg; + u32 bb_cfg; + int cs; +}; + +#define to_tango_chip(ptr) container_of(ptr, struct tango_chip, chip) + +#define XFER_CFG(cs, page_count, steps, metadata_size) \ + ((cs) << 24 | (page_count) << 16 | (steps) << 8 | (metadata_size) << 0) + +#define PKT_CFG(size, strength) ((size) << 16 | (strength) << 0) + +#define BB_CFG(bb_offset, bb_size) ((bb_offset) << 16 | (bb_size) << 0) + +#define TIMING(t0, t1, t2, t3) (t0 << 24 | t1 << 16 | t2 << 8 | t3 << 0) + +static void tango_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl) +{ + struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd)); + + if (ctrl & NAND_CLE) + writeb_relaxed(dat, chip->base + PBUS_CMD); + + if (ctrl & NAND_ALE) + writeb_relaxed(dat, chip->base + PBUS_ADDR); +} + +static int tango_dev_ready(struct mtd_info *mtd) +{ + struct nand_chip *chip = mtd_to_nand(mtd); + struct tango_nfc *nfc = to_tango_nfc(chip->controller); + + return readl_relaxed(nfc->pbus_base + PBUS_CS_CTRL) & PBUS_IORDY; +} + +static uint8_t tango_read_byte(struct mtd_info *mtd) +{ + struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd)); + + return readb_relaxed(chip->base + PBUS_DATA); +} + +static void tango_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) +{ + struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd)); + + ioread8_rep(chip->base + PBUS_DATA, buf, len); +} + +static void tango_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) +{ + struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd)); + + iowrite8_rep(chip->base + PBUS_DATA, buf, len); +} + +static void tango_select_chip(struct mtd_info *mtd, int idx) +{ + struct nand_chip *nand = mtd_to_nand(mtd); + struct tango_nfc *nfc = to_tango_nfc(nand->controller); + struct tango_chip *chip = to_tango_chip(nand); + + if (idx < 0) { + chip->base = NULL; + return; + } + + chip->base = nfc->pbus_base + (chip->cs * 256); + + writel_relaxed(chip->timing1, nfc->reg_base + NFC_TIMING1); + writel_relaxed(chip->timing2, nfc->reg_base + NFC_TIMING2); + writel_relaxed(chip->xfer_cfg, nfc->reg_base + NFC_XFER_CFG); + writel_relaxed(chip->pkt_0_cfg, nfc->reg_base + NFC_PKT_0_CFG); + writel_relaxed(chip->pkt_n_cfg, nfc->reg_base + NFC_PKT_N_CFG); + writel_relaxed(chip->bb_cfg, nfc->reg_base + NFC_BB_CFG); +} + +static int decode_error_report(struct tango_nfc *nfc) +{ + u32 status, res; + + status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS); + if (status & PAGE_IS_EMPTY) + return 0; + + res = readl_relaxed(nfc->mem_base + ERROR_REPORT); + + if (DECODE_OK_PKT_0(res) && DECODE_OK_PKT_N(res)) + return max(ERR_COUNT_PKT_0(res), ERR_COUNT_PKT_N(res)); + + return -EBADMSG; +} + +static void tango_dma_callback(void *arg) +{ + complete(arg); +} + +static int do_dma(struct tango_nfc *nfc, int dir, int cmd, + const void *buf, int len, int page) +{ + struct dma_async_tx_descriptor *desc; + struct scatterlist sg; + struct completion tx_done; + int err = -EIO; + u32 res, val; + + sg_init_one(&sg, buf, len); + if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1) + goto leave; + + desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT); + if (!desc) + goto dma_unmap; + + desc->callback = tango_dma_callback; + desc->callback_param = &tx_done; + init_completion(&tx_done); + + writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE); + + writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE); + writel_relaxed( 0, nfc->reg_base + NFC_ADDR_OFFSET); + writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD); + + dmaengine_submit(desc); + dma_async_issue_pending(nfc->chan); + + res = wait_for_completion_timeout(&tx_done, HZ); + if (res > 0) { + void __iomem *addr = nfc->reg_base + NFC_STATUS; + err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000); + } + + writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE); +dma_unmap: + dma_unmap_sg(nfc->chan->device->dev, &sg, 1, dir); +leave: + return err; +} + +static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip, + uint8_t *buf, int oob_required, int page) +{ + struct tango_nfc *nfc = to_tango_nfc(chip->controller); + int err, res, len = mtd->writesize; + + err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page); + if (err) + return err; + + if (oob_required) + chip->ecc.read_oob(mtd, chip, page); + + res = decode_error_report(nfc); + if (res >= 0) + return res; + + chip->ecc.read_oob(mtd, chip, page); + res = nand_check_erased_ecc_chunk(buf, len, chip->oob_poi, mtd->oobsize, + NULL, 0, chip->ecc.strength); + if (res < 0) + mtd->ecc_stats.failed++; + + return res; +} + +static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip, + const uint8_t *buf, int oob_required, int page) +{ + struct tango_nfc *nfc = to_tango_nfc(chip->controller); + int err, len = mtd->writesize; + + writel_relaxed(0xffffffff, nfc->mem_base + METADATA); + err = do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, buf, len, page); + if (err) + return err; + + if (oob_required) + chip->ecc.write_oob(mtd, chip, page); + + return 0; +} + +enum { OP_SKIP, OP_READ, OP_WRITE }; + +static void raw_aux(struct mtd_info *mtd, u8 **buf, int len, int op, int *lim) +{ + struct nand_chip *chip = mtd_to_nand(mtd); + int pos = mtd->writesize - *lim; + + if (op == OP_SKIP) + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, pos + len, -1); + else if (op == OP_READ) + tango_read_buf(mtd, *buf, len); + else if (op == OP_WRITE) + tango_write_buf(mtd, *buf, len); + + *lim -= len; + *buf += len; +} + +static int raw_access(struct mtd_info *mtd, uint8_t *buf, uint8_t *oob, int op) +{ + struct nand_chip *chip = mtd_to_nand(mtd); + int pkt_size = chip->ecc.size; + int ecc_size = chip->ecc.bytes; + int buf_op = buf ? op : OP_SKIP; + int oob_op = oob ? op : OP_SKIP; + int rem, lim = mtd->writesize; + u8 *oob_orig = oob; + + oob += BBM_SIZE; + raw_aux(mtd, &oob, METADATA_SIZE, oob_op, &lim); + + while (lim > pkt_size) + { + raw_aux(mtd, &buf, pkt_size, buf_op, &lim); + raw_aux(mtd, &oob, ecc_size, oob_op, &lim); + } + + rem = pkt_size - lim; + raw_aux(mtd, &buf, lim, buf_op, &lim); + raw_aux(mtd, &oob_orig, BBM_SIZE, oob_op, &lim); + raw_aux(mtd, &buf, rem, buf_op, &lim); + raw_aux(mtd, &oob, ecc_size, oob_op, &lim); + + return 0; +} + +static int tango_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, + uint8_t *buf, int oob_required, int page) +{ + return raw_access(mtd, buf, chip->oob_poi, OP_READ); +} + +static int tango_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, + const uint8_t *buf, int oob_required, int page) +{ + return raw_access(mtd, (void *)buf, chip->oob_poi, OP_WRITE); +} + +static int tango_read_oob(struct mtd_info *mtd, struct nand_chip *chip, int page) +{ + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); + return raw_access(mtd, NULL, chip->oob_poi, OP_READ); +} + +static int tango_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page) +{ + chip->pagebuf = -1; + memset(chip->buffers->databuf, 0xff, mtd->writesize); + chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0, page); + raw_access(mtd, chip->buffers->databuf, chip->oob_poi, OP_WRITE); + chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1); + chip->waitfunc(mtd, chip); + return 0; +} + +static int oob_ecc(struct mtd_info *mtd, int idx, struct mtd_oob_region *res) +{ + struct nand_chip *chip = mtd_to_nand(mtd); + struct nand_ecc_ctrl *ecc = &chip->ecc; + + if (idx >= ecc->steps) + return -ERANGE; + + res->offset = BBM_SIZE + METADATA_SIZE + ecc->bytes * idx; + res->length = ecc->bytes; + + return 0; +} + +static int oob_free(struct mtd_info *mtd, int idx, struct mtd_oob_region *res) +{ + return -ERANGE; /* no free space in spare area */ +} + +static const struct mtd_ooblayout_ops tango_nand_ooblayout_ops = { + .ecc = oob_ecc, + .free = oob_free, +}; + +static u32 to_ticks(int kHz, int ps) +{ + return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC); +} + +static int set_timings(struct tango_chip *p, int kHz) +{ + u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr; + const struct nand_sdr_timings *sdr; + int mode = onfi_get_async_timing_mode(&p->chip); + + if (mode & ONFI_TIMING_MODE_UNKNOWN) + return -ENODEV; + + sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1); + + Trdy = to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max); + Textw = to_ticks(kHz, sdr->tWB_max); + Twc = to_ticks(kHz, sdr->tWC_min); + Twpw = to_ticks(kHz, sdr->tWC_min - sdr->tWP_min); + + Tacc = to_ticks(kHz, sdr->tREA_max); + Thold = to_ticks(kHz, sdr->tREH_min); + Trpw = to_ticks(kHz, sdr->tRC_min - sdr->tREH_min); + Textr = to_ticks(kHz, sdr->tRHZ_max); + + p->timing1 = TIMING(Trdy, Textw, Twc, Twpw); + p->timing2 = TIMING(Tacc, Thold, Trpw, Textr); + + return 0; +} + +static int chip_init(struct device *dev, struct device_node *np, int kHz) +{ + int err; + u32 cs, ecc_bits; + struct nand_ecc_ctrl *ecc; + struct tango_nfc *nfc = dev_get_drvdata(dev); + struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); + if (!p) + return -ENOMEM; + + err = of_property_read_u32_index(np, "reg", 0, &cs); + if (err) + return err; + + if (cs >= MAX_CS) + return -ERANGE; + + p->chip.read_byte = tango_read_byte; + p->chip.read_buf = tango_read_buf; + p->chip.select_chip = tango_select_chip; + p->chip.cmd_ctrl = tango_cmd_ctrl; + p->chip.dev_ready = tango_dev_ready; + p->chip.options = NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE; + p->chip.controller = &nfc->hw; + + ecc = &p->chip.ecc; + ecc->mode = NAND_ECC_HW; + ecc->algo = NAND_ECC_BCH; + ecc->read_page_raw = tango_read_page_raw; + ecc->write_page_raw = tango_write_page_raw; + ecc->read_page = tango_read_page; + ecc->write_page = tango_write_page; + ecc->read_oob = tango_read_oob; + ecc->write_oob = tango_write_oob; + + nand_set_flash_node(&p->chip, np); + mtd_set_ooblayout(&p->chip.mtd, &tango_nand_ooblayout_ops); + err = nand_scan_ident(&p->chip.mtd, 1, NULL); + if (err) + return err; + + ecc_bits = ecc->strength * FIELD_ORDER; + p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, BITS_PER_BYTE); + set_timings(p, kHz); + + err = nand_scan_tail(&p->chip.mtd); + if (err) + return err; + + err = mtd_device_register(&p->chip.mtd, NULL, 0); + if (err) + return err; + + nfc->chips[cs] = p; + + p->xfer_cfg = XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE); + p->pkt_0_cfg = PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength); + p->pkt_n_cfg = PKT_CFG(ecc->size, ecc->strength); + p->bb_cfg = BB_CFG(p->chip.mtd.writesize, BBM_SIZE); + p->cs = cs; + + return 0; +} + +static int tango_nand_probe(struct platform_device *pdev) +{ + int kHz; + struct clk *clk; + struct resource *res; + struct tango_nfc *nfc; + struct device_node *np; + + nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL); + if (!nfc) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + nfc->reg_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(nfc->reg_base)) + return PTR_ERR(nfc->reg_base); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + nfc->mem_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(nfc->mem_base)) + return PTR_ERR(nfc->mem_base); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 2); + nfc->pbus_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(nfc->pbus_base)) + return PTR_ERR(nfc->pbus_base); + + nfc->chan = dma_request_chan(&pdev->dev, "nfc_sbox"); + if (IS_ERR(nfc->chan)) + return PTR_ERR(nfc->chan); + + clk = clk_get(&pdev->dev, NULL); + if (IS_ERR(clk)) + return PTR_ERR(clk); + + platform_set_drvdata(pdev, nfc); + nand_hw_control_init(&nfc->hw); + kHz = clk_get_rate(clk) / 1000; + + for_each_child_of_node(pdev->dev.of_node, np) { + int err = chip_init(&pdev->dev, np, kHz); + if (err) + return err; + } + + /* TODO: tweak (?) Peripheral Bus setup (timings and CS config) */ + + return 0; +} + +static int tango_nand_remove(struct platform_device *pdev) +{ + int cs; + struct tango_nfc *nfc = platform_get_drvdata(pdev); + + dma_release_channel(nfc->chan); + for (cs = 0; cs < MAX_CS; ++cs) + if (nfc->chips[cs] != NULL) + nand_release(&nfc->chips[cs]->chip.mtd); + + return 0; +} + +static const struct of_device_id tango_nand_ids[] = { + { .compatible = "sigma,smp8758-nand" }, + { /* sentinel */ } +}; + +static struct platform_driver tango_nand_driver = { + .probe = tango_nand_probe, + .remove = tango_nand_remove, + .driver = { + .name = "tango-nand", + .of_match_table = tango_nand_ids, + }, +}; + +module_platform_driver(tango_nand_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Sigma Designs"); +MODULE_DESCRIPTION("Tango4 NAND Flash controller driver");
This driver supports the NAND Flash controller embedded in recent Tango chips, such as SMP8758 and SMP8759. Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> --- Tests done to validate this driver: mtd_stresstest dev=1 count=500000 mtd_nandbiterrs dev=1 ubiattach -m 1 -d 3 && runtests.sh /dev/ubi3 --- drivers/mtd/nand/Kconfig | 6 + drivers/mtd/nand/Makefile | 1 + drivers/mtd/nand/tango_nand.c | 555 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 562 insertions(+) create mode 100644 drivers/mtd/nand/tango_nand.c