[Qemu-devel] [PATCH] ppc: Fix sam460ex devicetree when booting the Linux kernel

[Qemu-devel] [PATCH] ppc: Fix sam460ex devicetree when booting the Linux kernel

[Guenter Roeck]

sam4660ex (or at least this emulation) does not support the "ibm,cpm" power
management. As a result, Linux crashes when trying to access it. Remove
its devicetree node. Also, if/when we boot the Linux kernel directly,
u-boot will not fix up serial frequencies in the devicetree file, and
serial port initialization will fail. Add plausible frequency values to the
first serial port to be able to use it. Disable the second serial port
since it is not available on the board.  Also set valid values for the
other clock nodes otherwise set by u-boot.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/ppc/sam460ex.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index bdc53d2..045a255 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -308,6 +308,24 @@ static int sam460ex_load_device_tree(hwaddr addr,
     qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "timebase-frequency",
                               tb_freq);
 
+    /* Remove cpm node (not emulated) */
+    qemu_fdt_nop_node(fdt, "/cpm");
+    /* set serial port clock and speed */
+    qemu_fdt_setprop_cell(fdt, "/plb/opb/serial@ef600300", "clock-frequency",
+                              50000000);
+    qemu_fdt_setprop_cell(fdt, "/plb/opb/serial@ef600300", "current-speed",
+                              38400);
+    /* disable second serial port */
+    qemu_fdt_setprop_string(fdt, "/plb/opb/serial@ef600400", "status",
+                              "disabled");
+    /* some more clocks */
+    qemu_fdt_setprop_cell(fdt, "/plb", "clock-frequency",
+                              50000000);
+    qemu_fdt_setprop_cell(fdt, "/plb/opb", "clock-frequency",
+                              50000000);
+    qemu_fdt_setprop_cell(fdt, "/plb/opb/ebc", "clock-frequency",
+                              50000000);
+
     rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
     g_free(fdt);
     ret = fdt_size;
-- 
2.7.4

Re: [Qemu-devel] [PATCH] ppc: Fix sam460ex devicetree when booting the Linux kernel

[David Gibson]

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

Balaton, can you review this please.

On Thu, Jun 21, 2018 at 09:47:51PM -0700, Guenter Roeck wrote:
> sam4660ex (or at least this emulation) does not support the "ibm,cpm" power
> management. As a result, Linux crashes when trying to access it. Remove
> its devicetree node. Also, if/when we boot the Linux kernel directly,
> u-boot will not fix up serial frequencies in the devicetree file, and
> serial port initialization will fail. Add plausible frequency values to the
> first serial port to be able to use it. Disable the second serial port
> since it is not available on the board.  Also set valid values for the
> other clock nodes otherwise set by u-boot.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  hw/ppc/sam460ex.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index bdc53d2..045a255 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -308,6 +308,24 @@ static int sam460ex_load_device_tree(hwaddr addr,
>      qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "timebase-frequency",
>                                tb_freq);
>  
> +    /* Remove cpm node (not emulated) */
> +    qemu_fdt_nop_node(fdt, "/cpm");
> +    /* set serial port clock and speed */
> +    qemu_fdt_setprop_cell(fdt, "/plb/opb/serial@ef600300", "clock-frequency",
> +                              50000000);
> +    qemu_fdt_setprop_cell(fdt, "/plb/opb/serial@ef600300", "current-speed",
> +                              38400);
> +    /* disable second serial port */
> +    qemu_fdt_setprop_string(fdt, "/plb/opb/serial@ef600400", "status",
> +                              "disabled");
> +    /* some more clocks */
> +    qemu_fdt_setprop_cell(fdt, "/plb", "clock-frequency",
> +                              50000000);
> +    qemu_fdt_setprop_cell(fdt, "/plb/opb", "clock-frequency",
> +                              50000000);
> +    qemu_fdt_setprop_cell(fdt, "/plb/opb/ebc", "clock-frequency",
> +                              50000000);
> +
>      rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
>      g_free(fdt);
>      ret = fdt_size;

