[U-Boot,07/10] spi: sun4: Add A31 spi controller support

Message ID 20190209131500.29954-8-jagan@amarulasolutions.com
State Changes Requested
Delegated to: Jagannadha Sutradharudu Teki
Headers show
Series
  • spi: Add Allwinner A31 SPI driver
Related show

Commit Message

Jagan Teki Feb. 9, 2019, 1:14 p.m.
Add A31 spi controller support for existing sun4i_spi driver via driver
data, this would simply add A31 register along with proper register bits
via enum sets.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/spi/Kconfig     |  4 +-
 drivers/spi/sun4i_spi.c | 84 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 85 insertions(+), 3 deletions(-)

Comments

André Przywara Feb. 13, 2019, 1:19 a.m. | #1
On 09/02/2019 13:14, Jagan Teki wrote:
> Add A31 spi controller support for existing sun4i_spi driver via driver
> data, this would simply add A31 register along with proper register bits
> via enum sets.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/spi/Kconfig     |  4 +-
>  drivers/spi/sun4i_spi.c | 84 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 85 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index ac7fbab841..15207d23c1 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -223,9 +223,9 @@ config STM32_QSPI
>  	  this ST IP core.
>  
>  config SUN4I_SPI
> -	bool "Allwinner A10 SoCs SPI controller"
> +	bool "Allwinner A10/A31 SoCs SPI controller"
>  	help
> -	  SPI driver for Allwinner sun4i, sun5i and sun7i SoCs
> +	  This enables using the SPI controller on the Allwinner A10/A31 SoCs.
>  
>  config TEGRA114_SPI
>  	bool "nVidia Tegra114 SPI driver"
> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
> index aeed68764c..36f2215f7d 100644
> --- a/drivers/spi/sun4i_spi.c
> +++ b/drivers/spi/sun4i_spi.c
> @@ -24,6 +24,7 @@
>  #include <spi.h>
>  #include <errno.h>
>  #include <fdt_support.h>
> +#include <reset.h>
>  #include <wait_bit.h>
>  
>  #include <asm/bitops.h>
> @@ -84,6 +85,18 @@
>  #define SUN4I_FIFO_STA_TF_CNT_MASK	0x7f
>  #define SUN4I_FIFO_STA_TF_CNT_BITS	16
>  
> +/* sun6i spi registers */
> +#define SUN6I_GBL_CTL_REG		0x04
> +#define SUN6I_TFR_CTL_REG		0x08
> +#define SUN6I_FIFO_CTL_REG		0x18
> +#define SUN6I_FIFO_STA_REG		0x1c
> +#define SUN6I_CLK_CTL_REG		0x24
> +#define SUN6I_BURST_CNT_REG		0x30
> +#define SUN6I_XMIT_CNT_REG		0x34
> +#define SUN6I_BURST_CTL_REG		0x38
> +#define SUN6I_TXDATA_REG		0x200
> +#define SUN6I_RXDATA_REG		0x300
> +
>  #define SUN4I_SPI_MAX_RATE	24000000
>  #define SUN4I_SPI_MIN_RATE	3000
>  #define SUN4I_SPI_DEFAULT_RATE	1000000
> @@ -106,6 +119,7 @@ enum sun4i_spi_regs {
>  /* sun spi register bits */
>  enum sun4i_spi_bits {
>  	SPI_GCR_TP,
> +	SPI_GCR_SRST,
>  	SPI_TCR_CPHA,
>  	SPI_TCR_CPOL,
>  	SPI_TCR_CS_ACTIVE_LOW,
> @@ -133,6 +147,7 @@ struct sun4i_spi_platdata {
>  struct sun4i_spi_priv {
>  	struct sun4i_spi_variant *variant;
>  	struct clk clk_ahb, clk_mod;
> +	struct reset_ctl reset;
>  	u32 base_addr;
>  	u32 freq;
>  	u32 mode;
> @@ -255,7 +270,10 @@ static int sun4i_spi_parse_pins(struct udevice *dev)
>  			if (pin < 0)
>  				break;
>  
> -			sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_SPI0);
> +			if (IS_ENABLED(CONFIG_MACH_SUN50I))
> +				sunxi_gpio_set_cfgpin(pin, SUN50I_GPC_SPI0);
> +			else
> +				sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_SPI0);
>  			sunxi_gpio_set_drv(pin, drive);
>  			sunxi_gpio_set_pull(pin, pull);
>  		}
> @@ -271,6 +289,8 @@ static inline int sun4i_spi_set_clock(struct udevice *dev, bool enable)
>  	if (!enable) {
>  		clk_disable(&priv->clk_ahb);
>  		clk_disable(&priv->clk_mod);
> +		if (reset_valid(&priv->reset))
> +			reset_assert(&priv->reset);
>  		return 0;
>  	}
>  
> @@ -286,8 +306,18 @@ static inline int sun4i_spi_set_clock(struct udevice *dev, bool enable)
>  		goto err_ahb;
>  	}
>  
> +	if (reset_valid(&priv->reset)) {
> +		ret = reset_deassert(&priv->reset);
> +		if (ret) {
> +			dev_err(dev, "failed to deassert reset\n");
> +			goto err_mod;
> +		}
> +	}
> +
>  	return 0;
>  
> +err_mod:
> +	clk_disable(&priv->clk_mod);
>  err_ahb:
>  	clk_disable(&priv->clk_ahb);
>  	return ret;
> @@ -328,6 +358,12 @@ static int sun4i_spi_probe(struct udevice *bus)
>  		return ret;
>  	}
>  
> +	ret = reset_get_by_index(bus, 0, &priv->reset);
> +	if (ret && ret != -ENOENT) {
> +		dev_err(dev, "failed to get reset\n");
> +		return ret;
> +	}
> +
>  	sun4i_spi_parse_pins(bus);
>  
>  	priv->variant = plat->variant;
> @@ -351,6 +387,10 @@ static int sun4i_spi_claim_bus(struct udevice *dev)
>  		     SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER |
>  		     variant->bits[SPI_GCR_TP]);
>  
> +	if (IS_ENABLED(CONFIG_SUNXI_GEN_SUN6I))
> +		setbits_le32(priv->base_addr + variant->regs[SPI_GCR],
> +			     variant->bits[SPI_GCR_SRST]);
> +

