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

[Kevin Buettner <kevinb@redhat.com>]

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