| Message ID | 20221205065756.26875-1-xiangsheng.hou@mediatek.com |
|---|---|
| Headers | show |
| Series | Add MediaTek MT7986 SPI NAND and ECC support | expand |
On 05/12/2022 07:57, Xiangsheng Hou wrote: > 1. Split MediaTek ECC engine with rawnand controller and convert to > YAML schema. > 2. Change the existing node name in order to match NAND controller DT > bindings. One patch - one logical change. Not two. This applies to all your patches, so whenever you want to enumerate, please think twice. > > Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> > --- > .../bindings/mtd/mediatek,mtk-nfc.yaml | 171 +++++++++++++++++ > .../mtd/mediatek,nand-ecc-engine.yaml | 62 ++++++ > .../devicetree/bindings/mtd/mtk-nand.txt | 176 ------------------ > arch/arm/boot/dts/mt2701.dtsi | 2 +- > arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 2 +- > arch/arm64/boot/dts/mediatek/mt7622.dtsi | 2 +- Do not combine bindings and DTS. > 6 files changed, 236 insertions(+), 179 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mtd/mediatek,mtk-nfc.yaml > create mode 100644 Documentation/devicetree/bindings/mtd/mediatek,nand-ecc-engine.yaml > delete mode 100644 Documentation/devicetree/bindings/mtd/mtk-nand.txt > > diff --git a/Documentation/devicetree/bindings/mtd/mediatek,mtk-nfc.yaml b/Documentation/devicetree/bindings/mtd/mediatek,mtk-nfc.yaml > new file mode 100644 > index 000000000000..2b1c92edc9d0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/mediatek,mtk-nfc.yaml > @@ -0,0 +1,171 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mtd/mediatek,mtk-nfc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MediaTek(MTK) SoCs raw NAND FLASH controller (NFC) > + > +maintainers: > + - Xiangsheng Hou <xiangsheng.hou@mediatek.com> > + > +properties: > + compatible: > + enum: > + - mediatek,mt2701-nfc > + - mediatek,mt2712-nfc > + - mediatek,mt7622-nfc > + > + reg: > + items: > + - description: Base physical address and size of NFI. > + > + interrupts: > + items: > + - description: NFI interrupt > + > + clocks: > + items: > + - description: clock used for the controller > + - description: clock used for the pad > + > + clock-names: > + items: > + - const: nfi_clk > + - const: pad_clk > + > + ecc-engine: true I don't think this could be anything. You need to describe it, so $ref and description. > + > + partitions: > + $ref: mtd.yaml# How the partitions are MTD device? Open that file and see how it should be defined... Anyway mtd.yaml is part of nand-chip, not nand-controller. > + > +allOf: > + - $ref: nand-controller.yaml# > + > + - if: > + properties: > + compatible: > + contains: > + const: mediatek,mt2701-nfc > + then: > + patternProperties: > + "^nand@[a-f0-9]$": > + type: object No need for type, the definition is already there through nand-controller.yaml. > + properties: > + reg: > + minimum: 0 > + maximum: 1 This is the same as other variant, so should be defined in top-level pattern properties. > + nand-ecc-mode: > + const: hw Ditto > + nand-ecc-step-size: > + enum: [ 512, 1024 ]> + nand-ecc-strength: > + enum: [4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36, > + 40, 44, 48, 52, 56, 60] > + > + - if: > + properties: > + compatible: > + contains: > + const: mediatek,mt2712-nfc > + then: > + patternProperties: > + "^nand@[a-f0-9]$": > + type: object > + properties: > + reg: > + minimum: 0 > + maximum: 1 > + nand-ecc-mode: > + const: hw > + nand-ecc-step-size: > + enum: [ 512, 1024 ] > + nand-ecc-strength: > + enum: [4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36, > + 40, 44, 48, 52, 56, 60, 68, 72, 80] > + > + - if: > + properties: > + compatible: > + contains: > + const: mediatek,mt7622-nfc > + then: > + patternProperties: > + "^nand@[a-f0-9]$": > + type: object > + properties: > + reg: > + minimum: 0 > + maximum: 1 > + nand-ecc-mode: > + const: hw > + nand-ecc-step-size: > + const: 512 > + nand-ecc-strength: > + enum: [4, 6, 8, 10, 12] > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - ecc-engine > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/mt2701-clk.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + nand-controller@1100d000 { > + compatible = "mediatek,mt2701-nfc"; > + reg = <0 0x1100d000 0 0x1000>; > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_LOW>; > + clocks = <&pericfg CLK_PERI_NFI>, > + <&pericfg CLK_PERI_NFI_PAD>; > + clock-names = "nfi_clk", "pad_clk"; > + ecc-engine = <&bch>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + nand@0 { > + reg = <0>; > + > + nand-on-flash-bbt; > + nand-ecc-mode = "hw"; > + nand-ecc-step-size = <1024>; > + nand-ecc-strength = <24>; > + > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + preloader@0 { > + label = "pl"; > + read-only; > + reg = <0x0 0x400000>; > + }; > + android@400000 { > + label = "android"; > + reg = <0x400000 0x12c00000>; > + }; > + }; > + }; > + }; > + > + bch: ecc@1100e000 { > + compatible = "mediatek,mt2701-ecc"; > + reg = <0 0x1100e000 0 0x1000>; > + interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_LOW>; > + clocks = <&pericfg CLK_PERI_NFI_ECC>; > + clock-names = "nfiecc_clk"; You already have example of ecc in other binding, so drop from this one. > + }; > + }; > diff --git a/Documentation/devicetree/bindings/mtd/mediatek,nand-ecc-engine.yaml b/Documentation/devicetree/bindings/mtd/mediatek,nand-ecc-engine.yaml > new file mode 100644 > index 000000000000..b13d801eda76 > --- /dev/null Best regards, Krzysztof
On 05/12/2022 07:57, Xiangsheng Hou wrote: > Add dt-bindings documentation of ECC for MediaTek MT7986 SoC > platform. > Now your subject prefix does not match file. Filename is "mediatek,nand-ecc-engine." so use it instead of "ecc-mtk". Best regards, Krzysztof
Il 05/12/22 07:57, Xiangsheng Hou ha scritto: > Add ECC support fot MT7986 IC. > > Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> > --- > drivers/mtd/nand/ecc-mtk.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/mtd/nand/ecc-mtk.c b/drivers/mtd/nand/ecc-mtk.c > index 9f9b201fe706..c2f6cfa76a04 100644 > --- a/drivers/mtd/nand/ecc-mtk.c > +++ b/drivers/mtd/nand/ecc-mtk.c > @@ -79,6 +79,10 @@ static const u8 ecc_strength_mt7622[] = { > 4, 6, 8, 10, 12 > }; > > +static const u8 ecc_strength_mt7986[] = { > + 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24 > +}; > + > enum mtk_ecc_regs { > ECC_ENCPAR00, > ECC_ENCIRQ_EN, > @@ -483,6 +487,17 @@ static const struct mtk_ecc_caps mtk_ecc_caps_mt7622 = { > .pg_irq_sel = 0, > }; > > +static const struct mtk_ecc_caps mtk_ecc_caps_mt7986 = { > + .err_mask = 0x1f, Can't we use GENMASK() to define err_mask instead? #define MT7986_ERRNUM GENMASK(4, 0) P.S.: Did I get that right? Is that referred to the ERRNUM(x) bits? Regards, Angelo
Il 05/12/22 07:57, Xiangsheng Hou ha scritto: > Add optional nfi_hclk which needed for MT7986. > > Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> Is there any operation for which you need NFI_HCLK enabled, but at the same time PAD_CLK and/or NFI_CLK can be disabled? If NFI_HCLK and NFI_CLK must always be ON at the same time, adding this clock to spi-mtk-snfi.c is *not* an optimal way of doing things: you can, at this point, set NFI_HCLK as parent of NFI_CLK in the MT7986 clock driver instead, without making any addition to this driver at all. Regards, Angelo
Il 05/12/22 07:57, Xiangsheng Hou ha scritto: > Add snfi support for MT7986 IC. > > Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Hi Angelo, On Mon, 2022-12-05 at 15:21 +0100, AngeloGioacchino Del Regno wrote: > Il 05/12/22 07:57, Xiangsheng Hou ha scritto: > > Add ECC support fot MT7986 IC. > > > > Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> > > --- > > drivers/mtd/nand/ecc-mtk.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/drivers/mtd/nand/ecc-mtk.c b/drivers/mtd/nand/ecc- > > mtk.c > > index 9f9b201fe706..c2f6cfa76a04 100644 > > --- a/drivers/mtd/nand/ecc-mtk.c > > +++ b/drivers/mtd/nand/ecc-mtk.c > > @@ -79,6 +79,10 @@ static const u8 ecc_strength_mt7622[] = { > > 4, 6, 8, 10, 12 > > }; > > > > +static const u8 ecc_strength_mt7986[] = { > > + 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24 > > +}; > > + > > enum mtk_ecc_regs { > > ECC_ENCPAR00, > > ECC_ENCIRQ_EN, > > @@ -483,6 +487,17 @@ static const struct mtk_ecc_caps > > mtk_ecc_caps_mt7622 = { > > .pg_irq_sel = 0, > > }; > > > > +static const struct mtk_ecc_caps mtk_ecc_caps_mt7986 = { > > + .err_mask = 0x1f, > > Can't we use GENMASK() to define err_mask instead? > > #define MT7986_ERRNUM GENMASK(4, 0) > > P.S.: Did I get that right? Is that referred to the ERRNUM(x) bits Yes, you are right. I will change like #define ECC_ERRMASK(x) GENMASK(x, 0), since other IC driver data will use 0x3f and 0x7f err_mask. Thanks Xiangsheng Hou
Hi Krzysztof, On Mon, 2022-12-05 at 10:21 +0100, Krzysztof Kozlowski wrote: > On 05/12/2022 07:57, Xiangsheng Hou wrote: > > 1. Split MediaTek ECC engine with rawnand controller and convert to > > YAML schema. > > 2. Change the existing node name in order to match NAND controller > > DT > > bindings. > > One patch - one logical change. Not two. This applies to all your > patches, so whenever you want to enumerate, please think twice. Will be corrected in next series. > > > > > Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> > > --- > > .../bindings/mtd/mediatek,mtk-nfc.yaml | 171 > > +++++++++++++++++ > > .../mtd/mediatek,nand-ecc-engine.yaml | 62 ++++++ > > .../devicetree/bindings/mtd/mtk-nand.txt | 176 -------------- > > ---- > > arch/arm/boot/dts/mt2701.dtsi | 2 +- > > arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 2 +- > > arch/arm64/boot/dts/mediatek/mt7622.dtsi | 2 +- > > Do not combine bindings and DTS. The DTS modification will be separated. > > > > + > > + ecc-engine: true > > I don't think this could be anything. You need to describe it, so > $ref > and description. Will do. > > + > > + partitions: > > + $ref: mtd.yaml# > > How the partitions are MTD device? Open that file and see how it > should > be defined... Anyway mtd.yaml is part of nand-chip, not nand- > controller. This will be dropped in next series since nand-chip is part of nand- controller. > > + > > +allOf: > > + - $ref: nand-controller.yaml# > > + > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: mediatek,mt2701-nfc > > + then: > > + patternProperties: > > + "^nand@[a-f0-9]$": > > + type: object > > No need for type, the definition is already there through > nand-controller.yaml. > > > + properties: > > + reg: > > + minimum: 0 > > + maximum: 1 > > This is the same as other variant, so should be defined in top-level > pattern properties. > > > + nand-ecc-mode: > > + const: hw > > Ditto Will be fixed in next series. Thanks Xiangsheng Hou
Il 06/12/22 10:04, Xiangsheng Hou (侯祥胜) ha scritto: > Hi Angelo, > > On Mon, 2022-12-05 at 15:21 +0100, AngeloGioacchino Del Regno wrote: >> Il 05/12/22 07:57, Xiangsheng Hou ha scritto: >>> Add ECC support fot MT7986 IC. >>> >>> Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> >>> --- >>> drivers/mtd/nand/ecc-mtk.c | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git a/drivers/mtd/nand/ecc-mtk.c b/drivers/mtd/nand/ecc- >>> mtk.c >>> index 9f9b201fe706..c2f6cfa76a04 100644 >>> --- a/drivers/mtd/nand/ecc-mtk.c >>> +++ b/drivers/mtd/nand/ecc-mtk.c >>> @@ -79,6 +79,10 @@ static const u8 ecc_strength_mt7622[] = { >>> 4, 6, 8, 10, 12 >>> }; >>> >>> +static const u8 ecc_strength_mt7986[] = { >>> + 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24 >>> +}; >>> + >>> enum mtk_ecc_regs { >>> ECC_ENCPAR00, >>> ECC_ENCIRQ_EN, >>> @@ -483,6 +487,17 @@ static const struct mtk_ecc_caps >>> mtk_ecc_caps_mt7622 = { >>> .pg_irq_sel = 0, >>> }; >>> >>> +static const struct mtk_ecc_caps mtk_ecc_caps_mt7986 = { >>> + .err_mask = 0x1f, >> >> Can't we use GENMASK() to define err_mask instead? >> >> #define MT7986_ERRNUM GENMASK(4, 0) >> >> P.S.: Did I get that right? Is that referred to the ERRNUM(x) bits > > Yes, you are right. > I will change like > #define ECC_ERRMASK(x) GENMASK(x, 0), > since other IC driver data will use 0x3f and 0x7f err_mask. > I would prefer, instead, something like #define MT7986_ERRNUM GENMASK(....) #define MT7622_ERRNUM GENMASK(....) #define MT.... (etc) instead of a macro calling another macro. Regards, Angelo
On Mon, 5 Dec 2022 14:57:47 +0800, Xiangsheng Hou wrote: > This patch series add MediaTek MT7986 SPI NAND and ECC controller > support, split ECC engine with rawnand controller in bindings and > hange to YAML schema. > > Changes since V1: > - Use existing sample delay property. > - Add restricting for optional nfi_hclk. > - Improve and perfect dt-bindings documentation. > - Change existing node name to match NAND controller DT bingings. > - Fix issues reported by dt_binding_check. > - Fix issues reported by dtbs_check. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/9] spi: mtk-snfi: Add snfi support for MT7986 IC commit: 7073888c86601389e17f3ee8ab15ab7aef148839 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi Angelo, On Mon, 2022-12-05 at 15:21 +0100, AngeloGioacchino Del Regno wrote: > Il 05/12/22 07:57, Xiangsheng Hou ha scritto: > > Add optional nfi_hclk which needed for MT7986. > > > > Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> > > Is there any operation for which you need NFI_HCLK enabled, but at > the same time > PAD_CLK and/or NFI_CLK can be disabled? No, for the new IP design on MT7986, will need the PAD_CLK/NFI_CLK/NFI_HCLK enabled at the same time. > If NFI_HCLK and NFI_CLK must always be ON at the same time, adding > this clock to > spi-mtk-snfi.c is *not* an optimal way of doing things: you can, at > this point, > set NFI_HCLK as parent of NFI_CLK in the MT7986 clock driver instead, > without > making any addition to this driver at all. For some IC, there may have only NFI_CLK/PAD_CLK, and have no NFI_HCLK, this rely on IC design. Thanks Xiangsheng Hou
On Tue, 2022-12-06 at 13:22 +0100, AngeloGioacchino Del Regno wrote: > Il 06/12/22 10:04, Xiangsheng Hou (侯祥胜) ha scritto: > > Hi Angelo, > > > > On Mon, 2022-12-05 at 15:21 +0100, AngeloGioacchino Del Regno > > wrote: > > > Il 05/12/22 07:57, Xiangsheng Hou ha scritto: > > > > Add ECC support fot MT7986 IC. > > > > > > > > Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> > > > > --- > > > > drivers/mtd/nand/ecc-mtk.c | 18 ++++++++++++++++++ > > > > 1 file changed, 18 insertions(+) > > > > > > > > diff --git a/drivers/mtd/nand/ecc-mtk.c b/drivers/mtd/nand/ecc- > > > > mtk.c > > > > index 9f9b201fe706..c2f6cfa76a04 100644 > > > > --- a/drivers/mtd/nand/ecc-mtk.c > > > > +++ b/drivers/mtd/nand/ecc-mtk.c > > > > @@ -79,6 +79,10 @@ static const u8 ecc_strength_mt7622[] = { > > > > 4, 6, 8, 10, 12 > > > > }; > > > > > > > > +static const u8 ecc_strength_mt7986[] = { > > > > + 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24 > > > > +}; > > > > + > > > > enum mtk_ecc_regs { > > > > ECC_ENCPAR00, > > > > ECC_ENCIRQ_EN, > > > > @@ -483,6 +487,17 @@ static const struct mtk_ecc_caps > > > > mtk_ecc_caps_mt7622 = { > > > > .pg_irq_sel = 0, > > > > }; > > > > > > > > +static const struct mtk_ecc_caps mtk_ecc_caps_mt7986 = { > > > > + .err_mask = 0x1f, > > > > > > Can't we use GENMASK() to define err_mask instead? > > > > > > #define MT7986_ERRNUM GENMASK(4, 0) > > > > > > P.S.: Did I get that right? Is that referred to the ERRNUM(x) > > > bits > > > > Yes, you are right. > > I will change like > > #define ECC_ERRMASK(x) GENMASK(x, 0), > > since other IC driver data will use 0x3f and 0x7f err_mask. > > > > I would prefer, instead, something like > > #define MT7986_ERRNUM GENMASK(....) > #define MT7622_ERRNUM GENMASK(....) > #define MT.... (etc) > > instead of a macro calling another macro. Will do. Thanks Xiangsheng Hou
Il 07/12/22 02:42, Xiangsheng Hou (侯祥胜) ha scritto: > Hi Angelo, > > On Mon, 2022-12-05 at 15:21 +0100, AngeloGioacchino Del Regno wrote: >> Il 05/12/22 07:57, Xiangsheng Hou ha scritto: >>> Add optional nfi_hclk which needed for MT7986. >>> >>> Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> >> >> Is there any operation for which you need NFI_HCLK enabled, but at >> the same time >> PAD_CLK and/or NFI_CLK can be disabled? > > No, for the new IP design on MT7986, will need the > PAD_CLK/NFI_CLK/NFI_HCLK enabled at the same time. > >> If NFI_HCLK and NFI_CLK must always be ON at the same time, adding >> this clock to >> spi-mtk-snfi.c is *not* an optimal way of doing things: you can, at >> this point, >> set NFI_HCLK as parent of NFI_CLK in the MT7986 clock driver instead, >> without >> making any addition to this driver at all. > > For some IC, there may have only NFI_CLK/PAD_CLK, and have no NFI_HCLK, > this rely on IC design. > I've just checked clk-mt7986-infracfg and we can't reparent NFI1_CK, nor SPINFI1_CK as they have xxxx_sel parents already, which are not common with the HCK. You're right, the addition of the nfi_hclk clock is needed, which means that for this commit, you get my Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> P.S.: Thanks for clarifying! Regards, Angelo