On Tue, Sep 11, 2018 at 12:35 AM, Joel Stanley joel@jms.id.au wrote:

On Mon, 10 Sep 2018 at 23:48, Cédric Le Goater clg@kaod.org wrote:

...

The AHB clock is used by the FMC/SPI controllers.

Signed-off-by: Cédric Le Goater clg@kaod.org

arch/arm/include/asm/arch-aspeed/scu_ast2500.h | 2 ++ include/dt-bindings/clock/ast2500-scu.h | 1 + drivers/clk/aspeed/clk_ast2500.c | 12 ++++++++++++ 3 files changed, 15 insertions(+)

diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h

b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h

...

index 4988ced7ddcc..6a90ded752ad 100644 --- a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h @@ -11,6 +11,8 @@ #define SCU_HWSTRAP_VGAMEM_MASK (3 <<

SCU_HWSTRAP_VGAMEM_SHIFT)

...

#define SCU_HWSTRAP_MAC1_RGMII (1 << 6) #define SCU_HWSTRAP_MAC2_RGMII (1 << 7) +#define SCU_HWSTRAP_AXIAHB_DIV_SHIFT 9 +#define SCU_HWSTRAP_AXIAHB_DIV_MASK (0x7 <<

SCU_HWSTRAP_AXIAHB_DIV_SHIFT)

...

#define SCU_HWSTRAP_DDR4 (1 << 24) #define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)

diff --git a/include/dt-bindings/clock/ast2500-scu.h

b/include/dt-bindings/clock/ast2500-scu.h

...

index 4803abe9f628..03e6d16d3de0 100644 --- a/include/dt-bindings/clock/ast2500-scu.h +++ b/include/dt-bindings/clock/ast2500-scu.h @@ -17,6 +17,7 @@ #define BCLK_MACCLK 103 #define BCLK_SDCLK 104 #define BCLK_ARMCLK 105 +#define BCLK_HCLK 106

I like how the clocks are grouped in this file. Are we confident that HCLK is going in the correct spot?

...

#define MCLK_DDR 201

diff --git a/drivers/clk/aspeed/clk_ast2500.c b/drivers/clk/aspeed/clk_

ast2500.c

...

index 526470051c5d..c55f8d5ae30d 100644 --- a/drivers/clk/aspeed/clk_ast2500.c +++ b/drivers/clk/aspeed/clk_ast2500.c @@ -143,6 +143,18 @@ static ulong ast2500_clk_get_rate(struct clk *clk) rate = rate / apb_div; } break;

•   case BCLK_HCLK:
  

•           {
  

•                   ulong ahb_div = 1 + ((readl(&priv->scu->hwstrap)
  

•                                         &
  

SCU_HWSTRAP_AXIAHB_DIV_MASK)

...

•                                        >>
  

SCU_HWSTRAP_AXIAHB_DIV_SHIFT);

...

•                   ulong axi_div = 2;
  

•                   rate = ast2500_get_hpll_rate(
  

•                           clkin, readl(&priv->scu->h_pll_param));
  

•                   rate = rate / axi_div / ahb_div;
  

In the kernel driver I wrote it as:

rate / (axi_div + ahb_div)

These are two different formulae -- just want to make sure that the typo only made it into an email :)

In any case, the exact right way to do this computation depends on how this is implemented in the hardware itself. Most likely it's to dividers in sequence, so dividing twice should be more accurate. Of course it would be nice if somebody with hw design experience could comment.

If the datasheet has formula, I think the right way is to use it exactly as stated.

I know that the maths works, but do the numbers come out as we expect when doing integer division?

On Wed, 12 Sep 2018 at 03:13, Cédric Le Goater clg@kaod.org wrote:

On 09/11/2018 06:42 PM, Maxim Sloyko wrote:

...

On Tue, Sep 11, 2018 at 12:35 AM, Joel Stanley <joel@jms.id.au mailto:joel@jms.id.au> wrote:

On Mon, 10 Sep 2018 at 23:48, Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>> wrote:
>
> The AHB clock is used by the FMC/SPI controllers.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>>
> ---
>  arch/arm/include/asm/arch-aspeed/scu_ast2500.h |  2 ++
>  include/dt-bindings/clock/ast2500-scu.h        |  1 +
>  drivers/clk/aspeed/clk_ast2500.c               | 12 ++++++++++++
>  3 files changed, 15 insertions(+)
>
> diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> index 4988ced7ddcc..6a90ded752ad 100644
> --- a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> @@ -11,6 +11,8 @@
>  #define SCU_HWSTRAP_VGAMEM_MASK                (3 << SCU_HWSTRAP_VGAMEM_SHIFT)
>  #define SCU_HWSTRAP_MAC1_RGMII         (1 << 6)
>  #define SCU_HWSTRAP_MAC2_RGMII         (1 << 7)
> +#define SCU_HWSTRAP_AXIAHB_DIV_SHIFT   9
> +#define SCU_HWSTRAP_AXIAHB_DIV_MASK    (0x7 << SCU_HWSTRAP_AXIAHB_DIV_SHIFT)
>  #define SCU_HWSTRAP_DDR4               (1 << 24)
>  #define SCU_HWSTRAP_CLKIN_25MHZ                (1 << 23)
>
> diff --git a/include/dt-bindings/clock/ast2500-scu.h b/include/dt-bindings/clock/ast2500-scu.h
> index 4803abe9f628..03e6d16d3de0 100644
> --- a/include/dt-bindings/clock/ast2500-scu.h
> +++ b/include/dt-bindings/clock/ast2500-scu.h
> @@ -17,6 +17,7 @@
>  #define BCLK_MACCLK    103
>  #define BCLK_SDCLK     104
>  #define BCLK_ARMCLK    105
> +#define BCLK_HCLK      106

I like how the clocks are grouped in this file. Are we confident that
HCLK is going in the correct spot?

>  #define MCLK_DDR       201
>
> diff --git a/drivers/clk/aspeed/clk_ast2500.c b/drivers/clk/aspeed/clk_ast2500.c
> index 526470051c5d..c55f8d5ae30d 100644
> --- a/drivers/clk/aspeed/clk_ast2500.c
> +++ b/drivers/clk/aspeed/clk_ast2500.c
> @@ -143,6 +143,18 @@ static ulong ast2500_clk_get_rate(struct clk *clk)
>                         rate = rate / apb_div;
>                 }
>                 break;
> +       case BCLK_HCLK:
> +               {
> +                       ulong ahb_div = 1 + ((readl(&priv->scu->hwstrap)
> +                                             & SCU_HWSTRAP_AXIAHB_DIV_MASK)
> +                                            >> SCU_HWSTRAP_AXIAHB_DIV_SHIFT);
> +                       ulong axi_div = 2;
> +
> +                       rate = ast2500_get_hpll_rate(
> +                               clkin, readl(&priv->scu->h_pll_param));
> +                       rate = rate / axi_div / ahb_div;

In the kernel driver I wrote it as:

 rate / (axi_div + ahb_div)

These are two different formulae -- just want to make sure that the typo only made it into an email :)

