| Message ID | 20240330041928.1555578-18-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Manivannan Sadhasivam |
| Headers | show |
| Series | Improve PCI memory mapping API | expand |
On 30/03/2024 05:19, Damien Le Moal wrote: > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > Describe the `ep-gpios` property which is used to map the PERST# input > signal for endpoint mode. > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml > index 6b62f6f58efe..9331d44d6963 100644 > --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml > +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml > @@ -30,6 +30,9 @@ properties: > maximum: 32 > default: 32 > > + ep-gpios: > + description: Input GPIO configured for the PERST# signal. Missing maxItems. But more important: why existing property perst-gpios, which you already have there in common schema, is not correct for this case? Best regards, Krzysztof
On 3/30/24 18:16, Krzysztof Kozlowski wrote: > On 30/03/2024 05:19, Damien Le Moal wrote: >> From: Wilfred Mallawa <wilfred.mallawa@wdc.com> >> >> Describe the `ep-gpios` property which is used to map the PERST# input >> signal for endpoint mode. >> >> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> --- >> .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >> index 6b62f6f58efe..9331d44d6963 100644 >> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >> @@ -30,6 +30,9 @@ properties: >> maximum: 32 >> default: 32 >> >> + ep-gpios: >> + description: Input GPIO configured for the PERST# signal. > > Missing maxItems. But more important: why existing property perst-gpios, > which you already have there in common schema, is not correct for this case? I am confused... Where do you find perst-gpios defined for the rk3399 ? Under Documentation/devicetree/bindings/pci/, the only schema I see using perst-gpios property are for the qcom (Qualcomm) controllers. The RC bindings for the rockchip rk3399 PCIe controller (pci/rockchip,rk3399-pcie.yaml) already define the ep-gpios property. So if anything, this patch should be probably be modified to move this property to the common schema in pci/rockchip,rk3399-pcie-common.yaml. No ? > > Best regards, > Krzysztof >
On 01/04/2024 01:06, Damien Le Moal wrote: > On 3/30/24 18:16, Krzysztof Kozlowski wrote: >> On 30/03/2024 05:19, Damien Le Moal wrote: >>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>> >>> Describe the `ep-gpios` property which is used to map the PERST# input >>> signal for endpoint mode. >>> >>> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>> --- >>> .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>> index 6b62f6f58efe..9331d44d6963 100644 >>> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>> @@ -30,6 +30,9 @@ properties: >>> maximum: 32 >>> default: 32 >>> >>> + ep-gpios: >>> + description: Input GPIO configured for the PERST# signal. >> >> Missing maxItems. But more important: why existing property perst-gpios, >> which you already have there in common schema, is not correct for this case? > > I am confused... Where do you find perst-gpios defined for the rk3399 ? > Under Documentation/devicetree/bindings/pci/, the only schema I see using > perst-gpios property are for the qcom (Qualcomm) controllers. You are right, it's so far only in Qualcomm. > The RC bindings for the rockchip rk3399 PCIe controller > (pci/rockchip,rk3399-pcie.yaml) already define the ep-gpios property. So if Any reason why this cannot be named like GPIO? Is there already a user of this in Linux kernel? Commit msg says nothing about this, so that's why I would expect name matching the signal. Best regards, Krzysztof
On 4/1/24 18:57, Krzysztof Kozlowski wrote: > On 01/04/2024 01:06, Damien Le Moal wrote: >> On 3/30/24 18:16, Krzysztof Kozlowski wrote: >>> On 30/03/2024 05:19, Damien Le Moal wrote: >>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>> >>>> Describe the `ep-gpios` property which is used to map the PERST# input >>>> signal for endpoint mode. >>>> >>>> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>> --- >>>> .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>> index 6b62f6f58efe..9331d44d6963 100644 >>>> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>> @@ -30,6 +30,9 @@ properties: >>>> maximum: 32 >>>> default: 32 >>>> >>>> + ep-gpios: >>>> + description: Input GPIO configured for the PERST# signal. >>> >>> Missing maxItems. But more important: why existing property perst-gpios, >>> which you already have there in common schema, is not correct for this case? >> >> I am confused... Where do you find perst-gpios defined for the rk3399 ? >> Under Documentation/devicetree/bindings/pci/, the only schema I see using >> perst-gpios property are for the qcom (Qualcomm) controllers. > > You are right, it's so far only in Qualcomm. > >> The RC bindings for the rockchip rk3399 PCIe controller >> (pci/rockchip,rk3399-pcie.yaml) already define the ep-gpios property. So if > > Any reason why this cannot be named like GPIO? Is there already a user > of this in Linux kernel? Commit msg says nothing about this, so that's > why I would expect name matching the signal. The RC-mode PCIe controller node of the rk3399 DTS already defines the ep-gpios property for RC side PERST# signal handling. So we simply reused the exact same name to be consistent between RC and EP. I personnally have no preferences. If there is an effort to rename such signal with some preferred pattern, I will follow. For the EP node, there was no PERST signal handling in the driver and no property defined for it, so any name is fine. "perst-gpios" would indeed be a better name, but again, given that the RC controller node has ep-gpios, we reused that. What is your recommendation here ? > > Best regards, > Krzysztof >
On 02/04/2024 01:36, Damien Le Moal wrote: > On 4/1/24 18:57, Krzysztof Kozlowski wrote: >> On 01/04/2024 01:06, Damien Le Moal wrote: >>> On 3/30/24 18:16, Krzysztof Kozlowski wrote: >>>> On 30/03/2024 05:19, Damien Le Moal wrote: >>>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>>> >>>>> Describe the `ep-gpios` property which is used to map the PERST# input >>>>> signal for endpoint mode. >>>>> >>>>> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>>> --- >>>>> .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>> index 6b62f6f58efe..9331d44d6963 100644 >>>>> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>> @@ -30,6 +30,9 @@ properties: >>>>> maximum: 32 >>>>> default: 32 >>>>> >>>>> + ep-gpios: >>>>> + description: Input GPIO configured for the PERST# signal. >>>> >>>> Missing maxItems. But more important: why existing property perst-gpios, >>>> which you already have there in common schema, is not correct for this case? >>> >>> I am confused... Where do you find perst-gpios defined for the rk3399 ? >>> Under Documentation/devicetree/bindings/pci/, the only schema I see using >>> perst-gpios property are for the qcom (Qualcomm) controllers. >> >> You are right, it's so far only in Qualcomm. >> >>> The RC bindings for the rockchip rk3399 PCIe controller >>> (pci/rockchip,rk3399-pcie.yaml) already define the ep-gpios property. So if >> >> Any reason why this cannot be named like GPIO? Is there already a user >> of this in Linux kernel? Commit msg says nothing about this, so that's >> why I would expect name matching the signal. > > The RC-mode PCIe controller node of the rk3399 DTS already defines the ep-gpios > property for RC side PERST# signal handling. So we simply reused the exact same > name to be consistent between RC and EP. I personnally have no preferences. If > there is an effort to rename such signal with some preferred pattern, I will > follow. For the EP node, there was no PERST signal handling in the driver and > no property defined for it, so any name is fine. "perst-gpios" would indeed be > a better name, but again, given that the RC controller node has ep-gpios, we > reused that. What is your recommendation here ? Actually I don't know, perst and ep would work for me. If you do not have code for this in the driver yet (nothing is shared between ep and host), then maybe let's go with perst to match the actual name. Anyway, you need maxItems. I sent a patch for the other binding: https://lore.kernel.org/all/20240401100058.15749-1-krzysztof.kozlowski@linaro.org/ Best regards, Krzysztof
On 4/2/24 16:33, Krzysztof Kozlowski wrote: > On 02/04/2024 01:36, Damien Le Moal wrote: >> On 4/1/24 18:57, Krzysztof Kozlowski wrote: >>> On 01/04/2024 01:06, Damien Le Moal wrote: >>>> On 3/30/24 18:16, Krzysztof Kozlowski wrote: >>>>> On 30/03/2024 05:19, Damien Le Moal wrote: >>>>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>>>> >>>>>> Describe the `ep-gpios` property which is used to map the PERST# input >>>>>> signal for endpoint mode. >>>>>> >>>>>> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>>>> --- >>>>>> .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>> index 6b62f6f58efe..9331d44d6963 100644 >>>>>> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>> @@ -30,6 +30,9 @@ properties: >>>>>> maximum: 32 >>>>>> default: 32 >>>>>> >>>>>> + ep-gpios: >>>>>> + description: Input GPIO configured for the PERST# signal. >>>>> >>>>> Missing maxItems. But more important: why existing property perst-gpios, >>>>> which you already have there in common schema, is not correct for this case? >>>> >>>> I am confused... Where do you find perst-gpios defined for the rk3399 ? >>>> Under Documentation/devicetree/bindings/pci/, the only schema I see using >>>> perst-gpios property are for the qcom (Qualcomm) controllers. >>> >>> You are right, it's so far only in Qualcomm. >>> >>>> The RC bindings for the rockchip rk3399 PCIe controller >>>> (pci/rockchip,rk3399-pcie.yaml) already define the ep-gpios property. So if >>> >>> Any reason why this cannot be named like GPIO? Is there already a user >>> of this in Linux kernel? Commit msg says nothing about this, so that's >>> why I would expect name matching the signal. >> >> The RC-mode PCIe controller node of the rk3399 DTS already defines the ep-gpios >> property for RC side PERST# signal handling. So we simply reused the exact same >> name to be consistent between RC and EP. I personnally have no preferences. If >> there is an effort to rename such signal with some preferred pattern, I will >> follow. For the EP node, there was no PERST signal handling in the driver and >> no property defined for it, so any name is fine. "perst-gpios" would indeed be >> a better name, but again, given that the RC controller node has ep-gpios, we >> reused that. What is your recommendation here ? > > Actually I don't know, perst and ep would work for me. If you do not > have code for this in the driver yet (nothing is shared between ep and > host), then maybe let's go with perst to match the actual name. That works for me. The other simple solution would be to move the RC node ep-gpios description to the common schema pci/rockchip,rk3399-pcie-common.yaml, maybe ? Otherwise, perst-gpios like the Qualcomm schemas would be nice too. > > Anyway, you need maxItems. I sent a patch for the other binding: > https://lore.kernel.org/all/20240401100058.15749-1-krzysztof.kozlowski@linaro.org/ Thanks for that. > > Best regards, > Krzysztof >
On 4/2/24 16:33, Krzysztof Kozlowski wrote: > On 02/04/2024 01:36, Damien Le Moal wrote: >> On 4/1/24 18:57, Krzysztof Kozlowski wrote: >>> On 01/04/2024 01:06, Damien Le Moal wrote: >>>> On 3/30/24 18:16, Krzysztof Kozlowski wrote: >>>>> On 30/03/2024 05:19, Damien Le Moal wrote: >>>>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>>>> >>>>>> Describe the `ep-gpios` property which is used to map the PERST# input >>>>>> signal for endpoint mode. >>>>>> >>>>>> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>>>> --- >>>>>> .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>> index 6b62f6f58efe..9331d44d6963 100644 >>>>>> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>> @@ -30,6 +30,9 @@ properties: >>>>>> maximum: 32 >>>>>> default: 32 >>>>>> >>>>>> + ep-gpios: >>>>>> + description: Input GPIO configured for the PERST# signal. >>>>> >>>>> Missing maxItems. But more important: why existing property perst-gpios, >>>>> which you already have there in common schema, is not correct for this case? >>>> >>>> I am confused... Where do you find perst-gpios defined for the rk3399 ? >>>> Under Documentation/devicetree/bindings/pci/, the only schema I see using >>>> perst-gpios property are for the qcom (Qualcomm) controllers. >>> >>> You are right, it's so far only in Qualcomm. >>> >>>> The RC bindings for the rockchip rk3399 PCIe controller >>>> (pci/rockchip,rk3399-pcie.yaml) already define the ep-gpios property. So if >>> >>> Any reason why this cannot be named like GPIO? Is there already a user >>> of this in Linux kernel? Commit msg says nothing about this, so that's >>> why I would expect name matching the signal. >> >> The RC-mode PCIe controller node of the rk3399 DTS already defines the ep-gpios >> property for RC side PERST# signal handling. So we simply reused the exact same >> name to be consistent between RC and EP. I personnally have no preferences. If >> there is an effort to rename such signal with some preferred pattern, I will >> follow. For the EP node, there was no PERST signal handling in the driver and >> no property defined for it, so any name is fine. "perst-gpios" would indeed be >> a better name, but again, given that the RC controller node has ep-gpios, we >> reused that. What is your recommendation here ? > > Actually I don't know, perst and ep would work for me. If you do not > have code for this in the driver yet (nothing is shared between ep and > host), then maybe let's go with perst to match the actual name. Forgot to add: the driver code for the EP PERST gpio handling is added in patch 18 of the series, after this one. > > Anyway, you need maxItems. I sent a patch for the other binding: > https://lore.kernel.org/all/20240401100058.15749-1-krzysztof.kozlowski@linaro.org/ > > Best regards, > Krzysztof >
On 4/2/24 16:38, Damien Le Moal wrote: > On 4/2/24 16:33, Krzysztof Kozlowski wrote: >> On 02/04/2024 01:36, Damien Le Moal wrote: >>> On 4/1/24 18:57, Krzysztof Kozlowski wrote: >>>> On 01/04/2024 01:06, Damien Le Moal wrote: >>>>> On 3/30/24 18:16, Krzysztof Kozlowski wrote: >>>>>> On 30/03/2024 05:19, Damien Le Moal wrote: >>>>>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>>>>> >>>>>>> Describe the `ep-gpios` property which is used to map the PERST# input >>>>>>> signal for endpoint mode. >>>>>>> >>>>>>> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>>>>> --- >>>>>>> .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 3 +++ >>>>>>> 1 file changed, 3 insertions(+) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>>> index 6b62f6f58efe..9331d44d6963 100644 >>>>>>> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>>> @@ -30,6 +30,9 @@ properties: >>>>>>> maximum: 32 >>>>>>> default: 32 >>>>>>> >>>>>>> + ep-gpios: >>>>>>> + description: Input GPIO configured for the PERST# signal. >>>>>> >>>>>> Missing maxItems. But more important: why existing property perst-gpios, >>>>>> which you already have there in common schema, is not correct for this case? >>>>> >>>>> I am confused... Where do you find perst-gpios defined for the rk3399 ? >>>>> Under Documentation/devicetree/bindings/pci/, the only schema I see using >>>>> perst-gpios property are for the qcom (Qualcomm) controllers. >>>> >>>> You are right, it's so far only in Qualcomm. >>>> >>>>> The RC bindings for the rockchip rk3399 PCIe controller >>>>> (pci/rockchip,rk3399-pcie.yaml) already define the ep-gpios property. So if >>>> >>>> Any reason why this cannot be named like GPIO? Is there already a user >>>> of this in Linux kernel? Commit msg says nothing about this, so that's >>>> why I would expect name matching the signal. >>> >>> The RC-mode PCIe controller node of the rk3399 DTS already defines the ep-gpios >>> property for RC side PERST# signal handling. So we simply reused the exact same >>> name to be consistent between RC and EP. I personnally have no preferences. If >>> there is an effort to rename such signal with some preferred pattern, I will >>> follow. For the EP node, there was no PERST signal handling in the driver and >>> no property defined for it, so any name is fine. "perst-gpios" would indeed be >>> a better name, but again, given that the RC controller node has ep-gpios, we >>> reused that. What is your recommendation here ? >> >> Actually I don't know, perst and ep would work for me. If you do not >> have code for this in the driver yet (nothing is shared between ep and >> host), then maybe let's go with perst to match the actual name. > > That works for me. The other simple solution would be to move the RC node > ep-gpios description to the common schema pci/rockchip,rk3399-pcie-common.yaml, > maybe ? Otherwise, perst-gpios like the Qualcomm schemas would be nice too. Thinking more about this, I think moving the ep-gpios description to the common schema is the right thing to do given that the driver uses common code between RC and EP to get that property. But if that is not acceptable, I can rename it and get that property in the controller EP mode initialization code. That will be add a little more code in the driver. > >> >> Anyway, you need maxItems. I sent a patch for the other binding: >> https://lore.kernel.org/all/20240401100058.15749-1-krzysztof.kozlowski@linaro.org/ > > Thanks for that. > >> >> Best regards, >> Krzysztof >> >
On 02/04/2024 09:55, Damien Le Moal wrote: > On 4/2/24 16:38, Damien Le Moal wrote: >> On 4/2/24 16:33, Krzysztof Kozlowski wrote: >>> On 02/04/2024 01:36, Damien Le Moal wrote: >>>> On 4/1/24 18:57, Krzysztof Kozlowski wrote: >>>>> On 01/04/2024 01:06, Damien Le Moal wrote: >>>>>> On 3/30/24 18:16, Krzysztof Kozlowski wrote: >>>>>>> On 30/03/2024 05:19, Damien Le Moal wrote: >>>>>>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>>>>>> >>>>>>>> Describe the `ep-gpios` property which is used to map the PERST# input >>>>>>>> signal for endpoint mode. >>>>>>>> >>>>>>>> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> >>>>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>>>>>> --- >>>>>>>> .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 3 +++ >>>>>>>> 1 file changed, 3 insertions(+) >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>>>> index 6b62f6f58efe..9331d44d6963 100644 >>>>>>>> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>>>> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml >>>>>>>> @@ -30,6 +30,9 @@ properties: >>>>>>>> maximum: 32 >>>>>>>> default: 32 >>>>>>>> >>>>>>>> + ep-gpios: >>>>>>>> + description: Input GPIO configured for the PERST# signal. >>>>>>> >>>>>>> Missing maxItems. But more important: why existing property perst-gpios, >>>>>>> which you already have there in common schema, is not correct for this case? >>>>>> >>>>>> I am confused... Where do you find perst-gpios defined for the rk3399 ? >>>>>> Under Documentation/devicetree/bindings/pci/, the only schema I see using >>>>>> perst-gpios property are for the qcom (Qualcomm) controllers. >>>>> >>>>> You are right, it's so far only in Qualcomm. >>>>> >>>>>> The RC bindings for the rockchip rk3399 PCIe controller >>>>>> (pci/rockchip,rk3399-pcie.yaml) already define the ep-gpios property. So if >>>>> >>>>> Any reason why this cannot be named like GPIO? Is there already a user >>>>> of this in Linux kernel? Commit msg says nothing about this, so that's >>>>> why I would expect name matching the signal. >>>> >>>> The RC-mode PCIe controller node of the rk3399 DTS already defines the ep-gpios >>>> property for RC side PERST# signal handling. So we simply reused the exact same >>>> name to be consistent between RC and EP. I personnally have no preferences. If >>>> there is an effort to rename such signal with some preferred pattern, I will >>>> follow. For the EP node, there was no PERST signal handling in the driver and >>>> no property defined for it, so any name is fine. "perst-gpios" would indeed be >>>> a better name, but again, given that the RC controller node has ep-gpios, we >>>> reused that. What is your recommendation here ? >>> >>> Actually I don't know, perst and ep would work for me. If you do not >>> have code for this in the driver yet (nothing is shared between ep and >>> host), then maybe let's go with perst to match the actual name. >> >> That works for me. The other simple solution would be to move the RC node >> ep-gpios description to the common schema pci/rockchip,rk3399-pcie-common.yaml, >> maybe ? Otherwise, perst-gpios like the Qualcomm schemas would be nice too. > > Thinking more about this, I think moving the ep-gpios description to the common > schema is the right thing to do given that the driver uses common code between > RC and EP to get that property. But if that is not acceptable, I can rename it > and get that property in the controller EP mode initialization code. That will > be add a little more code in the driver. I forgot that it is actually the same hardware, so if host has "ep-gpios" already then EP mode should have the same property. Common schema is good idea. Best regards, Krzysztof
On 4/3/24 03:10, Krzysztof Kozlowski wrote: >> Thinking more about this, I think moving the ep-gpios description to the common >> schema is the right thing to do given that the driver uses common code between >> RC and EP to get that property. But if that is not acceptable, I can rename it >> and get that property in the controller EP mode initialization code. That will >> be add a little more code in the driver. > > I forgot that it is actually the same hardware, so if host has > "ep-gpios" already then EP mode should have the same property. Common > schema is good idea. OK. But this will conflict with the patch you sent to add the missing maxItem. Is that patch a fix or is it for 6.10 ? If it is the former, I can wait for next week to rebase on rc3 and avoid a conflict. > > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml index 6b62f6f58efe..9331d44d6963 100644 --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml @@ -30,6 +30,9 @@ properties: maximum: 32 default: 32 + ep-gpios: + description: Input GPIO configured for the PERST# signal. + required: - rockchip,max-outbound-regions