Re: [PATCH v2 4/5] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips

[Jonas Gorski <jogo@xxxxxxxxxxx>]

Hi,

On Wed, Apr 8, 2015 at 8:04 PM, Jonathan Richardson
<jonathar@xxxxxxxxxxxx> wrote:
> The Broadcom MSPI controller is used on various chips. The driver only
> supported BCM53xx chips with BCMA (an AMBA bus variant). It now supports
> both BCMA MSPI and non-BCMA MSPI. To do this the following changes were
> made:
>
>   - A new config for non-BCMA chips has been added.
>   - Common code between the BCMA and non BCMA version are shared.
>   - Function pointers to set read/write functions to abstract bcma
>     and non-bcma versions are provided.
>   - DT is now mandatory. Hard coded SPI devices are removed and must be
>     set in DT.
>   - Remove function was unnecessary and removed.
>
> Signed-off-by: Jonathan Richardson <jonathar@xxxxxxxxxxxx>
> ---
>  drivers/spi/Kconfig        |    5 +
>  drivers/spi/Makefile       |    1 +
>  drivers/spi/spi-bcm-mspi.c |  228 ++++++++++++++++++++++++++++++++------------
>  3 files changed, 171 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 766e08d..23f2357 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -120,6 +120,11 @@ config SPI_BCMA_MSPI
>         help
>           Enable support for the Broadcom BCMA MSPI controller.
>
> +config SPI_BCM_MSPI
> +       tristate "Broadcom MSPI controller"

You say "DT is now mandatory", but I don't see you depending on OF.
Does it compile with OF disabled?

> +       help
> +         Enable support for the Broadcom MSPI controller.
> +
>  config SPI_BCM63XX
>         tristate "Broadcom BCM63xx SPI controller"
>         depends on BCM63XX
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 6b58100..36872d2 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_SPI_ATH79)                       += spi-ath79.o
>  obj-$(CONFIG_SPI_AU1550)               += spi-au1550.o
>  obj-$(CONFIG_SPI_BCM2835)              += spi-bcm2835.o
>  obj-$(CONFIG_SPI_BCMA_MSPI)            += spi-bcm-mspi.o
> +obj-$(CONFIG_SPI_BCM_MSPI)             += spi-bcm-mspi.o

What happens if SPI_BCMA_MSPI and SPI_BCM_MSPI are both set? What
happens if they disagree, i.e. one is y, the other m?

>  obj-$(CONFIG_SPI_BCM63XX)              += spi-bcm63xx.o
>  obj-$(CONFIG_SPI_BCM63XX_HSSPI)                += spi-bcm63xx-hsspi.o
>  obj-$(CONFIG_SPI_BFIN5XX)              += spi-bfin5xx.o
> diff --git a/drivers/spi/spi-bcm-mspi.c b/drivers/spi/spi-bcm-mspi.c
> index 502227d..32bb1f0 100644
> --- a/drivers/spi/spi-bcm-mspi.c
> +++ b/drivers/spi/spi-bcm-mspi.c
> @@ -11,11 +11,13 @@
>   * GNU General Public License for more details.
>   */
>  #include <linux/kernel.h>
> +#include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/delay.h>
>  #include <linux/bcma/bcma.h>
>  #include <linux/spi/spi.h>
> +#include <linux/of.h>
>
>  #include "spi-bcm-mspi.h"
>
> @@ -25,22 +27,17 @@
>  #define BCM_MSPI_SPE_TIMEOUT_MS 80
>
>  struct bcm_mspi {
> +#ifdef CONFIG_SPI_BCMA_MSPI
>         struct bcma_device *core;
> -       struct spi_master *master;
> +#endif
>
> +       void __iomem *base;
> +       struct spi_master *master;

You could make this part a bit more readable by not reordering existing members.

>         size_t read_offset;
> -};
> -
> -static inline u32 bcm_mspi_read(struct bcm_mspi *mspi, u16 offset)
> -{
> -       return bcma_read32(mspi->core, offset);
> -}
>
> -static inline void bcm_mspi_write(struct bcm_mspi *mspi, u16 offset,
> -                                   u32 value)
> -{
> -       bcma_write32(mspi->core, offset, value);
> -}
> +       void (*mspi_write)(struct bcm_mspi *mspi, u16 offset, u32 value);
> +       u32 (*mspi_read)(struct bcm_mspi *mspi, u16 offset);
> +};

Why not keep these and let them call mspi->mspi_read() resp.
mspi->mspi_write()? It would reduce the amount of changes quite a bit.

