Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Carlo Caione]

On Tue, 2018-07-24 at 10:07 +0200, Dr. Philipp Tomsich wrote:
> 
> I have a patchset for changing the relevant fields in U-Boot to allow
> for 33bits (i.e. using u64) for the RAM size and it finally passes
> Travis cleanly.
> It will be another round or two of cleanup before I can submit the
> series — once this happens, I’ll copy you so you can give your
> Tested-by if it works…

I'm interested. Please put me in CC as well.

-- 
Carlo Caione 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Dr. Philipp Tomsich]

Marty,

> On 6 Jul 2018, at 05:11, Marty E. Plummer  wrote:
> 
> On Sat, May 19, 2018 at 02:08:53PM +0200, Dr. Philipp Tomsich wrote:
>> Marty,
>> 
>>> On 19 May 2018, at 12:40, Marty E. Plummer  wrote:
>>> 
>>> So explain to me what you'd like me to do here, if you would. What I
>>> gather from this is you want me to flip CONFIG_PHYS_64BIT and see if it
>>> works or what? I can flash/reflash u-boot and coreboot pretty easily on
>>> the device, so I'm down for any sort of hardware testing needed to get
>>> this into a usable state.
>> 
>> Yes, just enable PHYS_64BIT and report on how far it goes (activating some
>> debug may be helpful to understand what goes wrong, if it fails).
>> 
>> My gut feeling is that it could work, but there’s a number of pitfalls and 
>> we may
>> not be lucky.
>> 
> Testing flipping CONFIG_PHYS_64BIT, both with and without my 'clamping'
> patch to sdram_common.c, has the same results, in that all that is
> output on the servo console is that wierd  output. Where to from
> here, then?

I have a patchset for changing the relevant fields in U-Boot to allow for 
33bits (i.e. using u64) for the RAM size and it finally passes Travis cleanly.
It will be another round or two of cleanup before I can submit the series — 
once this happens, I’ll copy you so you can give your Tested-by if it works…

Thanks,
Phil.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Dr. Philipp Tomsich]

> On 10 Jul 2018, at 16:41, Simon Glass  wrote:
> 
> Hi,
> 
> On 8 May 2018 at 04:21, Dr. Philipp Tomsich
>  > wrote:
>> Marty,
>> 
>>> On 8 May 2018, at 02:52, Marty E. Plummer  wrote:
>>> 
>>> On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote:
 
> On 7 May 2018, at 04:34, Marty E. Plummer  wrote:
> 
> On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
>> Hi Marty,
>> 
>> 
>> On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
>>> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
>>> 
>>> Without this change, my u-boot build for the asus c201 chromebook (4GiB)
>>> is incorrectly detected as 0 Bytes of ram.
>> 
>> I know the root cause for this issue, and I have a local patch for it.
>> The rk3288 is 32bit, and 4GB size is just out of range, so we need to 
>> before
>> the max size before return with '<<20'. Sorry for forgot to send it out.
>> 
>>> 
>>> Signed-off-by: Marty E. Plummer 
>>> ---
>>> arch/arm/mach-rockchip/sdram_common.c | 62 ---
>>> 1 file changed, 37 insertions(+), 25 deletions(-)
>>> 
>>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
>>> b/arch/arm/mach-rockchip/sdram_common.c
>>> index 76dbdc8715..a9c9f970a4 100644
>>> --- a/arch/arm/mach-rockchip/sdram_common.c
>>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>>> @@ -10,6 +10,8 @@
>>> #include 
>>> #include 
>>> #include 
>>> +#include 
>>> +#include 
>>> 
>>> DECLARE_GLOBAL_DATA_PTR;
>>> size_t rockchip_sdram_size(phys_addr_t reg)
>>> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>>>  size_t size_mb = 0;
>>>  u32 ch;
>>> 
>>> - u32 sys_reg = readl(reg);
>>> - u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
>>> -& SYS_REG_NUM_CH_MASK);
>>> + if (!size_mb) {
>> 
>> I don't understand this and follow up changes, we don't really need it,
>> isn't it?
>> I think don't need the changes before here.
> Yeah, that was just another level of indentation for the if (!size_mb)
> guard, but I've reworked the patch to not do that as it was pointed out
> that since size_mb is initialized to 0 prior.
>>> + /*
>>> +  * we use the 0x~0xfeff space
>>> +  * since 0xff00~0x is soc register space
>>> +  * so we reserve it
>>> +  */
>>> + size_mb = min(size_mb, 0xff00/SZ_1M);
>> 
>> This is what we really need, as Klaus point out, we need to use
>> SDRAM_MAX_SIZE
>> instead of hard code.
> Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
> build and boot tested on my hardware.
 
 In that case you just masked the problem but didn???t solve it: assuming 
 size_mb
 is size_t (I???ll assume this is 64bit, but did not check), then your 4GB 
 is 0x1__ )
 which overflows to 0x0 when converted to a u32.
 
 In other words: we need to figure out where the truncation occurs (image 
 what
 happens if a new 32bit processor with LPAE comes out???).
 
>>> A very valid point. With the following patch to sdram_common.c and
>>> sdram_rk3288.c applied I get the debug output that follows it:
>>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
>>> b/arch/arm/mach-rockchip/sdram_common.c
>>> index 232a7fa655..0fe69bf558 100644
>>> --- a/arch/arm/mach-rockchip/sdram_common.c
>>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>>> @@ -4,6 +4,7 @@
>>> * SPDX-License-Identifier: GPL-2.0+
>>> */
>>> 
>>> +#define DEBUG 1
>>> #include 
>>> #include 
>>> #include 
>>> @@ -39,16 +40,19 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>>>  SYS_REG_ROW_3_4_MASK;
>>> 
>>>  chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
>>> + debug("%s: %d: chipsize_mb %x\n", __func__, __LINE__, 
>>> chipsize_mb);
>>> 
>>>  if (rank > 1)
>>>  chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
>>>  if (row_3_4)
>>>  chipsize_mb = chipsize_mb * 3 / 4;
>>>  size_mb += chipsize_mb;
>>> + debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>>>  debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
>>>rank, col, bk, cs0_row, bw, row_3_4);
>>>  }
>>> 
>>> + debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>>>  size_mb = min(size_mb, SDRAM_MAX_SIZE/SZ_1M);
>>> 
>>>  return (size_t)size_mb << 20;
>>> diff --git a/drivers/ram/rockchip/sdram_rk3288.c 
>>> b/drivers/ram/rockchip/sdram_rk3288.c
>>> index d99bf12476..9738eb088f 100644
>>> --- a/drivers/ram/rockchip/sdram_rk3288.c
>>> +++ b/drivers/ram/rockchip/sdram_rk3288.c
>>> @@ -7,6 +7,7 @@
>>> * Adapted from coreboot.

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Simon Glass]

Hi,

On 8 May 2018 at 04:21, Dr. Philipp Tomsich
 wrote:
> Marty,
>
>> On 8 May 2018, at 02:52, Marty E. Plummer  wrote:
>>
>> On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote:
>>>
 On 7 May 2018, at 04:34, Marty E. Plummer  wrote:

 On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
> Hi Marty,
>
>
> On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
>> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
>>
>> Without this change, my u-boot build for the asus c201 chromebook (4GiB)
>> is incorrectly detected as 0 Bytes of ram.
>
> I know the root cause for this issue, and I have a local patch for it.
> The rk3288 is 32bit, and 4GB size is just out of range, so we need to 
> before
> the max size before return with '<<20'. Sorry for forgot to send it out.
>
>>
>> Signed-off-by: Marty E. Plummer 
>> ---
>> arch/arm/mach-rockchip/sdram_common.c | 62 ---
>> 1 file changed, 37 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
>> b/arch/arm/mach-rockchip/sdram_common.c
>> index 76dbdc8715..a9c9f970a4 100644
>> --- a/arch/arm/mach-rockchip/sdram_common.c
>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>> @@ -10,6 +10,8 @@
>> #include 
>> #include 
>> #include 
>> +#include 
>> +#include 
>>
>> DECLARE_GLOBAL_DATA_PTR;
>> size_t rockchip_sdram_size(phys_addr_t reg)
>> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>>   size_t size_mb = 0;
>>   u32 ch;
>>
>> - u32 sys_reg = readl(reg);
>> - u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
>> -& SYS_REG_NUM_CH_MASK);
>> + if (!size_mb) {
>
> I don't understand this and follow up changes, we don't really need it,
> isn't it?
> I think don't need the changes before here.
 Yeah, that was just another level of indentation for the if (!size_mb)
 guard, but I've reworked the patch to not do that as it was pointed out
 that since size_mb is initialized to 0 prior.
>> + /*
>> +  * we use the 0x~0xfeff space
>> +  * since 0xff00~0x is soc register space
>> +  * so we reserve it
>> +  */
>> + size_mb = min(size_mb, 0xff00/SZ_1M);
>
> This is what we really need, as Klaus point out, we need to use
> SDRAM_MAX_SIZE
> instead of hard code.
 Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
 build and boot tested on my hardware.
>>>
>>> In that case you just masked the problem but didn???t solve it: assuming 
>>> size_mb
>>> is size_t (I???ll assume this is 64bit, but did not check), then your 4GB 
>>> is 0x1__ )
>>> which overflows to 0x0 when converted to a u32.
>>>
>>> In other words: we need to figure out where the truncation occurs (image 
>>> what
>>> happens if a new 32bit processor with LPAE comes out???).
>>>
>> A very valid point. With the following patch to sdram_common.c and
>> sdram_rk3288.c applied I get the debug output that follows it:
>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
>> b/arch/arm/mach-rockchip/sdram_common.c
>> index 232a7fa655..0fe69bf558 100644
>> --- a/arch/arm/mach-rockchip/sdram_common.c
>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>> @@ -4,6 +4,7 @@
>>  * SPDX-License-Identifier: GPL-2.0+
>>  */
>>
>> +#define DEBUG 1
>> #include 
>> #include 
>> #include 
>> @@ -39,16 +40,19 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>>   SYS_REG_ROW_3_4_MASK;
>>
>>   chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
>> + debug("%s: %d: chipsize_mb %x\n", __func__, __LINE__, 
>> chipsize_mb);
>>
>>   if (rank > 1)
>>   chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
>>   if (row_3_4)
>>   chipsize_mb = chipsize_mb * 3 / 4;
>>   size_mb += chipsize_mb;
>> + debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>>   debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
>> rank, col, bk, cs0_row, bw, row_3_4);
>>   }
>>
>> + debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>>   size_mb = min(size_mb, SDRAM_MAX_SIZE/SZ_1M);
>>
>>   return (size_t)size_mb << 20;
>> diff --git a/drivers/ram/rockchip/sdram_rk3288.c 
>> b/drivers/ram/rockchip/sdram_rk3288.c
>> index d99bf12476..9738eb088f 100644
>> --- a/drivers/ram/rockchip/sdram_rk3288.c
>> +++ b/drivers/ram/rockchip/sdram_rk3288.c
>> @@ -7,6 +7,7 @@
>>  * Adapted from coreboot.
>>  */
>>
>> +#define DEBUG 1
>> #include 
>> #include 
>> #include 
>>
>> ---
>> U-Boot SPL 2018.05-rc3-02370-g309384e84b-dirty (May 07 2018 - 19:42:15 -0500)
>> Trying to boot from SPI
>>
>>
>> U-Boot 

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer]

