mbox series

[v2,00/10] spi: spi-mem: Add driver for Aspeed SMC controllers

Message ID 20220302173114.927476-1-clg@kaod.org
Headers show
Series spi: spi-mem: Add driver for Aspeed SMC controllers | expand

Message

Cédric Le Goater March 2, 2022, 5:31 p.m. UTC
Hi,

This series adds a new SPI driver using the spi-mem interface for the
Aspeed static memory controllers of the AST2600, AST2500 and AST2400
SoCs.

 * AST2600 Firmware SPI Memory Controller (FMC)
 * AST2600 SPI Flash Controller (SPI1 and SPI2)
 * AST2500 Firmware SPI Memory Controller (FMC)
 * AST2500 SPI Flash Controller (SPI1 and SPI2)
 * AST2400 New Static Memory Controller (also referred as FMC)
 * AST2400 SPI Flash Controller (SPI)

It is based on the current OpenBMC kernel driver [1], using directly
the MTD SPI-NOR interface and on a patchset [2] previously proposed
adding support for the AST2600 only. This driver takes a slightly
different approach to cover all 6 controllers.

It does not make use of the controller register disabling Address and
Data byte lanes because is not available on the AST2400 SoC. We could
introduce a specific handler for new features available on recent SoCs
if needed. As there is not much difference on performance, the driver
chooses the common denominator: "User mode" which has been heavily
tested in [1]. "User mode" is also used as a fall back method when
flash device mapping window is too small.

Problems to address with spi-mem were the configuration of the mapping
windows and the calibration of the read timings. The driver handles
them in the direct mapping handler when some knowledge on the size of
the flash device is know. It is not perfect but not incorrect either.
The algorithm is one from [1] because it doesn't require the DMA
registers which are not available on all controllers.

Direct mapping for writes is not supported (yet). I have seen some
corruption with writes and I preferred to use the safer and proven
method of the initial driver [1]. We can improve that later.

The driver supports Quad SPI RX transfers on the AST2600 SoC but it
didn't have the expected results. Therefore it is not activated yet.
There are some issues on the pinctrl to investigate first. 

The series does not remove the current Aspeed SMC driver but prepares
ground for its removal by changing its CONFIG option. This last step
can be addressed as a followup when the new driver using the spi-mem
interface has been sufficiently exposed. 

Tested on:
 
 * OpenPOWER Palmetto (AST2400)
 * Facebook Wedge 100 BMC (AST2400) by Tao Ren <rentao.bupt@gmail.com>
 * Evaluation board (AST2500) 
 * Inspur FP5280G2 BMC  (AST2500) by John Wang <wangzq.jn@gmail.com>
 * Facebook Backpack CMM BMC (AST2500) by Tao Ren <rentao.bupt@gmail.com>
 * OpenPOWER Witherspoon (AST2500)
 * Evaluation board (AST2600 A0 and A3)
 * Rainier board (AST2600)
 
[1] https://github.com/openbmc/linux/blob/dev-5.15/drivers/mtd/spi-nor/controllers/aspeed-smc.c
[2] https://patchwork.ozlabs.org/project/linux-aspeed/list/?series=212394

Thanks,

C. 

Changes in v2:

 - Fixed dt_binding_check warnings (Rob)
 - New entry in MAINTAINERS 
 - Addressed Lukas comments regarding the SPI controller registration
   and device removal. Checked with driver bind/unbind   
 - Introduced setup and cleanup handlers and removed routine looping
   on the DT children properties (Pratyush)
 - Clarified in commit log requirements for training.
 - Removed defconfig changes of patch 1 since they were reverted in
   the last patch (Joel)

