Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/08/2024 05:34, 郑豪威 wrote:
> 
> 在 2024/8/12 16:25, Krzysztof Kozlowski 写道:
>> On 12/08/2024 10:09, 郑豪威 wrote:
>>> 在 2024/8/9 18:05, Krzysztof Kozlowski 写道:
>>>> On 09/08/2024 11:55, 郑豪威 wrote:
>>>>>>>>> +    description: Enables fractional-N division. Currently,
>>>>>>>>> +      only LS2K1500 and LS2K2000 support this feature.
>>>>>>>>> +
>>>>>>>>> +  rts-invert:
>>>>>>>>> +    description: Inverts the RTS value in the MCR register.
>>>>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>>>>> +
>>>>>>>>> +  dtr-invert:
>>>>>>>>> +    description: Inverts the DTR value in the MCR register.
>>>>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>>>>> +
>>>>>>>>> +  cts-invert:
>>>>>>>>> +    description: Inverts the CTS value in the MSR register.
>>>>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>>>>> +      and Loongson LS7A bridge chips.
>>>>>>>>> +
>>>>>>>>> +  dsr-invert:
>>>>>>>>> +    description: Inverts the DSR value in the MSR register.
>>>>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>>>>> +      and Loongson LS7A bridge chips.
>>>>>> Same questions for all these. Why choosing invert is a board level
>>>>>> decision? If it "should be used" then why it is not used always?
>>>>>>
>>>>> Because these features are not applicable to all chips, such as
>>>>> 'fractional-division',
>>>> Hm?
>>>>
>>>>> which is currently supported only by 2K1500 and 2K2000, and for
>>>>> Loongson-3 series
>>>> These are SoCs. Compatible defines that. Please align with your
>>>> colleagues, because *we talked about this* already.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>> I consulted with my colleagues and would like to confirm with you. For
>>> the five
>>>
>>> properties provided, fractional-division is offered as a new feature,
>>> supported by
>>>
>>> 2K1500 and 2K2000. As for the invert property, it is due to a bug in our
>>> controller,
>>>
>>> and its usage may vary across different chips. Should we add different
>>> compatible
>>>
>>> values to address these issues for different chips, whether they are new
>>> features or
>>>
>>> controller bugs?
>> How did you align? We had already talks with you about this problem -
>> you need specific compatibles. How you explain above properties, all of
>> them are deducible from the compatible, so drop them.
>>
>> Your entire argument above does not address at all my concerns, so
>> before you respond repeating the same, really talk with your colleagues.
>>
>> One of many previous discussions:
>> https://lore.kernel.org/linux-devicetree/25c30964-6bd3-c7eb-640a-ba1f513b7675@linaro.org/
>>
>> https://lore.kernel.org/linux-devicetree/20230526-dolly-reheat-06c4d5658415@wendy/
>>
>> I wish we do not have to keep repeating the same to Loongson. Please
>> STORE the feedback for any future submissions, so you will not repeat
>> the same mistakes over and over.
>>
>> Best regards,
>> Krzysztof
> 
> Hi:
> 
> I have been aligning with my colleagues over the past few days and
> 
> reviewing previous discussions. Based on these, I have made the
> 
> following modifications according to the differences in the controller:
> 
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - loongson,ls7a-uart
> +          - loongson,ls3a5000-uart
> +          - loongson,ls2k2000-uart
> +      - items:
> +          - enum:
> +              - loongson,ls2k1000-uart
> +              - loongson,ls2k0500-uart
> +          - const: loongson,ls7a-uart
> +      - items:
> +          - enum:
> +              - loongson,ls2k1500-uart
> +          - const: loongson,ls2k2000-uart
> +      - items:
> +          - enum:
> +              - loongson,ls3a6000-uart
> +          - const: loongson,ls3a5000-uart
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clock-frequency: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clock-frequency
> +
> +allOf:
> +  - $ref: serial.yaml
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/clock/loongson,ls2k-clk.h>
> +
> +    serial@1fe20000 {
> +        compatible = "loongson,ls2k1000-uart", "loongson,ls7a-uart";
> +        reg = <0x1fe20000 0x10>;
> +        clock-frequency = <125000000>;
> +        interrupt-parent = <&liointc0>;
> +        interrupts = <0x0 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> 
> Does this modification meet the expectation?

Yes, assuming ls7a is a specific SoC, not a family of SoC.

Best regards,
Krzysztof