[2/2] PCI: iproc: Add optional brcm,pci-hotplug

Message ID 1502167455-10516-3-git-send-email-oza.oza@broadcom.com
State Superseded
Headers show

Commit Message

Oza Pawandeep Aug. 8, 2017, 4:44 a.m.
Add description for optional device tree property
'brcm,pci-hotplug' for PCI hotplug feature.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>

Comments

Rob Herring Aug. 8, 2017, 2:20 p.m. | #1
Please send bindings to DT list.

On Mon, Aug 7, 2017 at 11:44 PM, Oza Pawandeep <oza.oza@broadcom.com> wrote:
> Add description for optional device tree property
> 'brcm,pci-hotplug' for PCI hotplug feature.
>
> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>
> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> index b8e48b4..a3bad24 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> @@ -72,6 +72,29 @@ Optional properties:
>  - brcm,pcie-msi-inten: Needs to be present for some older iProc platforms that
>  require the interrupt enable registers to be set explicitly to enable MSI
>
> +Optional properties:
> +- brcm,pci-hotplug: PCI hotplug feature is supported.

I think we should make this a common property. We already have
"ibm,slot-pluggable", so I'd propose "slot-pluggable".

There's also "hotpluggable" for memory nodes defined, so we could
reuse that here.

> +
> +If the brcm,pcie-hotplug property is present, the following properties become
> +effective:
> +
> +- brcm,prsnt-gpio: Array of gpios, needs to be present if Hotplug is supported.

prsnt-gpios

