Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 15/16] ARM: vexpress/dcscb: handle platform coherency exit/setup and CCI
From: Lorenzo Pieralisi @ 2013-01-14 12:25 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <50F10EF4.9070909@ti.com>

On Sat, Jan 12, 2013 at 07:21:24AM +0000, Santosh Shilimkar wrote:
> On Saturday 12 January 2013 12:58 AM, Nicolas Pitre wrote:
> > On Fri, 11 Jan 2013, Santosh Shilimkar wrote:
> >
> >> On Thursday 10 January 2013 05:50 AM, Nicolas Pitre wrote:
> >>> From: Dave Martin <dave.martin@linaro.org>
> >>>
> >>> +		/*
> >>> +		 * Flush the local CPU cache.
> >>> +		 *
> >>> +		 * A15/A7 can hit in the cache with SCTLR.C=0, so we don't need
> >>> +		 * a preliminary flush here for those CPUs.  At least, that's
> >>> +		 * the theory -- without the extra flush, Linux explodes on
> >>> +		 * RTSM (maybe not needed anymore, to be investigated).
> >>> +		 */
> >> This is expected if the entire code is not in one stack frame and the
> >> additional flush is needed to avoid possible stack corruption. This
> >> issue has been discussed in past on the list.
> >
> > I missed that.  Do you have a reference or pointer handy?
> >
> > What is strange is that this is 100% reproducible on RTSM while this
> > apparently is not an issue on real hardware so far.
> >
> I tried searching archives and realized the discussion was in private
> email thread. There are some bits and pieces on list but not all the
> information.
> 
> The main issue RMK pointed out is- An additional L1 flush needed
> to avoid the effective change of view of memory when the C bit is
> turned off, and the cache is no longer searched for local CPU accesses.
> 
> In your case dcscb_power_down() has updated the stack which can be hit
> in cache line and hence cache is dirty now. Then cpu_proc_fin() clears
> the C-bit and hence for sub sequent calls the L1 cache won't be
> searched. You then call flush_cache_all() which again updates the
> stack but avoids searching the L1 cache. So it overwrites previous
> saved stack frame. This seems to be an issue in your case as well.

On A15/A7 even with the C bit cleared the D-cache is searched, the
situation above cannot happen and if it does we are facing a HW/model bug.
If this code is run on A9 then we have a problem since there, when the C bit
is cleared D-cache is not searched (and that's why the sequence above
should be written in assembly with no data access whatsoever), but on
A15/A7 we do not.

I have been running this code on TC2 for hours on end with nary a problem.

The sequence:

- clear C bit
- clean D-cache
- exit SMP

must be written in assembly with no data access whatsoever to make it
portable across v7 implementations. I think I will write some docs and
add them to the kernel to avoid further discussion on this topic.

FYI, the thread Santosh mentioned:

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-May/099791.html

Lorenzo

^ permalink raw reply

* [PATCH 2/2] ARM: tegra: Use DT /cpu node to detect number of CPU core
From: Lorenzo Pieralisi @ 2013-01-14 12:35 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20130114.114925.1476619399284636466.hdoyu@nvidia.com>

On Mon, Jan 14, 2013 at 09:49:25AM +0000, Hiroshi Doyu wrote:
> Hi Russell,
> 
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote @ Mon, 14 Jan 2013 10:27:20 +0100:
> 
> > On Mon, Jan 14, 2013 at 09:52:50AM +0200, Hiroshi Doyu wrote:
> > > +	if (!arm_dt_cpu_map_valid())
> > > +		set_cpu_possible(0, true);
> > 
> > You don't need to do any of this (and, therefore, I don't think you even
> > need the first patch.)
> > 
> > The generic boot code will set CPU0 as possible, present and online.  Think
> > about it: you're booting on that very CPU so it better be possible, present
> > and online.  If it isn't, what CPU is executing your code?
> > 
> > arm_dt_init_cpu_maps() will only ever set _additional_ CPUs in the possible
> > map.
> > 
> > Ergo, by the time the above code is run, CPU0 will already be marked as
> > possible.  Therefore, the above code and arm_dt_cpu_map_valid() is not
> > required.
> 
> Right.
> 
> In Tegra, we've decided to not use SCU based detection at all any more
> and to continue with a single core even when DT cpu detection
> fails. For Tegra, "arm_dt_cpu_map_valid()" is not necessary. For other
> SoCs, this would be necessary when they want to detect again with
> SCU.

Correct. I will keep my patch around and repost it if and when we want other
platforms to rely on legacy HW number of cores count discovery.

Given that's going to be temporary legacy code anyway, I hope we won't need it
at all.

Lorenzo

^ permalink raw reply

* [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id
From: Peter Chen @ 2013-01-14 12:56 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20130114110600.GI10874@arwen.pp.htv.fi>

On Mon, Jan 14, 2013 at 01:06:00PM +0200, Felipe Balbi wrote:
> On Mon, Jan 14, 2013 at 12:03:04PM +0100, Marc Kleine-Budde wrote:
> > On 01/14/2013 11:53 AM, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Mon, Jan 14, 2013 at 11:50:41AM +0100, Marc Kleine-Budde wrote:
> > >> On 01/14/2013 11:39 AM, Felipe Balbi wrote:
> > >>> On Mon, Jan 14, 2013 at 11:34:05AM +0100, Marc Kleine-Budde wrote:
> > >>>> On 01/14/2013 11:24 AM, Felipe Balbi wrote:
> > >>>>> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
> > >>>>>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
> > >>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
> > >>>>>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
> > >>>>>>>>  
> > >>>>>>>>  	return fsl_udc_resume(NULL);
> > >>>>>>>>  }
> > >>>>>>>> -
> > >>>>>>>>  /*-------------------------------------------------------------------------
> > >>>>>>>>  	Register entry point for the peripheral controller driver
> > >>>>>>>>  --------------------------------------------------------------------------*/
> > >>>>>>>> -
> > >>>>>>>> +static const struct platform_device_id fsl_udc_devtype[] = {
> > >>>>>>>> +	{
> > >>>>>>>> +		.name = "imx-udc-mx25",
> > >>>>>>>> +		.driver_data = IMX25_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name = "imx-udc-mx27",
> > >>>>>>>> +		.driver_data = IMX27_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name = "imx-udc-mx31",
> > >>>>>>>> +		.driver_data = IMX31_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name = "imx-udc-mx35",
> > >>>>>>>> +		.driver_data = IMX35_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name = "imx-udc-mx51",
> > >>>>>>>> +		.driver_data = IMX51_UDC,
> > >>>>>>>> +	}
> > >>>>>>>> +};
> > >>>>>>>
> > >>>>>>> I wonder if your driver-data is actually needed since you can use string
> > >>>>>>> comparisson to achieve the exact same outcome.
> > >>>>>>
> > >>>>>> Why use a string compare, if the kernel infrastructure already does this
> > >>>>>> for you?
> > >>>>>
> > >>>>> what do you mean ? What kernel infrastructure is doing waht for me ?
> > >>>>
> > >>>> The kernel infrastructure is doing the string compare for you to match
> > >>>> the device against the driver (via platform_device_id->name). You get
> > >>>> the a pointer to the driver_data for free. So you don't need any string
> > >>>> compare in the driver later.
> > >>>
> > >>> but current driver data is just duplicating name with an integer, it's
> > >>> pretty useless driver data.
> > >>
> > >> I don't think so - another argument:
> > >> Less code. As struct platform_device_id is a static array the space is
> > >> allocated anyway. So it doesn't make any difference if driver_data is
> > >> NULL or not. Later you just need to make an integer comparison instead
> > >> of a call to a strcmp(), if you have a switch/case and IMX*_UDC is an
> > >> enum, the compiler will warn you if you've missed an IMX variant.
> > > 
> > > fair enough, but then don't create a different enum value for each imx
> > > instance if they're mostly the same. Differentiate only what's actually
> > > different.
> > 
> > Usually there isn't any Changelog between IP cores used in the different
> > fsl processors (at least available outside of fsl), that makes it quite
> > difficult to say if something found on one imx is really the same as on
> > the other one. And they (usually) don't provide any versioning
> > information in a register or the documentation.
> > 
> > just my 2?
> 
> $SUBJECT is trying to differentiate a single feature (or maybe two) to
> replace cpu_is_xxx(), then expose that on driver_data without creating
> one enum value for each release from fsl.

Felipe, every one or two SoCs may have their special operations for
integrate PHY interface, clk operation, or workaround for IC limitation.

Maybe, it will add more future or SoCs (maybe not for this driver)
in the future, using enum is easier than string comparison for expanding something.
> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen

^ permalink raw reply

* [PATCH v3 2/3] usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap
From: Peter Chen @ 2013-01-14 12:58 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20130114101708.GC10874@arwen.pp.htv.fi>

