From: "jian.yang" <jian.yang@mediatek.com>
Add new properties to support control power supplies and reset pin of
a downstream component.
Signed-off-by: jian.yang <jian.yang@mediatek.com>
---
.../bindings/pci/mediatek-pcie-gen3.yaml | 23 +++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
index 7e8c7a2a5f9b..46149cc63989 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
@@ -84,6 +84,29 @@ properties:
items:
enum: [ phy, mac ]
+ pcie1v8-supply:
+ description:
+ The regulator phandle that provides 1.8V power to downstream component.
+
+ pcie3v3-supply:
+ description:
+ The regulator phandle that provides 3.3V power to downstream component.
+
+ pcie12v-supply:
+ description:
+ The regulator phandle that provides 12V power to downstream component.
+
+ dsc-reset-gpios:
+ description:
+ The reset GPIO of a downstream component.
+ maxItems: 1
+
+ dsc-reset-msleep:
+ description:
+ The delay time between assertion and de-assertion of a downstream
+ component's reset GPIO.
+ maxItems: 1
+
clocks:
minItems: 4
maxItems: 6
--
2.18.0
On Tue, Jan 10, 2023 at 9:28 PM Jian Yang <jian.yang@mediatek.com> wrote: > > From: "jian.yang" <jian.yang@mediatek.com> > > Add new properties to support control power supplies and reset pin of > a downstream component. > > Signed-off-by: jian.yang <jian.yang@mediatek.com> > --- > .../bindings/pci/mediatek-pcie-gen3.yaml | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml > index 7e8c7a2a5f9b..46149cc63989 100644 > --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml > +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml > @@ -84,6 +84,29 @@ properties: > items: > enum: [ phy, mac ] > > + pcie1v8-supply: > + description: > + The regulator phandle that provides 1.8V power to downstream component. > + > + pcie3v3-supply: > + description: > + The regulator phandle that provides 3.3V power to downstream component. > + > + pcie12v-supply: > + description: > + The regulator phandle that provides 12V power to downstream component. While in some bindings we've allowed these in the host bridge node, that is a mistake. These should be in the root port node. You probably don't have one in DT, so add one. > + > + dsc-reset-gpios: > + description: > + The reset GPIO of a downstream component. > + maxItems: 1 > + > + dsc-reset-msleep: Doesn't the PCI spec define this time? We're talking about PERST#, right? > + description: > + The delay time between assertion and de-assertion of a downstream > + component's reset GPIO. > + maxItems: 1 > + > clocks: > minItems: 4 > maxItems: 6 > -- > 2.18.0 >
Dear Rob, Thanks for your comment. On Fri, 2023-02-03 at 10:07 -0600, Rob Herring wrote: > On Tue, Jan 10, 2023 at 9:28 PM Jian Yang <jian.yang@mediatek.com> > wrote: > > > > From: "jian.yang" <jian.yang@mediatek.com> > > > > Add new properties to support control power supplies and reset pin > > of > > a downstream component. > > > > Signed-off-by: jian.yang <jian.yang@mediatek.com> > > --- > > .../bindings/pci/mediatek-pcie-gen3.yaml | 23 > > +++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie- > > gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie- > > gen3.yaml > > index 7e8c7a2a5f9b..46149cc63989 100644 > > --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml > > +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml > > @@ -84,6 +84,29 @@ properties: > > items: > > enum: [ phy, mac ] > > > > + pcie1v8-supply: > > + description: > > + The regulator phandle that provides 1.8V power to downstream > > component. > > + > > + pcie3v3-supply: > > + description: > > + The regulator phandle that provides 3.3V power to downstream > > component. > > + > > + pcie12v-supply: > > + description: > > + The regulator phandle that provides 12V power to downstream > > component. > > While in some bindings we've allowed these in the host bridge node, > that is a mistake. These should be in the root port node. You > probably > don't have one in DT, so add one. In Mediatek's PCIe structure, there is only one root port under a host bridge. I am wondering if you think it's necessary to add a root port node in our host bridge node based on that structure? And I'm a bit confused about how to declare a property which should be added in a root port node. I would be grateful if you could provide me some good example about it. > > + > > + dsc-reset-gpios: > > + description: > > + The reset GPIO of a downstream component. > > + maxItems: 1 > > + > > + dsc-reset-msleep: > > Doesn't the PCI spec define this time? We're talking about PERST#, > right? The "dsc-reset-gpios" represents an extra reset pin other than PERST# required by a downstream component. I tried to add a property here so that users can control the delay time between the assertion and deassertion according to their requirement, but Krzysztof mentioned that there is an ongoing discussion about a "GPIO delay" driver can handle this. So I will remove the "dsc-reset-msleep" in V2 patch. > > + description: > > + The delay time between assertion and de-assertion of a > > downstream > > + component's reset GPIO. > > + maxItems: 1 > > + > > clocks: > > minItems: 4 > > maxItems: 6 > > -- > > 2.18.0 > > Best regards, Jian Yang
On 11/01/2023 04:28, Jian Yang wrote: > From: "jian.yang" <jian.yang@mediatek.com> > > Add new properties to support control power supplies and reset pin of > a downstream component. > > Signed-off-by: jian.yang <jian.yang@mediatek.com> Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. > --- > .../bindings/pci/mediatek-pcie-gen3.yaml | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml > index 7e8c7a2a5f9b..46149cc63989 100644 > --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml > +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml > @@ -84,6 +84,29 @@ properties: > items: > enum: [ phy, mac ] > > + pcie1v8-supply: > + description: > + The regulator phandle that provides 1.8V power to downstream component. > + > + pcie3v3-supply: > + description: > + The regulator phandle that provides 3.3V power to downstream component. > + > + pcie12v-supply: > + description: > + The regulator phandle that provides 12V power to downstream component. > + > + dsc-reset-gpios: > + description: > + The reset GPIO of a downstream component. Why you cannot use standard reset-gpios property? > + maxItems: 1 > + > + dsc-reset-msleep: Wrong property unit/suffix. > + description: > + The delay time between assertion and de-assertion of a downstream > + component's reset GPIO. Why this should be a property of DT? > + maxItems: 1 maxItems of what? > + > clocks: > minItems: 4 > maxItems: 6 Best regards, Krzysztof
Dear Krzysztof, Sorry for the late response and thanks for your comment. On Wed, 2023-01-11 at 10:30 +0100, Krzysztof Kozlowski wrote: > On 11/01/2023 04:28, Jian Yang wrote: > > From: "jian.yang" <jian.yang@mediatek.com> > > > > Add new properties to support control power supplies and reset pin > > of > > a downstream component. > > > > Signed-off-by: jian.yang <jian.yang@mediatek.com> > > Please use scripts/get_maintainers.pl to get a list of necessary > people > and lists to CC. It might happen, that command when run on an older > kernel, gives you outdated entries. Therefore please be sure you > base > your patches on recent Linux kernel. > > > --- > > .../bindings/pci/mediatek-pcie-gen3.yaml | 23 > > +++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie- > > gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie- > > gen3.yaml > > index 7e8c7a2a5f9b..46149cc63989 100644 > > --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml > > +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml > > @@ -84,6 +84,29 @@ properties: > > items: > > enum: [ phy, mac ] > > > > + pcie1v8-supply: > > + description: > > + The regulator phandle that provides 1.8V power to downstream > > component. > > + > > + pcie3v3-supply: > > + description: > > + The regulator phandle that provides 3.3V power to downstream > > component. > > + > > + pcie12v-supply: > > + description: > > + The regulator phandle that provides 12V power to downstream > > component. > > + > > + dsc-reset-gpios: > > + description: > > + The reset GPIO of a downstream component. > > Why you cannot use standard reset-gpios property? The "dsc-reset-gpios" represents an extra reset pin other than PERST# required by a PCIe downstream device. But the "reset-gpios", described in "pci.txt", represents the PERST#. So I tend to add a new property to meet this requirement. > > > + maxItems: 1 > > + > > + dsc-reset-msleep: > > Wrong property unit/suffix. Thanks for the correction. I will modify it in v2 patch. > > > + description: > > + The delay time between assertion and de-assertion of a > > downstream > > + component's reset GPIO. > > Why this should be a property of DT? Same as the reason I described above. I suppose we need to add a property to let user determine the delay time due to differences in requirements between various devices. > > > + maxItems: 1 > > maxItems of what? Seems unnecessary to add a "maxItems" here. Also I will remove it in v2 patch. Thanks a lot. Best regards, Jian Yang
On 03/02/2023 10:38, Jian Yang (杨戬) wrote: >>> + pcie12v-supply: >>> + description: >>> + The regulator phandle that provides 12V power to downstream >>> component. >>> + >>> + dsc-reset-gpios: >>> + description: >>> + The reset GPIO of a downstream component. >> >> Why you cannot use standard reset-gpios property? > > The "dsc-reset-gpios" represents an extra reset pin other than PERST# > required by a PCIe downstream device. But the "reset-gpios", described > in "pci.txt", represents the PERST#. So I tend to add a new property to > meet this requirement. OK >> >>> + description: >>> + The delay time between assertion and de-assertion of a >>> downstream >>> + component's reset GPIO. >> >> Why this should be a property of DT? > > Same as the reason I described above. I suppose we need to add a > property to let user determine the delay time due to differences > in requirements between various devices. No, I don't think we want individual properties like that. There is ongoing discussion about this: https://lore.kernel.org/all/20221214095342.937303-1-alexander.stein@ew.tq-group.com/ Feedback is welcomed - there. Don't create your own half-baked delays for different hardware designs. Best regards, Krzysztof
Dear Krzysztof, On Fri, 2023-02-03 at 13:32 +0100, Krzysztof Kozlowski wrote: > On 03/02/2023 10:38, Jian Yang (杨戬) wrote: > > > > + pcie12v-supply: > > > > + description: > > > > + The regulator phandle that provides 12V power to > > > > downstream > > > > component. > > > > + > > > > + dsc-reset-gpios: > > > > + description: > > > > + The reset GPIO of a downstream component. > > > > > > Why you cannot use standard reset-gpios property? > > > > The "dsc-reset-gpios" represents an extra reset pin other than > > PERST# > > required by a PCIe downstream device. But the "reset-gpios", > > described > > in "pci.txt", represents the PERST#. So I tend to add a new > > property to > > meet this requirement. > > OK > > > > > > > > + description: > > > > + The delay time between assertion and de-assertion of a > > > > downstream > > > > + component's reset GPIO. > > > > > > Why this should be a property of DT? > > > > Same as the reason I described above. I suppose we need to add a > > property to let user determine the delay time due to differences > > in requirements between various devices. > > No, I don't think we want individual properties like that. There is > ongoing discussion about this: > https://lore.kernel.org/all/20221214095342.937303-1-alexander.stein@ew.tq-group.com/ > > Feedback is welcomed - there. Don't create your own half-baked delays > for different hardware designs. Thanks for your helpful suggestion. I will remove that property of delay-time in V2 patch and let the work-in-progress "GPIO delay" driver to handle this requirement. Best regards, Jian Yang
© 2016 - 2024 Red Hat, Inc.