Re: [PATCH 1/1] mmc: pwrseq: Fix error code propagation in mmc_pwrseq_simple_alloc()

2015-04-13 Thread Alexandre Courbot 

On 04/13/2015 11:07 PM, Javier Martinez Canillas wrote:

If the struct mmc_pwrseq_match .alloc function used to allocate a
struct mmc_pwrseq fails, the error is propagated to mmc_of_parse().

But instead of returning the error code in pwrseq, host-pwrseq is
returned which will always be 0. So mmc_of_parse() succeeds even if
the pwrseq .alloc function failed and host-pwrseq is NULL.

This makes the SDIO device to not be powered if the power sequencing
.alloc functions wants to be deferred due a missing resource because
the mmc controller driver probe did wrongly succeed.

Fixes: 0f12a0ce4ce4a (mmc: pwrseq: simplify alloc/free hooks)
Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk


I obviously overlooked that one. Thanks for fixing it.

Reviewed-by: Alexandre Courbot acour...@nvidia.com
--
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: DT on s3c24xx

2014-12-18 Thread Alexandre Courbot 
On Thu, Dec 18, 2014 at 5:34 PM, Vasily Khoruzhick anars...@gmail.com wrote:
 On Thu, Dec 18, 2014 at 10:52 AM, Uwe Kleine-König
 u.kleine-koe...@pengutronix.de wrote:
 Hello,

 Hi Uwe,

 [Cc += linusw, linux-gpio]

 On Wed, Dec 17, 2014 at 10:04:24PM +0300, Vasily Khoruzhick wrote:
 I'd like to port several s3c24xx to DT, and I'm stuck with s3c24xx LCD
 controller and power drivers for H1940 and RX1950.

 Please see [1]. I want to move this function into another LCD power
 driver, but I'm not sure what to do with s3c_gpio_cfgpin(). I need to
 change pin function in runtime, and as far as I understand it should
 be handled via pinctrl driver somehow. But how?
 You can pass 1 pinctrl setups to a node:

 somedevice {
 pinctrl-names = default, foo, bar;
 pinctrl-0 = pinctrl_somedevice_default;
 pinctrl-1 = pinctrl_somedevice_foo;
 pinctrl-2 = pinctrl_somedevice_bar;
 cfg-gpios = gpio4 12 3, gpio2 7 5;
 };

 Then I think you can fiddle with pinctrl_select_state(). For the gpios
 you can then use the standard gpiod_{request,direction_{in,out}put}
 combo.

 Thanks for you response!

 Can I change pin function after gpio was requested? In low-power state
 (i.e. when display is disabled) it's gpio driving some level,
 and in active state it's some LCD controller pin (don't remember which
 one exactly)

If your driver can release the GPIO and change the pinctrl when
switching to active state (and change the pinctrl again and request
the GPIO when switching to low-power state) then this should be
doable. I suspect the switch is made by the same driver anyway, isn't
it?
--
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 6/8] regulator: max77686: Add external GPIO control

