[RFC PATCH v3] watchdog: sp805: Add clock-frequency property

[RFC PATCH v3] watchdog: sp805: Add clock-frequency property

[Srinath Mannam]

When using ACPI node, binding clock devices are
not available as device tree, So clock-frequency
property given in _DSD object of ACPI device is
used to calculate Watchdog rate.

Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
---
 drivers/watchdog/sp805_wdt.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index 9849db0..a896b1c 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -11,6 +11,7 @@
  * warranty of any kind, whether express or implied.
  */
 
+#include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/resource.h>
 #include <linux/amba/bus.h>
@@ -22,6 +23,7 @@
 #include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -65,6 +67,7 @@ struct sp805_wdt {
 	spinlock_t			lock;
 	void __iomem			*base;
 	struct clk			*clk;
+	u64				rate;
 	struct amba_device		*adev;
 	unsigned int			load_val;
 };
@@ -80,7 +83,7 @@ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
 	struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
 	u64 load, rate;
 
-	rate = clk_get_rate(wdt->clk);
+	rate = wdt->rate;
 
 	/*
 	 * sp805 runs counter with given value twice, after the end of first
@@ -106,9 +109,7 @@ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
 static unsigned int wdt_timeleft(struct watchdog_device *wdd)
 {
 	struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
-	u64 load, rate;
-
-	rate = clk_get_rate(wdt->clk);
+	u64 load;
 
 	spin_lock(&wdt->lock);
 	load = readl_relaxed(wdt->base + WDTVALUE);
@@ -118,7 +119,7 @@ static unsigned int wdt_timeleft(struct watchdog_device *wdd)
 		load += wdt->load_val + 1;
 	spin_unlock(&wdt->lock);
 
-	return div_u64(load, rate);
+	return div_u64(load, wdt->rate);
 }
 
 static int
@@ -228,11 +229,25 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
 	if (IS_ERR(wdt->base))
 		return PTR_ERR(wdt->base);
 
-	wdt->clk = devm_clk_get(&adev->dev, NULL);
-	if (IS_ERR(wdt->clk)) {
-		dev_warn(&adev->dev, "Clock not found\n");
-		ret = PTR_ERR(wdt->clk);
-		goto err;
+	if (adev->dev.of_node) {
+		wdt->clk = devm_clk_get(&adev->dev, NULL);
+		if (IS_ERR(wdt->clk)) {
+			dev_err(&adev->dev, "Clock not found\n");
+			return PTR_ERR(wdt->clk);
+		}
+		wdt->rate = clk_get_rate(wdt->clk);
+	} else if (has_acpi_companion(&adev->dev)) {
+		/*
+		 * When Driver probe with ACPI device, clock devices
+		 * are not available, so watchdog rate get from
+		 * clock-frequency property given in _DSD object.
+		 */
+		device_property_read_u64(&adev->dev, "clock-frequency",
+					 &wdt->rate);
+		if (!wdt->rate) {
+			dev_err(&adev->dev, "no clock-frequency property\n");
+			return -ENODEV;
+		}
 	}
 
 	wdt->adev = adev;
-- 
2.7.4

Re: [RFC PATCH v3] watchdog: sp805: Add clock-frequency property

[Guenter Roeck]

On Tue, Jul 10, 2018 at 02:14:39PM +0530, Srinath Mannam wrote:
> When using ACPI node, binding clock devices are
> not available as device tree, So clock-frequency
> property given in _DSD object of ACPI device is
> used to calculate Watchdog rate.
> 
> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>

I am ok with the patch itself. All that is missing now is a
reference to the _DSD property documentation. Is that published
somewhere or is it all wild-wild-west ?

Thanks,
Guenter

