[U-Boot] [PATCH 1/4] serial: ns16550: da8xx (freon/primus) is not omap-like

[U-Boot] [PATCH 1/4] serial: ns16550: da8xx (freon/primus) is not omap-like

[Matthijs van Duin]

These are SoCs in the lineage of TI C6x DSPs, and as such have the same
uart as TI Keystone SoCs.

Signed-off-by: Matthijs van Duin <matthijsvanduin@gmail.com>
---
 drivers/serial/ns16550.c | 8 ++++----
 include/ns16550.h        | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index c702304e79bd..9cec58c887c8 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -36,7 +36,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #endif
 #endif /* !CONFIG_DM_SERIAL */
 
-#if defined(CONFIG_SOC_KEYSTONE)
+#if defined(CONFIG_SOC_KEYSTONE) || defined(CONFIG_SOC_DA8XX)
 #define UART_REG_VAL_PWREMU_MGMT_UART_DISABLE   0
 #define UART_REG_VAL_PWREMU_MGMT_UART_ENABLE ((1 << 14) | (1 << 13) | (1 << 0))
 #undef UART_MCRVAL
@@ -181,12 +181,12 @@ void NS16550_init(NS16550_t com_port, int baud_divisor)
 	serial_out(ns16550_getfcr(com_port), &com_port->fcr);
 	if (baud_divisor != -1)
 		NS16550_setbrg(com_port, baud_divisor);
-#if defined(CONFIG_ARCH_OMAP2PLUS) || defined(CONFIG_SOC_DA8XX)
+#if defined(CONFIG_ARCH_OMAP2PLUS)
 	/* /16 is proper to hit 115200 with 48MHz */
 	serial_out(0, &com_port->mdr1);
 #endif
-#if defined(CONFIG_SOC_KEYSTONE)
-	serial_out(UART_REG_VAL_PWREMU_MGMT_UART_ENABLE, &com_port->regC);
+#if defined(CONFIG_SOC_KEYSTONE) || defined(CONFIG_SOC_DA8XX)
+	serial_out(UART_REG_VAL_PWREMU_MGMT_UART_ENABLE, &com_port->pwr_mgmt);
 #endif
 }
 
diff --git a/include/ns16550.h b/include/ns16550.h
index 5fcbcd2e74e3..a988d297e144 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -71,7 +71,7 @@ struct NS16550 {
 	UART_REG(lsr);		/* 5 */
 	UART_REG(msr);		/* 6 */
 	UART_REG(spr);		/* 7 */
-#ifdef CONFIG_SOC_DA8XX
+#if defined(CONFIG_SOC_KEYSTONE) || defined(CONFIG_SOC_DA8XX)
 	UART_REG(reg8);		/* 8 */
 	UART_REG(reg9);		/* 9 */
 	UART_REG(revid1);	/* A */
-- 
2.11.0

[U-Boot] [U-Boot, 1/4] serial: ns16550: da8xx (freon/primus) is not omap-like

[Tom Rini]

On Mon, Jan 08, 2018 at 01:50:59PM +0100, Matthijs van Duin wrote:

> These are SoCs in the lineage of TI C6x DSPs, and as such have the same
> uart as TI Keystone SoCs.
> 
> Signed-off-by: Matthijs van Duin <matthijsvanduin@gmail.com>
> ---
>  drivers/serial/ns16550.c | 8 ++++----
>  include/ns16550.h        | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index c702304e79bd..9cec58c887c8 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -36,7 +36,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  #endif
>  #endif /* !CONFIG_DM_SERIAL */
>  
> -#if defined(CONFIG_SOC_KEYSTONE)
> +#if defined(CONFIG_SOC_KEYSTONE) || defined(CONFIG_SOC_DA8XX)
>  #define UART_REG_VAL_PWREMU_MGMT_UART_DISABLE   0
>  #define UART_REG_VAL_PWREMU_MGMT_UART_ENABLE ((1 << 14) | (1 << 13) | (1 << 0))
>  #undef UART_MCRVAL
> @@ -181,12 +181,12 @@ void NS16550_init(NS16550_t com_port, int baud_divisor)
>  	serial_out(ns16550_getfcr(com_port), &com_port->fcr);
>  	if (baud_divisor != -1)
>  		NS16550_setbrg(com_port, baud_divisor);
> -#if defined(CONFIG_ARCH_OMAP2PLUS) || defined(CONFIG_SOC_DA8XX)
> +#if defined(CONFIG_ARCH_OMAP2PLUS)
>  	/* /16 is proper to hit 115200 with 48MHz */
>  	serial_out(0, &com_port->mdr1);
>  #endif
> -#if defined(CONFIG_SOC_KEYSTONE)
> -	serial_out(UART_REG_VAL_PWREMU_MGMT_UART_ENABLE, &com_port->regC);
> +#if defined(CONFIG_SOC_KEYSTONE) || defined(CONFIG_SOC_DA8XX)
> +	serial_out(UART_REG_VAL_PWREMU_MGMT_UART_ENABLE, &com_port->pwr_mgmt);
>  #endif
>  }