-- 
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: [Qemu-devel] [PATCH] ppc: Fix sam460ex devicetree when booting the Linux kernel

[BALATON Zoltan]

Hello,

Thanks for the feedback, really appreciated.

On Fri, 22 Jun 2018, David Gibson wrote:
> On Thu, Jun 21, 2018 at 09:47:51PM -0700, Guenter Roeck wrote:
>> sam4660ex (or at least this emulation) does not support the "ibm,cpm" power
>> management. As a result, Linux crashes when trying to access it. Remove
>> its devicetree node. Also, if/when we boot the Linux kernel directly,
>> u-boot will not fix up serial frequencies in the devicetree file, and
>> serial port initialization will fail. Add plausible frequency values to the
>> first serial port to be able to use it. Disable the second serial port
>> since it is not available on the board.  Also set valid values for the
>> other clock nodes otherwise set by u-boot.

Patching clock values when using -kernel instead of u-boot looks good. 
Although I've tested booting a Linux kernel and could see serial output 
while the kernel boots, it only went silent after started user space. Is 
this the same you see or you don't get serial output (with loglevel set to 
some higher value) even during kernel boot? Which kernel and image do you 
test with?

Does leaving second UART in device tree cause any problems? The fdt in 
kernel has these and I'm not sure u-boot would patch this out. Do we need 
to remove this? This did not seem to cause any problem with guests I've 
tried so far. Does real hardware have a different fdt than the stock 
kernel one (which is also what's downloadable from ACube's site).

The version of the Linux kernel I've tried (which is from the Linux CD on 
ACube's site) did not try to access the power management register, neither 
any guest OSes I've tested with. Looks like it may be specific to the 
kernel config you're using.

By the way, when I've tried with a more recent Linux kernel (4.15.10) I've 
noticed that the sm501 driver seemed like having endianness problems and 
thus did not find the chip, while it works with other older kernels made 
for sam460ex. I did not try to debug or bisect this yet. Do you know 
anything about that?

Regards,
BALATON Zoltan

>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  hw/ppc/sam460ex.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index bdc53d2..045a255 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -308,6 +308,24 @@ static int sam460ex_load_device_tree(hwaddr addr,
>>      qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "timebase-frequency",
>>                                tb_freq);
>>
>> +    /* Remove cpm node (not emulated) */
>> +    qemu_fdt_nop_node(fdt, "/cpm");
>> +    /* set serial port clock and speed */
>> +    qemu_fdt_setprop_cell(fdt, "/plb/opb/serial@ef600300", "clock-frequency",
>> +                              50000000);
>> +    qemu_fdt_setprop_cell(fdt, "/plb/opb/serial@ef600300", "current-speed",
>> +                              38400);
>> +    /* disable second serial port */
>> +    qemu_fdt_setprop_string(fdt, "/plb/opb/serial@ef600400", "status",
>> +                              "disabled");
>> +    /* some more clocks */
>> +    qemu_fdt_setprop_cell(fdt, "/plb", "clock-frequency",
>> +                              50000000);
>> +    qemu_fdt_setprop_cell(fdt, "/plb/opb", "clock-frequency",
>> +                              50000000);
>> +    qemu_fdt_setprop_cell(fdt, "/plb/opb/ebc", "clock-frequency",
>> +                              50000000);
>> +
>>      rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
>>      g_free(fdt);
>>      ret = fdt_size;
>
>

Re: [Qemu-devel] [PATCH] ppc: Fix sam460ex devicetree when booting the Linux kernel

[Guenter Roeck]

