[PATCH v3 0/5] Misc sam460ex fixes (was: Clean up sam460ex irq mapping)

[PATCH v3 0/5] Misc sam460ex fixes (was: Clean up sam460ex irq mapping)

[BALATON Zoltan via]

So this is v3 of a series that started to fix a potential problem with
irq mapping in pci440_pcix (used by sam460ex) that got some other
fixes along the way as by-products. But it turns out the irq issue
this was trying to fix is not really a problem so finally we just
update the comment for now documenting why it works and only the
by-products remain in this series. Of which there are two more in this
v3 finally fixing a long standing problem booting MorphOS on sam460ex
(which I've got after debugging similar problem with pegasos2/vt8231
that gave me an idea how to debug this.)

Regards,
BALATON Zoltan

BALATON Zoltan (5):
  ppc4xx: Move common dependency on serial to common option
  sam460ex: Remove FDT_PPC dependency from KConfig
  ppc440_pcix: Improve comment for IRQ mapping
  ppc440_pcix: Fix register write trace event
  ppc440_pcix: Fix up pci config access

 hw/ppc/Kconfig       |  6 +-----
 hw/ppc/ppc440_pcix.c | 50 ++++++++++++++++++++++++++++----------------
 hw/ppc/trace-events  |  1 +
 3 files changed, 34 insertions(+), 23 deletions(-)

-- 
2.21.3

[PATCH v3 1/5] ppc4xx: Move common dependency on serial to common option

[BALATON Zoltan via]

All machines that select SERIAL also select PPC4XX so we can just add
this common dependency there once.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/Kconfig | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index dd86e664d2..8548f42b0d 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -37,7 +37,6 @@ config PPC405
     select M48T59
     select PFLASH_CFI02
     select PPC4XX
-    select SERIAL
 
 config PPC440
     bool
@@ -46,13 +45,13 @@ config PPC440
     imply E1000_PCI
     select PCI_EXPRESS
     select PPC4XX
-    select SERIAL
     select FDT_PPC
 
 config PPC4XX
     bool
     select BITBANG_I2C
     select PCI
+    select SERIAL
 
 config SAM460EX
     bool
@@ -61,7 +60,6 @@ config SAM460EX
     select IDE_SII3112
     select M41T80
     select PPC440
-    select SERIAL
     select SM501
     select SMBUS_EEPROM
     select USB_EHCI_SYSBUS
@@ -123,7 +121,6 @@ config VIRTEX
     bool
     select PPC4XX
     select PFLASH_CFI01
-    select SERIAL
     select XILINX
     select XILINX_ETHLITE
     select FDT_PPC
-- 
2.21.3

[PATCH v3 5/5] ppc440_pcix: Fix up pci config access

[BALATON Zoltan via]

This fixes a long standing issue with MorphOS booting on sam460ex
which turns out to be because of suspicious values written to PCI
config address that apparently works on real machine but caused wrong
access on this device model. This replaces a previous work around for
this with a better fix that makes it work.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc440_pcix.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
index 7829d3e556..91cbcd0504 100644
--- a/hw/ppc/ppc440_pcix.c
+++ b/hw/ppc/ppc440_pcix.c
@@ -449,28 +449,35 @@ static AddressSpace *ppc440_pcix_set_iommu(PCIBus *b, void *opaque, int devfn)
     return &s->bm_as;
 }
 
-/* The default pci_host_data_{read,write} functions in pci/pci_host.c
- * deny access to registers without bit 31 set but our clients want
- * this to work so we have to override these here */
-static void pci_host_data_write(void *opaque, hwaddr addr,
-                                uint64_t val, unsigned len)
+/*
+ * Some guests on sam460ex write all kinds of garbage here such as
+ * missing enable bit and low bits set and still expect this to work
+ * (apparently it does on real hardware because these boot there) so
+ * we have to override these ops here and fix it up
+ */
+static void pci_host_config_write(void *opaque, hwaddr addr,
+                                  uint64_t val, unsigned len)
 {
     PCIHostState *s = opaque;
-    pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
+
+    if (addr != 0 || len != 4) {
+        return;
+    }
+    s->config_reg = (val & 0xfffffffcULL) | (1UL << 31);
 }
 
-static uint64_t pci_host_data_read(void *opaque,
-                                   hwaddr addr, unsigned len)
+static uint64_t pci_host_config_read(void *opaque, hwaddr addr,
+                                     unsigned len)
 {
     PCIHostState *s = opaque;
-    uint32_t val;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
+    uint32_t val = s->config_reg;
+
     return val;
 }
 
-const MemoryRegionOps ppc440_pcix_host_data_ops = {
-    .read = pci_host_data_read,
-    .write = pci_host_data_write,
+const MemoryRegionOps ppc440_pcix_host_conf_ops = {
+    .read = pci_host_config_read,
+    .write = pci_host_config_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
@@ -497,9 +504,9 @@ static void ppc440_pcix_realize(DeviceState *dev, Error **errp)
     pci_setup_iommu(h->bus, ppc440_pcix_set_iommu, s);
 
     memory_region_init(&s->container, OBJECT(s), "pci-container", PCI_ALL_SIZE);
-    memory_region_init_io(&h->conf_mem, OBJECT(s), &pci_host_conf_le_ops,
+    memory_region_init_io(&h->conf_mem, OBJECT(s), &ppc440_pcix_host_conf_ops,
                           h, "pci-conf-idx", 4);
-    memory_region_init_io(&h->data_mem, OBJECT(s), &ppc440_pcix_host_data_ops,
+    memory_region_init_io(&h->data_mem, OBJECT(s), &pci_host_data_le_ops,
                           h, "pci-conf-data", 4);
     memory_region_init_io(&s->iomem, OBJECT(s), &pci_reg_ops, s,
                           "pci.reg", PPC440_REG_SIZE);
-- 
2.21.3

[PATCH v3 2/5] sam460ex: Remove FDT_PPC dependency from KConfig

[BALATON Zoltan via]

Dependency on FDT_PPC was added in commit b0048f76095
("hw/ppc/Kconfig: Only select FDT helper for machines using it") but
it does not seem to be really necessary so remove it again.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ppc/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 8548f42b0d..f1e1be208e 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -64,7 +64,6 @@ config SAM460EX
     select SMBUS_EEPROM
     select USB_EHCI_SYSBUS
     select USB_OHCI
-    select FDT_PPC
 
 config PREP
     bool
-- 
2.21.3

[PATCH v3 3/5] ppc440_pcix: Improve comment for IRQ mapping

[BALATON Zoltan via]

The code mapping all PCI interrupts to a single CPU IRQ works but is
not trivial so document it in a comment.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc440_pcix.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
index ee952314c8..eb1290ffc8 100644
--- a/hw/ppc/ppc440_pcix.c
+++ b/hw/ppc/ppc440_pcix.c
@@ -415,8 +415,15 @@ static void ppc440_pcix_reset(DeviceState *dev)
     s->sts = 0;
 }
 
-/* All pins from each slot are tied to a single board IRQ.
- * This may need further refactoring for other boards. */
+/*
+ * All four IRQ[ABCD] pins from all slots are tied to a single board
+ * IRQ, so our mapping function here maps everything to IRQ 0.
+ * The code in pci_change_irq_level() tracks the number of times
+ * the mapped IRQ is asserted and deasserted, so if multiple devices
+ * assert an IRQ at the same time the behaviour is correct.
+ *
+ * This may need further refactoring for boards that use multiple IRQ lines.
+ */
 static int ppc440_pcix_map_irq(PCIDevice *pci_dev, int irq_num)
 {
     trace_ppc440_pcix_map_irq(pci_dev->devfn, irq_num, 0);
-- 
2.21.3

[PATCH v3 4/5] ppc440_pcix: Fix register write trace event

[BALATON Zoltan via]

The trace event for pci_host_config_write() was also using the trace
event for read. Add corresponding trace and correct this.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc440_pcix.c | 2 +-
 hw/ppc/trace-events  | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
index eb1290ffc8..7829d3e556 100644
--- a/hw/ppc/ppc440_pcix.c
+++ b/hw/ppc/ppc440_pcix.c
@@ -169,7 +169,7 @@ static void ppc440_pcix_reg_write4(void *opaque, hwaddr addr,
 {
     struct PPC440PCIXState *s = opaque;
 
-    trace_ppc440_pcix_reg_read(addr, val);
+    trace_ppc440_pcix_reg_write(addr, val, size);
     switch (addr) {
     case PCI_VENDOR_ID ... PCI_MAX_LAT:
         stl_le_p(s->dev->config + addr, val);
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index ed05f2fc9a..017c48624f 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -117,3 +117,4 @@ ppc440_pcix_set_irq(int irq_num) "PCI irq %d"
 ppc440_pcix_update_pim(int idx, uint64_t size, uint64_t la) "Added window %d of size=0x%" PRIx64 " to CPU=0x%" PRIx64
 ppc440_pcix_update_pom(int idx, uint32_t size, uint64_t la, uint64_t pcia) "Added window %d of size=0x%x from CPU=0x%" PRIx64 " to PCI=0x%" PRIx64
 ppc440_pcix_reg_read(uint64_t addr, uint32_t val) "addr 0x%" PRIx64 " = 0x%" PRIx32
+ppc440_pcix_reg_write(uint64_t addr, uint32_t val, uint32_t size) "addr 0x%" PRIx64 " = 0x%" PRIx32 " size 0x%" PRIx32
-- 
2.21.3

Re: [PATCH v3 1/5] ppc4xx: Move common dependency on serial to common option

[Thomas Huth]

On 03/01/2021 02.09, BALATON Zoltan via wrote:
> All machines that select SERIAL also select PPC4XX so we can just add
> this common dependency there once.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/Kconfig | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index dd86e664d2..8548f42b0d 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -37,7 +37,6 @@ config PPC405
>       select M48T59
>       select PFLASH_CFI02
>       select PPC4XX
> -    select SERIAL
>   
>   config PPC440
>       bool
> @@ -46,13 +45,13 @@ config PPC440
>       imply E1000_PCI
>       select PCI_EXPRESS
>       select PPC4XX
> -    select SERIAL
>       select FDT_PPC
>   
>   config PPC4XX
>       bool
>       select BITBANG_I2C
>       select PCI
> +    select SERIAL

Not sure whether this is really the right way to go... serial_hd() and 
serial_mm_init() are only used in ppc405_uc.c
and ppc440_bamboo.c, so IMHO it does make sense to keep the SERIAL setting 
with PPC405 and PPC440.


>   config SAM460EX
>       bool
> @@ -61,7 +60,6 @@ config SAM460EX
>       select IDE_SII3112
>       select M41T80
>       select PPC440
> -    select SERIAL

But this hunk here should be fine, I think, since PPC440 already includes 
the SERIAL switch.

  Thomas


>       select SM501
>       select SMBUS_EEPROM
>       select USB_EHCI_SYSBUS
> @@ -123,7 +121,6 @@ config VIRTEX
>       bool
>       select PPC4XX
>       select PFLASH_CFI01
> -    select SERIAL
>       select XILINX
>       select XILINX_ETHLITE
>       select FDT_PPC
> 

Re: [PATCH v3 4/5] ppc440_pcix: Fix register write trace event

[Philippe Mathieu-Daudé]

On 1/3/21 2:09 AM, BALATON Zoltan via wrote:
> The trace event for pci_host_config_write() was also using the trace
> event for read. Add corresponding trace and correct this.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ppc/ppc440_pcix.c | 2 +-
>  hw/ppc/trace-events  | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 0/5] Misc sam460ex fixes (was: Clean up sam460ex irq mapping)

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

On Sun, Jan 03, 2021 at 02:09:33AM +0100, BALATON Zoltan via wrote:
> So this is v3 of a series that started to fix a potential problem with
> irq mapping in pci440_pcix (used by sam460ex) that got some other
> fixes along the way as by-products. But it turns out the irq issue
> this was trying to fix is not really a problem so finally we just
> update the comment for now documenting why it works and only the
> by-products remain in this series. Of which there are two more in this
> v3 finally fixing a long standing problem booting MorphOS on sam460ex
> (which I've got after debugging similar problem with pegasos2/vt8231
> that gave me an idea how to debug this.)

Applied to ppc-for-6.0.

So.. you're pretty much the only person who's shown any interest in
the embedded ppc stuff in qemu for a pretty long time.  Any chance I
could convince you to be ppc4xx submaintainer?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/5] Misc sam460ex fixes (was: Clean up sam460ex irq mapping)

[BALATON Zoltan via]

On Mon, 4 Jan 2021, David Gibson wrote:
> On Sun, Jan 03, 2021 at 02:09:33AM +0100, BALATON Zoltan via wrote:
>> So this is v3 of a series that started to fix a potential problem with
>> irq mapping in pci440_pcix (used by sam460ex) that got some other
>> fixes along the way as by-products. But it turns out the irq issue
>> this was trying to fix is not really a problem so finally we just
>> update the comment for now documenting why it works and only the
>> by-products remain in this series. Of which there are two more in this
>> v3 finally fixing a long standing problem booting MorphOS on sam460ex
>> (which I've got after debugging similar problem with pegasos2/vt8231
>> that gave me an idea how to debug this.)
>
> Applied to ppc-for-6.0.
>
> So.. you're pretty much the only person who's shown any interest in
> the embedded ppc stuff in qemu for a pretty long time.  Any chance I
> could convince you to be ppc4xx submaintainer?

Not if it involves testing or sending pull requests because I don't have 
time for that. I'm already listed as maintainer for stuff I contributed 
and try to review and test changes to that (i.e. sam460ex) but I'm afraid 
that's all I can do.

Regards,
BALATON Zoltan

Re: [PATCH v3 1/5] ppc4xx: Move common dependency on serial to common option

[BALATON Zoltan via]

On Sun, 3 Jan 2021, Thomas Huth wrote:
> On 03/01/2021 02.09, BALATON Zoltan via wrote:
>> All machines that select SERIAL also select PPC4XX so we can just add
>> this common dependency there once.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ppc/Kconfig | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>> 
>> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
>> index dd86e664d2..8548f42b0d 100644
>> --- a/hw/ppc/Kconfig
>> +++ b/hw/ppc/Kconfig
>> @@ -37,7 +37,6 @@ config PPC405
>>       select M48T59
>>       select PFLASH_CFI02
>>       select PPC4XX
>> -    select SERIAL
>>     config PPC440
>>       bool
>> @@ -46,13 +45,13 @@ config PPC440
>>       imply E1000_PCI
>>       select PCI_EXPRESS
>>       select PPC4XX
>> -    select SERIAL
>>       select FDT_PPC
>>     config PPC4XX
>>       bool
>>       select BITBANG_I2C
>>       select PCI
>> +    select SERIAL
>
> Not sure whether this is really the right way to go... serial_hd() and 
> serial_mm_init() are only used in ppc405_uc.c
> and ppc440_bamboo.c, so IMHO it does make sense to keep the SERIAL setting 
> with PPC405 and PPC440.
>
>
>>   config SAM460EX
>>       bool
>> @@ -61,7 +60,6 @@ config SAM460EX
>>       select IDE_SII3112
>>       select M41T80
>>       select PPC440
>> -    select SERIAL
>
> But this hunk here should be fine, I think, since PPC440 already includes the 
> SERIAL switch.

If it's OK in this case why doesn't the same argument apply in case of 
PPC440 including PPC4XX which then includes SERIAL. All these boards use 
serial_mm_init but they also all include PPC4XX either directly or 
indirectly via an intermeriate option such as PPC440 or PPC405.

Regards,
BALATON Zoltan

> Thomas
>
>
>>       select SM501
>>       select SMBUS_EEPROM
>>       select USB_EHCI_SYSBUS
>> @@ -123,7 +121,6 @@ config VIRTEX
>>       bool
>>       select PPC4XX
>>       select PFLASH_CFI01
>> -    select SERIAL
>>       select XILINX
>>       select XILINX_ETHLITE
>>       select FDT_PPC
>> 
>
>

Re: [PATCH v3 1/5] ppc4xx: Move common dependency on serial to common option

[Thomas Huth]

On 04/01/2021 02.39, BALATON Zoltan via wrote:
> On Sun, 3 Jan 2021, Thomas Huth wrote:
>> On 03/01/2021 02.09, BALATON Zoltan via wrote:
>>> All machines that select SERIAL also select PPC4XX so we can just add
>>> this common dependency there once.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/ppc/Kconfig | 5 +----
>>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
>>> index dd86e664d2..8548f42b0d 100644
>>> --- a/hw/ppc/Kconfig
>>> +++ b/hw/ppc/Kconfig
>>> @@ -37,7 +37,6 @@ config PPC405
>>>       select M48T59
>>>       select PFLASH_CFI02
>>>       select PPC4XX
>>> -    select SERIAL
>>>     config PPC440
>>>       bool
>>> @@ -46,13 +45,13 @@ config PPC440
>>>       imply E1000_PCI
>>>       select PCI_EXPRESS
>>>       select PPC4XX
>>> -    select SERIAL
>>>       select FDT_PPC
>>>     config PPC4XX
>>>       bool
>>>       select BITBANG_I2C
>>>       select PCI
>>> +    select SERIAL
>>
>> Not sure whether this is really the right way to go... serial_hd() and 
>> serial_mm_init() are only used in ppc405_uc.c
>> and ppc440_bamboo.c, so IMHO it does make sense to keep the SERIAL setting 
>> with PPC405 and PPC440.
>>
>>
>>>   config SAM460EX
>>>       bool
>>> @@ -61,7 +60,6 @@ config SAM460EX
>>>       select IDE_SII3112
>>>       select M41T80
>>>       select PPC440
>>> -    select SERIAL
>>
>> But this hunk here should be fine, I think, since PPC440 already includes 
>> the SERIAL switch.
> 
> If it's OK in this case why doesn't the same argument apply in case of 
> PPC440 including PPC4XX which then includes SERIAL. All these boards use 
> serial_mm_init but they also all include PPC4XX either directly or 
> indirectly via an intermeriate option such as PPC440 or PPC405.

Yeah, it's likely cleaner if you keep the "select SERIAL" here, too, since 
sam460ex.c uses serial_hd() and serial_mm_init() directly, too.

I guess we should rather have a separate switch for the bamboo board, and 
then move the "select SERIAL" from the PPC440 section into the BAMBOO section...

  Thomas