RE: [GIT PULL 3/5] Samsung exynos cpuidle update for v3.17
Kevin Hilman wrote: > > Hello, > Hi, > On Fri, Jul 18, 2014 at 5:52 PM, Kukjin Kim wrote: > > The following changes since commit 1795cd9b3a91d4b5473c97f491d63892442212ab: > > > > Linux 3.16-rc5 (2014-07-13 14:04:33 -0700) > > > > are available in the git repository at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > > tags/exynos-cpuidle > > > > for you to fetch changes up to fc2cac41ebbfb16da8b036cba6ec6714ab780a6d: > > > > ARM: EXYNOS: populate suspend and powered_up callbacks for mcpm > > (2014-07-19 03:36:00 +0900) > > > > > > exynos cpuidle update for v3.17 > > > > - add callbacks exynos_suspend() and exynos_powered_up() > > for support cpuidle through mcpm > > - skip exynos_cpuidle for exynos5420 because is uses > > cpuidle-big-liggle generic cpuidle driver > > - add generic functions to calculate cpu number is used > > for pmu and this is required for exynos5420 multi-cluster > > - add of_device_id structure for big.LITTLE cpuidle and > > add "samsung,exynos5420" compatible string for exynos5420 > > I'm curious what platforms this is expected to work on, and where it's > been tested. > exynos5420 and its reference board, smdk5420. > I tried it on exynos5800-peach-pi (chromebook2) and it hangs up (no > kernel messages) shortly after the driver loads, and never finishes to > boot. > Oh, sorry about that, if so, it should be fixed before -rc1. Just note that I thought since exynos5800 is very similar with exynos5420 so I didn't ask Chander to test the series on exynos5800 based board...Just asked to test with Nico's mcpm series. BTW, unfortunately I have no exynos5800-peach-pi... > I needed an extra compatible entry for the 5800 in the driver for the > driver to load, which suggests it hasn't been tested on 5800, but at > least in theory, this should be compatible with the 542x, right? > Yes I think so. As you can see in exynos5800.dtsi, it is including exynos5420. > For testing, I tried today's linux-next and arm-soc/for-next. > Hmm...why is the problem happened at last? Not at that time? :( I need to figure it out... Thanks, Kukjin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
Javier, On Fri, Aug 8, 2014 at 3:26 PM, Javier Martinez Canillas wrote: >> I have some vague recollection that I set interrupts to pin-function >> "0" by default for some reason (assuming they would go to 0xf when >> interrupts were enabled). ...but I can't for the life of me remember >> if it was a good reason or just seemed like the right thing to do. >> > > It would be great to know if there is a good reason. I see indeed that all > pinctrl setup in the Peach Pit/Pi and Snow DTS that involves interrupts are > using 0x0 as the pin function. Since as Tomasz explained Samsung SoC makes a > difference between GPIO-IRQ and GPIO input I guess that it's better to change > those to 0xf instead. What do you think? I think it's worth trying out. If there are no problems with it then let's do it. My vague recollection is that I was worried that pinctrl would take effect right at driver probe time (maybe this used to happen? or maybe I imagined it?) and that configuring to 0xf at this point in time would cause a spurious interrupt. I can't remember ever testing it so it was probably just something I imagined. Even if it was configured as 0xf I'd imagine that the interrupt would be masked anyway so there should be no spurious interrupt, right? > Regardless of this though I think that both the patch to move the IRQ > pinmux setup from .irq_set_type to the .irq_request_resources and the patch to > to prevent any pinmux reconfiguration are good improvements to avoid future > issues like the one we found here. OK. I'll let you, Tomasz, and Linus figure out what's best here since I haven't done extensive thinking on it. ;) -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs
From: Tomasz Figa
Currently after configuring a GPIO pin as an interrupt related pinmux
registers are changed, but there is no protection from calling
gpio_direction_*() in a badly written driver, which would cause the same
pinmux register to be reconfigured for regular input/output and this
disabling interrupt capability of the pin.
This patch addresses this issue by moving pinmux reconfiguration to
.irq_{request,release}_resources() callback of irq_chip and calling
gpio_lock_as_irq() helper to prevent reconfiguration of pin direction.
Setting up a GPIO interrupt on Samsung SoCs is a two-step operation -
in addition to trigger configuration in a dedicated register, the pinmux
must be also reconfigured to GPIO interrupt, which is a different function
than normal GPIO input, although I/O-wise they both behave in the same way
and gpio_get_value() can be used on a pin configured as IRQ as well.
Such design implies subtleties such as gpio_direction_input() not having
to fail if a pin is already configured as an interrupt nor change the
configuration to normal input. But the FLAG_USED_AS_IRQ set in gpiolib by
gpio_lock_as_irq() is only used to check that gpio_direction_output() is
not called, it's not used to prevent gpio_direction_input() to be called.
So this is not a complete solution for Samsung SoCs but it's definitely a
move in the right direction.
Signed-off-by: Tomasz Figa
[javier: use request resources instead of startup and expand commit message]
Signed-off-by: Javier Martinez Canillas
---
This patch solves the issue explained in https://lkml.org/lkml/2014/8/8/461
drivers/pinctrl/samsung/pinctrl-exynos.c | 69 ---
drivers/pinctrl/samsung/pinctrl-samsung.h | 1 +
2 files changed, 64 insertions(+), 6 deletions(-)
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c
b/drivers/pinctrl/samsung/pinctrl-exynos.c
index 003bfd8..d7154ed 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -127,14 +127,10 @@ static int exynos_irq_set_type(struct irq_data *irqd,
unsigned int type)
struct irq_chip *chip = irq_data_get_irq_chip(irqd);
struct exynos_irq_chip *our_chip = to_exynos_irq_chip(chip);
struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
- struct samsung_pin_bank_type *bank_type = bank->type;
struct samsung_pinctrl_drv_data *d = bank->drvdata;
- unsigned int pin = irqd->hwirq;
- unsigned int shift = EXYNOS_EINT_CON_LEN * pin;
+ unsigned int shift = EXYNOS_EINT_CON_LEN * irqd->hwirq;
unsigned int con, trig_type;
unsigned long reg_con = our_chip->eint_con + bank->eint_offset;
- unsigned long flags;
- unsigned int mask;
switch (type) {
case IRQ_TYPE_EDGE_RISING:
@@ -167,8 +163,32 @@ static int exynos_irq_set_type(struct irq_data *irqd,
unsigned int type)
con |= trig_type << shift;
writel(con, d->virt_base + reg_con);
+ return 0;
+}
+
+static int exynos_irq_request_resources(struct irq_data *irqd)
+{
+ struct irq_chip *chip = irq_data_get_irq_chip(irqd);
+ struct exynos_irq_chip *our_chip = to_exynos_irq_chip(chip);
+ struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
+ struct samsung_pin_bank_type *bank_type = bank->type;
+ struct samsung_pinctrl_drv_data *d = bank->drvdata;
+ unsigned int shift = EXYNOS_EINT_CON_LEN * irqd->hwirq;
+ unsigned long reg_con = our_chip->eint_con + bank->eint_offset;
+ unsigned long flags;
+ unsigned int mask;
+ unsigned int con;
+ int ret;
+
+ ret = gpio_lock_as_irq(&bank->gpio_chip, irqd->hwirq);
+ if (ret) {
+ dev_err(bank->gpio_chip.dev, "unable to lock pin %s-%lu IRQ\n",
+ bank->name, irqd->hwirq);
+ return ret;
+ }
+
reg_con = bank->pctl_offset + bank_type->reg_offset[PINCFG_TYPE_FUNC];
- shift = pin * bank_type->fld_width[PINCFG_TYPE_FUNC];
+ shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC];
mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1;
spin_lock_irqsave(&bank->slock, flags);
@@ -180,9 +200,42 @@ static int exynos_irq_set_type(struct irq_data *irqd,
unsigned int type)
spin_unlock_irqrestore(&bank->slock, flags);
+ exynos_irq_unmask(irqd);
+
return 0;
}
+static void exynos_irq_release_resources(struct irq_data *irqd)
+{
+ struct irq_chip *chip = irq_data_get_irq_chip(irqd);
+ struct exynos_irq_chip *our_chip = to_exynos_irq_chip(chip);
+ struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
+ struct samsung_pin_bank_type *bank_type = bank->type;
+ struct samsung_pinctrl_drv_data *d = bank->drvdata;
+ unsigned int shift = EXYNOS_EINT_CON_LEN * irqd->hwirq;
+ unsigned long reg_con = our_chip->eint_con + bank->eint_offset;
+ unsigned long flags;
+
Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
Hello Doug,
On 08/08/2014 10:54 PM, Doug Anderson wrote:
> Hi,
>>
>> To fix the issue a variation of patch [0] will be posted that moves the IRQ
>> pinmux setup from .irq_set_type to the .irq_request_resources function
>> handler.
>> That way the pin will be setup as IRQ regardless of the the trigger type [1]
>> when someone calls request_[threaded]_irq().
>>
>> Only the mentioned patch fixes the issue but Tomasz said that even a call to
>> gpio_direction_{input,output} can change the pin configuration so he will
>> post
>> another patch that will add a bit mask to samsung_pin_bank to prevent any
>> pinmux
>> reconfiguration.
>
> Would just making a device tree change fix this? AKA for the pin, do:
>
> samsung,pin-function = <0xf>;
>
Yes, when configuring the pin as 0xf (GPIO interrupt) instead of 0x0 (GPIO
input) the issue is not present indeed. So after Nick answer my question about
what I got wrong with the "linux,gpio-keymap" property, I will change the pin
function when re-posing the DTS changes for the atmel touchpad.
> I have some vague recollection that I set interrupts to pin-function
> "0" by default for some reason (assuming they would go to 0xf when
> interrupts were enabled). ...but I can't for the life of me remember
> if it was a good reason or just seemed like the right thing to do.
>
It would be great to know if there is a good reason. I see indeed that all
pinctrl setup in the Peach Pit/Pi and Snow DTS that involves interrupts are
using 0x0 as the pin function. Since as Tomasz explained Samsung SoC makes a
difference between GPIO-IRQ and GPIO input I guess that it's better to change
those to 0xf instead. What do you think?
Regardless of this though I think that both the patch to move the IRQ
pinmux setup from .irq_set_type to the .irq_request_resources and the patch to
to prevent any pinmux reconfiguration are good improvements to avoid future
issues like the one we found here.
> -Doug
>
Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL 3/5] Samsung exynos cpuidle update for v3.17
Hello, On Fri, Jul 18, 2014 at 5:52 PM, Kukjin Kim wrote: > The following changes since commit 1795cd9b3a91d4b5473c97f491d63892442212ab: > > Linux 3.16-rc5 (2014-07-13 14:04:33 -0700) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > tags/exynos-cpuidle > > for you to fetch changes up to fc2cac41ebbfb16da8b036cba6ec6714ab780a6d: > > ARM: EXYNOS: populate suspend and powered_up callbacks for mcpm > (2014-07-19 03:36:00 +0900) > > > exynos cpuidle update for v3.17 > > - add callbacks exynos_suspend() and exynos_powered_up() > for support cpuidle through mcpm > - skip exynos_cpuidle for exynos5420 because is uses > cpuidle-big-liggle generic cpuidle driver > - add generic functions to calculate cpu number is used > for pmu and this is required for exynos5420 multi-cluster > - add of_device_id structure for big.LITTLE cpuidle and > add "samsung,exynos5420" compatible string for exynos5420 I'm curious what platforms this is expected to work on, and where it's been tested. I tried it on exynos5800-peach-pi (chromebook2) and it hangs up (no kernel messages) shortly after the driver loads, and never finishes to boot. I needed an extra compatible entry for the 5800 in the driver for the driver to load, which suggests it hasn't been tested on 5800, but at least in theory, this should be compatible with the 542x, right? For testing, I tried today's linux-next and arm-soc/for-next. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
Hi,
On Fri, Aug 8, 2014 at 11:29 AM, Javier Martinez Canillas
wrote:
> Hello,
>
> On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote:
>>
>>
>> It seems as if the first call to exynos_irq_set_type() that is made by OF is
>> a
>> no-op while the second call is the one that actually setups the hw correctly.
>> Does this make any sense? Maybe is related to the pin not being muxed in the
>> correct function when the "interrupts" property is parsed by OF?
>>
>
> So after a conversation with Tomasz Figa over IRC the problem was after all
> that
> the pin was reconfigured. The IRQ trigger type resulted to be just a red
> herring
> and not a direct cause.
>
> The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type
> function handler. So what happened was that OF parsed the "interrupts"
> property
> and called exynos_irq_set_type() which did the pinmux setup.
>
> But after that, due a DTS pinctrl configuration the pin function was changed
> as
> a GPIO input and that happened before the atmel driver was probed. So when the
> driver called request_threaded_irq(), the correct flags were used but the pin
> was not configured as an IRQ anymore so IRQ were not fired.
>
> Setting a trigger type just had the side effect of calling
> exynos_irq_set_type()
> which again setup the pin as an IRQ.
>
> To fix the issue a variation of patch [0] will be posted that moves the IRQ
> pinmux setup from .irq_set_type to the .irq_request_resources function
> handler.
> That way the pin will be setup as IRQ regardless of the the trigger type [1]
> when someone calls request_[threaded]_irq().
>
> Only the mentioned patch fixes the issue but Tomasz said that even a call to
> gpio_direction_{input,output} can change the pin configuration so he will post
> another patch that will add a bit mask to samsung_pin_bank to prevent any
> pinmux
> reconfiguration.
Would just making a device tree change fix this? AKA for the pin, do:
samsung,pin-function = <0xf>;
I have some vague recollection that I set interrupts to pin-function
"0" by default for some reason (assuming they would go to 0xf when
interrupts were enabled). ...but I can't for the life of me remember
if it was a good reason or just seemed like the right thing to do.
-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
+CC Linus, as this became also pinctrl related.
On 08.08.2014 20:29, Javier Martinez Canillas wrote:
> Hello,
>
> On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote:
>>
>>
>> It seems as if the first call to exynos_irq_set_type() that is made by OF is
>> a
>> no-op while the second call is the one that actually setups the hw correctly.
>> Does this make any sense? Maybe is related to the pin not being muxed in the
>> correct function when the "interrupts" property is parsed by OF?
>>
>
> So after a conversation with Tomasz Figa over IRC the problem was after all
> that
> the pin was reconfigured. The IRQ trigger type resulted to be just a red
> herring
> and not a direct cause.
>
> The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type
> function handler. So what happened was that OF parsed the "interrupts"
> property
> and called exynos_irq_set_type() which did the pinmux setup.
>
> But after that, due a DTS pinctrl configuration the pin function was changed
> as
> a GPIO input and that happened before the atmel driver was probed. So when the
> driver called request_threaded_irq(), the correct flags were used but the pin
> was not configured as an IRQ anymore so IRQ were not fired.
>
> Setting a trigger type just had the side effect of calling
> exynos_irq_set_type()
> which again setup the pin as an IRQ.
>
> To fix the issue a variation of patch [0] will be posted that moves the IRQ
> pinmux setup from .irq_set_type to the .irq_request_resources function
> handler.
> That way the pin will be setup as IRQ regardless of the the trigger type [1]
> when someone calls request_[threaded]_irq().
>
> Only the mentioned patch fixes the issue but Tomasz said that even a call to
> gpio_direction_{input,output} can change the pin configuration so he will post
> another patch that will add a bit mask to samsung_pin_bank to prevent any
> pinmux
> reconfiguration.
To add a bit more information about the hardware, setting up a GPIO
interrupt on Samsung SoCs is a two-step operation - in addition to
trigger configuration in a dedicated register, the pinmux must be also
reconfigured to GPIO interrupt, which is a different function than
normal GPIO input, although I/O-wise they both behave in the same way
and gpio_get_value() can be used on a pin configured as IRQ as well.
I'm afraid that such design implies that handling of this in the driver
must be done on a very low level, because it involves three possible
interfaces changing the pinmux - pinctrl, GPIO and irqchip and such
subtletes like gpio_direction_input() that shouldn't neither fail if a
pin is already configured as an interrupt nor change the configuration
to normal input.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
Hello,
On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote:
>
>
> It seems as if the first call to exynos_irq_set_type() that is made by OF is a
> no-op while the second call is the one that actually setups the hw correctly.
> Does this make any sense? Maybe is related to the pin not being muxed in the
> correct function when the "interrupts" property is parsed by OF?
>
So after a conversation with Tomasz Figa over IRC the problem was after all that
the pin was reconfigured. The IRQ trigger type resulted to be just a red herring
and not a direct cause.
The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type
function handler. So what happened was that OF parsed the "interrupts" property
and called exynos_irq_set_type() which did the pinmux setup.
But after that, due a DTS pinctrl configuration the pin function was changed as
a GPIO input and that happened before the atmel driver was probed. So when the
driver called request_threaded_irq(), the correct flags were used but the pin
was not configured as an IRQ anymore so IRQ were not fired.
Setting a trigger type just had the side effect of calling exynos_irq_set_type()
which again setup the pin as an IRQ.
To fix the issue a variation of patch [0] will be posted that moves the IRQ
pinmux setup from .irq_set_type to the .irq_request_resources function handler.
That way the pin will be setup as IRQ regardless of the the trigger type [1]
when someone calls request_[threaded]_irq().
Only the mentioned patch fixes the issue but Tomasz said that even a call to
gpio_direction_{input,output} can change the pin configuration so he will post
another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux
reconfiguration.
Thanks a lot and best regards,
Javier
[0]: http://thread.gmane.org/gmane.linux.kernel.samsung-soc/34259/focus=34261
[1]: http://lxr.free-electrons.com/source/kernel/irq/manage.c#L1162
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
+Thomas Gleixner, Jason Cooper, Benjamin Herrenschmidt and Thomas Abraham
Hello Dmitry,
On 08/08/2014 06:21 PM, Dmitry Torokhov wrote:
> On Fri, Aug 08, 2014 at 03:07:33PM +0100, Nick Dyer wrote:
>> On 07/08/14 08:44, Javier Martinez Canillas wrote:
>> > The Atmel maXTouch driver assumed that the IRQ type flags will
>> > always be passed using platform data but this is not true when
>> > booting using Device Trees. In these setups the interrupt type
>> > was ignored by the driver when requesting an IRQ.
>> >
>> > This means that it will fail if a machine specified other type
>> > than IRQ_TYPE_NONE. The right approach is to get the IRQ flags
>> > that was parsed by OF from the "interrupt" Device Tree propery.
>> >
>> > Signed-off-by: Javier Martinez Canillas
>>
>> I'm happy for this to go in if Dmitry will accept it. It does seem to be a
>> quirk of some platforms that it is necessary, but it's only one line.
>
> I'd rather not as it masks the deeper platform issue. There might be
> other drovers also expecting platform/OF code set up interrupt triggers
> and working/not working by chance.
>
I totally agree. When posted the patch I thought that it was the right fix but
after your explanation and studying the IRQ core I see that as you said this i
just hiding a more fundamental issue. We should fix the root cause instead of
adding a workaround for every driver.
> Can we figure out why the platform in question needs this change?
>
I dig further on this. First I wanted to see if the problem was on IRQ core or
in the irqchip driver so I tried calling the chip's .irq_set_type function
handler directly using the flags set by __irq_set_trigger() the first time that
it's called from OF when the "interrupts" property is parsed.
Doing that makes the device to trigger interrupts so the problem seems to be
related to the pinctrl-exynos driver and is not in the IRQ core.
So, this is the change I did just for testing purposes to make it work again:
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 3dc6a61..ed76b25 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1176,6 +1176,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
struct irqaction *new)
if (ret)
goto out_mask;
+ } else if (irq == 283 /* mapped IRQ number for touchpad */) {
+ struct irq_chip *chip = desc->irq_data.chip;
+ chip->irq_set_type(&desc->irq_data,
+ irq_get_trigger_type(irq));
}
desc->istate &= ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \
It seems as if the first call to exynos_irq_set_type() that is made by OF is a
no-op while the second call is the one that actually setups the hw correctly.
Does this make any sense? Maybe is related to the pin not being muxed in the
correct function when the "interrupts" property is parsed by OF?
Now I added some debug logs to see what could be different but all the variables
have the same values in both cases:
When called happens due irq_parse_and_map():
irq 283 type 2 pin 1 shift 4 base 4026679296 reg_con 3588 con 32 flags 0 mask 0
When called happens due request_threaded_irq():
irq 283 type 2 pin 1 shift 4 base 4026679296 reg_con 3588 con 32 flags 0 mask 0
I would really appreciate if someone that is more familiar with the driver or
this chip can provide some hints.
Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
+CC Thomas, Jason and Ben
On 08.08.2014 15:24, Javier Martinez Canillas wrote:
> Hello,
>
> On 08/07/2014 06:47 PM, Dmitry Torokhov wrote:
>>
>> Actually, I take this back. In mainline everything as it should: if
>> pdata does not specify particular trigger the flags requested end up
>> being IRQF_ONESHOT, which should preserve trigger bits previously set up
>> by the board or OF code. In Chrome kernel we have:
>>
>
> In theory it should work as Dmitry and Nick said since if no trigger flags are
> set then whatever was set before (by OF, platform code, ACPI) should be used.
>
> I also verified what Tomasz mentioned that the IRQ trigger type is parsed and
> set correctly by OF:
>
> irq_of_parse_and_map()
>irq_create_of_mapping()
> irq_set_irq_type()
> __irq_set_trigger()
> chip->irq_set_type()
> exynos_irq_set_type()
>
> But for some reason it doesn't work for me unless I set the trigger type in
> the
> flags passed to request_threaded_irq().
>
> I found that what makes it to work is the logic in __setup_irq() [0] that Nick
> pointed out on a previous email:
>
> /* Setup the type (level, edge polarity) if configured: */
> if (new->flags & IRQF_TRIGGER_MASK) {
> ret = __irq_set_trigger(desc, irq,
> new->flags & IRQF_TRIGGER_MASK);
>
> if (ret)
> goto out_mask;
> }
>
> So __irq_set_trigger() is only executed if the struct irqaction flags has a
> trigger flag which makes sense since this shouldn't be necessary with DT due
> __irq_set_trigger() being called from irq_create_of_mapping() before when the
> "interrupts" property is parsed.
>
> But for some reason it is necessary for me... I checked that struct irq_data
> state_use_accessors value is correct and even tried setting that value to
> new->flags after the mentioned code block but it makes no difference. Input
> events are not reported by evtest and AFAICT interrupts are not being
> generated.
>
> It works though if the trigger type is passed to request_threaded_irq() like
> $subject does or if new->flags are set before the new->flags &
> IRQF_TRIGGER_MASK
> conditional.
>
> For example, with the following changes interrupts are fired correctly:
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 3dc6a61..2d8adbb 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1169,6 +1169,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
> struct irqaction *new)
>
> init_waitqueue_head(&desc->wait_for_threads);
>
> + if (!(new->flags & IRQF_TRIGGER_MASK))
> + new->flags |= irqd_get_trigger_type(desc);
> +
> /* Setup the type (level, edge polarity) if configured: */
> if (new->flags & IRQF_TRIGGER_MASK) {
> ret = __irq_set_trigger(desc, irq,
>
> Any ideas what could be wrong here?
>
>> /* Default to falling edge if no platform data provided */
>> irqflags = data->pdata ? data->pdata->irqflags : IRQF_TRIGGER_FALLING;
>> error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
>> irqflags | IRQF_ONESHOT,
>> client->name, data);
>>
>
> Exactly, that's how I found the issue. When I compared both drivers I noticed
> that the Chrome OS driver did that and since all the supported platforms are
> DT
> based, the above is equivalent to just IRQF_TRIGGER_FALLING | IRQF_ONESHOT.
>
> So according to my explanation, new->flags & IRQF_TRIGGER_MASK is true so
> __irq_set_trigger() is executed and that's why it works with the Chrome OS
> driver.
>
> In fact the Chrome OS DTS does not set a trigger type in the "interrupts"
> property:
>
> trackpad@4b {
> reg=<0x4b>;
> compatible="atmel,atmel_mxt_tp";
> interrupts=<1 0>;
> interrupt-parent=<&gpx1>;
> wakeup-source;
> };
>
>
>> which I believe should go away. If it is needed on ACPI systems we need
>> to figure out how to do things we can do with OF there.
>>
>
> The above code should not be related to ACPI systems since whatever code that
> parses an ACPI table should just call irq_set_irq_type() like is made by OF,
> so
> request_threaded_irq() should just work with ACPI too.
>
> I agree it should go away but first I want to understand why is needed in the
> first place. Unfortunately commit:
>
> 031f136 ("Input: atmel_mxt_ts - Set default irqflags when there is no pdata")
>
> from the Chrome OS 3.8 does not explain why this is needed, instead of adding
> this information in the DTS (e.g: interrupts=<1 IRQ_TYPE_EDGE_FALLING>).
>
>> Thanks.
>>
>
> Best regards,
> Javier
>
> [0]: http://lxr.free-electrons.com/source/kernel/irq/manage.c#L1172
>
--
Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
On Fri, Aug 08, 2014 at 03:07:33PM +0100, Nick Dyer wrote: > On 07/08/14 08:44, Javier Martinez Canillas wrote: > > The Atmel maXTouch driver assumed that the IRQ type flags will > > always be passed using platform data but this is not true when > > booting using Device Trees. In these setups the interrupt type > > was ignored by the driver when requesting an IRQ. > > > > This means that it will fail if a machine specified other type > > than IRQ_TYPE_NONE. The right approach is to get the IRQ flags > > that was parsed by OF from the "interrupt" Device Tree propery. > > > > Signed-off-by: Javier Martinez Canillas > > I'm happy for this to go in if Dmitry will accept it. It does seem to be a > quirk of some platforms that it is necessary, but it's only one line. I'd rather not as it masks the deeper platform issue. There might be other drovers also expecting platform/OF code set up interrupt triggers and working/not working by chance. Can we figure out why the platform in question needs this change? Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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] Input: atmel_mxt_ts - Add keycodes array example
Hello Nick, On 08/07/2014 02:38 PM, Nick Dyer wrote: >> >> Also, the current documentation says that the array limit >> is four entries but the driver dynamically allocates the >> keymap array and does not limit the array size. > > There is a physical limit to the number of GPIOs on the device. The number > 4 is wrong, the protocol does allow for up to 8 GPIOs. But it is a hard limit. > Thanks a lot for the explanation, then I guess s/4/8 is enough. >> >> Signed-off-by: Javier Martinez Canillas >> --- >> Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 8 +++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt >> b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt >> index baef432..be50476 100644 >> --- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt >> +++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt >> @@ -11,7 +11,7 @@ Required properties: >> >> Optional properties for main touchpad device: >> >> -- linux,gpio-keymap: An array of up to 4 entries indicating the Linux >> +- linux,gpio-keymap: An array of entries indicating the Linux >> keycode generated by each GPIO. Linux keycodes are defined in >> . >> >> @@ -22,4 +22,10 @@ Example: >> reg = <0x4b>; >> interrupt-parent = <&gpio>; >> interrupts = ; >> +linux,gpio-keymap = < BTN_LEFT >> + BTN_TOOL_FINGER >> + BTN_TOOL_DOUBLETAP >> + BTN_TOOL_TRIPLETAP >> + BTN_TOOL_QUADTAP >> + BTN_TOOL_QUINTTAP >; > > I'm afraid you have misunderstood the impact of this change to the way that > the GPIOs coming in to the touch controller are mapped to key codes. Look Unfortunately there are no boards in mainline using this "linux,gpio-keymap" property so I tried to figure out what the expected values were by reading the driver. So is more than possible that I got them wrong. By passing all these keycodes the touchpad worked as expected for me and the driver did the same than the Chrome OS driver that has these keycodes hardcoded when is_tp is true. > at the protocol guide for T19. > I don't have access to proper documentation and I wouldn't expect people to have access to non-public docs in order to use a Device Tree binding. That's why I wanted to document an example, so using this property could be easier for others and they shouldn't have to look at the driver in order to figure it out (and getting it wrong as you said :) ) So it would be great if you could provide an example on how this is supposed to be used. > The DOUBLE/TRIPLE/QUAD/QUINTTAP stuff is filled in for us by the input core > when we use INPUT_MT_POINTER, anyway. > Thanks for the hint, I didn't know that this was the case but I just looked at input_mt_init_slots() [0] and indeed those are not needed. Best regards, Javier [0]: http://lxr.free-electrons.com/source/drivers/input/input-mt.c#L69 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 v3] reset: Add a defer reset object to send board specific reset
Am Dienstag, den 08.07.2014, 11:38 +0200 schrieb Linus Walleij: > On Tue, Jul 8, 2014 at 10:05 AM, Maxime Ripard > wrote: > > On Tue, Jul 08, 2014 at 09:52:03AM +0200, Linus Walleij wrote: > >> On Wed, Jun 18, 2014 at 3:37 PM, Houcheng Lin wrote: > >> > >> > The Problem > >> > --- > >> > The reset signal on a hardware board is send either: > >> > - during machine initialization > >> > - during bus master's initialization > >> > >> I just thought about this a bit, since there isn't already a generic GPIO > >> reset driver, just call this drivers/reset/reset-gpio.c and make the > >> ability to deferral just a configuration detail of the GPIO reset driver. > > > > Philipp has been working on one for quite some time. See > > http://www.spinics.net/lists/arm-kernel/msg321927.html > > > > However, it seems to progress slowly, and we don't seem to be able to > > reach a consensus here. Mostly because Maxime and I seem to have a completely different opinion and nobody else argued one way or the other. > > If you ask me, having to set a few extra properties like this just > > advocates for a regular reset driver and DT node for the reset GPIO, > > but I'm pretty sure Philipp will feel otherwise :) > > Hm haha yeah let's fight it out :-) > > This is not my subsystem so I'm getting some popcorn. Sorry about missing this earlier, I hope the popcorn is not stale. I think a reset that needs to be released before a fixed device appears on the bus, should be handled by the bus driver. The reset framework could be made to help with that, but I don't think a separate entity that scans the whole device tree itself is the right solution. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
On 07/08/14 08:44, Javier Martinez Canillas wrote:
> The Atmel maXTouch driver assumed that the IRQ type flags will
> always be passed using platform data but this is not true when
> booting using Device Trees. In these setups the interrupt type
> was ignored by the driver when requesting an IRQ.
>
> This means that it will fail if a machine specified other type
> than IRQ_TYPE_NONE. The right approach is to get the IRQ flags
> that was parsed by OF from the "interrupt" Device Tree propery.
>
> Signed-off-by: Javier Martinez Canillas
I'm happy for this to go in if Dmitry will accept it. It does seem to be a
quirk of some platforms that it is necessary, but it's only one line.
Thanks for spending so much time debugging this.
Signed-off-by: Nick Dyer
> ---
>
> This patch was first sent as a part of a two part series along
> with [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example.
>
> But there are no dependencies between these two patches so there
> is no need to resend that one with no changes for v2.
>
> Changes since v1:
> - Assign flags to pdata->irqflags in mxt_parse_dt() instead of probe().
>Suggested by Tomasz Figa.
>
> drivers/input/touchscreen/atmel_mxt_ts.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
> b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 03b8571..5c8cbd3 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -22,6 +22,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -2093,6 +2094,8 @@ static struct mxt_platform_data *mxt_parse_dt(struct
> i2c_client *client)
> if (!pdata)
> return ERR_PTR(-ENOMEM);
>
> + pdata->irqflags = irq_get_trigger_type(client->irq);
> +
> if (of_find_property(client->dev.of_node, "linux,gpio-keymap",
>&proplen)) {
> pdata->t19_num_keys = proplen / sizeof(u32);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 4/5] ARM: dts: add pinctrl support to Exynos5410
Hi,
Please see my comment inline.
On 28.07.2014 14:18, Andreas Färber wrote:
> From: Hakjoo Kim
>
> Add the required pin configuration support to Exynos5410 using pinctrl
> interface.
[snip]
> + pinctrl_0: pinctrl@1340 {
> + compatible = "samsung,exynos5410-pinctrl";
> + reg = <0x1340 0x1000>;
> + interrupts = <0 45 0>;
I believe you are missing the wake-up IRQ controller node here, which is
needed to handle the interrupts of EINTW (gpxN) banks.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 3/5] pinctrl: exynos: add exynos5410 SoC specific data
Linus, Andreas, On 08.08.2014 14:55, Linus Walleij wrote: > On Mon, Jul 28, 2014 at 2:18 PM, Andreas Färber wrote: > >> From: Hakjoo Kim >> >> Add Samsung EXYNOS5410 SoC specific data to enable pinctrl >> support for all platforms based on EXYNOS5410. >> >> Cc: Hakjoo Kim >> [AF: Rebased onto Exynos5260] > > I'm waiting for Tomasz to review this before applying. Thanks for the reminder. I'm currently on holidays and I'm not following the lists too closely, so sorry if I happen to miss some patches. In general the changes look good, although I can't verify whether the data being added really match the hardware, as I don't have access to detailed information about Exynos5410. Also I believe this patch needs to be rebased onto current pinctrl tree as several (conflicting) improvements to the pinctrl-samsung driver hit it before this patch was posted. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
Hello,
On 08/07/2014 06:47 PM, Dmitry Torokhov wrote:
>
> Actually, I take this back. In mainline everything as it should: if
> pdata does not specify particular trigger the flags requested end up
> being IRQF_ONESHOT, which should preserve trigger bits previously set up
> by the board or OF code. In Chrome kernel we have:
>
In theory it should work as Dmitry and Nick said since if no trigger flags are
set then whatever was set before (by OF, platform code, ACPI) should be used.
I also verified what Tomasz mentioned that the IRQ trigger type is parsed and
set correctly by OF:
irq_of_parse_and_map()
irq_create_of_mapping()
irq_set_irq_type()
__irq_set_trigger()
chip->irq_set_type()
exynos_irq_set_type()
But for some reason it doesn't work for me unless I set the trigger type in the
flags passed to request_threaded_irq().
I found that what makes it to work is the logic in __setup_irq() [0] that Nick
pointed out on a previous email:
/* Setup the type (level, edge polarity) if configured: */
if (new->flags & IRQF_TRIGGER_MASK) {
ret = __irq_set_trigger(desc, irq,
new->flags & IRQF_TRIGGER_MASK);
if (ret)
goto out_mask;
}
So __irq_set_trigger() is only executed if the struct irqaction flags has a
trigger flag which makes sense since this shouldn't be necessary with DT due
__irq_set_trigger() being called from irq_create_of_mapping() before when the
"interrupts" property is parsed.
But for some reason it is necessary for me... I checked that struct irq_data
state_use_accessors value is correct and even tried setting that value to
new->flags after the mentioned code block but it makes no difference. Input
events are not reported by evtest and AFAICT interrupts are not being generated.
It works though if the trigger type is passed to request_threaded_irq() like
$subject does or if new->flags are set before the new->flags & IRQF_TRIGGER_MASK
conditional.
For example, with the following changes interrupts are fired correctly:
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 3dc6a61..2d8adbb 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1169,6 +1169,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
struct irqaction *new)
init_waitqueue_head(&desc->wait_for_threads);
+ if (!(new->flags & IRQF_TRIGGER_MASK))
+ new->flags |= irqd_get_trigger_type(desc);
+
/* Setup the type (level, edge polarity) if configured: */
if (new->flags & IRQF_TRIGGER_MASK) {
ret = __irq_set_trigger(desc, irq,
Any ideas what could be wrong here?
> /* Default to falling edge if no platform data provided */
> irqflags = data->pdata ? data->pdata->irqflags : IRQF_TRIGGER_FALLING;
> error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
>irqflags | IRQF_ONESHOT,
>client->name, data);
>
Exactly, that's how I found the issue. When I compared both drivers I noticed
that the Chrome OS driver did that and since all the supported platforms are DT
based, the above is equivalent to just IRQF_TRIGGER_FALLING | IRQF_ONESHOT.
So according to my explanation, new->flags & IRQF_TRIGGER_MASK is true so
__irq_set_trigger() is executed and that's why it works with the Chrome OS
driver.
In fact the Chrome OS DTS does not set a trigger type in the "interrupts"
property:
trackpad@4b {
reg=<0x4b>;
compatible="atmel,atmel_mxt_tp";
interrupts=<1 0>;
interrupt-parent=<&gpx1>;
wakeup-source;
};
> which I believe should go away. If it is needed on ACPI systems we need
> to figure out how to do things we can do with OF there.
>
The above code should not be related to ACPI systems since whatever code that
parses an ACPI table should just call irq_set_irq_type() like is made by OF, so
request_threaded_irq() should just work with ACPI too.
I agree it should go away but first I want to understand why is needed in the
first place. Unfortunately commit:
031f136 ("Input: atmel_mxt_ts - Set default irqflags when there is no pdata")
from the Chrome OS 3.8 does not explain why this is needed, instead of adding
this information in the DTS (e.g: interrupts=<1 IRQ_TYPE_EDGE_FALLING>).
> Thanks.
>
Best regards,
Javier
[0]: http://lxr.free-electrons.com/source/kernel/irq/manage.c#L1172
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 3/5] pinctrl: exynos: add exynos5410 SoC specific data
On Mon, Jul 28, 2014 at 2:18 PM, Andreas Färber wrote: > From: Hakjoo Kim > > Add Samsung EXYNOS5410 SoC specific data to enable pinctrl > support for all platforms based on EXYNOS5410. > > Cc: Hakjoo Kim > [AF: Rebased onto Exynos5260] I'm waiting for Tomasz to review this before applying. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mfd: sec-irq: fix support for devices without irq specified
On Friday, August 08, 2014 09:48:56 AM Krzysztof Kozlowski wrote:
> On pią, 2014-08-08 at 09:34 +0200, Krzysztof Kozlowski wrote:
> > On czw, 2014-08-07 at 18:48 +0200, Bartlomiej Zolnierkiewicz wrote:
> > > [ added missing linux-samsung-soc ML, sorry for the noise ]
> > >
> > > On Thursday, August 07, 2014 06:42:28 PM Bartlomiej Zolnierkiewicz wrote:
> > > > Add missing check for the case of device without irq specified
> > > > in sec_irq_exit() (please note that sec_irq_init() already
> > > > correctly handles such devices).
> > > >
> > > > This is needed for Insignal's Exynos4412 based Origen board.
> > > >
> > > > Cc: Krzysztof Kozlowski
> > > > Cc: Sangbeom Kim
> > > > Signed-off-by: Bartlomiej Zolnierkiewicz
> > > > Acked-by: Kyungmin Park
> > > > ---
> > > > patch is against next-20140804 branch of linux-next kernel
> > > >
> > > > drivers/mfd/sec-irq.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > Looks and works good (tested on board with S2MPS14).
> >
> > Reviewed-by: Krzysztof Kozlowski
> > Tested-by: Krzysztof Kozlowski
> >
> > Best regards,
> > Krzysztof
> >
> > > >
> > > > diff --git a/drivers/mfd/sec-irq.c b/drivers/mfd/sec-irq.c
> > > > index f9a5786..b65a7f0 100644
> > > > --- a/drivers/mfd/sec-irq.c
> > > > +++ b/drivers/mfd/sec-irq.c
> > > > @@ -478,5 +478,6 @@ int sec_irq_init(struct sec_pmic_dev *sec_pmic)
> > > >
> > > > void sec_irq_exit(struct sec_pmic_dev *sec_pmic)
> > > > {
> > > > - regmap_del_irq_chip(sec_pmic->irq, sec_pmic->irq_data);
> > > > + if (sec_pmic->irq)
> > > > + regmap_del_irq_chip(sec_pmic->irq, sec_pmic->irq_data);
> > > > }
>
> Seems I jumped too far with this one. Patch looks OK and works fine but
> is it really needed? If (!sec_pmic->irq) then sec_pmic->irq_data will be
> NULL and regmap_del_irq_chip() will handle it correctly.
>
> Your change adds some sense of precautions (the sec_pmic->irq_data may
> be set by some other module by mistake) but still it does not look like
> "needed" for Origen.
Indeed, I somehow assumed that regmap_del_irq_chip() doesn't check for
NULL 'struct regmap_irq_chip_data *d' argument. Since such check is
actually present in regmap_del_irq_chip() the patch is not needed and
can be dropped. Thanks for noticing this.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mfd: max77686: fix support for devices without irq specified
On 08/08/2014 02:24 PM, Bartlomiej Zolnierkiewicz wrote: > > It seems that Daniel Drake has already fixed ODROID dtsi with: > > [PATCH 1/2] ARM: dts: Enable PMIC interrupts on ODROID > http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg34191.html > > and > > [PATCH 2/2] ARM: dts: ODROID i2c improvements > http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg34190.html > > Both patches are needed to fix boot on ODROID. They also fix RTC driver > registration: > > MFD driver error message with only patch #1 applied: > [0.164488] max77686 0-0009: failed to add RTC irq chip: -6 > > RTC driver messages with both patches #1 and #2 applied: > [1.731461] max77686-rtc max77686-rtc: max77686_rtc_probe > [1.833303] max77686-rtc max77686-rtc: rtc core: registered max77686-rtc > as rtc0 > Great! > Kukjin, could you please apply Daniel's fixes? They are critical for ODROID > boards. > > Javier, I think that it still would be useful to apply my patch for max77686 > as it makes new kernels work with older dtbs and leaves the possibility to > use PMIC on boards without IRQ connected. If you agree I will post v2 of > the patch (with suspend/resume handlers fixed and bindings documentation > updated). > Well it really depends on whether it is possible or not to have a design where a IRQ line is not connected to the PMIC. If such a design makes sense then your patch should be applied since the driver should not fail to probe in that case. But then the DT binding doc should be changed and the "interrupt" property moved from required to optional in order to reflect that it's not a requirement. If the motivation is just to support old DTB then I tend to disagree since those DTBs were not following the documented binding. Drivers should be robust enough to not crash of course but if a required property is not defined in a DTB but failing to probe is the right thing to do IMHO. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mfd: max77686: fix support for devices without irq specified
Hi Javier,
On Friday, August 08, 2014 12:58:50 PM Javier Martinez Canillas wrote:
> Hello Krzysztof,
>
> On 08/08/2014 12:54 PM, Krzysztof Kozlowski wrote:
> >> >>
> >> >> Not sufficient. You have to also fix RTC driver (OOPS from Trats2
> >> >> attached). Also consider adding checks for (max77686->irq) to the
> >> >> suspend and resume.
> >> >>
> >> >
> >> > Right, the max77686 RTC driver assumes that an IRQ domain will be
> >> > created on the
> >> > mfd driver so a virtual IRQ can be mapped for the RTC alarm1 IRQ. This
> >> > assumptions comes from the fact that the "interrupt" property is required
> >> > according to the DT binding doc.
> >> >
> >>
> >> Although for Trats2 I see that arch/arm/boot/dts/exynos4412-trats2.dts
> >> defines
> >> an interrupt, so I wonder why regmap_irq_get_virq() is giving an oops
> >> there:
> >>
> >> max77686_pmic@09 {
> >>compatible = "maxim,max77686";
> >>interrupt-parent = <&gpx0>;
> >>interrupts = <7 0>;
> >>reg = <0x09>;
> >>#clock-cells = <1>;
> >> ...
> >
> > Because I am a nasty user :) and I removed the interrupts properties
> > manually (to test how the RTC will behave). Still the driver shouldn't
> > oops.
> >
>
> Oh, now it makes sense :)
>
> Yes, I agree with you that the driver should not oops. The probe should fail
> though if no IRQ domain was created on the MFD driver since that DT property
> is
> required.
>
> I'll send a fix for this today, thanks a lot for reporting it.
>
> >>
> >>
> >> > So the max77686 RTC wakealarm was not working for these boards before?
> >
> > I don't know for Odroid but on Trats2 it works fine.
On ODROID U3 with my max77686 fix applied RTC driver fails with:
[1.743607] max77686-rtc max77686-rtc: max77686_rtc_probe
[1.748312] max77686-rtc max77686-rtc: max77686_rtc_init_reg: fail to write
controlm reg(-6)
[1.756509] max77686-rtc max77686-rtc: Failed to initialize RTC reg:-6
However it turns out that RTC can be fixed with patches from Daniel Drake
(please see below).
> >> >
> >> > Just to be sure that I understand the issue: these boards don't really
> >> > have an
> >> > IRQ connected to the PMIC, is not that this information is just missing
> >> > in the
> >> > Device Tree, right?
> >> >
> >>
> >> By looking at Odroid's 3.8 based vendor tree I see that an IRQ for the
> >> max77686
> >> PMIC is defined [0] using platform data:
> >>
> >> static struct max77686_platform_data exynos4_max77686_info = {
> >>.irq_gpio = EXYNOS4_GPX3(2),
> >>.ono= EXYNOS4_GPX1(2),
> >>.num_regulators = ARRAY_SIZE(max77686_regulators),
> >>.regulators = max77686_regulators,
> >> ...
> >>
> >> So maybe this information is missing in
> >> arch/arm/boot/dts/exynos4412-odroid-common.dtsi?
> >
> > Yes, it seems it is missing.
> >
>
> Indeed.
It seems that Daniel Drake has already fixed ODROID dtsi with:
[PATCH 1/2] ARM: dts: Enable PMIC interrupts on ODROID
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg34191.html
and
[PATCH 2/2] ARM: dts: ODROID i2c improvements
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg34190.html
Both patches are needed to fix boot on ODROID. They also fix RTC driver
registration:
MFD driver error message with only patch #1 applied:
[0.164488] max77686 0-0009: failed to add RTC irq chip: -6
RTC driver messages with both patches #1 and #2 applied:
[1.731461] max77686-rtc max77686-rtc: max77686_rtc_probe
[1.833303] max77686-rtc max77686-rtc: rtc core: registered max77686-rtc as
rtc0
Kukjin, could you please apply Daniel's fixes? They are critical for ODROID
boards.
Javier, I think that it still would be useful to apply my patch for max77686
as it makes new kernels work with older dtbs and leaves the possibility to
use PMIC on boards without IRQ connected. If you agree I will post v2 of
the patch (with suspend/resume handlers fixed and bindings documentation
updated).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mfd: max77686: fix support for devices without irq specified
Hello Krzysztof,
On 08/08/2014 12:54 PM, Krzysztof Kozlowski wrote:
>> >>
>> >> Not sufficient. You have to also fix RTC driver (OOPS from Trats2
>> >> attached). Also consider adding checks for (max77686->irq) to the
>> >> suspend and resume.
>> >>
>> >
>> > Right, the max77686 RTC driver assumes that an IRQ domain will be created
>> > on the
>> > mfd driver so a virtual IRQ can be mapped for the RTC alarm1 IRQ. This
>> > assumptions comes from the fact that the "interrupt" property is required
>> > according to the DT binding doc.
>> >
>>
>> Although for Trats2 I see that arch/arm/boot/dts/exynos4412-trats2.dts
>> defines
>> an interrupt, so I wonder why regmap_irq_get_virq() is giving an oops there:
>>
>> max77686_pmic@09 {
>> compatible = "maxim,max77686";
>> interrupt-parent = <&gpx0>;
>> interrupts = <7 0>;
>> reg = <0x09>;
>> #clock-cells = <1>;
>> ...
>
> Because I am a nasty user :) and I removed the interrupts properties
> manually (to test how the RTC will behave). Still the driver shouldn't
> oops.
>
Oh, now it makes sense :)
Yes, I agree with you that the driver should not oops. The probe should fail
though if no IRQ domain was created on the MFD driver since that DT property is
required.
I'll send a fix for this today, thanks a lot for reporting it.
>>
>>
>> > So the max77686 RTC wakealarm was not working for these boards before?
>
> I don't know for Odroid but on Trats2 it works fine.
>
>> >
>> > Just to be sure that I understand the issue: these boards don't really
>> > have an
>> > IRQ connected to the PMIC, is not that this information is just missing in
>> > the
>> > Device Tree, right?
>> >
>>
>> By looking at Odroid's 3.8 based vendor tree I see that an IRQ for the
>> max77686
>> PMIC is defined [0] using platform data:
>>
>> static struct max77686_platform_data exynos4_max77686_info = {
>> .irq_gpio = EXYNOS4_GPX3(2),
>> .ono= EXYNOS4_GPX1(2),
>> .num_regulators = ARRAY_SIZE(max77686_regulators),
>> .regulators = max77686_regulators,
>> ...
>>
>> So maybe this information is missing in
>> arch/arm/boot/dts/exynos4412-odroid-common.dtsi?
>
> Yes, it seems it is missing.
>
Indeed.
> Best regards,
> Krzysztof
>
Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mfd: max77686: fix support for devices without irq specified
On pią, 2014-08-08 at 12:09 +0200, Javier Martinez Canillas wrote:
> Hello,
>
> On 08/08/2014 10:58 AM, Javier Martinez Canillas wrote:
> >
> >>
> >> Not sufficient. You have to also fix RTC driver (OOPS from Trats2
> >> attached). Also consider adding checks for (max77686->irq) to the
> >> suspend and resume.
> >>
> >
> > Right, the max77686 RTC driver assumes that an IRQ domain will be created
> > on the
> > mfd driver so a virtual IRQ can be mapped for the RTC alarm1 IRQ. This
> > assumptions comes from the fact that the "interrupt" property is required
> > according to the DT binding doc.
> >
>
> Although for Trats2 I see that arch/arm/boot/dts/exynos4412-trats2.dts defines
> an interrupt, so I wonder why regmap_irq_get_virq() is giving an oops there:
>
> max77686_pmic@09 {
> compatible = "maxim,max77686";
> interrupt-parent = <&gpx0>;
> interrupts = <7 0>;
> reg = <0x09>;
> #clock-cells = <1>;
> ...
Because I am a nasty user :) and I removed the interrupts properties
manually (to test how the RTC will behave). Still the driver shouldn't
oops.
>
>
> > So the max77686 RTC wakealarm was not working for these boards before?
I don't know for Odroid but on Trats2 it works fine.
> >
> > Just to be sure that I understand the issue: these boards don't really have
> > an
> > IRQ connected to the PMIC, is not that this information is just missing in
> > the
> > Device Tree, right?
> >
>
> By looking at Odroid's 3.8 based vendor tree I see that an IRQ for the
> max77686
> PMIC is defined [0] using platform data:
>
> static struct max77686_platform_data exynos4_max77686_info = {
> .irq_gpio = EXYNOS4_GPX3(2),
> .ono= EXYNOS4_GPX1(2),
> .num_regulators = ARRAY_SIZE(max77686_regulators),
> .regulators = max77686_regulators,
> ...
>
> So maybe this information is missing in
> arch/arm/boot/dts/exynos4412-odroid-common.dtsi?
Yes, it seems it is missing.
Best regards,
Krzysztof
>
> Best regards,
> Javier
>
> [0]:
> https://github.com/hardkernel/linux/blob/odroid-3.8.y/arch/arm/mach-exynos/pmic-77686.h#L927
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mfd: max77686: fix support for devices without irq specified
Hello,
On 08/08/2014 10:58 AM, Javier Martinez Canillas wrote:
>
>>
>> Not sufficient. You have to also fix RTC driver (OOPS from Trats2
>> attached). Also consider adding checks for (max77686->irq) to the
>> suspend and resume.
>>
>
> Right, the max77686 RTC driver assumes that an IRQ domain will be created on
> the
> mfd driver so a virtual IRQ can be mapped for the RTC alarm1 IRQ. This
> assumptions comes from the fact that the "interrupt" property is required
> according to the DT binding doc.
>
Although for Trats2 I see that arch/arm/boot/dts/exynos4412-trats2.dts defines
an interrupt, so I wonder why regmap_irq_get_virq() is giving an oops there:
max77686_pmic@09 {
compatible = "maxim,max77686";
interrupt-parent = <&gpx0>;
interrupts = <7 0>;
reg = <0x09>;
#clock-cells = <1>;
...
> So the max77686 RTC wakealarm was not working for these boards before?
>
> Just to be sure that I understand the issue: these boards don't really have an
> IRQ connected to the PMIC, is not that this information is just missing in the
> Device Tree, right?
>
By looking at Odroid's 3.8 based vendor tree I see that an IRQ for the max77686
PMIC is defined [0] using platform data:
static struct max77686_platform_data exynos4_max77686_info = {
.irq_gpio = EXYNOS4_GPX3(2),
.ono= EXYNOS4_GPX1(2),
.num_regulators = ARRAY_SIZE(max77686_regulators),
.regulators = max77686_regulators,
...
So maybe this information is missing in
arch/arm/boot/dts/exynos4412-odroid-common.dtsi?
Best regards,
Javier
[0]:
https://github.com/hardkernel/linux/blob/odroid-3.8.y/arch/arm/mach-exynos/pmic-77686.h#L927
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
On Fri, Aug 08, 2014 at 04:37:07PM +0900, Inki Dae wrote: > On 2014년 08월 08일 16:03, Thierry Reding wrote: > > On Fri, Aug 08, 2014 at 10:45:47AM +0900, Inki Dae wrote: > >> On 2014년 08월 07일 22:55, Thierry Reding wrote: > >>> On Thu, Aug 07, 2014 at 10:39:29PM +0900, Inki Dae wrote: > On 2014년 08월 07일 22:17, Thierry Reding wrote: > > On Thu, Aug 07, 2014 at 10:05:44PM +0900, Inki Dae wrote: > >> On 2014년 08월 07일 20:09, Thierry Reding wrote: > >>> On Thu, Aug 07, 2014 at 07:49:03PM +0900, Inki Dae wrote: > On 2014년 08월 07일 18:09, Thierry Reding wrote: > > On Thu, Aug 07, 2014 at 04:51:18PM +0900, Inki Dae wrote: > >> On 2014년 08월 07일 15:58, Thierry Reding wrote: > >>> On Thu, Aug 07, 2014 at 02:09:19AM +0900, Inki Dae wrote: > 2014-08-06 16:43 GMT+09:00 Thierry Reding > : > > [...] > > As far as I can tell non-continuous mode simply means that the > > host can > > turn off the HS clock after a high-speed transmission. I think > > Andrzej > > mentioned this already in another subthread, but this is an > > optional > > mode that peripherals can support if they have extra circuitry > > that > > provides an internal clock. Peripherals that don't have such > > circuitry > > may rely on the HS clock to perform in between transmissions and > > therefore require the HS clock to be always on (continuous > > mode). That's > > what the MIPI_DSI_CLOCK_NON_CONTINUOUS flag is: it advertises > > that the > > peripheral supports non-continuous mode and therefore the host > > can turn > > the HS clock off after high-speed transmissions. > > What I don't make sure is this sentence. With > MIPI_DSI_CLOCK_NON_CONTIUOUS flag, I guess two possible > operations. > One is, > 1. host controller will generates signals if a bit of a register > related to non-contiguous clock mode is set or unset. > 2. And then video data is transmitted to panel in HS mode. > 3. And then D-PHY detects LP-11 signal (positive and negative > lane all > are high). > 4. And then D-PHY disables HS clock of host controller. > 5. At this time, operation mode of host controller becomes LPM. > > Other is, > 1. host controller will generates signals if a bit of a register > related to non-contiguous clock mode is set or unset. > 2. And then D-PHY detects LP-11 signal (positive and negative > lane all > are high). > 3. And then video data is transmitted to panel in LPM. > 4. At this time, operation mode of host controller becomes LPM. > > It seems that you says latter case. > >>> > >>> No. High speed clock and low power mode are orthogonal. > >>> Non-continuous > >>> mode simply means that the clock lane enters LP-11 between HS > >>> transmissions (see 5.6 "Clock Management" of the DSI > >>> specification). > >>> > >> > >> It seems that clock lane enters LP-11 regardless of HS clock > >> enabled if > >> non-continous mode is used. Right? > > > > No, I think as long as HS clock is enabled the clock lane won't > > enter > > LP-11. Non-continuous mode means that the controller can disable > > the HS > > clock between HS transmissions. So in non-continuous mode the clock > > lane > > can enter LP-11 because the controller disables the HS clock. > > It makes me a little bit confusing. You said "if HS clock is enabled, > the the clock lane won't enter LP-11" But you said again "the clock > lane > can enter LP-11 because the controller disables the HS clock" What is > the meaning? > >>> > >>> It means that if the HS clock is enabled, then the clock lane is not > >>> in > >>> LP-11. When the HS clock stops, then the clock lane enters LP-11. > >>> > > In continuous mode, then the clock will never be disabled, hence the > > clock lane will never enter LP-11. > > > >> And also it seems that non-continous mode is different from LPM at > >> all > >> because with non-continuous mode, command data is transmitted to > >> panel > >> in HS mode, but with LPM, command data is transmitted to panel in > >> LP > >> mode. Also right? > > > > No. I think you can send command data to the peripheral in low power > > mode in both continuous and
Re: [PATCH v2 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
On 08/08/2014 11:02 AM, Andrzej Hajda wrote: > On 08/08/2014 09:37 AM, Inki Dae wrote: >> On 2014년 08월 08일 16:03, Thierry Reding wrote: >>> On Fri, Aug 08, 2014 at 10:45:47AM +0900, Inki Dae wrote: On 2014년 08월 07일 22:55, Thierry Reding wrote: > On Thu, Aug 07, 2014 at 10:39:29PM +0900, Inki Dae wrote: >> On 2014년 08월 07일 22:17, Thierry Reding wrote: >>> On Thu, Aug 07, 2014 at 10:05:44PM +0900, Inki Dae wrote: On 2014년 08월 07일 20:09, Thierry Reding wrote: > On Thu, Aug 07, 2014 at 07:49:03PM +0900, Inki Dae wrote: >> On 2014년 08월 07일 18:09, Thierry Reding wrote: >>> On Thu, Aug 07, 2014 at 04:51:18PM +0900, Inki Dae wrote: On 2014년 08월 07일 15:58, Thierry Reding wrote: > On Thu, Aug 07, 2014 at 02:09:19AM +0900, Inki Dae wrote: >> 2014-08-06 16:43 GMT+09:00 Thierry Reding >> : >>> [...] >>> As far as I can tell non-continuous mode simply means that the >>> host can >>> turn off the HS clock after a high-speed transmission. I think >>> Andrzej >>> mentioned this already in another subthread, but this is an >>> optional >>> mode that peripherals can support if they have extra circuitry >>> that >>> provides an internal clock. Peripherals that don't have such >>> circuitry >>> may rely on the HS clock to perform in between transmissions and >>> therefore require the HS clock to be always on (continuous >>> mode). That's >>> what the MIPI_DSI_CLOCK_NON_CONTINUOUS flag is: it advertises >>> that the >>> peripheral supports non-continuous mode and therefore the host >>> can turn >>> the HS clock off after high-speed transmissions. >> What I don't make sure is this sentence. With >> MIPI_DSI_CLOCK_NON_CONTIUOUS flag, I guess two possible >> operations. >> One is, >> 1. host controller will generates signals if a bit of a register >> related to non-contiguous clock mode is set or unset. >> 2. And then video data is transmitted to panel in HS mode. >> 3. And then D-PHY detects LP-11 signal (positive and negative >> lane all >> are high). >> 4. And then D-PHY disables HS clock of host controller. >> 5. At this time, operation mode of host controller becomes LPM. >> >> Other is, >> 1. host controller will generates signals if a bit of a register >> related to non-contiguous clock mode is set or unset. >> 2. And then D-PHY detects LP-11 signal (positive and negative >> lane all >> are high). >> 3. And then video data is transmitted to panel in LPM. >> 4. At this time, operation mode of host controller becomes LPM. >> >> It seems that you says latter case. > No. High speed clock and low power mode are orthogonal. > Non-continuous > mode simply means that the clock lane enters LP-11 between HS > transmissions (see 5.6 "Clock Management" of the DSI > specification). > It seems that clock lane enters LP-11 regardless of HS clock enabled if non-continous mode is used. Right? >>> No, I think as long as HS clock is enabled the clock lane won't >>> enter >>> LP-11. Non-continuous mode means that the controller can disable >>> the HS >>> clock between HS transmissions. So in non-continuous mode the clock >>> lane >>> can enter LP-11 because the controller disables the HS clock. >> It makes me a little bit confusing. You said "if HS clock is enabled, >> the the clock lane won't enter LP-11" But you said again "the clock >> lane >> can enter LP-11 because the controller disables the HS clock" What is >> the meaning? > It means that if the HS clock is enabled, then the clock lane is not > in > LP-11. When the HS clock stops, then the clock lane enters LP-11. > >>> In continuous mode, then the clock will never be disabled, hence the >>> clock lane will never enter LP-11. >>> And also it seems that non-continous mode is different from LPM at all because with non-continuous mode, command data is transmitted to panel in HS mode, but with LPM, command data is transmitted to panel in LP mode. Also right? >>> No. I think you can send command data to the peripheral in low power >>> mode in both continuous and non-continuous clock modes. >>> If
Re: [PATCH v2 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
On 08/08/2014 09:37 AM, Inki Dae wrote: > On 2014년 08월 08일 16:03, Thierry Reding wrote: >> On Fri, Aug 08, 2014 at 10:45:47AM +0900, Inki Dae wrote: >>> On 2014년 08월 07일 22:55, Thierry Reding wrote: On Thu, Aug 07, 2014 at 10:39:29PM +0900, Inki Dae wrote: > On 2014년 08월 07일 22:17, Thierry Reding wrote: >> On Thu, Aug 07, 2014 at 10:05:44PM +0900, Inki Dae wrote: >>> On 2014년 08월 07일 20:09, Thierry Reding wrote: On Thu, Aug 07, 2014 at 07:49:03PM +0900, Inki Dae wrote: > On 2014년 08월 07일 18:09, Thierry Reding wrote: >> On Thu, Aug 07, 2014 at 04:51:18PM +0900, Inki Dae wrote: >>> On 2014년 08월 07일 15:58, Thierry Reding wrote: On Thu, Aug 07, 2014 at 02:09:19AM +0900, Inki Dae wrote: > 2014-08-06 16:43 GMT+09:00 Thierry Reding > : >> [...] >> As far as I can tell non-continuous mode simply means that the >> host can >> turn off the HS clock after a high-speed transmission. I think >> Andrzej >> mentioned this already in another subthread, but this is an >> optional >> mode that peripherals can support if they have extra circuitry >> that >> provides an internal clock. Peripherals that don't have such >> circuitry >> may rely on the HS clock to perform in between transmissions and >> therefore require the HS clock to be always on (continuous >> mode). That's >> what the MIPI_DSI_CLOCK_NON_CONTINUOUS flag is: it advertises >> that the >> peripheral supports non-continuous mode and therefore the host >> can turn >> the HS clock off after high-speed transmissions. > What I don't make sure is this sentence. With > MIPI_DSI_CLOCK_NON_CONTIUOUS flag, I guess two possible > operations. > One is, > 1. host controller will generates signals if a bit of a register > related to non-contiguous clock mode is set or unset. > 2. And then video data is transmitted to panel in HS mode. > 3. And then D-PHY detects LP-11 signal (positive and negative > lane all > are high). > 4. And then D-PHY disables HS clock of host controller. > 5. At this time, operation mode of host controller becomes LPM. > > Other is, > 1. host controller will generates signals if a bit of a register > related to non-contiguous clock mode is set or unset. > 2. And then D-PHY detects LP-11 signal (positive and negative > lane all > are high). > 3. And then video data is transmitted to panel in LPM. > 4. At this time, operation mode of host controller becomes LPM. > > It seems that you says latter case. No. High speed clock and low power mode are orthogonal. Non-continuous mode simply means that the clock lane enters LP-11 between HS transmissions (see 5.6 "Clock Management" of the DSI specification). >>> It seems that clock lane enters LP-11 regardless of HS clock >>> enabled if >>> non-continous mode is used. Right? >> No, I think as long as HS clock is enabled the clock lane won't enter >> LP-11. Non-continuous mode means that the controller can disable the >> HS >> clock between HS transmissions. So in non-continuous mode the clock >> lane >> can enter LP-11 because the controller disables the HS clock. > It makes me a little bit confusing. You said "if HS clock is enabled, > the the clock lane won't enter LP-11" But you said again "the clock > lane > can enter LP-11 because the controller disables the HS clock" What is > the meaning? It means that if the HS clock is enabled, then the clock lane is not in LP-11. When the HS clock stops, then the clock lane enters LP-11. >> In continuous mode, then the clock will never be disabled, hence the >> clock lane will never enter LP-11. >> >>> And also it seems that non-continous mode is different from LPM at >>> all >>> because with non-continuous mode, command data is transmitted to >>> panel >>> in HS mode, but with LPM, command data is transmitted to panel in LP >>> mode. Also right? >> No. I think you can send command data to the peripheral in low power >> mode in both continuous and non-continuous clock modes. >> >>> If so, shouldn't the host driver disable HS clock, in case of LP >>> mode, >>> before the host driver transmits command data? >> No. If the peripheral does
Re: [PATCH] mfd: max77686: fix support for devices without irq specified
Hello Krzysztof and Bartlomiej,
On 08/08/2014 09:59 AM, Krzysztof Kozlowski wrote:
> On czw, 2014-08-07 at 18:09 +0200, Bartlomiej Zolnierkiewicz wrote:
>> Before commit 6f1c1e71d933 ("mfd: max77686: Convert to use regmap_irq")
>> max77686_irq_init() return value was never checked so devices without
>> irq specified (like Hardkernel's Exynos4412 based ODROID-U3 board)
>> worked fine even though -ENODEV was returned by the function. Add
>> handling for no irq specified case in max77686_i2c_probe() restoring
>> the previous driver's behavior.
>>
>> The patch fixes boot for Hardkernel's Exynos4412 based ODROID-U3 board.
>>
>> Error messages before the patch:
>> ...
>> [0.163995] max77686 0-0009: Failed to request IRQ 0 for max77686-pmic:
>> -22
>> [0.164020] max77686 0-0009: failed to add PMIC irq chip: -22
>> [0.164478] max77686: probe of 0-0009 failed with error -22
>> ...
>>
I thought that not specifying an IRQ was not possible since the property says to
be required in Documentation/devicetree/bindings/mfd/max77686.txt:
Required properties:
- compatible : Must be "maxim,max77686";
- reg : Specifies the i2c slave address of PMIC block.
- interrupts : This i2c device has an IRQ line connected to the main SoC.
- interrupt-parent : The parent interrupt controller.
So if this change is necessary then we should also move "interrupt" to the
optional properties section.
>
> Not sufficient. You have to also fix RTC driver (OOPS from Trats2
> attached). Also consider adding checks for (max77686->irq) to the
> suspend and resume.
>
Right, the max77686 RTC driver assumes that an IRQ domain will be created on the
mfd driver so a virtual IRQ can be mapped for the RTC alarm1 IRQ. This
assumptions comes from the fact that the "interrupt" property is required
according to the DT binding doc.
So the max77686 RTC wakealarm was not working for these boards before?
Just to be sure that I understand the issue: these boards don't really have an
IRQ connected to the PMIC, is not that this information is just missing in the
Device Tree, right?
> Best regards,
> Krzysztof
>
>
Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mfd: max77686: fix support for devices without irq specified
On czw, 2014-08-07 at 18:09 +0200, Bartlomiej Zolnierkiewicz wrote:
> Before commit 6f1c1e71d933 ("mfd: max77686: Convert to use regmap_irq")
> max77686_irq_init() return value was never checked so devices without
> irq specified (like Hardkernel's Exynos4412 based ODROID-U3 board)
> worked fine even though -ENODEV was returned by the function. Add
> handling for no irq specified case in max77686_i2c_probe() restoring
> the previous driver's behavior.
>
> The patch fixes boot for Hardkernel's Exynos4412 based ODROID-U3 board.
>
> Error messages before the patch:
> ...
> [0.163995] max77686 0-0009: Failed to request IRQ 0 for max77686-pmic: -22
> [0.164020] max77686 0-0009: failed to add PMIC irq chip: -22
> [0.164478] max77686: probe of 0-0009 failed with error -22
> ...
>
> Fixes: 6f1c1e71d933 ("mfd: max77686: Convert to use regmap_irq")
> Cc: Krzysztof Kozlowski
> Cc: Javier Martinez Canillas
> Cc: Doug Anderson
> Signed-off-by: Bartlomiej Zolnierkiewicz
> Acked-by: Kyungmin Park
> ---
> patch is against next-20140804 branch of linux-next kernel
>
> drivers/mfd/max77686.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
> index 86e5523..5fe024c 100644
> --- a/drivers/mfd/max77686.c
> +++ b/drivers/mfd/max77686.c
> @@ -314,6 +314,11 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
> }
> }
>
> + if (!max77686->irq) {
> + dev_info(max77686->dev, "irq is not specified\n");
> + goto skip_irq_setup;
> + }
> +
> ret = regmap_add_irq_chip(max77686->regmap, max77686->irq,
> IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
> IRQF_SHARED, 0, irq_chip,
> @@ -332,6 +337,7 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
> goto err_del_irqc;
> }
>
> +skip_irq_setup:
> ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL);
> if (ret < 0) {
> dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
Not sufficient. You have to also fix RTC driver (OOPS from Trats2
attached). Also consider adding checks for (max77686->irq) to the
suspend and resume.
Best regards,
Krzysztof
[0.00] Linux version 3.16.0-next-20140808-4-g6ff587eb153e-dirty
(k.kozlowski@AMDC1943) (gcc version 4.7.3 (Ubuntu/Linaro 4.7.3-12ubuntu1) )
#157 SMP PREEMPT Fri Aug 8 09:40:37 CEST 2014
[0.00] CPU: ARMv7 Processor [413fc090] revision 0 (ARMv7), cr=10c5387d
[0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[0.00] Machine model: Samsung Trats 2 based on Exynos4412
[0.00] cma: Reserved 64 MiB at 6b80
[0.00] Memory policy: Data cache writealloc
[0.00] Running under secure firmware.
[0.00] PERCPU: Embedded 7 pages/cpu @eaf84000 s7488 r8192 d12992 u32768
[0.00] Built 1 zonelists in Zone order, mobility grouping on. Total
pages: 260368
[0.00] Kernel command line: root=/dev/mmcblk0p5 rw rootfstype=ext4
rootwait console=ttySAC2,115200n8 fbmem=24M@0x5200 normal lcd=s6e8ax0
pmic_info=3 resume=179:3 csa=/dev/mmcblk0p1 bootloader_log=1068@0x43908010
[0.00] PID hash table entries: 4096 (order: 2, 16384 bytes)
[0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
[0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] Memory: 965980K/1047552K available (4249K kernel code, 273K
rwdata, 1440K rodata, 283K init, 275K bss, 81572K reserved, 269312K highmem)
[0.00] Virtual kernel memory layout:
[0.00] vector : 0x - 0x1000 ( 4 kB)
[0.00] fixmap : 0xffc0 - 0xffe0 (2048 kB)
[0.00] vmalloc : 0xf000 - 0xff00 ( 240 MB)
[0.00] lowmem : 0xc000 - 0xef80 ( 760 MB)
[0.00] pkmap : 0xbfe0 - 0xc000 ( 2 MB)
[0.00] modules : 0xbf00 - 0xbfe0 ( 14 MB)
[0.00] .text : 0xc0008000 - 0xc0596870 (5691 kB)
[0.00] .init : 0xc0597000 - 0xc05ddd40 ( 284 kB)
[0.00] .data : 0xc05de000 - 0xc0622400 ( 273 kB)
[0.00].bss : 0xc0622400 - 0xc06670c0 ( 276 kB)
[0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1
[0.00] Preemptible hierarchical RCU implementation.
[0.00] RCU restricting CPUs from NR_CPUS=8 to nr_cpu_ids=4.
[0.00] RCU: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=4
[0.00] NR_IRQS:16 nr_irqs:16 16
[0.00] GIC physical location is 0x1049
[0.00] L2C: failed to init: -19
[0.00]
Re: [PATCH] mfd: sec-irq: fix support for devices without irq specified
On pią, 2014-08-08 at 09:34 +0200, Krzysztof Kozlowski wrote:
> On czw, 2014-08-07 at 18:48 +0200, Bartlomiej Zolnierkiewicz wrote:
> > [ added missing linux-samsung-soc ML, sorry for the noise ]
> >
> > On Thursday, August 07, 2014 06:42:28 PM Bartlomiej Zolnierkiewicz wrote:
> > > Add missing check for the case of device without irq specified
> > > in sec_irq_exit() (please note that sec_irq_init() already
> > > correctly handles such devices).
> > >
> > > This is needed for Insignal's Exynos4412 based Origen board.
> > >
> > > Cc: Krzysztof Kozlowski
> > > Cc: Sangbeom Kim
> > > Signed-off-by: Bartlomiej Zolnierkiewicz
> > > Acked-by: Kyungmin Park
> > > ---
> > > patch is against next-20140804 branch of linux-next kernel
> > >
> > > drivers/mfd/sec-irq.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> Looks and works good (tested on board with S2MPS14).
>
> Reviewed-by: Krzysztof Kozlowski
> Tested-by: Krzysztof Kozlowski
>
> Best regards,
> Krzysztof
>
> > >
> > > diff --git a/drivers/mfd/sec-irq.c b/drivers/mfd/sec-irq.c
> > > index f9a5786..b65a7f0 100644
> > > --- a/drivers/mfd/sec-irq.c
> > > +++ b/drivers/mfd/sec-irq.c
> > > @@ -478,5 +478,6 @@ int sec_irq_init(struct sec_pmic_dev *sec_pmic)
> > >
> > > void sec_irq_exit(struct sec_pmic_dev *sec_pmic)
> > > {
> > > - regmap_del_irq_chip(sec_pmic->irq, sec_pmic->irq_data);
> > > + if (sec_pmic->irq)
> > > + regmap_del_irq_chip(sec_pmic->irq, sec_pmic->irq_data);
> > > }
Seems I jumped too far with this one. Patch looks OK and works fine but
is it really needed? If (!sec_pmic->irq) then sec_pmic->irq_data will be
NULL and regmap_del_irq_chip() will handle it correctly.
Your change adds some sense of precautions (the sec_pmic->irq_data may
be set by some other module by mistake) but still it does not look like
"needed" for Origen.
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
On 2014년 08월 08일 16:03, Thierry Reding wrote: > On Fri, Aug 08, 2014 at 10:45:47AM +0900, Inki Dae wrote: >> On 2014년 08월 07일 22:55, Thierry Reding wrote: >>> On Thu, Aug 07, 2014 at 10:39:29PM +0900, Inki Dae wrote: On 2014년 08월 07일 22:17, Thierry Reding wrote: > On Thu, Aug 07, 2014 at 10:05:44PM +0900, Inki Dae wrote: >> On 2014년 08월 07일 20:09, Thierry Reding wrote: >>> On Thu, Aug 07, 2014 at 07:49:03PM +0900, Inki Dae wrote: On 2014년 08월 07일 18:09, Thierry Reding wrote: > On Thu, Aug 07, 2014 at 04:51:18PM +0900, Inki Dae wrote: >> On 2014년 08월 07일 15:58, Thierry Reding wrote: >>> On Thu, Aug 07, 2014 at 02:09:19AM +0900, Inki Dae wrote: 2014-08-06 16:43 GMT+09:00 Thierry Reding : > [...] > As far as I can tell non-continuous mode simply means that the > host can > turn off the HS clock after a high-speed transmission. I think > Andrzej > mentioned this already in another subthread, but this is an > optional > mode that peripherals can support if they have extra circuitry > that > provides an internal clock. Peripherals that don't have such > circuitry > may rely on the HS clock to perform in between transmissions and > therefore require the HS clock to be always on (continuous mode). > That's > what the MIPI_DSI_CLOCK_NON_CONTINUOUS flag is: it advertises > that the > peripheral supports non-continuous mode and therefore the host > can turn > the HS clock off after high-speed transmissions. What I don't make sure is this sentence. With MIPI_DSI_CLOCK_NON_CONTIUOUS flag, I guess two possible operations. One is, 1. host controller will generates signals if a bit of a register related to non-contiguous clock mode is set or unset. 2. And then video data is transmitted to panel in HS mode. 3. And then D-PHY detects LP-11 signal (positive and negative lane all are high). 4. And then D-PHY disables HS clock of host controller. 5. At this time, operation mode of host controller becomes LPM. Other is, 1. host controller will generates signals if a bit of a register related to non-contiguous clock mode is set or unset. 2. And then D-PHY detects LP-11 signal (positive and negative lane all are high). 3. And then video data is transmitted to panel in LPM. 4. At this time, operation mode of host controller becomes LPM. It seems that you says latter case. >>> >>> No. High speed clock and low power mode are orthogonal. >>> Non-continuous >>> mode simply means that the clock lane enters LP-11 between HS >>> transmissions (see 5.6 "Clock Management" of the DSI specification). >>> >> >> It seems that clock lane enters LP-11 regardless of HS clock enabled >> if >> non-continous mode is used. Right? > > No, I think as long as HS clock is enabled the clock lane won't enter > LP-11. Non-continuous mode means that the controller can disable the > HS > clock between HS transmissions. So in non-continuous mode the clock > lane > can enter LP-11 because the controller disables the HS clock. It makes me a little bit confusing. You said "if HS clock is enabled, the the clock lane won't enter LP-11" But you said again "the clock lane can enter LP-11 because the controller disables the HS clock" What is the meaning? >>> >>> It means that if the HS clock is enabled, then the clock lane is not in >>> LP-11. When the HS clock stops, then the clock lane enters LP-11. >>> > In continuous mode, then the clock will never be disabled, hence the > clock lane will never enter LP-11. > >> And also it seems that non-continous mode is different from LPM at >> all >> because with non-continuous mode, command data is transmitted to >> panel >> in HS mode, but with LPM, command data is transmitted to panel in LP >> mode. Also right? > > No. I think you can send command data to the peripheral in low power > mode in both continuous and non-continuous clock modes. > >> If so, shouldn't the host driver disable HS clock, in case of LP >> mode, >> before the host driver transmits command data? > > No. If the peripheral doesn't support non-continuous mode, then the HS > clock must never be turne
Re: [PATCH] mfd: sec-irq: fix support for devices without irq specified
On czw, 2014-08-07 at 18:48 +0200, Bartlomiej Zolnierkiewicz wrote:
> [ added missing linux-samsung-soc ML, sorry for the noise ]
>
> On Thursday, August 07, 2014 06:42:28 PM Bartlomiej Zolnierkiewicz wrote:
> > Add missing check for the case of device without irq specified
> > in sec_irq_exit() (please note that sec_irq_init() already
> > correctly handles such devices).
> >
> > This is needed for Insignal's Exynos4412 based Origen board.
> >
> > Cc: Krzysztof Kozlowski
> > Cc: Sangbeom Kim
> > Signed-off-by: Bartlomiej Zolnierkiewicz
> > Acked-by: Kyungmin Park
> > ---
> > patch is against next-20140804 branch of linux-next kernel
> >
> > drivers/mfd/sec-irq.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
Looks and works good (tested on board with S2MPS14).
Reviewed-by: Krzysztof Kozlowski
Tested-by: Krzysztof Kozlowski
Best regards,
Krzysztof
> >
> > diff --git a/drivers/mfd/sec-irq.c b/drivers/mfd/sec-irq.c
> > index f9a5786..b65a7f0 100644
> > --- a/drivers/mfd/sec-irq.c
> > +++ b/drivers/mfd/sec-irq.c
> > @@ -478,5 +478,6 @@ int sec_irq_init(struct sec_pmic_dev *sec_pmic)
> >
> > void sec_irq_exit(struct sec_pmic_dev *sec_pmic)
> > {
> > - regmap_del_irq_chip(sec_pmic->irq, sec_pmic->irq_data);
> > + if (sec_pmic->irq)
> > + regmap_del_irq_chip(sec_pmic->irq, sec_pmic->irq_data);
> > }
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rtc: s5m: re-add support for devices without irq specified
On czw, 2014-08-07 at 18:48 +0200, Bartlomiej Zolnierkiewicz wrote: > [ added missing linux-samsung-soc ML, sorry for the noise ] > > On Thursday, August 07, 2014 06:41:02 PM Bartlomiej Zolnierkiewicz wrote: > > The rtc-s5m driver used to support devices without irq specified > > in the past. Re-add this support. > > > > The patch fixes boot for Insignal's Exynos4412 based Origen board. Looks and works good (tested on board with S2MPS14 with and without interrupt). Reviewed-by: Krzysztof Kozlowski Tested-by: Krzysztof Kozlowski Best regards, Krzysztof > > > > Error messages before the patch: > > ... > > [2.095991] Unable to handle kernel NULL pointer dereference at virtual > > address 0094 > > [2.103299] pgd = c0004000 > > [2.105944] [0094] *pgd= > > [2.109510] Internal error: Oops: 5 [#1] PREEMPT SMP ARM > > [2.114796] Modules linked in: > > [2.117840] CPU: 1 PID: 1 Comm: swapper/0 Not tainted > > 3.16.0-next-20140804-8-ga59480f-dirty #701 > > [2.126950] task: ea80f000 ti: ea882000 task.ti: ea882000 > > [2.132348] PC is at regmap_irq_get_virq+0x0/0x28 > > [2.137033] LR is at s5m_rtc_probe+0xdc/0x310 > > [2.141360] pc : []lr : []psr: 8153 > > [2.141360] sp : ea883e48 ip : fp : > > [2.152815] r10: 000c r9 : c05de7ac r8 : eaabc600 > > [2.158024] r7 : eaa6b4d0 r6 : c0439e8c r5 : eaabc610 r4 : eab30e50 > > [2.164533] r3 : r2 : r1 : 000c r0 : > > [2.171045] Flags: Nzcv IRQs on FIQs off Mode SVC_32 ISA ARM > > Segment kernel > > [2.178423] Control: 10c5387d Table: 4000404a DAC: 0015 > > [2.184150] Process swapper/0 (pid: 1, stack limit = 0xea882240) > > [2.190139] Stack: (0xea883e48 to 0xea884000) > > [2.194483] 3e40: eab32910 0001 c05de7ac eab329b0 > > eaabc610 > > [2.202642] 3e60: c0618c68 eaabc610 c0618c68 c0618c68 > > 008f c02429cc > > [2.210801] 3e80: c02429a0 eaabc610 c06628a8 c02415f0 eaabc610 c0618c68 > > eaabc644 > > [2.218960] 3ea0: eab30d00 c024179c c0618c68 c0241710 c023ffb4 > > ea805478 eaadefc0 > > [2.227119] 3ec0: c0618c68 ea115e80 c0610b10 c0240df8 c0518984 c0618c68 > > c0618c68 > > [2.235278] 3ee0: c05ecc18 c05ecc18 c0241db8 c02426a0 c05b480c > > c00088a4 > > [2.243437] 3f00: 009f c040f134 ea92a300 c0649fe0 ea80f000 6153 > > c05efe54 > > [2.251596] 3f20: c05efe54 6153 eb7ff916 eb7ff908 > > c0573048 c00376dc > > [2.259756] 3f40: c05288f0 c0572758 0006 0006 c05efdf4 c05c2da4 > > 0006 c05c2d84 > > [2.267914] 3f60: c0626900 c059a508 c05de7ac 008f c059ac94 > > 0006 0006 > > [2.276074] 3f80: c059a508 c003c3fc c0401330 > > > > [2.284233] 3fa0: c0401338 c000e538 > > > > [2.292392] 3fc0: > > > > [2.300551] 3fe0: 0013 > > c0c0c0c0 c0c0c0c0 > > [2.308729] [] (regmap_irq_get_virq) from [] > > (s5m_rtc_probe+0xdc/0x310) > > [2.317057] [] (s5m_rtc_probe) from [] > > (platform_drv_probe+0x2c/0x5c) > > [2.325209] [] (platform_drv_probe) from [] > > (driver_probe_device+0x114/0x234) > > [2.334060] [] (driver_probe_device) from [] > > (__driver_attach+0x8c/0x90) > > [2.342479] [] (__driver_attach) from [] > > (bus_for_each_dev+0x54/0x88) > > [2.350637] [] (bus_for_each_dev) from [] > > (bus_add_driver+0xd8/0x1cc) > > [2.358796] [] (bus_add_driver) from [] > > (driver_register+0x78/0xf4) > > [2.366785] [] (driver_register) from [] > > (do_one_initcall+0x80/0x1d0) > > [2.374947] [] (do_one_initcall) from [] > > (kernel_init_freeable+0x108/0x1d4) > > [2.383634] [] (kernel_init_freeable) from [] > > (kernel_init+0x8/0xe4) > > [2.391704] [] (kernel_init) from [] > > (ret_from_fork+0x14/0x3c) > > [2.399246] Code: e8bd8070 e350 1590009c e12fff1e (e5903094) > > [2.405391] ---[ end trace a954d7f019122700 ]--- > > [2.409987] Kernel panic - not syncing: Attempted to kill init! > > exitcode=0x000b > > ... > > > > Cc: Krzysztof Kozlowski > > Signed-off-by: Bartlomiej Zolnierkiewicz > > Acked-by: Kyungmin Park > > --- > > patch is against next-20140804 branch of linux-next kernel > > > > drivers/rtc/rtc-s5m.c | 21 ++--- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c > > index 8f06250a..8754c33 100644 > > --- a/drivers/rtc/rtc-s5m.c > > +++ b/drivers/rtc/rtc-s5m.c > > @@ -717,12 +717,14 @@ static int s5m_rtc_probe(struct platform_device *pdev) > > info->device_type = s5m87xx->device_type; > > info->wtsr_smpl =
Re: [PATCH] mfd: sec-core: add missing sec_irq_init() return value checking
On czw, 2014-08-07 at 18:48 +0200, Bartlomiej Zolnierkiewicz wrote: > [ added missing linux-samsung-soc ML, sorry for the noise ] > > On Thursday, August 07, 2014 06:44:18 PM Bartlomiej Zolnierkiewicz wrote: > > sec_irq_init() can fail if it encounters unknown device type or > > on regmap_add_irq_chip() error. Add missing sec_irq_init() return > > value checking to sec_pmic_probe(). > > > > Tested on Insignal's Exynos4412 based Origen board. > > > > Cc: Krzysztof Kozlowski > > Cc: Sangbeom Kim > > Signed-off-by: Bartlomiej Zolnierkiewicz > > Acked-by: Kyungmin Park > > --- > > patch is against next-20140804 branch of linux-next kernel > > > > drivers/mfd/sec-core.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) Looks and works good (tested on board with S2MPS14). Reviewed-by: Krzysztof Kozlowski Tested-by: Krzysztof Kozlowski Best regards, Krzysztof > > > > diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c > > index dba7e2b..f498867 100644 > > --- a/drivers/mfd/sec-core.c > > +++ b/drivers/mfd/sec-core.c > > @@ -353,7 +353,9 @@ static int sec_pmic_probe(struct i2c_client *i2c, > > if (pdata && pdata->cfg_pmic_irq) > > pdata->cfg_pmic_irq(); > > > > - sec_irq_init(sec_pmic); > > + ret = sec_irq_init(sec_pmic); > > + if (ret) > > + return ret; > > > > pm_runtime_set_active(sec_pmic->dev); -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
On Fri, Aug 08, 2014 at 10:45:47AM +0900, Inki Dae wrote: > On 2014년 08월 07일 22:55, Thierry Reding wrote: > > On Thu, Aug 07, 2014 at 10:39:29PM +0900, Inki Dae wrote: > >> On 2014년 08월 07일 22:17, Thierry Reding wrote: > >>> On Thu, Aug 07, 2014 at 10:05:44PM +0900, Inki Dae wrote: > On 2014년 08월 07일 20:09, Thierry Reding wrote: > > On Thu, Aug 07, 2014 at 07:49:03PM +0900, Inki Dae wrote: > >> On 2014년 08월 07일 18:09, Thierry Reding wrote: > >>> On Thu, Aug 07, 2014 at 04:51:18PM +0900, Inki Dae wrote: > On 2014년 08월 07일 15:58, Thierry Reding wrote: > > On Thu, Aug 07, 2014 at 02:09:19AM +0900, Inki Dae wrote: > >> 2014-08-06 16:43 GMT+09:00 Thierry Reding > >> : > >>> [...] > >>> As far as I can tell non-continuous mode simply means that the > >>> host can > >>> turn off the HS clock after a high-speed transmission. I think > >>> Andrzej > >>> mentioned this already in another subthread, but this is an > >>> optional > >>> mode that peripherals can support if they have extra circuitry > >>> that > >>> provides an internal clock. Peripherals that don't have such > >>> circuitry > >>> may rely on the HS clock to perform in between transmissions and > >>> therefore require the HS clock to be always on (continuous mode). > >>> That's > >>> what the MIPI_DSI_CLOCK_NON_CONTINUOUS flag is: it advertises > >>> that the > >>> peripheral supports non-continuous mode and therefore the host > >>> can turn > >>> the HS clock off after high-speed transmissions. > >> > >> What I don't make sure is this sentence. With > >> MIPI_DSI_CLOCK_NON_CONTIUOUS flag, I guess two possible operations. > >> One is, > >> 1. host controller will generates signals if a bit of a register > >> related to non-contiguous clock mode is set or unset. > >> 2. And then video data is transmitted to panel in HS mode. > >> 3. And then D-PHY detects LP-11 signal (positive and negative lane > >> all > >> are high). > >> 4. And then D-PHY disables HS clock of host controller. > >> 5. At this time, operation mode of host controller becomes LPM. > >> > >> Other is, > >> 1. host controller will generates signals if a bit of a register > >> related to non-contiguous clock mode is set or unset. > >> 2. And then D-PHY detects LP-11 signal (positive and negative lane > >> all > >> are high). > >> 3. And then video data is transmitted to panel in LPM. > >> 4. At this time, operation mode of host controller becomes LPM. > >> > >> It seems that you says latter case. > > > > No. High speed clock and low power mode are orthogonal. > > Non-continuous > > mode simply means that the clock lane enters LP-11 between HS > > transmissions (see 5.6 "Clock Management" of the DSI specification). > > > > It seems that clock lane enters LP-11 regardless of HS clock enabled > if > non-continous mode is used. Right? > >>> > >>> No, I think as long as HS clock is enabled the clock lane won't enter > >>> LP-11. Non-continuous mode means that the controller can disable the > >>> HS > >>> clock between HS transmissions. So in non-continuous mode the clock > >>> lane > >>> can enter LP-11 because the controller disables the HS clock. > >> > >> It makes me a little bit confusing. You said "if HS clock is enabled, > >> the the clock lane won't enter LP-11" But you said again "the clock > >> lane > >> can enter LP-11 because the controller disables the HS clock" What is > >> the meaning? > > > > It means that if the HS clock is enabled, then the clock lane is not in > > LP-11. When the HS clock stops, then the clock lane enters LP-11. > > > >>> In continuous mode, then the clock will never be disabled, hence the > >>> clock lane will never enter LP-11. > >>> > And also it seems that non-continous mode is different from LPM at > all > because with non-continuous mode, command data is transmitted to > panel > in HS mode, but with LPM, command data is transmitted to panel in LP > mode. Also right? > >>> > >>> No. I think you can send command data to the peripheral in low power > >>> mode in both continuous and non-continuous clock modes. > >>> > If so, shouldn't the host driver disable HS clock, in case of LP > mode, > before the host driver transmits command data? > >>> > >>> No. If the peripheral doesn't support non-continuous mode, then the HS > >>> clock must never be turned off. On the other hand, if the peripheral > >>>
