lkml.org 
[lkml]   [2014]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/5] ARM: Add platform support for LSI AXM55xx SoC
On Tue, Apr 15, 2014 at 02:30:55PM +0200, Arnd Bergmann wrote:
> On Tuesday 15 April 2014 14:06:10 Anders Berg wrote:
>
> > diff --git a/arch/arm/mach-axxia/Kconfig b/arch/arm/mach-axxia/Kconfig
> > new file mode 100644
> > index 0000000..336426a
> > --- /dev/null
> > +++ b/arch/arm/mach-axxia/Kconfig
> > @@ -0,0 +1,19 @@
> > +config ARCH_AXXIA
> > + bool "LSI Axxia platforms" if (ARCH_MULTI_V7 && ARM_LPAE)
> > + select ARM_GIC
> > + select HAVE_SMP
> > + select MFD_SYSCON
> > + select ARM_AMBA
> > + select ARCH_WANT_OPTIONAL_GPIOLIB
> > + select HAVE_ARM_ARCH_TIMER
> > + select ARM_TIMER_SP804
> > + select ZONE_DMA
> > + select ARCH_DMA_ADDR_T_64BIT
> > + select ARCH_SUPPORTS_BIG_ENDIAN
> > + select MIGHT_HAVE_PCI
> > + select PCI_DOMAINS if PCI
>
> No need to select HAVE_SMP or ARCH_WANT_OPTIONAL_GPIOLIB any more.

OK.

>
> We should rethink the ARCH_SUPPORTS_BIG_ENDIAN option I guess. It makes
> little sense to select that from one platform in a multiplatform build
> if other platforms don't support it.
>
> > --- /dev/null
> > +++ b/arch/arm/mach-axxia/Makefile.boot
> > @@ -0,0 +1,2 @@
> > + zreladdr-y += 0x00308000
> > +params_phys-y := 0x00300100
>
> This won't be used, since AUTO_ZRELADDR is set by ARCH_MULTIPLATFORM.

OK.

>
> > diff --git a/arch/arm/mach-axxia/axxia.c b/arch/arm/mach-axxia/axxia.c
> > new file mode 100644
> > index 0000000..8d85926
> > --- /dev/null
> > +++ b/arch/arm/mach-axxia/axxia.c
> > @@ -0,0 +1,117 @@
> > +/*
> > + * Support for the LSI Axxia SoC devices based on ARM cores.
> > + *
> > + * Copyright (C) 2012 LSI
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/amba/bus.h>
> > +#include <linux/device.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/sizes.h>
> > +#include <asm/mach/arch.h>
> > +#include <asm/pmu.h>
> > +#include "axxia.h"
> > +
> > +/*
> > + * The PMU IRQ lines of four cores are wired together into a single interrupt.
> > + * Bounce the interrupt to other cores if it's not ours.
> > + */
> > +static irqreturn_t axxia_pmu_handler(int irq, void *dev, irq_handler_t handler)
> > +{
> > + irqreturn_t ret = handler(irq, dev);
> > +
> > + if (ret == IRQ_NONE) {
> > + int cpu = smp_processor_id();
> > + int cluster = cpu / CORES_PER_CLUSTER;
> > + int other;
> > +
> > + /* Look until we find another cpu that's online. */
> > + do {
> > + other = (++cpu % CORES_PER_CLUSTER) +
> > + (cluster * CORES_PER_CLUSTER);
> > + } while (!cpu_online(other));
> > +
> > + irq_set_affinity(irq, cpumask_of(other));
> > + }
> > +
> > + /*
> > + * We should be able to get away with the amount of IRQ_NONEs we give,
> > + * while still having the spurious IRQ detection code kick in if the
> > + * interrupt really starts hitting spuriously.
> > + */
> > + return ret;
> > +}
> > +
> > +static struct arm_pmu_platdata pmu_pdata = {
> > + .handle_irq = axxia_pmu_handler,
> > +};
> > +
> > +static struct of_dev_auxdata axxia_auxdata_lookup[] __initdata = {
> > + OF_DEV_AUXDATA("arm,cortex-a15-pmu", 0, "pmu", &pmu_pdata),
> > + {}
> > +};
>
> This looks similar to what we have in mach-ux500 as db8500_pmu_handler.
>
> To avoid duplication, I'd prefer moving support for this into the
> perf_event code itself, and get rid of the auxdata.
>

I'll drop it fow now.

