Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

2015-11-20 Thread Andy Shevchenko 
On Fri, Nov 20, 2015 at 12:58 PM, Arnd Bergmann  wrote:
> On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote:
>> On 11/19/2015 01:25 PM, Arnd Bergmann wrote:

> Another idea would be to remove the filter function from struct dma_chan_map
> and pass the map through platform data

Why not unified device properties?

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

2015-11-20 Thread Peter Ujfalusi 
On 11/20/2015 02:24 PM, Andy Shevchenko wrote:
> On Fri, Nov 20, 2015 at 12:58 PM, Arnd Bergmann  wrote:
>> On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote:
>>> On 11/19/2015 01:25 PM, Arnd Bergmann wrote:
> 
>> Another idea would be to remove the filter function from struct dma_chan_map
>> and pass the map through platform data
> 
> Why not unified device properties?

Is this some Windows/ACPI feature? Quick search gives mostly MSDN and
Windows10 related links.

We only need dma_chan_map for platforms which has not been converted to DT and
still using legacy boot. Or platforms which can still boot in legacy mode. In
DT/ACPI mode we do not need this map at all.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] extcon: palmas: add support for using VBUSDET output

2015-11-20 Thread Chanwoo Choi 
Hi Felipe,

On Fri, Nov 20, 2015 at 11:37 PM, Felipe Balbi  wrote:
>
> Hi Chanwoo,
>
> Chanwoo Choi  writes:
>> Hi Felipe,
>>
>> On 2015년 11월 20일 14:33, Chanwoo Choi wrote:
>>> Hi Felipe,
>>>
>>> Looks good to me. But I have one comment.
>>>
>>> On 2015년 11월 13일 02:57, Felipe Balbi wrote:
 TPS659038 can remux its GPIO_1 as VBUSDET output,
 which can be tied to a SoC GPIO and used as a VBUS
 interrupt.

 Beagle X15 uses that, in fact, and without it, I
 could not get USB peripheral working with that
 board.

 Signed-off-by: Felipe Balbi 
 ---
  drivers/extcon/extcon-palmas.c | 22 --
  1 file changed, 20 insertions(+), 2 deletions(-)

 diff --git a/drivers/extcon/extcon-palmas.c 
 b/drivers/extcon/extcon-palmas.c
 index 93c30a885740..7985d092c069 100644
 --- a/drivers/extcon/extcon-palmas.c
 +++ b/drivers/extcon/extcon-palmas.c
 @@ -296,10 +296,28 @@ static int palmas_usb_probe(struct platform_device 
 *pdev)
 }

 if (palmas_usb->enable_vbus_detection) {
 +   int irq = platform_get_irq(pdev, 0);
 +
 +   if (irq > 0) {
 +   /* remux GPIO_1 as VBUSDET */
 +   status = palmas_update_bits(palmas, 
 PALMAS_PU_PD_OD_BASE,
 +   PALMAS_PRIMARY_SECONDARY_PAD1,
 +   
 PALMAS_PRIMARY_SECONDARY_PAD1_GPIO_1_MASK,
 +   (1 << 3));
>>>
>>> PALMAS_PRIMARY_SECONDARY_PAD1_GPIO_1_SHIFT is appropriate instead of
>>> using '3'.
>
> good point :-)
>
 +   if (status < 0) {
 +   dev_err(>dev, "can't remux GPIO1\n");
 +   return status;
 +   }
 +
 +   palmas_usb->vbus_irq = irq;
 +   } else {
 +   irq = regmap_irq_get_virq(palmas->irq_data,
 +   PALMAS_VBUS_IRQ);
 +   palmas_usb->vbus_irq = irq;
 +   }
 +
 palmas_usb->vbus_otg_irq = 
 regmap_irq_get_virq(palmas->irq_data,
PALMAS_VBUS_OTG_IRQ);
 -   palmas_usb->vbus_irq = regmap_irq_get_virq(palmas->irq_data,
 -  PALMAS_VBUS_IRQ);
 status = devm_request_threaded_irq(palmas_usb->dev,
 palmas_usb->vbus_irq, NULL,
 palmas_vbus_irq_handler,

>>>
>>> Thanks,
>>> Chanwoo Choi
>>> --
>>> 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/
>>>
>>
>> If you are OK about following patch, I'll apply it on extcon branch.
>
> that's perfect, thanks for fixing it :-)

Applied it.

Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

2015-11-20 Thread Arnd Bergmann 
On Friday 20 November 2015 14:52:03 Peter Ujfalusi wrote:
> 
> >> For legacy the filter function is pretty much needed to handle the 
> >> differences
> >> between the platforms as not all of them does the filtering in a same way. 
> >> So
> >> the first type of map would be feasible IMHO.
> > 
> > It certainly makes the transition to a map table much easier.
> 
> And the aim anyway is to convert everything to DT, right?

We won't be able to do that. Some architectures (avr32 and sh for instance)
use the dmaengine API but will likely never support DT. On ARM, at
least sa1100 is in the same category, probably also ep93xx and portions
of pxa, omap1 and davinci.

> > int dmam_register_platform_map(struct device *dev, dma_filter_fn filter, 
> > struct dma_chan_map *map)
> > {
> >   struct dma_map_list *list = kmalloc(sizeof(*list), GFP_KERNEL);
> > 
> >   if (!list)
> >   return -ENOMEM;
> > 
> >   list->dev = dev;
> >   list->filter = filter;
> >   list->map = map;
> > 
> >   mutex_lock(_map_mutex);
> >   list_add(_map_list, >node);
> >   mutex_unlock(_map_mutex);
> > }
> > 
> > Now we can completely remove the dependency on the filter function 
> > definition
> > from platform code and slave drivers.
> 
> Sounds feasible for OMAP and daVinci and for others as well. I think 
> I would go with this if someone asks my opinion 

Ok.

> The core change to add the new API + the dma_map support should be pretty
> straight forward. It can live alongside with the old API and we can phase out
> the users of the old one.
> The legacy support would need more time since we need to modify the arch codes
> and the corresponding DMA drivers to get the map registered, but after that
> the remaining drivers can be converted to use the new API.

Right. It's not urgent and as long as we agree on the overall approach, we can
always do the platform support first and wait for the following merge window
before moving over the slave drivers.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

2015-11-20 Thread Grygorii Strashko 
On -RT, TI DRA7 PCIe driver always produces below backtrace when the
first PCI interrupt is triggered:

[ cut here ]
WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 
handle_irq_event_percpu+0x14c/0x174()
irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts
Modules linked in: ahci_platform(+) ti_vip(+) libahci_platform xhci_pci(+) 
c_can_platform(+)
libahci xhci_hcd ti_vpe c_can libata v4l2_mem2mem can_dev gpio_keys leds_gpio
snd_soc_simple_card usbcore spi_ti_qspi videobuf2_core extcon_usb_gpio omap_wdt
ti_vpdma videobuf2_dma_contig ti_soc_thermal videobuf2_memops dwc3_omap extcon
rtc_omap ov1063x v4l2_common videodev media snd_soc_tlv320aic3x omap_rng 
rng_core omap_remoteproc
CPU: 1 PID: 82 Comm: irq/26-dra7-pci Not tainted 
4.1.10-rt10-02638-g6486d7e-dirty #1
Hardware name: Generic DRA74X (Flattened Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
 r7:c07acca0 r6: r5:c09034e4 r4:
[] (show_stack) from [] (dump_stack+0x90/0xac)
[] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8)
 r7:c07acca0 r6:0096 r5:0009 r4:ee4d3e38
[] (warn_slowpath_common) from [] 
(warn_slowpath_fmt+0x38/0x40)
 r8:ee3fcc00 r7:01cc r6: r5:0002 r4:c07acc78
[] (warn_slowpath_fmt) from [] 
(handle_irq_event_percpu+0x14c/0x174)
 r3:01cc r2:c07acc78
 r4:ecfcdd80
[] (handle_irq_event_percpu) from [] 
(handle_irq_event+0x84/0xb8)
 r10:0001 r9:ee4b59c0 r8:ee1d0900 r7:0001 r6: r5:ecfcdd80
 r4:ee3fcc00
[] (handle_irq_event) from [] (handle_simple_irq+0x90/0x118)
 r5:ee4a9020 r4:ee3fcc00
[] (handle_simple_irq) from [] 
(generic_handle_irq+0x30/0x44)
 r5:ee4a9020 r4:01cc
[] (generic_handle_irq) from [] 
(dra7xx_pcie_msi_irq_handler+0x7c/0x8c)
 r5:ee4a9020 r4:0001
[] (dra7xx_pcie_msi_irq_handler) from [] 
(irq_forced_thread_fn+0x28/0x5c)
 r5:ee1d0900 r4:ee4b59c0
[] (irq_forced_thread_fn) from [] (irq_thread+0x128/0x204)
 r7:0001 r6: r5:ee4d2000 r4:ee4b59e4
[] (irq_thread) from [] (kthread+0xd4/0xec)
 r10: r9: r8: r7:c00870b4 r6:ee4b59c0 r5:ee4b5a00
 r4:
[] (kthread) from [] (ret_from_fork+0x14/0x2c)
 r7: r6: r5:c005e228 r4:ee4b5a00
---[ end trace 0002 ]---

This backtrace is triggered because dra7xx_pcie_msi_irq_handler()
forced to be threaded by default on -RT but, in the same time, it's
IRQ dispatcher and calls generic_handle_irq(), which leads to
handle_simple_irq() call. The handle_simple_irq() expected to be
called with IRQ disabled.

The same issue will also happen if kernel will boot with "threadirqs"
cmdline parameter (CONFIG_IRQ_FORCED_THREADING).

As discussed in [1], the current DRA7 PCIe hw configuration supports
32 interrupts, which cannot change because it's hardwired in silicon,
this is a single status read and assuming that only a few (most of the
time it will be exactly ONE) of those interrupts are pending at the
same time is pretty much a sane assumption. And recommended fix for
this issue is to request dra7xx_pcie_msi IRQ with IRQF_NO_THREAD flag.

[1] https://lkml.org/lkml/2015/11/3/660

Cc: Thomas Gleixner 
Cc: Sebastian Andrzej Siewior 
Signed-off-by: Grygorii Strashko 
---
Changes in v2:
- comment in code truncated
Link on v1:
 https://lkml.org/lkml/2015/11/5/593
 drivers/pci/host/pci-dra7xx.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
index 8c36880..0415192 100644
--- a/drivers/pci/host/pci-dra7xx.c
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -301,8 +301,19 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie 
*dra7xx,
return -EINVAL;
}
 
+   /*
+* Mark dra7xx_pcie_msi IRQ as IRQF_NO_THREAD
+* On -RT and if kernel is booting with "threadirqs" cmd line parameter
+* the dra7xx_pcie_msi_irq_handler() will be forced threaded but,
+* in the same time, it's IRQ dispatcher and calls generic_handle_irq(),
+* which, in turn, will be resolved to handle_simple_irq() call.
+* The handle_simple_irq() expected to be called with IRQ disabled, as
+* result kernle will display warning:
+* "irq XXX handler YYY+0x0/0x14 enabled interrupts".
+*/
ret = devm_request_irq(>dev, pp->irq,
-  dra7xx_pcie_msi_irq_handler, IRQF_SHARED,
+  dra7xx_pcie_msi_irq_handler,
+  IRQF_SHARED | IRQF_NO_THREAD,
   "dra7-pcie-msi", pp);
