[Skiboot] [PATCH v11 13/23] core/pci: Support PCI slot

Russell Currey ruscur at russell.cc
Tue May 31 14:35:21 AEST 2016


On Fri, 2016-05-20 at 16:32 +1000, Gavin Shan wrote:
> Every PCIE bridge port or PHB is expected to be bound with PCI slot
> , to which various PCI slot's functionalities are attached (e.g. power,
> link, reset). This supports PCI slot:
> 
>    * PCI slot is reprsented by "struct pci_slot".
>    * "struct pci_slot_ops" represents the functions supported on the
>      PCI slot. It's initialized by PCI slot core at the beginning and
>      allowed to be overrided by platform partially or completely.
>    * On PCI hot plugging event, the PCI devices behind the slot are
>      enumarated. Device sub-tree is populated and sent to OS by OPAL
>      message.
>    * On PCI hot unplugging event, the PCI devices behind the slot are
>      destroyed. Device sub-tree is removed and the slot is powered off.
> 
> Signed-off-by: Gavin Shan <gwshan at linux.vnet.ibm.com>

This patch is huge and I don't particularly think it can be split up much, so
apologies if I miss anything.  My comments are pretty minor.  I think there's
still a decent amount of room for refactoring, but nothing significant enough
that it would need to be changed before this series is merged.

