Flash memory size not aligned to address

Flash memory size not aligned to address

[Mark Rages]

The Nordic nRF52 memory map, reported from black magic probe:

Num Enb Low Addr High Addr Attrs
0 y 0x00000000 0x00080000 flash blocksize 0x1000 nocache
1 y 0x10001000 0x10001210 flash blocksize 0x210 nocache
2 y 0x20000000 0x20010000 rw nocache

The region at 0x10001000 is "UICR" and it is a section of flash that is
erased all at once.

Notice the odd size: 0x210 is the size of the region defined in the
datasheet.

But because the block size was listed as 0x210, gdb was insisting on
issuing two erase commands divisible by 0x210, starting below 0x10001000.

This patch fixes it by doing the alignment computation from the start of
the region, not from address 0:

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d8d5772..d2dc194 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-10-05  Mark Rages <markrages@gmail.com>
+
+       * target-memory.c (block_boundaries): Fix for block address not
+       aligned on block size.
+
 2017-10-05  Tristan Gingold  <tgingold@free.fr>

        * MAINTAINERS (Misc): Update my email address.
@@ -6962,7 +6967,7 @@

 2017-05-08  Alan Hayward  <alan.hayward@arm.com>

-       * mn10300-linux-tdep.c (am33_supply_gregset_method): Use
+       * mn10300-linux-tdep.c (am33_supply_gregset_method): Use
        regcache->raw_supply_zeroed.

 2017-05-06  Sergio Durigan Junior  <sergiodj@redhat.com>