On Mon, Jan 14, 2013 at 12:17:08PM +0200, Felipe Balbi wrote:
> On Mon, Jan 14, 2013 at 06:12:40PM +0800, Peter Chen wrote:
> > As mach/hardware.h is deleted, we can't visit platform code at driver.
> > It has no phy driver to combine with this controller, so it has to use
> > ioremap to map phy address as a workaround.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  drivers/usb/gadget/fsl_mxc_udc.c |   12 +++++++-----
> >  1 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/fsl_mxc_udc.c b/drivers/usb/gadget/fsl_mxc_udc.c
> > index 6df45f7..0e858e6 100644
> > --- a/drivers/usb/gadget/fsl_mxc_udc.c
> > +++ b/drivers/usb/gadget/fsl_mxc_udc.c
> > @@ -23,7 +23,8 @@ static struct clk *mxc_per_clk;
> >  static struct clk *mxc_ipg_clk;
> >  
> >  /* workaround ENGcm09152 for i.MX35 */
> > -#define USBPHYCTRL_OTGBASE_OFFSET	0x608
> > +#define MX35_USBPHYCTRL_OFFSET		0x600
> > +#define USBPHYCTRL_OTGBASE_OFFSET	0x8
> >  #define USBPHYCTRL_EVDO			(1 << 23)
> >  
> >  int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev)
> > @@ -83,15 +84,16 @@ void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
> >  	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
> >  	if (devtype == IMX35_UDC) {
> >  		unsigned int v;
> > +		void __iomem *phy_regs = ioremap((unsigned long)pdata->regs +
> > +						MX35_USBPHYCTRL_OFFSET, 512);
> 
> as I said before, this should be passed via struct resource, not pdata.
My careless, will change at next version
> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen

^ permalink raw reply

* [PATCH v5 02/14] ARM: Section based HYP idmap
From: Russell King - ARM Linux @ 2013-01-14 13:07 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20130114110756.GA10209@redhat.com>

On Mon, Jan 14, 2013 at 01:07:56PM +0200, Gleb Natapov wrote:
> Ah, of course. This is ident map so by definition it cannot map phys
> addresses above 4G. And since __virt_to_phys() suppose to work only on
> ident map it's OK to returns unsigned long.

Let's get this right... the lack of correct definition in these
comments needs correction.

Firstly, __virt_to_phys() is an ARM-arch private function.  Only
ARM-arch private code should be using it - it must not be used outside
that context.

Secondly, it's "public" counterpart, virt_to_phys(), and itself are
valid _only_ for what we call the "kernel direct map", which are the
addresses corresponding with the lowmem pages mapped from PAGE_OFFSET
up to the _start_ of vmalloc space.  No other mapping is valid for this.

That means that addresses in the identity mapping, if the identity
mapping is outside of the range {PAGE_OFFSET..vmalloc start}, are _not_
valid for virt_to_phys().

The same is true of their counterparts, __phys_to_virt() and
phys_to_virt().  These are _only_ valid for physical addresses
corresponding to the pages mapped in as "lowmem" and they will return
addresses for that mapping of the pages.

Both these functions are invalid when used on highmem pages.
*virt_to_phys() is invalid when used on pointers returned from
ioremap(), vmalloc(), vmap(), dma_alloc_coherent(), and any other
interface which remaps memory.

^ permalink raw reply

* [PATCH] ARM: imx: disable cpu in .cpu_kill hook
From: Shawn Guo @ 2013-01-14 13:09 UTC (permalink / raw)
  To: linux-arm-kernel

It's buggy to disable the cpu that is being hot-unplugged in .cpu_die
hook which runs on the cpu itself.  Instead, it should be done in
.cpu_kill which runs on the thread (another cpu) that asks for shutting
down the cpu.  Move imx_enable_cpu(cpu, false) call into .cpu_kill
hook, and leave the cpu to be hot-unplugged in WFI with a recovery call
cpu_leave_lowpower(), so that we can get a more stable cpu hot-plug
operation.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/mach-imx/common.h  |    1 +
 arch/arm/mach-imx/hotplug.c |   31 +++++++++++++++++++++++++++----
 arch/arm/mach-imx/platsmp.c |    1 +
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
index 7191ab4..fa36fb8 100644
--- a/arch/arm/mach-imx/common.h
+++ b/arch/arm/mach-imx/common.h
@@ -142,6 +142,7 @@ extern int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode);
 extern void imx6q_clock_map_io(void);
 
 extern void imx_cpu_die(unsigned int cpu);
+extern int imx_cpu_kill(unsigned int cpu);
 
 #ifdef CONFIG_PM
 extern void imx6q_pm_init(void);
diff --git a/arch/arm/mach-imx/hotplug.c b/arch/arm/mach-imx/hotplug.c
index 3dec962..a9b4096 100644
--- a/arch/arm/mach-imx/hotplug.c
+++ b/arch/arm/mach-imx/hotplug.c
@@ -38,6 +38,22 @@ static inline void cpu_enter_lowpower(void)
 	  : "cc");
 }
 
+static inline void cpu_leave_lowpower(void)
+{
+	unsigned int v;
+
+	asm volatile(
+		"mrc	p15, 0, %0, c1, c0, 0\n"
+	"	orr	%0, %0, %1\n"
+	"	mcr	p15, 0, %0, c1, c0, 0\n"
+	"	mrc	p15, 0, %0, c1, c0, 1\n"
+	"	orr	%0, %0, %2\n"
+	"	mcr	p15, 0, %0, c1, c0, 1\n"
+	  : "=&r" (v)
+	  : "Ir" (CR_C), "Ir" (0x40)
+	  : "cc");
+}
+
 /*
  * platform-specific code to shutdown a CPU
  *
@@ -45,10 +61,17 @@ static inline void cpu_enter_lowpower(void)
  */
 void imx_cpu_die(unsigned int cpu)
 {
+	static int spurious;
+
 	cpu_enter_lowpower();
-	imx_enable_cpu(cpu, false);
+	cpu_do_idle();
+	cpu_leave_lowpower();
 
-	/* spin here until hardware takes it down */
-	while (1)
-		;
+	pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, ++spurious);
+}
+
+int imx_cpu_kill(unsigned int cpu)
+{
+	imx_enable_cpu(cpu, false);
+	return 1;
 }
diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c
index 3777b80..66fae88 100644
--- a/arch/arm/mach-imx/platsmp.c
+++ b/arch/arm/mach-imx/platsmp.c
@@ -92,5 +92,6 @@ struct smp_operations  imx_smp_ops __initdata = {
 	.smp_boot_secondary	= imx_boot_secondary,
 #ifdef CONFIG_HOTPLUG_CPU
 	.cpu_die		= imx_cpu_die,
+	.cpu_kill		= imx_cpu_kill,
 #endif
 };
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH v3 2/3] usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap
From: Russell King - ARM Linux @ 2013-01-14 13:10 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1358158361-25550-3-git-send-email-peter.chen@freescale.com>