Just a note: This makes the driver only support one class of device, set
at compile time. Which means we could have used an #ifdef approach from
the beginning ....
Can you make this dependent on some bits in variant? That should solve it.

>  	setbits_le32(priv->base_addr + variant->regs[SPI_TCR],
>  		     variant->bits[SPI_TCR_CS_MANUAL] |
>  		     variant->bits[SPI_TCR_CS_ACTIVE_LOW]);
> @@ -409,6 +449,9 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  		       priv->base_addr + variant->regs[SPI_BC]);
>  		writel(SUN4I_XMIT_CNT(nbytes),
>  		       priv->base_addr + variant->regs[SPI_TC]);
> +		if (IS_ENABLED(CONFIG_SUNXI_GEN_SUN6I))
> +			writel(SUN4I_BURST_CNT(nbytes),
> +			       priv->base_addr + variant->regs[SPI_BCTL]);
>  
>  		/* Fill the TX FIFO */
>  		sun4i_spi_fill_fifo(priv, nbytes);
> @@ -547,17 +590,56 @@ static const unsigned long sun4i_spi_bits[] = {
>  	[SPI_FSR_RF_CNT_MASK]	= GENMASK(6, 0),
>  };
>  
> +static const unsigned long sun8i_spi_regs[] = {
> +	[SPI_GCR]		= SUN6I_GBL_CTL_REG,
> +	[SPI_TCR]		= SUN6I_TFR_CTL_REG,
> +	[SPI_FCR]		= SUN6I_FIFO_CTL_REG,
> +	[SPI_FSR]		= SUN6I_FIFO_STA_REG,
> +	[SPI_CCR]		= SUN6I_CLK_CTL_REG,
> +	[SPI_BC]		= SUN6I_BURST_CNT_REG,
> +	[SPI_TC]		= SUN6I_XMIT_CNT_REG,
> +	[SPI_BCTL]		= SUN6I_BURST_CTL_REG,
> +	[SPI_TXD]		= SUN6I_TXDATA_REG,
> +	[SPI_RXD]		= SUN6I_RXDATA_REG,
> +};
> +
> +static const unsigned long sun8i_spi_bits[] = {
> +	[SPI_GCR_TP]		= BIT(7),
> +	[SPI_GCR_SRST]		= BIT(31),
> +	[SPI_TCR_CPHA]		= BIT(0),
> +	[SPI_TCR_CPOL]		= BIT(1),
> +	[SPI_TCR_CS_ACTIVE_LOW] = BIT(2),
> +	[SPI_TCR_CS_SEL]	= 4,
> +	[SPI_TCR_CS_MASK]	= 0x30,
> +	[SPI_TCR_CS_MANUAL]	= BIT(6),
> +	[SPI_TCR_CS_LEVEL]	= BIT(7),
> +	[SPI_TCR_XCH]		= BIT(31),
> +	[SPI_FCR_RF_RST]	= BIT(15),
> +	[SPI_FCR_TF_RST]	= BIT(31),
> +	[SPI_FSR_RF_CNT_MASK]	= GENMASK(7, 0),
> +};
> +
>  static const struct sun4i_spi_variant sun4i_a10_spi_variant = {
>  	.regs			= sun4i_spi_regs,
>  	.bits			= sun4i_spi_bits,
>  	.fifo_depth		= 64,
>  };
>  
> +static const struct sun4i_spi_variant sun8i_h3_spi_variant = {
> +	.regs			= sun8i_spi_regs,
> +	.bits			= sun8i_spi_bits,
> +	.fifo_depth		= 64,
> +};
> +
>  static const struct udevice_id sun4i_spi_ids[] = {
>  	{
>  	  .compatible = "allwinner,sun4i-a10-spi",
>  	  .data = (ulong)&sun4i_a10_spi_variant,
>  	},
> +	{
> +	  .compatible = "allwinner,sun8i-h3-spi",
> +	  .data = (ulong)&sun8i_h3_spi_variant,
> +	},

Please add the allwinner,sun6i-a31-spi compatible as well.

Cheers,
Andre

>  	{ }
>  };
>  
>
Chen-Yu Tsai Feb. 13, 2019, 3:20 a.m. | #2
On Wed, Feb 13, 2019 at 9:21 AM André Przywara <andre.przywara@arm.com> wrote:
>
> On 09/02/2019 13:14, Jagan Teki wrote:
> > Add A31 spi controller support for existing sun4i_spi driver via driver
> > data, this would simply add A31 register along with proper register bits
> > via enum sets.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  drivers/spi/Kconfig     |  4 +-
> >  drivers/spi/sun4i_spi.c | 84 ++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 85 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index ac7fbab841..15207d23c1 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -223,9 +223,9 @@ config STM32_QSPI
> >         this ST IP core.
> >
> >  config SUN4I_SPI
> > -     bool "Allwinner A10 SoCs SPI controller"
> > +     bool "Allwinner A10/A31 SoCs SPI controller"
> >       help
> > -       SPI driver for Allwinner sun4i, sun5i and sun7i SoCs
> > +       This enables using the SPI controller on the Allwinner A10/A31 SoCs.
> >
> >  config TEGRA114_SPI
> >       bool "nVidia Tegra114 SPI driver"
> > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
> > index aeed68764c..36f2215f7d 100644
> > --- a/drivers/spi/sun4i_spi.c
> > +++ b/drivers/spi/sun4i_spi.c
> > @@ -24,6 +24,7 @@
> >  #include <spi.h>
> >  #include <errno.h>
> >  #include <fdt_support.h>
> > +#include <reset.h>
> >  #include <wait_bit.h>
> >
> >  #include <asm/bitops.h>
> > @@ -84,6 +85,18 @@
> >  #define SUN4I_FIFO_STA_TF_CNT_MASK   0x7f
> >  #define SUN4I_FIFO_STA_TF_CNT_BITS   16
> >
> > +/* sun6i spi registers */
> > +#define SUN6I_GBL_CTL_REG            0x04
> > +#define SUN6I_TFR_CTL_REG            0x08
> > +#define SUN6I_FIFO_CTL_REG           0x18
> > +#define SUN6I_FIFO_STA_REG           0x1c
> > +#define SUN6I_CLK_CTL_REG            0x24
> > +#define SUN6I_BURST_CNT_REG          0x30
> > +#define SUN6I_XMIT_CNT_REG           0x34
> > +#define SUN6I_BURST_CTL_REG          0x38
> > +#define SUN6I_TXDATA_REG             0x200
> > +#define SUN6I_RXDATA_REG             0x300
> > +
> >  #define SUN4I_SPI_MAX_RATE   24000000
> >  #define SUN4I_SPI_MIN_RATE   3000
> >  #define SUN4I_SPI_DEFAULT_RATE       1000000
> > @@ -106,6 +119,7 @@ enum sun4i_spi_regs {
> >  /* sun spi register bits */
> >  enum sun4i_spi_bits {
> >       SPI_GCR_TP,
> > +     SPI_GCR_SRST,
> >       SPI_TCR_CPHA,
> >       SPI_TCR_CPOL,
> >       SPI_TCR_CS_ACTIVE_LOW,
> > @@ -133,6 +147,7 @@ struct sun4i_spi_platdata {
> >  struct sun4i_spi_priv {
> >       struct sun4i_spi_variant *variant;
> >       struct clk clk_ahb, clk_mod;
> > +     struct reset_ctl reset;
> >       u32 base_addr;
> >       u32 freq;
> >       u32 mode;
> > @@ -255,7 +270,10 @@ static int sun4i_spi_parse_pins(struct udevice *dev)
> >                       if (pin < 0)
> >                               break;
> >
> > -                     sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_SPI0);
> > +                     if (IS_ENABLED(CONFIG_MACH_SUN50I))
> > +                             sunxi_gpio_set_cfgpin(pin, SUN50I_GPC_SPI0);
> > +                     else
> > +                             sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_SPI0);
> >                       sunxi_gpio_set_drv(pin, drive);
> >                       sunxi_gpio_set_pull(pin, pull);
> >               }
> > @@ -271,6 +289,8 @@ static inline int sun4i_spi_set_clock(struct udevice *dev, bool enable)
> >       if (!enable) {
> >               clk_disable(&priv->clk_ahb);
> >               clk_disable(&priv->clk_mod);
> > +             if (reset_valid(&priv->reset))
> > +                     reset_assert(&priv->reset);
> >               return 0;
> >       }
> >
> > @@ -286,8 +306,18 @@ static inline int sun4i_spi_set_clock(struct udevice *dev, bool enable)
> >               goto err_ahb;
> >       }
> >
> > +     if (reset_valid(&priv->reset)) {
> > +             ret = reset_deassert(&priv->reset);
> > +             if (ret) {
> > +                     dev_err(dev, "failed to deassert reset\n");
> > +                     goto err_mod;
> > +             }
> > +     }
> > +
> >       return 0;
> >
> > +err_mod:
> > +     clk_disable(&priv->clk_mod);
> >  err_ahb:
> >       clk_disable(&priv->clk_ahb);
> >       return ret;
> > @@ -328,6 +358,12 @@ static int sun4i_spi_probe(struct udevice *bus)
> >               return ret;
> >       }
> >
> > +     ret = reset_get_by_index(bus, 0, &priv->reset);
> > +     if (ret && ret != -ENOENT) {
> > +             dev_err(dev, "failed to get reset\n");
> > +             return ret;
> > +     }
> > +
> >       sun4i_spi_parse_pins(bus);
> >
> >       priv->variant = plat->variant;
> > @@ -351,6 +387,10 @@ static int sun4i_spi_claim_bus(struct udevice *dev)
> >                    SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER |
> >                    variant->bits[SPI_GCR_TP]);
> >
> > +     if (IS_ENABLED(CONFIG_SUNXI_GEN_SUN6I))
> > +             setbits_le32(priv->base_addr + variant->regs[SPI_GCR],
> > +                          variant->bits[SPI_GCR_SRST]);
> > +
>
> Just a note: This makes the driver only support one class of device, set
> at compile time. Which means we could have used an #ifdef approach from
> the beginning ....
> Can you make this dependent on some bits in variant? That should solve it.

