lkml.org 
[lkml]   [2017]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH v8 1/7] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
Hi Brian,

On 10/27/2017 10:33 AM, Brian Norris wrote:
> On Thu, Oct 26, 2017 at 09:28:34PM +0800, Jeffy Chen wrote:
>> Add optional interrupts for PCIe WAKE# pin and PCI interrupt pin.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>> Changes in v8:
>> Add optional "pci", and rewrite commit message.
>>
>> Changes in v7: None
>> Changes in v6: None
>> Changes in v5:
>> Move to pci.txt
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>> Documentation/devicetree/bindings/pci/pci.txt | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
>> index c77981c5dd18..faed405811cd 100644
>> --- a/Documentation/devicetree/bindings/pci/pci.txt
>> +++ b/Documentation/devicetree/bindings/pci/pci.txt
>> @@ -24,3 +24,6 @@ driver implementation may support the following properties:
>> unsupported link speed, for instance, trying to do training for
>> unsupported link speed, etc. Must be '4' for gen4, '3' for gen3, '2'
>> for gen2, and '1' for gen1. Any other values are invalid.
>> +- interrupts: Interrupt specifier for each name in interrupt-names.
>> +- interrupt-names: May contains "wakeup" for PCIe WAKE# interrupt and "pci"
>> + for PCI interrupt.
>
> Similar criticism to what Rob made on patch 4: this file already says "a
> host bridge driver implementation may support the following properties",
> so this property is clearly not for child devices. And so having the
> "PCI interrupt" here doesn't make much sense.
>
> Similarly, you're documenting "wakeup" here as a host bridge property,
> but then patch 7 is adding per-device support it seems? That seems
> wrong.
oops...so there's no section for PCI device here, maybe i should add a
section about "PCI device have standardized Device Tree bindings:" to
place it? will do in next version.


>
> In fact, I'm pretty sure this series fails to actually look in the host
> bridge for the "wakeup" interrupt at all! Did you actually test this?
actually it could...

static void *of_pci_setup(struct device *dev)
{
...
device_init_wakeup(dev, false);

dev_info(dev, "Wakeup IRQ %d\n", irq);
return data;
}

[ 1.546561] OF: PCI: MEM 0xfa000000..0xfbdfffff -> 0xfa000000
[ 1.553154] OF: PCI: IO 0xfbe00000..0xfbefffff -> 0xfbe00000
[ 1.560859] rockchip-pcie f8000000.pcie: Wakeup IRQ 64
[ 1.566555] rockchip-pcie f8000000.pcie: PCI host bridge to bus

>
> And again, describing your intentions a little better in the commit
> message would make this clearer. Then we could tell which way you
> intended this to work...
ok, will do in next version...
>
> Brian
>
>
>


\
 
 \ /
  Last update: 2017-10-27 05:08    [W:0.086 / U:0.596 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site