mbox series

[v3,0/3] serial: Support a disabled serial port

Message ID 20241120153554.860710-1-sjg@chromium.org
Headers show
Series serial: Support a disabled serial port | expand

Message

Simon Glass Nov. 20, 2024, 3:35 p.m. UTC
This series provides a way to tell a serial UART that it can't actually
work, at runtime. The main motivation is to deal with a coreboot feature
where it does not provide UART details in the sysinfo structure unless
the UART is also enabled in coreboot.

Attempts to introduce a way to enable a silent UART in coreboot have
lead to a large amount of discussion but no result, sadly.

This series reworks a patch sent last year, which was not quite ready to
be applied. A coreboot user (on irc) hit this problem, so it needs to be
resolved.

Link: https://patchwork.ozlabs.org/project/uboot/patch/20231101180447.99361-1-sjg@chromium.org/

Changes in v3:
- Put the feature behind a Kconfig
- Move the feature to the serial uclass, so any serial driver can use it

Changes in v2:
- Drop RFC tag since there were no comments

Simon Glass (3):
  serial: Allow a serial port to be silent disabled
  serial: ns16550: Avoid probing hardware when disabled
  x86: coreboot: Make use of disabled console

 arch/x86/cpu/coreboot/Kconfig    |  1 +
 drivers/serial/Kconfig           |  9 +++++++++
 drivers/serial/ns16550.c         |  3 ++-
 drivers/serial/serial-uclass.c   | 19 ++++++++++++++++++-
 drivers/serial/serial_coreboot.c |  2 ++
 include/serial.h                 | 25 +++++++++++++++++++++++++
 6 files changed, 57 insertions(+), 2 deletions(-)

Comments

Caleb Connolly Nov. 20, 2024, 3:56 p.m. UTC | #1
Hi Simon,

On 20/11/2024 16:35, Simon Glass wrote:
> This series provides a way to tell a serial UART that it can't actually
> work, at runtime. The main motivation is to deal with a coreboot feature
> where it does not provide UART details in the sysinfo structure unless
> the UART is also enabled in coreboot.

Why is the UART driver probed if coreboot doesn't provide the necessary
info for it?

Couldn't you disable CONFIG_REQUIRE_SERIAL_CONSOLE and skip probing it?

Kind regards,
> 
> Attempts to introduce a way to enable a silent UART in coreboot have
> lead to a large amount of discussion but no result, sadly.
> 
> This series reworks a patch sent last year, which was not quite ready to
> be applied. A coreboot user (on irc) hit this problem, so it needs to be
> resolved.
> 
> Link: https://patchwork.ozlabs.org/project/uboot/patch/20231101180447.99361-1-sjg@chromium.org/
> 
> Changes in v3:
> - Put the feature behind a Kconfig
> - Move the feature to the serial uclass, so any serial driver can use it
> 
> Changes in v2:
> - Drop RFC tag since there were no comments
> 
> Simon Glass (3):
>   serial: Allow a serial port to be silent disabled
>   serial: ns16550: Avoid probing hardware when disabled
>   x86: coreboot: Make use of disabled console
> 
>  arch/x86/cpu/coreboot/Kconfig    |  1 +
>  drivers/serial/Kconfig           |  9 +++++++++
>  drivers/serial/ns16550.c         |  3 ++-
>  drivers/serial/serial-uclass.c   | 19 ++++++++++++++++++-
>  drivers/serial/serial_coreboot.c |  2 ++
>  include/serial.h                 | 25 +++++++++++++++++++++++++
>  6 files changed, 57 insertions(+), 2 deletions(-)
>
Simon Glass Nov. 20, 2024, 4:03 p.m. UTC | #2
Hi Caleb,

On Wed, 20 Nov 2024 at 08:56, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
> Hi Simon,
>
> On 20/11/2024 16:35, Simon Glass wrote:
> > This series provides a way to tell a serial UART that it can't actually
> > work, at runtime. The main motivation is to deal with a coreboot feature
> > where it does not provide UART details in the sysinfo structure unless
> > the UART is also enabled in coreboot.
>
> Why is the UART driver probed if coreboot doesn't provide the necessary
> info for it?

