Hi Guenter,
Thank you for the clarification.. Please find my comments.
On Sun, Jul 8, 2018 at 11:36 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On 07/06/2018 01:18 AM, Srinath Mannam wrote:
>>
>> Hi Guenter,
>>
>> Thank you very much for your feedback. Please find my comments.
>>
>> On Thu, Jul 5, 2018 at 8:58 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>>>
>>> On 07/04/2018 08:22 PM, 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@xxxxxxxxxxxx>
>>>> ---
>>>> drivers/watchdog/sp805_wdt.c | 29 ++++++++++++++++++++++++-----
>>>> 1 file changed, 24 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>>>> index 9849db0..d830dbc 100644
>>>> --- a/drivers/watchdog/sp805_wdt.c
>>>> +++ b/drivers/watchdog/sp805_wdt.c
>>>> @@ -22,6 +22,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 +66,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 +82,10 @@ 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);
>>>> + if (wdt->rate)
>>>> + rate = wdt->rate;
>>>> + else
>>>> + rate = clk_get_rate(wdt->clk);
>>>
>>>
>>>
>>> No. Get the rate once during probe and store it in wdt->rate.
>>
>> clk_get_rate function was called multiple places in the driver.
>> so that we thought, there may be cases where clock rate can change at
>> run-time.
>> That is the reason, I keep clk_get_rate function.
>
>
> This is not an argument. A changing clock rate without notifying the driver
> would be fatal for a watchdog driver, since it would affect the timeout and
> - if the clock rate is increased - result in arbitrary reboots. If that can
> happen with the clock used by this watchdog, some notification would have to
> be implemented to let the driver know.
>
As you said, I will keep wdt->rate and remove the clk_get_rate call. Thank you.
>>>
>>>> /*
>>>> * sp805 runs counter with given value twice, after the end of
>>>> first
>>>> @@ -108,7 +113,10 @@ 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);
>>>> + if (wdt->rate)
>>>> + rate = wdt->rate;
>>>> + else
>>>> + rate = clk_get_rate(wdt->clk);
>>>
>>>
>>>
>>> Same here.
>>>
>>>> spin_lock(&wdt->lock);
>>>> load = readl_relaxed(wdt->base + WDTVALUE);
>>>> @@ -230,11 +238,22 @@ sp805_wdt_probe(struct amba_device *adev, const
>>>> struct amba_id *id)
>>>> 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;
>>>> + dev_warn(&adev->dev, "Clock device not found\n");
>>>
>>>
>>>
>>> "Clock _device_" ? Why this change ? And why retain that warning
>>> unconditionally ?
>>
>> I mean, peripheral may have clock input but clock device node is not
>> available to this driver.
>> So I keep the warning.
>
>
> There is now a warning even though everything is fine, if the clock rate
> is provided with the "clock-frequency" property instead of the documented
> properties. That is unacceptable. I don't want to get flooded with messages
> from people asking what this message is about.
>
>> I thought to handle this better, divide this part of code into two
>> parts based on device tree and ACPI.
>> But this driver implementation is independent of device tree and ACPI
>> calls.
>> To get device-tree properties watch-dog framework APIs are called ex:
>> timeout.
>> Can I add ACPI and device tree node availability check to get clock
>> device and clock-frequency properties. Please advice.
>
>
> Sorry, I fail to parse what you are trying to say.
>
This wdt driver is AMBA framework based driver.
AMBA framework itself expects apb_pclk clock device node from device tree
to enable pclk. So we must add apb_pclk property in device tree node.
For example, In our platform which is based on device tree,
we pass clock nodes to sp805 driver as
clocks = <&hsls_25m_div2_clk>, <&hsls_div4_clk>;
clock-names = "wdogclk", "apb_pclk";
hsls_div4_clk is abp_pclk, and hsls_25m_div2_clk is wdogclk which is
the first node.
AMBA driver get the clock node using clock_get API with "apb_pclk" string.
In wdt driver clk_get api called with NULL string so first clock node
is taken as its clock node.
few other vendor platforms take same clock for both apb_pclk and wdt clocks.
In that case only one clock node in dt node and clock-name must be
given as apb_pclk.
then the same clock node taken to wdt clock also because wdt driver
call clock_get API with NULL.
So we can't use clock-frequency as alternative to clock nodes in
device tree system.
If we want to use.
we need to pass apb_pclk node and clock-frequency properties in dt node.
In that case apb_pclk node is taken as wdt clock because clk_get API
called with NULL string.
To avoid this, we need to call clk_get function in wdt driver with
valid string instead of NULL
Then clk_get failed and go for clock-frequency property.
If valid string we used for wdt clock in the driver, then it will be
problem for few platforms
where only one clock node used for both apb_pclk and wdt clock.
To solve this we need to pass same clock node for both apb_pclk and wdt clock.
So it causes changes in their DTS files.
I have a different approach to use clock-frequency support in only ACPI systems.
I will push those changes in next patch set.
> Lets summarize: You are introducing a new means for this driver to obtain
> the clock rate used by the watchdog. This is quite independent of the
> instantiation method, since it works for both ACPI and non-ACPI systems.
> This change needs to be documented and approved by devicetree maintainers.
> Its implementation must then accept all valid methods to obtain the clock
> rate without warning or error message.
>
> Thanks,
> Guenter
>
>>>
>>>> + wdt->clk = NULL;
>>>> + /*
>>>> + * 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) {
>>>> + ret = -ENODEV;
>>>
>>>
>>>
>>> This unconditionally overrides the original error.
>>
>> I accept, I will change.
>>>
>>>
>>>> + goto err;
>>>> + }
>>>> }
>>>> +
>>>
>>>
>>>
>>> No whitespace changes, please.
>>
>> I will remove.
>>>
>>>
>>> Does that mean that ACPI doesn't support the clock subsystem and that
>>> each
>>> driver
>>> supporting ACPI must do this ? That would be messy. Also, the above does
>>> not
>>> check
>>> if the device was probed through ACPI. It just tries to find an
>>> undocumented
>>> "clock-frequency" property which is quite different and would apply to
>>> (probably mis-configured) devicetree files as well.
>>>
>>> Either case, I would rather have this addressed through the clock
>>> subsystem.
>>> Otherwise, someone with subject knowledge will have to confirm that this
>>> is
>>> the
>>> appropriate solution. If so, the new property will have to be documented
>>> as
>>> an
>>> alternative to clock specifiers and accepted by devicetree maintainers.
>>>
>> I checked with ACPI maintainers, ACPI does not support common-clock
>> framework and also no plan.
>> AMBA framework also request for "apb_pclk" clock node to enable pclk.
>> But ACPI does not do the same.
>> So in device-tree use case, both apb_pclk and wdt clock nodes are
>> required properties, so passing
>> clock-frequency alone can not be alternative.
>> Because ACPI does not support clk node method, I came up with
>> alternate fixed-clock property clock-frequency.
>> clock-frequency is only specific to ACPI case, so we can't add in
>> binding document.
>> I will add device tree and ACPI specific check to read clock nodes and
>> clock-frequency properties separately.
>>
>> If you are fine with this I will send the next patch with modifications.
>>
>>> Guenter
>>>
>>>> wdt->adev = adev;
>>>> wdt->wdd.info = &wdt_info;
>>>> wdt->wdd.ops = &wdt_ops;
>>>>
>>>
>> Thank you,
>>
>> Regards,
>> Srinath.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog"
>> in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
Regards,
Srinath.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