In any case, the exact right way to do this computation depends on how this is implemented in the hardware itself. Most likely it's to dividers in sequence, so dividing twice should be more accurate. Of course it would be nice if somebody with hw design experience could comment.

If the datasheet has formula, I think the right way is to use it exactly as stated.

it's two dividers in sequence in the datasheet. In the SDK also.

...

I know that the maths works, but do the numbers come out as we expect
when doing integer division?

Yes. 192MHz.

Thanks for clarifying.

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

Hello Simon,

The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash memory, and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some misunderstanding on this driver, it might come from the fact it is closer to a SPI-NOR driver like we have in Linux, than a generic SPI driver. The stm32 SPI driver is somewhat similar.

Should we move it under drivers/mtd/spi/ maybe ?

Nevertheless, I think I can improve the dependency on the spi_flash driver and probably remove the aspeed_spi_flash struct.

On 9/27/18 3:41 PM, Simon Glass wrote:

Hi Cedric,

On 10 September 2018 at 07:16, Cédric Le Goater clg@kaod.org wrote:

...

The Aspeed AST2500 SoC comes with three static memory controllers, all with a similar interface :

• Firmware SPI Memory Controller (FMC) . BMC firmware . 3 chip select pins (CE0 ~ CE2) . supports SPI type flash memory (CE0 ~ CE1) . CE2 can be of NOR type flash but this is not supported by the driver

• SPI Flash Controller (SPI1 and SPI2) . host firmware . 2 chip select pins (CE0 ~ CE1)

Each controller has a defined AHB window for its registers and another AHB window on which all the flash devices are mapped. Each device is assigned a memory range through a set of registers called the Segment Address Registers.

Devices are controlled using two different modes: the USER command mode or the READ/WRITE command mode. When in USER command mode, accesses to the AHB window of the SPI flash device are translated into SPI command transfers. When in READ/WRITE command mode, the HW generates the SPI commands depending on the setting of the CE control register.

The following driver supports the FMC and the SPI controllers with the attached SPI flash devices. When the controller is probed, the driver performs a read timing calibration using specific DMA control registers (FMC only). The driver's primary goal is to support the first device of the FMC controller on which reside U-Boot but it should support the other controllers also.

The Aspeed FMC controller automatically detects at boot time if a flash device is in 4BYTE address mode and self configures to use the appropriate address width. This can be a problem for the U-Boot SPI Flash layer which only supports 3 byte addresses. In such a case, a warning is emitted and the address width is fixed when sent on the bus.

Signed-off-by: Cédric Le Goater clg@kaod.org

arch/arm/include/asm/arch-aspeed/spi.h | 114 ++++ drivers/spi/aspeed_spi.c | 788 +++++++++++++++++++++++++ drivers/spi/Kconfig | 8 + drivers/spi/Makefile | 1 + 4 files changed, 911 insertions(+) create mode 100644 arch/arm/include/asm/arch-aspeed/spi.h create mode 100644 drivers/spi/aspeed_spi.c

diff --git a/arch/arm/include/asm/arch-aspeed/spi.h b/arch/arm/include/asm/arch-aspeed/spi.h new file mode 100644 index 000000000000..9e952933e1f1 --- /dev/null +++ b/arch/arm/include/asm/arch-aspeed/spi.h @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (c) 2018, IBM Corporation.
• */

+#ifndef _ASM_ARCH_ASPEED_SPI_H +#define _ASM_ARCH_ASPEED_SPI_H

+/* CE Type Setting Register */ +#define CONF_ENABLE_W2 BIT(18) +#define CONF_ENABLE_W1 BIT(17) +#define CONF_ENABLE_W0 BIT(16) +#define CONF_FLASH_TYPE2 4 +#define CONF_FLASH_TYPE1 2 /* Hardwired to SPI */ +#define CONF_FLASH_TYPE0 0 /* Hardwired to SPI */ +#define CONF_FLASH_TYPE_NOR 0x0 +#define CONF_FLASH_TYPE_SPI 0x2

+/* CE Control Register */ +#define CTRL_EXTENDED2 BIT(2) /* 32 bit addressing for SPI */ +#define CTRL_EXTENDED1 BIT(1) /* 32 bit addressing for SPI */ +#define CTRL_EXTENDED0 BIT(0) /* 32 bit addressing for SPI */

+/* Interrupt Control and Status Register */ +#define INTR_CTRL_DMA_STATUS BIT(11) +#define INTR_CTRL_CMD_ABORT_STATUS BIT(10) +#define INTR_CTRL_WRITE_PROTECT_STATUS BIT(9) +#define INTR_CTRL_DMA_EN BIT(3) +#define INTR_CTRL_CMD_ABORT_EN BIT(2) +#define INTR_CTRL_WRITE_PROTECT_EN BIT(1)

+/* CEx Control Register */ +#define CE_CTRL_IO_MODE_MASK GENMASK(30, 28) +#define CE_CTRL_IO_DUAL_DATA BIT(29) +#define CE_CTRL_IO_DUAL_ADDR_DATA (BIT(29) | BIT(28)) +#define CE_CTRL_CMD_SHIFT 16 +#define CE_CTRL_CMD_MASK 0xff +#define CE_CTRL_CMD(cmd) \

•   (((cmd) & CE_CTRL_CMD_MASK) << CE_CTRL_CMD_SHIFT)
  

+#define CE_CTRL_DUMMY_HIGH_SHIFT 14 +#define CE_CTRL_CLOCK_FREQ_SHIFT 8 +#define CE_CTRL_CLOCK_FREQ_MASK 0xf +#define CE_CTRL_CLOCK_FREQ(div) \

•   (((div) & CE_CTRL_CLOCK_FREQ_MASK) << CE_CTRL_CLOCK_FREQ_SHIFT)
  

Do you need this, and other macros like it? I suppose you do use them twice in the code, at least.

CE_CTRL_CLOCK_FREQ() is used twice in aspeed_spi_flash_init().

Are you suggesting that we should not use such macros ? I agree this one is not very useful.

...

+#define CE_CTRL_DUMMY_LOW_SHIFT 6 /* 2 bits [7:6] */ +#define CE_CTRL_DUMMY(dummy) \