I believe the argument for IS_ENABLED from working in the kernel is that
it still gives you compile-time checks, whereas #ifdef doesn't.

Though I believe your ultimate goal is to have one image for all? In that
case checking against the variants at runtime is better.

ChenYu
André Przywara Feb. 13, 2019, 10:14 a.m. | #3
On Wed, 13 Feb 2019 11:20:11 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

Hi Chen-Yu,

> On Wed, Feb 13, 2019 at 9:21 AM André Przywara <andre.przywara@arm.com> wrote:
> >
> > On 09/02/2019 13:14, Jagan Teki wrote:  
> > > Add A31 spi controller support for existing sun4i_spi driver via driver
> > > data, this would simply add A31 register along with proper register bits
> > > via enum sets.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > >  drivers/spi/Kconfig     |  4 +-
> > >  drivers/spi/sun4i_spi.c | 84 ++++++++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 85 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > > index ac7fbab841..15207d23c1 100644
> > > --- a/drivers/spi/Kconfig
> > > +++ b/drivers/spi/Kconfig
> > > @@ -223,9 +223,9 @@ config STM32_QSPI
> > >         this ST IP core.
> > >
> > >  config SUN4I_SPI
> > > -     bool "Allwinner A10 SoCs SPI controller"
> > > +     bool "Allwinner A10/A31 SoCs SPI controller"
> > >       help
> > > -       SPI driver for Allwinner sun4i, sun5i and sun7i SoCs
> > > +       This enables using the SPI controller on the Allwinner A10/A31 SoCs.
> > >
> > >  config TEGRA114_SPI
> > >       bool "nVidia Tegra114 SPI driver"
> > > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
> > > index aeed68764c..36f2215f7d 100644
> > > --- a/drivers/spi/sun4i_spi.c
> > > +++ b/drivers/spi/sun4i_spi.c
> > > @@ -24,6 +24,7 @@
> > >  #include <spi.h>
> > >  #include <errno.h>
> > >  #include <fdt_support.h>
> > > +#include <reset.h>
> > >  #include <wait_bit.h>
> > >
> > >  #include <asm/bitops.h>
> > > @@ -84,6 +85,18 @@
> > >  #define SUN4I_FIFO_STA_TF_CNT_MASK   0x7f
> > >  #define SUN4I_FIFO_STA_TF_CNT_BITS   16
> > >
> > > +/* sun6i spi registers */
> > > +#define SUN6I_GBL_CTL_REG            0x04
> > > +#define SUN6I_TFR_CTL_REG            0x08
> > > +#define SUN6I_FIFO_CTL_REG           0x18
> > > +#define SUN6I_FIFO_STA_REG           0x1c
> > > +#define SUN6I_CLK_CTL_REG            0x24
> > > +#define SUN6I_BURST_CNT_REG          0x30
> > > +#define SUN6I_XMIT_CNT_REG           0x34
> > > +#define SUN6I_BURST_CTL_REG          0x38
> > > +#define SUN6I_TXDATA_REG             0x200
> > > +#define SUN6I_RXDATA_REG             0x300
> > > +
> > >  #define SUN4I_SPI_MAX_RATE   24000000
> > >  #define SUN4I_SPI_MIN_RATE   3000
> > >  #define SUN4I_SPI_DEFAULT_RATE       1000000
> > > @@ -106,6 +119,7 @@ enum sun4i_spi_regs {
> > >  /* sun spi register bits */
> > >  enum sun4i_spi_bits {
> > >       SPI_GCR_TP,
> > > +     SPI_GCR_SRST,
> > >       SPI_TCR_CPHA,
> > >       SPI_TCR_CPOL,
> > >       SPI_TCR_CS_ACTIVE_LOW,
> > > @@ -133,6 +147,7 @@ struct sun4i_spi_platdata {
> > >  struct sun4i_spi_priv {
> > >       struct sun4i_spi_variant *variant;
> > >       struct clk clk_ahb, clk_mod;
> > > +     struct reset_ctl reset;
> > >       u32 base_addr;
> > >       u32 freq;
> > >       u32 mode;
> > > @@ -255,7 +270,10 @@ static int sun4i_spi_parse_pins(struct udevice *dev)
> > >                       if (pin < 0)
> > >                               break;
> > >
> > > -                     sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_SPI0);
> > > +                     if (IS_ENABLED(CONFIG_MACH_SUN50I))
> > > +                             sunxi_gpio_set_cfgpin(pin, SUN50I_GPC_SPI0);
> > > +                     else
> > > +                             sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_SPI0);
> > >                       sunxi_gpio_set_drv(pin, drive);
> > >                       sunxi_gpio_set_pull(pin, pull);
> > >               }
> > > @@ -271,6 +289,8 @@ static inline int sun4i_spi_set_clock(struct udevice *dev, bool enable)
> > >       if (!enable) {
> > >               clk_disable(&priv->clk_ahb);
> > >               clk_disable(&priv->clk_mod);
> > > +             if (reset_valid(&priv->reset))
> > > +                     reset_assert(&priv->reset);
> > >               return 0;
> > >       }
> > >
> > > @@ -286,8 +306,18 @@ static inline int sun4i_spi_set_clock(struct udevice *dev, bool enable)
> > >               goto err_ahb;
> > >       }
> > >
> > > +     if (reset_valid(&priv->reset)) {
> > > +             ret = reset_deassert(&priv->reset);
> > > +             if (ret) {
> > > +                     dev_err(dev, "failed to deassert reset\n");
> > > +                     goto err_mod;
> > > +             }
> > > +     }
> > > +
> > >       return 0;
> > >
> > > +err_mod:
> > > +     clk_disable(&priv->clk_mod);
> > >  err_ahb:
> > >       clk_disable(&priv->clk_ahb);
> > >       return ret;
> > > @@ -328,6 +358,12 @@ static int sun4i_spi_probe(struct udevice *bus)
> > >               return ret;
> > >       }
> > >
> > > +     ret = reset_get_by_index(bus, 0, &priv->reset);
> > > +     if (ret && ret != -ENOENT) {
> > > +             dev_err(dev, "failed to get reset\n");
> > > +             return ret;
> > > +     }
> > > +
> > >       sun4i_spi_parse_pins(bus);
> > >
> > >       priv->variant = plat->variant;
> > > @@ -351,6 +387,10 @@ static int sun4i_spi_claim_bus(struct udevice *dev)
> > >                    SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER |
> > >                    variant->bits[SPI_GCR_TP]);
> > >
> > > +     if (IS_ENABLED(CONFIG_SUNXI_GEN_SUN6I))
> > > +             setbits_le32(priv->base_addr + variant->regs[SPI_GCR],
> > > +                          variant->bits[SPI_GCR_SRST]);
> > > +  
> >
> > Just a note: This makes the driver only support one class of device, set
> > at compile time. Which means we could have used an #ifdef approach from
> > the beginning ....
> > Can you make this dependent on some bits in variant? That should solve it.  
> 
> I believe the argument for IS_ENABLED from working in the kernel is that
> it still gives you compile-time checks, whereas #ifdef doesn't.

