On 09/20/2017 05:00 PM, Mario Hüttel wrote:
>> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
>> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
>> interface clock is managed by hwmod driver via pm_runtime_get and
>> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
>> and thus the driver shouldn't fail if this clock isn't found.
> I also agree in this point.
> However, there's a problem I want to point out:
>
> The M_CAN can only function correctly, if the condition
> 'hclk >= cclk' holds true.
>
> The internal clock domain crossing can fail if this condition
> is violated.
>
> I thought about adding the condition to the driver to ensure
> correct operation. But I had some problems:
>
> 1. Determine the clock rates:
> The devices you've mentioned above don't have an assigned
> hclk. Is there still a possibility to get the clock frequency?
I believe interface clocks via hwmod aren't exposed to drivers. So the
only way to get access to the clock frequency is to add this interface
clock to dt.
>
> 2. What to do if 'hclk < cclk'?
> Is there a general way to process such an error? - I think not.
> Is a simple warning in case of an error enough?
>
> Is there a way of ensuring that the condition is always met at all?
>
> I think it is quite unlikely that the condition is violated, because
> devices that actually run Linux usually have (bus) clock rates that
> are high enough to ensure the correct operation.
>
> Would it be safe to just ignore this possible fault?
I think alot of peripherals have similar constraints when there is an
interface and functional clock. However, I haven't seen drivers verify
that this kind of constraint is properly met. Personally I think its a
valid fault but one that can be ignored from the driver perspective.
>
> Regards
>
> Mario
>
>>
>> Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
>> ---
>> Version 2 changes:
>> Used NULL instead of 0 for unused hclk handle
>>
>> drivers/net/can/m_can/m_can.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index f4947a7..ea48e59 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device
>> *pdev)
>> hclk = devm_clk_get(&pdev->dev, "hclk");
>> cclk = devm_clk_get(&pdev->dev, "cclk");
>>
>> - if (IS_ERR(hclk) || IS_ERR(cclk)) {
>> - dev_err(&pdev->dev, "no clock found\n");
>> + if (IS_ERR(hclk)) {
>> + dev_warn(&pdev->dev, "hclk could not be found\n");
>> + hclk = NULL;
>> + }
>> +
>> + if (IS_ERR(cclk)) {
>> + dev_err(&pdev->dev, "cclk could not be found\n");
>> ret = -ENODEV;
>> goto failed_ret;
>> }
>
>
