| Message ID | 20200214114618.29704-1-vadivel.muruganx.ramuthevar@linux.intel.com (mailing list archive) |
|---|---|
| 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 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, 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
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