Re: [PATCH v2 2/3] PCI: brcmstb: CLKREQ# accomodations of downstream device

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Apr 14, 2023 at 01:33:29PM -0700, Florian Fainelli wrote:
> On 4/14/23 13:27, Bjorn Helgaas wrote:
> > On Tue, Apr 11, 2023 at 12:59:17PM -0400, Jim Quinlan wrote:
> ...

> > > The HW+driver is able to tell us when mode (a) mode is needed.  But there
> > > is no easy way to tell if L1SS mode should be configured.  In certain
> > > situations, getting this wrong may cause a panic during boot time.  So we
> > > rely on the DT prop "brcm,enable-l1ss" to tell us when mode (c) is desired.
> > > Using this mode only makes sense when the downstream device is L1SS-capable
> > > and the OS has been configured to activate L1SS
> > > (e.g. policy==powersupersave).
> > 
> > I'm really concerned about the user experience here.  I assume users
> > do not want to edit the DT based on what device they plug in.  They
> > shouldn't need to (and probably won't) know whether the device
> > supports L1SS.
> > 
> > I hate kernel/module parameters, but I think even that would be better
> > then having to edit the DT.
> 
> The problem I see with kernel/module parameters is that it is really hard to
> differentiate whether they should be applied across all instances of the
> device/drivers or just for one in particular.
> 
> The Raspberry Pi 4 is a single pcie-brcmstb instance, but we have other
> systems supported by that driver on Set-top-box and Cable Modem chips for
> instance where we may have different types of end-points being connected,
> some of which could be Multi-chip-module (MCM) where everything is known
> ahead of time, and sometimes cards that are plugged to full-sized PCIe or
> mini-PCIe connectors, where some amount of runtime discoverability is
> involved.
> 
> Without inventing some custom modular parameter syntax, it may not work that
> well. The Device Tree properties at least easily allow for per-instance
> configuration.

We do have this already (from
Documentation/admin-guide/kernel-parameters.txt):

        pci=option[,option...]  [PCI] various PCI subsystem options.

                                Some options herein operate on a specific device
                                or a set of devices (<pci_dev>). These are
                                specified in one of the following formats:

                                [<domain>:]<bus>:<dev>.<func>[/<dev>.<func>]*
                                pci:<vendor>:<device>[:<subvendor>:<subdevice>]

                                Note: the first format specifies a PCI
                                bus/device/function address which may change
                                if new hardware is inserted, if motherboard
                                firmware changes, or due to changes caused
                                by other kernel parameters. If the
                                domain is left unspecified, it is
                                taken to be zero. Optionally, a path
                                to a device through multiple device/function
                                addresses can be specified after the base
                                address (this is more robust against
                                renumbering issues).  The second format
                                selects devices using IDs from the
                                configuration space which may match multiple
                                devices in the system.