On Mon, Jan 14, 2013 at 06:12:40PM +0800, Peter Chen wrote:
> @@ -83,15 +84,16 @@ void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
>  	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
>  	if (devtype == IMX35_UDC) {
>  		unsigned int v;
> +		void __iomem *phy_regs = ioremap((unsigned long)pdata->regs +
> +						MX35_USBPHYCTRL_OFFSET, 512);

Consider that ioremap() can fail.

^ permalink raw reply

* [PATCH v3 2/3] usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap
From: Peter Chen @ 2013-01-14 13:16 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20130114131056.GB23505@n2100.arm.linux.org.uk>

On Mon, Jan 14, 2013 at 01:10:56PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 14, 2013 at 06:12:40PM +0800, Peter Chen wrote:
> > @@ -83,15 +84,16 @@ void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
> >  	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
> >  	if (devtype == IMX35_UDC) {
> >  		unsigned int v;
> > +		void __iomem *phy_regs = ioremap((unsigned long)pdata->regs +
> > +						MX35_USBPHYCTRL_OFFSET, 512);
> 
> Consider that ioremap() can fail.
> 
Thanks, will check NULL pointer.

-- 

Best Regards,
Peter Chen

^ permalink raw reply

* [PATCH] ARM: imx: disable cpu in .cpu_kill hook
From: Russell King - ARM Linux @ 2013-01-14 13:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1358168977-9631-1-git-send-email-shawn.guo@linaro.org>

On Mon, Jan 14, 2013 at 09:09:37PM +0800, Shawn Guo wrote:
>  void imx_cpu_die(unsigned int cpu)
>  {
> +	static int spurious;
> +
>  	cpu_enter_lowpower();
> -	imx_enable_cpu(cpu, false);
> +	cpu_do_idle();
> +	cpu_leave_lowpower();
>  
> -	/* spin here until hardware takes it down */
> -	while (1)
> -		;
> +	pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, ++spurious);

Returning from the platform cpu die implementation should only be done
in extreme situations, particularly where there's no possibility for the
CPU to actually be taken offline (in other words, the ARM development
platforms where there is _no_ possibility of powering down or resetting
the secondary CPUs individually.)

I'm actually thinking about supplementing cpu_die() in arch/arm/kernel/smp.c
with a warning:

@@ -259,6 +259,9 @@ void __ref cpu_die(void)
         */
        platform_cpu_die(cpu);

+       pr_warn("CPU%u: platform cpu_die() returned, trying to resuscitate\n",
+               cpu);
+

so that people get warned of this buggy behaviour.

^ permalink raw reply

* Question: ARM: local timers: add num_possible_cpus() in local_timer_register()
From: Steve zhan @ 2013-01-14 13:33 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Marc,

	Do you think we should add num_possible_cpus() in 
local_timer_register() function, When core number is 
> 1, then reutrn TRUE? Enable the local timer only if 
we have more than one CPU, if the current config is
SMP and setup_max_cpus >= 1, but if smp_init_cpus in 
platform code have set cpu possible value to One(that 
maybe read number from scu or other hardware), then 
percpu_timer_setup in smp_prepare_cpus have no chance
to be called.

	For local timer, because local_timer_register have
return 1, that will miss global timer register. 

	Could you pls check it?


Regards,
Steve.

^ permalink raw reply

* Question: ARM: local timers: add num_possible_cpus() in local_timer_register()
From: Russell King - ARM Linux @ 2013-01-14 13:35 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20130114133345.GA26701@android11.spreadtrum.com>

On Mon, Jan 14, 2013 at 09:33:45PM +0800, Steve zhan wrote:
> Hi Marc,
> 
> 	Do you think we should add num_possible_cpus() in 
> local_timer_register() function, When core number is 
> > 1, then reutrn TRUE? Enable the local timer only if 
> we have more than one CPU, if the current config is
> SMP and setup_max_cpus >= 1, but if smp_init_cpus in 
> platform code have set cpu possible value to One(that 
> maybe read number from scu or other hardware), then 
> percpu_timer_setup in smp_prepare_cpus have no chance
> to be called.
> 
> 	For local timer, because local_timer_register have
> return 1, that will miss global timer register. 

What about platforms where all they have are the local timers and no
global timer?

^ permalink raw reply

* [PATCH 2/4] serial: vt8500: ioremap'd resource is never freed
From: Sergei Shtylyov @ 2013-01-14 13:37 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1358129060-9399-3-git-send-email-linux@prisktech.co.nz>

Hello.

On 14-01-2013 6:04, Tony Prisk wrote:

> Memory mapped via ioremap call is never released. Rather than add an
> iounmap call, change allocation function to devm_request_and_ioremap.

> Also, change the error on failure for this call to -EBUSY rather than
> -ENOMEM.

    Not really well justified. See below.

> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> ---
>   drivers/tty/serial/vt8500_serial.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

> diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
> index 4c4a58d..8865afd 100644
> --- a/drivers/tty/serial/vt8500_serial.c
> +++ b/drivers/tty/serial/vt8500_serial.c
> @@ -615,9 +615,9 @@ static int vt8500_serial_probe(struct platform_device *pdev)
>   	snprintf(vt8500_port->name, sizeof(vt8500_port->name),
>   		 "VT8500 UART%d", pdev->id);
>
> -	vt8500_port->uart.membase = ioremap(mmres->start, resource_size(mmres));
> +	vt8500_port->uart.membase = devm_request_and_ioremap(&pdev->dev, mmres);
>   	if (!vt8500_port->uart.membase) {
> -		ret = -ENOMEM;
> +		ret = -EBUSY;

    According to the comment to devm_request_and_ioremap(), the recommended 
return code on its failure -EADDRNOTAVAIL.

WBR, Sergei

^ permalink raw reply

* [PATCH] ARM: imx: disable cpu in .cpu_kill hook
From: Shawn Guo @ 2013-01-14 13:48 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20130114133208.GC23505@n2100.arm.linux.org.uk>

On Mon, Jan 14, 2013 at 01:32:08PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 14, 2013 at 09:09:37PM +0800, Shawn Guo wrote:
> >  void imx_cpu_die(unsigned int cpu)
> >  {
> > +	static int spurious;
> > +
> >  	cpu_enter_lowpower();
> > -	imx_enable_cpu(cpu, false);
> > +	cpu_do_idle();
> > +	cpu_leave_lowpower();
> >  
> > -	/* spin here until hardware takes it down */
> > -	while (1)
> > -		;
> > +	pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, ++spurious);
> 
> Returning from the platform cpu die implementation should only be done
> in extreme situations, particularly where there's no possibility for the
> CPU to actually be taken offline (in other words, the ARM development
> platforms where there is _no_ possibility of powering down or resetting
> the secondary CPUs individually.)
> 
Indeed.  I have to admit that I have copied the code from Vexpress but
never seen it actually runs on my platform.  Will remove the recovery
completely.

Shawn

^ permalink raw reply

* [PATCH] ARM: imx: disable cpu in .cpu_kill hook
From: Rob Herring @ 2013-01-14 13:55 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1358168977-9631-1-git-send-email-shawn.guo@linaro.org>

On 01/14/2013 07:09 AM, Shawn Guo wrote:
> It's buggy to disable the cpu that is being hot-unplugged in .cpu_die
> hook which runs on the cpu itself.  Instead, it should be done in
> .cpu_kill which runs on the thread (another cpu) that asks for shutting
> down the cpu.  Move imx_enable_cpu(cpu, false) call into .cpu_kill
> hook, and leave the cpu to be hot-unplugged in WFI with a recovery call
> cpu_leave_lowpower(), so that we can get a more stable cpu hot-plug
> operation.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/mach-imx/common.h  |    1 +
>  arch/arm/mach-imx/hotplug.c |   31 +++++++++++++++++++++++++++----
>  arch/arm/mach-imx/platsmp.c |    1 +
>  3 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
> index 7191ab4..fa36fb8 100644
> --- a/arch/arm/mach-imx/common.h
> +++ b/arch/arm/mach-imx/common.h
> @@ -142,6 +142,7 @@ extern int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode);
>  extern void imx6q_clock_map_io(void);
>  
>  extern void imx_cpu_die(unsigned int cpu);
> +extern int imx_cpu_kill(unsigned int cpu);
>  
>  #ifdef CONFIG_PM
>  extern void imx6q_pm_init(void);
> diff --git a/arch/arm/mach-imx/hotplug.c b/arch/arm/mach-imx/hotplug.c
> index 3dec962..a9b4096 100644
> --- a/arch/arm/mach-imx/hotplug.c
> +++ b/arch/arm/mach-imx/hotplug.c
> @@ -38,6 +38,22 @@ static inline void cpu_enter_lowpower(void)
>  	  : "cc");
>  }
>  
> +static inline void cpu_leave_lowpower(void)
> +{
> +	unsigned int v;
> +
> +	asm volatile(
> +		"mrc	p15, 0, %0, c1, c0, 0\n"
> +	"	orr	%0, %0, %1\n"
> +	"	mcr	p15, 0, %0, c1, c0, 0\n"

Can't you do:

set_cr(get_cr() | CR_C);

> +	"	mrc	p15, 0, %0, c1, c0, 1\n"
> +	"	orr	%0, %0, %2\n"
> +	"	mcr	p15, 0, %0, c1, c0, 1\n"

Setting or clearing the SMP bit is showing up in multiple places
(big.LITTLE series, Tegra cpuidle). I did inline versions of
set_auxcr/get_auxcr for highbank cpuidle. We should be make those common.

Rob

> +	  : "=&r" (v)
> +	  : "Ir" (CR_C), "Ir" (0x40)
> +	  : "cc");
> +}
> +
>  /*
>   * platform-specific code to shutdown a CPU
>   *
> @@ -45,10 +61,17 @@ static inline void cpu_enter_lowpower(void)
>   */
>  void imx_cpu_die(unsigned int cpu)
>  {
> +	static int spurious;
> +
>  	cpu_enter_lowpower();
> -	imx_enable_cpu(cpu, false);
> +	cpu_do_idle();
> +	cpu_leave_lowpower();
>  
> -	/* spin here until hardware takes it down */
> -	while (1)
> -		;
> +	pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, ++spurious);
> +}
> +
> +int imx_cpu_kill(unsigned int cpu)
> +{
> +	imx_enable_cpu(cpu, false);
> +	return 1;
>  }
> diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c
> index 3777b80..66fae88 100644
> --- a/arch/arm/mach-imx/platsmp.c
> +++ b/arch/arm/mach-imx/platsmp.c
> @@ -92,5 +92,6 @@ struct smp_operations  imx_smp_ops __initdata = {
>  	.smp_boot_secondary	= imx_boot_secondary,
>  #ifdef CONFIG_HOTPLUG_CPU
>  	.cpu_die		= imx_cpu_die,
> +	.cpu_kill		= imx_cpu_kill,
>  #endif
>  };
> 

^ permalink raw reply

* [PATCH] ARM: imx: disable cpu in .cpu_kill hook
From: Shawn Guo @ 2013-01-14 14:00 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <50F40E58.4030808@gmail.com>

