Re: [PATCH 3/3 v6] ARC: hsdk: initial port for HSDK board

[Alexey Brodkin]

Hi Rob,

On Mon, 2017-06-26 at 10:51 -0500, Rob Herring wrote:
> On Mon, Jun 26, 2017 at 10:11 AM, Eugeniy Paltsev
> <Eugeniy.Paltsev@xxxxxxxxxxxx> wrote:
> > 
> > From: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
> > 

[snip]

> > +
> > +ÂÂÂÂÂÂÂchosen {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbootargs = "earlycon=uart8250,mmio32,0xf0005000,115200n8 console=ttyS0,115200n8 debug print-fatal-signals=1";
> 
> Use stdout-path for the console. Really, the bootargs should be blank
> and populated by the bootloader especially debug options.

Agree but in case of devboards we quite often disable bootloader and
load Linux image in memory via JTAG. So in that case bootargs make perfect sense IMHO.Â

> > 
> > +ÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂcpu {
> 
> 'cpu' is for cpu nodes, so need something different.

Agree.

> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "simple-bus";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#address-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#size-cells = <1>;
> 
> There are no reg properties, so these aren't needed. This doesn't look
> like a simple-bus either.
> 
> Probably these child nodes should just be moved up to root level.

Looks like that. Thanks.

> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupt-parent = <&core_intc>;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcore_clk: core_clk {
> 
> core-clk {

Ok.

> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#clock-cells = <0>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "fixed-clock";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclock-frequency = <1000000000>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcore_intc: archs-intc@cpu {
> 
> cpu is not a valid unit-address. How are these interrupt controllers addressed?

We have per-core INTC so each core communicates to its own INTC and there's no way
for any core to talk with INTC of another core.

But then we have the next level INTC which is IDU (Interrupt Distribution Unit)
which dispatches "common" IRQs to different upstream per-core INTC, see below its node.Â

> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "snps,archs-intc";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupt-controller;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#interrupt-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂidu_intc: idu-interrupt-controller {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "snps,archs-idu-intc";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupt-controller;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#interrupt-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂarcpct0: pct {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "snps,archs-pct";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +ÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂsoc {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "simple-bus";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#address-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#size-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupt-parent = <&idu_intc>;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂranges = <0x00000000 0xf0000000 0x10000000>;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂuart: dw-apb-uart@5000 {
> 
> serial@...

Yep.

> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "snps,dw-apb-uart";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0x5000 0x100>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclock-frequency = <33330000>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupts = <6>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbaud = <115200>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg-shift = <2>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg-io-width = <4>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgmacclk: gmacclk {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "fixed-clock";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclock-frequency = <400000000>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#clock-cells = <0>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmmcclk_ciu: mmcclk_ciu {
> 
> Don't use underscores in node names.

Ok.

> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "fixed-clock";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclock-frequency = <100000000>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#clock-cells = <0>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmmcclk_biu: mmcclk_biu {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "fixed-clock";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclock-frequency = <400000000>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#clock-cells = <0>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂethernet@8000 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#interrupt-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "snps,dwmac";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0x8000 0x2000>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupts = <10>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupt-names = "macirq";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂphy-mode = "rgmii";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂsnps,pbl = <32>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclocks = <&gmacclk>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclock-names = "stmmaceth";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂphy-handle = <&phy0>;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmdio0 {
> 
> mdio {
> 
> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#address-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#size-cells = <0>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "snps,dwmac-mdio";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂphy0: ethernet-phy@0 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂti,tx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂohci@60000 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "generic-ohci";
> 
> This should have an SoC specific compatible.

But why? We don't have any SoC-specific extensions.
This essentially applies to EHCI as well.

> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0x60000 0x100>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupts = <15>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂehci@40000 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "generic-ehci";
> 
> This should have an SoC specific compatible.
> 
> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0x40000 0x100>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupts = <15>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmmc@a000 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "altr,socfpga-dw-mshc";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0xa000 0x400>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂnum-slots = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfifo-depth = <16>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcard-detect-delay = <200>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclocks = <&mmcclk_biu>, <&mmcclk_ciu>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclock-names = "biu", "ciu";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupts = <12>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbus-width = <4>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +ÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂmemory {
> 
> memory@80000000

Ok.

> 
> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#address-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#size-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdevice_type = "memory";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0x80000000 0x40000000>;ÂÂ/* 1 GiB */
> > +ÂÂÂÂÂÂÂ};
> > +};
> > diff --git a/arch/arc/configs/hsdk_defconfig b/arch/arc/configs/hsdk_defconfig
> > new file mode 100644
> > index 0000000..53bc9f3
> > --- /dev/null
> > +++ b/arch/arc/configs/hsdk_defconfig
> > @@ -0,0 +1,75 @@
> > +CONFIG_DEFAULT_HOSTNAME="ARCLinux"
> > +CONFIG_SYSVIPC=y
> > +# CONFIG_CROSS_MEMORY_ATTACH is not set
> > +CONFIG_NO_HZ_IDLE=y
> > +CONFIG_HIGH_RES_TIMERS=y
> > +CONFIG_IKCONFIG=y
> > +CONFIG_IKCONFIG_PROC=y
> > +CONFIG_NAMESPACES=y
> > +# CONFIG_UTS_NS is not set
> > +# CONFIG_PID_NS is not set
> > +CONFIG_BLK_DEV_INITRD=y
> > +CONFIG_INITRAMFS_SOURCE="../../arc_initramfs_hs/"
> > +CONFIG_EMBEDDED=y
> > +CONFIG_PERF_EVENTS=y
> > +# CONFIG_VM_EVENT_COUNTERS is not set
> > +# CONFIG_COMPAT_BRK is not set
> > +CONFIG_SLAB=y
> > +CONFIG_ARC_PLAT_HSDK=y
> > +CONFIG_ISA_ARCV2=y
> > +CONFIG_SMP=y
> > +CONFIG_LINUX_LINK_BASE=0x90000000
> > +CONFIG_KERNEL_RAM_BASE_ADDRESS=0x80000000
> > +CONFIG_ARC_BUILTIN_DTB_NAME="hsdk"
> > +CONFIG_PREEMPT=y
> > +# CONFIG_COMPACTION is not set
> > +CONFIG_NET=y
> > +CONFIG_PACKET=y
> > +CONFIG_UNIX=y
> > +CONFIG_INET=y
> > +CONFIG_DEVTMPFS=y
> > +# CONFIG_STANDALONE is not set
> > +# CONFIG_PREVENT_FIRMWARE_BUILD is not set
> > +# CONFIG_FIRMWARE_IN_KERNEL is not set
> > +CONFIG_SCSI=y
> > +CONFIG_BLK_DEV_SD=y
> > +CONFIG_NETDEVICES=y
> > +CONFIG_STMMAC_ETH=y
> > +CONFIG_MICREL_PHY=y
> > +# CONFIG_INPUT_KEYBOARD is not set
> > +# CONFIG_INPUT_MOUSE is not set
> > +# CONFIG_SERIO is not set
> > +# CONFIG_LEGACY_PTYS is not set
> > +CONFIG_SERIAL_8250=y
> > +CONFIG_SERIAL_8250_CONSOLE=y
> > +CONFIG_SERIAL_8250_DW=y
> > +CONFIG_SERIAL_OF_PLATFORM=y
> > +# CONFIG_HW_RANDOM is not set
> > +# CONFIG_HWMON is not set
> > +CONFIG_FB=y
> > +CONFIG_FB_UDL=y
> > +CONFIG_FRAMEBUFFER_CONSOLE=y
> > +CONFIG_USB=y
> > +CONFIG_USB_EHCI_HCD=y
> > +CONFIG_USB_EHCI_HCD_PLATFORM=y
> > +CONFIG_USB_OHCI_HCD=y
> > +CONFIG_USB_OHCI_HCD_PLATFORM=y
> > +CONFIG_USB_STORAGE=y
> > +CONFIG_MMC=y
> > +CONFIG_MMC_SDHCI=y
> > +CONFIG_MMC_SDHCI_PLTFM=y
> > +CONFIG_MMC_DW=y
> > +# CONFIG_IOMMU_SUPPORT is not set
> > +CONFIG_EXT3_FS=y
> > +CONFIG_VFAT_FS=y
> > +CONFIG_TMPFS=y
> > +CONFIG_NLS_CODEPAGE_437=y
> > +CONFIG_NLS_ISO8859_1=y
> > +# CONFIG_ENABLE_WARN_DEPRECATED is not set
> > +# CONFIG_ENABLE_MUST_CHECK is not set
> > +CONFIG_STRIP_ASM_SYMS=y
> > +CONFIG_LOCKUP_DETECTOR=y
> > +CONFIG_DEFAULT_HUNG_TASK_TIMEOUT=10
> > +# CONFIG_SCHED_DEBUG is not set
> > +# CONFIG_DEBUG_PREEMPT is not set
> > +# CONFIG_FTRACE is not set
> > diff --git a/arch/arc/kernel/devtree.c b/arch/arc/kernel/devtree.c
> > index 3b67f53..521ef35 100644
> > --- a/arch/arc/kernel/devtree.c
> > +++ b/arch/arc/kernel/devtree.c
> > @@ -29,8 +29,9 @@ static void __init arc_set_early_base_baud(unsigned long dt_root)
> > Â{
> > ÂÂÂÂÂÂÂÂif (of_flat_dt_is_compatible(dt_root, "abilis,arc-tb10x"))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂarc_base_baud = 166666666;ÂÂÂÂÂÂ/* Fixed 166.6MHz clk (TB10x) */
> > -ÂÂÂÂÂÂÂelse if (of_flat_dt_is_compatible(dt_root, "snps,arc-sdp"))
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂarc_base_baud = 33333333;ÂÂÂÂÂÂÂ/* Fixed 33MHz clk (AXS10x) */
> > +ÂÂÂÂÂÂÂelse if (of_flat_dt_is_compatible(dt_root, "snps,arc-sdp") ||
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂof_flat_dt_is_compatible(dt_root, "snps,hsdk"))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂarc_base_baud = 33333333;ÂÂÂÂÂÂÂ/* Fixed 33MHz clk (AXS10x & HSDK) */
> 
> You should get this info from DT. You don't need to address that now though.

For normal UART we indeed don't need that. But this is really required for early
console otherwise how may we start outputting stuff even before we have .dtb
unflattened.

The only thing I may think of early platform code that extracts this value from .dtb
image but:
Â1) I'm not sure it is more elegant
Â2) Still it will be a bit too late - now we start printing very-very early.

> > 
> > ÂÂÂÂÂÂÂÂelse if (of_flat_dt_is_compatible(dt_root, "ezchip,arc-nps"))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂarc_base_baud = 800000000;ÂÂÂÂÂÂ/* Fixed 800MHz clk (NPS) */
> > ÂÂÂÂÂÂÂÂelse
> > diff --git a/arch/arc/plat-hsdk/Kconfig b/arch/arc/plat-hsdk/Kconfig
> > new file mode 100644
> > index 0000000..29dffed
> > --- /dev/null
> > +++ b/arch/arc/plat-hsdk/Kconfig
> > @@ -0,0 +1,12 @@
> > +#
> > +# Copyright (C) 2017 Synopsys, Inc. (www.synopsys.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.
> > +#
> > +
> > +menuconfig ARC_PLAT_HSDK
> > +ÂÂÂÂÂÂÂbool "ARC HS Development Kit board"
> 
> Per board config options don't scale and shouldn't be necessary with DT...

Not sure if I understand that completely.
Could you please explain in a bit more details?

-Alexey