Message-ID: <fbe88d1e-6ffb-4806-8210-fa83df0287a5@gmail.com>
Date: Sun, 5 Jan 2025 12:51:50 +0200
From: Ivaylo Ivanov <ivo.ivanov.ivanov1@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Alim Akhtar <alim.akhtar@...sung.com>,
 Sam Protsenko <semen.protsenko@...aro.org>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] dt-bindings: soc: samsung: usi: add USIv1 and
 samsung,exynos8895-usi

On 1/5/25 12:39, Krzysztof Kozlowski wrote:
> On 05/01/2025 10:51, Ivaylo Ivanov wrote:
>> On 1/5/25 11:18, Krzysztof Kozlowski wrote:
>>> On Sat, Jan 04, 2025 at 06:29:14PM +0200, Ivaylo Ivanov wrote:
>>>>  
>>>>    reg:
>>>>      maxItems: 1
>>>> @@ -64,7 +75,6 @@ properties:
>>>>  
>>>>    samsung,mode:
>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>> -    enum: [0, 1, 2, 3]
>>> Widest constraints stay here, so minimum/maximum.
>> Why?
> Because that's the coding style and that's how you define the field,
> considering you might miss a variant in multiple if:then: .
>
>
>> If we are going to add new enum values specific for each USI version,
>> isn't it better to selectively constrain them in the binding?
> You are supposed to constrained them.
>
> Again: widest constrains always stay in top level property. This applies
> to all bindings, all fields. Repeated multiple times, so here is
> standard example:
>
> https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127

Ah, I see now. Will get that fixed.

>
>
>>>>      description:
>>>>        Selects USI function (which serial protocol to use). Refer to
>>>>        <include/dt-bindings/soc/samsung,exynos-usi.h> for valid USI mode values.
>>>> @@ -101,18 +111,42 @@ required:
>>>>    - samsung,sysreg
>>>>    - samsung,mode
>>>>  
>>>> +allOf:
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - samsung,exynos850-usi
>>>> +    then:
>>>> +      properties:
>>>> +        reg:
>>>> +          maxItems: 1
>>>> +
>>>> +        samsung,mode:
>>>> +          enum: [0, 1, 2, 3]
>>>> +
>>>> +      required:
>>>> +        - reg
>>>> +
>>>> +    else:
>>>> +      properties:
>>>> +        reg: false
>>>> +        samsung,clkreq-on: false
>>>> +
>>>> +        samsung,mode:
>>>> +          enum: [4, 5, 6, 7, 8, 9, 10]
>>> Is it really true? Previously for example GS101 had values 0-3, now you
>>> claim has values 4-10, so an ABI change without explanation.
>> How is it unexplained? Citing the commit message:
>> "Add constants for choosing USIv1 configuration mode in device tree.
>> Those are further used in the USI driver to figure out which value to
>> write into SW_CONF register."
>>
> I don't see reference here about GS101 and others.
>
>> USIv1 and v2 write different values to the SW_CONF register. For example:
>>
>> #define USI_V1_SW_CONF_UART        0x8
>> #define USI_V2_SW_CONF_UART    BIT(0)
>>
>> ..
>>  [USI_V2_UART] =    { .name = "uart", .val = USI_V2_SW_CONF_UART },
>>  [USI_V1_UART] =    { .name = "uart", .val = USI_V1_SW_CONF_UART },
>> ..
>>
>> Hence the decision to have separate constants for different USI versions,
>> which in my opinion is cleaner than meshing the enums together and
>> choosing what to use with IFs in the driver code.
> This does not answer at all why GS101 receives now different values than
> before.
>
> Explain why you are changing ABI.

Oh I see what I've missed, it should be everything non-8895 having 0-3,
not just the top-level compatible samsung,exynos850-usi.

>
>>>> +
>>>>  if:
>>> Missing allOf:
>>>
>>>>    properties:
>>>>      compatible:
>>>>        contains:
>>>>          enum:
>>>>            - samsung,exynos850-usi
>>>> +          - samsung,exynos8895-usi
>>> Effect is not readable and not correct. You have two if:then:else.
>>> Usually it is easier to just have separate if: for each group of
>>> variants and define/constrain complete for such group within its if:.
>>>
>>>>  
>>>>  then:
>>>>    properties:
>>>> -    reg:
>>>> -      maxItems: 1
>>>> -
>>>>      clocks:
>>>>        items:
>>>>          - description: Bus (APB) clock
>>>> @@ -122,16 +156,13 @@ then:
>>>>        maxItems: 2
>>>>  
>>>>    required:
>>>> -    - reg
>>>>      - clocks
>>>>      - clock-names
>>>>  
>>>>  else:
>>>>    properties:
>>>> -    reg: false
>>>>      clocks: false
>>>>      clock-names: false
>>>> -    samsung,clkreq-on: false
>>>>  
>>>>  additionalProperties: false
>>>>  
>>>> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
>>>> index a01af169d..4c077c9a8 100644
>>>> --- a/include/dt-bindings/soc/samsung,exynos-usi.h
>>>> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h
>>>> @@ -13,5 +13,12 @@
>>>>  #define USI_V2_UART		1
>>>>  #define USI_V2_SPI		2
>>>>  #define USI_V2_I2C		3
>>>> +#define USI_V1_NONE		4
>>> Drop, it is already there.
>>>
>>>> +#define USI_V1_I2C0		5
>>>> +#define USI_V1_I2C1		6
>>>> +#define USI_V1_I2C0_1		7
>>>> +#define USI_V1_SPI		8
>>> Drop
>>>
>>>> +#define USI_V1_UART		9
>>> Drop
>> How so? These bring different configuration values. Could you specify how
>> exactly you want me to implement these in the driver?
> Heh, so the binding was made poorly for the driver and driver was
> developed in a matching way, so now this became an argument. Binding and
> drivers are independent, so whatever argument was in the driver does not
> matter really.
>
> What I don't understand is downstream for USIv1 and USIv2 has it correct
> - proper string values without mentioning any version. So, surprisingly
> proper downstream binding, really rare case, was converted to something
> tied to current driver implementation.
>
> You have only one sort of property - the mode how you configure the USI
> engine. The mode can be: I2C, SPI, I2C0, I2C1 for special cases with two
> I2C etc.
>
> The mode is not "USI_V1_I2C" because it is redundant. USI V1 cannot be
> USI V2. You cannot tell USI to be v1 or v2. It is either v1 or v2. Only
> one of these, thus encoding this information in the binding is meaningless.
>
> I even mentioned this in original binding review:
> "so please drop everywhere v2 (bindings, symbols, Kconfig,
> functions) except the compatible."
> Well, then I missed to check on that, so now this mess has to be fixed.

Yeah I get it now. Alright, I'll look into what I can do to unmangle this
mess.

Thanks again!

Best regards,
Ivaylo

>
> Best regards,
> Krzysztof

Powered by blists - more mailing lists