[U-Boot] [PATCH v2] patman: make run results better visible
For an occasional user of patman some failures are not obvious: for instance when checkpatch reports warnings, the dry run still reports that the email would be sent. If it is not dry run, the warnings are shown on the screen, but it is not clear that the email was not sent. Add some code to report failure to send email explicitly. Tested by running the script on a patch with style violations, observed error messages in the script output. Signed-off-by: Vadim Bendebury vben...@chromium.org --- Changes in v2: - modified the error message for accuracy tools/patman/patman.py | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tools/patman/patman.py b/tools/patman/patman.py index c60aa5a..86e8e63 100755 --- a/tools/patman/patman.py +++ b/tools/patman/patman.py @@ -154,13 +154,18 @@ else: # Email the patches out (giving the user time to check / cancel) cmd = '' -if ok or options.ignore_errors: +its_a_go = ok or options.ignore_errors +if its_a_go: cmd = gitutil.EmailPatches(series, cover_fname, args, options.dry_run, not options.ignore_bad_tags, cc_file, in_reply_to=options.in_reply_to) +else: +print col.Color(col.RED, Not sending emails due to errors/warnings) # For a dry run, just show our actions as a sanity check if options.dry_run: series.ShowActions(args, cmd, options.process_tags) +if not its_a_go: +print col.Color(col.RED, Email would not be sent) os.remove(cc_file) -- 2.1.0.rc2.206.gedb03e5 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] patman: make run results better visible
For an occasional user of patman some failures are not obvious: for instance when checkpatch reports warnings, the dry run still reports that the email would be sent. If it is not dry run, the warnings are shown on the screen, but it is not clear that the email was not sent. Add some code to report failure to send email explicitly. Tested by running the script on a patch with style violations, observed error messages in the script output. Signed-off-by: Vadim Bendebury vben...@chromium.org --- tools/patman/patman.py | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tools/patman/patman.py b/tools/patman/patman.py index c60aa5a..0163ccd 100755 --- a/tools/patman/patman.py +++ b/tools/patman/patman.py @@ -154,13 +154,19 @@ else: # Email the patches out (giving the user time to check / cancel) cmd = '' -if ok or options.ignore_errors: +its_a_go = ok or options.ignore_errors +if its_a_go: cmd = gitutil.EmailPatches(series, cover_fname, args, options.dry_run, not options.ignore_bad_tags, cc_file, in_reply_to=options.in_reply_to) +else: +print col.Color(col.RED, +Not sending emails due to checkpatch errors/warnings) # For a dry run, just show our actions as a sanity check if options.dry_run: series.ShowActions(args, cmd, options.process_tags) +if not its_a_go: +print col.Color(col.RED, Email would not be sent) os.remove(cc_file) -- 2.1.0.rc2.206.gedb03e5 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] patman: make run results better visible
On Wed, Sep 3, 2014 at 3:14 PM, Doug Anderson diand...@chromium.org wrote: Vadim, On Wed, Sep 3, 2014 at 12:16 PM, Vadim Bendebury vben...@chromium.org wrote: For an occasional user of patman some failures are not obvious: for instance when checkpatch reports warnings, the dry run still reports that the email would be sent. If it is not dry run, the warnings are shown on the screen, but it is not clear that the email was not sent. Add some code to report failure to send email explicitly. Tested by running the script on a patch with style violations, observed error messages in the script output. Signed-off-by: Vadim Bendebury vben...@chromium.org --- tools/patman/patman.py | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tools/patman/patman.py b/tools/patman/patman.py index c60aa5a..0163ccd 100755 --- a/tools/patman/patman.py +++ b/tools/patman/patman.py @@ -154,13 +154,19 @@ else: # Email the patches out (giving the user time to check / cancel) cmd = '' -if ok or options.ignore_errors: +its_a_go = ok or options.ignore_errors +if its_a_go: cmd = gitutil.EmailPatches(series, cover_fname, args, options.dry_run, not options.ignore_bad_tags, cc_file, in_reply_to=options.in_reply_to) +else: +print col.Color(col.RED, +Not sending emails due to checkpatch errors/warnings) Technically it could be due to other problems, too (like errors applying). good point, what wording would you suggest? --vb # For a dry run, just show our actions as a sanity check if options.dry_run: series.ShowActions(args, cmd, options.process_tags) +if not its_a_go: +print col.Color(col.RED, Email would not be sent) os.remove(cc_file) Looks good to me, other than that. -Doug ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] x86: coreboot: delete unused coreboot/config.mk
On Tue, Feb 25, 2014 at 3:43 PM, Simon Glass s...@chromium.org wrote: Hi Masahiro, On 24 February 2014 05:36, Masahiro Yamada yamad...@jp.panasonic.com wrote: HOSTCFLAGS_autoconf.mk.dep was added by commit 422322f but it has never been used. Cc: Vadim Bendebury vben...@chromium.org Cc: Simon Glass s...@chromium.org Signed-off-by: Masahiro Yamada yamad...@jp.panasonic.com --- Hello Vadim, Simon. I cannot understand what the hell this macro is used for. Why HOSTCFLAGS_...? Why not CFLAGS_... ? Can you explain? No I can't. I don't think this makes any sense. this all is ancient history by now, but I think this made sense at the time, it was affecting compilation options for coreboot. --vb Acked-by: Simon Glass s...@chromium.org Regards, Simon board/chromebook-x86/coreboot/config.mk | 7 --- 1 file changed, 7 deletions(-) delete mode 100644 board/chromebook-x86/coreboot/config.mk diff --git a/board/chromebook-x86/coreboot/config.mk b/board/chromebook-x86/coreboot/config.mk deleted file mode 100644 index 0c05dd0..000 --- a/board/chromebook-x86/coreboot/config.mk +++ /dev/null @@ -1,7 +0,0 @@ -# -# Copyright (c) 2011 The Chromium OS Authors. All rights reserved. -# -# SPDX-License-Identifier: GPL-2.0 BSD-3-Clause -# - -HOSTCFLAGS_autoconf.mk.dep = -Wno-variadic-macros -- 1.8.3.2 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] u-boot gerrit server
On Tue, Nov 12, 2013 at 10:07 AM, Otavio Salvador ota...@ossystems.com.br wrote: On Tue, Nov 12, 2013 at 3:30 PM, Albert ARIBAUD albert.u.b...@aribaud.net wrote: Hi Otavio, On Tue, 12 Nov 2013 15:16:15 -0200, Otavio Salvador ota...@ossystems.com.br wrote: Hello Albert, On Tue, Nov 12, 2013 at 3:13 PM, Albert ARIBAUD albert.u.b...@aribaud.net wrote: On Tue, 12 Nov 2013 15:00:06 -0200, Otavio Salvador ota...@ossystems.com.br wrote: On Tue, Nov 12, 2013 at 2:55 PM, Vadim Bendebury (вб) vben...@google.com wrote: On Tue, Nov 12, 2013 at 8:47 AM, Otavio Salvador ota...@ossystems.com.br wrote: Besides, how people will 'transfer' one patch from one tree to another? This will happen quite often as someone mistakenly sending a patch for the wrong tree or custodians wanting the set to go together in a single merge... How is it handled today? Gerrit is after all just a means of keeping track of patches in a more efficient way, the rest could be similar to what is in use now, or enhanced using gerrit's features. Currently it is just reassigned in Patchwork; using multiple trees will complicate this. How about one branch per custodian? At my previous job we had a couple branches, one per distinct product. I am not aware of a way to 'transfer' a patch from one branch to another. There would not be such transfers -- we don't do this right now with our trees. We do merge requests, which means pulling two custodian repos into the same working repo, doing a merge between what are now two custodian branches, and pushing the result back onto one of the custodian trees. So here, if there is one branch per custodian, what we would need is the ability to merge one custodian branch into another. Right but currently you are not 'denied' to review a patch someone didn't put for you. The custodians as done 'on-the-fly' as each custodian takes his duties and process the patches and apply them, later updating patchwork. On the 'new Gerrit' workflow, if it is assigned for a branch/tree and this is mistakenly done, how it will be done? the patch can always be abandoned on gerrit, so no problem there. As for moving it to the right tree - either the original submitter could resubmit it, or the custodian could cherry pick it, does not seem to be the problem either way. --v -- Otavio Salvador O.S. Systems http://www.ossystems.com.brhttp://code.ossystems.com.br Mobile: +55 (53) 9981-7854Mobile: +1 (347) 903-9750 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2 0/2] add sdhci generic framework
Hello Lei, I am looking into adding eMMC support for some intel SDHCI controllers based on this common SDHCI driver submission. One thing which is not quite clear is the quirks - can you (or somebody else on this list) please shed some light on what the quirks are. Is there some kind of formal description/definition of various defined quirks and their processing. Thank you in advance, --vb On Wed, Jun 29, 2011 at 12:50 AM, Lei Wen lei...@marvell.com wrote: V1: add sdhci generic framework and with marvell sdhci implementation V2: rename the previous file name from sdhci-mv to mv_sdhci Lei Wen (2): MMC: add sdhci generic framework MMC: add marvell sdhci driver drivers/mmc/Makefile |2 + drivers/mmc/mv_sdhci.c | 21 +++ drivers/mmc/sdhci.c| 433 include/sdhci.h| 325 4 files changed, 781 insertions(+), 0 deletions(-) create mode 100644 drivers/mmc/mv_sdhci.c create mode 100644 drivers/mmc/sdhci.c create mode 100644 include/sdhci.h ___ 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 V2 0/2] add sdhci generic framework
On Mon, Oct 14, 2013 at 7:16 PM, Lei Wen lei...@marvell.com wrote: Hi Vadim, The quirk is standing for some workaround for those host that has some limitation that it cannot directly be applied with standard sdhci.c code, so that we add one quirk for it, which let it could use the sdhci.c. Like the quirk as SDHCI_QUIRK_32BIT_DMA_ADDR, it indicate that some host has dma engine limitation that it must start from address with alignement. So if sdhci find caller set this quirk, it would follow corresponding workaround for it. There is no formal description for it as I could see. All right, that's what I figured :). Thank you for the explanation, --vb Thanks, Lei -Original Message- From: vben...@gmail.com [mailto:vben...@gmail.com] On Behalf Of Vadim Bendebury Sent: Tuesday, October 15, 2013 10:09 AM To: Lei Wen Cc: Prafulla Wadaskar; aflem...@gmail.com; Rob Herring; u- b...@lists.denx.de; Yu Tang; Prabhanjan Sarnaik; Ashish Karkare; Andy Fleming Subject: Re: [U-Boot] [PATCH V2 0/2] add sdhci generic framework Hello Lei, I am looking into adding eMMC support for some intel SDHCI controllers based on this common SDHCI driver submission. One thing which is not quite clear is the quirks - can you (or somebody else on this list) please shed some light on what the quirks are. Is there some kind of formal description/definition of various defined quirks and their processing. Thank you in advance, --vb On Wed, Jun 29, 2011 at 12:50 AM, Lei Wen lei...@marvell.com wrote: V1: add sdhci generic framework and with marvell sdhci implementation V2: rename the previous file name from sdhci-mv to mv_sdhci Lei Wen (2): MMC: add sdhci generic framework MMC: add marvell sdhci driver drivers/mmc/Makefile |2 + drivers/mmc/mv_sdhci.c | 21 +++ drivers/mmc/sdhci.c| 433 include/sdhci.h| 325 4 files changed, 781 insertions(+), 0 deletions(-) create mode 100644 drivers/mmc/mv_sdhci.c create mode 100644 drivers/mmc/sdhci.c create mode 100644 include/sdhci.h ___ 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Compiling certain files with extra compiler flags
On Wed, Sep 4, 2013 at 8:16 PM, Simon Glass s...@google.com wrote: Hi Vadim, OK I see. You can do something like this: CFLAGS_arch/arm/cpu/tegra20-common/warmboot_avp.o += -march=armv4t (this is used by Tegra) Regards, Simon Ah, this works, thank you, Simon! --vb On Wed, Sep 4, 2013 at 8:25 PM, Vadim Bendebury vben...@chromium.org wrote: this is the tweak -- Forwarded message -- From: Vadim Bendebury vben...@chromium.org Date: Wed, Sep 4, 2013 at 11:49 AM Subject: Compiling certain files with extra compiler flags To: uboot u-boot@lists.denx.de Does u-boot provide the ability to change compilation options 'on the fly', say when certain files need to be compiled for symbolic debugging? I looked but did not find any. How about something like this: vvv $ git diff config.mk diff --git a/config.mk b/config.mk index 853e6d8..41cafbb 100644 --- a/config.mk +++ b/config.mk @@ -406,7 +406,7 @@ $(obj)%.o: %.c ifneq ($(CHECKSRC),0) $(CHECK) $(CHECKFLAGS) $(ALL_CFLAGS) $ endif - $(CC) $(ALL_CFLAGS) -o $@ $ -c + $(CC) $(ALL_CFLAGS) $(CPP_$(subst .,_,$)) -o $@ $ -c $(obj)%.i: %.c $(CPP) $(ALL_CFLAGS) -o $@ $ -c $(obj)%.s: %.c ^ Then, say I want to have spl_boot.c compiled with extra flags, just invoking make like this does the trick: CPP_spl_boot_c='-O0 -fno-default-inline' make ... Or is there a better way? --vb ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] Compiling certain files with extra compiler flags
Does u-boot provide the ability to change compilation options 'on the fly', say when certain files need to be compiled for symbolic debugging? I looked but did not find any. How about something like this: vvv $ git diff config.mk diff --git a/config.mk b/config.mk index 853e6d8..41cafbb 100644 --- a/config.mk +++ b/config.mk @@ -406,7 +406,7 @@ $(obj)%.o: %.c ifneq ($(CHECKSRC),0) $(CHECK) $(CHECKFLAGS) $(ALL_CFLAGS) $ endif - $(CC) $(ALL_CFLAGS) -o $@ $ -c + $(CC) $(ALL_CFLAGS) $(CPP_$(subst .,_,$)) -o $@ $ -c $(obj)%.i: %.c $(CPP) $(ALL_CFLAGS) -o $@ $ -c $(obj)%.s: %.c ^ Then, say I want to have spl_boot.c compiled with extra flags, just invoking make like this does the trick: CPP_spl_boot_c='-O0 -fno-default-inline' make ... Or is there a better way? --vb ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] fdt_support: Use CONFIG_NR_DRAM_BANKS if defined
On Tue, Apr 30, 2013 at 2:14 PM, Tom Rini tr...@ti.com wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 04/30/2013 04:49 PM, Doug Anderson wrote: Tom, On Tue, Apr 30, 2013 at 1:35 PM, Tom Rini tr...@ti.com wrote: And I guess having this knowledge correct for the kernel is useful in other contexts like when we want to power down some banks of memory but not others? I mean, there's lots of platforms that lie and say 1 bank since we require contiguous mapping. Thanks! Thanks for the review! At the moment I'm _not_ convinced that there's a good reason to specify 8 banks. We appear to have lied and said 1 bank on exynos5250-snow (ARM Chromebook) and I don't know of any bad side effects. The code I'm looking at right now indicates 8 banks. We need to track down why someone did that but it doesn't seem totally crazy to allow specifying the proper number of banks so I figured I'd send this patch up. If you prefer, we can leave this patch hanging until we actually track down if specifying 8 banks was really needed. Yes please, lets hold. Thanks! I looked into this a bit more, what happens on this particular target (Exynos5420 with 4GB DRAM onboard) is that out of 4GB of memory only 3.5GB is usable, as the lower .5 GB of address range is taken by the architecture, and the address bus width is 32 bits. U-boot code makes several assumptions: - bank size is a power of 2 - bank base is aligned with bank size - all bank sizes are the same with this in mind, the only way to describe our memory situation is to define 7 banks, .5GB each, the lowest one starting at 0x2000 (.5GB). This is not a big deal for u-boot (maybe very marginally inefficient when determining the actual memory size). Is this a big deal for kernel? I mean it is easy to squash these seven memory banks into one when filling out the memory node of the device tree, the question is is it even necessary? cheers --vb - -- Tom -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRgDQoAAoJENk4IS6UOR1WztkP+QEs7IvExh9Dq0AHrj81wQ9Q Ml29BZGsdJ5mLIt6jhJ7HSr310cu3FODgbVuNt01Aj0Q2X+C1mCRYqhoIDwfcSUJ EWVhUaphlmiBd2OrMH+3HPUwQ+kFfjt5LNFuXwRei0tgz+sy6NTQ+QZFuZ9FiBJD UKtavOsvd3XipdklU5UEGoBj6OJxU6hBOyehZ3Cckwgfeg0L/1uV07Vd8kSFFc5e xoWXN7O+QkdlNkWeruxPF7uq1MeM2VusCuvGWK4srrED+WSAKFhqsi7t3N66iNny lXDhYPtuSr5HF5xua4kwWdbM/GneVd5m0p979TvIwvwhM1bMr00mfIoH9HEjzNF6 Bvq0wcCwIEZLwBFNNpn9X9zIzwXIgUKbMqjHQXiuizY8LROdXXnkg53k9o2pDO5+ uGO8cKZMXJYEU4zW+wbSlI/Cz7WoylsXhSBPfF5gkRSIxKtYmcS/iQn/nKMgebVO TaGx76/r8xOvA5WY+wCs7HMEJip5UU00rG7MvjokwxOSUf/2rVHiDWl0MEAlh7M4 4KAMzb61P/fUiXrZv5K9Z6sgPmGynjItKnw0UigTWKG6DvRy0HuOlF//O8qAuWKH +eyjg2F24pS9cGRMni3M9cUBH1W6secIpZkqs3goxeNVZyfb29kswolymfbcU4GC zXmnz8gBTLDKGtTzLlXC =s42z -END PGP SIGNATURE- ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
On Tue, May 14, 2013 at 3:15 PM, Tom Rini tr...@ti.com wrote: On Tue, May 14, 2013 at 02:27:32PM -0700, Simon Glass wrote: Hi Tom, On Tue, May 14, 2013 at 2:21 PM, Tom Rini tr...@ti.com wrote: On Mon, May 13, 2013 at 03:12:07PM -0700, Simon Glass wrote: Hi Tom, On Thu, Mar 21, 2013 at 7:38 AM, Tom Rini tr...@ti.com wrote: On Tue, Feb 26, 2013 at 08:10:53AM -0800, Simon Glass wrote: Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes different boards compile different versions of the source code, meaning that we must build all boards to check for failures. It is easy to misspell an #ifdef and there is not as much checking of this by the compiler. Multiple dependent #ifdefs are harder to do than with if..then..else. Variable declarations must be #idefed as well as the code that uses them, often much later in the file/function. #ifdef indents don't match code indents and have their own separate indent feature. Overall, excessive use of #idef hurts readability and makes the code harder to modify and refactor. For people coming newly into the code base, #ifdefs can be a big barrier. The use of #ifdef in U-Boot has possibly got a little out of hand. In an attempt to turn the tide, this series includes a patch which provides a way to make CONFIG macros available to C code without using the preprocessor. This makes it possible to use standard C conditional features such as if/then instead of #ifdef. A README update exhorts compliance. OK, this is true. Looking over the series, a number of the patches are just general fixes / improvements that don't depend on the autoconf_... work. Lets split this out now and take them in now as they seem like reviewable by inspection code. Well sorry I didn't make time to get this done last time. I can do this now or... For the approach itself, I'm not sure which is best here. In a lot of cases we're trading an #ifdef for adding a level of indent to already pretty indented code and that feels like a wash, in terms of readability to me. We probably need to re-factor some of the code in question there too for readability, then see about using autoconf_... type things, or maybe something else. I think you are saying to do the rearrangement and clean-up first, then add autoconf afterwards. I can do that but really I am wondering what you think of the autoconf concept? The Kconfig stuff is related here too, but first I would like to decide on what to do with the #ifdefs. I think a lot of our #ifdefery is a problem of code that's in need of some love and re-org and cleaning and updating. One of the old style rules I still try and follow is that after a few levels of indent code doesn't read well. Also big nested #ifdefs don't read well. So we're trading one in for the other. But your series showed a lot of places where we can re-factor things to improve readability. So lets go that way. Then we can see if there's still things to improve on, and what dead code we still have around. So are you saying that you are keen on the autoconf idea? I'm saying lets clean up the code and see if we still need something like autoconf. It seems to provide the most benefit in terms of readability in places that could read a lot better with some clean up and refactoring before we add new tools to the pile. Yet another great advantage of autoconf is that it ensures a consistent combination of the configuration options, with the status quo it is so easy to make a mistake and create a deficient configuration. --vb -- Tom ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] EXYNOS: SPI: Support SPI_PREAMBLE mode
On Mon, May 6, 2013 at 7:37 AM, Simon Glass s...@chromium.org wrote: HI Vadim, On Thu, May 2, 2013 at 11:12 PM, Vadim Bendebury vben...@chromium.org wrote: [the original patch removed, re-sending from a registered address] So, I spent some more time debugging a system which requires this patch: a system, where on a SPI interface a response to a command could come way later then the command data transmission completes. The original patch was trying to address many corner cases, but come to think of it, in this situation the slave does not care about extra data sent on the transmit interface, as otherwise there is no clock and no data could be transferred from the slave. Then, for this SPI interface we do not need to set the counter of clocks, and do not need to keep adding more clocks if the data has not been received yet, the clocks could be just free running. And then the patch becomes much simpler, what do you think: Does this deal with the performance problems that the old driver code had? There were a number of other patches sent upstream by Rajeshwari also. I wonder if it might be easier to do your improvement as a separate patch on top of those instead. Then it can be considered on its merits. Hi Simon, what performance problems are there? Do you mean that u-boot is not fast enough when polling the SPI interface? I thought about this - even when clocking at 50MHz (resulting in 6.125 MB/s transfer rate) with 64 byte FIFOs there should be no problem when serving the interface, especially when receive and transmit flows are split in time. Have there been any evidence of performance problems? Also, I noticed that the driver does not pay any respect to error conditions. I am planning to add error monitoring/processing code, as this would be a good way to know if there indeed are performance problems. cheers, --vb Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 2/2] EXYNOS: SPI: Support SPI_PREAMBLE mode
[the original patch removed]
So, I spent some more time debugging a system which requires this
patch: a system, where on a SPI interface a response to a command
could come way later then the command data transmission completes.
The original patch was trying to address many corner cases, but come
to think of it, in this situation the slave does not care about extra
data sent on the transmit interface, as otherwise there is no clock
and no data could be transferred from the slave.
Then, for this SPI interface we do not need to set the counter of
clocks, and do not need to keep adding more clocks if the data has not
been received yet, the clocks could be just free running. And then the
patch becomes much simpler, what do you think:
diff --git a/drivers/spi/exynos_spi.c b/drivers/spi/exynos_spi.c
index c697db8..fff8310 100644
--- a/drivers/spi/exynos_spi.c
+++ b/drivers/spi/exynos_spi.c
@@ -211,10 +211,10 @@ static void spi_get_fifo_levels(struct exynos_spi *regs,
*/
static void spi_request_bytes(struct exynos_spi *regs, int count)
{
- assert(count count (1 16));
setbits_le32(regs-ch_cfg, SPI_CH_RST);
clrbits_le32(regs-ch_cfg, SPI_CH_RST);
- writel(count | SPI_PACKET_CNT_EN, regs-pkt_cnt);
+ if (count)
+ writel(count | SPI_PACKET_CNT_EN, regs-pkt_cnt);
}
static void spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
@@ -225,14 +225,20 @@ static void spi_rx_tx(struct exynos_spi_slave
*spi_slave, int todo,
const uchar *txp = *doutp;
int rx_lvl, tx_lvl;
uint out_bytes, in_bytes;
-
+ int hunting;
+
+ if (spi_slave-free_running_mode) {
+ spi_request_bytes(regs, 0);
+ hunting = 1;
+ } else {
+ hunting = 0;
+ spi_request_bytes(regs, todo);
+ }
out_bytes = in_bytes = todo;
- /*
-* If there's something to send, do a software reset and set a
-* transaction size.
-*/
- spi_request_bytes(regs, todo);
+ /* Software reset the channel. */
+ setbits_le32(regs-ch_cfg, SPI_CH_RST);
+ clrbits_le32(regs-ch_cfg, SPI_CH_RST);
/*
* Bytes are transmitted/received in pairs. Wait to receive all the
@@ -243,13 +249,23 @@ static void spi_rx_tx(struct exynos_spi_slave
*spi_slave, int todo,
/* Keep the fifos full/empty. */
spi_get_fifo_levels(regs, rx_lvl, tx_lvl);
- if (tx_lvl spi_slave-fifo_size out_bytes) {
- temp = txp ? *txp++ : 0xff;
+ if (tx_lvl spi_slave-fifo_size) {
+ if (txp out_bytes) {
+ temp = *txp++;
+ out_bytes--;
+ } else {
+ temp = 0xff;
+ }
writel(temp, regs-tx_data);
- out_bytes--;
}
if (rx_lvl 0 in_bytes) {
temp = readl(regs-rx_data);
+ if (hunting) {
+ if ((temp 0xff) != PREAMBLE_VALUE)
+ continue;
+ else
+ hunting = 0;
+ }
if (rxp)
*rxp++ = temp;
in_bytes--;
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] EXYNOS: SPI: Support SPI_PREAMBLE mode
On Thu, Mar 21, 2013 at 11:29 PM, Rajeshwari Shinde
rajeshwar...@samsung.com wrote:
Support interfaces with a preamble before each received message.
We handle this when the client has requested a SPI_XFER_END, meaning
that we must close of the transaction. In this case we read until we
see the preamble (or a timeout occurs), skipping all data before and
including the preamble. The client will receive only data bytes after
the preamble.
Signed-off-by: Simon Glass s...@chromium.org
Signed-off-by: Rajeshwari Shinde rajeshwar...@samsung.com
---
drivers/spi/exynos_spi.c | 62
--
1 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/drivers/spi/exynos_spi.c b/drivers/spi/exynos_spi.c
index be60ada..09e88d5 100644
--- a/drivers/spi/exynos_spi.c
+++ b/drivers/spi/exynos_spi.c
@@ -51,6 +51,7 @@ struct exynos_spi_slave {
unsigned int mode;
enum periph_id periph_id; /* Peripheral ID for this device */
unsigned int fifo_size;
+ int skip_preamble;
};
static struct spi_bus *spi_get_bus(unsigned dev_index)
@@ -107,6 +108,8 @@ struct spi_slave *spi_setup_slave(unsigned int busnum,
unsigned int cs,
else
spi_slave-fifo_size = 256;
+ spi_slave-skip_preamble = 0;
+
spi_slave-freq = bus-frequency;
if (max_hz)
spi_slave-freq = min(max_hz, spi_slave-freq);
@@ -219,17 +222,23 @@ static void spi_request_bytes(struct exynos_spi *regs,
int count)
writel(count | SPI_PACKET_CNT_EN, regs-pkt_cnt);
}
-static void spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
- void **dinp, void const **doutp)
+static int spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
+ void **dinp, void const **doutp, unsigned long flags)
{
struct exynos_spi *regs = spi_slave-regs;
uchar *rxp = *dinp;
const uchar *txp = *doutp;
int rx_lvl, tx_lvl;
uint out_bytes, in_bytes;
+ int toread, preamable_count = 0;
preamable_count: the name is misspelled, and the variable is never modified.
+ unsigned start = get_timer(0);
+ int stopping;
out_bytes = in_bytes = todo;
+ stopping = spi_slave-skip_preamble (flags SPI_XFER_END)
+ !(spi_slave-mode SPI_SLAVE);
+
/*
* If there's something to send, do a software reset and set a
* transaction size.
@@ -240,6 +249,7 @@ static void spi_rx_tx(struct exynos_spi_slave *spi_slave,
int todo,
* Bytes are transmitted/received in pairs. Wait to receive all the
* data because then transmission will be done as well.
*/
+ toread = in_bytes;
while (in_bytes) {
int temp;
@@ -252,13 +262,41 @@ static void spi_rx_tx(struct exynos_spi_slave
*spi_slave, int todo,
}
if (rx_lvl 0 in_bytes) {
temp = readl(regs-rx_data);
- if (rxp)
+ if (!rxp !stopping) {
+ in_bytes--;
+ } else if (spi_slave-skip_preamble) {
+ if (temp == SPI_PREAMBLE_END_BYTE) {
+ spi_slave-skip_preamble = 0;
+ stopping = 0;
+ }
+ } else {
*rxp++ = temp;
- in_bytes--;
+ in_bytes--;
+ }
+ toread--;
+ }
+ /*
+* We have run out of input data, but haven't read enough
+* bytes after the preamble yet. Read some more, and make
+* sure that we transmit dummy bytes too, to keep things
+* going.
+*/
+ else if (in_bytes !toread) {
+ assert(!out_bytes);
+ toread = out_bytes = in_bytes;
+ txp = NULL;
+ spi_request_bytes(regs, toread);
+ }
+ if (spi_slave-skip_preamble get_timer(start) 100) {
+ printf(SPI timeout: in_bytes=%d, out_bytes=%d, ,
+ in_bytes, out_bytes);
+ printf(count = %d\n, preamable_count);
+ return -1;
}
}
*dinp = rxp;
*doutp = txp;
+ return 0;
}
/**
@@ -278,6 +316,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int
bitlen, const void *dout,
struct exynos_spi_slave *spi_slave = to_exynos_spi(slave);
int upto, todo;
int bytelen;
+ int ret = 0;
/* spi core configured to do 8 bit transfers
Re: [U-Boot] [PATCH 2/2] EXYNOS: SPI: Support SPI_PREAMBLE mode
[the original patch removed, re-sending from a registered address]
So, I spent some more time debugging a system which requires this
patch: a system, where on a SPI interface a response to a command
could come way later then the command data transmission completes.
The original patch was trying to address many corner cases, but come
to think of it, in this situation the slave does not care about extra
data sent on the transmit interface, as otherwise there is no clock
and no data could be transferred from the slave.
Then, for this SPI interface we do not need to set the counter of
clocks, and do not need to keep adding more clocks if the data has not
been received yet, the clocks could be just free running. And then the
patch becomes much simpler, what do you think:
diff --git a/drivers/spi/exynos_spi.c b/drivers/spi/exynos_spi.c
index c697db8..fff8310 100644
--- a/drivers/spi/exynos_spi.c
+++ b/drivers/spi/exynos_spi.c
@@ -211,10 +211,10 @@ static void spi_get_fifo_levels(struct exynos_spi *regs,
*/
static void spi_request_bytes(struct exynos_spi *regs, int count)
{
- assert(count count (1 16));
setbits_le32(regs-ch_cfg, SPI_CH_RST);
clrbits_le32(regs-ch_cfg, SPI_CH_RST);
- writel(count | SPI_PACKET_CNT_EN, regs-pkt_cnt);
+ if (count)
+ writel(count | SPI_PACKET_CNT_EN, regs-pkt_cnt);
}
static void spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
@@ -225,14 +225,20 @@ static void spi_rx_tx(struct exynos_spi_slave
*spi_slave, int todo,
const uchar *txp = *doutp;
int rx_lvl, tx_lvl;
uint out_bytes, in_bytes;
-
+ int hunting;
+
+ if (spi_slave-free_running_mode) {
+ spi_request_bytes(regs, 0);
+ hunting = 1;
+ } else {
+ hunting = 0;
+ spi_request_bytes(regs, todo);
+ }
out_bytes = in_bytes = todo;
- /*
-* If there's something to send, do a software reset and set a
-* transaction size.
-*/
- spi_request_bytes(regs, todo);
+ /* Software reset the channel. */
+ setbits_le32(regs-ch_cfg, SPI_CH_RST);
+ clrbits_le32(regs-ch_cfg, SPI_CH_RST);
/*
* Bytes are transmitted/received in pairs. Wait to receive all the
@@ -243,13 +249,23 @@ static void spi_rx_tx(struct exynos_spi_slave
*spi_slave, int todo,
/* Keep the fifos full/empty. */
spi_get_fifo_levels(regs, rx_lvl, tx_lvl);
- if (tx_lvl spi_slave-fifo_size out_bytes) {
- temp = txp ? *txp++ : 0xff;
+ if (tx_lvl spi_slave-fifo_size) {
+ if (txp out_bytes) {
+ temp = *txp++;
+ out_bytes--;
+ } else {
+ temp = 0xff;
+ }
writel(temp, regs-tx_data);
- out_bytes--;
}
if (rx_lvl 0 in_bytes) {
temp = readl(regs-rx_data);
+ if (hunting) {
+ if ((temp 0xff) != PREAMBLE_VALUE)
+ continue;
+ else
+ hunting = 0;
+ }
if (rxp)
*rxp++ = temp;
in_bytes--;
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] i2c: add NACK logic in write
On Mon, Mar 25, 2013 at 4:17 AM, Akshay Saraswat aksha...@samsung.com wrote:
From: Naveen Krishna Chatradhi ch.nav...@samsung.com
Adding NACK logic for i2c write.
Verified by reading and writing to device with address 9 on i2c-0.
Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
Signed-off-by: Akshay Saraswat aksha...@samsung.com
---
drivers/i2c/s3c24x0_i2c.c | 111
+++---
1 file changed, 56 insertions(+), 55 deletions(-)
diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c
index 769a2ba..d2b4eb0 100644
--- a/drivers/i2c/s3c24x0_i2c.c
+++ b/drivers/i2c/s3c24x0_i2c.c
@@ -201,6 +201,35 @@ unsigned int i2c_get_bus_num(void)
}
#endif
+/*
+ * Verify the whether I2C ACK was received or not
+ *
+ * @param i2c pointer to I2C register base
+ * @param buf array of data
+ * @param len length of data
+ * return I2C_OK when transmission done
+ * I2C_NACK otherwise
+ */
+static int i2c_send_verify(struct s3c24x0_i2c *i2c, unsigned char buf[],
+ unsigned char len)
+{
+ int i, result = I2C_OK;
+
+ if (IsACK(i2c)) {
+ for (i = 0; (i len) (result == I2C_OK); i++) {
+ writel(buf[i], i2c-iicds);
+ ReadWriteByte(i2c);
+ result = WaitForXfer(i2c);
+ if (!IsACK(i2c))
+ result = I2C_NACK;
+ }
+ } else {
+ result = I2C_NACK;
+ }
+
+ return result;
+}
+
void i2c_init(int speed, int slaveadd)
{
struct s3c24x0_i2c *i2c;
@@ -302,41 +331,30 @@ static int i2c_transfer(struct s3c24x0_i2c *i2c,
return I2C_NOK_TOUT;
writel(readl(i2c-iiccon) | I2CCON_ACKGEN, i2c-iiccon);
- result = I2C_OK;
+
+ if (addr addr_len) {
+ writel(chip, i2c-iicds);
+ /* send START */
+ writel(I2C_MODE_MT | I2C_TXRX_ENA | I2C_START_STOP,
+ i2c-iicstat);
+ if (WaitForXfer(i2c) == I2C_OK)
+ result = i2c_send_verify(i2c, addr, addr_len);
+ else
+ result = I2C_NACK;
+ } else
+ result = I2C_NACK;
switch (cmd_type) {
case I2C_WRITE:
- if (addr addr_len) {
- writel(chip, i2c-iicds);
- /* send START */
- writel(I2C_MODE_MT | I2C_TXRX_ENA |
I2C_START_STOP,
- i2c-iicstat);
- i = 0;
- while ((i addr_len) (result == I2C_OK)) {
- result = WaitForXfer(i2c);
- writel(addr[i], i2c-iicds);
- ReadWriteByte(i2c);
- i++;
- }
- i = 0;
- while ((i data_len) (result == I2C_OK)) {
- result = WaitForXfer(i2c);
- writel(data[i], i2c-iicds);
- ReadWriteByte(i2c);
- i++;
- }
- } else {
+ if (result == I2C_OK)
+ result = i2c_send_verify(i2c, data, data_len);
+ else {
writel(chip, i2c-iicds);
/* send START */
writel(I2C_MODE_MT | I2C_TXRX_ENA |
I2C_START_STOP,
- i2c-iicstat);
- i = 0;
- while ((i data_len) (result = I2C_OK)) {
- result = WaitForXfer(i2c);
- writel(data[i], i2c-iicds);
- ReadWriteByte(i2c);
- i++;
- }
+ i2c-iicstat);
+ if (WaitForXfer(i2c) == I2C_OK)
+ result = i2c_send_verify(i2c, data,
data_len);
}
if (result == I2C_OK)
@@ -348,42 +366,25 @@ static int i2c_transfer(struct s3c24x0_i2c *i2c,
break;
case I2C_READ:
- if (addr addr_len) {
- writel(I2C_MODE_MT | I2C_TXRX_ENA, i2c-iicstat);
+ if (result == I2C_OK) {
+ writel(I2C_MODE_MR | I2C_TXRX_ENA, i2c-iicstat);
writel(chip, i2c-iicds);
/* send START */
writel(readl(i2c-iicstat) | I2C_START_STOP,
i2c-iicstat);
- result = WaitForXfer(i2c);
- if (IsACK(i2c)) {
-
[U-Boot] [PATCH] Do not call board_early_init_f() twice
Apparently due to a missed rebase conflict resolution
board_early_init_f() is included twice in the list of initialization
functions.
Leave only the first occurrence.
. built and boot an Exynos 5250 target
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
common/board_f.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/common/board_f.c b/common/board_f.c
index 29b49c3..7698891 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -788,9 +788,6 @@ static init_fnc_t init_sequence_f[] = {
/* TODO: can we rename this to timer_init()? */
init_timebase,
#endif
-#if defined(CONFIG_BOARD_EARLY_INIT_F)
- board_early_init_f,
-#endif
#ifdef CONFIG_ARM
timer_init, /* initialize timer */
#endif
--
1.8.1.3
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 4/4] Exynos5: config: enable cpu freq
On Tue, Apr 2, 2013 at 11:27 PM, Akshay Saraswat aksha...@samsung.com wrote: This patch enables cpu freq support for exynos5 by adding config for it. Signed-off-by: Akshay Saraswat aksha...@samsung.com --- include/configs/exynos5250-dt.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/configs/exynos5250-dt.h b/include/configs/exynos5250-dt.h index 496a194..18d3e04 100644 --- a/include/configs/exynos5250-dt.h +++ b/include/configs/exynos5250-dt.h @@ -128,6 +128,9 @@ #define CONFIG_CMD_DTT #define CONFIG_TMU_CMD_DTT +/* CPU Frequency Scaling */ +#define CONFIG_EXYNOS_CPUFREQ + /* USB */ #define CONFIG_CMD_USB #define CONFIG_USB_EHCI -- 1.8.0 These patches should be applicable to both 5250 and 5420 families, but seem to be addressing 5250 only, cab they be generalized? --vb ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] build: Fix make errors generated when building 'distclean'
It was noticed that when `make distclean' is run, the make process terminates with error reporting something like: rm: cannot remove '/tmp/foobar/': Is a directory make: *** [clobber] Error 1 The problem is that the list of files targeted for removal includes a directory in case CONFIG_SPL_TARGET is not set. The fix has been tested as follows: Ran several times the following sequence of commands: CROSS_COMPILE=/usr/bin/arm-linux-gnueabi- make O=/tmp/foobar smdk5250_config CROSS_COMPILE=/usr/bin/arm-linux-gnueabi- make O=/tmp/foobar distclean it did not cause an error, it used to before this change. Signed-off-by: Vadim Bendebury vben...@chromium.org --- Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 12763ce..23f266a 100644 --- a/Makefile +++ b/Makefile @@ -406,8 +406,10 @@ ALL-y += $(obj)u-boot.srec $(obj)u-boot.bin $(obj)System.map ALL-$(CONFIG_NAND_U_BOOT) += $(obj)u-boot-nand.bin ALL-$(CONFIG_ONENAND_U_BOOT) += $(obj)u-boot-onenand.bin ALL-$(CONFIG_SPL) += $(obj)spl/u-boot-spl.bin -ALL-$(CONFIG_SPL) += $(obj)$(subst ,,$(CONFIG_SPL_TARGET)) ALL-$(CONFIG_OF_SEPARATE) += $(obj)u-boot.dtb $(obj)u-boot-dtb.bin +ifneq ($(CONFIG_SPL_TARGET),) +ALL-$(CONFIG_SPL) += $(obj)$(subst ,,$(CONFIG_SPL_TARGET)) +endif # enable combined SPL/u-boot/dtb rules for tegra ifneq ($(CONFIG_TEGRA),) -- 1.8.1.3 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] u-boot repository mirror and gerrit on Google inftrastructure
Just wanted to let everybody know that there has been a mirror set up on Google infrastructure which polls git://git.denx.de/u-boot.git every 5 minutes. URL for browsing is https://u-boot.googlesource.com, and https://u-boot-review.googlesource.com is actually a gerrit instance ready to be used if ever decided. It is an extremely high availability service with replicas in six google datacenters across the globe. For those interested, here are some slides describing a similar setup hosting android repositories: https://sites.google.com/site/gerritsummit2012/program/Sunday-android.googlesource.com.pdf?attredirects=0d=1 cheers, /vb ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] u-boot repository mirror and gerrit on Google inftrastructure
On Thu, Jan 31, 2013 at 9:23 PM, Wolfgang Denk w...@denx.de wrote: Dear Vadim Bendebury, In message cany1bulo6a_vnvhbqdlbcahp5xg4+djch258thkew3-xi1t...@mail.gmail.com you wrote: Just wanted to let everybody know that there has been a mirror set up on Google infrastructure which polls git://git.denx.de/u-boot.git every 5 minutes. Every 5 minutes? Are you crazy??? Please stop doing this now! You are just putting a lot of load on our server, thus hampering regular work. And it makjes zero sense, as the data on the public server get updated only every 6 hours. URL for browsing is https://u-boot.googlesource.com, and https://u-boot-review.googlesource.com is actually a gerrit instance ready to be used if ever decided. We discussed this in the past, and decided not to, as it does not fit at all into our mainly e-mail based work flow. So please STOP sabotaging our server! Well, I should have asked for details and share the plans before setting this up. As briefly discussed offline, I will change this to poll once in six hours soon after each public server update. 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 You said you didn't want to use CGI.pm, but methinks you are needlessly reinventing the wheel, one spoke at a time. Either you are masochistic, or you just haven't seen enough of what CGI.pm can do for you. -- Randal L. Schwartz in 8cyb81rg81@gadget.cscaper.com ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Googlers please reply: commiters in U-Boot
On Sat, Jan 12, 2013 at 9:20 AM, Simon Glass s...@chromium.org wrote: Hi, You are being copied because you have written U-Boot code which is now in mainline. The chromium.org domain does not automatically attribute U-Boot commits by company. Each author needs to be manually added to the list and this can only be done if you confirm your employer. So, if you are on the CC list and work at Google, please reply-all with a quick email stating this (without top posting). Regards, Simon All emails from vben...@chromium.org come from me while I work for Google. Vadim Bendebury ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4] patman: Allow use outside of u-boot tree
On Thu, Jan 10, 2013 at 9:05 AM, Simon Glass s...@chromium.org wrote: Hi Vadim, On Wed, Jan 9, 2013 at 6:00 PM, Vadim Bendebury vben...@chromium.org wrote: To make it usable in git trees not providing a patch checker implementation, add a command line option, allowing to suppress patch check. While we are at it, sort debug options alphabetically. Also, do not raise an exception if checkpatch.pl is not found - just print an error message suggesting to use the new option, and return nonzero status. . unit test passes: $ ./patman -t unittest.result.TestResult run=7 errors=0 failures=0 . successfully used patman in the autotest tree to generate a patch email (with --no-check option) . successfully used patman in the u-boot tree to generate a patch email . `patman --help' now shows command line options ordered alphabetically Signed-off-by: Vadim Bendebury vben...@chromium.org Acked-by: Simon Glass s...@chromium.org BTW what is this patch against - it doesn't seem to apply to mainline for me. Right, it depends on the previous patches which still have not been applied to the upstream ToT. cheers, /vb --- Changes in v4: . now properly corrected typo in the error message Changes in v3: . corrected typo in the error message Changes in v2: . addressed comments WRT use of double negative . added code to gracefully handle absence of checkpatch.cl tools/patman/checkpatch.py | 10 +- tools/patman/patman.py | 14 ++ 2 files changed, 15 insertions(+), 9 deletions(-) [snip] Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] patman: Allow use outside of u-boot tree
To make it usable in git trees not providing a patch checker
implementation, add a command line option, allowing to suppress patch
check. While we are at it, sort debug options alphabetically.
. unit test passes:
$ ./patman -t
unittest.result.TestResult run=7 errors=0 failures=0
. successfully used patman in the autotest tree to generate a patch
email (with --no-check option)
. successfully used patman in the u-boot tree to generate a patch
email
. `patman --help' now shows command line options ordered
alphabetically
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
tools/patman/patman.py | 14 ++
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index e56dd01..6620a48 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -50,6 +50,9 @@ parser.add_option('-i', '--ignore-errors',
action='store_true',
help='Send patches email even if patch errors are found')
parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
default=False, help=Do a try run (create but don't email patches))
+parser.add_option('-p', '--project', default=project.DetectProject(),
+ help=Project name; affects default option values and
+ aliases [default: %default])
parser.add_option('-s', '--start', dest='start', type='int',
default=0, help='Commit to start creating patches from (0 = HEAD)')
parser.add_option('-t', '--test', action='store_true', dest='test',
@@ -58,11 +61,11 @@ parser.add_option('-v', '--verbose', action='store_true',
dest='verbose',
default=False, help='Verbose output of errors and warnings')
parser.add_option('--cc-cmd', dest='cc_cmd', type='string', action='store',
default=None, help='Output cc list for patch file (used by git)')
+parser.add_option('--no-check', action='store_true', dest='no_check',
+ default=False,
+ help=Don't check for patch compliance)
parser.add_option('--no-tags', action='store_false', dest='process_tags',
default=True, help=Don't process subject tags as aliaes)
-parser.add_option('-p', '--project', default=project.DetectProject(),
- help=Project name; affects default option values and
- aliases [default: %default])
parser.usage = patman [options]
@@ -146,7 +149,10 @@ else:
series.DoChecks()
# Check the patches, and run them through 'git am' just to be sure
-ok = checkpatch.CheckPatches(options.verbose, args)
+if options.no_check:
+ok = True
+else:
+ok = checkpatch.CheckPatches(options.verbose, args)
if not gitutil.ApplyPatches(options.verbose, args,
options.count + options.start):
ok = False
--
1.7.7.3
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] Make patman usable outside of u-boot tree
To make it usable in git trees not providing a patch checker
implementation, add a command line option, allowing to suippress patch
check. While we are at it, sort debug options alphabetically.
. unit test passes:
$ ./patman -t
unittest.result.TestResult run=7 errors=0 failures=0
. successfully used patman in the autotest tree to generate a patch
email (with --no-check option)
. successfully used patman in the u-boot tree to generate a patch
email
. `patman --help' now shows command line options ordered
alphabetically
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
tools/patman/patman.py | 14 ++
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index e56dd01..6620a48 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -50,6 +50,9 @@ parser.add_option('-i', '--ignore-errors',
action='store_true',
help='Send patches email even if patch errors are found')
parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
default=False, help=Do a try run (create but don't email patches))
+parser.add_option('-p', '--project', default=project.DetectProject(),
+ help=Project name; affects default option values and
+ aliases [default: %default])
parser.add_option('-s', '--start', dest='start', type='int',
default=0, help='Commit to start creating patches from (0 = HEAD)')
parser.add_option('-t', '--test', action='store_true', dest='test',
@@ -58,11 +61,11 @@ parser.add_option('-v', '--verbose', action='store_true',
dest='verbose',
default=False, help='Verbose output of errors and warnings')
parser.add_option('--cc-cmd', dest='cc_cmd', type='string', action='store',
default=None, help='Output cc list for patch file (used by git)')
+parser.add_option('--no-check', action='store_true', dest='no_check',
+ default=False,
+ help=Don't check for patch compliance)
parser.add_option('--no-tags', action='store_false', dest='process_tags',
default=True, help=Don't process subject tags as aliaes)
-parser.add_option('-p', '--project', default=project.DetectProject(),
- help=Project name; affects default option values and
- aliases [default: %default])
parser.usage = patman [options]
@@ -146,7 +149,10 @@ else:
series.DoChecks()
# Check the patches, and run them through 'git am' just to be sure
-ok = checkpatch.CheckPatches(options.verbose, args)
+if options.no_check:
+ok = True
+else:
+ok = checkpatch.CheckPatches(options.verbose, args)
if not gitutil.ApplyPatches(options.verbose, args,
options.count + options.start):
ok = False
--
1.7.7.3
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] patman: Allow use outside of u-boot tree
On Wed, Jan 9, 2013 at 2:01 PM, Vadim Bendebury vben...@chromium.org wrote:
To make it usable in git trees not providing a patch checker
implementation, add a command line option, allowing to suppress patch
check. While we are at it, sort debug options alphabetically.
. unit test passes:
$ ./patman -t
unittest.result.TestResult run=7 errors=0 failures=0
. successfully used patman in the autotest tree to generate a patch
email (with --no-check option)
. successfully used patman in the u-boot tree to generate a patch
email
. `patman --help' now shows command line options ordered
alphabetically
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
tools/patman/patman.py | 14 ++
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index e56dd01..6620a48 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -50,6 +50,9 @@ parser.add_option('-i', '--ignore-errors',
action='store_true',
help='Send patches email even if patch errors are found')
parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
default=False, help=Do a try run (create but don't email patches))
+parser.add_option('-p', '--project', default=project.DetectProject(),
+ help=Project name; affects default option values and
+ aliases [default: %default])
parser.add_option('-s', '--start', dest='start', type='int',
default=0, help='Commit to start creating patches from (0 = HEAD)')
parser.add_option('-t', '--test', action='store_true', dest='test',
@@ -58,11 +61,11 @@ parser.add_option('-v', '--verbose', action='store_true',
dest='verbose',
default=False, help='Verbose output of errors and warnings')
parser.add_option('--cc-cmd', dest='cc_cmd', type='string', action='store',
default=None, help='Output cc list for patch file (used by git)')
+parser.add_option('--no-check', action='store_true', dest='no_check',
+ default=False,
+ help=Don't check for patch compliance)
parser.add_option('--no-tags', action='store_false', dest='process_tags',
default=True, help=Don't process subject tags as aliaes)
-parser.add_option('-p', '--project', default=project.DetectProject(),
- help=Project name; affects default option values and
- aliases [default: %default])
parser.usage = patman [options]
@@ -146,7 +149,10 @@ else:
series.DoChecks()
# Check the patches, and run them through 'git am' just to be sure
-ok = checkpatch.CheckPatches(options.verbose, args)
+if options.no_check:
+ok = True
+else:
+ok = checkpatch.CheckPatches(options.verbose, args)
if not gitutil.ApplyPatches(options.verbose, args,
options.count + options.start):
ok = False
--
1.7.7.3
Doug, thank you for a prompt review, copying your response here,
please see below:
On Wed, Jan 9, 2013 at 1:48 PM, Doug Anderson diand...@chromium.org wrote:
Vadim,
Thanks for the patch! Looks good in general, though please add the
patman prefix to the first line of your commit message.
done
On Wed, Jan 9, 2013 at 1:13 PM, Vadim Bendebury vben...@chromium.org wrote:
To make it usable in git trees not providing a patch checker
implementation, add a command line option, allowing to suippress patch
s/suippress/suppress
done
+parser.add_option('--no-check', action='store_true', dest='no_check',
+ default=False,
+ help=Don't check for patch compliance)
IMHO It would be slightly better to use action='store_false',
dest='check', and default=True (just to avoid so many
double-negatives).
I don't quite agree with this part - I think it's perfectly reasonable
to use 'no-check' to suppress the check, just as well as to use
'no-tags' to suppress interpreting tags.
`--no' communicates that by default the respective feature is enabled,
and to disable it one needs to add a command line option with no
parameter.
cheers,
/vb
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Make patman usable outside of u-boot tree
On Wed, Jan 9, 2013 at 2:13 PM, Simon Glass s...@google.com wrote:
Hi Vadim,
Looks good! Please can you add a single character option?
Simon, I could not think of a good single letter option to pick, so I
did not, but if you have a suggestion I implement it.
Can you also please add an option to skip the 'apply' step? This takes
quite a bit of time, and it would be nice to have a 'fast' option.
does it have to be in this CL?
cheers,
/vb
Regards,
Simon
On Wed, Jan 9, 2013 at 1:48 PM, Doug Anderson diand...@chromium.org wrote:
Vadim,
Thanks for the patch! Looks good in general, though please add the
patman prefix to the first line of your commit message.
On Wed, Jan 9, 2013 at 1:13 PM, Vadim Bendebury vben...@chromium.org wrote:
To make it usable in git trees not providing a patch checker
implementation, add a command line option, allowing to suippress patch
s/suippress/suppress
+parser.add_option('--no-check', action='store_true', dest='no_check',
+ default=False,
+ help=Don't check for patch compliance)
IMHO It would be slightly better to use action='store_false',
dest='check', and default=True (just to avoid so many
double-negatives).
-Doug
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Make patman usable outside of u-boot tree
On Wed, Jan 9, 2013 at 2:22 PM, Simon Glass s...@chromium.org wrote:
Hi Vadim,
On Wed, Jan 9, 2013 at 2:16 PM, Vadim Bendebury vben...@chromium.org wrote:
On Wed, Jan 9, 2013 at 2:13 PM, Simon Glass s...@google.com wrote:
Hi Vadim,
Looks good! Please can you add a single character option?
Simon, I could not think of a good single letter option to pick, so I
did not, but if you have a suggestion I implement it.
I can't think of a good one.
-C or -P ?
Yeah, I feel a bit awkward about these, also, it would be inconsistent
with --no-tags which does not have a single letter alternative. Let's
see what others think about it - I would rather leave it as is...
cheers,
/v
Can you also please add an option to skip the 'apply' step? This takes
quite a bit of time, and it would be nice to have a 'fast' option.
does it have to be in this CL?
No not at all.
cheers,
/vb
Regards,
Simon
Regards,
Simon
On Wed, Jan 9, 2013 at 1:48 PM, Doug Anderson diand...@chromium.org wrote:
Vadim,
Thanks for the patch! Looks good in general, though please add the
patman prefix to the first line of your commit message.
On Wed, Jan 9, 2013 at 1:13 PM, Vadim Bendebury vben...@chromium.org
wrote:
To make it usable in git trees not providing a patch checker
implementation, add a command line option, allowing to suippress patch
s/suippress/suppress
+parser.add_option('--no-check', action='store_true', dest='no_check',
+ default=False,
+ help=Don't check for patch compliance)
IMHO It would be slightly better to use action='store_false',
dest='check', and default=True (just to avoid so many
double-negatives).
-Doug
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] patman: Allow use outside of u-boot tree
On Wed, Jan 9, 2013 at 2:57 PM, Doug Anderson diand...@chromium.org wrote:
Vadim,
On Wed, Jan 9, 2013 at 2:07 PM, Vadim Bendebury vben...@chromium.org wrote:
+parser.add_option('--no-check', action='store_true', dest='no_check',
+ default=False,
+ help=Don't check for patch compliance)
IMHO It would be slightly better to use action='store_false',
dest='check', and default=True (just to avoid so many
double-negatives).
I don't quite agree with this part - I think it's perfectly reasonable
to use 'no-check' to suppress the check, just as well as to use
'no-tags' to suppress interpreting tags.
`--no' communicates that by default the respective feature is enabled,
and to disable it one needs to add a command line option with no
parameter.
Sorry--should have been more explicit. Was still expecting the option
to be --no-check. Just asking for a change to the way it's stored.
Like this in the python dev guide:
parser.add_option(--clobber, action=store_true, dest=clobber)
parser.add_option(--no-clobber, action=store_false, dest=clobber)
In your case, I don't think you need to add the check option too,
but just store to the check option:
parser.add_option('--no-check', action='store_false', dest='check',
default=True,
help=Don't check for patch compliance)
ah, a good idea, changed. Also, modified the code in checkpatch.py not
to throw an exception, but to print an error message and return a
nonzero status.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] patman: Allow use outside of u-boot tree
To make it usable in git trees not providing a patch checker
implementation, add a command line option, allowing to suppress patch
check. While we are at it, sort debug options alphabetically.
Also, do not raise an exception if checkpatch.pl is not found - just
print an error message suggesting to use the new option, and return
nonzero status.
. unit test passes:
$ ./patman -t
unittest.result.TestResult run=7 errors=0 failures=0
. successfully used patman in the autotest tree to generate a patch
email (with --no-check option)
. successfully used patman in the u-boot tree to generate a patch
email
. `patman --help' now shows command line options ordered
alphabetically
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
tools/patman/checkpatch.py | 10 +-
tools/patman/patman.py | 14 ++
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py
index f72f8ee..00aef09 100644
--- a/tools/patman/checkpatch.py
+++ b/tools/patman/checkpatch.py
@@ -23,6 +23,7 @@ import command
import gitutil
import os
import re
+import sys
import terminal
def FindCheckPatch():
@@ -47,8 +48,10 @@ def FindCheckPatch():
if os.path.isfile(fname):
return fname
path = os.path.dirname(path)
-print 'Could not find checkpatch.pl'
-return None
+
+print sys.stderr, ('Cannot find checkpatch.pl - please put it in your '
+
+'~/bin directory or use --no_patch')
+sys.exit(1)
def CheckPatch(fname, verbose=False):
Run checkpatch.pl on a file.
@@ -67,9 +70,6 @@ def CheckPatch(fname, verbose=False):
error_count, warning_count, lines = 0, 0, 0
problems = []
chk = FindCheckPatch()
-if not chk:
-raise OSError, ('Cannot find checkpatch.pl - please put it in your ' +
-'~/bin directory')
item = {}
stdout = command.Output(chk, '--no-tree', fname)
#pipe = subprocess.Popen(cmd, stdout=subprocess.PIPE)
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index e56dd01..e049081 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -50,6 +50,9 @@ parser.add_option('-i', '--ignore-errors',
action='store_true',
help='Send patches email even if patch errors are found')
parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
default=False, help=Do a try run (create but don't email patches))
+parser.add_option('-p', '--project', default=project.DetectProject(),
+ help=Project name; affects default option values and
+ aliases [default: %default])
parser.add_option('-s', '--start', dest='start', type='int',
default=0, help='Commit to start creating patches from (0 = HEAD)')
parser.add_option('-t', '--test', action='store_true', dest='test',
@@ -58,11 +61,11 @@ parser.add_option('-v', '--verbose', action='store_true',
dest='verbose',
default=False, help='Verbose output of errors and warnings')
parser.add_option('--cc-cmd', dest='cc_cmd', type='string', action='store',
default=None, help='Output cc list for patch file (used by git)')
+parser.add_option('--no-check', action='store_false', dest='check_patch',
+ default=True,
+ help=Don't check for patch compliance)
parser.add_option('--no-tags', action='store_false', dest='process_tags',
default=True, help=Don't process subject tags as aliaes)
-parser.add_option('-p', '--project', default=project.DetectProject(),
- help=Project name; affects default option values and
- aliases [default: %default])
parser.usage = patman [options]
@@ -146,7 +149,10 @@ else:
series.DoChecks()
# Check the patches, and run them through 'git am' just to be sure
-ok = checkpatch.CheckPatches(options.verbose, args)
+if options.check_patch:
+ok = checkpatch.CheckPatches(options.verbose, args)
+else:
+ok = True
if not gitutil.ApplyPatches(options.verbose, args,
options.count + options.start):
ok = False
--
1.7.7.3
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2] patman: Allow use outside of u-boot tree
To make it usable in git trees not providing a patch checker
implementation, add a command line option, allowing to suppress patch
check. While we are at it, sort debug options alphabetically.
Also, do not raise an exception if checkpatch.pl is not found - just
print an error message suggesting to use the new option, and return
nonzero status.
. unit test passes:
$ ./patman -t
unittest.result.TestResult run=7 errors=0 failures=0
. successfully used patman in the autotest tree to generate a patch
email (with --no-check option)
. successfully used patman in the u-boot tree to generate a patch
email
. `patman --help' now shows command line options ordered
alphabetically
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
Changes in v2:
. addressed comments WRT use of double negative
. added code to gracefully handle absence of checkpatch.cl
tools/patman/checkpatch.py | 10 +-
tools/patman/patman.py | 14 ++
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py
index f72f8ee..00aef09 100644
--- a/tools/patman/checkpatch.py
+++ b/tools/patman/checkpatch.py
@@ -23,6 +23,7 @@ import command
import gitutil
import os
import re
+import sys
import terminal
def FindCheckPatch():
@@ -47,8 +48,10 @@ def FindCheckPatch():
if os.path.isfile(fname):
return fname
path = os.path.dirname(path)
-print 'Could not find checkpatch.pl'
-return None
+
+print sys.stderr, ('Cannot find checkpatch.pl - please put it in your '
+
+'~/bin directory or use --no_patch')
+sys.exit(1)
def CheckPatch(fname, verbose=False):
Run checkpatch.pl on a file.
@@ -67,9 +70,6 @@ def CheckPatch(fname, verbose=False):
error_count, warning_count, lines = 0, 0, 0
problems = []
chk = FindCheckPatch()
-if not chk:
-raise OSError, ('Cannot find checkpatch.pl - please put it in your ' +
-'~/bin directory')
item = {}
stdout = command.Output(chk, '--no-tree', fname)
#pipe = subprocess.Popen(cmd, stdout=subprocess.PIPE)
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index e56dd01..e049081 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -50,6 +50,9 @@ parser.add_option('-i', '--ignore-errors',
action='store_true',
help='Send patches email even if patch errors are found')
parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
default=False, help=Do a try run (create but don't email patches))
+parser.add_option('-p', '--project', default=project.DetectProject(),
+ help=Project name; affects default option values and
+ aliases [default: %default])
parser.add_option('-s', '--start', dest='start', type='int',
default=0, help='Commit to start creating patches from (0 = HEAD)')
parser.add_option('-t', '--test', action='store_true', dest='test',
@@ -58,11 +61,11 @@ parser.add_option('-v', '--verbose', action='store_true',
dest='verbose',
default=False, help='Verbose output of errors and warnings')
parser.add_option('--cc-cmd', dest='cc_cmd', type='string', action='store',
default=None, help='Output cc list for patch file (used by git)')
+parser.add_option('--no-check', action='store_false', dest='check_patch',
+ default=True,
+ help=Don't check for patch compliance)
parser.add_option('--no-tags', action='store_false', dest='process_tags',
default=True, help=Don't process subject tags as aliaes)
-parser.add_option('-p', '--project', default=project.DetectProject(),
- help=Project name; affects default option values and
- aliases [default: %default])
parser.usage = patman [options]
@@ -146,7 +149,10 @@ else:
series.DoChecks()
# Check the patches, and run them through 'git am' just to be sure
-ok = checkpatch.CheckPatches(options.verbose, args)
+if options.check_patch:
+ok = checkpatch.CheckPatches(options.verbose, args)
+else:
+ok = True
if not gitutil.ApplyPatches(options.verbose, args,
options.count + options.start):
ok = False
--
1.7.7.3
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3] patman: Allow use outside of u-boot tree
To make it usable in git trees not providing a patch checker
implementation, add a command line option, allowing to suppress patch
check. While we are at it, sort debug options alphabetically.
Also, do not raise an exception if checkpatch.pl is not found - just
print an error message suggesting to use the new option, and return
nonzero status.
. unit test passes:
$ ./patman -t
unittest.result.TestResult run=7 errors=0 failures=0
. successfully used patman in the autotest tree to generate a patch
email (with --no-check option)
. successfully used patman in the u-boot tree to generate a patch
email
. `patman --help' now shows command line options ordered
alphabetically
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
Changes in v3:
. corrected typo in the error message
Changes in v2:
. addressed comments WRT use of double negative
. added code to gracefully handle absence of checkpatch.cl
tools/patman/checkpatch.py | 10 +-
tools/patman/patman.py | 14 ++
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py
index f72f8ee..5a03314 100644
--- a/tools/patman/checkpatch.py
+++ b/tools/patman/checkpatch.py
@@ -23,6 +23,7 @@ import command
import gitutil
import os
import re
+import sys
import terminal
def FindCheckPatch():
@@ -47,8 +48,10 @@ def FindCheckPatch():
if os.path.isfile(fname):
return fname
path = os.path.dirname(path)
-print 'Could not find checkpatch.pl'
-return None
+
+print sys.stderr, ('Cannot find checkpatch.pl - please put it in your '
+
+'~/bin directory or use --no-patch')
+sys.exit(1)
def CheckPatch(fname, verbose=False):
Run checkpatch.pl on a file.
@@ -67,9 +70,6 @@ def CheckPatch(fname, verbose=False):
error_count, warning_count, lines = 0, 0, 0
problems = []
chk = FindCheckPatch()
-if not chk:
-raise OSError, ('Cannot find checkpatch.pl - please put it in your ' +
-'~/bin directory')
item = {}
stdout = command.Output(chk, '--no-tree', fname)
#pipe = subprocess.Popen(cmd, stdout=subprocess.PIPE)
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index e56dd01..e049081 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -50,6 +50,9 @@ parser.add_option('-i', '--ignore-errors',
action='store_true',
help='Send patches email even if patch errors are found')
parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
default=False, help=Do a try run (create but don't email patches))
+parser.add_option('-p', '--project', default=project.DetectProject(),
+ help=Project name; affects default option values and
+ aliases [default: %default])
parser.add_option('-s', '--start', dest='start', type='int',
default=0, help='Commit to start creating patches from (0 = HEAD)')
parser.add_option('-t', '--test', action='store_true', dest='test',
@@ -58,11 +61,11 @@ parser.add_option('-v', '--verbose', action='store_true',
dest='verbose',
default=False, help='Verbose output of errors and warnings')
parser.add_option('--cc-cmd', dest='cc_cmd', type='string', action='store',
default=None, help='Output cc list for patch file (used by git)')
+parser.add_option('--no-check', action='store_false', dest='check_patch',
+ default=True,
+ help=Don't check for patch compliance)
parser.add_option('--no-tags', action='store_false', dest='process_tags',
default=True, help=Don't process subject tags as aliaes)
-parser.add_option('-p', '--project', default=project.DetectProject(),
- help=Project name; affects default option values and
- aliases [default: %default])
parser.usage = patman [options]
@@ -146,7 +149,10 @@ else:
series.DoChecks()
# Check the patches, and run them through 'git am' just to be sure
-ok = checkpatch.CheckPatches(options.verbose, args)
+if options.check_patch:
+ok = checkpatch.CheckPatches(options.verbose, args)
+else:
+ok = True
if not gitutil.ApplyPatches(options.verbose, args,
options.count + options.start):
ok = False
--
1.7.7.3
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v4] patman: Allow use outside of u-boot tree
To make it usable in git trees not providing a patch checker
implementation, add a command line option, allowing to suppress patch
check. While we are at it, sort debug options alphabetically.
Also, do not raise an exception if checkpatch.pl is not found - just
print an error message suggesting to use the new option, and return
nonzero status.
. unit test passes:
$ ./patman -t
unittest.result.TestResult run=7 errors=0 failures=0
. successfully used patman in the autotest tree to generate a patch
email (with --no-check option)
. successfully used patman in the u-boot tree to generate a patch
email
. `patman --help' now shows command line options ordered
alphabetically
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
Changes in v4:
. now properly corrected typo in the error message
Changes in v3:
. corrected typo in the error message
Changes in v2:
. addressed comments WRT use of double negative
. added code to gracefully handle absence of checkpatch.cl
tools/patman/checkpatch.py | 10 +-
tools/patman/patman.py | 14 ++
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py
index f72f8ee..d3a0477 100644
--- a/tools/patman/checkpatch.py
+++ b/tools/patman/checkpatch.py
@@ -23,6 +23,7 @@ import command
import gitutil
import os
import re
+import sys
import terminal
def FindCheckPatch():
@@ -47,8 +48,10 @@ def FindCheckPatch():
if os.path.isfile(fname):
return fname
path = os.path.dirname(path)
-print 'Could not find checkpatch.pl'
-return None
+
+print sys.stderr, ('Cannot find checkpatch.pl - please put it in your '
+
+'~/bin directory or use --no-check')
+sys.exit(1)
def CheckPatch(fname, verbose=False):
Run checkpatch.pl on a file.
@@ -67,9 +70,6 @@ def CheckPatch(fname, verbose=False):
error_count, warning_count, lines = 0, 0, 0
problems = []
chk = FindCheckPatch()
-if not chk:
-raise OSError, ('Cannot find checkpatch.pl - please put it in your ' +
-'~/bin directory')
item = {}
stdout = command.Output(chk, '--no-tree', fname)
#pipe = subprocess.Popen(cmd, stdout=subprocess.PIPE)
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index e56dd01..e049081 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -50,6 +50,9 @@ parser.add_option('-i', '--ignore-errors',
action='store_true',
help='Send patches email even if patch errors are found')
parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
default=False, help=Do a try run (create but don't email patches))
+parser.add_option('-p', '--project', default=project.DetectProject(),
+ help=Project name; affects default option values and
+ aliases [default: %default])
parser.add_option('-s', '--start', dest='start', type='int',
default=0, help='Commit to start creating patches from (0 = HEAD)')
parser.add_option('-t', '--test', action='store_true', dest='test',
@@ -58,11 +61,11 @@ parser.add_option('-v', '--verbose', action='store_true',
dest='verbose',
default=False, help='Verbose output of errors and warnings')
parser.add_option('--cc-cmd', dest='cc_cmd', type='string', action='store',
default=None, help='Output cc list for patch file (used by git)')
+parser.add_option('--no-check', action='store_false', dest='check_patch',
+ default=True,
+ help=Don't check for patch compliance)
parser.add_option('--no-tags', action='store_false', dest='process_tags',
default=True, help=Don't process subject tags as aliaes)
-parser.add_option('-p', '--project', default=project.DetectProject(),
- help=Project name; affects default option values and
- aliases [default: %default])
parser.usage = patman [options]
@@ -146,7 +149,10 @@ else:
series.DoChecks()
# Check the patches, and run them through 'git am' just to be sure
-ok = checkpatch.CheckPatches(options.verbose, args)
+if options.check_patch:
+ok = checkpatch.CheckPatches(options.verbose, args)
+else:
+ok = True
if not gitutil.ApplyPatches(options.verbose, args,
options.count + options.start):
ok = False
--
1.7.7.3
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 15/20] Add console command to access io space registers
On Wed, Dec 5, 2012 at 5:23 PM, Fabio Estevam feste...@gmail.com wrote: On Wed, Dec 5, 2012 at 10:46 PM, Simon Glass s...@chromium.org wrote: From: Vadim Bendebury vben...@chromium.org Provide u-boot console functions to access IO space registers. A no thrills implementation, accessing one register at a time. For example: boot iod 80 0080: 0094 boot iod.w 80 0080: 0094 boot iod.b 80 0080: 94 boot iow.b 0x80 12 boot iod 0x80 0080: 0012 Doesn't md/mw accomplish the same? No - md/mw access memory, the new commands access the IO space (which is an x86 architecture property). cheers, /vb Regards, Fabio Estevam ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 15/17] Add console command to access io space registers
On Sat, Nov 3, 2012 at 8:26 AM, Wolfgang Denk w...@denx.de wrote: Dear Simon Glass, In message 1351902453-27956-16-git-send-email-...@chromium.org you wrote: From: Vadim Bendebury vben...@chromium.org Provide u-boot console functions to access IO space registers. A no thrills implementation, accessing one register at a time. For example: boot iod 80 0080: 0094 boot iod.w 80 0080: 0094 boot iod.b 80 0080: 94 boot iow.b 0x80 12 boot iod 0x80 0080: 0012 What are the exact use cases where this is necessary, i. e. where plain md / mw does not work? Wolfgang, the x86 architecture defines this different kind of address space, device IO, in addition to memory address space. This is a remnant of the days when address lines were sparse. On typical x86 systems devices are accessed through this device IO space, and this patch adds a command which allows the operator to read and write device registers on such systems. 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 Quantum particles: The dreams that stuff is made of. ___ 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] tpm: Remove the LPC TPM driver
On Sun, Sep 30, 2012 at 10:36 AM, Marek Vasut ma...@denx.de wrote: The driver is not used anywhere. It is a dead code and it seems to be falling apart slowly due to bitrot. Remove this driver. This driver is used in several chromeOS devices which have not quite made it upstream yet. Why does it seem falling apart? cheers, /vb Signed-off-by: Marek Vasut ma...@denx.de Cc: Vadim Bendebury vben...@chromium.org Cc: Tom Rini tr...@ti.com --- Makefile |1 - README| 10 - drivers/tpm/Makefile | 43 drivers/tpm/generic_lpc_tpm.c | 495 - include/tpm.h | 71 -- 5 files changed, 620 deletions(-) delete mode 100644 drivers/tpm/Makefile delete mode 100644 drivers/tpm/generic_lpc_tpm.c delete mode 100644 include/tpm.h diff --git a/Makefile b/Makefile index a40d4cc..1325be0 100644 --- a/Makefile +++ b/Makefile @@ -293,7 +293,6 @@ LIBS-y += arch/powerpc/cpu/mpc8xxx/lib8xxx.o endif LIBS-y += drivers/rtc/librtc.o LIBS-y += drivers/serial/libserial.o -LIBS-$(CONFIG_GENERIC_LPC_TPM) += drivers/tpm/libtpm.o LIBS-y += drivers/twserial/libtws.o LIBS-y += drivers/usb/eth/libusb_eth.o LIBS-y += drivers/usb/gadget/libusb_gadget.o diff --git a/README b/README index a745d0b..c4ad6d8 100644 --- a/README +++ b/README @@ -1149,16 +1149,6 @@ 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 deleted file mode 100644 index be11c8b..000 --- a/drivers/tpm/Makefile +++ /dev/null @@ -1,43 +0,0 @@ -# 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 deleted file mode 100644 index 6c494eb..000 --- a/drivers/tpm/generic_lpc_tpm.c +++ /dev/null @@ -1,495 +0,0 @@ -/* - * 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 - */ - -/* - * The code in this file is based on the article Writing a TPM Device Driver - * published on http://ptgmedia.pearsoncmg.com. - * - * One
Re: [U-Boot] [PATCH] Fix compilation warnings when building eNET
On Sat, Dec 10, 2011 at 2:42 AM, Graeme Russ graeme.r...@gmail.com wrote:
Hi Vadim,
Oops, dropped the ML...
On Dec 10, 2011 9:20 PM, Graeme Russ graeme.r...@gmail.com wrote:
Hi Vadim,
On Dec 10, 2011 12:14 PM, Graeme Russ graeme.r...@gmail.com wrote:
Hi Vadim,
On Dec 10, 2011 12:08 PM, Vadim Bendebury vben...@chromium.org
wrote:
On Fri, Dec 9, 2011 at 3:57 PM, Graeme Russ graeme.r...@gmail.com
wrote:
Hi Vadim,
On Dec 10, 2011 10:31 AM, Vadim Bendebury vben...@chromium.org
wrote:
There have been a couple of unused variable cases, causing
compilation
warnings when building the eNET target.
While the board/eNET/eNET.c:last_stage_init() case seems a
leftover
from a quick edit, the
arch/x86/cpu/sc520/sc520_timer.c:sc520_udelay()
seems to actually require accessing the registers discarding the
values.
Thanks for picking this up, I've been a bit preoccupie lately.
I'll need to look a bit more closely, but there should be no need
for such trickery...
Regards,
Graeme
Hi Graeme, thank you for a quick response.
Do you mean that there is no need to read the registers before the
actual udelay functionality or do you have another way of pacifying
the compiler in mind?
On closer inspection, I can't think of a better way
Acked-by: Graeme Russ graeme.r...@gmail.com
Sorry, bit I just had a closer look at this and after reading the
datasheet I've come to the realization that the udelay function is broken -
not badly, but it can timeout up to 1ms early.
The read of swtmrmilli reads the current value of the millisecond timer,
latches the value of the internal microsecond timer and resets the
millisecond timer to zero
The read of swtmrmicro reads the latched microsecond value
The code assumes that reading swtmrmicro resets the microsecond counter,
which it does not. So if the internal microsecond timer is, say, 900 then
udelay(500) for example will return without any delay at all
I need to fix this, but cannot do so any time soon...
I have no objection to the compiler warning fix, but is there any point in
applying a cosmetic fix to broken code?
I'm not near my dev PC so the formatting is all wrong, but this should fix
the bug and the compiler warning:
void sc520_udelay(unsigned long usec)
{
int m;
long u;
long start_us;
/* Reset millisecond counter */
m = readw(sc520_mmcr-swtmrmilli);
m = 0;
Looks reasonable, not much difference from the original submission IMO.
The important thing is that there should be no compilation warnings
allowed, as if there are a few 'legitimate' ones, it becomes easy to
let the illegitimate ones to slip through :P
cheers,
/vb
/* Read initial microsecond count */
start_us = readw(sc520_mmcr-swtmrmicro);
do {
m += readw(sc520_mmcr-swtmrmilli);
u = readw(sc520_mmcr-swtmrmicro) + (m * 1000);
} while ((u - start_us) usec);
}
Regards,
Graeme
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] Fix compilation warnings when building eNET
There have been a couple of unused variable cases, causing compilation
warnings when building the eNET target.
While the board/eNET/eNET.c:last_stage_init() case seems a leftover
from a quick edit, the arch/x86/cpu/sc520/sc520_timer.c:sc520_udelay()
seems to actually require accessing the registers discarding the
values.
The source code is being modified accordingly.
TEST:
- the eNET target now builds cleanly
- examining disassembled sc520_timer.o shows that the registers are
still being accessed in the beginning of the function.
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
arch/x86/cpu/sc520/sc520_timer.c |1 +
board/eNET/eNET.c|5 -
2 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/arch/x86/cpu/sc520/sc520_timer.c b/arch/x86/cpu/sc520/sc520_timer.c
index 495a694..a7bbe92 100644
--- a/arch/x86/cpu/sc520/sc520_timer.c
+++ b/arch/x86/cpu/sc520/sc520_timer.c
@@ -87,4 +87,5 @@ void sc520_udelay(unsigned long usec)
m += readw(sc520_mmcr-swtmrmilli);
u = readw(sc520_mmcr-swtmrmicro) + (m * 1000);
} while (u usec);
+ (void) temp;
}
diff --git a/board/eNET/eNET.c b/board/eNET/eNET.c
index 429fe1b..2f26470 100644
--- a/board/eNET/eNET.c
+++ b/board/eNET/eNET.c
@@ -178,11 +178,6 @@ void show_boot_progress(int val)
int last_stage_init(void)
{
- int minor;
- int major;
-
- major = minor = 0;
-
outb(0x00, LED_LATCH_ADDRESS);
register_timer_isr(enet_timer_isr);
--
1.7.3.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] Fix compilation warnings when building eNET
There have been a couple of unused variable cases, causing compilation
warnings when building the eNET target.
While the board/eNET/eNET.c:last_stage_init() case seems a leftover
from a quick edit, the arch/x86/cpu/sc520/sc520_timer.c:sc520_udelay()
seems to actually require accessing the registers discarding the
values.
The source code is being modified accordingly.
TEST:
- the eNET target now builds cleanly
- examining disassembled sc520_timer.o shows that the registers are
still being accessed in the beginning of the function.
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
[Resend with the proper cc: header]
arch/x86/cpu/sc520/sc520_timer.c | 1 +
board/eNET/eNET.c | 5 -
2 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/arch/x86/cpu/sc520/sc520_timer.c b/arch/x86/cpu/sc520/sc520_timer.c
index 495a694..a7bbe92 100644
--- a/arch/x86/cpu/sc520/sc520_timer.c
+++ b/arch/x86/cpu/sc520/sc520_timer.c
@@ -87,4 +87,5 @@ void sc520_udelay(unsigned long usec)
m += readw(sc520_mmcr-swtmrmilli);
u = readw(sc520_mmcr-swtmrmicro) + (m * 1000);
} while (u usec);
+ (void) temp;
}
diff --git a/board/eNET/eNET.c b/board/eNET/eNET.c
index 429fe1b..2f26470 100644
--- a/board/eNET/eNET.c
+++ b/board/eNET/eNET.c
@@ -178,11 +178,6 @@ void show_boot_progress(int val)
int last_stage_init(void)
{
- int minor;
- int major;
-
- major = minor = 0;
-
outb(0x00, LED_LATCH_ADDRESS);
register_timer_isr(enet_timer_isr);
--
1.7.3.1
___
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] Fix compilation warnings when building eNET
On Fri, Dec 9, 2011 at 3:57 PM, Graeme Russ graeme.r...@gmail.com wrote:
Hi Vadim,
On Dec 10, 2011 10:31 AM, Vadim Bendebury vben...@chromium.org wrote:
There have been a couple of unused variable cases, causing compilation
warnings when building the eNET target.
While the board/eNET/eNET.c:last_stage_init() case seems a leftover
from a quick edit, the arch/x86/cpu/sc520/sc520_timer.c:sc520_udelay()
seems to actually require accessing the registers discarding the
values.
Thanks for picking this up, I've been a bit preoccupie lately.
I'll need to look a bit more closely, but there should be no need for such
trickery...
Regards,
Graeme
Hi Graeme, thank you for a quick response.
Do you mean that there is no need to read the registers before the
actual udelay functionality or do you have another way of pacifying
the compiler in mind?
cheers,
/v
The source code is being modified accordingly.
TEST:
- the eNET target now builds cleanly
- examining disassembled sc520_timer.o shows that the registers are
still being accessed in the beginning of the function.
Signed-off-by: Vadim Bendebury vben...@chromium.org
---
[Resend with the proper cc: header]
arch/x86/cpu/sc520/sc520_timer.c | 1 +
board/eNET/eNET.c | 5 -
2 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/arch/x86/cpu/sc520/sc520_timer.c
b/arch/x86/cpu/sc520/sc520_timer.c
index 495a694..a7bbe92 100644
--- a/arch/x86/cpu/sc520/sc520_timer.c
+++ b/arch/x86/cpu/sc520/sc520_timer.c
@@ -87,4 +87,5 @@ void sc520_udelay(unsigned long usec)
m += readw(sc520_mmcr-swtmrmilli);
u = readw(sc520_mmcr-swtmrmicro) + (m * 1000);
} while (u usec);
+ (void) temp;
}
diff --git a/board/eNET/eNET.c b/board/eNET/eNET.c
index 429fe1b..2f26470 100644
--- a/board/eNET/eNET.c
+++ b/board/eNET/eNET.c
@@ -178,11 +178,6 @@ void show_boot_progress(int val)
int last_stage_init(void)
{
- int minor;
- int major;
-
- major = minor = 0;
-
outb(0x00, LED_LATCH_ADDRESS);
register_timer_isr(enet_timer_isr);
--
1.7.3.1
___
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
[U-Boot] Fwd: [PATCH v6 1/2] Introduce generic TPM support in u-boot
Dear Wolfgang Denk, On Tue, Dec 6, 2011 at 11:47 PM, Wolfgang Denk w...@denx.de wrote: Dear Vadim Bendebury, [..] Applied, thanks. Thank you! But _please_ get used to providing full change logs to your patches. This is patch v6, so I would like to see a history for v2, v3, v4, v5 and v6 - but all you have is a totally useless (as incomprehensible) comment for v5. Do you mean all newer patch versions are supposed to include logs of all previous patch versions? I can do that, somehow nobody mentioned this omission while reviewing previous patches (up to version 5). 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 Anyone who doesn't believe in miracles is not a realist. - David Ben Gurion ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v6 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 vben...@chromium.org CC: Wolfgang Denk w...@denx.de --- v5 with A function comment header correction. Makefile |3 + README| 10 + drivers/tpm/Makefile | 43 drivers/tpm/generic_lpc_tpm.c | 495 + include/tpm.h | 71 ++ 5 files changed, 622 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..6c494eb --- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c @@ -0,0 +1,495 @@ +/* + * 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
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
Wolfgang Denk, On Sun, Oct 16, 2011 at 12:35 AM, Wolfgang Denk w...@denx.de wrote: Dear Vadim Bendebury, In message CANy1bu+DZmD_Z=au8ldtuy7ewhfwmu-8pvh1oa1jba9tz1x...@mail.gmail.com you wrote: 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? I explained this yesterday, too. Functions are preferred over macros. In this case ther eis no reason not to use a function. 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 No, you don't have to agree. But we also don't have to accept code that we don't like ;-) certainly, but wouldn't you risk to throw the baby out with the bath water this way? ;-) Also, what about situations when one reviewer requests a certain implementation and another one finds it inappropriate? somebody else to confirm that using a macro in this case in inappropriate? I already did. Thank you, I will fix this. Can you please also confirm that having a structure with a single element as an array is weird and must be changed to passing around a pointer to a single element without the size (or maybe the idea is that the pointer AND the size need to be passed around)? Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions? Please advise, 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 average woman would rather have beauty than brains, because the average man can see better than he can think. ___ 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
[U-Boot] [PATCH v4 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 vben...@chromium.org CC: Wolfgang Denk w...@denx.de --- v4 converts BURST_COUNT into a function. No explicit 'inline' keyword is used, as it is considered unnecessary with current gcc in cases like this. Side by side diffs between v4 and v3 can be seen at http://review-t.appspot.com/23001/diff2/2022:7002/drivers/tpm/generic_lpc_tpm.c Makefile |3 + README| 10 + drivers/tpm/Makefile | 43 drivers/tpm/generic_lpc_tpm.c | 487 + include/tpm.h | 71 ++ 5 files changed, 614 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..ba97707 --- /dev/null +++ b/drivers/tpm/generic_lpc_tpm.c @@ -0,0 +1,487
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
On Sun, Oct 16, 2011 at 5:28 AM, Marek Vasut marek.va...@gmail.com wrote:
On Sunday, October 16, 2011 05:45:40 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 8:31 PM, Marek Vasut marek.va...@gmail.com wrote:
On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut marek.va...@gmail.com
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 marek.va...@gmail.com
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.
Would you mind answering my second question please ?
I wrote this from scratch.
[..]
+
+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.
TPM_TOTAL_LOCALITIES big ?
I believe it is clearer when this information is included in a
structure describing the chip (as opposed to the array size being a
separate #define)
[..]
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?
converted BURST_COUNT into a function.
+
+/*
+ * 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
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
Dear Wolfgang Denk, On Sun, Oct 16, 2011 at 1:04 PM, Wolfgang Denk w...@denx.de wrote: Dear Vadim Bendebury, In message cany1bujcjdqnpq6ehtjj3jovge5fkqdodpevofhu6_-2y7l...@mail.gmail.com you wrote: Also, what about situations when one reviewer requests a certain implementation and another one finds it inappropriate? Here we had several people (Marek and me) asking the same thing. And actually my message was intended to tell you that I agree with Marek. In case of doubt, someone has to make a final decision. In this specific case I already decided (and told you) that I want to see a function instead of a macro. yes, I saw it and have already changed the implementation. Can you please also confirm that having a structure with a single element as an array is weird and must be changed to passing around a pointer to a single element without the size (or maybe the idea is that the pointer AND the size need to be passed around)? Yes, I consider this weird, too. And you failed to provide a good explanation why you think this would be needed so far. My explanation is that it is better readable when the entire information about a chip is contained in one type, as opposed to having the size separate and needed to be carried around (instead of using ARRAY_SIZE() when needed). When the pointer to the chip structure is passed around, the recipient does not have to be aware of a separate definition, all information about the chip is contained in a structure. Sorry, I am restating the reasons here because I am not sure if you wrote your previous reply before seeing them. Please let me know if you don't find this explanation convincing, I will change the code as you suggest. Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions? Sorry, I don't remember which code you are referring to here. I am referring to this message: http://lists.denx.de/pipermail/u-boot/2011-October/104780.html which is a part of this thread: http://lists.denx.de/pipermail/u-boot/2011-October/104665.html 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 I'm growing older, but not up. - Jimmy Buffett ___ 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 Wolfgang Denk, On Sun, Oct 16, 2011 at 1:31 PM, Wolfgang Denk w...@denx.de wrote: Dear Vadim Bendebury, In message cac3gerh1gpltefh3eli5ebtjpz0hlcjxj3yx3gguu9hgche...@mail.gmail.com you wrote: Yes, I consider this weird, too. And you failed to provide a good explanation why you think this would be needed so far. My explanation is that it is better readable when the entire information about a chip is contained in one type, as opposed to having the size separate and needed to be carried around (instead of using ARRAY_SIZE() when needed). When the pointer to the chip structure is passed around, the recipient does not have to be aware of a separate definition, all information about the chip is contained in a structure. I read your explanation, but I still fail to uinderstand why we need an additinal wrapper around the internal strcut, which is the only element. Sorry, I am restating the reasons here because I am not sure if you wrote your previous reply before seeing them. I did. That's why I wrote you failed to provide a _good_ explanation. ok. Please let me know if you don't find this explanation convincing, I will change the code as you suggest. Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions? Sorry, I don't remember which code you are referring to here. I am referring to this message: http://lists.denx.de/pipermail/u-boot/2011-October/104780.html Yes, I know that. But why would one macro turn into two inline functions? because this chip is sensitive to the access cycle size (byte versus word). When using macros one can rely on the passed in pointer type to decide which access to use (byte vs word). If using inline functions, there would be two separate functions required for read (byte and u32) and write (byte and u32). 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 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v5 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 vben...@chromium.org CC: Wolfgang Denk w...@denx.de --- The v5 patch does the following: - replaces the structure definition used to represent the TPM device with a pointer to a single locality. - replaces register wrapper macros with wrapper functions. Re-tested both by running the u-boot CLI command and by bringing up a verified ChromeOS image. The patch history can be seen at http://review-t.appspot.com/23001/ Makefile |3 + README| 10 + drivers/tpm/Makefile | 43 drivers/tpm/generic_lpc_tpm.c | 496 + include/tpm.h | 71 ++ 5 files changed, 623 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
Re: [U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
Dear Wolfgang Denk, On Sun, Oct 16, 2011 at 1:53 PM, Wolfgang Denk w...@denx.de wrote: Dear Vadim Bendebury, In message cany1buj0mhpdy948c+pm6u4eswtohw3+-u+m_fnig3bztbi...@mail.gmail.com you wrote: because this chip is sensitive to the access cycle size (byte versus word). When using macros one can rely on the passed in pointer type to decide which access to use (byte vs word). If using inline functions, there would be two separate functions required for read (byte and u32) and write (byte and u32). This would then be an improvement in the code, as we then can see what the code is actually doing. please see the latest patch (v5) which addresses the chip representation and register access wrapper comments. 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 What the gods would destroy they first submit to an IEEE standards committee. ___ 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 Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:02 AM, Marek Vasut marek.va...@gmail.com 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 data [data] - 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] [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 marek.va...@gmail.com 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;
+
+ 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
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 vap...@gentoo.org 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 2/2] Add a cli command to test the TPM device.
On Sat, Oct 15, 2011 at 1:01 PM, Mike Frysinger vap...@gentoo.org 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 2/2] Add a cli command to test the TPM device.
On Sat, Oct 15, 2011 at 12:08 PM, Mike Frysinger vap...@gentoo.org 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 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 w...@denx.de wrote:
Dear Vadim Bendebury,
In message
cac3gerhaagx39xjd04mnjwe3sa9xc087llpf6sycvc6k7sl...@mail.gmail.com 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 1/2] Introduce generic TPM support in u-boot
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut marek.va...@gmail.com 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 marek.va...@gmail.com 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 success.
So there's no case where this can loop endlessly ? fine.
+ 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__, __LINE__, len -
offset, len
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 w...@denx.de 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 12:42 PM, Mike Frysinger vap...@gentoo.org 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
[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 vben...@chromium.org CC: Wolfgang Denk w...@denx.de --- 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
[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 vben...@chromium.org
CC: Wolfgang Denk w...@denx.de
---
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 common.h
+#include command.h
+#include tpm.h
+
+#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,
+ byte [byte ...] - write data and read response,
+ send arbitrary data (at least 6 bytes) to the TPM
+ device and read the response
+);
--
1.7.3.1
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 vben...@chromium.org 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 vben...@chromium.org CC: Wolfgang Denk w...@denx.de --- 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
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 vben...@chromium.org 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 v2 1/2] Introduce generic TPM support in u-boot
On Sat, Oct 15, 2011 at 8:31 PM, Marek Vasut marek.va...@gmail.com wrote:
On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut marek.va...@gmail.com 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 marek.va...@gmail.com
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 object size at run
time.
This seems wrong like hell.
You are entitled to your opinion, but you should not be suggesting to
change this code to inline functions, because it would break it.
__v is needed to avoid side effects when invoking the macro.
Side effects ? What side effects ?
https://www.securecoding.cert.org/confluence/display/seccode/PRE31-C.+Avoid+side-effects+in+arguments+to+unsafe+macros
[...]
Cheers
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
cheers,
/vb
[U-Boot] [PATCH v2 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. Signed-off-by: Vadim Bendebury vben...@chromium.org CC: Wolfgang Denk w...@denx.de --- Makefile |3 + README| 11 + drivers/tpm/Makefile | 27 +++ drivers/tpm/generic_lpc_tpm.c | 488 + include/tpm.h | 54 + 5 files changed, 583 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 cd6fc8c..76f780a 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 0868531..1dced5a 100644 --- a/README +++ b/README @@ -1010,12 +1010,23 @@ The following options need to be configured: CONFIG_SH_ETHER_USE_PORT Define the number of ports to be used + CONFIG_SH_ETHER_PHY_ADDR Define the ETH PHY's address 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..7df586f --- /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. +# + +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..8319286 --- /dev/null +++ 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. + * + * 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
[U-Boot] [PATCH v2 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
Signed-off-by: Vadim Bendebury vben...@chromium.org
CC: Wolfgang Denk w...@denx.de
---
common/Makefile |2 +
common/cmd_tpm.c | 111 ++
2 files changed, 113 insertions(+), 0 deletions(-)
create mode 100644 common/cmd_tpm.c
diff --git a/common/Makefile b/common/Makefile
index 371a0d9..28c2ec5 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -153,6 +153,8 @@ COBJS-$(CONFIG_CMD_UBI) += cmd_ubi.o
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
+
ifdef CONFIG_CMD_USB
COBJS-y += cmd_usb.o
COBJS-y += usb.o
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.
+ *
+ * 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 common.h
+#include command.h
+#include tpm.h
+
+#define MAX_TRANSACTION_SIZE 30
+static void report_error(const char *msg);
+
+/*
+ * tpm_write() expects a variable number of parameters: the internal address
+ * followed by data to write, byte by byte.
+ *
+ * Returns 0 on success or ~0 on errors (wrong arguments or TPM failure).
+ */
+static int tpm_process(int argc, char * const argv[])
+{
+ u8 tpm_buffer[MAX_TRANSACTION_SIZE];
+ u32 write_size, read_size;
+ char *p;
+ int rv = ~0;
+
+ for (write_size = 0; write_size argc; write_size++) {
+ u32 datum = simple_strtoul(argv[write_size], p, 0);
+ if (*p || (datum 0xff)) {
+ printf(%s: , argv[write_size]);
+ report_error(bad data value\n);
+ 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 ~0;
+ }
+
+ 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;
+ }
+
+ return rv;
+}
+
+U_BOOT_CMD(tpm, MAX_TRANSACTION_SIZE, 1, do_tpm,
+ tpm data [data] - 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)
+ printf
Re: [U-Boot] [PATCH] Introduce generic TPM support in u-boot
Wolfgang, thank you for your comments, I'll address them in a
follow-up submission, but I have a question regarding the register
access (and the issue was indeed brought up by vapier@ at an earlier
review on a different submission).
On Mon, Oct 10, 2011 at 3:45 AM, Wolfgang Denk w...@denx.de wrote:
+#define TIS_REG(LOCALITY, REG) \
+ (void *)(CONFIG_TPM_TIS_BASE_ADDRESS + (LOCALITY 12) + REG)
We do not allow to access device registers through base address +
offset. Please always use C structs instead.
so, this chip has five different areas (localities) which have the
same structure, and are mapped at certain regular offsets inside the
chip.
thus the structure describing the chip would be something like
struct locality {
u16 field_a;
u8 field_b;
u32 field_c;
..
u8 padding[padding size];
} __packed;
struct tmp_chip {
struct locality localities[5];
} __packed;
which is very compiler dependent, but probably not the only place in
u-boot, so I could live with that if you could.
Yet another inconvenience though is the requirement to be able to
trace accesses to the registers. Some of the registers can be accessed
in 32 bit mode or 8 bit mode, and this determines how many bytes get
sent into/read from a data fifo. The tracing function should be able
to tell what mode the register was accessed in. A way I see to do it
through a structure layout is to define the same fields through
unions.
What I am getting at is that the code is much better readable as it is
now even though it is in violation of the 'use structure to access
registers' requirement.
Or maybe I am missing an obvious way to do it? Can you please elaborate,
cheers,
/vb
...
+/* TPM access functions are carved out to make tracing easier. */
+static u32 tpm_read(int locality, u32 reg)
+{
+ u32 value;
+ /*
+ * Data FIFO register must be read and written in byte access mode,
+ * otherwise the FIFO values are returned 4 bytes at a time.
+ */
Please insert blank line between declarations and code. Please fix
globally.
...
+ /* this will have to be converted into debug printout */
+ TPM_DEBUG(Found TPM %s by %s\n, device_name, vendor_name);
Is this comment still correct?
+int tis_init(void)
+{
+ if (tis_probe())
+ return TPM_DRIVER_ERR;
+ return 0;
+}
Or simply:
return tis_probe();
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
Hegel was right when he said that we learn from history that man can
never learn anything from history. - George Bernard Shaw
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Introduce generic TPM support in u-boot
On Mon, Oct 10, 2011 at 1:48 PM, Wolfgang Denk w...@denx.de wrote: [..] Yet another inconvenience though is the requirement to be able to trace accesses to the registers. Some of the registers can be accessed in 32 bit mode or 8 bit mode, and this determines how many bytes get Can you not always use one of the modes only? I sure can, but what still is not clear to me - how to provide the ability to trace when using structure pointer dereferencing. TPM is a tricky device, and the ability to trace all accesses made debugging much easier. Is there an example of how tracing should be done when the device is referenced through a memory structure, or do you suggest that the tracing should be dropped? cheers, /vb What I am getting at is that the code is much better readable as it is now even though it is in violation of the 'use structure to access registers' requirement. The purpose is to enable the compiler to perform type checking. 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 Wait! You have not been prepared! -- Mr. Atoz, Tomorrow is Yesterday, stardate 3113.2 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] 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
and a submission by Stefan Berger on Qemu-devel mailing list
(http://lists.gnu.org/archive/html/qemu-devel).
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 will be submitted separately.
Change-Id: I22a33c3e5b2e20eec9557a7621bd463b30389d73
Signed-off-by: Vadim Bendebury vben...@chromium.org
CC: Wolfgang Denk w...@denx.de
---
Makefile |3 +
README|8 +
drivers/tpm/Makefile | 27 ++
drivers/tpm/generic_lpc_tpm.c | 523 +
include/tpm.h | 17 ++
5 files changed, 578 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 cd6fc8c..996db11 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
+ifneq ($(CONFIG_GENERIC_LPC_TPM),)
+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 0868531..e831d68 100644
--- a/README
+++ b/README
@@ -1016,6 +1016,14 @@ 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.
+
+ CONFIG_TPM_TIS_BASE_ADDRESS
+ Base address where the generic TPM device is mapped
+ to. Default value is 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..abe1180
--- /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.
+#
+
+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)
+ $(AR) $(ARFLAGS) $@ $(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..939f715
--- /dev/null
+++ b/drivers/tpm/generic_lpc_tpm.c
@@ -0,0 +1,523 @@
+/* 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.
+ *
+ * The code in this file has been heavily based on the article Writing a TPM
+ * Device Driver published on http://ptgmedia.pearsoncmg.com and the
+ * submission by Stefan Berger on Qemu-devel mailing list.
+ *
+ * One principal difference is that in the simplest config the other than 0
+ * TPM localities do not get mapped by some devices (for instance, by
+ * Infineon slb9635), so this driver provides access to locality 0 only.
+ */
+
+/* #define DEBUG */
+#include common.h
+#include asm/io.h
+#include tpm.h
+
+#ifdef DEBUG
+#define TPM_DEBUG_ON 1
+#else
+#define TPM_DEBUG_ON 0
+#endif
+
+#define PREFIX lpc_tpm:
+#defineTPM_DEBUG(fmt, args...) \
+ if (TPM_DEBUG_ON) { \
+ printf(PREFIX); \
+ printf(fmt , ##args); \
+ }
+
+#ifndef
Re: [U-Boot] [PATCH V4] console: Implement pre-console buffer
On Thu, Sep 29, 2011 at 4:15 PM, Graeme Russ graeme.r...@gmail.com wrote:
Hi Vadim,
On Wed, Sep 28, 2011 at 12:55 AM, Vadim Bendebury vben...@chromium.org
wrote:
On Tue, Sep 27, 2011 at 4:22 AM, Graeme Russ graeme.r...@gmail.com wrote:
Hi Vadim,
On 27/09/11 08:50, Vadim Bendebury wrote:
On Wed, Aug 31, 2011 at 5:58 AM, Graeme Russ graeme.r...@gmail.com wrote:
[snip]
Typically, the pre-console buffer would exist in the CPU cache (or similar
non-(S)DRAM location)
hi Graeme,
Actually, there are many cases when u-boot starts running with memory
fully initialized - ARM platforms is one case and coreboot/u-boot
combination on x86 is another, but in general, yes, this buffer could
be mapped to the internal CPU memory nailed to a fixed address.
And we have to satisfy the 'absolute majority', not the 'many' or the
'simply majority'. And I'm not sure it's always true that ARM platforms
have fully initialised SDRAM when U-Boot starts
- all console output needs to be saved, not just until the moment when
the console hardware is initialized.
That could be a _huge_ amount of info and highly variable. Remember, the
available space for a pre-console buffer could be tiny. If this is needed,
then maybe look at forking stdio instead (one branch to console, one branch
to you console buffer)
Sure, if the room in the preallocated buffer is not enough, only the
most recent data fitting in the space would be kept.
I don't quite understand what you mean by forking stdio. I was
Search for CONFIG_CONSOLE_MUX - There appears to be support for sending
stdout to multiple output devices:
static void console_putc(int file, const char c)
{
int i;
struct stdio_dev *dev;
for (i = 0; i cd_count[file]; i++) {
dev = console_devices[file][i];
if (dev-putc != NULL)
dev-putc(c);
}
}
I don't know have to register additional devices though
hi Graeme,
sure, I understand how the console mux works, i was not sure you were
referring to it.
thinking about introducing a separate driver for this memory stored
console output, but sjg@ explained that while running from ROM u-boot
supports only one console interface, so there is no way to have
console stream sent to more than one driver before relocation.
Yes, while running from ROM your options are very limited, but if you have
a console buffer big enough to get you into RAM you can do lot more
I could work on top of this patch and send another one once this one
has been accepted. May I suggest an improvement though:
is it really necessary to store the index in the global data
structure. This requires editing all these .h files adding another
unsighty conditionally compiled field. Why not to store the index as
the first word in the buffer allocated for this temp storage?
I like this - but instead:
struct pre_con_buff {
int idx;
char *buffer[CONFIG_PRE_CON_BUF_SZ];
}
struct pre_con_buff *pre_con_buffer;
pre_con_buffer = (struct pre_con_buff *)CONFIG_PRE_CON_BUF_ADDR;
yes, this is exactly what I meant,
Thinking more about this, I think I prefer the current patch for two
reasons:
1) gd is guaranteed to be cleared - The memory holding the buffer is not
so you would need to initialise it somehow - That could mean splitting
the init for each arch
doesn't each console type have an init routine? this would be a place
to initialize the header.
2) pre_con_buffer is larger than CONFIG_PRE_CON_BUF_SZ. This will need to
be taken into consideration if the buffer is being crammed into a very
tightly crafted memory map - Forgetting to take this into account is
going to cause lots of pain. Now you could do:
struct pre_con_buff {
u16 idx;
char *buffer[CONFIG_PRE_CON_BUF_SZ - 2];
}
I actually have just implemented this for coreboot. It has its own
idiosyncrasies of course, the console buffer needs to be kept in three
different places and the contents copied three times on the way up.
I used this structure for the log buffer:
struct pre_con_buff {
u16 size;
u16 idx;
char buffer[0]
};
Then, the initialization code would just get the memory area address
and size, overlay this structure on top of it and set the size field
to
area size - sizeof(struct pre_con_buff)
yes, this results in a non power of two buffer size, but IMO the
convenience of keeping everything in one place and (and not changing
multiple .h files) is worth the lost performance of not being able to
utilize power of two arithmetic optimization (which I think is
negligible in any case).
cheers,
/vb
but the buffer size should really be a power two (so the compiler
optimises the divides into shifts) - So now we have to define
CONFIG_PRE_CON_BUF_SZ as say 258. It's starting to get messy
Regards,
Graeme
___
U
Re: [U-Boot] [PATCH V4] console: Implement pre-console buffer
On Tue, Sep 27, 2011 at 4:22 AM, Graeme Russ graeme.r...@gmail.com wrote:
Hi Vadim,
On 27/09/11 08:50, Vadim Bendebury wrote:
On Wed, Aug 31, 2011 at 5:58 AM, Graeme Russ graeme.r...@gmail.com wrote:
Allow redirection of console output prior to console initialisation to a
temporary buffer.
To enable this functionality, the board configuration file must define:
- CONFIG_PRE_CONSOLE_BUFFER - Enable pre-console buffer
- CONFIG_PRE_CON_BUF_ADDR - Base address of pre-console buffer
- CONFIG_PRE_CON_BUF_SZ - Size of pre-console buffer (in bytes)
The pre-console buffer will buffer the last CONFIG_PRE_CON_BUF_SZ bytes
Any earlier characters are silently dropped.
Signed-off-by: Graeme Russ graeme.r...@gmail.com
[snip]
I know I am late to the party here, but all of a sudden I need to
implement something similar, albeit slightly different:
better late than never :)
- the memory could be allocated by the cold bootprom which starts u-boot;
Typically, the pre-console buffer would exist in the CPU cache (or similar
non-(S)DRAM location)
hi Graeme,
Actually, there are many cases when u-boot starts running with memory
fully initialized - ARM platforms is one case and coreboot/u-boot
combination on x86 is another, but in general, yes, this buffer could
be mapped to the internal CPU memory nailed to a fixed address.
- all console output needs to be saved, not just until the moment when
the console hardware is initialized.
That could be a _huge_ amount of info and highly variable. Remember, the
available space for a pre-console buffer could be tiny. If this is needed,
then maybe look at forking stdio instead (one branch to console, one branch
to you console buffer)
Sure, if the room in the preallocated buffer is not enough, only the
most recent data fitting in the space would be kept.
I don't quite understand what you mean by forking stdio. I was
thinking about introducing a separate driver for this memory stored
console output, but sjg@ explained that while running from ROM u-boot
supports only one console interface, so there is no way to have
console stream sent to more than one driver before relocation.
I could work on top of this patch and send another one once this one
has been accepted. May I suggest an improvement though:
is it really necessary to store the index in the global data
structure. This requires editing all these .h files adding another
unsighty conditionally compiled field. Why not to store the index as
the first word in the buffer allocated for this temp storage?
I like this - but instead:
struct pre_con_buff {
int idx;
char *buffer[CONFIG_PRE_CON_BUF_SZ];
}
struct pre_con_buff *pre_con_buffer;
pre_con_buffer = (struct pre_con_buff *)CONFIG_PRE_CON_BUF_ADDR;
yes, this is exactly what I meant,
cheers,
/vb
Regards,
Graeme
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V4] console: Implement pre-console buffer
On Wed, Aug 31, 2011 at 5:58 AM, Graeme Russ graeme.r...@gmail.com wrote:
Allow redirection of console output prior to console initialisation to a
temporary buffer.
To enable this functionality, the board configuration file must define:
- CONFIG_PRE_CONSOLE_BUFFER - Enable pre-console buffer
- CONFIG_PRE_CON_BUF_ADDR - Base address of pre-console buffer
- CONFIG_PRE_CON_BUF_SZ - Size of pre-console buffer (in bytes)
The pre-console buffer will buffer the last CONFIG_PRE_CON_BUF_SZ bytes
Any earlier characters are silently dropped.
Signed-off-by: Graeme Russ graeme.r...@gmail.com
---
Changes since V3
- Fixed blank subject caused by gap between the Cc: list and Date:
Changes since V2
- Cast buffer size to unsigned long to help compilers produce tighter
code
- Use inline stub functions to reduce #ifdef clutter
- Add documentation to README
Changes Since V1
- Implemented circular buffer
- Trivial code styl corrections
README | 14 +
arch/arm/include/asm/global_data.h | 3 ++
arch/avr32/include/asm/global_data.h | 3 ++
arch/blackfin/include/asm/global_data.h | 3 ++
arch/m68k/include/asm/global_data.h | 3 ++
arch/microblaze/include/asm/global_data.h | 3 ++
arch/mips/include/asm/global_data.h | 3 ++
arch/nios2/include/asm/global_data.h | 3 ++
arch/powerpc/include/asm/global_data.h | 3 ++
arch/sh/include/asm/global_data.h | 3 ++
arch/sparc/include/asm/global_data.h | 3 ++
arch/x86/include/asm/global_data.h | 3 ++
I know I am late to the party here, but all of a sudden I need to
implement something similar, albeit slightly different:
- the memory could be allocated by the cold bootprom which starts u-boot;
- all console output needs to be saved, not just until the moment when
the console hardware is initialized.
I could work on top of this patch and send another one once this one
has been accepted. May I suggest an improvement though:
is it really necessary to store the index in the global data
structure. This requires editing all these .h files adding another
unsighty conditionally compiled field. Why not to store the index as
the first word in the buffer allocated for this temp storage?
cheers,
/vb
common/console.c | 43 +++-
13 files changed, 88 insertions(+), 2 deletions(-)
diff --git a/README b/README
index 0886987..170e67b 100644
--- a/README
+++ b/README
@@ -619,6 +619,20 @@ The following options need to be configured:
must be defined, to setup the maximum idle timeout for
the SMC.
+- Pre-Console Buffer:
+ Prior to the console being initialised (i.e. serial UART
+ initialised etc) all console output is silently discarded.
+ Defining CONFIG_PRE_CONSOLE_BUFFER will cause U-Boot to
+ buffer any console messages prior to the console being
+ initialised to a buffer of size CONFIG_PRE_CON_BUF_SZ
+ bytes located at CONFIG_PRE_CON_BUF_ADDR. The buffer is
+ a cicular buffer, so if more than CONFIG_PRE_CON_BUF_SZ
+ bytes are output before the console is initialised, the
+ earlier bytes are discarded.
+
+ 'Sane' compilers will generate smaller code if
+ CONFIG_PRE_CON_BUF_SZ is a power of 2
+
- Boot Delay: CONFIG_BOOTDELAY - in seconds
Delay before automatically booting the default image;
set to -1 to disable autoboot.
diff --git a/arch/arm/include/asm/global_data.h
b/arch/arm/include/asm/global_data.h
index 4fc51fd..b85b7fe 100644
--- a/arch/arm/include/asm/global_data.h
+++ b/arch/arm/include/asm/global_data.h
@@ -38,6 +38,9 @@ typedef struct global_data {
unsigned long flags;
unsigned long baudrate;
unsigned long have_console; /* serial_init() was called */
+#ifdef CONFIG_PRE_CONSOLE_BUFFER
+ unsigned long precon_buf_idx; /* Pre-Console buffer index */
+#endif
unsigned long env_addr; /* Address of Environment struct */
unsigned long env_valid; /* Checksum of Environment valid? */
unsigned long fb_base; /* base address of frame buffer */
diff --git a/arch/avr32/include/asm/global_data.h
b/arch/avr32/include/asm/global_data.h
index 4ef8fc5..5c654bd 100644
--- a/arch/avr32/include/asm/global_data.h
+++ b/arch/avr32/include/asm/global_data.h
@@ -38,6 +38,9 @@ typedef struct global_data {
unsigned long baudrate;
unsigned long stack_end; /* highest stack address */
unsigned long have_console; /* serial_init() was called */
+#ifdef CONFIG_PRE_CONSOLE_BUFFER
+ unsigned long precon_buf_idx; /* Pre-Console buffer index */
+#endif
unsigned long
