Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Archit Taneja]

On 10/11/2017 02:56 PM, Boris Brezillon wrote:

Hi Archit,

On Thu, 7 Sep 2017 15:06:13 +0530
Archit Taneja  wrote:




+
+static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
+{
+   struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+   struct cdns_dsi *dsi = input_to_dsi(input);
+   u32 val;
+
+   val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
+   val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
+DISP_EOT_GEN);


I see some truncation related sparse warnings here and a couple of other places
when building against arm32. Those would be nice to fix.


I had a look and it seems to happen everytime I use GENMASK() (the
truncation is harmless, but sparse complains).
If you don't mind, I'd prefer to keep GENMASK() rather than manually
defining masks, but maybe you have an idea how to fix these warnings
without getting rid of GENMASK().


I don't know either. Let's live with the warnings for now.

Thanks,
Archit






+   writel(val, dsi->regs + MCTL_MAIN_DATA_CTL);
+
+   val = readl(dsi->regs + MCTL_MAIN_EN) & ~IF_EN(input->id);
+   writel(val, dsi->regs + MCTL_MAIN_EN);
+}




--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

Hi Archit,

On Thu, 7 Sep 2017 15:06:13 +0530
Archit Taneja  wrote:

> 
> > +
> > +static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
> > +{
> > +   struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> > +   struct cdns_dsi *dsi = input_to_dsi(input);
> > +   u32 val;
> > +
> > +   val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
> > +   val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
> > +DISP_EOT_GEN);  
> 
> I see some truncation related sparse warnings here and a couple of other 
> places
> when building against arm32. Those would be nice to fix.

I had a look and it seems to happen everytime I use GENMASK() (the
truncation is harmless, but sparse complains).
If you don't mind, I'd prefer to keep GENMASK() rather than manually
defining masks, but maybe you have an idea how to fix these warnings
without getting rid of GENMASK().

> 
> > +   writel(val, dsi->regs + MCTL_MAIN_DATA_CTL);
> > +
> > +   val = readl(dsi->regs + MCTL_MAIN_EN) & ~IF_EN(input->id);
> > +   writel(val, dsi->regs + MCTL_MAIN_EN);
> > +}

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

On Wed, 20 Sep 2017 15:42:50 +0300
Tomi Valkeinen  wrote:

> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 20/09/17 15:32, Boris Brezillon wrote:
> > On Wed, 20 Sep 2017 14:55:02 +0300
> > Tomi Valkeinen  wrote:
> >   
> >> Hi Boris,
> >>
> >>
> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
> >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >>
> >> On 31/08/17 18:55, Boris Brezillon wrote:  
> >>> Add a driver for Cadence DPI -> DSI bridge.
> >>>
> >>> This driver only support a subset of Cadence DSI bridge capabilities.
> >>>
> >>> Here is a non-exhaustive list of missing features:
> >>>  * burst mode
> >>>  * dynamic configuration of the DPHY based on the
> >>>  * support for additional input interfaces (SDI input)
> >>>
> >>> Signed-off-by: Boris Brezillon 
> >>> ---
> >>> Changes in v3:
> >>> - replace magic values by real timing calculation. The DPHY PLL clock
> >>>   is still hardcoded since we don't have a working DPHY block yet, and
> >>>   this is the piece of HW we need to dynamically configure the PLL
> >>>   rate based on the display refresh rate and the resolution.
> >>> - parse DSI devices represented with the OF-graph. This is needed to
> >>>   support DSI devices controlled through an external bus like I2C or
> >>>   SPI.
> >>> - use the DRM panel-bridge infrastructure to simplify the DRM panel
> >>>   logic
> >>>
> >>> Changes in v2:
> >>> - rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
> >>> - return the correct error when devm_clk_get(sysclk) fails
> >>> - add missing depends on OF and select DRM_PANEL in the Kconfig entry
> >>> ---
> >>>  drivers/gpu/drm/bridge/Kconfig|9 +
> >>>  drivers/gpu/drm/bridge/Makefile   |1 +
> >>>  drivers/gpu/drm/bridge/cdns-dsi.c | 1090 
> >>> +
> >>>  3 files changed, 1100 insertions(+)
> >>>  create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c
> >>
> >> We need some power management. At the moment the clocks are kept always
> >> enabled. Those need to be turned off when the IP is not used.  
> > 
> > I can try to move the clk_prepare_enable/disable_unprepare() calls in
> > the bridge->enable/disable() hooks, but I'm not sure the DSI regs
> > content is kept when I disable dsi_p_clk.  
> 
> Yes, context restore has to be handled.
> 
> I'm not sure how different it would be but you could use runtime PM, and
> its resume and suspend callbacks. Then you'd get delayed power-down for
> free, which would prevent suspend, for example, when the bridge is
> disabled, reconfigured and enabled again right away.

As you might already know I'm testing on an emulated system, and I'm
not sure everything is behaving as it will in the final design (once
integrated in a real SoC). I can add support for advanced PM mechanism
but I probably won't be able to test it, so I'd recommend doing the PM
related changes in a follow-up patch (AFAICT, none of the design
choices made in this driver prevent PM optimizations, so it should be
pretty easy to add this afterward).

> 
> >>> +static irqreturn_t cdns_dsi_interrupt(int irq, void *data)
> >>> +{
> >>> + struct cdns_dsi *dsi = data;
> >>> + irqreturn_t ret = IRQ_NONE;
> >>> + u32 flag, ctl;
> >>> +
> >>> + flag = readl(dsi->regs + DIRECT_CMD_STS_FLAG);
> >>> + if (flag) {
> >>> + ctl = readl(dsi->regs + DIRECT_CMD_STS_CTL);
> >>> + ctl &= ~flag;
> >>> + writel(ctl, dsi->regs + DIRECT_CMD_STS_CTL);
> >>
> >> I presume it's the enable/disable bit in STS_CTL that prevents the
> >> interrupt from triggering again, instead of the status flag?  
> > 
> > Yep.
> >   
> >> Just making
> >> sure, because I think on some IPs the status flag has been the one that
> >> triggers the interrupt.
> >>  
> >>> + complete(&dsi->direct_cmd_comp);
> >>> + ret = IRQ_HANDLED;
> >>> + }
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
> >>> +  const struct mipi_dsi_msg *msg)
> >>> +{
> >>> + struct cdns_dsi *dsi = to_cdns_dsi(host);
> >>> + u32 cmd, sts, val, wait = WRITE_COMPLETED, ctl = 0;
> >>> + struct mipi_dsi_packet packet;
> >>> + int ret, i, tx_len, rx_len;
> >>> +
> >>> + ret = mipi_dsi_create_packet(&packet, msg);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + tx_len = msg->tx_buf ? msg->tx_len : 0;
> >>> + rx_len = msg->rx_buf ? msg->rx_len : 0;
> >>> +
> >>> + /* For read operations, the maximum TX len is 2. */
> >>
> >> Hmm, why is that?  
> > 
> > I don't know, that's what is stated in the spec.
> > Excerpt from the CMD_SIZE field description:
> > 
> > "
> > For read operations, any value
> > written which is larger than 2
> > bytes will be ignored and the
> > command payload will be truncated
> > to 2 bytes.
> > "  
> 
> Hmm ok... In other part ("Direct command usage") it says that 

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Tomi Valkeinen]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 20/09/17 15:32, Boris Brezillon wrote:
> On Wed, 20 Sep 2017 14:55:02 +0300
> Tomi Valkeinen  wrote:
> 
>> Hi Boris,
>>
>>
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
>> On 31/08/17 18:55, Boris Brezillon wrote:
>>> Add a driver for Cadence DPI -> DSI bridge.
>>>
>>> This driver only support a subset of Cadence DSI bridge capabilities.
>>>
>>> Here is a non-exhaustive list of missing features:
>>>  * burst mode
>>>  * dynamic configuration of the DPHY based on the
>>>  * support for additional input interfaces (SDI input)
>>>
>>> Signed-off-by: Boris Brezillon 
>>> ---
>>> Changes in v3:
>>> - replace magic values by real timing calculation. The DPHY PLL clock
>>>   is still hardcoded since we don't have a working DPHY block yet, and
>>>   this is the piece of HW we need to dynamically configure the PLL
>>>   rate based on the display refresh rate and the resolution.
>>> - parse DSI devices represented with the OF-graph. This is needed to
>>>   support DSI devices controlled through an external bus like I2C or
>>>   SPI.
>>> - use the DRM panel-bridge infrastructure to simplify the DRM panel
>>>   logic
>>>
>>> Changes in v2:
>>> - rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
>>> - return the correct error when devm_clk_get(sysclk) fails
>>> - add missing depends on OF and select DRM_PANEL in the Kconfig entry
>>> ---
>>>  drivers/gpu/drm/bridge/Kconfig|9 +
>>>  drivers/gpu/drm/bridge/Makefile   |1 +
>>>  drivers/gpu/drm/bridge/cdns-dsi.c | 1090 
>>> +
>>>  3 files changed, 1100 insertions(+)
>>>  create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c  
>>
>> We need some power management. At the moment the clocks are kept always
>> enabled. Those need to be turned off when the IP is not used.
> 
> I can try to move the clk_prepare_enable/disable_unprepare() calls in
> the bridge->enable/disable() hooks, but I'm not sure the DSI regs
> content is kept when I disable dsi_p_clk.

