Hello Arnd, > Do you have a strong reason to have your own defconfig file? We > currently try to consolidate as much as possible into > multi_v7_defconfig, so please see if you can extend that instead to > do what you need.
There's no reason why we can't use the multi-platform defconfig. I'll make sure our platform uses it in the next revision. > This seems like stuff that should go into the device drivers for the > respective hardware blocks, not into platform code. Understood. I'm not sure if you recall this [1] conversation, but short of having a big array of registers offsets per chip ID (which will probably grow to 10 or more), leveraging the DT to let the bootloader tell the kernel these randomly-located registers are proved to be very lightweight. Seems like there's one thing I could possibly factor out into a separate driver... the registers that assert a chip reset (sw-master-reset-reg). Maybe I could register a reset-controller driver specifically for this purpose? > We try hard to avoid having register available this early and outside > of device drivers. Can you try to make at least some (if not all) of > these more regular, and provide specific comments in the code why this > is not possible for the others? Just to be sure, you're asking to reduce our dependence on touching these machine-specific registers? I will incorporate all of your suggestions into the next revision of the patch set. Thank you for the review! Regards, Marc [1] http://www.spinics.net/lists/arm-kernel/msg288567.html On 12/3/2013 7:01 AM, Arnd Bergmann wrote: > On Wednesday 27 November 2013, Marc Carino wrote: >> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug >> index 5765abf..266c699 100644 >> --- a/arch/arm/Kconfig.debug >> +++ b/arch/arm/Kconfig.debug >> @@ -94,6 +94,17 @@ choice >> depends on ARCH_BCM2835 >> select DEBUG_UART_PL01X >> >> + config DEBUG_BRCMSTB_UART >> + bool "Use BRCMSTB UART for low-level debug" >> + depends on ARCH_BRCMSTB >> + select DEBUG_UART_8250 >> + help >> + Say Y here if you want the debug print routines to direct >> + their output to the first serial port on these devices. >> + >> + If you have a Broadcom STB chip and would like early print >> + messages to appear over the UART, select this option. >> + >> config DEBUG_CLPS711X_UART1 >> bool "Kernel low-level debugging messages via UART1" >> depends on ARCH_CLPS711X > > Can you split out the debug UART changes into a separate patch? > >> diff --git a/arch/arm/configs/brcmstb_defconfig >> b/arch/arm/configs/brcmstb_defconfig >> new file mode 100644 >> index 0000000..1741d92 >> --- /dev/null >> +++ b/arch/arm/configs/brcmstb_defconfig >> @@ -0,0 +1,127 @@ >> +CONFIG_CROSS_COMPILE="arm-linux-" >> +CONFIG_KERNEL_LZO=y >> +CONFIG_SYSVIPC=y >> +CONFIG_POSIX_MQUEUE=y >> +CONFIG_LOG_BUF_SHIFT=14 >> +CONFIG_SYSFS_DEPRECATED=y >> +CONFIG_RELAY=y >> +CONFIG_BLK_DEV_INITRD=y > > Do you have a strong reason to have your own defconfig file? We currently > try to consolidate as much as possible into multi_v7_defconfig, so please > see if you can extend that instead to do what you need. > >> diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig >> index 9fe6d88..9179259 100644 >> --- a/arch/arm/mach-bcm/Kconfig >> +++ b/arch/arm/mach-bcm/Kconfig >> @@ -31,6 +31,24 @@ config ARCH_BCM_MOBILE >> BCM11130, BCM11140, BCM11351, BCM28145 and >> BCM28155 variants. >> >> +config ARCH_BRCMSTB >> + bool "Broadcom BCM7XXX based boards" if ARCH_MULTI_V7 >> + depends on MMU >> + select ARM_ARCH_TIMER >> + select ARM_GIC >> + select BRCMSTB >> + select MIGHT_HAVE_PCI >> + select HAVE_SMP >> + select USE_OF >> + select CPU_V7 >> + select GENERIC_CLOCKEVENTS > > Some of these are already implied by ARCH_MULTI_V7 and can be dropped > from this list. > >> +struct platform_regs brcm_plat_regs; >> + >> +/*********************************************************************** >> + * STB CPU (main application processor) >> + ***********************************************************************/ >> + >> +static struct map_desc brcmstb_io_map[] __initdata = { >> + { >> + .virtual = (unsigned long)BRCMSTB_PERIPH_VIRT, >> + .pfn = __phys_to_pfn(BRCMSTB_PERIPH_PHYS), >> + .length = BRCMSTB_PERIPH_LENGTH, >> + .type = MT_DEVICE, >> + }, >> +}; > > Why do you need a static I/O mapping? You should not rely on hardcoded > virtual or physical addresses in general. > >> +static struct node_reg sun_top_ctrl_regs[] __initdata = { >> + {"reset-source-enable-reg", &brcm_plat_regs.reset_source_enable_reg}, >> + {"sw-master-reset-reg", &brcm_plat_regs.sw_master_reset_reg}, >> + {NULL, NULL} >> +}; >> + >> +static struct node_reg cpu_biu_ctrl_regs[] __initdata = { >> + {"cpu-reset-config-reg", &brcm_plat_regs.cpu_reset_config_reg}, >> + {"cpu0-pwr-zone-ctrl-reg", &brcm_plat_regs.cpu0_pwr_zone_ctrl_reg}, >> + {NULL, NULL} >> +}; >> + >> +static struct node_reg hif_continuation_regs[] __initdata = { >> + {"stb-boot-hi-addr0-reg", &brcm_plat_regs.hif_continuation_regs_base}, >> + {NULL, NULL} >> +}; >> + >> +static struct node_reg_block top_reg_blocks[] __initdata = { >> + {"brcm,brcmstb-sun-top-ctrl", sun_top_ctrl_regs}, >> + {"brcm,brcmstb-cpu-biu-ctrl", cpu_biu_ctrl_regs}, >> + {"brcm,brcmstb-hif-continuation", hif_continuation_regs}, >> + {NULL, NULL} >> +}; > > This seems like stuff that should go into the device drivers for the > respective hardware blocks, not into platform code. > >> + addr = ioremap(BPHYSADDR(BCHP_IRQ0_IRQEN), sizeof(u32)); >> + writel_relaxed(BCHP_IRQ0_IRQEN_uarta_irqen_MASK >> + | BCHP_IRQ0_IRQEN_uartb_irqen_MASK >> + | BCHP_IRQ0_IRQEN_uartc_irqen_MASK, addr); >> + iounmap(addr); > > What does this part do? Isn't that something that should have been set > up by the boot loader? > >> + block = top_reg_blocks; >> + while (block->compatible) { >> + struct device_node *np; >> + struct node_reg *reg; >> + >> + np = of_find_compatible_node(NULL, NULL, block->compatible); >> + if (!np) >> + panic("brcmstb: DT missing \"%s\" node\n", >> + block->compatible); >> + >> + addr = of_iomap(np, 0); >> + if (!addr) >> + panic("brcmstb: iomap failure\n"); >> + >> + reg = block->regs; >> + while (reg->prop) { >> + u32 val; >> + >> + if (!of_property_read_u32(np, reg->prop, &val)) >> + *(reg->addr) = addr + val; >> + else >> + panic("brcmstb: node \"%s\" missing prop >> \"%s\"\n", >> + block->compatible, reg->prop); >> + >> + reg++; >> + } >> + >> + of_node_put(np); >> + >> + block++; >> + } >> +} > > We try hard to avoid having register available this early and outside > of device drivers. Can you try to make at least some (if not all) of > these more regular, and provide specific comments in the code why this > is not possible for the others? > >> +static void __init brcmstb_init(void) >> +{ >> + of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); >> +} > > This is the default function an can be omitted. > >> +#define BRCMSTB_PERIPH_VIRT 0xfc000000 >> +#define BRCMSTB_PERIPH_PHYS 0xf0000000 >> +#define BRCMSTB_PERIPH_LENGTH 0x02000000 >> + >> +#define BVIRTADDR(x) (BRCMSTB_PERIPH_VIRT + ((x) & 0x0fffffff)) >> +#define BPHYSADDR(x) ((x) + BRCMSTB_PERIPH_PHYS) >> + >> +#define BCHP_UARTA_REG_START 0x00406b00 >> + >> +#define BCHP_IRQ0_IRQEN 0x00406780 >> +#define BCHP_IRQ0_IRQEN_uarta_irqen_MASK 0x00010000 >> +#define BCHP_IRQ0_IRQEN_uartb_irqen_MASK 0x00020000 >> +#define BCHP_IRQ0_IRQEN_uartc_irqen_MASK 0x00040000 > > These should probably all be private to the files that use them > >> + >> +ENTRY(brcmstb_secondary_startup) >> + mov r0, #0xd3 >> + msr cpsr_fsxc, r0 > > You should have comments here about why this is necessary. > >> +#define ZONE_PWR_DN_REQ_MASK 0x00000200 >> +#define ZONE_PWR_UP_REQ_MASK 0x00000400 >> +#define ZONE_BLK_RST_ASSERT_MASK 0x00001000 >> +#define ZONE_PWR_OFF_STATE_MASK 0x02000000 >> +#define ZONE_PWR_ON_STATE_MASK 0x04000000 >> +#define ZONE_RESET_STATE_MASK 0x80000000 >> + >> +static void __iomem *pwr_zone_ctrl_get_base(unsigned int cpu) >> +{ >> + void __iomem *base = brcm_plat_regs.cpu0_pwr_zone_ctrl_reg; >> + base += (cpu * 4); >> + return base; >> +} > > It looks like you are accessing a register area that is providing power > domains for not just the CPUs but other parts of the system as well, > which should be a proper device driver, e.g. pm_domain or regulator > depending on what the hardware actually does, and then you plug > the SMP code into that. Do you think that would work here? > > In the long run, I would like to see the platform SMP code get merged with > the cpuidle device drivers and moved to drivers/cpuidle, but we're not > quite at the point where this can be done. > >> + /* Magic delay from misc_bpcm_arm.c */ >> + busy_wait(10000); > > I think you should use udelay() here rather than creating your own, but > I may be missing the specific requirements. Of course it would be better > not to need it at all. > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
