Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Sat, 07 Oct 2023 19:54:53 +0200
From:   Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
To:     =?UTF-8?Q?Alvin_=C5=A0ipraga?= <alvin@pqrs.dk>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Andrew Lunn <andrew@lunn.ch>,
        Gregory Clement <gregory.clement@bootlin.com>,
        =?UTF-8?B?77+9aXByYWdh?= <alsi@bang-olufsen.dk>
CC:     Rabeeh Khoury <rabeeh@solid-run.com>,
        Jacob Siverskog <jacob@teenage.engineering>,
        Sergej Sawazki <sergej@taudac.com>, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: =?US-ASCII?Q?Re=3A_=5BPATCH_v2_4/4=5D_clk=3A_si5351=3A_all?= =?US-ASCII?Q?ow_PLLs_to_be_adjusted_without_reset?=
User-Agent: K-9 Mail for Android
In-Reply-To: <20231004063712.3348978-5-alvin@pqrs.dk>
References: <20231004063712.3348978-1-alvin@pqrs.dk> <20231004063712.3348978-5-alvin@pqrs.dk>
Message-ID: <C2AD7957-E361-4B27-BCA3-B807314752C4@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Alvin,

thanks for the patch=2E In general, I am fine with the change as default b=
ehavior is to do what it did before=2E

So,
Acked-by: <sebastian=2Ehesselbarth@gmail=2Ecom>
for the functional changes=2E

For the DT changes you'll need Rob, Stephen, Michael's approval more than =
mine=2E

However, as Jacob and Sergej already noticed on their patches, I cannot sp=
end enough time for maintaining the driver anymore=2E

Is there anyone volunteering to pick maintainership up?

Regards,
  Sebastian

(Hopefully plain/text now)