•   (((((dummy) >> 2) & 0x1) << CE_CTRL_DUMMY_HIGH_SHIFT) |  \
  

•    (((dummy) & 0x3) << CE_CTRL_DUMMY_LOW_SHIFT))
  

I think this needs MASK values instead of open-coding them.

yes. That's a copy/paste from Linux. I will fix.

...

+#define CE_CTRL_STOP_ACTIVE BIT(2) +#define CE_CTRL_MODE_MASK 0x3 +#define CE_CTRL_READMODE 0x0 +#define CE_CTRL_FREADMODE 0x1 +#define CE_CTRL_WRITEMODE 0x2 +#define CE_CTRL_USERMODE 0x3

+/*

• • The Segment Register uses a 8MB unit to encode the start address
• • and the end address of the ABH window of a SPI flash device.

AHB?

yes !

...

• • Default segment addresses are :
• • CE0 0x20000000 - 0x2FFFFFFF 128MB
• • CE1 0x28000000 - 0x29FFFFFF 32MB
• • CE2 0x2A000000 - 0x2BFFFFFF 32MB

Can you use local-case hex for consistency?

ok.

...

• • The full address space of the AHB window of the controller is
• • covered and CE0 start address and CE2 end addresses are read-only.
• */

+#define SEGMENT_ADDR_START(reg) ((((reg) >> 16) & 0xff) << 23) +#define SEGMENT_ADDR_END(reg) ((((reg) >> 24) & 0xff) << 23) +#define SEGMENT_ADDR_VALUE(start, end) \

•   (((((start) >> 23) & 0xff) << 16) | ((((end) >> 23) & 0xff) << 24))
  

I think these should be done as MASK and SHIFT values instead. They are only used once int he code.

OK. That's a copy/paste from Linux again. I will fix.

...

+/* DMA Control/Status Register */ +#define DMA_CTRL_DELAY_SHIFT 8 +#define DMA_CTRL_DELAY_MASK 0xf +#define DMA_CTRL_FREQ_SHIFT 4 +#define DMA_CTRL_FREQ_MASK 0xf +#define TIMING_MASK(div, delay) \

•   (((delay & DMA_CTRL_DELAY_MASK) << DMA_CTRL_DELAY_SHIFT) | \
  

•    ((div & DMA_CTRL_FREQ_MASK) << DMA_CTRL_FREQ_SHIFT))
  

Just move this code down below?

'below' as 'closer' to aspeed_spi_fmc_checksum() ?

...

+#define DMA_CTRL_CALIB BIT(3) +#define DMA_CTRL_CKSUM BIT(2) +#define DMA_CTRL_WRITE BIT(1) +#define DMA_CTRL_ENABLE BIT(0)

+#define ASPEED_SPI_MAX_CS 3

+struct aspeed_spi_regs {

•   u32 conf;                       /* 0x00 CE Type Setting */
  

•   u32 ctrl;                       /* 0x04 Control */
  

•   u32 intr_ctrl;                  /* 0x08 Interrupt Control and Status */
  

•   u32 cmd_ctrl;                   /* 0x0C Command Control */
  

•   u32 ce_ctrl[ASPEED_SPI_MAX_CS]; /* 0x10 .. 0x18 CEx Control */
  

•   u32 _reserved0[5];              /* .. */
  

•   u32 segment_addr[ASPEED_SPI_MAX_CS];
  

•                                   /* 0x30 .. 0x38 Segment Address */
  

•   u32 _reserved1[17];             /* .. */
  

•   u32 dma_ctrl;                   /* 0x80 DMA Control/Status */
  

•   u32 dma_flash_addr;             /* 0x84 DMA Flash Side Address */
  

•   u32 dma_dram_addr;              /* 0x88 DMA DRAM Side Address */
  

•   u32 dma_len;                    /* 0x8C DMA Length Register */
  

•   u32 dma_checksum;               /* 0x8C Checksum Calculation Result */
  

•   u32 timings;                    /* 0x94 Read Timing Compensation */
  

•   /* not used */
  

•   u32 soft_strap_status;          /* 0x9C Software Strap Status */
  

•   u32 write_cmd_filter_ctrl;      /* 0xA0 Write Command Filter Control */
  

•   u32 write_addr_filter_ctrl;     /* 0xA4 Write Address Filter Control */
  

•   u32 lock_ctrl_reset;            /* 0xA8 Lock Control (SRST#) */
  

•   u32 lock_ctrl_wdt;              /* 0xAC Lock Control (Watchdog) */
  

•   u32 write_addr_filter[5];       /* 0xB0 Write Address Filter */
  

Lower-case hex again

ok.

...

+};

+#endif /* _ASM_ARCH_ASPEED_SPI_H */ diff --git a/drivers/spi/aspeed_spi.c b/drivers/spi/aspeed_spi.c new file mode 100644 index 000000000000..7f1a39d8e698 --- /dev/null +++ b/drivers/spi/aspeed_spi.c @@ -0,0 +1,788 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • ASPEED AST2500 FMC/SPI Controller driver
• • Copyright (c) 2015-2018, IBM Corporation.
• */

+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <common.h> +#include <clk.h> +#include <dm.h> +#include <asm/io.h> +#include <asm/arch/spi.h> +#include <linux/ioport.h> +#include <spi.h> +#include <spi_flash.h>

These last two should go immediately below dm.h

ok. I understand the coding standard now.

...

+DECLARE_GLOBAL_DATA_PTR;

Do you need this?

no. It's a left over.

...

+struct aspeed_spi_flash {

struct comment - what is this for?

...

•   u8              cs;
  

•   bool            init;           /* Initialized when the SPI bus is
  

•                                    * first claimed
  

•                                    */
  

Please avoid this type of comment - either put it before the line, or add a struct comment with each member listed.

yes. This will be better.

Also, can you drop this variable and do the init in the probe() method instead?

I haven't found a good way to do this.

The problem is that the SPI slave flash devices are not available yet when the controller is probed. So I am using the claim_bus() method to initialize the settings related to each SPI device.

This is what the struct aspeed_spi_flash is about.

...

•   void __iomem    *ahb_base;      /* AHB Window for this device */
  

Should that be a struct *?

no. This is really just the memory address where the flash contents is mapped. Depending on the configured controller's mode, accesses will be interpreted as direct to the flash contents or as a command/control way to do SPI transfers.

But the controller has no registers behind this address.

...

•   u32             ahb_size;       /* AHB Window segment size */
  

•   u32             ce_ctrl_user;   /* CE Control Register for USER mode */
  

•   u32             ce_ctrl_fread;  /* CE Control Register for FREAD mode */
  

•   u32             iomode;
  

•   struct spi_flash *spi;          /* Associated SPI Flash device */
  

You should not need this struct here with driver model.

OK. I think I can simplify as I only need the size and the dummy_bytes from the spi_flash struct. It felt convenient at the time.

...

+};

