On Thu, 26 Jun 2014, Alexander Graf wrote:
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 balaton@eik.bme.hu Acked-by: Alexander Graf agraf@suse.de
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 why not let them try and yell if it breaks if they are still around? A USB driver would be useful for real Macs with USB keyboard so they might want to try and fix it anyway. (Maybe that's why noone uses those targets any more.)
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.
If OpenBIOS takes enough care with configuring the host controller then it should not crash. If it doesn't then that's not a bug in the OHCI driver so why should it be disabled?
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).
Isn't that overly cautious and just making code more complicated by putting conditionals everywhere for no good reason other then the fear of breaking something that might not be used any more anyway? I could put another conditional around the definition of OHCI in pci_database.c and have it only enabled for QEMU but I'm arguing that it should work on real hardware and thus I don't see why it should not be enabled until someone finds it crashes there and even then it would be better to fix the host controller config than disabling this driver.
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
No, I think it needs to do DMA and bus mastering enabled. Here is the thread where it was established:
http://lists.nongnu.org/archive/html/qemu-devel/2014-05/msg06587.html
requires it enabled because of MorphOS again (in which case that should be documented in a comment!)
MorphOS is a different case. It has it's own driver but that currently cannot use the OHCI controller on QEMU, maybe because it expects OpenFirmware to initalise an IRQ which this driver does not do. I haven't investigated that yet as it is not needed for using VNC which is the only way to access MorphOS at the moment. So it is not needed for MorphOS but for the driver itself.
or the driver is completely useless on non-QEMU platforms and shouldn't be around because it would probably just time out.
It shouldn't be useless and should work as it is identical to what's in CoreBoot (plus big endian fixes) where it is supposedly used on real hardware.
Regards, BALATON Zoltan