| Message ID | 20240828101503.1478491-5-heiko@sntech.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Binding and driver for gated-fixed-clocks | expand |
Quoting Heiko Stuebner (2024-08-28 03:15:02) > diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c > index cda362a2eca0..8bcdef340b4c 100644 > --- a/drivers/clk/clk-gpio.c > +++ b/drivers/clk/clk-gpio.c > @@ -239,3 +240,184 @@ static struct platform_driver gpio_clk_driver = { > }, > }; > builtin_platform_driver(gpio_clk_driver); > + > +/** > + * DOC: gated fixed clock, controlled with a gpio output and a regulator > + * Traits of this clock: > + * prepare - clk_prepare and clk_unprepare are function & control regulator > + * optionally a gpio that can sleep > + * enable - clk_enable and clk_disable are functional & control gpio > + * rate - rate is fixed and set on clock generation Maybe 'clock registration' > + * parent - fixed clock is a root clock and has no parent. Not sure why this one gets the period while other lines above don't. > + */ > + > +/** > + * struct clk_gate_fixed - gated-fixed-clock > + * > + * clk_gpio: instance of clk_gpio for gate-gpio > + * supply: supply regulator > + * rate: fixed rate > + */ > +struct clk_gated_fixed { > + struct clk_gpio clk_gpio; > + struct regulator *supply; > + u32 rate; unsigned long rate to match the CCF type please. > +}; > + > +#define to_clk_gated_fixed(_clk_gpio) container_of(_clk_gpio, struct clk_gated_fixed, clk_gpio) > + > +static unsigned long clk_gated_fixed_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + return to_clk_gated_fixed(to_clk_gpio(hw))->rate; > +} > + > +static int clk_gated_fixed_prepare(struct clk_hw *hw) > +{ > + struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw)); > + > + if (!clk->supply) > + return 0; > + > + return regulator_enable(clk->supply); > +} > + > +static void clk_gated_fixed_unprepare(struct clk_hw *hw) > +{ > + struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw)); > + > + if (!clk->supply) > + return; > + > + regulator_disable(clk->supply); > +} > + > +static int clk_gated_fixed_is_prepared(struct clk_hw *hw) > +{ > + struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw)); > + > + if (!clk->supply) > + return true; > + > + return regulator_is_enabled(clk->supply); > +} > + > +/* > + * Fixed gated clock with non-sleeping gpio. > + * > + * Prepare operation turns on the supply regulator > + * and the enable operation switches the enable-gpio. > + */ > +const struct clk_ops clk_gated_fixed_ops = { static > + .prepare = clk_gated_fixed_prepare, > + .unprepare = clk_gated_fixed_unprepare, > + .is_prepared = clk_gated_fixed_is_prepared, > + .enable = clk_gpio_gate_enable, > + .disable = clk_gpio_gate_disable, > + .is_enabled = clk_gpio_gate_is_enabled, > + .recalc_rate = clk_gated_fixed_recalc_rate, > +}; > + > +static int clk_sleeping_gated_fixed_prepare(struct clk_hw *hw) > +{ > + int ret; > + > + ret = clk_gated_fixed_prepare(hw); > + if (ret) > + return ret; > + > + ret = clk_sleeping_gpio_gate_prepare(hw); > + if (ret) > + clk_gated_fixed_unprepare(hw); > + > + return ret; > +} > + > +static void clk_sleeping_gated_fixed_unprepare(struct clk_hw *hw) > +{ > + clk_gated_fixed_unprepare(hw); > + clk_sleeping_gpio_gate_unprepare(hw); > +} > + > +/* > + * Fixed gated clock with non-sleeping gpio. > + * > + * Enabling the supply regulator and switching the enable-gpio happens > + * both in the prepare step. > + * is_prepared only needs to check the gpio state, as toggling the > + * gpio is the last step when preparing. > + */ > +const struct clk_ops clk_sleeping_gated_fixed_ops = { static > + .prepare = clk_sleeping_gated_fixed_prepare, > + .unprepare = clk_sleeping_gated_fixed_unprepare, > + .is_prepared = clk_sleeping_gpio_gate_is_prepared, > + .recalc_rate = clk_gated_fixed_recalc_rate, > +}; > + > +static int clk_gated_fixed_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct clk_gated_fixed *clk; > + const struct clk_ops *ops; > + const char *clk_name; > + int ret; > + > + clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL); > + if (!clk) > + return -ENOMEM; > + > + if (device_property_read_u32(dev, "clock-frequency", &clk->rate)) Why not return the error code? > + return dev_err_probe(dev, -EIO, "failed to get clock-frequency"); Missing newline on printk. > + > + ret = device_property_read_string(dev, "clock-output-names", &clk_name); > + if (ret) > + clk_name = fwnode_get_name(dev->fwnode); > + > + clk->supply = devm_regulator_get_optional(dev, "vdd"); > + if (IS_ERR(clk->supply)) { > + if (PTR_ERR(clk->supply) != -ENODEV) > + return dev_err_probe(dev, PTR_ERR(clk->supply), > + "failed to get regulator\n"); > + clk->supply = NULL; > + } > + > + clk->clk_gpio.gpiod = devm_gpiod_get_optional(dev, "enable", > + GPIOD_OUT_LOW); > + if (IS_ERR(clk->clk_gpio.gpiod)) > + return dev_err_probe(dev, PTR_ERR(clk->clk_gpio.gpiod), > + "failed to get gpio\n"); > + > + if (gpiod_cansleep(clk->clk_gpio.gpiod)) > + ops = &clk_sleeping_gated_fixed_ops; > + else > + ops = &clk_gated_fixed_ops; > + > + clk->clk_gpio.hw.init = CLK_HW_INIT_NO_PARENT(clk_name, ops, 0); > + > + /* register the clock */ > + ret = devm_clk_hw_register(dev, &clk->clk_gpio.hw); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to register clock\n"); > + > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, > + &clk->clk_gpio.hw); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to register clock provider\n"); > + > + return 0; > +} > + > +static const struct of_device_id gated_fixed_clk_match_table[] = { > + { .compatible = "gated-fixed-clock" }, Add a sentinel. > +}; > + > +static struct platform_driver gated_fixed_clk_driver = { > + .probe = clk_gated_fixed_probe, > + .driver = { > + .name = "gated-fixed-clk", > + .of_match_table = gated_fixed_clk_match_table, > + }, > +}; > +builtin_platform_driver(gated_fixed_clk_driver); The comment above builtin_platform_driver says "Each driver may only use this macro once". Seems that we need to expand the macro.
Am Mittwoch, 28. August 2024, 20:30:51 CEST schrieb Stephen Boyd: > Quoting Heiko Stuebner (2024-08-28 03:15:02) [leaving out all the "will fix" parts :-) ] > > +static struct platform_driver gated_fixed_clk_driver = { > > + .probe = clk_gated_fixed_probe, > > + .driver = { > > + .name = "gated-fixed-clk", > > + .of_match_table = gated_fixed_clk_match_table, > > + }, > > +}; > > +builtin_platform_driver(gated_fixed_clk_driver); > > The comment above builtin_platform_driver says "Each driver may only use > this macro once". Seems that we need to expand the macro. each _driver_, not each file is the important point I think. Looking at the code generation, it just wants to use the name of the driver struct for generating the init functions. So in the builtin_driver macro [0] it wants to use the gated_fixed_clk_driver to create the init-function as gated_fixed_clk_driver_init() hence anybody using the macro a second time for the same driver would create that function two times. Also as can be seen with the imx gpc driver [1], the two-drivers in the same file is already in use. I've also done a practical test with that and did [2], which resulted in both drivers getting registered as expected: [ 0.132087] ----init gpio_clk_driver [ 0.132160] ----init gated_fixed_clk_driver So not sure, if I misinterpreted your comment, but I don't think changes are necessary for this portion. Heiko [0] https://elixir.bootlin.com/linux/v6.10.8/source/include/linux/device/driver.h#L284 [1] https://elixir.bootlin.com/linux/v6.10.8/source/drivers/pmdomain/imx/gpc.c#L239 https://elixir.bootlin.com/linux/v6.10.8/source/drivers/pmdomain/imx/gpc.c#L556 [2] diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h index 1fc8b68786de..e306f554cd0f 100644 --- a/include/linux/device/driver.h +++ b/include/linux/device/driver.h @@ -284,6 +284,7 @@ module_exit(__driver##_exit); #define builtin_driver(__driver, __register, ...) \ static int __init __driver##_init(void) \ { \ + printk("----init %s\n", __stringify(__driver)); \ return __register(&(__driver) , ##__VA_ARGS__); \ } \ device_initcall(__driver##_init);
Quoting Heiko Stübner (2024-09-05 15:48:35) > Am Mittwoch, 28. August 2024, 20:30:51 CEST schrieb Stephen Boyd: > > Quoting Heiko Stuebner (2024-08-28 03:15:02) > > [leaving out all the "will fix" parts :-) ] > > > > +static struct platform_driver gated_fixed_clk_driver = { > > > + .probe = clk_gated_fixed_probe, > > > + .driver = { > > > + .name = "gated-fixed-clk", > > > + .of_match_table = gated_fixed_clk_match_table, > > > + }, > > > +}; > > > +builtin_platform_driver(gated_fixed_clk_driver); > > > > The comment above builtin_platform_driver says "Each driver may only use > > this macro once". Seems that we need to expand the macro. > > each _driver_, not each file is the important point I think. Ok!
diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c index cda362a2eca0..8bcdef340b4c 100644 --- a/drivers/clk/clk-gpio.c +++ b/drivers/clk/clk-gpio.c @@ -17,6 +17,7 @@ #include <linux/device.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/regulator/consumer.h> /** * DOC: basic gpio gated clock which can be enabled and disabled @@ -239,3 +240,184 @@ static struct platform_driver gpio_clk_driver = { }, }; builtin_platform_driver(gpio_clk_driver); + +/** + * DOC: gated fixed clock, controlled with a gpio output and a regulator + * Traits of this clock: + * prepare - clk_prepare and clk_unprepare are function & control regulator + * optionally a gpio that can sleep + * enable - clk_enable and clk_disable are functional & control gpio + * rate - rate is fixed and set on clock generation + * parent - fixed clock is a root clock and has no parent. + */ + +/** + * struct clk_gate_fixed - gated-fixed-clock + * + * clk_gpio: instance of clk_gpio for gate-gpio + * supply: supply regulator + * rate: fixed rate + */ +struct clk_gated_fixed { + struct clk_gpio clk_gpio; + struct regulator *supply; + u32 rate; +}; + +#define to_clk_gated_fixed(_clk_gpio) container_of(_clk_gpio, struct clk_gated_fixed, clk_gpio) + +static unsigned long clk_gated_fixed_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + return to_clk_gated_fixed(to_clk_gpio(hw))->rate; +} + +static int clk_gated_fixed_prepare(struct clk_hw *hw) +{ + struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw)); + + if (!clk->supply) + return 0; + + return regulator_enable(clk->supply); +} + +static void clk_gated_fixed_unprepare(struct clk_hw *hw) +{ + struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw)); + + if (!clk->supply) + return; + + regulator_disable(clk->supply); +} + +static int clk_gated_fixed_is_prepared(struct clk_hw *hw) +{ + struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw)); + + if (!clk->supply) + return true; + + return regulator_is_enabled(clk->supply); +} + +/* + * Fixed gated clock with non-sleeping gpio. + * + * Prepare operation turns on the supply regulator + * and the enable operation switches the enable-gpio. + */ +const struct clk_ops clk_gated_fixed_ops = { + .prepare = clk_gated_fixed_prepare, + .unprepare = clk_gated_fixed_unprepare, + .is_prepared = clk_gated_fixed_is_prepared, + .enable = clk_gpio_gate_enable, + .disable = clk_gpio_gate_disable, + .is_enabled = clk_gpio_gate_is_enabled, + .recalc_rate = clk_gated_fixed_recalc_rate, +}; + +static int clk_sleeping_gated_fixed_prepare(struct clk_hw *hw) +{ + int ret; + + ret = clk_gated_fixed_prepare(hw); + if (ret) + return ret; + + ret = clk_sleeping_gpio_gate_prepare(hw); + if (ret) + clk_gated_fixed_unprepare(hw); + + return ret; +} + +static void clk_sleeping_gated_fixed_unprepare(struct clk_hw *hw) +{ + clk_gated_fixed_unprepare(hw); + clk_sleeping_gpio_gate_unprepare(hw); +} + +/* + * Fixed gated clock with non-sleeping gpio. + * + * Enabling the supply regulator and switching the enable-gpio happens + * both in the prepare step. + * is_prepared only needs to check the gpio state, as toggling the + * gpio is the last step when preparing. + */ +const struct clk_ops clk_sleeping_gated_fixed_ops = { + .prepare = clk_sleeping_gated_fixed_prepare, + .unprepare = clk_sleeping_gated_fixed_unprepare, + .is_prepared = clk_sleeping_gpio_gate_is_prepared, + .recalc_rate = clk_gated_fixed_recalc_rate, +}; + +static int clk_gated_fixed_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct clk_gated_fixed *clk; + const struct clk_ops *ops; + const char *clk_name; + int ret; + + clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL); + if (!clk) + return -ENOMEM; + + if (device_property_read_u32(dev, "clock-frequency", &clk->rate)) + return dev_err_probe(dev, -EIO, "failed to get clock-frequency"); + + ret = device_property_read_string(dev, "clock-output-names", &clk_name); + if (ret) + clk_name = fwnode_get_name(dev->fwnode); + + clk->supply = devm_regulator_get_optional(dev, "vdd"); + if (IS_ERR(clk->supply)) { + if (PTR_ERR(clk->supply) != -ENODEV) + return dev_err_probe(dev, PTR_ERR(clk->supply), + "failed to get regulator\n"); + clk->supply = NULL; + } + + clk->clk_gpio.gpiod = devm_gpiod_get_optional(dev, "enable", + GPIOD_OUT_LOW); + if (IS_ERR(clk->clk_gpio.gpiod)) + return dev_err_probe(dev, PTR_ERR(clk->clk_gpio.gpiod), + "failed to get gpio\n"); + + if (gpiod_cansleep(clk->clk_gpio.gpiod)) + ops = &clk_sleeping_gated_fixed_ops; + else + ops = &clk_gated_fixed_ops; + + clk->clk_gpio.hw.init = CLK_HW_INIT_NO_PARENT(clk_name, ops, 0); + + /* register the clock */ + ret = devm_clk_hw_register(dev, &clk->clk_gpio.hw); + if (ret) + return dev_err_probe(dev, ret, + "failed to register clock\n"); + + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, + &clk->clk_gpio.hw); + if (ret) + return dev_err_probe(dev, ret, + "failed to register clock provider\n"); + + return 0; +} + +static const struct of_device_id gated_fixed_clk_match_table[] = { + { .compatible = "gated-fixed-clock" }, +}; + +static struct platform_driver gated_fixed_clk_driver = { + .probe = clk_gated_fixed_probe, + .driver = { + .name = "gated-fixed-clk", + .of_match_table = gated_fixed_clk_match_table, + }, +}; +builtin_platform_driver(gated_fixed_clk_driver);
In contrast to fixed clocks that are described as ungateable, boards sometimes use additional oscillators for things like PCIe reference clocks, that need actual supplies to get enabled and enable-gpios to be toggled for them to work. This adds a driver for those generic gated-fixed-clocks that can show up in schematics looking like ---------------- Enable - | 100MHz,3.3V, | - VDD | 3225 | GND - | | - OUT ---------------- The new driver gets grouped together with the existing gpio-gate and gpio-mux, as it for one re-uses a lot of the gpio-gate functions and also in it's core it's just another gpio-controlled clock, just with a fixed rate and a regulator-supply added in. The regulator-API provides function stubs for the !CONFIG_REGULATOR case, so no special handling is necessary. Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- drivers/clk/clk-gpio.c | 182 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 182 insertions(+)