Yes, context restore has to be handled.

I'm not sure how different it would be but you could use runtime PM, and
its resume and suspend callbacks. Then you'd get delayed power-down for
free, which would prevent suspend, for example, when the bridge is
disabled, reconfigured and enabled again right away.

>>> +static irqreturn_t cdns_dsi_interrupt(int irq, void *data)
>>> +{
>>> +   struct cdns_dsi *dsi = data;
>>> +   irqreturn_t ret = IRQ_NONE;
>>> +   u32 flag, ctl;
>>> +
>>> +   flag = readl(dsi->regs + DIRECT_CMD_STS_FLAG);
>>> +   if (flag) {
>>> +   ctl = readl(dsi->regs + DIRECT_CMD_STS_CTL);
>>> +   ctl &= ~flag;
>>> +   writel(ctl, dsi->regs + DIRECT_CMD_STS_CTL);  
>>
>> I presume it's the enable/disable bit in STS_CTL that prevents the
>> interrupt from triggering again, instead of the status flag?
> 
> Yep.
> 
>> Just making
>> sure, because I think on some IPs the status flag has been the one that
>> triggers the interrupt.
>>
>>> +   complete(&dsi->direct_cmd_comp);
>>> +   ret = IRQ_HANDLED;
>>> +   }
>>> +
>>> +   return ret;
>>> +}
>>> +
>>> +static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
>>> +const struct mipi_dsi_msg *msg)
>>> +{
>>> +   struct cdns_dsi *dsi = to_cdns_dsi(host);
>>> +   u32 cmd, sts, val, wait = WRITE_COMPLETED, ctl = 0;
>>> +   struct mipi_dsi_packet packet;
>>> +   int ret, i, tx_len, rx_len;
>>> +
>>> +   ret = mipi_dsi_create_packet(&packet, msg);
>>> +   if (ret)
>>> +   return ret;
>>> +
>>> +   tx_len = msg->tx_buf ? msg->tx_len : 0;
>>> +   rx_len = msg->rx_buf ? msg->rx_len : 0;
>>> +
>>> +   /* For read operations, the maximum TX len is 2. */  
>>
>> Hmm, why is that?
> 
> I don't know, that's what is stated in the spec.
> Excerpt from the CMD_SIZE field description:
> 
> "
> For read operations, any value
> written which is larger than 2
> bytes will be ignored and the
> command payload will be truncated
> to 2 bytes.
> "

Hmm ok... In other part ("Direct command usage") it says that for short
packets the max is 2, but for long packets max is the fifo size.

>>> +   if (rx_len && tx_len > 2)
>>> +   return -ENOTSUPP;
>>> +
>>> +   /* TX len is limited by the CMD FIFO depth. */
>>> +   if (tx_len > dsi->direct_cmd_fifo_depth)
>>> +   return -ENOTSUPP;
>>> +
>>> +   /* RX len is limited by the RX FIFO depth. */
>>> +   if (rx_len > dsi->rx_fifo_depth)
>>> +   return -ENOTSUPP;
>>> +
>>> +   cmd = CMD_SIZE(tx_len) | CMD_VCHAN_ID(msg->channel) |
>>> + CMD_DATATYPE(msg->type);
>>> +
>>> +   if (msg->flags & MIPI_DSI_MSG_USE_LPM)
>>> +   cmd |= CMD_LP_EN;
>>> +
>>> +   if (mipi_dsi_packet_format_is_long(msg->type))
>>> +   cmd |= CMD_LONG;
>>> +
>>> +   if (rx_le

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

On Wed, 20 Sep 2017 14:55:02 +0300
Tomi Valkeinen  wrote:

> Hi Boris,
> 
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 31/08/17 18:55, Boris Brezillon wrote:
> > Add a driver for Cadence DPI -> DSI bridge.
> > 
> > This driver only support a subset of Cadence DSI bridge capabilities.
> > 
> > Here is a non-exhaustive list of missing features:
> >  * burst mode
> >  * dynamic configuration of the DPHY based on the
> >  * support for additional input interfaces (SDI input)
> > 
> > Signed-off-by: Boris Brezillon 
> > ---
> > Changes in v3:
> > - replace magic values by real timing calculation. The DPHY PLL clock
> >   is still hardcoded since we don't have a working DPHY block yet, and
> >   this is the piece of HW we need to dynamically configure the PLL
> >   rate based on the display refresh rate and the resolution.
> > - parse DSI devices represented with the OF-graph. This is needed to
> >   support DSI devices controlled through an external bus like I2C or
> >   SPI.
> > - use the DRM panel-bridge infrastructure to simplify the DRM panel
> >   logic
> > 
> > Changes in v2:
> > - rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
> > - return the correct error when devm_clk_get(sysclk) fails
> > - add missing depends on OF and select DRM_PANEL in the Kconfig entry
> > ---
> >  drivers/gpu/drm/bridge/Kconfig|9 +
> >  drivers/gpu/drm/bridge/Makefile   |1 +
> >  drivers/gpu/drm/bridge/cdns-dsi.c | 1090 
> > +
> >  3 files changed, 1100 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c  
> 
> We need some power management. At the moment the clocks are kept always
> enabled. Those need to be turned off when the IP is not used.

I can try to move the clk_prepare_enable/disable_unprepare() calls in
the bridge->enable/disable() hooks, but I'm not sure the DSI regs
content is kept when I disable dsi_p_clk.

> 
> > +static irqreturn_t cdns_dsi_interrupt(int irq, void *data)
> > +{
> > +   struct cdns_dsi *dsi = data;
> > +   irqreturn_t ret = IRQ_NONE;
> > +   u32 flag, ctl;
> > +
> > +   flag = readl(dsi->regs + DIRECT_CMD_STS_FLAG);
> > +   if (flag) {
> > +   ctl = readl(dsi->regs + DIRECT_CMD_STS_CTL);
> > +   ctl &= ~flag;
> > +   writel(ctl, dsi->regs + DIRECT_CMD_STS_CTL);  
> 
> I presume it's the enable/disable bit in STS_CTL that prevents the
> interrupt from triggering again, instead of the status flag?

Yep.

> Just making
> sure, because I think on some IPs the status flag has been the one that
> triggers the interrupt.
> 
> > +   complete(&dsi->direct_cmd_comp);
> > +   ret = IRQ_HANDLED;
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
> > +const struct mipi_dsi_msg *msg)
> > +{
> > +   struct cdns_dsi *dsi = to_cdns_dsi(host);
> > +   u32 cmd, sts, val, wait = WRITE_COMPLETED, ctl = 0;
> > +   struct mipi_dsi_packet packet;
> > +   int ret, i, tx_len, rx_len;
> > +
> > +   ret = mipi_dsi_create_packet(&packet, msg);
> > +   if (ret)
> > +   return ret;
> > +
> > +   tx_len = msg->tx_buf ? msg->tx_len : 0;
> > +   rx_len = msg->rx_buf ? msg->rx_len : 0;
> > +
> > +   /* For read operations, the maximum TX len is 2. */  
> 
> Hmm, why is that?

I don't know, that's what is stated in the spec.
Excerpt from the CMD_SIZE field description:

"
For read operations, any value
written which is larger than 2
bytes will be ignored and the
command payload will be truncated
to 2 bytes.
"

> 
> > +   if (rx_len && tx_len > 2)
> > +   return -ENOTSUPP;
> > +
> > +   /* TX len is limited by the CMD FIFO depth. */
> > +   if (tx_len > dsi->direct_cmd_fifo_depth)
> > +   return -ENOTSUPP;
> > +
> > +   /* RX len is limited by the RX FIFO depth. */
> > +   if (rx_len > dsi->rx_fifo_depth)
> > +   return -ENOTSUPP;
> > +
> > +   cmd = CMD_SIZE(tx_len) | CMD_VCHAN_ID(msg->channel) |
> > + CMD_DATATYPE(msg->type);
> > +
> > +   if (msg->flags & MIPI_DSI_MSG_USE_LPM)
> > +   cmd |= CMD_LP_EN;
> > +
> > +   if (mipi_dsi_packet_format_is_long(msg->type))
> > +   cmd |= CMD_LONG;
> > +
> > +   if (rx_len) {
> > +   cmd |= READ_CMD;
> > +   wait = READ_COMPLETED_WITH_ERR | READ_COMPLETED;
> > +   ctl = READ_EN | BTA_EN;
> > +   } else if (msg->flags & MIPI_DSI_MSG_REQ_ACK) {
> > +   cmd |= BTA_REQ;
> > +   wait = ACK_WITH_ERR_RCVD | ACK_RCVD;
> > +   ctl = BTA_EN;
> > +   }  
> 
> It's been a while since I worked with DSI, but... Shouldn't there be a
> check somewhere that the packet(s) can fit into the blanking intervals?

Hm, I'm not sure. DSI commands are usually sent when the encoder/bridge
is not transmitting video, so in this case we don't have any constraint
on the packet length. Also, it seems othe

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

On Tue, 19 Sep 2017 17:25:29 +0300
Tomi Valkeinen  wrote:

> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 19/09/17 16:48, Boris Brezillon wrote:
> > On Tue, 19 Sep 2017 16:38:31 +0300
> > Tomi Valkeinen  wrote:
> >   
> >> 
> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
> >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >>
> >> On 19/09/17 16:25, Boris Brezillon wrote:  
> >>> On Tue, 19 Sep 2017 15:59:20 +0300
> >>> Tomi Valkeinen  wrote:
> >>> 
>  Hi Boris,
> 
> 
>  Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
>  Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
>  On 31/08/17 18:55, Boris Brezillon wrote:
> > Add a driver for Cadence DPI -> DSI bridge.
> >
> > This driver only support a subset of Cadence DSI bridge capabilities.
> >
> > Here is a non-exhaustive list of missing features:
> >  * burst mode
> >  * dynamic configuration of the DPHY based on the
> >  * support for additional input interfaces (SDI input)
> >
> > Signed-off-by: Boris Brezillon  
> >  
> 
>  
> 
> > +   dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
> > +   if (IS_ERR(dsi->pclk))
> > +   return PTR_ERR(dsi->pclk);  
> 
>  What's the purpose of pclk? Isn't that normally dealt with the normal
>  modesetting, enabled with the video stream? How could it even be enabled
>  here, without anyone setting the rate?
> >>>
> >>> It's the peripheral clock, not the pixel clock, and AFAIU it has to be
> >>> enabled before accessing DSI registers.
> >>
> >> Is that the dsi_p_clk? I can't find "peripheral clock" in the specs.  
> > 
> > Yep, it is dsi_p_clk (the APB clock).
> >   
> >>
> >> I think calling it "pclk" in a display driver is very confusing, as
> >> pclk, at least for me, always means pixel clock =).  
> > 
> > I can rename it if you prefer. What name would you like to see?
> > abp_clk? periph_clk? Something else?  
> 
> Is there something wrong with dsi_p_clk? If possible, it's nice if the
> terms in SW match to the HW docs. In the minimum, the DT doc should give
> the mapping from SW to HW terms, at the moment it just says "pclk".

I'll switch to dsi_p_clk and dsi_sys_clk.

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Tomi Valkeinen]

Hi Boris,


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 31/08/17 18:55, Boris Brezillon wrote:
> Add a driver for Cadence DPI -> DSI bridge.
> 
> This driver only support a subset of Cadence DSI bridge capabilities.
> 
> Here is a non-exhaustive list of missing features:
>  * burst mode
>  * dynamic configuration of the DPHY based on the
>  * support for additional input interfaces (SDI input)
> 
> Signed-off-by: Boris Brezillon 
> ---
> Changes in v3:
> - replace magic values by real timing calculation. The DPHY PLL clock
>   is still hardcoded since we don't have a working DPHY block yet, and
>   this is the piece of HW we need to dynamically configure the PLL
>   rate based on the display refresh rate and the resolution.
> - parse DSI devices represented with the OF-graph. This is needed to
>   support DSI devices controlled through an external bus like I2C or
>   SPI.
> - use the DRM panel-bridge infrastructure to simplify the DRM panel
>   logic
> 
> Changes in v2:
> - rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
> - return the correct error when devm_clk_get(sysclk) fails
> - add missing depends on OF and select DRM_PANEL in the Kconfig entry
> ---
>  drivers/gpu/drm/bridge/Kconfig|9 +
>  drivers/gpu/drm/bridge/Makefile   |1 +
>  drivers/gpu/drm/bridge/cdns-dsi.c | 1090 
> +
>  3 files changed, 1100 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c

We need some power management. At the moment the clocks are kept always
enabled. Those need to be turned off when the IP is not used.

> +static irqreturn_t cdns_dsi_interrupt(int irq, void *data)
> +{
> + struct cdns_dsi *dsi = data;
> + irqreturn_t ret = IRQ_NONE;
> + u32 flag, ctl;
> +
> + flag = readl(dsi->regs + DIRECT_CMD_STS_FLAG);
> + if (flag) {
> + ctl = readl(dsi->regs + DIRECT_CMD_STS_CTL);
> + ctl &= ~flag;
> + writel(ctl, dsi->regs + DIRECT_CMD_STS_CTL);

I presume it's the enable/disable bit in STS_CTL that prevents the
interrupt from triggering again, instead of the status flag? Just making
sure, because I think on some IPs the status flag has been the one that
triggers the interrupt.

> + complete(&dsi->direct_cmd_comp);
> + ret = IRQ_HANDLED;
> + }
> +
> + return ret;
> +}
> +
> +static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
> +  const struct mipi_dsi_msg *msg)
> +{
> + struct cdns_dsi *dsi = to_cdns_dsi(host);
> + u32 cmd, sts, val, wait = WRITE_COMPLETED, ctl = 0;
> + struct mipi_dsi_packet packet;
> + int ret, i, tx_len, rx_len;
> +
> + ret = mipi_dsi_create_packet(&packet, msg);
> + if (ret)
> + return ret;
> +
> + tx_len = msg->tx_buf ? msg->tx_len : 0;
> + rx_len = msg->rx_buf ? msg->rx_len : 0;
> +
> + /* For read operations, the maximum TX len is 2. */

Hmm, why is that?

> + if (rx_len && tx_len > 2)
> + return -ENOTSUPP;
> +
> + /* TX len is limited by the CMD FIFO depth. */
> + if (tx_len > dsi->direct_cmd_fifo_depth)
> + return -ENOTSUPP;
> +
> + /* RX len is limited by the RX FIFO depth. */
> + if (rx_len > dsi->rx_fifo_depth)
> + return -ENOTSUPP;
> +
> + cmd = CMD_SIZE(tx_len) | CMD_VCHAN_ID(msg->channel) |
> +   CMD_DATATYPE(msg->type);
> +
> + if (msg->flags & MIPI_DSI_MSG_USE_LPM)
> + cmd |= CMD_LP_EN;
> +
> + if (mipi_dsi_packet_format_is_long(msg->type))
> + cmd |= CMD_LONG;
> +
> + if (rx_len) {
> + cmd |= READ_CMD;
> + wait = READ_COMPLETED_WITH_ERR | READ_COMPLETED;
> + ctl = READ_EN | BTA_EN;
> + } else if (msg->flags & MIPI_DSI_MSG_REQ_ACK) {
> + cmd |= BTA_REQ;
> + wait = ACK_WITH_ERR_RCVD | ACK_RCVD;
> + ctl = BTA_EN;
> + }

It's been a while since I worked with DSI, but... Shouldn't there be a
check somewhere that the packet(s) can fit into the blanking intervals?

 Tomi

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Tomi Valkeinen]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 19/09/17 16:48, Boris Brezillon wrote:
> On Tue, 19 Sep 2017 16:38:31 +0300
> Tomi Valkeinen  wrote:
> 
>> 
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
>> On 19/09/17 16:25, Boris Brezillon wrote:
>>> On Tue, 19 Sep 2017 15:59:20 +0300
>>> Tomi Valkeinen  wrote:
>>>   
 Hi Boris,


 Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
 Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

 On 31/08/17 18:55, Boris Brezillon wrote:  
> Add a driver for Cadence DPI -> DSI bridge.
>
> This driver only support a subset of Cadence DSI bridge capabilities.
>
> Here is a non-exhaustive list of missing features:
>  * burst mode
>  * dynamic configuration of the DPHY based on the
>  * support for additional input interfaces (SDI input)
>
> Signed-off-by: Boris Brezillon 

 
  
> + dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
> + if (IS_ERR(dsi->pclk))
> + return PTR_ERR(dsi->pclk);

 What's the purpose of pclk? Isn't that normally dealt with the normal
 modesetting, enabled with the video stream? How could it even be enabled
 here, without anyone setting the rate?  
>>>
>>> It's the peripheral clock, not the pixel clock, and AFAIU it has to be
>>> enabled before accessing DSI registers.  
>>
>> Is that the dsi_p_clk? I can't find "peripheral clock" in the specs.
> 
> Yep, it is dsi_p_clk (the APB clock).
> 
>>
>> I think calling it "pclk" in a display driver is very confusing, as
>> pclk, at least for me, always means pixel clock =).
> 
> I can rename it if you prefer. What name would you like to see?
> abp_clk? periph_clk? Something else?