This introduces a warning on all SOC_DA8XX systems because they do not
set CONFIG_SYS_NS16550_MEM32 so serial_out is writeb and
UART_REG_VAL_PWREMU_MGMT_UART_ENABLE is larger than a u8.  Are you able
to test SOC_DA8XX platforms?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180119/544e1e8e/attachment.sig>

[U-Boot] [U-Boot, 1/4] serial: ns16550: da8xx (freon/primus) is not omap-like

[Matthijs van Duin]

On 19 January 2018 at 16:26, Tom Rini <trini@konsulko.com> wrote:
> This introduces a warning on all SOC_DA8XX systems because they do not
> set CONFIG_SYS_NS16550_MEM32 so serial_out is writeb

Well that's a mistake. Its uart uses 32-bit registers, and 8-bit
access is not documented to be acceptable.

Apologies for forgetting to at least compile-test this for all
affected targets, I really should have done that.

> Are you able to test SOC_DA8XX platforms?

No, this was based on my knowledge that freon/primus SoCs
(OMAP-L1xx/AM1xxx/TMS320C674x/DA8xx) are architecturally members of
the TI C6x family and not remotely related to the OMAP (despite the
part code "OMAP-L1xx"), and I double-checked the technical reference
manual: their UART is identical to that on Keystone.

Matthijs

[U-Boot] [U-Boot, 1/4] serial: ns16550: da8xx (freon/primus) is not omap-like

[Tom Rini]

On Fri, Jan 19, 2018 at 04:39:15PM +0100, Matthijs van Duin wrote:
> On 19 January 2018 at 16:26, Tom Rini <trini@konsulko.com> wrote:
> > This introduces a warning on all SOC_DA8XX systems because they do not
> > set CONFIG_SYS_NS16550_MEM32 so serial_out is writeb
> 
> Well that's a mistake. Its uart uses 32-bit registers, and 8-bit
> access is not documented to be acceptable.
> 
> Apologies for forgetting to at least compile-test this for all
> affected targets, I really should have done that.
> 
> > Are you able to test SOC_DA8XX platforms?
> 
> No, this was based on my knowledge that freon/primus SoCs
> (OMAP-L1xx/AM1xxx/TMS320C674x/DA8xx) are architecturally members of
> the TI C6x family and not remotely related to the OMAP (despite the
> part code "OMAP-L1xx"), and I double-checked the technical reference
> manual: their UART is identical to that on Keystone.

OK.  For v2, if you can convert CONFIG_SYS_NS16550_MEM32 to Kconfig as
well I'd appreciate it.  I'll see about digging out and setting up one
of my da8xx platforms for a quick runtime test too.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180119/0071f9a5/attachment.sig>

[U-Boot] [U-Boot, 1/4] serial: ns16550: da8xx (freon/primus) is not omap-like

[Matthijs van Duin]

On 19 January 2018 at 16:41, Tom Rini <trini@konsulko.com> wrote:
> OK.  For v2, if you can convert CONFIG_SYS_NS16550_MEM32 to Kconfig as
> well I'd appreciate it.

