RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

From: Stephen Boyd <sboyd@kernel.org>
To: Biju Das <biju.das.jz@bp.renesas.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: "Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Hans Verkuil" <hverkuil-cisco@xs4all.nl>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Rob Herring" <robh@kernel.org>,
	"Prabhakar Mahadev Lad" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	linux-media@vger.kernel.org
Subject: RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling
Date: Wed, 24 Jan 2024 12:48:48 -0800	[thread overview]
Message-ID: <cae77ff909448cd7eb88716696d24091.sboyd@kernel.org> (raw)
In-Reply-To: <TYCPR01MB1126908A3DCEE35723E102993867B2@TYCPR01MB11269.jpnprd01.prod.outlook.com>

Quoting Biju Das (2024-01-24 06:01:30)
> > > > > > CSI2nMCT0_VDLN(csi2->lanes)); @@
> > > > > > -386,11 +389,40 @@ static void
> > > > > > rzg2l_csi2_mipi_link_enable(struct
> > > > rzg2l_csi2 *csi2)
> > > > > >       rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
> > > > > >       rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
> > > > > >
> > > > > > +     clk_disable_unprepare(csi2->vclk);
> > > > > > +     for (count = 0; count < 5; count++) {
> > > > > > +             if (!(__clk_is_enabled(csi2->vclk)))

__clk_is_enabled() is a clk provider API. You shouldn't be using it in
consumer drivers.

> > > > > > +                     break;
> > > > > > +             usleep_range(10, 20);
> > > > > > +     }
> > > > > > +
> > > > > > +     if (count == 5) {
> > > > > > +             dev_err(csi2->dev, "Timeout, not able to turn OFF
> > vclk\n");
> > > > > > +             return -ETIMEDOUT;
> > > > > > +     }
> > > > >
> > > > > It'd be nice to have a CCF function to do this. Either way, you
> > > > > can use read_poll_timeout().
> > > >
> > > > I would have sworn that clk_disable_unprepare() is synchronous, and
> > > > would have sworn even stronger for clk_prepare_enable(). What's
> > > > going on here, is there really a delay ? It sounds like a bug in the
> > clock driver.
> > >
> > > At the moment we are not checking clock status when we turn off a
> > > clock However, for clock ON we are checking the status while turning it
> > ON.
> > > We need to fix the driver for clk_disable_unprepare().
> > 
> > Does that mean that the check below clk_prepare_enable() could be removed
> > already ?
> 
> Yes, that is correct I will remove in the next version as clk_prepare_enable() is
> synchronous as it checks the status to make sure it is turned ON.
> 
> > 
> > Regarding clock disable, it isn't clear if the API guarantees synchronous
> > calls. I'd recommend asking the clock maintainers. If it doesn't, then the
> > clock driver isn't wrong (and may lead to faster operation in most cases),
> > but I think synchronizing the clock disable by waiting for the clock to be
> > actually disabled should be implemented as a helper in CCF.
> 
> +clk.
> 
> Hi Stephen and all,
> 
> Can you please shed some light on this?
> 

clk_disable() is reference counted. The call to clk_disable() may do
nothing besides decrement a count and return. Per the documentation in
clk.h it means that the consumer is no longer using the clk. The clk
could be turned off later, or not at all.

It seems like the clk driver has a bug. Make the clk driver synchronize
the disable with enable please.