| Message ID | 20240215134856.1313239-1-quic_mdalam@quicinc.com |
|---|---|
| Headers | show |
| Series | Add QPIC SPI NAND driver | expand |
On Thu, Feb 15, 2024 at 07:18:52PM +0530, Md Sadre Alam wrote: > + clocks: > + minItems: 2 > + maxItems: 3 > + > + clock-names: > + minItems: 2 > + maxItems: 3 The driver requests the clocks by name but this does not document the expected set of names. The driver also unconditionally requests all three clocks so won't work with only two clocks.
On 15.02.2024 16:00, Dmitry Baryshkov wrote: > On Thu, 15 Feb 2024 at 15:49, Md Sadre Alam <quic_mdalam@quicinc.com> wrote: >> >> Disable eMMC node for rdp433, since rdp433 default boot mode >> is norplusnand. > > Are they exclusive? Even if they're not, having access to the eMMC/sdcard would still be nice.. Konrad
On 15/02/2024 14:48, Md Sadre Alam wrote: > Add device-tree binding documentation for QCOM QPIC-SNAND-NAND Flash > Interface. > A nit, subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Co-developed-by: Varadarajan Narayanan <quic_varada@quicinc.com> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> > --- > .../bindings/spi/qcom,spi-qpic-snand.yaml | 82 +++++++++++++++++++ > 1 file changed, 82 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml > > diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml > new file mode 100644 > index 000000000000..fa7484ce1319 > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml Filename like compatible. > @@ -0,0 +1,82 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/spi/qcom,spi-qpic-snand.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm QPIC NAND controller > + > +maintainers: > + - Md sadre Alam <quic_mdalam@quicinc.com> > + Provide description which will describe hardware. > +properties: > + compatible: > + enum: > + - qcom,ipq9574-snand > + > + reg: > + maxItems: 1 > + > + clocks: > + minItems: 2 > + maxItems: 3 You must document the items (could be sufficient in clock-names if the names are obvious). Why the clocks are flexible? This given IPQ9574 has variable clock inputs? Please explain. > + > + clock-names: > + minItems: 2 > + maxItems: 3 > + required goes here. > +allOf: > + - $ref: /schemas/spi/spi-controller.yaml# > + - if: > + properties: > + compatible: > + contains: > + enum: > + - qcom,ipq9574-snand > + > + then: > + properties: > + dmas: > + items: > + - description: tx DMA channel > + - description: rx DMA channel > + - description: cmd DMA channel > + > + dma-names: > + items: > + - const: tx > + - const: rx > + - const: cmd No clue why it is here, move it to top level. > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/qcom,ipq9574-gcc.h> > + qpic_nand: spi@79b0000 { Drop unused label > + compatible = "qcom,ipq9574-snand"; > + reg = <0x1ac00000 0x800>; > + > + clocks = <&gcc GCC_QPIC_CLK>, > + <&gcc GCC_QPIC_AHB_CLK>, > + <&gcc GCC_QPIC_IO_MACRO_CLK>; > + clock-names = "core", "aon", "iom"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + flash@0 { > + compatible = "spi-nand"; > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <1>; > + nand-ecc-engine = <&qpic_nand>; > + nand-ecc-strength = <4>; > + nand-ecc-step-size = <512>; > + }; Fix indentation. > + }; Best regards, Krzysztof
On 2/15/2024 7:18 PM, Md Sadre Alam wrote: > Disable eMMC node for rdp433, since rdp433 default boot mode > is norplusnand. > > Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Co-developed-by: Varadarajan Narayanan <quic_varada@quicinc.com> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> Single line of change is developed by 3 authors? > --- > arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts > index 1bb8d96c9a82..e33e7fafd695 100644 > --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts > +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts > @@ -24,7 +24,7 @@ &sdhc_1 { > mmc-hs400-enhanced-strobe; > max-frequency = <384000000>; > bus-width = <8>; > - status = "okay"; > + status = "disabled"; > }; > > &tlmm {
On Thu, Feb 15, 2024 at 07:18:51PM +0530, Md Sadre Alam wrote: > This series of patches will add initial supports > for QPIC SPI NAND driver. > > Currently this driver support following commands > > -- RESET > -- READ ID > -- BLOCK ERASE > -- PAGE READ > -- PAGE WRITE > -- GET FEATURE > -- SET FEATURE > -- BAD BLOCK CHECK > > This driver has been tested with dd command with read/write page > with multiple file size 1MiB, 10MiB,40MiB etc. > Also tested with "mtd" command like mtd erase, mtd write, mtd verify etc. > This is not the first version isn't it? Where is the changelog describing what has changed since then? - Mani > Need help to test these all patches on SDX65 and SDX75 platform. > > Md Sadre Alam (5): > spi: dt-bindings: add binding doc for spi-qpic-snand > drivers: mtd: nand: Add qpic_common API file > spi: spi-qpic: Add qpic spi nand driver support > arm64: dts: qcom: ipq9574: Add SPI nand support > arm64: dts: qcom: ipq9574: Disable eMMC node > > .../bindings/spi/qcom,spi-qpic-snand.yaml | 82 ++ > .../boot/dts/qcom/ipq9574-rdp-common.dtsi | 43 + > arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts | 2 +- > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 27 + > drivers/mtd/nand/Makefile | 1 + > drivers/mtd/nand/qpic_common.c | 794 +++++++++++ > drivers/mtd/nand/raw/qcom_nandc.c | 1226 +---------------- > drivers/spi/Kconfig | 9 + > drivers/spi/Makefile | 1 + > drivers/spi/spi-qpic-snand.c | 1025 ++++++++++++++ > include/linux/mtd/nand-qpic-common.h | 548 ++++++++ > 11 files changed, 2547 insertions(+), 1211 deletions(-) > create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml > create mode 100644 drivers/mtd/nand/qpic_common.c > create mode 100644 drivers/spi/spi-qpic-snand.c > create mode 100644 include/linux/mtd/nand-qpic-common.h > > -- > 2.34.1 >
On 2/19/2024 6:34 PM, Manivannan Sadhasivam wrote: > On Thu, Feb 15, 2024 at 07:18:51PM +0530, Md Sadre Alam wrote: >> This series of patches will add initial supports >> for QPIC SPI NAND driver. >> >> Currently this driver support following commands >> >> -- RESET >> -- READ ID >> -- BLOCK ERASE >> -- PAGE READ >> -- PAGE WRITE >> -- GET FEATURE >> -- SET FEATURE >> -- BAD BLOCK CHECK >> >> This driver has been tested with dd command with read/write page >> with multiple file size 1MiB, 10MiB,40MiB etc. >> Also tested with "mtd" command like mtd erase, mtd write, mtd verify etc. >> > > This is not the first version isn't it? Where is the changelog describing what > has changed since then? The earlier patch was the RFC for design review only. > > - Mani > >> Need help to test these all patches on SDX65 and SDX75 platform. >> >> Md Sadre Alam (5): >> spi: dt-bindings: add binding doc for spi-qpic-snand >> drivers: mtd: nand: Add qpic_common API file >> spi: spi-qpic: Add qpic spi nand driver support >> arm64: dts: qcom: ipq9574: Add SPI nand support >> arm64: dts: qcom: ipq9574: Disable eMMC node >> >> .../bindings/spi/qcom,spi-qpic-snand.yaml | 82 ++ >> .../boot/dts/qcom/ipq9574-rdp-common.dtsi | 43 + >> arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts | 2 +- >> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 27 + >> drivers/mtd/nand/Makefile | 1 + >> drivers/mtd/nand/qpic_common.c | 794 +++++++++++ >> drivers/mtd/nand/raw/qcom_nandc.c | 1226 +---------------- >> drivers/spi/Kconfig | 9 + >> drivers/spi/Makefile | 1 + >> drivers/spi/spi-qpic-snand.c | 1025 ++++++++++++++ >> include/linux/mtd/nand-qpic-common.h | 548 ++++++++ >> 11 files changed, 2547 insertions(+), 1211 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml >> create mode 100644 drivers/mtd/nand/qpic_common.c >> create mode 100644 drivers/spi/spi-qpic-snand.c >> create mode 100644 include/linux/mtd/nand-qpic-common.h >> >> -- >> 2.34.1 >> >
On 20/02/2024 12:32, Md Sadre Alam wrote: > > > On 2/19/2024 6:34 PM, Manivannan Sadhasivam wrote: >> On Thu, Feb 15, 2024 at 07:18:51PM +0530, Md Sadre Alam wrote: >>> This series of patches will add initial supports >>> for QPIC SPI NAND driver. >>> >>> Currently this driver support following commands >>> >>> -- RESET >>> -- READ ID >>> -- BLOCK ERASE >>> -- PAGE READ >>> -- PAGE WRITE >>> -- GET FEATURE >>> -- SET FEATURE >>> -- BAD BLOCK CHECK >>> >>> This driver has been tested with dd command with read/write page >>> with multiple file size 1MiB, 10MiB,40MiB etc. >>> Also tested with "mtd" command like mtd erase, mtd write, mtd verify etc. >>> >> >> This is not the first version isn't it? Where is the changelog describing what >> has changed since then? > > The earlier patch was the RFC for design review only. RFC is state of patch, not version. This is v2 then. These RFC postings are really becoming mess. Some people make multiple RFCs and then post v1 hiding entire previous history... And why even bother with calling it RFC? Best regards, Krzysztof
On 2/15/2024 7:52 PM, Mark Brown wrote: > On Thu, Feb 15, 2024 at 07:18:52PM +0530, Md Sadre Alam wrote: > >> + clocks: >> + minItems: 2 >> + maxItems: 3 >> + >> + clock-names: >> + minItems: 2 >> + maxItems: 3 > > The driver requests the clocks by name but this does not document the > expected set of names. The driver also unconditionally requests all > three clocks so won't work with only two clocks. Thanks for reviewing, Will document the clock name in next patch. By mistake i have given minItems = 2 and maxItems = 3 , will fix this in next patch.
On 2/15/2024 11:28 PM, Konrad Dybcio wrote: > On 15.02.2024 16:00, Dmitry Baryshkov wrote: >> On Thu, 15 Feb 2024 at 15:49, Md Sadre Alam <quic_mdalam@quicinc.com> wrote: >>> >>> Disable eMMC node for rdp433, since rdp433 default boot mode >>> is norplusnand. >> >> Are they exclusive? > > Even if they're not, having access to the eMMC/sdcard would still > be nice.. GPIO are shared b/w eMMC and NAND so we can't keep both. > > Konrad
On 2/16/2024 12:32 AM, Krzysztof Kozlowski wrote: > On 15/02/2024 14:48, Md Sadre Alam wrote: >> Add device-tree binding documentation for QCOM QPIC-SNAND-NAND Flash >> Interface. >> > > A nit, subject: drop second/last, redundant "bindings". The > "dt-bindings" prefix is already stating that these are bindings. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 Ok > >> Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> >> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> >> Co-developed-by: Varadarajan Narayanan <quic_varada@quicinc.com> >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >> --- >> .../bindings/spi/qcom,spi-qpic-snand.yaml | 82 +++++++++++++++++++ >> 1 file changed, 82 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml >> >> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml >> new file mode 100644 >> index 000000000000..fa7484ce1319 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml > > Filename like compatible. Ok > >> @@ -0,0 +1,82 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/spi/qcom,spi-qpic-snand.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm QPIC NAND controller >> + >> +maintainers: >> + - Md sadre Alam <quic_mdalam@quicinc.com> >> + > > Provide description which will describe hardware. Ok > >> +properties: >> + compatible: >> + enum: >> + - qcom,ipq9574-snand >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + minItems: 2 >> + maxItems: 3 > > You must document the items (could be sufficient in clock-names if the > names are obvious). Ok > > > Why the clocks are flexible? This given IPQ9574 has variable clock > inputs? Please explain. I have checked Hardware Spec. and clocks are fixed in IPQ9574. Will fix in next patch. > >> + >> + clock-names: >> + minItems: 2 >> + maxItems: 3 >> + > > required goes here. Ok > >> +allOf: >> + - $ref: /schemas/spi/spi-controller.yaml# > > >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - qcom,ipq9574-snand >> + >> + then: >> + properties: >> + dmas: >> + items: >> + - description: tx DMA channel >> + - description: rx DMA channel >> + - description: cmd DMA channel >> + >> + dma-names: >> + items: >> + - const: tx >> + - const: rx >> + - const: cmd > > No clue why it is here, move it to top level. Ok > >> +required: >> + - compatible >> + - reg >> + - clocks >> + - clock-names >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/qcom,ipq9574-gcc.h> >> + qpic_nand: spi@79b0000 { > > Drop unused label Ok > >> + compatible = "qcom,ipq9574-snand"; >> + reg = <0x1ac00000 0x800>; >> + >> + clocks = <&gcc GCC_QPIC_CLK>, >> + <&gcc GCC_QPIC_AHB_CLK>, >> + <&gcc GCC_QPIC_IO_MACRO_CLK>; >> + clock-names = "core", "aon", "iom"; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + flash@0 { >> + compatible = "spi-nand"; >> + reg = <0>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + nand-ecc-engine = <&qpic_nand>; >> + nand-ecc-strength = <4>; >> + nand-ecc-step-size = <512>; >> + }; > > Fix indentation. Ok > >> + }; > > Best regards, > Krzysztof >
On 2/16/2024 9:13 PM, Kathiravan Thirumoorthy wrote: > > > On 2/15/2024 7:18 PM, Md Sadre Alam wrote: >> Disable eMMC node for rdp433, since rdp433 default boot mode >> is norplusnand. >> >> Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> >> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> >> Co-developed-by: Varadarajan Narayanan <quic_varada@quicinc.com> >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> > > > Single line of change is developed by 3 authors? Will fix in next patch. > > >> --- >> arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts >> index 1bb8d96c9a82..e33e7fafd695 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts >> +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts >> @@ -24,7 +24,7 @@ &sdhc_1 { >> mmc-hs400-enhanced-strobe; >> max-frequency = <384000000>; >> bus-width = <8>; >> - status = "okay"; >> + status = "disabled"; >> }; >> &tlmm {
On 2/20/2024 5:06 PM, Krzysztof Kozlowski wrote: > On 20/02/2024 12:32, Md Sadre Alam wrote: >> >> >> On 2/19/2024 6:34 PM, Manivannan Sadhasivam wrote: >>> On Thu, Feb 15, 2024 at 07:18:51PM +0530, Md Sadre Alam wrote: >>>> This series of patches will add initial supports >>>> for QPIC SPI NAND driver. >>>> >>>> Currently this driver support following commands >>>> >>>> -- RESET >>>> -- READ ID >>>> -- BLOCK ERASE >>>> -- PAGE READ >>>> -- PAGE WRITE >>>> -- GET FEATURE >>>> -- SET FEATURE >>>> -- BAD BLOCK CHECK >>>> >>>> This driver has been tested with dd command with read/write page >>>> with multiple file size 1MiB, 10MiB,40MiB etc. >>>> Also tested with "mtd" command like mtd erase, mtd write, mtd verify etc. >>>> >>> >>> This is not the first version isn't it? Where is the changelog describing what >>> has changed since then? >> >> The earlier patch was the RFC for design review only. > > RFC is state of patch, not version. This is v2 then. > > These RFC postings are really becoming mess. Some people make multiple > RFCs and then post v1 hiding entire previous history... And why even > bother with calling it RFC? Sorry, I was not aware of this. Shall I post the next one as V3 and add references to the RFC patch and this patch in the cover letter of V3? > > Best regards, > Krzysztof >
On 21/02/2024 11:34, Md Sadre Alam wrote: > > > On 2/20/2024 5:06 PM, Krzysztof Kozlowski wrote: >> On 20/02/2024 12:32, Md Sadre Alam wrote: >>> >>> >>> On 2/19/2024 6:34 PM, Manivannan Sadhasivam wrote: >>>> On Thu, Feb 15, 2024 at 07:18:51PM +0530, Md Sadre Alam wrote: >>>>> This series of patches will add initial supports >>>>> for QPIC SPI NAND driver. >>>>> >>>>> Currently this driver support following commands >>>>> >>>>> -- RESET >>>>> -- READ ID >>>>> -- BLOCK ERASE >>>>> -- PAGE READ >>>>> -- PAGE WRITE >>>>> -- GET FEATURE >>>>> -- SET FEATURE >>>>> -- BAD BLOCK CHECK >>>>> >>>>> This driver has been tested with dd command with read/write page >>>>> with multiple file size 1MiB, 10MiB,40MiB etc. >>>>> Also tested with "mtd" command like mtd erase, mtd write, mtd verify etc. >>>>> >>>> >>>> This is not the first version isn't it? Where is the changelog describing what >>>> has changed since then? >>> >>> The earlier patch was the RFC for design review only. >> >> RFC is state of patch, not version. This is v2 then. >> >> These RFC postings are really becoming mess. Some people make multiple >> RFCs and then post v1 hiding entire previous history... And why even >> bother with calling it RFC? > > Sorry, I was not aware of this. Shall I post the next one as V3 > and add references to the RFC patch and this patch in the cover > letter of V3? Yes, like with every posting. Best regards, Krzysztof
On 2/21/2024 4:31 PM, Krzysztof Kozlowski wrote: > On 21/02/2024 11:34, Md Sadre Alam wrote: >> >> >> On 2/20/2024 5:06 PM, Krzysztof Kozlowski wrote: >>> On 20/02/2024 12:32, Md Sadre Alam wrote: >>>> >>>> >>>> On 2/19/2024 6:34 PM, Manivannan Sadhasivam wrote: >>>>> On Thu, Feb 15, 2024 at 07:18:51PM +0530, Md Sadre Alam wrote: >>>>>> This series of patches will add initial supports >>>>>> for QPIC SPI NAND driver. >>>>>> >>>>>> Currently this driver support following commands >>>>>> >>>>>> -- RESET >>>>>> -- READ ID >>>>>> -- BLOCK ERASE >>>>>> -- PAGE READ >>>>>> -- PAGE WRITE >>>>>> -- GET FEATURE >>>>>> -- SET FEATURE >>>>>> -- BAD BLOCK CHECK >>>>>> >>>>>> This driver has been tested with dd command with read/write page >>>>>> with multiple file size 1MiB, 10MiB,40MiB etc. >>>>>> Also tested with "mtd" command like mtd erase, mtd write, mtd verify etc. >>>>>> >>>>> >>>>> This is not the first version isn't it? Where is the changelog describing what >>>>> has changed since then? >>>> >>>> The earlier patch was the RFC for design review only. >>> >>> RFC is state of patch, not version. This is v2 then. >>> >>> These RFC postings are really becoming mess. Some people make multiple >>> RFCs and then post v1 hiding entire previous history... And why even >>> bother with calling it RFC? >> >> Sorry, I was not aware of this. Shall I post the next one as V3 >> and add references to the RFC patch and this patch in the cover >> letter of V3? > > Yes, like with every posting. Thank you. > > Best regards, > Krzysztof >