Is there something wrong with dsi_p_clk? If possible, it's nice if the
terms in SW match to the HW docs. In the minimum, the DT doc should give
the mapping from SW to HW terms, at the moment it just says "pclk".

 Tomi

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

On Tue, 19 Sep 2017 16:38:31 +0300
Tomi Valkeinen  wrote:

> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 19/09/17 16:25, Boris Brezillon wrote:
> > On Tue, 19 Sep 2017 15:59:20 +0300
> > Tomi Valkeinen  wrote:
> >   
> >> Hi Boris,
> >>
> >>
> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
> >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >>
> >> On 31/08/17 18:55, Boris Brezillon wrote:  
> >>> Add a driver for Cadence DPI -> DSI bridge.
> >>>
> >>> This driver only support a subset of Cadence DSI bridge capabilities.
> >>>
> >>> Here is a non-exhaustive list of missing features:
> >>>  * burst mode
> >>>  * dynamic configuration of the DPHY based on the
> >>>  * support for additional input interfaces (SDI input)
> >>>
> >>> Signed-off-by: Boris Brezillon 
> >>
> >> 
> >>  
> >>> + dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
> >>> + if (IS_ERR(dsi->pclk))
> >>> + return PTR_ERR(dsi->pclk);
> >>
> >> What's the purpose of pclk? Isn't that normally dealt with the normal
> >> modesetting, enabled with the video stream? How could it even be enabled
> >> here, without anyone setting the rate?  
> > 
> > It's the peripheral clock, not the pixel clock, and AFAIU it has to be
> > enabled before accessing DSI registers.  
> 
> Is that the dsi_p_clk? I can't find "peripheral clock" in the specs.

Yep, it is dsi_p_clk (the APB clock).

> 
> I think calling it "pclk" in a display driver is very confusing, as
> pclk, at least for me, always means pixel clock =).

I can rename it if you prefer. What name would you like to see?
abp_clk? periph_clk? Something else?

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Tomi Valkeinen]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 19/09/17 16:25, Boris Brezillon wrote:
> On Tue, 19 Sep 2017 15:59:20 +0300
> Tomi Valkeinen  wrote:
> 
>> Hi Boris,
>>
>>
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
>> On 31/08/17 18:55, Boris Brezillon wrote:
>>> Add a driver for Cadence DPI -> DSI bridge.
>>>
>>> This driver only support a subset of Cadence DSI bridge capabilities.
>>>
>>> Here is a non-exhaustive list of missing features:
>>>  * burst mode
>>>  * dynamic configuration of the DPHY based on the
>>>  * support for additional input interfaces (SDI input)
>>>
>>> Signed-off-by: Boris Brezillon   
>>
>> 
>>
>>> +   dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
>>> +   if (IS_ERR(dsi->pclk))
>>> +   return PTR_ERR(dsi->pclk);  
>>
>> What's the purpose of pclk? Isn't that normally dealt with the normal
>> modesetting, enabled with the video stream? How could it even be enabled
>> here, without anyone setting the rate?
> 
> It's the peripheral clock, not the pixel clock, and AFAIU it has to be
> enabled before accessing DSI registers.

Is that the dsi_p_clk? I can't find "peripheral clock" in the specs.

I think calling it "pclk" in a display driver is very confusing, as
pclk, at least for me, always means pixel clock =).

 Tomi

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

On Tue, 19 Sep 2017 15:59:20 +0300
Tomi Valkeinen  wrote:

> Hi Boris,
> 
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 31/08/17 18:55, Boris Brezillon wrote:
> > Add a driver for Cadence DPI -> DSI bridge.
> > 
> > This driver only support a subset of Cadence DSI bridge capabilities.
> > 
> > Here is a non-exhaustive list of missing features:
> >  * burst mode
> >  * dynamic configuration of the DPHY based on the
> >  * support for additional input interfaces (SDI input)
> > 
> > Signed-off-by: Boris Brezillon   
> 
> 
> 
> > +   dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
> > +   if (IS_ERR(dsi->pclk))
> > +   return PTR_ERR(dsi->pclk);  
> 
> What's the purpose of pclk? Isn't that normally dealt with the normal
> modesetting, enabled with the video stream? How could it even be enabled
> here, without anyone setting the rate?

It's the peripheral clock, not the pixel clock, and AFAIU it has to be
enabled before accessing DSI registers.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Tomi Valkeinen]

Hi Boris,


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 31/08/17 18:55, Boris Brezillon wrote:
> Add a driver for Cadence DPI -> DSI bridge.
> 
> This driver only support a subset of Cadence DSI bridge capabilities.
> 
> Here is a non-exhaustive list of missing features:
>  * burst mode
>  * dynamic configuration of the DPHY based on the
>  * support for additional input interfaces (SDI input)
> 
> Signed-off-by: Boris Brezillon 



> + dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
> + if (IS_ERR(dsi->pclk))
> + return PTR_ERR(dsi->pclk);

What's the purpose of pclk? Isn't that normally dealt with the normal
modesetting, enabled with the video stream? How could it even be enabled
here, without anyone setting the rate?

 Tomi

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

On Thu, 7 Sep 2017 15:06:13 +0530
Archit Taneja  wrote:

> Hi,
> 
> On 08/31/2017 09:25 PM, Boris Brezillon wrote:
> > Add a driver for Cadence DPI -> DSI bridge.
> > 
> > This driver only support a subset of Cadence DSI bridge capabilities.
> > 
> > Here is a non-exhaustive list of missing features:
> >   * burst mode
> >   * dynamic configuration of the DPHY based on the
> >   * support for additional input interfaces (SDI input)
> > 
> > Signed-off-by: Boris Brezillon 
> > ---
> > Changes in v3:
> > - replace magic values by real timing calculation. The DPHY PLL clock
> >is still hardcoded since we don't have a working DPHY block yet, and
> >this is the piece of HW we need to dynamically configure the PLL
> >rate based on the display refresh rate and the resolution.
> > - parse DSI devices represented with the OF-graph. This is needed to
> >support DSI devices controlled through an external bus like I2C or
> >SPI.
> > - use the DRM panel-bridge infrastructure to simplify the DRM panel
> >logic
> > 
> > Changes in v2:
> > - rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
> > - return the correct error when devm_clk_get(sysclk) fails
> > - add missing depends on OF and select DRM_PANEL in the Kconfig entry
> > ---
> >   drivers/gpu/drm/bridge/Kconfig|9 +
> >   drivers/gpu/drm/bridge/Makefile   |1 +
> >   drivers/gpu/drm/bridge/cdns-dsi.c | 1090 
> > +
> >   3 files changed, 1100 insertions(+)
> >   create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c
> > 
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index adf9ae0e0b7c..88c324b12e16 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -25,6 +25,15 @@ config DRM_ANALOGIX_ANX78XX
> >   the HDMI output of an application processor to MyDP
> >   or DisplayPort.
> >   
> > +config DRM_CDNS_DSI
> > +   tristate "Cadence DPI/DSI bridge"
> > +   select DRM_KMS_HELPER
> > +   select DRM_MIPI_DSI
> > +   select DRM_PANEL
> > +   depends on OF
> > +   help
> > + Support Cadence DPI to DSI bridge.  
> 
> Maybe we can briefly mention here that it's a internal bridge/IP, and not an 
> external chip?

Sure.

> 
> > +
> >   config DRM_DUMB_VGA_DAC
> > tristate "Dumb VGA DAC Bridge support"
> > depends on OF
> > diff --git a/drivers/gpu/drm/bridge/Makefile 
> > b/drivers/gpu/drm/bridge/Makefile
> > index defcf1e7ca1c..77b65e8ecf59 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -1,4 +1,5 @@
> >   obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> > +obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> >   obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> >   obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> >   obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += 
> > megachips-stdp-ge-b850v3-fw.o
> > diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c 
> > b/drivers/gpu/drm/bridge/cdns-dsi.c  
> 
> 
> 
> > +
> > +static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
> > +{
> > +   struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> > +   struct cdns_dsi *dsi = input_to_dsi(input);
> > +   u32 val;
> > +
> > +   val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
> > +   val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
> > +DISP_EOT_GEN);  
> 
> I see some truncation related sparse warnings here and a couple of other 
> places
> when building against arm32. Those would be nice to fix.

I'll have a look.


