[1/2] dt-bindings: clk: rs9: Add Renesas 9-series I2C PCIe clock generator

Add binding for Renesas 9-series PCIe clock generators. This binding
is designed to support 9FGV/9DBV/9DMV/9FGL/9DML/9QXL/9SQ series I2C
PCIe clock generators, currently the only tested and supported chip
is 9FGV0241.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: devicetree@vger.kernel.org
To: linux-clk@vger.kernel.org
---
 .../bindings/clock/renesas,9series.yaml       | 102 ++++++++++++++++++
 1 file changed, 102 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,9series.yaml

Quoting Marek Vasut (2022-02-13 09:33:09)
> diff --git a/Documentation/devicetree/bindings/clock/renesas,9series.yaml b/Documentation/devicetree/bindings/clock/renesas,9series.yaml
> new file mode 100644
> index 0000000000000..774053748d9f0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,9series.yaml
> @@ -0,0 +1,102 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/renesas,9series.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binding for Renesas 9-series I2C PCIe clock generators
> +
> +description: |
> +  The Renesas 9-series are I2C PCIe clock generators providing
> +  from 1 to 20 output clocks.
> +
> +  When referencing the provided clock in the DT using phandle
> +  and clock specifier, the following mapping applies:
> +
> +  - 9FGV0241:
> +    0 -- DIF0
> +    1 -- DIF1
> +
> +maintainers:
> +  - Marek Vasut <marex@denx.de>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - renesas,9fgv0241
> +
> +  reg:
> +    description: I2C device address
> +    enum: [ 0x68, 0x6a ]
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  clocks:
> +    items:
> +      - description: XTal input clock
> +
> +  renesas,out-amplitude:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 600000, 700000, 800000, 900000 ]
> +    description: Output clock signal amplitude in uV
> +
> +  renesas,out-spread-spectrum:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 100000, 99750, 99500 ]
> +    description: Output clock down spread in pcm
> +
> +patternProperties:
> +  "^DIF[0-19]$":
> +    type: object
> +    description:
> +      Description of one of the outputs (DIF0..DIF19).
> +    properties:
> +      renesas,slew-rate:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [ 2000000, 3000000 ]
> +        description: Output clock slew rate select in V/ns
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#clock-cells'

Can it operate without an input xtal?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    /* 25MHz reference crystal */
> +    ref25: ref25m {
> +        compatible = "fixed-clock";
> +        #clock-cells = <0>;
> +        clock-frequency = <25000000>;
> +    };
> +
> +    i2c@0 {
> +        reg = <0x0 0x100>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rs9: clock-generator@6a {
> +            compatible = "renesas,9fgv0241";
> +            reg = <0x6a>;
> +            #clock-cells = <1>;
> +
> +            clocks = <&ref25m>;
> +
> +            DIF0 {
> +                renesas,slew-rate = <3000000>;
> +            };
> +        };
> +    };
> +
> +    /* Consumer referencing the 9FGV0241 pin DIF0 */

Consumers are typically left out of clk bindings.

> +    consumer {
> +        /* ... */
> +        clocks = <&rs9 0>;
> +        /* ... */
> +    };
> +
> +...
> -- 
> 2.34.1
>

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", &amp);
> +       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;
> +}

On 2/18/22 00:39, Stephen Boyd wrote:
> Quoting Marek Vasut (2022-02-13 09:33:09)
>> diff --git a/Documentation/devicetree/bindings/clock/renesas,9series.yaml b/Documentation/devicetree/bindings/clock/renesas,9series.yaml
>> new file mode 100644
>> index 0000000000000..774053748d9f0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/renesas,9series.yaml
>> @@ -0,0 +1,102 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/renesas,9series.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Binding for Renesas 9-series I2C PCIe clock generators
>> +
>> +description: |
>> +  The Renesas 9-series are I2C PCIe clock generators providing
>> +  from 1 to 20 output clocks.
>> +
>> +  When referencing the provided clock in the DT using phandle
>> +  and clock specifier, the following mapping applies:
>> +
>> +  - 9FGV0241:
>> +    0 -- DIF0
>> +    1 -- DIF1
>> +
>> +maintainers:
>> +  - Marek Vasut <marex@denx.de>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - renesas,9fgv0241
>> +
>> +  reg:
>> +    description: I2C device address
>> +    enum: [ 0x68, 0x6a ]
>> +
>> +  '#clock-cells':
>> +    const: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: XTal input clock
>> +
>> +  renesas,out-amplitude:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [ 600000, 700000, 800000, 900000 ]
>> +    description: Output clock signal amplitude in uV
>> +
>> +  renesas,out-spread-spectrum:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [ 100000, 99750, 99500 ]
>> +    description: Output clock down spread in pcm
>> +
>> +patternProperties:
>> +  "^DIF[0-19]$":
>> +    type: object
>> +    description:
>> +      Description of one of the outputs (DIF0..DIF19).
>> +    properties:
>> +      renesas,slew-rate:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        enum: [ 2000000, 3000000 ]
>> +        description: Output clock slew rate select in V/ns
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#clock-cells'
> 
> Can it operate without an input xtal?

