[U-Boot] [PATCH 0/3] Support for the Aspeed ast2500 FMC/SPI controllers

[U-Boot] [PATCH 0/3] Support for the Aspeed ast2500 FMC/SPI controllers

[Cédric Le Goater]

Hello,

This series adds a driver for the Aspeed ast2500 FMC/SPI controllers
as one can find on the POWER9 OpenPOWER platforms.

It was tested on the AST2500 evb using a w25q256 flash device.

Git tree available here:

  https://github.com/legoater/u-boot/commits/aspeed

Thanks,

C.

Cédric Le Goater (3):
  aspeed: ast2500: Add AHB clock
  spi: Add support for the Aspeed ast2500 SPI controllers
  aspeed: Add SPI support to the ast2500 Eval Board

 .../arm/include/asm/arch-aspeed/scu_ast2500.h |   2 +
 arch/arm/include/asm/arch-aspeed/spi.h        | 114 +++
 include/dt-bindings/clock/ast2500-scu.h       |   1 +
 drivers/clk/aspeed/clk_ast2500.c              |  12 +
 drivers/spi/aspeed_spi.c                      | 788 ++++++++++++++++++
 arch/arm/dts/ast2500-evb.dts                  |  17 +
 arch/arm/dts/ast2500-u-boot.dtsi              |  12 +
 arch/arm/dts/ast2500.dtsi                     |  71 ++
 configs/evb-ast2500_defconfig                 |  10 +
 drivers/spi/Kconfig                           |   8 +
 drivers/spi/Makefile                          |   1 +
 11 files changed, 1036 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-aspeed/spi.h
 create mode 100644 drivers/spi/aspeed_spi.c

-- 
2.17.1

[U-Boot] [PATCH 1/3] aspeed: ast2500: Add AHB clock

[Cédric Le Goater]

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
 
 #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;
+		}
+		break;
 	case PCLK_UART1:
 		rate = ast2500_get_uart_clk_rate(priv->scu, 1);
 		break;
-- 
2.17.1

[U-Boot] [PATCH 1/3] aspeed: ast2500: Add AHB clock

[Joel Stanley]

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)

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

[U-Boot] [PATCH 1/3] aspeed: ast2500: Add AHB clock

[Maxim Sloyko]

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?
>



-- 
*M*axim *S*loyko

[U-Boot] [PATCH 1/3] aspeed: ast2500: Add AHB clock

[Cédric Le Goater]

On 09/11/2018 06:42 PM, Maxim Sloyko wrote:
> 
> 
> On Tue, Sep 11, 2018 at 12:35 AM, Joel Stanley <joel at jms.id.au <mailto:joel@jms.id.au>> wrote:
> 
>     On Mon, 10 Sep 2018 at 23:48, Cédric Le Goater <clg at 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 at 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,

C.

[U-Boot] [PATCH 1/3] aspeed: ast2500: Add AHB clock

[Joel Stanley]

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 at jms.id.au <mailto:joel@jms.id.au>> wrote:
> >
> >     On Mon, 10 Sep 2018 at 23:48, Cédric Le Goater <clg at 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 at 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>

[U-Boot] [PATCH 1/3] aspeed: ast2500: Add AHB clock

[Simon Glass]

On 10 September 2018 at 07:16, 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(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Cédric Le Goater]

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)
+#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))
+#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.
+ * Default segment addresses are :
+ *
+ *   CE0  0x20000000 - 0x2FFFFFFF  128MB
+ *   CE1  0x28000000 - 0x29FFFFFF   32MB
+ *   CE2  0x2A000000 - 0x2BFFFFFF   32MB
+ *
+ * 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))
+
+/* 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))
+#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 */
+};
+
+#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>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct aspeed_spi_flash {
+	u8		cs;
+	bool		init;		/* Initialized when the SPI bus is
+					 * first claimed
+					 */
+	void __iomem	*ahb_base;	/* AHB Window for this device */
+	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 */
+};
+
+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];
+	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)
+{
+	/* 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;
+
+	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
+ */
+static u32 aspeed_spi_fmc_checksum(struct aspeed_spi_priv *priv, u8 div,
+				   u8 delay)
+{
+	u32 flash_addr = (u32)priv->ahb_base + 0x10000;
+	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))
+		;
+
+	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.
+	 * The algorithm is the same.
+	 */
+	if (!priv->is_fmc) {
+		pr_warn("No timing calibration support for SPI controllers");
+		return 0xbadc0de;
+	}
+
+	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 */
+	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);
+	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 */
+	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;
+
+	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;
+
+	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;
+
+	/*
+	 * 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 ?
+	 */
+	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);
+
+	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,
+};
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
+
 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] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Joel Stanley]

