From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 830B9134744; Wed, 24 Jan 2024 20:48:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706129331; cv=none; b=B8NQtjJK6tiU07aQi+2sdvpqdveq/CxORpe5r1NZJ2sTQy3o11fp3cYf8ZsXMvTvyRVLLE7p0TFvP9vUXLPBvGz3CJCzAa0P4tAN7B6lpaBbAK3z9ayD0b5OD00T97MlY3YW/CWUxmM8cgw23nQO0Jx+3IH+KzqEhoxShiTzC8w= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706129331; c=relaxed/simple; bh=Pu2WnJULiZ3SHOQa+Ls5gMiBM9GGmreLuufDrBDDCxY=; h=Message-ID:Content-Type:MIME-Version:In-Reply-To:References: Subject:From:Cc:To:Date; b=Uf08sUOzJ4pW1qMkV9Ht2FU/CAPzzNkaah2flzseZHvvcbYmnnWdjk72RHaqqlbM6KfOz3+4LajtJRsLRe6dpHmXLFHtNdh0KAez+9rDWcKZyc1Uc8ybJAzy7Py9n0za/nLqse4q0THJyytfRhyaDFuuJWrArxu42diouFWWcGA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=l/lDnsnr; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="l/lDnsnr" Received: by smtp.kernel.org (Postfix) with ESMTPSA id D15F6C433C7; Wed, 24 Jan 2024 20:48:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1706129330; bh=Pu2WnJULiZ3SHOQa+Ls5gMiBM9GGmreLuufDrBDDCxY=; h=In-Reply-To:References:Subject:From:Cc:To:Date:From; b=l/lDnsnrKGyL4NokUGCsi1Dqf3sT2rJfYHrXct9Zr7T2d5xHZklzHnHzME7Eh9v0M 3BHNHwvIlFu/Unu8C2lQmm46LAVa2hT+wPbd81/wdHD4gh0hnjgoy4w56q3WJv2xfH q7HOrUFDXOuWYrd0wxS6m7yDHyE5reYUHX7ovIqmjpCfP5d5adU7uQf1Qb8gRaJ5Yj KlycmGGIsCESbhnKmb3TeMBdTK7Sss/DnCDlxbAgi/doD/hQ6yUzeMMuz6UHu1+MmX rlClWFvh0p71trdnWXIZXqg0HnqytUqUTGRYk+FmHqO43y8xd7c5SwvuKUnIhqYvGR 3O7Y9BSLwS89w== Message-ID: Content-Type: text/plain; charset="utf-8" Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable In-Reply-To: References: <20240123115821.292787-1-biju.das.jz@bp.renesas.com> <20240123115821.292787-5-biju.das.jz@bp.renesas.com> <20240123153550.GT10679@pendragon.ideasonboard.com> <20240123221011.GA16581@pendragon.ideasonboard.com> Subject: RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling From: Stephen Boyd Cc: Sakari Ailus , Mauro Carvalho Chehab , Hans Verkuil , Uwe =?utf-8?q?Kleine-K=C3=B6nig?= , Rob Herring , Prabhakar Mahadev Lad , linux-media@vger.kernel.org , Geert Uytterhoeven , biju.das.au , linux-renesas-soc@vger.kernel.org , linux-clk@vger.kernel.org To: Biju Das , Laurent Pinchart Date: Wed, 24 Jan 2024 12:48:48 -0800 User-Agent: alot/0.10 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 =3D 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 =3D=3D 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(). > >=20 > > Does that mean that the check below clk_prepare_enable() could be remov= ed > > already ? >=20 > Yes, that is correct I will remove in the next version as clk_prepare_ena= ble() is > synchronous as it checks the status to make sure it is turned ON. >=20 > >=20 > > Regarding clock disable, it isn't clear if the API guarantees synchrono= us > > 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 case= s), > > but I think synchronizing the clock disable by waiting for the clock to= be > > actually disabled should be implemented as a helper in CCF. >=20 > +clk. >=20 > Hi Stephen and all, >=20 > Can you please shed some light on this? >=20 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.