| Message ID | 20220610092924.754942-26-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Fix hotplug for vc4 | expand |
Hi Maxime. On Fri, 10 Jun 2022 at 10:30, Maxime Ripard <maxime@cerno.tech> wrote: > > Adding a device-managed action will make the error path easier, so let's > create one to disable our clock. The DPI block has two clocks (core and pixel), and this is only affecting one of them (the core clock). (I'm actually puzzling over what it's wanting to do with the core clock here as it's only enabling it rather than setting a rate. I think it may be redundant). With that text amended: Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Dave > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/vc4/vc4_dpi.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c > index 5a6cdea7bf7b..4e24dbad77f2 100644 > --- a/drivers/gpu/drm/vc4/vc4_dpi.c > +++ b/drivers/gpu/drm/vc4/vc4_dpi.c > @@ -237,6 +237,13 @@ static int vc4_dpi_init_bridge(struct vc4_dpi *dpi) > return drm_bridge_attach(&dpi->encoder.base, bridge, NULL, 0); > } > > +static void vc4_dpi_disable_clock(void *ptr) > +{ > + struct vc4_dpi *dpi = ptr; > + > + clk_disable_unprepare(dpi->core_clock); > +} > + > static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) > { > struct platform_device *pdev = to_platform_device(dev); > @@ -285,6 +292,10 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) > return ret; > } > > + ret = devm_add_action_or_reset(dev, vc4_dpi_disable_clock, dpi); > + if (ret) > + return ret; > + > drm_simple_encoder_init(drm, &dpi->encoder.base, DRM_MODE_ENCODER_DPI); > drm_encoder_helper_add(&dpi->encoder.base, &vc4_dpi_encoder_helper_funcs); > > @@ -300,7 +311,6 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) > > err_destroy_encoder: > drm_encoder_cleanup(&dpi->encoder.base); > - clk_disable_unprepare(dpi->core_clock); > return ret; > } > > @@ -310,8 +320,6 @@ static void vc4_dpi_unbind(struct device *dev, struct device *master, > struct vc4_dpi *dpi = dev_get_drvdata(dev); > > drm_encoder_cleanup(&dpi->encoder.base); > - > - clk_disable_unprepare(dpi->core_clock); > } > > static const struct component_ops vc4_dpi_ops = { > -- > 2.36.1 >
Hi Dave, On Tue, Jun 14, 2022 at 05:47:28PM +0100, Dave Stevenson wrote: > Hi Maxime. > > On Fri, 10 Jun 2022 at 10:30, Maxime Ripard <maxime@cerno.tech> wrote: > > > > Adding a device-managed action will make the error path easier, so let's > > create one to disable our clock. > > The DPI block has two clocks (core and pixel), and this is only > affecting one of them (the core clock). Thanks for the suggestion, I've amended the commit message. > (I'm actually puzzling over what it's wanting to do with the core > clock here as it's only enabling it rather than setting a rate. I > think it may be redundant). Could it be that it a "bus" clock that we need it to access the registers? Maxime
On Thu, 16 Jun 2022 at 09:38, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Dave, > > On Tue, Jun 14, 2022 at 05:47:28PM +0100, Dave Stevenson wrote: > > Hi Maxime. > > > > On Fri, 10 Jun 2022 at 10:30, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > Adding a device-managed action will make the error path easier, so let's > > > create one to disable our clock. > > > > The DPI block has two clocks (core and pixel), and this is only > > affecting one of them (the core clock). > > Thanks for the suggestion, I've amended the commit message. > > > (I'm actually puzzling over what it's wanting to do with the core > > clock here as it's only enabling it rather than setting a rate. I > > think it may be redundant). > > Could it be that it a "bus" clock that we need it to access the > registers? No idea. Normally it's the power domain that is required to access registers. AFAIK the core clock is never turned off, so the request for it is just a little odd. It is what it is though, so fine to leave it alone. Dave
On Thu, Jun 16, 2022 at 10:47:56AM +0100, Dave Stevenson wrote: > On Thu, 16 Jun 2022 at 09:38, Maxime Ripard <maxime@cerno.tech> wrote: > > > > Hi Dave, > > > > On Tue, Jun 14, 2022 at 05:47:28PM +0100, Dave Stevenson wrote: > > > Hi Maxime. > > > > > > On Fri, 10 Jun 2022 at 10:30, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > Adding a device-managed action will make the error path easier, so let's > > > > create one to disable our clock. > > > > > > The DPI block has two clocks (core and pixel), and this is only > > > affecting one of them (the core clock). > > > > Thanks for the suggestion, I've amended the commit message. > > > > > (I'm actually puzzling over what it's wanting to do with the core > > > clock here as it's only enabling it rather than setting a rate. I > > > think it may be redundant). > > > > Could it be that it a "bus" clock that we need it to access the > > registers? > > No idea. Normally it's the power domain that is required to access > registers. For HDMI at least, the power domain being off will make a read return some bogus value, but the core clock being off will just make the CPU stall. I can only assume it would be the same for the DPI controller? > AFAIK the core clock is never turned off, so the request for it is > just a little odd. Even if the clock driver never shuts it off, I think it still has value to follow the Clock Framework conventions. We might have a different clock policy in the future, and it would throw people reading the DPI driver off. Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c index 5a6cdea7bf7b..4e24dbad77f2 100644 --- a/drivers/gpu/drm/vc4/vc4_dpi.c +++ b/drivers/gpu/drm/vc4/vc4_dpi.c @@ -237,6 +237,13 @@ static int vc4_dpi_init_bridge(struct vc4_dpi *dpi) return drm_bridge_attach(&dpi->encoder.base, bridge, NULL, 0); } +static void vc4_dpi_disable_clock(void *ptr) +{ + struct vc4_dpi *dpi = ptr; + + clk_disable_unprepare(dpi->core_clock); +} + static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) { struct platform_device *pdev = to_platform_device(dev); @@ -285,6 +292,10 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) return ret; } + ret = devm_add_action_or_reset(dev, vc4_dpi_disable_clock, dpi); + if (ret) + return ret; + drm_simple_encoder_init(drm, &dpi->encoder.base, DRM_MODE_ENCODER_DPI); drm_encoder_helper_add(&dpi->encoder.base, &vc4_dpi_encoder_helper_funcs); @@ -300,7 +311,6 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) err_destroy_encoder: drm_encoder_cleanup(&dpi->encoder.base); - clk_disable_unprepare(dpi->core_clock); return ret; } @@ -310,8 +320,6 @@ static void vc4_dpi_unbind(struct device *dev, struct device *master, struct vc4_dpi *dpi = dev_get_drvdata(dev); drm_encoder_cleanup(&dpi->encoder.base); - - clk_disable_unprepare(dpi->core_clock); } static const struct component_ops vc4_dpi_ops = {
Adding a device-managed action will make the error path easier, so let's create one to disable our clock. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/vc4/vc4_dpi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)