if (ret) {
dev_err(>dev, "failed to request irq\n");
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread Grygorii Strashko 
Now the System stall is observed on TI AM437x based board
(am437x-gp-evm) during resuming from System suspend when ARM Global
timer is selected as clocksource device - SysRq are working, but
nothing else. The reason of stall is that ARM Global timer loses its
contexts.

The reason of stall is that ARM Global timer loses its contexts during
System suspend:
   GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
   GT_COUNTERx = 0

Hence, update ARM Global timer driver to reflect above behaviour
- re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
- ensure clocksource and clockevent devices have coresponding flags
  (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
  depending on presence of "always-on" DT property.

CC: Arnd Bergmann 
Cc: John Stultz 
Cc: Felipe Balbi 
Cc: Tony Lindgren 
Cc: Santosh Shilimkar 
Signed-off-by: Grygorii Strashko 
---
Changes in v2:
 - suspend/resume simplified: nothing is stored any more and
   ARM GT just re-enabled
Link on v1:
 https://lkml.org/lkml/2015/11/13/456

 drivers/clocksource/arm_global_timer.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/drivers/clocksource/arm_global_timer.c 
b/drivers/clocksource/arm_global_timer.c
index a2cb6fa..867e546 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -51,6 +51,7 @@ static void __iomem *gt_base;
 static unsigned long gt_clk_rate;
 static int gt_ppi;
 static struct clock_event_device __percpu *gt_evt;
+static bool gt_always_on;
 
 /*
  * To get the value from the Global Timer Counter register proceed as follows:
@@ -168,6 +169,9 @@ static int gt_clockevents_init(struct clock_event_device 
*clk)
 {
int cpu = smp_processor_id();
 
+   if (!gt_always_on)
+   clk->features |= CLOCK_EVT_FEAT_C3STOP;
+
clk->name = "arm_global_timer";
clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
CLOCK_EVT_FEAT_PERCPU;
@@ -195,12 +199,19 @@ static cycle_t gt_clocksource_read(struct clocksource *cs)
return gt_counter_read();
 }
 
+static void gt_resume(struct clocksource *cs)
+{
+   /* re-enable timer on resume */
+   writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
+}
+
 static struct clocksource gt_clocksource = {
.name   = "arm_global_timer",
.rating = 300,
.read   = gt_clocksource_read,
.mask   = CLOCKSOURCE_MASK(64),
.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+   .resume = gt_resume,
 };
 
 #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
@@ -218,6 +229,9 @@ static void __init gt_clocksource_init(void)
/* enables timer on all the cores */
writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
 
+   if (gt_always_on)
+   gt_clocksource.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP;
+
 #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate);
 #endif
@@ -289,6 +303,8 @@ static void __init global_timer_of_register(struct 
device_node *np)
goto out_clk;
}
 
+   gt_always_on = of_property_read_bool(np, "always-on");
+
err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt,
 "gt", gt_evt);
if (err) {
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[4.4-rc][PATCH] gpio: omap: drop omap1 mpuio specific irq_mask/unmask callbacks

2015-11-20 Thread Grygorii Strashko 
Originally OMAP MPUIO GPIO irqchip was implemented using Generic irq
chip, but after set of reworks Generic irq chip code was replaced by
common OMAP GPIO implementation and finally removed by
commit d2d05c65c40e ("gpio: omap: Fix regression for MPUIO interrupts").
Unfortunately, above commit left .irq_mask/unmask callbacks assigned
as below for MPUIO GPIO case:
irqc->irq_mask = irq_gc_mask_set_bit;
irqc->irq_unmask = irq_gc_mask_clr_bit;

This now causes boot failure on OMAP1 platforms, after
commit 450fa54cfd66 ("gpio: omap: convert to use generic irq handler")
which forces these callbacks to be called during GPIO IRQs mapping
from gpiochip_irq_map:

Unable to handle kernel NULL pointer dereference at virtual address 
pgd = c0004000
[] *pgd=
Internal error: Oops: 75 [#1] ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 
4.4.0-rc1-e3-los_afe0c+-2-g25379c0-dirty #1
Hardware name: Amstrad E3 (Delta)
task: c1836000 ti: c1838000 task.ti: c1838000
PC is at irq_gc_mask_set_bit+0x1c/0x60
LR is at __irq_do_set_handler+0x118/0x15c
pc : []lr : []psr: 60d3
sp : c1839c90  ip : c1862c64  fp : c1839c9c
r10:   r9 : c0411950  r8 : c0411bbc
r7 :   r6 : c185c310  r5 : c00444e8  r4 : c185c300
r3 : c1854b50  r2 :   r1 :   r0 : c185c310
Flags: nZCv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: 317f  Table: 10004000  DAC: 0057
Process swapper (pid: 1, stack limit = 0xc1838190)
Stack: (0xc1839c90 to 0xc183a000)

[...]

Backtrace:
[] (irq_gc_mask_set_bit) from [] 
(__irq_do_set_handler+0x118/0x15c)
[] (__irq_do_set_handler) from [] 
(__irq_set_handler+0x44/0x5c)
 r6: r5:c00444e8 r4:c185c300
[] (__irq_set_handler) from [] 
(irq_set_chip_and_handler_name+0x30/0x34)
 r7:0050 r6: r5:c00444e8 r4:0050
[] (irq_set_chip_and_handler_name) from [] 
(gpiochip_irq_map+0x3c/0x8c)
 r7:0050 r6: r5:0050 r4:c1862c64
[] (gpiochip_irq_map) from [] 
(irq_domain_associate+0x7c/0x1c4)
 r5:c185c310 r4:c185cb00
[] (irq_domain_associate) from [] 
(irq_domain_add_simple+0x98/0xc0)
 r8:c0411bbc r7:c185cb00 r6:0050 r5:0010 r4:0001
[] (irq_domain_add_simple) from [] 
(_gpiochip_irqchip_add+0x64/0x10c)
 r7:c1862c64 r6:c0419280 r5:c1862c64 r4:c1854b50
[] (_gpiochip_irqchip_add) from [] 
(omap_gpio_probe+0x2fc/0x63c)
 r5:c1854b50 r4:c1862c10
[] (omap_gpio_probe) from [] (platform_drv_probe+0x2c/0x64)
 r10: r9:c03e45e8 r8: r7:c0419294 r6:c0411984 r5:c0419294
 r4:c0411950
[] (platform_drv_probe) from [] (really_probe+0x160/0x29c)

Hence, fix it by remove obsolete callbacks assignment. After this
change  omap_gpio_mask_irq()/omap_gpio_unmask_irq() will be used
for MPUIO IRQs masking, but this now happens anyway from
omap_gpio_irq_startup/shutdown().

Cc: Tony Lindgren 
Fixes: commit d2d05c65c40e ("gpio: omap: Fix regression for MPUIO interrupts")
Reported-by: Aaro Koskinen 
Signed-off-by: Grygorii Strashko 
---
 drivers/gpio/gpio-omap.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 56d2d02..f7fbb46 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1122,8 +1122,6 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, 
struct irq_chip *irqc)
/* MPUIO is a bit different, reading IRQ status clears it */
if (bank->is_mpuio) {
irqc->irq_ack = dummy_irq_chip.irq_ack;
-   irqc->irq_mask = irq_gc_mask_set_bit;
-   irqc->irq_unmask = irq_gc_mask_clr_bit;
if (!bank->regs->wkup_en)
irqc->irq_set_wake = NULL;
}
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

2015-11-20 Thread Andy Shevchenko 
On Fri, Nov 20, 2015 at 2:30 PM, Peter Ujfalusi  wrote:
> On 11/20/2015 02:24 PM, Andy Shevchenko wrote:
>> On Fri, Nov 20, 2015 at 12:58 PM, Arnd Bergmann  wrote:
>>> On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote:
 On 11/19/2015 01:25 PM, Arnd Bergmann wrote:
>>
>>> Another idea would be to remove the filter function from struct dma_chan_map
>>> and pass the map through platform data
>>
>> Why not unified device properties?
>
> Is this some Windows/ACPI feature?

Nope.

Check drivers/base/property.c

> Quick search gives mostly MSDN and
> Windows10 related links.
>
> We only need dma_chan_map for platforms which has not been converted to DT and
> still using legacy boot. Or platforms which can still boot in legacy mode. In
> DT/ACPI mode we do not need this map at all.


-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] extcon: palmas: add support for using VBUSDET output

2015-11-20 Thread Felipe Balbi 

Hi Chanwoo,

Chanwoo Choi  writes:
> Hi Felipe,
>
> On 2015년 11월 20일 14:33, Chanwoo Choi wrote:
>> Hi Felipe,
>> 
>> Looks good to me. But I have one comment.
>> 
>> On 2015년 11월 13일 02:57, Felipe Balbi wrote:
>>> TPS659038 can remux its GPIO_1 as VBUSDET output,
>>> which can be tied to a SoC GPIO and used as a VBUS
>>> interrupt.
>>>
>>> Beagle X15 uses that, in fact, and without it, I
>>> could not get USB peripheral working with that
>>> board.
>>>
>>> Signed-off-by: Felipe Balbi 
>>> ---
>>>  drivers/extcon/extcon-palmas.c | 22 --
>>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>>> index 93c30a885740..7985d092c069 100644
>>> --- a/drivers/extcon/extcon-palmas.c
>>> +++ b/drivers/extcon/extcon-palmas.c
>>> @@ -296,10 +296,28 @@ static int palmas_usb_probe(struct platform_device 
>>> *pdev)
>>> }
>>>  
>>> if (palmas_usb->enable_vbus_detection) {
>>> +   int irq = platform_get_irq(pdev, 0);
>>> +
>>> +   if (irq > 0) {
>>> +   /* remux GPIO_1 as VBUSDET */
>>> +   status = palmas_update_bits(palmas, 
>>> PALMAS_PU_PD_OD_BASE,
>>> +   PALMAS_PRIMARY_SECONDARY_PAD1,
>>> +   
>>> PALMAS_PRIMARY_SECONDARY_PAD1_GPIO_1_MASK,
>>> +   (1 << 3));
>> 
>> PALMAS_PRIMARY_SECONDARY_PAD1_GPIO_1_SHIFT is appropriate instead of
>> using '3'.

good point :-)

>>> +   if (status < 0) {
>>> +   dev_err(>dev, "can't remux GPIO1\n");
>>> +   return status;
>>> +   }
>>> +
>>> +   palmas_usb->vbus_irq = irq;
>>> +   } else {
>>> +   irq = regmap_irq_get_virq(palmas->irq_data,
>>> +   PALMAS_VBUS_IRQ);
>>> +   palmas_usb->vbus_irq = irq;
>>> +   }
>>> +
>>> palmas_usb->vbus_otg_irq = regmap_irq_get_virq(palmas->irq_data,
>>>PALMAS_VBUS_OTG_IRQ);
>>> -   palmas_usb->vbus_irq = regmap_irq_get_virq(palmas->irq_data,
>>> -  PALMAS_VBUS_IRQ);
>>> status = devm_request_threaded_irq(palmas_usb->dev,
>>> palmas_usb->vbus_irq, NULL,
>>> palmas_vbus_irq_handler,
>>>
>> 
>> Thanks,
>> Chanwoo Choi
>> --
>> 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/
>> 
>
> If you are OK about following patch, I'll apply it on extcon branch.

that's perfect, thanks for fixing it :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

2015-11-20 Thread Arnd Bergmann 
On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote:
> On 11/19/2015 01:25 PM, Arnd Bergmann wrote:
> >> dma_request_channel(mask); /* memcpy. etc, non slave mostly */
> >>
> >> Not sure how to name this as reusing existing (good, descriptive) function
> >> names would mean changes all over the kernel to start off this.
> >>
> >> Not used and
> >> request_dma_channel(); /* as _irq/_mem_region/_resource, etc */
> >> request_dma();
> >> dma_channel_request();
> > 
> > dma_request_slavechan();
> > dma_request_slave();
> > dma_request_mask();
> 
> Let me think aloud here a bit...
> 1. To request slave channel which will return you the channel your device is
> bind via DT/ACPI or the platform map table you propose later:
> 
> dma_request_chan(struct device *dev, const char *name);
> 
> 2. To request a channel (any channel) matching with the capabilities the
> driver needs, like memcpy, memset, etc:
> 
> #define dma_request_chan_by_mask(mask) __dma_request_chan_by_mask(&(mask))
> __dma_request_chan_by_mask(const dma_cap_mask_t *mask);
> 
> I think the dma_request_chan() does not need mask to be passed, since via this
> we request a specific channel which has been defined/set by DT/ACPI or the
> lookup map. We could add a mask parameter which could be used to sanity check
> the channel we got against the capabilities the driver needs from the channel.
> We currently do this in the drivers where the author wanted to make sure that
> the channel is capable of what it should be capable.
> 
> So two API to request channel:
> struct dma_chan *dma_request_chan(struct device *dev, const char *name);
> struct dma_chan *dma_request_chan_by_mask(const dma_cap_mask_t *mask);
> 
> Both will return with the valid channel pointer or in case of failure with
> ERR_PTR().
> 
> We need to go through the code in regards to return codes also to have sane
> mapping.

Right.
> > That way the vast majority of drivers can use one of the two nice interfaces
> > and the rest can be converted to use __dma_request_chan().
> > 
> > On a related topic, we had in the past considered providing a way for
> > platform code to register a lookup table of some sort, to associate
> > a device/name pair with a configuration. That would let us use the
> > simplified dma_request_slavechan(dev, name) pair everywhere. We could
> > use the same method that we have for clk_register_clkdevs() or
> > pinctrl_register_map().
> > 
> > Something like either
> > 
> > static struct dma_chan_map myplatform_dma_map[] = {
> > { .devname = "omap-aes0", .slave = "tx", .filter = omap_dma_filter_fn, 
> > .arg = (void *)65, },
> > { .devname = "omap-aes0", .slave = "rx", .filter = omap_dma_filter_fn, 
> > .arg = (void *)66, },
> > };
> > 
> > or
> > 
> > static struct dma_chan_map myplatform_dma_map[] = {
> > { .devname = "omap-aes0", .slave = "tx", .master = "omap-dma-engine0", 
> > .req = 65, },
> > { .devname = "omap-aes0", .slave = "rx", .master = "omap-dma-engine0", 
> > .req = 66, },
> 
> sa11x0-dma expects the fn_param as string :o

Some of them do, but the new API requires changes in both the DMA master and
slave drivers, so that could be changed if we wanted to, or we just allow 
both methods indefinitely and let sa11x0-dma pass the filterfn+data rather than
a number.

> > };
> 
> Basically we are deprecating the use of IORESOURCE_DMA?

I thought we already had ;-)

