Re: [U-Boot] [PATCH v2 3/8] binman: Add a new "skip-at-start" property in Section class
Hi,
On 30 August 2018 at 23:22, Jagdish Gediya wrote:
> Hi,
>
>> -Original Message-
>> From: s...@google.com On Behalf Of Simon Glass
>> Sent: Thursday, August 30, 2018 8:21 AM
>> To: Jagdish Gediya
>> Cc: U-Boot Mailing List ; Prabhakar Kushwaha
>> ; York Sun ; Poonam
>> Aggrwal ; Bin Meng ;
>> Tom Rini
>> Subject: Re: [PATCH v2 3/8] binman: Add a new "skip-at-start" property in
>> Section class
>>
>> Hi,
>>
>> On 28 August 2018 at 08:49, Jagdish Gediya
>> wrote:
>> >
>> > Currently binman calculates '_skip_at_start' based on 'end-at-4gb'
>> > property and it is used for x86 images.
>> >
>> > For Powerpc mpc85xx based CPU, CONFIG_SYS_TEXT_BASE is the first entry
>> > offset which can be 0xeff4 or 0xfff4 for nor flash boot,
>> > 0x201000 for sd boot etc, so "_skip_at_start" should be set to
>> > CONFIG_SYS_TEXT_BASE.
>> >
>> > 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE +
>> > Image size != 4gb.
>> >
>> > Add new property "skip-at-start" in Section class so that
>> > '_skip_at_start' can be calculated either based on "end-at-4gb"
>> > or based on "skip-at-start".
>> >
>> > Signed-off-by: Jagdish Gediya
>> > ---
>> > Changes for v2:
>> > - Renamed 'start-pos' property to 'skip-at-start'
>> > - Updated README
>> >
>> > tools/binman/README | 9 +
>> > tools/binman/bsection.py | 1 +
>> > 2 files changed, 10 insertions(+)
>> >
>>
>> Please add a test for this feature. You will need to add a new numbered dts
>> in tools/binman/tests and test in ftest.py
>>
>> > diff --git a/tools/binman/README b/tools/binman/README index
>> > cb34171..7b4bf2e 100644
>> > --- a/tools/binman/README
>> > +++ b/tools/binman/README
>> > @@ -397,6 +397,15 @@ end-at-4gb:
>> > 8MB ROM, the offset of the first entry would be 0xfff8 with
>> > this option, instead of 0 without this option.
>> >
>> > +skip-at-start:
>> > + This property specifies the first entry offset if not 0.
>> > +
>> > + For Powerpc Book-E architecture, CONFIG_SYS_TEXT_BASE is the first
>> > + entry offset which can be 0xeff4 or 0xfff4 for nor flash
>> > boot,
>> > + 0x201000 for sd boot etc.
>>
>> Can you say 'entry offset of the first entry. It can be ...'. I think it is
>> clearer.
>>
>> > +
>> > + 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE
>> +
>> > + Image size != 4gb.
>> >
>> > Examples of the above options can be found in the tests. See the
>> > tools/binman/test directory.
>> > diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py index
>> > a0bd1b6..68997b7 100644
>> > --- a/tools/binman/bsection.py
>> > +++ b/tools/binman/bsection.py
>> > @@ -79,6 +79,7 @@ class Section(object):
>> > self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0)
>> > self._sort = fdt_util.GetBool(self._node, 'sort-by-offset')
>> > self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')
>> > +self._skip_at_start = fdt_util.GetInt(self._node,
>> > + 'skip-at-start', 0)
>>
>> This is a bit pedantic, but...
>>
>> I think this needs to drop the '0' default value. Also in the __init__
>> constructor, set _skip_at_start to None
>>
>> > if self._end_4gb and not self._size:
>> > self._Raise("Section size must be provided when using
>> > end-at-4gb")
>> > if self._end_4gb:
>>
>> ...here you need to check that self._skip_at_start is None, so people don't
>> set
>> both properties. Then set it to 0 if not set and not end_4gb. Something like:
>>
>> if self._end_4gb:
>>if if self._skip_at_start is not None:
>> self.Raise...
>>self._skip_at_start = 0x1 - self._size
>> else:
>>self._skip_at_start = 0
>>
>> Does that make sense? This needs a test too...
> I think it should be checked that self._skip_at_start is None before setting
> it to 0 in else part.
> What's your opinion on below implementation?
>
> self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')
> self._skip_at_start = fdt_util.GetInt(self._node, 'skip-at-start')
> if self._end_4gb:
> if not self._size:
> self._Raise("Section size must be provided when using
> end-at-4gb")
> if self._skip_at_start is not None:
> self._Raise("Provide either 'end-at-4gb' or 'skip-at-start'")
> else:
> self._skip_at_start = 0x1 - self._size
> else:
> if self._skip_at_start is None:
> self._skip_at_start = 0
That looks OK to me. Make sure you change __init__() to set it to None
instead of 0.
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/8] binman: Add a new "skip-at-start" property in Section class
Hi,
> -Original Message-
> From: s...@google.com On Behalf Of Simon Glass
> Sent: Thursday, August 30, 2018 8:21 AM
> To: Jagdish Gediya
> Cc: U-Boot Mailing List ; Prabhakar Kushwaha
> ; York Sun ; Poonam
> Aggrwal ; Bin Meng ;
> Tom Rini
> Subject: Re: [PATCH v2 3/8] binman: Add a new "skip-at-start" property in
> Section class
>
> Hi,
>
> On 28 August 2018 at 08:49, Jagdish Gediya
> wrote:
> >
> > Currently binman calculates '_skip_at_start' based on 'end-at-4gb'
> > property and it is used for x86 images.
> >
> > For Powerpc mpc85xx based CPU, CONFIG_SYS_TEXT_BASE is the first entry
> > offset which can be 0xeff4 or 0xfff4 for nor flash boot,
> > 0x201000 for sd boot etc, so "_skip_at_start" should be set to
> > CONFIG_SYS_TEXT_BASE.
> >
> > 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE +
> > Image size != 4gb.
> >
> > Add new property "skip-at-start" in Section class so that
> > '_skip_at_start' can be calculated either based on "end-at-4gb"
> > or based on "skip-at-start".
> >
> > Signed-off-by: Jagdish Gediya
> > ---
> > Changes for v2:
> > - Renamed 'start-pos' property to 'skip-at-start'
> > - Updated README
> >
> > tools/binman/README | 9 +
> > tools/binman/bsection.py | 1 +
> > 2 files changed, 10 insertions(+)
> >
>
> Please add a test for this feature. You will need to add a new numbered dts
> in tools/binman/tests and test in ftest.py
>
> > diff --git a/tools/binman/README b/tools/binman/README index
> > cb34171..7b4bf2e 100644
> > --- a/tools/binman/README
> > +++ b/tools/binman/README
> > @@ -397,6 +397,15 @@ end-at-4gb:
> > 8MB ROM, the offset of the first entry would be 0xfff8 with
> > this option, instead of 0 without this option.
> >
> > +skip-at-start:
> > + This property specifies the first entry offset if not 0.
> > +
> > + For Powerpc Book-E architecture, CONFIG_SYS_TEXT_BASE is the first
> > + entry offset which can be 0xeff4 or 0xfff4 for nor flash
> > boot,
> > + 0x201000 for sd boot etc.
>
> Can you say 'entry offset of the first entry. It can be ...'. I think it is
> clearer.
>
> > +
> > + 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE
> +
> > + Image size != 4gb.
> >
> > Examples of the above options can be found in the tests. See the
> > tools/binman/test directory.
> > diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py index
> > a0bd1b6..68997b7 100644
> > --- a/tools/binman/bsection.py
> > +++ b/tools/binman/bsection.py
> > @@ -79,6 +79,7 @@ class Section(object):
> > self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0)
> > self._sort = fdt_util.GetBool(self._node, 'sort-by-offset')
> > self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')
> > +self._skip_at_start = fdt_util.GetInt(self._node,
> > + 'skip-at-start', 0)
>
> This is a bit pedantic, but...
>
> I think this needs to drop the '0' default value. Also in the __init__
> constructor, set _skip_at_start to None
>
> > if self._end_4gb and not self._size:
> > self._Raise("Section size must be provided when using
> > end-at-4gb")
> > if self._end_4gb:
>
> ...here you need to check that self._skip_at_start is None, so people don't
> set
> both properties. Then set it to 0 if not set and not end_4gb. Something like:
>
> if self._end_4gb:
>if if self._skip_at_start is not None:
> self.Raise...
>self._skip_at_start = 0x1 - self._size
> else:
>self._skip_at_start = 0
>
> Does that make sense? This needs a test too...
I think it should be checked that self._skip_at_start is None before setting it
to 0 in else part.
What's your opinion on below implementation?
self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')
self._skip_at_start = fdt_util.GetInt(self._node, 'skip-at-start')
if self._end_4gb:
if not self._size:
self._Raise("Section size must be provided when using
end-at-4gb")
if self._skip_at_start is not None:
self._Raise("Provide either 'end-at-4gb' or 'skip-at-start'")
else:
self._skip_at_start = 0x1 - self._size
else:
if self._skip_at_start is None:
self._skip_at_start = 0
Thanks,
Jagdish
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/8] binman: Add a new "skip-at-start" property in Section class
Hi,
On 28 August 2018 at 08:49, Jagdish Gediya wrote:
>
> Currently binman calculates '_skip_at_start' based on 'end-at-4gb'
> property and it is used for x86 images.
>
> For Powerpc mpc85xx based CPU, CONFIG_SYS_TEXT_BASE is the first
> entry offset which can be 0xeff4 or 0xfff4 for nor flash boot,
> 0x201000 for sd boot etc, so "_skip_at_start" should be set to
> CONFIG_SYS_TEXT_BASE.
>
> 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE +
> Image size != 4gb.
>
> Add new property "skip-at-start" in Section class so that
> '_skip_at_start' can be calculated either based on "end-at-4gb"
> or based on "skip-at-start".
>
> Signed-off-by: Jagdish Gediya
> ---
> Changes for v2:
> - Renamed 'start-pos' property to 'skip-at-start'
> - Updated README
>
> tools/binman/README | 9 +
> tools/binman/bsection.py | 1 +
> 2 files changed, 10 insertions(+)
>
Please add a test for this feature. You will need to add a new
numbered dts in tools/binman/tests and test in ftest.py
> diff --git a/tools/binman/README b/tools/binman/README
> index cb34171..7b4bf2e 100644
> --- a/tools/binman/README
> +++ b/tools/binman/README
> @@ -397,6 +397,15 @@ end-at-4gb:
> 8MB ROM, the offset of the first entry would be 0xfff8 with
> this option, instead of 0 without this option.
>
> +skip-at-start:
> + This property specifies the first entry offset if not 0.
> +
> + For Powerpc Book-E architecture, CONFIG_SYS_TEXT_BASE is the first
> + entry offset which can be 0xeff4 or 0xfff4 for nor flash boot,
> + 0x201000 for sd boot etc.
Can you say 'entry offset of the first entry. It can be ...'. I think
it is clearer.
> +
> + 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE +
> + Image size != 4gb.
>
> Examples of the above options can be found in the tests. See the
> tools/binman/test directory.
> diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py
> index a0bd1b6..68997b7 100644
> --- a/tools/binman/bsection.py
> +++ b/tools/binman/bsection.py
> @@ -79,6 +79,7 @@ class Section(object):
> self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0)
> self._sort = fdt_util.GetBool(self._node, 'sort-by-offset')
> self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')
> +self._skip_at_start = fdt_util.GetInt(self._node, 'skip-at-start', 0)
This is a bit pedantic, but...
I think this needs to drop the '0' default value. Also in the __init__
constructor, set _skip_at_start to None
> if self._end_4gb and not self._size:
> self._Raise("Section size must be provided when using
> end-at-4gb")
> if self._end_4gb:
...here you need to check that self._skip_at_start is None, so people
don't set both properties. Then set it to 0 if not set and not
end_4gb. Something like:
if self._end_4gb:
if if self._skip_at_start is not None:
self.Raise...
self._skip_at_start = 0x1 - self._size
else:
self._skip_at_start = 0
Does that make sense? This needs a test too...
> --
> 2.7.4
>
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/8] binman: Add a new "skip-at-start" property in Section class
On Tue, Aug 28, 2018 at 11:53 AM Jagdish Gediya wrote:
>
> Currently binman calculates '_skip_at_start' based on 'end-at-4gb'
> property and it is used for x86 images.
>
> For Powerpc mpc85xx based CPU, CONFIG_SYS_TEXT_BASE is the first
nits: PowerPC
> entry offset which can be 0xeff4 or 0xfff4 for nor flash boot,
> 0x201000 for sd boot etc, so "_skip_at_start" should be set to
> CONFIG_SYS_TEXT_BASE.
>
> 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE +
> Image size != 4gb.
>
> Add new property "skip-at-start" in Section class so that
> '_skip_at_start' can be calculated either based on "end-at-4gb"
> or based on "skip-at-start".
>
> Signed-off-by: Jagdish Gediya
> ---
> Changes for v2:
> - Renamed 'start-pos' property to 'skip-at-start'
> - Updated README
>
> tools/binman/README | 9 +
> tools/binman/bsection.py | 1 +
> 2 files changed, 10 insertions(+)
>
Reviewed-by: Bin Meng
But two nits below:
> diff --git a/tools/binman/README b/tools/binman/README
> index cb34171..7b4bf2e 100644
> --- a/tools/binman/README
> +++ b/tools/binman/README
> @@ -397,6 +397,15 @@ end-at-4gb:
> 8MB ROM, the offset of the first entry would be 0xfff8 with
> this option, instead of 0 without this option.
>
> +skip-at-start:
> + This property specifies the first entry offset if not 0.
> +
> + For Powerpc Book-E architecture, CONFIG_SYS_TEXT_BASE is the first
nits: PowerPC
> + entry offset which can be 0xeff4 or 0xfff4 for nor flash boot,
> + 0x201000 for sd boot etc.
> +
> + 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE +
> + Image size != 4gb.
>
> Examples of the above options can be found in the tests. See the
> tools/binman/test directory.
> diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py
> index a0bd1b6..68997b7 100644
> --- a/tools/binman/bsection.py
> +++ b/tools/binman/bsection.py
> @@ -79,6 +79,7 @@ class Section(object):
> self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0)
> self._sort = fdt_util.GetBool(self._node, 'sort-by-offset')
> self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')
> +self._skip_at_start = fdt_util.GetInt(self._node, 'skip-at-start', 0)
> if self._end_4gb and not self._size:
> self._Raise("Section size must be provided when using
> end-at-4gb")
> if self._end_4gb:
> --
Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v2 3/8] binman: Add a new "skip-at-start" property in Section class
Currently binman calculates '_skip_at_start' based on 'end-at-4gb'
property and it is used for x86 images.
For Powerpc mpc85xx based CPU, CONFIG_SYS_TEXT_BASE is the first
entry offset which can be 0xeff4 or 0xfff4 for nor flash boot,
0x201000 for sd boot etc, so "_skip_at_start" should be set to
CONFIG_SYS_TEXT_BASE.
'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE +
Image size != 4gb.
Add new property "skip-at-start" in Section class so that
'_skip_at_start' can be calculated either based on "end-at-4gb"
or based on "skip-at-start".
Signed-off-by: Jagdish Gediya
---
Changes for v2:
- Renamed 'start-pos' property to 'skip-at-start'
- Updated README
tools/binman/README | 9 +
tools/binman/bsection.py | 1 +
2 files changed, 10 insertions(+)
diff --git a/tools/binman/README b/tools/binman/README
index cb34171..7b4bf2e 100644
--- a/tools/binman/README
+++ b/tools/binman/README
@@ -397,6 +397,15 @@ end-at-4gb:
8MB ROM, the offset of the first entry would be 0xfff8 with
this option, instead of 0 without this option.
+skip-at-start:
+ This property specifies the first entry offset if not 0.
+
+ For Powerpc Book-E architecture, CONFIG_SYS_TEXT_BASE is the first
+ entry offset which can be 0xeff4 or 0xfff4 for nor flash boot,
+ 0x201000 for sd boot etc.
+
+ 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE +
+ Image size != 4gb.
Examples of the above options can be found in the tests. See the
tools/binman/test directory.
diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py
index a0bd1b6..68997b7 100644
--- a/tools/binman/bsection.py
+++ b/tools/binman/bsection.py
@@ -79,6 +79,7 @@ class Section(object):
self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0)
self._sort = fdt_util.GetBool(self._node, 'sort-by-offset')
self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')
+self._skip_at_start = fdt_util.GetInt(self._node, 'skip-at-start', 0)
if self._end_4gb and not self._size:
self._Raise("Section size must be provided when using end-at-4gb")
if self._end_4gb:
--
2.7.4
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