> ---
>  drivers/watchdog/sp805_wdt.c | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> index 9849db0..a896b1c 100644
> --- a/drivers/watchdog/sp805_wdt.c
> +++ b/drivers/watchdog/sp805_wdt.c
> @@ -11,6 +11,7 @@
>   * warranty of any kind, whether express or implied.
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/device.h>
>  #include <linux/resource.h>
>  #include <linux/amba/bus.h>
> @@ -22,6 +23,7 @@
>  #include <linux/math64.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/of.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> @@ -65,6 +67,7 @@ struct sp805_wdt {
>  	spinlock_t			lock;
>  	void __iomem			*base;
>  	struct clk			*clk;
> +	u64				rate;
>  	struct amba_device		*adev;
>  	unsigned int			load_val;
>  };
> @@ -80,7 +83,7 @@ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
>  	struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>  	u64 load, rate;
>  
> -	rate = clk_get_rate(wdt->clk);
> +	rate = wdt->rate;
>  
>  	/*
>  	 * sp805 runs counter with given value twice, after the end of first
> @@ -106,9 +109,7 @@ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
>  static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>  {
>  	struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
> -	u64 load, rate;
> -
> -	rate = clk_get_rate(wdt->clk);
> +	u64 load;
>  
>  	spin_lock(&wdt->lock);
>  	load = readl_relaxed(wdt->base + WDTVALUE);
> @@ -118,7 +119,7 @@ static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>  		load += wdt->load_val + 1;
>  	spin_unlock(&wdt->lock);
>  
> -	return div_u64(load, rate);
> +	return div_u64(load, wdt->rate);
>  }
>  
>  static int
> @@ -228,11 +229,25 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
>  	if (IS_ERR(wdt->base))
>  		return PTR_ERR(wdt->base);
>  
> -	wdt->clk = devm_clk_get(&adev->dev, NULL);
> -	if (IS_ERR(wdt->clk)) {
> -		dev_warn(&adev->dev, "Clock not found\n");
> -		ret = PTR_ERR(wdt->clk);
> -		goto err;
> +	if (adev->dev.of_node) {
> +		wdt->clk = devm_clk_get(&adev->dev, NULL);
> +		if (IS_ERR(wdt->clk)) {
> +			dev_err(&adev->dev, "Clock not found\n");
> +			return PTR_ERR(wdt->clk);
> +		}
> +		wdt->rate = clk_get_rate(wdt->clk);
> +	} else if (has_acpi_companion(&adev->dev)) {
> +		/*
> +		 * When Driver probe with ACPI device, clock devices
> +		 * are not available, so watchdog rate get from
> +		 * clock-frequency property given in _DSD object.
> +		 */
> +		device_property_read_u64(&adev->dev, "clock-frequency",
> +					 &wdt->rate);
> +		if (!wdt->rate) {
> +			dev_err(&adev->dev, "no clock-frequency property\n");
> +			return -ENODEV;
> +		}
>  	}
>  
>  	wdt->adev = adev;
> -- 
> 2.7.4
> 

Re: [RFC PATCH v3] watchdog: sp805: Add clock-frequency property

[Srinath Mannam]

Hi Guenter,

Thank you very much for all the help with your feedback and review
comments to complete the changes very fast.

About the documentation..
I have gone through few similar patches available in the kernel are
listed in the mail of previous version.
No documentation available in Linux for the properties used in those
patches also.
For example,
1: "src-clock-hz" property added in the part of ACPI support with the
below patch.
Patch details: commit 515da746983bc6382e380ba8b1ce9345a9550ffe
Author: Naveen Kaje <nkaje@codeaurora.org>
Date:   Tue Oct 11 10:27:56 2016 -0600

    i2c: qup: add ACPI support
2: "amd,dma-freq" property added in the part of ACPI support with the
below patch
commit 82a19035d000c8b4fd7d6f61b614f63dec75d389
Author: Lendacky, Thomas <Thomas.Lendacky@amd.com>
Date:   Fri Jan 16 12:47:16 2015 -0600

    amd-xgbe: Add ACPI support

Regards,
Srinath.