> For legacy the filter function is pretty much needed to handle the differences
> between the platforms as not all of them does the filtering in a same way. So
> the first type of map would be feasible IMHO.

It certainly makes the transition to a map table much easier.

> > we could even allow a combination of the two, so the simple case just 
> > specifies
> > master and req number, which requires changes to the dmaengine driver, but 
> > we could
> > also do a mass-conversion to the .filter/.arg variant.
> 
> This will get rid of the need for the fn and fn_param parameters when
> requesting dma channel, but it will not get rid of the exported function from
> the dma engine drivers since in arch code we need to have visibility to the
> filter_fn.

Correct. A lot of dmaengine drivers already need to be built-in so the platform
code can put a pointer to the filter function, so it would not be worse for 
them.

Another idea would be to remove the filter function from struct dma_chan_map
and pass the map through platform data to the dmaengine driver, which then
registers it to the core along with the mask. Something like:

/* platform code */
static struct dma_chan_map oma_dma_map[] = {
{ .devname = "omap-aes0", .slave = "tx", .arg = (void *)65, },
{ .devname = "omap-aes0", .slave = "rx", .arg = (void *)66, },
...
{},
};

static struct omap_system_dma_plat_info dma_plat_info __initdata = {
.dma_map = _dma_map,
...
};  

machine_init(void)
{
...
platform_device_register_data(NULL, "omap-dma-engine", 0, 
_plat_info,

Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

2015-11-20 Thread Peter Ujfalusi 
On 11/20/2015 12:58 PM, Arnd Bergmann wrote:
>>> That way the vast majority of drivers can use one of the two nice interfaces
>>> and the rest can be converted to use __dma_request_chan().
>>>
>>> On a related topic, we had in the past considered providing a way for
>>> platform code to register a lookup table of some sort, to associate
>>> a device/name pair with a configuration. That would let us use the
>>> simplified dma_request_slavechan(dev, name) pair everywhere. We could
>>> use the same method that we have for clk_register_clkdevs() or
>>> pinctrl_register_map().
>>>
>>> Something like either
>>>
>>> static struct dma_chan_map myplatform_dma_map[] = {
>>> { .devname = "omap-aes0", .slave = "tx", .filter = omap_dma_filter_fn, 
>>> .arg = (void *)65, },
>>> { .devname = "omap-aes0", .slave = "rx", .filter = omap_dma_filter_fn, 
>>> .arg = (void *)66, },
>>> };
>>>
>>> or
>>>
>>> static struct dma_chan_map myplatform_dma_map[] = {
>>> { .devname = "omap-aes0", .slave = "tx", .master = "omap-dma-engine0", 
>>> .req = 65, },
>>> { .devname = "omap-aes0", .slave = "rx", .master = "omap-dma-engine0", 
>>> .req = 66, },
>>
>> sa11x0-dma expects the fn_param as string :o
> 
> Some of them do, but the new API requires changes in both the DMA master and
> slave drivers, so that could be changed if we wanted to, or we just allow 
> both methods indefinitely and let sa11x0-dma pass the filterfn+data rather 
> than
> a number.

Hrm, I would say that we need to push everyone to use the new API. sa11x0
should not be a big deal to fix IMHO and other users should be reasonably
simple to convert.

>>> };
>>
>> Basically we are deprecating the use of IORESOURCE_DMA?
> 
> I thought we already had ;-)

For DT boot, yes. Not for the legacy boot.

>> For legacy the filter function is pretty much needed to handle the 
>> differences
>> between the platforms as not all of them does the filtering in a same way. So
>> the first type of map would be feasible IMHO.
> 
> It certainly makes the transition to a map table much easier.

And the aim anyway is to convert everything to DT, right?

>>> we could even allow a combination of the two, so the simple case just 
>>> specifies
>>> master and req number, which requires changes to the dmaengine driver, but 
>>> we could
>>> also do a mass-conversion to the .filter/.arg variant.
>>
>> This will get rid of the need for the fn and fn_param parameters when
>> requesting dma channel, but it will not get rid of the exported function from
>> the dma engine drivers since in arch code we need to have visibility to the
>> filter_fn.
> 
> Correct. A lot of dmaengine drivers already need to be built-in so the 
> platform
> code can put a pointer to the filter function, so it would not be worse for 
> them.
> 
> Another idea would be to remove the filter function from struct dma_chan_map
> and pass the map through platform data to the dmaengine driver, which then
> registers it to the core along with the mask. Something like:
> 
> /* platform code */
> static struct dma_chan_map oma_dma_map[] = {
>   { .devname = "omap-aes0", .slave = "tx", .arg = (void *)65, },
>   { .devname = "omap-aes0", .slave = "rx", .arg = (void *)66, },
>   ...
>   {},
> };
> 
> static struct omap_system_dma_plat_info dma_plat_info __initdata = {
>   .dma_map = _dma_map,
>   ...
> };  
> 
> machine_init(void)
> {
>   ...
>   platform_device_register_data(NULL, "omap-dma-engine", 0, 
> _plat_info, sizeof(dma_plat_info);
>   ...
> }
> 
> /* dmaengine driver */
> 
> static int omap_dma_probe(struct platform_device *pdev)
> {
>   struct omap_system_dma_plat_info *pdata = dev_get_platdata(>dev);
>   ...
> 
>   dmam_register_platform_map(>dev, omap_dma_filter_fn, 
> pdata->dma_map);
> }
> 
> /* dmaengine core */
> 
> struct dma_map_list {
>   struct list_head node;
>   struct device *master;
>   dma_filter_fn filter;
>   struct dma_chan_map *map;
> };
> 
> static LIST_HEAD(dma_map_list);
> static DEFINE_MUTEX(dma_map_mutex);
> 
> int dmam_register_platform_map(struct device *dev, dma_filter_fn filter, 
> struct dma_chan_map *map)
> {
>   struct dma_map_list *list = kmalloc(sizeof(*list), GFP_KERNEL);
> 
>   if (!list)
>   return -ENOMEM;
> 
>   list->dev = dev;
>   list->filter = filter;
>   list->map = map;
> 
>   mutex_lock(_map_mutex);
>   list_add(_map_list, >node);
>   mutex_unlock(_map_mutex);
> }
> 
> Now we can completely remove the dependency on the filter function definition
> from platform code and slave drivers.

Sounds feasible for OMAP and daVinci and for others as well. I think ;)
I would go with this if someone asks my opinion :D

The core change to add the new API + the dma_map support should be pretty
straight forward. It can live alongside with the old API and we can phase out
the users of the old one.
The legacy support would need more time since we need to modify the arch

Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

2015-11-20 Thread Peter Ujfalusi 
On 11/19/2015 01:25 PM, Arnd Bergmann wrote:
>> If we have two main APIs, one to request slave channels and one to get any
>> channel with given capability
>> dma_request_slave_channel(NULL, NULL, , fn, fn_param); /* Legacy slave 
>> */
>> dma_request_slave_channel(dev, name, NULL, NULL, NULL); /* DT/ACPI, current
>>slave */
>> dma_request_slave_channel(dev, name, , fn, fn_param); /* current 
>> compat*/
>>
>> This way we can omit the mask also in cases when the client only want to get
>> DMA_SLAVE, we can just build up the mask within the function. If the mask is
>> provided we would copy the bits from the provided mask, so for example if you
>> want to have DMA_SLAVE+DMA_CYCLIC, the driver only needs to pass DMA_CYCLIC,
>> the DMA_SLAVE is going to be set anyways.
> 
> I think it's more logical here to have mask=NULL mean that we want DMA_SLAVE,
> but otherwise pass the full mask as DMA_SLAVE|DMA_CYCLIC etc.

Yep, could be, while I would write the core part to set the DMA_SLAVE
unconditionally anyways. If the API say it is dma_request_slavechan() it is
expected to get channel which is capable of DMA_SLAVE.

>> dma_request_channel(mask); /* memcpy. etc, non slave mostly */
>>
>> Not sure how to name this as reusing existing (good, descriptive) function
>> names would mean changes all over the kernel to start off this.
>>
>> Not used and
>> request_dma_channel(); /* as _irq/_mem_region/_resource, etc */
>> request_dma();
>> dma_channel_request();
> 
> dma_request_slavechan();
> dma_request_slave();
> dma_request_mask();

Let me think aloud here a bit...
1. To request slave channel which will return you the channel your device is
bind via DT/ACPI or the platform map table you propose later:

dma_request_chan(struct device *dev, const char *name);

2. To request a channel (any channel) matching with the capabilities the
driver needs, like memcpy, memset, etc:

#define dma_request_chan_by_mask(mask) __dma_request_chan_by_mask(&(mask))
__dma_request_chan_by_mask(const dma_cap_mask_t *mask);

I think the dma_request_chan() does not need mask to be passed, since via this
we request a specific channel which has been defined/set by DT/ACPI or the
lookup map. We could add a mask parameter which could be used to sanity check
the channel we got against the capabilities the driver needs from the channel.
We currently do this in the drivers where the author wanted to make sure that
the channel is capable of what it should be capable.

So two API to request channel:
struct dma_chan *dma_request_chan(struct device *dev, const char *name);
struct dma_chan *dma_request_chan_by_mask(const dma_cap_mask_t *mask);

Both will return with the valid channel pointer or in case of failure with
ERR_PTR().

We need to go through the code in regards to return codes also to have sane
mapping.

> 
>> All in all, not sure which way would be better...
> 
> I think I would prefer the simplest API to have only the dev+name
> arguments, as we tend to move that way for all platforms anyway, and it
> seems silly to have all drivers pass three NULL arguments all the time.
> At the moment, there are 139 references to dma_request_slave_channel_*
> in the kernel, and only 46 of them are dma_request_slave_channel_compat.
> Out of those 46, a couple can already be converted back to use
> dma_request_slave_channel() because the platform now only supports
> devicetree based boots and will not go back to platform data.
> 
> How about something like
> 
> extern struct dma_chan *
> __dma_request_chan(struct device *dev, const char *name,
>   const dma_cap_mask_t *mask, dma_filter_fn fn, void 
> *fn_param);
> 
> static inline struct dma_chan *
> dma_request_slavechan(struct device *dev, const char *name)
> {
>   return __dma_request_chan(dev, name, NULL, NULL, NULL);
> }
> 
> static inline struct dma_chan *
> dma_request_chan(const dma_cap_mask_t *mask)
> {
>   return __dma_request_chan(NULL, NULL, mask, NULL, NULL);
> }
> 
> That way the vast majority of drivers can use one of the two nice interfaces
> and the rest can be converted to use __dma_request_chan().
> 
> On a related topic, we had in the past considered providing a way for
> platform code to register a lookup table of some sort, to associate
> a device/name pair with a configuration. That would let us use the
> simplified dma_request_slavechan(dev, name) pair everywhere. We could
> use the same method that we have for clk_register_clkdevs() or
> pinctrl_register_map().
> 
> Something like either
> 
> static struct dma_chan_map myplatform_dma_map[] = {
>   { .devname = "omap-aes0", .slave = "tx", .filter = omap_dma_filter_fn, 
> .arg = (void *)65, },
>   { .devname = "omap-aes0", .slave = "rx", .filter = omap_dma_filter_fn, 
> .arg = (void *)66, },
> };
> 
> or
> 
> static struct dma_chan_map myplatform_dma_map[] = {
>   { .devname = "omap-aes0", .slave = "tx", .master = "omap-dma-engine0", 
> .req

Re: [PATCH] extcon: palmas: add support for using VBUSDET output

2015-11-20 Thread Chanwoo Choi 
Hi Felipe,

On 2015. 11. 20. 오후 11:37, Felipe Balbi wrote:
> 
> Hi Chanwoo,
> 
> Chanwoo Choi  writes:
>> Hi Felipe,
>>
>> On 2015년 11월 20일 14:33, Chanwoo Choi wrote:
>>> Hi Felipe,
>>>
>>> Looks good to me. But I have one comment.
>>>
>>> On 2015년 11월 13일 02:57, Felipe Balbi wrote:
 TPS659038 can remux its GPIO_1 as VBUSDET output,
 which can be tied to a SoC GPIO and used as a VBUS
 interrupt.

 Beagle X15 uses that, in fact, and without it, I
 could not get USB peripheral working with that
 board.

 Signed-off-by: Felipe Balbi 
 ---
  drivers/extcon/extcon-palmas.c | 22 --
  1 file changed, 20 insertions(+), 2 deletions(-)

 diff --git a/drivers/extcon/extcon-palmas.c 
 b/drivers/extcon/extcon-palmas.c
 index 93c30a885740..7985d092c069 100644
 --- a/drivers/extcon/extcon-palmas.c
 +++ b/drivers/extcon/extcon-palmas.c
 @@ -296,10 +296,28 @@ static int palmas_usb_probe(struct platform_device 
 *pdev)
}
  
if (palmas_usb->enable_vbus_detection) {
 +  int irq = platform_get_irq(pdev, 0);
 +
 +  if (irq > 0) {
 +  /* remux GPIO_1 as VBUSDET */
 +  status = palmas_update_bits(palmas, 
 PALMAS_PU_PD_OD_BASE,
 +  PALMAS_PRIMARY_SECONDARY_PAD1,
 +  
 PALMAS_PRIMARY_SECONDARY_PAD1_GPIO_1_MASK,
 +  (1 << 3));
>>>
>>> PALMAS_PRIMARY_SECONDARY_PAD1_GPIO_1_SHIFT is appropriate instead of
>>> using '3'.
> 
> good point :-)
> 
 +  if (status < 0) {
 +  dev_err(>dev, "can't remux GPIO1\n");
 +  return status;
 +  }
 +
 +  palmas_usb->vbus_irq = irq;
 +  } else {
 +  irq = regmap_irq_get_virq(palmas->irq_data,
 +  PALMAS_VBUS_IRQ);
 +  palmas_usb->vbus_irq = irq;
 +  }
 +
palmas_usb->vbus_otg_irq = regmap_irq_get_virq(palmas->irq_data,
   PALMAS_VBUS_OTG_IRQ);
 -  palmas_usb->vbus_irq = regmap_irq_get_virq(palmas->irq_data,
 - PALMAS_VBUS_IRQ);
status = devm_request_threaded_irq(palmas_usb->dev,
palmas_usb->vbus_irq, NULL,
palmas_vbus_irq_handler,

>>>
>>> Thanks,
>>> Chanwoo Choi
>>> --
>>> 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/
>>>
>>
>> If you are OK about following patch, I'll apply it on extcon branch.
> 
> that's perfect, thanks for fixing it :-)
> 

Before applying this patch, I think that there are more generic method?
So, I check the extcon-palmas.c driver again. There is similiar case for id 
detection.

In some case, id detection use whether 'enable_id_detection' with own interrupt
or 'enable_gpio_id_dectection' with specific h/w gpio like this case of vbus.

In result, I implement the following patch for vbus gpio detection.
But, I'm not sure because I have not any H/W board for test.

If you possible, could you test it with following patch?

Thanks,
Chanwoo Choi

diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
index 93c30a8..885ee95 100644
--- a/drivers/extcon/extcon-palmas.c
+++ b/drivers/extcon/extcon-palmas.c
@@ -216,11 +216,23 @@ static int palmas_usb_probe(struct platform_device *pdev)
return PTR_ERR(palmas_usb->id_gpiod);
}
 
+   palmas_usb->vbus_gpiod = devm_gpiod_get_optional(>dev, "vbus",
+   GPIOD_IN);
+   if (IS_ERR(palmas_usb->vbus_gpiod)) {
+   dev_err(>dev, "failed to get vbus gpio\n");
+   return PTR_ERR(palmas_usb->vbus_gpiod);
+   }
+
if (palmas_usb->enable_id_detection && palmas_usb->id_gpiod) {
palmas_usb->enable_id_detection = false;
palmas_usb->enable_gpio_id_detection = true;
}
 
+   if (palmas_usb->enable_vbus_detection && palmas_usb->vbus_gpiod) {
+   palmas_usb->enable_vbus_detection = false;
+   palmas_usb->enable_gpio_vbus_detection = true;
+   }
+
if (palmas_usb->enable_gpio_id_detection) {
u32 debounce;
 
@@ -311,6 +323,40 @@ static int palmas_usb_probe(struct platform_device *pdev)
palmas_usb->vbus_irq, status);
return status;
}
+   } else if

