mbox series

[v3,00/10] Fix DSI host idx detection on HW revision clash

Message ID 20230307-topic-dsi_qcm-v3-0-8bd7e1add38a@linaro.org
Headers show
Series Fix DSI host idx detection on HW revision clash | expand

Message

Konrad Dybcio March 7, 2023, 1:01 p.m. UTC
v2 -> v3:
- Merge with [1], I should have done that earlier..
  - Squash 6115 compatible patches into one
- Pick up tags (except Rob's ack in 6115 compatible addition, as it was changed)
- Use b4 (sorry if you got an incomplete set of messages before..)

[1] https://lore.kernel.org/linux-arm-msm/145066db-5723-6baa-237d-7c2b8fd476d9@linaro.org/
v2: https://lore.kernel.org/linux-arm-msm/20230213121012.1768296-1-konrad.dybcio@linaro.org/

v1 -> v2:
- squash the 2d-array-ification and fixing up the logic into one patch
- drop num_variants, loop over VARIANTS_MAX*DSI_MAX unconditionally
- drop inadequate Fixes: tags
- pick up rbs

v1: https://lore.kernel.org/linux-arm-msm/20230211115110.1462920-1-konrad.dybcio@linaro.org/

Some DSI host versions are implemented on multiple SoCs which use
vastly different register maps. This messes with our current
assumptions of being able to map {dsi0, dsi1} to {reg0, reg1}.
Solve that by adding a way of specifying multiple sets of base
registers and try comparing them against the register specified in DT
until we find a match.

This removes the need for the QCM2290-specific compatible which was
used in the SM6115 DT (which uses DSIv2.4.1, just like SC7180).
The series also takes care of that.

Tested on SM6115P Lenovo Tab P11 and SM8350 PDX215

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (10):
      dt-bindings: display/msm: dsi-controller-main: Fix deprecated QCM2290 compatible
      drm/msm/dsi: Get rid of msm_dsi_config::num_dsi
      drm/msm/dsi: Fix DSI index detection when version clash occurs
      drm/msm/dsi: dsi_cfg: Deduplicate identical structs
      drm/msm/dsi: dsi_cfg: Merge SC7180 config into SDM845
      drm/msm/dsi: Switch the QCM2290-specific compatible to index autodetection
      drm/msm/dsi: Remove custom DSI config handling
      dt-bindings: display/msm: dsi-controller-main: Fix deprecated compatible
      dt-bindings: display/msm: dsi-controller-main: Add SM6115
      arm64: dts: qcom: sm6115: Use the correct DSI compatible

 .../bindings/display/msm/dsi-controller-main.yaml  |   6 +-
 .../bindings/display/msm/qcom,sm6115-mdss.yaml     |   8 +-
 arch/arm64/boot/dts/qcom/sm6115.dtsi               |   2 +-
 drivers/gpu/drm/msm/dsi/dsi.c                      |   6 +-
 drivers/gpu/drm/msm/dsi/dsi_cfg.c                  | 161 ++++++++-------------
 drivers/gpu/drm/msm/dsi/dsi_cfg.h                  |   6 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c                 |  14 +-
 7 files changed, 82 insertions(+), 121 deletions(-)
---
base-commit: dc837c1a5137a8cf2e9432c1891392b6a66f4d8d
change-id: 20230307-topic-dsi_qcm-5cd03c230f8f

Best regards,

Comments

Marijn Suijten March 13, 2023, 11:37 p.m. UTC | #1
On 2023-03-07 14:01:40, Konrad Dybcio wrote:
> In preparation for supporting multiple sets of possible base registers,
> remove the num_dsi variable. We're comparing the io_start array contents
> with the reg value from the DTS, so it will either match one of the
> expected values or don't match against a zero (which we get from partial

"but won't match against zero" (and drop "either"), that makes this
sentence flow more naturally IMO (otherwise "fail to match against
zero").

> array initialization).
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/dsi/dsi_cfg.c  | 13 -------------
>  drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  1 -
>  drivers/gpu/drm/msm/dsi/dsi_host.c |  2 +-
>  3 files changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 6d21f0b33411..4515f52b407a 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -22,7 +22,6 @@ static const struct msm_dsi_config apq8064_dsi_cfg = {
>  	.bus_clk_names = dsi_v2_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_v2_bus_clk_names),
>  	.io_start = { 0x4700000, 0x5800000 },
> -	.num_dsi = 2,
>  };
>  
>  static const char * const dsi_6g_bus_clk_names[] = {
> @@ -42,7 +41,6 @@ static const struct msm_dsi_config msm8974_apq8084_dsi_cfg = {
>  	.bus_clk_names = dsi_6g_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
>  	.io_start = { 0xfd922800, 0xfd922b00 },
> -	.num_dsi = 2,
>  };
>  
>  static const char * const dsi_8916_bus_clk_names[] = {
> @@ -61,7 +59,6 @@ static const struct msm_dsi_config msm8916_dsi_cfg = {
>  	.bus_clk_names = dsi_8916_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_8916_bus_clk_names),
>  	.io_start = { 0x1a98000 },
> -	.num_dsi = 1,
>  };
>  
>  static const char * const dsi_8976_bus_clk_names[] = {
> @@ -80,7 +77,6 @@ static const struct msm_dsi_config msm8976_dsi_cfg = {
>  	.bus_clk_names = dsi_8976_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_8976_bus_clk_names),
>  	.io_start = { 0x1a94000, 0x1a96000 },
> -	.num_dsi = 2,
>  };
>  
>  static const struct regulator_bulk_data msm8994_dsi_regulators[] = {
> @@ -99,7 +95,6 @@ static const struct msm_dsi_config msm8994_dsi_cfg = {
>  	.bus_clk_names = dsi_6g_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
>  	.io_start = { 0xfd998000, 0xfd9a0000 },
> -	.num_dsi = 2,
>  };
>  
>  static const char * const dsi_8996_bus_clk_names[] = {
> @@ -119,7 +114,6 @@ static const struct msm_dsi_config msm8996_dsi_cfg = {
>  	.bus_clk_names = dsi_8996_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_8996_bus_clk_names),
>  	.io_start = { 0x994000, 0x996000 },
> -	.num_dsi = 2,
>  };
>  
>  static const char * const dsi_msm8998_bus_clk_names[] = {
> @@ -138,7 +132,6 @@ static const struct msm_dsi_config msm8998_dsi_cfg = {
>  	.bus_clk_names = dsi_msm8998_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_msm8998_bus_clk_names),
>  	.io_start = { 0xc994000, 0xc996000 },
> -	.num_dsi = 2,
>  };
>  
>  static const char * const dsi_sdm660_bus_clk_names[] = {
> @@ -156,7 +149,6 @@ static const struct msm_dsi_config sdm660_dsi_cfg = {
>  	.bus_clk_names = dsi_sdm660_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_sdm660_bus_clk_names),
>  	.io_start = { 0xc994000, 0xc996000 },
> -	.num_dsi = 2,
>  };
>  
>  static const char * const dsi_sdm845_bus_clk_names[] = {
> @@ -178,7 +170,6 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
>  	.bus_clk_names = dsi_sdm845_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
>  	.io_start = { 0xae94000, 0xae96000 },
> -	.num_dsi = 2,
>  };
>  
>  static const struct regulator_bulk_data sm8550_dsi_regulators[] = {
> @@ -192,7 +183,6 @@ static const struct msm_dsi_config sm8550_dsi_cfg = {
>  	.bus_clk_names = dsi_sdm845_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
>  	.io_start = { 0xae94000, 0xae96000 },
> -	.num_dsi = 2,
>  };
>  
>  static const struct regulator_bulk_data sc7180_dsi_regulators[] = {
> @@ -206,7 +196,6 @@ static const struct msm_dsi_config sc7180_dsi_cfg = {
>  	.bus_clk_names = dsi_sc7180_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
>  	.io_start = { 0xae94000 },
> -	.num_dsi = 1,
>  };
>  
>  static const char * const dsi_sc7280_bus_clk_names[] = {
> @@ -224,7 +213,6 @@ static const struct msm_dsi_config sc7280_dsi_cfg = {
>  	.bus_clk_names = dsi_sc7280_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_sc7280_bus_clk_names),
>  	.io_start = { 0xae94000, 0xae96000 },
> -	.num_dsi = 2,
>  };
>  
>  static const char * const dsi_qcm2290_bus_clk_names[] = {
> @@ -242,7 +230,6 @@ static const struct msm_dsi_config qcm2290_dsi_cfg = {
>  	.bus_clk_names = dsi_qcm2290_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_qcm2290_bus_clk_names),
>  	.io_start = { 0x5e94000 },
> -	.num_dsi = 1,
>  };
>  
>  static const struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> index 44be4a88aa83..6b6b16c5fd25 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> @@ -39,7 +39,6 @@ struct msm_dsi_config {
>  	const char * const *bus_clk_names;
>  	const int num_bus_clks;
>  	const resource_size_t io_start[DSI_MAX];
> -	const int num_dsi;
>  };
>  
>  struct msm_dsi_host_cfg_ops {
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 18fa30e1e858..9021f0d65515 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1868,7 +1868,7 @@ static int dsi_host_get_id(struct msm_dsi_host *msm_host)
>  	if (!res)
>  		return -EINVAL;
>  
> -	for (i = 0; i < cfg->num_dsi; i++) {
> +	for (i = 0; i < DSI_MAX; i++) {
>  		if (cfg->io_start[i] == res->start)
>  			return i;
>  	}
> 
> -- 
> 2.39.2
>
Marijn Suijten March 13, 2023, 11:51 p.m. UTC | #2
On 2023-03-07 14:01:41, Konrad Dybcio wrote:
> Currently, we allow for MAX_DSI entries in io_start to facilitate for
> MAX_DSI number of DSI hosts at different addresses. The configuration
> is matched against the DSI CTRL hardware revision read back from the
> component. We need a way to resolve situations where multiple SoCs
> with different register maps may use the same version of DSI CTRL. In
> preparation to do so, make msm_dsi_config a 2d array where each entry
> represents a set of configurations adequate for a given SoC.

Note that this code isn't fool-proof against different SoCs sharing the
same DSI host address but for different indices (for example, the
address at variant 0 DSI 0 could be the same as variant 1 DSI 1) and the
matching logic would wrongly return ID 0 instead of 1 for SoC variant 1,
because that's the first matching address it finds.

> This is totally fine to do, as the only differentiating factors
> between same-version-different-SoCs configurations are the number of
> DSI hosts (1 or 2, at least as of today) and the set of base registers.
> The regulator setup is the same, because the DSI hardware is the same,
> regardless of the SoC it was implemented in.
> 
> In addition to that, update the matching logic such that it will loop
> over VARIANTS_MAX variants, making sure they are all taken into account.

"in addition to that" makes it sound like you're doing a separate new
thing in this patch, when the match logic must in fact be updated to
make it compatible with the change described above (as in, it doesn't
compile if you don't account for the extra depth in the array).

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Regardless of the above, I don't think it's a problem right now and I
really like the direction this is headed in: miles better than having a
single distinct SoC with a separate way (compatible) of selecting the
host DSI CTRL, rather than the hw revision readback.

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/dsi/dsi_cfg.c  | 52 ++++++++++++++++++++++++++++----------
>  drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  5 +++-
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++----
>  3 files changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 4515f52b407a..6c192963c100 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -21,7 +21,9 @@ static const struct msm_dsi_config apq8064_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(apq8064_dsi_regulators),
>  	.bus_clk_names = dsi_v2_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_v2_bus_clk_names),
> -	.io_start = { 0x4700000, 0x5800000 },
> +	.io_start = {
> +		{ 0x4700000, 0x5800000 },
> +	},
>  };
>  
>  static const char * const dsi_6g_bus_clk_names[] = {
> @@ -40,7 +42,9 @@ static const struct msm_dsi_config msm8974_apq8084_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(msm8974_apq8084_regulators),
>  	.bus_clk_names = dsi_6g_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
> -	.io_start = { 0xfd922800, 0xfd922b00 },
> +	.io_start = {
> +		{ 0xfd922800, 0xfd922b00 },
> +	},
>  };
>  
>  static const char * const dsi_8916_bus_clk_names[] = {
> @@ -58,7 +62,9 @@ static const struct msm_dsi_config msm8916_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(msm8916_dsi_regulators),
>  	.bus_clk_names = dsi_8916_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_8916_bus_clk_names),
> -	.io_start = { 0x1a98000 },
> +	.io_start = {
> +		{ 0x1a98000 },
> +	},
>  };
>  
>  static const char * const dsi_8976_bus_clk_names[] = {
> @@ -76,7 +82,9 @@ static const struct msm_dsi_config msm8976_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(msm8976_dsi_regulators),
>  	.bus_clk_names = dsi_8976_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_8976_bus_clk_names),
> -	.io_start = { 0x1a94000, 0x1a96000 },
> +	.io_start = {
> +		{ 0x1a94000, 0x1a96000 },
> +	},
>  };
>  
>  static const struct regulator_bulk_data msm8994_dsi_regulators[] = {
> @@ -94,7 +102,9 @@ static const struct msm_dsi_config msm8994_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(msm8994_dsi_regulators),
>  	.bus_clk_names = dsi_6g_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
> -	.io_start = { 0xfd998000, 0xfd9a0000 },
> +	.io_start = {
> +		{ 0xfd998000, 0xfd9a0000 },
> +	},
>  };
>  
>  static const char * const dsi_8996_bus_clk_names[] = {
> @@ -113,7 +123,9 @@ static const struct msm_dsi_config msm8996_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(msm8996_dsi_regulators),
>  	.bus_clk_names = dsi_8996_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_8996_bus_clk_names),
> -	.io_start = { 0x994000, 0x996000 },
> +	.io_start = {
> +		{ 0x994000, 0x996000 },
> +	},
>  };
>  
>  static const char * const dsi_msm8998_bus_clk_names[] = {
> @@ -131,7 +143,9 @@ static const struct msm_dsi_config msm8998_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(msm8998_dsi_regulators),
>  	.bus_clk_names = dsi_msm8998_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_msm8998_bus_clk_names),
> -	.io_start = { 0xc994000, 0xc996000 },
> +	.io_start = {
> +		{ 0xc994000, 0xc996000 },
> +	},
>  };
>  
>  static const char * const dsi_sdm660_bus_clk_names[] = {
> @@ -148,7 +162,9 @@ static const struct msm_dsi_config sdm660_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(sdm660_dsi_regulators),
>  	.bus_clk_names = dsi_sdm660_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_sdm660_bus_clk_names),
> -	.io_start = { 0xc994000, 0xc996000 },
> +	.io_start = {
> +		{ 0xc994000, 0xc996000 },
> +	},
>  };
>  
>  static const char * const dsi_sdm845_bus_clk_names[] = {
> @@ -169,7 +185,9 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(sdm845_dsi_regulators),
>  	.bus_clk_names = dsi_sdm845_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
> -	.io_start = { 0xae94000, 0xae96000 },
> +	.io_start = {
> +		{ 0xae94000, 0xae96000 },
> +	},
>  };
>  
>  static const struct regulator_bulk_data sm8550_dsi_regulators[] = {
> @@ -182,7 +200,9 @@ static const struct msm_dsi_config sm8550_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(sm8550_dsi_regulators),
>  	.bus_clk_names = dsi_sdm845_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
> -	.io_start = { 0xae94000, 0xae96000 },
> +	.io_start = {
> +		{ 0xae94000, 0xae96000 },
> +	},
>  };
>  
>  static const struct regulator_bulk_data sc7180_dsi_regulators[] = {
> @@ -195,7 +215,9 @@ static const struct msm_dsi_config sc7180_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(sc7180_dsi_regulators),
>  	.bus_clk_names = dsi_sc7180_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
> -	.io_start = { 0xae94000 },
> +	.io_start = {
> +		{ 0xae94000 },
> +	},
>  };
>  
>  static const char * const dsi_sc7280_bus_clk_names[] = {
> @@ -212,7 +234,9 @@ static const struct msm_dsi_config sc7280_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(sc7280_dsi_regulators),
>  	.bus_clk_names = dsi_sc7280_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_sc7280_bus_clk_names),
> -	.io_start = { 0xae94000, 0xae96000 },
> +	.io_start = {
> +		{ 0xae94000, 0xae96000 },
> +	},
>  };
>  
>  static const char * const dsi_qcm2290_bus_clk_names[] = {
> @@ -229,7 +253,9 @@ static const struct msm_dsi_config qcm2290_dsi_cfg = {
>  	.num_regulators = ARRAY_SIZE(qcm2290_dsi_cfg_regulators),
>  	.bus_clk_names = dsi_qcm2290_bus_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_qcm2290_bus_clk_names),
> -	.io_start = { 0x5e94000 },
> +	.io_start = {
> +		{ 0x5e94000 },
> +	},
>  };
>  
>  static const struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> index 6b6b16c5fd25..8772a3631ac1 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> @@ -32,13 +32,16 @@
>  
>  #define DSI_6G_REG_SHIFT	4
>  
> +/* Maximum number of configurations matched against the same hw revision */
> +#define VARIANTS_MAX			2
> +
>  struct msm_dsi_config {
>  	u32 io_offset;
>  	const struct regulator_bulk_data *regulator_data;
>  	int num_regulators;
>  	const char * const *bus_clk_names;
>  	const int num_bus_clks;
> -	const resource_size_t io_start[DSI_MAX];
> +	const resource_size_t io_start[VARIANTS_MAX][DSI_MAX];
>  };
>  
>  struct msm_dsi_host_cfg_ops {
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 9021f0d65515..9cfb9e91bfea 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1862,16 +1862,16 @@ static int dsi_host_get_id(struct msm_dsi_host *msm_host)
>  	struct platform_device *pdev = msm_host->pdev;
>  	const struct msm_dsi_config *cfg = msm_host->cfg_hnd->cfg;
>  	struct resource *res;
> -	int i;
> +	int i, j;
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dsi_ctrl");
>  	if (!res)
>  		return -EINVAL;
>  
> -	for (i = 0; i < DSI_MAX; i++) {
> -		if (cfg->io_start[i] == res->start)
> -			return i;
> -	}
> +	for (i = 0; i < VARIANTS_MAX; i++)
> +		for (j = 0; j < DSI_MAX; j++)
> +			if (cfg->io_start[i][j] == res->start)
> +				return j;
>  
>  	return -EINVAL;
>  }
> 
> -- 
> 2.39.2
>
Marijn Suijten March 13, 2023, 11:55 p.m. UTC | #3
On 2023-03-07 14:01:42, Konrad Dybcio wrote:
> Some structs were defined multiple times for no apparent reason.
> Deduplicate them.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Seems a bit inconsistent to name some of these with their DSI host
revision, and keep some named after the SoC.  Also in the name of
msm_dsi_config.  Regardless:

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/dsi/dsi_cfg.c | 77 +++++++++++++--------------------------
>  1 file changed, 26 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 6c192963c100..d39521850018 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -47,41 +47,32 @@ static const struct msm_dsi_config msm8974_apq8084_dsi_cfg = {
>  	},
>  };
>  
> -static const char * const dsi_8916_bus_clk_names[] = {
> +static const char * const dsi_v1_3_1_clk_names[] = {
>  	"mdp_core", "iface", "bus",
>  };
>  
> -static const struct regulator_bulk_data msm8916_dsi_regulators[] = {
> +static const struct regulator_bulk_data dsi_v1_3_1_regulators[] = {
>  	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
>  	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
>  };
>  
>  static const struct msm_dsi_config msm8916_dsi_cfg = {
>  	.io_offset = DSI_6G_REG_SHIFT,
> -	.regulator_data = msm8916_dsi_regulators,
> -	.num_regulators = ARRAY_SIZE(msm8916_dsi_regulators),
> -	.bus_clk_names = dsi_8916_bus_clk_names,
> -	.num_bus_clks = ARRAY_SIZE(dsi_8916_bus_clk_names),
> +	.regulator_data = dsi_v1_3_1_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_v1_3_1_regulators),
> +	.bus_clk_names = dsi_v1_3_1_clk_names,
> +	.num_bus_clks = ARRAY_SIZE(dsi_v1_3_1_clk_names),
>  	.io_start = {
>  		{ 0x1a98000 },
>  	},
>  };
>  
> -static const char * const dsi_8976_bus_clk_names[] = {
> -	"mdp_core", "iface", "bus",
> -};
> -
> -static const struct regulator_bulk_data msm8976_dsi_regulators[] = {
> -	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
> -	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
> -};
> -
>  static const struct msm_dsi_config msm8976_dsi_cfg = {
>  	.io_offset = DSI_6G_REG_SHIFT,
> -	.regulator_data = msm8976_dsi_regulators,
> -	.num_regulators = ARRAY_SIZE(msm8976_dsi_regulators),
> -	.bus_clk_names = dsi_8976_bus_clk_names,
> -	.num_bus_clks = ARRAY_SIZE(dsi_8976_bus_clk_names),
> +	.regulator_data = dsi_v1_3_1_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_v1_3_1_regulators),
> +	.bus_clk_names = dsi_v1_3_1_clk_names,
> +	.num_bus_clks = ARRAY_SIZE(dsi_v1_3_1_clk_names),
>  	.io_start = {
>  		{ 0x1a94000, 0x1a96000 },
>  	},
> @@ -107,10 +98,6 @@ static const struct msm_dsi_config msm8994_dsi_cfg = {
>  	},
>  };
>  
> -static const char * const dsi_8996_bus_clk_names[] = {
> -	"mdp_core", "iface", "bus", "core_mmss",
> -};
> -
>  static const struct regulator_bulk_data msm8996_dsi_regulators[] = {
>  	{ .supply = "vdda", .init_load_uA = 18160 },	/* 1.25 V */
>  	{ .supply = "vcca", .init_load_uA = 17000 },	/* 0.925 V */
> @@ -121,8 +108,8 @@ static const struct msm_dsi_config msm8996_dsi_cfg = {
>  	.io_offset = DSI_6G_REG_SHIFT,
>  	.regulator_data = msm8996_dsi_regulators,
>  	.num_regulators = ARRAY_SIZE(msm8996_dsi_regulators),
> -	.bus_clk_names = dsi_8996_bus_clk_names,
> -	.num_bus_clks = ARRAY_SIZE(dsi_8996_bus_clk_names),
> +	.bus_clk_names = dsi_6g_bus_clk_names,
> +	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
>  	.io_start = {
>  		{ 0x994000, 0x996000 },
>  	},
> @@ -167,24 +154,20 @@ static const struct msm_dsi_config sdm660_dsi_cfg = {
>  	},
>  };
>  
> -static const char * const dsi_sdm845_bus_clk_names[] = {
> +static const char * const dsi_v2_4_clk_names[] = {
>  	"iface", "bus",
>  };
>  
> -static const char * const dsi_sc7180_bus_clk_names[] = {
> -	"iface", "bus",
> -};
> -
> -static const struct regulator_bulk_data sdm845_dsi_regulators[] = {
> +static const struct regulator_bulk_data dsi_v2_4_regulators[] = {
>  	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
>  };
>  
>  static const struct msm_dsi_config sdm845_dsi_cfg = {
>  	.io_offset = DSI_6G_REG_SHIFT,
> -	.regulator_data = sdm845_dsi_regulators,
> -	.num_regulators = ARRAY_SIZE(sdm845_dsi_regulators),
> -	.bus_clk_names = dsi_sdm845_bus_clk_names,
> -	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
> +	.regulator_data = dsi_v2_4_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_v2_4_regulators),
> +	.bus_clk_names = dsi_v2_4_clk_names,
> +	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>  	.io_start = {
>  		{ 0xae94000, 0xae96000 },
>  	},
> @@ -198,32 +181,24 @@ static const struct msm_dsi_config sm8550_dsi_cfg = {
>  	.io_offset = DSI_6G_REG_SHIFT,
>  	.regulator_data = sm8550_dsi_regulators,
>  	.num_regulators = ARRAY_SIZE(sm8550_dsi_regulators),
> -	.bus_clk_names = dsi_sdm845_bus_clk_names,
> -	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
> +	.bus_clk_names = dsi_v2_4_clk_names,
> +	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>  	.io_start = {
>  		{ 0xae94000, 0xae96000 },
>  	},
>  };
>  
> -static const struct regulator_bulk_data sc7180_dsi_regulators[] = {
> -	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
> -};
> -
>  static const struct msm_dsi_config sc7180_dsi_cfg = {
>  	.io_offset = DSI_6G_REG_SHIFT,
> -	.regulator_data = sc7180_dsi_regulators,
> -	.num_regulators = ARRAY_SIZE(sc7180_dsi_regulators),
> -	.bus_clk_names = dsi_sc7180_bus_clk_names,
> -	.num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
> +	.regulator_data = dsi_v2_4_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_v2_4_regulators),
> +	.bus_clk_names = dsi_v2_4_clk_names,
> +	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>  	.io_start = {
>  		{ 0xae94000 },
>  	},
>  };
>  
> -static const char * const dsi_sc7280_bus_clk_names[] = {
> -	"iface", "bus",
> -};
> -
>  static const struct regulator_bulk_data sc7280_dsi_regulators[] = {
>  	{ .supply = "vdda", .init_load_uA = 8350 },	/* 1.2 V */
>  };
> @@ -232,8 +207,8 @@ static const struct msm_dsi_config sc7280_dsi_cfg = {
>  	.io_offset = DSI_6G_REG_SHIFT,
>  	.regulator_data = sc7280_dsi_regulators,
>  	.num_regulators = ARRAY_SIZE(sc7280_dsi_regulators),
> -	.bus_clk_names = dsi_sc7280_bus_clk_names,
> -	.num_bus_clks = ARRAY_SIZE(dsi_sc7280_bus_clk_names),
> +	.bus_clk_names = dsi_v2_4_clk_names,
> +	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>  	.io_start = {
>  		{ 0xae94000, 0xae96000 },
>  	},
> 
> -- 
> 2.39.2
>
Marijn Suijten March 13, 2023, 11:58 p.m. UTC | #4
On 2023-03-07 14:01:43, Konrad Dybcio wrote:
> The configs are identical, other than the number of *maximum* DSI
> hosts allowed. This isn't an issue, unless somebody deliberately
> tries to access the inexistent host by adding a dt node for it.
> 
> Remove the SC7180 struct and point the hw revision match to the
> SDM845's one. On a note, this could have been done back when
> 7180 support was introduced.

Maybe differences were expected, but then sdm845_dsi_cfg is used on many
other revisions.

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/dsi/dsi_cfg.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index d39521850018..6d4b2ce4b918 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -169,7 +169,7 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
>  	.bus_clk_names = dsi_v2_4_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>  	.io_start = {
> -		{ 0xae94000, 0xae96000 },
> +		{ 0xae94000, 0xae96000 }, /* SDM845 / SDM670 / SC7180 */
>  	},
>  };
>  
> @@ -188,17 +188,6 @@ static const struct msm_dsi_config sm8550_dsi_cfg = {
>  	},
>  };
>  
> -static const struct msm_dsi_config sc7180_dsi_cfg = {
> -	.io_offset = DSI_6G_REG_SHIFT,
> -	.regulator_data = dsi_v2_4_regulators,
> -	.num_regulators = ARRAY_SIZE(dsi_v2_4_regulators),
> -	.bus_clk_names = dsi_v2_4_clk_names,
> -	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
> -	.io_start = {
> -		{ 0xae94000 },
> -	},
> -};
> -
>  static const struct regulator_bulk_data sc7280_dsi_regulators[] = {
>  	{ .supply = "vdda", .init_load_uA = 8350 },	/* 1.2 V */
>  };
> @@ -299,7 +288,7 @@ static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
>  	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_4_0,
>  		&sdm845_dsi_cfg, &msm_dsi_6g_v2_host_ops},
>  	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_4_1,
> -		&sc7180_dsi_cfg, &msm_dsi_6g_v2_host_ops},
> +		&sdm845_dsi_cfg, &msm_dsi_6g_v2_host_ops},
>  	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_5_0,
>  		&sc7280_dsi_cfg, &msm_dsi_6g_v2_host_ops},
>  	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_6_0,
> 
> -- 
> 2.39.2
>
Marijn Suijten March 14, 2023, 12:03 a.m. UTC | #5
On 2023-03-07 14:01:44, Konrad Dybcio wrote:
> Now that the logic can handle multiple sets of registers, move
> the QCM2290 to the common logic and mark it deprecated. This allows us
> to remove a couple of structs, saving some memory.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.c     |  4 +++-
>  drivers/gpu/drm/msm/dsi/dsi_cfg.c | 28 ++--------------------------
>  2 files changed, 5 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index 31fdee2052be..90d43628b22b 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -174,7 +174,9 @@ static int dsi_dev_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id dt_match[] = {
>  	{ .compatible = "qcom,mdss-dsi-ctrl", .data = NULL /* autodetect cfg */ },
> -	{ .compatible = "qcom,dsi-ctrl-6g-qcm2290", .data = &qcm2290_dsi_cfg_handler },
> +
> +	/* Deprecated, don't use */
> +	{ .compatible = "qcom,dsi-ctrl-6g-qcm2290", .data = NULL },
>  	{}
>  };
>  
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 6d4b2ce4b918..29ccd755cc2e 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -169,7 +169,8 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
>  	.bus_clk_names = dsi_v2_4_clk_names,
>  	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>  	.io_start = {
> -		{ 0xae94000, 0xae96000 }, /* SDM845 / SDM670 / SC7180 */
> +		{ 0xae94000, 0xae96000 }, /* SDM845 / SDM670 */
> +		{ 0x5e94000 }, /* QCM2290 / SM6115 / SM6125 / SM6375 */
>  	},
>  };
>  
> @@ -203,25 +204,6 @@ static const struct msm_dsi_config sc7280_dsi_cfg = {
>  	},
>  };
>  
> -static const char * const dsi_qcm2290_bus_clk_names[] = {
> -	"iface", "bus",
> -};
> -
> -static const struct regulator_bulk_data qcm2290_dsi_cfg_regulators[] = {
> -	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
> -};

