On Wed Apr 17, 2024 at 10:25 PM AEST, Cédric Le Goater wrote:
> On 4/17/24 13:02, Nicholas Piggin wrote:
> > One of the functions of the ADU is indirect memory access engines that
> > send and receive data via ADU registers.
> >
> > This implements the ADU LPC memory access functionality sufficiently
> > for IBM proprietary firmware to access the UART and print characters
> > to the serial port as it does on real hardware.
> >
> > This requires a linkage between adu and lpc, which allows adu to
> > perform memory access in the lpc space.
> >
> > Signed-off-by: Nicholas Piggin <npig...@gmail.com>
> > ---
> > include/hw/ppc/pnv_adu.h | 7 ++++
> > include/hw/ppc/pnv_lpc.h | 5 +++
> > hw/ppc/pnv.c | 4 ++
> > hw/ppc/pnv_adu.c | 91 ++++++++++++++++++++++++++++++++++++++++
> > hw/ppc/pnv_lpc.c | 12 +++---
> > 5 files changed, 113 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/hw/ppc/pnv_adu.h b/include/hw/ppc/pnv_adu.h
> > index 9dc91857a9..b7b5d1bb21 100644
> > --- a/include/hw/ppc/pnv_adu.h
> > +++ b/include/hw/ppc/pnv_adu.h
> > @@ -10,6 +10,7 @@
> > #define PPC_PNV_ADU_H
> >
> > #include "hw/ppc/pnv.h"
> > +#include "hw/ppc/pnv_lpc.h"
> > #include "hw/qdev-core.h"
> >
> > #define TYPE_PNV_ADU "pnv-adu"
> > @@ -19,6 +20,12 @@ OBJECT_DECLARE_TYPE(PnvADU, PnvADUClass, PNV_ADU)
> > struct PnvADU {
> > DeviceState xd;
> >
> > + /* LPCMC (LPC Master Controller) access engine */
> > + PnvLpcController *lpc;
> > + uint64_t lpc_base_reg;
> > + uint64_t lpc_cmd_reg;
> > + uint64_t lpc_data_reg;
>
> I don't see reset values for these registers. Is that ok ?
>
> > MemoryRegion xscom_regs;
> > };
> >
> > diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> > index 5d22c45570..016e2998a8 100644
> > --- a/include/hw/ppc/pnv_lpc.h
> > +++ b/include/hw/ppc/pnv_lpc.h
> > @@ -94,6 +94,11 @@ struct PnvLpcClass {
> > DeviceRealize parent_realize;
> > };
> >
> > +bool pnv_opb_lpc_read(PnvLpcController *lpc, uint32_t addr,
> > + uint8_t *data, int sz);
> > +bool pnv_opb_lpc_write(PnvLpcController *lpc, uint32_t addr,
> > + uint8_t *data, int sz);
>
> May be rename to pnv_lpc_opb_read/write ?
Yes that's more appropriate.
> > ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error
> > **errp);
> > int pnv_dt_lpc(PnvChip *chip, void *fdt, int root_offset,
> > uint64_t lpcm_addr, uint64_t lpcm_size);
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 5869aac89a..eb9dbc62dd 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -1642,6 +1642,8 @@ static void pnv_chip_power9_realize(DeviceState *dev,
> > Error **errp)
> > }
> >
> > /* ADU */
> > + object_property_set_link(OBJECT(&chip9->adu), "lpc",
> > OBJECT(&chip9->lpc),
> > + &error_abort);
>
> I would add an assert on the lpc pointer in the ADU realize routine.
A assert != NULL, in case this failed to link correctly? (Maybe if it
was called before lpc object was realized). I will do.
Thanks,
Nick
