Re: [PATCH v2 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
On Thu, Aug 07, 2014 at 02:09:19AM +0900, Inki Dae wrote: > 2014-08-06 16:43 GMT+09:00 Thierry Reding : > > On Wed, Aug 06, 2014 at 04:11:54PM +0900, Inki Dae wrote: > >> On 2014년 08월 05일 20:12, Thierry Reding wrote: > > [...] > >> > I think that low power mode is more often used for command transmission > >> > (in host-driven mode). I'm not sure how much sense it really makes to > >> > transmit video data in low power mode. It also seems like low power mode > >> > is what all peripherals need to support (if they can do command mode). > >> > Hence I'd like to propose the attached patch that makes all command > >> > messages use low power mode. > >> > >> To use low power mode, I think SoC drivers should add more codes: > >> checking xxx_MSG_LPM, and maybe disabling HS clock. My patch does > >> exactly that, > >> http://www.spinics.net/lists/linux-samsung-soc/msg34866.html > > > > I agree in general that DSI host drivers need to check the flags to make > > a decision about which mode to enable. But your patch also introduces > > additional flags that I don't think are necessary (at least for any of > > the use-cases discussed so far). > > > > As I understand it the MIPI_DSI_MODE_CMD_LPM and MIPI_DSI_MODE_VIDEO_LPM > > flags that you introduce would advertise that the device only supports > > low power mode for command or video modes respectively. However, I doubt > > Not so. My intention is to add LPM support for video and command data > transfers because mipi-dsi framework enforces implicitly using HS mode > for by default. No, it doesn't enforce anything at this point. Everyone is free to use whatever they see fit. Drivers that require low power mode for command transmission can set the MIPI_DSI_MSG_USE_LPM flag in messages. For video there's no way to specify what a given peripheral uses, so DSI host controller drivers are free to do whatever they want. So for command data we already have a means, and for video data I don't think it makes sense to use low power mode. Therefore I don't think these new flags are necessary. > > that there really are devices that only support low power video mode. It > > wouldn't make much sense because you'd get a maximum of 10 MHz for the > > clock, which is about 1.6 frames per second at 1920x1080 resolution (not > > counting blanking). And even with lower resolutions such as 1024x768 it > > would be somewhere around 4 frames per second. And I think it's > > reasonable to assume that we'll see that kind of resolution become very > > rare in the future. > > > > So my point is that devices which support video mode will always support > > high speed mode for video transmission too. Similarly, if a device > > supports command mode, then it will likely support it in low-power mode, > > and optionally in high speed mode too. > > > >> And what I and Andrzej don't make sure is non-continuous clock mode. Do > >> you know how non-continuous clock mode is related to HS clock? > > > > 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). For low power mode the clock is embedded in the signal on the data lane and therefore independent of the high speed clock. A peripheral device will set the MIPI_DSI_CLOCK_NON_CONTINUOUS flag if it sup
Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
On Thu, Aug 07, 2014 at 03:47:24AM +0200, Javier Martinez Canillas wrote: > Hello Tomasz, > > Thanks a lot for your feedback. > > On 08/07/2014 03:14 AM, Tomasz Figa wrote: > > Hi Javier, > > > > > > Have you observed an actual failure due to this? I believe that > > Yes, I found this issue since the driver was not taking into account the value > defined in the edge/level type cells from the "interrupts" DT property. > > Only doing the change in the following patch was not enough: > > [PATCH 1/1] ARM: dts: Add Peach Pit and Pi dts entry for atmel touchpad [0]. > > > irq_of_parse_and_map() already sets up IRQ trigger type based on DT > > data, by calling irq_create_of_mapping() which in turn calls > > irq_set_irq_type(). > > > > Right but somehow when the IRQ is actually requested the type is overwritten > by > the value passed to request_threaded_irq() and interrupts are not being > generated by the device without this patch. > > Do you think that this is a bug in the "interrupt-parent" irqchip driver or > the > IRQ core? I'm not that familiar with the IRQ subsystem. No, this is clearly driver fault - it smashed previously done setup with new flags. > > >> > >> + if (client->dev.of_node) > >> + irqflags = irq_get_trigger_type(client->irq); > > > > It might be a bit cleaner to just assign the flags to pdata->irqflags in > > mxt_parse_dt() instead. That would also account for the fact that pdata, > > if provided, should have priority over DT. > > > > You are totally right, also this will break if CONFIG_OF is not enabled since > dev.of_node will not be defined. While this already is taken into account for > mxt_parse_dt() by defining an empty function. > > I'll change it in v2 if getting the flags from the driver is the right > approach Yes, please. > instead of fixing the irqchip driver or the IRQ core. 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 1/1] ARM: dts: Add Peach Pit and Pi dts entry for atmel touchpad
Fabio,
On Wed, Aug 6, 2014 at 6:35 PM, Fabio Estevam wrote:
> On Wed, Aug 6, 2014 at 10:08 PM, Javier Martinez Canillas
> wrote:
>
>> +&hsi2c_8 {
>> + status = "okay";
>> + clock-frequency = <333000>;
>
> Doesn't it work at the more standard 400kHz i2c frequency?
I'm pretty sure that they had signaling problems at 400kHz, but
perhaps Benson will rememer.
--
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/1] ARM: dts: Add Peach Pit and Pi dts entry for atmel touchpad
Hello Fabio,
On 08/07/2014 03:35 AM, Fabio Estevam wrote:
> On Wed, Aug 6, 2014 at 10:08 PM, Javier Martinez Canillas
> wrote:
>
>> +&hsi2c_8 {
>> + status = "okay";
>> + clock-frequency = <333000>;
>
> Doesn't it work at the more standard 400kHz i2c frequency?
>
Most bits of this DTS snippet have been taken from the downstream Chrome OS 3.8
kernel so I'll let one of the Chromium folks to answer this question.
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
Hello Tomasz, Thanks a lot for your feedback. On 08/07/2014 03:14 AM, Tomasz Figa wrote: > Hi Javier, > > > Have you observed an actual failure due to this? I believe that Yes, I found this issue since the driver was not taking into account the value defined in the edge/level type cells from the "interrupts" DT property. Only doing the change in the following patch was not enough: [PATCH 1/1] ARM: dts: Add Peach Pit and Pi dts entry for atmel touchpad [0]. > irq_of_parse_and_map() already sets up IRQ trigger type based on DT > data, by calling irq_create_of_mapping() which in turn calls > irq_set_irq_type(). > Right but somehow when the IRQ is actually requested the type is overwritten by the value passed to request_threaded_irq() and interrupts are not being generated by the device without this patch. Do you think that this is a bug in the "interrupt-parent" irqchip driver or the IRQ core? I'm not that familiar with the IRQ subsystem. >> >> +if (client->dev.of_node) >> +irqflags = irq_get_trigger_type(client->irq); > > It might be a bit cleaner to just assign the flags to pdata->irqflags in > mxt_parse_dt() instead. That would also account for the fact that pdata, > if provided, should have priority over DT. > You are totally right, also this will break if CONFIG_OF is not enabled since dev.of_node will not be defined. While this already is taken into account for mxt_parse_dt() by defining an empty function. I'll change it in v2 if getting the flags from the driver is the right approach instead of fixing the irqchip driver or the IRQ core. > Best regards, > Tomasz > Best regards, Javier [0]: http://www.spinics.net/lists/kernel/msg1802099.html -- 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/1] ARM: dts: Add Peach Pit and Pi dts entry for atmel touchpad
On Wed, Aug 6, 2014 at 10:08 PM, Javier Martinez Canillas
wrote:
> +&hsi2c_8 {
> + status = "okay";
> + clock-frequency = <333000>;
Doesn't it work at the more standard 400kHz i2c frequency?
--
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
Hi Javier,
On 07.08.2014 02:48, 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.
Have you observed an actual failure due to this? I believe that
irq_of_parse_and_map() already sets up IRQ trigger type based on DT
data, by calling irq_create_of_mapping() which in turn calls
irq_set_irq_type().
>
> Signed-off-by: Javier Martinez Canillas
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
> b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 03b8571..0fb56c9 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
> @@ -2130,6 +2131,7 @@ static int mxt_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> struct mxt_data *data;
> const struct mxt_platform_data *pdata;
> int error;
> + unsigned long irqflags;
>
> pdata = dev_get_platdata(&client->dev);
> if (!pdata) {
> @@ -2156,8 +2158,13 @@ static int mxt_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> init_completion(&data->reset_completion);
> init_completion(&data->crc_completion);
>
> + if (client->dev.of_node)
> + irqflags = irq_get_trigger_type(client->irq);
It might be a bit cleaner to just assign the flags to pdata->irqflags in
mxt_parse_dt() instead. That would also account for the fact that pdata,
if provided, should have priority over DT.
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
[PATCH 1/1] ARM: dts: Add Peach Pit and Pi dts entry for atmel touchpad
From: Sjoerd Simons
The Peach Pit and Pi boards have an Atmel maXTouch device.
Add the needed Device Tree nodes to support it.
Signed-off-by: Sjoerd Simons
[javier.martinez: added linux,gpio-keymap property and changed IRQ type]
Signed-off-by: Javier Martinez Canillas
---
With only this patch the touchpad is probed but interrupts are not being
generated. The following is needed to have a fully functional touchpad:
[PATCH 1/2] "Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting"
https://lkml.org/lkml/2014/8/6/585
arch/arm/boot/dts/exynos5420-peach-pit.dts | 29 +
arch/arm/boot/dts/exynos5800-peach-pi.dts | 29 +
2 files changed, 58 insertions(+)
diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts
b/arch/arm/boot/dts/exynos5420-peach-pit.dts
index 228a6b1..7dce444 100644
--- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
@@ -11,6 +11,7 @@
/dts-v1/;
#include
#include
+#include
#include "exynos5420.dtsi"
/ {
@@ -157,6 +158,27 @@
};
};
+&hsi2c_8 {
+ status = "okay";
+ clock-frequency = <333000>;
+
+ trackpad@4b {
+ compatible="atmel,maxtouch";
+ reg=<0x4b>;
+ interrupt-parent=<&gpx1>;
+ interrupts=<1 IRQ_TYPE_EDGE_FALLING>;
+ wakeup-source;
+ pinctrl-names = "default";
+ pinctrl-0 = <&trackpad_irq>;
+ linux,gpio-keymap = < BTN_LEFT
+ BTN_TOOL_FINGER
+ BTN_TOOL_DOUBLETAP
+ BTN_TOOL_TRIPLETAP
+ BTN_TOOL_QUADTAP
+ BTN_TOOL_QUINTTAP >;
+ };
+};
+
&hsi2c_9 {
status = "okay";
clock-frequency = <40>;
@@ -249,6 +271,13 @@
samsung,pin-drv = <0>;
};
+ trackpad_irq: trackpad-irq {
+ samsung,pins = "gpx1-1";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <0>;
+ };
+
power_key_irq: power-key-irq {
samsung,pins = "gpx1-2";
samsung,pin-function = <0>;
diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts
b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index f3ee48b..de946b3 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -11,6 +11,7 @@
/dts-v1/;
#include
#include
+#include
#include "exynos5800.dtsi"
/ {
@@ -155,6 +156,27 @@
};
};
+&hsi2c_8 {
+ status = "okay";
+ clock-frequency = <333000>;
+
+ trackpad@4b {
+ compatible="atmel,maxtouch";
+ reg=<0x4b>;
+ interrupt-parent=<&gpx1>;
+ interrupts=<1 IRQ_TYPE_EDGE_FALLING>;
+ wakeup-source;
+ pinctrl-names = "default";
+ pinctrl-0 = <&trackpad_irq>;
+ linux,gpio-keymap = < BTN_LEFT
+ BTN_TOOL_FINGER
+ BTN_TOOL_DOUBLETAP
+ BTN_TOOL_TRIPLETAP
+ BTN_TOOL_QUADTAP
+ BTN_TOOL_QUINTTAP >;
+ };
+};
+
&hsi2c_9 {
status = "okay";
clock-frequency = <40>;
@@ -247,6 +269,13 @@
samsung,pin-drv = <0>;
};
+ trackpad_irq: trackpad-irq {
+ samsung,pins = "gpx1-1";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <0>;
+ };
+
power_key_irq: power-key-irq {
samsung,pins = "gpx1-2";
samsung,pin-function = <0>;
--
2.0.0.rc2
--
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 2/2] Input: atmel_mxt_ts - Add keycodes array example
The Atmel maXTouch driver allows to dynamically define the event keycodes set that the device is capable of. This may not be evident when reading the DT binding document so add an example to make it easier to understand how this works. 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. 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 >; }; -- 2.0.0.rc2 -- 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/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
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
---
drivers/input/touchscreen/atmel_mxt_ts.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
b/drivers/input/touchscreen/atmel_mxt_ts.c
index 03b8571..0fb56c9 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
@@ -2130,6 +2131,7 @@ static int mxt_probe(struct i2c_client *client, const
struct i2c_device_id *id)
struct mxt_data *data;
const struct mxt_platform_data *pdata;
int error;
+ unsigned long irqflags;
pdata = dev_get_platdata(&client->dev);
if (!pdata) {
@@ -2156,8 +2158,13 @@ static int mxt_probe(struct i2c_client *client, const
struct i2c_device_id *id)
init_completion(&data->reset_completion);
init_completion(&data->crc_completion);
+ if (client->dev.of_node)
+ irqflags = irq_get_trigger_type(client->irq);
+ else
+ irqflags = pdata->irqflags;
+
error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
-pdata->irqflags | IRQF_ONESHOT,
+irqflags | IRQF_ONESHOT,
client->name, data);
if (error) {
dev_err(&client->dev, "Failed to register interrupt\n");
--
2.0.0.rc2
--
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] cpufreq: tests: Providing cpufreq regression test
On Thursday, July 24, 2014 09:04:02 AM Lukasz Majewski wrote: > Hi Rafael, > > > On Monday, July 21, 2014 09:02:34 AM Lukasz Majewski wrote: > > > This commit adds first regression test "cpufreq_freq_test.sh" for > > > the cpufreq subsystem. > > > > First of all, I'm not seeing any explanation why this script should be > > shipped with the kernel. > > OK. > > > > > What regressions it tests against in particular and > > Do you require SHA's/commit messages of commits which were developed to > fix issues spotted with this test script? No. I want information about what kind of bugs can be catched with the help of this script. > > how it does that. > > Is this information required in the commit message or can it stay in > the README file created in the same commit? There should be *some* information in the changelog too. -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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 v3] Documentation: devicetree: Fix tps65090 typos in example
On Wed, Jul 30, 2014 at 11:29:27PM +0200, Andreas Färber wrote:
> Specification and existing device trees use vsys-l{1,2}-supply,
> not vsys_l{1,2}-supply. Fix the example to match the specification.
Applied. I've previously reminded you to use subject lines appropraite
for the subsystem, please do so - this is important for telling people
what the patch is for.
signature.asc
Description: Digital signature
Re: [PATCH v2 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
2014-08-06 16:43 GMT+09:00 Thierry Reding : > On Wed, Aug 06, 2014 at 04:11:54PM +0900, Inki Dae wrote: >> On 2014년 08월 05일 20:12, Thierry Reding wrote: > [...] >> > I think that low power mode is more often used for command transmission >> > (in host-driven mode). I'm not sure how much sense it really makes to >> > transmit video data in low power mode. It also seems like low power mode >> > is what all peripherals need to support (if they can do command mode). >> > Hence I'd like to propose the attached patch that makes all command >> > messages use low power mode. >> >> To use low power mode, I think SoC drivers should add more codes: >> checking xxx_MSG_LPM, and maybe disabling HS clock. My patch does >> exactly that, >> http://www.spinics.net/lists/linux-samsung-soc/msg34866.html > > I agree in general that DSI host drivers need to check the flags to make > a decision about which mode to enable. But your patch also introduces > additional flags that I don't think are necessary (at least for any of > the use-cases discussed so far). > > As I understand it the MIPI_DSI_MODE_CMD_LPM and MIPI_DSI_MODE_VIDEO_LPM > flags that you introduce would advertise that the device only supports > low power mode for command or video modes respectively. However, I doubt Not so. My intention is to add LPM support for video and command data transfers because mipi-dsi framework enforces implicitly using HS mode for by default. > that there really are devices that only support low power video mode. It > wouldn't make much sense because you'd get a maximum of 10 MHz for the > clock, which is about 1.6 frames per second at 1920x1080 resolution (not > counting blanking). And even with lower resolutions such as 1024x768 it > would be somewhere around 4 frames per second. And I think it's > reasonable to assume that we'll see that kind of resolution become very > rare in the future. > > So my point is that devices which support video mode will always support > high speed mode for video transmission too. Similarly, if a device > supports command mode, then it will likely support it in low-power mode, > and optionally in high speed mode too. > >> And what I and Andrzej don't make sure is non-continuous clock mode. Do >> you know how non-continuous clock mode is related to HS clock? > > 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. I really know that with non-contiguous clock mode, HS clock of host controller can be disabled. My question is who controls HS clock in this case. D-PHY or host controller? In other words, with LPM and MIPI_DSI_CLOCK_NON_CONTIUOUS flags, should the host driver check these two flags to disable HS clock? or In this case, does the D-PHY disable HS clock regardless of host driver? I think we should make sure that to handle LP and HS modes correctly. > > If a device doesn't specify that flag then the host needs to keep the HS > clock running all the time. > >> > The .transfer() function was really designed with initialization >> > commands in mind, so it doesn't deal with mixing video data and commands >> > anyway and for initialization low-power mode should be fast enough. The >> > downside is that it may not be optimal for some peripherals, but it >> > gives us a good solution for the general case since it should support >> > all devices. >> > >> > If we absolutely must have faster initialization, or if we come across a >> > device that can only initialize in high speed mode, then I think we >> > should int
Re: [PATCH 0/2] Convert exynos PPMU driver to be built as module
Punit Agrawal writes: > Hi, > > There's no reason why the exynos PPMU can't be built as a module > except you need - > > - The first patch exports the functions that are needed to build > devfreq drivers as modules. > > - The second patch then converts the exynos PPMU devfreq driver to be > built as a module. > Ping! > Compile tested only. > > Thanks > > Punit Agrawal (1): > PM / devfreq: exynos: Enable building exynos PPMU as module > > Ørjan Eide (1): > PM / devfreq: Export helper functions for drivers > > drivers/devfreq/Kconfig |2 +- > drivers/devfreq/devfreq.c|3 +++ > drivers/devfreq/exynos/exynos_ppmu.c |3 +++ > 3 files changed, 7 insertions(+), 1 deletion(-) -- 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] ARM: EXYNOS: Refactor the pm code to use DT based lookup
Hi Tomasz,
On Wed, Aug 6, 2014 at 6:57 PM, Tomasz Figa wrote:
> On 06.08.2014 14:58, Vikas Sajjan wrote:
>> On Wed, Aug 6, 2014 at 1:35 AM, Tomasz Figa wrote:
>>> On 25.07.2014 13:49, Vikas Sajjan wrote:
>
> [snip]
>
>>
+
+ /* Disable USE_RETENTION of JPEG_MEM_OPTION */
+ tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
+ tmp &= ~EXYNOS5_OPTION_USE_RETENTION;
+ pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);
>>>
>>> This looks completely like a copy paste from a vendor tree needed to
>>> workaround some issues in early revisions of the SoC. Are you sure this
>>> is still needed in production versions of the silicon used on boards
>>> supported in mainline?
>>
>> This piece of code is NOT copy paste from my side, it is an already
>> existing code. I just moved it into the function
>> exynos5250_pm_prepare().
>
> Sure, I'm aware of this. It might be good to know though if this is
> really necessary, as I don't think we want useless code.
>
>> However I removed this piece of code and
>> made a common function for exynos4 and exynos5250, S2R works on 5250
>> well even without this piece of code.
>
> Thanks for checking this. I wonder if we could get some clarification
> about this from hardware guys. Kukjin, Jingoo, any ideas or people that
> might know what this is about?
>
> For now, I believe it could be handled the same way as Exynos4, in PMU
> register array as I suggested in my previous reply (quoted below).
>
>>
>>>
>>> Even if yes, Exynos4 handles such registers in PMU register array in
>>> pmu.c, so I wonder whether it shouldn't be moved there and allow
>>> handling of all Exynos SoCs uniformly in this file.
>>>
+
+ exynos_pm_enter_sleep_mode();
+}
+
+static void exynos4_pm_prepare(void)
+{
+ /* Set wake-up mask registers */
+ exynos_pm_set_wakeup_mask();
+
+ s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
+
+ exynos_pm_enter_sleep_mode();
+}
>>>
>>> In fact, exynos4_pm_prepare() is a direct subset of
>>> exynos5250_pm_prepare(). This just confirms that it might be a good idea
>>> to just merge both functions into a single generic one.
>>
>> Right, can be merged into a common function which can be used for
>> exynos4 and exynos5250.
>> But I am afraid we still need specific functions in case of 5420.
>
> Could you provide some details about Exynos5420 specific things?
Please refer my post [1] for 5420 PMU and S2R Support
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/269956.html
>
>>
>>>
+
static void exynos_pm_central_suspend(void)
{
unsigned long tmp;
>
> [snip]
>
+
/* Platform-specific GIC callback */
gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
/* All wakeup disable */
tmp = pmu_raw_readl(S5P_WAKEUP_MASK);
- tmp |= ((0xFF << 8) | (0x1F << 1));
+ tmp |= pm_data->wake_disable_mask;
>>>
>>> Does this vary between SoCs?
>>
>> Yes, It is different in case of 5420.
>
> Could you provide more information about the difference?
In case of 5420, it is wake_disable_mask = (0x7F << 7) | (0x1F << 1)
>
> 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
--
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] ARM: EXYNOS: Refactor the pm code to use DT based lookup
On 06.08.2014 14:58, Vikas Sajjan wrote:
> On Wed, Aug 6, 2014 at 1:35 AM, Tomasz Figa wrote:
>> On 25.07.2014 13:49, Vikas Sajjan wrote:
[snip]
>
>>> +
>>> + /* Disable USE_RETENTION of JPEG_MEM_OPTION */
>>> + tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
>>> + tmp &= ~EXYNOS5_OPTION_USE_RETENTION;
>>> + pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);
>>
>> This looks completely like a copy paste from a vendor tree needed to
>> workaround some issues in early revisions of the SoC. Are you sure this
>> is still needed in production versions of the silicon used on boards
>> supported in mainline?
>
> This piece of code is NOT copy paste from my side, it is an already
> existing code. I just moved it into the function
> exynos5250_pm_prepare().
Sure, I'm aware of this. It might be good to know though if this is
really necessary, as I don't think we want useless code.
> However I removed this piece of code and
> made a common function for exynos4 and exynos5250, S2R works on 5250
> well even without this piece of code.
Thanks for checking this. I wonder if we could get some clarification
about this from hardware guys. Kukjin, Jingoo, any ideas or people that
might know what this is about?
For now, I believe it could be handled the same way as Exynos4, in PMU
register array as I suggested in my previous reply (quoted below).
>
>>
>> Even if yes, Exynos4 handles such registers in PMU register array in
>> pmu.c, so I wonder whether it shouldn't be moved there and allow
>> handling of all Exynos SoCs uniformly in this file.
>>
>>> +
>>> + exynos_pm_enter_sleep_mode();
>>> +}
>>> +
>>> +static void exynos4_pm_prepare(void)
>>> +{
>>> + /* Set wake-up mask registers */
>>> + exynos_pm_set_wakeup_mask();
>>> +
>>> + s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
>>> +
>>> + exynos_pm_enter_sleep_mode();
>>> +}
>>
>> In fact, exynos4_pm_prepare() is a direct subset of
>> exynos5250_pm_prepare(). This just confirms that it might be a good idea
>> to just merge both functions into a single generic one.
>
> Right, can be merged into a common function which can be used for
> exynos4 and exynos5250.
> But I am afraid we still need specific functions in case of 5420.
Could you provide some details about Exynos5420 specific things?
>
>>
>>> +
>>> static void exynos_pm_central_suspend(void)
>>> {
>>> unsigned long tmp;
[snip]
>>> +
>>> /* Platform-specific GIC callback */
>>> gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
>>>
>>> /* All wakeup disable */
>>> tmp = pmu_raw_readl(S5P_WAKEUP_MASK);
>>> - tmp |= ((0xFF << 8) | (0x1F << 1));
>>> + tmp |= pm_data->wake_disable_mask;
>>
>> Does this vary between SoCs?
>
> Yes, It is different in case of 5420.
Could you provide more information about the difference?
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] ARM: EXYNOS: Refactor the pm code to use DT based lookup
Hi Tomasz,
On Wed, Aug 6, 2014 at 1:35 AM, Tomasz Figa wrote:
> Hi Vikas,
>
> Please see my comments inline.
>
> On 25.07.2014 13:49, Vikas Sajjan wrote:
>> Refactoring the pm.c to avoid using "soc_is_exynos" checks,
>> instead use the DT based lookup.
>>
>> While at it, consolidate the common code across SoCs
>> and create a static helper functions.
>
> [snip]
>
>> @@ -241,13 +266,13 @@ static void exynos_cpu_restore_register(void)
>>
>> static int exynos_cpu_suspend(unsigned long arg)
>> {
>> -#ifdef CONFIG_CACHE_L2X0
>> - outer_flush_all();
>> -#endif
>> -
>> - if (soc_is_exynos5250())
>> - flush_cache_all();
>> + if (pm_data->cpu_suspend)
>> + return pm_data->cpu_suspend(arg);
>> + return -1;
>> +}
>
> I believe you could just pass pm_data->cpu_suspend to cpu_suspend(),
> without this three-liner.
>
OK.
>>
>> +static int exynos_cpu_do_idle(void)
>> +{
>> /* issue the standby signal into the pm unit. */
>> cpu_do_idle();
>>
>> @@ -257,32 +282,73 @@ static int exynos_cpu_suspend(unsigned long arg)
>>
>> static void exynos_pm_prepare(void)
>> {
>> - unsigned int tmp;
>> + if (pm_data->pm_prepare)
>> + pm_data->pm_prepare();
>
> You could just directly insert this code into exynos_suspend_enter()
> instead of adding this two-liner.
OK.
>
>> +}
>>
>> +static int exynos4_cpu_suspend(unsigned long arg)
>> +{
>> +#ifdef CONFIG_CACHE_L2X0
>> + outer_flush_all();
>> +#endif
>> + return exynos_cpu_do_idle();
>> +}
>> +
>> +static int exynos5250_cpu_suspend(unsigned long arg)
>> +{
>> +#ifdef CONFIG_CACHE_L2X0
>> + outer_flush_all();
>> +#endif
>> + flush_cache_all();
>> + return exynos_cpu_do_idle();
>> +}
>
> I believe both can be merged safely into the same single function. A
> call to flush_cache_all() will not hurt on Exynos4, but it should be
> moved before outer_flush_all().
>
> Moreover, the #ifdef CONFIG_CACHE_L2X0 is superfluous, because the
> existence of outer_flush_all() symbol doesn't depend on this Kconfig
> symbol (it depends on CONFIG_OUTER_CACHE_SYNC, but a stub is provided if
> it is disabled).
>
>> +
>> +static void exynos_pm_set_wakeup_mask(void)
>> +{
>> /* Set wake-up mask registers */
>> pmu_raw_writel(exynos_get_eint_wake_mask(), S5P_EINT_WAKEUP_MASK);
>> pmu_raw_writel(exynos_irqwake_intmask & ~(1 << 31), S5P_WAKEUP_MASK);
>> +}
>>
>> - s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
>> -
>> - if (soc_is_exynos5250()) {
>> - s3c_pm_do_save(exynos5_sys_save, ARRAY_SIZE(exynos5_sys_save));
>> - /* Disable USE_RETENTION of JPEG_MEM_OPTION */
>> - tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
>> - tmp &= ~EXYNOS5_OPTION_USE_RETENTION;
>> - pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);
>> - }
>> -
>> +static void exynos_pm_enter_sleep_mode(void)
>> +{
>> /* Set value of power down register for sleep mode */
>> -
>> exynos_sys_powerdown_conf(SYS_SLEEP);
>> pmu_raw_writel(S5P_CHECK_SLEEP, S5P_INFORM1);
>>
>> /* ensure at least INFORM0 has the resume address */
>> -
>> pmu_raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0);
>> }
>>
>> +static void exynos5250_pm_prepare(void)
>> +{
>> + unsigned int tmp;
>> +
>> + /* Set wake-up mask registers */
>> + exynos_pm_set_wakeup_mask();
>> +
>> + s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
>> +
>> + s3c_pm_do_save(exynos5_sys_save, ARRAY_SIZE(exynos5_sys_save));
>
> You could store a pointer to this array in new .extra_save field of the
> variant struct and handle this with generic code, like below:
>
> if (pm_data->extra_save)
> s3c_pm_do_save(pm_data->extra_save,
> pm_data->num_extra_save);
>
OK.
>> +
>> + /* Disable USE_RETENTION of JPEG_MEM_OPTION */
>> + tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
>> + tmp &= ~EXYNOS5_OPTION_USE_RETENTION;
>> + pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);
>
> This looks completely like a copy paste from a vendor tree needed to
> workaround some issues in early revisions of the SoC. Are you sure this
> is still needed in production versions of the silicon used on boards
> supported in mainline?
This piece of code is NOT copy paste from my side, it is an already
existing code. I just moved it into the function
exynos5250_pm_prepare(). However I removed this piece of code and
made a common function for exynos4 and exynos5250, S2R works on 5250
well even without this piece of code.
>
> Even if yes, Exynos4 handles such registers in PMU register array in
> pmu.c, so I wonder whether it shouldn't be moved there and allow
> handling of all Exynos SoCs uniformly in this file.
>
>> +
>> + exynos_pm_enter_sleep_mode();
>> +}
>> +
>> +static void exynos4_pm_prepare(void)
>> +{
>> + /* Set wake-up mask registers */
>> + exynos_pm_set_wakeup_mask();
>> +
>> +
Re: [PATCH v2 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
On Wed, Aug 06, 2014 at 04:11:54PM +0900, Inki Dae wrote: > On 2014년 08월 05일 20:12, Thierry Reding wrote: [...] > > I think that low power mode is more often used for command transmission > > (in host-driven mode). I'm not sure how much sense it really makes to > > transmit video data in low power mode. It also seems like low power mode > > is what all peripherals need to support (if they can do command mode). > > Hence I'd like to propose the attached patch that makes all command > > messages use low power mode. > > To use low power mode, I think SoC drivers should add more codes: > checking xxx_MSG_LPM, and maybe disabling HS clock. My patch does > exactly that, > http://www.spinics.net/lists/linux-samsung-soc/msg34866.html I agree in general that DSI host drivers need to check the flags to make a decision about which mode to enable. But your patch also introduces additional flags that I don't think are necessary (at least for any of the use-cases discussed so far). As I understand it the MIPI_DSI_MODE_CMD_LPM and MIPI_DSI_MODE_VIDEO_LPM flags that you introduce would advertise that the device only supports low power mode for command or video modes respectively. However, I doubt that there really are devices that only support low power video mode. It wouldn't make much sense because you'd get a maximum of 10 MHz for the clock, which is about 1.6 frames per second at 1920x1080 resolution (not counting blanking). And even with lower resolutions such as 1024x768 it would be somewhere around 4 frames per second. And I think it's reasonable to assume that we'll see that kind of resolution become very rare in the future. So my point is that devices which support video mode will always support high speed mode for video transmission too. Similarly, if a device supports command mode, then it will likely support it in low-power mode, and optionally in high speed mode too. > And what I and Andrzej don't make sure is non-continuous clock mode. Do > you know how non-continuous clock mode is related to HS clock? 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. If a device doesn't specify that flag then the host needs to keep the HS clock running all the time. > > The .transfer() function was really designed with initialization > > commands in mind, so it doesn't deal with mixing video data and commands > > anyway and for initialization low-power mode should be fast enough. The > > downside is that it may not be optimal for some peripherals, but it > > gives us a good solution for the general case since it should support > > all devices. > > > > If we absolutely must have faster initialization, or if we come across a > > device that can only initialize in high speed mode, then I think we > > should introduce a new flag to allow DSI host controllers to optimize in > > those cases. > > Originally, mipi-dsi framework enforces implicitly using HS mode for > video and command data as default so I added LPM relevant flags - for > video and also command data Yes, it transmits in HS mode by default. Quite frankly, I'm not sure if that's really the right default. Given that .transfer() is meant for sending synchronous commands, low-power mode would probably be a better default. > - for host driver can consider Low Power Mode. However, your patch makes > it use Low Power Mode for command data as default. Not as default. It just means that all messages that are sent using the standard functions use low-power mode. Drivers could still override the default by constructing the messages themselves. > So your patch would mean that default transfer mode for command data is > Low Power Mode, but HS mode for video data. Exactly. For the reasons specified above, I'd expect peripherals to always support command transmissions in low-power mode, whereas I'd equally expect them to support video transmission in high-speed mode. Therefore I think the default should be to send commands in low-power mode and video data in high speed mode (by default), because those are the normal cases. If we ever encounter a device that requires something different we can always introduce an additional flag/quirk at that point. > Do you intend to control transfer mode - HS or LPM - only for command > data? If so, we would need only one flag, i.e., MIPI_DSI_MODE_HS. We already have that flag, it's called MIPI_DSI_MSG_USE_LPM.
Re: [PATCH v2 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
Thanks for comments. On 2014년 08월 05일 20:12, Thierry Reding wrote: > On Mon, Jul 28, 2014 at 06:09:58PM +0200, Andrzej Hajda wrote: >> On 07/28/2014 04:00 AM, Inki Dae wrote: >>> This patch adds below two flags for LPM transfer, and it attaches LPM flags >>> to a msg in accordance with master's mode_flags set by LCD Panel driver. >>> >>> MIPI_DSI_MODE_CMD_LPM >>> - If this flag is set by Panel driver, MIPI-DSI controller will tranfer >>> command data to Panel device in Low Power Mode. >> >> What do you mean by command data? It could be: >> - all transfer in command mode of operations, >> - transfer initialized by the driver by writing to DSIM registers. >> >>> >>> MIPI_DSI_MODE_VIDEO_LPM >>> - If this flag is set by Panel driver, MIPI-DSI controller will tranfer >>> image data to Panel device in Low Power Mode. >> >> What is the meaning of this flag in case of command mode of operation? >> >> Maybe it would be better to create flags based on source of data/FIFOs: >> - commands send by SFR registers, >> - commands generated from data sent from Display Controller. > > I have no idea what SFR is. But it sounds like you're talking about two > ways to generate command packets here. We have something similar on > Tegra, where it's called "host-driven command mode" and "DC-driven > command mode". > > In host-driven command mode the driver needs to explicitly program some > of the DSI controller registers to send command packets. This is > essentially a FIFO register and a control register to trigger > transmission and poll for completion. > > DC-driven command mode is typically used to update contents of a remote > framebuffer (what MIPI calls "Type 1 Display Architecture"). This is > done by programming a different set of registers that cause the DSI > controller to take data output by the display controller and wrap it > into DCS commands (e.g. write_memory_start and write_memory_continue). > Exynos also supports above two command mode but we have never used DC-driven command mode. > I think that low power mode is more often used for command transmission > (in host-driven mode). I'm not sure how much sense it really makes to > transmit video data in low power mode. It also seems like low power mode > is what all peripherals need to support (if they can do command mode). > Hence I'd like to propose the attached patch that makes all command > messages use low power mode. To use low power mode, I think SoC drivers should add more codes: checking xxx_MSG_LPM, and maybe disabling HS clock. My patch does exactly that, http://www.spinics.net/lists/linux-samsung-soc/msg34866.html And what I and Andrzej don't make sure is non-continuous clock mode. Do you know how non-continuous clock mode is related to HS clock? > > The .transfer() function was really designed with initialization > commands in mind, so it doesn't deal with mixing video data and commands > anyway and for initialization low-power mode should be fast enough. The > downside is that it may not be optimal for some peripherals, but it > gives us a good solution for the general case since it should support > all devices. > > If we absolutely must have faster initialization, or if we come across a > device that can only initialize in high speed mode, then I think we > should introduce a new flag to allow DSI host controllers to optimize in > those cases. Originally, mipi-dsi framework enforces implicitly using HS mode for video and command data as default so I added LPM relevant flags - for video and also command data - for host driver can consider Low Power Mode. However, your patch makes it use Low Power Mode for command data as default. So your patch would mean that default transfer mode for command data is Low Power Mode, but HS mode for video data. Do you intend to control transfer mode - HS or LPM - only for command data? If so, we would need only one flag, i.e., MIPI_DSI_MODE_HS. Thanks, Inki Dae > > Note that this is based on some of my local patches, so it won't apply > as-is. But if anybody wants to give this a go it should be easy to apply > manually as well. > > Thierry > -- 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