On 06/22/2018 12:52 AM, BALATON Zoltan wrote:
> Hello,
> 
> Thanks for the feedback, really appreciated.
> 
> On Fri, 22 Jun 2018, David Gibson wrote:
>> On Thu, Jun 21, 2018 at 09:47:51PM -0700, Guenter Roeck wrote:
>>> sam4660ex (or at least this emulation) does not support the "ibm,cpm" power
>>> management. As a result, Linux crashes when trying to access it. Remove
>>> its devicetree node. Also, if/when we boot the Linux kernel directly,
>>> u-boot will not fix up serial frequencies in the devicetree file, and
>>> serial port initialization will fail. Add plausible frequency values to the
>>> first serial port to be able to use it. Disable the second serial port
>>> since it is not available on the board.  Also set valid values for the
>>> other clock nodes otherwise set by u-boot.
> 
> Patching clock values when using -kernel instead of u-boot looks good. Although I've tested booting a Linux kernel and could see serial output while the kernel boots, it only went silent after started user space. Is this the same you see or you don't get serial output (with loglevel set to some higher value) even during kernel boot? Which kernel and image do you test with?
> 
I test with all stable kernel releases starting with 3.16.

Correct, this is exactly what I was seeing as well. I also noticed that the
system keeps running, only there is no serial output.

> Does leaving second UART in device tree cause any problems? The fdt in kernel has these and I'm not sure u-boot would patch this out. Do we need to remove this? This did not seem to cause any problem with guests I've tried so far. Does real hardware have a different fdt than the stock kernel one (which is also what's downloadable from ACube's site).
> 
Only that the port fails to initialize without clock. We could leave it in the tree,
but it would only have value if the clocks are actually set, and then it would
be a qemu-only value (presumably; I read somewhere that the board only has a single
serial port). I don't have a strong opinion either way. Also, I don't know what
u-boot does.

> The version of the Linux kernel I've tried (which is from the Linux CD on ACube's site) did not try to access the power management register, neither any guest OSes I've tested with. Looks like it may be specific to the kernel config you're using.
> 
Interesting. This is with the standard upstream kernel, using canyonlands_defconfig.
The code seems to have been in the upstream kernel forever.

> By the way, when I've tried with a more recent Linux kernel (4.15.10) I've noticed that the sm501 driver seemed like having endianness problems and thus did not find the chip, while it works with other older kernels made for sam460ex. I did not try to debug or bisect this yet. Do you know anything about that?
> 

No, I had not noticed. SM501 is disabled in the latest canyonlands_defconfig,
and I only use the serial console for my testing. It fails as far back as 3.18.y,
so I am not sure if this is a Linux or a qemu problem, or if it is a problem that
was never fixed in the upstream kernel. What kernels did you try ?

Thanks,
Guenter

> Regards,
> BALATON Zoltan
> 
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>>  hw/ppc/sam460ex.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>>> index bdc53d2..045a255 100644
>>> --- a/hw/ppc/sam460ex.c
>>> +++ b/hw/ppc/sam460ex.c
>>> @@ -308,6 +308,24 @@ static int sam460ex_load_device_tree(hwaddr addr,
>>>      qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "timebase-frequency",
>>>                                tb_freq);
>>>
>>> +    /* Remove cpm node (not emulated) */
>>> +    qemu_fdt_nop_node(fdt, "/cpm");
>>> +    /* set serial port clock and speed */
>>> +    qemu_fdt_setprop_cell(fdt, "/plb/opb/serial@ef600300", "clock-frequency",
>>> +                              50000000);
>>> +    qemu_fdt_setprop_cell(fdt, "/plb/opb/serial@ef600300", "current-speed",
>>> +                              38400);
>>> +    /* disable second serial port */
>>> +    qemu_fdt_setprop_string(fdt, "/plb/opb/serial@ef600400", "status",
>>> +                              "disabled");
>>> +    /* some more clocks */
>>> +    qemu_fdt_setprop_cell(fdt, "/plb", "clock-frequency",
>>> +                              50000000);
>>> +    qemu_fdt_setprop_cell(fdt, "/plb/opb", "clock-frequency",
>>> +                              50000000);
>>> +    qemu_fdt_setprop_cell(fdt, "/plb/opb/ebc", "clock-frequency",
>>> +                              50000000);
>>> +
>>>      rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
>>>      g_free(fdt);
>>>      ret = fdt_size;
>>
>>
> 