These two consts should really have already been deleted as part of
04/10: drm/msm/dsi: dsi_cfg: Deduplicate identical structs.

> -static const struct msm_dsi_config qcm2290_dsi_cfg = {
> -	.io_offset = DSI_6G_REG_SHIFT,
> -	.regulator_data = qcm2290_dsi_cfg_regulators,
> -	.num_regulators = ARRAY_SIZE(qcm2290_dsi_cfg_regulators),
> -	.bus_clk_names = dsi_qcm2290_bus_clk_names,
> -	.num_bus_clks = ARRAY_SIZE(dsi_qcm2290_bus_clk_names),
> -	.io_start = {
> -		{ 0x5e94000 },
> -	},
> -};
> -
>  static const struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
>  	.link_clk_set_rate = dsi_link_clk_set_rate_v2,
>  	.link_clk_enable = dsi_link_clk_enable_v2,
> @@ -312,9 +294,3 @@ const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
>  
>  	return cfg_hnd;
>  }
> -
> -/*  Non autodetect configs */
> -const struct msm_dsi_cfg_handler qcm2290_dsi_cfg_handler = {
> -	.cfg = &qcm2290_dsi_cfg,
> -	.ops = &msm_dsi_6g_v2_host_ops,
> -};

And how do you think dsi.c is able to reference this... don't forget to
remove it from dsi_cfg.h in v4.  In fact, if you look at how this was
implemented you should also be able to remove #include "dsi_cfg.h" from
dsi.c.  A clean revert of that patch would be nice, or just use it as
reference to find the remnants:

https://lore.kernel.org/all/1644853060-12222-2-git-send-email-loic.poulain@linaro.org/

- Marijn
Marijn Suijten March 14, 2023, 12:07 a.m. UTC | #6
On 2023-03-07 14:01:45, Konrad Dybcio wrote:
> Now that the only user is handled by common code, remove the option to
> specify custom handlers through match data.
> 
> This is effectively a revert of commit:
> 5ae15e76271 ("drm/msm/dsi: Allow to specify dsi config as pdata")

Would it also be worth to mention something along these lines in the
previous patch, but for ee1f09678f14 ("drm/msm/dsi: Add support for
qcm2290 dsi controller")?

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/dsi/dsi.c      | 4 ++--
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ----
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index 90d43628b22b..e0b911af618d 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -173,10 +173,10 @@ static int dsi_dev_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id dt_match[] = {
> -	{ .compatible = "qcom,mdss-dsi-ctrl", .data = NULL /* autodetect cfg */ },
> +	{ .compatible = "qcom,mdss-dsi-ctrl" },
>  
>  	/* Deprecated, don't use */
> -	{ .compatible = "qcom,dsi-ctrl-6g-qcm2290", .data = NULL },
> +	{ .compatible = "qcom,dsi-ctrl-6g-qcm2290" },
>  	{}
>  };
>  
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 9cfb9e91bfea..961689a255c4 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -214,10 +214,6 @@ static const struct msm_dsi_cfg_handler *dsi_get_config(
>  	int ret;
>  	u32 major = 0, minor = 0;
>  
> -	cfg_hnd = device_get_match_data(dev);
> -	if (cfg_hnd)
> -		return cfg_hnd;
> -
>  	ahb_clk = msm_clk_get(msm_host->pdev, "iface");
>  	if (IS_ERR(ahb_clk)) {
>  		pr_err("%s: cannot get interface clock\n", __func__);
> 
> -- 
> 2.39.2
>
Marijn Suijten March 14, 2023, 12:19 a.m. UTC | #7
On 2023-03-07 14:01:48, Konrad Dybcio wrote:
> Use the non-deprecated, SoC-specific DSI compatible.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> index 4d6ec815b78b..26e2c7919961 100644
> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> @@ -1218,7 +1218,7 @@ opp-384000000 {
>  			};
>  
>  			mdss_dsi0: dsi@5e94000 {
> -				compatible = "qcom,dsi-ctrl-6g-qcm2290";
> +				compatible = "qcom,sm6115-dsi-ctrl", "qcom,mdss-dsi-ctrl";

This is what the example should look like in qcom,sm6115-mdss.yaml, too.

- Marijn

>  				reg = <0x0 0x05e94000 0x0 0x400>;
>  				reg-names = "dsi_ctrl";
>  
> 
> -- 
> 2.39.2
>
Konrad Dybcio March 14, 2023, 10:42 a.m. UTC | #8
On 14.03.2023 01:07, Marijn Suijten wrote:
> On 2023-03-07 14:01:45, Konrad Dybcio wrote:
>> Now that the only user is handled by common code, remove the option to
>> specify custom handlers through match data.
>>
>> This is effectively a revert of commit:
>> 5ae15e76271 ("drm/msm/dsi: Allow to specify dsi config as pdata")
> 
> Would it also be worth to mention something along these lines in the
> previous patch, but for ee1f09678f14 ("drm/msm/dsi: Add support for
> qcm2290 dsi controller")?
50/50, it wouldn't hurt but it doesn't sound groundbreaking to skip it..

Konrad
> 
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
>> ---
>>  drivers/gpu/drm/msm/dsi/dsi.c      | 4 ++--
>>  drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ----
>>  2 files changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
>> index 90d43628b22b..e0b911af618d 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>> @@ -173,10 +173,10 @@ static int dsi_dev_remove(struct platform_device *pdev)
>>  }
>>  
>>  static const struct of_device_id dt_match[] = {
>> -	{ .compatible = "qcom,mdss-dsi-ctrl", .data = NULL /* autodetect cfg */ },
>> +	{ .compatible = "qcom,mdss-dsi-ctrl" },
>>  
>>  	/* Deprecated, don't use */
>> -	{ .compatible = "qcom,dsi-ctrl-6g-qcm2290", .data = NULL },
>> +	{ .compatible = "qcom,dsi-ctrl-6g-qcm2290" },
>>  	{}
>>  };
>>  
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 9cfb9e91bfea..961689a255c4 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -214,10 +214,6 @@ static const struct msm_dsi_cfg_handler *dsi_get_config(
>>  	int ret;
>>  	u32 major = 0, minor = 0;
>>  
>> -	cfg_hnd = device_get_match_data(dev);
>> -	if (cfg_hnd)
>> -		return cfg_hnd;
>> -
>>  	ahb_clk = msm_clk_get(msm_host->pdev, "iface");
>>  	if (IS_ERR(ahb_clk)) {
>>  		pr_err("%s: cannot get interface clock\n", __func__);
>>
>> -- 
>> 2.39.2
>>
Konrad Dybcio March 14, 2023, 11:59 a.m. UTC | #9
On 14.03.2023 00:51, Marijn Suijten wrote:
> On 2023-03-07 14:01:41, Konrad Dybcio wrote:
>> Currently, we allow for MAX_DSI entries in io_start to facilitate for
>> MAX_DSI number of DSI hosts at different addresses. The configuration
>> is matched against the DSI CTRL hardware revision read back from the
>> component. We need a way to resolve situations where multiple SoCs
>> with different register maps may use the same version of DSI CTRL. In
>> preparation to do so, make msm_dsi_config a 2d array where each entry
>> represents a set of configurations adequate for a given SoC.
> 
> Note that this code isn't fool-proof against different SoCs sharing the
> same DSI host address but for different indices (for example, the
> address at variant 0 DSI 0 could be the same as variant 1 DSI 1) and the
> matching logic would wrongly return ID 0 instead of 1 for SoC variant 1,
> because that's the first matching address it finds.
I don't think we've had that happen yet, but if it ever does, that's out
of scope of this patchset.

> 
>> This is totally fine to do, as the only differentiating factors
>> between same-version-different-SoCs configurations are the number of
>> DSI hosts (1 or 2, at least as of today) and the set of base registers.
>> The regulator setup is the same, because the DSI hardware is the same,
>> regardless of the SoC it was implemented in.
>>
>> In addition to that, update the matching logic such that it will loop
>> over VARIANTS_MAX variants, making sure they are all taken into account.
> 
> "in addition to that" makes it sound like you're doing a separate new
> thing in this patch, when the match logic must in fact be updated to
> make it compatible with the change described above (as in, it doesn't
> compile if you don't account for the extra depth in the array).
I really think you're nitpicking here..

> 
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> Regardless of the above, I don't think it's a problem right now and I
> really like the direction this is headed in: miles better than having a
> single distinct SoC with a separate way (compatible) of selecting the
> host DSI CTRL, rather than the hw revision readback.
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
Thanks!

Konrad
> 
>> ---
>>  drivers/gpu/drm/msm/dsi/dsi_cfg.c  | 52 ++++++++++++++++++++++++++++----------
>>  drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  5 +++-
>>  drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++----
>>  3 files changed, 48 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> index 4515f52b407a..6c192963c100 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> @@ -21,7 +21,9 @@ static const struct msm_dsi_config apq8064_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(apq8064_dsi_regulators),
>>  	.bus_clk_names = dsi_v2_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_v2_bus_clk_names),
>> -	.io_start = { 0x4700000, 0x5800000 },
>> +	.io_start = {
>> +		{ 0x4700000, 0x5800000 },
>> +	},
>>  };
>>  
>>  static const char * const dsi_6g_bus_clk_names[] = {
>> @@ -40,7 +42,9 @@ static const struct msm_dsi_config msm8974_apq8084_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(msm8974_apq8084_regulators),
>>  	.bus_clk_names = dsi_6g_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
>> -	.io_start = { 0xfd922800, 0xfd922b00 },
>> +	.io_start = {
>> +		{ 0xfd922800, 0xfd922b00 },
>> +	},
>>  };
>>  
>>  static const char * const dsi_8916_bus_clk_names[] = {
>> @@ -58,7 +62,9 @@ static const struct msm_dsi_config msm8916_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(msm8916_dsi_regulators),
>>  	.bus_clk_names = dsi_8916_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_8916_bus_clk_names),
>> -	.io_start = { 0x1a98000 },
>> +	.io_start = {
>> +		{ 0x1a98000 },
>> +	},
>>  };
>>  
>>  static const char * const dsi_8976_bus_clk_names[] = {
>> @@ -76,7 +82,9 @@ static const struct msm_dsi_config msm8976_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(msm8976_dsi_regulators),
>>  	.bus_clk_names = dsi_8976_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_8976_bus_clk_names),
>> -	.io_start = { 0x1a94000, 0x1a96000 },
>> +	.io_start = {
>> +		{ 0x1a94000, 0x1a96000 },
>> +	},
>>  };
>>  
>>  static const struct regulator_bulk_data msm8994_dsi_regulators[] = {
>> @@ -94,7 +102,9 @@ static const struct msm_dsi_config msm8994_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(msm8994_dsi_regulators),
>>  	.bus_clk_names = dsi_6g_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
>> -	.io_start = { 0xfd998000, 0xfd9a0000 },
>> +	.io_start = {
>> +		{ 0xfd998000, 0xfd9a0000 },
>> +	},
>>  };
>>  
>>  static const char * const dsi_8996_bus_clk_names[] = {
>> @@ -113,7 +123,9 @@ static const struct msm_dsi_config msm8996_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(msm8996_dsi_regulators),
>>  	.bus_clk_names = dsi_8996_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_8996_bus_clk_names),
>> -	.io_start = { 0x994000, 0x996000 },
>> +	.io_start = {
>> +		{ 0x994000, 0x996000 },
>> +	},
>>  };
>>  
>>  static const char * const dsi_msm8998_bus_clk_names[] = {
>> @@ -131,7 +143,9 @@ static const struct msm_dsi_config msm8998_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(msm8998_dsi_regulators),
>>  	.bus_clk_names = dsi_msm8998_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_msm8998_bus_clk_names),
>> -	.io_start = { 0xc994000, 0xc996000 },
>> +	.io_start = {
>> +		{ 0xc994000, 0xc996000 },
>> +	},
>>  };
>>  
>>  static const char * const dsi_sdm660_bus_clk_names[] = {
>> @@ -148,7 +162,9 @@ static const struct msm_dsi_config sdm660_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(sdm660_dsi_regulators),
>>  	.bus_clk_names = dsi_sdm660_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_sdm660_bus_clk_names),
>> -	.io_start = { 0xc994000, 0xc996000 },
>> +	.io_start = {
>> +		{ 0xc994000, 0xc996000 },
>> +	},
>>  };
>>  
>>  static const char * const dsi_sdm845_bus_clk_names[] = {
>> @@ -169,7 +185,9 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(sdm845_dsi_regulators),
>>  	.bus_clk_names = dsi_sdm845_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
>> -	.io_start = { 0xae94000, 0xae96000 },
>> +	.io_start = {
>> +		{ 0xae94000, 0xae96000 },
>> +	},
>>  };
>>  
>>  static const struct regulator_bulk_data sm8550_dsi_regulators[] = {
>> @@ -182,7 +200,9 @@ static const struct msm_dsi_config sm8550_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(sm8550_dsi_regulators),
>>  	.bus_clk_names = dsi_sdm845_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
>> -	.io_start = { 0xae94000, 0xae96000 },
>> +	.io_start = {
>> +		{ 0xae94000, 0xae96000 },
>> +	},
>>  };
>>  
>>  static const struct regulator_bulk_data sc7180_dsi_regulators[] = {
>> @@ -195,7 +215,9 @@ static const struct msm_dsi_config sc7180_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(sc7180_dsi_regulators),
>>  	.bus_clk_names = dsi_sc7180_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
>> -	.io_start = { 0xae94000 },
>> +	.io_start = {
>> +		{ 0xae94000 },
>> +	},
>>  };
>>  
>>  static const char * const dsi_sc7280_bus_clk_names[] = {
>> @@ -212,7 +234,9 @@ static const struct msm_dsi_config sc7280_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(sc7280_dsi_regulators),
>>  	.bus_clk_names = dsi_sc7280_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_sc7280_bus_clk_names),
>> -	.io_start = { 0xae94000, 0xae96000 },
>> +	.io_start = {
>> +		{ 0xae94000, 0xae96000 },
>> +	},
>>  };
>>  
>>  static const char * const dsi_qcm2290_bus_clk_names[] = {
>> @@ -229,7 +253,9 @@ static const struct msm_dsi_config qcm2290_dsi_cfg = {
>>  	.num_regulators = ARRAY_SIZE(qcm2290_dsi_cfg_regulators),
>>  	.bus_clk_names = dsi_qcm2290_bus_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_qcm2290_bus_clk_names),
>> -	.io_start = { 0x5e94000 },
>> +	.io_start = {
>> +		{ 0x5e94000 },
>> +	},
>>  };
>>  
>>  static const struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> index 6b6b16c5fd25..8772a3631ac1 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> @@ -32,13 +32,16 @@
>>  
>>  #define DSI_6G_REG_SHIFT	4
>>  
>> +/* Maximum number of configurations matched against the same hw revision */
>> +#define VARIANTS_MAX			2
>> +
>>  struct msm_dsi_config {
>>  	u32 io_offset;
>>  	const struct regulator_bulk_data *regulator_data;
>>  	int num_regulators;
>>  	const char * const *bus_clk_names;
>>  	const int num_bus_clks;
>> -	const resource_size_t io_start[DSI_MAX];
>> +	const resource_size_t io_start[VARIANTS_MAX][DSI_MAX];
>>  };
>>  
>>  struct msm_dsi_host_cfg_ops {
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 9021f0d65515..9cfb9e91bfea 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -1862,16 +1862,16 @@ static int dsi_host_get_id(struct msm_dsi_host *msm_host)
>>  	struct platform_device *pdev = msm_host->pdev;
>>  	const struct msm_dsi_config *cfg = msm_host->cfg_hnd->cfg;
>>  	struct resource *res;
>> -	int i;
>> +	int i, j;
>>  
>>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dsi_ctrl");
>>  	if (!res)
>>  		return -EINVAL;
>>  
>> -	for (i = 0; i < DSI_MAX; i++) {
>> -		if (cfg->io_start[i] == res->start)
>> -			return i;
>> -	}
>> +	for (i = 0; i < VARIANTS_MAX; i++)
>> +		for (j = 0; j < DSI_MAX; j++)
>> +			if (cfg->io_start[i][j] == res->start)
>> +				return j;
>>  
>>  	return -EINVAL;
>>  }
>>
>> -- 
>> 2.39.2
>>
Konrad Dybcio March 14, 2023, 12:01 p.m. UTC | #10
On 14.03.2023 00:55, Marijn Suijten wrote:
> On 2023-03-07 14:01:42, Konrad Dybcio wrote:
>> Some structs were defined multiple times for no apparent reason.
>> Deduplicate them.
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> Seems a bit inconsistent to name some of these with their DSI host
> revision, and keep some named after the SoC.  Also in the name of
> msm_dsi_config.  Regardless:
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
I think it's a good indicator of whether it's shared or used
by just one platform. Truth be told, some of the entries *are*
actually SoC-specific (think 8996 and its magic MMAGIC additions)