On Mon, Jan 14, 2013 at 07:55:36AM -0600, Rob Herring wrote:
> Can't you do:
> 
> set_cr(get_cr() | CR_C);
> 
Yes, this is nice.  But I'm taking Russell's suggestion to remove
the recovery piece, which is unnecessary for imx6q.

Shawn

> > +	"	mrc	p15, 0, %0, c1, c0, 1\n"
> > +	"	orr	%0, %0, %2\n"
> > +	"	mcr	p15, 0, %0, c1, c0, 1\n"
> 
> Setting or clearing the SMP bit is showing up in multiple places
> (big.LITTLE series, Tegra cpuidle). I did inline versions of
> set_auxcr/get_auxcr for highbank cpuidle. We should be make those common.
> 

^ permalink raw reply

* [PATCH 00/16] big.LITTLE low-level CPU and cluster power management
From: Nicolas Pitre @ 2013-01-14 14:05 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1358157392.19304.243.camel@jlo-ubuntu-64.nvidia.com>

On Mon, 14 Jan 2013, Joseph Lo wrote:

> Hi Nicolas,
> 
> On Thu, 2013-01-10 at 08:20 +0800, Nicolas Pitre wrote:
> > This is the initial public posting of the initial support for big.LITTLE.
> > Included here is the code required to safely power up and down CPUs in a
> > b.L system, whether this is via CPU hotplug, a cpuidle driver or the
> > Linaro b.L in-kernel switcher[*] on top of this.  Only  SMP secondary
> > boot and CPU hotplug support is included at this time.  Getting to this
> > point already represents a significcant chunk of code as illustrated by
> > the diffstat below.
> > 
> > This work was presented at Linaro Connect in Copenhagen by Dave Martin and
> > myself.  The presentation slides are available here:
> > 
> > http://www.linaro.org/documents/download/f3569407bb1fb8bde0d6da80e285b832508f92f57223c
> > 
> > The code is now stable on both Fast Models as well as Virtual Express TC2
> > and ready for public review.
> > 
> > Platform support is included for Fast Models implementing the
> > Cortex-A15x4-A7x4 and Cortex-A15x1-A7x1 configurations.  To allow
> > successful compilation, I also included a preliminary version of the
> > CCI400 driver from Lorenzo Pieralisi.
> > 
> > Support for actual hardware such as Vexpress TC2 should come later,
> > once the basic infrastructure from this series is merged.  A few DT
> > bindings are used but not yet documented.
> > 
> > This series is made of the following parts:
> > 
> > Low-level support code:
> > [PATCH 01/16] ARM: b.L: secondary kernel entry code
> > [PATCH 02/16] ARM: b.L: introduce the CPU/cluster power API
> > [PATCH 03/16] ARM: b.L: introduce helpers for platform coherency
> > [PATCH 04/16] ARM: b.L: Add baremetal voting mutexes
> > [PATCH 05/16] ARM: bL_head: vlock-based first man election
> > 
> > Adaptation layer to hook with the generic kernel infrastructure:
> > [PATCH 06/16] ARM: b.L: generic SMP secondary bringup and hotplug
> > [PATCH 07/16] ARM: bL_platsmp.c: close the kernel entry gate before
> > [PATCH 08/16] ARM: bL_platsmp.c: make sure the GIC interface of a
> > [PATCH 09/16] ARM: vexpress: Select the correct SMP operations at
> > 
> > Fast Models support:
> > [PATCH 10/16] ARM: vexpress: introduce DCSCB support
> > [PATCH 11/16] ARM: vexpress/dcscb: add CPU use counts to the power
> > [PATCH 12/16] ARM: vexpress/dcscb: do not hardcode number of CPUs
> > [PATCH 13/16] drivers: misc: add ARM CCI support
> > [PATCH 14/16] ARM: TC2: ensure powerdown-time data is flushed from
> > [PATCH 15/16] ARM: vexpress/dcscb: handle platform coherency
> > [PATCH 16/16] ARM: vexpress/dcscb: probe via device tree
> > 
> 
> Thanks for introducing this series.
> I am taking a look at this series. It introduced an algorithm for
> syncing and avoid racing when syncing the power status of clusters and
> CPUs. Do you think these codes could have a chance to become a generic
> framework?

Yes.  As I mentioned before, the bL_ prefix is implied only by the fact 
that big.LITTLE was the motivation for creating this code.

> The Tegra chip series had a similar design for CPU clusters and it 
had
> limitation that the CPU0 always needs to be the last CPU to be shut down
> before cluster power down as well. I believe it can also get benefits of
> this works. We indeed need a similar algorithm to sync CPUs power status
> before cluster power down and switching.
> 
> The "bL_entry.c", "bL_entry.S", "bL_entry.h", "vlock.h" and "vlock.S"
> looks have a chance to be a common framework for ARM platform even if it
> just support one cluster. Because some systems had the limitations for
> cluster power down. That's why the coupled cpuidle been introduced. And
> this framework could be enabled automatically if platform dependent or
> by menuconfig.

Absolutely.


> For ex,
> 	select CPUS_CLUSTERS_POWER_SYNC_FRAMEWORK if SMP && CPU_PM
> 
> How do you think of this suggestion?

I'd prefer a more concise name though.

> BTW, some questions...
> 1. The "bL_entry_point" looks like a first run function when CPUs just
> power up, then jumping to original reset vector that it should be
> called. Do you think this should be a function and be called by reset
> handler? Or in your design, this should be called as soon as possible
> when the CPU power be resumed?

This should be called as soon as possible.

> 2. Does the Last_man mechanism should implement in platform specific
> code to check something like cpu_online_status and if there is a
> limitation for the specific last CPU to be powered down?

The selection of the last man is accomplished using a platform specific 
mechanism.  By the time this has to be done, the CPU is already dead as 
far as the Linux kernel is concerned, and therefore the generic CPU map 
is not reliable.  In the DCSCB case we simply look at the hardware reset 
register being modified to directly determine the last man.  On TC2 (not 
yet posted) we have to keep a local map of online CPUs.

In your case, the selection of the last man would simply be forced on 
CPU0.


Nicolas

^ permalink raw reply

* [PATCH v2] gpio: vt8500: memory cleanup missing
From: Grant Likely @ 2013-01-14 14:14 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1357844986-21891-1-git-send-email-linux@prisktech.co.nz>

On Fri, 11 Jan 2013 08:09:46 +1300, Tony Prisk <linux@prisktech.co.nz> wrote:
> This driver is missing a .remove callback, and the fail path on
> probe is incomplete.
> 
> If an error occurs in vt8500_add_chips, gpio_base is not unmapped.
> The driver is also ignoring the return value from this function so
> if a chip fails to register it completes as successful.
> 
> Replaced pr_err with dev_err in vt8500_add_chips since the device is
> available.
> 
> There is also no .remove callback defined. To allow removing the
> registered chips, I have moved *vtchip to be a static global.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> ---
> v2:
> Remove global variable and use platform_set_drvdata instead.
> 
>  drivers/gpio/gpio-vt8500.c |   51 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-vt8500.c b/drivers/gpio/gpio-vt8500.c
> index b53320a..87e59b5 100644
> --- a/drivers/gpio/gpio-vt8500.c
> +++ b/drivers/gpio/gpio-vt8500.c
> @@ -233,10 +233,12 @@ static int vt8500_add_chips(struct platform_device *pdev, void __iomem *base,
>  			sizeof(struct vt8500_gpio_chip) * data->num_banks,
>  			GFP_KERNEL);
>  	if (!vtchip) {
> -		pr_err("%s: failed to allocate chip memory\n", __func__);
> +		dev_err(&pdev->dev, "failed to allocate chip memory\n");
>  		return -ENOMEM;
>  	}
>  
> +	platform_set_drvdata(pdev, vtchip);
> +
>  	for (i = 0; i < data->num_banks; i++) {
>  		vtchip[i].base = base;
>  		vtchip[i].regs = &data->banks[i];
> @@ -261,6 +263,7 @@ static int vt8500_add_chips(struct platform_device *pdev, void __iomem *base,
>  
>  		gpiochip_add(chip);
>  	}
> +
>  	return 0;
>  }
>  

Watch out; irrelevant whitespace change here. From a maintainer point of
voiew, I'm less confident about a patch when I see unrelated whitespace
changes because it suggests that there are things in the patch that you
don't intend. It helps me out a lot if this stuff gets scrubbed before I
see it.

