| Message ID | 20181112210729.23580-2-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2,v2] mtd: maps: Leave assigned complex mappings | expand |
On Mon, 12 Nov 2018 22:07:29 +0100 Linus Walleij <linus.walleij@linaro.org> wrote: > This enables the complex mapping for the Gemini and kicks in > custom read/write functions that will wrap the existing > simple functions in calls to enable/disable the parallel > flash pins using pin controls. > > This is necessary on some hardware such as the D-Link > DIR-685 where all flash pins are patched in/out at the same > time, but some of the flash pins are in practice unused by > the flash and have anyway been reused as GPIO. > > This concerns specifically CE1 on the Gemini. There is only > one flash chip, so only CE0 is used, and the line for CE1 > has been reused as chip select for the emulated SPI port > connected to the display. If we try to use the same lines > for flash and GPIO at the same time, one of them will loose: > the GPIO line will disappear because it gets disconnected > from the pin when the flash group is muxed out. > > Fix this by introducing two pin control states named simply > "enabled" and "disabled" and only enable the flash lines > when absolutely necessary (during read/write/copy). This > way, they are available for GPIO at all other times and > the display works. Okay, so your GPIOs might be unavailable when the flash is accessed? How does that work in practice? Is CE1 muxed as a GPIO on demand? What happens if the flash and SPI bus are accessed at the same time? Do you get -EBUSY? Not sure all MTD users retry when they get -EBUSY... > > Collect all the state variables in a struct named > struct gemini_flash and allocate this struct at probe > time. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - Rebase on latest MTD development branch > --- > drivers/mtd/maps/Kconfig | 1 + > drivers/mtd/maps/physmap-gemini.c | 110 +++++++++++++++++++++++++++++- > 2 files changed, 110 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig > index 2edd8bcdbe1c..e0cf869c8544 100644 > --- a/drivers/mtd/maps/Kconfig > +++ b/drivers/mtd/maps/Kconfig > @@ -88,6 +88,7 @@ config MTD_PHYSMAP_GEMINI > bool "Cortina Gemini OF-based physical memory map handling" > depends on MTD_PHYSMAP_OF > depends on MFD_SYSCON > + select MTD_COMPLEX_MAPPINGS > default ARCH_GEMINI > help > This provides some extra DT physmap parsing for the Gemini > diff --git a/drivers/mtd/maps/physmap-gemini.c b/drivers/mtd/maps/physmap-gemini.c > index 1cf128a0526d..60775b208fc9 100644 > --- a/drivers/mtd/maps/physmap-gemini.c > +++ b/drivers/mtd/maps/physmap-gemini.c > @@ -10,9 +10,11 @@ > #include <linux/of.h> > #include <linux/of_device.h> > #include <linux/mtd/map.h> > +#include <linux/mtd/xip.h> > #include <linux/mfd/syscon.h> > #include <linux/regmap.h> > #include <linux/bitops.h> > +#include <linux/pinctrl/consumer.h> > #include "physmap-gemini.h" > > /* > @@ -44,6 +46,82 @@ > > #define FLASH_PARALLEL_HIGH_PIN_CNT (1 << 20) /* else low pin cnt */ > > +static const struct of_device_id syscon_match[] = { > + { .compatible = "cortina,gemini-syscon" }, > + { }, > +}; > + > +struct gemini_flash { > + struct device *dev; > + struct pinctrl *p; > + struct pinctrl_state *enabled_state; > + struct pinctrl_state *disabled_state; > +}; > + > +/* Static local state */ > +static struct gemini_flash *gf; > + > +static void gemini_flash_enable_pins(void) > +{ > + int ret; > + > + if (IS_ERR(gf->enabled_state)) > + return; > + ret = pinctrl_select_state(gf->p, gf->enabled_state); > + if (ret) > + dev_err(gf->dev, "failed to enable pins\n"); > +} > + > +static void gemini_flash_disable_pins(void) > +{ > + int ret; > + > + if (IS_ERR(gf->disabled_state)) > + return; > + ret = pinctrl_select_state(gf->p, gf->disabled_state); > + if (ret) > + dev_err(gf->dev, "failed to disable pins\n"); > +} > + > +static map_word __xipram gemini_flash_map_read(struct map_info *map, > + unsigned long ofs) > +{ > + map_word __xipram ret; > + > + gemini_flash_enable_pins(); > + ret = inline_map_read(map, ofs); > + gemini_flash_disable_pins(); > + > + return ret; > +} > + > +static void __xipram gemini_flash_map_write(struct map_info *map, > + const map_word datum, > + unsigned long ofs) > +{ > + gemini_flash_enable_pins(); > + inline_map_write(map, datum, ofs); > + gemini_flash_disable_pins(); > +} > + > +static void __xipram gemini_flash_map_copy_from(struct map_info *map, > + void *to, unsigned long from, > + ssize_t len) > +{ > + gemini_flash_enable_pins(); > + inline_map_copy_from(map, to, from, len); > + gemini_flash_disable_pins(); > +} > + > +static void __xipram gemini_flash_map_copy_to(struct map_info *map, > + unsigned long to, > + const void *from, ssize_t len) > +{ > + gemini_flash_enable_pins(); > + inline_map_copy_to(map, to, from, len); > + gemini_flash_disable_pins(); > +} > + > int of_flash_probe_gemini(struct platform_device *pdev, > struct device_node *np, > struct map_info *map) > @@ -57,6 +135,11 @@ int of_flash_probe_gemini(struct platform_device *pdev, > if (!of_device_is_compatible(np, "cortina,gemini-flash")) > return 0; > > + gf = devm_kzalloc(dev, sizeof(*gf), GFP_KERNEL); > + if (!gf) > + return -ENOMEM; > + gf->dev = dev; > + > rmap = syscon_regmap_lookup_by_phandle(np, "syscon"); > if (IS_ERR(rmap)) { > dev_err(dev, "no syscon\n"); > @@ -91,7 +174,32 @@ int of_flash_probe_gemini(struct platform_device *pdev, > map->bankwidth * 8); > } > > - dev_info(&pdev->dev, "initialized Gemini-specific physmap control\n"); > + gf->p = devm_pinctrl_get(dev); > + if (IS_ERR(gf->p)) { > + dev_err(dev, "no pinctrl handle\n"); > + ret = PTR_ERR(gf->p); > + return ret; > + } > + > + gf->enabled_state = pinctrl_lookup_state(gf->p, "enabled"); > + if (IS_ERR(gf->enabled_state)) > + dev_err(dev, "no enabled pin control state\n"); > + > + gf->disabled_state = pinctrl_lookup_state(gf->p, "disabled"); > + if (IS_ERR(gf->enabled_state)) { > + dev_err(dev, "no disabled pin control state\n"); > + } else { > + ret = pinctrl_select_state(gf->p, gf->disabled_state); > + if (ret) > + dev_err(gf->dev, "failed to disable pins\n"); > + } > + > + map->read = gemini_flash_map_read; > + map->write = gemini_flash_map_write; > + map->copy_from = gemini_flash_map_copy_from; > + map->copy_to = gemini_flash_map_copy_to; > + > + dev_info(dev, "initialized Gemini-specific physmap control\n"); > > return 0; > } ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Tue, Nov 20, 2018 at 3:36 PM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Mon, 12 Nov 2018 22:07:29 +0100 > Linus Walleij <linus.walleij@linaro.org> wrote: > > > This enables the complex mapping for the Gemini and kicks in > > custom read/write functions that will wrap the existing > > simple functions in calls to enable/disable the parallel > > flash pins using pin controls. > > > > This is necessary on some hardware such as the D-Link > > DIR-685 where all flash pins are patched in/out at the same > > time, but some of the flash pins are in practice unused by > > the flash and have anyway been reused as GPIO. > > > > This concerns specifically CE1 on the Gemini. There is only > > one flash chip, so only CE0 is used, and the line for CE1 > > has been reused as chip select for the emulated SPI port > > connected to the display. If we try to use the same lines > > for flash and GPIO at the same time, one of them will loose: > > the GPIO line will disappear because it gets disconnected > > from the pin when the flash group is muxed out. > > > > Fix this by introducing two pin control states named simply > > "enabled" and "disabled" and only enable the flash lines > > when absolutely necessary (during read/write/copy). This > > way, they are available for GPIO at all other times and > > the display works. > > Okay, so your GPIOs might be unavailable when the flash is accessed? > > How does that work in practice? Is CE1 muxed as a GPIO > on demand? Yes, well the actual pin will go into the state of CE1 which default to a low state, the effect is hardware-defined. This is not a recommendable type of electronics engineering if that is what you wonder... it's a way to patch over design mistakes in the design with software. (Which is what the vendor tree does.) In one case (SQ201) the LED connected to them will light up since it is active low. So the LED goes on during flash access and gives a kind of "flash access flicker" on top of whatever it was indicating before. That LED is marked "power" and I put a heartbeat on it so it will be brief interruptions of steady light in the heartbeat. The "only" problem is in UX. The vendor device exhibits the same strange behavior. > What happens if the flash and SPI bus are > accessed at the same time? Do you > get -EBUSY? Not sure all MTD users retry when they > get -EBUSY... The way I set it up flash access "always win", so no worries for MTD. If I simulate SPI or any other slow bus on the GPIO at the same time as we have muxed it out for flash access, it will have to do error recovery, because the bytes will not get through. Ideally I would want to patch the GPIO library to redirect the access to this line to enable/disable CE1 so the pin is driven from the flash controller instead. But since that is essentially an address bit it looked pretty tricky. In practice the use cases do not overlap, so we are fine, I guess if problems still occur the path of least resistance would simply be to have the display driver using this SPI line delay and retry a few times. It's an ugly solution perfectly fitting the ugly electronics IMO. Yours, Linus Walleij ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Thu, 22 Nov 2018 15:10:47 +0100 Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Nov 20, 2018 at 3:36 PM Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Mon, 12 Nov 2018 22:07:29 +0100 > > Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > This enables the complex mapping for the Gemini and kicks in > > > custom read/write functions that will wrap the existing > > > simple functions in calls to enable/disable the parallel > > > flash pins using pin controls. > > > > > > This is necessary on some hardware such as the D-Link > > > DIR-685 where all flash pins are patched in/out at the same > > > time, but some of the flash pins are in practice unused by > > > the flash and have anyway been reused as GPIO. > > > > > > This concerns specifically CE1 on the Gemini. There is only > > > one flash chip, so only CE0 is used, and the line for CE1 > > > has been reused as chip select for the emulated SPI port > > > connected to the display. If we try to use the same lines > > > for flash and GPIO at the same time, one of them will loose: > > > the GPIO line will disappear because it gets disconnected > > > from the pin when the flash group is muxed out. > > > > > > Fix this by introducing two pin control states named simply > > > "enabled" and "disabled" and only enable the flash lines > > > when absolutely necessary (during read/write/copy). This > > > way, they are available for GPIO at all other times and > > > the display works. > > > > Okay, so your GPIOs might be unavailable when the flash is accessed? > > > > How does that work in practice? Is CE1 muxed as a GPIO > > on demand? > > Yes, well the actual pin will go into the state of CE1 > which default to a low state, the effect is > hardware-defined. > > This is not a recommendable type of electronics > engineering if that is what you wonder... it's a way to > patch over design mistakes in the design with > software. (Which is what the vendor tree does.) > > In one case (SQ201) the LED connected to them will light up > since it is active low. So the LED goes on during flash > access and gives a kind of "flash access flicker" on top > of whatever it was indicating before. > > That LED is marked "power" and I put a heartbeat on it > so it will be brief interruptions of steady light in the > heartbeat. The "only" problem is in UX. The vendor > device exhibits the same strange behavior. > > > What happens if the flash and SPI bus are > > accessed at the same time? Do you > > get -EBUSY? Not sure all MTD users retry when they > > get -EBUSY... > > The way I set it up flash access "always win", so > no worries for MTD. > > If I simulate SPI or any other slow bus on the GPIO > at the same time as we have muxed it out for > flash access, it will have to do error recovery, > because the bytes will not get through. Do you mean that the GPIO controller does not request the pins it's using? If it does, the pinctrl framework should complain, when the pins are already assigned to another dev. > > Ideally I would want to patch the GPIO library to > redirect the access to this line to enable/disable > CE1 so the pin is driven from the flash controller > instead. But since that is essentially an address > bit it looked pretty tricky. > > In practice the use cases do not overlap, so > we are fine, I guess if problems still occur the > path of least resistance would simply be to have the > display driver using this SPI line delay and > retry a few times. It's an ugly solution perfectly > fitting the ugly electronics IMO. If the flash and SPI bus are never accessed concurrently and things are controlled from userspace, maybe it's cleaner to bind/unbind devices on demand, and let each driver mux the conflicting pins as they expect it to be muxed. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Fri, Nov 30, 2018 at 12:17 PM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Thu, 22 Nov 2018 15:10:47 +0100 Linus Walleij <linus.walleij@linaro.org> wrote: > Do you mean that the GPIO controller does not request the pins it's > using? If it does, the pinctrl framework should complain, when the pins > are already assigned to another dev. That depends on whether the pin controller is "strict" or not. Several pin controllers actually allow the pin to be used for some device and GPIO at the same time. Gemini is not strict, there is a field in struct pinmux_ops that controls strictness and this depends on the hardware and use cases. I would prefer if the world was simpler, but sadly hardware engineers doing this kind of stuff don't know what that means. > If the flash and SPI bus are never accessed concurrently and things are > controlled from userspace, maybe it's cleaner to bind/unbind devices on > demand, and let each driver mux the conflicting pins as they expect it > to be muxed. These are in-kernel use cases. The problem is not entirely new: if we want a pin to be absolutely exclusively under control of a certain hardware block, or even another CPU, we use a hardware semaphore, see for example how STM32 does it: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/commit/?h=devel&id=97cfb6cd34f2a3dab03dce5f885333c213bbec8a The same can of course be done using software, somehow, with something like a global mutex that the SPI host and MTD need to take respectively. (I imagine this would be retrieved as a resource like any other like regulator, clock etc). However implementing that when there isn't even a clash in reality, just in theory, and only if I construct specific test programs to make this happen ... I dunno it's just a bit overly ambitious. This router is already working fine, so... Yours, Linus Walleij ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig index 2edd8bcdbe1c..e0cf869c8544 100644 --- a/drivers/mtd/maps/Kconfig +++ b/drivers/mtd/maps/Kconfig @@ -88,6 +88,7 @@ config MTD_PHYSMAP_GEMINI bool "Cortina Gemini OF-based physical memory map handling" depends on MTD_PHYSMAP_OF depends on MFD_SYSCON + select MTD_COMPLEX_MAPPINGS default ARCH_GEMINI help This provides some extra DT physmap parsing for the Gemini diff --git a/drivers/mtd/maps/physmap-gemini.c b/drivers/mtd/maps/physmap-gemini.c index 1cf128a0526d..60775b208fc9 100644 --- a/drivers/mtd/maps/physmap-gemini.c +++ b/drivers/mtd/maps/physmap-gemini.c @@ -10,9 +10,11 @@ #include <linux/of.h> #include <linux/of_device.h> #include <linux/mtd/map.h> +#include <linux/mtd/xip.h> #include <linux/mfd/syscon.h> #include <linux/regmap.h> #include <linux/bitops.h> +#include <linux/pinctrl/consumer.h> #include "physmap-gemini.h" /* @@ -44,6 +46,82 @@ #define FLASH_PARALLEL_HIGH_PIN_CNT (1 << 20) /* else low pin cnt */ +static const struct of_device_id syscon_match[] = { + { .compatible = "cortina,gemini-syscon" }, + { }, +}; + +struct gemini_flash { + struct device *dev; + struct pinctrl *p; + struct pinctrl_state *enabled_state; + struct pinctrl_state *disabled_state; +}; + +/* Static local state */ +static struct gemini_flash *gf; + +static void gemini_flash_enable_pins(void) +{ + int ret; + + if (IS_ERR(gf->enabled_state)) + return; + ret = pinctrl_select_state(gf->p, gf->enabled_state); + if (ret) + dev_err(gf->dev, "failed to enable pins\n"); +} + +static void gemini_flash_disable_pins(void) +{ + int ret; + + if (IS_ERR(gf->disabled_state)) + return; + ret = pinctrl_select_state(gf->p, gf->disabled_state); + if (ret) + dev_err(gf->dev, "failed to disable pins\n"); +} + +static map_word __xipram gemini_flash_map_read(struct map_info *map, + unsigned long ofs) +{ + map_word __xipram ret; + + gemini_flash_enable_pins(); + ret = inline_map_read(map, ofs); + gemini_flash_disable_pins(); + + return ret; +} + +static void __xipram gemini_flash_map_write(struct map_info *map, + const map_word datum, + unsigned long ofs) +{ + gemini_flash_enable_pins(); + inline_map_write(map, datum, ofs); + gemini_flash_disable_pins(); +} + +static void __xipram gemini_flash_map_copy_from(struct map_info *map, + void *to, unsigned long from, + ssize_t len) +{ + gemini_flash_enable_pins(); + inline_map_copy_from(map, to, from, len); + gemini_flash_disable_pins(); +} + +static void __xipram gemini_flash_map_copy_to(struct map_info *map, + unsigned long to, + const void *from, ssize_t len) +{ + gemini_flash_enable_pins(); + inline_map_copy_to(map, to, from, len); + gemini_flash_disable_pins(); +} + int of_flash_probe_gemini(struct platform_device *pdev, struct device_node *np, struct map_info *map) @@ -57,6 +135,11 @@ int of_flash_probe_gemini(struct platform_device *pdev, if (!of_device_is_compatible(np, "cortina,gemini-flash")) return 0; + gf = devm_kzalloc(dev, sizeof(*gf), GFP_KERNEL); + if (!gf) + return -ENOMEM; + gf->dev = dev; + rmap = syscon_regmap_lookup_by_phandle(np, "syscon"); if (IS_ERR(rmap)) { dev_err(dev, "no syscon\n"); @@ -91,7 +174,32 @@ int of_flash_probe_gemini(struct platform_device *pdev, map->bankwidth * 8); } - dev_info(&pdev->dev, "initialized Gemini-specific physmap control\n"); + gf->p = devm_pinctrl_get(dev); + if (IS_ERR(gf->p)) { + dev_err(dev, "no pinctrl handle\n"); + ret = PTR_ERR(gf->p); + return ret; + } + + gf->enabled_state = pinctrl_lookup_state(gf->p, "enabled"); + if (IS_ERR(gf->enabled_state)) + dev_err(dev, "no enabled pin control state\n"); + + gf->disabled_state = pinctrl_lookup_state(gf->p, "disabled"); + if (IS_ERR(gf->enabled_state)) { + dev_err(dev, "no disabled pin control state\n"); + } else { + ret = pinctrl_select_state(gf->p, gf->disabled_state); + if (ret) + dev_err(gf->dev, "failed to disable pins\n"); + } + + map->read = gemini_flash_map_read; + map->write = gemini_flash_map_write; + map->copy_from = gemini_flash_map_copy_from; + map->copy_to = gemini_flash_map_copy_to; + + dev_info(dev, "initialized Gemini-specific physmap control\n"); return 0; }
This enables the complex mapping for the Gemini and kicks in custom read/write functions that will wrap the existing simple functions in calls to enable/disable the parallel flash pins using pin controls. This is necessary on some hardware such as the D-Link DIR-685 where all flash pins are patched in/out at the same time, but some of the flash pins are in practice unused by the flash and have anyway been reused as GPIO. This concerns specifically CE1 on the Gemini. There is only one flash chip, so only CE0 is used, and the line for CE1 has been reused as chip select for the emulated SPI port connected to the display. If we try to use the same lines for flash and GPIO at the same time, one of them will loose: the GPIO line will disappear because it gets disconnected from the pin when the flash group is muxed out. Fix this by introducing two pin control states named simply "enabled" and "disabled" and only enable the flash lines when absolutely necessary (during read/write/copy). This way, they are available for GPIO at all other times and the display works. Collect all the state variables in a struct named struct gemini_flash and allocate this struct at probe time. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v1->v2: - Rebase on latest MTD development branch --- drivers/mtd/maps/Kconfig | 1 + drivers/mtd/maps/physmap-gemini.c | 110 +++++++++++++++++++++++++++++- 2 files changed, 110 insertions(+), 1 deletion(-) -- 2.19.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/