On 25/01/18 12:41, Peter De Schrijver wrote:

On Thu, Jan 25, 2018 at 10:19:08AM +0000, Jon Hunter wrote:

quoted

On 25/01/18 09:02, Peter De Schrijver wrote:

quoted

On Wed, Jan 24, 2018 at 09:59:56PM +0000, Jon Hunter wrote:

...

quoted

quoted

quoted

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

quoted

quoted

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

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

+
+		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?

quoted

quoted

quoted

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

quoted

				tegra210_pg_mbist_war[i].clks = NULL;
				break;
			}

			clk_data[j].clk = clk;

..

quoted

quoted

quoted

quoted

 	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.

Yes indeed.

Cheers
Jon

-- 
nvpublic