> @@ -273,36 +276,64 @@ static struct of_device_id vt8500_gpio_dt_ids[] = {
>  
>  static int vt8500_gpio_probe(struct platform_device *pdev)
>  {
> +	int ret;
>  	void __iomem *gpio_base;
> -	struct device_node *np;
> +	struct device_node *np = pdev->dev.of_node;
>  	const struct of_device_id *of_id =
>  				of_match_device(vt8500_gpio_dt_ids, &pdev->dev);
>  
> -	if (!of_id) {
> -		dev_err(&pdev->dev, "Failed to find gpio controller\n");
> +	if (!np) {
> +		dev_err(&pdev->dev, "GPIO node missing in devicetree\n");
>  		return -ENODEV;
>  	}
>  
> -	np = pdev->dev.of_node;
> -	if (!np) {
> -		dev_err(&pdev->dev, "Missing GPIO description in devicetree\n");
> -		return -EFAULT;
> +	if (!of_id) {
> +		dev_err(&pdev->dev, "No matching driver data\n");
> +		return -ENODEV;
>  	}

Why is this flipped around? I don't see any functional reason for this
change.

In actual fact, since the driver needs both it only needs to test for
the of_id. If there is no node, then of_id will never be set.

>  
>  	gpio_base = of_iomap(np, 0);
>  	if (!gpio_base) {
>  		dev_err(&pdev->dev, "Unable to map GPIO registers\n");
> -		of_node_put(np);
>  		return -ENOMEM;
>  	}
>  
> -	vt8500_add_chips(pdev, gpio_base, of_id->data);
> +	ret = vt8500_add_chips(pdev, gpio_base, of_id->data);
> +	if (ret) {
> +		iounmap(gpio_base);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vt8500_gpio_remove(struct platform_device *pdev)
> +{
> +	int i;
> +	int ret;
> +	const struct vt8500_gpio_data *data;
> +	struct vt8500_gpio_chip *vtchip = platform_get_drvdata(pdev);
> +	void __iomem *gpio_base = vtchip[0].base;
> +	const struct of_device_id *of_id =
> +				of_match_device(vt8500_gpio_dt_ids, &pdev->dev);
> +
> +	data = of_id->data;
> +
> +	for (i = 0; i < data->num_banks; i++) {

It would make for simpler code all around if num_banks was cached in the
vt8500_gpio_chip structure during the .probe() routine. It looks wrong
to be calling of_match_device() in the remove hook.

Otherwise this iteration looks much better.

g.

> +		ret = gpiochip_remove(&vtchip[i].chip);
> +		if (ret)
> +			dev_warn(&pdev->dev, "gpiochip_remove returned %d\n",
> +				 ret);
> +	}
> +
> +	iounmap(gpio_base);
>  
>  	return 0;
>  }
>  
>  static struct platform_driver vt8500_gpio_driver = {
>  	.probe		= vt8500_gpio_probe,
> +	.remove		= vt8500_gpio_remove,
>  	.driver		= {
>  		.name	= "vt8500-gpio",
>  		.owner	= THIS_MODULE,
> -- 
> 1.7.9.5
> 
> --
> 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/

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

^ permalink raw reply

* [PATCH] ARM: imx: correct low-power mode setting
From: Shawn Guo @ 2013-01-14 14:15 UTC (permalink / raw)
  To: linux-arm-kernel

The hardware reset value of bit CCM_CLPCR_LPM enables WAIT mode
(WAIT_UNCLOCKED) by default.  However this is undesirable because
WAIT mode should only be enabled when there is a driver managing
ARM clock gating.  Correct the initial power mode to WAIT_CLOCKED
(disable WAIT mode).  While at it, the power mode after resuming
is also set back to WAIT_CLOCKED from STOP_POWER_OFF.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/mach-imx/clk-imx6q.c |    3 +++
 arch/arm/mach-imx/pm-imx6q.c  |    1 +
 2 files changed, 4 insertions(+)

diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index 7f2c10c..c0c4e72 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -436,6 +436,9 @@ int __init mx6q_clocks_init(void)
 	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
 		clk_prepare_enable(clk[clks_init_on[i]]);
 
+	/* Set initial power mode */
+	imx6q_set_lpm(WAIT_CLOCKED);
+
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-gpt");
 	base = of_iomap(np, 0);
 	WARN_ON(!base);
diff --git a/arch/arm/mach-imx/pm-imx6q.c b/arch/arm/mach-imx/pm-imx6q.c
index a17543d..ee42d20 100644
--- a/arch/arm/mach-imx/pm-imx6q.c
+++ b/arch/arm/mach-imx/pm-imx6q.c
@@ -41,6 +41,7 @@ static int imx6q_pm_enter(suspend_state_t state)
 		cpu_suspend(0, imx6q_suspend_finish);
 		imx_smp_prepare();
 		imx_gpc_post_resume();
+		imx6q_set_lpm(WAIT_CLOCKED);
 		break;
 	default:
 		return -EINVAL;
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH v4 0/3] Fix the Build error for fsl_mxc_udc.c
From: Peter Chen @ 2013-01-14 14:16 UTC (permalink / raw)
  To: linux-arm-kernel

Changes for v4:
- Using pdev's struct resource to do ioremap
- Add ioremap return value check

Changes for v3:
- Split the one big patch into three patches

Changes for v2:
- Add const for fsl_udc_devtype
- Do ioremap for phy address at fsl-mxc-udc

Peter Chen (3):
  usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id
  usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap
  ARM: i.MX clock: Change the connection-id for fsl-usb2-udc

 arch/arm/mach-imx/clk-imx25.c                     |    6 +-
 arch/arm/mach-imx/clk-imx27.c                     |    6 +-
 arch/arm/mach-imx/clk-imx31.c                     |    6 +-
 arch/arm/mach-imx/clk-imx35.c                     |    6 +-
 arch/arm/mach-imx/clk-imx51-imx53.c               |    6 +-
 arch/arm/mach-imx/devices/devices-common.h        |    1 +
 arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c |   15 +++---
 drivers/usb/gadget/fsl_mxc_udc.c                  |   36 ++++++++++----
 drivers/usb/gadget/fsl_udc_core.c                 |   54 ++++++++++++++-------
 drivers/usb/gadget/fsl_usb2_udc.h                 |   13 ++++--
 include/linux/fsl_devices.h                       |    8 +++
 11 files changed, 102 insertions(+), 55 deletions(-)

^ permalink raw reply

* [PATCH v4 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id
From: Peter Chen @ 2013-01-14 14:16 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1358172969-14898-1-git-send-email-peter.chen@freescale.com>

As mach/hardware.h is deleted, we need to use platform_device_id to
differentiate SoCs.

Besides we update the platform code accordingly.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 arch/arm/mach-imx/devices/devices-common.h        |    1 +
 arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c |   15 +++---
 drivers/usb/gadget/fsl_mxc_udc.c                  |   11 ++--
 drivers/usb/gadget/fsl_udc_core.c                 |   52 +++++++++++++-------
 drivers/usb/gadget/fsl_usb2_udc.h                 |   13 ++++--
 include/linux/fsl_devices.h                       |    8 +++
 6 files changed, 65 insertions(+), 35 deletions(-)

diff --git a/arch/arm/mach-imx/devices/devices-common.h b/arch/arm/mach-imx/devices/devices-common.h
index 6277baf..9bd5777 100644
--- a/arch/arm/mach-imx/devices/devices-common.h
+++ b/arch/arm/mach-imx/devices/devices-common.h
@@ -63,6 +63,7 @@ struct platform_device *__init imx_add_flexcan(
 
 #include <linux/fsl_devices.h>
 struct imx_fsl_usb2_udc_data {
+	const char *devid;
 	resource_size_t iobase;
 	resource_size_t irq;
 };
diff --git a/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c b/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c
index 37e4439..fb527c7 100644
--- a/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c
+++ b/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c
@@ -11,35 +11,36 @@
 #include "../hardware.h"
 #include "devices-common.h"
 
-#define imx_fsl_usb2_udc_data_entry_single(soc)				\
+#define imx_fsl_usb2_udc_data_entry_single(soc, _devid)			\
 	{								\
+		.devid = _devid,					\
 		.iobase = soc ## _USB_OTG_BASE_ADDR,			\
 		.irq = soc ## _INT_USB_OTG,				\
 	}
 
 #ifdef CONFIG_SOC_IMX25
 const struct imx_fsl_usb2_udc_data imx25_fsl_usb2_udc_data __initconst =
-	imx_fsl_usb2_udc_data_entry_single(MX25);
+	imx_fsl_usb2_udc_data_entry_single(MX25, "imx-udc-mx25");
 #endif /* ifdef CONFIG_SOC_IMX25 */
 
 #ifdef CONFIG_SOC_IMX27
 const struct imx_fsl_usb2_udc_data imx27_fsl_usb2_udc_data __initconst =
-	imx_fsl_usb2_udc_data_entry_single(MX27);
+	imx_fsl_usb2_udc_data_entry_single(MX27, "imx-udc-mx27");
 #endif /* ifdef CONFIG_SOC_IMX27 */
 
 #ifdef CONFIG_SOC_IMX31
 const struct imx_fsl_usb2_udc_data imx31_fsl_usb2_udc_data __initconst =
-	imx_fsl_usb2_udc_data_entry_single(MX31);
+	imx_fsl_usb2_udc_data_entry_single(MX31, "imx-udc-mx31");
 #endif /* ifdef CONFIG_SOC_IMX31 */
 
 #ifdef CONFIG_SOC_IMX35
 const struct imx_fsl_usb2_udc_data imx35_fsl_usb2_udc_data __initconst =
-	imx_fsl_usb2_udc_data_entry_single(MX35);
+	imx_fsl_usb2_udc_data_entry_single(MX35, "imx-udc-mx35");
 #endif /* ifdef CONFIG_SOC_IMX35 */
 
 #ifdef CONFIG_SOC_IMX51
 const struct imx_fsl_usb2_udc_data imx51_fsl_usb2_udc_data __initconst =
-	imx_fsl_usb2_udc_data_entry_single(MX51);
+	imx_fsl_usb2_udc_data_entry_single(MX51, "imx-udc-mx51");
 #endif
 
 struct platform_device *__init imx_add_fsl_usb2_udc(
@@ -57,7 +58,7 @@ struct platform_device *__init imx_add_fsl_usb2_udc(
 			.flags = IORESOURCE_IRQ,
 		},
 	};
-	return imx_add_platform_device_dmamask("fsl-usb2-udc", -1,
+	return imx_add_platform_device_dmamask(data->devid, -1,
 			res, ARRAY_SIZE(res),
 			pdata, sizeof(*pdata), DMA_BIT_MASK(32));
 }
diff --git a/drivers/usb/gadget/fsl_mxc_udc.c b/drivers/usb/gadget/fsl_mxc_udc.c
index 1b0f086..6df45f7 100644
--- a/drivers/usb/gadget/fsl_mxc_udc.c
+++ b/drivers/usb/gadget/fsl_mxc_udc.c
@@ -18,8 +18,6 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 
-#include <mach/hardware.h>
-
 static struct clk *mxc_ahb_clk;
 static struct clk *mxc_per_clk;
 static struct clk *mxc_ipg_clk;
@@ -28,7 +26,7 @@ static struct clk *mxc_ipg_clk;
 #define USBPHYCTRL_OTGBASE_OFFSET	0x608
 #define USBPHYCTRL_EVDO			(1 << 23)
 
-int fsl_udc_clk_init(struct platform_device *pdev)
+int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev)
 {
 	struct fsl_usb2_platform_data *pdata;
 	unsigned long freq;
@@ -59,7 +57,7 @@ int fsl_udc_clk_init(struct platform_device *pdev)
 	clk_prepare_enable(mxc_per_clk);
 
 	/* make sure USB_CLK is running@60 MHz +/- 1000 Hz */
-	if (!cpu_is_mx51()) {
+	if (!(devtype == IMX51_UDC)) {
 		freq = clk_get_rate(mxc_per_clk);
 		if (pdata->phy_mode != FSL_USB2_PHY_ULPI &&
 		    (freq < 59999000 || freq > 60001000)) {
@@ -79,10 +77,11 @@ eclkrate:
 	return ret;
 }
 
-void fsl_udc_clk_finalize(struct platform_device *pdev)
+void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
+	struct platform_device *pdev)
 {
 	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
-	if (cpu_is_mx35()) {
+	if (devtype == IMX35_UDC) {
 		unsigned int v;
 
 		/* workaround ENGcm09152 for i.MX35 */
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index c19f7f1..c32119b 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -41,6 +41,7 @@
 #include <linux/fsl_devices.h>
 #include <linux/dmapool.h>
 #include <linux/delay.h>
+#include <linux/of_device.h>
 
 #include <asm/byteorder.h>
 #include <asm/io.h>
@@ -2438,17 +2439,13 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
 	unsigned int i;
 	u32 dccparams;
 
-	if (strcmp(pdev->name, driver_name)) {
-		VDBG("Wrong device");
-		return -ENODEV;
-	}
-
 	udc_controller = kzalloc(sizeof(struct fsl_udc), GFP_KERNEL);
 	if (udc_controller == NULL) {
 		ERR("malloc udc failed\n");
 		return -ENOMEM;
 	}
 
+	udc_controller->devtype = pdev->id_entry->driver_data;
 	pdata = pdev->dev.platform_data;
 	udc_controller->pdata = pdata;
 	spin_lock_init(&udc_controller->lock);
@@ -2505,7 +2502,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
 #endif
 
 	/* Initialize USB clocks */
-	ret = fsl_udc_clk_init(pdev);
+	ret = fsl_udc_clk_init(udc_controller->devtype, pdev);
 	if (ret < 0)
 		goto err_iounmap_noclk;
 
@@ -2547,7 +2544,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
 		dr_controller_setup(udc_controller);
 	}
 
-	fsl_udc_clk_finalize(pdev);
+	fsl_udc_clk_finalize(udc_controller->devtype, pdev);
 
 	/* Setup gadget structure */
 	udc_controller->gadget.ops = &fsl_gadget_ops;
@@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
 
 	return fsl_udc_resume(NULL);
 }
-
 /*-------------------------------------------------------------------------
 	Register entry point for the peripheral controller driver
 --------------------------------------------------------------------------*/
-
+static const struct platform_device_id fsl_udc_devtype[] = {
+	{
+		.name = "imx-udc-mx25",
+		.driver_data = IMX25_UDC,
+	}, {
+		.name = "imx-udc-mx27",
+		.driver_data = IMX27_UDC,
+	}, {
+		.name = "imx-udc-mx31",
+		.driver_data = IMX31_UDC,
+	}, {
+		.name = "imx-udc-mx35",
+		.driver_data = IMX35_UDC,
+	}, {
+		.name = "imx-udc-mx51",
+		.driver_data = IMX51_UDC,
+	}
+};
+MODULE_DEVICE_TABLE(platform, fsl_udc_devtype);
 static struct platform_driver udc_driver = {
-	.remove  = __exit_p(fsl_udc_remove),
+	.remove		= __exit_p(fsl_udc_remove),
+	/* Just for FSL i.mx SoC currently */
+	.id_table	= fsl_udc_devtype,
 	/* these suspend and resume are not usb suspend and resume */
-	.suspend = fsl_udc_suspend,
-	.resume  = fsl_udc_resume,
-	.driver  = {
-		.name = (char *)driver_name,
-		.owner = THIS_MODULE,
-		/* udc suspend/resume called from OTG driver */
-		.suspend = fsl_udc_otg_suspend,
-		.resume  = fsl_udc_otg_resume,
+	.suspend	= fsl_udc_suspend,
+	.resume		= fsl_udc_resume,
+	.driver		= {
+			.name = (char *)driver_name,
+			.owner = THIS_MODULE,
+			/* udc suspend/resume called from OTG driver */
+			.suspend = fsl_udc_otg_suspend,
+			.resume  = fsl_udc_otg_resume,
 	},
 };
 
diff --git a/drivers/usb/gadget/fsl_usb2_udc.h b/drivers/usb/gadget/fsl_usb2_udc.h
index f61a967..bc1f6d0 100644
--- a/drivers/usb/gadget/fsl_usb2_udc.h
+++ b/drivers/usb/gadget/fsl_usb2_udc.h
@@ -505,6 +505,8 @@ struct fsl_udc {
 	u32 ep0_dir;		/* Endpoint zero direction: can be
 				   USB_DIR_IN or USB_DIR_OUT */
 	u8 device_address;	/* Device USB address */
+	/* devtype for kinds of SoC, only i.mx uses it now */
+	enum fsl_udc_type devtype;
 };
 
 /*-------------------------------------------------------------------------*/
@@ -591,15 +593,18 @@ static inline struct ep_queue_head *get_qh_by_ep(struct fsl_ep *ep)
 
 struct platform_device;
 #ifdef CONFIG_ARCH_MXC
-int fsl_udc_clk_init(struct platform_device *pdev);
-void fsl_udc_clk_finalize(struct platform_device *pdev);
+int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev);
+void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
+		struct platform_device *pdev);
 void fsl_udc_clk_release(void);
 #else
-static inline int fsl_udc_clk_init(struct platform_device *pdev)
+static inline int fsl_udc_clk_init(enum fsl_udc_type devtype,
+		struct platform_device *pdev)
 {
 	return 0;
 }
-static inline void fsl_udc_clk_finalize(struct platform_device *pdev)
+static inline void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
+		struct platform_device *pdev)
 {
 }
 static inline void fsl_udc_clk_release(void)
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index a82296a..7cb3fe0 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -66,6 +66,14 @@ enum fsl_usb2_phy_modes {
 	FSL_USB2_PHY_SERIAL,
 };
 
+enum fsl_udc_type {
+	IMX25_UDC,
+	IMX27_UDC,
+	IMX31_UDC,
+	IMX35_UDC,
+	IMX51_UDC,
+};
+
 struct clk;
 struct platform_device;
 
-- 
1.7.0.4

^ permalink raw reply related

* [PATCH v4 2/3] usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap
From: Peter Chen @ 2013-01-14 14:16 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1358172969-14898-1-git-send-email-peter.chen@freescale.com>

As mach/hardware.h is deleted, we can't visit platform code at driver.
It has no phy driver to combine with this controller, so it has to use
ioremap to map phy address as a workaround.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/gadget/fsl_mxc_udc.c  |   27 +++++++++++++++++++++------
 drivers/usb/gadget/fsl_udc_core.c |    4 +++-
 drivers/usb/gadget/fsl_usb2_udc.h |    4 ++--
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/fsl_mxc_udc.c b/drivers/usb/gadget/fsl_mxc_udc.c
index 6df45f7..e505d60 100644
--- a/drivers/usb/gadget/fsl_mxc_udc.c
+++ b/drivers/usb/gadget/fsl_mxc_udc.c
@@ -23,7 +23,8 @@ static struct clk *mxc_per_clk;
 static struct clk *mxc_ipg_clk;
 
 /* workaround ENGcm09152 for i.MX35 */
-#define USBPHYCTRL_OTGBASE_OFFSET	0x608
+#define MX35_USBPHYCTRL_OFFSET		0x600
+#define USBPHYCTRL_OTGBASE_OFFSET	0x8
 #define USBPHYCTRL_EVDO			(1 << 23)
 
 int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev)
@@ -77,28 +78,42 @@ eclkrate:
 	return ret;
 }
 
-void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
+int fsl_udc_clk_finalize(enum fsl_udc_type devtype,
 	struct platform_device *pdev)
 {
 	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
+	int ret = 0;
+
 	if (devtype == IMX35_UDC) {
 		unsigned int v;
+		struct resource *res = platform_get_resource
+			(pdev, IORESOURCE_MEM, 0);
+		void __iomem *phy_regs = ioremap(res->start +
+						MX35_USBPHYCTRL_OFFSET, 512);
+		if (!phy_regs) {
+			dev_err(&pdev->dev, "ioremap for phy address fails\n");
+			ret = -EINVAL;
+			goto ioremap_err;
+		}
 
 		/* workaround ENGcm09152 for i.MX35 */
 		if (pdata->workaround & FLS_USB2_WORKAROUND_ENGCM09152) {
-			v = readl(MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
-					USBPHYCTRL_OTGBASE_OFFSET));
+			v = readl(phy_regs + USBPHYCTRL_OTGBASE_OFFSET);
 			writel(v | USBPHYCTRL_EVDO,
-				MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
-					USBPHYCTRL_OTGBASE_OFFSET));
+				phy_regs + USBPHYCTRL_OTGBASE_OFFSET);
 		}
+
+		iounmap(phy_regs);
 	}
 
+ioremap_err:
 	/* ULPI transceivers don't need usbpll */
 	if (pdata->phy_mode == FSL_USB2_PHY_ULPI) {
 		clk_disable_unprepare(mxc_per_clk);
 		mxc_per_clk = NULL;
 	}
+
+	return ret;
 }
 
 void fsl_udc_clk_release(void)
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index c32119b..4391d49 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -2544,7 +2544,9 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
 		dr_controller_setup(udc_controller);
 	}
 
-	fsl_udc_clk_finalize(udc_controller->devtype, pdev);
+	ret = fsl_udc_clk_finalize(udc_controller->devtype, pdev);
+	if (ret)
+		goto err_free_irq;
 
 	/* Setup gadget structure */
 	udc_controller->gadget.ops = &fsl_gadget_ops;
diff --git a/drivers/usb/gadget/fsl_usb2_udc.h b/drivers/usb/gadget/fsl_usb2_udc.h
index bc1f6d0..7ead5f7 100644
--- a/drivers/usb/gadget/fsl_usb2_udc.h
+++ b/drivers/usb/gadget/fsl_usb2_udc.h
@@ -594,7 +594,7 @@ static inline struct ep_queue_head *get_qh_by_ep(struct fsl_ep *ep)
 struct platform_device;
 #ifdef CONFIG_ARCH_MXC
 int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev);
-void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
+int fsl_udc_clk_finalize(enum fsl_udc_type devtype,
 		struct platform_device *pdev);
 void fsl_udc_clk_release(void);
 #else
@@ -603,7 +603,7 @@ static inline int fsl_udc_clk_init(enum fsl_udc_type devtype,
 {
 	return 0;
 }
-static inline void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
+static inline int fsl_udc_clk_finalize(enum fsl_udc_type devtype,
 		struct platform_device *pdev)
 {
 }
-- 
1.7.0.4

^ permalink raw reply related

* [PATCH v4 3/3] ARM: i.MX clock: Change the connection-id for fsl-usb2-udc
From: Peter Chen @ 2013-01-14 14:16 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1358172969-14898-1-git-send-email-peter.chen@freescale.com>

As we use platform_device_id for fsl-usb2-udc driver, it needs to
change clk connection-id, or the related devm_clk_get will be failed.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 arch/arm/mach-imx/clk-imx25.c       |    6 +++---
 arch/arm/mach-imx/clk-imx27.c       |    6 +++---
 arch/arm/mach-imx/clk-imx31.c       |    6 +++---
 arch/arm/mach-imx/clk-imx35.c       |    6 +++---
 arch/arm/mach-imx/clk-imx51-imx53.c |    6 +++---
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-imx/clk-imx25.c b/arch/arm/mach-imx/clk-imx25.c
index b197aa7..67e353d 100644
--- a/arch/arm/mach-imx/clk-imx25.c
+++ b/arch/arm/mach-imx/clk-imx25.c
@@ -254,9 +254,9 @@ int __init mx25_clocks_init(void)
 	clk_register_clkdev(clk[ipg], "ipg", "mxc-ehci.2");
 	clk_register_clkdev(clk[usbotg_ahb], "ahb", "mxc-ehci.2");
 	clk_register_clkdev(clk[usb_div], "per", "mxc-ehci.2");
-	clk_register_clkdev(clk[ipg], "ipg", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usbotg_ahb], "ahb", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
+	clk_register_clkdev(clk[ipg], "ipg", "imx-udc-mx25");
+	clk_register_clkdev(clk[usbotg_ahb], "ahb", "imx-udc-mx25");
+	clk_register_clkdev(clk[usb_div], "per", "imx-udc-mx25");
 	clk_register_clkdev(clk[nfc_ipg_per], NULL, "imx25-nand.0");
 	/* i.mx25 has the i.mx35 type cspi */
 	clk_register_clkdev(clk[cspi1_ipg], NULL, "imx35-cspi.0");
diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c
index 4c1d1e4..1ffe3b5 100644
--- a/arch/arm/mach-imx/clk-imx27.c
+++ b/arch/arm/mach-imx/clk-imx27.c
@@ -236,9 +236,9 @@ int __init mx27_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[lcdc_ahb_gate], "ahb", "imx21-fb.0");
 	clk_register_clkdev(clk[csi_ahb_gate], "ahb", "imx27-camera.0");
 	clk_register_clkdev(clk[per4_gate], "per", "imx27-camera.0");
-	clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usb_ipg_gate], "ipg", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usb_ahb_gate], "ahb", "fsl-usb2-udc");
+	clk_register_clkdev(clk[usb_div], "per", "imx-udc-mx27");
+	clk_register_clkdev(clk[usb_ipg_gate], "ipg", "imx-udc-mx27");
+	clk_register_clkdev(clk[usb_ahb_gate], "ahb", "imx-udc-mx27");
 	clk_register_clkdev(clk[usb_div], "per", "mxc-ehci.0");
 	clk_register_clkdev(clk[usb_ipg_gate], "ipg", "mxc-ehci.0");
 	clk_register_clkdev(clk[usb_ahb_gate], "ahb", "mxc-ehci.0");