2014-10-31 Thread Alexandre Courbot 
On Fri, Oct 31, 2014 at 4:51 PM, Krzysztof Kozlowski
k.kozlow...@samsung.com wrote:
 On pią, 2014-10-31 at 12:31 +0900, Alexandre Courbot wrote:
 On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski
 k.kozlow...@samsung.com wrote:
  On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
  Hi, and thanks for bringing this issue to us!
 
  On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
  javier.marti...@collabora.co.uk wrote:
   [adding Linus and Alexandre to the cc list]
  
   Hello Krzysztof,
  
   On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
   On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
   On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
 Hello Krzysztof,

 On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
  @@ -85,6 +91,9 @@ struct max77686_data {
 struct max77686_regulator_data *regulators;
 int num_regulators;
 
  +  /* Array of size num_regulators with GPIOs for external 
  control. */
  +  int *ext_control_gpio;
  +

 The integer-based GPIO API is deprecated in favor of the 
 descriptor-based GPIO
 interface (Documentation/gpio/consumer.txt). Could you please use 
 the later?
   
Sure, I can. Please have in mind that regulator core still accepts 
old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.
  
   It seems I was too hasty... I think usage of the new gpiod API implies
   completely different bindings.
  
   The gpiod_get() gets GPIO from a device level, not from given sub-node
   pointer. This means that you cannot have DTS like this:
   ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpy2 0 0;
   };
  
   ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpk0 2 0;
   };
  
  
   I could put GPIOs in device node:
  
   max77686_pmic@09 {
compatible = maxim,max77686;
interrupt-parent = gpx0;
interrupts = 7 0;
reg = 0x09;
#clock-cells = 1;
ldo21-gpio = gpy2 0 0;
ldo22-gpio = gpk0 2 0;
  
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
  
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
  
   This would work but I don't like it. The properties of a regulator are
   above the node configuring that regulator.
  
   Any ideas?
  
  
   Continuing talking to myself... I found another problem - GPIO cannot 
   be
   requested more than once (-EBUSY). In case of this driver (and board:
   Trats2) one GPIO is connected to regulators. The legacy GPIO API and
   regulator core handle this.
  
   With new GPIO API I would have to implement some additional steps in
   such case...
  
   So there are 2 issues:
   1. Cannot put GPIO property in regulator node.
 
  For this problem you will probably want to use the
  dev(m)_get_named_gpiod_from_child() function from the following patch:
 
  https://lkml.org/lkml/2014/10/6/529
 
  It should reach -next soon now.
 
  Thanks! Probably I would switch to top level gpios property anyway
  (other reasons) but it would be valuable in some cases to specify them
  per child node.

 Mmm, but doesn't it make more sense to have them in the child nodes?

 Yes, it makes more sense. Using old way of parsing regulators from DT it
 was straightforward.

 However new DT style parsing (regulator_of_get_init_data()) does the
 basic parsing stuff and this removes a lot of code from driver. The
 driver no longer parses all regulaotrs but the regulator core does it.
 Unfortunately regulator core does not parse custom bindings (like such
 GPIOs) so I would have to iterate once again through all regulators just
 to find gpios property.

 It is simpler just to do something like (5 regulators can be controlled
 by GPIO):

 max77686_pmic_dt_parse_gpio_control(struct platform_device *pdev, *gpio)
 {
   gpio[MAX77686_LDO20] = of_get_named_gpio(np, ldo20-gpios, 0);
   gpio[MAX77686_LDO21] = of_get_named_gpio(np, ldo21-gpios, 0);
   gpio[MAX77686_LDO22] = of_get_named_gpio(np, ldo22-gpios, 0);
   gpio[MAX77686_BUCK8] = of_get_named_gpio(np, buck8-gpios, 0);
   gpio[MAX77686_BUCK9] = of_get_named_gpio(np, buck9-gpios, 0);
 }

It is simpler from the driver's perspective, but if I understand
correctly DT bindings

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

2014-10-30 Thread Alexandre Courbot 
Hi, and thanks for bringing this issue to us!

On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
javier.marti...@collabora.co.uk wrote:
 [adding Linus and Alexandre to the cc list]

 Hello Krzysztof,

 On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
 On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
 On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
  On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
   Hello Krzysztof,
  
   On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
   struct max77686_regulator_data *regulators;
   int num_regulators;
   
+  /* Array of size num_regulators with GPIOs for external 
control. */
+  int *ext_control_gpio;
+
  
   The integer-based GPIO API is deprecated in favor of the 
   descriptor-based GPIO
   interface (Documentation/gpio/consumer.txt). Could you please use the 
   later?
 
  Sure, I can. Please have in mind that regulator core still accepts old
  GPIO so I will have to use desc_to_gpio(). That should work... and
  should be future-ready.

 It seems I was too hasty... I think usage of the new gpiod API implies
 completely different bindings.

 The gpiod_get() gets GPIO from a device level, not from given sub-node
 pointer. This means that you cannot have DTS like this:
 ldo21_reg: ldo21 {
  regulator-compatible = LDO21;
  regulator-name = VTF_2.8V;
  regulator-min-microvolt = 280;
  regulator-max-microvolt = 280;
  ec-gpio = gpy2 0 0;
 };

 ldo22_reg: ldo22 {
  regulator-compatible = LDO22;
  regulator-name = VMEM_VDD_2.8V;
  regulator-min-microvolt = 280;
  regulator-max-microvolt = 280;
  ec-gpio = gpk0 2 0;
 };


 I could put GPIOs in device node:

 max77686_pmic@09 {
  compatible = maxim,max77686;
  interrupt-parent = gpx0;
  interrupts = 7 0;
  reg = 0x09;
  #clock-cells = 1;
  ldo21-gpio = gpy2 0 0;
  ldo22-gpio = gpk0 2 0;

  ldo21_reg: ldo21 {
  regulator-compatible = LDO21;
  regulator-name = VTF_2.8V;
  regulator-min-microvolt = 280;
  regulator-max-microvolt = 280;
  };

  ldo22_reg: ldo22 {
  regulator-compatible = LDO22;
  regulator-name = VMEM_VDD_2.8V;
  regulator-min-microvolt = 280;
  regulator-max-microvolt = 280;
  };

 This would work but I don't like it. The properties of a regulator are
 above the node configuring that regulator.

 Any ideas?


 Continuing talking to myself... I found another problem - GPIO cannot be
 requested more than once (-EBUSY). In case of this driver (and board:
 Trats2) one GPIO is connected to regulators. The legacy GPIO API and
 regulator core handle this.

 With new GPIO API I would have to implement some additional steps in
 such case...

 So there are 2 issues:
 1. Cannot put GPIO property in regulator node.

For this problem you will probably want to use the
dev(m)_get_named_gpiod_from_child() function from the following patch:

https://lkml.org/lkml/2014/10/6/529

It should reach -next soon now.

 2. Cannot request some GPIO more than once.

We have been confronted to this problem with the regulator core as well:

http://marc.info/?l=linux-arm-kernelm=140417649119733w=1

I have a draft patch that allows GPIOs to be requested by several
clients. What prevented me from submitting it was that I wanted to
make sure the different requested configurations were compatible, but
maybe I am overthinking this. There are also a couple of other patches
that this depends on (like removal of the big descs array), so I don't
think it will be available too soon, sadly.

So maybe your best shot for now is to keep using the integer API, as
much as I hate it. Once we become able to request the same GPIO
several times, you should be good to switch to descriptors. Sorry this
has not been done faster.
--
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 6/8] regulator: max77686: Add external GPIO control

2014-10-30 Thread Alexandre Courbot 
On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski
k.kozlow...@samsung.com wrote:
 On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
 Hi, and thanks for bringing this issue to us!

 On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
 javier.marti...@collabora.co.uk wrote:
  [adding Linus and Alexandre to the cc list]
 
  Hello Krzysztof,
 
  On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
  On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
  On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
   On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
Hello Krzysztof,
   
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
 @@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;

 +  /* Array of size num_regulators with GPIOs for external 
 control. */
 +  int *ext_control_gpio;
 +
   
The integer-based GPIO API is deprecated in favor of the 
descriptor-based GPIO
interface (Documentation/gpio/consumer.txt). Could you please use 
the later?
  
   Sure, I can. Please have in mind that regulator core still accepts old
   GPIO so I will have to use desc_to_gpio(). That should work... and
   should be future-ready.
 
  It seems I was too hasty... I think usage of the new gpiod API implies
  completely different bindings.
 
  The gpiod_get() gets GPIO from a device level, not from given sub-node
  pointer. This means that you cannot have DTS like this:
  ldo21_reg: ldo21 {
   regulator-compatible = LDO21;
   regulator-name = VTF_2.8V;
   regulator-min-microvolt = 280;
   regulator-max-microvolt = 280;
   ec-gpio = gpy2 0 0;
  };
 
  ldo22_reg: ldo22 {
   regulator-compatible = LDO22;
   regulator-name = VMEM_VDD_2.8V;
   regulator-min-microvolt = 280;
   regulator-max-microvolt = 280;
   ec-gpio = gpk0 2 0;
  };
 
 
  I could put GPIOs in device node:
 
  max77686_pmic@09 {
   compatible = maxim,max77686;
   interrupt-parent = gpx0;
   interrupts = 7 0;
   reg = 0x09;
   #clock-cells = 1;
   ldo21-gpio = gpy2 0 0;
   ldo22-gpio = gpk0 2 0;
 
   ldo21_reg: ldo21 {
   regulator-compatible = LDO21;
   regulator-name = VTF_2.8V;
   regulator-min-microvolt = 280;
   regulator-max-microvolt = 280;
   };
 
   ldo22_reg: ldo22 {
   regulator-compatible = LDO22;
   regulator-name = VMEM_VDD_2.8V;
   regulator-min-microvolt = 280;
   regulator-max-microvolt = 280;
   };
 
  This would work but I don't like it. The properties of a regulator are
  above the node configuring that regulator.
 
  Any ideas?
 
 
  Continuing talking to myself... I found another problem - GPIO cannot be
  requested more than once (-EBUSY). In case of this driver (and board:
  Trats2) one GPIO is connected to regulators. The legacy GPIO API and
  regulator core handle this.
 
  With new GPIO API I would have to implement some additional steps in
  such case...
 
  So there are 2 issues:
  1. Cannot put GPIO property in regulator node.

 For this problem you will probably want to use the
 dev(m)_get_named_gpiod_from_child() function from the following patch:

 https://lkml.org/lkml/2014/10/6/529

 It should reach -next soon now.

 Thanks! Probably I would switch to top level gpios property anyway
 (other reasons) but it would be valuable in some cases to specify them
 per child node.

Mmm, but doesn't it make more sense to have them in the child nodes?
Besides if the bindings of this driver have already been published,
I'm afraid you will have to maintain backward compability.



  2. Cannot request some GPIO more than once.

 We have been confronted to this problem with the regulator core as well:

 http://marc.info/?l=linux-arm-kernelm=140417649119733w=1

 I have a draft patch that allows GPIOs to be requested by several
 clients. What prevented me from submitting it was that I wanted to
 make sure the different requested configurations were compatible, but
 maybe I am overthinking this. There are also a couple of other patches
 that this depends on (like removal of the big descs array), so I don't
 think it will be available too soon, sadly.

 Shouldn't be the nature of get()/put() interface to allow multiple
 requests?

Only if it makes sense for the resource to be requested multiple
times. GPIOs are kind of a difficult case here. Consumers could ask
for e.g. conflicting directions. But for cases where it should work I
agree that multiple consumers would be welcome.

 To me it was a kind of intuitive that I could do another
 devm_gpiod_get() for the same gpio. But then it hit me with EBUSY :).


 So maybe your best shot for now is to keep using the integer API, as
 much as I hate it. Once we become able to request the same GPIO
 several times, you should

Re: [PATCH] drm/exynos: fix vblank handling during dpms off

2014-10-09 Thread Alexandre Courbot 
On Thu, Oct 9, 2014 at 7:08 PM, Thierry Reding tred...@nvidia.com wrote:
 On Thu, Oct 09, 2014 at 02:43:02PM +0900, Alexandre Courbot wrote:
 On 10/02/2014 08:52 PM, Andrzej Hajda wrote:
 Hi,
 
 +CC possible victims
 
 On 10/02/2014 12:52 PM, Inki Dae wrote:
 On 2014년 10월 02일 17:58, Joonyoung Shim wrote:
 Hi Andrzej,
 
 On 10/01/2014 05:14 PM, Andrzej Hajda wrote:
 The patch disables vblanks during dpms off only if pagefilp has
 not been finished. It also replaces drm_vblank_off with 
 drm_crtc_vblank_put.
 It fixes issue with page_flip ioctl not being able to acquire vblank 
 counter.
 This problem isn't related with pageflip, it just causes from
 7ffd7a68511c710b84db3548a1997fd2625f580a commit (drm: Always reject
 drm_vblank_get() after drm_vblank_off()).
 
 We need to use drm_vblank_on() as a counterpart to drm_vblank_off()
 after the commit .
 
 This patch should break also other drivers, it seems at least following
 drms could be affected:
 armada, sti, tegra.

 Indeed we (tegra) have just been hit by this. The problem seems to come from
 the fact that we have been using drm_vblank_pre_modeset,
 drm_vblank_post_modeset and drm_vblank_off conjointly. All these functions
 depend on the value of vblank-inmodeset, and 7ffd7a68511 increases the
 vblank reference counter only in drm_vblank_off, which can result in the
 acquired reference never being released.

 The following seems to fix this for Tegra, by stopping using
 drm_vblank_pre/post_modeset and relying on drm_vblank_off/on instead:

 diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
 index b08df07cad47..3955d81236d0 100644
 --- a/drivers/gpu/drm/tegra/dc.c
 +++ b/drivers/gpu/drm/tegra/dc.c
 @@ -739,7 +739,6 @@ static const struct drm_crtc_funcs tegra_crtc_funcs = {

  static void tegra_crtc_disable(struct drm_crtc *crtc)
  {
 -   struct tegra_dc *dc = to_tegra_dc(crtc);
 struct drm_device *drm = crtc-dev;
 struct drm_plane *plane;

 @@ -755,7 +754,7 @@ static void tegra_crtc_disable(struct drm_crtc *crtc)
 }
 }

 -   drm_vblank_off(drm, dc-pipe);
 +   drm_crtc_vblank_off(crtc);
  }

  static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
 @@ -844,7 +843,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
 u32 value;
 int err;

 -   drm_vblank_pre_modeset(crtc-dev, dc-pipe);
 +   drm_crtc_vblank_off(crtc);

 err = tegra_crtc_setup_clk(crtc, mode);
 if (err) {
 @@ -946,7 +945,7 @@ static void tegra_crtc_commit(struct drm_crtc *crtc)
 value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
 tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);

 -   drm_vblank_post_modeset(crtc-dev, dc-pipe);
 +   drm_crtc_vblank_on(crtc);
  }

  static void tegra_crtc_load_lut(struct drm_crtc *crtc)

 Thierry, does this look ok to you?

 Yes, that looks like almost the same patch that I sent out yesterday.
 The difference is that I didn't replace the drm_vblank_pre_modeset()
 call with drm_vblank_off() like you did, but rather just dropped the
 former.

 I /think/ your version is more correct in that regard.

