Page MenuHomeFreeBSD
Differential D44207

pci_host_generic: Use SYS_RES_MEMORY for the parent of I/O resource ranges
ClosedPublic
Actions

Authored by jhb on Mar 4 2024, 10:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jan 21, 4:09 PM
Unknown Object (File)
Fri, Jan 17, 3:19 PM
Unknown Object (File)
Thu, Jan 16, 9:00 PM
Unknown Object (File)
Wed, Jan 15, 8:15 PM
Unknown Object (File)
Sep 19 2024, 5:33 AM
Unknown Object (File)
Sep 12 2024, 10:31 PM
Unknown Object (File)
Sep 9 2024, 2:13 AM
Unknown Object (File)
Sep 8 2024, 8:49 PM
View All 25 Files
Subscribers
imp
markj

Details

Reviewers
andrew
dch
tuexen
jrtc27
Commits
rGc19f1ad1c252: pci_host_generic: Use SYS_RES_MEMORY for the parent of I/O resource ranges
Summary

When a SYS_RES_IOPORT resource crosses a pci_host_generic bridge, it
is translated into a memory access for an associated range, so use
SYS_RES_MEMORY for the resource allocated from the parent. This is
arguably a cleaner solution than having simplebus rewrite
SYS_RES_MEMORY to SYS_RES_IOPORT.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 56428
Build 53316: arc lint + arc unit

Event Timeline

jhb created this revision.Mar 4 2024, 10:44 PM
Herald added a subscriber: imp. · View Herald TranscriptMar 4 2024, 10:44 PM
jhb requested review of this revision.Mar 4 2024, 10:44 PM
Harbormaster completed remote builds in B56428: Diff 135371.Mar 4 2024, 10:44 PM
jhb added a parent revision: D44206: pci_host_generic: Simplify generic_pcie_translate_resource.Mar 4 2024, 10:44 PM
jhb added a reviewer: jrtc27.Mar 5 2024, 4:50 AM
jrtc27 added a comment.Mar 5 2024, 5:13 AM

What about all the other PCI controller drivers though? Various use ofw_pcib, for example.

jhb added a comment.Mar 6 2024, 3:20 PM

ofw_pcib doesn't pass child resources up to simplebus for allocation or activation, it uses bus_space_map directly with a new-bus method to get the bus_space_tag. I think this might be the only one passing SYS_RES_IOPORT up to simplebus and that if this works we might be able to remove all SYS_RES_IOPORT support from simplebus.

jrtc27 added a comment.Mar 6 2024, 4:46 PM
In D44207#1009367, @jhb wrote:

ofw_pcib doesn't pass child resources up to simplebus for allocation or activation, it uses bus_space_map directly with a new-bus method to get the bus_space_tag. I think this might be the only one passing SYS_RES_IOPORT up to simplebus and that if this works we might be able to remove all SYS_RES_IOPORT support from simplebus.

Should ofw_pcib not be doing it this way too then?

jhb added a comment.Mar 13 2024, 10:11 PM
In D44207#1009408, @jrtc27 wrote:
In D44207#1009367, @jhb wrote:

ofw_pcib doesn't pass child resources up to simplebus for allocation or activation, it uses bus_space_map directly with a new-bus method to get the bus_space_tag. I think this might be the only one passing SYS_RES_IOPORT up to simplebus and that if this works we might be able to remove all SYS_RES_IOPORT support from simplebus.

Should ofw_pcib not be doing it this way too then?

I have thought about changing ofw_pcib, but to be clear, ofw_pcib is not passing SYS_RES_IOPORT resources up to its parent bus driver currently.

jhb mentioned this in D45434: pci_host_generic: Permit multiple bridges to share the same I/O window.Jun 6 2024, 9:52 PM
jhb added a subscriber: markj.Wed, Jan 15, 3:12 PM

@markj can you test this change on your Ampere box?

markj added a comment.Wed, Jan 15, 8:18 PM
In D44207#1105839, @jhb wrote:

@markj can you test this change on your Ampere box?

Could you please rebase it past b313229969cc5? It doesn't apply to the latest main.

jhb updated this revision to Diff 149454.Fri, Jan 17, 2:56 PM

