Re: [PATCH v3 1/8] ppc/pnv: Add pca9552 to powernv10 for PCIe hotplug power control

[Miles Glenn]

On Wed, 2023-11-15 at 08:28 +0100, Cédric Le Goater wrote:
> On 11/14/23 20:56, Glenn Miles wrote:
> > The Power Hypervisor code expects to see a pca9552 device connected
> > to the 3rd PNV I2C engine on port 1 at I2C address 0x63 (or left-
> > justified address of 0xC6).  This is used by hypervisor code to
> > control PCIe slot power during hotplug events.
> > 
> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > ---
> > Based-on: <20231024181144.4045056-3-milesg@linux.vnet.ibm.com>
> > [PATCH v3 2/2] misc/pca9552: Let external devices set pca9552
> > inputs
> > 
> > No changes from v2
> > 
> >   hw/ppc/Kconfig | 1 +
> >   hw/ppc/pnv.c   | 7 +++++++
> >   2 files changed, 8 insertions(+)
> > 
> > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> > index 56f0475a8e..f77ca773cf 100644
> > --- a/hw/ppc/Kconfig
> > +++ b/hw/ppc/Kconfig
> > @@ -32,6 +32,7 @@ config POWERNV
> >       select XIVE
> >       select FDT_PPC
> >       select PCI_POWERNV
> > +    select PCA9552
> >   
> >   config PPC405
> >       bool
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 9c29727337..7afaf1008f 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -1877,6 +1877,13 @@ static void
> > pnv_chip_power10_realize(DeviceState *dev, Error **errp)
> >                                 qdev_get_gpio_in(DEVICE(&chip10-
> > >psi),
> >                                                  PSIHB9_IRQ_SBE_I2C
> > ));
> >       }
> > +
> > +    /*
> > +     * Add a PCA9552 I2C device for PCIe hotplug control
> > +     * to engine 2, bus 1, address 0x63
> > +     */
> > +    i2c_slave_create_simple(chip10->i2c[2].busses[1], "pca9552",
> > 0x63);
> 
> You didn't answer my question in v2. Is this a P10 chip device or a
> board/machine device ?
> 
> Thanks,
> 
> C.
> 
> 

Sorry, you're right, I did miss that one, and after looking at the
Denali spec, I see that the topology is indeed different from Rainier
(which is what I have been modeling).  For the Denali, the PCA9552
has a different I2C address (0x62 instead of 0x63) and the GPIO
connections are also different.  Also, there is no PCA9554 chip because
it looks like they were able to cover all of the functionality with
just the  GPIO's of the PCA9552.  So, good catch!

I'll look at what they did on the Aspeed machines like you suggested.

Thanks,

Glenn

> 
> >   }
> >   
> >   static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip,
> > uint64_t addr)