Add serial1 and serial2 nodes to define the existence of
UART1 and UART2.
Signed-off-by: Justin Swartz <[email protected]>
---
arch/mips/boot/dts/ralink/mt7621.dtsi | 38 +++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
index dca415fdd..2069249c8 100644
--- a/arch/mips/boot/dts/ralink/mt7621.dtsi
+++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
@@ -128,6 +128,44 @@ serial0: serial@c00 {
pinctrl-0 = <&uart1_pins>;
};
+ serial1: serial@d00 {
+ status = "disabled";
+
+ compatible = "ns16550a";
+ reg = <0xd00 0x100>;
+
+ clocks = <&sysc MT7621_CLK_UART2>;
+
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>;
+
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ no-loopback-test;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart2_pins>;
+ };
+
+ serial2: serial@e00 {
+ status = "disabled";
+
+ compatible = "ns16550a";
+ reg = <0xe00 0x100>;
+
+ clocks = <&sysc MT7621_CLK_UART3>;
+
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 28 IRQ_TYPE_LEVEL_HIGH>;
+
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ no-loopback-test;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart3_pins>;
+ };
+
spi0: spi@b00 {
status = "disabled";
--
Hi Justin,
On Wed, Mar 6, 2024 at 9:11 PM Justin Swartz
<[email protected]> wrote:
>
> Add serial1 and serial2 nodes to define the existence of
> UART1 and UART2.
>
> Signed-off-by: Justin Swartz <[email protected]>
> ---
> arch/mips/boot/dts/ralink/mt7621.dtsi | 38 +++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
> index dca415fdd..2069249c8 100644
> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
> @@ -128,6 +128,44 @@ serial0: serial@c00 {
> pinctrl-0 = <&uart1_pins>;
> };
>
> + serial1: serial@d00 {
> + status = "disabled";
> +
> + compatible = "ns16550a";
> + reg = <0xd00 0x100>;
> +
> + clocks = <&sysc MT7621_CLK_UART2>;
> +
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>;
> +
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + no-loopback-test;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart2_pins>;
> + };
> +
> + serial2: serial@e00 {
> + status = "disabled";
> +
> + compatible = "ns16550a";
> + reg = <0xe00 0x100>;
> +
> + clocks = <&sysc MT7621_CLK_UART3>;
> +
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SHARED 28 IRQ_TYPE_LEVEL_HIGH>;
> +
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + no-loopback-test;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart3_pins>;
> + };
> +
Please follow the preferred order for properties described in dts
coding style [0]. I know that there is some mess around the properties
order in some nodes with the current dtsi file but we did not have
coding style before and now we have it, so I think we should follow it
at least for new additions.
Best regards,
Sergio Paracuellos
[0]: https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
Hi Sergio
On 2024-03-07 12:04, Sergio Paracuellos wrote:
> Hi Justin,
>
> On Wed, Mar 6, 2024 at 9:11 PM Justin Swartz
> <[email protected]> wrote:
>>
>> Add serial1 and serial2 nodes to define the existence of
>> UART1 and UART2.
>>
>> Signed-off-by: Justin Swartz <[email protected]>
>> ---
>> arch/mips/boot/dts/ralink/mt7621.dtsi | 38
>> +++++++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi
>> b/arch/mips/boot/dts/ralink/mt7621.dtsi
>> index dca415fdd..2069249c8 100644
>> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
>> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
>> @@ -128,6 +128,44 @@ serial0: serial@c00 {
>> pinctrl-0 = <&uart1_pins>;
>> };
>>
>> + serial1: serial@d00 {
>> + status = "disabled";
>> +
>> + compatible = "ns16550a";
>> + reg = <0xd00 0x100>;
>> +
>> + clocks = <&sysc MT7621_CLK_UART2>;
>> +
>> + interrupt-parent = <&gic>;
>> + interrupts = <GIC_SHARED 27
>> IRQ_TYPE_LEVEL_HIGH>;
>> +
>> + reg-shift = <2>;
>> + reg-io-width = <4>;
>> + no-loopback-test;
>> +
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&uart2_pins>;
>> + };
>> +
>> + serial2: serial@e00 {
>> + status = "disabled";
>> +
>> + compatible = "ns16550a";
>> + reg = <0xe00 0x100>;
>> +
>> + clocks = <&sysc MT7621_CLK_UART3>;
>> +
>> + interrupt-parent = <&gic>;
>> + interrupts = <GIC_SHARED 28
>> IRQ_TYPE_LEVEL_HIGH>;
>> +
>> + reg-shift = <2>;
>> + reg-io-width = <4>;
>> + no-loopback-test;
>> +
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&uart3_pins>;
>> + };
>> +
>
> Please follow the preferred order for properties described in dts
> coding style [0]. I know that there is some mess around the properties
> order in some nodes with the current dtsi file but we did not have
> coding style before and now we have it, so I think we should follow it
> at least for new additions.
No problem. I see you've already "Acked-by" patch 1 (adding pinctrl
properties to serial0) of this set, so would it be a better move to
submit a new patch set that would look something like:
1. add pinctrl-name and pinctrl-0 to serial0 [no changes from what I
sent]
2. reorder serial0 properties according to the DTS style guidelines
3. add serial1 and serial2 with the correct property order
Or instead, submit one more patch that will reorder the properties in
serial0, serial1 and serial2 - which would depend on the current set?
> Best regards,
> Sergio Paracuellos
Regards
Justin
> [0]: https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
Hi Justin,
On Thu, Mar 7, 2024 at 4:15 PM Justin Swartz
<[email protected]> wrote:
>
> Hi Sergio
>
> On 2024-03-07 12:04, Sergio Paracuellos wrote:
> > Hi Justin,
> >
> > On Wed, Mar 6, 2024 at 9:11 PM Justin Swartz
> > <[email protected]> wrote:
> >>
> >> Add serial1 and serial2 nodes to define the existence of
> >> UART1 and UART2.
> >>
> >> Signed-off-by: Justin Swartz <[email protected]>
> >> ---
> >> arch/mips/boot/dts/ralink/mt7621.dtsi | 38
> >> +++++++++++++++++++++++++++
> >> 1 file changed, 38 insertions(+)
> >>
> >> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi
> >> b/arch/mips/boot/dts/ralink/mt7621.dtsi
> >> index dca415fdd..2069249c8 100644
> >> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
> >> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
> >> @@ -128,6 +128,44 @@ serial0: serial@c00 {
> >> pinctrl-0 = <&uart1_pins>;
> >> };
> >>
> >> + serial1: serial@d00 {
> >> + status = "disabled";
> >> +
> >> + compatible = "ns16550a";
> >> + reg = <0xd00 0x100>;
> >> +
> >> + clocks = <&sysc MT7621_CLK_UART2>;
> >> +
> >> + interrupt-parent = <&gic>;
> >> + interrupts = <GIC_SHARED 27
> >> IRQ_TYPE_LEVEL_HIGH>;
> >> +
> >> + reg-shift = <2>;
> >> + reg-io-width = <4>;
> >> + no-loopback-test;
> >> +
> >> + pinctrl-names = "default";
> >> + pinctrl-0 = <&uart2_pins>;
> >> + };
> >> +
> >> + serial2: serial@e00 {
> >> + status = "disabled";
> >> +
> >> + compatible = "ns16550a";
> >> + reg = <0xe00 0x100>;
> >> +
> >> + clocks = <&sysc MT7621_CLK_UART3>;
> >> +
> >> + interrupt-parent = <&gic>;
> >> + interrupts = <GIC_SHARED 28
> >> IRQ_TYPE_LEVEL_HIGH>;
> >> +
> >> + reg-shift = <2>;
> >> + reg-io-width = <4>;
> >> + no-loopback-test;
> >> +
> >> + pinctrl-names = "default";
> >> + pinctrl-0 = <&uart3_pins>;
> >> + };
> >> +
> >
> > Please follow the preferred order for properties described in dts
> > coding style [0]. I know that there is some mess around the properties
> > order in some nodes with the current dtsi file but we did not have
> > coding style before and now we have it, so I think we should follow it
> > at least for new additions.
>
> No problem. I see you've already "Acked-by" patch 1 (adding pinctrl
> properties to serial0) of this set, so would it be a better move to
> submit a new patch set that would look something like:
>
> 1. add pinctrl-name and pinctrl-0 to serial0 [no changes from what I
> sent]
> 2. reorder serial0 properties according to the DTS style guidelines
> 3. add serial1 and serial2 with the correct property order
This would be ok, thank you.
Best regards,
Sergio Paracuellos
Add missing pinctrl-name and pinctrl-0 properties to declare
that the uart1_pins group is associated with serial0.
Signed-off-by: Justin Swartz <[email protected]>
---
arch/mips/boot/dts/ralink/mt7621.dtsi | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
index 35a10258f..dca415fdd 100644
--- a/arch/mips/boot/dts/ralink/mt7621.dtsi
+++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
@@ -123,6 +123,9 @@ serial0: serial@c00 {
reg-shift = <2>;
reg-io-width = <4>;
no-loopback-test;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart1_pins>;
};
spi0: spi@b00 {
--
Add serial1 and serial2 nodes to define the existence of
the MT7621's second and third UARTs.
Signed-off-by: Justin Swartz <[email protected]>
---
arch/mips/boot/dts/ralink/mt7621.dtsi | 28 +++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
index 3ad4e2343..5a89f0b8c 100644
--- a/arch/mips/boot/dts/ralink/mt7621.dtsi
+++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
@@ -124,6 +124,34 @@ serial0: serial@c00 {
pinctrl-0 = <&uart1_pins>;
};
+ serial1: serial@d00 {
+ compatible = "ns16550a";
+ reg = <0xd00 0x100>;
+ reg-io-width = <4>;
+ reg-shift = <2>;
+ clocks = <&sysc MT7621_CLK_UART2>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>;
+ no-loopback-test;
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart2_pins>;
+ status = "disabled";
+ };
+
+ serial2: serial@e00 {
+ compatible = "ns16550a";
+ reg = <0xe00 0x100>;
+ reg-io-width = <4>;
+ reg-shift = <2>;
+ clocks = <&sysc MT7621_CLK_UART3>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 28 IRQ_TYPE_LEVEL_HIGH>;
+ no-loopback-test;
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart3_pins>;
+ status = "disabled";
+ };
+
spi0: spi@b00 {
status = "disabled";
--
Reorder serial0 properties according to the guidelines laid
out in Documentation/devicetree/bindings/dts-coding-style.rst
Signed-off-by: Justin Swartz <[email protected]>
---
arch/mips/boot/dts/ralink/mt7621.dtsi | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
index dca415fdd..3ad4e2343 100644
--- a/arch/mips/boot/dts/ralink/mt7621.dtsi
+++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
@@ -114,16 +114,12 @@ memc: memory-controller@5000 {
serial0: serial@c00 {
compatible = "ns16550a";
reg = <0xc00 0x100>;
-
+ reg-io-width = <4>;
+ reg-shift = <2>;
clocks = <&sysc MT7621_CLK_UART1>;
-
interrupt-parent = <&gic>;
interrupts = <GIC_SHARED 26 IRQ_TYPE_LEVEL_HIGH>;
-
- reg-shift = <2>;
- reg-io-width = <4>;
no-loopback-test;
-
pinctrl-names = "default";
pinctrl-0 = <&uart1_pins>;
};
--
On Thu, Mar 7, 2024 at 8:05 PM Justin Swartz
<[email protected]> wrote:
>
> Add missing pinctrl-name and pinctrl-0 properties to declare
> that the uart1_pins group is associated with serial0.
>
> Signed-off-by: Justin Swartz <[email protected]>
> ---
> arch/mips/boot/dts/ralink/mt7621.dtsi | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
> index 35a10258f..dca415fdd 100644
> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
> @@ -123,6 +123,9 @@ serial0: serial@c00 {
> reg-shift = <2>;
> reg-io-width = <4>;
> no-loopback-test;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart1_pins>;
> };
>
> spi0: spi@b00 {
> --
>
Please add Acked-by/Reviewed-by tags when posting new versions.
Thanks,
Sergio Paracuellos
On Thu, Mar 7, 2024 at 8:05 PM Justin Swartz
<[email protected]> wrote:
>
> Add serial1 and serial2 nodes to define the existence of
> the MT7621's second and third UARTs.
>
> Signed-off-by: Justin Swartz <[email protected]>
> ---
> arch/mips/boot/dts/ralink/mt7621.dtsi | 28 +++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
Acked-by: Sergio Paracuellos <[email protected]>
Thanks,
Sergio Paracuellos
On Thu, Mar 7, 2024 at 8:05 PM Justin Swartz
<[email protected]> wrote:
>
> Reorder serial0 properties according to the guidelines laid
> out in Documentation/devicetree/bindings/dts-coding-style.rst
>
> Signed-off-by: Justin Swartz <[email protected]>
> ---
> arch/mips/boot/dts/ralink/mt7621.dtsi | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
Acked-by: Sergio Paracuellos <[email protected]>
Thanks,
Sergio Paracuellos
On Fri, Mar 8, 2024 at 8:27 AM Sergio Paracuellos
<[email protected]> wrote:
>
> On Thu, Mar 7, 2024 at 8:05 PM Justin Swartz
> <[email protected]> wrote:
> >
> > Add missing pinctrl-name and pinctrl-0 properties to declare
> > that the uart1_pins group is associated with serial0.
> >
> > Signed-off-by: Justin Swartz <[email protected]>
> > ---
> > arch/mips/boot/dts/ralink/mt7621.dtsi | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
> > index 35a10258f..dca415fdd 100644
> > --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
> > +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
> > @@ -123,6 +123,9 @@ serial0: serial@c00 {
> > reg-shift = <2>;
> > reg-io-width = <4>;
> > no-loopback-test;
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&uart1_pins>;
> > };
> >
> > spi0: spi@b00 {
> > --
> >
>
> Please add Acked-by/Reviewed-by tags when posting new versions.
>
> Thanks,
> Sergio Paracuellos
Anyway:
Acked-by: Sergio Paracuellos <[email protected]>
Thanks,
Sergio Paracuellos
Il 07/03/24 20:04, Justin Swartz ha scritto:
> Add missing pinctrl-name and pinctrl-0 properties to declare
> that the uart1_pins group is associated with serial0.
>
> Signed-off-by: Justin Swartz <[email protected]>
> ---
> arch/mips/boot/dts/ralink/mt7621.dtsi | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
> index 35a10258f..dca415fdd 100644
> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
> @@ -123,6 +123,9 @@ serial0: serial@c00 {
> reg-shift = <2>;
> reg-io-width = <4>;
> no-loopback-test;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart1_pins>;
> };
>
> spi0: spi@b00 {
The pins are muxed and can be either UART, or some other function that
is supported by the mux: this means that the pinctrl-xxx properties shall
*not* go into the SoC dtsi file, but in board dts files instead.
Said differently: the usage of the UART pins is board-specific, not SoC-wide.
Regards,
Angelo
Il 07/03/24 20:04, Justin Swartz ha scritto:
> Reorder serial0 properties according to the guidelines laid
> out in Documentation/devicetree/bindings/dts-coding-style.rst
>
> Signed-off-by: Justin Swartz <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 07/03/24 20:04, Justin Swartz ha scritto:
> Add serial1 and serial2 nodes to define the existence of
> the MT7621's second and third UARTs.
>
> Signed-off-by: Justin Swartz <[email protected]>
> ---
> arch/mips/boot/dts/ralink/mt7621.dtsi | 28 +++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
> index 3ad4e2343..5a89f0b8c 100644
> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
> @@ -124,6 +124,34 @@ serial0: serial@c00 {
> pinctrl-0 = <&uart1_pins>;
> };
>
> + serial1: serial@d00 {
> + compatible = "ns16550a";
> + reg = <0xd00 0x100>;
> + reg-io-width = <4>;
> + reg-shift = <2>;
> + clocks = <&sysc MT7621_CLK_UART2>;
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>;
> + no-loopback-test;
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart2_pins>;
As already commented on patch [1/3], pin muxing is board specific. Please remove.
Also, is there any reason why you can't simply use the `interrupts-extended`
property instead of interrupt-parent and interrupts?
Regards,
Angelo
On 8.03.2024 11:44, AngeloGioacchino Del Regno wrote:
> Il 07/03/24 20:04, Justin Swartz ha scritto:
>> Add serial1 and serial2 nodes to define the existence of
>> the MT7621's second and third UARTs.
>>
>> Signed-off-by: Justin Swartz <[email protected]>
>> ---
>> arch/mips/boot/dts/ralink/mt7621.dtsi | 28 +++++++++++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
>> index 3ad4e2343..5a89f0b8c 100644
>> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
>> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
>> @@ -124,6 +124,34 @@ serial0: serial@c00 {
>> pinctrl-0 = <&uart1_pins>;
>> };
>> + serial1: serial@d00 {
>> + compatible = "ns16550a";
>> + reg = <0xd00 0x100>;
>> + reg-io-width = <4>;
>> + reg-shift = <2>;
>> + clocks = <&sysc MT7621_CLK_UART2>;
>> + interrupt-parent = <&gic>;
>> + interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>;
>> + no-loopback-test;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&uart2_pins>;
>
> As already commented on patch [1/3], pin muxing is board specific. Please remove.
>
> Also, is there any reason why you can't simply use the `interrupts-extended`
> property instead of interrupt-parent and interrupts?
I'm looking at the documentation [1], it seems to be useful when multiple
interrupt parents need to be defined on a node. I'd continue using
interrupt-parent and interrupts in this case.
[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
Arınç
Hi Angelo
On 2024-03-08 10:41, AngeloGioacchino Del Regno wrote:
> Il 07/03/24 20:04, Justin Swartz ha scritto:
>> Add missing pinctrl-name and pinctrl-0 properties to declare
>> that the uart1_pins group is associated with serial0.
>>
>> Signed-off-by: Justin Swartz <[email protected]>
>> ---
>> arch/mips/boot/dts/ralink/mt7621.dtsi | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi
>> b/arch/mips/boot/dts/ralink/mt7621.dtsi
>> index 35a10258f..dca415fdd 100644
>> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
>> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
>> @@ -123,6 +123,9 @@ serial0: serial@c00 {
>> reg-shift = <2>;
>> reg-io-width = <4>;
>> no-loopback-test;
>> +
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&uart1_pins>;
>> };
>> spi0: spi@b00 {
>
> The pins are muxed and can be either UART, or some other function that
> is supported by the mux: this means that the pinctrl-xxx properties
> shall
> *not* go into the SoC dtsi file, but in board dts files instead.
>
> Said differently: the usage of the UART pins is board-specific, not
> SoC-wide.
Thanks for the explanation. I agree that the pinctrl properties
would make more sense in a serial node extension in a board's dts,
but my reason for including them in the SoC's dtsi is due to the
precedent set with these existing nodes:
i2c
spi0
mmc
ethernet
pcie
There is also a default function declared for each of the pin
groups defined under the pinctrl node. These functions co-incide
with what is intended for each of those device nodes to function
correctly, rather than in the alternative GPIO-mode.
So I thought that sticking with that existing pattern would get
the least resistance from the community.
I can imagine how moving the pinctrl node to the board dts, and
then moving all of the pinctrl properties associated with device
nodes to their board dts references could be a better separation
logically.
What do you recommend?
Regards
Justin
On 8.03.2024 15:40, Justin Swartz wrote:
> Hi Angelo
>
> On 2024-03-08 10:41, AngeloGioacchino Del Regno wrote:
>> Il 07/03/24 20:04, Justin Swartz ha scritto:
>>> Add missing pinctrl-name and pinctrl-0 properties to declare
>>> that the uart1_pins group is associated with serial0.
>>>
>>> Signed-off-by: Justin Swartz <[email protected]>
>>> ---
>>> arch/mips/boot/dts/ralink/mt7621.dtsi | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
>>> index 35a10258f..dca415fdd 100644
>>> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
>>> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
>>> @@ -123,6 +123,9 @@ serial0: serial@c00 {
>>> reg-shift = <2>;
>>> reg-io-width = <4>;
>>> no-loopback-test;
>>> +
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&uart1_pins>;
>>> };
>>> spi0: spi@b00 {
>>
>> The pins are muxed and can be either UART, or some other function that
>> is supported by the mux: this means that the pinctrl-xxx properties shall
>> *not* go into the SoC dtsi file, but in board dts files instead.
>>
>> Said differently: the usage of the UART pins is board-specific, not SoC-wide.
>
> Thanks for the explanation. I agree that the pinctrl properties
> would make more sense in a serial node extension in a board's dts,
> but my reason for including them in the SoC's dtsi is due to the
> precedent set with these existing nodes:
>
> i2c
> spi0
> mmc
> ethernet
> pcie
>
> There is also a default function declared for each of the pin
> groups defined under the pinctrl node. These functions co-incide
> with what is intended for each of those device nodes to function
> correctly, rather than in the alternative GPIO-mode.
>
> So I thought that sticking with that existing pattern would get
> the least resistance from the community.
>
> I can imagine how moving the pinctrl node to the board dts, and
> then moving all of the pinctrl properties associated with device
> nodes to their board dts references could be a better separation
> logically.
>
> What do you recommend?
As a maintainer, this is the logic I follow on the MT7621 device tree
source files regarding the description of pin groups:
- Claim the relevant pin group with the default function (pinctrl-names &
pinctrl-0) on the node that describes a component of the SoC.
- Keep the node disabled and leave it to the board DTS file to enable it.
I don't disable serial@c00 as we can expect every board to use it. Same
goes for ethernet@1e100000. Some boards use the pins on the rgmii2 pin
group as GPIO, so the pinctrl-0 property on ethernet@1e100000 is
overwritten on the board DTS file without rgmii2_pins listed.
So I'm fine with this patch as is.
Reviewed-by: Arınç ÜNAL <[email protected]>
Arınç
On 7.03.2024 22:04, Justin Swartz wrote:
> Add serial1 and serial2 nodes to define the existence of
> the MT7621's second and third UARTs.
>
> Signed-off-by: Justin Swartz <[email protected]>
> ---
> arch/mips/boot/dts/ralink/mt7621.dtsi | 28 +++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
> index 3ad4e2343..5a89f0b8c 100644
> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
> @@ -124,6 +124,34 @@ serial0: serial@c00 {
> pinctrl-0 = <&uart1_pins>;
> };
>
> + serial1: serial@d00 {
> + compatible = "ns16550a";
> + reg = <0xd00 0x100>;
> + reg-io-width = <4>;
> + reg-shift = <2>;
> + clocks = <&sysc MT7621_CLK_UART2>;
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>;
> + no-loopback-test;
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart2_pins>;
> + status = "disabled";
> + };
I would group this:
serial1: serial@d00 {
compatible = "ns16550a";
reg = <0xd00 0x100>;
reg-io-width = <4>;
reg-shift = <2>;
clocks = <&sysc MT7621_CLK_UART2>;
interrupt-parent = <&gic>;
interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>;
no-loopback-test;
pinctrl-names = "default";
pinctrl-0 = <&uart2_pins>;
status = "disabled";
};
Same goes for patch 2.
Arınç
On 2024-03-08 15:50, Arınç ÜNAL wrote:
> On 7.03.2024 22:04, Justin Swartz wrote:
>> Add serial1 and serial2 nodes to define the existence of
>> the MT7621's second and third UARTs.
>>
>> Signed-off-by: Justin Swartz <[email protected]>
>> ---
>> arch/mips/boot/dts/ralink/mt7621.dtsi | 28
>> +++++++++++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi
>> b/arch/mips/boot/dts/ralink/mt7621.dtsi
>> index 3ad4e2343..5a89f0b8c 100644
>> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
>> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
>> @@ -124,6 +124,34 @@ serial0: serial@c00 {
>> pinctrl-0 = <&uart1_pins>;
>> };
>> + serial1: serial@d00 {
>> + compatible = "ns16550a";
>> + reg = <0xd00 0x100>;
>> + reg-io-width = <4>;
>> + reg-shift = <2>;
>> + clocks = <&sysc MT7621_CLK_UART2>;
>> + interrupt-parent = <&gic>;
>> + interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>;
>> + no-loopback-test;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&uart2_pins>;
>> + status = "disabled";
>> + };
>
> I would group this:
>
> serial1: serial@d00 {
> compatible = "ns16550a";
> reg = <0xd00 0x100>;
>
> reg-io-width = <4>;
> reg-shift = <2>;
>
> clocks = <&sysc MT7621_CLK_UART2>;
>
> interrupt-parent = <&gic>;
> interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>;
>
> no-loopback-test;
>
> pinctrl-names = "default";
> pinctrl-0 = <&uart2_pins>;
>
> status = "disabled";
> };
>
> Same goes for patch 2.
Thanks for the example.
Regards
Justin