| Message ID | 20191004115919.20788-1-clg@kaod.org |
|---|---|
| Headers | show |
| Series | mtd: spi-nor: aspeed: AST2600 support and extensions | expand |
Hi Cedric, On Fri, 4 Oct 2019 13:59:03 +0200 Cédric Le Goater <clg@kaod.org> wrote: > Hello, > > This series first extends the support for the Aspeed AST2500 and > AST2400 SMC driver. It adds Dual Data support and read training giving > the best read settings for a given chip. Support for the new AST2600 > SoC is added at the end. > > I understand that a new spi_mem framework exists and I do have an > experimental driver using it. But unfortunately, it is difficult to > integrate the read training. The Aspeed constraints are not compatible > and i haven't had the time to extend the current framework. Hm, I don't think that's a good reason to push new features to the existing driver, especially since I asked others to migrate their drivers to spi-mem in the past. I do understand your concerns, and I'll let the SPI NOR/MTD maintainers make the final call, but I think it'd be better for the SPI MEM ecosystem to think about this link-training API (Vignesh needs it for the Cadence driver IIRC) rather than pushing this kind of feature to spi-nor controller drivers. > > This patchset has been in use for some time in the OpenBMC kernel on > these systems : > > * OpenPOWER Palmetto (AST2400) > * Evaluation board (AST2500) > * OpenPOWER Witherspoon (AST2500) > * OpenPOWER Romulus (AST2500) > * OpenPOWER Zaius (AST2500) > and many others > > and it is now in use on these boards with the new SoC : > > * Evaluation board (AST2600) > * Tacoma board (AST2600) > > Thanks, > > C. > > Alexander Soldatov (1): > mtd: spi-nor: fix options for mx66l51235f > > Cédric Le Goater (15): > mtd: spi-nor: aspeed: Use command mode for reads > mtd: spi-nor: aspeed: Add support for SPI dual IO read mode > mtd: spi-nor: aspeed: Link controller with the ahb clock > mtd: spi-nor: aspeed: Add read training > mtd: spi-nor: aspeed: Limit the maximum SPI frequency > mtd: spi-nor: aspeed: Add support for the 4B opcodes > mtd: spi-nor: Add support for w25q512jv > mtd: spi-nor: aspeed: Introduce a field for the AHB physical address > mtd: spi-nor: aspeed: Introduce segment operations > dt-bindings: mtd: aspeed-smc: Add new comptatible for AST2600 > mtd: spi-nor: aspeed: Add initial support for the AST2600 > mtd: spi-nor: aspeed: Check for disabled segments on the AST2600 > mtd: spi-nor: aspeed: Introduce training operations per platform > mtd: spi-nor: aspeed: Introduce a HCLK mask for training > mtd: spi-nor: aspeed: Add read training support for the AST2600 > > drivers/mtd/spi-nor/aspeed-smc.c | 593 ++++++++++++++++-- > drivers/mtd/spi-nor/spi-nor.c | 5 +- > .../devicetree/bindings/mtd/aspeed-smc.txt | 2 + > 3 files changed, 551 insertions(+), 49 deletions(-) >
On Wed, 9 Oct 2019 at 20:56, Boris Brezillon <boris.brezillon@collabora.com> wrote: > > Hi Cedric, > > On Fri, 4 Oct 2019 13:59:03 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > > > Hello, > > > > This series first extends the support for the Aspeed AST2500 and > > AST2400 SMC driver. It adds Dual Data support and read training giving > > the best read settings for a given chip. Support for the new AST2600 > > SoC is added at the end. > > > > I understand that a new spi_mem framework exists and I do have an > > experimental driver using it. But unfortunately, it is difficult to > > integrate the read training. The Aspeed constraints are not compatible > > and i haven't had the time to extend the current framework. > > Hm, I don't think that's a good reason to push new features to the > existing driver, especially since I asked others to migrate their > drivers to spi-mem in the past. I do understand your concerns, and I'll > let the SPI NOR/MTD maintainers make the final call, but I think it'd > be better for the SPI MEM ecosystem to think about this link-training > API (Vignesh needs it for the Cadence driver IIRC) rather than pushing > this kind of feature to spi-nor controller drivers. As Cedric mentioned, the OpenBMC project has been shipping the read training code for the ast2400/ast2400 for several years now. It would be great to see it in mainline. I think it's reasonable to ask for the driver to be moved to the spi-mem subsystem once it has the required APIs. Cheers, Joel > > > > > This patchset has been in use for some time in the OpenBMC kernel on > > these systems : > > > > * OpenPOWER Palmetto (AST2400) > > * Evaluation board (AST2500) > > * OpenPOWER Witherspoon (AST2500) > > * OpenPOWER Romulus (AST2500) > > * OpenPOWER Zaius (AST2500) > > and many others > > > > and it is now in use on these boards with the new SoC : > > > > * Evaluation board (AST2600) > > * Tacoma board (AST2600) > > > > Thanks, > > > > C. > > > > Alexander Soldatov (1): > > mtd: spi-nor: fix options for mx66l51235f > > > > Cédric Le Goater (15): > > mtd: spi-nor: aspeed: Use command mode for reads > > mtd: spi-nor: aspeed: Add support for SPI dual IO read mode > > mtd: spi-nor: aspeed: Link controller with the ahb clock > > mtd: spi-nor: aspeed: Add read training > > mtd: spi-nor: aspeed: Limit the maximum SPI frequency > > mtd: spi-nor: aspeed: Add support for the 4B opcodes > > mtd: spi-nor: Add support for w25q512jv > > mtd: spi-nor: aspeed: Introduce a field for the AHB physical address > > mtd: spi-nor: aspeed: Introduce segment operations > > dt-bindings: mtd: aspeed-smc: Add new comptatible for AST2600 > > mtd: spi-nor: aspeed: Add initial support for the AST2600 > > mtd: spi-nor: aspeed: Check for disabled segments on the AST2600 > > mtd: spi-nor: aspeed: Introduce training operations per platform > > mtd: spi-nor: aspeed: Introduce a HCLK mask for training > > mtd: spi-nor: aspeed: Add read training support for the AST2600 > > > > drivers/mtd/spi-nor/aspeed-smc.c | 593 ++++++++++++++++-- > > drivers/mtd/spi-nor/spi-nor.c | 5 +- > > .../devicetree/bindings/mtd/aspeed-smc.txt | 2 + > > 3 files changed, 551 insertions(+), 49 deletions(-) > > >
On Thu, 10 Oct 2019 23:47:45 +0000 Joel Stanley <joel@jms.id.au> wrote: > On Wed, 9 Oct 2019 at 20:56, Boris Brezillon > <boris.brezillon@collabora.com> wrote: > > > > Hi Cedric, > > > > On Fri, 4 Oct 2019 13:59:03 +0200 > > Cédric Le Goater <clg@kaod.org> wrote: > > > > > Hello, > > > > > > This series first extends the support for the Aspeed AST2500 and > > > AST2400 SMC driver. It adds Dual Data support and read training giving > > > the best read settings for a given chip. Support for the new AST2600 > > > SoC is added at the end. > > > > > > I understand that a new spi_mem framework exists and I do have an > > > experimental driver using it. But unfortunately, it is difficult to > > > integrate the read training. The Aspeed constraints are not compatible > > > and i haven't had the time to extend the current framework. > > > > Hm, I don't think that's a good reason to push new features to the > > existing driver, especially since I asked others to migrate their > > drivers to spi-mem in the past. I do understand your concerns, and I'll > > let the SPI NOR/MTD maintainers make the final call, but I think it'd > > be better for the SPI MEM ecosystem to think about this link-training > > API (Vignesh needs it for the Cadence driver IIRC) rather than pushing > > this kind of feature to spi-nor controller drivers. > > As Cedric mentioned, the OpenBMC project has been shipping the read > training code for the ast2400/ast2400 for several years now. It would > be great to see it in mainline. > > I think it's reasonable to ask for the driver to be moved to the > spi-mem subsystem once it has the required APIs. Except it won't have the necessary APIs unless someone works on it, and adding this feature to existing spi-nor drivers won't help achieving this goal.
On 11/10/2019 08:45, Boris Brezillon wrote: > On Thu, 10 Oct 2019 23:47:45 +0000 > Joel Stanley <joel@jms.id.au> wrote: > >> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon >> <boris.brezillon@collabora.com> wrote: >>> >>> Hi Cedric, >>> >>> On Fri, 4 Oct 2019 13:59:03 +0200 >>> Cédric Le Goater <clg@kaod.org> wrote: >>> >>>> Hello, >>>> >>>> This series first extends the support for the Aspeed AST2500 and >>>> AST2400 SMC driver. It adds Dual Data support and read training giving >>>> the best read settings for a given chip. Support for the new AST2600 >>>> SoC is added at the end. >>>> >>>> I understand that a new spi_mem framework exists and I do have an >>>> experimental driver using it. But unfortunately, it is difficult to >>>> integrate the read training. The Aspeed constraints are not compatible >>>> and i haven't had the time to extend the current framework. >>> >>> Hm, I don't think that's a good reason to push new features to the >>> existing driver, especially since I asked others to migrate their >>> drivers to spi-mem in the past. I do understand your concerns, and I'll >>> let the SPI NOR/MTD maintainers make the final call, but I think it'd >>> be better for the SPI MEM ecosystem to think about this link-training >>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing >>> this kind of feature to spi-nor controller drivers. >> >> As Cedric mentioned, the OpenBMC project has been shipping the read >> training code for the ast2400/ast2400 for several years now. It would >> be great to see it in mainline. >> >> I think it's reasonable to ask for the driver to be moved to the >> spi-mem subsystem once it has the required APIs. > > Except it won't have the necessary APIs unless someone works on it, and > adding this feature to existing spi-nor drivers won't help achieving > this goal. What would you suggest ? Something like the patch below which would call a 'train' operation at the end of spi_add_device(). Also, when doing read training, we might need to know some lowlevel characteristics of the chip being trained. Should we offer a way to grab the probed m25p80 device and give access to the underlying 'struct spi_nor' ? static struct spi_nor *spi_get_pnor(struct spi_device *spi) { struct spi_mem *spimem = spi_get_drvdata(spi); struct m25p *flash = spi_mem_get_drvdata(spimem); return flash ? &flash->spi_nor : NULL; } Yeah, it's hideous. I just want to raise the issue. Thanks, C. From b34297e6b991ff051bc1e16103d14b2a05c81827 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org> Date: Fri, 11 Oct 2019 11:09:33 +0200 Subject: [PATCH] spi: core: Add a device link training operation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/linux/spi/spi.h | 4 ++++ drivers/spi/spi.c | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index af4f265d0f67..950b39304807 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -409,6 +409,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) * @fw_translate_cs: If the boot firmware uses different numbering scheme * what Linux expects, this optional hook can be used to translate * between the two. + * @train : perform device link training * * Each SPI controller can communicate with one or more @spi_device * children. These make a small bus, sharing MOSI, MISO and SCK signals @@ -604,6 +605,9 @@ struct spi_controller { void *dummy_tx; int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs); + + int (*train)(struct spi_device *spi); + }; static inline void *spi_controller_get_devdata(struct spi_controller *ctlr) diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 75ac046cae52..759a66d74822 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -542,6 +542,22 @@ static int spi_dev_check(struct device *dev, void *data) return 0; } +/** + * spi_train - link training of SPI device + * @spi: the device whose being trained + * + * Return: zero on success, else a negative error code. + */ +static int spi_train(struct spi_device *spi) +{ + int status = 0; + + if (spi->controller->train) + status = spi->controller->train(spi); + + return status; +} + /** * spi_add_device - Add spi_device allocated with spi_alloc_device * @spi: spi_device to register @@ -606,6 +622,13 @@ int spi_add_device(struct spi_device *spi) else dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev)); + status = spi_train(spi); + if (status < 0) { + dev_err(dev, "can't train %s, status %d\n", + dev_name(&spi->dev), status); + goto done; + } + done: mutex_unlock(&spi_add_lock); return status;
On Fri, 11 Oct 2019 11:29:49 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 11/10/2019 08:45, Boris Brezillon wrote: > > On Thu, 10 Oct 2019 23:47:45 +0000 > > Joel Stanley <joel@jms.id.au> wrote: > > > >> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon > >> <boris.brezillon@collabora.com> wrote: > >>> > >>> Hi Cedric, > >>> > >>> On Fri, 4 Oct 2019 13:59:03 +0200 > >>> Cédric Le Goater <clg@kaod.org> wrote: > >>> > >>>> Hello, > >>>> > >>>> This series first extends the support for the Aspeed AST2500 and > >>>> AST2400 SMC driver. It adds Dual Data support and read training giving > >>>> the best read settings for a given chip. Support for the new AST2600 > >>>> SoC is added at the end. > >>>> > >>>> I understand that a new spi_mem framework exists and I do have an > >>>> experimental driver using it. But unfortunately, it is difficult to > >>>> integrate the read training. The Aspeed constraints are not compatible > >>>> and i haven't had the time to extend the current framework. > >>> > >>> Hm, I don't think that's a good reason to push new features to the > >>> existing driver, especially since I asked others to migrate their > >>> drivers to spi-mem in the past. I do understand your concerns, and I'll > >>> let the SPI NOR/MTD maintainers make the final call, but I think it'd > >>> be better for the SPI MEM ecosystem to think about this link-training > >>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing > >>> this kind of feature to spi-nor controller drivers. > >> > >> As Cedric mentioned, the OpenBMC project has been shipping the read > >> training code for the ast2400/ast2400 for several years now. It would > >> be great to see it in mainline. > >> > >> I think it's reasonable to ask for the driver to be moved to the > >> spi-mem subsystem once it has the required APIs. > > > > Except it won't have the necessary APIs unless someone works on it, and > > adding this feature to existing spi-nor drivers won't help achieving > > this goal. > > > What would you suggest ? Something like the patch below which would > call a 'train' operation at the end of spi_add_device(). This has been discussed in the past with Vignesh, but I can't find the thread where this discussion happened. My understanding was that link training would use a command with well-known output (is there a dedicated SPI NOR command for that?) and test different clk settings until it finds one that works. > > Also, when doing read training, we might need to know some lowlevel > characteristics of the chip being trained. Should we offer a way > to grab the probed m25p80 device and give access to the underlying > 'struct spi_nor' ? > > static struct spi_nor *spi_get_pnor(struct spi_device *spi) > { > struct spi_mem *spimem = spi_get_drvdata(spi); > struct m25p *flash = spi_mem_get_drvdata(spimem); > > return flash ? &flash->spi_nor : NULL; > } > > Yeah, it's hideous. I just want to raise the issue. Oh no. We definitely don't want to expose the spi_nor chip to the spi_mem layer, but, if needed, we can add more fields to spi_mem and let the spi_mem driver fill them. We just need to figure out what's really needed. > > Thanks, > > C. > > > From b34297e6b991ff051bc1e16103d14b2a05c81827 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org> > Date: Fri, 11 Oct 2019 11:09:33 +0200 > Subject: [PATCH] spi: core: Add a device link training operation > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > include/linux/spi/spi.h | 4 ++++ > drivers/spi/spi.c | 23 +++++++++++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index af4f265d0f67..950b39304807 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -409,6 +409,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) > * @fw_translate_cs: If the boot firmware uses different numbering scheme > * what Linux expects, this optional hook can be used to translate > * between the two. > + * @train : perform device link training > * > * Each SPI controller can communicate with one or more @spi_device > * children. These make a small bus, sharing MOSI, MISO and SCK signals > @@ -604,6 +605,9 @@ struct spi_controller { > void *dummy_tx; > > int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs); > + > + int (*train)(struct spi_device *spi); Was more thinking of something like: int (*link_setup)(struct spi_mem *mem, struct spi_mem_op *op_template, ...); where the op_template would potentially differ depending on the type of memory (NOR, NAND, SRAM?). I also don't know what other params would be needed to do the link training. BTW, this hook should be in the spi_mem_controller_ops struct not in spi_controller. > + > }; > > static inline void *spi_controller_get_devdata(struct spi_controller *ctlr) > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 75ac046cae52..759a66d74822 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -542,6 +542,22 @@ static int spi_dev_check(struct device *dev, void *data) > return 0; > } > > +/** > + * spi_train - link training of SPI device > + * @spi: the device whose being trained > + * > + * Return: zero on success, else a negative error code. > + */ > +static int spi_train(struct spi_device *spi) > +{ > + int status = 0; > + > + if (spi->controller->train) > + status = spi->controller->train(spi); > + > + return status; > +} > + > /** > * spi_add_device - Add spi_device allocated with spi_alloc_device > * @spi: spi_device to register > @@ -606,6 +622,13 @@ int spi_add_device(struct spi_device *spi) > else > dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev)); > > + status = spi_train(spi); > + if (status < 0) { > + dev_err(dev, "can't train %s, status %d\n", > + dev_name(&spi->dev), status); > + goto done; > + } > + > done: > mutex_unlock(&spi_add_lock); > return status;
On 11/10/2019 11:51, Boris Brezillon wrote: > On Fri, 11 Oct 2019 11:29:49 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > >> On 11/10/2019 08:45, Boris Brezillon wrote: >>> On Thu, 10 Oct 2019 23:47:45 +0000 >>> Joel Stanley <joel@jms.id.au> wrote: >>> >>>> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon >>>> <boris.brezillon@collabora.com> wrote: >>>>> >>>>> Hi Cedric, >>>>> >>>>> On Fri, 4 Oct 2019 13:59:03 +0200 >>>>> Cédric Le Goater <clg@kaod.org> wrote: >>>>> >>>>>> Hello, >>>>>> >>>>>> This series first extends the support for the Aspeed AST2500 and >>>>>> AST2400 SMC driver. It adds Dual Data support and read training giving >>>>>> the best read settings for a given chip. Support for the new AST2600 >>>>>> SoC is added at the end. >>>>>> >>>>>> I understand that a new spi_mem framework exists and I do have an >>>>>> experimental driver using it. But unfortunately, it is difficult to >>>>>> integrate the read training. The Aspeed constraints are not compatible >>>>>> and i haven't had the time to extend the current framework. >>>>> >>>>> Hm, I don't think that's a good reason to push new features to the >>>>> existing driver, especially since I asked others to migrate their >>>>> drivers to spi-mem in the past. I do understand your concerns, and I'll >>>>> let the SPI NOR/MTD maintainers make the final call, but I think it'd >>>>> be better for the SPI MEM ecosystem to think about this link-training >>>>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing >>>>> this kind of feature to spi-nor controller drivers. >>>> >>>> As Cedric mentioned, the OpenBMC project has been shipping the read >>>> training code for the ast2400/ast2400 for several years now. It would >>>> be great to see it in mainline. >>>> >>>> I think it's reasonable to ask for the driver to be moved to the >>>> spi-mem subsystem once it has the required APIs. >>> >>> Except it won't have the necessary APIs unless someone works on it, and >>> adding this feature to existing spi-nor drivers won't help achieving >>> this goal. >> >> >> What would you suggest ? Something like the patch below which would >> call a 'train' operation at the end of spi_add_device(). > > This has been discussed in the past with Vignesh, but I can't find the > thread where this discussion happened. My understanding was that link > training would use a command with well-known output (is there a > dedicated SPI NOR command for that?) and test different clk settings > until it finds one that works. The read training on Aspeed consists of finding the appropriate read timing delays for well-known HCLK dividers and store the results in the Read Timing Compensation register. We first read a golden buffer at low speed and then perform reads with different clocks and delay cycle settings to find a breaking point. This selects the default clock frequency for the CEx control register. >> Also, when doing read training, we might need to know some lowlevel >> characteristics of the chip being trained. Should we offer a way >> to grab the probed m25p80 device and give access to the underlying >> 'struct spi_nor' ? >> >> static struct spi_nor *spi_get_pnor(struct spi_device *spi) >> { >> struct spi_mem *spimem = spi_get_drvdata(spi); >> struct m25p *flash = spi_mem_get_drvdata(spimem); >> >> return flash ? &flash->spi_nor : NULL; >> } >> >> Yeah, it's hideous. I just want to raise the issue. > > Oh no. We definitely don't want to expose the spi_nor chip to the > spi_mem layer, but, if needed, we can add more fields to spi_mem and > let the spi_mem driver fill them. We just need to figure out what's > really needed. We need the SPI/NOR flash characteristics for link training and its size to configure the controller to access the CS on the AHB window. [ ... ] >> int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs); >> + >> + int (*train)(struct spi_device *spi); > > Was more thinking of something like: > > int (*link_setup)(struct spi_mem *mem, > struct spi_mem_op *op_template, > ...); > > where the op_template would potentially differ depending on the type of > memory (NOR, NAND, SRAM?). I also don't know what other params would be > needed to do the link training. The Aspeed controller needs to set read delay timings at different HCLK settings. It's doing read operations with the default IO mode. > BTW, this hook should be in the spi_mem_controller_ops struct not in > spi_controller. ok. Let's wait for feedback from Vinesh. Thanks, C.
On Fri, 11 Oct 2019 13:47:53 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 11/10/2019 11:51, Boris Brezillon wrote: > > On Fri, 11 Oct 2019 11:29:49 +0200 > > Cédric Le Goater <clg@kaod.org> wrote: > > > >> On 11/10/2019 08:45, Boris Brezillon wrote: > >>> On Thu, 10 Oct 2019 23:47:45 +0000 > >>> Joel Stanley <joel@jms.id.au> wrote: > >>> > >>>> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon > >>>> <boris.brezillon@collabora.com> wrote: > >>>>> > >>>>> Hi Cedric, > >>>>> > >>>>> On Fri, 4 Oct 2019 13:59:03 +0200 > >>>>> Cédric Le Goater <clg@kaod.org> wrote: > >>>>> > >>>>>> Hello, > >>>>>> > >>>>>> This series first extends the support for the Aspeed AST2500 and > >>>>>> AST2400 SMC driver. It adds Dual Data support and read training giving > >>>>>> the best read settings for a given chip. Support for the new AST2600 > >>>>>> SoC is added at the end. > >>>>>> > >>>>>> I understand that a new spi_mem framework exists and I do have an > >>>>>> experimental driver using it. But unfortunately, it is difficult to > >>>>>> integrate the read training. The Aspeed constraints are not compatible > >>>>>> and i haven't had the time to extend the current framework. > >>>>> > >>>>> Hm, I don't think that's a good reason to push new features to the > >>>>> existing driver, especially since I asked others to migrate their > >>>>> drivers to spi-mem in the past. I do understand your concerns, and I'll > >>>>> let the SPI NOR/MTD maintainers make the final call, but I think it'd > >>>>> be better for the SPI MEM ecosystem to think about this link-training > >>>>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing > >>>>> this kind of feature to spi-nor controller drivers. > >>>> > >>>> As Cedric mentioned, the OpenBMC project has been shipping the read > >>>> training code for the ast2400/ast2400 for several years now. It would > >>>> be great to see it in mainline. > >>>> > >>>> I think it's reasonable to ask for the driver to be moved to the > >>>> spi-mem subsystem once it has the required APIs. > >>> > >>> Except it won't have the necessary APIs unless someone works on it, and > >>> adding this feature to existing spi-nor drivers won't help achieving > >>> this goal. > >> > >> > >> What would you suggest ? Something like the patch below which would > >> call a 'train' operation at the end of spi_add_device(). > > > > This has been discussed in the past with Vignesh, but I can't find the > > thread where this discussion happened. My understanding was that link > > training would use a command with well-known output (is there a > > dedicated SPI NOR command for that?) and test different clk settings > > until it finds one that works. > > The read training on Aspeed consists of finding the appropriate read > timing delays for well-known HCLK dividers and store the results in > the Read Timing Compensation register. > > We first read a golden buffer at low speed and then perform reads with > different clocks and delay cycle settings to find a breaking point. Where does this golden buffer come from? Do you have a specific command to access it? Is it a NOR section with well-known data? > This > selects the default clock frequency for the CEx control register. > > > >> Also, when doing read training, we might need to know some lowlevel > >> characteristics of the chip being trained. Should we offer a way > >> to grab the probed m25p80 device and give access to the underlying > >> 'struct spi_nor' ? > >> > >> static struct spi_nor *spi_get_pnor(struct spi_device *spi) > >> { > >> struct spi_mem *spimem = spi_get_drvdata(spi); > >> struct m25p *flash = spi_mem_get_drvdata(spimem); > >> > >> return flash ? &flash->spi_nor : NULL; > >> } > >> > >> Yeah, it's hideous. I just want to raise the issue. > > > > Oh no. We definitely don't want to expose the spi_nor chip to the > > spi_mem layer, but, if needed, we can add more fields to spi_mem and > > let the spi_mem driver fill them. We just need to figure out what's > > really needed. > > We need the SPI/NOR flash characteristics for link training and its > size to configure the controller to access the CS on the AHB window. We managed to deal with direct mapping without having to explicitly pass the NOR size (we just pass the size of the direct mapping we want to create). What do you need the size for? Is it just to configure the AHB window for the link-training stuff? If that's the case, I guess this can be part of the op_template I was talking about, or maybe passed as extra parameters. > > [ ... ] > > >> int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs); > >> + > >> + int (*train)(struct spi_device *spi); > > > > Was more thinking of something like: > > > > int (*link_setup)(struct spi_mem *mem, > > struct spi_mem_op *op_template, > > ...); > > > > where the op_template would potentially differ depending on the type of > > memory (NOR, NAND, SRAM?). I also don't know what other params would be > > needed to do the link training. > > The Aspeed controller needs to set read delay timings at different HCLK > settings. It's doing read operations with the default IO mode. So you need information about the theoretical read delay so you can adjust them, right. I guess that's something we can pass to the ->link_setup() hook. > > > BTW, this hook should be in the spi_mem_controller_ops struct not in > > spi_controller. > > ok. Let's wait for feedback from Vinesh. Thanks for starting this discussion. Boris
On 11/10/2019 14:07, Boris Brezillon wrote: > On Fri, 11 Oct 2019 13:47:53 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > >> On 11/10/2019 11:51, Boris Brezillon wrote: >>> On Fri, 11 Oct 2019 11:29:49 +0200 >>> Cédric Le Goater <clg@kaod.org> wrote: >>> >>>> On 11/10/2019 08:45, Boris Brezillon wrote: >>>>> On Thu, 10 Oct 2019 23:47:45 +0000 >>>>> Joel Stanley <joel@jms.id.au> wrote: >>>>> >>>>>> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon >>>>>> <boris.brezillon@collabora.com> wrote: >>>>>>> >>>>>>> Hi Cedric, >>>>>>> >>>>>>> On Fri, 4 Oct 2019 13:59:03 +0200 >>>>>>> Cédric Le Goater <clg@kaod.org> wrote: >>>>>>> >>>>>>>> Hello, >>>>>>>> >>>>>>>> This series first extends the support for the Aspeed AST2500 and >>>>>>>> AST2400 SMC driver. It adds Dual Data support and read training giving >>>>>>>> the best read settings for a given chip. Support for the new AST2600 >>>>>>>> SoC is added at the end. >>>>>>>> >>>>>>>> I understand that a new spi_mem framework exists and I do have an >>>>>>>> experimental driver using it. But unfortunately, it is difficult to >>>>>>>> integrate the read training. The Aspeed constraints are not compatible >>>>>>>> and i haven't had the time to extend the current framework. >>>>>>> >>>>>>> Hm, I don't think that's a good reason to push new features to the >>>>>>> existing driver, especially since I asked others to migrate their >>>>>>> drivers to spi-mem in the past. I do understand your concerns, and I'll >>>>>>> let the SPI NOR/MTD maintainers make the final call, but I think it'd >>>>>>> be better for the SPI MEM ecosystem to think about this link-training >>>>>>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing >>>>>>> this kind of feature to spi-nor controller drivers. >>>>>> >>>>>> As Cedric mentioned, the OpenBMC project has been shipping the read >>>>>> training code for the ast2400/ast2400 for several years now. It would >>>>>> be great to see it in mainline. >>>>>> >>>>>> I think it's reasonable to ask for the driver to be moved to the >>>>>> spi-mem subsystem once it has the required APIs. >>>>> >>>>> Except it won't have the necessary APIs unless someone works on it, and >>>>> adding this feature to existing spi-nor drivers won't help achieving >>>>> this goal. >>>> >>>> >>>> What would you suggest ? Something like the patch below which would >>>> call a 'train' operation at the end of spi_add_device(). >>> >>> This has been discussed in the past with Vignesh, but I can't find the >>> thread where this discussion happened. My understanding was that link >>> training would use a command with well-known output (is there a >>> dedicated SPI NOR command for that?) and test different clk settings >>> until it finds one that works. >> >> The read training on Aspeed consists of finding the appropriate read >> timing delays for well-known HCLK dividers and store the results in >> the Read Timing Compensation register. >> >> We first read a golden buffer at low speed and then perform reads with >> different clocks and delay cycle settings to find a breaking point. > > Where does this golden buffer come from? Do you have a specific command > to access it? Is it a NOR section with well-known data? We read the first 16K of the flash device at low speed. >> This >> selects the default clock frequency for the CEx control register. >> >> >>>> Also, when doing read training, we might need to know some lowlevel >>>> characteristics of the chip being trained. Should we offer a way >>>> to grab the probed m25p80 device and give access to the underlying >>>> 'struct spi_nor' ? >>>> >>>> static struct spi_nor *spi_get_pnor(struct spi_device *spi) >>>> { >>>> struct spi_mem *spimem = spi_get_drvdata(spi); >>>> struct m25p *flash = spi_mem_get_drvdata(spimem); >>>> >>>> return flash ? &flash->spi_nor : NULL; >>>> } >>>> >>>> Yeah, it's hideous. I just want to raise the issue. >>> >>> Oh no. We definitely don't want to expose the spi_nor chip to the >>> spi_mem layer, but, if needed, we can add more fields to spi_mem and >>> let the spi_mem driver fill them. We just need to figure out what's >>> really needed. >> >> We need the SPI/NOR flash characteristics for link training and its >> size to configure the controller to access the CS on the AHB window. > > We managed to deal with direct mapping without having to explicitly pass > the NOR size (we just pass the size of the direct mapping we want to > create). What do you need the size for? because the AHB window on which are mapped all CS is segmented. There is one sub window for each CS and all segments need to be configured in the controller (in the segment registers). In case of a bad value or an overlap, you can loose access to the CS or hang the system. To access the SPI NOR registers, you don't need a window of the exact size because in that case the controller operates in 'User' command mode and the window only needs to be partially opened. Reads and Writes use the start address of AHB sub-window of the CS. If you want access to the full contents through a direct mapping, the controller operates in 'Read' or 'Fast Read' command mode and the window needs to be at least as wide as the flash size. > Is it just to configure the AHB window for the link-training stuff? The link-training uses the direct mapping mode but not on the whole contents, only on the first 16K. So the AHB window does not have to be as wide as the chip and we could use a default setting. But generally, we already have good knowledge on the chip before training is started. > If that's the case, I guess this can be part of the op_template > I was talking about, or maybe passed as extra parameters. > >> [ ... ] >> >>>> int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs); >>>> + >>>> + int (*train)(struct spi_device *spi); >>> >>> Was more thinking of something like: >>> >>> int (*link_setup)(struct spi_mem *mem, >>> struct spi_mem_op *op_template, >>> ...); >>> >>> where the op_template would potentially differ depending on the type of >>> memory (NOR, NAND, SRAM?). I also don't know what other params would be >>> needed to do the link training. >> >> The Aspeed controller needs to set read delay timings at different HCLK >> settings. It's doing read operations with the default IO mode. > > So you need information about the theoretical read delay so you can > adjust them, right. I guess that's something we can pass to the > ->link_setup() hook. The read delay settings really depend on the controller. I was thinking more of a post_setup() hook called once in m25p_probe() after the chip is scanned. As for the parameter, we would need some SPI-NOR info or some generic SPI device data that the controller would know how to interpret. Cheers, C. >>> BTW, this hook should be in the spi_mem_controller_ops struct not in >>> spi_controller. >> >> ok. Let's wait for feedback from Vinesh. > > Thanks for starting this discussion. > > Boris >
On Fri, 11 Oct 2019 15:07:18 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 11/10/2019 14:07, Boris Brezillon wrote: > > On Fri, 11 Oct 2019 13:47:53 +0200 > > Cédric Le Goater <clg@kaod.org> wrote: > > > >> On 11/10/2019 11:51, Boris Brezillon wrote: > >>> On Fri, 11 Oct 2019 11:29:49 +0200 > >>> Cédric Le Goater <clg@kaod.org> wrote: > >>> > >>>> On 11/10/2019 08:45, Boris Brezillon wrote: > >>>>> On Thu, 10 Oct 2019 23:47:45 +0000 > >>>>> Joel Stanley <joel@jms.id.au> wrote: > >>>>> > >>>>>> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon > >>>>>> <boris.brezillon@collabora.com> wrote: > >>>>>>> > >>>>>>> Hi Cedric, > >>>>>>> > >>>>>>> On Fri, 4 Oct 2019 13:59:03 +0200 > >>>>>>> Cédric Le Goater <clg@kaod.org> wrote: > >>>>>>> > >>>>>>>> Hello, > >>>>>>>> > >>>>>>>> This series first extends the support for the Aspeed AST2500 and > >>>>>>>> AST2400 SMC driver. It adds Dual Data support and read training giving > >>>>>>>> the best read settings for a given chip. Support for the new AST2600 > >>>>>>>> SoC is added at the end. > >>>>>>>> > >>>>>>>> I understand that a new spi_mem framework exists and I do have an > >>>>>>>> experimental driver using it. But unfortunately, it is difficult to > >>>>>>>> integrate the read training. The Aspeed constraints are not compatible > >>>>>>>> and i haven't had the time to extend the current framework. > >>>>>>> > >>>>>>> Hm, I don't think that's a good reason to push new features to the > >>>>>>> existing driver, especially since I asked others to migrate their > >>>>>>> drivers to spi-mem in the past. I do understand your concerns, and I'll > >>>>>>> let the SPI NOR/MTD maintainers make the final call, but I think it'd > >>>>>>> be better for the SPI MEM ecosystem to think about this link-training > >>>>>>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing > >>>>>>> this kind of feature to spi-nor controller drivers. > >>>>>> > >>>>>> As Cedric mentioned, the OpenBMC project has been shipping the read > >>>>>> training code for the ast2400/ast2400 for several years now. It would > >>>>>> be great to see it in mainline. > >>>>>> > >>>>>> I think it's reasonable to ask for the driver to be moved to the > >>>>>> spi-mem subsystem once it has the required APIs. > >>>>> > >>>>> Except it won't have the necessary APIs unless someone works on it, and > >>>>> adding this feature to existing spi-nor drivers won't help achieving > >>>>> this goal. > >>>> > >>>> > >>>> What would you suggest ? Something like the patch below which would > >>>> call a 'train' operation at the end of spi_add_device(). > >>> > >>> This has been discussed in the past with Vignesh, but I can't find the > >>> thread where this discussion happened. My understanding was that link > >>> training would use a command with well-known output (is there a > >>> dedicated SPI NOR command for that?) and test different clk settings > >>> until it finds one that works. > >> > >> The read training on Aspeed consists of finding the appropriate read > >> timing delays for well-known HCLK dividers and store the results in > >> the Read Timing Compensation register. > >> > >> We first read a golden buffer at low speed and then perform reads with > >> different clocks and delay cycle settings to find a breaking point. > > > > Where does this golden buffer come from? Do you have a specific command > > to access it? Is it a NOR section with well-known data? > > We read the first 16K of the flash device at low speed. That's what I figured after reading the code. > > >> This > >> selects the default clock frequency for the CEx control register. > >> > >> > >>>> Also, when doing read training, we might need to know some lowlevel > >>>> characteristics of the chip being trained. Should we offer a way > >>>> to grab the probed m25p80 device and give access to the underlying > >>>> 'struct spi_nor' ? > >>>> > >>>> static struct spi_nor *spi_get_pnor(struct spi_device *spi) > >>>> { > >>>> struct spi_mem *spimem = spi_get_drvdata(spi); > >>>> struct m25p *flash = spi_mem_get_drvdata(spimem); > >>>> > >>>> return flash ? &flash->spi_nor : NULL; > >>>> } > >>>> > >>>> Yeah, it's hideous. I just want to raise the issue. > >>> > >>> Oh no. We definitely don't want to expose the spi_nor chip to the > >>> spi_mem layer, but, if needed, we can add more fields to spi_mem and > >>> let the spi_mem driver fill them. We just need to figure out what's > >>> really needed. > >> > >> We need the SPI/NOR flash characteristics for link training and its > >> size to configure the controller to access the CS on the AHB window. > > > > We managed to deal with direct mapping without having to explicitly pass > > the NOR size (we just pass the size of the direct mapping we want to > > create). What do you need the size for? > > because the AHB window on which are mapped all CS is segmented. > > There is one sub window for each CS and all segments need to be > configured in the controller (in the segment registers). In case > of a bad value or an overlap, you can loose access to the CS or > hang the system. The dirmap API is here for that. If you implement the ->{create,destroy}_dirmap() hooks you should be able to decide which AHB window is assigned to which chip. The SPI NOR layer will (soon) make use of dirmaps instead of using plain spi_mem_exec_op() for data read/write accesses BTW. > > To access the SPI NOR registers, you don't need a window of the > exact size because in that case the controller operates in 'User' > command mode and the window only needs to be partially opened. Sounds good. > Reads and Writes use the start address of AHB sub-window of the CS. This too. > > If you want access to the full contents through a direct mapping, > the controller operates in 'Read' or 'Fast Read' command mode and > the window needs to be at least as wide as the flash size. Well, we usually don't enforce that in other drivers (we use the concept of sliding window when the flash is bigger than the physical AHB window, and we adjust the start offset dynamically based on the dirmap_read/write() request), but if you want to enforce that in your driver it's fine. > > > Is it just to configure the AHB window for the link-training stuff? > > The link-training uses the direct mapping mode but not on the > whole contents, only on the first 16K. So the AHB window does not > have to be as wide as the chip and we could use a default setting. > But generally, we already have good knowledge on the chip before > training is started. If you need the direct mapping to be set up for the link training, that shouldn't be a problem. > > > If that's the case, I guess this can be part of the op_template > > I was talking about, or maybe passed as extra parameters. > > > >> [ ... ] > >> > >>>> int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs); > >>>> + > >>>> + int (*train)(struct spi_device *spi); > >>> > >>> Was more thinking of something like: > >>> > >>> int (*link_setup)(struct spi_mem *mem, > >>> struct spi_mem_op *op_template, > >>> ...); > >>> > >>> where the op_template would potentially differ depending on the type of > >>> memory (NOR, NAND, SRAM?). I also don't know what other params would be > >>> needed to do the link training. > >> > >> The Aspeed controller needs to set read delay timings at different HCLK > >> settings. It's doing read operations with the default IO mode. > > > > So you need information about the theoretical read delay so you can > > adjust them, right. I guess that's something we can pass to the > > ->link_setup() hook. > > The read delay settings really depend on the controller. I was thinking > more of a post_setup() hook called once in m25p_probe() after the chip > is scanned. For the record, m25p is gone (code has been merged in spi-nor.c) ;-). Back to the original topic, I don't think link setup is something so controller specific that we can't abstract it at the spi-nor level. Of course the piece of HW doing the link training is still the SPI controller, but the SPI NOR framework can abstract a few things, like figuring out which SPI_NOR operation to use, where to read the data from, ... > > As for the parameter, we would need some SPI-NOR info or some generic > SPI device data that the controller would know how to interpret. Yes, that's my point. This part is memory specific. The link training with a SPI NAND is likely to use a different operation than what's used for a SPI NOR, and we definitely don't want SPI controllers to be aware of that.