[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Lei Wen]

Some hardware would dysfunctional if only access the register by
byte. This patch is tend to recover original access the responding
register according to CONFIG_SYS_NS16550_REG_SIZE.

Signed-off-by: Lei Wen <leiwen@marvell.com>
---
 README                   |    5 ++++
 drivers/serial/ns16550.c |    7 ------
 include/ns16550.h        |   51 +++++++++++++++++++++++++++++++++++++--------
 3 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/README b/README
index 21cd71b..45bc7dd 100644
--- a/README
+++ b/README
@@ -2660,6 +2660,11 @@ use the "saveenv" command to store a valid environment.
 		space for already greatly restricted images, including but not
 		limited to NAND_SPL configurations.
 
+- CONFIG_SYS_NS16550_MAX_REG_SIZE:
+		Define the ns16550 max register size,
+		if the CONFIG_SYS_NS16550_REG_SIZE is smaller than this value,
+		use padding to fill those gap.
+
 Low Level (hardware related) configuration options:
 ---------------------------------------------------
 
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 8eeb48f..4956c7f 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -16,13 +16,6 @@
 #define UART_FCRVAL (UART_FCR_FIFO_EN |	\
 		     UART_FCR_RXSR |	\
 		     UART_FCR_TXSR)		/* Clear & enable FIFOs */
-#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
-#define serial_out(x,y)	outb(x,(ulong)y)
-#define serial_in(y)	inb((ulong)y)
-#else
-#define serial_out(x,y) writeb(x,y)
-#define serial_in(y) 	readb(y)
-#endif
 
 #ifndef CONFIG_SYS_NS16550_IER
 #define CONFIG_SYS_NS16550_IER  0x00
diff --git a/include/ns16550.h b/include/ns16550.h
index 9ea81e9..a51b6e6 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -20,17 +20,50 @@
  * Note that the following macro magic uses the fact that the compiler
  * will not allocate storage for arrays of size 0
  */
-
-#if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
+#if (CONFIG_SYS_NS16550_REG_SIZE == 4) || (CONFIG_SYS_NS16550_REG_SIZE == -4)
+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
+#define serial_out(x, y)	outl(x, y)
+#define serial_in(y)		inl(y)
+#else
+#define serial_out(x, y)	writel(x, y)
+#define serial_in(y)		readl(y)
+#endif
+#define UART_REG_TYPE		unsigned int
+#elif (CONFIG_SYS_NS16550_REG_SIZE == 2) || (CONFIG_SYS_NS16550_REG_SIZE == -2)
+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
+#define serial_out(x, y)	outw(x, y)
+#define serial_in(y)		inw(y)
+#else
+#define serial_out(x, y)	writew(x, y)
+#define serial_in(y)		readw(y)
+#endif
+#define UART_REG_TYPE		unsigned short
+#elif (CONFIG_SYS_NS16550_REG_SIZE == 1) || (CONFIG_SYS_NS16550_REG_SIZE == -1)
+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
+#define serial_out(x, y)	outb(x, y)
+#define serial_in(y)		inb(y)
+#else
+#define serial_out(x, y)	writeb(x, y)
+#define serial_in(y)		readb(y)
+#endif
+#define UART_REG_TYPE		unsigned char
+#else
 #error "Please define NS16550 registers size."
-#elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
-#define UART_REG(x)						   \
-	unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1]; \
-	unsigned char x;
-#elif (CONFIG_SYS_NS16550_REG_SIZE < 0)
+#endif
+
+#ifndef CONFIG_SYS_NS16550_MAX_REG_SIZE
+#define CONFIG_SYS_NS16550_MAX_REG_SIZE 4
+#endif
+#if (CONFIG_SYS_NS16550_REG_SIZE > 0)
+#define UART_REG(x)							\
+	unsigned char prepad_##x[CONFIG_SYS_NS16550_MAX_REG_SIZE	\
+				- CONFIG_SYS_NS16550_REG_SIZE];		\
+	UART_REG_TYPE x;
+#else
 #define UART_REG(x)							\
-	unsigned char x;						\
-	unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
+	UART_REG_TYPE x;						\
+	unsigned char prepad_##x[CONFIG_SYS_NS16550_MAX_REG_SIZE	\
+				+ CONFIG_SYS_NS16550_REG_SIZE];
 #endif
 
 struct NS16550 {
-- 
1.7.0.4

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Wolfgang Denk]

Dear Lei Wen,

