Re: [PATCH v2 RESEND 1/1] PCI: pcie_bus_config can be set at build time

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Hans, Niklas]

On Mon, Sep 28, 2020 at 03:46:51PM -0400, Jim Quinlan wrote:
> The Kconfig is modified so that the pcie_bus_config setting can be done at
> build time in the same manner as the CONFIG_PCIEASPM_XXXX choice.  The
> pci_bus_config setting may still be overridden by the bootline param.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

We merged this as b0e85c3c8554 ("PCI: Add Kconfig options for MPS/MRRS
strategy"), which appeared in v5.10.

In retrospect, I think this might have been a mistake because it
forces a build-time configuration for something that may not be known
at build time and can be set via command-line parameter.

But I can't find any discussion about it.  Did you have a use case
where command line parameters weren't usable?

If there's a platform that requires one of these settings, maybe
there's a way to do that programmatically in the host bridge driver
rather than using a Kconfig symbol.

> ---
>  drivers/pci/Kconfig | 56 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.c   | 12 ++++++++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 4bef5c2bae9f..15ce948858fb 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -187,6 +187,62 @@ config PCI_HYPERV
>  	  The PCI device frontend driver allows the kernel to import arbitrary
>  	  PCI devices from a PCI backend to support PCI driver domains.
>  
> +choice
> +	prompt "PCIE default bus config setting"
> +	default PCIE_BUS_DEFAULT
> +	depends on PCI
> +	help
> +	  One of the following choices will set the pci_bus_config at
> +	  compile time.  The choices offered are the same as those offered
> +	  for the bootline parameter 'pci'; i.e. 'pci=pcie_bus_tune_off',
> +	  'pci=pcie_bus_safe', 'pci=pcie_bus_perf', and 'pci=pcie_bus_peer2peer'.
> +	  This is a compile-time setting and is still be overridden by the
> +	  above bootline parameters, if present.  If unsure, chose PCIE_BUS_DEFAULT.
> +
> +config PCIE_BUS_TUNE_OFF
> +	bool "Tune Off"
> +	depends on PCI
> +	help
> +	  Use the BIOS defaults; doesn't touch MPS at all.  This is the same
> +	  as booting with 'pci=pcie_bus_tune_off'.
> +
> +config PCIE_BUS_DEFAULT
> +	bool "Default"
> +	depends on PCI
> +	help
> +	  Default choice; ensures that the MPS matches upstream bridge.
> +
> +config PCIE_BUS_SAFE
> +	bool "Safe"
> +	depends on PCI
> +	help
> +	  Use largest MPS that boot-time devices support.  If you have a
> +	  closed system with no possibility of adding new devices,
> +	  this will use the largest MPS that's supported by all devices.
> +	  This is the same as booting with 'pci=pcie_bus_safe'.
> +
> +config PCIE_BUS_PERFORMANCE
> +	bool "Performance"
> +	depends on PCI
> +	help
> +	  Use MPS and MRRS for best performance.  This setting ensures
> +	  that a given device's MPS is no larger than its parent MPS,
> +	  which allows us to keep all switches/bridges to the max MPS supported
> +	  by their parent and eventually the PHB.  This is the same as
> +	  booting with 'pci=pcie_bus_perf'.
> +
> +config PCIE_BUS_PEER2PEER
> +	bool "Peer2peer"
> +	depends on PCI
> +	help
> +	  Set MPS = 128 for all devices.  MPS configuration effected by
> +	  the other options could cause the MPS on one root port to be
> +	  different than that of the MPS on another.  Simply making the system
> +	  wide MPS be set to the smallest possible value (128B) solves
> +	  this issue.  This is the same as booting with 'pci=pcie_bus_peer2peer'.
> +
> +endchoice
> +
>  source "drivers/pci/hotplug/Kconfig"
>  source "drivers/pci/controller/Kconfig"
>  source "drivers/pci/endpoint/Kconfig"
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e39c5499770f..dfb52ed4a931 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -101,7 +101,19 @@ unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
>  #define DEFAULT_HOTPLUG_BUS_SIZE	1
>  unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
>  
> +
> +/* PCIE bus config, can be overridden by bootline param */
> +#ifdef CONFIG_PCIE_BUS_TUNE_OFF
> +enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_TUNE_OFF;
> +#elif defined CONFIG_PCIE_BUS_SAFE
> +enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_SAFE;
> +#elif defined CONFIG_PCIE_BUS_PERFORMANCE
> +enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_PERFORMANCE;
> +#elif defined CONFIG_PCIE_BUS_PEER2PEER
> +enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_PEER2PEER;
> +#else
>  enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_DEFAULT;
> +#endif
>  
>  /*
>   * The default CLS is used if arch didn't set CLS explicitly and not
> -- 
> 2.17.1
>