[U-Boot] [PATCH v2 1/3] mips: spi: mscc: Add fast bitbang SPI driver

Lars.Povlsen at microchip.com Lars.Povlsen at microchip.com
Tue Jan 8 08:54:46 UTC 2019


Hi again,

Thank you for your input, I will submit a (hopefully)
final v3 patch right away.

Comments below,

---Lars

> -----Original Message-----
> From: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> Sent: Monday, January 7, 2019 18:32
> To: Lars Povlsen - M31675 <Lars.Povlsen at microchip.com>; u-
> boot at lists.denx.de
> Cc: gregory.clement at bootlin.com; Horatiu Vultur - M31836
> <Horatiu.Vultur at microchip.com>
> Subject: Re: [PATCH v2 1/3] mips: spi: mscc: Add fast bitbang SPI driver
> 
> 
> 
> Am 07.01.19 um 14:28 schrieb Lars Povlsen:
> > This patch add a new SPI driver for MSCC SOCs that does not sport the
> > designware SPI hardware controller.
> >
> > Performance gain: 7.664 seconds vs. 17.633 for 1 Mbyte write.
> >
> > Signed-off-by: Lars Povlsen <lars.povlsen at microchip.com>
> > ---
> >  MAINTAINERS                               |   1 +
> >  arch/mips/mach-mscc/include/mach/common.h |  38 ++++
> >  drivers/spi/Kconfig                       |   7 +
> >  drivers/spi/Makefile                      |   1 +
> >  drivers/spi/mscc_bb_spi.c                 | 253
> ++++++++++++++++++++++
> >  5 files changed, 300 insertions(+)
> >  create mode 100644 drivers/spi/mscc_bb_spi.c
> >
> 
> Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> 
> nits below
> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 494962e9b3..0cee99ef56 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -524,6 +524,7 @@ F:	arch/mips/dts/ocelot*
> >  F:	board/mscc/
> >  F:	configs/mscc*
> >  F:	drivers/gpio/mscc_sgpio.c
> > +F:	drivers/spi/mscc_bb_spi.c
> >  F:	include/configs/vcoreiii.h
> >
> >  MIPS JZ4780
> > diff --git a/arch/mips/mach-mscc/include/mach/common.h
> b/arch/mips/mach-mscc/include/mach/common.h
> > index d18ae78bfd..7765c060ed 100644
> > --- a/arch/mips/mach-mscc/include/mach/common.h
> > +++ b/arch/mips/mach-mscc/include/mach/common.h
> > @@ -29,6 +29,44 @@
> >
> >  /* Common utility functions */
> >
> > +/*
> > + * Perform a number of NOP instructions, blocks of 8 instructions.
> > + * The (inlined) function will not affect cache or processor state.
> > + */
> > +static inline void mscc_vcoreiii_nop_delay(int delay)
> > +{
> > +	while (delay > 0) {
> > +#define DELAY_8_NOPS() asm volatile("nop; nop; nop; nop; nop; nop;
> nop; nop;")
> > +		switch (delay) {
> > +		case 8:
> > +			DELAY_8_NOPS();
> > +			/* fallthrough */
> > +		case 7:
> > +			DELAY_8_NOPS();
> > +			/* fallthrough */
> > +		case 6:
> > +			DELAY_8_NOPS();
> > +			/* fallthrough */
> > +		case 5:
> > +			DELAY_8_NOPS();
> > +			/* fallthrough */
> > +		case 4:
> > +			DELAY_8_NOPS();
> > +			/* fallthrough */
> > +		case 3:
> > +			DELAY_8_NOPS();
> > +			/* fallthrough */
> > +		case 2:
> > +			DELAY_8_NOPS();
> > +			/* fallthrough */
> > +		case 1:
> > +			DELAY_8_NOPS();
> > +		}
> > +		delay -= 8;
> > +#undef DELAY_8_NOPS
> > +	}
> > +}
> > +
> >  int mscc_phy_rd_wr(u8 read,
> >  		   u32 miim_controller,
> >  		   u8 miim_addr,
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index a7bb5b35c2..de4d62dd8f 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -294,6 +294,13 @@ config SOFT_SPI
> >  	 Enable Soft SPI driver. This driver is to use GPIO simulate
> >  	 the SPI protocol.
> >
> > +config MSCC_BB_SPI
> > +	bool "MSCC bitbang SPI driver"
> > +	depends on SOC_VCOREIII
> > +	help
> > +	  Enable MSCC bitbang SPI driver. This driver can be used on
> > +	  MSCC SOCs.
> > +
> >  config CF_SPI
> >  	bool "ColdFire SPI driver"
> >  	help
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index 392a925795..4acec3ea17 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -36,6 +36,7 @@ obj-$(CONFIG_MPC8XX_SPI) += mpc8xx_spi.o
> >  obj-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
> >  obj-$(CONFIG_MTK_QSPI) += mtk_qspi.o
> >  obj-$(CONFIG_MT7621_SPI) += mt7621_spi.o
> > +obj-$(CONFIG_MSCC_BB_SPI) += mscc_bb_spi.o
> >  obj-$(CONFIG_MVEBU_A3700_SPI) += mvebu_a3700_spi.o
> >  obj-$(CONFIG_MXC_SPI) += mxc_spi.o
> >  obj-$(CONFIG_MXS_SPI) += mxs_spi.o
> > diff --git a/drivers/spi/mscc_bb_spi.c b/drivers/spi/mscc_bb_spi.c
> > new file mode 100644
> > index 0000000000..5685878597
> > --- /dev/null
> > +++ b/drivers/spi/mscc_bb_spi.c
> > @@ -0,0 +1,253 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Microsemi SoCs spi driver
> > + *
> > + * Copyright (c) 2018 Microsemi Corporation
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <errno.h>
> > +#include <malloc.h>
> > +#include <spi.h>
> > +#include <dm.h>
> > +#include <asm/gpio.h>
> > +#include <asm/io.h>
> > +#include <linux/delay.h>
> > +
> > +struct mscc_bb_platdata {
> > +	void __iomem *regs;
> > +	u32 deactivate_delay_us;
> > +};
> 
> you could drop this along with mscc_bb_spi_ofdata_to_platdata() if you
> don't need to support a non-device-tree config or a SPL with DM and
> platdata only (I guess neither is true for MSCC platform). Otherwise you
> can init mscc_bb_priv from device-tree in your probe function.

We only use DT configs, so I will move this to the probe func.

> 
> > +
> > +struct mscc_bb_priv {
> > +	void __iomem *regs;
> > +	bool cs_active;   /* State flag as to whether CS is asserted */
> > +	int cs_num;
> > +	u32 svalue;			/* Value to start transfer with */
> > +	u32 clk1;			/* Clock value start */
> > +	u32 clk2;			/* Clock value 2nd phase */
> > +};
> > +
> > +/* Delay 24 instructions for this particular application */
> > +#define hold_time_delay() mscc_vcoreiii_nop_delay(3)
> > +
> > +static int mscc_bb_spi_cs_activate(struct mscc_bb_priv *priv, int
> mode, int cs)
> > +{
> > +	if (!priv->cs_active) {
> > +		int cpha = mode & SPI_CPHA;
> > +		u32 cs_value;
> > +
> > +		priv->cs_num = cs;
> > +
> > +		if (cpha) {
> > +			/* Initial clock starts SCK=1 */
> > +			priv->clk1 = ICPU_SW_MODE_SW_SPI_SCK;
> > +			priv->clk2 = 0;
> > +		} else {
> > +			/* Initial clock starts SCK=0 */
> > +			priv->clk1 = 0;
> > +			priv->clk2 = ICPU_SW_MODE_SW_SPI_SCK;
> > +		}
> > +
> > +		/* Enable bitbang, SCK_OE, SDO_OE */
> > +		priv->svalue = (ICPU_SW_MODE_SW_PIN_CTRL_MODE | /* Bitbang */
> > +				ICPU_SW_MODE_SW_SPI_SCK_OE    | /* SCK_OE */
> > +				ICPU_SW_MODE_SW_SPI_SDO_OE);   /* SDO OE */
> > +
> > +		/* Add CS */
> > +		if (cs >= 0) {
> > +			cs_value =
> > +				ICPU_SW_MODE_SW_SPI_CS_OE(BIT(cs)) |
> > +				ICPU_SW_MODE_SW_SPI_CS(BIT(cs));
> > +		} else {
> > +			cs_value = 0;
> > +		}
> > +
> > +		priv->svalue |= cs_value;
> > +
> > +		/* Enable the CS in HW, Initial clock value */
> > +		writel(priv->svalue | priv->clk2, priv->regs);
> > +
> > +		priv->cs_active = true;
> > +		debug("Activated CS%d\n", priv->cs_num);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mscc_bb_spi_cs_deactivate(struct mscc_bb_priv *priv, int
> deact_delay)
> > +{
> > +	if (priv->cs_active) {
> > +		/* Keep driving the CLK to its current value while
> > +		 * actively deselecting CS.
> > +		 */
> > +		u32 value = readl(priv->regs);
> > +
> > +		value &= ~ICPU_SW_MODE_SW_SPI_CS_M;
> > +		writel(value, priv->regs);
> > +		hold_time_delay();
> > +
> > +		/* Stop driving the clock, but keep CS with nCS == 1 */
> > +		value &= ~ICPU_SW_MODE_SW_SPI_SCK_OE;
> > +		writel(value, priv->regs);
> > +
> > +		/* Deselect hold time delay */
> > +		if (deact_delay)
> > +			udelay(deact_delay);
> > +
> > +		/* Drop everything */
> > +		writel(0, priv->regs);
> > +
> > +		priv->cs_active = false;
> > +		debug("Deactivated CS%d\n", priv->cs_num);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int mscc_bb_spi_claim_bus(struct udevice *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +int mscc_bb_spi_release_bus(struct udevice *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +int mscc_bb_spi_xfer(struct udevice *dev, unsigned int bitlen,
> > +		     const void *dout, void *din, unsigned long flags)
> > +{
> > +	struct udevice *bus = dev_get_parent(dev);
> > +	struct dm_spi_slave_platdata *plat = dev_get_parent_platdata(dev);
> > +	struct mscc_bb_platdata *bus_plat = dev_get_platdata(bus);
> > +	struct mscc_bb_priv *priv = dev_get_priv(bus);
> > +	u32             i, count;
> > +	const u8	*txd = dout;
> > +	u8		*rxd = din;
> > +
> > +	debug("spi_xfer: slave %s:%s cs%d mode %d, dout %p din %p bitlen
> %u\n",
> > +	      dev->parent->name, dev->name, plat->cs,  plat->mode, dout,
> > +	      din, bitlen);
> > +
> > +	if (flags & SPI_XFER_BEGIN)
> > +		mscc_bb_spi_cs_activate(priv, plat->mode, plat->cs);
> > +
> > +	count = bitlen / 8;
> > +	for (i = 0; i < count; i++) {
> > +		u32 rx = 0, mask = 0x80, value;
> > +
> > +		while (mask) {
> > +			/* Initial condition: CLK is low. */
> > +			value = priv->svalue;
> > +			if (txd && txd[i] & mask)
> > +				value |= ICPU_SW_MODE_SW_SPI_SDO;
> > +
> > +			/* Drive data while taking CLK low. The device
> > +			 * we're accessing will sample on the
> > +			 * following rising edge and will output data
> > +			 * on this edge for us to be sampled at the
> > +			 * end of this loop.
> > +			 */
> > +			writel(value | priv->clk1, priv->regs);
> > +
> > +			/* Wait for t_setup. All devices do have a
> > +			 * setup-time, so we always insert some delay
> > +			 * here. Some devices have a very long
> > +			 * setup-time, which can be adjusted by the
> > +			 * user through vcoreiii_device->delay.
> > +			 */
> > +			hold_time_delay();
> > +
> > +			/* Drive the clock high. */
> > +			writel(value | priv->clk2, priv->regs);
> > +
> > +			/* Wait for t_hold. See comment about t_setup
> > +			 * above.
> > +			 */
> > +			hold_time_delay();
> > +
> > +			/* We sample as close to the next falling edge
> > +			 * as possible.
> > +			 */
> > +			value = readl(priv->regs);
> > +			if (value & ICPU_SW_MODE_SW_SPI_SDI)
> > +				rx |= mask;
> > +			mask >>= 1;
> > +		}
> > +		if (rxd) {
> > +			debug("Read 0x%02x\n", rx);
> > +			rxd[i] = (u8)rx;
> > +		}
> > +		debug("spi_xfer: byte %d/%d\n", i + 1, count);
> > +	}
> > +
> > +	debug("spi_xfer: done\n");
> > +
> > +	if (flags & SPI_XFER_END)
> > +		mscc_bb_spi_cs_deactivate(priv, bus_plat-
> >deactivate_delay_us);
> > +
> > +	return 0;
> > +}
> > +
> > +int mscc_bb_spi_set_speed(struct udevice *dev, unsigned int speed)
> > +{
> > +	/* Accept any speed */
> > +	return 0;
> > +}
> > +
> > +int mscc_bb_spi_set_mode(struct udevice *dev, unsigned int mode)
> > +{
> > +	return 0;
> > +}
> > +
> > +static const struct dm_spi_ops mscc_bb_ops = {
> > +	.claim_bus	= mscc_bb_spi_claim_bus,
> > +	.release_bus	= mscc_bb_spi_release_bus,
> > +	.xfer		= mscc_bb_spi_xfer,
> > +	.set_speed	= mscc_bb_spi_set_speed,
> > +	.set_mode	= mscc_bb_spi_set_mode,
> > +};
> > +
> > +static const struct udevice_id mscc_bb_ids[] = {
> > +	{ .compatible = "mscc,luton-bb-spi" },
> > +	{ }
> > +};
> > +
> > +static int mscc_bb_spi_probe(struct udevice *bus)
> > +{
> > +	struct mscc_bb_priv *priv = dev_get_priv(bus);
> > +	struct mscc_bb_platdata *plat = dev_get_platdata(bus);
> > +
> > +	debug("%s: loaded, priv %p, plat %p\n", __func__, priv, plat);
> > +
> > +	/* Initialize */
> > +	priv->regs = plat->regs;
> > +	priv->cs_active = false;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mscc_bb_spi_ofdata_to_platdata(struct udevice *bus)
> > +{
> > +	struct mscc_bb_platdata *plat = dev_get_platdata(bus);
> > +
> > +	plat->regs = (void __iomem *)dev_read_addr(bus);
> > +
> > +	plat->deactivate_delay_us =
> > +		dev_read_u32_default(bus, "spi-deactivate-delay", 0);
> 
> I think this property should have a "mscc," prefix if it's a custom
> properry?

At last count, 7 other drivers were using this property, so I was assuming
it was no so custom.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +U_BOOT_DRIVER(mscc_bb) = {
> > +	.name	= "mscc_bb",
> > +	.id	= UCLASS_SPI,
> > +	.of_match = mscc_bb_ids,
> > +	.ops	= &mscc_bb_ops,
> > +	.ofdata_to_platdata = mscc_bb_spi_ofdata_to_platdata,
> > +	.platdata_auto_alloc_size = sizeof(struct mscc_bb_platdata),
> > +	.priv_auto_alloc_size = sizeof(struct mscc_bb_priv),
> > +	.probe	= mscc_bb_spi_probe,
> > +};
> >
> 
> --
> - Daniel


More information about the U-Boot mailing list