diff mbox series

[v2,17/18] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

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

Commit Message

Damien Le Moal March 30, 2024, 4:19 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       | 3 +++
 1 file changed, 3 insertions(+)

Comments

Krzysztof Kozlowski March 30, 2024, 9:16 a.m. UTC | #1
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
Damien Le Moal March 31, 2024, 11:06 p.m. UTC | #2
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
>
Krzysztof Kozlowski April 1, 2024, 9:57 a.m. UTC | #3
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
Damien Le Moal April 1, 2024, 11:36 p.m. UTC | #4
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
>
Krzysztof Kozlowski April 2, 2024, 7:33 a.m. UTC | #5
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
Damien Le Moal April 2, 2024, 7:38 a.m. UTC | #6
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
>
Damien Le Moal April 2, 2024, 7:38 a.m. UTC | #7
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
>
Damien Le Moal April 2, 2024, 7:55 a.m. UTC | #8
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
>>
>
Krzysztof Kozlowski April 2, 2024, 6:10 p.m. UTC | #9
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
Damien Le Moal April 2, 2024, 11:23 p.m. UTC | #10
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 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..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