>
>  static inline unsigned int bcm_mspi_calc_timeout(size_t len)
>  {
> @@ -56,7 +53,7 @@ static int bcm_mspi_wait(struct bcm_mspi *mspi, unsigned int timeout_ms)
>         /* SPE bit has to be 0 before we read MSPI STATUS */
>         deadline = jiffies + BCM_MSPI_SPE_TIMEOUT_MS * HZ / 1000;
>         do {
> -               tmp = bcm_mspi_read(mspi, MSPI_SPCR2);
> +               tmp = mspi->mspi_read(mspi, MSPI_SPCR2);
>                 if (!(tmp & MSPI_SPCR2_SPE))
>                         break;
>                 udelay(5);
> @@ -68,9 +65,9 @@ static int bcm_mspi_wait(struct bcm_mspi *mspi, unsigned int timeout_ms)
>         /* Check status */
>         deadline = jiffies + timeout_ms * HZ / 1000;
>         do {
> -               tmp = bcm_mspi_read(mspi, MSPI_STATUS);
> +               tmp = mspi->mspi_read(mspi, MSPI_STATUS);
>                 if (tmp & MSPI_STATUS_SPIF) {
> -                       bcm_mspi_write(mspi, MSPI_STATUS, 0);
> +                       mspi->mspi_write(mspi, MSPI_STATUS, 0);
>                         return 0;
>                 }
>
> @@ -79,7 +76,7 @@ static int bcm_mspi_wait(struct bcm_mspi *mspi, unsigned int timeout_ms)
>         } while (!time_after_eq(jiffies, deadline));
>
>  spi_timeout:
> -       bcm_mspi_write(mspi, MSPI_STATUS, 0);
> +       mspi->mspi_write(mspi, MSPI_STATUS, 0);
>
>         pr_err("Timeout waiting for SPI to be ready!\n");
>
> @@ -94,7 +91,7 @@ static void bcm_mspi_buf_write(struct bcm_mspi *mspi, u8 *w_buf,
>
>         for (i = 0; i < len; i++) {
>                 /* Transmit Register File MSB */
> -               bcm_mspi_write(mspi, MSPI_TXRAM + 4 * (i * 2),
> +               mspi->mspi_write(mspi, MSPI_TXRAM + 4 * (i * 2),
>                                  (unsigned int)w_buf[i]);
>         }
>
> @@ -105,28 +102,28 @@ static void bcm_mspi_buf_write(struct bcm_mspi *mspi, u8 *w_buf,
>                         tmp &= ~MSPI_CDRAM_CONT;
>                 tmp &= ~0x1;
>                 /* Command Register File */
> -               bcm_mspi_write(mspi, MSPI_CDRAM + 4 * i, tmp);
> +               mspi->mspi_write(mspi, MSPI_CDRAM + 4 * i, tmp);
>         }
>
>         /* Set queue pointers */
> -       bcm_mspi_write(mspi, MSPI_NEWQP, 0);
> -       bcm_mspi_write(mspi, MSPI_ENDQP, len - 1);
> +       mspi->mspi_write(mspi, MSPI_NEWQP, 0);
> +       mspi->mspi_write(mspi, MSPI_ENDQP, len - 1);
>
>         if (cont)
> -               bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 1);
> +               mspi->mspi_write(mspi, MSPI_WRITE_LOCK, 1);
>
>         /* Start SPI transfer */
> -       tmp = bcm_mspi_read(mspi, MSPI_SPCR2);
> +       tmp = mspi->mspi_read(mspi, MSPI_SPCR2);
>         tmp |= MSPI_SPCR2_SPE;
>         if (cont)
>                 tmp |= MSPI_SPCR2_CONT_AFTER_CMD;
> -       bcm_mspi_write(mspi, MSPI_SPCR2, tmp);
> +       mspi->mspi_write(mspi, MSPI_SPCR2, tmp);
>
>         /* Wait for SPI to finish */
>         bcm_mspi_wait(mspi, bcm_mspi_calc_timeout(len));
>
>         if (!cont)
> -               bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 0);
> +               mspi->mspi_write(mspi, MSPI_WRITE_LOCK, 0);
>
>         mspi->read_offset = len;
>  }
> @@ -144,34 +141,35 @@ static void bcm_mspi_buf_read(struct bcm_mspi *mspi, u8 *r_buf,
>                         tmp &= ~MSPI_CDRAM_CONT;
>                 tmp &= ~0x1;
>                 /* Command Register File */
> -               bcm_mspi_write(mspi, MSPI_CDRAM + 4 * i, tmp);
> +               mspi->mspi_write(mspi, MSPI_CDRAM + 4 * i, tmp);
>         }
>
>         /* Set queue pointers */
> -       bcm_mspi_write(mspi, MSPI_NEWQP, 0);
> -       bcm_mspi_write(mspi, MSPI_ENDQP, mspi->read_offset + len - 1);
> +       mspi->mspi_write(mspi, MSPI_NEWQP, 0);
> +       mspi->mspi_write(mspi, MSPI_ENDQP,
> +                        mspi->read_offset + len - 1);
>
>         if (cont)
> -               bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 1);
> +               mspi->mspi_write(mspi, MSPI_WRITE_LOCK, 1);
>
>         /* Start SPI transfer */
> -       tmp = bcm_mspi_read(mspi, MSPI_SPCR2);
> +       tmp = mspi->mspi_read(mspi, MSPI_SPCR2);
>         tmp |= MSPI_SPCR2_SPE;
>         if (cont)
>                 tmp |= MSPI_SPCR2_CONT_AFTER_CMD;
> -       bcm_mspi_write(mspi, MSPI_SPCR2, tmp);
> +       mspi->mspi_write(mspi, MSPI_SPCR2, tmp);
>
>         /* Wait for SPI to finish */
>         bcm_mspi_wait(mspi, bcm_mspi_calc_timeout(len));
>
>         if (!cont)
> -               bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 0);
> +               mspi->mspi_write(mspi, MSPI_WRITE_LOCK, 0);
>
>         for (i = 0; i < len; ++i) {
>                 int offset = mspi->read_offset + i;
>
>                 /* Data stored in the transmit register file LSB */
> -               r_buf[i] = (u8)bcm_mspi_read(mspi,
> +               r_buf[i] = (u8)mspi->mspi_read(mspi,
>                         MSPI_RXRAM + 4 * (1 + offset * 2));
>         }
>
> @@ -216,10 +214,104 @@ static int bcm_mspi_transfer_one(struct spi_master *master,
>         return 0;
>  }
>
> -static struct spi_board_info bcm_mspi_info = {
> -       .modalias       = "bcm53xxspiflash",
> +/*
> + * Allocate SPI master for both bcma and non bcma bus. The SPI device must be
> + * configured in DT.
> + */
> +static struct bcm_mspi *bcm_mspi_init(struct device *dev)
> +{
> +       struct bcm_mspi *data;
> +       struct spi_master *master;
> +
> +       master = spi_alloc_master(dev, sizeof(*data));
> +       if (!master) {
> +               dev_err(dev, "error allocating spi_master\n");
> +               return 0;

return NULL;

> +       }
> +
> +       data = spi_master_get_devdata(master);
> +       data->master = master;
> +
> +       /* SPI master will always use the SPI device(s) from DT. */
> +       master->dev.of_node = dev->of_node;
> +       master->transfer_one = bcm_mspi_transfer_one;
> +
> +       return data;
> +}
> +
> +#ifdef CONFIG_SPI_BCM_MSPI

Won't this break when being build as a module? I think you need

#if IS_ENABLED(CONFIG_SPI_BCM_MSPI)

> +
> +static const struct of_device_id bcm_mspi_dt[] = {
> +       { .compatible = "brcm,mspi" },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, bcm_mspi_dt);
> +
> +static inline u32 bcm_mspi_read(struct bcm_mspi *mspi, u16 offset)
> +{
> +       return readl(mspi->base + offset);
> +}
> +
> +static inline void bcm_mspi_write(struct bcm_mspi *mspi, u16 offset,
> +       u32 value)
> +{
> +       writel(value, mspi->base + offset);
> +}
> +
> +/*
> + * Probe routine for non-bcma devices.
> + */
> +static int bcm_mspi_probe(struct platform_device *pdev)
> +{
> +       struct bcm_mspi *data;
> +       struct device *dev = &pdev->dev;
> +       int err;
> +       struct resource *res;
> +
> +       dev_info(dev, "BCM MSPI probe\n");
> +
> +       data = bcm_mspi_init(dev);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       /* Map base memory address. */
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       data->base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(data->base)) {
> +               dev_err(&pdev->dev, "unable to map I/O memory\n");

devm_ioremap_resource will already complain for you.

> +               err = PTR_ERR(data->base);
> +               goto out;
> +       }
> +
> +       data->mspi_read = bcm_mspi_read;
> +       data->mspi_write = bcm_mspi_write;
> +       platform_set_drvdata(pdev, data);
> +
> +       err = devm_spi_register_master(dev, data->master);
> +       if (err)
> +               goto out;
> +
> +       return 0;
> +
> +out:
> +       spi_master_put(data->master);
> +       return err;
> +}
> +
> +static struct platform_driver bcm_mspi_driver = {
> +       .driver = {
> +               .name = "bcm-mspi",
> +               .of_match_table = bcm_mspi_dt,
> +       },
> +       .probe = bcm_mspi_probe,
>  };
>
> +module_platform_driver(bcm_mspi_driver);
> +
> +#endif
> +
> +#ifdef CONFIG_SPI_BCMA_MSPI

likewise.

> +
>  static const struct bcma_device_id bcm_mspi_bcma_tbl[] = {
>         BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_NS_QSPI, BCMA_ANY_REV,
>                 BCMA_ANY_CLASS),
> @@ -227,62 +319,70 @@ static const struct bcma_device_id bcm_mspi_bcma_tbl[] = {
>  };
>  MODULE_DEVICE_TABLE(bcma, bcm_mspi_bcma_tbl);
>
> +static const struct of_device_id bcm_bcma_mspi_dt[] = {
> +       { .compatible = "brcm,bcma-mspi" },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, bcm_mspi_dt);
> +
> +static inline u32 bcm_bcma_mspi_read(struct bcm_mspi *mspi, u16 offset)
> +{
> +       return bcma_read32(mspi->core, offset);
> +}
> +
> +static inline void bcm_bcma_mspi_write(struct bcm_mspi *mspi, u16 offset,
> +       u32 value)
> +{
> +       bcma_write32(mspi->core, offset, value);
> +}
> +
> +/*
> + * Probe routine for bcma devices.
> + */
>  static int bcm_mspi_bcma_probe(struct bcma_device *core)
>  {
>         struct bcm_mspi *data;
> -       struct spi_master *master;
>         int err;
>
>         dev_info(&core->dev, "BCM MSPI BCMA probe\n");
>
>         if (core->bus->drv_cc.core->id.rev != 42) {
> -               pr_err("SPI on SoC with unsupported ChipCommon rev\n");
> +               dev_err(&core->dev,
> +                       "SPI on SoC with unsupported ChipCommon rev\n");
>                 return -ENOTSUPP;
>         }
>
> -       master = spi_alloc_master(&core->dev, sizeof(*data));
> -       if (!master)
> +       data = bcm_mspi_init(&core->dev);
> +       if (!data)
>                 return -ENOMEM;
>
> -       data = spi_master_get_devdata(master);
> -       data->master = master;
> +       data->mspi_read = bcm_bcma_mspi_read;
> +       data->mspi_write = bcm_bcma_mspi_write;
>         data->core = core;
>
> -       master->transfer_one = bcm_mspi_transfer_one;
> -
>         bcma_set_drvdata(core, data);
>
>         err = devm_spi_register_master(&core->dev, data->master);
>         if (err) {
> -               spi_master_put(master);
> -               bcma_set_drvdata(core, NULL);
> -               goto out;
> +               spi_master_put(data->master);
> +               return err;
>         }
>
> -       /* Broadcom SoCs (at least with the CC rev 42) use SPI for flash only */
> -       spi_new_device(master, &bcm_mspi_info);

Why are you removing the registration of the flash device? Won't this
break bcm53xx's flash registration?

> -
> -out:
> -       return err;
> -}
> -
> -static void bcm_mspi_bcma_remove(struct bcma_device *core)
> -{
> -       struct bcm_mspi *mspi = bcma_get_drvdata(core);
> -
> -       spi_unregister_master(mspi->master);
> +       return 0;
>  }
>
>  static struct bcma_driver bcm_mspi_bcma_driver = {
>         .name           = KBUILD_MODNAME,
> +       .drv = {
> +               .of_match_table = bcm_bcma_mspi_dt,
> +       },
>         .id_table       = bcm_mspi_bcma_tbl,
>         .probe          = bcm_mspi_bcma_probe,
> -       .remove         = bcm_mspi_bcma_remove,
>  };
>
> -static int __init bcm_mspi_module_init(void)
> +static int __init bcm_mspi_bcma_module_init(void)
>  {
> -       int err = 0;
> +       int err;

Unrelated change.

>
>         err = bcma_driver_register(&bcm_mspi_bcma_driver);
>         if (err)
> @@ -291,13 +391,15 @@ static int __init bcm_mspi_module_init(void)
>         return err;
>  }
>
> -static void __exit bcm_mspi_module_exit(void)
> +static void __exit bcm_mspi_bcma_module_exit(void)
>  {
>         bcma_driver_unregister(&bcm_mspi_bcma_driver);
>  }
>
> -module_init(bcm_mspi_module_init);
> -module_exit(bcm_mspi_module_exit);
> +module_init(bcm_mspi_bcma_module_init);
> +module_exit(bcm_mspi_bcma_module_exit);
> +
> +#endif
>
>  MODULE_DESCRIPTION("Broadcom MSPI SPI Controller driver");
>  MODULE_AUTHOR("Rafał Miłecki <zajec5@xxxxxxxxx>");
> --


Regards
Jonas
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html