[PATCH v4 01/10] mtd: st_spi_fsm: dt-bindings: Deprecate generic compatible string

[PATCH v4 01/10] mtd: st_spi_fsm: dt-bindings: Deprecate generic compatible string

[Lee Jones]

This driver now obtains platform information via DT matching, which requires
a compatible string per platform.  This change introduces the new specific
strings and deprecates the old generic one.

We also take out all of the old, unused properties which are no longer
required.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/mtd/st-fsm.txt | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/st-fsm.txt b/Documentation/devicetree/bindings/mtd/st-fsm.txt
index c248939..389e43d 100644
--- a/Documentation/devicetree/bindings/mtd/st-fsm.txt
+++ b/Documentation/devicetree/bindings/mtd/st-fsm.txt
@@ -1,26 +1,19 @@
 * ST-Microelectronics SPI FSM Serial (NOR) Flash Controller
 
 Required properties:
-  - compatible : Should be "st,spi-fsm"
+  - compatible : Should be one of;
+		   "st,stid127-spi-fsm"
+		   "st,stih407-spi-fsm"
+		   "st,stih416-spi-fsm"
   - reg        : Contains register's location and length.
-  - reg-names  : Should contain the reg names "spi-fsm"
   - interrupts : The interrupt number
   - pinctrl-0  : Standard Pinctrl phandle (see: pinctrl/pinctrl-bindings.txt)
-
-Optional properties:
-  - st,syscfg          : Phandle to boot-device system configuration registers
-  - st,boot-device-reg : Address of the aforementioned boot-device register(s)
-  - st,boot-device-spi : Expected boot-device value if booted via this device
+  - st,syscfg  : Phandle to boot-device system configuration registers
 
 Example:
 	spifsm: spifsm@fe902000{
-	        compatible         = "st,spi-fsm";
+		compatible         = "st,stih407-spi-fsm";
 	        reg                =  <0xfe902000 0x1000>;
-	        reg-names          = "spi-fsm";
 	        pinctrl-0          = <&pinctrl_fsm>;
 		st,syscfg	   = <&syscfg_rear>;
-	        st,boot-device-reg = <0x958>;
-	        st,boot-device-spi = <0x1a>;
-		status = "okay";
 	};
-
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 02/10] mtd: st_spi_fsm: Fetch boot device locations from DT match tables

[Lee Jones]

To trim down on the amount of properties used by this driver and to conform
to the newly agreed method of acquiring syscfg registers/offsets, we now
obtain this information using match tables.

In the process we are deprecating the old generic compatible string and
providing 3 shiny new ones for each of the support platforms.  The
deprecated compatible string will be removed in due course.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 74 ++++++++++++++++++++++++++++++----------
 1 file changed, 56 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 3060025..9e4fded 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -29,6 +29,21 @@
 #include "serial_flash_cmds.h"
 
 /*
+ * FSM SPI Boot Mode Registers/Masks
+ */
+#define STID127_SYSCON_BOOT_DEV_REG	0x0D8
+#define STID127_SYSCON_BOOT_DEV_SPI	0x068
+#define STID127_SYSCON_BOOT_DEV_MASK	0x07C
+
+#define STIH407_SYSCON_BOOT_DEV_REG	0x8C4
+#define STIH407_SYSCON_BOOT_DEV_SPI	0x068
+#define STIH407_SYSCON_BOOT_DEV_MASK	0x07C
+
+#define STIH416_SYSCON_BOOT_DEV_REG	0x958
+#define STIH416_SYSCON_BOOT_DEV_SPI	0x01A
+#define STIH416_SYSCON_BOOT_DEV_MASK	0x01F
+
+/*
  * FSM SPI Controller Registers
  */
 #define SPI_CLOCKDIV			0x0010
@@ -288,6 +303,12 @@ struct seq_rw_config {
 	uint8_t         dummy_cycles;   /* No. of DUMMY cycles */
 };
 