In message <1301586774-25447-1-git-send-email-leiwen@marvell.com> you wrote:
> Some hardware would dysfunctional if only access the register by
> byte. This patch is tend to recover original access the responding
> register according to CONFIG_SYS_NS16550_REG_SIZE.

Can you please explain on what board, and with which tool chain, you
see any problems?

> +- CONFIG_SYS_NS16550_MAX_REG_SIZE:
> +		Define the ns16550 max register size,
> +		if the CONFIG_SYS_NS16550_REG_SIZE is smaller than this value,
> +		use padding to fill those gap.

This makes no sense to me.  A register is always one specific size,
so the term "MAX_REG_SIZE" is bogus.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Landru! Guide us!
	-- A Beta 3-oid, "The Return of the Archons", stardate 3157.4

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Lei Wen]

Hi Wolfgang,

On Thu, Mar 31, 2011 at 11:58 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <1301586774-25447-1-git-send-email-leiwen@marvell.com> you wrote:
>> Some hardware would dysfunctional if only access the register by
>> byte. This patch is tend to recover original access the responding
>> register according to CONFIG_SYS_NS16550_REG_SIZE.
>
> Can you please explain on what board, and with which tool chain, you
> see any problems?

I test on Marvell pxa955 (MG1) board, with android 4.4.0 toolchain.
The pxa955's ns16550 register's IER has nine bits. The 8th bit is HSE, which
means the high speed mode. It seems something wrong there, if access the ier
by byte, the 8th bit would be 1 at the beginning, and would be cleared
by the following
set value in the ns16550 driver, which cause problem on that board,
for the baudrate
would be dysfunction.
If access the ier by int type, then the 8bit would be cleared, and
uart behavior normal.

>
>> +- CONFIG_SYS_NS16550_MAX_REG_SIZE:
>> + ? ? ? ? ? ? Define the ns16550 max register size,
>> + ? ? ? ? ? ? if the CONFIG_SYS_NS16550_REG_SIZE is smaller than this value,
>> + ? ? ? ? ? ? use padding to fill those gap.
>
> This makes no sense to me. ?A register is always one specific size,
> so the term "MAX_REG_SIZE" is bogus.

I introduce this CONFIG is for I found when
CONFIG_SYS_NS16550_REG_SIZE is 1 or -1,
there would be no prepad in the structure. And this means the register
size is char only?

Best regards,
Lei

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTi=BbS7wzcbsH26Hb0y_GVXzHtEQspoVZkKn1PMU@mail.gmail.com> you wrote:
> 
> > Can you please explain on what board, and with which tool chain, you
> > see any problems?
>
> I test on Marvell pxa955 (MG1) board, with android 4.4.0 toolchain.
> The pxa955's ns16550 register's IER has nine bits. The 8th bit is HSE, which
> means the high speed mode. It seems something wrong there, if access the ier
> by byte, the 8th bit would be 1 at the beginning, and would be cleared
> by the following
> set value in the ns16550 driver, which cause problem on that board,
> for the baudrate
> would be dysfunction.

This makes no sense to me. I have never seen any 9 bit registers in
any processor I ever encountered in real life.

Registers are typically 8, 16, 32 or 64 bit.

If your register holds 9 data bits, then it is most probably a 16 or
32 bit wide register.

Also, in this case the serial controller is probably not NS16550
compatible, because AFAICT the NS16550 uses only 8 bit wide
registers.


Further, it is not clear to me why there is a Mervell specific version
of the NS16550 driver (board/Marvell/common/ns16550.*).

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Time is an illusion perpetrated by the manufacturers of space.

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Lei Wen]

Hi Wolfgang,

On Fri, Apr 1, 2011 at 1:35 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTi=BbS7wzcbsH26Hb0y_GVXzHtEQspoVZkKn1PMU@mail.gmail.com> you wrote:
>>
>> > Can you please explain on what board, and with which tool chain, you
>> > see any problems?
>>
>> I test on Marvell pxa955 (MG1) board, with android 4.4.0 toolchain.
>> The pxa955's ns16550 register's IER has nine bits. The 8th bit is HSE, which
>> means the high speed mode. It seems something wrong there, if access the ier
>> by byte, the 8th bit would be 1 at the beginning, and would be cleared
>> by the following
>> set value in the ns16550 driver, which cause problem on that board,
>> for the baudrate
>> would be dysfunction.
>
> This makes no sense to me. I have never seen any 9 bit registers in
> any processor I ever encountered in real life.

