| Message ID | 20210925133407.1259392-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART | expand |
On Sat, Sep 25, 2021 at 9:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the > size occupied by all the registers. However all registers are > 32-bit wide, and the MemoryRegionOps handlers are restricted to > 32-bit: > > static const MemoryRegionOps mchp_pfsoc_mmuart_ops = { > .read = mchp_pfsoc_mmuart_read, > .write = mchp_pfsoc_mmuart_write, > .impl = { > .min_access_size = 4, > .max_access_size = 4, > }, > > Avoid being triskaidekaphobic, simplify by using the number of typo? See https://www.dictionary.com/browse/triskaidekaphobia Learned a new word today but I have to say this word is too hard for a non-native speaker :) > registers. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/hw/char/mchp_pfsoc_mmuart.h | 4 ++-- > hw/char/mchp_pfsoc_mmuart.c | 14 ++++++++------ > 2 files changed, 10 insertions(+), 8 deletions(-) > Reviewed-by: Bin Meng <bin.meng@windriver.com> Tested-by: Bin Meng <bin.meng@windriver.com>
On Sun, Sep 26, 2021 at 4:31 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Sat, Sep 25, 2021 at 9:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the > > size occupied by all the registers. However all registers are > > 32-bit wide, and the MemoryRegionOps handlers are restricted to > > 32-bit: > > > > static const MemoryRegionOps mchp_pfsoc_mmuart_ops = { > > .read = mchp_pfsoc_mmuart_read, > > .write = mchp_pfsoc_mmuart_write, > > .impl = { > > .min_access_size = 4, > > .max_access_size = 4, > > }, > > > > Avoid being triskaidekaphobic, simplify by using the number of > > typo? See https://www.dictionary.com/browse/triskaidekaphobia > > Learned a new word today but I have to say this word is too hard for a > non-native speaker :) > Never mind, triskaidekaphobia is a noun, and triskaidekaphobic is the adjective which is grammarly correct :) > > registers. > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > include/hw/char/mchp_pfsoc_mmuart.h | 4 ++-- > > hw/char/mchp_pfsoc_mmuart.c | 14 ++++++++------ > > 2 files changed, 10 insertions(+), 8 deletions(-) > > > > Reviewed-by: Bin Meng <bin.meng@windriver.com> > Tested-by: Bin Meng <bin.meng@windriver.com> Regards, Bin
On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the > size occupied by all the registers. However all registers are > 32-bit wide, and the MemoryRegionOps handlers are restricted to > 32-bit: > > static const MemoryRegionOps mchp_pfsoc_mmuart_ops = { > .read = mchp_pfsoc_mmuart_read, > .write = mchp_pfsoc_mmuart_write, > .impl = { > .min_access_size = 4, > .max_access_size = 4, > }, > > Avoid being triskaidekaphobic, simplify by using the number of > registers. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > include/hw/char/mchp_pfsoc_mmuart.h | 4 ++-- > hw/char/mchp_pfsoc_mmuart.c | 14 ++++++++------ > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h > index f61990215f0..9c012e6c977 100644 > --- a/include/hw/char/mchp_pfsoc_mmuart.h > +++ b/include/hw/char/mchp_pfsoc_mmuart.h > @@ -30,7 +30,7 @@ > > #include "hw/char/serial.h" > > -#define MCHP_PFSOC_MMUART_REG_SIZE 52 > +#define MCHP_PFSOC_MMUART_REG_COUNT 13 > > typedef struct MchpPfSoCMMUartState { > MemoryRegion iomem; > @@ -39,7 +39,7 @@ typedef struct MchpPfSoCMMUartState { > > SerialMM *serial; > > - uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)]; > + uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT]; > } MchpPfSoCMMUartState; > > /** > diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c > index 2facf85c2d8..584e7fec17c 100644 > --- a/hw/char/mchp_pfsoc_mmuart.c > +++ b/hw/char/mchp_pfsoc_mmuart.c > @@ -29,13 +29,14 @@ static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size) > { > MchpPfSoCMMUartState *s = opaque; > > - if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) { > + addr >>= 2; > + if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) { > qemu_log_mask(LOG_GUEST_ERROR, "%s: read: addr=0x%" HWADDR_PRIx "\n", > - __func__, addr); > + __func__, addr << 2); > return 0; > } > > - return s->reg[addr / sizeof(uint32_t)]; > + return s->reg[addr]; > } > > static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr, > @@ -44,13 +45,14 @@ static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr, > MchpPfSoCMMUartState *s = opaque; > uint32_t val32 = (uint32_t)value; > > - if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) { > + addr >>= 2; > + if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) { > qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx > - " v=0x%x\n", __func__, addr, val32); > + " v=0x%x\n", __func__, addr << 2, val32); > return; > } > > - s->reg[addr / sizeof(uint32_t)] = val32; > + s->reg[addr] = val32; > } > > static const MemoryRegionOps mchp_pfsoc_mmuart_ops = { > -- > 2.31.1 >
On Sun, Sep 26, 2021 at 6:31 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Sat, Sep 25, 2021 at 9:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the > > size occupied by all the registers. However all registers are > > 32-bit wide, and the MemoryRegionOps handlers are restricted to > > 32-bit: > > > > static const MemoryRegionOps mchp_pfsoc_mmuart_ops = { > > .read = mchp_pfsoc_mmuart_read, > > .write = mchp_pfsoc_mmuart_write, > > .impl = { > > .min_access_size = 4, > > .max_access_size = 4, > > }, > > > > Avoid being triskaidekaphobic, simplify by using the number of > > typo? See https://www.dictionary.com/browse/triskaidekaphobia > > Learned a new word today but I have to say this word is too hard for a > non-native speaker :) Ha! Even as a native English speaker I had to look it up :) Alistair
On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the > size occupied by all the registers. However all registers are > 32-bit wide, and the MemoryRegionOps handlers are restricted to > 32-bit: > > static const MemoryRegionOps mchp_pfsoc_mmuart_ops = { > .read = mchp_pfsoc_mmuart_read, > .write = mchp_pfsoc_mmuart_write, > .impl = { > .min_access_size = 4, > .max_access_size = 4, > }, > > Avoid being triskaidekaphobic, simplify by using the number of > registers. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Thanks! Applied to riscv-to-apply.next Alistair > --- > include/hw/char/mchp_pfsoc_mmuart.h | 4 ++-- > hw/char/mchp_pfsoc_mmuart.c | 14 ++++++++------ > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h > index f61990215f0..9c012e6c977 100644 > --- a/include/hw/char/mchp_pfsoc_mmuart.h > +++ b/include/hw/char/mchp_pfsoc_mmuart.h > @@ -30,7 +30,7 @@ > > #include "hw/char/serial.h" > > -#define MCHP_PFSOC_MMUART_REG_SIZE 52 > +#define MCHP_PFSOC_MMUART_REG_COUNT 13 > > typedef struct MchpPfSoCMMUartState { > MemoryRegion iomem; > @@ -39,7 +39,7 @@ typedef struct MchpPfSoCMMUartState { > > SerialMM *serial; > > - uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)]; > + uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT]; > } MchpPfSoCMMUartState; > > /** > diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c > index 2facf85c2d8..584e7fec17c 100644 > --- a/hw/char/mchp_pfsoc_mmuart.c > +++ b/hw/char/mchp_pfsoc_mmuart.c > @@ -29,13 +29,14 @@ static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size) > { > MchpPfSoCMMUartState *s = opaque; > > - if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) { > + addr >>= 2; > + if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) { > qemu_log_mask(LOG_GUEST_ERROR, "%s: read: addr=0x%" HWADDR_PRIx "\n", > - __func__, addr); > + __func__, addr << 2); > return 0; > } > > - return s->reg[addr / sizeof(uint32_t)]; > + return s->reg[addr]; > } > > static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr, > @@ -44,13 +45,14 @@ static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr, > MchpPfSoCMMUartState *s = opaque; > uint32_t val32 = (uint32_t)value; > > - if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) { > + addr >>= 2; > + if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) { > qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx > - " v=0x%x\n", __func__, addr, val32); > + " v=0x%x\n", __func__, addr << 2, val32); > return; > } > > - s->reg[addr / sizeof(uint32_t)] = val32; > + s->reg[addr] = val32; > } > > static const MemoryRegionOps mchp_pfsoc_mmuart_ops = { > -- > 2.31.1 >
diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h index f61990215f0..9c012e6c977 100644 --- a/include/hw/char/mchp_pfsoc_mmuart.h +++ b/include/hw/char/mchp_pfsoc_mmuart.h @@ -30,7 +30,7 @@ #include "hw/char/serial.h" -#define MCHP_PFSOC_MMUART_REG_SIZE 52 +#define MCHP_PFSOC_MMUART_REG_COUNT 13 typedef struct MchpPfSoCMMUartState { MemoryRegion iomem; @@ -39,7 +39,7 @@ typedef struct MchpPfSoCMMUartState { SerialMM *serial; - uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)]; + uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT]; } MchpPfSoCMMUartState; /** diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c index 2facf85c2d8..584e7fec17c 100644 --- a/hw/char/mchp_pfsoc_mmuart.c +++ b/hw/char/mchp_pfsoc_mmuart.c @@ -29,13 +29,14 @@ static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size) { MchpPfSoCMMUartState *s = opaque; - if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) { + addr >>= 2; + if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) { qemu_log_mask(LOG_GUEST_ERROR, "%s: read: addr=0x%" HWADDR_PRIx "\n", - __func__, addr); + __func__, addr << 2); return 0; } - return s->reg[addr / sizeof(uint32_t)]; + return s->reg[addr]; } static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr, @@ -44,13 +45,14 @@ static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr, MchpPfSoCMMUartState *s = opaque; uint32_t val32 = (uint32_t)value; - if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) { + addr >>= 2; + if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) { qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx - " v=0x%x\n", __func__, addr, val32); + " v=0x%x\n", __func__, addr << 2, val32); return; } - s->reg[addr / sizeof(uint32_t)] = val32; + s->reg[addr] = val32; } static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the size occupied by all the registers. However all registers are 32-bit wide, and the MemoryRegionOps handlers are restricted to 32-bit: static const MemoryRegionOps mchp_pfsoc_mmuart_ops = { .read = mchp_pfsoc_mmuart_read, .write = mchp_pfsoc_mmuart_write, .impl = { .min_access_size = 4, .max_access_size = 4, }, Avoid being triskaidekaphobic, simplify by using the number of registers. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/hw/char/mchp_pfsoc_mmuart.h | 4 ++-- hw/char/mchp_pfsoc_mmuart.c | 14 ++++++++------ 2 files changed, 10 insertions(+), 8 deletions(-)