[PATCH v10 09/10] dt-binding: mtd: Document gpio-addr-flash

[PATCH v10 09/10] dt-binding: mtd: Document gpio-addr-flash

[Ricardo Ribalda Delgado]

Add documentation for gpio-addr-flash. This binding allow creating
flash devices that are paged using GPIOs.

Cc: devicetree@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 Documentation/devicetree/bindings/mtd/mtd-physmap.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
index 232fa12e90ef..7df0dcaccb7d 100644
--- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
+++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
@@ -29,6 +29,8 @@ file systems on embedded devices.
  - use-advanced-sector-protection: boolean to enable support for the
    advanced sector protection (Spansion: PPB - Persistent Protection
    Bits) locking.
+ - addr-gpios : (optional) List of GPIO descriptors that will be used to
+   address the MSBs address lines. The order goes from LSB to MSB.
 
 For JEDEC compatible devices, the following additional properties
 are defined:
-- 
2.19.0

[PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

Allow creating gpio-addr-flash via device-tree and not just via platform
data.

Mimic what physmap_of_versatile and physmap_of_gemini does to reduce
code duplicity.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/mtd/maps/Kconfig           |  8 +++
 drivers/mtd/maps/Makefile          |  3 +-
 drivers/mtd/maps/gpio-addr-flash.c | 95 +++++++++++++++++++-----------
 drivers/mtd/maps/gpio-addr-flash.h | 34 +++++++++++
 drivers/mtd/maps/physmap_of_core.c |  5 ++
 drivers/mtd/maps/physmap_of_gpio.c | 21 +++++++
 6 files changed, 129 insertions(+), 37 deletions(-)
 create mode 100644 drivers/mtd/maps/gpio-addr-flash.h
 create mode 100644 drivers/mtd/maps/physmap_of_gpio.c

diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
index afb36bff13a7..427143d42168 100644
--- a/drivers/mtd/maps/Kconfig
+++ b/drivers/mtd/maps/Kconfig
@@ -94,6 +94,14 @@ config MTD_PHYSMAP_OF_GEMINI
 	  platforms, some detection and setting up parallel mode on the
 	  external interface.
 
+config MTD_PHYSMAP_OF_GPIO
+	bool "GPIO-assisted OF-based physical memory map handling"
+	depends on MTD_PHYSMAP_OF
+	depends on MTD_GPIO_ADDR
+	help
+	  This provides some extra DT physmap parsing for flashes that are
+	  partially physically addressed and assisted by GPIOs.
+
 config MTD_PMC_MSP_EVM
 	tristate "CFI Flash device mapped on PMC-Sierra MSP"
 	depends on PMC_MSP && MTD_CFI
diff --git a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile
index 51acf1fec19b..c232ccf05bee 100644
--- a/drivers/mtd/maps/Makefile
+++ b/drivers/mtd/maps/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_MTD_PHYSMAP)	+= physmap.o
 physmap_of-objs-y		+= physmap_of_core.o
 physmap_of-objs-$(CONFIG_MTD_PHYSMAP_OF_VERSATILE) += physmap_of_versatile.o
 physmap_of-objs-$(CONFIG_MTD_PHYSMAP_OF_GEMINI) += physmap_of_gemini.o
+physmap_of-objs-$(CONFIG_MTD_PHYSMAP_OF_GPIO) += physmap_of_gpio.o
 physmap_of-objs			:= $(physmap_of-objs-y)
 obj-$(CONFIG_MTD_PHYSMAP_OF)	+= physmap_of.o
 obj-$(CONFIG_MTD_PISMO)		+= pismo.o
@@ -44,6 +45,6 @@ obj-$(CONFIG_MTD_PLATRAM)	+= plat-ram.o
 obj-$(CONFIG_MTD_INTEL_VR_NOR)	+= intel_vr_nor.o
 obj-$(CONFIG_MTD_RBTX4939)	+= rbtx4939-flash.o
 obj-$(CONFIG_MTD_VMU)		+= vmu-flash.o
-obj-$(CONFIG_MTD_GPIO_ADDR)	+= gpio-addr-flash.o
+obj-$(CONFIG_MTD_GPIO_ADDR)    += gpio-addr-flash.o
 obj-$(CONFIG_MTD_LATCH_ADDR)	+= latch-addr-flash.o
 obj-$(CONFIG_MTD_LANTIQ)	+= lantiq-flash.o
diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index a20e85aa770e..0d1f66aedf77 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -25,25 +25,25 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include "gpio-addr-flash.h"
 
 #define win_mask(x) ((BIT(x)) - 1)
 
 #define DRIVER_NAME "gpio-addr-flash"
 
+#define gf_map_info_to_state(mi) ((struct async_state *)(mi)->map_priv_1)
+
 /**
- * struct async_state - keep GPIO flash state
+ * struct async_state_pdev - Async state platform device
  *	@mtd:         MTD state for this mapping
  *	@map:         MTD map state for this flash
  *	@gpios:       Struct containing the array of GPIO descriptors
- *	@gpio_values: cached GPIO values
- *	@win_order:   dedicated memory size (if no GPIOs)
+ *	@state:       GPIO flash state
  */