> +
> +PCI hotplug implementation is SOC/Board specific, and also it depends on
> +how add-in card is designed (e.g. how many present pins are implemented).
> +
> +If x8 card is connected, then it might be possible that all the
> +3 present pins could go low, or at least one pin goes low.
> +
> +If x4 card is connected, then it might be possible that 2 present
> +pins go low, or at least one pin goes low.
> +
> +Example:
> +brcm,prsnt-gpio: <&pca9505 32 1>, <&pca9505 33 1>;
> +This is x4 connector: monitoring max 2 present lines.
> +brcm,prsnt-gpio: <&pca9505 32 1>, <&pca9505 33 1>, <&pca9505 34 1>;
> +This is x8 connector: monitoring max 3 present lines.
> +
>  Example:
>         pcie0: pcie@18012000 {
>                 compatible = "brcm,iproc-pcie";
> --
> 1.9.1
>
Oza Pawandeep Aug. 9, 2017, 5:22 a.m. | #2
On Tue, Aug 8, 2017 at 7:50 PM, Rob Herring <robh+dt@kernel.org> wrote:
> Please send bindings to DT list.

Sure, will do that.

>
> On Mon, Aug 7, 2017 at 11:44 PM, Oza Pawandeep <oza.oza@broadcom.com> wrote:
>> Add description for optional device tree property
>> 'brcm,pci-hotplug' for PCI hotplug feature.
>>
>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>
>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> index b8e48b4..a3bad24 100644
>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> @@ -72,6 +72,29 @@ Optional properties:
>>  - brcm,pcie-msi-inten: Needs to be present for some older iProc platforms that
>>  require the interrupt enable registers to be set explicitly to enable MSI
>>
>> +Optional properties:
>> +- brcm,pci-hotplug: PCI hotplug feature is supported.
>
> I think we should make this a common property. We already have
> "ibm,slot-pluggable", so I'd propose "slot-pluggable".
>
> There's also "hotpluggable" for memory nodes defined, so we could
> reuse that here.
>

ok I will rename this to
brcm,slot-pluggable

>> +
>> +If the brcm,pcie-hotplug property is present, the following properties become
>> +effective:
>> +
>> +- brcm,prsnt-gpio: Array of gpios, needs to be present if Hotplug is supported.
>
> prsnt-gpios

will take care.

>
>> +
>> +PCI hotplug implementation is SOC/Board specific, and also it depends on
>> +how add-in card is designed (e.g. how many present pins are implemented).
>> +
>> +If x8 card is connected, then it might be possible that all the
>> +3 present pins could go low, or at least one pin goes low.
>> +
>> +If x4 card is connected, then it might be possible that 2 present
>> +pins go low, or at least one pin goes low.
>> +
>> +Example:
>> +brcm,prsnt-gpio: <&pca9505 32 1>, <&pca9505 33 1>;
>> +This is x4 connector: monitoring max 2 present lines.
>> +brcm,prsnt-gpio: <&pca9505 32 1>, <&pca9505 33 1>, <&pca9505 34 1>;
>> +This is x8 connector: monitoring max 3 present lines.
>> +
>>  Example:
>>         pcie0: pcie@18012000 {
>>                 compatible = "brcm,iproc-pcie";
>> --
>> 1.9.1
>>
Ray Jui Aug. 9, 2017, 5:27 a.m. | #3
On 8/8/2017 10:22 PM, Oza Oza wrote:
> On Tue, Aug 8, 2017 at 7:50 PM, Rob Herring <robh+dt@kernel.org> wrote:
>> Please send bindings to DT list.
> Sure, will do that.
>
>> On Mon, Aug 7, 2017 at 11:44 PM, Oza Pawandeep <oza.oza@broadcom.com> wrote:
>>> Add description for optional device tree property
>>> 'brcm,pci-hotplug' for PCI hotplug feature.
>>>
>>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>> index b8e48b4..a3bad24 100644
>>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>> @@ -72,6 +72,29 @@ Optional properties:
>>>   - brcm,pcie-msi-inten: Needs to be present for some older iProc platforms that
>>>   require the interrupt enable registers to be set explicitly to enable MSI
>>>
>>> +Optional properties:
>>> +- brcm,pci-hotplug: PCI hotplug feature is supported.
>> I think we should make this a common property. We already have
>> "ibm,slot-pluggable", so I'd propose "slot-pluggable".
>>
>> There's also "hotpluggable" for memory nodes defined, so we could
>> reuse that here.
>>
> ok I will rename this to
> brcm,slot-pluggable

How's brcm,slot-pluggable a common property? It's still brcm specific. 
Didn't Rob propose either "slot-pluggable" or "hotpluggable"?

And note it goes to the generic PCI binding instead iProc PCIe specific 
binding.
Oza Pawandeep Aug. 9, 2017, 5:43 a.m. | #4
On Wed, Aug 9, 2017 at 10:57 AM, Ray Jui <ray.jui@broadcom.com> wrote:
>
>
> On 8/8/2017 10:22 PM, Oza Oza wrote:
>>
>> On Tue, Aug 8, 2017 at 7:50 PM, Rob Herring <robh+dt@kernel.org> wrote:
>>>
>>> Please send bindings to DT list.
>>
>> Sure, will do that.
>>
>>> On Mon, Aug 7, 2017 at 11:44 PM, Oza Pawandeep <oza.oza@broadcom.com>
>>> wrote:
>>>>
>>>> Add description for optional device tree property
>>>> 'brcm,pci-hotplug' for PCI hotplug feature.
>>>>
>>>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>>>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>> b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>> index b8e48b4..a3bad24 100644
>>>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>> @@ -72,6 +72,29 @@ Optional properties:
>>>>   - brcm,pcie-msi-inten: Needs to be present for some older iProc
>>>> platforms that
>>>>   require the interrupt enable registers to be set explicitly to enable
>>>> MSI
>>>>
>>>> +Optional properties:
>>>> +- brcm,pci-hotplug: PCI hotplug feature is supported.
>>>
>>> I think we should make this a common property. We already have
>>> "ibm,slot-pluggable", so I'd propose "slot-pluggable".
>>>
>>> There's also "hotpluggable" for memory nodes defined, so we could
>>> reuse that here.
>>>
>> ok I will rename this to
>> brcm,slot-pluggable
>
>
> How's brcm,slot-pluggable a common property? It's still brcm specific.
> Didn't Rob propose either "slot-pluggable" or "hotpluggable"?
>
> And note it goes to the generic PCI binding instead iProc PCIe specific
> binding.
>

Initially I thought, Rob suggested either "slot-pluggable".
followed by, "hotpluggable" since memory node already has such property.

but not sure in which generic pci binding I should add ?
should it be part of
Documentation/devicetree/bindings/pci/host-generic-pci.txt

Can you please clarify Rob ?

Regards,
Oza.
Oza Pawandeep Aug. 9, 2017, 5:52 a.m. | #5
On Wed, Aug 9, 2017 at 11:13 AM, Oza Oza <oza.oza@broadcom.com> wrote:
> On Wed, Aug 9, 2017 at 10:57 AM, Ray Jui <ray.jui@broadcom.com> wrote:
>>
>>
>> On 8/8/2017 10:22 PM, Oza Oza wrote:
>>>
>>> On Tue, Aug 8, 2017 at 7:50 PM, Rob Herring <robh+dt@kernel.org> wrote:
>>>>
>>>> Please send bindings to DT list.
>>>
>>> Sure, will do that.
>>>
>>>> On Mon, Aug 7, 2017 at 11:44 PM, Oza Pawandeep <oza.oza@broadcom.com>
>>>> wrote:
>>>>>
>>>>> Add description for optional device tree property
>>>>> 'brcm,pci-hotplug' for PCI hotplug feature.
>>>>>
>>>>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>>>>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>> b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>> index b8e48b4..a3bad24 100644
>>>>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>> @@ -72,6 +72,29 @@ Optional properties:
>>>>>   - brcm,pcie-msi-inten: Needs to be present for some older iProc
>>>>> platforms that
>>>>>   require the interrupt enable registers to be set explicitly to enable
>>>>> MSI
>>>>>
>>>>> +Optional properties:
>>>>> +- brcm,pci-hotplug: PCI hotplug feature is supported.
>>>>
>>>> I think we should make this a common property. We already have
>>>> "ibm,slot-pluggable", so I'd propose "slot-pluggable".
>>>>
>>>> There's also "hotpluggable" for memory nodes defined, so we could
>>>> reuse that here.
>>>>
>>> ok I will rename this to
>>> brcm,slot-pluggable
>>
>>
>> How's brcm,slot-pluggable a common property? It's still brcm specific.
>> Didn't Rob propose either "slot-pluggable" or "hotpluggable"?
>>
>> And note it goes to the generic PCI binding instead iProc PCIe specific
>> binding.
>>
>
> Initially I thought, Rob suggested either "slot-pluggable".
> followed by, "hotpluggable" since memory node already has such property.
>
> but not sure in which generic pci binding I should add ?
> should it be part of
> Documentation/devicetree/bindings/pci/host-generic-pci.txt
>
> Can you please clarify Rob ?
>
> Regards,
> Oza.

To add, every SOC might have different way of implementing hotplug.
so I suppose both the binding documents have to be updated.

Documentation/devicetree/bindings/pci/host-generic-pci.txt
which can have common boolean property
named "hotpluggable"

and SOC specific implementation can stay here
for e.g.
Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
brcm,prsnt-gpios

Rob and Ray:
please let me know how this sounds.

Regards,
Oza.
Rob Herring Aug. 10, 2017, 6 p.m. | #6
On Wed, Aug 9, 2017 at 12:52 AM, Oza Oza <oza.oza@broadcom.com> wrote:
> On Wed, Aug 9, 2017 at 11:13 AM, Oza Oza <oza.oza@broadcom.com> wrote:
>> On Wed, Aug 9, 2017 at 10:57 AM, Ray Jui <ray.jui@broadcom.com> wrote:
>>>
>>>
>>> On 8/8/2017 10:22 PM, Oza Oza wrote:
>>>>
>>>> On Tue, Aug 8, 2017 at 7:50 PM, Rob Herring <robh+dt@kernel.org> wrote:
>>>>>
>>>>> Please send bindings to DT list.
>>>>
>>>> Sure, will do that.
>>>>
>>>>> On Mon, Aug 7, 2017 at 11:44 PM, Oza Pawandeep <oza.oza@broadcom.com>
>>>>> wrote:
>>>>>>
>>>>>> Add description for optional device tree property
>>>>>> 'brcm,pci-hotplug' for PCI hotplug feature.
>>>>>>
>>>>>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>>>>>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>>> b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>>> index b8e48b4..a3bad24 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>>> @@ -72,6 +72,29 @@ Optional properties:
>>>>>>   - brcm,pcie-msi-inten: Needs to be present for some older iProc
>>>>>> platforms that
>>>>>>   require the interrupt enable registers to be set explicitly to enable
>>>>>> MSI
>>>>>>
>>>>>> +Optional properties:
>>>>>> +- brcm,pci-hotplug: PCI hotplug feature is supported.
>>>>>
>>>>> I think we should make this a common property. We already have
>>>>> "ibm,slot-pluggable", so I'd propose "slot-pluggable".
>>>>>
>>>>> There's also "hotpluggable" for memory nodes defined, so we could
>>>>> reuse that here.
>>>>>
>>>> ok I will rename this to
>>>> brcm,slot-pluggable
>>>
>>>
>>> How's brcm,slot-pluggable a common property? It's still brcm specific.
>>> Didn't Rob propose either "slot-pluggable" or "hotpluggable"?
>>>
>>> And note it goes to the generic PCI binding instead iProc PCIe specific
>>> binding.
>>>
>>
>> Initially I thought, Rob suggested either "slot-pluggable".
>> followed by, "hotpluggable" since memory node already has such property.

I didn't say which because I'm fine with either one. Pick the color of
the bikeshed.

>>
>> but not sure in which generic pci binding I should add ?
>> should it be part of
>> Documentation/devicetree/bindings/pci/host-generic-pci.txt

That's for generic CAM/ECAM hosts.

pci.txt would be the place.

>>
>> Can you please clarify Rob ?
>>
>> Regards,
>> Oza.
>
> To add, every SOC might have different way of implementing hotplug.
> so I suppose both the binding documents have to be updated.
>
> Documentation/devicetree/bindings/pci/host-generic-pci.txt
> which can have common boolean property
> named "hotpluggable"
>
> and SOC specific implementation can stay here
> for e.g.
> Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> brcm,prsnt-gpios

PRSNT# is a standard signal though, right? The h/w specific part would
be whether it is connected to GPIOs or the host controller has some
built-in controls. So yes, it should be documented if iproc-pcie uses
"prsnt-gpios", but it can still be a common property.

Rob

Patch

diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
index b8e48b4..a3bad24 100644
--- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -72,6 +72,29 @@  Optional properties:
 - brcm,pcie-msi-inten: Needs to be present for some older iProc platforms that
 require the interrupt enable registers to be set explicitly to enable MSI
 
+Optional properties:
+- brcm,pci-hotplug: PCI hotplug feature is supported.
+
+If the brcm,pcie-hotplug property is present, the following properties become
+effective:
+
+- brcm,prsnt-gpio: Array of gpios, needs to be present if Hotplug is supported.
+
+PCI hotplug implementation is SOC/Board specific, and also it depends on
+how add-in card is designed (e.g. how many present pins are implemented).
+
+If x8 card is connected, then it might be possible that all the
+3 present pins could go low, or at least one pin goes low.
+
+If x4 card is connected, then it might be possible that 2 present
+pins go low, or at least one pin goes low.
+
+Example:
+brcm,prsnt-gpio: <&pca9505 32 1>, <&pca9505 33 1>;
+This is x4 connector: monitoring max 2 present lines.
+brcm,prsnt-gpio: <&pca9505 32 1>, <&pca9505 33 1>, <&pca9505 34 1>;
+This is x8 connector: monitoring max 3 present lines.
+
 Example:
 	pcie0: pcie@18012000 {
 		compatible = "brcm,iproc-pcie";