| Message ID | 1436979285-8177-4-git-send-email-ddaney.cavm@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi David, On Wed, Jul 15, 2015 at 05:54:43PM +0100, David Daney wrote: > From: David Daney <david.daney@cavium.com> > > The default is to continue doing the what we have done before, but add > a hook so that this can be overridden. This commit log is inappropriate. On top of that, it is already complicated enough to keep ARM and ARM64 in sync, I can't even imagine what would happen if we added a hook to pcibios_add_device so that RCs can implement their quirks beyond ARM64 core control. > Signed-off-by: David Daney <david.daney@cavium.com> > --- > arch/arm64/include/asm/pci.h | 3 +++ > arch/arm64/kernel/pci.c | 10 ++++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h > index b008a72..ad3fb18 100644 > --- a/arch/arm64/include/asm/pci.h > +++ b/arch/arm64/include/asm/pci.h > @@ -37,6 +37,9 @@ static inline int pci_proc_domain(struct pci_bus *bus) > { > return 1; > } > + > +void set_pcibios_add_device(int (*arg)(struct pci_dev *)); > + > #endif /* CONFIG_PCI */ > > #endif /* __KERNEL__ */ > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > index 4095379..3356023 100644 > --- a/arch/arm64/kernel/pci.c > +++ b/arch/arm64/kernel/pci.c > @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, > return res->start; > } > > +static int (*pcibios_add_device_impl)(struct pci_dev *); > + > +void set_pcibios_add_device(int (*arg)(struct pci_dev *)) > +{ > + pcibios_add_device_impl = arg; > +} > + > /* > * Try to assign the IRQ number from DT when adding a new device > */ > int pcibios_add_device(struct pci_dev *dev) > { > + if (pcibios_add_device_impl) > + return pcibios_add_device_impl(dev); I am totally against this (and to be honest by reading the other patches I failed to understand why you even need it), see above. Thanks, Lorenzo > + > dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); > > return 0; > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
On 07/16/2015 02:04 AM, Lorenzo Pieralisi wrote: > Hi David, > > On Wed, Jul 15, 2015 at 05:54:43PM +0100, David Daney wrote: >> From: David Daney <david.daney@cavium.com> >> [...] >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c >> index 4095379..3356023 100644 >> --- a/arch/arm64/kernel/pci.c >> +++ b/arch/arm64/kernel/pci.c >> @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, >> return res->start; >> } >> >> +static int (*pcibios_add_device_impl)(struct pci_dev *); >> + >> +void set_pcibios_add_device(int (*arg)(struct pci_dev *)) >> +{ >> + pcibios_add_device_impl = arg; >> +} >> + >> /* >> * Try to assign the IRQ number from DT when adding a new device >> */ >> int pcibios_add_device(struct pci_dev *dev) >> { >> + if (pcibios_add_device_impl) >> + return pcibios_add_device_impl(dev); > > I am totally against this (and to be honest by reading the other > patches I failed to understand why you even need it), see above. > It is because ... > Thanks, > Lorenzo > >> + >> dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); ... this is total crap. But I didn't want to break existing systems. The PCI RC drivers need a way to configure the legacy virtual-wire interrupts, because the existing code doesn't do it. David Daney
Hi David, On Thu, Jul 16, 2015 at 06:00:36PM +0100, David Daney wrote: > On 07/16/2015 02:04 AM, Lorenzo Pieralisi wrote: > > Hi David, > > > > On Wed, Jul 15, 2015 at 05:54:43PM +0100, David Daney wrote: > >> From: David Daney <david.daney@cavium.com> > >> > [...] > >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > >> index 4095379..3356023 100644 > >> --- a/arch/arm64/kernel/pci.c > >> +++ b/arch/arm64/kernel/pci.c > >> @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, > >> return res->start; > >> } > >> > >> +static int (*pcibios_add_device_impl)(struct pci_dev *); > >> + > >> +void set_pcibios_add_device(int (*arg)(struct pci_dev *)) > >> +{ > >> + pcibios_add_device_impl = arg; > >> +} > >> + > >> /* > >> * Try to assign the IRQ number from DT when adding a new device > >> */ > >> int pcibios_add_device(struct pci_dev *dev) > >> { > >> + if (pcibios_add_device_impl) > >> + return pcibios_add_device_impl(dev); > > > > I am totally against this (and to be honest by reading the other > > patches I failed to understand why you even need it), see above. > > > > It is because ... > > > > Thanks, > > Lorenzo > > > >> + > >> dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); > > > ... this is total crap. But I didn't want to break existing systems. That's a good aim, but you are still failing to explain the issue properly I am afraid. > The PCI RC drivers need a way to configure the legacy virtual-wire > interrupts, because the existing code doesn't do it. Can I ask you please to explain the issue a bit more clearly (and why the OF API does not work for you ?) Thank you ! Lorenzo
On 07/17/2015 04:00 AM, Lorenzo Pieralisi wrote: > Hi David, > > On Thu, Jul 16, 2015 at 06:00:36PM +0100, David Daney wrote: >> On 07/16/2015 02:04 AM, Lorenzo Pieralisi wrote: >>> Hi David, >>> >>> On Wed, Jul 15, 2015 at 05:54:43PM +0100, David Daney wrote: >>>> From: David Daney <david.daney@cavium.com> >>>> >> [...] >>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c >>>> index 4095379..3356023 100644 >>>> --- a/arch/arm64/kernel/pci.c >>>> +++ b/arch/arm64/kernel/pci.c >>>> @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, >>>> return res->start; >>>> } >>>> >>>> +static int (*pcibios_add_device_impl)(struct pci_dev *); >>>> + >>>> +void set_pcibios_add_device(int (*arg)(struct pci_dev *)) >>>> +{ >>>> + pcibios_add_device_impl = arg; >>>> +} >>>> + >>>> /* >>>> * Try to assign the IRQ number from DT when adding a new device >>>> */ >>>> int pcibios_add_device(struct pci_dev *dev) >>>> { >>>> + if (pcibios_add_device_impl) >>>> + return pcibios_add_device_impl(dev); >>> >>> I am totally against this (and to be honest by reading the other >>> patches I failed to understand why you even need it), see above. >>> >> >> It is because ... >> >> >>> Thanks, >>> Lorenzo >>> >>>> + >>>> dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); >> >> >> ... this is total crap. But I didn't want to break existing systems. > > That's a good aim, but you are still failing to explain the issue properly > I am afraid. > >> The PCI RC drivers need a way to configure the legacy virtual-wire >> interrupts, because the existing code doesn't do it. > > Can I ask you please to explain the issue a bit more clearly (and why > the OF API does not work for you ?) Several problems: 1) It prints many times to the boot log this string: pci 0000:01:0e.2: of_irq_parse_pci() failed with rc=-19 2) For a RC with no device_node it does nothing (in addition to printing the annoying message). > > Thank you ! > Lorenzo >
On Fri, Jul 17, 2015 at 05:38:45PM +0100, David Daney wrote: > On 07/17/2015 04:00 AM, Lorenzo Pieralisi wrote: > > Hi David, > > > > On Thu, Jul 16, 2015 at 06:00:36PM +0100, David Daney wrote: > >> On 07/16/2015 02:04 AM, Lorenzo Pieralisi wrote: > >>> Hi David, > >>> > >>> On Wed, Jul 15, 2015 at 05:54:43PM +0100, David Daney wrote: > >>>> From: David Daney <david.daney@cavium.com> > >>>> > >> [...] > >>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > >>>> index 4095379..3356023 100644 > >>>> --- a/arch/arm64/kernel/pci.c > >>>> +++ b/arch/arm64/kernel/pci.c > >>>> @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, > >>>> return res->start; > >>>> } > >>>> > >>>> +static int (*pcibios_add_device_impl)(struct pci_dev *); > >>>> + > >>>> +void set_pcibios_add_device(int (*arg)(struct pci_dev *)) > >>>> +{ > >>>> + pcibios_add_device_impl = arg; > >>>> +} > >>>> + > >>>> /* > >>>> * Try to assign the IRQ number from DT when adding a new device > >>>> */ > >>>> int pcibios_add_device(struct pci_dev *dev) > >>>> { > >>>> + if (pcibios_add_device_impl) > >>>> + return pcibios_add_device_impl(dev); > >>> > >>> I am totally against this (and to be honest by reading the other > >>> patches I failed to understand why you even need it), see above. > >>> > >> > >> It is because ... > >> > >> > >>> Thanks, > >>> Lorenzo > >>> > >>>> + > >>>> dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); > >> > >> > >> ... this is total crap. But I didn't want to break existing systems. > > > > That's a good aim, but you are still failing to explain the issue properly > > I am afraid. > > > >> The PCI RC drivers need a way to configure the legacy virtual-wire > >> interrupts, because the existing code doesn't do it. > > > > Can I ask you please to explain the issue a bit more clearly (and why > > the OF API does not work for you ?) > > Several problems: > > 1) It prints many times to the boot log this string: > pci 0000:01:0e.2: of_irq_parse_pci() failed with rc=-19 which happens because...? That's -ENODEV. As far as I can tell, that only gets printed if the device doesn't have a wired interrupt (the interrupt pin field in config space read as zero). So that's beningn, but annoying? Surely we can clean that up. > 2) For a RC with no device_node it does nothing (in addition to printing > the annoying message). Your root complex has a device_node if it is described in DT. Is the problem that this doesn't work with ACPI? Thanks, Mark.
diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h index b008a72..ad3fb18 100644 --- a/arch/arm64/include/asm/pci.h +++ b/arch/arm64/include/asm/pci.h @@ -37,6 +37,9 @@ static inline int pci_proc_domain(struct pci_bus *bus) { return 1; } + +void set_pcibios_add_device(int (*arg)(struct pci_dev *)); + #endif /* CONFIG_PCI */ #endif /* __KERNEL__ */ diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 4095379..3356023 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, return res->start; } +static int (*pcibios_add_device_impl)(struct pci_dev *); + +void set_pcibios_add_device(int (*arg)(struct pci_dev *)) +{ + pcibios_add_device_impl = arg; +} + /* * Try to assign the IRQ number from DT when adding a new device */ int pcibios_add_device(struct pci_dev *dev) { + if (pcibios_add_device_impl) + return pcibios_add_device_impl(dev); + dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); return 0;