On Wed, Jul 11, 2018 at 3:05 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Tue, Jul 10, 2018 at 02:14:39PM +0530, Srinath Mannam wrote:
>> When using ACPI node, binding clock devices are
>> not available as device tree, So clock-frequency
>> property given in _DSD object of ACPI device is
>> used to calculate Watchdog rate.
>>
>> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
>
> I am ok with the patch itself. All that is missing now is a
> reference to the _DSD property documentation. Is that published
> somewhere or is it all wild-wild-west ?
>
> Thanks,
> Guenter
>
>> ---
>>  drivers/watchdog/sp805_wdt.c | 35 +++++++++++++++++++++++++----------
>>  1 file changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>> index 9849db0..a896b1c 100644
>> --- a/drivers/watchdog/sp805_wdt.c
>> +++ b/drivers/watchdog/sp805_wdt.c
>> @@ -11,6 +11,7 @@
>>   * warranty of any kind, whether express or implied.
>>   */
>>
>> +#include <linux/acpi.h>
>>  #include <linux/device.h>
>>  #include <linux/resource.h>
>>  #include <linux/amba/bus.h>
>> @@ -22,6 +23,7 @@
>>  #include <linux/math64.h>
>>  #include <linux/module.h>
>>  #include <linux/moduleparam.h>
>> +#include <linux/of.h>
>>  #include <linux/pm.h>
>>  #include <linux/slab.h>
>>  #include <linux/spinlock.h>
>> @@ -65,6 +67,7 @@ struct sp805_wdt {
>>       spinlock_t                      lock;
>>       void __iomem                    *base;
>>       struct clk                      *clk;
>> +     u64                             rate;
>>       struct amba_device              *adev;
>>       unsigned int                    load_val;
>>  };
>> @@ -80,7 +83,7 @@ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
>>       struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>>       u64 load, rate;
>>
>> -     rate = clk_get_rate(wdt->clk);
>> +     rate = wdt->rate;
>>
>>       /*
>>        * sp805 runs counter with given value twice, after the end of first
>> @@ -106,9 +109,7 @@ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
>>  static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>>  {
>>       struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>> -     u64 load, rate;
>> -
>> -     rate = clk_get_rate(wdt->clk);
>> +     u64 load;
>>
>>       spin_lock(&wdt->lock);
>>       load = readl_relaxed(wdt->base + WDTVALUE);
>> @@ -118,7 +119,7 @@ static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>>               load += wdt->load_val + 1;
>>       spin_unlock(&wdt->lock);
>>
>> -     return div_u64(load, rate);
>> +     return div_u64(load, wdt->rate);
>>  }
>>
>>  static int
>> @@ -228,11 +229,25 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
>>       if (IS_ERR(wdt->base))
>>               return PTR_ERR(wdt->base);
>>
>> -     wdt->clk = devm_clk_get(&adev->dev, NULL);
>> -     if (IS_ERR(wdt->clk)) {
>> -             dev_warn(&adev->dev, "Clock not found\n");
>> -             ret = PTR_ERR(wdt->clk);
>> -             goto err;
>> +     if (adev->dev.of_node) {
>> +             wdt->clk = devm_clk_get(&adev->dev, NULL);
>> +             if (IS_ERR(wdt->clk)) {
>> +                     dev_err(&adev->dev, "Clock not found\n");
>> +                     return PTR_ERR(wdt->clk);
>> +             }
>> +             wdt->rate = clk_get_rate(wdt->clk);
>> +     } else if (has_acpi_companion(&adev->dev)) {
>> +             /*
>> +              * When Driver probe with ACPI device, clock devices
>> +              * are not available, so watchdog rate get from
>> +              * clock-frequency property given in _DSD object.
>> +              */
>> +             device_property_read_u64(&adev->dev, "clock-frequency",
>> +                                      &wdt->rate);
>> +             if (!wdt->rate) {
>> +                     dev_err(&adev->dev, "no clock-frequency property\n");
>> +                     return -ENODEV;
>> +             }
>>       }
>>
>>       wdt->adev = adev;
>> --
>> 2.7.4
>>

Re: [RFC PATCH v3] watchdog: sp805: Add clock-frequency property

[Guenter Roeck]

On 07/11/2018 06:22 AM, Srinath Mannam wrote:
> Hi Guenter,
> 
> Thank you very much for all the help with your feedback and review
> comments to complete the changes very fast.
> 
> About the documentation..
> I have gone through few similar patches available in the kernel are
> listed in the mail of previous version.
> No documentation available in Linux for the properties used in those
> patches also.

" No documentation available _in Linux_"

Emphasis mine. Yes, I noticed this as well. I was asking for a reference
to documentation _outside_ Linux. Sorry for not being more specific.

Guenter

