[1/2] ARM: dts: BCM5301X: Specify serial console parameters

Message ID 20170314075830.2247-1-zajec5@gmail.com
State New
Headers show

Commit Message

Rafał Miłecki March 14, 2017, 7:58 a.m.
From: Rafał Miłecki <rafal@milecki.pl>

This adds baud rate, parity & number of data bits. It's required to get
serial working correctly.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 arch/arm/boot/dts/bcm5301x.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Andrew Lunn March 14, 2017, 12:26 p.m. | #1
On Tue, Mar 14, 2017 at 08:58:29AM +0100, Rafa?? Mi??ecki wrote:
> From: Rafa?? Mi??ecki <rafal@milecki.pl>
> 
> This adds baud rate, parity & number of data bits. It's required to get
> serial working correctly.
> 
> Signed-off-by: Rafa?? Mi??ecki <rafal@milecki.pl>
> ---
>  arch/arm/boot/dts/bcm5301x.dtsi | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
> index 8fd1ef9f0c2d..468107166a6f 100644
> --- a/arch/arm/boot/dts/bcm5301x.dtsi
> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
> @@ -18,8 +18,12 @@
>  / {
>  	interrupt-parent = <&gic>;
>  
> +	aliases {
> +		serial0 = &uart0;
> +	};
> +
>  	chosen {
> -		stdout-path = &uart0;
> +		stdout-path = "serial0:115200n8";
>  	};

Hi Rafal

The alias is fine. But putting the stdout-path here is unusual. Which
serial port is used for console is board specific, where as
bcm5301x.dtsi is very generic, it describes the SoC, not a board.  If
you look at other .dtsi files, those that specify stdout-path contain
properties which are common to a range of similar boards.

	   Andrew
Rafał Miłecki March 14, 2017, 12:32 p.m. | #2
On 03/14/2017 01:26 PM, Andrew Lunn wrote:
> On Tue, Mar 14, 2017 at 08:58:29AM +0100, Rafa?? Mi??ecki wrote:
>> From: Rafa?? Mi??ecki <rafal@milecki.pl>
>>
>> This adds baud rate, parity & number of data bits. It's required to get
>> serial working correctly.
>>
>> Signed-off-by: Rafa?? Mi??ecki <rafal@milecki.pl>
>> ---
>>  arch/arm/boot/dts/bcm5301x.dtsi | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
>> index 8fd1ef9f0c2d..468107166a6f 100644
>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>> @@ -18,8 +18,12 @@
>>  / {
>>  	interrupt-parent = <&gic>;
>>
>> +	aliases {
>> +		serial0 = &uart0;
>> +	};
>> +
>>  	chosen {
>> -		stdout-path = &uart0;
>> +		stdout-path = "serial0:115200n8";
>>  	};
>
> Hi Rafal
>
> The alias is fine. But putting the stdout-path here is unusual. Which
> serial port is used for console is board specific, where as
> bcm5301x.dtsi is very generic, it describes the SoC, not a board.  If
> you look at other .dtsi files, those that specify stdout-path contain
> properties which are common to a range of similar boards.

So far I've never seen any board using other uart. Also uart0 is disabled by
default and we enable it per board family (BCM4708 / BCM47081 / BCM4709 /
BCM47094). I think it's just more practical to have simple DTS that covers
99,9% boards by default and handled that margin of devices separately.

We already got a lot of duplicated code for enabling uart0, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b790d3b2943bf8e7e7bafe184008bd6451fe0bd

Jon Mason from Broadcom who knows this hardware and designs very well confirmed
it's a sane/safe assumption.
Rafał Miłecki March 14, 2017, 12:37 p.m. | #3
On 03/14/2017 01:32 PM, Rafał Miłecki wrote:
> On 03/14/2017 01:26 PM, Andrew Lunn wrote:
>> On Tue, Mar 14, 2017 at 08:58:29AM +0100, Rafa?? Mi??ecki wrote:
>>> From: Rafa?? Mi??ecki <rafal@milecki.pl>
>>>
>>> This adds baud rate, parity & number of data bits. It's required to get
>>> serial working correctly.
>>>
>>> Signed-off-by: Rafa?? Mi??ecki <rafal@milecki.pl>
>>> ---
>>>  arch/arm/boot/dts/bcm5301x.dtsi | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
>>> index 8fd1ef9f0c2d..468107166a6f 100644
>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>> @@ -18,8 +18,12 @@
>>>  / {
>>>      interrupt-parent = <&gic>;
>>>
>>> +    aliases {
>>> +        serial0 = &uart0;
>>> +    };
>>> +
>>>      chosen {
>>> -        stdout-path = &uart0;
>>> +        stdout-path = "serial0:115200n8";
>>>      };
>>
>> Hi Rafal
>>
>> The alias is fine. But putting the stdout-path here is unusual. Which
>> serial port is used for console is board specific, where as
>> bcm5301x.dtsi is very generic, it describes the SoC, not a board.  If
>> you look at other .dtsi files, those that specify stdout-path contain
>> properties which are common to a range of similar boards.
>
> So far I've never seen any board using other uart. Also uart0 is disabled by
> default and we enable it per board family (BCM4708 / BCM47081 / BCM4709 /
> BCM47094). I think it's just more practical to have simple DTS that covers
> 99,9% boards by default and handled that margin of devices separately.
>
> We already got a lot of duplicated code for enabling uart0, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b790d3b2943bf8e7e7bafe184008bd6451fe0bd
>
> Jon Mason from Broadcom who knows this hardware and designs very well confirmed
> it's a sane/safe assumption.

That was supposed to be "any other uart or parameters".
Andrew Lunn March 14, 2017, 12:51 p.m. | #4
> >The alias is fine. But putting the stdout-path here is unusual. Which
> >serial port is used for console is board specific, where as
> >bcm5301x.dtsi is very generic, it describes the SoC, not a board.  If
> >you look at other .dtsi files, those that specify stdout-path contain
> >properties which are common to a range of similar boards.
> 
> So far I've never seen any board using other uart. Also uart0 is disabled by
> default and we enable it per board family (BCM4708 / BCM47081 / BCM4709 /
> BCM47094). I think it's just more practical to have simple DTS that covers
> 99,9% boards by default and handled that margin of devices separately.
> 
> We already got a lot of duplicated code for enabling uart0, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b790d3b2943bf8e7e7bafe184008bd6451fe0bd
> 
> Jon Mason from Broadcom who knows this hardware and designs very well confirmed
> it's a sane/safe assumption.

I don't disagree with you, it does make sense. But it is just
different to every other SoC .dtsi file Linux has. And we try to avoid
things being different.

It is down to Florian to decide, as maintainer.

   Andrew
Jon Mason March 14, 2017, 2:53 p.m. | #5
On Tue, Mar 14, 2017 at 8:32 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 03/14/2017 01:26 PM, Andrew Lunn wrote:
>>
>> On Tue, Mar 14, 2017 at 08:58:29AM +0100, Rafa?? Mi??ecki wrote:
>>>
>>> From: Rafa?? Mi??ecki <rafal@milecki.pl>
>>>
>>> This adds baud rate, parity & number of data bits. It's required to get
>>> serial working correctly.
>>>
>>> Signed-off-by: Rafa?? Mi??ecki <rafal@milecki.pl>
>>> ---
>>>  arch/arm/boot/dts/bcm5301x.dtsi | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>> index 8fd1ef9f0c2d..468107166a6f 100644
>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>> @@ -18,8 +18,12 @@
>>>  / {
>>>         interrupt-parent = <&gic>;
>>>
>>> +       aliases {
>>> +               serial0 = &uart0;
>>> +       };
>>> +
>>>         chosen {
>>> -               stdout-path = &uart0;
>>> +               stdout-path = "serial0:115200n8";
>>>         };
>>
>>
>> Hi Rafal
>>
>> The alias is fine. But putting the stdout-path here is unusual. Which
>> serial port is used for console is board specific, where as
>> bcm5301x.dtsi is very generic, it describes the SoC, not a board.  If
>> you look at other .dtsi files, those that specify stdout-path contain
>> properties which are common to a range of similar boards.
>
>
> So far I've never seen any board using other uart. Also uart0 is disabled by
> default and we enable it per board family (BCM4708 / BCM47081 / BCM4709 /
> BCM47094). I think it's just more practical to have simple DTS that covers
> 99,9% boards by default and handled that margin of devices separately.
>
> We already got a lot of duplicated code for enabling uart0, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b790d3b2943bf8e7e7bafe184008bd6451fe0bd
>
> Jon Mason from Broadcom who knows this hardware and designs very well
> confirmed
> it's a sane/safe assumption.


It should work for all the boards.  The question is whether it should
be in the DTSI or DTS file.  This code replicates code I have in
arch/arm/boot/dts/bcm953012k.dts
So, either it should be removed or we should change all of the other
DTS files to have this entry.  My understanding is that the latter
approach is the acceptable one (which matches Andrew's comment), but I
can make the other changes if necessary.

Thanks,
Jon
Scott Branden March 14, 2017, 8:44 p.m. | #6
On 17-03-14 07:53 AM, Jon Mason wrote:
> On Tue, Mar 14, 2017 at 8:32 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 03/14/2017 01:26 PM, Andrew Lunn wrote:
>>>
>>> On Tue, Mar 14, 2017 at 08:58:29AM +0100, Rafa?? Mi??ecki wrote:
>>>>
>>>> From: Rafa?? Mi??ecki <rafal@milecki.pl>
>>>>
>>>> This adds baud rate, parity & number of data bits. It's required to get
>>>> serial working correctly.
>>>>
>>>> Signed-off-by: Rafa?? Mi??ecki <rafal@milecki.pl>
>>>> ---
>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>> index 8fd1ef9f0c2d..468107166a6f 100644
>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>> @@ -18,8 +18,12 @@
>>>>  / {
>>>>         interrupt-parent = <&gic>;
>>>>
>>>> +       aliases {
>>>> +               serial0 = &uart0;
>>>> +       };
>>>> +
>>>>         chosen {
>>>> -               stdout-path = &uart0;
>>>> +               stdout-path = "serial0:115200n8";
>>>>         };
>>>
>>>
>>> Hi Rafal
>>>
>>> The alias is fine. But putting the stdout-path here is unusual. Which
>>> serial port is used for console is board specific, where as
>>> bcm5301x.dtsi is very generic, it describes the SoC, not a board.  If
>>> you look at other .dtsi files, those that specify stdout-path contain
>>> properties which are common to a range of similar boards.
>>
>>
>> So far I've never seen any board using other uart. Also uart0 is disabled by
>> default and we enable it per board family (BCM4708 / BCM47081 / BCM4709 /
>> BCM47094). I think it's just more practical to have simple DTS that covers
>> 99,9% boards by default and handled that margin of devices separately.
>>
>> We already got a lot of duplicated code for enabling uart0, see:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b790d3b2943bf8e7e7bafe184008bd6451fe0bd
>>
>> Jon Mason from Broadcom who knows this hardware and designs very well
>> confirmed
>> it's a sane/safe assumption.
>
>
> It should work for all the boards.  The question is whether it should
> be in the DTSI or DTS file.  This code replicates code I have in
> arch/arm/boot/dts/bcm953012k.dts
> So, either it should be removed or we should change all of the other
> DTS files to have this entry.  My understanding is that the latter
> approach is the acceptable one (which matches Andrew's comment), but I
> can make the other changes if necessary.
dtsi file is fine to handle 99% of the use cases and not duplicate the 
entry.  This is what we do for newer SoCs (such as Cygnus) where the 
console will always be a particular UART (as the BootROM already uses a 
certain UART the boards are designed to use that UART as the console 
always).  For bcm953012 a similar situation exists as a common 
bootloader is used across boards.  So, the uart used as the terminal 
will always be the same.

I don't see a good reason for duplicating the code in every dts file.

Acked-by: Scott Branden <scott.branden@broadcom.com>
>
> Thanks,
> Jon
>
Jon Mason March 14, 2017, 9:18 p.m. | #7
On Tue, Mar 14, 2017 at 4:44 PM, Scott Branden
<scott.branden@broadcom.com> wrote:
>
>
> On 17-03-14 07:53 AM, Jon Mason wrote:
>>
>> On Tue, Mar 14, 2017 at 8:32 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>>>
>>> On 03/14/2017 01:26 PM, Andrew Lunn wrote:
>>>>
>>>>
>>>> On Tue, Mar 14, 2017 at 08:58:29AM +0100, Rafa?? Mi??ecki wrote:
>>>>>
>>>>>
>>>>> From: Rafa?? Mi??ecki <rafal@milecki.pl>
>>>>>
>>>>> This adds baud rate, parity & number of data bits. It's required to get
>>>>> serial working correctly.
>>>>>
>>>>> Signed-off-by: Rafa?? Mi??ecki <rafal@milecki.pl>
>>>>> ---
>>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 6 +++++-
>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> index 8fd1ef9f0c2d..468107166a6f 100644
>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> @@ -18,8 +18,12 @@
>>>>>  / {
>>>>>         interrupt-parent = <&gic>;
>>>>>
>>>>> +       aliases {
>>>>> +               serial0 = &uart0;
>>>>> +       };
>>>>> +
>>>>>         chosen {
>>>>> -               stdout-path = &uart0;
>>>>> +               stdout-path = "serial0:115200n8";
>>>>>         };
>>>>
>>>>
>>>>
>>>> Hi Rafal
>>>>
>>>> The alias is fine. But putting the stdout-path here is unusual. Which
>>>> serial port is used for console is board specific, where as
>>>> bcm5301x.dtsi is very generic, it describes the SoC, not a board.  If
>>>> you look at other .dtsi files, those that specify stdout-path contain
>>>> properties which are common to a range of similar boards.
>>>
>>>
>>>
>>> So far I've never seen any board using other uart. Also uart0 is disabled
>>> by
>>> default and we enable it per board family (BCM4708 / BCM47081 / BCM4709 /
>>> BCM47094). I think it's just more practical to have simple DTS that
>>> covers
>>> 99,9% boards by default and handled that margin of devices separately.
>>>
>>> We already got a lot of duplicated code for enabling uart0, see:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b790d3b2943bf8e7e7bafe184008bd6451fe0bd
>>>
>>> Jon Mason from Broadcom who knows this hardware and designs very well
>>> confirmed
>>> it's a sane/safe assumption.
>>
>>
>>
>> It should work for all the boards.  The question is whether it should
>> be in the DTSI or DTS file.  This code replicates code I have in
>> arch/arm/boot/dts/bcm953012k.dts
>> So, either it should be removed or we should change all of the other
>> DTS files to have this entry.  My understanding is that the latter
>> approach is the acceptable one (which matches Andrew's comment), but I
>> can make the other changes if necessary.
>
> dtsi file is fine to handle 99% of the use cases and not duplicate the
> entry.  This is what we do for newer SoCs (such as Cygnus) where the console
> will always be a particular UART (as the BootROM already uses a certain UART
> the boards are designed to use that UART as the console always).  For
> bcm953012 a similar situation exists as a common bootloader is used across
> boards.  So, the uart used as the terminal will always be the same.

Unfortunately, there is not a common bootloader.  CFE is used for the
4708/9 boards, and u-boot is used for the 5301x boards.

Thanks,
Jon

> I don't see a good reason for duplicating the code in every dts file.
>
> Acked-by: Scott Branden <scott.branden@broadcom.com>
>>
>>
>> Thanks,
>> Jon
>>
>
Scott Branden March 14, 2017, 9:28 p.m. | #8
On 17-03-14 02:18 PM, Jon Mason wrote:
> On Tue, Mar 14, 2017 at 4:44 PM, Scott Branden
> <scott.branden@broadcom.com> wrote:
>>
>>
>> On 17-03-14 07:53 AM, Jon Mason wrote:
>>>
>>> On Tue, Mar 14, 2017 at 8:32 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>>>>
>>>> On 03/14/2017 01:26 PM, Andrew Lunn wrote:
>>>>>
>>>>>
>>>>> On Tue, Mar 14, 2017 at 08:58:29AM +0100, Rafa?? Mi??ecki wrote:
>>>>>>
>>>>>>
>>>>>> From: Rafa?? Mi??ecki <rafal@milecki.pl>
>>>>>>
>>>>>> This adds baud rate, parity & number of data bits. It's required to get
>>>>>> serial working correctly.
>>>>>>
>>>>>> Signed-off-by: Rafa?? Mi??ecki <rafal@milecki.pl>
>>>>>> ---
>>>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 6 +++++-
>>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> index 8fd1ef9f0c2d..468107166a6f 100644
>>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> @@ -18,8 +18,12 @@
>>>>>>  / {
>>>>>>         interrupt-parent = <&gic>;
>>>>>>
>>>>>> +       aliases {
>>>>>> +               serial0 = &uart0;
>>>>>> +       };
>>>>>> +
>>>>>>         chosen {
>>>>>> -               stdout-path = &uart0;
>>>>>> +               stdout-path = "serial0:115200n8";
>>>>>>         };
>>>>>
>>>>>
>>>>>
>>>>> Hi Rafal
>>>>>
>>>>> The alias is fine. But putting the stdout-path here is unusual. Which
>>>>> serial port is used for console is board specific, where as
>>>>> bcm5301x.dtsi is very generic, it describes the SoC, not a board.  If
>>>>> you look at other .dtsi files, those that specify stdout-path contain
>>>>> properties which are common to a range of similar boards.
>>>>
>>>>
>>>>
>>>> So far I've never seen any board using other uart. Also uart0 is disabled
>>>> by
>>>> default and we enable it per board family (BCM4708 / BCM47081 / BCM4709 /
>>>> BCM47094). I think it's just more practical to have simple DTS that
>>>> covers
>>>> 99,9% boards by default and handled that margin of devices separately.
>>>>
>>>> We already got a lot of duplicated code for enabling uart0, see:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b790d3b2943bf8e7e7bafe184008bd6451fe0bd
>>>>
>>>> Jon Mason from Broadcom who knows this hardware and designs very well
>>>> confirmed
>>>> it's a sane/safe assumption.
>>>
>>>
>>>
>>> It should work for all the boards.  The question is whether it should
>>> be in the DTSI or DTS file.  This code replicates code I have in
>>> arch/arm/boot/dts/bcm953012k.dts
>>> So, either it should be removed or we should change all of the other
>>> DTS files to have this entry.  My understanding is that the latter
>>> approach is the acceptable one (which matches Andrew's comment), but I
>>> can make the other changes if necessary.
>>
>> dtsi file is fine to handle 99% of the use cases and not duplicate the
>> entry.  This is what we do for newer SoCs (such as Cygnus) where the console
>> will always be a particular UART (as the BootROM already uses a certain UART
>> the boards are designed to use that UART as the console always).  For
>> bcm953012 a similar situation exists as a common bootloader is used across
>> boards.  So, the uart used as the terminal will always be the same.
>
> Unfortunately, there is not a common bootloader.  CFE is used for the
> 4708/9 boards, and u-boot is used for the 5301x boards.
OK, little different situation not having a boot rom with console output 
enforcing all designs to use the same UART.
Do both of the bootloaders happen to use the same UART and all the 
boards are designed to use the same UART?
>
> Thanks,
> Jon
>
>> I don't see a good reason for duplicating the code in every dts file.
>>
>> Acked-by: Scott Branden <scott.branden@broadcom.com>
>>>
>>>
>>> Thanks,
>>> Jon
>>>
>>
Andrew Lunn March 14, 2017, 9:51 p.m. | #9
> >Unfortunately, there is not a common bootloader.  CFE is used for the
> >4708/9 boards, and u-boot is used for the 5301x boards.
> OK, little different situation not having a boot rom with console
> output enforcing all designs to use the same UART.
> Do both of the bootloaders happen to use the same UART and all the
> boards are designed to use the same UART?

You need to be a little bit careful here. Some vendor could use a
different UART in there design, use some other bootloader,
etc. Somebody in the community might want to throw out uboot and use
barebox etc....

Device tree does allow .dtsi properties to be over written, so it is
not an issue in practice.

    Andrew

Patch

diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
index 8fd1ef9f0c2d..468107166a6f 100644
--- a/arch/arm/boot/dts/bcm5301x.dtsi
+++ b/arch/arm/boot/dts/bcm5301x.dtsi
@@ -18,8 +18,12 @@ 
 / {
 	interrupt-parent = <&gic>;
 
+	aliases {
+		serial0 = &uart0;
+	};
+
 	chosen {
-		stdout-path = &uart0;
+		stdout-path = "serial0:115200n8";
 	};
 
 	chipcommonA {