Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3549 bytes --]

On Thu, Aug 29, 2019 at 04:58:20PM +0200, Thierry Reding wrote:

> However, the same PCI controller can also be used with onboard devices
> where no standard 3.3 V and 12 V need to be supplied (as in the PCIe
> slot case above). Instead there are usually different power supplies
> to power the device. Also, since these supplies are usually required to
> bring the device up and make it detectable on the PCI bus, these
> supplies are typically always on. Also, since the PCI controller is not
> the consumer of the power supplies, it's impossible to specify which
> supplies exactly are needed by the device (they'd have to be described
> by a binding for these devices, but drivers couldn't be requesting them
> because without the supplies being enabled the device would never show
> up on the PCI bus and the driver would never bind).

These on board devices that might have non-standard supplies are more of
a problem as you say.  This is a general problem with enumerable buses
that get deployed in embedded contexts, things like clocks and GPIOs can
also be required for enumeration.  Ideally we'd have some pre-probe
callback that could sort these things out but that's core device model
work I've never found the time/enthusiasm to work on.  IIRC there is
already a PCI binding for DT so I guess we could do something like say
it's up to the driver for the PCI device and just call probe() even
without enumeration and require the device to power things up but that
feels very messy.

> Do you think those 3.3 V and 12 V regulators would qualify for use of
> the regulator_get_optional() API? Because in the case where the PCIe
> controller doesn't drive a PCIe slot, the 3.3 V and 12 V supplies really
> are not there.

It doesn't fill me with joy.  I think the main thing is working out
where to attach the supply.

The cleanest and most idiomatic thing from a regulator perspective for
the physical slots would be for the supplies to be attached to the PCI
slot rather than the controller in the DT, even though they will get
driven by the controller driver in practice.  This would mean that
controllers would have optional slot objects with mandatory regulators,
the controller would then have to cope with powering the slot up before
enumerating it but would be able to power it off if nothing's plugged in
and save a bit of power, it also copes with the case where individual
slots have separately controllable power.  I'm not sure how this plays
more generally but since the slots are a physical thing you can point to
on the board it doesn't seem unreasonable.  Every time I see these
supplies attached to the controller for a bus it always feels a bit
wrong, especially in interactions with soldered down devices.

That feels cleaner than saying that the controller has a couple of
optional supplies, it plays a bit better with soldered down devices and
with slots having distinct supplies and it generally feels a bit more
consistent.  I'm not super sure I'm *happy* with this approach though,
it feels a bit awkward with the device model.

> > When I say users shouldn't be using this API I mean _get_optional()
> > specifically.

> True, even if the regulator is specified as optional in the bindings,
> using regulator_get() would effectively make it optional for the driver
> given that it returns the dummy.

Unless the hardware can genuinely cope with there being literally no
power supply there (as opposed to some fixed voltage thing) it probably
shouldn't be specifying the supply as optional in the bindings either :/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]