Add Broadcom MSPI driver
Subsystems (2)
cover Add Broadcom MSPI driver
This patchset adds support for Broadcoms MSPI SPI controller. It can be found on various SoC's such as Cygnus.
Jonathan Richardson (2):
ARM: dts: Add binding for Broadcom MSPI driver.
spi: bcm-mspi: Add support for Broadcom MSPI driver.
.../devicetree/bindings/spi/brcm,mspi-spi.txt | 39 ++ drivers/spi/Kconfig | 5 + drivers/spi/Makefile | 1 + drivers/spi/spi-bcm-mspi.c | 505 ++++++++++++++++++++ 4 files changed, 550 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt create mode 100644 drivers/spi/spi-bcm-mspi.c
Signature · 1 line
1.7.9.5
PATCH 1/2 ARM: dts: Add binding for Broadcom MSPI driver.
--- .../devicetree/bindings/spi/brcm,mspi-spi.txt | 39 ++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt new file +39 -0
@@ -0,0 +1,39 @@ +Broadcom MSPI controller + +The Broadcom MSPI controller is a SPI controller found on various chips such +as Cygnus. + +Required properties: +- compatible: Must be "brcm,mspi-v0". + +- reg: The first register is the physical base address of the MSPI controller. + The second register is the address of the MSPI interrupt control registers. It + is only required for chips that have a separate register set for extended + interrupt control. This is required for Cygnus. + +- interrupts: Interrupt ID of the controller. + +Optional properties: +- clocks: The MSPI reference clock. If not provided then it is assumed a clock + is enabled by default and no control of clock-frequency (see below) is + possible. + +- clock-names: The name of the reference clock. + +- clock-frequency: Desired frequency of the clock. This will set the serial + clock baud rate (SPBR) based on the reference clock frequency. The frequency + of the SPBR is mspi_clk / (2 * SPBR) where SPBR is a value between 1-255 + determined by the desired 'clock-frequency'. If not provided then the default + baud rate of the controller is used. + +Example: + +mspi: mspi@18047200 { + compatible = "brcm,mspi-v0"; + reg = <0x18047200 0x188>, + <0x180473a0 0x1c>; + interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&axi41_clk>; + clock-names = "mspi_clk"; + clock-frequency = <12500000>; +};
Signature · 1 line
1.7.9.5
Re: [PATCH 1/2] ARM: dts: Add binding for Broadcom MSPI driver.
Quoted 15 lines · Jonathan Richardson
--- .../devicetree/bindings/spi/brcm,mspi-spi.txt | 39 ++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txtDocumentation/devicetree/bindings/spi/brcm,mspi-spi.txt new file +2
-0@@ -0,0 +1,39 @@ +Broadcom MSPI controller +
Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt line 12
Quoted 10 lines
Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt new file +10
-0+The Broadcom MSPI controller is a SPI controller found on various chips such +as Cygnus. + +Required properties: +- compatible: Must be "brcm,mspi-v0". + +- reg: The first register is the physical base address of the MSPI controller. + The second register is the address of the MSPI interrupt control registers. It + is only required for chips that have a separate register set for extended + interrupt control. This is required for Cygnus.
In which case we would want to mandate the use of a "reg-names" property, because there could be other kinds of controllers, such as the one found on BCM7xxx where we have the 3 "reg" units, the first one is the MSPI controller register range, while the two others are used for the Boot SPI portion of the controller to accelerate flash reads, so completely unrelated here.
Right now, using your driver on this Device Tree/configuration makes bcm_mspi request the second register range and mistakenly assuming this is the extended interrupt control register.
You could argue that the BSPI portion should be a separate node, but even so, better be safe than sorry.
Signature · 5 lines
Florian -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/2 spi: bcm-mspi: Add support for Broadcom MSPI driver.
In reply to Jonathan RichardsonThe MSPI controller is a SPI controller found on various Broadcom SoC's such as Cygnus.
--- drivers/spi/Kconfig | 5 + drivers/spi/Makefile | 1 + drivers/spi/spi-bcm-mspi.c | 505 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 511 insertions(+) create mode 100644 drivers/spi/spi-bcm-mspi.c
drivers/spi/Kconfig +5 -0
@@ -121,6 +121,11 @@ config SPI_BCM53XX help Enable support for the SPI controller on Broadcom BCM53xx ARM SoCs. +config SPI_BCM_MSPI + tristate "Broadcom MSPI controller" + help + Enable support for the Broadcom MSPI controller. + config SPI_BCM63XX tristate "Broadcom BCM63xx SPI controller" depends on BCM63XX
drivers/spi/Makefile +1 -0
@@ -18,6 +18,7 @@ obj-$(CONFIG_SPI_BCM2835) += spi-bcm2835.o obj-$(CONFIG_SPI_BCM53XX) += spi-bcm53xx.o obj-$(CONFIG_SPI_BCM63XX) += spi-bcm63xx.o obj-$(CONFIG_SPI_BCM63XX_HSSPI) += spi-bcm63xx-hsspi.o +obj-$(CONFIG_SPI_BCM_MSPI) += spi-bcm-mspi.o obj-$(CONFIG_SPI_BFIN5XX) += spi-bfin5xx.o obj-$(CONFIG_SPI_ADI_V3) += spi-adi-v3.o obj-$(CONFIG_SPI_BFIN_SPORT) += spi-bfin-sport.o
drivers/spi/spi-bcm-mspi.c new file +505 -0
505 lines not on this page · raw message
Signature · 1 line
1.7.9.5
Re: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Quoted 2 lines · Jonathan Richardson
The MSPI controller is a SPI controller found on various Broadcom SoC's such as Cygnus.
After adding an include/linux/io.h to fix the build error on the implicit readl/write declaration, plus some additional tweaking, this seems to detect a SPI flash correctly on BCM7xxx, however read transfers don't return correct data, I think I have an idea why though.
Thanks!
Signature · 5 lines
Florian -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerRe: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
In reply to Jonathan Richardsondrivers/spi/spi-bcm-mspi.c line 123
Quoted 3 lines · Jonathan Richardson
drivers/spi/spi-bcm-mspi.c new file +3
-0+/* Wait for interrupt indicating transfer is complete. */ +if (!wait_for_completion_timeout (&mspi->xfer_done, +msecs_to_jiffies (10 ))) {
What if we have a large transfer on a slow bus?
drivers/spi/spi-bcm-mspi.c line 205
Quoted 6 lines
drivers/spi/spi-bcm-mspi.c new file +5
-0+while (bytes_processed < transfer->len) { +/* Start transfer and wait until complete. */ + err =bcm_mspi_start_transfer (mspi, !last_slot, slot); +if (err) +return err;
This isn't really start_transfer() is it? It's doing the entire operation.
drivers/spi/spi-bcm-mspi.c line 209
Quoted 3 lines
drivers/spi/spi-bcm-mspi.c new file +3
-0+/* Delay requested amount before next transfer. */ +udelay (transfer->delay_usecs); + }
This is buggy, it's applying the per-transfer delay every timme we fill the FIFO.
drivers/spi/spi-bcm-mspi.c line 215
Quoted 5 lines
drivers/spi/spi-bcm-mspi.c new file +5
-0+/* The rx data will go into RXRAM0/1 + last tx length. */ +if (slot +1 >=NUM_SLOTS ) + mspi->rx_slot =0 ; +else + mspi->rx_slot = slot +1 ;
How is this going to work for full duplex transfers if we had to fill the FIFO more than once?
drivers/spi/spi-bcm-mspi.c line 320
Quoted 8 lines
drivers/spi/spi-bcm-mspi.c new file +8
-0+static int bcm_mspi_transfer_one (struct spi_master *master, +struct spi_device *spidev,struct spi_transfer *transfer) +{ +int err; + +/* 8 bit transfers only are currently supported. */ +if (transfer->bits_per_word >8 ) +return -ENOTSUPP ;
Tell the core what the device supports and it will check for you.
drivers/spi/spi-bcm-mspi.c line 331
Quoted 11 lines
drivers/spi/spi-bcm-mspi.c new file +11
-0+ + err =bcm_mspi_tx_data (master, spidev, transfer); +if (err) +return err; + + err =bcm_mspi_rx_data (master, spidev, transfer); +if (err) +return err; + +return 0 ; +}
I would expect the recieve and transmit operations to be running in parallel, not executed one after another, given the need to keep manually filling and draining the FIFOs.
drivers/spi/spi-bcm-mspi.c line 382
Quoted 7 lines
drivers/spi/spi-bcm-mspi.c new file +7
-0+struct spi_master *master; +struct device *dev = &pdev->dev; +int err; +struct resource *res; +unsigned int irq; + +dev_info (dev,"Initializing BCM MSPI\n" );
Don't spam the logs like this, there's no content in this message.
drivers/spi/spi-bcm-mspi.c line 385
Quoted 2 lines
drivers/spi/spi-bcm-mspi.c new file +2
-0+ master =spi_alloc_master (dev,sizeof (*data)); +if (!master) {
devm_spi_alloc_master().
drivers/spi/spi-bcm-mspi.c line 448
Quoted 5 lines
drivers/spi/spi-bcm-mspi.c new file +5
-0+/* 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 base address\n" );
devm_ioremap_resource() will complain for you.
Re: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Quoted 7 lines · Mark Brown
drivers/spi/spi-bcm-mspi.c line 123
Quoted 3 lines · Jonathan Richardson
drivers/spi/spi-bcm-mspi.c new file +3
-0+/* Wait for interrupt indicating transfer is complete. */ +if (!wait_for_completion_timeout (&mspi->xfer_done, +msecs_to_jiffies (10 ))) {What if we have a large transfer on a slow bus?
I guess it depends on the reference clock frequency (as well as spbr) which we may not know because I made it optional. I could possibly calculate it. I'll look into this. I used the slowest possible spbr based on our ref clock frequency and tested large transfers which never timed out.
Quoted 10 lines
drivers/spi/spi-bcm-mspi.c line 205
Quoted 7 lines
drivers/spi/spi-bcm-mspi.c new file +5
-0+while (bytes_processed < transfer->len) { +/* Start transfer and wait until complete. */ + err =bcm_mspi_start_transfer (mspi, !last_slot, slot); +if (err) +return err;This isn't really
start_transfer()is it? It's doing the entire operation.
Yes, the entire operation.
Quoted 7 lines
drivers/spi/spi-bcm-mspi.c line 209
Quoted 4 lines
drivers/spi/spi-bcm-mspi.c new file +3
-0+/* Delay requested amount before next transfer. */ +udelay (transfer->delay_usecs); + }This is buggy, it's applying the per-transfer delay every timme we fill the FIFO.
Thanks, I'll fix.
Quoted 9 lines
drivers/spi/spi-bcm-mspi.c line 215
Quoted 6 lines
drivers/spi/spi-bcm-mspi.c new file +5
-0+/* The rx data will go into RXRAM0/1 + last tx length. */ +if (slot +1 >=NUM_SLOTS ) + mspi->rx_slot =0 ; +else + mspi->rx_slot = slot +1 ;How is this going to work for full duplex transfers if we had to fill the FIFO more than once?
See below, it's not.
Quoted 12 lines
drivers/spi/spi-bcm-mspi.c line 320
Quoted 9 lines
drivers/spi/spi-bcm-mspi.c new file +8
-0+static int bcm_mspi_transfer_one (struct spi_master *master, +struct spi_device *spidev,struct spi_transfer *transfer) +{ +int err; + +/* 8 bit transfers only are currently supported. */ +if (transfer->bits_per_word >8 ) +return -ENOTSUPP ;Tell the core what the device supports and it will check for you.
Quoted 15 lines
drivers/spi/spi-bcm-mspi.c line 331
Quoted 11 lines
drivers/spi/spi-bcm-mspi.c new file +11
-0+ + err =bcm_mspi_tx_data (master, spidev, transfer); +if (err) +return err; + + err =bcm_mspi_rx_data (master, spidev, transfer); +if (err) +return err; + +return 0 ; +}I would expect the recieve and transmit operations to be running in parallel, not executed one after another, given the need to keep manually filling and draining the FIFOs.
The driver was only written with NOR flash in mind as the slave. Since this is really just half duplex, it works, though it won't with a real full duplex slave. m25p80 never calls transfer_one with both tx and rx buffers. The rx bytes that we don't care about from a tx request are dropped.
We don't have any full duplex slaves so it's a hard to test. I could possibly re-write this so that tx/rx is interleaved and that should be good for full duplex or close enough that it won't have to be completely overhauled should we ever connect a full duplex slave to it. Maybe I should do that. This came from a pre-existing driver that had the same limitation.
Quoted 11 lines
drivers/spi/spi-bcm-mspi.c line 382
Quoted 8 lines
drivers/spi/spi-bcm-mspi.c new file +7
-0+struct spi_master *master; +struct device *dev = &pdev->dev; +int err; +struct resource *res; +unsigned int irq; + +dev_info (dev,"Initializing BCM MSPI\n" );Don't spam the logs like this, there's no content in this message.
Quoted 4 lines
drivers/spi/spi-bcm-mspi.c line 385
Quoted 2 lines
drivers/spi/spi-bcm-mspi.c new file +2
-0+ master =spi_alloc_master (dev,sizeof (*data)); +if (!master) {
devm_spi_alloc_master().
This doesn't exist but devm_spi_register_master() does which I used.
Quoted 9 lines
drivers/spi/spi-bcm-mspi.c line 448
Quoted 6 lines
drivers/spi/spi-bcm-mspi.c new file +5
-0+/* 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 base address\n" );
devm_ioremap_resource()will complain for you.
Thanks,
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Hi Jon,
comment below for full-duplex operation.
Quoted 49 lines · Jonathan Richardson
Quoted 11 lines · Mark Brown
drivers/spi/spi-bcm-mspi.c line 215
Quoted 5 lines · Jonathan Richardson
drivers/spi/spi-bcm-mspi.c new file +5
-0+/* The rx data will go into RXRAM0/1 + last tx length. */ +if (slot +1 >=NUM_SLOTS ) + mspi->rx_slot =0 ; +else + mspi->rx_slot = slot +1 ;How is this going to work for full duplex transfers if we had to fill the FIFO more than once?
See below, it's not.
Quoted 12 lines
drivers/spi/spi-bcm-mspi.c line 320
Quoted 9 lines
drivers/spi/spi-bcm-mspi.c new file +8
-0+static int bcm_mspi_transfer_one (struct spi_master *master, +struct spi_device *spidev,struct spi_transfer *transfer) +{ +int err; + +/* 8 bit transfers only are currently supported. */ +if (transfer->bits_per_word >8 ) +return -ENOTSUPP ;Tell the core what the device supports and it will check for you.
Quoted 15 lines
drivers/spi/spi-bcm-mspi.c line 331
Quoted 11 lines
drivers/spi/spi-bcm-mspi.c new file +11
-0+ + err =bcm_mspi_tx_data (master, spidev, transfer); +if (err) +return err; + + err =bcm_mspi_rx_data (master, spidev, transfer); +if (err) +return err; + +return 0 ; +}I would expect the recieve and transmit operations to be running in parallel, not executed one after another, given the need to keep manually filling and draining the FIFOs.
The driver was only written with NOR flash in mind as the slave. Since this is really just half duplex, it works, though it won't with a real full duplex slave. m25p80 never calls
transfer_onewith both tx and rx buffers. The rx bytes that we don't care about from a tx request are dropped.
Quoted 8 lines
We don't have any full duplex slaves so it's a hard to test. I could possibly re-write this so that tx/rx is interleaved and that should be good for full duplex or close enough that it won't have to be completely overhauled should we ever connect a full duplex slave to it. Maybe I should do that. This came from a pre-existing driver that had the same limitation.
Quoted 1 line
The purpose of this mspi interface is to connect to NOR flash. There are other SPI interfaces on the devices used to connect to other SPI devies. We don't have any need to support full duplex slaves on this port (NOR have any hardware wired this way - pun realized after typing this). --
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerRe: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Quoted 4 lines · Scott Branden
The purpose of this mspi interface is to connect to NOR flash. There are other SPI interfaces on the devices used to connect to other SPI devies. We don't have any need to support full duplex slaves on this port (NOR have any hardware wired this way - pun realized after typing this).
Chip vendors often say this sort of thing and then get surprised by what their users choose to do, and even if it only ever gets used with flash all it would take is some new flash command which can use full duplex for something. Please write the code so it at least tries to handle full duplex operation, if you can't test it fully that's not the end of the world. It doesn't look like it should be particularly difficult.
Re: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Hi Mark,
Quoted 7 lines · Mark Brown
Quoted 4 lines · Scott Branden
The purpose of this mspi interface is to connect to NOR flash. There are other SPI interfaces on the devices used to connect to other SPI devies. We don't have any need to support full duplex slaves on this port (NOR have any hardware wired this way - pun realized after typing this).
Quoted 7 lines
Chip vendors often say this sort of thing and then get surprised by what their users choose to do, and even if it only ever gets used with flash all it would take is some new flash command which can use full duplex for something. Please write the code so it at least tries to handle full duplex operation, if you can't test it fully that's not the end of the world. It doesn't look like it should be particularly difficult.
Yes, there is always room for improvements in code. In this case - it really is not worth our time to add code we can't test. We try to deliver code that we can test and actually works. Yes, if anyone needs to use the mspi for full duplex operation code can be added in the future - it is software. This block has gone through many generations of our SoCs and has only been used for this purpose - the bootROM boots from this SPI only. It is dedicated for this purpose.
Also, there are other SPI blocks on the SoC that are used for connect other SPI devices. These SPI blocks are properly designed for full-duplex operation. This mspi block is designed for connecting to NOR flash.
Regards,
Scott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Quoted 28 lines · Scott Branden
Hi Mark,
Quoted 10 lines · Mark Brown
Quoted 7 lines · Scott Branden
The purpose of this mspi interface is to connect to NOR flash. There are
other SPI interfaces on the devices used to connect to other SPI devies. We
don't have any need to support full duplex slaves on this port (NOR have any
hardware wired this way - pun realized after typing this).Quoted 7 lines
Chip vendors often say this sort of thing and then get surprised by what their users choose to do, and even if it only ever gets used with flash all it would take is some new flash command which can use full duplex for something. Please write the code so it at least tries to handle full duplex operation, if you can't test it fully that's not the end of the world. It doesn't look like it should be particularly difficult.
Yes, there is always room for improvements in code. In this case - it really is not worth our time to add code we can't test. We try to deliver code that we can test and actually works. Yes, if anyone needs to use the mspi for full duplex operation code can be added in the future - it is software. This block has gone through many generations of our SoCs and has only been used for this purpose - the bootROM boots from this SPI only. It is dedicated for this purpose.
Quoted 4 lines
Also, there are other SPI blocks on the SoC that are used for connect other SPI devices. These SPI blocks are properly designed for full-duplex operation. This mspi block is designed for connecting to NOR flash.
This is an implementation detail and architectural choice that is specific to the Cygnus SoCs where a single MSPI is present, right?
This same MSPI block is instantiated multiple times in BCM7xxx chips to interface to general purpose SPI slaves, ranging from Ethernet switches to bluetooth chips etc... those will definitively want full-duplex to be working.
I would greatly appreciate if some thought to supporting full duplex transfers was given in this driver drop. If you cannot test that, then maybe just flag the driver has been half-duplex only for now...
Thank you.
Signature · 5 lines
Florian -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Quoted 42 lines · Florian Fainelli
Quoted 28 lines · Scott Branden
Hi Mark,
Quoted 10 lines · Mark Brown
Quoted 7 lines · Scott Branden
The purpose of this mspi interface is to connect to NOR flash. There are
other SPI interfaces on the devices used to connect to other SPI devies. We
don't have any need to support full duplex slaves on this port (NOR have any
hardware wired this way - pun realized after typing this).Quoted 7 lines
Chip vendors often say this sort of thing and then get surprised by what their users choose to do, and even if it only ever gets used with flash all it would take is some new flash command which can use full duplex for something. Please write the code so it at least tries to handle full duplex operation, if you can't test it fully that's not the end of the world. It doesn't look like it should be particularly difficult.
Yes, there is always room for improvements in code. In this case - it really is not worth our time to add code we can't test. We try to deliver code that we can test and actually works. Yes, if anyone needs to use the mspi for full duplex operation code can be added in the future - it is software. This block has gone through many generations of our SoCs and has only been used for this purpose - the bootROM boots from this SPI only. It is dedicated for this purpose.
Quoted 4 lines
Also, there are other SPI blocks on the SoC that are used for connect other SPI devices. These SPI blocks are properly designed for full-duplex operation. This mspi block is designed for connecting to NOR flash.
This is an implementation detail and architectural choice that is specific to the Cygnus SoCs where a single MSPI is present, right?
This same MSPI block is instantiated multiple times in BCM7xxx chips to interface to general purpose SPI slaves, ranging from Ethernet switches to bluetooth chips etc... those will definitively want full-duplex to be working.
Quoted 6 lines
I would greatly appreciate if some thought to supporting full duplex transfers was given in this driver drop. If you cannot test that, then maybe just flag the driver has been half-duplex only for now...
Thank you.
Hi Florian,
Yes, we can flag that - but we have no understanding of how the block
operates in this mode. It would be great if your team could add the necessary full duplex support in a patch enhancement to the driver?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Quoted 48 lines · Scott Branden
Quoted 47 lines · Florian Fainelli
Quoted 29 lines · Scott Branden
Hi Mark,
Quoted 10 lines · Mark Brown
Quoted 7 lines · Scott Branden
The purpose of this mspi interface is to connect to NOR flash. There are
other SPI interfaces on the devices used to connect to other SPI devies. We
don't have any need to support full duplex slaves on this port (NOR have any
hardware wired this way - pun realized after typing this).Quoted 8 lines
Chip vendors often say this sort of thing and then get surprised by what
their users choose to do, and even if it only ever gets used with flash all it would take is some new flash command which can use full duplex for something. Please write the code so it at least tries to handle full duplex operation, if you can't test it fully that's not the end of the world. It doesn't look like it should be particularly difficult.Yes, there is always room for improvements in code. In this case - it really is not worth our time to add code we can't test. We try to deliver code that we can test and actually works. Yes, if anyone needs to use the mspi for full duplex operation code can be added in the future - it is software. This block has gone through many generations of our SoCs and has only been used for this purpose - the bootROM boots from this SPI only. It is dedicated for this purpose.
Quoted 4 lines
Also, there are other SPI blocks on the SoC that are used for connect other SPI devices. These SPI blocks are properly designed for full-duplex operation. This mspi block is designed for connecting to NOR flash.
This is an implementation detail and architectural choice that is specific to the Cygnus SoCs where a single MSPI is present, right?
This same MSPI block is instantiated multiple times in BCM7xxx chips to interface to general purpose SPI slaves, ranging from Ethernet switches to bluetooth chips etc... those will definitively want full-duplex to be working.
I would greatly appreciate if some thought to supporting full duplex transfers was given in this driver drop. If you cannot test that, then maybe just flag the driver has been half-duplex only for now...
Quoted 7 lines
Quoted 2 lines
Thank you.
Hi Florian,
Yes, we can flag that - but we have no understanding of how the block
operates in this mode. It would be great if your team could add the necessary full duplex support in a patch enhancement to the driver?
Going blind on this one isn't the way to go. If I had a real full duplex slave I could experiment with it to get it working. But with the docs I have on this and considering how finicky the block is I think we're wasting our time.
Jon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerRe: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
In reply to Scott BrandenQuoted 9 lines · Scott Branden
Quoted 6 lines · Mark Brown
Chip vendors often say this sort of thing and then get surprised by what their users choose to do, and even if it only ever gets used with flash all it would take is some new flash command which can use full duplex for something. Please write the code so it at least tries to handle full duplex operation, if you can't test it fully that's not the end of the world. It doesn't look like it should be particularly difficult.
Quoted 3 lines
Yes, there is always room for improvements in code. In this case - it really is not worth our time to add code we can't test. We try to deliver code that we can test and actually works. Yes, if anyone needs to use the
While I try to not apply code that has obvious problems with silent data corruption in it which is what we have just now.
Quoted 4 lines
mspi for full duplex operation code can be added in the future - it is software. This block has gone through many generations of our SoCs and has only been used for this purpose - the bootROM boots from this SPI only. It is dedicated for this purpose.
All it takes is one hardware engineer who sees a SPI controller and a GPIO they can use for chip select; I wouldn't be so sure that nobody ever fixed this up locally (or happened to use a device that only needed single duplex).
Re: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Quoted 22 lines · Mark Brown
Quoted 9 lines · Scott Branden
Quoted 6 lines · Mark Brown
Chip vendors often say this sort of thing and then get surprised by what their users choose to do, and even if it only ever gets used with flash all it would take is some new flash command which can use full duplex for something. Please write the code so it at least tries to handle full duplex operation, if you can't test it fully that's not the end of the world. It doesn't look like it should be particularly difficult.
Quoted 3 lines
Yes, there is always room for improvements in code. In this case - it really is not worth our time to add code we can't test. We try to deliver code that we can test and actually works. Yes, if anyone needs to use the
While I try to not apply code that has obvious problems with silent data corruption in it which is what we have just now.
Quoted 4 lines
mspi for full duplex operation code can be added in the future - it is software. This block has gone through many generations of our SoCs and has only been used for this purpose - the bootROM boots from this SPI only. It is dedicated for this purpose.
Quoted 5 lines
All it takes is one hardware engineer who sees a SPI controller and a GPIO they can use for chip select; I wouldn't be so sure that nobody ever fixed this up locally (or happened to use a device that only needed single duplex).
Hi Mark. Would it help if we just set the flags to half duplex using SPI_MASTER_HALF_DUPLEX when we register the master?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerRe: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Quoted 9 lines · Jonathan Richardson
Quoted 4 lines · Mark Brown
All it takes is one hardware engineer who sees a SPI controller and a GPIO they can use for chip select; I wouldn't be so sure that nobody ever fixed this up locally (or happened to use a device that only needed single duplex).
Hi Mark. Would it help if we just set the flags to half duplex using
SPI_MASTER_HALF_DUPLEXwhen we register the master?
That would deal with the data corruption issues, yes. Though it seems like there are some people who want full duplex support here - I don't know if any of them can point you at suitable hardware to test?
Re: [PATCH 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
Quoted 8 lines · Mark Brown
Quoted 6 lines · Jonathan Richardson
Quoted 4 lines · Mark Brown
All it takes is one hardware engineer who sees a SPI controller and a GPIO they can use for chip select; I wouldn't be so sure that nobody ever fixed this up locally (or happened to use a device that only needed single duplex).
Quoted 6 lines
Quoted 2 lines
Hi Mark. Would it help if we just set the flags to half duplex using
SPI_MASTER_HALF_DUPLEXwhen we register the master?That would deal with the data corruption issues, yes. Though it seems like there are some people who want full duplex support here - I don't know if any of them can point you at suitable hardware to test?
I am fine with having the driver supporting only half-duplex for now, considering that Jonathan and Scott have no peripheral to test full-duplex with.
Getting you platforms and test setups could take more time than needed so, let's proceed with the shortest path.
Signature · 5 lines
Florian -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html