diff --git a/arch/arm/mach-imx/clk-imx31.c b/arch/arm/mach-imx/clk-imx31.c
index 8be64e0..ef66eaf 100644
--- a/arch/arm/mach-imx/clk-imx31.c
+++ b/arch/arm/mach-imx/clk-imx31.c
@@ -139,9 +139,9 @@ int __init mx31_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[usb_div_post], "per", "mxc-ehci.2");
 	clk_register_clkdev(clk[usb_gate], "ahb", "mxc-ehci.2");
 	clk_register_clkdev(clk[ipg], "ipg", "mxc-ehci.2");
-	clk_register_clkdev(clk[usb_div_post], "per", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usb_gate], "ahb", "fsl-usb2-udc");
-	clk_register_clkdev(clk[ipg], "ipg", "fsl-usb2-udc");
+	clk_register_clkdev(clk[usb_div_post], "per", "imx-udc-mx31");
+	clk_register_clkdev(clk[usb_gate], "ahb", "imx-udc-mx31");
+	clk_register_clkdev(clk[ipg], "ipg", "imx-udc-mx31");
 	clk_register_clkdev(clk[csi_gate], NULL, "mx3-camera.0");
 	/* i.mx31 has the i.mx21 type uart */
 	clk_register_clkdev(clk[uart1_gate], "per", "imx21-uart.0");