+struct stfsm_boot_dev {
+	uint32_t		reg;
+	uint32_t		spi;
+	uint32_t		mask;
+};
+
 /* SPI Flash Device Table */
 struct flash_info {
 	char            *name;
@@ -313,6 +334,24 @@ struct flash_info {
 	int             (*config)(struct stfsm *);
 };
 
+static struct stfsm_boot_dev stfsm_stid127_data = {
+	.reg  = STID127_SYSCON_BOOT_DEV_REG,
+	.spi  = STID127_SYSCON_BOOT_DEV_SPI,
+	.mask = STID127_SYSCON_BOOT_DEV_MASK,
+};
+
+static struct stfsm_boot_dev stfsm_stih407_data = {
+	.reg  = STIH407_SYSCON_BOOT_DEV_REG,
+	.spi  = STIH407_SYSCON_BOOT_DEV_SPI,
+	.mask = STIH407_SYSCON_BOOT_DEV_MASK,
+};
+
+static struct stfsm_boot_dev stfsm_stih416_data = {
+	.reg  = STIH416_SYSCON_BOOT_DEV_REG,
+	.spi  = STIH416_SYSCON_BOOT_DEV_SPI,
+	.mask = STIH416_SYSCON_BOOT_DEV_MASK,
+};
+
 static int stfsm_n25q_config(struct stfsm *fsm);
 static int stfsm_mx25_config(struct stfsm *fsm);
 static int stfsm_s25fl_config(struct stfsm *fsm);
@@ -1977,14 +2016,22 @@ static int stfsm_init(struct stfsm *fsm)
 	return 0;
 }
 