> For example,
> 1: "src-clock-hz" property added in the part of ACPI support with the
> below patch.
> Patch details: commit 515da746983bc6382e380ba8b1ce9345a9550ffe
> Author: Naveen Kaje <nkaje@codeaurora.org>
> Date:   Tue Oct 11 10:27:56 2016 -0600
> 
>      i2c: qup: add ACPI support
> 2: "amd,dma-freq" property added in the part of ACPI support with the
> below patch
> commit 82a19035d000c8b4fd7d6f61b614f63dec75d389
> Author: Lendacky, Thomas <Thomas.Lendacky@amd.com>
> Date:   Fri Jan 16 12:47:16 2015 -0600
> 
>      amd-xgbe: Add ACPI support
> 
> Regards,
> Srinath.
> 
> 
> 
> 
> On Wed, Jul 11, 2018 at 3:05 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Tue, Jul 10, 2018 at 02:14:39PM +0530, Srinath Mannam wrote:
>>> When using ACPI node, binding clock devices are
>>> not available as device tree, So clock-frequency
>>> property given in _DSD object of ACPI device is
>>> used to calculate Watchdog rate.
>>>
>>> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
>>
>> I am ok with the patch itself. All that is missing now is a
>> reference to the _DSD property documentation. Is that published
>> somewhere or is it all wild-wild-west ?
>>
>> Thanks,
>> Guenter
>>
>>> ---
>>>   drivers/watchdog/sp805_wdt.c | 35 +++++++++++++++++++++++++----------
>>>   1 file changed, 25 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>>> index 9849db0..a896b1c 100644
>>> --- a/drivers/watchdog/sp805_wdt.c
>>> +++ b/drivers/watchdog/sp805_wdt.c
>>> @@ -11,6 +11,7 @@
>>>    * warranty of any kind, whether express or implied.
>>>    */
>>>
>>> +#include <linux/acpi.h>
>>>   #include <linux/device.h>
>>>   #include <linux/resource.h>
>>>   #include <linux/amba/bus.h>
>>> @@ -22,6 +23,7 @@
>>>   #include <linux/math64.h>
>>>   #include <linux/module.h>
>>>   #include <linux/moduleparam.h>
>>> +#include <linux/of.h>
>>>   #include <linux/pm.h>
>>>   #include <linux/slab.h>
>>>   #include <linux/spinlock.h>
>>> @@ -65,6 +67,7 @@ struct sp805_wdt {
>>>        spinlock_t                      lock;
>>>        void __iomem                    *base;
>>>        struct clk                      *clk;
>>> +     u64                             rate;
>>>        struct amba_device              *adev;
>>>        unsigned int                    load_val;
>>>   };
>>> @@ -80,7 +83,7 @@ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
>>>        struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>>>        u64 load, rate;
>>>
>>> -     rate = clk_get_rate(wdt->clk);
>>> +     rate = wdt->rate;
>>>
>>>        /*
>>>         * sp805 runs counter with given value twice, after the end of first
>>> @@ -106,9 +109,7 @@ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
>>>   static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>>>   {
>>>        struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>>> -     u64 load, rate;
>>> -
>>> -     rate = clk_get_rate(wdt->clk);
>>> +     u64 load;
>>>
>>>        spin_lock(&wdt->lock);
>>>        load = readl_relaxed(wdt->base + WDTVALUE);
>>> @@ -118,7 +119,7 @@ static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>>>                load += wdt->load_val + 1;
>>>        spin_unlock(&wdt->lock);
>>>
>>> -     return div_u64(load, rate);
>>> +     return div_u64(load, wdt->rate);
>>>   }
>>>
>>>   static int
>>> @@ -228,11 +229,25 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
>>>        if (IS_ERR(wdt->base))
>>>                return PTR_ERR(wdt->base);
>>>
>>> -     wdt->clk = devm_clk_get(&adev->dev, NULL);
>>> -     if (IS_ERR(wdt->clk)) {
>>> -             dev_warn(&adev->dev, "Clock not found\n");
>>> -             ret = PTR_ERR(wdt->clk);
>>> -             goto err;
>>> +     if (adev->dev.of_node) {
>>> +             wdt->clk = devm_clk_get(&adev->dev, NULL);
>>> +             if (IS_ERR(wdt->clk)) {
>>> +                     dev_err(&adev->dev, "Clock not found\n");
>>> +                     return PTR_ERR(wdt->clk);
>>> +             }
>>> +             wdt->rate = clk_get_rate(wdt->clk);
>>> +     } else if (has_acpi_companion(&adev->dev)) {
>>> +             /*
>>> +              * When Driver probe with ACPI device, clock devices
>>> +              * are not available, so watchdog rate get from
>>> +              * clock-frequency property given in _DSD object.
>>> +              */
>>> +             device_property_read_u64(&adev->dev, "clock-frequency",
>>> +                                      &wdt->rate);
>>> +             if (!wdt->rate) {
>>> +                     dev_err(&adev->dev, "no clock-frequency property\n");
>>> +                     return -ENODEV;
>>> +             }
>>>        }
>>>
>>>        wdt->adev = adev;
>>> --
>>> 2.7.4
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC PATCH v3] watchdog: sp805: Add clock-frequency property

