Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <16684442-35d4-df51-d9f7-4de36d7cf6fd@linaro.org>
Date:   Mon, 20 Jun 2022 12:54:02 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH net-next 01/28] dt-bindings: phy: Add QorIQ SerDes binding
Content-Language: en-US
To:     Sean Anderson <sean.anderson@seco.com>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Madalin Bucur <madalin.bucur@nxp.com>, netdev@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Paolo Abeni <pabeni@redhat.com>,
        Russell King <linux@armlinux.org.uk>,
        Eric Dumazet <edumazet@google.com>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Vinod Koul <vkoul@kernel.org>, devicetree@vger.kernel.org,
        linux-phy@lists.infradead.org
References: <20220617203312.3799646-1-sean.anderson@seco.com>
 <20220617203312.3799646-2-sean.anderson@seco.com>
 <110c4a4b-8007-1826-ee27-02eaedd22d8f@linaro.org>
 <535a0389-6c97-523d-382f-e54d69d3907e@seco.com>
 <d79239ce-3959-15f8-7121-478fc6d432e4@linaro.org>
 <e6ed314d-290f-ace5-b0ff-01a9a2edca88@seco.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <e6ed314d-290f-ace5-b0ff-01a9a2edca88@seco.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 19/06/2022 17:53, Sean Anderson wrote:
>>>
>>>>> +          The first lane in the group. Lanes are numbered based on the register
>>>>> +          offsets, not the I/O ports. This corresponds to the letter-based
>>>>> +          ("Lane A") naming scheme, and not the number-based ("Lane 0") naming
>>>>> +          scheme. On most SoCs, "Lane A" is "Lane 0", but not always.
>>>>> +        minimum: 0
>>>>> +        maximum: 7
>>>>> +      - description: |
>>>>> +          Last lane. For single-lane protocols, this should be the same as the
>>>>> +          first lane.
>>>>> +        minimum: 0
>>>>> +        maximum: 7
>>>>> +
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - fsl,ls1046a-serdes-1
>>>>> +      - fsl,ls1046a-serdes-2
>>>>
>>>> Does not look like proper compatible and your explanation from commit
>>>> msg did not help me. What "1" and "2" stand for? Usually compatibles
>>>> cannot have some arbitrary properties encoded.
>>>
>>> Each serdes has a different set of supported protocols for each lane. This is encoded
>>> in the driver data associated with the compatible
>>
>> Implementation does not matter.
> 
> Of *course* implementation matters. Devicetree bindings do not happen in a vacuum. They
> describe the hardware, but only in service to the implementation.

This is so not true. Bindings do not service implementation. Bindings
happen in vacuum, because they are used by different implementations:
Linux, u-Boot, BSD and several other quite different systems.

Any references to implemention from the bindings is questionable,
although of course not always wrong.

Building bindings per specific implementation is as well usually not
correct.

> 
>>> , along with the appropriate values
>>> to plug into the protocol control registers. Because each serdes has a different set
>>> of supported protocols
>>
>> Another way is to express it with a property.
>>
>>> and register configuration,
>>
>> What does it mean exactly? The same protocols have different programming
>> model on the instances?
> 
> (In the below paragraph, when I say "register" I mean "register or field within a
> register")
> 
> Yes. Every serdes instance has a different way to program protocols into lanes. While
> there is a little bit of orthogonality (the same registers are typically used for the
> same protocols), each serdes is different. The values programmed into the registers are
> unique to the serdes, and the lane which they apply to is also unique (e.g. the same
> register may be used to program a different lane with a different protocol).

That's not answering the point here, but I'll respond to the later
paragraph.

> 
>>> adding support for a new SoC will
>>> require adding the appropriate configuration to the driver, and adding a new compatible
>>> string. Although most of the driver is generic, this critical portion is shared only
>>> between closely-related SoCs (such as variants with differing numbers of cores).
>>>
>>
>> Again implementation - we do not talk here about driver, but the bindings.
>>
>>> The 1 and 2 stand for the number of the SerDes on that SoC. e.g. the documentation will
>>> refer to SerDes1 and SerDes2.
>>>    
>>> So e.g. other compatibles might be
>>>
>>> - fsl,ls1043a-serdes-1 # There's only one serdes on this SoC
>>> - fsl,t4042-serdes-1 # This SoC has four serdes
>>> - fsl,t4042-serdes-2
>>> - fsl,t4042-serdes-3
>>> - fsl,t4042-serdes-4
>>
>> If the devices are really different - there is no common parts in the
>> programming model (registers) - then please find some descriptive
>> compatible. However if the programming model of common part is
>> consistent and the differences are only for different protocols (kind of
>> expected), this should be rather a property describing which protocols
>> are supported.
>>
> 
> I do not want to complicate the driver by attempting to encode such information in the
> bindings. Storing the information in the driver is extremely common. Please refer to e.g.

Yes, quirks are even more common, more flexible and are in general
recommended for more complicated cases. Yet you talk about driver
implementation, which I barely care.

> 
> - mvebu_comphy_cp110_modes in drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> - mvebu_a3700_comphy_modes in drivers/phy/marvell/phy-mvebu-a3700-comphy.c
> - icm_matrix in drivers/phy/xilinx/phy-zynqmp.c
> - samsung_usb2_phy_config in drivers/phy/samsung/

This one is a good example - where do you see there compatibles with
arbitrary numbers attached?

> - qmp_phy_init_tbl in drivers/phy/qualcomm/phy-qcom-qmp.c
> 
> All of these drivers (and there are more)
> 
> - Use a driver-internal struct to encode information specific to different device models.
> - Select that struct based on the compatible

Driver implementation. You can do it in many different ways. Does not
matter for the bindings.

> 
> The other thing is that while the LS1046A SerDes are fairly generic, other SerDes of this
> type have particular restructions on the clocks. E.g. on some SoCs, certain protocols
> cannot be used together (even if they would otherwise be legal), and some protocols must
> use particular PLLs (whereas in general there is no such restriction). There are also
> some register fields which are required to program on some SoCs, and which are reserved
> on others.

Just to be clear, because you are quite unspecific here ("some
protocols") - we talk about the same protocol programmed on two of these
serdes (serdes-1 and serdes-2 how you call it). Does it use different
registers? Are some registers - for the same protocol - reserved in one
version?

> 
> There is, frankly, a large amount of variation between devices as implemented on different
> SoCs. 

This I don't get. You mean different SoCs have entirely different
Serdes? Sure, no problem. We talk here only about this SoC, this
serdes-1 and serdes-2.

> Especially because (AIUI) drivers must remain compatible with old devicetrees, I
> think using a specific compatible string is especially appropriate here. 

This argument does not make any sense in case of new bindings and new
drivers, unless you build on top of existing implementation. Anyway no
one asks you to break existing bindings...

> It will give us
> the ability to correct any implementation quirks as they are discovered (and I anticipate
> that there will be) rather than having to determine everything up front.

All the quirks can be also chosen by respective properties.

Anyway, "serdes-1" and "serdes-2" are not correct compatibles, so my NAK
stays. These might be separate compatibles, although that would require
proper naming and proper justification (as you did not answer my actual
questions about differences when using same protocols). Judging by the
bindings and your current description (implementation does not matter),
this also looks like a property.


Best regards,
Krzysztof