+static const struct of_device_id stfsm_match[] = {
+	{ .compatible = "st,stid127-spi-fsm", .data = &stfsm_stid127_data },
+	{ .compatible = "st,stih407-spi-fsm", .data = &stfsm_stih407_data },
+	{ .compatible = "st,stih416-spi-fsm", .data = &stfsm_stih416_data },
+	{},
+};
+MODULE_DEVICE_TABLE(of, stfsm_match);
+
 static void stfsm_fetch_platform_configs(struct platform_device *pdev)
 {
 	struct stfsm *fsm = platform_get_drvdata(pdev);
 	struct device_node *np = pdev->dev.of_node;
+	const struct stfsm_boot_dev *boot_dev;
+	const struct of_device_id *match;
 	struct regmap *regmap;
-	uint32_t boot_device_reg;
-	uint32_t boot_device_spi;
-	uint32_t boot_device;     /* Value we read from *boot_device_reg */
+	uint32_t boot_device;    /* Value we read from the boot dev mode pins */
 	int ret;
 
 	/* Booting from SPI NOR Flash is the default */
@@ -1998,21 +2045,18 @@ static void stfsm_fetch_platform_configs(struct platform_device *pdev)
 
 	fsm->reset_por = of_property_read_bool(np, "st,reset-por");
 
-	/* Where in the syscon the boot device information lives */
-	ret = of_property_read_u32(np, "st,boot-device-reg", &boot_device_reg);
-	if (ret)
+	match = of_match_node(stfsm_match, np);
+	if (!match)
 		goto boot_device_fail;
+	boot_dev = match->data;
 
-	/* Boot device value when booted from SPI NOR */
-	ret = of_property_read_u32(np, "st,boot-device-spi", &boot_device_spi);
+	ret = regmap_read(regmap, boot_dev->reg, &boot_device);
 	if (ret)
 		goto boot_device_fail;
 
-	ret = regmap_read(regmap, boot_device_reg, &boot_device);
-	if (ret)
-		goto boot_device_fail;
+	boot_device &= boot_dev->mask;
 
-	if (boot_device != boot_device_spi)
+	if (boot_device != boot_dev->spi)
 		fsm->booted_from_spi = false;
 
 	return;
@@ -2156,12 +2200,6 @@ static int stfsmfsm_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(stfsm_pm_ops, stfsmfsm_suspend, stfsmfsm_resume);
 
-static const struct of_device_id stfsm_match[] = {
-	{ .compatible = "st,spi-fsm", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, stfsm_match);
-
 static struct platform_driver stfsm_driver = {
 	.probe		= stfsm_probe,
 	.remove		= stfsm_remove,
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 09/10] ARM: STi: stih416: Use new platform specific compatible string

[Lee Jones]

The FSM SPI NOR driver now obtains syscfg particulars using DT match. In
order for this to happen each platform is required to supply their own
specific compatible string.

We're also remove the old, now unused vendor properties from the node.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/stih416.dtsi | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index fad9073..2e35bab7 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -216,13 +216,11 @@
 
 		/* FSM */
 		spifsm: spifsm@fe902000 {
-			compatible	   = "st,spi-fsm";
+			compatible	   = "st,stih416-spi-fsm";
 			reg		   = <0xfe902000 0x1000>;
 			pinctrl-0	   = <&pinctrl_fsm>;
 
 			st,syscfg	   = <&syscfg_rear>;
-			st,boot-device-reg = <0x958>;
-			st,boot-device-spi = <0x1a>;
 
 			status = "disabled";
 		};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 10/10] ARM: STi: stih416: Supply EMI clock reference to FSM SPI NOR

[Lee Jones]

While we're at it we're also adding a new human readable define for the
aforementioned clock.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/stih416.dtsi           | 2 +-
 include/dt-bindings/clock/stih416-clks.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 2e35bab7..39b856b 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -219,7 +219,7 @@
 			compatible	   = "st,stih416-spi-fsm";
 			reg		   = <0xfe902000 0x1000>;
 			pinctrl-0	   = <&pinctrl_fsm>;
-
+			clocks		   = <&clk_s_a0_ls CLK_EMISS>;
 			st,syscfg	   = <&syscfg_rear>;
 
 			status = "disabled";
diff --git a/include/dt-bindings/clock/stih416-clks.h b/include/dt-bindings/clock/stih416-clks.h
index f9bdbd1..4a46a47e 100644
--- a/include/dt-bindings/clock/stih416-clks.h
+++ b/include/dt-bindings/clock/stih416-clks.h
@@ -7,6 +7,7 @@
 
 /* CLOCKGEN A0 */
 #define CLK_ICN_REG		0
+#define CLK_EMISS		3
 #define CLK_ETH1_PHY		4
 
 /* CLOCKGEN A1 */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 02/10] mtd: st_spi_fsm: Fetch boot device locations from DT match tables

[Brian Norris]

On Wed, Jan 21, 2015 at 03:24:20PM +0000, Lee Jones wrote:
> To trim down on the amount of properties used by this driver and to conform
> to the newly agreed method of acquiring syscfg registers/offsets, we now
> obtain this information using match tables.

Where did this agreement happen? Are you only referring to the previous
patch?

I think I asked this previously, and I didn't get an answer.

Also, I realized that all this boot device / syscfg gymnastics is just
for one simple fact; your driver is trying to hide the fact that your
system can't reliably handle 4-byte addressing for the boot device. Even
if you try your best at toggling 4-byte addressing before/after each
read/write/erase, you still are vulnerable to power cuts during the
operation. This is a bad design, and we have consistently agreed that we
aren't going to work around that in Linux.

Better solutions: hook up a reset line to your flash; improve your boot
ROM / bootloader to handle 4-byte addressing for large flash.

What's the possibility of dropping all this 4-byte address toggling
shenanigans? This will be a blocker to merging with spi-nor.c.

> In the process we are deprecating the old generic compatible string and
> providing 3 shiny new ones for each of the support platforms.  The
> deprecated compatible string will be removed in due course.

Aren't you already removing the compatible string? (You changed this in
the latest revision.)

> Cc: devicetree@vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

[snip]

Brian

Re: [PATCH v4 02/10] mtd: st_spi_fsm: Fetch boot device locations from DT match tables

[Lee Jones]

On Thu, 05 Feb 2015, Brian Norris wrote:

> On Wed, Jan 21, 2015 at 03:24:20PM +0000, Lee Jones wrote:
> > To trim down on the amount of properties used by this driver and to conform
> > to the newly agreed method of acquiring syscfg registers/offsets, we now
> > obtain this information using match tables.
> 
> Where did this agreement happen? Are you only referring to the previous
> patch?

I think your interpretation of the above text and my intentions are
not the same.  I have no idea why there is a different configuration
depending on if we booted from SPI NOR or not and hence can not answer
your query below.  The description above is pertaining to the
different/new way in which we obtain and request syscfg registers.

In previous incarnations of this patchset, we were defining new vendor
specific properties in order to request and register and the mask:

  st,boot-device-reg = <0x958>;
  st,boot-device-spi = <0x1a>;

... this is not optimal, as DT properties should only be created if
there are no other way to obtain platform specific information.  As
there are few supported platforms and this configuration does not
change through variants, we are now supplying it via static tables,
which can be obtained easily using the DT match framework.

> I think I asked this previously, and I didn't get an answer.

Not sure if you did or not to be honest.

> Also, I realized that all this boot device / syscfg gymnastics is just
> for one simple fact; your driver is trying to hide the fact that your
> system can't reliably handle 4-byte addressing for the boot device. Even
> if you try your best at toggling 4-byte addressing before/after each
> read/write/erase, you still are vulnerable to power cuts during the
> operation. This is a bad design, and we have consistently agreed that we
> aren't going to work around that in Linux.
> 
> Better solutions: hook up a reset line to your flash; improve your boot
> ROM / bootloader to handle 4-byte addressing for large flash.

You have reached the boundaries of my knowledge on this.  Perhaps
Angus (BCC'ed) would be kind enough to assist.

> What's the possibility of dropping all this 4-byte address toggling
> shenanigans? This will be a blocker to merging with spi-nor.c.
> 
> > In the process we are deprecating the old generic compatible string and
> > providing 3 shiny new ones for each of the support platforms.  The
> > deprecated compatible string will be removed in due course.
> 
> Aren't you already removing the compatible string? (You changed this in
> the latest revision.)

You're right.  I need to remove this line from the commit log.

> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> [snip]
> 
> Brian

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 02/10] mtd: st_spi_fsm: Fetch boot device locations from DT match tables

[Brian Norris]

On Tue, Feb 10, 2015 at 03:46:34PM +0800, Lee Jones wrote:
> On Thu, 05 Feb 2015, Brian Norris wrote:
> > On Wed, Jan 21, 2015 at 03:24:20PM +0000, Lee Jones wrote:
> > > To trim down on the amount of properties used by this driver and to conform
> > > to the newly agreed method of acquiring syscfg registers/offsets, we now
> > > obtain this information using match tables.
> > 
> > Where did this agreement happen? Are you only referring to the previous
> > patch?
> 
> I think your interpretation of the above text and my intentions are
> not the same.

To be clear: I'm simply asking what do you mean by "agreed method". I
never agreed to syscfg registers/offsets. So who did? Are you agreeing
with yourself?

> I have no idea why there is a different configuration
> depending on if we booted from SPI NOR or not and hence can not answer
> your query below.

Seriously? That's all you can come up with? Sheesh. And you wonder why I
called you out on not understanding the code that you're sending me.

> The description above is pertaining to the
> different/new way in which we obtain and request syscfg registers.

OK. So you're dealing with the "how" but not the "why." That is not a
reasonable way to develop good code.

> In previous incarnations of this patchset, we were defining new vendor
> specific properties in order to request and register and the mask:
> 
>   st,boot-device-reg = <0x958>;
>   st,boot-device-spi = <0x1a>;
> 
> ... this is not optimal, as DT properties should only be created if
> there are no other way to obtain platform specific information.  As
> there are few supported platforms and this configuration does not
> change through variants, we are now supplying it via static tables,
> which can be obtained easily using the DT match framework.