Feel free to take that extra line in your patch then. ;)


 Thierry

 But there might be another issue, which is that calls to drm_vblank_get()
 will return -EINVAL if invoked between drm_blank_off and drm_blank_on. Is
 this really the desired behavior? Can it at least happen? If so, how are
 drivers supposed to react to this situation?

 It shouldn't happen. If drm_vblank_off() and drm_vblank_on() are called
 around a modeset they should never conflict with drm_vblank_get(), at
 least on Tegra, because the modeset and page-flip IOCTLs will be
 serialized.

Ok, that's good. I was just wondering whether this case has been thought of.

Actually, and seeing how drm_vblank_pre/post_modeset() have become
useless (if not harmful), maybe it would be a good idea to come with a
fixup patch that gets rid of them altogether and make their callers
invoke drm_vblank_off/on() instead?
--
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] drm/exynos: fix vblank handling during dpms off

2014-10-08 Thread Alexandre Courbot 

On 10/02/2014 08:52 PM, Andrzej Hajda wrote:

Hi,

+CC possible victims

On 10/02/2014 12:52 PM, Inki Dae wrote:

On 2014년 10월 02일 17:58, Joonyoung Shim wrote:

Hi Andrzej,

On 10/01/2014 05:14 PM, Andrzej Hajda wrote:

The patch disables vblanks during dpms off only if pagefilp has
not been finished. It also replaces drm_vblank_off with drm_crtc_vblank_put.
It fixes issue with page_flip ioctl not being able to acquire vblank counter.

This problem isn't related with pageflip, it just causes from
7ffd7a68511c710b84db3548a1997fd2625f580a commit (drm: Always reject
drm_vblank_get() after drm_vblank_off()).

We need to use drm_vblank_on() as a counterpart to drm_vblank_off()
after the commit .


This patch should break also other drivers, it seems at least following
drms could be affected:
armada, sti, tegra.


Indeed we (tegra) have just been hit by this. The problem seems to come 
from the fact that we have been using drm_vblank_pre_modeset, 
drm_vblank_post_modeset and drm_vblank_off conjointly. All these 
functions depend on the value of vblank-inmodeset, and 7ffd7a68511 
increases the vblank reference counter only in drm_vblank_off, which can 
result in the acquired reference never being released.


The following seems to fix this for Tegra, by stopping using 
drm_vblank_pre/post_modeset and relying on drm_vblank_off/on instead:


diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index b08df07cad47..3955d81236d0 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -739,7 +739,6 @@ static const struct drm_crtc_funcs tegra_crtc_funcs = {

 static void tegra_crtc_disable(struct drm_crtc *crtc)
 {
-   struct tegra_dc *dc = to_tegra_dc(crtc);
struct drm_device *drm = crtc-dev;
struct drm_plane *plane;

@@ -755,7 +754,7 @@ static void tegra_crtc_disable(struct drm_crtc *crtc)
}
}

