Re: [PATCH v6 2/5] clk: qcom: regmap: add PHY clock source implementation

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 18/05/2022 20:58, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-13 10:53:36)
>> diff --git a/drivers/clk/qcom/clk-regmap-phy-mux.c b/drivers/clk/qcom/clk-regmap-phy-mux.c
>> new file mode 100644
>> index 000000000000..d7a45f7fa1aa
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-regmap-phy-mux.c
>> @@ -0,0 +1,62 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022, Linaro Ltd.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/bitops.h>
>> +#include <linux/regmap.h>
>> +#include <linux/export.h>
> 
> clk-provider.h for clk_hw/clk_ops usage. It helps with grep to identify
> clk providers.
> 
>> +
>> +#include "clk-regmap-phy-mux.h"
> 
> Same for clk-regmap.h, avoid include hell.

I couldn't catch this comment. I think we need clk-regmap.h in 
clk-regmap-phy-mux.h as clk_regmap is a part of defined structure.

>> +
>> +static inline struct clk_regmap_phy_mux *to_clk_regmap_phy_mux(struct clk_hw *hw)
>> +{
>> +       return container_of(to_clk_regmap(hw), struct clk_regmap_phy_mux, clkr);
>> +}
>> +
>> +static int phy_mux_is_enabled(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
>> +       struct clk_regmap *clkr = to_clk_regmap(hw);
>> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
>> +       unsigned int val;
>> +
>> +       regmap_read(clkr->regmap, phy_mux->reg, &val);
>> +       val = (val & mask) >> phy_mux->shift;
> 
> Can this use FIELD_GET?
> 
>> +
>> +       WARN_ON(val != phy_mux->phy_src_val && val != phy_mux->ref_src_val);
>> +
>> +       return val == phy_mux->phy_src_val;
>> +}
>> +
>> +static int phy_mux_enable(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
>> +       struct clk_regmap *clkr = to_clk_regmap(hw);
>> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
>> +       unsigned int val;
>> +
>> +       val = phy_mux->phy_src_val << phy_mux->shift;
> 
> Can this use FIELD_PREP?
> 
>> +
>> +       return regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
>> +}
>> +
>> +static void phy_mux_disable(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
>> +       struct clk_regmap *clkr = to_clk_regmap(hw);
>> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
>> +       unsigned int val;
>> +
>> +       val = phy_mux->ref_src_val << phy_mux->shift;
>> +
>> +       regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
>> +}
>> +
>> +const struct clk_ops clk_regmap_phy_mux_ops = {
>> +       .enable = phy_mux_enable,
>> +       .disable = phy_mux_disable,
>> +       .is_enabled = phy_mux_is_enabled,
>> +};
>> +EXPORT_SYMBOL_GPL(clk_regmap_phy_mux_ops);
>> diff --git a/drivers/clk/qcom/clk-regmap-phy-mux.h b/drivers/clk/qcom/clk-regmap-phy-mux.h
>> new file mode 100644
>> index 000000000000..6260912191c5
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-regmap-phy-mux.h
>> @@ -0,0 +1,37 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2022, Linaro Ltd.
>> + * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> + */
>> +
>> +#ifndef __QCOM_CLK_REGMAP_PHY_MUX_H__
>> +#define __QCOM_CLK_REGMAP_PHY_MUX_H__
>> +
>> +#include <linux/clk-provider.h>
>> +#include "clk-regmap.h"
>> +
>> +/*
>> + * A special clock implementation for PHY pipe and symbols clock sources.
> 
> Remove "special" please. Everything is special :)
> 
>> + *
>> + * If the clock is running off the from-PHY source, report it as enabled.
> 
> from-PHY is @phy_src_val? Maybe add that information like "from-PHY
> source (@phy_src_val)"
> 
>> + * Report it as disabled otherwise (if it uses reference source).
> 
> Same for @ref_src_val
> 
>> + *
>> + * This way the PHY will disable the pipe clock before turning off the GDSC,
>> + * which in turn would lead to disabling corresponding pipe_clk_src (and thus
>> + * it being parked to a safe, reference clock source). And vice versa, after
>> + * enabling the GDSC the PHY will enable the pipe clock, which would cause
>> + * pipe_clk_src to be switched from a safe source to the working one.
> 
> Might as well make it into real kernel-doc at the same time.
> 
>> + */
>> +
>> +struct clk_regmap_phy_mux {
>> +       u32                     reg;
>> +       u32                     shift;
>> +       u32                     width;
> 
> Technically neither of these need to be u32 and could be u8 to save a
> byte or two. The other thing is that possibly the width and shift never
> changes? The RCG layout is pretty well fixed. Does hardcoding it work?
> 
>> +       u32                     phy_src_val;
>> +       u32                     ref_src_val;
> 
> I feel like "_val" is redundant. Just "ref_src" and "phy_src"? Shorter
> is nice.
> 
>> +       struct clk_regmap       clkr;
>> +};
>> +
>> +extern const struct clk_ops clk_regmap_phy_mux_ops;


-- 
With best wishes
Dmitry