* Re: [PATCH v3 04/13] mtd: st_spi_fsm: Fetch boot device locations from DT match tables
[not found] ` <1418644760-18773-5-git-send-email-lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
@ 2015-01-13 4:55 ` Brian Norris
2015-01-21 12:56 ` Lee Jones
0 siblings, 1 reply; 6+ messages in thread
From: Brian Norris @ 2015-01-13 4:55 UTC (permalink / raw)
To: Lee Jones
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kernel-F5mvAk5X5gdBDgjK7y7TUQ, devicetree-u79uwXL29TY76Z2rM5mHXA
+ devicetree
On Mon, Dec 15, 2014 at 11:59:11AM +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.
>
> 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.
I'm not sure how this agreement was done, but by default, we expect not
to break backwards compatibility. This patch does just that.
> Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> drivers/mtd/devices/st_spi_fsm.c | 75 ++++++++++++++++++++++++++++++----------
> 1 file changed, 57 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> index fac0fe9..d82394a 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,23 @@ static int stfsm_init(struct stfsm *fsm)
> return 0;
> }
>
> +static const struct of_device_id stfsm_match[] = {
> + { .compatible = "st,spi-fsm" }, /* DEPRECATED */
> + { .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 +2046,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);
^^^ NULL pointer dereference for any existing DTB that uses the
"st,spi-fsm" compatible string. So I'll NAK this patch as-is. Either we
need to completely drop the "deprecated" compatible property (what's the
point binding to it, if we're going to OOPS on it anyway), or else we
need to maintain both methods (at least for whatever deprecation period
is deemed acceptable).
> 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 +2201,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,
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 03/13] mtd: st_spi_fsm: dt-bindings: Deprecate generic compatible string
[not found] ` <1418644760-18773-4-git-send-email-lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
@ 2015-01-13 5:02 ` Brian Norris
0 siblings, 0 replies; 6+ messages in thread
From: Brian Norris @ 2015-01-13 5:02 UTC (permalink / raw)
To: Lee Jones
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kernel-F5mvAk5X5gdBDgjK7y7TUQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell
+ devicetree
Please include devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org whenever you send DT patches
like this.
On Mon, Dec 15, 2014 at 11:59:10AM +0000, Lee Jones wrote:
> 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.
>
> Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> Documentation/devicetree/bindings/mtd/st-fsm.txt | 20 +++++++-------------
> 1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/st-fsm.txt b/Documentation/devicetree/bindings/mtd/st-fsm.txt
> index c248939..4e3a16c 100644
> --- a/Documentation/devicetree/bindings/mtd/st-fsm.txt
> +++ b/Documentation/devicetree/bindings/mtd/st-fsm.txt
> @@ -1,26 +1,20 @@
> * ST-Microelectronics SPI FSM Serial (NOR) Flash Controller
>
> Required properties:
> - - compatible : Should be "st,spi-fsm"
> + - compatible : "st,spi-fsm" is now DEPRECATED
See my comments on patch 4.
In light of that, it may make sense to support both, so DTS files could
list:
compatible = "st,stid127-spi-fsm", "st,spi-fsm";
But I'm not really privy to whatever discussion determined that you
needed to deprecate the current binding. If there really are no users of
the current upstream binding, then maybe your current deprecation plan
can work fine.
> + 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";
> };
> -
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 00/13] mtd: st_spi_fsm: Align with ST's internal development
[not found] ` <1418644760-18773-1-git-send-email-lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
@ 2015-01-13 5:14 ` Brian Norris
2015-01-21 12:49 ` Lee Jones
0 siblings, 1 reply; 6+ messages in thread
From: Brian Norris @ 2015-01-13 5:14 UTC (permalink / raw)
To: Lee Jones
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kernel-F5mvAk5X5gdBDgjK7y7TUQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
Rob Herring, Mark Rutland
On Mon, Dec 15, 2014 at 11:59:07AM +0000, Lee Jones wrote:
> Hi Brian, all,
>
> [x] Bulid test
> [x] Bisectable
> [x] Smatch
> [x] Sparse
Picked patches 1, 2, and 5 to l2-mtd.git. I requested changes to patch 3
and possibly 4. The rest look OK, but I don't want to futz with patch
conflicts on the remaining. Please rebase to l2-mtd.git if/when you
resend.
> v3:
> Further aligned with upstream changes as suggested by Peter Griffin.
> The clk_ignore_unused kernel command line parameter is due to be turned
> off on STiH4* platforms, so we no longer give the driver the chance to
> opt-out of fetching the EMI clock. We also now provide syscfg offsets
> inside the driver, as it's simpler and reduces the cost of having lots
> of extra DT properties.
Typically "simpler" and "reducing the cost of having lots of extra DT
properties" aren't good enough reasons for immediately breaking an
existing DT binding. We usually expect to support both for some period
of time.
> v2:
> These are the last remaining patches in the set, all rebased and
> with the DT documentation requested after the last submission.
>
> v1:
> This patch-set updates ST's FSM SPI-NOR driver with all the internal
> goodness which has happened since the initial (now upstreamed) snapshot
> was taken. It covers just over 6 months worth of internal development
> and bug-fixes. A final whitespace clean-up is also appended to the set
> - to make it look pretty and stuff. :)
>
> Angus Clark (3):
> mtd: st_spi_fsm: Add support for N25Q512 and N25Q00A devices
> mtd: st_spi_fsm: Update Spansion device entries
> mtd: st_spi_fsm: Improve busy wait handling
>
> Lee Jones (9):
> mtd: st_spi_fsm: Extend fsm_clear_fifo to handle unwanted bytes
> mtd: st_spi_fsm: Obtain and use EMI clock
> mtd: st_spi_fsm: dt-bindings: Deprecate generic compatible string
> mtd: st_spi_fsm: Fetch boot device locations from DT match tables
> mtd: st_spi_fsm: Fix [-Wsign-compare] build warning
> mtd: st_spi_fsm: Update the JEDEC probe to handle extended READIDs
> mtd: st_spi_fsm: General tidy-up
> ARM: STi: stih416: Use new platform specific compatible string
> ARM: STi: stih416: Supply EMI clock reference to FSM SPI NOR
>
> Nunzio Raciti (1):
> mtd: st_spi_fsm: Add support for Micron N25Q512A
>
> Documentation/devicetree/bindings/mtd/st-fsm.txt | 20 +-
> arch/arm/boot/dts/stih416.dtsi | 6 +-
> drivers/mtd/devices/st_spi_fsm.c | 747 +++++++++++++++--------
> include/dt-bindings/clock/stih416-clks.h | 1 +
> 4 files changed, 519 insertions(+), 255 deletions(-)
>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 00/13] mtd: st_spi_fsm: Align with ST's internal development
2015-01-13 5:14 ` [PATCH v3 00/13] mtd: st_spi_fsm: Align with ST's internal development Brian Norris
@ 2015-01-21 12:49 ` Lee Jones
2015-02-06 1:59 ` Brian Norris
0 siblings, 1 reply; 6+ messages in thread
From: Lee Jones @ 2015-01-21 12:49 UTC (permalink / raw)
To: Brian Norris
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kernel-F5mvAk5X5gdBDgjK7y7TUQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
Rob Herring, Mark Rutland
On Mon, 12 Jan 2015, Brian Norris wrote:
> On Mon, Dec 15, 2014 at 11:59:07AM +0000, Lee Jones wrote:
> > Hi Brian, all,
> >
> > [x] Bulid test
> > [x] Bisectable
> > [x] Smatch
> > [x] Sparse
>
> Picked patches 1, 2, and 5 to l2-mtd.git. I requested changes to patch 3
> and possibly 4. The rest look OK, but I don't want to futz with patch
> conflicts on the remaining. Please rebase to l2-mtd.git if/when you
> resend.
Sure. I'll get on to that now.
> > v3:
> > Further aligned with upstream changes as suggested by Peter Griffin.
> > The clk_ignore_unused kernel command line parameter is due to be turned
> > off on STiH4* platforms, so we no longer give the driver the chance to
> > opt-out of fetching the EMI clock. We also now provide syscfg offsets
> > inside the driver, as it's simpler and reduces the cost of having lots
> > of extra DT properties.
>
> Typically "simpler" and "reducing the cost of having lots of extra DT
> properties" aren't good enough reasons for immediately breaking an
> existing DT binding. We usually expect to support both for some period
> of time.
Only once a binding has became ABI. As there are no users of this
driver yet (this will change after this sync'ing patch-set has been
applied), so we can safely consider this binding to be in-progress and
as such, not yet ABI, thus we can do this ol' switch-aroo without the
usual 6 month deprecation period.
> > v2:
> > These are the last remaining patches in the set, all rebased and
> > with the DT documentation requested after the last submission.
> >
> > v1:
> > This patch-set updates ST's FSM SPI-NOR driver with all the internal
> > goodness which has happened since the initial (now upstreamed) snapshot
> > was taken. It covers just over 6 months worth of internal development
> > and bug-fixes. A final whitespace clean-up is also appended to the set
> > - to make it look pretty and stuff. :)
> >
> > Angus Clark (3):
> > mtd: st_spi_fsm: Add support for N25Q512 and N25Q00A devices
> > mtd: st_spi_fsm: Update Spansion device entries
> > mtd: st_spi_fsm: Improve busy wait handling
> >
> > Lee Jones (9):
> > mtd: st_spi_fsm: Extend fsm_clear_fifo to handle unwanted bytes
> > mtd: st_spi_fsm: Obtain and use EMI clock
> > mtd: st_spi_fsm: dt-bindings: Deprecate generic compatible string
> > mtd: st_spi_fsm: Fetch boot device locations from DT match tables
> > mtd: st_spi_fsm: Fix [-Wsign-compare] build warning
> > mtd: st_spi_fsm: Update the JEDEC probe to handle extended READIDs
> > mtd: st_spi_fsm: General tidy-up
> > ARM: STi: stih416: Use new platform specific compatible string
> > ARM: STi: stih416: Supply EMI clock reference to FSM SPI NOR
> >
> > Nunzio Raciti (1):
> > mtd: st_spi_fsm: Add support for Micron N25Q512A
> >
> > Documentation/devicetree/bindings/mtd/st-fsm.txt | 20 +-
> > arch/arm/boot/dts/stih416.dtsi | 6 +-
> > drivers/mtd/devices/st_spi_fsm.c | 747 +++++++++++++++--------
> > include/dt-bindings/clock/stih416-clks.h | 1 +
> > 4 files changed, 519 insertions(+), 255 deletions(-)
> >
>
> Brian
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 04/13] mtd: st_spi_fsm: Fetch boot device locations from DT match tables
2015-01-13 4:55 ` [PATCH v3 04/13] mtd: st_spi_fsm: Fetch boot device locations from DT match tables Brian Norris
@ 2015-01-21 12:56 ` Lee Jones
0 siblings, 0 replies; 6+ messages in thread
From: Lee Jones @ 2015-01-21 12:56 UTC (permalink / raw)
To: Brian Norris
Cc: linux-arm-kernel, linux-kernel, linux-mtd, kernel, devicetree
On Mon, 12 Jan 2015, Brian Norris wrote:
> + devicetree
>
> On Mon, Dec 15, 2014 at 11:59:11AM +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.
> >
> > 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.
>
> I'm not sure how this agreement was done, but by default, we expect not
> to break backwards compatibility. This patch does just that.
>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> > drivers/mtd/devices/st_spi_fsm.c | 75 ++++++++++++++++++++++++++++++----------
> > 1 file changed, 57 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> > index fac0fe9..d82394a 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,23 @@ static int stfsm_init(struct stfsm *fsm)
> > return 0;
> > }
> >
> > +static const struct of_device_id stfsm_match[] = {
> > + { .compatible = "st,spi-fsm" }, /* DEPRECATED */
> > + { .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 +2046,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);
>
>
> ^^^ NULL pointer dereference for any existing DTB that uses the
> "st,spi-fsm" compatible string. So I'll NAK this patch as-is. Either we
> need to completely drop the "deprecated" compatible property (what's the
> point binding to it, if we're going to OOPS on it anyway), or else we
> need to maintain both methods (at least for whatever deprecation period
> is deemed acceptable).
Great spot Brian.
So, as I explained in reply to the cover-letter, this driver isn't
being used in anger yet. As the driver is still considered
in-progress I take the view that it's perfectly okay to remove the old
compatible string and not match on it at all.
I'll make the necessary changes and re-submit.
> > 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 +2201,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,
>
> Brian
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 00/13] mtd: st_spi_fsm: Align with ST's internal development
2015-01-21 12:49 ` Lee Jones
@ 2015-02-06 1:59 ` Brian Norris
0 siblings, 0 replies; 6+ messages in thread
From: Brian Norris @ 2015-02-06 1:59 UTC (permalink / raw)
To: Lee Jones
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kernel-F5mvAk5X5gdBDgjK7y7TUQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
Rob Herring, Mark Rutland
On Wed, Jan 21, 2015 at 12:49:30PM +0000, Lee Jones wrote:
> On Mon, 12 Jan 2015, Brian Norris wrote:
> > Typically "simpler" and "reducing the cost of having lots of extra DT
> > properties" aren't good enough reasons for immediately breaking an
> > existing DT binding. We usually expect to support both for some period
> > of time.
>
> Only once a binding has became ABI. As there are no users of this
> driver yet (this will change after this sync'ing patch-set has been
> applied), so we can safely consider this binding to be in-progress and
> as such, not yet ABI, thus we can do this ol' switch-aroo without the
> usual 6 month deprecation period.
OK, fine with me.
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-06 1:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1418644760-18773-1-git-send-email-lee.jones@linaro.org>
[not found] ` <1418644760-18773-5-git-send-email-lee.jones@linaro.org>
[not found] ` <1418644760-18773-5-git-send-email-lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2015-01-13 4:55 ` [PATCH v3 04/13] mtd: st_spi_fsm: Fetch boot device locations from DT match tables Brian Norris
2015-01-21 12:56 ` Lee Jones
[not found] ` <1418644760-18773-4-git-send-email-lee.jones@linaro.org>
[not found] ` <1418644760-18773-4-git-send-email-lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2015-01-13 5:02 ` [PATCH v3 03/13] mtd: st_spi_fsm: dt-bindings: Deprecate generic compatible string Brian Norris
[not found] ` <1418644760-18773-1-git-send-email-lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2015-01-13 5:14 ` [PATCH v3 00/13] mtd: st_spi_fsm: Align with ST's internal development Brian Norris
2015-01-21 12:49 ` Lee Jones
2015-02-06 1:59 ` Brian Norris
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).