-   drm_vblank_off(drm, dc-pipe);
+   drm_crtc_vblank_off(crtc);
 }

 static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
@@ -844,7 +843,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
u32 value;
int err;

-   drm_vblank_pre_modeset(crtc-dev, dc-pipe);
+   drm_crtc_vblank_off(crtc);

err = tegra_crtc_setup_clk(crtc, mode);
if (err) {
@@ -946,7 +945,7 @@ static void tegra_crtc_commit(struct drm_crtc *crtc)
value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);

-   drm_vblank_post_modeset(crtc-dev, dc-pipe);
+   drm_crtc_vblank_on(crtc);
 }

 static void tegra_crtc_load_lut(struct drm_crtc *crtc)

Thierry, does this look ok to you?

But there might be another issue, which is that calls to 
drm_vblank_get() will return -EINVAL if invoked between drm_blank_off 
and drm_blank_on. Is this really the desired behavior? Can it at least 
happen? If so, how are drivers supposed to react to this situation?

--
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] ARM: firmware: Introduce suspend and resume operations

2014-06-25 Thread Alexandre Courbot 

On 06/26/2014 01:18 AM, Tomasz Figa wrote:

This patch extends the firmware_ops structure with two new callbacks:
.suspend() and .resume(). The former is intended to ask the firmware to
save all its volatile state and suspend the system, without returning
back to the kernel in between. The latter is to be called early by
very low level platform suspend code after waking up to restore low
level hardware state, which can't be restored in non-secure mode.

While at it, outdated version of the structure is removed from the
documentation and replaced with a reference to the header file.


Acked-by: Alexandre Courbot acour...@nvidia.com
--
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] ARM: move firmware_ops to drivers/firmware

2013-11-19 Thread Alexandre Courbot 
On Tue, Nov 19, 2013 at 9:26 PM, Catalin Marinas
catalin.mari...@arm.com wrote:
 On Tue, Nov 19, 2013 at 02:46:55AM +, Alex Courbot wrote:
 On 11/18/2013 08:58 PM, Catalin Marinas wrote:
  On Mon, Nov 18, 2013 at 03:05:59AM +, Alex Courbot wrote:
  On 11/18/2013 12:59 AM, Catalin Marinas wrote:
  On 17 November 2013 08:49, Alexandre Courbot acour...@nvidia.com wrote:
  The ARM tree includes a firmware_ops interface that is designed to
  implement support for simple, TrustZone-based firmwares but could
  also cover other use-cases. It has been suggested that this
  interface might be useful to other architectures (e.g. arm64) and
  that it should be moved out of arch/arm.
 
  NAK. I'm for code sharing with arm via common locations but this API
  goes against the ARMv8 firmware standardisation efforts like PSCI,
  encouraging each platform to define there own non-standard interface.
 
  I have to say, I pretty much agree with your NAK.
 
  The reason for this patch is that the suggestion to move firmware_ops
  out of arch/arm is the last (I hope) thing that prevents my Trusted
  Foundation support series from being merged.
 
  Moving it into drivers shouldn't be a workaround. Nice try ;).

 Hehe. I thought that just sending a patch would settle the issue one way
 or the other and avoid a huge discussion. Woke up this morning to see
 how wrong I was.

 It's a sensitive topic ;).

  BTW, is legacy code the reason for not converting the SMC # to PSCI?
  It's already supported on ARMv7, so you may not have much code left to
  merge in the kernel ;).

 The problem here is twofold:

 1) we are just consumers of the TrustZone secure monitor who receive a
 binary and do not have any control over its calling conventions. I agree
 that it would be trivial to make it compatible with PSCI, but it's just
 not something we can make by ourselves (TF does not even follow the SMC
 calling convention). If this problem is to be addressed, it should be
 done by forcing the TrustZone secure monitors providers to follow PSCI.

 I agree and such discussions do happen ('forcing' is a bit harder, more
 like 'strongly recommending'). On my side, I voice this message via the
 Linux channels, so SoC vendors can also encourage their secure provider
 in this direction. The benefit is that the Linux changes are minimal
 afterwards, single image is easier.

 But as I replied to Stephen, make sure you separate the secure OS (EL1)
 from the secure firmware (EL3). The latter (or parts of it) are provided
 by the SoC vendor (e.g. NVidia) and may be eventually linked into a big
 blob by the secure OS provider. ARM is encouraging separation here and a
 multi-stage firmware loading approach (and ARM started a public generic
 firmware project, it's in the early days now).

Will keep that in mind and check whether that could apply to future
devices, thanks.


 2) devices have already shipped with this firmware. Are we going to just
 renounce supporting them, even though the necessary support is
 lightweight and fits within already existing interfaces?

 I'm talking only about ARMv8 here. Please see my reply to Stephen for
 the details of (not) reusing existing firmware.

 I certainly do hope that for ARMv8 things will be different and more
 standardized. But that's not something that can be guaranteed unless ARM
 strongly enforces it to firmware vendors. In case such a non-standard
 firmware gets used again, I *do* hope that using cpu_ops will be
 preferred over saying this device cannot be supported in mainline, ever.

 cpu_ops or firmware_ops is just a name and can be unified (TBD what
 common functionality it contains). What I don't want to encourage is
 each SoC registering its own firmware interface.