-struct async_state {
+struct async_state_pdev {
 	struct mtd_info *mtd;
 	struct map_info map;
-	struct gpio_descs *gpios;
-	unsigned int gpio_values;
-	unsigned int win_order;
+	struct async_state state;
 };
 #define gf_map_info_to_state(mi) ((struct async_state *)(mi)->map_priv_1)
 
@@ -174,6 +174,35 @@ static void gf_copy_to(struct map_info *map, unsigned long to,
 static const char * const part_probe_types[] = {
 	"cmdlinepart", "RedBoot", NULL };
 
+int gpio_flash_probe_common(struct device *dev, struct async_state *state,
+			    struct map_info *map)
+{
+	if (!is_power_of_2(map->size)) {
+		dev_err(dev, "Window size must be aligned\n");
+		return -EIO;
+	}
+
+	state->gpios = devm_gpiod_get_array_optional(dev, "addr",
+						     GPIOD_OUT_LOW);
+	if (!state->gpios)
+		return 0;
+
+	if (IS_ERR(state->gpios))
+		return PTR_ERR(state->gpios);
+
+	state->win_order  = get_bitmask_order(map->size) - 1;
+	map->read       = gf_read;
+	map->copy_from  = gf_copy_from;
+	map->write      = gf_write;
+	map->copy_to    = gf_copy_to;
+	map->size       = BIT(state->win_order + state->gpios->ndescs);
+	map->phys	= NO_XIP;
+	map->map_priv_1 = (unsigned long)state;
+
+	return 0;
+}
+EXPORT_SYMBOL(gpio_flash_probe_common);
+
 /**
  * gpio_flash_probe() - setup a mapping for a GPIO assisted flash
  *	@pdev: platform device
@@ -210,7 +239,8 @@ static int gpio_flash_probe(struct platform_device *pdev)
 {
 	struct physmap_flash_data *pdata;
 	struct resource *memory;
-	struct async_state *state;
+	struct async_state_pdev *state_pdev;
+	int ret;
 
 	pdata = dev_get_platdata(&pdev->dev);
 	memory = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -218,40 +248,33 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	if (!memory)
 		return -EINVAL;
 
-	state = devm_kzalloc(&pdev->dev, sizeof(*state), GFP_KERNEL);
-	if (!state)
+	state_pdev = devm_kzalloc(&pdev->dev, sizeof(*state_pdev), GFP_KERNEL);
+	if (!state_pdev)
 		return -ENOMEM;
 
-	state->gpios = devm_gpiod_get_array(&pdev->dev, "addr", GPIOD_OUT_LOW);
-	if (IS_ERR(state->gpios))
-		return PTR_ERR(state->gpios);
-
-	state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
+	state_pdev->map.virt = devm_ioremap_resource(&pdev->dev, memory);
+	if (IS_ERR(state_pdev->map.virt))
+		return PTR_ERR(state_pdev->map.virt);
 
-	state->map.name       = DRIVER_NAME;
-	state->map.read       = gf_read;
-	state->map.copy_from  = gf_copy_from;
-	state->map.write      = gf_write;
-	state->map.copy_to    = gf_copy_to;
-	state->map.bankwidth  = pdata->width;
-	state->map.size       = BIT(state->win_order + state->gpios->ndescs);
-	state->map.virt	      = devm_ioremap_resource(&pdev->dev, memory);
-	if (IS_ERR(state->map.virt))
-		return PTR_ERR(state->map.virt);
+	state_pdev->map.name       = DRIVER_NAME;
+	state_pdev->map.bankwidth  = pdata->width;
+	state_pdev->map.size       = resource_size(memory);
 
-	state->map.phys       = NO_XIP;
-	state->map.map_priv_1 = (unsigned long)state;
+	ret = gpio_flash_probe_common(&pdev->dev, &state_pdev->state,
+				      &state_pdev->map);
+	if (ret)
+		return ret;
 
-	platform_set_drvdata(pdev, state);
+	platform_set_drvdata(pdev, state_pdev);
 
 	dev_notice(&pdev->dev, "probing %d-bit flash bus\n",
-		   state->map.bankwidth * 8);
-	state->mtd = do_map_probe(memory->name, &state->map);
-	if (!state->mtd)
+		   state_pdev->map.bankwidth * 8);
+	state_pdev->mtd = do_map_probe(memory->name, &state_pdev->map);
+	if (!state_pdev->mtd)
 		return -ENXIO;
-	state->mtd->dev.parent = &pdev->dev;
+	state_pdev->mtd->dev.parent = &pdev->dev;
 
-	mtd_device_parse_register(state->mtd, part_probe_types, NULL,
+	mtd_device_parse_register(state_pdev->mtd, part_probe_types, NULL,
 				  pdata->parts, pdata->nr_parts);
 
 	return 0;
@@ -259,10 +282,10 @@ static int gpio_flash_probe(struct platform_device *pdev)
 
 static int gpio_flash_remove(struct platform_device *pdev)
 {
-	struct async_state *state = platform_get_drvdata(pdev);
+	struct async_state_pdev *state_pdev = platform_get_drvdata(pdev);
 
-	mtd_device_unregister(state->mtd);
-	map_destroy(state->mtd);
+	mtd_device_unregister(state_pdev->mtd);
+	map_destroy(state_pdev->mtd);
 	return 0;
 }
 
diff --git a/drivers/mtd/maps/gpio-addr-flash.h b/drivers/mtd/maps/gpio-addr-flash.h
new file mode 100644
index 000000000000..443577f7fdfd
--- /dev/null
+++ b/drivers/mtd/maps/gpio-addr-flash.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <linux/of.h>
+#include <linux/mtd/map.h>
+
+/**
+ * struct async_state - keep GPIO flash state
+ *	@gpios:       Struct containing the array of GPIO descriptors
+ *	@gpio_values: cached GPIO values
+ *	@win_order:   dedicated memory size (if no GPIOs)
+ */
+struct async_state {
+	struct gpio_descs *gpios;
+	unsigned int gpio_values;
+	unsigned int win_order;
+};
+
+int gpio_flash_probe_common(struct device *dev,
+				   struct async_state *state,
+				   struct map_info *map);
+
+#ifdef CONFIG_MTD_PHYSMAP_OF_GPIO
+int of_flash_probe_gpio(struct platform_device *pdev, struct device_node *np,
+			struct map_info *map);
+#else
+static inline
+int of_flash_probe_gpio(struct platform_device *pdev, struct device_node *np,
+			struct map_info *map)
+{
+	if (of_find_property(np, "addr-gpios", NULL))
+		return -EINVAL;
+
+	return 0;
+}
+#endif
diff --git a/drivers/mtd/maps/physmap_of_core.c b/drivers/mtd/maps/physmap_of_core.c
index ece605d78c21..c187b6ff873d 100644
--- a/drivers/mtd/maps/physmap_of_core.c
+++ b/drivers/mtd/maps/physmap_of_core.c
@@ -27,6 +27,7 @@
 #include <linux/slab.h>
 #include "physmap_of_gemini.h"
 #include "physmap_of_versatile.h"
+#include "gpio-addr-flash.h"
 
 struct of_flash_list {
 	struct mtd_info *mtd;
@@ -240,6 +241,10 @@ static int of_flash_probe(struct platform_device *dev)
 
 		simple_map_init(&info->list[i].map);
 
+		err = of_flash_probe_gpio(dev, dp, &info->list[i].map);
+		if (err)
+			goto err_out;
+
 		/*
 		 * On some platforms (e.g. MPC5200) a direct 1:1 mapping
 		 * may cause problems with JFFS2 usage, as the local bus (LPB)
diff --git a/drivers/mtd/maps/physmap_of_gpio.c b/drivers/mtd/maps/physmap_of_gpio.c
new file mode 100644
index 000000000000..c84c5e96e592
--- /dev/null
+++ b/drivers/mtd/maps/physmap_of_gpio.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2018 Qtechnology A/S
+ *
+ * Ricardo Ribalda <ricardo.ribalda@gmail.com>
+ *
+ */
+#include <linux/platform_device.h>
+#include "gpio-addr-flash.h"
+
+int of_flash_probe_gpio(struct platform_device *pdev, struct device_node *np,
+			struct map_info *map)
+{
+	struct async_state *state;
+
+	state = devm_kzalloc(&pdev->dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	return gpio_flash_probe_common(&pdev->dev, state, map);
+}
-- 
2.19.0

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

Hi Ricardo,

On Thu,  4 Oct 2018 16:29:42 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Allow creating gpio-addr-flash via device-tree and not just via platform
> data.
> 
> Mimic what physmap_of_versatile and physmap_of_gemini does to reduce
> code duplicity.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/mtd/maps/Kconfig           |  8 +++
>  drivers/mtd/maps/Makefile          |  3 +-
>  drivers/mtd/maps/gpio-addr-flash.c | 95 +++++++++++++++++++-----------
>  drivers/mtd/maps/gpio-addr-flash.h | 34 +++++++++++
>  drivers/mtd/maps/physmap_of_core.c |  5 ++
>  drivers/mtd/maps/physmap_of_gpio.c | 21 +++++++
>  6 files changed, 129 insertions(+), 37 deletions(-)
>  create mode 100644 drivers/mtd/maps/gpio-addr-flash.h
>  create mode 100644 drivers/mtd/maps/physmap_of_gpio.c
> 
> diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> index afb36bff13a7..427143d42168 100644
> --- a/drivers/mtd/maps/Kconfig
> +++ b/drivers/mtd/maps/Kconfig
> @@ -94,6 +94,14 @@ config MTD_PHYSMAP_OF_GEMINI
>  	  platforms, some detection and setting up parallel mode on the
>  	  external interface.
>  
> +config MTD_PHYSMAP_OF_GPIO
> +	bool "GPIO-assisted OF-based physical memory map handling"
> +	depends on MTD_PHYSMAP_OF
> +	depends on MTD_GPIO_ADDR
> +	help
> +	  This provides some extra DT physmap parsing for flashes that are
> +	  partially physically addressed and assisted by GPIOs.
> +

Hm, so now we have the physmap_of driver which uses a function exposed
by the gpio-addr-flash module, but this module is also declaring a
platform_driver. It's probably working fine, but it's hard to follow.

So, I decided to give it a try and started to rework a bit the physmap,
physmap_of and gpio-addr-flash drivers. Here is the result [1] (it's
only been compile tested). With this rework we now have a single
driver which supports DT and !DT init and can also use GPIOs to
extend the physical memory range in case it's not large enough to
address the whole memory dev.

Let me know what you think of this approach.

Regards,

Boris

[1]https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

Hi Boris


On Fri, Oct 5, 2018 at 12:21 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> Hi Ricardo,
>
> On Thu,  4 Oct 2018 16:29:42 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > Allow creating gpio-addr-flash via device-tree and not just via platform
> > data.
> >
> > Mimic what physmap_of_versatile and physmap_of_gemini does to reduce
> > code duplicity.
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > ---
> >  drivers/mtd/maps/Kconfig           |  8 +++
> >  drivers/mtd/maps/Makefile          |  3 +-
> >  drivers/mtd/maps/gpio-addr-flash.c | 95 +++++++++++++++++++-----------
> >  drivers/mtd/maps/gpio-addr-flash.h | 34 +++++++++++
> >  drivers/mtd/maps/physmap_of_core.c |  5 ++
> >  drivers/mtd/maps/physmap_of_gpio.c | 21 +++++++
> >  6 files changed, 129 insertions(+), 37 deletions(-)
> >  create mode 100644 drivers/mtd/maps/gpio-addr-flash.h
> >  create mode 100644 drivers/mtd/maps/physmap_of_gpio.c
> >
> > diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> > index afb36bff13a7..427143d42168 100644
> > --- a/drivers/mtd/maps/Kconfig
> > +++ b/drivers/mtd/maps/Kconfig
> > @@ -94,6 +94,14 @@ config MTD_PHYSMAP_OF_GEMINI
> >         platforms, some detection and setting up parallel mode on the
> >         external interface.
> >
> > +config MTD_PHYSMAP_OF_GPIO
> > +     bool "GPIO-assisted OF-based physical memory map handling"
> > +     depends on MTD_PHYSMAP_OF
> > +     depends on MTD_GPIO_ADDR
> > +     help
> > +       This provides some extra DT physmap parsing for flashes that are
> > +       partially physically addressed and assisted by GPIOs.
> > +
>
> Hm, so now we have the physmap_of driver which uses a function exposed
> by the gpio-addr-flash module, but this module is also declaring a
> platform_driver. It's probably working fine, but it's hard to follow.
>
> So, I decided to give it a try and started to rework a bit the physmap,
> physmap_of and gpio-addr-flash drivers. Here is the result [1] (it's
> only been compile tested). With this rework we now have a single
> driver which supports DT and !DT init and can also use GPIOs to
> extend the physical memory range in case it's not large enough to
> address the whole memory dev.
>
> Let me know what you think of this approach.
>

The code is definitely easier to follow. But I believe you should
rebase your changes on mines (1-9) and instead of 10 apply your
patch-set.:
- "mtd: maps: Merge gpio-addr-flash.c into physmap-core.c" includes
all my changes and some of them are not obvious:( Port to gpiod,
gpio_values, win_order, use new apis....)
- Also there would be 1 (or 2) fixes that should be included in stable
that we lose using your approach "mtd: maps: gpio-addr-flash: Fix
ioremapped size", "mtd: physmap_of: Release resources on error"
- And last, but not least, I want some credit for all this work ;)
After 10 iterations I guess that I deserve more than a Documentation
change, specially when my code would be on the tree.

My other concern is that maybe we are giving too much entity to
gpio-addr by including it in the core. But if you are fine with it, I
am fine with it.

If you want i can do the rebase and start testing on my board. What do
you think?

Regards!



> Regards,
>
> Boris
>
> [1]https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup



--
Ricardo Ribalda

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

Hi Ricardo,

On Fri, 5 Oct 2018 08:31:35 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> 
> 
> On Fri, Oct 5, 2018 at 12:21 AM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > Hi Ricardo,
> >
> > On Thu,  4 Oct 2018 16:29:42 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> >  
> > > Allow creating gpio-addr-flash via device-tree and not just via platform
> > > data.
> > >
> > > Mimic what physmap_of_versatile and physmap_of_gemini does to reduce
> > > code duplicity.
> > >
> > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > > ---
> > >  drivers/mtd/maps/Kconfig           |  8 +++
> > >  drivers/mtd/maps/Makefile          |  3 +-
> > >  drivers/mtd/maps/gpio-addr-flash.c | 95 +++++++++++++++++++-----------
> > >  drivers/mtd/maps/gpio-addr-flash.h | 34 +++++++++++
> > >  drivers/mtd/maps/physmap_of_core.c |  5 ++
> > >  drivers/mtd/maps/physmap_of_gpio.c | 21 +++++++
> > >  6 files changed, 129 insertions(+), 37 deletions(-)
> > >  create mode 100644 drivers/mtd/maps/gpio-addr-flash.h
> > >  create mode 100644 drivers/mtd/maps/physmap_of_gpio.c
> > >
> > > diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> > > index afb36bff13a7..427143d42168 100644
> > > --- a/drivers/mtd/maps/Kconfig
> > > +++ b/drivers/mtd/maps/Kconfig
> > > @@ -94,6 +94,14 @@ config MTD_PHYSMAP_OF_GEMINI
> > >         platforms, some detection and setting up parallel mode on the
> > >         external interface.
> > >
> > > +config MTD_PHYSMAP_OF_GPIO
> > > +     bool "GPIO-assisted OF-based physical memory map handling"
> > > +     depends on MTD_PHYSMAP_OF
> > > +     depends on MTD_GPIO_ADDR
> > > +     help
> > > +       This provides some extra DT physmap parsing for flashes that are
> > > +       partially physically addressed and assisted by GPIOs.
> > > +  
> >
> > Hm, so now we have the physmap_of driver which uses a function exposed
> > by the gpio-addr-flash module, but this module is also declaring a
> > platform_driver. It's probably working fine, but it's hard to follow.
> >
> > So, I decided to give it a try and started to rework a bit the physmap,
> > physmap_of and gpio-addr-flash drivers. Here is the result [1] (it's
> > only been compile tested). With this rework we now have a single
> > driver which supports DT and !DT init and can also use GPIOs to
> > extend the physical memory range in case it's not large enough to
> > address the whole memory dev.
> >
> > Let me know what you think of this approach.
> >  
> 
> The code is definitely easier to follow. But I believe you should
> rebase your changes on mines (1-9) and instead of 10 apply your
> patch-set.:
> - "mtd: maps: Merge gpio-addr-flash.c into physmap-core.c" includes
> all my changes and some of them are not obvious:( Port to gpiod,
> gpio_values, win_order, use new apis....)

True.

> - Also there would be 1 (or 2) fixes that should be included in stable
> that we lose using your approach "mtd: maps: gpio-addr-flash: Fix
> ioremapped size", "mtd: physmap_of: Release resources on error"

I'm not sure this is really useful to backport them to stable since
blackfin users never complained about it (plus, blackin arch is now
gone), but okay.

> - And last, but not least, I want some credit for all this work ;)
> After 10 iterations I guess that I deserve more than a Documentation
> change, specially when my code would be on the tree.

Fair enough.

> 
> My other concern is that maybe we are giving too much entity to
> gpio-addr by including it in the core. But if you are fine with it, I
> am fine with it.

Well, the gpio-addr stuff is just here to support platforms that do not
have enough ADDR lines (or iomem address space) to address the whole
device. I see it as an 'extension' of the physmap logic, which is why I
think it makes sense to have this logic in the physmap driver instead
of creating a completely new driver.

> 
> If you want i can do the rebase and start testing on my board. What do
> you think?

That'd be great!

Thanks,

Boris

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

On Fri, Oct 5, 2018 at 9:08 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> Hi Ricardo,
>
> On Fri, 5 Oct 2018 08:31:35 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > Hi Boris
> >
> >
> > On Fri, Oct 5, 2018 at 12:21 AM Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Thu,  4 Oct 2018 16:29:42 +0200
> > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > >
> > > > Allow creating gpio-addr-flash via device-tree and not just via platform
> > > > data.
> > > >
> > > > Mimic what physmap_of_versatile and physmap_of_gemini does to reduce
> > > > code duplicity.
> > > >
> > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > > > ---
> > > >  drivers/mtd/maps/Kconfig           |  8 +++
> > > >  drivers/mtd/maps/Makefile          |  3 +-
> > > >  drivers/mtd/maps/gpio-addr-flash.c | 95 +++++++++++++++++++-----------
> > > >  drivers/mtd/maps/gpio-addr-flash.h | 34 +++++++++++
> > > >  drivers/mtd/maps/physmap_of_core.c |  5 ++
> > > >  drivers/mtd/maps/physmap_of_gpio.c | 21 +++++++
> > > >  6 files changed, 129 insertions(+), 37 deletions(-)
> > > >  create mode 100644 drivers/mtd/maps/gpio-addr-flash.h
> > > >  create mode 100644 drivers/mtd/maps/physmap_of_gpio.c
> > > >
> > > > diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> > > > index afb36bff13a7..427143d42168 100644
> > > > --- a/drivers/mtd/maps/Kconfig
> > > > +++ b/drivers/mtd/maps/Kconfig
> > > > @@ -94,6 +94,14 @@ config MTD_PHYSMAP_OF_GEMINI
> > > >         platforms, some detection and setting up parallel mode on the
> > > >         external interface.
> > > >
> > > > +config MTD_PHYSMAP_OF_GPIO
> > > > +     bool "GPIO-assisted OF-based physical memory map handling"
> > > > +     depends on MTD_PHYSMAP_OF
> > > > +     depends on MTD_GPIO_ADDR
> > > > +     help
> > > > +       This provides some extra DT physmap parsing for flashes that are
> > > > +       partially physically addressed and assisted by GPIOs.
> > > > +
> > >
> > > Hm, so now we have the physmap_of driver which uses a function exposed
> > > by the gpio-addr-flash module, but this module is also declaring a
> > > platform_driver. It's probably working fine, but it's hard to follow.
> > >
> > > So, I decided to give it a try and started to rework a bit the physmap,
> > > physmap_of and gpio-addr-flash drivers. Here is the result [1] (it's
> > > only been compile tested). With this rework we now have a single
> > > driver which supports DT and !DT init and can also use GPIOs to
> > > extend the physical memory range in case it's not large enough to
> > > address the whole memory dev.
> > >
> > > Let me know what you think of this approach.
> > >
> >
> > The code is definitely easier to follow. But I believe you should
> > rebase your changes on mines (1-9) and instead of 10 apply your
> > patch-set.:
> > - "mtd: maps: Merge gpio-addr-flash.c into physmap-core.c" includes
> > all my changes and some of them are not obvious:( Port to gpiod,
> > gpio_values, win_order, use new apis....)
>
> True.
>
> > - Also there would be 1 (or 2) fixes that should be included in stable
> > that we lose using your approach "mtd: maps: gpio-addr-flash: Fix
> > ioremapped size", "mtd: physmap_of: Release resources on error"
>
> I'm not sure this is really useful to backport them to stable since
> blackfin users never complained about it (plus, blackin arch is now
> gone), but okay.
>
> > - And last, but not least, I want some credit for all this work ;)
> > After 10 iterations I guess that I deserve more than a Documentation
> > change, specially when my code would be on the tree.
>
> Fair enough.
>
> >
> > My other concern is that maybe we are giving too much entity to
> > gpio-addr by including it in the core. But if you are fine with it, I
> > am fine with it.
>
> Well, the gpio-addr stuff is just here to support platforms that do not
> have enough ADDR lines (or iomem address space) to address the whole
> device. I see it as an 'extension' of the physmap logic, which is why I
> think it makes sense to have this logic in the physmap driver instead
> of creating a completely new driver.
>
> >
> > If you want i can do the rebase and start testing on my board. What do
> > you think?
>
> That'd be great!

Can you start by picking 1-8 from my patchset so i do not have to
resend it again and again while we work on your changes?


Thanks!
>
> Thanks,
>
> Boris



-- 
Ricardo Ribalda

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

On Fri, 5 Oct 2018 10:10:22 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> On Fri, Oct 5, 2018 at 9:08 AM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > Hi Ricardo,
> >
> > On Fri, 5 Oct 2018 08:31:35 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> >  
> > > Hi Boris
> > >
> > >
> > > On Fri, Oct 5, 2018 at 12:21 AM Boris Brezillon
> > > <boris.brezillon@bootlin.com> wrote:  
> > > >
> > > > Hi Ricardo,
> > > >
> > > > On Thu,  4 Oct 2018 16:29:42 +0200
> > > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > > >  
> > > > > Allow creating gpio-addr-flash via device-tree and not just via platform
> > > > > data.
> > > > >
> > > > > Mimic what physmap_of_versatile and physmap_of_gemini does to reduce
> > > > > code duplicity.
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > > > > ---
> > > > >  drivers/mtd/maps/Kconfig           |  8 +++
> > > > >  drivers/mtd/maps/Makefile          |  3 +-
> > > > >  drivers/mtd/maps/gpio-addr-flash.c | 95 +++++++++++++++++++-----------
> > > > >  drivers/mtd/maps/gpio-addr-flash.h | 34 +++++++++++
> > > > >  drivers/mtd/maps/physmap_of_core.c |  5 ++
> > > > >  drivers/mtd/maps/physmap_of_gpio.c | 21 +++++++
> > > > >  6 files changed, 129 insertions(+), 37 deletions(-)
> > > > >  create mode 100644 drivers/mtd/maps/gpio-addr-flash.h
> > > > >  create mode 100644 drivers/mtd/maps/physmap_of_gpio.c
> > > > >
> > > > > diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> > > > > index afb36bff13a7..427143d42168 100644
> > > > > --- a/drivers/mtd/maps/Kconfig
> > > > > +++ b/drivers/mtd/maps/Kconfig
> > > > > @@ -94,6 +94,14 @@ config MTD_PHYSMAP_OF_GEMINI
> > > > >         platforms, some detection and setting up parallel mode on the
> > > > >         external interface.
> > > > >
> > > > > +config MTD_PHYSMAP_OF_GPIO
> > > > > +     bool "GPIO-assisted OF-based physical memory map handling"
> > > > > +     depends on MTD_PHYSMAP_OF
> > > > > +     depends on MTD_GPIO_ADDR
> > > > > +     help
> > > > > +       This provides some extra DT physmap parsing for flashes that are
> > > > > +       partially physically addressed and assisted by GPIOs.
> > > > > +  
> > > >
> > > > Hm, so now we have the physmap_of driver which uses a function exposed
> > > > by the gpio-addr-flash module, but this module is also declaring a
> > > > platform_driver. It's probably working fine, but it's hard to follow.
> > > >
> > > > So, I decided to give it a try and started to rework a bit the physmap,
> > > > physmap_of and gpio-addr-flash drivers. Here is the result [1] (it's
> > > > only been compile tested). With this rework we now have a single
> > > > driver which supports DT and !DT init and can also use GPIOs to
> > > > extend the physical memory range in case it's not large enough to
> > > > address the whole memory dev.
> > > >
> > > > Let me know what you think of this approach.
> > > >  
> > >
> > > The code is definitely easier to follow. But I believe you should
> > > rebase your changes on mines (1-9) and instead of 10 apply your
> > > patch-set.:
> > > - "mtd: maps: Merge gpio-addr-flash.c into physmap-core.c" includes
> > > all my changes and some of them are not obvious:( Port to gpiod,
> > > gpio_values, win_order, use new apis....)  
> >
> > True.
> >  
> > > - Also there would be 1 (or 2) fixes that should be included in stable
> > > that we lose using your approach "mtd: maps: gpio-addr-flash: Fix
> > > ioremapped size", "mtd: physmap_of: Release resources on error"  
> >
> > I'm not sure this is really useful to backport them to stable since
> > blackfin users never complained about it (plus, blackin arch is now
> > gone), but okay.
> >  
> > > - And last, but not least, I want some credit for all this work ;)
> > > After 10 iterations I guess that I deserve more than a Documentation
> > > change, specially when my code would be on the tree.  
> >
> > Fair enough.
> >  
> > >
> > > My other concern is that maybe we are giving too much entity to
> > > gpio-addr by including it in the core. But if you are fine with it, I
> > > am fine with it.  
> >
> > Well, the gpio-addr stuff is just here to support platforms that do not
> > have enough ADDR lines (or iomem address space) to address the whole
> > device. I see it as an 'extension' of the physmap logic, which is why I
> > think it makes sense to have this logic in the physmap driver instead
> > of creating a completely new driver.
> >  
> > >
> > > If you want i can do the rebase and start testing on my board. What do
> > > you think?  
> >
> > That'd be great!  
> 
> Can you start by picking 1-8 from my patchset so i do not have to
> resend it again and again while we work on your changes?

