diff mbox series

[v3,11/12] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

Message ID 20241007041218.157516-12-dlemoal@kernel.org (mailing list archive)
State New
Delegated to: Manivannan Sadhasivam
Headers show
Series [v3,01/12] PCI: rockchip-ep: Fix address translation unit programming | expand

Commit Message

Damien Le Moal Oct. 7, 2024, 4:12 a.m. UTC 
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      | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Krzysztof Kozlowski Oct. 7, 2024, 6:12 a.m. UTC | #1 
On Mon, Oct 07, 2024 at 01:12:17PM +0900, 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.

Why "ep" for PERST signal? Looks totally unrelated name. There is
already reset-gpios exactly for PERST, so you are duplicating it. Why?

Best regards,
Krzysztof
Damien Le Moal Oct. 7, 2024, 6:50 a.m. UTC | #2 
On 10/7/24 15:12, Krzysztof Kozlowski wrote:
> On Mon, Oct 07, 2024 at 01:12:17PM +0900, 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.
> 
> Why "ep" for PERST signal? Looks totally unrelated name. There is
> already reset-gpios exactly for PERST, so you are duplicating it. Why?

Because the host side controller already has the same "ep-gpios" property.

Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml

So naming that property the same allows common code to initialize that gpio in
rockchip_pcie_parse_dt().

Also, I do not see reset-gpios being defined/used by this driver (host and ep
sides).
Krzysztof Kozlowski Oct. 7, 2024, 6:54 a.m. UTC | #3 
On 07/10/2024 08:50, Damien Le Moal wrote:
> On 10/7/24 15:12, Krzysztof Kozlowski wrote:
>> On Mon, Oct 07, 2024 at 01:12:17PM +0900, 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.
>>
>> Why "ep" for PERST signal? Looks totally unrelated name. There is
>> already reset-gpios exactly for PERST, so you are duplicating it. Why?
> 
> Because the host side controller already has the same "ep-gpios" property.
> 
> Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml

If host has it, then it is a common property so goes to common schema
for these devices.

> 
> So naming that property the same allows common code to initialize that gpio in
> rockchip_pcie_parse_dt().
> 
> Also, I do not see reset-gpios being defined/used by this driver (host and ep
> sides).

I am talking about bindings, not driver.

Best regards,
Krzysztof
Damien Le Moal Oct. 7, 2024, 6:58 a.m. UTC | #4 
On 10/7/24 15:54, Krzysztof Kozlowski wrote:
> On 07/10/2024 08:50, Damien Le Moal wrote:
>> On 10/7/24 15:12, Krzysztof Kozlowski wrote:
>>> On Mon, Oct 07, 2024 at 01:12:17PM +0900, 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.
>>>
>>> Why "ep" for PERST signal? Looks totally unrelated name. There is
>>> already reset-gpios exactly for PERST, so you are duplicating it. Why?
>>
>> Because the host side controller already has the same "ep-gpios" property.
>>
>> Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
> 
> If host has it, then it is a common property so goes to common schema
> for these devices.

Ah. OK. I will move it to
Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-common.yaml then.

>> So naming that property the same allows common code to initialize that gpio in
>> rockchip_pcie_parse_dt().
>>
>> Also, I do not see reset-gpios being defined/used by this driver (host and ep
>> sides).
> 
> I am talking about bindings, not driver.

I do not see reset-gpios being defined in the bindings (common, host and ep).
resets and reset-names are defined though but these have nothing to do with
#PERST control.
Krzysztof Kozlowski Oct. 7, 2024, 7 a.m. UTC | #5 
On 07/10/2024 08:58, Damien Le Moal wrote:
> On 10/7/24 15:54, Krzysztof Kozlowski wrote:
>> On 07/10/2024 08:50, Damien Le Moal wrote:
>>> On 10/7/24 15:12, Krzysztof Kozlowski wrote:
>>>> On Mon, Oct 07, 2024 at 01:12:17PM +0900, 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.
>>>>
>>>> Why "ep" for PERST signal? Looks totally unrelated name. There is
>>>> already reset-gpios exactly for PERST, so you are duplicating it. Why?
>>>
>>> Because the host side controller already has the same "ep-gpios" property.
>>>
>>> Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
>>
>> If host has it, then it is a common property so goes to common schema
>> for these devices.
> 
> Ah. OK. I will move it to
> Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-common.yaml then.
> 
>>> So naming that property the same allows common code to initialize that gpio in
>>> rockchip_pcie_parse_dt().
>>>
>>> Also, I do not see reset-gpios being defined/used by this driver (host and ep
>>> sides).
>>
>> I am talking about bindings, not driver.
> 
> I do not see reset-gpios being defined in the bindings (common, host and ep).
> resets and reset-names are defined though but these have nothing to do with
> #PERST control.

Bindings for all PCI devices. See pci-bus-common.yaml

Best regards,
Krzysztof
Damien Le Moal Oct. 7, 2024, 7:22 a.m. UTC | #6 
On 10/7/24 16:00, Krzysztof Kozlowski wrote:
>> I do not see reset-gpios being defined in the bindings (common, host and ep).
>> resets and reset-names are defined though but these have nothing to do with
>> #PERST control.
> 
> Bindings for all PCI devices. See pci-bus-common.yaml

Got it. But in this case, since ep-gpios is already defined for the RC host mode
controller, isn't it simpler to simply move that property to
rockchip,rk3399-pcie-common.yaml ?

I can of course instead re-use the reset-gpios property for the endpoint mode,
but that will need a bit more code in the driver.

Which way do you recommend ?
Manivannan Sadhasivam Oct. 7, 2024, 7:27 a.m. UTC | #7 
On Mon, Oct 07, 2024 at 04:22:17PM +0900, Damien Le Moal wrote:
> On 10/7/24 16:00, Krzysztof Kozlowski wrote:
> >> I do not see reset-gpios being defined in the bindings (common, host and ep).
> >> resets and reset-names are defined though but these have nothing to do with
> >> #PERST control.
> > 
> > Bindings for all PCI devices. See pci-bus-common.yaml
> 
> Got it. But in this case, since ep-gpios is already defined for the RC host mode
> controller, isn't it simpler to simply move that property to
> rockchip,rk3399-pcie-common.yaml ?
> 
> I can of course instead re-use the reset-gpios property for the endpoint mode,
> but that will need a bit more code in the driver.
> 
> Which way do you recommend ?
> 

Please use 'reset-gpios' instead. Using 'ep-gpios' for the endpoint controller
doesn't convey the actual use.

- Mani
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
index 6b62f6f58efe..a8970bda7174 100644
--- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
@@ -30,6 +30,10 @@  properties:
     maximum: 32
     default: 32
 
+  ep-gpios:
+    description: Input GPIO configured for the PERST# signal
+    maxItems: 1
+
 required:
   - rockchip,max-outbound-regions