On Friday 09 May 2014, Linus Walleij wrote:
> diff --git a/arch/arm/boot/dts/arm-realview-pb1176.dts
> b/arch/arm/boot/dts/arm-realview-pb1176.dts
> new file mode 100644
> index 000000000000..0eea0f4a7b39
> --- /dev/null
> +++ b/arch/arm/boot/dts/arm-realview-pb1176.dts
Can you split this up into an arm-realview.dtsi file for the common parts and
a pb1176 specific file for the rest?
> + soc {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "simple-bus";
> + ranges;
> +
> + syscon: syscon@10000000 {
> + compatible = "arm,realview-pb1176-syscon", "syscon";
> + reg = <0x10000000 0x1000>;
> + };
Hmm, in order to have a proper reset driver, we probably want a separate
node for the reset controller. I believe the x-gene people have just
posted a generic reset driver based on syscon. Let's see if we can share
that.
> + pb1176_serial0: uart@1010c000 {
> + compatible = "arm,pl011", "arm,primecell";
> + reg = <0x1010c000 0x1000>;
> + interrupt-parent = <&intc_dc1176>;
> + interrupts = <0 18 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&uartclk>, <&pclk>;
> + clock-names = "uartclk", "apb_pclk";
> + };
I believe the "official" name for a uart is serial@1010c000, not uart@1010c000.
I know we are inconsistent with that.
> +
> +/* Pointer to the system controller */
> +struct regmap *syscon_regmap;
> +u8 syscon_type;
> +
> +static struct map_desc realview_dt_io_desc[] __initdata = {
> + {
> + /* FIXME: static map needed for LED driver */
> + .virtual = IO_ADDRESS(REALVIEW_SYS_BASE),
> + .pfn = __phys_to_pfn(REALVIEW_SYS_BASE),
> + .length = SZ_4K,
> + .type = MT_DEVICE,
> + },
> +};
I've looked at the driver and I don't see why this is required.
The driver does a devm_ioremap_resource(), which should work with
or without a static mapping.
> +/*
> + * We detect the different syscon types from the compatible strings.
> + */
> +enum realview_syscon {
> + REALVIEW_SYSCON_EB,
> + REALVIEW_SYSCON_PB1176,
> + REALVIEW_SYSCON_PB11MP,
> + REALVIEW_SYSCON_PBA8,
> + REALVIEW_SYSCON_PBX,
> +};
> +
> +static const struct of_device_id realview_syscon_match[] = {
> + {
> + .compatible = "arm,realview-eb-syscon",
> + .data = (void *)REALVIEW_SYSCON_EB,
> + },
> + {
> + .compatible = "arm,realview-pb1176-syscon",
> + .data = (void *)REALVIEW_SYSCON_PB1176,
> + },
> + {
> + .compatible = "arm,realview-pb11mp-syscon",
> + .data = (void *)REALVIEW_SYSCON_PB11MP,
> + },
> + {
> + .compatible = "arm,realview-pba8-syscon",
> + .data = (void *)REALVIEW_SYSCON_PBA8,
> + },
> + {
> + .compatible = "arm,realview-pbx-syscon",
> + .data = (void *)REALVIEW_SYSCON_PBX,
> + },
> +};
If not the generic syscon reset driver, it should at least live in
drivers/power/reset/ rather than here.
> +#if IS_ENABLED(CONFIG_CACHE_L2X0)
> + if (of_machine_is_compatible("arm,realview-eb"))
> + /*
> + * 1MB (128KB/way), 8-way associativity,
> + * evmon/parity/share enabled
> + * Bits: .... ...0 0111 1001 0000 .... .... ....
> + */
> + l2x0_of_init(0x00790000, 0xfe000fff);
> + else if (of_machine_is_compatible("arm,realview-pb1176"))
> + /*
> + * 128Kb (16Kb/way) 8-way associativity.
> + * evmon/parity/share enabled.
> + */
> + l2x0_of_init(0x00730000, 0xfe000fff);
> + else if (of_machine_is_compatible("arm,realview-pb11mp"))
> + /*
> + * 1MB (128KB/way), 8-way associativity,
> + * evmon/parity/share enabled
> + * Bits: .... ...0 0111 1001 0000 .... .... ....
> + */
> + l2x0_of_init(0x00730000, 0xfe000fff);
> + else if (of_machine_is_compatible("arm,realview-pbx"))
> + /*
> + * 16KB way size, 8-way associativity, parity disabled
> + * Bits: .. 0 0 0 0 1 00 1 0 1 001 0 000 0 .... .... ....
> + */
> + l2x0_of_init(0x02520000, 0xc0000fff);
> +#endif
You wrote that you added the #if IS_ENABLED() here. Why?
l2x0_of_init is already a nop if CONFIG_CACHE_L2X0 is not set. If you want to
skip a bit of dead code here, you could make it an inline function and do
if (IS_ENABLED(CONFIG_CACHE_L2X0))
realview_setup_l2x0();
Other than that, I still think we should avoid doing anything platform
specific here at all, and move it all into the l2x0_of_init function,
based on Russell's l2x0 patch series.
> + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> + if (!soc_dev_attr)
> + return;
> + ret = of_property_read_string(root, "compatible",
> + &soc_dev_attr->soc_id);
> + if (ret)
> + return;
> + ret = of_property_read_string(root, "model", &soc_dev_attr->machine);
> + if (ret)
> + return;
> + soc_dev_attr->family = "RealView";
> + soc_dev = soc_device_register(soc_dev_attr);
> + if (IS_ERR(soc_dev)) {
> + kfree(soc_dev_attr);
> + return;
> + }
> + parent = soc_device_to_device(soc_dev);
> + ret = of_platform_populate(root, of_default_bus_match_table,
> + NULL, parent);
> + if (ret) {
> + pr_crit("could not populate device tree\n");
> + return;
> + }
I wonder if there is a way to do this without having code in the platform.
I would hate it if we get to the point where each platform is completely
empty but still requires a copy of this code.
We just faced the same discussion on the exynos chip id patches.
I also noticed that you pass the DT root here, which isn't actually
the intention of the soc device interface: you should pass the DT node
that has the on-chip devices but not the board-level (top-level) nodes
such as your fixed clocks.
> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> index 5bf7c3c3b301..5461c4401ed6 100644
> --- a/arch/arm/mm/Kconfig
> +++ b/arch/arm/mm/Kconfig
> @@ -928,7 +928,7 @@ config ARM_L1_CACHE_SHIFT
> config ARM_DMA_MEM_BUFFERABLE
> bool "Use non-cacheable memory for DMA" if (CPU_V6 || CPU_V6K) &&
> !CPU_V7
> depends on !(MACH_REALVIEW_PB1176 || REALVIEW_EB_ARM11MP || \
> - MACH_REALVIEW_PB11MP)
> + MACH_REALVIEW_PB11MP || REALVIEW_DT)
> default y if CPU_V6 || CPU_V6K || CPU_V7
> help
> Historically, the kernel has used strongly ordered mappings to
Do you have an explanation for why this is needed? I don't remember seeing
this before, but I guess it gets in the way of multiplatform support.
Is this just a performance optimization on realview, or is it actually
required?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