[Sudeep Holla]

On Wed, Jul 11, 2018 at 06:47:49AM -0700, Guenter Roeck wrote:
> On 07/11/2018 06:22 AM, Srinath Mannam wrote:
> >Hi Guenter,
> >
> >Thank you very much for all the help with your feedback and review
> >comments to complete the changes very fast.
> >
> >About the documentation..
> >I have gone through few similar patches available in the kernel are
> >listed in the mail of previous version.
> >No documentation available in Linux for the properties used in those
> >patches also.
>
> " No documentation available _in Linux_"
>
> Emphasis mine. Yes, I noticed this as well. I was asking for a reference
> to documentation _outside_ Linux. Sorry for not being more specific.

Typically new properties needs to registered or discussed in dsd@acpica.org
Though there's almost no activity on that list for more than a year now.
IIRC, the thread[1] gives kind of agreement that was reached after
elaborate discussion on _DSD properties.

--
Regards,
Sudeep

[1] https://lists.acpica.org/pipermail/dsd/2015-December/000027.html

Re: [RFC PATCH v3] watchdog: sp805: Add clock-frequency property

[Guenter Roeck]

On Wed, Jul 11, 2018 at 04:30:16PM +0100, Sudeep Holla wrote:
> On Wed, Jul 11, 2018 at 06:47:49AM -0700, Guenter Roeck wrote:
> > On 07/11/2018 06:22 AM, Srinath Mannam wrote:
> > >Hi Guenter,
> > >
> > >Thank you very much for all the help with your feedback and review
> > >comments to complete the changes very fast.
> > >
> > >About the documentation..
> > >I have gone through few similar patches available in the kernel are
> > >listed in the mail of previous version.
> > >No documentation available in Linux for the properties used in those
> > >patches also.
> >
> > " No documentation available _in Linux_"
> >
> > Emphasis mine. Yes, I noticed this as well. I was asking for a reference
> > to documentation _outside_ Linux. Sorry for not being more specific.
> 
> Typically new properties needs to registered or discussed in dsd@acpica.org
> Though there's almost no activity on that list for more than a year now.
> IIRC, the thread[1] gives kind of agreement that was reached after
> elaborate discussion on _DSD properties.
> 

I think you are saying that there are no real rules or governing body
for _DSD properties, that _DSD properties are free for all, subject to no
scrutiny, that a database with assigned _DSD properties does not exist,
and that therefore there is no means for me to determine if this is an
approved property.

What prevents someone else to use a different property name for the same
driver and property next week, on a different product using the same
hardware ?

Guenter

Re: [RFC PATCH v3] watchdog: sp805: Add clock-frequency property

[Sudeep Holla]

On Wed, Jul 11, 2018 at 08:39:50AM -0700, Guenter Roeck wrote:
> On Wed, Jul 11, 2018 at 04:30:16PM +0100, Sudeep Holla wrote:

[...]

> >
> > Typically new properties needs to registered or discussed in dsd@acpica.org
> > Though there's almost no activity on that list for more than a year now.
> > IIRC, the thread[1] gives kind of agreement that was reached after
> > elaborate discussion on _DSD properties.
> >
>
> I think you are saying that there are no real rules or governing body
> for _DSD properties, that _DSD properties are free for all, subject to no
> scrutiny, that a database with assigned _DSD properties does not exist,
> and that therefore there is no means for me to determine if this is an
> approved property.
>

Yes and no. The only intent of the review on dsd@acpica.org to catch
functional/non-compliance issues with the property. The vendor needs to
own it and ensure the support is added in the kernel before shipping it.

> What prevents someone else to use a different property name for the same
> driver and property next week, on a different product using the same
> hardware ?
>

