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.schwierz...@gmail.com>
> Sent: Monday, January 7, 2019 18:32
> To: Lars Povlsen - M31675 <lars.povl...@microchip.com>; u-
> b...@lists.denx.de
> Cc: gregory.clem...@bootlin.com; Horatiu Vultur - M31836
> <horatiu.vul...@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.povl...@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.schwierz...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
