Message-ID: <e922714b-29c7-0f41-9e5c-9a0aef9fb5de@linaro.org>
Date:   Mon, 20 Jun 2022 20:21:19 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Sean Anderson <sean.anderson@...o.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....com>, netdev@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        Eric Dumazet <edumazet@...gle.com>,
        Kishon Vijay Abraham I <kishon@...com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Vinod Koul <vkoul@...nel.org>, devicetree@...r.kernel.org,
        linux-phy@...ts.infradead.org
Subject: Re: [PATCH net-next 01/28] dt-bindings: phy: Add QorIQ SerDes binding

On 20/06/2022 19:19, Sean Anderson wrote:
> 
> 
> On 6/20/22 6:54 AM, Krzysztof Kozlowski wrote:
>> 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
> 
> Where are all the bindings for hardware without drivers?
> 
> Why don't device trees describe the entire hardware before any drivers are written?
> 
> Actually, I have seen some device trees written like that (baked into the chip's ROM),
> and they cannot be used because the bindings
> 
> - Do not fully describe the hardware (e.g. clocks, resets, interrupts, and other things)
> - Do not describe the hardware in a compatible way (e.g. using different names for
>   registers and clocks, or ordering fields differently).
> - Contain typos and errors (since they were never used)
> 
> These same issues apply to any new binding documentation. Claiming that bindings happen
> in a vacuum is de facto untrue, and would be unsound practice if it wasn't.
> 
>> because they are used by different implementations:
>> Linux, u-Boot, BSD and several other quite different systems.
> 
> U-Boot doesn't use devicetree for this device (and if it did the port would likely be
> based on the Linux driver). BSD doesn't support this hardware at all. We are the first
> to create a driver for this device, so we get to choose the binding.

You choose the binding in respect to the guidelines. You cannot choose
some random weirdness just because you are first. It does not matter
that there are no other implementations in that way - you must choose
reasonable bindings.

> 
>> 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.
> 
> Sure, but there are of course many ways to create bindings, even for the same hardware.
> As an example, pinctrl bindings can be written like
> 
> pinctrl@...ebabe {
> 	uart-tx {
> 		function = "uart-tx";
> 		pins = "5";
> 	};
> };
> 
> or
> 
> pinctrl@...dbeef {
> 	uart-tx {
> 		pinmux = <SOME_MACRO(5, UART_TX)>;
> 	};
> };
> 
> or
> 
> pinctrl@...dfeed {
> 	uart-tx {
> 		pinmux = <SOME_MACRO(5, FUNC3)>;
> 	};
> };
> 
> and which one to use depends both on the structure of the hardware, as well as the
> driver. These bindings require a different driver style under the hood, and using
> the wrong binding can unnecessarily complicate the driver for no reason.
> 
> To further beat home the point, someone might use a "fixed-clock" to describe a clock
> and then later change to a more detailed implementation. They could use "simple-pinctrl"
> and then later move to a device-specific driver. 
> 
> If the devicetree author is smart, then they will create a binding like
> 
> clock {
> 	compatible = "vendor,my-clock", "fixed-clock";
> 	...
> };
> 
> so that better support might be added in the future. In fact, that is *exactly* what I
> am suggesting here.
> 
>>>
>>>>> , 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?
> 
> samsung_usb2_phy_of_match in drivers/phy/samsung/phy-samsung-usb2.c
> 
> There is a different compatible for each SoC variant. Each compatible selects a struct
> containing
> 
> - A list of phys, each with custom power on and off functions
> - A function which converts a rate to an arbitrary value to program into a register
> 
> This is further documented in Documentation/driver-api/phy/samsung-usb2.rst

Exactly, please follow this approach. Compatible is per different
device, e.g. different SoC variant. Of course you could have different
devices on same SoC, but "1" and "2" are not different devices.

> 
>>> - 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.
> 
> And because this both describes the hardware and is convenient to the implementation,
> I have chosen this way.
> 
>>>
>>> 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?
> 
> Yes.
> 
>> Are some registers - for the same protocol - reserved in one version?
> 
> Yes.
> 
> For example, I excerpt part of the documentation for PCCR2 on the T4240:
> 
>> XFIa Configuration:
>> XFIA_CFG Default value set by RCW configuration.
>> This field must be 0 for SerDes 3 & 4
>> All settings not shown are reserved
>>
>> 00 Disabled
>> 01 x1 on Lane 3 to FM2 MAC 9
> 
> And here is part of the documentation for PCCR2 on the LS1046A:
> 
>> SATAa Configuration
>> All others reserved
>> NOTE: This field is not supported in every instance. The following table includes only
>>       supported registers.
>> Field supported in	Field not supported in
>> SerDes1_PCCR2		—
>> —			SerDes2_PCCR2
>>
>> 000b - Disabled
>> 001b - x1 on Lane 3 (SerDes #2 only)
> 
> And here is part of the documentation for PCCRB on the LS1046A:
> 
>> XFIa Configuration
>> All others reserved Default value set by RCW configuration.
>>
>> 000b - Disabled
>> 010b - x1 on Lane 1 to XGMIIa (Serdes #1 only)
> You may notice that
> 
> - For some SerDes on the same SoC, these fields are reserved

That all sounds like quite different devices, which indeed usually is
described with different compatibles. Still "xxx-1" and "xxx-2" are not
valid compatibles. You need to come with some more reasonable name
describing them. Maybe the block has revision or different model/vendor.

> - Between different SoCs, different protocols may be configured in different registers
> - The same registers may be used for different protocols in different SoCs (though
>   generally there are several general layouts)

Different SoCs give you different compatibles, so problem is solved and
that's not exactly argument for this case.

> - Fields have particular values which must be programmed
> 
> In addition, the documentation also says
> 
>> Reserved registers and fields must be preserved on writes.
> 
> All of these combined issues make it so that we need detailed, serdes-specific
> configuration. The easiest way to store this configuration is in the driver. This
> is consistent with *many* existing phy implementations. I would like to write a
> standard phy driver, not one twisted by unusual device tree requirements.

Sure.

> 
>>>
>>> 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...
> 
> When there is a bug in the bindings how do you fix it? If I were to follow your suggested method, it would be difficult to determine the particular devices
> 
>>> 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.
> 
> Quirks are *exactly* the sort of implementation-specific details that you were opposed to above.
> 
>> Anyway, "serdes-1" and "serdes-2" are not correct compatibles,
> 
> The compatibles suggested were "fsl,ls1046-serdes-1" and -2. As noted above, these are separate
> devices which, while having many similarities, have different register layouts and protocol
> support. They are *not* 100% compatible with each other. Would you require that clock drivers
> for different SoCs use the same compatibles just because they had the same registers, even though
> the clocks themselves had different functions and hierarchy?

You miss the point. Clock controllers on same SoC have different names
used in compatibles. We do not describe them as "vendor,aa-clk-1" and
"vendor,aa-clk-2".

Come with proper naming and entire discussion might be not valid
(although with not perfect naming Rob might come with questions). I
cannot propose the name because I don't know these hardware blocks and I
do not have access to datasheet.

Other way, if any reasonable naming is not possible, could be also to
describe the meaning of "-1" suffix, e.g. that it does not mean some
index but a variant from specification.

> 
> --Sean
> 
>> 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

Powered by blists - more mailing lists