On Fri, Oct 22, 2021 at 10:06:57AM -0400, Jim Quinlan wrote:
> +static const char * const supplies[] = {
> + "vpcie3v3-supply",
> + "vpcie3v3aux-supply",
> + "brcm-ep-a-supply",
> + "brcm-ep-b-supply",
> +};
Why are you including "-supply" in the names here? That will lead to
a double -supply when we look in the DT which probably isn't what you're
looking for.
Also are you *sure* that the device has supplies with names like
"brcm-ep-a"? That seems rather unidiomatic for electrical engineering,
the names here are supposed to correspond to the names used in the
datasheet for the part.
> + /* This is for Broadcom STB/CM chips only */
> + if (pcie->type == BCM2711)
> + return 0;
It is a relief that other chips have managed to work out how to avoid
requiring power.
On Fri, Oct 22, 2021 at 10:31 AM Mark Brown <[email protected]> wrote:
>
> On Fri, Oct 22, 2021 at 10:06:57AM -0400, Jim Quinlan wrote:
>
> > +static const char * const supplies[] = {
> > + "vpcie3v3-supply",
> > + "vpcie3v3aux-supply",
> > + "brcm-ep-a-supply",
> > + "brcm-ep-b-supply",
> > +};
>
> Why are you including "-supply" in the names here? That will lead to
> a double -supply when we look in the DT which probably isn't what you're
> looking for.
I'm not sure how this got past testing; will fix.
>
> Also are you *sure* that the device has supplies with names like
> "brcm-ep-a"? That seems rather unidiomatic for electrical engineering,
> the names here are supposed to correspond to the names used in the
> datasheet for the part.
I try to explain this in the commit message of"PCI: allow for callback
to prepare nascent subdev". Wrt to the names,
"These regulators typically govern the actual power supply to the
endpoint chip. Sometimes they may be a the official PCIe socket
power -- such as 3.3v or aux-3.3v. Sometimes they are truly
the regulator(s) that supply power to the EP chip."
Each different SOC./board we deal with may present different ways of
making the EP device power on. We are using
an abstraction name "brcm-ep-a" to represent some required regulator
to make the EP work for a specific board. The RC
driver cannot hard code a descriptive name as it must work for all
boards designed by us, others, and third parties.
The EP driver also doesn't know or care about the regulator name, and
this driver is often closed source and often immutable. The EP
device itself may come from Brcm, a third party, or sometimes a competitor.
Basically, we find using a generic name such as "brcm-ep-a-supply"
quite handy and many of our customers embrace this feature.
I know that Rob was initially against such a generic name, but I
vaguely remember him seeing some merit to this, perhaps a tiny bit :-)
Or my memory is shot, which could very well be the case.
>
> > + /* This is for Broadcom STB/CM chips only */
> > + if (pcie->type == BCM2711)
> > + return 0;
>
> It is a relief that other chips have managed to work out how to avoid
> requiring power.
I'm not sure that the other Broadcom groups have our customers, our
customers' requirements, and the amount and variation of boards that
run our PCIe driver on the SOC.
Jim
On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
> Each different SOC./board we deal with may present different ways of
> making the EP device power on. We are using
> an abstraction name "brcm-ep-a" to represent some required regulator
> to make the EP work for a specific board. The RC
> driver cannot hard code a descriptive name as it must work for all
> boards designed by us, others, and third parties.
> The EP driver also doesn't know or care about the regulator name, and
> this driver is often closed source and often immutable. The EP
> device itself may come from Brcm, a third party, or sometimes a competitor.
> Basically, we find using a generic name such as "brcm-ep-a-supply"
> quite handy and many of our customers embrace this feature.
> I know that Rob was initially against such a generic name, but I
> vaguely remember him seeing some merit to this, perhaps a tiny bit :-)
> Or my memory is shot, which could very well be the case.
That sounds like it just shouldn't be a regulator at all, perhaps the
board happens to need a regulator there but perhaps it needs a clock,
GPIO or some specific sequence of actions. It sounds like you need some
sort of quirking mechanism to cope with individual boards with board
specific bindings.
I'd suggest as a first pass omitting this and then looking at some
actual systems later when working out how to support them, no sense in
getting the main thing held up by difficult edge cases.
> > > + /* This is for Broadcom STB/CM chips only */
> > > + if (pcie->type == BCM2711)
> > > + return 0;
> > It is a relief that other chips have managed to work out how to avoid
> > requiring power.
> I'm not sure that the other Broadcom groups have our customers, our
> customers' requirements, and the amount and variation of boards that
> run our PCIe driver on the SOC.
Sure, but equally they might (even if they didn't spot it yet) and in
general it's safer to err on the side of describing the hardware so we
can use that information later.
On Fri, Oct 22, 2021 at 3:47 PM Mark Brown <[email protected]> wrote:
>
> On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
>
> > Each different SOC./board we deal with may present different ways of
> > making the EP device power on. We are using
> > an abstraction name "brcm-ep-a" to represent some required regulator
> > to make the EP work for a specific board. The RC
> > driver cannot hard code a descriptive name as it must work for all
> > boards designed by us, others, and third parties.
> > The EP driver also doesn't know or care about the regulator name, and
> > this driver is often closed source and often immutable. The EP
> > device itself may come from Brcm, a third party, or sometimes a competitor.
>
> > Basically, we find using a generic name such as "brcm-ep-a-supply"
> > quite handy and many of our customers embrace this feature.
> > I know that Rob was initially against such a generic name, but I
> > vaguely remember him seeing some merit to this, perhaps a tiny bit :-)
> > Or my memory is shot, which could very well be the case.
>
> That sounds like it just shouldn't be a regulator at all, perhaps the
> board happens to need a regulator there but perhaps it needs a clock,
> GPIO or some specific sequence of actions. It sounds like you need some
> sort of quirking mechanism to cope with individual boards with board
> specific bindings.
The boards involved may have no PCIe sockets, or run the gamut of the different
PCIe sockets. They all offer gpio(s) to turn off/on their power supply(s) to
make their PCIe device endpoint functional. It is not viable to add
new Linux quirk or DT
code for each board. First is the volume and variety of the boards
that use our SOCs.. Second, is
our lack of information/control: often, the board is designed by one
company (not us), and
given to another company as the middleman, and then they want the
features outlined
in my aforementioned commit message.
>
> I'd suggest as a first pass omitting this and then looking at some
> actual systems later when working out how to support them, no sense in
> getting the main thing held up by difficult edge cases.
These are not edge cases -- some of these are major customers.
Regards,
Jim
>
> > > > + /* This is for Broadcom STB/CM chips only */
> > > > + if (pcie->type == BCM2711)
> > > > + return 0;
>
> > > It is a relief that other chips have managed to work out how to avoid
> > > requiring power.
>
> > I'm not sure that the other Broadcom groups have our customers, our
> > customers' requirements, and the amount and variation of boards that
> > run our PCIe driver on the SOC.
>
> Sure, but equally they might (even if they didn't spot it yet) and in
> general it's safer to err on the side of describing the hardware so we
> can use that information later.
On Mon, Oct 25, 2021 at 09:50:09AM -0400, Jim Quinlan wrote:
> On Fri, Oct 22, 2021 at 3:47 PM Mark Brown <[email protected]> wrote:
> > On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
> > That sounds like it just shouldn't be a regulator at all, perhaps the
> > board happens to need a regulator there but perhaps it needs a clock,
> > GPIO or some specific sequence of actions. It sounds like you need some
> > sort of quirking mechanism to cope with individual boards with board
> > specific bindings.
> The boards involved may have no PCIe sockets, or run the gamut of the different
> PCIe sockets. They all offer gpio(s) to turn off/on their power supply(s) to
> make their PCIe device endpoint functional. It is not viable to add
> new Linux quirk or DT
> code for each board. First is the volume and variety of the boards
> that use our SOCs.. Second, is
> our lack of information/control: often, the board is designed by one
> company (not us), and
> given to another company as the middleman, and then they want the
> features outlined
> in my aforementioned commit message.
Other vendors have plenty of variation in board design yet we still have
device trees that describe the hardware, I can't see why these systems
should be so different. It is entirely normal for system integrators to
collaborate on this and even upstream their own code, this happens all
the time, there is no need for everything to be implemented directly the
SoC vendor.
If there are generic quirks that match a common pattern seen in a lot of
board then properties can be defined for those, this is in fact the
common case. This is no reason to just shove in some random thing that
doesn't describe the hardware, that's a great way to end up stuck with
an ABI that is fragile and difficult to understand or improve.
Potentially some of these things should be being handled in the bindings
and drivers drivers for the relevant PCI devices rather than in the PCI
controller.
> > I'd suggest as a first pass omitting this and then looking at some
> > actual systems later when working out how to support them, no sense in
> > getting the main thing held up by difficult edge cases.
> These are not edge cases -- some of these are major customers.
I'm trying to help you progress the driver by decoupling things which
are causing difficulty from the simple parts so that we don't need to
keep looking at the simple bits over and over again. If these systems
are very common or familiar then it should be fairly easy to describe
the common patterns in what they're doing.
In any case whatever gets done needs to be documented in the bindings
documents.
On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
> On Fri, Oct 22, 2021 at 10:31 AM Mark Brown <[email protected]> wrote:
> >
> > On Fri, Oct 22, 2021 at 10:06:57AM -0400, Jim Quinlan wrote:
> >
> > > +static const char * const supplies[] = {
> > > + "vpcie3v3-supply",
> > > + "vpcie3v3aux-supply",
> > > + "brcm-ep-a-supply",
> > > + "brcm-ep-b-supply",
> > > +};
> >
> > Why are you including "-supply" in the names here? That will lead to
> > a double -supply when we look in the DT which probably isn't what you're
> > looking for.
> I'm not sure how this got past testing; will fix.
>
> >
> > Also are you *sure* that the device has supplies with names like
> > "brcm-ep-a"? That seems rather unidiomatic for electrical engineering,
> > the names here are supposed to correspond to the names used in the
> > datasheet for the part.
> I try to explain this in the commit message of"PCI: allow for callback
> to prepare nascent subdev". Wrt to the names,
>
> "These regulators typically govern the actual power supply to the
> endpoint chip. Sometimes they may be a the official PCIe socket
> power -- such as 3.3v or aux-3.3v. Sometimes they are truly
> the regulator(s) that supply power to the EP chip."
>
> Each different SOC./board we deal with may present different ways of
> making the EP device power on. We are using
> an abstraction name "brcm-ep-a" to represent some required regulator
> to make the EP work for a specific board. The RC
> driver cannot hard code a descriptive name as it must work for all
> boards designed by us, others, and third parties.
> The EP driver also doesn't know or care about the regulator name, and
> this driver is often closed source and often immutable. The EP
> device itself may come from Brcm, a third party, or sometimes a competitor.
>
> Basically, we find using a generic name such as "brcm-ep-a-supply"
> quite handy and many of our customers embrace this feature.
> I know that Rob was initially against such a generic name, but I
> vaguely remember him seeing some merit to this, perhaps a tiny bit :-)
> Or my memory is shot, which could very well be the case.
I don't recall being in favor of this. If you've got standard rails
(3.3V and 12V), then I'm fine with standard properties for them with or
without a slot.
Rob
On 10/25/21 7:58 AM, Mark Brown wrote:
> On Mon, Oct 25, 2021 at 09:50:09AM -0400, Jim Quinlan wrote:
>> On Fri, Oct 22, 2021 at 3:47 PM Mark Brown <[email protected]> wrote:
>>> On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
>
>>> That sounds like it just shouldn't be a regulator at all, perhaps the
>>> board happens to need a regulator there but perhaps it needs a clock,
>>> GPIO or some specific sequence of actions. It sounds like you need some
>>> sort of quirking mechanism to cope with individual boards with board
>>> specific bindings.
>
>> The boards involved may have no PCIe sockets, or run the gamut of the different
>> PCIe sockets. They all offer gpio(s) to turn off/on their power supply(s) to
>> make their PCIe device endpoint functional. It is not viable to add
>> new Linux quirk or DT
>> code for each board. First is the volume and variety of the boards
>> that use our SOCs.. Second, is
>> our lack of information/control: often, the board is designed by one
>> company (not us), and
>> given to another company as the middleman, and then they want the
>> features outlined
>> in my aforementioned commit message.
>
> Other vendors have plenty of variation in board design yet we still have
> device trees that describe the hardware, I can't see why these systems
> should be so different. It is entirely normal for system integrators to
> collaborate on this and even upstream their own code, this happens all
> the time, there is no need for everything to be implemented directly the
> SoC vendor.
This is all well and good and there is no disagreement here that it
should just be that way but it does not reflect what Jim and I are
confronted with on a daily basis. We work in a tightly controlled
environment using a waterfall approach, whatever we come up with as a
SoC vendor gets used usually without further modification by the OEMs,
when OEMs do change things we have no visibility into anyway.
We have a boot loader that goes into great lengths to tailor the FDT
blob passed to the kernel to account for board-specific variations, PCIe
voltage regulators being just one of those variations. This is usually
how we quirk and deal with any board specific details, so I fail to
understand what you mean by "quirks that match a common pattern".
Also, I don't believe other vendors are quite as concerned with
conserving power as we are, it could be that they are just better at it
through different ways, or we have a particular sensitivity to the subject.
>
> If there are generic quirks that match a common pattern seen in a lot of
> board then properties can be defined for those, this is in fact the
> common case. This is no reason to just shove in some random thing that
> doesn't describe the hardware, that's a great way to end up stuck with
> an ABI that is fragile and difficult to understand or improve.
I would argue that at least 2 out of the 4 supplies proposed do describe
hardware signals. The latter two are more abstract to say the least,
however I believe it is done that way because they are composite
supplies controlling both the 12V and 3.3V supplies coming into a PCIe
device (Jim is that right?). So how do we call the latter supply then?
vpcie12v3v...-supply?
> Potentially some of these things should be being handled in the bindings
> and drivers drivers for the relevant PCI devices rather than in the PCI
> controller.
The description and device tree binding can be and should be in a PCI
device binding rather than pci-bus.yaml.
The handling however goes back to the chicken and egg situation that we
talked about multiple times before: no supply -> no link UP -> no
enumeration -> no PCI device, therefore no driver can have a chance to
control the regulator. These are not hotplug capable systems by the way,
but even if they were, we would still run into the same problem. Given
that most reference boards do have mechanical connectors that people can
plug random devices into, we cannot provide a compatible string
containing the PCI vendor/device ID ahead of time because we don't know
what will be plugged in. In the case of a MCM, we would, but then we
only solved about 15% of the boards we need to support, so we have not
really progressed much.
>
>>> I'd suggest as a first pass omitting this and then looking at some
>>> actual systems later when working out how to support them, no sense in
>>> getting the main thing held up by difficult edge cases.
>
>> These are not edge cases -- some of these are major customers.
>
> I'm trying to help you progress the driver by decoupling things which
> are causing difficulty from the simple parts so that we don't need to
> keep looking at the simple bits over and over again. If these systems
> are very common or familiar then it should be fairly easy to describe
> the common patterns in what they're doing.
We are appreciative of your feedback, and Rob's, and everyone else
looking at the patches really.
>
> In any case whatever gets done needs to be documented in the bindings
> documents.
Is not that what patch #1 does?
--
Florian
On Mon, Oct 25, 2021 at 03:04:34PM -0700, Florian Fainelli wrote:
> On 10/25/21 7:58 AM, Mark Brown wrote:
> > On Mon, Oct 25, 2021 at 09:50:09AM -0400, Jim Quinlan wrote:
> >> On Fri, Oct 22, 2021 at 3:47 PM Mark Brown <[email protected]> wrote:
> >>> On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
> >
> >>> That sounds like it just shouldn't be a regulator at all, perhaps the
> >>> board happens to need a regulator there but perhaps it needs a clock,
> >>> GPIO or some specific sequence of actions. It sounds like you need some
> >>> sort of quirking mechanism to cope with individual boards with board
> >>> specific bindings.
> >
> >> The boards involved may have no PCIe sockets, or run the gamut of the different
> >> PCIe sockets. They all offer gpio(s) to turn off/on their power supply(s) to
> >> make their PCIe device endpoint functional. It is not viable to add
> >> new Linux quirk or DT
> >> code for each board. First is the volume and variety of the boards
> >> that use our SOCs.. Second, is
> >> our lack of information/control: often, the board is designed by one
> >> company (not us), and
> >> given to another company as the middleman, and then they want the
> >> features outlined
> >> in my aforementioned commit message.
> >
> > Other vendors have plenty of variation in board design yet we still have
> > device trees that describe the hardware, I can't see why these systems
> > should be so different. It is entirely normal for system integrators to
> > collaborate on this and even upstream their own code, this happens all
> > the time, there is no need for everything to be implemented directly the
> > SoC vendor.
>
> This is all well and good and there is no disagreement here that it
> should just be that way but it does not reflect what Jim and I are
> confronted with on a daily basis. We work in a tightly controlled
> environment using a waterfall approach, whatever we come up with as a
> SoC vendor gets used usually without further modification by the OEMs,
> when OEMs do change things we have no visibility into anyway.
>
> We have a boot loader that goes into great lengths to tailor the FDT
> blob passed to the kernel to account for board-specific variations, PCIe
> voltage regulators being just one of those variations. This is usually
> how we quirk and deal with any board specific details, so I fail to
> understand what you mean by "quirks that match a common pattern".
>
> Also, I don't believe other vendors are quite as concerned with
> conserving power as we are, it could be that they are just better at it
> through different ways, or we have a particular sensitivity to the subject.
>
> >
> > If there are generic quirks that match a common pattern seen in a lot of
> > board then properties can be defined for those, this is in fact the
> > common case. This is no reason to just shove in some random thing that
> > doesn't describe the hardware, that's a great way to end up stuck with
> > an ABI that is fragile and difficult to understand or improve.
>
> I would argue that at least 2 out of the 4 supplies proposed do describe
> hardware signals. The latter two are more abstract to say the least,
> however I believe it is done that way because they are composite
> supplies controlling both the 12V and 3.3V supplies coming into a PCIe
> device (Jim is that right?). So how do we call the latter supply then?
> vpcie12v3v...-supply?
>
> > Potentially some of these things should be being handled in the bindings
> > and drivers drivers for the relevant PCI devices rather than in the PCI
> > controller.
>
> The description and device tree binding can be and should be in a PCI
> device binding rather than pci-bus.yaml.
>
> The handling however goes back to the chicken and egg situation that we
> talked about multiple times before: no supply -> no link UP -> no
> enumeration -> no PCI device, therefore no driver can have a chance to
> control the regulator. These are not hotplug capable systems by the way,
> but even if they were, we would still run into the same problem. Given
> that most reference boards do have mechanical connectors that people can
> plug random devices into, we cannot provide a compatible string
> containing the PCI vendor/device ID ahead of time because we don't know
> what will be plugged in.
I thought you didn't have connectors or was it just they are
non-standard? If the latter case, what are the supply rails for the
connector?
I'd be okay if there's no compatible as long as there's not a continual
stream of DT properties trying to describe power sequencing
requirements.
> In the case of a MCM, we would, but then we
> only solved about 15% of the boards we need to support, so we have not
> really progressed much.
MCM is multi-chip module?
Rob
On 10/25/21 3:43 PM, Rob Herring wrote:
> On Mon, Oct 25, 2021 at 03:04:34PM -0700, Florian Fainelli wrote:
>> On 10/25/21 7:58 AM, Mark Brown wrote:
>>> On Mon, Oct 25, 2021 at 09:50:09AM -0400, Jim Quinlan wrote:
>>>> On Fri, Oct 22, 2021 at 3:47 PM Mark Brown <[email protected]> wrote:
>>>>> On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
>>>
>>>>> That sounds like it just shouldn't be a regulator at all, perhaps the
>>>>> board happens to need a regulator there but perhaps it needs a clock,
>>>>> GPIO or some specific sequence of actions. It sounds like you need some
>>>>> sort of quirking mechanism to cope with individual boards with board
>>>>> specific bindings.
>>>
>>>> The boards involved may have no PCIe sockets, or run the gamut of the different
>>>> PCIe sockets. They all offer gpio(s) to turn off/on their power supply(s) to
>>>> make their PCIe device endpoint functional. It is not viable to add
>>>> new Linux quirk or DT
>>>> code for each board. First is the volume and variety of the boards
>>>> that use our SOCs.. Second, is
>>>> our lack of information/control: often, the board is designed by one
>>>> company (not us), and
>>>> given to another company as the middleman, and then they want the
>>>> features outlined
>>>> in my aforementioned commit message.
>>>
>>> Other vendors have plenty of variation in board design yet we still have
>>> device trees that describe the hardware, I can't see why these systems
>>> should be so different. It is entirely normal for system integrators to
>>> collaborate on this and even upstream their own code, this happens all
>>> the time, there is no need for everything to be implemented directly the
>>> SoC vendor.
>>
>> This is all well and good and there is no disagreement here that it
>> should just be that way but it does not reflect what Jim and I are
>> confronted with on a daily basis. We work in a tightly controlled
>> environment using a waterfall approach, whatever we come up with as a
>> SoC vendor gets used usually without further modification by the OEMs,
>> when OEMs do change things we have no visibility into anyway.
>>
>> We have a boot loader that goes into great lengths to tailor the FDT
>> blob passed to the kernel to account for board-specific variations, PCIe
>> voltage regulators being just one of those variations. This is usually
>> how we quirk and deal with any board specific details, so I fail to
>> understand what you mean by "quirks that match a common pattern".
>>
>> Also, I don't believe other vendors are quite as concerned with
>> conserving power as we are, it could be that they are just better at it
>> through different ways, or we have a particular sensitivity to the subject.
>>
>>>
>>> If there are generic quirks that match a common pattern seen in a lot of
>>> board then properties can be defined for those, this is in fact the
>>> common case. This is no reason to just shove in some random thing that
>>> doesn't describe the hardware, that's a great way to end up stuck with
>>> an ABI that is fragile and difficult to understand or improve.
>>
>> I would argue that at least 2 out of the 4 supplies proposed do describe
>> hardware signals. The latter two are more abstract to say the least,
>> however I believe it is done that way because they are composite
>> supplies controlling both the 12V and 3.3V supplies coming into a PCIe
>> device (Jim is that right?). So how do we call the latter supply then?
>> vpcie12v3v...-supply?
>>
>>> Potentially some of these things should be being handled in the bindings
>>> and drivers drivers for the relevant PCI devices rather than in the PCI
>>> controller.
>>
>> The description and device tree binding can be and should be in a PCI
>> device binding rather than pci-bus.yaml.
>>
>> The handling however goes back to the chicken and egg situation that we
>> talked about multiple times before: no supply -> no link UP -> no
>> enumeration -> no PCI device, therefore no driver can have a chance to
>> control the regulator. These are not hotplug capable systems by the way,
>> but even if they were, we would still run into the same problem. Given
>> that most reference boards do have mechanical connectors that people can
>> plug random devices into, we cannot provide a compatible string
>> containing the PCI vendor/device ID ahead of time because we don't know
>> what will be plugged in.
>
> I thought you didn't have connectors or was it just they are
> non-standard? If the latter case, what are the supply rails for the
> connector?
We now have reference boards with full-sized x1 and x4 connectors in
addition to half sized and full-sized mini-PCIe connectors and the
soldered down Wi-Fi on board (WOMBO) and the Multi-chip Module packages
(MCM).
When using connectors we would use the standard PCIe pinout nomenclature
however for the latter two, there appears to be a variety of
non-standard stuff being done there. We reviewed some schematics with
Jim and it looks like some of the usages for the regulators are just
laziness on the Wi-Fi driver side, like asking the kernel to keep the
radio on (PA, LNA etc.) as if it was as critical and necessary as the
12V and 3.3V supplies that actually power on the PCIe end-point... We
will get those fixed hopefully.
>
> I'd be okay if there's no compatible as long as there's not a continual
> stream of DT properties trying to describe power sequencing
> requirements.
Have not looked whether Dmitry's power sequencing generalizing is
helping us here:
https://www.spinics.net/lists/linux-bluetooth/msg93564.html
it still looks like the PCIe host controller is involved in requesting
the power sequence.
>
>> In the case of a MCM, we would, but then we
>> only solved about 15% of the boards we need to support, so we have not
>> really progressed much.
>
> MCM is multi-chip module?
Correct, see above.
--
Florian
On Mon, Oct 25, 2021 at 03:04:34PM -0700, Florian Fainelli wrote:
> On 10/25/21 7:58 AM, Mark Brown wrote:
> > Other vendors have plenty of variation in board design yet we still have
> > device trees that describe the hardware, I can't see why these systems
> > should be so different. It is entirely normal for system integrators to
> > collaborate on this and even upstream their own code, this happens all
> > the time, there is no need for everything to be implemented directly the
> > SoC vendor.
> This is all well and good and there is no disagreement here that it
> should just be that way but it does not reflect what Jim and I are
> confronted with on a daily basis. We work in a tightly controlled
> environment using a waterfall approach, whatever we come up with as a
> SoC vendor gets used usually without further modification by the OEMs,
> when OEMs do change things we have no visibility into anyway.
This doesn't really sound terribly unusual frankly, which means it
shouldn't be insurmountable. It also doesn't sound like a great
approach to base ABIs around.
> We have a boot loader that goes into great lengths to tailor the FDT
> blob passed to the kernel to account for board-specific variations, PCIe
> voltage regulators being just one of those variations. This is usually
> how we quirk and deal with any board specific details, so I fail to
> understand what you mean by "quirks that match a common pattern".
If more than one board needs the same accomodation, for example if it's
common for a reset line to be GPIO controlled, then a common property
could be used by many different boards rather than requiring each
individual board to have board specific code. This is on some level
what most DT properties boil down to.
> Also, I don't believe other vendors are quite as concerned with
> conserving power as we are, it could be that they are just better at it
> through different ways, or we have a particular sensitivity to the subject.
I'm fairly sure that for example phone vendors pay a bit of attention to
power consumption.
> > If there are generic quirks that match a common pattern seen in a lot of
> > board then properties can be defined for those, this is in fact the
> > common case. This is no reason to just shove in some random thing that
> > doesn't describe the hardware, that's a great way to end up stuck with
> > an ABI that is fragile and difficult to understand or improve.
> I would argue that at least 2 out of the 4 supplies proposed do describe
> hardware signals. The latter two are more abstract to say the least,
> however I believe it is done that way because they are composite
> supplies controlling both the 12V and 3.3V supplies coming into a PCIe
> device (Jim is that right?). So how do we call the latter supply then?
> vpcie12v3v...-supply?
The binding for a consumer should reflect what's going into that
consumer, this means that if you have 12V and 3.3V supplies then the
device should have two distinct supplies for that. The device binding
should not change based on how those supplies are provided or any
relationship between the supplies outside the device, there should
definitely be no reason to define any new supplies for this purpose -
that would reflect a fundamental misunderstanding of the abstractions in
the API.
If (as it sounds) you've got systems with two supplies with GPIO enables
controlled by a single GPIO then you should just describe that directly
in device tree, this is quite common and there is support in there
already for identifying and reference counting the shared GPIO in that
case.
> > Potentially some of these things should be being handled in the bindings
> > and drivers drivers for the relevant PCI devices rather than in the PCI
> > controller.
> The description and device tree binding can be and should be in a PCI
> device binding rather than pci-bus.yaml.
Well, it's a bit of a shared responsibility where the thing being
provided is a standards conforming connector rather than there being an
embedded device with much more potential for variation.
> The handling however goes back to the chicken and egg situation that we
> talked about multiple times before: no supply -> no link UP -> no
> enumeration -> no PCI device, therefore no driver can have a chance to
> control the regulator. These are not hotplug capable systems by the way,
> but even if they were, we would still run into the same problem. Given
> that most reference boards do have mechanical connectors that people can
> plug random devices into, we cannot provide a compatible string
> containing the PCI vendor/device ID ahead of time because we don't know
> what will be plugged in. In the case of a MCM, we would, but then we
> only solved about 15% of the boards we need to support, so we have not
> really progressed much.
I would expect it's possible to make a PCI binding for a standard
physical layer bus connection as part of the generic PCI bindings, for
example by using some standard invalid ID for the device ID that can't
exist in a physical system or just omitting the device ID. That seems
like a fairly clear case of being able to describe actual hardware that
physically exists - you can see the physical socket on the board.
> > In any case whatever gets done needs to be documented in the bindings
> > documents.
> Is not that what patch #1 does?
It just updated the example, it didn't document any new properties. The
standard supplies are documented in the patch to the core standard that
was referenced so they're fine but not the extra Broadcom specific ones
that I've raised concerns with.