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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <89e71573-9365-2e61-bb38-759363df1b8b@loongson.cn>
Date: Fri, 9 Aug 2024 17:55:04 +0800
From: 郑豪威 <zhenghaowei@...ngson.cn>
To: Krzysztof Kozlowski <krzk@...nel.org>, gregkh@...uxfoundation.org,
 jirislaby@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, chenhuacai@...nel.org, kernel@...0n.name,
 p.zabel@...gutronix.de
Cc: linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, loongarch@...ts.linux.dev
Subject: Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller


在 2024/8/9 13:53, Krzysztof Kozlowski 写道:
> On 07/08/2024 10:23, 郑豪威 wrote:
>> 在 2024/8/4 16:43, Krzysztof Kozlowski 写道:
>>> On 04/08/2024 08:38,zhenghaowei@...ngson.cn  wrote:
>>>
>>> Due to lack of changelog, I assume you send the same patch, so:
>>>
>>> <form letter>
>>> This is a friendly reminder during the review process.
>>>
>>> It seems my or other reviewer's previous comments were not fully
>>> addressed. Maybe the feedback got lost between the quotes, maybe you
>>> just forgot to apply it. Please go back to the previous discussion and
>>> either implement all requested changes or keep discussing them.
>>>
>>> Thank you.
>>> </form letter>
>>>
>>> Also:
>>>
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>> +
>>>> +  fractional-division:
>>> Where are this and following defined? In which schema?
>>>
>> These and the ones below are new definitions, can I use them like this?
>>
>> +  fractional-division:
>> +    description: Enables fractional-N division. Currently,
>> +      only LS2K1500 and LS2K2000 support this feature.
>> +    type: boolean
>>
> Missing vendor prefix, but what's more important, why would this be
> property of DT? Just enable it always...
>
>>>> +    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',

which is currently supported only by 2K1500 and 2K2000, and for 
Loongson-3 series

CPUs,  'cts-invert' and 'dtr-invert' are not needed. More importantly, 
for future chip

designs, these issues may no longer exist.

>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - interrupts
>>>> +  - clocks
>>>> +
>>>> +allOf:
>>>> +  - $ref: serial.yaml
>>>> +
>>>> +unevaluatedProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>> +    #include <dt-bindings/clock/loongson,ls2k-clk.h>
>>>> +
>>>> +    serial@...001e0 {
>>>> +        compatible = "loongson,ls7a-uart";
>>>> +        reg = <0x0 0x1fe001e0 0x0 0x10>;
>>>> +        clock-frequency = <100000000>;
>>>> +        interrupt-parent = <&liointc>;
>>>> +        interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
>>>> +        fractional-division;
>>>> +        rts-invert;
>>>> +        dtr-invert;
>>>> +    };
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 8766f3e5e87e..a6306327dba5 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -13189,6 +13189,13 @@ S:	Maintained
>>>>    F:	Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
>>>>    F:	drivers/i2c/busses/i2c-ls2x.c
>>>>    
>>>> +LOONGSON UART DRIVER
>>>> +M:	Haowei Zheng<zhenghaowei@...ngson.cn>
>>>> +L:	linux-serial@...r.kernel.org
>>>> +S:	Maintained
>>>> +F:	Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml
>>>> +F:	drivers/tty/serial/8250/8250_loongson.c
>>> There is no such file.
>>>
>>> Best regards,
>>> Krzysztof
>> The file "drivers/tty/serial/8250/8250_loongson.c" will be created in
>> the patch
>>
>> "tty: serial: 8250: Add loongson uart driver support". Is it
>> inappropriate to reference it here?
> Apply this patch and run get_maintainers self tests. What do you see?
>
> Of course it is not appropriate here. The file does not exist.
>
> Best regards,
> Krzysztof

I got it, I  will include it in the next patch.


Best regards,

Haowei Zheng