| Message ID | 1453417531-23669-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Accepted |
| Commit | 05266103349e8409f5fbd55197c75c7bb58f575d |
| Delegated to: | Simon Glass |
| Headers | show |
On 21 January 2016 at 16:05, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > find_ram_base() is a shared utility function, not a core part of the > U-Boot console interaction. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > These two patches depend on my previous series starting with: > test/py: fix timeout to be absolute > and ending with: > test/py: add DFU test > --- > test/py/tests/test_md.py | 5 +++-- > test/py/u_boot_console_base.py | 37 ------------------------------------- > test/py/u_boot_utils.py | 38 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 41 insertions(+), 39 deletions(-) Acked-by: Simon Glass <sjg@chromium.org>
Dear Stephen, In message <1453417531-23669-1-git-send-email-swarren@wwwdotorg.org> you wrote: > > find_ram_base() is a shared utility function, not a core part of the > U-Boot console interaction. On which boards did you test this feature? Eventually ARM only? > + with u_boot_console.log.section('find_ram_base'): > + response = u_boot_console.run_command('bdinfo') > + for l in response.split('\n'): > + if '-> start' in l: > + ram_base = int(l.split('=')[1].strip(), 16) > + break Searching for "-> start" is probably not exactly portable. For example, on a PowerPC system the output of "bdi" might look like this: => bdi memstart = 0x00000000 memsize = 0x04000000 flashstart = 0xFC000000 flashsize = 0x02000000 flashoffset = 0x0005D000 sramstart = 0x00000000 sramsize = 0x00000000 bootflags = 0x00000000 intfreq = 396 MHz busfreq = 132 MHz ethaddr = 00:D0:93:2A:C2:1A IP addr = 192.168.240.240 baudrate = 115200 bps relocaddr = 0x03F47000 => [example is from a TQM5200S, U-Boot 2016.01-00223-gb57843e] Best regards, Wolfgang Denk
On 01/22/2016 03:30 PM, Wolfgang Denk wrote: > Dear Stephen, > > In message <1453417531-23669-1-git-send-email-swarren@wwwdotorg.org> you wrote: >> >> find_ram_base() is a shared utility function, not a core part of the >> U-Boot console interaction. > > On which boards did you test this feature? Eventually ARM only? It's been tested on a few ARM, sandbox, and at least one microblaze. >> + with u_boot_console.log.section('find_ram_base'): >> + response = u_boot_console.run_command('bdinfo') >> + for l in response.split('\n'): >> + if '-> start' in l: >> + ram_base = int(l.split('=')[1].strip(), 16) >> + break > > Searching for "-> start" is probably not exactly portable. For > example, on a PowerPC system the output of "bdi" might look like this: > > => bdi > memstart = 0x00000000 > memsize = 0x04000000 ... > > [example is from a TQM5200S, U-Boot 2016.01-00223-gb57843e] Good point. I think the best fix here is to modify all implementations of "bdinfo" to print the same information and in the same format as much as possible. Do you agree?
Hi Stephen, On 25 January 2016 at 09:50, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 01/22/2016 03:30 PM, Wolfgang Denk wrote: >> >> Dear Stephen, >> >> In message <1453417531-23669-1-git-send-email-swarren@wwwdotorg.org> you >> wrote: >>> >>> >>> find_ram_base() is a shared utility function, not a core part of the >>> U-Boot console interaction. >> >> >> On which boards did you test this feature? Eventually ARM only? > > > It's been tested on a few ARM, sandbox, and at least one microblaze. > >>> + with u_boot_console.log.section('find_ram_base'): >>> + response = u_boot_console.run_command('bdinfo') >>> + for l in response.split('\n'): >>> + if '-> start' in l: >>> + ram_base = int(l.split('=')[1].strip(), 16) >>> + break >> >> >> Searching for "-> start" is probably not exactly portable. For >> example, on a PowerPC system the output of "bdi" might look like this: >> >> => bdi >> memstart = 0x00000000 >> memsize = 0x04000000 > > ... >> >> >> [example is from a TQM5200S, U-Boot 2016.01-00223-gb57843e] > > > Good point. I think the best fix here is to modify all implementations of > "bdinfo" to print the same information and in the same format as much as > possible. Do you agree? Yes - and the best way to do this is to use the same code for all boards if possible. BTW I can't apply this patch as the u_boot_utils.py file is missing. Can you please rebase and resend? Regards, Simon
On 01/25/2016 06:03 PM, Simon Glass wrote: > Hi Stephen, > > On 25 January 2016 at 09:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 01/22/2016 03:30 PM, Wolfgang Denk wrote: >>> >>> Dear Stephen, >>> >>> In message <1453417531-23669-1-git-send-email-swarren@wwwdotorg.org> you >>> wrote: >>>> >>>> >>>> find_ram_base() is a shared utility function, not a core part of the >>>> U-Boot console interaction. >>> >>> >>> On which boards did you test this feature? Eventually ARM only? >> >> >> It's been tested on a few ARM, sandbox, and at least one microblaze. >> >>>> + with u_boot_console.log.section('find_ram_base'): >>>> + response = u_boot_console.run_command('bdinfo') >>>> + for l in response.split('\n'): >>>> + if '-> start' in l: >>>> + ram_base = int(l.split('=')[1].strip(), 16) >>>> + break >>> >>> >>> Searching for "-> start" is probably not exactly portable. For >>> example, on a PowerPC system the output of "bdi" might look like this: >>> >>> => bdi >>> memstart = 0x00000000 >>> memsize = 0x04000000 >> >> ... >>> >>> >>> [example is from a TQM5200S, U-Boot 2016.01-00223-gb57843e] >> >> >> Good point. I think the best fix here is to modify all implementations of >> "bdinfo" to print the same information and in the same format as much as >> possible. Do you agree? > > Yes - and the best way to do this is to use the same code for all > boards if possible. > > BTW I can't apply this patch as the u_boot_utils.py file is missing. > Can you please rebase and resend? Do you have "test/py: add various utility code" already applied? That creates u_boot_utils.py. As mentioned in the original patch email, this series depends on the series that contains that patch. You had replied earlier that you had applied that series in u-boot-dm.
Hi Stephen, On 25 January 2016 at 18:09, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 01/25/2016 06:03 PM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 25 January 2016 at 09:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> On 01/22/2016 03:30 PM, Wolfgang Denk wrote: >>>> >>>> >>>> Dear Stephen, >>>> >>>> In message <1453417531-23669-1-git-send-email-swarren@wwwdotorg.org> you >>>> wrote: >>>>> >>>>> >>>>> >>>>> find_ram_base() is a shared utility function, not a core part of the >>>>> U-Boot console interaction. >>>> >>>> >>>> >>>> On which boards did you test this feature? Eventually ARM only? >>> >>> >>> >>> It's been tested on a few ARM, sandbox, and at least one microblaze. >>> >>>>> + with u_boot_console.log.section('find_ram_base'): >>>>> + response = u_boot_console.run_command('bdinfo') >>>>> + for l in response.split('\n'): >>>>> + if '-> start' in l: >>>>> + ram_base = int(l.split('=')[1].strip(), 16) >>>>> + break >>>> >>>> >>>> >>>> Searching for "-> start" is probably not exactly portable. For >>>> example, on a PowerPC system the output of "bdi" might look like this: >>>> >>>> => bdi >>>> memstart = 0x00000000 >>>> memsize = 0x04000000 >>> >>> >>> ... >>>> >>>> >>>> >>>> [example is from a TQM5200S, U-Boot 2016.01-00223-gb57843e] >>> >>> >>> >>> Good point. I think the best fix here is to modify all implementations of >>> "bdinfo" to print the same information and in the same format as much as >>> possible. Do you agree? >> >> >> Yes - and the best way to do this is to use the same code for all >> boards if possible. >> >> BTW I can't apply this patch as the u_boot_utils.py file is missing. >> Can you please rebase and resend? > > > Do you have "test/py: add various utility code" already applied? That > creates u_boot_utils.py. As mentioned in the original patch email, this > series depends on the series that contains that patch. You had replied > earlier that you had applied that series in u-boot-dm. Ah yes, user error, sorry. BTW re your question about """ for comments, please see PEP8 etc.: http://legacy.python.org/dev/peps/pep-0008/#block-comments https://www.python.org/dev/peps/pep-0257/ Applied to u-boot-dm, thanks! Regards, Simon
On 01/25/2016 06:15 PM, Simon Glass wrote: > Hi Stephen, > > On 25 January 2016 at 18:09, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 01/25/2016 06:03 PM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 25 January 2016 at 09:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> >>>> On 01/22/2016 03:30 PM, Wolfgang Denk wrote: >>>>> >>>>> >>>>> Dear Stephen, >>>>> >>>>> In message <1453417531-23669-1-git-send-email-swarren@wwwdotorg.org> you >>>>> wrote: >>>>>> >>>>>> >>>>>> >>>>>> find_ram_base() is a shared utility function, not a core part of the >>>>>> U-Boot console interaction. >>>>> >>>>> >>>>> >>>>> On which boards did you test this feature? Eventually ARM only? >>>> >>>> >>>> >>>> It's been tested on a few ARM, sandbox, and at least one microblaze. >>>> >>>>>> + with u_boot_console.log.section('find_ram_base'): >>>>>> + response = u_boot_console.run_command('bdinfo') >>>>>> + for l in response.split('\n'): >>>>>> + if '-> start' in l: >>>>>> + ram_base = int(l.split('=')[1].strip(), 16) >>>>>> + break >>>>> >>>>> >>>>> >>>>> Searching for "-> start" is probably not exactly portable. For >>>>> example, on a PowerPC system the output of "bdi" might look like this: >>>>> >>>>> => bdi >>>>> memstart = 0x00000000 >>>>> memsize = 0x04000000 >>>> >>>> >>>> ... >>>>> >>>>> >>>>> >>>>> [example is from a TQM5200S, U-Boot 2016.01-00223-gb57843e] >>>> >>>> >>>> >>>> Good point. I think the best fix here is to modify all implementations of >>>> "bdinfo" to print the same information and in the same format as much as >>>> possible. Do you agree? >>> >>> >>> Yes - and the best way to do this is to use the same code for all >>> boards if possible. >>> >>> BTW I can't apply this patch as the u_boot_utils.py file is missing. >>> Can you please rebase and resend? >> >> >> Do you have "test/py: add various utility code" already applied? That >> creates u_boot_utils.py. As mentioned in the original patch email, this >> series depends on the series that contains that patch. You had replied >> earlier that you had applied that series in u-boot-dm. > > Ah yes, user error, sorry. > > BTW re your question about """ for comments, please see PEP8 etc.: > > http://legacy.python.org/dev/peps/pep-0008/#block-comments > https://www.python.org/dev/peps/pep-0257/ OK, I see the recommendation to use """ for docstrings. Can we also use " rather than ' for regular string too please, to avoid mixing different quote characters?
Hi Stephen, On 26 January 2016 at 11:13, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 01/25/2016 06:15 PM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 25 January 2016 at 18:09, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> On 01/25/2016 06:03 PM, Simon Glass wrote: >>>> >>>> >>>> Hi Stephen, >>>> >>>> On 25 January 2016 at 09:50, Stephen Warren <swarren@wwwdotorg.org> >>>> wrote: >>>>> >>>>> >>>>> On 01/22/2016 03:30 PM, Wolfgang Denk wrote: >>>>>> >>>>>> >>>>>> >>>>>> Dear Stephen, >>>>>> >>>>>> In message <1453417531-23669-1-git-send-email-swarren@wwwdotorg.org> >>>>>> you >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> find_ram_base() is a shared utility function, not a core part of the >>>>>>> U-Boot console interaction. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On which boards did you test this feature? Eventually ARM only? >>>>> >>>>> >>>>> >>>>> >>>>> It's been tested on a few ARM, sandbox, and at least one microblaze. >>>>> >>>>>>> + with u_boot_console.log.section('find_ram_base'): >>>>>>> + response = u_boot_console.run_command('bdinfo') >>>>>>> + for l in response.split('\n'): >>>>>>> + if '-> start' in l: >>>>>>> + ram_base = int(l.split('=')[1].strip(), 16) >>>>>>> + break >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Searching for "-> start" is probably not exactly portable. For >>>>>> example, on a PowerPC system the output of "bdi" might look like this: >>>>>> >>>>>> => bdi >>>>>> memstart = 0x00000000 >>>>>> memsize = 0x04000000 >>>>> >>>>> >>>>> >>>>> ... >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> [example is from a TQM5200S, U-Boot 2016.01-00223-gb57843e] >>>>> >>>>> >>>>> >>>>> >>>>> Good point. I think the best fix here is to modify all implementations >>>>> of >>>>> "bdinfo" to print the same information and in the same format as much >>>>> as >>>>> possible. Do you agree? >>>> >>>> >>>> >>>> Yes - and the best way to do this is to use the same code for all >>>> boards if possible. >>>> >>>> BTW I can't apply this patch as the u_boot_utils.py file is missing. >>>> Can you please rebase and resend? >>> >>> >>> >>> Do you have "test/py: add various utility code" already applied? That >>> creates u_boot_utils.py. As mentioned in the original patch email, this >>> series depends on the series that contains that patch. You had replied >>> earlier that you had applied that series in u-boot-dm. >> >> >> Ah yes, user error, sorry. >> >> BTW re your question about """ for comments, please see PEP8 etc.: >> >> http://legacy.python.org/dev/peps/pep-0008/#block-comments >> https://www.python.org/dev/peps/pep-0257/ > > > OK, I see the recommendation to use """ for docstrings. Can we also use " > rather than ' for regular string too please, to avoid mixing different quote > characters? That's the style used for patman/buildman. I think it's actually good to have them different. You will sometimes hit the case where you need a quoted double quote, like print 'This is a "test" of things'. Regards, Simon
On 01/26/2016 12:59 PM, Simon Glass wrote: > Hi Stephen, > > On 26 January 2016 at 11:13, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 01/25/2016 06:15 PM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 25 January 2016 at 18:09, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> >>>> On 01/25/2016 06:03 PM, Simon Glass wrote: >>>>> >>>>> >>>>> Hi Stephen, >>>>> >>>>> On 25 January 2016 at 09:50, Stephen Warren <swarren@wwwdotorg.org> >>>>> wrote: >>>>>> >>>>>> >>>>>> On 01/22/2016 03:30 PM, Wolfgang Denk wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Dear Stephen, >>>>>>> >>>>>>> In message <1453417531-23669-1-git-send-email-swarren@wwwdotorg.org> >>>>>>> you >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> find_ram_base() is a shared utility function, not a core part of the >>>>>>>> U-Boot console interaction. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On which boards did you test this feature? Eventually ARM only? >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> It's been tested on a few ARM, sandbox, and at least one microblaze. >>>>>> >>>>>>>> + with u_boot_console.log.section('find_ram_base'): >>>>>>>> + response = u_boot_console.run_command('bdinfo') >>>>>>>> + for l in response.split('\n'): >>>>>>>> + if '-> start' in l: >>>>>>>> + ram_base = int(l.split('=')[1].strip(), 16) >>>>>>>> + break >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> Searching for "-> start" is probably not exactly portable. For >>>>>>> example, on a PowerPC system the output of "bdi" might look like this: >>>>>>> >>>>>>> => bdi >>>>>>> memstart = 0x00000000 >>>>>>> memsize = 0x04000000 >>>>>> >>>>>> >>>>>> >>>>>> ... >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> [example is from a TQM5200S, U-Boot 2016.01-00223-gb57843e] >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Good point. I think the best fix here is to modify all implementations >>>>>> of >>>>>> "bdinfo" to print the same information and in the same format as much >>>>>> as >>>>>> possible. Do you agree? >>>>> >>>>> >>>>> >>>>> Yes - and the best way to do this is to use the same code for all >>>>> boards if possible. >>>>> >>>>> BTW I can't apply this patch as the u_boot_utils.py file is missing. >>>>> Can you please rebase and resend? >>>> >>>> >>>> >>>> Do you have "test/py: add various utility code" already applied? That >>>> creates u_boot_utils.py. As mentioned in the original patch email, this >>>> series depends on the series that contains that patch. You had replied >>>> earlier that you had applied that series in u-boot-dm. >>> >>> >>> Ah yes, user error, sorry. >>> >>> BTW re your question about """ for comments, please see PEP8 etc.: >>> >>> http://legacy.python.org/dev/peps/pep-0008/#block-comments >>> https://www.python.org/dev/peps/pep-0257/ >> >> >> OK, I see the recommendation to use """ for docstrings. Can we also use " >> rather than ' for regular string too please, to avoid mixing different quote >> characters? > > That's the style used for patman/buildman. I think it's actually good > to have them different. You will sometimes hit the case where you need > a quoted double quote, like print 'This is a "test" of things'. There are counter-cases where ' needs to be escaped in the current scheme too, e.g.: > assert('Unknown command \'non_existent_cmd\' - try \'help\'' in response) ... although I haven't quantified which way around would lead to more escapes. I'll just convert the docstrings for now. Since you've acked/reviewed everything I have sent so far, I'll build my patch on top of all those patches.
diff --git a/test/py/tests/test_md.py b/test/py/tests/test_md.py index 94603c7df609..32cce4f15c53 100644 --- a/test/py/tests/test_md.py +++ b/test/py/tests/test_md.py @@ -4,13 +4,14 @@ # SPDX-License-Identifier: GPL-2.0 import pytest +import u_boot_utils @pytest.mark.buildconfigspec('cmd_memory') def test_md(u_boot_console): '''Test that md reads memory as expected, and that memory can be modified using the mw command.''' - ram_base = u_boot_console.find_ram_base() + ram_base = u_boot_utils.find_ram_base(u_boot_console) addr = '%08x' % ram_base val = 'a5f09876' expected_response = addr + ': ' + val @@ -26,7 +27,7 @@ def test_md_repeat(u_boot_console): '''Test command repeat (via executing an empty command) operates correctly for "md"; the command must repeat and dump an incrementing address.''' - ram_base = u_boot_console.find_ram_base() + ram_base = u_boot_utils.find_ram_base(u_boot_console) addr_base = '%08x' % ram_base words = 0x10 addr_repeat = '%08x' % (ram_base + (words * 4)) diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py index 06f61f987180..51163bc0db68 100644 --- a/test/py/u_boot_console_base.py +++ b/test/py/u_boot_console_base.py @@ -86,7 +86,6 @@ class ConsoleBase(object): self.at_prompt = False self.at_prompt_logevt = None - self.ram_base = None def close(self): '''Terminate the connection to the U-Boot console. @@ -378,39 +377,3 @@ class ConsoleBase(object): ''' return ConsoleDisableCheck(self, check_type) - - def find_ram_base(self): - '''Find the running U-Boot's RAM location. - - Probe the running U-Boot to determine the address of the first bank - of RAM. This is useful for tests that test reading/writing RAM, or - load/save files that aren't associated with some standard address - typically represented in an environment variable such as - ${kernel_addr_r}. The value is cached so that it only needs to be - actively read once. - - Args: - None. - - Returns: - The address of U-Boot's first RAM bank, as an integer. - ''' - - if self.config.buildconfig.get('config_cmd_bdi', 'n') != 'y': - pytest.skip('bdinfo command not supported') - if self.ram_base == -1: - pytest.skip('Previously failed to find RAM bank start') - if self.ram_base is not None: - return self.ram_base - - with self.log.section('find_ram_base'): - response = self.run_command('bdinfo') - for l in response.split('\n'): - if '-> start' in l: - self.ram_base = int(l.split('=')[1].strip(), 16) - break - if self.ram_base is None: - self.ram_base = -1 - raise Exception('Failed to find RAM bank start in `bdinfo`') - - return self.ram_base diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py index 539af618dbf2..522390a207ef 100644 --- a/test/py/u_boot_utils.py +++ b/test/py/u_boot_utils.py @@ -169,3 +169,41 @@ def run_and_log(u_boot_console, cmd, ignore_errors=False): runner = u_boot_console.log.get_runner(cmd[0], sys.stdout) runner.run(cmd, ignore_errors=ignore_errors) runner.close() + +ram_base = None +def find_ram_base(u_boot_console): + '''Find the running U-Boot's RAM location. + + Probe the running U-Boot to determine the address of the first bank + of RAM. This is useful for tests that test reading/writing RAM, or + load/save files that aren't associated with some standard address + typically represented in an environment variable such as + ${kernel_addr_r}. The value is cached so that it only needs to be + actively read once. + + Args: + u_boot_console: A console connection to U-Boot. + + Returns: + The address of U-Boot's first RAM bank, as an integer. + ''' + + global ram_base + if u_boot_console.config.buildconfig.get('config_cmd_bdi', 'n') != 'y': + pytest.skip('bdinfo command not supported') + if ram_base == -1: + pytest.skip('Previously failed to find RAM bank start') + if ram_base is not None: + return ram_base + + with u_boot_console.log.section('find_ram_base'): + response = u_boot_console.run_command('bdinfo') + for l in response.split('\n'): + if '-> start' in l: + ram_base = int(l.split('=')[1].strip(), 16) + break + if ram_base is None: + ram_base = -1 + raise Exception('Failed to find RAM bank start in `bdinfo`') + + return ram_base