Re: [PATCH v3 3/4] clk: tegra: MBIST work around for Tegra210

[Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On Thu, Jan 25, 2018 at 10:19:08AM +0000, Jon Hunter wrote:
> 
> On 25/01/18 09:02, Peter De Schrijver wrote:
> > On Wed, Jan 24, 2018 at 09:59:56PM +0000, Jon Hunter wrote:
> 
> ...
> 
> >>> +void tegra210_clk_handle_mbist_war(unsigned int id)
> >>> +{
> >>> +	int err;
> >>> +	struct tegra210_domain_mbist_war *mbist_war;
> >>> +
> >>> +	if (id >= ARRAY_SIZE(tegra210_pg_mbist_war)) {
> >>> +		WARN(1, "unknown domain id in MBIST WAR handler\n");
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	mbist_war = &tegra210_pg_mbist_war[id];
> >>> +	if (!mbist_war->handle_lvl2_ovr)
> >>> +		return;
> >>> +
> >>> +	err = mbist_war->handle_lvl2_ovr(mbist_war);
> >>
> >> Why not move the clk_bulk_prepare_enable/disable_unprepare and
> >> mutex_lock/unlock functions into this function around the call to
> >> ->handle_lvl2_ovr to save the duplication of that code in each of the
> >> war functions?
> >>
> > 
> > This could be done yes.
> > 
> >>> +	WARN(err < 0, "error handling MBIST WAR for domain: %d\n", id);
> >>> +}
> >>
> >> I think that the above function should return an error and we should let
> >> the power-domain power-on fail.
> >>
> > 
> > This would only be useful if the user (tegra_powergate_power_up) would do
> > rollback. I don't think that's done correctly today.
> 
> It does and so I think that we should return an error.
> 

Ok.

> >>> +
> >>> +
> >>>  void tegra210_put_utmipll_in_iddq(void)
> >>>  {
> >>>  	u32 reg;
> >>> @@ -3163,6 +3500,40 @@ static int tegra210_reset_deassert(unsigned long id)
> >>>  	return 0;
> >>>  }
> >>>  
> >>> +static void tegra210_mbist_clk_init(void)
> >>> +{
> >>> +	int i, j;
> >>> +
> >>> +	for (i = 0; i < ARRAY_SIZE(tegra210_pg_mbist_war); i++) {
> >>> +		int num_clks = tegra210_pg_mbist_war[i].num_clks;
> >>> +		struct clk_bulk_data *clk_data;
> >>> +
> >>> +		if (!num_clks)
> >>> +			continue;
> >>> +
> >>> +		clk_data = kmalloc_array(num_clks, sizeof(*clk_data),
> >>> +					 GFP_KERNEL);
> >>> +		if (WARN(!clk_data,
> >>> +			"no space for MBIST WAR clk array for %d\n", i)) {
> >>> +			tegra210_pg_mbist_war[i].handle_lvl2_ovr = NULL;
> >>> +			continue;
> >>> +		}
> >>
> >> Printing error messages on memory allocation failures are not needed and
> >> have been removed from various drivers. So lets no add any error
> >> messages or warnings here.
> >>
> >> Also I think that we should just return an error here and not bother
> >> continuing as there is no point.
> 
> So maybe here just ...
> 
> 		if (WARN_ON(!clk_data))
> 			return -ENOMEM;
> 

Given that we do WARN_ON() here..

> >>> +
> >>> +		tegra210_pg_mbist_war[i].clks = clk_data;
> >>
> >> I think that you should only populate this when all the clocks have been
> >> initialised correctly. You could then use this to check the clocks have
> >> been setup correctly when executing the war.
> >>
> > 
> > For some domains no extra clocks are needed (ie the clocks enabled by the
> > power domain driver are enough). So an extra flag would be needed then.
> 
> Yes but you have num_clks to detect if a domain has extra clocks. So you
> can use both of these to detect if the clocks are setup correctly. Right?
> 
> >>> +		for (j = 0; j < num_clks; j++) {
> >>> +			int clk_id = tegra210_pg_mbist_war[i].clk_init_data[j];
> >>> +			struct clk *clk = clks[clk_id];
> >>> +
> >>> +			if (IS_ERR(clk)) {
> >>> +				clk_data[j].clk = NULL;
> >>> +				WARN(1, "clk_id: %d\n", clk_id);
> >>
> >> I think that we should return an error here.
> >>
> > 
> > I don't think letting clock init fail because of this, is a good idea. Too
> > many things rely on working clocks.
> 
> It should never fail and if it does something is badly broken.
> 
> Maybe what we could do ...
> 
> 			if (WARN_ON(IS_ERR(clk))) {

and here..

> 				tegra210_pg_mbist_war[i].clks = NULL;
> 				break;
> 			}
> 
> 			clk_data[j].clk = clk;
> 

..

> >>>  	if (!clks)
> >>> @@ -3233,6 +3622,8 @@ static void __init tegra210_clock_init(struct device_node *np)
> >>>  	tegra_add_of_provider(np);
> >>>  	tegra_register_devclks(devclks, ARRAY_SIZE(devclks));
> >>>  
> >>> +	tegra210_mbist_clk_init();
> >>> +
> >>
> >> Maybe add a print here if the mbist init fails and return. I understand
> >> it may not be a critical failure but it should never fail.
> >>
> > 
> > You mean have the entire clock init fail and undo all the clock registrations?
> > That seems overkill to me. Returning early would only prevent some sleep states
> > from working because tegra_cpu_car_ops will not be initialized then. So I would
> > do a warning then.
> 
> I don't think it is necessary to undo it. Ok, don't worry about
> returning an error here, the warnings should be sufficient.
> 

I don't think there's much value in yet another warning here.

Peter.