I don't mean that register is 9bit...
I means that register, IER, is 32bit long, but 9-31th bit is reserved, and
0th to 8th bit is used... Maybe I don't say clearly...
So byte access would only cover 0-7th bit, while 8th bit is not covered.


> Also, in this case the serial controller is probably not NS16550
> compatible, because AFAICT the NS16550 uses only 8 bit wide
> registers.

This is may be additional feature added. For another part except this
one bit is all compatible with ns16550.

Best regards,
Lei

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTindN8kSqiq28H68Rs77Dc6Pf-4MSTibAq_2dRaV@mail.gmail.com> you wrote:
> 
> > This makes no sense to me. I have never seen any 9 bit registers in
> > any processor I ever encountered in real life.
> 
> I don't mean that register is 9bit...
> I means that register, IER, is 32bit long, but 9-31th bit is reserved, and
> 0th to 8th bit is used... Maybe I don't say clearly...
> So byte access would only cover 0-7th bit, while 8th bit is not covered.
> 
> > Also, in this case the serial controller is probably not NS16550
> > compatible, because AFAICT the NS16550 uses only 8 bit wide
> > registers.
> 
> This is may be additional feature added. For another part except this
> one bit is all compatible with ns16550.

OK, so let's summarize the facts we found so far:

1. Your hardware is NOT NS16550 compatible.  It does not have the
   typical 8 bit register interface, but provides additional bits that
   somehow control non-standard functionality.

2. You say there is one additional bit (bit 9, i. e. 0x00000100) which
   must remain set to 1 which appears to be the default setting after
   power-on reset.

3. You say that the current implementation, which uses a writeb() call
   (i. e. a byte write operation) to this register would not only
   affect bits 0...7, as expected, but also clear bit 9.

   This seems somewhat unlikely to me, so please confirm that I
   understood correctly.

4. You say that writing a 32 bit value instead (i. e. using writel())
   would work for you.

   This seems also unlikely to me, as the driver just operates on 8
   bit data, and will insert only 8 bit data into the word written, so
   you will write some data word like 0x000000XX - and in this case
   (and only in this case) the 9th bit would explicitly be set to 0.

So from my understanding of the code the original version is supposed
to work on your hardware, while your patched version should definitely
NOT work.

Yet you state it's exactly vice versa.

Either I'm missing something, or you fail to provide complete
information.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Space is big. You just won't believe how vastly, hugely, mind-
bogglingly big it is. I mean, you may think it's a long way down the
road to the drug store, but that's just peanuts to space.
                              -- The Hitchhiker's Guide to the Galaxy

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Lei Wen]

Hi Wolfgang,

On Fri, Apr 1, 2011 at 3:39 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTindN8kSqiq28H68Rs77Dc6Pf-4MSTibAq_2dRaV@mail.gmail.com> you wrote:
>>
>> > This makes no sense to me. I have never seen any 9 bit registers in
>> > any processor I ever encountered in real life.
>>
>> I don't mean that register is 9bit...
>> I means that register, IER, is 32bit long, but 9-31th bit is reserved, and
>> 0th to 8th bit is used... Maybe I don't say clearly...
>> So byte access would only cover 0-7th bit, while 8th bit is not covered.
>>
>> > Also, in this case the serial controller is probably not NS16550
>> > compatible, because AFAICT the NS16550 uses only 8 bit wide
>> > registers.
>>
>> This is may be additional feature added. For another part except this
>> one bit is all compatible with ns16550.
>
> OK, so let's summarize the facts we found so far:
>
> 1. Your hardware is NOT NS16550 compatible. ?It does not have the
> ? typical 8 bit register interface, but provides additional bits that
> ? somehow control non-standard functionality.
>
> 2. You say there is one additional bit (bit 9, i. e. 0x00000100) which
> ? must remain set to 1 which appears to be the default setting after
> ? power-on reset.
>
> 3. You say that the current implementation, which uses a writeb() call
> ? (i. e. a byte write operation) to this register would not only
> ? affect bits 0...7, as expected, but also clear bit 9.

That is not my case. In my case, for writeb, it would affect only
bits0-7, but leave
bit 8 untouched. However, I need the bit 8 to be set to be 0, which is
1 at the power
on.