U-Boot doesn't do well without some sort of serial device. That could
perhaps be improved. But the point here is that we need a single
U-Boot build which can start from coreboot, regardless of what
coreboot decides to do.

>
> Couldn't you disable CONFIG_REQUIRE_SERIAL_CONSOLE and skip probing it?

No, because I want it to work if the serial info is available.

[..]

Regards,
SImon
Caleb Connolly Nov. 20, 2024, 4:37 p.m. UTC | #3
On 20/11/2024 17:03, Simon Glass wrote:
> Hi Caleb,
> 
> On Wed, 20 Nov 2024 at 08:56, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>
>> Hi Simon,
>>
>> On 20/11/2024 16:35, Simon Glass wrote:
>>> This series provides a way to tell a serial UART that it can't actually
>>> work, at runtime. The main motivation is to deal with a coreboot feature
>>> where it does not provide UART details in the sysinfo structure unless
>>> the UART is also enabled in coreboot.
>>
>> Why is the UART driver probed if coreboot doesn't provide the necessary
>> info for it?
> 
> U-Boot doesn't do well without some sort of serial device. That could
> perhaps be improved. But the point here is that we need a single
> U-Boot build which can start from coreboot, regardless of what
> coreboot decides to do.

Right, I'm suggesting that at runtime you notice that coreboot didn't
provide a serial port definition (or gave you a bogus address or
something) and skip probing the serial port in that case.
> 
>>
>> Couldn't you disable CONFIG_REQUIRE_SERIAL_CONSOLE and skip probing it?
> 
> No, because I want it to work if the serial info is available.

This option doesn't disable the serial port, it just makes U-Boot not
panic if it can't find one. For example I enable this when building
U-Boot for the upstream supported Qualcomm phones since many don't
enable the serial port in their DTS.
> 
> [..]
> 
> Regards,
> SImon
Tom Rini Nov. 20, 2024, 6:10 p.m. UTC | #4
On Wed, Nov 20, 2024 at 05:37:35PM +0100, Caleb Connolly wrote:
> 
> 
> On 20/11/2024 17:03, Simon Glass wrote:
> > Hi Caleb,
> > 
> > On Wed, 20 Nov 2024 at 08:56, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 20/11/2024 16:35, Simon Glass wrote:
> >>> This series provides a way to tell a serial UART that it can't actually
> >>> work, at runtime. The main motivation is to deal with a coreboot feature
> >>> where it does not provide UART details in the sysinfo structure unless
> >>> the UART is also enabled in coreboot.
> >>
> >> Why is the UART driver probed if coreboot doesn't provide the necessary
> >> info for it?
> > 
> > U-Boot doesn't do well without some sort of serial device. That could
> > perhaps be improved. But the point here is that we need a single
> > U-Boot build which can start from coreboot, regardless of what
> > coreboot decides to do.
> 
> Right, I'm suggesting that at runtime you notice that coreboot didn't
> provide a serial port definition (or gave you a bogus address or
> something) and skip probing the serial port in that case.
> > 
> >>
> >> Couldn't you disable CONFIG_REQUIRE_SERIAL_CONSOLE and skip probing it?
> > 
> > No, because I want it to work if the serial info is available.
> 
> This option doesn't disable the serial port, it just makes U-Boot not
> panic if it can't find one. For example I enable this when building
> U-Boot for the upstream supported Qualcomm phones since many don't
> enable the serial port in their DTS.

Yes, this feels like an oddly coreboot specific solution to a generic
problem that we should be able to handle already I thought? Like a Pi
can have or not have a serial port enabled and still have video and we
use that. We might well need something specific to know the port is
disabled since I gather we aren't getting a devicetree that says status
= disabled in it. But this ought to be a common enough general case?