Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754783AbbDHT1y (ORCPT ); Wed, 8 Apr 2015 15:27:54 -0400 Received: from arrakis.dune.hu ([78.24.191.176]:55413 "EHLO arrakis.dune.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753373AbbDHT1v convert rfc822-to-8bit (ORCPT ); Wed, 8 Apr 2015 15:27:51 -0400 MIME-Version: 1.0 In-Reply-To: <1428516275-12819-5-git-send-email-jonathar@broadcom.com> References: <1428516275-12819-1-git-send-email-jonathar@broadcom.com> <1428516275-12819-5-git-send-email-jonathar@broadcom.com> From: Jonas Gorski Date: Wed, 8 Apr 2015 21:27:19 +0200 Message-ID: Subject: Re: [PATCH v2 4/5] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips To: Jonathan Richardson Cc: Mark Brown , Dmitry Torokhov , Anatol Pomazau , Scott Branden , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , "linux-kernel@vger.kernel.org" , linux-spi@vger.kernel.org, bcm-kernel-feedback-list , "devicetree@vger.kernel.org" , Rafal Milecki Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15884 Lines: 488 Hi, On Wed, Apr 8, 2015 at 8:04 PM, Jonathan Richardson 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 > --- > 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 > +#include > #include > #include > #include > #include > #include > +#include > > #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 "); > -- Regards Jonas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/