can: grcan: Enhance driver with CANFD Support and Improvements

[PATCH 04/10] can: grcan: Add clock handling

Posted by Arun Muthusamy 1 week, 6 days ago

From: Daniel Hellstrom <daniel@gaisler.com>

Add clock handling and add error messages for missing 'freq' DT property.

Signed-off-by: Arun Muthusamy <arun.muthusamy@gaisler.com>
Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>
---
 drivers/net/can/grcan.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
index 3b1b09943436..538a9b4f82ab 100644
--- a/drivers/net/can/grcan.c
+++ b/drivers/net/can/grcan.c
@@ -34,7 +34,7 @@
 #include <linux/spinlock.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
-
+#include <linux/clk.h>
 #include <linux/dma-mapping.h>
 
 #define DRV_NAME	"grcan"
@@ -1644,6 +1644,7 @@ static int grcan_probe(struct platform_device *ofdev)
 {
 	struct device_node *np = ofdev->dev.of_node;
 	struct device_node *sysid_parent;
+	struct clk *clk;
 	u32 sysid, ambafreq;
 	int irq, err;
 	void __iomem *base;
@@ -1663,8 +1664,20 @@ static int grcan_probe(struct platform_device *ofdev)
 
 	err = of_property_read_u32(np, "freq", &ambafreq);
 	if (err) {
-		dev_err(&ofdev->dev, "unable to fetch \"freq\" property\n");
-		goto exit_error;
+		clk = devm_clk_get(&ofdev->dev, NULL);
+		if (IS_ERR(clk)) {
+			dev_err_probe(&ofdev->dev, PTR_ERR(clk),
+				      "Failed both to get \"freq\" property and clock for fallback\n");
+			err = PTR_ERR(clk);
+			goto exit_error;
+		}
+		if (clk) {
+			ambafreq = clk_get_rate(clk);
+			if (!ambafreq) {
+				dev_err(&ofdev->dev, "Invalid or Uninitialized clock\n");
+				return -EINVAL;
+			}
+		}
 	}
 
 	base = devm_platform_ioremap_resource(ofdev, 0);
-- 
2.51.0

Re: [PATCH 04/10] can: grcan: Add clock handling

Posted by Krzysztof Kozlowski 1 week, 6 days ago

On 18/11/2025 10:21, Arun Muthusamy wrote:
> From: Daniel Hellstrom <daniel@gaisler.com>
> 
> Add clock handling and add error messages for missing 'freq' DT property.
> 
> Signed-off-by: Arun Muthusamy <arun.muthusamy@gaisler.com>
> Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>
> ---
>  drivers/net/can/grcan.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 3b1b09943436..538a9b4f82ab 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
> @@ -34,7 +34,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
> -
> +#include <linux/clk.h>
>  #include <linux/dma-mapping.h>
>  
>  #define DRV_NAME	"grcan"
> @@ -1644,6 +1644,7 @@ static int grcan_probe(struct platform_device *ofdev)
>  {
>  	struct device_node *np = ofdev->dev.of_node;
>  	struct device_node *sysid_parent;
> +	struct clk *clk;
>  	u32 sysid, ambafreq;
>  	int irq, err;
>  	void __iomem *base;
> @@ -1663,8 +1664,20 @@ static int grcan_probe(struct platform_device *ofdev)
>  
>  	err = of_property_read_u32(np, "freq", &ambafreq);
>  	if (err) {
> -		dev_err(&ofdev->dev, "unable to fetch \"freq\" property\n");
> -		goto exit_error;
> +		clk = devm_clk_get(&ofdev->dev, NULL);

Nope, your binding said there is no clock... you cannot add undocumented
ABI.


Best regards,
Krzysztof

Re: [PATCH 04/10] can: grcan: Add clock handling

Posted by Arun Muthusamy 1 week ago

Hi Krzysztof,

Thank you for your thorough review. I wanted to get your guidance 
regarding the clock property in the DT binding.

In the binding, I included the clocks property with maxItems: 1 to 
indicate that a clock should be described. The driver calls:

clk = devm_clk_get(dev, NULL);

Since we pass NULL, the driver always requests the first (and only) 
clock from the clocks property.

I want to ensure the binding is fully compliant with the Linux DT ABI. 
Could you advise the preferred way to document the clocks and 
clock-names properties in this scenario? Specifically:

Do we still need a clock-names entry even if the driver never uses it by 
name?
For LEON systems, the driver relies on the "freq" property, while NOEL 
systems use a standard "clocks" binding. Given this dual approach, 
should the clocks property be marked as optional or required in the 
binding?

Thank you for your time and help.

-- 
BR,

Arun Muthusamy
Software Engineer
Frontgrade Gaisler
T : +46 (0) 700 558 528
arun.muthusamy@gaisler.com

Frontgrade Gaisler AB, Kungsgatan 12, SE-411 19 GÖTEBORG, Sweden.
+46 (0) 31 775 8650, www.gaisler.com

On 18.11.2025 12:01, Krzysztof Kozlowski wrote:
> On 18/11/2025 10:21, Arun Muthusamy wrote:
>> From: Daniel Hellstrom <daniel@gaisler.com>
>> 
>> Add clock handling and add error messages for missing 'freq' DT 
>> property.
>> 
>> Signed-off-by: Arun Muthusamy <arun.muthusamy@gaisler.com>
>> Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>
>> ---
>>  drivers/net/can/grcan.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
>> index 3b1b09943436..538a9b4f82ab 100644
>> --- a/drivers/net/can/grcan.c
>> +++ b/drivers/net/can/grcan.c
>> @@ -34,7 +34,7 @@
>>  #include <linux/spinlock.h>
>>  #include <linux/of.h>
>>  #include <linux/of_irq.h>
>> -
>> +#include <linux/clk.h>
>>  #include <linux/dma-mapping.h>
>> 
>>  #define DRV_NAME	"grcan"
>> @@ -1644,6 +1644,7 @@ static int grcan_probe(struct platform_device 
>> *ofdev)
>>  {
>>  	struct device_node *np = ofdev->dev.of_node;
>>  	struct device_node *sysid_parent;
>> +	struct clk *clk;
>>  	u32 sysid, ambafreq;
>>  	int irq, err;
>>  	void __iomem *base;
>> @@ -1663,8 +1664,20 @@ static int grcan_probe(struct platform_device 
>> *ofdev)
>> 
>>  	err = of_property_read_u32(np, "freq", &ambafreq);
>>  	if (err) {
>> -		dev_err(&ofdev->dev, "unable to fetch \"freq\" property\n");
>> -		goto exit_error;
>> +		clk = devm_clk_get(&ofdev->dev, NULL);
> 
> Nope, your binding said there is no clock... you cannot add 
> undocumented
> ABI.
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 04/10] can: grcan: Add clock handling

Posted by Krzysztof Kozlowski 1 week ago

On 24/11/2025 10:46, Arun Muthusamy wrote:
> Hi Krzysztof,
> 
> Thank you for your thorough review. I wanted to get your guidance 
> regarding the clock property in the DT binding.
> 
> In the binding, I included the clocks property with maxItems: 1 to 
> indicate that a clock should be described. The driver calls:

Ah, you added clocks, I completely missed it.

> 
> clk = devm_clk_get(dev, NULL);
> 
> Since we pass NULL, the driver always requests the first (and only) 
> clock from the clocks property

Yes this is correct.

.
> 
> I want to ensure the binding is fully compliant with the Linux DT ABI. 

It is fine, please ignore my comment. It was a mistake.


Best regards,
Krzysztof