>
> ? This seems somewhat unlikely to me, so please confirm that I
> ? understood correctly.
>
> 4. You say that writing a 32 bit value instead (i. e. using writel())
> ? would work for you.
>
> ? This seems also unlikely to me, as the driver just operates on 8
> ? bit data, and will insert only 8 bit data into the word written, so
> ? you will write some data word like 0x000000XX - and in this case
> ? (and only in this case) the 9th bit would explicitly be set to 0.

Yes, that is what I want. The bit8 set to 0.


Best regards,
Lei

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTingSAd=sQH=QbmJjvOFjVbjsCbWH0SFFyKzVrs4@mail.gmail.com> you wrote:
> 
> > 3. You say that the current implementation, which uses a writeb() call
> >   (i. e. a byte write operation) to this register would not only
> >   affect bits 0...7, as expected, but also clear bit 9.
> 
> That is not my case. In my case, for writeb, it would affect only
> bits0-7, but leave
> bit 8 untouched. However, I need the bit 8 to be set to be 0, which is
> 1 at the power
> on.
...
> Yes, that is what I want. The bit8 set to 0.

Ah.  But this is completely different thing, then.

Your code would only perform this operation by accident, and this is
definitely wrong.   Assume some other system where this bit needs to
be set to 1 - then your code would corrupt the setting.

So I think:

1) The current NS16550 driver behaves correctly.  It sets up the
   NS16550 compatible parts of your chip in the correct way, without
   messing with any non-standard bits.

2) If you have additional, non-standard bits in your device, you must
   initialize these separately.  Eventually you provide a custom
   driver which just calls the standard NS16550 driver functions,
   except where you have additional need to manipulate the extra,
   non-standard bits of your device.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
For every complex problem, there is a solution that is simple,  neat,
and wrong.                                           -- H. L. Mencken

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Lei Wen]

Hi Wolfgang,

On Fri, Apr 1, 2011 at 6:25 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTingSAd=sQH=QbmJjvOFjVbjsCbWH0SFFyKzVrs4@mail.gmail.com> you wrote:
>>
>> > 3. You say that the current implementation, which uses a writeb() call
>> > ? (i. e. a byte write operation) to this register would not only
>> > ? affect bits 0...7, as expected, but also clear bit 9.
>>
>> That is not my case. In my case, for writeb, it would affect only
>> bits0-7, but leave
>> bit 8 untouched. However, I need the bit 8 to be set to be 0, which is
>> 1 at the power
>> on.
> ...
>> Yes, that is what I want. The bit8 set to 0.
>
> Ah. ?But this is completely different thing, then.
>
> Your code would only perform this operation by accident, and this is
> definitely wrong. ? Assume some other system where this bit needs to
> be set to 1 - then your code would corrupt the setting.

I think my code also could handle this. They only could set the
CONFIG_SYS_NS16550_REG_SIZE  to be 1
and CONFIG_SYS_NS16550_MAX_REG_SIZE to be 4. Then
the other bits is untouched by this driver.
If CONFIG_SYS_NS16550_REG_SIZE is 4, then it would fulfill
my need to clear the all other bits.

>
> So I think:
>
> 1) The current NS16550 driver behaves correctly. ?It sets up the
> ? NS16550 compatible parts of your chip in the correct way, without
> ? messing with any non-standard bits.
>
> 2) If you have additional, non-standard bits in your device, you must
> ? initialize these separately. ?Eventually you provide a custom
> ? driver which just calls the standard NS16550 driver functions,
> ? except where you have additional need to manipulate the extra,
> ? non-standard bits of your device.
>
The previous version of ns16550 has the ability of control the access
width according to the CONFIG_SYS_NS16550_REG_SIZE. I just don't
understand why my patch is rejected for I give this back?

Best regards,
Lei

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTikiH8FUKprdHOuWNe8su0fTN_HcawsZB9xehMaT@mail.gmail.com> you wrote:
> 
> I think my code also could handle this. They only could set the
> CONFIG_SYS_NS16550_REG_SIZE  to be 1
> and CONFIG_SYS_NS16550_MAX_REG_SIZE to be 4. Then
> the other bits is untouched by this driver.

I don't think so. You still use just a single writel() call then.  To
leave the other bits untouched, you would have to perform a readl()
first, then insert one data byte, and then write it back.  Your patch
does not do that.

> The previous version of ns16550 has the ability of control the access
> width according to the CONFIG_SYS_NS16550_REG_SIZE. I just don't
> understand why my patch is rejected for I give this back?

There are two reasons:

First, your code is not correct (see above), it works just for your
setup and by pure chance.

