| Message ID | cover.1701035944.git.balaton@eik.bme.hu |
|---|---|
| Headers | show |
| Series | Fix IRQ routing in via south bridge | expand |
On Sun, 26 Nov 2023, BALATON Zoltan wrote: > Philippe, > > Could this be merged for 8.2 as it fixes USB on the amigaone machine? > This would be useful as usb-storage is the simplest way to share data > with the host with these machines. Philippe, do you have some time to look at this now for 8.2 please? I still hope this could be fixed for the amigaone machine on release and dont' have to wait until the next one for USB to work on that machine. Regards, BALATON Zoltan > This is a slight change from v2 adding more comments and improving > commit messages and clean things a bit but otherwise should be the > same as previous versions. Even v1 worked the same as this one and v2, > the additional check to avoid stuck bits is just paranoia, it does not > happen in practice as IRQ mappings are quite static, they are set once > at boot and don't change afterwards. > > The rest is just some explanation on how we got here but can be > skipped if not interested in history. > > This is going back to my original implementation of this IRQ routing > that I submitted already for 8.0 in the beginning of this year > (https://patchew.org/QEMU/cover.1677004414.git.balaton@eik.bme.hu/) > but Mark had concerns about that because he wanted to use PCI > interrupt routing instead. I've already told back then that won't work > but I could not convince reviewers about that. Now with the amigaone > machine this can also be seen and that's why this series is needed now. > > The routing code in PCIBus cannot be used as that only considers the 4 > PCI interrupts but there are about 12 interrupt sources in this chip > that need to be routed to ISA IRQs (the embedded chip functions and > the 4 PCI interrupts that are coming in through 4 pins of the chip). > Also the chip does not own the PCIBus, that's part of the north bridge > so it should not change the PCI interrupt routing of a bus it does not > own. (Piix calling pci_bus_irqs() I think is also wrong because the > PCI bus in that machine is also owned by the north bridge and piix > should not take over routing of IRQs on a bus it's connected to.) > Another concern from Mark was that this makes chip functions specific > to the chip and they cannot be used as individual PCI devices. Well, > yes, they are chip functions, are not user creatable and don't exist > as individual devices so it does not make sense to use them without > the actual VIA chip they are a function of so that's not a real > concern. These functions are also not actual PCI devices, they are > PCIDevice because they appear as PCI functions of the chip but are > connected internally and this series also models that correctly. This > seems to be supported by comments in Linux about how these VIA chip > function aren't following PCI standards and use ISA IRQs instead: > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/pci/quirks.c?h=v6.5.6#n1172 > > Therefore I think Mark's proposals are not improving this model so I > went back to the original approach which was tested to work and is > also simpler and easier to understand than trying to reuse PCI > intrrupt routing which does not work and would be more complex anyway > for no good reason. > > Regards, > BALATON Zoltan > > BALATON Zoltan (4): > hw/isa/vt82c686: Bring back via_isa_set_irq() > hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts > hw/isa/vt82c686: Route PIRQ inputs using via_isa_set_irq() > hw/audio/via-ac97: Route interrupts using via_isa_set_irq() > > hw/audio/via-ac97.c | 8 ++-- > hw/isa/vt82c686.c | 79 +++++++++++++++++++++++++------------- > hw/usb/vt82c686-uhci-pci.c | 9 +++++ > include/hw/isa/vt82c686.h | 2 + > 4 files changed, 67 insertions(+), 31 deletions(-) > >
Hi Zoltan, On 28/11/23 13:47, BALATON Zoltan wrote: > On Sun, 26 Nov 2023, BALATON Zoltan wrote: >> Philippe, >> >> Could this be merged for 8.2 as it fixes USB on the amigaone machine? >> This would be useful as usb-storage is the simplest way to share data >> with the host with these machines. > > Philippe, do you have some time to look at this now for 8.2 please? I > still hope this could be fixed for the amigaone machine on release and > dont' have to wait until the next one for USB to work on that machine. Thanks for your detailed cover and patch descriptions. I just finished to run my tests and they all passed. I couldn't spend much time reviewing the patches, but having a quick look I don't think the way you model it is correct. This is a tricky setup and apparently we don't fully understand it (I understand what you explained, but some pieces don't make sense to me). That said, I understand it help you and the AmigaOne users, and nobody objected. So, while being a bit reluctant, I am queuing this series; and will send a PR in a few. We'll have time to improve this model later. Regards, Phil.
On Tue, 28 Nov 2023, Philippe Mathieu-Daudé wrote: > On 28/11/23 13:47, BALATON Zoltan wrote: >> On Sun, 26 Nov 2023, BALATON Zoltan wrote: >>> Philippe, >>> >>> Could this be merged for 8.2 as it fixes USB on the amigaone machine? >>> This would be useful as usb-storage is the simplest way to share data >>> with the host with these machines. >> >> Philippe, do you have some time to look at this now for 8.2 please? I still >> hope this could be fixed for the amigaone machine on release and dont' have >> to wait until the next one for USB to work on that machine. > > Thanks for your detailed cover and patch descriptions. > > I just finished to run my tests and they all passed. > > I couldn't spend much time reviewing the patches, but having a quick > look I don't think the way you model it is correct. This is a tricky > setup and apparently we don't fully understand it (I understand what > you explained, but some pieces don't make sense to me). That said, > I understand it help you and the AmigaOne users, and nobody objected. > So, while being a bit reluctant, I am queuing this series; and will > send a PR in a few. We'll have time to improve this model later. Thanks very much. I'm open to further discussion and improving this model, just wanted to have something working in master now. The discussion about this seemed never ending, it started before 8.0 and still could not get to a conclusion yet so until then this should work for now and allow users to use it and does not prevent improving it later. So I'm still interested in your review and why do you think this is not modelling it correctly but we have more time for that now and can change this further as a follow up. I think the current way makes it easier to add Bernhard's SCI interrupt as well for which I had a review proposal before. The main disagreement seemsd to be if the chip functions should be PCI devices or not. I think they aren't like regular PCI devices and clearly don't use PCI interrupts so Mark's and Bernhard's idea to use PCI bus interrupt routing for these does not work because they need to be independently routable to ISA interrupts. So whatever we do we'll need to distinguish the interrupt sources and keep track of their state individually because more than one of them can control a single ISA IRQ. Doing this in the ISA bridge seems like the best place because that already owns the ISA interrupts so no other component will need access to them and it can keep track of state of IRQ sources at one place. Regards, BALATON Zoltan
27.11.2023 01:49, BALATON Zoltan: > Philippe, > > Could this be merged for 8.2 as it fixes USB on the amigaone machine? > This would be useful as usb-storage is the simplest way to share data > with the host with these machines. > > This is a slight change from v2 adding more comments and improving > commit messages and clean things a bit but otherwise should be the > same as previous versions. Even v1 worked the same as this one and v2, > the additional check to avoid stuck bits is just paranoia, it does not > happen in practice as IRQ mappings are quite static, they are set once > at boot and don't change afterwards. Should this patchset be picked up for stable-8.1? Thanks, /mjt
On Wed, 29 Nov 2023, Michael Tokarev wrote: > 27.11.2023 01:49, BALATON Zoltan: >> Philippe, >> >> Could this be merged for 8.2 as it fixes USB on the amigaone machine? >> This would be useful as usb-storage is the simplest way to share data >> with the host with these machines. >> >> This is a slight change from v2 adding more comments and improving >> commit messages and clean things a bit but otherwise should be the >> same as previous versions. Even v1 worked the same as this one and v2, >> the additional check to avoid stuck bits is just paranoia, it does not >> happen in practice as IRQ mappings are quite static, they are set once >> at boot and don't change afterwards. > > Should this patchset be picked up for stable-8.1? The amigaone machine was added now in 8.2 so the problem does not occur in 8.1 yet (the different usage of this chip by amigaone firmware uncovered the issue that made this fix necessary). So 8.1 should still work without this therefore I don't think this is needed in stable. Regards, BALATON Zoltan