arm64,hi6220: Enable Hisilicon Hi6220 SoC

This patch adds documentation for the devicetree bindings used by the DT files of Hisilicon hi6220 SoC mobile platform.

Signed-off-by Bintian Wang <[email protected]>

Quoted 4 lines · Bintian Wang

This patch adds documentation for the devicetree bindings used by the DT files of Hisilicon hi6220 SoC mobile platform.

Signed-off-by Bintian Wang <[email protected]>

Acked-by Stephen Boyd <[email protected]>

Document DT files bindings for Hisilicon hi6220 clock.

Signed-off-by Bintian Wang <[email protected]>

Reviewed-by Haojian Zhuang <[email protected]>

drivers/clk/hisilicon/Kconfig new file +2 -0

+	help
+	  Build the Hisilicon Hi6220 clock driver based on the common clock framework.

drivers/clk/hisilicon/clk-hi6220.c new file +11 -0

new file mode 100644
index 0000000..91b1cd7
@@ -0,0 +1,292 @@
+/*
+ * Hisilicon Hi6220 clock driver
+ *
+ * Copyright (c) 2015 Hisilicon Limited.
+ *
+ * Author: Bintian Wang <[email protected]>
+ *
+ * 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.
+ */

drivers/clk/hisilicon/clk-hi6220.c new file +5 -0

+
+#include <dt-bindings/clock/hi6220-clock.h>
+
+#include "clk.h"
+

drivers/clk/hisilicon/clk.c +0 -0

index a078e84..5d2305c 100644

drivers/clk/hisilicon/clk.c +0 -0

 #endif	/* __HISI_CLK_H */

drivers/clk/hisilicon/clkdivider-hi6220.c +11 -0

+
+/**
+ * struct hi6220_clk_divider - divider clock for hi6220
+ *
+ * @hw:		handle between common and hardware-specific interfaces
+ * @reg:	register containing divider
+ * @shift:	shift to the divider bit field
+ * @width:	width of the divider bit field
+ * @mask:	mask for setting divider rate
+ * @table:	the div table that the divider supports
+ * @lock:	register lock

drivers/clk/hisilicon/clkdivider-hi6220.c +2 -0

+
+#define to_hi6220_clk_divider(_hw)	\

drivers/clk/hisilicon/clkdivider-hi6220.c +10 -0

+	.recalc_rate = hi6220_clkdiv_recalc_rate,
+	.round_rate = hi6220_clkdiv_round_rate,
+	.set_rate = hi6220_clkdiv_set_rate,
+};
+
+struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
+	const char *parent_name, unsigned long flags, void __iomem *reg,
+	u8 shift, u8 width, u32 mask_bit, spinlock_t *lock)
+{
+	struct hi6220_clk_divider *div;

drivers/clk/hisilicon/clkdivider-hi6220.c +2 -0

+	init.parent_names = parent_name ? &parent_name : NULL;
+	init.num_parents = parent_name ? 1 : 0;

Quoted 6 lines · Bintian Wang

drivers/clk/Kconfig +0 -0

index 9897f35..935c44b 100644
@@ -152,6 +152,8 @@ config COMMON_CLK_CDCE706
 

drivers/clk/Kconfig config COMMON_CLK_CDCE706 · line 156

Quoted 4 lines

drivers/clk/Kconfig +2 -0

  source "drivers/clk/qcom/Kconfig"
 
+source "drivers/clk/hisilicon/Kconfig"
+

Please move this above qcom to maintain alphabet sort.

Quoted 10 lines

drivers/clk/Kconfig +0 -0

  endmenu
 
  source "drivers/clk/bcm/Kconfig"

drivers/clk/hisilicon/Kconfig new file +0 -0

new file mode 100644
index 0000000..8034739

drivers/clk/hisilicon/Kconfig line 4

Quoted 4 lines

drivers/clk/hisilicon/Kconfig new file +4 -0

@@ -0,0 +1,6 @@
+config COMMON_CLK_HI6220
+	bool "Hi6220 Clock Driver"
+	depends on OF && ARCH_HISI
+	default y

Can this be

	depends on ARCH_HISI || COMPILE_TEST
	default ARCH_HISI

instead? I'd like to increase build coverage.

Quoted 25 lines

drivers/clk/hisilicon/Kconfig new file +2 -0

+	help
+	  Build the Hisilicon Hi6220 clock driver based on the common clock framework.

drivers/clk/hisilicon/clk-hi6220.c new file +16 -0

new file mode 100644
index 0000000..91b1cd7
@@ -0,0 +1,292 @@
+/*
+ * Hisilicon Hi6220 clock driver
+ *
+ * Copyright (c) 2015 Hisilicon Limited.
+ *
+ * Author: Bintian Wang <[email protected]>
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/io.h>

drivers/clk/hisilicon/clk-hi6220.c line 21

Quoted 5 lines

drivers/clk/hisilicon/clk-hi6220.c new file +5 -0

+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+#include <linux/clk.h>

Do we need to include linux/clk.h? I don't see any consumer usage here.

Quoted 10 lines

drivers/clk/hisilicon/clk-hi6220.c new file +5 -0

+
+#include <dt-bindings/clock/hi6220-clock.h>
+
+#include "clk.h"
+

drivers/clk/hisilicon/clk.c +0 -0

index a078e84..5d2305c 100644

drivers/clk/hisilicon/clk.c void __init hisi_clk_register_gate(struct hisi_gate_clock *, · line 127

Quoted 6 lines

drivers/clk/hisilicon/clk.c +2 -0

@@ -108,4 +123,6 @@ void __init hisi_clk_register_gate(struct hisi_gate_clock *,
  					int, struct hisi_clock_data *);
  void __init hisi_clk_register_gate_sep(struct hisi_gate_clock *,
  					int, struct hisi_clock_data *);
+void __init hi6220_clk_register_divider(struct hi6220_divider_clock *,
+					int, struct hisi_clock_data *);

__init markings on function prototypes are useless. Please remove them.

Quoted 21 lines

drivers/clk/hisilicon/clk.c +0 -0

  #endif	/* __HISI_CLK_H */

drivers/clk/hisilicon/clkdivider-hi6220.c +18 -0

+
+/**
+ * struct hi6220_clk_divider - divider clock for hi6220
+ *
+ * @hw:		handle between common and hardware-specific interfaces
+ * @reg:	register containing divider
+ * @shift:	shift to the divider bit field
+ * @width:	width of the divider bit field
+ * @mask:	mask for setting divider rate
+ * @table:	the div table that the divider supports
+ * @lock:	register lock
+ */
+struct hi6220_clk_divider {
+	struct clk_hw	hw;
+	void __iomem	*reg;
+	u8		shift;
+	u8		width;
+	u32		mask;

drivers/clk/hisilicon/clkdivider-hi6220.c line 47

Quoted 3 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +3 -0

+	const struct clk_div_table *table;
+	spinlock_t	*lock;
+};

The clk-divider.c code has been made "reusable". Can you please try to use the functions that it now exposes instead of copy/pasting it and modifying it to suit your needs? A lot of this code looks the same.

Quoted 3 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +2 -0

+
+#define to_hi6220_clk_divider(_hw)	\

[..]

drivers/clk/hisilicon/clkdivider-hi6220.c line 209

Quoted 2 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +2 -0