I'm not hugely comfortable doing that, since that would affect even
more targets. To be honest, I don't understand why it even exists as a
separate var instead of just testing whether
CONFIG_SYS_NS16550_REG_SIZE is 4 or -4. Are there actually some crazy
targets that allocate 4 bytes per register yet fail if you use 32-bit
access?

Would you object hugely to me just implicitly setting
CONFIG_SYS_NS16550_MEM32 in ns16550.h for the c6x uart variant? I
think it makes a lot of sense to do so, since in that case the driver
itself explicitly depends on being able to write a value that doesn't
fit in a byte.

> I'll see about digging out and setting up one
> of my da8xx platforms for a quick runtime test too.

Thanks!

Matthijs

[U-Boot] [U-Boot, 1/4] serial: ns16550: da8xx (freon/primus) is not omap-like

[Tom Rini]

On Fri, Jan 19, 2018 at 05:39:32PM +0100, Matthijs van Duin wrote:
> On 19 January 2018 at 16:41, Tom Rini <trini@konsulko.com> wrote:
> > OK.  For v2, if you can convert CONFIG_SYS_NS16550_MEM32 to Kconfig as
> > well I'd appreciate it.
> 
> I'm not hugely comfortable doing that, since that would affect even
> more targets. To be honest, I don't understand why it even exists as a
> separate var instead of just testing whether
> CONFIG_SYS_NS16550_REG_SIZE is 4 or -4. Are there actually some crazy
> targets that allocate 4 bytes per register yet fail if you use 32-bit
> access?

It's most likely a case that just wasn't cleaned up.  If you want to
correct things by dropping CONFIG_SYS_NS16550_MEM32 and checking
CONFIG_SYS_NS16550_REG_SIZE instead, that's fine.

> Would you object hugely to me just implicitly setting
> CONFIG_SYS_NS16550_MEM32 in ns16550.h for the c6x uart variant? I
> think it makes a lot of sense to do so, since in that case the driver
> itself explicitly depends on being able to write a value that doesn't
> fit in a byte.

Well, it's just SOC_DA8XX that isn't doing it today, and we need to
migrate that CONFIG symbol or do away with it.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180119/b0fa3e1a/attachment.sig>

[U-Boot] [U-Boot, 1/4] serial: ns16550: da8xx (freon/primus) is not omap-like

[Adam Ford]

On Fri, Jan 19, 2018 at 2:22 PM, Tom Rini <trini@konsulko.com> wrote:
> On Fri, Jan 19, 2018 at 05:39:32PM +0100, Matthijs van Duin wrote:
>> On 19 January 2018 at 16:41, Tom Rini <trini@konsulko.com> wrote:
>> > OK.  For v2, if you can convert CONFIG_SYS_NS16550_MEM32 to Kconfig as
>> > well I'd appreciate it.
>>
>> I'm not hugely comfortable doing that, since that would affect even
>> more targets. To be honest, I don't understand why it even exists as a
>> separate var instead of just testing whether
>> CONFIG_SYS_NS16550_REG_SIZE is 4 or -4. Are there actually some crazy
>> targets that allocate 4 bytes per register yet fail if you use 32-bit
>> access?
>
> It's most likely a case that just wasn't cleaned up.  If you want to
> correct things by dropping CONFIG_SYS_NS16550_MEM32 and checking
> CONFIG_SYS_NS16550_REG_SIZE instead, that's fine.
>
>> Would you object hugely to me just implicitly setting
>> CONFIG_SYS_NS16550_MEM32 in ns16550.h for the c6x uart variant? I
>> think it makes a lot of sense to do so, since in that case the driver
>> itself explicitly depends on being able to write a value that doesn't
>> fit in a byte.
>
> Well, it's just SOC_DA8XX that isn't doing it today, and we need to
> migrate that CONFIG symbol or do away with it.

If you want to CC me on the next round of the patch, I can test it on
a DA850-EVM.

adam
>
> --
> Tom
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>