| Message ID | 20170724225142.19975-2-fcooper@ti.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
+ some OMAP folks and Linux OMAP list On Tuesday 25 July 2017 04:21 AM, Franklin Cooper 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 agree that hclk is defined as interface clock for M_CAN IP on DRA76x. However, there may be a need for the driver to know the value of hclk to properly configure the RAM watchdog register which has a counter counting down using hclk. Looks like the driver does not use the RAM watchdog today. But if there is a need to configure it in future, it might be a problem. Is there a restriction in OMAP architecture against passing the interface clock also in the 'clocks' property in DT. I have not tried it myself, but wonder if you hit an issue that led to this patch. > > Signed-off-by: Franklin S Cooper Jr <fcooper@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; What is the purpose of NULL setting the clock. I think this is taking it into a very implementation defined territory and the result could be different on different architectures. See Russell's explanation here: https://lkml.org/lkml/2016/11/10/799 Thanks, Sekhar > + } > + > + if (IS_ERR(cclk)) { > + dev_err(&pdev->dev, "cclk could not be found\n"); > ret = -ENODEV; > goto failed_ret; > } >
> 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? 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? Regards Mario > > Signed-off-by: Franklin S Cooper Jr <fcooper@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; > }
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 <fcooper@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; >> } > >
On 08/24/2017 03:00 AM, Sekhar Nori wrote: > + some OMAP folks and Linux OMAP list > > On Tuesday 25 July 2017 04:21 AM, Franklin Cooper 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 agree that hclk is defined as interface clock for M_CAN IP on DRA76x. > > However, there may be a need for the driver to know the value of hclk to > properly configure the RAM watchdog register which has a counter > counting down using hclk. Looks like the driver does not use the RAM > watchdog today. But if there is a need to configure it in future, it > might be a problem. Honestly the RAM watchdog seems like a fundamental design problem. This RAM watchdog seems to be used in case a request to access the message ram is made but it hangs for what ever reason. Its even more complicated since the Message RAM is external to the MCAN IP so its implementation or how its handled probably differs from device to device. From example say you do have this error it isn't clear how you would recover from it. A logically answer would be to reset the entire IP but that also assumes that Message RAM will be reset along with the ip which likely depends on each SoC. But if a readl/writel command hangs will the kernel eventually throw an error on its on or will the driver just hang? If it does hang can a driver in the ISR do something to properly terminate the driver or even recover from it? > > Is there a restriction in OMAP architecture against passing the > interface clock also in the 'clocks' property in DT. I have not tried it > myself, but wonder if you hit an issue that led to this patch. No but not passing the interface clock is typical. > >> >> Signed-off-by: Franklin S Cooper Jr <fcooper@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; > > What is the purpose of NULL setting the clock. I think this is taking it > into a very implementation defined territory and the result could be > different on different architectures. See Russell's explanation here: > https://lkml.org/lkml/2016/11/10/799 > > Thanks, > Sekhar > >> + } >> + >> + if (IS_ERR(cclk)) { >> + dev_err(&pdev->dev, "cclk could not be found\n"); >> ret = -ENODEV; >> goto failed_ret; >> } >> >
On Thursday 21 September 2017 06:01 AM, Franklin S Cooper Jr wrote: > > > On 08/24/2017 03:00 AM, Sekhar Nori wrote: >> + some OMAP folks and Linux OMAP list >> >> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper 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 agree that hclk is defined as interface clock for M_CAN IP on DRA76x. >> >> However, there may be a need for the driver to know the value of hclk to >> properly configure the RAM watchdog register which has a counter >> counting down using hclk. Looks like the driver does not use the RAM >> watchdog today. But if there is a need to configure it in future, it >> might be a problem. > > Honestly the RAM watchdog seems like a fundamental design problem. > This RAM watchdog seems to be used in case a request to access the > message ram is made but it hangs for what ever reason. Its even more > complicated since the Message RAM is external to the MCAN IP so its > implementation or how its handled probably differs from device to > device. From example say you do have this error it isn't clear how you > would recover from it. A logically answer would be to reset the entire > IP but that also assumes that Message RAM will be reset along with the > ip which likely depends on each SoC. > > But if a readl/writel command hangs will the kernel eventually throw an > error on its on or will the driver just hang? If it does hang can a > driver in the ISR do something to properly terminate the driver or even > recover from it? >> >> Is there a restriction in OMAP architecture against passing the >> interface clock also in the 'clocks' property in DT. I have not tried it >> myself, but wonder if you hit an issue that led to this patch. > > No but not passing the interface clock is typical. Okay, then it sounds like it will just be easier to pass the hclk too? So it can be used if needed in future and also so that the driver can stay the same as today. Thanks, Sekhar
On 09/21/2017 09:08 AM, Sekhar Nori wrote: > On Thursday 21 September 2017 06:01 AM, Franklin S Cooper Jr wrote: >> >> >> On 08/24/2017 03:00 AM, Sekhar Nori wrote: >>> + some OMAP folks and Linux OMAP list >>> >>> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper 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 agree that hclk is defined as interface clock for M_CAN IP on DRA76x. >>> >>> However, there may be a need for the driver to know the value of hclk to >>> properly configure the RAM watchdog register which has a counter >>> counting down using hclk. Looks like the driver does not use the RAM >>> watchdog today. But if there is a need to configure it in future, it >>> might be a problem. >> >> Honestly the RAM watchdog seems like a fundamental design problem. >> This RAM watchdog seems to be used in case a request to access the >> message ram is made but it hangs for what ever reason. Its even more >> complicated since the Message RAM is external to the MCAN IP so its >> implementation or how its handled probably differs from device to >> device. From example say you do have this error it isn't clear how you >> would recover from it. A logically answer would be to reset the entire >> IP but that also assumes that Message RAM will be reset along with the >> ip which likely depends on each SoC. >> >> But if a readl/writel command hangs will the kernel eventually throw an >> error on its on or will the driver just hang? If it does hang can a >> driver in the ISR do something to properly terminate the driver or even >> recover from it? >>> >>> Is there a restriction in OMAP architecture against passing the >>> interface clock also in the 'clocks' property in DT. I have not tried it >>> myself, but wonder if you hit an issue that led to this patch. >> >> No but not passing the interface clock is typical. > > Okay, then it sounds like it will just be easier to pass the hclk too? > > So it can be used if needed in future and also so that the driver can > stay the same as today. That is fine. For now I can just drop this patch unless I discover when enabling it on DRA76x I am unable to add the interface clock to dt. > > Thanks, > Sekhar >
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; }
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. Signed-off-by: Franklin S Cooper Jr <fcooper@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(-)