On Mon, 10 Sep 2018 at 23:48, 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>

Looks good. I compared some things against the data sheet, and read
through the driver. There were to small questions I had, so please
clear those up and add my:

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

> +static u32 aspeed_spi_hclk_divisor(u32 hclk_rate, u32 max_hz)
> +{
> +       /* 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;
> +
> +       for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
> +               if (max_hz >= (hclk_rate / (i + 1)))

We spoke offline about why the values in hclk_divisors are not used in
this calculation. I think we decided to change the name of
hclk_divisors to something like hclk_masks?

> +                       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];
> +}

> +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.
> +        * The algorithm is the same.
> +        */
> +       if (!priv->is_fmc) {
> +               pr_warn("No timing calibration support for SPI controllers");
> +               return 0xbadc0de;
> +       }
> +
> +       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++) {

6 here is the number of items in hclk_divisors?

> +                       /* 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;
> +}
> +

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Cédric Le Goater]

On 09/12/2018 12:38 AM, Joel Stanley wrote:
> On Mon, 10 Sep 2018 at 23:48, 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>
> 
> Looks good. I compared some things against the data sheet, and read
> through the driver. There were to small questions I had, so please
> clear those up and add my:
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
>> +static u32 aspeed_spi_hclk_divisor(u32 hclk_rate, u32 max_hz)
>> +{
>> +       /* 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;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
>> +               if (max_hz >= (hclk_rate / (i + 1)))
> 
> We spoke offline about why the values in hclk_divisors are not used in
> this calculation. I think we decided to change the name of
> hclk_divisors to something like hclk_masks?

yes. I have changed the name everywhere as it makes more sense.  
 
>> +                       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];
>> +}
> 
>> +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.
>> +        * The algorithm is the same.
>> +        */
>> +       if (!priv->is_fmc) {
>> +               pr_warn("No timing calibration support for SPI controllers");
>> +               return 0xbadc0de;
>> +       }
>> +
>> +       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++) {
> 
> 6 here is the number of items in hclk_divisors?

no. these are counts of HCLK cycles before input, possible values are [0-5]. 
I will add a define to clarify '6'
 
> 
>> +                       /* 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;
>> +}
>> +

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Simon Glass]

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,@least.

> +#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.

> +#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?

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

Can you use local-case hex for consistency?

> + *
> + * 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.

> +/* 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?

> +#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

> +};
> +
> +#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

> +
> +DECLARE_GLOBAL_DATA_PTR;

Do you need this?

> +
> +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.

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

> +       void __iomem    *ahb_base;      /* AHB Window for this device */

Should that be a struct *?

> +       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.

> +};
> +
> +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.

> +       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

> +{
> +       /* 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.

> +
> +       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.

> + */
> +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. Should cast a pointer to ulong, not u32. Also, perhaps
ahb_base should be a ulong instead of a pointer?

> +       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?

> +
> +       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) :...
 * ...
 */

> +        * 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?

> +       }
> +
> +       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?

> +       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

> +       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.

> +       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.

> +
> +       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

> +
> +       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?

> +
> +       /*
> +        * 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)

> +        */
> +       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 have a hard time understanding why this driver knows about SPI flash devices?

> 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?

> +
>  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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Regards,
Simon

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Cédric Le Goater]

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 at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
> 
> Regards,
> Simon
> 

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Simon Glass]

Hi Cedric,

On 28 September 2018 at 04:42, Cédric Le Goater <clg@kaod.org> wrote:
> 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.

OK, I am not sure of the best thing to do.

Jagan (on cc) has been working on SPI NOR, but I'm not sure of the status.

