From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752204AbdF2Iub (ORCPT ); Thu, 29 Jun 2017 04:50:31 -0400 Received: from mail-wr0-f181.google.com ([209.85.128.181]:32801 "EHLO mail-wr0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751855AbdF2IuW (ORCPT ); Thu, 29 Jun 2017 04:50:22 -0400 Message-ID: <1498726219.27840.2.camel@baylibre.com> Subject: Re: [PATCH] clk: scpi: error when clock fails to register From: Jerome Brunet To: Sudeep Holla , Michael Turquette , Stephen Boyd Cc: linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, Neil Armstrong , Kevin Hilman Date: Thu, 29 Jun 2017 10:50:19 +0200 In-Reply-To: <3e77fd3c-9807-10d4-3a8c-cab8b5562f6c@arm.com> References: <20170628135345.9337-1-jbrunet@baylibre.com> <1498664332.2337.6.camel@baylibre.com> <76efd418-9fd5-6ac5-b4c9-c75c5df69df0@arm.com> <1498668381.2337.10.camel@baylibre.com> <3e77fd3c-9807-10d4-3a8c-cab8b5562f6c@arm.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6 (3.22.6-2.fc25) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2017-06-28 at 18:07 +0100, Sudeep Holla wrote: > > On 28/06/17 17:46, Jerome Brunet wrote: > > On Wed, 2017-06-28 at 16:52 +0100, Sudeep Holla wrote: > > [..] > > > > > > > Thanks for this stack. I just worked out the same path now. I did come > > > up with the patch as below. That should work if my understanding is > > > correct. > > > > I tried. > > Thanks. > > > It does not work unfortunately. Still crashes but somewhere else: > > [    2.301482] [] scpi_of_clk_src_get+0x14/0x58 > > [    2.307261] [] __of_clk_get_by_name+0x100/0x118 > > [    2.313297] [] clk_get+0x2c/0x78 > > [    2.318044] [] dev_pm_opp_get_opp_table+0xb0/0x118 > > [    2.324338] [] dev_pm_opp_add+0x20/0x68 > > [    2.329687] [] scpi_init_opp_table+0xa8/0x188 > > [    2.335550] [] > > _get_cluster_clk_and_freq_table+0x80/0x180 > > [    2.342450] [] bL_cpufreq_init+0x3f0/0x480 > > [    2.348056] [] cpufreq_online+0xc0/0x658 > > [    2.353490] [] cpufreq_add_dev+0x78/0x88 > > [    2.358924] [] subsys_interface_register+0x84/0xc8 > > [    2.365220] [] cpufreq_register_driver+0x138/0x1b8 > > [    2.371516] [] bL_cpufreq_register+0x74/0x120 > > [    2.377381] [] scpi_cpufreq_probe+0x28/0x38 > > [    2.383076] [] platform_drv_probe+0x50/0xb8 > > [    2.388766] [] driver_probe_device+0x21c/0x2d8 > > > > Looks like a different route and I know why. I have added an extra check > now which should work if I have not missed anything more. > > > I have not looked at ALL the clock providers, but I have seen a few and I > > don't > > remember seeing any which fails, at some point, to register a clocks and > > still > > register successfully.  > > > > No problem, as I said I am fine with the patch you sent as a fix for now > but just curious to know what are the issues to be fixed to continue > supporting that feature. Please bear with me. I am :) and I understand what you are trying to do, having a degraded clock provider is better than nothing according to you, correct? I'm wondering whether this is correct or not, that why I'm challenging this a bit. If you failed to register an scpi clock it is probably because the communication with the FW is not working, or at least 'not that good', right ? If for some reason, you manage to register some other clocks from the same FW, how confident can you be that communication will be ok for them ? that the settings you request will be applied correctly ? Is it possible that you may be causing more harm/damage playing with a broken HW ? > > > It seems strange to continue with a broken controller. > > > > I would have agreed if it was single driver or h/w controlled by Linux. > Since it's in the firmware, we should allow the working clocks/opps to > work though few are broken. It's not good if we had to disable > everything if some piece of firmware is not yet ready or broken. > But again, we can get it working later, for now, I am fine with you patch. I tried your last version, and it does not Oops, at least not for me. The end result still looks odd to me: [    1.115219] scpi_clocks scpi:clocks: failed to register clock 'vcpu' [    1.159490] cpu cpu0: _get_cluster_clk_and_freq_table: Failed to get clk for cpu: 0, cluster: 0 [    1.162986] cpu cpu0: _get_cluster_clk_and_freq_table: Failed to get data for cluster: 0 [    1.170945] cpu cpu1: _get_cluster_clk_and_freq_table: Failed to get clk for cpu: 1, cluster: 0 [    1.179634] cpu cpu1: _get_cluster_clk_and_freq_table: Failed to get data for cluster: 0 [    1.187654] cpu cpu2: _get_cluster_clk_and_freq_table: Failed to get clk for cpu: 2, cluster: 0 [    1.196284] cpu cpu2: _get_cluster_clk_and_freq_table: Failed to get data for cluster: 0 [    1.204375] cpu cpu3: _get_cluster_clk_and_freq_table: Failed to get clk for cpu: 3, cluster: 0 [    1.212911] cpu cpu3: _get_cluster_clk_and_freq_table: Failed to get data for cluster: 0 [    1.220612] arm_big_little: bL_cpufreq_register: Registered platform driver: scpi So now, I have an scpi clock provider which registers successfully but fails to register its only clock. As a consequence, I also have a cpufreq driver which manages to register but has no clock cpu clock to drive ... > > Regards, > Sudeep > > --- > > diff --git i/drivers/clk/clk-scpi.c w/drivers/clk/clk-scpi.c > index 96d37175d0ad..a0b9b4c84be3 100644 > --- i/drivers/clk/clk-scpi.c > +++ w/drivers/clk/clk-scpi.c > @@ -192,7 +192,7 @@ scpi_of_clk_src_get(struct of_phandle_args *clkspec, > void *data) > >         for (count = 0; count < clk_data->clk_num; count++) { >                 sclk = clk_data->clk[count]; > -               if (idx == sclk->id) > +               if (sclk && idx == sclk->id) >                         return &sclk->hw; >         } > > @@ -245,11 +245,14 @@ static int scpi_clk_add(struct device *dev, struct > device_node *np, >                 sclk->id = val; > >                 err = scpi_clk_ops_init(dev, match, sclk, name); > -               if (err) > +               if (err) { >                         dev_err(dev, "failed to register clock '%s'\n", > name); > -               else > +                       clk_data->clk[idx] = NULL; > +                       devm_kfree(dev, sclk); > +               } else { >                         dev_dbg(dev, "Registered clock '%s'\n", name); > -               clk_data->clk[idx] = sclk; > +                       clk_data->clk[idx] = sclk; > +               } >         } > >         return of_clk_add_hw_provider(np, scpi_of_clk_src_get, clk_data); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at  http://vger.kernel.org/majordomo-info.html