Re: [PATCH] extcon: palmas: add support for using VBUSDET output

2015-11-20 Thread Chanwoo Choi 
Hi,

On Sat, Nov 21, 2015 at 12:05 AM, Chanwoo Choi  wrote:
> Hi Felipe,
>
> On Fri, Nov 20, 2015 at 11:37 PM, Felipe Balbi  wrote:
>>
>> Hi Chanwoo,
>>
>> Chanwoo Choi  writes:
>>> Hi Felipe,
>>>
>>> On 2015년 11월 20일 14:33, Chanwoo Choi wrote:
 Hi Felipe,

 Looks good to me. But I have one comment.

 On 2015년 11월 13일 02:57, Felipe Balbi wrote:
> TPS659038 can remux its GPIO_1 as VBUSDET output,
> which can be tied to a SoC GPIO and used as a VBUS
> interrupt.
>
> Beagle X15 uses that, in fact, and without it, I
> could not get USB peripheral working with that
> board.
>
> Signed-off-by: Felipe Balbi 
> ---
>  drivers/extcon/extcon-palmas.c | 22 --
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/extcon/extcon-palmas.c 
> b/drivers/extcon/extcon-palmas.c
> index 93c30a885740..7985d092c069 100644
> --- a/drivers/extcon/extcon-palmas.c
> +++ b/drivers/extcon/extcon-palmas.c
> @@ -296,10 +296,28 @@ static int palmas_usb_probe(struct platform_device 
> *pdev)
> }
>
> if (palmas_usb->enable_vbus_detection) {
> +   int irq = platform_get_irq(pdev, 0);
> +
> +   if (irq > 0) {
> +   /* remux GPIO_1 as VBUSDET */
> +   status = palmas_update_bits(palmas, 
> PALMAS_PU_PD_OD_BASE,
> +   PALMAS_PRIMARY_SECONDARY_PAD1,
> +   
> PALMAS_PRIMARY_SECONDARY_PAD1_GPIO_1_MASK,
> +   (1 << 3));

 PALMAS_PRIMARY_SECONDARY_PAD1_GPIO_1_SHIFT is appropriate instead of
 using '3'.
>>
>> good point :-)
>>
> +   if (status < 0) {
> +   dev_err(>dev, "can't remux GPIO1\n");
> +   return status;
> +   }
> +
> +   palmas_usb->vbus_irq = irq;
> +   } else {
> +   irq = regmap_irq_get_virq(palmas->irq_data,
> +   PALMAS_VBUS_IRQ);
> +   palmas_usb->vbus_irq = irq;
> +   }
> +
> palmas_usb->vbus_otg_irq = 
> regmap_irq_get_virq(palmas->irq_data,
>PALMAS_VBUS_OTG_IRQ);
> -   palmas_usb->vbus_irq = regmap_irq_get_virq(palmas->irq_data,
> -  PALMAS_VBUS_IRQ);
> status = devm_request_threaded_irq(palmas_usb->dev,
> palmas_usb->vbus_irq, NULL,
> palmas_vbus_irq_handler,
>

 Thanks,
 Chanwoo Choi
 --
 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/

>>>
>>> If you are OK about following patch, I'll apply it on extcon branch.
>>
>> that's perfect, thanks for fixing it :-)
>
> Applied it.

Please ignore this reply about applies because I need to discuss about it.

Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] arm: boot: beaglex15: pass correct interrupt

2015-11-20 Thread Chanwoo Choi 
Hi,

On Sat, Nov 21, 2015 at 1:42 AM, Chanwoo Choi  wrote:
> Hi,
>
> On 2015. 11. 20. 오후 2:39, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2015년 11월 13일 02:53, Felipe Balbi wrote:
>>> According to latest schematics [1], GPIO_1/VBUSDET
>>> on TPS659038 is tied to AM57x GPIO4_21. We can use
>>> that as a VBUS interrupt, instead of relying on
>>> PMIC's VBUS interrupts which don't seem to be firing
>>> on x15 at all.
>>>
>>> A follow up patch will add support for using this
>>> GPIO-based interrupt mechanism for notifying about
>>> VBUS.
>>>
>>> [1] 
>>> https://github.com/beagleboard/beagleboard-x15/blob/master/BeagleBoard-X15_RevA2.pdf
>>>
>>> Signed-off-by: Felipe Balbi 
>>> ---
>>>  arch/arm/boot/dts/am57xx-beagle-x15.dts | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm/boot/dts/am57xx-beagle-x15.dts 
>>> b/arch/arm/boot/dts/am57xx-beagle-x15.dts
>>> index 6f3a1a7ec5f9..5e47162f7883 100644
>>> --- a/arch/arm/boot/dts/am57xx-beagle-x15.dts
>>> +++ b/arch/arm/boot/dts/am57xx-beagle-x15.dts
>>> @@ -560,6 +560,7 @@
>>>  extcon_usb2: tps659038_usb {
>>>  compatible = "ti,palmas-usb-vid";
>>>  ti,enable-vbus-detection;
>>> +interrupts-extended = < 21 IRQ_TYPE_EDGE_RISING>;
>
> vbus-gpio = < 21>;

I'm sorry. I'm sending the missing email without writing completion.

I agree the Felipe's opinion. Just I think that we can use the
following property
instead of 'interrupt-extended'. Because I think 'vbus-gpio' is more
readability than before. The following property mean the attribute of
GPIO pin as VBUS.
- vbus-gpio = < 21>;

Thanks,
Chanwoo Choi

>
>>>  };
>>>
>>>  };
>>>
>>
>> I check the schematic file. The GPIO4_21 pin is connected as following:
>> - GPIO_1/VBUSDET -> PMIC_VBUS_DET -> VBUS_DET -> AM5728 GPIO4_21 pin
>>
>> Reviewed-by: Chanwoo Choi 
>>
>> Thanks,
>> Chanwoo Choi
>> --
>> 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/
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: cpsw: Fix ethernet regression for dm814x