I understand what you're doing with syscfg and these register offsets.
But if you follow the code as to what they're actually producing, you
see that it yields the 'booted_from_spi' boolean. That's a pretty simple
concept.

Now, unless you were able to provide an additional enlightening
viewpoint, then the following paragraph likely all holds true:

> > Also, I realized that all this boot device / syscfg gymnastics is just
> > for one simple fact; your driver is trying to hide the fact that your
> > system can't reliably handle 4-byte addressing for the boot device. Even
> > if you try your best at toggling 4-byte addressing before/after each
> > read/write/erase, you still are vulnerable to power cuts during the
> > operation. This is a bad design, and we have consistently agreed that we
> > aren't going to work around that in Linux.
> > 
> > Better solutions: hook up a reset line to your flash; improve your boot
> > ROM / bootloader to handle 4-byte addressing for large flash.
> 
> You have reached the boundaries of my knowledge on this.  Perhaps
> Angus (BCC'ed) would be kind enough to assist.

And so we have also reached the boundaries of my willingness to review
your code. There's a significant technical point here that drove you to
define several new DT compatible strings. I propose (and am now more
convinced) that this is not actually necessary. But apparently you are
not equipped to have a discussion about this.

I'm tempted to:

  git rm drivers/mtd/devices/st_spi_fsm.c

(Along with the appropriate Kconfig and Makefile entries, of course.)

> > What's the possibility of dropping all this 4-byte address toggling
> > shenanigans? This will be a blocker to merging with spi-nor.c.

Brian

Re: [PATCH v4 02/10] mtd: st_spi_fsm: Fetch boot device locations from DT match tables

[Lee Jones]

On Mon, 23 Feb 2015, Brian Norris wrote:

> On Tue, Feb 10, 2015 at 03:46:34PM +0800, Lee Jones wrote:
> > On Thu, 05 Feb 2015, Brian Norris wrote:
> > > On Wed, Jan 21, 2015 at 03:24:20PM +0000, Lee Jones wrote:
> > > > To trim down on the amount of properties used by this driver and to conform
> > > > to the newly agreed method of acquiring syscfg registers/offsets, we now
> > > > obtain this information using match tables.
> > > 
> > > Where did this agreement happen? Are you only referring to the previous
> > > patch?
> > 
> > I think your interpretation of the above text and my intentions are
> > not the same.
> 
> To be clear: I'm simply asking what do you mean by "agreed method". I
> never agreed to syscfg registers/offsets. So who did? Are you agreeing
> with yourself?

Look:

> The description above is pertaining to the
> different/new way in which we obtain and request syscfg registers.

When I say "agreed method", I mean the way in which we obtain syscfg
registers/offsets, not the reason for using them.  How is that not
clear in the commit log, "agreed method of acquiring syscfg registers/
offsets"?

And no, you never agreed to that.  You weren't part of the
conversation.  For your own reference one of the first patches which
deals with this "newly agreed method" and supplies a succinct
explanation can be found here:

  https://lkml.org/lkml/2014/11/19/78

> > The description above is pertaining to the
> > different/new way in which we obtain and request syscfg registers.
> 
> OK. So you're dealing with the "how" but not the "why."

I'm dealing with answering the question that was asked.  You mentioned
that you did not "agree" to using the boot devices in this way and I
was explaining that when I said "agreed", I was talking about
something else.

> That is not a reasonable way to develop good code.

I'm not entirely sure what you're talking about.  This patch was
designed to introduce a clean way to extract important values from
syscfg to be used with functionality written by our local MTD expert.
I don't know about you, but I believe that identifying a need, setting
an aim and successfully achieving that aim is a great way to code.

Unless of course you re you insinuating that I should have been aware
of conversations which you previously had with other parties about
resilience to mode setting over reset/power-outage?

> > I have no idea why there is a different configuration
> > depending on if we booted from SPI NOR or not and hence can not answer
> > your query below.
> 
> Seriously? That's all you can come up with? Sheesh. And you wonder why I
> called you out on not understanding the code that you're sending me.

I already explained to you that I am not an MTD expert and still you
cut me no slack.  At one time I was supported by someone who can
answer all of these questions; however, as you well know, this is no
longer the case.  The company does have people that specialise in MTD,
but they are so busy with customer projects that they have no time to
support these endeavours.  So I am on my own!

Everything that I know now, I have learned from you.  So please don't
act so surprised when I struggle to answer all these new questions
you're posing.

> > In previous incarnations of this patchset, we were defining new vendor
> > specific properties in order to request and register and the mask:
> > 
> >   st,boot-device-reg = <0x958>;
> >   st,boot-device-spi = <0x1a>;
> > 
> > ... this is not optimal, as DT properties should only be created if
> > there are no other way to obtain platform specific information.  As
> > there are few supported platforms and this configuration does not
> > change through variants, we are now supplying it via static tables,
> > which can be obtained easily using the DT match framework.
> 
> I understand what you're doing with syscfg and these register offsets.
> But if you follow the code as to what they're actually producing, you
> see that it yields the 'booted_from_spi' boolean. That's a pretty simple
> concept.
> 
> Now, unless you were able to provide an additional enlightening
> viewpoint, then the following paragraph likely all holds true:
> 
> > > Also, I realized that all this boot device / syscfg gymnastics is just
> > > for one simple fact; your driver is trying to hide the fact that your
> > > system can't reliably handle 4-byte addressing for the boot device. Even
> > > if you try your best at toggling 4-byte addressing before/after each
> > > read/write/erase, you still are vulnerable to power cuts during the
> > > operation. This is a bad design, and we have consistently agreed that we
> > > aren't going to work around that in Linux.
> > > 
> > > Better solutions: hook up a reset line to your flash; improve your boot
> > > ROM / bootloader to handle 4-byte addressing for large flash.

Okay, I'm re-read the code and have a new understanding about the
boot-from-spi 'gymnastics'. 

There is a separate controller on the platform which acts as a boot
device and makes the NOR chip appear as though it is memory mapped.
This expects the NOR Controller to be in its default state [24-bit
addressing] on boot.  The issue arises if a warm-reset occurs and the
device is still in 32-bit addressing mode.  To minimise the risk, the
controller attempts to stay in 24-bit addressing mode for as long as
possible.

You mentioned power-cuts.  I do not believe this to be an issue, as
when the power is completely removed the controller will reset back
into default state.  Only warm-resets are an issue.

> And so we have also reached the boundaries of my willingness to review
> your code. There's a significant technical point here that drove you to
> define several new DT compatible strings. I propose (and am now more
> convinced) that this is not actually necessary. But apparently you are
> not equipped to have a discussion about this.
> 
> I'm tempted to:
> 
>   git rm drivers/mtd/devices/st_spi_fsm.c
> 
> (Along with the appropriate Kconfig and Makefile entries, of course.)

That would be very immature indeed.

> > > What's the possibility of dropping all this 4-byte address toggling
> > > shenanigans? This will be a blocker to merging with spi-nor.c.

We wouldn't be able to remove this code without significantly
weakening resilience to warm-reset mishaps, and changing the hardware
design for devices which have already been released is obviously out
of the question.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 02/10] mtd: st_spi_fsm: Fetch boot device locations from DT match tables