+struct aspeed_spi_priv {

•   struct aspeed_spi_regs  *regs;
  

•   void __iomem    *ahb_base;      /* AHB Window for all flash devices */
  

•   u32             ahb_size;       /* AHB Window segments size */
  

•   ulong           hclk_rate;      /* AHB clock rate */
  

•   u32             max_hz;
  

•   u8              num_cs;
  

•   bool            is_fmc;
  

•   struct aspeed_spi_flash flashes[ASPEED_SPI_MAX_CS];
  

SPI flash should be modelled as UCLASS_SPI_FLASH devices. Much of the code in here looks like it should move to a separate driver.

This struct aspeed_spi_flash holds all the parameters related to a SPI slave flash device. The confusion surely comes from the fact the driver looks like the SPI-NOR driver we have in Linux.

...

•   u32             flash_count;
  

•   u8              cmd_buf[16];    /* SPI command in progress */
  

•   size_t          cmd_len;
  

+};

+static struct aspeed_spi_flash *aspeed_spi_get_flash(struct udevice *dev) +{

•   struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
  

•   struct aspeed_spi_priv *priv = dev_get_priv(dev->parent);
  

•   u8 cs = slave_plat->cs;
  

•   if (cs >= priv->flash_count) {
  

•           pr_err("invalid CS %u\n", cs);
  

•           return NULL;
  

•   }
  

•   return &priv->flashes[cs];
  

+}

+static u32 aspeed_spi_hclk_divisor(u32 hclk_rate, u32 max_hz)

Function comment

ok.

...

+{

•   /* HCLK/1 ..    HCLK/16 */
  

•   const u8 hclk_divisors[] = {
  

•           15, 7, 14, 6, 13, 5, 12, 4, 11, 3, 10, 2, 9, 1, 8, 0
  

•   };
  

•   u32 i;
  

int or uint. This does not need to be 32-bit.

yes.

...

•   for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
  

•           if (max_hz >= (hclk_rate / (i + 1)))
  

•                   break;
  

•   }
  

•   debug("hclk=%d required=%d divisor is %d (mask %x) speed=%d\n",
  

•         hclk_rate, max_hz, i + 1, hclk_divisors[i], hclk_rate / (i + 1));
  

•   return hclk_divisors[i];
  

+}

+/*

• • Use some address/size under the first flash device CE0

Please add a function comment with purpose, args and return value. Same throughout.

ok.

...

• */

+static u32 aspeed_spi_fmc_checksum(struct aspeed_spi_priv *priv, u8 div,

•                              u8 delay)
  

+{

•   u32 flash_addr = (u32)priv->ahb_base + 0x10000;
  

What is happening here? The 0x10000 offset should be in the device tree, I think.

hmm. This address points to some random data which will be read to tune the SPI read delay cycles. In this case, it points somewhere in the u-boot .text section.

I am not sure it belongs to the device tree but it deserves a define at minimum. I haven't seen other devices with similar needs.

Should cast a pointer to ulong, not u32. Also, perhaps ahb_base should be a ulong instead of a pointer?

yes. That might remove a few other casts. I will try it out.

...

•   u32 flash_len = 0x200;
  

•   u32 dma_ctrl;
  

•   u32 checksum;
  

•   writel(flash_addr, &priv->regs->dma_flash_addr);
  

•   writel(flash_len,  &priv->regs->dma_len);
  

•   /*
  

•    * When doing calibration, the SPI clock rate in the CE0
  

•    * Control Register and the read delay cycles in the Read
  

•    * Timing Compensation Register are replaced by bit[11:4].
  

•    */
  

•   dma_ctrl = DMA_CTRL_ENABLE | DMA_CTRL_CKSUM | DMA_CTRL_CALIB |
  

•           TIMING_MASK(div, delay);
  

•   writel(dma_ctrl, &priv->regs->dma_ctrl);
  

•   while (!(readl(&priv->regs->intr_ctrl) & INTR_CTRL_DMA_STATUS))
  

•           ;
  

I assume this never times out?

Indeed. No timeouts.

...

•   writel(0x0, &priv->regs->intr_ctrl);
  

•   checksum = readl(&priv->regs->dma_checksum);
  

•   writel(0x0, &priv->regs->dma_ctrl);
  

•   return checksum;
  

+}

+static u32 aspeed_spi_read_checksum(struct aspeed_spi_priv *priv, u8 div,

•                               u8 delay)
  

+{

•   /* TODO: the SPI controllers do not have the DMA registers.
  

/*

• TODO(email) :...
• ...

*/

ok. This is a good pratice. I will keep in it mind.

...

•    * The algorithm is the same.
  

•    */
  

•   if (!priv->is_fmc) {
  

•           pr_warn("No timing calibration support for SPI controllers");
  

•           return 0xbadc0de;
  

What is this value? Can it be a #define?

yes it can be a define.

It's a bogus checksum value for the SPI controllers, which control the flash devices for the host firmware. u-boot should not care about these.

...

•   }
  

•   return aspeed_spi_fmc_checksum(priv, div, delay);
  

+}