Sorry, are you talking about interface as in SMC interface, or as in
cpu_operations/firmware_ops?


 The kernel already supports non-standard hardware, BIOS, ACPI through
 hacks that are *way* more horrible than that. This should certainly not
 be encouraged, but that's not a valid reason to forbid otherwise
 perfectly fine devices to run mainline IMHO.

 So you say we should just stop trying to standardise anything because
 people don't care anyway. Why do we even bother with DT (or ACPI) since
 board files were fine in the past (with a bit more code)?

Oh no, that's not what I am saying at all. Standardization is good.
PSCI is good. Of course I would prefer that the secure monitor we use
follow established conventions - that'd be less work to support it and
less hassle to get my patches merged.

I may have misunderstood you, but I felt your mail sounded a bit like
we won't merge support for firmwares that do not follow PSCI. I
agree that whenever it is possible to support a firmware through a
standard interface, this should be done - no discussion. But right now
I have two devices that are good representatives of Tegra 4 and
available in stores, which I would like to see supported in mainline
to satisfy requests from the community

Re: [PATCH] ARM: move firmware_ops to drivers/firmware

2013-11-19 Thread Alexandre Courbot 
On Wed, Nov 20, 2013 at 12:07 AM, Catalin Marinas
catalin.mari...@arm.com wrote:
 On Tue, Nov 19, 2013 at 02:29:39PM +, Alexandre Courbot wrote:
 On Tue, Nov 19, 2013 at 9:26 PM, Catalin Marinas
 catalin.mari...@arm.com wrote:
  On Tue, Nov 19, 2013 at 02:46:55AM +, Alex Courbot wrote:
  2) devices have already shipped with this firmware. Are we going to just
  renounce supporting them, even though the necessary support is
  lightweight and fits within already existing interfaces?
 
  I'm talking only about ARMv8 here. Please see my reply to Stephen for
  the details of (not) reusing existing firmware.
 
  I certainly do hope that for ARMv8 things will be different and more
  standardized. But that's not something that can be guaranteed unless ARM
  strongly enforces it to firmware vendors. In case such a non-standard
  firmware gets used again, I *do* hope that using cpu_ops will be
  preferred over saying this device cannot be supported in mainline, ever.
 
  cpu_ops or firmware_ops is just a name and can be unified (TBD what
  common functionality it contains). What I don't want to encourage is
  each SoC registering its own firmware interface.

 Sorry, are you talking about interface as in SMC interface, or as in
 cpu_operations/firmware_ops?

 Both. I don't want to see platforms defining their own SMC interface for
 no good reason. The cpu_ops/firmware_ops handling in the kernel is just
 some naming but the key is having standard SMC interfaces for CPU
 operations.

Fair enough.


  The kernel already supports non-standard hardware, BIOS, ACPI through
  hacks that are *way* more horrible than that. This should certainly not
  be encouraged, but that's not a valid reason to forbid otherwise
  perfectly fine devices to run mainline IMHO.
 
  So you say we should just stop trying to standardise anything because
  people don't care anyway. Why do we even bother with DT (or ACPI) since
  board files were fine in the past (with a bit more code)?

 Oh no, that's not what I am saying at all. Standardization is good.
 PSCI is good. Of course I would prefer that the secure monitor we use
 follow established conventions - that'd be less work to support it and
 less hassle to get my patches merged.

 I may have misunderstood you, but I felt your mail sounded a bit like
 we won't merge support for firmwares that do not follow PSCI.

 Just to clarify it: I won't merge support for _ARMv8_ firmware that does
 not follow a standard CPU booting/power protocol supported by Linux.
 Currently we only support PSCI. If there is a need for another protocol
 and a good proposal, I'm open for discussions.

 The above is all related to having no SoC code under arch/arm64 (or
 board files, whatever you want to call them).

 I
 agree that whenever it is possible to support a firmware through a
 standard interface, this should be done - no discussion. But right now
 I have two devices that are good representatives of Tegra 4 and
 available in stores, which I would like to see supported in mainline
 to satisfy requests from the community for Tegra development
 platforms, and also initiate the habit to support future
 NVIDIA-branded devices in mainline. Their secure monitor unfortunately
 does not follow PSCI or the SMC convention and needs a custom
 firmware_ops. Are they unworthy of mainline?

 Are they ARMv8? Since we didn't have any such rules on ARMv7 and
 earlier, standard secure interface is nice to have but should not
 prevent upstreaming. I made this clear already that it is ARMv8 only,
 please don't try to generalise it.

Sorry, that was not my intention at all - I just misunderstood what
you meant. Thanks for clarifying it.


 And if, by sheer misfortune, the same thing happened on an ARMv8
 device (despite the EL1/EL3 separation), what would be the outcome?

 If some people get it wrong and they have to work around firmware bugs
 for devices already in the field, we may need to bend the rules (bugs do
 happen, both in software and hardware). But definitely _not_ when people
 don't even bother.

Ok, I guess for ARMv8 there is absolutely no excuse not to follow PSCI
anyways. We'll need to be careful about this one.


 IMHO, more devices in mainline is beneficial to everybody, and
 actually *encourages* SoC vendors/firmware providers to follow
 conventions. Banning devices is what triggers the kind of screw it
 reactions mentioned earlier,

 By following some rules and doing things in a standard way (firmware
 interface in this case), it is more likely that their SoC support
 requires minimal kernel code and it's easier to upstream and maintain.

 and on the contrary once a device is in,
 you tend to make sure the next ones follow the kernel trends because
 you know you will need to support them in mainline as well and it will
 make your life easier.

 Not really. The next device won't follow the kernel trends but just the
 same non-standard way of doing things that were accepted in the first
 place.