diff --git a/arch/arm/mach-imx/clk-imx35.c b/arch/arm/mach-imx/clk-imx35.c
index 66f3d65..69fe9c8 100644
--- a/arch/arm/mach-imx/clk-imx35.c
+++ b/arch/arm/mach-imx/clk-imx35.c
@@ -251,9 +251,9 @@ int __init mx35_clocks_init()
 	clk_register_clkdev(clk[usb_div], "per", "mxc-ehci.2");
 	clk_register_clkdev(clk[ipg], "ipg", "mxc-ehci.2");
 	clk_register_clkdev(clk[usbotg_gate], "ahb", "mxc-ehci.2");
-	clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
-	clk_register_clkdev(clk[ipg], "ipg", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usbotg_gate], "ahb", "fsl-usb2-udc");
+	clk_register_clkdev(clk[usb_div], "per", "imx-udc-mx35");
+	clk_register_clkdev(clk[ipg], "ipg", "imx-udc-mx35");
+	clk_register_clkdev(clk[usbotg_gate], "ahb", "imx-udc-mx35");
 	clk_register_clkdev(clk[wdog_gate], NULL, "imx2-wdt.0");
 	clk_register_clkdev(clk[nfc_div], NULL, "imx25-nand.0");
 	clk_register_clkdev(clk[csi_gate], NULL, "mx3-camera.0");
diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c
index 579023f..fb7cb84 100644
--- a/arch/arm/mach-imx/clk-imx51-imx53.c
+++ b/arch/arm/mach-imx/clk-imx51-imx53.c
@@ -269,9 +269,9 @@ static void __init mx5_clocks_common_init(unsigned long rate_ckil,
 	clk_register_clkdev(clk[usboh3_per_gate], "per", "mxc-ehci.2");
 	clk_register_clkdev(clk[usboh3_gate], "ipg", "mxc-ehci.2");
 	clk_register_clkdev(clk[usboh3_gate], "ahb", "mxc-ehci.2");
-	clk_register_clkdev(clk[usboh3_per_gate], "per", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usboh3_gate], "ipg", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usboh3_gate], "ahb", "fsl-usb2-udc");
+	clk_register_clkdev(clk[usboh3_per_gate], "per", "imx-udc-mx51");
+	clk_register_clkdev(clk[usboh3_gate], "ipg", "imx-udc-mx51");
+	clk_register_clkdev(clk[usboh3_gate], "ahb", "imx-udc-mx51");
 	clk_register_clkdev(clk[nfc_gate], NULL, "imx51-nand");
 	clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "imx-ssi.0");
 	clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "imx-ssi.1");
-- 
1.7.0.4

^ permalink raw reply related

* [PATCHv2 1/4] clockevents: Add generic timer broadcast receiver
From: Thomas Gleixner @ 2013-01-14 14:17 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20130114121247.GD7990@e106331-lin.cambridge.arm.com>

On Mon, 14 Jan 2013, Mark Rutland wrote:
> On Mon, Jan 14, 2013 at 11:50:55AM +0000, Thomas Gleixner wrote:
> > On Mon, 14 Jan 2013, Mark Rutland wrote:
> > 
> > > On Mon, Jan 14, 2013 at 11:06:31AM +0000, Thomas Gleixner wrote:
> > > > On Wed, 9 Jan 2013, Mark Rutland wrote:
> > > > > +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> > > > > +extern int tick_receive_broadcast(void);
> > > > > +#else
> > > > > +static inline int tick_receive_broadcast(void)
> > > > > +{
> > > > > +	return 0;
> > > > > +}
> > > > 
> > > > What's the inline function for? If an arch does not have broadcasting
> > > > support it should not have a receive broadcast function call either.
> > > 
> > > That was how this was originally structured [1], but Santosh suggested this
> > > would break the build for !GENERIC_CLOCKEVENTS_BROADCAST [1]. It means that the
> > > arch-specific receive path (i.e. IPI handler) doesn't have to be #ifdef'd,
> > > which makes it less ugly.
> > 
> > Hmm. If you want to keep the IPI around unconditionally the inline
> > makes some sense, though the question is whether keeping an unused IPI
> > around makes sense in the first place. I'd rather see a warning that
> > an unexpected IPI happened than a silent inline function being called.
> 
> How about I add a warning (e.g. "Impossible timer broadcast received.") and
> return -EOPNOTSUPP when !GENERIC_CLOCKEVENTS_BROADCAST?

You still need to do something with the return value in the arch IPI
code, right?

> > > > Is anything going to use the return value?
> > > 
> > > I'd added this after looking at the x86 lapic timers, where interrupts might
> > > remain pending over a kexec, and lapic interrupts come up before timers are
> > > registered. The return value is useful for shutting down the timer in that case
> > > (see x86's local_apic_timer_interrupt).
> > 
> > Right, though then you need to check for evt->event_handler as well.
> 
> I thought this previously also [1], but I couldn't find any path such that a
> tick_cpu_device would have an evtdev without an event_handler. We always set the
> handler before setting evtdev, and alway wipe evtdev before wiping the handler.
> 
> Have I missed something?

That's an x86 specific issue. Though we could try and make that
functionality completely generic.

Thanks,

	tglx

^ permalink raw reply

* Question: ARM: local timers: add num_possible_cpus() in local_timer_register()
From: Marc Zyngier @ 2013-01-14 14:19 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20130114133345.GA26701@android11.spreadtrum.com>

On 14/01/13 13:33, Steve zhan wrote:

Hi Steve,

> 	Do you think we should add num_possible_cpus() in 
> local_timer_register() function, When core number is 
>> 1, then reutrn TRUE? Enable the local timer only if 
> we have more than one CPU, if the current config is
> SMP and setup_max_cpus >= 1, but if smp_init_cpus in 
> platform code have set cpu possible value to One(that 
> maybe read number from scu or other hardware), then 
> percpu_timer_setup in smp_prepare_cpus have no chance
> to be called.
> 
> 	For local timer, because local_timer_register have
> return 1, that will miss global timer register. 
> 
> 	Could you pls check it?

I'm not sure I understand your question.

We only enable the use of the SMP local timers infrastructure if we're
SMP and we have more that one CPU. If you only have one CPU up and
running, then you can still use your CPU local timer as a global timer.

What would we gain by using the local timer infrastructure when we're
not running on SMP hardware?

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply

* [PATCH v2] ARM: imx: disable cpu in .cpu_kill hook
From: Shawn Guo @ 2013-01-14 14:23 UTC (permalink / raw)
  To: linux-arm-kernel

It's buggy to disable the cpu that is being hot-unplugged in .cpu_die
hook which runs on the cpu itself.  Instead, it should be done in
.cpu_kill which runs on the thread (another cpu) that asks for shutting
down the cpu.  Move imx_enable_cpu(cpu, false) call into .cpu_kill
hook, and leave the cpu to be hot-unplugged in WFI within .cpu_die,
so that we can get a more stable cpu hot-plug operation.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/mach-imx/common.h  |    1 +
 arch/arm/mach-imx/hotplug.c |   10 ++++++----
 arch/arm/mach-imx/platsmp.c |    1 +
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
index 7191ab4..fa36fb8 100644
--- a/arch/arm/mach-imx/common.h
+++ b/arch/arm/mach-imx/common.h
@@ -142,6 +142,7 @@ extern int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode);
 extern void imx6q_clock_map_io(void);
 
 extern void imx_cpu_die(unsigned int cpu);
+extern int imx_cpu_kill(unsigned int cpu);
 
 #ifdef CONFIG_PM
 extern void imx6q_pm_init(void);
diff --git a/arch/arm/mach-imx/hotplug.c b/arch/arm/mach-imx/hotplug.c
index 3dec962..7bc5fe1 100644
--- a/arch/arm/mach-imx/hotplug.c
+++ b/arch/arm/mach-imx/hotplug.c
@@ -46,9 +46,11 @@ static inline void cpu_enter_lowpower(void)
 void imx_cpu_die(unsigned int cpu)
 {
 	cpu_enter_lowpower();
-	imx_enable_cpu(cpu, false);
+	cpu_do_idle();
+}
 
-	/* spin here until hardware takes it down */
-	while (1)
-		;
+int imx_cpu_kill(unsigned int cpu)
+{
+	imx_enable_cpu(cpu, false);
+	return 1;
 }
diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c
index 3777b80..66fae88 100644
--- a/arch/arm/mach-imx/platsmp.c
+++ b/arch/arm/mach-imx/platsmp.c
@@ -92,5 +92,6 @@ struct smp_operations  imx_smp_ops __initdata = {
 	.smp_boot_secondary	= imx_boot_secondary,
 #ifdef CONFIG_HOTPLUG_CPU
 	.cpu_die		= imx_cpu_die,
+	.cpu_kill		= imx_cpu_kill,
 #endif
 };
-- 
1.7.9.5

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox