[U-Boot] [PATCH] serial: ns16550: fix different reg size access
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 {
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
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
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
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
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
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:
- 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.
- 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.
- 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.
- 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
Dear Lei Wen,
In message AANLkTingSAd=sQH=QbmJjvOFjVbjsCbWH0SFFyKzVrs4@mail.gmail.com you wrote:
- 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
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:
- 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:
- 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.
- 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
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
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
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
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
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
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
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
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Lei Wen Sent: Friday, April 01, 2011 8:04 PM To: Wolfgang Denk Cc: Lei Wen; u-boot@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@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 . .
Dear Prafulla Wadaskar,
In message F766E4F80769BD478052FB6533FA745D19F9A29D36@SC-VEXCH4.marvell.com you wrote:
- ns16550 is standard IP used across several SoC and has driver in place.
- 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-
- do not disturb/touch nx16550 driver at all.
- 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
participants (4)
-
Lei Wen -
Lei Wen -
Prafulla Wadaskar -
Wolfgang Denk