Hello, On Wed, 29 Mar 2017 15:19:39 +0900 Masanobu SAITOH <[email protected]> wrote:
> On 2017/03/24 8:13, Michael wrote: > > Hello, > > > > On Wed, 22 Mar 2017 16:35:21 +0900 > > Masanobu SAITOH <[email protected]> wrote: > > > >> A) modify pci_mapreg_map(). Stop setting BUS_SPACE_MAP_PREFETCHABLE > >> by defalut when prefetchable bit is set. If a driver really know > >> the whole area of the BAR is prefetchable, set > >> BUS_SPACE_MAP_PREFETCHABLE > >> in the 4th argument(busflags) of pci_mapreg_map(). pci_mapreg_map() > >> check for both the 4th argument and the prefetchable bit, it sets > >> BUS_SPACE_MAP_PREFETCHABLE only when both bits are set. > > > > On 2nd thought, I think we should ignore the prefetchable bit in the > > BAR here > > in pci_mapreg_map()? > > > and just go with whatever the driver wants, > > You mean: > > For pci_mapreg_map(): > > Don't check prefetchable bit and don't set BUS_SPACE_MAP_PREFETCHABLE > in pci_mapreg_map() like OpenBSD. > > For each driver side: > > Don't use pci_mapreg_map() and use pci_mapreg_info() and > bus_space_map() to use an area with BUS_SPACE_MAP_PREFETCHABLE > (Plan D in my original mail) > > Right? Let call this plan E. I don't know what OpenBSD does - I think the driver should decide wether to map something with write combining, prefetching or similar techniques applied, simply by passing BUS_SPACE_MAP_PREFETCHABLE, wether the BAR in question is marked as PREFETCHABLE or not. Mostly because the bit is not a reliable indicator - you brought examples where it's set but shouldn't, I brought some where it should be set but isn't. > The plan E is OK, but it makes code duplication and the come won't > be simple. To make code simple, How? The driver knows the hardware's capabilities, no need for the pci_mapreg_info() dance. Wether BUS_SPACE_MAP_PREFETCHABLE actually does anything is MD anyway. In some cases, like MIPS, it's even CPU model dependent. > > A) modify pci_mapreg_map(). Stop setting BUS_SPACE_MAP_PREFETCHABLE > > by defalut when prefetchable bit is set. If a driver really know > > the whole area of the BAR is prefetchable, set > > BUS_SPACE_MAP_PREFETCHABLE in the 4th argument(busflags) of > > pci_mapreg_map(). pci_mapreg_map() check for both the 4th argument > > and the prefetchable bit, it sets BUS_SPACE_MAP_PREFETCHABLE only > > when both bits are set. > > > > B) Don't modify pci_mapreg_ma(). Add new API like FreeBSD's > > pmap_change_attr() and use it when a driver want. > > > > C) make new function pci_mapreg_map_wc() > > > > D) Keep. If a driver know a BAR has prefetchable bit and not the > > whole area is prefetchable, do pci_mem_find() -> drop > > BUS_SPACE_MAP_PREFETCHABLE and do bus_space_map(). > > E) Don't check prefetchable bit and don't set > BUS_SPACE_MAP_PREFETCHABLE in any case in pci_mapreg_map() like > OpenBSD. Don't use pci_mapreg_map() and use pci_mapreg_info() and > bus_space_map() if a driver want to use an area with > BUS_SPACE_MAP_PREFETCHABLE. > > F) Any other solution(s). > > Plan A, B and C are useful than D and E to make code simple. > > > since you ran into > > devices which have the bit set and shouldn't, and I have seen > > graphics chips which don't set it for their video memory > > apertures. > > Even if we choose A or E, we have to check all driver > which already use pci_mapreg_map() and modify some of > them. > > For A: > - BAR has no prefetchable bit: > not required to modify > - BAR has prefetchable bit and BUS_SPACE_MAP_PREFETCHABLE can't > be used: not required to modify > - really prefetchable: > set BUS_SPACE_MAP_PREFETCHABLE in the 4th argument > > For D: > - BAR has no prefetchable bit: > not required to modify > - BAR has prefetchable bit and BUS_SPACE_MAP_PREFETCHABLE can't > be used: not required to modify > - really prefetchable: > Extract it with pci_mapreg_info() and bus_space_map() For E: - for drivers that don't set BUS_SPACE_MAP_PREFETCHABLE and don't want it - nothing to do here - drivers that would benefit from BUS_SPACE_MAP_PREFETCHABLE need to be changed to explicitly set it, but they will continue to function if not - drivers that set BUS_SPACE_MAP_PREFETCHABLE but really shouldn't - do they actually exist? have fun Michael