Am 4=2E Oktober 2023 08:35:30 MESZ schrieb "Alvin =C5=A0ipraga" <alvin@pqr=
s=2Edk>:
>From: Alvin =C5=A0ipraga <alsi@bang-olufsen=2Edk>
>
>Introduce a new PLL reset mode flag which controls whether or not to
>reset a PLL after adjusting its rate=2E The mode can be configured throug=
h
>platform data or device tree=2E
>
>Since commit 6dc669a22c77 ("clk: si5351: Add PLL soft reset"), the
>driver unconditionally resets a PLL whenever its rate is adjusted=2E
>The rationale was that a PLL reset was required to get three outputs
>working at the same time=2E Before this change, the driver never reset th=
e
>PLLs=2E
>
>Commit b26ff127c52c ("clk: si5351: Apply PLL soft reset before enabling
>the outputs") subsequently introduced an option to reset the PLL when
>enabling a clock output that sourced it=2E Here, the rationale was that
>this is required to get a deterministic phase relationship between
>multiple output clocks=2E
>
>This clearly shows that it is useful to reset the PLLs in applications
>where multiple clock outputs are used=2E However, the Si5351 also allows
>for glitch-free rate adjustment of its PLLs if one avoids resetting the
>PLL=2E In our audio application where a single Si5351 clock output is use=
d
>to supply a runtime adjustable bit clock, this unconditional PLL reset
>behaviour introduces unwanted glitches in the clock output=2E
>
>It would appear that the problem being solved in the former commit
>may be solved by using the optional device tree property introduced in
>the latter commit, obviating the need for an unconditional PLL reset
>after rate adjustment=2E But it's not OK to break the default behaviour o=
f
>the driver, and it cannot be assumed that all device trees are using the
>property introduced in the latter commit=2E Hence, the new behaviour is
>made opt-in=2E
>
>Cc: Sebastian Hesselbarth <sebastian=2Ehesselbarth@gmail=2Ecom>
>Cc: Rabeeh Khoury <rabeeh@solid-run=2Ecom>
>Cc: Jacob Siverskog <jacob@teenage=2Eengineering>
>Cc: Sergej Sawazki <sergej@taudac=2Ecom>
>Signed-off-by: Alvin =C5=A0ipraga <alsi@bang-olufsen=2Edk>
>---
> drivers/clk/clk-si5351=2Ec             | 47 ++++++++++++++++++++++++++--
> include/linux/platform_data/si5351=2Eh |  2 ++
> 2 files changed, 46 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/clk/clk-si5351=2Ec b/drivers/clk/clk-si5351=2Ec
>index 00fb9b09e030=2E=2E95d7afb8cfc6 100644
>--- a/drivers/clk/clk-si5351=2Ec
>+++ b/drivers/clk/clk-si5351=2Ec
>@@ -506,6 +506,8 @@ static int si5351_pll_set_rate(struct clk_hw *hw, uns=
igned long rate,
> {
> 	struct si5351_hw_data *hwdata =3D
> 		container_of(hw, struct si5351_hw_data, hw);
>+	struct si5351_platform_data *pdata =3D
>+		hwdata->drvdata->client->dev=2Eplatform_data;
> 	u8 reg =3D (hwdata->num =3D=3D 0) ? SI5351_PLLA_PARAMETERS :
> 		SI5351_PLLB_PARAMETERS;
>=20
>@@ -518,9 +520,10 @@ static int si5351_pll_set_rate(struct clk_hw *hw, un=
signed long rate,
> 		(hwdata->params=2Ep2 =3D=3D 0) ? SI5351_CLK_INTEGER_MODE : 0);
>=20
> 	/* Do a pll soft reset on the affected pll */
>-	si5351_reg_write(hwdata->drvdata, SI5351_PLL_RESET,
>-			 hwdata->num =3D=3D 0 ? SI5351_PLL_RESET_A :
>-					    SI5351_PLL_RESET_B);
>+	if (pdata->pll_reset[hwdata->num])
>+		si5351_reg_write(hwdata->drvdata, SI5351_PLL_RESET,
>+				 hwdata->num =3D=3D 0 ? SI5351_PLL_RESET_A :
>+						    SI5351_PLL_RESET_B);
>=20
> 	dev_dbg(&hwdata->drvdata->client->dev,
> 		"%s - %s: p1 =3D %lu, p2 =3D %lu, p3 =3D %lu, parent_rate =3D %lu, rat=
e =3D %lu\n",
>@@ -1222,6 +1225,44 @@ static int si5351_dt_parse(struct i2c_client *clie=
nt,
> 		}
> 	}
>=20
>+	/*
>+	 * Parse PLL reset mode=2E For compatibility with older device trees, t=
he
>+	 * default is to always reset a PLL after setting its rate=2E
>+	 */
>+	pdata->pll_reset[0] =3D true;
>+	pdata->pll_reset[1] =3D true;
>+
>+	of_property_for_each_u32(np, "silabs,pll-reset-mode", prop, p, num) {
>+		if (num >=3D 2) {
>+			dev_err(&client->dev,
>+				"invalid pll %d on pll-reset-mode prop\n", num);
>+			return -EINVAL;
>+		}
>+
>+		p =3D of_prop_next_u32(prop, p, &val);
>+		if (!p) {
>+			dev_err(&client->dev,
>+				"missing pll-reset-mode for pll %d\n", num);
>+			return -EINVAL;
>+		}
>+
>+		switch (val) {
>+		case 0:
>+			/* Reset PLL whenever its rate is adjusted */
>+			pdata->pll_reset[num] =3D true;
>+			break;
>+		case 1:
>+			/* Don't reset PLL whenever its rate is adjusted */
>+			pdata->pll_reset[num] =3D false;
>+			break;
>+		default:
>+			dev_err(&client->dev,
>+				"invalid pll-reset-mode %d for pll %d\n", val,
>+				num);
>+			return -EINVAL;
>+		}
>+	}
>+
> 	/* per clkout properties */
> 	for_each_child_of_node(np, child) {
> 		if (of_property_read_u32(child, "reg", &num)) {
>diff --git a/include/linux/platform_data/si5351=2Eh b/include/linux/platf=
orm_data/si5351=2Eh
>index c71a2dd66143=2E=2E5f412a615532 100644
>--- a/include/linux/platform_data/si5351=2Eh
>+++ b/include/linux/platform_data/si5351=2Eh
>@@ -105,10 +105,12 @@ struct si5351_clkout_config {
>  * @clk_xtal: xtal input clock
>  * @clk_clkin: clkin input clock
>  * @pll_src: array of pll source clock setting
>+ * @pll_reset: array indicating if plls should be reset after setting th=
e rate
>  * @clkout: array of clkout configuration
>  */
> struct si5351_platform_data {
> 	enum si5351_pll_src pll_src[2];
>+	bool pll_reset[2];
> 	struct si5351_clkout_config clkout[8];
> };
>=20