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

[Matthias Kaehlcke]

Hi Taniya,

On Thu, Oct 31, 2019 at 04:59:26PM +0530, Taniya Das wrote:
> 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.

I see. My assumption was that the entire clock hierarchy should be registered,
but Stephen almost certainly knows better :)

> Yes it would throw these warnings, but no functional issue is observed from
> display. I have tested it on the cheza board.

The driver considers it an error (uses DEV_ERR to log the message) and doesn't
handle other clocks when one is found missing. I'm not really famililar with
the dpu_mdss driver, but I imagine this can have some side effects. Added some
of the authors/contributors to cc.

> I guess we could fix the DRM driver to use the "devm_clk_get_optional()"
> instead?

It would also require a minor rework of the driver, which currently expects
all specified clocks to be available.

Another option could be to not list the clock in the device tree, then the
driver won't notice it as missing.

> > 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.

Ok, thanks