I see that, but my point was that this series is more complicated than the simple one I sent yesterday, using compile time decisions[1].
And the #ifdefs there were not guarding any *code* as well, only defines.
So I am not against this approach here, I just wanted to point out that despite the extra effort of doing runtime choices we will actually fail in doing so anyway.
But if I am not mistaken, this is the only place where this is critical, and it can be fixed.

> Though I believe your ultimate goal is to have one image for all? In that
> case checking against the variants at runtime is better.

I am not sure we really want to cover *every* Allwinner board with a single U-Boot image. For my part I am fine if we can cover all 64-bit boards, maybe an extra build for newer (>= SUN6I) 32-bit boards. But to be honest those drivers are the least of our problems then ;-)

Cheers,
Andre.

[1] https://lists.denx.de/pipermail/u-boot/2019-February/358502.html
Jagan Teki Feb. 13, 2019, 12:17 p.m. | #4
On Wed, Feb 13, 2019 at 6:51 AM André Przywara <andre.przywara@arm.com> wrote:
>
> On 09/02/2019 13:14, Jagan Teki wrote:
> > Add A31 spi controller support for existing sun4i_spi driver via driver
> > data, this would simply add A31 register along with proper register bits
> > via enum sets.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  drivers/spi/Kconfig     |  4 +-
> >  drivers/spi/sun4i_spi.c | 84 ++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 85 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index ac7fbab841..15207d23c1 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -223,9 +223,9 @@ config STM32_QSPI
> >         this ST IP core.
> >
> >  config SUN4I_SPI
> > -     bool "Allwinner A10 SoCs SPI controller"
> > +     bool "Allwinner A10/A31 SoCs SPI controller"
> >       help
> > -       SPI driver for Allwinner sun4i, sun5i and sun7i SoCs
> > +       This enables using the SPI controller on the Allwinner A10/A31 SoCs.
> >
> >  config TEGRA114_SPI
> >       bool "nVidia Tegra114 SPI driver"
> > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
> > index aeed68764c..36f2215f7d 100644
> > --- a/drivers/spi/sun4i_spi.c
> > +++ b/drivers/spi/sun4i_spi.c
> > @@ -24,6 +24,7 @@
> >  #include <spi.h>
> >  #include <errno.h>
> >  #include <fdt_support.h>
> > +#include <reset.h>
> >  #include <wait_bit.h>
> >
> >  #include <asm/bitops.h>
> > @@ -84,6 +85,18 @@
> >  #define SUN4I_FIFO_STA_TF_CNT_MASK   0x7f
> >  #define SUN4I_FIFO_STA_TF_CNT_BITS   16
> >
> > +/* sun6i spi registers */
> > +#define SUN6I_GBL_CTL_REG            0x04
> > +#define SUN6I_TFR_CTL_REG            0x08
> > +#define SUN6I_FIFO_CTL_REG           0x18
> > +#define SUN6I_FIFO_STA_REG           0x1c
> > +#define SUN6I_CLK_CTL_REG            0x24
> > +#define SUN6I_BURST_CNT_REG          0x30
> > +#define SUN6I_XMIT_CNT_REG           0x34
> > +#define SUN6I_BURST_CTL_REG          0x38
> > +#define SUN6I_TXDATA_REG             0x200
> > +#define SUN6I_RXDATA_REG             0x300
> > +
> >  #define SUN4I_SPI_MAX_RATE   24000000
> >  #define SUN4I_SPI_MIN_RATE   3000
> >  #define SUN4I_SPI_DEFAULT_RATE       1000000
> > @@ -106,6 +119,7 @@ enum sun4i_spi_regs {
> >  /* sun spi register bits */
> >  enum sun4i_spi_bits {
> >       SPI_GCR_TP,
> > +     SPI_GCR_SRST,
> >       SPI_TCR_CPHA,
> >       SPI_TCR_CPOL,
> >       SPI_TCR_CS_ACTIVE_LOW,
> > @@ -133,6 +147,7 @@ struct sun4i_spi_platdata {
> >  struct sun4i_spi_priv {
> >       struct sun4i_spi_variant *variant;
> >       struct clk clk_ahb, clk_mod;
> > +     struct reset_ctl reset;
> >       u32 base_addr;
> >       u32 freq;
> >       u32 mode;
> > @@ -255,7 +270,10 @@ static int sun4i_spi_parse_pins(struct udevice *dev)
> >                       if (pin < 0)
> >                               break;
> >
> > -                     sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_SPI0);
> > +                     if (IS_ENABLED(CONFIG_MACH_SUN50I))
> > +                             sunxi_gpio_set_cfgpin(pin, SUN50I_GPC_SPI0);
> > +                     else
> > +                             sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_SPI0);
> >                       sunxi_gpio_set_drv(pin, drive);
> >                       sunxi_gpio_set_pull(pin, pull);
> >               }
> > @@ -271,6 +289,8 @@ static inline int sun4i_spi_set_clock(struct udevice *dev, bool enable)
> >       if (!enable) {
> >               clk_disable(&priv->clk_ahb);
> >               clk_disable(&priv->clk_mod);
> > +             if (reset_valid(&priv->reset))
> > +                     reset_assert(&priv->reset);
> >               return 0;
> >       }
> >
> > @@ -286,8 +306,18 @@ static inline int sun4i_spi_set_clock(struct udevice *dev, bool enable)
> >               goto err_ahb;
> >       }
> >
> > +     if (reset_valid(&priv->reset)) {
> > +             ret = reset_deassert(&priv->reset);
> > +             if (ret) {
> > +                     dev_err(dev, "failed to deassert reset\n");
> > +                     goto err_mod;
> > +             }
> > +     }
> > +
> >       return 0;
> >
> > +err_mod:
> > +     clk_disable(&priv->clk_mod);
> >  err_ahb:
> >       clk_disable(&priv->clk_ahb);
> >       return ret;
> > @@ -328,6 +358,12 @@ static int sun4i_spi_probe(struct udevice *bus)
> >               return ret;
> >       }
> >
> > +     ret = reset_get_by_index(bus, 0, &priv->reset);
> > +     if (ret && ret != -ENOENT) {
> > +             dev_err(dev, "failed to get reset\n");
> > +             return ret;
> > +     }
> > +
> >       sun4i_spi_parse_pins(bus);
> >
> >       priv->variant = plat->variant;
> > @@ -351,6 +387,10 @@ static int sun4i_spi_claim_bus(struct udevice *dev)
> >                    SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER |
> >                    variant->bits[SPI_GCR_TP]);
> >
> > +     if (IS_ENABLED(CONFIG_SUNXI_GEN_SUN6I))
> > +             setbits_le32(priv->base_addr + variant->regs[SPI_GCR],
> > +                          variant->bits[SPI_GCR_SRST]);
> > +
>
> Just a note: This makes the driver only support one class of device, set
> at compile time. Which means we could have used an #ifdef approach from
> the beginning ....
> Can you make this dependent on some bits in variant? That should solve it.