> > +static int
> > +axxia_bus_notifier(struct notifier_block *nb, unsigned long event, void *obj)
> > +{
> > + struct device *dev = obj;
> > +
> > + if (event != BUS_NOTIFY_ADD_DEVICE)
> > + return NOTIFY_DONE;
> > +
> > + if (!of_property_read_bool(dev->of_node, "dma-coherent"))
> > + return NOTIFY_DONE;
> > +
> > + set_dma_ops(dev, &arm_coherent_dma_ops);
> > +
> > + return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block axxia_platform_nb = {
> > + .notifier_call = axxia_bus_notifier,
> > +};
> > +
> > +static struct notifier_block axxia_amba_nb = {
> > + .notifier_call = axxia_bus_notifier,
> > +};
>
> And we definitely want to do this in a generic fashion. That should not be in
> platform specific code.

I'll drop this too. I saw there was some work in progress on this.

>
> > +static const char *axxia_dt_match[] __initconst = {
> > + "lsi,axm55xx",
> > + "lsi,axm55xx-sim",
> > + "lsi,axm55xx-emu",
> > + NULL
> > +};
>
> Please no 'xx' in compatible strings. You can list all the relevant
> model numbers here, or require that machines list a specific model
> as compatible, e.g.
>
> compatible = "lsi,axm5510-sim", "lsi,axm5510", "lsi,axm5502";
>

Got that. I'll use lsi,axm5516 as the complatible model number.

> > +
> > +DT_MACHINE_START(AXXIA_DT, "LSI Axxia AXM55XX")
> > + .dt_compat = axxia_dt_match,
> > + .smp = smp_ops(axxia_smp_ops),
> > + .init_machine = axxia_dt_init,
> > +MACHINE_END
>
> Please have a look at how smp ops are hooked up in mach-qcom and see
> if you can do it that way as well.
>

Yes, that will work nicely.

> > diff --git a/arch/arm/mach-axxia/axxia.h b/arch/arm/mach-axxia/axxia.h
> > new file mode 100644
> > index 0000000..594dd97
> > --- /dev/null
> > +++ b/arch/arm/mach-axxia/axxia.h
> > @@ -0,0 +1,42 @@
> > +/*
> > + * Prototypes for platform functions.
> > + *
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +#ifndef __AXXIA_H
> > +#define __AXXIA_H
> > +
> > +#define CORES_PER_CLUSTER 4
> > +
> > +#define AXXIA_PERIPH_PHYS 0x2000000000ULL
> > +#define AXXIA_SYSCON_PHYS 0x2010030000ULL
>
> Remove these -- they have to come from DT.
>
> > +#if 0
> > +#define AXXIA_UART0_PHYS 0x2010080000ULL
> > +#define AXXIA_UART1_PHYS 0x2010081000ULL
> > +#define AXXIA_UART2_PHYS 0x2010082000ULL
> > +#define AXXIA_UART3_PHYS 0x2010083000ULL
> > +
> > +#ifdef CONFIG_DEBUG_LL
> > +#define AXXIA_DEBUG_UART_VIRT 0xf0080000
> > +#define AXXIA_DEBUG_UART_PHYS AXXIA_UART0_PHYS
> > +#endif
> > +#endif
>
> You can put these constants directly into debug code.
>

Ok.

>
> > diff --git a/arch/arm/mach-axxia/platsmp.c b/arch/arm/mach-axxia/platsmp.c
> > new file mode 100644
> > index 0000000..fd7f507
> > --- /dev/null
> > +++ b/arch/arm/mach-axxia/platsmp.c
> > @@ -0,0 +1,183 @@
> > +/*
> > + * linux/arch/arm/mach-axxia/platsmp.c
> > + *
> > + * Copyright (C) 2012 LSI Corporation
> > + *
> > + * 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.
>
> Have you considered using PSCI to bring up the secondary CPUs?
>

The current u-boot is a bit dated... But I agree. I'll push for an update to this.

> > +static int axxia_boot_secondary(unsigned int cpu,
> > + struct task_struct *idle)
> > +{
> > + unsigned long timeout;
> > +
> > + /* Release the specified core */
> > + write_pen_release(cpu_logical_map(cpu));
> > +
> > + /* Send a wakeup event to get the idled cpu out of WFE state */
> > + dsb_sev();
> > +
> > + /* Wait for so long, then give up if nothing happens ... */
> > + timeout = jiffies + (1 * HZ);
> > + while (time_before(jiffies, timeout)) {
> > + /* Make sure stores to pen_release have completed */
> > + smp_rmb();
> > + if (pen_release == -1)
> > + break;
> > + udelay(1);
> > + }
> > +
> > + return pen_release != -1 ? -ENOSYS : 0;
> > +}
>
> This is pretty sad. No hardware support for waking up CPUs?
>

I'll rework this. There are sw control of the resets to individual cores. And
the holding pen is not needed (too much other platsmp implementations), I will
have the secondary cores released directly into secondary_startup.

> > +static void __init axxia_smp_prepare_cpus(unsigned int max_cpus)
> > +{
> > + void __iomem *syscon;
> > + int cpu_count = 0;
> > + int cpu;
> > +
> > + syscon = ioremap(AXXIA_SYSCON_PHYS, SZ_64K);
> > + if (WARN_ON(!syscon))
> > + return;
> > +
> > + check_fixup_sev(syscon);
>
> Please use the syscon driver and regmap. That driver might need a little
> help to get syscon_node_to_regmap() to work before driver initialization,
> but other people need the same thing, and we should just implement it.

Ok, until then... I'll drop the hardcoded address and get the address from the
syscon node.

>
> > diff --git a/drivers/clk/clk-axxia.c b/drivers/clk/clk-axxia.c
> > new file mode 100644
> > index 0000000..996b8f2
> > --- /dev/null
> > +++ b/drivers/clk/clk-axxia.c
> > @@ -0,0 +1,281 @@
> > +/*
> > + * arch/arm/mach-axxia/clock.c
>
> This should be a separate patch, and get merged by the clk maintainer.
Ok.

I'll submit a v2 with the corrections.

/Anders

>
> Arnd
>
>


\
 
 \ /
  Last update: 2014-04-24 11:41    [W:0.144 / U:2.372 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site