Second, this is a NS16550 driver, that can be used with all 16550
compatible devices.  Handling incompatible UART chips, or UART chips
with additional fatures, is out of scope of this driver.

If you need to handle additional register settings, use a wrapper
driver as I suggested before.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Little known fact about Middle Earth:   The Hobbits had a very sophi-
sticated computer network!   It was a Tolkien Ring...

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Lei Wen]

On Fri, Apr 1, 2011 at 8:41 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTikiH8FUKprdHOuWNe8su0fTN_HcawsZB9xehMaT@mail.gmail.com> you wrote:
>>
>> I think my code also could handle this. They only could set the
>> CONFIG_SYS_NS16550_REG_SIZE ?to be 1
>> and CONFIG_SYS_NS16550_MAX_REG_SIZE to be 4. Then
>> the other bits is untouched by this driver.
>
> I don't think so. You still use just a single writel() call then. ?To
> leave the other bits untouched, you would have to perform a readl()
> first, then insert one data byte, and then write it back. ?Your patch
> does not do that.

My original patch is like below, so where it call writel?...
+#elif (CONFIG_SYS_NS16550_REG_SIZE == 1) || (CONFIG_SYS_NS16550_REG_SIZE == -1)
+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
+#define serial_out(x, y)       outb(x, y)
+#define serial_in(y)           inb(y)
+#else
+#define serial_out(x, y)       writeb(x, y)
+#define serial_in(y)           readb(y)

Best regards,
Lei

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTinqAyDx7=61FMgPdCg3ULt9nk93jF-y0Sdi=5uX@mail.gmail.com> you wrote:
>
> >> I think my code also could handle this. They only could set the
> >> CONFIG_SYS_NS16550_REG_SIZE =A0to be 1
> >> and CONFIG_SYS_NS16550_MAX_REG_SIZE to be 4. Then
> >> the other bits is untouched by this driver.
> >
> > I don't think so. You still use just a single writel() call then. =A0To
> > leave the other bits untouched, you would have to perform a readl()
> > first, then insert one data byte, and then write it back. =A0Your patch
> > does not do that.
> 
> My original patch is like below, so where it call writel?...
> +#elif (CONFIG_SYS_NS16550_REG_SIZE == 1) || (CONFIG_SYS_NS16550_REG_SIZE == -1)
> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> +#define serial_out(x, y)       outb(x, y)
> +#define serial_in(y)           inb(y)
> +#else
> +#define serial_out(x, y)       writeb(x, y)
> +#define serial_in(y)           readb(y)

If you use writeb() [as the current driver would do as well}, then how
do you expect to set this bit 8 (which is in the next byte) to 0 as
you claim you have to?

Either I don't understand what you want, or you don't understand how
the ns16550 driver works.

I can see no shortcomings in the existent driver, thus no need to
make it more complicated than it already is.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Time is a drug. Too much of it kills you.
                                      - Terry Pratchett, _Small Gods_

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Lei Wen]

Hi Wolfgang,

On Fri, Apr 1, 2011 at 9:55 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTinqAyDx7=61FMgPdCg3ULt9nk93jF-y0Sdi=5uX@mail.gmail.com> you wrote:
>>
>> >> I think my code also could handle this. They only could set the
>> >> CONFIG_SYS_NS16550_REG_SIZE =A0to be 1
>> >> and CONFIG_SYS_NS16550_MAX_REG_SIZE to be 4. Then
>> >> the other bits is untouched by this driver.
>> >
>> > I don't think so. You still use just a single writel() call then. =A0To
>> > leave the other bits untouched, you would have to perform a readl()
>> > first, then insert one data byte, and then write it back. =A0Your patch
>> > does not do that.
>>
>> My original patch is like below, so where it call writel?...
>> +#elif (CONFIG_SYS_NS16550_REG_SIZE == 1) || (CONFIG_SYS_NS16550_REG_SIZE == -1)
>> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>> +#define serial_out(x, y) ? ? ? outb(x, y)
>> +#define serial_in(y) ? ? ? ? ? inb(y)
>> +#else
>> +#define serial_out(x, y) ? ? ? writeb(x, y)
>> +#define serial_in(y) ? ? ? ? ? readb(y)
>
> If you use writeb() [as the current driver would do as well}, then how
> do you expect to set this bit 8 (which is in the next byte) to 0 as
> you claim you have to?

