Per the datasheet (reference added in file header, p.9)
'Programming Model' -> 'Register Data Types and Organization':
"The XPS INTC registers are read as big-endian data"
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/intc/xilinx_intc.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
index 1762b34564e..71f743a1f14 100644
--- a/hw/intc/xilinx_intc.c
+++ b/hw/intc/xilinx_intc.c
@@ -3,6 +3,9 @@
*
* Copyright (c) 2009 Edgar E. Iglesias.
*
+ * https://docs.amd.com/v/u/en-US/xps_intc
+ * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a)
+ *
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
@@ -143,12 +146,20 @@ static void pic_write(void *opaque, hwaddr addr,
static const MemoryRegionOps pic_ops = {
.read = pic_read,
.write = pic_write,
- .endianness = DEVICE_NATIVE_ENDIAN,
+ /* The XPS INTC registers are read as big-endian data. */
+ .endianness = DEVICE_BIG_ENDIAN,
.impl = {
.min_access_size = 4,
.max_access_size = 4,
},
.valid = {
+ /*
+ * All XPS INTC registers are accessed through the PLB interface.
+ * The base address for these registers is provided by the
+ * configuration parameter, C_BASEADDR. Each register is 32 bits
+ * although some bits may be unused and is accessed on a 4-byte
+ * boundary offset from the base address.
+ */
.min_access_size = 4,
.max_access_size = 4,
},
--
2.45.2On Tue, Nov 05, 2024 at 02:04:21PM +0100, Philippe Mathieu-Daudé wrote:
> Per the datasheet (reference added in file header, p.9)
> 'Programming Model' -> 'Register Data Types and Organization':
>
> "The XPS INTC registers are read as big-endian data"
Hi Phil,
Some of these devices exist in both big and little endian versions.
So far we've reused the same model by using DEVICE_NATIVE_ENDIAN.
Here's the little endian version:
https://docs.amd.com/v/u/en-US/ds747_axi_intc
Can we have add property to select the endianess?
For the Xilinx use-cases I think it may be a good idea to default it
to little endian and have the big-endian machines explicitly set it.
Cheers,
Edgar
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/intc/xilinx_intc.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
> index 1762b34564e..71f743a1f14 100644
> --- a/hw/intc/xilinx_intc.c
> +++ b/hw/intc/xilinx_intc.c
> @@ -3,6 +3,9 @@
> *
> * Copyright (c) 2009 Edgar E. Iglesias.
> *
> + * https://docs.amd.com/v/u/en-US/xps_intc
> + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a)
> + *
> * Permission is hereby granted, free of charge, to any person obtaining a copy
> * of this software and associated documentation files (the "Software"), to deal
> * in the Software without restriction, including without limitation the rights
> @@ -143,12 +146,20 @@ static void pic_write(void *opaque, hwaddr addr,
> static const MemoryRegionOps pic_ops = {
> .read = pic_read,
> .write = pic_write,
> - .endianness = DEVICE_NATIVE_ENDIAN,
> + /* The XPS INTC registers are read as big-endian data. */
> + .endianness = DEVICE_BIG_ENDIAN,
> .impl = {
> .min_access_size = 4,
> .max_access_size = 4,
> },
> .valid = {
> + /*
> + * All XPS INTC registers are accessed through the PLB interface.
> + * The base address for these registers is provided by the
> + * configuration parameter, C_BASEADDR. Each register is 32 bits
> + * although some bits may be unused and is accessed on a 4-byte
> + * boundary offset from the base address.
> + */
> .min_access_size = 4,
> .max_access_size = 4,
> },
> --
> 2.45.2
>
+Michal for Linux driver
On 5/11/24 23:08, Edgar E. Iglesias wrote:
> On Tue, Nov 05, 2024 at 02:04:21PM +0100, Philippe Mathieu-Daudé wrote:
>> Per the datasheet (reference added in file header, p.9)
>> 'Programming Model' -> 'Register Data Types and Organization':
>>
>> "The XPS INTC registers are read as big-endian data"
>
> Hi Phil,
>
> Some of these devices exist in both big and little endian versions.
> So far we've reused the same model by using DEVICE_NATIVE_ENDIAN.
>
> Here's the little endian version:
> https://docs.amd.com/v/u/en-US/ds747_axi_intc
This model is specified as:
- https://docs.amd.com/v/u/en-US/xps_intc
LogiCORE IP XPS Interrupt Controller (v2.01a)
DS572 April 19, 2010
Spec is from 2010, you added it in 2009:
commit 17628bc642260df3a07b9df8b8a9ca7da2e7e87c
Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Date: Wed May 20 20:11:30 2009 +0200
xilinx: Add interrupt controller.
The spec is explicit:
"The XPS INTC registers are read as big-endian data"
The other model you mention is:
- https://docs.amd.com/v/u/en-US/ds747_axi_intc
LogiCORE IP AXI INTC (v1.04a)
DS747 June 19, 2013
The spec is more recent than your addition, and contains
the Interrupt Vector Address Register (IVAR) which is not
present in our model.
Indeed the latter seems to extend the former, but they are
not the same and we need some work to model the latter.
The endianness explicit for each model (and is not listed
in the "IP Configurable Design Parameters" tables).
That said, let's look at Linux use. Driver was added in:
commit eedbdab99fffb8ed71cac75a722088b8ace2583c
Author: Michal Simek <monstr@monstr.eu>
Date: Fri Mar 27 14:25:49 2009 +0100
microblaze_v8: Interrupt handling and timer support
Using explicit big-endian API:
+void __init init_IRQ(void)
+{
...
+ /*
+ * Disable all external interrupts until they are
+ * explicity requested.
+ */
+ out_be32(intc_baseaddr + IER, 0);
+
+ /* Acknowledge any pending interrupts just in case. */
+ out_be32(intc_baseaddr + IAR, 0xffffffff);
+
+ /* Turn on the Master Enable. */
+ out_be32(intc_baseaddr + MER, MER_HIE | MER_ME);
Then the driver became clever in:
commit 1aa1243c339d4c902c0f9c1ced45742729a86e6a
Author: Michal Simek <michal.simek@amd.com>
Date: Mon Feb 24 14:56:32 2014 +0100
microblaze: Make intc driver endian aware
Detect endianess directly on the hardware and use
ioread/iowrite functions.
+static void intc_write32(u32 val, void __iomem *addr)
+{
+ iowrite32(val, addr);
+}
+
+static void intc_write32_be(u32 val, void __iomem *addr)
+{
+ iowrite32be(val, addr);
+}
@@ -140,17 +163,25 @@ static int __init xilinx_intc_of_init(struct
device_node *intc,
+ write_fn = intc_write32;
+ read_fn = intc_read32;
+
/*
* Disable all external interrupts until they are
* explicity requested.
*/
- out_be32(intc_baseaddr + IER, 0);
+ write_fn(0, intc_baseaddr + IER);
/* Acknowledge any pending interrupts just in case. */
- out_be32(intc_baseaddr + IAR, 0xffffffff);
+ write_fn(0xffffffff, intc_baseaddr + IAR);
/* Turn on the Master Enable. */
- out_be32(intc_baseaddr + MER, MER_HIE | MER_ME);
+ write_fn(MER_HIE | MER_ME, intc_baseaddr + MER);
+ if (!(read_fn(intc_baseaddr + MER) & (MER_HIE | MER_ME))) {
+ write_fn = intc_write32_be;
+ read_fn = intc_read32_be;
+ write_fn(MER_HIE | MER_ME, intc_baseaddr + MER);
+ }
Interestingly little endianness became the default, although
the driver detect it on init.
This is from 2014, maybe to work with the "LogiCORE IP AXI INTC"
you mentioned, which spec date is 2013.
Indeed when forcing different endianness [*], the Linux kernel used
in our tests (tests/functional/test_microblaze*) does the check
and ends up using the correct INTC endianness:
LE CPU, LE INTC model
pic_write addr=8 val=0
pic_write addr=c val=ffffffff
pic_write addr=1c val=3 <- LE
pic_read 1c=3
pic_write addr=c val=1
pic_write addr=10 val=1
pic_read 18=0
LE CPU, BE INTC model
pic_write addr=8 val=0
pic_write addr=c val=ffffffff
pic_write addr=1c val=3000000 <- LE test
pic_read 1c=0
pic_write addr=1c val=3 <- BE
pic_write addr=c val=1
pic_write addr=10 val=1
pic_read 18=0
BE CPU, BE INTC model
pic_write addr=8 val=0
pic_write addr=c val=ffffffff
pic_write addr=1c val=3000000 <- LE test
pic_read 1c=0
pic_write addr=1c val=3 <- BE
pic_write addr=c val=1
pic_write addr=10 val=1
pic_read 18=0
BE CPU, LE INTC model
pic_write addr=8 val=0
pic_write addr=c val=ffffffff
pic_write addr=1c val=3 <- LE
pic_read 1c=3
pic_write addr=c val=1
pic_write addr=10 val=1
pic_read 18=0
IMHO this patch behavior is correct. Besides, I don't expect
firmwares to be as clever as Linux.
> Can we have add property to select the endianess?
> For the Xilinx use-cases I think it may be a good idea to default it
> to little endian and have the big-endian machines explicitly set it.
What you suggested is implemented in v3:
https://lore.kernel.org/qemu-devel/20241108154317.12129-4-philmd@linaro.org/
but after the analysis, I wonder if it isn't safer to not
make the endianness configurable, but expose as 2 models:
- xlnx,xps_intc (2010) in BE
- xlnx,axi_intc (2013) in LE
>
> Cheers,
> Edgar
>
>
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/intc/xilinx_intc.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
>> index 1762b34564e..71f743a1f14 100644
>> --- a/hw/intc/xilinx_intc.c
>> +++ b/hw/intc/xilinx_intc.c
>> @@ -3,6 +3,9 @@
>> *
>> * Copyright (c) 2009 Edgar E. Iglesias.
>> *
>> + * https://docs.amd.com/v/u/en-US/xps_intc
>> + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a)
>> + *
>> * Permission is hereby granted, free of charge, to any person obtaining a copy
>> * of this software and associated documentation files (the "Software"), to deal
>> * in the Software without restriction, including without limitation the rights
>> @@ -143,12 +146,20 @@ static void pic_write(void *opaque, hwaddr addr,
>> static const MemoryRegionOps pic_ops = {
>> .read = pic_read,
>> .write = pic_write,
>> - .endianness = DEVICE_NATIVE_ENDIAN,
>> + /* The XPS INTC registers are read as big-endian data. */
>> + .endianness = DEVICE_BIG_ENDIAN,
>> .impl = {
>> .min_access_size = 4,
>> .max_access_size = 4,
>> },
>> .valid = {
>> + /*
>> + * All XPS INTC registers are accessed through the PLB interface.
>> + * The base address for these registers is provided by the
>> + * configuration parameter, C_BASEADDR. Each register is 32 bits
>> + * although some bits may be unused and is accessed on a 4-byte
>> + * boundary offset from the base address.
>> + */
>> .min_access_size = 4,
>> .max_access_size = 4,
>> },
>> --
>> 2.45.2
>>
[*] diff used:
-- >8 --
@@ -32,7 +45,7 @@
diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
index fc55c8afca..883ec685f4 100644
--- a/hw/intc/xilinx_intc.c
+++ b/hw/intc/xilinx_intc.c
#include "hw/qdev-properties.h"
#include "qom/object.h"
-#define D(x)
+#define D(x) x
#define R_ISR 0
#define R_IPR 1
@@ -105,7 +122,7 @@ static uint64_t pic_read(void *opaque, hwaddr addr,
unsigned int size)
break;
}
- D(printf("%s %x=%x\n", __func__, addr * 4, r));
+ D(printf("%s %llx=%x\n", __func__, addr * 4, r));
return r;
}
@@ -116,7 +133,7 @@ static void pic_write(void *opaque, hwaddr addr,
uint32_t value = val64;
addr >>= 2;
- D(qemu_log("%s addr=%x val=%x\n", __func__, addr * 4, value));
+ D(printf("%s addr=%llx val=%x\n", __func__, addr * 4, value));
switch (addr)
{
case R_IAR:
---
On 11/14/24 23:43, Philippe Mathieu-Daudé wrote:
> +Michal for Linux driver
>
> On 5/11/24 23:08, Edgar E. Iglesias wrote:
>> On Tue, Nov 05, 2024 at 02:04:21PM +0100, Philippe Mathieu-Daudé wrote:
>>> Per the datasheet (reference added in file header, p.9)
>>> 'Programming Model' -> 'Register Data Types and Organization':
>>>
>>> "The XPS INTC registers are read as big-endian data"
>>
>> Hi Phil,
>>
>> Some of these devices exist in both big and little endian versions.
>> So far we've reused the same model by using DEVICE_NATIVE_ENDIAN.
>>
>> Here's the little endian version:
>> https://docs.amd.com/v/u/en-US/ds747_axi_intc
>
> This model is specified as:
>
> - https://docs.amd.com/v/u/en-US/xps_intc
> LogiCORE IP XPS Interrupt Controller (v2.01a)
> DS572 April 19, 2010
>
> Spec is from 2010, you added it in 2009:
>
> commit 17628bc642260df3a07b9df8b8a9ca7da2e7e87c
> Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Date: Wed May 20 20:11:30 2009 +0200
>
> xilinx: Add interrupt controller.
>
> The spec is explicit:
>
> "The XPS INTC registers are read as big-endian data"
>
>
> The other model you mention is:
>
> - https://docs.amd.com/v/u/en-US/ds747_axi_intc
> LogiCORE IP AXI INTC (v1.04a)
> DS747 June 19, 2013
>
> The spec is more recent than your addition, and contains
> the Interrupt Vector Address Register (IVAR) which is not
> present in our model.
>
>
> Indeed the latter seems to extend the former, but they are
> not the same and we need some work to model the latter.
>
> The endianness explicit for each model (and is not listed
> in the "IP Configurable Design Parameters" tables).
>
>
> That said, let's look at Linux use. Driver was added in:
>
> commit eedbdab99fffb8ed71cac75a722088b8ace2583c
> Author: Michal Simek <monstr@monstr.eu>
> Date: Fri Mar 27 14:25:49 2009 +0100
>
> microblaze_v8: Interrupt handling and timer support
>
> Using explicit big-endian API:
>
> +void __init init_IRQ(void)
> +{
> ...
> + /*
> + * Disable all external interrupts until they are
> + * explicity requested.
> + */
> + out_be32(intc_baseaddr + IER, 0);
> +
> + /* Acknowledge any pending interrupts just in case. */
> + out_be32(intc_baseaddr + IAR, 0xffffffff);
> +
> + /* Turn on the Master Enable. */
> + out_be32(intc_baseaddr + MER, MER_HIE | MER_ME);
>
> Then the driver became clever in:
>
> commit 1aa1243c339d4c902c0f9c1ced45742729a86e6a
> Author: Michal Simek <michal.simek@amd.com>
> Date: Mon Feb 24 14:56:32 2014 +0100
>
> microblaze: Make intc driver endian aware
>
> Detect endianess directly on the hardware and use
> ioread/iowrite functions.
>
> +static void intc_write32(u32 val, void __iomem *addr)
> +{
> + iowrite32(val, addr);
> +}
> +
> +static void intc_write32_be(u32 val, void __iomem *addr)
> +{
> + iowrite32be(val, addr);
> +}
>
> @@ -140,17 +163,25 @@ static int __init xilinx_intc_of_init(struct device_node
> *intc,
>
> + write_fn = intc_write32;
> + read_fn = intc_read32;
> +
> /*
> * Disable all external interrupts until they are
> * explicity requested.
> */
> - out_be32(intc_baseaddr + IER, 0);
> + write_fn(0, intc_baseaddr + IER);
>
> /* Acknowledge any pending interrupts just in case. */
> - out_be32(intc_baseaddr + IAR, 0xffffffff);
> + write_fn(0xffffffff, intc_baseaddr + IAR);
>
> /* Turn on the Master Enable. */
> - out_be32(intc_baseaddr + MER, MER_HIE | MER_ME);
> + write_fn(MER_HIE | MER_ME, intc_baseaddr + MER);
> + if (!(read_fn(intc_baseaddr + MER) & (MER_HIE | MER_ME))) {
> + write_fn = intc_write32_be;
> + read_fn = intc_read32_be;
> + write_fn(MER_HIE | MER_ME, intc_baseaddr + MER);
> + }
>
> Interestingly little endianness became the default, although
> the driver detect it on init.
>
> This is from 2014, maybe to work with the "LogiCORE IP AXI INTC"
> you mentioned, which spec date is 2013.
>
> Indeed when forcing different endianness [*], the Linux kernel used
> in our tests (tests/functional/test_microblaze*) does the check
> and ends up using the correct INTC endianness:
>
> LE CPU, LE INTC model
> pic_write addr=8 val=0
> pic_write addr=c val=ffffffff
> pic_write addr=1c val=3 <- LE
> pic_read 1c=3
> pic_write addr=c val=1
> pic_write addr=10 val=1
> pic_read 18=0
>
> LE CPU, BE INTC model
> pic_write addr=8 val=0
> pic_write addr=c val=ffffffff
> pic_write addr=1c val=3000000 <- LE test
> pic_read 1c=0
> pic_write addr=1c val=3 <- BE
> pic_write addr=c val=1
> pic_write addr=10 val=1
> pic_read 18=0
>
> BE CPU, BE INTC model
> pic_write addr=8 val=0
> pic_write addr=c val=ffffffff
> pic_write addr=1c val=3000000 <- LE test
> pic_read 1c=0
> pic_write addr=1c val=3 <- BE
> pic_write addr=c val=1
> pic_write addr=10 val=1
> pic_read 18=0
>
> BE CPU, LE INTC model
> pic_write addr=8 val=0
> pic_write addr=c val=ffffffff
> pic_write addr=1c val=3 <- LE
> pic_read 1c=3
> pic_write addr=c val=1
> pic_write addr=10 val=1
> pic_read 18=0
>
>
> IMHO this patch behavior is correct. Besides, I don't expect
> firmwares to be as clever as Linux.
>
>> Can we have add property to select the endianess?
>> For the Xilinx use-cases I think it may be a good idea to default it
>> to little endian and have the big-endian machines explicitly set it.
>
> What you suggested is implemented in v3:
> https://lore.kernel.org/qemu-devel/20241108154317.12129-4-philmd@linaro.org/
> but after the analysis, I wonder if it isn't safer to not
> make the endianness configurable, but expose as 2 models:
> - xlnx,xps_intc (2010) in BE
> - xlnx,axi_intc (2013) in LE
It is a little bit more complicated.
In past everything started as big endian on OPB bus. Then PLB bus still big
endian and then AXI came and things have been moved to little endian.
That's from bus perspective.
From CPU perspective itself till AXI microblaze was big endian only. With AXI
cpu started to be by default little endian but still today you can still
configured cpu to be big endian (C_ENDIANNESS config) with using AXI bus.
Truth is that I am not aware about anybody configuring MB to big endian and we
are on AXI for quite a long time. There is still code in Linux kernel for it but
I can't see any reason to keep it around. I don't think that make sense to keep
big endian configurations alive at all.
Thanks,
Michal
On Tue, Nov 5, 2024 at 11:08 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Per the datasheet (reference added in file header, p.9)
> 'Programming Model' -> 'Register Data Types and Organization':
>
> "The XPS INTC registers are read as big-endian data"
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/intc/xilinx_intc.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
> index 1762b34564e..71f743a1f14 100644
> --- a/hw/intc/xilinx_intc.c
> +++ b/hw/intc/xilinx_intc.c
> @@ -3,6 +3,9 @@
> *
> * Copyright (c) 2009 Edgar E. Iglesias.
> *
> + * https://docs.amd.com/v/u/en-US/xps_intc
> + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a)
> + *
> * Permission is hereby granted, free of charge, to any person obtaining a copy
> * of this software and associated documentation files (the "Software"), to deal
> * in the Software without restriction, including without limitation the rights
> @@ -143,12 +146,20 @@ static void pic_write(void *opaque, hwaddr addr,
> static const MemoryRegionOps pic_ops = {
> .read = pic_read,
> .write = pic_write,
> - .endianness = DEVICE_NATIVE_ENDIAN,
> + /* The XPS INTC registers are read as big-endian data. */
> + .endianness = DEVICE_BIG_ENDIAN,
> .impl = {
> .min_access_size = 4,
> .max_access_size = 4,
> },
> .valid = {
> + /*
> + * All XPS INTC registers are accessed through the PLB interface.
> + * The base address for these registers is provided by the
> + * configuration parameter, C_BASEADDR. Each register is 32 bits
> + * although some bits may be unused and is accessed on a 4-byte
> + * boundary offset from the base address.
> + */
> .min_access_size = 4,
> .max_access_size = 4,
> },
> --
> 2.45.2
>
>
On 05/11/24, Philippe Mathieu-Daudé wrote: > Per the datasheet (reference added in file header, p.9) > 'Programming Model' -> 'Register Data Types and Organization': > > "The XPS INTC registers are read as big-endian data" > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/intc/xilinx_intc.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) Reviewed-by: Anton Johansson <anjo@rev.ng>
© 2016 - 2024 Red Hat, Inc.