> > +static int cdns_dsi_attach(struct mipi_dsi_host *host,
> > +  struct mipi_dsi_device *dev)
> > +{
> > +   struct cdns_dsi *dsi = to_cdns_dsi(host);
> > +   struct cdns_dsi_output *output = &dsi->output;
> > +   struct cdns_dsi_input *input = &dsi->input;
> > +   struct drm_bridge *bridge;
> > +   struct drm_panel *panel;
> > +   struct device_node *np;
> > +   int ret;
> > +
> > +   /*
> > +* We currently do not support connecting several DSI devices to the
> > +* same host. In order to support that we'd need the DRM bridge
> > +* framework to allow dynamic reconfiguration of the bridge chain.
> > +*/
> > +   if (output->dev)
> > +   return -EBUSY;
> > +
> > +   /* We do not support burst mode yet. */
> > +   if (dev->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
> > +   return -ENOTSUPP;
> > +
> > +   /*
> > +* The host <-> device link might be described using an OF-graph
> > +* representation, in this case we extract the device of_node from
> > +* this representation, otherwise we use dsidev->dev.of_node which
> > +* should have been filled by the core.
> > +*/
> > +   np = of_graph_get_remote_node(dsi->base.dev->of_node, DSI_OUTPUT_PORT,
> > + dev->channel);
> > +   if (!np)
> > +   np = of_node_get(dev->dev.of_node); > +
> > +   panel = of_drm_find_panel(np);
> > +   if (

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Archit Taneja]

Hi,

On 08/31/2017 09:25 PM, Boris Brezillon wrote:

Add a driver for Cadence DPI -> DSI bridge.

This driver only support a subset of Cadence DSI bridge capabilities.

Here is a non-exhaustive list of missing features:
  * burst mode
  * dynamic configuration of the DPHY based on the
  * support for additional input interfaces (SDI input)

Signed-off-by: Boris Brezillon 
---
Changes in v3:
- replace magic values by real timing calculation. The DPHY PLL clock
   is still hardcoded since we don't have a working DPHY block yet, and
   this is the piece of HW we need to dynamically configure the PLL
   rate based on the display refresh rate and the resolution.
- parse DSI devices represented with the OF-graph. This is needed to
   support DSI devices controlled through an external bus like I2C or
   SPI.
- use the DRM panel-bridge infrastructure to simplify the DRM panel
   logic

Changes in v2:
- rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
- return the correct error when devm_clk_get(sysclk) fails
- add missing depends on OF and select DRM_PANEL in the Kconfig entry
---
  drivers/gpu/drm/bridge/Kconfig|9 +
  drivers/gpu/drm/bridge/Makefile   |1 +
  drivers/gpu/drm/bridge/cdns-dsi.c | 1090 +
  3 files changed, 1100 insertions(+)
  create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index adf9ae0e0b7c..88c324b12e16 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -25,6 +25,15 @@ config DRM_ANALOGIX_ANX78XX
  the HDMI output of an application processor to MyDP
  or DisplayPort.
  
+config DRM_CDNS_DSI

+   tristate "Cadence DPI/DSI bridge"
+   select DRM_KMS_HELPER
+   select DRM_MIPI_DSI
+   select DRM_PANEL
+   depends on OF
+   help
+ Support Cadence DPI to DSI bridge.


Maybe we can briefly mention here that it's a internal bridge/IP, and not an 
external chip?


+
  config DRM_DUMB_VGA_DAC
tristate "Dumb VGA DAC Bridge support"
depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index defcf1e7ca1c..77b65e8ecf59 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,4 +1,5 @@
  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
+obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
  obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += 
megachips-stdp-ge-b850v3-fw.o
diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c 
b/drivers/gpu/drm/bridge/cdns-dsi.c





+
+static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
+{
+   struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+   struct cdns_dsi *dsi = input_to_dsi(input);
+   u32 val;
+
+   val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
+   val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
+DISP_EOT_GEN);


I see some truncation related sparse warnings here and a couple of other places
when building against arm32. Those would be nice to fix.


+   writel(val, dsi->regs + MCTL_MAIN_DATA_CTL);
+
+   val = readl(dsi->regs + MCTL_MAIN_EN) & ~IF_EN(input->id);
+   writel(val, dsi->regs + MCTL_MAIN_EN);
+}
+
+#define DSI_HBP_FRAME_OVERHEAD 12
+#define DSI_HSA_FRAME_OVERHEAD 14
+#define DSI_HFP_FRAME_OVERHEAD 6
+#define DSI_HSS_VSS_VSE_FRAME_OVERHEAD 4
+#define DSI_BLANKING_FRAME_OVERHEAD6
+#define DSI_NULL_FRAME_OVERHEAD6
+#define DSI_EOT_PKT_SIZE   4
+
+#define REG_WAKEUP_TIME_NS 800
+#define DPHY_PLL_RATE_HZ   10800
+
+static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
+{
+   struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+   struct cdns_dsi *dsi = input_to_dsi(input);
+   struct cdns_dsi_output *output = &dsi->output;
+   u32 hsize0, hsa, hline, tmp, reg_wakeup, div;
+   unsigned long dphy_pll_period, tx_byte_period;
+   struct drm_display_mode *mode;
+   int bpp, nlanes;
+
+   mode = &bridge->encoder->crtc->state->adjusted_mode;
+   bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
+   nlanes = output->dev->lanes;
+
+   if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
+   tmp = mode->crtc_htotal - mode->crtc_hsync_end;
+   else
+   tmp = mode->crtc_htotal - mode->crtc_hsync_start;
+   tmp = (tmp * bpp) / 8;
+   tmp = tmp < DSI_HBP_FRAME_OVERHEAD ? 0 : tmp - DSI_HBP_FRAME_OVERHEAD;
+   hsize0 = HBP_LEN(tmp);
+
+   tmp = mode->crtc_hsync_start - mode->crtc_hdisplay;
+   tmp = (tmp * bpp) / 8;
+   tmp = tmp < DSI_HFP_FRAME_OVERHEAD ? 0 : tmp - DSI_HFP_FRAME_OVERHEAD;
+   hsize0 |= HFP_LEN(tmp);
+
+   if (output->dev->mode_flags & MIPI

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Eric Anholt]

Boris Brezillon  writes:

> Hi Eric,
>
> On Thu, 31 Aug 2017 10:03:23 -0700
> Eric Anholt  wrote:
>
>> Boris Brezillon  writes:
>> > +VID_DATATYPE(MIPI_DSI_PACKED_PIXEL_STREAM_16);
>> > +  break;
>> > +
>> > +  default:
>> > +  dev_err(dsi->base.dev, "Unsupported DSI format\n");
>> > +  return;
>> > +  }  
>> 
>> > +static irqreturn_t cdns_dsi_interrupt(int irq, void *data)
>> > +{
>> > +  struct cdns_dsi *dsi = data;
>> > +  irqreturn_t ret = IRQ_NONE;
>> > +  u32 flag, ctl;
>> > +
>> > +  flag = readl(dsi->regs + DIRECT_CMD_STS_FLAG);
>> > +  if (flag) {
>> > +  ctl = readl(dsi->regs + DIRECT_CMD_STS_CTL);
>> > +  ctl &= ~flag;
>> > +  writel(ctl, dsi->regs + DIRECT_CMD_STS_CTL);  
>> 
>> Are you meant to just write flag to DIRECT_CMD_STS_CLEAR, maybe?
>
> Actually, I want to keep the status flag untouched, I'm just masking the
> interrupt to not be interrupted until the waiter has cleared the
> status flags.
>
> I'm doing that because the user may want to wait for several different
> events (error, done, timeout, ...), and I'm not doing the check in the
> interrupt handler. Instead, I'm just waking up the waiter and let him
> check the status flag himself.

Oh, OK.  That sounds good.


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

On Fri, 01 Sep 2017 09:51:59 +0200
Andrzej Hajda  wrote:

> On 31.08.2017 17:55, Boris Brezillon wrote:
> > Add a driver for Cadence DPI -> DSI bridge.
> >
> > This driver only support a subset of Cadence DSI bridge capabilities.
> >
> > Here is a non-exhaustive list of missing features:
> >  * burst mode
> >  * dynamic configuration of the DPHY based on the
> >  * support for additional input interfaces (SDI input)
> >
> > Signed-off-by: Boris Brezillon 
> > ---
> > Changes in v3:
> > - replace magic values by real timing calculation. The DPHY PLL clock
> >   is still hardcoded since we don't have a working DPHY block yet, and
> >   this is the piece of HW we need to dynamically configure the PLL
> >   rate based on the display refresh rate and the resolution.
> > - parse DSI devices represented with the OF-graph. This is needed to
> >   support DSI devices controlled through an external bus like I2C or
> >   SPI.
> > - use the DRM panel-bridge infrastructure to simplify the DRM panel
> >   logic
> >
> > Changes in v2:
> > - rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
> > - return the correct error when devm_clk_get(sysclk) fails
> > - add missing depends on OF and select DRM_PANEL in the Kconfig entry
> > ---
> >  drivers/gpu/drm/bridge/Kconfig|9 +
> >  drivers/gpu/drm/bridge/Makefile   |1 +
> >  drivers/gpu/drm/bridge/cdns-dsi.c | 1090 
> > +
> >  3 files changed, 1100 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index adf9ae0e0b7c..88c324b12e16 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -25,6 +25,15 @@ config DRM_ANALOGIX_ANX78XX
> >   the HDMI output of an application processor to MyDP
> >   or DisplayPort.
> >  
> > +config DRM_CDNS_DSI
> > +   tristate "Cadence DPI/DSI bridge"
> > +   select DRM_KMS_HELPER
> > +   select DRM_MIPI_DSI
> > +   select DRM_PANEL  
> 
> what about:
> select DRM_PANEL_BRIDGE

Oops, indeed. I'll add this dependency.

[...]

> > +
> > +static const struct of_device_id cdns_dsi_of_match[] = {
> > +   { .compatible = "cdns,dsi-1.3.1" },  
> 
> Do you really need version here? Wouldn't be enough checking ID_REG?
> This is only suggestion, no strong feelings.

You're right, I'll drop the version number.

> 
> The rest looks OK, so with Eric's comments addressed you can add:
> Reviewed-by: Andrzej Hajda 

Thanks for your review.

Boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Andrzej Hajda]

On 31.08.2017 17:55, Boris Brezillon wrote:
> Add a driver for Cadence DPI -> DSI bridge.
>
> This driver only support a subset of Cadence DSI bridge capabilities.
>
> Here is a non-exhaustive list of missing features:
>  * burst mode
>  * dynamic configuration of the DPHY based on the
>  * support for additional input interfaces (SDI input)
>
> Signed-off-by: Boris Brezillon 
> ---
> Changes in v3:
> - replace magic values by real timing calculation. The DPHY PLL clock
>   is still hardcoded since we don't have a working DPHY block yet, and
>   this is the piece of HW we need to dynamically configure the PLL
>   rate based on the display refresh rate and the resolution.
> - parse DSI devices represented with the OF-graph. This is needed to
>   support DSI devices controlled through an external bus like I2C or
>   SPI.
> - use the DRM panel-bridge infrastructure to simplify the DRM panel
>   logic
>
> Changes in v2:
> - rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
> - return the correct error when devm_clk_get(sysclk) fails
> - add missing depends on OF and select DRM_PANEL in the Kconfig entry
> ---
>  drivers/gpu/drm/bridge/Kconfig|9 +
>  drivers/gpu/drm/bridge/Makefile   |1 +
>  drivers/gpu/drm/bridge/cdns-dsi.c | 1090 
> +
>  3 files changed, 1100 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index adf9ae0e0b7c..88c324b12e16 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -25,6 +25,15 @@ config DRM_ANALOGIX_ANX78XX
> the HDMI output of an application processor to MyDP
> or DisplayPort.
>  
> +config DRM_CDNS_DSI
> + tristate "Cadence DPI/DSI bridge"
> + select DRM_KMS_HELPER
> + select DRM_MIPI_DSI
> + select DRM_PANEL

what about:
select DRM_PANEL_BRIDGE

> + depends on OF
> + help
> +   Support Cadence DPI to DSI bridge.
> +
>  config DRM_DUMB_VGA_DAC
>   tristate "Dumb VGA DAC Bridge support"
>   depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index defcf1e7ca1c..77b65e8ecf59 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> +obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
>  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += 
> megachips-stdp-ge-b850v3-fw.o
> diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c 
> b/drivers/gpu/drm/bridge/cdns-dsi.c
> new file mode 100644
> index ..f5dc26c90b20
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cdns-dsi.c
> @@ -0,0 +1,1090 @@
> +/*
> + * Copyright: 2017 Cadence Design Systems, Inc.
> + *
> + * Author: Boris Brezillon 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published 
> by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program.  If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define IP_CONF  0x0
> +#define SP_HS_FIFO_DEPTH(x)  (((x) & GENMASK(30, 26)) >> 26)
> +#define SP_LP_FIFO_DEPTH(x)  (((x) & GENMASK(25, 21)) >> 21)
> +#define VRS_FIFO_DEPTH(x)(((x) & GENMASK(20, 16)) >> 16)
> +#define DIRCMD_FIFO_DEPTH(x) (((x) & GENMASK(15, 13)) >> 13)
> +#define SDI_IFACE_32 BIT(12)
> +#define INTERNAL_DATAPATH_32 (0 << 10)
> +#define INTERNAL_DATAPATH_16 (1 << 10)
> +#define INTERNAL_DATAPATH_8  (3 << 10)
> +#define INTERNAL_DATAPATH_SIZE   ((x) & GENMASK(11, 10))
> +#define INTERNAL_DATAPATH_32 (0 << 10)
> +#define INTERNAL_DATAPATH_16 (1 << 10)
> +#define INTERNAL_DATAPATH_8  (3 << 10)
> +#define NUM_IFACE(x) x) & GENMASK(9, 8)) >> 8) + 1)
> +#define MAX_LANE_NB(x)   (((x) & GENMASK(7, 6)) >> 6)
> +#define RX_FIFO_DEPTH(x) ((x) & GENMASK(5, 0))
> +
> +#define MCTL_MAIN_DATA_CTL   0x4
> +#define TE_MIPI_POLLING_EN   BIT(25)
> +#define TE_HW_POLLING_EN BIT(24)
> +#define DISP_EOT_GEN BIT(18)
> +#define HOST_EOT_GEN BIT(17)
> +#define DISP_GEN_CH

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

Hi Eric,

On Thu, 31 Aug 2017 10:03:23 -0700
Eric Anholt  wrote:

> Boris Brezillon  writes:
> 
> > Add a driver for Cadence DPI -> DSI bridge.
> >
> > This driver only support a subset of Cadence DSI bridge capabilities.
> >
> > Here is a non-exhaustive list of missing features:
> >  * burst mode
> >  * dynamic configuration of the DPHY based on the
> >  * support for additional input interfaces (SDI input)
> >
> > Signed-off-by: Boris Brezillon 
> > ---
> > Changes in v3:
> > - replace magic values by real timing calculation. The DPHY PLL clock
> >   is still hardcoded since we don't have a working DPHY block yet, and
> >   this is the piece of HW we need to dynamically configure the PLL
> >   rate based on the display refresh rate and the resolution.
> > - parse DSI devices represented with the OF-graph. This is needed to
> >   support DSI devices controlled through an external bus like I2C or
> >   SPI.
> > - use the DRM panel-bridge infrastructure to simplify the DRM panel
> >   logic  
> 
> Just a few comments from me.  With the few straightforward nitpicks
> fixed, it gets my:
> 
> Acked-by: Eric Anholt 
> 
> > +static enum drm_mode_status
> > +cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
> > +  const struct drm_display_mode *mode)
> > +{
> > +   struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> > +   struct cdns_dsi *dsi = input_to_dsi(input);
> > +   struct cdns_dsi_output *output = &dsi->output;
> > +   int bpp;
> > +
> > +   /*
> > +* VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
> > +* least 1.
> > +*/
> > +   if (mode->vtotal - mode->vsync_end < 2)
> > +   return MODE_V_ILLEGAL;
> > +
> > +   /* VSA_DSI = VSA_DPI and must be at least 2. */
> > +   if (mode->vsync_end - mode->vsync_start < 2)
> > +   return MODE_V_ILLEGAL;
> > +
> > +   /* HACT must be a 32-bits aligned. */  
> 
> s/a /
> 
> > +   bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
> > +   if ((mode->hdisplay * bpp) % 32)
> > +   return MODE_H_ILLEGAL;
> > +
> > +   return MODE_OK;
> > +}  
> 
> 
> > +static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
> > +{
> > +   struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> > +   struct cdns_dsi *dsi = input_to_dsi(input);
> > +   struct cdns_dsi_output *output = &dsi->output;
> > +   u32 hsize0, hsa, hline, tmp, reg_wakeup, div;
> > +   unsigned long dphy_pll_period, tx_byte_period;
> > +   struct drm_display_mode *mode;
> > +   int bpp, nlanes;
> > +
> > +   mode = &bridge->encoder->crtc->state->adjusted_mode;
> > +   bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
> > +   nlanes = output->dev->lanes;
> > +
> > +   if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> > +   tmp = mode->crtc_htotal - mode->crtc_hsync_end;
> > +   else
> > +   tmp = mode->crtc_htotal - mode->crtc_hsync_start;
> > +   tmp = (tmp * bpp) / 8;
> > +   tmp = tmp < DSI_HBP_FRAME_OVERHEAD ? 0 : tmp - DSI_HBP_FRAME_OVERHEAD;
> > +   hsize0 = HBP_LEN(tmp);
> > +
> > +   tmp = mode->crtc_hsync_start - mode->crtc_hdisplay;
> > +   tmp = (tmp * bpp) / 8;  
> 
> How is this scaling supposed to work when you have RGB666_PACKED (bpp =
> 18)?  It seems like there would be bad rounding issues.

That's a good question. The spec does not say anything about rounding,
but I guess rounding up is safer. Richard, any opinion on that?

> 
> > +   tmp = tmp < DSI_HFP_FRAME_OVERHEAD ? 0 : tmp - DSI_HFP_FRAME_OVERHEAD;
> > +   hsize0 |= HFP_LEN(tmp);
> > +
> > +   if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> > +   tmp = mode->crtc_hsync_end - mode->crtc_hsync_start;
> > +   else
> > +   tmp = 0;
> > +   tmp = (tmp * 8) / bpp;  
> 
> This scaling is suspiciously different from the others around it.  Did
> you mean this?

It's a mistake, I'll fix that.

> 
> > +   tmp = tmp < DSI_HSA_FRAME_OVERHEAD ? 0 : tmp - DSI_HSA_FRAME_OVERHEAD;
> > +   hsa = tmp;
> > +   hsize0 |= HSA_LEN(tmp);
> > +
> > +   writel(hsize0, dsi->regs + VID_HSIZE1);
> > +   writel((mode->crtc_hdisplay * bpp) / 8, dsi->regs + VID_HSIZE2);
> > +
> > +   writel(VBP_LEN(mode->crtc_vtotal - mode->crtc_vsync_end - 1) |
> > +  VFP_LEN(mode->crtc_vsync_start - mode->crtc_vdisplay) |
> > +  VSA_LEN(mode->crtc_vsync_end - mode->crtc_vsync_start),
> > +  dsi->regs + VID_VSIZE1);
> > +   writel(mode->crtc_vdisplay, dsi->regs + VID_VSIZE2);
> > +
> > +   hline = (mode->crtc_htotal * bpp) / 8;
> > +   tmp = hline - DSI_HSA_FRAME_OVERHEAD;
> > +   if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> > +   tmp -= hsa + DSI_HSA_FRAME_OVERHEAD;
> > +
> > +   writel(BLK_LINE_PULSE_PKT_LEN(tmp), dsi->regs + VID_BLKSIZE2);
> > +   if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> > +   writel(MAX_LINE_LIMIT(tmp - DSI_NULL_FRAME_OVERHEAD),
> > +  dsi->regs + VID_VCA_SETTING2);
> > +
> > +   tmp = hline -
> >

Re: [PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Eric Anholt]

Boris Brezillon  writes:

> Add a driver for Cadence DPI -> DSI bridge.
>
> This driver only support a subset of Cadence DSI bridge capabilities.
>
> Here is a non-exhaustive list of missing features:
>  * burst mode
>  * dynamic configuration of the DPHY based on the
>  * support for additional input interfaces (SDI input)
>
> Signed-off-by: Boris Brezillon 
> ---
> Changes in v3:
> - replace magic values by real timing calculation. The DPHY PLL clock
>   is still hardcoded since we don't have a working DPHY block yet, and
>   this is the piece of HW we need to dynamically configure the PLL
>   rate based on the display refresh rate and the resolution.
> - parse DSI devices represented with the OF-graph. This is needed to
>   support DSI devices controlled through an external bus like I2C or
>   SPI.
> - use the DRM panel-bridge infrastructure to simplify the DRM panel
>   logic

Just a few comments from me.  With the few straightforward nitpicks
fixed, it gets my:

Acked-by: Eric Anholt 

> +static enum drm_mode_status
> +cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
> +const struct drm_display_mode *mode)
> +{
> + struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> + struct cdns_dsi *dsi = input_to_dsi(input);
> + struct cdns_dsi_output *output = &dsi->output;
> + int bpp;
> +
> + /*
> +  * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
> +  * least 1.
> +  */
> + if (mode->vtotal - mode->vsync_end < 2)
> + return MODE_V_ILLEGAL;
> +
> + /* VSA_DSI = VSA_DPI and must be at least 2. */
> + if (mode->vsync_end - mode->vsync_start < 2)
> + return MODE_V_ILLEGAL;
> +
> + /* HACT must be a 32-bits aligned. */

s/a /

> + bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
> + if ((mode->hdisplay * bpp) % 32)
> + return MODE_H_ILLEGAL;
> +
> + return MODE_OK;
> +}


> +static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
> +{
> + struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> + struct cdns_dsi *dsi = input_to_dsi(input);
> + struct cdns_dsi_output *output = &dsi->output;
> + u32 hsize0, hsa, hline, tmp, reg_wakeup, div;
> + unsigned long dphy_pll_period, tx_byte_period;
> + struct drm_display_mode *mode;
> + int bpp, nlanes;
> +
> + mode = &bridge->encoder->crtc->state->adjusted_mode;
> + bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
> + nlanes = output->dev->lanes;
> +
> + if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> + tmp = mode->crtc_htotal - mode->crtc_hsync_end;
> + else
> + tmp = mode->crtc_htotal - mode->crtc_hsync_start;
> + tmp = (tmp * bpp) / 8;
> + tmp = tmp < DSI_HBP_FRAME_OVERHEAD ? 0 : tmp - DSI_HBP_FRAME_OVERHEAD;
> + hsize0 = HBP_LEN(tmp);
> +
> + tmp = mode->crtc_hsync_start - mode->crtc_hdisplay;
> + tmp = (tmp * bpp) / 8;

How is this scaling supposed to work when you have RGB666_PACKED (bpp =
18)?  It seems like there would be bad rounding issues.

> + tmp = tmp < DSI_HFP_FRAME_OVERHEAD ? 0 : tmp - DSI_HFP_FRAME_OVERHEAD;
> + hsize0 |= HFP_LEN(tmp);
> +
> + if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> + tmp = mode->crtc_hsync_end - mode->crtc_hsync_start;
> + else
> + tmp = 0;
> + tmp = (tmp * 8) / bpp;

This scaling is suspiciously different from the others around it.  Did
you mean this?

> + tmp = tmp < DSI_HSA_FRAME_OVERHEAD ? 0 : tmp - DSI_HSA_FRAME_OVERHEAD;
> + hsa = tmp;
> + hsize0 |= HSA_LEN(tmp);
> +
> + writel(hsize0, dsi->regs + VID_HSIZE1);
> + writel((mode->crtc_hdisplay * bpp) / 8, dsi->regs + VID_HSIZE2);
> +
> + writel(VBP_LEN(mode->crtc_vtotal - mode->crtc_vsync_end - 1) |
> +VFP_LEN(mode->crtc_vsync_start - mode->crtc_vdisplay) |
> +VSA_LEN(mode->crtc_vsync_end - mode->crtc_vsync_start),
> +dsi->regs + VID_VSIZE1);
> + writel(mode->crtc_vdisplay, dsi->regs + VID_VSIZE2);
> +
> + hline = (mode->crtc_htotal * bpp) / 8;
> + tmp = hline - DSI_HSA_FRAME_OVERHEAD;
> + if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> + tmp -= hsa + DSI_HSA_FRAME_OVERHEAD;
> +
> + writel(BLK_LINE_PULSE_PKT_LEN(tmp), dsi->regs + VID_BLKSIZE2);
> + if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> + writel(MAX_LINE_LIMIT(tmp - DSI_NULL_FRAME_OVERHEAD),
> +dsi->regs + VID_VCA_SETTING2);
> +
> + tmp = hline -
> +   (DSI_HSS_VSS_VSE_FRAME_OVERHEAD + DSI_BLANKING_FRAME_OVERHEAD);
> + writel(BLK_LINE_EVENT_PKT_LEN(tmp), dsi->regs + VID_BLKSIZE1);
> + if (!(output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE))
> + writel(MAX_LINE_LIMIT(tmp - DSI_NULL_FRAME_OVERHEAD),
> +dsi->regs + VID_VCA_SETTING2);
> +
> + tmp = DIV_ROU

[PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

Add a driver for Cadence DPI -> DSI bridge.

This driver only support a subset of Cadence DSI bridge capabilities.

Here is a non-exhaustive list of missing features:
 * burst mode
 * dynamic configuration of the DPHY based on the
 * support for additional input interfaces (SDI input)

Signed-off-by: Boris Brezillon 
---
Changes in v3:
- replace magic values by real timing calculation. The DPHY PLL clock
  is still hardcoded since we don't have a working DPHY block yet, and
  this is the piece of HW we need to dynamically configure the PLL
  rate based on the display refresh rate and the resolution.
- parse DSI devices represented with the OF-graph. This is needed to
  support DSI devices controlled through an external bus like I2C or
  SPI.
- use the DRM panel-bridge infrastructure to simplify the DRM panel
  logic

Changes in v2:
- rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
- return the correct error when devm_clk_get(sysclk) fails
- add missing depends on OF and select DRM_PANEL in the Kconfig entry
---
 drivers/gpu/drm/bridge/Kconfig|9 +
 drivers/gpu/drm/bridge/Makefile   |1 +
 drivers/gpu/drm/bridge/cdns-dsi.c | 1090 +
 3 files changed, 1100 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index adf9ae0e0b7c..88c324b12e16 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -25,6 +25,15 @@ config DRM_ANALOGIX_ANX78XX
  the HDMI output of an application processor to MyDP
  or DisplayPort.
 
+config DRM_CDNS_DSI
+   tristate "Cadence DPI/DSI bridge"
+   select DRM_KMS_HELPER
+   select DRM_MIPI_DSI
+   select DRM_PANEL
+   depends on OF
+   help
+ Support Cadence DPI to DSI bridge.
+
 config DRM_DUMB_VGA_DAC
tristate "Dumb VGA DAC Bridge support"
depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index defcf1e7ca1c..77b65e8ecf59 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
+obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
 obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
 obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
 obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += 
megachips-stdp-ge-b850v3-fw.o
diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c 
b/drivers/gpu/drm/bridge/cdns-dsi.c
new file mode 100644
index ..f5dc26c90b20
--- /dev/null
+++ b/drivers/gpu/drm/bridge/cdns-dsi.c
@@ -0,0 +1,1090 @@
+/*
+ * Copyright: 2017 Cadence Design Systems, Inc.
+ *
+ * Author: Boris Brezillon 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IP_CONF0x0
+#define SP_HS_FIFO_DEPTH(x)(((x) & GENMASK(30, 26)) >> 26)
+#define SP_LP_FIFO_DEPTH(x)(((x) & GENMASK(25, 21)) >> 21)
+#define VRS_FIFO_DEPTH(x)  (((x) & GENMASK(20, 16)) >> 16)
+#define DIRCMD_FIFO_DEPTH(x)   (((x) & GENMASK(15, 13)) >> 13)
+#define SDI_IFACE_32   BIT(12)
+#define INTERNAL_DATAPATH_32   (0 << 10)
+#define INTERNAL_DATAPATH_16   (1 << 10)
+#define INTERNAL_DATAPATH_8(3 << 10)
+#define INTERNAL_DATAPATH_SIZE ((x) & GENMASK(11, 10))
+#define INTERNAL_DATAPATH_32   (0 << 10)
+#define INTERNAL_DATAPATH_16   (1 << 10)
+#define INTERNAL_DATAPATH_8(3 << 10)
+#define NUM_IFACE(x)   x) & GENMASK(9, 8)) >> 8) + 1)
+#define MAX_LANE_NB(x) (((x) & GENMASK(7, 6)) >> 6)
+#define RX_FIFO_DEPTH(x)   ((x) & GENMASK(5, 0))
+
+#define MCTL_MAIN_DATA_CTL 0x4
+#define TE_MIPI_POLLING_EN BIT(25)
+#define TE_HW_POLLING_EN   BIT(24)
+#define DISP_EOT_GEN   BIT(18)
+#define HOST_EOT_GEN   BIT(17)
+#define DISP_GEN_CHECKSUM  BIT(16)
+#define DISP_GEN_ECC   BIT(15)
+#define BTA_EN BIT(14)
+#define READ_ENBIT(13)
+#define REG_TE_EN  BIT(12)
+#define IF_TE_EN(x)BIT(8 + (x))
+#define T