Discussion:
[PATCH 1/2 v2] mtd: maps: Leave assigned complex mappings
Linus Walleij
2018-11-12 21:07:28 UTC
Permalink
The simple_map_init() may need to be called with some
function pointers already assigned for complex mappings,
just bail out if complex handlers have already been
assigned.

Signed-off-by: Linus Walleij <***@linaro.org>
---
ChangeLog v1->v2:
- Rebase on latest MTD development branch
- Use a new approach as the code changed under me
---
drivers/mtd/maps/map_funcs.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/maps/map_funcs.c b/drivers/mtd/maps/map_funcs.c
index 3f268370eeca..60e132293e1c 100644
--- a/drivers/mtd/maps/map_funcs.c
+++ b/drivers/mtd/maps/map_funcs.c
@@ -31,6 +31,10 @@ static void __xipram simple_map_copy_to(struct map_info *map, unsigned long to,

void simple_map_init(struct map_info *map)
{
+ /* Complex map functions already assigned */
+ if (map->read)
+ return;
+
BUG_ON(!map_bankwidth_supported(map->bankwidth));

map->read = simple_map_read;
--
2.19.1
Linus Walleij
2018-11-12 21:07:29 UTC
Permalink
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 <***@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;
}
--
2.19.1
Boris Brezillon
2018-11-20 14:36:02 UTC
Permalink
On Mon, 12 Nov 2018 22:07:29 +0100
Post by Linus Walleij
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
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...
Post by Linus Walleij
Collect all the state variables in a struct named
struct gemini_flash and allocate this struct at probe
time.
---
- 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;
}
Linus Walleij
2018-11-22 14:10:47 UTC
Permalink
On Tue, Nov 20, 2018 at 3:36 PM Boris Brezillon
Post by Boris Brezillon
On Mon, 12 Nov 2018 22:07:29 +0100
Post by Linus Walleij
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
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.
Post by Boris Brezillon
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
Boris Brezillon
2018-11-30 11:17:30 UTC
Permalink
On Thu, 22 Nov 2018 15:10:47 +0100
Post by Linus Walleij
On Tue, Nov 20, 2018 at 3:36 PM Boris Brezillon
Post by Boris Brezillon
On Mon, 12 Nov 2018 22:07:29 +0100
Post by Linus Walleij
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
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.
Post by Boris Brezillon
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.
Post by Linus Walleij
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.
Linus Walleij
2018-11-30 15:14:20 UTC
Permalink
On Fri, Nov 30, 2018 at 12:17 PM Boris Brezillon
Post by Boris Brezillon
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.
Post by Boris Brezillon
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

Boris Brezillon
2018-11-20 13:43:17 UTC
Permalink
Hi Linus,

On Mon, 12 Nov 2018 22:07:28 +0100
Post by Linus Walleij
The simple_map_init() may need to be called with some
function pointers already assigned for complex mappings,
just bail out if complex handlers have already been
assigned.
---
- Rebase on latest MTD development branch
- Use a new approach as the code changed under me
---
drivers/mtd/maps/map_funcs.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mtd/maps/map_funcs.c b/drivers/mtd/maps/map_funcs.c
index 3f268370eeca..60e132293e1c 100644
--- a/drivers/mtd/maps/map_funcs.c
+++ b/drivers/mtd/maps/map_funcs.c
@@ -31,6 +31,10 @@ static void __xipram simple_map_copy_to(struct map_info *map, unsigned long to,
void simple_map_init(struct map_info *map)
{
+ /* Complex map functions already assigned */
+ if (map->read)
+ return;
+
BUG_ON(!map_bankwidth_supported(map->bankwidth));
map->read = simple_map_read;
Can we move that to the physmap driver?

--->8---
diff --git a/drivers/mtd/maps/physmap-core.c b/drivers/mtd/maps/physmap-core.c
index e8c3b250d842..043c7de39757 100644
--- a/drivers/mtd/maps/physmap-core.c
+++ b/drivers/mtd/maps/physmap-core.c
@@ -514,10 +514,16 @@ static int physmap_flash_probe(struct platform_device *dev)
err = physmap_addr_gpios_map_init(&info->maps[i]);
if (err)
goto err_out;
- } else {
- simple_map_init(&info->maps[i]);
}

+ /*
+ * Only use the simple_map implementation if map hooks are not
+ * implemented. Since map->read() is mandatory checking for its
+ * presence is enough.
+ */
+ if (map->read)
+ simple_map_init(&info->maps[i]);
+
if (info->probe_type) {
info->mtds[i] = do_map_probe(info->probe_type,
&info->maps[i]);
Loading...