[PATCH] drm/msm: Implement msm drm fb_mmap callback function

[h...@codeaurora.org]

Re-send the patch to remove the unnecessary initialization and the comment

> This change implements msm drm specific fb_mmap function for fb device
> to properly map the fb address to userspace.
>
> Signed-off-by: Hai Li 
> Signed-off-by: Stephane Viau 
> ---
>  drivers/gpu/drm/msm/msm_fbdev.c | 38
> --
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c
> b/drivers/gpu/drm/msm/msm_fbdev.c
> index a752ab8..2694e90 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -19,6 +19,11 @@
>
>  #include "drm_crtc.h"
>  #include "drm_fb_helper.h"
> +#include "msm_gem.h"
> +
> +extern int msm_gem_mmap_obj(struct drm_gem_object *obj,
> + struct vm_area_struct *vma);
> +static int msm_fbdev_mmap(struct fb_info *info, struct vm_area_struct
> *vma);
>
>  /*
>   * fbdev funcs, to implement legacy fbdev interface on top of drm driver
> @@ -43,6 +48,7 @@ static struct fb_ops msm_fb_ops = {
>   .fb_fillrect = sys_fillrect,
>   .fb_copyarea = sys_copyarea,
>   .fb_imageblit = sys_imageblit,
> + .fb_mmap = msm_fbdev_mmap,
>
>   .fb_check_var = drm_fb_helper_check_var,
>   .fb_set_par = drm_fb_helper_set_par,
> @@ -51,6 +57,31 @@ static struct fb_ops msm_fb_ops = {
>   .fb_setcmap = drm_fb_helper_setcmap,
>  };
>
> +static int msm_fbdev_mmap(struct fb_info *info, struct vm_area_struct
> *vma)
> +{
> + struct drm_fb_helper *helper = (struct drm_fb_helper *)info->par;
> + struct msm_fbdev *fbdev = to_msm_fbdev(helper);
> + struct drm_gem_object *drm_obj = fbdev->bo;
> + struct drm_device *dev = helper->dev;
> + int ret;
> +
> + if (drm_device_is_unplugged(dev))
> + return -ENODEV;
> +
> + mutex_lock(&dev->struct_mutex);
> +
> + ret = drm_gem_mmap_obj(drm_obj, drm_obj->size, vma);
> +
> + mutex_unlock(&dev->struct_mutex);
> +
> + if (ret) {
> + pr_err("%s:drm_gem_mmap_obj fail\n", __func__);
> + return ret;
> + }
> +
> + return msm_gem_mmap_obj(drm_obj, vma);
> +}
> +
>  static int msm_fbdev_create(struct drm_fb_helper *helper,
>   struct drm_fb_helper_surface_size *sizes)
>  {
> @@ -104,8 +135,11 @@ static int msm_fbdev_create(struct drm_fb_helper
> *helper,
>
>   mutex_lock(&dev->struct_mutex);
>
> - /* TODO implement our own fb_mmap so we don't need this: */
> - msm_gem_get_iova_locked(fbdev->bo, 0, &paddr);
> + ret = msm_gem_get_iova_locked(fbdev->bo, 0, &paddr);
> + if (ret) {
> + dev_err(dev->dev, "failed to get buffer obj iova: %d\n", ret);
> + goto fail;
> + }
>
>   fbi = framebuffer_alloc(0, dev->dev);
>   if (!fbi) {
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>
>

[PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask

[h...@codeaurora.org]

Hi Archit,

> mdp5_hw_init and mdp5_set_irqmask configure registers but may not have
> clocks enabled.
>
> Add mdp5_enable/disable calls in these funcs to ensure clocks are
> enabled. We need this until we get proper runtime pm support.
>
> Signed-off-by: Archit Taneja 
> ---
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c | 10 --
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c |  2 ++
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> index b1f73be..9fabfca 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> @@ -24,9 +24,15 @@
>  void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
>   uint32_t old_irqmask)
>  {
> - mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_CLEAR(0),
> + struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
> +
> + mdp5_enable(mdp5_kms);
> +
> + mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_CLEAR(0),
>   irqmask ^ (irqmask & old_irqmask));
> - mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_EN(0), irqmask);
> + mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_EN(0), irqmask);
> +
> + mdp5_disable(mdp5_kms);
>  }

