On Tue, May 15, 2012 at 10:59:36AM -0400, Alan Stern wrote:
> On Tue, 15 May 2012, Felipe Balbi wrote:
>
> > > > Unrelated to $SUBJECT, but all those quirks could be passed through
> > > > driver_data from pci ID table. That will remove the need for the switch
> > > > statement. My 2 cents
> > >
> > > Many of them could, it's true. I don't know that it would be much of
> > > an improvement, though. The code would still have to test for most of
> > > the quirk flags because they require runtime checks (the one added by
> > > this patch is an exception).
> > >
> > > Also, which do you think would occupy less memory space: an array of
> > > pci_device_id structures or a switch statement jump table?
> >
> > The array gets freed after init with proper section annotations. I also
>
> Then what happens if you unbind ehci-hcd from a controller and rebind
> it later?
if you use "proper" section annotations that won't be a problem.
__devinitconst won't free that table in such a case.
> > don't understand why you say you still require runtime checks. The only
>
> Let's look through the code. The first NVIDIA handler does this:
>
> if (pci_set_consistent_dma_mask(pdev,
> DMA_BIT_MASK(31)) < 0)
> ehci_warn(ehci, "can't enable NVidia "
> "workaround for >2GB RAM\n");
you could set a flag for this, then check on probe()
if (data->needs_nvidia_dma_workaround)
pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(31));
> One of the clauses in the Intel and TDI handlers does this:
>
> hcd->has_tt = 1;
> tdi_reset(ehci);
if (data->intel_tdi_handler) {
hcd->has_tt = true;
tdi_reset(ehci);
}
> The AMD handler does this:
>
> if (usb_amd_find_chipset_info())
this only finds correct Vendor/Product ID pairs. Can be turned into
PCI_DEVICE() entries, although I'm missing something. Not very PCI-savvy
> ehci->amd_pll_fix = 1;
>
> I could go on. The point is that all of these involve more than just
> copying flag bits.
>
> > problem I see is that the way the *HCI PCI drivers were written, we
> > do all quirk checks too late and don't have access to the pci_device_id
> > pointer anymore. Still, all your switch jump table could be converted
> > into something like:
> >
> > ehci->no_selective_suspend = data->no_selective_suspend;
> > ehci->has_fsl_port_bug = data->has_fsl_port_bug;
> > ehci->big_endian_desc = data->big_endian_desc;
> > ehci->big_endian_capbase = data->big_endian_capbase;
> > ehci->has_amcc_usb23 = data->has_amcc_usb23;
> > ehci->need_io_watchdog = data->need_io_watchdog;
> > ehci->broken_periodic = data->broken_periodic;
> > ehci->amd_pll_fix = data->amd_pll_fix;
> > ehci->fs_i_thresh = data->fs_i_thresh;
> > ehci->use_dummy_qh = data->use_dummy_qh;
> > ehci->has_synopsys_hc_bug = data->has_synopsys_hc_bug;
> > ehci->frame_index_bug = data->frame_index_bug;
> >
> > #ifdef CONFIG_USB_EHCI_BIG_ENDIAN_MMIO
> > ehci->big_endian_mmio = data->big_endian_mmio;
> > #endif
> >
> > Because all those field were properly initialized on pci_device_id. It
> > would look something like:
> >
> > static const struct
> > ehci_pci_driver_data ehci_intel_27cc_driver_data __devinitconst = {
> > .fs_i_thresh = true,
> > .broken_periodic = true,
> > };
> >
> > static DEFINE_PCI_DEVICE_TABLE(pci_ids) = {
> > PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x27cc)
> > .driver_data = (kernel_ulong_t) &ehci_intel_27cc_driver_data,
> > };
> >
> > or something similar. The only thing I saw was the VIA VT6212 which uses
> > a different sleep time, but that register write can be done no matter if
> > it's VIA or not... just write the 10usec to the register on all cases.
> > It won't (shouldn't) do any harm.
>
> See all the examples listed above. Your scheme won't handle them.
Not the way it's done currently, no. The thing is that we have
driver_data for a reason and that's not to pass the driver itself, IMHO.
--
balbi
Attachment:
signature.asc
Description: Digital signature