As I explain, if set CONFIG_SYS_NS16550_REG_SIZE to 4, and
set CONFIG_SYS_NS16550_MAX_REG_SIZE also to 4, then the serial_out
becomes writel. :)
+#if (CONFIG_SYS_NS16550_REG_SIZE == 4) || (CONFIG_SYS_NS16550_REG_SIZE == -4)
+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
+#define serial_out(x, y)       outl(x, y)
+#define serial_in(y)           inl(y)
+#else
+#define serial_out(x, y)       writel(x, y)
+#define serial_in(y)           readl(y)
+#endif

Best regards,
Lei

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTi=q-cj-Q5iNqiQpEddkH1DxUAR1H_5wenaES7bE@mail.gmail.com> you wrote:
> 
> >> > I don't think so. You still use just a single writel() call then.  To
> >> > leave the other bits untouched, you would have to perform a readl()
> >> > first, then insert one data byte, and then write it back.  Your patch
> >> > does not do that.
> >>
> >> My original patch is like below, so where it call writel?...
> >> +#elif (CONFIG_SYS_NS16550_REG_SIZE == 1) || (CONFIG_SYS_NS16550_REG_SIZE == -1)
> >> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> >> +#define serial_out(x, y)       outb(x, y)
> >> +#define serial_in(y)           inb(y)
> >> +#else
> >> +#define serial_out(x, y)       writeb(x, y)
> >> +#define serial_in(y)           readb(y)
> >
> > If you use writeb() [as the current driver would do as well}, then how
> > do you expect to set this bit 8 (which is in the next byte) to 0 as
> > you claim you have to?
>
> As I explain, if set CONFIG_SYS_NS16550_REG_SIZE to 4, and
> set CONFIG_SYS_NS16550_MAX_REG_SIZE also to 4, then the serial_out
> becomes writel. :)

Right - which is exactly what I said, and which you denied.

I give up, I have other things to do as well.

You know my proposal how to implement the driver for your non-standard
chip.

Your patch is rejected.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The universe contains any amount of horrible ways  to  be  woken  up,
such as the noise of the mob breaking down the front door, the scream
of fire engines, or the realization that today is the Monday which on
Friday night was a comfortably long way off.
                                 - Terry Pratchett, _Moving Pictures_

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Lei Wen]

Hi Wolfgang,

On Fri, Apr 1, 2011 at 10:25 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTi=q-cj-Q5iNqiQpEddkH1DxUAR1H_5wenaES7bE@mail.gmail.com> you wrote:
>>
>> >> > I don't think so. You still use just a single writel() call then. ?To
>> >> > leave the other bits untouched, you would have to perform a readl()
>> >> > first, then insert one data byte, and then write it back. ?Your patch
>> >> > does not do that.
>> >>
>> >> My original patch is like below, so where it call writel?...
>> >> +#elif (CONFIG_SYS_NS16550_REG_SIZE == 1) || (CONFIG_SYS_NS16550_REG_SIZE == -1)
>> >> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>> >> +#define serial_out(x, y) ? ? ? outb(x, y)
>> >> +#define serial_in(y) ? ? ? ? ? inb(y)
>> >> +#else
>> >> +#define serial_out(x, y) ? ? ? writeb(x, y)
>> >> +#define serial_in(y) ? ? ? ? ? readb(y)
>> >
>> > If you use writeb() [as the current driver would do as well}, then how
>> > do you expect to set this bit 8 (which is in the next byte) to 0 as
>> > you claim you have to?
>>
>> As I explain, if set CONFIG_SYS_NS16550_REG_SIZE to 4, and
>> set CONFIG_SYS_NS16550_MAX_REG_SIZE also to 4, then the serial_out
>> becomes writel. :)
>
> Right - which is exactly what I said, and which you denied.
>
> I give up, I have other things to do as well.

Just a question to clarify... What your point I denied, that is really
confused me...
I think in this thread, I explain to you, my patch could recover what original
CONFIG_SYS_NS16550_REG_SIZE  means...

Since you reject, I have nothing else to say...

Best regards,
Lei

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTi=7F-hja5Avb7u0gpv-RnYJ5s73Gzb7RkHu2+EV@mail.gmail.com> you wrote:
> 
> Just a question to clarify... What your point I denied, that is really
> confused me...
> I think in this thread, I explain to you, my patch could recover what original
> CONFIG_SYS_NS16550_REG_SIZE  means...

But there is nothing to "recover" in that driver.  It just works fine.