Done.

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

Hi Boris

Just seen that  you already did the rebase at
https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup

Thanks for that.

I am about to test it in real hw (unless you want me wait)

Cheers!


On Fri, Oct 5, 2018 at 10:37 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Fri, 5 Oct 2018 10:10:22 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > On Fri, Oct 5, 2018 at 9:08 AM Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Fri, 5 Oct 2018 08:31:35 +0200
> > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > >
> > > > Hi Boris
> > > >
> > > >
> > > > On Fri, Oct 5, 2018 at 12:21 AM Boris Brezillon
> > > > <boris.brezillon@bootlin.com> wrote:
> > > > >
> > > > > Hi Ricardo,
> > > > >
> > > > > On Thu,  4 Oct 2018 16:29:42 +0200
> > > > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > > > >
> > > > > > Allow creating gpio-addr-flash via device-tree and not just via platform
> > > > > > data.
> > > > > >
> > > > > > Mimic what physmap_of_versatile and physmap_of_gemini does to reduce
> > > > > > code duplicity.
> > > > > >
> > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > > > > > ---
> > > > > >  drivers/mtd/maps/Kconfig           |  8 +++
> > > > > >  drivers/mtd/maps/Makefile          |  3 +-
> > > > > >  drivers/mtd/maps/gpio-addr-flash.c | 95 +++++++++++++++++++-----------
> > > > > >  drivers/mtd/maps/gpio-addr-flash.h | 34 +++++++++++
> > > > > >  drivers/mtd/maps/physmap_of_core.c |  5 ++
> > > > > >  drivers/mtd/maps/physmap_of_gpio.c | 21 +++++++
> > > > > >  6 files changed, 129 insertions(+), 37 deletions(-)
> > > > > >  create mode 100644 drivers/mtd/maps/gpio-addr-flash.h
> > > > > >  create mode 100644 drivers/mtd/maps/physmap_of_gpio.c
> > > > > >
> > > > > > diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> > > > > > index afb36bff13a7..427143d42168 100644
> > > > > > --- a/drivers/mtd/maps/Kconfig
> > > > > > +++ b/drivers/mtd/maps/Kconfig
> > > > > > @@ -94,6 +94,14 @@ config MTD_PHYSMAP_OF_GEMINI
> > > > > >         platforms, some detection and setting up parallel mode on the
> > > > > >         external interface.
> > > > > >
> > > > > > +config MTD_PHYSMAP_OF_GPIO
> > > > > > +     bool "GPIO-assisted OF-based physical memory map handling"
> > > > > > +     depends on MTD_PHYSMAP_OF
> > > > > > +     depends on MTD_GPIO_ADDR
> > > > > > +     help
> > > > > > +       This provides some extra DT physmap parsing for flashes that are
> > > > > > +       partially physically addressed and assisted by GPIOs.
> > > > > > +
> > > > >
> > > > > Hm, so now we have the physmap_of driver which uses a function exposed
> > > > > by the gpio-addr-flash module, but this module is also declaring a
> > > > > platform_driver. It's probably working fine, but it's hard to follow.
> > > > >
> > > > > So, I decided to give it a try and started to rework a bit the physmap,
> > > > > physmap_of and gpio-addr-flash drivers. Here is the result [1] (it's
> > > > > only been compile tested). With this rework we now have a single
> > > > > driver which supports DT and !DT init and can also use GPIOs to
> > > > > extend the physical memory range in case it's not large enough to
> > > > > address the whole memory dev.
> > > > >
> > > > > Let me know what you think of this approach.
> > > > >
> > > >
> > > > The code is definitely easier to follow. But I believe you should
> > > > rebase your changes on mines (1-9) and instead of 10 apply your
> > > > patch-set.:
> > > > - "mtd: maps: Merge gpio-addr-flash.c into physmap-core.c" includes
> > > > all my changes and some of them are not obvious:( Port to gpiod,
> > > > gpio_values, win_order, use new apis....)
> > >
> > > True.
> > >
> > > > - Also there would be 1 (or 2) fixes that should be included in stable
> > > > that we lose using your approach "mtd: maps: gpio-addr-flash: Fix
> > > > ioremapped size", "mtd: physmap_of: Release resources on error"
> > >
> > > I'm not sure this is really useful to backport them to stable since
> > > blackfin users never complained about it (plus, blackin arch is now
> > > gone), but okay.
> > >
> > > > - And last, but not least, I want some credit for all this work ;)
> > > > After 10 iterations I guess that I deserve more than a Documentation
> > > > change, specially when my code would be on the tree.
> > >
> > > Fair enough.
> > >
> > > >
> > > > My other concern is that maybe we are giving too much entity to
> > > > gpio-addr by including it in the core. But if you are fine with it, I
> > > > am fine with it.
> > >
> > > Well, the gpio-addr stuff is just here to support platforms that do not
> > > have enough ADDR lines (or iomem address space) to address the whole
> > > device. I see it as an 'extension' of the physmap logic, which is why I
> > > think it makes sense to have this logic in the physmap driver instead
> > > of creating a completely new driver.
> > >
> > > >
> > > > If you want i can do the rebase and start testing on my board. What do
> > > > you think?
> > >
> > > That'd be great!
> >
> > Can you start by picking 1-8 from my patchset so i do not have to
> > resend it again and again while we work on your changes?
>
> Done.



