Hi Biju,
Thank you for the review.
On Sun, May 4, 2025 at 1:41 PM Biju Das <biju.das...@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> > -----Original Message-----
> > From: Prabhakar <prabhakar.cse...@gmail.com>
> > Sent: 30 April 2025 21:41
> > Subject: [PATCH v4 09/15] drm: renesas: rz-du: mipi_dsi: Add OF data support
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad...@bp.renesas.com>
> >
> > In preparation for adding support for the Renesas RZ/V2H(P) SoC, this patch
> > introduces a mechanism to
> > pass SoC-specific information via OF data in the DSI driver. This enables
> > the driver to adapt
> > dynamically to various SoC-specific requirements without hardcoding
> > configurations.
> >
> > The MIPI DSI interface on the RZ/V2H(P) SoC is nearly identical to the one
> > on the RZ/G2L SoC. While
> > the LINK registers are shared between the two SoCs, the D-PHY registers
> > differ. Also the VCLK range
> > differs on both these SoCs. To accommodate these differences `struct
> > rzg2l_mipi_dsi_hw_info` is
> > introduced and as now passed as OF data.
> >
> > These changes lay the groundwork for the upcoming RZ/V2H(P) SoC support by
> > allowing SoC-specific data
> > to be passed through OF.
> >
> > 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>
> > ---
> > v3->v4:
> > - No changes
> >
> > v2->v3:
> > - Dropped !dsi->info check in rzg2l_mipi_dsi_probe() as it is not needed.
> >
> > v1->v2:
> > - Added DPHY_RST as feature flag
> > ---
> > .../gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c | 65 ++++++++++++++-----
> > .../drm/renesas/rz-du/rzg2l_mipi_dsi_regs.h | 2 -
> > 2 files changed, 48 insertions(+), 19 deletions(-)
> >
> > 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 911c955a3a76..ed259627f5e8 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> > @@ -28,10 +28,26 @@
> >
> > #include "rzg2l_mipi_dsi_regs.h"
> >
> > +#define RZ_MIPI_DSI_FEATURE_DPHY_RST BIT(0)
> > +
> > +struct rzg2l_mipi_dsi;
> > +
> > +struct rzg2l_mipi_dsi_hw_info {
> > + int (*dphy_init)(struct rzg2l_mipi_dsi *dsi, unsigned long hsfreq);
> > + void (*dphy_exit)(struct rzg2l_mipi_dsi *dsi);
> > + u32 phy_reg_offset;
> > + u32 link_reg_offset;
> > + unsigned long max_dclk;
> > + unsigned long min_dclk;
> > + u8 features;
> > +};
> > +
> > struct rzg2l_mipi_dsi {
> > struct device *dev;
> > void __iomem *mmio;
> >
> > + const struct rzg2l_mipi_dsi_hw_info *info;
> > +
> > struct reset_control *rstc;
> > struct reset_control *arstc;
> > struct reset_control *prstc;
> > @@ -164,22 +180,22 @@ static const struct rzg2l_mipi_dsi_timings
> > rzg2l_mipi_dsi_global_timings[] = {
> >
> > static void rzg2l_mipi_dsi_phy_write(struct rzg2l_mipi_dsi *dsi, u32 reg,
> > u32 data) {
> > - iowrite32(data, dsi->mmio + reg);
> > + iowrite32(data, dsi->mmio + dsi->info->phy_reg_offset + reg);
> > }
> >
> > static void rzg2l_mipi_dsi_link_write(struct rzg2l_mipi_dsi *dsi, u32 reg,
> > u32 data) {
> > - iowrite32(data, dsi->mmio + LINK_REG_OFFSET + reg);
> > + iowrite32(data, dsi->mmio + dsi->info->link_reg_offset + reg);
> > }
> >
> > static u32 rzg2l_mipi_dsi_phy_read(struct rzg2l_mipi_dsi *dsi, u32 reg) {
> > - return ioread32(dsi->mmio + reg);
> > + return ioread32(dsi->mmio + dsi->info->phy_reg_offset + reg);
> > }
> >
> > static u32 rzg2l_mipi_dsi_link_read(struct rzg2l_mipi_dsi *dsi, u32 reg) {
> > - return ioread32(dsi->mmio + LINK_REG_OFFSET + reg);
> > + return ioread32(dsi->mmio + dsi->info->link_reg_offset + reg);
> > }
> >
> > /*
> > -----------------------------------------------------------------------------
> > @@ -291,7 +307,7 @@ static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi
> > *dsi,
> > vclk_rate = clk_get_rate(dsi->vclk);
> > hsfreq = DIV_ROUND_CLOSEST_ULL(vclk_rate * bpp, dsi->lanes);
> >
> > - ret = rzg2l_mipi_dsi_dphy_init(dsi, hsfreq);
> > + ret = dsi->info->dphy_init(dsi, hsfreq);
> > if (ret < 0)
> > goto err_phy;
> >
> > @@ -334,7 +350,7 @@ static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi
> > *dsi,
> > return 0;
> >
> > err_phy:
> > - rzg2l_mipi_dsi_dphy_exit(dsi);
> > + dsi->info->dphy_exit(dsi);
> > pm_runtime_put(dsi->dev);
> >
> > return ret;
> > @@ -342,7 +358,7 @@ static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi
> > *dsi,
> >
> > static void rzg2l_mipi_dsi_stop(struct rzg2l_mipi_dsi *dsi) {
> > - rzg2l_mipi_dsi_dphy_exit(dsi);
> > + dsi->info->dphy_exit(dsi);
> > pm_runtime_put(dsi->dev);
> > }
> >
> > @@ -584,10 +600,12 @@ rzg2l_mipi_dsi_bridge_mode_valid(struct drm_bridge
> > *bridge,
> > const struct drm_display_info *info,
> > const struct drm_display_mode *mode) {
> > - if (mode->clock > 148500)
> > + struct rzg2l_mipi_dsi *dsi = bridge_to_rzg2l_mipi_dsi(bridge);
> > +
> > + if (mode->clock > dsi->info->max_dclk)
> > return MODE_CLOCK_HIGH;
> >
> > - if (mode->clock < 5803)
> > + if (mode->clock < dsi->info->min_dclk)
> > return MODE_CLOCK_LOW;
> >
> > return MODE_OK;
> > @@ -713,6 +731,8 @@ static int rzg2l_mipi_dsi_probe(struct platform_device
> > *pdev)
> > platform_set_drvdata(pdev, dsi);
> > dsi->dev = &pdev->dev;
> >
> > + dsi->info = of_device_get_match_data(&pdev->dev);
> > +
> > ret = drm_of_get_data_lanes_count_ep(dsi->dev->of_node, 1, 0, 1, 4);
> > if (ret < 0)
> > return dev_err_probe(dsi->dev, ret,
> > @@ -728,10 +748,12 @@ static int rzg2l_mipi_dsi_probe(struct
> > platform_device *pdev)
> > if (IS_ERR(dsi->vclk))
> > return PTR_ERR(dsi->vclk);
> >
> > - dsi->rstc = devm_reset_control_get_exclusive(dsi->dev, "rst");
> > - if (IS_ERR(dsi->rstc))
> > - return dev_err_probe(dsi->dev, PTR_ERR(dsi->rstc),
> > - "failed to get rst\n");
> > + if (dsi->info->features & RZ_MIPI_DSI_FEATURE_DPHY_RST) {
> > + dsi->rstc = devm_reset_control_get_exclusive(dsi->dev, "rst");
> > + if (IS_ERR(dsi->rstc))
> > + return dev_err_probe(dsi->dev, PTR_ERR(dsi->rstc),
> > + "failed to get rst\n");
> > + }
>
> Dt binding check already checks "rst" as required property the currently
> supported
> SoCs. So for RZ/V2H if it is optional maybe replace
> devm_reset_control_get_exclusive()->devm_reset_control_get_optional_exclusive()
> and get rid of this feature bit check? If I understand correctly, optional
> APIs
> are introduced for this purpose.
>
Ok, I'll make use of devm_reset_control_get_optional_exclusive()
Cheers,
Prabhakar
