| Message ID | 20190916073843.39618-1-vadivel.muruganx.ramuthevar@linux.intel.com |
|---|---|
| Headers | show |
| Series | spi: cadence-qspi: Add cadence-qspi support for Intel LGM SoC | expand |
On Mon, Sep 16, 2019 at 03:38:43PM +0800, Ramuthevar,Vadivel MuruganX wrote: > Existing cadence drivers do not support SPI-NAND, it only supports to > SPI-NOR and SPI devices. To state that is the driver for the same IP > but due to different SPI flash memory(NAND) need to write from scratch. What makes you say you need to write a separate driver? It's perfectly possible to support the normal and flash I/O mechanisms in a single driver and as well as the maintainence issues obviously someone could build a system with both flash and non-flash devices on the same SPI controller.
Hi, On 16/09/19 1:08 PM, Ramuthevar,Vadivel MuruganX wrote: > patch 1: Add YAML for cadence-qspi devicetree cdocumentation. > patch 2: cadence-qspi controller driver to support QSPI-NAND flash > using existing spi-nand framework with legacy spi protocol. Nope, you cannot have two drivers for the same IP (i.e Cadence QSPI) just to support to different types of SPI memories. This is the reason why spi_mem_ops was introduced. Please rewrite this driver over to use spi_mem_ops (instead of using generic SPI xfers) so that same driver supports both SPI-NOR and SPI-NAND flashes. Once that's done drivers/mtd/spi-nor/cadence-quadspi.c can be deleted. There are few existing examples of spi_mem_ops users in drivers/spi/ (git grep spi_mem_ops) and materials here on how to write such a driver: [1] https://bootlin.com/blog/spi-mem-bringing-some-consistency-to-the-spi-memory-ecosystem/ [2] https://www.youtube.com/watch?v=PkWbuLM_gmU > > Ramuthevar Vadivel Murugan (2): > dt-bindings: spi: Add support for cadence-qspi IP Intel LGM SoC > spi: cadence-qspi: Add QSPI support for Intel LGM SoC > > .../devicetree/bindings/spi/cadence,qspi-nand.yaml | 84 +++ > drivers/spi/Kconfig | 9 + > drivers/spi/Makefile | 1 + > drivers/spi/spi-cadence-qspi-apb.c | 644 +++++++++++++++++++++ > drivers/spi/spi-cadence-qspi-apb.h | 174 ++++++ > drivers/spi/spi-cadence-qspi.c | 461 +++++++++++++++ > drivers/spi/spi-cadence-qspi.h | 73 +++ > 7 files changed, 1446 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/cadence,qspi-nand.yaml > create mode 100644 drivers/spi/spi-cadence-qspi-apb.c > create mode 100644 drivers/spi/spi-cadence-qspi-apb.h > create mode 100644 drivers/spi/spi-cadence-qspi.c > create mode 100644 drivers/spi/spi-cadence-qspi.h >
Hi Vignesh, Thank you for the review comments and suggestions. On 17/9/2019 12:50 AM, Vignesh Raghavendra wrote: > Hi, > > On 16/09/19 1:08 PM, Ramuthevar,Vadivel MuruganX wrote: >> patch 1: Add YAML for cadence-qspi devicetree cdocumentation. >> patch 2: cadence-qspi controller driver to support QSPI-NAND flash >> using existing spi-nand framework with legacy spi protocol. > Nope, you cannot have two drivers for the same IP (i.e Cadence QSPI) > just to support to different types of SPI memories. This is the reason > why spi_mem_ops was introduced. > > Please rewrite this driver over to use spi_mem_ops (instead of using > generic SPI xfers) so that same driver supports both SPI-NOR and > SPI-NAND flashes. Once that's done drivers/mtd/spi-nor/cadence-quadspi.c > can be deleted. > > There are few existing examples of spi_mem_ops users in drivers/spi/ > (git grep spi_mem_ops) and materials here on how to write such a driver: > > [1] > https://bootlin.com/blog/spi-mem-bringing-some-consistency-to-the-spi-memory-ecosystem/ > [2] https://www.youtube.com/watch?v=PkWbuLM_gmU Agreed!, Surely let me go through the above link and put my effort to rewrite the drivers as per your suggestions. --- With Best Regards Vadivel Murugan R >> Ramuthevar Vadivel Murugan (2): >> dt-bindings: spi: Add support for cadence-qspi IP Intel LGM SoC >> spi: cadence-qspi: Add QSPI support for Intel LGM SoC >> >> .../devicetree/bindings/spi/cadence,qspi-nand.yaml | 84 +++ >> drivers/spi/Kconfig | 9 + >> drivers/spi/Makefile | 1 + >> drivers/spi/spi-cadence-qspi-apb.c | 644 +++++++++++++++++++++ >> drivers/spi/spi-cadence-qspi-apb.h | 174 ++++++ >> drivers/spi/spi-cadence-qspi.c | 461 +++++++++++++++ >> drivers/spi/spi-cadence-qspi.h | 73 +++ >> 7 files changed, 1446 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/spi/cadence,qspi-nand.yaml >> create mode 100644 drivers/spi/spi-cadence-qspi-apb.c >> create mode 100644 drivers/spi/spi-cadence-qspi-apb.h >> create mode 100644 drivers/spi/spi-cadence-qspi.c >> create mode 100644 drivers/spi/spi-cadence-qspi.h >>
On Tue, Sep 17, 2019 at 10:11:28AM +0800, Ramuthevar, Vadivel MuruganX wrote: > * spi-cadence.c* in *drivers/spi/*, which supports very old legacy > cadence-spi based devices(normal) > * cadence-quadspi.c(drivers/mtd/spi-nor/)* : specific support to SPI-NOR > flash with new spi-nor layer. > all the API's in this driver purely on spi-nor specific, so couldn't > proceed to adapt. Are these completely separate IPs or are they just different versions of the same IP?
Hi Mark, Thank you for the review comments. On 17/9/2019 11:36 PM, Mark Brown wrote: > On Tue, Sep 17, 2019 at 10:11:28AM +0800, Ramuthevar, Vadivel MuruganX wrote: > >> * spi-cadence.c* in *drivers/spi/*, which supports very old legacy >> cadence-spi based devices(normal) >> * cadence-quadspi.c(drivers/mtd/spi-nor/)* : specific support to SPI-NOR >> flash with new spi-nor layer. >> all the API's in this driver purely on spi-nor specific, so couldn't >> proceed to adapt. > Are these completely separate IPs or are they just different versions of > the same IP? These are same IPs , but different features Enabled/Disabled depends upon the SoC vendors. for e.g: Intel LGM SoC uses the same IP, but without DMA and Direct access controller. also dedicated support to flash devices. Best regards Vadivel
On Wed, Sep 18, 2019 at 01:59:06PM +0800, Ramuthevar, Vadivel MuruganX wrote: > On 17/9/2019 11:36 PM, Mark Brown wrote: > > On Tue, Sep 17, 2019 at 10:11:28AM +0800, Ramuthevar, Vadivel MuruganX wrote: > > > * spi-cadence.c* in *drivers/spi/*, which supports very old legacy > > > cadence-spi based devices(normal) > > > * cadence-quadspi.c(drivers/mtd/spi-nor/)* : specific support to SPI-NOR > > > flash with new spi-nor layer. > > > all the API's in this driver purely on spi-nor specific, so couldn't > > > proceed to adapt. > > Are these completely separate IPs or are they just different versions of > > the same IP? > These are same IPs , but different features Enabled/Disabled depends upon > the SoC vendors. > for e.g: Intel LGM SoC uses the same IP, but without DMA and Direct access > controller. > also dedicated support to flash devices. If it's different versions of the same IP then everything should be in one driver with the optional features enabled depending on what's in a given system.
Hi Mark, Thank you for the comments and queries. On 18/9/2019 8:08 PM, Mark Brown wrote: > On Wed, Sep 18, 2019 at 01:59:06PM +0800, Ramuthevar, Vadivel MuruganX wrote: >> On 17/9/2019 11:36 PM, Mark Brown wrote: >>> On Tue, Sep 17, 2019 at 10:11:28AM +0800, Ramuthevar, Vadivel MuruganX wrote: >>>> * spi-cadence.c* in *drivers/spi/*, which supports very old legacy >>>> cadence-spi based devices(normal) >>>> * cadence-quadspi.c(drivers/mtd/spi-nor/)* : specific support to SPI-NOR >>>> flash with new spi-nor layer. >>>> all the API's in this driver purely on spi-nor specific, so couldn't >>>> proceed to adapt. >>> Are these completely separate IPs or are they just different versions of >>> the same IP? >> These are same IPs , but different features Enabled/Disabled depends upon >> the SoC vendors. >> for e.g: Intel LGM SoC uses the same IP, but without DMA and Direct access >> controller. >> also dedicated support to flash devices. > If it's different versions of the same IP then everything should be in > one driver with the optional features enabled depending on what's in a > given system. Agreed!, I am trying to adapt the driver/mtd/spi-nor/cadence-quadspi.c and newly sent patches in a single driver file, also trying to use spi_mem_ops framework. With Best Regards Vadivel
HI Vignesh, On 17/9/2019 12:50 AM, Vignesh Raghavendra wrote: > Hi, > > On 16/09/19 1:08 PM, Ramuthevar,Vadivel MuruganX wrote: >> patch 1: Add YAML for cadence-qspi devicetree cdocumentation. >> patch 2: cadence-qspi controller driver to support QSPI-NAND flash >> using existing spi-nand framework with legacy spi protocol. > Nope, you cannot have two drivers for the same IP (i.e Cadence QSPI) > just to support to different types of SPI memories. This is the reason > why spi_mem_ops was introduced. > > Please rewrite this driver over to use spi_mem_ops (instead of using > generic SPI xfers) so that same driver supports both SPI-NOR and > SPI-NAND flashes. Once that's done drivers/mtd/spi-nor/cadence-quadspi.c > can be deleted. > > There are few existing examples of spi_mem_ops users in drivers/spi/ > (git grep spi_mem_ops) and materials here on how to write such a driver: > > [1] > https://bootlin.com/blog/spi-mem-bringing-some-consistency-to-the-spi-memory-ecosystem/ > [2] https://www.youtube.com/watch?v=PkWbuLM_gmU As per Mark Brown and your suggestion, I have started adapting cadence-qaudspi driver with spi_mem_ops framework to work QSPI-NAND/NOR as a generic driver(completely removed the legacy SPI-XFERS), is in progress on Intel LGM SoC. QSPI-IP on Intel LGM do not have DMA support and also not part of QSPI IP, so couldn't able to validate DMA related. will adapt the DMA things which are existing in cadence-quadspi.c as it is. currently TI and Altera SoC's use this Cadence-qspi IP , both are not using DMA as per my understanding (correct me if it is wrong). confirmed through device tree entry. what is your opinion on DMA related stuff? also using macronix(QSPI-NOR) flash/Micron(QSPI-NAND). --- With Regards Vadivel >> Ramuthevar Vadivel Murugan (2): >> dt-bindings: spi: Add support for cadence-qspi IP Intel LGM SoC >> spi: cadence-qspi: Add QSPI support for Intel LGM SoC >> >> .../devicetree/bindings/spi/cadence,qspi-nand.yaml | 84 +++ >> drivers/spi/Kconfig | 9 + >> drivers/spi/Makefile | 1 + >> drivers/spi/spi-cadence-qspi-apb.c | 644 +++++++++++++++++++++ >> drivers/spi/spi-cadence-qspi-apb.h | 174 ++++++ >> drivers/spi/spi-cadence-qspi.c | 461 +++++++++++++++ >> drivers/spi/spi-cadence-qspi.h | 73 +++ >> 7 files changed, 1446 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/spi/cadence,qspi-nand.yaml >> create mode 100644 drivers/spi/spi-cadence-qspi-apb.c >> create mode 100644 drivers/spi/spi-cadence-qspi-apb.h >> create mode 100644 drivers/spi/spi-cadence-qspi.c >> create mode 100644 drivers/spi/spi-cadence-qspi.h >>
On 10/10/19 7:04 AM, Ramuthevar, Vadivel MuruganX wrote: > HI Vignesh, > > On 17/9/2019 12:50 AM, Vignesh Raghavendra wrote: >> Hi, >> >> On 16/09/19 1:08 PM, Ramuthevar,Vadivel MuruganX wrote: >>> patch 1: Add YAML for cadence-qspi devicetree cdocumentation. >>> patch 2: cadence-qspi controller driver to support QSPI-NAND flash >>> using existing spi-nand framework with legacy spi protocol. >> Nope, you cannot have two drivers for the same IP (i.e Cadence QSPI) >> just to support to different types of SPI memories. This is the reason >> why spi_mem_ops was introduced. >> >> Please rewrite this driver over to use spi_mem_ops (instead of using >> generic SPI xfers) so that same driver supports both SPI-NOR and >> SPI-NAND flashes. Once that's done drivers/mtd/spi-nor/cadence-quadspi.c >> can be deleted. >> >> There are few existing examples of spi_mem_ops users in drivers/spi/ >> (git grep spi_mem_ops) and materials here on how to write such a driver: >> >> [1] >> https://bootlin.com/blog/spi-mem-bringing-some-consistency-to-the-spi-memory-ecosystem/ >> >> [2] https://www.youtube.com/watch?v=PkWbuLM_gmU > As per Mark Brown and your suggestion, I have started adapting > cadence-qaudspi driver with spi_mem_ops framework to work > QSPI-NAND/NOR as a generic driver(completely removed the legacy > SPI-XFERS), is in progress on Intel LGM SoC. > QSPI-IP on Intel LGM do not have DMA support and also not part of QSPI > IP, so couldn't able to validate DMA related. > will adapt the DMA things which are existing in cadence-quadspi.c as it is. > Great, appreciate the effort! > currently TI and Altera SoC's use this Cadence-qspi IP , both are not > using DMA as per my understanding (correct me if it is wrong). > confirmed through device tree entry. > TI platforms use DMA to read data from flash in memory mapped mode (direct access controller) using mem-to-mem DMA channels. Mem-to-mem DMA channels are requested as and when needed and are not part of DT description (as they are not bound to a device) > what is your opinion on DMA related stuff? Not having DMA support would be a regression. Please keep the DAC + DMA part as is. I can help you will all the DMA related testing... Regards Vignesh > also using macronix(QSPI-NOR) > flash/Micron(QSPI-NAND). > --- > With Regards > Vadivel >>> Ramuthevar Vadivel Murugan (2): >>> dt-bindings: spi: Add support for cadence-qspi IP Intel LGM SoC >>> spi: cadence-qspi: Add QSPI support for Intel LGM SoC >>> >>> .../devicetree/bindings/spi/cadence,qspi-nand.yaml | 84 +++ >>> drivers/spi/Kconfig | 9 + >>> drivers/spi/Makefile | 1 + >>> drivers/spi/spi-cadence-qspi-apb.c | 644 >>> +++++++++++++++++++++ >>> drivers/spi/spi-cadence-qspi-apb.h | 174 ++++++ >>> drivers/spi/spi-cadence-qspi.c | 461 >>> +++++++++++++++ >>> drivers/spi/spi-cadence-qspi.h | 73 +++ >>> 7 files changed, 1446 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/spi/cadence,qspi-nand.yaml >>> create mode 100644 drivers/spi/spi-cadence-qspi-apb.c >>> create mode 100644 drivers/spi/spi-cadence-qspi-apb.h >>> create mode 100644 drivers/spi/spi-cadence-qspi.c >>> create mode 100644 drivers/spi/spi-cadence-qspi.h >>>
Hi Vignesh, On 10/10/2019 12:18 PM, Vignesh Raghavendra wrote: > > On 10/10/19 7:04 AM, Ramuthevar, Vadivel MuruganX wrote: >> HI Vignesh, >> >> On 17/9/2019 12:50 AM, Vignesh Raghavendra wrote: >>> Hi, >>> >>> On 16/09/19 1:08 PM, Ramuthevar,Vadivel MuruganX wrote: >>>> patch 1: Add YAML for cadence-qspi devicetree cdocumentation. >>>> patch 2: cadence-qspi controller driver to support QSPI-NAND flash >>>> using existing spi-nand framework with legacy spi protocol. >>> Nope, you cannot have two drivers for the same IP (i.e Cadence QSPI) >>> just to support to different types of SPI memories. This is the reason >>> why spi_mem_ops was introduced. >>> >>> Please rewrite this driver over to use spi_mem_ops (instead of using >>> generic SPI xfers) so that same driver supports both SPI-NOR and >>> SPI-NAND flashes. Once that's done drivers/mtd/spi-nor/cadence-quadspi.c >>> can be deleted. >>> >>> There are few existing examples of spi_mem_ops users in drivers/spi/ >>> (git grep spi_mem_ops) and materials here on how to write such a driver: >>> >>> [1] >>> https://bootlin.com/blog/spi-mem-bringing-some-consistency-to-the-spi-memory-ecosystem/ >>> >>> [2] https://www.youtube.com/watch?v=PkWbuLM_gmU >> As per Mark Brown and your suggestion, I have started adapting >> cadence-qaudspi driver with spi_mem_ops framework to work >> QSPI-NAND/NOR as a generic driver(completely removed the legacy >> SPI-XFERS), is in progress on Intel LGM SoC. >> QSPI-IP on Intel LGM do not have DMA support and also not part of QSPI >> IP, so couldn't able to validate DMA related. >> will adapt the DMA things which are existing in cadence-quadspi.c as it is. >> > Great, appreciate the effort! > >> currently TI and Altera SoC's use this Cadence-qspi IP , both are not >> using DMA as per my understanding (correct me if it is wrong). >> confirmed through device tree entry. >> > TI platforms use DMA to read data from flash in memory mapped mode > (direct access controller) using mem-to-mem DMA channels. Mem-to-mem DMA > channels are requested as and when needed and are not part of DT > description (as they are not bound to a device) yes, understood now, Thanks! >> what is your opinion on DMA related stuff? > Not having DMA support would be a regression. Please keep the DAC + DMA > part as is. I can help you will all the DMA related testing... Sure, will keep DAC + DMA, as we discussed earlier use QUIRKS to differentiate and follow the same. --- With Regards Vadivel > Regards > Vignesh > >> also using macronix(QSPI-NOR) >> flash/Micron(QSPI-NAND). >> --- >> With Regards >> Vadivel >>>> Ramuthevar Vadivel Murugan (2): >>>> dt-bindings: spi: Add support for cadence-qspi IP Intel LGM SoC >>>> spi: cadence-qspi: Add QSPI support for Intel LGM SoC >>>> >>>> .../devicetree/bindings/spi/cadence,qspi-nand.yaml | 84 +++ >>>> drivers/spi/Kconfig | 9 + >>>> drivers/spi/Makefile | 1 + >>>> drivers/spi/spi-cadence-qspi-apb.c | 644 >>>> +++++++++++++++++++++ >>>> drivers/spi/spi-cadence-qspi-apb.h | 174 ++++++ >>>> drivers/spi/spi-cadence-qspi.c | 461 >>>> +++++++++++++++ >>>> drivers/spi/spi-cadence-qspi.h | 73 +++ >>>> 7 files changed, 1446 insertions(+) >>>> create mode 100644 >>>> Documentation/devicetree/bindings/spi/cadence,qspi-nand.yaml >>>> create mode 100644 drivers/spi/spi-cadence-qspi-apb.c >>>> create mode 100644 drivers/spi/spi-cadence-qspi-apb.h >>>> create mode 100644 drivers/spi/spi-cadence-qspi.c >>>> create mode 100644 drivers/spi/spi-cadence-qspi.h >>>>