[Brian Norris]

On Tue, Feb 24, 2015 at 09:41:10AM +0000, Lee Jones wrote:
> On Mon, 23 Feb 2015, Brian Norris wrote:
> > On Tue, Feb 10, 2015 at 03:46:34PM +0800, Lee Jones wrote:
> > > On Thu, 05 Feb 2015, Brian Norris wrote:

[snip other discussion]

> > Now, unless you were able to provide an additional enlightening
> > viewpoint, then the following paragraph likely all holds true:
> > 
> > > > Also, I realized that all this boot device / syscfg gymnastics is just
> > > > for one simple fact; your driver is trying to hide the fact that your
> > > > system can't reliably handle 4-byte addressing for the boot device. Even
> > > > if you try your best at toggling 4-byte addressing before/after each
> > > > read/write/erase, you still are vulnerable to power cuts during the
> > > > operation. This is a bad design, and we have consistently agreed that we
> > > > aren't going to work around that in Linux.
> > > > 
> > > > Better solutions: hook up a reset line to your flash; improve your boot
> > > > ROM / bootloader to handle 4-byte addressing for large flash.
> 
> Okay, I'm re-read the code and have a new understanding about the
> boot-from-spi 'gymnastics'. 

Great! See, much of that could be done by reading your own code (yeah,
yeah, not "yours"; but still) and honestly dealing with my questions,
rather than giving up and deferring to me or your MIA authorities. I'm
happy to return to technical points and avoid the other unpleasantness.

