Re: [PATCH v2 3/4] arm64: dts: Add mediatek MT8173 SoC and evaluation board dts and Makefile

Re: [PATCH v2 3/4] arm64: dts: Add mediatek MT8173 SoC and evaluation board dts and Makefile

[Mark Rutland]

On Wed, Dec 17, 2014 at 02:17:32PM +0000, Eddie Huang wrote:
> Add device tree support for MT8173 SoC and evaluation board based on it.
> 
> Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> ---
>  arch/arm64/boot/dts/Makefile       |   1 +
>  arch/arm64/boot/dts/mt8173-evb.dts |  34 +++++++++
>  arch/arm64/boot/dts/mt8173.dtsi    | 152 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 187 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/mt8173-evb.dts
>  create mode 100644 arch/arm64/boot/dts/mt8173.dtsi
> 
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index f8001a6..db7661e 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -1,3 +1,4 @@
> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8173-evb.dtb
>  dtb-$(CONFIG_ARCH_THUNDER) += thunder-88xx.dtb
>  dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb foundation-v8.dtb
>  dtb-$(CONFIG_ARCH_XGENE) += apm-mustang.dtb
> diff --git a/arch/arm64/boot/dts/mt8173-evb.dts b/arch/arm64/boot/dts/mt8173-evb.dts
> new file mode 100644
> index 0000000..b8b2621
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mt8173-evb.dts
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: Eddie Huang <eddie.huang@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +/dts-v1/;
> +#include "mt8173.dtsi"
> +
> +/ {
> +	model = "mediatek,mt8173-evb";
> +
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;
> +		serial3 = &uart3;
> +	};
> +
> +	memory@40000000 {
> +		device_type = "memory";
> +		reg = <0 0x40000000 0 0x80000000>;
> +	};
> +
> +	chosen { };
> +};
> diff --git a/arch/arm64/boot/dts/mt8173.dtsi b/arch/arm64/boot/dts/mt8173.dtsi
> new file mode 100644
> index 0000000..41c1441
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mt8173.dtsi
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: Eddie Huang <eddie.huang@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +/ {
> +	compatible = "mediatek,mt8173";
> +	interrupt-parent = <&sysirq>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu-map {
> +			cluster0 {
> +				core0 {
> +					cpu = <&cpu0>;
> +				};
> +				core1 {
> +					cpu = <&cpu1>;
> +				};
> +			};
> +
> +			cluster1 {
> +				core0 {
> +					cpu = <&cpu2>;
> +				};
> +				core1 {
> +					cpu = <&cpu3>;
> +				};
> +			};
> +		};
> +
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			reg = <0x000>;
> +		};
> +
> +		cpu1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			reg = <0x001>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu2: cpu@2 {

The unit-address should be 100 rather than 2, matching the reg property.
This should be 'cpu@100'.

> +			device_type = "cpu";
> +			compatible = "arm,cortex-a57";
> +			reg = <0x100>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu3: cpu@3 {

Similarly this should be 'cpu@101'.

> +			device_type = "cpu";
> +			compatible = "arm,cortex-a57";
> +			reg = <0x101>;
> +			enable-method = "psci";
> +		};
> +	};
> +
> +	psci {
> +		compatible = "arm,psci";
> +		method = "smc";
> +		cpu_suspend   = <0x84000001>;
> +		cpu_off	      = <0x84000002>;
> +		cpu_on	      = <0x84000003>;
> +		affinity_info = <0x84000004>;
> +	};

There is no AFFINITY_INFO function prior to PSCI 0.2, and
'affinity_info' does not exist in the "arm,psci" binding.

I take it hotplug has been tested for all but CPU0?

