| Message ID | 1575394348-17649-1-git-send-email-sumitg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [TEGRA194_CPUFREQ,1/3] firmware: tegra: adding function to get BPMP data | expand |
On Tue, Dec 03, 2019 at 11:02:26PM +0530, Sumit Gupta wrote: > Adding new function of_tegra_bpmp_get() to get BPMP data. > This function can be used by other drivers like cpufreq to > get BPMP data without adding any property in respective > drivers DT node. What's wrong with adding the property in the DT node? We already do that for Tegra186's CPU frequency driver, so it makes sense to continue that for Tegra194. Thierry > Signed-off-by: Sumit Gupta <sumitg@nvidia.com> > --- > drivers/firmware/tegra/bpmp.c | 38 ++++++++++++++++++++++++++++++++++++++ > include/soc/tegra/bpmp.h | 5 +++++ > 2 files changed, 43 insertions(+) > > diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c > index 6741fcd..9c3d7f1 100644 > --- a/drivers/firmware/tegra/bpmp.c > +++ b/drivers/firmware/tegra/bpmp.c > @@ -38,6 +38,44 @@ channel_to_ops(struct tegra_bpmp_channel *channel) > return bpmp->soc->ops; > } > > +struct tegra_bpmp *of_tegra_bpmp_get(void) > +{ > + struct platform_device *pdev; > + struct device_node *bpmp_dev; > + struct tegra_bpmp *bpmp; > + > + /* Check for bpmp device status in DT */ > + bpmp_dev = of_find_compatible_node(NULL, NULL, "nvidia,tegra186-bpmp"); > + if (!bpmp_dev) { > + bpmp = ERR_PTR(-ENODEV); > + goto err_out; > + } > + if (!of_device_is_available(bpmp_dev)) { > + bpmp = ERR_PTR(-ENODEV); > + goto err_put; > + } > + > + pdev = of_find_device_by_node(bpmp_dev); > + if (!pdev) { > + bpmp = ERR_PTR(-ENODEV); > + goto err_put; > + } > + > + bpmp = platform_get_drvdata(pdev); > + if (!bpmp) { > + bpmp = ERR_PTR(-EPROBE_DEFER); > + put_device(&pdev->dev); > + goto err_put; > + } > + > + return bpmp; > +err_put: > + of_node_put(bpmp_dev); > +err_out: > + return bpmp; > +} > +EXPORT_SYMBOL_GPL(of_tegra_bpmp_get); > + > struct tegra_bpmp *tegra_bpmp_get(struct device *dev) > { > struct platform_device *pdev; > diff --git a/include/soc/tegra/bpmp.h b/include/soc/tegra/bpmp.h > index f2604e9..21402d9 100644 > --- a/include/soc/tegra/bpmp.h > +++ b/include/soc/tegra/bpmp.h > @@ -107,6 +107,7 @@ struct tegra_bpmp_message { > }; > > #if IS_ENABLED(CONFIG_TEGRA_BPMP) > +struct tegra_bpmp *of_tegra_bpmp_get(void); > struct tegra_bpmp *tegra_bpmp_get(struct device *dev); > void tegra_bpmp_put(struct tegra_bpmp *bpmp); > int tegra_bpmp_transfer_atomic(struct tegra_bpmp *bpmp, > @@ -122,6 +123,10 @@ void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, > void *data); > bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq); > #else > +static inline struct tegra_bpmp *of_tegra_bpmp_get(void) > +{ > + return ERR_PTR(-ENOTSUPP); > +} > static inline struct tegra_bpmp *tegra_bpmp_get(struct device *dev) > { > return ERR_PTR(-ENOTSUPP); > -- > 2.7.4 >
The difference here is that whereas on Tegra186 the frequency is managed through a specific memory-mapped device, on Tegra194 the frequency is managed through a CPU MSR leaving no "specific" node for this property apart from the cpu nodes itself. Now, my original patchset (which this series is based on) did add nvidia,bpmp properties on the CPU DT nodes itself and query BPMP based on that. A change is still required for that since tegra_bpmp_get() currently takes a 'struct device *' which we don't have for a CPU DT node. Mikko On 3.12.2019 19.42, Thierry Reding wrote: > On Tue, Dec 03, 2019 at 11:02:26PM +0530, Sumit Gupta wrote: >> Adding new function of_tegra_bpmp_get() to get BPMP data. >> This function can be used by other drivers like cpufreq to >> get BPMP data without adding any property in respective >> drivers DT node. > > What's wrong with adding the property in the DT node? We already do that > for Tegra186's CPU frequency driver, so it makes sense to continue that > for Tegra194. > > Thierry > >> Signed-off-by: Sumit Gupta <sumitg@nvidia.com> >> --- >> drivers/firmware/tegra/bpmp.c | 38 ++++++++++++++++++++++++++++++++++++++ >> include/soc/tegra/bpmp.h | 5 +++++ >> 2 files changed, 43 insertions(+) >> >> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c >> index 6741fcd..9c3d7f1 100644 >> --- a/drivers/firmware/tegra/bpmp.c >> +++ b/drivers/firmware/tegra/bpmp.c >> @@ -38,6 +38,44 @@ channel_to_ops(struct tegra_bpmp_channel *channel) >> return bpmp->soc->ops; >> } >> >> +struct tegra_bpmp *of_tegra_bpmp_get(void) >> +{ >> + struct platform_device *pdev; >> + struct device_node *bpmp_dev; >> + struct tegra_bpmp *bpmp; >> + >> + /* Check for bpmp device status in DT */ >> + bpmp_dev = of_find_compatible_node(NULL, NULL, "nvidia,tegra186-bpmp"); >> + if (!bpmp_dev) { >> + bpmp = ERR_PTR(-ENODEV); >> + goto err_out; >> + } >> + if (!of_device_is_available(bpmp_dev)) { >> + bpmp = ERR_PTR(-ENODEV); >> + goto err_put; >> + } >> + >> + pdev = of_find_device_by_node(bpmp_dev); >> + if (!pdev) { >> + bpmp = ERR_PTR(-ENODEV); >> + goto err_put; >> + } >> + >> + bpmp = platform_get_drvdata(pdev); >> + if (!bpmp) { >> + bpmp = ERR_PTR(-EPROBE_DEFER); >> + put_device(&pdev->dev); >> + goto err_put; >> + } >> + >> + return bpmp; >> +err_put: >> + of_node_put(bpmp_dev); >> +err_out: >> + return bpmp; >> +} >> +EXPORT_SYMBOL_GPL(of_tegra_bpmp_get); >> + >> struct tegra_bpmp *tegra_bpmp_get(struct device *dev) >> { >> struct platform_device *pdev; >> diff --git a/include/soc/tegra/bpmp.h b/include/soc/tegra/bpmp.h >> index f2604e9..21402d9 100644 >> --- a/include/soc/tegra/bpmp.h >> +++ b/include/soc/tegra/bpmp.h >> @@ -107,6 +107,7 @@ struct tegra_bpmp_message { >> }; >> >> #if IS_ENABLED(CONFIG_TEGRA_BPMP) >> +struct tegra_bpmp *of_tegra_bpmp_get(void); >> struct tegra_bpmp *tegra_bpmp_get(struct device *dev); >> void tegra_bpmp_put(struct tegra_bpmp *bpmp); >> int tegra_bpmp_transfer_atomic(struct tegra_bpmp *bpmp, >> @@ -122,6 +123,10 @@ void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, >> void *data); >> bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq); >> #else >> +static inline struct tegra_bpmp *of_tegra_bpmp_get(void) >> +{ >> + return ERR_PTR(-ENOTSUPP); >> +} >> static inline struct tegra_bpmp *tegra_bpmp_get(struct device *dev) >> { >> return ERR_PTR(-ENOTSUPP); >> -- >> 2.7.4 >>
On 04-12-19, 10:45, Mikko Perttunen wrote: > Now, my original patchset (which this series is based on) did add > nvidia,bpmp properties on the CPU DT nodes itself and query BPMP based on > that. A change is still required for that since tegra_bpmp_get() currently > takes a 'struct device *' which we don't have for a CPU DT node. I may be missing the context, but the CPUs always have a struct device * for them, which we get via a call to get_cpu_device(cpu), isn't ?
On Wed, Dec 04, 2019 at 02:47:03PM +0530, Viresh Kumar wrote: > On 04-12-19, 10:45, Mikko Perttunen wrote: > > Now, my original patchset (which this series is based on) did add > > nvidia,bpmp properties on the CPU DT nodes itself and query BPMP based on > > that. A change is still required for that since tegra_bpmp_get() currently > > takes a 'struct device *' which we don't have for a CPU DT node. > > I may be missing the context, but the CPUs always have a struct device > * for them, which we get via a call to get_cpu_device(cpu), isn't ? Yeah, the code that registers this device is in drivers/base/cpu.c in register_cpu(). It even retrieves the device tree node for the CPU from device tree and stores it in cpu->dev.of_node, so we should be able to just pass &cpu->dev to tegra_bpmp_get() in order to retrieve a reference to the BPMP. That said, I'm wondering if perhaps we could just add a compatible string to the /cpus node for cases like this where we don't have an actual device representing the CPU complex. There are a number of CPU frequency drivers that register dummy devices just so that they have something to bind a driver to. If we allow the /cpus node to represent the CPU complex (if no other "device" does that yet), we can add a compatible string and have the cpufreq driver match on that. Of course this would be slightly difficult to retrofit into existing drivers because they'd need to remain backwards compatible with existing device trees. But it would allow future drivers to do this a little more elegantly. For some SoCs this may not matter, but especially once you start depending on additional resources this would come in handy. Adding Rob and the device tree mailing list for feedback on this idea. Thierry
On 04-12-19, 10:33, Thierry Reding wrote: > Yeah, the code that registers this device is in drivers/base/cpu.c in > register_cpu(). It even retrieves the device tree node for the CPU from > device tree and stores it in cpu->dev.of_node, so we should be able to > just pass &cpu->dev to tegra_bpmp_get() in order to retrieve a reference > to the BPMP. > > That said, I'm wondering if perhaps we could just add a compatible > string to the /cpus node for cases like this where we don't have an > actual device representing the CPU complex. There are a number of CPU > frequency drivers that register dummy devices just so that they have > something to bind a driver to. > > If we allow the /cpus node to represent the CPU complex (if no other > "device" does that yet), we can add a compatible string and have the > cpufreq driver match on that. > > Of course this would be slightly difficult to retrofit into existing > drivers because they'd need to remain backwards compatible with existing > device trees. But it would allow future drivers to do this a little more > elegantly. For some SoCs this may not matter, but especially once you > start depending on additional resources this would come in handy. > > Adding Rob and the device tree mailing list for feedback on this idea. Took some time to find this thread, but something around this was suggested by Rafael earlier. https://lore.kernel.org/lkml/8139001.Q4eV8YG1Il@vostro.rjw.lan/
Ah, it seems I was mistaken here. Thanks for the information. Thanks, Mikko On 4.12.2019 11.17, Viresh Kumar wrote: > On 04-12-19, 10:45, Mikko Perttunen wrote: >> Now, my original patchset (which this series is based on) did add >> nvidia,bpmp properties on the CPU DT nodes itself and query BPMP based on >> that. A change is still required for that since tegra_bpmp_get() currently >> takes a 'struct device *' which we don't have for a CPU DT node. > > I may be missing the context, but the CPUs always have a struct device > * for them, which we get via a call to get_cpu_device(cpu), isn't ? >
On 04-12-19, 12:21, Mikko Perttunen wrote:
> Ah, it seems I was mistaken here. Thanks for the information.
Please avoid top-posting [1][2] for LKML discussions.
Thanks.
On Wed, Dec 04, 2019 at 03:21:38PM +0530, Viresh Kumar wrote: > On 04-12-19, 10:33, Thierry Reding wrote: > > Yeah, the code that registers this device is in drivers/base/cpu.c in > > register_cpu(). It even retrieves the device tree node for the CPU from > > device tree and stores it in cpu->dev.of_node, so we should be able to > > just pass &cpu->dev to tegra_bpmp_get() in order to retrieve a reference > > to the BPMP. > > > > That said, I'm wondering if perhaps we could just add a compatible > > string to the /cpus node for cases like this where we don't have an > > actual device representing the CPU complex. There are a number of CPU > > frequency drivers that register dummy devices just so that they have > > something to bind a driver to. > > > > If we allow the /cpus node to represent the CPU complex (if no other > > "device" does that yet), we can add a compatible string and have the > > cpufreq driver match on that. > > > > Of course this would be slightly difficult to retrofit into existing > > drivers because they'd need to remain backwards compatible with existing > > device trees. But it would allow future drivers to do this a little more > > elegantly. For some SoCs this may not matter, but especially once you > > start depending on additional resources this would come in handy. > > > > Adding Rob and the device tree mailing list for feedback on this idea. > > Took some time to find this thread, but something around this was > suggested by Rafael earlier. > > https://lore.kernel.org/lkml/8139001.Q4eV8YG1Il@vostro.rjw.lan/ I gave this a try and came up with the following: --- >8 --- diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi index f4ede86e32b4..e4462f95f0b3 100644 --- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi +++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi @@ -1764,6 +1764,9 @@ bpmp_thermal: thermal { }; cpus { + compatible = "nvidia,tegra194-ccplex"; + nvidia,bpmp = <&bpmp>; + #address-cells = <1>; #size-cells = <0>; --- >8 --- Now I can do something rougly like this, although I have a more complete patch locally that also gets rid of all the global variables because we now actually have a struct platform_device that we can anchor everything at: --- >8 --- static const struct of_device_id tegra194_cpufreq_of_match[] = { { .compatible = "nvidia,tegra194-ccplex", }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, tegra194_cpufreq_of_match); static struct platform_driver tegra194_ccplex_driver = { .driver = { .name = "tegra194-cpufreq", .of_match_table = tegra194_cpufreq_of_match, }, .probe = tegra194_cpufreq_probe, .remove = tegra194_cpufreq_remove, }; module_platform_driver(tegra194_ccplex_driver); --- >8 --- I don't think that's exactly what Rafael (Cc'ed) had in mind, since the above thread seems to have mostly talked about binding a driver to each individual CPU. But this seems a lot better than having to instantiate a device from scratch just so that a driver can bind to it and it allows additional properties to be associated with the CCPLEX device. Rob, any thoughts on this from a device tree point of view? The /cpus bindings don't mention the compatible property, but there doesn't seem to be anything in the bindings that would prohibit its use. If we can agree on that, I can forward my local changes to Sumit for inclusion or reference. Thierry
On Tue, Apr 07, 2020 at 12:05:20PM +0200, Thierry Reding wrote: > On Wed, Dec 04, 2019 at 03:21:38PM +0530, Viresh Kumar wrote: > > On 04-12-19, 10:33, Thierry Reding wrote: > > > Yeah, the code that registers this device is in drivers/base/cpu.c in > > > register_cpu(). It even retrieves the device tree node for the CPU from > > > device tree and stores it in cpu->dev.of_node, so we should be able to > > > just pass &cpu->dev to tegra_bpmp_get() in order to retrieve a reference > > > to the BPMP. > > > > > > That said, I'm wondering if perhaps we could just add a compatible > > > string to the /cpus node for cases like this where we don't have an > > > actual device representing the CPU complex. There are a number of CPU > > > frequency drivers that register dummy devices just so that they have > > > something to bind a driver to. > > > > > > If we allow the /cpus node to represent the CPU complex (if no other > > > "device" does that yet), we can add a compatible string and have the > > > cpufreq driver match on that. > > > > > > Of course this would be slightly difficult to retrofit into existing > > > drivers because they'd need to remain backwards compatible with existing > > > device trees. But it would allow future drivers to do this a little more > > > elegantly. For some SoCs this may not matter, but especially once you > > > start depending on additional resources this would come in handy. > > > > > > Adding Rob and the device tree mailing list for feedback on this idea. > > > > Took some time to find this thread, but something around this was > > suggested by Rafael earlier. > > > > https://lore.kernel.org/lkml/8139001.Q4eV8YG1Il@vostro.rjw.lan/ > > I gave this a try and came up with the following: > > --- >8 --- > diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi > index f4ede86e32b4..e4462f95f0b3 100644 > --- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi > +++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi > @@ -1764,6 +1764,9 @@ bpmp_thermal: thermal { > }; > > cpus { > + compatible = "nvidia,tegra194-ccplex"; > + nvidia,bpmp = <&bpmp>; > + > #address-cells = <1>; > #size-cells = <0>; > > --- >8 --- > > Now I can do something rougly like this, although I have a more complete > patch locally that also gets rid of all the global variables because we > now actually have a struct platform_device that we can anchor everything > at: > > --- >8 --- > static const struct of_device_id tegra194_cpufreq_of_match[] = { > { .compatible = "nvidia,tegra194-ccplex", }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, tegra194_cpufreq_of_match); > > static struct platform_driver tegra194_ccplex_driver = { > .driver = { > .name = "tegra194-cpufreq", > .of_match_table = tegra194_cpufreq_of_match, > }, > .probe = tegra194_cpufreq_probe, > .remove = tegra194_cpufreq_remove, > }; > module_platform_driver(tegra194_ccplex_driver); > --- >8 --- > > I don't think that's exactly what Rafael (Cc'ed) had in mind, since the > above thread seems to have mostly talked about binding a driver to each > individual CPU. > > But this seems a lot better than having to instantiate a device from > scratch just so that a driver can bind to it and it allows additional > properties to be associated with the CCPLEX device. > > Rob, any thoughts on this from a device tree point of view? The /cpus > bindings don't mention the compatible property, but there doesn't seem > to be anything in the bindings that would prohibit its use. > > If we can agree on that, I can forward my local changes to Sumit for > inclusion or reference. Rob, do you see any reason why we shouldn't be able to use a compatible string in the /cpus node for devices such as Tegra194 where there is no dedicated hardware block for the CCPLEX? Thierry
On 27/04/20 12:48 PM, Thierry Reding wrote: > On Tue, Apr 07, 2020 at 12:05:20PM +0200, Thierry Reding wrote: >> On Wed, Dec 04, 2019 at 03:21:38PM +0530, Viresh Kumar wrote: >>> On 04-12-19, 10:33, Thierry Reding wrote: >>>> Yeah, the code that registers this device is in drivers/base/cpu.c in >>>> register_cpu(). It even retrieves the device tree node for the CPU from >>>> device tree and stores it in cpu->dev.of_node, so we should be able to >>>> just pass &cpu->dev to tegra_bpmp_get() in order to retrieve a reference >>>> to the BPMP. >>>> >>>> That said, I'm wondering if perhaps we could just add a compatible >>>> string to the /cpus node for cases like this where we don't have an >>>> actual device representing the CPU complex. There are a number of CPU >>>> frequency drivers that register dummy devices just so that they have >>>> something to bind a driver to. >>>> >>>> If we allow the /cpus node to represent the CPU complex (if no other >>>> "device" does that yet), we can add a compatible string and have the >>>> cpufreq driver match on that. >>>> >>>> Of course this would be slightly difficult to retrofit into existing >>>> drivers because they'd need to remain backwards compatible with existing >>>> device trees. But it would allow future drivers to do this a little more >>>> elegantly. For some SoCs this may not matter, but especially once you >>>> start depending on additional resources this would come in handy. >>>> >>>> Adding Rob and the device tree mailing list for feedback on this idea. >>> >>> Took some time to find this thread, but something around this was >>> suggested by Rafael earlier. >>> >>> https://lore.kernel.org/lkml/8139001.Q4eV8YG1Il@vostro.rjw.lan/ >> >> I gave this a try and came up with the following: >> >> --- >8 --- >> diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi >> index f4ede86e32b4..e4462f95f0b3 100644 >> --- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi >> +++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi >> @@ -1764,6 +1764,9 @@ bpmp_thermal: thermal { >> }; >> >> cpus { >> + compatible = "nvidia,tegra194-ccplex"; >> + nvidia,bpmp = <&bpmp>; >> + >> #address-cells = <1>; >> #size-cells = <0>; >> >> --- >8 --- >> >> Now I can do something rougly like this, although I have a more complete >> patch locally that also gets rid of all the global variables because we >> now actually have a struct platform_device that we can anchor everything >> at: >> >> --- >8 --- >> static const struct of_device_id tegra194_cpufreq_of_match[] = { >> { .compatible = "nvidia,tegra194-ccplex", }, >> { /* sentinel */ } >> }; >> MODULE_DEVICE_TABLE(of, tegra194_cpufreq_of_match); >> >> static struct platform_driver tegra194_ccplex_driver = { >> .driver = { >> .name = "tegra194-cpufreq", >> .of_match_table = tegra194_cpufreq_of_match, >> }, >> .probe = tegra194_cpufreq_probe, >> .remove = tegra194_cpufreq_remove, >> }; >> module_platform_driver(tegra194_ccplex_driver); >> --- >8 --- >> >> I don't think that's exactly what Rafael (Cc'ed) had in mind, since the >> above thread seems to have mostly talked about binding a driver to each >> individual CPU. >> >> But this seems a lot better than having to instantiate a device from >> scratch just so that a driver can bind to it and it allows additional >> properties to be associated with the CCPLEX device. >> >> Rob, any thoughts on this from a device tree point of view? The /cpus >> bindings don't mention the compatible property, but there doesn't seem >> to be anything in the bindings that would prohibit its use. >> >> If we can agree on that, I can forward my local changes to Sumit for >> inclusion or reference. > > Rob, do you see any reason why we shouldn't be able to use a compatible > string in the /cpus node for devices such as Tegra194 where there is no > dedicated hardware block for the CCPLEX? > > Thierry > Ping. Please suggest if we can add compatible string in the '/cpus' node. If not then i propose accepting the current patch to get BPMP data without adding any property in respective driver's DT node. We can push separate patch later if we need to add compatible string in the '/cpus' node or create new DT node for cpufreq. Regards, Sumit
On Mon, Apr 27, 2020 at 09:18:00AM +0200, Thierry Reding wrote: > On Tue, Apr 07, 2020 at 12:05:20PM +0200, Thierry Reding wrote: > > On Wed, Dec 04, 2019 at 03:21:38PM +0530, Viresh Kumar wrote: > > > On 04-12-19, 10:33, Thierry Reding wrote: > > > > Yeah, the code that registers this device is in drivers/base/cpu.c in > > > > register_cpu(). It even retrieves the device tree node for the CPU from > > > > device tree and stores it in cpu->dev.of_node, so we should be able to > > > > just pass &cpu->dev to tegra_bpmp_get() in order to retrieve a reference > > > > to the BPMP. > > > > > > > > That said, I'm wondering if perhaps we could just add a compatible > > > > string to the /cpus node for cases like this where we don't have an > > > > actual device representing the CPU complex. There are a number of CPU > > > > frequency drivers that register dummy devices just so that they have > > > > something to bind a driver to. > > > > > > > > If we allow the /cpus node to represent the CPU complex (if no other > > > > "device" does that yet), we can add a compatible string and have the > > > > cpufreq driver match on that. > > > > > > > > Of course this would be slightly difficult to retrofit into existing > > > > drivers because they'd need to remain backwards compatible with existing > > > > device trees. But it would allow future drivers to do this a little more > > > > elegantly. For some SoCs this may not matter, but especially once you > > > > start depending on additional resources this would come in handy. > > > > > > > > Adding Rob and the device tree mailing list for feedback on this idea. > > > > > > Took some time to find this thread, but something around this was > > > suggested by Rafael earlier. > > > > > > https://lore.kernel.org/lkml/8139001.Q4eV8YG1Il@vostro.rjw.lan/ > > > > I gave this a try and came up with the following: > > > > --- >8 --- > > diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi > > index f4ede86e32b4..e4462f95f0b3 100644 > > --- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi > > +++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi > > @@ -1764,6 +1764,9 @@ bpmp_thermal: thermal { > > }; > > > > cpus { > > + compatible = "nvidia,tegra194-ccplex"; > > + nvidia,bpmp = <&bpmp>; > > + > > #address-cells = <1>; > > #size-cells = <0>; > > > > --- >8 --- > > > > Now I can do something rougly like this, although I have a more complete > > patch locally that also gets rid of all the global variables because we > > now actually have a struct platform_device that we can anchor everything > > at: > > > > --- >8 --- > > static const struct of_device_id tegra194_cpufreq_of_match[] = { > > { .compatible = "nvidia,tegra194-ccplex", }, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, tegra194_cpufreq_of_match); > > > > static struct platform_driver tegra194_ccplex_driver = { > > .driver = { > > .name = "tegra194-cpufreq", > > .of_match_table = tegra194_cpufreq_of_match, > > }, > > .probe = tegra194_cpufreq_probe, > > .remove = tegra194_cpufreq_remove, > > }; > > module_platform_driver(tegra194_ccplex_driver); > > --- >8 --- > > > > I don't think that's exactly what Rafael (Cc'ed) had in mind, since the > > above thread seems to have mostly talked about binding a driver to each > > individual CPU. > > > > But this seems a lot better than having to instantiate a device from > > scratch just so that a driver can bind to it and it allows additional > > properties to be associated with the CCPLEX device. > > > > Rob, any thoughts on this from a device tree point of view? The /cpus > > bindings don't mention the compatible property, but there doesn't seem > > to be anything in the bindings that would prohibit its use. > > > > If we can agree on that, I can forward my local changes to Sumit for > > inclusion or reference. > > Rob, do you see any reason why we shouldn't be able to use a compatible > string in the /cpus node for devices such as Tegra194 where there is no > dedicated hardware block for the CCPLEX? Rob, can you take a brief look and provide some feedback on this? I'd like to get this merged for v5.8 and where to instantiate this is the only thing holding this up at this point. Thanks, Thierry
On Tue, Apr 7, 2020 at 4:05 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Dec 04, 2019 at 03:21:38PM +0530, Viresh Kumar wrote: > > On 04-12-19, 10:33, Thierry Reding wrote: > > > Yeah, the code that registers this device is in drivers/base/cpu.c in > > > register_cpu(). It even retrieves the device tree node for the CPU from > > > device tree and stores it in cpu->dev.of_node, so we should be able to > > > just pass &cpu->dev to tegra_bpmp_get() in order to retrieve a reference > > > to the BPMP. > > > > > > That said, I'm wondering if perhaps we could just add a compatible > > > string to the /cpus node for cases like this where we don't have an > > > actual device representing the CPU complex. There are a number of CPU > > > frequency drivers that register dummy devices just so that they have > > > something to bind a driver to. > > > > > > If we allow the /cpus node to represent the CPU complex (if no other > > > "device" does that yet), we can add a compatible string and have the > > > cpufreq driver match on that. > > > > > > Of course this would be slightly difficult to retrofit into existing > > > drivers because they'd need to remain backwards compatible with existing > > > device trees. But it would allow future drivers to do this a little more > > > elegantly. For some SoCs this may not matter, but especially once you > > > start depending on additional resources this would come in handy. > > > > > > Adding Rob and the device tree mailing list for feedback on this idea. > > > > Took some time to find this thread, but something around this was > > suggested by Rafael earlier. > > > > https://lore.kernel.org/lkml/8139001.Q4eV8YG1Il@vostro.rjw.lan/ > > I gave this a try and came up with the following: > > --- >8 --- > diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi > index f4ede86e32b4..e4462f95f0b3 100644 > --- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi > +++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi > @@ -1764,6 +1764,9 @@ bpmp_thermal: thermal { > }; > > cpus { > + compatible = "nvidia,tegra194-ccplex"; > + nvidia,bpmp = <&bpmp>; Is there more than 1 bpmp? If not you don't need this. Just lookup the node by compatible. > + > #address-cells = <1>; > #size-cells = <0>; > > --- >8 --- > > Now I can do something rougly like this, although I have a more complete > patch locally that also gets rid of all the global variables because we > now actually have a struct platform_device that we can anchor everything > at: > > --- >8 --- > static const struct of_device_id tegra194_cpufreq_of_match[] = { > { .compatible = "nvidia,tegra194-ccplex", }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, tegra194_cpufreq_of_match); > > static struct platform_driver tegra194_ccplex_driver = { > .driver = { > .name = "tegra194-cpufreq", > .of_match_table = tegra194_cpufreq_of_match, > }, > .probe = tegra194_cpufreq_probe, > .remove = tegra194_cpufreq_remove, > }; > module_platform_driver(tegra194_ccplex_driver); > --- >8 --- > > I don't think that's exactly what Rafael (Cc'ed) had in mind, since the > above thread seems to have mostly talked about binding a driver to each > individual CPU. > > But this seems a lot better than having to instantiate a device from > scratch just so that a driver can bind to it and it allows additional > properties to be associated with the CCPLEX device. What additional properties? A continual stream of properties added 1 by 1 would negatively affect my opinion of this. > Rob, any thoughts on this from a device tree point of view? The /cpus > bindings don't mention the compatible property, but there doesn't seem > to be anything in the bindings that would prohibit its use. What happens when you have more than one cpu related driver in addition to cpufreq? You may still have to end up creating child platform devices and then gained very little. You could solve this without DT changes. You can bind on node names. The driver probe can then check the parent compatible and return if not matching. I'm not sure if you could get module auto loading to work in that case. It would have to be based on the root compatible (rather than the driver match table) and be able to load multiple matching modules. Rob
On Wed, May 20, 2020 at 08:43:03AM -0600, Rob Herring wrote: > On Tue, Apr 7, 2020 at 4:05 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Wed, Dec 04, 2019 at 03:21:38PM +0530, Viresh Kumar wrote: > > > On 04-12-19, 10:33, Thierry Reding wrote: > > > > Yeah, the code that registers this device is in drivers/base/cpu.c in > > > > register_cpu(). It even retrieves the device tree node for the CPU from > > > > device tree and stores it in cpu->dev.of_node, so we should be able to > > > > just pass &cpu->dev to tegra_bpmp_get() in order to retrieve a reference > > > > to the BPMP. > > > > > > > > That said, I'm wondering if perhaps we could just add a compatible > > > > string to the /cpus node for cases like this where we don't have an > > > > actual device representing the CPU complex. There are a number of CPU > > > > frequency drivers that register dummy devices just so that they have > > > > something to bind a driver to. > > > > > > > > If we allow the /cpus node to represent the CPU complex (if no other > > > > "device" does that yet), we can add a compatible string and have the > > > > cpufreq driver match on that. > > > > > > > > Of course this would be slightly difficult to retrofit into existing > > > > drivers because they'd need to remain backwards compatible with existing > > > > device trees. But it would allow future drivers to do this a little more > > > > elegantly. For some SoCs this may not matter, but especially once you > > > > start depending on additional resources this would come in handy. > > > > > > > > Adding Rob and the device tree mailing list for feedback on this idea. > > > > > > Took some time to find this thread, but something around this was > > > suggested by Rafael earlier. > > > > > > https://lore.kernel.org/lkml/8139001.Q4eV8YG1Il@vostro.rjw.lan/ > > > > I gave this a try and came up with the following: > > > > --- >8 --- > > diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi > > index f4ede86e32b4..e4462f95f0b3 100644 > > --- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi > > +++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi > > @@ -1764,6 +1764,9 @@ bpmp_thermal: thermal { > > }; > > > > cpus { > > + compatible = "nvidia,tegra194-ccplex"; > > + nvidia,bpmp = <&bpmp>; > > Is there more than 1 bpmp? If not you don't need this. Just lookup the > node by compatible. There no SoCs currently than need to differentiate between multiple BPMPs, so yes, it would be possible to look up the node by compatible. But we also used to assume that PCs would only ever come with a single GPU or audio card and that's always caused a lot of work to clean up when it turned out to no longer be true. Also, we already have a couple of devices referencing the BPMP by phandle like this, so having this in a CCPLEX node would keep things consistent. One of the reasons why we initially did it this way was also so that we could make the dependencies explicit within device tree. If we look up by compatible string, then the driver is the only one with the knowledge about where to get at it. If we have the explicit reference we at least have a chance of determining the dependency by just looking at the device tree. > > + > > #address-cells = <1>; > > #size-cells = <0>; > > > > --- >8 --- > > > > Now I can do something rougly like this, although I have a more complete > > patch locally that also gets rid of all the global variables because we > > now actually have a struct platform_device that we can anchor everything > > at: > > > > --- >8 --- > > static const struct of_device_id tegra194_cpufreq_of_match[] = { > > { .compatible = "nvidia,tegra194-ccplex", }, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, tegra194_cpufreq_of_match); > > > > static struct platform_driver tegra194_ccplex_driver = { > > .driver = { > > .name = "tegra194-cpufreq", > > .of_match_table = tegra194_cpufreq_of_match, > > }, > > .probe = tegra194_cpufreq_probe, > > .remove = tegra194_cpufreq_remove, > > }; > > module_platform_driver(tegra194_ccplex_driver); > > --- >8 --- > > > > I don't think that's exactly what Rafael (Cc'ed) had in mind, since the > > above thread seems to have mostly talked about binding a driver to each > > individual CPU. > > > > But this seems a lot better than having to instantiate a device from > > scratch just so that a driver can bind to it and it allows additional > > properties to be associated with the CCPLEX device. > > What additional properties? A continual stream of properties added 1 > by 1 would negatively affect my opinion of this. I don't expect there would be many. I think there's an earlier generation of Tegra that requires a regulator and I can imagine that's pretty common. But other than that I would expect this to be a fairly narrow set of properties. > > Rob, any thoughts on this from a device tree point of view? The /cpus > > bindings don't mention the compatible property, but there doesn't seem > > to be anything in the bindings that would prohibit its use. > > What happens when you have more than one cpu related driver in > addition to cpufreq? You may still have to end up creating child > platform devices and then gained very little. That's only if you absolutely want to stick with the "one driver per subsystem" model. I personally think that's completely obsolete these days. If you have a CPU complex device that can do both CPU frequency scaling and put the CPU into idle states, for example, then there is really no reason to artificially split that into two separate drivers just to match the subsystems that we have. Most subsystems that I've come across work just fine if a single driver registers with multiple subsystems. I also know that some people like it better when things are nicely split up into multiple drivers. But I really don't see how that simplifies things. In fact in my opinion that makes things only more complicated because you have additional boilerplate and then you need to be extra careful about how these different drivers are ordered, and you need to take extra precautions when sharing things like clocks and register regions. > You could solve this without DT changes. You can bind on node names. > The driver probe can then check the parent compatible and return if > not matching. I'm not sure if you could get module auto loading to > work in that case. It would have to be based on the root compatible > (rather than the driver match table) and be able to load multiple > matching modules. That sounds like it would get very complicated for something this simple. Having a compatible string in /cpus seemed like the most logical option because it would basically just work out of the box and the same way we're used to from other devices. Thierry
On Wed, May 20, 2020 at 9:39 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, May 20, 2020 at 08:43:03AM -0600, Rob Herring wrote: > > On Tue, Apr 7, 2020 at 4:05 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > On Wed, Dec 04, 2019 at 03:21:38PM +0530, Viresh Kumar wrote: > > > > On 04-12-19, 10:33, Thierry Reding wrote: > > > > > Yeah, the code that registers this device is in drivers/base/cpu.c in > > > > > register_cpu(). It even retrieves the device tree node for the CPU from > > > > > device tree and stores it in cpu->dev.of_node, so we should be able to > > > > > just pass &cpu->dev to tegra_bpmp_get() in order to retrieve a reference > > > > > to the BPMP. > > > > > > > > > > That said, I'm wondering if perhaps we could just add a compatible > > > > > string to the /cpus node for cases like this where we don't have an > > > > > actual device representing the CPU complex. There are a number of CPU > > > > > frequency drivers that register dummy devices just so that they have > > > > > something to bind a driver to. > > > > > > > > > > If we allow the /cpus node to represent the CPU complex (if no other > > > > > "device" does that yet), we can add a compatible string and have the > > > > > cpufreq driver match on that. > > > > > > > > > > Of course this would be slightly difficult to retrofit into existing > > > > > drivers because they'd need to remain backwards compatible with existing > > > > > device trees. But it would allow future drivers to do this a little more > > > > > elegantly. For some SoCs this may not matter, but especially once you > > > > > start depending on additional resources this would come in handy. > > > > > > > > > > Adding Rob and the device tree mailing list for feedback on this idea. > > > > > > > > Took some time to find this thread, but something around this was > > > > suggested by Rafael earlier. > > > > > > > > https://lore.kernel.org/lkml/8139001.Q4eV8YG1Il@vostro.rjw.lan/ > > > > > > I gave this a try and came up with the following: > > > > > > --- >8 --- > > > diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi > > > index f4ede86e32b4..e4462f95f0b3 100644 > > > --- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi > > > +++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi > > > @@ -1764,6 +1764,9 @@ bpmp_thermal: thermal { > > > }; > > > > > > cpus { > > > + compatible = "nvidia,tegra194-ccplex"; > > > + nvidia,bpmp = <&bpmp>; > > > > Is there more than 1 bpmp? If not you don't need this. Just lookup the > > node by compatible. > > There no SoCs currently than need to differentiate between multiple > BPMPs, so yes, it would be possible to look up the node by compatible. > But we also used to assume that PCs would only ever come with a single > GPU or audio card and that's always caused a lot of work to clean up > when it turned out to no longer be true. Job security. ;) > Also, we already have a couple of devices referencing the BPMP by > phandle like this, so having this in a CCPLEX node would keep things > consistent. > > One of the reasons why we initially did it this way was also so that we > could make the dependencies explicit within device tree. If we look up > by compatible string, then the driver is the only one with the knowledge > about where to get at it. If we have the explicit reference we at least > have a chance of determining the dependency by just looking at the > device tree. This case probably makes sense, but then driver dependencies can evolve and you'd be updating the DT for every dependency. There's just this general mindset that a driver can't look at the DT outside of its own node. > > > #address-cells = <1>; > > > #size-cells = <0>; > > > > > > --- >8 --- > > > > > > Now I can do something rougly like this, although I have a more complete > > > patch locally that also gets rid of all the global variables because we > > > now actually have a struct platform_device that we can anchor everything > > > at: > > > > > > --- >8 --- > > > static const struct of_device_id tegra194_cpufreq_of_match[] = { > > > { .compatible = "nvidia,tegra194-ccplex", }, > > > { /* sentinel */ } > > > }; > > > MODULE_DEVICE_TABLE(of, tegra194_cpufreq_of_match); > > > > > > static struct platform_driver tegra194_ccplex_driver = { > > > .driver = { > > > .name = "tegra194-cpufreq", > > > .of_match_table = tegra194_cpufreq_of_match, > > > }, > > > .probe = tegra194_cpufreq_probe, > > > .remove = tegra194_cpufreq_remove, > > > }; > > > module_platform_driver(tegra194_ccplex_driver); > > > --- >8 --- > > > > > > I don't think that's exactly what Rafael (Cc'ed) had in mind, since the > > > above thread seems to have mostly talked about binding a driver to each > > > individual CPU. > > > > > > But this seems a lot better than having to instantiate a device from > > > scratch just so that a driver can bind to it and it allows additional > > > properties to be associated with the CCPLEX device. > > > > What additional properties? A continual stream of properties added 1 > > by 1 would negatively affect my opinion of this. > > I don't expect there would be many. I think there's an earlier > generation of Tegra that requires a regulator and I can imagine that's > pretty common. But other than that I would expect this to be a fairly > narrow set of properties. > > > > Rob, any thoughts on this from a device tree point of view? The /cpus > > > bindings don't mention the compatible property, but there doesn't seem > > > to be anything in the bindings that would prohibit its use. > > > > What happens when you have more than one cpu related driver in > > addition to cpufreq? You may still have to end up creating child > > platform devices and then gained very little. > > That's only if you absolutely want to stick with the "one driver per > subsystem" model. I personally think that's completely obsolete these > days. If you have a CPU complex device that can do both CPU frequency > scaling and put the CPU into idle states, for example, then there is > really no reason to artificially split that into two separate drivers > just to match the subsystems that we have. > > Most subsystems that I've come across work just fine if a single driver > registers with multiple subsystems. Yes exactly. If only everyone thought this way... > I also know that some people like it better when things are nicely split > up into multiple drivers. But I really don't see how that simplifies > things. In fact in my opinion that makes things only more complicated > because you have additional boilerplate and then you need to be extra > careful about how these different drivers are ordered, and you need to > take extra precautions when sharing things like clocks and register > regions. I just cleaned up this exact mess with VExpress drivers... It's just a constant issue to deal with. > > You could solve this without DT changes. You can bind on node names. > > The driver probe can then check the parent compatible and return if > > not matching. I'm not sure if you could get module auto loading to > > work in that case. It would have to be based on the root compatible > > (rather than the driver match table) and be able to load multiple > > matching modules. > > That sounds like it would get very complicated for something this > simple. Having a compatible string in /cpus seemed like the most logical > option because it would basically just work out of the box and the same > way we're used to from other devices. That's also why I get the node per driver... That said, I'm fine with adding the compatible. I hope I don't regret it. Rob
diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c index 6741fcd..9c3d7f1 100644 --- a/drivers/firmware/tegra/bpmp.c +++ b/drivers/firmware/tegra/bpmp.c @@ -38,6 +38,44 @@ channel_to_ops(struct tegra_bpmp_channel *channel) return bpmp->soc->ops; } +struct tegra_bpmp *of_tegra_bpmp_get(void) +{ + struct platform_device *pdev; + struct device_node *bpmp_dev; + struct tegra_bpmp *bpmp; + + /* Check for bpmp device status in DT */ + bpmp_dev = of_find_compatible_node(NULL, NULL, "nvidia,tegra186-bpmp"); + if (!bpmp_dev) { + bpmp = ERR_PTR(-ENODEV); + goto err_out; + } + if (!of_device_is_available(bpmp_dev)) { + bpmp = ERR_PTR(-ENODEV); + goto err_put; + } + + pdev = of_find_device_by_node(bpmp_dev); + if (!pdev) { + bpmp = ERR_PTR(-ENODEV); + goto err_put; + } + + bpmp = platform_get_drvdata(pdev); + if (!bpmp) { + bpmp = ERR_PTR(-EPROBE_DEFER); + put_device(&pdev->dev); + goto err_put; + } + + return bpmp; +err_put: + of_node_put(bpmp_dev); +err_out: + return bpmp; +} +EXPORT_SYMBOL_GPL(of_tegra_bpmp_get); + struct tegra_bpmp *tegra_bpmp_get(struct device *dev) { struct platform_device *pdev; diff --git a/include/soc/tegra/bpmp.h b/include/soc/tegra/bpmp.h index f2604e9..21402d9 100644 --- a/include/soc/tegra/bpmp.h +++ b/include/soc/tegra/bpmp.h @@ -107,6 +107,7 @@ struct tegra_bpmp_message { }; #if IS_ENABLED(CONFIG_TEGRA_BPMP) +struct tegra_bpmp *of_tegra_bpmp_get(void); struct tegra_bpmp *tegra_bpmp_get(struct device *dev); void tegra_bpmp_put(struct tegra_bpmp *bpmp); int tegra_bpmp_transfer_atomic(struct tegra_bpmp *bpmp, @@ -122,6 +123,10 @@ void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, void *data); bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq); #else +static inline struct tegra_bpmp *of_tegra_bpmp_get(void) +{ + return ERR_PTR(-ENOTSUPP); +} static inline struct tegra_bpmp *tegra_bpmp_get(struct device *dev) { return ERR_PTR(-ENOTSUPP);
Adding new function of_tegra_bpmp_get() to get BPMP data. This function can be used by other drivers like cpufreq to get BPMP data without adding any property in respective drivers DT node. Signed-off-by: Sumit Gupta <sumitg@nvidia.com> --- drivers/firmware/tegra/bpmp.c | 38 ++++++++++++++++++++++++++++++++++++++ include/soc/tegra/bpmp.h | 5 +++++ 2 files changed, 43 insertions(+)