From: Stephen Boyd <sboyd@kernel.org>
To: Marek Vasut <marex@denx.de>, linux-clk@vger.kernel.org
Cc: devicetree@vger.kernel.org, Marek Vasut <marex@denx.de>,
Michael Turquette <mturquette@baylibre.com>,
Rob Herring <robh+dt@kernel.org>
Subject: Re: [PATCH 2/2] clk: rs9: Add Renesas 9-series PCIe clock generator driver
Date: Thu, 17 Feb 2022 15:45:36 -0800 [thread overview]
Message-ID: <20220217234539.819AEC340E8@smtp.kernel.org> (raw)
In-Reply-To: <20220213173310.152230-2-marex@denx.de>
Quoting Marek Vasut (2022-02-13 09:33:10)
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 6a98291350b64..3ec27842ec779 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_COMMON_CLK_STM32MP157) += clk-stm32mp1.o
> obj-$(CONFIG_COMMON_CLK_TPS68470) += clk-tps68470.o
> obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o
> obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
> +obj-$(CONFIG_COMMON_CLK_RS9_PCIE) += clk-renesas-pcie.o
Is there a reason it doesn't go into drivers/clk/renesas?
> obj-$(CONFIG_COMMON_CLK_VC5) += clk-versaclock5.o
> obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
> obj-$(CONFIG_COMMON_CLK_XGENE) += clk-xgene.o
> diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
> new file mode 100644
> index 0000000000000..0ad132cbe41b8
> --- /dev/null
> +++ b/drivers/clk/clk-renesas-pcie.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for Renesas 9-series PCIe clock generator driver
> + *
> + * The following series can be supported:
> + * - 9FGV/9DBV/9DMV/9FGL/9DML/9QXL/9SQ
> + * Currently supported:
> + * - 9FGV0241
> + *
> + * Copyright (C) 2022 Marek Vasut <marex@denx.de>
> + */
> +
> +#include <linux/clk.h>
Drop this include please.
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
Is this used? If not please remove.
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
Is this used? If not please remove.
> +#include <linux/rational.h>
Is this used? If not please remove.
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define RS9_REG_OE 0x0
> +#define RS9_REG_OE_DIF_OE(n) BIT((n) + 1)
> +#define RS9_REG_SS 0x1
> +#define RS9_REG_SS_AMP_0V6 0x0
> +#define RS9_REG_SS_AMP_0V7 0x1
> +#define RS9_REG_SS_AMP_0V8 0x2
> +#define RS9_REG_SS_AMP_0V9 0x3
> +#define RS9_REG_SS_AMP_MASK 0x3
> +#define RS9_REG_SS_SSC_100 0
> +#define RS9_REG_SS_SSC_M025 (1 << 3)
> +#define RS9_REG_SS_SSC_M050 (3 << 3)
> +#define RS9_REG_SS_SSC_MASK (3 << 3)
> +#define RS9_REG_SS_SSC_LOCK BIT(5)
> +#define RS9_REG_SR 0x2
> +#define RS9_REG_SR_2V0_DIF(n) 0
> +#define RS9_REG_SR_3V0_DIF(n) BIT((n) + 1)
> +#define RS9_REG_SR_DIF_MASK(n) BIT((n) + 1)
> +#define RS9_REG_REF 0x3
> +#define RS9_REG_REF_OE BIT(4)
> +#define RS9_REG_REF_OD BIT(5)
> +#define RS9_REG_REF_SR_SLOWEST 0
> +#define RS9_REG_REF_SR_SLOW (1 << 6)
> +#define RS9_REG_REF_SR_FAST (2 << 6)
> +#define RS9_REG_REF_SR_FASTER (3 << 6)
> +#define RS9_REG_VID 0x5
> +#define RS9_REG_DID 0x6
> +#define RS9_REG_BCP 0x7
> +
> +/* Supported Renesas 9-series models. */
> +enum rs9_model {
> + RENESAS_9FGV0241,
> +};
> +
> +/* Structure to describe features of a particular 9-series model */
> +struct rs9_chip_info {
> + const enum rs9_model model;
> + unsigned int num_clks;
> +};
> +
> +struct rs9_driver_data {
> + struct i2c_client *client;
> + struct regmap *regmap;
> + const struct rs9_chip_info *chip_info;
> + struct clk *pin_xin;
> + struct clk_hw *clk_dif[2];
> + u8 pll_amplitude;
> + u8 pll_ssc;
> + u8 clk_dif_sr;
> +};
> +
> +/*
> + * Renesas 9-series i2c regmap
> + */
> +static const struct regmap_range rs9_readable_ranges[] = {
> + regmap_reg_range(RS9_REG_OE, RS9_REG_REF),
> + regmap_reg_range(RS9_REG_VID, RS9_REG_BCP),
> +};
> +
> +static const struct regmap_access_table rs9_readable_table = {
> + .yes_ranges = rs9_readable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(rs9_readable_ranges),
> +};
> +
> +static const struct regmap_range rs9_writeable_ranges[] = {
> + regmap_reg_range(RS9_REG_OE, RS9_REG_REF),
> + regmap_reg_range(RS9_REG_BCP, RS9_REG_BCP),
> +};
> +
> +static const struct regmap_access_table rs9_writeable_table = {
> + .yes_ranges = rs9_writeable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(rs9_writeable_ranges),
> +};
> +
> +static const struct regmap_config rs9_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .cache_type = REGCACHE_RBTREE,
> + .max_register = 0x8,
That's a huge cache!
> + .rd_table = &rs9_readable_table,
> + .wr_table = &rs9_writeable_table,
> +};
> +
> +static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
> +{
> + struct i2c_client *client = rs9->client;
> + unsigned char *name = "DIF0";
> + struct device_node *np;
> + int ret;
> + u32 sr;
> +
> + /* Set defaults */
> + rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
> + rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx);
> +
> + name[3] += idx;
> + np = of_get_child_by_name(client->dev.of_node, name);
> + if (!np)
> + return 0;
> +
> + /* Output clock slew rate */
> + ret = of_property_read_u32(np, "renesas,slew-rate", &sr);
Put of_node_put(np) here please
> + if (!ret) {
> + if (sr == 2000000) { /* 2V/ns */
> + rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
> + rs9->clk_dif_sr |= RS9_REG_SR_2V0_DIF(idx);
> + } else if (sr == 3000000) { /* 3V/ns (default) */
> + rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
> + rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx);
> + } else
> + ret = dev_err_probe(&client->dev, -EINVAL,
> + "Invalid renesas,slew-rate value\n");
> + }
> +
> + of_node_put(np);
> +
> + return ret;
> +}
> +
> +static int rs9_get_common_config(struct rs9_driver_data *rs9)
> +{
> + struct i2c_client *client = rs9->client;
> + struct device_node *np = client->dev.of_node;
> + unsigned int amp, ssc;
> + int ret;
> +
> + /* Set defaults */
> + rs9->pll_amplitude = RS9_REG_SS_AMP_0V7;
> + rs9->pll_ssc = RS9_REG_SS_SSC_100;
> +
> + /* Output clock amplitude */
> + ret = of_property_read_u32(np, "renesas,out-amplitude", &);
> + if (!ret) {
> + if (amp == 600000) /* 0.6V */
> + rs9->pll_amplitude = RS9_REG_SS_AMP_0V6;
> + else if (amp == 700000) /* 0.7V (default) */
> + rs9->pll_amplitude = RS9_REG_SS_AMP_0V7;
> + else if (amp == 800000) /* 0.8V */
> + rs9->pll_amplitude = RS9_REG_SS_AMP_0V8;
> + else if (amp == 900000) /* 0.9V */
> + rs9->pll_amplitude = RS9_REG_SS_AMP_0V9;
> + else
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Invalid renesas,out-amplitude value\n");
> + }
> +
> + /* Output clock spread spectrum */
> + ret = of_property_read_u32(np, "renesas,out-spread-spectrum", &ssc);
> + if (!ret) {
> + if (ssc == 100000) /* 100% ... no spread (default) */
> + rs9->pll_ssc = RS9_REG_SS_SSC_100;
> + else if (ssc == 99750) /* -0.25% ... down spread */
> + rs9->pll_ssc = RS9_REG_SS_SSC_M025;
> + else if (ssc == 99500) /* -0.50% ... down spread */
> + rs9->pll_ssc = RS9_REG_SS_SSC_M050;
> + else
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Invalid renesas,out-spread-spectrum value\n");
> + }
> +
> + return 0;
> +}
> +
> +static void rs9_update_config(struct rs9_driver_data *rs9)
> +{
> + int i;
> +
> + /* If amplitude is non-default, update it. */
> + if (rs9->pll_amplitude != RS9_REG_SS_AMP_0V7) {
> + regmap_update_bits(rs9->regmap, RS9_REG_SS, RS9_REG_SS_AMP_MASK,
> + rs9->pll_amplitude);
> + }
> +
> + /* If SSC is non-default, update it. */
> + if (rs9->pll_ssc != RS9_REG_SS_SSC_100) {
> + regmap_update_bits(rs9->regmap, RS9_REG_SS, RS9_REG_SS_SSC_MASK,
> + rs9->pll_ssc);
> + }
> +
> + for (i = 0; i < rs9->chip_info->num_clks; i++) {
> + if (rs9->clk_dif_sr & RS9_REG_SR_3V0_DIF(i))
> + continue;
> +
> + regmap_update_bits(rs9->regmap, RS9_REG_SR, RS9_REG_SR_3V0_DIF(i),
> + rs9->clk_dif_sr & RS9_REG_SR_3V0_DIF(i));
> + }
> +}
> +
> +static struct clk_hw *
> +rs9_of_clk_get(struct of_phandle_args *clkspec, void *data)
> +{
> + struct rs9_driver_data *rs9 = data;
> + unsigned int idx = clkspec->args[0];
> +
> + return rs9->clk_dif[idx];
> +}
> +
> +static const struct of_device_id clk_rs9_of_match[];
Why the forward declare?
> +
> +static int rs9_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct device_node *np = client->dev.of_node;
> + unsigned char *name = "DIF0";
> + struct rs9_driver_data *rs9;
> + const char *parent_clk;
> + struct clk_hw *hw;
> + int i, ret;
> +
> + rs9 = devm_kzalloc(&client->dev, sizeof(*rs9), GFP_KERNEL);
> + if (!rs9)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, rs9);
> + rs9->client = client;
> + rs9->chip_info = of_device_get_match_data(&client->dev);
Check for NULL? Use device_get_match_data()? Or does that not work for
i2c devices?
> +
> + /* Fetch common configuration from DT (if specified) */
> + ret = rs9_get_common_config(rs9);
> + if (ret)
> + return ret;
> +
> + /* Fetch DIFx output configuration from DT (if specified) */
> + for (i = 0; i < rs9->chip_info->num_clks; i++) {
> + ret = rs9_get_output_config(rs9, i);
> + if (ret)
> + return ret;
> + }
> +
> + /* Mandatory XTal */
Oh it's mandatory here but not in the binding?
> + parent_clk = of_clk_get_parent_name(np, 0);
Use clk_parent_data please.
> + if (!parent_clk)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Missing XTal input clock\n");
> +
> + rs9->regmap = devm_regmap_init_i2c(client, &rs9_regmap_config);
> + if (IS_ERR(rs9->regmap))
> + return dev_err_probe(&client->dev, PTR_ERR(rs9->regmap),
> + "Failed to allocate register map\n");
> +
> + /* Register clock */
> + for (i = 0; i < rs9->chip_info->num_clks; i++) {
> + name[3]++;
> + hw = clk_hw_register_fixed_factor(&client->dev, name,
> + parent_clk, 0, 4, 1);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + rs9->clk_dif[i] = hw;
> + }
> +
> + ret = devm_of_clk_add_hw_provider(&client->dev, rs9_of_clk_get, rs9);
> + if (!ret)
> + rs9_update_config(rs9);
> +
> + return ret;
> +}
next prev parent reply other threads:[~2022-02-17 23:56 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-13 17:33 [PATCH 1/2] dt-bindings: clk: rs9: Add Renesas 9-series I2C PCIe clock generator Marek Vasut
2022-02-13 17:33 ` [PATCH 2/2] clk: rs9: Add Renesas 9-series PCIe clock generator driver Marek Vasut
2022-02-17 23:45 ` Stephen Boyd [this message]
2022-02-18 1:26 ` Marek Vasut
2022-02-18 22:15 ` Stephen Boyd
2022-02-19 1:11 ` Marek Vasut
2022-02-19 3:05 ` Stephen Boyd
2022-02-19 3:27 ` Marek Vasut
2022-02-17 23:39 ` [PATCH 1/2] dt-bindings: clk: rs9: Add Renesas 9-series I2C PCIe clock generator Stephen Boyd
2022-02-18 1:09 ` Marek Vasut
2022-02-18 1:28 ` Stephen Boyd
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220217234539.819AEC340E8@smtp.kernel.org \
--to=sboyd@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-clk@vger.kernel.org \
--cc=marex@denx.de \
--cc=mturquette@baylibre.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.