| Message ID | 1498720566-20782-2-git-send-email-absahu@codeaurora.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
On 06/29/2017 09:15 AM, Abhishek Sahu wrote: > The current driver only support EBI2 NAND which uses ADM DMA. The > latest QCOM controller supports QPIC NAND which uses BAM DMA. NAND > registers and programming sequence are same for EBI2 and QPIC > NAND so the same driver can support QPIC NAND also by adding the > BAM DMA support. This patch adds the QPIC NAND support in current > NAND driver with compatible string "qcom,qpic-nandc-v1.4.0" and > maps it with different configuration parameter in driver data. > > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> Split this into DT bindings patch and code patch ... > --- > .../devicetree/bindings/mtd/qcom_nandc.txt | 41 +++++++++++++++++++++- > drivers/mtd/nand/qcom_nandc.c | 37 ++++++++++++++++--- > 2 files changed, 73 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > index 70dd511..5d0f7ae 100644 > --- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > +++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > @@ -1,7 +1,9 @@ > * Qualcomm NAND controller > > Required properties: > -- compatible: should be "qcom,ipq806x-nand" > +- compatible: must be one of the following: > + * "qcom,ebi2-nandc" - EBI2 NAND which uses ADM DMA like IPQ8064. > + * "qcom,qpic-nandc-v1.4.0" - QPIC NAND v1.4.0 which uses BAM DMA like IPQ4019. > - reg: MMIO address range > - clocks: must contain core clock and always on clock > - clock-names: must contain "core" for the core clock and "aon" for the > @@ -84,3 +86,40 @@ nand@1ac00000 { > }; > }; > }; > + > +nand@79b0000 { > + compatible = "qcom,qpic-nandc-v1.4.0"; > + reg = <0x79b0000 0x1000>; > + > + clocks = <&gcc GCC_QPIC_CLK>, > + <&gcc GCC_QPIC_AHB_CLK>; > + clock-names = "core", "aon"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + nandcs@0 { > + compatible = "qcom,nandcs"; > + reg = <0>; > + > + nand-ecc-strength = <4>; > + nand-ecc-step-size = <512>; > + nand-bus-width = <8>; > + > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + partition@0 { > + label = "boot-nand"; > + reg = <0 0x58a0000>; > + }; > + > + partition@58a0000 { > + label = "fs-nand"; > + reg = <0x58a0000 0x4000000>; > + }; > + }; > + }; > +}; > diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c > index 57d483a..f55f728 100644 > --- a/drivers/mtd/nand/qcom_nandc.c > +++ b/drivers/mtd/nand/qcom_nandc.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2016, The Linux Foundation. All rights reserved. > + * Copyright (c) 2016-2017, The Linux Foundation. All rights reserved. > * > * This software is licensed under the terms of the GNU General Public > * License version 2, as published by the Free Software Foundation, and > @@ -234,6 +234,8 @@ struct nandc_regs { > * @cmd1/vld: some fixed controller register values > * @ecc_modes: supported ECC modes by the current controller, > * initialized via DT match data > + * @dma_bam_enabled: flag to tell whether nand controller is using > + * bam dma > */ > struct qcom_nand_controller { > struct nand_hw_control controller; > @@ -253,6 +255,7 @@ struct qcom_nand_controller { > struct list_head desc_list; > > u8 *data_buffer; > + bool dma_bam_enabled; > int buf_size; > int buf_count; > int buf_start; > @@ -316,6 +319,17 @@ struct qcom_nand_host { > u32 clrreadstatus; > }; > > +/* > + * This data type corresponds to the nand driver data which will be used at > + * driver probe time > + * @ecc_modes - ecc mode for nand > + * @dma_bam_enabled - whether this driver is using bam > + */ > +struct qcom_nand_driver_data { > + u32 ecc_modes; > + bool dma_bam_enabled; > +}; > + > static inline struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip) > { > return container_of(chip, struct qcom_nand_host, chip); > @@ -2073,6 +2087,7 @@ static int qcom_nandc_probe(struct platform_device *pdev) > struct device_node *dn = dev->of_node, *child; > struct resource *res; > int ret; > + const struct qcom_nand_driver_data *driver_data; > > nandc = devm_kzalloc(&pdev->dev, sizeof(*nandc), GFP_KERNEL); > if (!nandc) > @@ -2087,7 +2102,10 @@ static int qcom_nandc_probe(struct platform_device *pdev) > return -ENODEV; > } > > - nandc->ecc_modes = (unsigned long)dev_data; > + driver_data = (const struct qcom_nand_driver_data *)dev_data; > + > + nandc->ecc_modes = driver_data->ecc_modes; > + nandc->dma_bam_enabled = driver_data->dma_bam_enabled; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > nandc->base = devm_ioremap_resource(dev, res); > @@ -2179,15 +2197,26 @@ static int qcom_nandc_remove(struct platform_device *pdev) > return 0; > } > > -#define EBI2_NANDC_ECC_MODES (ECC_RS_4BIT | ECC_BCH_8BIT) > > +static const struct qcom_nand_driver_data ebi2_nandc_data = { > + .ecc_modes = (ECC_RS_4BIT | ECC_BCH_8BIT), > + .dma_bam_enabled = false, > +}; > + > +static const struct qcom_nand_driver_data qpic_nandc_v1_4_0_data = { > + .ecc_modes = (ECC_BCH_4BIT | ECC_BCH_8BIT), > + .dma_bam_enabled = true, > +}; > /* > * data will hold a struct pointer containing more differences once we support > * more controller variants > */ > static const struct of_device_id qcom_nandc_of_match[] = { > { .compatible = "qcom,ipq806x-nand", > - .data = (void *)EBI2_NANDC_ECC_MODES, > + .data = (void *)&ebi2_nandc_data, > + }, > + { .compatible = "qcom,qpic-nandc-v1.4.0", > + .data = (void *)&qpic_nandc_v1_4_0_data, > }, > {} > }; >
On 06/29/2017 12:45 PM, Abhishek Sahu wrote: > The current driver only support EBI2 NAND which uses ADM DMA. The > latest QCOM controller supports QPIC NAND which uses BAM DMA. NAND > registers and programming sequence are same for EBI2 and QPIC > NAND so the same driver can support QPIC NAND also by adding the > BAM DMA support. This patch adds the QPIC NAND support in current > NAND driver with compatible string "qcom,qpic-nandc-v1.4.0" and > maps it with different configuration parameter in driver data. > > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> > --- > .../devicetree/bindings/mtd/qcom_nandc.txt | 41 +++++++++++++++++++++- > drivers/mtd/nand/qcom_nandc.c | 37 ++++++++++++++++--- > 2 files changed, 73 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > index 70dd511..5d0f7ae 100644 > --- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > +++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > @@ -1,7 +1,9 @@ > * Qualcomm NAND controller > > Required properties: > -- compatible: should be "qcom,ipq806x-nand" Since you're changing the compatible string, could you mention in the commit message that it's okay to do so since there aren't any upstream dtsi files using this binding? > +- compatible: must be one of the following: > + * "qcom,ebi2-nandc" - EBI2 NAND which uses ADM DMA like IPQ8064. Are we sure that all EBI2 based NAND controllers would work by this single binding? Should we put a version here too like we've done for QPIC? > + * "qcom,qpic-nandc-v1.4.0" - QPIC NAND v1.4.0 which uses BAM DMA like IPQ4019. > - reg: MMIO address range > - clocks: must contain core clock and always on clock > - clock-names: must contain "core" for the core clock and "aon" for the > @@ -84,3 +86,40 @@ nand@1ac00000 { > }; > }; > }; > + > +nand@79b0000 { > + compatible = "qcom,qpic-nandc-v1.4.0"; > + reg = <0x79b0000 0x1000>; > + > + clocks = <&gcc GCC_QPIC_CLK>, > + <&gcc GCC_QPIC_AHB_CLK>; > + clock-names = "core", "aon"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + nandcs@0 { > + compatible = "qcom,nandcs"; > + reg = <0>; > + > + nand-ecc-strength = <4>; > + nand-ecc-step-size = <512>; > + nand-bus-width = <8>; > + > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + partition@0 { > + label = "boot-nand"; > + reg = <0 0x58a0000>; > + }; > + > + partition@58a0000 { > + label = "fs-nand"; > + reg = <0x58a0000 0x4000000>; > + }; > + }; > + }; > +}; > diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c > index 57d483a..f55f728 100644 > --- a/drivers/mtd/nand/qcom_nandc.c > +++ b/drivers/mtd/nand/qcom_nandc.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2016, The Linux Foundation. All rights reserved. > + * Copyright (c) 2016-2017, The Linux Foundation. All rights reserved. > * > * This software is licensed under the terms of the GNU General Public > * License version 2, as published by the Free Software Foundation, and > @@ -234,6 +234,8 @@ struct nandc_regs { > * @cmd1/vld: some fixed controller register values > * @ecc_modes: supported ECC modes by the current controller, > * initialized via DT match data > + * @dma_bam_enabled: flag to tell whether nand controller is using > + * bam dma > */ > struct qcom_nand_controller { > struct nand_hw_control controller; > @@ -253,6 +255,7 @@ struct qcom_nand_controller { > struct list_head desc_list; > > u8 *data_buffer; > + bool dma_bam_enabled; > int buf_size; > int buf_count; > int buf_start; > @@ -316,6 +319,17 @@ struct qcom_nand_host { > u32 clrreadstatus; > }; > > +/* > + * This data type corresponds to the nand driver data which will be used at > + * driver probe time > + * @ecc_modes - ecc mode for nand > + * @dma_bam_enabled - whether this driver is using bam > + */ > +struct qcom_nand_driver_data { > + u32 ecc_modes; > + bool dma_bam_enabled; > +}; > + > static inline struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip) > { > return container_of(chip, struct qcom_nand_host, chip); > @@ -2073,6 +2087,7 @@ static int qcom_nandc_probe(struct platform_device *pdev) > struct device_node *dn = dev->of_node, *child; > struct resource *res; > int ret; > + const struct qcom_nand_driver_data *driver_data; > > nandc = devm_kzalloc(&pdev->dev, sizeof(*nandc), GFP_KERNEL); > if (!nandc) > @@ -2087,7 +2102,10 @@ static int qcom_nandc_probe(struct platform_device *pdev) > return -ENODEV; > } > > - nandc->ecc_modes = (unsigned long)dev_data; > + driver_data = (const struct qcom_nand_driver_data *)dev_data; > + > + nandc->ecc_modes = driver_data->ecc_modes; > + nandc->dma_bam_enabled = driver_data->dma_bam_enabled; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > nandc->base = devm_ioremap_resource(dev, res); > @@ -2179,15 +2197,26 @@ static int qcom_nandc_remove(struct platform_device *pdev) > return 0; > } > > -#define EBI2_NANDC_ECC_MODES (ECC_RS_4BIT | ECC_BCH_8BIT) > > +static const struct qcom_nand_driver_data ebi2_nandc_data = { > + .ecc_modes = (ECC_RS_4BIT | ECC_BCH_8BIT), > + .dma_bam_enabled = false, > +}; > + > +static const struct qcom_nand_driver_data qpic_nandc_v1_4_0_data = { > + .ecc_modes = (ECC_BCH_4BIT | ECC_BCH_8BIT), > + .dma_bam_enabled = true, > +}; > /* > * data will hold a struct pointer containing more differences once we support > * more controller variants > */ > static const struct of_device_id qcom_nandc_of_match[] = { > { .compatible = "qcom,ipq806x-nand", Please make sure that you update the compatible string above too. Thanks, Archit > - .data = (void *)EBI2_NANDC_ECC_MODES, > + .data = (void *)&ebi2_nandc_data, > + }, > + { .compatible = "qcom,qpic-nandc-v1.4.0", > + .data = (void *)&qpic_nandc_v1_4_0_data, > }, > {} > }; >
Hi Abhishek, On 6/29/2017 12:45 PM, Abhishek Sahu wrote: > The current driver only support EBI2 NAND which uses ADM DMA. The > latest QCOM controller supports QPIC NAND which uses BAM DMA. NAND > registers and programming sequence are same for EBI2 and QPIC > NAND so the same driver can support QPIC NAND also by adding the > BAM DMA support. This patch adds the QPIC NAND support in current > NAND driver with compatible string "qcom,qpic-nandc-v1.4.0" and > maps it with different configuration parameter in driver data. > > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> > --- > .../devicetree/bindings/mtd/qcom_nandc.txt | 41 +++++++++++++++++++++- > drivers/mtd/nand/qcom_nandc.c | 37 ++++++++++++++++--- > 2 files changed, 73 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > index 70dd511..5d0f7ae 100644 > --- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > +++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > @@ -1,7 +1,9 @@ > * Qualcomm NAND controller > > Required properties: > -- compatible: should be "qcom,ipq806x-nand" > +- compatible: must be one of the following: > + * "qcom,ebi2-nandc" - EBI2 NAND which uses ADM DMA like IPQ8064. > + * "qcom,qpic-nandc-v1.4.0" - QPIC NAND v1.4.0 which uses BAM DMA like IPQ4019. > - reg: MMIO address range > - clocks: must contain core clock and always on clock > - clock-names: must contain "core" for the core clock and "aon" for the > @@ -84,3 +86,40 @@ nand@1ac00000 { > }; > }; > }; > + > +nand@79b0000 { > + compatible = "qcom,qpic-nandc-v1.4.0"; > + reg = <0x79b0000 0x1000>; > + > + clocks = <&gcc GCC_QPIC_CLK>, > + <&gcc GCC_QPIC_AHB_CLK>; > + clock-names = "core", "aon"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + nandcs@0 { > + compatible = "qcom,nandcs"; > + reg = <0>; > + > + nand-ecc-strength = <4>; > + nand-ecc-step-size = <512>; > + nand-bus-width = <8>; > + > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + partition@0 { > + label = "boot-nand"; > + reg = <0 0x58a0000>; > + }; > + > + partition@58a0000 { > + label = "fs-nand"; > + reg = <0x58a0000 0x4000000>; > + }; > + }; > + }; > +}; > diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c > index 57d483a..f55f728 100644 > --- a/drivers/mtd/nand/qcom_nandc.c > +++ b/drivers/mtd/nand/qcom_nandc.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2016, The Linux Foundation. All rights reserved. > + * Copyright (c) 2016-2017, The Linux Foundation. All rights reserved. > * > * This software is licensed under the terms of the GNU General Public > * License version 2, as published by the Free Software Foundation, and > @@ -234,6 +234,8 @@ struct nandc_regs { > * @cmd1/vld: some fixed controller register values > * @ecc_modes: supported ECC modes by the current controller, > * initialized via DT match data > + * @dma_bam_enabled: flag to tell whether nand controller is using > + * bam dma > */ simply is_bam ? > struct qcom_nand_controller { > struct nand_hw_control controller; > @@ -253,6 +255,7 @@ struct qcom_nand_controller { > struct list_head desc_list; > > u8 *data_buffer; > + bool dma_bam_enabled; > int buf_size; > int buf_count; > int buf_start; > @@ -316,6 +319,17 @@ struct qcom_nand_host { > u32 clrreadstatus; > }; > > +/* > + * This data type corresponds to the nand driver data which will be used at > + * driver probe time > + * @ecc_modes - ecc mode for nand > + * @dma_bam_enabled - whether this driver is using bam > + */ > +struct qcom_nand_driver_data { > + u32 ecc_modes; > + bool dma_bam_enabled; > +}; > + > static inline struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip) > { > return container_of(chip, struct qcom_nand_host, chip); > @@ -2073,6 +2087,7 @@ static int qcom_nandc_probe(struct platform_device *pdev) > struct device_node *dn = dev->of_node, *child; > struct resource *res; > int ret; > + const struct qcom_nand_driver_data *driver_data; Directly assign of the of_device_match_data here and avoid additional variable, cast. Regards, Sricharan > > nandc = devm_kzalloc(&pdev->dev, sizeof(*nandc), GFP_KERNEL); > if (!nandc) > @@ -2087,7 +2102,10 @@ static int qcom_nandc_probe(struct platform_device *pdev) > return -ENODEV; > } > > - nandc->ecc_modes = (unsigned long)dev_data; > + driver_data = (const struct qcom_nand_driver_data *)dev_data; > + > + nandc->ecc_modes = driver_data->ecc_modes; > + nandc->dma_bam_enabled = driver_data->dma_bam_enabled; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > nandc->base = devm_ioremap_resource(dev, res); > @@ -2179,15 +2197,26 @@ static int qcom_nandc_remove(struct platform_device *pdev) > return 0; > } > > -#define EBI2_NANDC_ECC_MODES (ECC_RS_4BIT | ECC_BCH_8BIT) > > +static const struct qcom_nand_driver_data ebi2_nandc_data = { > + .ecc_modes = (ECC_RS_4BIT | ECC_BCH_8BIT), > + .dma_bam_enabled = false, > +}; > + > +static const struct qcom_nand_driver_data qpic_nandc_v1_4_0_data = { > + .ecc_modes = (ECC_BCH_4BIT | ECC_BCH_8BIT), > + .dma_bam_enabled = true, > +}; > /* > * data will hold a struct pointer containing more differences once we support > * more controller variants > */ > static const struct of_device_id qcom_nandc_of_match[] = { > { .compatible = "qcom,ipq806x-nand", > - .data = (void *)EBI2_NANDC_ECC_MODES, > + .data = (void *)&ebi2_nandc_data, > + }, > + { .compatible = "qcom,qpic-nandc-v1.4.0", > + .data = (void *)&qpic_nandc_v1_4_0_data, > }, > {} > }; >
On Mon, 3 Jul 2017 10:08:32 +0530 Archit Taneja <architt@codeaurora.org> wrote: > On 06/29/2017 12:45 PM, Abhishek Sahu wrote: > > The current driver only support EBI2 NAND which uses ADM DMA. The > > latest QCOM controller supports QPIC NAND which uses BAM DMA. NAND > > registers and programming sequence are same for EBI2 and QPIC > > NAND so the same driver can support QPIC NAND also by adding the > > BAM DMA support. This patch adds the QPIC NAND support in current > > NAND driver with compatible string "qcom,qpic-nandc-v1.4.0" and > > maps it with different configuration parameter in driver data. > > > > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> > > --- > > .../devicetree/bindings/mtd/qcom_nandc.txt | 41 +++++++++++++++++++++- > > drivers/mtd/nand/qcom_nandc.c | 37 ++++++++++++++++--- > > 2 files changed, 73 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > > index 70dd511..5d0f7ae 100644 > > --- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > > +++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > > @@ -1,7 +1,9 @@ > > * Qualcomm NAND controller > > > > Required properties: > > -- compatible: should be "qcom,ipq806x-nand" > > Since you're changing the compatible string, could you mention in the commit message that > it's okay to do so since there aren't any upstream dtsi files using this binding? Yep. I was going to ask about backward compat, but I guess it's fine if there's no user in mainline yet, just mention it in the commit message as suggested by Archit. > > > +- compatible: must be one of the following: > > + * "qcom,ebi2-nandc" - EBI2 NAND which uses ADM DMA like IPQ8064. > > Are we sure that all EBI2 based NAND controllers would work by this single binding? > Should we put a version here too like we've done for QPIC? > > > + * "qcom,qpic-nandc-v1.4.0" - QPIC NAND v1.4.0 which uses BAM DMA like IPQ4019. > > - reg: MMIO address range > > - clocks: must contain core clock and always on clock > > - clock-names: must contain "core" for the core clock and "aon" for the > > @@ -84,3 +86,40 @@ nand@1ac00000 { > > }; > > }; > > }; > > + > > +nand@79b0000 { nand-controller@xxxx { BTW, glad to see another driver moving to the new DT representation :-). > > + compatible = "qcom,qpic-nandc-v1.4.0"; > > + reg = <0x79b0000 0x1000>; > > + > > + clocks = <&gcc GCC_QPIC_CLK>, > > + <&gcc GCC_QPIC_AHB_CLK>; > > + clock-names = "core", "aon"; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + nandcs@0 { nand@0 { > > + compatible = "qcom,nandcs"; Why do you need a compatible here? > > + reg = <0>; > > + > > + nand-ecc-strength = <4>; > > + nand-ecc-step-size = <512>; > > + nand-bus-width = <8>; > > + > > + partitions { > > + compatible = "fixed-partitions"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + > > + partition@0 { > > + label = "boot-nand"; > > + reg = <0 0x58a0000>; > > + }; > > + > > + partition@58a0000 { > > + label = "fs-nand"; > > + reg = <0x58a0000 0x4000000>; > > + }; > > + }; > > + }; > > +}; > > diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c > > index 57d483a..f55f728 100644 > > --- a/drivers/mtd/nand/qcom_nandc.c > > +++ b/drivers/mtd/nand/qcom_nandc.c > > @@ -1,5 +1,5 @@ > > /* > > - * Copyright (c) 2016, The Linux Foundation. All rights reserved. > > + * Copyright (c) 2016-2017, The Linux Foundation. All rights reserved. > > * > > * This software is licensed under the terms of the GNU General Public > > * License version 2, as published by the Free Software Foundation, and > > @@ -234,6 +234,8 @@ struct nandc_regs { > > * @cmd1/vld: some fixed controller register values > > * @ecc_modes: supported ECC modes by the current controller, > > * initialized via DT match data > > + * @dma_bam_enabled: flag to tell whether nand controller is using > > + * bam dma > > */ > > struct qcom_nand_controller { > > struct nand_hw_control controller; > > @@ -253,6 +255,7 @@ struct qcom_nand_controller { > > struct list_head desc_list; > > > > u8 *data_buffer; > > + bool dma_bam_enabled; > > int buf_size; > > int buf_count; > > int buf_start; > > @@ -316,6 +319,17 @@ struct qcom_nand_host { > > u32 clrreadstatus; > > }; > > > > +/* > > + * This data type corresponds to the nand driver data which will be used at > > + * driver probe time > > + * @ecc_modes - ecc mode for nand > > + * @dma_bam_enabled - whether this driver is using bam > > + */ > > +struct qcom_nand_driver_data { > > + u32 ecc_modes; > > + bool dma_bam_enabled; > > +}; > > + > > static inline struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip) > > { > > return container_of(chip, struct qcom_nand_host, chip); > > @@ -2073,6 +2087,7 @@ static int qcom_nandc_probe(struct platform_device *pdev) > > struct device_node *dn = dev->of_node, *child; > > struct resource *res; > > int ret; > > + const struct qcom_nand_driver_data *driver_data; > > > > nandc = devm_kzalloc(&pdev->dev, sizeof(*nandc), GFP_KERNEL); > > if (!nandc) > > @@ -2087,7 +2102,10 @@ static int qcom_nandc_probe(struct platform_device *pdev) > > return -ENODEV; > > } > > > > - nandc->ecc_modes = (unsigned long)dev_data; > > + driver_data = (const struct qcom_nand_driver_data *)dev_data; Cast is unneeded here. > > + > > + nandc->ecc_modes = driver_data->ecc_modes; > > + nandc->dma_bam_enabled = driver_data->dma_bam_enabled; Why don't you store a pointer to the driver data object in your nandc struct? > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > nandc->base = devm_ioremap_resource(dev, res); > > @@ -2179,15 +2197,26 @@ static int qcom_nandc_remove(struct platform_device *pdev) > > return 0; > > } > > > > -#define EBI2_NANDC_ECC_MODES (ECC_RS_4BIT | ECC_BCH_8BIT) > > > > +static const struct qcom_nand_driver_data ebi2_nandc_data = { > > + .ecc_modes = (ECC_RS_4BIT | ECC_BCH_8BIT), > > + .dma_bam_enabled = false, > > +}; > > + > > +static const struct qcom_nand_driver_data qpic_nandc_v1_4_0_data = { > > + .ecc_modes = (ECC_BCH_4BIT | ECC_BCH_8BIT), > > + .dma_bam_enabled = true, > > +}; This patch should be split in 2 IMO: 1/ introduce the qcom_nand_driver_data struct (which I'd prefer to call qcom_nand_controller_caps, or something like that) and use it for the existing compatible 2/ add the new compat with its own set of capabilities. > > /* > > * data will hold a struct pointer containing more differences once we support > > * more controller variants > > */ > > static const struct of_device_id qcom_nandc_of_match[] = { > > { .compatible = "qcom,ipq806x-nand", > > Please make sure that you update the compatible string above too. > > Thanks, > Archit > > > - .data = (void *)EBI2_NANDC_ECC_MODES, > > + .data = (void *)&ebi2_nandc_data, Cast unneeded. > > + }, > > + { .compatible = "qcom,qpic-nandc-v1.4.0", > > + .data = (void *)&qpic_nandc_v1_4_0_data, Ditto. > > }, > > {} > > }; > > >
On 2017-07-04 01:11, Boris Brezillon wrote: > On Mon, 3 Jul 2017 10:08:32 +0530 > Archit Taneja <architt@codeaurora.org> wrote: > >> On 06/29/2017 12:45 PM, Abhishek Sahu wrote: >> > The current driver only support EBI2 NAND which uses ADM DMA. The >> > latest QCOM controller supports QPIC NAND which uses BAM DMA. NAND >> > registers and programming sequence are same for EBI2 and QPIC >> > NAND so the same driver can support QPIC NAND also by adding the >> > BAM DMA support. This patch adds the QPIC NAND support in current >> > NAND driver with compatible string "qcom,qpic-nandc-v1.4.0" and >> > maps it with different configuration parameter in driver data. >> > >> > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> >> > --- >> > .../devicetree/bindings/mtd/qcom_nandc.txt | 41 > +++++++++++++++++++++- >> > drivers/mtd/nand/qcom_nandc.c | 37 > ++++++++++++++++--- >> > 2 files changed, 73 insertions(+), 5 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt >> > index 70dd511..5d0f7ae 100644 >> > --- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt >> > +++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt >> > @@ -1,7 +1,9 @@ >> > * Qualcomm NAND controller >> > >> > Required properties: >> > -- compatible: should be "qcom,ipq806x-nand" >> >> Since you're changing the compatible string, could you mention in the > commit message that >> it's okay to do so since there aren't any upstream dtsi files using >> this > binding? > > Yep. I was going to ask about backward compat, but I guess it's fine if > there's no user in mainline yet, just mention it in the commit message > as suggested by Archit. > >> >> > +- compatible: must be one of the following: >> > + * "qcom,ebi2-nandc" - EBI2 NAND which uses ADM DMA like IPQ8064. >> >> Are we sure that all EBI2 based NAND controllers would work by this > single binding? >> Should we put a version here too like we've done for QPIC? The offsets are different in QPIC version 1.4.0 and 1.5.0 that's why I put different version. For EBI2, it uses same reg offsets as QPIC version 1.4.0. The EBI2 version for IPQ806x is 4.3.0 and if put like 4.3.0 then it will imply that NANDC driver only supports this particular version. since the original driver does not specifies any version, qcom,ebi2-nandc will support all the versions. >> >> > + * "qcom,qpic-nandc-v1.4.0" - QPIC NAND v1.4.0 which uses BAM DMA > like IPQ4019. >> > - reg: MMIO address range >> > - clocks: must contain core clock and always on clock >> > - clock-names: must contain "core" for the core clock and > "aon" for the >> > @@ -84,3 +86,40 @@ nand@1ac00000 { >> > }; >> > }; >> > }; >> > + >> > +nand@79b0000 { > > nand-controller@xxxx { > > BTW, glad to see another driver moving to the new DT representation > :-). > >> > + compatible = "qcom,qpic-nandc-v1.4.0"; >> > + reg = <0x79b0000 0x1000>; >> > + >> > + clocks = <&gcc GCC_QPIC_CLK>, >> > + <&gcc GCC_QPIC_AHB_CLK>; >> > + clock-names = "core", "aon"; >> > + >> > + #address-cells = <1>; >> > + #size-cells = <0>; >> > + >> > + nandcs@0 { > > nand@0 { > >> > + compatible = "qcom,nandcs"; > > Why do you need a compatible here? It is the part of original driver. We can connect multiple NAND devices in the same bus and qcom,nandcs is being used for each connected NAND device. Each NAND device can use different chip select, ecc strength etc which we can specify under this sub node. > >> > + reg = <0>; >> > + >> > + nand-ecc-strength = <4>; >> > + nand-ecc-step-size = <512>; >> > + nand-bus-width = <8>; >> > + >> > + partitions { >> > + compatible = "fixed-partitions"; >> > + #address-cells = <1>; >> > + #size-cells = <1>; >> > + >> > + partition@0 { >> > + label = "boot-nand"; >> > + reg = <0 0x58a0000>; >> > + }; >> > + >> > + partition@58a0000 { >> > + label = "fs-nand"; >> > + reg = <0x58a0000 0x4000000>; >> > + }; >> > + }; >> > + }; >> > +}; >> > diff --git a/drivers/mtd/nand/qcom_nandc.c > b/drivers/mtd/nand/qcom_nandc.c >> > index 57d483a..f55f728 100644 >> > --- a/drivers/mtd/nand/qcom_nandc.c >> > +++ b/drivers/mtd/nand/qcom_nandc.c >> > @@ -1,5 +1,5 @@ >> > /* >> > - * Copyright (c) 2016, The Linux Foundation. All rights reserved. >> > + * Copyright (c) 2016-2017, The Linux Foundation. All rights > reserved. >> > * >> > * This software is licensed under the terms of the GNU General > Public >> > * License version 2, as published by the Free Software Foundation, > and >> > @@ -234,6 +234,8 @@ struct nandc_regs { >> > * @cmd1/vld: some fixed controller register > values >> > * @ecc_modes: supported ECC modes by the current > controller, >> > * initialized via DT match data >> > + * @dma_bam_enabled: flag to tell whether nand > controller is using >> > + * bam dma >> > */ >> > struct qcom_nand_controller { >> > struct nand_hw_control controller; >> > @@ -253,6 +255,7 @@ struct qcom_nand_controller { >> > struct list_head desc_list; >> > >> > u8 *data_buffer; >> > + bool dma_bam_enabled; >> > int buf_size; >> > int buf_count; >> > int buf_start; >> > @@ -316,6 +319,17 @@ struct qcom_nand_host { >> > u32 clrreadstatus; >> > }; >> > >> > +/* >> > + * This data type corresponds to the nand driver data which will be > used at >> > + * driver probe time >> > + * @ecc_modes - ecc mode for nand >> > + * @dma_bam_enabled - whether this driver is using bam >> > + */ >> > +struct qcom_nand_driver_data { >> > + u32 ecc_modes; >> > + bool dma_bam_enabled; >> > +}; >> > + >> > static inline struct qcom_nand_host *to_qcom_nand_host(struct > nand_chip *chip) >> > { >> > return container_of(chip, struct qcom_nand_host, chip); >> > @@ -2073,6 +2087,7 @@ static int qcom_nandc_probe(struct > platform_device *pdev) >> > struct device_node *dn = dev->of_node, *child; >> > struct resource *res; >> > int ret; >> > + const struct qcom_nand_driver_data *driver_data; >> > >> > nandc = devm_kzalloc(&pdev->dev, sizeof(*nandc), GFP_KERNEL); >> > if (!nandc) >> > @@ -2087,7 +2102,10 @@ static int qcom_nandc_probe(struct > platform_device *pdev) >> > return -ENODEV; >> > } >> > >> > - nandc->ecc_modes = (unsigned long)dev_data; >> > + driver_data = (const struct qcom_nand_driver_data *)dev_data; > > Cast is unneeded here. Sure. Will remove in v2. > >> > + >> > + nandc->ecc_modes = driver_data->ecc_modes; >> > + nandc->dma_bam_enabled = driver_data->dma_bam_enabled; > > Why don't you store a pointer to the driver data object in your nandc > struct? Storing driver data would be better. I will do the same in v2. > >> > >> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> > nandc->base = devm_ioremap_resource(dev, res); >> > @@ -2179,15 +2197,26 @@ static int qcom_nandc_remove(struct > platform_device *pdev) >> > return 0; >> > } >> > >> > -#define EBI2_NANDC_ECC_MODES (ECC_RS_4BIT | ECC_BCH_8BIT) >> > >> > +static const struct qcom_nand_driver_data ebi2_nandc_data = { >> > + .ecc_modes = (ECC_RS_4BIT | ECC_BCH_8BIT), >> > + .dma_bam_enabled = false, >> > +}; >> > + >> > +static const struct qcom_nand_driver_data qpic_nandc_v1_4_0_data = { >> > + .ecc_modes = (ECC_BCH_4BIT | ECC_BCH_8BIT), >> > + .dma_bam_enabled = true, >> > +}; > > This patch should be split in 2 IMO: > 1/ introduce the qcom_nand_driver_data struct (which I'd prefer to call > qcom_nand_controller_caps, or something like that) and use it for the > existing compatible > 2/ add the new compat with its own set of capabilities. Yes. Will do the same in v2. > >> > /* >> > * data will hold a struct pointer containing more differences once > we support >> > * more controller variants >> > */ >> > static const struct of_device_id qcom_nandc_of_match[] = { >> > { .compatible = "qcom,ipq806x-nand", >> >> Please make sure that you update the compatible string above too. >> >> Thanks, >> Archit >> >> > - .data = (void *)EBI2_NANDC_ECC_MODES, >> > + .data = (void *)&ebi2_nandc_data, > > Cast unneeded. > >> > + }, >> > + { .compatible = "qcom,qpic-nandc-v1.4.0", >> > + .data = (void *)&qpic_nandc_v1_4_0_data, > > Ditto. Will remove in v2. > >> > }, >> > {} >> > }; >> > >>
On Mon, 17 Jul 2017 11:41:01 +0530 Abhishek Sahu <absahu@codeaurora.org> wrote: > >> > + > >> > +nand@79b0000 { > > > > nand-controller@xxxx { > > > > BTW, glad to see another driver moving to the new DT representation > > :-). > > > >> > + compatible = "qcom,qpic-nandc-v1.4.0"; > >> > + reg = <0x79b0000 0x1000>; > >> > + > >> > + clocks = <&gcc GCC_QPIC_CLK>, > >> > + <&gcc GCC_QPIC_AHB_CLK>; > >> > + clock-names = "core", "aon"; > >> > + > >> > + #address-cells = <1>; > >> > + #size-cells = <0>; > >> > + > >> > + nandcs@0 { > > > > nand@0 { > > > >> > + compatible = "qcom,nandcs"; > > > > Why do you need a compatible here? > It is the part of original driver. We can connect multiple > NAND devices in the same bus and qcom,nandcs is being used > for each connected NAND device. Each NAND device can use > different chip select, ecc strength etc which we can specify > under this sub node. Still don't understand why you need a compatible? Is this a memory bus where you can connect other kind of memories (parallel NORs, SRAMs, ...)? If that's not the case, then considering all subnodes of the nand-controller node containing a reg property as NAND devices is fine, you don't need this compatible = "nand,cs" (see sunxi-nand bindings [1]). If the bus is generic and can be attached non-NAND devices, I'd recommend looking at atmel's binding [2], because you're likely to have one instance of the NAND controller logic for all NAND devices connected on this bus. And more importantly, if the bus a generic, the node should not be named nand or nand-controller, and the compatible should not contain 'nandc' in it. [1]http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt#L34 [2]http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/mtd/atmel-nand.txt#L70
On 2017-07-17 12:52, Boris Brezillon wrote: > On Mon, 17 Jul 2017 11:41:01 +0530 > Abhishek Sahu <absahu@codeaurora.org> wrote: > >> >> > + >> >> > +nand@79b0000 { >> > >> > nand-controller@xxxx { >> > >> > BTW, glad to see another driver moving to the new DT representation >> > :-). >> > >> >> > + compatible = "qcom,qpic-nandc-v1.4.0"; >> >> > + reg = <0x79b0000 0x1000>; >> >> > + >> >> > + clocks = <&gcc GCC_QPIC_CLK>, >> >> > + <&gcc GCC_QPIC_AHB_CLK>; >> >> > + clock-names = "core", "aon"; >> >> > + >> >> > + #address-cells = <1>; >> >> > + #size-cells = <0>; >> >> > + >> >> > + nandcs@0 { >> > >> > nand@0 { >> > >> >> > + compatible = "qcom,nandcs"; >> > >> > Why do you need a compatible here? >> It is the part of original driver. We can connect multiple >> NAND devices in the same bus and qcom,nandcs is being used >> for each connected NAND device. Each NAND device can use >> different chip select, ecc strength etc which we can specify >> under this sub node. > > > Still don't understand why you need a compatible? Is this a memory bus > where you can connect other kind of memories (parallel NORs, > SRAMs, ...)? > > If that's not the case, then considering all subnodes of the > nand-controller node containing a reg property as NAND devices is fine, > you don't need this compatible = "nand,cs" (see sunxi-nand bindings > [1]). > Thanks Boris for giving detailed references. We can connect other parallel devices also but we have different hardware wrappers over generic EBI2/QPIC which will MUX/arbitrate the device access from the hardware itself. So this NAND driver will only control multiple NAND devices. We can remove this compatible and use the bindings similar to sunxi-nand. I will do the required changes and will post in v2 of the same patch series. > If the bus is generic and can be attached non-NAND devices, I'd > recommend looking at atmel's binding [2], because you're likely to > have one instance of the NAND controller logic for all NAND devices > connected on this bus. > And more importantly, if the bus a generic, the node should not be > named nand or nand-controller, and the compatible should not contain > 'nandc' in it. > > [1]http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt#L34 > [2]http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/mtd/atmel-nand.txt#L70
diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt index 70dd511..5d0f7ae 100644 --- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt +++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt @@ -1,7 +1,9 @@ * Qualcomm NAND controller Required properties: -- compatible: should be "qcom,ipq806x-nand" +- compatible: must be one of the following: + * "qcom,ebi2-nandc" - EBI2 NAND which uses ADM DMA like IPQ8064. + * "qcom,qpic-nandc-v1.4.0" - QPIC NAND v1.4.0 which uses BAM DMA like IPQ4019. - reg: MMIO address range - clocks: must contain core clock and always on clock - clock-names: must contain "core" for the core clock and "aon" for the @@ -84,3 +86,40 @@ nand@1ac00000 { }; }; }; + +nand@79b0000 { + compatible = "qcom,qpic-nandc-v1.4.0"; + reg = <0x79b0000 0x1000>; + + clocks = <&gcc GCC_QPIC_CLK>, + <&gcc GCC_QPIC_AHB_CLK>; + clock-names = "core", "aon"; + + #address-cells = <1>; + #size-cells = <0>; + + nandcs@0 { + compatible = "qcom,nandcs"; + reg = <0>; + + nand-ecc-strength = <4>; + nand-ecc-step-size = <512>; + nand-bus-width = <8>; + + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + label = "boot-nand"; + reg = <0 0x58a0000>; + }; + + partition@58a0000 { + label = "fs-nand"; + reg = <0x58a0000 0x4000000>; + }; + }; + }; +}; diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c index 57d483a..f55f728 100644 --- a/drivers/mtd/nand/qcom_nandc.c +++ b/drivers/mtd/nand/qcom_nandc.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, The Linux Foundation. All rights reserved. + * Copyright (c) 2016-2017, The Linux Foundation. All rights reserved. * * This software is licensed under the terms of the GNU General Public * License version 2, as published by the Free Software Foundation, and @@ -234,6 +234,8 @@ struct nandc_regs { * @cmd1/vld: some fixed controller register values * @ecc_modes: supported ECC modes by the current controller, * initialized via DT match data + * @dma_bam_enabled: flag to tell whether nand controller is using + * bam dma */ struct qcom_nand_controller { struct nand_hw_control controller; @@ -253,6 +255,7 @@ struct qcom_nand_controller { struct list_head desc_list; u8 *data_buffer; + bool dma_bam_enabled; int buf_size; int buf_count; int buf_start; @@ -316,6 +319,17 @@ struct qcom_nand_host { u32 clrreadstatus; }; +/* + * This data type corresponds to the nand driver data which will be used at + * driver probe time + * @ecc_modes - ecc mode for nand + * @dma_bam_enabled - whether this driver is using bam + */ +struct qcom_nand_driver_data { + u32 ecc_modes; + bool dma_bam_enabled; +}; + static inline struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip) { return container_of(chip, struct qcom_nand_host, chip); @@ -2073,6 +2087,7 @@ static int qcom_nandc_probe(struct platform_device *pdev) struct device_node *dn = dev->of_node, *child; struct resource *res; int ret; + const struct qcom_nand_driver_data *driver_data; nandc = devm_kzalloc(&pdev->dev, sizeof(*nandc), GFP_KERNEL); if (!nandc) @@ -2087,7 +2102,10 @@ static int qcom_nandc_probe(struct platform_device *pdev) return -ENODEV; } - nandc->ecc_modes = (unsigned long)dev_data; + driver_data = (const struct qcom_nand_driver_data *)dev_data; + + nandc->ecc_modes = driver_data->ecc_modes; + nandc->dma_bam_enabled = driver_data->dma_bam_enabled; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); nandc->base = devm_ioremap_resource(dev, res); @@ -2179,15 +2197,26 @@ static int qcom_nandc_remove(struct platform_device *pdev) return 0; } -#define EBI2_NANDC_ECC_MODES (ECC_RS_4BIT | ECC_BCH_8BIT) +static const struct qcom_nand_driver_data ebi2_nandc_data = { + .ecc_modes = (ECC_RS_4BIT | ECC_BCH_8BIT), + .dma_bam_enabled = false, +}; + +static const struct qcom_nand_driver_data qpic_nandc_v1_4_0_data = { + .ecc_modes = (ECC_BCH_4BIT | ECC_BCH_8BIT), + .dma_bam_enabled = true, +}; /* * data will hold a struct pointer containing more differences once we support * more controller variants */ static const struct of_device_id qcom_nandc_of_match[] = { { .compatible = "qcom,ipq806x-nand", - .data = (void *)EBI2_NANDC_ECC_MODES, + .data = (void *)&ebi2_nandc_data, + }, + { .compatible = "qcom,qpic-nandc-v1.4.0", + .data = (void *)&qpic_nandc_v1_4_0_data, }, {} };
The current driver only support EBI2 NAND which uses ADM DMA. The latest QCOM controller supports QPIC NAND which uses BAM DMA. NAND registers and programming sequence are same for EBI2 and QPIC NAND so the same driver can support QPIC NAND also by adding the BAM DMA support. This patch adds the QPIC NAND support in current NAND driver with compatible string "qcom,qpic-nandc-v1.4.0" and maps it with different configuration parameter in driver data. Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> --- .../devicetree/bindings/mtd/qcom_nandc.txt | 41 +++++++++++++++++++++- drivers/mtd/nand/qcom_nandc.c | 37 ++++++++++++++++--- 2 files changed, 73 insertions(+), 5 deletions(-)