-- 
Ricardo Ribalda

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

On Fri, 5 Oct 2018 11:54:18 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> 
> Just seen that  you already did the rebase at
> https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> 
> Thanks for that.
> 
> I am about to test it in real hw (unless you want me wait)

Sure, go ahead and test it.

Thanks,

Boris

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

Hi Boris
On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Fri, 5 Oct 2018 11:54:18 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > Hi Boris
> >
> > Just seen that  you already did the rebase at
> > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> >
> > Thanks for that.
> >
> > I am about to test it in real hw (unless you want me wait)
>
> Sure, go ahead and test it.
>
> Thanks,
>
> Boris
I had to change this on your patchset to have it working on hw:
https://pastebin.com/78A7yhJ9

If you send the patchset to the mailing list I can review it patch by patch.

Also
mtd: maps: Prepare merging of physmap and physmap_of

I do not think that can be bisected. (Not sure, I have to test it)

I add the diff to the mail, but gmail will probably scramble the
lines(yes I know I have to use other mail client)

Thanks!



diff --git a/drivers/mtd/maps/physmap-core.c b/drivers/mtd/maps/physmap-core.c
index 2e236ef60e04..d7a902afc9a7 100644
--- a/drivers/mtd/maps/physmap-core.c
+++ b/drivers/mtd/maps/physmap-core.c
@@ -69,8 +69,6 @@ static int physmap_flash_remove(struct platform_device *dev)
  if (!info)
  return 0;

- physmap_data = dev_get_platdata(&dev->dev);
-
  if (info->cmtd) {
  err = mtd_device_unregister(info->cmtd);
  if (err)
@@ -80,12 +78,12 @@ static int physmap_flash_remove(struct platform_device *dev)
  mtd_concat_destroy(info->cmtd);
  }

- for (i = 0; i < info->nmaps; i++) {
- if (!info->mtds[i])
+ for (i = 0; i < info->nmaps; i++)
+ if (info->mtds[i])
  map_destroy(info->mtds[i]);
- }

- if (physmap_data->exit)
+ physmap_data = dev_get_platdata(&dev->dev);
+ if (physmap_data && physmap_data->exit)
  physmap_data->exit(dev);

  return 0;
