[Skiboot] [PATCH v11 17/23] platforms/ibm-fsp: Support PCI slot

Alistair Popple alistair at popple.id.au
Tue May 24 13:58:58 AEST 2016


Hi Gavin,

I haven't looked at the code here in depth yet, but I have made a few comments 
below. It looks like there is still a reasonable amount of non-platform 
specific code under the FSP platform which personally I think would be much 
better in the core pci code. This way other platforms could just implement 
slot power on/off and get all the functionality required for hotplug.

- Alistair

On Fri, 20 May 2016 16:32:19 Gavin Shan wrote:
> The patch reworks PCI stuff for IBM's Apollo and Firenze platforms to
> support PCI slot:
> 
>   * Platform shared PCI slot is represented by "struct lxvpd_pci_slot"
>     for Apollo and Firenze. The information of that is fetched from
>     VPD.
>   * Apollo platform uses "struct lxvpd_pci_slot" as its platform slot,
>     while Firenze platform uses "struct firenze_pci_slot" as its
>     platform slot in order to support external I2C-based PCI slot power
>     maangement as well as PERST supported by the downstream ports of
>     particular PLX PCIe switches.
>   * On Firenze platform, the properties and methods to manage PHB slot
>     might be overrided to utilize the capability of external power
>     management.
> 
> Signed-off-by: Gavin Shan <gwshan at linux.vnet.ibm.com>
> ---
>  platforms/ibm-fsp/Makefile.inc  |   3 +-
>  platforms/ibm-fsp/apollo-pci.c  |  94 ++++
>  platforms/ibm-fsp/apollo.c      |  23 +-
>  platforms/ibm-fsp/firenze-pci.c | 957 
++++++++++++++++++++++++++++++++++++++++
>  platforms/ibm-fsp/firenze.c     | 345 +--------------
>  platforms/ibm-fsp/ibm-fsp.h     |  13 +
>  platforms/ibm-fsp/lxvpd.c       | 374 +++++++++-------
>  platforms/ibm-fsp/lxvpd.h       |  35 +-
>  8 files changed, 1321 insertions(+), 523 deletions(-)
>  create mode 100644 platforms/ibm-fsp/apollo-pci.c
>  create mode 100644 platforms/ibm-fsp/firenze-pci.c
> 
> diff --git a/platforms/ibm-fsp/Makefile.inc b/platforms/ibm-fsp/Makefile.inc
> index a885cbb..c5bec84 100644
> --- a/platforms/ibm-fsp/Makefile.inc
> +++ b/platforms/ibm-fsp/Makefile.inc
> @@ -1,6 +1,7 @@
>  SUBDIRS += $(PLATDIR)/ibm-fsp
>  
> -IBM_FSP_OBJS = common.o lxvpd.o apollo.o firenze.o
> +IBM_FSP_OBJS = common.o lxvpd.o apollo.o apollo-pci.o \
> +	       firenze.o firenze-pci.o
>  IBM_FSP = $(PLATDIR)/ibm-fsp/built-in.o
>  $(IBM_FSP): $(IBM_FSP_OBJS:%=$(PLATDIR)/ibm-fsp/%)
>  
> diff --git a/platforms/ibm-fsp/apollo-pci.c b/platforms/ibm-fsp/apollo-pci.c
> new file mode 100644
> index 0000000..f13b69d
> --- /dev/null
> +++ b/platforms/ibm-fsp/apollo-pci.c
> @@ -0,0 +1,94 @@
> +/* Copyright 2013-2015 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <skiboot.h>
> +#include <device.h>
> +#include <fsp.h>
> +#include <p7ioc.h>
> +#include <pci-cfg.h>
> +#include <pci.h>
> +#include <pci-slot.h>
> +
> +#include "ibm-fsp.h"
> +#include "lxvpd.h"
> +
> +/* Debugging option */
> +#define APOLLO_PCI_DBG(fmt, a...)	\
> +	prlog(PR_DEBUG, "APOLLO-PCI: " fmt, ##a)
> +#define APOLLO_PCI_INFO(fmt, a...)	\
> +	prlog(PR_INFO, "APOLLO-PCI: " fmt, ##a)
> +#define APOLLO_PCI_ERR(fmt, a...)	\
> +	prlog(PR_ERR, "APOLLO-PCI: " fmt, ##a)
> +
> +void apollo_pci_setup_phb(struct phb *phb, unsigned int index)
> +{
> +	struct dt_node *ioc_node;
> +	struct lxvpd_pci_slot *s = NULL;
> +	struct pci_slot *slot = NULL;
> +
> +	/* Grab the device-tree node of the IOC */
> +	ioc_node = phb->dt_node->parent;
> +	if (!ioc_node) {
> +		APOLLO_PCI_DBG("No IOC devnode for PHB%04x\n",
> +			       phb->opal_id);
> +		return;
> +	}
> +
> +	/*
> +	 * Process the pcie slot entries from the lx vpd lid
> +	 *
> +	 * FIXME: We currently assume chip 1 always, this will have to be
> +	 * fixed once we understand the right way to get the BRxy/BRxy "x"
> +	 * "x" value. It's not working well. I found 2 different root ports
> +	 * on Firebird-L has been assigned to same slot label.
> +	 */
> +	lxvpd_process_slot_entries(phb, ioc_node, 1,
> +				   index, sizeof(struct lxvpd_pci_slot));
> +
> +	/* Fixup P7IOC PHB slot */
> +	slot = phb->slot;
> +	s = slot ? lxvpd_get_slot(slot) : NULL;
> +	if (s)
> +		lxvpd_extract_info(slot, s);
> +}
> +
> +void apollo_pci_get_slot_info(struct phb *phb, struct pci_device *pd)
> +{
> +	struct pci_slot *slot;
> +	struct lxvpd_pci_slot *s = NULL;
> +
> +	if (pd->dev_type != PCIE_TYPE_ROOT_PORT     &&
> +	    pd->dev_type != PCIE_TYPE_SWITCH_UPPORT &&
> +	    pd->dev_type != PCIE_TYPE_SWITCH_DNPORT &&
> +	    pd->dev_type != PCIE_TYPE_PCIE_TO_PCIX)
> +		return;
> +
> +	/* Create PCIe slot */
> +	slot = pcie_slot_create(phb, pd);
> +	if (!slot)
> +		return;
> +
> +	/* Root complex inherits methods from PHB slot */
> +	if (!pd->parent && phb->slot)
> +		memcpy(&slot->ops, &phb->slot->ops, sizeof(struct 
pci_slot_ops));
> +
> +	/* Patch PCIe slot */
> +	s = lxvpd_get_slot(slot);
> +	if (s) {
> +		lxvpd_extract_info(slot, s);
> +		slot->ops.add_properties = lxvpd_add_slot_properties;
> +	}
> +}
> diff --git a/platforms/ibm-fsp/apollo.c b/platforms/ibm-fsp/apollo.c
> index e9616d5..d98699d 100644
> --- a/platforms/ibm-fsp/apollo.c
> +++ b/platforms/ibm-fsp/apollo.c
> @@ -19,6 +19,7 @@
>  #include <device.h>
>  #include <fsp.h>
>  #include <pci.h>
> +#include <pci-slot.h>
>  
>  #include "ibm-fsp.h"
>  #include "lxvpd.h"
> @@ -28,24 +29,6 @@ static bool apollo_probe(void)
>  	return dt_node_is_compatible(dt_root, "ibm,apollo");
>  }
>  
> -static void apollo_setup_phb(struct phb *phb, unsigned int index)
> -{
> -	struct dt_node *ioc_node;
> -
> -	/* Grab the device-tree node of the IOC */
> -	ioc_node = phb->dt_node->parent;
> -	if (!ioc_node)
> -		return;
> -
> -	/*
> -	 * Process the pcie slot entries from the lx vpd lid
> -	 *
> -	 * FIXME: We currently assume chip 1 always, this will have to be
> -	 * fixed once we understand the right way to get the BRxy/BRxy "x"
> -	 * "x" value. (this actually seems to work...)
> -	 */
> -	lxvpd_process_slot_entries(phb, ioc_node, 1, index);
> -}
>  
>  DECLARE_PLATFORM(apollo) = {
>  	.name			= "Apollo",
> @@ -54,8 +37,8 @@ DECLARE_PLATFORM(apollo) = {
>  	.exit			= ibm_fsp_exit,
>  	.cec_power_down		= ibm_fsp_cec_power_down,
>  	.cec_reboot		= ibm_fsp_cec_reboot,
> -	.pci_setup_phb		= apollo_setup_phb,
> -	.pci_get_slot_info	= lxvpd_get_slot_info,
> +	.pci_setup_phb		= apollo_pci_setup_phb,
> +	.pci_get_slot_info	= apollo_pci_get_slot_info,
>  	.nvram_info		= fsp_nvram_info,
>  	.nvram_start_read	= fsp_nvram_start_read,
>  	.nvram_write		= fsp_nvram_write,
> diff --git a/platforms/ibm-fsp/firenze-pci.c b/platforms/ibm-fsp/firenze-
pci.c
> new file mode 100644
> index 0000000..a72951a
> --- /dev/null
> +++ b/platforms/ibm-fsp/firenze-pci.c
> @@ -0,0 +1,957 @@
> +/* Copyright 2013-2015 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <skiboot.h>
> +#include <device.h>
> +#include <fsp.h>
> +#include <lock.h>
> +#include <timer.h>
> +#include <xscom.h>
> +#include <pci-cfg.h>
> +#include <pci.h>
> +#include <pci-slot.h>
> +#include <phb3.h>
> +#include <chip.h>
> +#include <i2c.h>
> +
> +#include "ibm-fsp.h"
> +#include "lxvpd.h"
> +
> +/* Debugging options */
> +#define FIRENZE_PCI_DBG(fmt, a...)	\
> +	prlog(PR_DEBUG, "FIRENZE-PCI: " fmt, ##a)
> +#define FIRENZE_PCI_INFO(fmt, a...)	\
> +	prlog(PR_INFO, "FIRENZE-PCI: " fmt, ##a)
> +#define FIRENZE_PCI_ERR(fmt, a...)	\
> +	prlog(PR_ERR, "FIRENZE-PCI: " fmt, ##a)
> +
> +/* Dump PCI slots before sending to FSP */
> +#define FIRENZE_PCI_INVENTORY_DUMP
> +
> +/*
> + * Firenze PCI slot states to override the default set.
> + * Refer to pci-slot.h for the default PCI state set
> + * when you're going to change below values.
> + */
> +#define FIRENZE_PCI_SLOT_NORMAL			0x00000000
> +#define FIRENZE_PCI_SLOT_HRESET			0x00000200
> +#define   FIRENZE_PCI_SLOT_HRESET_START		0x00000201
> +#define FIRENZE_PCI_SLOT_FRESET			0x00000300
> +#define FIRENZE_PCI_SLOT_FRESET_START		0x00000301
> +#define   FIRENZE_PCI_SLOT_FRESET_WAIT_RSP	0x00000302
> +#define   FIRENZE_PCI_SLOT_FRESET_DELAY		0x00000303
> +#define   FIRENZE_PCI_SLOT_FRESET_POWER_STATE	0x00000304
> +#define   FIRENZE_PCI_SLOT_FRESET_POWER_OFF	0x00000305
> +#define   FIRENZE_PCI_SLOT_FRESET_POWER_ON	0x00000306
> +#define   FIRENZE_PCI_SLOT_PERST_DEASSERT	0x00000307
> +#define   FIRENZE_PCI_SLOT_PERST_DELAY		0x00000308
> +#define FIRENZE_PCI_SLOT_PFRESET		0x00000400
> +#define   FIRENZE_PCI_SLOT_PFRESET_START	0x00000401
> +#define FIRENZE_PCI_SLOT_GPOWER			0x00000600
> +#define   FIRENZE_PCI_SLOT_GPOWER_START		0x00000601
> +#define FIRENZE_PCI_SLOT_SPOWER			0x00000700
> +#define   FIRENZE_PCI_SLOT_SPOWER_START		0x00000701
> +#define   FIRENZE_PCI_SLOT_SPOWER_DONE		0x00000702

Why are you copying the state values from pci-slot.h here as different names? 
I dislike the slot state machine as it is, I dislike it even more when the 
states are defined and changed across multiple areas of skiboot (hw/phb3.c, 
core/pci*.c, platforms/ibm-fsp/firence-pci.c).

> +/* Timeout for power status */
> +#define FIRENZE_PCI_SLOT_RETRIES	500
> +#define FIRENZE_PCI_SLOT_DELAY		10	/* ms */
> +#define FIRENZE_PCI_I2C_TIMEOUT		500	/* ms */
> +
> +/*
> + * Need figure out more stuff later: LED and presence
> + * detection sensors are accessed from PSI/FSP.
> + */
> +struct firenze_pci_slot {
> +	struct lxvpd_pci_slot	lxvpd_slot;	/* LXVPD slot data    */
> +
> +	/* Next slot state */
> +	uint32_t		next_state;
> +
> +	/* Power management */
> +	struct i2c_bus		*i2c_bus;	/* Where MAX5961 seats   */
> +	struct i2c_request	*req;		/* I2C request message   */
> +	uint8_t			i2c_rw_buf[8];	/* I2C read/write buffer */
> +	uint8_t			power_mask;	/* Bits for power status */
> +	uint8_t			power_on;	/* Bits for power on     */
> +	uint8_t			power_off;	/* Bits for power off    */
> +	uint8_t			*power_status;	/* Last power status     */
> +	uint16_t		perst_reg;	/* PERST config register */
> +	uint16_t		perst_bit;	/* PERST bit             */
> +};
> +
> +struct firenze_pci_slot_info {
> +	uint8_t		index;
> +	const char	*label;
> +	uint8_t		external_power_mgt;
> +	uint8_t		inband_perst;
> +	uint8_t		chip_id;
> +	uint8_t		master_id;
> +	uint8_t		port_id;
> +	uint8_t		slave_addr;
> +	uint8_t		channel;
> +	uint8_t		power_status;
> +	uint8_t		buddy;
> +};
> +
> +struct firenze_pci_inv {
> +	uint32_t	hw_proc_id;
> +	uint16_t	slot_idx;
> +	uint16_t	reserved;
> +	uint16_t	vendor_id;
> +	uint16_t	device_id;
> +	uint16_t	subsys_vendor_id;
> +	uint16_t	subsys_device_id;
> +};
> +
> +struct firenze_pci_inv_data {
> +	uint32_t                version;	/* currently 1 */
> +	uint32_t                num_entries;
> +	uint32_t                entry_size;
> +	uint32_t                entry_offset;
> +	struct firenze_pci_inv	entries[];
> +};
> +
> +/*
> + * Note: According to Tuleta system workbook, I didn't figure
> + * out the I2C mapping info for slot C14/C15.
> + */
> +static struct firenze_pci_inv_data *firenze_inv_data;
> +static uint32_t firenze_inv_cnt;
> +static struct firenze_pci_slot_info firenze_pci_slots[] = {
> +	{ 0x0B,  "C7", 1, 1,    0, 1, 0, 0x35, 1, 0xAA,  0 },
> +	{ 0x11, "C14", 0, 1,    0, 0, 0, 0x00, 0, 0xAA,  1 },
> +	{ 0x0F, "C11", 1, 1,    0, 1, 0, 0x32, 1, 0xAA,  2 },
> +	{ 0x10, "C12", 1, 1,    0, 1, 0, 0x39, 0, 0xAA,  3 },
> +	{ 0x0A,  "C6", 1, 1,    0, 1, 0, 0x35, 0, 0xAA,  0 },
> +	{ 0x12, "C15", 0, 1,    0, 0, 0, 0x00, 0, 0xAA,  5 },
> +	{ 0x01, "USB", 0, 0,    0, 0, 0, 0x00, 0, 0xAA,  6 },
> +	{ 0x0C,  "C8", 1, 1,    0, 1, 0, 0x36, 0, 0xAA,  7 },
> +	{ 0x0D,  "C9", 1, 1,    0, 1, 0, 0x36, 1, 0xAA,  7 },
> +	{ 0x0E, "C10", 1, 1,    0, 1, 0, 0x32, 0, 0xAA,  2 },
> +	{ 0x09,  "C5", 1, 1, 0x10, 1, 0, 0x39, 1, 0xAA, 10 },
> +	{ 0x08,  "C4", 1, 1, 0x10, 1, 0, 0x39, 0, 0xAA, 10 },
> +	{ 0x07,  "C3", 1, 1, 0x10, 1, 0, 0x3A, 1, 0xAA, 12 },
> +	{ 0x06,  "C2", 1, 1, 0x10, 1, 0, 0x3A, 0, 0xAA, 12 }
> +};
> +
> +static void firenze_pci_add_inventory(struct phb *phb,
> +				      struct pci_device *pd)
> +{
> +	struct lxvpd_pci_slot *lxvpd_slot;
> +	struct firenze_pci_inv *entry;
> +	struct proc_chip *chip;
> +	size_t size;
> +	bool need_init = false;
> +
> +	/*
> +	 * Do we need to add that to the FSP inventory for power
> +	 * management?
> +	 *
> +	 * For now, we only add devices that:
> +	 *
> +	 *  - Are function 0
> +	 *  - Are not an RC or a downstream bridge
> +	 *  - Have a direct parent that has a slot entry
> +	 *  - Slot entry says pluggable
> +	 *  - Aren't an upstream switch that has slot info
> +	 */
> +	if (!pd || !pd->parent)
> +		return;
> +	if (pd->bdfn & 7)
> +		return;
> +	if (pd->dev_type == PCIE_TYPE_ROOT_PORT ||
> +	    pd->dev_type == PCIE_TYPE_SWITCH_DNPORT)
> +		return;
> +	if (pd->dev_type == PCIE_TYPE_SWITCH_UPPORT &&
> +	    pd->slot && pd->slot->data)
> +		return;
> +	if (!pd->parent->slot ||
> +	    !pd->parent->slot->data)
> +		return;
> +	lxvpd_slot = pd->parent->slot->data;
> +	if (!lxvpd_slot->pluggable)
> +		return;
> +
> +	/* Check if we need to do some (Re)allocation */
> +	if (!firenze_inv_data ||
> +            firenze_inv_data->num_entries == firenze_inv_cnt) {
> +		need_init = !firenze_inv_data;
> +
> +		/* (Re)allocate the block to the new size */
> +		firenze_inv_cnt += 4;
> +		size = sizeof(struct firenze_pci_inv_data) +
> +		       sizeof(struct firenze_pci_inv) * firenze_inv_cnt;
> +                firenze_inv_data = realloc(firenze_inv_data, size);
> +	}
> +
> +	/* Initialize the header for a new inventory */
> +	if (need_init) {
> +		firenze_inv_data->version = 1;
> +		firenze_inv_data->num_entries = 0;
> +		firenze_inv_data->entry_size =
> +			sizeof(struct firenze_pci_inv);
> +		firenze_inv_data->entry_offset =
> +			offsetof(struct firenze_pci_inv_data, entries);
> +	}
> +
> +	/* Append slot entry */
> +	entry = &firenze_inv_data->entries[firenze_inv_data->num_entries++];
> +	chip = get_chip(dt_get_chip_id(phb->dt_node));
> +	if (!chip) {
> +		FIRENZE_PCI_ERR("No chip device node for PHB%04x\n",
> +				phb->opal_id);
> +                return;
> +	}
> +
> +	entry->hw_proc_id = chip->pcid;
> +	entry->reserved = 0;
> +	if (pd->parent &&
> +	    pd->parent->slot &&
> +	    pd->parent->slot->data) {
> +		lxvpd_slot = pd->parent->slot->data;
> +		entry->slot_idx = lxvpd_slot->slot_index;
> +	}
> +
> +	pci_cfg_read16(phb, pd->bdfn, PCI_CFG_VENDOR_ID, &entry->vendor_id);
> +	pci_cfg_read16(phb, pd->bdfn, PCI_CFG_DEVICE_ID, &entry->device_id);
> +        if (pd->is_bridge) {
> +                int64_t ssvc = pci_find_cap(phb, pd->bdfn,
> +					    PCI_CFG_CAP_ID_SUBSYS_VID);
> +		if (ssvc <= 0) {
> +			entry->subsys_vendor_id = 0xffff;
> +			entry->subsys_device_id = 0xffff;
> +		} else {
> +			pci_cfg_read16(phb, pd->bdfn,
> +				       ssvc + PCICAP_SUBSYS_VID_VENDOR,
> +				       &entry->subsys_vendor_id);
> +			pci_cfg_read16(phb, pd->bdfn,
> +				       ssvc + PCICAP_SUBSYS_VID_DEVICE,
> +				       &entry->subsys_device_id);
> +		}
> +        } else {
> +		pci_cfg_read16(phb, pd->bdfn, PCI_CFG_SUBSYS_VENDOR_ID,
> +			       &entry->subsys_vendor_id);
> +		pci_cfg_read16(phb, pd->bdfn, PCI_CFG_SUBSYS_ID,
> +			       &entry->subsys_device_id);
> +	}
> +}
> +
> +static void firenze_dump_pci_inventory(void)
> +{
> +#ifdef FIRENZE_PCI_INVENTORY_DUMP
> +	struct firenze_pci_inv *e;
> +	uint32_t i;
> +
> +	if (!firenze_inv_data)
> +		return;
> +
> +	FIRENZE_PCI_INFO("Dumping Firenze PCI inventory\n");
> +	FIRENZE_PCI_INFO("HWP SLT VDID DVID SVID SDID\n");
> +	FIRENZE_PCI_INFO("---------------------------\n");
> +	for (i = 0; i < firenze_inv_data->num_entries; i++) {
> +		e = &firenze_inv_data->entries[i];
> +
> +		FIRENZE_PCI_INFO("%03d %03d %04x %04x %04x %04x\n",
> +				 e->hw_proc_id, e->slot_idx,
> +				 e->vendor_id, e->device_id,
> +				 e->subsys_vendor_id, e->subsys_device_id);
> +	}
> +#endif /* FIRENZE_PCI_INVENTORY_DUMP */
> +}
> +
> +void firenze_pci_send_inventory(void)
> +{
> +	uint64_t base, abase, end, aend, offset;
> +	int64_t rc;
> +
> +	if (!firenze_inv_data)
> +		return;
> +
> +	/* Dump the inventory */
> +	FIRENZE_PCI_INFO("Sending %d inventory to FSP\n",
> +			 firenze_inv_data->num_entries);
> +	firenze_dump_pci_inventory();
> +
> +	/* Memory location for inventory */
> +        base = (uint64_t)firenze_inv_data;
> +        end = base +
> +	      sizeof(struct firenze_pci_inv_data) +
> +	      firenze_inv_data->num_entries * firenze_inv_data->entry_size;
> +	abase = base & ~0xffful;
> +	aend = (end + 0xffful) & ~0xffful;
> +	offset = PSI_DMA_PCIE_INVENTORY + (base & 0xfff);
> +
> +	/* We can only accomodate so many entries in the PSI map */
> +	if ((aend - abase) > PSI_DMA_PCIE_INVENTORY_SIZE) {
> +		FIRENZE_PCI_ERR("Inventory (%lld bytes) too large\n",
> +				aend - abase);
> +		goto bail;
> +	}
> +
> +	/* Map this in the TCEs */
> +	fsp_tce_map(PSI_DMA_PCIE_INVENTORY, (void *)abase, aend - abase);
> +
> +	/* Send FSP message */
> +	rc = fsp_sync_msg(fsp_mkmsg(FSP_CMD_PCI_POWER_CONF, 3,
> +				    hi32(offset), lo32(offset),
> +				    end - base), true);
> +	if (rc)
> +		FIRENZE_PCI_ERR("Error %lld sending inventory\n",
> +				rc);
> +
> +	/* Unmap */
> +	fsp_tce_unmap(PSI_DMA_PCIE_INVENTORY, aend - abase);
> +bail:
> +	/*
> +	 * We free the inventory. We'll have to redo that on hotplug
> +	 * when we support it but that isn't the case yet
> +	 */
> +	free(firenze_inv_data);
> +	firenze_inv_data = NULL;
> +	firenze_inv_cnt = 0;
> +}
> +
> +/* The function is called when the I2C request is completed
> + * successfully, or with errors.
> + */
> +static void firenze_i2c_req_done(int rc, struct i2c_request *req)
> +{
> +	struct pci_slot *slot = req->user_data;
> +	uint32_t state;
> +
> +	/* Check if there are errors for the completion */
> +	if (rc) {
> +		FIRENZE_PCI_ERR("Error %d from I2C request on slot %016llx\n",
> +				rc, slot->id);
> +		return;
> +	}
> +
> +	/* Check the request type */
> +	if (req->op != SMBUS_READ && req->op != SMBUS_WRITE) {
> +		FIRENZE_PCI_ERR("Invalid I2C request %d on slot %016llx\n",
> +				req->op, slot->id);
> +		return;
> +	}
> +
> +	/* After writting power status to I2C slave, we need at least
> +	 * 5ms delay for the slave to settle down. We also have the
> +	 * delay after reading the power status as well.
> +	 */
> +	switch (slot->state) {
> +	case FIRENZE_PCI_SLOT_FRESET_WAIT_RSP:
> +		FIRENZE_PCI_DBG("%016llx FRESET: I2C request completed\n",
> +				slot->id);
> +		state = FIRENZE_PCI_SLOT_FRESET_DELAY;
> +		break;
> +	case FIRENZE_PCI_SLOT_SPOWER_START:
> +		FIRENZE_PCI_DBG("%016llx SPOWER: I2C request completed\n",
> +				slot->id);
> +		state = FIRENZE_PCI_SLOT_SPOWER_DONE;
> +		break;
> +	default:
> +		FIRENZE_PCI_ERR("Wrong state %08x on slot %016llx\n",
> +				slot->state, slot->id);
> +		return;
> +	}
> +
> +	/* Switch to net state */
> +	pci_slot_set_state(slot, state);
> +}
> +
> +/* This function is called to setup normal PCI device or PHB slot.
> + * For the later case, the slot doesn't have the associated PCI
> + * device. Besides, the I2C response timeout is set to 5s. We might
> + * improve I2C in future to support priorized requests so that the
> + * timeout can be shortened.
> + */
> +static int64_t firenze_pci_slot_freset(struct pci_slot *slot)

Why do we have platform specific reset functions? Shouldn't these be generic 
functions in core code?

As far as I know the only platform part that is unique to a slot is the method 
of powering on/off the slot. The other functionality implemented here is 
either generic PCI stuff (eg. delays between power on/off) or specific to the 
PHB or switch the slot is connected to.

> +{
> +	struct firenze_pci_slot *plat_slot = slot->data;
> +	uint8_t *pval, presence = 1;
> +
> +	switch (slot->state) {
> +	case FIRENZE_PCI_SLOT_NORMAL:
> +	case FIRENZE_PCI_SLOT_FRESET_START:
> +		FIRENZE_PCI_DBG("%016llx FRESET: Starts\n",
> +				slot->id);
> +
> +		/* Bail if nothing is connected */
> +		if (slot->ops.get_presence_state)
> +			slot->ops.get_presence_state(slot, &presence);
> +		if (!presence) {
> +			FIRENZE_PCI_DBG("%016llx FRESET: No device\n",
> +					slot->id);
> +			return OPAL_SUCCESS;
> +		}
> +
> +		/* Prepare link down */
> +		if (slot->ops.prepare_link_change) {
> +			FIRENZE_PCI_DBG("%016llx FRESET: Prepares link 
down\n",
> +					slot->id);
> +			slot->ops.prepare_link_change(slot, false);
> +		}
> +
> +		/* Send I2C request */
> +		FIRENZE_PCI_DBG("%016llx FRESET: Check power state\n",
> +				slot->id);
> +		plat_slot->next_state =
> +			FIRENZE_PCI_SLOT_FRESET_POWER_STATE;
> +		plat_slot->req->op = SMBUS_READ;
> +		slot->retries = FIRENZE_PCI_SLOT_RETRIES;
> +		pci_slot_set_state(slot,
> +			FIRENZE_PCI_SLOT_FRESET_WAIT_RSP);
> +		if (pci_slot_has_flags(slot, PCI_SLOT_FLAG_BOOTUP))
> +			i2c_set_req_timeout(plat_slot->req,
> +					    FIRENZE_PCI_I2C_TIMEOUT);
> +		else
> +			i2c_set_req_timeout(plat_slot->req, 0ul);
> +		i2c_queue_req(plat_slot->req);
> +		return pci_slot_set_sm_timeout(slot,
> +				msecs_to_tb(FIRENZE_PCI_SLOT_DELAY));
> +	case FIRENZE_PCI_SLOT_FRESET_WAIT_RSP:
> +		if (slot->retries-- == 0) {
> +			FIRENZE_PCI_DBG("%016llx FRESET: Timeout waiting for 
%08x\n",
> +					slot->id, plat_slot->next_state);
> +			goto out;
> +		}
> +
> +		check_timers(false);
> +		return pci_slot_set_sm_timeout(slot,
> +				msecs_to_tb(FIRENZE_PCI_SLOT_DELAY));
> +	case FIRENZE_PCI_SLOT_FRESET_DELAY:
> +		FIRENZE_PCI_DBG("%016llx FRESET: Delay %dms on I2C 
completion\n",
> +				slot->id, FIRENZE_PCI_SLOT_DELAY);
> +		pci_slot_set_state(slot, plat_slot->next_state);
> +		return pci_slot_set_sm_timeout(slot,
> +				msecs_to_tb(FIRENZE_PCI_SLOT_DELAY));
> +	case FIRENZE_PCI_SLOT_FRESET_POWER_STATE:
> +		/* Update last power status */
> +		pval = (uint8_t *)(plat_slot->req->rw_buf);
> +		*plat_slot->power_status = *pval;
> +
> +		/* Power is on, turn it off */
> +		if (((*pval) & plat_slot->power_mask) == plat_slot->power_on) 
{
> +			FIRENZE_PCI_DBG("%016llx FRESET: Power (%02x) on, turn 
off\n",
> +					slot->id, *pval);
> +			(*pval) &= ~plat_slot->power_mask;
> +			(*pval) |= plat_slot->power_off;
> +			plat_slot->req->op = SMBUS_WRITE;
> +			slot->retries = FIRENZE_PCI_SLOT_RETRIES;
> +			plat_slot->next_state =
> +				FIRENZE_PCI_SLOT_FRESET_POWER_OFF;
> +			pci_slot_set_state(slot,
> +				FIRENZE_PCI_SLOT_FRESET_WAIT_RSP);
> +
> +			if (pci_slot_has_flags(slot, PCI_SLOT_FLAG_BOOTUP))
> +				i2c_set_req_timeout(plat_slot->req,
> +						    FIRENZE_PCI_I2C_TIMEOUT);
> +			else
> +				i2c_set_req_timeout(plat_slot->req, 0ul);
> +			i2c_queue_req(plat_slot->req);
> +			return pci_slot_set_sm_timeout(slot,
> +					msecs_to_tb(FIRENZE_PCI_SLOT_DELAY));
> +		}
> +
> +		/* Fall through: Power is off, turn it on */
> +	case FIRENZE_PCI_SLOT_FRESET_POWER_OFF:
> +		/* Update last power status */
> +		pval = (uint8_t *)(plat_slot->req->rw_buf);
> +		*plat_slot->power_status = *pval;
> +
> +		FIRENZE_PCI_DBG("%016llx FRESET: Power (%02x) off, turn on\n",
> +				slot->id, *pval);
> +		(*pval) &= ~plat_slot->power_mask;
> +		(*pval) |= plat_slot->power_on;
> +		plat_slot->req->op = SMBUS_WRITE;
> +		plat_slot->next_state =
> +			FIRENZE_PCI_SLOT_FRESET_POWER_ON;
> +		slot->retries = FIRENZE_PCI_SLOT_RETRIES;
> +		pci_slot_set_state(slot,
> +			FIRENZE_PCI_SLOT_FRESET_WAIT_RSP);
> +
> +		if (pci_slot_has_flags(slot, PCI_SLOT_FLAG_BOOTUP))
> +			i2c_set_req_timeout(plat_slot->req,
> +					    FIRENZE_PCI_I2C_TIMEOUT);
> +		else
> +			i2c_set_req_timeout(plat_slot->req, 0ul);
> +		i2c_queue_req(plat_slot->req);
> +		return pci_slot_set_sm_timeout(slot,
> +				msecs_to_tb(FIRENZE_PCI_SLOT_DELAY));
> +	case FIRENZE_PCI_SLOT_FRESET_POWER_ON:
> +		/* Update last power status */
> +		pval = (uint8_t *)(plat_slot->req->rw_buf);
> +		*plat_slot->power_status = *pval;
> +
> +		/* PHB3 slot supports post fundamental reset, we switch
> +		 * to that. For normal PCI slot, we switch to hot reset
> +		 * instead.
> +		 */
> +		if (slot->ops.pfreset) {
> +			FIRENZE_PCI_DBG("%016llx FRESET: Switch to PFRESET\n",
> +					slot->id);
> +			pci_slot_set_state(slot,
> +				FIRENZE_PCI_SLOT_PFRESET_START);
> +			return slot->ops.pfreset(slot);
> +		} else if (slot->ops.hreset) {
> +			FIRENZE_PCI_DBG("%016llx FRESET: Switch to HRESET\n",
> +					slot->id);
> +			pci_slot_set_state(slot,
> +				FIRENZE_PCI_SLOT_HRESET_START);
> +			return slot->ops.hreset(slot);
> +		}
> +
> +		pci_slot_set_state(slot, FIRENZE_PCI_SLOT_NORMAL);
> +		return OPAL_SUCCESS;
> +	default:
> +		FIRENZE_PCI_DBG("%016llx FRESET: Unexpected state %08x\n",
> +				slot->id, slot->state);
> +	}
> +
> +out:
> +	pci_slot_set_state(slot, FIRENZE_PCI_SLOT_NORMAL);
> +	return OPAL_HARDWARE;
> +}

For example it looks like the above code just:
1) Checks error conditions
2) Checks current power status
3) Powers off the slot
4) Wait/delay a bit
5) Power on the slot

Of these the only ones unique to the slot are 2, 3 & 5. So the rest should 
live in a core freset() function that calls the platform code to get/set power 
state. This would remove the state machine from platform specific code.

> +
> +static int64_t firenze_pci_slot_perst(struct pci_slot *slot)
> +{
> +	struct firenze_pci_slot *plat_slot = slot->data;
> +	uint8_t presence = 1;
> +	uint16_t ctrl;
> +
> +	switch (slot->state) {
> +	case FIRENZE_PCI_SLOT_NORMAL:
> +	case FIRENZE_PCI_SLOT_FRESET_START:
> +		FIRENZE_PCI_DBG("%016llx PERST: Starts\n",
> +				slot->id);
> +
> +		/* Bail if nothing is connected */
> +		if (slot->ops.get_presence_state)
> +			slot->ops.get_presence_state(slot, &presence);
> +		if (!presence) {
> +			FIRENZE_PCI_DBG("%016llx PERST: No device\n",
> +					slot->id);
> +			return OPAL_SUCCESS;
> +		}
> +
> +		/* Prepare link down */
> +		if (slot->ops.prepare_link_change) {
> +			FIRENZE_PCI_DBG("%016llx PERST: Prepare link down\n",
> +					slot->id);
> +			slot->ops.prepare_link_change(slot, false);
> +		}
> +
> +		/* Assert PERST */
> +		FIRENZE_PCI_DBG("%016llx PERST: Assert\n",
> +				slot->id);
> +		pci_cfg_read16(slot->phb, slot->pd->bdfn,
> +			       plat_slot->perst_reg, &ctrl);
> +		ctrl |= plat_slot->perst_bit;
> +		pci_cfg_write16(slot->phb, slot->pd->bdfn,
> +				plat_slot->perst_reg, ctrl);
> +		pci_slot_set_state(slot,
> +			FIRENZE_PCI_SLOT_PERST_DEASSERT);
> +		return pci_slot_set_sm_timeout(slot, msecs_to_tb(250));
> +	case FIRENZE_PCI_SLOT_PERST_DEASSERT:
> +		/* Deassert PERST */
> +		pci_cfg_read16(slot->phb, slot->pd->bdfn,
> +			       plat_slot->perst_reg, &ctrl);
> +		ctrl &= ~plat_slot->perst_bit;
> +		pci_cfg_write16(slot->phb, slot->pd->bdfn,
> +				plat_slot->perst_reg, ctrl);
> +		pci_slot_set_state(slot,
> +			FIRENZE_PCI_SLOT_PERST_DELAY);
> +		return pci_slot_set_sm_timeout(slot, msecs_to_tb(1500));
> +	case FIRENZE_PCI_SLOT_PERST_DELAY:
> +		/*
> +		 * Switch to post fundamental reset if the slot supports
> +		 * that. Otherwise, we issue a proceeding hot reset on
> +		 * the slot.
> +		 */
> +		if (slot->ops.pfreset) {
> +			FIRENZE_PCI_DBG("%016llx PERST: Switch to PFRESET\n",
> +					slot->id);
> +			pci_slot_set_state(slot,
> +				FIRENZE_PCI_SLOT_PFRESET_START);
> +			return slot->ops.pfreset(slot);
> +		} else if (slot->ops.hreset) {
> +			FIRENZE_PCI_DBG("%016llx PERST: Switch to HRESET\n",
> +					slot->id);
> +			pci_slot_set_state(slot,
> +				FIRENZE_PCI_SLOT_HRESET_START);
> +			return slot->ops.hreset(slot);
> +		}
> +
> +		pci_slot_set_state(slot, FIRENZE_PCI_SLOT_NORMAL);
> +		return OPAL_SUCCESS;
> +	default:
> +		FIRENZE_PCI_DBG("%016llx PERST: Unexpected state %08x\n",
> +				slot->id, slot->state);
> +	}
> +
> +	pci_slot_set_state(slot, FIRENZE_PCI_SLOT_NORMAL);
> +	return OPAL_HARDWARE;
> +}

Similar comments to above. In fact this seems even less specific to Firenze as 
you're touching PCI config space registers which presumably exist on other 
platform such as OpenPower?

> +static int64_t firenze_pci_slot_get_power_state(struct pci_slot *slot,
> +						uint8_t *val)
> +{
> +	if (slot->state != FIRENZE_PCI_SLOT_NORMAL)
> +		FIRENZE_PCI_ERR("%016llx GPOWER: Unexpected state %08x\n",
> +				slot->id, slot->state);
> +
> +	*val = slot->power_state;
> +	return OPAL_SUCCESS;
> +}
> +
> +static int64_t firenze_pci_slot_set_power_state(struct pci_slot *slot,
> +						uint8_t val)
> +{
> +	struct firenze_pci_slot *plat_slot = slot->data;
> +	uint8_t *pval;
> +
> +	if (slot->state != FIRENZE_PCI_SLOT_NORMAL)

Do we need any locking here? What is to stop the state changing after this 
check?

> +		FIRENZE_PCI_ERR("%016llx SPOWER: Unexpected state %08x\n",
> +				slot->id, slot->state);
> +
> +	if (val != PCI_SLOT_POWER_OFF && val != PCI_SLOT_POWER_ON)
> +		return OPAL_PARAMETER;
> +
> +	if (slot->power_state == val)
> +		return OPAL_SUCCESS;
> +
> +	slot->power_state = val;
> +	pci_slot_set_state(slot, FIRENZE_PCI_SLOT_SPOWER_START);
> +
> +	plat_slot->req->op = SMBUS_WRITE;
> +	pval = (uint8_t *)plat_slot->req->rw_buf;
> +	if (val == PCI_SLOT_POWER_ON) {
> +		*pval = *plat_slot->power_status;
> +		(*pval) &= ~plat_slot->power_mask;
> +		(*pval) |= plat_slot->power_on;
> +	} else {
> +		*pval = *plat_slot->power_status;
> +		(*pval) &= ~plat_slot->power_mask;
> +		(*pval) |= plat_slot->power_off;
> +	}
> +
> +	if (pci_slot_has_flags(slot, PCI_SLOT_FLAG_BOOTUP))
> +		i2c_set_req_timeout(plat_slot->req, FIRENZE_PCI_I2C_TIMEOUT);
> +	else
> +		i2c_set_req_timeout(plat_slot->req, 0ul);
> +	i2c_queue_req(plat_slot->req);
> +
> +	return OPAL_ASYNC_COMPLETION;
> +}
>
> +static struct i2c_bus *firenze_pci_find_i2c_bus(uint8_t chip,
> +						uint8_t eng,
> +						uint8_t port)
> +{
> +	struct dt_node *np, *child;
> +	uint32_t reg;
> +
> +	/* Iterate I2C masters */
> +	dt_for_each_compatible(dt_root, np, "ibm,power8-i2cm") {
> +		if (!np->parent ||
> +		    !dt_node_is_compatible(np->parent, "ibm,power8-xscom"))
> +			continue;
> +
> +		/* Check chip index */
> +		reg = dt_prop_get_u32(np->parent, "ibm,chip-id");
> +		if (reg != chip)
> +			continue;
> +
> +		/* Check I2C master index */
> +		reg = dt_prop_get_u32(np, "chip-engine#");
> +		if (reg != eng)
> +			continue;
> +
> +		/* Iterate I2C buses */
> +		dt_for_each_child(np, child) {
> +			if (!dt_node_is_compatible(child, "ibm,power8-i2c-
port"))
> +				continue;
> +
> +			/* Check I2C port index */
> +			reg = dt_prop_get_u32(child, "reg");
> +			if (reg != port)
> +				continue;
> +
> +			reg = dt_prop_get_u32(child, "ibm,opal-id");
> +			return i2c_find_bus_by_id(reg);
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static void firenze_pci_slot_init(struct pci_slot *slot)
> +{
> +	struct lxvpd_pci_slot *s = slot->data;
> +	struct firenze_pci_slot *plat_slot = slot->data;
> +	struct firenze_pci_slot_info *info = NULL;
> +	uint32_t vdid;
> +	uint8_t buddy;
> +	int i;
> +
> +	/* Search for PCI slot info */
> +	for (i = 0; i < ARRAY_SIZE(firenze_pci_slots); i++) {
> +		if (firenze_pci_slots[i].index == s->slot_index &&
> +		    !strcmp(firenze_pci_slots[i].label, s->label)) {
> +			info = &firenze_pci_slots[i];
> +			break;
> +		}
> +	}
> +	if (!info)
> +		return;
> +
> +	/* Search I2C bus for external power mgt */
> +	buddy = info->buddy;
> +	plat_slot->i2c_bus = firenze_pci_find_i2c_bus(info->chip_id,
> +						      info->master_id,
> +						      info->port_id);
> +	if (plat_slot->i2c_bus) {
> +		plat_slot->req = i2c_alloc_req(plat_slot->i2c_bus);
> +		if (!plat_slot->req)
> +			plat_slot->i2c_bus = NULL;
> +
> +		plat_slot->req->dev_addr	= info->slave_addr;
> +		plat_slot->req->offset_bytes	= 1;
> +		plat_slot->req->offset		= 0x69;
> +		plat_slot->req->rw_buf		= plat_slot->i2c_rw_buf;
> +		plat_slot->req->rw_len		= 1;
> +		plat_slot->req->completion	= firenze_i2c_req_done;
> +		plat_slot->req->user_data	= slot;
> +		switch (info->channel) {
> +		case 0:
> +			plat_slot->power_status = 
&firenze_pci_slots[buddy].power_status;
> +			plat_slot->power_mask = 0x33;
> +			plat_slot->power_on   = 0x22;
> +			plat_slot->power_off  = 0x33;
> +			break;
> +		case 1:
> +			plat_slot->power_status = 
&firenze_pci_slots[buddy].power_status;
> +			plat_slot->power_mask = 0xcc;
> +			plat_slot->power_on   = 0x88;
> +			plat_slot->power_off  = 0xcc;
> +			break;
> +		default:
> +			FIRENZE_PCI_DBG("%016llx: Invalid channel %d\n",
> +					slot->id, info->channel);
> +			plat_slot->i2c_bus = NULL;
> +		}
> +	}
> +
> +	/*
> +	 * If the slot has external power logic, to override the
> +	 * default power management methods. Because of the bad
> +	 * I2C design, the API supplied by I2C is really hard to
> +	 * be utilized. To figure out power status retrival or
> +	 * configuration after we have a blocking API for that.
> +	 */
> +	if (plat_slot->i2c_bus) {
> +		slot->ops.freset = firenze_pci_slot_freset;
> +		slot->ops.get_power_state = firenze_pci_slot_get_power_state;
> +		slot->ops.set_power_state = firenze_pci_slot_set_power_state;
> +		FIRENZE_PCI_DBG("%016llx: External power mgt initialized\n",
> +				slot->id);
> +	} else if (info->inband_perst) {
> +		/*
> +		 * For PLX downstream ports, PCI config register can be
> +		 * leveraged to do PERST. If the slot doesn't have external
> +		 * power management stuff, lets try to stick to the PERST
> +		 * logic if applicable
> +		 */
> +		if (slot->pd->dev_type == PCIE_TYPE_SWITCH_DNPORT) {
> +			pci_cfg_read32(slot->phb, slot->pd->bdfn,
> +				       PCI_CFG_VENDOR_ID, &vdid);
> +			switch (vdid) {
> +			case 0x873210b5:        /* PLX8732 */
> +			case 0x874810b5:        /* PLX8748 */
> +				plat_slot->perst_reg = 0x80;
> +				plat_slot->perst_bit = 0x0400;
> +				slot->ops.freset = firenze_pci_slot_perst;
> +				break;
> +			}
> +		}
> +	}

Moving freset etc. into core code would also move this into core code which is 
a good thing. It seems there are two methods for power on/off/perst slots:

1) A platform specific one (in this case using an i2c device)
2) A PCI standard one implemented by switches

It appears we use (1) if possible and fall back to (2) if it isn't and the 
device is downstream from a switch. This logic makes sense, but again it's 
something that I assume would be the same for other OpenPower platforms so it 
would be better if we didn't have to copy this logic into every platform.

> +	/*
> +         * Anyway, the slot has platform specific info. That
> +         * requires platform specific method to parse it out
> +         * properly.
> +         */
> +	slot->ops.add_properties = lxvpd_add_slot_properties;
> +}
> +
> +void firenze_pci_setup_phb(struct phb *phb, unsigned int index)
> +{
> +	uint32_t hub_id;
> +	struct pci_slot *slot;
> +	struct lxvpd_pci_slot *s;
> +
> +	/* Grab Hub ID used to parse VPDs */
> +	hub_id = dt_prop_get_u32_def(phb->dt_node, "ibm,hub-id", 0);
> +
> +	/* Process the pcie slot entries from the lx vpd lid */
> +	lxvpd_process_slot_entries(phb, dt_root, hub_id,
> +				   index, sizeof(struct firenze_pci_slot));
> +
> +	/* Fixup PHB3 slot */
> +	slot = phb->slot;
> +	s = slot ? lxvpd_get_slot(slot) : NULL;
> +	if (s) {
> +                lxvpd_extract_info(slot, s);
> +		firenze_pci_slot_init(slot);
> +	}
> +}
> +
> +static void firenze_pci_i2c_complete(int rc, struct i2c_request *req)
> +{
> +	*(int *)req->user_data = rc;
> +}
> +
> +static void firenze_pci_do_i2c_byte(uint8_t chip, uint8_t eng, uint8_t 
port,
> +				    uint8_t addr, uint8_t reg, uint8_t data)
> +{
> +	struct i2c_bus *bus;
> +	struct i2c_request *req;
> +	uint8_t verif;
> +	int rc;
> +
> +        bus = firenze_pci_find_i2c_bus(chip, eng, port);
> +        if (!bus) {
> +                prerror("FIRENZE: Failed to find i2c (%d/%d/%d)\n", chip, 
eng, port);
> +                return;
> +        }
> +        req = i2c_alloc_req(bus);
> +        if (!req) {
> +                prerror("FIRENZE: Failed to allocate i2c request\n");
> +                return;
> +        }
> +        req->op           = SMBUS_WRITE;
> +        req->dev_addr     = addr >> 1;
> +        req->offset_bytes = 1;
> +        req->offset       = reg;
> +        req->rw_buf       = &data;
> +        req->rw_len       = 1;
> +        req->completion   = firenze_pci_i2c_complete;
> +        req->user_data    = &rc;
> +        rc = 1;
> +        i2c_queue_req(req);
> +        while(rc == 1) {
> +                time_wait_us(10);
> +        }
> +        if (rc != 0) {
> +                prerror("FIRENZE: I2C error %d writing byte\n", rc);
> +                return;
> +        }
> +        req->op           = SMBUS_READ;
> +        req->dev_addr     = addr >> 1;
> +        req->offset_bytes = 1;
> +        req->offset       = reg;
> +        req->rw_buf       = &verif;
> +        req->rw_len       = 1;
> +        req->completion   = firenze_pci_i2c_complete;
> +        req->user_data    = &rc;
> +        rc = 1;
> +        i2c_queue_req(req);
> +        while(rc == 1) {
> +                time_wait_us(10);
> +        }
> +        if (rc != 0) {
> +                prerror("FIRENZE: I2C error %d reading byte\n", rc);
> +                return;
> +        }
> +        if (verif != data) {
> +                prerror("FIRENZE: I2C miscompare want %02x got %02x\n", 
data, verif);
> +        }
> +}
> +
> +static void firenze_pci_slot_fixup(struct pci_slot *slot)
> +{
> +	uint64_t id;
> +	const uint32_t *p;
> +	struct lxvpd_pci_slot *s;
> +
> +	p = dt_prop_get_def(dt_root, "ibm,vpd-lx-info", NULL);
> +	if (!p)
> +		return;
> +
> +	/* FIXME: support fixup with generic way */
> +	id = ((uint64_t)p[1] << 32) | p[2];
> +	id = 0;
> +	if (id != LX_VPD_2S4U_BACKPLANE &&
> +	    id != LX_VPD_1S4U_BACKPLANE)
> +		return;
> +
> +	s = slot->data;
> +	if (!s || !s->pluggable)
> +		return;
> +
> +	/* Note: We apply the settings twice for C6/C7 but that shouldn't
> +	 * be a problem
> +	 */
> +	if (!strncmp(s->label, "C6 ", 2) ||
> +	    !strncmp(s->label, "C7 ", 2)) {
> +		printf("FIRENZE: Fixing power on %s...\n", s->label);
> +		firenze_pci_do_i2c_byte(0, 1, 0, 0x6a, 0x5e, 0xfa);
> +		firenze_pci_do_i2c_byte(0, 1, 0, 0x6a, 0x5a, 0xff);
> +		firenze_pci_do_i2c_byte(0, 1, 0, 0x6a, 0x5b, 0xff);
> +	} else if (!strncmp(s->label, "C5 ", 2)) {
> +                printf("FIRENZE: Fixing power on %s...\n", s->label);
> +		firenze_pci_do_i2c_byte(0, 1, 0, 0x72, 0x5e, 0xfb);
> +		firenze_pci_do_i2c_byte(0, 1, 0, 0x72, 0x5b, 0xff);
> +        } else if (!strncmp(s->label, "C3 ", 2)) {
> +		printf("FIRENZE: Fixing power on %s...\n", s->label);
> +		firenze_pci_do_i2c_byte(0, 1, 0, 0x74, 0x5e, 0xfb);
> +		firenze_pci_do_i2c_byte(0, 1, 0, 0x74, 0x5b, 0xff);
> +        }
> +}
> +
> +void firenze_pci_get_slot_info(struct phb *phb, struct pci_device *pd)
> +{
> +	struct pci_slot *slot;
> +	struct lxvpd_pci_slot *s;
> +
> +	/* Prepare the PCI inventory */
> +	firenze_pci_add_inventory(phb, pd);
> +
> +	if (pd->dev_type != PCIE_TYPE_ROOT_PORT &&
> +	    pd->dev_type != PCIE_TYPE_SWITCH_UPPORT &&
> +	    pd->dev_type != PCIE_TYPE_SWITCH_DNPORT &&
> +	    pd->dev_type != PCIE_TYPE_PCIE_TO_PCIX)
> +		return;
> +
> +	/* Create PCIe slot */
> +	slot = pcie_slot_create(phb, pd);
> +	if (!slot)
> +		return;
> +
> +	/* Root complex inherits methods from PHB slot */
> +	if (!pd->parent && phb->slot)
> +		memcpy(&slot->ops, &phb->slot->ops, sizeof(struct 
pci_slot_ops));
> +
> +	/* Patch PCIe slot */
> +	s = lxvpd_get_slot(slot);
> +	if (s) {
> +		lxvpd_extract_info(slot, s);
> +		firenze_pci_slot_init(slot);
> +	}
> +
> +	/* Fixup the slot's power status */
> +	firenze_pci_slot_fixup(slot);
> +}
> diff --git a/platforms/ibm-fsp/firenze.c b/platforms/ibm-fsp/firenze.c
> index 75f566c..00aba8d 100644
> --- a/platforms/ibm-fsp/firenze.c
> +++ b/platforms/ibm-fsp/firenze.c
> @@ -28,30 +28,6 @@
>  #include "ibm-fsp.h"
>  #include "lxvpd.h"
>  
> -/* Structure used to send PCIe card info to FSP */
> -struct fsp_pcie_entry {
> -	uint32_t	hw_proc_id;
> -	uint16_t	slot_idx;
> -	uint16_t	reserved;
> -	uint16_t	vendor_id;
> -	uint16_t	device_id;
> -	uint16_t	subsys_vendor_id;
> -	uint16_t	subsys_device_id;
> -};
> -
> -struct fsp_pcie_inventory {
> -	uint32_t		version; /* currently 1 */
> -	uint32_t		num_entries;
> -	uint32_t		entry_size;
> -	uint32_t		entry_offset;
> -	struct fsp_pcie_entry	entries[];
> -};
> -
> -static struct fsp_pcie_inventory *fsp_pcie_inv;
> -static unsigned int fsp_pcie_inv_alloc_count;
> -#define FSP_PCIE_INV_ALLOC_CHUNK	4
> -static uint64_t lx_vpd_id;
> -
>  struct lock fsp_pcie_inv_lock = LOCK_UNLOCKED;
>  
>  static struct dt_node *dt_create_i2c_master(struct dt_node *n, uint32_t 
eng_id)
> @@ -122,44 +98,6 @@ static struct dt_node *dt_create_i2c_device(struct 
dt_node *bus, uint8_t addr,
>  	return dev;
>  }
>  
> -static struct i2c_bus *firenze_pci_find_i2c_bus(uint8_t chip, uint8_t eng, 
uint8_t port)
> -{
> -       struct dt_node *np, *child;
> -       uint32_t reg;
> -
> -       /* Iterate I2C masters */
> -       dt_for_each_compatible(dt_root, np, "ibm,power8-i2cm") {
> -               if (!np->parent ||
> -                   !dt_node_is_compatible(np->parent, "ibm,power8-xscom"))
> -                       continue;
> -
> -               /* Check chip index */
> -               reg = dt_prop_get_u32(np->parent, "ibm,chip-id");
> -               if (reg != chip)
> -                       continue;
> -
> -               /* Check I2C master index */
> -               reg = dt_prop_get_u32(np, "chip-engine#");
> -               if (reg != eng)
> -                       continue;
> -
> -               /* Iterate I2C buses */
> -               dt_for_each_child(np, child) {
> -                       if (!dt_node_is_compatible(child, "ibm,power8-i2c-
port"))
> -                               continue;
> -
> -                       /* Check I2C port index */
> -                       reg = dt_prop_get_u32(child, "reg");
> -                       if (reg != port)
> -                               continue;
> -
> -                       reg = dt_prop_get_u32(child, "ibm,opal-id");
> -                       return i2c_find_bus_by_id(reg);
> -               }
> -       }
> -       return NULL;
> -}
> -
>  static void firenze_dt_fixup_i2cm(void)
>  {
>  	struct dt_node *master, *bus, *dev;
> @@ -256,281 +194,6 @@ static bool firenze_probe(void)
>  	return true;
>  }
>  
> -static void firenze_send_pci_inventory(void)
> -{
> -	uint64_t base, abase, end, aend, offset;
> -	int64_t rc;
> -
> -	if (!fsp_pcie_inv)
> -		return;
> -
> -	prlog(PR_DEBUG, "PLAT: Sending PCI inventory to FSP, table has"
> -	      " %d entries\n",
> -	      fsp_pcie_inv->num_entries);
> -
> -	{
> -		unsigned int i;
> -
> -		prlog(PR_DEBUG, "HWP SLT VDID DVID SVID SDID\n");
> -		prlog(PR_DEBUG, "---------------------------\n");
> -		for (i = 0; i < fsp_pcie_inv->num_entries; i++) {
> -			struct fsp_pcie_entry *e = &fsp_pcie_inv->entries[i];
> -
> -			prlog(PR_DEBUG, "%03d %03d %04x %04x %04x %04x\n",
> -			      e->hw_proc_id, e->slot_idx,
> -			      e->vendor_id, e->device_id,
> -			      e->subsys_vendor_id, e->subsys_device_id);
> -		}
> -	}
> -
> -	/*
> -	 * Get the location of the table in a form we can send
> -	 * to the FSP
> -	 */
> -	base = (uint64_t)fsp_pcie_inv;
> -	end = base + sizeof(struct fsp_pcie_inventory) +
> -		fsp_pcie_inv->num_entries * fsp_pcie_inv->entry_size;
> -	abase = base & ~0xffful;
> -	aend = (end + 0xffful) & ~0xffful;
> -	offset = PSI_DMA_PCIE_INVENTORY + (base & 0xfff);
> -
> -	/* We can only accommodate so many entries in the PSI map */
> -	if ((aend - abase) > PSI_DMA_PCIE_INVENTORY_SIZE) {
> -		prerror("PLAT: PCIe inventory too large (%lld bytes)\n",
> -			aend - abase);
> -		goto bail;
> -	}
> -
> -	/* Map this in the TCEs */
> -	fsp_tce_map(PSI_DMA_PCIE_INVENTORY, (void *)abase, aend - abase);
> -
> -	/* Send FSP message */
> -	rc = fsp_sync_msg(fsp_mkmsg(FSP_CMD_PCI_POWER_CONF, 3,
> -				    hi32(offset), lo32(offset),
> -				    end - base), true);
> -	if (rc)
> -		prerror("PLAT: FSP error %lld sending inventory\n", rc);
> -
> -	/* Unmap */
> -	fsp_tce_unmap(PSI_DMA_PCIE_INVENTORY, aend - abase);
> - bail:
> -	/*
> -	 * We free the inventory. We'll have to redo that on hotplug
> -	 * when we support it but that isn't the case yet
> -	 */
> -	free(fsp_pcie_inv);
> -	fsp_pcie_inv = NULL;
> -}
> -
> -static void firenze_i2c_complete(int rc, struct i2c_request *req)
> -{
> -	*(int *)req->user_data = rc;
> -}
> -
> -static void firenze_do_i2c_byte(uint8_t chip, uint8_t eng, uint8_t port,
> -				uint8_t addr, uint8_t reg, uint8_t data)
> -{
> -	struct i2c_bus *bus;
> -	struct i2c_request *req;
> -	uint8_t verif;
> -	int rc;
> -
> -	bus = firenze_pci_find_i2c_bus(chip, eng, port);
> -	if (!bus) {
> -		prerror("FIRENZE: Failed to find i2c (%d/%d/%d)\n", chip, eng, 
port);
> -		return;
> -	}
> -	req = i2c_alloc_req(bus);
> -	if (!req) {
> -		prerror("FIRENZE: Failed to allocate i2c request\n");
> -		return;
> -	}
> -	req->op		     = SMBUS_WRITE;
> -	req->dev_addr        = addr >> 1;
> -	req->offset_bytes    = 1;
> -	req->offset          = reg;
> -	req->rw_buf          = &data;
> -	req->rw_len          = 1;
> -	req->completion      = firenze_i2c_complete;
> -	req->user_data	     = &rc;
> -	rc = 1;
> -	i2c_queue_req(req);
> -	while(rc == 1) {
> -		time_wait_us(10);
> -	}
> -	if (rc != 0) {
> -		prerror("FIRENZE: I2C error %d writing byte\n", rc);
> -		return;
> -	}
> -	req->op		     = SMBUS_READ;
> -	req->dev_addr        = addr >> 1;
> -	req->offset_bytes    = 1;
> -	req->offset          = reg;
> -	req->rw_buf          = &verif;
> -	req->rw_len          = 1;
> -	req->completion      = firenze_i2c_complete;
> -	req->user_data	     = &rc;
> -	rc = 1;
> -	i2c_queue_req(req);
> -	while(rc == 1) {
> -		time_wait_us(10);
> -	}
> -	if (rc != 0) {
> -		prerror("FIRENZE: I2C error %d reading byte\n", rc);
> -		return;
> -	}
> -	if (verif != data) {
> -		prerror("FIRENZE: I2C miscompare want %02x got %02x\n", data, 
verif);
> -	}
> -}
> -
> -static void firenze_fixup_pcie_slot_power(struct pci_device * pd)
> -{
> -	const char *label = pd->slot_info->label;
> -
> -	if (!pd->slot_info->pluggable)
> -		return;
> -
> -	if (lx_vpd_id != LX_VPD_2S4U_BACKPLANE &&
> -	    lx_vpd_id != LX_VPD_1S4U_BACKPLANE)
> -		return;
> -
> -	printf("FIRENZE: Checking slot %s for power fixup\n", label);
> -
> -	/* Note: We apply the settings twice for C6/C7 but that shouldn't
> -	 * be a problem
> -	 */
> -	if (!strncmp(label, "C6 ", 3) || !strncmp(label, "C7 ", 3)) {
> -		printf("FIRENZE: Fixing power on %s...\n", label);
> -		firenze_do_i2c_byte(0, 1, 0, 0x6a, 0x5e, 0xfa);
> -		firenze_do_i2c_byte(0, 1, 0, 0x6a, 0x5a, 0xff);
> -		firenze_do_i2c_byte(0, 1, 0, 0x6a, 0x5b, 0xff);
> -	}
> -	if (!strncmp(label, "C5 ", 3)) {
> -		printf("FIRENZE: Fixing power on %s...\n", label);
> -		firenze_do_i2c_byte(0, 1, 0, 0x72, 0x5e, 0xfb);
> -		firenze_do_i2c_byte(0, 1, 0, 0x72, 0x5b, 0xff);
> -	}
> -	if (!strncmp(label, "C3 ", 3)) {
> -		printf("FIRENZE: Fixing power on %s...\n", label);
> -		firenze_do_i2c_byte(0, 1, 0, 0x74, 0x5e, 0xfb);
> -		firenze_do_i2c_byte(0, 1, 0, 0x74, 0x5b, 0xff);
> -	}
> -}
> -
> -static void firenze_add_pcidev_to_fsp_inventory(struct phb *phb,
> -						struct pci_device *pd)
> -{
> -	struct fsp_pcie_entry *entry;
> -	struct proc_chip *chip;
> -
> -	/* Check if we need to do some (Re)allocation */
> -	if (!fsp_pcie_inv ||
> -	    fsp_pcie_inv->num_entries == fsp_pcie_inv_alloc_count) {
> -		unsigned int new_count;
> -		size_t new_size;
> -		bool need_init = !fsp_pcie_inv;
> -
> -		/* (Re)allocate the block to the new size */
> -		new_count = fsp_pcie_inv_alloc_count + 
FSP_PCIE_INV_ALLOC_CHUNK;
> -		new_size = sizeof(struct fsp_pcie_inventory);
> -		new_size += sizeof(struct fsp_pcie_entry) * new_count;
> -		fsp_pcie_inv = realloc(fsp_pcie_inv, new_size);
> -		fsp_pcie_inv_alloc_count = new_count;
> -
> -		/* Initialize the header for a new inventory */
> -		if (need_init) {
> -			fsp_pcie_inv->version = 1;
> -			fsp_pcie_inv->num_entries = 0;
> -			fsp_pcie_inv->entry_size =
> -				sizeof(struct fsp_pcie_entry);
> -			fsp_pcie_inv->entry_offset =
> -				offsetof(struct fsp_pcie_inventory, entries);
> -		}
> -	}
> -
> -	/* Add entry */
> -	entry = &fsp_pcie_inv->entries[fsp_pcie_inv->num_entries++];
> -	chip = get_chip(dt_get_chip_id(phb->dt_node));
> -	if (!chip) {
> -		prerror("PLAT: Failed to get chip for PHB !\n");
> -		return;
> -	}
> -	entry->hw_proc_id = chip->pcid;
> -	entry->slot_idx = pd->parent->slot_info->slot_index;
> -	entry->reserved = 0;
> -	pci_cfg_read16(phb, pd->bdfn, PCI_CFG_VENDOR_ID, &entry->vendor_id);
> -	pci_cfg_read16(phb, pd->bdfn, PCI_CFG_DEVICE_ID, &entry->device_id);
> -	if (pd->is_bridge) {
> -		int64_t ssvc = pci_find_cap(phb, pd->bdfn,
> -					    PCI_CFG_CAP_ID_SUBSYS_VID);
> -		if (ssvc < 0) {
> -			entry->subsys_vendor_id = 0xffff;
> -			entry->subsys_device_id = 0xffff;
> -		} else {
> -			pci_cfg_read16(phb, pd->bdfn,
> -				       ssvc + PCICAP_SUBSYS_VID_VENDOR,
> -				       &entry->subsys_vendor_id);
> -			pci_cfg_read16(phb, pd->bdfn,
> -				       ssvc + PCICAP_SUBSYS_VID_DEVICE,
> -				       &entry->subsys_device_id);
> -		}
> -	} else {
> -		pci_cfg_read16(phb, pd->bdfn, PCI_CFG_SUBSYS_VENDOR_ID,
> -			       &entry->subsys_vendor_id);
> -		pci_cfg_read16(phb, pd->bdfn, PCI_CFG_SUBSYS_ID,
> -			       &entry->subsys_device_id);
> -	}
> -}
> -
> -static void firenze_get_slot_info(struct phb *phb, struct pci_device * pd)
> -{
> -	/* Call the main LXVPD function first */
> -	lxvpd_get_slot_info(phb, pd);
> -
> -	/*
> -	 * Do we need to add that to the FSP inventory for power management ?
> -	 *
> -	 * For now, we only add devices that:
> -	 *
> -	 *  - Are function 0
> -	 *  - Are not an RC or a downstream bridge
> -	 *  - Have a direct parent that has a slot entry
> -	 *  - Slot entry says pluggable
> -	 *  - Aren't an upstream switch that has slot info
> -	 */
> -	if (!pd)
> -		return;
> -	if (pd->dev_type == PCIE_TYPE_ROOT_PORT ||
> -	    pd->dev_type == PCIE_TYPE_SWITCH_DNPORT) {
> -		firenze_fixup_pcie_slot_power(pd);
> -		return;
> -	}
> -	if (pd->bdfn & 7)
> -		return;
> -	if (pd->dev_type == PCIE_TYPE_SWITCH_UPPORT &&
> -	    pd->slot_info)
> -		return;
> -	if (!pd->parent || !pd->parent->slot_info)
> -		return;
> -	if (!pd->parent->slot_info->pluggable)
> -		return;
> -	lock(&fsp_pcie_inv_lock);
> -	firenze_add_pcidev_to_fsp_inventory(phb, pd);
> -	unlock(&fsp_pcie_inv_lock);
> -}
> -
> -static void firenze_setup_phb(struct phb *phb, unsigned int index)
> -{
> -	uint32_t hub_id;
> -
> -	/* Grab Hub ID used to parse VPDs */
> -	hub_id = dt_prop_get_u32_def(phb->dt_node, "ibm,hub-id", 0);
> -
> -	/* Process the pcie slot entries from the lx vpd lid */
> -	lxvpd_process_slot_entries(phb, dt_root, hub_id, index);
> -}
> -
>  static uint32_t ibm_fsp_occ_timeout(void)
>  {
>  	/* Use a fixed 60s value for now */
> @@ -554,9 +217,9 @@ DECLARE_PLATFORM(firenze) = {
>  	.exit			= ibm_fsp_exit,
>  	.cec_power_down		= ibm_fsp_cec_power_down,
>  	.cec_reboot		= ibm_fsp_cec_reboot,
> -	.pci_setup_phb		= firenze_setup_phb,
> -	.pci_get_slot_info	= firenze_get_slot_info,
> -	.pci_probe_complete	= firenze_send_pci_inventory,
> +	.pci_setup_phb		= firenze_pci_setup_phb,
> +	.pci_get_slot_info	= firenze_pci_get_slot_info,
> +	.pci_probe_complete	= firenze_pci_send_inventory,
>  	.nvram_info		= fsp_nvram_info,
>  	.nvram_start_read	= fsp_nvram_start_read,
>  	.nvram_write		= fsp_nvram_write,
> @@ -566,4 +229,4 @@ DECLARE_PLATFORM(firenze) = {
>  	.resource_loaded	= fsp_resource_loaded,
>  	.sensor_read		= ibm_fsp_sensor_read,
>  	.terminate		= ibm_fsp_terminate,
> -} ;
> +};
> diff --git a/platforms/ibm-fsp/ibm-fsp.h b/platforms/ibm-fsp/ibm-fsp.h
> index 3b24b5b..3f6e9c5 100644
> --- a/platforms/ibm-fsp/ibm-fsp.h
> +++ b/platforms/ibm-fsp/ibm-fsp.h
> @@ -30,4 +30,17 @@ extern int elog_fsp_commit(struct errorlog *buf);
>  extern int64_t ibm_fsp_sensor_read(uint32_t sensor_hndl, int token,
>  				uint32_t *sensor_data);
>  
> +/* Apollo PCI support */
> +extern void apollo_pci_setup_phb(struct phb *phb,
> +				 unsigned int index);
> +extern void apollo_pci_get_slot_info(struct phb *phb,
> +				     struct pci_device *pd);
> +
> +/* Firenze PCI support */
> +extern void firenze_pci_send_inventory(void);
> +extern void firenze_pci_setup_phb(struct phb *phb,
> +				  unsigned int index);
> +extern void firenze_pci_get_slot_info(struct phb *phb,
> +				      struct pci_device *pd);
> +
>  #endif /*  __IBM_FSP_COMMON_H */
> diff --git a/platforms/ibm-fsp/lxvpd.c b/platforms/ibm-fsp/lxvpd.c
> index 542c49a..1796013 100644
> --- a/platforms/ibm-fsp/lxvpd.c
> +++ b/platforms/ibm-fsp/lxvpd.c
> @@ -13,11 +13,6 @@
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
>   */
> -/*
> - * LXVPD support
> - *
> - */
> -
>  
>  #include <skiboot.h>
>  #include <device.h>
> @@ -28,31 +23,35 @@
>  
>  #include "lxvpd.h"
>  
> -struct lxvpd_slot_info {
> -	uint8_t    switch_id;
> -	uint8_t    vswitch_id;
> -	uint8_t    dev_id;
> -	struct pci_slot_info	ps;
> -};
> +/* Debugging options */
> +#define LXVPD_DBG(fmt, a...)	prlog(PR_DEBUG, "LXVPD: " fmt, ##a)
> +#define LXVPD_INFO(fmt, a...)	prlog(PR_INFO, "LXVPD: " fmt, ##a)
> +#define LXVPD_WARN(fmt, a...)	prlog(PR_WARNING, "LXVPD: " fmt, ##a)
> +#define LXVPD_ERR(fmt, a...)	prlog(PR_ERR, "LXVPD: " fmt, ##a)
>  
>  /*
> - * XXX TODO: Add 1006 maps to add function loc codes and loc code maps
> - * (ie. -Tn part of the location code) 
> + * Currently, the lxvpd PCI slot struct is shared by multiple
> + * platforms (Apollo and Firenze), but each slot still has
> + * platform specific features. In order for unified data structs,
> + * "struct lxvpd_slot" is expected to be embedded in platform
> + * PCI slot struct. "entry_size" indicates the size of platform
> + * specific PCI slot instance.
>   */
> -struct lxvpd_slot_info_data {
> -	uint8_t			num_slots;
> -	struct lxvpd_slot_info	info[];
> +struct lxvpd_pci_slot_data {
> +	uint8_t		num_slots;
> +	int32_t		entry_size;	/* Size of platform PCI slot  */
> +	void		*slots;		/* Data of platform PCI slots */
>  };
>  
>  static bool lxvpd_supported_slot(struct phb *phb, struct pci_device *pd)
>  {
> -	/* PCI/PCI-X we only support top level PHB with NULL "pd" */
> -	if (phb->phb_type < phb_type_pcie_v1)
> -		return pd == NULL;
> +	/* PHB should always be valid */
> +	if (!phb)
> +		return false;
>  
> -	/* Now we have PCI Express, we should never have a NULL "pd" */
> +	/* We expect platform slot for root complex */
>  	if (!pd)
> -		return false;
> +		return true;
>  
>  	/* We support the root complex at the top level */
>  	if (pd->dev_type == PCIE_TYPE_ROOT_PORT && !pd->parent)
> @@ -78,174 +77,214 @@ static bool lxvpd_supported_slot(struct phb *phb, 
struct pci_device *pd)
>  	return false;
>  }
>  
> -void lxvpd_get_slot_info(struct phb *phb, struct pci_device * pd)
> +void *lxvpd_get_slot(struct pci_slot *slot)
>  {
> -	struct lxvpd_slot_info_data *sdata = phb->platform_data;
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +	struct lxvpd_pci_slot_data *sdata = phb->platform_data;
> +	struct lxvpd_pci_slot *s = NULL;
> +	uint8_t slot_num = pd ? ((pd->bdfn >> 3) & 0x1f) : 0xff;
>  	bool is_phb = (pd && pd->parent) ? false : true;
> -	bool entry_found = false;
> -	uint8_t idx;
> +	uint8_t index;
>  
>  	/* Check if we have slot info */
> -	if (!sdata)
> -		return;
> +	if (!sdata) {
> +		LXVPD_DBG("PHB%04x not have VPD data\n",
> +			  phb->opal_id);
> +		return NULL;
> +	}
>  
> -	prlog(PR_TRACE, "LXVPD: Get Slot Info PHB%d pd=%x\n", phb->opal_id,
> -	    pd ? pd->bdfn : 0);
> +	/* Platform slot attached ? */
> +	s = slot->data;
> +	if (s) {
> +		LXVPD_DBG("Slot %016llx had platform data [%s]\n",
> +			  slot->id, s->label);
> +		return s;
> +	}
>  
>  	/*
> -	 * This code only handles PHBs and PCIe switches at the top level.
> -	 *
> -	 * We do not handle any other switch nor any other type of PCI/PCI-X
> -	 * bridge.
> +	 * This code only handles PHBs and PCIe switches at the
> +	 * top level. We do not handle any other switch nor any
> +	 * other type of PCI/PCI-X bridge. Generally, we have
> +	 * more strict rules to support slot than PCI core.
>  	 */
>  	if (!lxvpd_supported_slot(phb, pd)) {
> -		prlog(PR_TRACE, "LXVPD: Unsupported slot\n");
> -		return;
> +		LXVPD_DBG("Slot %016llx not supported\n",
> +			  slot->id);
> +		return NULL;
>  	}
>  
> -	/* Iterate the slot map */
> -	for (idx = 0; idx <= sdata->num_slots; idx++) {
> -		struct lxvpd_slot_info *info = &sdata->info[idx];
> -		uint8_t pd_dev = (pd->bdfn >> 3) & 0x1f;
> +	/* Iterate the platform slot array */
> +	for (index = 0; index < sdata->num_slots; index++) {
> +		s = sdata->slots + (index * sdata->entry_size);
>  
>  		/* Match PHB with switch_id == 0 */
> -		if (is_phb && info->switch_id == 0) {
> -			entry_found = true;
> -			break;
> +		if (is_phb && s->switch_id == 0) {
> +			slot->data = s;
> +			s->pci_slot = slot;
> +			LXVPD_DBG("Found [%s] for PHB slot %016llx\n",
> +				  s->label, slot->id);
> +
> +			return s;
>  		}
>  
>  		/* Match switch port with switch_id != 0 */
> -		if (!is_phb && info->switch_id !=0 && info->dev_id == pd_dev) 
{
> -			entry_found = true;
> -			break;
> +		if (!is_phb && s->switch_id != 0 && s->dev_id == slot_num) {
> +			slot->data = s;
> +			s->pci_slot = slot;
> +			LXVPD_DBG("Found [%s] for slot %016llx\n",
> +				  s->label, slot->id);
> +
> +			return s;
>  		}
>  	}
>  
> -	if (entry_found) {
> -		pd->slot_info = &sdata->info[idx].ps;
> -		prlog(PR_TRACE, "PCI: PCIE Slot Info: \n"
> -		      "       Label       %s\n"
> -		      "       Pluggable   0x%x\n"
> -		      "       Power Ctl   0x%x\n"
> -		      "       Wired Lanes 0x%x\n"
> -		      "       Bus Clock   0x%x\n"
> -		      "       Connector   0x%x\n"
> -		      "       Slot Index  %d\n",
> -		      pd->slot_info->label,
> -		      pd->slot_info->pluggable?1:0,
> -		      pd->slot_info->power_ctl?1:0,
> -		      pd->slot_info->wired_lanes,
> -		      pd->slot_info->bus_clock,
> -		      pd->slot_info->connector_type,
> -		      pd->slot_info->slot_index);
> -	} else {
> -		prlog(PR_TRACE, "PCI: PCIE Slot Info Not Found\n");
> -	}
> +	LXVPD_DBG("No data found for %sslot %016llx\n",
> +		  is_phb ? "PHB " : " ", slot->id);
> +	return NULL;
>  }
>  
> -static struct lxvpd_slot_info *lxvpd_alloc_slot_info(struct phb *phb, int 
count)
> +void lxvpd_extract_info(struct pci_slot *slot, struct lxvpd_pci_slot *s)
>  {
> -	struct lxvpd_slot_info_data *data;
> +	slot->pluggable      = s->pluggable ? 1 : 0;
> +	slot->power_ctl      = s->power_ctl ? 1 : 0;
> +	slot->power_led_ctl  = s->pwr_led_ctl;
> +	slot->attn_led_ctl   = s->attn_led_ctl;
> +	slot->connector_type = s->connector_type;
> +	slot->card_desc      = s->card_desc;
> +	slot->card_mech      = s->card_mech;
> +	slot->wired_lanes    = s->wired_lanes;
> +}
> +
> +static struct lxvpd_pci_slot_data *lxvpd_alloc_slots(struct phb *phb,
> +						     uint8_t count,
> +						     uint32_t slot_size)
> +{
> +	struct lxvpd_pci_slot_data *sdata;
>  
> -	data = zalloc(sizeof(struct lxvpd_slot_info_data) *
> -		      count * sizeof(struct lxvpd_slot_info));
> -	assert(data);
> -	data->num_slots = count;
> -	phb->platform_data = data;
> +	sdata = zalloc(sizeof(struct lxvpd_pci_slot_data) + count * 
slot_size);
> +	assert(sdata);
> +	sdata->num_slots   = count;
> +	sdata->entry_size  = slot_size;
> +	sdata->slots       = sdata + 1;
> +	phb->platform_data = sdata;
>  
> -	return data->info;
> +	return sdata;
>  }
>  
> -static void lxvpd_parse_1004_map(struct phb *phb, const uint8_t *sm, 
uint8_t sz)
> +static void lxvpd_format_label(char *dst, const char *src, size_t len)
>  {
> -	const struct pci_slot_entry_1004 *entry = NULL;
> -	struct lxvpd_slot_info *slot_info, *info;
> -	uint8_t num_slots, slot, idx;
> +	int i;
>  
> -	num_slots = (sz / sizeof(struct pci_slot_entry_1004));
> -	slot_info = lxvpd_alloc_slot_info(phb, num_slots);
> +	memcpy(dst, src, len);
> +
> +	/* Remove blank suffix */
> +	for (i = strlen(dst) - 1; i >= 0; i--) {
> +		if (dst[i] != ' ')
> +			break;
> +
> +		dst[i] = 0;
> +	}
> +}
> +
> +static void lxvpd_parse_1004_map(struct phb *phb,
> +				 const uint8_t *sm,
> +				 uint8_t size,
> +				 uint32_t slot_size)
> +{
> +	struct lxvpd_pci_slot_data *sdata;
> +	struct lxvpd_pci_slot *s;
> +	const struct pci_slot_entry_1004 *entry;
> +	uint8_t num_slots, slot;
> +
> +	num_slots = (size / sizeof(struct pci_slot_entry_1004));
> +	sdata = lxvpd_alloc_slots(phb, num_slots, slot_size);
>  
>  	/* Iterate through the entries in the keyword */
>  	entry = (const struct pci_slot_entry_1004 *)sm;
> -	for (slot = 0; slot < num_slots; slot++) {
> -		info = &slot_info[slot];
> +	for (slot = 0; slot < num_slots; slot++, entry++) {
> +		s = sdata->slots + slot * sdata->entry_size;
> +
> +		/* Figure out PCI slot info */
> +		lxvpd_format_label(s->label, entry->label, 3);
> +		s->slot_index     = entry->slot_index;
> +		s->switch_id      = entry->pba >> 4;
> +		s->vswitch_id     = entry->pba & 0xf;
> +		s->dev_id         = entry->sba;
> +		s->pluggable      = ((entry->p0.byte & 0x20) == 0);
> +		s->power_ctl      = ((entry->p0.power_ctl & 0x40) == 1);
> +		s->bus_clock      = entry->p2.bus_clock - 4;
> +		s->connector_type = entry->p2.connector_type - 5;
> +		s->card_desc      = entry->p3.byte >> 6;
> +		if (entry->p3.byte < 0xc0)
> +			s->card_desc -= 4;
> +		s->card_mech      = (entry->p3.byte >> 4) & 0x3;
> +		s->pwr_led_ctl    = (entry->p3.byte & 0xf) >> 2;
> +		s->attn_led_ctl   = entry->p3.byte & 0x3;
>  
> -		/* Put slot info into pci device structure */
> -		info->switch_id = entry->pba >> 4;
> -		info->vswitch_id = entry->pba &0xf;
> -		info->dev_id = entry->sba;
> -		for (idx = 0; idx < 3; idx++)
> -			info->ps.label[idx] = entry->label[idx];
> -		info->ps.label[3] = 0;
> -		info->ps.pluggable = ((entry->p0.byte & 0x20) == 0);
> -		info->ps.power_ctl = ((entry->p0.power_ctl & 0x40) == 1);
>  		switch(entry->p1.wired_lanes) {
> -		case 1: info->ps.wired_lanes = PCI_SLOT_WIRED_LANES_PCIX_32; 
break;
> +		case 1: s->wired_lanes = PCI_SLOT_WIRED_LANES_PCIX_32;  break;
>  		case 2: /* fall through */
> -		case 3: info->ps.wired_lanes = PCI_SLOT_WIRED_LANES_PCIX_64; 
break;
> -		case 4: info->ps.wired_lanes = PCI_SLOT_WIRED_LANES_PCIE_X1; 
break;
> -		case 5: info->ps.wired_lanes = PCI_SLOT_WIRED_LANES_PCIE_X4; 
break;
> -		case 6: info->ps.wired_lanes = PCI_SLOT_WIRED_LANES_PCIE_X8; 
break;
> -		case 7: info->ps.wired_lanes = PCI_SLOT_WIRED_LANES_PCIE_X16; 
break;
> +		case 3: s->wired_lanes = PCI_SLOT_WIRED_LANES_PCIX_64;  break;
> +		case 4: s->wired_lanes = PCI_SLOT_WIRED_LANES_PCIE_X1;  break;
> +		case 5: s->wired_lanes = PCI_SLOT_WIRED_LANES_PCIE_X4;  break;
> +		case 6: s->wired_lanes = PCI_SLOT_WIRED_LANES_PCIE_X8;  break;
> +		case 7: s->wired_lanes = PCI_SLOT_WIRED_LANES_PCIE_X16; break;
>  		default:
> -			info->ps.wired_lanes = PCI_SLOT_WIRED_LANES_UNKNOWN;
> +			s->wired_lanes = PCI_SLOT_WIRED_LANES_UNKNOWN;
>  		}
> -		info->ps.wired_lanes = (entry->p1.wired_lanes - 3);
> -		info->ps.bus_clock = (entry->p2.bus_clock - 4);
> -		info->ps.connector_type = (entry->p2.connector_type - 5);
> -		if (entry->p3.byte < 0xC0)
> -			info->ps.card_desc = ((entry->p3.byte >> 6) - 4) ;
> -		else
> -			info->ps.card_desc = (entry->p3.byte >> 6);
> -		info->ps.card_mech = ((entry->p3.byte >> 4) & 0x3);
> -		info->ps.pwr_led_ctl = ((entry->p3.byte & 0xF) >> 2);
> -		info->ps.attn_led_ctl = (entry->p3.byte & 0x3);
> -		info->ps.slot_index = entry->slot_index;
> -		entry++;
> +
> +		LXVPD_DBG("1004 Platform data [%s] %02x %02x on PHB%04x\n",
> +			  s->label, s->switch_id, s->dev_id, phb->opal_id);
>  	}
>  }
>  
> -static void lxvpd_parse_1005_map(struct phb *phb, const uint8_t *sm, 
uint8_t sz)
> +static void lxvpd_parse_1005_map(struct phb *phb,
> +				 const uint8_t *sm,
> +				 uint8_t size,
> +				 uint32_t slot_size)
>  {
> -	const struct pci_slot_entry_1005 *entry = NULL;
> -	struct lxvpd_slot_info *slot_info, *info;
> -	uint8_t num_slots, slot, idx;
> +	struct lxvpd_pci_slot_data *sdata;
> +	struct lxvpd_pci_slot *s;
> +	const struct pci_slot_entry_1005 *entry;
> +	uint8_t num_slots, slot;
>  
> -	num_slots = (sz / sizeof(struct pci_slot_entry_1005));
> -	slot_info = lxvpd_alloc_slot_info(phb, num_slots);
> +	num_slots = (size / sizeof(struct pci_slot_entry_1005));
> +	sdata = lxvpd_alloc_slots(phb, num_slots, slot_size);
>  
>  	/* Iterate through the entries in the keyword */
>  	entry = (const struct pci_slot_entry_1005 *)sm;
> -	for (slot = 0; slot < num_slots; slot++) {
> -		info = &slot_info[slot];
> +	for (slot = 0; slot < num_slots; slot++, entry++) {
> +		s = sdata->slots + slot * sdata->entry_size;
>  
>  		/* Put slot info into pci device structure */
> -		info->switch_id = entry->pba >> 4;
> -		info->vswitch_id = entry->pba &0xf;
> -		info->dev_id = entry->switch_device_id;
> -		for (idx = 0; idx < 8; idx++)
> -			info->ps.label[idx] = entry->label[idx];
> -		info->ps.label[8] = 0;
> -		info->ps.pluggable = (entry->p0.pluggable == 0);
> -		info->ps.power_ctl = entry->p0.power_ctl;
> -		info->ps.wired_lanes = entry->p1.wired_lanes;
> -		if (info->ps.wired_lanes > PCI_SLOT_WIRED_LANES_PCIE_X32)
> -			info->ps.wired_lanes = PCI_SLOT_WIRED_LANES_UNKNOWN;
> -		info->ps.bus_clock = entry->p2.bus_clock;
> -		info->ps.connector_type = entry->p2.connector_type;
> -		info->ps.card_desc = (entry->p3.byte >> 6);
> -		info->ps.card_mech = ((entry->p3.byte >> 4) & 0x3);
> -		info->ps.pwr_led_ctl = ((entry->p3.byte & 0xF) >> 2);
> -		info->ps.attn_led_ctl = (entry->p3.byte & 0x3);
> -		info->ps.slot_index = entry->slot_index;
> -		entry++;
> +		lxvpd_format_label(s->label, entry->label, 8);
> +		s->slot_index     = entry->slot_index;
> +		s->switch_id      = entry->pba >> 4;
> +		s->vswitch_id     = entry->pba & 0xf;
> +		s->dev_id         = entry->switch_device_id;
> +		s->pluggable      = (entry->p0.pluggable == 0);
> +		s->power_ctl      = entry->p0.power_ctl;
> +		s->bus_clock      = entry->p2.bus_clock;
> +		s->connector_type = entry->p2.connector_type;
> +		s->card_desc      = entry->p3.byte >> 6;
> +		s->card_mech      = (entry->p3.byte >> 4) & 0x3;
> +		s->pwr_led_ctl    = (entry->p3.byte & 0xf) >> 2;
> +		s->attn_led_ctl   = entry->p3.byte & 0x3;
> +		s->wired_lanes    = entry->p1.wired_lanes;
> +		if (s->wired_lanes > PCI_SLOT_WIRED_LANES_PCIE_X32)
> +			s->wired_lanes = PCI_SLOT_WIRED_LANES_UNKNOWN;
> +
> +		LXVPD_DBG("1005 Platform data [%s] %02x %02x on PHB%04x\n",
> +			  s->label, s->switch_id, s->dev_id, phb->opal_id);
>  	}
>  }
>  
>  void lxvpd_process_slot_entries(struct phb *phb,
>  				struct dt_node *node,
>  				uint8_t chip_id,
> -				uint8_t index)
> +				uint8_t index,
> +				uint32_t slot_size)
>  {
>  	const void *lxvpd;
>  	const uint8_t *pr_rec, *pr_end, *sm;
> @@ -262,24 +301,23 @@ void lxvpd_process_slot_entries(struct phb *phb,
>  	/* Get LX VPD pointer */
>  	lxvpd = dt_prop_get_def_size(node, "ibm,io-vpd", NULL, &lxvpd_size);
>  	if (!lxvpd) {
> -		printf("LXVPD: LX VPD not found for %s in %p\n",
> -		       record, phb->dt_node);
> +		LXVPD_WARN("No data found for PHB%04x %s\n",
> +			   phb->opal_id, record);
>  		return;
>  	}
>  
>  	pr_rec = vpd_find_record(lxvpd, lxvpd_size, record, &pr_size);
>  	if (!pr_rec) {
> -		printf("LXVPD: %s record not found in LX VPD in %p\n",
> -		       record, phb->dt_node);
> +		LXVPD_WARN("Record %s not found on PHB%04x\n",
> +			   record, phb->opal_id);
>  		return;
>  	}
> -	pr_end = pr_rec + pr_size;
>  
> -	prlog(PR_TRACE, "LXVPD: %s record for PHB%d is %ld bytes\n",
> -	      record, phb->opal_id, pr_size);
> -
> -	/* As long as there's still something in the PRxy record... */
> -	while(pr_rec < pr_end) {
> +	/* As long as there's still something in the PRxy record */
> +	LXVPD_DBG("PHB%04x record %s has %ld bytes\n",
> +		  phb->opal_id, record, pr_size);
> +	pr_end = pr_rec + pr_size;
> +	while (pr_rec < pr_end) {
>  		pr_size = pr_end - pr_rec;
>  
>  		/* Find the next MF keyword */
> @@ -288,24 +326,56 @@ void lxvpd_process_slot_entries(struct phb *phb,
>  		sm = vpd_find_keyword(pr_rec, pr_size, "SM", &sm_sz);
>  		if (!mf || !sm) {
>  			if (!found)
> -				printf("LXVPD: Slot Map keyword %s not 
found\n",
> -				       record);
> +				LXVPD_WARN("Slot Map keyword %s not found\n",
> +					   record);
>  			return;
>  		}
> -		prlog(PR_TRACE, "LXVPD: Found 0x%04x map...\n", *mf);
>  
> +		LXVPD_DBG("Found 0x%04x map...\n", *mf);
>  		switch (*mf) {
>  		case 0x1004:
> -			lxvpd_parse_1004_map(phb, sm + 1, sm_sz - 1);
> +			lxvpd_parse_1004_map(phb, sm + 1, sm_sz - 1, 
slot_size);
>  			found = true;
>  			break;
>  		case 0x1005:
> -			lxvpd_parse_1005_map(phb, sm + 1, sm_sz - 1);
> +			lxvpd_parse_1005_map(phb, sm + 1, sm_sz - 1, 
slot_size);
>  			found = true;
>  			break;
>  			/* Add support for 0x1006 maps ... */
>  		}
> +
>  		pr_rec = sm + sm_sz;
>  	}
>  }
>  
> +void lxvpd_add_slot_properties(struct pci_slot *slot,
> +			       struct dt_node *np)
> +{
> +	struct phb *phb = slot->phb;
> +	struct lxvpd_pci_slot *s = slot->data;
> +	char loc_code[LOC_CODE_SIZE];
> +	size_t base_loc_code_len, slot_label_len;
> +
> +	/* Check if we have platform specific slot */
> +	if (!s || !np)
> +		return;
> +
> +	/* Check PHB base location code */
> +	if (!phb->base_loc_code)
> +		return;
> +
> +	/* Check location length is valid */
> +	base_loc_code_len = strlen(phb->base_loc_code);
> +	slot_label_len = strlen(s->label);
> +	if ((base_loc_code_len + slot_label_len + 1) >= LOC_CODE_SIZE)
> +		return;
> +
> +	/* Location code */
> +	strcpy(loc_code, phb->base_loc_code);
> +	strcat(loc_code, "-");
> +	strcat(loc_code, s->label);
> +	dt_add_property(np, "ibm,slot-location-code",
> +			loc_code, strlen(loc_code) + 1);
> +	dt_add_property_string(np, "ibm,slot-label",
> +			       s->label);
> +}
> diff --git a/platforms/ibm-fsp/lxvpd.h b/platforms/ibm-fsp/lxvpd.h
> index dbb9513..c7ca21b 100644
> --- a/platforms/ibm-fsp/lxvpd.h
> +++ b/platforms/ibm-fsp/lxvpd.h
> @@ -23,8 +23,6 @@
>  #define LX_VPD_1S4U_BACKPLANE	0x3100040100300043ull
>  #define LX_VPD_2S4U_BACKPLANE	0x3100040100300044ull
>  
> -/* P8 PCI Slot Entry Definitions -- 1005 */
> -
>  struct slot_p0 {
>  	union {
>  		uint8_t     byte;
> @@ -83,6 +81,7 @@ struct pci_slot_entry_1004 {
>  	uint8_t               max_slot_power;
>  };
>  
> +/* P8 PCI Slot Entry Definitions -- 1005 */
>  struct pci_slot_entry_1005 {
>  	union {
>  		uint8_t    pba;
> @@ -106,12 +105,30 @@ struct pci_slot_entry_1005 {
>  	uint8_t               rsvd_22[2];
>  };
>  
> -struct phb;
> -
> -extern void lxvpd_process_slot_entries(struct phb *phb,
> -				       struct dt_node *node,
> -				       uint8_t chip_id, uint8_t index);
> -
> -extern void lxvpd_get_slot_info(struct phb *phb, struct pci_device * pd);
> +struct lxvpd_pci_slot {
> +	struct pci_slot	*pci_slot;
> +	uint8_t		switch_id;
> +	uint8_t		vswitch_id;
> +	uint8_t		dev_id;
> +	char		label[9];
> +	bool		pluggable;
> +	bool		power_ctl;
> +	uint8_t		wired_lanes;
> +	uint8_t		bus_clock;
> +	uint8_t		connector_type;
> +	uint8_t		card_desc;
> +	uint8_t		card_mech;
> +	uint8_t		pwr_led_ctl;
> +	uint8_t		attn_led_ctl;
> +	uint8_t		slot_index;
> +};
>  
> +extern void lxvpd_process_slot_entries(struct phb *phb, struct dt_node 
*node,
> +				       uint8_t chip_id, uint8_t index,
> +				       uint32_t slot_size);
> +extern void *lxvpd_get_slot(struct pci_slot *slot);
> +extern void lxvpd_extract_info(struct pci_slot *slot,
> +			       struct lxvpd_pci_slot *s);
> +extern void lxvpd_add_slot_properties(struct pci_slot *slot,
> +				      struct dt_node *np);
>  #endif /* __LXVPD_H */
> 



More information about the Skiboot mailing list