Not to my knowledge.

[...]

On 2/18/22 00:45, Stephen Boyd wrote:
> 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?

The drivers/clk/renesas/ is for renesas SoC (R-Car/RZ/...),
this chip is different group (it's probably even IDT PLL IP).

[...]

>> +#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.

This one is for of_device_get_match_data()

[...]

>> +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?

Remnant from development, dropped.

>> +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.

This one line I don't understand -- can you expand on what you expect me 
to do here ?

>> +       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;
>> +}

[...]

Quoting Marek Vasut (2022-02-17 17:09:26)
> On 2/18/22 00:39, Stephen Boyd wrote:
> > Quoting Marek Vasut (2022-02-13 09:33:09)
> >> diff --git a/Documentation/devicetree/bindings/clock/renesas,9series.yaml b/Documentation/devicetree/bindings/clock/renesas,9series.yaml
> >> new file mode 100644
> >> index 0000000000000..774053748d9f0
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/renesas,9series.yaml
> >> @@ -0,0 +1,102 @@
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - '#clock-cells'
> > 
> > Can it operate without an input xtal?
> 
> Not to my knowledge.
> 

Ok please make it required property then.

Quoting Marek Vasut (2022-02-17 17:26:22)
> On 2/18/22 00:45, Stephen Boyd wrote:
> > 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?
> 
> The drivers/clk/renesas/ is for renesas SoC (R-Car/RZ/...),
> this chip is different group (it's probably even IDT PLL IP).

Ah ok so it's not a renesas SoC but a renesas IP?

> 
> [...]
> 
> >> +#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.
> 
> This one is for of_device_get_match_data()
> 

So it's going away?

> 
> >> +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.
> 
> This one line I don't understand -- can you expand on what you expect me 
> to do here ?

Use 'struct clk_parent_data' and set .index to 0 so that when
registering the clk you don't need to get the parent clk name.

> 
> >> +       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);

To do that it looks like maybe we'll need to export
__clk_hw_register_fixed_factor() and introduces some sort of
clk_hw_register_fixed_factor_parent_data() API.

> >> +               if (IS_ERR(hw))
> >> +                       return PTR_ERR(hw);
> >> +
> >> +               rs9->clk_dif[i] = hw;

On 2/18/22 23:15, Stephen Boyd wrote:

Hi,

>>>> @@ -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?
>>
>> The drivers/clk/renesas/ is for renesas SoC (R-Car/RZ/...),
>> this chip is different group (it's probably even IDT PLL IP).
> 
> Ah ok so it's not a renesas SoC but a renesas IP?

I suspect it is IDT IP, since the datasheet looks similar to what 
versaclock datasheets used to look like, except the register layout is 
much simpler and the registers are completely different. So it _might_ 
be some mutation of the IDT PLL, and it is now owned by renesas.

The renesas SoC clock IP is something entirely different from the IP 
used here.

>>>> +#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.
>>
>> This one is for of_device_get_match_data()
>>
> 
> So it's going away?

Yes

[...]

>>> Use clk_parent_data please.
>>
>> This one line I don't understand -- can you expand on what you expect me
>> to do here ?
> 
> Use 'struct clk_parent_data' and set .index to 0 so that when
> registering the clk you don't need to get the parent clk name.
> 
>>
>>>> +       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);
> 
> To do that it looks like maybe we'll need to export
> __clk_hw_register_fixed_factor() and introduces some sort of
> clk_hw_register_fixed_factor_parent_data() API.

Setting parent_clk to NULL should be enough.

[...]

Quoting Marek Vasut (2022-02-18 17:11:04)
> On 2/18/22 23:15, Stephen Boyd wrote:
> >>
> >>>> +       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);
> > 
> > To do that it looks like maybe we'll need to export
> > __clk_hw_register_fixed_factor() and introduces some sort of
> > clk_hw_register_fixed_factor_parent_data() API.
> 
> Setting parent_clk to NULL should be enough.
> 

Perfect, but also weird. I worry that's a bug that snuck in. Probably a
good idea to not rely on that.

On 2/19/22 04:05, Stephen Boyd wrote:
> Quoting Marek Vasut (2022-02-18 17:11:04)
>> On 2/18/22 23:15, Stephen Boyd wrote:
>>>>
>>>>>> +       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);
>>>
>>> To do that it looks like maybe we'll need to export
>>> __clk_hw_register_fixed_factor() and introduces some sort of
>>> clk_hw_register_fixed_factor_parent_data() API.
>>
>> Setting parent_clk to NULL should be enough.
>>
> 
> Perfect, but also weird. I worry that's a bug that snuck in. Probably a
> good idea to not rely on that.

No, I was wrong, the index=0 is right, and it is already fixed in V2.