Rebase

Harbormaster completed remote builds in B61784: Diff 149454.Fri, Jan 17, 2:56 PM
jhb added a child revision: D48501: simplebus: Stop accepting SYS_RES_IOPORT resources.Fri, Jan 17, 2:59 PM
markj added a comment.Mon, Jan 20, 7:30 PM
In D44207#1105839, @jhb wrote:

@markj can you test this change on your Ampere box?

It boots fine with this change. Is there anything in particular you'd like to see?

jhb added a comment.Tue, Jan 21, 3:08 AM
In D44207#1107391, @markj wrote:
In D44207#1105839, @jhb wrote:

@markj can you test this change on your Ampere box?

It boots fine with this change. Is there anything in particular you'd like to see?

Booting fine is the main thing I wanted to see. Can you also apply D48501 (in the stack) on top of this and make sure it still boots fine?

markj added a comment.Tue, Jan 21, 2:18 PM
In D44207#1107878, @jhb wrote:
In D44207#1107391, @markj wrote:
In D44207#1105839, @jhb wrote:

@markj can you test this change on your Ampere box?

It boots fine with this change. Is there anything in particular you'd like to see?

Booting fine is the main thing I wanted to see. Can you also apply D48501 (in the stack) on top of this and make sure it still boots fine?

It still boots without problems, but this system boots using ACPI, I think that patch is a no-op here.

jhb added a comment.Tue, Jan 21, 3:21 PM
In D44207#1107979, @markj wrote:
In D44207#1107878, @jhb wrote:
In D44207#1107391, @markj wrote:
In D44207#1105839, @jhb wrote:

@markj can you test this change on your Ampere box?

It boots fine with this change. Is there anything in particular you'd like to see?

Booting fine is the main thing I wanted to see. Can you also apply D48501 (in the stack) on top of this and make sure it still boots fine?

It still boots without problems, but this system boots using ACPI, I think that patch is a no-op here.

Ah, fair enough.

jhb added a comment.Tue, Jan 21, 3:53 PM
In D44207#1108043, @jhb wrote:
In D44207#1107979, @markj wrote:
In D44207#1107878, @jhb wrote:
In D44207#1107391, @markj wrote:
In D44207#1105839, @jhb wrote:

@markj can you test this change on your Ampere box?

It boots fine with this change. Is there anything in particular you'd like to see?

Booting fine is the main thing I wanted to see. Can you also apply D48501 (in the stack) on top of this and make sure it still boots fine?

It still boots without problems, but this system boots using ACPI, I think that patch is a no-op here.

Ah, fair enough.

Actually, can you show me the parent devices of pcib0? I wonder if ACPI was passing these requests up to the arm64 nexus and if it was just treating I/O ports as memory?

jhb added a child revision: D48581: arm64 nexus: Remove support for I/O port resources.Tue, Jan 21, 4:01 PM
markj added a comment.Tue, Jan 21, 4:02 PM
In D44207#1108078, @jhb wrote:

Actually, can you show me the parent devices of pcib0? I wonder if ACPI was passing these requests up to the arm64 nexus and if it was just treating I/O ports as memory?

I saved devinfo -r output, I hope that's sufficient: https://reviews.freebsd.org/P655

jhb added a comment.Tue, Jan 21, 4:02 PM
In D44207#1108078, @jhb wrote:

Actually, can you show me the parent devices of pcib0? I wonder if ACPI was passing these requests up to the arm64 nexus and if it was just treating I/O ports as memory?

Or perhaps, just test the patch from D48581 (now linked as a child of this) on top of this patch (which I will merge to main shortly)

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Jan 21, 4:03 PM
Closed by commit rGc19f1ad1c252: pci_host_generic: Use SYS_RES_MEMORY for the parent of I/O resource ranges (authored by jhb). · Explain Why
This revision was automatically updated to reflect the committed changes.
jhb added a commit: rGc19f1ad1c252: pci_host_generic: Use SYS_RES_MEMORY for the parent of I/O resource ranges.

Revision Contents
Changeset List

PathSize
sys/
dev/
pci/
28 lines

Diff 135371

View Options

sys/dev/pci/pci_host_generic.c

Loading...