Re: [PATCH v2 1/3] arm64: dts: qcom: msm8998: correct xo clock name

[Jeffrey Hugo <jhugo@codeaurora.org>]

On 12/5/2018 2:42 PM, Stephen Boyd wrote:
> Quoting Jeffrey Hugo (2018-12-05 13:20:07)
>> On 12/5/2018 2:04 PM, Stephen Boyd wrote:
>>> Quoting Jeffrey Hugo (2018-12-05 09:03:54)
>>>
>>> I don't quite understand the patch in general. The xo_board clk should
>>> always exist in DT and the fixed factor clk in GCC is there until the
>>> rpm clk driver can control the XO clk state vote for the kernel.
>>
>> Sorry, this wasn't apparent.  It doesn't seem like this "requirement" is
>> captured anywhere.
> 
> Agreed!
> 
>>
>> As far as the SD clocks are concerned, they are defined in GCC, and
>> eventually have a root parent called "xo".  "xo" isn't defined anywhere,
>> so the SD clocks can't really be used, and the hardware doesn't come up.
>>    This patch "fixed" that, but I missed the link to the rpm driver that
>> Marc pointed out.
> 
> Hmm ok. The SD DT node should just point to the xo_board clk for now and
> later on it can be changed to use the rpm clk when the rpm node is
> created.
> 
>>
>>
>>>
>>> If anything, change the DT node to be named xo-board instead of xo_board
>>> because that matches DT naming schemes and then add a clock-output-names
>>> = "xo_board" property to it so that we keep the underscore.
>>
>> I see this now, and I agree with it, but then SD goes back to a broken
>> state because there is "xo" clock for GCC.  Its not quite clear to me
>> how to make GCC (and thus SD) happy again with this change reverted/fixed.
>>
>> Bjorn mentioned offline he is going to take a look, but he has a few
>> other things on his plate first.
>>
> 
> There is an XO clk created in drivers/clk/qcom/gcc-msm8996.c, we should
> do the same here until rpm can handle this. I'll pack this patch up and
> merge it to clk-next soon.

Thanks.  I pulled in the below change into my tree, and fixed up the DT 
based on the discussion we had.  SD works, and things look sane to me 
per clk_summary in debugfs.

Feel free to throw my tested-by on if you want.

Andy,
How would you like this little mess from my patch series to be fixed? 
It looks like you picked up a hybrid of v1 and v2 in your tree, and it 
would appear that went in a pull request to the ARM maintainers.  Do you 
want fixes based upon your next branch?

> 
> ----8<----
> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
> index c3bb9fffd040..d72b908137e2 100644
> --- a/drivers/clk/qcom/gcc-msm8998.c
> +++ b/drivers/clk/qcom/gcc-msm8998.c
> @@ -117,6 +117,17 @@ static const char * const gcc_parent_names_5[] = {
>   	"core_bi_pll_test_se",
>   };
>   
> +static struct clk_fixed_factor xo = {
> +	.mult = 1,
> +	.div = 1,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "xo",
> +		.parent_names = (const char *[]){ "xo_board" },
> +		.num_parents = 1,
> +		.ops = &clk_fixed_factor_ops,
> +	},
> +};
> +
>   static struct pll_vco fabia_vco[] = {
>   	{ 250000000, 2000000000, 0 },
>   	{ 125000000, 1000000000, 1 },
> @@ -2798,6 +2809,10 @@ static int gcc_msm8998_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> +	ret = devm_clk_hw_register(&pdev->dev, &xo.hw);
> +	if (ret)
> +		return ret;
> +
>   	return qcom_cc_really_probe(pdev, &gcc_msm8998_desc, regmap);
>   }
>   


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel