Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1381586 > unrolled thread

[PATCH for-next] spi: bcm53xx: add spi_flash_read callback for MMIO-based reads

Back to article view | Back to linux.kernel

Contents

  [PATCH for-next] spi: bcm53xx: add spi_flash_read callback for MMIO-based reads Rafał Miłecki <zajec5@gmail.com> - 2016-04-18 13:20 +0200
    Re: [PATCH for-next] spi: bcm53xx: add spi_flash_read callback for  MMIO-based reads Mark Brown <broonie@kernel.org> - 2016-04-18 13:30 +0200
      Re: [PATCH for-next] spi: bcm53xx: add spi_flash_read callback for  MMIO-based reads Rafał Miłecki <zajec5@gmail.com> - 2016-04-18 13:40 +0200
        Re: [PATCH for-next] spi: bcm53xx: add spi_flash_read callback for  MMIO-based reads Mark Brown <broonie@kernel.org> - 2016-04-18 14:00 +0200
        Re: [PATCH for-next] spi: bcm53xx: add spi_flash_read callback for  MMIO-based reads Vignesh R <vigneshr@ti.com> - 2016-04-18 15:50 +0200
    [PATCH V2 for-next] spi: bcm53xx: add spi_flash_read callback for MMIO-based reads Rafał Miłecki <zajec5@gmail.com> - 2016-04-18 14:40 +0200

#1381586 — [PATCH for-next] spi: bcm53xx: add spi_flash_read callback for MMIO-based reads

This implements more efficient reads of SPI-attached flash content.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/spi/spi-bcm53xx.c | 73 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 71 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bcm53xx.c b/drivers/spi/spi-bcm53xx.c
index cc3f938..3bc7b77 100644
--- a/drivers/spi/spi-bcm53xx.c
+++ b/drivers/spi/spi-bcm53xx.c
@@ -17,8 +17,10 @@
 struct bcm53xxspi {
 	struct bcma_device *core;
 	struct spi_master *master;
+	void __iomem *mmio_base;
 
 	size_t read_offset;
+	bool bspi;				/* Boot SPI mode with memory mapping */
 };
 
 static inline u32 bcm53xxspi_read(struct bcm53xxspi *b53spi, u16 offset)
@@ -32,6 +34,50 @@ static inline void bcm53xxspi_write(struct bcm53xxspi *b53spi, u16 offset,
 	bcma_write32(b53spi->core, offset, value);
 }
 
+static void bcm53xxspi_disable_bspi(struct bcm53xxspi *b53spi)
+{
+	struct device *dev = &b53spi->core->dev;
+	unsigned long deadline;
+	u32 tmp;
+
+	if (!b53spi->bspi)
+		return;
+
+	tmp = bcm53xxspi_read(b53spi, B53SPI_BSPI_MAST_N_BOOT_CTRL);
+	if (tmp & 0x1)
+		return;
+
+	deadline = jiffies + usecs_to_jiffies(200);
+	do {
+		tmp = bcm53xxspi_read(b53spi, B53SPI_BSPI_BUSY_STATUS);
+		if (!(tmp & 0x1)) {
+			bcm53xxspi_write(b53spi, B53SPI_BSPI_MAST_N_BOOT_CTRL,
+					 0x1);
+			ndelay(200);
+			b53spi->bspi = false;
+			return;
+		}
+		udelay(1);
+	} while (!time_after_eq(jiffies, deadline));
+
+	dev_warn(dev, "Timeout disabling BSPI\n");
+}
+
+static void bcm53xxspi_enable_bspi(struct bcm53xxspi *b53spi)
+{
+	u32 tmp;
+
+	if (b53spi->bspi)
+		return;
+
+	tmp = bcm53xxspi_read(b53spi, B53SPI_BSPI_MAST_N_BOOT_CTRL);
+	if (!(tmp & 0x1))
+		return;
+
+	bcm53xxspi_write(b53spi, B53SPI_BSPI_MAST_N_BOOT_CTRL, 0x0);
+	b53spi->bspi = true;
+}
+
 static inline unsigned int bcm53xxspi_calc_timeout(size_t len)
 {
 	/* Do some magic calculation based on length and buad. Add 10% and 1. */
@@ -176,6 +222,8 @@ static int bcm53xxspi_transfer_one(struct spi_master *master,
 	u8 *buf;
 	size_t left;
 
+	bcm53xxspi_disable_bspi(b53spi);
+
 	if (t->tx_buf) {
 		buf = (u8 *)t->tx_buf;
 		left = t->len;
@@ -206,6 +254,19 @@ static int bcm53xxspi_transfer_one(struct spi_master *master,
 	return 0;
 }
 
+static int bcm53xxspi_flash_read(struct spi_device *spi,
+				 struct spi_flash_read_message *msg)
+{
+	struct bcm53xxspi *b53spi = spi_master_get_devdata(spi->master);
+	int ret = 0;
+
+	bcm53xxspi_enable_bspi(b53spi);
+	memcpy_fromio(msg->buf, b53spi->mmio_base + msg->from, msg->len);
+	msg->retlen = msg->len;
+
+	return ret;
+}
+
 /**************************************************
  * BCMA
  **************************************************/
@@ -222,6 +283,7 @@ MODULE_DEVICE_TABLE(bcma, bcm53xxspi_bcma_tbl);
 
 static int bcm53xxspi_bcma_probe(struct bcma_device *core)
 {
+	struct device *dev = &core->dev;
 	struct bcm53xxspi *b53spi;
 	struct spi_master *master;
 	int err;
@@ -231,7 +293,7 @@ static int bcm53xxspi_bcma_probe(struct bcma_device *core)
 		return -ENOTSUPP;
 	}
 
-	master = spi_alloc_master(&core->dev, sizeof(*b53spi));
+	master = spi_alloc_master(dev, sizeof(*b53spi));
 	if (!master)
 		return -ENOMEM;
 
@@ -239,11 +301,18 @@ static int bcm53xxspi_bcma_probe(struct bcma_device *core)
 	b53spi->master = master;
 	b53spi->core = core;
 
+	if (core->addr_s[0])
+		b53spi->mmio_base = devm_ioremap(dev, core->addr_s[0], SZ_32M);
+	b53spi->bspi = true;
+	bcm53xxspi_disable_bspi(b53spi);
+
 	master->transfer_one = bcm53xxspi_transfer_one;
+	if (b53spi->mmio_base)
+		master->spi_flash_read = bcm53xxspi_flash_read;
 
 	bcma_set_drvdata(core, b53spi);
 
-	err = devm_spi_register_master(&core->dev, master);
+	err = devm_spi_register_master(dev, master);
 	if (err) {
 		spi_master_put(master);
 		bcma_set_drvdata(core, NULL);
-- 
1.8.4.5

[toc] | [next] | [standalone]

#1381597 — Re: [PATCH for-next] spi: bcm53xx: add spi_flash_read callback for MMIO-based reads

[Multipart message — attachments visible in raw view] — view raw

On Mon, Apr 18, 2016 at 01:10:43PM +0200, Rafał Miłecki wrote:

> +static int bcm53xxspi_flash_read(struct spi_device *spi,
> +				 struct spi_flash_read_message *msg)
> +{
> +	struct bcm53xxspi *b53spi = spi_master_get_devdata(spi->master);
> +	int ret = 0;
> +
> +	bcm53xxspi_enable_bspi(b53spi);
> +	memcpy_fromio(msg->buf, b53spi->mmio_base + msg->from, msg->len);
> +	msg->retlen = msg->len;

There's no bounds check here but...

> +	if (core->addr_s[0])
> +		b53spi->mmio_base = devm_ioremap(dev, core->addr_s[0], SZ_32M);

...we only mapped 32M here.  What if something tries to do a larger
read?  It's also a bit surprising that we're mapping a specific size
here rather than the entire resource.

[toc] | [prev] | [next] | [standalone]

#1381612 — Re: [PATCH for-next] spi: bcm53xx: add spi_flash_read callback for MMIO-based reads

On 18 April 2016 at 13:24, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Apr 18, 2016 at 01:10:43PM +0200, Rafał Miłecki wrote:
>
>> +static int bcm53xxspi_flash_read(struct spi_device *spi,
>> +                              struct spi_flash_read_message *msg)
>> +{
>> +     struct bcm53xxspi *b53spi = spi_master_get_devdata(spi->master);
>> +     int ret = 0;
>> +
>> +     bcm53xxspi_enable_bspi(b53spi);
>> +     memcpy_fromio(msg->buf, b53spi->mmio_base + msg->from, msg->len);
>> +     msg->retlen = msg->len;
>
> There's no bounds check here but...

That's true, I was looking at ti_qspi_spi_flash_read and somehow
incorrectly (!) assumed there is a check above. Of course there isn't
one and there can't be, I'll simply fix this code. I guess we should
fix ti_qspi_spi_flash_read in ti-qspi driver as well.

I also realized there wasn't any fallback introduced in:
mtd: devices: m25p80: add support for mmap read request
http://git.infradead.org/l2-mtd.git/commitdiff/08922f644878c9163ada8df3ef9def89be1d5e90

What shall we do if spi_flash_read fails? Should we always fallback to
the standard SPI flash read? Or should we standarize error codes
returned by spi_flash_read and fallback on some particular error code
only?


>> +     if (core->addr_s[0])
>> +             b53spi->mmio_base = devm_ioremap(dev, core->addr_s[0], SZ_32M);
>
> ...we only mapped 32M here.  What if something tries to do a larger
> read?  It's also a bit surprising that we're mapping a specific size
> here rather than the entire resource.

This is based on what I found in Broadcom's SDK (they don't release
any real specifications):
#define SI_NS_FLASH_WINDOW 0x02000000 /* Flash XIP Window */

-- 
Rafał

[toc] | [prev] | [next] | [standalone]

#1381640 — Re: [PATCH for-next] spi: bcm53xx: add spi_flash_read callback for MMIO-based reads

[Multipart message — attachments visible in raw view] — view raw

On Mon, Apr 18, 2016 at 01:38:28PM +0200, Rafał Miłecki wrote:

> What shall we do if spi_flash_read fails? Should we always fallback to
> the standard SPI flash read? Or should we standarize error codes
> returned by spi_flash_read and fallback on some particular error code
> only?

I'm not sure, probably just fall back all the time.

[toc] | [prev] | [next] | [standalone]

#1381713 — Re: [PATCH for-next] spi: bcm53xx: add spi_flash_read callback for MMIO-based reads

On 04/18/2016 05:08 PM, Rafał Miłecki wrote:
> On 18 April 2016 at 13:24, Mark Brown <broonie@kernel.org> wrote:
>> On Mon, Apr 18, 2016 at 01:10:43PM +0200, Rafał Miłecki wrote:
>>
>>> +static int bcm53xxspi_flash_read(struct spi_device *spi,
>>> +                              struct spi_flash_read_message *msg)
>>> +{
>>> +     struct bcm53xxspi *b53spi = spi_master_get_devdata(spi->master);
>>> +     int ret = 0;
>>> +
>>> +     bcm53xxspi_enable_bspi(b53spi);
>>> +     memcpy_fromio(msg->buf, b53spi->mmio_base + msg->from, msg->len);
>>> +     msg->retlen = msg->len;
>>
>> There's no bounds check here but...
> 
> That's true, I was looking at ti_qspi_spi_flash_read and somehow
> incorrectly (!) assumed there is a check above. Of course there isn't
> one and there can't be, I'll simply fix this code. I guess we should
> fix ti_qspi_spi_flash_read in ti-qspi driver as well.

My assumption was controller will provide ability to map entire flash
under memory mapped region either in oneshot or in parts. It would be
odd to have some parts of flash accessible under memory mapped mode and
some only under SPI mode. Therefore there is no bound check in case of
ti-qspi(but I see is no harm in adding one. I will fix it up).

> 
> I also realized there wasn't any fallback introduced in:
> mtd: devices: m25p80: add support for mmap read request
> http://git.infradead.org/l2-mtd.git/commitdiff/08922f644878c9163ada8df3ef9def89be1d5e90
> 
> What shall we do if spi_flash_read fails? Should we always fallback to
> the standard SPI flash read? Or should we standarize error codes
> returned by spi_flash_read and fallback on some particular error code
> only?
> 

spi_flash_read() can return error for incorrect protocol format
(opcode/data/address_nbits), this will fail anyways for normal SPI mode
as well. Else, its the error code returned by master->spi_flash_read()
callback (individual driver dependent). Not sure what can be
standardized here.
I think fallback would be to try SPI mode when ret!=-EINVAL.




-- 
Regards
Vignesh

[toc] | [prev] | [next] | [standalone]

#1381658 — [PATCH V2 for-next] spi: bcm53xx: add spi_flash_read callback for MMIO-based reads

This implements more efficient reads of SPI-attached flash content.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: Add BCM53XXSPI_FLASH_WINDOW and check it in bcm53xxspi_flash_read.
---
 drivers/spi/spi-bcm53xx.c | 78 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bcm53xx.c b/drivers/spi/spi-bcm53xx.c
index cc3f938..afb5169 100644
--- a/drivers/spi/spi-bcm53xx.c
+++ b/drivers/spi/spi-bcm53xx.c
@@ -10,6 +10,7 @@
 #include "spi-bcm53xx.h"
 
 #define BCM53XXSPI_MAX_SPI_BAUD	13500000	/* 216 MHz? */
+#define BCM53XXSPI_FLASH_WINDOW	SZ_32M
 
 /* The longest observed required wait was 19 ms */
 #define BCM53XXSPI_SPE_TIMEOUT_MS	80
@@ -17,8 +18,10 @@
 struct bcm53xxspi {
 	struct bcma_device *core;
 	struct spi_master *master;
+	void __iomem *mmio_base;
 
 	size_t read_offset;
+	bool bspi;				/* Boot SPI mode with memory mapping */
 };
 
 static inline u32 bcm53xxspi_read(struct bcm53xxspi *b53spi, u16 offset)
@@ -32,6 +35,50 @@ static inline void bcm53xxspi_write(struct bcm53xxspi *b53spi, u16 offset,
 	bcma_write32(b53spi->core, offset, value);
 }
 
+static void bcm53xxspi_disable_bspi(struct bcm53xxspi *b53spi)
+{
+	struct device *dev = &b53spi->core->dev;
+	unsigned long deadline;
+	u32 tmp;
+
+	if (!b53spi->bspi)
+		return;
+
+	tmp = bcm53xxspi_read(b53spi, B53SPI_BSPI_MAST_N_BOOT_CTRL);
+	if (tmp & 0x1)
+		return;
+
+	deadline = jiffies + usecs_to_jiffies(200);
+	do {
+		tmp = bcm53xxspi_read(b53spi, B53SPI_BSPI_BUSY_STATUS);
+		if (!(tmp & 0x1)) {
+			bcm53xxspi_write(b53spi, B53SPI_BSPI_MAST_N_BOOT_CTRL,
+					 0x1);
+			ndelay(200);
+			b53spi->bspi = false;
+			return;
+		}
+		udelay(1);
+	} while (!time_after_eq(jiffies, deadline));
+
+	dev_warn(dev, "Timeout disabling BSPI\n");
+}
+
+static void bcm53xxspi_enable_bspi(struct bcm53xxspi *b53spi)
+{
+	u32 tmp;
+
+	if (b53spi->bspi)
+		return;
+
+	tmp = bcm53xxspi_read(b53spi, B53SPI_BSPI_MAST_N_BOOT_CTRL);
+	if (!(tmp & 0x1))
+		return;
+
+	bcm53xxspi_write(b53spi, B53SPI_BSPI_MAST_N_BOOT_CTRL, 0x0);
+	b53spi->bspi = true;
+}
+
 static inline unsigned int bcm53xxspi_calc_timeout(size_t len)
 {
 	/* Do some magic calculation based on length and buad. Add 10% and 1. */
@@ -176,6 +223,8 @@ static int bcm53xxspi_transfer_one(struct spi_master *master,
 	u8 *buf;
 	size_t left;
 
+	bcm53xxspi_disable_bspi(b53spi);
+
 	if (t->tx_buf) {
 		buf = (u8 *)t->tx_buf;
 		left = t->len;
@@ -206,6 +255,22 @@ static int bcm53xxspi_transfer_one(struct spi_master *master,
 	return 0;
 }
 
+static int bcm53xxspi_flash_read(struct spi_device *spi,
+				 struct spi_flash_read_message *msg)
+{
+	struct bcm53xxspi *b53spi = spi_master_get_devdata(spi->master);
+	int ret = 0;
+
+	if (msg->from + msg->len > BCM53XXSPI_FLASH_WINDOW)
+		return -EINVAL;
+
+	bcm53xxspi_enable_bspi(b53spi);
+	memcpy_fromio(msg->buf, b53spi->mmio_base + msg->from, msg->len);
+	msg->retlen = msg->len;
+
+	return ret;
+}
+
 /**************************************************
  * BCMA
  **************************************************/
@@ -222,6 +287,7 @@ MODULE_DEVICE_TABLE(bcma, bcm53xxspi_bcma_tbl);
 
 static int bcm53xxspi_bcma_probe(struct bcma_device *core)
 {
+	struct device *dev = &core->dev;
 	struct bcm53xxspi *b53spi;
 	struct spi_master *master;
 	int err;
@@ -231,7 +297,7 @@ static int bcm53xxspi_bcma_probe(struct bcma_device *core)
 		return -ENOTSUPP;
 	}
 
-	master = spi_alloc_master(&core->dev, sizeof(*b53spi));
+	master = spi_alloc_master(dev, sizeof(*b53spi));
 	if (!master)
 		return -ENOMEM;
 
@@ -239,11 +305,19 @@ static int bcm53xxspi_bcma_probe(struct bcma_device *core)
 	b53spi->master = master;
 	b53spi->core = core;
 
+	if (core->addr_s[0])
+		b53spi->mmio_base = devm_ioremap(dev, core->addr_s[0],
+						 BCM53XXSPI_FLASH_WINDOW);
+	b53spi->bspi = true;
+	bcm53xxspi_disable_bspi(b53spi);
+
 	master->transfer_one = bcm53xxspi_transfer_one;
+	if (b53spi->mmio_base)
+		master->spi_flash_read = bcm53xxspi_flash_read;
 
 	bcma_set_drvdata(core, b53spi);
 
-	err = devm_spi_register_master(&core->dev, master);
+	err = devm_spi_register_master(dev, master);
 	if (err) {
 		spi_master_put(master);
 		bcma_set_drvdata(core, NULL);
-- 
1.8.4.5

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web