[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 3/7] hw/riscv/microchip_pfsoc.c: add an Icicle Kit fdt add
From: |
Alistair Francis |
Subject: |
Re: [PATCH v3 3/7] hw/riscv/microchip_pfsoc.c: add an Icicle Kit fdt address function |
Date: |
Mon, 23 Jan 2023 21:49:11 +1000 |
On Mon, Jan 23, 2023 at 8:19 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 1/22/23 19:53, Alistair Francis wrote:
> > On Sun, Jan 22, 2023 at 5:16 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> Conor,
> >>
> >> Thanks for the Icicle-kit walk-through! I'll not claim that I fully
> >> understood it,
> >> but I understood enough to handle the situation ATM.
> >>
> >> Without this change, this is where the FDT is being installed in the board
> >> when
> >> I start it with 8Gb of RAM (retrieved via 'info roms'):
> >>
> >> addr=00000000bfe00000 size=0x00a720 mem=ram name="fdt"
> >>
> >> Which surprised me at first because this is almost at the end of the LO
> >> area which has
> >> 1Gb and I figured it would be in the middle of another RAM area. I took
> >> another read
> >> at what we're doing in riscv_load_fdt():
> >>
> >> -----------
> >> temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> >> fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
> >> -----------
> >>
> >> This code can be read as "if the starting address of the RAM is lower than
> >> 3Gb, put
> >> the FDT no further than 3Gb (0xc0000000). Otherwise, put it at the end of
> >> dram",
> >> where "dram_base" is the starting address of the RAM block that the
> >> function
> >> receives.
> >>
> >> For icicle-kit, this is being passed as
> >> memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> >> 0x80000000, which is 2Gb.
> >>
> >> So, regardless of how much RAM we have (dram_end), the FDT will always be
> >> capped at
> >> 3Gb. At this moment, this fits exactly at the end of the LO area for the
> >> Icicle Kit.
> >> Which is funny because this 3Gb restriction was added by commit
> >> 1a475d39ef54 to fix
> >> 32 bit guest boot and it happened to also work for the Microchip SoC.
> >>
> >> So yeah, I thought that I was fixing a bug and in the end I caused one.
> >> This patch
> >> needs to go.
> >>
> >>
> >> Alistair, I believe I should re-send v2, this time explaining why the
> >> existing function
> >> will not break the Microchip board because we'll never put the FDT out of
> >> the LO area
> >> of the board. Does this work for you?
> >
> > I think that's fine. My only worry is that we are losing some
> > flexibility that some future board might want.
>
> What if we change riscv_load_fdt() parameters to pass a
> MemoryRegion/MemMapEntry
> instead of just dram_base?
>
> Instead of this:
>
> uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>
> We would have this:
>
> uint64_t riscv_load_fdt(MemMapEntry mem, uint64_t mem_size, void *fdt)
>
> Or even this:
>
> uint64_t riscv_load_fdt(hwaddr dram_base, hwaddr dram_size,
> uint64_t mem_size, void *fdt)
>
>
> And then we can make assumptions based on the actual memory region that the
> fdt
> is going to fit into, instead of having a starting address and a total memory
> size and have to deal with issues such as sparse memory.
>
> We can keep all the assumptions already made today (e.g. the 3Gb maximum addr)
> while also having a guarantee that the fdt isn't going to be put in the wrong
> memory region/spot if we decide to change the assumptions later on.
That seems like a good direction. We currently don't need this though,
so don't feel like it needs to be done today.
Alistair
>
>
> Thanks,
>
> Daniel
>
>
>
> >
> > Alistair
- [PATCH v3 4/7] hw/riscv: simplify riscv_compute_fdt_addr(), (continued)
[PATCH v3 6/7] hw/riscv/virt.c: rename MachineState 'mc' pointers to 'ms', Daniel Henrique Barboza, 2023/01/19
[PATCH v3 5/7] hw/riscv/virt.c: calculate socket count once in create_fdt_imsic(), Daniel Henrique Barboza, 2023/01/19
[PATCH v3 7/7] hw/riscv/spike.c: rename MachineState 'mc' pointers to' ms', Daniel Henrique Barboza, 2023/01/19