| Message ID | 1386229462-3474-3-git-send-email-bilhuang@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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? > -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? > +int __init tegra_cpufreq_init(void) > +{ > + struct platform_device_info devinfo = { .name = "tegra-cpufreq", }; > + > + platform_device_register_full(&devinfo); > + > + return 0; > } > EXPORT_SYMBOL(tegra_cpufreq_init); Perhaps instead of hard-coding the name "tegra-cpufreq" here, you could dynamically construct the device name based on the DT's root compatible value, register "${root_compatible}-cpufreq", e.g. "nvidia,tegra20-cpufreq" or "nvidia,tegra30-cpufreq". That would allow the kernel's standard device/driver matching mechanism to pick the correct driver to instantiate. Perhaps you could even dynamically register an OF device so that you can use of_match_device() in probe, if there's some advantage of having a single driver that supports N chips. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > >> -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? > >> +int __init tegra_cpufreq_init(void) >> +{ >> + struct platform_device_info devinfo = { .name = "tegra-cpufreq", }; >> + >> + platform_device_register_full(&devinfo); >> + >> + return 0; >> } >> EXPORT_SYMBOL(tegra_cpufreq_init); > > Perhaps instead of hard-coding the name "tegra-cpufreq" here, you could > dynamically construct the device name based on the DT's root compatible > value, register "${root_compatible}-cpufreq", e.g. > "nvidia,tegra20-cpufreq" or "nvidia,tegra30-cpufreq". That would allow > the kernel's standard device/driver matching mechanism to pick the > correct driver to instantiate. Perhaps you could even dynamically > register an OF device so that you can use of_match_device() in probe, if I guess what you meant dynamically register an OF device is registering an fake OF device by calling of_device_add(), no? If yes then what of_node should we give? > there's some advantage of having a single driver that supports N chips. > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. >>> -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). >>> +int __init tegra_cpufreq_init(void) >>> +{ >>> + struct platform_device_info devinfo = { .name = "tegra-cpufreq", }; >>> + >>> + platform_device_register_full(&devinfo); >>> + >>> + return 0; >>> } >>> EXPORT_SYMBOL(tegra_cpufreq_init); >> >> Perhaps instead of hard-coding the name "tegra-cpufreq" here, you could >> dynamically construct the device name based on the DT's root compatible >> value, register "${root_compatible}-cpufreq", e.g. >> "nvidia,tegra20-cpufreq" or "nvidia,tegra30-cpufreq". That would allow >> the kernel's standard device/driver matching mechanism to pick the >> correct driver to instantiate. Perhaps you could even dynamically >> register an OF device so that you can use of_match_device() in probe, if > > I guess what you meant dynamically register an OF device is registering > an fake OF device by calling of_device_add(), no? If yes then what > of_node should we give? Yes. Good question about which node. I guess the root node would be the only one that made any sense at all, and admittedly it doesn't make a huge amount of sense. Perhaps registers a platform device rather than an OF device would make more sense. See platform_device_register() I think. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... 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? > >>>> +int __init tegra_cpufreq_init(void) >>>> +{ >>>> + struct platform_device_info devinfo = { .name = "tegra-cpufreq", }; >>>> + >>>> + platform_device_register_full(&devinfo); >>>> + >>>> + return 0; >>>> } >>>> EXPORT_SYMBOL(tegra_cpufreq_init); >>> >>> Perhaps instead of hard-coding the name "tegra-cpufreq" here, you could >>> dynamically construct the device name based on the DT's root compatible >>> value, register "${root_compatible}-cpufreq", e.g. >>> "nvidia,tegra20-cpufreq" or "nvidia,tegra30-cpufreq". That would allow >>> the kernel's standard device/driver matching mechanism to pick the >>> correct driver to instantiate. Perhaps you could even dynamically >>> register an OF device so that you can use of_match_device() in probe, if >> >> I guess what you meant dynamically register an OF device is registering >> an fake OF device by calling of_device_add(), no? If yes then what >> of_node should we give? > > Yes. Good question about which node. I guess the root node would be the > only one that made any sense at all, and admittedly it doesn't make a > huge amount of sense. Perhaps registers a platform device rather than an > OF device would make more sense. See platform_device_register() I think. > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 December 2013 13:14, Bill Huang <bilhuang@nvidia.com> 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). I strongly feel we must reuse cpufreq-cpu0 driver here after adding a clk/regulator driver for tegra to support all that. @Stephen: If you want we can keep all that tegra specific stuff (clk/regulator) in tegra-cpufreq.c, but we can easily use cpufreq-cpu0 driver without much complications.. I have tried it earlier, got some comments and then got busy in other stuff.. https://lkml.org/lkml/2013/8/7/364 > static int tegra_cpu_exit(struct cpufreq_policy *policy) > { > - clk_disable_unprepare(cpu_clk); > - clk_disable_unprepare(emc_clk); > + cpufreq_frequency_table_cpuinfo(policy, tegra_data->freq_table); Btw, why do you need this here? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/17/2013 02:54 PM, Viresh Kumar wrote: > On 5 December 2013 13:14, Bill Huang <bilhuang@nvidia.com> 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). > > I strongly feel we must reuse cpufreq-cpu0 driver here after adding a > clk/regulator driver for tegra to support all that. Tegra20 DVFS is a little bit complicated due to the fact that we can't scale VDD_CPU directly, there are constraints or relationship to other power rails so I don't think it is a good idea to use generic cpufreq-cpu0 driver if we're going to support voltage scaling. > > @Stephen: If you want we can keep all that tegra specific stuff > (clk/regulator) in > tegra-cpufreq.c, but we can easily use cpufreq-cpu0 driver without much > complications.. > > I have tried it earlier, got some comments and then got busy in other stuff.. > https://lkml.org/lkml/2013/8/7/364 > >> static int tegra_cpu_exit(struct cpufreq_policy *policy) >> { >> - clk_disable_unprepare(cpu_clk); >> - clk_disable_unprepare(emc_clk); >> + cpufreq_frequency_table_cpuinfo(policy, tegra_data->freq_table); > > Btw, why do you need this here? > Actually the latest version is v4 which is quite different against v3. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17 December 2013 16:22, bilhuang <bilhuang@nvidia.com> wrote: > Tegra20 DVFS is a little bit complicated due to the fact that we can't scale > VDD_CPU directly, there are constraints or relationship to other power rails > so I don't think it is a good idea to use generic cpufreq-cpu0 driver if > we're going to support voltage scaling. But why can't we handle that in a CPU specific regulator code? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/18/2013 07:11 PM, Viresh Kumar wrote: > On 17 December 2013 16:22, bilhuang <bilhuang@nvidia.com> wrote: >> Tegra20 DVFS is a little bit complicated due to the fact that we can't scale >> VDD_CPU directly, there are constraints or relationship to other power rails >> so I don't think it is a good idea to use generic cpufreq-cpu0 driver if >> we're going to support voltage scaling. > > But why can't we handle that in a CPU specific regulator code? > cpufreq-cpu0 driver will call regulator_set_voltage_tol() directly according to the pre-defined OPP freq/volt pairs, the regulator drivers could be shared by other SoC so is not suitable to handle this, or do I misunderstand? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18 December 2013 17:03, bilhuang <bilhuang@nvidia.com> wrote: > cpufreq-cpu0 driver will call regulator_set_voltage_tol() directly according > to the pre-defined OPP freq/volt pairs, the regulator drivers could be > shared by other SoC so is not suitable to handle this, or do I > misunderstand? In case regulator's driver is shared, then you can probably add another virtual regulator for CPU which would have the special code you want. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/18/2013 10:39 PM, Viresh Kumar wrote: > On 18 December 2013 17:03, bilhuang <bilhuang@nvidia.com> wrote: >> cpufreq-cpu0 driver will call regulator_set_voltage_tol() directly according >> to the pre-defined OPP freq/volt pairs, the regulator drivers could be >> shared by other SoC so is not suitable to handle this, or do I >> misunderstand? > > In case regulator's driver is shared, then you can probably add another > virtual regulator for CPU which would have the special code you want. > I'm not sure virtual regulator for CPU is a good idea, in addition to that, we don't have a single SoC OPP table, we need several which are speedo-id and process-id dependant, but generic cpufreq-cpu0 is assuming there is only one statically, for some SoC the frequency table is not fixed, they are created at runtime combining our fast and slow CPU frequency table and dvfs table. So I'm really not sure is it worth adding so many tweaks in order to use the generic cpufreq-cpu0 driver. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19 December 2013 10:56, bilhuang <bilhuang@nvidia.com> wrote: > I'm not sure virtual regulator for CPU is a good idea, in addition to that, > we don't have a single SoC OPP table, we need several which are speedo-id > and process-id dependant, but generic cpufreq-cpu0 is assuming there is only > one statically Can't that be handled via DT ? > for some SoC the frequency table is not fixed, they are > created at runtime combining our fast and slow CPU frequency table and dvfs > table. So I'm really not sure is it worth adding so many tweaks in order to > use the generic cpufreq-cpu0 driver. Hmm, maybe I got confused because I don't have a clear picture in my mind. It might be better to go ahead with your implementation for now and after everything is set, we can choose to use cpufreq-cpu0 if it is worth it. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/19/2013 01:29 PM, Viresh Kumar wrote: > On 19 December 2013 10:56, bilhuang <bilhuang@nvidia.com> wrote: >> I'm not sure virtual regulator for CPU is a good idea, in addition to that, >> we don't have a single SoC OPP table, we need several which are speedo-id >> and process-id dependant, but generic cpufreq-cpu0 is assuming there is only >> one statically > > Can't that be handled via DT ? I don't think it can be handled via DT unless we separate DTB according to different CPU speedo/process-id but that is not a good idea. > >> for some SoC the frequency table is not fixed, they are >> created at runtime combining our fast and slow CPU frequency table and dvfs >> table. So I'm really not sure is it worth adding so many tweaks in order to >> use the generic cpufreq-cpu0 driver. > > Hmm, maybe I got confused because I don't have a clear picture in my mind. > It might be better to go ahead with your implementation for now and after > everything is set, we can choose to use cpufreq-cpu0 if it is worth it. > This makes more sense to me, thanks. > -- > viresh > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index ce52ed9..1cc9213 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -225,6 +225,18 @@ config ARM_TEGRA_CPUFREQ help This adds the CPUFreq driver support for TEGRA SOCs. +config ARM_TEGRA20_CPUFREQ + bool "NVIDIA TEGRA20" + depends on ARM_TEGRA_CPUFREQ && ARCH_TEGRA_2x_SOC + default y + help + This enables Tegra20 cpufreq functionality, it adds + Tegra20 CPU frequency ladder and the call back functions + to set CPU rate. All the non-SoC dependant codes are + controlled by the config ARM_TEGRA20_CPUFREQ. + + If in doubt, say N. + config ARM_VEXPRESS_SPC_CPUFREQ tristate "Versatile Express SPC based CPUfreq driver" select ARM_BIG_LITTLE_CPUFREQ diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index 7494565..331964b 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -74,6 +74,7 @@ obj-$(CONFIG_ARM_SA1100_CPUFREQ) += sa1100-cpufreq.o obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o +obj-$(CONFIG_ARM_TEGRA20_CPUFREQ) += tegra20-cpufreq.o obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o ################################################################################## diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c index ae1c0f1..5f19c18 100644 --- a/drivers/cpufreq/tegra-cpufreq.c +++ b/drivers/cpufreq/tegra-cpufreq.c @@ -1,5 +1,6 @@ /* * Copyright (C) 2010 Google, Inc. + * Copyright (c) 2013, NVIDIA Corporation. * * Author: * Colin Cross <ccross@google.com> @@ -18,69 +19,17 @@ #include <linux/kernel.h> #include <linux/module.h> -#include <linux/types.h> -#include <linux/sched.h> #include <linux/cpufreq.h> -#include <linux/delay.h> -#include <linux/init.h> #include <linux/err.h> #include <linux/clk.h> -#include <linux/io.h> - -static struct cpufreq_frequency_table freq_table[] = { - { .frequency = 216000 }, - { .frequency = 312000 }, - { .frequency = 456000 }, - { .frequency = 608000 }, - { .frequency = 760000 }, - { .frequency = 816000 }, - { .frequency = 912000 }, - { .frequency = 1000000 }, - { .frequency = CPUFREQ_TABLE_END }, -}; - -#define NUM_CPUS 2 +#include <linux/cpu.h> +#include <linux/platform_device.h> +#include <linux/of.h> -static struct clk *cpu_clk; -static struct clk *pll_x_clk; -static struct clk *pll_p_clk; -static struct clk *emc_clk; - -static int tegra_cpu_clk_set_rate(unsigned long rate) -{ - int ret; +#include "tegra-cpufreq.h" - /* - * Take an extra reference to the main pll so it doesn't turn - * off when we move the cpu off of it - */ - clk_prepare_enable(pll_x_clk); - - ret = clk_set_parent(cpu_clk, pll_p_clk); - if (ret) { - pr_err("Failed to switch cpu to clock pll_p\n"); - goto out; - } - - if (rate == clk_get_rate(pll_p_clk)) - goto out; - - ret = clk_set_rate(pll_x_clk, rate); - if (ret) { - pr_err("Failed to change pll_x to %lu\n", rate); - goto out; - } - - ret = clk_set_parent(cpu_clk, pll_x_clk); - if (ret) { - pr_err("Failed to switch cpu to clock pll_x\n"); - goto out; - } - -out: - clk_disable_unprepare(pll_x_clk); - return ret; -} +static struct tegra_cpufreq_data *tegra_data; +static const struct tegra_cpufreq_config *soc_config; static int tegra_update_cpu_speed(struct cpufreq_policy *policy, unsigned long rate) @@ -91,14 +40,10 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy, * Vote on memory bus frequency based on cpu frequency * This sets the minimum frequency, display or avp may request higher */ - if (rate >= 816000) - clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */ - else if (rate >= 456000) - clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */ - else - clk_set_rate(emc_clk, 100000000); /* emc 50Mhz */ - - ret = tegra_cpu_clk_set_rate(rate * 1000); + 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); @@ -108,6 +53,8 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy, static int tegra_target(struct cpufreq_policy *policy, unsigned int index) { + struct cpufreq_frequency_table *freq_table = tegra_data->freq_table; + return tegra_update_cpu_speed(policy, freq_table[index].frequency); } @@ -115,29 +62,27 @@ static int tegra_cpu_init(struct cpufreq_policy *policy) { int ret; - if (policy->cpu >= NUM_CPUS) - return -EINVAL; - - clk_prepare_enable(emc_clk); - clk_prepare_enable(cpu_clk); + if (soc_config->cpufreq_clk_init) + soc_config->cpufreq_clk_init(); /* FIXME: what's the actual transition time? */ - ret = cpufreq_generic_init(policy, freq_table, 300 * 1000); + ret = cpufreq_generic_init(policy, tegra_data->freq_table, 300 * 1000); if (ret) { - clk_disable_unprepare(cpu_clk); - clk_disable_unprepare(emc_clk); + if (soc_config->cpufreq_clk_exit) + soc_config->cpufreq_clk_exit(); return ret; } - policy->clk = cpu_clk; - policy->suspend_freq = freq_table[0].frequency; + policy->clk = tegra_data->cpu_clk; + policy->suspend_freq = tegra_data->freq_table[0].frequency; return 0; } static int tegra_cpu_exit(struct cpufreq_policy *policy) { - clk_disable_unprepare(cpu_clk); - clk_disable_unprepare(emc_clk); + cpufreq_frequency_table_cpuinfo(policy, tegra_data->freq_table); + if (soc_config->cpufreq_clk_exit) + soc_config->cpufreq_clk_exit(); return 0; } @@ -155,27 +100,75 @@ static struct cpufreq_driver tegra_cpufreq_driver = { #endif }; -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) { - cpu_clk = clk_get_sys(NULL, "cclk"); - if (IS_ERR(cpu_clk)) - return PTR_ERR(cpu_clk); - - pll_x_clk = clk_get_sys(NULL, "pll_x"); - if (IS_ERR(pll_x_clk)) - return PTR_ERR(pll_x_clk); - - pll_p_clk = clk_get_sys(NULL, "pll_p"); - if (IS_ERR(pll_p_clk)) - return PTR_ERR(pll_p_clk); - - emc_clk = clk_get_sys("cpu", "emc"); - if (IS_ERR(emc_clk)) { - clk_put(cpu_clk); - return PTR_ERR(emc_clk); + int i; + int ret = -EINVAL; + + tegra_data = devm_kzalloc(&pdev->dev, + sizeof(*tegra_data), GFP_KERNEL); + if (!tegra_data) { + ret = -ENOMEM; + goto out; + } + + tegra_data->dev = &pdev->dev; + + 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; + + ret = cpufreq_register_driver(&tegra_cpufreq_driver); + if (ret) { + dev_err(tegra_data->dev, + "%s: failed to register cpufreq driver\n", __func__); + goto out; + } + + return 0; +out: + return ret; +} + +static int tegra_cpufreq_remove(struct platform_device *pdev) +{ + cpufreq_unregister_driver(&tegra_cpufreq_driver); + return 0; +} - return cpufreq_register_driver(&tegra_cpufreq_driver); +static struct platform_driver tegra_cpufreq_platdrv = { + .driver = { + .name = "tegra-cpufreq", + .owner = THIS_MODULE, + }, + .probe = tegra_cpufreq_probe, + .remove = tegra_cpufreq_remove, +}; +module_platform_driver(tegra_cpufreq_platdrv); + +int __init tegra_cpufreq_init(void) +{ + struct platform_device_info devinfo = { .name = "tegra-cpufreq", }; + + platform_device_register_full(&devinfo); + + return 0; } EXPORT_SYMBOL(tegra_cpufreq_init); diff --git a/drivers/cpufreq/tegra-cpufreq.h b/drivers/cpufreq/tegra-cpufreq.h new file mode 100644 index 0000000..0556354 --- /dev/null +++ b/drivers/cpufreq/tegra-cpufreq.h @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2013, NVIDIA Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + */ + +#ifndef __LINUX_TEGRA_CPUFREQ_H +#define __LINUX_TEGRA_CPUFREQ_H + +struct tegra_cpufreq_config { + void (*vote_emc_on_cpu_rate)(unsigned long); + int (*cpu_clk_set_rate)(unsigned long); + void (*cpufreq_clk_init)(void); + void (*cpufreq_clk_exit)(void); +}; + +struct tegra_cpufreq_data { + struct device *dev; + struct clk *cpu_clk; + struct cpufreq_frequency_table *freq_table; +}; + +#ifdef CONFIG_ARM_TEGRA20_CPUFREQ +int tegra20_cpufreq_init(struct tegra_cpufreq_data *data, + const struct tegra_cpufreq_config **config); +#else +static inline int tegra20_cpufreq_init(struct tegra_cpufreq_data *data, + const struct tegra_cpufreq_config **config) +{ return -EINVAL; } +#endif + +#endif /* __LINUX_TEGRA_CPUFREQ_H_ */ diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c new file mode 100644 index 0000000..b5b4e95 --- /dev/null +++ b/drivers/cpufreq/tegra20-cpufreq.c @@ -0,0 +1,136 @@ +/* + * Copyright (c) 2013, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/cpufreq.h> +#include <linux/err.h> +#include <linux/clk.h> +#include <linux/suspend.h> +#include <linux/cpu.h> +#include <linux/platform_device.h> +#include <linux/of.h> + +#include "tegra-cpufreq.h" + +static struct cpufreq_frequency_table freq_table[] = { + { .frequency = 216000 }, + { .frequency = 312000 }, + { .frequency = 456000 }, + { .frequency = 608000 }, + { .frequency = 760000 }, + { .frequency = 816000 }, + { .frequency = 912000 }, + { .frequency = 1000000 }, + { .frequency = CPUFREQ_TABLE_END }, +}; + +static struct clk *cpu_clk; +static struct clk *pll_x_clk; +static struct clk *pll_p_clk; +static struct clk *emc_clk; + +static void tegra20_vote_emc_on_cpu_rate(unsigned long rate) +{ + if (rate >= 816000) + clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */ + else if (rate >= 456000) + clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */ + else + clk_set_rate(emc_clk, 100000000); /* emc 50Mhz */ +} + +static int tegra20_cpu_clk_set_rate(unsigned long rate) +{ + int ret; + + /* + * Take an extra reference to the main pll so it doesn't turn + * off when we move the cpu off of it + */ + clk_prepare_enable(pll_x_clk); + + ret = clk_set_parent(cpu_clk, pll_p_clk); + if (ret) { + pr_err("Failed to switch cpu to clock pll_p\n"); + goto out; + } + + if (rate == clk_get_rate(pll_p_clk)) + goto out; + + ret = clk_set_rate(pll_x_clk, rate); + if (ret) { + pr_err("Failed to change pll_x to %lu\n", rate); + goto out; + } + + ret = clk_set_parent(cpu_clk, pll_x_clk); + if (ret) { + pr_err("Failed to switch cpu to clock pll_x\n"); + goto out; + } + +out: + clk_disable_unprepare(pll_x_clk); + return ret; +} + +static void tegra20_cpufreq_clk_init(void) +{ + clk_prepare_enable(emc_clk); + clk_prepare_enable(cpu_clk); +} + +static void tegra20_cpufreq_clk_exit(void) +{ + clk_disable_unprepare(cpu_clk); + clk_disable_unprepare(emc_clk); +} + +static const struct tegra_cpufreq_config tegra20_cpufreq_config = { + .vote_emc_on_cpu_rate = tegra20_vote_emc_on_cpu_rate, + .cpu_clk_set_rate = tegra20_cpu_clk_set_rate, + .cpufreq_clk_init = tegra20_cpufreq_clk_init, + .cpufreq_clk_exit = tegra20_cpufreq_clk_exit, +}; + +int tegra20_cpufreq_init(struct tegra_cpufreq_data *data, + const struct tegra_cpufreq_config **soc_config) +{ + cpu_clk = clk_get_sys(NULL, "cclk"); + if (IS_ERR(cpu_clk)) + return PTR_ERR(cpu_clk); + + pll_x_clk = clk_get_sys(NULL, "pll_x"); + if (IS_ERR(pll_x_clk)) + return PTR_ERR(pll_x_clk); + + pll_p_clk = clk_get_sys(NULL, "pll_p"); + if (IS_ERR(pll_p_clk)) + return PTR_ERR(pll_p_clk); + + emc_clk = clk_get_sys("cpu", "emc"); + if (IS_ERR(emc_clk)) { + clk_put(cpu_clk); + return PTR_ERR(emc_clk); + } + + data->cpu_clk = cpu_clk; + data->freq_table = freq_table; + *soc_config = &tegra20_cpufreq_config; + + return 0; +} +EXPORT_SYMBOL(tegra20_cpufreq_init);
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). Signed-off-by: Bill Huang <bilhuang@nvidia.com> --- drivers/cpufreq/Kconfig.arm | 12 +++ drivers/cpufreq/Makefile | 1 + drivers/cpufreq/tegra-cpufreq.c | 185 ++++++++++++++++++------------------- drivers/cpufreq/tegra-cpufreq.h | 40 ++++++++ drivers/cpufreq/tegra20-cpufreq.c | 136 +++++++++++++++++++++++++++ 5 files changed, 278 insertions(+), 96 deletions(-) create mode 100644 drivers/cpufreq/tegra-cpufreq.h create mode 100644 drivers/cpufreq/tegra20-cpufreq.c