[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/ppc/ppc4xx: Only accept (combination of) pow2 DDR sizes
From: |
Markus Armbruster |
Subject: |
Re: [PATCH] hw/ppc/ppc4xx: Only accept (combination of) pow2 DDR sizes |
Date: |
Wed, 08 Jul 2020 09:16:18 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
BALATON Zoltan <balaton@eik.bme.hu> writes:
> On Mon, 29 Jun 2020, Philippe Mathieu-Daudé wrote:
>> Use popcount instruction to count the number of bits set in
>> the RAM size. Allow at most 1 bit for each bank. This avoid
>> using invalid hardware configurations.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/ppc/ppc4xx_devs.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
>> index f1651e04d9..c2484a5695 100644
>> --- a/hw/ppc/ppc4xx_devs.c
>> +++ b/hw/ppc/ppc4xx_devs.c
>> @@ -687,6 +687,15 @@ void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
>> int i;
>> int j;
>>
>> + if (ctpop64(size_left) > nr_banks) {
>> + if (nr_banks) {
>> + error_report("RAM size must be a power of 2");
>> + } else {
>> + error_report("RAM size must be the combination of %d powers of
>> 2",
>> + nr_banks);
>> + }
>> + exit(1);
>
> What is this supposed to fix?
The commit message doesn't really tell. It should.
I suspect this new check is redundant. What am I missing?
The loop that follows it finds a split of @ram's size guided by
@nr_banks and sdram_bank_sizes[]. The conditional after the loop fails
the function when no such split can be found.
In other words, the function fails unless @ram's size is the sum of
@nr_banks elements of sdram_bank_sizes[].
The actual sdram_bank_sizes[] contain only powers of two. Anything else
would be deeply weird.
ctpop64(size_left) > nr_banks is weaker than that, isn't it?
To prove me wrong, show me a scenario where the new check fails, and the
existing check doesn't.
> Is it a good idea to exit() from a
> helper?
Depends on what exactly is being helped.
> I don't think so because the board code should be in control
> in my opinion to decide what it can work with or what it cannot handle
> and wants to abort. So maybe it's better to return error in some way
> and let board code handle it. (We already exit from this function but
> that was added in commit a0258e4afa1 when the size fix up was removed
> due to memdev.
Complicate your helper when you genuinely need to. But YAGNI.
> That exit uses EXIT_FAILURE constant.)
I consider exit(EXIT_FAILURE) a case of portability virtue signalling ;)
But yes, local consistency should be maintained.
>> + }
>> for (i = 0; i < nr_banks; i++) {
>> for (j = 0; sdram_bank_sizes[j] != 0; j++) {
>> bank_size = sdram_bank_sizes[j];
>>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH] hw/ppc/ppc4xx: Only accept (combination of) pow2 DDR sizes,
Markus Armbruster <=