Yes, we can do this meaningfully via driverdata or compatible check.

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ac7fbab841..15207d23c1 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -223,9 +223,9 @@  config STM32_QSPI
 	  this ST IP core.
 
 config SUN4I_SPI
-	bool "Allwinner A10 SoCs SPI controller"
+	bool "Allwinner A10/A31 SoCs SPI controller"
 	help
-	  SPI driver for Allwinner sun4i, sun5i and sun7i SoCs
+	  This enables using the SPI controller on the Allwinner A10/A31 SoCs.
 
 config TEGRA114_SPI
 	bool "nVidia Tegra114 SPI driver"
diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
index aeed68764c..36f2215f7d 100644
--- a/drivers/spi/sun4i_spi.c
+++ b/drivers/spi/sun4i_spi.c
@@ -24,6 +24,7 @@ 
 #include <spi.h>
 #include <errno.h>
 #include <fdt_support.h>
+#include <reset.h>
 #include <wait_bit.h>
 
 #include <asm/bitops.h>
@@ -84,6 +85,18 @@ 
 #define SUN4I_FIFO_STA_TF_CNT_MASK	0x7f
 #define SUN4I_FIFO_STA_TF_CNT_BITS	16
 
+/* sun6i spi registers */
+#define SUN6I_GBL_CTL_REG		0x04
+#define SUN6I_TFR_CTL_REG		0x08
+#define SUN6I_FIFO_CTL_REG		0x18
+#define SUN6I_FIFO_STA_REG		0x1c
+#define SUN6I_CLK_CTL_REG		0x24
+#define SUN6I_BURST_CNT_REG		0x30
+#define SUN6I_XMIT_CNT_REG		0x34
+#define SUN6I_BURST_CTL_REG		0x38
+#define SUN6I_TXDATA_REG		0x200
+#define SUN6I_RXDATA_REG		0x300
+
 #define SUN4I_SPI_MAX_RATE	24000000
 #define SUN4I_SPI_MIN_RATE	3000
 #define SUN4I_SPI_DEFAULT_RATE	1000000
