Re: [OpenBIOS] [PATCH v3] Add USB OHCI + HID driver

[Alexander Graf]

On 26.06.14 12:55, BALATON Zoltan wrote:
> On Thu, 26 Jun 2014, Alexander Graf wrote:
> > On 26.06.14 00:36, BALATON Zoltan wrote:
> > > On Thu, 26 Jun 2014, Alexander Graf wrote:
> > > > On 01.06.14 23:55, BALATON Zoltan wrote:
> > > > > This driver is ported from CoreBoot's libpayload and fixed to work on
> > > > > big endian CPUs (tested on PPC with QEMU). It is enough to support a
> > > > > USB keyboard on an Apple Keylargo OHCI compliant USB host and makes
> > > > > OpenBIOS usable on qemu-system-ppc64 with mac99 machine type as well
> > > > > as allows to emulate PowerMac3,1 better.
> > > > >
> > > > > Signed-off-by: BALATON Zoltan 
> > > > > Acked-by: Alexander Graf
> > > > >
> > > > > v2: improve init and teardown by moving pci configuration to the 
> > > > > driver
> > > > >      and giving up using the controller on quiesce interface call 
> > > > > also
> > > > >      resetting it to avoid runaway DMA until the OS driver takes over
> > > > > v3: Only enable bus master bit in PCI config for QEMU
> > > >
> > > > [...]
> > > > > +hci_t *
> > > > > +ohci_pci_init (pci_addr addr)
> > > > > +{
> > > > > u32 reg_base;
> > > > >
> > > > > +#if defined(CONFIG_QEMU)
> > > > > uint16_t cmd;
> > > > > cmd = pci_config_read16(addr, PCI_COMMAND);
> > > > > cmd |= PCI_COMMAND_BUS_MASTER;
> > > > > pci_config_write16(addr, PCI_COMMAND, cmd);
> > > > >
> > > > > +#endif
> > > >
> > > > I don't think the driver will work without bus mastering.
> > >
> > > So then apply the first version without this #if.
> > > > Just don't match on it on non-QEMU machines and add a comment why 
> > > > you don't.
> > >
> > > Why not? Coreboot is supposed to work on real hardware too.
> >
> > No, it won't because it won't have bus mastering enabled. Do you want 
> > to be the guy trying to debug why OHCI support doesn't only to find 
> > out that it's because bus mastering isn't on?
>
> I'm afraid I'll be the one to debug it if it breaks anyway... If it 
> needs bus mastering everywhere then apply v2 without special casing 
> QEMU. (You asked to put ifdefs around enabling bus mastering.) What I 
> don't see is why should we special case this driver for QEMU if
> It's supposed to works on real hardware too as it is used in CoreBoot
> in such situations
> I'm not aware of any usage of OpenBIOS on anything else than QEMU

OpenBIOS started off on real Macs :). I don't know if anyone still uses 
those targets.
> So restricting this driver for QEMU seems an unnecessary complication.
> What am I missing?

If you enable bus mastering for the device without taking a lot of care 
for its PCI host controller, the system might just crash. That's why we 
don't want to enable bus mastering on devices for configurations where 
we aren't sure it works (read: non-QEMU targets).

So either you can give me a lot of confidence that the OHCI driver does 
not do any DMA and thus works just fine without bus mastering enabled 
and only requires it enabled because of MorphOS again (in which case 
that should be documented in a comment!) or the driver is completely 
useless on non-QEMU platforms and shouldn't be around because it would 
probably just time out.

Alex