Re: [Qemu-devel] [PATCH] ppc: Fix sam460ex devicetree when booting the Linux kernel

[BALATON Zoltan]

On Fri, 22 Jun 2018, Guenter Roeck wrote:
> On 06/22/2018 12:52 AM, BALATON Zoltan wrote:
>> On Fri, 22 Jun 2018, David Gibson wrote:
>>> On Thu, Jun 21, 2018 at 09:47:51PM -0700, Guenter Roeck wrote:
>>>> sam4660ex (or at least this emulation) does not support the "ibm,cpm" 
>>>> power
>>>> management. As a result, Linux crashes when trying to access it. Remove
>>>> its devicetree node. Also, if/when we boot the Linux kernel directly,
>>>> u-boot will not fix up serial frequencies in the devicetree file, and
>>>> serial port initialization will fail. Add plausible frequency values to 
>>>> the
>>>> first serial port to be able to use it. Disable the second serial port
>>>> since it is not available on the board.? Also set valid values for the
>>>> other clock nodes otherwise set by u-boot.
>> 
>> Patching clock values when using -kernel instead of u-boot looks good. 
>> Although I've tested booting a Linux kernel and could see serial output 
>> while the kernel boots, it only went silent after started user space. Is 
>> this the same you see or you don't get serial output (with loglevel set to 
>> some higher value) even during kernel boot? Which kernel and image do you 
>> test with?
>> 
> I test with all stable kernel releases starting with 3.16.
>
> Correct, this is exactly what I was seeing as well. I also noticed that the
> system keeps running, only there is no serial output.
>
>> Does leaving second UART in device tree cause any problems? The fdt in 
>> kernel has these and I'm not sure u-boot would patch this out. Do we need 
>> to remove this? This did not seem to cause any problem with guests I've 
>> tried so far. Does real hardware have a different fdt than the stock kernel 
>> one (which is also what's downloadable from ACube's site).
>> 
> Only that the port fails to initialize without clock. We could leave it 
> in the tree, but it would only have value if the clocks are actually 
> set, and then it would be a qemu-only value (presumably; I read 
> somewhere that the board only has a single serial port). I don't have a 
> strong opinion either way. Also, I don't know what u-boot does.

I think you probably read the comment in qemu/hw/ppc/sam460ex.c:526 which 
mentions hardware should have 4 serial ports but board has only one and 
two are in device tree therefore only these two are emulated. Not sure 
what's the right way here so unless this causes any problems I'd leave it 
as it is now and not patch the device tree for this because that would 
make it different from what you get when using u-boot so unless u-boot 
also removes this node I think -kernel should behave the same and leave 
it there.

>> The version of the Linux kernel I've tried (which is from the Linux CD on 
>> ACube's site) did not try to access the power management register, neither 
>> any guest OSes I've tested with. Looks like it may be specific to the 
>> kernel config you're using.
>> 
> Interesting. This is with the standard upstream kernel, using 
> canyonlands_defconfig.
> The code seems to have been in the upstream kernel forever.

Maybe the Sam460ex specific image has some patches not in upstream kernel.

>> By the way, when I've tried with a more recent Linux kernel (4.15.10) I've 
>> noticed that the sm501 driver seemed like having endianness problems and 
>> thus did not find the chip, while it works with other older kernels made 
>> for sam460ex. I did not try to debug or bisect this yet. Do you know 
>> anything about that?
>> 
>
> No, I had not noticed. SM501 is disabled in the latest canyonlands_defconfig,
> and I only use the serial console for my testing. It fails as far back as 
> 3.18.y,
> so I am not sure if this is a Linux or a qemu problem, or if it is a problem 
> that
> was never fixed in the upstream kernel. What kernels did you try ?

