[Skiboot] [PATCH] activate shared PCI slot support on witherspoon

[Michael Neuling]

> [PATCH] activate shared PCI slot support on witherspoon

Subject should start with a capital letter and also have a prefix.  

I'd suggest:
  phb4: Activate shared PCI slot on witherspoon

On Mon, 2017-05-22 at 20:49 +0200, Frederic Barrat wrote:
> Witherspoon systems come with a 'shared' PCI slot: it looks like a x16
> slot, but it's actually two x8 connected to two PHBs of two different
> chips. Taking advantage of it requires some logic on the PCI
> adapter. Only the Mellanox CX5 adapter is known to support it at the
> time of this writing.
> 
> This patch enables support for the shared slot on witherspoon. It
> looks for the presence bits from the 2 PHBs and activate sharing
> through a gpio setup.

What happens if you plug in a x8 or some other PCIe device?

If you can document the various configurations so we have something to point
people to when this becomes a support issue. x8 in this x16 slot.  x16 that
doesn't understand this shared stuff in this slot? CX5 in this slot. Nothing in
this slot?

> Note that there's no easy way to be sure that the card is indeed a
> shared-slot compatible PCI adapter and not a normal x16 card. Plugging
> a normal x16 adapter on the shared slot should be avoided on
> witherspoon, as the link won't train on the second slot. Link training
> results in a timeout and a longer boot time.
> 
> Signed-off-by: Frederic Barrat <fbarrat at linux.vnet.ibm.com>
> ---
>  core/init.c                    |  2 ++
>  hw/phb4.c                      | 62
> +++++++++++++++++++++++++++++++++++++++++-
>  include/platform.h             |  5 ++++
>  include/skiboot.h              |  1 +
>  include/xscom-p9-regs.h        |  3 ++
>  platforms/astbmc/witherspoon.c |  6 ++++
>  6 files changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/core/init.c b/core/init.c
> index dce10fd6..54c13a16 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -1018,6 +1018,8 @@ void __noreturn __nomcount main_cpu_entry(const void
> *fdt)
>  	probe_npu();
>  	probe_npu2();
>  
> +	if (platform.pre_pci_fixup)
> +		platform.pre_pci_fixup();

Do this this check in a wrapper function to keep this function cleaner..

> 	/* Initialize PCI */
>  	pci_init_slots();
>  
> diff --git a/hw/phb4.c b/hw/phb4.c
> index 318d934d..2843e6d7 100644
> --- a/hw/phb4.c
> +++ b/hw/phb4.c
> @@ -77,6 +77,66 @@ static void phb4_init_hw(struct phb4 *p, bool first_init);
>  #define PHBLOGCFG(p, fmt, a...) do {} while (0)
>  #endif
>  
> +static int get_opal_id(unsigned int chip_id, unsigned int index)
> +{
> +	return chip_id * 6 + index;

6?

> +}
> +
> +static void phb4_activate_shared_slot(struct proc_chip *chip)

This should be phb4_activate_shared_slot_witherspoon?

What is this function actually doing?  There needs to be some documentation
here.