You attempt to introduce an unnecessary complexity, which is not even
generally correct for your own, non-standard controller.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
How does a project get to be a year late?      ... One day at a time.

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Prafulla Wadaskar]

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
> On Behalf Of Lei Wen
> Sent: Friday, April 01, 2011 8:04 PM
> To: Wolfgang Denk
> Cc: Lei Wen; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] serial: ns16550: fix different reg size
> access
> 
> Hi Wolfgang,
> 
> On Fri, Apr 1, 2011 at 10:25 PM, Wolfgang Denk <wd@denx.de> wrote:
> > Dear Lei Wen,
> >
> > In message <AANLkTi=q-cj-
> Q5iNqiQpEddkH1DxUAR1H_5wenaES7bE at mail.gmail.com> you wrote:
> >>
> >> >> > I don't think so. You still use just a single writel() call
> then. ?To
> >> >> > leave the other bits untouched, you would have to perform a
> readl()
> >> >> > first, then insert one data byte, and then write it back. ?Your
> patch
> >> >> > does not do that.
> >> >>
> >> >> My original patch is like below, so where it call writel?...
> >> >> +#elif (CONFIG_SYS_NS16550_REG_SIZE == 1) ||
> (CONFIG_SYS_NS16550_REG_SIZE == -1)
> >> >> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> >> >> +#define serial_out(x, y) ? ? ? outb(x, y)
> >> >> +#define serial_in(y) ? ? ? ? ? inb(y)
> >> >> +#else
> >> >> +#define serial_out(x, y) ? ? ? writeb(x, y)
> >> >> +#define serial_in(y) ? ? ? ? ? readb(y)
> >> >
> >> > If you use writeb() [as the current driver would do as well}, then
> how
> >> > do you expect to set this bit 8 (which is in the next byte) to 0 as
> >> > you claim you have to?
> >>
> >> As I explain, if set CONFIG_SYS_NS16550_REG_SIZE to 4, and
> >> set CONFIG_SYS_NS16550_MAX_REG_SIZE also to 4, then the serial_out
> >> becomes writel. :)
> >
> > Right - which is exactly what I said, and which you denied.
> >
> > I give up, I have other things to do as well.
> 
> Just a question to clarify... What your point I denied, that is really
> confused me...
> I think in this thread, I explain to you, my patch could recover what
> original
> CONFIG_SYS_NS16550_REG_SIZE  means...
> 
> Since you reject, I have nothing else to say...

Hi Lei
I understood this thread correctly as-
1. ns16550 is standard IP used across several SoC and has driver in place.
2. Your specific implementation of the same IP on your specific SoC need bit9 or x register to be set to 0 to work this IP correctly on your h/w.
3. but doing in ns16550 driver may brake other implementations.

(correct me if I am wrong)

So what I suggest here-
1. do not disturb/touch nx16550 driver at all.
2. write a small init code in cpu.c (specific to this SoC/arch) to reset this bit only.

This would be straight forward and will satisfy your need and acceptance too.

Regards..
Prafulla . .

[U-Boot] [PATCH] serial: ns16550: fix different reg size access

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <F766E4F80769BD478052FB6533FA745D19F9A29D36@SC-VEXCH4.marvell.com> you wrote:
> 
> 1. ns16550 is standard IP used across several SoC and has driver in place.
> 2. Your specific implementation of the same IP on your specific SoC need
> bit9 or x register to be set to 0 to work this IP correctly on your h/w.
> 3. but doing in ns16550 driver may brake other implementations.

Right.

> (correct me if I am wrong)
>
> So what I suggest here-
> 1. do not disturb/touch nx16550 driver at all.
> 2. write a small init code in cpu.c (specific to this SoC/arch) to reset this bit only.

Sorry, but I dislike this approach.  I think it would be better to
provide a proper driver infrastructure for this type of UART, as it
might be used in other appliances, and it is a good idea to keep all
related code parts together in one driver file.

Here is what I suggested (I think I repeat it the 3rd time now):

| 2) If you have additional, non-standard bits in your device, you must
|    initialize these separately.  Eventually you provide a custom
|    driver which just calls the standard NS16550 driver functions,
|    except where you have additional need to manipulate the extra,
|    non-standard bits of your device.

Thanks.

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
About the use of language: it is impossible to sharpen a pencil  with
a  blunt  ax.  It is equally vain to try to do it with ten blunt axes
instead.                                           -- Edsger Dijkstra