Cédric Le Goater (10):
  mtd: spi-nor: aspeed: Rename Kconfig option
  ARM: dts: aspeed: Adjust "reg" property of FMC/SPI controllers
  dt-bindings: spi: Add Aspeed SMC controllers device tree binding
  spi: spi-mem: Add driver for Aspeed SMC controllers
  spi: aspeed: Add support for direct mapping
  spi: aspeed: Adjust direct mapping to device size
  spi: aspeed: Workaround AST2500 limitations
  spi: aspeed: Add support for the AST2400 SPI controller
  spi: aspeed: Calibrate read timings
  ARM: dts: aspeed: Enable Dual SPI RX transfers

 drivers/spi/spi-aspeed-smc.c                  | 1186 +++++++++++++++++
 .../bindings/spi/aspeed,ast2600-fmc.yaml      |   90 ++
 MAINTAINERS                                   |   10 +
 arch/arm/boot/dts/aspeed-g4.dtsi              |   12 +-
 arch/arm/boot/dts/aspeed-g5.dtsi              |   16 +-
 arch/arm/boot/dts/aspeed-g6.dtsi              |   17 +-
 drivers/mtd/spi-nor/controllers/Kconfig       |    4 +-
 drivers/mtd/spi-nor/controllers/Makefile      |    2 +-
 drivers/spi/Kconfig                           |   11 +
 drivers/spi/Makefile                          |    1 +
 10 files changed, 1330 insertions(+), 19 deletions(-)
 create mode 100644 drivers/spi/spi-aspeed-smc.c
 create mode 100644 Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml

Comments

Joel Stanley March 2, 2022, 10:16 p.m. UTC | #1
On Wed, 2 Mar 2022 at 17:31, Cédric Le Goater <clg@kaod.org> wrote:
>
> To prepare transition to the new Aspeed SMC SPI controller driver using
> the spi-mem interface, change the kernel CONFIG option of the current
> driver to reflect that the implementation uses the MTD SPI-NOR interface.
> Once the new driver is sufficiently exposed, we should remove the old one.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