+static int aspeed_spi_timing_calibration(struct aspeed_spi_priv *priv) +{

•   /* HCLK/5 .. HCLK/1 */
  

•   const u8 hclk_divisors[] = { 13, 6, 14, 7, 15 };
  

•   u32 timing_reg = 0;
  

•   u32 checksum, gold_checksum;
  

•   int i, j;
  

•   debug("Read timing calibration :\n");
  

•   /* Compute reference checksum at lowest freq HCLK/16 */
  

•   gold_checksum = aspeed_spi_read_checksum(priv, 0, 0);
  

•   /*
  

•    * Set CE0 Control Register to FAST READ command mode. The
  

•    * HCLK divisor will be set through the DMA Control Register.
  

•    */
  

•   writel(CE_CTRL_CMD(0xB) | CE_CTRL_DUMMY(1) | CE_CTRL_FREADMODE,
  

•          &priv->regs->ce_ctrl[0]);
  

•   /* Increase HCLK freq */
  

•   for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
  

•           u32 hdiv = 5 - i;
  

•           u32 hshift = (hdiv - 1) << 2;
  

•           bool pass = false;
  

•           u8 delay;
  

•           if (priv->hclk_rate / hdiv > priv->max_hz) {
  

•                   debug("skipping freq %ld\n", priv->hclk_rate / hdiv);
  

•                   continue;
  

•           }
  

•           /* Increase HCLK delays until read succeeds */
  

•           for (j = 0; j < 6; j++) {
  

•                   /* Try first with a 4ns DI delay */
  

•                   delay = 0x8 | j;
  

•                   checksum = aspeed_spi_read_checksum(
  

•                           priv, hclk_divisors[i], delay);
  

•                   pass = (checksum == gold_checksum);
  

•                   debug(" HCLK/%d, 4ns DI delay, %d HCLK delay : %s\n",
  

•                         hdiv, j, pass ? "PASS" : "FAIL");
  

•                   /* Try again with more HCLK delays */
  

•                   if (!pass)
  

•                           continue;
  

•                   /* Try without the 4ns DI delay */
  

•                   delay = j;
  

•                   checksum = aspeed_spi_read_checksum(
  

•                           priv, hclk_divisors[i], delay);
  

•                   pass = (checksum == gold_checksum);
  

•                   debug(" HCLK/%d, 0ns DI delay, %d HCLK delay : %s\n",
  

•                         hdiv, j, pass ? "PASS" : "FAIL");
  

•                   /* All good for this freq  */
  

•                   if (pass)
  

•                           break;
  

•           }
  

•           if (pass) {
  

•                   timing_reg &= ~(0xfu << hshift);
  

•                   timing_reg |= delay << hshift;
  

•           }
  

•   }
  

•   debug("Timing Register set to 0x%08x\n", timing_reg);
  

•   writel(timing_reg, &priv->regs->timings);
  

•   /* Reset CE0 Control Register */
  

•   writel(0x0, &priv->regs->ce_ctrl[0]);
  

•   return 0;
  

+}

+static int aspeed_spi_controller_init(struct aspeed_spi_priv *priv) +{

•   int cs, ret;
  

•   /*
  

•    * Enable write on all flash devices as USER command mode
  

•    * requires it.
  

•    */
  

•   setbits_le32(&priv->regs->conf,
  

•                CONF_ENABLE_W2 | CONF_ENABLE_W1 | CONF_ENABLE_W0);
  

•   /*
  

•    * Set the Read Timing Compensation Register. This setting
  

•    * applies to all devices.
  

•    */
  

•   ret = aspeed_spi_timing_calibration(priv);
  

•   if (ret)
  

•           return ret;
  

•   /*
  

•    * Set safe default settings for each device. These will be
  

•    * tuned after the SPI flash devices are probed.
  

•    */
  

•   for (cs = 0; cs < priv->flash_count; cs++) {
  

•           struct aspeed_spi_flash *flash = &priv->flashes[cs];
  

•           u32 seg_addr = readl(&priv->regs->segment_addr[cs]);
  

•           /*
  

•            * The start address of the AHB window of CE0 is
  

•            * read-only and is the same as the address of the
  

•            * overall AHB window of the controller for all flash
  

•            * devices.
  

•            */
  

•           flash->ahb_base = cs ? (void *)SEGMENT_ADDR_START(seg_addr) :
  

•                   priv->ahb_base;
  

•           flash->cs = cs;
  

•           flash->ce_ctrl_user = CE_CTRL_USERMODE;
  

•           flash->ce_ctrl_fread = CE_CTRL_READMODE;
  

•   }
  

•   return 0;
  

+}

+static int aspeed_spi_read_from_ahb(void __iomem *ahb_base, void *buf,

•                               size_t len)
  

+{

•   size_t offset = 0;
  

•   if (!((uintptr_t)buf % 4)) {
  

•           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 (!((uintptr_t)buf % 4)) {
  

•           writesl(ahb_base, buf, len >> 2);
  

•           offset = len & ~0x3;
  

•           len -= offset;
  

•   }
  

•   writesb(ahb_base, (u8 *)buf + offset, len);
  

•   return 0;
  

+}

+static void aspeed_spi_start_user(struct aspeed_spi_priv *priv,

•                             struct aspeed_spi_flash *flash)
  

+{

•   u32 ctrl_reg = flash->ce_ctrl_user | CE_CTRL_STOP_ACTIVE;
  

•   /* Unselect CS and set USER command mode */
  

Deselect?

yes :)

...

•   writel(ctrl_reg, &priv->regs->ce_ctrl[flash->cs]);
  

•   /* Select CS */
  

•   clrbits_le32(&priv->regs->ce_ctrl[flash->cs], CE_CTRL_STOP_ACTIVE);
  

+}

+static void aspeed_spi_stop_user(struct aspeed_spi_priv *priv,

•                            struct aspeed_spi_flash *flash)
  

+{

•   /* Unselect CS first */
  

•   setbits_le32(&priv->regs->ce_ctrl[flash->cs], CE_CTRL_STOP_ACTIVE);
  

•   /* Restore default command mode */
  

•   writel(flash->ce_ctrl_fread, &priv->regs->ce_ctrl[flash->cs]);
  

+}

+static int aspeed_spi_read_reg(struct aspeed_spi_priv *priv,

•                          struct aspeed_spi_flash *flash,
  

•                          u8 opcode, u8 *read_buf, int len)
  

+{

•   aspeed_spi_start_user(priv, flash);
  

•   aspeed_spi_write_to_ahb(flash->ahb_base, &opcode, 1);
  

•   aspeed_spi_read_from_ahb(flash->ahb_base, read_buf, len);
  

•   aspeed_spi_stop_user(priv, flash);
  

Please add blank line before return

ok

...

•   return 0;
  

+}

+static int aspeed_spi_write_reg(struct aspeed_spi_priv *priv,

•                           struct aspeed_spi_flash *flash,
  

•                           u8 opcode, const u8 *write_buf, int len)
  

+{

•   aspeed_spi_start_user(priv, flash);
  

•   aspeed_spi_write_to_ahb(flash->ahb_base, &opcode, 1);
  

•   aspeed_spi_write_to_ahb(flash->ahb_base, write_buf, len);
  

•   aspeed_spi_stop_user(priv, flash);
  

•   return 0;
  

+}

+static void aspeed_spi_send_cmd_addr(struct aspeed_spi_priv *priv,

•                                struct aspeed_spi_flash *flash,
  

•                                const u8 *cmdbuf, unsigned int cmdlen)
  