Honestly nothing. But the agreement was vendor needs to proactively get
it reviewed and add the support. The community can reject if it has
functional/compliance issues.

There has been elaborate discussions in the past on this and I provided
the link to the final agreement on that. It's always better to avoid
using them as first option if possible, else get the review/agreement
that it's good to use property.

--
Regards,
Sudeep

Re: [RFC PATCH v3] watchdog: sp805: Add clock-frequency property

[Guenter Roeck]

On 07/10/2018 01:44 AM, Srinath Mannam wrote:
> When using ACPI node, binding clock devices are
> not available as device tree, So clock-frequency
> property given in _DSD object of ACPI device is
> used to calculate Watchdog rate.
> 
> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>

Please resend as non-RFC and add a note in the description indicating
that there is no formal review process for _DSD properties.

Thanks,
Guenter

> ---
>   drivers/watchdog/sp805_wdt.c | 35 +++++++++++++++++++++++++----------
>   1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> index 9849db0..a896b1c 100644
> --- a/drivers/watchdog/sp805_wdt.c
> +++ b/drivers/watchdog/sp805_wdt.c
> @@ -11,6 +11,7 @@
>    * warranty of any kind, whether express or implied.
>    */
>   
> +#include <linux/acpi.h>
>   #include <linux/device.h>
>   #include <linux/resource.h>
>   #include <linux/amba/bus.h>
> @@ -22,6 +23,7 @@
>   #include <linux/math64.h>
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
> +#include <linux/of.h>
>   #include <linux/pm.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
> @@ -65,6 +67,7 @@ struct sp805_wdt {
>   	spinlock_t			lock;
>   	void __iomem			*base;
>   	struct clk			*clk;
> +	u64				rate;
>   	struct amba_device		*adev;
>   	unsigned int			load_val;
>   };
> @@ -80,7 +83,7 @@ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
>   	struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>   	u64 load, rate;
>   
> -	rate = clk_get_rate(wdt->clk);
> +	rate = wdt->rate;
>   
>   	/*
>   	 * sp805 runs counter with given value twice, after the end of first
> @@ -106,9 +109,7 @@ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
>   static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>   {
>   	struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
> -	u64 load, rate;
> -
> -	rate = clk_get_rate(wdt->clk);
> +	u64 load;
>   
>   	spin_lock(&wdt->lock);
>   	load = readl_relaxed(wdt->base + WDTVALUE);
> @@ -118,7 +119,7 @@ static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>   		load += wdt->load_val + 1;
>   	spin_unlock(&wdt->lock);
>   
> -	return div_u64(load, rate);
> +	return div_u64(load, wdt->rate);
>   }
>   
>   static int
> @@ -228,11 +229,25 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
>   	if (IS_ERR(wdt->base))
>   		return PTR_ERR(wdt->base);
>   
> -	wdt->clk = devm_clk_get(&adev->dev, NULL);
> -	if (IS_ERR(wdt->clk)) {
> -		dev_warn(&adev->dev, "Clock not found\n");
> -		ret = PTR_ERR(wdt->clk);
> -		goto err;
> +	if (adev->dev.of_node) {
> +		wdt->clk = devm_clk_get(&adev->dev, NULL);
> +		if (IS_ERR(wdt->clk)) {
> +			dev_err(&adev->dev, "Clock not found\n");
> +			return PTR_ERR(wdt->clk);
> +		}
> +		wdt->rate = clk_get_rate(wdt->clk);
> +	} else if (has_acpi_companion(&adev->dev)) {
> +		/*
> +		 * When Driver probe with ACPI device, clock devices
> +		 * are not available, so watchdog rate get from
> +		 * clock-frequency property given in _DSD object.
> +		 */
> +		device_property_read_u64(&adev->dev, "clock-frequency",
> +					 &wdt->rate);
> +		if (!wdt->rate) {
> +			dev_err(&adev->dev, "no clock-frequency property\n");
> +			return -ENODEV;
> +		}
>   	}
>   
>   	wdt->adev = adev;
> 

Re: [RFC PATCH v3] watchdog: sp805: Add clock-frequency property

[Srinath Mannam]

Hi Guenter,

I will send the updated patch..
Thank you.

Regards,
Srinath.

