From: Ariel D'Alessandro <ariel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> To: Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Cc: "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" <linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>, Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>, Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Subject: Re: [PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver Date: Fri, 30 Oct 2015 11:55:07 -0300 [thread overview] Message-ID: <563384CB.3070607@vanguardiasur.com.ar> (raw) In-Reply-To: <CAGhQ9Vyg6sScq7yM=7judsMPHOc5VF2zf=7LPxmbmL7wF=vvgw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> Joachim, El 24/10/15 a las 19:04, Joachim Eastwood escribió: > Hi Ariel, > > On 19 October 2015 at 19:32, Ariel D'Alessandro > <ariel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> wrote: >> This commit adds support for NXP LPC18xx EEPROM memory found in NXP >> LPC185x/3x and LPC435x/3x/2x/1x devices. >> >> EEPROM size is 16384 bytes and it can be entirely read and >> written/erased with 1 word (4 bytes) granularity. The last page >> (128 bytes) contains the EEPROM initialization data and is not writable. >> >> Erase/program time is less than 3ms. The EEPROM device requires a >> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing >> the system bus clock by the division factor, contained in the divider >> register (minus 1 encoded). >> >> Signed-off-by: Ariel D'Alessandro <ariel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> >> --- >> drivers/nvmem/Kconfig | 9 ++ >> drivers/nvmem/Makefile | 2 + >> drivers/nvmem/lpc18xx_eeprom.c | 266 +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 277 insertions(+) >> create mode 100644 drivers/nvmem/lpc18xx_eeprom.c > >> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg, >> + size_t reg_size, const void *val, >> + size_t val_size) >> +{ >> + struct lpc18xx_eeprom_dev *eeprom = context; >> + unsigned int offset = *(u32 *)reg; >> + >> + /* 3 ms of erase/program time between each writing */ >> + while (val_size) { >> + writel(*(u32 *)val, eeprom->mem_base + offset); >> + usleep_range(3000, 4000); >> + val_size -= eeprom->val_bytes; >> + val += eeprom->val_bytes; >> + offset += eeprom->val_bytes; >> + } > > What happens here if 'val_size' is less than 4 or not dividable by 4? AFAIK, before calling to gather_write, the caller ensures that 'val_size' % map->format.val_bytes == 0 Like nvmem_device_write() do this in drivers/base/regmap/regmap.c:regmap_raw_write() Then, if val_size == 0, nothing will happen. > Same thing for 'offset'. Let me check this one. > > I tested the driver from sysfs by writing strings into the nvmem-file > with echo. Writing a string not dividable by 4 seems to hang the > system. Yeah, I saw that too. I think that's a nvmem core issue and Srinivas's patch solves this. Will confirm that as soon as I have the board back. > > >> +static int lpc18xx_eeprom_read(void *context, const void *reg, size_t reg_size, >> + void *val, size_t val_size) >> +{ >> + struct lpc18xx_eeprom_dev *eeprom = context; >> + unsigned int offset = *(u32 *)reg; >> + >> + while (val_size) { >> + *(u32 *)val = readl(eeprom->mem_base + offset); >> + val_size -= eeprom->val_bytes; >> + val += eeprom->val_bytes; >> + offset += eeprom->val_bytes; >> + } >> + >> + return 0; >> +} > > Same comments as for lpc18xx_eeprom_gather_write(). Same answer. > > >> +static const struct of_device_id lpc18xx_eeprom_of_match[] = { >> + { .compatible = "nxp,lpc1857-eeprom" }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, lpc18xx_eeprom_of_match); > > nit: It's usual to place of_device_id struct just above the > platform_driver struct. I see. > > >> + eeprom->val_bytes = lpc18xx_regmap_config.val_bits / 8; >> + eeprom->reg_bytes = lpc18xx_regmap_config.reg_bits / 8; > > There is a BITS_PER_BYTE define in bitops.h that you might want to use here. Right. Thanks. > > >> + /* >> + * Clock rate is generated by dividing the system bus clock by the >> + * division factor, contained in the divider register (minus 1 encoded). >> + */ >> + clk_rate = clk_get_rate(eeprom->clk); >> + clk_rate = DIV_ROUND_UP(clk_rate, LPC18XX_EEPROM_CLOCK_HZ) - 1; >> + lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_CLKDIV, clk_rate); >> + >> + /* >> + * Writing a single word to the page will start the erase/program cycle >> + * automatically >> + */ >> + lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_AUTOPROG, >> + LPC18XX_EEPROM_AUTOPROG_WORD); >> + >> + lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN, >> + LPC18XX_EEPROM_PWRDWN_NO); >> + >> + lpc18xx_regmap_config.max_register = resource_size(res) - 1; >> + lpc18xx_regmap_config.writeable_reg = lpc18xx_eeprom_writeable_reg; >> + lpc18xx_regmap_config.readable_reg = lpc18xx_eeprom_readable_reg; >> + >> + regmap = devm_regmap_init(dev, &lpc18xx_eeprom_bus, eeprom, >> + &lpc18xx_regmap_config); >> + if (IS_ERR(regmap)) { >> + dev_err(dev, "regmap init failed: %ld\n", PTR_ERR(regmap)); >> + ret = PTR_ERR(regmap); >> + goto err_clk; >> + } >> + >> + lpc18xx_nvmem_config.dev = dev; >> + >> + eeprom->nvmem = nvmem_register(&lpc18xx_nvmem_config); >> + if (IS_ERR(eeprom->nvmem)) { >> + ret = PTR_ERR(eeprom->nvmem); >> + goto err_clk; >> + } >> + >> + platform_set_drvdata(pdev, eeprom); >> + >> + return 0; >> + >> +err_clk: >> + clk_disable_unprepare(eeprom->clk); >> + >> + return ret; >> +} >> + >> +static int lpc18xx_eeprom_remove(struct platform_device *pdev) >> +{ >> + struct lpc18xx_eeprom_dev *eeprom = platform_get_drvdata(pdev); >> + >> + lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN, >> + LPC18XX_EEPROM_PWRDWN_YES); >> + >> + clk_disable_unprepare(eeprom->clk); >> + >> + return nvmem_unregister(eeprom->nvmem); > > Normally you do tear down in the reverse order of initialization. > > Consider what happens here when you power down and disable the clock > while there still are nvmem users of the eeprom. You're right! Will fix this. Thanks again. -- Ariel D'Alessandro, VanguardiaSur www.vanguardiasur.com.ar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: ariel@vanguardiasur.com.ar (Ariel D'Alessandro) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver Date: Fri, 30 Oct 2015 11:55:07 -0300 [thread overview] Message-ID: <563384CB.3070607@vanguardiasur.com.ar> (raw) In-Reply-To: <CAGhQ9Vyg6sScq7yM=7judsMPHOc5VF2zf=7LPxmbmL7wF=vvgw@mail.gmail.com> Joachim, El 24/10/15 a las 19:04, Joachim Eastwood escribi?: > Hi Ariel, > > On 19 October 2015 at 19:32, Ariel D'Alessandro > <ariel@vanguardiasur.com.ar> wrote: >> This commit adds support for NXP LPC18xx EEPROM memory found in NXP >> LPC185x/3x and LPC435x/3x/2x/1x devices. >> >> EEPROM size is 16384 bytes and it can be entirely read and >> written/erased with 1 word (4 bytes) granularity. The last page >> (128 bytes) contains the EEPROM initialization data and is not writable. >> >> Erase/program time is less than 3ms. The EEPROM device requires a >> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing >> the system bus clock by the division factor, contained in the divider >> register (minus 1 encoded). >> >> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar> >> --- >> drivers/nvmem/Kconfig | 9 ++ >> drivers/nvmem/Makefile | 2 + >> drivers/nvmem/lpc18xx_eeprom.c | 266 +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 277 insertions(+) >> create mode 100644 drivers/nvmem/lpc18xx_eeprom.c > >> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg, >> + size_t reg_size, const void *val, >> + size_t val_size) >> +{ >> + struct lpc18xx_eeprom_dev *eeprom = context; >> + unsigned int offset = *(u32 *)reg; >> + >> + /* 3 ms of erase/program time between each writing */ >> + while (val_size) { >> + writel(*(u32 *)val, eeprom->mem_base + offset); >> + usleep_range(3000, 4000); >> + val_size -= eeprom->val_bytes; >> + val += eeprom->val_bytes; >> + offset += eeprom->val_bytes; >> + } > > What happens here if 'val_size' is less than 4 or not dividable by 4? AFAIK, before calling to gather_write, the caller ensures that 'val_size' % map->format.val_bytes == 0 Like nvmem_device_write() do this in drivers/base/regmap/regmap.c:regmap_raw_write() Then, if val_size == 0, nothing will happen. > Same thing for 'offset'. Let me check this one. > > I tested the driver from sysfs by writing strings into the nvmem-file > with echo. Writing a string not dividable by 4 seems to hang the > system. Yeah, I saw that too. I think that's a nvmem core issue and Srinivas's patch solves this. Will confirm that as soon as I have the board back. > > >> +static int lpc18xx_eeprom_read(void *context, const void *reg, size_t reg_size, >> + void *val, size_t val_size) >> +{ >> + struct lpc18xx_eeprom_dev *eeprom = context; >> + unsigned int offset = *(u32 *)reg; >> + >> + while (val_size) { >> + *(u32 *)val = readl(eeprom->mem_base + offset); >> + val_size -= eeprom->val_bytes; >> + val += eeprom->val_bytes; >> + offset += eeprom->val_bytes; >> + } >> + >> + return 0; >> +} > > Same comments as for lpc18xx_eeprom_gather_write(). Same answer. > > >> +static const struct of_device_id lpc18xx_eeprom_of_match[] = { >> + { .compatible = "nxp,lpc1857-eeprom" }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, lpc18xx_eeprom_of_match); > > nit: It's usual to place of_device_id struct just above the > platform_driver struct. I see. > > >> + eeprom->val_bytes = lpc18xx_regmap_config.val_bits / 8; >> + eeprom->reg_bytes = lpc18xx_regmap_config.reg_bits / 8; > > There is a BITS_PER_BYTE define in bitops.h that you might want to use here. Right. Thanks. > > >> + /* >> + * Clock rate is generated by dividing the system bus clock by the >> + * division factor, contained in the divider register (minus 1 encoded). >> + */ >> + clk_rate = clk_get_rate(eeprom->clk); >> + clk_rate = DIV_ROUND_UP(clk_rate, LPC18XX_EEPROM_CLOCK_HZ) - 1; >> + lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_CLKDIV, clk_rate); >> + >> + /* >> + * Writing a single word to the page will start the erase/program cycle >> + * automatically >> + */ >> + lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_AUTOPROG, >> + LPC18XX_EEPROM_AUTOPROG_WORD); >> + >> + lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN, >> + LPC18XX_EEPROM_PWRDWN_NO); >> + >> + lpc18xx_regmap_config.max_register = resource_size(res) - 1; >> + lpc18xx_regmap_config.writeable_reg = lpc18xx_eeprom_writeable_reg; >> + lpc18xx_regmap_config.readable_reg = lpc18xx_eeprom_readable_reg; >> + >> + regmap = devm_regmap_init(dev, &lpc18xx_eeprom_bus, eeprom, >> + &lpc18xx_regmap_config); >> + if (IS_ERR(regmap)) { >> + dev_err(dev, "regmap init failed: %ld\n", PTR_ERR(regmap)); >> + ret = PTR_ERR(regmap); >> + goto err_clk; >> + } >> + >> + lpc18xx_nvmem_config.dev = dev; >> + >> + eeprom->nvmem = nvmem_register(&lpc18xx_nvmem_config); >> + if (IS_ERR(eeprom->nvmem)) { >> + ret = PTR_ERR(eeprom->nvmem); >> + goto err_clk; >> + } >> + >> + platform_set_drvdata(pdev, eeprom); >> + >> + return 0; >> + >> +err_clk: >> + clk_disable_unprepare(eeprom->clk); >> + >> + return ret; >> +} >> + >> +static int lpc18xx_eeprom_remove(struct platform_device *pdev) >> +{ >> + struct lpc18xx_eeprom_dev *eeprom = platform_get_drvdata(pdev); >> + >> + lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN, >> + LPC18XX_EEPROM_PWRDWN_YES); >> + >> + clk_disable_unprepare(eeprom->clk); >> + >> + return nvmem_unregister(eeprom->nvmem); > > Normally you do tear down in the reverse order of initialization. > > Consider what happens here when you power down and disable the clock > while there still are nvmem users of the eeprom. You're right! Will fix this. Thanks again. -- Ariel D'Alessandro, VanguardiaSur www.vanguardiasur.com.ar
next prev parent reply other threads:[~2015-10-30 14:55 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-10-19 17:32 [PATCH v2 0/4] Add support for NXP LPC18xx EEPROM using nvmem Ariel D'Alessandro 2015-10-19 17:32 ` Ariel D'Alessandro [not found] ` <1445275946-32653-1-git-send-email-ariel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> 2015-10-19 17:32 ` [PATCH v2 1/4] DT: nvmem: Add NXP LPC18xx EEPROM memory binding documentation Ariel D'Alessandro 2015-10-19 17:32 ` Ariel D'Alessandro [not found] ` <1445275946-32653-2-git-send-email-ariel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> 2015-10-24 21:44 ` Joachim Eastwood 2015-10-24 21:44 ` Joachim Eastwood [not found] ` <CAGhQ9VyCBTWh6qgZ__eNLqooNURsYr9ZVtDz2qCKoa0MoVgXtA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-10-30 12:45 ` Ariel D'Alessandro 2015-10-30 12:45 ` Ariel D'Alessandro 2015-10-27 7:49 ` Rob Herring 2015-10-27 7:49 ` Rob Herring 2015-10-19 17:32 ` [PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver Ariel D'Alessandro 2015-10-19 17:32 ` Ariel D'Alessandro [not found] ` <1445275946-32653-3-git-send-email-ariel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> 2015-10-24 22:04 ` Joachim Eastwood 2015-10-24 22:04 ` Joachim Eastwood [not found] ` <CAGhQ9Vyg6sScq7yM=7judsMPHOc5VF2zf=7LPxmbmL7wF=vvgw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-10-26 13:37 ` Srinivas Kandagatla 2015-10-26 13:37 ` Srinivas Kandagatla [not found] ` <562E2CB1.80706-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2015-10-30 14:58 ` Ariel D'Alessandro 2015-10-30 14:58 ` Ariel D'Alessandro [not found] ` <563385B2.90403-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> 2015-11-16 15:33 ` Ariel D'Alessandro 2015-11-16 15:33 ` Ariel D'Alessandro [not found] ` <5649F74A.9020706-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> 2015-11-16 15:37 ` Srinivas Kandagatla 2015-11-16 15:37 ` Srinivas Kandagatla [not found] ` <5649F856.9000101-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2015-12-03 18:39 ` Ezequiel Garcia 2015-12-03 18:39 ` Ezequiel Garcia 2015-10-30 14:55 ` Ariel D'Alessandro [this message] 2015-10-30 14:55 ` Ariel D'Alessandro [not found] ` <563384CB.3070607-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> 2015-11-16 15:24 ` Ariel D'Alessandro 2015-11-16 15:24 ` Ariel D'Alessandro 2015-10-26 14:23 ` Srinivas Kandagatla 2015-10-26 14:23 ` Srinivas Kandagatla [not found] ` <562E377A.3040604-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2015-10-30 15:42 ` Ariel D'Alessandro 2015-10-30 15:42 ` Ariel D'Alessandro [not found] ` <56338FF4.8050102-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> 2015-10-30 16:00 ` Ezequiel Garcia 2015-10-30 16:00 ` Ezequiel Garcia 2015-11-03 8:20 ` Stefan Wahren 2015-11-03 8:20 ` Stefan Wahren [not found] ` <56386E30.4060905-eS4NqCHxEME@public.gmane.org> 2015-11-16 15:29 ` Ariel D'Alessandro 2015-11-16 15:29 ` Ariel D'Alessandro [not found] ` <5649F64B.5050407-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> 2015-11-17 10:01 ` Stefan Wahren 2015-11-17 10:01 ` Stefan Wahren [not found] ` <1526033037.5264.1447754499675.JavaMail.open-xchange-h4m1HHXQYNFdfASV6gReHsgmgJlYmuWJ@public.gmane.org> 2015-11-17 19:53 ` Ariel D'Alessandro 2015-11-17 19:53 ` Ariel D'Alessandro 2015-10-19 17:32 ` [PATCH v2 3/4] ARM: dts: lpc18xx: add EEPROM memory node Ariel D'Alessandro 2015-10-19 17:32 ` Ariel D'Alessandro [not found] ` <1445275946-32653-4-git-send-email-ariel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> 2015-10-24 21:42 ` Joachim Eastwood 2015-10-24 21:42 ` Joachim Eastwood 2015-10-19 17:32 ` [PATCH v2 4/4] ARM: configs: lpc18xx: enable EEPROM NVMEM driver Ariel D'Alessandro 2015-10-19 17:32 ` Ariel D'Alessandro [not found] ` <1445275946-32653-5-git-send-email-ariel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org> 2015-10-24 21:41 ` Joachim Eastwood 2015-10-24 21:41 ` Joachim Eastwood
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=563384CB.3070607@vanguardiasur.com.ar \ --to=ariel-30ulvvutt6g51wmpkgsgjgyuob5fgqpz@public.gmane.org \ --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=ezequiel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org \ --cc=galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org \ --cc=ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org \ --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \ --cc=manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \ --cc=maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org \ --cc=pawel.moll-5wv7dgnIgG8@public.gmane.org \ --cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \ --cc=srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.