Re: [PATCH v4 5/5] clk: qcom: Add Global Clock controller (GCC) driver for SC7180

[Taniya Das]

Hi Matthias,

Thanks for your comments.

On 10/29/2019 11:29 PM, Matthias Kaehlcke wrote:
> Hi Taniya,
>
> On Mon, Oct 14, 2019 at 03:53:08PM +0530, Taniya Das wrote:
> > Add support for the global clock controller found on SC7180
> > based devices. This should allow most non-multimedia device
> > drivers to probe and control their clocks.
> >
> > Signed-off-by: Taniya Das <tdas@xxxxxxxxxxxxxx>

> v3 also had
>
> +	[GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,
>
> Removing it makes the dpu_mdss driver unhappy:
>
> [    2.999855] dpu_mdss_enable+0x2c/0x58->msm_dss_enable_clk: 'iface' is not available
>
> because:
>
>          mdss: mdss@ae00000 {
>      	        ...
>
>   =>             clocks = <&gcc GCC_DISP_AHB_CLK>,
>                           <&gcc GCC_DISP_HF_AXI_CLK>,
>                           <&dispcc DISP_CC_MDSS_MDP_CLK>;
>                  clock-names = "iface", "gcc_bus", "core";
> 	};

The basic idea as you mentioned below was to move the CRITICAL clocks to 
probe. The clock provider to return NULL in case the clocks are not 
registered.
This was discussed with Stephen on v3. Thus I submitted the below patch.
clk: qcom: common: Return NULL from clk_hw OF provider.

Yes it would throw these warnings, but no functional issue is observed 
from display. I have tested it on the cheza board.
I guess we could fix the DRM driver to use the "devm_clk_get_optional()" 
instead?

> More clocks were removed in v4:
>
> -       [GCC_CPUSS_GNOC_CLK] = &gcc_cpuss_gnoc_clk.clkr,
> -       [GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr,
> -       [GCC_VIDEO_AHB_CLK] = &gcc_video_ahb_clk.clkr,
>
> I guess this part of "remove registering the CRITICAL clocks to clock provider
> and leave them always ON from the GCC probe." (change log entry), but are you
> sure nobody is going to reference these clocks?

Even if they are referenced clk provider would return NULL.

> > +static int gcc_sc7180_probe(struct platform_device *pdev)
> > +{
> > +	struct regmap *regmap;
> > +	int ret;
> > +
> > +	regmap = qcom_cc_map(pdev, &gcc_sc7180_desc);
> > +	if (IS_ERR(regmap))
> > +		return PTR_ERR(regmap);
> > +
> > +	/*
> > +	 * Disable the GPLL0 active input to MM blocks, NPU
> > +	 * and GPU via MISC registers.
> > +	 */
> > +	regmap_update_bits(regmap, 0x09ffc, 0x3, 0x3);
> > +	regmap_update_bits(regmap, 0x4d110, 0x3, 0x3);
> > +	regmap_update_bits(regmap, 0x71028, 0x3, 0x3);
>
> In v3 this was:
>
> 	regmap_update_bits(regmap, GCC_MMSS_MISC, 0x3, 0x3);
> 	regmap_update_bits(regmap, GCC_NPU_MISC, 0x3, 0x3);
> 	regmap_update_bits(regmap, GCC_GPU_MISC, 0x3, 0x3);
>
> IMO register names seem preferable, why switch to literal addresses
> instead?

:). These cleanups where done based on the comments I had received 
during SDM845 review. If Stephen is fine moving them to names, I could 
submit them in the next patch series.

> > +
> > +	/*
> > +	 * Keep the clocks always-ON
> > +	 * GCC_CPUSS_GNOC_CLK, GCC_VIDEO_AHB_CLK, GCC_DISP_AHB_CLK
> > +	 * GCC_GPU_CFG_AHB_CLK
> > +	 */
> > +	regmap_update_bits(regmap, 0x48004, BIT(0), BIT(0));
> > +	regmap_update_bits(regmap, 0x0b004, BIT(0), BIT(0));
> > +	regmap_update_bits(regmap, 0x0b00c, BIT(0), BIT(0));
> > +	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
>
> ditto, register names seem preferable.

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--