Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
Dear Vadim Bendebury, In message you wrote: > > > Make it an inline function then, this will do the typechecking for you. > > I am not sure what is wrong with a short macro in this case - is this > against the coding style? "Documentation/CodingStyle": Generally, inline functions are preferable to macros resembling functions. Marek listed one (important) reason why this is the case. So please change this. 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: w...@denx.de "'Tis true, 'tis pity, and pity 'tis 'tis true." - Poloniouius, in Willie the Shake's _Hamlet, Prince of Darkness_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v4 2/2] NS16550: buffer reads
From: Scott Wood
From: Scott Wood
This improves the performance of U-Boot when accepting rapid input,
such as pasting a sequence of commands.
Without this patch, on P4080DS I see a maximum of around 5 lines can
be pasted. With this patch, it handles around 70 lines before lossage,
long enough for most things you'd paste.
With it enabled, on ppc it's an extra 396 bytes of image size, and 1056
bytes of BSS.
ARM note from Simon Glass - ARM code size goes from
212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
Signed-off-by: Scott Wood
---
Changes in v2:
- Fix checkpatch warnings, the other one was already there
Changes in v3:
- Use CONFIG_NS16550_BUFFER_READS to set the buffer size also
Changes in v4:
- Change config option to CONFIG_NS16550_RBUF_SIZE
- Add additional checkpatch-cleanup patch
README | 12 ++
drivers/serial/ns16550.c | 97 +++--
drivers/serial/serial.c |5 +-
include/ns16550.h|4 +-
4 files changed, 109 insertions(+), 9 deletions(-)
diff --git a/README b/README
index 7e032a9..d8b4c4d 100644
--- a/README
+++ b/README
@@ -2862,6 +2862,18 @@ use the "saveenv" command to store a valid environment.
space for already greatly restricted images, including but not
limited to NAND_SPL configurations.
+- CONFIG_NS16550_RBUF_SIZE:
+ Instead of reading directly from the receive register
+ every time U-Boot is ready for another byte, keep a
+ buffer and fill it from the hardware fifo every time
+ U-Boot reads a character. This helps U-Boot keep up with
+ a larger amount of rapid input, such as happens when
+ pasting text into the terminal.
+
+ To use this option, define CONFIG_NS16550_RBUF_SIZE to
+ the size of the buffer you want (256 is a reasonable value).
+
+
Low Level (hardware related) configuration options:
---
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 056c25d..12de014 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -4,12 +4,15 @@
* modified to use CONFIG_SYS_ISA_MEM and new defines
*/
+#include
#include
#include
#include
#include
#include
+DECLARE_GLOBAL_DATA_PTR;
+
#define UART_LCRVAL UART_LCR_8N1 /* 8 data, 1 stop, no parity */
#define UART_MCRVAL (UART_MCR_DTR | \
UART_MCR_RTS) /* RTS/DTR */
@@ -95,21 +98,105 @@ void NS16550_putc(NS16550_t com_port, char c)
}
#ifndef CONFIG_NS16550_MIN_FUNCTIONS
-char NS16550_getc(NS16550_t com_port)
+static char NS16550_raw_getc(NS16550_t regs)
{
- while ((serial_in(&com_port->lsr) & UART_LSR_DR) == 0) {
+ while ((serial_in(®s->lsr) & UART_LSR_DR) == 0) {
#ifdef CONFIG_USB_TTY
extern void usbtty_poll(void);
usbtty_poll();
#endif
WATCHDOG_RESET();
}
- return serial_in(&com_port->rbr);
+ return serial_in(®s->rbr);
+}
+
+static int NS16550_raw_tstc(NS16550_t regs)
+{
+ return ((serial_in(®s->lsr) & UART_LSR_DR) != 0);
+}
+
+
+#ifdef CONFIG_NS16550_RBUF_SIZE
+
+#define NUM_PORTS 4
+
+struct ns16550_priv {
+ char buf[CONFIG_NS16550_RBUF_SIZE];
+ unsigned int head, tail;
+};
+
+static struct ns16550_priv rxstate[NUM_PORTS];
+
+static void enqueue(unsigned int port, char ch)
+{
+ /* If queue is full, drop the character. */
+ if ((rxstate[port].head - rxstate[port].tail - 1) %
+ CONFIG_NS16550_RBUF_SIZE == 0)
+ return;
+
+ rxstate[port].buf[rxstate[port].tail] = ch;
+ rxstate[port].tail = (rxstate[port].tail + 1) %
+ CONFIG_NS16550_RBUF_SIZE;
+}
+
+static int dequeue(unsigned int port, char *ch)
+{
+ /* Empty queue? */
+ if (rxstate[port].head == rxstate[port].tail)
+ return 0;
+
+ *ch = rxstate[port].buf[rxstate[port].head];
+ rxstate[port].head = (rxstate[port].head + 1) %
+ CONFIG_NS16550_RBUF_SIZE;
+ return 1;
+}
+
+static void fill_rx_buf(NS16550_t regs, unsigned int port)
+{
+ while ((serial_in(®s->lsr) & UART_LSR_DR) != 0)
+ enqueue(port, serial_in(®s->rbr));
+}
+
+char NS16550_getc(NS16550_t regs, unsigned int port)
+{
+ char ch;
+
+ if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC))
+ return NS16550_raw_getc(regs);
+
+ do {
+#ifdef CONFIG_USB_TTY
+ extern void usbtty_poll(void);
+ usbtty_poll();
+#endif
+ fill_rx_buf(regs, port);
+ WATCHDOG_RESET();
+ } while (!dequeue(port, &ch));
+
+ return ch;
+}
+
+int NS16550_tstc(NS16550_t regs, unsigned int port)
+{
+ if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC))
+ return NS16550_raw_tstc(regs);
+
+ fill_rx_buf(regs,
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
On Saturday 15 October 2011 16:27:02 Vadim Bendebury wrote: > On Sat, Oct 15, 2011 at 1:01 PM, Mike Frysinger wrote: > > On Saturday 15 October 2011 15:44:04 Wolfgang Denk wrote: > >> Vadim Bendebury wrote: > >> > > Two underscores aren't a good practice. > >> > > >> > I did this as a result of a previous review. Do you have a suggestion > >> > how this should be done instead? > >> > >> First, and most important, __u_boot_cmd_tpm appears to be undefined in > >> your two patches, so it looks to be a real bug. > >> > >> Second, please read the C standard's section about reserved > >> identifiers. > > > > no, this is coming from common u-boot code. look at > > include/command.h:U_BOOT_CMD defines. > > or, more importantly: the question is what is the right/suggested way > to print out the help message associated with a U_BOOT_CMD > declaration? your command is given a cmd_tbl_t *cmdtp pointer to pass to cmd_usage -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v4 1/2] NS16550: trivial code clean for checkpatch
This removes most checkpatch warnings from the ns16550 driver and its
header.
Signed-off-by: Simon Glass
---
drivers/serial/ns16550.c | 37 -
include/ns16550.h| 16
2 files changed, 28 insertions(+), 25 deletions(-)
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 0174744..056c25d 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -17,24 +17,24 @@
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)
+#define serial_out(x, y) outb(x, (ulong)y)
+#define serial_in(y) inb((ulong)y)
#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE > 0)
-#define serial_out(x,y) out_be32(y,x)
-#define serial_in(y) in_be32(y)
+#define serial_out(x, y) out_be32(y, x)
+#define serial_in(y) in_be32(y)
#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE < 0)
-#define serial_out(x,y) out_le32(y,x)
-#define serial_in(y) in_le32(y)
+#define serial_out(x, y) out_le32(y, x)
+#define serial_in(y) in_le32(y)
#else
-#define serial_out(x,y) writeb(x,y)
-#define serial_in(y) readb(y)
+#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
#endif /* CONFIG_SYS_NS16550_IER */
-void NS16550_init (NS16550_t com_port, int baud_divisor)
+void NS16550_init(NS16550_t com_port, int baud_divisor)
{
serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
#if defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)
@@ -52,15 +52,17 @@ void NS16550_init (NS16550_t com_port, int baud_divisor)
serial_out(UART_LCRVAL, &com_port->lcr);
#if defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)
#if defined(CONFIG_APTIX)
- serial_out(3, &com_port->mdr1); /* /13 mode so Aptix 6MHz can hit
115200 */
+ /* /13 mode so Aptix 6MHz can hit 115200 */
+ serial_out(3, &com_port->mdr1);
#else
- serial_out(0, &com_port->mdr1); /* /16 is proper to hit 115200 with
48MHz */
+ /* /16 is proper to hit 115200 with 48MHz */
+ serial_out(0, &com_port->mdr1);
#endif
#endif /* CONFIG_OMAP */
}
#ifndef CONFIG_NS16550_MIN_FUNCTIONS
-void NS16550_reinit (NS16550_t com_port, int baud_divisor)
+void NS16550_reinit(NS16550_t com_port, int baud_divisor)
{
serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
serial_out(UART_LCR_BKSE | UART_LCRVAL, &com_port->lcr);
@@ -76,9 +78,10 @@ void NS16550_reinit (NS16550_t com_port, int baud_divisor)
}
#endif /* CONFIG_NS16550_MIN_FUNCTIONS */
-void NS16550_putc (NS16550_t com_port, char c)
+void NS16550_putc(NS16550_t com_port, char c)
{
- while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0);
+ while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0)
+ ;
serial_out(c, &com_port->thr);
/*
@@ -92,7 +95,7 @@ void NS16550_putc (NS16550_t com_port, char c)
}
#ifndef CONFIG_NS16550_MIN_FUNCTIONS
-char NS16550_getc (NS16550_t com_port)
+char NS16550_getc(NS16550_t com_port)
{
while ((serial_in(&com_port->lsr) & UART_LSR_DR) == 0) {
#ifdef CONFIG_USB_TTY
@@ -104,9 +107,9 @@ char NS16550_getc (NS16550_t com_port)
return serial_in(&com_port->rbr);
}
-int NS16550_tstc (NS16550_t com_port)
+int NS16550_tstc(NS16550_t com_port)
{
- return ((serial_in(&com_port->lsr) & UART_LSR_DR) != 0);
+ return (serial_in(&com_port->lsr) & UART_LSR_DR) != 0;
}
#endif /* CONFIG_NS16550_MIN_FUNCTIONS */
diff --git a/include/ns16550.h b/include/ns16550.h
index 51f1c17..e9d2eda 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -65,12 +65,12 @@ struct NS16550 {
#define dll rbr
#define dlm ier
-typedef volatile struct NS16550 *NS16550_t;
+typedef struct NS16550 *NS16550_t;
/*
* These are the definitions for the FIFO Control Register
*/
-#define UART_FCR_FIFO_EN 0x01 /* Fifo enable */
+#define UART_FCR_FIFO_EN 0x01 /* Fifo enable */
#define UART_FCR_CLEAR_RCVR0x02 /* Clear the RCVR FIFO */
#define UART_FCR_CLEAR_XMIT0x04 /* Clear the XMIT FIFO */
#define UART_FCR_DMA_SELECT0x08 /* For DMA applications */
@@ -106,7 +106,7 @@ typedef volatile struct NS16550 *NS16550_t;
#define UART_LCR_WLS_6 0x01/* 6 bit character length */
#define UART_LCR_WLS_7 0x02/* 7 bit character length */
#define UART_LCR_WLS_8 0x03/* 8 bit character length */
-#define UART_LCR_STB 0x04/* Number of stop Bits, off = 1, on =
1.5 or 2) */
+#define UART_LCR_STB 0x04/* # stop Bits, off=1, on=1.5 or 2) */
#define UART_LCR_PEN 0x08/* Parity eneble */
#define UART_LCR_EPS 0x10/* Even Parity Select */
#define UAR
Re: [U-Boot] [PATCH v4 6/6] fdt: add decode helper library
On Saturday 15 October 2011 11:48:25 Simon Glass wrote:
> --- /dev/null
> +++ b/lib/fdtdec.c
>
> +static const char *compat_names[COMPAT_COUNT] = {
static const char * const compat_names[COMPAT_COUNT] = {
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] NS16550: buffer reads
Hi Marek, On Sat, Oct 15, 2011 at 9:18 PM, Marek Vasut wrote: > On Sunday, October 16, 2011 05:56:56 AM Simon Glass wrote: >> Hi Marek, >> >> On Sat, Oct 15, 2011 at 10:55 AM, Marek Vasut wrote: >> > On Saturday, October 15, 2011 06:03:52 PM Simon Glass wrote: >> >> From: Scott Wood >> >> >> >> From: Scott Wood >> >> >> >> This improves the performance of U-Boot when accepting rapid input, >> >> such as pasting a sequence of commands. >> > >> > Hi Simon, >> > >> > [...] >> > >> >> diff --git a/include/ns16550.h b/include/ns16550.h >> >> index 51f1c17..60b0abc 100644 >> >> --- a/include/ns16550.h >> >> +++ b/include/ns16550.h >> >> @@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t; >> >> >> >> void NS16550_init (NS16550_t com_port, int baud_divisor); >> >> void NS16550_putc (NS16550_t com_port, char c); >> >> -char NS16550_getc (NS16550_t com_port); >> >> -int NS16550_tstc (NS16550_t com_port); >> >> +char NS16550_getc(NS16550_t regs, unsigned int port); >> >> +int NS16550_tstc(NS16550_t regs, unsigned int port); >> >> void NS16550_reinit (NS16550_t com_port, int baud_divisor); >> > >> > Maybe you should fix the indent altogether ? >> >> I should probably do a separate patch for that. > > Maybe queue it before this one ? :) Thanks! OK I will add a new patch before the main one which cleans up the warnings in the file. Regards, Simon > >> >> Regards, >> Simon >> >> > Cheers > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] NS16550: buffer reads
Hi Wolfgang,
On Sat, Oct 15, 2011 at 12:08 PM, Wolfgang Denk wrote:
> Dear Simon Glass,
>
> In message <1318694632-21872-1-git-send-email-...@chromium.org> you wrote:
>> From: Scott Wood
>>
>> From: Scott Wood
>>
>> This improves the performance of U-Boot when accepting rapid input,
>> such as pasting a sequence of commands.
>>
>> Without this patch, on P4080DS I see a maximum of around 5 lines can
>> be pasted. With this patch, it handles around 70 lines before lossage,
>> long enough for most things you'd paste.
>>
>> With it enabled, on ppc it's an extra 396 bytes of image size, and 1056
>> bytes of BSS.
>>
>> ARM note from Simon Glass - ARM code size goes from
>> 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
>>
>> Signed-off-by: Scott Wood
>> ---
>> Changes in v2:
>> - Fix checkpatch warnings, the other one was already there
>>
>> Changes in v3:
>> - Use CONFIG_NS16550_BUFFER_READS to set the buffer size also
>>
>> README | 12 ++
>> drivers/serial/ns16550.c | 96
>> +++--
>> drivers/serial/serial.c | 5 +-
>> include/ns16550.h | 4 +-
>> 4 files changed, 108 insertions(+), 9 deletions(-)
>>
>> diff --git a/README b/README
>> index 7e032a9..1150d6c 100644
>> --- a/README
>> +++ b/README
>> @@ -2862,6 +2862,18 @@ use the "saveenv" command to store a valid
>> environment.
>> space for already greatly restricted images, including but not
>> limited to NAND_SPL configurations.
>>
>> +- CONFIG_NS16550_BUFFER_READS:
>> + Instead of reading directly from the receive register
>> + every time U-Boot is ready for another byte, keep a
>> + buffer and fill it from the hardware fifo every time
>> + U-Boot reads a character. This helps U-Boot keep up with
>> + a larger amount of rapid input, such as happens when
>> + pasting text into the terminal.
>> +
>> + To use this option, define CONFIG_NS16550_BUFFER_READS to
>> + the size of the buffer you want (256 is a reasonable value).
>
> If we should accept that patch, then please chose a better name here.
> For example, CONFIG_NS16550_RBUF_SIZE (or similar) seems to be a
> somewhat better choice.
Yes much better.
>
>> +#ifdef CONFIG_NS16550_BUFFER_READS
>> +
>> +#define BUF_SIZE CONFIG_NS16550_BUFFER_READS
>> +#define NUM_PORTS 4
>> +
>> +struct ns16550_priv {
>> + char buf[BUF_SIZE];
>> + unsigned int head, tail;
>> +};
>
> There is little sense in adding another variable here. Just write:
>
> char buf[CONFIG_NS16550_BUFFER_READS];
>
> (or whatever variable name would be used here).
Yes, this makes more sense with the new name, too.
Regards,
Simon
>
>
> 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: w...@denx.de
> "The release of emotion is what keeps us health. Emotionally
> healthy."
> "That may be, Doctor. However, I have noted that the healthy release
> of emotion is frequently unhealthy for those closest to you."
> -- McCoy and Spock, "Plato's Stepchildren", stardate 5784.3
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
Hi Wolfgang, On Sat, Oct 15, 2011 at 12:14 PM, Wolfgang Denk wrote: > Dear Simon Glass, > > In message > you > wrote: >> >> Another thought...the problem is that every character we receive we >> echo, so we can of course never get ahead of the workload. But we can > > Even worse, if your input contains any commands that prodce output, > mayeb even lots of output (say "md $addr 1") then even your > increased buffer size is not going to help you. It helps a little - provided there are no more than 250 characters more 'serial input' after the md command, all is well. But I think this example is a little contrived. > >> way until there is a let-up. Even worse if one of the commands outputs >> something, since then we might be 20 or 30 characters behind, and >> start dropping things. Even if no commands output anything, delays and >> the U-Boot prompt will create a problem. > > Indeed. And this is why the simple rule applies: always wait for the > next command prompt before sending the next line of input. I feel this is a painful restriction. I see no indication of this limitation anywhere on the command line, no error or warning, etc. It is common to want to paste in new bootargs, new network parameters, new environment variables. U-Boot has a command line which purports to support this, and with a fairly straightforward patch and in the majority of cases, it can. Yes this is a minor point, but it does bite people, and usability is important. Regards, Simon > > 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: w...@denx.de > To understand a program you must become both the machine and the > program. > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
Hi Wolfgang, On Sat, Oct 15, 2011 at 12:00 PM, Wolfgang Denk wrote: > Dear Simon Glass, > > In message > you > wrote: >> >> The situation this occurs is when you paste characters into a serial >> terminal connected to U-Boot. This is a pretty common requirement. Yes >> you can manually select each line and paste it but that is a pain. > > But that's exactly how it's supposed to work. Depending on hardware > that may or may not be FIFOs available to buffer such asynchronous > input, and I rather see limited, but consistent behaviour, instead of > fancy stuff that works oin one board but not on another. Well this is particular serial driver is in pretty widespread use. We could of course introduce a serial buffer scheme that extends across all drivers but I don't feel this is the right time, given that there are other priorities in serial. > >> With this patch it is possible to paste a series of 'setenv' commands, >> for example. > > If you want to run multi-line command sequences, then load and run a > script. If the setenv commands are configuration commands (such as network config, or setting up a new boot path) then they may not already exist in the machine. It is much easier to paste commands from a text editor / notepad than create a script, put it onto the tftp server, tftp the script and run it. > > In the special case of setting environment variables, consider using > featurs like "env import" instead. s/and run it/and "env import" it/ > >> This caused an awful lot of confusion here because characters would be >> silently dropped within the command line (normally about 16 chars from >> the end). Then the system would not boot. It is really not a friendly >> experience. > > There should never be character loss when sending data on a line by > line base. Even the slowest processors we support are fast enough to > deal with that. That's right. > > Multi-line input has never been supported. Hence this patch, I feel. Pasting a number of lines into the terminal is convenient, natural, useful, and when it fails it is unexpected and feels like a bug to many. I would certainly like to fix it. Regards, Simon > > 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: w...@denx.de > Alan Turing thought about criteria to settle the question of whether > machines can think, a question of which we now know that it is about > as relevant as the question of whether submarines can swim. > -- Edsger Dijkstra > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] NS16550: buffer reads
On Sunday, October 16, 2011 05:56:56 AM Simon Glass wrote: > Hi Marek, > > On Sat, Oct 15, 2011 at 10:55 AM, Marek Vasut wrote: > > On Saturday, October 15, 2011 06:03:52 PM Simon Glass wrote: > >> From: Scott Wood > >> > >> From: Scott Wood > >> > >> This improves the performance of U-Boot when accepting rapid input, > >> such as pasting a sequence of commands. > > > > Hi Simon, > > > > [...] > > > >> diff --git a/include/ns16550.h b/include/ns16550.h > >> index 51f1c17..60b0abc 100644 > >> --- a/include/ns16550.h > >> +++ b/include/ns16550.h > >> @@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t; > >> > >> void NS16550_init (NS16550_t com_port, int baud_divisor); > >> void NS16550_putc (NS16550_t com_port, char c); > >> -char NS16550_getc (NS16550_t com_port); > >> -int NS16550_tstc (NS16550_t com_port); > >> +char NS16550_getc(NS16550_t regs, unsigned int port); > >> +int NS16550_tstc(NS16550_t regs, unsigned int port); > >> void NS16550_reinit (NS16550_t com_port, int baud_divisor); > > > > Maybe you should fix the indent altogether ? > > I should probably do a separate patch for that. Maybe queue it before this one ? :) Thanks! > > Regards, > Simon > > > Cheers ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] NS16550: buffer reads
Hi Marek, On Sat, Oct 15, 2011 at 10:55 AM, Marek Vasut wrote: > On Saturday, October 15, 2011 06:03:52 PM Simon Glass wrote: >> From: Scott Wood >> >> From: Scott Wood >> >> This improves the performance of U-Boot when accepting rapid input, >> such as pasting a sequence of commands. > > Hi Simon, > > [...] > >> diff --git a/include/ns16550.h b/include/ns16550.h >> index 51f1c17..60b0abc 100644 >> --- a/include/ns16550.h >> +++ b/include/ns16550.h >> @@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t; >> >> void NS16550_init (NS16550_t com_port, int baud_divisor); >> void NS16550_putc (NS16550_t com_port, char c); >> -char NS16550_getc (NS16550_t com_port); >> -int NS16550_tstc (NS16550_t com_port); >> +char NS16550_getc(NS16550_t regs, unsigned int port); >> +int NS16550_tstc(NS16550_t regs, unsigned int port); >> void NS16550_reinit (NS16550_t com_port, int baud_divisor); > > Maybe you should fix the indent altogether ? > I should probably do a separate patch for that. Regards, Simon > Cheers > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
On Sat, Oct 15, 2011 at 8:31 PM, Marek Vasut wrote:
> On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
>> On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut wrote:
>> > On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
>> >> Dear Marek Vasut,
>> >>
>> >> thank you for your comments, please see below:
>> >>
>> >> On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut
> wrote:
>> >> > On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
>> >> >> TPM (Trusted Platform Module) is an integrated circuit and
>> >> >> software platform that provides computer manufacturers with the
>> >> >> core components of a subsystem used to assure authenticity,
>> >> >> integrity and confidentiality.
>> >> >
>> >> > [...]
>> >> >
>> >> > Quick points:
>> >> > * The license
>> >>
>> >> Please suggest the appropriate file header text.
>> >
>> > Uh ... you should know the license !!!
>>
>> removed the BSD part
>
> Are you sure you're not relicensing code you don't own ? I'm just curious,
> what's the origin of the code ? I'd prefer to avoid legal crap.
>
I am sure.
>> [..]
>>
>> >> >> +
>> >> >> +struct lpc_tpm {
>> >> >> + struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
>> >> >> +};
>> >> >
>> >> > Do you need such envelope ?
>> >>
>> >> I think I do - this accurately describes the structure of the chip.
>> >
>> > There's just one member ... it's weird?
>>
>> I think it is appropriate in this case to encapsulate the entire chip
>> description in a structure. Among other things makes it easier to pass
>> a pointer to the chip description around.
>
> can't you pass the locality array ?
>
no, because it would not be clear how big the array is.
>>
>> [..]
>>
>> >> > Dot missing at the end.
>> >>
>> >> ok.
>> >
>> > Please fix globally.
>>
>> done
>>
>> >> >> +#define TPM_DRIVER_ERR (-1)
>> >> >> +
>> >> >> + /* 1 second is plenty for anything TPM does.*/
>> >> >> +#define MAX_DELAY_US (1000 * 1000)
>> >> >> +
>> >> >> +/* Retrieve burst count value out of the status register contents.
>> >> >> */ +#define BURST_COUNT(status) ((u16)(((status) >>
>> >> >> TIS_STS_BURST_COUNT_SHIFT) & \ +
>> >> >> TIS_STS_BURST_COUNT_MASK))
>> >> >
>> >> > Do you need the cast ?
>> >>
>> >> I think it demonstrates the intentional truncation of the value, it
>> >> gets assigned to u16 values down the road, prevents compiler warnings
>> >> about assigning incompatible values in some cases.
>> >
>> > Make it an inline function then, this will do the typechecking for you.
>>
>> I am not sure what is wrong with a short macro in this case - is this
>> against the coding style?
>
> It doesn't do typechecking.
but the code around it does, doesn't it?
Sorry, as I said, I am new here: how does this work on this project -
does the submitter have to agree to all reviewer's comments? Can I ask
somebody else to confirm that using a macro in this case in
inappropriate?
>>
>> >> >> +
>> >> >> +/*
>> >> >> + * Structures defined below allow creating descriptions of TPM
>> >> >> vendor/device + * ID information for run time discovery. The only
>> >> >> device the system knows + * about at this time is Infineon slb9635
>> >> >> + */
>> >> >> +struct device_name {
>> >> >> + u16 dev_id;
>> >> >> + const char * const dev_name;
>> >> >> +};
>> >> >> +
>> >> >> +struct vendor_name {
>> >> >> + u16 vendor_id;
>> >> >> + const char *vendor_name;
>> >> >> + const struct device_name *dev_names;
>> >> >> +};
>> >> >> +
>> >> >> +static const struct device_name infineon_devices[] = {
>> >> >> + {0xb, "SLB9635 TT 1.2"},
>> >> >> + {0}
>> >> >> +};
>> >> >> +
>> >> >> +static const struct vendor_name vendor_names[] = {
>> >> >> + {0x15d1, "Infineon", infineon_devices},
>> >> >> +};
>> >> >> +
>> >> >> +/*
>> >> >> + * Cached vendor/device ID pair to indicate that the device has been
>> >> >> already + * discovered
>> >> >> + */
>> >> >> +static u32 vendor_dev_id;
>> >> >> +
>> >> >> +/* TPM access going through macros to make tracing easier. */
>> >> >> +#define tpm_read(ptr) ({ \
>> >> >> + u32 __ret; \
>> >> >> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
>> >> >> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
>> >> >> + (u32)ptr - (u32)lpc_tpm_dev, __ret); \
>> >> >> + __ret; })
>> >> >> +
>> >> >
>> >> > Make this inline function
>> >> >
>> >> >> +#define tpm_write(value, ptr) ({ \
>> >> >> + u32 __v = value; \
>> >> >> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \
>> >> >> + (u32)ptr - (u32)lpc_tpm_dev, __v); \
>> >> >> + if (sizeof(*ptr) == 1) \
>> >> >> + writeb(__v, ptr); \
>> >> >> + else \
>> >> >> + writel(__v, ptr); })
>> >> >> +
>> >> >
>> >> > DTTO
>> >>
>> >> Are you sure these will work as inline functions?
>> >
>> > Why not ? Also, why do you introduce the __v ?
>>
>> macro vs function: need to be able to tell the pointed objec
[U-Boot] Porting lpc313x support up to current u-boot
I'm porting the lpc313x uboot located here, up to current uboot. It is based on U-boot.2009.11.1 http://git.lpclinux.com/?p=uboot-2009.11-lpc313x.git;a=summary What do I do with the cpu directory? http://git.lpclinux.com/?p=uboot-2009.11-lpc313x.git;a=tree;f=cpu/lpc313x;h=5cc2c80b1ef75c729abff5ecbcaaef6c49f181b1;hb=HEAD It has a lds file in it containing symbols needs by init.c This is an arm926ejs based cpu so I am working on moving it into the arm926ejs directory, but how do I handle the linker script? No one at NXP seems interested in helping with this. When I get it working again I'll submit it to the main repository. -- Jon Smirl jonsm...@gmail.com ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] x86: Rename include/asm/ic to include/asm/arch-sc520
Hi Wolfgang,
On 16/10/11 14:29, Graeme Russ wrote:
> Also include some trivial related cleanups
>
> Signed-off-by: Graeme Russ
> ---
> arch/x86/cpu/sc520/sc520.c |2 +-
> arch/x86/cpu/sc520/sc520_car.S |2 +-
> arch/x86/cpu/sc520/sc520_pci.c |4 ++--
> arch/x86/cpu/sc520/sc520_reset.c|2 +-
> arch/x86/cpu/sc520/sc520_sdram.c|2 +-
> arch/x86/cpu/sc520/sc520_ssi.c |4 ++--
> arch/x86/cpu/sc520/sc520_timer.c|2 +-
> arch/x86/include/asm/{ic => arch-sc520}/pci.h |0
> arch/x86/include/asm/{ic => arch-sc520}/sc520.h |0
> arch/x86/include/asm/{ic => arch-sc520}/ssi.h |0
> arch/x86/lib/zimage.c |1 -
> board/eNET/eNET.c |2 +-
> board/eNET/eNET_pci.c |2 +-
> board/eNET/eNET_start16.S |4 +---
> 14 files changed, 12 insertions(+), 15 deletions(-)
> rename arch/x86/include/asm/{ic => arch-sc520}/pci.h (100%)
> rename arch/x86/include/asm/{ic => arch-sc520}/sc520.h (100%)
> rename arch/x86/include/asm/{ic => arch-sc520}/ssi.h (100%)
I missed the merge window by a day, but since this is a tidy-up that
touches only x86, do you mind if I apply it to the x86 repo anyway before I
send through a pull request?
It is checkpatch clean in case your wondering
Regards,
Graeme
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
> On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut wrote:
> > On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
> >> Dear Marek Vasut,
> >>
> >> thank you for your comments, please see below:
> >>
> >> On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut
wrote:
> >> > On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
> >> >> TPM (Trusted Platform Module) is an integrated circuit and
> >> >> software platform that provides computer manufacturers with the
> >> >> core components of a subsystem used to assure authenticity,
> >> >> integrity and confidentiality.
> >> >
> >> > [...]
> >> >
> >> > Quick points:
> >> > * The license
> >>
> >> Please suggest the appropriate file header text.
> >
> > Uh ... you should know the license !!!
>
> removed the BSD part
Are you sure you're not relicensing code you don't own ? I'm just curious,
what's the origin of the code ? I'd prefer to avoid legal crap.
> [..]
>
> >> >> +
> >> >> +struct lpc_tpm {
> >> >> + struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
> >> >> +};
> >> >
> >> > Do you need such envelope ?
> >>
> >> I think I do - this accurately describes the structure of the chip.
> >
> > There's just one member ... it's weird?
>
> I think it is appropriate in this case to encapsulate the entire chip
> description in a structure. Among other things makes it easier to pass
> a pointer to the chip description around.
can't you pass the locality array ?
>
> [..]
>
> >> > Dot missing at the end.
> >>
> >> ok.
> >
> > Please fix globally.
>
> done
>
> >> >> +#define TPM_DRIVER_ERR (-1)
> >> >> +
> >> >> + /* 1 second is plenty for anything TPM does.*/
> >> >> +#define MAX_DELAY_US (1000 * 1000)
> >> >> +
> >> >> +/* Retrieve burst count value out of the status register contents.
> >> >> */ +#define BURST_COUNT(status) ((u16)(((status) >>
> >> >> TIS_STS_BURST_COUNT_SHIFT) & \ +
> >> >> TIS_STS_BURST_COUNT_MASK))
> >> >
> >> > Do you need the cast ?
> >>
> >> I think it demonstrates the intentional truncation of the value, it
> >> gets assigned to u16 values down the road, prevents compiler warnings
> >> about assigning incompatible values in some cases.
> >
> > Make it an inline function then, this will do the typechecking for you.
>
> I am not sure what is wrong with a short macro in this case - is this
> against the coding style?
It doesn't do typechecking.
>
> >> >> +
> >> >> +/*
> >> >> + * Structures defined below allow creating descriptions of TPM
> >> >> vendor/device + * ID information for run time discovery. The only
> >> >> device the system knows + * about at this time is Infineon slb9635
> >> >> + */
> >> >> +struct device_name {
> >> >> + u16 dev_id;
> >> >> + const char * const dev_name;
> >> >> +};
> >> >> +
> >> >> +struct vendor_name {
> >> >> + u16 vendor_id;
> >> >> + const char *vendor_name;
> >> >> + const struct device_name *dev_names;
> >> >> +};
> >> >> +
> >> >> +static const struct device_name infineon_devices[] = {
> >> >> + {0xb, "SLB9635 TT 1.2"},
> >> >> + {0}
> >> >> +};
> >> >> +
> >> >> +static const struct vendor_name vendor_names[] = {
> >> >> + {0x15d1, "Infineon", infineon_devices},
> >> >> +};
> >> >> +
> >> >> +/*
> >> >> + * Cached vendor/device ID pair to indicate that the device has been
> >> >> already + * discovered
> >> >> + */
> >> >> +static u32 vendor_dev_id;
> >> >> +
> >> >> +/* TPM access going through macros to make tracing easier. */
> >> >> +#define tpm_read(ptr) ({ \
> >> >> + u32 __ret; \
> >> >> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
> >> >> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
> >> >> +(u32)ptr - (u32)lpc_tpm_dev, __ret); \
> >> >> + __ret; })
> >> >> +
> >> >
> >> > Make this inline function
> >> >
> >> >> +#define tpm_write(value, ptr) ({\
> >> >> + u32 __v = value;\
> >> >> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \
> >> >> +(u32)ptr - (u32)lpc_tpm_dev, __v); \
> >> >> + if (sizeof(*ptr) == 1) \
> >> >> + writeb(__v, ptr); \
> >> >> + else \
> >> >> + writel(__v, ptr); })
> >> >> +
> >> >
> >> > DTTO
> >>
> >> Are you sure these will work as inline functions?
> >
> > Why not ? Also, why do you introduce the __v ?
>
> macro vs function: need to be able to tell the pointed object size at run
> time.
This seems wrong like hell.
>
> __v is needed to avoid side effects when invoking the macro.
Side effects ? What side effects ?
[...]
Cheers
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] x86: turn off cache: set control register properly
On 01/10/11 04:27, Ondrej Kupka wrote: > Bits should be ORed when they are supposed to be added together > > Cc: Graeme Russ > Signed-off-by: Ondrej Kupka > --- > arch/x86/cpu/start16.S |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > Applied to u-boot-x86 Thanks, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] x86: Rename include/asm/ic to include/asm/arch-sc520
Also include some trivial related cleanups
Signed-off-by: Graeme Russ
---
arch/x86/cpu/sc520/sc520.c |2 +-
arch/x86/cpu/sc520/sc520_car.S |2 +-
arch/x86/cpu/sc520/sc520_pci.c |4 ++--
arch/x86/cpu/sc520/sc520_reset.c|2 +-
arch/x86/cpu/sc520/sc520_sdram.c|2 +-
arch/x86/cpu/sc520/sc520_ssi.c |4 ++--
arch/x86/cpu/sc520/sc520_timer.c|2 +-
arch/x86/include/asm/{ic => arch-sc520}/pci.h |0
arch/x86/include/asm/{ic => arch-sc520}/sc520.h |0
arch/x86/include/asm/{ic => arch-sc520}/ssi.h |0
arch/x86/lib/zimage.c |1 -
board/eNET/eNET.c |2 +-
board/eNET/eNET_pci.c |2 +-
board/eNET/eNET_start16.S |4 +---
14 files changed, 12 insertions(+), 15 deletions(-)
rename arch/x86/include/asm/{ic => arch-sc520}/pci.h (100%)
rename arch/x86/include/asm/{ic => arch-sc520}/sc520.h (100%)
rename arch/x86/include/asm/{ic => arch-sc520}/ssi.h (100%)
diff --git a/arch/x86/cpu/sc520/sc520.c b/arch/x86/cpu/sc520/sc520.c
index e37c403..4892c01 100644
--- a/arch/x86/cpu/sc520/sc520.c
+++ b/arch/x86/cpu/sc520/sc520.c
@@ -27,7 +27,7 @@
#include
#include
#include
-#include
+#include
DECLARE_GLOBAL_DATA_PTR;
diff --git a/arch/x86/cpu/sc520/sc520_car.S b/arch/x86/cpu/sc520/sc520_car.S
index a33f94f..7cac4d1 100644
--- a/arch/x86/cpu/sc520/sc520_car.S
+++ b/arch/x86/cpu/sc520/sc520_car.S
@@ -23,7 +23,7 @@
#include
#include
-#include
+#include
.section .text
diff --git a/arch/x86/cpu/sc520/sc520_pci.c b/arch/x86/cpu/sc520/sc520_pci.c
index 32d4802..e26793a 100644
--- a/arch/x86/cpu/sc520/sc520_pci.c
+++ b/arch/x86/cpu/sc520/sc520_pci.c
@@ -28,8 +28,8 @@
#include
#include
#include
-#include
-#include
+#include
+#include
static struct {
u8 priority;
diff --git a/arch/x86/cpu/sc520/sc520_reset.c b/arch/x86/cpu/sc520/sc520_reset.c
index 18890c3..137af97 100644
--- a/arch/x86/cpu/sc520/sc520_reset.c
+++ b/arch/x86/cpu/sc520/sc520_reset.c
@@ -26,7 +26,7 @@
#include
#include
-#include
+#include
DECLARE_GLOBAL_DATA_PTR;
diff --git a/arch/x86/cpu/sc520/sc520_sdram.c b/arch/x86/cpu/sc520/sc520_sdram.c
index f3623f5..57e4e7d 100644
--- a/arch/x86/cpu/sc520/sc520_sdram.c
+++ b/arch/x86/cpu/sc520/sc520_sdram.c
@@ -24,7 +24,7 @@
#include
#include
#include
-#include
+#include
DECLARE_GLOBAL_DATA_PTR;
diff --git a/arch/x86/cpu/sc520/sc520_ssi.c b/arch/x86/cpu/sc520/sc520_ssi.c
index 47aa80b..3a6a858 100644
--- a/arch/x86/cpu/sc520/sc520_ssi.c
+++ b/arch/x86/cpu/sc520/sc520_ssi.c
@@ -23,8 +23,8 @@
#include
#include
-#include
-#include
+#include
+#include
int ssi_set_interface(int freq, int lsb_first, int inv_clock, int inv_phase)
{
diff --git a/arch/x86/cpu/sc520/sc520_timer.c b/arch/x86/cpu/sc520/sc520_timer.c
index 5cccda1..05bc9c1 100644
--- a/arch/x86/cpu/sc520/sc520_timer.c
+++ b/arch/x86/cpu/sc520/sc520_timer.c
@@ -27,7 +27,7 @@
#include
#include
#include
-#include
+#include
void sc520_timer_isr(void)
{
diff --git a/arch/x86/include/asm/ic/pci.h
b/arch/x86/include/asm/arch-sc520/pci.h
similarity index 100%
rename from arch/x86/include/asm/ic/pci.h
rename to arch/x86/include/asm/arch-sc520/pci.h
diff --git a/arch/x86/include/asm/ic/sc520.h
b/arch/x86/include/asm/arch-sc520/sc520.h
similarity index 100%
rename from arch/x86/include/asm/ic/sc520.h
rename to arch/x86/include/asm/arch-sc520/sc520.h
diff --git a/arch/x86/include/asm/ic/ssi.h
b/arch/x86/include/asm/arch-sc520/ssi.h
similarity index 100%
rename from arch/x86/include/asm/ic/ssi.h
rename to arch/x86/include/asm/arch-sc520/ssi.h
diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
index 6682e0d..d2dd6fd 100644
--- a/arch/x86/lib/zimage.c
+++ b/arch/x86/lib/zimage.c
@@ -35,7 +35,6 @@
#include
#include
#include
-#include
/*
* Memory lay-out:
diff --git a/board/eNET/eNET.c b/board/eNET/eNET.c
index 2a5636c..c4ed820 100644
--- a/board/eNET/eNET.c
+++ b/board/eNET/eNET.c
@@ -23,7 +23,7 @@
#include
#include
-#include
+#include
#include
#include
diff --git a/board/eNET/eNET_pci.c b/board/eNET/eNET_pci.c
index d97387e..29d13d2 100644
--- a/board/eNET/eNET_pci.c
+++ b/board/eNET/eNET_pci.c
@@ -27,7 +27,7 @@
#include
#include
#include
-#include
+#include
static void pci_enet_fixup_irq(struct pci_controller *hose, pci_dev_t dev)
{
diff --git a/board/eNET/eNET_start16.S b/board/eNET/eNET_start16.S
index 77e5519..4241f6e 100644
--- a/board/eNET/eNET_start16.S
+++ b/board/eNET/eNET_start16.S
@@ -27,11 +27,9 @@
* that is used by U-boot to its final destination.
*/
-/* #include */
-
#include "config.h"
#include "hardware.h"
-#include
+#include
.text
.section .start16, "ax"
--
1.7.5.2.317.g391b14
___
U-Boot mailing l
Re: [U-Boot] [PATCH v3 1/2] Introduce generic TPM support in u-boot
On Sat, Oct 15, 2011 at 6:20 PM, Vadim Bendebury wrote: > > sorry, sent this and the other patchset with a wrong version number, > will resend with the correct number. > or maybe not - looks like the latest patches were sent with the correct version number (v3) but gmail reader seems to be collapsing text in square brackets in the subject line... cheers, /vb > BTW, here one can see the differences between v2 and v3 > http://review-t.appspot.com/23001 and > http://review-t.appspot.com/24001. > > Among other things this app allows adding comments while reviewing the > diffs (just doubleclick on the side by side diffs page). > > Has it been ever discussed - setting up a tool like this for u-boot? > > cheers, > /vb > ___ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/2] Introduce generic TPM support in u-boot
On Sat, Oct 15, 2011 at 6:13 PM, Vadim Bendebury wrote: > TPM (Trusted Platform Module) is an integrated circuit and > software platform that provides computer manufacturers with the > core components of a subsystem used to assure authenticity, > integrity and confidentiality. > > This driver supports version 1.2 of the TCG (Trusted Computing > Group) specifications. > > The TCG specification defines several so called localities in a > TPM chip, to be controlled by different software layers. When > used on a typical x86 platform during the firmware phase, only > locality 0 can be accessed by the CPU, so this driver even while > supporting the locality concept presumes that only locality zero > is used. > > This implementation is loosely based on the article "Writing a > TPM Device Driver" published on http://ptgmedia.pearsoncmg.com > > Compiling this driver with DEBUG defined will generate trace of > all accesses to TMP registers. > > This driver has been tested and is being used in three different > functional ChromeOS machines (Pinetrail and Sandy Bridge Intel > chipsets) all using the same Infineon SLB 9635 TT 1.2 device. > > A u-boot cli command allowing access to the TPM was also > implemented and is being submitted as a second patch. > > Change-Id: I22a33c3e5b2e20eec9557a7621bd463b30389d73 > Signed-off-by: Vadim Bendebury > CC: Wolfgang Denk > --- > Makefile | 3 + > README | 10 + > drivers/tpm/Makefile | 43 > drivers/tpm/generic_lpc_tpm.c | 485 > + > include/tpm.h | 71 ++ > 5 files changed, 612 insertions(+), 0 deletions(-) > create mode 100644 drivers/tpm/Makefile > create mode 100644 drivers/tpm/generic_lpc_tpm.c > create mode 100644 include/tpm.h > > diff --git a/Makefile b/Makefile > index 5db2e0e..df86088 100644 > --- a/Makefile > +++ b/Makefile > @@ -268,6 +268,9 @@ LIBS += arch/powerpc/cpu/mpc8xxx/lib8xxx.o > endif > LIBS += drivers/rtc/librtc.o > LIBS += drivers/serial/libserial.o > +ifeq ($(CONFIG_GENERIC_LPC_TPM),y) > +LIBS += drivers/tpm/libtpm.o > +endif > LIBS += drivers/twserial/libtws.o > LIBS += drivers/usb/eth/libusb_eth.o > LIBS += drivers/usb/gadget/libusb_gadget.o > diff --git a/README b/README > index 7e032a9..bcd3695 100644 > --- a/README > +++ b/README > @@ -1018,6 +1018,16 @@ The following options need to be configured: > CONFIG_SH_ETHER_CACHE_WRITEBACK > If this option is set, the driver enables cache flush. > > +- TPM Support: > + CONFIG_GENERIC_LPC_TPM > + Support for generic parallel port TPM devices. Only one device > + per system is supported at this time. > + > + CONFIG_TPM_TIS_BASE_ADDRESS > + Base address where the generic TPM device is mapped > + to. Contemporary x86 systems usually map it at > + 0xfed4. > + > - USB Support: > At the moment only the UHCI host controller is > supported (PIP405, MIP405, MPC5200); define > diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile > new file mode 100644 > index 000..be11c8b > --- /dev/null > +++ b/drivers/tpm/Makefile > @@ -0,0 +1,43 @@ > +# Copyright (c) 2011 The Chromium OS Authors. All rights reserved. > +# > +# See file CREDITS for list of people who contributed to this > +# project. > +# > +# This program is free software; you can redistribute it and/or > +# modify it under the terms of the GNU General Public License as > +# published by the Free Software Foundation; either version 2 of > +# the License, or (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write to the Free Software > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, > +# MA 02111-1307 USA > +# > + > +include $(TOPDIR)/config.mk > + > +LIB := $(obj)libtpm.o > + > +COBJS-$(CONFIG_GENERIC_LPC_TPM) = generic_lpc_tpm.o > + > +COBJS := $(COBJS-y) > +SRCS := $(COBJS:.o=.c) > +OBJS := $(addprefix $(obj),$(COBJS)) > + > +all: $(LIB) > + > +$(LIB): $(obj).depend $(OBJS) > + $(call cmd_link_o_target, $(OBJS)) > + > +# > + > +include $(SRCTREE)/rules.mk > + > +sinclude $(obj).depend > + > +# > diff --git a/drivers/tpm/generic_lpc_tpm.c b/drivers/tpm/generic_lpc_tpm.c > new file mode 100644 > index 000..6b58420 > --- /dev/null > +++ b/drivers/tpm/generic_lpc_tpm.c > @@ -0,0 +1,485 @@ > +/*
[U-Boot] [PATCH v3 2/2] Add a cli command to test the TPM device.
The command gets an arbitrary number of arguments (up to 30), which
are interpreted as byte values and are feed into the TPM device after
proper initialization. Then the return value and data of the TPM
driver is examined.
TPM commands are described in the TCG specification.
For instance, the following sequence is the 'TPM Startup' command, it
is processed by the TPM and a response is generated:
boot > tpm 0x0 0xc1 0x0 0x0 0x0 0xc 0x0 0x0 0x0 0x99 0x0 0x1
Found TPM SLB9635 TT 1.2 by Infineon
Got TPM response:
00 c4 00 00 00 0a 00 00 00 00
If the command is corrupted (fed one byte short), an error is reported:
boot > tpm 0x0 0xc1 0x0 0x0 0x0 0xc 0x0 0x0 0x0 0x99 0x0
generic_lpc_tpm.c:311 unexpected TPM status 0xff000888
generic_lpc_tpm.c:516 failed sending data to TPM
tpm command failed
boot >
Change-Id: I3f3c5bfec8b852e208c4e99ba37b0f2b875140b0
Signed-off-by: Vadim Bendebury
CC: Wolfgang Denk
---
common/Makefile |1 +
common/cmd_tpm.c | 103 ++
2 files changed, 104 insertions(+), 0 deletions(-)
create mode 100644 common/cmd_tpm.c
diff --git a/common/Makefile b/common/Makefile
index fdc4206..fb0c7fe 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -149,6 +149,7 @@ COBJS-$(CONFIG_CMD_STRINGS) += cmd_strings.o
COBJS-$(CONFIG_CMD_TERMINAL) += cmd_terminal.o
COBJS-$(CONFIG_CMD_TIME) += cmd_time.o
COBJS-$(CONFIG_SYS_HUSH_PARSER) += cmd_test.o
+COBJS-$(CONFIG_CMD_TPM) += cmd_tpm.o
COBJS-$(CONFIG_CMD_TSI148) += cmd_tsi148.o
COBJS-$(CONFIG_CMD_UBI) += cmd_ubi.o
COBJS-$(CONFIG_CMD_UBIFS) += cmd_ubifs.o
diff --git a/common/cmd_tpm.c b/common/cmd_tpm.c
new file mode 100644
index 000..6f5cd48
--- /dev/null
+++ b/common/cmd_tpm.c
@@ -0,0 +1,103 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include
+#include
+#include
+
+#define MAX_TRANSACTION_SIZE 30
+
+/*
+ * tpm_write() expects a variable number of parameters: the internal address
+ * followed by data to write, byte by byte.
+ *
+ * Returns 0 on success or -1 on errors (wrong arguments or TPM failure).
+ */
+static int tpm_process(int argc, char * const argv[], cmd_tbl_t *cmdtp)
+{
+ u8 tpm_buffer[MAX_TRANSACTION_SIZE];
+ u32 write_size, read_size;
+ char *p;
+ int rv = -1;
+
+ for (write_size = 0; write_size < argc; write_size++) {
+ u32 datum = simple_strtoul(argv[write_size], &p, 0);
+ if (*p || (datum > 0xff)) {
+ printf("\n%s: bad data value\n\n", argv[write_size]);
+ cmd_usage(cmdtp);
+ return rv;
+ }
+ tpm_buffer[write_size] = (u8)datum;
+ }
+
+ read_size = sizeof(tpm_buffer);
+ if (!tis_sendrecv(tpm_buffer, write_size, tpm_buffer, &read_size)) {
+ int i;
+ puts("Got TPM response:\n");
+ for (i = 0; i < read_size; i++)
+ printf(" %2.2x", tpm_buffer[i]);
+ puts("\n");
+ rv = 0;
+ } else {
+ puts("tpm command failed\n");
+ }
+ return rv;
+}
+
+static int do_tpm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+ int rv = 0;
+
+ /*
+* Verify that in case it is present, the first argument, it is
+* exactly one character in size.
+*/
+ if (argc < 7) {
+ puts("command should be at least six bytes in size\n");
+ return -1;
+ }
+
+ if (tis_init()) {
+ puts("tis_init() failed!\n");
+ return -1;
+ }
+
+ if (tis_open()) {
+ puts("tis_open() failed!\n");
+ return -1;
+ }
+
+ rv = tpm_process(argc - 1, argv + 1, cmdtp);
+
+ if (tis_close()) {
+ puts("tis_close() failed!\n");
+ rv = -1;
+ }
+
+ return rv;
+}
+
+U_BOOT_CMD(tpm, MAX_TRANSACTION_SIZE, 1, do_tpm,
+ " [ ...] - write data and read response",
+ "send arbitrary data (at least 6 bytes) to the TPM "
+ "device and read the response"
+);
--
1.7.3.1
[U-Boot] [PATCH v3 1/2] Introduce generic TPM support in u-boot
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality. This driver supports version 1.2 of the TCG (Trusted Computing Group) specifications. The TCG specification defines several so called localities in a TPM chip, to be controlled by different software layers. When used on a typical x86 platform during the firmware phase, only locality 0 can be accessed by the CPU, so this driver even while supporting the locality concept presumes that only locality zero is used. This implementation is loosely based on the article "Writing a TPM Device Driver" published on http://ptgmedia.pearsoncmg.com Compiling this driver with DEBUG defined will generate trace of all accesses to TMP registers. This driver has been tested and is being used in three different functional ChromeOS machines (Pinetrail and Sandy Bridge Intel chipsets) all using the same Infineon SLB 9635 TT 1.2 device. A u-boot cli command allowing access to the TPM was also implemented and is being submitted as a second patch. Change-Id: I22a33c3e5b2e20eec9557a7621bd463b30389d73 Signed-off-by: Vadim Bendebury CC: Wolfgang Denk --- Makefile |3 + README| 10 + drivers/tpm/Makefile | 43 drivers/tpm/generic_lpc_tpm.c | 485 + include/tpm.h | 71 ++ 5 files changed, 612 insertions(+), 0 deletions(-) create mode 100644 drivers/tpm/Makefile create mode 100644 drivers/tpm/generic_lpc_tpm.c create mode 100644 include/tpm.h diff --git a/Makefile b/Makefile index 5db2e0e..df86088 100644 --- a/Makefile +++ b/Makefile @@ -268,6 +268,9 @@ LIBS += arch/powerpc/cpu/mpc8xxx/lib8xxx.o endif LIBS += drivers/rtc/librtc.o LIBS += drivers/serial/libserial.o +ifeq ($(CONFIG_GENERIC_LPC_TPM),y) +LIBS += drivers/tpm/libtpm.o +endif LIBS += drivers/twserial/libtws.o LIBS += drivers/usb/eth/libusb_eth.o LIBS += drivers/usb/gadget/libusb_gadget.o diff --git a/README b/README index 7e032a9..bcd3695 100644 --- a/README +++ b/README @@ -1018,6 +1018,16 @@ The following options need to be configured: CONFIG_SH_ETHER_CACHE_WRITEBACK If this option is set, the driver enables cache flush. +- TPM Support: + CONFIG_GENERIC_LPC_TPM + Support for generic parallel port TPM devices. Only one device + per system is supported at this time. + + CONFIG_TPM_TIS_BASE_ADDRESS + Base address where the generic TPM device is mapped + to. Contemporary x86 systems usually map it at + 0xfed4. + - USB Support: At the moment only the UHCI host controller is supported (PIP405, MIP405, MPC5200); define diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile new file mode 100644 index 000..be11c8b --- /dev/null +++ b/drivers/tpm/Makefile @@ -0,0 +1,43 @@ +# Copyright (c) 2011 The Chromium OS Authors. All rights reserved. +# +# See file CREDITS for list of people who contributed to this +# project. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation; either version 2 of +# the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, +# MA 02111-1307 USA +# + +include $(TOPDIR)/config.mk + +LIB := $(obj)libtpm.o + +COBJS-$(CONFIG_GENERIC_LPC_TPM) = generic_lpc_tpm.o + +COBJS := $(COBJS-y) +SRCS := $(COBJS:.o=.c) +OBJS := $(addprefix $(obj),$(COBJS)) + +all: $(LIB) + +$(LIB): $(obj).depend $(OBJS) + $(call cmd_link_o_target, $(OBJS)) + +# + +include $(SRCTREE)/rules.mk + +sinclude $(obj).depend + +# diff --git a/drivers/tpm/generic_lpc_tpm.c b/drivers/tpm/generic_lpc_tpm.c new file mode 100644 index 000..6b58420 --- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c @@ -0,0 +1,485 @@ +/* + * Copyright (c) 2011 The Chromium OS Authors. + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation;
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
On Sat, Oct 15, 2011 at 12:42 PM, Mike Frysinger wrote:
> On Friday 14 October 2011 23:38:50 Vadim Bendebury wrote:
>> --- /dev/null
>> +++ b/drivers/tpm/generic_lpc_tpm.c
>>
>> +#define TPM_TIMEOUT_ERR (~0)
>> +#define TPM_DRIVER_ERR (-1)
>
> these are the same thing. another reason why you shouldn't mix ~ with normal
> values. use -2 or something.
>
done
>> +/* TPM access going through macros to make tracing easier. */
>> +#define tpm_read(ptr) ({ \
>> + u32 __ret; \
>> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
>> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
>> + (u32)ptr - (u32)lpc_tpm_dev, __ret); \
>> + __ret; })
>
> that last "__ret;" is indented way too far. it should be on the same level as
> "u32 __ret;" and such.
>
done
>> +#define tpm_write(value, ptr) ({ \
>> + u32 __v = value; \
>> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \
>> + (u32)ptr - (u32)lpc_tpm_dev, __v); \
>> + if (sizeof(*ptr) == 1) \
>> + writeb(__v, ptr); \
>> + else \
>> + writel(__v, ptr); })
>
> ({...}) doesn't make sense here. this should be a do{...}while(0).
>
done
[..]
cheers,
/vb
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
On Sat, Oct 15, 2011 at 12:25 PM, Wolfgang Denk wrote:
> Dear Vadim Bendebury,
>
> In message <20111015033850.74ad541...@eskimo.mtv.corp.google.com> you wrote:
>> TPM (Trusted Platform Module) is an integrated circuit and
>> software platform that provides computer manufacturers with the
>> core components of a subsystem used to assure authenticity,
>> integrity and confidentiality.
> ...
>> --- /dev/null
>> +++ b/drivers/tpm/Makefile
>> @@ -0,0 +1,27 @@
>> +# Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
>> +# Use of this source code is governed by a BSD-style license that can be
>> +# found in the LICENSE file.
>
> Please fix this. This has been pointed out before. Please work a
> bit more careful. Thanks.
>
done
> ...
>> +++ b/drivers/tpm/generic_lpc_tpm.c
>> @@ -0,0 +1,488 @@
>> +/*
>> + * Copyright (c) 2011 The Chromium OS Authors.
>> + * Released under the 2-clause BSD license.
> ...
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>
> If we can have it under GPLv2+, then we don't need the BSD part.
> Please drop these lines.
>
done
> ...
>> +/* TPM access going through macros to make tracing easier. */
>> +#define tpm_read(ptr) ({ \
>> + u32 __ret; \
>> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
>> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
>> + (u32)ptr - (u32)lpc_tpm_dev, __ret); \
>> + __ret; })
>
> Please test with something like this instead:
>
> debug("%sRead reg 0x%x returns 0x%x\n", PREFIX, ...
>
> It might result in smaller code. Please verify. If so, please fix
> globally.
>
did not see any difference in the code layout as reported by objdump
of the module
> ...
>> + vid = didvid & 0x;
>> + did = (didvid >> 16) & 0x;
>> + for (i = 0; i < ARRAY_SIZE(vendor_names); i++) {
>> + int j = 0;
>> + u16 known_did;
>> + if (vid == vendor_names[i].vendor_id)
>> + vendor_name = vendor_names[i].vendor_name;
>
> Please separate declarations and code by a blank line.
>
done
>
>> + burst = BURST_COUNT(value);
>> + if ((offset == (len - 1)) && burst)
>> + /*
>> + * We need to be able to send the last byte to the
>> + * device, so burst size must be nonzero before we
>> + * break out.
>> + */
>> + break;
>
> We require braces around multiline statements.
>
done
>
> 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: w...@denx.de
> "Deliver yesterday, code today, think tomorrow."
>
chrrs,
/vb
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut wrote:
> On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
>> Dear Marek Vasut,
>>
>> thank you for your comments, please see below:
>>
>> On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut wrote:
>> > On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
>> >> TPM (Trusted Platform Module) is an integrated circuit and
>> >> software platform that provides computer manufacturers with the
>> >> core components of a subsystem used to assure authenticity,
>> >> integrity and confidentiality.
>> >
>> > [...]
>> >
>> > Quick points:
>> > * The license
>>
>> Please suggest the appropriate file header text.
>
> Uh ... you should know the license !!!
removed the BSD part
>>
[..]
>> >> +
>> >> +struct lpc_tpm {
>> >> + struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
>> >> +};
>> >
>> > Do you need such envelope ?
>>
>> I think I do - this accurately describes the structure of the chip.
>
> There's just one member ... it's weird?
>
I think it is appropriate in this case to encapsulate the entire chip
description in a structure. Among other things makes it easier to pass
a pointer to the chip description around.
[..]
>> >
>> > Dot missing at the end.
>>
>> ok.
>
> Please fix globally.
>
done
>>
>> >> +#define TPM_DRIVER_ERR (-1)
>> >> +
>> >> + /* 1 second is plenty for anything TPM does.*/
>> >> +#define MAX_DELAY_US (1000 * 1000)
>> >> +
>> >> +/* Retrieve burst count value out of the status register contents. */
>> >> +#define BURST_COUNT(status) ((u16)(((status) >>
>> >> TIS_STS_BURST_COUNT_SHIFT) & \ +
>> >> TIS_STS_BURST_COUNT_MASK))
>> >
>> > Do you need the cast ?
>>
>> I think it demonstrates the intentional truncation of the value, it
>> gets assigned to u16 values down the road, prevents compiler warnings
>> about assigning incompatible values in some cases.
>
> Make it an inline function then, this will do the typechecking for you.
>
I am not sure what is wrong with a short macro in this case - is this
against the coding style?
>>
>> >> +
>> >> +/*
>> >> + * Structures defined below allow creating descriptions of TPM
>> >> vendor/device + * ID information for run time discovery. The only device
>> >> the system knows + * about at this time is Infineon slb9635
>> >> + */
>> >> +struct device_name {
>> >> + u16 dev_id;
>> >> + const char * const dev_name;
>> >> +};
>> >> +
>> >> +struct vendor_name {
>> >> + u16 vendor_id;
>> >> + const char *vendor_name;
>> >> + const struct device_name *dev_names;
>> >> +};
>> >> +
>> >> +static const struct device_name infineon_devices[] = {
>> >> + {0xb, "SLB9635 TT 1.2"},
>> >> + {0}
>> >> +};
>> >> +
>> >> +static const struct vendor_name vendor_names[] = {
>> >> + {0x15d1, "Infineon", infineon_devices},
>> >> +};
>> >> +
>> >> +/*
>> >> + * Cached vendor/device ID pair to indicate that the device has been
>> >> already + * discovered
>> >> + */
>> >> +static u32 vendor_dev_id;
>> >> +
>> >> +/* TPM access going through macros to make tracing easier. */
>> >> +#define tpm_read(ptr) ({ \
>> >> + u32 __ret; \
>> >> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
>> >> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
>> >> + (u32)ptr - (u32)lpc_tpm_dev, __ret); \
>> >> + __ret; })
>> >> +
>> >
>> > Make this inline function
>> >
>> >> +#define tpm_write(value, ptr) ({ \
>> >> + u32 __v = value; \
>> >> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \
>> >> + (u32)ptr - (u32)lpc_tpm_dev, __v); \
>> >> + if (sizeof(*ptr) == 1) \
>> >> + writeb(__v, ptr); \
>> >> + else \
>> >> + writel(__v, ptr); })
>> >> +
>> >
>> > DTTO
>>
>> Are you sure these will work as inline functions?
>
> Why not ? Also, why do you introduce the __v ?
macro vs function: need to be able to tell the pointed object size at run time.
__v is needed to avoid side effects when invoking the macro.
>>
>> > [...]
>> >
>> >> +static u32 tis_senddata(const u8 * const data, u32 len)
>> >> +{
>> >> + u32 offset = 0;
>> >> + u16 burst = 0;
>> >> + u32 max_cycles = 0;
>> >> + u8 locality = 0;
>> >> + u32 value;
>> >> +
>> >> + value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
>> >> + TIS_STS_COMMAND_READY,
>> >> TIS_STS_COMMAND_READY); + if (value == TPM_TIMEOUT_ERR) {
>> >> + printf("%s:%d - failed to get 'command_ready' status\n",
>> >> + __FILE__, __LINE__);
>> >> + return TPM_DRIVER_ERR;
>> >> + }
>> >> + burst = BURST_COUNT(value);
>> >> +
>> >> + while (1) {
>> >
>> > No endless loops please.
>>
>> You are the third person looking at this, but the first one
>> uncomfortable with this. Obviously this is not an endless loop, there
>> are three points where the loop terminates, two on timeout errors and
>> one on su
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
Dear Wolfgang Denk,
On Sat, Oct 15, 2011 at 12:44 PM, Wolfgang Denk wrote:
> Dear Vadim Bendebury,
>
> In message
> you
> wrote:
>>
>> >> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
>> >> + * Released under the 2-clause BSD license.
>> >
>> > Are we ok with this ? Also, you say something about GPL in the same
>> > comment?
>> >
>>
>> Can someone please tell me what needs to be put in the license headers
>> and I will do it. I hear different suggestions from different people.
>
> See previous comment - drop the BSD part if you include a GPLv2+
> license header.
>
done
>> >> + return ~0;
>> >
>> > Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
>>
>> I was under impression that any nonzero value is good. I see sometimes
>> -1 returned for error in other u-boot sources. Also, I am sorry, I am
>> new to this, when someone says "it is weird" - does this mean that it
>> has to be changed?
>
> Commands are running in some sort of shell environment. SO please
> return 0 for OK and 1 for general errors like all other commands do
> (or should do).
>
done
> ...
>> >> +static void report_error(const char *msg)
>> >> +{
>> >> + if (msg && *msg)
>> >
>> > Uhm, you also check if first character is non-zero? why ?
>>
>> To avoid printing an empty string if someone calls this with an empty
>> message?
>
> It's your code, so just don't do it, then.
>
> And what's wrong about printing an empty string? YOuy're just adding
> dead code (and increased memory footprint) here.
>
>> > Two underscores aren't a good practice.
>>
>> I did this as a result of a previous review. Do you have a suggestion
>> how this should be done instead?
>
> First, and most important, __u_boot_cmd_tpm appears to be undefined in
> your two patches, so it looks to be a real bug.
>
> Second, please read the C standard's section about reserved
> identifiers.
>
reworked to avoid all the complications.
cheers,
/vb
> 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: w...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
On Sat, Oct 15, 2011 at 12:08 PM, Mike Frysinger wrote:
> On Saturday 15 October 2011 14:02:29 Marek Vasut wrote:
>> On Saturday, October 15, 2011 05:39:08 AM Vadim Bendebury wrote:
>> > --- /dev/null
>> > +++ b/common/cmd_tpm.c
>> > @@ -0,0 +1,111 @@
>> > +/*
>> > + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
>> > + * Released under the 2-clause BSD license.
>>
>> Are we ok with this ? Also, you say something about GPL in the same
>> comment?
>
> there's nothing wrong with adding files under the BSD license. what is odd
> about this code though is that it says BSD on one line, and then it says
> GPL-2+ a few lines later. pick one or the other.
>
done
>> > + /*
>> > + * Verify that in case it is present, the first argument, it is
>> > + * exactly one character in size.
>> > + */
>> > + if (argc < 7) {
>> > + puts("command should be at least six bytes in size\n");
>> > + return ~0;
>>
>> Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
>
> ~0 is weird. this should be 1 or -1.
done
> -mike
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: CMD7:MMC_CMD_SELECT_CARD response fix
>> I need to think about this. Was this causing you a problem? What were
>> the symptoms?
>>
> yes... I am working on adding mmc support for Marvell GplugD board (Armada168
> SoC). So during mmc startup, MMC_CMD_SELECT_CARD(CMD7) gets timed out. So
> digging more I found out that CMD7 in driver was expecting R1b instead of R1.
> I referred SD specs and JEDEC document for confirmation, and it clearly said
> in its footnote (which I added to patch description) that from stand-by to
> transfer mode response will be R1. I even crosschecked mmc driver in Kernel
> code, where in function mmc_select_card; CMD7 response is expected to be R1.
>
> I also checked if the standard SDHCI driver I am using is at fault or what,
> But I did not find any problem with mmc driver (sdhci.c). I then referred to
> blackfin SDH driver to see how they are handing this... so they are kind of
> bypassing this. According to specs, for R1b one should wait for CMD_SENT and
> data end flag to get set as card may send busy response. whereas blackfin
> driver simply checks if any of the flag is set it comes out of loop see code
> below for reference.
>
> ref bfin_sdh.c
> [..]
> /* wait for a while */
> timeout = 0;
> do {
> if (++timeout > 100) {
> status = CMD_TIME_OUT;
> break;
> }
> udelay(1);
> status = bfin_read_SDH_STATUS();
> } while (!(status & (CMD_SENT | CMD_RESP_END | CMD_TIME_OUT |
> CMD_CRC_FAIL)));
>
> Incase of sdhci, driver waits for response endlessly. till both
> SDHCI_INT_RESPONSE and SDHCI_INT_DATA_END flags are set.
>
> I hope I am able to clear the issue?
Yes, that sounds fine, then. I'll apply it for this release.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] imx: imx31_phycore.h: fix checkpatch warnings
Hi Anatolij, On Sat, Oct 15, 2011 at 7:00 PM, Anatolij Gustschin wrote: > Cleanup board config file and fix issues reported by > checkpatch.pl script. > > Signed-off-by: Anatolij Gustschin > Cc: Stefano Babic > --- > include/configs/imx31_phycore.h | 112 > +++ > 1 files changed, 67 insertions(+), 45 deletions(-) > > diff --git a/include/configs/imx31_phycore.h b/include/configs/imx31_phycore.h > index 48bd50b..f8cc5ec 100644 > --- a/include/configs/imx31_phycore.h > +++ b/include/configs/imx31_phycore.h > @@ -30,7 +30,7 @@ > > #include > > - /* High Level Configuration Options */ > +/* High Level Configuration Options */ > #define CONFIG_ARM1136 1 /* This is an arm1136 CPU core */ > #define CONFIG_MX31 1 /* in a mx31 */ While you are at it, maybe it is also a good idea to change: #define ABCD 1 to #define ABCD (ie, remove the "1") that appears several times in this file. Regards, Fabio Estevam ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] imx: imx31_phycore.h: fix checkpatch warnings
Cleanup board config file and fix issues reported by checkpatch.pl script. Signed-off-by: Anatolij Gustschin Cc: Stefano Babic --- include/configs/imx31_phycore.h | 112 +++ 1 files changed, 67 insertions(+), 45 deletions(-) diff --git a/include/configs/imx31_phycore.h b/include/configs/imx31_phycore.h index 48bd50b..f8cc5ec 100644 --- a/include/configs/imx31_phycore.h +++ b/include/configs/imx31_phycore.h @@ -30,7 +30,7 @@ #include - /* High Level Configuration Options */ +/* High Level Configuration Options */ #define CONFIG_ARM1136 1/* This is an arm1136 CPU core */ #define CONFIG_MX311/* in a mx31 */ #define CONFIG_MX31_HCLK_FREQ 2600 @@ -86,26 +86,40 @@ #define CONFIG_BOOTDELAY 3 -#define MTDPARTS_DEFAULT "mtdparts=physmap-flash.0:128k(uboot)ro,1536k(kernel),-(root)" +#define MTDPARTS_DEFAULT "mtdparts=physmap-flash.0:128k(uboot)ro," \ + "1536k(kernel),-(root)" #define CONFIG_NETMASK 255.255.255.0 #define CONFIG_IPADDR 192.168.23.168 #define CONFIG_SERVERIP192.168.23.2 -#defineCONFIG_EXTRA_ENV_SETTINGS \ - "bootargs_base=setenv bootargs console=ttySMX0,115200\0" \ - "bootargs_nfs=setenv bootargs $(bootargs) root=/dev/nfs ip=dhcp nfsroot=$(serverip):$(nfsrootfs),v3,tcp\0" \ - "bootargs_flash=setenv bootargs $(bootargs) root=/dev/mtdblock2 rootfstype=jffs2" \ - "bootargs_mtd=setenv bootargs $(bootargs) $(mtdparts)" \ - "bootcmd=run bootcmd_net\0" \ - "bootcmd_net=run bootargs_base bootargs_mtd bootargs_nfs; tftpboot 0x8000 $(uimage); bootm\0" \ - "bootcmd_flash=run bootargs_base bootargs_mtd bootargs_flash; bootm 0x8000\0" \ - "unlock=yes\0" \ - "mtdparts=" MTDPARTS_DEFAULT "\0" \ - "prg_uboot=tftpboot 0x8000 $(uboot); protect off 0xa000 +0x2; erase 0xa000 +0x2; cp.b 0x8000 0xa000 $(filesize)\0" \ - "prg_kernel=tftpboot 0x8000 $(uimage); erase 0xa004 +0x18; cp.b 0x8000 0xa004 $(filesize)\0"\ - "prg_jffs2=tftpboot 0x8000 $(jffs2); erase 0xa01c 0xa1ff; cp.b 0x8000 0xa01c $(filesize)\0" \ - "videomode=video=ctfb:x:240,y:320,depth:16,mode:0,pclk:185925,le:9,ri:17,up:7,lo:10,hs:1,vs:1,sync:1241513985,vmode:0\0" +#defineCONFIG_EXTRA_ENV_SETTINGS \ + "bootargs_base=setenv bootargs console=ttySMX0,115200\0"\ + "bootargs_nfs=setenv bootargs $(bootargs) root=/dev/nfs " \ + "ip=dhcp nfsroot=$(serverip):$(nfsrootfs),v3,tcp\0" \ + "bootargs_flash=setenv bootargs $(bootargs) " \ + "root=/dev/mtdblock2 rootfstype=jffs2\0"\ + "bootargs_mtd=setenv bootargs $(bootargs) $(mtdparts)\0"\ + "bootcmd=run bootcmd_net\0" \ + "bootcmd_net=run bootargs_base bootargs_mtd bootargs_nfs;" \ + "tftpboot 0x8000 $(uimage);bootm\0" \ + "bootcmd_flash=run bootargs_base bootargs_mtd bootargs_flash;" \ + "bootm 0x8000\0"\ + "unlock=yes\0" \ + "mtdparts=" MTDPARTS_DEFAULT "\0" \ + "prg_uboot=tftpboot 0x8000 $(uboot);" \ + "protect off 0xa000 +0x2;" \ + "erase 0xa000 +0x2;"\ + "cp.b 0x8000 0xa000 $(filesize)\0" \ + "prg_kernel=tftpboot 0x8000 $(uimage);" \ + "erase 0xa004 +0x18;" \ + "cp.b 0x8000 0xa004 $(filesize)\0" \ + "prg_jffs2=tftpboot 0x8000 $(jffs2);" \ + "erase 0xa01c 0xa1ff;" \ + "cp.b 0x8000 0xa01c $(filesize)\0" \ + "videomode=video=ctfb:x:240,y:320,depth:16,mode:0," \ + "pclk:185925,le:9,ri:17,up:7,lo:10,hs:1,vs:1," \ + "sync:1241513985,vmode:0\0" #define CONFIG_SMC911X 1 @@ -117,11 +131,15 @@ */ #defi
Re: [U-Boot] [PATCH] STx AMC8548: initial support for Silicon Turnkey Express AMC8548 board
Dear Kumar, In message <1302584653-15312-1-git-send-email-oa...@yahoo.com> Alex Dubov wrote: > From: Alex Dubov > > AMC8548 is a RapidIO development board in AMC form factor, featuring MPC8548E > processor, DDR2 SO-DIMM slot, 16MB of hardwired NAND flash memory, real time > clock and additional serial EEPROM on i2c bus (enabled). USB controller is > available, but not presently enabled. > > Additional board information is available at: > http://www.silicontkx.com/AMC8548.htm > > Signed-off-by: Alex Dubov Can you please comment on the status of this patch? http://article.gmane.org/gmane.comp.boot-loaders.u-boot/97396 IDStateName --- 80612 New [U-Boot] STx AMC8548: initial support for Silicon Turnkey Express AMC8548 board 90713 New [U-Boot] STx AMC8548: initial support for Silicon Turnkey Express AMC8548 board 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: w...@denx.de Unsichtbar macht sich die Dummheit, indem sie immer größere Ausmaße annimmt. -- Bertold Brecht: Der Tui-Roman ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
> Dear Marek Vasut,
>
> thank you for your comments, please see below:
>
> On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut wrote:
> > On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
> >> TPM (Trusted Platform Module) is an integrated circuit and
> >> software platform that provides computer manufacturers with the
> >> core components of a subsystem used to assure authenticity,
> >> integrity and confidentiality.
> >
> > [...]
> >
> > Quick points:
> > * The license
>
> Please suggest the appropriate file header text.
Uh ... you should know the license !!!
>
> > * Use debug() when fitting
>
> It seems to me debug/puts/printf are used where appropriate - do you
> have some cases in mind which need to be changed?
Ok
>
> >> diff --git a/drivers/tpm/generic_lpc_tpm.c
> >> b/drivers/tpm/generic_lpc_tpm.c new file mode 100644
> >> index 000..8319286
> >> --- /dev/null
> >> +++ b/drivers/tpm/generic_lpc_tpm.c
> >
> > [...]
> >
> >> +
> >> +struct lpc_tpm {
> >> + struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
> >> +};
> >
> > Do you need such envelope ?
>
> I think I do - this accurately describes the structure of the chip.
There's just one member ... it's weird?
>
> >> +
> >> +static struct lpc_tpm *lpc_tpm_dev =
> >> + (struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS;
> >> +
> >> +/* Some registers' bit field definitions */
> >> +#define TIS_STS_VALID (1 << 7) /* 0x80 */
> >> +#define TIS_STS_COMMAND_READY (1 << 6) /* 0x40 */
> >> +#define TIS_STS_TPM_GO (1 << 5) /* 0x20 */
> >> +#define TIS_STS_DATA_AVAILABLE (1 << 4) /* 0x10 */
> >> +#define TIS_STS_EXPECT (1 << 3) /* 0x08 */
> >> +#define TIS_STS_RESPONSE_RETRY (1 << 1) /* 0x02 */
> >> +
> >> +#define TIS_ACCESS_TPM_REG_VALID_STS (1 << 7) /* 0x80 */
> >> +#define TIS_ACCESS_ACTIVE_LOCALITY (1 << 5) /* 0x20 */
> >> +#define TIS_ACCESS_BEEN_SEIZED (1 << 4) /* 0x10 */
> >> +#define TIS_ACCESS_SEIZE (1 << 3) /* 0x08 */
> >> +#define TIS_ACCESS_PENDING_REQUEST (1 << 2) /* 0x04 */
> >> +#define TIS_ACCESS_REQUEST_USE (1 << 1) /* 0x02 */
> >> +#define TIS_ACCESS_TPM_ESTABLISHMENT (1 << 0) /* 0x01 */
> >> +
> >> +#define TIS_STS_BURST_COUNT_MASK (0x)
> >> +#define TIS_STS_BURST_COUNT_SHIFT (8)
> >> +
> >> +/*
> >> + * Error value returned if a tpm register does not enter the expected
> >> state + * after continuous polling. No actual TPM register reading ever
> >> returns ~0, + * so this value is a safe error indication to be mixed
> >> with possible status + * register values.
> >> + */
> >> +#define TPM_TIMEOUT_ERR (~0)
> >> +
> >> +/* Error value returned on various TPM driver errors */
> >
> > Dot missing at the end.
>
> ok.
Please fix globally.
>
> >> +#define TPM_DRIVER_ERR (-1)
> >> +
> >> + /* 1 second is plenty for anything TPM does.*/
> >> +#define MAX_DELAY_US (1000 * 1000)
> >> +
> >> +/* Retrieve burst count value out of the status register contents. */
> >> +#define BURST_COUNT(status) ((u16)(((status) >>
> >> TIS_STS_BURST_COUNT_SHIFT) & \ +
> >> TIS_STS_BURST_COUNT_MASK))
> >
> > Do you need the cast ?
>
> I think it demonstrates the intentional truncation of the value, it
> gets assigned to u16 values down the road, prevents compiler warnings
> about assigning incompatible values in some cases.
Make it an inline function then, this will do the typechecking for you.
>
> >> +
> >> +/*
> >> + * Structures defined below allow creating descriptions of TPM
> >> vendor/device + * ID information for run time discovery. The only device
> >> the system knows + * about at this time is Infineon slb9635
> >> + */
> >> +struct device_name {
> >> + u16 dev_id;
> >> + const char * const dev_name;
> >> +};
> >> +
> >> +struct vendor_name {
> >> + u16 vendor_id;
> >> + const char *vendor_name;
> >> + const struct device_name *dev_names;
> >> +};
> >> +
> >> +static const struct device_name infineon_devices[] = {
> >> + {0xb, "SLB9635 TT 1.2"},
> >> + {0}
> >> +};
> >> +
> >> +static const struct vendor_name vendor_names[] = {
> >> + {0x15d1, "Infineon", infineon_devices},
> >> +};
> >> +
> >> +/*
> >> + * Cached vendor/device ID pair to indicate that the device has been
> >> already + * discovered
> >> + */
> >> +static u32 vendor_dev_id;
> >> +
> >> +/* TPM access going through macros to make tracing easier. */
> >> +#define tpm_read(ptr) ({ \
> >> + u32 __ret; \
> >> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
> >> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
> >> +(u32)ptr - (u32)lpc_tpm_dev, __ret); \
> >> + __ret; })
> >> +
> >
> > Make this inline function
> >
> >> +#define tpm_write(value, ptr) ({\
> >> + u32 __v = val
Re: [U-Boot] [PATCH 2/4] arm, davinci: Correct the MDSTAT.STATE mask
Hello. On 12-10-2011 15:31, Christian Riesch wrote: > MDSTAT.STATE occupies bits 0..5 according to all available documentation, > therefore change the bitmask to 0x3f. > Signed-off-by: Christian Riesch > Cc: Heiko Schocher > Cc: Paulraj Sandeep > Cc: Albert ARIBAUD > Cc: Sergei Shtylyov > --- > arch/arm/cpu/arm926ejs/davinci/psc.c |4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > diff --git a/arch/arm/cpu/arm926ejs/davinci/psc.c > b/arch/arm/cpu/arm926ejs/davinci/psc.c > index 8273a7f..486adb0 100644 > --- a/arch/arm/cpu/arm926ejs/davinci/psc.c > +++ b/arch/arm/cpu/arm926ejs/davinci/psc.c > @@ -83,7 +83,7 @@ void lpsc_on(unsigned int id) > while (readl(ptstat)& 0x01) > continue; > > - if ((readl(mdstat)& 0x1f) == 0x03) > + if ((readl(mdstat)& 0x3f) == 0x03) > return; /* Already on and enabled */ > > writel(readl(mdctl) | 0x03, mdctl); > @@ -114,7 +114,7 @@ void lpsc_on(unsigned int id) > > while (readl(ptstat)& 0x01) > continue; > - while ((readl(mdstat)& 0x1f) != 0x03) > + while ((readl(mdstat)& 0x3f) != 0x03) > continue; > } This patch is incomplete but I guess you've figured that out already as you've commented on my analogous patch. So marking for the custodian -- don't apply this patch. WBR, Sergei ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2] Ethernut 5 board support
Dear Albert, do you have any idea what's the status of this patch: 03/14 Harald Kipp[U-Boot] [PATCH V2] Ethernut 5 board support http://article.gmane.org/gmane.comp.boot-loaders.u-boot/95743 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: w...@denx.de "Probably the best operating system in the world is the [operating system] made for the PDP-11 by Bell Laboratories." - Ted Nelson, October 1977 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
On Sat, Oct 15, 2011 at 1:01 PM, Mike Frysinger wrote: > On Saturday 15 October 2011 15:44:04 Wolfgang Denk wrote: >> Vadim Bendebury wrote: >> > > Two underscores aren't a good practice. >> > >> > I did this as a result of a previous review. Do you have a suggestion >> > how this should be done instead? >> >> First, and most important, __u_boot_cmd_tpm appears to be undefined in >> your two patches, so it looks to be a real bug. >> >> Second, please read the C standard's section about reserved >> identifiers. > > no, this is coming from common u-boot code. look at > include/command.h:U_BOOT_CMD defines. > -mike > or, more importantly: the question is what is the right/suggested way to print out the help message associated with a U_BOOT_CMD declaration? cheers, /vb ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
On Sat, Oct 15, 2011 at 12:42 PM, Mike Frysinger wrote:
> On Friday 14 October 2011 23:38:50 Vadim Bendebury wrote:
>> --- /dev/null
>> +++ b/drivers/tpm/generic_lpc_tpm.c
>>
>> +#define TPM_TIMEOUT_ERR (~0)
>> +#define TPM_DRIVER_ERR (-1)
>
> these are the same thing. another reason why you shouldn't mix ~ with normal
> values. use -2 or something.
>
ok
>> +/* TPM access going through macros to make tracing easier. */
>> +#define tpm_read(ptr) ({ \
>> + u32 __ret; \
>> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
>> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
>> + (u32)ptr - (u32)lpc_tpm_dev, __ret); \
>> + __ret; })
>
> that last "__ret;" is indented way too far. it should be on the same level as
> "u32 __ret;" and such.
>
ok
>> +#define tpm_write(value, ptr) ({ \
>> + u32 __v = value; \
>> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \
>> + (u32)ptr - (u32)lpc_tpm_dev, __v); \
>> + if (sizeof(*ptr) == 1) \
>> + writeb(__v, ptr); \
>> + else \
>> + writel(__v, ptr); })
>
> ({...}) doesn't make sense here. this should be a do{...}while(0).
>
ok
>> + printf("%s:%d - failed to get 'command_ready' status\n",
>> + __FILE__, __LINE__);
>
> replace __FILE__/__LINE__ with __func__ here and everywhere else in the file
> -mike
>
Mike, you seem the only one concerned with this. As I described in our
previous exchange, I believe specifying file and line number is
better. So, I would like to hear a second opinion on this.
cheers,
/vb
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/3] mkimage: adding support for Davinci AIS image
Dear Stefano Babic, In message <1317889214-10567-1-git-send-email-sba...@denx.de> you wrote: > Some Davinci processors supports the Application > Image Script (AIS) boot process. The patch adds the generation > of the AIS image inside the mkimage tool to make possible > to generate a bootable U-boot without external tools > (TI Davinci AIS Generator). > > Signed-off-by: Stefano Babic > CC: Wolfgang Denk > > --- > > Changes: > - removed warning in gcc 4.6 iwhen -Wunused-but-set-variable is set > - drop remained warnings raised by checkpatch > > common/image.c |1 + > include/image.h |1 + > tools/Makefile |4 +- > tools/aisimage.c | 451 > ++ > tools/aisimage.h | 97 > tools/mkimage.c |2 + > tools/mkimage.h |1 + > 7 files changed, 556 insertions(+), 1 deletions(-) > create mode 100644 tools/aisimage.c > create mode 100644 tools/aisimage.h This doesn't apply any more - please rebase, and keep list sorted. Thanks. 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: w...@denx.de Felson's Law: To steal ideas from one person is plagiarism; to steal from many is research. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] serial_exit: punt unused prototype
Dear Mike Frysinger, In message <1317690674-15608-1-git-send-email-vap...@gentoo.org> you wrote: > No code defines or calls this, so drop the prototype. > > Signed-off-by: Mike Frysinger > --- > include/common.h |1 - > 1 files changed, 0 insertions(+), 1 deletions(-) Applied, thanks. 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: w...@denx.de 2000 pounds of chinese soup = 1 Won Ton ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] punt unused clean/distclean targets
Dear Mike Frysinger, In message <1318524897-19001-1-git-send-email-vap...@gentoo.org> you wrote: > The top level Makefile does not do any recursion into subdirs when > cleaning, so these clean/distclean targets in random arch/board dirs > never get used. Punt them all. > > MAKEALL didn't report any errors related to this that I could see. > > Signed-off-by: Mike Frysinger > --- > v2 > - add a few more makefiles since last patch Applied, thanks. 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: w...@denx.de Romulan women are not like Vulcan females. We are not dedicated to pure logic and the sterility of non-emotion. -- Romulan Commander, "The Enterprise Incident", stardate 5027.3 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/3] net: emaclite: Fix coding style
Dear Michal Simek, In message <1317974226-25252-3-git-send-email-mon...@monstr.eu> you wrote: > Checked by checkpatch.pl script. > No functional changes. > > Signed-off-by: Michal Simek > --- > drivers/net/xilinx_emaclite.c | 57 +++- > 1 files changed, 27 insertions(+), 30 deletions(-) Applied, thanks. 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: w...@denx.de Machines take me by surprise with great frequency. - Alan Turing ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] common: fix missing function pointer relocation in fixup_cmdtable()
Dear Daniel Schwierzeck, In message <1318522059-16182-1-git-send-email-daniel.schwierz...@googlemail.com> you wrote: > The command auto-completion does not work on architectures relying > on CONFIG_NEEDS_MANUAL_RELOC like MIPS. Cause is the missing function > pointer fixup for cmd_tbl_t::complete function in fixup_cmdtable(). > > This patch adds the missing pointer fixup in case of CONFIG_AUTO_COMPLETE > is defined. > > Signed-off-by: Daniel Schwierzeck > --- > common/command.c |6 ++ > 1 files changed, 6 insertions(+), 0 deletions(-) Applied, thanks. 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: w...@denx.de "It is easier to port a shell than a shell script." - Larry Wall ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mpc5200: digsy_mtc: fix detection of Coral-PA
Dear Anatolij Gustschin, In message <1318519157-4945-1-git-send-email-ag...@denx.de> you wrote: > A delay of approximately 250 ms after PCI bus reset in > pci_mpc5xxx_init() is needed to recognize the Coral-PA > controller on the graphic extention board. > > Signed-off-by: Anatolij Gustschin > --- > To make it actually work another patch > "pci: move pcidelay code to new location just before PCI bus scan" > is also required. It has been submitted earlier [1] and there is > already some positive feedback. > > [1] http://patchwork.ozlabs.org/patch/119171/ > > include/configs/digsy_mtc.h |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) Applied, thanks. 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: w...@denx.de "Plan to throw one away. You will anyway." - Fred Brooks, "The Mythical Man Month" ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/1] pci: move pcidelay code to new location just before PCI bus scan
Dear Anatolij Gustschin, In message <1318409070-11792-1-git-send-email-ag...@denx.de> you wrote: > PCI cards might need some time after reset to respond. On some > boards (mpc5200 or mpc8260 based) the PCI bus reset is deasserted > at pci_init_board() time, so we currently can not use available > "pcidelay" option for waiting before PCI bus scan since this > waiting takes place before calling pci_init_board(). By moving > the pcidelay code to the new location using of the "pcidelay" > option is possible on mpc5200 or mpc8260 based boards, too. > > Since pci_hose_scan() could be called multiple times, restrict > the function to wait only during its first call and to ignore > pcidelay for any further call (as pointed out by Matthias). > > Signed-off-by: Anatolij Gustschin > Cc: Matthias Fuchs > --- > Changes since first version: > - extend to wait only during initial pci_hose_scan() call >as pointed out by Matthias > > drivers/pci/pci.c | 30 +- > 1 files changed, 17 insertions(+), 13 deletions(-) Applied, thanks. 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: w...@denx.de A Freudian slip is when you say one thing but mean your mother. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4] net: axi_ethernet: Add driver to u-boot
Dear Michal Simek, In message <1317969335-21798-1-git-send-email-mon...@monstr.eu> you wrote: > Add axi_ethernet driver for little-endian Microblaze. > > RX/TX BDs and rxframe buffer are shared among all axi_ethernet MACs. > Only one MAC can work in one time. > > Signed-off-by: Michal Simek > > --- > > v2: Fix cammelcase weirdness > Create mdio_wait function with timeouts > Synchronize debug messages > Remove base address + offset notation -> use struct instead > > v3: Fix comments > Add timeouts/delays to waiting loops > Use in/out_be32 for dma accesses > Setup return values for phy functions > > v4: Fix merge confict - no functional change > --- > drivers/net/Makefile |1 + > drivers/net/xilinx_axi_emac.c | 664 > + > include/netdev.h |2 + > 3 files changed, 667 insertions(+), 0 deletions(-) > create mode 100644 drivers/net/xilinx_axi_emac.c Applied, thanks. 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: w...@denx.de He'd been wrong, there _was_ a light at the end of the tunnel, and it was a flamethrower. - Terry Pratchett, _Mort_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] m68k: fix ambiguous bit testing
Building for some m68k boards results in the warning: cpu_init.c: In function 'cpu_init_f': cpu_init.c:287: warning: suggest parentheses around operand of '!' or change '&' to '&&' or '!' to '~' Signed-off-by: Mike Frysinger --- arch/m68k/cpu/mcf52x2/cpu_init.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/arch/m68k/cpu/mcf52x2/cpu_init.c b/arch/m68k/cpu/mcf52x2/cpu_init.c index 170bbfc..a98a926 100644 --- a/arch/m68k/cpu/mcf52x2/cpu_init.c +++ b/arch/m68k/cpu/mcf52x2/cpu_init.c @@ -284,7 +284,7 @@ void cpu_init_f(void) mbar_writeLong(MCF_FMPLL_SYNCR, MCF_FMPLL_SYNCR_MFD(0) | MCF_FMPLL_SYNCR_RFD(0)); #endif - while (!mbar_readByte(MCF_FMPLL_SYNSR) & MCF_FMPLL_SYNSR_LOCK) ; + while (!(mbar_readByte(MCF_FMPLL_SYNSR) & MCF_FMPLL_SYNSR_LOCK)) ; } /* -- 1.7.6.1 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/3] net: emaclite: Fix coding style
Dear Michal Simek, In message <1317974226-25252-3-git-send-email-mon...@monstr.eu> you wrote: > Checked by checkpatch.pl script. > No functional changes. Note : this should go into the comment section (pwclient picked the original submit anyway). 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: w...@denx.de There is a time in the tides of men, Which, taken at its flood, leads on to success. On the other hand, don't count on it. - T. K. Lawson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] net: emaclite: Use PKTSIZE directly
Dear Michal Simek, In message <1317974226-25252-2-git-send-email-mon...@monstr.eu> you wrote: > Do not setup additional ENET_MAX_MTU macro. > > Signed-off-by: Michal Simek > --- > drivers/net/xilinx_emaclite.c | 10 -- > 1 files changed, 4 insertions(+), 6 deletions(-) Applied, thanks. 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: w...@denx.de Perl already has an IDE. It's called Unix. -- Tom Christiansen in 375bd...@cs.colorado.edu ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3 v2] net: emaclite: Setup RX/TX ping pong for every instance
Dear Michal Simek, In message <1317974226-25252-1-git-send-email-mon...@monstr.eu> you wrote: > Setup RX/TX ping-pong buffer for every emaclite IP separately. > The next patch move initialization directly to board code. > > Signed-off-by: Michal Simek > > --- > v2: Fix coding style violations > --- > drivers/net/xilinx_emaclite.c | 123 ++-- > 1 files changed, 68 insertions(+), 55 deletions(-) Applied, thanks. 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: w...@denx.de Romulan women are not like Vulcan females. We are not dedicated to pure logic and the sterility of non-emotion. -- Romulan Commander, "The Enterprise Incident", stardate 5027.3 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6 v2] ColdFire: Cleanup lds files for multiple defined symbols
Acked-by: Mike Frysinger -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Pull request: u-boot-fdt
Dear Jerry Van Baren, In message <4e99a789.8090...@cideas.com> you wrote: > The following changes since commit d8fffa057c9430fd0c5104ab6ff7db4cdb03db51: > >Merge branch 'master' of git://git.denx.de/u-boot-mips (2011-10-12 > 22:47:15 +0200) > > are available in the git repository at: > >git://git.denx.de/u-boot-fdt.git master > > Chunhe Lan (1): >fdt: Add a do_fixup_by_path_string() function > > Timur Tabi (3): >fdt: check for fdt errors in fdt_create_phandle >fdt: update fdt_alloc_phandle to use fdt_get_phandle >powerpc/85xx: use fdt_create_phandle() to create the Fman > firmware phandles > > arch/powerpc/cpu/mpc85xx/fdt.c |5 ++--- > common/fdt_support.c | 18 +++--- > include/fdt_support.h |9 - > 3 files changed, 21 insertions(+), 11 deletions(-) Applied, thanks. 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: w...@denx.de The price of curiosity is a terminal experience. - Terry Pratchett, _The Dark Side of the Sun_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
On Saturday 15 October 2011 15:44:04 Wolfgang Denk wrote: > Vadim Bendebury wrote: > > > Two underscores aren't a good practice. > > > > I did this as a result of a previous review. Do you have a suggestion > > how this should be done instead? > > First, and most important, __u_boot_cmd_tpm appears to be undefined in > your two patches, so it looks to be a real bug. > > Second, please read the C standard's section about reserved > identifiers. no, this is coming from common u-boot code. look at include/command.h:U_BOOT_CMD defines. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [GIT PULL] Pull request u-boot-85xx.git
Dear Kumar Gala, In message you wrote: > The following changes since commit d8fffa057c9430fd0c5104ab6ff7db4cdb03db51: > > Merge branch 'master' of git://git.denx.de/u-boot-mips (2011-10-12 22:47:15 > +0200) > > are available in the git repository at: > > git://git.denx.de/u-boot-mpc85xx master > > Haiying Wang (3): > powerpc/p5020: fixup portal config info > powerpc/p2041: fixup portal config info > powerpc/p3041: fixup portal config info > > Kuldip Giroh (1): > powerpc/85xx: Added secure boot option for P2041RDB boards > > Kumar Gala (1): > powerpc/85xx: Drop CONFIG_VIDEO on P1_P2_RDB-PC boards to reduce size > > Timur Tabi (2): > powerpc/p3060: remove all references to RCW bits EC1_EXT, EC2_EXT, and > EC3 > phylib: wait for TN2020 to achieve SERDES lane alignment at startup > > chenhui zhao (2): > powerpc/mpc8548cds: Code cleanup and refactoring > powerpc/mpc8548cds: Add 36-bit support > > arch/powerpc/cpu/mpc85xx/p2041_ids.c| 20 +++--- > arch/powerpc/cpu/mpc85xx/p3041_ids.c| 20 +++--- > arch/powerpc/cpu/mpc85xx/p3060_serdes.c | 20 - > arch/powerpc/cpu/mpc85xx/p5020_ids.c| 20 +++--- > arch/powerpc/include/asm/immap_85xx.h | 10 --- > board/freescale/mpc8548cds/law.c| 31 +--- > board/freescale/mpc8548cds/mpc8548cds.c |8 +- > board/freescale/mpc8548cds/tlb.c| 64 > boards.cfg |2 + > drivers/net/fm/p3060.c | 17 > drivers/net/phy/teranetics.c| 33 > include/configs/MPC8548CDS.h| 125 > +++ > include/configs/p1_p2_rdb_pc.h | 16 > 13 files changed, 196 insertions(+), 190 deletions(-) Applied, thanks. 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: w...@denx.de Es ist offensichtlich, dass das menschliche Gehirn wie ein Computer funktioniert. Da es keine dummen Computer gibt, gibt es also auch keine dummen Menschen. Nur ein paar Leute, die unter DOS laufen. -- ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] Pull request: u-boot-video/master
Hello Wolfgang, The following changes since commit d8fffa057c9430fd0c5104ab6ff7db4cdb03db51: Merge branch 'master' of git://git.denx.de/u-boot-mips (2011-10-12 22:47:15 +0200) are available in the git repository at: git://git.denx.de/u-boot-video.git master Helmut Raiger (2): mx31: make HSP clock for mx3fb driver available video: Moving mx3fb.c to CONFIG_VIDEO Marek Vasut (1): MX5: Make IPU display output and pixel format configurable Stefano Babic (2): VIDEO: MX5: Switch MX5 to CONFIG_VIDEO VIDEO: MX5: export pix format Timur Tabi (1): video: update the Freescale DIU driver to use linux/fb.h arch/arm/cpu/arm1136/mx31/generic.c | 40 ++- arch/arm/include/asm/arch-mx31/clock.h|1 + arch/arm/include/asm/arch-mx31/imx-regs.h | 14 + board/ttcontrol/vision2/vision2.c | 40 ++- drivers/video/Makefile|2 +- drivers/video/cfb_console.c |7 + drivers/video/fsl_diu_fb.c| 54 +--- drivers/video/ipu.h | 46 +--- drivers/video/mx3fb.c | 459 + drivers/video/mxc_ipuv3_fb.c | 119 +++- include/configs/imx31_phycore.h | 24 +- include/configs/qong.h| 14 +- include/configs/vision2.h | 11 +- include/ipu_pixfmt.h | 81 + 14 files changed, 497 insertions(+), 415 deletions(-) create mode 100644 include/ipu_pixfmt.h Please pull. Thanks! Anatolij ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
Dear Vadim Bendebury,
In message
you wrote:
>
> >> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
> >> + * Released under the 2-clause BSD license.
> >
> > Are we ok with this ? Also, you say something about GPL in the same comment?
> >
>
> Can someone please tell me what needs to be put in the license headers
> and I will do it. I hear different suggestions from different people.
See previous comment - drop the BSD part if you include a GPLv2+
license header.
> >> + return ~0;
> >
> > Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
>
> I was under impression that any nonzero value is good. I see sometimes
> -1 returned for error in other u-boot sources. Also, I am sorry, I am
> new to this, when someone says "it is weird" - does this mean that it
> has to be changed?
Commands are running in some sort of shell environment. SO please
return 0 for OK and 1 for general errors like all other commands do
(or should do).
...
> >> +static void report_error(const char *msg)
> >> +{
> >> + if (msg && *msg)
> >
> > Uhm, you also check if first character is non-zero? why ?
>
> To avoid printing an empty string if someone calls this with an empty message?
It's your code, so just don't do it, then.
And what's wrong about printing an empty string? YOuy're just adding
dead code (and increased memory footprint) here.
> > Two underscores aren't a good practice.
>
> I did this as a result of a previous review. Do you have a suggestion
> how this should be done instead?
First, and most important, __u_boot_cmd_tpm appears to be undefined in
your two patches, so it looks to be a real bug.
Second, please read the C standard's section about reserved
identifiers.
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: w...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
On Friday 14 October 2011 23:38:50 Vadim Bendebury wrote:
> --- /dev/null
> +++ b/drivers/tpm/generic_lpc_tpm.c
>
> +#define TPM_TIMEOUT_ERR (~0)
> +#define TPM_DRIVER_ERR (-1)
these are the same thing. another reason why you shouldn't mix ~ with normal
values. use -2 or something.
> +/* TPM access going through macros to make tracing easier. */
> +#define tpm_read(ptr) ({ \
> + u32 __ret; \
> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
> +(u32)ptr - (u32)lpc_tpm_dev, __ret); \
> + __ret; })
that last "__ret;" is indented way too far. it should be on the same level as
"u32 __ret;" and such.
> +#define tpm_write(value, ptr) ({\
> + u32 __v = value;\
> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \
> +(u32)ptr - (u32)lpc_tpm_dev, __v); \
> + if (sizeof(*ptr) == 1) \
> + writeb(__v, ptr); \
> + else \
> + writel(__v, ptr); })
({...}) doesn't make sense here. this should be a do{...}while(0).
> + printf("%s:%d - failed to get 'command_ready' status\n",
> +__FILE__, __LINE__);
replace __FILE__/__LINE__ with __func__ here and everywhere else in the file
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
Dear Vadim Bendebury,
In message <20111015033850.74ad541...@eskimo.mtv.corp.google.com> you wrote:
> TPM (Trusted Platform Module) is an integrated circuit and
> software platform that provides computer manufacturers with the
> core components of a subsystem used to assure authenticity,
> integrity and confidentiality.
...
> --- /dev/null
> +++ b/drivers/tpm/Makefile
> @@ -0,0 +1,27 @@
> +# Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
> +# Use of this source code is governed by a BSD-style license that can be
> +# found in the LICENSE file.
Please fix this. This has been pointed out before. Please work a
bit more careful. Thanks.
...
> +++ b/drivers/tpm/generic_lpc_tpm.c
> @@ -0,0 +1,488 @@
> +/*
> + * Copyright (c) 2011 The Chromium OS Authors.
> + * Released under the 2-clause BSD license.
...
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
If we can have it under GPLv2+, then we don't need the BSD part.
Please drop these lines.
...
> +/* TPM access going through macros to make tracing easier. */
> +#define tpm_read(ptr) ({ \
> + u32 __ret; \
> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
> +(u32)ptr - (u32)lpc_tpm_dev, __ret); \
> + __ret; })
Please test with something like this instead:
debug("%sRead reg 0x%x returns 0x%x\n", PREFIX, ...
It might result in smaller code. Please verify. If so, please fix
globally.
...
> + vid = didvid & 0x;
> + did = (didvid >> 16) & 0x;
> + for (i = 0; i < ARRAY_SIZE(vendor_names); i++) {
> + int j = 0;
> + u16 known_did;
> + if (vid == vendor_names[i].vendor_id)
> + vendor_name = vendor_names[i].vendor_name;
Please separate declarations and code by a blank line.
> + burst = BURST_COUNT(value);
> + if ((offset == (len - 1)) && burst)
> + /*
> + * We need to be able to send the last byte to the
> + * device, so burst size must be nonzero before we
> + * break out.
> + */
> + break;
We require braces around multiline statements.
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: w...@denx.de
"Deliver yesterday, code today, think tomorrow."
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
Dear Simon Glass, In message you wrote: > > Another thought...the problem is that every character we receive we > echo, so we can of course never get ahead of the workload. But we can Even worse, if your input contains any commands that prodce output, mayeb even lots of output (say "md $addr 1") then even your increased buffer size is not going to help you. > way until there is a let-up. Even worse if one of the commands outputs > something, since then we might be 20 or 30 characters behind, and > start dropping things. Even if no commands output anything, delays and > the U-Boot prompt will create a problem. Indeed. And this is why the simple rule applies: always wait for the next command prompt before sending the next line of input. 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: w...@denx.de To understand a program you must become both the machine and the program. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
On Friday 14 October 2011 23:39:08 Vadim Bendebury wrote: > --- a/common/Makefile > +++ b/common/Makefile > > COBJS-$(CONFIG_CMD_UBIFS) += cmd_ubifs.o > COBJS-$(CONFIG_CMD_UNIVERSE) += cmd_universe.o > COBJS-$(CONFIG_CMD_UNZIP) += cmd_unzip.o > +COBJS-$(CONFIG_CMD_TPM) += cmd_tpm.o keep the list sorted -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
On Friday 14 October 2011 23:39:08 Vadim Bendebury wrote:
> --- /dev/null
> +++ b/common/cmd_tpm.c
>
> + /*
> + * Verify that in case it is present, the first argument, it is
> + * exactly one character in size.
> + */
> + if (argc < 7) {
> + puts("command should be at least six bytes in size\n");
> + return ~0;
> + }
> ...
> +U_BOOT_CMD(tpm, MAX_TRANSACTION_SIZE, 1, do_tpm,
> +"tpm [] - write data and read ressponse\n",
the usage information does not convey that you have to do:
tpm 1 2 3 4 5 6 7 8 9
perhaps says " [ ...]" instead ? and note that you have to
specify at least 6 ?
also, there's a typo: ressponse -> response
> +static void report_error(const char *msg)
> +{
> + if (msg && *msg)
> + printf("%s\n", msg);
> + cmd_usage(&__u_boot_cmd_tpm);
> +}
this gets used in one place, and the one place where it does get used, i don't
see a point in calling cmd_usage(). just have the one place where this is
used call puts() instead.
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] NS16550: buffer reads
Dear Simon Glass,
In message <1318694632-21872-1-git-send-email-...@chromium.org> you wrote:
> From: Scott Wood
>
> From: Scott Wood
>
> This improves the performance of U-Boot when accepting rapid input,
> such as pasting a sequence of commands.
>
> Without this patch, on P4080DS I see a maximum of around 5 lines can
> be pasted. With this patch, it handles around 70 lines before lossage,
> long enough for most things you'd paste.
>
> With it enabled, on ppc it's an extra 396 bytes of image size, and 1056
> bytes of BSS.
>
> ARM note from Simon Glass - ARM code size goes from
> 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
>
> Signed-off-by: Scott Wood
> ---
> Changes in v2:
> - Fix checkpatch warnings, the other one was already there
>
> Changes in v3:
> - Use CONFIG_NS16550_BUFFER_READS to set the buffer size also
>
> README | 12 ++
> drivers/serial/ns16550.c | 96 +++--
> drivers/serial/serial.c |5 +-
> include/ns16550.h|4 +-
> 4 files changed, 108 insertions(+), 9 deletions(-)
>
> diff --git a/README b/README
> index 7e032a9..1150d6c 100644
> --- a/README
> +++ b/README
> @@ -2862,6 +2862,18 @@ use the "saveenv" command to store a valid environment.
> space for already greatly restricted images, including but not
> limited to NAND_SPL configurations.
>
> +- CONFIG_NS16550_BUFFER_READS:
> + Instead of reading directly from the receive register
> + every time U-Boot is ready for another byte, keep a
> + buffer and fill it from the hardware fifo every time
> + U-Boot reads a character. This helps U-Boot keep up with
> + a larger amount of rapid input, such as happens when
> + pasting text into the terminal.
> +
> + To use this option, define CONFIG_NS16550_BUFFER_READS to
> + the size of the buffer you want (256 is a reasonable value).
If we should accept that patch, then please chose a better name here.
For example, CONFIG_NS16550_RBUF_SIZE (or similar) seems to be a
somewhat better choice.
> +#ifdef CONFIG_NS16550_BUFFER_READS
> +
> +#define BUF_SIZE CONFIG_NS16550_BUFFER_READS
> +#define NUM_PORTS 4
> +
> +struct ns16550_priv {
> + char buf[BUF_SIZE];
> + unsigned int head, tail;
> +};
There is little sense in adding another variable here. Just write:
char buf[CONFIG_NS16550_BUFFER_READS];
(or whatever variable name would be used here).
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: w...@denx.de
"The release of emotion is what keeps us health. Emotionally
healthy."
"That may be, Doctor. However, I have noted that the healthy release
of emotion is frequently unhealthy for those closest to you."
-- McCoy and Spock, "Plato's Stepchildren", stardate 5784.3
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
On Saturday 15 October 2011 14:02:29 Marek Vasut wrote:
> On Saturday, October 15, 2011 05:39:08 AM Vadim Bendebury wrote:
> > --- /dev/null
> > +++ b/common/cmd_tpm.c
> > @@ -0,0 +1,111 @@
> > +/*
> > + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
> > + * Released under the 2-clause BSD license.
>
> Are we ok with this ? Also, you say something about GPL in the same
> comment?
there's nothing wrong with adding files under the BSD license. what is odd
about this code though is that it says BSD on one line, and then it says
GPL-2+ a few lines later. pick one or the other.
> > + /*
> > +* Verify that in case it is present, the first argument, it is
> > +* exactly one character in size.
> > +*/
> > + if (argc < 7) {
> > + puts("command should be at least six bytes in size\n");
> > + return ~0;
>
> Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
~0 is weird. this should be 1 or -1.
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
Dear Simon Glass, In message you wrote: > > > How about using the time-honored flow-control mechanism? If the serial port > > supports hardware CTS and RTS signals, you can use them to sync with the > > sender. If not, you can send XOFF and XON to respectively ask the sender to > > pause and resume sending. Of course, the sender must support hardware or > > software flow control. > > It is common to have just a 2-wire UART for the console. Perhaps this > is unfortunate. Indeed that's a pretty common configuration - but soft-flow would still work fine. > Are you suggesting that U-Boot should send XOFF sometimes, or does it > support this already? (I haven't seen it). When would U-Boot send > XOFF? Perhaps it could send XOFF at the end of each line that it > receives. Then XON when it starts reading the next line or maybe when > it runs out of characters and wants more. It should be sufficient to send XOFF when receiving a '\n' character (and storing that state) and XON when expecting inout (if state says XOFF had been sent before). > Most terminal problems will support software flow control. But are you > sure this is better? Definitely. for example, because it is hardware independent and will work on all architectures, all SoCs, all UARTs. 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: w...@denx.de "We don't care. We don't have to. We're the Phone Company." ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
Dear Simon Glass, In message you wrote: > > The situation this occurs is when you paste characters into a serial > terminal connected to U-Boot. This is a pretty common requirement. Yes > you can manually select each line and paste it but that is a pain. But that's exactly how it's supposed to work. Depending on hardware that may or may not be FIFOs available to buffer such asynchronous input, and I rather see limited, but consistent behaviour, instead of fancy stuff that works oin one board but not on another. > With this patch it is possible to paste a series of 'setenv' commands, > for example. If you want to run multi-line command sequences, then load and run a script. In the special case of setting environment variables, consider using featurs like "env import" instead. > This caused an awful lot of confusion here because characters would be > silently dropped within the command line (normally about 16 chars from > the end). Then the system would not boot. It is really not a friendly > experience. There should never be character loss when sending data on a line by line base. Even the slowest processors we support are fast enough to deal with that. Multi-line input has never been supported. 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: w...@denx.de Alan Turing thought about criteria to settle the question of whether machines can think, a question of which we now know that it is about as relevant as the question of whether submarines can swim. -- Edsger Dijkstra ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut wrote:
> On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
>> TPM (Trusted Platform Module) is an integrated circuit and
>> software platform that provides computer manufacturers with the
>> core components of a subsystem used to assure authenticity,
>> integrity and confidentiality.
>
> [...]
>
> Quick points:
> * The license
Please suggest the appropriate file header text.
> * Use debug() when fitting
>
It seems to me debug/puts/printf are used where appropriate - do you
have some cases in mind which need to be changed?
>> diff --git a/drivers/tpm/generic_lpc_tpm.c b/drivers/tpm/generic_lpc_tpm.c
>> new file mode 100644
>> index 000..8319286
>> --- /dev/null
>> +++ b/drivers/tpm/generic_lpc_tpm.c
>
> [...]
>
>> +
>> +struct lpc_tpm {
>> + struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
>> +};
>
> Do you need such envelope ?
>
I think I do - this accurately describes the structure of the chip.
>> +
>> +static struct lpc_tpm *lpc_tpm_dev =
>> + (struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS;
>> +
>> +/* Some registers' bit field definitions */
>> +#define TIS_STS_VALID (1 << 7) /* 0x80 */
>> +#define TIS_STS_COMMAND_READY (1 << 6) /* 0x40 */
>> +#define TIS_STS_TPM_GO (1 << 5) /* 0x20 */
>> +#define TIS_STS_DATA_AVAILABLE (1 << 4) /* 0x10 */
>> +#define TIS_STS_EXPECT (1 << 3) /* 0x08 */
>> +#define TIS_STS_RESPONSE_RETRY (1 << 1) /* 0x02 */
>> +
>> +#define TIS_ACCESS_TPM_REG_VALID_STS (1 << 7) /* 0x80 */
>> +#define TIS_ACCESS_ACTIVE_LOCALITY (1 << 5) /* 0x20 */
>> +#define TIS_ACCESS_BEEN_SEIZED (1 << 4) /* 0x10 */
>> +#define TIS_ACCESS_SEIZE (1 << 3) /* 0x08 */
>> +#define TIS_ACCESS_PENDING_REQUEST (1 << 2) /* 0x04 */
>> +#define TIS_ACCESS_REQUEST_USE (1 << 1) /* 0x02 */
>> +#define TIS_ACCESS_TPM_ESTABLISHMENT (1 << 0) /* 0x01 */
>> +
>> +#define TIS_STS_BURST_COUNT_MASK (0x)
>> +#define TIS_STS_BURST_COUNT_SHIFT (8)
>> +
>> +/*
>> + * Error value returned if a tpm register does not enter the expected
>> state + * after continuous polling. No actual TPM register reading ever
>> returns ~0, + * so this value is a safe error indication to be mixed with
>> possible status + * register values.
>> + */
>> +#define TPM_TIMEOUT_ERR (~0)
>> +
>> +/* Error value returned on various TPM driver errors */
>
> Dot missing at the end.
>
ok.
>> +#define TPM_DRIVER_ERR (-1)
>> +
>> + /* 1 second is plenty for anything TPM does.*/
>> +#define MAX_DELAY_US (1000 * 1000)
>> +
>> +/* Retrieve burst count value out of the status register contents. */
>> +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT)
>> & \ + TIS_STS_BURST_COUNT_MASK))
>
> Do you need the cast ?
>
I think it demonstrates the intentional truncation of the value, it
gets assigned to u16 values down the road, prevents compiler warnings
about assigning incompatible values in some cases.
>> +
>> +/*
>> + * Structures defined below allow creating descriptions of TPM
>> vendor/device + * ID information for run time discovery. The only device
>> the system knows + * about at this time is Infineon slb9635
>> + */
>> +struct device_name {
>> + u16 dev_id;
>> + const char * const dev_name;
>> +};
>> +
>> +struct vendor_name {
>> + u16 vendor_id;
>> + const char *vendor_name;
>> + const struct device_name *dev_names;
>> +};
>> +
>> +static const struct device_name infineon_devices[] = {
>> + {0xb, "SLB9635 TT 1.2"},
>> + {0}
>> +};
>> +
>> +static const struct vendor_name vendor_names[] = {
>> + {0x15d1, "Infineon", infineon_devices},
>> +};
>> +
>> +/*
>> + * Cached vendor/device ID pair to indicate that the device has been
>> already + * discovered
>> + */
>> +static u32 vendor_dev_id;
>> +
>> +/* TPM access going through macros to make tracing easier. */
>> +#define tpm_read(ptr) ({ \
>> + u32 __ret; \
>> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
>> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
>> + (u32)ptr - (u32)lpc_tpm_dev, __ret); \
>> + __ret; })
>> +
>
> Make this inline function
>
>> +#define tpm_write(value, ptr) ({ \
>> + u32 __v = value; \
>> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \
>> + (u32)ptr - (u32)lpc_tpm_dev, __v); \
>> + if (sizeof(*ptr) == 1) \
>> + writeb(__v, ptr); \
>> + else \
>> + writel(__v, ptr); })
>> +
>
> DTTO
>
Are you sure these will work as inline functions?
> [...]
>
>> +static u32 tis_senddata(const u8 * const data, u32 len)
>> +{
>> + u32 offset = 0;
>> + u16 burst = 0;
>> + u32 max_cycles = 0;
>> + u8 locality = 0;
>> + u32 value;
>> +
>> +
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:02 AM, Marek Vasut wrote:
> On Saturday, October 15, 2011 05:39:08 AM Vadim Bendebury wrote:
>> The command gets an arbitrary number of arguments (up to 30), which
>> are interpreted as byte values and are feed into the TPM device after
>> proper initialization. Then the return value and data of the TPM
>> driver is examined.
>>
>
> Dear Vadim Bendebury,
>
> [...]
>
>> diff --git a/common/cmd_tpm.c b/common/cmd_tpm.c
>> new file mode 100644
>> index 000..e008a78
>> --- /dev/null
>> +++ b/common/cmd_tpm.c
>> @@ -0,0 +1,111 @@
>> +/*
>> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
>> + * Released under the 2-clause BSD license.
>
> Are we ok with this ? Also, you say something about GPL in the same comment?
>
Can someone please tell me what needs to be put in the license headers
and I will do it. I hear different suggestions from different people.
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>
> [...]
>
>> + puts("Got TPM response:\n");
>> + for (i = 0; i < read_size; i++)
>> + printf(" %2.2x", tpm_buffer[i]);
>> + puts("\n");
>> + rv = 0;
>> + } else {
>> + puts("tpm command failed\n");
>> + }
>> + return rv;
>> +}
>
> You might want to use debug() where fitting ?
>
I don't expect failures and if happen prefer them to be printed
always, not only if debug is enabled.
>> +
>> +static int do_tpm(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>> argv[]) +{
>> + int rv = 0;
>> +
>> + /*
>> + * Verify that in case it is present, the first argument, it is
>> + * exactly one character in size.
>> + */
>> + if (argc < 7) {
>> + puts("command should be at least six bytes in size\n");
>> + return ~0;
>
> Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
>
I was under impression that any nonzero value is good. I see sometimes
-1 returned for error in other u-boot sources. Also, I am sorry, I am
new to this, when someone says "it is weird" - does this mean that it
has to be changed?
>> + }
>> +
>> + if (tis_init()) {
>> + puts("tis_init() failed!\n");
>> + return ~0;
>> + }
>> +
>> + if (tis_open()) {
>> + puts("tis_open() failed!\n");
>> + return ~0;
>> + }
>> +
>> + rv = tpm_process(argc - 1, argv + 1);
>> +
>> + if (!rv && tis_close()) {
>> + puts("tis_close() failed!\n");
>> + rv = ~0;
>
> This doesn't make much sense, tis_close() might not be called under all
> circumstances, is it ok ?
>
good point, I thought about this, but left untouched. It does not
matter with this driver, but is better to call tis_close() no matter
what. I'll fix it.
>> + }
>> +
>> + return rv;
>> +}
>> +
>> +U_BOOT_CMD(tpm, MAX_TRANSACTION_SIZE, 1, do_tpm,
>> + "tpm [] - write data and read ressponse\n",
>> + "send arbitrary data to the TPM and read the response"
>> +);
>> +
>> +static void report_error(const char *msg)
>> +{
>> + if (msg && *msg)
>
> Uhm, you also check if first character is non-zero? why ?
To avoid printing an empty string if someone calls this with an empty message?
>
>> + printf("%s\n", msg);
>> + cmd_usage(&__u_boot_cmd_tpm);
>
> Two underscores aren't a good practice.
>
I did this as a result of a previous review. Do you have a suggestion
how this should be done instead?
cheers,
/vb
>> +}
>
> Cheers
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Checkpatch warnings for "volatile"
On Saturday, October 15, 2011 10:56:54 AM Wolfgang Denk wrote: > Dear Prabhakar Lad, > > In message you wrote: > > > I've explained this a number of times recently - there are actually > > > very, very few occasions where "volatile" actually makes sense. > > > > > > Agreed, but I see a piece of code where virtual address are > > > compared. > > > > For example in arch/arm/cpu/arm926ejs/davinci/cpu.c > > In this function static inline unsigned pll_prediv(unsigned pllbase) > > > > and > > > > also in this static inline unsigned pll_postdiv(unsigned pllbase) > > > > Any suggestion on this on how to tackle or let it remain stagnant? > > I cannot see a justification for any of the ""volatile" in this file. > > Of course, all these ugly REG() calls should be converted to proper > use of I/O accessors. Definitelly ... but I'm not swiping this one, I have enough mess on my hands already ;-) Cheers ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
> TPM (Trusted Platform Module) is an integrated circuit and
> software platform that provides computer manufacturers with the
> core components of a subsystem used to assure authenticity,
> integrity and confidentiality.
[...]
Quick points:
* The license
* Use debug() when fitting
> diff --git a/drivers/tpm/generic_lpc_tpm.c b/drivers/tpm/generic_lpc_tpm.c
> new file mode 100644
> index 000..8319286
> --- /dev/null
> +++ b/drivers/tpm/generic_lpc_tpm.c
[...]
> +
> +struct lpc_tpm {
> + struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
> +};
Do you need such envelope ?
> +
> +static struct lpc_tpm *lpc_tpm_dev =
> + (struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS;
> +
> +/* Some registers' bit field definitions */
> +#define TIS_STS_VALID (1 << 7) /* 0x80 */
> +#define TIS_STS_COMMAND_READY (1 << 6) /* 0x40 */
> +#define TIS_STS_TPM_GO (1 << 5) /* 0x20 */
> +#define TIS_STS_DATA_AVAILABLE (1 << 4) /* 0x10 */
> +#define TIS_STS_EXPECT (1 << 3) /* 0x08 */
> +#define TIS_STS_RESPONSE_RETRY (1 << 1) /* 0x02 */
> +
> +#define TIS_ACCESS_TPM_REG_VALID_STS (1 << 7) /* 0x80 */
> +#define TIS_ACCESS_ACTIVE_LOCALITY (1 << 5) /* 0x20 */
> +#define TIS_ACCESS_BEEN_SEIZED (1 << 4) /* 0x10 */
> +#define TIS_ACCESS_SEIZE (1 << 3) /* 0x08 */
> +#define TIS_ACCESS_PENDING_REQUEST (1 << 2) /* 0x04 */
> +#define TIS_ACCESS_REQUEST_USE (1 << 1) /* 0x02 */
> +#define TIS_ACCESS_TPM_ESTABLISHMENT (1 << 0) /* 0x01 */
> +
> +#define TIS_STS_BURST_COUNT_MASK (0x)
> +#define TIS_STS_BURST_COUNT_SHIFT (8)
> +
> +/*
> + * Error value returned if a tpm register does not enter the expected
> state + * after continuous polling. No actual TPM register reading ever
> returns ~0, + * so this value is a safe error indication to be mixed with
> possible status + * register values.
> + */
> +#define TPM_TIMEOUT_ERR (~0)
> +
> +/* Error value returned on various TPM driver errors */
Dot missing at the end.
> +#define TPM_DRIVER_ERR (-1)
> +
> + /* 1 second is plenty for anything TPM does.*/
> +#define MAX_DELAY_US (1000 * 1000)
> +
> +/* Retrieve burst count value out of the status register contents. */
> +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT)
> & \ +TIS_STS_BURST_COUNT_MASK))
Do you need the cast ?
> +
> +/*
> + * Structures defined below allow creating descriptions of TPM
> vendor/device + * ID information for run time discovery. The only device
> the system knows + * about at this time is Infineon slb9635
> + */
> +struct device_name {
> + u16 dev_id;
> + const char * const dev_name;
> +};
> +
> +struct vendor_name {
> + u16 vendor_id;
> + const char *vendor_name;
> + const struct device_name *dev_names;
> +};
> +
> +static const struct device_name infineon_devices[] = {
> + {0xb, "SLB9635 TT 1.2"},
> + {0}
> +};
> +
> +static const struct vendor_name vendor_names[] = {
> + {0x15d1, "Infineon", infineon_devices},
> +};
> +
> +/*
> + * Cached vendor/device ID pair to indicate that the device has been
> already + * discovered
> + */
> +static u32 vendor_dev_id;
> +
> +/* TPM access going through macros to make tracing easier. */
> +#define tpm_read(ptr) ({ \
> + u32 __ret; \
> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
> +(u32)ptr - (u32)lpc_tpm_dev, __ret); \
> + __ret; })
> +
Make this inline function
> +#define tpm_write(value, ptr) ({\
> + u32 __v = value;\
> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \
> +(u32)ptr - (u32)lpc_tpm_dev, __v); \
> + if (sizeof(*ptr) == 1) \
> + writeb(__v, ptr); \
> + else \
> + writel(__v, ptr); })
> +
DTTO
[...]
> +static u32 tis_senddata(const u8 * const data, u32 len)
> +{
> + u32 offset = 0;
> + u16 burst = 0;
> + u32 max_cycles = 0;
> + u8 locality = 0;
> + u32 value;
> +
> + value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
> + TIS_STS_COMMAND_READY, TIS_STS_COMMAND_READY);
> + if (value == TPM_TIMEOUT_ERR) {
> + printf("%s:%d - failed to get 'command_ready' status\n",
> +__FILE__, __LINE__);
> + return TPM_DRIVER_ERR;
> + }
> + burst = BURST_COUNT(value);
> +
> + while (1) {
No endless loops please.
> + unsigned count;
> +
> + /* Wait till the device is ready to accept more data. */
> + while (!burst) {
> + if (max_cycles++ == MAX_DELAY_US) {
> + printf("%s:%d failed to feed %d bytes of %d\n",
> +__FILE__
Re: [U-Boot] [PATCH v2 2/2] Add a cli command to test the TPM device.
On Saturday, October 15, 2011 05:39:08 AM Vadim Bendebury wrote:
> The command gets an arbitrary number of arguments (up to 30), which
> are interpreted as byte values and are feed into the TPM device after
> proper initialization. Then the return value and data of the TPM
> driver is examined.
>
Dear Vadim Bendebury,
[...]
> diff --git a/common/cmd_tpm.c b/common/cmd_tpm.c
> new file mode 100644
> index 000..e008a78
> --- /dev/null
> +++ b/common/cmd_tpm.c
> @@ -0,0 +1,111 @@
> +/*
> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
> + * Released under the 2-clause BSD license.
Are we ok with this ? Also, you say something about GPL in the same comment?
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
[...]
> + puts("Got TPM response:\n");
> + for (i = 0; i < read_size; i++)
> + printf(" %2.2x", tpm_buffer[i]);
> + puts("\n");
> + rv = 0;
> + } else {
> + puts("tpm command failed\n");
> + }
> + return rv;
> +}
You might want to use debug() where fitting ?
> +
> +static int do_tpm(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[]) +{
> + int rv = 0;
> +
> + /*
> + * Verify that in case it is present, the first argument, it is
> + * exactly one character in size.
> + */
> + if (argc < 7) {
> + puts("command should be at least six bytes in size\n");
> + return ~0;
Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
> + }
> +
> + if (tis_init()) {
> + puts("tis_init() failed!\n");
> + return ~0;
> + }
> +
> + if (tis_open()) {
> + puts("tis_open() failed!\n");
> + return ~0;
> + }
> +
> + rv = tpm_process(argc - 1, argv + 1);
> +
> + if (!rv && tis_close()) {
> + puts("tis_close() failed!\n");
> + rv = ~0;
This doesn't make much sense, tis_close() might not be called under all
circumstances, is it ok ?
> + }
> +
> + return rv;
> +}
> +
> +U_BOOT_CMD(tpm, MAX_TRANSACTION_SIZE, 1, do_tpm,
> +"tpm [] - write data and read ressponse\n",
> +"send arbitrary data to the TPM and read the response"
> +);
> +
> +static void report_error(const char *msg)
> +{
> + if (msg && *msg)
Uhm, you also check if first character is non-zero? why ?
> + printf("%s\n", msg);
> + cmd_usage(&__u_boot_cmd_tpm);
Two underscores aren't a good practice.
> +}
Cheers
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] general ULPI support
On Saturday, October 15, 2011 03:19:45 PM Jana Rapava wrote: > Hi all, Dear Jana Rapava, please Cc respective custodians if you want to get _ANY_ feedback at all. > my patch, which added USB support for Efika, needed som ULPI functions. I > was suggested to write general ULPI support code rather than implenent > demanded functionality in the driver. > However, I've encountered following problems: > 1. Where should I put this code? Linux kernel has it in ./drivers/usb/otg/ > together with OTG utility code. Use this name looks a bit deceiving for me. > Should I create some directory named like ./drivers/usb/ulpi/, or is there > better place? drivers/usb/ulpi looks ok > 2. I don't know what to add to the Makefile. Should I create a new config > option (named CONFIG_ULPI I suppose) or simply add rule for new files? You definitelly don't want to compile the code in unconditionally. Anyway, do you really need the linux ulpi code as is or can you implement more lightweight thing? Cheers ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] NS16550: buffer reads
On Saturday, October 15, 2011 06:03:52 PM Simon Glass wrote: > From: Scott Wood > > From: Scott Wood > > This improves the performance of U-Boot when accepting rapid input, > such as pasting a sequence of commands. Hi Simon, [...] > diff --git a/include/ns16550.h b/include/ns16550.h > index 51f1c17..60b0abc 100644 > --- a/include/ns16550.h > +++ b/include/ns16550.h > @@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t; > > void NS16550_init (NS16550_t com_port, int baud_divisor); > void NS16550_putc (NS16550_t com_port, char c); > -char NS16550_getc (NS16550_t com_port); > -int NS16550_tstc (NS16550_t com_port); > +char NS16550_getc(NS16550_t regs, unsigned int port); > +int NS16550_tstc(NS16550_t regs, unsigned int port); > void NS16550_reinit (NS16550_t com_port, int baud_divisor); Maybe you should fix the indent altogether ? Cheers ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
On Sat, Oct 15, 2011 at 9:50 AM, Simon Glass wrote: > Hi Albert, > > On Sat, Oct 15, 2011 at 9:21 AM, Albert ARIBAUD > wrote: >> Le 15/10/2011 18:12, Simon Glass a écrit : >>> >>> Hi Wolfgang, >>> >>> On Sat, Oct 15, 2011 at 9:02 AM, Wolfgang Denk wrote: Dear Simon Glass, In message you wrote: > >> A general question: is the issue with input as such, or with echoing >> this >> input? > > The problem is in echoing the input, since this requires the uart to > wait once its transmit FIFO is full. While it is waiting (and not > checking the receive side), it misses characters coming from the other > end. Basically we need to constantly check the receiving side while we > are transmitting. No, we do not. U-Boot is strictly single tasking. This is one of the fundamental design principles. We do not receive network packets while sending ourself, or when doing other things. We do not receive characters on the serial line while sending characters, or when doing other things. This is known behaviour, and you should just adapt to it. I don't know what exactly is your situation when you run into such problems - the recommended approach for example when remote-controlling U-Boot is to wait for the next input prompt before sending the next line of characters - and stop after the newline, until you receive the next prompt. >>> >>> I certainly don't want to break a fundamental design principle. >>> . >>> The situation this occurs is when you paste characters into a serial >>> terminal connected to U-Boot. This is a pretty common requirement. Yes >>> you can manually select each line and paste it but that is a pain. >>> With this patch it is possible to paste a series of 'setenv' commands, >>> for example. >>> >>> This caused an awful lot of confusion here because characters would be >>> silently dropped within the command line (normally about 16 chars from >>> the end). Then the system would not boot. It is really not a friendly >>> experience. >>> >>> Regards, >>> Simon >> >> How about using the time-honored flow-control mechanism? If the serial port >> supports hardware CTS and RTS signals, you can use them to sync with the >> sender. If not, you can send XOFF and XON to respectively ask the sender to >> pause and resume sending. Of course, the sender must support hardware or >> software flow control. > > It is common to have just a 2-wire UART for the console. Perhaps this > is unfortunate. > > Are you suggesting that U-Boot should send XOFF sometimes, or does it > support this already? (I haven't seen it). When would U-Boot send > XOFF? Perhaps it could send XOFF at the end of each line that it > receives. Then XON when it starts reading the next line or maybe when > it runs out of characters and wants more. > > Most terminal problems will support software flow control. But are you > sure this is better? > > Regards, > Simon Another thought...the problem is that every character we receive we echo, so we can of course never get ahead of the workload. But we can get behind, since while we are doing something other than receiving serial characters (e.g. processing a command, outputing a prompt, updating the LCD) we are not transmitting. Then when we next go to receive, we find (say) 10 characters built up, and start echoing them all. We are now 10 characters behind the receive and will stay that way until there is a let-up. Even worse if one of the commands outputs something, since then we might be 20 or 30 characters behind, and start dropping things. Even if no commands output anything, delays and the U-Boot prompt will create a problem. (As an aside, I note the call to the USB code to deal with transmission/reception so clearly there is some interrupt handling going on...) So another approach would be to buffer all output at the console layer and send it out in the fullness of time using a function similar to tstc() that tests whether or not there is space in the output fifo. This seems a more invasive but more general fix. I'm not that keen on it since it changes something that people might rely on - that printf() waits if the serial fifo is full. It might mean that prior to running bootm U-Boot would need to wait for the buffer to flush. Ick. But I am happy to look at this if you think it isn't even worse. Regards, Simon > >> >> Amicalement, >> -- >> Albert. >> > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 01/10] Add getenv_ulong() to read an integer from an environment variable
Acked-by: Mike Frysinger -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] zlib: handle overflow while calculating available stream input size
On Saturday 15 October 2011 12:32:52 Anatolij Gustschin wrote: > If compressed data is located in sectors at the end of the flash and > it's offset + input stream size > 0x, the uncompressing time > is very long, since processing of the stream is done bytewise (and > not blockwise) due to overflow in inflate_fast() while calculation > and checking for enough input available. > > Check for this overflow condition and limit the available stream > input size to the actually max. possible input size. This fixes > the problem. > > The issue is easily reproduceable by placing a gziped bitmap in flash, > e.g. at FFF8, and running 'bmp' commands like 'bmp info FFF8' > or 'bmp display FFF8'. The uncompressing can take up to 3 sec. > whereas it should normaly take a fraction of a second. If the > 'splashimage' environment variable points to this address, the > booting time also increases significantly. can we send this to the upstream zlib project if it isn't there already and get feedback from them ? -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
Hi Albert, On Sat, Oct 15, 2011 at 9:21 AM, Albert ARIBAUD wrote: > Le 15/10/2011 18:12, Simon Glass a écrit : >> >> Hi Wolfgang, >> >> On Sat, Oct 15, 2011 at 9:02 AM, Wolfgang Denk wrote: >>> >>> Dear Simon Glass, >>> >>> In >>> message >>> you wrote: > A general question: is the issue with input as such, or with echoing > this > input? The problem is in echoing the input, since this requires the uart to wait once its transmit FIFO is full. While it is waiting (and not checking the receive side), it misses characters coming from the other end. Basically we need to constantly check the receiving side while we are transmitting. >>> >>> No, we do not. >>> >>> U-Boot is strictly single tasking. This is one of the fundamental >>> design principles. We do not receive network packets while sending >>> ourself, or when doing other things. We do not receive characters on >>> the serial line while sending characters, or when doing other things. >>> >>> This is known behaviour, and you should just adapt to it. I don't know >>> what exactly is your situation when you run into such problems - the >>> recommended approach for example when remote-controlling U-Boot is to >>> wait for the next input prompt before sending the next line of >>> characters - and stop after the newline, until you receive the next >>> prompt. >> >> I certainly don't want to break a fundamental design principle. >> . >> The situation this occurs is when you paste characters into a serial >> terminal connected to U-Boot. This is a pretty common requirement. Yes >> you can manually select each line and paste it but that is a pain. >> With this patch it is possible to paste a series of 'setenv' commands, >> for example. >> >> This caused an awful lot of confusion here because characters would be >> silently dropped within the command line (normally about 16 chars from >> the end). Then the system would not boot. It is really not a friendly >> experience. >> >> Regards, >> Simon > > How about using the time-honored flow-control mechanism? If the serial port > supports hardware CTS and RTS signals, you can use them to sync with the > sender. If not, you can send XOFF and XON to respectively ask the sender to > pause and resume sending. Of course, the sender must support hardware or > software flow control. It is common to have just a 2-wire UART for the console. Perhaps this is unfortunate. Are you suggesting that U-Boot should send XOFF sometimes, or does it support this already? (I haven't seen it). When would U-Boot send XOFF? Perhaps it could send XOFF at the end of each line that it receives. Then XON when it starts reading the next line or maybe when it runs out of characters and wants more. Most terminal problems will support software flow control. But are you sure this is better? Regards, Simon > > Amicalement, > -- > Albert. > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] zlib: handle overflow while calculating available stream input size
If compressed data is located in sectors at the end of the flash and
it's offset + input stream size > 0x, the uncompressing time
is very long, since processing of the stream is done bytewise (and
not blockwise) due to overflow in inflate_fast() while calculation
and checking for enough input available.
Check for this overflow condition and limit the available stream
input size to the actually max. possible input size. This fixes
the problem.
The issue is easily reproduceable by placing a gziped bitmap in flash,
e.g. at FFF8, and running 'bmp' commands like 'bmp info FFF8'
or 'bmp display FFF8'. The uncompressing can take up to 3 sec.
whereas it should normaly take a fraction of a second. If the
'splashimage' environment variable points to this address, the
booting time also increases significantly.
Signed-off-by: Anatolij Gustschin
---
Note:
there are 6 errors and 4 warning reported by checkpatch.pl
for this patch. I didn't fix them intentionally and used the
coding style of the zlib code here. The code would look ugly
otherwise.
lib/zlib/inffast.c |8
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/lib/zlib/inffast.c b/lib/zlib/inffast.c
index 8e823df..4834b0c 100644
--- a/lib/zlib/inffast.c
+++ b/lib/zlib/inffast.c
@@ -100,6 +100,14 @@ unsigned start; /* inflate()'s starting value for
strm->avail_out */
state = (struct inflate_state FAR *)strm->state;
in = strm->next_in - OFF;
last = in + (strm->avail_in - 5);
+if (in > last && strm->avail_in > 5) {
+/*
+ * overflow detected, limit strm->avail_in to the
+ * max. possible size and recalculate last
+ */
+strm->avail_in = 0x - (unsigned int)in;
+last = in + (strm->avail_in - 5);
+}
out = strm->next_out - OFF;
beg = out - (start - strm->avail_out);
end = out + (strm->avail_out - 257);
--
1.7.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
Le 15/10/2011 18:12, Simon Glass a écrit : > Hi Wolfgang, > > On Sat, Oct 15, 2011 at 9:02 AM, Wolfgang Denk wrote: >> Dear Simon Glass, >> >> In >> message >> you wrote: >>> A general question: is the issue with input as such, or with echoing this input? >>> >>> The problem is in echoing the input, since this requires the uart to >>> wait once its transmit FIFO is full. While it is waiting (and not >>> checking the receive side), it misses characters coming from the other >>> end. Basically we need to constantly check the receiving side while we >>> are transmitting. >> >> No, we do not. >> >> U-Boot is strictly single tasking. This is one of the fundamental >> design principles. We do not receive network packets while sending >> ourself, or when doing other things. We do not receive characters on >> the serial line while sending characters, or when doing other things. >> >> This is known behaviour, and you should just adapt to it. I don't know >> what exactly is your situation when you run into such problems - the >> recommended approach for example when remote-controlling U-Boot is to >> wait for the next input prompt before sending the next line of >> characters - and stop after the newline, until you receive the next >> prompt. > > I certainly don't want to break a fundamental design principle. > . > The situation this occurs is when you paste characters into a serial > terminal connected to U-Boot. This is a pretty common requirement. Yes > you can manually select each line and paste it but that is a pain. > With this patch it is possible to paste a series of 'setenv' commands, > for example. > > This caused an awful lot of confusion here because characters would be > silently dropped within the command line (normally about 16 chars from > the end). Then the system would not boot. It is really not a friendly > experience. > > Regards, > Simon How about using the time-honored flow-control mechanism? If the serial port supports hardware CTS and RTS signals, you can use them to sync with the sender. If not, you can send XOFF and XON to respectively ask the sender to pause and resume sending. Of course, the sender must support hardware or software flow control. Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3] NS16550: buffer reads
From: Scott Wood
From: Scott Wood
This improves the performance of U-Boot when accepting rapid input,
such as pasting a sequence of commands.
Without this patch, on P4080DS I see a maximum of around 5 lines can
be pasted. With this patch, it handles around 70 lines before lossage,
long enough for most things you'd paste.
With it enabled, on ppc it's an extra 396 bytes of image size, and 1056
bytes of BSS.
ARM note from Simon Glass - ARM code size goes from
212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
Signed-off-by: Scott Wood
---
Changes in v2:
- Fix checkpatch warnings, the other one was already there
Changes in v3:
- Use CONFIG_NS16550_BUFFER_READS to set the buffer size also
README | 12 ++
drivers/serial/ns16550.c | 96 +++--
drivers/serial/serial.c |5 +-
include/ns16550.h|4 +-
4 files changed, 108 insertions(+), 9 deletions(-)
diff --git a/README b/README
index 7e032a9..1150d6c 100644
--- a/README
+++ b/README
@@ -2862,6 +2862,18 @@ use the "saveenv" command to store a valid environment.
space for already greatly restricted images, including but not
limited to NAND_SPL configurations.
+- CONFIG_NS16550_BUFFER_READS:
+ Instead of reading directly from the receive register
+ every time U-Boot is ready for another byte, keep a
+ buffer and fill it from the hardware fifo every time
+ U-Boot reads a character. This helps U-Boot keep up with
+ a larger amount of rapid input, such as happens when
+ pasting text into the terminal.
+
+ To use this option, define CONFIG_NS16550_BUFFER_READS to
+ the size of the buffer you want (256 is a reasonable value).
+
+
Low Level (hardware related) configuration options:
---
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 0174744..6429e4d 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -4,12 +4,15 @@
* modified to use CONFIG_SYS_ISA_MEM and new defines
*/
+#include
#include
#include
#include
#include
#include
+DECLARE_GLOBAL_DATA_PTR;
+
#define UART_LCRVAL UART_LCR_8N1 /* 8 data, 1 stop, no parity */
#define UART_MCRVAL (UART_MCR_DTR | \
UART_MCR_RTS) /* RTS/DTR */
@@ -92,21 +95,104 @@ void NS16550_putc (NS16550_t com_port, char c)
}
#ifndef CONFIG_NS16550_MIN_FUNCTIONS
-char NS16550_getc (NS16550_t com_port)
+
+static char NS16550_raw_getc(NS16550_t regs)
{
- while ((serial_in(&com_port->lsr) & UART_LSR_DR) == 0) {
+ while ((serial_in(®s->lsr) & UART_LSR_DR) == 0) {
#ifdef CONFIG_USB_TTY
extern void usbtty_poll(void);
usbtty_poll();
#endif
WATCHDOG_RESET();
}
- return serial_in(&com_port->rbr);
+ return serial_in(®s->rbr);
+}
+
+static int NS16550_raw_tstc(NS16550_t regs)
+{
+ return ((serial_in(®s->lsr) & UART_LSR_DR) != 0);
+}
+
+
+#ifdef CONFIG_NS16550_BUFFER_READS
+
+#define BUF_SIZE CONFIG_NS16550_BUFFER_READS
+#define NUM_PORTS 4
+
+struct ns16550_priv {
+ char buf[BUF_SIZE];
+ unsigned int head, tail;
+};
+
+static struct ns16550_priv rxstate[NUM_PORTS];
+
+static void enqueue(unsigned int port, char ch)
+{
+ /* If queue is full, drop the character. */
+ if ((rxstate[port].head - rxstate[port].tail - 1) % BUF_SIZE == 0)
+ return;
+
+ rxstate[port].buf[rxstate[port].tail] = ch;
+ rxstate[port].tail = (rxstate[port].tail + 1) % BUF_SIZE;
+}
+
+static int dequeue(unsigned int port, char *ch)
+{
+ /* Empty queue? */
+ if (rxstate[port].head == rxstate[port].tail)
+ return 0;
+
+ *ch = rxstate[port].buf[rxstate[port].head];
+ rxstate[port].head = (rxstate[port].head + 1) % BUF_SIZE;
+ return 1;
+}
+
+static void fill_rx_buf(NS16550_t regs, unsigned int port)
+{
+ while ((serial_in(®s->lsr) & UART_LSR_DR) != 0)
+ enqueue(port, serial_in(®s->rbr));
+}
+
+char NS16550_getc(NS16550_t regs, unsigned int port)
+{
+ char ch;
+
+ if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC))
+ return NS16550_raw_getc(regs);
+
+ do {
+#ifdef CONFIG_USB_TTY
+ extern void usbtty_poll(void);
+ usbtty_poll();
+#endif
+ fill_rx_buf(regs, port);
+ WATCHDOG_RESET();
+ } while (!dequeue(port, &ch));
+
+ return ch;
+}
+
+int NS16550_tstc(NS16550_t regs, unsigned int port)
+{
+ if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC))
+ return NS16550_raw_tstc(regs);
+
+ fill_rx_buf(regs, port);
+
+ return rxstate[port].head != rxstate[port].tail;
+}
+
+#else /* CONFIG_NS16550_BUFFER_READS */
+
+char NS16550_getc(NS16550_t regs, unsigned int port)
+{
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
Hi Wolfgang, On Sat, Oct 15, 2011 at 9:02 AM, Wolfgang Denk wrote: > Dear Simon Glass, > > In message > you > wrote: >> >> > A general question: is the issue with input as such, or with echoing this >> > input? >> >> The problem is in echoing the input, since this requires the uart to >> wait once its transmit FIFO is full. While it is waiting (and not >> checking the receive side), it misses characters coming from the other >> end. Basically we need to constantly check the receiving side while we >> are transmitting. > > No, we do not. > > U-Boot is strictly single tasking. This is one of the fundamental > design principles. We do not receive network packets while sending > ourself, or when doing other things. We do not receive characters on > the serial line while sending characters, or when doing other things. > > This is known behaviour, and you should just adapt to it. I don't know > what exactly is your situation when you run into such problems - the > recommended approach for example when remote-controlling U-Boot is to > wait for the next input prompt before sending the next line of > characters - and stop after the newline, until you receive the next > prompt. I certainly don't want to break a fundamental design principle. . The situation this occurs is when you paste characters into a serial terminal connected to U-Boot. This is a pretty common requirement. Yes you can manually select each line and paste it but that is a pain. With this patch it is possible to paste a series of 'setenv' commands, for example. This caused an awful lot of confusion here because characters would be silently dropped within the command line (normally about 16 chars from the end). Then the system would not boot. It is really not a friendly experience. Regards, Simon > > 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: w...@denx.de > I believe you find life such a problem because you think there are > the good people and the bad people. You're wrong, of course. There > are, always and only, the bad people, but some of them are on oppo- > site sides. - Terry Pratchett, _Guards! Guards!_ > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v4 6/6] fdt: add decode helper library
This library provides useful functions to drivers which want to use
the fdt to control their operation. Functions are provided to:
- look up and enumerate a device type (for example assigning i2c bus 0,
i2c bus 1, etc.)
- decode basic types from the fdt, like addresses and integers
While this library is not strictly necessary, it helps to minimise the
changes to a driver, in order to make it work under fdt control. Less
code is required, and so the barrier to switch drivers over is lower.
Additional functions to read arrays and GPIOs could be made available
here also.
Signed-off-by: Simon Glass
---
Changes in v2:
- Add example proposed decode helper library
Changes in v3:
- Simplify decode library to remove provide only primitive functions
- Remove i2c decode function
- Rename fdt_decode to fdtdec, since it will be used a lot
- Moved fdt_decode.c to /lib
- Export almost all functions from fdtdec, to allow widespread use
- Remove use of FDT_ERR_MISSING which is not strictly needed now
Changes in v4:
- Add assert on sprintf() string length
- Rename addr_t to fdt_addr_t to make it more fdt-specific
- Remove gpio.h header in fdtdec.c which is not needed yet
include/fdtdec.h | 122 ++
lib/Makefile |1 +
lib/fdtdec.c | 131 ++
3 files changed, 254 insertions(+), 0 deletions(-)
create mode 100644 include/fdtdec.h
create mode 100644 lib/fdtdec.c
diff --git a/include/fdtdec.h b/include/fdtdec.h
new file mode 100644
index 000..ae36f9c
--- /dev/null
+++ b/include/fdtdec.h
@@ -0,0 +1,122 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+
+/*
+ * This file contains convenience functions for decoding useful and
+ * enlightening information from FDTs. It is intended to be used by device
+ * drivers and board-specific code within U-Boot. It aims to reduce the
+ * amount of FDT munging required within U-Boot itself, so that driver code
+ * changes to support FDT are minimized.
+ */
+
+#include
+
+/*
+ * A typedef for a physical address. Note that fdt data is always big
+ * endian even on a litle endian machine.
+ */
+#ifdef CONFIG_PHYS_64BIT
+typedef u64 fdt_addr_t;
+#define FDT_ADDR_T_NONE (-1ULL)
+#define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
+#else
+typedef u32 fdt_addr_t;
+#define FDT_ADDR_T_NONE (-1U)
+#define fdt_addr_to_cpu(reg) be32_to_cpu(reg)
+#endif
+
+/* Information obtained about memory from the FDT */
+struct fdt_memory {
+ fdt_addr_t start;
+ fdt_addr_t end;
+};
+
+/**
+ * Compat types that we know about and for which we might have drivers.
+ * Each is named COMPAT__ where is the directory
+ * within drivers.
+ */
+enum fdt_compat_id {
+ COMPAT_UNKNOWN,
+
+ COMPAT_COUNT,
+};
+
+/**
+ * Find the next numbered alias for a peripheral. This is used to enumerate
+ * all the peripherals of a certain type.
+ *
+ * Do the first call with *upto = 0. Assuming /aliases/0 exists then
+ * this function will return a pointer to the node the alias points to, and
+ * then update *upto to 1. Next time you call this function, the next node
+ * will be returned.
+ *
+ * All nodes returned will match the compatible ID, as it is assumed that
+ * all peripherals use the same driver.
+ *
+ * @param blob FDT blob to use
+ * @param name Root name of alias to search for
+ * @param id Compatible ID to look for
+ * @return offset of next compatible node, or -FDT_ERR_NOTFOUND if no more
+ */
+int fdtdec_next_alias(const void *blob, const char *name,
+ enum fdt_compat_id id, int *upto);
+
+/**
+ * Look up an address property in a node and return it as an address.
+ * The property must hold either one address with no trailing data or
+ * one address with a length. This is only tested on 32-bit machines.
+ *
+ * @param blob FDT blob
+ * @param node node to examine
+ * @param prop_namename of property to find
+ * @return address, if found, or FDT_ADDR_T_NONE if not
+ */
+fdt_addr_t fdtdec_get_addr(const void *blob, int node,
+ const char *prop_name);
+
+/**
+ * Look up a 32-bit integer property in a node and return it.
[U-Boot] [PATCH v4 1/6] fdt: ARM: Add device tree control of U-Boot (CONFIG_OF_CONTROL)
This adds a device tree pointer to the global data. It can be set by
board code. A later commit will add support for making a device
tree binary blob available to U-Boot for run-time configuration.
Signed-off-by: Simon Glass
---
Changes in v3:
- Rename gd->blob to gd->fdt_blob
Changes in v4:
- Fix reference to gd->blob which should be gd->fdt_blob
README | 11 +++
arch/arm/include/asm/global_data.h |1 +
2 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/README b/README
index 7e032a9..499349f 100644
--- a/README
+++ b/README
@@ -811,6 +811,17 @@ The following options need to be configured:
XXX - this list needs to get updated!
+- Device tree:
+ CONFIG_OF_CONTROL
+ If this variable is defined, U-Boot will use a device tree
+ to configure its devices, instead of relying on statically
+ compiled #defines in the board file. This option is
+ experimental and only available on a few boards. The device
+ tree is available in the global data as gd->fdt_blob.
+
+ U-Boot needs to get its device tree from somewhere. This will
+ be enabled in a future patch.
+
- Watchdog:
CONFIG_WATCHDOG
If this variable is defined, it enables watchdog
diff --git a/arch/arm/include/asm/global_data.h
b/arch/arm/include/asm/global_data.h
index fac98d5..c3ff789 100644
--- a/arch/arm/include/asm/global_data.h
+++ b/arch/arm/include/asm/global_data.h
@@ -76,6 +76,7 @@ typedef struct global_data {
#if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
unsigned long tlb_addr;
#endif
+ const void *fdt_blob; /* Our device tree, NULL if none */
void**jt; /* jump table */
charenv_buf[32];/* buffer for getenv() before reloc. */
#if defined(CONFIG_POST) || defined(CONFIG_LOGBUFFER)
--
1.7.3.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v4 3/6] fdt: Add support for a separate device tree (CONFIG_OF_SEPARATE)
This adds support for an FDT to be build as a separate binary file called
u-boot.dtb. This can be concatenated with the U-Boot binary to provide a
device tree located at run-time by U-Boot. The Makefile is modified to
provide this file in u-boot-dtb.bin.
Signed-off-by: Simon Glass
---
Changes in v2:
- Modify Makefile to create combined U-boot/fdt (u-boot-dtb.bin)
.gitignore |1 +
Makefile |8
README | 16 ++--
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/.gitignore b/.gitignore
index 0a9dc0c..a367082 100644
--- a/.gitignore
+++ b/.gitignore
@@ -35,6 +35,7 @@
/u-boot.dis
/u-boot.lds
/u-boot.ubl
+/u-boot.dtb
#
# Generated files
diff --git a/Makefile b/Makefile
index 05f64b7..9a640e9 100644
--- a/Makefile
+++ b/Makefile
@@ -354,9 +354,17 @@ ALL-$(CONFIG_ONENAND_U_BOOT) += $(obj)u-boot-onenand.bin
ONENAND_BIN ?= $(obj)onenand_ipl/onenand-ipl-2k.bin
ALL-$(CONFIG_MMC_U_BOOT) += $(obj)mmc_spl/u-boot-mmc-spl.bin
ALL-$(CONFIG_SPL) += $(obj)spl/u-boot-spl.bin
+ALL-$(CONFIG_OF_SEPARATE) += $(obj)u-boot.dtb $(obj)u-boot-dtb.bin
all: $(ALL-y)
+$(obj)u-boot.dtb: $(obj)u-boot
+ $(MAKE) -C dts binary
+ mv $(obj)dts/dt.dtb $@
+
+$(obj)u-boot-dtb.bin: $(obj)u-boot.bin $(obj)u-boot.dtb
+ cat $^ >$@
+
$(obj)u-boot.hex: $(obj)u-boot
$(OBJCOPY) ${OBJCFLAGS} -O ihex $< $@
diff --git a/README b/README
index b5cdcb5..fc1cd00 100644
--- a/README
+++ b/README
@@ -819,8 +819,8 @@ The following options need to be configured:
experimental and only available on a few boards. The device
tree is available in the global data as gd->fdt_blob.
- U-Boot needs to get its device tree from somewhere. At present
- the only way is to embed it in the image with CONFIG_OF_EMBED.
+ U-Boot needs to get its device tree from somewhere. This can
+ be done using one of the two options below:
CONFIG_OF_EMBED
If this variable is defined, U-Boot will embed a device tree
@@ -829,6 +829,18 @@ The following options need to be configured:
is then picked up in board_init_f() and made available through
the global data structure as gd->blob.
+ CONFIG_OF_SEPARATE
+ If this variable is defined, U-Boot will build a device tree
+ binary. It will be called u-boot.dtb. Architecture-specific
+ code will locate it at run-time. Generally this works by:
+
+ cat u-boot.bin u-boot.dtb >image.bin
+
+ and in fact, U-Boot does this for you, creating a file called
+ u-boot-dtb.bin which is useful in the common case. You can
+ still use the individual files if you need something more
+ exotic.
+
- Watchdog:
CONFIG_WATCHDOG
If this variable is defined, it enables watchdog
--
1.7.3.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v4 4/6] fdt: ARM: Implement and verify embedded and separate device tree
This locates the device tree either embedded within U-Boot or attached to the
end as a separate binary.
When CONFIG_OF_CONTROL is defined, U-Boot requires a valid fdt. A check is
provided for this early in initialisation.
Signed-off-by: Simon Glass
---
arch/arm/lib/board.c | 22 ++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index 1fe3751..b0f3162 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -48,6 +48,7 @@
#include
#include
#include
+#include
#include
#include
@@ -197,6 +198,17 @@ static int arm_pci_init(void)
}
#endif /* CONFIG_CMD_PCI || CONFIG_PCI */
+#ifdef CONFIG_OF_CONTROL
+static int check_fdt(void)
+{
+ /* We must have an fdt */
+ if (fdt_check_header(gd->fdt_blob))
+ panic("No valid fdt found - please append one to U-Boot\n"
+ "binary or define CONFIG_OF_EMBED\n");
+ return 0;
+}
+#endif
+
/*
* Breathe some life into the board...
*
@@ -239,6 +251,9 @@ init_fnc_t *init_sequence[] = {
#if defined(CONFIG_BOARD_EARLY_INIT_F)
board_early_init_f,
#endif
+#ifdef CONFIG_OF_CONTROL
+ check_fdt,
+#endif
timer_init, /* initialize timer */
#ifdef CONFIG_FSL_ESDHC
get_clocks,
@@ -276,6 +291,13 @@ void board_init_f(ulong bootflag)
memset((void *)gd, 0, sizeof(gd_t));
gd->mon_len = _bss_end_ofs;
+#ifdef CONFIG_OF_EMBED
+ /* Get a pointer to the FDT */
+ gd->fdt_blob = _binary_dt_dtb_start;
+#elif defined CONFIG_OF_SEPARATE
+ /* FDT is at end of image */
+ gd->fdt_blob = (void *)(_end_ofs + _TEXT_BASE);
+#endif
for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
if ((*init_fnc_ptr)() != 0) {
--
1.7.3.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v4 2/6] fdt: Add support for embedded device tree (CONFIG_OF_EMBED)
This new option allows U-Boot to embed a binary device tree into its image
to allow run-time control of peripherals. This device tree is for U-Boot's
own use and is not necessarily the same one as is passed to the kernel.
The device tree compiler output should be placed in the $(obj)
rooted tree. Since $(OBJCOPY) insists on adding the path to the
generated symbol names, to ensure consistency it should be
invoked from the directory where the .dtb file is located and
given the input file name without the path.
This commit contains my entry for the ugliest Makefile / shell interaction
competition.
Signed-off-by: Simon Glass
---
Changes in v3:
- Move fdt files into board//dts
- Provide access to ARCH_CPU_DTS which is the CPU's base device tree
Changes in v4:
- Remove unused 'clean' targets in dts/Makefile (remmove *.tmp at top-level)
- Move documentation into the first 'fdt' patch in the series
- Add note about CONFIG_ARCH_DEVICE_TREE
Makefile |5 ++
README | 11 +++-
config.mk |1 +
doc/README.fdt-control | 172
dts/Makefile | 103 +
include/common.h |1 +
6 files changed, 291 insertions(+), 2 deletions(-)
create mode 100644 doc/README.fdt-control
create mode 100644 dts/Makefile
diff --git a/Makefile b/Makefile
index 5db2e0e..05f64b7 100644
--- a/Makefile
+++ b/Makefile
@@ -224,6 +224,9 @@ endif
ifeq ($(CPU),ixp)
LIBS += arch/arm/cpu/ixp/npe/libnpe.o
endif
+ifeq ($(CONFIG_OF_EMBED),y)
+LIBS += dts/libdts.o
+endif
LIBS += arch/$(ARCH)/lib/lib$(ARCH).o
LIBS += fs/cramfs/libcramfs.o fs/fat/libfat.o fs/fdos/libfdos.o
fs/jffs2/libjffs2.o \
fs/reiserfs/libreiserfs.o fs/ext2/libext2fs.o fs/yaffs2/libyaffs2.o \
@@ -962,6 +965,7 @@ clobber:clean
@rm -f $(obj)u-boot.kwb
@rm -f $(obj)u-boot.imx
@rm -f $(obj)u-boot.ubl
+ @rm -f $(obj)u-boot.dtb
@rm -f $(obj)tools/{env/crc32.c,inca-swap-bytes}
@rm -f $(obj)arch/powerpc/cpu/mpc824x/bedbug_603e.c
@rm -fr $(obj)include/asm/proc $(obj)include/asm/arch $(obj)include/asm
@@ -969,6 +973,7 @@ clobber:clean
@[ ! -d $(obj)nand_spl ] || find $(obj)nand_spl -name "*" -type l
-print | xargs rm -f
@[ ! -d $(obj)onenand_ipl ] || find $(obj)onenand_ipl -name "*" -type l
-print | xargs rm -f
@[ ! -d $(obj)mmc_spl ] || find $(obj)mmc_spl -name "*" -type l -print
| xargs rm -f
+ @rm -f $(obj)dts/*.tmp
mrproper \
distclean: clobber unconfig
diff --git a/README b/README
index 499349f..b5cdcb5 100644
--- a/README
+++ b/README
@@ -819,8 +819,15 @@ The following options need to be configured:
experimental and only available on a few boards. The device
tree is available in the global data as gd->fdt_blob.
- U-Boot needs to get its device tree from somewhere. This will
- be enabled in a future patch.
+ U-Boot needs to get its device tree from somewhere. At present
+ the only way is to embed it in the image with CONFIG_OF_EMBED.
+
+ CONFIG_OF_EMBED
+ If this variable is defined, U-Boot will embed a device tree
+ binary in its image. This device tree file should be in the
+ board directory and called -.dts. The binary file
+ is then picked up in board_init_f() and made available through
+ the global data structure as gd->blob.
- Watchdog:
CONFIG_WATCHDOG
diff --git a/config.mk b/config.mk
index e2b440d..6e61eb6 100644
--- a/config.mk
+++ b/config.mk
@@ -124,6 +124,7 @@ STRIP = $(CROSS_COMPILE)strip
OBJCOPY = $(CROSS_COMPILE)objcopy
OBJDUMP = $(CROSS_COMPILE)objdump
RANLIB = $(CROSS_COMPILE)RANLIB
+DTC= dtc
#
diff --git a/doc/README.fdt-control b/doc/README.fdt-control
new file mode 100644
index 000..3f8bb5a
--- /dev/null
+++ b/doc/README.fdt-control
@@ -0,0 +1,172 @@
+#
+# Copyright (c) 2011 The Chromium OS Authors.
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundatio; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+#
+
+Device Tree Control in U-Boot
+==
[U-Boot] [PATCH v4 5/6] fdt: ARM: Add fdtcontroladdr to set device tree address in environment
This adds support for a new environment variable called 'fdtcontroladdr'. If
defined, the hex address is used as the address of the control fdt for U-Boot.
Note: I have not changed CONFIG_PRAM section as I already have an
outstanding patch on that.
Signed-off-by: Simon Glass
---
Changes in v3:
- Add fdtcontroladdr environment variable
README |4
arch/arm/lib/board.c | 30 +++---
doc/README.fdt-control | 12
3 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/README b/README
index fc1cd00..3b1faf9 100644
--- a/README
+++ b/README
@@ -3493,6 +3493,10 @@ List of environment variables (most likely not complete):
add the information it needs into it, and the memory
must be accessible by the kernel.
+ fdtcontroladdr- if set this is the address of the control flattened
+ device tree used by U-Boot when CONFIG_OF_CONTROL is
+ defined.
+
i2cfast - (PPC405GP|PPC405EP only)
if set to 'y' configures Linux I2C driver for fast
mode (400kHZ). This environment variable is used in
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index b0f3162..f26d77a 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -116,18 +116,31 @@ void blue_led_off(void) __attribute__((weak,
alias("__blue_led_off")));
* but let's get it working (again) first...
*/
+/**
+ * Decode the value of an environment variable and return it.
+ *
+ * @param name Name of environemnt variable
+ * @param base Number base to use (normally 10, or 16 for hex)
+ * @param default_val Default value to return if the variable is not
+ * found
+ * @return the decoded value, or default_val if not found
+ */
+static int getenv_int(const char *name, int base, int default_val)
+{
+ char tmp[64]; /* long enough for environment variables */
+ int i = getenv_f(name, tmp, sizeof(tmp));
+
+ return (i > 0)
+ ? (int) simple_strtoul(tmp, NULL, base)
+ : default_val;
+}
+
#if defined(CONFIG_ARM_DCC) && !defined(CONFIG_BAUDRATE)
#define CONFIG_BAUDRATE 115200
#endif
static int init_baudrate(void)
{
- char tmp[64]; /* long enough for environment variables */
- int i = getenv_f("baudrate", tmp, sizeof(tmp));
-
- gd->baudrate = (i > 0)
- ? (int) simple_strtoul(tmp, NULL, 10)
- : CONFIG_BAUDRATE;
-
+ gd->baudrate = getenv_int("baudrate", 10, CONFIG_BAUDRATE);
return (0);
}
@@ -298,6 +311,9 @@ void board_init_f(ulong bootflag)
/* FDT is at end of image */
gd->fdt_blob = (void *)(_end_ofs + _TEXT_BASE);
#endif
+ /* Allow the early environment to override the fdt address */
+ gd->fdt_blob = (void *)getenv_int("fdtcontroladdr", 16,
+ (uintptr_t)gd->fdt_blob);
for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
if ((*init_fnc_ptr)() != 0) {
diff --git a/doc/README.fdt-control b/doc/README.fdt-control
index 3f8bb5a..85bda03 100644
--- a/doc/README.fdt-control
+++ b/doc/README.fdt-control
@@ -144,6 +144,18 @@ and then flash image.bin onto your board.
You cannot use both of these options at the same time.
+If you wish to put the fdt at a different address in memory, you can
+define the "fdtcontroladdr" environment variable. This is the hex
+address of the fdt binary blob, and will override either of the options.
+Be aware that this environment variable is checked prior to relocation,
+when only the compiled-in environment is available. Therefore it is not
+possible to define this variable in the saved SPI/NAND flash
+environment, for example (it will be ignored).
+
+To use this, put something like this in your board header file:
+
+#define CONFIG_EXTRA_ENV_SETTINGS "fdtcontroladdr=1\0"
+
Limitations
---
--
1.7.3.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v4 0/6] Run-time configuration of U-Boot via a flat device tree (fdt)
At present in U-Boot configuration is mostly done using CONFIG options in the board file. This patch set makes it possible for a single U-Boot binary to support multiple boards, with the exact configuration of each board controlled by a flat device tree (fdt). This is the approach recently taken by the ARM Linux kernel and has been used by PowerPC for some time. In other words, manufacturers can potentially use a single U-Boot binary across an entire product line, with one fdt per model. The fdt is a convenient vehicle for implementing run-time configuration for three reasons. Firstly it is easy to use, being a simple text file. It is extensible since it consists of nodes and properties in a nice hierarchical format. Finally, there is already excellent infrastructure for the fdt: a compiler checks the text file and converts it to a compact binary format, and a library is already available in U-Boot (libfdt) for handling this format. To read about fdts, take a look at the specification here: https://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf You also might find this section of the Linux kernel documentation useful: (access this in the Linux kernel source code) Documentation/devicetree/booting-without-of.txt To use this patch set you will need to get the device tree compiler here: git://jdl.com/software/dtc.git and add some defines to your board (only ARM is currently supported): #define CONFIG_OF_CONTROL (to enable run-time config control via fdt) #define CONFIG_OF_EMBED or CONFIG_OF_SEPARATE (either build the fdt blob into U-Boot, or create a separate u-boot.dtb and u-boot-dtb.bin) #define CONFIG_DEFAULT_DEVICE_TREE "" (to specify the name of the device tree file is board//dts/.dts) Typically a CPU device tree include file is provided which defines all the devices available on that CPU/SOC, with each set to 'status = "ok"'. Board device tree files should adjust only the devices they need to. If a device should be disabled, then it should be changed to 'status = 'disabled"' to indicate this. To facilitate this, a CPU/SOC device tree header is supported in arch//dts. The name of this is defined by CONFIG_ARCH_DEVICE_TREE, typically defined in arch//cpu/.../config.mk. You can use the following line within the board device tree file to include this header: /include/ ARCH_CPU_DTS For example, for Tegra2 we might have in arch/arm/cpu/armv7/tegra2/config.mk these lines: CONFIG_ARCH_DEVICE_TREE := tegra20 This means that ARCH_CPU_DTS will point to arch/arm/dts/tegra20.dtsi. This patch set does not include any drivers which actually use the fdt, but there is a basic fdt decode library (fdtdec) to simplify this process. I have provided an example i2c implementation previously: http://patchwork.ozlabs.org/patch/114425/ It is important to understand that the fdt only selects options available in the platform / drivers. It cannot add new drivers (yet). So you must still have the CONFIG option to enable the driver. For example, you need to define CONFIG_SYS_NS16550 to bring in the NS16550 driver, but can use the fdt to specific the UART clock, peripheral address, etc. In very broad terms, the CONFIG options in general control *what* driver files are pulled in, and the fdt controls *how* those files work. While only ARM is supported in this patch series, it should be easy enough to add support for other architectures. I experimented with using dtc's asm output to avoid all the arch/oformat ugliness in dts/Makefile as suggested by Stephen Warren . This simplified the Makefile commands greatly by removing the need to detect the output format and architecture. However Grant Likely explained that this feature is not well tested and still has some big-endian-ims in it. Changes in v2: - Modify Makefile to create combined U-boot/fdt (u-boot-dtb.bin) - Add example proposed decode helper library Changes in v3: - Rename gd->blob to gd->fdt_blob - Move fdt files into board//dts - Provide access to ARCH_CPU_DTS which is the CPU's base device tree - Add fdtcontroladdr environment variable - Simplify decode library to remove provide only primitive functions - Remove i2c decode function - Rename fdt_decode to fdtdec, since it will be used a lot - Moved fdt_decode.c to /lib - Export almost all functions from fdtdec, to allow widespread use - Remove use of FDT_ERR_MISSING which is not strictly needed now Changes in v4: - Fix reference to gd->blob which should be gd->fdt_blob - Remove unused 'clean' targets in dts/Makefile (remmove *.tmp at top-level) - Move documentation into the first 'fdt' patch in the series - Add note about CONFIG_ARCH_DEVICE_TREE - Add assert on sprintf() string length - Rename addr_t to fdt_addr_t to make it more fdt-specific - Remove gpio.h header in fdtdec.c which is not needed yet Simon Glass (6): fdt: ARM: Add device tree control of U-Boot (CONFIG_OF_CONTROL) fdt: Add support for embedded device tree (CONFIG_OF_EMBED)
Re: [U-Boot] [PATCH V2 1/2] mmc: change magic number to macro define
Dear Lei Wen, In message you wrote: > > Thanks for your kindly explaination on the c structure usage in UBOOT. > So should I keep the V2 version of this patch, or anything else need > to be modified? >From my point of view this is OK, so: Acked-by: WOlfgang Denk 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: w...@denx.de There's no honorable way to kill, no gentle way to destroy. There is nothing good in war. Except its ending. -- Abraham Lincoln, "The Savage Curtain", stardate 5906.5 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
Dear Simon Glass, In message you wrote: > > > A general question: is the issue with input as such, or with echoing this > > input? > > The problem is in echoing the input, since this requires the uart to > wait once its transmit FIFO is full. While it is waiting (and not > checking the receive side), it misses characters coming from the other > end. Basically we need to constantly check the receiving side while we > are transmitting. No, we do not. U-Boot is strictly single tasking. This is one of the fundamental design principles. We do not receive network packets while sending ourself, or when doing other things. We do not receive characters on the serial line while sending characters, or when doing other things. This is known behaviour, and you should just adapt to it. I don't know what exactly is your situation when you run into such problems - the recommended approach for example when remote-controlling U-Boot is to wait for the next input prompt before sending the next line of characters - and stop after the newline, until you receive the next prompt. 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: w...@denx.de I believe you find life such a problem because you think there are the good people and the bad people. You're wrong, of course. There are, always and only, the bad people, but some of them are on oppo- site sides. - Terry Pratchett, _Guards! Guards!_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] fdt: check for fdt errors in fdt_create_phandle
On 09/20/2011 07:24 PM, Timur Tabi wrote: > fdt_create_phandle() was ignoring errors from fdt_set_phandle(). If an > error occurs, print an error message and return 0, which is an invalid > phandle. We also need to change the return type for fdt_create_phandle() > to indicate that it cannot return an error code. > > Signed-off-by: Timur Tabi > --- > common/fdt_support.c | 11 +-- > include/fdt_support.h |2 +- > 2 files changed, 10 insertions(+), 3 deletions(-) Added to u-boot-fdt, sent a pull request to wd. Thanks, gvb [snip] ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v6] fdt: Add a do_fixup_by_path_string() function
On 08/30/2011 10:36 PM, Chunhe Lan wrote: > The do_fixup_by_path_string() will set the specified node's property to the > value contained in "status". It would just be an inline wrapper for > do_fixup_by_path() that calls strlen on the argument. > > Signed-off-by: Chunhe Lan Added to u-boot-fdt, sent a pull request to wd. Thanks, gvb [snip] ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] Pull request: u-boot-fdt
The following changes since commit d8fffa057c9430fd0c5104ab6ff7db4cdb03db51: Merge branch 'master' of git://git.denx.de/u-boot-mips (2011-10-12 22:47:15 +0200) are available in the git repository at: git://git.denx.de/u-boot-fdt.git master Chunhe Lan (1): fdt: Add a do_fixup_by_path_string() function Timur Tabi (3): fdt: check for fdt errors in fdt_create_phandle fdt: update fdt_alloc_phandle to use fdt_get_phandle powerpc/85xx: use fdt_create_phandle() to create the Fman firmware phandles arch/powerpc/cpu/mpc85xx/fdt.c |5 ++--- common/fdt_support.c | 18 +++--- include/fdt_support.h |9 - 3 files changed, 21 insertions(+), 11 deletions(-) ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 5/6] fdt: ARM: Add fdtcontroladdr to set device tree address in environment
Hi Mike,
On Thu, Oct 13, 2011 at 2:08 PM, Simon Glass wrote:
> Hi Mike,
>
> On Thu, Oct 13, 2011 at 1:27 PM, Mike Frysinger wrote:
>> On Tuesday 11 October 2011 18:26:10 Simon Glass wrote:
>>> --- a/arch/arm/lib/board.c
>>> +++ b/arch/arm/lib/board.c
>>>
>>> +/**
>>> + * Decode the value of an environment variable and return it.
>>> + *
>>> + * @param name Name of environemnt variable
>>> + * @param base Number base to use (normally 10, or 16 for
>>> hex)
>>> + * @param default_val Default value to return if the variable is not
>>> + * found
>>> + * @return the decoded value, or default_val if not found
>>> + */
>>> +static int getenv_int(const char *name, int base, int default_val)
>>> +{
>>> + char tmp[64]; /* long enough for environment variables */
>>> + int i = getenv_f(name, tmp, sizeof(tmp));
>>> +
>>> + return (i > 0)
>>> + ? (int) simple_strtoul(tmp, NULL, base)
>>> + : default_val;
>>> +}
>>
>> pretty much everyone does this with gd->baudrate. would make sense to put
>> this into common/cmd_nvedit.c and convert all arches.
>> -mike
>>
>
> I was worried someone might say that. I will take a look.
That patch series has been posted, so if it is accepted then we can
remove the definition of this function from the patch series. I am
going to send the fdt series against upstream/master but will happily
adjust this particular patch if getenv_ulong() is accepted.
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 4/6] fdt: ARM: Implement embedded and separate device tree
Hi Stephen, On Thu, Oct 13, 2011 at 2:39 PM, Stephen Warren wrote: > Simon Glass wrote at Tuesday, October 11, 2011 4:26 PM: >> This locates the device tree either embedded within U-Boot or attached to the >> end as a separate binary. >> >> When CONFIG_OF_CONTROL is defined, U-Boot requires a valid fdt. A check is >> provided for this early in initialisation. > > The subject of this patch seems a little misleading; something more like > "fdt: ARM: Check that a control fdt is present" would be more accurate. It does also implement it. Before this, there is no code to find the device tree on ARM. But yes it also checks that it gets something valid. I will change the name to something like 'fdt: ARM: Implement and verify embedded and separate device tree' > > Sorry for the trivial comments! Thanks for looking at it. Regards, Simon > > -- > nvpublic > > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 3/6] fdt: Add support for a separate device tree (CONFIG_OF_SEPARATE)
Hi Stephen, On Thu, Oct 13, 2011 at 2:36 PM, Stephen Warren wrote: > Simon Glass wrote at Tuesday, October 11, 2011 4:26 PM: >> This adds support for an FDT to be build as a separate binary file called >> u-boot.dtb. This can be concatenated with the U-Boot binary to provide a >> device tree located at run-time by U-Boot. The Makefile is modified to >> provide this file in u-boot-dtb.bin. >> >> Signed-off-by: Simon Glass >> --- >> Changes in v2: >> - Modify Makefile to create combined U-boot/fdt (u-boot-dtb.bin) >> - Add note about CONFIG_ARCH_DEVICE_TREE >> >> .gitignore | 1 + >> Makefile | 8 ++ >> README | 16 - >> doc/README.fdt-control | 172 >> >> 4 files changed, 195 insertions(+), 2 deletions(-) >> create mode 100644 doc/README.fdt-control > > Should the documentation be in the first patch; it seems a little random > to include it in one of the two somewhat equivalent patches that add > build support for a DTB transport mechanism. Unfortunately the first patch is ARM-specific so I probably should not put it there (maybe this is just a minor issue, I'm not sure). I have not done patches to enable fdt in all architectures so far and I'm not sure this is wanted / required. I will move the README up one patch to the first non-ARM-specific patch. Regards, Simon > > -- > nvpublic > > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Checkpatch warnings for "volatile"
Hi Wolfgang On 10/15/11, Wolfgang Denk wrote: > Dear Prabhakar Lad, > > In message > you > wrote: >> >> > I've explained this a number of times recently - there are actually >> > very, very few occasions where "volatile" actually makes sense. >> > >> > Agreed, but I see a piece of code where virtual address are >> > compared. >> For example in arch/arm/cpu/arm926ejs/davinci/cpu.c >> In this function static inline unsigned pll_prediv(unsigned pllbase) >> and >> also in this static inline unsigned pll_postdiv(unsigned pllbase) >> >> Any suggestion on this on how to tackle or let it remain stagnant? > > I cannot see a justification for any of the ""volatile" in this file. > > Of course, all these ugly REG() calls should be converted to proper > use of I/O accessors. > Thanks for the reply. Regards --Prabhakar Lad > 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: w...@denx.de > Hindsight is an exact science. > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/6] fdt: Add support for embedded device tree (CONFIG_OF_EMBED)
Hi Grant,
On Fri, Oct 14, 2011 at 10:46 PM, Grant Likely
wrote:
> On Thu, Oct 13, 2011 at 3:50 PM, Stephen Warren wrote:
>> Simon Glass wrote at Thursday, October 13, 2011 3:25 PM:
>>> Hi Stephen,
>>>
>>> On Thu, Oct 13, 2011 at 2:21 PM, Stephen Warren wrote:
>>> > Simon Glass wrote at Tuesday, October 11, 2011 4:26 PM:
>>> >> This new option allows U-Boot to embed a binary device tree into its
>>> >> image
>>> >> to allow run-time control of peripherals. This device tree is for
>>> >> U-Boot's
>>> >> own use and is not necessarily the same one as is passed to the kernel.
>>> >>
>>> >> The device tree compiler output should be placed in the $(obj)
>>> >> rooted tree. Since $(OBJCOPY) insists on adding the path to the
>>> >> generated symbol names, to ensure consistency it should be
>>> >> invoked from the directory where the .dtb file is located and
>>> >> given the input file name without the path.
>>> > ...
>>> >> +process_lds = \
>>> >> + $(1) | sed -r -n 's/^OUTPUT_$(2)[ ("]*([^")]*).*/\1/p'
>>> >> +
>>> >> +# Run the compiler and get the link script from the linker
>>> >> +GET_LDS = $(CC) $(CFLAGS) $(LDFLAGS) -Wl,--verbose 2>&1
>>> >> +
>>> >> +$(obj)dt.o: $(DT_BIN)
>>> >> + # We want the output format and arch.
>>> >> + # We also hope to win a prize for ugliest Makefile / shell
>>> >> interaction
>>> >> + # We look in the LDSCRIPT first.
>>> >> + # Then try the linker which should give us the answer.
>>> >> + # Then check it worked.
>>> >> + oformat=`$(call process_lds,cat $(LDSCRIPT),FORMAT)` ;\
>>> >> + oarch=`$(call process_lds,cat $(LDSCRIPT),ARCH)` ;\
>>> >> + \
>>> >> + [ -z $${oformat} ] && \
>>> >> + oformat=`$(call process_lds,$(GET_LDS),FORMAT)` ;\
>>> >> + [ -z $${oarch} ] && \
>>> >> + oarch=`$(call process_lds,$(GET_LDS),ARCH)` ;\
>>> >> + \
>>> >> + [ -z $${oformat} ] && \
>>> >> + echo "Cannot read OUTPUT_FORMAT from lds file $(LDSCRIPT)"
>>> >> && \
>>> >> + exit 1 || true ;\
>>> >> + [ -z $${oarch} ] && \
>>> >> + echo "Cannot read OUTPUT_ARCH from lds file $(LDSCRIPT)"
>>> >> && \
>>> >> + exit 1 || true ;\
>>> >> + \
>>> >> + cd $(dir ${DT_BIN}) && \
>>> >> + $(OBJCOPY) -I binary -O $${oformat} -B $${oarch} \
>>> >> + $(notdir ${DT_BIN}) $@
>>> >> + rm $(DT_BIN)
>>> >
>>> > Instead of all that, can't you just run a trivial script to generate a .c
>>> > file containing the data from DTB_BIN, and then use the compiler to
>>> > compile
>>> > that, i.e. spit out something like:
>>> >
>>> > const unsigned char dtb[] = {
>>> > 0xaa, 0x55, ..
>>> > };
>>> >
>>> > That'd certainly drastically simplify the makefile, although waste a
>>> > little
>>> > more time and temp disk space.
>>>
>>> What, and withdraw my Makefile contest entry? :-)
>>
>> :-)
>>
>>> I feel that objcopy is designed to do exactly this, and generating C
>>> code is a roundabout way of producing an object file with data in it.
>>> The difficulty of finding out the output format/architecture is
>>> something we might clean up in U-Boot generally at some point (e.g.
>>> figure it out as part of the original 'make ..._config') in which case
>>> this would all go away.
>>>
>>> Thoughts?
>>
>> Looking some more, dtc has option "-O asm" which writes directly to a text
>> file that can be assembled; there'd be no extra temp files or conversions
>> if you used that.
>
> I recommend *not* using the asm output option. It's not nearly as
> well tested and it is likely to have some big-endian-isms in it that
> don't work well. I prefer the objcopy approach myself. That's what
> it is for.
Oh dear that sounds bad. The difficultly is in figuring out the
arguments to objcopy (architecture and oformat). But we do need it to
work reliably, so I will move back to the original approach for now.
Regards,
Simon
>
> g.
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] NS16550: buffer reads
Hi Albert,
On Sat, Oct 15, 2011 at 3:43 AM, Albert ARIBAUD
wrote:
> Hi Simon,
>
> Le 15/10/2011 02:03, Simon Glass a écrit :
>>
>> From: Scott Wood
>>
>> From: Scott Wood
>>
>> This improves the performance of U-Boot when accepting rapid input,
>> such as pasting a sequence of commands.
>>
>> Without this patch, on P4080DS I see a maximum of around 5 lines can
>> be pasted. With this patch, it handles around 70 lines before lossage,
>> long enough for most things you'd paste.
>>
>> With it enabled, on ppc it's an extra 396 bytes of image size, and 1056
>> bytes of BSS.
>>
>> ARM note from Simon Glass - ARM code size goes from
>> 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
>>
>> Signed-off-by: Scott Wood
>> ---
>> Changes in v2:
>> - Fix checkpatch warnings, the other one was already there
>
> A general question: is the issue with input as such, or with echoing this
> input?
The problem is in echoing the input, since this requires the uart to
wait once its transmit FIFO is full. While it is waiting (and not
checking the receive side), it misses characters coming from the other
end. Basically we need to constantly check the receiving side while we
are transmitting.
>
>> README | 8
>> drivers/serial/ns16550.c | 96
>> +++--
>> drivers/serial/serial.c | 5 +-
>> include/ns16550.h | 4 +-
>> 4 files changed, 104 insertions(+), 9 deletions(-)
>>
>> diff --git a/README b/README
>> index 7e032a9..28d2e8a 100644
>> --- a/README
>> +++ b/README
>> @@ -2862,6 +2862,14 @@ use the "saveenv" command to store a valid
>> environment.
>> space for already greatly restricted images, including but
>> not
>> limited to NAND_SPL configurations.
>>
>> +- CONFIG_NS16550_BUFFER_READS:
>> + Instead of reading directly from the receive register
>> + every time U-Boot is ready for another byte, keep a
>> + buffer and fill it from the hardware fifo every time
>> + U-Boot reads a character. This helps U-Boot keep up with
>> + a larger amount of rapid input, such as happens when
>> + pasting text into the terminal.
>> +
>> Low Level (hardware related) configuration options:
>> ---
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 0174744..a012e5d 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -4,12 +4,15 @@
>> * modified to use CONFIG_SYS_ISA_MEM and new defines
>> */
>>
>> +#include
>> #include
>> #include
>> #include
>> #include
>> #include
>>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> #define UART_LCRVAL UART_LCR_8N1 /* 8 data, 1 stop, no
>> parity */
>> #define UART_MCRVAL (UART_MCR_DTR | \
>> UART_MCR_RTS) /* RTS/DTR */
>> @@ -92,21 +95,104 @@ void NS16550_putc (NS16550_t com_port, char c)
>> }
>>
>> #ifndef CONFIG_NS16550_MIN_FUNCTIONS
>> -char NS16550_getc (NS16550_t com_port)
>> +
>> +static char NS16550_raw_getc(NS16550_t regs)
>> {
>> - while ((serial_in(&com_port->lsr)& UART_LSR_DR) == 0) {
>> + while ((serial_in(®s->lsr)& UART_LSR_DR) == 0) {
>> #ifdef CONFIG_USB_TTY
>> extern void usbtty_poll(void);
>> usbtty_poll();
>> #endif
>> WATCHDOG_RESET();
>> }
>> - return serial_in(&com_port->rbr);
>> + return serial_in(®s->rbr);
>> +}
>> +
>> +static int NS16550_raw_tstc(NS16550_t regs)
>> +{
>> + return ((serial_in(®s->lsr)& UART_LSR_DR) != 0);
>> +}
>> +
>> +
>> +#ifdef CONFIG_NS16550_BUFFER_READS
>> +
>> +#define BUF_SIZE 256
>
> I guess this define conditions how well U-Boot will cope with steady
> high-rate input, correct? If so, I'd like it to be a configuration option --
> actually, one could combine enabling the option *and* setting the expected
> buffer size.
Yes so you could have something like:
#define CONFIG_NS16550_BUFFER_READS 256
>
>> +#define NUM_PORTS 4
>> +
>> +struct ns16550_priv {
>> + char buf[BUF_SIZE];
>> + unsigned int head, tail;
>> +};
>> +
>> +static struct ns16550_priv rxstate[NUM_PORTS];
>> +
>> +static void enqueue(unsigned int port, char ch)
>> +{
>> + /* If queue is full, drop the character. */
>> + if ((rxstate[port].head - rxstate[port].tail - 1) % BUF_SIZE == 0)
>> + return;
>> +
>> + rxstate[port].buf[rxstate[port].tail] = ch;
>> + rxstate[port].tail = (rxstate[port].tail + 1) % BUF_SIZE;
>> +}
>> +
>> +static int dequeue(unsigned int port, char *ch)
>> +{
>> + /* Empty queue? */
>> + if (rxstate[port].head == rxstate[port].tail)
>> + return 0;
>> +
>> + *ch = rxstate[port].buf[rxstate[port].head];
>> + rxstate[port].head = (rxstate[port].head + 1) % BUF_SIZE;
>> + return 1;
>> +}
>> +
>> +static void fill_rx_buf(NS16550_t regs, unsigned
Re: [U-Boot] [PATCH V2 1/2] mmc: change magic number to macro define
Hi Wolfgang,
On Sat, Oct 15, 2011 at 4:36 AM, Wolfgang Denk wrote:
> Dear Lei Wen,
>
> In message
> you
> wrote:
>>
>> > And both the "index" and "value" arguments are never used in I/O
>> > context, i. e. they are actual plain integer parameters. So just keep
>> > it as
>> >
>> > err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);
>>
>> Should I also keep the EXT_CSD_HS_TIMING like macro for previous
>> ext_csd parsing?
>> Or just add another ext_csd structure definition to the parse the
>> ext_csd, while keep macros
>> to be called by mmc_switch?
>
> Let's try to understand what we are trying to acchieve. The purpose
> of using C structs instead of a notation of "base address + register
> offset" is that this way the struct entries that represent the
> registers now have types, and the compiler can perform proper type
> checking when using these data in I/O accessors.
>
> So we use structs to describe the "memory map" of the hardware, the
> mapping of device registers to addresses, and the data types give
> information about the width of the register (8, 16, 32, ... bit).
>
> Note that all the time we are talking about device registers and I/O
> operations - that is situations where I/O accessors will be used.
>
>
> On the other hand, when it comes to definitions of bit fields, like
> here:
>
> /*
> * EXT_CSD field definitions
> */
>
> #define EXT_CSD_CMD_SET_NORMAL (1 << 0)
> #define EXT_CSD_CMD_SET_SECURE (1 << 1)
> #define EXT_CSD_CMD_SET_CPSECURE (1 << 2)
> ...
>
> or of indexes, like here:
>
> /*
> * EXT_CSD fields
> */
>
> #define EXT_CSD_PART_CONF 179 /* R/W */
> #define EXT_CSD_BUS_WIDTH 183 /* R/W */
> #define EXT_CSD_HS_TIMING 185 /* R/W */
> ...
>
> ..then a struct makes no sense - we have plain integer constants
> here.
>
>
> To verify, please have a look at the code of mmc_switch():
>
> int mmc_switch(struct mmc *mmc, u8 set, u8 index, u8 value)
> {
> struct mmc_cmd cmd;
> int timeout = 1000;
> int ret;
>
> cmd.cmdidx = MMC_CMD_SWITCH;
> cmd.resp_type = MMC_RSP_R1b;
> cmd.cmdarg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
> (index << 16) |
> (value << 8);
> ...
> ret = mmc_send_cmd(mmc, &cmd, NULL);
>
>
> As you can see, the arguments are ORed together to form an argument
> to mmc_send_cmd() - they are not used in a I/O accessor or any
> similar function.
>
>
> In short: neither EXT_CSD_CMD_SET_NORMAL nor EXT_CSD_HS_TIMING
> describe a device register that is used in any form of I/O
> operations, so it is OK to leave these as simple #define's; the use
> of a struct would not make sense here.
>
Thanks for your kindly explaination on the c structure usage in UBOOT.
So should I keep the V2 version of this patch, or anything else need
to be modified?
Thanks,
Lei
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [RFC] general ULPI support
Hi all, my patch, which added USB support for Efika, needed som ULPI functions. I was suggested to write general ULPI support code rather than implenent demanded functionality in the driver. However, I've encountered following problems: 1. Where should I put this code? Linux kernel has it in ./drivers/usb/otg/ together with OTG utility code. Use this name looks a bit deceiving for me. Should I create some directory named like ./drivers/usb/ulpi/, or is there better place? 2. I don't know what to add to the Makefile. Should I create a new config option (named CONFIG_ULPI I suppose) or simply add rule for new files? Regards, Jana Rapava ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] fdt: Add new fdt_set_node_status & fdt_set_status_by_alias helpers
Hi Kumar, Shengzhou, On 10/14/2011 03:10 PM, Kumar Gala wrote: > From: Shengzhou Liu > > Add common function fdt_set_node_status() to assist in various locations > that we set a nodes status. This function utilizes the status values > that are part of the EPAPR spec (on power.org). > > fdt_set_status_by_alias() is based on fdt_set_node_status() but uses an > alias string to identify the node to update. > > We also add some shortcut functions to help the common cases of setting > "okay" and "disabled": > > fdt_status_okay() > fdt_status_disabled() > fdt_status_okay_by_alias() > fdt_status_disabled_by_alias() > > Finally, we fixup the corenet_ds ethernet code which previously had > a function by the same name that can be replaced with the new helpers. > > Signed-off-by: Shengzhou Liu > Signed-off-by: Kumar Gala > --- > board/freescale/corenet_ds/eth_hydra.c | 26 ++ > board/freescale/corenet_ds/eth_p4080.c | 36 +-- > common/fdt_support.c | 60 > +++- > include/fdt_support.h | 28 +++ > 4 files changed, 101 insertions(+), 49 deletions(-) While this touches fdt_support.[ch] which is nominally "mine", it is coupled with the Freescale support. I could make you break it into two, but then I would have to do more work and you (and wd) would have to make sure they went into u-boot in the right order. That sounds ugly, so I would prefer Kumar apply it to his subrepo to feed into u-boot. Acked-by: Gerald Van Baren Best regards, gvb [snip] ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2 5/8] ARM: moved general function to arm/lib
On 10/15/2011 12:15 PM, Albert ARIBAUD wrote: > Hi Stefano, > Hi Albert, > > I agree with Wofgang that sdelay() is redundant wrt udelay() and has > weaker semantics. > > I'll add that sr32() is kind of not ARM specific, so I fail to see why > it should move to generic ARM, and besides, it is a half-baked solution > to the general problem of setting a bitfield in a register Right, it is. > > I conclude like Wolfgang that despite this code having gone through > review unnoticed, it should not be promoted for wider use. Understood. > > If some functionality of these three functions is desirable and cannot > be met with other means, please submit new code for general ARM > inclusion (and maybe consider adapting armv7 to this new code). I understand your points - and I agree with you, thanks for clarification. Stefano -- = DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de = ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2 3/3] mx31: Add board support for HALE TT-01
On 10/15/2011 10:52 AM, Wolfgang Denk wrote: > - if we do something like that, we should discuss it > first, and put it in context with the (Not yet complete) discussion > about the new Timer API. Agree, you're right - this should be part of the Timer API > I definitely do not want to see this function spread uncontrolled and > undiscussed. If we find it's useful, it should be made really > general. Understood, you convinced me ;-). Best regards, Stefano Babic -- = DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de = ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
