| Message ID | 20240804063834.70022-1-zhenghaowei@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] dt-bindings: serial: Add Loongson UART controller | expand |
在 2024/8/4 16:41, Krzysztof Kozlowski 写道: > On 04/08/2024 08:38,zhenghaowei@loongson.cn wrote: >> From: Haowei Zheng<zhenghaowei@loongson.cn> >> >> Add Loongson UART controller binding with DT schema format using >> json-schema. >> > Where is the changelog? Are you sending the same patch again? > > Best regards, > Krzysztof Sorry, here are the change log from V1 to V2, and I will include the update in the next patch update. Changes in V2: - Correct the schema formatting errors. - file name changed from 'loongson-uart.yaml' to 'loongson,ls7a-uart.yaml' - Replace 'loongson,loongson-uart' with 'loongson,ls7a-uart'. Best regards, Haowei Zheng
在 2024/8/4 16:43, Krzysztof Kozlowski 写道: > On 04/08/2024 08:38,zhenghaowei@loongson.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 >> + 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. >> + >> +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@1fe001e0 { >> + 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@loongson.cn> >> +L: linux-serial@vger.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? Best regards, Haowei Zheng
在 2024/8/4 23:33, Krzysztof Kozlowski 写道: > On 04/08/2024 08:38,zhenghaowei@loongson.cn wrote: >> From: Haowei Zheng<zhenghaowei@loongson.cn> >> >> Due to certain hardware design challenges, we have opted to >> utilize a dedicated UART driver to probe the UART interface. >> >> Presently, we have defined four parameters — 'fractional-division', >> 'invert-rts', 'invert-dtr', 'invert-cts', and 'invert-dsr' — which >> will be employed as needed. >> >> Signed-off-by: Haowei Zheng<zhenghaowei@loongson.cn> >> --- >> drivers/tty/serial/8250/8250_loongson.c | 208 ++++++++++++++++++++++++ >> drivers/tty/serial/8250/8250_port.c | 8 + >> drivers/tty/serial/8250/Kconfig | 9 + >> drivers/tty/serial/8250/Makefile | 1 + >> include/uapi/linux/serial_core.h | 1 + >> 5 files changed, 227 insertions(+) >> create mode 100644 drivers/tty/serial/8250/8250_loongson.c >> >> diff --git a/drivers/tty/serial/8250/8250_loongson.c b/drivers/tty/serial/8250/8250_loongson.c >> new file mode 100644 >> index 000000000000..eb16677f1dde >> --- /dev/null >> +++ b/drivers/tty/serial/8250/8250_loongson.c >> @@ -0,0 +1,208 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * Copyright (C) 2020-2024 Loongson Technology Corporation Limited >> + */ >> + >> +#include <linux/acpi.h> > How is this used? I forgot to drop it, Before this, when the kernel was booted in ACPI mode, we used acpi_match_table for driver registration. To maintain code simplicity, now we use "PRP0001" for driver registration, so we don't need 'acpi.h' anymore. >> +#include <linux/clk.h> > And this? Currently, it doesn't seem to serve much purpose, and I will remove it in the next version. >> +#include <linux/console.h> >> +#include <linux/delay.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/reset.h> >> + >> +#include "8250.h" >> + >> +struct loongson_uart_data { >> + struct reset_control *rst; >> + int line; >> + int mcr_invert; >> + int msr_invert; >> +}; > ... > >> +static int loongson_uart_probe(struct platform_device *pdev) >> +{ >> + struct uart_8250_port uart = {}; >> + struct loongson_uart_data *data; >> + struct uart_port *port; >> + struct resource *res; >> + int ret; >> + >> + port = &uart.port; >> + spin_lock_init(&port->lock); >> + >> + port->flags = UPF_SHARE_IRQ | UPF_FIXED_PORT | UPF_FIXED_TYPE; >> + port->iotype = UPIO_MEM; >> + port->regshift = 0; >> + port->dev = &pdev->dev; >> + port->type = (unsigned long)device_get_match_data(&pdev->dev); >> + port->serial_in = loongson_serial_in; >> + port->serial_out = loongson_serial_out; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) >> + return -ENODEV; >> + >> + port->membase = devm_ioremap(&pdev->dev, res->start, resource_size(res)); >> + if (!port->membase) >> + return -ENOMEM; >> + > Use wrapper combining both calls. I got it, did you mean like this? + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -ENODEV; + + port->mapbase = res->start; + port->mapsize = resource_size(res); + + port->membase = devm_ioremap(&pdev->dev, port->mapbase, port->mapsize); + if (!port->membase) + return -ENOMEM; >> + port->mapbase = res->start; >> + port->mapsize = resource_size(res); >> + >> + port->irq = platform_get_irq(pdev, 0); >> + if (port->irq < 0) >> + return -EINVAL; >> + >> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + port->private_data = data; >> + >> + if (device_property_read_bool(&pdev->dev, "fractional-division")) { >> + port->get_divisor = loongson_frac_get_divisor; >> + port->set_divisor = loongson_frac_set_divisor; >> + } >> + >> + if (device_property_read_bool(&pdev->dev, "rts-invert")) >> + data->mcr_invert |= UART_MCR_RTS; >> + >> + if (device_property_read_bool(&pdev->dev, "dtr-invert")) >> + data->mcr_invert |= UART_MCR_DTR; >> + >> + if (device_property_read_bool(&pdev->dev, "cts-invert")) >> + data->msr_invert |= UART_MSR_CTS; >> + >> + if (device_property_read_bool(&pdev->dev, "dsr-invert")) >> + data->msr_invert |= UART_MSR_DSR; >> + >> + data->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL); >> + if (IS_ERR(data->rst)) >> + return PTR_ERR(data->rst); >> + >> + device_property_read_u32(&pdev->dev, "clock-frequency", &port->uartclk); >> + >> + ret = reset_control_deassert(data->rst); >> + if (ret) >> + goto err_unprepare; >> + >> + ret = serial8250_register_8250_port(&uart); >> + if (ret < 0) >> + goto err_unprepare; >> + >> + platform_set_drvdata(pdev, data); >> + data->line = ret; >> + >> + return 0; >> + >> +err_unprepare: >> + >> + return ret; >> +} >> + >> +static void loongson_uart_remove(struct platform_device *pdev) >> +{ >> + struct loongson_uart_data *data = platform_get_drvdata(pdev); >> + >> + serial8250_unregister_port(data->line); >> + reset_control_assert(data->rst); >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int loongson_uart_suspend(struct device *dev) >> +{ >> + struct loongson_uart_data *data = dev_get_drvdata(dev); >> + >> + serial8250_suspend_port(data->line); >> + >> + return 0; >> +} >> + >> +static int loongson_uart_resume(struct device *dev) >> +{ >> + struct loongson_uart_data *data = dev_get_drvdata(dev); >> + >> + serial8250_resume_port(data->line); >> + >> + return 0; >> +} >> +#endif >> + >> +static const struct dev_pm_ops loongson_uart_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(loongson_uart_suspend, loongson_uart_resume) >> +}; >> + >> +static const struct of_device_id of_platform_serial_table[] = { >> + {.compatible = "loongson,ls7a-uart", .data = (void *)PORT_LOONGSON}, > Why do you need match data if there is no choice? Considering whether new port types might be added in the future. Of course, currently it doesn't seem necessary to do so. >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, of_platform_serial_table); >> + >> +static struct platform_driver loongson_uart_driver = { >> + .probe = loongson_uart_probe, >> + .remove = loongson_uart_remove, >> + .driver = { >> + .name = "ls7a-uart", >> + .pm = &loongson_uart_pm_ops, >> + .of_match_table = of_match_ptr(of_platform_serial_table), > Except that this does not build... drop of_match_ptr(), not needed and > causes warnings. > Ok, I got it. >> + }, >> +}; >> + >> +module_platform_driver(loongson_uart_driver); >> + >> +MODULE_DESCRIPTION("LOONGSON 8250 Driver"); >> +MODULE_AUTHOR("Haowei Zheng<zhenghaowei@loongson.cn>"); >> +MODULE_LICENSE("GPL"); >> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c >> index 2786918aea98..60b72c785028 100644 >> --- a/drivers/tty/serial/8250/8250_port.c >> +++ b/drivers/tty/serial/8250/8250_port.c >> @@ -319,6 +319,14 @@ static const struct serial8250_config uart_config[] = { >> .rxtrig_bytes = {1, 8, 16, 30}, >> .flags = UART_CAP_FIFO | UART_CAP_AFE, >> }, >> + [PORT_LOONGSON] = { >> + .name = "Loongson", >> + .fifo_size = 16, >> + .tx_loadsz = 16, >> + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10, >> + .rxtrig_bytes = {1, 4, 8, 14}, >> + .flags = UART_CAP_FIFO, >> + }, >> }; >> >> /* Uart divisor latch read */ >> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig >> index 47ff50763c04..a696afc4f8a8 100644 >> --- a/drivers/tty/serial/8250/Kconfig >> +++ b/drivers/tty/serial/8250/Kconfig >> @@ -568,6 +568,15 @@ config SERIAL_8250_BCM7271 >> including DMA support and high accuracy BAUD rates, say >> Y to this option. If unsure, say N. >> >> +config SERIAL_8250_LOONGSON >> + tristate "Loongson 8250 serial port support" >> + default SERIAL_8250 >> + depends on SERIAL_8250 >> + depends on LOONGARCH || MIPS > MIPS? Why? > > You also miss COMPILE_TEST. > > > > Best regards, > Krzysztof The addition of mips was intended to maintain compatibility with loongson-3a4000 and earlier chips. Currently, it appears that this lacks sufficient validation, and I will remove it in the next version. I compiled and verified it on the Loongson 3A6000 machine, and currently it seems to have issues. I will fix the compilation problem in the next version. Best regards, Haowei Zheng
On Wed, 2024-08-07 at 16:23 +0800, 郑豪威 wrote: > 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? You should add this line in the second patch then. Separating a large change into multiple patches in a series is not for formalities' sake. Each patch should be logically intact on its own.
在 2024/8/7 16:39, Xi Ruoyao 写道: > On Wed, 2024-08-07 at 16:23 +0800, 郑豪威 wrote: >> 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? > You should add this line in the second patch then. Separating a large > change into multiple patches in a series is not for formalities' sake. > Each patch should be logically intact on its own. > Thank you, I got it. Best regards, Haowei Zheng
在 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@loongson.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@1fe001e0 { >>>> + 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@loongson.cn> >>>> +L: linux-serial@vger.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
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
在 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? Best regards, Haowei Zheng
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
在 2024/8/25 14:55, Krzysztof Kozlowski 写道: > 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 I got it, thank you for your reply. I will update the discussion content in the next version. Best regards, Haowei Zheng
diff --git a/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml b/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml new file mode 100644 index 000000000000..22d9cca5569e --- /dev/null +++ b/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml @@ -0,0 +1,74 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/loongson,ls7a-uart.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Loongson UART + +maintainers: + - Haowei Zheng <zhenghaowei@loongson.cn> + +properties: + compatible: + const: loongson,ls7a-uart + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + fractional-division: + 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. + +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@1fe001e0 { + 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@loongson.cn> +L: linux-serial@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml +F: drivers/tty/serial/8250/8250_loongson.c + LOONGSON-2 SOC SERIES CLOCK DRIVER M: Yinbo Zhu <zhuyinbo@loongson.cn> L: linux-clk@vger.kernel.org