Konrad

> 
>> ---
>>  drivers/gpu/drm/msm/dsi/dsi_cfg.c | 77 +++++++++++++--------------------------
>>  1 file changed, 26 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> index 6c192963c100..d39521850018 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> @@ -47,41 +47,32 @@ static const struct msm_dsi_config msm8974_apq8084_dsi_cfg = {
>>  	},
>>  };
>>  
>> -static const char * const dsi_8916_bus_clk_names[] = {
>> +static const char * const dsi_v1_3_1_clk_names[] = {
>>  	"mdp_core", "iface", "bus",
>>  };
>>  
>> -static const struct regulator_bulk_data msm8916_dsi_regulators[] = {
>> +static const struct regulator_bulk_data dsi_v1_3_1_regulators[] = {
>>  	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
>>  	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
>>  };
>>  
>>  static const struct msm_dsi_config msm8916_dsi_cfg = {
>>  	.io_offset = DSI_6G_REG_SHIFT,
>> -	.regulator_data = msm8916_dsi_regulators,
>> -	.num_regulators = ARRAY_SIZE(msm8916_dsi_regulators),
>> -	.bus_clk_names = dsi_8916_bus_clk_names,
>> -	.num_bus_clks = ARRAY_SIZE(dsi_8916_bus_clk_names),
>> +	.regulator_data = dsi_v1_3_1_regulators,
>> +	.num_regulators = ARRAY_SIZE(dsi_v1_3_1_regulators),
>> +	.bus_clk_names = dsi_v1_3_1_clk_names,
>> +	.num_bus_clks = ARRAY_SIZE(dsi_v1_3_1_clk_names),
>>  	.io_start = {
>>  		{ 0x1a98000 },
>>  	},
>>  };
>>  
>> -static const char * const dsi_8976_bus_clk_names[] = {
>> -	"mdp_core", "iface", "bus",
>> -};
>> -
>> -static const struct regulator_bulk_data msm8976_dsi_regulators[] = {
>> -	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
>> -	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
>> -};
>> -
>>  static const struct msm_dsi_config msm8976_dsi_cfg = {
>>  	.io_offset = DSI_6G_REG_SHIFT,
>> -	.regulator_data = msm8976_dsi_regulators,
>> -	.num_regulators = ARRAY_SIZE(msm8976_dsi_regulators),
>> -	.bus_clk_names = dsi_8976_bus_clk_names,
>> -	.num_bus_clks = ARRAY_SIZE(dsi_8976_bus_clk_names),
>> +	.regulator_data = dsi_v1_3_1_regulators,
>> +	.num_regulators = ARRAY_SIZE(dsi_v1_3_1_regulators),
>> +	.bus_clk_names = dsi_v1_3_1_clk_names,
>> +	.num_bus_clks = ARRAY_SIZE(dsi_v1_3_1_clk_names),
>>  	.io_start = {
>>  		{ 0x1a94000, 0x1a96000 },
>>  	},
>> @@ -107,10 +98,6 @@ static const struct msm_dsi_config msm8994_dsi_cfg = {
>>  	},
>>  };
>>  
>> -static const char * const dsi_8996_bus_clk_names[] = {
>> -	"mdp_core", "iface", "bus", "core_mmss",
>> -};
>> -
>>  static const struct regulator_bulk_data msm8996_dsi_regulators[] = {
>>  	{ .supply = "vdda", .init_load_uA = 18160 },	/* 1.25 V */
>>  	{ .supply = "vcca", .init_load_uA = 17000 },	/* 0.925 V */
>> @@ -121,8 +108,8 @@ static const struct msm_dsi_config msm8996_dsi_cfg = {
>>  	.io_offset = DSI_6G_REG_SHIFT,
>>  	.regulator_data = msm8996_dsi_regulators,
>>  	.num_regulators = ARRAY_SIZE(msm8996_dsi_regulators),
>> -	.bus_clk_names = dsi_8996_bus_clk_names,
>> -	.num_bus_clks = ARRAY_SIZE(dsi_8996_bus_clk_names),
>> +	.bus_clk_names = dsi_6g_bus_clk_names,
>> +	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
>>  	.io_start = {
>>  		{ 0x994000, 0x996000 },
>>  	},
>> @@ -167,24 +154,20 @@ static const struct msm_dsi_config sdm660_dsi_cfg = {
>>  	},
>>  };
>>  
>> -static const char * const dsi_sdm845_bus_clk_names[] = {
>> +static const char * const dsi_v2_4_clk_names[] = {
>>  	"iface", "bus",
>>  };
>>  
>> -static const char * const dsi_sc7180_bus_clk_names[] = {
>> -	"iface", "bus",
>> -};
>> -
>> -static const struct regulator_bulk_data sdm845_dsi_regulators[] = {
>> +static const struct regulator_bulk_data dsi_v2_4_regulators[] = {
>>  	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
>>  };
>>  
>>  static const struct msm_dsi_config sdm845_dsi_cfg = {
>>  	.io_offset = DSI_6G_REG_SHIFT,
>> -	.regulator_data = sdm845_dsi_regulators,
>> -	.num_regulators = ARRAY_SIZE(sdm845_dsi_regulators),
>> -	.bus_clk_names = dsi_sdm845_bus_clk_names,
>> -	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
>> +	.regulator_data = dsi_v2_4_regulators,
>> +	.num_regulators = ARRAY_SIZE(dsi_v2_4_regulators),
>> +	.bus_clk_names = dsi_v2_4_clk_names,
>> +	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>>  	.io_start = {
>>  		{ 0xae94000, 0xae96000 },
>>  	},
>> @@ -198,32 +181,24 @@ static const struct msm_dsi_config sm8550_dsi_cfg = {
>>  	.io_offset = DSI_6G_REG_SHIFT,
>>  	.regulator_data = sm8550_dsi_regulators,
>>  	.num_regulators = ARRAY_SIZE(sm8550_dsi_regulators),
>> -	.bus_clk_names = dsi_sdm845_bus_clk_names,
>> -	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
>> +	.bus_clk_names = dsi_v2_4_clk_names,
>> +	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>>  	.io_start = {
>>  		{ 0xae94000, 0xae96000 },
>>  	},
>>  };
>>  
>> -static const struct regulator_bulk_data sc7180_dsi_regulators[] = {
>> -	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
>> -};
>> -
>>  static const struct msm_dsi_config sc7180_dsi_cfg = {
>>  	.io_offset = DSI_6G_REG_SHIFT,
>> -	.regulator_data = sc7180_dsi_regulators,
>> -	.num_regulators = ARRAY_SIZE(sc7180_dsi_regulators),
>> -	.bus_clk_names = dsi_sc7180_bus_clk_names,
>> -	.num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
>> +	.regulator_data = dsi_v2_4_regulators,
>> +	.num_regulators = ARRAY_SIZE(dsi_v2_4_regulators),
>> +	.bus_clk_names = dsi_v2_4_clk_names,
>> +	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>>  	.io_start = {
>>  		{ 0xae94000 },
>>  	},
>>  };
>>  
>> -static const char * const dsi_sc7280_bus_clk_names[] = {
>> -	"iface", "bus",
>> -};
>> -
>>  static const struct regulator_bulk_data sc7280_dsi_regulators[] = {
>>  	{ .supply = "vdda", .init_load_uA = 8350 },	/* 1.2 V */
>>  };
>> @@ -232,8 +207,8 @@ static const struct msm_dsi_config sc7280_dsi_cfg = {
>>  	.io_offset = DSI_6G_REG_SHIFT,
>>  	.regulator_data = sc7280_dsi_regulators,
>>  	.num_regulators = ARRAY_SIZE(sc7280_dsi_regulators),
>> -	.bus_clk_names = dsi_sc7280_bus_clk_names,
>> -	.num_bus_clks = ARRAY_SIZE(dsi_sc7280_bus_clk_names),
>> +	.bus_clk_names = dsi_v2_4_clk_names,
>> +	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>>  	.io_start = {
>>  		{ 0xae94000, 0xae96000 },
>>  	},
>>
>> -- 
>> 2.39.2
>>
Konrad Dybcio March 14, 2023, 12:06 p.m. UTC | #11
On 14.03.2023 01:03, Marijn Suijten wrote:
> On 2023-03-07 14:01:44, Konrad Dybcio wrote:
>> Now that the logic can handle multiple sets of registers, move
>> the QCM2290 to the common logic and mark it deprecated. This allows us
>> to remove a couple of structs, saving some memory.
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/gpu/drm/msm/dsi/dsi.c     |  4 +++-
>>  drivers/gpu/drm/msm/dsi/dsi_cfg.c | 28 ++--------------------------
>>  2 files changed, 5 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
>> index 31fdee2052be..90d43628b22b 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>> @@ -174,7 +174,9 @@ static int dsi_dev_remove(struct platform_device *pdev)
>>  
>>  static const struct of_device_id dt_match[] = {
>>  	{ .compatible = "qcom,mdss-dsi-ctrl", .data = NULL /* autodetect cfg */ },
>> -	{ .compatible = "qcom,dsi-ctrl-6g-qcm2290", .data = &qcm2290_dsi_cfg_handler },
>> +
>> +	/* Deprecated, don't use */
>> +	{ .compatible = "qcom,dsi-ctrl-6g-qcm2290", .data = NULL },
>>  	{}
>>  };
>>  
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> index 6d4b2ce4b918..29ccd755cc2e 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> @@ -169,7 +169,8 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
>>  	.bus_clk_names = dsi_v2_4_clk_names,
>>  	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>>  	.io_start = {
>> -		{ 0xae94000, 0xae96000 }, /* SDM845 / SDM670 / SC7180 */
>> +		{ 0xae94000, 0xae96000 }, /* SDM845 / SDM670 */
>> +		{ 0x5e94000 }, /* QCM2290 / SM6115 / SM6125 / SM6375 */
>>  	},
>>  };
>>  
>> @@ -203,25 +204,6 @@ static const struct msm_dsi_config sc7280_dsi_cfg = {
>>  	},
>>  };
>>  
>> -static const char * const dsi_qcm2290_bus_clk_names[] = {
>> -	"iface", "bus",
>> -};
>> -
>> -static const struct regulator_bulk_data qcm2290_dsi_cfg_regulators[] = {
>> -	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
>> -};
> 
> These two consts should really have already been deleted as part of
> 04/10: drm/msm/dsi: dsi_cfg: Deduplicate identical structs.
Right, will fix

> 
>> -static const struct msm_dsi_config qcm2290_dsi_cfg = {
>> -	.io_offset = DSI_6G_REG_SHIFT,
>> -	.regulator_data = qcm2290_dsi_cfg_regulators,
>> -	.num_regulators = ARRAY_SIZE(qcm2290_dsi_cfg_regulators),
>> -	.bus_clk_names = dsi_qcm2290_bus_clk_names,
>> -	.num_bus_clks = ARRAY_SIZE(dsi_qcm2290_bus_clk_names),
>> -	.io_start = {
>> -		{ 0x5e94000 },
>> -	},
>> -};
>> -
>>  static const struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
>>  	.link_clk_set_rate = dsi_link_clk_set_rate_v2,
>>  	.link_clk_enable = dsi_link_clk_enable_v2,
>> @@ -312,9 +294,3 @@ const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
>>  
>>  	return cfg_hnd;
>>  }
>> -
>> -/*  Non autodetect configs */
>> -const struct msm_dsi_cfg_handler qcm2290_dsi_cfg_handler = {
>> -	.cfg = &qcm2290_dsi_cfg,
>> -	.ops = &msm_dsi_6g_v2_host_ops,
>> -};
> 
> And how do you think dsi.c is able to reference this... don't forget to
> remove it from dsi_cfg.h in v4.  In fact, if you look at how this was
> implemented you should also be able to remove #include "dsi_cfg.h" from
> dsi.c.  A clean revert of that patch would be nice, or just use it as
> reference to find the remnants:
> 
> https://lore.kernel.org/all/1644853060-12222-2-git-send-email-loic.poulain@linaro.org/
Ack

Konrad
> 
> - Marijn
Marijn Suijten March 14, 2023, 12:59 p.m. UTC | #12
On 2023-03-14 12:59:40, Konrad Dybcio wrote:
> 
> 
> On 14.03.2023 00:51, Marijn Suijten wrote:
> > On 2023-03-07 14:01:41, Konrad Dybcio wrote:
> >> Currently, we allow for MAX_DSI entries in io_start to facilitate for
> >> MAX_DSI number of DSI hosts at different addresses. The configuration
> >> is matched against the DSI CTRL hardware revision read back from the
> >> component. We need a way to resolve situations where multiple SoCs
> >> with different register maps may use the same version of DSI CTRL. In
> >> preparation to do so, make msm_dsi_config a 2d array where each entry
> >> represents a set of configurations adequate for a given SoC.
> > 
> > Note that this code isn't fool-proof against different SoCs sharing the
> > same DSI host address but for different indices (for example, the
> > address at variant 0 DSI 0 could be the same as variant 1 DSI 1) and the
> > matching logic would wrongly return ID 0 instead of 1 for SoC variant 1,
> > because that's the first matching address it finds.
> I don't think we've had that happen yet, but if it ever does, that's out
> of scope of this patchset.

Sure, as long as we're at least aware of this.

> >> This is totally fine to do, as the only differentiating factors
> >> between same-version-different-SoCs configurations are the number of
> >> DSI hosts (1 or 2, at least as of today) and the set of base registers.
> >> The regulator setup is the same, because the DSI hardware is the same,
> >> regardless of the SoC it was implemented in.
> >>
> >> In addition to that, update the matching logic such that it will loop
> >> over VARIANTS_MAX variants, making sure they are all taken into account.
> > 
> > "in addition to that" makes it sound like you're doing a separate new
> > thing in this patch, when the match logic must in fact be updated to
> > make it compatible with the change described above (as in, it doesn't
> > compile if you don't account for the extra depth in the array).
> I really think you're nitpicking here..

It's not, this genuinely had me confused for a while.  Could have at
least been addressed as part of v4 that had to be sent regardless.

- Marijn