| Message ID | 20231114195659.1219821-2-milesg@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add powernv10 I2C devices and tests | expand |
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. > } > > static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr)
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)
On 11/15/23 17:37, Miles Glenn wrote: > 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. It should be a machine class extension with an i2c_setup handler and a new "powernv10-rainier" machine modeling the board layout. The rest looks good. Please include the pca9552 series in the respin. The pca9554 model will need a MAINTAINER (you?) I would be happy to let you take over pca9552 if you agree. First, let's get patch 3 and 4 in QEMU 8.2. Thanks, C. > > Thanks, > > Glenn > >> >>> } >>> >>> static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, >>> uint64_t addr) >
On Wed, 2023-11-15 at 23:34 +0100, Cédric Le Goater wrote: > On 11/15/23 17:37, Miles Glenn wrote: > > 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(&chip1 > > > > 0- > > > > > psi), > > > > PSIHB9_IRQ_SB > > > > E_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. > > It should be a machine class extension with an i2c_setup handler and > a new "powernv10-rainier" machine modeling the board layout. The rest > looks good. > > Please include the pca9552 series in the respin. The pca9554 model > will > need a MAINTAINER (you?) I would be happy to let you take over > pca9552 > if you agree. > > First, let's get patch 3 and 4 in QEMU 8.2. > > Thanks, > > C. > > Well, I was hoping to sweep the pca9554 model under the PowerNV maintainership (like pca9552 is under the BMC aspeed maintainership). I did update the PowerNV list to include it, but perhaps that was presumptuous of me. :-) I would be ok with being added as a reviewer under the PowerNV list, but I wonder if I shouldn't have more opensource experience before becoming a maintainer? TBH, I have no idea what that would entail. As for patches 3 and 4, it sounds like I should split those changes out from the current patch series so that they can make it into QEMU 8.2. Correct? Thanks, Glenn > > > > Thanks, > > > > Glenn > > > > > > } > > > > > > > > static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, > > > > uint64_t addr)
> Well, I was hoping to sweep the pca9554 model under the PowerNV > maintainership (like pca9552 is under the BMC aspeed maintainership). > I did update the PowerNV list to include it, but perhaps that was > presumptuous of me. :-) Well, you are the person who has the most knowledge on both and you have access to HW to check changes ! > I would be ok with being added as a reviewer under the PowerNV list, > but I wonder if I shouldn't have more opensource experience before > becoming a maintainer? TBH, I have no idea what that would entail. For these devices, mostly acking the changes. I don't think anyone will ask you to send PRs. This can be handled through some other tree, PPC or Aspeed. > As for patches 3 and 4, it sounds like I should split those changes out > from the current patch series so that they can make it into QEMU 8.2. > Correct? I don't think this is needed. They can be picked in the series and merged in the ppc tree independently. Thanks, C.
On Fri, 2023-11-17 at 17:04 +0100, Cédric Le Goater wrote: > > Well, I was hoping to sweep the pca9554 model under the PowerNV > > maintainership (like pca9552 is under the BMC aspeed > > maintainership). > > I did update the PowerNV list to include it, but perhaps that was > > presumptuous of me. :-) > > Well, you are the person who has the most knowledge on both and > you have access to HW to check changes ! > > > I would be ok with being added as a reviewer under the PowerNV > > list, > > but I wonder if I shouldn't have more opensource experience before > > becoming a maintainer? TBH, I have no idea what that would entail. > > For these devices, mostly acking the changes. I don't think anyone > will ask you to send PRs. This can be handled through some other > tree, PPC or Aspeed. > Ok, that doesn't sound too bad. Sign me up! > > > As for patches 3 and 4, it sounds like I should split those changes > > out > > from the current patch series so that they can make it into QEMU > > 8.2. > > Correct? > > I don't think this is needed. They can be picked in the series and > merged in the ppc tree independently. > > Thanks, > > C. > Ok, sounds good. Thanks, Glenn
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); + } static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr)
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(+)