diff mbox series

[4/4] drm/msm/dp: Add support for SC8180x eDP

Message ID 20210511042043.592802-5-bjorn.andersson@linaro.org (mailing list archive)
State New
Headers show
Series drm/msm/dp: Add support for SC8180x eDP controller | expand

Commit Message

Bjorn Andersson May 11, 2021, 4:20 a.m. UTC
The eDP controller found in SC8180x is at large compatible with the
current implementation, but has its register blocks at slightly
different offsets.

Add the compatible and the new register layout.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c |  1 +
 drivers/gpu/drm/msm/dp/dp_parser.c  | 28 ++++++++++++++++++++--------
 2 files changed, 21 insertions(+), 8 deletions(-)

Comments

Abhinav Kumar May 28, 2021, 11:40 p.m. UTC | #1
On 2021-05-10 21:20, Bjorn Andersson wrote:
> The eDP controller found in SC8180x is at large compatible with the
> current implementation, but has its register blocks at slightly
> different offsets.
> 
> Add the compatible and the new register layout.
> 
I am not able to completely recall the history of why in the DP bindings
we added DP register base as a big hunk and let catalog handle the 
submodule
offsets.

I guess earlier that made sense because DP sub-block offsets were fixed.
But if we plan to re-use the DP driver for eDP as well like this series, 
then maybe it might be
better if this comes from device tree like the earlier version was 
planning to

https://patchwork.kernel.org/project/dri-devel/patch/0101016ec6ddf446-e87ab1ce-5cbf-40a0-a0bb-cd0151cd577a-000000@us-west-2.amazonses.com/


+- reg:                  Base address and length of DP hardware's memory 
mapped regions.
+- cell-index:           Specifies the controller instance.
+- reg-names:            A list of strings that name the list of regs.
+			"dp_ahb" - DP controller memory region.
+			"dp_aux" - DP AUX memory region.
+			"dp_link" - DP link layer memory region.
+			"dp_p0" - DP pixel clock domain memory region.
+			"dp_phy" - DP PHY memory region.
+			"dp_ln_tx0" - USB3 DP PHY combo TX-0 lane memory region.
+			"dp_ln_tx1" - USB3 DP PHY combo TX-1 lane memory region.

