Hi Biju,
Thank you for the review.
On Sat, Apr 12, 2025 at 9:01 AM Biju Das <biju.das...@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> Thanks for the patch.
>
> > -----Original Message-----
> > From: Prabhakar <prabhakar.cse...@gmail.com>
> > Sent: 08 April 2025 21:09
> > Subject: [PATCH v2 15/15] drm: renesas: rz-du: mipi_dsi: Add support for
> > RZ/V2H(P) SoC
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad...@bp.renesas.com>
> >
> > Add DSI support for Renesas RZ/V2H(P) SoC.
> >
> > Co-developed-by: Fabrizio Castro <fabrizio.castro...@renesas.com>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro...@renesas.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad...@bp.renesas.com>
> > ---
> > v1->v2:
> > - Dropped unused macros
> > - Added missing LPCLK flag to rzvv2h info
> > ---
> > .../gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c | 451 ++++++++++++++++++
> > .../drm/renesas/rz-du/rzg2l_mipi_dsi_regs.h | 34 ++
> > 2 files changed, 485 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> > b/drivers/gpu/drm/renesas/rz-
> > du/rzg2l_mipi_dsi.c
> > index 6c6bc59eabbc..e260e2ed03c1 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> > @@ -5,6 +5,7 @@
> > * Copyright (C) 2022 Renesas Electronics Corporation
> > */
> > #include <linux/clk.h>
> > +#include <linux/clk/renesas-rzv2h-dsi.h>
>
> This patch has hard dependency on clk driver.
>
> > #include <linux/delay.h>
> > #include <linux/io.h>
> > #include <linux/iopoll.h>
> > @@ -32,6 +33,9 @@
> > #define RZ_MIPI_DSI_FEATURE_16BPP BIT(1)
> > #define RZ_MIPI_DSI_FEATURE_LPCLK BIT(2)
> >
> > +#define RZV2H_MIPI_DPHY_FOUT_MIN_IN_MEGA (80 * MEGA)
> > +#define RZV2H_MIPI_DPHY_FOUT_MAX_IN_MEGA (1500 * MEGA)
> > +
> > struct rzg2l_mipi_dsi;
> >
> > struct rzg2l_mipi_dsi_hw_info {
> > @@ -42,6 +46,7 @@ struct rzg2l_mipi_dsi_hw_info {
> > unsigned long long *hsfreq_mhz);
> > unsigned int (*dphy_mode_clk_check)(struct rzg2l_mipi_dsi *dsi,
> > unsigned long mode_freq);
> > + const struct rzv2h_plldsi_div_limits *cpg_dsi_limits;
> > u32 phy_reg_offset;
> > u32 link_reg_offset;
> > unsigned long max_dclk;
> > @@ -49,6 +54,11 @@ struct rzg2l_mipi_dsi_hw_info {
> > u8 features;
> > };
> >
> > +struct rzv2h_dsi_mode_calc {
> > + unsigned long mode_freq;
> > + unsigned long long mode_freq_hz;
> > +};
> > +
> > struct rzg2l_mipi_dsi {
> > struct device *dev;
> > void __iomem *mmio;
> > @@ -70,6 +80,18 @@ struct rzg2l_mipi_dsi {
> > unsigned int num_data_lanes;
> > unsigned int lanes;
> > unsigned long mode_flags;
> > +
> > + struct rzv2h_dsi_mode_calc mode_calc;
> > + struct rzv2h_plldsi_parameters dsi_parameters; };
> > +
> > +static const struct rzv2h_plldsi_div_limits rzv2h_plldsi_div_limits = {
> > + .m = { .min = 64, .max = 1023 },
> > + .p = { .min = 1, .max = 4 },
> > + .s = { .min = 0, .max = 5 },
> > + .k = { .min = -32768, .max = 32767 },
> > + .csdiv = { .min = 1, .max = 1 },
> > + .fvco = { .min = 1050 * MEGA, .max = 2100 * MEGA }
> > };
> >
> > static inline struct rzg2l_mipi_dsi *
> > @@ -186,6 +208,249 @@ static const struct rzg2l_mipi_dsi_timings
> > rzg2l_mipi_dsi_global_timings[] = {
> > },
> > };
> >
> > +struct rzv2h_mipi_dsi_timings {
> > + unsigned long hsfreq;
> > + u16 value;
> > +};
> > +
> > +static const struct rzv2h_mipi_dsi_timings TCLKPRPRCTL[] = {
> > + {150000000UL, 0},
> > + {260000000UL, 1},
> > + {370000000UL, 2},
> > + {470000000UL, 3},
> > + {580000000UL, 4},
> > + {690000000UL, 5},
> > + {790000000UL, 6},
> > + {900000000UL, 7},
> > + {1010000000UL, 8},
> > + {1110000000UL, 9},
> > + {1220000000UL, 10},
> > + {1330000000UL, 11},
> > + {1430000000UL, 12},
> > + {1500000000UL, 13},
> > +};
>
> Not sure, Is it right approach to optimize this table as the second entry is
> sequential
> with a fixed number for all tables except last one?
>
Agreed, I'll simplify this.
> 28 bytes can be saved, if we use a variable holding start_index.
>
> > +
> > +static const struct rzv2h_mipi_dsi_timings TCLKZEROCTL[] = {
> > + {90000000UL, 2},
> > + {110000000UL, 3},
> > + {130000000UL, 4},
> > + {150000000UL, 5},
> > + {180000000UL, 6},
> > + {210000000UL, 7},
> > + {230000000UL, 8},
> > + {240000000UL, 9},
> > + {250000000UL, 10},
> > + {270000000UL, 11},
> > + {290000000UL, 12},
> > + {310000000UL, 13},
> > + {340000000UL, 14},
> > + {360000000UL, 15},
> > + {380000000UL, 16},
> > + {410000000UL, 17},
> > + {430000000UL, 18},
> > + {450000000UL, 19},
> > + {470000000UL, 20},
> > + {500000000UL, 21},
> > + {520000000UL, 22},
> > + {540000000UL, 23},
> > + {570000000UL, 24},
> > + {590000000UL, 25},
> > + {610000000UL, 26},
> > + {630000000UL, 27},
> > + {660000000UL, 28},
> > + {680000000UL, 29},
> > + {700000000UL, 30},
> > + {730000000UL, 31},
> > + {750000000UL, 32},
> > + {770000000UL, 33},
> > + {790000000UL, 34},
> > + {820000000UL, 35},
> > + {840000000UL, 36},
> > + {860000000UL, 37},
> > + {890000000UL, 38},
> > + {910000000UL, 39},
> > + {930000000UL, 40},
> > + {950000000UL, 41},
> > + {980000000UL, 42},
> > + {1000000000UL, 43},
> > + {1020000000UL, 44},
> > + {1050000000UL, 45},
> > + {1070000000UL, 46},
> > + {1090000000UL, 47},
> > + {1110000000UL, 48},
> > + {1140000000UL, 49},
> > + {1160000000UL, 50},
> > + {1180000000UL, 51},
> > + {1210000000UL, 52},
> > + {1230000000UL, 53},
> > + {1250000000UL, 54},
> > + {1270000000UL, 55},
> > + {1300000000UL, 56},
> > + {1320000000UL, 57},
> > + {1340000000UL, 58},
> > + {1370000000UL, 59},
> > + {1390000000UL, 60},
> > + {1410000000UL, 61},
> > + {1430000000UL, 62},
> > + {1460000000UL, 63},
> > + {1480000000UL, 64},
> > + {1500000000UL, 65},
>
> Same. Here 128 bytes
>
> > +};
> > +
> > +static const struct rzv2h_mipi_dsi_timings TCLKPOSTCTL[] = {
> > + {80000000UL, 6},
> > + {210000000UL, 7},
> > + {340000000UL, 8},
> > + {480000000UL, 9},
> > + {610000000UL, 10},
> > + {740000000UL, 11},
> > + {880000000UL, 12},
> > + {1010000000UL, 13},
> > + {1140000000UL, 14},
> > + {1280000000UL, 15},
> > + {1410000000UL, 16},
> > + {1500000000UL, 17},
>
> Same. Here 24 bytes
>
> > +};
> > +
> > +static const struct rzv2h_mipi_dsi_timings TCLKTRAILCTL[] = {
> > + {140000000UL, 1},
> > + {250000000UL, 2},
> > + {370000000UL, 3},
> > + {480000000UL, 4},
> > + {590000000UL, 5},
> > + {710000000UL, 6},
> > + {820000000UL, 7},
> > + {940000000UL, 8},
> > + {1050000000UL, 9},
> > + {1170000000UL, 10},
> > + {1280000000UL, 11},
> > + {1390000000UL, 12},
> > + {1500000000UL, 13},
> Same. Here 26 bytes
>
> > +};
> > +
> > +static const struct rzv2h_mipi_dsi_timings THSPRPRCTL[] = {
> > + {110000000UL, 0},
> > + {190000000UL, 1},
> > + {290000000UL, 2},
> > + {400000000UL, 3},
> > + {500000000UL, 4},
> > + {610000000UL, 5},
> > + {720000000UL, 6},
> > + {820000000UL, 7},
> > + {930000000UL, 8},
> > + {1030000000UL, 9},
> > + {1140000000UL, 10},
> > + {1250000000UL, 11},
> > + {1350000000UL, 12},
> > + {1460000000UL, 13},
> > + {1500000000UL, 14},
>
> Same. Here 30 bytes
>
> > +};
> > +
> > +static const struct rzv2h_mipi_dsi_timings THSZEROCTL[] = {
> > + {180000000UL, 0},
> > + {240000000UL, 1},
> > + {290000000UL, 2},
> > + {350000000UL, 3},
> > + {400000000UL, 4},
> > + {460000000UL, 5},
> > + {510000000UL, 6},
> > + {570000000UL, 7},
> > + {620000000UL, 8},
> > + {680000000UL, 9},
> > + {730000000UL, 10},
> > + {790000000UL, 11},
> > + {840000000UL, 12},
> > + {900000000UL, 13},
> > + {950000000UL, 14},
> > + {1010000000UL, 15},
> > + {1060000000UL, 16},
> > + {1120000000UL, 17},
> > + {1170000000UL, 18},
> > + {1230000000UL, 19},
> > + {1280000000UL, 20},
> > + {1340000000UL, 21},
> > + {1390000000UL, 22},
> > + {1450000000UL, 23},
> > + {1500000000UL, 24},
>
> Same. Here 50 bytes.
>
> > +};
> > +
> > +static const struct rzv2h_mipi_dsi_timings THSTRAILCTL[] = {
> > + {100000000UL, 3},
> > + {210000000UL, 4},
> > + {320000000UL, 5},
> > + {420000000UL, 6},
> > + {530000000UL, 7},
> > + {640000000UL, 8},
> > + {750000000UL, 9},
> > + {850000000UL, 10},
> > + {960000000UL, 11},
> > + {1070000000UL, 12},
> > + {1180000000UL, 13},
> > + {1280000000UL, 14},
> > + {1390000000UL, 15},
> > + {1500000000UL, 16},
>
> Same. Here 28 bytes?
>
> > +};
> > +
> > +static const struct rzv2h_mipi_dsi_timings TLPXCTL[] = {
> > + {130000000UL, 0},
> > + {260000000UL, 1},
> > + {390000000UL, 2},
> > + {530000000UL, 3},
> > + {660000000UL, 4},
> > + {790000000UL, 5},
> > + {930000000UL, 6},
> > + {1060000000UL, 7},
> > + {1190000000UL, 8},
> > + {1330000000UL, 9},
> > + {1460000000UL, 10},
> > + {1500000000UL, 11},
>
> Same. Here 24 bytes
>
> > +};
> > +
> > +static const struct rzv2h_mipi_dsi_timings THSEXITCTL[] = {
> > + {150000000UL, 1},
> > + {230000000UL, 2},
> > + {310000000UL, 3},
> > + {390000000UL, 4},
> > + {470000000UL, 5},
> > + {550000000UL, 6},
> > + {630000000UL, 7},
> > + {710000000UL, 8},
> > + {790000000UL, 9},
> > + {870000000UL, 10},
> > + {950000000UL, 11},
> > + {1030000000UL, 12},
> > + {1110000000UL, 13},
> > + {1190000000UL, 14},
> > + {1270000000UL, 15},
> > + {1350000000UL, 16},
> > + {1430000000UL, 17},
> > + {1500000000UL, 18},
>
> Same. Here 36 bytes.
>
> > +};
> > +
> > +static const struct rzv2h_mipi_dsi_timings ULPSEXIT[] = {
> > + {1953125UL, 49},
> > + {3906250UL, 98},
> > + {7812500UL, 195},
> > + {15625000UL, 391},
>
> Here we have problem as it is non-sequential and only has 4 entries?
>
> Since it is ULPS EXIT Counter values compared to DPHY timings
> This can have its own structure?
>
Agreed as this will have a separate function of its own no need for a
new struct, I'll have something like below:
static u16 rzv2h_dphy_find_ulpsexit(unsigned long freq)
{
const unsigned long hsfreq[] = {
1953125UL,
3906250UL,
7812500UL,
15625000UL,
};
const u16 ulpsexit[] = {49, 98, 195, 391};
unsigned int i;
for (i = 0; i < ARRAY_SIZE(hsfreq); i++) {
if (freq <= hsfreq[i])
break;
}
if (i == ARRAY_SIZE(hsfreq))
i -= 1;
return ulpsexit[i];
}
> > +};
> > +
> > +static int rzv2h_dphy_find_timings_val(unsigned long freq,
> > + const struct rzv2h_mipi_dsi_timings
> > timings[],
> > + unsigned int size)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < size; i++) {
> > + if (freq <= timings[i].hsfreq)
> > + break;
> > + }
> > +
> > + if (i == size)
> > + i -= 1;
> > +
> > + return timings[i].value;
>
> This will be then start_index + i for all DPHY timing parmeter table.
>
Agreed.
> And
>
> Maybe use another function to find ULPS EXIT Counter values??
>
>
> > +};
> > +
> > static void rzg2l_mipi_dsi_phy_write(struct rzg2l_mipi_dsi *dsi, u32 reg,
> > u32 data) {
> > iowrite32(data, dsi->mmio + dsi->info->phy_reg_offset + reg); @@
> > -307,6 +572,168 @@ static int
> > rzg2l_dphy_conf_clks(struct rzg2l_mipi_dsi *dsi, unsigned long mode_f
> > return 0;
> > }
> >
> > +static unsigned int rzv2h_dphy_mode_clk_check(struct rzg2l_mipi_dsi *dsi,
> > + unsigned long mode_freq)
> > +{
> > + struct rzv2h_plldsi_parameters *dsi_parameters = &dsi->dsi_parameters;
> > + unsigned long long hsfreq_mhz, mode_freq_hz, mode_freq_mhz;
> > + struct rzv2h_plldsi_parameters cpg_dsi_parameters;
> > + unsigned int bpp, i;
> > +
> > + bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > +
> > + for (i = 0; i < 10; i += 1) {
> > + unsigned long hsfreq;
> > + bool parameters_found;
> > +
> > + mode_freq_hz = mode_freq * KILO + i;
> > + mode_freq_mhz = mode_freq_hz * KILO * 1ULL;
> > + parameters_found =
> > rzv2h_dsi_get_pll_parameters_values(dsi->info->cpg_dsi_limits,
> > +
> > &cpg_dsi_parameters,
> > +
> > mode_freq_mhz);
> > + if (!parameters_found)
> > + continue;
> > +
> > + hsfreq_mhz =
> > DIV_ROUND_CLOSEST_ULL(cpg_dsi_parameters.freq_mhz * bpp, dsi->lanes);
> > + parameters_found =
> > rzv2h_dsi_get_pll_parameters_values(&rzv2h_plldsi_div_limits,
> > +
> > dsi_parameters,
> > +
> > hsfreq_mhz);
> > + if (!parameters_found)
> > + continue;
> > +
> > + if (abs(dsi_parameters->error_mhz) >= 500)
> > + continue;
> > +
> > + hsfreq = DIV_ROUND_CLOSEST_ULL(hsfreq_mhz, KILO);
> > + if (hsfreq >= RZV2H_MIPI_DPHY_FOUT_MIN_IN_MEGA &&
> > + hsfreq <= RZV2H_MIPI_DPHY_FOUT_MAX_IN_MEGA) {
> > + dsi->mode_calc.mode_freq_hz = mode_freq_hz;
> > + dsi->mode_calc.mode_freq = mode_freq;
> > + return MODE_OK;
> > + }
> > + }
> > +
> > + return MODE_CLOCK_RANGE;
> > +}
> > +
> > +static int rzv2h_dphy_conf_clks(struct rzg2l_mipi_dsi *dsi, unsigned long
> > mode_freq,
> > + unsigned long long *hsfreq_mhz)
> > +{
> > + struct rzv2h_plldsi_parameters *dsi_parameters = &dsi->dsi_parameters;
> > + unsigned long status;
> > +
> > + if (dsi->mode_calc.mode_freq != mode_freq) {
> > + status = rzv2h_dphy_mode_clk_check(dsi, mode_freq);
> > + if (status != MODE_OK) {
> > + dev_err(dsi->dev, "No PLL parameters found for mode
> > clk %lu\n",
> > + mode_freq);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + clk_set_rate(dsi->vclk, dsi->mode_calc.mode_freq_hz);
> > + *hsfreq_mhz = dsi_parameters->freq_mhz;
> > +
> > + return 0;
> > +}
> > +
> > +static int rzv2h_mipi_dsi_dphy_init(struct rzg2l_mipi_dsi *dsi,
> > + unsigned long long hsfreq_mhz)
> > +{
> > + struct rzv2h_plldsi_parameters *dsi_parameters = &dsi->dsi_parameters;
> > + unsigned long lpclk_rate = clk_get_rate(dsi->lpclk);
> > + u32 phytclksetr, phythssetr, phytlpxsetr, phycr;
> > + struct rzg2l_mipi_dsi_timings dphy_timings;
> > + unsigned long long hsfreq;
> > + u32 ulpsexit;
> > +
> > + hsfreq = DIV_ROUND_CLOSEST_ULL(hsfreq_mhz, KILO);
> > +
> > + if (dsi_parameters->freq_mhz == hsfreq_mhz)
> > + goto parameters_found;
> > +
> > + if (rzv2h_dsi_get_pll_parameters_values(&rzv2h_plldsi_div_limits,
> > + dsi_parameters, hsfreq_mhz))
> > + goto parameters_found;
> > +
> > + dev_err(dsi->dev, "No PLL parameters found for HSFREQ %lluHz\n",
> > hsfreq);
> > + return -EINVAL;
> > +
> > +parameters_found:
> > + dphy_timings.tclk_trail =
> > + rzv2h_dphy_find_timings_val(hsfreq, TCLKTRAILCTL,
> > + ARRAY_SIZE(TCLKTRAILCTL));
> > + dphy_timings.tclk_post =
> > + rzv2h_dphy_find_timings_val(hsfreq, TCLKPOSTCTL,
> > + ARRAY_SIZE(TCLKPOSTCTL));
> > + dphy_timings.tclk_zero =
> > + rzv2h_dphy_find_timings_val(hsfreq, TCLKZEROCTL,
> > + ARRAY_SIZE(TCLKZEROCTL));
> > + dphy_timings.tclk_prepare =
> > + rzv2h_dphy_find_timings_val(hsfreq, TCLKPRPRCTL,
> > + ARRAY_SIZE(TCLKPRPRCTL));
> > + dphy_timings.ths_exit =
> > + rzv2h_dphy_find_timings_val(hsfreq, THSEXITCTL,
> > + ARRAY_SIZE(THSEXITCTL));
> > + dphy_timings.ths_trail =
> > + rzv2h_dphy_find_timings_val(hsfreq, THSTRAILCTL,
> > + ARRAY_SIZE(THSTRAILCTL));
> > + dphy_timings.ths_zero =
> > + rzv2h_dphy_find_timings_val(hsfreq, THSZEROCTL,
> > + ARRAY_SIZE(THSZEROCTL));
> > + dphy_timings.ths_prepare =
> > + rzv2h_dphy_find_timings_val(hsfreq, THSPRPRCTL,
> > + ARRAY_SIZE(THSPRPRCTL));
> > + dphy_timings.tlpx =
> > + rzv2h_dphy_find_timings_val(hsfreq, TLPXCTL,
> > + ARRAY_SIZE(TLPXCTL));
> > + ulpsexit =
> > + rzv2h_dphy_find_timings_val(lpclk_rate, ULPSEXIT,
> > + ARRAY_SIZE(ULPSEXIT));
> > +
> > + phytclksetr = PHYTCLKSETR_TCLKTRAILCTL(dphy_timings.tclk_trail) |
> > + PHYTCLKSETR_TCLKPOSTCTL(dphy_timings.tclk_post) |
> > + PHYTCLKSETR_TCLKZEROCTL(dphy_timings.tclk_zero) |
> > + PHYTCLKSETR_TCLKPRPRCTL(dphy_timings.tclk_prepare);
> > + phythssetr = PHYTHSSETR_THSEXITCTL(dphy_timings.ths_exit) |
> > + PHYTHSSETR_THSTRAILCTL(dphy_timings.ths_trail) |
> > + PHYTHSSETR_THSZEROCTL(dphy_timings.ths_zero) |
> > + PHYTHSSETR_THSPRPRCTL(dphy_timings.ths_prepare);
> > + phytlpxsetr = rzg2l_mipi_dsi_phy_read(dsi, PHYTLPXSETR) & ~GENMASK(7,
> > 0);
> > + phytlpxsetr |= PHYTLPXSETR_TLPXCTL(dphy_timings.tlpx);
> > + phycr = rzg2l_mipi_dsi_phy_read(dsi, PHYCR) & ~GENMASK(9, 0);
> > + phycr |= PHYCR_ULPSEXIT(ulpsexit);
> > +
> > + /* Setting all D-PHY Timings Registers */
> > + rzg2l_mipi_dsi_phy_write(dsi, PHYTCLKSETR, phytclksetr);
> > + rzg2l_mipi_dsi_phy_write(dsi, PHYTHSSETR, phythssetr);
> > + rzg2l_mipi_dsi_phy_write(dsi, PHYTLPXSETR, phytlpxsetr);
> > + rzg2l_mipi_dsi_phy_write(dsi, PHYCR, phycr);
> > +
> > + rzg2l_mipi_dsi_phy_write(dsi, PLLCLKSET0R,
> > + PLLCLKSET0R_PLL_S(dsi_parameters->s) |
> > + PLLCLKSET0R_PLL_P(dsi_parameters->p) |
> > + PLLCLKSET0R_PLL_M(dsi_parameters->m));
> > + rzg2l_mipi_dsi_phy_write(dsi, PLLCLKSET1R,
> > PLLCLKSET1R_PLL_K(dsi_parameters->k));
> > + udelay(20);
> > +
> > + rzg2l_mipi_dsi_phy_write(dsi, PLLENR, PLLENR_PLLEN);
> > + udelay(500);
>
> Checkpatch warnings, maybe use fsleep()?
>
> CHECK: usleep_range is preferred over udelay; see function description of
> usleep_range() and udelay().
> #475: FILE: drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c:718:
> + udelay(20);
> CHECK: usleep_range is preferred over udelay; see function description of
> usleep_range() and udelay().
> #478: FILE: drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c:721:
> + udelay(500);
> CHECK: usleep_range is preferred over udelay; see function description of
> usleep_range() and udelay().
> #485: FILE: drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c:728:
> + udelay(220);
>
Ok, I will switch to fsleep().
Cheers,
Prabhakar
