Message ID | 20190822195600.30787-1-jacopo+renesas@jmondi.org (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | arm64: dts: renesas: Add LIF channel index to 'vsps' | expand |
Hi Jacopo, Thank you for the patch. On Thu, Aug 22, 2019 at 09:56:00PM +0200, Jacopo Mondi wrote: > According to the Renesas R-Car DU bindings documentation, the 'vsps' > property should be composed by a phandle to the VSP instance and the s/composed by/composed of/ > index of the LIF channel assigned to the DU channel. Some SoC device > tree source files do not specify any LIF channel index, relying on the > driver defaulting it to 0 if not specified. > > Align all device tree files by specifying the LIF channel index as > prescribed by the bindings documentation. While at it, add a comment to > the 'vsps' property parsing routine to point out the LIF channel index > is still defaulted to 0 for backward compatibility with non-standard DTB > found in the wild. I wouldn't say non-standard, I would instead mention compatible with a previous version of the DT bindings. > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > > Patch based on Geert's latest renesas-devel master branch > --- > > arch/arm64/boot/dts/renesas/r8a774a1.dtsi | 2 +- > arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi | 2 +- > arch/arm64/boot/dts/renesas/r8a7796.dtsi | 2 +- > arch/arm64/boot/dts/renesas/r8a77970.dtsi | 2 +- > arch/arm64/boot/dts/renesas/r8a77980.dtsi | 2 +- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 9 ++++++++- > 6 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi > index 06c7c849c8ab..d179ee3da308 100644 > --- a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi > +++ b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi > @@ -2651,7 +2651,7 @@ > clock-names = "du.0", "du.1", "du.2"; > status = "disabled"; > > - vsps = <&vspd0 &vspd1 &vspd2>; > + vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>; > > ports { > #address-cells = <1>; > diff --git a/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi b/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi > index e4650ae5b75a..14d8513d2a47 100644 > --- a/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi > +++ b/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi > @@ -30,7 +30,7 @@ > }; > > &du { > - vsps = <&vspd0 &vspd1 &vspd2 &vspd3>; > + vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd3 0>; > }; > > &fcpvb1 { > diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi > index 3dc9d73f589a..8c9bf985d436 100644 > --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi > +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi > @@ -2765,7 +2765,7 @@ > clock-names = "du.0", "du.1", "du.2"; > status = "disabled"; > > - vsps = <&vspd0 &vspd1 &vspd2>; > + vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>; > > ports { > #address-cells = <1>; > diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi b/arch/arm64/boot/dts/renesas/r8a77970.dtsi > index 0cd3b376635d..2c4ab70e2a39 100644 > --- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi > +++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi > @@ -1120,7 +1120,7 @@ > clock-names = "du.0"; > power-domains = <&sysc R8A77970_PD_ALWAYS_ON>; > resets = <&cpg 724>; > - vsps = <&vspd0>; > + vsps = <&vspd0 0>; > status = "disabled"; > > ports { > diff --git a/arch/arm64/boot/dts/renesas/r8a77980.dtsi b/arch/arm64/boot/dts/renesas/r8a77980.dtsi > index 461a47ea656d..042f4089e546 100644 > --- a/arch/arm64/boot/dts/renesas/r8a77980.dtsi > +++ b/arch/arm64/boot/dts/renesas/r8a77980.dtsi > @@ -1495,7 +1495,7 @@ > clock-names = "du.0"; > power-domains = <&sysc R8A77980_PD_ALWAYS_ON>; > resets = <&cpg 724>; > - vsps = <&vspd0>; > + vsps = <&vspd0 0>; > status = "disabled"; > > ports { > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > index 2dc9caee8767..1a9e182b2b55 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > @@ -585,7 +585,14 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu) > > vsps[j].crtcs_mask |= BIT(i); > > - /* Store the VSP pointer and pipe index in the CRTC. */ > + /* > + * Store the VSP pointer and pipe index in the CRTC. > + * > + * FIXME: According to the DT bindings, the LIF pipe instance > + * index shall always be specified. For backward compatibility > + * with older DTB without any index specified, default it to 0 > + * if cells < 1. No need for a FIXME, there's nothing to be fixed here. I would write this as * If the second cell of the VSP specifier isn't * present, default to 0 to remain compatible with older * DT bindings. With this fixed here and in the commit message, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Note that Simon or Geert will likely ask you to split this patch in two, in which case I'll take the driver part in my tree. > + */ > rcdu->crtcs[i].vsp = &rcdu->vsps[j]; > rcdu->crtcs[i].vsp_pipe = cells >= 1 ? args.args[0] : 0; > }
Hi Laurent, On Fri, Aug 23, 2019 at 03:12:09AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Aug 22, 2019 at 09:56:00PM +0200, Jacopo Mondi wrote: > > According to the Renesas R-Car DU bindings documentation, the 'vsps' > > property should be composed by a phandle to the VSP instance and the > > s/composed by/composed of/ > > > index of the LIF channel assigned to the DU channel. Some SoC device > > tree source files do not specify any LIF channel index, relying on the > > driver defaulting it to 0 if not specified. > > > > Align all device tree files by specifying the LIF channel index as > > prescribed by the bindings documentation. While at it, add a comment to > > the 'vsps' property parsing routine to point out the LIF channel index > > is still defaulted to 0 for backward compatibility with non-standard DTB > > found in the wild. > > I wouldn't say non-standard, I would instead mention compatible with a > previous version of the DT bindings. > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > > > Patch based on Geert's latest renesas-devel master branch > > --- > > > > arch/arm64/boot/dts/renesas/r8a774a1.dtsi | 2 +- > > arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi | 2 +- > > arch/arm64/boot/dts/renesas/r8a7796.dtsi | 2 +- > > arch/arm64/boot/dts/renesas/r8a77970.dtsi | 2 +- > > arch/arm64/boot/dts/renesas/r8a77980.dtsi | 2 +- > > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 9 ++++++++- > > 6 files changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi > > index 06c7c849c8ab..d179ee3da308 100644 > > --- a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi > > +++ b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi > > @@ -2651,7 +2651,7 @@ > > clock-names = "du.0", "du.1", "du.2"; > > status = "disabled"; > > > > - vsps = <&vspd0 &vspd1 &vspd2>; > > + vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>; > > > > ports { > > #address-cells = <1>; > > diff --git a/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi b/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi > > index e4650ae5b75a..14d8513d2a47 100644 > > --- a/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi > > +++ b/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi > > @@ -30,7 +30,7 @@ > > }; > > > > &du { > > - vsps = <&vspd0 &vspd1 &vspd2 &vspd3>; > > + vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd3 0>; > > }; > > > > &fcpvb1 { > > diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi > > index 3dc9d73f589a..8c9bf985d436 100644 > > --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi > > +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi > > @@ -2765,7 +2765,7 @@ > > clock-names = "du.0", "du.1", "du.2"; > > status = "disabled"; > > > > - vsps = <&vspd0 &vspd1 &vspd2>; > > + vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>; > > > > ports { > > #address-cells = <1>; > > diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi b/arch/arm64/boot/dts/renesas/r8a77970.dtsi > > index 0cd3b376635d..2c4ab70e2a39 100644 > > --- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi > > +++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi > > @@ -1120,7 +1120,7 @@ > > clock-names = "du.0"; > > power-domains = <&sysc R8A77970_PD_ALWAYS_ON>; > > resets = <&cpg 724>; > > - vsps = <&vspd0>; > > + vsps = <&vspd0 0>; > > status = "disabled"; > > > > ports { > > diff --git a/arch/arm64/boot/dts/renesas/r8a77980.dtsi b/arch/arm64/boot/dts/renesas/r8a77980.dtsi > > index 461a47ea656d..042f4089e546 100644 > > --- a/arch/arm64/boot/dts/renesas/r8a77980.dtsi > > +++ b/arch/arm64/boot/dts/renesas/r8a77980.dtsi > > @@ -1495,7 +1495,7 @@ > > clock-names = "du.0"; > > power-domains = <&sysc R8A77980_PD_ALWAYS_ON>; > > resets = <&cpg 724>; > > - vsps = <&vspd0>; > > + vsps = <&vspd0 0>; > > status = "disabled"; > > > > ports { > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > > index 2dc9caee8767..1a9e182b2b55 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > > @@ -585,7 +585,14 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu) > > > > vsps[j].crtcs_mask |= BIT(i); > > > > - /* Store the VSP pointer and pipe index in the CRTC. */ > > + /* > > + * Store the VSP pointer and pipe index in the CRTC. > > + * > > + * FIXME: According to the DT bindings, the LIF pipe instance > > + * index shall always be specified. For backward compatibility > > + * with older DTB without any index specified, default it to 0 > > + * if cells < 1. > > No need for a FIXME, there's nothing to be fixed here. I would write > this as > > * If the second cell of the VSP specifier isn't > * present, default to 0 to remain compatible with older > * DT bindings. > > With this fixed here and in the commit message, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Thanks > Note that Simon or Geert will likely ask you to split this patch in two, > in which case I'll take the driver part in my tree. I was not sure how to split this in facts.. Simon, Geert, would you like a v2 with DT changes separated from the driver comment update? (for the DT changes, one patch per SoC, or a single one?) Thanks j > > > + */ > > rcdu->crtcs[i].vsp = &rcdu->vsps[j]; > > rcdu->crtcs[i].vsp_pipe = cells >= 1 ? args.args[0] : 0; > > } > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Fri, Aug 23, 2019 at 8:31 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > On Fri, Aug 23, 2019 at 03:12:09AM +0300, Laurent Pinchart wrote: > > On Thu, Aug 22, 2019 at 09:56:00PM +0200, Jacopo Mondi wrote: > > > According to the Renesas R-Car DU bindings documentation, the 'vsps' > > > property should be composed by a phandle to the VSP instance and the > > > > > index of the LIF channel assigned to the DU channel. Some SoC device > > > tree source files do not specify any LIF channel index, relying on the > > > driver defaulting it to 0 if not specified. > > > > > > Align all device tree files by specifying the LIF channel index as > > > prescribed by the bindings documentation. While at it, add a comment to > > > the 'vsps' property parsing routine to point out the LIF channel index > > > is still defaulted to 0 for backward compatibility with non-standard DTB > > > found in the wild. > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > Note that Simon or Geert will likely ask you to split this patch in two, > > in which case I'll take the driver part in my tree. > > I was not sure how to split this in facts.. Simon, Geert, would you > like a v2 with DT changes separated from the driver comment update? Yes please. > (for the DT changes, one patch per SoC, or a single one?) One patch for all SoCs is fine: arm64: dts: renesas: Add LIF channel indices to vsps properties Thanks! Gr{oetje,eeting}s, Geert
diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi index 06c7c849c8ab..d179ee3da308 100644 --- a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi @@ -2651,7 +2651,7 @@ clock-names = "du.0", "du.1", "du.2"; status = "disabled"; - vsps = <&vspd0 &vspd1 &vspd2>; + vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>; ports { #address-cells = <1>; diff --git a/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi b/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi index e4650ae5b75a..14d8513d2a47 100644 --- a/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi @@ -30,7 +30,7 @@ }; &du { - vsps = <&vspd0 &vspd1 &vspd2 &vspd3>; + vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd3 0>; }; &fcpvb1 { diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi index 3dc9d73f589a..8c9bf985d436 100644 --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi @@ -2765,7 +2765,7 @@ clock-names = "du.0", "du.1", "du.2"; status = "disabled"; - vsps = <&vspd0 &vspd1 &vspd2>; + vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>; ports { #address-cells = <1>; diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi b/arch/arm64/boot/dts/renesas/r8a77970.dtsi index 0cd3b376635d..2c4ab70e2a39 100644 --- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi @@ -1120,7 +1120,7 @@ clock-names = "du.0"; power-domains = <&sysc R8A77970_PD_ALWAYS_ON>; resets = <&cpg 724>; - vsps = <&vspd0>; + vsps = <&vspd0 0>; status = "disabled"; ports { diff --git a/arch/arm64/boot/dts/renesas/r8a77980.dtsi b/arch/arm64/boot/dts/renesas/r8a77980.dtsi index 461a47ea656d..042f4089e546 100644 --- a/arch/arm64/boot/dts/renesas/r8a77980.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77980.dtsi @@ -1495,7 +1495,7 @@ clock-names = "du.0"; power-domains = <&sysc R8A77980_PD_ALWAYS_ON>; resets = <&cpg 724>; - vsps = <&vspd0>; + vsps = <&vspd0 0>; status = "disabled"; ports { diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 2dc9caee8767..1a9e182b2b55 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -585,7 +585,14 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu) vsps[j].crtcs_mask |= BIT(i); - /* Store the VSP pointer and pipe index in the CRTC. */ + /* + * Store the VSP pointer and pipe index in the CRTC. + * + * FIXME: According to the DT bindings, the LIF pipe instance + * index shall always be specified. For backward compatibility + * with older DTB without any index specified, default it to 0 + * if cells < 1. + */ rcdu->crtcs[i].vsp = &rcdu->vsps[j]; rcdu->crtcs[i].vsp_pipe = cells >= 1 ? args.args[0] : 0; }
According to the Renesas R-Car DU bindings documentation, the 'vsps' property should be composed by a phandle to the VSP instance and the index of the LIF channel assigned to the DU channel. Some SoC device tree source files do not specify any LIF channel index, relying on the driver defaulting it to 0 if not specified. Align all device tree files by specifying the LIF channel index as prescribed by the bindings documentation. While at it, add a comment to the 'vsps' property parsing routine to point out the LIF channel index is still defaulted to 0 for backward compatibility with non-standard DTB found in the wild. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- Patch based on Geert's latest renesas-devel master branch --- arch/arm64/boot/dts/renesas/r8a774a1.dtsi | 2 +- arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi | 2 +- arch/arm64/boot/dts/renesas/r8a7796.dtsi | 2 +- arch/arm64/boot/dts/renesas/r8a77970.dtsi | 2 +- arch/arm64/boot/dts/renesas/r8a77980.dtsi | 2 +- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 9 ++++++++- 6 files changed, 13 insertions(+), 6 deletions(-) -- 2.22.0