| Message ID | 20211022140714.28767-5-jim2101024@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: brcmstb: have host-bridge turn on sub-device power | expand |
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 <broonie@kernel.org> 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 <broonie@kernel.org> 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 <broonie@kernel.org> 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 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 <broonie@kernel.org> 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?
On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote: > On Fri, Oct 22, 2021 at 10:31 AM Mark Brown <broonie@kernel.org> 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 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 <broonie@kernel.org> 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 <broonie@kernel.org> 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.
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.
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index ba4d6daf312c..35924af1c3aa 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -24,6 +24,7 @@ #include <linux/pci.h> #include <linux/pci-ecam.h> #include <linux/printk.h> +#include <linux/regulator/consumer.h> #include <linux/reset.h> #include <linux/sizes.h> #include <linux/slab.h> @@ -192,6 +193,13 @@ static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val); static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val); static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val); +static const char * const supplies[] = { + "vpcie3v3-supply", + "vpcie3v3aux-supply", + "brcm-ep-a-supply", + "brcm-ep-b-supply", +}; + enum { RGR1_SW_INIT_1, EXT_CFG_INDEX, @@ -295,8 +303,27 @@ struct brcm_pcie { u32 hw_rev; void (*perst_set)(struct brcm_pcie *pcie, u32 val); void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val); + struct regulator_bulk_data supplies[ARRAY_SIZE(supplies)]; + unsigned int num_supplies; }; +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on) +{ + struct device *dev = pcie->dev; + int ret; + + if (!pcie->num_supplies) + return 0; + if (on) + ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies); + else + ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies); + if (ret) + dev_err(dev, "failed to %s EP regulators\n", + on ? "enable" : "disable"); + return ret; +} + /* * This is to convert the size of the inbound "BAR" region to the * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE @@ -1148,6 +1175,55 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) pcie->bridge_sw_init_set(pcie, 1); } +static int brcm_pcie_get_regulators(struct brcm_pcie *pcie, struct pci_dev *dev) +{ + const unsigned int ns = ARRAY_SIZE(supplies); + struct device_node *dn; + struct property *pp; + unsigned int i; + int ret; + + /* This is for Broadcom STB/CM chips only */ + if (pcie->type == BCM2711) + return 0; + + pci_set_of_node(dev); + dn = dev->dev.of_node; + if (!dn) + return 0; + + for_each_property_of_node(dn, pp) { + for (i = 0; i < ns; i++) + if (strcmp(supplies[i], pp->name) == 0) + break; + if (i >= ns || pcie->num_supplies >= ARRAY_SIZE(supplies)) + continue; + + pcie->supplies[pcie->num_supplies++].supply = supplies[i]; + } + + if (pcie->num_supplies == 0) + return 0; + + /* + * We set the name ahead of time as the regulator core expects + * it to exist when regulator_bulk_get() is called. + */ + dev_set_name(&dev->dev, "%04x:%02x:%02x.%d", + pci_domain_nr(dev->bus), + dev->bus->number, PCI_SLOT(dev->devfn), + PCI_FUNC(dev->devfn)); + /* + * We cannot use devm_regulator_bulk_get() because the + * downstream device may be removed w/o the regulator + * first being disabled by the host bridge. + */ + ret = regulator_bulk_get(&dev->dev, pcie->num_supplies, + pcie->supplies); + + return ret; +} + static int brcm_pcie_suspend(struct device *dev) { struct brcm_pcie *pcie = dev_get_drvdata(dev); @@ -1158,7 +1234,7 @@ static int brcm_pcie_suspend(struct device *dev) reset_control_rearm(pcie->rescal); clk_disable_unprepare(pcie->clk); - return ret; + return brcm_set_regulators(pcie, false); } static int brcm_pcie_resume(struct device *dev) @@ -1174,6 +1250,9 @@ static int brcm_pcie_resume(struct device *dev) ret = reset_control_reset(pcie->rescal); if (ret) goto err_disable_clk; + ret = brcm_set_regulators(pcie, true); + if (ret) + goto err_reset; ret = brcm_phy_start(pcie); if (ret) @@ -1217,6 +1296,10 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) brcm_phy_stop(pcie); reset_control_rearm(pcie->rescal); clk_disable_unprepare(pcie->clk); + if (pcie->num_supplies) { + (void)brcm_set_regulators(pcie, false); + regulator_bulk_free(pcie->num_supplies, pcie->supplies); + } } static int brcm_pcie_remove(struct platform_device *pdev) @@ -1241,6 +1324,56 @@ static const struct of_device_id brcm_pcie_match[] = { {}, }; +static int brcm_pcie_pci_subdev_prepare(bool query, struct pci_bus *bus, int devfn, + struct pci_host_bridge *bridge, + struct pci_dev *pdev) +{ + struct brcm_pcie *pcie; + int ret = 0; + + /* + * We only care about a device that is directly connected + * to the root complex, ie bus == 1 and slot == 0. + */ + if (query) + return (bus->number == 1 && PCI_SLOT(devfn) == 0); + + + pcie = (struct brcm_pcie *) bridge->sysdata; + ret = brcm_pcie_get_regulators(pcie, pdev); + if (ret) { + pcie->num_supplies = 0; + if (ret != -EPROBE_DEFER) + dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret); + return ret; + } + + ret = brcm_set_regulators(pcie, true); + if (ret) + goto err_out0; + + ret = brcm_pcie_linkup(pcie); + if (ret) + goto err_out1; + + /* + * dev_set_name() was called in brcm_set_regulators(). Free the + * string it allocated as it will be called again when + * pci_setup_device() is invoked. + */ + kfree_const(pdev->dev.kobj.name); + pdev->dev.kobj.name = NULL; + + return 0; + +err_out1: + brcm_set_regulators(pcie, false); +err_out0: + regulator_bulk_free(pcie->num_supplies, pcie->supplies); + pcie->num_supplies = 0; + return ret; +} + static int brcm_pcie_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node, *msi_np; @@ -1327,12 +1460,23 @@ static int brcm_pcie_probe(struct platform_device *pdev) } } + bridge->pci_subdev_prepare = brcm_pcie_pci_subdev_prepare; bridge->ops = &brcm_pcie_ops; bridge->sysdata = pcie; platform_set_drvdata(pdev, pcie); - return pci_host_probe(bridge); + ret = pci_host_probe(bridge); + if (ret) + goto fail; + + if (!brcm_pcie_link_up(pcie)) { + brcm_pcie_remove(pdev); + return -ENODEV; + } + + return 0; + fail: __brcm_pcie_remove(pcie); return ret;
This Broadcom STB PCIe RC driver has one port and connects directly to one device, be it a switch or an endpoint. We want to be able to turn on/off any regulators for that device. Control of regulators is needed because of the chicken-and-egg situation: although the regulator is "owned" by the device and would be best handled by its driver, the device cannot be discovered and probed unless its regulator is already turned on. Signed-off-by: Jim Quinlan <jim2101024@gmail.com> --- drivers/pci/controller/pcie-brcmstb.c | 148 +++++++++++++++++++++++++- 1 file changed, 146 insertions(+), 2 deletions(-)