[TEGRA194_CPUFREQ,1/3] firmware: tegra: adding function to get BPMP data
diff mbox series

Message ID 1575394348-17649-1-git-send-email-sumitg@nvidia.com
State Changes Requested
Headers show
Series
  • [TEGRA194_CPUFREQ,1/3] firmware: tegra: adding function to get BPMP data
Related show

Commit Message

sumitg Dec. 3, 2019, 5:32 p.m. UTC
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(+)

Comments

Thierry Reding Dec. 3, 2019, 5:42 p.m. UTC | #1
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
>
Mikko Perttunen Dec. 4, 2019, 8:45 a.m. UTC | #2
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
>>
Viresh Kumar Dec. 4, 2019, 9:17 a.m. UTC | #3
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 ?
Thierry Reding Dec. 4, 2019, 9:33 a.m. UTC | #4
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
Viresh Kumar Dec. 4, 2019, 9:51 a.m. UTC | #5
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/
Mikko Perttunen Dec. 4, 2019, 10:21 a.m. UTC | #6
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 ?
>
Viresh Kumar Dec. 4, 2019, 10:26 a.m. UTC | #7
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.

Patch
diff mbox series

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);