I've tried the kernel from this iso:

http://www.acube-systems.biz/index.php?page=news&id=106

(the image does not boot with u-boot because it seems to hit a bug in the 
firmware so could only test it with directly booting kernel with -kernel 
option).

I've also tried a stock upstream 4.15.10 kernel which is where I've seen 
it reading the PCI IDs of the SM501 endian swapped and failed to find 
device because of that. There are also several other kernels and images 
here:

http://www.supertuxkart-amiga.de/amiga/sam.html#downloads

That I haven't tried. The images start to boot but I could not get to boot 
one fully.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] ppc: Fix sam460ex devicetree when booting the Linux kernel

[Guenter Roeck]

On Fri, Jun 22, 2018 at 11:37:30PM +0200, BALATON Zoltan wrote:
> 
> >>The version of the Linux kernel I've tried (which is from the Linux CD
> >>on ACube's site) did not try to access the power management register,
> >>neither any guest OSes I've tested with. Looks like it may be specific
> >>to the kernel config you're using.
> >>
> >Interesting. This is with the standard upstream kernel, using
> >canyonlands_defconfig.
> >The code seems to have been in the upstream kernel forever.
> 
> Maybe the Sam460ex specific image has some patches not in upstream kernel.
> 

Yes, it does. To start with, the DesignWare sata driver is different.

This is not the reason for the problem, though. canyonlands_defconfig
in the upstream kernel has CONFIG_SUSPEND enabled, but the configuration
used by aCube doesn't. The kernel code accessing the power management
registers is only enabled with CONFIG_SUSPEND.

> >>By the way, when I've tried with a more recent Linux kernel (4.15.10)
> >>I've noticed that the sm501 driver seemed like having endianness
> >>problems and thus did not find the chip, while it works with other older
> >>kernels made for sam460ex. I did not try to debug or bisect this yet. Do
> >>you know anything about that?
> >>
> >
> >No, I had not noticed. SM501 is disabled in the latest canyonlands_defconfig,
> >and I only use the serial console for my testing. It fails as far back as
> >3.18.y,
> >so I am not sure if this is a Linux or a qemu problem, or if it is a
> >problem that
> >was never fixed in the upstream kernel. What kernels did you try ?
> 

The problem is this (from the kernel diffs provided by aCube):

 #if defined(CONFIG_PPC32)
-#define smc501_readl(addr)             ioread32be((addr))
-#define smc501_writel(val, addr)       iowrite32be((val), (addr))
+#define smc501_readl(addr)             ioread32((addr))
+#define smc501_writel(val, addr)       iowrite32((val), (addr))
 #else
 #define smc501_readl(addr)             readl(addr)
 #define smc501_writel(val, addr)       writel(val, addr)

This is a bit fishy since the cpu is big endian and iowrite32be()
should be identical to iowrite32(), but apparently that is not the
case here. I don't think I'll have time to track this down, though.

Guenter

Re: [Qemu-devel] [PATCH] ppc: Fix sam460ex devicetree when booting the Linux kernel

[BALATON Zoltan]

On Fri, 22 Jun 2018, Guenter Roeck wrote:
> The problem is this (from the kernel diffs provided by aCube):
>
> #if defined(CONFIG_PPC32)
> -#define smc501_readl(addr)             ioread32be((addr))
> -#define smc501_writel(val, addr)       iowrite32be((val), (addr))
> +#define smc501_readl(addr)             ioread32((addr))
> +#define smc501_writel(val, addr)       iowrite32((val), (addr))
> #else
> #define smc501_readl(addr)             readl(addr)
> #define smc501_writel(val, addr)       writel(val, addr)
>
> This is a bit fishy since the cpu is big endian and iowrite32be()
> should be identical to iowrite32(), but apparently that is not the
> case here. I don't think I'll have time to track this down, though.

