Re: [PATCH v6 4/5] clk: qcom: rpmh: Add support for SM8550 rpmh clocks

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 28/12/2022 19:59, Dmitry Baryshkov wrote:
> On 28/12/2022 20:52, Bjorn Andersson wrote:
>> On Wed, Dec 14, 2022 at 06:25:01PM +0200, Dmitry Baryshkov wrote:
>>> On 07/12/2022 00:45, Abel Vesa wrote:
>>>> Adds the RPMH clocks present in SM8550 SoC.
>>>>
>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>    drivers/clk/qcom/clk-rpmh.c | 110 +++++++++++++++++++++++++++++-------
>>>>    1 file changed, 90 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>>>> index 2c2ef4b6d130..ce81c76ed0fd 100644
>>>> --- a/drivers/clk/qcom/clk-rpmh.c
>>>> +++ b/drivers/clk/qcom/clk-rpmh.c
>>>> @@ -130,6 +130,34 @@ static DEFINE_MUTEX(rpmh_clk_lock);
>>>>    		},							\
>>>>    	}
>>>> +#define DEFINE_CLK_FIXED_FACTOR(_name, _parent_name, _div)		\
>>>> +	static struct clk_fixed_factor clk_fixed_factor##_##_name = {	\
>>>> +		.mult = 1,						\
>>>> +		.div = _div,						\
>>>> +		.hw.init = &(struct clk_init_data){			\
>>>> +			.ops = &clk_fixed_factor_ops,			\
>>>> +			.name = #_name,					\
>>>> +			.parent_data =  &(const struct clk_parent_data){ \
>>>> +				.fw_name = #_parent_name,		\
>>>> +				.name = #_parent_name,			\
>>>> +			},						\
>>>> +			.num_parents = 1,				\
>>>> +		},							\
>>>> +	};								\
>>>> +	static struct clk_fixed_factor clk_fixed_factor##_##_name##_ao = { \
>>>> +		.mult = 1,						\
>>>> +		.div = _div,						\
>>>> +		.hw.init = &(struct clk_init_data){			\
>>>> +			.ops = &clk_fixed_factor_ops,			\
>>>> +			.name = #_name "_ao",				\
>>>> +			.parent_data =  &(const struct clk_parent_data){ \
>>>> +				.fw_name = #_parent_name "_ao",		\
>>>> +				.name = #_parent_name "_ao",		\
>>>> +			},						\
>>>> +			.num_parents = 1,				\
>>>> +		},							\
>>>> +	}
>>>> +
>>>>    static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
>>>>    {
>>>>    	return container_of(_hw, struct clk_rpmh, hw);
>>>> @@ -345,6 +373,8 @@ DEFINE_CLK_RPMH_ARC(bi_tcxo, "xo.lvl", 0x3, 2);
>>>>    DEFINE_CLK_RPMH_ARC(bi_tcxo, "xo.lvl", 0x3, 4);
>>>>    DEFINE_CLK_RPMH_ARC(qlink, "qphy.lvl", 0x1, 4);
>>>> +DEFINE_CLK_FIXED_FACTOR(bi_tcxo_div2, bi_tcxo, 2);
>>>> +
>>>>    DEFINE_CLK_RPMH_VRM(ln_bb_clk1, _a2, "lnbclka1", 2);
>>>>    DEFINE_CLK_RPMH_VRM(ln_bb_clk2, _a2, "lnbclka2", 2);
>>>>    DEFINE_CLK_RPMH_VRM(ln_bb_clk3, _a2, "lnbclka3", 2);
>>>> @@ -366,6 +396,16 @@ DEFINE_CLK_RPMH_VRM(rf_clk2, _d, "rfclkd2", 1);
>>>>    DEFINE_CLK_RPMH_VRM(rf_clk3, _d, "rfclkd3", 1);
>>>>    DEFINE_CLK_RPMH_VRM(rf_clk4, _d, "rfclkd4", 1);
>>>> +DEFINE_CLK_RPMH_VRM(clk1, _a1, "clka1", 1);
>>>> +DEFINE_CLK_RPMH_VRM(clk2, _a1, "clka2", 1);
>>>> +DEFINE_CLK_RPMH_VRM(clk3, _a1, "clka3", 1);
>>>> +DEFINE_CLK_RPMH_VRM(clk4, _a1, "clka4", 1);
>>>> +DEFINE_CLK_RPMH_VRM(clk5, _a1, "clka5", 1);
>>>> +
>>>> +DEFINE_CLK_RPMH_VRM(clk6, _a2, "clka6", 2);
>>>> +DEFINE_CLK_RPMH_VRM(clk7, _a2, "clka7", 2);
>>>> +DEFINE_CLK_RPMH_VRM(clk8, _a2, "clka8", 2);
>>>> +
>>>>    DEFINE_CLK_RPMH_VRM(div_clk1, _div2, "divclka1", 2);
>>>>    DEFINE_CLK_RPMH_BCM(ce, "CE0");
>>>> @@ -576,6 +616,33 @@ static const struct clk_rpmh_desc clk_rpmh_sm8450 = {
>>>>    	.num_clks = ARRAY_SIZE(sm8450_rpmh_clocks),
>>>>    };
>>>> +static struct clk_hw *sm8550_rpmh_clocks[] = {
>>>> +	[RPMH_CXO_PAD_CLK]      = &clk_rpmh_bi_tcxo_div2.hw,
>>>> +	[RPMH_CXO_PAD_CLK_A]    = &clk_rpmh_bi_tcxo_div2_ao.hw,
>>>> +	[RPMH_CXO_CLK]		= &clk_fixed_factor_bi_tcxo_div2.hw,
>>>> +	[RPMH_CXO_CLK_A]	= &clk_fixed_factor_bi_tcxo_div2_ao.hw,
>>>> +	[RPMH_LN_BB_CLK1]	= &clk_rpmh_clk6_a2.hw,
>>>> +	[RPMH_LN_BB_CLK1_A]	= &clk_rpmh_clk6_a2_ao.hw,
>>>> +	[RPMH_LN_BB_CLK2]	= &clk_rpmh_clk7_a2.hw,
>>>> +	[RPMH_LN_BB_CLK2_A]	= &clk_rpmh_clk7_a2_ao.hw,
>>>> +	[RPMH_LN_BB_CLK3]	= &clk_rpmh_clk8_a2.hw,
>>>> +	[RPMH_LN_BB_CLK3_A]	= &clk_rpmh_clk8_a2_ao.hw,
>>>> +	[RPMH_RF_CLK1]		= &clk_rpmh_clk1_a1.hw,
>>>> +	[RPMH_RF_CLK1_A]	= &clk_rpmh_clk1_a1_ao.hw,
>>>> +	[RPMH_RF_CLK2]		= &clk_rpmh_clk2_a1.hw,
>>>> +	[RPMH_RF_CLK2_A]	= &clk_rpmh_clk2_a1_ao.hw,
>>>> +	[RPMH_RF_CLK3]		= &clk_rpmh_clk3_a1.hw,
>>>> +	[RPMH_RF_CLK3_A]	= &clk_rpmh_clk3_a1_ao.hw,
>>>> +	[RPMH_RF_CLK4]		= &clk_rpmh_clk4_a1.hw,
>>>> +	[RPMH_RF_CLK4_A]	= &clk_rpmh_clk4_a1_ao.hw,
>>>> +	[RPMH_IPA_CLK]		= &clk_rpmh_ipa.hw,
>>>> +};
>>>> +
>>>> +static const struct clk_rpmh_desc clk_rpmh_sm8550 = {
>>>> +	.clks = sm8550_rpmh_clocks,
>>>> +	.num_clks = ARRAY_SIZE(sm8550_rpmh_clocks),
>>>> +};
>>>> +
>>>>    static struct clk_hw *sc7280_rpmh_clocks[] = {
>>>>    	[RPMH_CXO_CLK]      = &clk_rpmh_bi_tcxo_div4.hw,
>>>>    	[RPMH_CXO_CLK_A]    = &clk_rpmh_bi_tcxo_div4_ao.hw,
>>>> @@ -683,29 +750,31 @@ static int clk_rpmh_probe(struct platform_device *pdev)
>>>>    		name = hw_clks[i]->init->name;
>>>> -		rpmh_clk = to_clk_rpmh(hw_clks[i]);
>>>> -		res_addr = cmd_db_read_addr(rpmh_clk->res_name);
>>>> -		if (!res_addr) {
>>>> -			dev_err(&pdev->dev, "missing RPMh resource address for %s\n",
>>>> -				rpmh_clk->res_name);
>>>> -			return -ENODEV;
>>>> -		}
>>>> +		if (hw_clks[i]->init->ops != &clk_fixed_factor_ops) {
>>>
>>> We discussed this separately, the fixed factor clocks will be moved to the
>>> child nodes, leaving rpmhcc with only cmd-db based clocks.
>>>
>>
>> Are you saying that you will represent bi_tcxo as a fixed-factor-clock
>> under /clocks with RPMH_CXO_PAD_CLK as parent and a clock-div = <2>; ?
> 
> Yes, this was the idea. The rpmhcc driver is pretty much centric around 
> the cmd-db clocks. Adding a fixed-factor clock results either in a 
> horrible hacks or in a significant code refactoring. However we already 
> have an existing way to fixed-factor clocks: DT nodes. Adding support 
> for such nodes to rpmhcc driver requires just a single additional API 
> call: devm_of_platform_populate().

Please no. DT is not to solve driver issues, skip some code or make
things simpler for driver developers. If everyone - U-boot, *BSD,
firmwares - pushes to DT stuff like this, because this makes their
driver development easier, you would have total mess. Linux does not
have any priorities here in this approach.

Best regards,
Krzysztof