Now there is more reason to separate the sub-module offsets like 
ahb/aux/link/p0
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c |  1 +
>  drivers/gpu/drm/msm/dp/dp_parser.c  | 28 ++++++++++++++++++++--------
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index d1319b58e901..0be03bdc882c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -121,6 +121,7 @@ struct dp_display_private {
> 
>  static const struct of_device_id dp_dt_match[] = {
>  	{.compatible = "qcom,sc7180-dp"},
> +	{ .compatible = "qcom,sc8180x-edp" },
>  	{}
>  };
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> index 51ec85b4803b..47cf18bba4b2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -251,6 +251,7 @@ static int dp_parser_clock(struct dp_parser 
> *parser)
>  static int dp_parser_parse(struct dp_parser *parser)
>  {
>  	struct dss_io_data *io = &parser->io.dp_controller;
> +	struct device *dev = &parser->pdev->dev;
>  	int rc = 0;
> 
>  	if (!parser) {
> @@ -276,14 +277,25 @@ static int dp_parser_parse(struct dp_parser 
> *parser)
>  	 */
>  	parser->regulator_cfg = &sdm845_dp_reg_cfg;
> 
> -	io->ahb = io->base + 0x0;
> -	io->ahb_len = 0x200;
> -	io->aux = io->base + 0x200;
> -	io->aux_len = 0x200;
> -	io->link = io->base + 0x400;
> -	io->link_len = 0x600;
> -	io->p0 = io->base + 0x1000;
> -	io->p0_len = 0x400;
> +	if (of_device_is_compatible(dev->of_node, "qcom,sc8180x-edp")) {
> +		io->ahb = io->base + 0x0;
> +		io->ahb_len = 0x200;
> +		io->aux = io->base + 0x200;
> +		io->aux_len = 0x200;
> +		io->link = io->base + 0x400;
> +		io->link_len = 0x600;
> +		io->p0 = io->base + 0xa00;
> +		io->p0_len = 0x400;
> +	} else {
> +		io->ahb = io->base + 0x0;
> +		io->ahb_len = 0x200;
> +		io->aux = io->base + 0x200;
> +		io->aux_len = 0x200;
> +		io->link = io->base + 0x400;
> +		io->link_len = 0x600;
> +		io->p0 = io->base + 0x1000;
> +		io->p0_len = 0x400;
> +	}
> 
>  	return 0;
>  }
Bjorn Andersson May 29, 2021, 5:09 p.m. UTC | #2
On Fri 28 May 18:40 CDT 2021, abhinavk@codeaurora.org wrote:

> On 2021-05-10 21:20, Bjorn Andersson wrote:
> > The eDP controller found in SC8180x is at large compatible with the
> > current implementation, but has its register blocks at slightly
> > different offsets.
> > 
> > Add the compatible and the new register layout.
> > 
> I am not able to completely recall the history of why in the DP bindings
> we added DP register base as a big hunk and let catalog handle the submodule
> offsets.
> 
> I guess earlier that made sense because DP sub-block offsets were fixed.
> But if we plan to re-use the DP driver for eDP as well like this series,
> then maybe it might be
> better if this comes from device tree like the earlier version was planning
> to
> 
> https://patchwork.kernel.org/project/dri-devel/patch/0101016ec6ddf446-e87ab1ce-5cbf-40a0-a0bb-cd0151cd577a-000000@us-west-2.amazonses.com/
> 
> 
> +- reg:                  Base address and length of DP hardware's memory
> mapped regions.
> +- cell-index:           Specifies the controller instance.
> +- reg-names:            A list of strings that name the list of regs.
> +			"dp_ahb" - DP controller memory region.
> +			"dp_aux" - DP AUX memory region.
> +			"dp_link" - DP link layer memory region.
> +			"dp_p0" - DP pixel clock domain memory region.
> +			"dp_phy" - DP PHY memory region.
> +			"dp_ln_tx0" - USB3 DP PHY combo TX-0 lane memory region.
> +			"dp_ln_tx1" - USB3 DP PHY combo TX-1 lane memory region.
> 
> Now there is more reason to separate the sub-module offsets like
> ahb/aux/link/p0

I like it, will rewrite the patch accordingly.

Regards,
Bjorn

> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_display.c |  1 +
> >  drivers/gpu/drm/msm/dp/dp_parser.c  | 28 ++++++++++++++++++++--------
> >  2 files changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index d1319b58e901..0be03bdc882c 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -121,6 +121,7 @@ struct dp_display_private {
> > 
> >  static const struct of_device_id dp_dt_match[] = {
> >  	{.compatible = "qcom,sc7180-dp"},
> > +	{ .compatible = "qcom,sc8180x-edp" },
> >  	{}
> >  };
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> > b/drivers/gpu/drm/msm/dp/dp_parser.c
> > index 51ec85b4803b..47cf18bba4b2 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> > @@ -251,6 +251,7 @@ static int dp_parser_clock(struct dp_parser *parser)
> >  static int dp_parser_parse(struct dp_parser *parser)
> >  {
> >  	struct dss_io_data *io = &parser->io.dp_controller;
> > +	struct device *dev = &parser->pdev->dev;
> >  	int rc = 0;
> > 
> >  	if (!parser) {
> > @@ -276,14 +277,25 @@ static int dp_parser_parse(struct dp_parser
> > *parser)
> >  	 */
> >  	parser->regulator_cfg = &sdm845_dp_reg_cfg;
> > 
> > -	io->ahb = io->base + 0x0;
> > -	io->ahb_len = 0x200;
> > -	io->aux = io->base + 0x200;
> > -	io->aux_len = 0x200;
> > -	io->link = io->base + 0x400;
> > -	io->link_len = 0x600;
> > -	io->p0 = io->base + 0x1000;
> > -	io->p0_len = 0x400;
> > +	if (of_device_is_compatible(dev->of_node, "qcom,sc8180x-edp")) {
> > +		io->ahb = io->base + 0x0;
> > +		io->ahb_len = 0x200;
> > +		io->aux = io->base + 0x200;
> > +		io->aux_len = 0x200;
> > +		io->link = io->base + 0x400;
> > +		io->link_len = 0x600;
> > +		io->p0 = io->base + 0xa00;
> > +		io->p0_len = 0x400;
> > +	} else {
> > +		io->ahb = io->base + 0x0;
> > +		io->ahb_len = 0x200;
> > +		io->aux = io->base + 0x200;
> > +		io->aux_len = 0x200;
> > +		io->link = io->base + 0x400;
> > +		io->link_len = 0x600;
> > +		io->p0 = io->base + 0x1000;
> > +		io->p0_len = 0x400;
> > +	}
> > 
> >  	return 0;
> >  }
Stephen Boyd June 4, 2021, 9:56 p.m. UTC | #3
Quoting abhinavk@codeaurora.org (2021-05-28 16:40:32)
> On 2021-05-10 21:20, Bjorn Andersson wrote:
> > The eDP controller found in SC8180x is at large compatible with the
> > current implementation, but has its register blocks at slightly
> > different offsets.
> >
> > Add the compatible and the new register layout.
> >
> I am not able to completely recall the history of why in the DP bindings
> we added DP register base as a big hunk and let catalog handle the
> submodule
> offsets.

I complained that there were many I/O regions for the DP block that
didn't seem to be changing between SoCs. Nobody objected to removing it
back then, but if the plan was to move things around later on then it
makes sense to split it out like it was done initially.

>
> I guess earlier that made sense because DP sub-block offsets were fixed.
> But if we plan to re-use the DP driver for eDP as well like this series,
> then maybe it might be
> better if this comes from device tree like the earlier version was
> planning to
>
> https://patchwork.kernel.org/project/dri-devel/patch/0101016ec6ddf446-e87ab1ce-5cbf-40a0-a0bb-cd0151cd577a-000000@us-west-2.amazonses.com/
>

Agreed.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index d1319b58e901..0be03bdc882c 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -121,6 +121,7 @@  struct dp_display_private {
 
 static const struct of_device_id dp_dt_match[] = {
 	{.compatible = "qcom,sc7180-dp"},
+	{ .compatible = "qcom,sc8180x-edp" },
 	{}
 };
 
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 51ec85b4803b..47cf18bba4b2 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -251,6 +251,7 @@  static int dp_parser_clock(struct dp_parser *parser)
 static int dp_parser_parse(struct dp_parser *parser)
 {
 	struct dss_io_data *io = &parser->io.dp_controller;
+	struct device *dev = &parser->pdev->dev;
 	int rc = 0;
 
 	if (!parser) {
@@ -276,14 +277,25 @@  static int dp_parser_parse(struct dp_parser *parser)
 	 */
 	parser->regulator_cfg = &sdm845_dp_reg_cfg;
 
-	io->ahb = io->base + 0x0;
-	io->ahb_len = 0x200;
-	io->aux = io->base + 0x200;
-	io->aux_len = 0x200;
-	io->link = io->base + 0x400;
-	io->link_len = 0x600;
-	io->p0 = io->base + 0x1000;
-	io->p0_len = 0x400;
+	if (of_device_is_compatible(dev->of_node, "qcom,sc8180x-edp")) {
+		io->ahb = io->base + 0x0;
+		io->ahb_len = 0x200;
+		io->aux = io->base + 0x200;
+		io->aux_len = 0x200;
+		io->link = io->base + 0x400;
+		io->link_len = 0x600;
+		io->p0 = io->base + 0xa00;
+		io->p0_len = 0x400;
+	} else {
+		io->ahb = io->base + 0x0;
+		io->ahb_len = 0x200;
+		io->aux = io->base + 0x200;
+		io->aux_len = 0x200;
+		io->link = io->base + 0x400;
+		io->link_len = 0x600;
+		io->p0 = io->base + 0x1000;
+		io->p0_len = 0x400;
+	}
 
 	return 0;
 }