@@ -456,18 +454,18 @@ static int physmap_flash_probe(struct
platform_device *dev)
  info->maps = devm_kzalloc(&dev->dev,
    sizeof(*info->maps) * info->nmaps,
    GFP_KERNEL);
- if (info->maps)
+ if (!info->maps)
  return -ENOMEM;

  info->mtds = devm_kzalloc(&dev->dev,
    sizeof(*info->mtds) * info->nmaps,
    GFP_KERNEL);
- if (info->mtds)
+ if (!info->mtds)
  return -ENOMEM;

  platform_set_drvdata(dev, info);

- info->gpios = devm_gpiod_get_array_optional(&dev->dev, "addr-gpios",
+ info->gpios = devm_gpiod_get_array_optional(&dev->dev, "addr",
      GPIOD_OUT_LOW);
  if (IS_ERR(info->gpios))
  return PTR_ERR(info->gpios);
@@ -480,14 +478,6 @@ static int physmap_flash_probe(struct platform_device *dev)
  err = physmap_flash_of_init(dev);
  if (err)
  err = physmap_flash_pdata_init(dev);
-
- if (err)
- return err;
-
- err = physmap_flash_of_init(dev);
- if (err)
- err = physmap_flash_pdata_init(dev);
-
  if (err)
  return err;

@@ -509,7 +499,6 @@ static int physmap_flash_probe(struct platform_device *dev)
  if (!info->maps[i].phys)
  info->maps[i].phys = res->start;

- info->maps[i].size = resource_size(res);
  info->win_order = get_bitmask_order(resource_size(res)) - 1;
  info->maps[i].size = BIT(info->win_order +
  (info->gpios ?





-- 
Ricardo Ribalda

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

On Fri, 5 Oct 2018 14:04:52 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > On Fri, 5 Oct 2018 11:54:18 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> >  
> > > Hi Boris
> > >
> > > Just seen that  you already did the rebase at
> > > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> > >
> > > Thanks for that.
> > >
> > > I am about to test it in real hw (unless you want me wait)  
> >
> > Sure, go ahead and test it.
> >
> > Thanks,
> >
> > Boris  
> I had to change this on your patchset to have it working on hw:
> https://pastebin.com/78A7yhJ9
> 
> If you send the patchset to the mailing list I can review it patch by patch.
> 
> Also
> mtd: maps: Prepare merging of physmap and physmap_of
> 
> I do not think that can be bisected. (Not sure, I have to test it)

Okay, I'll have a look.

> 
> I add the diff to the mail, but gmail will probably scramble the
> lines(yes I know I have to use other mail client)

The diff looks good, I'll fix that an send a push a new version.

Thanks,

Boris

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

Hi again Boris


On Fri, Oct 5, 2018 at 2:10 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Fri, 5 Oct 2018 14:04:52 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > Hi Boris
> > On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:
> > >
> > > On Fri, 5 Oct 2018 11:54:18 +0200
> > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > >
> > > > Hi Boris
> > > >
> > > > Just seen that  you already did the rebase at
> > > > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> > > >
> > > > Thanks for that.
> > > >
> > > > I am about to test it in real hw (unless you want me wait)
> > >
> > > Sure, go ahead and test it.
> > >
> > > Thanks,
> > >
> > > Boris
> > I had to change this on your patchset to have it working on hw:
> > https://pastebin.com/78A7yhJ9
> >
> > If you send the patchset to the mailing list I can review it patch by patch.
> >
> > Also
> > mtd: maps: Prepare merging of physmap and physmap_of
> >
> > I do not think that can be bisected. (Not sure, I have to test it)
>
> Okay, I'll have a look.
>
> >
> > I add the diff to the mail, but gmail will probably scramble the
> > lines(yes I know I have to use other mail client)
>
> The diff looks good, I'll fix that an send a push a new version.

Also fix on physmap_flash_remove

physmap_data->exit(dev); must be called BEFORE
map_destroy(info->mtds[i]);

otherwise OOPS

Cheers!

[  162.700421] Unable to handle kernel paging request at virtual
address 7974696e6966a6
[  162.705461] Mem abort info:
[  162.713171]   Exception class = DABT (current EL), IL = 32 bits
[  162.715678]   SET = 0, FnV = 0
[  162.721588]   EA = 0, S1PTW = 0
[  162.724703] Data abort info:
[  162.727767]   ISV = 0, ISS = 0x00000004
[  162.730879]   CM = 0, WnR = 0
[  162.734436] [007974696e6966a6] address between user and kernel address ranges
[  162.737567] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[  162.744674] Modules linked in: qtec_m43 qtec_mem qt5023_video
qtec_white qtec_cmosis arc4 snd_soc_hdmi_codec ath10k_pci ath10k_core
venus_dec venus_enc ath mac80211 q6asm_dai cfi_cmdset_0002 q6routing
q6afe_dai cfi_probe q6adm cfi_util q6asm gen_probe q6afe q6dsp_common
q6core physmap gpio_xilinx qtec_pcie qtec_clk apr qrtr_smd rpmsg_char
imx214 ad5820 v4l2_common joydev cfg80211 msm venus_core hci_uart
qt5023 qcom_camss btqca v4l2_mem2mem drm_kms_helper videobuf2_dma_sg
v4l2_fwnode videobuf2_memops videobuf2_v4l2 bluetooth drm
videobuf2_core videodev ecdh_generic i2c_qcom_cci media rtc_pm8xxx
leds_qcom_lpg qrtr crc32_ce qcom_adsp_pil crct10dif_ce qcom_common
snd_soc_apq8096 qcom_glink_smem qcom_sysmon remoteproc qmi_helpers
rmtfs_mem mdt_loader sch_fq_codel
[  162.795188] CPU: 2 PID: 1 Comm: systemd-shutdow Not tainted
4.14.53-qtec-linaro #34
[  162.817417] Hardware name: Qtechnology QT5506 (DT)
[  162.824793] task: ffff8000baac8000 task.stack: ffff000008058000
[  162.829671] PC is at cfi_amdstd_reset+0xb4/0x188 [cfi_cmdset_0002]
[  162.835474] LR is at cfi_amdstd_reset+0xb4/0x188 [cfi_cmdset_0002]
[  162.841721] pc : [<ffff00000107fe44>] lr : [<ffff00000107fe44>]
pstate: 40000145
[  162.847885] sp : ffff00000805bc80
[  162.855433] x29: ffff00000805bc80 x28: ffff8000baac8000
[  162.858648] x27: ffff000008b51000 x26: 000000000000008e
[  162.864030] x25: 0000000000000124 x24: 0000000000000014
[  162.869324] x23: 0000000000000000 x22: 0000000000000000
[  162.874619] x21: 5f7974696e696666 x20: ffff8000b3178a18
[  162.879915] x19: ffff8000b5215800 x18: 0000000000000000
[  162.885209] x17: 0000ffffa46d1768 x16: ffff0000080f9960
[  162.890504] x15: 0000000000000010 x14: ffffffffffffffff
[  162.895800] x13: ffff0000896355e7 x12: ffff0000096355ef
[  162.901094] x11: ffff00000944a000 x10: ffff00000805b9b0
[  162.906389] x9 : 0000000000000039 x8 : 000000000000000d
[  162.911686] x7 : ffff00000805ba2c x6 : 0000000000000491
[  162.916980] x5 : 0000000000000000 x4 : 0000000000000000
[  162.922275] x3 : ffffffffffffffff x2 : ffff00000944a278
[  162.927570] x1 : ffff8000baac8000 x0 : 0000000000000028
[  162.932867] Process systemd-shutdow (pid: 1, stack limit =
0xffff000008058000)
[  162.938164] Call trace:
[  162.945191] Exception stack(0xffff00000805bb40 to 0xffff00000805bc80)
[  162.947538] bb40: 0000000000000028 ffff8000baac8000
ffff00000944a278 ffffffffffffffff
[  162.954136] bb60: 0000000000000000 0000000000000000
0000000000000491 ffff00000805ba2c
[  162.961949] bb80: 000000000000000d 0000000000000039
ffff00000805b9b0 ffff00000944a000
[  162.969762] bba0: ffff0000096355ef ffff0000896355e7
ffffffffffffffff 0000000000000010
[  162.977574] bbc0: ffff0000080f9960 0000ffffa46d1768
0000000000000000 ffff8000b5215800
[  162.985386] bbe0: ffff8000b3178a18 5f7974696e696666
0000000000000000 0000000000000000
[  162.993200] bc00: 0000000000000014 0000000000000124
000000000000008e ffff000008b51000
[  163.001011] bc20: ffff8000baac8000 ffff00000805bc80
ffff00000107fe44 ffff00000805bc80
[  163.008823] bc40: ffff00000107fe44 0000000040000145
0000000000000b5b ffffffffffffffff
[  163.016637] bc60: 0000ffffffffffff 0000000000000000
ffff00000805bc80 ffff00000107fe44
[  163.024454] [<ffff00000107fe44>] cfi_amdstd_reset+0xb4/0x188
[cfi_cmdset_0002]
[  163.032264] [<ffff00000107ff78>] cfi_amdstd_reboot+0x10/0x98
[cfi_cmdset_0002]
[  163.039386] [<ffff0000080f7d0c>] notifier_call_chain+0x54/0x90
[  163.046583] [<ffff0000080f85d4>] blocking_notifier_call_chain+0x54/0x78
[  163.052399] [<ffff0000080f96ac>] kernel_restart_prepare+0x1c/0x40
[  163.058910] [<ffff0000080f97a4>] kernel_restart+0x14/0x60
[  163.065158] [<ffff0000080f9b68>] SyS_reboot+0x208/0x230


>
> Thanks,
>
> Boris



-- 
Ricardo Ribalda

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

On Fri, 5 Oct 2018 16:06:57 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi again Boris
> 
> 
> On Fri, Oct 5, 2018 at 2:10 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > On Fri, 5 Oct 2018 14:04:52 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> >  
> > > Hi Boris
> > > On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
> > > <boris.brezillon@bootlin.com> wrote:  
> > > >
> > > > On Fri, 5 Oct 2018 11:54:18 +0200
> > > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > > >  
> > > > > Hi Boris
> > > > >
> > > > > Just seen that  you already did the rebase at
> > > > > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> > > > >
> > > > > Thanks for that.
> > > > >
> > > > > I am about to test it in real hw (unless you want me wait)  
> > > >
> > > > Sure, go ahead and test it.
> > > >
> > > > Thanks,
> > > >
> > > > Boris  
> > > I had to change this on your patchset to have it working on hw:
> > > https://pastebin.com/78A7yhJ9
> > >
> > > If you send the patchset to the mailing list I can review it patch by patch.
> > >
> > > Also
> > > mtd: maps: Prepare merging of physmap and physmap_of
> > >
> > > I do not think that can be bisected. (Not sure, I have to test it)  
> >
> > Okay, I'll have a look.
> >  
> > >
> > > I add the diff to the mail, but gmail will probably scramble the
> > > lines(yes I know I have to use other mail client)  
> >
> > The diff looks good, I'll fix that an send a push a new version.  
> 
> Also fix on physmap_flash_remove
> 
> physmap_data->exit(dev); must be called BEFORE
> map_destroy(info->mtds[i]);

Hm, that's weird. That shouldn't happen. Do you have a non-NULL
->exit()? Can you detail why you think ->exit() call is the cause of
this OOPS?

> 
> otherwise OOPS
> 
> Cheers!
> 
> [  162.700421] Unable to handle kernel paging request at virtual
> address 7974696e6966a6
> [  162.705461] Mem abort info:
> [  162.713171]   Exception class = DABT (current EL), IL = 32 bits
> [  162.715678]   SET = 0, FnV = 0
> [  162.721588]   EA = 0, S1PTW = 0
> [  162.724703] Data abort info:
> [  162.727767]   ISV = 0, ISS = 0x00000004
> [  162.730879]   CM = 0, WnR = 0
> [  162.734436] [007974696e6966a6] address between user and kernel address ranges
> [  162.737567] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [  162.744674] Modules linked in: qtec_m43 qtec_mem qt5023_video
> qtec_white qtec_cmosis arc4 snd_soc_hdmi_codec ath10k_pci ath10k_core
> venus_dec venus_enc ath mac80211 q6asm_dai cfi_cmdset_0002 q6routing
> q6afe_dai cfi_probe q6adm cfi_util q6asm gen_probe q6afe q6dsp_common
> q6core physmap gpio_xilinx qtec_pcie qtec_clk apr qrtr_smd rpmsg_char
> imx214 ad5820 v4l2_common joydev cfg80211 msm venus_core hci_uart
> qt5023 qcom_camss btqca v4l2_mem2mem drm_kms_helper videobuf2_dma_sg
> v4l2_fwnode videobuf2_memops videobuf2_v4l2 bluetooth drm
> videobuf2_core videodev ecdh_generic i2c_qcom_cci media rtc_pm8xxx
> leds_qcom_lpg qrtr crc32_ce qcom_adsp_pil crct10dif_ce qcom_common
> snd_soc_apq8096 qcom_glink_smem qcom_sysmon remoteproc qmi_helpers
> rmtfs_mem mdt_loader sch_fq_codel
> [  162.795188] CPU: 2 PID: 1 Comm: systemd-shutdow Not tainted
> 4.14.53-qtec-linaro #34
> [  162.817417] Hardware name: Qtechnology QT5506 (DT)
> [  162.824793] task: ffff8000baac8000 task.stack: ffff000008058000
> [  162.829671] PC is at cfi_amdstd_reset+0xb4/0x188 [cfi_cmdset_0002]
> [  162.835474] LR is at cfi_amdstd_reset+0xb4/0x188 [cfi_cmdset_0002]
> [  162.841721] pc : [<ffff00000107fe44>] lr : [<ffff00000107fe44>]
> pstate: 40000145
> [  162.847885] sp : ffff00000805bc80
> [  162.855433] x29: ffff00000805bc80 x28: ffff8000baac8000
> [  162.858648] x27: ffff000008b51000 x26: 000000000000008e
> [  162.864030] x25: 0000000000000124 x24: 0000000000000014
> [  162.869324] x23: 0000000000000000 x22: 0000000000000000
> [  162.874619] x21: 5f7974696e696666 x20: ffff8000b3178a18
> [  162.879915] x19: ffff8000b5215800 x18: 0000000000000000
> [  162.885209] x17: 0000ffffa46d1768 x16: ffff0000080f9960
> [  162.890504] x15: 0000000000000010 x14: ffffffffffffffff
> [  162.895800] x13: ffff0000896355e7 x12: ffff0000096355ef
> [  162.901094] x11: ffff00000944a000 x10: ffff00000805b9b0
> [  162.906389] x9 : 0000000000000039 x8 : 000000000000000d
> [  162.911686] x7 : ffff00000805ba2c x6 : 0000000000000491
> [  162.916980] x5 : 0000000000000000 x4 : 0000000000000000
> [  162.922275] x3 : ffffffffffffffff x2 : ffff00000944a278
> [  162.927570] x1 : ffff8000baac8000 x0 : 0000000000000028
> [  162.932867] Process systemd-shutdow (pid: 1, stack limit =
> 0xffff000008058000)
> [  162.938164] Call trace:
> [  162.945191] Exception stack(0xffff00000805bb40 to 0xffff00000805bc80)
> [  162.947538] bb40: 0000000000000028 ffff8000baac8000
> ffff00000944a278 ffffffffffffffff
> [  162.954136] bb60: 0000000000000000 0000000000000000
> 0000000000000491 ffff00000805ba2c
> [  162.961949] bb80: 000000000000000d 0000000000000039
> ffff00000805b9b0 ffff00000944a000
> [  162.969762] bba0: ffff0000096355ef ffff0000896355e7
> ffffffffffffffff 0000000000000010
> [  162.977574] bbc0: ffff0000080f9960 0000ffffa46d1768
> 0000000000000000 ffff8000b5215800
> [  162.985386] bbe0: ffff8000b3178a18 5f7974696e696666
> 0000000000000000 0000000000000000
> [  162.993200] bc00: 0000000000000014 0000000000000124
> 000000000000008e ffff000008b51000
> [  163.001011] bc20: ffff8000baac8000 ffff00000805bc80
> ffff00000107fe44 ffff00000805bc80
> [  163.008823] bc40: ffff00000107fe44 0000000040000145
> 0000000000000b5b ffffffffffffffff
> [  163.016637] bc60: 0000ffffffffffff 0000000000000000
> ffff00000805bc80 ffff00000107fe44
> [  163.024454] [<ffff00000107fe44>] cfi_amdstd_reset+0xb4/0x188
> [cfi_cmdset_0002]
> [  163.032264] [<ffff00000107ff78>] cfi_amdstd_reboot+0x10/0x98
> [cfi_cmdset_0002]
> [  163.039386] [<ffff0000080f7d0c>] notifier_call_chain+0x54/0x90
> [  163.046583] [<ffff0000080f85d4>] blocking_notifier_call_chain+0x54/0x78
> [  163.052399] [<ffff0000080f96ac>] kernel_restart_prepare+0x1c/0x40
> [  163.058910] [<ffff0000080f97a4>] kernel_restart+0x14/0x60
> [  163.065158] [<ffff0000080f9b68>] SyS_reboot+0x208/0x230

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

Hi Boris
On Fri, Oct 5, 2018 at 4:52 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Fri, 5 Oct 2018 16:06:57 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > Hi again Boris
> >
> >
> > On Fri, Oct 5, 2018 at 2:10 PM Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:
> > >
> > > On Fri, 5 Oct 2018 14:04:52 +0200
> > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > >
> > > > Hi Boris
> > > > On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
> > > > <boris.brezillon@bootlin.com> wrote:
> > > > >
> > > > > On Fri, 5 Oct 2018 11:54:18 +0200
> > > > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > > > >
> > > > > > Hi Boris
> > > > > >
> > > > > > Just seen that  you already did the rebase at
> > > > > > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> > > > > >
> > > > > > Thanks for that.
> > > > > >
> > > > > > I am about to test it in real hw (unless you want me wait)
> > > > >
> > > > > Sure, go ahead and test it.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Boris
> > > > I had to change this on your patchset to have it working on hw:
> > > > https://pastebin.com/78A7yhJ9
> > > >
> > > > If you send the patchset to the mailing list I can review it patch by patch.
> > > >
> > > > Also
> > > > mtd: maps: Prepare merging of physmap and physmap_of
> > > >
> > > > I do not think that can be bisected. (Not sure, I have to test it)
> > >
> > > Okay, I'll have a look.
> > >
> > > >
> > > > I add the diff to the mail, but gmail will probably scramble the
> > > > lines(yes I know I have to use other mail client)
> > >
> > > The diff looks good, I'll fix that an send a push a new version.
> >
> > Also fix on physmap_flash_remove
> >
> > physmap_data->exit(dev); must be called BEFORE
> > map_destroy(info->mtds[i]);
>
> Hm, that's weird. That shouldn't happen. Do you have a non-NULL
> ->exit()? Can you detail why you think ->exit() call is the cause of
> this OOPS?
>

No idea.  It was crashing at:
https://github.com/bbrezillon/linux-0day/blob/mtd/physmap-cleanup/drivers/mtd/chips/cfi_cmdset_0002.c#L2839
cfi_cmdset_0002.c seesm to play with cfi->chips on its reset callback

I added some printfs:

if (!cfi),
if (!chip)
if (!cfi->chips)

sometimes it crashed on one place, sometimes in another :S. Reading
back our patch it seemed more logical (semantically :P) to
destroy after exit and not the other way around.

I made that change and it stopped OOPsing at reboot.

Havent had the time to dig deeper. But if it does not break anything
on your side to invert destroy and exit please do so. It was not
oopsing with my patchset.

Cheers
> >
> > otherwise OOPS
> >
> > Cheers!
> >
> > [  162.700421] Unable to handle kernel paging request at virtual
> > address 7974696e6966a6
> > [  162.705461] Mem abort info:
> > [  162.713171]   Exception class = DABT (current EL), IL = 32 bits
> > [  162.715678]   SET = 0, FnV = 0
> > [  162.721588]   EA = 0, S1PTW = 0
> > [  162.724703] Data abort info:
> > [  162.727767]   ISV = 0, ISS = 0x00000004
> > [  162.730879]   CM = 0, WnR = 0
> > [  162.734436] [007974696e6966a6] address between user and kernel address ranges
> > [  162.737567] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> > [  162.744674] Modules linked in: qtec_m43 qtec_mem qt5023_video
> > qtec_white qtec_cmosis arc4 snd_soc_hdmi_codec ath10k_pci ath10k_core
> > venus_dec venus_enc ath mac80211 q6asm_dai cfi_cmdset_0002 q6routing
> > q6afe_dai cfi_probe q6adm cfi_util q6asm gen_probe q6afe q6dsp_common
> > q6core physmap gpio_xilinx qtec_pcie qtec_clk apr qrtr_smd rpmsg_char
> > imx214 ad5820 v4l2_common joydev cfg80211 msm venus_core hci_uart
> > qt5023 qcom_camss btqca v4l2_mem2mem drm_kms_helper videobuf2_dma_sg
> > v4l2_fwnode videobuf2_memops videobuf2_v4l2 bluetooth drm
> > videobuf2_core videodev ecdh_generic i2c_qcom_cci media rtc_pm8xxx
> > leds_qcom_lpg qrtr crc32_ce qcom_adsp_pil crct10dif_ce qcom_common
> > snd_soc_apq8096 qcom_glink_smem qcom_sysmon remoteproc qmi_helpers
> > rmtfs_mem mdt_loader sch_fq_codel
> > [  162.795188] CPU: 2 PID: 1 Comm: systemd-shutdow Not tainted
> > 4.14.53-qtec-linaro #34
> > [  162.817417] Hardware name: Qtechnology QT5506 (DT)
> > [  162.824793] task: ffff8000baac8000 task.stack: ffff000008058000
> > [  162.829671] PC is at cfi_amdstd_reset+0xb4/0x188 [cfi_cmdset_0002]
> > [  162.835474] LR is at cfi_amdstd_reset+0xb4/0x188 [cfi_cmdset_0002]
> > [  162.841721] pc : [<ffff00000107fe44>] lr : [<ffff00000107fe44>]
> > pstate: 40000145
> > [  162.847885] sp : ffff00000805bc80
> > [  162.855433] x29: ffff00000805bc80 x28: ffff8000baac8000
> > [  162.858648] x27: ffff000008b51000 x26: 000000000000008e
> > [  162.864030] x25: 0000000000000124 x24: 0000000000000014
> > [  162.869324] x23: 0000000000000000 x22: 0000000000000000
> > [  162.874619] x21: 5f7974696e696666 x20: ffff8000b3178a18
> > [  162.879915] x19: ffff8000b5215800 x18: 0000000000000000
> > [  162.885209] x17: 0000ffffa46d1768 x16: ffff0000080f9960
> > [  162.890504] x15: 0000000000000010 x14: ffffffffffffffff
> > [  162.895800] x13: ffff0000896355e7 x12: ffff0000096355ef
> > [  162.901094] x11: ffff00000944a000 x10: ffff00000805b9b0
> > [  162.906389] x9 : 0000000000000039 x8 : 000000000000000d
> > [  162.911686] x7 : ffff00000805ba2c x6 : 0000000000000491
> > [  162.916980] x5 : 0000000000000000 x4 : 0000000000000000
> > [  162.922275] x3 : ffffffffffffffff x2 : ffff00000944a278
> > [  162.927570] x1 : ffff8000baac8000 x0 : 0000000000000028
> > [  162.932867] Process systemd-shutdow (pid: 1, stack limit =
> > 0xffff000008058000)
> > [  162.938164] Call trace:
> > [  162.945191] Exception stack(0xffff00000805bb40 to 0xffff00000805bc80)
> > [  162.947538] bb40: 0000000000000028 ffff8000baac8000
> > ffff00000944a278 ffffffffffffffff
> > [  162.954136] bb60: 0000000000000000 0000000000000000
> > 0000000000000491 ffff00000805ba2c
> > [  162.961949] bb80: 000000000000000d 0000000000000039
> > ffff00000805b9b0 ffff00000944a000
> > [  162.969762] bba0: ffff0000096355ef ffff0000896355e7
> > ffffffffffffffff 0000000000000010
> > [  162.977574] bbc0: ffff0000080f9960 0000ffffa46d1768
> > 0000000000000000 ffff8000b5215800
> > [  162.985386] bbe0: ffff8000b3178a18 5f7974696e696666
> > 0000000000000000 0000000000000000
> > [  162.993200] bc00: 0000000000000014 0000000000000124
> > 000000000000008e ffff000008b51000
> > [  163.001011] bc20: ffff8000baac8000 ffff00000805bc80
> > ffff00000107fe44 ffff00000805bc80
> > [  163.008823] bc40: ffff00000107fe44 0000000040000145
> > 0000000000000b5b ffffffffffffffff
> > [  163.016637] bc60: 0000ffffffffffff 0000000000000000
> > ffff00000805bc80 ffff00000107fe44
> > [  163.024454] [<ffff00000107fe44>] cfi_amdstd_reset+0xb4/0x188
> > [cfi_cmdset_0002]
> > [  163.032264] [<ffff00000107ff78>] cfi_amdstd_reboot+0x10/0x98
> > [cfi_cmdset_0002]
> > [  163.039386] [<ffff0000080f7d0c>] notifier_call_chain+0x54/0x90
> > [  163.046583] [<ffff0000080f85d4>] blocking_notifier_call_chain+0x54/0x78
> > [  163.052399] [<ffff0000080f96ac>] kernel_restart_prepare+0x1c/0x40
> > [  163.058910] [<ffff0000080f97a4>] kernel_restart+0x14/0x60
> > [  163.065158] [<ffff0000080f9b68>] SyS_reboot+0x208/0x230



-- 
Ricardo Ribalda

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

On Fri, 5 Oct 2018 17:40:44 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> On Fri, Oct 5, 2018 at 4:52 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > On Fri, 5 Oct 2018 16:06:57 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> >  
> > > Hi again Boris
> > >
> > >
> > > On Fri, Oct 5, 2018 at 2:10 PM Boris Brezillon
> > > <boris.brezillon@bootlin.com> wrote:  
> > > >
> > > > On Fri, 5 Oct 2018 14:04:52 +0200
> > > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > > >  
> > > > > Hi Boris
> > > > > On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
> > > > > <boris.brezillon@bootlin.com> wrote:  
> > > > > >
> > > > > > On Fri, 5 Oct 2018 11:54:18 +0200
> > > > > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > > > > >  
> > > > > > > Hi Boris
> > > > > > >
> > > > > > > Just seen that  you already did the rebase at
> > > > > > > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> > > > > > >
> > > > > > > Thanks for that.
> > > > > > >
> > > > > > > I am about to test it in real hw (unless you want me wait)  
> > > > > >
> > > > > > Sure, go ahead and test it.
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Boris  
> > > > > I had to change this on your patchset to have it working on hw:
> > > > > https://pastebin.com/78A7yhJ9
> > > > >
> > > > > If you send the patchset to the mailing list I can review it patch by patch.
> > > > >
> > > > > Also
> > > > > mtd: maps: Prepare merging of physmap and physmap_of
> > > > >
> > > > > I do not think that can be bisected. (Not sure, I have to test it)  
> > > >
> > > > Okay, I'll have a look.
> > > >  
> > > > >
> > > > > I add the diff to the mail, but gmail will probably scramble the
> > > > > lines(yes I know I have to use other mail client)  
> > > >
> > > > The diff looks good, I'll fix that an send a push a new version.  
> > >
> > > Also fix on physmap_flash_remove
> > >
> > > physmap_data->exit(dev); must be called BEFORE
> > > map_destroy(info->mtds[i]);  
> >
> > Hm, that's weird. That shouldn't happen. Do you have a non-NULL  
> > ->exit()? Can you detail why you think ->exit() call is the cause of  
> > this OOPS?
> >  
> 
> No idea.  It was crashing at:
> https://github.com/bbrezillon/linux-0day/blob/mtd/physmap-cleanup/drivers/mtd/chips/cfi_cmdset_0002.c#L2839
> cfi_cmdset_0002.c seesm to play with cfi->chips on its reset callback
> 
> I added some printfs:
> 
> if (!cfi),
> if (!chip)
> if (!cfi->chips)
> 
> sometimes it crashed on one place, sometimes in another :S. Reading
> back our patch it seemed more logical (semantically :P) to
> destroy after exit and not the other way around.

Actually no, it makes more sense to call ->exit() after destroying the
maps, because the platform-specific ->exit() implem might release
resources that are used during the destroy_map() operation. Another
reason to keep it in this order is that operations in the remove path
should be in the reverse order of those done in the probe path, and
->init() is definitely called before map_probe().

> 
> I made that change and it stopped OOPsing at reboot.

Maybe it's just a side effect, especially since I'm pretty sure your
physmap_data->exit() is NULL (assuming you do use DT to declare your
flash).

> 
> Havent had the time to dig deeper. But if it does not break anything
> on your side to invert destroy and exit please do so. It was not
> oopsing with my patchset.

I'd like to understand what's happening before doing this change. As I
said, I fear this reordering just hides something else.

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

Hi Boris
On Fri, Oct 5, 2018 at 6:29 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Fri, 5 Oct 2018 17:40:44 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > Hi Boris
> > On Fri, Oct 5, 2018 at 4:52 PM Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:
> > >
> > > On Fri, 5 Oct 2018 16:06:57 +0200
> > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > >
> > > > Hi again Boris
> > > >
> > > >
> > > > On Fri, Oct 5, 2018 at 2:10 PM Boris Brezillon
> > > > <boris.brezillon@bootlin.com> wrote:
> > > > >
> > > > > On Fri, 5 Oct 2018 14:04:52 +0200
> > > > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > > > >
> > > > > > Hi Boris
> > > > > > On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
> > > > > > <boris.brezillon@bootlin.com> wrote:
> > > > > > >
> > > > > > > On Fri, 5 Oct 2018 11:54:18 +0200
> > > > > > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > > > > > >
> > > > > > > > Hi Boris
> > > > > > > >
> > > > > > > > Just seen that  you already did the rebase at
> > > > > > > > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> > > > > > > >
> > > > > > > > Thanks for that.
> > > > > > > >
> > > > > > > > I am about to test it in real hw (unless you want me wait)
> > > > > > >
> > > > > > > Sure, go ahead and test it.
> > > > > > >
> > > > > > > Thanks,
> > > > > > >
> > > > > > > Boris
> > > > > > I had to change this on your patchset to have it working on hw:
> > > > > > https://pastebin.com/78A7yhJ9
> > > > > >
> > > > > > If you send the patchset to the mailing list I can review it patch by patch.
> > > > > >
> > > > > > Also
> > > > > > mtd: maps: Prepare merging of physmap and physmap_of
> > > > > >
> > > > > > I do not think that can be bisected. (Not sure, I have to test it)
> > > > >
> > > > > Okay, I'll have a look.
> > > > >
> > > > > >
> > > > > > I add the diff to the mail, but gmail will probably scramble the
> > > > > > lines(yes I know I have to use other mail client)
> > > > >
> > > > > The diff looks good, I'll fix that an send a push a new version.
> > > >
> > > > Also fix on physmap_flash_remove
> > > >
> > > > physmap_data->exit(dev); must be called BEFORE
> > > > map_destroy(info->mtds[i]);
> > >
> > > Hm, that's weird. That shouldn't happen. Do you have a non-NULL
> > > ->exit()? Can you detail why you think ->exit() call is the cause of
> > > this OOPS?
> > >
> >
> > No idea.  It was crashing at:
> > https://github.com/bbrezillon/linux-0day/blob/mtd/physmap-cleanup/drivers/mtd/chips/cfi_cmdset_0002.c#L2839
> > cfi_cmdset_0002.c seesm to play with cfi->chips on its reset callback
> >
> > I added some printfs:
> >
> > if (!cfi),
> > if (!chip)
> > if (!cfi->chips)
> >
> > sometimes it crashed on one place, sometimes in another :S. Reading
> > back our patch it seemed more logical (semantically :P) to
> > destroy after exit and not the other way around.
>
> Actually no, it makes more sense to call ->exit() after destroying the
> maps, because the platform-specific ->exit() implem might release
> resources that are used during the destroy_map() operation. Another
> reason to keep it in this order is that operations in the remove path
> should be in the reverse order of those done in the probe path, and
> ->init() is definitely called before map_probe().
>

I think I know what might be the issue. on cfi_cmdset_002.c
cfi_amdstd_reset can be called in parrallel to cfi_amdstd_destroy.

maybe we should call
unregister_reboot_notifier(&mtd->reboot_notifier);
before
cfi_amdstd_reset(mtd)

need to try that on real hw with some strategical printfs :P. But that
will ahve to wait til Monday

Cheers


> >
> > I made that change and it stopped OOPsing at reboot.
>
> Maybe it's just a side effect, especially since I'm pretty sure your
> physmap_data->exit() is NULL (assuming you do use DT to declare your
> flash).
>
> >
> > Havent had the time to dig deeper. But if it does not break anything
> > on your side to invert destroy and exit please do so. It was not
> > oopsing with my patchset.
>
> I'd like to understand what's happening before doing this change. As I
> said, I fear this reordering just hides something else.



-- 
Ricardo Ribalda

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

On Fri, 5 Oct 2018 20:12:43 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> 
> I think I know what might be the issue. on cfi_cmdset_002.c
> cfi_amdstd_reset can be called in parrallel to cfi_amdstd_destroy.
> 
> maybe we should call
> unregister_reboot_notifier(&mtd->reboot_notifier);
> before
> cfi_amdstd_reset(mtd)
> 
> need to try that on real hw with some strategical printfs :P. But that
> will ahve to wait til Monday

No problem, nothing urgent here. Have a nice weekend.

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

Hi Boris
On Fri, Oct 5, 2018 at 9:32 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Fri, 5 Oct 2018 20:12:43 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> >
> > I think I know what might be the issue. on cfi_cmdset_002.c
> > cfi_amdstd_reset can be called in parrallel to cfi_amdstd_destroy.
> >
> > maybe we should call
> > unregister_reboot_notifier(&mtd->reboot_notifier);
> > before
> > cfi_amdstd_reset(mtd)
> >
> > need to try that on real hw with some strategical printfs :P. But that
> > will ahve to wait til Monday
>
> No problem, nothing urgent here. Have a nice weekend.

Just updated my kernel version (4.14.53 ->  4.14.69) with you last
github and I do not see the crash anymore. So I guess we are good to
go.

Thanks!


-- 
Ricardo Ribalda

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

On Mon, 8 Oct 2018 09:40:09 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> On Fri, Oct 5, 2018 at 9:32 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > On Fri, 5 Oct 2018 20:12:43 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> >  
> > >
> > > I think I know what might be the issue. on cfi_cmdset_002.c
> > > cfi_amdstd_reset can be called in parrallel to cfi_amdstd_destroy.
> > >
> > > maybe we should call
> > > unregister_reboot_notifier(&mtd->reboot_notifier);
> > > before
> > > cfi_amdstd_reset(mtd)
> > >
> > > need to try that on real hw with some strategical printfs :P. But that
> > > will ahve to wait til Monday  
> >
> > No problem, nothing urgent here. Have a nice weekend.  
> 
> Just updated my kernel version (4.14.53 ->  4.14.69) with you last
> github and I do not see the crash anymore. So I guess we are good to
> go.

Okay, I'll try to send the patch series. Don't hesitate to ping me if I
forget.

Thanks,

Boris

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

On Fri, 5 Oct 2018 14:04:52 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > On Fri, 5 Oct 2018 11:54:18 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> >  
> > > Hi Boris
> > >
> > > Just seen that  you already did the rebase at
> > > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> > >
> > > Thanks for that.
> > >
> > > I am about to test it in real hw (unless you want me wait)  
> >
> > Sure, go ahead and test it.
> >
> > Thanks,
> >
> > Boris  
> I had to change this on your patchset to have it working on hw:
> https://pastebin.com/78A7yhJ9
> 
> If you send the patchset to the mailing list I can review it patch by patch.
> 
> Also
> mtd: maps: Prepare merging of physmap and physmap_of

It seems to compile just fine here. Is there a problem at runtime?

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

Hi Boris
On Fri, Oct 5, 2018 at 4:46 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Fri, 5 Oct 2018 14:04:52 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > Hi Boris
> > On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:
> > >
> > > On Fri, 5 Oct 2018 11:54:18 +0200
> > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > >
> > > > Hi Boris
> > > >
> > > > Just seen that  you already did the rebase at
> > > > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> > > >
> > > > Thanks for that.
> > > >
> > > > I am about to test it in real hw (unless you want me wait)
> > >
> > > Sure, go ahead and test it.
> > >
> > > Thanks,
> > >
> > > Boris
> > I had to change this on your patchset to have it working on hw:
> > https://pastebin.com/78A7yhJ9
> >
> > If you send the patchset to the mailing list I can review it patch by patch.
> >
> > Also
> > mtd: maps: Prepare merging of physmap and physmap_of
>
> It seems to compile just fine here. Is there a problem at runtime?

Sorry, my fault, that is just me trying to review in github. I saw the
cahnge on the Makefile but not the rename. Ignore this comment



-- 
Ricardo Ribalda

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

On Fri, 5 Oct 2018 14:04:52 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > On Fri, 5 Oct 2018 11:54:18 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> >  
> > > Hi Boris
> > >
> > > Just seen that  you already did the rebase at
> > > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> > >
> > > Thanks for that.
> > >
> > > I am about to test it in real hw (unless you want me wait)  
> >
> > Sure, go ahead and test it.
> >
> > Thanks,
> >
> > Boris  
> I had to change this on your patchset to have it working on hw:
> https://pastebin.com/78A7yhJ9


I pushed a new version to my branch. Can you test it? The reboot
notifier bug should still be present, but I don't think this is
something we introduced.

Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

HI again!
On Fri, Oct 5, 2018 at 5:23 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Fri, 5 Oct 2018 14:04:52 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > Hi Boris
> > On Fri, Oct 5, 2018 at 12:12 PM Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:
> > >
> > > On Fri, 5 Oct 2018 11:54:18 +0200
> > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > >
> > > > Hi Boris
> > > >
> > > > Just seen that  you already did the rebase at
> > > > https://github.com/bbrezillon/linux-0day/commits/mtd/physmap-cleanup
> > > >
> > > > Thanks for that.
> > > >
> > > > I am about to test it in real hw (unless you want me wait)
> > >
> > > Sure, go ahead and test it.
> > >
> > > Thanks,
> > >
> > > Boris
> > I had to change this on your patchset to have it working on hw:
> > https://pastebin.com/78A7yhJ9
>
>
> I pushed a new version to my branch. Can you test it? The reboot
> notifier bug should still be present, but I don't think this is
> something we introduced.

I will do it next week, I already left the office and this weekend I
have a big 2 year old bday party .

Have a very nice weekend and thanks for your help!


-- 
Ricardo Ribalda