On Sat, May 19, 2018 at 02:08:53PM +0200, Dr. Philipp Tomsich wrote:
> Marty,
> 
> > On 19 May 2018, at 12:40, Marty E. Plummer  wrote:
> > 
> > So explain to me what you'd like me to do here, if you would. What I
> > gather from this is you want me to flip CONFIG_PHYS_64BIT and see if it
> > works or what? I can flash/reflash u-boot and coreboot pretty easily on
> > the device, so I'm down for any sort of hardware testing needed to get
> > this into a usable state.
> 
> Yes, just enable PHYS_64BIT and report on how far it goes (activating some
> debug may be helpful to understand what goes wrong, if it fails).
> 
> My gut feeling is that it could work, but there’s a number of pitfalls and we 
> may
> not be lucky.
> 
Testing flipping CONFIG_PHYS_64BIT, both with and without my 'clamping'
patch to sdram_common.c, has the same results, in that all that is
output on the servo console is that wierd  output. Where to from
here, then?
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Dr. Philipp Tomsich]

Marty,

> On 19 May 2018, at 12:40, Marty E. Plummer  wrote:
> 
> On Mon, May 14, 2018 at 05:56:27PM +0200, Dr. Philipp Tomsich wrote:
>> I had a bit more time to look into this and it looks as if we have two 
>> problem-spots...
>> 
>> First, there's a type-mismatch between ram_info.size (a size_t) and 
>> gd.ram_size (phys_size_t).
>> While we can increase the size of a phys_size_t to 64bit (by defining 
>> CONFIG_PHYS_64BIT),
>> the size_t will always be an unsigned int on a 32bit arm architecture.  So 
>> here's one possible
>> pitfall that should be resolved.
>> 
>> Once this is adjusted, we might just increase the width of ram_info.size to 
>> 64bit by enabling
>> CONFIG_PHYS_64BIT ... however, this comes with a caveat: the default cell 
>> sizes for the
>> FDT (via fdtdec) also increases.  I.e. if any come in our arch or the 
>> drivers still goes through
>> the fdtdec-functions, we'll end up with a problem.
>> 
>> As my test coverage is limited to 64bit targets, I can't tell whether 
>> defining the PHYS_64BIT
>> configuration would be possible for the RK3288 -- if it is, we'd have a 
>> rather easy way forward
>> and could reuse the phys_size_t for ram_info.size.
>> 
>> I'd appreciate if you could take a look at whether CONFIG_PHYS_64BIT gets us 
>> into a lot
>> of trouble on the RK3288 or whether this will trigger just a few minor 
>> adjustments...
>> 
>> Thanks,
>> Philipp.
> 
> So explain to me what you'd like me to do here, if you would. What I
> gather from this is you want me to flip CONFIG_PHYS_64BIT and see if it
> works or what? I can flash/reflash u-boot and coreboot pretty easily on
> the device, so I'm down for any sort of hardware testing needed to get
> this into a usable state.

Yes, just enable PHYS_64BIT and report on how far it goes (activating some
debug may be helpful to understand what goes wrong, if it fails).

My gut feeling is that it could work, but there’s a number of pitfalls and we 
may
not be lucky.

Regards,
Philipp.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer]

On Mon, May 14, 2018 at 05:56:27PM +0200, Dr. Philipp Tomsich wrote:
> I had a bit more time to look into this and it looks as if we have two 
> problem-spots...
> 
> First, there's a type-mismatch between ram_info.size (a size_t) and 
> gd.ram_size (phys_size_t).
> While we can increase the size of a phys_size_t to 64bit (by defining 
> CONFIG_PHYS_64BIT),
> the size_t will always be an unsigned int on a 32bit arm architecture.  So 
> here's one possible
> pitfall that should be resolved.
> 
> Once this is adjusted, we might just increase the width of ram_info.size to 
> 64bit by enabling
> CONFIG_PHYS_64BIT ... however, this comes with a caveat: the default cell 
> sizes for the
> FDT (via fdtdec) also increases.  I.e. if any come in our arch or the drivers 
> still goes through
> the fdtdec-functions, we'll end up with a problem.
> 
> As my test coverage is limited to 64bit targets, I can't tell whether 
> defining the PHYS_64BIT
> configuration would be possible for the RK3288 -- if it is, we'd have a 
> rather easy way forward
> and could reuse the phys_size_t for ram_info.size.
> 
> I'd appreciate if you could take a look at whether CONFIG_PHYS_64BIT gets us 
> into a lot
> of trouble on the RK3288 or whether this will trigger just a few minor 
> adjustments...
> 
> Thanks,
> Philipp.

So explain to me what you'd like me to do here, if you would. What I
gather from this is you want me to flip CONFIG_PHYS_64BIT and see if it
works or what? I can flash/reflash u-boot and coreboot pretty easily on
the device, so I'm down for any sort of hardware testing needed to get
this into a usable state.

Regards,
Marty.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Dr. Philipp Tomsich]

I had a bit more time to look into this and it looks as if we have two 
problem-spots...

First, there's a type-mismatch between ram_info.size (a size_t) and gd.ram_size 
(phys_size_t).
While we can increase the size of a phys_size_t to 64bit (by defining 
CONFIG_PHYS_64BIT),
the size_t will always be an unsigned int on a 32bit arm architecture.  So 
here’s one possible
pitfall that should be resolved.

Once this is adjusted, we might just increase the width of ram_info.size to 
64bit by enabling
CONFIG_PHYS_64BIT … however, this comes with a caveat: the default cell sizes 
for the
FDT (via fdtdec) also increases.  I.e. if any come in our arch or the drivers 
still goes through
the fdtdec-functions, we’ll end up with a problem.

As my test coverage is limited to 64bit targets, I can’t tell whether defining 
the PHYS_64BIT
configuration would be possible for the RK3288 — if it is, we’d have a rather 
easy way forward
and could reuse the phys_size_t for ram_info.size.

I’d appreciate if you could take a look at whether CONFIG_PHYS_64BIT gets us 
into a lot
of trouble on the RK3288 or whether this will trigger just a few minor 
adjustments…

Thanks,
Philipp.

> On 9 May 2018, at 09:24, Dr. Philipp Tomsich 
>  wrote:
> 
>> 
>> On 9 May 2018, at 07:29, Marty E. Plummer  wrote:
>> 
>> On Tue, May 08, 2018 at 11:08:14PM +0200, Dr. Philipp Tomsich wrote:
>>> 
 On 8 May 2018, at 21:21, Marty E. Plummer  wrote:
 
 On Tue, May 08, 2018 at 12:21:24PM +0200, Dr. Philipp Tomsich wrote:
> Marty,
> 
>> On 8 May 2018, at 02:52, Marty E. Plummer  wrote:
>> 
>> On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote:
>>> 
 On 7 May 2018, at 04:34, Marty E. Plummer  
 wrote:
 
 On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
> Hi Marty,
> 
> 
> On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
>> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
>> 
>> Without this change, my u-boot build for the asus c201 chromebook 
>> (4GiB)
>> is incorrectly detected as 0 Bytes of ram.
> 
> I know the root cause for this issue, and I have a local patch for it.
> The rk3288 is 32bit, and 4GB size is just out of range, so we need to 
> before
> the max size before return with '<<20'. Sorry for forgot to send it 
> out.
> 
>> 
>> Signed-off-by: Marty E. Plummer 
>> ---
>> arch/arm/mach-rockchip/sdram_common.c | 62 
>> ---
>> 1 file changed, 37 insertions(+), 25 deletions(-)
>> 
>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
>> b/arch/arm/mach-rockchip/sdram_common.c
>> index 76dbdc8715..a9c9f970a4 100644
>> --- a/arch/arm/mach-rockchip/sdram_common.c
>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>> @@ -10,6 +10,8 @@
>> #include 
>> #include 
>> #include 
>> +#include 
>> +#include 
>> 
>> DECLARE_GLOBAL_DATA_PTR;
>> size_t rockchip_sdram_size(phys_addr_t reg)
>> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>>  size_t size_mb = 0;
>>  u32 ch;
>> 
>> -u32 sys_reg = readl(reg);
>> -u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
>> -   & SYS_REG_NUM_CH_MASK);
>> +if (!size_mb) {
> 
> I don't understand this and follow up changes, we don't really need 
> it,
> isn't it?
> I think don't need the changes before here.
 Yeah, that was just another level of indentation for the if (!size_mb)
 guard, but I've reworked the patch to not do that as it was pointed out
 that since size_mb is initialized to 0 prior.
>> +/*
>> + * we use the 0x~0xfeff space
>> + * since 0xff00~0x is soc register space
>> + * so we reserve it
>> + */
>> +size_mb = min(size_mb, 0xff00/SZ_1M);
> 
> This is what we really need, as Klaus point out, we need to use
> SDRAM_MAX_SIZE
> instead of hard code.
 Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as 
 instructed,
 build and boot tested on my hardware.
>>> 
>>> In that case you just masked the problem but didn???t solve it: 
>>> assuming size_mb
>>> is size_t (I???ll assume this is 64bit, but did not check), then your 
>>> 4GB is 0x1__ )
>>> which overflows to 0x0 when converted to a u32.
>>> 
>>> In other words: we need 

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Dr. Philipp Tomsich]

