Hi Cédric,
> -----Original Message-----
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Monday, July 4, 2022 11:25 PM
> Subject: Re: [v4 03/12] spi: aspeed: Add ASPEED SPI controller driver
>
> Hello Chin-Ting,
>
> On 7/3/22 10:47, Chin-Ting Kuo wrote:
> > Hi Cédric,
> >
> > Thanks for the review.
> >
> >> -----Original Message-----
> >> From: Cédric Le Goater <c...@kaod.org>
> >> Sent: Friday, July 1, 2022 5:28 PM
> >> To: Chin-Ting Kuo <chin-ting_...@aspeedtech.com>; ChiaWei Wang
> >> <chiawei_w...@aspeedtech.com>; lu...@denx.de; sean...@gmail.com;
> Ryan
> >> Chen <ryan_c...@aspeedtech.com>; BMC-SW
> <bmc...@aspeedtech.com>;
> >> ja...@amarulasolutions.com; vigne...@ti.com; u-boot@lists.denx.de;
> >> p.ya...@ti.com
> >> Subject: Re: [v4 03/12] spi: aspeed: Add ASPEED SPI controller driver
> >>
> >> Hello Chin-Ting,
> >>
> >> On 5/24/22 07:56, Chin-Ting Kuo wrote:
> >>> Add ASPEED BMC FMC/SPI memory controller driver with spi-mem
> >>> interface for AST2500 and AST2600 platform.
> >>>
> >>> There are three SPI memory controllers embedded in an ASPEED SoC.
> >>> - FMC: Named as Firmware Memory Controller. After AC on, MCU ROM
> >>> fetches initial device boot image from FMC chip select(CS) 0.
> >>>
> >>> - SPI1: Play the role of a SPI Master controller. Or, there is a
> >>> dedicated path for HOST(X86) to access its BIOS flash
> mounted
> >>> under BMC. spi-aspeed.c implements the control sequence
> when
> >>> SPI1 is a SPI master.
> >>>
> >>> - SPI2: It is a pure SPI flash controller. For most scenarios, flashes
> >>> mounted under it are for pure storage purpose.
> >>>
> >>> ASPEED SPI controller supports 1-1-1, 1-1-2 and 1-1-4 SPI flash mode.
> >>> Three types of command mode are supported, normal mode, command
> >>> read/write mode and user mode.
> >>> - Normal mode: Default mode. After power on, normal read command
> 03h
> >> or
> >>> 13h is used to fetch boot image from SPI flash.
> >>> - AST2500: Only 03h command can be used after
> power
> >> on
> >>> or reset.
> >>> - AST2600: If FMC04[6:4] is set, 13h command is
> used,
> >>> otherwise, 03h command.
> >>> The address length is decided by FMC04[2:0].
> >>>
> >>> - Command mode: SPI controller can send command and address
> >>> automatically when CPU read/write the related
> >> remapped
> >>> or decoded address area. The command used by
> this
> >> mode
> >>> can be configured by FMC10/14/18[23:16]. Also,
> the
> >>> address length is decided by FMC04[2:0]. This mode
> >> will
> >>> be implemented in the following patch series.
> >>>
> >>> - User mode: It is a traditional and pure SPI operation, where
> >>> SPI transmission is controlled by CPU. It is the main
> >>> mode in this patch.
> >>>
> >>> Each SPI controller in ASPEED SoC has its own decoded address mapping.
> >>> Within each SPI controller decoded address, driver can assign a
> >>> specific address region for each CS of a SPI controller. The decoded
> >>> address cannot overlap to each other. With normal mode and command
> >>> mode, the decoded address accessed by the CPU determines which CS is
> >> active.
> >>> When user mode is adopted, the CS decoded address is a FIFO, CPU can
> >>> send/receive any SPI transmission by accessing the related decoded
> >>> address for the target CS.
> >>>
> >>> Signed-off-by: Chin-Ting Kuo <chin-ting_...@aspeedtech.com>
> >>
> >> I would split the patch furthermore to ease reading.
> >
> > Okay, this will be update in the next version.
> >
> >> 1 - Add basic support
> >>
> >> with default decoding ranges set for all possible CS, even
> >> without a device.
> >>
> >> WE only have USER mode for now. So it's not important to
> >> correctly set the ranges since we won't use them before
> >> direct mapping is introduced. They should not overlap,
> >> that's all.
> >>
> >> 2 - decoding range adjustments
> >>
> >> On that topic, we might want to take the simple DT approach
> >> with a "ranges" property defining the mapping windows of each
> >> CE. I think it is safer than trying to compute perfect ranges
> >> like on Linux.
> >>
> >> 3 - clock settings
> >>
> >> That should simply be the property defined in the DT
> >>
> >>
> >>> ---
> >>> v2: Remove defconfig files from this patch.
> >>>
> >>> drivers/spi/Kconfig | 8 +
> >>> drivers/spi/Makefile | 1 +
> >>> drivers/spi/spi-aspeed.c | 822
> >> +++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 831 insertions(+)
> >>> create mode 100644 drivers/spi/spi-aspeed.c
> >>>
> >>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
> >>> a1e515cb2b..a616294910 100644
> >>> --- a/drivers/spi/Kconfig
> >>> +++ b/drivers/spi/Kconfig
> >>> @@ -387,6 +387,14 @@ config SANDBOX_SPI
> >>> };
> >>> };
> >>>
> >>> +config SPI_ASPEED
> >>> + bool "ASPEED SPI controller driver"
> >>> + depends on DM_SPI && SPI_MEM
> >>> + default n
> >>> + help
> >>> + Enable ASPEED SPI controller driver for AST2500
> >>> + and AST2600 SoCs.
> >>> +
> >>> config SPI_SIFIVE
> >>> bool "SiFive SPI driver"
> >>> help
> >>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index
> >>> 06e81b465b..36a4bd5dce 100644
> >>> --- a/drivers/spi/Makefile
> >>> +++ b/drivers/spi/Makefile
> >>> @@ -9,6 +9,7 @@ obj-y += spi-uclass.o
> >>> obj-$(CONFIG_CADENCE_QSPI) += cadence_qspi.o
> cadence_qspi_apb.o
> >>> obj-$(CONFIG_SANDBOX) += spi-emul-uclass.o
> >>> obj-$(CONFIG_SOFT_SPI) += soft_spi.o
> >>> +obj-$(CONFIG_SPI_ASPEED) += spi-aspeed.o
> >>> obj-$(CONFIG_SPI_MEM) += spi-mem.o
> >>> obj-$(CONFIG_TI_QSPI) += ti_qspi.o
> >>> obj-$(CONFIG_FSL_QSPI) += fsl_qspi.o diff --git
> >>> a/drivers/spi/spi-aspeed.c b/drivers/spi/spi-aspeed.c new file mode
> >>> 100644 index 0000000000..9574aff793
> >>> --- /dev/null
> >>> +++ b/drivers/spi/spi-aspeed.c
> >>> @@ -0,0 +1,822 @@
> >>> +// SPDX-License-Identifier: GPL-2.0+
> >>> +/*
> >>> + * ASPEED FMC/SPI Controller driver
> >>> + *
> >>> + * Copyright (c) 2022 ASPEED Corporation.
> >>> + * Copyright (c) 2022 IBM Corporation.
> >>> + *
> >>> + * Author:
> >>> + * Chin-Ting Kuo <chin-ting_...@aspeedtech.com>
> >>> + * Cedric Le Goater <c...@kaod.org>
> >>> + */
> >>> +
> >>> +#include <asm/io.h>
> >>> +#include <clk.h>
> >>> +#include <common.h>
> >>> +#include <dm.h>
> >>> +#include <dm/device_compat.h>
> >>> +#include <linux/bitops.h>
> >>> +#include <linux/bug.h>
> >>> +#include <linux/err.h>
> >>> +#include <linux/iopoll.h>
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/mtd/spi-nor.h>
> >>> +#include <linux/sizes.h>
> >>> +#include <malloc.h>
> >>> +#include <spi.h>
> >>> +#include <spi-mem.h>
> >>> +
> >>> +/* ASPEED FMC/SPI memory control register related */
> >>> +#define REG_CE_TYPE_SETTING 0x00
> >>> +#define REG_CE_ADDR_MODE_CTRL 0x04
> >>> +#define REG_INTR_CTRL_STATUS 0x08
> >>> +#define REG_CE0_CTRL_REG 0x10
> >>> +#define REG_CE0_DECODED_ADDR_REG 0x30
> >>> +
> >>> +#define ASPEED_SPI_MAX_CS 3
> >>> +#define FLASH_CALIBRATION_LEN 0x400
> >>> +
> >>> +#define CTRL_IO_SINGLE_DATA 0
> >>> +#define CTRL_IO_QUAD_DATA BIT(30)
> >>> +#define CTRL_IO_DUAL_DATA BIT(29)
> >>> +
> >>> +#define CTRL_IO_MODE_USER GENMASK(1, 0)
> >>> +#define CTRL_IO_MODE_CMD_READ BIT(0)
> >>> +#define CTRL_IO_MODE_CMD_WRITE BIT(1)
> >>> +#define CTRL_STOP_ACTIVE BIT(2)
> >>> +
> >>> +struct aspeed_spi_plat {
> >>> + fdt_addr_t ctrl_base;
> >>
> >> are these the registers ?
> >
> > No, "struct aspeed_spi_plat" is used to record some basic information of
> > this
> platform.
> >
> >>> + void __iomem *ahb_base; /* AHB address base for all flash devices.
> */
> >>> + fdt_size_t ahb_sz; /* Overall AHB window size for all flash device. */
> >>> + u32 hclk_rate; /* AHB clock rate */
> >>> + u8 max_cs;
> >>
> >>
> >> I don't think we need a "max_cs" in the controller struct and a "num-cs"
> >> property in the DT. We could simply use a HW maxmimum defined in
> >> aspeed_spi_info.
> >>
> >
> > "num-cs" is used to detect the number of active flash node.
> > This property is mainly used to maintain the address decoded range.
>
> I am not sure we need "num-cs" since we should configure all decoding range
> registers to avoid overlaps. At least on some of the aST2500 controllers.
>
> Does the AST2600 FMC and SPI1/2 allow to configure a single CE to cover the
> whole mapping window of the controller while the other CE windows are
> closed ?
Yes, AST2600 allows a single CE to occupy the whole decoded windows while
the other CEs are closed.
>
> > "max-cs" is used for controlling register access.
> > We need to know the maximum CS number supported by the current
> controller.
>
> Yes. That's fine. It is an HW limit per controller and we need to keep this
> property in the DT or in the driver.
>
Okay.
> I have been defining this HW property in the driver. That might have been a
> wrong choice. Something to fix in Linux may be.
>
> >>
> >>> +};
> >>> +
> >>> +struct aspeed_spi_flash {
> >>> + u8 cs;
> >>> + void __iomem *ahb_base;
> >>> + u32 ahb_win_sz;
> >>> + u32 ce_ctrl_user;
> >>> + u32 ce_ctrl_read;
> >>> + u32 max_freq;
> >>> + bool trimmed_decoded_sz;
> >>
> >> I wonder what this is for. We need to split the patches :)
> >
> > Oh, it is the redundant one and it will be removed in the next patch
> > version.
> >
> >>
> >>> +};
> >>> +
> >>> +struct aspeed_spi_priv {
> >>> + u32 num_cs;
> >>
> >> See above.
> >>
> >>> + struct aspeed_spi_info *info;
> >>> + struct aspeed_spi_flash flashes[ASPEED_SPI_MAX_CS];
> >>> + u32 decoded_sz_arr[ASPEED_SPI_MAX_CS];
> >>> +};
> >>
> >>
> >> Couldn't we have a 'struct aspeed_spi_regs' defining the layout of
> >> the registers ?
> >>
> >
> > Why? The register offset has been defined by macro previously.
>
> Not always. Look at the *_regs struct in the different drivers and :
>
> https://github.com/legoater/u-boot/blob/aspeed-v2022.04/arch/arm/include/a
> sm/arch-aspeed/spi.h#L72
>
> I am not sure what is the preferred practice in U-Boot. But a few Aspeed
> drivers
> have been folowing this scheme.
>
Okay, but we need to take some time to modify the whole driver.
> >
> >>> +struct aspeed_spi_info {
> >>> + u32 cmd_io_ctrl_mask;
> >>> + u32 clk_ctrl_mask;
> >>> + u32 max_data_bus_width;
> >>> + u32 min_decoded_sz;
> >>> + void (*set_4byte)(struct udevice *bus, u32 cs);
> >>> + u32 (*segment_start)(struct udevice *bus, u32 reg);
> >>> + u32 (*segment_end)(struct udevice *bus, u32 reg);
> >>> + u32 (*segment_reg)(u32 start, u32 end);
> >>> + int (*adjust_decoded_sz)(struct udevice *bus, u32 decoded_sz_arr[]);
> >>> + u32 (*get_clk_setting)(struct udevice *dev, uint hz); };
> >>> +
> >>> +static int aspeed_spi_trim_decoded_size(struct udevice *bus,
> >>> + u32 decoded_sz_arr[]);
> >>> +
> >>> +static u32 aspeed_spi_get_io_mode(u32 bus_width) {
> >>> + switch (bus_width) {
> >>> + case 1:
> >>> + return CTRL_IO_SINGLE_DATA;
> >>> + case 2:
> >>> + return CTRL_IO_DUAL_DATA;
> >>> + case 4:
> >>> + return CTRL_IO_QUAD_DATA;
> >>> + default:
> >>> + return CTRL_IO_SINGLE_DATA;
> >>> + }
> >>> +}
> >>> +
> >>> +static u32 ast2500_spi_segment_start(struct udevice *bus, u32 reg) {
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + u32 start_offset = ((reg >> 16) & 0xff) << 23;
> >>> +
> >>> + if (start_offset == 0)
> >>> + return (u32)plat->ahb_base;
> >>> +
> >>> + return (u32)plat->ahb_base + start_offset; }
> >>> +
> >>> +static u32 ast2500_spi_segment_end(struct udevice *bus, u32 reg) {
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + u32 end_offset = ((reg >> 24) & 0xff) << 23;
> >>> +
> >>> + /* Meaningless end_offset, set to physical ahb base. */
> >>> + if (end_offset == 0)
> >>> + return (u32)plat->ahb_base;
> >>> +
> >>> + return (u32)plat->ahb_base + end_offset + 0x100000; }
> >>> +
> >>> +static u32 ast2500_spi_segment_reg(u32 start, u32 end) {
> >>> + if (start == end)
> >>> + return 0;
> >>> +
> >>> + return ((((start) >> 23) & 0xff) << 16) | ((((end) >> 23) & 0xff)
> >>> +<< 24); }
> >>> +
> >>> +static void ast2500_spi_chip_set_4byte(struct udevice *bus, u32 cs) {
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + u32 reg_val;
> >>> +
> >>> + reg_val = readl(plat->ctrl_base + REG_CE_ADDR_MODE_CTRL);
> >>> + reg_val |= 0x1 << cs;
> >>> + writel(reg_val, plat->ctrl_base + REG_CE_ADDR_MODE_CTRL); }
> >>> +
> >>> +/*
> >>> + * For AST2500, the minimum address decoded size for each CS
> >>> + * is 8MB instead of zero. This address decoded size is
> >>> + * mandatory for each CS no matter whether it will be used.
> >>> + * This is a HW limitation.
> >>> + */
> >>> +static int ast2500_adjust_decoded_size(struct udevice *bus,
> >>> + u32 decoded_sz_arr[])
> >>> +{
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(bus);
> >>> + int ret;
> >>> + int cs;
> >>> +
> >>> + /* Assign min_decoded_sz to unused CS. */
> >>> + for (cs = priv->num_cs; cs < plat->max_cs; cs++) {
> >>> + if (decoded_sz_arr[cs] < priv->info->min_decoded_sz)
> >>> + decoded_sz_arr[cs] = priv->info->min_decoded_sz;
> >>> + }
> >>> +
> >>> + ret = aspeed_spi_trim_decoded_size(bus, decoded_sz_arr);
> >>> + if (ret != 0)
> >>> + return ret;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +/* Transfer maximum clock frequency to register setting */ static
> >>> +u32 ast2500_get_clk_setting(struct udevice *dev, uint max_hz) {
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(dev->parent);
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(dev->parent);
> >>> + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
> >>> + u32 hclk_clk = plat->hclk_rate;
> >>> + u32 hclk_div = 0x000; /* default value */
> >>> + u32 i;
> >>> + bool found = false;
> >>> + /* HCLK/1 .. HCLK/16 */
> >>> + u32 hclk_masks[] = {15, 7, 14, 6, 13, 5, 12, 4,
> >>> + 11, 3, 10, 2, 9, 1, 8, 0};
> >>> +
> >>> + /* FMC/SPIR10[11:8] */
> >>> + for (i = 0; i < ARRAY_SIZE(hclk_masks); i++) {
> >>> + if (hclk_clk / (i + 1) <= max_hz) {
> >>> + found = true;
> >>> + priv->flashes[slave_plat->cs].max_freq =
> >>> + hclk_clk / (i + 1);
> >>> + break;
> >>> + }
> >>> + }
> >>> +
> >>> + if (found) {
> >>> + hclk_div = hclk_masks[i] << 8;
> >>> + goto end;
> >>> + }
> >>> +
> >>> + for (i = 0; i < ARRAY_SIZE(hclk_masks); i++) {
> >>> + if (hclk_clk / ((i + 1) * 4) <= max_hz) {
> >>> + found = true;
> >>> + priv->flashes[slave_plat->cs].max_freq =
> >>> + hclk_clk / ((i + 1) * 4);
> >>> + break;
> >>> + }
> >>> + }
> >>> +
> >>> + if (found)
> >>> + hclk_div = BIT(13) | (hclk_masks[i] << 8);
> >>> +
> >>> +end:
> >>> + dev_dbg(dev, "found: %s, hclk: %d, max_clk: %d\n", found ? "yes" :
> "no",
> >>> + hclk_clk, max_hz);
> >>> +
> >>> + if (found) {
> >>> + dev_dbg(dev, "h_div: %d (mask %x), speed: %d\n",
> >>> + i + 1, hclk_masks[i],
> priv->flashes[slave_plat->cs].max_freq);
> >>> + }
> >>> +
> >>> + return hclk_div;
> >>> +}
> >>> +
> >>> +static u32 ast2600_spi_segment_start(struct udevice *bus, u32 reg) {
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + u32 start_offset = (reg << 16) & 0x0ff00000;
> >>> +
> >>> + if (start_offset == 0)
> >>> + return (u32)plat->ahb_base;
> >>> +
> >>> + return (u32)plat->ahb_base + start_offset; }
> >>> +
> >>> +static u32 ast2600_spi_segment_end(struct udevice *bus, u32 reg) {
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + u32 end_offset = reg & 0x0ff00000;
> >>> +
> >>> + /* Meaningless end_offset, set to physical ahb base. */
> >>> + if (end_offset == 0)
> >>> + return (u32)plat->ahb_base;
> >>> +
> >>> + return (u32)plat->ahb_base + end_offset + 0x100000; }
> >>> +
> >>> +static u32 ast2600_spi_segment_reg(u32 start, u32 end) {
> >>> + if (start == end)
> >>> + return 0;
> >>> +
> >>> + return ((start & 0x0ff00000) >> 16) | ((end - 0x100000) &
> >>> +0x0ff00000); }
> >>> +
> >>> +static void ast2600_spi_chip_set_4byte(struct udevice *bus, u32 cs) {
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + u32 reg_val;
> >>> +
> >>> + reg_val = readl(plat->ctrl_base + REG_CE_ADDR_MODE_CTRL);
> >>> + reg_val |= 0x11 << cs;
> >>> + writel(reg_val, plat->ctrl_base + REG_CE_ADDR_MODE_CTRL); }
> >>> +
> >>> +static int ast2600_adjust_decoded_size(struct udevice *bus,
> >>> + u32 decoded_sz_arr[])
> >>> +{
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(bus);
> >>> + int ret;
> >>> + int i;
> >>> + int cs;
> >>> + u32 pre_sz;
> >>> + u32 lack_sz;
> >>> +
> >>> + /*
> >>> + * If commnad mode or normal mode is used, the start address of a
> >>> + * decoded range should be multiple of its related flash size.
> >>> + * Namely, the total decoded size from flash 0 to flash N should
> >>> + * be multiple of the size of flash (N + 1).
> >>> + */
> >>> + for (cs = priv->num_cs - 1; cs >= 0; cs--) {
> >>> + pre_sz = 0;
> >>> + for (i = 0; i < cs; i++)
> >>> + pre_sz += decoded_sz_arr[i];
> >>> +
> >>> + if (decoded_sz_arr[cs] != 0 && (pre_sz % decoded_sz_arr[cs]) !=
> 0) {
> >>> + lack_sz = decoded_sz_arr[cs] - (pre_sz %
> decoded_sz_arr[cs]);
> >>> + decoded_sz_arr[0] += lack_sz;
> >>> + }
> >>> + }
> >>> +
> >>> + ret = aspeed_spi_trim_decoded_size(bus, decoded_sz_arr);
> >>> + if (ret != 0)
> >>> + return ret;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +/* Transfer maximum clock frequency to register setting */ static
> >>> +u32 ast2600_get_clk_setting(struct udevice *dev, uint max_hz) {
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(dev->parent);
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(dev->parent);
> >>> + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
> >>> + u32 hclk_clk = plat->hclk_rate;
> >>> + u32 hclk_div = 0x400; /* default value */
> >>> + u32 i, j;
> >>> + bool found = false;
> >>> + /* HCLK/1 .. HCLK/16 */
> >>> + u32 hclk_masks[] = {15, 7, 14, 6, 13, 5, 12, 4,
> >>> + 11, 3, 10, 2, 9, 1, 8, 0};
> >>> +
> >>> + /* FMC/SPIR10[27:24] */
> >>> + for (j = 0; j < 0xf; j++) {
> >>> + /* FMC/SPIR10[11:8] */
> >>> + for (i = 0; i < ARRAY_SIZE(hclk_masks); i++) {
> >>> + if (i == 0 && j == 0)
> >>> + continue;
> >>> +
> >>> + if (hclk_clk / (i + 1 + (j * 16)) <= max_hz) {
> >>> + found = true;
> >>> + break;
> >>> + }
> >>> + }
> >>> +
> >>> + if (found) {
> >>> + hclk_div = ((j << 24) | hclk_masks[i] << 8);
> >>> + priv->flashes[slave_plat->cs].max_freq =
> >>> + hclk_clk / (i + 1 + j * 16);
> >>> + break;
> >>> + }
> >>> + }
> >>> +
> >>> + dev_dbg(dev, "found: %s, hclk: %d, max_clk: %d\n", found ? "yes" :
> "no",
> >>> + hclk_clk, max_hz);
> >>> +
> >>> + if (found) {
> >>> + dev_dbg(dev, "base_clk: %d, h_div: %d (mask %x), speed: %d\n",
> >>> + j, i + 1, hclk_masks[i],
> priv->flashes[slave_plat->cs].max_freq);
> >>> + }
> >>> +
> >>> + return hclk_div;
> >>> +}
> >>> +
> >>> +/*
> >>> + * As the flash size grows up, we need to trim some decoded
> >>> + * size if needed for the sake of conforming the maximum
> >>> + * decoded size. We trim the decoded size from the largest
> >>> + * CS in order to avoid affecting the default boot up sequence
> >>> + * from CS0 where command mode or normal mode is used.
> >>> + * Notice, if a CS decoded size is trimmed, command mode may
> >>> + * not work perfectly on that CS.
> >>> + */
> >>> +static int aspeed_spi_trim_decoded_size(struct udevice *bus,
> >>> + u32 decoded_sz_arr[])
> >>> +{
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(bus);
> >>> + u32 total_sz;
> >>> + int cs = plat->max_cs - 1;
> >>> + u32 i;
> >>> +
> >>> + do {
> >>> + total_sz = 0;
> >>> + for (i = 0; i < plat->max_cs; i++)
> >>> + total_sz += decoded_sz_arr[i];
> >>> +
> >>> + if (decoded_sz_arr[cs] <= priv->info->min_decoded_sz)
> >>> + cs--;
> >>> +
> >>> + if (cs < 0)
> >>> + return -ENOMEM;
> >>> +
> >>> + if (total_sz > plat->ahb_sz) {
> >>> + decoded_sz_arr[cs] -= priv->info->min_decoded_sz;
> >>> + total_sz -= priv->info->min_decoded_sz;
> >>> + priv->flashes[cs].trimmed_decoded_sz = true;
> >>> + }
> >>> + } while (total_sz > plat->ahb_sz);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int aspeed_spi_read_from_ahb(void __iomem *ahb_base, void
> *buf,
> >>> + size_t len)
> >>> +{
> >>> + size_t offset = 0;
> >>> +
> >>> + if (IS_ALIGNED((uintptr_t)ahb_base, sizeof(uintptr_t)) &&
> >>> + IS_ALIGNED((uintptr_t)buf, sizeof(uintptr_t))) {
> >>> + readsl(ahb_base, buf, len >> 2);
> >>> + offset = len & ~0x3;
> >>> + len -= offset;
> >>> + }
> >>> +
> >>> + readsb(ahb_base, (u8 *)buf + offset, len);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int aspeed_spi_write_to_ahb(void __iomem *ahb_base, const
> >>> +void
> >> *buf,
> >>> + size_t len)
> >>> +{
> >>> + size_t offset = 0;
> >>> +
> >>> + if (IS_ALIGNED((uintptr_t)ahb_base, sizeof(uintptr_t)) &&
> >>> + IS_ALIGNED((uintptr_t)buf, sizeof(uintptr_t))) {
> >>> + writesl(ahb_base, buf, len >> 2);
> >>> + offset = len & ~0x3;
> >>> + len -= offset;
> >>> + }
> >>> +
> >>> + writesb(ahb_base, (u8 *)buf + offset, len);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Currently, only support 1-1-1, 1-1-2 or 1-1-4
> >>> + * SPI NOR flash operation format.
> >>> + */
> >>> +static bool aspeed_spi_supports_op(struct spi_slave *slave,
> >>> + const struct spi_mem_op *op) {
> >>> + struct udevice *bus = slave->dev->parent;
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(bus);
> >>> +
> >>> + if (op->cmd.buswidth > 1)
> >>> + return false;
> >>> +
> >>> + if (op->addr.buswidth > 1 || op->addr.nbytes > 4)
> >>> + return false;
> >>> +
> >>> + if (op->dummy.buswidth > 1 || op->dummy.nbytes > 7)
> >>> + return false;
> >>> +
> >>> + if (op->data.buswidth > priv->info->max_data_bus_width)
> >>> + return false;
> >>> +
> >>> + if (!spi_mem_default_supports_op(slave, op))
> >>> + return false;
> >>> +
> >>> + return true;
> >>> +}
> >>
> >> You could copy the Linux aspeed_spi_supports_op()
> >>
> >
> > Okay, this patch series may be too old.
> >
> >>> +
> >>> +static int aspeed_spi_exec_op_user_mode(struct spi_slave *slave,
> >>> + const struct spi_mem_op *op)
> >>> +{
> >>> + struct udevice *dev = slave->dev;
> >>> + struct udevice *bus = dev->parent;
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(bus);
> >>> + struct dm_spi_slave_plat *slave_plat =
> dev_get_parent_plat(slave->dev);
> >>> + u32 cs = slave_plat->cs;
> >>> + fdt_addr_t ctrl_reg = plat->ctrl_base + REG_CE0_CTRL_REG + cs * 4;
> >>> + struct aspeed_spi_flash *flash = &priv->flashes[cs];
> >>> + u32 ctrl_val;
> >>> + u8 dummy_data[16] = {0};
> >>> + u8 addr[4] = {0};
> >>> + int i;
> >>> +
> >>> + dev_dbg(dev,
> >> "cmd:%x(%d),addr:%llx(%d),dummy:%d(%d),data_len:0x%x(%d)\n",
> >>> + op->cmd.opcode, op->cmd.buswidth, op->addr.val,
> >>> + op->addr.buswidth, op->dummy.nbytes, op->dummy.buswidth,
> >>> + op->data.nbytes, op->data.buswidth);
> >>> +
> >>> + /* Start user mode */
> >>> + ctrl_val = flash->ce_ctrl_user;
> >>> + writel(ctrl_val, ctrl_reg);
> >>> + ctrl_val &= (~CTRL_STOP_ACTIVE);
> >>> + writel(ctrl_val, ctrl_reg);
> >>> +
> >>> + /* Send command */
> >>> + aspeed_spi_write_to_ahb(flash->ahb_base, &op->cmd.opcode, 1);
> >>> +
> >>> + /* Send address */
> >>> + for (i = op->addr.nbytes; i > 0; i--) {
> >>> + addr[op->addr.nbytes - i] =
> >>> + ((u32)op->addr.val >> ((i - 1) * 8)) & 0xff;
> >>> + }
> >>> + aspeed_spi_write_to_ahb(flash->ahb_base, addr, op->addr.nbytes);
> >>
> >> This could be writing 3 bytes. Not optimal.
> >
> > Why? Doesn't it depend on the value of "op->addr.nbytes"? This function has
> been verified with different flash parts.
>
> It works but you could issue a single 32-bit transaction if the OP was
> included
> in the write, as Linux does. This is an optimisation to avoid
> 4 writes of one byte. This is minor.
>
"aspeed_spi_write_to_ahb" function here is almost the same as the one in Linux.
It can write a 32-bit transaction once if needed.
> >>
> >>> +
> >>> + /* Send dummy cycle */
> >>> + aspeed_spi_write_to_ahb(flash->ahb_base, dummy_data,
> >>> +op->dummy.nbytes);
> >>> +
> >>> + /* Change io_mode */
> >>> + ctrl_val |= aspeed_spi_get_io_mode(op->data.buswidth);
> >>> + writel(ctrl_val, ctrl_reg);
> >>> +
> >>> + /* Send data */
> >>> + if (op->data.dir == SPI_MEM_DATA_OUT) {
> >>> + aspeed_spi_write_to_ahb(flash->ahb_base, op->data.buf.out,
> >>> + op->data.nbytes);
> >>> + } else {
> >>> + aspeed_spi_read_from_ahb(flash->ahb_base, op->data.buf.in,
> >>> + op->data.nbytes);
> >>> + }
> >>> +
> >>> + ctrl_val |= CTRL_STOP_ACTIVE;
> >>> + writel(ctrl_val, ctrl_reg);
> >>> +
> >>> + /* Restore controller setting. */
> >>> + writel(flash->ce_ctrl_read, ctrl_reg);
> >>> +
> >>> + /* Set controller to 4-byte mode when flash is in 4-byte mode. */
> >>> + if (op->cmd.opcode == SPINOR_OP_EN4B)
> >>> + priv->info->set_4byte(bus, cs);
> >>
> >> We don't need to set 4B earlier ? I trust you there.
> >
> > Do you mean early in this function? It may be okay.
> >
> >>
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static struct aspeed_spi_flash *aspeed_spi_get_flash(struct udevice
> >>> +*dev) {
> >>> + struct udevice *bus = dev->parent;
> >>> + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(bus);
> >>> + u32 cs = slave_plat->cs;
> >>> +
> >>> + if (cs >= plat->max_cs) {
> >>> + dev_err(dev, "invalid CS %u\n", cs);
> >>> + return NULL;
> >>> + }
> >>> +
> >>> + return &priv->flashes[cs];
> >>> +}
> >>> +
> >>> +static int aspeed_spi_decoded_range_config(struct udevice *bus,
> >>> + u32 decoded_sz_arr[])
> >>> +{
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(bus);
> >>> + int ret;
> >>> + u32 cs;
> >>> + u32 decoded_reg_val;
> >>> + u32 start_addr;
> >>> + u32 end_addr = 0;
> >>> +
> >>> + ret = priv->info->adjust_decoded_sz(bus, decoded_sz_arr);
> >>> + if (ret != 0)
> >>> + return ret;
> >>> +
> >>> + /* Configure each CS decoded range */
> >>> + for (cs = 0; cs < plat->max_cs; cs++) {
> >>> + if (cs == 0)
> >>> + start_addr = (u32)plat->ahb_base;
> >>> + else
> >>> + start_addr = end_addr;
> >>> + priv->flashes[cs].ahb_base = (void __iomem *)start_addr;
> >>> + priv->flashes[cs].ahb_win_sz = decoded_sz_arr[cs];
> >>> +
> >>> + end_addr = start_addr + decoded_sz_arr[cs];
> >>> + decoded_reg_val = priv->info->segment_reg(start_addr,
> end_addr);
> >>> +
> >>> + writel(decoded_reg_val,
> >>> + plat->ctrl_base + REG_CE0_DECODED_ADDR_REG + cs
> * 4);
> >>> +
> >>> + dev_dbg(bus, "cs: %d, decoded_reg: 0x%x, start: 0x%x, end:
> 0x%x\n",
> >>> + cs, decoded_reg_val, start_addr, end_addr);
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Initialize SPI controller for each chip select.
> >>> + * Here, only the minimum decode range is configured
> >>> + * in order to get device (SPI NOR flash) information
> >>> + * at the early stage.
> >>> + */
> >>> +static int aspeed_spi_ctrl_init(struct udevice *bus) {
> >>> + int ret;
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(bus);
> >>> + u32 cs;
> >>> + u32 reg_val;
> >>> + u32 decoded_sz;
> >>> +
> >>> + /* Enable write capability for all CS. */
> >>> + reg_val = readl(plat->ctrl_base + REG_CE_TYPE_SETTING);
> >>> + writel(reg_val | (GENMASK(plat->max_cs - 1, 0) << 16),
> >>> + plat->ctrl_base + REG_CE_TYPE_SETTING);
> >>> +
> >>> + memset(priv->flashes, 0x0,
> >>> + sizeof(struct aspeed_spi_flash) * ASPEED_SPI_MAX_CS);
> >>> +
> >>> + /* Initial each CS controller register */
> >>> + for (cs = 0; cs < priv->num_cs; cs++) {
> >>> + priv->flashes[cs].ce_ctrl_user &=
> >>> + ~(priv->info->cmd_io_ctrl_mask);
> >>> + priv->flashes[cs].ce_ctrl_user |=
> >>> + (CTRL_STOP_ACTIVE | CTRL_IO_MODE_USER);
> >>> + writel(priv->flashes[cs].ce_ctrl_user,
> >>> + plat->ctrl_base + REG_CE0_CTRL_REG + cs * 4);
> >>> + }
> >>>
> >>
> >> and we should start by setting sane defaults for the ranges.
> >>
> >> It's too early to add the decoding ranges calculation.
> >
> > Okay.
> >
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>> + memset(priv->decoded_sz_arr, 0x0, sizeof(u32) *
> >>> +ASPEED_SPI_MAX_CS);
> >>> +
> >>> + for (cs = 0; cs < priv->num_cs; cs++) {
> >>> + reg_val = readl(plat->ctrl_base +
> REG_CE0_DECODED_ADDR_REG +
> >> cs * 4);
> >>> + decoded_sz = priv->info->segment_end(bus, reg_val) -
> >>> + priv->info->segment_start(bus, reg_val);
> >>> +
> >>> + /*
> >>> + * For CS0, if the default address decoded area exists,
> >>> + * keep its value in order to make sure that the whole boot
> >>> + * image can be accessed with normal read mode.
> >>> + */
> >>> + if (cs == 0 && decoded_sz != 0)
> >>> + priv->decoded_sz_arr[cs] = decoded_sz;
> >>> + else
> >>> + priv->decoded_sz_arr[cs] = priv->info->min_decoded_sz;
> >>> + }
> >>> +
> >>> + ret = aspeed_spi_decoded_range_config(bus, priv->decoded_sz_arr);
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static const struct aspeed_spi_info ast2500_fmc_info = {
> >>> + .max_data_bus_width = 2,
> >>> + .cmd_io_ctrl_mask = 0x70ff40c3,
> >>> + .clk_ctrl_mask = 0x00002f00,
> >>> + .min_decoded_sz = 0x800000,
> >>> + .set_4byte = ast2500_spi_chip_set_4byte,
> >>> + .segment_start = ast2500_spi_segment_start,
> >>> + .segment_end = ast2500_spi_segment_end,
> >>> + .segment_reg = ast2500_spi_segment_reg,
> >>> + .adjust_decoded_sz = ast2500_adjust_decoded_size,
> >>> + .get_clk_setting = ast2500_get_clk_setting, };
> >>> +
> >>> +/*
> >>> + * There are some different between FMC and SPI controllers.
> >>> + * For example, DMA operation, but this isn't implemented currently.
> >>> + */
> >>> +static const struct aspeed_spi_info ast2500_spi_info = {
> >>> + .max_data_bus_width = 2,
> >>> + .cmd_io_ctrl_mask = 0x70ff40c3,
> >>> + .clk_ctrl_mask = 0x00002f00,
> >>> + .min_decoded_sz = 0x800000,
> >>> + .set_4byte = ast2500_spi_chip_set_4byte,
> >>> + .segment_start = ast2500_spi_segment_start,
> >>> + .segment_end = ast2500_spi_segment_end,
> >>> + .segment_reg = ast2500_spi_segment_reg,
> >>> + .adjust_decoded_sz = ast2500_adjust_decoded_size,
> >>> + .get_clk_setting = ast2500_get_clk_setting, };
> >>> +
> >>> +static const struct aspeed_spi_info ast2600_fmc_info = {
> >>> + .max_data_bus_width = 4,
> >>> + .cmd_io_ctrl_mask = 0xf0ff40c3,
> >>> + .clk_ctrl_mask = 0x0f000f00,
> >>> + .min_decoded_sz = 0x200000,
> >>> + .set_4byte = ast2600_spi_chip_set_4byte,
> >>> + .segment_start = ast2600_spi_segment_start,
> >>> + .segment_end = ast2600_spi_segment_end,
> >>> + .segment_reg = ast2600_spi_segment_reg,
> >>> + .adjust_decoded_sz = ast2600_adjust_decoded_size,
> >>> + .get_clk_setting = ast2600_get_clk_setting, };
> >>> +
> >>> +static const struct aspeed_spi_info ast2600_spi_info = {
> >>> + .max_data_bus_width = 4,
> >>> + .cmd_io_ctrl_mask = 0xf0ff40c3,
> >>> + .clk_ctrl_mask = 0x0f000f00,
> >>> + .min_decoded_sz = 0x200000,
> >>> + .set_4byte = ast2600_spi_chip_set_4byte,
> >>> + .segment_start = ast2600_spi_segment_start,
> >>> + .segment_end = ast2600_spi_segment_end,
> >>> + .segment_reg = ast2600_spi_segment_reg,
> >>> + .adjust_decoded_sz = ast2600_adjust_decoded_size,
> >>> + .get_clk_setting = ast2600_get_clk_setting, };
> >>> +
> >>> +static int aspeed_spi_claim_bus(struct udevice *dev) {
> >>> + struct udevice *bus = dev->parent;
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(dev->parent);
> >>> + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
> >>> + struct aspeed_spi_flash *flash = &priv->flashes[slave_plat->cs];
> >>> + u32 clk_setting;
> >>> +
> >>> + dev_dbg(bus, "%s: claim bus CS%u\n", bus->name, slave_plat->cs);
> >>> +
> >>> + if (flash->max_freq == 0) {
> >>> + clk_setting = priv->info->get_clk_setting(dev,
> slave_plat->max_hz);
> >>> + flash->ce_ctrl_user &= ~(priv->info->clk_ctrl_mask);
> >>> + flash->ce_ctrl_user |= clk_setting;
> >>> + flash->ce_ctrl_read &= ~(priv->info->clk_ctrl_mask);
> >>> + flash->ce_ctrl_read |= clk_setting;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int aspeed_spi_release_bus(struct udevice *dev) {
> >>> + struct udevice *bus = dev->parent;
> >>> + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
> >>> +
> >>> + dev_dbg(bus, "%s: release bus CS%u\n", bus->name, slave_plat->cs);
> >>> +
> >>> + if (!aspeed_spi_get_flash(dev))
> >>> + return -ENODEV;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int aspeed_spi_set_mode(struct udevice *bus, uint mode) {
> >>> + dev_dbg(bus, "%s: setting mode to %x\n", bus->name, mode);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int aspeed_spi_set_speed(struct udevice *bus, uint hz) {
> >>> + dev_dbg(bus, "%s: setting speed to %u\n", bus->name, hz);
> >>> + /* ASPEED SPI controller supports multiple CS with different
> >>> + * clock frequency. We cannot distinguish which CS here.
> >>> + * Thus, the related implementation is postponed to claim_bus.
> >>> + */
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int apseed_spi_of_to_plat(struct udevice *bus) {
> >>> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> >>> + struct clk hclk;
> >>> + int ret;
> >>> +
> >>> + plat->ctrl_base = devfdt_get_addr_index(bus, 0);
> >>> + if ((u32)plat->ctrl_base == FDT_ADDR_T_NONE) {
> >>> + dev_err(bus, "wrong AHB base\n");
> >>> + return -ENODEV;
> >>> + }
> >>> +
> >>> + plat->ahb_base =
> >>> + (void __iomem *)devfdt_get_addr_size_index(bus, 1,
> &plat->ahb_sz);
> >>> + if ((u32)plat->ahb_base == FDT_ADDR_T_NONE) {
> >>> + dev_err(bus, "wrong AHB base\n");
> >>> + return -ENODEV;
> >>> + }
> >>> +
> >>> + ret = clk_get_by_index(bus, 0, &hclk);
> >>> + if (ret < 0) {
> >>> + dev_err(bus, "%s could not get clock: %d\n", bus->name, ret);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + plat->hclk_rate = clk_get_rate(&hclk);
> >>> + clk_free(&hclk);
> >>> +
> >>> + plat->max_cs = dev_read_u32_default(bus, "num-cs",
> >> ASPEED_SPI_MAX_CS);
> >>> + if (plat->max_cs > ASPEED_SPI_MAX_CS)
> >>> + return -EINVAL;
> >>> +
> >>> + dev_dbg(bus, "ctrl_base = 0x%lx, ahb_base = 0x%p, size = 0x%lx\n",
> >>> + plat->ctrl_base, plat->ahb_base, plat->ahb_sz);
> >>> + dev_dbg(bus, "hclk = %dMHz, max_cs = %d\n",
> >>> + plat->hclk_rate / 1000000, plat->max_cs);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int aspeed_spi_probe(struct udevice *bus) {
> >>> + int ret;
> >>> + struct aspeed_spi_priv *priv = dev_get_priv(bus);
> >>> + struct udevice *dev;
> >>> +
> >>> + priv->info = (struct aspeed_spi_info *)dev_get_driver_data(bus);
> >>> +
> >>> + priv->num_cs = 0;
> >>> + for (device_find_first_child(bus, &dev); dev;
> >>> + device_find_next_child(&dev)) {
> >>> + priv->num_cs++;
> >>> + }
> >>> +
> >>> + if (priv->num_cs > ASPEED_SPI_MAX_CS)
> >>> + return -EINVAL;
> >>> +
> >>> + ret = aspeed_spi_ctrl_init(bus);
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static const struct spi_controller_mem_ops aspeed_spi_mem_ops = {
> >>> + .supports_op = aspeed_spi_supports_op,
> >>> + .exec_op = aspeed_spi_exec_op_user_mode, };
> >>> +
> >>> +static const struct dm_spi_ops aspeed_spi_ops = {
> >>> + .claim_bus = aspeed_spi_claim_bus,
> >>> + .release_bus = aspeed_spi_release_bus,
> >>> + .set_speed = aspeed_spi_set_speed,
> >>> + .set_mode = aspeed_spi_set_mode,
> >>> + .mem_ops = &aspeed_spi_mem_ops,
> >>> +};
> >>> +
> >>> +static const struct udevice_id aspeed_spi_ids[] = {
> >>> + { .compatible = "aspeed,ast2500-fmc", .data =
> >> (ulong)&ast2500_fmc_info, },
> >>> + { .compatible = "aspeed,ast2500-spi", .data =
> (ulong)&ast2500_spi_info, },
> >>> + { .compatible = "aspeed,ast2600-fmc", .data =
> >> (ulong)&ast2600_fmc_info, },
> >>> + { .compatible = "aspeed,ast2600-spi", .data =
> (ulong)&ast2600_spi_info, },
> >>> + { }
> >>> +};
> >>> +
> >>> +U_BOOT_DRIVER(aspeed_spi) = {
> >>> + .name = "aspeed_spi",
> >>> + .id = UCLASS_SPI,
> >>> + .of_match = aspeed_spi_ids,
> >>> + .ops = &aspeed_spi_ops,
> >>> + .of_to_plat = apseed_spi_of_to_plat,
> >>> + .plat_auto = sizeof(struct aspeed_spi_plat),
> >>> + .priv_auto = sizeof(struct aspeed_spi_priv),
> >>> + .probe = aspeed_spi_probe,
> >>> +};
> >
