Message ID | 20230227125732.20941-1-shentey@gmail.com |
---|---|
Headers | show |
Series | Pegasos2 fixes and audio output support | expand |
On Mon, 27 Feb 2023, Bernhard Beschow wrote:
> On behalve of Zoltan BALATON:
You don't have to do that and in fact please don't. I'll handle this
series just reply to the one patch that needs update with only the changes
that were asked by review.
Regards,
BALATON Zoltan
On Mon, Feb 27, 2023 at 2:00 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > On Mon, 27 Feb 2023, Bernhard Beschow wrote: > > On behalve of Zoltan BALATON: > > You don't have to do that and in fact please don't. I'll handle this > series just reply to the one patch that needs update with only the changes > that were asked by review. > > Regards, > BALATON Zoltan > Google seems to agree with me by not letting me send your patches :/ Sent [PATCH v4 0/7] Pegasos2 fixes and audio output support Sent [PATCH v4 1/7] hw/display/sm501: Implement more 2D raster operations Sent [PATCH v4 2/7] hw/display/sm501: Add fallbacks to pixman routines Sent [PATCH v4 3/7] hw/display/sm501: Add debug property to control pixman usage 4.3.0 Mail server temporarily rejected message. bk4-20020a170906b0c400b008d7a8083dffsm3186414ejb.222 - gsmtp As said before I don't want to iterate on the changes of this series. I can't test them and from my POV they seem unnecessary to me since all the test cases I can perform still work without the IRQ changes. Looking at the schematics I get the impression that the PCI IRQs actually work the other way around: Instead of the INTx lines of the 2nd PCI bus triggering both the north and the south bridge IRQ controllers, the two PCI buses of the north bridge both trigger the VT82xx PCI IRQ router. I'm not a hardware engineer so I could be totally off here. That's why I rather leave my hands off of this stuff. Best regards, Bernhard
On Mon, 27 Feb 2023, Bernhard Beschow wrote: > On Mon, Feb 27, 2023 at 2:00 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > >> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>> On behalve of Zoltan BALATON: >> >> You don't have to do that and in fact please don't. I'll handle this >> series just reply to the one patch that needs update with only the changes >> that were asked by review. >> >> Regards, >> BALATON Zoltan >> > > Google seems to agree with me by not letting me send your patches :/ > > Sent [PATCH v4 0/7] Pegasos2 fixes and audio output support > Sent [PATCH v4 1/7] hw/display/sm501: Implement more 2D raster operations > Sent [PATCH v4 2/7] hw/display/sm501: Add fallbacks to pixman routines > Sent [PATCH v4 3/7] hw/display/sm501: Add debug property to control > pixman usage > 4.3.0 Mail server temporarily rejected message. > bk4-20020a170906b0c400b008d7a8083dffsm3186414ejb.222 - gsmtp > > As said before I don't want to iterate on the changes of this series. I > can't test them and from my POV they seem unnecessary to me since all the > test cases I can perform still work without the IRQ changes. Then why do you make me track your series and asking me to check if you broke anything in my patches during your rebase that I've asked you not to do? The series I've submitted (both my original and the one with your changes) were tested and made sure it worked with AmigaOS that you say you can't test. > Looking at the schematics I get the impression that the PCI IRQs actually > work the other way around: Instead of the INTx lines of the 2nd PCI bus > triggering both the north and the south bridge IRQ controllers, the two PCI > buses of the north bridge both trigger the VT82xx PCI IRQ router. I'm not a > hardware engineer so I could be totally off here. That's why I rather leave > my hands off of this stuff. You don't seem to leave your hands off and got involved voluntarily so now don't run away :-) I'm no hardware engineer either but in any case pci_set_irq cannot be used from a PCIDevice as I explained in the other message so your approach with that is clearly wrong and we need gpios that something else connects to the PCI bus. You could only do that if the VIA chip was a north bridge that had a PCI bus but it doesn't. In pegasos2 the north bridge is the MV64361 but the PCI interrupt lines are connected to its GPP (General purpose or multi purpose pins in docs that are just gpio lines, which are programmable inputs or outputs). These can generate an interrupt in the MV64361 but the VT8231 also has an ability to route PCI IRQs to ISA interrupts which some guests use. So I think the way I've modeled it is correct by connecting the PCI bus interrupt pins to both of these chips and since they are independent models the only place you can do it cleanly is the board code. Other boards may connect the VIA PIRQ pins differently but this model allows for that now. What is that you still don't like about it? Regards, BALATON Zoltan
On Mon, Feb 27, 2023 at 2:45 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > On Mon, 27 Feb 2023, Bernhard Beschow wrote: > > On Mon, Feb 27, 2023 at 2:00 PM BALATON Zoltan <balaton@eik.bme.hu> > wrote: > > > >> On Mon, 27 Feb 2023, Bernhard Beschow wrote: > >>> On behalve of Zoltan BALATON: > >> > >> You don't have to do that and in fact please don't. I'll handle this > >> series just reply to the one patch that needs update with only the > changes > >> that were asked by review. > >> > >> Regards, > >> BALATON Zoltan > >> > > > > Google seems to agree with me by not letting me send your patches :/ > > > > Sent [PATCH v4 0/7] Pegasos2 fixes and audio output support > > Sent [PATCH v4 1/7] hw/display/sm501: Implement more 2D raster > operations > > Sent [PATCH v4 2/7] hw/display/sm501: Add fallbacks to pixman routines > > Sent [PATCH v4 3/7] hw/display/sm501: Add debug property to control > > pixman usage > > 4.3.0 Mail server temporarily rejected message. > > bk4-20020a170906b0c400b008d7a8083dffsm3186414ejb.222 - gsmtp > > > > As said before I don't want to iterate on the changes of this series. I > > can't test them and from my POV they seem unnecessary to me since all the > > test cases I can perform still work without the IRQ changes. > > Then why do you make me track your series and asking me to check if you > broke anything in my patches during your rebase that I've asked you not > to do? Because I couldn't convince you about the PCI IRQ router changes ;) I've asked how to proceed and got the suggestion to post an alternative series. > The series I've submitted (both my original and the one with your > changes) were tested and made sure it worked with AmigaOS that you say you > can't test. > Unfortunately my patches had changes merged in. This now makes it hard to show what really changed (spoiler: nothing that affects behavior). As you probably noticed in the "resend" version of this iteration I split off a patch introducing the priq properties. It belongs to the sub series of the Pegasos2 IRQ fixes which appear unnecessary to me, so I don't want to show up in `git blame` as the author of any of these changes. I attributed it to you because this was really your change which I'm not even sure is legal. Let's avoid such complications by keeping our series separate. > > Looking at the schematics I get the impression that the PCI IRQs actually > > work the other way around: Instead of the INTx lines of the 2nd PCI bus > > triggering both the north and the south bridge IRQ controllers, the two > PCI > > buses of the north bridge both trigger the VT82xx PCI IRQ router. I'm > not a > > hardware engineer so I could be totally off here. That's why I rather > leave > > my hands off of this stuff. > > You don't seem to leave your hands off and got involved voluntarily so now > don't run away :-) > I'm happy to comment on it but please don't make me change anything there for now. Feature freeze is approaching soon after all which in turn raises the temperature for development. > I'm no hardware engineer either but in any case pci_set_irq cannot be used > from a PCIDevice as I explained in the other message so your approach with > that is clearly wrong and we need gpios that something else connects to > the PCI bus. You could only do that if the VIA chip was a north bridge > that had a PCI bus but it doesn't. In pegasos2 the north bridge is the > MV64361 but the PCI interrupt lines are connected to its GPP (General > purpose or multi purpose pins in docs that are just gpio lines, which are > programmable inputs or outputs). These can generate an interrupt in the > MV64361 but the VT8231 also has an ability to route PCI IRQs to ISA > interrupts which some guests use. So I think the way I've modeled it is > correct by connecting the PCI bus interrupt pins to both of these chips > and since they are independent models the only place you can do it cleanly > is the board code. Other boards may connect the VIA PIRQ pins differently > but this model allows for that now. What is that you still don't like > about it? > On page 13 there is a note saying "Southbridge is INT controller". Another note says "AGP uses A as first Int for none shared operation". These notes describe hardware, so should apply to all guests. Furthermore, I couldn't find any remotely useful documentation for the MV64361 chip. This turns any discussion into guesswork. Best regards, Bernhard > > Regards, > BALATON Zoltan
On Mon, 27 Feb 2023, Bernhard Beschow wrote: > On Mon, Feb 27, 2023 at 2:45 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: >> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>> On Mon, Feb 27, 2023 at 2:00 PM BALATON Zoltan <balaton@eik.bme.hu> >> wrote: >>> >>>> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>>>> On behalve of Zoltan BALATON: >>>> >>>> You don't have to do that and in fact please don't. I'll handle this >>>> series just reply to the one patch that needs update with only the >> changes >>>> that were asked by review. >>>> >>>> Regards, >>>> BALATON Zoltan >>>> >>> >>> Google seems to agree with me by not letting me send your patches :/ >>> >>> Sent [PATCH v4 0/7] Pegasos2 fixes and audio output support >>> Sent [PATCH v4 1/7] hw/display/sm501: Implement more 2D raster >> operations >>> Sent [PATCH v4 2/7] hw/display/sm501: Add fallbacks to pixman routines >>> Sent [PATCH v4 3/7] hw/display/sm501: Add debug property to control >>> pixman usage >>> 4.3.0 Mail server temporarily rejected message. >>> bk4-20020a170906b0c400b008d7a8083dffsm3186414ejb.222 - gsmtp >>> >>> As said before I don't want to iterate on the changes of this series. I >>> can't test them and from my POV they seem unnecessary to me since all the >>> test cases I can perform still work without the IRQ changes. >> >> Then why do you make me track your series and asking me to check if you >> broke anything in my patches during your rebase that I've asked you not >> to do? > > > Because I couldn't convince you about the PCI IRQ router changes ;) I've > asked how to proceed and got the suggestion to post an alternative series. That's fine and you did that and I got your changes incorporated in my series and had that tested again and then submitted as one series as asked by the maintainer to keep all this togetner. Then you come back willing to split this series again, reposting some untested changes that I have no idea what did you change. This isn't very friendly before a freeze so please stop doing that and keep this in one series otherwise we get lost between all the changes. >> The series I've submitted (both my original and the one with your >> changes) were tested and made sure it worked with AmigaOS that you say you >> can't test. >> > > Unfortunately my patches had changes merged in. This now makes it hard to > show what really changed (spoiler: nothing that affects behavior). The two changes I've made and noted in the commit message were: 1. removing *empty* lines from a switch that only made it half as long and easier to read without any change in content. 2. instead of changing the interrupts of the PCI bus this device is connected to with pci_bus_irqs(), I export these as gpio pins to model the real chip which is then connected in the board as in real hadware. > As you probably noticed in the "resend" version of this iteration I split > off a patch introducing the priq properties. It belongs to the sub series > of the Pegasos2 IRQ fixes which appear unnecessary to me, so I don't want > to show up in `git blame` as the author of any of these changes. I > attributed it to you because this was really your change which I'm not even > sure is legal. > > Let's avoid such complications by keeping our series separate. Yout series is wrong because of the pci_bus_irqs (did you check PCI cards such as adding one with -device still work on fuloong2e with your patch?) I've corrected in my series so that it also fits in with my changes. If you don't like this change then we can drop your series and go back to mine instead or make the patch Suggested-by you and I take the From: or just keep out of this but please decide what you want and dont make it unnecessarily difficult to review and merge this series. >>> Looking at the schematics I get the impression that the PCI IRQs actually >>> work the other way around: Instead of the INTx lines of the 2nd PCI bus >>> triggering both the north and the south bridge IRQ controllers, the two >> PCI >>> buses of the north bridge both trigger the VT82xx PCI IRQ router. I'm >> not a >>> hardware engineer so I could be totally off here. That's why I rather >> leave >>> my hands off of this stuff. >> >> You don't seem to leave your hands off and got involved voluntarily so now >> don't run away :-) >> > > I'm happy to comment on it but please don't make me change anything there > for now. Feature freeze is approaching soon after all which in turn raises > the temperature for development. I can just say the same to you. This was my series that you changed so don't ask me to not change it. I've changed it as you proposed despite I'm not buting that's the right way and had it re-tested but it's still not good for you? >> I'm no hardware engineer either but in any case pci_set_irq cannot be used >> from a PCIDevice as I explained in the other message so your approach with >> that is clearly wrong and we need gpios that something else connects to >> the PCI bus. You could only do that if the VIA chip was a north bridge >> that had a PCI bus but it doesn't. In pegasos2 the north bridge is the >> MV64361 but the PCI interrupt lines are connected to its GPP (General >> purpose or multi purpose pins in docs that are just gpio lines, which are >> programmable inputs or outputs). These can generate an interrupt in the >> MV64361 but the VT8231 also has an ability to route PCI IRQs to ISA >> interrupts which some guests use. So I think the way I've modeled it is >> correct by connecting the PCI bus interrupt pins to both of these chips >> and since they are independent models the only place you can do it cleanly >> is the board code. Other boards may connect the VIA PIRQ pins differently >> but this model allows for that now. What is that you still don't like >> about it? >> > > On page 13 there is a note saying "Southbridge is INT controller". Another > note says "AGP uses A as first Int for none shared operation". These notes > describe hardware, so should apply to all guests. > > Furthermore, I couldn't find any remotely useful documentation for the > MV64361 chip. This turns any discussion into guesswork. Maybe check here: qmiga.osdn.io there are some hints how to find some docs. Can't link then due to copyright reasons. Regards, BALATON Zoltan
On Mon, 27 Feb 2023, Bernhard Beschow wrote: > Unfortunately my patches had changes merged in. This now makes it hard to > show what really changed (spoiler: nothing that affects behavior). > > As you probably noticed in the "resend" version of this iteration I split > off a patch introducing the priq properties. It belongs to the sub series > of the Pegasos2 IRQ fixes which appear unnecessary to me, so I don't want > to show up in `git blame` as the author of any of these changes. I > attributed it to you because this was really your change which I'm not even > sure is legal. > > Let's avoid such complications by keeping our series separate. Let's cool down a bit. Philippe took some of the sm501 patches in his giant pull request (and a lot of your patches too) now so I'll wait until that lands and hope to get some review for the remaining patches too. Once that pull req is merged I'll rebase the remaining patches and resubmit the series also adding changes for reasonable review comments I get by then. I plan to include your patches as before and competely ignore your alternative series as that's just complicating things now. I also would like to add another patch to implement the same workaround for MorphOS that I had in my original series if I can figure thet out. This could be a separate patch though so it's easy to revert when the i8259 model is fixed in the future (if ever). If you don't want to be author of your patch with only changing pci_bus_irqs for gpio_named then I can add Suggested-by instead which would still show it was your idea but would not get attributed in git blame for it. You can't convince me that using pci_bus_irqs is the right way for the reasons I've explained before. It's also only used by pci-hosts and boards everywhere else except in piix which you've added (and those may also be wrong then). It may still work if I fix that up in pegasos2 but still not the right way. I mean now: mv64361 connects PCI interrupts to its gpp 12-15 pins within its model, this lacks the connections to vt8231 so ISA IRQs aren't raised for PCI devices such as -device rtl8139 after my changes: pegasos2 board connects PCI interrupts to both gpp 12-15 of mv64371 and PINTA-D pins of VT8231 after it instantiates both which matches what the schematics say and also how QDev prefers to model chip pins. with your series: mv64361 connects it as above then VT8132 replaces it with its own. Then you want me to add another patch to fix that all up to arrive at the same result as above? That's just clearly wrong and unnecessary. I don't see why you don't accept the above solution? Regards, BALATON Zoltan
On 27/2/23 18:47, BALATON Zoltan wrote: > On Mon, 27 Feb 2023, Bernhard Beschow wrote: >> Unfortunately my patches had changes merged in. This now makes it hard to >> show what really changed (spoiler: nothing that affects behavior). >> >> As you probably noticed in the "resend" version of this iteration I split >> off a patch introducing the priq properties. It belongs to the sub series >> of the Pegasos2 IRQ fixes which appear unnecessary to me, so I don't want >> to show up in `git blame` as the author of any of these changes. I >> attributed it to you because this was really your change which I'm not >> even >> sure is legal. >> >> Let's avoid such complications by keeping our series separate. > > Let's cool down a bit. Philippe took some of the sm501 patches in his > giant pull request (and a lot of your patches too) now so I'll wait > until that lands and hope to get some review for the remaining patches > too. Once that pull req is merged I'll rebase the remaining patches and > resubmit the series also adding changes for reasonable review comments I > get by then. I'm sorry it took me so long, I was expecting these patches to be picked up by other maintainers but everybody is very busy. I know you'll need to rebase and it might be painful. But I did that believing it is the best I could give to the project with my current capacity. Also I don't want to overlap (too much) into other (sub)maintained areas. If you are stuck with a rebase, I can try to help. We'll get to the end of PIIX, if this isn't this release, it will be the next one. I've been waiting for these cleanups since 4 years already, before Hervé Poussineau pushed toward that direction during 3 years. We always hit problems due to PC world inheritance which, as a production system, can not regress. Also I don't want to compete with you guys, I'd really love to work together as team, but I have other responsibilities and sometime I can't spend the time I'd like here. What blocks the PIIX changes is the "q35/ich9/-device piix3" broken config. I'll explain elsewhere why it is broken. The problem is I don't know how to suggest alternative. Bernhard sometimes it is easier to share visions in a 30min call, rather than on a such thread. If you want I'm open for one.
Am 27. Februar 2023 22:12:46 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 27/2/23 18:47, BALATON Zoltan wrote: >> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>> Unfortunately my patches had changes merged in. This now makes it hard to >>> show what really changed (spoiler: nothing that affects behavior). >>> >>> As you probably noticed in the "resend" version of this iteration I split >>> off a patch introducing the priq properties. It belongs to the sub series >>> of the Pegasos2 IRQ fixes which appear unnecessary to me, so I don't want >>> to show up in `git blame` as the author of any of these changes. I >>> attributed it to you because this was really your change which I'm not even >>> sure is legal. >>> >>> Let's avoid such complications by keeping our series separate. >> >> Let's cool down a bit. Philippe took some of the sm501 patches in his giant pull request (and a lot of your patches too) now so I'll wait until that lands and hope to get some review for the remaining patches too. Once that pull req is merged I'll rebase the remaining patches and resubmit the series also adding changes for reasonable review comments I get by then. > >I'm sorry it took me so long, I was expecting these patches to be picked >up by other maintainers but everybody is very busy. I know you'll need >to rebase and it might be painful. But I did that believing it is the >best I could give to the project with my current capacity. Also I don't >want to overlap (too much) into other (sub)maintained areas. >If you are stuck with a rebase, I can try to help. >We'll get to the end of PIIX, if this isn't this release, it will be >the next one. I've been waiting for these cleanups since 4 years >already, before Hervé Poussineau pushed toward that direction during >3 years. We always hit problems due to PC world inheritance which, >as a production system, can not regress. Did you intend to reply in the PIIX consolidation or global ISA bus renoval series? This is a VT82xx thread... Thanks for picking up the ICH9 changes! > >Also I don't want to compete with you guys, I'd really love to work >together as team, but I have other responsibilities and sometime I >can't spend the time I'd like here. > >What blocks the PIIX changes is the "q35/ich9/-device piix3" broken >config. I'll explain elsewhere why it is broken. The problem is I >don't know how to suggest alternative. > >Bernhard sometimes it is easier to share visions in a 30min call, >rather than on a such thread. If you want I'm open for one. > Sure, I'm open for that. I'll let you know when I'll have some time. Best regards, Bernhard
On Mon, 27 Feb 2023, Philippe Mathieu-Daudé wrote: > On 27/2/23 18:47, BALATON Zoltan wrote: >> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>> Unfortunately my patches had changes merged in. This now makes it hard to >>> show what really changed (spoiler: nothing that affects behavior). >>> >>> As you probably noticed in the "resend" version of this iteration I split >>> off a patch introducing the priq properties. It belongs to the sub series >>> of the Pegasos2 IRQ fixes which appear unnecessary to me, so I don't want >>> to show up in `git blame` as the author of any of these changes. I >>> attributed it to you because this was really your change which I'm not >>> even >>> sure is legal. >>> >>> Let's avoid such complications by keeping our series separate. >> >> Let's cool down a bit. Philippe took some of the sm501 patches in his giant >> pull request (and a lot of your patches too) now so I'll wait until that >> lands and hope to get some review for the remaining patches too. Once that >> pull req is merged I'll rebase the remaining patches and resubmit the >> series also adding changes for reasonable review comments I get by then. > > I'm sorry it took me so long, I was expecting these patches to be picked > up by other maintainers but everybody is very busy. I know you'll need You have no reason to apologise really, you did a great job merging all the patches. I was thinking that because as you say every maintainer is very busy now and we also had CI outage for a few weeks should we consider extending the date until the freeze by one or two weeks? That would allow people to relax a bit and be able to consolidate and merge all still pending patches. Postponing the 8.0 release one or two weeks is probably better than missing a lot of changes until the next release in September. We'd still aim for the original freeze date but if we fail to meet that it would be more convenient to know there could be a possibility for extending it. But make it clear that this is only for this one time because of CI outage and additional maintainer load caused by that so not something that should be done regularly but under current circumstances I would consider it. Regards, BALATON Zoltan
On Tue, Feb 28, 2023 at 04:05:30PM +0100, BALATON Zoltan wrote: > On Mon, 27 Feb 2023, Philippe Mathieu-Daudé wrote: > > On 27/2/23 18:47, BALATON Zoltan wrote: > > > On Mon, 27 Feb 2023, Bernhard Beschow wrote: > > > > Unfortunately my patches had changes merged in. This now makes it hard to > > > > show what really changed (spoiler: nothing that affects behavior). > > > > > > > > As you probably noticed in the "resend" version of this iteration I split > > > > off a patch introducing the priq properties. It belongs to the sub series > > > > of the Pegasos2 IRQ fixes which appear unnecessary to me, so I don't want > > > > to show up in `git blame` as the author of any of these changes. I > > > > attributed it to you because this was really your change which > > > > I'm not even > > > > sure is legal. > > > > > > > > Let's avoid such complications by keeping our series separate. > > > > > > Let's cool down a bit. Philippe took some of the sm501 patches in > > > his giant pull request (and a lot of your patches too) now so I'll > > > wait until that lands and hope to get some review for the remaining > > > patches too. Once that pull req is merged I'll rebase the remaining > > > patches and resubmit the series also adding changes for reasonable > > > review comments I get by then. > > > > I'm sorry it took me so long, I was expecting these patches to be picked > > up by other maintainers but everybody is very busy. I know you'll need > > You have no reason to apologise really, you did a great job merging all the > patches. I was thinking that because as you say every maintainer is very > busy now and we also had CI outage for a few weeks should we consider > extending the date until the freeze by one or two weeks? That would allow > people to relax a bit and be able to consolidate and merge all still pending > patches. Postponing the 8.0 release one or two weeks is probably better than > missing a lot of changes until the next release in September. We'd still aim > for the original freeze date but if we fail to meet that it would be more > convenient to know there could be a possibility for extending it. But make > it clear that this is only for this one time because of CI outage and > additional maintainer load caused by that so not something that should be > done regularly but under current circumstances I would consider it. There's no need to change the release schedule IMHO. Subsystem maintainers should continue to send pull requests as normal. Peter is still processing PRs, albeit at a lower rate with adhoc CI. From the soft freeze POV what matters is just that the PRs are posted on the mailing list before the deadline. If they're posted in time, they're still valid for inclusion in the release. Our CI allowance is reset at the end of today anyway. With regards, Daniel
On Tue, 28 Feb 2023, Daniel P. Berrangé wrote: > On Tue, Feb 28, 2023 at 04:05:30PM +0100, BALATON Zoltan wrote: >> On Mon, 27 Feb 2023, Philippe Mathieu-Daudé wrote: >>> On 27/2/23 18:47, BALATON Zoltan wrote: >>>> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>>>> Unfortunately my patches had changes merged in. This now makes it hard to >>>>> show what really changed (spoiler: nothing that affects behavior). >>>>> >>>>> As you probably noticed in the "resend" version of this iteration I split >>>>> off a patch introducing the priq properties. It belongs to the sub series >>>>> of the Pegasos2 IRQ fixes which appear unnecessary to me, so I don't want >>>>> to show up in `git blame` as the author of any of these changes. I >>>>> attributed it to you because this was really your change which >>>>> I'm not even >>>>> sure is legal. >>>>> >>>>> Let's avoid such complications by keeping our series separate. >>>> >>>> Let's cool down a bit. Philippe took some of the sm501 patches in >>>> his giant pull request (and a lot of your patches too) now so I'll >>>> wait until that lands and hope to get some review for the remaining >>>> patches too. Once that pull req is merged I'll rebase the remaining >>>> patches and resubmit the series also adding changes for reasonable >>>> review comments I get by then. >>> >>> I'm sorry it took me so long, I was expecting these patches to be picked >>> up by other maintainers but everybody is very busy. I know you'll need >> >> You have no reason to apologise really, you did a great job merging all the >> patches. I was thinking that because as you say every maintainer is very >> busy now and we also had CI outage for a few weeks should we consider >> extending the date until the freeze by one or two weeks? That would allow >> people to relax a bit and be able to consolidate and merge all still pending >> patches. Postponing the 8.0 release one or two weeks is probably better than >> missing a lot of changes until the next release in September. We'd still aim >> for the original freeze date but if we fail to meet that it would be more >> convenient to know there could be a possibility for extending it. But make >> it clear that this is only for this one time because of CI outage and >> additional maintainer load caused by that so not something that should be >> done regularly but under current circumstances I would consider it. > > There's no need to change the release schedule IMHO. Subsystem maintainers > should continue to send pull requests as normal. Peter is still processing > PRs, albeit at a lower rate with adhoc CI. From the soft freeze POV what > matters is just that the PRs are posted on the mailing list before the > deadline. If they're posted in time, they're still valid for inclusion in > the release. Our CI allowance is reset at the end of today anyway. Problem is that some patches will need to be rebased on still pending pull request and also may need the maintainer's attention to review them and send the final pull despite series have been on the list few weeks before the freeze that should be in time. So this will mean we might still have some pending patches end of this week and if somebody asks for last minute changes then it will be hard to meet the deadline. So at least one week extension seems to resolve the pressure this causes and also give maintainers some time to catch up. It's better than just saying "Sorry it was out fault but you've still missed the release, see you next time." Regards, BALATON Zoltan
On 27/02/2023 16:58, BALATON Zoltan wrote: > On Mon, 27 Feb 2023, Bernhard Beschow wrote: >> On Mon, Feb 27, 2023 at 2:45 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: >>> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>>> On Mon, Feb 27, 2023 at 2:00 PM BALATON Zoltan <balaton@eik.bme.hu> >>> wrote: >>>> >>>>> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>>>>> On behalve of Zoltan BALATON: >>>>> >>>>> You don't have to do that and in fact please don't. I'll handle this >>>>> series just reply to the one patch that needs update with only the >>> changes >>>>> that were asked by review. >>>>> >>>>> Regards, >>>>> BALATON Zoltan >>>>> >>>> >>>> Google seems to agree with me by not letting me send your patches :/ >>>> >>>> Sent [PATCH v4 0/7] Pegasos2 fixes and audio output support >>>> Sent [PATCH v4 1/7] hw/display/sm501: Implement more 2D raster >>> operations >>>> Sent [PATCH v4 2/7] hw/display/sm501: Add fallbacks to pixman routines >>>> Sent [PATCH v4 3/7] hw/display/sm501: Add debug property to control >>>> pixman usage >>>> 4.3.0 Mail server temporarily rejected message. >>>> bk4-20020a170906b0c400b008d7a8083dffsm3186414ejb.222 - gsmtp >>>> >>>> As said before I don't want to iterate on the changes of this series. I >>>> can't test them and from my POV they seem unnecessary to me since all the >>>> test cases I can perform still work without the IRQ changes. >>> >>> Then why do you make me track your series and asking me to check if you >>> broke anything in my patches during your rebase that I've asked you not >>> to do? >> >> >> Because I couldn't convince you about the PCI IRQ router changes ;) I've >> asked how to proceed and got the suggestion to post an alternative series. > > That's fine and you did that and I got your changes incorporated in my series and had > that tested again and then submitted as one series as asked by the maintainer to keep > all this togetner. Then you come back willing to split this series again, reposting > some untested changes that I have no idea what did you change. This isn't very > friendly before a freeze so please stop doing that and keep this in one series > otherwise we get lost between all the changes. > >>> The series I've submitted (both my original and the one with your >>> changes) were tested and made sure it worked with AmigaOS that you say you >>> can't test. >>> >> >> Unfortunately my patches had changes merged in. This now makes it hard to >> show what really changed (spoiler: nothing that affects behavior). > > The two changes I've made and noted in the commit message were: > > 1. removing *empty* lines from a switch that only made it half as long and easier to > read without any change in content. > > 2. instead of changing the interrupts of the PCI bus this device is connected to with > pci_bus_irqs(), I export these as gpio pins to model the real chip which is then > connected in the board as in real hadware. > >> As you probably noticed in the "resend" version of this iteration I split >> off a patch introducing the priq properties. It belongs to the sub series >> of the Pegasos2 IRQ fixes which appear unnecessary to me, so I don't want >> to show up in `git blame` as the author of any of these changes. I >> attributed it to you because this was really your change which I'm not even >> sure is legal. >> >> Let's avoid such complications by keeping our series separate. > > Yout series is wrong because of the pci_bus_irqs (did you check PCI cards such as > adding one with -device still work on fuloong2e with your patch?) I've corrected in > my series so that it also fits in with my changes. If you don't like this change then > we can drop your series and go back to mine instead or make the patch Suggested-by > you and I take the From: or just keep out of this but please decide what you want and > dont make it unnecessarily difficult to review and merge this series. > >>>> Looking at the schematics I get the impression that the PCI IRQs actually >>>> work the other way around: Instead of the INTx lines of the 2nd PCI bus >>>> triggering both the north and the south bridge IRQ controllers, the two >>> PCI >>>> buses of the north bridge both trigger the VT82xx PCI IRQ router. I'm >>> not a >>>> hardware engineer so I could be totally off here. That's why I rather >>> leave >>>> my hands off of this stuff. >>> >>> You don't seem to leave your hands off and got involved voluntarily so now >>> don't run away :-) >>> >> >> I'm happy to comment on it but please don't make me change anything there >> for now. Feature freeze is approaching soon after all which in turn raises >> the temperature for development. > > I can just say the same to you. This was my series that you changed so don't ask me > to not change it. I've changed it as you proposed despite I'm not buting that's the > right way and had it re-tested but it's still not good for you? > >>> I'm no hardware engineer either but in any case pci_set_irq cannot be used >>> from a PCIDevice as I explained in the other message so your approach with >>> that is clearly wrong and we need gpios that something else connects to >>> the PCI bus. You could only do that if the VIA chip was a north bridge >>> that had a PCI bus but it doesn't. In pegasos2 the north bridge is the >>> MV64361 but the PCI interrupt lines are connected to its GPP (General >>> purpose or multi purpose pins in docs that are just gpio lines, which are >>> programmable inputs or outputs). These can generate an interrupt in the >>> MV64361 but the VT8231 also has an ability to route PCI IRQs to ISA >>> interrupts which some guests use. So I think the way I've modeled it is >>> correct by connecting the PCI bus interrupt pins to both of these chips >>> and since they are independent models the only place you can do it cleanly >>> is the board code. Other boards may connect the VIA PIRQ pins differently >>> but this model allows for that now. What is that you still don't like >>> about it? >>> >> >> On page 13 there is a note saying "Southbridge is INT controller". Another >> note says "AGP uses A as first Int for none shared operation". These notes >> describe hardware, so should apply to all guests. >> >> Furthermore, I couldn't find any remotely useful documentation for the >> MV64361 chip. This turns any discussion into guesswork. > > Maybe check here: qmiga.osdn.io there are some hints how to find some docs. Can't > link then due to copyright reasons. I tried using the recommended searches, however all I can find are PDFs of the product summary. Can you provide some more hints, such as a filename for example? ATB, Mark.
On Wed, 1 Mar 2023, Mark Cave-Ayland wrote: > On 27/02/2023 16:58, BALATON Zoltan wrote: >> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>> On Mon, Feb 27, 2023 at 2:45 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: >>>> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>>>> On Mon, Feb 27, 2023 at 2:00 PM BALATON Zoltan <balaton@eik.bme.hu> >>>> wrote: >>>>> >>>>>> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>>>>>> On behalve of Zoltan BALATON: >>>>>> >>>>>> You don't have to do that and in fact please don't. I'll handle this >>>>>> series just reply to the one patch that needs update with only the >>>> changes >>>>>> that were asked by review. >>>>>> >>>>>> Regards, >>>>>> BALATON Zoltan >>>>>> >>>>> >>>>> Google seems to agree with me by not letting me send your patches :/ >>>>> >>>>> Sent [PATCH v4 0/7] Pegasos2 fixes and audio output support >>>>> Sent [PATCH v4 1/7] hw/display/sm501: Implement more 2D raster >>>> operations >>>>> Sent [PATCH v4 2/7] hw/display/sm501: Add fallbacks to pixman routines >>>>> Sent [PATCH v4 3/7] hw/display/sm501: Add debug property to control >>>>> pixman usage >>>>> 4.3.0 Mail server temporarily rejected message. >>>>> bk4-20020a170906b0c400b008d7a8083dffsm3186414ejb.222 - gsmtp >>>>> >>>>> As said before I don't want to iterate on the changes of this series. I >>>>> can't test them and from my POV they seem unnecessary to me since all >>>>> the >>>>> test cases I can perform still work without the IRQ changes. >>>> >>>> Then why do you make me track your series and asking me to check if you >>>> broke anything in my patches during your rebase that I've asked you not >>>> to do? >>> >>> >>> Because I couldn't convince you about the PCI IRQ router changes ;) I've >>> asked how to proceed and got the suggestion to post an alternative series. >> >> That's fine and you did that and I got your changes incorporated in my >> series and had that tested again and then submitted as one series as asked >> by the maintainer to keep all this togetner. Then you come back willing to >> split this series again, reposting some untested changes that I have no >> idea what did you change. This isn't very friendly before a freeze so >> please stop doing that and keep this in one series otherwise we get lost >> between all the changes. >> >>>> The series I've submitted (both my original and the one with your >>>> changes) were tested and made sure it worked with AmigaOS that you say >>>> you >>>> can't test. >>>> >>> >>> Unfortunately my patches had changes merged in. This now makes it hard to >>> show what really changed (spoiler: nothing that affects behavior). >> >> The two changes I've made and noted in the commit message were: >> >> 1. removing *empty* lines from a switch that only made it half as long and >> easier to read without any change in content. >> >> 2. instead of changing the interrupts of the PCI bus this device is >> connected to with pci_bus_irqs(), I export these as gpio pins to model the >> real chip which is then connected in the board as in real hadware. >> >>> As you probably noticed in the "resend" version of this iteration I split >>> off a patch introducing the priq properties. It belongs to the sub series >>> of the Pegasos2 IRQ fixes which appear unnecessary to me, so I don't want >>> to show up in `git blame` as the author of any of these changes. I >>> attributed it to you because this was really your change which I'm not >>> even >>> sure is legal. >>> >>> Let's avoid such complications by keeping our series separate. >> >> Yout series is wrong because of the pci_bus_irqs (did you check PCI cards >> such as adding one with -device still work on fuloong2e with your patch?) >> I've corrected in my series so that it also fits in with my changes. If you >> don't like this change then we can drop your series and go back to mine >> instead or make the patch Suggested-by you and I take the From: or just >> keep out of this but please decide what you want and dont make it >> unnecessarily difficult to review and merge this series. >> >>>>> Looking at the schematics I get the impression that the PCI IRQs >>>>> actually >>>>> work the other way around: Instead of the INTx lines of the 2nd PCI bus >>>>> triggering both the north and the south bridge IRQ controllers, the two >>>> PCI >>>>> buses of the north bridge both trigger the VT82xx PCI IRQ router. I'm >>>> not a >>>>> hardware engineer so I could be totally off here. That's why I rather >>>> leave >>>>> my hands off of this stuff. >>>> >>>> You don't seem to leave your hands off and got involved voluntarily so >>>> now >>>> don't run away :-) >>>> >>> >>> I'm happy to comment on it but please don't make me change anything there >>> for now. Feature freeze is approaching soon after all which in turn raises >>> the temperature for development. >> >> I can just say the same to you. This was my series that you changed so >> don't ask me to not change it. I've changed it as you proposed despite I'm >> not buting that's the right way and had it re-tested but it's still not >> good for you? >> >>>> I'm no hardware engineer either but in any case pci_set_irq cannot be >>>> used >>>> from a PCIDevice as I explained in the other message so your approach >>>> with >>>> that is clearly wrong and we need gpios that something else connects to >>>> the PCI bus. You could only do that if the VIA chip was a north bridge >>>> that had a PCI bus but it doesn't. In pegasos2 the north bridge is the >>>> MV64361 but the PCI interrupt lines are connected to its GPP (General >>>> purpose or multi purpose pins in docs that are just gpio lines, which are >>>> programmable inputs or outputs). These can generate an interrupt in the >>>> MV64361 but the VT8231 also has an ability to route PCI IRQs to ISA >>>> interrupts which some guests use. So I think the way I've modeled it is >>>> correct by connecting the PCI bus interrupt pins to both of these chips >>>> and since they are independent models the only place you can do it >>>> cleanly >>>> is the board code. Other boards may connect the VIA PIRQ pins differently >>>> but this model allows for that now. What is that you still don't like >>>> about it? >>>> >>> >>> On page 13 there is a note saying "Southbridge is INT controller". Another >>> note says "AGP uses A as first Int for none shared operation". These notes >>> describe hardware, so should apply to all guests. >>> >>> Furthermore, I couldn't find any remotely useful documentation for the >>> MV64361 chip. This turns any discussion into guesswork. >> >> Maybe check here: qmiga.osdn.io there are some hints how to find some docs. >> Can't link then due to copyright reasons. > > I tried using the recommended searches, however all I can find are PDFs of > the product summary. Can you provide some more hints, such as a filename for > example? Maybe these are not available any more but it's a family of chips called MV64360, MB64361 and MV64362 (or Marvell Discovery II) with the different numbers are the same chip with different options such as number of PCI buses or ethernet controllers so the datasheet refers to them as MV64360/1/2 but maybe it's under MV64360. Hope this helps. Regards, BALATON Zoltan