* [RFC] Inconsistent sifive,fu540-c000-uart binding.
@ 2024-03-04 10:59 Sebastian Andrzej Siewior
2024-03-04 18:53 ` Conor Dooley
0 siblings, 1 reply; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2024-03-04 10:59 UTC (permalink / raw)
To: linux-riscv
Cc: Samuel Holland, Paul Walmsley, devicetree, linux-serial, Thomas Gleixner
| $ git grep fu540-c000-uart
| Documentation/devicetree/bindings/serial/sifive-serial.yaml: - sifive,fu540-c000-uart
| Documentation/devicetree/bindings/serial/sifive-serial.yaml: compatible = "sifive,fu540-c000-uart", "sifive,uart0";
| Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt:"sifive,fu540-c000-uart". This way, if SoC-specific
| Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt: compatible = "sifive,fu540-c000-uart", "sifive,uart0";
| arch/riscv/boot/dts/sifive/fu540-c000.dtsi: compatible = "sifive,fu540-c000-uart", "sifive,uart0";
| arch/riscv/boot/dts/sifive/fu540-c000.dtsi: compatible = "sifive,fu540-c000-uart", "sifive,uart0";
| drivers/tty/serial/sifive.c:OF_EARLYCON_DECLARE(sifive, "sifive,fu540-c000-uart0",
| drivers/tty/serial/sifive.c: { .compatible = "sifive,fu540-c000-uart0" },
note that the driver has a trailing 0 in the binding while the yaml
description and the DT part does not.
The 'sifive,uart' has a trailing 0 where the 0 denotes the version UART
IP.
Was this also intended for the fu540-c000-uart binding? Should the 0 be
added everywhere or removed from the driver?
Sebastian
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC] Inconsistent sifive,fu540-c000-uart binding. 2024-03-04 10:59 [RFC] Inconsistent sifive,fu540-c000-uart binding Sebastian Andrzej Siewior @ 2024-03-04 18:53 ` Conor Dooley 2024-03-07 2:48 ` Paul Walmsley 0 siblings, 1 reply; 7+ messages in thread From: Conor Dooley @ 2024-03-04 18:53 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: linux-riscv, Samuel Holland, Paul Walmsley, devicetree, linux-serial, Thomas Gleixner [-- Attachment #1.1: Type: text/plain, Size: 1579 bytes --] On Mon, Mar 04, 2024 at 11:59:47AM +0100, Sebastian Andrzej Siewior wrote: > | $ git grep fu540-c000-uart > | Documentation/devicetree/bindings/serial/sifive-serial.yaml: - sifive,fu540-c000-uart > | Documentation/devicetree/bindings/serial/sifive-serial.yaml: compatible = "sifive,fu540-c000-uart", "sifive,uart0"; > | Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt:"sifive,fu540-c000-uart". This way, if SoC-specific > | Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt: compatible = "sifive,fu540-c000-uart", "sifive,uart0"; > | arch/riscv/boot/dts/sifive/fu540-c000.dtsi: compatible = "sifive,fu540-c000-uart", "sifive,uart0"; > | arch/riscv/boot/dts/sifive/fu540-c000.dtsi: compatible = "sifive,fu540-c000-uart", "sifive,uart0"; > | drivers/tty/serial/sifive.c:OF_EARLYCON_DECLARE(sifive, "sifive,fu540-c000-uart0", > | drivers/tty/serial/sifive.c: { .compatible = "sifive,fu540-c000-uart0" }, > > note that the driver has a trailing 0 in the binding while the yaml > description and the DT part does not. > The 'sifive,uart' has a trailing 0 where the 0 denotes the version UART > IP. > > Was this also intended for the fu540-c000-uart binding? Should the 0 be > added everywhere or removed from the driver? I suspect that the driver is what's incorrect, given there's little value in putting the IP version in the SoC-specific compatible as it's a fixed implementation. I'd change the driver to match the bindings. Cheers, Conor. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] Inconsistent sifive,fu540-c000-uart binding. 2024-03-04 18:53 ` Conor Dooley @ 2024-03-07 2:48 ` Paul Walmsley 2024-03-07 9:09 ` [PATCH] serial: sifive: Remove 0 from fu540-c000-uart0 binding Sebastian Andrzej Siewior 0 siblings, 1 reply; 7+ messages in thread From: Paul Walmsley @ 2024-03-07 2:48 UTC (permalink / raw) To: Conor Dooley Cc: Sebastian Andrzej Siewior, linux-riscv, Samuel Holland, devicetree, linux-serial, Thomas Gleixner On Mon, 4 Mar 2024, Conor Dooley wrote: > On Mon, Mar 04, 2024 at 11:59:47AM +0100, Sebastian Andrzej Siewior wrote: > > | $ git grep fu540-c000-uart > > | Documentation/devicetree/bindings/serial/sifive-serial.yaml: - sifive,fu540-c000-uart > > | Documentation/devicetree/bindings/serial/sifive-serial.yaml: compatible = "sifive,fu540-c000-uart", "sifive,uart0"; > > | Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt:"sifive,fu540-c000-uart". This way, if SoC-specific > > | Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt: compatible = "sifive,fu540-c000-uart", "sifive,uart0"; > > | arch/riscv/boot/dts/sifive/fu540-c000.dtsi: compatible = "sifive,fu540-c000-uart", "sifive,uart0"; > > | arch/riscv/boot/dts/sifive/fu540-c000.dtsi: compatible = "sifive,fu540-c000-uart", "sifive,uart0"; > > | drivers/tty/serial/sifive.c:OF_EARLYCON_DECLARE(sifive, "sifive,fu540-c000-uart0", > > | drivers/tty/serial/sifive.c: { .compatible = "sifive,fu540-c000-uart0" }, > > > > note that the driver has a trailing 0 in the binding while the yaml > > description and the DT part does not. > > The 'sifive,uart' has a trailing 0 where the 0 denotes the version UART > > IP. > > > > Was this also intended for the fu540-c000-uart binding? Should the 0 be > > added everywhere or removed from the driver? > > I suspect that the driver is what's incorrect, given there's little > value in putting the IP version in the SoC-specific compatible as it's > a fixed implementation. I'd change the driver to match the bindings. Agreed - Paul _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] serial: sifive: Remove 0 from fu540-c000-uart0 binding. 2024-03-07 2:48 ` Paul Walmsley @ 2024-03-07 9:09 ` Sebastian Andrzej Siewior 2024-03-07 17:39 ` Conor Dooley 0 siblings, 1 reply; 7+ messages in thread From: Sebastian Andrzej Siewior @ 2024-03-07 9:09 UTC (permalink / raw) To: Paul Walmsley Cc: Conor Dooley, linux-riscv, Samuel Holland, linux-serial, Thomas Gleixner, Greg Kroah-Hartman, Jiri Slaby The driver is using "sifive,fu540-c000-uart0" as a binding. The device tree and documentation states "sifive,fu540-c000-uart" instead. This means the binding is not matched and not used. This did not cause any problems because the alternative binding, used in the device tree, "sifive,uart0" is not handling the hardware any different. Align the binding in the driver with the documentation. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- On 2024-03-06 18:48:13 [-0800], Paul Walmsley wrote: > On Mon, 4 Mar 2024, Conor Dooley wrote: > > I suspect that the driver is what's incorrect, given there's little > > value in putting the IP version in the SoC-specific compatible as it's > > a fixed implementation. I'd change the driver to match the bindings. > > Agreed I didn't add any stable/ fixes tags as I guess there is no point in backporting this. > - Paul drivers/tty/serial/sifive.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c index 0670fd9f84967..cbfce65c9d221 100644 --- a/drivers/tty/serial/sifive.c +++ b/drivers/tty/serial/sifive.c @@ -761,7 +761,7 @@ static int __init early_sifive_serial_setup(struct earlycon_device *dev, } OF_EARLYCON_DECLARE(sifive, "sifive,uart0", early_sifive_serial_setup); -OF_EARLYCON_DECLARE(sifive, "sifive,fu540-c000-uart0", +OF_EARLYCON_DECLARE(sifive, "sifive,fu540-c000-uart", early_sifive_serial_setup); #endif /* CONFIG_SERIAL_EARLYCON */ @@ -1032,7 +1032,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(sifive_uart_pm_ops, sifive_serial_suspend, sifive_serial_resume); static const struct of_device_id sifive_serial_of_match[] = { - { .compatible = "sifive,fu540-c000-uart0" }, + { .compatible = "sifive,fu540-c000-uart" }, { .compatible = "sifive,uart0" }, {}, }; -- 2.43.0 Sebastian _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] serial: sifive: Remove 0 from fu540-c000-uart0 binding. 2024-03-07 9:09 ` [PATCH] serial: sifive: Remove 0 from fu540-c000-uart0 binding Sebastian Andrzej Siewior @ 2024-03-07 17:39 ` Conor Dooley 2024-03-07 17:43 ` Samuel Holland 0 siblings, 1 reply; 7+ messages in thread From: Conor Dooley @ 2024-03-07 17:39 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: Paul Walmsley, linux-riscv, Samuel Holland, linux-serial, Thomas Gleixner, Greg Kroah-Hartman, Jiri Slaby [-- Attachment #1.1: Type: text/plain, Size: 2294 bytes --] On Thu, Mar 07, 2024 at 10:09:50AM +0100, Sebastian Andrzej Siewior wrote: > The driver is using "sifive,fu540-c000-uart0" as a binding. The device > tree and documentation states "sifive,fu540-c000-uart" instead. This > means the binding is not matched and not used. > > This did not cause any problems because the alternative binding, used in > the device tree, "sifive,uart0" is not handling the hardware any > different. > > Align the binding in the driver with the documentation. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > On 2024-03-06 18:48:13 [-0800], Paul Walmsley wrote: > > On Mon, 4 Mar 2024, Conor Dooley wrote: > > > I suspect that the driver is what's incorrect, given there's little > > > value in putting the IP version in the SoC-specific compatible as it's > > > a fixed implementation. I'd change the driver to match the bindings. > > > > Agreed > > I didn't add any stable/ fixes tags as I guess there is no point in > backporting this. Every documented device falls back to "sifive,uart0", as you mention above, so I think that's reasonable. Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Conor. > > > - Paul > > drivers/tty/serial/sifive.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c > index 0670fd9f84967..cbfce65c9d221 100644 > --- a/drivers/tty/serial/sifive.c > +++ b/drivers/tty/serial/sifive.c > @@ -761,7 +761,7 @@ static int __init early_sifive_serial_setup(struct earlycon_device *dev, > } > > OF_EARLYCON_DECLARE(sifive, "sifive,uart0", early_sifive_serial_setup); > -OF_EARLYCON_DECLARE(sifive, "sifive,fu540-c000-uart0", > +OF_EARLYCON_DECLARE(sifive, "sifive,fu540-c000-uart", > early_sifive_serial_setup); > #endif /* CONFIG_SERIAL_EARLYCON */ > > @@ -1032,7 +1032,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(sifive_uart_pm_ops, sifive_serial_suspend, > sifive_serial_resume); > > static const struct of_device_id sifive_serial_of_match[] = { > - { .compatible = "sifive,fu540-c000-uart0" }, > + { .compatible = "sifive,fu540-c000-uart" }, > { .compatible = "sifive,uart0" }, > {}, > }; > -- > 2.43.0 > > Sebastian [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] serial: sifive: Remove 0 from fu540-c000-uart0 binding. 2024-03-07 17:39 ` Conor Dooley @ 2024-03-07 17:43 ` Samuel Holland 2024-03-07 17:56 ` Conor Dooley 0 siblings, 1 reply; 7+ messages in thread From: Samuel Holland @ 2024-03-07 17:43 UTC (permalink / raw) To: Conor Dooley, Sebastian Andrzej Siewior Cc: Paul Walmsley, linux-riscv, linux-serial, Thomas Gleixner, Greg Kroah-Hartman, Jiri Slaby Hi Conor, Sebastian, On 2024-03-07 11:39 AM, Conor Dooley wrote: > On Thu, Mar 07, 2024 at 10:09:50AM +0100, Sebastian Andrzej Siewior wrote: >> The driver is using "sifive,fu540-c000-uart0" as a binding. The device >> tree and documentation states "sifive,fu540-c000-uart" instead. This >> means the binding is not matched and not used. >> >> This did not cause any problems because the alternative binding, used in >> the device tree, "sifive,uart0" is not handling the hardware any >> different. >> >> Align the binding in the driver with the documentation. >> >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> --- >> On 2024-03-06 18:48:13 [-0800], Paul Walmsley wrote: >>> On Mon, 4 Mar 2024, Conor Dooley wrote: >>>> I suspect that the driver is what's incorrect, given there's little >>>> value in putting the IP version in the SoC-specific compatible as it's >>>> a fixed implementation. I'd change the driver to match the bindings. >>> >>> Agreed >> > >> I didn't add any stable/ fixes tags as I guess there is no point in >> backporting this. > > Every documented device falls back to "sifive,uart0", as you mention > above, so I think that's reasonable. Right. In fact this means the sifive,fu540-c000-uart compatible can be removed from the driver entirely, since the driver would match sifive,uart0 anyway. Regards, Samuel > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > Thanks, > Conor. > >> >>> - Paul >> >> drivers/tty/serial/sifive.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c >> index 0670fd9f84967..cbfce65c9d221 100644 >> --- a/drivers/tty/serial/sifive.c >> +++ b/drivers/tty/serial/sifive.c >> @@ -761,7 +761,7 @@ static int __init early_sifive_serial_setup(struct earlycon_device *dev, >> } >> >> OF_EARLYCON_DECLARE(sifive, "sifive,uart0", early_sifive_serial_setup); >> -OF_EARLYCON_DECLARE(sifive, "sifive,fu540-c000-uart0", >> +OF_EARLYCON_DECLARE(sifive, "sifive,fu540-c000-uart", >> early_sifive_serial_setup); >> #endif /* CONFIG_SERIAL_EARLYCON */ >> >> @@ -1032,7 +1032,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(sifive_uart_pm_ops, sifive_serial_suspend, >> sifive_serial_resume); >> >> static const struct of_device_id sifive_serial_of_match[] = { >> - { .compatible = "sifive,fu540-c000-uart0" }, >> + { .compatible = "sifive,fu540-c000-uart" }, >> { .compatible = "sifive,uart0" }, >> {}, >> }; >> -- >> 2.43.0 >> >> Sebastian _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] serial: sifive: Remove 0 from fu540-c000-uart0 binding. 2024-03-07 17:43 ` Samuel Holland @ 2024-03-07 17:56 ` Conor Dooley 0 siblings, 0 replies; 7+ messages in thread From: Conor Dooley @ 2024-03-07 17:56 UTC (permalink / raw) To: Samuel Holland Cc: Sebastian Andrzej Siewior, Paul Walmsley, linux-riscv, linux-serial, Thomas Gleixner, Greg Kroah-Hartman, Jiri Slaby [-- Attachment #1.1: Type: text/plain, Size: 1907 bytes --] On Thu, Mar 07, 2024 at 11:43:53AM -0600, Samuel Holland wrote: > Hi Conor, Sebastian, > > On 2024-03-07 11:39 AM, Conor Dooley wrote: > > On Thu, Mar 07, 2024 at 10:09:50AM +0100, Sebastian Andrzej Siewior wrote: > >> The driver is using "sifive,fu540-c000-uart0" as a binding. The device > >> tree and documentation states "sifive,fu540-c000-uart" instead. This > >> means the binding is not matched and not used. > >> > >> This did not cause any problems because the alternative binding, used in > >> the device tree, "sifive,uart0" is not handling the hardware any > >> different. > >> > >> Align the binding in the driver with the documentation. > >> > >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > >> --- > >> On 2024-03-06 18:48:13 [-0800], Paul Walmsley wrote: > >>> On Mon, 4 Mar 2024, Conor Dooley wrote: > >>>> I suspect that the driver is what's incorrect, given there's little > >>>> value in putting the IP version in the SoC-specific compatible as it's > >>>> a fixed implementation. I'd change the driver to match the bindings. > >>> > >>> Agreed > >> > > > >> I didn't add any stable/ fixes tags as I guess there is no point in > >> backporting this. > > > > Every documented device falls back to "sifive,uart0", as you mention > > above, so I think that's reasonable. > > Right. In fact this means the sifive,fu540-c000-uart compatible can be removed > from the driver entirely, since the driver would match sifive,uart0 anyway. I'm always a bit hesitant when it comes to removing compatibles that are backed up by a mandatory fallback in where there could be some old firmware/DT floating around that didn't have the fallback. I think in this case that's pretty moot though, so ye, it could totally be dropped from the driver entirely. I'm happy with either, both cases are an undocumented compatible being removed ;) [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-03-07 17:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-04 10:59 [RFC] Inconsistent sifive,fu540-c000-uart binding Sebastian Andrzej Siewior 2024-03-04 18:53 ` Conor Dooley 2024-03-07 2:48 ` Paul Walmsley 2024-03-07 9:09 ` [PATCH] serial: sifive: Remove 0 from fu540-c000-uart0 binding Sebastian Andrzej Siewior 2024-03-07 17:39 ` Conor Dooley 2024-03-07 17:43 ` Samuel Holland 2024-03-07 17:56 ` Conor Dooley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).