Message-ID: <ef0e2cba-a060-0495-98ac-7a78b480281e@exceet.de>
Date:   Mon, 16 Jul 2018 09:04:12 +0200
From:   Frieder Schrempf <frieder.schrempf@...eet.de>
To:     Rob Herring <robh@...nel.org>
Cc:     MTD Maling List <linux-mtd@...ts.infradead.org>,
        Boris Brezillon <boris.brezillon@...tlin.com>,
        linux-spi <linux-spi@...r.kernel.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Marek Vašut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        Miquèl Raynal <miquel.raynal@...tlin.com>,
        Mark Brown <broonie@...nel.org>, david.wolfe@....com,
        Fabio Estevam <fabio.estevam@....com>,
        Prabhakar Kushwaha <prabhakar.kushwaha@....com>,
        Yogesh Gaur <yogeshnarayan.gaur@....com>,
        Han Xu <han.xu@....com>, Shawn Guo <shawnguo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 05/12] dt-bindings: spi: Adjust the bindings for the
 FSL QSPI driver

Hi Rob,

On 12.07.2018 17:20, Rob Herring wrote:
> On Thu, Jul 12, 2018 at 2:14 AM Frieder Schrempf
> <frieder.schrempf@...eet.de> wrote:
>>
>> Hi Rob,
>>
>> On 11.07.2018 18:05, Rob Herring wrote:
>>> On Thu, Jul 05, 2018 at 01:15:01PM +0200, Frieder Schrempf wrote:
>>>> Adjust the documentation of the new SPI memory interface based
>>>> driver to reflect the new drivers settings.
>>>
>>> Bindings shouldn't change (other than new properties) due to driver
>>> changes.
>>
>> Right, I added an explanation below, why I think the changes are necessary.
>>
>>>
>>>>
>>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@...eet.de>
>>>> ---
>>>> Changes in v2:
>>>> ==============
>>>> * Split the moving and editing of the dt-bindings in two patches
>>>>
>>>>    .../devicetree/bindings/spi/spi-fsl-qspi.txt    | 22 ++++++++++----------
>>>>    1 file changed, 11 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-qspi.txt b/Documentation/devicetree/bindings/spi/spi-fsl-qspi.txt
>>>> index 483e9cf..8b4eed7 100644
>>>> --- a/Documentation/devicetree/bindings/spi/spi-fsl-qspi.txt
>>>> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-qspi.txt
>>>> @@ -3,9 +3,8 @@
>>>>    Required properties:
>>>>      - compatible : Should be "fsl,vf610-qspi", "fsl,imx6sx-qspi",
>>>>                "fsl,imx7d-qspi", "fsl,imx6ul-qspi",
>>>> -             "fsl,ls1021a-qspi"
>>>> +             "fsl,ls1021a-qspi", "fsl,ls2080a-qspi"
>>>>                or
>>>> -             "fsl,ls2080a-qspi" followed by "fsl,ls1021a-qspi",
>>>>                "fsl,ls1043a-qspi" followed by "fsl,ls1021a-qspi"
>>>
>>> So the 2080a h/w was compatible with the 1021a h/w, but now it is not?
>>> How did the h/w change?
>>
>> I guess this should be posted as a separate fix. Formerly there was only
>> "fsl,ls1021a-qspi" handled in the driver and the bindings here claimed
>> that "fsl,ls2080a-qspi" is compatible.
>>
>> Some time ago a separate entry for "fsl,ls2080a-qspi" was added to the
>> driver [1] and it adds a quirk, that is not set for "fsl,ls1021a-qspi".
>> That's why I concluded, that these two are actually not compatible.
> 
> So before the driver change, the driver didn't work at all on the
> ls2080a? If so, then removing is appropriate. If not, then removing
> breaks all kernel versions before the change if you use a newer DT.

Before the driver change it couldn't work fully on the ls2080a, as the 
quirk was missing. At least that's what I deduce from the driver.

> 
> 
>>>>      - reg : the first contains the register location and length,
>>>>              the second contains the memory mapping address and length
>>>> @@ -15,14 +14,15 @@ Required properties:
>>>>      - clock-names : Should contain the name of the clocks: "qspi_en" and "qspi".
>>>>
>>>>    Optional properties:
>>>> -  - fsl,qspi-has-second-chip: The controller has two buses, bus A and bus B.
>>>> -                              Each bus can be connected with two NOR flashes.
>>>> -                          Most of the time, each bus only has one NOR flash
>>>> -                          connected, this is the default case.
>>>> -                          But if there are two NOR flashes connected to the
>>>> -                          bus, you should enable this property.
>>>> -                          (Please check the board's schematic.)
>>>
>>> You can't just remove properties without explanation. Why is this no
>>> longer needed? What about backwards compatibility with existing dtbs?
>>
>> You're right, the explanation is missing here.
>>
>> The "old" driver was using this property to select one of two dual chip
>> setups (two chips on one bus or two chips on separate buses). And it
>> used the order in which the subnodes are defined in the dt to select the
>> CS, the chip is connected to.
>>
>> Both methods are wrong and in fact the "reg" property should be used to
>> determine which bus and CS a chip is connected to. This also enables us
>> to use different setups than just single chip, or symmetric dual chip.
>>
>> So the porting of the driver from the MTD to the SPI framework actually
>> enforces the use of the "reg" properties and makes
>> "fsl,qspi-has-second-chip" superfluous.
>>
>> As all boards that have "fsl,qspi-has-second-chip" set, also have
>> correct "reg" properties, the removal of this property shouldn't lead to
>> any incompatibilities.
>>
>> The only compatibility issues I can see are with imx6sx-sdb.dts and
>> imx6sx-sdb-reva.dts, which have their reg properties set incorrectly
>> (see explanation here: [2]), all other boards should stay compatible.
> 
> Add this to the commit msg.

Ok.

> 
>>>> -  - big-endian : That means the IP register is big endian
>>>> +  - big-endian : That means the IP registers format is big endian
>>>
>>> This is a standard property so it doesn't really need to be redefined
>>> here, but just reference the definition.
>>
>> So I will change that to:
>>
>>          big-endian : See common-properties.txt for a definition
> 
> You can drop "for a definition"

Ok.

Thanks,
Frieder

Powered by blists - more mailing lists