+{

•   int i;
  

•   u8 byte0 = 0x0;
  

•   u8 addrlen = cmdlen - 1;
  

•   /* First, send the opcode */
  

•   aspeed_spi_write_to_ahb(flash->ahb_base, &cmdbuf[0], 1);
  

•   /*
  

•    * The controller is configured for 4BYTE address mode. Fix
  

•    * the address width and send an extra byte if the SPI Flash
  

•    * layer is still 3 bytes addresses.
  

•    */
  

•   if (addrlen == 3 && readl(&priv->regs->ctrl) & BIT(flash->cs))
  

•           aspeed_spi_write_to_ahb(flash->ahb_base, &byte0, 1);
  

•   /* Then the address */
  

•   for (i = 1 ; i < cmdlen; i++)
  

•           aspeed_spi_write_to_ahb(flash->ahb_base, &cmdbuf[i], 1);
  

+}

+static ssize_t aspeed_spi_read_user(struct aspeed_spi_priv *priv,

•                               struct aspeed_spi_flash *flash,
  

•                               unsigned int cmdlen, const u8 *cmdbuf,
  

•                               unsigned int len, u8 *read_buf)
  

+{

•   u8 dummy = 0xFF;
  

•   int i;
  

•   aspeed_spi_start_user(priv, flash);
  

•   /* cmd buffer = cmd + addr + dummies */
  

•   aspeed_spi_send_cmd_addr(priv, flash, cmdbuf,
  

•                            cmdlen - flash->spi->dummy_byte);
  

•   for (i = 0 ; i < flash->spi->dummy_byte; i++)
  

•           aspeed_spi_write_to_ahb(flash->ahb_base, &dummy, 1);
  

•   if (flash->iomode) {
  

•           clrbits_le32(&priv->regs->ce_ctrl[flash->cs],
  

•                        CE_CTRL_IO_MODE_MASK);
  

•           setbits_le32(&priv->regs->ce_ctrl[flash->cs], flash->iomode);
  

•   }
  

•   aspeed_spi_read_from_ahb(flash->ahb_base, read_buf, len);
  

•   aspeed_spi_stop_user(priv, flash);
  

•   return 0;
  

+}

+static ssize_t aspeed_spi_write_user(struct aspeed_spi_priv *priv,

•                                struct aspeed_spi_flash *flash,
  

•                                unsigned int cmdlen, const u8 *cmdbuf,
  

•                                unsigned int len,  const u8 *write_buf)
  

+{

•   aspeed_spi_start_user(priv, flash);
  

•   /* cmd buffer = cmd + addr */
  

•   aspeed_spi_send_cmd_addr(priv, flash, cmdbuf, cmdlen);
  

•   aspeed_spi_write_to_ahb(flash->ahb_base, write_buf, len);
  

•   aspeed_spi_stop_user(priv, flash);
  

•   return 0;
  

+}

+static u32 aspeed_spi_flash_to_addr(struct aspeed_spi_flash *flash,

•                               const u8 *cmdbuf, unsigned int cmdlen)
  

+{

•   /* cmd buffer = cmd + addr + dummies */
  

•   u8 addrlen = cmdlen - 1;
  

•   u32 addr = (cmdbuf[1] << 16) | (cmdbuf[2] << 8) | cmdbuf[3];
  

•   /* U-Boot SPI Flash layer does not support 4BYTE address mode yet */
  

Are you sure? I did see some BAR stuff.

ah. I see that, through the Read Extended Address Register. I haven't tried it. I will see how it works and remove the comments from the patch.

...

•   if (addrlen == 4)
  

•           addr = (addr << 8) | cmdbuf[4];
  

•   return addr;
  

+}

+/* TODO: add support for XFER_MMAP instead ? */ +static ssize_t aspeed_spi_read(struct aspeed_spi_priv *priv,

•                          struct aspeed_spi_flash *flash,
  

•                          unsigned int cmdlen, const u8 *cmdbuf,
  

•                          unsigned int len, u8 *read_buf)
  

+{

•   u32 offset = aspeed_spi_flash_to_addr(flash, cmdbuf,
  

•                                         cmdlen - flash->spi->dummy_byte);
  

•   /*
  

•    * Switch to USER command mode if the AHB window configured
  

•    * for the device is too small for the read operation
  

•    */
  

•   if (offset + len >= flash->ahb_size) {
  

•           return aspeed_spi_read_user(priv, flash, cmdlen, cmdbuf,
  

•                                       len, read_buf);
  

•   }
  

•   memcpy_fromio(read_buf, flash->ahb_base + offset, len);
  

•   return 0;
  

+}

+static int aspeed_spi_xfer(struct udevice *dev, unsigned int bitlen,

•                      const void *dout, void *din, unsigned long flags)
  

+{

•   struct udevice *bus = dev->parent;
  

•   struct aspeed_spi_priv *priv = dev_get_priv(bus);
  

•   struct aspeed_spi_flash *flash;
  

•   u8 *cmd_buf = priv->cmd_buf;
  

•   size_t data_bytes;
  

•   int err = 0;
  

•   flash = aspeed_spi_get_flash(dev);
  

•   if (!flash)
  

•           return -ENODEV;
  

-ENXIO perhaps? There is already a device since struct udevice *dev is valid. So you cannot return -ENODEV.

yes. ENXIO is better.

...

•   if (flags & SPI_XFER_BEGIN) {
  

•           /* save command in progress */
  

•           priv->cmd_len = bitlen / 8;
  

•           memcpy(cmd_buf, dout, priv->cmd_len);
  

•   }
  

•   if (flags == (SPI_XFER_BEGIN | SPI_XFER_END)) {
  

•           /* if start and end bit are set, the data bytes is 0. */
  

•           data_bytes = 0;
  

•   } else {
  

•           data_bytes = bitlen / 8;
  

•   }
  

•   debug("CS%u: %s cmd %zu bytes data %zu bytes\n", flash->cs,
  

•         din ? "read" : "write", priv->cmd_len, data_bytes);
  

•   if ((flags & SPI_XFER_END) || flags == 0) {
  

•           if (priv->cmd_len == 0) {
  

•                   pr_err("No command is progress !\n");
  

•                   return -1;
  

•           }
  

•           if (din && data_bytes) {
  

•                   if (priv->cmd_len == 1)
  

•                           err = aspeed_spi_read_reg(priv, flash,
  

•                                                     cmd_buf[0],
  

•                                                     din, data_bytes);
  

•                   else
  

•                           err = aspeed_spi_read(priv, flash,
  

•                                                 priv->cmd_len,
  

•                                                 cmd_buf, data_bytes,
  

•                                                 din);
  

•           } else if (dout) {
  

•                   if (priv->cmd_len == 1)
  

•                           err = aspeed_spi_write_reg(priv, flash,
  

•                                                      cmd_buf[0],
  

•                                                      dout, data_bytes);
  

•                   else
  

•                           err = aspeed_spi_write_user(priv, flash,
  

•                                                       priv->cmd_len,
  

•                                                       cmd_buf, data_bytes,
  

•                                                       dout);
  

•           }
  

•           if (flags & SPI_XFER_END) {
  

•                   /* clear command */
  

•                   memset(cmd_buf, 0, sizeof(priv->cmd_buf));
  

•                   priv->cmd_len = 0;
  

•           }
  

•   }
  

•   return err;
  

+}

