On Mon, Mar 5, 2012 at 12:32 PM, Jakub Jermar <[email protected]> wrote: > On 02/24/2012 09:53 PM, Ján Veselý wrote: >> please test/review > > I went through all your changes, but especially the ones related to > enabling the intelpci driver on ppc32. As a prove, here is a negligible > nit I found in ohci_batch.c: > > + assert(addr_to_phys(ohci_ep->td) == ed_head_td(ohci_batch->ed)); > + assert(addr_to_phys(ohci_ep->td) == ed_tail_td(ohci_batch->ed)); > +// const uint32_t pa = addr_to_phys(ohci_ep->td); > +// assert(pa == (ohci_batch->ed->td_head & ED_TDHEAD_PTR_MASK)); > +// assert(pa == (ohci_batch->ed->td_tail & ED_TDTAIL_PTR_MASK)); > > The commented lines look like they will stay forever.
thx for review, lines removed > > As for your approach for reusing the intelpci driver, can a similar > split of registers be found e.g. in Linux? yes in arch/powerpc/sysdev/grackle.c:58 (Grackle is MPC106) there is a call to setup_indirect_pci, with the same addresses that are now hard-coded in uspace/drv/infrastructure/rootmac that function (found in arch/powerpc/sysdev/indirect_pci.c:158) does almost the same thing as we do: - map cfg_addr_reg - check if cfg_data_reg is in the same page, map cfg_data_reg if not - set addresses to those regs to mapped values > > As we discussed on IRC, it still seems to me like at least something in > the definition of hc registers should be volatile, ergo ioport[8,16,32]_t. all registers are typed ioportX_t, see ohci_regs.h (OHCI uses MMIO regs), structres in hw_struct are just that 'memory structures' (much like buffers) hc needs to use memory access to access them, so unless compiler can decide to ignore/optimize-out writes to these structures entirely we are ok. (maybe missing a barrier or two) I wouldn't mind adding volatile to the declarations, but using ioportX_t is IMO bad idea as it will lead to confusion. I will split OHCI_WR/RD/SET/CLEAR to something like OHCI_REG_WR/RD/SET/CLR and OHCI_MEM_WR/RD/SET/CLR to make the distinction clearer. > > Otherwise, the changes look good. > >> PS: I removed all optical separators :) > > Hopefully you used some automated tool, only reading through those > changes was quite tedious. find + sed -i did work quite well thx for your comments, j.v. > > Cheers, > Jakub > > _______________________________________________ > HelenOS-devel mailing list > [email protected] > http://lists.modry.cz/cgi-bin/listinfo/helenos-devel _______________________________________________ HelenOS-devel mailing list [email protected] http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