@@ -106,6 +119,7 @@  enum sun4i_spi_regs {
 /* sun spi register bits */
 enum sun4i_spi_bits {
 	SPI_GCR_TP,
+	SPI_GCR_SRST,
 	SPI_TCR_CPHA,
 	SPI_TCR_CPOL,
 	SPI_TCR_CS_ACTIVE_LOW,
@@ -133,6 +147,7 @@  struct sun4i_spi_platdata {
 struct sun4i_spi_priv {
 	struct sun4i_spi_variant *variant;
 	struct clk clk_ahb, clk_mod;
+	struct reset_ctl reset;
 	u32 base_addr;
 	u32 freq;
 	u32 mode;
@@ -255,7 +270,10 @@  static int sun4i_spi_parse_pins(struct udevice *dev)
 			if (pin < 0)
 				break;
 
-			sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_SPI0);
+			if (IS_ENABLED(CONFIG_MACH_SUN50I))
+				sunxi_gpio_set_cfgpin(pin, SUN50I_GPC_SPI0);
+			else
+				sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_SPI0);
 			sunxi_gpio_set_drv(pin, drive);
 			sunxi_gpio_set_pull(pin, pull);
 		}
@@ -271,6 +289,8 @@  static inline int sun4i_spi_set_clock(struct udevice *dev, bool enable)
 	if (!enable) {
 		clk_disable(&priv->clk_ahb);
 		clk_disable(&priv->clk_mod);
+		if (reset_valid(&priv->reset))
+			reset_assert(&priv->reset);
 		return 0;
 	}
 
@@ -286,8 +306,18 @@  static inline int sun4i_spi_set_clock(struct udevice *dev, bool enable)
 		goto err_ahb;
 	}
 