Thanks for finding this, it helps even if you don't have time to track it 
down completely. Could this be related to using little endian PCI device 
on a big endian CPU? Not sure which implementation will this use but in 
arch/powerpc/kernel/iomap.c:

unsigned int ioread32(void __iomem *addr)
{
         return readl(addr);
}
unsigned int ioread32be(void __iomem *addr)
{
         return readl_be(addr);
}

so they don't look identical.

Re: [Qemu-devel] [PATCH] ppc: Fix sam460ex devicetree when booting the Linux kernel

[Guenter Roeck]

On 06/23/2018 01:55 PM, BALATON Zoltan wrote:
> On Fri, 22 Jun 2018, Guenter Roeck wrote:
>> The problem is this (from the kernel diffs provided by aCube):
>>
>> #if defined(CONFIG_PPC32)
>> -#define smc501_readl(addr)             ioread32be((addr))
>> -#define smc501_writel(val, addr)       iowrite32be((val), (addr))
>> +#define smc501_readl(addr)             ioread32((addr))
>> +#define smc501_writel(val, addr)       iowrite32((val), (addr))
>> #else
>> #define smc501_readl(addr)             readl(addr)
>> #define smc501_writel(val, addr)       writel(val, addr)
>>
>> This is a bit fishy since the cpu is big endian and iowrite32be()
>> should be identical to iowrite32(), but apparently that is not the
>> case here. I don't think I'll have time to track this down, though.
> 
> Thanks for finding this, it helps even if you don't have time to track it down completely. Could this be related to using little endian PCI device on a big endian CPU? Not sure which implementation will this use but in arch/powerpc/kernel/iomap.c:
> 
> unsigned int ioread32(void __iomem *addr)
> {
>          return readl(addr);
> }
> unsigned int ioread32be(void __iomem *addr)
> {
>          return readl_be(addr);
> }
> 
> so they don't look identical.
>

Sure, but normally I would assume that readl_be() matches readl() on a big endian system.
This is not the case here - it specifically maps to an assembler instruction which
afaics always swaps the byte order. Other architectures typically have something like

#define ioread32be(p) be32_to_cpu(ioread32(p))

or similar. FWIW, using ioread32() instead of ioread32be() does improve the situation:

sm501 0002:00:06.0: enabling device (0000 -> 0002)
sm501 0002:00:06.0: SM501 At (ptrval): Version 050100a0, 64 Mb, IRQ 0
sm501 0002:00:06.0: setting M1XCLK to 144000000
sm501 0002:00:06.0: setting MCLK to 72000000
sm501-usb[0] [mem 0xd84040000-0xd8405ffff]
sm501-usb[1] [mem 0xd83fc0000-0xd83ffffff]
sm501-usb[2] [irq 0]
sm501-fb[0] [mem 0xd84080000-0xd8408ffff]
sm501-fb[1] [mem 0xd84100000-0xd8414ffff]
sm501-fb[2] [mem 0xd80000000-0xd83fbffff]
sm501-fb[3] [irq 0]

even though irq 0 looks fishy. This may be related to:

qemu-system-ppc: ppc440_pcix_set_irq: PCI irq -1

seen when starting qemu.

Also:

sm501-usb sm501-usb.80: cannot declare coherent memory
sm501-usb: probe of sm501-usb.80 failed with error -38

This is as far as I got. Fixing this may require a new configuration option -
something like CONFIG_MFD_SM501_NATIVE_ENDIAN which would be set to false
by default. I think this should be tested on real HW, though, not only on
sam460ex but also on some other PPC32 system with SM501. Unfortunately
I have neither the hardware nor the time to do all the testing.

Guenter

Re: [Qemu-devel] [PATCH] ppc: Fix sam460ex devicetree when booting the Linux kernel

[BALATON Zoltan]