> There is a separate controller on the platform which acts as a boot
> device and makes the NOR chip appear as though it is memory mapped.
> This expects the NOR Controller to be in its default state [24-bit
> addressing] on boot.  The issue arises if a warm-reset occurs and the
> device is still in 32-bit addressing mode.

OK, this is all familiar. This is common to many other systems.

> To minimise the risk, the
> controller attempts to stay in 24-bit addressing mode for as long as
> possible.

This is the part where we differ, I suppose. The "as long as possible"
statement is still not sufficient; I believe this still leaves holes
that simply cannot be fixed in Linux.

> You mentioned power-cuts.  I do not believe this to be an issue, as
> when the power is completely removed the controller will reset back
> into default state.  Only warm-resets are an issue.

You're right: power cuts shouldn't be a problem. But what about other
unexpected warm resets? (Watchdogs?) Do you have any solution for them?

> > > > What's the possibility of dropping all this 4-byte address toggling
> > > > shenanigans? This will be a blocker to merging with spi-nor.c.
> 
> We wouldn't be able to remove this code without significantly
> weakening resilience to warm-reset mishaps, and changing the hardware
> design for devices which have already been released is obviously out
> of the question.

Then maybe we can't solve this. That doesn't mean that upstream will
support you, though.

Problems like this are why "release early, release often" makes sense.
If your employer didn't take the "fire the engineers and dump software
support to the community" approach, but rather honestly engaged on
driver support earlier, then perhaps your employer could have fixed the
SoCs/boot ROMs/board designs earlier, rather than later, and you
wouldn't be stuck trying to wedge in upstream workarounds for bad
designs in the wild.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 02/10] mtd: st_spi_fsm: Fetch boot device locations from DT match tables

[Lee Jones]

On Fri, 13 Mar 2015, Brian Norris wrote:
> On Tue, Feb 24, 2015 at 09:41:10AM +0000, Lee Jones wrote:
> > On Mon, 23 Feb 2015, Brian Norris wrote:
> > > On Tue, Feb 10, 2015 at 03:46:34PM +0800, Lee Jones wrote:
> > > > On Thu, 05 Feb 2015, Brian Norris wrote:
> 
> [snip other discussion]

[...]

> > > Now, unless you were able to provide an additional enlightening
> > > viewpoint, then the following paragraph likely all holds true:
> > > 
> > > > > Also, I realized that all this boot device / syscfg gymnastics is just
> > > > > for one simple fact; your driver is trying to hide the fact that your
> > > > > system can't reliably handle 4-byte addressing for the boot device. Even
> > > > > if you try your best at toggling 4-byte addressing before/after each
> > > > > read/write/erase, you still are vulnerable to power cuts during the
> > > > > operation. This is a bad design, and we have consistently agreed that we
> > > > > aren't going to work around that in Linux.
> > > > > 
> > > > > Better solutions: hook up a reset line to your flash; improve your boot
> > > > > ROM / bootloader to handle 4-byte addressing for large flash.
> > 
> > Okay, I'm re-read the code and have a new understanding about the
> > boot-from-spi 'gymnastics'. 

[snipping lecture]

> I'm
> happy to return to technical points and avoid the other unpleasantness.

