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
