| Message ID | 20200214114618.29704-1-vadivel.muruganx.ramuthevar@linux.intel.com |
|---|---|
| Headers | show |
| Series | spi: cadence-quadpsi: Add support for the Cadence QSPI controller | expand |
On Fri, Feb 14, 2020 at 12:46 PM Ramuthevar,Vadivel MuruganX <vadivel.muruganx.ramuthevar@linux.intel.com> wrote: > > Add support for the Cadence QSPI controller. This controller is > present in the Intel Lightning Mountain(LGM) SoCs, Altera and TI SoCs. > This driver has been tested on the Intel LGM SoCs. This is v9 and still, none of the altera maintainers are on CC? How will it be ensured that this doesn't break altera if it is merged? Regards, Simon > > This driver does not support generic SPI and also the implementation > only supports spi-mem interface to replace the existing driver in > mtd/spi-nor/cadence-quadspi.c, the existing driver only support SPI-NOR > flash memory. > > Thanks a lot!!! Vignesh for the review, suggestion to optimize the patch. > Tested with mx25u12835f on LGM platform. > > changes from v8: > -- remove the depends MTD macro > -- comment into C++ style > -- remove spaces and tabs where not applicable. > -- align the macro string as same as existing one. > -- replace QUAD to op->data.buswidth variable. > -- add CQSPI_NEEDS_ADDR_SWAP instead of soc_selection variable > > changes from v7: > -- remove addr_buf kept like as original > -- drop bus-num, chipselect variable > -- add soc_selection varible to differetiate the features > -- replace dev->ddev in dma function > -- add seperate function to handle the 24bit slave device address > translation for lgm soc > -- correct sentence seems incomplete in Kconfig > -- add cqspi->soc_selection check to keep the original TI platform > working code. > > changes from v6: > -- Add the Signed-off-by Vignesh in commit message > -- bus_num, num_chipselect added to avoid the garbage bus number > during the probe and spi_register. > -- master mode bits updated > -- address sequence is different from TI and Intel SoC Ip handling > so modified as per Intel and differentiating by use_dac_mode variable. > -- dummy cycles also different b/w two platforms, so keeping separate check > -- checkpatch errors which are intentional left as is for better readability > > changes from v5: > -- kbuild test robot warnings fixed > -- Add Reported-By: Dan Carpenter <dan.carpenter@oracle.com> > > changes from v4: > -- kbuild test robot warnings fixed > -- Add Reborted-by: tag > > changes from v3: > spi-cadence-quadspi.c > -- static to all functions wrt to local to the file. > -- Prefix cqspi_ and make the function static > -- cmd_ops, data_ops and dummy_ops dropped > -- addr_ops kept since it is required for address calculation. > -- devm_ used for supported functions , removed legacy API's > -- removed "indirect" name from functions > -- replaced by master->mode_bits = SPI_RX_QUAD | SPI_TX_DUAL | SPI_RX_DUAL | SPI_RX_OCTAL; > as per Vignesh susggestion > -- removed free functions since devm_ handles automatically. > -- dropped all unused Macros > > YAML file update: > -- cadence,qspi.yaml file name replace by cdns,qspi-nor.yaml > -- compatible string updated as per Vignesh suggestion > -- for single entry, removed descriptions > -- removed optional parameters > Build Result: > linux$ make DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml dt_binding_check > CHKDT Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml > SCHEMA Documentation/devicetree/bindings/processed-schema.yaml > DTC Documentation/devicetree/bindings/spi/cdns,qspi-nor.example.dt.yaml > CHECK Documentation/devicetree/bindings/spi/cdns,qspi-nor.example.dt.yaml > > Ramuthevar Vadivel Murugan (2): > dt-bindings: spi: Add schema for Cadence QSPI Controller driver > spi: cadence-quadpsi: Add support for the Cadence QSPI controller > > .../devicetree/bindings/spi/cdns,qspi-nor.yaml | 147 ++ > drivers/spi/Kconfig | 8 + > drivers/spi/Makefile | 1 + > drivers/spi/spi-cadence-quadspi.c | 1508 ++++++++++++++++++++ > 4 files changed, 1664 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml > create mode 100644 drivers/spi/spi-cadence-quadspi.c > > -- > 2.11.0 >
On Fri, Feb 14, 2020 at 01:02:22PM +0100, Simon Goldschmidt wrote: > On Fri, Feb 14, 2020 at 12:46 PM Ramuthevar,Vadivel MuruganX > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote: > > Add support for the Cadence QSPI controller. This controller is > > present in the Intel Lightning Mountain(LGM) SoCs, Altera and TI SoCs. > > This driver has been tested on the Intel LGM SoCs. > This is v9 and still, none of the altera maintainers are on CC? > How will it be ensured that this doesn't break altera if it is merged? Given that this is a new driver I'd be very surprised if it broke other users? I can imagine it might not work for them and it would definitely be much better to get their review but it shouldn't be any worse than the current lack of support.
On Fri, Feb 14, 2020 at 1:11 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Feb 14, 2020 at 01:02:22PM +0100, Simon Goldschmidt wrote: > > On Fri, Feb 14, 2020 at 12:46 PM Ramuthevar,Vadivel MuruganX > > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote: > > > > Add support for the Cadence QSPI controller. This controller is > > > present in the Intel Lightning Mountain(LGM) SoCs, Altera and TI SoCs. > > > This driver has been tested on the Intel LGM SoCs. > > > This is v9 and still, none of the altera maintainers are on CC? > > How will it be ensured that this doesn't break altera if it is merged? > > Given that this is a new driver I'd be very surprised if it broke other > users? I can imagine it might not work for them and it would definitely > be much better to get their review but it shouldn't be any worse than > the current lack of support. It is a new driver, but the hardware it supports is not currently unsupported. Both Vadivel and Vignesh have stated that this driver merely moves the existing generic spi driver to the spi-mem interface and should replace the existing driver. So please correct me if I'm wrong, but to me it seems like if this driver won't work on altera, and after merging it the currently working driver will be removed, altera will be broken. Regards, Simon
On Fri, Feb 14, 2020 at 07:46:18PM +0800, Ramuthevar,Vadivel MuruganX wrote: > +static irqreturn_t cqspi_irq_handler(int this_irq, void *dev) > +{ > + struct cqspi_st *cqspi = dev; > + unsigned int irq_status; > + > + /* Read interrupt status */ > + irq_status = readl(cqspi->iobase + CQSPI_REG_IRQSTATUS); > + > + /* Clear interrupt */ > + writel(irq_status, cqspi->iobase + CQSPI_REG_IRQSTATUS); > + > + irq_status &= CQSPI_IRQ_MASK_RD | CQSPI_IRQ_MASK_WR; > + > + if (irq_status) > + complete(&cqspi->transfer_complete); > + > + return IRQ_HANDLED; > +} This will unconditionally handle the interrupt regardless of if the hardware was actually flagging an interrupt which will break shared interrupts and the fault handling code in genirq. > + tmpbufsize = op->addr.nbytes + op->dummy.nbytes; > + tmpbuf = kzalloc(tmpbufsize, GFP_KERNEL | GFP_DMA); > + if (!tmpbuf) > + return -ENOMEM; I'm not clear where tmpbuf gets freed or passed out of this function? > + > + if (op->addr.nbytes) { > + for (i = 0; i < op->addr.nbytes; i++) > + tmpbuf[i] = op->addr.val >> (8 * (op->addr.nbytes - i - 1)); > + > + addr_buf = tmpbuf; We assign tmpbuf to addr_buf here but addr_buf just gets read from so it's not via that AFAICT. > + } > + /* Invalid address return zero. */ Missing blank line. > +static void cqspi_chipselect(struct cqspi_flash_pdata *f_pdata) > +{ > + struct cqspi_st *cqspi = f_pdata->cqspi; > + void __iomem *reg_base = cqspi->iobase; > + unsigned int chip_select = f_pdata->cs; > + unsigned int reg; > + > + reg = readl(reg_base + CQSPI_REG_CONFIG); > + reg &= ~CQSPI_REG_CONFIG_DECODE_MASK; > + > + /* Convert CS if without decoder. > + * CS0 to 4b'1110 > + * CS1 to 4b'1101 > + * CS2 to 4b'1011 > + * CS3 to 4b'0111 > + */ > + chip_select = 0xF & ~(1 << chip_select); This says "if without decoder" but there's no conditionals here, what if we do have a decoder? > + cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk); > + ddata = of_device_get_match_data(dev); > + if (ddata) { > + if (ddata->quirks & CQSPI_NEEDS_WR_DELAY) > + cqspi->wr_delay = 5 * DIV_ROUND_UP(NSEC_PER_SEC, > + cqspi->master_ref_clk_hz); > + if (ddata->hwcaps_mask & CQSPI_SUPPORTS_OCTAL) > + master->mode_bits |= SPI_RX_OCTAL; > + if (!(ddata->quirks & CQSPI_DISABLE_DAC_MODE)) > + cqspi->use_dac_mode = true; > + if (ddata->quirks & CQSPI_NEEDS_ADDR_SWAP) { > + master->bus_num = 0; > + master->num_chipselect = 2; > + } > + } Given that the driver appears to unconditionally dereference match data in other places I'd expect this to return an error if there's none, otherwise we'll oops in those other code paths later on. > + ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0, > + pdev->name, cqspi); > + if (ret) { > + dev_err(dev, "Cannot request IRQ.\n"); > + goto probe_reset_failed; > + } Are you sure that it's safe to use devm_request_irq() - what happens if the interrupt fires in the process of removing the device?
On Fri, Feb 14, 2020 at 01:50:44PM +0100, Simon Goldschmidt wrote: > So please correct me if I'm wrong, but to me it seems like if this driver won't > work on altera, and after merging it the currently working driver will be > removed, altera will be broken. I'm not seeing anything in the driver that removes whatever the current support is? Unless it's just adding a duplicate driver for the same compatible strings which is obviously a bad idea but at least means that unless people enable the driver there's no risk of it colliding with the existing one.
On Fri, Feb 14, 2020 at 2:15 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Feb 14, 2020 at 01:50:44PM +0100, Simon Goldschmidt wrote: > > > So please correct me if I'm wrong, but to me it seems like if this driver won't > > work on altera, and after merging it the currently working driver will be > > removed, altera will be broken. > > I'm not seeing anything in the driver that removes whatever the current > support is? Unless it's just adding a duplicate driver for the same > compatible strings which is obviously a bad idea but at least means that > unless people enable the driver there's no risk of it colliding with the > existing one. It does add a duplicate driver for the same compatible strings. The current working driver is in 'drivers/mtd/spi-nor/cadence-quadspi.c'. In fact, the compatible string "cdns,qspi-nor" copied from the old driver to this new driver is *only* used for altera. TI has its own compatible string, the new Intel platform adds its own as well. As long as that one doesn't get removed, I have nothing against this driver here. I'm only concerned that this will get forgotten. And given that I added altera guys to the loop in one of the previous versions, I just was surprised they aren't on CC in this version. I'm not familiar with whom to CC for Linux drivers, so sorry for the noise if I'm overreacting here, just tell me. Regards, Simon
On Fri, Feb 14, 2020 at 02:49:48PM +0100, Simon Goldschmidt wrote: > On Fri, Feb 14, 2020 at 2:15 PM Mark Brown <broonie@kernel.org> wrote: > > I'm not seeing anything in the driver that removes whatever the current > > support is? Unless it's just adding a duplicate driver for the same > > compatible strings which is obviously a bad idea but at least means that > > unless people enable the driver there's no risk of it colliding with the > > existing one. > It does add a duplicate driver for the same compatible strings. The current > working driver is in 'drivers/mtd/spi-nor/cadence-quadspi.c'. > In fact, the compatible string "cdns,qspi-nor" copied from the old driver to > this new driver is *only* used for altera. TI has its own compatible string, > the new Intel platform adds its own as well. Oh, that's not good - it's adding a completely new binding for the same compatibles which isn't OK. We can transition to a new driver using the same binding but we should be keeping the old binding. If we're moving the binding document around and/or transitioning to YAML that needs to be done explicitly rather than adding a new document for the same compatible. > As long as that one doesn't get removed, I have nothing against this driver > here. I'm only concerned that this will get forgotten. And given that I added > altera guys to the loop in one of the previous versions, I just was surprised > they aren't on CC in this version. Yes, like I say it'd be much better to get their review.
Hi Mark, Thank you for the review comments, response in inline. On 14/2/2020 9:09 PM, Mark Brown wrote: > On Fri, Feb 14, 2020 at 07:46:18PM +0800, Ramuthevar,Vadivel MuruganX wrote: > >> +static irqreturn_t cqspi_irq_handler(int this_irq, void *dev) >> +{ >> + struct cqspi_st *cqspi = dev; >> + unsigned int irq_status; >> + >> + /* Read interrupt status */ >> + irq_status = readl(cqspi->iobase + CQSPI_REG_IRQSTATUS); >> + >> + /* Clear interrupt */ >> + writel(irq_status, cqspi->iobase + CQSPI_REG_IRQSTATUS); >> + >> + irq_status &= CQSPI_IRQ_MASK_RD | CQSPI_IRQ_MASK_WR; >> + >> + if (irq_status) >> + complete(&cqspi->transfer_complete); >> + >> + return IRQ_HANDLED; >> +} > This will unconditionally handle the interrupt regardless of if the > hardware was actually flagging an interrupt which will break shared > interrupts and the fault handling code in genirq. Yes, you're correct, it doesn't check unconditionally, will update the INT flag in the INT_STATUS register after successful completion of read/write operation. but in this case it is dedicated to qspi-interrupt,not shared with any other HW/SW interrupts. >> + tmpbufsize = op->addr.nbytes + op->dummy.nbytes; >> + tmpbuf = kzalloc(tmpbufsize, GFP_KERNEL | GFP_DMA); >> + if (!tmpbuf) >> + return -ENOMEM; > I'm not clear where tmpbuf gets freed or passed out of this function? Agreed!, will fix it. >> + >> + if (op->addr.nbytes) { >> + for (i = 0; i < op->addr.nbytes; i++) >> + tmpbuf[i] = op->addr.val >> (8 * (op->addr.nbytes - i - 1)); >> + >> + addr_buf = tmpbuf; > We assign tmpbuf to addr_buf here but addr_buf just gets read from so > it's not via that AFAICT. Agreed, will fix it. >> + } >> + /* Invalid address return zero. */ > Missing blank line. Noted. >> +static void cqspi_chipselect(struct cqspi_flash_pdata *f_pdata) >> +{ >> + struct cqspi_st *cqspi = f_pdata->cqspi; >> + void __iomem *reg_base = cqspi->iobase; >> + unsigned int chip_select = f_pdata->cs; >> + unsigned int reg; >> + >> + reg = readl(reg_base + CQSPI_REG_CONFIG); >> + reg &= ~CQSPI_REG_CONFIG_DECODE_MASK; >> + >> + /* Convert CS if without decoder. >> + * CS0 to 4b'1110 >> + * CS1 to 4b'1101 >> + * CS2 to 4b'1011 >> + * CS3 to 4b'0111 >> + */ >> + chip_select = 0xF & ~(1 << chip_select); > This says "if without decoder" but there's no conditionals here, what if > we do have a decoder? Good catch, will add the check in the next patch, the below check to be added. if (cqspi->is_decoded_cs) { reg |= CQSPI_REG_CONFIG_DECODE_MASK; } else { reg &= ~CQSPI_REG_CONFIG_DECODE_MASK; >> + cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk); >> + ddata = of_device_get_match_data(dev); >> + if (ddata) { >> + if (ddata->quirks & CQSPI_NEEDS_WR_DELAY) >> + cqspi->wr_delay = 5 * DIV_ROUND_UP(NSEC_PER_SEC, >> + cqspi->master_ref_clk_hz); >> + if (ddata->hwcaps_mask & CQSPI_SUPPORTS_OCTAL) >> + master->mode_bits |= SPI_RX_OCTAL; >> + if (!(ddata->quirks & CQSPI_DISABLE_DAC_MODE)) >> + cqspi->use_dac_mode = true; >> + if (ddata->quirks & CQSPI_NEEDS_ADDR_SWAP) { >> + master->bus_num = 0; >> + master->num_chipselect = 2; >> + } >> + } > Given that the driver appears to unconditionally dereference match data > in other places I'd expect this to return an error if there's none, > otherwise we'll oops in those other code paths later on. Noted, will double check is there any impact if there is no data provided. also will add the error check if return an error. >> + ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0, >> + pdev->name, cqspi); >> + if (ret) { >> + dev_err(dev, "Cannot request IRQ.\n"); >> + goto probe_reset_failed; >> + } > Are you sure that it's safe to use devm_request_irq() Yes, I'm sure that it's safe to use devm_request_irq(). > - what happens if > the interrupt fires in the process of removing the device? This is not external interrupt which is coming from the device, its inbuilt to QSPI controller which has 2 Registers 1) INT_STATUS_REG 2) INT_MASK_REG. It fires an interrupt and updating INT flag bit in the interrupt status register once reached read/write completion state then irq_handler is called, there we are clearing the INT_FLAG bit by masking of RD/WR flag in INT_MASK register. Regards Vadivel
Hi Mark, On 14/2/2020 8:11 PM, Mark Brown wrote: > On Fri, Feb 14, 2020 at 01:02:22PM +0100, Simon Goldschmidt wrote: >> On Fri, Feb 14, 2020 at 12:46 PM Ramuthevar,Vadivel MuruganX >> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote: >>> Add support for the Cadence QSPI controller. This controller is >>> present in the Intel Lightning Mountain(LGM) SoCs, Altera and TI SoCs. >>> This driver has been tested on the Intel LGM SoCs. >> This is v9 and still, none of the altera maintainers are on CC? >> How will it be ensured that this doesn't break altera if it is merged? > Given that this is a new driver I'd be very surprised if it broke other > users? I can imagine it might not work for them and it would definitely > be much better to get their review but it shouldn't be any worse than > the current lack of support. Thanks for the clarification, Please kindly see the below discussion b/w Vignesh and Dinh in the earlier mail chain. [Vignesh] The legacy driver under drivers/mtd/spi-nor will be removed as we cannot support both SPI NOR and SPI NAND with single driver if its under spi-nor. New driver should be functionally equivalent to existing one. So I suggest you test this driver on legcay SoCFPGA products. [Dinh] I don't have the original patch series, but will monitor going forward. As long as the new driver does not break legacy SoCFPGA products that use the cadence-quadspi driver then it should be ok. Regards vadivel
Hi Simon, On 14/2/2020 8:02 PM, Simon Goldschmidt wrote: > On Fri, Feb 14, 2020 at 12:46 PM Ramuthevar,Vadivel MuruganX > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote: >> Add support for the Cadence QSPI controller. This controller is >> present in the Intel Lightning Mountain(LGM) SoCs, Altera and TI SoCs. >> This driver has been tested on the Intel LGM SoCs. > This is v9 and still, none of the altera maintainers are on CC? > How will it be ensured that this doesn't break altera if it is merged? Thanks for reminded me, sorry, next send time will add them in CC. Regards Vadivel > > Regards, > Simon >
On Mon, Feb 17, 2020 at 05:28:38PM +0800, Ramuthevar, Vadivel MuruganX wrote: > On 14/2/2020 8:11 PM, Mark Brown wrote: > > Given that this is a new driver I'd be very surprised if it broke other > > users? I can imagine it might not work for them and it would definitely > > be much better to get their review but it shouldn't be any worse than > > the current lack of support. > *[Vignesh]* The legacy driver under drivers/mtd/spi-nor will be removed as > we cannot > support both SPI NOR and SPI NAND with single driver if its under > spi-nor. New driver should be functionally equivalent to existing one. > So I suggest you test this driver on legcay SoCFPGA products. You're not actually removing the driver here, you're adding another driver for the same thing.
Hi Vadivel, On 17/02/20 5:22 pm, Mark Brown wrote: > On Mon, Feb 17, 2020 at 05:28:38PM +0800, Ramuthevar, Vadivel MuruganX wrote: >> On 14/2/2020 8:11 PM, Mark Brown wrote: > >>> Given that this is a new driver I'd be very surprised if it broke other >>> users? I can imagine it might not work for them and it would definitely >>> be much better to get their review but it shouldn't be any worse than >>> the current lack of support. > >> *[Vignesh]* The legacy driver under drivers/mtd/spi-nor will be removed as >> we cannot >> support both SPI NOR and SPI NAND with single driver if its under >> spi-nor. New driver should be functionally equivalent to existing one. >> So I suggest you test this driver on legcay SoCFPGA products. > > You're not actually removing the driver here, you're adding another > driver for the same thing. > I agree with Mark here. I realized that you are using same CONFIG option as the old one to build this driver. This causes new driver to fail to probe as old driver would bind to the node instead (both drivers will be built into the kernel and both drivers have same compatible). So, you should remove the old driver. Could you also include patches removing old driver? New driver and bindings are anyways backward compatible with existing one
On Mon, Feb 17, 2020 at 05:18:10PM +0800, Ramuthevar, Vadivel MuruganX wrote: > On 14/2/2020 9:09 PM, Mark Brown wrote: > > This will unconditionally handle the interrupt regardless of if the > > hardware was actually flagging an interrupt which will break shared > > interrupts and the fault handling code in genirq. > Yes, you're correct, it doesn't check unconditionally, will update the > INT flag in the INT_STATUS register after successful completion of > read/write operation. > but in this case it is dedicated to qspi-interrupt,not shared with any other > HW/SW interrupts. Currently, on the system you're looking at. Given that this is already a widely reused IP there's no guarantee that this will always be the case, and like I say even without sharing it also defeats the fault handling code.
Hi Mark, On 18/2/2020 1:09 AM, Mark Brown wrote: > On Mon, Feb 17, 2020 at 05:18:10PM +0800, Ramuthevar, Vadivel MuruganX wrote: >> On 14/2/2020 9:09 PM, Mark Brown wrote: >>> This will unconditionally handle the interrupt regardless of if the >>> hardware was actually flagging an interrupt which will break shared >>> interrupts and the fault handling code in genirq. >> Yes, you're correct, it doesn't check unconditionally, will update the >> INT flag in the INT_STATUS register after successful completion of >> read/write operation. >> but in this case it is dedicated to qspi-interrupt,not shared with any other >> HW/SW interrupts. > Currently, on the system you're looking at. Given that this is already > a widely reused IP there's no guarantee that this will always be the > case, and like I say even without sharing it also defeats the fault > handling code. Got it, Thanks! a lot will take care of it, even though it is not present in the system current, to avoid future conflicts. Flagging and check to be added to avoid if the interrupt raises from shared devices. Regards Vadivel
Hi Vignesh, On 17/2/2020 8:18 PM, Vignesh Raghavendra wrote: > Hi Vadivel, > > On 17/02/20 5:22 pm, Mark Brown wrote: >> On Mon, Feb 17, 2020 at 05:28:38PM +0800, Ramuthevar, Vadivel MuruganX wrote: >>> On 14/2/2020 8:11 PM, Mark Brown wrote: >>>> Given that this is a new driver I'd be very surprised if it broke other >>>> users? I can imagine it might not work for them and it would definitely >>>> be much better to get their review but it shouldn't be any worse than >>>> the current lack of support. >>> *[Vignesh]* The legacy driver under drivers/mtd/spi-nor will be removed as >>> we cannot >>> support both SPI NOR and SPI NAND with single driver if its under >>> spi-nor. New driver should be functionally equivalent to existing one. >>> So I suggest you test this driver on legcay SoCFPGA products. >> You're not actually removing the driver here, you're adding another >> driver for the same thing. >> > I agree with Mark here. > > I realized that you are using same CONFIG option as the old one to build > this driver. This causes new driver to fail to probe as old driver would > bind to the node instead (both drivers will be built into the kernel and > both drivers have same compatible). > > So, you should remove the old driver. Could you also include patches > removing old driver? New driver and bindings are anyways backward > compatible with existing one Sure , will remove the existing driver and sending single patch, Thanks! Regards Vadivel
Add support for the Cadence QSPI controller. This controller is present in the Intel Lightning Mountain(LGM) SoCs, Altera and TI SoCs. This driver has been tested on the Intel LGM SoCs. This driver does not support generic SPI and also the implementation only supports spi-mem interface to replace the existing driver in mtd/spi-nor/cadence-quadspi.c, the existing driver only support SPI-NOR flash memory. Thanks a lot!!! Vignesh for the review, suggestion to optimize the patch. Tested with mx25u12835f on LGM platform. changes from v8: -- remove the depends MTD macro -- comment into C++ style -- remove spaces and tabs where not applicable. -- align the macro string as same as existing one. -- replace QUAD to op->data.buswidth variable. -- add CQSPI_NEEDS_ADDR_SWAP instead of soc_selection variable changes from v7: -- remove addr_buf kept like as original -- drop bus-num, chipselect variable -- add soc_selection varible to differetiate the features -- replace dev->ddev in dma function -- add seperate function to handle the 24bit slave device address translation for lgm soc -- correct sentence seems incomplete in Kconfig -- add cqspi->soc_selection check to keep the original TI platform working code. changes from v6: -- Add the Signed-off-by Vignesh in commit message -- bus_num, num_chipselect added to avoid the garbage bus number during the probe and spi_register. -- master mode bits updated -- address sequence is different from TI and Intel SoC Ip handling so modified as per Intel and differentiating by use_dac_mode variable. -- dummy cycles also different b/w two platforms, so keeping separate check -- checkpatch errors which are intentional left as is for better readability changes from v5: -- kbuild test robot warnings fixed -- Add Reported-By: Dan Carpenter <dan.carpenter@oracle.com> changes from v4: -- kbuild test robot warnings fixed -- Add Reborted-by: tag changes from v3: spi-cadence-quadspi.c -- static to all functions wrt to local to the file. -- Prefix cqspi_ and make the function static -- cmd_ops, data_ops and dummy_ops dropped -- addr_ops kept since it is required for address calculation. -- devm_ used for supported functions , removed legacy API's -- removed "indirect" name from functions -- replaced by master->mode_bits = SPI_RX_QUAD | SPI_TX_DUAL | SPI_RX_DUAL | SPI_RX_OCTAL; as per Vignesh susggestion -- removed free functions since devm_ handles automatically. -- dropped all unused Macros YAML file update: -- cadence,qspi.yaml file name replace by cdns,qspi-nor.yaml -- compatible string updated as per Vignesh suggestion -- for single entry, removed descriptions -- removed optional parameters Build Result: linux$ make DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml dt_binding_check CHKDT Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml SCHEMA Documentation/devicetree/bindings/processed-schema.yaml DTC Documentation/devicetree/bindings/spi/cdns,qspi-nor.example.dt.yaml CHECK Documentation/devicetree/bindings/spi/cdns,qspi-nor.example.dt.yaml Ramuthevar Vadivel Murugan (2): dt-bindings: spi: Add schema for Cadence QSPI Controller driver spi: cadence-quadpsi: Add support for the Cadence QSPI controller .../devicetree/bindings/spi/cdns,qspi-nor.yaml | 147 ++ drivers/spi/Kconfig | 8 + drivers/spi/Makefile | 1 + drivers/spi/spi-cadence-quadspi.c | 1508 ++++++++++++++++++++ 4 files changed, 1664 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml create mode 100644 drivers/spi/spi-cadence-quadspi.c