> On 9 May 2018, at 07:29, Marty E. Plummer  wrote:
> 
> On Tue, May 08, 2018 at 11:08:14PM +0200, Dr. Philipp Tomsich wrote:
>> 
>>> On 8 May 2018, at 21:21, Marty E. Plummer  wrote:
>>> 
>>> On Tue, May 08, 2018 at 12:21:24PM +0200, Dr. Philipp Tomsich wrote:
 Marty,
 
> On 8 May 2018, at 02:52, Marty E. Plummer  wrote:
> 
> On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote:
>> 
>>> On 7 May 2018, at 04:34, Marty E. Plummer  
>>> wrote:
>>> 
>>> On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
 Hi Marty,
 
 
 On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> 
> Without this change, my u-boot build for the asus c201 chromebook 
> (4GiB)
> is incorrectly detected as 0 Bytes of ram.
 
 I know the root cause for this issue, and I have a local patch for it.
 The rk3288 is 32bit, and 4GB size is just out of range, so we need to 
 before
 the max size before return with '<<20'. Sorry for forgot to send it 
 out.
 
> 
> Signed-off-by: Marty E. Plummer 
> ---
> arch/arm/mach-rockchip/sdram_common.c | 62 ---
> 1 file changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> b/arch/arm/mach-rockchip/sdram_common.c
> index 76dbdc8715..a9c9f970a4 100644
> --- a/arch/arm/mach-rockchip/sdram_common.c
> +++ b/arch/arm/mach-rockchip/sdram_common.c
> @@ -10,6 +10,8 @@
> #include 
> #include 
> #include 
> +#include 
> +#include 
> 
> DECLARE_GLOBAL_DATA_PTR;
> size_t rockchip_sdram_size(phys_addr_t reg)
> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>   size_t size_mb = 0;
>   u32 ch;
> 
> - u32 sys_reg = readl(reg);
> - u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> -& SYS_REG_NUM_CH_MASK);
> + if (!size_mb) {
 
 I don't understand this and follow up changes, we don't really need it,
 isn't it?
 I think don't need the changes before here.
>>> Yeah, that was just another level of indentation for the if (!size_mb)
>>> guard, but I've reworked the patch to not do that as it was pointed out
>>> that since size_mb is initialized to 0 prior.
> + /*
> +  * we use the 0x~0xfeff space
> +  * since 0xff00~0x is soc register space
> +  * so we reserve it
> +  */
> + size_mb = min(size_mb, 0xff00/SZ_1M);
 
 This is what we really need, as Klaus point out, we need to use
 SDRAM_MAX_SIZE
 instead of hard code.
>>> Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
>>> build and boot tested on my hardware.
>> 
>> In that case you just masked the problem but didn???t solve it: assuming 
>> size_mb
>> is size_t (I???ll assume this is 64bit, but did not check), then your 
>> 4GB is 0x1__ )
>> which overflows to 0x0 when converted to a u32.
>> 
>> In other words: we need to figure out where the truncation occurs (image 
>> what
>> happens if a new 32bit processor with LPAE comes out???).
>> 
> A very valid point. With the following patch to sdram_common.c and
> sdram_rk3288.c applied I get the debug output that follows it:
> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> b/arch/arm/mach-rockchip/sdram_common.c
> index 232a7fa655..0fe69bf558 100644
> --- a/arch/arm/mach-rockchip/sdram_common.c
> +++ b/arch/arm/mach-rockchip/sdram_common.c
> @@ -4,6 +4,7 @@
> * SPDX-License-Identifier: GPL-2.0+
> */
> 
> +#define DEBUG 1
> #include 
> #include 
> #include 
> @@ -39,16 +40,19 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>   SYS_REG_ROW_3_4_MASK;
> 
>   chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> + debug("%s: %d: chipsize_mb %x\n", __func__, __LINE__, 
> chipsize_mb);
> 
>   if (rank > 1)
>   chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
>   if (row_3_4)
>   chipsize_mb = chipsize_mb * 3 / 4;
>   size_mb += chipsize_mb;
> + debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>   debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> rank, col, 

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer]

On Tue, May 08, 2018 at 11:08:14PM +0200, Dr. Philipp Tomsich wrote:
> 
> > On 8 May 2018, at 21:21, Marty E. Plummer  wrote:
> > 
> > On Tue, May 08, 2018 at 12:21:24PM +0200, Dr. Philipp Tomsich wrote:
> >> Marty,
> >> 
> >>> On 8 May 2018, at 02:52, Marty E. Plummer  wrote:
> >>> 
> >>> On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote:
>  
> > On 7 May 2018, at 04:34, Marty E. Plummer  
> > wrote:
> > 
> > On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
> >> Hi Marty,
> >> 
> >> 
> >> On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
> >>> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> >>> 
> >>> Without this change, my u-boot build for the asus c201 chromebook 
> >>> (4GiB)
> >>> is incorrectly detected as 0 Bytes of ram.
> >> 
> >> I know the root cause for this issue, and I have a local patch for it.
> >> The rk3288 is 32bit, and 4GB size is just out of range, so we need to 
> >> before
> >> the max size before return with '<<20'. Sorry for forgot to send it 
> >> out.
> >> 
> >>> 
> >>> Signed-off-by: Marty E. Plummer 
> >>> ---
> >>> arch/arm/mach-rockchip/sdram_common.c | 62 ---
> >>> 1 file changed, 37 insertions(+), 25 deletions(-)
> >>> 
> >>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> >>> b/arch/arm/mach-rockchip/sdram_common.c
> >>> index 76dbdc8715..a9c9f970a4 100644
> >>> --- a/arch/arm/mach-rockchip/sdram_common.c
> >>> +++ b/arch/arm/mach-rockchip/sdram_common.c
> >>> @@ -10,6 +10,8 @@
> >>> #include 
> >>> #include 
> >>> #include 
> >>> +#include 
> >>> +#include 
> >>> 
> >>> DECLARE_GLOBAL_DATA_PTR;
> >>> size_t rockchip_sdram_size(phys_addr_t reg)
> >>> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> >>>   size_t size_mb = 0;
> >>>   u32 ch;
> >>> 
> >>> - u32 sys_reg = readl(reg);
> >>> - u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> >>> -& SYS_REG_NUM_CH_MASK);
> >>> + if (!size_mb) {
> >> 
> >> I don't understand this and follow up changes, we don't really need it,
> >> isn't it?
> >> I think don't need the changes before here.
> > Yeah, that was just another level of indentation for the if (!size_mb)
> > guard, but I've reworked the patch to not do that as it was pointed out
> > that since size_mb is initialized to 0 prior.
> >>> + /*
> >>> +  * we use the 0x~0xfeff space
> >>> +  * since 0xff00~0x is soc register space
> >>> +  * so we reserve it
> >>> +  */
> >>> + size_mb = min(size_mb, 0xff00/SZ_1M);
> >> 
> >> This is what we really need, as Klaus point out, we need to use
> >> SDRAM_MAX_SIZE
> >> instead of hard code.
> > Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
> > build and boot tested on my hardware.
>  
>  In that case you just masked the problem but didn???t solve it: assuming 
>  size_mb
>  is size_t (I???ll assume this is 64bit, but did not check), then your 
>  4GB is 0x1__ )
>  which overflows to 0x0 when converted to a u32.
>  
>  In other words: we need to figure out where the truncation occurs (image 
>  what
>  happens if a new 32bit processor with LPAE comes out???).
>  
> >>> A very valid point. With the following patch to sdram_common.c and
> >>> sdram_rk3288.c applied I get the debug output that follows it:
> >>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> >>> b/arch/arm/mach-rockchip/sdram_common.c
> >>> index 232a7fa655..0fe69bf558 100644
> >>> --- a/arch/arm/mach-rockchip/sdram_common.c
> >>> +++ b/arch/arm/mach-rockchip/sdram_common.c
> >>> @@ -4,6 +4,7 @@
> >>> * SPDX-License-Identifier: GPL-2.0+
> >>> */
> >>> 
> >>> +#define DEBUG 1
> >>> #include 
> >>> #include 
> >>> #include 
> >>> @@ -39,16 +40,19 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> >>>   SYS_REG_ROW_3_4_MASK;
> >>> 
> >>>   chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> >>> + debug("%s: %d: chipsize_mb %x\n", __func__, __LINE__, 
> >>> chipsize_mb);
> >>> 
> >>>   if (rank > 1)
> >>>   chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
> >>>   if (row_3_4)
> >>>   chipsize_mb = chipsize_mb * 3 / 4;
> >>>   size_mb += chipsize_mb;
> >>> + debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
> >>>   debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> >>> rank, col, bk, cs0_row, bw, row_3_4);
> >>>   }
> >>> 
> >>> + debug("%s: %d: size_mb %x\n", 

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Dr. Philipp Tomsich]

> On 8 May 2018, at 21:21, Marty E. Plummer  wrote:
> 
> On Tue, May 08, 2018 at 12:21:24PM +0200, Dr. Philipp Tomsich wrote:
>> Marty,
>> 
>>> On 8 May 2018, at 02:52, Marty E. Plummer  wrote:
>>> 
>>> On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote:
 
