Messages in this thread | | | Date | Fri, 27 Oct 2017 11:06:34 +0800 | From | jeffy <> | Subject | Re: [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 > > >
| |