| Message ID | 20240812005929.113499-9-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V: ACPI: Add external interrupt controller support | expand |
On Mon, Aug 12, 2024 at 06:29:20AM +0530, Sunil V L wrote: > RISC-V platforms need to use dependencies between PCI host bridge, Link > devices and the interrupt controllers to ensure probe order. The > dependency is like below. > > Interrupt controller <-- Link Device <-- PCI Host bridge. > > If there is no dependency between Link device and PCI Host Bridge, > then PCI devices may be probed prior to Link devices. If a PCI > device is probed before its Link device, we won't be able to find > its INTx mapping. This seems to explain why we want these dependencies, which is useful, but *this* patch only removes the dependencies. Maybe this description should be in the patch that *adds* the dependencies, e.g., "ACPI: RISC-V: Implement function to add implicit dependencies"? > So, add the link device's HID to dependency honor list and clear the > dependency after probe is done so that the dependent devices are > unblocked to probe. This still claims this patch adds HID, which I don't think it does. > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > Tested-by: Björn Töpel <bjorn@rivosinc.com> > --- > drivers/acpi/pci_link.c | 2 ++ > drivers/acpi/scan.c | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > index aa1038b8aec4..b727db968f33 100644 > --- a/drivers/acpi/pci_link.c > +++ b/drivers/acpi/pci_link.c > @@ -748,6 +748,8 @@ static int acpi_pci_link_add(struct acpi_device *device, > if (result) > kfree(link); > > + acpi_dev_clear_dependencies(device); > + > return result < 0 ? result : 1; > } > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 28a221f956d7..753539a1f26b 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -863,6 +863,7 @@ static const char * const acpi_honor_dep_ids[] = { > "INTC10CF", /* IVSC (MTL) driver must be loaded to allow i2c access to camera sensors */ > "RSCV0001", /* RISC-V PLIC */ > "RSCV0002", /* RISC-V APLIC */ > + "PNP0C0F", /* PCI Link Device */ > NULL > }; > > -- > 2.43.0 >
Hi Bjorn, On Thu, Aug 22, 2024 at 04:44:15PM -0500, Bjorn Helgaas wrote: > On Mon, Aug 12, 2024 at 06:29:20AM +0530, Sunil V L wrote: > > RISC-V platforms need to use dependencies between PCI host bridge, Link > > devices and the interrupt controllers to ensure probe order. The > > dependency is like below. > > > > Interrupt controller <-- Link Device <-- PCI Host bridge. > > > > If there is no dependency between Link device and PCI Host Bridge, > > then PCI devices may be probed prior to Link devices. If a PCI > > device is probed before its Link device, we won't be able to find > > its INTx mapping. > > This seems to explain why we want these dependencies, which is useful, > but *this* patch only removes the dependencies. > > Maybe this description should be in the patch that *adds* the > dependencies, e.g., "ACPI: RISC-V: Implement function to add implicit > dependencies"? > Okay. Let me move this to the patch you suggested. > > So, add the link device's HID to dependency honor list and clear the > > dependency after probe is done so that the dependent devices are > > unblocked to probe. > > This still claims this patch adds HID, which I don't think it does. > Please see below. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > Tested-by: Björn Töpel <bjorn@rivosinc.com> > > --- > > drivers/acpi/pci_link.c | 2 ++ > > drivers/acpi/scan.c | 1 + > > 2 files changed, 3 insertions(+) > > > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > > index aa1038b8aec4..b727db968f33 100644 > > --- a/drivers/acpi/pci_link.c > > +++ b/drivers/acpi/pci_link.c > > @@ -748,6 +748,8 @@ static int acpi_pci_link_add(struct acpi_device *device, > > if (result) > > kfree(link); > > > > + acpi_dev_clear_dependencies(device); > > + > > return result < 0 ? result : 1; > > } > > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > index 28a221f956d7..753539a1f26b 100644 > > --- a/drivers/acpi/scan.c > > +++ b/drivers/acpi/scan.c > > @@ -863,6 +863,7 @@ static const char * const acpi_honor_dep_ids[] = { > > "INTC10CF", /* IVSC (MTL) driver must be loaded to allow i2c access to camera sensors */ > > "RSCV0001", /* RISC-V PLIC */ > > "RSCV0002", /* RISC-V APLIC */ > > + "PNP0C0F", /* PCI Link Device */ This is the change which I meant adding HID to the honor list. Do you recommend to make this change separate patch so that it doesn't confuse with adding a new HID to the probe match table? Thanks! Sunil
On Fri, Aug 23, 2024 at 12:03:25PM +0530, Sunil V L wrote: > On Thu, Aug 22, 2024 at 04:44:15PM -0500, Bjorn Helgaas wrote: > > On Mon, Aug 12, 2024 at 06:29:20AM +0530, Sunil V L wrote: > > > RISC-V platforms need to use dependencies between PCI host bridge, Link > > > devices and the interrupt controllers to ensure probe order. The > > > dependency is like below. > > > > > > Interrupt controller <-- Link Device <-- PCI Host bridge. > > > > > > If there is no dependency between Link device and PCI Host Bridge, > > > then PCI devices may be probed prior to Link devices. If a PCI > > > device is probed before its Link device, we won't be able to find > > > its INTx mapping. > > > > This seems to explain why we want these dependencies, which is useful, > > but *this* patch only removes the dependencies. > > > > Maybe this description should be in the patch that *adds* the > > dependencies, e.g., "ACPI: RISC-V: Implement function to add implicit > > dependencies"? > > > Okay. Let me move this to the patch you suggested. Given my forgetfulness and your pointing out that this *does* add the dependencies (by virtue of adding PNP0C0F to the acpi_honor_dep_ids[] list), it does make sense here. > > > So, add the link device's HID to dependency honor list and clear the > > > dependency after probe is done so that the dependent devices are > > > unblocked to probe. Maybe expanding this to "Add the link devices HID (PNP0C0F) to the acpi_honor_dep_ids[] dependency list" would help connect this all together? > > This still claims this patch adds HID, which I don't think it does. > > > Please see below. > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > > Tested-by: Björn Töpel <bjorn@rivosinc.com> > > > --- > > > drivers/acpi/pci_link.c | 2 ++ > > > drivers/acpi/scan.c | 1 + > > > 2 files changed, 3 insertions(+) > > > > > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > > > index aa1038b8aec4..b727db968f33 100644 > > > --- a/drivers/acpi/pci_link.c > > > +++ b/drivers/acpi/pci_link.c > > > @@ -748,6 +748,8 @@ static int acpi_pci_link_add(struct acpi_device *device, > > > if (result) > > > kfree(link); > > > > > > + acpi_dev_clear_dependencies(device); > > > + > > > return result < 0 ? result : 1; > > > } > > > > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > > index 28a221f956d7..753539a1f26b 100644 > > > --- a/drivers/acpi/scan.c > > > +++ b/drivers/acpi/scan.c > > > @@ -863,6 +863,7 @@ static const char * const acpi_honor_dep_ids[] = { > > > "INTC10CF", /* IVSC (MTL) driver must be loaded to allow i2c access to camera sensors */ > > > "RSCV0001", /* RISC-V PLIC */ > > > "RSCV0002", /* RISC-V APLIC */ > > > + "PNP0C0F", /* PCI Link Device */ > > This is the change which I meant adding HID to the honor list. Do you > recommend to make this change separate patch so that it doesn't confuse > with adding a new HID to the probe match table? Oh, right, sorry. I remember working this out in the past, but I had forgotten. I think it makes sense in this patch because the add and removal are matched when they're in the same patch. Sorry for the noise! Bjorn
diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c index aa1038b8aec4..b727db968f33 100644 --- a/drivers/acpi/pci_link.c +++ b/drivers/acpi/pci_link.c @@ -748,6 +748,8 @@ static int acpi_pci_link_add(struct acpi_device *device, if (result) kfree(link); + acpi_dev_clear_dependencies(device); + return result < 0 ? result : 1; } diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 28a221f956d7..753539a1f26b 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -863,6 +863,7 @@ static const char * const acpi_honor_dep_ids[] = { "INTC10CF", /* IVSC (MTL) driver must be loaded to allow i2c access to camera sensors */ "RSCV0001", /* RISC-V PLIC */ "RSCV0002", /* RISC-V APLIC */ + "PNP0C0F", /* PCI Link Device */ NULL };