> ---
>  core/Makefile.inc         |   4 +-
>  core/pci-slot.c           | 203 +++++++++++++++++++++
>  core/pci.c                |  77 +++++---
>  core/pcie-slot.c          | 452
> ++++++++++++++++++++++++++++++++++++++++++++++
>  include/opal-api.h        |  10 +
>  include/pci-slot.h        | 254 ++++++++++++++++++++++++++
>  include/pci.h             |  66 ++-----
>  include/platform.h        |   1 +
>  platforms/ibm-fsp/lxvpd.c |   1 +
>  9 files changed, 988 insertions(+), 80 deletions(-)
>  create mode 100644 core/pci-slot.c
>  create mode 100644 core/pcie-slot.c
>  create mode 100644 include/pci-slot.h
> 
> diff --git a/core/Makefile.inc b/core/Makefile.inc
> index 5af0d7c..13b287c 100644
> --- a/core/Makefile.inc
> +++ b/core/Makefile.inc
> @@ -3,8 +3,8 @@
>  SUBDIRS += core
>  CORE_OBJS = relocate.o console.o stack.o init.o chip.o mem_region.o
>  CORE_OBJS += malloc.o lock.o cpu.o utils.o fdt.o opal.o interrupts.o
> -CORE_OBJS += timebase.o opal-msg.o pci.o pci-opal.o fast-reboot.o
> -CORE_OBJS += device.o exceptions.o trace.o affinity.o vpd.o
> +CORE_OBJS += timebase.o opal-msg.o pci.o pci-slot.o pcie-slot.o pci-opal.o
> +CORE_OBJS += fast-reboot.o device.o exceptions.o trace.o affinity.o vpd.o
>  CORE_OBJS += hostservices.o platform.o nvram.o nvram-format.o hmi.o
>  CORE_OBJS += console-log.o ipmi.o time-utils.o pel.o pool.o errorlog.o
>  CORE_OBJS += timer.o i2c.o rtc.o flash.o sensor.o ipmi-opal.o
> diff --git a/core/pci-slot.c b/core/pci-slot.c
> new file mode 100644
> index 0000000..f1416a8
> --- /dev/null
> +++ b/core/pci-slot.c
> @@ -0,0 +1,203 @@
> +/* Copyright 2013-2016 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 <opal-msg.h>
> +#include <pci-cfg.h>
> +#include <pci.h>
> +#include <pci-slot.h>
> +
> +/* Debugging options */
> +#define PCI_SLOT_PREFIX	"PCI-SLOT-%016llx "
> +#define PCI_SLOT_DBG(s, fmt, a...)		 \
> +	prlog(PR_DEBUG, PCI_SLOT_PREFIX fmt, (s)->id, ##a)
> +
> +static void pci_slot_prepare_link_change(struct pci_slot *slot, bool up)
> +{
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +	uint32_t aercap, mask;
> +
> +	/*
> +	 * Mask the link down and receiver error before the link becomes
> +	 * down. Otherwise, unmask the errors when the link is up.
> +	 */
> +	if (pci_has_cap(pd, PCIECAP_ID_AER, true)) {
> +		aercap = pci_cap(pd, PCIECAP_ID_AER, true);
> +
> +		/* Link down error */
> +		pci_cfg_read32(phb, pd->bdfn, aercap + PCIECAP_AER_UE_MASK,
> +			       &mask);
> +		if (up)
> +			mask &= ~PCIECAP_AER_UE_MASK_SURPRISE_DOWN;
> +		else
> +			mask |= PCIECAP_AER_UE_MASK_SURPRISE_DOWN;
> +		pci_cfg_write32(phb, pd->bdfn, aercap + PCIECAP_AER_UE_MASK,
> +				mask);
> +
> +		/* Receiver error */
> +		pci_cfg_read32(phb, pd->bdfn, aercap + PCIECAP_AER_CE_MASK,
> +			       &mask);
> +		if (up)
> +			mask &= ~PCIECAP_AER_CE_RECVR_ERR;
> +		else
> +			mask |= PCIECAP_AER_CE_RECVR_ERR;
> +		pci_cfg_write32(phb, pd->bdfn, aercap + PCIECAP_AER_CE_MASK,
> +				mask);
> +	}
> +
> +	/*
> +	 * We're coming back from reset. We need restore bus ranges
> +	 * and reinitialize the affected bridges and devices.
> +	 */
> +	if (up) {
> +		pci_restore_bridge_buses(phb, pd);
> +		if (phb->ops->device_init)
> +			pci_walk_dev(phb, pd, phb->ops->device_init, NULL);
> +	}
> +}
> +
> +static int64_t pci_slot_sm_poll(struct pci_slot *slot)

What's "sm" refer to here?

> +{
> +	uint64_t now = mftb();
> +	int64_t ret;
> +
> +	/* Return timeout value if we have */

Could be phrased better.  "Return remaining timeout if we're still waiting"
maybe?

> +	if (slot->delay_tgt_tb &&
> +	    tb_compare(now, slot->delay_tgt_tb) == TB_ABEFOREB)
> +		return slot->delay_tgt_tb - now;
> +
> +	slot->delay_tgt_tb = 0;
> +	switch (slot->state & PCI_SLOT_STATE_MASK) {
> +	case PCI_SLOT_STATE_LINK:
> +		ret = slot->ops.poll_link(slot);
> +		break;
> +	case PCI_SLOT_STATE_HRESET:
> +		ret = slot->ops.hreset(slot);
> +		break;
> +	case PCI_SLOT_STATE_FRESET:
> +		ret = slot->ops.freset(slot);
> +		break;
> +	case PCI_SLOT_STATE_PFRESET:
> +		ret = slot->ops.pfreset(slot);
> +		break;
> +	case PCI_SLOT_STATE_CRESET:
> +		ret = slot->ops.creset(slot);
> +		break;
> +	default:
> +		prlog(PR_ERR, PCI_SLOT_PREFIX
> +		      "Invalid state %08x\n", slot->id, slot->state);
> +		pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
> +		return OPAL_HARDWARE;
> +	}
> +
> +	return ret;
> +}
> +
> +void pci_slot_add_properties(struct pci_slot *slot,
> +			     struct dt_node *np)

Function name reads as if you're populating the pci_slot rather than the
dt_node.  Perhaps pci_slot_add_dt_properties()?

> +{
> +	/* Bail without device node */
> +	if (!np)
> +		return;
> +
> +	dt_add_property_cells(np, "ibm,reset-by-firmware", 1);
> +	dt_add_property_cells(np, "ibm,slot-pluggable", slot->pluggable);
> +	dt_add_property_cells(np, "ibm,slot-power-ctl", slot->power_ctl);
> +	dt_add_property_cells(np, "ibm,slot-power-led-ctlled",
> +			      slot->power_led_ctl);
> +	dt_add_property_cells(np, "ibm,slot-attn-led", slot->attn_led_ctl);
> +	dt_add_property_cells(np, "ibm,slot-connector-type",
> +			      slot->connector_type);
> +	dt_add_property_cells(np, "ibm,slot-card-desc", slot->card_desc);
> +	dt_add_property_cells(np, "ibm,slot-card-mech", slot->card_mech);
> +	dt_add_property_cells(np, "ibm,slot-wired-lanes", slot->wired_lanes);
> +
> +	if (slot->ops.add_properties)
> +		slot->ops.add_properties(slot, np);
> +}
> +
> +struct pci_slot *pci_slot_alloc(struct phb *phb,
> +				struct pci_device *pd)
> +{
> +	struct pci_slot *slot = NULL;
> +
> +	/*
> +	 * The function can be used to allocate either PHB slot or normal
> +	 * one. For both cases, the @phb should be always valid.
> +	 */
> +	if (!phb)
> +		return NULL;
> +
> +	/*
> +	 * When @pd is NULL, we're going to create a PHB slot. Otherwise,
> +	 * a normal slot will be created. Check if the specified slot has
> +	 * been existing or not.
> +	 */

"Check if the specified slot already exists or not"

> +	slot = pd ? pd->slot : phb->slot;
> +	if (slot) {
> +		prlog(PR_ERR, PCI_SLOT_PREFIX "Already existing\n", slot-
> >id);

"already exists"

> +		return slot;
> +	}
> +
> +	/* Allocate memory chunk */
> +	slot = zalloc(sizeof(struct pci_slot));
> +	if (!slot) {
> +		prlog(PR_ERR, "%s: Out of memory\n", __func__);
> +		return NULL;
> +	}
> +
> +	/* The polling function shouldn't be overrided by individual platform
> */

"The polling function sholdn't be overridden by individual platforms"

> +	slot->phb = phb;
> +	slot->pd = pd;
> +	pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
> +	slot->power_state = PCI_SLOT_POWER_ON;
> +	slot->ops.poll = pci_slot_sm_poll;
> +	slot->ops.prepare_link_change = pci_slot_prepare_link_change;
> +	if (!pd) {
> +		slot->id = PCI_PHB_SLOT_ID(phb);
> +		phb->slot = slot;
> +	} else {
> +		slot->id = PCI_SLOT_ID(phb, pd->bdfn);
> +		pd->slot = slot;
> +	}
> +
> +	return slot;
> +}
> +
> +struct pci_slot *pci_slot_find(uint64_t id)
> +{
> +	struct phb *phb;
> +	struct pci_device *pd;
> +	struct pci_slot *slot;
> +	uint64_t index;
> +	uint16_t bdfn;
> +
> +	index = PCI_SLOT_PHB_INDEX(id);
> +	phb = pci_get_phb(index);
> +
> +	/* PHB slot */
> +	if (!(id & PCI_SLOT_ID_PREFIX)) {
> +		slot = phb ? phb->slot : NULL;
> +		return slot;
> +	}
> +
> +	/* Normal PCI slot */
> +	bdfn = PCI_SLOT_BDFN(id);
> +	pd = phb ? pci_find_dev(phb, bdfn) : NULL;
> +	slot = pd ? pd->slot : NULL;
> +	return slot;
> +}
> diff --git a/core/pci.c b/core/pci.c
> index 91e45f3..5172698 100644
> --- a/core/pci.c
> +++ b/core/pci.c
> @@ -18,6 +18,7 @@
>  #include <cpu.h>
>  #include <pci.h>
>  #include <pci-cfg.h>
> +#include <pci-slot.h>
>  #include <timebase.h>
>  #include <device.h>
>  #include <fsp.h>
> @@ -452,11 +453,33 @@ static void pci_cleanup_bridge(struct phb *phb, struct
> pci_device *pd)
>  	pci_cfg_write16(phb, pd->bdfn, PCI_CFG_CMD, cmd);	
>  }
>  
> +/* Remove all subordinate PCI devices leading from the indicated
> + * PCI bus. It's used to remove all PCI devices behind one PCI
> + * slot at unplugging time
> + */
> +void pci_remove_bus(struct phb *phb, struct list_head *list)
> +{
> +	struct pci_device *pd, *tmp;
> +
> +	list_for_each_safe(list, pd, tmp, link) {
> +		pci_remove_bus(phb, &pd->children);
> +
> +		/* Release device node and PCI slot */
> +		if (pd->dn)
> +			dt_free(pd->dn);
> +		if (pd->slot)
> +			free(pd->slot);
> +
> +		/* Remove from parent list and release itself */
> +		list_del(&pd->link);
> +		free(pd);
> +	}
> +}
>  
> -/* pci_scan - Perform a recursive scan of the bus at bus_number
> - *            populating the list passed as an argument. This also
> - *            performs the bus numbering, so it returns the largest
> - *            bus number that was assigned.
> +/* Perform a recursive scan of the bus at bus_number populating
> + * the list passed as an argument. This also performs the bus
> + * numbering, so it returns the largest bus number that was
> + * assigned.
>   *
>   * Note: Eventually this might want to access some VPD information
>   *       in order to know what slots to scan and what not etc..
> @@ -466,9 +489,9 @@ static void pci_cleanup_bridge(struct phb *phb, struct
> pci_device *pd)
>   * XXX NOTE: We might also want to setup the PCIe MPS/MRSS properly
>   *           here as Linux may or may not do it
>   */
> -static uint8_t pci_scan(struct phb *phb, uint8_t bus, uint8_t max_bus,
> -			struct list_head *list, struct pci_device *parent,
> -			bool scan_downstream)
> +uint8_t pci_scan_bus(struct phb *phb, uint8_t bus, uint8_t max_bus,
> +		     struct list_head *list, struct pci_device *parent,
> +		     bool scan_downstream)
>  {
>  	struct pci_device *pd = NULL;
>  	uint8_t dev, fn, next_bus, max_sub, save_max;
> @@ -585,8 +608,8 @@ static uint8_t pci_scan(struct phb *phb, uint8_t bus,
> uint8_t max_bus,
>  
>  		/* Perform recursive scan */
>  		if (do_scan) {
> -			max_sub = pci_scan(phb, next_bus, max_bus,
> -					   &pd->children, pd, true);
> +			max_sub = pci_scan_bus(phb, next_bus, max_bus,
> +					       &pd->children, pd, true);
>  		} else if (!use_max) {
>  			/* XXX Empty bridge... we leave room for hotplug
>  			 * slots etc.. but we should be smarter at figuring
> @@ -794,7 +817,7 @@ static void pci_scan_phb(void *data)
>  	/* Scan root port and downstream ports if applicable */
>  	PCIDBG(phb, 0, "Scanning (upstream%s)...\n",
>  	       has_link ? "+downsteam" : " only");
> -	pci_scan(phb, 0, 0xff, &phb->devices, NULL, has_link);
> +	pci_scan_bus(phb, 0, 0xff, &phb->devices, NULL, has_link);
>  
>  	/* Configure MPS (Max Payload Size) for PCIe domain */
>  	pci_walk_dev(phb, NULL, pci_get_mps, &mps);
> @@ -1320,12 +1343,12 @@ static void pci_print_summary_line(struct phb *phb,
> struct pci_device *pd,
>  			  rev_class & 0xff, rev_class >> 8, cname, slotstr);
>  }
>  
> -
> -static void pci_add_one_node(struct phb *phb, struct pci_device *pd,
> -			     struct dt_node *parent_node,
> -			     struct pci_lsi_state *lstate, uint8_t swizzle)
> +static void pci_add_one_device_node(struct phb *phb,
> +				    struct pci_device *pd,
> +				    struct dt_node *parent_node,
> +				    struct pci_lsi_state *lstate,
> +				    uint8_t swizzle)
>  {
> -	struct pci_device *child;
>  	struct dt_node *np;
>  	const char *cname;
>  #define MAX_NAME 256
> @@ -1440,14 +1463,14 @@ static void pci_add_one_node(struct phb *phb, struct
> pci_device *pd,
>  	 * Instead add a ranges property that explicitly translates 1:1.
>  	 */
>  	dt_add_property(np, "ranges", ranges_direct, sizeof(ranges_direct));
> -
> -	list_for_each(&pd->children, child, link)
> -		pci_add_one_node(phb, child, np, lstate, swizzle);
>  }
>  
> -static void pci_add_nodes(struct phb *phb)
> +void pci_add_device_nodes(struct phb *phb,
> +			  struct list_head *list,
> +			  struct dt_node *parent_node,
> +			  struct pci_lsi_state *lstate,
> +			  uint8_t swizzle)
>  {
> -	struct pci_lsi_state *lstate = &phb->lstate;
>  	struct pci_device *pd;
>  
>  	/* If the PHB has its own slot info, add them */
> @@ -1455,8 +1478,15 @@ static void pci_add_nodes(struct phb *phb)
>  		pci_add_slot_properties(phb, phb->slot_info, NULL);
>  
>  	/* Add all child devices */
> -	list_for_each(&phb->devices, pd, link)
> -		pci_add_one_node(phb, pd, phb->dt_node, lstate, 0);
> +	list_for_each(list, pd, link) {
> +		pci_add_one_device_node(phb, pd, parent_node,
> +					lstate, swizzle);
> +		if (list_empty(&pd->children))
> +			continue;
> +
> +		pci_add_device_nodes(phb, &pd->children,
> +				     pd->dn, lstate, swizzle);
> +	}
>  }
>  
>  static void __pci_reset(struct list_head *list)
> @@ -1537,7 +1567,8 @@ void pci_init_slots(void)
>  	for (i = 0; i < ARRAY_SIZE(phbs); i++) {
>  		if (!phbs[i])
>  			continue;
> -		pci_add_nodes(phbs[i]);
> +		pci_add_device_nodes(phbs[i], &phbs[i]->devices,
> +				     phbs[i]->dt_node, &phbs[i]->lstate, 0);
>  	}
>  
>  	/* PHB final fixup */
> diff --git a/core/pcie-slot.c b/core/pcie-slot.c
> new file mode 100644
> index 0000000..4783dbe
> --- /dev/null
> +++ b/core/pcie-slot.c
> @@ -0,0 +1,452 @@
> +/* Copyright 2013-2016 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 <opal-msg.h>
> +#include <pci-cfg.h>
> +#include <pci.h>
> +#include <pci-slot.h>
> +
> +/* Debugging options */
> +#define PCIE_SLOT_PREFIX	"PCIE-SLOT-%016llx "
> +#define PCIE_SLOT_DBG(s, fmt, a...)		  \
> +	prlog(PR_DEBUG, PCIE_SLOT_PREFIX fmt, (s)->id, ##a)
> +
> +static int64_t pcie_slot_get_presence_state(struct pci_slot *slot, uint8_t
> *val)
> +{
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +	uint32_t ecap;
> +	uint16_t state;
> +
> +	/* The presence is always on if it's switch upstream port */

	/* The presence is always on if it's a switch upstream port */

> +	if (pd->dev_type == PCIE_TYPE_SWITCH_UPPORT) {
> +		*val = OPAL_PCI_SLOT_PRESENT;
> +		return OPAL_SUCCESS;
> +	}
> +
> +	/*
> +	 * The presence is always on if switch downstream port doesn't

	 * The presence is always on if a switch downstream port doesn't

> +	 * support slot capability according to PCIE spec.
> +	 */
> +	if (pd->dev_type == PCIE_TYPE_SWITCH_DNPORT &&
> +	    !(slot->slot_cap & PCICAP_EXP_CAP_SLOT)) {
> +		*val = OPAL_PCI_SLOT_PRESENT;
> +		return OPAL_SUCCESS;
> +	}
> +
> +	/* Retrieve presence status */
> +	ecap = pci_cap(pd, PCI_CFG_CAP_ID_EXP, false);
> +	pci_cfg_read16(phb, pd->bdfn, ecap + PCICAP_EXP_SLOTSTAT, &state);
> +	if (state & PCICAP_EXP_SLOTSTAT_PDETECTST)
> +		*val = OPAL_PCI_SLOT_PRESENT;
> +	else
> +		*val = OPAL_PCI_SLOT_EMPTY;
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +static int64_t pcie_slot_get_link_state(struct pci_slot *slot,
> +					uint8_t *val)
> +{
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +	uint32_t ecap;
> +	int16_t state;
> +
> +	/*
> +	 * The link behind switch upstream port is always on
> +	 * since it doesn't have valid link indicator.

	 * since it doesn't have a valid link indicator.

> +	 */
> +	if (pd->dev_type == PCIE_TYPE_SWITCH_UPPORT) {
> +		*val = 1;
> +		return OPAL_SUCCESS;
> +	}
> +
> +	/* Retrieve link width */
> +	ecap = pci_cap(pd, PCI_CFG_CAP_ID_EXP, false);
> +	pci_cfg_read16(phb, pd->bdfn, ecap + PCICAP_EXP_LSTAT, &state);
> +	if (state & PCICAP_EXP_LSTAT_DLLL_ACT)
> +		*val = ((state & PCICAP_EXP_LSTAT_WIDTH) >> 4);
> +	else
> +		*val = 0;
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +static int64_t pcie_slot_get_power_state(struct pci_slot *slot __unused,
> +					 uint8_t *val)
> +{
> +	/* The power is always on if no functionality is supported */
> +	if (!(slot->slot_cap & PCICAP_EXP_SLOTCAP_PWCTRL))
> +		*val = PCI_SLOT_POWER_ON;
> +	else
> +		*val = slot->power_state;
> +	return OPAL_SUCCESS;
> +}
> +
> +static int64_t pcie_slot_get_attention_state(struct pci_slot *slot,
> +					     uint8_t *val)
> +{
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +	uint32_t ecap;
> +	uint16_t state;
> +
> +	/* Attention is off if the capability is missed */

Should probably be "missing" instead of "missed"

> +	if (!(slot->slot_cap & PCICAP_EXP_SLOTCAP_ATTNI)) {
> +		*val = 0;
> +		return OPAL_SUCCESS;
> +	}
> +
> +	/* Retrieve attention state */
> +	ecap = pci_cap(pd, PCI_CFG_CAP_ID_EXP, false);
> +	pci_cfg_read16(phb, pd->bdfn, ecap + PCICAP_EXP_SLOTCTL, &state);
> +	state = (state & PCICAP_EXP_SLOTCTL_ATTNI) >> 6;
> +	switch (state) {
> +	case PCIE_INDIC_ON:
> +		*val = PCI_SLOT_ATTN_LED_ON;
> +		break;
> +	case PCIE_INDIC_BLINK:
> +		*val = PCI_SLOT_ATTN_LED_BLINK;
> +		break;
> +	case PCIE_INDIC_OFF:
> +	default:
> +		*val = PCI_SLOT_ATTN_LED_OFF;
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +static int64_t pcie_slot_get_latch_state(struct pci_slot *slot,
> +					 uint8_t *val)
> +{
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +	uint32_t ecap;
> +	uint16_t state;
> +
> +	/* Latch is off if MRL sensor doesn't exist */
> +	if (!(slot->slot_cap & PCICAP_EXP_SLOTCAP_MRLSENS)) {
> +		*val = 0;
> +		return OPAL_SUCCESS;
> +	}
> +
> +	/* Retrieve MRL sensor state */
> +	ecap = pci_cap(pd, PCI_CFG_CAP_ID_EXP, false);
> +	pci_cfg_read16(phb, pd->bdfn, ecap + PCICAP_EXP_SLOTSTAT, &state);
> +	if (state & PCICAP_EXP_SLOTSTAT_MRLSENSST)
> +		*val = 1;
> +	else
> +		*val = 0;
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +static int64_t pcie_slot_set_attention_state(struct pci_slot *slot,
> +					     uint8_t val)
> +{
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +	uint32_t ecap;
> +	uint16_t state;
> +
> +	/* Drop the request if functionality doesn't exist */
> +	if (!(slot->slot_cap & PCICAP_EXP_SLOTCAP_ATTNI))
> +		return OPAL_SUCCESS;
> +
> +	/* Update with the requested state */
> +	ecap = pci_cap(pd, PCI_CFG_CAP_ID_EXP, false);
> +	pci_cfg_read16(phb, pd->bdfn, ecap + PCICAP_EXP_SLOTCTL, &state);
> +	state &= ~PCICAP_EXP_SLOTCTL_ATTNI;
> +	switch (val) {
> +	case PCI_SLOT_ATTN_LED_ON:
> +		state |= (PCIE_INDIC_ON << 6);
> +		break;
> +	case PCI_SLOT_ATTN_LED_BLINK:
> +		state |= (PCIE_INDIC_BLINK << 6);
> +		break;
> +	case PCI_SLOT_ATTN_LED_OFF:
> +		state |= (PCIE_INDIC_OFF << 6);
> +		break;
> +	default:
> +		prlog(PR_ERR, PCIE_SLOT_PREFIX
> +		      "Invalid attention state (0x%x)\n", slot->id, val);
> +		return OPAL_PARAMETER;
> +	}
> +
> +	pci_cfg_write16(phb, pd->bdfn, ecap + PCICAP_EXP_SLOTCTL, state);
> +	return OPAL_SUCCESS;
> +}
> +
> +static int64_t pcie_slot_set_power_state(struct pci_slot *slot, uint8_t val)
> +{
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +	uint32_t ecap;
> +	uint16_t state;
> +
> +	/* Drop the request if functionality doesn't exist */
> +	if (!(slot->slot_cap & PCICAP_EXP_SLOTCAP_PWCTRL))
> +		return OPAL_SUCCESS;
> +
> +	if (slot->power_state == val)
> +		return OPAL_SUCCESS;
> +
> +	pci_slot_set_state(slot, PCI_SLOT_STATE_SPOWER_START);
> +	slot->power_state = val;
> +	ecap = pci_cap(pd, PCI_CFG_CAP_ID_EXP, false);
> +	pci_cfg_read16(phb, pd->bdfn, ecap + PCICAP_EXP_SLOTCTL, &state);
> +	state &= ~(PCICAP_EXP_SLOTCTL_PWRCTLR | PCICAP_EXP_SLOTCTL_PWRI);
> +	switch (val) {
> +	case PCI_SLOT_POWER_OFF:
> +		state |= (PCICAP_EXP_SLOTCTL_PWRCTLR | (PCIE_INDIC_OFF <<
> 8));
> +		break;
> +	case PCI_SLOT_POWER_ON:
> +		state |= (PCIE_INDIC_ON << 8);
> +		break;
> +	default:
> +		pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
> +		prlog(PR_ERR, PCIE_SLOT_PREFIX
> +		      "Invalid power state (0x%x)\n", slot->id, val);
> +		return OPAL_PARAMETER;
> +	}
> +
> +	pci_cfg_write16(phb, pd->bdfn, ecap + PCICAP_EXP_SLOTCTL, state);
> +	pci_slot_set_state(slot, PCI_SLOT_STATE_SPOWER_DONE);
> +
> +	return OPAL_ASYNC_COMPLETION;
> +}
> +
> +static int64_t pcie_slot_sm_poll_link(struct pci_slot *slot)
> +{
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +	uint32_t ecap = pci_cap(pd, PCI_CFG_CAP_ID_EXP, false);
> +	uint16_t val;
> +	uint8_t presence = 0;
> +
> +	switch (slot->state) {
> +	case PCI_SLOT_STATE_LINK_START_POLL:
> +		PCIE_SLOT_DBG(slot, "LINK: Start polling\n");
> +
> +		/* Link is down for ever without devices attached */
> +		if (slot->ops.get_presence_state)
> +			slot->ops.get_presence_state(slot, &presence);
> +		if (!presence) {
> +			PCIE_SLOT_DBG(slot, "LINK: No adapter, end
> polling\n");
> +			pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
> +			return OPAL_SUCCESS;
> +		}
> +
> +		/* Enable the link without check */
> +		pci_cfg_read16(phb, pd->bdfn, ecap + PCICAP_EXP_LCTL, &val);
> +		val &= ~PCICAP_EXP_LCTL_LINK_DIS;
> +		pci_cfg_write16(phb, pd->bdfn, ecap + PCICAP_EXP_LCTL, val);
> +
> +		/*
> +		 * If the link change report isn't supported, we expect
> +		 * the link is up and stabilized after one second.
> +		 */
> +		if (!(slot->link_cap & PCICAP_EXP_LCAP_DL_ACT_REP)) {
> +			pci_slot_set_state(slot,
> +					   PCI_SLOT_STATE_LINK_DELAY_FINALIZE
> D);
> +			return pci_slot_set_sm_timeout(slot, secs_to_tb(1));
> +		}
> +
> +		/*
> +		 * Poll the link state if link state change report is
> +		 * supported on the link.
> +		 */
> +		pci_slot_set_state(slot, PCI_SLOT_STATE_LINK_POLLING);
> +		slot->retries = 250;

This retry count should probably be a #define somewhere.

> +		return pci_slot_set_sm_timeout(slot, msecs_to_tb(20));
> +	case PCI_SLOT_STATE_LINK_DELAY_FINALIZED:
> +		PCIE_SLOT_DBG(slot, "LINK: No link report, end polling\n");
> +		if (slot->ops.prepare_link_change)
> +			slot->ops.prepare_link_change(slot, true);
> +		pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
> +		return OPAL_SUCCESS;
> +	case PCI_SLOT_STATE_LINK_POLLING:
> +		pci_cfg_read16(phb, pd->bdfn, ecap + PCICAP_EXP_LSTAT, &val);
> +		if (val & PCICAP_EXP_LSTAT_DLLL_ACT) {
> +			PCIE_SLOT_DBG(slot, "LINK: Link is up, end
> polling\n");
> +			if (slot->ops.prepare_link_change)
> +				slot->ops.prepare_link_change(slot, true);
> +			pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
> +			return OPAL_SUCCESS;
> +		}
> +
> +		/* Check link state again until timeout */
> +		if (slot->retries-- == 0) {

This seems a bit dodgy...

> +			prlog(PR_ERR, PCIE_SLOT_PREFIX
> +			      "LINK: Timeout waiting for up (%04x)\n",
> +			      slot->id, val);
> +			pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
> +			return OPAL_SUCCESS;
> +		}
> +
> +		return pci_slot_set_sm_timeout(slot, msecs_to_tb(20));
> +	default:
> +		prlog(PR_ERR, PCIE_SLOT_PREFIX
> +		      "Link: Unexpected slot state %08x\n",
> +		      slot->id, slot->state);
> +	}
> +
> +	pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
> +	return OPAL_HARDWARE;
> +}
> +
> +static void pcie_slot_reset(struct pci_slot *slot, bool assert)
> +{
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +	uint16_t ctl;
> +
> +	pci_cfg_read16(phb, pd->bdfn, PCI_CFG_BRCTL, &ctl);
> +	if (assert)
> +		ctl |= PCI_CFG_BRCTL_SECONDARY_RESET;
> +	else
> +		ctl &= ~PCI_CFG_BRCTL_SECONDARY_RESET;
> +	pci_cfg_write16(phb, pd->bdfn, PCI_CFG_BRCTL, ctl);
> +}
> +
> +static int64_t pcie_slot_sm_hreset(struct pci_slot *slot)
> +{
> +	switch (slot->state) {
> +	case PCI_SLOT_STATE_NORMAL:
> +		PCIE_SLOT_DBG(slot, "HRESET: Starts\n");
> +		if (slot->ops.prepare_link_change) {
> +			PCIE_SLOT_DBG(slot, "HRESET: Prepare for link
> down\n");
> +			slot->ops.prepare_link_change(slot, false);
> +		}
> +		/* fall through */
> +	case PCI_SLOT_STATE_HRESET_START:
> +		PCIE_SLOT_DBG(slot, "HRESET: Assert\n");
> +		pcie_slot_reset(slot, true);
> +		pci_slot_set_state(slot, PCI_SLOT_STATE_HRESET_HOLD);
> +		return pci_slot_set_sm_timeout(slot, msecs_to_tb(250));
> +	case PCI_SLOT_STATE_HRESET_HOLD:
> +		PCIE_SLOT_DBG(slot, "HRESET: Deassert\n");
> +		pcie_slot_reset(slot, false);
> +		pci_slot_set_state(slot, PCI_SLOT_STATE_LINK_START_POLL);
> +		return pci_slot_set_sm_timeout(slot, msecs_to_tb(1800));
> +	default:
> +		PCIE_SLOT_DBG(slot, "HRESET: Unexpected slot state %08x\n",
> +			      slot->state);
> +	}
> +
> +	pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
> +	return OPAL_HARDWARE;
> +}
> +
> +/*
> + * Usually, individual platforms need override the power
> + * management methods for fundamental reset because the
> + * hot reset is commonly shared.
> + */

Perhaps:

/* 
 * Usually, individual platforms need to override the power
 * management methods for fundamental reset because the
 * hot reset method is commonly shared.
 */

> +static int64_t pcie_slot_sm_freset(struct pci_slot *slot)
> +{
> +	uint8_t power_state = PCI_SLOT_POWER_ON;
> +
> +	switch (slot->state) {
> +	case PCI_SLOT_STATE_NORMAL:
> +		PCIE_SLOT_DBG(slot, "FRESET: Starts\n");
> +		if (slot->ops.prepare_link_change)
> +			slot->ops.prepare_link_change(slot, false);
> +
> +		/* Retrieve power state */
> +		if (slot->ops.get_power_state) {
> +			PCIE_SLOT_DBG(slot, "FRESET: Retrieve power
> state\n");
> +			slot->ops.get_power_state(slot, &power_state);
> +		}
> +
> +		/* In power on state, power it off */
> +		if (power_state == PCI_SLOT_POWER_ON &&
> +		    slot->ops.set_power_state) {
> +			PCIE_SLOT_DBG(slot, "FRESET: Power is on, turn
> off\n");
> +			slot->ops.set_power_state(slot, PCI_SLOT_POWER_OFF);
> +			pci_slot_set_state(slot,
> +				PCI_SLOT_STATE_FRESET_POWER_OFF);
> +			return pci_slot_set_sm_timeout(slot,
> msecs_to_tb(50));
> +		}
> +		/* No power state change, fall through */
> +	case PCI_SLOT_STATE_FRESET_POWER_OFF:
> +		PCIE_SLOT_DBG(slot, "FRESET: Power is off, turn on\n");
> +		if (slot->ops.set_power_state)
> +			slot->ops.set_power_state(slot, PCI_SLOT_POWER_ON);
> +		pci_slot_set_state(slot, PCI_SLOT_STATE_HRESET_START);
> +		return pci_slot_set_sm_timeout(slot, msecs_to_tb(50));
> +	default:
> +		prlog(PR_ERR, PCIE_SLOT_PREFIX
> +		      "FRESET: Unexpected slot state %08x\n",
> +		      slot->id, slot->state);
> +	}
> +
> +	pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
> +	return OPAL_HARDWARE;
> +}
> +
> +struct pci_slot *pcie_slot_create(struct phb *phb, struct pci_device *pd)
> +{
> +	struct pci_slot *slot;
> +	uint32_t ecap;
> +
> +	/* Allocate PCI slot */
> +	slot = pci_slot_alloc(phb, pd);
> +	if (!slot)
> +		return NULL;
> +
> +	/* Cache the link and slot capabilities */
> +	if (pd) {
> +		ecap = pci_cap(pd, PCI_CFG_CAP_ID_EXP, false);
> +		pci_cfg_read32(phb, pd->bdfn, ecap + PCICAP_EXP_LCAP,
> +			       &slot->link_cap);
> +		pci_cfg_read32(phb, pd->bdfn, ecap + PCICAP_EXP_SLOTCAP,
> +			       &slot->slot_cap);
> +	}
> +
> +	/* Slot info */

Should probably change this comment to something else so as not to confuse
things with the pci_slot_info struct

> +	if ((slot->slot_cap & PCICAP_EXP_SLOTCAP_HPLUG_SURP) &&
> +	    (slot->slot_cap & PCICAP_EXP_SLOTCAP_HPLUG_CAP))
> +		slot->pluggable = 1;
> +	if (slot->slot_cap & PCICAP_EXP_SLOTCAP_PWCTRL)
> +		slot->power_ctl = 1;
> +	if (slot->slot_cap & PCICAP_EXP_SLOTCAP_PWRI)
> +		slot->power_led_ctl = PCI_SLOT_PWR_LED_CTL_KERNEL;
> +	if (slot->slot_cap & PCICAP_EXP_SLOTCAP_ATTNI)
> +		slot->attn_led_ctl = PCI_SLOT_ATTN_LED_CTL_KERNEL;
> +	slot->wired_lanes = ((slot->link_cap & PCICAP_EXP_LCAP_MAXWDTH) >>
> 4);
> +	/* Standard slot operations */
> +	slot->ops.get_presence_state  = pcie_slot_get_presence_state;
> +	slot->ops.get_link_state      = pcie_slot_get_link_state;
> +	slot->ops.get_power_state     = pcie_slot_get_power_state;
> +	slot->ops.get_attention_state = pcie_slot_get_attention_state;
> +	slot->ops.get_latch_state     = pcie_slot_get_latch_state;
> +	slot->ops.set_power_state     = pcie_slot_set_power_state;
> +	slot->ops.set_attention_state = pcie_slot_set_attention_state;
> +
> +	/*
> +	 * SM based reset stuff. The poll function is always
> +	 * unified for all cases.
> +	 */

Probably a good place to expand on what SM is.

> +	slot->ops.poll_link             = pcie_slot_sm_poll_link;
> +	slot->ops.hreset                = pcie_slot_sm_hreset;
> +	slot->ops.freset                = pcie_slot_sm_freset;
> +	slot->ops.pfreset               = NULL;
> +
> +	return slot;
> +}
> diff --git a/include/opal-api.h b/include/opal-api.h
> index 41af0e5..a81d870 100644
> --- a/include/opal-api.h
> +++ b/include/opal-api.h
> @@ -376,6 +376,16 @@ enum OpalPciMaskAction {
>  	OPAL_MASK_ERROR_TYPE = 1
>  };
>  
> +enum OpalPciSlotPresentenceState {

Should be "OpalPciSlotPresenceState" or even just "OpalPciSlotPresence".

> +	OPAL_PCI_SLOT_EMPTY	= 0,
> +	OPAL_PCI_SLOT_PRESENT	= 1
> +};
> +
> +enum OpalPciSlotPowerState {
> +	OPAL_PCI_SLOT_POWER_OFF	= 0,
> +	OPAL_PCI_SLOT_POWER_ON	= 1
> +};
> +
>  enum OpalSlotLedType {
>  	OPAL_SLOT_LED_TYPE_ID = 0,	/* IDENTIFY LED */
>  	OPAL_SLOT_LED_TYPE_FAULT = 1,	/* FAULT LED */

In v10 you defined a new opal_msg_type called OPAL_MSG_PCI_HOTPLUG, is there a
reason that went away?

> diff --git a/include/pci-slot.h b/include/pci-slot.h
> new file mode 100644
> index 0000000..cc64b0e
> --- /dev/null
> +++ b/include/pci-slot.h
> @@ -0,0 +1,254 @@
> +/* Copyright 2013-2016 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.
> + */
> +
> +#ifndef __PCI_SLOT_H
> +#define __PCI_SLOT_H
> +
> +#include <opal.h>
> +#include <device.h>
> +#include <timebase.h>
> +#include <timer.h>
> +#include <ccan/list/list.h>
> +
> +/*
> + * PCI Slot Info: Wired Lane Values
> + *
> + * Values 0 to 6 match slot map 1005. In case of *any* change here
> + * make sure to keep the lxvpd.c parsing code in sync *and* the
> + * corresponding label strings in pci.c
> + */
> +#define PCI_SLOT_WIRED_LANES_UNKNOWN	0x00
> +#define PCI_SLOT_WIRED_LANES_PCIE_X1	0x01
> +#define PCI_SLOT_WIRED_LANES_PCIE_X2	0x02
> +#define PCI_SLOT_WIRED_LANES_PCIE_X4	0x03
> +#define PCI_SLOT_WIRED_LANES_PCIE_X8	0x04
> +#define PCI_SLOT_WIRED_LANES_PCIE_X16	0x05
> +#define PCI_SLOT_WIRED_LANES_PCIE_X32	0x06
> +#define PCI_SLOT_WIRED_LANES_PCIX_32	0x07
> +#define PCI_SLOT_WIRED_LANES_PCIX_64	0x08
> +
> +/* PCI Slot Info: Bus Clock Values */
> +#define PCI_SLOT_BUS_CLK_RESERVED	0x00
> +#define PCI_SLOT_BUS_CLK_GEN_1		0x01
> +#define PCI_SLOT_BUS_CLK_GEN_2		0x02
> +#define PCI_SLOT_BUS_CLK_GEN_3		0x03
> +
> +/* PCI Slot Info: Connector Type Values */
> +#define PCI_SLOT_CONNECTOR_PCIE_EMBED	0x00
> +#define PCI_SLOT_CONNECTOR_PCIE_X1	0x01
> +#define PCI_SLOT_CONNECTOR_PCIE_X2	0x02
> +#define PCI_SLOT_CONNECTOR_PCIE_X4	0x03
> +#define PCI_SLOT_CONNECTOR_PCIE_X8	0x04
> +#define PCI_SLOT_CONNECTOR_PCIE_X16	0x05
> +#define PCI_SLOT_CONNECTOR_PCIE_NS	0x0E	/* Non-Standard */
> +
> +/* PCI Slot Info: Card Description Values */
> +#define PCI_SLOT_DESC_NON_STANDARD	0x00	/* Embed/Non-
> Standard       */
> +#define PCI_SLOT_DESC_PCIE_FH_FL	0x00	/* Full Height, Full
> Length */
> +#define PCI_SLOT_DESC_PCIE_FH_HL	0x01	/* Full Height, Half
> Length */
> +#define PCI_SLOT_DESC_PCIE_HH_FL	0x02	/* Half Height, Full
> Length */
> +#define PCI_SLOT_DESC_PCIE_HH_HL	0x03	/* Half Height, Half
> Length */
> +
> +/* PCI Slot Info: Mechanicals Values */
> +#define PCI_SLOT_MECH_NONE		0x00
> +#define PCI_SLOT_MECH_RIGHT		0x01
> +#define PCI_SLOT_MECH_LEFT		0x02
> +#define PCI_SLOT_MECH_RIGHT_LEFT	0x03
> +
> +/* PCI Slot Info: Power LED Control Values */
> +#define PCI_SLOT_PWR_LED_CTL_NONE	0x00	/* No Control        */
> +#define PCI_SLOT_PWR_LED_CTL_FSP	0x01	/* FSP Controlled    */
> +#define PCI_SLOT_PWR_LED_CTL_KERNEL	0x02	/* Kernel Controlled
> */
> +
> +/* PCI Slot Info: ATTN LED Control Values */
> +#define PCI_SLOT_ATTN_LED_CTL_NONE	0x00	/* No Control        */
> +#define PCI_SLOT_ATTN_LED_CTL_FSP	0x01	/* FSP Controlled    */
> +#define PCI_SLOT_ATTN_LED_CTL_KERNEL	0x02	/* Kernel Controlled
> */
> +
> +/* Attention LED */
> +#define PCI_SLOT_ATTN_LED_OFF		0
> +#define PCI_SLOT_ATTN_LED_ON		1
> +#define PCI_SLOT_ATTN_LED_BLINK		2
> +
> +/* Power state */
> +#define PCI_SLOT_POWER_OFF		0
> +#define PCI_SLOT_POWER_ON		1
> +
> +/*
> + * We have hard and soft reset for slot. Hard reset requires
> + * power-off and then power-on, but soft reset only resets
> + * secondary bus.
> + */
> +struct pci_slot;
> +struct pci_slot_ops {
> +	/* For slot management */
> +	int64_t (*get_presence_state)(struct pci_slot *slot, uint8_t *val);
> +	int64_t (*get_link_state)(struct pci_slot *slot, uint8_t *val);
> +	int64_t (*get_power_state)(struct pci_slot *slot, uint8_t *val);
> +	int64_t (*get_attention_state)(struct pci_slot *slot, uint8_t *val);
> +	int64_t (*get_latch_state)(struct pci_slot *slot, uint8_t *val);
> +	int64_t (*set_power_state)(struct pci_slot *slot, uint8_t val);
> +	int64_t (*set_attention_state)(struct pci_slot *slot, uint8_t val);
> +
> +	/* SM based functions for reset */
> +	void (*prepare_link_change)(struct pci_slot *slot, bool is_up);
> +	int64_t (*poll_link)(struct pci_slot *slot);
> +	int64_t (*creset)(struct pci_slot *slot);
> +	int64_t (*freset)(struct pci_slot *slot);
> +	int64_t (*pfreset)(struct pci_slot *slot);
> +	int64_t (*hreset)(struct pci_slot *slot);
> +	int64_t (*poll)(struct pci_slot *slot);
> +
> +	/* Auxillary functions */
> +	void (*add_properties)(struct pci_slot *slot, struct dt_node *np);
> +};
> +
> +/*
> + * The PCI slot state is split up into base and number. With this
> + * design, the individual platforms can introduce their own PCI
> + * slot states with addition to the base. Eventually, the base
> + * state can be recognized by PCI slot core.
> + */
> +#define PCI_SLOT_STATE_MASK			0xFFFFFF00
> +#define PCI_SLOT_STATE_NORMAL			0x00000000
> +#define PCI_SLOT_STATE_LINK			0x00000100
> +#define   PCI_SLOT_STATE_LINK_START_POLL	0x00000101
> +#define   PCI_SLOT_STATE_LINK_DELAY_FINALIZED	0x00000102
> +#define   PCI_SLOT_STATE_LINK_POLLING		0x00000103
> +#define PCI_SLOT_STATE_HRESET			0x00000200
> +#define   PCI_SLOT_STATE_HRESET_START		0x00000201
> +#define   PCI_SLOT_STATE_HRESET_HOLD		0x00000202
> +#define PCI_SLOT_STATE_FRESET			0x00000300
> +#define   PCI_SLOT_STATE_FRESET_POWER_OFF	0x00000301
> +#define PCI_SLOT_STATE_PFRESET			0x00000400
> +#define   PCI_SLOT_STATE_PFRESET_START		0x00000401
> +#define PCI_SLOT_STATE_CRESET			0x00000500
> +#define   PCI_SLOT_STATE_CRESET_START		0x00000501
> +#define PCI_SLOT_STATE_GPOWER			0x00000600
> +#define   PCI_SLOT_STATE_GPOWER_START		0x00000601
> +#define PCI_SLOT_STATE_SPOWER			0x00000700
> +#define   PCI_SLOT_STATE_SPOWER_START		0x00000701
> +#define   PCI_SLOT_STATE_SPOWER_DONE		0x00000702
> +#define PCI_SLOT_STATE_GPRESENCE		0x00000800
> +#define   PCI_SLOT_STATE_GPRESENCE_START	0x00000801
> +
> +
> +struct pci_slot {
> +	uint32_t		flags;
> +#define PCI_SLOT_FLAG_BOOTUP		0x1
> +
> +	struct phb		*phb;
> +	struct pci_device	*pd;
> +
> +	/* Identifier */
> +	uint64_t		id;
> +	struct timer		timer;
> +	uint64_t		async_token;
> +	uint8_t			power_state;
> +
> +	/* Slot information */
> +	uint8_t			pluggable;
> +	uint8_t			power_ctl;
> +	uint8_t			power_led_ctl;
> +	uint8_t			attn_led_ctl;
> +	uint8_t			connector_type;
> +	uint8_t			card_desc;
> +	uint8_t			card_mech;
> +	uint8_t			wired_lanes;
> +
> +	/* State machine */
> +	uint32_t		state;
> +	uint32_t		retry_state;
> +	uint32_t		link_cap;
> +	uint32_t		slot_cap;
> +	uint64_t		delay_tgt_tb;

"delay_tgt_tb"? Unlike the other values, I don't think this one is particularly
self-explanatory.  It'd be good to have a comment here given it's not especially
clear what it is when it's set, either.

> +	uint64_t		retries;
> +	struct pci_slot_ops	ops;
> +	void			*data;
> +};
> +
> +#define PCI_SLOT_ID_PREFIX	0x8000000000000000
> +#define PCI_SLOT_ID(phb, bdfn)	\
> +	(PCI_SLOT_ID_PREFIX | ((uint64_t)(bdfn) << 16) | (phb)->opal_id)
> +#define PCI_PHB_SLOT_ID(phb)	((phb)->opal_id)
> +#define PCI_SLOT_PHB_INDEX(id)	((id) & 0xfffful)
> +#define PCI_SLOT_BDFN(id)	(((id) >> 16) & 0xfffful)
> +
> +static inline uint32_t pci_slot_add_flags(struct pci_slot *slot,
> +					  uint32_t flags)
> +{
> +	uint32_t old = 0;
> +
> +	if (slot) {
> +		old = slot->flags;
> +		slot->flags |= flags;
> +	}
> +
> +	return old;
> +}
> +
> +static inline bool pci_slot_has_flags(struct pci_slot *slot,
> +				      uint32_t flags)
> +{
> +	if (!slot)
> +		return false;
> +
> +	if ((slot->flags & flags) == flags)
> +		return true;
> +
> +	return false;
> +}
> +
> +static inline uint32_t pci_slot_remove_flags(struct pci_slot *slot,
> +					     uint32_t flags)
> +{
> +	uint32_t old = 0;
> +
> +	if (slot) {
> +		old = slot->flags;
> +		slot->flags &= ~flags;
> +	}
> +
> +	return old;
> +}
> +
> +static inline void pci_slot_set_state(struct pci_slot *slot, uint32_t state)
> +{
> +	if (slot)
> +		slot->state = state;
> +}
> +
> +static inline uint64_t pci_slot_set_sm_timeout(struct pci_slot *slot,
> +					       uint64_t dur)
> +{
> +	uint64_t target, now = mftb();
> +
> +	target = now + dur;
> +	if (target == 0)
> +		target++;
> +	slot->delay_tgt_tb = target;

What's going on here?

> +
> +	return dur;
> +}
> +
> +extern struct pci_slot *pci_slot_alloc(struct phb *phb,
> +				       struct pci_device *pd);
> +extern struct pci_slot *pcie_slot_create(struct phb *phb,
> +					 struct pci_device *pd);
> +extern void pci_slot_add_properties(struct pci_slot *slot,
> +				    struct dt_node *np);
> +extern struct pci_slot *pci_slot_find(uint64_t id);
> +#endif /* __PCI_SLOT_H */
> diff --git a/include/pci.h b/include/pci.h
> index 3481ed4..11d6126 100644
> --- a/include/pci.h
> +++ b/include/pci.h
> @@ -22,60 +22,6 @@
>  #include <lock.h>
>  #include <ccan/list/list.h>
>  
> -/* PCI Slot Info: Wired Lane Values
> - *
> - * Values 0 to 6 match slot map 1005. In case of *any* change here
> - * make sure to keep the lxvpd.c parsing code in sync *and* the
> - * corresponding label strings in pci.c
> - */
> -#define PCI_SLOT_WIRED_LANES_UNKNOWN   0x00
> -#define PCI_SLOT_WIRED_LANES_PCIE_X1   0x01
> -#define PCI_SLOT_WIRED_LANES_PCIE_X2   0x02
> -#define PCI_SLOT_WIRED_LANES_PCIE_X4   0x03
> -#define PCI_SLOT_WIRED_LANES_PCIE_X8   0x04
> -#define PCI_SLOT_WIRED_LANES_PCIE_X16  0x05
> -#define PCI_SLOT_WIRED_LANES_PCIE_X32  0x06
> -#define PCI_SLOT_WIRED_LANES_PCIX_32   0x07
> -#define PCI_SLOT_WIRED_LANES_PCIX_64   0x08
> -
> -/* PCI Slot Info: Bus Clock Values */
> -#define PCI_SLOT_BUS_CLK_RESERVED      0x00
> -#define PCI_SLOT_BUS_CLK_GEN_1         0x01
> -#define PCI_SLOT_BUS_CLK_GEN_2         0x02
> -#define PCI_SLOT_BUS_CLK_GEN_3         0x03
> -
> -/* PCI Slot Info: Connector Type Values */
> -#define PCI_SLOT_CONNECTOR_PCIE_EMBED  0x00
> -#define PCI_SLOT_CONNECTOR_PCIE_X1     0x01
> -#define PCI_SLOT_CONNECTOR_PCIE_X2     0x02
> -#define PCI_SLOT_CONNECTOR_PCIE_X4     0x03
> -#define PCI_SLOT_CONNECTOR_PCIE_X8     0x04
> -#define PCI_SLOT_CONNECTOR_PCIE_X16    0x05
> -#define PCI_SLOT_CONNECTOR_PCIE_NS     0x0E  /* Non-Standard */
> -
> -/* PCI Slot Info: Card Description Values */
> -#define PCI_SLOT_DESC_NON_STANDARD     0x00 /* Embed/Non-Standard Connector
> */
> -#define PCI_SLOT_DESC_PCIE_FH_FL       0x00 /* Full Height, Full Length */
> -#define PCI_SLOT_DESC_PCIE_FH_HL       0x01 /* Full Height, Half Length */
> -#define PCI_SLOT_DESC_PCIE_HH_FL       0x02 /* Half Height, Full Length */
> -#define PCI_SLOT_DESC_PCIE_HH_HL       0x03 /* Half Height, Half Length */
> -
> -/* PCI Slot Info: Mechanicals Values */
> -#define PCI_SLOT_MECH_NONE             0x00
> -#define PCI_SLOT_MECH_RIGHT            0x01
> -#define PCI_SLOT_MECH_LEFT             0x02
> -#define PCI_SLOT_MECH_RIGHT_LEFT       0x03
> -
> -/* PCI Slot Info: Power LED Control Values */
> -#define PCI_SLOT_PWR_LED_CTL_NONE      0x00 /* No Control        */
> -#define PCI_SLOT_PWR_LED_CTL_FSP       0x01 /* FSP Controlled    */
> -#define PCI_SLOT_PWR_LED_CTL_KERNEL    0x02 /* Kernel Controlled */
> -
> -/* PCI Slot Info: ATTN LED Control Values */
> -#define PCI_SLOT_ATTN_LED_CTL_NONE     0x00 /* No Control        */
> -#define PCI_SLOT_ATTN_LED_CTL_FSP      0x01 /* FSP Controlled    */
> -#define PCI_SLOT_ATTN_LED_CTL_KERNEL   0x02 /* Kernel Controlled */
> -
>  /* PCI Slot Entry Information */
>  struct pci_slot_info {
>  	char       label[16];
> @@ -151,6 +97,7 @@ struct pci_device {
>  	struct list_head	pcrf;
>  
>  	struct dt_node		*dn;
> +	struct pci_slot		*slot;
>  	struct pci_slot_info    *slot_info;
>  	struct pci_device	*parent;
>  	struct list_head	children;
> @@ -468,6 +415,7 @@ struct phb {
>  	uint32_t		mps;
>  
>  	/* PCI-X only slot info, for PCI-E this is in the RC bridge */
> +	struct pci_slot		*slot;
>  	struct pci_slot_info    *slot_info;

In both the pci_device and phb structs, I don't see why we need separate
pci_slot and pci_slot_info structs; logically, they should be merged together.
This is something that can be changed after this series is merged, I think.

Do you see any reason why we should have to separate structs here?

>  
>  	/* Base location code used to generate the children one */
> @@ -525,7 +473,15 @@ static inline int64_t pci_cfg_write32(struct phb *phb,
> uint32_t bdfn,
>  }
>  
>  /* Utilities */
> -
> +extern void pci_remove_bus(struct phb *phb, struct list_head *list);
> +extern uint8_t pci_scan_bus(struct phb *phb, uint8_t bus, uint8_t max_bus,
> +			    struct list_head *list, struct pci_device
> *parent,
> +			    bool scan_downstream);
> +extern void pci_add_device_nodes(struct phb *phb,
> +				 struct list_head *list,
> +				 struct dt_node *parent_node,
> +				 struct pci_lsi_state *lstate,
> +				 uint8_t swizzle);
>  extern int64_t pci_find_cap(struct phb *phb, uint16_t bdfn, uint8_t cap);
>  extern int64_t pci_find_ecap(struct phb *phb, uint16_t bdfn, uint16_t cap,
>  			     uint8_t *version);
> diff --git a/include/platform.h b/include/platform.h
> index f1bdc30..d07994f 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -20,6 +20,7 @@
>  /* Some fwd declarations for types used further down */
>  struct phb;
>  struct pci_device;
> +struct pci_slot;
>  struct errorlog;
>  
>  enum resource_id {
> diff --git a/platforms/ibm-fsp/lxvpd.c b/platforms/ibm-fsp/lxvpd.c
> index 90c9e09..542c49a 100644
> --- a/platforms/ibm-fsp/lxvpd.c
> +++ b/platforms/ibm-fsp/lxvpd.c
> @@ -24,6 +24,7 @@
>  #include <vpd.h>
>  #include <pci.h>
>  #include <pci-cfg.h>
> +#include <pci-slot.h>
>  
>  #include "lxvpd.h"
>  



More information about the Skiboot mailing list