diff --git a/gdb/target-memory.c b/gdb/target-memory.c
index 1c8faa8..4651200 100644
--- a/gdb/target-memory.c
+++ b/gdb/target-memory.c
@@ -138,14 +138,18 @@ block_boundaries (CORE_ADDR address, CORE_ADDR
*begin, CORE_ADDR *end)
 {
   struct mem_region *region;
   unsigned blocksize;
+  CORE_ADDR address_in_region;

   region = lookup_mem_region (address);
   gdb_assert (region->attrib.mode == MEM_FLASH);
   blocksize = region->attrib.blocksize;
+
+  address_in_region = address - region->lo;
+
   if (begin)
-    *begin = address / blocksize * blocksize;
+    *begin = region->lo + address_in_region / blocksize * blocksize;
   if (end)
-    *end = (address + blocksize - 1) / blocksize * blocksize;
+    *end = region->lo + (address_in_region + blocksize - 1) / blocksize *
blocksize;
 }

 /* Given the list of memory requests to be WRITTEN, this function



-- 
Regards,
Mark
markrages@gmail

[PATCH] Re: Flash memory size not aligned to address

[Mark Rages]

Who can review my patch?

Regards,
Mark

On Oct 5, 2017 3:49 PM, "Mark Rages" <markrages@gmail.com> wrote:

The Nordic nRF52 memory map, reported from black magic probe:

Num Enb Low Addr High Addr Attrs
0 y 0x00000000 0x00080000 flash blocksize 0x1000 nocache
1 y 0x10001000 0x10001210 flash blocksize 0x210 nocache
2 y 0x20000000 0x20010000 rw nocache

The region at 0x10001000 is "UICR" and it is a section of flash that is
erased all at once.

Notice the odd size: 0x210 is the size of the region defined in the
datasheet.

But because the block size was listed as 0x210, gdb was insisting on
issuing two erase commands divisible by 0x210, starting below 0x10001000.

This patch fixes it by doing the alignment computation from the start of
the region, not from address 0:

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d8d5772..d2dc194 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-10-05  Mark Rages <markrages@gmail.com>
+
+       * target-memory.c (block_boundaries): Fix for block address not
+       aligned on block size.
+
 2017-10-05  Tristan Gingold  <tgingold@free.fr>

        * MAINTAINERS (Misc): Update my email address.
@@ -6962,7 +6967,7 @@

 2017-05-08  Alan Hayward  <alan.hayward@arm.com>

-       * mn10300-linux-tdep.c (am33_supply_gregset_method): Use
+       * mn10300-linux-tdep.c (am33_supply_gregset_method): Use
        regcache->raw_supply_zeroed.

 2017-05-06  Sergio Durigan Junior  <sergiodj@redhat.com>
diff --git a/gdb/target-memory.c b/gdb/target-memory.c
index 1c8faa8..4651200 100644
--- a/gdb/target-memory.c
+++ b/gdb/target-memory.c
@@ -138,14 +138,18 @@ block_boundaries (CORE_ADDR address, CORE_ADDR
*begin, CORE_ADDR *end)
 {
   struct mem_region *region;
   unsigned blocksize;
+  CORE_ADDR address_in_region;

   region = lookup_mem_region (address);
   gdb_assert (region->attrib.mode == MEM_FLASH);
   blocksize = region->attrib.blocksize;
+
+  address_in_region = address - region->lo;
+
   if (begin)
-    *begin = address / blocksize * blocksize;
+    *begin = region->lo + address_in_region / blocksize * blocksize;
   if (end)
-    *end = (address + blocksize - 1) / blocksize * blocksize;
+    *end = region->lo + (address_in_region + blocksize - 1) / blocksize *
blocksize;
 }

 /* Given the list of memory requests to be WRITTEN, this function



-- 
Regards,
Mark
markrages@gmail

Re: [PATCH] Re: Flash memory size not aligned to address

[Kevin Buettner]

Hi Mark,

On Sat, 7 Oct 2017 12:36:53 -0600
Mark Rages <markrages@gmail.com> wrote:

> Who can review my patch?

As I understand it, suggested timing for patch pings is 2 weeks after
the patch is first submitted and then one week after that.

I did notice, however, that you added [PATCH] to the subject line,
so perhaps you were just getting the subject line formatted in
such a way so that it might be noticed?

Regardless, I did look over your patch and have some comments below.

But, first...

Do you have an FSF Copyright Assignment?

See:

https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment

(That page has other useful information too.)

> On Oct 5, 2017 3:49 PM, "Mark Rages" <markrages@gmail.com> wrote:
> 
> The Nordic nRF52 memory map, reported from black magic probe:
> 
> Num Enb Low Addr High Addr Attrs
> 0 y 0x00000000 0x00080000 flash blocksize 0x1000 nocache
> 1 y 0x10001000 0x10001210 flash blocksize 0x210 nocache
> 2 y 0x20000000 0x20010000 rw nocache
> 
> The region at 0x10001000 is "UICR" and it is a section of flash that is
> erased all at once.
> 
> Notice the odd size: 0x210 is the size of the region defined in the
> datasheet.
> 
> But because the block size was listed as 0x210, gdb was insisting on
> issuing two erase commands divisible by 0x210, starting below 0x10001000.
> 
> This patch fixes it by doing the alignment computation from the start of
> the region, not from address 0:
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index d8d5772..d2dc194 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2017-10-05  Mark Rages <markrages@gmail.com>
> +
> +       * target-memory.c (block_boundaries): Fix for block address not
> +       aligned on block size.
> +

ChangeLog entries are generally not posted as patches.  It is unlikely
that this portion of the patch will cleanly apply once it's approved.

> diff --git a/gdb/target-memory.c b/gdb/target-memory.c
> index 1c8faa8..4651200 100644
> --- a/gdb/target-memory.c
> +++ b/gdb/target-memory.c
> @@ -138,14 +138,18 @@ block_boundaries (CORE_ADDR address, CORE_ADDR
> *begin, CORE_ADDR *end)
>  {
>    struct mem_region *region;
>    unsigned blocksize;
> +  CORE_ADDR address_in_region;
> 
>    region = lookup_mem_region (address);
>    gdb_assert (region->attrib.mode == MEM_FLASH);
>    blocksize = region->attrib.blocksize;
> +
> +  address_in_region = address - region->lo;
> +
>    if (begin)
> -    *begin = address / blocksize * blocksize;
> +    *begin = region->lo + address_in_region / blocksize * blocksize;
>    if (end)
> -    *end = (address + blocksize - 1) / blocksize * blocksize;
> +    *end = region->lo + (address_in_region + blocksize - 1) / blocksize *
> blocksize;
>  }
> 
>  /* Given the list of memory requests to be WRITTEN, this function

It occurs to me that address_in_region might be better named
offset_in_region.

It seems to me that there will be no difference in behavior for
targets whose region boundaries are already aligned to the block size. 
I do wonder, however, about behavior on other targets that don't meet
this criteria.  I'm hoping that someone else who has more experience in
this area will comment.

Otherwise, if you agree regarding the variable name, please make that
change and also let us know about whether you have an FSF assignment.

Kevin

Re: [PATCH] Re: Flash memory size not aligned to address

[Mark Rages]

On Sat, Oct 7, 2017 at 3:42 PM, Kevin Buettner <kevinb@redhat.com> wrote:
>
> Hi Mark,
>
> I did notice, however, that you added [PATCH] to the subject line,
> so perhaps you were just getting the subject line formatted in
> such a way so that it might be noticed?


Indeed, that was my plan.

>
> Do you have an FSF Copyright Assignment?
>

No.

> ChangeLog entries are generally not posted as patches.  It is unlikely
> that this portion of the patch will cleanly apply once it's approved.

Ok.  Thank you for pointing me to the wiki.  I was going off of
gdb/CONTRIBUTE.  Which one is authoritative?

> It occurs to me that address_in_region might be better named
> offset_in_region.

Updated patch follows.

> It seems to me that there will be no difference in behavior for
> targets whose region boundaries are already aligned to the block size.
> I do wonder, however, about behavior on other targets that don't meet
> this criteria.  I'm hoping that someone else who has more experience in
> this area will comment.

The only situation I can think of would be a device that begins a
section of flash unaligned, but requires an aligned address for the
erase command.  I'm not aware of such, but it's a big world.

Anyway, thanks for looking it over. Here goes second try:

gdb/ChangeLog:

        * target-memory.c (block_boundaries): Fix for block address not
        aligned on block size.

diff --git a/gdb/target-memory.c b/gdb/target-memory.c
index 1c8faa8..7f048de 100644
--- a/gdb/target-memory.c
+++ b/gdb/target-memory.c
@@ -138,14 +138,18 @@ block_boundaries (CORE_ADDR address, CORE_ADDR
*begin, CORE_ADDR *end)
 {
   struct mem_region *region;
   unsigned blocksize;
+  CORE_ADDR offset_in_region;

   region = lookup_mem_region (address);
   gdb_assert (region->attrib.mode == MEM_FLASH);
   blocksize = region->attrib.blocksize;
+
+  offset_in_region = address - region->lo;
+
   if (begin)
-    *begin = address / blocksize * blocksize;
+    *begin = region->lo + offset_in_region / blocksize * blocksize;
   if (end)
-    *end = (address + blocksize - 1) / blocksize * blocksize;
+    *end = region->lo + (offset_in_region + blocksize - 1) /
blocksize * blocksize;
 }

-- 
Regards,
Mark
markrages@gmail

Re: [PATCH] Re: Flash memory size not aligned to address

[Kevin Buettner]

On Mon, 9 Oct 2017 10:28:13 -0600
Mark Rages <markrages@gmail.com> wrote:

> > Do you have an FSF Copyright Assignment?
> >  
> 
> No.

According to this link...

https://www.gnu.org/prep/maintain/maintain.html#Legally-Significant

...contributions that consist of more than about 15 lines of code or
text are significant for copyright purposes.

Your patch is well under that amount, so I do not think that you
*need* a copyright assignment for this particular change.

However, if you plan to make other contributions to GDB, I encourage
you to have a copyright assignment on file with the FSF.

Please let us know if you intend to pursue a copyright assignment.
If not, I can commit/push this change for you.

> > ChangeLog entries are generally not posted as patches.  It is unlikely
> > that this portion of the patch will cleanly apply once it's approved.  
> 
> Ok.  Thank you for pointing me to the wiki.  I was going off of
> gdb/CONTRIBUTE.  Which one is authoritative?

I think that gdb/CONTRIBUTE is meant to be authoritative.

But I find that the wiki document provides some really useful nuts
and bolts type of stuff for contributing to GDB.  Personally, I find
it to be a lot more useful than gdb/CONTRIBUTE.

For example, the wiki contains some really useful info on how to
format your commit comment when using git.  It also contains some
good information on formatting ChangeLog entries.

> > It occurs to me that address_in_region might be better named
> > offset_in_region.  
> 
> Updated patch follows.
> 
> > It seems to me that there will be no difference in behavior for
> > targets whose region boundaries are already aligned to the block size.
> > I do wonder, however, about behavior on other targets that don't meet
> > this criteria.  I'm hoping that someone else who has more experience in
> > this area will comment.  
> 
> The only situation I can think of would be a device that begins a
> section of flash unaligned, but requires an aligned address for the
> erase command.  I'm not aware of such, but it's a big world.
> 
> Anyway, thanks for looking it over. Here goes second try:
> 
> gdb/ChangeLog:
> 
>         * target-memory.c (block_boundaries): Fix for block address not
>         aligned on block size.
> 
> diff --git a/gdb/target-memory.c b/gdb/target-memory.c
> index 1c8faa8..7f048de 100644
> --- a/gdb/target-memory.c
> +++ b/gdb/target-memory.c
> @@ -138,14 +138,18 @@ block_boundaries (CORE_ADDR address, CORE_ADDR
> *begin, CORE_ADDR *end)
>  {
>    struct mem_region *region;
>    unsigned blocksize;
> +  CORE_ADDR offset_in_region;
> 
>    region = lookup_mem_region (address);
>    gdb_assert (region->attrib.mode == MEM_FLASH);
>    blocksize = region->attrib.blocksize;
> +
> +  offset_in_region = address - region->lo;
> +
>    if (begin)
> -    *begin = address / blocksize * blocksize;
> +    *begin = region->lo + offset_in_region / blocksize * blocksize;
>    if (end)
> -    *end = (address + blocksize - 1) / blocksize * blocksize;
> +    *end = region->lo + (offset_in_region + blocksize - 1) /
> blocksize * blocksize;
>  }
> 

This is okay.

As noted earlier, please let us know if you intend to pursue an FSF
copyright assignment.  If so, in order have write access to the git
repository, you'll need a sourceware account.  You should also submit
a patch adding yourself to the "Write After Approval" section of
gdb/MAINTAINERS.  (We can go into how all of that is accomplished
after the copyright assignment process is finished.)

Kevin

Re: [PATCH] Re: Flash memory size not aligned to address

[Mark Rages]

On Mon, Oct 9, 2017 at 11:51 AM, Kevin Buettner <kevinb@redhat.com> wrote:
>
> Please let us know if you intend to pursue a copyright assignment.
> If not, I can commit/push this change for you.

I am not pursuing copyright assignment right now.   Please push this
patch for me.

I am not opposed to assignment, but I will wait until I have a more
substantial contribution to make.

Regards,
Mark
markrages@gmail

Re: [PATCH] Re: Flash memory size not aligned to address

[Kevin Buettner]

On Mon, 9 Oct 2017 13:08:44 -0600
Mark Rages <markrages@gmail.com> wrote:

> On Mon, Oct 9, 2017 at 11:51 AM, Kevin Buettner <kevinb@redhat.com> wrote:
> >
> > Please let us know if you intend to pursue a copyright assignment.
> > If not, I can commit/push this change for you.  
> 
> I am not pursuing copyright assignment right now.   Please push this
> patch for me.

I've pushed it for you.  The ChangeLog entry is attributed to you.  The
git commit has my name on it, but I indicate where the patch came
from in the commit comment.

(I hope that's the correct procedure...)

Kevin

Re: [PATCH] Re: Flash memory size not aligned to address

[Pedro Alves]

On 10/11/2017 08:56 AM, Kevin Buettner wrote:
> On Mon, 9 Oct 2017 13:08:44 -0600
> Mark Rages <markrages@gmail.com> wrote:
> 
>> On Mon, Oct 9, 2017 at 11:51 AM, Kevin Buettner <kevinb@redhat.com> wrote:
>>>
>>> Please let us know if you intend to pursue a copyright assignment.
>>> If not, I can commit/push this change for you.  
>>
>> I am not pursuing copyright assignment right now.   Please push this
>> patch for me.
> 
> I've pushed it for you.  The ChangeLog entry is attributed to you.  The
> git commit has my name on it, but I indicate where the patch came
> from in the commit comment.
> 
> (I hope that's the correct procedure...)

Actually, the right procedure is to amend to commit's authorship too.
There are useful instructions here:

 https://sourceware.org/glibc/wiki/GlibcGit#Author_Attribution

Thanks,
Pedro Alves

Re: [PATCH] Re: Flash memory size not aligned to address

[Kevin Buettner]

On Wed, 11 Oct 2017 10:31:08 +0100
Pedro Alves <palves@redhat.com> wrote:

> On 10/11/2017 08:56 AM, Kevin Buettner wrote:
> > On Mon, 9 Oct 2017 13:08:44 -0600
> > Mark Rages <markrages@gmail.com> wrote:
> >   
> >> On Mon, Oct 9, 2017 at 11:51 AM, Kevin Buettner <kevinb@redhat.com> wrote:  
> >>>
> >>> Please let us know if you intend to pursue a copyright assignment.
> >>> If not, I can commit/push this change for you.    
> >>
> >> I am not pursuing copyright assignment right now.   Please push this
> >> patch for me.  
> > 
> > I've pushed it for you.  The ChangeLog entry is attributed to you.  The
> > git commit has my name on it, but I indicate where the patch came
> > from in the commit comment.
> > 
> > (I hope that's the correct procedure...)  
> 
> Actually, the right procedure is to amend to commit's authorship too.
> There are useful instructions here:
> 
>  https://sourceware.org/glibc/wiki/GlibcGit#Author_Attribution

Okay, thanks.

I gather it too late to amend it now, right?  (Since git commit --amend
only works on the most recent commit.)

Kevin

Re: [PATCH] Re: Flash memory size not aligned to address

[Pedro Alves]

On 10/11/2017 04:53 PM, Kevin Buettner wrote:

> I gather it too late to amend it now, right?  (Since git commit --amend
> only works on the most recent commit.)

Right, it's too late, but the reason is we don't allow history
rewrites of the upstream repo; we only allow fast-forward merges
to keep history linear -- even if it were the most recent commit,
you wouldn't be able to push it.  (And it's possible to amend
more than one commit.)  The closest would be to revert the patch, and
reapply the original patch with author fixed.  (Not sure it's
worth complicating git history a bit, but it's easy enough to
do if desired.)

Thanks,
Pedro Alves

Re: [PATCH] Re: Flash memory size not aligned to address

[Kevin Buettner]

On Wed, 11 Oct 2017 17:11:17 +0100
Pedro Alves <palves@redhat.com> wrote:

> On 10/11/2017 04:53 PM, Kevin Buettner wrote:
> 
> > I gather it too late to amend it now, right?  (Since git commit --amend
> > only works on the most recent commit.)  
> 
> Right, it's too late, but the reason is we don't allow history
> rewrites of the upstream repo; we only allow fast-forward merges
> to keep history linear -- even if it were the most recent commit,
> you wouldn't be able to push it.  (And it's possible to amend
> more than one commit.)  The closest would be to revert the patch, and
> reapply the original patch with author fixed.  (Not sure it's
> worth complicating git history a bit, but it's easy enough to
> do if desired.)

Thanks for the explanation.

Unless someone strenuously objects, I'll leave it as it is.  As noted
earlier, Mark is credited in both the ChangeLog as well as in the
commit comment.

Thanks again,

Kevin