| Message ID | 1421853868-8262-3-git-send-email-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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. ;)
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,
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@vger.kernel.org Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mtd/devices/st_spi_fsm.c | 74 ++++++++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 18 deletions(-)