From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [v3 2/2] clk: ls1028a: Add clock driver for Display output interface Date: Thu, 22 Aug 2019 18:26:58 -0700 Message-ID: <20190823012658.DB213233A2@mail.kernel.org> References: <20190822020847.10159-1-wen.he_1@nxp.com> <20190822020847.10159-2-wen.he_1@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20190822020847.10159-2-wen.he_1@nxp.com> Sender: linux-kernel-owner@vger.kernel.org To: Mark Rutland , Michael Turquette , Rob Herring , devicetree@vger.kernel.org, linux-clk@vger.kernel.org, linux-devel@linux.nxdi.nxp.com, linux-kernel@vger.kernel.org Cc: leoyang.li@nxp.com, liviu.dudau@arm.com, Wen He List-Id: devicetree@vger.kernel.org Quoting Wen He (2019-08-21 19:08:47) > Add clock driver for QorIQ LS1028A Display output interfaces(LCD, DPHY), > as implemented in TSMC CLN28HPM PLL, this PLL supports the programmable > integer division and range of the display output pixel clock's 27-594MHz. >=20 > Signed-off-by: Wen He > --- > change in v3: > - remove the OF dependency > - use clk_parent_data instead of parent_name >=20 > drivers/clk/Kconfig | 10 ++ > drivers/clk/Makefile | 1 + > drivers/clk/clk-plldig.c | 283 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 294 insertions(+) > create mode 100644 drivers/clk/clk-plldig.c >=20 > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 801fa1cd0321..ab05f342af04 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -223,6 +223,16 @@ config CLK_QORIQ > This adds the clock driver support for Freescale QorIQ platforms > using common clock framework. > =20 > +config CLK_LS1028A_PLLDIG > + bool "Clock driver for LS1028A Display output" > + depends on ARCH_LAYERSCAPE || COMPILE_TEST > + default ARCH_LAYERSCAPE > + help > + This driver support the Display output interfaces(LCD, DPHY) p= ixel clocks > + of the QorIQ Layerscape LS1028A, as implemented TSMC CLN28HPM = PLL. Not all > + features of the PLL are currently supported by the driver. By = default, > + configured bypass mode with this PLL. > + > config COMMON_CLK_XGENE > bool "Clock driver for APM XGene SoC" > default ARCH_XGENE > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index 0cad76021297..c8e22a764c4d 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -44,6 +44,7 @@ obj-$(CONFIG_COMMON_CLK_OXNAS) +=3D clk-= oxnas.o > obj-$(CONFIG_COMMON_CLK_PALMAS) +=3D clk-palmas.o > obj-$(CONFIG_COMMON_CLK_PWM) +=3D clk-pwm.o > obj-$(CONFIG_CLK_QORIQ) +=3D clk-qoriq.o > +obj-$(CONFIG_CLK_LS1028A_PLLDIG) +=3D clk-plldig.o > obj-$(CONFIG_COMMON_CLK_RK808) +=3D clk-rk808.o > obj-$(CONFIG_COMMON_CLK_HI655X) +=3D clk-hi655x.o > obj-$(CONFIG_COMMON_CLK_S2MPS11) +=3D clk-s2mps11.o > diff --git a/drivers/clk/clk-plldig.c b/drivers/clk/clk-plldig.c > new file mode 100644 > index 000000000000..c5ce80a46fd4 > --- /dev/null > +++ b/drivers/clk/clk-plldig.c > @@ -0,0 +1,283 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright 2019 NXP Please leave this as C style /* */ comment for the NXP part, but comply with the SPDX comment style of // on the first line. > + > +static long plldig_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent) > +{ > + unsigned long parent_rate =3D *parent; > + unsigned long round_rate; > + u32 mult =3D 0, rfdphi1 =3D 0; > + bool found =3D false; > + > + found =3D plldig_is_valid_range(rate, parent_rate, &mult, > + &rfdphi1, &round_rate); > + if (!found) { > + pr_warn("%s: unable to round rate %lu, parent rate :%lu\n= ", > + clk_hw_get_name(hw), rate, parent_rate); > + return 0; This can return an error instead? In fact, you may want to use determine_rate clk op instead. > + } > + > + return round_rate / rfdphi1; > +} > + > +static int plldig_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct clk_plldig *data =3D to_clk_plldig(hw); > + bool valid =3D false; > + unsigned long round_rate =3D 0; > + u32 rfdphi1 =3D 0, val, mult =3D 0, cond =3D 0; > + int ret =3D -ETIMEDOUT; > + > + valid =3D plldig_is_valid_range(rate, parent_rate, &mult, > + &rfdphi1, &round_rate); > + if (!valid) { > + pr_warn("%s: unable to support rate %lu, parent_rate: %lu= \n", > + clk_hw_get_name(hw), rate, parent_rate); Shouldn't determine_rate or round_rate make this impossible to hit in practice? I mean that those ops should prevent the rate from being rounded to such a frequency that it becomes invalid. > + return -EINVAL; > + } > + > + val =3D readl(data->regs + PLLDIG_REG_PLLDV); > + val =3D mult; > + rfdphi1 =3D PLLDIG_SET_RFDPHI1(rfdphi1); > + val |=3D rfdphi1; > + > + writel(val, data->regs + PLLDIG_REG_PLLDV); > + > + /* delay 200us make sure that old lock state is cleared */ > + udelay(200); > + > + /* Wait until PLL is locked or timeout (maximum 1000 usecs) */ > + ret =3D readl_poll_timeout_atomic(data->regs + PLLDIG_REG_PLLSR, = cond, > + cond & PLLDIG_LOCK_MASK, 0, > + USEC_PER_MSEC); > + > + return ret; > +} > + > +static const struct clk_ops plldig_clk_ops =3D { > + .enable =3D plldig_enable, > + .disable =3D plldig_disable, > + .is_enabled =3D plldig_is_enabled, > + .recalc_rate =3D plldig_recalc_rate, > + .round_rate =3D plldig_round_rate, > + .set_rate =3D plldig_set_rate, > +}; [...] > + > + ret =3D devm_clk_hw_register(dev, &data->hw); > + if (ret) { > + dev_err(dev, "failed to register %s clock\n", init.name); > + return ret; > + } > + > + ret =3D devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &d= ata->hw); > + if (ret) > + dev_err(dev, "failed adding the clock provider\n"); > + > + return ret; > +} > + > +static int plldig_clk_remove(struct platform_device *pdev) > +{ > + of_clk_del_provider(pdev->dev.of_node); This isn't required. devm already does it. > + return 0; > +} > + > +static const struct of_device_id plldig_clk_id[] =3D { > + { .compatible =3D "fsl,ls1028a-plldig", .data =3D NULL}, You can leave out the data assignment.