Yes, let's start again.

> > There is a separate controller on the platform which acts as a boot
> > device and makes the NOR chip appear as though it is memory mapped.
> > This expects the NOR Controller to be in its default state [24-bit
> > addressing] on boot.  The issue arises if a warm-reset occurs and the
> > device is still in 32-bit addressing mode.
> 
> OK, this is all familiar. This is common to many other systems.
> 
> > To minimise the risk, the
> > controller attempts to stay in 24-bit addressing mode for as long as
> > possible.
> 
> This is the part where we differ, I suppose. The "as long as possible"
> statement is still not sufficient; I believe this still leaves holes
> that simply cannot be fixed in Linux.

Linux supports lots of devices which are not perfect.  Minimising risk
to error prone hardware is one of software's key roles.  Being a
software guy, I don't like it any more than you do, but it is a fact
of life that hardware isn't perfect.  That's why the Linux kernel
supports a truck-load of 'errata' and 'quirks'.  Saying "we're not
going to support that in Linux" doesn't sound like the right attitude
to me.

So when you said "we have consistently agreed that we aren't going to
work around that in Linux", who has agreed this.  Would you be kind
enough to point me in the direction of that conversation please?

> > You mentioned power-cuts.  I do not believe this to be an issue, as
> > when the power is completely removed the controller will reset back
> > into default state.  Only warm-resets are an issue.
> 
> You're right: power cuts shouldn't be a problem. But what about other
> unexpected warm resets? (Watchdogs?) Do you have any solution for them?

They are possible and are the point of the patch.  If they happen
while we're in 24bit mode, we risk corruption.  The best solution
provided by out estrange colleague is provided in this set.  The risk
of a soft reset happening during the small amount of time that we're
in 32bit mode is considered acceptable.  Without this patch, the risk
is significantly more substantial.

> > > > > What's the possibility of dropping all this 4-byte address toggling
> > > > > shenanigans? This will be a blocker to merging with spi-nor.c.
> > 
> > We wouldn't be able to remove this code without significantly
> > weakening resilience to warm-reset mishaps, and changing the hardware
> > design for devices which have already been released is obviously out
> > of the question.
> 
> Then maybe we can't solve this. That doesn't mean that upstream will
> support you, though.
> 
> Problems like this are why "release early, release often" makes sense.
> If your employer didn't take the "fire the engineers and dump software
> support to the community" approach, but rather honestly engaged on
> driver support earlier, then perhaps your employer could have fixed the
> SoCs/boot ROMs/board designs earlier, rather than later, and you
> wouldn't be stuck trying to wedge in upstream workarounds for bad
> designs in the wild.

These lessons have now been learnt.  The attitude to upstreaming in
the present day is vastly different to how it was when this driver was
initially authored.

As for the business decisions you allude to, I'm afraid I have no
influence in those and am not qualified to comment. ;)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 02/10] mtd: st_spi_fsm: Fetch boot device locations from DT match tables

[Brian Norris]

On Mon, Mar 16, 2015 at 08:13:06AM +0000, Lee Jones wrote:
> So when you said "we have consistently agreed that we aren't going to
> work around that in Linux", who has agreed this.  Would you be kind
> enough to point me in the direction of that conversation please?

A few pointers from recent history:

http://lists.infradead.org/pipermail/linux-mtd/2014-September/055156.html

http://lists.infradead.org/pipermail/linux-mtd/2014-September/055559.html

If the question came up in other threads, though, I'm sure the answers
were the same.

BTW, there are sound solutions to this problem in Linux, and they are
supported where possible. Some flash support a separate set of opcodes
that always expect a 4-byte address. If we use these opcodes, we never
have to do any persistent mode-switching. We only found that Spansion
consistently supports these opcodes on all their flash, though.

See:

commit 87c9511fba2bd069a35e1312587a29e112fc0cd6
Author: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date:   Thu Apr 11 01:34:57 2013 -0700

    mtd: m25p80: utilize dedicated 4-byte addressing commands

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html