> +
> +	uart_clk: dummy26m {
> +		compatible = "fixed-clock";
> +		clock-frequency = <26000000>;
> +		#clock-cells = <0>;
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupt-parent = <&gic>;
> +		interrupts = <1 13 0xf08>,
> +			     <1 14 0xf08>,
> +			     <1 11 0xf08>,
> +			     <1 10 0xf08>;
> +	};
> +
> +	soc {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		compatible = "simple-bus";
> +		ranges;
> +
> +		sysirq: intpol-controller@10200620 {
> +			compatible = "mediatek,mt8173-sysirq", "mediatek,mt6577-sysirq";
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			reg = <0 0x10200620 0 0x20>;
> +		};
> +
> +		gic: interrupt-controller@10220000 {
> +			compatible = "arm,gic-400";
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupt-controller;
> +			reg = <0 0x10221000 0 0x1000>,
> +			      <0 0x10222000 0 0x1000>,
> +			      <0 0x10224000 0 0x2000>,
> +			      <0 0x10226000 0 0x2000>;
> +			interrupts = <1 9 0xf04>;
> +		};

I don't think these reg entries are quite right; GICC should be 8k long.

Marc?

Thanks,
Mark.

Re: [PATCH v2 0/4] Add basic support for Mediatek MT8173 SoC

[Arnd Bergmann]

On Wednesday 17 December 2014 22:17:29 Eddie Huang wrote:
> MT8173 is a SoC based on 64bit ARMv8 architecture. It contains 2 CA53 + 2 CA57 cores. MT8173 share many HW IP with MT8135 and other MT65xx series. This patchset was tested on MT8173 engineer sample, and boot to shell ok.
> 
> This series contains document bindings, device tree, defconfig, and one sysirq modification for MT8173.
> 
> Changes in v2:
> 
> 1. Modify mt8173.dtsi and mt8173-evb.dts according to Mark Rutland's comment, change cpu-maps, psci, clock, GIC properties.
> 2. Modify irq-mtk-sysirq.c according to Arnd Bergmann's comment to calculate irq number from reg range.
> 
> 

Looks good to me,

Acked-by: Arnd Bergmann <arnd@arndb.de>

In the 3.19 merge window, we had a few problems with new arm64 platforms,
as it wasn't entirely clear to us who is a maintainer and whether patches
were sent for review or inclusion. I see no problem including your series
in 3.20, but we probably need to figure out the best process for making
sure we don't miss it.

For now, I'd suggest we do this the same way as the arm32 patches, and
you send them to Matthias for inclusion in his tree, who will then
send a pull request for an arm64 branch.

	Arnd

Re: [PATCH v2 3/4] arm64: dts: Add mediatek MT8173 SoC and evaluation board dts and Makefile

[Marc Zyngier]

On 17/12/14 14:33, Mark Rutland wrote:
> On Wed, Dec 17, 2014 at 02:17:32PM +0000, Eddie Huang wrote:
>> Add device tree support for MT8173 SoC and evaluation board based on it.
>>
>> Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
>> ---
>>  arch/arm64/boot/dts/Makefile       |   1 +
>>  arch/arm64/boot/dts/mt8173-evb.dts |  34 +++++++++
>>  arch/arm64/boot/dts/mt8173.dtsi    | 152 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 187 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/mt8173-evb.dts
>>  create mode 100644 arch/arm64/boot/dts/mt8173.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
>> index f8001a6..db7661e 100644
>> --- a/arch/arm64/boot/dts/Makefile
>> +++ b/arch/arm64/boot/dts/Makefile
>> @@ -1,3 +1,4 @@
>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8173-evb.dtb
>>  dtb-$(CONFIG_ARCH_THUNDER) += thunder-88xx.dtb
>>  dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb foundation-v8.dtb
>>  dtb-$(CONFIG_ARCH_XGENE) += apm-mustang.dtb
>> diff --git a/arch/arm64/boot/dts/mt8173-evb.dts b/arch/arm64/boot/dts/mt8173-evb.dts
>> new file mode 100644
>> index 0000000..b8b2621
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/mt8173-evb.dts
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Copyright (c) 2014 MediaTek Inc.
>> + * Author: Eddie Huang <eddie.huang@mediatek.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +/dts-v1/;
>> +#include "mt8173.dtsi"
>> +
>> +/ {
>> +	model = "mediatek,mt8173-evb";
>> +
>> +	aliases {
>> +		serial0 = &uart0;
>> +		serial1 = &uart1;
>> +		serial2 = &uart2;
>> +		serial3 = &uart3;
>> +	};
>> +
>> +	memory@40000000 {
>> +		device_type = "memory";
>> +		reg = <0 0x40000000 0 0x80000000>;
>> +	};
>> +
>> +	chosen { };
>> +};
>> diff --git a/arch/arm64/boot/dts/mt8173.dtsi b/arch/arm64/boot/dts/mt8173.dtsi
>> new file mode 100644
>> index 0000000..41c1441
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/mt8173.dtsi
>> @@ -0,0 +1,152 @@
>> +/*
>> + * Copyright (c) 2014 MediaTek Inc.
>> + * Author: Eddie Huang <eddie.huang@mediatek.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +/ {
>> +	compatible = "mediatek,mt8173";
>> +	interrupt-parent = <&sysirq>;
>> +	#address-cells = <2>;
>> +	#size-cells = <2>;
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		cpu-map {
>> +			cluster0 {
>> +				core0 {
>> +					cpu = <&cpu0>;
>> +				};
>> +				core1 {
>> +					cpu = <&cpu1>;
>> +				};
>> +			};
>> +
>> +			cluster1 {
>> +				core0 {
>> +					cpu = <&cpu2>;
>> +				};
>> +				core1 {
>> +					cpu = <&cpu3>;
>> +				};
>> +			};
>> +		};
>> +
>> +		cpu0: cpu@0 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a53";
>> +			reg = <0x000>;
>> +		};
>> +
>> +		cpu1: cpu@1 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a53";
>> +			reg = <0x001>;
>> +			enable-method = "psci";
>> +		};
>> +
>> +		cpu2: cpu@2 {
> 
> The unit-address should be 100 rather than 2, matching the reg property.
> This should be 'cpu@100'.
> 
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a57";
>> +			reg = <0x100>;
>> +			enable-method = "psci";
>> +		};
>> +
>> +		cpu3: cpu@3 {
> 
> Similarly this should be 'cpu@101'.
> 
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a57";
>> +			reg = <0x101>;
>> +			enable-method = "psci";
>> +		};
>> +	};
>> +
>> +	psci {
>> +		compatible = "arm,psci";
>> +		method = "smc";
>> +		cpu_suspend   = <0x84000001>;
>> +		cpu_off	      = <0x84000002>;
>> +		cpu_on	      = <0x84000003>;
>> +		affinity_info = <0x84000004>;
>> +	};
> 
> There is no AFFINITY_INFO function prior to PSCI 0.2, and
> 'affinity_info' does not exist in the "arm,psci" binding.
> 
> I take it hotplug has been tested for all but CPU0?
> 
>> +
>> +	uart_clk: dummy26m {
>> +		compatible = "fixed-clock";
>> +		clock-frequency = <26000000>;
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	timer {
>> +		compatible = "arm,armv8-timer";
>> +		interrupt-parent = <&gic>;
>> +		interrupts = <1 13 0xf08>,
>> +			     <1 14 0xf08>,
>> +			     <1 11 0xf08>,
>> +			     <1 10 0xf08>;
>> +	};
>> +
>> +	soc {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		compatible = "simple-bus";
>> +		ranges;
>> +
>> +		sysirq: intpol-controller@10200620 {
>> +			compatible = "mediatek,mt8173-sysirq", "mediatek,mt6577-sysirq";
>> +			interrupt-controller;
>> +			#interrupt-cells = <3>;
>> +			interrupt-parent = <&gic>;
>> +			reg = <0 0x10200620 0 0x20>;
>> +		};
>> +
>> +		gic: interrupt-controller@10220000 {
>> +			compatible = "arm,gic-400";
>> +			#interrupt-cells = <3>;
>> +			interrupt-parent = <&gic>;
>> +			interrupt-controller;
>> +			reg = <0 0x10221000 0 0x1000>,
>> +			      <0 0x10222000 0 0x1000>,
>> +			      <0 0x10224000 0 0x2000>,
>> +			      <0 0x10226000 0 0x2000>;
>> +			interrupts = <1 9 0xf04>;
>> +		};
> 
> I don't think these reg entries are quite right; GICC should be 8k long.
> 
> Marc?

Indeed, as described in the documentation:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0471b/CHDIFAEE.html

Also it is worth noticing that given how GICV is placed, it will never
work with 64K pages and virtualization. Pretty sad.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 3/4] arm64: dts: Add mediatek MT8173 SoC and evaluation board dts and Makefile

[Arnd Bergmann]

On Wednesday 17 December 2014 15:01:29 Marc Zyngier wrote:
> 
> Indeed, as described in the documentation:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0471b/CHDIFAEE.html
> 
> Also it is worth noticing that given how GICV is placed, it will never
> work with 64K pages and virtualization. Pretty sad.

Does this mean no VGIC support on this platform so you have to emulate it
in order to run virtual machines with 64K pages, or does it mean that
it's impossible to use that way because you can't emulate it?

	Arnd

Re: [PATCH v2 3/4] arm64: dts: Add mediatek MT8173 SoC and evaluation board dts and Makefile

[Peter Maydell]

On 20 December 2014 at 20:07, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 17 December 2014 15:01:29 Marc Zyngier wrote:
>> Also it is worth noticing that given how GICV is placed, it will never
>> work with 64K pages and virtualization. Pretty sad.
>
> Does this mean no VGIC support on this platform so you have to emulate it
> in order to run virtual machines with 64K pages, or does it mean that
> it's impossible to use that way because you can't emulate it?

Currently having the guest use the generic timer requires an
in-kernel GIC, because we don't provide an ABI for having the
kernel say "here is a generic timer interrupt for the userspace
emulated GIC". And at least for QEMU userspace the "virt" board
doesn't provide any other kind of timer, so you'd need to add one
if you wanted to use a userspace GIC. So for practical purposes
"out-of-kernel GIC" is not a supported config for KVM+QEMU.

-- PMM

Re: [PATCH v2 3/4] arm64: dts: Add mediatek MT8173 SoC and evaluation board dts and Makefile

[Marc Zyngier]

On 20/12/14 20:07, Arnd Bergmann wrote:
> On Wednesday 17 December 2014 15:01:29 Marc Zyngier wrote:
>>
>> Indeed, as described in the documentation:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0471b/CHDIFAEE.html
>>
>> Also it is worth noticing that given how GICV is placed, it will never
>> work with 64K pages and virtualization. Pretty sad.
> 
> Does this mean no VGIC support on this platform so you have to emulate it
> in order to run virtual machines with 64K pages, or does it mean that
> it's impossible to use that way because you can't emulate it?

As Peter said, this is not a configuration we're willing to support:
- we don't have a API to tell userspace emulation about interrupts
generated by the generic timers
- we could move the whole GIC emulation into the kernel (at the moment,
only the distributor is there), but that would be a complete nightmare

It really looks like a case of "let's drop a bunch of 64bit cores into
an existing SoC". Shame people can't read integration guidelines...

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 3/4] arm64: dts: Add mediatek MT8173 SoC and evaluation board dts and Makefile

[Marc Zyngier]

Hi Eddie,

On 2014-12-23 08:02, Eddie Huang wrote:
> Hi,
>
> On Mon, 2014-12-22 at 09:07 +0000, Marc Zyngier wrote:
>> On 20/12/14 20:07, Arnd Bergmann wrote:
>> > On Wednesday 17 December 2014 15:01:29 Marc Zyngier wrote:
>> >>
>> >> Indeed, as described in the documentation:
>> >> 
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0471b/CHDIFAEE.html
>> >>
>> >> Also it is worth noticing that given how GICV is placed, it will 
>> never
>> >> work with 64K pages and virtualization. Pretty sad.
>> >
>> > Does this mean no VGIC support on this platform so you have to 
>> emulate it
>> > in order to run virtual machines with 64K pages, or does it mean 
>> that
>> > it's impossible to use that way because you can't emulate it?
>>
>> As Peter said, this is not a configuration we're willing to support:
>> - we don't have a API to tell userspace emulation about interrupts
>> generated by the generic timers
>> - we could move the whole GIC emulation into the kernel (at the 
>> moment,
>> only the distributor is there), but that would be a complete 
>> nightmare
>>
>> It really looks like a case of "let's drop a bunch of 64bit cores 
>> into
>> an existing SoC". Shame people can't read integration guidelines...
>>
>> 	M.
>
> MT8173 use GIC-400. We check GIC-400 TRM that VGIC address is not 
> 64KB
> alignment. but GIC-500 (GICv3) VGIC base address is. We also check
> 3.19-rc1 arm64 device tree, amd-seattle-soc.dtsi VGIC is 64KB 
> alignment,
> but arm juno.dts is not, they are both GIC-400. So we are a little
> confused, and still try to figure out what is the correct address.

I'm afraid you're missing the point.

It is not a matter of alignment, but a matter of having put the GICV
region in the same 64K page as the rest of the GIC. Which means that
if the hypervisor (KVM or Xen) wants to map the GICV region inside the
guest, it will also be forced to give it access to GICC, GICD, and 
GICH.

Goodbye isolation, see your hypervisor going down at the first buggy 
guest,
not to mention all kind of other nice hacks...

I could understand the need to tightly pack everything on a 32bit 
system,
but doing so on a 64bit system without consideration of the page size
is a bit of a bummer.

> Neverthless, MT8173 SoC already use 0x10224000 and 0x10225000 as VGIC
> base address.

Then its fate is sealed virtualization wise.

Thanks,

         M.
-- 
Fast, cheap, reliable. Pick two.

Re: [PATCH v2 3/4] arm64: dts: Add mediatek MT8173 SoC and evaluation board dts and Makefile

[Arnd Bergmann]

On Tuesday 23 December 2014 08:43:55 Marc Zyngier wrote:
> 
> > Neverthless, MT8173 SoC already use 0x10224000 and 0x10225000 as VGIC
> > base address.
> 
> Then its fate is sealed virtualization wise.

It should still work with 4kb pages on the host, right? Would that still
allow you to have 64kb pages on the guest, or does the guest page size
have to be smaller or equal to the host?

	Arnd

Re: [PATCH v2 3/4] arm64: dts: Add mediatek MT8173 SoC and evaluation board dts and Makefile

[Marc Zyngier]

On 2014-12-23 21:07, Arnd Bergmann wrote:
> On Tuesday 23 December 2014 08:43:55 Marc Zyngier wrote:
>>
>> > Neverthless, MT8173 SoC already use 0x10224000 and 0x10225000 as 
>> VGIC
>> > base address.
>>
>> Then its fate is sealed virtualization wise.
>
> It should still work with 4kb pages on the host, right? Would that 
> still
> allow you to have 64kb pages on the guest, or does the guest page 
> size
> have to be smaller or equal to the host?

Of course, it will still work with 4kB pages, and the guest can still 
use
whatever page size it wants, including 64kB. Not very good from a 
performance
point of view, but it will work.

My main worry is that we're already starting to see more and more 
platforms
having divergent requirements in their MM setup, and distributions are 
going
to struggle to provide a full-featured single kernel image, specially 
as some
feel the need to push 64kB pages down to everybody's throat.

Thanks,

         M.
-- 
Fast, cheap, reliable. Pick two.