+static int aspeed_spi_child_pre_probe(struct udevice *dev) +{

•   struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
  

•   debug("pre_probe slave device on CS%u, max_hz %u, mode 0x%x.\n",
  

•         slave_plat->cs, slave_plat->max_hz, slave_plat->mode);
  

•   if (!aspeed_spi_get_flash(dev))
  

•           return -ENODEV;
  

-ENXIO, same below also

ok

...

•   return 0;
  

+}

+/*

• • It is possible to automatically define a contiguous address space
• • on top of all CEs in the AHB window of the controller but it would
• • require much more work. Let's start with a simple mapping scheme
• • which should work fine for a single flash device.
• • More complex schemes should probably be defined with the device
• • tree.
• */

+static int aspeed_spi_flash_set_segment(struct aspeed_spi_priv *priv,

•                                   struct aspeed_spi_flash *flash)
  

+{

•   u32 seg_addr;
  

•   /* could be configured through the device tree */
  

•   flash->ahb_size = flash->spi->size;
  

•   seg_addr = SEGMENT_ADDR_VALUE((u32)flash->ahb_base,
  

•                                 (u32)flash->ahb_base + flash->ahb_size);
  

•   writel(seg_addr, &priv->regs->segment_addr[flash->cs]);
  

•   return 0;
  

+}

+/*

• • The Aspeed FMC controller automatically detects at boot time if a
• • flash device is in 4BYTE address mode and self configures to use
• • the appropriate address width. This can be a problem for the U-Boot
• • SPI Flash layer which only supports 3 byte addresses. In such a
• • case, a warning is emitted and the address width is fixed when sent
• • on the bus.
• */

+static void aspeed_spi_flash_check_4b(struct aspeed_spi_priv *priv,

•                                 struct aspeed_spi_flash *flash)
  

+{

•   if (readl(&priv->regs->ctrl) & BIT(flash->cs))
  

•           pr_err("CS%u: 4BYTE address mode is active\n", flash->cs);
  

+}

+static int aspeed_spi_flash_init(struct aspeed_spi_priv *priv,

•                            struct aspeed_spi_flash *flash,
  

•                            struct udevice *dev)
  

+{

•   struct spi_flash *spi_flash = dev_get_uclass_priv(dev);
  

•   struct spi_slave *slave = dev_get_parent_priv(dev);
  

•   u32 user_hclk;
  

•   u32 read_hclk;
  

•   /*
  

•    * The SPI flash device slave should not change, so initialize
  

•    * it only once.
  

•    */
  

•   if (flash->init)
  

•           return 0;
  

Why does the init happen here?

I would rather do it under the probe routine but the flash device is probed later in the sequence. I do agree it's a bit awkard and looks like a work around.

We could also do the setting each time the device claims the bus, like in the stm32_qspi driver ?

...

•   /*
  

•    * The flash device has not been probed yet. Initial transfers
  

•    * to read the JEDEC of the device will use the initial
  

•    * default settings of the registers.
  

•    */
  

•   if (!spi_flash->name)
  

•           return 0;
  

•   debug("CS%u: init %s flags:%x size:%d page:%d sector:%d erase:%d "
  

•         "cmds [ erase:%x read=%x write:%x ] dummy:%d mmap:%p\n",
  

•         flash->cs,
  

•         spi_flash->name, spi_flash->flags, spi_flash->size,
  

•         spi_flash->page_size, spi_flash->sector_size,
  

•         spi_flash->erase_size, spi_flash->erase_cmd,
  

•         spi_flash->read_cmd, spi_flash->write_cmd,
  

•         spi_flash->dummy_byte, spi_flash->memory_map);
  

•   flash->spi = spi_flash;
  

•   /*
  

•    * Tune the CE Control Register values for the modes the
  

•    * driver will use:
  

•    *   - USER command for specific SPI commands, write and
  

•    *     erase.
  

•    *   - FAST READ command mode for reads. The flash device is
  

•    *     directly accessed through its AHB window.
  

•    *
  

•    * TODO: introduce a DT property for writes ?
  

TODO(email)

yes

...

•    */
  

•   user_hclk = 0;
  

•   flash->ce_ctrl_user = CE_CTRL_CLOCK_FREQ(user_hclk) |
  

•           CE_CTRL_USERMODE;
  

•   read_hclk = aspeed_spi_hclk_divisor(priv->hclk_rate, slave->speed);
  

•   if (slave->mode & (SPI_RX_DUAL | SPI_TX_DUAL)) {
  

•           debug("CS%u: setting dual data mode\n", flash->cs);
  

•           flash->iomode = CE_CTRL_IO_DUAL_DATA;
  

•   }
  

•   flash->ce_ctrl_fread = CE_CTRL_CLOCK_FREQ(read_hclk) |
  

•           flash->iomode |
  

•           CE_CTRL_CMD(flash->spi->read_cmd) |
  

•           CE_CTRL_DUMMY(flash->spi->dummy_byte) |
  

•           CE_CTRL_FREADMODE;
  

•   debug("CS%u: USER mode 0x%08x FREAD mode 0x%08x\n", flash->cs,
  

•         flash->ce_ctrl_user, flash->ce_ctrl_fread);
  

•   /* Set the CE Control Register default (FAST READ) */
  

•   writel(flash->ce_ctrl_fread, &priv->regs->ce_ctrl[flash->cs]);
  

•   /* Enable 4BYTE addresses */
  

•   if (flash->spi->size >= 16 << 20)
  

•           aspeed_spi_flash_check_4b(priv, flash);
  

•   /* Set Address Segment Register for direct AHB accesses */
  

•   aspeed_spi_flash_set_segment(priv, flash);
  

•   /* All done */
  

•   flash->init = true;
  

•   return 0;
  

+}

+static int aspeed_spi_claim_bus(struct udevice *dev) +{

•   struct udevice *bus = dev->parent;
  

•   struct aspeed_spi_priv *priv = dev_get_priv(bus);
  

•   struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
  

•   struct aspeed_spi_flash *flash;
  

•   debug("%s: claim bus CS%u\n", bus->name, slave_plat->cs);Casting a pointer to
  

•   flash = aspeed_spi_get_flash(dev);
  

•   if (!flash)
  

•           return -ENODEV;
  

•   return aspeed_spi_flash_init(priv, flash, dev);
  

+}

