Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <f1a1084f-ecf3-35dd-7822-a6b5931132a3@quicinc.com>
Date:   Mon, 20 Nov 2023 12:00:06 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [RFC PATCH 1/5] mtd: nand: ecc-qcom: Add support for ECC Engine
 Driver
To:     Miquel Raynal <miquel.raynal@bootlin.com>
CC:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <robh+dt@kernel.org>,
        <conor+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <richard@nod.at>, <vigneshr@ti.com>, <broonie@kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-mtd@lists.infradead.org>,
        <linux-spi@vger.kernel.org>, <quic_srichara@quicinc.com>,
        <qpic_varada@quicinc.com>
References: <20231031120307.1600689-1-quic_mdalam@quicinc.com>
 <20231031120307.1600689-2-quic_mdalam@quicinc.com>
 <b9af01d2-1a86-4b41-9bd6-3bf7a0dde1c0@linaro.org>
 <553c1373-c9a0-b2af-2286-058824e31bad@quicinc.com>
 <CAA8EJpp-xsP1x==a5DH8pKpy7XH75UF-L8ewKWmeL8ePtxUq-A@mail.gmail.com>
 <4b911907-44b9-c164-9648-3d399e557672@quicinc.com>
 <20231103144639.6ff40ae2@xps-13>
Content-Language: en-US
From:   Md Sadre Alam <quic_mdalam@quicinc.com>
In-Reply-To: <20231103144639.6ff40ae2@xps-13>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 11/3/2023 7:16 PM, Miquel Raynal wrote:
> Hello,
> 
>> Based on below feedback [1] and NAK on the device node patch
>> got idea of having separate device node for ECC is not acceptable.
>> Could you please help to clarify that.
> 
> If I may try to compare with the Macronix situation, the ECC engine
> was an independent hardware block, with its own mapping and its own
> registers, so it was described as an independent node in the DT. The
> type of ECC controller (pipelined or external) is described by the
> nand-ecc-engine property which either points at the parent node
> (pipelined) or an external node (external). The SPI host would itself
> point at the external ECC engine node with its own nand-ecc-engine
> property (see mtd/mxicy,nand-ecc-engine.yaml in the bindings).

Sorry for late reply.

Since QPIC controller ECC engine is not a separate HW block. To control ECC
functionality there is only one register 4-bytes long.As you suggested above,
ECC controller described by the property nand-ecc-engine.I have checked
mtd/mxicy,nand-ecc-engine.yaml file and got to know I can use like
nand-ecc-engine = <&qpic_nand>; in dts.Now additional ECC node not needed
in DTS. Will clean up everything in next patch.

> 
>> Since ECC block is inlined with QPIC controller so is the below
>> device node acceptable ?
>>
>>      bch: qpic_ecc {
>>                             compatible = "qcom,ipq9574-ecc";
>>                             status = "ok";
>>                     };
> 
> If it does not has its own mapping and if you access the ECC engine
> through the host registers then the controller should be part of the
> host node, but I am not sure it really needs to be described
> explicitly, most of them are not for historical reasons. Conceptually
> there is a problem with subnodes of each of these controllers having
> a signification already: SPI devices or NAND chips.

New device node for spi nand looks like as below.

&qpic_nand {
          status = "okay";
          pinctrl-0 = <&qspi_nand_pins>;
          pinctrl-names = "default";
          spi_nand: spi_nand@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>;
                  spi-max-frequency = <8000000>;
          };
};

With the above device node I have tested the spi nand device enumeration
its working fine. Will cleanup everything and post in next patch.


> 
> Thanks,
> Miquèl