[PATCH 1/2] mips: dts: ralink: mt7621: associate uart1_pins with serial0

[PATCH 1/2] mips: dts: ralink: mt7621: associate uart1_pins with serial0

[Justin Swartz]

Add pinctrl-name and pinctrl-0 properties to declare
that the uart1_pins group is associated with serial0.

Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
---
 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 {
-- 

[PATCH 2/2] mips: dts: ralink: mt7621: add serial1 and serial2 nodes

[Justin Swartz]

Add serial1 and serial2 nodes to define the existence of
UART1 and UART2.

Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
---
 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";
 
-- 

Re: [PATCH 2/2] mips: dts: ralink: mt7621: add serial1 and serial2 nodes

[Sergio Paracuellos]

Hi Justin,

On Wed, Mar 6, 2024 at 9:11 PM Justin Swartz
<justin.swartz@risingedge.co.za> wrote:
>
> Add serial1 and serial2 nodes to define the existence of
> UART1 and UART2.
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
> ---
>  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

Re: [PATCH 1/2] mips: dts: ralink: mt7621: associate uart1_pins with serial0

[Sergio Paracuellos]

On Wed, Mar 6, 2024 at 9:11 PM Justin Swartz
<justin.swartz@risingedge.co.za> wrote:
>
> Add pinctrl-name and pinctrl-0 properties to declare
> that the uart1_pins group is associated with serial0.
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
> ---
>  arch/mips/boot/dts/ralink/mt7621.dtsi | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Thanks,
    Sergio Paracuellos

Re: [PATCH 2/2] mips: dts: ralink: mt7621: add serial1 and serial2 nodes

[Justin Swartz]

Hi Sergio

On 2024-03-07 12:04, Sergio Paracuellos wrote:
> Hi Justin,
> 
> On Wed, Mar 6, 2024 at 9:11 PM Justin Swartz
> <justin.swartz@risingedge.co.za> wrote:
>> 
>> Add serial1 and serial2 nodes to define the existence of
>> UART1 and UART2.
>> 
>> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
>> ---
>>  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

Re: [PATCH 2/2] mips: dts: ralink: mt7621: add serial1 and serial2 nodes

[Sergio Paracuellos]

Hi Justin,

On Thu, Mar 7, 2024 at 4:15 PM Justin Swartz
<justin.swartz@risingedge.co.za> 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
> > <justin.swartz@risingedge.co.za> wrote:
> >>
> >> Add serial1 and serial2 nodes to define the existence of
> >> UART1 and UART2.
> >>
> >> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
> >> ---
> >>  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

[PATCH v2 1/3] mips: dts: ralink: mt7621: associate uart1_pins with serial0

[Justin Swartz]

Add missing pinctrl-name and pinctrl-0 properties to declare
that the uart1_pins group is associated with serial0.

Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
---
 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 {
-- 

[PATCH v2 2/3] mips: dts: ralink: mt7621: reorder serial0 properties

[Justin Swartz]

Reorder serial0 properties according to the guidelines laid
out in Documentation/devicetree/bindings/dts-coding-style.rst

Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
---
 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>;
 		};
-- 

[PATCH v2 3/3] mips: dts: ralink: mt7621: add serial1 and serial2 nodes

[Justin Swartz]

Add serial1 and serial2 nodes to define the existence of
the MT7621's second and third UARTs.

Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
---
 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";
 
-- 

Re: [PATCH v2 1/3] mips: dts: ralink: mt7621: associate uart1_pins with serial0

[Sergio Paracuellos]

On Thu, Mar 7, 2024 at 8:05 PM Justin Swartz
<justin.swartz@risingedge.co.za> 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 <justin.swartz@risingedge.co.za>
> ---
>  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

Re: [PATCH v2 3/3] mips: dts: ralink: mt7621: add serial1 and serial2 nodes

[Sergio Paracuellos]

On Thu, Mar 7, 2024 at 8:05 PM Justin Swartz
<justin.swartz@risingedge.co.za> wrote:
>
> Add serial1 and serial2 nodes to define the existence of
> the MT7621's second and third UARTs.
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
> ---
>  arch/mips/boot/dts/ralink/mt7621.dtsi | 28 +++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

Acked-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Thanks,
    Sergio Paracuellos

Re: [PATCH v2 2/3] mips: dts: ralink: mt7621: reorder serial0 properties

[Sergio Paracuellos]

On Thu, Mar 7, 2024 at 8:05 PM Justin Swartz
<justin.swartz@risingedge.co.za> wrote:
>
> Reorder serial0 properties according to the guidelines laid
> out in Documentation/devicetree/bindings/dts-coding-style.rst
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
> ---
>  arch/mips/boot/dts/ralink/mt7621.dtsi | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

Acked-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Thanks,
    Sergio Paracuellos

Re: [PATCH v2 1/3] mips: dts: ralink: mt7621: associate uart1_pins with serial0

[Sergio Paracuellos]

On Fri, Mar 8, 2024 at 8:27 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> On Thu, Mar 7, 2024 at 8:05 PM Justin Swartz
> <justin.swartz@risingedge.co.za> 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 <justin.swartz@risingedge.co.za>
> > ---
> >  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 <sergio.paracuellos@gmail.com>

Thanks,
    Sergio Paracuellos

Re: [PATCH v2 1/3] mips: dts: ralink: mt7621: associate uart1_pins with serial0

[AngeloGioacchino Del Regno]

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 <justin.swartz@risingedge.co.za>
> ---
>   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

Re: [PATCH v2 2/3] mips: dts: ralink: mt7621: reorder serial0 properties

[AngeloGioacchino Del Regno]

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 <justin.swartz@risingedge.co.za>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v2 3/3] mips: dts: ralink: mt7621: add serial1 and serial2 nodes

[AngeloGioacchino Del Regno]

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 <justin.swartz@risingedge.co.za>
> ---
>   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

Re: [PATCH v2 3/3] mips: dts: ralink: mt7621: add serial1 and serial2 nodes

[Arınç ÜNAL]

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 <justin.swartz@risingedge.co.za>
>> ---
>>   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ç

Re: [PATCH v2 1/3] mips: dts: ralink: mt7621: associate uart1_pins with serial0

[Justin Swartz]

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 <justin.swartz@risingedge.co.za>
>> ---
>>   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

Re: [PATCH v2 1/3] mips: dts: ralink: mt7621: associate uart1_pins with serial0

[Arınç ÜNAL]

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 <justin.swartz@risingedge.co.za>
>>> ---
>>>   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 <arinc.unal@arinc9.com>

Arınç

Re: [PATCH v2 3/3] mips: dts: ralink: mt7621: add serial1 and serial2 nodes

[Arınç ÜNAL]

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 <justin.swartz@risingedge.co.za>
> ---
>   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ç

Re: [PATCH v2 3/3] mips: dts: ralink: mt7621: add serial1 and serial2 nodes

[Justin Swartz]

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 <justin.swartz@risingedge.co.za>
>> ---
>>   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