On Sat, 23 Jun 2018, Guenter Roeck wrote:
> On 06/23/2018 01:55 PM, BALATON Zoltan wrote:
>> On Fri, 22 Jun 2018, Guenter Roeck wrote:
>>> The problem is this (from the kernel diffs provided by aCube):
>>> 
>>> #if defined(CONFIG_PPC32)
>>> -#define smc501_readl(addr)???????????? ioread32be((addr))
>>> -#define smc501_writel(val, addr)?????? iowrite32be((val), (addr))
>>> +#define smc501_readl(addr)???????????? ioread32((addr))
>>> +#define smc501_writel(val, addr)?????? iowrite32((val), (addr))
>>> #else
>>> #define smc501_readl(addr)???????????? readl(addr)
>>> #define smc501_writel(val, addr)?????? writel(val, addr)
>>> 
>>> This is a bit fishy since the cpu is big endian and iowrite32be()
>>> should be identical to iowrite32(), but apparently that is not the
>>> case here. I don't think I'll have time to track this down, though.
>> 
>> Thanks for finding this, it helps even if you don't have time to track it 
>> down completely. Could this be related to using little endian PCI device on 
>> a big endian CPU? Not sure which implementation will this use but in 
>> arch/powerpc/kernel/iomap.c:
>> 
>> unsigned int ioread32(void __iomem *addr)
>> {
>>  ??????? return readl(addr);
>> }
>> unsigned int ioread32be(void __iomem *addr)
>> {
>>  ??????? return readl_be(addr);
>> }
>> 
>> so they don't look identical.
>> 
>
> Sure, but normally I would assume that readl_be() matches readl() on a big 
> endian system.
> This is not the case here - it specifically maps to an assembler instruction 
> which
> afaics always swaps the byte order. Other architectures typically have 
> something like
>
> #define ioread32be(p) be32_to_cpu(ioread32(p))
>
> or similar. FWIW, using ioread32() instead of ioread32be() does improve the 
> situation:
>
> sm501 0002:00:06.0: enabling device (0000 -> 0002)
> sm501 0002:00:06.0: SM501 At (ptrval): Version 050100a0, 64 Mb, IRQ 0
> sm501 0002:00:06.0: setting M1XCLK to 144000000
> sm501 0002:00:06.0: setting MCLK to 72000000
> sm501-usb[0] [mem 0xd84040000-0xd8405ffff]
> sm501-usb[1] [mem 0xd83fc0000-0xd83ffffff]
> sm501-usb[2] [irq 0]
> sm501-fb[0] [mem 0xd84080000-0xd8408ffff]
> sm501-fb[1] [mem 0xd84100000-0xd8414ffff]
> sm501-fb[2] [mem 0xd80000000-0xd83fbffff]
> sm501-fb[3] [irq 0]
>
> even though irq 0 looks fishy. This may be related to:
>
> qemu-system-ppc: ppc440_pcix_set_irq: PCI irq -1
>
> seen when starting qemu.

I think sm501 irq is not emulated currently so it's not expected to work 
anyway.

> Also:
>
> sm501-usb sm501-usb.80: cannot declare coherent memory

I think I've seen this in logs from real hardware too but not sure what it 
is but may not be emulation related.

> sm501-usb: probe of sm501-usb.80 failed with error -38

USB part is only emulated on the sysbus version used on SH, the PCI 
version does not create it because AFAIK it's unused on sam460ex so this 
is expected too. Sam460ex has other USB ports in SoC which is emulated.

> This is as far as I got. Fixing this may require a new configuration option -
> something like CONFIG_MFD_SM501_NATIVE_ENDIAN which would be set to false
> by default. I think this should be tested on real HW, though, not only on
> sam460ex but also on some other PPC32 system with SM501. Unfortunately
> I have neither the hardware nor the time to do all the testing.

I don't have real hardware either nor know about any other system that 
have SM501/SM502 besides sam460ex so I only hope this conversation in the 
list archives might help someone in the future who has a way to test it 
and submit a patch for Linux to fix it.

Regards,
BALATON Zoltan