Re: [PATCH 2/2] ppc/pnv: Implement ADU access to LPC space

["Nicholas Piggin" <npiggin@gmail.com>]

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 <npiggin@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