| Message ID | 20240215134856.1313239-1-quic_mdalam@quicinc.com |
|---|---|
| Headers | show |
| Series | Add QPIC SPI NAND driver | expand |
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:44 PM, Mark Brown wrote: > On Thu, Feb 15, 2024 at 07:18:54PM +0530, Md Sadre Alam wrote: > >> +config SPI_QPIC_SNAND >> + tristate "QPIC SNAND controller" >> + default y > > Why is this driver so special it should be enabled by default? Sorry by mistake I kept this enabled by default, will change in next patch. > >> + depends on ARCH_QCOM > > Please add an || COMPILE_TEST so this gets some build coverage. Ok > >> + help >> + QPIC_SNAND (QPIC SPI NAND) driver for Qualcomm QPIC controller. >> + QPIC controller supports both parallel nand and serial nand. >> + This config will enable serial nand driver for QPIC controller. >> + >> config SPI_QUP >> tristate "Qualcomm SPI controller with QUP interface" >> depends on ARCH_QCOM || COMPILE_TEST >> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile >> index 4ff8d725ba5e..1ac3bac35007 100644 >> --- a/drivers/spi/Makefile >> +++ b/drivers/spi/Makefile >> @@ -153,6 +153,7 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA) += spi-xtensa-xtfpga.o >> obj-$(CONFIG_SPI_ZYNQ_QSPI) += spi-zynq-qspi.o >> obj-$(CONFIG_SPI_ZYNQMP_GQSPI) += spi-zynqmp-gqspi.o >> obj-$(CONFIG_SPI_AMD) += spi-amd.o >> +obj-$(CONFIG_SPI_QPIC_SNAND) += spi-qpic-snand.o > > Please keep this sorted. Ok > >> --- /dev/null >> +++ b/drivers/spi/spi-qpic-snand.c >> @@ -0,0 +1,1025 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved. > > Please make the entire comment a C++ one so things look more > intentional. Ok > >> +#define snandc_set_read_loc_first(snandc, reg, cw_offset, read_size, is_last_read_loc) \ >> +snandc_set_reg(snandc, reg, \ >> + ((cw_offset) << READ_LOCATION_OFFSET) | \ >> + ((read_size) << READ_LOCATION_SIZE) | \ >> + ((is_last_read_loc) << READ_LOCATION_LAST)) >> + >> +#define snandc_set_read_loc_last(snandc, reg, cw_offset, read_size, is_last_read_loc) \ >> +snandc_set_reg(snandc, reg, \ >> + ((cw_offset) << READ_LOCATION_OFFSET) | \ >> + ((read_size) << READ_LOCATION_SIZE) | \ >> + ((is_last_read_loc) << READ_LOCATION_LAST)) > > For type safety and legibility please write these as functions, mark > them as static inline if needed. Ok > >> +void snandc_set_reg(struct qcom_nand_controller *snandc, int offset, u32 val) >> +{ >> + struct nandc_regs *regs = snandc->regs; >> + __le32 *reg; >> + >> + reg = offset_to_nandc_reg(regs, offset); >> + >> + if (reg) >> + *reg = cpu_to_le32(val); >> +} > > This silently ignores writes to invalid registers, that doesn't seem > great. Ok > >> + return snandc->ecc_stats.failed ? -EBADMSG : snandc->ecc_stats.bitflips; > > For legibility please just write normal conditional statements. Ok > >> +static int qpic_snand_program_execute(struct qcom_nand_controller *snandc, >> + const struct spi_mem_op *op) >> +{ > >> + int num_cw = 4; > >> + data_buf = (u8 *)snandc->wbuf; > > Why the cast? If it's needed that smells like it's masking a bug, it > looks like it's casting from a u8 * to a u8 *. Ok Will fix in next patch. > >> + for (i = 0; i < num_cw; i++) { >> + int data_size; > > All these functions appear to hard code "num_cw" to 4. What is "num_cw" > and why are we doing this per function? QPIC controller internally works on code word size not on page and each code word size is 512-bytes so if page size is 2K then num_cw = 4, if page size is 4K then num_cw = 8. Will not hard code this value to 4 or 8 , will fix this in next patch. > >> +static int qpic_snand_program_execute(struct qcom_nand_controller *snandc, >> + const struct spi_mem_op *op) > >> + if (op->cmd.opcode == SPINAND_READID) { >> + snandc->buf_count = 4; >> + read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL); >> + >> + ret = submit_descs(snandc); >> + if (ret) >> + dev_err(snandc->dev, "failure in submitting descriptor for readid\n"); >> + >> + nandc_read_buffer_sync(snandc, true); >> + memcpy(op->data.buf.in, snandc->reg_read_buf, snandc->buf_count); > > These memcpy()s don't seem great, why aren't we just reading directly > into the output buffer? This reg_read_buf is being used in common API so that it will be used by both serial nand as well raw nand, so I can't directly use the output buffer since internally CW mechanism I have to maintain in common API. > >> + if (op->cmd.opcode == SPINAND_GET_FEATURE) { > > This function looks like it should be a switch statement. Ok > >> +static bool qpic_snand_is_page_op(const struct spi_mem_op *op) >> +{ > >> + if (op->data.dir == SPI_MEM_DATA_IN) { >> + if (op->addr.buswidth == 4 && op->data.buswidth == 4) >> + return true; >> + >> + if (op->addr.nbytes == 2 && op->addr.buswidth == 1) >> + return true; >> + >> + } else if (op->data.dir == SPI_MEM_DATA_OUT) { >> + if (op->data.buswidth == 4) >> + return true; >> + if (op->addr.nbytes == 2 && op->addr.buswidth == 1) >> + return true; >> + } > > Again looks like a switch statement. Ok > >> + ctlr = devm_spi_alloc_master(dev, sizeof(*snandc)); >> + if (!ctlr) >> + return -ENOMEM; > > Please use _alloc_controller. Ok > >> +static int qcom_snand_remove(struct platform_device *pdev) >> +{ >> + struct spi_controller *ctlr = platform_get_drvdata(pdev); >> + >> + spi_unregister_controller(ctlr); >> + >> + return 0; >> +} > > We don't disable any of the clocks in the remove path. OK will fix in next patch. Thanks for reviewing, will address all your comments in the next patch. Regards, Alam.
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 7:54 PM, Conor Dooley wrote: > On Thu, Feb 15, 2024 at 07:18:52PM +0530, Md Sadre Alam wrote: >> Add device-tree binding documentation for QCOM QPIC-SNAND-NAND Flash >> Interface. >> >> 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 >> @@ -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> >> + >> +properties: >> + compatible: >> + enum: >> + - qcom,ipq9574-snand >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + minItems: 2 >> + maxItems: 3 >> + >> + clock-names: >> + minItems: 2 >> + maxItems: 3 >> + >> +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 > > None of this complexity here is needed, you have only one device in this > binding and therefore can define these properties at the top level. Thanks for reviewing, Will fix this in next patch. > > Cheers, > Conor. > >> +required: >> + - compatible >> + - reg >> + - clocks >> + - clock-names >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/qcom,ipq9574-gcc.h> >> + qpic_nand: spi@79b0000 { >> + 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>; >> + }; >> + }; >> -- >> 2.34.1 >>
On 2/15/2024 8:30 PM, 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? Yes its exclusive, gpios are shared b/w NAND and eMMC so that only one could get enable. > >> >> 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> >> --- >> 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 { >> -- >> 2.34.1 >> >> > >
On 2/15/2024 11:27 PM, Konrad Dybcio wrote: > On 15.02.2024 14:48, Md Sadre Alam wrote: >> Add qpic spi nand driver support. The spi nand >> driver currently supported the below commands. >> >> -- RESET >> -- READ ID >> -- SET FEATURE >> -- GET FEATURE >> -- READ PAGE >> -- WRITE PAGE >> -- ERASE PAGE >> >> 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> >> --- > > [...] > >> +void snandc_set_reg(struct qcom_nand_controller *snandc, int offset, u32 val) >> +{ >> + struct nandc_regs *regs = snandc->regs; >> + __le32 *reg; >> + >> + reg = offset_to_nandc_reg(regs, offset); >> + >> + if (reg) >> + *reg = cpu_to_le32(val); > > if (WARN_ON(!reg)) > return; > > instead? > > This would be tragic.. will fix this in next patch. > > [...] > >> + >> + ecc_cfg->cfg0 = (cwperpage - 1) << CW_PER_PAGE >> + | ecc_cfg->cw_data << UD_SIZE_BYTES >> + | 1 << DISABLE_STATUS_AFTER_WRITE >> + | 3 << NUM_ADDR_CYCLES >> + | ecc_cfg->ecc_bytes_hw << ECC_PARITY_SIZE_BYTES_RS >> + | 0 << STATUS_BFR_READ >> + | 1 << SET_RD_MODE_AFTER_STATUS >> + | ecc_cfg->spare_bytes << SPARE_SIZE_BYTES; > > Let me introduce you to FIELD_PREP/GET and GENMASK().. Many assignments > in this file could use these. Ok > > Konrad Thanks for reviewing, will fix all the comments in next patch. Regards, Alam.
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 Tue, Feb 20, 2024 at 05:24:43PM +0530, Md Sadre Alam wrote: > On 2/15/2024 7:44 PM, Mark Brown wrote: > > On Thu, Feb 15, 2024 at 07:18:54PM +0530, Md Sadre Alam wrote: > > > + if (op->cmd.opcode == SPINAND_READID) { > > > + snandc->buf_count = 4; > > > + read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL); > > > + > > > + ret = submit_descs(snandc); > > > + if (ret) > > > + dev_err(snandc->dev, "failure in submitting descriptor for readid\n"); > > > + > > > + nandc_read_buffer_sync(snandc, true); > > > + memcpy(op->data.buf.in, snandc->reg_read_buf, snandc->buf_count); > > These memcpy()s don't seem great, why aren't we just reading directly > > into the output buffer? > This reg_read_buf is being used in common API so that it will be used by both > serial nand as well raw nand, so I can't directly use the output buffer since > internally CW mechanism I have to maintain in common API. We have control over all the source code in the kernel so if there's problems with the internal interfaces we can improve them.
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 2/20/2024 9:34 PM, Mark Brown wrote: > On Tue, Feb 20, 2024 at 05:24:43PM +0530, Md Sadre Alam wrote: >> On 2/15/2024 7:44 PM, Mark Brown wrote: >>> On Thu, Feb 15, 2024 at 07:18:54PM +0530, Md Sadre Alam wrote: > >>>> + if (op->cmd.opcode == SPINAND_READID) { >>>> + snandc->buf_count = 4; >>>> + read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL); >>>> + >>>> + ret = submit_descs(snandc); >>>> + if (ret) >>>> + dev_err(snandc->dev, "failure in submitting descriptor for readid\n"); >>>> + >>>> + nandc_read_buffer_sync(snandc, true); >>>> + memcpy(op->data.buf.in, snandc->reg_read_buf, snandc->buf_count); > >>> These memcpy()s don't seem great, why aren't we just reading directly >>> into the output buffer? > >> This reg_read_buf is being used in common API so that it will be used by both >> serial nand as well raw nand, so I can't directly use the output buffer since >> internally CW mechanism I have to maintain in common API. > > We have control over all the source code in the kernel so if there's > problems with the internal interfaces we can improve them. Ok , will try to maintain CW mechanism uniform and remove from common API and move to corresponding driver. Will try to fix in next patch.
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 >
Konrad, On 2/20/2024 5:44 PM, Md Sadre Alam wrote: >>> + ecc_cfg->cfg0 = (cwperpage - 1) << CW_PER_PAGE >>> + | ecc_cfg->cw_data << UD_SIZE_BYTES >>> + | 1 << DISABLE_STATUS_AFTER_WRITE >>> + | 3 << NUM_ADDR_CYCLES >>> + | ecc_cfg->ecc_bytes_hw << ECC_PARITY_SIZE_BYTES_RS >>> + | 0 << STATUS_BFR_READ >>> + | 1 << SET_RD_MODE_AFTER_STATUS >>> + | ecc_cfg->spare_bytes << SPARE_SIZE_BYTES; >> >> Let me introduce you to FIELD_PREP/GET and GENMASK().. Many assignments >> in this file could use these. > > Ok While doing the change i realized that it will impact raw nand driver as well. Shall I post this change as separate patch. Is this ok? Please let me know. Regards, Alam. >> >> Konrad > > Thanks for reviewing, will fix all the comments in next patch. > > Regards, > Alam. >
On 3/6/24 07:01, Md Sadre Alam wrote: > Konrad, > > On 2/20/2024 5:44 PM, Md Sadre Alam wrote: >>>> + ecc_cfg->cfg0 = (cwperpage - 1) << CW_PER_PAGE >>>> + | ecc_cfg->cw_data << UD_SIZE_BYTES >>>> + | 1 << DISABLE_STATUS_AFTER_WRITE >>>> + | 3 << NUM_ADDR_CYCLES >>>> + | ecc_cfg->ecc_bytes_hw << ECC_PARITY_SIZE_BYTES_RS >>>> + | 0 << STATUS_BFR_READ >>>> + | 1 << SET_RD_MODE_AFTER_STATUS >>>> + | ecc_cfg->spare_bytes << SPARE_SIZE_BYTES; >>> >>> Let me introduce you to FIELD_PREP/GET and GENMASK().. Many assignments >>> in this file could use these. >> >> Ok > > While doing the change i realized that it will impact raw nand driver as well. > Shall I post this change as separate patch. Is this ok? Please let me know. One patch per file/topic, yes, please Konrad
On 3/6/2024 9:57 PM, Konrad Dybcio wrote: > > > On 3/6/24 07:01, Md Sadre Alam wrote: >> Konrad, >> >> On 2/20/2024 5:44 PM, Md Sadre Alam wrote: >>>>> + ecc_cfg->cfg0 = (cwperpage - 1) << CW_PER_PAGE >>>>> + | ecc_cfg->cw_data << UD_SIZE_BYTES >>>>> + | 1 << DISABLE_STATUS_AFTER_WRITE >>>>> + | 3 << NUM_ADDR_CYCLES >>>>> + | ecc_cfg->ecc_bytes_hw << ECC_PARITY_SIZE_BYTES_RS >>>>> + | 0 << STATUS_BFR_READ >>>>> + | 1 << SET_RD_MODE_AFTER_STATUS >>>>> + | ecc_cfg->spare_bytes << SPARE_SIZE_BYTES; >>>> >>>> Let me introduce you to FIELD_PREP/GET and GENMASK().. Many assignments >>>> in this file could use these. >>> >>> Ok >> >> While doing the change i realized that it will impact raw nand driver as well. >> Shall I post this change as separate patch. Is this ok? Please let me know. > > One patch per file/topic, yes, please Thank you > > Konrad