2015-11-20 Thread David Miller 
From: Tony Lindgren 
Date: Wed, 18 Nov 2015 17:27:25 -0800

> Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
> id to common file") started using of_machine_is_compatible for detecting
> type but missed at dm8148 causing Ethernet to stop working.
> 
> Let's fix the issue by adding handling for dm814x.
> 
> Cc: Mugunthan V N 
> Signed-off-by: Tony Lindgren 

Applied, thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] clocksource: ti-32k: convert to platform device

2015-11-20 Thread Felipe Balbi 

Hi,

Grygorii Strashko  writes:
> Since system clocksource is finally selected by Clocksource core at
> fs_initcall stage during boot there are no reasons to initialize
> ti_32k_timer at early boot stages. Hence, ti_32k_timer can be
> converted to use platform device/driver model and its PM can be
> implemented using PM runtime which is common for OMAP devices.
>
> Platform specific initialization code has to be disabled once as
> ti_32k_timer is converted to platform device - otherwise OMAP platform
> code will generate boot warnings.
>
> After this change, all counter_32k's platform code can be removed
> once all OMAP boards will be converted to DT.
>
> Cc: Tony Lindgren 
> Cc: Felipe Balbi 
> Signed-off-by: Grygorii Strashko 
> ---
>  arch/arm/mach-omap2/timer.c| 16 +++
>  drivers/clocksource/timer-ti-32k.c | 58 
> --
>  2 files changed, 53 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index b18ebbe..3bfde44 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -393,23 +393,15 @@ static const struct of_device_id omap_counter_match[] 
> __initconst = {
>  static int __init __maybe_unused omap2_sync32k_clocksource_init(void)
>  {
>   int ret;
> - struct device_node *np = NULL;
>   struct omap_hwmod *oh;
>   const char *oh_name = "counter_32k";
>  
>   /*
> -  * If device-tree is present, then search the DT blob
> -  * to see if the 32kHz counter is supported.
> +  * If device-tree is present, then just exit -
> +  * 32kHz clocksource driver will handle it.
>*/
> - if (of_have_populated_dt()) {
> - np = omap_get_timer_dt(omap_counter_match, NULL);
> - if (!np)
> - return -ENODEV;
> -
> - of_property_read_string_index(np, "ti,hwmods", 0, _name);
> - if (!oh_name)
> - return -ENODEV;
> - }
> + if (of_have_populated_dt())
> + return 0;
>  
>   /*
>* First check hwmod data is available for sync32k counter
> diff --git a/drivers/clocksource/timer-ti-32k.c 
> b/drivers/clocksource/timer-ti-32k.c
> index 8518d9d..e71496f 100644
> --- a/drivers/clocksource/timer-ti-32k.c
> +++ b/drivers/clocksource/timer-ti-32k.c
> @@ -39,8 +39,11 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  /*
>   * 32KHz clocksource ... always available, on pretty most chips except
> @@ -88,15 +91,28 @@ static u64 notrace omap_32k_read_sched_clock(void)
>   return ti_32k_read_cycles(_32k_timer.cs);
>  }
>  
> -static void __init ti_32k_timer_init(struct device_node *np)
> +static const struct of_device_id ti_32k_of_table[] = {
> + { .compatible = "ti,omap-counter32k" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ti_32k_of_table);
> +
> +static int __init ti_32k_probe(struct platform_device *pdev)
>  {
> + struct device *dev = >dev;
> + struct resource *res;
>   int ret;
>  
> - ti_32k_timer.base = of_iomap(np, 0);
> - if (!ti_32k_timer.base) {
> - pr_err("Can't ioremap 32k timer base\n");
> - return;
> - }
> + /* Static mapping, never released */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + ti_32k_timer.base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(ti_32k_timer.base))
> + return PTR_ERR(ti_32k_timer.base);
> +
> + pm_runtime_enable(dev);
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0)
> + goto probe_err;
>  
>   ti_32k_timer.counter = ti_32k_timer.base;
>  
> @@ -116,11 +132,35 @@ static void __init ti_32k_timer_init(struct device_node 
> *np)
>   ret = clocksource_register_hz(_32k_timer.cs, 32768);
>   if (ret) {
>   pr_err("32k_counter: can't register clocksource\n");
> - return;
> + goto probe_err;
>   }
>  
>   sched_clock_register(omap_32k_read_sched_clock, 32, 32768);
>   pr_info("OMAP clocksource: 32k_counter at 32768 Hz\n");
> + return 0;
> +
> +probe_err:
> + pm_runtime_put_noidle(dev);
> + return ret;
> +};
> +
> +static struct platform_driver ti_32k_driver __initdata = {
> + .probe  = ti_32k_probe,
> + .driver = {
> + .name   = "ti_32k_timer",
> + .of_match_table = of_match_ptr(ti_32k_of_table),
> + }
> +};
> +
> +static int __init ti_32k_init(void)
> +{
> + return platform_driver_register(_32k_driver);
>  }
> -CLOCKSOURCE_OF_DECLARE(ti_32k_timer, "ti,omap-counter32k",
> - ti_32k_timer_init);
> +
> +subsys_initcall(ti_32k_init);
> +
> +MODULE_AUTHOR("Paul Mundt");
> +MODULE_AUTHOR("Juha Yrjölä");
> +MODULE_DESCRIPTION("OMAP2 32k Timer");
> +MODULE_ALIAS("platform:ti_32k_timer");
> +MODULE_LICENSE("GPL

[RFC PATCH] clocksource: ti-32k: convert to platform device

2015-11-20 Thread Grygorii Strashko 
Since system clocksource is finally selected by Clocksource core at
fs_initcall stage during boot there are no reasons to initialize
ti_32k_timer at early boot stages. Hence, ti_32k_timer can be
converted to use platform device/driver model and its PM can be
implemented using PM runtime which is common for OMAP devices.

Platform specific initialization code has to be disabled once as
ti_32k_timer is converted to platform device - otherwise OMAP platform
code will generate boot warnings.

After this change, all counter_32k's platform code can be removed
once all OMAP boards will be converted to DT.

Cc: Tony Lindgren 
Cc: Felipe Balbi 
Signed-off-by: Grygorii Strashko 
---
 arch/arm/mach-omap2/timer.c| 16 +++
 drivers/clocksource/timer-ti-32k.c | 58 --
 2 files changed, 53 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index b18ebbe..3bfde44 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -393,23 +393,15 @@ static const struct of_device_id omap_counter_match[] 
__initconst = {
 static int __init __maybe_unused omap2_sync32k_clocksource_init(void)
 {
int ret;
-   struct device_node *np = NULL;
struct omap_hwmod *oh;
const char *oh_name = "counter_32k";
 
/*
-* If device-tree is present, then search the DT blob
-* to see if the 32kHz counter is supported.
+* If device-tree is present, then just exit -
+* 32kHz clocksource driver will handle it.
 */
-   if (of_have_populated_dt()) {
-   np = omap_get_timer_dt(omap_counter_match, NULL);
-   if (!np)
-   return -ENODEV;
-
-   of_property_read_string_index(np, "ti,hwmods", 0, _name);
-   if (!oh_name)
-   return -ENODEV;
-   }
+   if (of_have_populated_dt())
+   return 0;
 
/*
 * First check hwmod data is available for sync32k counter
diff --git a/drivers/clocksource/timer-ti-32k.c 
b/drivers/clocksource/timer-ti-32k.c
index 8518d9d..e71496f 100644
--- a/drivers/clocksource/timer-ti-32k.c
+++ b/drivers/clocksource/timer-ti-32k.c
@@ -39,8 +39,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
+#include 
+#include 
 
 /*
  * 32KHz clocksource ... always available, on pretty most chips except
@@ -88,15 +91,28 @@ static u64 notrace omap_32k_read_sched_clock(void)
return ti_32k_read_cycles(_32k_timer.cs);
 }
 
-static void __init ti_32k_timer_init(struct device_node *np)
+static const struct of_device_id ti_32k_of_table[] = {
+   { .compatible = "ti,omap-counter32k" },
+   { }
+};
+MODULE_DEVICE_TABLE(of, ti_32k_of_table);
+
+static int __init ti_32k_probe(struct platform_device *pdev)
 {
+   struct device *dev = >dev;
+   struct resource *res;
int ret;
 
-   ti_32k_timer.base = of_iomap(np, 0);
-   if (!ti_32k_timer.base) {
-   pr_err("Can't ioremap 32k timer base\n");
-   return;
-   }
+   /* Static mapping, never released */
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   ti_32k_timer.base = devm_ioremap_resource(dev, res);
+   if (IS_ERR(ti_32k_timer.base))
+   return PTR_ERR(ti_32k_timer.base);
+
+   pm_runtime_enable(dev);
+   ret = pm_runtime_get_sync(dev);
+   if (ret < 0)
+   goto probe_err;
 
ti_32k_timer.counter = ti_32k_timer.base;
 
@@ -116,11 +132,35 @@ static void __init ti_32k_timer_init(struct device_node 
*np)
ret = clocksource_register_hz(_32k_timer.cs, 32768);
if (ret) {
pr_err("32k_counter: can't register clocksource\n");
-   return;
+   goto probe_err;
}
 
sched_clock_register(omap_32k_read_sched_clock, 32, 32768);
pr_info("OMAP clocksource: 32k_counter at 32768 Hz\n");
+   return 0;
+
+probe_err:
+   pm_runtime_put_noidle(dev);
+   return ret;
+};
+
+static struct platform_driver ti_32k_driver __initdata = {
+   .probe  = ti_32k_probe,
+   .driver = {
+   .name   = "ti_32k_timer",
+   .of_match_table = of_match_ptr(ti_32k_of_table),
+   }
+};
+
+static int __init ti_32k_init(void)
+{
+   return platform_driver_register(_32k_driver);
 }
-CLOCKSOURCE_OF_DECLARE(ti_32k_timer, "ti,omap-counter32k",
-   ti_32k_timer_init);
+
+subsys_initcall(ti_32k_init);
+
+MODULE_AUTHOR("Paul Mundt");
+MODULE_AUTHOR("Juha Yrjölä");
+MODULE_DESCRIPTION("OMAP2 32k Timer");
+MODULE_ALIAS("platform:ti_32k_timer");
+MODULE_LICENSE("GPL v2");
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread Marc Zyngier 
On 20/11/15 18:35, Grygorii Strashko wrote:
> Hi Santosh,
> 
> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
>> + Thomas, Marc
>>
>> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>>> Now the System stall is observed on TI AM437x based board
>>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>>> timer is selected as clocksource device - SysRq are working, but
>>> nothing else. The reason of stall is that ARM Global timer loses its
>>> contexts.
>>>
>>> The reason of stall is that ARM Global timer loses its contexts during
>>> System suspend:
>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>> GT_COUNTERx = 0
>>>
>>> Hence, update ARM Global timer driver to reflect above behaviour
>>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>>> - ensure clocksource and clockevent devices have coresponding flags
>>>(CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>>depending on presence of "always-on" DT property.
>>>
>> Something which loses context in low power states can't be
>> called "always-on"
> 
> Sry, it's kinda new area for me and I could make mistakes.
> 
> While working on this patch I've:
>  - re-used implementation from ARM arch timer 
> commit 82a5619410d4c4df65c04272db198eca5a867c18
> Author: Lorenzo Pieralisi 
> Date:   Tue Apr 8 10:04:32 2014 +0100
> 
> clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue

[...]

This patch has a very specific purpose: instructing the core code that
this timer will never stop ticking, ever. It is really targeted at
virtual machines, whose timer is backed by the host timer, even when the
VM is not running.

Using it on actual hardware is may not be the best idea, specially in
the presence of PM.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread Grygorii Strashko 
Hi Santosh,

On 11/20/2015 07:23 PM, santosh shilimkar wrote:
> + Thomas, Marc
> 
> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>> Now the System stall is observed on TI AM437x based board
>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>> timer is selected as clocksource device - SysRq are working, but
>> nothing else. The reason of stall is that ARM Global timer loses its
>> contexts.
>>
>> The reason of stall is that ARM Global timer loses its contexts during
>> System suspend:
>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>> GT_COUNTERx = 0
>>
>> Hence, update ARM Global timer driver to reflect above behaviour
>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>> - ensure clocksource and clockevent devices have coresponding flags
>>(CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>depending on presence of "always-on" DT property.
>>
> Something which loses context in low power states can't be
> called "always-on"

Sry, it's kinda new area for me and I could make mistakes.

While working on this patch I've:
 - re-used implementation from ARM arch timer 
commit 82a5619410d4c4df65c04272db198eca5a867c18
Author: Lorenzo Pieralisi 
Date:   Tue Apr 8 10:04:32 2014 +0100

clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue


- and followed timekeeping.txt:
"Typically the clock source is a monotonic, atomic counter which will provide
 n bits which count from 0 to 2^(n-1) and then wraps around to 0 and start over.
It will ideally NEVER stop ticking as long as the system is running. It
may stop during system suspend."
^^

And that exactly my use-case: I'd like to use ARM GT as clocksource
and with CPUIdle = n. But System suspend has to be allowed.


> 
> Also if the clock-soucre can't tick in the low power states
> then that device shouldn't be used as a clock-source.

Agree. clocksource can't (except with suspend). Have I missed something? 

> 
>> CC: Arnd Bergmann 
>> Cc: John Stultz 
>> Cc: Felipe Balbi 
>> Cc: Tony Lindgren 
>> Cc: Santosh Shilimkar 
>> Signed-off-by: Grygorii Strashko 
>> ---
>> Changes in v2:
>>   - suspend/resume simplified: nothing is stored any more and
>> ARM GT just re-enabled
>> Link on v1:
>>   https://lkml.org/lkml/2015/11/13/456
>>
>>   drivers/clocksource/arm_global_timer.c | 16 
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/clocksource/arm_global_timer.c 
>> b/drivers/clocksource/arm_global_timer.c
>> index a2cb6fa..867e546 100644
>> --- a/drivers/clocksource/arm_global_timer.c
>> +++ b/drivers/clocksource/arm_global_timer.c
>> @@ -51,6 +51,7 @@ static void __iomem *gt_base;
>>   static unsigned long gt_clk_rate;
>>   static int gt_ppi;
>>   static struct clock_event_device __percpu *gt_evt;
>> +static bool gt_always_on;
>>
>>   /*
>>* To get the value from the Global Timer Counter register proceed 
>> as follows:
>> @@ -168,6 +169,9 @@ static int gt_clockevents_init(struct 
>> clock_event_device *clk)
>>   {
>>   int cpu = smp_processor_id();
>>
>> +if (!gt_always_on)
>> +clk->features |= CLOCK_EVT_FEAT_C3STOP;
>> +
I can drop ^

>>   clk->name = "arm_global_timer";
>>   clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
>>   CLOCK_EVT_FEAT_PERCPU;

and change this as
   clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
   CLOCK_EVT_FEAT_PERCPU | CLOCK_EVT_FEAT_C3STOP;

>> @@ -195,12 +199,19 @@ static cycle_t gt_clocksource_read(struct 
>> clocksource *cs)
>>   return gt_counter_read();
>>   }
>>
>> +static void gt_resume(struct clocksource *cs)
>> +{
>> +/* re-enable timer on resume */
>> +writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
>> +}
>> +
>>   static struct clocksource gt_clocksource = {
>>   .name= "arm_global_timer",
>>   .rating= 300,
>>   .read= gt_clocksource_read,
>>   .mask= CLOCKSOURCE_MASK(64),
>>   .flags= CLOCK_SOURCE_IS_CONTINUOUS,
>> +.resume = gt_resume,
>>   };
>>
>>   #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>> @@ -218,6 +229,9 @@ static void __init gt_clocksource_init(void)
>>   /* enables timer on all the cores */
>>   writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
>>
>> +if (gt_always_on)
>> +gt_clocksource.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP;
>> +

Or, may just drop only this? ^

>>   #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>>   sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate);
>>   #endif
>> @@ -289,6 +303,8 @@ static void __init global_timer_of_register(struct 
>> device_node *np)
>>   goto out_clk;
>>   }
>>
>> +gt_always_on = of_property_read_bool(np, "always-on");
>> +
>>   err = request_percpu_irq(gt_ppi,

Re: [4.4-rc][PATCH] gpio: omap: drop omap1 mpuio specific irq_mask/unmask callbacks

2015-11-20 Thread santosh shilimkar 

On 11/20/2015 5:35 AM, Grygorii Strashko wrote:

Originally OMAP MPUIO GPIO irqchip was implemented using Generic irq
chip, but after set of reworks Generic irq chip code was replaced by
common OMAP GPIO implementation and finally removed by
commit d2d05c65c40e ("gpio: omap: Fix regression for MPUIO interrupts").
Unfortunately, above commit left .irq_mask/unmask callbacks assigned
as below for MPUIO GPIO case:
irqc->irq_mask = irq_gc_mask_set_bit;
irqc->irq_unmask = irq_gc_mask_clr_bit;

This now causes boot failure on OMAP1 platforms, after
commit 450fa54cfd66 ("gpio: omap: convert to use generic irq handler")
which forces these callbacks to be called during GPIO IRQs mapping
from gpiochip_irq_map:

Unable to handle kernel NULL pointer dereference at virtual address 
pgd = c0004000
[] *pgd=
Internal error: Oops: 75 [#1] ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 
4.4.0-rc1-e3-los_afe0c+-2-g25379c0-dirty #1
Hardware name: Amstrad E3 (Delta)
task: c1836000 ti: c1838000 task.ti: c1838000
PC is at irq_gc_mask_set_bit+0x1c/0x60
LR is at __irq_do_set_handler+0x118/0x15c
pc : []lr : []psr: 60d3
sp : c1839c90  ip : c1862c64  fp : c1839c9c
r10:   r9 : c0411950  r8 : c0411bbc
r7 :   r6 : c185c310  r5 : c00444e8  r4 : c185c300
r3 : c1854b50  r2 :   r1 :   r0 : c185c310
Flags: nZCv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: 317f  Table: 10004000  DAC: 0057
Process swapper (pid: 1, stack limit = 0xc1838190)
Stack: (0xc1839c90 to 0xc183a000)

[...]

Backtrace:
[] (irq_gc_mask_set_bit) from [] 
(__irq_do_set_handler+0x118/0x15c)
[] (__irq_do_set_handler) from [] 
(__irq_set_handler+0x44/0x5c)
  r6: r5:c00444e8 r4:c185c300
[] (__irq_set_handler) from [] 
(irq_set_chip_and_handler_name+0x30/0x34)
  r7:0050 r6: r5:c00444e8 r4:0050
[] (irq_set_chip_and_handler_name) from [] 
(gpiochip_irq_map+0x3c/0x8c)
  r7:0050 r6: r5:0050 r4:c1862c64
[] (gpiochip_irq_map) from [] 
(irq_domain_associate+0x7c/0x1c4)
  r5:c185c310 r4:c185cb00
[] (irq_domain_associate) from [] 
(irq_domain_add_simple+0x98/0xc0)
  r8:c0411bbc r7:c185cb00 r6:0050 r5:0010 r4:0001
[] (irq_domain_add_simple) from [] 
(_gpiochip_irqchip_add+0x64/0x10c)
  r7:c1862c64 r6:c0419280 r5:c1862c64 r4:c1854b50
[] (_gpiochip_irqchip_add) from [] 
(omap_gpio_probe+0x2fc/0x63c)
  r5:c1854b50 r4:c1862c10
[] (omap_gpio_probe) from [] (platform_drv_probe+0x2c/0x64)
  r10: r9:c03e45e8 r8: r7:c0419294 r6:c0411984 r5:c0419294
  r4:c0411950
[] (platform_drv_probe) from [] (really_probe+0x160/0x29c)

Hence, fix it by remove obsolete callbacks assignment. After this
change  omap_gpio_mask_irq()/omap_gpio_unmask_irq() will be used
for MPUIO IRQs masking, but this now happens anyway from
omap_gpio_irq_startup/shutdown().

Cc: Tony Lindgren 
Fixes: commit d2d05c65c40e ("gpio: omap: Fix regression for MPUIO interrupts")
Reported-by: Aaro Koskinen 
Signed-off-by: Grygorii Strashko 
---

Acked-by: Santosh Shilimkar 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread santosh shilimkar 

+ Thomas, Marc

On 11/20/2015 5:57 AM, Grygorii Strashko wrote:

Now the System stall is observed on TI AM437x based board
(am437x-gp-evm) during resuming from System suspend when ARM Global
timer is selected as clocksource device - SysRq are working, but
nothing else. The reason of stall is that ARM Global timer loses its
contexts.

The reason of stall is that ARM Global timer loses its contexts during
System suspend:
GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
GT_COUNTERx = 0

Hence, update ARM Global timer driver to reflect above behaviour
- re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
- ensure clocksource and clockevent devices have coresponding flags
   (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
   depending on presence of "always-on" DT property.


Something which loses context in low power states can't be
called "always-on"

Also if the clock-soucre can't tick in the low power states
then that device shouldn't be used as a clock-source.


CC: Arnd Bergmann 
Cc: John Stultz 
Cc: Felipe Balbi 
Cc: Tony Lindgren 
Cc: Santosh Shilimkar 
Signed-off-by: Grygorii Strashko 
---
Changes in v2:
  - suspend/resume simplified: nothing is stored any more and
ARM GT just re-enabled
Link on v1:
  https://lkml.org/lkml/2015/11/13/456

  drivers/clocksource/arm_global_timer.c | 16 
  1 file changed, 16 insertions(+)

diff --git a/drivers/clocksource/arm_global_timer.c 
b/drivers/clocksource/arm_global_timer.c
index a2cb6fa..867e546 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -51,6 +51,7 @@ static void __iomem *gt_base;
  static unsigned long gt_clk_rate;
  static int gt_ppi;
  static struct clock_event_device __percpu *gt_evt;
+static bool gt_always_on;

  /*
   * To get the value from the Global Timer Counter register proceed as follows:
@@ -168,6 +169,9 @@ static int gt_clockevents_init(struct clock_event_device 
*clk)
  {
int cpu = smp_processor_id();

+   if (!gt_always_on)
+   clk->features |= CLOCK_EVT_FEAT_C3STOP;
+
clk->name = "arm_global_timer";
clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
CLOCK_EVT_FEAT_PERCPU;
@@ -195,12 +199,19 @@ static cycle_t gt_clocksource_read(struct clocksource *cs)
return gt_counter_read();
  }

+static void gt_resume(struct clocksource *cs)
+{
+   /* re-enable timer on resume */
+   writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
+}
+
  static struct clocksource gt_clocksource = {
.name   = "arm_global_timer",
.rating = 300,
.read   = gt_clocksource_read,
.mask   = CLOCKSOURCE_MASK(64),
.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+   .resume = gt_resume,
  };

  #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
@@ -218,6 +229,9 @@ static void __init gt_clocksource_init(void)
/* enables timer on all the cores */
writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);

+   if (gt_always_on)
+   gt_clocksource.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP;
+
  #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate);
  #endif
@@ -289,6 +303,8 @@ static void __init global_timer_of_register(struct 
device_node *np)
goto out_clk;
}

+   gt_always_on = of_property_read_bool(np, "always-on");
+
err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt,
 "gt", gt_evt);
if (err) {


Am really confused with this patch. Because 'C3STOP' is a clock-event
flag which we use for cases where we have back-up broadcast timer which
continue to tick in low power states.

If the ARM global timer is considered as that device which actually
doesn't tick then we are doomed. Clocksource device must be *CONTINUOUS*
and if it is not then its not a viable device to be used as clock-source.

May be am missing the context but this whole patch doesn't make sense
to me.

Regards,
Santosh


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread santosh shilimkar 

On 11/20/2015 10:46 AM, Marc Zyngier wrote:

On 20/11/15 18:35, Grygorii Strashko wrote:

Hi Santosh,

On 11/20/2015 07:23 PM, santosh shilimkar wrote:

+ Thomas, Marc

On 11/20/2015 5:57 AM, Grygorii Strashko wrote:

Now the System stall is observed on TI AM437x based board
(am437x-gp-evm) during resuming from System suspend when ARM Global
timer is selected as clocksource device - SysRq are working, but
nothing else. The reason of stall is that ARM Global timer loses its
contexts.

The reason of stall is that ARM Global timer loses its contexts during
System suspend:
 GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
 GT_COUNTERx = 0

Hence, update ARM Global timer driver to reflect above behaviour
- re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
- ensure clocksource and clockevent devices have coresponding flags
(CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
depending on presence of "always-on" DT property.


Something which loses context in low power states can't be
called "always-on"


Sry, it's kinda new area for me and I could make mistakes.

While working on this patch I've:
  - re-used implementation from ARM arch timer
commit 82a5619410d4c4df65c04272db198eca5a867c18
Author: Lorenzo Pieralisi 
Date:   Tue Apr 8 10:04:32 2014 +0100

 clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue


[...]

This patch has a very specific purpose: instructing the core code that
this timer will never stop ticking, ever. It is really targeted at
virtual machines, whose timer is backed by the host timer, even when the
VM is not running.

Using it on actual hardware is may not be the best idea, specially in
the presence of PM.


Exactly. Thanks for clarifying the commit Marc.

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread John Stultz 
On Fri, Nov 20, 2015 at 10:35 AM, Grygorii Strashko
 wrote:
> Hi Santosh,
>
> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
>> + Thomas, Marc
>>
>> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>>> Now the System stall is observed on TI AM437x based board
>>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>>> timer is selected as clocksource device - SysRq are working, but
>>> nothing else. The reason of stall is that ARM Global timer loses its
>>> contexts.
>>>
>>> The reason of stall is that ARM Global timer loses its contexts during
>>> System suspend:
>>> GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>> GT_COUNTERx = 0
>>>
>>> Hence, update ARM Global timer driver to reflect above behaviour
>>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>>> - ensure clocksource and clockevent devices have coresponding flags
>>>(CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>>depending on presence of "always-on" DT property.
>>>
>> Something which loses context in low power states can't be
>> called "always-on"
>
> Sry, it's kinda new area for me and I could make mistakes.
>
> While working on this patch I've:
>  - re-used implementation from ARM arch timer
> commit 82a5619410d4c4df65c04272db198eca5a867c18
> Author: Lorenzo Pieralisi 
> Date:   Tue Apr 8 10:04:32 2014 +0100
>
> clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue
>
>
> - and followed timekeeping.txt:
> "Typically the clock source is a monotonic, atomic counter which will provide
>  n bits which count from 0 to 2^(n-1) and then wraps around to 0 and start 
> over.
> It will ideally NEVER stop ticking as long as the system is running. It
> may stop during system suspend."
> ^^
>
> And that exactly my use-case: I'd like to use ARM GT as clocksource
> and with CPUIdle = n. But System suspend has to be allowed.
>
>
>>
>> Also if the clock-soucre can't tick in the low power states
>> then that device shouldn't be used as a clock-source.
>
> Agree. clocksource can't (except with suspend). Have I missed something?

I think the point Stantosh is making is that if the clocksource stops
in suspend (which is allowed) you should not be setting
CLOCK_SOURCE_SUSPEND_NONSTOP (which promises the clocksource doesn't
stop in suspend, so it can be used for suspend timing as well).

The contradictory part in your patch is that you're also setting the
clockevent logic as  CLOCK_EVT_FEAT_C3STOP, which flags that the
clockevent hardware might stop in low-power idle states (ie: not
suspend, but while the system is running).  Usually hardware that
halts in low-power mode (like the apic on some x86 machines) is not
also used for timekeeping (instead we use the hpet/acpi there).

So its unlikely that the hardware both stays running through suspend,
but also might halt in idle. That would be "unique".

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread John Stultz 
On Fri, Nov 20, 2015 at 11:28 AM, Grygorii Strashko
 wrote:
> On 11/20/2015 09:09 PM, John Stultz wrote:
>> On Fri, Nov 20, 2015 at 10:35 AM, Grygorii Strashko
>>  wrote:
>>> Hi Santosh,
>>>
>>> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
 + Thomas, Marc

 On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
> Now the System stall is observed on TI AM437x based board
> (am437x-gp-evm) during resuming from System suspend when ARM Global
> timer is selected as clocksource device - SysRq are working, but
> nothing else. The reason of stall is that ARM Global timer loses its
> contexts.
>
> The reason of stall is that ARM Global timer loses its contexts during
> System suspend:
>  GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>  GT_COUNTERx = 0
>
> Hence, update ARM Global timer driver to reflect above behaviour
> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
> - ensure clocksource and clockevent devices have coresponding flags
> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
> depending on presence of "always-on" DT property.
>
 Something which loses context in low power states can't be
 called "always-on"
>>>
>>> Sry, it's kinda new area for me and I could make mistakes.
>>>
>>> While working on this patch I've:
>>>   - re-used implementation from ARM arch timer
>>> commit 82a5619410d4c4df65c04272db198eca5a867c18
>>> Author: Lorenzo Pieralisi 
>>> Date:   Tue Apr 8 10:04:32 2014 +0100
>>>
>>>  clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection 
>>> issue
>>>
>>>
>>> - and followed timekeeping.txt:
>>> "Typically the clock source is a monotonic, atomic counter which will 
>>> provide
>>>   n bits which count from 0 to 2^(n-1) and then wraps around to 0 and start 
>>> over.
>>> It will ideally NEVER stop ticking as long as the system is running. It
>>> may stop during system suspend."
>>> ^^
>>>
>>> And that exactly my use-case: I'd like to use ARM GT as clocksource
>>> and with CPUIdle = n. But System suspend has to be allowed.
>>>
>>>

 Also if the clock-soucre can't tick in the low power states
 then that device shouldn't be used as a clock-source.
>>>
>>> Agree. clocksource can't (except with suspend). Have I missed something?
>>
>> I think the point Stantosh is making is that if the clocksource stops
>> in suspend (which is allowed) you should not be setting
>> CLOCK_SOURCE_SUSPEND_NONSTOP (which promises the clocksource doesn't
>> stop in suspend, so it can be used for suspend timing as well).
>>
>
> Ok. Thanks. I got it now. Adding CLOCK_SOURCE_SUSPEND_NONSTOP is mistake.
>
>> The contradictory part in your patch is that you're also setting the
>> clockevent logic as  CLOCK_EVT_FEAT_C3STOP, which flags that the
>> clockevent hardware might stop in low-power idle states (ie: not
>> suspend, but while the system is running).  Usually hardware that
>> halts in low-power mode (like the apic on some x86 machines) is not
>> also used for timekeeping (instead we use the hpet/acpi there).
>
> Sry, I've set CLOCK_EVT_FEAT_C3STOP if "always-on" = *false*

You might also consider renaming that value from always_on to
something more descriptive, given the subtlety of the different states
here. Maybe instead use a flag called halts_in_idle or something?

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [4.4-rc][PATCH] gpio: omap: drop omap1 mpuio specific irq_mask/unmask callbacks

2015-11-20 Thread Aaro Koskinen 
Hi,

On Fri, Nov 20, 2015 at 03:35:14PM +0200, Grygorii Strashko wrote:
> Originally OMAP MPUIO GPIO irqchip was implemented using Generic irq
> chip, but after set of reworks Generic irq chip code was replaced by
> common OMAP GPIO implementation and finally removed by
> commit d2d05c65c40e ("gpio: omap: Fix regression for MPUIO interrupts").
> Unfortunately, above commit left .irq_mask/unmask callbacks assigned
> as below for MPUIO GPIO case:
>   irqc->irq_mask = irq_gc_mask_set_bit;
>   irqc->irq_unmask = irq_gc_mask_clr_bit;
> 
> This now causes boot failure on OMAP1 platforms, after
> commit 450fa54cfd66 ("gpio: omap: convert to use generic irq handler")
> which forces these callbacks to be called during GPIO IRQs mapping
> from gpiochip_irq_map:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 
> pgd = c0004000
> [] *pgd=
> Internal error: Oops: 75 [#1] ARM
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 
> 4.4.0-rc1-e3-los_afe0c+-2-g25379c0-dirty #1
> Hardware name: Amstrad E3 (Delta)
> task: c1836000 ti: c1838000 task.ti: c1838000
> PC is at irq_gc_mask_set_bit+0x1c/0x60
> LR is at __irq_do_set_handler+0x118/0x15c
> pc : []lr : []psr: 60d3
> sp : c1839c90  ip : c1862c64  fp : c1839c9c
> r10:   r9 : c0411950  r8 : c0411bbc
> r7 :   r6 : c185c310  r5 : c00444e8  r4 : c185c300
> r3 : c1854b50  r2 :   r1 :   r0 : c185c310
> Flags: nZCv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> Control: 317f  Table: 10004000  DAC: 0057
> Process swapper (pid: 1, stack limit = 0xc1838190)
> Stack: (0xc1839c90 to 0xc183a000)
> 
> [...]
> 
> Backtrace:
> [] (irq_gc_mask_set_bit) from [] 
> (__irq_do_set_handler+0x118/0x15c)
> [] (__irq_do_set_handler) from [] 
> (__irq_set_handler+0x44/0x5c)
>  r6: r5:c00444e8 r4:c185c300
> [] (__irq_set_handler) from [] 
> (irq_set_chip_and_handler_name+0x30/0x34)
>  r7:0050 r6: r5:c00444e8 r4:0050
> [] (irq_set_chip_and_handler_name) from [] 
> (gpiochip_irq_map+0x3c/0x8c)
>  r7:0050 r6: r5:0050 r4:c1862c64
> [] (gpiochip_irq_map) from [] 
> (irq_domain_associate+0x7c/0x1c4)
>  r5:c185c310 r4:c185cb00
> [] (irq_domain_associate) from [] 
> (irq_domain_add_simple+0x98/0xc0)
>  r8:c0411bbc r7:c185cb00 r6:0050 r5:0010 r4:0001
> [] (irq_domain_add_simple) from [] 
> (_gpiochip_irqchip_add+0x64/0x10c)
>  r7:c1862c64 r6:c0419280 r5:c1862c64 r4:c1854b50
> [] (_gpiochip_irqchip_add) from [] 
> (omap_gpio_probe+0x2fc/0x63c)
>  r5:c1854b50 r4:c1862c10
> [] (omap_gpio_probe) from [] 
> (platform_drv_probe+0x2c/0x64)
>  r10: r9:c03e45e8 r8: r7:c0419294 r6:c0411984 r5:c0419294
>  r4:c0411950
> [] (platform_drv_probe) from [] (really_probe+0x160/0x29c)
> 
> Hence, fix it by remove obsolete callbacks assignment. After this
> changeomap_gpio_mask_irq()/omap_gpio_unmask_irq() will be used
> for MPUIO IRQs masking, but this now happens anyway from
> omap_gpio_irq_startup/shutdown().
> 
> Cc: Tony Lindgren 
> Fixes: commit d2d05c65c40e ("gpio: omap: Fix regression for MPUIO interrupts")
> Reported-by: Aaro Koskinen 
> Signed-off-by: Grygorii Strashko 

Tested-by: Aaro Koskinen 

> ---
>  drivers/gpio/gpio-omap.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 56d2d02..f7fbb46 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1122,8 +1122,6 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, 
> struct irq_chip *irqc)
>   /* MPUIO is a bit different, reading IRQ status clears it */
>   if (bank->is_mpuio) {
>   irqc->irq_ack = dummy_irq_chip.irq_ack;
> - irqc->irq_mask = irq_gc_mask_set_bit;
> - irqc->irq_unmask = irq_gc_mask_clr_bit;
>   if (!bank->regs->wkup_en)
>   irqc->irq_set_wake = NULL;
>   }
> -- 
> 2.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread John Stultz 
On Fri, Nov 20, 2015 at 10:46 AM, Marc Zyngier  wrote:
>
> This patch has a very specific purpose: instructing the core code that
> this timer will never stop ticking, ever. It is really targeted at
> virtual machines, whose timer is backed by the host timer, even when the
> VM is not running.
>
> Using it on actual hardware is may not be the best idea, specially in
> the presence of PM.

And actually, the CLOCK_SOURCE_SUSPEND_NONSTOP flag was added to
support real hardware, so its not just limited to VMs. But that
hardware does have to keep the lights on for the counter backing the
clocksource, and I'm not sure how common it ever became.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread Grygorii Strashko 
On 11/20/2015 09:09 PM, John Stultz wrote:
> On Fri, Nov 20, 2015 at 10:35 AM, Grygorii Strashko
>  wrote:
>> Hi Santosh,
>>
>> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
>>> + Thomas, Marc
>>>
>>> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
 Now the System stall is observed on TI AM437x based board
 (am437x-gp-evm) during resuming from System suspend when ARM Global
 timer is selected as clocksource device - SysRq are working, but
 nothing else. The reason of stall is that ARM Global timer loses its
 contexts.

 The reason of stall is that ARM Global timer loses its contexts during
 System suspend:
  GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
  GT_COUNTERx = 0

 Hence, update ARM Global timer driver to reflect above behaviour
 - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
 - ensure clocksource and clockevent devices have coresponding flags
 (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
 depending on presence of "always-on" DT property.

>>> Something which loses context in low power states can't be
>>> called "always-on"
>>
>> Sry, it's kinda new area for me and I could make mistakes.
>>
>> While working on this patch I've:
>>   - re-used implementation from ARM arch timer
>> commit 82a5619410d4c4df65c04272db198eca5a867c18
>> Author: Lorenzo Pieralisi 
>> Date:   Tue Apr 8 10:04:32 2014 +0100
>>
>>  clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection 
>> issue
>>
>>
>> - and followed timekeeping.txt:
>> "Typically the clock source is a monotonic, atomic counter which will provide
>>   n bits which count from 0 to 2^(n-1) and then wraps around to 0 and start 
>> over.
>> It will ideally NEVER stop ticking as long as the system is running. It
>> may stop during system suspend."
>> ^^
>>
>> And that exactly my use-case: I'd like to use ARM GT as clocksource
>> and with CPUIdle = n. But System suspend has to be allowed.
>>
>>
>>>
>>> Also if the clock-soucre can't tick in the low power states
>>> then that device shouldn't be used as a clock-source.
>>
>> Agree. clocksource can't (except with suspend). Have I missed something?
> 
> I think the point Stantosh is making is that if the clocksource stops
> in suspend (which is allowed) you should not be setting
> CLOCK_SOURCE_SUSPEND_NONSTOP (which promises the clocksource doesn't
> stop in suspend, so it can be used for suspend timing as well).
> 

Ok. Thanks. I got it now. Adding CLOCK_SOURCE_SUSPEND_NONSTOP is mistake.

> The contradictory part in your patch is that you're also setting the
> clockevent logic as  CLOCK_EVT_FEAT_C3STOP, which flags that the
> clockevent hardware might stop in low-power idle states (ie: not
> suspend, but while the system is running).  Usually hardware that
> halts in low-power mode (like the apic on some x86 machines) is not
> also used for timekeeping (instead we use the hpet/acpi there).

Sry, I've set CLOCK_EVT_FEAT_C3STOP if "always-on" = *false*

^ this, I thought, might be valid because it will stop in low-power idle states
  and "always-on" was expected to indicate case when CPUIdle = n (no CPU PM),
  same way as Lorenzo did.
  Would it be correct to just set it always?

and CLOCK_SOURCE_SUSPEND_NONSTOP if "always-on" = *true*

^ this part is mistake

> 
> So its unlikely that the hardware both stays running through suspend,
> but also might halt in idle. That would be "unique".
> 

Thanks a lot.


-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread Grygorii Strashko 
On 11/20/2015 08:52 PM, santosh shilimkar wrote:
> On 11/20/2015 10:46 AM, Marc Zyngier wrote:
>> On 20/11/15 18:35, Grygorii Strashko wrote:
>>> Hi Santosh,
>>>
>>> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
 + Thomas, Marc

 On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
> Now the System stall is observed on TI AM437x based board
> (am437x-gp-evm) during resuming from System suspend when ARM Global
> timer is selected as clocksource device - SysRq are working, but
> nothing else. The reason of stall is that ARM Global timer loses its
> contexts.
>
> The reason of stall is that ARM Global timer loses its contexts during
> System suspend:
>  GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>  GT_COUNTERx = 0
>
> Hence, update ARM Global timer driver to reflect above behaviour
> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
> - ensure clocksource and clockevent devices have coresponding flags
> (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
> depending on presence of "always-on" DT property.
>
 Something which loses context in low power states can't be
 called "always-on"
>>>
>>> Sry, it's kinda new area for me and I could make mistakes.
>>>
>>> While working on this patch I've:
>>>   - re-used implementation from ARM arch timer
>>> commit 82a5619410d4c4df65c04272db198eca5a867c18
>>> Author: Lorenzo Pieralisi 
>>> Date:   Tue Apr 8 10:04:32 2014 +0100
>>>
>>>  clocksource: arch_arm_timer: Fix age-old arch timer C3STOP 
>>> detection issue
>>
>> [...]
>>
>> This patch has a very specific purpose: instructing the core code that
>> this timer will never stop ticking, ever. It is really targeted at
>> virtual machines, whose timer is backed by the host timer, even when the
>> VM is not running.
>>
>> Using it on actual hardware is may not be the best idea, specially in
>> the presence of PM.
>>
> Exactly. Thanks for clarifying the commit Marc.

Thanks for your comments, but could you clarify pls - 
Can i use ARM GT for my use case CPUIDLE=n and SUSPEND=y,
taking into account that System time can be corrected by RTC core
during resume? (it will be used as clocksource only).

And would it be reasonable to resend new version if i will drop 
all "always-on" related code?

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread Thomas Gleixner 
On Fri, 20 Nov 2015, John Stultz wrote:
> So its unlikely that the hardware both stays running through suspend,
> but also might halt in idle. That would be "unique".

The amount of creativity put into the next variants of differently
broken timers is amazing. So I wouldn't be too surprised if such a
thing actually surfaces.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

2015-11-20 Thread Grygorii Strashko 
On 11/20/2015 09:32 PM, John Stultz wrote:
> On Fri, Nov 20, 2015 at 11:28 AM, Grygorii Strashko
>  wrote:
>> On 11/20/2015 09:09 PM, John Stultz wrote:
>>> On Fri, Nov 20, 2015 at 10:35 AM, Grygorii Strashko
>>>  wrote:
 Hi Santosh,

 On 11/20/2015 07:23 PM, santosh shilimkar wrote:
> + Thomas, Marc
>
> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>> Now the System stall is observed on TI AM437x based board
>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>> timer is selected as clocksource device - SysRq are working, but
>> nothing else. The reason of stall is that ARM Global timer loses its
>> contexts.
>>
>> The reason of stall is that ARM Global timer loses its contexts during
>> System suspend:
>>   GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>   GT_COUNTERx = 0
>>
>> Hence, update ARM Global timer driver to reflect above behaviour
>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>> - ensure clocksource and clockevent devices have coresponding flags
>>  (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>  depending on presence of "always-on" DT property.
>>
> Something which loses context in low power states can't be
> called "always-on"

 Sry, it's kinda new area for me and I could make mistakes.

 While working on this patch I've:
- re-used implementation from ARM arch timer
 commit 82a5619410d4c4df65c04272db198eca5a867c18
 Author: Lorenzo Pieralisi 
 Date:   Tue Apr 8 10:04:32 2014 +0100

   clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection 
 issue


 - and followed timekeeping.txt:
 "Typically the clock source is a monotonic, atomic counter which will 
 provide
n bits which count from 0 to 2^(n-1) and then wraps around to 0 and 
 start over.
 It will ideally NEVER stop ticking as long as the system is running. It
 may stop during system suspend."
 ^^

 And that exactly my use-case: I'd like to use ARM GT as clocksource
 and with CPUIdle = n. But System suspend has to be allowed.


>
> Also if the clock-soucre can't tick in the low power states
> then that device shouldn't be used as a clock-source.

 Agree. clocksource can't (except with suspend). Have I missed something?
>>>
>>> I think the point Stantosh is making is that if the clocksource stops
>>> in suspend (which is allowed) you should not be setting
>>> CLOCK_SOURCE_SUSPEND_NONSTOP (which promises the clocksource doesn't
>>> stop in suspend, so it can be used for suspend timing as well).
>>>
>>
>> Ok. Thanks. I got it now. Adding CLOCK_SOURCE_SUSPEND_NONSTOP is mistake.
>>
>>> The contradictory part in your patch is that you're also setting the
>>> clockevent logic as  CLOCK_EVT_FEAT_C3STOP, which flags that the
>>> clockevent hardware might stop in low-power idle states (ie: not
>>> suspend, but while the system is running).  Usually hardware that
>>> halts in low-power mode (like the apic on some x86 machines) is not
>>> also used for timekeeping (instead we use the hpet/acpi there).
>>
>> Sry, I've set CLOCK_EVT_FEAT_C3STOP if "always-on" = *false*
> 
> You might also consider renaming that value from always_on to
> something more descriptive, given the subtlety of the different states
> here. Maybe instead use a flag called halts_in_idle or something?
> 

I'd better just drop it for now hence I'm still not sure what is more reasonable
continue with DT property or just add this flag always.

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clk: ti: dra7: constify clk_hw_omap_ops structure

2015-11-20 Thread Stephen Boyd 
On 11/15, Julia Lawall wrote:
> The clk_hw_omap_ops structures are never modified, so declare this one as
> const, like the others.
> 
> Done with the help of Coccinelle.
> 
> Signed-off-by: Julia Lawall 
> 
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html