| Message ID | 20221018210146.193159-7-shentey@gmail.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ppc/e500: Add support for two types of flash, cleanup | expand |
Hi Bernhard, On 18/10/22 23:01, Bernhard Beschow wrote: > Will allow e500 boards to access SD cards using just their own devices. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/sd/sdhci.c | 120 +++++++++++++++++++++++++++++++++++++++++- > include/hw/sd/sdhci.h | 3 ++ > 2 files changed, 122 insertions(+), 1 deletion(-) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index 306070c872..8d8ad9ff24 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s) > s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); > > s->io_ops = &sdhci_mmio_ops; > + s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE; > } > > void sdhci_uninitfn(SDHCIState *s) > @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) > s->fifo_buffer = g_malloc0(s->buf_maxsz); > > memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci", > - SDHC_REGISTERS_MAP_SIZE); > + s->io_registers_map_size); I don't think we want to change this region size. [see below] > void sdhci_common_unrealize(SDHCIState *s) > @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = { > .class_init = sdhci_bus_class_init, > }; > > +/* --- qdev Freescale eSDHC --- */ > + > +/* Watermark Level Register */ > +#define ESDHC_WML 0x44 > + > +/* Control Register for DMA transfer */ > +#define ESDHC_DMA_SYSCTL 0x40c > + > +#define ESDHC_REGISTERS_MAP_SIZE 0x410 My preferred approach would be to create a container region with a size of ESDHC_REGISTERS_MAP_SIZE. Map the SDHC_REGISTERS_MAP region in the container at offset 0, priority -1. Add 2 register regions for ESDHC_WML and ESDHC_DMA_SYSCTL, and map them with priority 1 in the container. ... > +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size) > +{ > + uint64_t ret; > + > + switch (offset) { > + case SDHC_SYSAD: > + case SDHC_BLKSIZE: > + case SDHC_ARGUMENT: > + case SDHC_TRNMOD: > + case SDHC_RSPREG0: > + case SDHC_RSPREG1: > + case SDHC_RSPREG2: > + case SDHC_RSPREG3: > + case SDHC_BDATA: > + case SDHC_PRNSTS: > + case SDHC_HOSTCTL: > + case SDHC_CLKCON: > + case SDHC_NORINTSTS: > + case SDHC_NORINTSTSEN: > + case SDHC_NORINTSIGEN: > + case SDHC_ACMD12ERRSTS: > + case SDHC_CAPAB: > + case SDHC_SLOT_INT_STATUS: > + ret = sdhci_read(opaque, offset, size); > + break; ... Then you don't need these cases. > + case ESDHC_WML: > + case ESDHC_DMA_SYSCTL: > + ret = 0; > + qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx > + " not implemented\n", offset); But then I realize you only treat these 2 registers as UNIMP. So now, I'd create 1 UNIMP region for ESDHC_WML and map it into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in hw/arm/bcm2835_peripherals.c. But the ESDHC_WML register has address 0x44 and fits inside the SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the upper part of the SDHC_CAPAB register. These bits are undefined on the spec v2, which I see you are setting in esdhci_init(). So this register should already return 0, otherwise we have a bug. Thus we don't need to handle this ESDHC_WML particularly. And your model is reduced to handling create_unimp() in esdhci_realize(). Am I missing something? Regards, Phil.
Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >Hi Bernhard, > >On 18/10/22 23:01, Bernhard Beschow wrote: >> Will allow e500 boards to access SD cards using just their own devices. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/sd/sdhci.c | 120 +++++++++++++++++++++++++++++++++++++++++- >> include/hw/sd/sdhci.h | 3 ++ >> 2 files changed, 122 insertions(+), 1 deletion(-) >> >> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c >> index 306070c872..8d8ad9ff24 100644 >> --- a/hw/sd/sdhci.c >> +++ b/hw/sd/sdhci.c >> @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s) >> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); >> s->io_ops = &sdhci_mmio_ops; >> + s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE; >> } >> void sdhci_uninitfn(SDHCIState *s) >> @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) >> s->fifo_buffer = g_malloc0(s->buf_maxsz); >> memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci", >> - SDHC_REGISTERS_MAP_SIZE); >> + s->io_registers_map_size); > >I don't think we want to change this region size. [see below] > >> void sdhci_common_unrealize(SDHCIState *s) >> @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = { >> .class_init = sdhci_bus_class_init, >> }; >> +/* --- qdev Freescale eSDHC --- */ >> + >> +/* Watermark Level Register */ >> +#define ESDHC_WML 0x44 >> + >> +/* Control Register for DMA transfer */ >> +#define ESDHC_DMA_SYSCTL 0x40c >> + >> +#define ESDHC_REGISTERS_MAP_SIZE 0x410 > >My preferred approach would be to create a container region with a >size of ESDHC_REGISTERS_MAP_SIZE. Map the SDHC_REGISTERS_MAP region >in the container at offset 0, priority -1. Add 2 register regions >for ESDHC_WML and ESDHC_DMA_SYSCTL, and map them with priority 1 in >the container. ... > >> +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size) >> +{ >> + uint64_t ret; >> + >> + switch (offset) { >> + case SDHC_SYSAD: >> + case SDHC_BLKSIZE: >> + case SDHC_ARGUMENT: >> + case SDHC_TRNMOD: >> + case SDHC_RSPREG0: >> + case SDHC_RSPREG1: >> + case SDHC_RSPREG2: >> + case SDHC_RSPREG3: >> + case SDHC_BDATA: >> + case SDHC_PRNSTS: >> + case SDHC_HOSTCTL: >> + case SDHC_CLKCON: >> + case SDHC_NORINTSTS: >> + case SDHC_NORINTSTSEN: >> + case SDHC_NORINTSIGEN: >> + case SDHC_ACMD12ERRSTS: >> + case SDHC_CAPAB: >> + case SDHC_SLOT_INT_STATUS: >> + ret = sdhci_read(opaque, offset, size); >> + break; > >... Then you don't need these cases. > >> + case ESDHC_WML: >> + case ESDHC_DMA_SYSCTL: >> + ret = 0; >> + qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx >> + " not implemented\n", offset); > >But then I realize you only treat these 2 registers as UNIMP. > >So now, I'd create 1 UNIMP region for ESDHC_WML and map it >into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add >another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset >0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in >hw/arm/bcm2835_peripherals.c. > >But the ESDHC_WML register has address 0x44 and fits inside the >SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the >upper part of the SDHC_CAPAB register. These bits are undefined >on the spec v2, which I see you are setting in esdhci_init(). >So this register should already return 0, otherwise we have >a bug. Thus we don't need to handle this ESDHC_WML particularly. > >And your model is reduced to handling create_unimp() in esdhci_realize(). > >Am I missing something? The mmio ops are big endian and need to be aligned to a 4-byte boundary. It took me quite a while to debug this. So shall I just create an additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL? Best regards, Bernhard > >Regards, > >Phil.
Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow <shentey@gmail.com>: >Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>Hi Bernhard, >> >>On 18/10/22 23:01, Bernhard Beschow wrote: >>> Will allow e500 boards to access SD cards using just their own devices. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/sd/sdhci.c | 120 +++++++++++++++++++++++++++++++++++++++++- >>> include/hw/sd/sdhci.h | 3 ++ >>> 2 files changed, 122 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c >>> index 306070c872..8d8ad9ff24 100644 >>> --- a/hw/sd/sdhci.c >>> +++ b/hw/sd/sdhci.c >>> @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s) >>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); >>> s->io_ops = &sdhci_mmio_ops; >>> + s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE; >>> } >>> void sdhci_uninitfn(SDHCIState *s) >>> @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) >>> s->fifo_buffer = g_malloc0(s->buf_maxsz); >>> memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci", >>> - SDHC_REGISTERS_MAP_SIZE); >>> + s->io_registers_map_size); >> >>I don't think we want to change this region size. [see below] >> >>> void sdhci_common_unrealize(SDHCIState *s) >>> @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = { >>> .class_init = sdhci_bus_class_init, >>> }; >>> +/* --- qdev Freescale eSDHC --- */ >>> + >>> +/* Watermark Level Register */ >>> +#define ESDHC_WML 0x44 >>> + >>> +/* Control Register for DMA transfer */ >>> +#define ESDHC_DMA_SYSCTL 0x40c >>> + >>> +#define ESDHC_REGISTERS_MAP_SIZE 0x410 >> >>My preferred approach would be to create a container region with a >>size of ESDHC_REGISTERS_MAP_SIZE. Map the SDHC_REGISTERS_MAP region >>in the container at offset 0, priority -1. Add 2 register regions >>for ESDHC_WML and ESDHC_DMA_SYSCTL, and map them with priority 1 in >>the container. ... >> >>> +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size) >>> +{ >>> + uint64_t ret; >>> + >>> + switch (offset) { >>> + case SDHC_SYSAD: >>> + case SDHC_BLKSIZE: >>> + case SDHC_ARGUMENT: >>> + case SDHC_TRNMOD: >>> + case SDHC_RSPREG0: >>> + case SDHC_RSPREG1: >>> + case SDHC_RSPREG2: >>> + case SDHC_RSPREG3: >>> + case SDHC_BDATA: >>> + case SDHC_PRNSTS: >>> + case SDHC_HOSTCTL: >>> + case SDHC_CLKCON: >>> + case SDHC_NORINTSTS: >>> + case SDHC_NORINTSTSEN: >>> + case SDHC_NORINTSIGEN: >>> + case SDHC_ACMD12ERRSTS: >>> + case SDHC_CAPAB: >>> + case SDHC_SLOT_INT_STATUS: >>> + ret = sdhci_read(opaque, offset, size); >>> + break; >> >>... Then you don't need these cases. >> >>> + case ESDHC_WML: >>> + case ESDHC_DMA_SYSCTL: >>> + ret = 0; >>> + qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx >>> + " not implemented\n", offset); >> >>But then I realize you only treat these 2 registers as UNIMP. >> >>So now, I'd create 1 UNIMP region for ESDHC_WML and map it >>into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add >>another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset >>0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in >>hw/arm/bcm2835_peripherals.c. >> >>But the ESDHC_WML register has address 0x44 and fits inside the >>SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the >>upper part of the SDHC_CAPAB register. These bits are undefined >>on the spec v2, which I see you are setting in esdhci_init(). >>So this register should already return 0, otherwise we have >>a bug. Thus we don't need to handle this ESDHC_WML particularly. My idea here was to catch this unimplemented case in order to indicate this clearly to users. Perhaps it nudges somebody to provide a patch? >> >>And your model is reduced to handling create_unimp() in esdhci_realize(). >> >>Am I missing something? > >The mmio ops are big endian and need to be aligned to a 4-byte boundary. It took me quite a while to debug this. So shall I just create an additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL? All in all I currently don't have a better idea than keeping the custom i/o ops for the standard region and adding an additional unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to dynamically allocate memory for it where I still need to figure out how not to leak it. Best regards, Bernhard > >Best regards, >Bernhard >> >>Regards, >> >>Phil. >
Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow <shentey@gmail.com>: >Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow <shentey@gmail.com>: >>Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>>Hi Bernhard, >>> >>>On 18/10/22 23:01, Bernhard Beschow wrote: >>>> Will allow e500 boards to access SD cards using just their own devices. >>>> >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> hw/sd/sdhci.c | 120 +++++++++++++++++++++++++++++++++++++++++- >>>> include/hw/sd/sdhci.h | 3 ++ >>>> 2 files changed, 122 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c >>>> index 306070c872..8d8ad9ff24 100644 >>>> --- a/hw/sd/sdhci.c >>>> +++ b/hw/sd/sdhci.c >>>> @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s) >>>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); >>>> s->io_ops = &sdhci_mmio_ops; >>>> + s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE; >>>> } >>>> void sdhci_uninitfn(SDHCIState *s) >>>> @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) >>>> s->fifo_buffer = g_malloc0(s->buf_maxsz); >>>> memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci", >>>> - SDHC_REGISTERS_MAP_SIZE); >>>> + s->io_registers_map_size); >>> >>>I don't think we want to change this region size. [see below] >>> >>>> void sdhci_common_unrealize(SDHCIState *s) >>>> @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = { >>>> .class_init = sdhci_bus_class_init, >>>> }; >>>> +/* --- qdev Freescale eSDHC --- */ >>>> + >>>> +/* Watermark Level Register */ >>>> +#define ESDHC_WML 0x44 >>>> + >>>> +/* Control Register for DMA transfer */ >>>> +#define ESDHC_DMA_SYSCTL 0x40c >>>> + >>>> +#define ESDHC_REGISTERS_MAP_SIZE 0x410 >>> >>>My preferred approach would be to create a container region with a >>>size of ESDHC_REGISTERS_MAP_SIZE. Map the SDHC_REGISTERS_MAP region >>>in the container at offset 0, priority -1. Add 2 register regions >>>for ESDHC_WML and ESDHC_DMA_SYSCTL, and map them with priority 1 in >>>the container. ... >>> >>>> +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size) >>>> +{ >>>> + uint64_t ret; >>>> + >>>> + switch (offset) { >>>> + case SDHC_SYSAD: >>>> + case SDHC_BLKSIZE: >>>> + case SDHC_ARGUMENT: >>>> + case SDHC_TRNMOD: >>>> + case SDHC_RSPREG0: >>>> + case SDHC_RSPREG1: >>>> + case SDHC_RSPREG2: >>>> + case SDHC_RSPREG3: >>>> + case SDHC_BDATA: >>>> + case SDHC_PRNSTS: >>>> + case SDHC_HOSTCTL: >>>> + case SDHC_CLKCON: >>>> + case SDHC_NORINTSTS: >>>> + case SDHC_NORINTSTSEN: >>>> + case SDHC_NORINTSIGEN: >>>> + case SDHC_ACMD12ERRSTS: >>>> + case SDHC_CAPAB: >>>> + case SDHC_SLOT_INT_STATUS: >>>> + ret = sdhci_read(opaque, offset, size); >>>> + break; >>> >>>... Then you don't need these cases. >>> >>>> + case ESDHC_WML: >>>> + case ESDHC_DMA_SYSCTL: >>>> + ret = 0; >>>> + qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx >>>> + " not implemented\n", offset); >>> >>>But then I realize you only treat these 2 registers as UNIMP. >>> >>>So now, I'd create 1 UNIMP region for ESDHC_WML and map it >>>into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add >>>another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset >>>0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in >>>hw/arm/bcm2835_peripherals.c. >>> >>>But the ESDHC_WML register has address 0x44 and fits inside the >>>SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the >>>upper part of the SDHC_CAPAB register. These bits are undefined >>>on the spec v2, which I see you are setting in esdhci_init(). >>>So this register should already return 0, otherwise we have >>>a bug. Thus we don't need to handle this ESDHC_WML particularly. > >My idea here was to catch this unimplemented case in order to indicate this clearly to users. Perhaps it nudges somebody to provide a patch? > >>> >>>And your model is reduced to handling create_unimp() in esdhci_realize(). >>> >>>Am I missing something? >> >>The mmio ops are big endian and need to be aligned to a 4-byte boundary. It took me quite a while to debug this. So shall I just create an additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL? > >All in all I currently don't have a better idea than keeping the custom i/o ops for the standard region and adding an additional unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to dynamically allocate memory for it where I still need to figure out how not to leak it. By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I was able to remove the custom implementations while having big endian and the alignments proper. However, I don't see a way of adding two memory regions - with or without a container. With a container I'd have to somehow preserve the mmio attribute which is initialized by the parent class, re-initialize it with the container, and add the preserved memory region as child. This seems very fragile, esp. since the parent class has created an alias for mmio in sysbus. Without a container, one would have two memory regions that both have to be mapped separately by the caller, i.e. it burdens the caller with an implementation detail. Any suggestions? Best regards, Bernhard > >Best regards, >Bernhard >> >>Best regards, >>Bernhard >>> >>>Regards, >>> >>>Phil. >> >
On 29/10/22 20:28, Bernhard Beschow wrote: > Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow <shentey@gmail.com>: >> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow <shentey@gmail.com>: >>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>>> Hi Bernhard, >>>> >>>> On 18/10/22 23:01, Bernhard Beschow wrote: >>>>> Will allow e500 boards to access SD cards using just their own devices. >>>>> >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>>> --- >>>>> hw/sd/sdhci.c | 120 +++++++++++++++++++++++++++++++++++++++++- >>>>> include/hw/sd/sdhci.h | 3 ++ >>>>> 2 files changed, 122 insertions(+), 1 deletion(-) >>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it >>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add >>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset >>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in >>>> hw/arm/bcm2835_peripherals.c. >>>> >>>> But the ESDHC_WML register has address 0x44 and fits inside the >>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the >>>> upper part of the SDHC_CAPAB register. These bits are undefined >>>> on the spec v2, which I see you are setting in esdhci_init(). >>>> So this register should already return 0, otherwise we have >>>> a bug. Thus we don't need to handle this ESDHC_WML particularly. >> >> My idea here was to catch this unimplemented case in order to indicate this clearly to users. Perhaps it nudges somebody to provide a patch? >> >>>> >>>> And your model is reduced to handling create_unimp() in esdhci_realize(). >>>> >>>> Am I missing something? >>> >>> The mmio ops are big endian and need to be aligned to a 4-byte boundary. It took me quite a while to debug this. So shall I just create an additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL? >> >> All in all I currently don't have a better idea than keeping the custom i/o ops for the standard region and adding an additional unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to dynamically allocate memory for it where I still need to figure out how not to leak it. > > By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I was able to remove the custom implementations while having big endian and the alignments proper. However, I don't see a way of adding two memory regions - with or without a container. With a container I'd have to somehow preserve the mmio attribute which is initialized by the parent class, re-initialize it with the container, and add the preserved memory region as child. This seems very fragile, esp. since the parent class has created an alias for mmio in sysbus. Without a container, one would have two memory regions that both have to be mapped separately by the caller, i.e. it burdens the caller with an implementation detail. > > Any suggestions? Can you share branch and how to test?
On Sun, Oct 30, 2022 at 1:10 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 29/10/22 20:28, Bernhard Beschow wrote: > > Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow < > shentey@gmail.com>: > >> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow < > shentey@gmail.com>: > >>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" < > philmd@linaro.org>: > >>>> Hi Bernhard, > >>>> > >>>> On 18/10/22 23:01, Bernhard Beschow wrote: > >>>>> Will allow e500 boards to access SD cards using just their own > devices. > >>>>> > >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> > >>>>> --- > >>>>> hw/sd/sdhci.c | 120 > +++++++++++++++++++++++++++++++++++++++++- > >>>>> include/hw/sd/sdhci.h | 3 ++ > >>>>> 2 files changed, 122 insertions(+), 1 deletion(-) > > >>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it > >>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add > >>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - > SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset > >>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in > >>>> hw/arm/bcm2835_peripherals.c. > >>>> > >>>> But the ESDHC_WML register has address 0x44 and fits inside the > >>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the > >>>> upper part of the SDHC_CAPAB register. These bits are undefined > >>>> on the spec v2, which I see you are setting in esdhci_init(). > >>>> So this register should already return 0, otherwise we have > >>>> a bug. Thus we don't need to handle this ESDHC_WML particularly. > >> > >> My idea here was to catch this unimplemented case in order to indicate > this clearly to users. Perhaps it nudges somebody to provide a patch? > >> > >>>> > >>>> And your model is reduced to handling create_unimp() in > esdhci_realize(). > >>>> > >>>> Am I missing something? > >>> > >>> The mmio ops are big endian and need to be aligned to a 4-byte > boundary. It took me quite a while to debug this. So shall I just create an > additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for > ESDHC_DMA_SYSCTL? > >> > >> All in all I currently don't have a better idea than keeping the custom > i/o ops for the standard region and adding an additional unimplemented > region for ESDHC_DMA_SYSCTL. I think I'd have to dynamically allocate > memory for it where I still need to figure out how not to leak it. > > > > By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I was able > to remove the custom implementations while having big endian and the > alignments proper. However, I don't see a way of adding two memory regions > - with or without a container. With a container I'd have to somehow > preserve the mmio attribute which is initialized by the parent class, > re-initialize it with the container, and add the preserved memory region as > child. This seems very fragile, esp. since the parent class has created an > alias for mmio in sysbus. Without a container, one would have two memory > regions that both have to be mapped separately by the caller, i.e. it > burdens the caller with an implementation detail. > > > > Any suggestions? > > Can you share branch and how to test? > QEMU branch: https://github.com/shentok/qemu/tree/e500-flash How to test: 1. `git clone -b e500 https://github.com/shentok/buildroot.git` 2. `cd buildroot` 3. `make qemu_ppc_e500mc_defconfig` 4. `make` 5. `cd output/images` 6. `dd if=/dev/zero of=root.img bs=1M count=64 && dd if=rootfs.ext2 of=root.img bs=1M conv=notrunc` 7. `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive id=mydrive,if=none,file=root.img,format=raw` Note that step 6 might be required every time before qemu-system-ppc is started, otherwise this may cause an fsck. The output on the boot console will look something along these lines (note that no errors are reported): Memory CAM mapping: 16/16/16/16/64/64/64 Mb, residual: 0Mb Activating Kernel Userspace Access Protection Activating Kernel Userspace Execution Prevention Linux version 5.17.7 (zcone-pisint@osoxes) (powerpc-buildroot-linux-gnu-gcc.br_real (Buildroot 2022.08-655-gf8a0d1480a) 11.3.0, GNU ld (GNU Binutils) 2.38) #1 SMP Sat Oct 29 12:49:54 CEST 2022 Using QEMU e500 machine description printk: bootconsole [udbg0] enabled CPU maps initialized for 1 thread per core ----------------------------------------------------- phys_mem_size = 0x10000000 dcache_bsize = 0x40 icache_bsize = 0x40 cpu_features = 0x0000000000000194 possible = 0x000000000001039c always = 0x0000000000000100 cpu_user_features = 0x8c008000 0x08000000 mmu_features = 0x000a0010 ----------------------------------------------------- qemu_e500_setup_arch() barrier-nospec: using isync; sync as speculation barrier Zone ranges: Normal [mem 0x0000000000000000-0x000000000fffffff] HighMem empty Movable zone start for each node Early memory node ranges node 0: [mem 0x0000000000000000-0x000000000fffffff] Initmem setup node 0 [mem 0x0000000000000000-0x000000000fffffff] MMU: Allocated 1088 bytes of context maps for 255 contexts percpu: Embedded 16 pages/cpu s33804 r8192 d23540 u65536 Built 1 zonelists, mobility grouping on. Total pages: 65024 Kernel command line: console=ttyS0 rootwait root=/dev/mmcblk0 nokaslr Unknown kernel command line parameters "nokaslr", will be passed to user space. Dentry cache hash table entries: 32768 (order: 5, 131072 bytes, linear) Inode-cache hash table entries: 16384 (order: 4, 65536 bytes, linear) mem auto-init: stack:off, heap alloc:off, heap free:off Kernel virtual memory layout: * 0xffa5f000..0xfffff000 : fixmap * 0xff800000..0xffa00000 : highmem PTEs * 0xd1000000..0xff800000 : vmalloc & ioremap Memory: 174608K/262144K available (12680K kernel code, 1080K rwdata, 3552K rodata, 396K init, 238K bss, 87536K reserved, 0K cma-reserved, 0K highmem) SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=1, Nodes=1 rcu: Hierarchical RCU implementation. rcu: RCU event tracing is enabled. rcu: RCU restricting CPUs from NR_CPUS=24 to nr_cpu_ids=1. rcu: RCU calculated value of scheduler-enlistment delay is 25 jiffies. rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=1 NR_IRQS: 512, nr_irqs: 512, preallocated irqs: 16 mpic: Setting up MPIC " OpenPIC " version 1.2 at fe0040000, max 1 CPUs mpic: ISU size: 256, shift: 8, mask: ff mpic: Initializing for 256 sources random: get_random_u32 called from start_kernel+0x524/0x6b0 with crng_init=0 clocksource: timebase: mask: 0xffffffffffffffff max_cycles: 0x5c4093a7d1, max_idle_ns: 440795210635 ns clocksource: timebase mult[2800000] shift[24] registered Console: colour dummy device 80x25 pid_max: default: 32768 minimum: 301 Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear) Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear) e500 family performance monitor hardware support registered rcu: Hierarchical SRCU implementation. smp: Bringing up secondary CPUs ... smp: Brought up 1 node, 1 CPU devtmpfs: initialized clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 7645041785100000 ns futex hash table entries: 256 (order: 2, 16384 bytes, linear) NET: Registered PF_NETLINK/PF_ROUTE protocol family audit: initializing netlink subsys (disabled) Found FSL PCI host bridge at 0x0000000fe0008000. Firmware bus number: 0->255 PCI host bridge /pci@fe0008000 (primary) ranges: MEM 0x0000000c00000000..0x0000000c1fffffff -> 0x00000000e0000000 IO 0x0000000fe1000000..0x0000000fe100ffff -> 0x0000000000000000 /pci@fe0008000: PCICSRBAR @ 0xdff00000 setup_pci_atmu: end of DRAM 10000000 Machine: QEMU ppce500 SoC family: QorIQ SoC ID: svr:0x00000000, Revision: 0.0 audit: type=2000 audit(0.092:1): state=initialized audit_enabled=0 res=1 fsl-pamu: fsl_pamu_init: could not find a PAMU node software IO TLB: tearing down default memory pool PCI: Probing PCI hardware fsl-pci fe0008000.pci: PCI host bridge to bus 8000:00 pci_bus 8000:00: root bus resource [io 0x0000-0xffff] pci_bus 8000:00: root bus resource [mem 0xc00000000-0xc1fffffff] (bus address [0xe0000000-0xffffffff]) pci_bus 8000:00: root bus resource [bus 00-ff] pci_bus 8000:00: busn_res: [bus 00-ff] end is updated to ff pci 8000:00:00.0: [1957:0030] type 00 class 0x0b2000 pci 8000:00:00.0: reg 0x10: [mem 0xdff00000-0xdfffffff] pci 8000:00:01.0: [1af4:1000] type 00 class 0x020000 pci 8000:00:01.0: reg 0x10: [io 0x0000-0x001f] pci 8000:00:01.0: reg 0x14: [mem 0x00000000-0x00000fff] pci 8000:00:01.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit pref] pci 8000:00:01.0: reg 0x30: [mem 0x00000000-0x0003ffff pref] pci_bus 8000:00: busn_res: [bus 00-ff] end is updated to 00 pci 8000:00:01.0: BAR 6: assigned [mem 0xc00000000-0xc0003ffff pref] pci 8000:00:01.0: BAR 4: assigned [mem 0xc00040000-0xc00043fff 64bit pref] pci 8000:00:01.0: BAR 1: assigned [mem 0xc00044000-0xc00044fff] pci 8000:00:01.0: BAR 0: assigned [io 0x1000-0x101f] pci_bus 8000:00: resource 4 [io 0x0000-0xffff] pci_bus 8000:00: resource 5 [mem 0xc00000000-0xc1fffffff] HugeTLB registered 4.00 MiB page size, pre-allocated 0 pages HugeTLB registered 16.0 MiB page size, pre-allocated 0 pages HugeTLB registered 64.0 MiB page size, pre-allocated 0 pages HugeTLB registered 256 MiB page size, pre-allocated 0 pages HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages Freescale Elo series DMA driver iommu: Default domain type: Translated iommu: DMA domain TLB invalidation policy: strict mode vgaarb: loaded SCSI subsystem initialized usbcore: registered new interface driver usbfs usbcore: registered new interface driver hub usbcore: registered new device driver usb pps_core: LinuxPPS API ver. 1 registered pps_core: Software ver. 5.3.6 - Copyright 2005-2007 Rodolfo Giometti < giometti@linux.it> PTP clock support registered Advanced Linux Sound Architecture Driver Initialized. clocksource: Switched to clocksource timebase NET: Registered PF_INET protocol family IP idents hash table entries: 4096 (order: 3, 32768 bytes, linear) tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 6144 bytes, linear) TCP established hash table entries: 2048 (order: 1, 8192 bytes, linear) TCP bind hash table entries: 2048 (order: 2, 16384 bytes, linear) TCP: Hash tables configured (established 2048 bind 2048) UDP hash table entries: 256 (order: 1, 8192 bytes, linear) UDP-Lite hash table entries: 256 (order: 1, 8192 bytes, linear) NET: Registered PF_UNIX/PF_LOCAL protocol family RPC: Registered named UNIX socket transport module. RPC: Registered udp transport module. RPC: Registered tcp transport module. RPC: Registered tcp NFSv4.1 backchannel transport module. PCI: CLS 0 bytes, default 64 workingset: timestamp_bits=30 max_order=16 bucket_order=0 squashfs: version 4.0 (2009/01/31) Phillip Lougher NFS: Registering the id_resolver key type Key type id_resolver registered Key type id_legacy registered Installing knfsd (copyright (C) 1996 okir@monad.swb.de). ntfs: driver 2.1.32 [Flags: R/O]. jffs2: version 2.2. (NAND) © 2001-2006 Red Hat, Inc. io scheduler mq-deadline registered io scheduler kyber registered virtio-pci 8000:00:01.0: enabling device (0000 -> 0003) Serial: 8250/16550 driver, 6 ports, IRQ sharing enabled printk: console [ttyS0] disabled serial8250.0: ttyS0 at MMIO 0xfe0004500 (irq = 42, base_baud = 25000000) is a 16550A printk: console [ttyS0] enabled printk: console [ttyS0] enabled printk: bootconsole [udbg0] disabled printk: bootconsole [udbg0] disabled ePAPR hypervisor byte channel driver brd: module loaded loop: module loaded st: Version 20160209, fixed bufsize 32768, s/g segs 256 ucc_geth_driver: QE UCC Gigabit Ethernet Controller e1000: Intel(R) PRO/1000 Network Driver e1000: Copyright (c) 1999-2006 Intel Corporation. e1000e: Intel(R) PRO/1000 Network Driver e1000e: Copyright(c) 1999 - 2015 Intel Corporation. igb: Intel(R) Gigabit Ethernet Network Driver igb: Copyright (c) 2007-2014 Intel Corporation. ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver ehci-pci: EHCI PCI platform driver ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver ohci-pci: OHCI PCI platform driver ehci-fsl: Freescale EHCI Host controller driver usbcore: registered new interface driver usb-storage i2c_dev: i2c /dev entries driver mpc-i2c fe0003000.i2c: timeout 1000000 us rtc-ds1307 0-0068: registered as rtc0 rtc-ds1307 0-0068: setting system clock to 2022-10-30T11:27:44 UTC (1667129264) sdhci: Secure Digital Host Controller Interface driver sdhci: Copyright(c) Pierre Ossman sdhci-pltfm: SDHCI platform and OF driver helper Freescale hypervisor management driver fsl-hv: no hypervisor found mmc0 bounce up to 128 segments into one, max segment size 65536 bytes ipip: IPv4 and MPLS over IPv4 tunneling driver Initializing XFRM netlink socket NET: Registered PF_INET6 protocol family Segment Routing with IPv6 In-situ OAM (IOAM) with IPv6 sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver NET: Registered PF_PACKET protocol family NET: Registered PF_KEY protocol family Key type dns_resolver registered drmem: No dynamic reconfiguration memory found mmc0: SDHCI controller on fe002e000.sdhc [fe002e000.sdhc] using DMA ALSA device list: No soundcards found. Waiting for root device /dev/mmcblk0... mmc0: new high speed SD card at address 4567 mmcblk0: mmc0:4567 QEMU! 64.0 MiB EXT4-fs (mmcblk0): mounted filesystem without journal. Quota mode: disabled. VFS: Mounted root (ext4 filesystem) readonly on device 179:0. devtmpfs: mounted Freeing unused kernel image (initmem) memory: 396K Run /sbin/init as init process EXT4-fs (mmcblk0): re-mounted. Quota mode: disabled. Starting syslogd: OK Starting klogd: OK Running sysctl: OK Saving random seed: random: dd: uninitialized urandom read (512 bytes read) OK Starting network: udhcpc: started, v1.35.0 udhcpc: broadcasting discover udhcpc: broadcasting select for 10.0.2.15, server 10.0.2.2 random: fast init done udhcpc: lease of 10.0.2.15 obtained from 10.0.2.2, lease time 86400 deleting routers adding dns 10.0.2.3 OK Welcome to Buildroot buildroot login: Best regards, Bernhard
On 30/10/22 12:46, Bernhard Beschow wrote: > > > On Sun, Oct 30, 2022 at 1:10 AM Philippe Mathieu-Daudé > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: > > On 29/10/22 20:28, Bernhard Beschow wrote: > > Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow > <shentey@gmail.com <mailto:shentey@gmail.com>>: > >> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow > <shentey@gmail.com <mailto:shentey@gmail.com>>: > >>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe > Mathieu-Daudé" <philmd@linaro.org <mailto:philmd@linaro.org>>: > >>>> Hi Bernhard, > >>>> > >>>> On 18/10/22 23:01, Bernhard Beschow wrote: > >>>>> Will allow e500 boards to access SD cards using just their > own devices. > >>>>> > >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com > <mailto:shentey@gmail.com>> > >>>>> --- > >>>>> hw/sd/sdhci.c | 120 > +++++++++++++++++++++++++++++++++++++++++- > >>>>> include/hw/sd/sdhci.h | 3 ++ > >>>>> 2 files changed, 122 insertions(+), 1 deletion(-) > > >>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it > >>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add > >>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - > SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset > >>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in > >>>> hw/arm/bcm2835_peripherals.c. > >>>> > >>>> But the ESDHC_WML register has address 0x44 and fits inside the > >>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the > >>>> upper part of the SDHC_CAPAB register. These bits are undefined > >>>> on the spec v2, which I see you are setting in esdhci_init(). > >>>> So this register should already return 0, otherwise we have > >>>> a bug. Thus we don't need to handle this ESDHC_WML particularly. > >> > >> My idea here was to catch this unimplemented case in order to > indicate this clearly to users. Perhaps it nudges somebody to > provide a patch? > >> > >>>> > >>>> And your model is reduced to handling create_unimp() in > esdhci_realize(). > >>>> > >>>> Am I missing something? > >>> > >>> The mmio ops are big endian and need to be aligned to a 4-byte > boundary. It took me quite a while to debug this. So shall I just > create an additional memory region for the region above > SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL? > >> > >> All in all I currently don't have a better idea than keeping the > custom i/o ops for the standard region and adding an additional > unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to > dynamically allocate memory for it where I still need to figure out > how not to leak it. > > > > By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I > was able to remove the custom implementations while having big > endian and the alignments proper. However, I don't see a way of > adding two memory regions - with or without a container. With a > container I'd have to somehow preserve the mmio attribute which is > initialized by the parent class, re-initialize it with the > container, and add the preserved memory region as child. This seems > very fragile, esp. since the parent class has created an alias for > mmio in sysbus. Without a container, one would have two memory > regions that both have to be mapped separately by the caller, i.e. > it burdens the caller with an implementation detail. > > > > Any suggestions? See https://lore.kernel.org/qemu-devel/20221031115402.91912-7-philmd@linaro.org/ > Can you share branch and how to test? > > > QEMU branch: https://github.com/shentok/qemu/tree/e500-flash > <https://github.com/shentok/qemu/tree/e500-flash> > > How to test: > 1. `git clone -b e500 https://github.com/shentok/buildroot.git > <https://github.com/shentok/buildroot.git>` > 2. `cd buildroot` > 3. `make qemu_ppc_e500mc_defconfig` > 4. `make` > 5. `cd output/images` > 6. `dd if=/dev/zero of=root.img bs=1M count=64 && dd if=rootfs.ext2 > of=root.img bs=1M conv=notrunc` > 7. `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append > "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive > -drive id=mydrive,if=none,file=root.img,format=raw` Could you add an Avocado-based test? > Welcome to Buildroot > buildroot login: Regards, Phil.
Am 31. Oktober 2022 12:11:37 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 30/10/22 12:46, Bernhard Beschow wrote: >> >> >> On Sun, Oct 30, 2022 at 1:10 AM Philippe Mathieu-Daudé <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: >> >> On 29/10/22 20:28, Bernhard Beschow wrote: >> > Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow >> <shentey@gmail.com <mailto:shentey@gmail.com>>: >> >> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow >> <shentey@gmail.com <mailto:shentey@gmail.com>>: >> >>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe >> Mathieu-Daudé" <philmd@linaro.org <mailto:philmd@linaro.org>>: >> >>>> Hi Bernhard, >> >>>> >> >>>> On 18/10/22 23:01, Bernhard Beschow wrote: >> >>>>> Will allow e500 boards to access SD cards using just their >> own devices. >> >>>>> >> >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com >> <mailto:shentey@gmail.com>> >> >>>>> --- >> >>>>> hw/sd/sdhci.c | 120 >> +++++++++++++++++++++++++++++++++++++++++- >> >>>>> include/hw/sd/sdhci.h | 3 ++ >> >>>>> 2 files changed, 122 insertions(+), 1 deletion(-) >> >> >>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it >> >>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add >> >>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - >> SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset >> >>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in >> >>>> hw/arm/bcm2835_peripherals.c. >> >>>> >> >>>> But the ESDHC_WML register has address 0x44 and fits inside the >> >>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the >> >>>> upper part of the SDHC_CAPAB register. These bits are undefined >> >>>> on the spec v2, which I see you are setting in esdhci_init(). >> >>>> So this register should already return 0, otherwise we have >> >>>> a bug. Thus we don't need to handle this ESDHC_WML particularly. >> >> >> >> My idea here was to catch this unimplemented case in order to >> indicate this clearly to users. Perhaps it nudges somebody to >> provide a patch? >> >> >> >>>> >> >>>> And your model is reduced to handling create_unimp() in >> esdhci_realize(). >> >>>> >> >>>> Am I missing something? >> >>> >> >>> The mmio ops are big endian and need to be aligned to a 4-byte >> boundary. It took me quite a while to debug this. So shall I just >> create an additional memory region for the region above >> SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL? >> >> >> >> All in all I currently don't have a better idea than keeping the >> custom i/o ops for the standard region and adding an additional >> unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to >> dynamically allocate memory for it where I still need to figure out >> how not to leak it. >> > >> > By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I >> was able to remove the custom implementations while having big >> endian and the alignments proper. However, I don't see a way of >> adding two memory regions - with or without a container. With a >> container I'd have to somehow preserve the mmio attribute which is >> initialized by the parent class, re-initialize it with the >> container, and add the preserved memory region as child. This seems >> very fragile, esp. since the parent class has created an alias for >> mmio in sysbus. Without a container, one would have two memory >> regions that both have to be mapped separately by the caller, i.e. >> it burdens the caller with an implementation detail. >> > >> > Any suggestions? > >See https://lore.kernel.org/qemu-devel/20221031115402.91912-7-philmd@linaro.org/ > >> Can you share branch and how to test? >> >> >> QEMU branch: https://github.com/shentok/qemu/tree/e500-flash <https://github.com/shentok/qemu/tree/e500-flash> >> >> How to test: >> 1. `git clone -b e500 https://github.com/shentok/buildroot.git <https://github.com/shentok/buildroot.git>` >> 2. `cd buildroot` >> 3. `make qemu_ppc_e500mc_defconfig` >> 4. `make` >> 5. `cd output/images` >> 6. `dd if=/dev/zero of=root.img bs=1M count=64 && dd if=rootfs.ext2 of=root.img bs=1M conv=notrunc` >> 7. `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive id=mydrive,if=none,file=root.img,format=raw` > >Could you add an Avocado-based test? I could give it a try at least. Where would I drop the binaries? Best regards, Bernhard > >> Welcome to Buildroot >> buildroot login: > >Regards, > >Phil.
Am 1. November 2022 10:49:20 UTC schrieb Bernhard Beschow <shentey@gmail.com>: >Am 31. Oktober 2022 12:11:37 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>On 30/10/22 12:46, Bernhard Beschow wrote: >>> >>> >>> On Sun, Oct 30, 2022 at 1:10 AM Philippe Mathieu-Daudé <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: >>> >>> On 29/10/22 20:28, Bernhard Beschow wrote: >>> > Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow >>> <shentey@gmail.com <mailto:shentey@gmail.com>>: >>> >> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow >>> <shentey@gmail.com <mailto:shentey@gmail.com>>: >>> >>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe >>> Mathieu-Daudé" <philmd@linaro.org <mailto:philmd@linaro.org>>: >>> >>>> Hi Bernhard, >>> >>>> >>> >>>> On 18/10/22 23:01, Bernhard Beschow wrote: >>> >>>>> Will allow e500 boards to access SD cards using just their >>> own devices. >>> >>>>> >>> >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com >>> <mailto:shentey@gmail.com>> >>> >>>>> --- >>> >>>>> hw/sd/sdhci.c | 120 >>> +++++++++++++++++++++++++++++++++++++++++- >>> >>>>> include/hw/sd/sdhci.h | 3 ++ >>> >>>>> 2 files changed, 122 insertions(+), 1 deletion(-) >>> >>> >>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it >>> >>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add >>> >>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - >>> SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset >>> >>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in >>> >>>> hw/arm/bcm2835_peripherals.c. >>> >>>> >>> >>>> But the ESDHC_WML register has address 0x44 and fits inside the >>> >>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the >>> >>>> upper part of the SDHC_CAPAB register. These bits are undefined >>> >>>> on the spec v2, which I see you are setting in esdhci_init(). >>> >>>> So this register should already return 0, otherwise we have >>> >>>> a bug. Thus we don't need to handle this ESDHC_WML particularly. >>> >> >>> >> My idea here was to catch this unimplemented case in order to >>> indicate this clearly to users. Perhaps it nudges somebody to >>> provide a patch? >>> >> >>> >>>> >>> >>>> And your model is reduced to handling create_unimp() in >>> esdhci_realize(). >>> >>>> >>> >>>> Am I missing something? >>> >>> >>> >>> The mmio ops are big endian and need to be aligned to a 4-byte >>> boundary. It took me quite a while to debug this. So shall I just >>> create an additional memory region for the region above >>> SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL? >>> >> >>> >> All in all I currently don't have a better idea than keeping the >>> custom i/o ops for the standard region and adding an additional >>> unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to >>> dynamically allocate memory for it where I still need to figure out >>> how not to leak it. >>> > >>> > By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I >>> was able to remove the custom implementations while having big >>> endian and the alignments proper. However, I don't see a way of >>> adding two memory regions - with or without a container. With a >>> container I'd have to somehow preserve the mmio attribute which is >>> initialized by the parent class, re-initialize it with the >>> container, and add the preserved memory region as child. This seems >>> very fragile, esp. since the parent class has created an alias for >>> mmio in sysbus. Without a container, one would have two memory >>> regions that both have to be mapped separately by the caller, i.e. >>> it burdens the caller with an implementation detail. >>> > >>> > Any suggestions? >> >>See https://lore.kernel.org/qemu-devel/20221031115402.91912-7-philmd@linaro.org/ >> >>> Can you share branch and how to test? >>> >>> >>> QEMU branch: https://github.com/shentok/qemu/tree/e500-flash <https://github.com/shentok/qemu/tree/e500-flash> >>> >>> How to test: >>> 1. `git clone -b e500 https://github.com/shentok/buildroot.git <https://github.com/shentok/buildroot.git>` >>> 2. `cd buildroot` >>> 3. `make qemu_ppc_e500mc_defconfig` >>> 4. `make` >>> 5. `cd output/images` >>> 6. `dd if=/dev/zero of=root.img bs=1M count=64 && dd if=rootfs.ext2 of=root.img bs=1M conv=notrunc` >>> 7. `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive id=mydrive,if=none,file=root.img,format=raw` >> >>Could you add an Avocado-based test? > >I could give it a try at least. Where would I drop the binaries? Looks like Cederic already had a repo [1]. Let's see what I can do. Best regards, Bernhard [1] https://github.com/legoater/qemu-ppc-boot/tree/main/buildroot > >Best regards, >Bernhard >> >>> Welcome to Buildroot >>> buildroot login: >> >>Regards, >> >>Phil. >
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 306070c872..8d8ad9ff24 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s) s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); s->io_ops = &sdhci_mmio_ops; + s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE; } void sdhci_uninitfn(SDHCIState *s) @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) s->fifo_buffer = g_malloc0(s->buf_maxsz); memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci", - SDHC_REGISTERS_MAP_SIZE); + s->io_registers_map_size); } void sdhci_common_unrealize(SDHCIState *s) @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = { .class_init = sdhci_bus_class_init, }; +/* --- qdev Freescale eSDHC --- */ + +/* Watermark Level Register */ +#define ESDHC_WML 0x44 + +/* Control Register for DMA transfer */ +#define ESDHC_DMA_SYSCTL 0x40c + +#define ESDHC_REGISTERS_MAP_SIZE 0x410 + +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size) +{ + uint64_t ret; + + switch (offset) { + case SDHC_SYSAD: + case SDHC_BLKSIZE: + case SDHC_ARGUMENT: + case SDHC_TRNMOD: + case SDHC_RSPREG0: + case SDHC_RSPREG1: + case SDHC_RSPREG2: + case SDHC_RSPREG3: + case SDHC_BDATA: + case SDHC_PRNSTS: + case SDHC_HOSTCTL: + case SDHC_CLKCON: + case SDHC_NORINTSTS: + case SDHC_NORINTSTSEN: + case SDHC_NORINTSIGEN: + case SDHC_ACMD12ERRSTS: + case SDHC_CAPAB: + case SDHC_SLOT_INT_STATUS: + ret = sdhci_read(opaque, offset, size); + break; + + case ESDHC_WML: + case ESDHC_DMA_SYSCTL: + ret = 0; + qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx + " not implemented\n", offset); + break; + + default: + ret = 0; + qemu_log_mask(LOG_GUEST_ERROR, "ESDHC rd @0x%02" HWADDR_PRIx + " unknown offset\n", offset); + break; + } + + return ret; +} + +static void esdhci_write(void *opaque, hwaddr offset, uint64_t val, + unsigned size) +{ + switch (offset) { + case SDHC_SYSAD: + case SDHC_BLKSIZE: + case SDHC_ARGUMENT: + case SDHC_TRNMOD: + case SDHC_BDATA: + case SDHC_HOSTCTL: + case SDHC_CLKCON: + case SDHC_NORINTSTS: + case SDHC_NORINTSTSEN: + case SDHC_NORINTSIGEN: + case SDHC_FEAER: + sdhci_write(opaque, offset, val, size); + break; + + case ESDHC_WML: + case ESDHC_DMA_SYSCTL: + qemu_log_mask(LOG_UNIMP, "ESDHC wr @0x%02" HWADDR_PRIx " <- 0x%08lx " + "not implemented\n", offset, val); + break; + + default: + qemu_log_mask(LOG_GUEST_ERROR, "ESDHC wr @0x%02" HWADDR_PRIx + " <- 0x%08lx unknown offset\n", offset, val); + break; + } +} + +static const MemoryRegionOps esdhc_mmio_ops = { + .read = esdhci_read, + .write = esdhci_write, + .valid = { + .min_access_size = 4, + .max_access_size = 4, + .unaligned = false + }, + .endianness = DEVICE_BIG_ENDIAN, +}; + +static void esdhci_init(Object *obj) +{ + DeviceState *dev = DEVICE(obj); + SDHCIState *s = SYSBUS_SDHCI(obj); + + s->io_ops = &esdhc_mmio_ops; + s->io_registers_map_size = ESDHC_REGISTERS_MAP_SIZE; + + /* + * Compatible with: + * - SD Host Controller Specification Version 2.0 Part A2 + */ + qdev_prop_set_uint8(dev, "sd-spec-version", 2); +} + +static const TypeInfo esdhc_info = { + .name = TYPE_FSL_ESDHC, + .parent = TYPE_SYSBUS_SDHCI, + .instance_init = esdhci_init, +}; + /* --- qdev i.MX eSDHC --- */ #define USDHC_MIX_CTRL 0x48 @@ -1907,6 +2024,7 @@ static void sdhci_register_types(void) { type_register_static(&sdhci_sysbus_info); type_register_static(&sdhci_bus_info); + type_register_static(&esdhc_info); type_register_static(&imx_usdhc_info); type_register_static(&sdhci_s3c_info); } diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h index 01a64c5442..5b32e83eee 100644 --- a/include/hw/sd/sdhci.h +++ b/include/hw/sd/sdhci.h @@ -45,6 +45,7 @@ struct SDHCIState { AddressSpace *dma_as; MemoryRegion *dma_mr; const MemoryRegionOps *io_ops; + uint64_t io_registers_map_size; QEMUTimer *insert_timer; /* timer for 'changing' sd card. */ QEMUTimer *transfer_timer; @@ -122,6 +123,8 @@ DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI, DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI, TYPE_SYSBUS_SDHCI) +#define TYPE_FSL_ESDHC "fsl-esdhc" + #define TYPE_IMX_USDHC "imx-usdhc" #define TYPE_S3C_SDHCI "s3c-sdhci"
Will allow e500 boards to access SD cards using just their own devices. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/sd/sdhci.c | 120 +++++++++++++++++++++++++++++++++++++++++- include/hw/sd/sdhci.h | 3 ++ 2 files changed, 122 insertions(+), 1 deletion(-)