+	if (reset_valid(&priv->reset)) {
+		ret = reset_deassert(&priv->reset);
+		if (ret) {
+			dev_err(dev, "failed to deassert reset\n");
+			goto err_mod;
+		}
+	}
+
 	return 0;
 
+err_mod:
+	clk_disable(&priv->clk_mod);
 err_ahb:
 	clk_disable(&priv->clk_ahb);
 	return ret;
@@ -328,6 +358,12 @@  static int sun4i_spi_probe(struct udevice *bus)
 		return ret;
 	}
 
+	ret = reset_get_by_index(bus, 0, &priv->reset);
+	if (ret && ret != -ENOENT) {
+		dev_err(dev, "failed to get reset\n");
+		return ret;
+	}
+
 	sun4i_spi_parse_pins(bus);
 
 	priv->variant = plat->variant;
@@ -351,6 +387,10 @@  static int sun4i_spi_claim_bus(struct udevice *dev)
 		     SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER |
 		     variant->bits[SPI_GCR_TP]);
 
+	if (IS_ENABLED(CONFIG_SUNXI_GEN_SUN6I))
+		setbits_le32(priv->base_addr + variant->regs[SPI_GCR],
+			     variant->bits[SPI_GCR_SRST]);
+
 	setbits_le32(priv->base_addr + variant->regs[SPI_TCR],
 		     variant->bits[SPI_TCR_CS_MANUAL] |
 		     variant->bits[SPI_TCR_CS_ACTIVE_LOW]);