> On 7 May 2018, at 04:34, Marty E. Plummer  wrote:
> 
> On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
>> Hi Marty,
>> 
>> 
>> On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
>>> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
>>> 
>>> Without this change, my u-boot build for the asus c201 chromebook (4GiB)
>>> is incorrectly detected as 0 Bytes of ram.
>> 
>> I know the root cause for this issue, and I have a local patch for it.
>> The rk3288 is 32bit, and 4GB size is just out of range, so we need to 
>> before
>> the max size before return with '<<20'. Sorry for forgot to send it out.
>> 
>>> 
>>> Signed-off-by: Marty E. Plummer 
>>> ---
>>> arch/arm/mach-rockchip/sdram_common.c | 62 ---
>>> 1 file changed, 37 insertions(+), 25 deletions(-)
>>> 
>>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
>>> b/arch/arm/mach-rockchip/sdram_common.c
>>> index 76dbdc8715..a9c9f970a4 100644
>>> --- a/arch/arm/mach-rockchip/sdram_common.c
>>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>>> @@ -10,6 +10,8 @@
>>> #include 
>>> #include 
>>> #include 
>>> +#include 
>>> +#include 
>>> 
>>> DECLARE_GLOBAL_DATA_PTR;
>>> size_t rockchip_sdram_size(phys_addr_t reg)
>>> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>>> size_t size_mb = 0;
>>> u32 ch;
>>> 
>>> -   u32 sys_reg = readl(reg);
>>> -   u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
>>> -  & SYS_REG_NUM_CH_MASK);
>>> +   if (!size_mb) {
>> 
>> I don't understand this and follow up changes, we don't really need it,
>> isn't it?
>> I think don't need the changes before here.
> Yeah, that was just another level of indentation for the if (!size_mb)
> guard, but I've reworked the patch to not do that as it was pointed out
> that since size_mb is initialized to 0 prior.
>>> +   /*
>>> +* we use the 0x~0xfeff space
>>> +* since 0xff00~0x is soc register space
>>> +* so we reserve it
>>> +*/
>>> +   size_mb = min(size_mb, 0xff00/SZ_1M);
>> 
>> This is what we really need, as Klaus point out, we need to use
>> SDRAM_MAX_SIZE
>> instead of hard code.
> Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
> build and boot tested on my hardware.
 
 In that case you just masked the problem but didn???t solve it: assuming 
 size_mb
 is size_t (I???ll assume this is 64bit, but did not check), then your 4GB 
 is 0x1__ )
 which overflows to 0x0 when converted to a u32.
 
 In other words: we need to figure out where the truncation occurs (image 
 what
 happens if a new 32bit processor with LPAE comes out???).
 
>>> A very valid point. With the following patch to sdram_common.c and
>>> sdram_rk3288.c applied I get the debug output that follows it:
>>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
>>> b/arch/arm/mach-rockchip/sdram_common.c
>>> index 232a7fa655..0fe69bf558 100644
>>> --- a/arch/arm/mach-rockchip/sdram_common.c
>>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>>> @@ -4,6 +4,7 @@
>>> * SPDX-License-Identifier: GPL-2.0+
>>> */
>>> 
>>> +#define DEBUG 1
>>> #include 
>>> #include 
>>> #include 
>>> @@ -39,16 +40,19 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>>> SYS_REG_ROW_3_4_MASK;
>>> 
>>> chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
>>> +   debug("%s: %d: chipsize_mb %x\n", __func__, __LINE__, 
>>> chipsize_mb);
>>> 
>>> if (rank > 1)
>>> chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
>>> if (row_3_4)
>>> chipsize_mb = chipsize_mb * 3 / 4;
>>> size_mb += chipsize_mb;
>>> +   debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>>> debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
>>>   rank, col, bk, cs0_row, bw, row_3_4);
>>> }
>>> 
>>> +   debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>>> size_mb = min(size_mb, SDRAM_MAX_SIZE/SZ_1M);
>>> 
>>> return (size_t)size_mb << 20;
>>> diff --git a/drivers/ram/rockchip/sdram_rk3288.c 
>>> b/drivers/ram/rockchip/sdram_rk3288.c
>>> index d99bf12476..9738eb088f 100644
>>> --- 

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer]

On Tue, May 08, 2018 at 12:21:24PM +0200, Dr. Philipp Tomsich wrote:
> Marty,
> 
> > On 8 May 2018, at 02:52, Marty E. Plummer  wrote:
> > 
> > On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote:
> >> 
> >>> On 7 May 2018, at 04:34, Marty E. Plummer  wrote:
> >>> 
> >>> On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
>  Hi Marty,
>  
>  
>  On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
> > Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> > 
> > Without this change, my u-boot build for the asus c201 chromebook (4GiB)
> > is incorrectly detected as 0 Bytes of ram.
>  
>  I know the root cause for this issue, and I have a local patch for it.
>  The rk3288 is 32bit, and 4GB size is just out of range, so we need to 
>  before
>  the max size before return with '<<20'. Sorry for forgot to send it out.
>  
> > 
> > Signed-off-by: Marty E. Plummer 
> > ---
> > arch/arm/mach-rockchip/sdram_common.c | 62 ---
> > 1 file changed, 37 insertions(+), 25 deletions(-)
> > 
> > diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> > b/arch/arm/mach-rockchip/sdram_common.c
> > index 76dbdc8715..a9c9f970a4 100644
> > --- a/arch/arm/mach-rockchip/sdram_common.c
> > +++ b/arch/arm/mach-rockchip/sdram_common.c
> > @@ -10,6 +10,8 @@
> > #include 
> > #include 
> > #include 
> > +#include 
> > +#include 
> > 
> > DECLARE_GLOBAL_DATA_PTR;
> > size_t rockchip_sdram_size(phys_addr_t reg)
> > @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> > size_t size_mb = 0;
> > u32 ch;
> > 
> > -   u32 sys_reg = readl(reg);
> > -   u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> > -  & SYS_REG_NUM_CH_MASK);
> > +   if (!size_mb) {
>  
>  I don't understand this and follow up changes, we don't really need it,
>  isn't it?
>  I think don't need the changes before here.
> >>> Yeah, that was just another level of indentation for the if (!size_mb)
> >>> guard, but I've reworked the patch to not do that as it was pointed out
> >>> that since size_mb is initialized to 0 prior.
> > +   /*
> > +* we use the 0x~0xfeff space
> > +* since 0xff00~0x is soc register space
> > +* so we reserve it
> > +*/
> > +   size_mb = min(size_mb, 0xff00/SZ_1M);
>  
>  This is what we really need, as Klaus point out, we need to use
>  SDRAM_MAX_SIZE
>  instead of hard code.
> >>> Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
> >>> build and boot tested on my hardware.
> >> 
> >> In that case you just masked the problem but didn???t solve it: assuming 
> >> size_mb
> >> is size_t (I???ll assume this is 64bit, but did not check), then your 4GB 
> >> is 0x1__ )
> >> which overflows to 0x0 when converted to a u32.
> >> 
> >> In other words: we need to figure out where the truncation occurs (image 
> >> what
> >> happens if a new 32bit processor with LPAE comes out???).
> >> 
> > A very valid point. With the following patch to sdram_common.c and
> > sdram_rk3288.c applied I get the debug output that follows it:
> > diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> > b/arch/arm/mach-rockchip/sdram_common.c
> > index 232a7fa655..0fe69bf558 100644
> > --- a/arch/arm/mach-rockchip/sdram_common.c
> > +++ b/arch/arm/mach-rockchip/sdram_common.c
> > @@ -4,6 +4,7 @@
> >  * SPDX-License-Identifier: GPL-2.0+
> >  */
> > 
> > +#define DEBUG 1
> > #include 
> > #include 
> > #include 
> > @@ -39,16 +40,19 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> > SYS_REG_ROW_3_4_MASK;
> > 
> > chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> > +   debug("%s: %d: chipsize_mb %x\n", __func__, __LINE__, 
> > chipsize_mb);
> > 
> > if (rank > 1)
> > chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
> > if (row_3_4)
> > chipsize_mb = chipsize_mb * 3 / 4;
> > size_mb += chipsize_mb;
> > +   debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
> > debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> >   rank, col, bk, cs0_row, bw, row_3_4);
> > }
> > 
> > +   debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
> > size_mb = min(size_mb, SDRAM_MAX_SIZE/SZ_1M);
> > 
> > return (size_t)size_mb << 20;
> > diff --git a/drivers/ram/rockchip/sdram_rk3288.c 
> > b/drivers/ram/rockchip/sdram_rk3288.c
> > index d99bf12476..9738eb088f 100644
> > --- a/drivers/ram/rockchip/sdram_rk3288.c
> > +++ b/drivers/ram/rockchip/sdram_rk3288.c
> > @@ -7,6 

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Dr. Philipp Tomsich]

Simon,

Looks like we’d like to have a 64bit type for size in ‘struct ram_info’ (see 
below).
Let us know what you think of the proposed change.

Thanks,
Philipp.


> On 8 May 2018, at 12:21, Dr. Philipp Tomsich 
>  wrote:
> 
> Marty,
> 
>> On 8 May 2018, at 02:52, Marty E. Plummer  wrote:
>> 
>> On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote:
>>> 
 On 7 May 2018, at 04:34, Marty E. Plummer  wrote:
 
 On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
> Hi Marty,
> 
> 
> On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
>> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
>> 
>> Without this change, my u-boot build for the asus c201 chromebook (4GiB)
>> is incorrectly detected as 0 Bytes of ram.
> 
> I know the root cause for this issue, and I have a local patch for it.
> The rk3288 is 32bit, and 4GB size is just out of range, so we need to 
> before
> the max size before return with '<<20'. Sorry for forgot to send it out.
> 
>> 
>> Signed-off-by: Marty E. Plummer 
>> ---
>> arch/arm/mach-rockchip/sdram_common.c | 62 ---
>> 1 file changed, 37 insertions(+), 25 deletions(-)
>> 
>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
>> b/arch/arm/mach-rockchip/sdram_common.c
>> index 76dbdc8715..a9c9f970a4 100644
>> --- a/arch/arm/mach-rockchip/sdram_common.c
>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>> @@ -10,6 +10,8 @@
>> #include 
>> #include 
>> #include 
>> +#include 
>> +#include 
>> 
>> DECLARE_GLOBAL_DATA_PTR;
>> size_t rockchip_sdram_size(phys_addr_t reg)
>> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>>  size_t size_mb = 0;
>>  u32 ch;
>> 
>> -u32 sys_reg = readl(reg);
>> -u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
>> -   & SYS_REG_NUM_CH_MASK);
>> +if (!size_mb) {
> 
> I don't understand this and follow up changes, we don't really need it,
> isn't it?
> I think don't need the changes before here.
 Yeah, that was just another level of indentation for the if (!size_mb)
 guard, but I've reworked the patch to not do that as it was pointed out
 that since size_mb is initialized to 0 prior.
>> +/*
>> + * we use the 0x~0xfeff space
>> + * since 0xff00~0x is soc register space
>> + * so we reserve it
>> + */
>> +size_mb = min(size_mb, 0xff00/SZ_1M);
> 
> This is what we really need, as Klaus point out, we need to use
> SDRAM_MAX_SIZE
> instead of hard code.
 Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
 build and boot tested on my hardware.
>>> 
>>> In that case you just masked the problem but didn???t solve it: assuming 
>>> size_mb
>>> is size_t (I???ll assume this is 64bit, but did not check), then your 4GB 
>>> is 0x1__ )
>>> which overflows to 0x0 when converted to a u32.
>>> 
>>> In other words: we need to figure out where the truncation occurs (image 
>>> what
>>> happens if a new 32bit processor with LPAE comes out???).
>>> 
>> A very valid point. With the following patch to sdram_common.c and
>> sdram_rk3288.c applied I get the debug output that follows it:
>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
>> b/arch/arm/mach-rockchip/sdram_common.c
>> index 232a7fa655..0fe69bf558 100644
>> --- a/arch/arm/mach-rockchip/sdram_common.c
>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>> @@ -4,6 +4,7 @@
>> * SPDX-License-Identifier: GPL-2.0+
>> */
>> 
>> +#define DEBUG 1
>> #include 
>> #include 
>> #include 
>> @@ -39,16 +40,19 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>>  SYS_REG_ROW_3_4_MASK;
>> 
>>  chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
>> +debug("%s: %d: chipsize_mb %x\n", __func__, __LINE__, 
>> chipsize_mb);
>> 
>>  if (rank > 1)
>>  chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
>>  if (row_3_4)
>>  chipsize_mb = chipsize_mb * 3 / 4;
>>  size_mb += chipsize_mb;
>> +debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>>  debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
>>rank, col, bk, cs0_row, bw, row_3_4);
>>  }
>> 
>> +debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>>  size_mb = min(size_mb, SDRAM_MAX_SIZE/SZ_1M);
>> 
>>  return (size_t)size_mb << 20;
>> diff --git a/drivers/ram/rockchip/sdram_rk3288.c 
>> b/drivers/ram/rockchip/sdram_rk3288.c
>> index d99bf12476..9738eb088f 100644
>> --- 

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Dr. Philipp Tomsich]

Marty,

> On 8 May 2018, at 02:52, Marty E. Plummer  wrote:
> 
> On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote:
>> 
>>> On 7 May 2018, at 04:34, Marty E. Plummer  wrote:
>>> 
>>> On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
 Hi Marty,
 
 
 On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> 
> Without this change, my u-boot build for the asus c201 chromebook (4GiB)
> is incorrectly detected as 0 Bytes of ram.
 
 I know the root cause for this issue, and I have a local patch for it.
 The rk3288 is 32bit, and 4GB size is just out of range, so we need to 
 before
 the max size before return with '<<20'. Sorry for forgot to send it out.
 
> 
> Signed-off-by: Marty E. Plummer 
> ---
> arch/arm/mach-rockchip/sdram_common.c | 62 ---
> 1 file changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> b/arch/arm/mach-rockchip/sdram_common.c
> index 76dbdc8715..a9c9f970a4 100644
> --- a/arch/arm/mach-rockchip/sdram_common.c
> +++ b/arch/arm/mach-rockchip/sdram_common.c
> @@ -10,6 +10,8 @@
> #include 
> #include 
> #include 
> +#include 
> +#include 
> 
> DECLARE_GLOBAL_DATA_PTR;
> size_t rockchip_sdram_size(phys_addr_t reg)
> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>   size_t size_mb = 0;
>   u32 ch;
> 
> - u32 sys_reg = readl(reg);
> - u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> -& SYS_REG_NUM_CH_MASK);
> + if (!size_mb) {
 
 I don't understand this and follow up changes, we don't really need it,
 isn't it?
 I think don't need the changes before here.
>>> Yeah, that was just another level of indentation for the if (!size_mb)
>>> guard, but I've reworked the patch to not do that as it was pointed out
>>> that since size_mb is initialized to 0 prior.
> + /*
> +  * we use the 0x~0xfeff space
> +  * since 0xff00~0x is soc register space
> +  * so we reserve it
> +  */
> + size_mb = min(size_mb, 0xff00/SZ_1M);
 
 This is what we really need, as Klaus point out, we need to use
 SDRAM_MAX_SIZE
 instead of hard code.
>>> Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
>>> build and boot tested on my hardware.
>> 
>> In that case you just masked the problem but didn???t solve it: assuming 
>> size_mb
>> is size_t (I???ll assume this is 64bit, but did not check), then your 4GB is 
>> 0x1__ )
>> which overflows to 0x0 when converted to a u32.
>> 
>> In other words: we need to figure out where the truncation occurs (image what
>> happens if a new 32bit processor with LPAE comes out???).
>> 
> A very valid point. With the following patch to sdram_common.c and
> sdram_rk3288.c applied I get the debug output that follows it:
> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> b/arch/arm/mach-rockchip/sdram_common.c
> index 232a7fa655..0fe69bf558 100644
> --- a/arch/arm/mach-rockchip/sdram_common.c
> +++ b/arch/arm/mach-rockchip/sdram_common.c
> @@ -4,6 +4,7 @@
>  * SPDX-License-Identifier: GPL-2.0+
>  */
> 
> +#define DEBUG 1
> #include 
> #include 
> #include 
> @@ -39,16 +40,19 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>   SYS_REG_ROW_3_4_MASK;
> 
>   chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> + debug("%s: %d: chipsize_mb %x\n", __func__, __LINE__, 
> chipsize_mb);
> 
>   if (rank > 1)
>   chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
>   if (row_3_4)
>   chipsize_mb = chipsize_mb * 3 / 4;
>   size_mb += chipsize_mb;
> + debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>   debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> rank, col, bk, cs0_row, bw, row_3_4);
>   }
> 
> + debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
>   size_mb = min(size_mb, SDRAM_MAX_SIZE/SZ_1M);
> 
>   return (size_t)size_mb << 20;
> diff --git a/drivers/ram/rockchip/sdram_rk3288.c 
> b/drivers/ram/rockchip/sdram_rk3288.c
> index d99bf12476..9738eb088f 100644
> --- a/drivers/ram/rockchip/sdram_rk3288.c
> +++ b/drivers/ram/rockchip/sdram_rk3288.c
> @@ -7,6 +7,7 @@
>  * Adapted from coreboot.
>  */
> 
> +#define DEBUG 1
> #include 
> #include 
> #include 
> 
> ---
> U-Boot SPL 2018.05-rc3-02370-g309384e84b-dirty (May 07 2018 - 19:42:15 -0500)
> Trying to boot from SPI
> 
> 
> U-Boot 2018.05-rc3-02370-g309384e84b-dirty (May 07 2018 - 19:42:15 -0500)
> 
> Model: Google Speedy
> DRAM:  rockchip_sdram_size ff73009c 

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer]

On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote:
> 
> > On 7 May 2018, at 04:34, Marty E. Plummer  wrote:
> > 
> > On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
> >> Hi Marty,
> >> 
> >> 
> >> On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
> >>> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> >>> 
> >>> Without this change, my u-boot build for the asus c201 chromebook (4GiB)
> >>> is incorrectly detected as 0 Bytes of ram.
> >> 
> >> I know the root cause for this issue, and I have a local patch for it.
> >> The rk3288 is 32bit, and 4GB size is just out of range, so we need to 
> >> before
> >> the max size before return with '<<20'. Sorry for forgot to send it out.
> >> 
> >>> 
> >>> Signed-off-by: Marty E. Plummer 
> >>> ---
> >>> arch/arm/mach-rockchip/sdram_common.c | 62 ---
> >>> 1 file changed, 37 insertions(+), 25 deletions(-)
> >>> 
> >>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> >>> b/arch/arm/mach-rockchip/sdram_common.c
> >>> index 76dbdc8715..a9c9f970a4 100644
> >>> --- a/arch/arm/mach-rockchip/sdram_common.c
> >>> +++ b/arch/arm/mach-rockchip/sdram_common.c
> >>> @@ -10,6 +10,8 @@
> >>> #include 
> >>> #include 
> >>> #include 
> >>> +#include 
> >>> +#include 
> >>> 
> >>> DECLARE_GLOBAL_DATA_PTR;
> >>> size_t rockchip_sdram_size(phys_addr_t reg)
> >>> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> >>>   size_t size_mb = 0;
> >>>   u32 ch;
> >>> 
> >>> - u32 sys_reg = readl(reg);
> >>> - u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> >>> -& SYS_REG_NUM_CH_MASK);
> >>> + if (!size_mb) {
> >> 
> >> I don't understand this and follow up changes, we don't really need it,
> >> isn't it?
> >> I think don't need the changes before here.
> > Yeah, that was just another level of indentation for the if (!size_mb)
> > guard, but I've reworked the patch to not do that as it was pointed out
> > that since size_mb is initialized to 0 prior.
> >>> + /*
> >>> +  * we use the 0x~0xfeff space
> >>> +  * since 0xff00~0x is soc register space
> >>> +  * so we reserve it
> >>> +  */
> >>> + size_mb = min(size_mb, 0xff00/SZ_1M);
> >> 
> >> This is what we really need, as Klaus point out, we need to use
> >> SDRAM_MAX_SIZE
> >> instead of hard code.
> > Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
> > build and boot tested on my hardware.
> 
> In that case you just masked the problem but didn???t solve it: assuming 
> size_mb
> is size_t (I???ll assume this is 64bit, but did not check), then your 4GB is 
> 0x1__ )
> which overflows to 0x0 when converted to a u32.
> 
> In other words: we need to figure out where the truncation occurs (image what
> happens if a new 32bit processor with LPAE comes out???).
> 
A very valid point. With the following patch to sdram_common.c and
sdram_rk3288.c applied I get the debug output that follows it:
diff --git a/arch/arm/mach-rockchip/sdram_common.c 
b/arch/arm/mach-rockchip/sdram_common.c
index 232a7fa655..0fe69bf558 100644
--- a/arch/arm/mach-rockchip/sdram_common.c
+++ b/arch/arm/mach-rockchip/sdram_common.c
@@ -4,6 +4,7 @@
  * SPDX-License-Identifier: GPL-2.0+
  */
 
+#define DEBUG 1
 #include 
 #include 
 #include 
@@ -39,16 +40,19 @@ size_t rockchip_sdram_size(phys_addr_t reg)
SYS_REG_ROW_3_4_MASK;
 
chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
+   debug("%s: %d: chipsize_mb %x\n", __func__, __LINE__, 
chipsize_mb);
 
if (rank > 1)
chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
if (row_3_4)
chipsize_mb = chipsize_mb * 3 / 4;
size_mb += chipsize_mb;
+   debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
  rank, col, bk, cs0_row, bw, row_3_4);
}
 
+   debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb);
size_mb = min(size_mb, SDRAM_MAX_SIZE/SZ_1M);
 
return (size_t)size_mb << 20;
diff --git a/drivers/ram/rockchip/sdram_rk3288.c 
b/drivers/ram/rockchip/sdram_rk3288.c
index d99bf12476..9738eb088f 100644
--- a/drivers/ram/rockchip/sdram_rk3288.c
+++ b/drivers/ram/rockchip/sdram_rk3288.c
@@ -7,6 +7,7 @@
  * Adapted from coreboot.
  */
 
+#define DEBUG 1
 #include 
 #include 
 #include 

---
U-Boot SPL 2018.05-rc3-02370-g309384e84b-dirty (May 07 2018 - 19:42:15 -0500)
Trying to boot from SPI


U-Boot 2018.05-rc3-02370-g309384e84b-dirty (May 07 2018 - 19:42:15 -0500)

Model: Google Speedy
DRAM:  rockchip_sdram_size ff73009c 3c50dc50
rockchip_sdram_size: 42: chipsize_mb 400
rockchip_sdram_size: 49: size_mb 800
rank 2 col 11 bk 3 cs0_row 14 bw 2 row_3_4 0
rockchip_sdram_size: 42: chipsize_mb 400

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Dr. Philipp Tomsich]

> On 7 May 2018, at 04:34, Marty E. Plummer  wrote:
> 
> On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
>> Hi Marty,
>> 
>> 
>> On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
>>> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
>>> 
>>> Without this change, my u-boot build for the asus c201 chromebook (4GiB)
>>> is incorrectly detected as 0 Bytes of ram.
>> 
>> I know the root cause for this issue, and I have a local patch for it.
>> The rk3288 is 32bit, and 4GB size is just out of range, so we need to before
>> the max size before return with '<<20'. Sorry for forgot to send it out.
>> 
>>> 
>>> Signed-off-by: Marty E. Plummer 
>>> ---
>>> arch/arm/mach-rockchip/sdram_common.c | 62 ---
>>> 1 file changed, 37 insertions(+), 25 deletions(-)
>>> 
>>> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
>>> b/arch/arm/mach-rockchip/sdram_common.c
>>> index 76dbdc8715..a9c9f970a4 100644
>>> --- a/arch/arm/mach-rockchip/sdram_common.c
>>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>>> @@ -10,6 +10,8 @@
>>> #include 
>>> #include 
>>> #include 
>>> +#include 
>>> +#include 
>>> 
>>> DECLARE_GLOBAL_DATA_PTR;
>>> size_t rockchip_sdram_size(phys_addr_t reg)
>>> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>>> size_t size_mb = 0;
>>> u32 ch;
>>> 
>>> -   u32 sys_reg = readl(reg);
>>> -   u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
>>> -  & SYS_REG_NUM_CH_MASK);
>>> +   if (!size_mb) {
>> 
>> I don't understand this and follow up changes, we don't really need it,
>> isn't it?
>> I think don't need the changes before here.
> Yeah, that was just another level of indentation for the if (!size_mb)
> guard, but I've reworked the patch to not do that as it was pointed out
> that since size_mb is initialized to 0 prior.
>>> +   /*
>>> +* we use the 0x~0xfeff space
>>> +* since 0xff00~0x is soc register space
>>> +* so we reserve it
>>> +*/
>>> +   size_mb = min(size_mb, 0xff00/SZ_1M);
>> 
>> This is what we really need, as Klaus point out, we need to use
>> SDRAM_MAX_SIZE
>> instead of hard code.
> Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
> build and boot tested on my hardware.

In that case you just masked the problem but didn’t solve it: assuming size_mb
is size_t (I’ll assume this is 64bit, but did not check), then your 4GB is 
0x1__ )
which overflows to 0x0 when converted to a u32.

In other words: we need to figure out where the truncation occurs (image what
happens if a new 32bit processor with LPAE comes out…).

>> 
>> Thanks,
>> - Kever
>>> }
>>> 
>>> return (size_t)size_mb << 20;
>> 
>> 
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de 
> https://lists.denx.de/listinfo/u-boot 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer]

On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
> Hi Marty,
> 
> 
> On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
> > Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> >
> > Without this change, my u-boot build for the asus c201 chromebook (4GiB)
> > is incorrectly detected as 0 Bytes of ram.
> 
> I know the root cause for this issue, and I have a local patch for it.
> The rk3288 is 32bit, and 4GB size is just out of range, so we need to before
> the max size before return with '<<20'. Sorry for forgot to send it out.
> 
> >
> > Signed-off-by: Marty E. Plummer 
> > ---
> >  arch/arm/mach-rockchip/sdram_common.c | 62 ---
> >  1 file changed, 37 insertions(+), 25 deletions(-)
> >
> > diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> > b/arch/arm/mach-rockchip/sdram_common.c
> > index 76dbdc8715..a9c9f970a4 100644
> > --- a/arch/arm/mach-rockchip/sdram_common.c
> > +++ b/arch/arm/mach-rockchip/sdram_common.c
> > @@ -10,6 +10,8 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >  
> >  DECLARE_GLOBAL_DATA_PTR;
> >  size_t rockchip_sdram_size(phys_addr_t reg)
> > @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> > size_t size_mb = 0;
> > u32 ch;
> >  
> > -   u32 sys_reg = readl(reg);
> > -   u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> > -  & SYS_REG_NUM_CH_MASK);
> > +   if (!size_mb) {
> 
> I don't understand this and follow up changes, we don't really need it,
> isn't it?
> I think don't need the changes before here.
Yeah, that was just another level of indentation for the if (!size_mb)
guard, but I've reworked the patch to not do that as it was pointed out
that since size_mb is initialized to 0 prior.
> > +   /*
> > +* we use the 0x~0xfeff space
> > +* since 0xff00~0x is soc register space
> > +* so we reserve it
> > +*/
> > +   size_mb = min(size_mb, 0xff00/SZ_1M);
> 
> This is what we really need, as Klaus point out, we need to use
> SDRAM_MAX_SIZE
> instead of hard code.
Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
build and boot tested on my hardware.
> 
> Thanks,
> - Kever
> > }
> >  
> > return (size_t)size_mb << 20;
> 
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Kever Yang]

Hi Marty,


On 05/06/2018 10:25 PM, Marty E. Plummer wrote:
> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
>
> Without this change, my u-boot build for the asus c201 chromebook (4GiB)
> is incorrectly detected as 0 Bytes of ram.

I know the root cause for this issue, and I have a local patch for it.
The rk3288 is 32bit, and 4GB size is just out of range, so we need to before
the max size before return with '<<20'. Sorry for forgot to send it out.

>
> Signed-off-by: Marty E. Plummer 
> ---
>  arch/arm/mach-rockchip/sdram_common.c | 62 ---
>  1 file changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> b/arch/arm/mach-rockchip/sdram_common.c
> index 76dbdc8715..a9c9f970a4 100644
> --- a/arch/arm/mach-rockchip/sdram_common.c
> +++ b/arch/arm/mach-rockchip/sdram_common.c
> @@ -10,6 +10,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  size_t rockchip_sdram_size(phys_addr_t reg)
> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>   size_t size_mb = 0;
>   u32 ch;
>  
> - u32 sys_reg = readl(reg);
> - u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> -& SYS_REG_NUM_CH_MASK);
> + if (!size_mb) {

I don't understand this and follow up changes, we don't really need it,
isn't it?
>  
> - debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> - for (ch = 0; ch < ch_num; ch++) {
> - rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> - SYS_REG_RANK_MASK);
> - col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & SYS_REG_COL_MASK);
> - bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
> - cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> - SYS_REG_CS0_ROW_MASK);
> - cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> - SYS_REG_CS1_ROW_MASK);
> - bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> - SYS_REG_BW_MASK));
> - row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> - SYS_REG_ROW_3_4_MASK;
> + u32 sys_reg = readl(reg);
> + u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> +& SYS_REG_NUM_CH_MASK);
>  
> - chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> + debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> + for (ch = 0; ch < ch_num; ch++) {
> + rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> + SYS_REG_RANK_MASK);
> + col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & 
> SYS_REG_COL_MASK);
> + bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & 
> SYS_REG_BK_MASK);
> + cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> + SYS_REG_CS0_ROW_MASK);
> + cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> + SYS_REG_CS1_ROW_MASK);
> + bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> + SYS_REG_BW_MASK));
> + row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> + SYS_REG_ROW_3_4_MASK;
>  
> - if (rank > 1)
> - chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
> - if (row_3_4)
> - chipsize_mb = chipsize_mb * 3 / 4;
> - size_mb += chipsize_mb;
> - debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> -   rank, col, bk, cs0_row, bw, row_3_4);
> + chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> +
> + if (rank > 1)
> + chipsize_mb += chipsize_mb >> (cs0_row - 
> cs1_row);
> + if (row_3_4)
> + chipsize_mb = chipsize_mb * 3 / 4;
> + size_mb += chipsize_mb;
> + debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 
> %d\n",
> +   rank, col, bk, cs0_row, bw, row_3_4);
> + }
> +

I think don't need the changes before here.
> + /*
> +  * we use the 0x~0xfeff space
> +  * since 0xff00~0x is soc register space
> +  * so we reserve it
> +  */
> + size_mb = min(size_mb, 0xff00/SZ_1M);

This is what we really need, as Klaus point out, we need to use
SDRAM_MAX_SIZE
instead of hard code.

Thanks,
- Kever
>   }
>  
>   return (size_t)size_mb << 20;


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer]