As the maintianer of the aspeed systems and the openbmc kernel, I
think this makes the most sense.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  drivers/mtd/spi-nor/controllers/Kconfig  | 2 +-
>  drivers/mtd/spi-nor/controllers/Makefile | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
> index 5c0e0ec2e6d1..876a47042fec 100644
> --- a/drivers/mtd/spi-nor/controllers/Kconfig
> +++ b/drivers/mtd/spi-nor/controllers/Kconfig
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -config SPI_ASPEED_SMC
> +config SPI_ASPEED_SMC_MTD_SPI_NOR
>         tristate "Aspeed flash controllers in SPI mode"
>         depends on ARCH_ASPEED || COMPILE_TEST
>         depends on HAS_IOMEM && OF
> diff --git a/drivers/mtd/spi-nor/controllers/Makefile b/drivers/mtd/spi-nor/controllers/Makefile
> index e7abba491d98..1e28297fb1e8 100644
> --- a/drivers/mtd/spi-nor/controllers/Makefile
> +++ b/drivers/mtd/spi-nor/controllers/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_SPI_ASPEED_SMC)   += aspeed-smc.o
> +obj-$(CONFIG_SPI_ASPEED_SMC_MTD_SPI_NOR)       += aspeed-smc.o
>  obj-$(CONFIG_SPI_HISI_SFC)     += hisi-sfc.o
>  obj-$(CONFIG_SPI_NXP_SPIFI)    += nxp-spifi.o
>  obj-$(CONFIG_SPI_INTEL_SPI)    += intel-spi.o
> --
> 2.34.1
>
Joel Stanley March 2, 2022, 10:20 p.m. UTC | #2
On Wed, 2 Mar 2022 at 17:31, Cédric Le Goater <clg@kaod.org> wrote:
>
> The "interrupt" property is optional because it is only necessary for
> controllers supporting DMAs (Not implemented yet in the new driver).
>
> Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  .../bindings/spi/aspeed,ast2600-fmc.yaml      | 90 +++++++++++++++++++
>  MAINTAINERS                                   |  9 ++
>  2 files changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
>
> diff --git a/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml b/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
> new file mode 100644
> index 000000000000..0289a4f52196
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/aspeed,ast2600-fmc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed SMC controllers bindings
> +
> +maintainers:
> +  - Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> +  - Cédric Le Goater <clg@kaod.org>
> +
> +description: |
> +  This binding describes the Aspeed Static Memory Controllers (FMC and
> +  SPI) of the AST2400, AST2500 and AST2600 SOCs.
> +
> +allOf:
> +  - $ref: "spi-controller.yaml#"
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-fmc
> +      - aspeed,ast2600-spi
> +      - aspeed,ast2500-fmc
> +      - aspeed,ast2500-spi
> +      - aspeed,ast2400-fmc
> +      - aspeed,ast2400-spi
> +
> +  reg:
> +    items:
> +      - description: registers
> +      - description: memory mapping
> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +patternProperties:
> +  "@[0-9a-f]+":
> +    type: object
> +
> +    properties:
> +      spi-rx-bus-width:
> +        enum: [1, 2, 4]
> +
> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/aspeed-scu-ic.h>
> +    #include <dt-bindings/clock/ast2600-clock.h>
> +
> +    spi@1e620000 {
> +        reg = <0x1e620000 0xc4>, <0x20000000 0x10000000>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        compatible = "aspeed,ast2600-fmc";
> +        clocks = <&syscon ASPEED_CLK_AHB>;
> +        interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> +        flash@0 {
> +                reg = < 0 >;
> +                compatible = "jedec,spi-nor";
> +                spi-max-frequency = <50000000>;
> +                spi-rx-bus-width = <2>;
> +        };
> +        flash@1 {
> +                reg = < 1 >;
> +                compatible = "jedec,spi-nor";
> +                spi-max-frequency = <50000000>;
> +                spi-rx-bus-width = <2>;
> +        };
> +        flash@2 {
> +                reg = < 2 >;
> +                compatible = "jedec,spi-nor";
> +                spi-max-frequency = <50000000>;
> +                spi-rx-bus-width = <2>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4175103e928d..f5ab77548ef6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2925,6 +2925,15 @@ S:       Maintained
>  F:     Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
>  F:     drivers/mmc/host/sdhci-of-aspeed*
>
> +ASPEED SMC SPI DRIVER
> +M:     Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> +M:     Cédric Le Goater <clg@kaod.org>
> +L:     linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
> +L:     openbmc@lists.ozlabs.org (moderated for non-subscribers)
> +L:     linux-spi@vger.kernel.org
> +S:     Maintained
> +F:     Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
> +
>  ASPEED VIDEO ENGINE DRIVER
>  M:     Eddie James <eajames@linux.ibm.com>
>  L:     linux-media@vger.kernel.org
> --
> 2.34.1
>
Joel Stanley March 2, 2022, 10:31 p.m. UTC | #3
On Wed, 2 Mar 2022 at 17:31, Cédric Le Goater <clg@kaod.org> wrote:
>
> The segment registers of the FMC/SPI controllers provide a way to
> configure the mapping window of the flash device contents on the AHB
> bus. Adjust this window to the size of the spi-mem mapping.
>
> Things get more complex with multiple devices. The driver needs to
> also adjust the window of the next device to make sure that there is
> no overlap, even if there is no available device. The proposal below
> is not perfect but it is covering all the cases we have seen on
> different boards with one and two devices on the same bus.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  drivers/spi/spi-aspeed-smc.c | 88 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
>
> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
> index b4854b521477..974ab215ec34 100644
> --- a/drivers/spi/spi-aspeed-smc.c
> +++ b/drivers/spi/spi-aspeed-smc.c
> @@ -405,6 +405,92 @@ static int aspeed_spi_chip_set_default_window(struct aspeed_spi_chip *chip)
>         return chip->ahb_window_size ? 0 : -1;
>  }
>
> +static int aspeed_spi_set_window(struct aspeed_spi *aspi,
> +                                const struct aspeed_spi_window *win)
> +{
> +       u32 start = aspi->ahb_base_phy + win->offset;
> +       u32 end = start + win->size;
> +       void __iomem *seg_reg = aspi->regs + CE0_SEGMENT_ADDR_REG + win->cs * 4;
> +       u32 seg_val_backup = readl(seg_reg);
> +       u32 seg_val = aspi->data->segment_reg(aspi, start, end);
> +
> +       if (seg_val == seg_val_backup)
> +               return 0;
> +
> +       writel(seg_val, seg_reg);
> +
> +       /*
> +        * Restore initial value if something goes wrong else we could
> +        * loose access to the chip.
> +        */
> +       if (seg_val != readl(seg_reg)) {
> +               dev_err(aspi->dev, "CE%d invalid window [ 0x%.8x - 0x%.8x ] %dMB",
> +                       win->cs, start, end - 1, win->size >> 20);
> +               writel(seg_val_backup, seg_reg);
> +               return -EIO;
> +       }
> +
> +       if (win->size)
> +               dev_dbg(aspi->dev, "CE%d new window [ 0x%.8x - 0x%.8x ] %dMB",
> +                       win->cs, start, end - 1,  win->size >> 20);
> +       else
> +               dev_dbg(aspi->dev, "CE%d window closed", win->cs);
> +
> +       return 0;
> +}
> +
> +/*
> + * Yet to be done when possible :
> + * - Align mappings on flash size (we don't have the info)
> + * - ioremap each window, not strictly necessary since the overall window
> + *   is correct.
> + */
> +static int aspeed_spi_chip_adjust_window(struct aspeed_spi_chip *chip,
> +                                        u32 local_offset, u32 size)
> +{
> +       struct aspeed_spi *aspi = chip->aspi;
> +       struct aspeed_spi_window windows[ASPEED_SPI_MAX_NUM_CS] = { 0 };
> +       struct aspeed_spi_window *win = &windows[chip->cs];
> +       int ret;
> +
> +       aspeed_spi_get_windows(aspi, windows);
> +
> +       /* Adjust this chip window */
> +       win->offset += local_offset;
> +       win->size = size;
> +
> +       if (win->offset + win->size > aspi->ahb_window_size) {
> +               win->size = aspi->ahb_window_size - win->offset;
> +               dev_warn(aspi->dev, "CE%d window resized to %dMB", chip->cs, win->size >> 20);
> +       }
> +
> +       ret = aspeed_spi_set_window(aspi, win);
> +       if (ret)
> +               return ret;
> +
> +       /* Update chip mapping info */
> +       chip->ahb_base = aspi->ahb_base + win->offset;
> +       chip->ahb_window_size = win->size;
> +
> +       /*
> +        * Also adjust next chip window to make sure that it does not
> +        * overlap with the current window.
> +        */
> +       if (chip->cs < aspi->data->max_cs - 1) {
> +               struct aspeed_spi_window *next = &windows[chip->cs + 1];
> +
> +               /* Change offset and size to keep the same end address */
> +               if ((next->offset + next->size) > (win->offset + win->size))
> +                       next->size = (next->offset + next->size) - (win->offset + win->size);
> +               else
> +                       next->size = 0;
> +               next->offset = win->offset + win->size;
> +
> +               aspeed_spi_set_window(aspi, next);
> +       }
> +       return 0;
> +}
> +
>  static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
>  {
>         struct aspeed_spi *aspi = spi_controller_get_devdata(desc->mem->spi->master);
> @@ -419,6 +505,8 @@ static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
>         if (op->data.dir != SPI_MEM_DATA_IN)
>                 return -EOPNOTSUPP;
>
> +       aspeed_spi_chip_adjust_window(chip, desc->info.offset, desc->info.length);
> +
>         if (desc->info.length > chip->ahb_window_size)
>                 dev_warn(aspi->dev, "CE%d window (%dMB) too small for mapping",
>                          chip->cs, chip->ahb_window_size >> 20);
> --
> 2.34.1
>
Joel Stanley March 2, 2022, 10:48 p.m. UTC | #4
On Wed, 2 Mar 2022 at 17:31, Cédric Le Goater <clg@kaod.org> wrote:
>
> Use direct mapping to read the flash device contents. This operation
> mode is called "Command mode" on Aspeed SoC SMC controllers. It uses a
> Control Register for the settings to apply when a memory operation is
> performed on the flash device mapping window.
>
> If the window is not big enough, fall back to the "User mode" to
> perform the read.
>
> Since direct mapping now handles all reads of the flash device
> contents, also use memcpy_fromio for other address spaces, such as
> SFDP.
>
> Direct mapping for writes will come later when validated.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  drivers/spi/spi-aspeed-smc.c | 67 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
> index 688f9472e0d7..b4854b521477 100644
> --- a/drivers/spi/spi-aspeed-smc.c
> +++ b/drivers/spi/spi-aspeed-smc.c
> @@ -324,8 +324,8 @@ static int do_aspeed_spi_exec_op(struct spi_mem *mem, const struct spi_mem_op *o
>                 if (!op->addr.nbytes)
>                         ret = aspeed_spi_read_reg(chip, op);
>                 else
> -                       ret = aspeed_spi_read_user(chip, op, op->addr.val,
> -                                                  op->data.nbytes, op->data.buf.in);
> +                       memcpy_fromio(op->data.buf.in, chip->ahb_base + op->addr.val,
> +                                     op->data.nbytes);
>         } else {
>                 if (!op->addr.nbytes)
>                         ret = aspeed_spi_write_reg(chip, op);
> @@ -405,10 +405,73 @@ static int aspeed_spi_chip_set_default_window(struct aspeed_spi_chip *chip)
>         return chip->ahb_window_size ? 0 : -1;
>  }
>
> +static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
> +{
> +       struct aspeed_spi *aspi = spi_controller_get_devdata(desc->mem->spi->master);
> +       struct aspeed_spi_chip *chip = &aspi->chips[desc->mem->spi->chip_select];
> +       struct spi_mem_op *op = &desc->info.op_tmpl;
> +       u32 ctl_val;
> +       int ret = 0;
> +
> +       chip->clk_freq = desc->mem->spi->max_speed_hz;
> +
> +       /* Only for reads */
> +       if (op->data.dir != SPI_MEM_DATA_IN)
> +               return -EOPNOTSUPP;
> +
> +       if (desc->info.length > chip->ahb_window_size)
> +               dev_warn(aspi->dev, "CE%d window (%dMB) too small for mapping",
> +                        chip->cs, chip->ahb_window_size >> 20);
> +
> +       /* Define the default IO read settings */
> +       ctl_val = readl(chip->ctl) & ~CTRL_IO_CMD_MASK;
> +       ctl_val |= aspeed_spi_get_io_mode(op) |
> +               op->cmd.opcode << CTRL_COMMAND_SHIFT |
> +               CTRL_IO_DUMMY_SET(op->dummy.nbytes / op->dummy.buswidth) |
> +               CTRL_IO_MODE_READ;
> +
> +       /* Tune 4BYTE address mode */
> +       if (op->addr.nbytes) {
> +               u32 addr_mode = readl(aspi->regs + CE_CTRL_REG);
> +
> +               if (op->addr.nbytes == 4)
> +                       addr_mode |= (0x11 << chip->cs);
> +               else
> +                       addr_mode &= ~(0x11 << chip->cs);
> +               writel(addr_mode, aspi->regs + CE_CTRL_REG);
> +       }
> +
> +       /* READ mode is the controller default setting */
> +       chip->ctl_val[ASPEED_SPI_READ] = ctl_val;
> +       writel(chip->ctl_val[ASPEED_SPI_READ], chip->ctl);
> +
> +       dev_info(aspi->dev, "CE%d read buswidth:%d [0x%08x]\n",
> +                chip->cs, op->data.buswidth, chip->ctl_val[ASPEED_SPI_READ]);
> +
> +       return ret;
> +}
> +
> +static int aspeed_spi_dirmap_read(struct spi_mem_dirmap_desc *desc,
> +                                 u64 offset, size_t len, void *buf)
> +{
> +       struct aspeed_spi *aspi = spi_controller_get_devdata(desc->mem->spi->master);
> +       struct aspeed_spi_chip *chip = &aspi->chips[desc->mem->spi->chip_select];
> +
> +       /* Switch to USER command mode if mapping window is too small */
> +       if (chip->ahb_window_size < offset + len)
> +               aspeed_spi_read_user(chip, &desc->info.op_tmpl, offset, len, buf);
> +       else
> +               memcpy_fromio(buf, chip->ahb_base + offset, len);
> +
> +       return len;
> +}
> +
>  static const struct spi_controller_mem_ops aspeed_spi_mem_ops = {
>         .supports_op = aspeed_spi_supports_op,
>         .exec_op = aspeed_spi_exec_op,
>         .get_name = aspeed_spi_get_name,
> +       .dirmap_create = aspeed_spi_dirmap_create,
> +       .dirmap_read = aspeed_spi_dirmap_read,
>  };
>
>  static void aspeed_spi_chip_set_type(struct aspeed_spi *aspi, unsigned int cs, int type)
> --
> 2.34.1
>