Re: [PATCH RFC 4/9] serial: serial_meson: add minimal non-DM driver

Tom Rini <[email protected]> Sat, 13 Sep 2025 09:51:00 -0600

Newsgroups	io.groups.u-boot-amlogic,org.u-boot-project.lists.u-boot
Message-ID	<20250913155100.GA67696@bill-the-cat>

On Thu, Sep 11, 2025 at 03:37:07PM +0000, Ferass El Hafidi wrote:
> On Wed Sep 10, 2025 at 8:30 PM UTC, Heinrich Schuchardt wrote:
> > On 9/10/25 22:14, Ferass El Hafidi wrote:
> >> On Wed Sep 10, 2025 at 7:53 PM UTC, Heinrich Schuchardt wrote:
> >>> <...>
> >>>> +static int meson_serial_getc(void)
> >>>> +{
> >>>> +	struct meson_uart *const uart = (struct meson_uart *)CONFIG_VAL(DEBUG_UART_BASE);
> >>>> +	uint32_t status = readl(&uart->status);
> >>>> +
> >>>> +	if (status & AML_UART_RX_EMPTY)
> >>>> +		return -EAGAIN;
> >>>> +
> >>>> +	if (status & AML_UART_ERR) {
> >>>> +		u32 val = readl(&uart->control);
> >>>> +
> >>>> +		/* Clear error */
> >>>> +		val |= AML_UART_CLR_ERR;
> >>>> +		writel(val, &uart->control);
> >>>> +		val &= ~AML_UART_CLR_ERR;
> >>>> +		writel(val, &uart->control);
> >>>> +
> >>>> +		/* Remove spurious byte from fifo */
> >>>> +		readl(&uart->rfifo);
> >>>> +		return -EIO;
> >>>> +	}
> >>>> +
> >>>> +	return readl(&uart->rfifo) & 0xff;
> >>>> +}
> >>>> +
> >>>> +static int meson_serial_tstc(void)
> >>>> +{
> >>>> +	struct meson_uart *const uart = (struct meson_uart *)CONFIG_VAL(DEBUG_UART_BASE);
> >>>
> >>> CONFIG_SPL_DEBUG_UART_BASE only exists for CONFIG_DEBUG_UART=y and
> >>> CONFIG_SPL=y. Shall all configurations depend on CONFIG_DEBUG_UART=y?
> >>>
> >>> Creating this dependency does not look right to me.
> >>>
> >> 
> >> All Amlogic defconfigs have CONFIG_DEBUG_UART enabled, but I can remove
> >> the dependency on CONFIG_DEBUG_UART_BASE if you think it doesn't look
> >> right.
> >
> > We should avoid the possibility of build failures for changed configuration.
> >
> > Will CONFIG_MESON_SERIAL depend on CONFIG_DEBUG_UART
> > or will CONFIG_ARCH_MESON select CONFIG_DEBUG_UART?
> 
> Then it might be best to not use DEBUG_UART_BASE.  I'll fix that for
> next revision.  Thanks!

The way we normally solve these kind of things, since they aren't
actually configurable (it's part of the chip itself) is just #define
them either in the driver header or some <asm/arch/..> header file.

-- 
Tom

signature.asc (application/pgp-signature, 228 B)