+static int aspeed_spi_release_bus(struct udevice *dev) +{

•   struct udevice *bus = dev->parent;
  

•   struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
  

•   debug("%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) +{

•   debug("%s: setting mode to %x\n", bus->name, mode);
  

•   if (mode & (SPI_RX_QUAD | SPI_TX_QUAD)) {
  

•           pr_err("%s invalid QUAD IO mode\n", bus->name);
  

•           return -EINVAL;
  

•   }
  

•   /* The CE Control Register is set in claim_bus() */
  

•   return 0;
  

+}

+static int aspeed_spi_set_speed(struct udevice *bus, uint hz) +{

•   debug("%s: setting speed to %u\n", bus->name, hz);
  

•   /* The CE Control Register is set in claim_bus() */
  

•   return 0;
  

+}

+static int aspeed_spi_count_flash_devices(struct udevice *bus) +{

•   ofnode node;
  

•   int count = 0;
  

•   dev_for_each_subnode(node, bus) {
  

•           if (ofnode_is_available(node) &&
  

•               ofnode_device_is_compatible(node, "spi-flash"))
  

•                   count++;
  

•   }
  

•   return count;
  

+}

+static int aspeed_spi_bind(struct udevice *bus) +{

•   debug("%s assigned req_seq=%d seq=%d\n", bus->name, bus->req_seq,
  

•         bus->seq);
  

•   return 0;
  

+}

+static int aspeed_spi_probe(struct udevice *bus) +{

•   struct resource res_regs, res_ahb;
  

•   struct aspeed_spi_priv *priv = dev_get_priv(bus);
  

•   struct clk hclk;
  

•   int ret;
  

•   ret = dev_read_resource(bus, 0, &res_regs);
  

•   if (ret < 0)
  

•           return ret;
  

•   priv->regs = (void __iomem *)res_regs.start;
  

•   ret = dev_read_resource(bus, 1, &res_ahb);
  

•   if (ret < 0)
  

•           return ret;
  

•   priv->ahb_base = (void __iomem *)res_ahb.start;
  

•   priv->ahb_size = res_ahb.end - res_ahb.start;
  

•   ret = clk_get_by_index(bus, 0, &hclk);
  

•   if (ret < 0) {
  

•           pr_err("%s could not get clock: %d\n", bus->name, ret);
  

•           return ret;
  

•   }
  

•   priv->hclk_rate = clk_get_rate(&hclk);
  

•   clk_free(&hclk);
  

•   priv->max_hz = dev_read_u32_default(bus, "spi-max-frequency",
  

•                                       100000000);
  

•   priv->num_cs = dev_read_u32_default(bus, "num-cs", ASPEED_SPI_MAX_CS);
  

•   priv->flash_count = aspeed_spi_count_flash_devices(bus);
  

•   if (priv->flash_count > priv->num_cs) {
  

•           pr_err("%s has too many flash devices: %d\n", bus->name,
  

•                  priv->flash_count);
  

•           return -EINVAL;
  

•   }
  

•   if (!priv->flash_count) {
  

•           pr_err("%s has no flash devices ?!\n", bus->name);
  

•           return -ENODEV;
  

•   }
  

•   /*
  

•    * There are some slight differences between the FMC and the
  

•    * SPI controllers
  

•    */
  

•   priv->is_fmc = device_is_compatible(bus, "aspeed,ast2500-fmc");
  

•   ret = aspeed_spi_controller_init(priv);
  

•   if (ret)
  

•           return ret;
  

•   debug("%s probed regs=%p ahb_base=%p max-hz=%d cs=%d seq=%d\n",
  

•         bus->name, priv->regs, priv->ahb_base, priv->max_hz,
  

•         priv->flash_count, bus->seq);
  

•   return 0;
  

+}

+static const struct dm_spi_ops aspeed_spi_ops = {

•   .claim_bus      = aspeed_spi_claim_bus,
  

•   .release_bus    = aspeed_spi_release_bus,
  

•   .set_mode       = aspeed_spi_set_mode,
  

•   .set_speed      = aspeed_spi_set_speed,
  

•   .xfer           = aspeed_spi_xfer,
  

+};

+static const struct udevice_id aspeed_spi_ids[] = {

•   { .compatible = "aspeed,ast2500-fmc" },
  

•   { .compatible = "aspeed,ast2500-spi" },
  

•   { }
  

+};

+U_BOOT_DRIVER(aspeed_spi) = {

•   .name = "aspeed_spi",
  

•   .id = UCLASS_SPI,
  

•   .of_match = aspeed_spi_ids,
  

•   .ops = &aspeed_spi_ops,
  

•   .priv_auto_alloc_size = sizeof(struct aspeed_spi_priv),
  

•   .child_pre_probe = aspeed_spi_child_pre_probe,
  

•   .bind  = aspeed_spi_bind,
  

•   .probe = aspeed_spi_probe,
  

+};

This is a SPI driver so it should implement the SPI interface. It should not be messing with SPI flash things.

I would agree but the Aspeed SoC FMC controller and the two SPI controllers are designed for SPI flash memory devices (and also NOR flash memory for the FMC)

I have a hard time understanding why this driver knows about SPI flash devices?

because I made look like a SPI-NOR driver I suppose. Should it be in its own uclass category ?

...

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index dcd719ff0ac6..fd5e930ec318 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -26,6 +26,14 @@ config ALTERA_SPI IP core. Please find details on the "Embedded Peripherals IP User Guide" of Altera.

+config ASPEED_SPI

•   bool "Aspeed SPI driver"
  

•   default y if ARCH_ASPEED
  

•   help
  

•     Enable the Aspeed AST2500 FMC/SPI driver. This driver can be
  

•     used to access the SPI NOR flash on boards using the Aspeed
  

•     AST2500 SoC, such as the POWER9 OpenPOWER platforms
  

What is FMC? Can you spell that one out?

I will :

Firmware SPI Memory Controller (FMC)

Thanks for the review,

C.

...

config ATCSPI200_SPI bool "Andestech ATCSPI200 SPI driver" help diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index 728e30c5383c..40d224130ea5 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_SOFT_SPI) += soft_spi_legacy.o endif

obj-$(CONFIG_ALTERA_SPI) += altera_spi.o +obj-$(CONFIG_ASPEED_SPI) += aspeed_spi.o obj-$(CONFIG_ATH79_SPI) += ath79_spi.o obj-$(CONFIG_ATMEL_SPI) += atmel_spi.o obj-$(CONFIG_BCM63XX_HSSPI) += bcm63xx_hsspi.o -- 2.17.1

---

U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot

Regards, Simon