linux-kernel - Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <52A8B159.4040700@wwwdotorg.org>
Date:	Wed, 11 Dec 2013 11:39:21 -0700
From:	Stephen Warren <swarren@...dotorg.org>
To:	bilhuang <bilhuang@...dia.com>,
	"rjw@...ysocki.net" <rjw@...ysocki.net>,
	"viresh.kumar@...aro.org" <viresh.kumar@...aro.org>,
	"thierry.reding@...il.com" <thierry.reding@...il.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

On 12/11/2013 04:18 AM, bilhuang wrote:
> On 12/10/2013 01:32 AM, Stephen Warren wrote:
>> On 12/09/2013 01:44 AM, bilhuang wrote:
>>> On 12/06/2013 07:04 AM, Stephen Warren wrote:
>>>> On 12/05/2013 12:44 AM, Bill Huang wrote:
>>>>> Re-model Tegra cpufreq driver to support all Tegra series of SoCs.
>>>>>
>>>>> * Make tegra-cpufreq.c a generic Tegra cpufreq driver.
>>>>> * Move Tegra20 specific codes into tegra20-cpufreq.c.
>>>>> * Bind Tegra cpufreq dirver with a fake device so defer probe would
>>>>> work
>>>>>     when we're going to get regulator in the driver to support voltage
>>>>>     scaling (DVFS).
>>>>
>>>>> diff --git a/drivers/cpufreq/tegra-cpufreq.c
>>>>> b/drivers/cpufreq/tegra-cpufreq.c
>>>>
>>>>> @@ -91,14 +40,10 @@ static int tegra_update_cpu_speed(struct
>>>>> cpufreq_policy *policy,
>>>> ...
>>>>> +    if (soc_config->vote_emc_on_cpu_rate)
>>>>> +        soc_config->vote_emc_on_cpu_rate(rate);
>>>>> +
>>>>> +    ret = soc_config->cpu_clk_set_rate(rate * 1000);
>>>>>        if (ret)
>>>>>            pr_err("cpu-tegra: Failed to set cpu frequency to %lu
>>>>> kHz\n",
>>>>>                rate);
>>>>
>>>> Is there any/much shared code left in this file after this patch? It
>>>> seems like all this file does now is make each cpufreq callback
>>>> function
>>>> call soc_config->the_same_function_name(). If so, wouldn't it be better
>>>> to simply implement completely separate tegar20-cpufreq and
>>>> tegra30-cpufreq drivers, and register them each directly with the
>>>> cpufreq core, to avoid this file doing all the indirection?
>>>
>>> I think this file is needed since we can shared the registration and
>>> probe logic for different SoCs.
>>
>> But there's basically nothing in probe() already, and if we have a
>> separate driver for each SoC, then there's even less code; just a call
>> to devm_kzalloc() for the device-specific data (which will be
>> SoC-specific in size anyway), and a call to cpufreq_register_driver(). I
>> don't think it's worth sharing that if it means that every other
>> function needs to be an indirect function call.
> OK that makes sense.
>>
>>>>> -int __init tegra_cpufreq_init(void)
>>>>> +static struct {
>>>>> +    char *compat;
>>>>> +    int (*init)(struct tegra_cpufreq_data *,
>>>>> +            const struct tegra_cpufreq_config **);
>>>>> +} tegra_init_funcs[] = {
>>>>> +    { "nvidia,tegra20", tegra20_cpufreq_init },
>>>>> +};
>>>>> +
>>>>> +static int tegra_cpufreq_probe(struct platform_device *pdev)
>>>> ...
>>>>> +    for (i = 0; i < ARRAY_SIZE(tegra_init_funcs); i++) {
>>>>> +        if (of_machine_is_compatible(tegra_init_funcs[i].compat)) {
>>>>> +            ret = tegra_init_funcs[i].init(tegra_data, &soc_config);
>>>>> +            if (!ret)
>>>>> +                break;
>>>>> +            else
>>>>> +                goto out;
>>>>> +        }
>>>>>        }
>>>>> +    if (i == ARRAY_SIZE(tegra_init_funcs))
>>>>> +        goto out;
>>>>
>>>> I think there are better ways of doing this than open-coding it.
>>>> Perhaps
>>>> of_match_device() or the platform-driver equivalent could be made to
>>>> work?
>>>
>>> Open coding is everywhere in OF helper functions actually. I doubt if we
>>> can use of_match_device() if we're not adding node in DT.
>>> If we're matching the platform device then we might need open coding,
>>> no?
>>
>> For platform devices, you can set up the id_table of struct
>> platform_driver, and then simply call platform_get_device_id(pdev)
>> inside probe() to find the matching entry. drivers/i2c/busses/i2c-at91.c
>> is an example of how this works (just some random driver I found using
>> grep).
>
> If we're going to have separate driver for each SoC, then we don't need
> platform_get_device_id(pdev) stuffs...

True.

> What I would like to do is creating platform cpufreq device with name
> "${root_compatible}-cpufreq" then each SoC cpufreq driver can bind to
> it, but the question is, which file is the best place to do this? Create
> a new file for this or use existing file like arch/arm/mach-tegra/tegra.c?

I think create the device in
drivers/cpufreq/cpufreq-tegra.c:tegra_cpufreq_init() (which is possibly
all that file would contain), and call that function from
arch/arm/mach-tegra/tegra.c. That way, we'll be able to share the
implementation of tegra_cpufreq_init() with any ARMv8 CPUs that might
appear.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/