I guess

[PATCH] ARM: move firmware_ops to drivers/firmware

2013-11-17 Thread Alexandre Courbot 
The ARM tree includes a firmware_ops interface that is designed to
implement support for simple, TrustZone-based firmwares but could
also cover other use-cases. It has been suggested that this
interface might be useful to other architectures (e.g. arm64) and
that it should be moved out of arch/arm.

This patch takes care of this by performing the following:
* Move the ARM firmware interface into drivers/firmware/ and rename
  it to platform_firmware,
* Update its documentation accordingly,
* Update the only user of the API to date (Exynos secure firmware
  support) to use the new API.

The ARM architecture's Kconfig now needs to include the Kconfig of
drivers/firmware, which will result in an empty menu for most platforms
that don't make use of any firmware. To avoid this, a new invisible
ARCH_SUPPORTS_FIRMWARE configuration variable is also defined for ARM,
that should explicitly be set by platforms that support firmware so that
the firmware menu is included.

Signed-off-by: Alexandre Courbot acour...@nvidia.com
---
 Documentation/arm/firmware.txt   | 88 ---
 Documentation/firmware/platform_firmware.txt | 89 
 arch/arm/Kconfig |  9 +++
 arch/arm/common/Makefile |  2 -
 arch/arm/common/firmware.c   | 18 --
 arch/arm/include/asm/firmware.h  | 66 -
 arch/arm/mach-exynos/Makefile|  2 +-
 arch/arm/mach-exynos/firmware.c  |  7 +--
 arch/arm/mach-exynos/platsmp.c   | 10 ++--
 drivers/firmware/Kconfig |  8 +++
 drivers/firmware/Makefile|  1 +
 drivers/firmware/platform_firmware.c | 17 ++
 include/linux/platform_firmware.h| 69 +
 13 files changed, 203 insertions(+), 183 deletions(-)
 delete mode 100644 Documentation/arm/firmware.txt
 create mode 100644 Documentation/firmware/platform_firmware.txt
 delete mode 100644 arch/arm/common/firmware.c
 delete mode 100644 arch/arm/include/asm/firmware.h
 create mode 100644 drivers/firmware/platform_firmware.c
 create mode 100644 include/linux/platform_firmware.h

diff --git a/Documentation/arm/firmware.txt b/Documentation/arm/firmware.txt
deleted file mode 100644
index c2e468f..000
--- a/Documentation/arm/firmware.txt
+++ /dev/null
@@ -1,88 +0,0 @@
-Interface for registering and calling firmware-specific operations for ARM.
-
-Written by Tomasz Figa t.f...@samsung.com
-
-Some boards are running with secure firmware running in TrustZone secure
-world, which changes the way some things have to be initialized. This makes
-a need to provide an interface for such platforms to specify available firmware
-operations and call them when needed.
-
-Firmware operations can be specified using struct firmware_ops
-
-   struct firmware_ops {
-   /*
-   * Enters CPU idle mode
-   */
-   int (*do_idle)(void);
-   /*
-   * Sets boot address of specified physical CPU
-   */
-   int (*set_cpu_boot_addr)(int cpu, unsigned long boot_addr);
-   /*
-   * Boots specified physical CPU
-   */
-   int (*cpu_boot)(int cpu);
-   /*
-   * Initializes L2 cache
-   */
-   int (*l2x0_init)(void);
-   };
-
-and then registered with register_firmware_ops function
-
-   void register_firmware_ops(const struct firmware_ops *ops)
-
-the ops pointer must be non-NULL.
-
-There is a default, empty set of operations provided, so there is no need to
-set anything if platform does not require firmware operations.
-
-To call a firmware operation, a helper macro is provided
-
-   #define call_firmware_op(op, ...)   \
-   ((firmware_ops-op) ? firmware_ops-op(__VA_ARGS__) : (-ENOSYS))
-
-the macro checks if the operation is provided and calls it or otherwise returns
--ENOSYS to signal that given operation is not available (for example, to allow
-fallback to legacy operation).
-
-Example of registering firmware operations:
-
-   /* board file */
-
-   static int platformX_do_idle(void)
-   {
-   /* tell platformX firmware to enter idle */
-   return 0;
-   }
-
-   static int platformX_cpu_boot(int i)
-   {
-   /* tell platformX firmware to boot CPU i */
-   return 0;
-   }
-
-   static const struct firmware_ops platformX_firmware_ops = {
-   .do_idle= exynos_do_idle,
-   .cpu_boot   = exynos_cpu_boot,
-   /* other operations not available on platformX */
-   };
-
-   /* init_early callback of machine descriptor */
-   static void __init board_init_early(void)
-   {
-   register_firmware_ops(platformX_firmware_ops

Re: GENERIC_GPIO considered deprecated

2013-04-08 Thread Alexandre Courbot 
On Mon, Apr 8, 2013 at 4:38 PM, Stephen Rothwell s...@canb.auug.org.au wrote:
 Hi all,

 On Mon, 8 Apr 2013 21:36:44 +0200 Arnd Bergmann a...@arndb.de wrote:

 On Monday 08 April 2013, Stephen Warren wrote:
  
   Should do the trick, if we can make sure that your tree is merged
   prior to my patches.
  
   I'm not sure but I think, arm-soc tree should be merged into mainline 
   before others...
  
   Can you put it into your tree for 3.10?
  
   I did, so it should be fine.
  
 
  You may want to discuss how to handle this dependency with the arm-soc
  maintainers (CC'd).

 I'm fine with putting the same branch into arm-soc as well as the gpio tree
 and anything else that might need it, that tends to be the least invasive
 way.

 Just a reminder: that had better be the exact same branch and that branch
 had better never be rebased/rewritten ...

Sorry, which branch are we talking about - is it the one I published
for -next initially? If so wouldn't it be simpler to withdraw it and
have Grant integrate the patches in his branch? Since no one depends
on them for now anyway...

I remember rebasing it once some time ago to add Acked-bys, but it
hasn't changed since then.

Alex.
--
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: GENERIC_GPIO considered deprecated

2013-04-04 Thread Alexandre Courbot 
On Wed, Apr 3, 2013 at 5:35 PM, Kukjin Kim kgene@samsung.com wrote:
 could you amend the patches that adds them such as they get changed
 into select ARCH_REQUIRE_GPIOLIB instead? You can grep for select

 I can do it for my tree but the branch already included in arm-soc tree so I 
 think, it should be fixed with another patch. And

 GENERIC_GPIO in arch/arm to find the offending lines. We are removing
 GENERIC_GPIO and this work cannot be merged until you do this since it
 would break ARM builds. Thanks!

 So how about following? If you are OK, let me take into samsung tree.

 88
 From: Kukjin Kim kgene@samsung.com
 Subject: [PATCH] ARM: SAMSUNG: change GENERIC_GPIO to ARCH_REQUIRE_GPIOLIB

 When I applied regarding samsung-time patches, the select GENERIC_GPIO
 has been added wrong, so this patch fixes that.
 And since the GENERIC_GPIO in arch/arm/ will be gone away, this adds
 ARCH_REQUIRE_GPIOLIB for S3C24XX and S5PC100 instead.

 Reported-by: Alexandre Courbot gnu...@gmail.com
 Cc: Romain Naour romain.na...@openwide.fr
 Signed-off-by: Kukjin Kim kgene@samsung.com
 ---
  arch/arm/Kconfig |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
 index 46fcfa8..a239c7e 100644
 --- a/arch/arm/Kconfig
 +++ b/arch/arm/Kconfig
 @@ -770,10 +770,10 @@ config ARCH_SA1100
  config ARCH_S3C24XX
 bool Samsung S3C24XX SoCs
 select ARCH_HAS_CPUFREQ
 +   select ARCH_REQUIRE_GPIOLIB
 select CLKDEV_LOOKUP
 select CLKSRC_MMIO
 select GENERIC_CLOCKEVENTS
 -   select GENERIC_GPIO
 select HAVE_CLK
 select HAVE_S3C2410_I2C if I2C
 select HAVE_S3C2410_WATCHDOG if WATCHDOG
 @@ -828,11 +828,11 @@ config ARCH_S5P64X0

  config ARCH_S5PC100
 bool Samsung S5PC100
 +   select ARCH_REQUIRE_GPIOLIB
 select CLKDEV_LOOKUP
 select CLKSRC_MMIO
 select CPU_V7
 select GENERIC_CLOCKEVENTS
 -   select GENERIC_GPIO
 select HAVE_CLK
 select HAVE_S3C2410_I2C if I2C
 select HAVE_S3C2410_WATCHDOG if WATCHDOG
 --
 1.7.10.4

Should do the trick, if we can make sure that your tree is merged
prior to my patches. Can you put it into your tree for 3.10?

Thanks!
Alex.
--
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: GENERIC_GPIO considered deprecated

2013-03-31 Thread Alexandre Courbot 
Hi Romain,

On Sat, Mar 30, 2013 at 3:07 PM, Romain Naour romain.na...@openwide.fr wrote:
 Hi Alex,

 When I read your mail, I was surprised that you were speaking about GPIOs, my 
 pathes for samsung CPUs are intended for timer sub-system.

 As you can see in this thread, when I send my patches ARM: S3C24XX: Add 
 samsung-time support for s3c24xx and Add samsung-time support for s5pc100. 
 They didn't add select GENERIC_GPIO.
 http://thread.gmane.org/gmane.linux.kernel.samsung-soc/13432/focus=14980
 http://thread.gmane.org/gmane.linux.kernel.samsung-soc/13432/focus=14982

 There is something wrong with the commit, I see that select GENERIC_GPIO 
 was added in my patches by mistake.

 I recommend you to speak directly with samsung's kernel maintainer that I CC 
 in this mail.

Indeed, it seems like these select GENERIC_GPIO have been added
during the merge of your patches, since I can see the line is here in
your patch (but not added by it). Kim, on the current next there are
two of these select GENERIC_GPIO that are added from your branch,
could you amend the patches that adds them such as they get changed
into select ARCH_REQUIRE_GPIOLIB instead? You can grep for select
GENERIC_GPIO in arch/arm to find the offending lines. We are removing
GENERIC_GPIO and this work cannot be merged until you do this since it
would break ARM builds. Thanks!

Alex.
--
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