On Mon, May 07, 2018 at 12:19:11AM +0200, Dr. Philipp Tomsich wrote:
> 
> > On 6 May 2018, at 16:25, Marty E. Plummer  wrote:
> > 
> > Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> > 
> > Without this change, my u-boot build for the asus c201 chromebook (4GiB)
> > is incorrectly detected as 0 Bytes of ram.
> 
> Could you elaborate what the change is and what root-cause this addresses (4GB
> reporting as 0 sounds a bit like a 32bit type overflowing)?
> It's really hard to tell from the patch below (which seems to have everything 
> simply
> reformatted to a different indentation)...
> 
if (!size_mb) {} wrapping, plus the min code near the end. However,
actual testing on hardware shows this if guard to be unneeded, so I'll
be dropping it. I was just taking what was different from coreboot's
implementation (which I knew to work), but not all was needed it seems.
> > 
> > Signed-off-by: Marty E. Plummer 
> > ---
> > arch/arm/mach-rockchip/sdram_common.c | 62 ---
> > 1 file changed, 37 insertions(+), 25 deletions(-)
> > 
> > diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> > b/arch/arm/mach-rockchip/sdram_common.c
> > index 76dbdc8715..a9c9f970a4 100644
> > --- a/arch/arm/mach-rockchip/sdram_common.c
> > +++ b/arch/arm/mach-rockchip/sdram_common.c
> > @@ -10,6 +10,8 @@
> > #include 
> > #include 
> > #include 
> > +#include 
> > +#include 
> > 
> > DECLARE_GLOBAL_DATA_PTR;
> > size_t rockchip_sdram_size(phys_addr_t reg)
> > @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> > size_t size_mb = 0;
> > u32 ch;
> > 
> > -   u32 sys_reg = readl(reg);
> > -   u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> > -  & SYS_REG_NUM_CH_MASK);
> > +   if (!size_mb) {
> 
> Given that there's a "size_mb = 0" just above it, this will always evaluate
> to true... 
> 
Very true, next patch revision will do away with this if guard, as its
unneeded according to hardware retesting.
> > 
> > -   debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> > -   for (ch = 0; ch < ch_num; ch++) {
> > -   rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> > -   SYS_REG_RANK_MASK);
> > -   col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & SYS_REG_COL_MASK);
> > -   bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
> > -   cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> > -   SYS_REG_CS0_ROW_MASK);
> > -   cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> > -   SYS_REG_CS1_ROW_MASK);
> > -   bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> > -   SYS_REG_BW_MASK));
> > -   row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> > -   SYS_REG_ROW_3_4_MASK;
> > +   u32 sys_reg = readl(reg);
> > +   u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> > +  & SYS_REG_NUM_CH_MASK);
> > 
> > -   chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> > +   debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> > +   for (ch = 0; ch < ch_num; ch++) {
> > +   rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> > +   SYS_REG_RANK_MASK);
> > +   col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & 
> > SYS_REG_COL_MASK);
> > +   bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & 
> > SYS_REG_BK_MASK);
> > +   cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> > +   SYS_REG_CS0_ROW_MASK);
> > +   cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> > +   SYS_REG_CS1_ROW_MASK);
> > +   bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> > +   SYS_REG_BW_MASK));
> > +   row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> > +   SYS_REG_ROW_3_4_MASK;
> > 
> > -   if (rank > 1)
> > -   chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
> > -   if (row_3_4)
> > -   chipsize_mb = chipsize_mb * 3 / 4;
> > -   size_mb += chipsize_mb;
> > -   debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> > - rank, col, bk, cs0_row, bw, row_3_4);
> > +   chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> > +
> > +   if (rank > 1)
> > +   chipsize_mb += chipsize_mb >> (cs0_row - 
> > cs1_row);
> > +   if (row_3_4)
> > +   chipsize_mb = chipsize_mb * 3 / 4;
> > +   size_mb += chipsize_mb;
> > +   debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 
> > %d\n",
> > + rank, col, bk, cs0_row, bw, row_3_4);
> > +   }
> > +
> > +   /*

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Dr. Philipp Tomsich]

> On 6 May 2018, at 16:25, Marty E. Plummer  wrote:
> 
> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> 
> Without this change, my u-boot build for the asus c201 chromebook (4GiB)
> is incorrectly detected as 0 Bytes of ram.

Could you elaborate what the change is and what root-cause this addresses (4GB
reporting as 0 sounds a bit like a 32bit type overflowing)?
It’s really hard to tell from the patch below (which seems to have everything 
simply
reformatted to a different indentation)...

> 
> Signed-off-by: Marty E. Plummer 
> ---
> arch/arm/mach-rockchip/sdram_common.c | 62 ---
> 1 file changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> b/arch/arm/mach-rockchip/sdram_common.c
> index 76dbdc8715..a9c9f970a4 100644
> --- a/arch/arm/mach-rockchip/sdram_common.c
> +++ b/arch/arm/mach-rockchip/sdram_common.c
> @@ -10,6 +10,8 @@
> #include 
> #include 
> #include 
> +#include 
> +#include 
> 
> DECLARE_GLOBAL_DATA_PTR;
> size_t rockchip_sdram_size(phys_addr_t reg)
> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>   size_t size_mb = 0;
>   u32 ch;
> 
> - u32 sys_reg = readl(reg);
> - u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> -& SYS_REG_NUM_CH_MASK);
> + if (!size_mb) {

Given that there’s a “size_mb = 0” just above it, this will always evaluate
to true… 

> 
> - debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> - for (ch = 0; ch < ch_num; ch++) {
> - rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> - SYS_REG_RANK_MASK);
> - col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & SYS_REG_COL_MASK);
> - bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
> - cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> - SYS_REG_CS0_ROW_MASK);
> - cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> - SYS_REG_CS1_ROW_MASK);
> - bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> - SYS_REG_BW_MASK));
> - row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> - SYS_REG_ROW_3_4_MASK;
> + u32 sys_reg = readl(reg);
> + u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> +& SYS_REG_NUM_CH_MASK);
> 
> - chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> + debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> + for (ch = 0; ch < ch_num; ch++) {
> + rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> + SYS_REG_RANK_MASK);
> + col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & 
> SYS_REG_COL_MASK);
> + bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & 
> SYS_REG_BK_MASK);
> + cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> + SYS_REG_CS0_ROW_MASK);
> + cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> + SYS_REG_CS1_ROW_MASK);
> + bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> + SYS_REG_BW_MASK));
> + row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> + SYS_REG_ROW_3_4_MASK;
> 
> - if (rank > 1)
> - chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
> - if (row_3_4)
> - chipsize_mb = chipsize_mb * 3 / 4;
> - size_mb += chipsize_mb;
> - debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> -   rank, col, bk, cs0_row, bw, row_3_4);
> + chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> +
> + if (rank > 1)
> + chipsize_mb += chipsize_mb >> (cs0_row - 
> cs1_row);
> + if (row_3_4)
> + chipsize_mb = chipsize_mb * 3 / 4;
> + size_mb += chipsize_mb;
> + debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 
> %d\n",
> +   rank, col, bk, cs0_row, bw, row_3_4);
> + }
> +
> + /*
> +  * we use the 0x~0xfeff space
> +  * since 0xff00~0x is soc register space
> +  * so we reserve it
> +  */
> + size_mb = min(size_mb, 0xff00/SZ_1M);
>   }
> 
>   return (size_t)size_mb << 20;
> -- 
> 2.17.0
> 
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot

___
U-Boot mailing list
U-Boot@lists.denx.de

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer]

On Sun, May 06, 2018 at 10:21:55PM +0200, klaus.go...@theobroma-systems.com 
wrote:
> 
> > On 06.05.2018, at 22:03, Marty E. Plummer  wrote:
> > 
> > On Sun, May 06, 2018 at 02:08:25PM -0500, Marty E. Plummer wrote:
> >>> On Sun, May 06, 2018 at 08:39:23PM +0200, 
> >>> klaus.go...@theobroma-systems.com wrote:
>  On 06.05.2018, at 16:25, Marty E. Plummer  wrote:
>  + * we use the 0x~0xfeff space
>  + * since 0xff00~0x is soc register space
>  + * so we reserve it 
>  + */
> >>> 
> >>> That's not true for all Rockchip SoCs. On the RK3399 for example the 
> >>> upper limit 
> >>> is 0xf800. Even on the RK3288 DRAM address range is actually
> >>> 0x-0xfe0
> >>> 
>  +size_mb = min(size_mb, 0xff00/SZ_1M);
>   }
> >>> 
> >>> Should be 0xfe00 (4G-32MB) for RK3288.
> >>> But there is already a define for that, SDRAM_MAX_SIZE is defined in
> >>> rkxxx_common.h. Using that one would help to avoid possible breakage
> >>> of other Rockchip SoCs.
> >>> 
> >> Oh, does that get #defined properly for each SoC? if so, I'll sub that
> >> in for better compat.
> > In fact, could you or someone else help me to understand the #include
> > chain here? I'm not certain as to what that is available in each header
> > mentioned can be used in this file to get max compat across the rockchip
> > lineup, and I don't want to break someone elses board/etc with something
> > that 'works for me'.
> 
> You can get a full list of included headers by adding the -H file to the gcc 
> command line.
> make V=1 will help you to see how it's called on your system.
> 
> In that particular case the relevant include chain is:
> . include/common.h
> .. include/config.h
> ... include/configs/popmetal_rk3288.h
>  include/configs/rk3288_common.h
> 
> So the correct rk3xxx_common.h will be selected by config.h and SDRAM_MAX_SIZE
> is defined in all of them. 
> 
Ah, so 's:0xff00:SDRAM_MAX_SIZE:' in that spot and it should be
golden for all the rockchip stuffs. I'll try that, then try without the
if (!size_mb) guard as well. Should it still work after that, I'll
resubmit that patch. Probably with patman, if I can grok how to use it.

Aside from these issues, does the rest of the series look fine?
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[klaus . goger]

> On 06.05.2018, at 22:03, Marty E. Plummer  wrote:
> 
> On Sun, May 06, 2018 at 02:08:25PM -0500, Marty E. Plummer wrote:
>>> On Sun, May 06, 2018 at 08:39:23PM +0200, klaus.go...@theobroma-systems.com 
>>> wrote:
 On 06.05.2018, at 16:25, Marty E. Plummer  wrote:
 +   * we use the 0x~0xfeff space
 +   * since 0xff00~0x is soc register space
 +   * so we reserve it 
 +   */