On Sat, Jul 21, 2018 at 8:08 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/10/2018 01:44 AM, Srinath Mannam wrote:
>>
>> When using ACPI node, binding clock devices are
>> not available as device tree, So clock-frequency
>> property given in _DSD object of ACPI device is
>> used to calculate Watchdog rate.
>>
>> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
>
>
> Please resend as non-RFC and add a note in the description indicating
> that there is no formal review process for _DSD properties.
>
> Thanks,
> Guenter
>
>> ---
>>   drivers/watchdog/sp805_wdt.c | 35 +++++++++++++++++++++++++----------
>>   1 file changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>> index 9849db0..a896b1c 100644
>> --- a/drivers/watchdog/sp805_wdt.c
>> +++ b/drivers/watchdog/sp805_wdt.c
>> @@ -11,6 +11,7 @@
>>    * warranty of any kind, whether express or implied.
>>    */
>>   +#include <linux/acpi.h>
>>   #include <linux/device.h>
>>   #include <linux/resource.h>
>>   #include <linux/amba/bus.h>
>> @@ -22,6 +23,7 @@
>>   #include <linux/math64.h>
>>   #include <linux/module.h>
>>   #include <linux/moduleparam.h>
>> +#include <linux/of.h>
>>   #include <linux/pm.h>
>>   #include <linux/slab.h>
>>   #include <linux/spinlock.h>
>> @@ -65,6 +67,7 @@ struct sp805_wdt {
>>         spinlock_t                      lock;
>>         void __iomem                    *base;
>>         struct clk                      *clk;
>> +       u64                             rate;
>>         struct amba_device              *adev;
>>         unsigned int                    load_val;
>>   };
>> @@ -80,7 +83,7 @@ static int wdt_setload(struct watchdog_device *wdd,
>> unsigned int timeout)
>>         struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>>         u64 load, rate;
>>   -     rate = clk_get_rate(wdt->clk);
>> +       rate = wdt->rate;
>>         /*
>>          * sp805 runs counter with given value twice, after the end of
>> first
>> @@ -106,9 +109,7 @@ static int wdt_setload(struct watchdog_device *wdd,
>> unsigned int timeout)
>>   static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>>   {
>>         struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>> -       u64 load, rate;
>> -
>> -       rate = clk_get_rate(wdt->clk);
>> +       u64 load;
>>         spin_lock(&wdt->lock);
>>         load = readl_relaxed(wdt->base + WDTVALUE);
>> @@ -118,7 +119,7 @@ static unsigned int wdt_timeleft(struct
>> watchdog_device *wdd)
>>                 load += wdt->load_val + 1;
>>         spin_unlock(&wdt->lock);
>>   -     return div_u64(load, rate);
>> +       return div_u64(load, wdt->rate);
>>   }
>>     static int
>> @@ -228,11 +229,25 @@ sp805_wdt_probe(struct amba_device *adev, const
>> struct amba_id *id)
>>         if (IS_ERR(wdt->base))
>>                 return PTR_ERR(wdt->base);
>>   -     wdt->clk = devm_clk_get(&adev->dev, NULL);
>> -       if (IS_ERR(wdt->clk)) {
>> -               dev_warn(&adev->dev, "Clock not found\n");
>> -               ret = PTR_ERR(wdt->clk);
>> -               goto err;
>> +       if (adev->dev.of_node) {
>> +               wdt->clk = devm_clk_get(&adev->dev, NULL);
>> +               if (IS_ERR(wdt->clk)) {
>> +                       dev_err(&adev->dev, "Clock not found\n");
>> +                       return PTR_ERR(wdt->clk);
>> +               }
>> +               wdt->rate = clk_get_rate(wdt->clk);
>> +       } else if (has_acpi_companion(&adev->dev)) {
>> +               /*
>> +                * When Driver probe with ACPI device, clock devices
>> +                * are not available, so watchdog rate get from
>> +                * clock-frequency property given in _DSD object.
>> +                */
>> +               device_property_read_u64(&adev->dev, "clock-frequency",
>> +                                        &wdt->rate);
>> +               if (!wdt->rate) {
>> +                       dev_err(&adev->dev, "no clock-frequency
>> property\n");
>> +                       return -ENODEV;
>> +               }
>>         }
>>         wdt->adev = adev;
>>
>