+
+static struct clk_ops hi6220_clkdiv_ops = {

const?

Quoted 17 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +16 -0

+	.recalc_rate = hi6220_clkdiv_recalc_rate,
+	.round_rate = hi6220_clkdiv_round_rate,
+	.set_rate = hi6220_clkdiv_set_rate,
+};
+
+struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
+	const char *parent_name, unsigned long flags, void __iomem *reg,
+	u8 shift, u8 width, u32 mask_bit, spinlock_t *lock)
+{
+	struct hi6220_clk_divider *div;
+	struct clk *clk;
+	struct clk_init_data init;
+	struct clk_div_table *table;
+	u32 max_div, min_div;
+	int i;
+

drivers/clk/hisilicon/clkdivider-hi6220.c line 229

Quoted 4 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +4 -0

+	/* allocate the divider */
+	div = kzalloc(sizeof(struct hi6220_clk_divider), GFP_KERNEL);
+	if (!div) {
+		pr_err("%s: could not allocate divider clk\n", __func__);

Useless error message on allocation failure. Please run checkpatch. I've sent a patch to remove these from basic clock types.

drivers/clk/hisilicon/clkdivider-hi6220.c line 240

Quoted 12 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +11 -0

+		return ERR_PTR(-ENOMEM);
+	}
+
+	/* Init the divider table */
+	max_div = div_mask(width) + 1;
+	min_div = 1;
+
+	table = kzalloc(sizeof(struct clk_div_table) * (max_div + 1), GFP_KERNEL);
+	if (!table) {
+		kfree(div);
+		pr_err("%s: failed to alloc divider table!\n", __func__);

ditto.

drivers/clk/hisilicon/clkdivider-hi6220.c line 251

Quoted 12 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +11 -0

+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (i = 0; i < max_div; i++) {
+		table[i].div = min_div + i;
+		table[i].val = table[i].div - 1;
+	}
+
+	init.name = name;
+	init.ops = &hi6220_clkdiv_ops;
+	init.flags = flags | CLK_IS_BASIC;

It's basic?

Quoted 3 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +2 -0

+	init.parent_names = parent_name ? &parent_name : NULL;
+	init.num_parents = parent_name ? 1 : 0;

Quoted 20 lines · Stephen Boyd

Quoted 19 lines · Bintian Wang

drivers/clk/hisilicon/clkdivider-hi6220.c +18 -0

+
+/**
+ * struct hi6220_clk_divider - divider clock for hi6220
+ *
+ * @hw:		handle between common and hardware-specific interfaces
+ * @reg:	register containing divider
+ * @shift:	shift to the divider bit field
+ * @width:	width of the divider bit field
+ * @mask:	mask for setting divider rate
+ * @table:	the div table that the divider supports
+ * @lock:	register lock
+ */
+struct hi6220_clk_divider {
+	struct clk_hw	hw;
+	void __iomem	*reg;
+	u8		shift;
+	u8		width;
+	u32		mask;

Quoted 8 lines

drivers/clk/hisilicon/clkdivider-hi6220.c line 47

Quoted 3 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +3 -0

+	const struct clk_div_table *table;
+	spinlock_t	*lock;
+};

The clk-divider.c code has been made "reusable". Can you please try to use the functions that it now exposes instead of copy/pasting it and modifying it to suit your needs? A lot of this code looks the same.

In fact, I discussed this problem with Rob Herring and Mike Turquette in the 96boards internal mail list before.

The divider in hi6220 has a mask bit to guarantee writing the correct bits in register when setting rate, but the index of this mask bit has no rules to get (e.g. by left shift some fixed bits), so I add this divider clock to handle it, we can regard hi6220_clk_divider as a special case of generic divider clock.

If I don't add this divider clock for hi6220 chip, then I should change the core APIs "clk_register_divider" and "clk_register_divider_table", and then many other drivers will be updated.
So I think just add this divider clock is a good solution now.

Quoted 5 lines

Quoted 5 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +5 -0

+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (i = 0; i < max_div; i++) {
+		table[i].div = min_div + i;

Quoted 8 lines

drivers/clk/hisilicon/clkdivider-hi6220.c line 251

Quoted 6 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +6 -0

+		table[i].val = table[i].div - 1;
+	}
+
+	init.name = name;
+	init.ops = &hi6220_clkdiv_ops;
+	init.flags = flags | CLK_IS_BASIC;

It's basic?

I rechecked this flag, it's really useless to us, so I can remove it. But can you tell me which case I should use it?

How about just send this patch for review not the whole patch set in next version?

Quoted 38 lines · Stephen Boyd

Quoted 20 lines · Bintian Wang

Quoted 19 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +18 -0

+
+/**
+ * struct hi6220_clk_divider - divider clock for hi6220
+ *
+ * @hw:            handle between common and hardware-specific interfaces
+ * @reg:   register containing divider
+ * @shift: shift to the divider bit field
+ * @width: width of the divider bit field
+ * @mask:  mask for setting divider rate
+ * @table: the div table that the divider supports
+ * @lock:  register lock
+ */
+struct hi6220_clk_divider {
+   struct clk_hw   hw;
+   void __iomem    *reg;
+   u8              shift;
+   u8              width;
+   u32             mask;

Quoted 8 lines

drivers/clk/hisilicon/clkdivider-hi6220.c line 47

Quoted 3 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +3 -0

+   const struct clk_div_table *table;
+   spinlock_t      *lock;
+};

The clk-divider.c code has been made "reusable". Can you please try to use the functions that it now exposes instead of copy/pasting it and modifying it to suit your needs? A lot of this code looks the same.

In fact, I discussed this problem with Rob Herring and Mike Turquette in the 96boards internal mail list before.

The divider in hi6220 has a mask bit to guarantee writing the correct bits in register when setting rate, but the index of this mask bit has no rules to get (e.g. by left shift some fixed bits), so I add this divider clock to handle it, we can regard hi6220_clk_divider as a special case of generic divider clock.

Quoted 4 lines

If I don't add this divider clock for hi6220 chip, then I should change the core APIs "clk_register_divider" and "clk_register_divider_table", and then many other drivers will be updated.
So I think just add this divider clock is a good solution now.

I think you missed my point. I didn't suggest using clk_register_divider or clk_register_divider_table(). I'm suggesting to use

unsigned long divider_recalc_rate(struct clk_hw *hw, unsigned long parent_rate,
unsigned int val, const struct clk_div_table *table, unsigned long flags);
long divider_round_rate(struct clk_hw *hw, unsigned long rate,
unsigned long *prate, const struct clk_div_table *table, u8 width, unsigned long flags);
int divider_get_val(unsigned long rate, unsigned long parent_rate,
const struct clk_div_table *table, u8 width, unsigned long flags);

Quoted 14 lines

Quoted 14 lines

drivers/clk/hisilicon/clkdivider-hi6220.c line 251

Quoted 12 lines

drivers/clk/hisilicon/clkdivider-hi6220.c +11 -0

+           return ERR_PTR(-ENOMEM);
+   }
+
+   for (i = 0; i < max_div; i++) {
+           table[i].div = min_div + i;
+           table[i].val = table[i].div - 1;
+   }
+
+   init.name = name;
+   init.ops = &hi6220_clkdiv_ops;
+   init.flags = flags | CLK_IS_BASIC;

It's basic?

Quoted 2 lines

I rechecked this flag, it's really useless to us, so I can remove it. But can you tell me which case I should use it?

I think the basic flag is there for drivers that want to know what type of clock they're dealing with when all they have is the struct clk_hw pointer. I like to discourage use of this flag in hopes of deleting it someday.

Quoted 1 line

Quoted 3 lines

How about just send this patch for review not the whole patch set in next version?

Yes a single patch is fine. I take it you want the patch to go through arm-soc with some Ack from us?

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 6 lines

Quoted 4 lines

How about just send this patch for review not the whole patch set in next version?

Yes a single patch is fine. I take it you want the patch to go through arm-soc with some Ack from us?

Yes, exactly.
The dts file includes the clock head file, this patch goes through arm-soc is a good choice.

Quoted 6 lines · Brent Wang

Quoted 6 lines

Quoted 4 lines

How about just send this patch for review not the whole patch set in next version?

Yes a single patch is fine. I take it you want the patch to go through arm-soc with some Ack from us?

Quoted 3 lines

Yes, exactly.
The dts file includes the clock head file, this patch goes through arm-soc is a good choice.

One way to avoid that problem is to split the clock header file into its own patch and then duplicate that patch in two trees, one that goes through arm-soc and one that goes through clk.

Hi,

arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts line 11

Quoted 2 lines

arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts new file +2 -0

+/*Reserved 1MB memory for MCU*/
+/memreserve/ 0x05e00000 0x00100000;

What exactly is the MCU used for, and what does it use this memory for?

Can this be carved out from the memory node instead? If the OS doesn't need to access this memory to communicate with the MCU, preventing the OS from mapping the memory avoids a number of potential issues.

I take it that with UEFI this region is not described to the OS?

[...]

arch/arm64/boot/dts/hisilicon/hi6220.dtsi line 19

Quoted 4 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +4 -0

+	psci {
+		compatible = "arm,psci-0.2";
+		method = "smc";
+	};

Are all the PSCI 0.2 mandatory features implemented?

Can CPU0 be hot unplugged?

[...]

arch/arm64/boot/dts/hisilicon/hi6220.dtsi line 170

Quoted 7 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +7 -0

+		uart0: uart at f8015000 {	/* console */
+			compatible = "arm,pl011", "arm,primecell";
+			reg = <0x0 0xf8015000 0x0 0x1000>;
+			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>;
+			clock-names = "uartclk", "apb_pclk";
+		};

In a previous discussion [1] the UART on HI6220 was described as not fully PL011 compliant, with a number of differences (e.g. the FIFO length).

Given that, I feel somewhat uncomfortable with the current compatible string list. What exactly are those differences? We may need a more specific compatible string (even if in addition to those existing ones), or perhaps other properties.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328978.html

.

Quoted 8 lines · Mark Rutland

[...]

arch/arm64/boot/dts/hisilicon/hi6220.dtsi line 19

Quoted 4 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +4 -0

+	psci {
+		compatible = "arm,psci-0.2";
+		method = "smc";
+	};

Are all the PSCI 0.2 mandatory features implemented?

The system off/suspend is under development, and system off will be released in next months, and system suspend may be released in the following two months.

So can I keep "arm,psci-0.2" here?

Quoted 1 line

Can CPU0 be hot unplugged?

Yes, CPU0~CPU7 all can be hot unplugged.

Quoted 10 lines · Bintian Wang

Quoted 8 lines · Mark Rutland

[...]

arch/arm64/boot/dts/hisilicon/hi6220.dtsi line 19

Quoted 4 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +4 -0

+	psci {
+		compatible = "arm,psci-0.2";
+		method = "smc";
+	};

Are all the PSCI 0.2 mandatory features implemented?

Quoted 3 lines

The system off/suspend is under development, and system off will be released in next months, and system suspend may be released in the following two months.

Yes; now for mandatory features, except SYSTEM_OFF others have supported.

for PSCI 0.2, SYSTEM_SUSPEND even is not supported, right?

Quoted 4 lines

So can I keep "arm,psci-0.2" here?

Quoted 1 line

Can CPU0 be hot unplugged?

Yes, CPU0~CPU7 all can be hot unplugged.

Just clarify: now use s/w method to emulate CPU and cluster's low power state; all cores can be hotplugged [1].

Quoted 11 lines · Leo Yan

Quoted 10 lines · Bintian Wang

Quoted 8 lines · Mark Rutland

[...]

arch/arm64/boot/dts/hisilicon/hi6220.dtsi line 19

Quoted 4 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +4 -0

+	psci {
+		compatible = "arm,psci-0.2";
+		method = "smc";
+	};

Are all the PSCI 0.2 mandatory features implemented?

Quoted 5 lines

Quoted 3 lines

The system off/suspend is under development, and system off will be released in next months, and system suspend may be released in the following two months.

Yes; now for mandatory features, except SYSTEM_OFF others have supported.

Ok. Do you mean in the next month, or in a number of months?

Quoted 1 line

for PSCI 0.2, SYSTEM_SUSPEND even is not supported, right?

SYSTEM_SUSPEND was introduced in PSCI 1.0, and is optional.

So long as PSCI_VERSION reports 0.2 or PSCI_FEATURES reports NOT_SUPPORTED for SYSTEM_SUSPEND, it's ok to not implement.

I guess PSCI_VERSION reports PSCI 0.2?

Quoted 7 lines

Quoted 4 lines

So can I keep "arm,psci-0.2" here?

Quoted 1 line

Can CPU0 be hot unplugged?

Yes, CPU0~CPU7 all can be hot unplugged.

Just clarify: now use s/w method to emulate CPU and cluster's low power state; all cores can be hotplugged [1].

So long as CPUs brought online are reset appropriately, and "offline" CPUs can't bring down the system somehow, that should be fine. For now the important part is the interface.

Hello Mark,

On 2015/5/6 1:13, Mark Rutland wrote:

Quoted 9 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi line 170

Quoted 8 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +5 -0

+        uart0: uart at f8015000 {    /* console */
+            compatible = "arm,pl011", "arm,primecell";
+            reg = <0x0 0xf8015000 0x0 0x1000>;
+            interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl

HI6220_UART0_PCLK>;

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +2 -0

+            clock-names = "uartclk", "apb_pclk";
+        };

Quoted 8 lines

In a previous discussion [1] the UART on HI6220 was described as not fully PL011 compliant, with a number of differences (e.g. the FIFO length).

Given that, I feel somewhat uncomfortable with the current compatible string list. What exactly are those differences? We may need a more specific compatible string (even if in addition to those existing ones), or perhaps other properties.

The small system can be booted and the console also works well without changing any code of driver amba-pl011.c, so I think the compatible string is OK for this patch set.

Hisilicon do some performance enhancements based on PL011, but the current driver "amba-pl011.c" also works on hi6220 without those enhancements driver code.

Quoted 11 lines

Thanks,
Mark.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328978.html

.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

.

Hello Mark,

Quoted 3 lines · Bintian

Hello Mark,

On 2015/5/6 1:13, Mark Rutland wrote:

[...]

Quoted 21 lines

Quoted 9 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi line 170

Quoted 8 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +5 -0

+        uart0: uart at f8015000 {    /* console */
+            compatible = "arm,pl011", "arm,primecell";
+            reg = <0x0 0xf8015000 0x0 0x1000>;
+            interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl

HI6220_UART0_PCLK>;

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +2 -0

+            clock-names = "uartclk", "apb_pclk";
+        };

Quoted 8 lines

In a previous discussion [1] the UART on HI6220 was described as not fully PL011 compliant, with a number of differences (e.g. the FIFO length).

Given that, I feel somewhat uncomfortable with the current compatible string list. What exactly are those differences? We may need a more specific compatible string (even if in addition to those existing ones), or perhaps other properties.

The small system can be booted and the console also works well without changing any code of driver amba-pl011.c, so I think the compatible string is OK for this patch set.

Quoted 3 lines

Hisilicon do some performance enhancements based on PL011, but the current driver "amba-pl011.c" also works on hi6220 without those enhancements driver code.

Checked with Hisilicon chip designer, the UART0 is used for DEBUG console and compliant with PL011 fully.

Quoted 28 lines · Bintian

Hello Mark,

Quoted 3 lines · Bintian

Hello Mark,

On 2015/5/6 1:13, Mark Rutland wrote:

[...]

Quoted 21 lines

Quoted 9 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi line 170

Quoted 8 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +5 -0

+        uart0: uart at f8015000 {    /* console */
+            compatible = "arm,pl011", "arm,primecell";
+            reg = <0x0 0xf8015000 0x0 0x1000>;
+            interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl

HI6220_UART0_PCLK>;

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +2 -0

+            clock-names = "uartclk", "apb_pclk";
+        };

Quoted 8 lines

In a previous discussion [1] the UART on HI6220 was described as not fully PL011 compliant, with a number of differences (e.g. the FIFO length).

Given that, I feel somewhat uncomfortable with the current compatible string list. What exactly are those differences? We may need a more specific compatible string (even if in addition to those existing ones), or perhaps other properties.

The small system can be booted and the console also works well without changing any code of driver amba-pl011.c, so I think the compatible string is OK for this patch set.

Quoted 5 lines

Quoted 3 lines

Hisilicon do some performance enhancements based on PL011, but the current driver "amba-pl011.c" also works on hi6220 without those enhancements driver code.

Checked with Hisilicon chip designer, the UART0 is used for DEBUG console and compliant with PL011 fully.

I think that given that we know the UART is not quite a PL011 we should add an additional compatible string just in case some difference crops up later that is problematic.

So we'd have something like:

	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";

That way we can add any optimisations or workarounds later as required.

Thanks,
Mark.

.

Quoted 6 lines

I think that given that we know the UART is not quite a PL011 we should add an additional compatible string just in case some difference crops up later that is problematic.

So we'd have something like:

Quoted 3 lines

	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";

That way we can add any optimisations or workarounds later as required.

I understand and thanks for your suggestion.

Can I do not do this work in this patch set? Because I got the information UART0 is PL011 compatible. Hisilicon uart engineer can do this work in the future, maybe for UART1/UART2.

Hi,

Quoted 6 lines

Quoted 6 lines

I think that given that we know the UART is not quite a PL011 we should add an additional compatible string just in case some difference crops up later that is problematic.

So we'd have something like:

Quoted 8 lines

Quoted 3 lines

	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";

That way we can add any optimisations or workarounds later as required.

I understand and thanks for your suggestion.

Can I do not do this work in this patch set? Because I got the information UART0 is PL011 compatible. Hisilicon uart engineer can do this work in the future, maybe for UART1/UART2.

I am not asking you to do any driver work for this -- the current driver should ignore the "hisilicon,hi6220-uart" string and recognise "arm,pl011".

I am only asking you to add the additional string to the DTS, and to update the binding document to list the new string. That way if and when we need the kernel to distinguish between a regular PL011 and the hi6220-specific variant, the DTB does not need to be updated in order to do so.

Is UART 0 different from UART1 and UART2?

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

How about add the following binding rule to my 2/5 patch:
---------------------------
*Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART

Required properties:
- compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011"

- reg: exactly one register range with length 0x1000
- interrupts: exactly one interrupt specifier

See also bindings/serial/pl011.txt

Example:

uart0: uart at f8015000 {
        compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
        reg = <0x0 0xf8015000 0x0 0x1000>;
        interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
        clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>;
        clock-names = "uartclk", "apb_pclk";
};

Quoted 1 line

Is UART 0 different from UART1 and UART2?

Yes, but my patch just includes UART0, we do some changements for UART1/2 to improve performance.

Hi,

Quoted 7 lines

How about add the following binding rule to my 2/5 patch:
---------------------------
*Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART

Required properties:
- compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011"

"arm,primecell" should come last.

Quoted 7 lines

- reg: exactly one register range with length 0x1000
- interrupts: exactly one interrupt specifier

See also bindings/serial/pl011.txt

Example:

Quoted 9 lines

uart0: uart at f8015000 {
        compatible = "hisilicon,hi6220-uart", "arm,pl011",
"arm,primecell";
        reg = <0x0 0xf8015000 0x0 0x1000>;
        interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
        clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
HI6220_UART0_PCLK>;
        clock-names = "uartclk", "apb_pclk";
};

How about for the moment we just modify the existing pl011 binding document to allow for the hi6220-specific string in addition to its existing strings? We can always split it out later if the hi6220 UART binding needs to be significantly divergent.

Quoted 5 lines

Quoted 2 lines

Is UART 0 different from UART1 and UART2?

Yes, but my patch just includes UART0, we do some changements for UART1/2
to improve performance.

Sure, but we need to make sure we can choose a sane compatible string for them, that won't clash with whatever we choose for UART0.

Are they the same IP as UART0, or fundamentally different?

Are they also PL011 derived?

Thanks,
Mark.

Hi Mark,

2015-05-06 23:44 GMT+08:00, Mark Rutland <[email protected]>:

Quoted 29 lines

Hi,

Quoted 7 lines

How about add the following binding rule to my 2/5 patch:
---------------------------
*Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART

Required properties:
- compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011"

"arm,primecell" should come last.

Quoted 7 lines

- reg: exactly one register range with length 0x1000
- interrupts: exactly one interrupt specifier

See also bindings/serial/pl011.txt

Example:

Quoted 9 lines

uart0: uart at f8015000 {
        compatible = "hisilicon,hi6220-uart", "arm,pl011",
"arm,primecell";
        reg = <0x0 0xf8015000 0x0 0x1000>;
        interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
        clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
HI6220_UART0_PCLK>;
        clock-names = "uartclk", "apb_pclk";
};

Quoted 4 lines

How about for the moment we just modify the existing pl011 binding document to allow for the hi6220-specific string in addition to its existing strings? We can always split it out later if the hi6220 UART binding needs to be significantly divergent.

How about adding the following rule to pl011.txt?
----------

Documentation/devicetree/bindings/serial/pl011.txt +0 -0

index ba3ecb8..1221ccb 100644

Documentation/devicetree/bindings/serial/pl011.txt +0 -1

 See also bindings/arm/primecell.txt
------------

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Is UART 0 different from UART1 and UART2?

Yes, but my patch just includes UART0, we do some changements for UART1/2
to improve performance.

Sure, but we need to make sure we can choose a sane compatible string for them, that won't clash with whatever we choose for UART0.

OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0 node?

Quoted 1 line

Are they the same IP as UART0, or fundamentally different?

The same IP.

Quoted 2 lines

Are they also PL011 derived?

Yes, just do some enhancements to improve performance.

Quoted 50 lines

Hi Mark,

2015-05-06 23:44 GMT+08:00, Mark Rutland <[email protected]>:

Quoted 29 lines

Hi,

Quoted 7 lines

How about add the following binding rule to my 2/5 patch:
---------------------------
*Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART

Required properties:
- compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011"

"arm,primecell" should come last.

Quoted 7 lines

- reg: exactly one register range with length 0x1000
- interrupts: exactly one interrupt specifier

See also bindings/serial/pl011.txt

Example:

Quoted 9 lines

uart0: uart at f8015000 {
        compatible = "hisilicon,hi6220-uart", "arm,pl011",
"arm,primecell";
        reg = <0x0 0xf8015000 0x0 0x1000>;
        interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
        clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
HI6220_UART0_PCLK>;
        clock-names = "uartclk", "apb_pclk";
};

Quoted 4 lines

How about for the moment we just modify the existing pl011 binding document to allow for the hi6220-specific string in addition to its existing strings? We can always split it out later if the hi6220 UART binding needs to be significantly divergent.

How about adding the following rule to pl011.txt?
----------

Documentation/devicetree/bindings/serial/pl011.txt +3 -0

index ba3ecb8..1221ccb 100644
@@ -35,6 +35,10 @@ Optional properties:
 - poll-timeout-ms:
           Poll timeout when auto-poll is set, default
           3000ms.
+- compatible: "hisilicon,hi6220-uart":
+          Hisilicon does some enhancements (e.g. larger FIFO length)
+           based on PL011, so when using these UART hosts, this

Quoted 2 lines

compatible
+ string should be added.

Up at the top of the document there's already a description of the compatible property. Just change it into a list something like:

- compatible: should contain one of the following sequences:
* "arm,pl011", "arm,primecell"
* "hisilicon,hi6220-pl011", "arm,pl011", "arm,primecell"

Quoted 5 lines

See also bindings/arm/primecell.txt
------------

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Is UART 0 different from UART1 and UART2?

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Yes, but my patch just includes UART0, we do some changements for UART1/2
to improve performance.

Sure, but we need to make sure we can choose a sane compatible string for them, that won't clash with whatever we choose for UART0.

OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0 node?

This depends on a number of factors. Questions below.

Quoted 5 lines

Quoted 1 line

Are they the same IP as UART0, or fundamentally different?

The same IP.

Quoted 2 lines

Are they also PL011 derived?

Yes, just do some enhancements to improve performance.

What exactly is different between UART0 and UART{1,2}, given they are the same IP?

Can UART{1,2} be used as if they are standard PL011 instances?

Is the difference internal, or do they just have different clocks/regulators/etc?

Do they have feature control pins tied off differently?

Are they simply programmed differently at reset by firmware?

Thanks,
Mark.

Hi Mark,

2015-05-07 0:23 GMT+08:00, Mark Rutland <[email protected]>:

Quoted 57 lines · Brent Wang

Quoted 50 lines

Hi Mark,

2015-05-06 23:44 GMT+08:00, Mark Rutland <[email protected]>:

Quoted 29 lines

Hi,

Quoted 7 lines

How about add the following binding rule to my 2/5 patch:
---------------------------
*Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART

Required properties:
- compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011"

"arm,primecell" should come last.

Quoted 7 lines

- reg: exactly one register range with length 0x1000
- interrupts: exactly one interrupt specifier

See also bindings/serial/pl011.txt

Example:

Quoted 9 lines

uart0: uart at f8015000 {
         compatible = "hisilicon,hi6220-uart", "arm,pl011",
"arm,primecell";
         reg = <0x0 0xf8015000 0x0 0x1000>;
         interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
         clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
HI6220_UART0_PCLK>;
         clock-names = "uartclk", "apb_pclk";
};

Quoted 4 lines

How about for the moment we just modify the existing pl011 binding document to allow for the hi6220-specific string in addition to its existing strings? We can always split it out later if the hi6220 UART binding needs to be significantly divergent.

How about adding the following rule to pl011.txt?
----------

Documentation/devicetree/bindings/serial/pl011.txt +3 -0

index ba3ecb8..1221ccb 100644
@@ -35,6 +35,10 @@ Optional properties:
  - poll-timeout-ms:
            Poll timeout when auto-poll is set, default
            3000ms.
+- compatible: "hisilicon,hi6220-uart":
+          Hisilicon does some enhancements (e.g. larger FIFO length)
+           based on PL011, so when using these UART hosts, this

Quoted 2 lines

compatible
+ string should be added.

Up at the top of the document there's already a description of the compatible property. Just change it into a list something like:

Quoted 3 lines

- compatible: should contain one of the following sequences:
* "arm,pl011", "arm,primecell"
* "hisilicon,hi6220-pl011", "arm,pl011", "arm,primecell"

OK, I will add in next version.

Quoted 16 lines

Quoted 5 lines

See also bindings/arm/primecell.txt
------------

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Is UART 0 different from UART1 and UART2?

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Yes, but my patch just includes UART0, we do some changements for UART1/2
to improve performance.

Sure, but we need to make sure we can choose a sane compatible string for them, that won't clash with whatever we choose for UART0.

OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0 node?

This depends on a number of factors. Questions below.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Are they the same IP as UART0, or fundamentally different?

The same IP.

Quoted 2 lines

Are they also PL011 derived?

Yes, just do some enhancements to improve performance.

What exactly is different between UART0 and UART{1,2}, given they are the same IP?

I know they are the same IP, but UART{1,2} have larger FIFO length.

Quoted 2 lines

Can UART{1,2} be used as if they are standard PL011 instances?

Yes

Quoted 2 lines

Is the difference internal, or do they just have different clocks/regulators/etc?

The different FIFO length means internal? Sure, they have different clocks/regualtors.

Quoted 2 lines

Do they have feature control pins tied off differently?

UART{1,2} have reset function, we must disable the reset before using them, is it the feature
control pins you mentioned ?

Quoted 2 lines

Are they simply programmed differently at reset by firmware?

I think so.

For hi6220 uart{1,2} , I think add one "struct vendor_data vendor_hisilicon" to "amba-pl011.c" may be a good solution in the future.

I am not the uart driver developer and the Hisilicon engineer who develop it may have a deep
discussion with you when upstreaming UART{1,2}.

I suggest not add "hisilicon,hi6220-uart" to UART0;

Must go to bed, it's very late here :(

Thanks,

Bintian

Quoted 4 lines

Thanks,
Mark.

Hi,

Quoted 6 lines

Quoted 6 lines

I think that given that we know the UART is not quite a PL011 we should add an additional compatible string just in case some difference crops up later that is problematic.

So we'd have something like:

Quoted 8 lines

Quoted 3 lines

	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";

That way we can add any optimisations or workarounds later as required.

I understand and thanks for your suggestion.

Can I do not do this work in this patch set? Because I got the information UART0 is PL011 compatible. Hisilicon uart engineer can do this work in the future, maybe for UART1/UART2.

I am not asking you to do any driver work for this -- the current driver should ignore the "hisilicon,hi6220-uart" string and recognise "arm,pl011".

I am only asking you to add the additional string to the DTS, and to update the binding document to list the new string. That way if and when we need the kernel to distinguish between a regular PL011 and the hi6220-specific variant, the DTB does not need to be updated in order to do so.

Is UART 0 different from UART1 and UART2?

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Mark,

Quoted 21 lines

Hi,

Quoted 6 lines

Quoted 6 lines

I think that given that we know the UART is not quite a PL011 we should add an additional compatible string just in case some difference crops up later that is problematic.

So we'd have something like:

Quoted 8 lines

Quoted 3 lines

	compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";

That way we can add any optimisations or workarounds later as required.

I understand and thanks for your suggestion.

Can I do not do this work in this patch set? Because I got the information UART0 is PL011 compatible. Hisilicon uart engineer can do this work in the future, maybe for UART1/UART2.

I am not asking you to do any driver work for this -- the current driver should ignore the "hisilicon,hi6220-uart" string and recognise "arm,pl011".

Quoted 5 lines

I am only asking you to add the additional string to the DTS, and to update the binding document to list the new string. That way if and when we need the kernel to distinguish between a regular PL011 and the hi6220-specific variant, the DTB does not need to be updated in order to do so.

How about add the following binding rule to the 2/5 patch:
---------------------------
*Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART

Required properties:
- compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011"
- reg: exactly one register range with length 0x1000
- interrupts: exactly one interrupt specifier

See also bindings/serial/pl011.txt

Example:

uart0: uart at f8015000 {
         compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";
         reg = <0x0 0xf8015000 0x0 0x1000>;
         interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
         clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>;
         clock-names = "uartclk", "apb_pclk";
};

---------------------------

Quoted 1 line

Is UART 0 different from UART1 and UART2?

Yes, but my patch just includes UART0, we do some changements for UART1/2 to improve performance.

Thanks,

Bintian

Quoted 8 lines

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

.

Quoted 28 lines · Bintian

Hello Mark,

Quoted 3 lines · Bintian

Hello Mark,

On 2015/5/6 1:13, Mark Rutland wrote:

[...]

Quoted 21 lines

Quoted 9 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi line 170

Quoted 8 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +5 -0

+        uart0: uart at f8015000 {    /* console */
+            compatible = "arm,pl011", "arm,primecell";
+            reg = <0x0 0xf8015000 0x0 0x1000>;
+            interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl

HI6220_UART0_PCLK>;

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +2 -0

+            clock-names = "uartclk", "apb_pclk";
+        };

Quoted 8 lines

In a previous discussion [1] the UART on HI6220 was described as not fully PL011 compliant, with a number of differences (e.g. the FIFO length).

Given that, I feel somewhat uncomfortable with the current compatible string list. What exactly are those differences? We may need a more specific compatible string (even if in addition to those existing ones), or perhaps other properties.

The small system can be booted and the console also works well without changing any code of driver amba-pl011.c, so I think the compatible string is OK for this patch set.

Quoted 5 lines

Quoted 3 lines

Hisilicon do some performance enhancements based on PL011, but the current driver "amba-pl011.c" also works on hi6220 without those enhancements driver code.

Checked with Hisilicon chip designer, the UART0 is used for DEBUG console and compliant with PL011 fully.

I think that given that we know the UART is not quite a PL011 we should add an additional compatible string just in case some difference crops up later that is problematic.

So we'd have something like:

        compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";

That way we can add any optimisations or workarounds later as required.

Quoted 39 lines · Mark Rutland

Quoted 28 lines · Bintian

Hello Mark,

Quoted 3 lines · Bintian

Hello Mark,

On 2015/5/6 1:13, Mark Rutland wrote:

[...]

Quoted 21 lines

Quoted 9 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi line 170

Quoted 8 lines

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +5 -0

+        uart0: uart at f8015000 {    /* console */
+            compatible = "arm,pl011", "arm,primecell";
+            reg = <0x0 0xf8015000 0x0 0x1000>;
+            interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl

HI6220_UART0_PCLK>;

arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file +2 -0

+            clock-names = "uartclk", "apb_pclk";
+        };

Quoted 8 lines

In a previous discussion [1] the UART on HI6220 was described as not fully PL011 compliant, with a number of differences (e.g. the FIFO length).

Given that, I feel somewhat uncomfortable with the current compatible string list. What exactly are those differences? We may need a more specific compatible string (even if in addition to those existing ones), or perhaps other properties.

The small system can be booted and the console also works well without changing any code of driver amba-pl011.c, so I think the compatible string is OK for this patch set.

Quoted 5 lines

Quoted 3 lines

Hisilicon do some performance enhancements based on PL011, but the current driver "amba-pl011.c" also works on hi6220 without those enhancements driver code.

Checked with Hisilicon chip designer, the UART0 is used for DEBUG console and compliant with PL011 fully.

I think that given that we know the UART is not quite a PL011 we should add an additional compatible string just in case some difference crops up later that is problematic.

Quoted 6 lines

So we'd have something like:

        compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell";

That way we can add any optimisations or workarounds later as required.

There's no code to handle the feature of hi6220 uart in mainline.

It's meaningless to append "hisilicon,hi6220-uart" at here. I think it's only required when those uart code are merged into mainline.

Hi6220 is one mobile solution of Hisilicon, this patchset contains initial support for Hi6220 SoC and HiKey development board, which supports octal ARM Cortex A53 cores. Initial support is minimal and includes just the arch configuration, clock driver, device tree configuration.

PSCI is enabled in device tree and there is no problem to boot all the octal cores, and the CPU hotplug is also working now, you can download and compile the latest firmware based on the following link to run this patch set:
https://github.com/96boards/documentation/wiki/UEFI

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts

Hi6220 is one mobile solution of Hisilicon, this patchset contains initial support for Hi6220 SoC and HiKey development board, which supports octal ARM Cortex A53 cores. Initial support is minimal and includes just the arch configuration, clock driver, device tree configuration.

PSCI is enabled in device tree and there is no problem to boot all the octal cores, and the CPU hotplug is also working now, you can download and compile the latest firmware based on the following link to run this patch set:
https://github.com/96boards/documentation/wiki/UEFI

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts

Changes v3:
* Verified the CPU hotplug based on the new released firmware
* Redefined the compatible strings of four system controllers in dts
* Setting COMMON_CLK_HI6220 to a bool symbol
* Keep CONFGI_ARCH_HISI sorted alphabetically

Changes v2:
* Split the DT bindings documents into earlier patches
* Change SMP enable method from spin-table to PSCI in device tree
* Remove "clock-frequency" from armv8-timer device node in device tree
* Add more description about Hisilicon designed system controllers in DT bindings document
* Enable high speed clock on UART1 mux
* Other changes based on the discussion in the mailing list:
https://lkml.org/lkml/2015/2/5/147

Bintian Wang (5):
arm64: Enable Hisilicon ARMv8 SoC family in Kconfig and defconfig
arm64: hi6220: Document devicetree bindings for Hisilicon hi6220 SoC
clk: hi6220: Document devicetree bindings for hi6220 clock
clk: hi6220: Clock driver support for Hisilicon hi6220 SoC
arm64: dts: Add dts files for Hisilicon Hi6220 SoC

 .../bindings/arm/hisilicon/hisilicon.txt           |  87 ++++++
 .../devicetree/bindings/clock/hi6220-clock.txt     |  34 +++
 arch/arm64/Kconfig                                 |   5 +
 arch/arm64/boot/dts/Makefile                       |   1 +
 arch/arm64/boot/dts/hisilicon/Makefile             |   5 +
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |  31 +++
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi          | 172 ++++++++++++
 arch/arm64/configs/defconfig                       |   1 +
 drivers/clk/Kconfig                                |   2 +
 drivers/clk/Makefile                               |   4 +-
 drivers/clk/hisilicon/Kconfig                      |   6 +
 drivers/clk/hisilicon/Makefile                     |   3 +-
 drivers/clk/hisilicon/clk-hi6220.c                 | 292 +++++++++++++++++++++
 drivers/clk/hisilicon/clk.c                        |  29 ++
 drivers/clk/hisilicon/clk.h                        |  17 ++
 drivers/clk/hisilicon/clkdivider-hi6220.c          | 273 +++++++++++++++++++
 include/dt-bindings/clock/hi6220-clock.h           | 173 ++++++++++++
 17 files changed, 1131 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/hi6220-clock.txt
 create mode 100644 arch/arm64/boot/dts/hisilicon/Makefile
 create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
 create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220.dtsi
 create mode 100644 drivers/clk/hisilicon/Kconfig
 create mode 100644 drivers/clk/hisilicon/clk-hi6220.c
 create mode 100644 drivers/clk/hisilicon/clkdivider-hi6220.c
 create mode 100644 include/dt-bindings/clock/hi6220-clock.h

--
1.9.1

Quoted 12 lines · Bintian Wang

Hi6220 is one mobile solution of Hisilicon, this patchset contains initial support for Hi6220 SoC and HiKey development board, which supports octal ARM Cortex A53 cores. Initial support is minimal and includes just the arch configuration, clock driver, device tree configuration.

PSCI is enabled in device tree and there is no problem to boot all the octal cores, and the CPU hotplug is also working now, you can download and compile the latest firmware based on the following link to run this patch set:
https://github.com/96boards/documentation/wiki/UEFI

Quoted 3 lines

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts

Tested by: Tyler Baker <[email protected]>

Built and booted all v4 patches in this series ontop a v4.1-rc1 based tree. Tested with the UEFI load mentioned above, booting to a minimal ramdisk userspace[0]. Confirmed all 8 CPUs were activated.

Quoted 51 lines

Changes v3:
* Verified the CPU hotplug based on the new released firmware
* Redefined the compatible strings of four system controllers in dts
* Setting COMMON_CLK_HI6220 to a bool symbol
* Keep CONFGI_ARCH_HISI sorted alphabetically

Changes v2:
* Split the DT bindings documents into earlier patches
* Change SMP enable method from spin-table to PSCI in device tree
* Remove "clock-frequency" from armv8-timer device node in device tree
* Add more description about Hisilicon designed system controllers in DT bindings document
* Enable high speed clock on UART1 mux
* Other changes based on the discussion in the mailing list:

   https://lkml.org/lkml/2015/2/5/147

Bintian Wang (5):
arm64: Enable Hisilicon ARMv8 SoC family in Kconfig and defconfig
arm64: hi6220: Document devicetree bindings for Hisilicon hi6220 SoC
clk: hi6220: Document devicetree bindings for hi6220 clock
clk: hi6220: Clock driver support for Hisilicon hi6220 SoC
arm64: dts: Add dts files for Hisilicon Hi6220 SoC

  .../bindings/arm/hisilicon/hisilicon.txt           |  87 ++++++
  .../devicetree/bindings/clock/hi6220-clock.txt     |  34 +++
  arch/arm64/Kconfig                                 |   5 +
  arch/arm64/boot/dts/Makefile                       |   1 +
  arch/arm64/boot/dts/hisilicon/Makefile             |   5 +
  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |  31 +++
  arch/arm64/boot/dts/hisilicon/hi6220.dtsi          | 172 ++++++++++++
  arch/arm64/configs/defconfig                       |   1 +
  drivers/clk/Kconfig                                |   2 +
  drivers/clk/Makefile                               |   4 +-
  drivers/clk/hisilicon/Kconfig                      |   6 +
  drivers/clk/hisilicon/Makefile                     |   3 +-
  drivers/clk/hisilicon/clk-hi6220.c                 | 292 +++++++++++++++++++++
  drivers/clk/hisilicon/clk.c                        |  29 ++
  drivers/clk/hisilicon/clk.h                        |  17 ++
  drivers/clk/hisilicon/clkdivider-hi6220.c          | 273 +++++++++++++++++++
  include/dt-bindings/clock/hi6220-clock.h           | 173 ++++++++++++
  17 files changed, 1131 insertions(+), 4 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/hi6220-clock.txt create mode 100644 arch/arm64/boot/dts/hisilicon/Makefile create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220.dtsi create mode 100644 drivers/clk/hisilicon/Kconfig
create mode 100644 drivers/clk/hisilicon/clk-hi6220.c
create mode 100644 drivers/clk/hisilicon/clkdivider-hi6220.c create mode 100644 include/dt-bindings/clock/hi6220-clock.h

Quoted 3 lines

--
1.9.1

Cheers,

Tyler

[0] http://kernelci.org/boot/hi6220-hikey/job/testing/kernel/v4.1-rc1-5-gf609561/defconfig/defconfig/lab/lab-tbaker/?_id=5549541559b51417e999c5cd

.

Hi6220 is one mobile solution of Hisilicon, this patchset contains initial support for Hi6220 SoC and HiKey development board, which supports octal ARM Cortex A53 cores. Initial support is minimal and includes just the arch configuration, clock driver, device tree configuration.

PSCI is enabled in device tree and there is no problem to boot all the octal cores, and the CPU hotplug is also working now, you can download and compile the latest firmware based on the following link to run this patch set:
https://github.com/96boards/documentation/wiki/UEFI

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts

Hi Bintian,

Quoted 12 lines · Bintian Wang

Hi6220 is one mobile solution of Hisilicon, this patchset contains initial support for Hi6220 SoC and HiKey development board, which supports octal ARM Cortex A53 cores. Initial support is minimal and includes just the arch configuration, clock driver, device tree configuration.

PSCI is enabled in device tree and there is no problem to boot all the octal cores, and the CPU hotplug is also working now, you can download and compile the latest firmware based on the following link to run this patch set:
https://github.com/96boards/documentation/wiki/UEFI

Quoted 3 lines

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts

I gave these patches a go on top of -rc2 using the ATF and UEFI you link to above.

The good news is that the thing booted and all the cores entered at EL2. Thanks!

The bad news is that running hackbench quickly got the *heatsink* temperature to 73 degress C and rising (measured with an infrared thermometer).

So my question is, does this SoC have an automatic thermal cut out? Whilst I'm all for merging enabling code into the kernel, if it really relies on the kernel to stop it from catching fire, maybe it's not a great idea putting these patches into people's hands just yet.

Will

.

Quoted 17 lines · Will Deacon

Quoted 15 lines · Bintian Wang

Hi6220 is one mobile solution of Hisilicon, this patchset contains initial support for Hi6220 SoC and HiKey development board, which supports octal ARM Cortex A53 cores. Initial support is minimal and includes just the arch configuration, clock driver, device tree configuration.

PSCI is enabled in device tree and there is no problem to boot all the octal cores, and the CPU hotplug is also working now, you can download and compile the latest firmware based on the following link to run this patch set:
https://github.com/96boards/documentation/wiki/UEFI

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts

Quoted 5 lines

I gave these patches a go on top of -rc2 using the ATF and UEFI you link to above.

The good news is that the thing booted and all the cores entered at EL2. Thanks!

Really thank you very much for testing this patch set.

Quoted 3 lines

The bad news is that running hackbench quickly got the *heatsink* temperature to 73 degress C and rising (measured with an infrared thermometer).

This patch set is just for booting the small system, if you want to test the temperature, I think you should using the HiKey released version (https://www.96boards.org/).

This patch is just for the small system, and not include those drivers for adjusting the CPU frequency, thermal control and so on. After this patch is merged, all those drivers will be submitted later.

Quoted 4 lines

So my question is, does this SoC have an automatic thermal cut out? Whilst I'm all for merging enabling code into the kernel, if it really relies on the kernel to stop it from catching fire, maybe it's not a great idea putting these patches into people's hands just yet.

Hikey is a low cost board, I think it doesn't have an automatic thermal cut out; I always use HiKey to test my patch, in the normal case, temperature is not a problem.

Quoted 2 lines · Bintian

Quoted 1 line · Will Deacon

On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:

Quoted 6 lines

Quoted 3 lines

The bad news is that running hackbench quickly got the *heatsink* temperature to 73 degress C and rising (measured with an infrared thermometer).

This patch set is just for booting the small system, if you want to test the temperature, I think you should using the HiKey released version (https://www.96boards.org/).

I'm not really interested in the temperature numbers, but I am interested in the board not melting and potentially setting fire to my desk.

Quoted 3 lines

This patch is just for the small system, and not include those drivers for adjusting the CPU frequency, thermal control and so on. After this patch is merged, all those drivers will be submitted later.

Should those drivers *really* exist only in the kernel? What happens if the kernel panics for some other reason? You'll basically have 8 spinning cores and no sensible way to handle the thermal interrupt.

Shouldn't there be something in the secure firmware as a last resort?

Quoted 7 lines

Quoted 4 lines

So my question is, does this SoC have an automatic thermal cut out? Whilst I'm all for merging enabling code into the kernel, if it really relies on the kernel to stop it from catching fire, maybe it's not a great idea putting these patches into people's hands just yet.

Hikey is a low cost board, I think it doesn't have an automatic thermal cut out; I always use HiKey to test my patch, in the normal case, temperature is not a problem.

I don't see why the cost has anything to do with this issue; any money I save on the board will quickly be re-invested in my increased insurance premium.

All I think we need is for secure software to keep an eye on the temperature and hit the power controller if it goes over some `fatal' threshold. Ideally, you'd be able to use a secure interrupt for this, but I suspect that you don't have the right hardware features for that (please correct me if I'm wrong). An alternative would be to hang something off a secure timer and get the firmware to check the board temperature on some low-frequency periodic tick.

Quoted 18 lines · Bintian

Quoted 17 lines · Will Deacon

Quoted 15 lines · Bintian Wang

Hi6220 is one mobile solution of Hisilicon, this patchset contains initial support for Hi6220 SoC and HiKey development board, which supports octal ARM Cortex A53 cores. Initial support is minimal and includes just the arch configuration, clock driver, device tree configuration.

PSCI is enabled in device tree and there is no problem to boot all the octal cores, and the CPU hotplug is also working now, you can download and compile the latest firmware based on the following link to run this patch set:
https://github.com/96boards/documentation/wiki/UEFI

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts

Quoted 6 lines

Quoted 5 lines

I gave these patches a go on top of -rc2 using the ATF and UEFI you link to above.

The good news is that the thing booted and all the cores entered at EL2. Thanks!

Really thank you very much for testing this patch set.

Feel free to add my tested-by if you like.

Quoted 7 lines

Quoted 4 lines

The bad news is that running hackbench quickly got the *heatsink* temperature to 73 degress C and rising (measured with an infrared thermometer).

This patch set is just for booting the small system, if you want to test the temperature, I think you should using the HiKey released version (https://www.96boards.org/).

I'm not really interested in the temperature numbers, but I am interested in the board not melting and potentially setting fire to my desk.

Quoted 3 lines

This patch is just for the small system, and not include those drivers for adjusting the CPU frequency, thermal control and so on. After this patch is merged, all those drivers will be submitted later.

Should those drivers *really* exist only in the kernel? What happens if the kernel panics for some other reason? You'll basically have 8 spinning cores and no sensible way to handle the thermal interrupt.

Shouldn't there be something in the secure firmware as a last resort?

Quoted 7 lines

Quoted 4 lines

So my question is, does this SoC have an automatic thermal cut out? Whilst I'm all for merging enabling code into the kernel, if it really relies on the kernel to stop it from catching fire, maybe it's not a great idea putting these patches into people's hands just yet.

Hikey is a low cost board, I think it doesn't have an automatic thermal cut out; I always use HiKey to test my patch, in the normal case, temperature is not a problem.

I don't see why the cost has anything to do with this issue; any money I save on the board will quickly be re-invested in my increased insurance premium.

All I think we need is for secure software to keep an eye on the temperature and hit the power controller if it goes over some `fatal' threshold. Ideally, you'd be able to use a secure interrupt for this, but I suspect that you don't have the right hardware features for that (please correct me if I'm wrong). An alternative would be to hang something off a secure timer and get the firmware to check the board temperature on some low-frequency periodic tick.

Will

.

Quoted 9 lines · Will Deacon

Quoted 3 lines · Bintian

Hikey is a low cost board, I think it doesn't have an automatic thermal cut out; I always use HiKey to test my patch, in the normal case, temperature is not a problem.

I don't see why the cost has anything to do with this issue; any money I save on the board will quickly be re-invested in my increased insurance premium.

Quoted 7 lines

All I think we need is for secure software to keep an eye on the temperature and hit the power controller if it goes over some `fatal' threshold. Ideally, you'd be able to use a secure interrupt for this, but I suspect that you don't have the right hardware features for that (please correct me if I'm wrong). An alternative would be to hang something off a secure timer and get the firmware to check the board temperature on some low-frequency periodic tick.

If there is exception occurred on A core, there are two methods to handle it:
(1) Delay for a period of time, watchdog will trigger the system reset.
(2) If the temperature is over 105 degree, the CPU will trigger reset(I guess it's chip level).

Quoted 17 lines · Bintian

Quoted 16 lines · Will Deacon

Quoted 3 lines · Bintian

Hikey is a low cost board, I think it doesn't have an automatic thermal cut out; I always use HiKey to test my patch, in the normal case, temperature is not a problem.

I don't see why the cost has anything to do with this issue; any money I save on the board will quickly be re-invested in my increased insurance premium.

All I think we need is for secure software to keep an eye on the temperature and hit the power controller if it goes over some `fatal' threshold. Ideally, you'd be able to use a secure interrupt for this, but I suspect that you don't have the right hardware features for that (please correct me if I'm wrong). An alternative would be to hang something off a secure timer and get the firmware to check the board temperature on some low-frequency periodic tick.

Quoted 5 lines

If there is exception occurred on A core, there are two methods to handle it:
(1) Delay for a period of time, watchdog will trigger the system reset.
(2) If the temperature is over 105 degree, the CPU will trigger reset(I guess it's chip level).

Aha, so now you're saying that there *is* a hardware shut-off at 105 degrees, regardless of what the kernel is doing?

If that's the case, then
we're fine and the current patches make sense in isolation.

Cheers,

Will

.

Hi Bintian,

Quoted 12 lines · Bintian Wang

Hi6220 is one mobile solution of Hisilicon, this patchset contains initial support for Hi6220 SoC and HiKey development board, which supports octal ARM Cortex A53 cores. Initial support is minimal and includes just the arch configuration, clock driver, device tree configuration.

PSCI is enabled in device tree and there is no problem to boot all the octal cores, and the CPU hotplug is also working now, you can download and compile the latest firmware based on the following link to run this patch set:
https://github.com/96boards/documentation/wiki/UEFI

Quoted 3 lines

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts

I gave these patches a go on top of -rc2 using the ATF and UEFI you link to above.

The good news is that the thing booted and all the cores entered at EL2. Thanks!

The bad news is that running hackbench quickly got the *heatsink* temperature to 73 degress C and rising (measured with an infrared thermometer).

So my question is, does this SoC have an automatic thermal cut out?

Quoted 24 lines · Will Deacon

Hi Bintian,

Quoted 12 lines · Bintian Wang

Hi6220 is one mobile solution of Hisilicon, this patchset contains initial support for Hi6220 SoC and HiKey development board, which supports octal ARM Cortex A53 cores. Initial support is minimal and includes just the arch configuration, clock driver, device tree configuration.

PSCI is enabled in device tree and there is no problem to boot all the octal cores, and the CPU hotplug is also working now, you can download and compile the latest firmware based on the following link to run this patch set:
https://github.com/96boards/documentation/wiki/UEFI

Quoted 3 lines

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts

I gave these patches a go on top of -rc2 using the ATF and UEFI you link to above.

The good news is that the thing booted and all the cores entered at EL2. Thanks!

Quoted 3 lines

The bad news is that running hackbench quickly got the *heatsink* temperature to 73 degress C and rising (measured with an infrared thermometer).

Because you're just testing with minimum patch set. If you can choose our release
on v3.10 & v3.18, you can observe lower temperature. Because we have thermal framework and cpufreq driver. We're also upstreaming the thermal driver, but it's not merged yet.

Quoted 2 lines

So my question is, does this SoC have an automatic thermal cut out?

It's a low cost board. This feature is implemente by software. So we need thermal
driver and cpufreq driver.

Regards
Haojian

Hi6220 is one mobile solution of Hisilicon, this patchset contains initial support for Hi6220 SoC and HiKey development board, which supports octal ARM Cortex A53 cores. Initial support is minimal and includes just the arch configuration, clock driver, device tree configuration.

PSCI is enabled in device tree and there is no problem to boot all the octal cores, and the CPU hotplug is also working now, you can download and compile the latest firmware based on the following link to run this patch set:
https://github.com/96boards/documentation/wiki/UEFI

Changes v4:
* Rebase to kernel 4.1-rc1
* Delete "arm,cortex-a15-gic" from the gic node in dts

Changes v3:
* Verified the CPU hotplug based on the new released firmware
* Redefined the compatible strings of four system controllers in dts
* Setting COMMON_CLK_HI6220 to a bool symbol
* Keep CONFGI_ARCH_HISI sorted alphabetically

Changes v2:
* Split the DT bindings documents into earlier patches
* Change SMP enable method from spin-table to PSCI in device tree
* Remove "clock-frequency" from armv8-timer device node in device tree
* Add more description about Hisilicon designed system controllers in DT bindings document
* Enable high speed clock on UART1 mux
* Other changes based on the discussion in the mailing list:

   https://lkml.org/lkml/2015/2/5/147

Bintian Wang (5):
arm64: Enable Hisilicon ARMv8 SoC family in Kconfig and defconfig
arm64: hi6220: Document devicetree bindings for Hisilicon hi6220 SoC
clk: hi6220: Document devicetree bindings for hi6220 clock
clk: hi6220: Clock driver support for Hisilicon hi6220 SoC
arm64: dts: Add dts files for Hisilicon Hi6220 SoC

  .../bindings/arm/hisilicon/hisilicon.txt           |  87 ++++++
  .../devicetree/bindings/clock/hi6220-clock.txt     |  34 +++
  arch/arm64/Kconfig                                 |   5 +
  arch/arm64/boot/dts/Makefile                       |   1 +
  arch/arm64/boot/dts/hisilicon/Makefile             |   5 +
  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |  31 +++
  arch/arm64/boot/dts/hisilicon/hi6220.dtsi          | 172 ++++++++++++
  arch/arm64/configs/defconfig                       |   1 +
  drivers/clk/Kconfig                                |   2 +
  drivers/clk/Makefile                               |   4 +-
  drivers/clk/hisilicon/Kconfig                      |   6 +
  drivers/clk/hisilicon/Makefile                     |   3 +-
  drivers/clk/hisilicon/clk-hi6220.c                 | 292 +++++++++++++++++++++
  drivers/clk/hisilicon/clk.c                        |  29 ++
  drivers/clk/hisilicon/clk.h                        |  17 ++
  drivers/clk/hisilicon/clkdivider-hi6220.c          | 273 +++++++++++++++++++
  include/dt-bindings/clock/hi6220-clock.h           | 173 ++++++++++++
  17 files changed, 1131 insertions(+), 4 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/hi6220-clock.txt create mode 100644 arch/arm64/boot/dts/hisilicon/Makefile create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220.dtsi create mode 100644 drivers/clk/hisilicon/Kconfig
create mode 100644 drivers/clk/hisilicon/clk-hi6220.c
create mode 100644 drivers/clk/hisilicon/clkdivider-hi6220.c create mode 100644 include/dt-bindings/clock/hi6220-clock.h

Any other suggestions for this patch set?

Quoted 1 line · Bintian

Any other suggestions for this patch set?

You sent a v5, right, so this version (4) has been superseded? The patches should all go via arm-soc once you've got the relevant acks in place (I don't think you need anything from me or Catalin -- the Kconfig changes are trivial).

Quoted 1 line · Bintian

Any other suggestions for this patch set?

You sent a v5, right, so this version (4) has been superseded?

The patches should all go via arm-soc once you've got the relevant acks in place (I don't think you need anything from me or Catalin -- the Kconfig changes are trivial).

Will

.