> +{
> +	uint64_t val;
> +
> +	xscom_read(chip->id, P9_GPIO_DATA_OUT_ENABLE, &val);
> +	val |= PPC_BIT(2);
> +	xscom_write(chip->id, P9_GPIO_DATA_OUT_ENABLE, val);
> +
> +	xscom_read(chip->id, P9_GPIO_DATA_OUT, &val);
> +	val |= PPC_BIT(2);
> +	xscom_write(chip->id, P9_GPIO_DATA_OUT, val);
> +	time_wait_us(100);
> +	prlog(PR_INFO, "Shared PCI slot activated\n");
> +}
> +
> +void phb4_pre_pci_fixup_witherspoon(void)
> +{
> +	struct pci_slot *slot0, *slot1;
> +	struct proc_chip *chip0, *chip1;
> +	uint8_t p0 = 0, p1 = 0;
> +
> +	/*
> +	 * Detect if a card (Mellanox CX5) is present on the

Why have the CX5 comment here?  None of this seems to be CX5 specific. 

> +	 * shared slot and do some extra configuration if it is.
> +	 *
> +	 * The shared slot, a.k.a "Slot 2", is connected to
> +	 * PEC2 phb index 3 on both chips.
> +	 *
> +	 * One issue is that if the presence bits of the 2 slots are set,
> +	 * we don't really know if it is a CX5 card or a 'normal' x16 PCI
> +	 * card. So we activate the shared slot when we see a card in the
> +	 * 2 slots. If it's not a Mellanox CX5, it doesn't hurt.
> +	 *
> +	 * On the 2nd slot, the link won't train for a normal x16 card, the
> +	 * procedure will timeout, thus adding some delay to the boot time.
> +	 */
> +	chip0 = next_chip(NULL);
> +	slot0 = pci_slot_find(get_opal_id(chip0->id, 3));
> +	chip1 = next_chip(chip0);
> +	slot1 = pci_slot_find(get_opal_id(chip1->id, 3));
> +	if (next_chip(chip1)) {
> +		prlog(PR_WARNING,
> +			"Unexpected number of chips, skipping shared slot
> detection\n");
> +		return;
> +	}
> +	if (slot0 && slot1) {
> +		if (slot0->ops.get_presence_state)
> +			slot0->ops.get_presence_state(slot0, &p0);
> +		if (slot1->ops.get_presence_state)
> +			slot1->ops.get_presence_state(slot1, &p1);
> +		if (p0 == 1 && p1 == 1)
> +			phb4_activate_shared_slot(chip1);
> +	}
> +}
> +
>  /* Note: The "ASB" name is historical, practically this means access via
>   * the XSCOM backdoor
>   */
> @@ -3284,7 +3344,7 @@ static void phb4_create(struct dt_node *np)
>  	/* We register the PHB before we initialize it so we
>  	 * get a useful OPAL ID for it
>  	 */
> -	pci_register_phb(&p->phb, p->chip_id * 6 + p->index); //6 PHBs per
> chip?

This "6" comment should be above?

> +	pci_register_phb(&p->phb, get_opal_id(p->chip_id, p->index));
>  
>  	/* Create slot structure */
>  	slot = phb4_slot_create(&p->phb);
> diff --git a/include/platform.h b/include/platform.h
> index 91332042..4dcdb335 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -101,6 +101,11 @@ struct platform {
>  	void		(*pci_setup_phb)(struct phb *phb, unsigned int
> index);
>  
>  	/*
> +	 * This is called before resetting the PHBs (lift PERST) and
> +	 * probing the devices. The PHBs have already been initialized.
> +	 */
> +	void		(*pre_pci_fixup)(void);
> +	/*
>  	 * Called during PCI scan for each device. For bridges, this is
>  	 * called before its children are probed. This is called for
>  	 * every device and for the PHB itself with a NULL pd though
> diff --git a/include/skiboot.h b/include/skiboot.h
> index 5c8b0c85..14953cab 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -218,6 +218,7 @@ extern int phb3_preload_capp_ucode(void);
>  extern void phb3_preload_vpd(void);
>  extern int phb4_preload_capp_ucode(void);
>  extern void phb4_preload_vpd(void);
> +extern void phb4_pre_pci_fixup_witherspoon(void);
>  extern void probe_npu(void);
>  extern void probe_npu2(void);
>  extern void uart_init(void);
> diff --git a/include/xscom-p9-regs.h b/include/xscom-p9-regs.h
> index 7da404ba..4738e812 100644
> --- a/include/xscom-p9-regs.h
> +++ b/include/xscom-p9-regs.h
> @@ -18,4 +18,7 @@
>  #define P9X_EX_NCU_DARN_BAR			0x11011
>  #define  P9X_EX_NCU_DARN_BAR_EN			PPC_BIT(0)
>  
> +#define P9_GPIO_DATA_OUT_ENABLE			0x00000000000B0054ull
> +#define P9_GPIO_DATA_OUT			0x00000000000B0051ull
> +
>  #endif /* __XSCOM_P9_REGS_H__ */
> diff --git a/platforms/astbmc/witherspoon.c b/platforms/astbmc/witherspoon.c
> index abaa7c9b..083e7cff 100644
> --- a/platforms/astbmc/witherspoon.c
> +++ b/platforms/astbmc/witherspoon.c
> @@ -38,10 +38,16 @@ static bool witherspoon_probe(void)
>  	return true;
>  }
>  
> +static void witherspoon_pre_pci_fixup(void)
> +{
> +	phb4_pre_pci_fixup_witherspoon();
> +}
> +
>  DECLARE_PLATFORM(witherspoon_platform) = {
>  	.name			= "Witherspoon",
>  	.probe			= witherspoon_probe,
>  	.init			= astbmc_init,
> +	.pre_pci_fixup		= witherspoon_pre_pci_fixup,
>  	.start_preload_resource	= flash_start_preload_resource,
>  	.resource_loaded	= flash_resource_loaded,
>  	.bmc			= NULL, /* FIXME: Add openBMC */