Re: [PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Stephen,

On 17.07.2024 01:28, Stephen Boyd wrote:
> Quoting Claudiu (2024-07-16 03:30:17)
>> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c
>> new file mode 100644
>> index 000000000000..8effe141fc0b
>> --- /dev/null
>> +++ b/drivers/clk/renesas/clk-vbattb.c
>> @@ -0,0 +1,212 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * VBATTB clock driver
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/cleanup.h>
>> +#include <linux/clk.h>
> 
> Prefer clk providers to not be clk consumers.

I added it here to be able to use devm_clk_get_optional() as it was
proposed to me in v1 to avoid adding a new binding for bypass and detect if
it's needed by checking the input clock name.


> 
>> +#include <linux/clk-provider.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
> 
> Is of_platform.h used?
> 
> Include mod_devicetable.h for of_device_id.

Ok.

> 
>> +#include <linux/platform_device.h>
>> +
>> +#define VBATTB_BKSCCR                  0x0
>> +#define VBATTB_BKSCCR_SOSEL            BIT(6)
>> +#define VBATTB_SOSCCR2                 0x8
>> +#define VBATTB_SOSCCR2_SOSTP2          BIT(0)
> [..]
>> +
>> +static int vbattb_clk_probe(struct platform_device *pdev)
>> +{
>> +       struct device_node *np = pdev->dev.of_node;
>> +       struct clk_parent_data parent_data = {};
>> +       struct device *dev = &pdev->dev;
>> +       struct clk_init_data init = {};
>> +       struct vbattb_clk *vbclk;
>> +       u32 load_capacitance;
>> +       struct clk_hw *hw;
>> +       int ret, bypass;
>> +
>> +       vbclk = devm_kzalloc(dev, sizeof(*vbclk), GFP_KERNEL);
>> +       if (!vbclk)
>> +               return -ENOMEM;
>> +
>> +       vbclk->base = devm_platform_ioremap_resource(pdev, 0);
>> +       if (IS_ERR(vbclk->base))
>> +               return PTR_ERR(vbclk->base);
>> +
>> +       bypass = vbattb_clk_need_bypass(dev);
> 
> This is a tri-state bool :(
> 
>> +       if (bypass < 0) {
>> +               return bypass;
>> +       } else if (bypass) {
>> +               parent_data.fw_name = "clkin";
>> +               bypass = VBATTB_BKSCCR_SOSEL;
> 
> And now it is a mask value.
> 
>> +       } else {
>> +               parent_data.fw_name = "xin";
>> +       }
>> +
>> +       ret = of_property_read_u32(np, "renesas,vbattb-load-nanofarads", &load_capacitance);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = vbattb_clk_validate_load_capacitance(vbclk, load_capacitance);
>> +       if (ret)
>> +               return ret;
>> +
>> +       vbattb_clk_update_bits(vbclk->base, VBATTB_BKSCCR, VBATTB_BKSCCR_SOSEL, bypass);
> 
> Please don't overload 'bypass'. Use two variables or a conditional.

OK.

> 
> I also wonder if this is really a mux, 

It's a way to determine what type of clock (crystal oscillator or device
clock) is connected to RTXIN/RTXOUT pins of the module
(the module block diagram at [1]) based on the clock name. Depending on the
type of the clock connected to RTXIN/RTXOUT we need to select the XC or
XBYP as input for the mux at [1].

[1] https://gcdnb.pbrd.co/images/QYsCvhfQlX6n.png


> and either assigned-clock-parents should be used, 
> or the clk_ops should have an init routine that looks at
> which parent is present by determining the index and then use that to
> set the mux. The framework can take care of failing to set the other
> parent when it isn't present.


On the board, at any moment, it will be only one clock as input to the
VBATTB clock (either crystal oscillator or a clock device). If I'm getting
you correctly, this will involve describing both clocks in some scenarios.

E.g. if want to use crystal osc, I can use this DT description:

vbattclk: clock-controller@1c {
	compatible = "renesas,r9a08g045-vbattb-clk";
	reg = <0 0x1c 0 0x10>;
	clocks = <&vbattb_xtal>;
	clock-names = "xin";
	#clock-cells = <0>;
	status = "disabled";
};

vbattb_xtal: vbattb-xtal {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <32768>;
};

If external clock device is to be used, I should describe a fake clock too:

vbattclk: clock-controller@1c {
	compatible = "renesas,r9a08g045-vbattb-clk";
	reg = <0 0x1c 0 0x10>;
	clocks = <&vbattb_xtal>, <&ext_clk>;
	clock-names = "xin", "clkin";
	#clock-cells = <0>;
	status = "disabled";
};

vbattb_xtal: vbattb-xtal {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <0>;
};

ext_clk: ext-clk {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <32768>;
};

Is this what you are suggesting?


> 
>> +
>> +       spin_lock_init(&vbclk->lock);
>> +
>> +       init.name = "vbattclk";
>> +       init.ops = &vbattb_clk_ops;
>> +       init.parent_data = &parent_data;
>> +       init.num_parents = 1;
>> +       init.flags = 0;
>> +
>> +       vbclk->hw.init = &init;
>> +       hw = &vbclk->hw;
>> +
>> +       ret = devm_clk_hw_register(dev, hw);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw);
>> +}
>> +
>> +static const struct of_device_id vbattb_clk_match[] = {
>> +       { .compatible = "renesas,r9a08g045-vbattb-clk" },
>> +       { /* sentinel */ }
>> +};
> 
> Any MODULE_DEVICE_TABLE?

I failed to added it.

Thank you for your review,
Claudiu Beznea