Hi Sergei,
On Friday, 12 January 2018 11:23:00 EET Sergei Shtylyov wrote:
> On 1/12/2018 4:13 AM, Laurent Pinchart wrote:
> >> Add support for the R-Car V3M (R8A77970) SoC to the DU driver (this SoC
> >> has only 1 display port). Note that there are some differences with the
> >> other R-Car gen3 SoCs in the LVDS encoder part, e.g. LVDPLLCR has the
> >> same layout as on the R-Car gen2 SoCs...
> >>
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
> >
> > Could you please rebase this series on top of the LVDS rework posted as
> > "[PATCH 00/10] R-Car DU: Convert LVDS code to bridge driver" (https://
> > www.spinics.net/lists/dri-devel/msg161931.html) ? It should make it easier
> > to implement support for V3M. Please then split the DU and LVDS drivers
> > changes in two patches.
> >
> >> ---
> >>
> >> Documentation/devicetree/bindings/display/renesas,du.txt | 1
> >
> > Please split the DT bindings changes to a separate patch.
>
> I don't like putting a one-line chnage into a separate bindings patch...
> >> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 23 +++++++
> >> drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1
> >> drivers/gpu/drm/rcar-du/rcar_du_group.c | 10 +++---
> >> drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 20 ++++---
> >> 5 files changed, 45 insertions(+), 10 deletions(-)
[snip]
> >> Index: linux/drivers/gpu/drm/rcar-du/rcar_du_group.c
> >> ===================================================================
> >> --- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_group.c
> >> +++ linux/drivers/gpu/drm/rcar-du/rcar_du_group.c
> >> @@ -133,10 +133,12 @@ static void rcar_du_group_setup(struct r
> >>
> >> rcar_du_group_write(rgrp, DORCR, DORCR_PG1D_DS1 | DORCR_DPRS);
> >>
> >> /* Apply planes to CRTCs association. */
> >>
> >> - mutex_lock(&rgrp->lock);
> >> - rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> >> - rgrp->dptsr_planes);
> >> - mutex_unlock(&rgrp->lock);
> >> + if (rcdu->info->num_crtcs > 1) {
> >> + mutex_lock(&rgrp->lock);
> >> + rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> >> + rgrp->dptsr_planes);
> >> + mutex_unlock(&rgrp->lock);
> >> + }
> >
> > Shouldn't you skip writing to the DPTSR register if there's a single DPTSR
> > in the group ? That would then apply to M3-W as well, which doesn't have
> > the DPTSR2 register. I'd split this change to a separate patch.
>
> OK, I guess you know this stuff better -- I didn't know DPTSR2 is used
> at all... :-)
Should I send a patch for this as well ?
> [...]
>
> >> Index: linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> >> ===================================================================
> >> --- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> >> +++ linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
>
> [...]
>
> >> @@ -177,14 +185,14 @@ int rcar_du_lvdsenc_enable(struct rcar_d
> >> void rcar_du_lvdsenc_atomic_check(struct rcar_du_lvdsenc *lvds,
> >> struct drm_display_mode *mode)
> >> {
> >> - struct rcar_du_device *rcdu = lvds->dev;
> >> + const struct rcar_du_device_info *info = lvds->dev->info;
> >>
> >> /*
> >> * The internal LVDS encoder has a restricted clock frequency
> >> operating
> >> - * range (30MHz to 150MHz on Gen2, 25.175MHz to 148.5MHz on Gen3).
> >> Clamp
> >> - * the clock accordingly.
> >> + * range (30MHz to 150MHz on Gen2 and R-Car V3M, 25.175MHz to 148.5MHz
> >> + * on Gen3). Clamp the clock accordingly.
> >> */
> >> - if (rcdu->info->gen < 3)
> >> + if (info->gen < 3 || info->model == R8A77970)
> >> mode->clock = clamp(mode->clock, 30000, 150000);
> >
> > According to the datasheet the frequency range for V3M is the same as for
> > the H3 and M3 SoCs.
>
> Indeed! I thought it's determined by the LVDPLLCR layout but it's not...
>
> > The range seems to have changed starting in datasheet version
> > 0.52. I would fix the range in a separate patch first.
>
> Yes.
>
> > If you want I can send patches to fix this issue
>
> Yes, please. You clearly know about DU more than me. :-)
I've prepared a patch, I'm testing it now and I'll then send it.
> > and the previous one, or you can write them and include them in v2. Let me
> > know what you'd prefer.
> >
> >> else
> >> mode->clock = clamp(mode->clock, 25175, 148500);
>
> The lower bound documented on gen3 is 31 MHz indeed...
And I just found out that the latest versions of the Gen2 datasheets also
document the 31 MHz - 148.5 MHz range. This will simplify the code.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