@@ -409,6 +449,9 @@  static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		       priv->base_addr + variant->regs[SPI_BC]);
 		writel(SUN4I_XMIT_CNT(nbytes),
 		       priv->base_addr + variant->regs[SPI_TC]);
+		if (IS_ENABLED(CONFIG_SUNXI_GEN_SUN6I))
+			writel(SUN4I_BURST_CNT(nbytes),
+			       priv->base_addr + variant->regs[SPI_BCTL]);
 
 		/* Fill the TX FIFO */
 		sun4i_spi_fill_fifo(priv, nbytes);
@@ -547,17 +590,56 @@  static const unsigned long sun4i_spi_bits[] = {
 	[SPI_FSR_RF_CNT_MASK]	= GENMASK(6, 0),
 };
 
+static const unsigned long sun8i_spi_regs[] = {
+	[SPI_GCR]		= SUN6I_GBL_CTL_REG,
+	[SPI_TCR]		= SUN6I_TFR_CTL_REG,
+	[SPI_FCR]		= SUN6I_FIFO_CTL_REG,
+	[SPI_FSR]		= SUN6I_FIFO_STA_REG,
+	[SPI_CCR]		= SUN6I_CLK_CTL_REG,
+	[SPI_BC]		= SUN6I_BURST_CNT_REG,
+	[SPI_TC]		= SUN6I_XMIT_CNT_REG,
+	[SPI_BCTL]		= SUN6I_BURST_CTL_REG,
+	[SPI_TXD]		= SUN6I_TXDATA_REG,
+	[SPI_RXD]		= SUN6I_RXDATA_REG,
+};
+
+static const unsigned long sun8i_spi_bits[] = {
+	[SPI_GCR_TP]		= BIT(7),
+	[SPI_GCR_SRST]		= BIT(31),
+	[SPI_TCR_CPHA]		= BIT(0),
+	[SPI_TCR_CPOL]		= BIT(1),
+	[SPI_TCR_CS_ACTIVE_LOW] = BIT(2),
+	[SPI_TCR_CS_SEL]	= 4,
+	[SPI_TCR_CS_MASK]	= 0x30,
+	[SPI_TCR_CS_MANUAL]	= BIT(6),
+	[SPI_TCR_CS_LEVEL]	= BIT(7),
+	[SPI_TCR_XCH]		= BIT(31),
+	[SPI_FCR_RF_RST]	= BIT(15),
+	[SPI_FCR_TF_RST]	= BIT(31),
+	[SPI_FSR_RF_CNT_MASK]	= GENMASK(7, 0),
+};
+
 static const struct sun4i_spi_variant sun4i_a10_spi_variant = {
 	.regs			= sun4i_spi_regs,
 	.bits			= sun4i_spi_bits,
 	.fifo_depth		= 64,
 };
 
+static const struct sun4i_spi_variant sun8i_h3_spi_variant = {
+	.regs			= sun8i_spi_regs,
+	.bits			= sun8i_spi_bits,
+	.fifo_depth		= 64,
+};
+
 static const struct udevice_id sun4i_spi_ids[] = {
 	{
 	  .compatible = "allwinner,sun4i-a10-spi",
 	  .data = (ulong)&sun4i_a10_spi_variant,
 	},
+	{
+	  .compatible = "allwinner,sun8i-h3-spi",
+	  .data = (ulong)&sun8i_h3_spi_variant,
+	},
 	{ }
 };