>>> 
>>> That's not true for all Rockchip SoCs. On the RK3399 for example the upper 
>>> limit 
>>> is 0xf800. Even on the RK3288 DRAM address range is actually
>>> 0x-0xfe0
>>> 
 +  size_mb = min(size_mb, 0xff00/SZ_1M);
}
>>> 
>>> Should be 0xfe00 (4G-32MB) for RK3288.
>>> But there is already a define for that, SDRAM_MAX_SIZE is defined in
>>> rkxxx_common.h. Using that one would help to avoid possible breakage
>>> of other Rockchip SoCs.
>>> 
>> Oh, does that get #defined properly for each SoC? if so, I'll sub that
>> in for better compat.
> In fact, could you or someone else help me to understand the #include
> chain here? I'm not certain as to what that is available in each header
> mentioned can be used in this file to get max compat across the rockchip
> lineup, and I don't want to break someone elses board/etc with something
> that 'works for me’.

You can get a full list of included headers by adding the -H file to the gcc 
command line.
make V=1 will help you to see how it’s called on your system.

In that particular case the relevant include chain is:
. include/common.h
.. include/config.h
... include/configs/popmetal_rk3288.h
 include/configs/rk3288_common.h

So the correct rk3xxx_common.h will be selected by config.h and SDRAM_MAX_SIZE
is defined in all of them. 

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer]

On Sun, May 06, 2018 at 02:08:25PM -0500, Marty E. Plummer wrote:
> > On Sun, May 06, 2018 at 08:39:23PM +0200, klaus.go...@theobroma-systems.com 
> > wrote:
> > > On 06.05.2018, at 16:25, Marty E. Plummer  wrote:
> > > +  * we use the 0x~0xfeff space
> > > +  * since 0xff00~0x is soc register space
> > > +  * so we reserve it 
> > > +  */
> > 
> > That's not true for all Rockchip SoCs. On the RK3399 for example the upper 
> > limit 
> > is 0xf800. Even on the RK3288 DRAM address range is actually
> >  0x-0xfe0
> >
> > > + size_mb = min(size_mb, 0xff00/SZ_1M);
> > >   }
> > 
> > Should be 0xfe00 (4G-32MB) for RK3288.
> > But there is already a define for that, SDRAM_MAX_SIZE is defined in
> > rkxxx_common.h. Using that one would help to avoid possible breakage
> > of other Rockchip SoCs.
> > 
> Oh, does that get #defined properly for each SoC? if so, I'll sub that
> in for better compat.
In fact, could you or someone else help me to understand the #include
chain here? I'm not certain as to what that is available in each header
mentioned can be used in this file to get max compat across the rockchip
lineup, and I don't want to break someone elses board/etc with something
that 'works for me'.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer]

On Sun, May 06, 2018 at 08:39:23PM +0200, klaus.go...@theobroma-systems.com 
wrote:
> CC Philipp and Simon due maintainership (you may want to use 
> get_maintainer.pl in the future)
> and Kever as the original author of the file.
> 
> > On 06.05.2018, at 16:25, Marty E. Plummer  wrote:
> > 
> > Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> > 
> > Without this change, my u-boot build for the asus c201 chromebook (4GiB)
> > is incorrectly detected as 0 Bytes of ram.
> > 
> > Signed-off-by: Marty E. Plummer 
> > ---
> > arch/arm/mach-rockchip/sdram_common.c | 62 ---
> > 1 file changed, 37 insertions(+), 25 deletions(-)
> > 
> > diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> > b/arch/arm/mach-rockchip/sdram_common.c
> > index 76dbdc8715..a9c9f970a4 100644
> > --- a/arch/arm/mach-rockchip/sdram_common.c
> > +++ b/arch/arm/mach-rockchip/sdram_common.c
> > @@ -10,6 +10,8 @@
> > #include 
> > #include 
> > #include 
> > +#include 
> > +#include 
> 
> Is adding these headers really necessary?
> common.h already includes kernel.h and sizes.h (with some redirections of 
> config.h)
> 
If it it does get that in eventually, I suppose its not required.
I didn't know that chain of inclusion existed, so I grabbed what I
needed.
> > DECLARE_GLOBAL_DATA_PTR;
> > size_t rockchip_sdram_size(phys_addr_t reg)
> > @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> > size_t size_mb = 0;
> > u32 ch;
> > 
> > -   u32 sys_reg = readl(reg);
> > -   u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> > -  & SYS_REG_NUM_CH_MASK);
> > +   if (!size_mb) {
> 
> Is this really required? I don't see a way that size_mb will already be set 
> at this point.
> But it blows up your diff that it takes quite a while to see that your only 
> real change is 
> the size_mb = min(...) part at the end.
> 
It may be unneeded. I just knew that ram init worked on coreboot and not
on u-boot, and the only difference between the codepaths on both was the
if and min()
> > -   debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> > -   for (ch = 0; ch < ch_num; ch++) {
> > -   rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> > -   SYS_REG_RANK_MASK);
> > -   col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & SYS_REG_COL_MASK);
> > -   bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
> > -   cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> > -   SYS_REG_CS0_ROW_MASK);
> > -   cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> > -   SYS_REG_CS1_ROW_MASK);
> > -   bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> > -   SYS_REG_BW_MASK));
> > -   row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> > -   SYS_REG_ROW_3_4_MASK;
> > +   u32 sys_reg = readl(reg);
> > +   u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> > +  & SYS_REG_NUM_CH_MASK);
> > 
> > -   chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> > +   debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> > +   for (ch = 0; ch < ch_num; ch++) {
> > +   rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> > +   SYS_REG_RANK_MASK);
> > +   col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & 
> > SYS_REG_COL_MASK);
> > +   bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & 
> > SYS_REG_BK_MASK);
> > +   cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> > +   SYS_REG_CS0_ROW_MASK);
> > +   cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> > +   SYS_REG_CS1_ROW_MASK);
> > +   bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> > +   SYS_REG_BW_MASK));
> > +   row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> > +   SYS_REG_ROW_3_4_MASK;
> > 
> > -   if (rank > 1)
> > -   chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
> > -   if (row_3_4)
> > -   chipsize_mb = chipsize_mb * 3 / 4;
> > -   size_mb += chipsize_mb;
> > -   debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> > - rank, col, bk, cs0_row, bw, row_3_4);
> > +   chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> > +
> > +   if (rank > 1)
> > +   chipsize_mb += chipsize_mb >> (cs0_row - 
> > cs1_row);
> > +   if (row_3_4)
> > +   chipsize_mb = chipsize_mb * 3 / 4;
> > +   size_mb += chipsize_mb;
> > +   debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 
> > %d\n",
> > + rank, col, bk, cs0_row, 

Re: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[klaus . goger]

CC Philipp and Simon due maintainership (you may want to use get_maintainer.pl 
in the future)
and Kever as the original author of the file.

> On 06.05.2018, at 16:25, Marty E. Plummer  wrote:
> 
> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> 
> Without this change, my u-boot build for the asus c201 chromebook (4GiB)
> is incorrectly detected as 0 Bytes of ram.
> 
> Signed-off-by: Marty E. Plummer 
> ---
> arch/arm/mach-rockchip/sdram_common.c | 62 ---
> 1 file changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/sdram_common.c 
> b/arch/arm/mach-rockchip/sdram_common.c
> index 76dbdc8715..a9c9f970a4 100644
> --- a/arch/arm/mach-rockchip/sdram_common.c
> +++ b/arch/arm/mach-rockchip/sdram_common.c
> @@ -10,6 +10,8 @@
> #include 
> #include 
> #include 
> +#include 
> +#include 

Is adding these headers really necessary?
common.h already includes kernel.h and sizes.h (with some redirections of 
config.h)

> DECLARE_GLOBAL_DATA_PTR;
> size_t rockchip_sdram_size(phys_addr_t reg)
> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>   size_t size_mb = 0;
>   u32 ch;
> 
> - u32 sys_reg = readl(reg);
> - u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> -& SYS_REG_NUM_CH_MASK);
> + if (!size_mb) {

Is this really required? I don’t see a way that size_mb will already be set at 
this point.
But it blows up your diff that it takes quite a while to see that your only 
real change is 
the size_mb = min(...) part at the end.

> - debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> - for (ch = 0; ch < ch_num; ch++) {
> - rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> - SYS_REG_RANK_MASK);
> - col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & SYS_REG_COL_MASK);
> - bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
> - cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> - SYS_REG_CS0_ROW_MASK);
> - cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> - SYS_REG_CS1_ROW_MASK);
> - bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> - SYS_REG_BW_MASK));
> - row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> - SYS_REG_ROW_3_4_MASK;
> + u32 sys_reg = readl(reg);
> + u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> +& SYS_REG_NUM_CH_MASK);
> 
> - chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> + debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> + for (ch = 0; ch < ch_num; ch++) {
> + rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> + SYS_REG_RANK_MASK);
> + col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & 
> SYS_REG_COL_MASK);
> + bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & 
> SYS_REG_BK_MASK);
> + cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> + SYS_REG_CS0_ROW_MASK);
> + cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> + SYS_REG_CS1_ROW_MASK);
> + bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> + SYS_REG_BW_MASK));
> + row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> + SYS_REG_ROW_3_4_MASK;
> 
> - if (rank > 1)
> - chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
> - if (row_3_4)
> - chipsize_mb = chipsize_mb * 3 / 4;
> - size_mb += chipsize_mb;
> - debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> -   rank, col, bk, cs0_row, bw, row_3_4);
> + chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> +
> + if (rank > 1)
> + chipsize_mb += chipsize_mb >> (cs0_row - 
> cs1_row);
> + if (row_3_4)
> + chipsize_mb = chipsize_mb * 3 / 4;
> + size_mb += chipsize_mb;
> + debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 
> %d\n",
> +   rank, col, bk, cs0_row, bw, row_3_4);
> + }
> +
> + /*
> +  * we use the 0x~0xfeff space
> +  * since 0xff00~0x is soc register space
> +  * so we reserve it 
> +  */

That’s not true for all Rockchip SoCs. On the RK3399 for example the upper 
limit 
is 0xf800. Even on the RK3288 DRAM address range is actually
 0x-0xfe0

> + size_mb = min(size_mb, 0xff00/SZ_1M);
>   }