Re: [PATCH v3 1/3] arm64: dts: add initial dts for Samsung GH7 SoC and SSDK-GH7 board
Hi,
On Mon, Mar 17, 2014 at 10:14:35PM +, Kukjin Kim wrote:
Signed-off-by: Kukjin Kim kgene@samsung.com
Reviewed-by: Thomas Abraham thomas...@samsung.com
Cc: Catalin Marinas catalin.mari...@arm.com
---
arch/arm64/boot/dts/samsung-gh7.dtsi | 134
+++
arch/arm64/boot/dts/samsung-ssdk-gh7.dts | 26 ++
2 files changed, 160 insertions(+)
create mode 100644 arch/arm64/boot/dts/samsung-gh7.dtsi
create mode 100644 arch/arm64/boot/dts/samsung-ssdk-gh7.dts
diff --git a/arch/arm64/boot/dts/samsung-gh7.dtsi
b/arch/arm64/boot/dts/samsung-gh7.dtsi
new file mode 100644
index 000..d3ab914
--- /dev/null
+++ b/arch/arm64/boot/dts/samsung-gh7.dtsi
@@ -0,0 +1,134 @@
+/*
+ * SAMSUNG GH7 SoC device tree source
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * http://www.samsung.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.
+*/
+
+/memreserve/ 0xFEC0 0x140; /* EL3 monitor, secure intepreter */
As I've mentioned, I'm concerned that this is even in the non-secure
address space that the kernel can access. Why is this not hidden from
the kernel entirely? Why is it expected to be mapped in and reserved?
Additionally, the memory the used by the spin-table (0x0 0x8000fff8) has
not been reserved, and thus the kernel is free to clobber it.
[...]
+ gic: interrupt-controller@1C00 {
+ compatible = arm,cortex-a15-gic;
For targeting any future workarounds I would very much prefer a more
specific string.
[...]
+ pmu {
+ compatible = samsung,gh7-pmu, armv8-pmuv3;
+ interrupts = 0 294 0,
+ 0 295 0,
+ 0 296 0,
+ 0 297 0,
+ 0 298 0,
+ 0 299 0,
+ 0 300 0,
+ 0 301 0;
+ };
These are all missing a trigger type (thus making them unusable), and as
GH7 is the SoC name rather than the CPU name, the compatible string is
somewhat bad.
+
+ amba {
+ compatible = arm,amba-bus;
+ #address-cells = 2;
+ #size-cells = 2;
+ ranges;
+
+ serial@12c0 {
+ compatible = arm,pl011, arm,primecell;
+ reg = 0 0x12c0 0 0x1;
+ interrupts = 0 418 0;
+ };
+
+ serial@12c2 {
+ compatible = arm,pl011, arm,primecell;
+ reg = 0 0x12c2 0 0x1;
+ interrupts = 0 420 0;
+ };
While the primecell bindings and PL011 bindings state that clocks are
optional, the primecell bus code requires a clock named apb_pclk, and
the pl011 driver requires a clock (which it expects to be UARTCLK) to
acquire the frequency from. As neither are provided I do not see how
this DT could possibly be used to boot a usable system.
Additionally the interrupt trigger types are missing.
Given that these are the only IO devices described in the dtsi/dts
combination, and they do not appear to be usable, what is the point in
merging this?
Cheers,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 1/3] arm64: dts: add initial dts for Samsung GH7 SoC and SSDK-GH7 board
Mark Rutland wrote:
Hi,
Hi Mark,
[...]
+/memreserve/ 0xFEC0 0x140; /* EL3 monitor, secure intepreter */
As I've mentioned, I'm concerned that this is even in the non-secure
address space that the kernel can access. Why is this not hidden from
the kernel entirely? Why is it expected to be mapped in and reserved?
OK, I will make kernel cannot access the memory area with hiding.
Additionally, the memory the used by the spin-table (0x0 0x8000fff8) has
not been reserved, and thus the kernel is free to clobber it.
Oops, I missed. OK I will add following instead of above.
+/memreserve/ 0x8000 0x0001;
[...]
+ gic: interrupt-controller@1C00 {
+ compatible = arm,cortex-a15-gic;
For targeting any future workarounds I would very much prefer a more
specific string.
If any workarounds are required later, will add specific string then.
[...]
+ pmu {
+ compatible = samsung,gh7-pmu, armv8-pmuv3;
+ interrupts = 0 294 0,
+0 295 0,
+0 296 0,
+0 297 0,
+0 298 0,
+0 299 0,
+0 300 0,
+0 301 0;
+ };
These are all missing a trigger type (thus making them unusable), and as
GH7 is the SoC name rather than the CPU name, the compatible string is
somewhat bad.
Oops, it should be 8.
And yes, as I've mentioned GH7 is SoC name not CPU name.
I'm still thinking _really_ I need to use CPU specific name for GH7 SoC because
we don't need to handle for the specific CPU implementation in kernel even we
didn't name it.
+
+ amba {
+ compatible = arm,amba-bus;
+ #address-cells = 2;
+ #size-cells = 2;
+ ranges;
+
+ serial@12c0 {
+ compatible = arm,pl011, arm,primecell;
+ reg = 0 0x12c0 0 0x1;
+ interrupts = 0 418 0;
+ };
+
+ serial@12c2 {
+ compatible = arm,pl011, arm,primecell;
+ reg = 0 0x12c2 0 0x1;
+ interrupts = 0 420 0;
+ };
While the primecell bindings and PL011 bindings state that clocks are
optional, the primecell bus code requires a clock named apb_pclk, and
the pl011 driver requires a clock (which it expects to be UARTCLK) to
acquire the frequency from. As neither are provided I do not see how
this DT could possibly be used to boot a usable system.
Additionally the interrupt trigger types are missing.
Given that these are the only IO devices described in the dtsi/dts
combination, and they do not appear to be usable, what is the point in
merging this?
Definitely, it is meaningful because we can enhance everything more based on
this for the mass product.
Thanks for your time.
- Kukjin
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
