Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754219AbaDPFE5 (ORCPT ); Wed, 16 Apr 2014 01:04:57 -0400 Received: from mail-pd0-f169.google.com ([209.85.192.169]:43902 "EHLO mail-pd0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751377AbaDPFEy (ORCPT ); Wed, 16 Apr 2014 01:04:54 -0400 Date: Tue, 15 Apr 2014 22:04:49 -0700 From: Brian Norris To: Sergei Ianovich Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Russell King , David Woodhouse , Grant Likely , Artem Bityutskiy , Robert Jarzmik , Kees Cook , Randy Dunlap , Philip Avinash , "open list:OPEN FIRMWARE AND..." , "open list:MEMORY TECHNOLOGY..." Subject: Re: [PATCH v3 10/21] mtd: support BB SRAM on ICP DAS LP-8x4x Message-ID: <20140416050449.GJ3131@norris-Latitude-E6410> References: <1386901645-28895-1-git-send-email-ynvich@gmail.com> <1387309071-22382-1-git-send-email-ynvich@gmail.com> <1387309071-22382-11-git-send-email-ynvich@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1387309071-22382-11-git-send-email-ynvich@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Sergei, On Tue, Dec 17, 2013 at 11:37:40PM +0400, Sergei Ianovich wrote: > This provides an MTD device driver for 512kB of battery backed up SRAM > on ICPDAS LP-8X4X programmable automation controllers. > > SRAM chip is connected via FPGA and is not accessible without a driver, > unlike flash memory which is wired to CPU MMU. > > This SRAM becomes an excellent persisent storage of volatile process > data like counter values and sensor statuses. Storing those data in > flash or mmc card is not a viable solution. > > Signed-off-by: Sergei Ianovich > --- > v2..v3 > * no changes (except number 08/16 -> 10/21) > > v0..v2 > * use device tree > * use devm helpers where possible What's the status of this series? Is the rest of this platform support going in via other trees yet? I have a few trivial comments, but this driver mostly looks good as-is. > arch/arm/boot/dts/pxa27x-lp8x4x.dts | 6 + > arch/arm/configs/lp8x4x_defconfig | 1 + > drivers/mtd/devices/Kconfig | 14 +++ > drivers/mtd/devices/Makefile | 1 + > drivers/mtd/devices/sram_lp8x4x.c | 227 ++++++++++++++++++++++++++++++++++++ > 5 files changed, 249 insertions(+) > create mode 100644 drivers/mtd/devices/sram_lp8x4x.c > > diff --git a/arch/arm/boot/dts/pxa27x-lp8x4x.dts b/arch/arm/boot/dts/pxa27x-lp8x4x.dts > index 872c386..07856e0 100644 > --- a/arch/arm/boot/dts/pxa27x-lp8x4x.dts > +++ b/arch/arm/boot/dts/pxa27x-lp8x4x.dts > @@ -161,6 +161,12 @@ > reg = <0x901c 0x1>; > status = "okay"; > }; > + > + sram@a000 { > + compatible = "icpdas,sram-lp8x4x"; > + reg = <0xa000 0x1000 > + 0x901e 0x1>; > + }; You'll need a (trivial) DT binding doc in Documentation/devicetree/bindings/ to go with this. You might also need to split the arch/arm/boot/dts/ stuff out to a separate patch, so the MTD driver can go in the MTD tree separate from the ARM tree bits. > }; > }; > }; > diff --git a/arch/arm/configs/lp8x4x_defconfig b/arch/arm/configs/lp8x4x_defconfig > index d60e37a..17a4e6f 100644 > --- a/arch/arm/configs/lp8x4x_defconfig > +++ b/arch/arm/configs/lp8x4x_defconfig > @@ -57,6 +57,7 @@ CONFIG_MTD_CFI_ADV_OPTIONS=y > CONFIG_MTD_CFI_GEOMETRY=y > CONFIG_MTD_CFI_INTELEXT=y > CONFIG_MTD_PHYSMAP_OF=y > +CONFIG_MTD_SRAM_LP8X4X=y > CONFIG_PROC_DEVICETREE=y > CONFIG_BLK_DEV_LOOP=y > CONFIG_BLK_DEV_LOOP_MIN_COUNT=2 > diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig > index 0128138..95f2075 100644 > --- a/drivers/mtd/devices/Kconfig > +++ b/drivers/mtd/devices/Kconfig > @@ -217,4 +217,18 @@ config BCH_CONST_T > default 4 > endif > > +config MTD_SRAM_LP8X4X > + tristate "SRAM on ICPDAS LP-8X4X" > + depends on OF && ARCH_PXA > + ---help--- > + This provides an MTD device driver for 512kiB of battery backed up SRAM > + on ICPDAS LP-8X4X programmable automation controllers. > + > + SRAM chip is connected via FPGA and is not accessible without a driver, > + unlike flash memory which is wired to CPU MMU. > + > + Say N, unless you plan to run this kernel on LP-8X4X. > + > + If you say M, the module will be called sram_lp8x4x. > + > endmenu > diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile > index d83bd73..56a74c9 100644 > --- a/drivers/mtd/devices/Makefile > +++ b/drivers/mtd/devices/Makefile > @@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_OMAP_BCH) += elm.o > obj-$(CONFIG_MTD_SPEAR_SMI) += spear_smi.o > obj-$(CONFIG_MTD_SST25L) += sst25l.o > obj-$(CONFIG_MTD_BCM47XXSFLASH) += bcm47xxsflash.o > +obj-$(CONFIG_MTD_SRAM_LP8X4X) += sram_lp8x4x.o > > > CFLAGS_docg3.o += -I$(src) > diff --git a/drivers/mtd/devices/sram_lp8x4x.c b/drivers/mtd/devices/sram_lp8x4x.c > new file mode 100644 > index 0000000..9dc7149 > --- /dev/null > +++ b/drivers/mtd/devices/sram_lp8x4x.c > @@ -0,0 +1,227 @@ > +/* > + * linux/drivers/mtd/devices/lp8x4x_sram.c > + * > + * MTD Driver for SRAM on ICPDAS LP-8x4x > + * Copyright (C) 2013 Sergei Ianovich > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation or any later version. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include Do you need this? It looks unused. > + > +struct lp8x4x_sram_info { > + void __iomem *bank; > + void __iomem *virt; > + struct mutex lock; > + unsigned active_bank; > + struct mtd_info mtd; > +}; > + > +static int > +lp8x4x_sram_erase(struct mtd_info *mtd, struct erase_info *instr) > +{ > + struct lp8x4x_sram_info *info = mtd->priv; > + unsigned bank = instr->addr >> 11; > + unsigned offset = (instr->addr & 0x7ff) << 1; > + loff_t i; > + > + mutex_lock(&info->lock); > + if (unlikely(bank != info->active_bank)) { > + info->active_bank = bank; > + iowrite8(bank, info->bank); > + } > + for (i = 0; i < instr->len; i++) { > + iowrite8(0xff, info->virt + offset); > + offset += 2; > + if (unlikely(offset == 0)) { > + info->active_bank++; > + iowrite8(info->active_bank, info->bank); > + } > + } > + mutex_unlock(&info->lock); > + instr->state = MTD_ERASE_DONE; > + mtd_erase_callback(instr); > + > + return 0; > +} > + > +static int > +lp8x4x_sram_write(struct mtd_info *mtd, loff_t to, size_t len, > + size_t *retlen, const u_char *b) > +{ > + struct lp8x4x_sram_info *info = mtd->priv; > + unsigned bank = to >> 11; > + unsigned offset = (to & 0x7ff) << 1; > + loff_t i; > + > + mutex_lock(&info->lock); > + if (unlikely(bank != info->active_bank)) { > + info->active_bank = bank; > + iowrite8(bank, info->bank); > + } > + for (i = 0; i < len; i++) { > + iowrite8(b[i], info->virt + offset); > + offset += 2; > + if (unlikely(offset == 0)) { > + info->active_bank++; > + iowrite8(info->active_bank, info->bank); > + } > + } > + mutex_unlock(&info->lock); > + *retlen = len; > + return 0; > +} > + > +static int > +lp8x4x_sram_read(struct mtd_info *mtd, loff_t from, size_t len, > + size_t *retlen, u_char *b) > +{ > + struct lp8x4x_sram_info *info = mtd->priv; > + unsigned bank = from >> 11; > + unsigned offset = (from & 0x7ff) << 1; > + loff_t i; > + > + mutex_lock(&info->lock); > + if (unlikely(bank != info->active_bank)) { > + info->active_bank = bank; > + iowrite8(bank, info->bank); > + } > + for (i = 0; i < len; i++) { > + b[i] = ioread8(info->virt + offset); > + offset += 2; > + if (unlikely(offset == 0)) { > + info->active_bank++; > + iowrite8(info->active_bank, info->bank); > + } > + } > + mutex_unlock(&info->lock); > + *retlen = len; > + return 0; > +} > + > +static void > +lp8x4x_sram_sync(struct mtd_info *mtd) > +{ > +} You don't need the empty call-back. See mtd_sync(), which only calls the _sync callback if it's filled in. > + > +static struct of_device_id of_flash_match[] = { > + { > + .compatible = "icpdas,sram-lp8x4x", > + }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, of_flash_match); > + > +static const char * const probe_types[] = { > + "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL }; Please drop 'ofoldpart'; it is obsolte. And do you really want 'RedBoot'? > + > +static int > +lp8x4x_sram_probe(struct platform_device *pdev) > +{ > + const struct of_device_id *match; > + struct lp8x4x_sram_info *info; > + struct resource *r1, *r2; > + char sz_str[16]; > + struct mtd_part_parser_data ppdata; > + int err = 0; > + > + match = of_match_device(of_flash_match, &pdev->dev); > + if (!match) > + return -EINVAL; > + > + r1 = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + r2 = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!r1 || !r2) > + return -ENODEV; Move these next to the devm_ioremap_resource(), and then remove the if (!r1 || !r2) checks, since devm_ioremap_resource() does them for you. (See lib/devres.c.) > + > + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + info->virt = devm_ioremap_resource(&pdev->dev, r1); > + if (!info->virt) { > + dev_err(&pdev->dev, "Failed to ioremap %p\n", > + info->virt); You don't need the print message. devm_ioremap_resource() handles this for you. > + return -EFAULT; > + } > + > + info->bank = devm_ioremap_resource(&pdev->dev, r2); > + if (!info->bank) { > + dev_err(&pdev->dev, "Failed to ioremap %p\n", > + info->bank); Ditto. > + return -EFAULT; > + } > + > + info->mtd.priv = info; > + info->mtd.name = "SRAM"; > + info->mtd.type = MTD_RAM; > + info->mtd.flags = MTD_CAP_RAM; > + info->mtd.size = resource_size(r1) << 7; > + info->mtd.erasesize = 512; > + info->mtd.writesize = 4; > + info->mtd._erase = lp8x4x_sram_erase; > + info->mtd._write = lp8x4x_sram_write; > + info->mtd._read = lp8x4x_sram_read; > + info->mtd._sync = lp8x4x_sram_sync; > + info->mtd.owner = THIS_MODULE; > + > + mutex_init(&info->lock); > + iowrite8(info->active_bank, info->bank); > + platform_set_drvdata(pdev, info); > + > + ppdata.of_node = pdev->dev.of_node; > + err = mtd_device_parse_register(&info->mtd, probe_types, &ppdata, > + NULL, 0); If you don't need the other partition types (commented above), then you can just use the defaults (drivers/mtd/mtdpart.c, default_mtd_part_types) by dropping your 'probe_types' and making the second argument NULL. > + > + if (err < 0) { > + dev_err(&pdev->dev, "Failed to register platform device\n"); How about "failed to register MTD"? > + return err; > + } > + > + string_get_size(info->mtd.size, STRING_UNITS_2, sz_str, > + sizeof(sz_str)); > + dev_info(&pdev->dev, "using %s SRAM on LP-8X4X as %s\n", sz_str, > + dev_name(&info->mtd.dev)); > + return 0; > +} > + > +static int > +lp8x4x_sram_remove(struct platform_device *dev) > +{ > + struct lp8x4x_sram_info *info = platform_get_drvdata(dev); > + > + mtd_device_unregister(&info->mtd); Please check the return code. > + platform_set_drvdata(dev, NULL); I don't think you need to do this any more. The driver core takes care of this on removal. > + return 0; (So just: return mtd_device_unregister(&info->mtd);) > +} > + > +static struct platform_driver lp8x4x_sram_driver = { > + .driver = { > + .name = "sram-lp8x4x", > + .owner = THIS_MODULE, > + .of_match_table = of_flash_match, > + }, > + .probe = lp8x4x_sram_probe, > + .remove = lp8x4x_sram_remove, > +}; > + > +module_platform_driver(lp8x4x_sram_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Sergei Ianovich "); > +MODULE_DESCRIPTION("MTD driver for SRAM on ICPDAS LP-8x4x"); With that, you have my: Reviewed-by: Brian Norris and I can merge this to the MTD tree if/when you'd like. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/