From: "jian.yang" <[email protected]>
Add new properties to support control power supplies and reset pin of
a downstream component.
Signed-off-by: jian.yang <[email protected]>
---
.../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 11/01/2023 04:28, Jian Yang wrote:
> From: "jian.yang" <[email protected]>
>
> Add new properties to support control power supplies and reset pin of
> a downstream component.
>
> Signed-off-by: jian.yang <[email protected]>
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" <[email protected]>
> >
> > Add new properties to support control power supplies and reset pin
> > of
> > a downstream component.
> >
> > Signed-off-by: jian.yang <[email protected]>
>
> 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/[email protected]/
Feedback is welcomed - there. Don't create your own half-baked delays
for different hardware designs.
Best regards,
Krzysztof
On Tue, Jan 10, 2023 at 9:28 PM Jian Yang <[email protected]> wrote:
>
> From: "jian.yang" <[email protected]>
>
> Add new properties to support control power supplies and reset pin of
> a downstream component.
>
> Signed-off-by: jian.yang <[email protected]>
> ---
> .../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 <[email protected]>
> wrote:
> >
> > From: "jian.yang" <[email protected]>
> >
> > Add new properties to support control power supplies and reset pin
> > of
> > a downstream component.
> >
> > Signed-off-by: jian.yang <[email protected]>
> > ---
> > .../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
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/[email protected]/
>
> 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