On 25/01/18 12:41, Peter De Schrijver wrote:
> 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.
Yes indeed.
Cheers
Jon
--
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