mdp5_set_irqmask() can be invoked in atomic context, clk_prepare() is not
allowed in this function because it may cause process to sleep. We can
enable the clocks in the caller at initialization.
>
>  static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t
> irqstatus)
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index 047cb04..2b760f5 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
>   unsigned long flags;
>
>   pm_runtime_get_sync(dev->dev);
> + mdp5_enable(mdp5_kms);
>
>   /* Magic unknown register writes:
>*
> @@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
>
>   mdp5_ctlm_hw_reset(mdp5_kms->ctlm);
>
> + mdp5_disable(mdp5_kms);
>   pm_runtime_put_sync(dev->dev);
>
>   return 0;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>
>

[PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask

[h...@codeaurora.org]

> 2015-08-26 9:55 GMT-04:00  :
>> Hi Archit,
>>
>>> mdp5_hw_init and mdp5_set_irqmask configure registers but may not have
>>> clocks enabled.
>>>
>>> Add mdp5_enable/disable calls in these funcs to ensure clocks are
>>> enabled. We need this until we get proper runtime pm support.
>>>
>>> Signed-off-by: Archit Taneja 
>>> ---
>>>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c | 10 --
>>>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c |  2 ++
>>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>> index b1f73be..9fabfca 100644
>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>> @@ -24,9 +24,15 @@
>>>  void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
>>>   uint32_t old_irqmask)
>>>  {
>>> - mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_CLEAR(0),
>>> + struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
>>> +
>>> + mdp5_enable(mdp5_kms);
>>> +
>>> + mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_CLEAR(0),
>>>   irqmask ^ (irqmask & old_irqmask));
>>> - mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_EN(0),
>>> irqmask);
>>> + mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_EN(0), irqmask);
>>> +
>>> + mdp5_disable(mdp5_kms);
>>>  }
>>
>> mdp5_set_irqmask() can be invoked in atomic context, clk_prepare() is
>> not
>> allowed in this function because it may cause process to sleep. We can
>> enable the clocks in the caller at initialization.
>
> iirc, it will be called with at least one spinlock held..
>
> We do already move the enable/disable_vblank() paths off to a worker
> so that we can ensure things are enabled before we get into
> update_irq()..  the only other path to update_irq() should be when
> driver code does mdp_irq_register/unregister().. so maybe we should
> just require that the mdp4/mdp5 kms code only calls those when clk's
> are already enabled (which should be mostly true already, I think)
>
> BR,
> -R

Yes, the only case that not been covered is mdp5_irq_postinstall(). We can
enable clocks in this function. Actually, this is what we are doing in
downstream test.
>
>>>
>>>  static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t
>>> irqstatus)
>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> index 047cb04..2b760f5 100644
>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> @@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
>>>   unsigned long flags;
>>>
>>>   pm_runtime_get_sync(dev->dev);
>>> + mdp5_enable(mdp5_kms);
>>>
>>>   /* Magic unknown register writes:
>>>*
>>> @@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
>>>
>>>   mdp5_ctlm_hw_reset(mdp5_kms->ctlm);
>>>
>>> + mdp5_disable(mdp5_kms);
>>>   pm_runtime_put_sync(dev->dev);
>>>
>>>   return 0;
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum,
>>> hosted by The Linux Foundation
>>>
>>>
>>
>>
>

drm/msm/dsi: hs_zero timing

[h...@codeaurora.org]

>
>
> On 08/27/2015 07:12 AM, Werner Johansson wrote:
>> On Aug 26, 2015 11:31 AM, "Rob Clark" > > wrote:
>>  >
>>  > I'm not completely sure.. I did observe that we calculated slightly
>>  > different settings w/ the auo novatek panel on z3, compared to what
>>  > downstream had hard-coded in dts (which presumably came from
>>  > magic-spreadsheet), because (I think) of rounding mode->clock to
>>  > integer KHz.  Although in the case of the z3 panel, it didn't seem to
>>  > matter.  What I am unsure about is whether other panels might be more
>>  > sensitive to different settings.
>>
>> Yes, the code definitely calculates different timing (as can be seen
>> with the calculations for this particular Panasonic panel as well). We
>> need more of the spreadsheet magic in the code it seems. This hs_zero
>> issue seems to be a bug of sorts inside the MSM SoC itself though, not
>> an issue with the panel (as exactly the same issue occurred with all
>> three panels I tried. The "every-eighth value fails" failure mode does
>> not seem to be timing related as I was able to fuzz the timing way
>> outside of the specified ranges and still have perfectly good display,
>> as long as I stayed out of the "every-eighth" value for hs_zero. The
>> panels are typically not crystal controlled so their frequency
>> tolerances are wide.
>>
>> The display mode seems a bit over-specified, can we derive clock from
>> htotal * vtotal * refreshrate instead and get the resolution needed (for
>> DSI that should always result in the correct clock, right)?
>
> There are certain modes (generally for HDMI/DVI) where the refresh rate
> isn't an integer. It can be something like 59.94 Hz, or 60.04Hz. The
> above calculation may not work well with such modes.
>
> For example, a 720p at 59.94Hz mode requires 74.175 Mhz. This value can
> be expressed relatively accurately in KHz if stored beforehand in
> mode->clock. If we try to calculate the pixel clock here ourselves,
> we'll need to round off the vrefresh to 60Hz, which gives us a less
> accurate 74.25 Mhz.
>
> We have platforms where the DSI output is connected to HDMI bridge
> chips. So the issue I mentioned holds true for msm/dsi too.
>
> Thanks,
> Archit
>

For this specific dphy timing issue, the KHz pixel clock precision is good
enough, but I agree to change it to Hz for long term.
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[PATCH 1/2] drm/msm: Initial add eDP support in msm drm driver (V2)

[h...@codeaurora.org]

>> +static int edp_bind(struct device *dev, struct device *master, void
>> *data)
>> +{
>> +struct drm_device *drm = dev_get_drvdata(master);
>> +struct msm_drm_private *priv = drm->dev_private;
>> +struct msm_edp *edp;
>> +
>> +DBG("");
>> +edp = edp_init(to_platform_device(dev));
>
> There's a lot of this casting to platform devices and then using
> pdev->dev to get at the struct device. I don't immediately see a use for
> the platform device, so why not just stick with struct device *
> consistently?
>

There are some places in edp_init() to use struct platform_device *.
Also, this is to make edp code consistent with hdmi.

>> + * It will call msm_edp_aux_ctrl() function to reset the aux channel,
>> + * if the waiting is timeout.
>> + * The caller who triggers the transaction should avoid the
>> + * msm_edp_aux_ctrl() running concurrently in other threads, i.e.
>> + * start transaction only when aux channel is fully enabled.
>> + */
>> +ssize_t edp_aux_transfer(struct drm_dp_aux *drm_aux, struct
>> drm_dp_aux_msg *msg)
>> +{
>> +struct edp_aux *aux = to_edp_aux(drm_aux);
>> +ssize_t ret;
>> +bool native = msg->request & (DP_AUX_NATIVE_WRITE &
>> DP_AUX_NATIVE_READ);
>> +bool read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
>
> These checks are confusing. It seems like they might actually work
> because of how these symbols are defined, but I'd expect something like:
>
>   native = msg->request & (DP_AUX_NATIVE_WRITE | DP_AUX_NATIVE_READ);
>

I think the above solution will not work because it will take
DP_AUX_I2C_READ as "native".

> Or perhaps even clearer:
>
>   switch (msg->request) {
>   case DP_AUX_NATIVE_WRITE:
>   case DP_AUX_NATIVE_READ:
>   native = true;
>   break;
>
>   ...
>   }
>

The switch/case code will work only if we remove other unrelated bits from
input msg->request.

>From my understanding, the idea to define these symbols is to take BIT(7)
as *native* mark and BIT(0) as *read* mark, and the
I2C_WRITE/I2C_READ/NATIVE_WRITE/NATIVE_READ are 4 combinations of these 2
bits. Hence i am still thinking the original way is reflecting the way
they are defined.

>> +/* Ignore address only message */
>> +if ((msg->size == 0) || (msg->buffer == NULL)) {
>> +msg->reply = native ?
>> +DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
>> +return msg->size;
>> +}
>
> How do you support I2C-over-AUX then? How else will the device know
> which I2C slave to address?
>

H/W takes care of the i2c details. S/W only needs to tell H/W if the
transaction is i2c or native and the address. Please see
edp_msg_fifo_tx().







>> +static int cont_splash; /* 1 to enable continuous splash screen */
>> +EXPORT_SYMBOL(cont_splash);
>> +
>> +module_param(cont_splash, int, 0);
>> +MODULE_PARM_DESC(cont_splash, "Enable continuous splash screen on
>> eDP");
>
> Huh? Is this supposed to allow hand-off from firmware to kernel? If so I
> don't think that's going to work without having proper support for it
> across the driver. I don't see support for this in the MDP subdriver, so
> I doubt that it's going to work at all.
>
> Either way, I don't think using a module parameter for this is the right
> solution.
>

I will remove the cont_splash support for now and will have a new change
to fully support hand-off, including all display subdrivers.

>> +struct edp_ctrl {
>> +struct platform_device *pdev;
>> +
>> +void __iomem *base;
>> +
>> +/* regulators */
>> +struct regulator *vdda_vreg;
>> +struct regulator *lvl_reg;
>> +
>> +/* clocks */
>> +struct clk *aux_clk;
>> +struct clk *pixel_clk;
>> +struct clk *ahb_clk;
>> +struct clk *link_clk;
>> +struct clk *mdp_core_clk;
>> +
>> +/* gpios */
>> +int gpio_panel_en;
>> +int gpio_panel_hpd;
>> +int gpio_lvl_en;
>> +int gpio_bkl_en;
>
> These should really be using the new gpiod_*() API. Also, at least
> panel_en and bkl_en seem wrongly placed. They should be handled in the
> panel and backlight drivers, not the eDP driver.
>

I will move bkl_en to pwm_backlight DT and use gpiod_* APIs. We don't have
a panel driver and hope the eDP driver can support all the panels.


>> +static const struct edp_pixel_clk_div clk_divs[2][EDP_PIXEL_CLK_NUM] =
>> {
>> +{ /* Link clock = 162MHz, source clock = 810MHz */
>> +{119000, 31,  211}, /* WSXGA+ 1680x1050 at 60Hz CVT */
>> +{130250, 32,  199}, /* UXGA 1600x1200 at 60Hz CVT */
>> +{148500, 11,  60},  /* FHD 1920x1080 at 60Hz */
>> +{154000, 50,  263}, /* WUXGA 1920x1200 at 60Hz CVT */
>> +{209250, 31,  120}, /* QXGA 2048x1536 at 60Hz CVT */
>> +{268500, 119, 359}, /* WQXGA 2560x1600 at 60Hz CVT */
>> +{138530, 33,  193}, /* AUO B116HAN03.0 Panel */
>> +{141400, 48,  275}, /* AUO B133HTN01.2 Panel */
>> +},
>> +{ /* Link

[PATCH 4/4] drm/msm: Fix default fb var width and height

[h...@codeaurora.org]

Hi Rob,

> On Fri, Mar 6, 2015 at 1:12 PM, Rob Clark  wrote:
>> On Thu, Mar 5, 2015 at 3:20 PM, Hai Li  wrote:
>>> The framebuffer var width and height should reflect the size of
>>> framebuffer memory allocated, which is the entire surface size.
>>>
>>> In case of dual DSI connectors with TILE properties, this change
>>> makes the whole image show on the dual DSI panel, instead of
>>> duplicated images on both sides.
>>>
>>> Signed-off-by: Hai Li 
>>> ---
>>>  drivers/gpu/drm/msm/msm_fbdev.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c
>>> b/drivers/gpu/drm/msm/msm_fbdev.c
>>> index df60f65..d3e8b14 100644
>>> --- a/drivers/gpu/drm/msm/msm_fbdev.c
>>> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
>>> @@ -169,7 +169,8 @@ static int msm_fbdev_create(struct drm_fb_helper
>>> *helper,
>>> }
>>>
>>> drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->depth);
>>> -   drm_fb_helper_fill_var(fbi, helper, sizes->fb_width,
>>> sizes->fb_height);
>>> +   drm_fb_helper_fill_var(fbi, helper,
>>> +   sizes->surface_width, sizes->surface_height);
>>>
>>
>> so, I believe the intention for separation if surface_width/height and
>> fb_width/height, it to allocate a buffer that is big enough (width and
>> height) for all connected displays (so as to not leave some display
>> scanning out too small of a buffer), but size the fbdev buffer small
>> enough that text would be visible on all screens.  Using
>> surface_width/height here instead of fb_width/height would break that.
>>
>> But I think I have a different idea.. we could implement fb helper
>> func initial_config() to just call drm_pick_crtcs() (which we'd have
>> to export), and then for each of the crtcs[n] connected to a connector
>> with TILE property, populate the offsets[n].x/y.  Or possibly we
>> should just make drm_setup_crtcs() clever enough to do that
>> automatically.. that would result in larger fb_size and the two crtc's
>> scanning out their own parts of the buffer.
>
> Oh, I spoke too soon.. looks like Dave already added it in
> drm_get_tile_offsets()..
>

As discussed in IRC, the TILE information has been set and the framebuffer
is allocated with the full size, not the half.

The problem is, we create the full size framebuffer but only tell the user
(fbcon here, but may be real user space application who will use the
default framebuffer.) the half resolution. I think this mismatch will
cause trouble not only in the case of TILE connectors.

For our specific case of TILE connectors, since the right crtc offset.x
has been set to the half width, while the framebuffer width set to plane
is 0~half_width-1 (got from fb var info), the PIPE takes it wrong settings
and will not fetch the image data from framebuffer.
> BR,
> -R
>

[PATCH 3/4] drm/msm: Initial add DSI connector support

[h...@codeaurora.org]

Hi Archit,

Thanks for your comments. Please see my response for some comments below.
Comments without response will be addressed in patch version 2. I will
wait for other comments if any to push patch V2.

>> +static int dsi_gpio_init(struct msm_dsi_host *msm_host)
>> +{
>> +int ret;
>> +
>> +msm_host->disp_en_gpio = devm_gpiod_get(&msm_host->pdev->dev,
>> +"disp-enable");
>> +if (IS_ERR(msm_host->disp_en_gpio)) {
>> +pr_warn("%s: cannot get disp-enable-gpios %ld\n",
>> +__func__, PTR_ERR(msm_host->disp_en_gpio));
>> +msm_host->disp_en_gpio = NULL;
>> +}
>> +if (msm_host->disp_en_gpio) {
>> +ret = gpiod_direction_output(msm_host->disp_en_gpio, 0);
>> +if (ret) {
>> +pr_err("cannot set dir to disp-en-gpios %d\n", ret);
>> +return ret;
>> +}
>> +}
>> +
>> +msm_host->te_gpio = devm_gpiod_get(&msm_host->pdev->dev, "disp-te");
>> +if (IS_ERR(msm_host->te_gpio)) {
>> +pr_warn("%s: cannot get disp-te-gpios %ld\n",
>> +__func__, PTR_ERR(msm_host->te_gpio));
>
> Video mode panels won't have te_gpio, we could probably make this
> pr_debug()
>
>> +msm_host->te_gpio = NULL;
>> +}
>> +
>> +if (msm_host->te_gpio) {
>> +ret = gpiod_direction_input(msm_host->te_gpio);
>> +if (ret) {
>> +pr_err("%s: cannot set dir to disp-te-gpios, %d\n",
>> +__func__, ret);
>> +return ret;
>> +}
>> +}
>
> These gpios currently need to be declared under the dsi DT node. Even if
> these are controlled via the dsi host, the gpios should still come under
> the panel DT node.
>
> We shout get the panel's of_node here and look for these.
>

>> +static void dsi_sw_reset(struct msm_dsi_host *msm_host)
>> +{
>> +dsi_write(msm_host, REG_DSI_CLK_CTRL,
>> +DSI_CLK_CTRL_AHBS_HCLK_ON | DSI_CLK_CTRL_AHBM_SCLK_ON |
>> +DSI_CLK_CTRL_PCLK_ON | DSI_CLK_CTRL_DSICLK_ON |
>> +DSI_CLK_CTRL_BYTECLK_ON | DSI_CLK_CTRL_ESCCLK_ON |
>> +DSI_CLK_CTRL_FORCE_ON_DYN_AHBM_HCLK);
>
> The same 7 bits seem to be set elsewhere, maybe make this a macro
> DSI_ENABLE_CLKS or something similar?
>

>> +int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>> +{
>> +struct msm_dsi_host *msm_host = NULL;
>> +struct platform_device *pdev = msm_dsi->pdev;
>> +int ret = 0;
>> +
>> +msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
>> +if (!msm_host) {
>> +pr_err("%s: FAILED: cannot alloc dsi host\n",
>> +   __func__);
>> +ret = -ENOMEM;
>> +goto fail;
>> +}
>> +
>> +ret = of_property_read_u32(pdev->dev.of_node,
>> +"cell-index", &msm_host->id);
>
> retrieving the instance number of a peripheral via a DT property like
> 'cell-index' has been debated quite a bit in the past. I suppose it's
> not the best thing to do.
>
> However, since the DSI instances in MDSS aren't completely identical(one
> acts a master and other slave in dual dsi mode), maybe we can get away
> with having a property like "qcom,dsi-master;", and that can be further
> used to identify whether this node is DSI_0 or DSI_1
>

2 DSIs are not always master-slave mode. It is possible that a single
panel is connected to any of the hosts, or 2 panels are controlled
independently. If 'cell-index' is not allowed to specify the instance
number, i would prefer to have a simple property like
"qcom,dsi-host-index".

>> +int msm_dsi_host_register(struct mipi_dsi_host *host, bool check_defer)
>> +{
>> +struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>> +struct device_node *node;
>> +int ret;
>> +
>> +/* Register mipi dsi host */
>> +if (!msm_host->registered) {
>> +host->dev = &msm_host->pdev->dev;
>> +host->ops = &dsi_host_ops;
>> +ret = mipi_dsi_host_register(host);
>> +if (ret)
>> +return ret;
>> +
>> +msm_host->registered = true;
>> +
>> +/* If the panel driver has not been probed after host register,
>> + * we should defer the host's probe.
>> + * It makes sure panel is connected when fbcon detects
>> + * connector status and gets the proper display mode to
>> + * create framebuffer.
>> + */
>> +if (check_defer) {
>> +node = of_parse_phandle(msm_host->pdev->dev.of_node,
>> +"qcom,panel", 0);
>> +if (node) {
>> +if (!of_drm_find_panel(node))
>> +return -EPROBE_DEFER;
>> +}
>> +}
>> +}
>
> We might have to defer probe multiple times before anot

[PATCH 3/4] drm/msm: Initial add DSI connector support

[h...@codeaurora.org]

Hi Archit,

> Hi Hai,
>
> On 03/19/2015 02:35 AM, hali at codeaurora.org wrote:
>> Hi Archit,
>>
>> Thanks for your comments. Please see my response for some comments
>> below.
>> Comments without response will be addressed in patch version 2. I will
>> wait for other comments if any to push patch V2.
>>
 +static int dsi_gpio_init(struct msm_dsi_host *msm_host)
 +{
 +  int ret;
 +
 +  msm_host->disp_en_gpio = devm_gpiod_get(&msm_host->pdev->dev,
 +  "disp-enable");
 +  if (IS_ERR(msm_host->disp_en_gpio)) {
 +  pr_warn("%s: cannot get disp-enable-gpios %ld\n",
 +  __func__, PTR_ERR(msm_host->disp_en_gpio));
 +  msm_host->disp_en_gpio = NULL;
 +  }
 +  if (msm_host->disp_en_gpio) {
 +  ret = gpiod_direction_output(msm_host->disp_en_gpio, 0);
 +  if (ret) {
 +  pr_err("cannot set dir to disp-en-gpios %d\n", ret);
 +  return ret;
 +  }
 +  }
 +
 +  msm_host->te_gpio = devm_gpiod_get(&msm_host->pdev->dev, "disp-te");
 +  if (IS_ERR(msm_host->te_gpio)) {
 +  pr_warn("%s: cannot get disp-te-gpios %ld\n",
 +  __func__, PTR_ERR(msm_host->te_gpio));
>>>
>>> Video mode panels won't have te_gpio, we could probably make this
>>> pr_debug()
>>>
 +  msm_host->te_gpio = NULL;
 +  }
 +
 +  if (msm_host->te_gpio) {
 +  ret = gpiod_direction_input(msm_host->te_gpio);
 +  if (ret) {
 +  pr_err("%s: cannot set dir to disp-te-gpios, %d\n",
 +  __func__, ret);
 +  return ret;
 +  }
 +  }
>>>
>>> These gpios currently need to be declared under the dsi DT node. Even
>>> if
>>> these are controlled via the dsi host, the gpios should still come
>>> under
>>> the panel DT node.
>>>
>>> We shout get the panel's of_node here and look for these.
>>>
>>
 +static void dsi_sw_reset(struct msm_dsi_host *msm_host)
 +{
 +  dsi_write(msm_host, REG_DSI_CLK_CTRL,
 +  DSI_CLK_CTRL_AHBS_HCLK_ON | DSI_CLK_CTRL_AHBM_SCLK_ON |
 +  DSI_CLK_CTRL_PCLK_ON | DSI_CLK_CTRL_DSICLK_ON |
 +  DSI_CLK_CTRL_BYTECLK_ON | DSI_CLK_CTRL_ESCCLK_ON |
 +  DSI_CLK_CTRL_FORCE_ON_DYN_AHBM_HCLK);
>>>
>>> The same 7 bits seem to be set elsewhere, maybe make this a macro
>>> DSI_ENABLE_CLKS or something similar?
>>>
>>
 +int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 +{
 +  struct msm_dsi_host *msm_host = NULL;
 +  struct platform_device *pdev = msm_dsi->pdev;
 +  int ret = 0;
 +
 +  msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
 +  if (!msm_host) {
 +  pr_err("%s: FAILED: cannot alloc dsi host\n",
 + __func__);
 +  ret = -ENOMEM;
 +  goto fail;
 +  }
 +
 +  ret = of_property_read_u32(pdev->dev.of_node,
 +  "cell-index", &msm_host->id);
>>>
>>> retrieving the instance number of a peripheral via a DT property like
>>> 'cell-index' has been debated quite a bit in the past. I suppose it's
>>> not the best thing to do.
>>>
>>> However, since the DSI instances in MDSS aren't completely
>>> identical(one
>>> acts a master and other slave in dual dsi mode), maybe we can get away
>>> with having a property like "qcom,dsi-master;", and that can be further
>>> used to identify whether this node is DSI_0 or DSI_1
>>>
>>
>> 2 DSIs are not always master-slave mode. It is possible that a single
>> panel is connected to any of the hosts, or 2 panels are controlled
>> independently. If 'cell-index' is not allowed to specify the instance
>> number, i would prefer to have a simple property like
>> "qcom,dsi-host-index".
>
> Okay, thanks for that clarification.
>
>>
 +int msm_dsi_host_register(struct mipi_dsi_host *host, bool
 check_defer)
 +{
 +  struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
 +  struct device_node *node;
 +  int ret;
 +
 +  /* Register mipi dsi host */
 +  if (!msm_host->registered) {
 +  host->dev = &msm_host->pdev->dev;
 +  host->ops = &dsi_host_ops;
 +  ret = mipi_dsi_host_register(host);
 +  if (ret)
 +  return ret;
 +
 +  msm_host->registered = true;
 +
 +  /* If the panel driver has not been probed after host register,
 +   * we should defer the host's probe.
 +   * It makes sure panel is connected when fbcon detects
 +   * connector status and gets the proper display mode to
 +   * create framebuffer.
 +   */
 +  if (check_defer) {
 +  node = of_parse_phandle(msm_host->pdev->dev.of_node,
 +  "qcom,panel", 0);
 +