If you use UCLASS_SPI_FLASH then you can put in drivers/mtd/spi, but
if UCLASS_SPI then drivers/spi
[..]

>>> +/* 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.

Yes I think it is better to just spell it out in the code. Defining
shifts and masks is good but creating macros with them can make the
code more confusing I think and it is less common to do that in
U-Boot.

BTW the mask is normally a shifted mask:

 +#define CE_CTRL_CLOCK_FREQ_SHIFT       8
 +#define CE_CTRL_CLOCK_FREQ_MASK                (0xf <<
CE_CTRL_CLOCK_FREQ_SHIFT)

and you can put it in an anonymous enum if you prefer.

[...]

>
>>> +/* 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() ?

I mean remove the #define if it is only used once
[...]

>>> +
>>> +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.

But you should be able to access it from the struct udevice *. Thie
struct spi_flash is available using dev_get_uclass_priv(dev) where dev
is the SPI flash device.


>
>>
>>> +};
>>> +
>>> +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.

Yes, I think I have to defer to Jagan on this one.

[..]

>>> +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 ?

I think the init option is OK, basd on what you said. But please add a
comment to the var explaining why it is needed.

How come the flash is not available when the driver is probed? Could
you probe whatever else is needed first, so that this IS available?

[..]
>>
>> 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 ?
>

As above, let's check with Jagan on SPI nor.

With the x86 driver (ich.c) we got around it by trying to make the SPI
controller look like a normal SPI controller.

Regards,
Simon

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Cédric Le Goater]

Hello Simon,

[...]

>>>> +/* 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.
> 
> Yes I think it is better to just spell it out in the code. Defining
> shifts and masks is good but creating macros with them can make the
> code more confusing I think and it is less common to do that in
> U-Boot.

ok. 

> BTW the mask is normally a shifted mask:
> 
>  +#define CE_CTRL_CLOCK_FREQ_SHIFT       8
>  +#define CE_CTRL_CLOCK_FREQ_MASK                (0xf <<
> CE_CTRL_CLOCK_FREQ_SHIFT)
> 
> and you can put it in an anonymous enum if you prefer.

Let's cleanup the defines then.

[...]

>>>> +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 ?
> 
> I think the init option is OK, basd on what you said. But please add a
> comment to the var explaining why it is needed.

ok. will do.

> How come the flash is not available when the driver is probed? 

The flash device OF nodes are available, this is how we get the count 
of CS, but when controller is probed, the flash device properties are 
unknown which makes sense as we need a working SPI controller to probe 
the SPI slave flash devices. 

> Could you probe whatever else is needed first, so that this IS available?

I would love to but I don't have much control on device_probe() and 
spi_get_bus_and_cs() ... :/

 
> [..]
>>>
>>> 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 ?
>>
> 
> As above, let's check with Jagan on SPI nor.
> 
> With the x86 driver (ich.c) we got around it by trying to make the SPI
> controller look like a normal SPI controller.

Ah. I will take a look.

Thanks,

C.

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Jagan Teki]

On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> 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 ?

Seems with new spi-mem in Linux flash memory driver rely on spi-mem
instead of mtd/spi-nor. So I think you can handle this via new
spi-mem. have you send any patches to Linux?

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Cédric Le Goater]

On 10/4/18 5:57 PM, Jagan Teki wrote:
> On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater <clg@kaod.org> wrote:
>>
>> 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 ?
> 
> Seems with new spi-mem in Linux flash memory driver rely on spi-mem
> instead of mtd/spi-nor. So I think you can handle this via new
> spi-mem. have you send any patches to Linux?

No, not yet. The patchset is sent  : 

	https://patchwork.ozlabs.org/cover/933293/

is not using spimem. I was not aware of that change in the spi-nor layer 
at the time. I will take a look.

Is there a similar framework in u-boot ?

Thanks,

C. 

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Jagan Teki]

On Mon, Oct 8, 2018 at 11:32 AM Cédric Le Goater <clg@kaod.org> wrote:
>
> On 10/4/18 5:57 PM, Jagan Teki wrote:
> > On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> 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 ?
> >
> > Seems with new spi-mem in Linux flash memory driver rely on spi-mem
> > instead of mtd/spi-nor. So I think you can handle this via new
> > spi-mem. have you send any patches to Linux?
>
> No, not yet. The patchset is sent  :
>
>         https://patchwork.ozlabs.org/cover/933293/
>
> is not using spimem. I was not aware of that change in the spi-nor layer
> at the time. I will take a look.

Yes, but for newly added drivers. added spi-mem guys, may be they can comment.

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Boris Brezillon]

Hi Cédric,

On Wed, 10 Oct 2018 11:46:56 +0530
Jagan Teki <jagan@amarulasolutions.com> wrote:

> On Mon, Oct 8, 2018 at 11:32 AM Cédric Le Goater <clg@kaod.org> wrote:
> >
> > On 10/4/18 5:57 PM, Jagan Teki wrote:  
> > > On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater <clg@kaod.org> wrote:  
> > >>
> > >> 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 ?  
> > >
> > > Seems with new spi-mem in Linux flash memory driver rely on spi-mem
> > > instead of mtd/spi-nor. So I think you can handle this via new
> > > spi-mem. have you send any patches to Linux?  
> >
> > No, not yet. The patchset is sent  :
> >
> >         https://patchwork.ozlabs.org/cover/933293/
> >
> > is not using spimem. I was not aware of that change in the spi-nor layer
> > at the time. I will take a look.  

Indeed, if you have some time to convert the Linux aspeed driver to
the spi-mem interface that would be appreciated.

> 
> Yes, but for newly added drivers. added spi-mem guys, may be they can comment.

Jagan, what's the plan for the spi-nor layer in u-boot? I mean, spi-mem
is just the controller side of things, but it requires spi-mem drivers
to support specific SPI memories. We added the spi-nand driver, but
AFAICT, the spi-nor driver does not exist yet. There's the spi-flash
layer already, but IIUC you were trying to replace it by a spi-nor
framework.

I see 2 options here:

1/ copy the spi-nor framework from linux and adjust it to make it work
   in uboot
2/ create a spi-nor driver which interfaces directly with the spi-mem
   layer

I know I usually recommend going for #1, but it might be a bit
different this time around since I'm trying to get rid of the
spi_nor interface in Linux (the one that allows people to implement
spi-nor controller drivers) in favor of a native spi-mem driver. So
I think it's worth considering option #2.

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Cédric Le Goater]

Hello Boris,

On 10/10/18 9:32 AM, Boris Brezillon wrote:
> Hi Cédric,
> 
> On Wed, 10 Oct 2018 11:46:56 +0530
> Jagan Teki <jagan@amarulasolutions.com> wrote:
> 
>> On Mon, Oct 8, 2018 at 11:32 AM Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>> On 10/4/18 5:57 PM, Jagan Teki wrote:  
>>>> On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater <clg@kaod.org> wrote:  
>>>>>
>>>>> 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 ?  
>>>>
>>>> Seems with new spi-mem in Linux flash memory driver rely on spi-mem
>>>> instead of mtd/spi-nor. So I think you can handle this via new
>>>> spi-mem. have you send any patches to Linux?  
>>>
>>> No, not yet. The patchset is sent  :
>>>
>>>         https://patchwork.ozlabs.org/cover/933293/
>>>
>>> is not using spimem. I was not aware of that change in the spi-nor layer
>>> at the time. I will take a look.  
> 
> Indeed, if you have some time to convert the Linux aspeed driver to
> the spi-mem interface that would be appreciated.

Yes. That's the plan. I have a series on the way but I will see if I can
rework a v2 to use spi-mem. 

Same for the u-boot aspeed spi driver which needs a spi-mem refresh if 
I understand correctly. 

Thanks,

C.

 
>>
>> Yes, but for newly added drivers. added spi-mem guys, may be they can comment.
> 
> Jagan, what's the plan for the spi-nor layer in u-boot? I mean, spi-mem
> is just the controller side of things, but it requires spi-mem drivers
> to support specific SPI memories. We added the spi-nand driver, but
> AFAICT, the spi-nor driver does not exist yet. There's the spi-flash
> layer already, but IIUC you were trying to replace it by a spi-nor
> framework.
> 
> I see 2 options here:
> 
> 1/ copy the spi-nor framework from linux and adjust it to make it work
>    in uboot
> 2/ create a spi-nor driver which interfaces directly with the spi-mem
>    layer
> 
> I know I usually recommend going for #1, but it might be a bit
> different this time around since I'm trying to get rid of the
> spi_nor interface in Linux (the one that allows people to implement
> spi-nor controller drivers) in favor of a native spi-mem driver. So
> I think it's worth considering option #2.
> 

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Cédric Le Goater]

Hello

On 10/10/18 2:02 PM, Cédric Le Goater wrote:
> Hello Boris,
> 
> On 10/10/18 9:32 AM, Boris Brezillon wrote:
>> Hi Cédric,
>>
>> On Wed, 10 Oct 2018 11:46:56 +0530
>> Jagan Teki <jagan@amarulasolutions.com> wrote:
>>
>>> On Mon, Oct 8, 2018 at 11:32 AM Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>> On 10/4/18 5:57 PM, Jagan Teki wrote:  
>>>>> On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater <clg@kaod.org> wrote:  
>>>>>>
>>>>>> 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 ?  
>>>>>
>>>>> Seems with new spi-mem in Linux flash memory driver rely on spi-mem
>>>>> instead of mtd/spi-nor. So I think you can handle this via new
>>>>> spi-mem. have you send any patches to Linux?  
>>>>
>>>> No, not yet. The patchset is sent  :
>>>>
>>>>         https://patchwork.ozlabs.org/cover/933293/
>>>>
>>>> is not using spimem. I was not aware of that change in the spi-nor layer
>>>> at the time. I will take a look.  
>>
>> Indeed, if you have some time to convert the Linux aspeed driver to
>> the spi-mem interface that would be appreciated.
> 
> Yes. That's the plan. I have a series on the way but I will see if I can
> rework a v2 to use spi-mem. 

I took a look at spi-mem and worked on an initial spi-aspeed-smc driver
using the new framework in Linux 5.0.x. It looks good but I have a couple
of questions.  


The first is about performing direct accesses on the AHB window on which 
the flash contents is mapped. 

How do you distinguish a flash read (fast, dual, etc) from a RDSFPD command 
for instance ? Are the drivers expected to check the SPI OP command and 
depending on the target/command redirect to the appropriate address space ?   

Also, Aspeed SPI controllers have a Read Timing Compensation Register which
defines different data input delay cycles depending on SPI clock rates. This 
register is supposed to be tuned when the flash chip characteristics are 
known, after the first bus scan. Is there a way to know that our SPI slave 
is alive and well detected before starting hammering successive reads on it 
to see how it behaves.


I think the U-Boot and Linux driver will be very similar wrt the issues 
above ? 

> Same for the u-boot aspeed spi driver which needs a spi-mem refresh if 
> I understand correctly. 

U-Boot, I haven't looked at yet but I expect the driver to be very similar.

Thanks,

C.

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Boris Brezillon]

+Vignesh

Hi Cédric,

On Fri, 25 Jan 2019 18:28:02 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> Hello
> 
> On 10/10/18 2:02 PM, Cédric Le Goater wrote:
> > Hello Boris,
> > 
> > On 10/10/18 9:32 AM, Boris Brezillon wrote:  
> >> Hi Cédric,
> >>
> >> On Wed, 10 Oct 2018 11:46:56 +0530
> >> Jagan Teki <jagan@amarulasolutions.com> wrote:
> >>  
> >>> On Mon, Oct 8, 2018 at 11:32 AM Cédric Le Goater <clg@kaod.org> wrote:  
> >>>>
> >>>> On 10/4/18 5:57 PM, Jagan Teki wrote:    
> >>>>> On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater <clg@kaod.org> wrote:    
> >>>>>>
> >>>>>> 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 ?    
> >>>>>
> >>>>> Seems with new spi-mem in Linux flash memory driver rely on spi-mem
> >>>>> instead of mtd/spi-nor. So I think you can handle this via new
> >>>>> spi-mem. have you send any patches to Linux?    
> >>>>
> >>>> No, not yet. The patchset is sent  :
> >>>>
> >>>>         https://patchwork.ozlabs.org/cover/933293/
> >>>>
> >>>> is not using spimem. I was not aware of that change in the spi-nor layer
> >>>> at the time. I will take a look.    
> >>
> >> Indeed, if you have some time to convert the Linux aspeed driver to
> >> the spi-mem interface that would be appreciated.  
> > 
> > Yes. That's the plan. I have a series on the way but I will see if I can
> > rework a v2 to use spi-mem.   
> 
> I took a look at spi-mem and worked on an initial spi-aspeed-smc driver
> using the new framework in Linux 5.0.x. It looks good but I have a couple
> of questions.

Great!

> 
> 
> The first is about performing direct accesses on the AHB window on which 
> the flash contents is mapped.

We have introduced the dirmap API/interface exactly for this purpose,
and the SPI NOR layer will use it in 5.1 (see [1]).
 
> 
> How do you distinguish a flash read (fast, dual, etc) from a RDSFPD command 
> for instance ?

Why do you need to know the access type?

> Are the drivers expected to check the SPI OP command and 
> depending on the target/command redirect to the appropriate address space ?   

Definitely not, the SPI MEM layer is supposed to be memory-type
agnostic, so you should not guess the operation type based on the
opcode. For direct mapping accesses, just implement the ->dirmap_xxx
hooks at the controller level and you'll be able to use the feature.

> 
> Also, Aspeed SPI controllers have a Read Timing Compensation Register which
> defines different data input delay cycles depending on SPI clock rates. This 
> register is supposed to be tuned when the flash chip characteristics are 
> known, after the first bus scan. Is there a way to know that our SPI slave 
> is alive and well detected before starting hammering successive reads on it 
> to see how it behaves.

Vignesh mentioned that a while back (couldn't find the thread where
this discussion happened) and I suggested adding a new hook to do this
"link training" process where you'd pass a spi_mem_op template + the
expected result so that the controller can test different setups until
it finds a working one.

> 
> 
> I think the U-Boot and Linux driver will be very similar wrt the issues 
> above ? 

I hope so.

Thanks for working on this conversion.

Boris

[1]https://patchwork.kernel.org/patch/10670755/

[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Vignesh R]

Hi Cédric,

On 25/01/19 11:30 PM, Boris Brezillon wrote:
> +Vignesh
> 
[...]
>> The first is about performing direct accesses on the AHB window on which 
>> the flash contents is mapped.
> 
> We have introduced the dirmap API/interface exactly for this purpose,
> and the SPI NOR layer will use it in 5.1 (see [1]).
>  
>>
>> How do you distinguish a flash read (fast, dual, etc) from a RDSFPD command 
>> for instance ?
> 
> Why do you need to know the access type?
> 
>> Are the drivers expected to check the SPI OP command and 
>> depending on the target/command redirect to the appropriate address space ?   
> 
> Definitely not, the SPI MEM layer is supposed to be memory-type
> agnostic, so you should not guess the operation type based on the
> opcode. For direct mapping accesses, just implement the ->dirmap_xxx
> hooks at the controller level and you'll be able to use the feature.>>
>> Also, Aspeed SPI controllers have a Read Timing Compensation Register which
>> defines different data input delay cycles depending on SPI clock rates. This 
>> register is supposed to be tuned when the flash chip characteristics are 
>> known, after the first bus scan. Is there a way to know that our SPI slave 
>> is alive and well detected before starting hammering successive reads on it 
>> to see how it behaves.
> 
> Vignesh mentioned that a while back (couldn't find the thread where
> this discussion happened) and I suggested adding a new hook to do this
> "link training" process where you'd pass a spi_mem_op template + the
> expected result so that the controller can test different setups until
> it finds a working one.
> 

Right, Cadence QSPI/OSPI needs PHY DLL values to be tuned for operating
at higher frequencies. So idea is to use READID to read flash ID at
lowest speed and use that as golden reference for tuning/link training
at higher frequencies. I am guessing Aspeed SPI controller has similar need.
Here is the discussion that Boris was talking about:
https://lkml.org/lkml/2018/10/4/468

I haven't been able to get to implement his suggestions yet, but I think
idea is generic enough.

>>
>>
>> I think the U-Boot and Linux driver will be very similar wrt the issues 
>> above ? 
> 


I have submitted patches[1] to sync SPI NOR framework from kernel to
U-Boot. Once that's merged then Aspeed SPI can make use of spi-mem
interface in U-Boot as well.

[1] https://patchwork.ozlabs.org/cover/1017335/

-- 
Regards
Vignesh

[U-Boot] [PATCH 3/3] aspeed: Add SPI support to the ast2500 Eval Board

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/arm/dts/ast2500-evb.dts     | 17 ++++++++
 arch/arm/dts/ast2500-u-boot.dtsi | 12 ++++++
 arch/arm/dts/ast2500.dtsi        | 71 ++++++++++++++++++++++++++++++++
 configs/evb-ast2500_defconfig    | 10 +++++
 4 files changed, 110 insertions(+)

diff --git a/arch/arm/dts/ast2500-evb.dts b/arch/arm/dts/ast2500-evb.dts
index 723941ac0bee..609678ff7989 100644
--- a/arch/arm/dts/ast2500-evb.dts
+++ b/arch/arm/dts/ast2500-evb.dts
@@ -11,6 +11,10 @@
 	chosen {
 		stdout-path = &uart5;
 	};
+
+	aliases {
+		spi0 = &fmc;
+	};
 };
 
 &uart5 {
@@ -36,3 +40,16 @@
 	u-boot,dm-pre-reloc;
 	status = "okay";
 };
+
+&fmc {
+	status = "okay";
+	flash at 0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "spi-flash", "sst,w25q256";
+		status = "okay";
+		spi-max-frequency = <50000000>;
+		spi-tx-bus-width = <2>;
+		spi-rx-bus-width = <2>;
+	};
+};
diff --git a/arch/arm/dts/ast2500-u-boot.dtsi b/arch/arm/dts/ast2500-u-boot.dtsi
index 7f80bad7d050..6cfdf3ba5413 100644
--- a/arch/arm/dts/ast2500-u-boot.dtsi
+++ b/arch/arm/dts/ast2500-u-boot.dtsi
@@ -70,3 +70,15 @@
 &mac1 {
 	clocks = <&scu PCLK_MAC2>, <&scu PLL_D2PLL>;
 };
+
+&fmc {
+	clocks = <&scu BCLK_HCLK>;
+};
+
+&spi1 {
+	clocks = <&scu BCLK_HCLK>;
+};
+
+&spi2 {
+	clocks = <&scu BCLK_HCLK>;
+};
diff --git a/arch/arm/dts/ast2500.dtsi b/arch/arm/dts/ast2500.dtsi
index 7e0ad3a41ac5..de7607aaafff 100644
--- a/arch/arm/dts/ast2500.dtsi
+++ b/arch/arm/dts/ast2500.dtsi
@@ -28,6 +28,77 @@
 		#size-cells = <1>;
 		ranges;
 
+		fmc: flash-controller at 1e620000 {
+			reg = < 0x1e620000 0xc4
+				0x20000000 0x10000000 >;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "aspeed,ast2500-fmc";
+			status = "disabled";
+			interrupts = <19>;
+			spi-max-frequency = <100000000>;
+			flash at 0 {
+				reg = < 0 >;
+				compatible = "jedec,spi-nor";
+				spi-max-frequency = <50000000>;
+				status = "disabled";
+			};
+			flash at 1 {
+				reg = < 1 >;
+				compatible = "jedec,spi-nor";
+				spi-max-frequency = <50000000>;
+				status = "disabled";
+			};
+			flash at 2 {
+				reg = < 2 >;
+				compatible = "jedec,spi-nor";
+				spi-max-frequency = <50000000>;
+				status = "disabled";
+			};
+		};
+
+		spi1: flash-controller at 1e630000 {
+			reg = < 0x1e630000 0xc4
+				0x30000000 0x08000000 >;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "aspeed,ast2500-spi";
+			status = "disabled";
+			flash at 0 {
+				reg = < 0 >;
+				compatible = "jedec,spi-nor";
+				spi-max-frequency = <50000000>;
+				status = "disabled";
+			};
+			flash at 1 {
+				reg = < 1 >;
+				compatible = "jedec,spi-nor";
+				spi-max-frequency = <50000000>;
+				status = "disabled";
+			};
+		};
+
+		spi2: flash-controller at 1e631000 {
+			reg = < 0x1e631000 0xc4
+				0x38000000 0x08000000 >;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "aspeed,ast2500-spi";
+			status = "disabled";
+			flash at 0 {
+				reg = < 0 >;
+				compatible = "jedec,spi-nor";
+				spi-max-frequency = <50000000>;
+				status = "disabled";
+			};
+			flash at 1 {
+				reg = < 1 >;
+				compatible = "jedec,spi-nor";
+				spi-max-frequency = <50000000>;
+				status = "disabled";
+			};
+		};
+
 		vic: interrupt-controller at 1e6c0080 {
 			compatible = "aspeed,ast2400-vic";
 			interrupt-controller;
diff --git a/configs/evb-ast2500_defconfig b/configs/evb-ast2500_defconfig
index 88230f4a12db..a418ff0e9e01 100644
--- a/configs/evb-ast2500_defconfig
+++ b/configs/evb-ast2500_defconfig
@@ -25,3 +25,13 @@ CONFIG_SYS_NS16550=y
 CONFIG_SYSRESET=y
 CONFIG_TIMER=y
 CONFIG_WDT=y
+CONFIG_SPI=y
+CONFIG_ASPEED_SPI=y
+CONFIG_DM_SPI=y
+CONFIG_DM_SPI_FLASH=y
+CONFIG_SPI_FLASH=y
+CONFIG_SPI_FLASH_MACRONIX=y
+CONFIG_SPI_FLASH_SPANSION=y
+CONFIG_SPI_FLASH_WINBOND=y
+CONFIG_SPI_FLASH_STMICRO=y
+CONFIG_CMD_SF=y
-- 
2.17.1

[U-Boot] [PATCH 3/3] aspeed: Add SPI support to the ast2500 Eval Board

[Joel Stanley]

On Mon, 10 Sep 2018 at 23:49, Cédric Le Goater <clg@kaod.org> wrote:
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

[U-Boot] [PATCH 3/3] aspeed: Add SPI support to the ast2500 Eval Board

[Jagan Teki]

On Mon, Sep 10, 2018 at 7:46 PM, Cédric Le Goater <clg@kaod.org> wrote:
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  arch/arm/dts/ast2500-evb.dts     | 17 ++++++++
>  arch/arm/dts/ast2500-u-boot.dtsi | 12 ++++++
>  arch/arm/dts/ast2500.dtsi        | 71 ++++++++++++++++++++++++++++++++
>  configs/evb-ast2500_defconfig    | 10 +++++

Except -u-boot.dtsi is rest of DTS files from Linux? If yes then
attach the sync commit details.

on that note.

Reviewed-by: Jagan Teki <jagan@openedev.com>

[U-Boot] [PATCH 3/3] aspeed: Add SPI support to the ast2500 Eval Board

[Cédric Le Goater]

On 09/20/2018 04:53 PM, Jagan Teki wrote:
> On Mon, Sep 10, 2018 at 7:46 PM, Cédric Le Goater <clg@kaod.org> wrote:
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  arch/arm/dts/ast2500-evb.dts     | 17 ++++++++
>>  arch/arm/dts/ast2500-u-boot.dtsi | 12 ++++++
>>  arch/arm/dts/ast2500.dtsi        | 71 ++++++++++++++++++++++++++++++++
>>  configs/evb-ast2500_defconfig    | 10 +++++
> 
> Except -u-boot.dtsi is rest of DTS files from Linux? If yes then
> attach the sync commit details.

Yes they are from Linux but I would not say the DTS files are in sync. 
They don't even have the same names, the layout is different. 

I will send v2 with a large resync. This is required anyhow for the 
Aspeed ftgmac100 ethernet driver I sent in another series.

> on that note.
> 
> Reviewed-by: Jagan Teki <jagan@openedev.com>
> 

Thanks,

C.