Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
On 10/19/2017 09:55 AM, Marc Kleine-Budde wrote: > On 10/19/2017 03:54 PM, Franklin S Cooper Jr wrote: >> On 10/19/2017 06:32 AM, Marc Kleine-Budde wrote: >>> On 10/19/2017 01:09 PM, Sekhar Nori wrote: >>>> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote: >>>>> On 10/19/2017 07:07 AM, Sekhar Nori wrote: >>>>>>>>> Sounds reasonable. What's the status of this series? >>>>>>>> >>>>>>>> I have had some offline discussions with Franklin on this, and I am not >>>>>>>> fully convinced that DT is the way to go here (although I don't have >>>>>>>> the >>>>>>>> agreement with Franklin there). >>>>>>> >>>>>>> Probably the fundamental area where we disagree is what "default" SSP >>>>>>> value should be used. Based on a short (< 1 ft) point to point test >>>>>>> using a SSP of 50% worked fine. However, I'm not convinced that this >>>>>>> default value of 50% will work in a more "traditional" CAN bus at higher >>>>>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN >>>>>>> board in even the simplest test cases. >>>>>>> >>>>>>> I believe that this default SSP should be a DT property that allows any >>>>>>> board to determine what default value works best in general. >>>>>> >>>>>> With that, I think, we are taking DT from describing board/hardware >>>>>> characteristics to providing default values that software should use. >>>>> >>>>> If the default value is board specific and cannot be calculated in >>>>> general or from other values present in the DT, then it's from my point >>>>> of view describing the hardware. >>>>> >>>>>> In any case, if Marc and/or Wolfgang are okay with it, binding >>>>>> documentation for such a property should be sent to DT maintainers for >>>>>> review. >>>>>> >>>>>>>> >>>>>>>> There are two components in configuring the secondary sample point. It >>>>>>>> is the transceiver loopback delay and an offset (example half of the >>>>>>>> bit >>>>>>>> time in data phase). >>>>>>>> >>>>>>>> While the transceiver loopback delay is pretty board dependent (and >>>>>>>> thus >>>>>>>> amenable to DT encoding), I am not quite sure the offset can be >>>>>>>> configured in DT because its not really board dependent. >>>>>>>> >>>>>>>> Unfortunately, offset calculation does not seem to be an exact science. >>>>>>>> There are recommendations ranging from using 50% of bit time to making >>>>>>>> it same as the sample point configured. This means users who need to >>>>>>>> change the SSP due to offset variations need to change their DT even >>>>>>>> without anything changing on their board. >>>>>>>> >>>>>>>> Since we have a netlink socket interface to configure sample point, I >>>>>>>> wonder if that should be extended to configure SSP too (or at least the >>>>>>>> offset part of SSP)? >>>>>>> >>>>>>> Sekhar is right that ideally the user should be able to set the SSP at >>>>>>> runtime. However, my issue is that based on my experience CAN users >>>>>>> expect the driver to just work the majority of times. For unique use >>>>>>> cases where the driver calculated values don't work then the user should >>>>>>> be able to override it. This should only be done for a very small >>>>>>> percentage of CAN users. Unless you allow DT to provide a default SSP >>>>>>> many users of MCAN may find that the default SSP doesn't work and must >>>>>>> always use runtime overrides to get anything to work. I don't think that >>>>>>> is a good user experience which is why I don't like the idea. >>>>>> >>>>>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just >>>>>> work" without doing
Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
On 10/19/2017 06:32 AM, Marc Kleine-Budde wrote: > On 10/19/2017 01:09 PM, Sekhar Nori wrote: >> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote: >>> On 10/19/2017 07:07 AM, Sekhar Nori wrote: >>> Sounds reasonable. What's the status of this series? >> >> I have had some offline discussions with Franklin on this, and I am not >> fully convinced that DT is the way to go here (although I don't have the >> agreement with Franklin there). > > Probably the fundamental area where we disagree is what "default" SSP > value should be used. Based on a short (< 1 ft) point to point test > using a SSP of 50% worked fine. However, I'm not convinced that this > default value of 50% will work in a more "traditional" CAN bus at higher > speeds. Nor am I convinced that a SSP of 50% will work on every MCAN > board in even the simplest test cases. > > I believe that this default SSP should be a DT property that allows any > board to determine what default value works best in general. With that, I think, we are taking DT from describing board/hardware characteristics to providing default values that software should use. >>> >>> If the default value is board specific and cannot be calculated in >>> general or from other values present in the DT, then it's from my point >>> of view describing the hardware. >>> In any case, if Marc and/or Wolfgang are okay with it, binding documentation for such a property should be sent to DT maintainers for review. >> >> There are two components in configuring the secondary sample point. It >> is the transceiver loopback delay and an offset (example half of the bit >> time in data phase). >> >> While the transceiver loopback delay is pretty board dependent (and thus >> amenable to DT encoding), I am not quite sure the offset can be >> configured in DT because its not really board dependent. >> >> Unfortunately, offset calculation does not seem to be an exact science. >> There are recommendations ranging from using 50% of bit time to making >> it same as the sample point configured. This means users who need to >> change the SSP due to offset variations need to change their DT even >> without anything changing on their board. >> >> Since we have a netlink socket interface to configure sample point, I >> wonder if that should be extended to configure SSP too (or at least the >> offset part of SSP)? > > Sekhar is right that ideally the user should be able to set the SSP at > runtime. However, my issue is that based on my experience CAN users > expect the driver to just work the majority of times. For unique use > cases where the driver calculated values don't work then the user should > be able to override it. This should only be done for a very small > percentage of CAN users. Unless you allow DT to provide a default SSP > many users of MCAN may find that the default SSP doesn't work and must > always use runtime overrides to get anything to work. I don't think that > is a good user experience which is why I don't like the idea. Fair enough. But not quite sure if CAN users expect CAN-FD to "just work" without doing any bittiming related setup. >>> >>> From my point of view I'd rather buy a board from a HW vendor where >>> CAN-FD works, rather than a board where I have to tweak the bit-timing >>> for a simple CAN-FD test setup. >>> >>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5 >>> MBit/s -> 50%". Do we need an array of tuples in general? Internally what I proposed was a binding that allowed you to pass in an array of a range of baud rates and then a SSP for that baud rate range. Therefore, if the baud rate being used impacted what SSP worked then it allows someone to provide a range of defaults. Of course a person also has the ability to use a single large range thus implementing a single default SSP value. > > Do we need more than one tuple here? > >>> If good default values are transceiver and board specific, they can go >>> into the DT. We need a generic (this means driver agnostic) binding for >>> this. If this table needs to be tweaked for special purpose, then we can >>> add a netlink interface for this as well. >>> >>> Comments? >> >> I dont know how a good default (other than 50% as the starting point) >> can be arrived at without doing any actual measurements on the actual >> network. Since we do know that the value has to be tweaked, agree that >> netlink interface has to be provided. Now I have seen in non public documentations that setting SP to SSP also works. This makes a bit more sense to me and I'm alot more comfortable going with this. However, since its based on non public information I can't justify it beyond that "it works for me". But I'm alot more comfortable going with then saying "hey this default value
Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
On 10/18/2017 08:24 AM, Sekhar Nori wrote: > Hi Marc, > > On Wednesday 18 October 2017 06:14 PM, Marc Kleine-Budde wrote: >> On 09/21/2017 02:48 AM, Franklin S Cooper Jr wrote: >>> >>> >>> On 09/20/2017 04:37 PM, Mario Hüttel wrote: >>>> >>>> >>>> On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote: >>>>> Hi Wenyou, >>>>> >>>>> On 09/17/2017 10:47 PM, Yang, Wenyou wrote: >>>>>> >>>>>> On 2017/9/14 13:06, Sekhar Nori wrote: >>>>>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote: >>>>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote: >>>>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only >>>>>>>>> resulted in errors. Scoping the signals I noticed that only a single >>>>>>>>> bit >>>>>>>>> was being transmitted and with a bit more investigation realized the >>>>>>>>> actual >>>>>>>>> MCAN IP would go back to initialization mode automatically. >>>>>>>>> >>>>>>>>> It appears this issue is due to the MCAN needing to use the >>>>>>>>> Transmitter >>>>>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this >>>>>>>>> mode is used the User's Guide indicates that the Transmitter Delay >>>>>>>>> Compensation Offset register should be set. The document mentions >>>>>>>>> that this >>>>>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq). >>>>>>>>> >>>>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document >>>>>>>>> indicates >>>>>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps. >>>>>>>>> Therefore, only enable this mode and only set TDCO when the data bit >>>>>>>>> rate >>>>>>>>> is above 2.5 Mbps. >>>>>>>>> >>>>>>>>> Signed-off-by: Franklin S Cooper Jr >>>>>>>>> --- >>>>>>>>> I'm pretty surprised that this hasn't been implemented already since >>>>>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP >>>>>>>>> supports up to 10 Mbps. >>>>>>>>> >>>>>>>>> So it will be nice to get comments from users of this driver to >>>>>>>>> understand >>>>>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this >>>>>>>>> patch. >>>>>>>>> If they haven't what did they do to get around it if they needed >>>>>>>>> higher >>>>>>>>> speeds. >>>>>>>>> >>>>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to >>>>>>>>> insure >>>>>>>>> everything still works at 5 Mbps which is the max speed of my CAN >>>>>>>>> transceiver. >>>>>>>> ping. Anyone has any thoughts on this? >>>>>>> I added Dong who authored the m_can driver and Wenyou who added the only >>>>>>> in-kernel user of the driver for any help. >>>>>> I tested it on SAMA5D2 Xplained board both with and without this patch, >>>>>> both work with the 4M bps data bit rate. >>>>> Thank you for testing this out. Its interesting that you have been able >>>>> to use higher speeds without this patch. What is the CAN transceiver >>>>> being used on the SAMA5D2 Xplained board? I tried looking at the >>>>> schematic but it seems the CAN signals are used on an extension board >>>>> which I can't find the schematic for. Also do you mind sharing your test >>>>> setup? Were you doing a short point to point test? >>>>> >>>>> Thank You, >>>>> Franklin >>>> Hello Franklin, >>>> >>>> your patch definitely makes sense. >>>> >>>> I forgot the TDC in my patches because it was not present in the >>>> previous
Re: [v2,1/3] can: m_can: Make hclk optional
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 >
Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
On 09/20/2017 04:37 PM, Mario Hüttel wrote: > > > On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote: >> Hi Wenyou, >> >> On 09/17/2017 10:47 PM, Yang, Wenyou wrote: >>> >>> On 2017/9/14 13:06, Sekhar Nori wrote: >>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote: >>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote: >>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only >>>>>> resulted in errors. Scoping the signals I noticed that only a single >>>>>> bit >>>>>> was being transmitted and with a bit more investigation realized the >>>>>> actual >>>>>> MCAN IP would go back to initialization mode automatically. >>>>>> >>>>>> It appears this issue is due to the MCAN needing to use the Transmitter >>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this >>>>>> mode is used the User's Guide indicates that the Transmitter Delay >>>>>> Compensation Offset register should be set. The document mentions >>>>>> that this >>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq). >>>>>> >>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document >>>>>> indicates >>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps. >>>>>> Therefore, only enable this mode and only set TDCO when the data bit >>>>>> rate >>>>>> is above 2.5 Mbps. >>>>>> >>>>>> Signed-off-by: Franklin S Cooper Jr >>>>>> --- >>>>>> I'm pretty surprised that this hasn't been implemented already since >>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP >>>>>> supports up to 10 Mbps. >>>>>> >>>>>> So it will be nice to get comments from users of this driver to >>>>>> understand >>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this >>>>>> patch. >>>>>> If they haven't what did they do to get around it if they needed higher >>>>>> speeds. >>>>>> >>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to >>>>>> insure >>>>>> everything still works at 5 Mbps which is the max speed of my CAN >>>>>> transceiver. >>>>> ping. Anyone has any thoughts on this? >>>> I added Dong who authored the m_can driver and Wenyou who added the only >>>> in-kernel user of the driver for any help. >>> I tested it on SAMA5D2 Xplained board both with and without this patch, >>> both work with the 4M bps data bit rate. >> Thank you for testing this out. Its interesting that you have been able >> to use higher speeds without this patch. What is the CAN transceiver >> being used on the SAMA5D2 Xplained board? I tried looking at the >> schematic but it seems the CAN signals are used on an extension board >> which I can't find the schematic for. Also do you mind sharing your test >> setup? Were you doing a short point to point test? >> >> Thank You, >> Franklin > Hello Franklin, > > your patch definitely makes sense. > > I forgot the TDC in my patches because it was not present in the > previous driver versions and because I didn't encounter any > problems when testing it myself. > > The error is highly dependent on the hardware (transceiver) setup. > So it is definitely possible that some people don't encounter errors > without your patch. So the Transmission Delay Compensation feature Value register is suppose to take into consideration the transceiver delay automatically and add the value of TDCO on top of that. So why would TDCO be dependent on the transceiver? I've heard conflicting things regarding TDC so any clarification on what actually impacts it would be appreciated. Also part of the issue I'm having is how can we properly configure TDCO? Configuring TDCO is essentially figuring out what Secondary Sample Point to use. However, it is unclear what value to set SSP to and which use cases a given SSP will work or doesn't work. I've seen various recommendations from Bosch on choosing SSP but ultimately it seems they suggestion "real world testing" to come up with a proper value. Not setting TDCO causes problems for my device and improperly setting TDCO causes problems for my device. So its likely any value I use could end up breaking something for someone else. Currently I leaning to a DT property that can be used for setting SSP. Perhaps use a generic default value and allow individuals to override it via DT? > > Could you clarify what you meant with >> Scoping the signals I noticed that only a single bit was being transmitted > > Do you mean one data bit (high bit rate) or did the core already fail > in the arbitration phase? A single bit during the arbitration phase. Essentially the Start of Frame bit is sent and then nothing else and the IP resets. > > There is also another aspect that can lead to errors: > > If the CAN clock 'cclk' is above the frequency of the interface/logic > clock 'hclk', the clock domain crossing of the CAN messages can't > work properly. However, I will throw this topic as an extra e-mail into > the round. > > Mario > > >
Re: [v2,3/3] can: m_can: Add PM Runtime
On 08/24/2017 03:30 AM, Sekhar Nori wrote:
> + OMAP mailing list
>
> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
>> Add support for PM Runtime which is the new way to handle managing clocks.
>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>> management approach in place.
>
> Since hclk is the interface clock, I think at a minimum there needs to
> be an assumption that pm_runtime_get_sync() will enable that clock and
> make the module ready for access.
>
> The only in-kernel user of this driver seems to be an atmel SoC. I am
> ccing folks who added support for M_CAN on that SoC to see if hclk
> management can be left to pm_runtime*() on their SoC.
>
> If they are okay, I think its a good to atleast drop explicit hclk
> enable disable in the driver. That way, we avoid double enable/disable
> of interface clock (hclk).
Wenyou,
Do you foresee this being a problem on your board? If not I can send a
v3 removing these clk_enable and clk_disable calls and it would be great
if you can test it out and provide your tested by.
>
>>
>> PM_RUNTIME is required by OMAP based devices to handle clock management.
>> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
>> to work with this driver.
>>
>> Signed-off-by: Franklin S Cooper Jr
>
> Thanks,
> Sekhar
>
>> ---
>> drivers/net/can/m_can/m_can.c | 13 +
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index ea48e59..93e23f5 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -23,6 +23,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -633,11 +634,15 @@ static int m_can_clk_start(struct m_can_priv *priv)
>> if (err)
>> clk_disable_unprepare(priv->hclk);
>>
>> +pm_runtime_get_sync(priv->device);
>> +
>> return err;
>> }
>>
>> static void m_can_clk_stop(struct m_can_priv *priv)
>> {
>> +pm_runtime_put_sync(priv->device);
>> +
>> clk_disable_unprepare(priv->cclk);
>> clk_disable_unprepare(priv->hclk);
>> }
>> @@ -1582,6 +1587,8 @@ static int m_can_plat_probe(struct platform_device
>> *pdev)
>> /* Enable clocks. Necessary to read Core Release in order to determine
>> * M_CAN version
>> */
>> +pm_runtime_enable(&pdev->dev);
>> +
>> ret = clk_prepare_enable(hclk);
>> if (ret)
>> goto disable_hclk_ret;
>> @@ -1626,6 +1633,8 @@ static int m_can_plat_probe(struct platform_device
>> *pdev)
>> */
>> tx_fifo_size = mram_config_vals[7];
>>
>> +pm_runtime_get_sync(&pdev->dev);
>> +
>> /* allocate the m_can device */
>> dev = alloc_m_can_dev(pdev, addr, tx_fifo_size);
>> if (!dev) {
>> @@ -1670,6 +1679,7 @@ static int m_can_plat_probe(struct platform_device
>> *pdev)
>> disable_hclk_ret:
>> clk_disable_unprepare(hclk);
>> failed_ret:
>> +pm_runtime_put_sync(&pdev->dev);
>> return ret;
>> }
>>
>> @@ -1726,6 +1736,9 @@ static int m_can_plat_remove(struct platform_device
>> *pdev)
>> struct net_device *dev = platform_get_drvdata(pdev);
>>
>> unregister_m_can_dev(dev);
>> +
>> +pm_runtime_disable(&pdev->dev);
>> +
>> platform_set_drvdata(pdev, NULL);
>>
>> free_m_can_dev(dev);
>>
>
Re: [v2,1/3] can: m_can: Make hclk optional
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
>> ---
>> 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;
>> }
>>
>
Re: [PATCH v2 1/3] can: m_can: Make hclk optional
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
>> ---
>> 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;
>> }
>
>
Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
Hi Wenyou,
On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>
>
> On 2017/9/14 13:06, Sekhar Nori wrote:
>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>
>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>> bit
>>>> was being transmitted and with a bit more investigation realized the
>>>> actual
>>>> MCAN IP would go back to initialization mode automatically.
>>>>
>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>> Compensation Offset register should be set. The document mentions
>>>> that this
>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>
>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>> indicates
>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>> rate
>>>> is above 2.5 Mbps.
>>>>
>>>> Signed-off-by: Franklin S Cooper Jr
>>>> ---
>>>> I'm pretty surprised that this hasn't been implemented already since
>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>> supports up to 10 Mbps.
>>>>
>>>> So it will be nice to get comments from users of this driver to
>>>> understand
>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>> patch.
>>>> If they haven't what did they do to get around it if they needed higher
>>>> speeds.
>>>>
>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>> insure
>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>> transceiver.
>>> ping. Anyone has any thoughts on this?
>> I added Dong who authored the m_can driver and Wenyou who added the only
>> in-kernel user of the driver for any help.
> I tested it on SAMA5D2 Xplained board both with and without this patch,
> both work with the 4M bps data bit rate.
Thank you for testing this out. Its interesting that you have been able
to use higher speeds without this patch. What is the CAN transceiver
being used on the SAMA5D2 Xplained board? I tried looking at the
schematic but it seems the CAN signals are used on an extension board
which I can't find the schematic for. Also do you mind sharing your test
setup? Were you doing a short point to point test?
Thank You,
Franklin
>
>>
>> Thanks,
>> Sekhar
>>
>>>> drivers/net/can/m_can/m_can.c | 24 +++-
>>>> 1 file changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/can/m_can/m_can.c
>>>> b/drivers/net/can/m_can/m_can.c
>>>> index f4947a7..720e073 100644
>>>> --- a/drivers/net/can/m_can/m_can.c
>>>> +++ b/drivers/net/can/m_can/m_can.c
>>>> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
>>>> #define DBTP_DSJW_SHIFT0
>>>> #define DBTP_DSJW_MASK(0xf << DBTP_DSJW_SHIFT)
>>>> +/* Transmitter Delay Compensation Register (TDCR) */
>>>> +#define TDCR_TDCO_SHIFT8
>>>> +#define TDCR_TDCO_MASK(0x7F << TDCR_TDCO_SHIFT)
>>>> +#define TDCR_TDCF_SHIFT0
>>>> +#define TDCR_TDCF_MASK(0x7F << TDCR_TDCO_SHIFT)
>>>> +
>>>> /* Test Register (TEST) */
>>>> #define TEST_LBCKBIT(4)
>>>> @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct
>>>> net_device *dev)
>>>> const struct can_bittiming *dbt = &priv->can.data_bittiming;
>>>> u16 brp, sjw, tseg1, tseg2;
>>>> u32 reg_btp;
>>>> +u32 enable_tdc = 0;
>>>> +u32 tdco;
>>>> brp = bt->brp - 1;
>>>> sjw = bt->sjw - 1;
>>>> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct
>>>> net_device *dev)
>>>> sjw = dbt->sjw - 1;
>>>> tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
>>>> tseg2 = dbt->phase_seg2 - 1;
>>>> +
>>>> +/* TDC is only needed for bitrates beyond 2.5 MBit/s
>>>> + * Specified in the "Bit Time Requirements for CAN FD"
>>>> document
>>>> + */
>>>> +if (dbt->bitrate > 250) {
>>>> +enable_tdc = DBTP_TDC;
>>>> +/* Equation based on Bosch's M_CAN User Manual's
>>>> + * Transmitter Delay Compensation Section
>>>> + */
>>>> +tdco = priv->can.clock.freq / (dbt->bitrate * 2);
>>>> +m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
>>>> +}
>>>> +
>>>> reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw <<
>>>> DBTP_DSJW_SHIFT) |
>>>> (tseg1 << DBTP_DTSEG1_SHIFT) |
>>>> -(tseg2 << DBTP_DTSEG2_SHIFT);
>>>> +(tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
>>>> +
>>>> m_can_write(priv, M_CAN_DBTP, reg_btp);
>>>> }
>>>>
>
> Regards,
> Wenyou Yang
>
Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
> resulted in errors. Scoping the signals I noticed that only a single bit
> was being transmitted and with a bit more investigation realized the actual
> MCAN IP would go back to initialization mode automatically.
>
> It appears this issue is due to the MCAN needing to use the Transmitter
> Delay Compensation Mode as defined in the MCAN User's Guide. When this
> mode is used the User's Guide indicates that the Transmitter Delay
> Compensation Offset register should be set. The document mentions that this
> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>
> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
> Therefore, only enable this mode and only set TDCO when the data bit rate
> is above 2.5 Mbps.
>
> Signed-off-by: Franklin S Cooper Jr
> ---
> I'm pretty surprised that this hasn't been implemented already since
> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
> supports up to 10 Mbps.
>
> So it will be nice to get comments from users of this driver to understand
> if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
> If they haven't what did they do to get around it if they needed higher
> speeds.
>
> Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
> everything still works at 5 Mbps which is the max speed of my CAN
> transceiver.
ping. Anyone has any thoughts on this?
>
> drivers/net/can/m_can/m_can.c | 24 +++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index f4947a7..720e073 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
> #define DBTP_DSJW_SHIFT 0
> #define DBTP_DSJW_MASK (0xf << DBTP_DSJW_SHIFT)
>
> +/* Transmitter Delay Compensation Register (TDCR) */
> +#define TDCR_TDCO_SHIFT 8
> +#define TDCR_TDCO_MASK (0x7F << TDCR_TDCO_SHIFT)
> +#define TDCR_TDCF_SHIFT 0
> +#define TDCR_TDCF_MASK (0x7F << TDCR_TDCO_SHIFT)
> +
> /* Test Register (TEST) */
> #define TEST_LBCKBIT(4)
>
> @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
> const struct can_bittiming *dbt = &priv->can.data_bittiming;
> u16 brp, sjw, tseg1, tseg2;
> u32 reg_btp;
> + u32 enable_tdc = 0;
> + u32 tdco;
>
> brp = bt->brp - 1;
> sjw = bt->sjw - 1;
> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
> sjw = dbt->sjw - 1;
> tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
> tseg2 = dbt->phase_seg2 - 1;
> +
> + /* TDC is only needed for bitrates beyond 2.5 MBit/s
> + * Specified in the "Bit Time Requirements for CAN FD" document
> + */
> + if (dbt->bitrate > 250) {
> + enable_tdc = DBTP_TDC;
> + /* Equation based on Bosch's M_CAN User Manual's
> + * Transmitter Delay Compensation Section
> + */
> + tdco = priv->can.clock.freq / (dbt->bitrate * 2);
> + m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
> + }
> +
> reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
> (tseg1 << DBTP_DTSEG1_SHIFT) |
> - (tseg2 << DBTP_DTSEG2_SHIFT);
> + (tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
> +
> m_can_write(priv, M_CAN_DBTP, reg_btp);
> }
>
>
[PATCH v4 1/2] i2c: davinci: Add PM Runtime Support
66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
unlike other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.
Signed-off-by: Franklin S Cooper Jr
---
Version 4 changes:
Fix wording on error message
Fix additional error path
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path
drivers/i2c/busses/i2c-davinci.c | 67 +---
1 file changed, 55 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..a2d9f7c 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
/* - global defines --- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000/* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem*base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(&dev->xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,13 +821,24 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(&pdev->dev, mem);
if (IS_ERR(dev->base)) {
- r = PTR_ERR(dev->base);
- goto err_unuse_clocks;
+ return PTR_ERR(dev->base);
+ }
+
+ pm_runtime_set_autosuspend_delay(dev->dev,
+DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ pm_runtime_enable(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
+ pm_runtime_put_noidle(dev->dev);
+ return r;
}
i2c_davinci_init(dev);
@@ -849,27 +879,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
return r;
}
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(&dev->adapter);
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&pdev->dev);
+
[PATCH v4 2/2] dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr Acked-by: Rob Herring Acked-by: Sekhar Nori --- Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt index 5b123e0..64e6e65 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt @@ -6,6 +6,18 @@ davinci/keystone i2c interface contains. Required properties: - compatible: "ti,davinci-i2c" or "ti,keystone-i2c"; - reg : Offset and length of the register set for the device +- clocks: I2C functional clock phandle. + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the I2C device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Recommended properties : - interrupts : standard interrupt property. -- 2.9.4.dirty
[PATCH v4 0/2] i2c: davinci: Add PM Runtime Support needed by 66AK2G
Add I2C support to 66AK2G. Primary requirement is to add PM Runtime support to the driver. This has been tested on following platforms by performing simple i2c test such as i2c detect and reading on board i2c devices: K2G GP evm OMAPL138 K2L GP EVM and boot tested on: K2E GP EVM K2HK GP EVM Version 2 changes: Moved ordering of pm runtime calls Version 3 changes: Dropped first patch since it was pulled in. Remove clk = NULL statements Fix error path Version 4 changes: Fix typo in commit message Fix error path Franklin S Cooper Jr (2): i2c: davinci: Add PM Runtime Support dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property .../devicetree/bindings/i2c/i2c-davinci.txt| 12 drivers/i2c/busses/i2c-davinci.c | 67 ++ 2 files changed, 67 insertions(+), 12 deletions(-) -- 2.9.4.dirty
Re: [PATCH v3 1/2] i2c: davinci: Add PM Runtime Support
On 09/11/2017 06:29 AM, Sekhar Nori wrote:
> On Friday 08 September 2017 11:24 PM, Franklin S Cooper Jr wrote:
>> 66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
>> like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
>
> unlike ?
>
>> is required to insure the power domain used by the specific I2C instance is
>> properly turned on along with its functional clock.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Version 3 changes:
>> Remove several statements that set clk to NULL
>> Fix error path
>>
>> drivers/i2c/busses/i2c-davinci.c | 64
>> +---
>> 1 file changed, 53 insertions(+), 11 deletions(-)
>
>> @@ -802,12 +821,22 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> dev->clk = devm_clk_get(&pdev->dev, NULL);
>> if (IS_ERR(dev->clk))
>> return PTR_ERR(dev->clk);
>> -clk_prepare_enable(dev->clk);
>>
>> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> dev->base = devm_ioremap_resource(&pdev->dev, mem);
>> if (IS_ERR(dev->base)) {
>> -r = PTR_ERR(dev->base);
>> +return PTR_ERR(dev->base);
>> +}
>> +
>> +pm_runtime_set_autosuspend_delay(dev->dev,
>> + DAVINCI_I2C_PM_TIMEOUT);
>> +pm_runtime_use_autosuspend(dev->dev);
>> +
>> +pm_runtime_enable(dev->dev);
>> +
>> +r = pm_runtime_get_sync(dev->dev);
>> +if (r < 0) {
>> +dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
>> goto err_unuse_clocks;
>
> You end up doing a pm_runtime_put_sync() on failure here, instead of
> pm_runtime_put_noidle() like rest of the patch.
>
> May be handle this failure here instead of relying on the goto path.
Ok
>
>> }
>>
>> @@ -849,27 +878,40 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> if (r)
>> goto err_unuse_clocks;
>>
>> +pm_runtime_mark_last_busy(dev->dev);
>> +pm_runtime_put_autosuspend(dev->dev);
>> +
>> return 0;
>>
>> err_unuse_clocks:
>> -clk_disable_unprepare(dev->clk);
>> -dev->clk = NULL;
>> +pm_runtime_dont_use_autosuspend(dev->dev);
>> +pm_runtime_put_sync(dev->dev);
>> +pm_runtime_disable(dev->dev);
>> +
>> return r;
>> }
>
> Thanks,
> Sekhar
>
[PATCH 3/3] ARM: dts: keystone-k2g-evm: add bindings for SPI NOR flash
From: Murali Karicheri
K2G EVM has n25q128a13 SPI NOR flash on SPI1. Enable SPI1 in the DT
node as well as add a subnode for the SPI NOR.
Signed-off-by: Murali Karicheri
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g-evm.dts | 37 ++
1 file changed, 37 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g-evm.dts
b/arch/arm/boot/dts/keystone-k2g-evm.dts
index f462f10..d5c43c7 100644
--- a/arch/arm/boot/dts/keystone-k2g-evm.dts
+++ b/arch/arm/boot/dts/keystone-k2g-evm.dts
@@ -81,6 +81,16 @@
K2G_CORE_IOPAD(0x1110) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* mmc1_cmd.mmc1_cmd */
>;
};
+
+ spi1_pins: pinmux_spi1_pins {
+ pinctrl-single,pins = <
+ K2G_CORE_IOPAD(0x11a4) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_scs0.spi1_scs0 */
+ K2G_CORE_IOPAD(0x11ac) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_clk.spi1_clk */
+ K2G_CORE_IOPAD(0x11b0) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_miso.spi1_miso */
+ K2G_CORE_IOPAD(0x11b4) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_mosi.spi1_mosi */
+ >;
+ };
+
};
&uart0 {
@@ -112,3 +122,30 @@
memory-region = <&dsp_common_memory>;
status = "okay";
};
+
+&spi1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&spi1_pins>;
+ status = "okay";
+
+ spi_nor: flash@0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "jedec,spi-nor";
+ spi-max-frequency = <500>;
+ m25p,fast-read;
+ reg = <0>;
+
+ partition@0 {
+ label = "u-boot-spl";
+ reg = <0x0 0x10>;
+ read-only;
+ };
+
+ partition@1 {
+ label = "misc";
+ reg = <0x10 0xf0>;
+ };
+ };
+};
+
--
2.9.4.dirty
[PATCH 1/3] dt-bindings: spi: spi-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr --- Documentation/devicetree/bindings/spi/spi-davinci.txt | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt index f5916c9..1925277 100644 --- a/Documentation/devicetree/bindings/spi/spi-davinci.txt +++ b/Documentation/devicetree/bindings/spi/spi-davinci.txt @@ -24,6 +24,16 @@ Required properties: based on a specific SoC configuration. - interrupts: interrupt number mapped to CPU. - clocks: spi clk phandle + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the SPI device id + value. This property is as per the binding, Optional: - cs-gpios: gpio chip selects -- 2.9.4.dirty
[PATCH 0/3] ARM: dts: keystone-k2g-evm: Add SPI nodes and enable SPI1 on K2G EVM
Update the binding document to indicate the required requirements for 66AK2G. In addition add the proper SPI nodes for 66AK2G. For K2G EVM enable SPI1 which is used for the on board SPI NOR. Franklin S Cooper Jr (1): dt-bindings: spi: spi-davinci: Update binding for 66AK2Gx pwr dm property Murali Karicheri (1): ARM: dts: keystone-k2g-evm: add bindings for SPI NOR flash Vitaly Andrianov (1): ARM: dts: keystone-k2g: Add SPI nodes .../devicetree/bindings/spi/spi-davinci.txt| 10 + arch/arm/boot/dts/keystone-k2g-evm.dts | 37 + arch/arm/boot/dts/keystone-k2g.dtsi| 48 ++ 3 files changed, 95 insertions(+) -- 2.9.4.dirty
[PATCH 2/3] ARM: dts: keystone-k2g: Add SPI nodes
From: Vitaly Andrianov
Add nodes for the various SPI instances.
Signed-off-by: Vitaly Andrianov
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g.dtsi | 48 +
1 file changed, 48 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index 826b286..9f9e9e1 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -343,5 +343,53 @@
clock-names = "fck", "mmchsdb_fck";
status = "disabled";
};
+
+ spi0: spi@21805400 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21805400 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <&k2g_pds 0x0010>;
+ clocks = <&k2g_clks 0x0010 0>;
+ };
+
+ spi1: spi@21805800 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21805800 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <&k2g_pds 0x0011>;
+ clocks = <&k2g_clks 0x0011 0>;
+ };
+
+ spi2: spi@21805c00 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21805C00 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <&k2g_pds 0x0012>;
+ clocks = <&k2g_clks 0x0012 0>;
+ };
+
+ spi3: spi@21806000 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21806000 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <&k2g_pds 0x0013>;
+ clocks = <&k2g_clks 0x0013 0>;
+ };
};
};
--
2.9.4.dirty
[PATCH v2 2/2] ARM: dts: k2g-evm: Enable USB 0 and 1
From: Roger Quadros
Enable USB 0 which will be used as a host port and USB 1 which will be
used in peripheral mode.
Signed-off-by: Roger Quadros
Signed-off-by: Murali Karicheri
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g-evm.dts | 26 ++
1 file changed, 26 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g-evm.dts
b/arch/arm/boot/dts/keystone-k2g-evm.dts
index 61883cb..4f56295 100644
--- a/arch/arm/boot/dts/keystone-k2g-evm.dts
+++ b/arch/arm/boot/dts/keystone-k2g-evm.dts
@@ -41,3 +41,29 @@
pinctrl-0 = <&uart0_pins>;
status = "okay";
};
+
+&keystone_usb0 {
+ status = "okay";
+};
+
+&usb0_phy {
+ status = "okay";
+};
+
+&usb0 {
+ dr_mode = "host";
+ status = "okay";
+};
+
+&keystone_usb1 {
+ status = "okay";
+};
+
+&usb1_phy {
+ status = "okay";
+};
+
+&usb1 {
+ dr_mode = "peripheral";
+ status = "okay";
+};
--
2.9.4.dirty
[PATCH v2 1/2] ARM: dts: k2g: Add USB instances
From: Vitaly Andrianov
Add nodes for both USB instances supported by 66AK2G.
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g.dtsi | 56 +
1 file changed, 56 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index a789f75..c8f3b93 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -139,5 +139,61 @@
interrupts = ,
;
};
+
+ usb0_phy: usb-phy@0 {
+ compatible = "usb-nop-xceiv";
+ status = "disabled";
+ };
+
+ keystone_usb0: keystone-dwc3@268 {
+ compatible = "ti,keystone-dwc3";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x268 0x1>;
+ interrupts = ;
+ ranges;
+ dma-coherent;
+ dma-ranges;
+ status = "disabled";
+ power-domains = <&k2g_pds 0x0016>;
+
+ usb0: usb@269 {
+ compatible = "snps,dwc3";
+ reg = <0x269 0x1>;
+ interrupts = ;
+ maximum-speed = "high-speed";
+ dr_mode = "otg";
+ usb-phy = <&usb0_phy>;
+ status = "disabled";
+ };
+ };
+
+ usb1_phy: usb-phy@1 {
+ compatible = "usb-nop-xceiv";
+ status = "disabled";
+ };
+
+ keystone_usb1: keystone-dwc3@258 {
+ compatible = "ti,keystone-dwc3";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x258 0x1>;
+ interrupts = ;
+ ranges;
+ dma-coherent;
+ dma-ranges;
+ status = "disabled";
+ power-domains = <&k2g_pds 0x0017>;
+
+ usb1: usb@259 {
+ compatible = "snps,dwc3";
+ reg = <0x259 0x1>;
+ interrupts = ;
+ maximum-speed = "high-speed";
+ dr_mode = "otg";
+ usb-phy = <&usb1_phy>;
+ status = "disabled";
+ };
+ };
};
};
--
2.9.4.dirty
[PATCH v2 0/2] ARM: dts: keystone-k2g-evm: Add USB nodes and enable USB on K2G EVM
Add USB nodes to 66AK2G dtsi. Also enable the two USB nodes that are on the K2G EVM. Roger Quadros (1): ARM: dts: k2g-evm: Enable USB 0 and 1 Vitaly Andrianov (1): ARM: dts: k2g: Add USB instances arch/arm/boot/dts/keystone-k2g-evm.dts | 26 arch/arm/boot/dts/keystone-k2g.dtsi| 56 ++ 2 files changed, 82 insertions(+) -- 2.9.4.dirty
[PATCH v2 2/2] ARM: dts: keystone-k2g-evm: Add I2C EEPROM DT entry
From: Murali Karicheri
K2G EVM has an onboard I2C EEPROM connected to I2C0. This patch adds
the necessary DT entry for the AT24CM01 EEPROM.
Signed-off-by: Murali Karicheri
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g-evm.dts | 19 +++
1 file changed, 19 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g-evm.dts
b/arch/arm/boot/dts/keystone-k2g-evm.dts
index f462f10..a6ad5fc 100644
--- a/arch/arm/boot/dts/keystone-k2g-evm.dts
+++ b/arch/arm/boot/dts/keystone-k2g-evm.dts
@@ -81,6 +81,14 @@
K2G_CORE_IOPAD(0x1110) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* mmc1_cmd.mmc1_cmd */
>;
};
+
+ i2c0_pins: pinmux_i2c0_pins {
+ pinctrl-single,pins = <
+ K2G_CORE_IOPAD(0x137c) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* i2c0_scl.i2c0_scl */
+ K2G_CORE_IOPAD(0x1380) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* i2c0_sda.i2c0_sda */
+ >;
+ };
+
};
&uart0 {
@@ -112,3 +120,14 @@
memory-region = <&dsp_common_memory>;
status = "okay";
};
+
+&i2c0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c0_pins>;
+ status = "okay";
+
+ eeprom@50 {
+ compatible = "atmel,24c1024";
+ reg = <0x50>;
+ };
+};
--
2.9.4.dirty
[PATCH v2 0/2] ARM: dts: keystone-k2g-evm: Add I2C nodes and enable I2C0 on K2G EVM
Add I2C DT nodes for 66AK2G. Also enable I2C0 in K2G EVM which is needed to access the I2C EEPROM. Version 2 changes: No real changes. Split patches into their own patchset Murali Karicheri (1): ARM: dts: keystone-k2g-evm: Add I2C EEPROM DT entry Vitaly Andrianov (1): ARM: dts: keystone-k2g: Add I2C nodes arch/arm/boot/dts/keystone-k2g-evm.dts | 19 ++ arch/arm/boot/dts/keystone-k2g.dtsi| 36 ++ 2 files changed, 55 insertions(+) -- 2.9.4.dirty
[PATCH v2 1/2] ARM: dts: keystone-k2g: Add I2C nodes
From: Vitaly Andrianov
Add nodes for the various I2C instances.
Signed-off-by: Vitaly Andrianov
Signed-off-by: Franklin S Cooper Jr
Reviewed-by: Grygorii Strashko
---
arch/arm/boot/dts/keystone-k2g.dtsi | 36
1 file changed, 36 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index 826b286..8a98d76 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -28,6 +28,9 @@
aliases {
serial0 = &uart0;
+ i2c0 = &i2c0;
+ i2c1 = &i2c1;
+ i2c2 = &i2c2;
rproc0 = &dsp0;
};
@@ -133,6 +136,39 @@
clocks = <&k2g_clks 0x0009 1>;
};
+ i2c0: i2c@253 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x0253 0x400>;
+ clocks = <&k2g_clks 0x003a 0>;
+ power-domains = <&k2g_pds 0x003a>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c1: i2c@2530400 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x02530400 0x400>;
+ clocks = <&k2g_clks 0x003b 0>;
+ power-domains = <&k2g_pds 0x003b>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c2: i2c@2530800 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x02530800 0x400>;
+ clocks = <&k2g_clks 0x003c 0>;
+ power-domains = <&k2g_pds 0x003c>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
kirq0: keystone_irq@026202a0 {
compatible = "ti,keystone-irq";
interrupts = ;
--
2.9.4.dirty
[PATCH v3 0/2] ARM: dts: keystone-k2g: Add I2C support for 66AK2G
Add I2C support to 66AK2G. Primary requirement is to add PM Runtime support to the driver. This has been tested on following platforms by performing simple i2c test such as i2c detect and reading on board i2c devices: K2G GP evm OMAPL138 K2L GP EVM and boot tested on: K2E GP EVM K2HK GP EVM Version 2 changes: Moved ordering of pm runtime calls Version 3 changes: Dropped first patch since it was pulled in. Remove clk = NULL statements Fix error path Franklin S Cooper Jr (2): i2c: davinci: Add PM Runtime Support dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property .../devicetree/bindings/i2c/i2c-davinci.txt| 12 drivers/i2c/busses/i2c-davinci.c | 64 ++ 2 files changed, 65 insertions(+), 11 deletions(-) -- 2.9.4.dirty
[PATCH v3 1/2] i2c: davinci: Add PM Runtime Support
66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.
Signed-off-by: Franklin S Cooper Jr
---
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path
drivers/i2c/busses/i2c-davinci.c | 64 +---
1 file changed, 53 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..57612cb 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
/* - global defines --- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000/* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem*base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(&dev->xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,12 +821,22 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(&pdev->dev, mem);
if (IS_ERR(dev->base)) {
- r = PTR_ERR(dev->base);
+ return PTR_ERR(dev->base);
+ }
+
+ pm_runtime_set_autosuspend_delay(dev->dev,
+DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ pm_runtime_enable(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
goto err_unuse_clocks;
}
@@ -849,27 +878,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
return r;
}
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(&dev->adapter);
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&pdev->dev);
+ return ret;
+ }
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0);
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(de
[PATCH v3 2/2] dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr Acked-by: Rob Herring Acked-by: Sekhar Nori --- Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt index 5b123e0..64e6e65 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt @@ -6,6 +6,18 @@ davinci/keystone i2c interface contains. Required properties: - compatible: "ti,davinci-i2c" or "ti,keystone-i2c"; - reg : Offset and length of the register set for the device +- clocks: I2C functional clock phandle. + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the I2C device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Recommended properties : - interrupts : standard interrupt property. -- 2.9.4.dirty
Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support
On 08/23/2017 01:34 AM, Sekhar Nori wrote:
> On Monday 21 August 2017 10:29 PM, Franklin S Cooper Jr wrote:
>>
>>
>> On 08/21/2017 07:20 AM, Sekhar Nori wrote:
>>> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>>>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>>>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>>>> power domains needed by UART are on. Keep legacy clk api calls since other
>>>> users of this driver may not support PM runtime.
>>
>> There are a significant amount of users across a wide variety of
>> architectures and boards that I have no means to properly test to insure
>> I'm avoiding regressions. My current approach which I've seen other
>> drivers in the past use when in similar situations allows things to work
>> without regressions.
>
> Since the users are all device-tree users, my hope is they can be
> tracked by looking at the DT files and the maintainers be copied on this
> patch to make sure nothing breaks for them. I understand its not a
> trivial list, but its finite :)
>
>>>
>>> Do we really have users like that? And even if there are, cant they use
>>> PM clock handling support available in drivers/base/power/clock_ops.c ?
>>
>> I don't see any current defconfig that enables CONFIG_PM_CLK and only a
>
> Thats because its a non-user-selectable def_bool. Most configs should
> have it enabled after the 'make config' step.
>
>> handful of instances where functions from clock_ops.c are actually used.
>> I don't quite understand what your suggestion is but in general I'm
>> concerned since any approach to move everyone to different apis is
>> rather risky especially for a critical driver.
>>
>> If I'm missing your point please let me know.
>
> I do agree we dont want to break anyone. But at the same time, this
> patch redundantly enables/disables clock for most known (and possible
> future) users without no clear indication of who benefits from such
> redundancy.
>
> If we ignore this now, the problem will only get worse as time passes. I
> suggest biting the bullet now, attempt to reach-out as many affected
> parties as possible but switch to pm_runtime once and for all.
>
>>
>>>
>>> The clock enable support itself was added pretty "recently" - about 5
>>> years back with 0bbeb3c3e84b ("of serial port driver - add
>>> clk_get_rate() support"). So I doubt any really legacy platforms relied
>>> on clock support being there. It was added by Murali, I assume for
>>> Keystone devices. Keystone devices can work with runtime PM using the PM
>>> clock support pointed to above.
>>
>> You might be right but I can't be confident that it is indeed the case.
>> But its possible that at some point people will start having problems if
>> they try to use this driver for other UART instances. The currently
>> could not be aware of an issue because of the bootloader powering things
>> for them or even that different UART instances could be apart of a non
>> always on power domain.
>
> I dont think you got my point there. I have no disagreement that
> pm_runtime support is needed. In fact, it should have been that way when
> clock enable/disable was added. But thats history now.
>
>>> Perhaps linux-arm-kernel list should be copied on this submission too,
>>> since most users of this driver are likely to be there on that list.
>>>
>>
>> Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
>> bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
>> PowerPC, MIPS, Xtensa and Arc.>
>> Looking at dts that enable some of the compatible it is still a
>> combination of quite a bit of architectures.
>
> Right, I do think there are quite a few users. But they should still be
> reachable and provide testing inputs for the change.
>
>> The current approach I've taken should be safe for all users of this
>> driver which. I have no issue going another approach as long as its
>> understood that I'm fairly limited in what I can test when you take into
>> account the large number of users of this driver.
>
> Understood that you are limited by hardware you possess. But we can
> probably reach out to as many folks affected by this as possible so we
> get a fair chance at doing the "right thing" without breaking anyone.
I guess I don't understand the acceptable criteria when we can go
forward and make this change for something that impac
Re: [PATCH v2 2/3] i2c: davinci: Add PM Runtime Support
On 08/21/2017 04:05 AM, Sekhar Nori wrote:
> On Thursday 17 August 2017 03:47 AM, Franklin S Cooper Jr wrote:
>
>> @@ -802,7 +821,6 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> dev->clk = devm_clk_get(&pdev->dev, NULL);
>> if (IS_ERR(dev->clk))
>> return PTR_ERR(dev->clk);
>> -clk_prepare_enable(dev->clk);
>>
>> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> dev->base = devm_ioremap_resource(&pdev->dev, mem);
>> @@ -811,6 +829,18 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> goto err_unuse_clocks;
>
> This goto is wrong now. There is no need to unwind the pm_runtime setup
> on a devm_ioremap_resource() failure. You can just return error here.
Ok
>
>> }
>>
>> +pm_runtime_set_autosuspend_delay(dev->dev,
>> + DAVINCI_I2C_PM_TIMEOUT);
>> +pm_runtime_use_autosuspend(dev->dev);
>> +
>> +pm_runtime_enable(dev->dev);
>> +
>> +r = pm_runtime_get_sync(dev->dev);
>> +if (r < 0) {
>> +dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
>> +goto err_unuse_clocks;
>> +}
>> +
>> i2c_davinci_init(dev);
>>
>> r = devm_request_irq(&pdev->dev, dev->irq, i2c_davinci_isr, 0,
>> @@ -849,10 +879,16 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> if (r)
>> goto err_unuse_clocks;
>>
>> +pm_runtime_mark_last_busy(dev->dev);
>> +pm_runtime_put_autosuspend(dev->dev);
>> +
>> return 0;
>>
>> err_unuse_clocks:
>> -clk_disable_unprepare(dev->clk);
>> +pm_runtime_dont_use_autosuspend(dev->dev);
>> +pm_runtime_put_sync(dev->dev);
>> +pm_runtime_disable(dev->dev);
>> +
>> dev->clk = NULL;
>
> This null setting of clk seems quite bogus and can be cleaned-up.
Do you mean that I should just remove this line?
>
>> return r;
>> }
>
> Thanks,
> Sekhar
>
Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support
On 08/21/2017 07:20 AM, Sekhar Nori wrote:
> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>> power domains needed by UART are on. Keep legacy clk api calls since other
>> users of this driver may not support PM runtime.
There are a significant amount of users across a wide variety of
architectures and boards that I have no means to properly test to insure
I'm avoiding regressions. My current approach which I've seen other
drivers in the past use when in similar situations allows things to work
without regressions.
>
> Do we really have users like that? And even if there are, cant they use
> PM clock handling support available in drivers/base/power/clock_ops.c ?
I don't see any current defconfig that enables CONFIG_PM_CLK and only a
handful of instances where functions from clock_ops.c are actually used.
I don't quite understand what your suggestion is but in general I'm
concerned since any approach to move everyone to different apis is
rather risky especially for a critical driver.
If I'm missing your point please let me know.
>
> The clock enable support itself was added pretty "recently" - about 5
> years back with 0bbeb3c3e84b ("of serial port driver - add
> clk_get_rate() support"). So I doubt any really legacy platforms relied
> on clock support being there. It was added by Murali, I assume for
> Keystone devices. Keystone devices can work with runtime PM using the PM
> clock support pointed to above.
You might be right but I can't be confident that it is indeed the case.
But its possible that at some point people will start having problems if
they try to use this driver for other UART instances. The currently
could not be aware of an issue because of the bootloader powering things
for them or even that different UART instances could be apart of a non
always on power domain.
>
> Perhaps linux-arm-kernel list should be copied on this submission too,
> since most users of this driver are likely to be there on that list.
>
Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
PowerPC, MIPS, Xtensa and Arc.
Looking at dts that enable some of the compatible it is still a
combination of quite a bit of architectures.
The current approach I've taken should be safe for all users of this
driver which. I have no issue going another approach as long as its
understood that I'm fairly limited in what I can test when you take into
account the large number of users of this driver.
>>
>> Signed-off-by: Franklin S Cooper Jr
>
> Thanks,
> Sekhar
>
[PATCH v5 2/4] dt-bindings: can: can-transceiver: Document new binding
Add documentation to describe usage of the new can-transceiver binding.
This new binding is applicable for any CAN device therefore it exists as
its own document.
Signed-off-by: Franklin S Cooper Jr
Acked-by: Rob Herring
---
Version 5 changes:
Remove @ symbol from can-transceiver example
.../bindings/net/can/can-transceiver.txt | 24 ++
1 file changed, 24 insertions(+)
create mode 100644
Documentation/devicetree/bindings/net/can/can-transceiver.txt
diff --git a/Documentation/devicetree/bindings/net/can/can-transceiver.txt
b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
new file mode 100644
index 000..0011f53
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
@@ -0,0 +1,24 @@
+Generic CAN transceiver Device Tree binding
+--
+
+CAN transceiver typically limits the max speed in standard CAN and CAN FD
+modes. Typically these limitations are static and the transceivers themselves
+provide no way to detect this limitation at runtime. For this situation,
+the "can-transceiver" node can be used.
+
+Required Properties:
+ max-bitrate: a positive non 0 value that determines the max
+ speed that CAN/CAN-FD can run. Any other value
+ will be ignored.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+
+ can-transceiver {
+ max-bitrate = <500>;
+ };
+ ...
+};
--
2.9.4.dirty
[PATCH v5 4/4] can: m_can: Add call to of_can_transceiver
Add call to new generic functions that provides support via a binding to limit the arbitration rate and/or data rate imposed by the physical transceiver connected to the MCAN peripheral. Signed-off-by: Franklin S Cooper Jr --- drivers/net/can/m_can/m_can.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index f4947a7..f72116e 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1649,6 +1649,8 @@ static int m_can_plat_probe(struct platform_device *pdev) devm_can_led_init(dev); + of_can_transceiver(dev); + dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n", KBUILD_MODNAME, dev->irq, priv->version); -- 2.9.4.dirty
[PATCH v5 0/4] can: Support transceiver based bit rate limit
Add a new generic binding that CAN drivers can be used to specify the max bit rate supported by a transceiver. This is useful since in some instances since the maximum speeds may be limited by the transceiver used. However, transceivers may not provide a means to determine this limitation at runtime. Therefore, create a new binding that mimics "fixed-link" that allows a user to hardcode the max speeds that can be used. Also add support for this new binding in the MCAN driver. Note this is an optional subnode so even if a driver adds support for parsing can-transceiver the user does not have to define it in their device tree. Version 5 changes: Got rid of is_bitrate_limited Removed @ symbol from can-transceiver binding Version 4 changes: Switch from fixed-transceiver to can-transceiver Drop unit address that snuck back in again. Indicate that can-transceiver is a subnode and not a property in documentation Version 3 changes: Switch from having two "max bitrates" to one universal bitrate. Version 2 changes: Rename function Define proper variable default Drop unit address Move check to changelink function Reword commit message Reword documentation Franklin S Cooper Jr (4): can: dev: Add support for limiting configured bitrate dt-bindings: can: can-transceiver: Document new binding dt-bindings: can: m_can: Document new can transceiver binding can: m_can: Add call to of_can_transceiver .../bindings/net/can/can-transceiver.txt | 24 + .../devicetree/bindings/net/can/m_can.txt | 9 + drivers/net/can/dev.c | 39 ++ drivers/net/can/m_can/m_can.c | 2 ++ include/linux/can/dev.h| 4 +++ 5 files changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt -- 2.9.4.dirty
[PATCH v5 3/4] dt-bindings: can: m_can: Document new can transceiver binding
Add information regarding can-transceiver binding. This is especially
important for MCAN since the IP allows CAN FD mode to run significantly
faster than what most transceivers are capable of.
Signed-off-by: Franklin S Cooper Jr
Acked-by: Rob Herring
---
Remove @ symbol from can-transceiver example
Documentation/devicetree/bindings/net/can/m_can.txt | 9 +
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
b/Documentation/devicetree/bindings/net/can/m_can.txt
index 9e33177..0bdae2a 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -43,6 +43,11 @@ Required properties:
Please refer to 2.4.1 Message RAM Configuration in
Bosch M_CAN user manual for details.
+Optional Subnode:
+- can-transceiver : Can-transceiver subnode describing maximum speed
+ that can be used for CAN/CAN-FD modes. See
+
Documentation/devicetree/bindings/net/can/can-transceiver.txt
+ for details.
Example:
SoC dtsi:
m_can1: can@020e8000 {
@@ -64,4 +69,8 @@ Board dts:
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_m_can1>;
status = "enabled";
+
+ can-transceiver {
+ max-bitrate = <500>;
+ };
};
--
2.9.4.dirty
[PATCH v5 1/4] can: dev: Add support for limiting configured bitrate
Various CAN or CAN-FD IP may be able to run at a faster rate than
what the transceiver the CAN node is connected to. This can lead to
unexpected errors. However, CAN transceivers typically have fixed
limitations and provide no means to discover these limitations at
runtime. Therefore, add support for a can-transceiver node that
can be reused by other CAN peripheral drivers to determine for both
CAN and CAN-FD what the max bitrate that can be used. If the user
tries to configure CAN to pass these maximum bitrates it will throw
an error.
Signed-off-by: Franklin S Cooper Jr
---
Version 5 changes:
Set values for some variables at the very top.
Remove usage of is_bitrate_limited
drivers/net/can/dev.c | 39 +++
include/linux/can/dev.h | 4
2 files changed, 43 insertions(+)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..d0e5b46 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#define MOD_DESC "CAN device driver interface"
@@ -814,6 +815,30 @@ int open_candev(struct net_device *dev)
}
EXPORT_SYMBOL_GPL(open_candev);
+#ifdef CONFIG_OF
+/*
+ * Common function that can be used to understand the limitation of
+ * a transceiver when it provides no means to determine these limitations
+ * at runtime.
+ */
+void of_can_transceiver(struct net_device *dev)
+{
+ struct device_node *dn;
+ struct can_priv *priv = netdev_priv(dev);
+ struct device_node *np = dev->dev.parent->of_node;
+ int ret;
+
+ dn = of_get_child_by_name(np, "can-transceiver");
+ if (!dn)
+ return;
+
+ ret = of_property_read_u32(dn, "max-bitrate", &priv->max_bitrate);
+ if ((ret && ret != -EINVAL) || (!ret && !priv->max_bitrate))
+ netdev_warn(dev, "Invalid value for transceiver max bitrate.
Ignoring bitrate limit.\n");
+}
+EXPORT_SYMBOL(of_can_transceiver);
+#endif
+
/*
* Common close function for cleanup before the device gets closed.
*
@@ -913,6 +938,13 @@ static int can_changelink(struct net_device *dev, struct
nlattr *tb[],
priv->bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->max_bitrate && bt.bitrate > priv->max_bitrate) {
+ netdev_err(dev, "arbitration bitrate surpasses
transceiver capabilities of %d bps\n",
+ priv->max_bitrate);
+ return -EINVAL;
+ }
+
memcpy(&priv->bittiming, &bt, sizeof(bt));
if (priv->do_set_bittiming) {
@@ -997,6 +1029,13 @@ static int can_changelink(struct net_device *dev, struct
nlattr *tb[],
priv->data_bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->max_bitrate && dbt.bitrate > priv->max_bitrate) {
+ netdev_err(dev, "canfd data bitrate surpasses
transceiver capabilities of %d bps\n",
+ priv->max_bitrate);
+ return -EINVAL;
+ }
+
memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
if (priv->do_set_data_bittiming) {
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 141b05a..0063c51 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -47,6 +47,8 @@ struct can_priv {
unsigned int data_bitrate_const_cnt;
struct can_clock clock;
+ unsigned int max_bitrate;
+
enum can_state state;
/* CAN controller features - see include/uapi/linux/can/netlink.h */
@@ -165,6 +167,8 @@ void can_put_echo_skb(struct sk_buff *skb, struct
net_device *dev,
unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
void can_free_echo_skb(struct net_device *dev, unsigned int idx);
+void of_can_transceiver(struct net_device *dev);
+
struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
struct sk_buff *alloc_canfd_skb(struct net_device *dev,
struct canfd_frame **cfd);
--
2.9.4.dirty
[RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
During test transmitting using CAN-FD at high bitrates (4 Mbps) only
resulted in errors. Scoping the signals I noticed that only a single bit
was being transmitted and with a bit more investigation realized the actual
MCAN IP would go back to initialization mode automatically.
It appears this issue is due to the MCAN needing to use the Transmitter
Delay Compensation Mode as defined in the MCAN User's Guide. When this
mode is used the User's Guide indicates that the Transmitter Delay
Compensation Offset register should be set. The document mentions that this
register should be set to (1/dbitrate)/2*(Func Clk Freq).
Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
that this TDC mode is only needed for data bit rates above 2.5 Mbps.
Therefore, only enable this mode and only set TDCO when the data bit rate
is above 2.5 Mbps.
Signed-off-by: Franklin S Cooper Jr
---
I'm pretty surprised that this hasn't been implemented already since
the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
supports up to 10 Mbps.
So it will be nice to get comments from users of this driver to understand
if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
If they haven't what did they do to get around it if they needed higher
speeds.
Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
everything still works at 5 Mbps which is the max speed of my CAN
transceiver.
drivers/net/can/m_can/m_can.c | 24 +++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f4947a7..720e073 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -126,6 +126,12 @@ enum m_can_mram_cfg {
#define DBTP_DSJW_SHIFT0
#define DBTP_DSJW_MASK (0xf << DBTP_DSJW_SHIFT)
+/* Transmitter Delay Compensation Register (TDCR) */
+#define TDCR_TDCO_SHIFT8
+#define TDCR_TDCO_MASK (0x7F << TDCR_TDCO_SHIFT)
+#define TDCR_TDCF_SHIFT0
+#define TDCR_TDCF_MASK (0x7F << TDCR_TDCO_SHIFT)
+
/* Test Register (TEST) */
#define TEST_LBCK BIT(4)
@@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
const struct can_bittiming *dbt = &priv->can.data_bittiming;
u16 brp, sjw, tseg1, tseg2;
u32 reg_btp;
+ u32 enable_tdc = 0;
+ u32 tdco;
brp = bt->brp - 1;
sjw = bt->sjw - 1;
@@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
sjw = dbt->sjw - 1;
tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
tseg2 = dbt->phase_seg2 - 1;
+
+ /* TDC is only needed for bitrates beyond 2.5 MBit/s
+* Specified in the "Bit Time Requirements for CAN FD" document
+*/
+ if (dbt->bitrate > 250) {
+ enable_tdc = DBTP_TDC;
+ /* Equation based on Bosch's M_CAN User Manual's
+* Transmitter Delay Compensation Section
+*/
+ tdco = priv->can.clock.freq / (dbt->bitrate * 2);
+ m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
+ }
+
reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
(tseg1 << DBTP_DTSEG1_SHIFT) |
- (tseg2 << DBTP_DTSEG2_SHIFT);
+ (tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
+
m_can_write(priv, M_CAN_DBTP, reg_btp);
}
--
2.9.4.dirty
Re: [PATCH v4 1/4] can: dev: Add support for limiting configured bitrate
Hi,
On 08/10/2017 10:29 AM, Marc Kleine-Budde wrote:
> Hello,
>
> sorry for stepping in late
>
> On 08/10/2017 02:59 AM, Franklin S Cooper Jr wrote:
>> Various CAN or CAN-FD IP may be able to run at a faster rate than
>> what the transceiver the CAN node is connected to. This can lead to
>> unexpected errors. However, CAN transceivers typically have fixed
>> limitations and provide no means to discover these limitations at
>> runtime. Therefore, add support for a can-transceiver node that
>> can be reused by other CAN peripheral drivers to determine for both
>> CAN and CAN-FD what the max bitrate that can be used. If the user
>> tries to configure CAN to pass these maximum bitrates it will throw
>> an error.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Version 4 changes:
>> Used can-transceiver instead of fixed-transceiver.
>>
>> drivers/net/can/dev.c | 50
>> +
>> include/linux/can/dev.h | 5 +
>> 2 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 365a8cc..372108f 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -27,6 +27,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>>
>> #define MOD_DESC "CAN device driver interface"
>> @@ -814,6 +815,39 @@ int open_candev(struct net_device *dev)
>> }
>> EXPORT_SYMBOL_GPL(open_candev);
>>
>> +#ifdef CONFIG_OF
>> +/*
>> + * Common function that can be used to understand the limitation of
>> + * a transceiver when it provides no means to determine these limitations
>> + * at runtime.
>> + */
>> +void of_can_transceiver(struct net_device *dev)
>> +{
>> +struct device_node *dn;
>> +struct can_priv *priv = netdev_priv(dev);
>> +struct device_node *np;
>> +unsigned int max_bitrate;
>> +int ret;
>> +
>> +np = dev->dev.parent->of_node;
>> +
>> +dn = of_get_child_by_name(np, "can-transceiver");
>> +if (!dn)
>> +return;
>> +
>> +max_bitrate = 0;
>> +ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);
>
> IIRC the last argument is only modified in case of no error, so what about:
> ret = of_property_read_u32(dn, "max-bitrate",
> &priv->max_bitrate);
That works
>
>> +
>> +if (max_bitrate > 0) {
>> +priv->max_bitrate = max_bitrate;
>> +priv->is_bitrate_limited = true;
>
> Do we need is_bitrate_limited...
No.
>
>> +} else if (ret != -EINVAL) {
>> +netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
>> +}
>> +}
>> +EXPORT_SYMBOL(of_can_transceiver);
>> +#endif
>> +
>> /*
>> * Common close function for cleanup before the device gets closed.
>> *
>> @@ -913,6 +947,14 @@ static int can_changelink(struct net_device *dev,
>> struct nlattr *tb[],
>> priv->bitrate_const_cnt);
>> if (err)
>> return err;
>> +
>> +if (priv->is_bitrate_limited &&
>> +bt.bitrate > priv->max_bitrate) {
>
> ...can we just use priv->max_bitrate?
Yes
Thanks for reviewing and for your feedback.
>
> if (priv->max_bitrate && bt.bitrate > priv->max_bitrate)
>
>> +netdev_err(dev, "arbitration bitrate surpasses
>> transceiver capabilities of %d bps\n",
>> + priv->max_bitrate);
>> +return -EINVAL;
>> +}
>> +
>> memcpy(&priv->bittiming, &bt, sizeof(bt));
>>
>> if (priv->do_set_bittiming) {
>> @@ -997,6 +1039,14 @@ static int can_changelink(struct net_device *dev,
>> struct nlattr *tb[],
>> priv->data_bitrate_const_cnt);
>> if (err)
>> return err;
>> +
>> +if (priv->is_bitrate_limited &&
>> +dbt.bitrate > priv->max_bitrate) {
>> +netdev_err(dev, "canfd data bitrate surpasses
>> transceiver capabilities of %d bps\n",
>> + priv->max_bitrate);
>> +return -EINVAL;
>> +
Re: [PATCH v4 1/4] can: dev: Add support for limiting configured bitrate
On 08/10/2017 05:05 AM, Sergei Shtylyov wrote:
> Hello!
>
> On 8/10/2017 3:59 AM, Franklin S Cooper Jr wrote:
>
>> Various CAN or CAN-FD IP may be able to run at a faster rate than
>> what the transceiver the CAN node is connected to. This can lead to
>> unexpected errors. However, CAN transceivers typically have fixed
>> limitations and provide no means to discover these limitations at
>> runtime. Therefore, add support for a can-transceiver node that
>> can be reused by other CAN peripheral drivers to determine for both
>> CAN and CAN-FD what the max bitrate that can be used. If the user
>> tries to configure CAN to pass these maximum bitrates it will throw
>> an error.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Version 4 changes:
>> Used can-transceiver instead of fixed-transceiver.
>>
>> drivers/net/can/dev.c | 50
>> +
>> include/linux/can/dev.h | 5 +
>> 2 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 365a8cc..372108f 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
> [...]
>> @@ -814,6 +815,39 @@ int open_candev(struct net_device *dev)
>> }
>> EXPORT_SYMBOL_GPL(open_candev);
>> +#ifdef CONFIG_OF
>> +/*
>> + * Common function that can be used to understand the limitation of
>> + * a transceiver when it provides no means to determine these
>> limitations
>> + * at runtime.
>> + */
>> +void of_can_transceiver(struct net_device *dev)
>> +{
>> +struct device_node *dn;
>> +struct can_priv *priv = netdev_priv(dev);
>> +struct device_node *np;
>> +unsigned int max_bitrate;
>> +int ret;
>> +
>> +np = dev->dev.parent->of_node;
>
>I'd do that as an initializer.
Ok
>
>> +
>> +dn = of_get_child_by_name(np, "can-transceiver");
>> +if (!dn)
>> +return;
>> +
>> +max_bitrate = 0;
>> +ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);
>
>I'd initialize max_bitrate to 0 as iff of_property_read_u32() fails,
> it'll leave the variable unset...>
>> +
>> +if (max_bitrate > 0) {
>
>You risk checking unset variable here.
Four lines up I am already setting max_bitrate to a default value of 0.
>
>> +priv->max_bitrate = max_bitrate;
>> +priv->is_bitrate_limited = true;
>> +} else if (ret != -EINVAL) {
>> +netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
>> +}
>> +}
>> +EXPORT_SYMBOL(of_can_transceiver);
>> +#endif
>> +
>> /*
>>* Common close function for cleanup before the device gets closed.
>>*
> [...]
>
> MBR, Sergei
Re: [PATCH v2 1/2] usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver
On 08/17/2017 07:04 AM, Felipe Balbi wrote: > > Hi, > > Franklin S Cooper Jr writes: >> For 66AK2Gx there is a requirement to use PM Runtime to properly manage >> clocks and the power domains. Therefore, add PM runtime support. Remove >> legacy clock api's calls since other users of this driver worked without >> these clock apis calls. >> >> Signed-off-by: Franklin S Cooper Jr >> --- > > I already have the previous version of this in my tree. Seems like the > only difference was on binding document, right? Do I need to change > anything in my 'next' and/or 'testing/next' branches? Correct the binding document is the only difference. Both of your branches looks good. >
[PATCH v2 0/3] ARM: dts: keystone-k2g: Add I2C support for 66AK2G
Add I2C support to 66AK2G. Primary requirement is to add PM Runtime support to the driver. This has been tested on following platforms by performing simple i2c test such as i2c detect and reading on board i2c devices: K2G GP evm OMAPL138 K2L GP EVM and boot tested on: K2E GP EVM K2HK GP EVM Version 2 changes: Moved ordering of pm runtime calls Franklin S Cooper Jr (3): i2c: davinci: Preserve return value of devm_clk_get i2c: davinci: Add PM Runtime Support dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property .../devicetree/bindings/i2c/i2c-davinci.txt| 12 drivers/i2c/busses/i2c-davinci.c | 64 +++--- 2 files changed, 67 insertions(+), 9 deletions(-) -- 2.9.4.dirty
[PATCH v2 2/3] i2c: davinci: Add PM Runtime Support
66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.
Signed-off-by: Franklin S Cooper Jr
---
Version 2 changes:
Move initial calls to pm runtime autosuspend before pm_runtime_enable
drivers/i2c/busses/i2c-davinci.c | 62 ++--
1 file changed, 54 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..6b1930d 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
/* - global defines --- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000/* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem*base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(&dev->xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,7 +821,6 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(&pdev->dev, mem);
@@ -811,6 +829,18 @@ static int davinci_i2c_probe(struct platform_device *pdev)
goto err_unuse_clocks;
}
+ pm_runtime_set_autosuspend_delay(dev->dev,
+DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ pm_runtime_enable(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
+ goto err_unuse_clocks;
+ }
+
i2c_davinci_init(dev);
r = devm_request_irq(&pdev->dev, dev->irq, i2c_davinci_isr, 0,
@@ -849,10 +879,16 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
dev->clk = NULL;
return r;
}
@@ -860,16 +896,26 @@ static int davinci_i2c_probe(struct platform_device *pdev)
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(&dev->adapter);
- clk_disable_unprepare(dev->clk);
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&pdev->dev);
+ return ret;
+
[PATCH v2 3/3] dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr Acked-by: Rob Herring --- Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt index 5b123e0..64e6e65 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt @@ -6,6 +6,18 @@ davinci/keystone i2c interface contains. Required properties: - compatible: "ti,davinci-i2c" or "ti,keystone-i2c"; - reg : Offset and length of the register set for the device +- clocks: I2C functional clock phandle. + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the I2C device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Recommended properties : - interrupts : standard interrupt property. -- 2.9.4.dirty
[PATCH v2 1/3] i2c: davinci: Preserve return value of devm_clk_get
The i2c driver can run into driver dependency issues if its loaded before a clock driver it depends on. Therefore, EPROBE_DEFER may be returned by devm_clk_get and should be returned in probe to allow the kernel to reprobe the driver at a later time. This patch allows the error value returned by devm_clk_get to be passed through and not overwritten. Signed-off-by: Franklin S Cooper Jr Reviewed-by: Grygorii Strashko --- drivers/i2c/busses/i2c-davinci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index 33ca9a3..b8c4353 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -801,7 +801,7 @@ static int davinci_i2c_probe(struct platform_device *pdev) dev->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(dev->clk)) - return -ENODEV; + return PTR_ERR(dev->clk); clk_prepare_enable(dev->clk); mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); -- 2.9.4.dirty
[PATCH v2 2/2] dt-bindings: usb: keystone-usb: Update bindings pm and clocks properties
Update various properties to properly indicate their requirement depending on the SoC. Signed-off-by: Franklin S Cooper Jr --- Version 2: Clarify clock requirements in binding document Documentation/devicetree/bindings/usb/keystone-usb.txt | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/usb/keystone-usb.txt b/Documentation/devicetree/bindings/usb/keystone-usb.txt index 60527d3..2d1bef1 100644 --- a/Documentation/devicetree/bindings/usb/keystone-usb.txt +++ b/Documentation/devicetree/bindings/usb/keystone-usb.txt @@ -12,8 +12,21 @@ Required properties: MPU. - ranges: allows valid 1:1 translation between child's address space and parent's address space. - - clocks: Clock IDs array as required by the controller. - - clock-names: names of clocks correseponding to IDs in the clock property. + +SoC-specific Required Properties: +The following are mandatory properties for Keystone 2 66AK2HK, 66AK2L and 66AK2E +SoCs only: + +- clocks: Clock ID for USB functional clock. +- clock-names: Must be "usb". + + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the USB device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Sub-nodes: The dwc3 core should be added as subnode to Keystone DWC3 glue. -- 2.9.4.dirty
[PATCH v2 0/2] ARM: dts: k2g: Add support for USB instances on 66AK2G
Add support for 66AK2G usb instances. However, the driver needs to be updated to support PM_RUNTIME. This update has been validated to work on K2L and boot tested on K2HK and K2E. Version 2: Clarify clock properties requirements in binding document Franklin S Cooper Jr (2): usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver dt-bindings: usb: keystone-usb: Update bindings pm and clocks properties .../devicetree/bindings/usb/keystone-usb.txt | 17 +++-- drivers/usb/dwc3/dwc3-keystone.c | 22 ++ 2 files changed, 25 insertions(+), 14 deletions(-) -- 2.9.4.dirty
[PATCH v2 1/2] usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver
For 66AK2Gx there is a requirement to use PM Runtime to properly manage
clocks and the power domains. Therefore, add PM runtime support. Remove
legacy clock api's calls since other users of this driver worked without
these clock apis calls.
Signed-off-by: Franklin S Cooper Jr
---
drivers/usb/dwc3/dwc3-keystone.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
index 12ee23f..d2ed952 100644
--- a/drivers/usb/dwc3/dwc3-keystone.c
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -15,7 +15,6 @@
* GNU General Public License for more details.
*/
-#include
#include
#include
#include
@@ -23,6 +22,7 @@
#include
#include
#include
+#include
/* USBSS register offsets */
#define USBSS_REVISION 0x
@@ -41,7 +41,6 @@
struct dwc3_keystone {
struct device *dev;
- struct clk *clk;
void __iomem*usbss;
};
@@ -106,17 +105,13 @@ static int kdwc3_probe(struct platform_device *pdev)
if (IS_ERR(kdwc->usbss))
return PTR_ERR(kdwc->usbss);
- kdwc->clk = devm_clk_get(kdwc->dev, "usb");
- if (IS_ERR(kdwc->clk)) {
- dev_err(kdwc->dev, "unable to get usb clock\n");
- return PTR_ERR(kdwc->clk);
- }
+ pm_runtime_enable(kdwc->dev);
- error = clk_prepare_enable(kdwc->clk);
+ error = pm_runtime_get_sync(kdwc->dev);
if (error < 0) {
- dev_err(kdwc->dev, "unable to enable usb clock, error %d\n",
+ dev_err(kdwc->dev, "pm_runtime_get_sync failed, error %d\n",
error);
- return error;
+ goto err_irq;
}
irq = platform_get_irq(pdev, 0);
@@ -147,7 +142,8 @@ static int kdwc3_probe(struct platform_device *pdev)
err_core:
kdwc3_disable_irqs(kdwc);
err_irq:
- clk_disable_unprepare(kdwc->clk);
+ pm_runtime_put_sync(kdwc->dev);
+ pm_runtime_disable(kdwc->dev);
return error;
}
@@ -167,7 +163,9 @@ static int kdwc3_remove(struct platform_device *pdev)
kdwc3_disable_irqs(kdwc);
device_for_each_child(&pdev->dev, NULL, kdwc3_remove_core);
- clk_disable_unprepare(kdwc->clk);
+ pm_runtime_put_sync(kdwc->dev);
+ pm_runtime_disable(kdwc->dev);
+
platform_set_drvdata(pdev, NULL);
return 0;
--
2.9.4.dirty
[PATCH v3] serial: 8250_of: Add basic PM runtime support
66AK2G UART instances are not apart of the ALWAYS_ON power domain.
Therefore, pm_runtime calls must be made to properly insure the appropriate
power domains needed by UART are on. Keep legacy clk api calls since other
users of this driver may not support PM runtime.
Signed-off-by: Franklin S Cooper Jr
---
Version 3 changes:
Commit message updated
Drop extra parenthesis
Version 2 changes:
Fix build error
Build tested using allmodconfig
drivers/tty/serial/8250/8250_of.c | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_of.c
b/drivers/tty/serial/8250/8250_of.c
index 6c5a8ca..be90c16 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -65,6 +66,10 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
int ret;
memset(port, 0, sizeof *port);
+
+ pm_runtime_enable(&ofdev->dev);
+ pm_runtime_get_sync(&ofdev->dev);
+
if (of_property_read_u32(np, "clock-frequency", &clk)) {
/* Get clk rate through clk driver if present */
@@ -72,12 +77,13 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
if (IS_ERR(info->clk)) {
dev_warn(&ofdev->dev,
"clk or clock-frequency not defined\n");
- return PTR_ERR(info->clk);
+ ret = PTR_ERR(info->clk);
+ goto err_pmruntime;
}
ret = clk_prepare_enable(info->clk);
if (ret < 0)
- return ret;
+ goto err_pmruntime;
clk = clk_get_rate(info->clk);
}
@@ -170,8 +176,10 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
err_dispose:
irq_dispose_mapping(port->irq);
err_unprepare:
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ clk_disable_unprepare(info->clk);
+err_pmruntime:
+ pm_runtime_put_sync(&ofdev->dev);
+ pm_runtime_disable(&ofdev->dev);
return ret;
}
@@ -227,8 +235,9 @@ static int of_platform_serial_probe(struct platform_device
*ofdev)
return 0;
err_dispose:
irq_dispose_mapping(port8250.port.irq);
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ pm_runtime_put_sync(&ofdev->dev);
+ pm_runtime_disable(&ofdev->dev);
+ clk_disable_unprepare(info->clk);
err_free:
kfree(info);
return ret;
@@ -244,8 +253,9 @@ static int of_platform_serial_remove(struct platform_device
*ofdev)
serial8250_unregister_port(info->line);
reset_control_assert(info->rst);
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ pm_runtime_put_sync(&ofdev->dev);
+ pm_runtime_disable(&ofdev->dev);
+ clk_disable_unprepare(info->clk);
kfree(info);
return 0;
}
@@ -259,9 +269,10 @@ static int of_serial_suspend(struct device *dev)
serial8250_suspend_port(info->line);
- if (info->clk && (!uart_console(port) || console_suspend_enabled))
+ if (!uart_console(port) || console_suspend_enabled) {
+ pm_runtime_put_sync(dev);
clk_disable_unprepare(info->clk);
-
+ }
return 0;
}
@@ -271,8 +282,10 @@ static int of_serial_resume(struct device *dev)
struct uart_8250_port *port8250 = serial8250_get_port(info->line);
struct uart_port *port = &port8250->port;
- if (info->clk && (!uart_console(port) || console_suspend_enabled))
+ if (!uart_console(port) || console_suspend_enabled) {
+ pm_runtime_get_sync(dev);
clk_prepare_enable(info->clk);
+ }
serial8250_resume_port(info->line);
--
2.9.4.dirty
Re: [PATCH v2 3/4] can: m_can: Update documentation to mention new fixed transceiver binding
Hi Rob,
On 08/03/2017 12:07 PM, Rob Herring wrote:
> On Mon, Jul 24, 2017 at 06:05:20PM -0500, Franklin S Cooper Jr wrote:
>> Add information regarding fixed transceiver binding. This is especially
>> important for MCAN since the IP allows CAN FD mode to run significantly
>> faster than what most transceivers are capable of.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Version 2 changes:
>> Drop unit address
>>
>> Documentation/devicetree/bindings/net/can/m_can.txt | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>> index 9e33177..e4abd2c 100644
>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>> @@ -43,6 +43,11 @@ Required properties:
>>Please refer to 2.4.1 Message RAM Configuration in
>>Bosch M_CAN user manual for details.
>>
>> +Optional properties:
>> +- fixed-transceiver : Fixed-transceiver subnode describing maximum speed
>
> This is a node, not a property. Sub nodes should have their own section.
Fixed in my v4 that I just sent.
>
>> + that can be used for CAN and/or CAN-FD modes. See
>> +
>> Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>> + for details.
>> Example:
>> SoC dtsi:
>> m_can1: can@020e8000 {
>> @@ -64,4 +69,9 @@ Board dts:
>> pinctrl-names = "default";
>> pinctrl-0 = <&pinctrl_m_can1>;
>> status = "enabled";
>> +
>> +fixed-transceiver {
>> +max-arbitration-speed = <100>;
>> +max-data-speed = <500>;
>> +};
>> };
>> --
>> 2.10.0
>>
[PATCH v4 4/4] can: m_can: Add call to of_can_transceiver
Add call to new generic functions that provides support via a binding to limit the arbitration rate and/or data rate imposed by the physical transceiver connected to the MCAN peripheral. Signed-off-by: Franklin S Cooper Jr --- drivers/net/can/m_can/m_can.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index f4947a7..f72116e 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1649,6 +1649,8 @@ static int m_can_plat_probe(struct platform_device *pdev) devm_can_led_init(dev); + of_can_transceiver(dev); + dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n", KBUILD_MODNAME, dev->irq, priv->version); -- 2.9.4.dirty
[PATCH v4 0/4] can: Support transceiver based bit rate limit
Add a new generic binding that CAN drivers can be used to specify the max bit rate supported by a transceiver. This is useful since in some instances since the maximum speeds may be limited by the transceiver used. However, transceivers may not provide a means to determine this limitation at runtime. Therefore, create a new binding that mimics "fixed-link" that allows a user to hardcode the max speeds that can be used. Also add support for this new binding in the MCAN driver. Note this is an optional subnode so even if a driver adds support for parsing can-transceiver the user does not have to define it in their device tree. Version 4 changes: Switch from fixed-transceiver to can-transceiver Drop unit address that snuck back in again. Indicate that can-transceiver is a subnode and not a property in documentation Version 3 changes: Switch from having two "max bitrates" to one universal bitrate. Version 2 changes: Rename function Define proper variable default Drop unit address Move check to changelink function Reword commit message Reword documentation Franklin S Cooper Jr (4): can: dev: Add support for limiting configured bitrate dt-bindings: can: can-transceiver: Document new binding dt-bindings: can: m_can: Document new can transceiver binding can: m_can: Add call to of_can_transceiver .../bindings/net/can/can-transceiver.txt | 24 +++ .../devicetree/bindings/net/can/m_can.txt | 9 drivers/net/can/dev.c | 50 ++ drivers/net/can/m_can/m_can.c | 2 + include/linux/can/dev.h| 5 +++ 5 files changed, 90 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt -- 2.9.4.dirty
Re: [PATCH v3 2/4] dt-bindings: can: fixed-transceiver: Add new CAN fixed transceiver bindings
Hi Sergei,
On 08/03/2017 10:38 AM, Franklin S Cooper Jr wrote:
>
>
> On 08/03/2017 07:22 AM, Sergei Shtylyov wrote:
>> On 08/03/2017 12:48 PM, Franklin S Cooper Jr wrote:
>>
>>>>> Add documentation to describe usage of the new fixed transceiver
>>>>> binding.
>>>>> This new binding is applicable for any CAN device therefore it
>>>>> exists as
>>>>> its own document.
>>>>>
>>>>> Signed-off-by: Franklin S Cooper Jr
>>>>> ---
>>>>>.../bindings/net/can/fixed-transceiver.txt | 24
>>>>> ++
>>>>>1 file changed, 24 insertions(+)
>>>>>create mode 100644
>>>>> Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>>>>> b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>>>>> new file mode 100644
>>>>> index 000..2f58838b
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>>>>> @@ -0,0 +1,24 @@
>>>>> +Fixed transceiver Device Tree binding
>>>>> +--
>>>>> +
>>>>> +CAN transceiver typically limits the max speed in standard CAN and
>>>>> CAN FD
>>>>> +modes. Typically these limitations are static and the transceivers
>>>>> themselves
>>>>> +provide no way to detect this limitation at runtime. For this
>>>>> situation,
>>>>> +the "fixed-transceiver" node can be used.
>>>>> +
>>>>> +Required Properties:
>>>>> + max-bitrate:a positive non 0 value that determines the max
>>>>> +speed that CAN/CAN-FD can run. Any other value
>>>>> +will be ignored.
>>>>> +
>>>>> +Examples:
>>>>> +
>>>>> +Based on Texas Instrument's TCAN1042HGV CAN Transceiver
>>>>> +
>>>>> +m_can0 {
>>>>> +
>>>>> +fixed-transceiver@0 {
>>>>
>>>> The (after @) must only be specified if there's "reg"
>>>
>>> Sorry. Fixed this in my v2 and some how it came back. Will fix.
>>>
>>>> prop in the device node. Also, please name the node "can-transceiver@"
>>>> to be more in line with the DT spec. which requires generic node names.
>>>
>>> Its possible for future can transceivers drivers to be created. So I
>>
>>So what? Ah, you are using the node name to match in the CAN drivers...
>>
>>> thought including fixed was important to indicate that this is a "dumb"
>>> transceiver similar to "fixed-link".
>>
>>I'm not sure the "fixed-link" MAC subnode assumed any transceiver at
>> all...
>
> Your right. I wasn't trying to imply that it does. What I meant was that
> having a node named "can-transceiver" may be a bit confusing in the
> future if can transceiver drivers are created. Prefix of "fixed" atleast
> to me makes it clear that this is something unique or a generic
> transceiver with limitations. Similar to "fixed-link" which is for MACs
> not connected to MDIO managed phy. Calling this subnode
> "can-transceiver" to me would be like renaming "fixed-link" to "phy".
>
>>
>>> So would "fixed-can-transceiver" be
>>> ok or do you want to go with can-transceiver?
>>
>>I'm somewhat perplexed at this point...
>
> If my reasoning still didn't change your views then I'll make the switch.
I went ahead and made your suggested switch in my v4. Thanks for taking
the time to review this series.
>>
>> MBR, Sergei
[PATCH v4 2/4] dt-bindings: can: can-transceiver: Document new binding
Add documentation to describe usage of the new can-transceiver binding.
This new binding is applicable for any CAN device therefore it exists as
its own document.
Signed-off-by: Franklin S Cooper Jr
---
Version 4 changes:
Drop unit address.
Switch from using fixed-transceiver to can-transceiver
.../bindings/net/can/can-transceiver.txt | 24 ++
1 file changed, 24 insertions(+)
create mode 100644
Documentation/devicetree/bindings/net/can/can-transceiver.txt
diff --git a/Documentation/devicetree/bindings/net/can/can-transceiver.txt
b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
new file mode 100644
index 000..2c31dc0
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
@@ -0,0 +1,24 @@
+Generic CAN transceiver Device Tree binding
+--
+
+CAN transceiver typically limits the max speed in standard CAN and CAN FD
+modes. Typically these limitations are static and the transceivers themselves
+provide no way to detect this limitation at runtime. For this situation,
+the "can-transceiver" node can be used.
+
+Required Properties:
+ max-bitrate: a positive non 0 value that determines the max
+ speed that CAN/CAN-FD can run. Any other value
+ will be ignored.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+
+ can-transceiver@ {
+ max-bitrate = <500>;
+ };
+ ...
+};
--
2.9.4.dirty
[PATCH v4 3/4] dt-bindings: can: m_can: Document new can transceiver binding
Add information regarding can-transceiver binding. This is especially
important for MCAN since the IP allows CAN FD mode to run significantly
faster than what most transceivers are capable of.
Signed-off-by: Franklin S Cooper Jr
---
Drop unit address.
Switch from using fixed-transceiver to can-transceiver
Indicate that can-transceiver is an optional subnode not a property.
Documentation/devicetree/bindings/net/can/m_can.txt | 9 +
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
b/Documentation/devicetree/bindings/net/can/m_can.txt
index 9e33177..ee90aac 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -43,6 +43,11 @@ Required properties:
Please refer to 2.4.1 Message RAM Configuration in
Bosch M_CAN user manual for details.
+Optional Subnode:
+- can-transceiver : Can-transceiver subnode describing maximum speed
+ that can be used for CAN/CAN-FD modes. See
+
Documentation/devicetree/bindings/net/can/can-transceiver.txt
+ for details.
Example:
SoC dtsi:
m_can1: can@020e8000 {
@@ -64,4 +69,8 @@ Board dts:
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_m_can1>;
status = "enabled";
+
+ can-transceiver@ {
+ max-bitrate = <500>;
+ };
};
--
2.9.4.dirty
[PATCH v4 1/4] can: dev: Add support for limiting configured bitrate
Various CAN or CAN-FD IP may be able to run at a faster rate than
what the transceiver the CAN node is connected to. This can lead to
unexpected errors. However, CAN transceivers typically have fixed
limitations and provide no means to discover these limitations at
runtime. Therefore, add support for a can-transceiver node that
can be reused by other CAN peripheral drivers to determine for both
CAN and CAN-FD what the max bitrate that can be used. If the user
tries to configure CAN to pass these maximum bitrates it will throw
an error.
Signed-off-by: Franklin S Cooper Jr
---
Version 4 changes:
Used can-transceiver instead of fixed-transceiver.
drivers/net/can/dev.c | 50 +
include/linux/can/dev.h | 5 +
2 files changed, 55 insertions(+)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..372108f 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#define MOD_DESC "CAN device driver interface"
@@ -814,6 +815,39 @@ int open_candev(struct net_device *dev)
}
EXPORT_SYMBOL_GPL(open_candev);
+#ifdef CONFIG_OF
+/*
+ * Common function that can be used to understand the limitation of
+ * a transceiver when it provides no means to determine these limitations
+ * at runtime.
+ */
+void of_can_transceiver(struct net_device *dev)
+{
+ struct device_node *dn;
+ struct can_priv *priv = netdev_priv(dev);
+ struct device_node *np;
+ unsigned int max_bitrate;
+ int ret;
+
+ np = dev->dev.parent->of_node;
+
+ dn = of_get_child_by_name(np, "can-transceiver");
+ if (!dn)
+ return;
+
+ max_bitrate = 0;
+ ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);
+
+ if (max_bitrate > 0) {
+ priv->max_bitrate = max_bitrate;
+ priv->is_bitrate_limited = true;
+ } else if (ret != -EINVAL) {
+ netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
+ }
+}
+EXPORT_SYMBOL(of_can_transceiver);
+#endif
+
/*
* Common close function for cleanup before the device gets closed.
*
@@ -913,6 +947,14 @@ static int can_changelink(struct net_device *dev, struct
nlattr *tb[],
priv->bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->is_bitrate_limited &&
+ bt.bitrate > priv->max_bitrate) {
+ netdev_err(dev, "arbitration bitrate surpasses
transceiver capabilities of %d bps\n",
+ priv->max_bitrate);
+ return -EINVAL;
+ }
+
memcpy(&priv->bittiming, &bt, sizeof(bt));
if (priv->do_set_bittiming) {
@@ -997,6 +1039,14 @@ static int can_changelink(struct net_device *dev, struct
nlattr *tb[],
priv->data_bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->is_bitrate_limited &&
+ dbt.bitrate > priv->max_bitrate) {
+ netdev_err(dev, "canfd data bitrate surpasses
transceiver capabilities of %d bps\n",
+ priv->max_bitrate);
+ return -EINVAL;
+ }
+
memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
if (priv->do_set_data_bittiming) {
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 141b05a..5519f59 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -47,6 +47,9 @@ struct can_priv {
unsigned int data_bitrate_const_cnt;
struct can_clock clock;
+ unsigned int max_bitrate;
+ bool is_bitrate_limited;
+
enum can_state state;
/* CAN controller features - see include/uapi/linux/can/netlink.h */
@@ -165,6 +168,8 @@ void can_put_echo_skb(struct sk_buff *skb, struct
net_device *dev,
unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
void can_free_echo_skb(struct net_device *dev, unsigned int idx);
+void of_can_transceiver(struct net_device *dev);
+
struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
struct sk_buff *alloc_canfd_skb(struct net_device *dev,
struct canfd_frame **cfd);
--
2.9.4.dirty
Re: [PATCH 0/3] ARM: dts: keystone-k2g: Add DCAN instances to 66AK2G
Hi Santosh, On 08/04/2017 12:07 PM, Santosh Shilimkar wrote: > Hi Franklin, > > On 8/2/2017 1:18 PM, Franklin S Cooper Jr wrote: >> Add D CAN nodes to 66AK2G based SoC dtsi. >> >> Franklin S Cooper Jr (2): >>dt-bindings: net: c_can: Update binding for clock and power-domains >> property >>ARM: configs: keystone: Enable D_CAN driver >> >> Lokesh Vutla (1): >>ARM: dts: k2g: Add DCAN nodes >> > Any DCAN driver dependency with these patchset ? If not, I can > queue this up so do let me know. There aren't any dependencies. > > Regards, > Santosh
[PATCH v2] serial: 8250_of: Add basic PM runtime support
Add basic PM Runtime support.
Signed-off-by: Franklin S Cooper Jr
---
Version 2 changes:
Fix build error.
Build tested using allmodconfig
drivers/tty/serial/8250/8250_of.c | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_of.c
b/drivers/tty/serial/8250/8250_of.c
index 6c5a8ca..9bad8bae 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -65,6 +66,10 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
int ret;
memset(port, 0, sizeof *port);
+
+ pm_runtime_enable(&ofdev->dev);
+ pm_runtime_get_sync(&ofdev->dev);
+
if (of_property_read_u32(np, "clock-frequency", &clk)) {
/* Get clk rate through clk driver if present */
@@ -72,12 +77,13 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
if (IS_ERR(info->clk)) {
dev_warn(&ofdev->dev,
"clk or clock-frequency not defined\n");
- return PTR_ERR(info->clk);
+ ret = PTR_ERR(info->clk);
+ goto err_pmruntime;
}
ret = clk_prepare_enable(info->clk);
if (ret < 0)
- return ret;
+ goto err_pmruntime;
clk = clk_get_rate(info->clk);
}
@@ -170,8 +176,10 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
err_dispose:
irq_dispose_mapping(port->irq);
err_unprepare:
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ clk_disable_unprepare(info->clk);
+err_pmruntime:
+ pm_runtime_put_sync(&ofdev->dev);
+ pm_runtime_disable(&ofdev->dev);
return ret;
}
@@ -227,8 +235,9 @@ static int of_platform_serial_probe(struct platform_device
*ofdev)
return 0;
err_dispose:
irq_dispose_mapping(port8250.port.irq);
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ pm_runtime_put_sync(&ofdev->dev);
+ pm_runtime_disable(&ofdev->dev);
+ clk_disable_unprepare(info->clk);
err_free:
kfree(info);
return ret;
@@ -244,8 +253,9 @@ static int of_platform_serial_remove(struct platform_device
*ofdev)
serial8250_unregister_port(info->line);
reset_control_assert(info->rst);
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ pm_runtime_put_sync(&ofdev->dev);
+ pm_runtime_disable(&ofdev->dev);
+ clk_disable_unprepare(info->clk);
kfree(info);
return 0;
}
@@ -259,9 +269,10 @@ static int of_serial_suspend(struct device *dev)
serial8250_suspend_port(info->line);
- if (info->clk && (!uart_console(port) || console_suspend_enabled))
+ if ((!uart_console(port) || console_suspend_enabled)) {
+ pm_runtime_put_sync(dev);
clk_disable_unprepare(info->clk);
-
+ }
return 0;
}
@@ -271,8 +282,10 @@ static int of_serial_resume(struct device *dev)
struct uart_8250_port *port8250 = serial8250_get_port(info->line);
struct uart_port *port = &port8250->port;
- if (info->clk && (!uart_console(port) || console_suspend_enabled))
+ if ((!uart_console(port) || console_suspend_enabled)) {
+ pm_runtime_get_sync(dev);
clk_prepare_enable(info->clk);
+ }
serial8250_resume_port(info->line);
--
2.9.4.dirty
Re: [PATCH v3 2/4] dt-bindings: can: fixed-transceiver: Add new CAN fixed transceiver bindings
On 08/03/2017 07:22 AM, Sergei Shtylyov wrote:
> On 08/03/2017 12:48 PM, Franklin S Cooper Jr wrote:
>
>>>> Add documentation to describe usage of the new fixed transceiver
>>>> binding.
>>>> This new binding is applicable for any CAN device therefore it
>>>> exists as
>>>> its own document.
>>>>
>>>> Signed-off-by: Franklin S Cooper Jr
>>>> ---
>>>>.../bindings/net/can/fixed-transceiver.txt | 24
>>>> ++
>>>>1 file changed, 24 insertions(+)
>>>>create mode 100644
>>>> Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>>>> b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>>>> new file mode 100644
>>>> index 000..2f58838b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>>>> @@ -0,0 +1,24 @@
>>>> +Fixed transceiver Device Tree binding
>>>> +--
>>>> +
>>>> +CAN transceiver typically limits the max speed in standard CAN and
>>>> CAN FD
>>>> +modes. Typically these limitations are static and the transceivers
>>>> themselves
>>>> +provide no way to detect this limitation at runtime. For this
>>>> situation,
>>>> +the "fixed-transceiver" node can be used.
>>>> +
>>>> +Required Properties:
>>>> + max-bitrate:a positive non 0 value that determines the max
>>>> +speed that CAN/CAN-FD can run. Any other value
>>>> +will be ignored.
>>>> +
>>>> +Examples:
>>>> +
>>>> +Based on Texas Instrument's TCAN1042HGV CAN Transceiver
>>>> +
>>>> +m_can0 {
>>>> +
>>>> +fixed-transceiver@0 {
>>>
>>> The (after @) must only be specified if there's "reg"
>>
>> Sorry. Fixed this in my v2 and some how it came back. Will fix.
>>
>>> prop in the device node. Also, please name the node "can-transceiver@"
>>> to be more in line with the DT spec. which requires generic node names.
>>
>> Its possible for future can transceivers drivers to be created. So I
>
>So what? Ah, you are using the node name to match in the CAN drivers...
>
>> thought including fixed was important to indicate that this is a "dumb"
>> transceiver similar to "fixed-link".
>
>I'm not sure the "fixed-link" MAC subnode assumed any transceiver at
> all...
Your right. I wasn't trying to imply that it does. What I meant was that
having a node named "can-transceiver" may be a bit confusing in the
future if can transceiver drivers are created. Prefix of "fixed" atleast
to me makes it clear that this is something unique or a generic
transceiver with limitations. Similar to "fixed-link" which is for MACs
not connected to MDIO managed phy. Calling this subnode
"can-transceiver" to me would be like renaming "fixed-link" to "phy".
>
>> So would "fixed-can-transceiver" be
>> ok or do you want to go with can-transceiver?
>
>I'm somewhat perplexed at this point...
If my reasoning still didn't change your views then I'll make the switch.
>
> MBR, Sergei
Re: [PATCH v3 3/4] dt-bindings: can: m_can: Include reference to new fixed transceiver binding
On 08/03/2017 04:20 AM, Sergei Shtylyov wrote:
> On 8/3/2017 3:51 AM, Franklin S Cooper Jr wrote:
>
>> Add information regarding fixed transceiver binding. This is especially
>> important for MCAN since the IP allows CAN FD mode to run significantly
>> faster than what most transceivers are capable of.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Documentation/devicetree/bindings/net/can/m_can.txt | 9 +
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>> index 9e33177..0b62f47 100644
>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>> @@ -43,6 +43,11 @@ Required properties:
>> Please refer to 2.4.1 Message RAM Configuration in
>> Bosch M_CAN user manual for details.
>> +Optional properties:
>> +- fixed-transceiver: Fixed-transceiver subnode describing maximum
>> speed
>
>Subnode is not a property.
Ok makes sense. Several bindings refer to fixed-link as an optional
property. I thought there was some kind of weird reasoning to do so
which I decided to follow. I'll update it to say "Optional subnode".
>
>> + that can be used for CAN/CAN-FD modes. See
>> +
>> Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>> + for details.
>> Example:
>> SoC dtsi:
>> m_can1: can@020e8000 {
>> @@ -64,4 +69,8 @@ Board dts:
>> pinctrl-names = "default";
>> pinctrl-0 = <&pinctrl_m_can1>;
>> status = "enabled";
>> +
>> +fixed-transceiver@0 {
>
>Same comments here as in previous patch.
Will fix.
>
> [...]
>
> MBR, Sergei
Re: [PATCH v3 2/4] dt-bindings: can: fixed-transceiver: Add new CAN fixed transceiver bindings
On 08/03/2017 04:18 AM, Sergei Shtylyov wrote:
> Hello!
>
> On 8/3/2017 3:51 AM, Franklin S Cooper Jr wrote:
>
>> Add documentation to describe usage of the new fixed transceiver binding.
>> This new binding is applicable for any CAN device therefore it exists as
>> its own document.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> .../bindings/net/can/fixed-transceiver.txt | 24
>> ++
>> 1 file changed, 24 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>> b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>> new file mode 100644
>> index 000..2f58838b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>> @@ -0,0 +1,24 @@
>> +Fixed transceiver Device Tree binding
>> +--
>> +
>> +CAN transceiver typically limits the max speed in standard CAN and
>> CAN FD
>> +modes. Typically these limitations are static and the transceivers
>> themselves
>> +provide no way to detect this limitation at runtime. For this situation,
>> +the "fixed-transceiver" node can be used.
>> +
>> +Required Properties:
>> + max-bitrate:a positive non 0 value that determines the max
>> +speed that CAN/CAN-FD can run. Any other value
>> +will be ignored.
>> +
>> +Examples:
>> +
>> +Based on Texas Instrument's TCAN1042HGV CAN Transceiver
>> +
>> +m_can0 {
>> +
>> +fixed-transceiver@0 {
>
>The (after @) must only be specified if there's "reg"
Sorry. Fixed this in my v2 and some how it came back. Will fix.
> prop in the device node. Also, please name the node "can-transceiver@"
> to be more in line with the DT spec. which requires generic node names.
Its possible for future can transceivers drivers to be created. So I
thought including fixed was important to indicate that this is a "dumb"
transceiver similar to "fixed-link". So would "fixed-can-transceiver" be
ok or do you want to go with can-transceiver?
>
> [...]
>
> MBR, Sergei
[PATCH v3 4/4] can: m_can: Add call to of_can_transceiver_fixed
Add call to new generic functions that provides support via a binding to limit the arbitration rate and/or data rate imposed by the physical transceiver connected to the MCAN peripheral. Signed-off-by: Franklin S Cooper Jr --- drivers/net/can/m_can/m_can.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index f4947a7..bd75df1 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1649,6 +1649,8 @@ static int m_can_plat_probe(struct platform_device *pdev) devm_can_led_init(dev); + of_can_transceiver_fixed(dev); + dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n", KBUILD_MODNAME, dev->irq, priv->version); -- 2.9.4.dirty
[PATCH v3 1/4] can: dev: Add support for limiting configured bitrate
Various CAN or CAN-FD IP may be able to run at a faster rate than
what the transceiver the CAN node is connected to. This can lead to
unexpected errors. However, CAN transceivers typically have fixed
limitations and provide no means to discover these limitations at
runtime. Therefore, add support for a fixed-transceiver node that
can be reused by other CAN peripheral drivers to determine for both
CAN and CAN-FD what the max bitrate that can be used. If the user
tries to configure CAN to pass these maximum bitrates it will throw
an error.
Signed-off-by: Franklin S Cooper Jr
---
drivers/net/can/dev.c | 50 +
include/linux/can/dev.h | 5 +
2 files changed, 55 insertions(+)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..db56914 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#define MOD_DESC "CAN device driver interface"
@@ -814,6 +815,39 @@ int open_candev(struct net_device *dev)
}
EXPORT_SYMBOL_GPL(open_candev);
+#ifdef CONFIG_OF
+/*
+ * Common function that can be used to understand the limitation of
+ * a transceiver when it provides no means to determine these limitations
+ * at runtime.
+ */
+void of_can_transceiver_fixed(struct net_device *dev)
+{
+ struct device_node *dn;
+ struct can_priv *priv = netdev_priv(dev);
+ struct device_node *np;
+ unsigned int max_bitrate;
+ int ret;
+
+ np = dev->dev.parent->of_node;
+
+ dn = of_get_child_by_name(np, "fixed-transceiver");
+ if (!dn)
+ return;
+
+ max_bitrate = 0;
+ ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);
+
+ if (max_bitrate > 0) {
+ priv->max_bitrate = max_bitrate;
+ priv->is_bitrate_limited = true;
+ } else if (ret != -EINVAL) {
+ netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
+ }
+}
+EXPORT_SYMBOL(of_can_transceiver_fixed);
+#endif
+
/*
* Common close function for cleanup before the device gets closed.
*
@@ -913,6 +947,14 @@ static int can_changelink(struct net_device *dev, struct
nlattr *tb[],
priv->bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->is_bitrate_limited &&
+ bt.bitrate > priv->max_bitrate) {
+ netdev_err(dev, "arbitration bitrate surpasses
transceiver capabilities of %d bps\n",
+ priv->max_bitrate);
+ return -EINVAL;
+ }
+
memcpy(&priv->bittiming, &bt, sizeof(bt));
if (priv->do_set_bittiming) {
@@ -997,6 +1039,14 @@ static int can_changelink(struct net_device *dev, struct
nlattr *tb[],
priv->data_bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->is_bitrate_limited &&
+ dbt.bitrate > priv->max_bitrate) {
+ netdev_err(dev, "canfd data bitrate surpasses
transceiver capabilities of %d bps\n",
+ priv->max_bitrate);
+ return -EINVAL;
+ }
+
memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
if (priv->do_set_data_bittiming) {
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 141b05a..a6e62df 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -47,6 +47,9 @@ struct can_priv {
unsigned int data_bitrate_const_cnt;
struct can_clock clock;
+ unsigned int max_bitrate;
+ bool is_bitrate_limited;
+
enum can_state state;
/* CAN controller features - see include/uapi/linux/can/netlink.h */
@@ -165,6 +168,8 @@ void can_put_echo_skb(struct sk_buff *skb, struct
net_device *dev,
unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
void can_free_echo_skb(struct net_device *dev, unsigned int idx);
+void of_can_transceiver_fixed(struct net_device *dev);
+
struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
struct sk_buff *alloc_canfd_skb(struct net_device *dev,
struct canfd_frame **cfd);
--
2.9.4.dirty
[PATCH v3 3/4] dt-bindings: can: m_can: Include reference to new fixed transceiver binding
Add information regarding fixed transceiver binding. This is especially
important for MCAN since the IP allows CAN FD mode to run significantly
faster than what most transceivers are capable of.
Signed-off-by: Franklin S Cooper Jr
---
Documentation/devicetree/bindings/net/can/m_can.txt | 9 +
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
b/Documentation/devicetree/bindings/net/can/m_can.txt
index 9e33177..0b62f47 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -43,6 +43,11 @@ Required properties:
Please refer to 2.4.1 Message RAM Configuration in
Bosch M_CAN user manual for details.
+Optional properties:
+- fixed-transceiver: Fixed-transceiver subnode describing maximum speed
+ that can be used for CAN/CAN-FD modes. See
+
Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
+ for details.
Example:
SoC dtsi:
m_can1: can@020e8000 {
@@ -64,4 +69,8 @@ Board dts:
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_m_can1>;
status = "enabled";
+
+ fixed-transceiver@0 {
+ max-bitrate = <500>;
+ };
};
--
2.9.4.dirty
[PATCH v3 2/4] dt-bindings: can: fixed-transceiver: Add new CAN fixed transceiver bindings
Add documentation to describe usage of the new fixed transceiver binding.
This new binding is applicable for any CAN device therefore it exists as
its own document.
Signed-off-by: Franklin S Cooper Jr
---
.../bindings/net/can/fixed-transceiver.txt | 24 ++
1 file changed, 24 insertions(+)
create mode 100644
Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
diff --git a/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
new file mode 100644
index 000..2f58838b
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
@@ -0,0 +1,24 @@
+Fixed transceiver Device Tree binding
+--
+
+CAN transceiver typically limits the max speed in standard CAN and CAN FD
+modes. Typically these limitations are static and the transceivers themselves
+provide no way to detect this limitation at runtime. For this situation,
+the "fixed-transceiver" node can be used.
+
+Required Properties:
+ max-bitrate: a positive non 0 value that determines the max
+ speed that CAN/CAN-FD can run. Any other value
+ will be ignored.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+
+ fixed-transceiver@0 {
+ max-bitrate = <500>;
+ };
+ ...
+};
--
2.9.4.dirty
[PATCH v3 0/4] can: Support transceiver based bit rate limit
Add a new generic binding that CAN drivers can be used to specify the max bit rate supported by a transceiver. This is useful since in some instances since the maximum speeds may be limited by the transceiver used. However, transceivers may not provide a means to determine this limitation at runtime. Therefore, create a new binding that mimics "fixed-link" that allows a user to hardcode the max speeds that can be used. Also add support for this new binding in the MCAN driver. Note this is an optional subnode so even if a driver adds support for parsing fixed-transceiver the user does not have to define it in their device tree. Version 3 changes: Switch from having two "max bitrates" to one universal bitrate. Version 2 changes: Rename function Define proper variable default Drop unit address Move check to changelink function Reword commit message Reword documentation Franklin S Cooper Jr (4): can: dev: Add support for limiting configured bitrate dt-bindings: can: fixed-transceiver: Add new CAN fixed transceiver bindings dt-bindings: can: m_can: Include reference to new fixed transceiver binding can: m_can: Add call to of_can_transceiver_fixed .../bindings/net/can/fixed-transceiver.txt | 24 +++ .../devicetree/bindings/net/can/m_can.txt | 9 drivers/net/can/dev.c | 50 ++ drivers/net/can/m_can/m_can.c | 2 + include/linux/can/dev.h| 5 +++ 5 files changed, 90 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/can/fixed-transceiver.txt -- 2.9.4.dirty
[PATCH 2/4] i2c: davinci: Add PM Runtime Support
This patch adds PM runtime support to the I2C Davinci driver.
Reviewed-by: Grygorii Strashko
Signed-off-by: Franklin S Cooper Jr
Signed-off-by: Sekhar Nori
---
drivers/i2c/busses/i2c-davinci.c | 61 ++--
1 file changed, 53 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 5749aac..62ff869 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
/* - global defines --- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000/* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem*base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(&dev->xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,7 +821,6 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(&pdev->dev, mem);
@@ -811,6 +829,17 @@ static int davinci_i2c_probe(struct platform_device *pdev)
goto err_unuse_clocks;
}
+ pm_runtime_enable(dev->dev);
+ pm_runtime_set_autosuspend_delay(dev->dev,
+DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
+ goto err_unuse_clocks;
+ }
+
i2c_davinci_init(dev);
r = devm_request_irq(&pdev->dev, dev->irq, i2c_davinci_isr, 0,
@@ -849,10 +878,16 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
dev->clk = NULL;
return r;
}
@@ -860,16 +895,26 @@ static int davinci_i2c_probe(struct platform_device *pdev)
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(&dev->adapter);
- clk_disable_unprepare(dev->clk);
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&pdev->dev);
+ return ret;
+ }
+
dev->clk = NULL;
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0);
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev-&
[PATCH 3/4] dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr --- Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt index 5b123e0..64e6e65 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt @@ -6,6 +6,18 @@ davinci/keystone i2c interface contains. Required properties: - compatible: "ti,davinci-i2c" or "ti,keystone-i2c"; - reg : Offset and length of the register set for the device +- clocks: I2C functional clock phandle. + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the I2C device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Recommended properties : - interrupts : standard interrupt property. -- 2.9.4.dirty
[PATCH 4/4] ARM: dts: keystone-k2g: Add I2C nodes
From: Vitaly Andrianov
Add nodes for the various I2C instances.
Signed-off-by: Vitaly Andrianov
[d-gerl...@ti.com: Add power domain and clock properties]
Signed-off-by: Dave Gerlach
[fcoo...@ti.com: Update subject and commit message]
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g.dtsi | 36
1 file changed, 36 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index bf4d1fa..5c0acf37 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -27,6 +27,9 @@
aliases {
serial0 = &uart0;
+ i2c0 = &i2c0;
+ i2c1 = &i2c1;
+ i2c2 = &i2c2;
};
cpus {
@@ -113,6 +116,39 @@
status = "disabled";
};
+ i2c0: i2c@253 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x0253 0x400>;
+ clocks = <&k2g_clks 0x003a 0>;
+ power-domains = <&k2g_pds 0x003a>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c1: i2c@2530400 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x02530400 0x400>;
+ clocks = <&k2g_clks 0x003b 0>;
+ power-domains = <&k2g_pds 0x003b>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c2: i2c@2530800 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x02530800 0x400>;
+ clocks = <&k2g_clks 0x003c 0>;
+ power-domains = <&k2g_pds 0x003c>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
kirq0: keystone_irq@026202a0 {
compatible = "ti,keystone-irq";
interrupts = ;
--
2.9.4.dirty
[PATCH 1/4] i2c: davinci: Preserve return value of devm_clk_get
The i2c driver can run into driver dependency issues if its loaded before a clock driver it depends on. Therefore, EPROBE_DEFER may be returned by devm_clk_get and should be returned in probe to allow the kernel to reprobe the driver at a later time. This patch allows the error value returned by devm_clk_get to be passed through and not overwritten. Reviewed-by: Grygorii Strashko Signed-off-by: Franklin S Cooper Jr Signed-off-by: Sekhar Nori --- drivers/i2c/busses/i2c-davinci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index 9e7ef5c..5749aac 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -801,7 +801,7 @@ static int davinci_i2c_probe(struct platform_device *pdev) dev->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(dev->clk)) - return -ENODEV; + return PTR_ERR(dev->clk); clk_prepare_enable(dev->clk); mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); -- 2.9.4.dirty
[PATCH 0/4] ARM: dts: keystone-k2g: Add I2C support for 66AK2G
Add I2C support to 66AK2G. Primary requirement is to add PM Runtime support to the driver. This has been tested on following platforms by performing simple i2c test such as i2c detect and reading on board i2c devices: K2G GP evm OMAPL138 K2L GP EVM and boot tested on: K2E GP EVM K2HK GP EVM Franklin S Cooper Jr (3): i2c: davinci: Preserve return value of devm_clk_get i2c: davinci: Add PM Runtime Support dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property Vitaly Andrianov (1): ARM: dts: keystone-k2g: Add I2C nodes .../devicetree/bindings/i2c/i2c-davinci.txt| 12 + arch/arm/boot/dts/keystone-k2g.dtsi| 36 + drivers/i2c/busses/i2c-davinci.c | 63 ++ 3 files changed, 102 insertions(+), 9 deletions(-) -- 2.9.4.dirty
[PATCH 3/3] ARM: configs: keystone: Enable D_CAN driver
Enable C_CAN/D_CAN driver supported by 66AK2G Signed-off-by: Franklin S Cooper Jr --- arch/arm/configs/keystone_defconfig | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm/configs/keystone_defconfig b/arch/arm/configs/keystone_defconfig index d47ea43..47be99e 100644 --- a/arch/arm/configs/keystone_defconfig +++ b/arch/arm/configs/keystone_defconfig @@ -112,6 +112,9 @@ CONFIG_IP_NF_ARP_MANGLE=y CONFIG_IP6_NF_IPTABLES=m CONFIG_IP_SCTP=y CONFIG_VLAN_8021Q=y +CONFIG_CAN=m +CONFIG_CAN_C_CAN=m +CONFIG_CAN_C_CAN_PLATFORM=m CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug" CONFIG_DEVTMPFS=y CONFIG_DEVTMPFS_MOUNT=y -- 2.9.4.dirty
[PATCH 2/3] ARM: dts: k2g: Add DCAN nodes
From: Lokesh Vutla
Add nodes for the two DCAN instances included in 66AK2G
Signed-off-by: Lokesh Vutla
[d-gerl...@ti.com: add power-domains and clock information]
Signed-off-by: Dave Gerlach
[fcoo...@ti.com: update subject and commit message. Misc minor updates]
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g.dtsi | 18 ++
1 file changed, 18 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index bf4d1fa..bebc857 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -113,6 +113,24 @@
status = "disabled";
};
+ dcan0: can@0260B200 {
+ compatible = "ti,am4372-d_can", "ti,am3352-d_can";
+ reg = <0x0260B200 0x200>;
+ interrupts = ;
+ status = "disabled";
+ power-domains = <&k2g_pds 0x0008>;
+ clocks = <&k2g_clks 0x0008 1>;
+ };
+
+ dcan1: can@0260B400 {
+ compatible = "ti,am4372-d_can", "ti,am3352-d_can";
+ reg = <0x0260B400 0x200>;
+ interrupts = ;
+ status = "disabled";
+ power-domains = <&k2g_pds 0x0009>;
+ clocks = <&k2g_clks 0x0009 1>;
+ };
+
kirq0: keystone_irq@026202a0 {
compatible = "ti,keystone-irq";
interrupts = ;
--
2.9.4.dirty
[PATCH 2/3] dt-bindings: usb: keystone-usb: Update bindings pm and clocks properties
Update varous properties to properly indicate their requirement depending on the SoC. Signed-off-by: Franklin S Cooper Jr --- Documentation/devicetree/bindings/usb/keystone-usb.txt | 18 -- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/usb/keystone-usb.txt b/Documentation/devicetree/bindings/usb/keystone-usb.txt index 60527d3..307531b 100644 --- a/Documentation/devicetree/bindings/usb/keystone-usb.txt +++ b/Documentation/devicetree/bindings/usb/keystone-usb.txt @@ -12,8 +12,22 @@ Required properties: MPU. - ranges: allows valid 1:1 translation between child's address space and parent's address space. - - clocks: Clock IDs array as required by the controller. - - clock-names: names of clocks correseponding to IDs in the clock property. + +SoC-specific Required Properties: +The following are mandatory properties for Keystone 2 66AK2HK, 66AK2L and 66AK2E +SoCs only: + +- clocks: Clock IDs array as required by the controller. +- clock-names: names of clocks correseponding to IDs in the clock + property. + + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the USB device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Sub-nodes: The dwc3 core should be added as subnode to Keystone DWC3 glue. -- 2.9.4.dirty
[PATCH 0/3] ARM: dts: keystone-k2g: Add DCAN instances to 66AK2G
Add D CAN nodes to 66AK2G based SoC dtsi. Franklin S Cooper Jr (2): dt-bindings: net: c_can: Update binding for clock and power-domains property ARM: configs: keystone: Enable D_CAN driver Lokesh Vutla (1): ARM: dts: k2g: Add DCAN nodes Documentation/devicetree/bindings/net/can/c_can.txt | 13 - arch/arm/boot/dts/keystone-k2g.dtsi | 18 ++ arch/arm/configs/keystone_defconfig | 3 +++ 3 files changed, 33 insertions(+), 1 deletion(-) -- 2.9.4.dirty
[PATCH 1/3] dt-bindings: net: c_can: Update binding for clock and power-domains property
CAN driver uses the clk_get_rate call to determine the frequency of the functional clock. OMAP based SoCs do not require the clock property since hwmod already handles creating a "fck" clock thats accessible to drivers. However, this isn't the case for 66AK2G which makes the clocks property require for that SoC. 66AK2G requires a new property. Therefore, update the binding to also make this property requirement clear. Also clarify that for OMAP based SoCs ti,hwmod is a required property. Signed-off-by: Franklin S Cooper Jr --- Documentation/devicetree/bindings/net/can/c_can.txt | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt index 5a1d8b0..2d50425 100644 --- a/Documentation/devicetree/bindings/net/can/c_can.txt +++ b/Documentation/devicetree/bindings/net/can/c_can.txt @@ -11,9 +11,20 @@ Required properties: - interrupts : property with a value describing the interrupt number -Optional properties: +The following are mandatory properties for DRA7x, AM33xx and AM43xx SoCs only: - ti,hwmods: Must be "d_can" or "c_can", n being the instance number + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the DCAN device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt +- clocks : CAN functional clock phandle. This property is as per the + binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +Optional properties: - syscon-raminit : Handle to system control region that contains the RAMINIT register, register offset to the RAMINIT register and the CAN instance number (0 offset). -- 2.9.4.dirty
[PATCH 0/3] ARM: dts: k2g: Add support for USB instances on 66AK2G
Add support for 66AK2G usb instances. However, the driver needs to be updated to support PM_RUNTIME. This update has been validated to work on K2L and boot tested on K2HK and K2E. Franklin S Cooper Jr (2): usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver dt-bindings: usb: keystone-usb: Update bindings pm and clocks properties Vitaly Andrianov (1): ARM: dts: k2g: Add USB instances .../devicetree/bindings/usb/keystone-usb.txt | 18 ++- arch/arm/boot/dts/keystone-k2g.dtsi| 56 ++ drivers/usb/dwc3/dwc3-keystone.c | 22 - 3 files changed, 82 insertions(+), 14 deletions(-) -- 2.9.4.dirty
[PATCH 3/3] ARM: dts: k2g: Add USB instances
From: Vitaly Andrianov
Add nodes for both USB instances supported by 66AK2G.
[d-gerl...@ti.com: Add power domain and clock properties]
Signed-off-by: Dave Gerlach
[fcoo...@ti.com: Update commit message and subject]
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g.dtsi | 56 +
1 file changed, 56 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index bf4d1fa..b206b69 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -168,5 +168,61 @@
#reset-cells = <2>;
};
};
+
+ usb0_phy: usb-phy@0 {
+ compatible = "usb-nop-xceiv";
+ status = "disabled";
+ };
+
+ keystone_usb0: keystone-dwc3@268 {
+ compatible = "ti,keystone-dwc3";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x268 0x1>;
+ interrupts = ;
+ ranges;
+ dma-coherent;
+ dma-ranges;
+ status = "disabled";
+ power-domains = <&k2g_pds 0x0016>;
+
+ usb0: usb@269 {
+ compatible = "snps,dwc3";
+ reg = <0x269 0x1>;
+ interrupts = ;
+ maximum-speed = "high-speed";
+ dr_mode = "otg";
+ usb-phy = <&usb0_phy>;
+ status = "disabled";
+ };
+ };
+
+ usb1_phy: usb-phy@1 {
+ compatible = "usb-nop-xceiv";
+ status = "disabled";
+ };
+
+ keystone_usb1: keystone-dwc3@258 {
+ compatible = "ti,keystone-dwc3";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x258 0x1>;
+ interrupts = ;
+ ranges;
+ dma-coherent;
+ dma-ranges;
+ status = "disabled";
+ power-domains = <&k2g_pds 0x0017>;
+
+ usb1: usb@259 {
+ compatible = "snps,dwc3";
+ reg = <0x259 0x1>;
+ interrupts = ;
+ maximum-speed = "high-speed";
+ dr_mode = "otg";
+ usb-phy = <&usb1_phy>;
+ status = "disabled";
+ };
+ };
};
};
--
2.9.4.dirty
[PATCH 1/3] usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver
For 66AK2Gx there is a requirement to use PM Runtime to properly manage
clocks and the power domains. Therefore, add PM runtime support. Remove
legacy clock api's calls since other users of this driver worked without
these clock apis calls.
Signed-off-by: Franklin S Cooper Jr
Signed-off-by: Sekhar Nori
---
drivers/usb/dwc3/dwc3-keystone.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
index 12ee23f..d2ed952 100644
--- a/drivers/usb/dwc3/dwc3-keystone.c
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -15,7 +15,6 @@
* GNU General Public License for more details.
*/
-#include
#include
#include
#include
@@ -23,6 +22,7 @@
#include
#include
#include
+#include
/* USBSS register offsets */
#define USBSS_REVISION 0x
@@ -41,7 +41,6 @@
struct dwc3_keystone {
struct device *dev;
- struct clk *clk;
void __iomem*usbss;
};
@@ -106,17 +105,13 @@ static int kdwc3_probe(struct platform_device *pdev)
if (IS_ERR(kdwc->usbss))
return PTR_ERR(kdwc->usbss);
- kdwc->clk = devm_clk_get(kdwc->dev, "usb");
- if (IS_ERR(kdwc->clk)) {
- dev_err(kdwc->dev, "unable to get usb clock\n");
- return PTR_ERR(kdwc->clk);
- }
+ pm_runtime_enable(kdwc->dev);
- error = clk_prepare_enable(kdwc->clk);
+ error = pm_runtime_get_sync(kdwc->dev);
if (error < 0) {
- dev_err(kdwc->dev, "unable to enable usb clock, error %d\n",
+ dev_err(kdwc->dev, "pm_runtime_get_sync failed, error %d\n",
error);
- return error;
+ goto err_irq;
}
irq = platform_get_irq(pdev, 0);
@@ -147,7 +142,8 @@ static int kdwc3_probe(struct platform_device *pdev)
err_core:
kdwc3_disable_irqs(kdwc);
err_irq:
- clk_disable_unprepare(kdwc->clk);
+ pm_runtime_put_sync(kdwc->dev);
+ pm_runtime_disable(kdwc->dev);
return error;
}
@@ -167,7 +163,9 @@ static int kdwc3_remove(struct platform_device *pdev)
kdwc3_disable_irqs(kdwc);
device_for_each_child(&pdev->dev, NULL, kdwc3_remove_core);
- clk_disable_unprepare(kdwc->clk);
+ pm_runtime_put_sync(kdwc->dev);
+ pm_runtime_disable(kdwc->dev);
+
platform_set_drvdata(pdev, NULL);
return 0;
--
2.9.4.dirty
[PATCH] serial: 8250_of: Add basic PM runtime support
Add basic PM Runtime support.
Signed-off-by: Franklin S Cooper Jr
---
drivers/tty/serial/8250/8250_of.c | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_of.c
b/drivers/tty/serial/8250/8250_of.c
index 6c5a8ca..94e7c93 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -65,6 +66,10 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
int ret;
memset(port, 0, sizeof *port);
+
+ pm_runtime_enable(&ofdev->dev);
+ pm_runtime_get_sync(&ofdev->dev);
+
if (of_property_read_u32(np, "clock-frequency", &clk)) {
/* Get clk rate through clk driver if present */
@@ -72,12 +77,13 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
if (IS_ERR(info->clk)) {
dev_warn(&ofdev->dev,
"clk or clock-frequency not defined\n");
- return PTR_ERR(info->clk);
+ ret = PTR_ERR(info->clk);
+ goto err_pmruntime;
}
ret = clk_prepare_enable(info->clk);
if (ret < 0)
- return ret;
+ goto err_pmruntime;
clk = clk_get_rate(info->clk);
}
@@ -170,8 +176,10 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
err_dispose:
irq_dispose_mapping(port->irq);
err_unprepare:
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ clk_disable_unprepare(info->clk);
+err_pmruntime:
+ pm_runtime_put_sync(&ofdev->dev);
+ pm_runtime_disable(&ofdev->dev);
return ret;
}
@@ -227,8 +235,9 @@ static int of_platform_serial_probe(struct platform_device
*ofdev)
return 0;
err_dispose:
irq_dispose_mapping(port8250.port.irq);
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ pm_runtime_put_sync(&ofdev->dev);
+ pm_runtime_disable(&ofdev->dev);
+ clk_disable_unprepare(info->clk);
err_free:
kfree(info);
return ret;
@@ -244,8 +253,9 @@ static int of_platform_serial_remove(struct platform_device
*ofdev)
serial8250_unregister_port(info->line);
reset_control_assert(info->rst);
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ pm_runtime_put_sync(&ofdev->dev);
+ pm_runtime_disable(&ofdev->dev);
+ clk_disable_unprepare(info->clk);
kfree(info);
return 0;
}
@@ -259,9 +269,10 @@ static int of_serial_suspend(struct device *dev)
serial8250_suspend_port(info->line);
- if (info->clk && (!uart_console(port) || console_suspend_enabled))
+ if ((!uart_console(port) || console_suspend_enabled)) {
+ pm_runtime_put_sync(&ofdev->dev);
clk_disable_unprepare(info->clk);
-
+ }
return 0;
}
@@ -271,8 +282,10 @@ static int of_serial_resume(struct device *dev)
struct uart_8250_port *port8250 = serial8250_get_port(info->line);
struct uart_port *port = &port8250->port;
- if (info->clk && (!uart_console(port) || console_suspend_enabled))
+ if ((!uart_console(port) || console_suspend_enabled)) {
+ pm_runtime_get_sync(&ofdev->dev);
clk_prepare_enable(info->clk);
+ }
serial8250_resume_port(info->line);
--
2.9.4.dirty
Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings
On 07/28/2017 01:33 PM, Oliver Hartkopp wrote: > Hi Kurt, > > On 07/28/2017 03:02 PM, Kurt Van Dijck wrote: > The word 'max-arbitration-bitrate' makes the difference very clear. >>> >>> I think you are mixing up ISO layer 1 and ISO layer 2. >> >> In order to provide higher data throughput without putting extra limits >> on transceiver & wire, the requirement for the round-trip delay to be >> within 1 bittime has been eliminated, but only for the data phase when >> arbitration is over. >> So layer 2 (CAN FD) has been adapted to circumvent the layer 1 >> (transceiver + wire) limitations. >> >> In fact, the round-trip delay requirement never actually did matter for >> plain CAN during data bits either. CAN FD just makes use of that, >> but is therefore incompatible on the wire. >> >> I forgot the precise wording, but this is the principle that Bosch >> explained on the CAN conference in Nurnberg several years ago, or at >> least this is how I remembered it :-) > > I just checked an example for a CAN FD qualified transceiver > > http://www.nxp.com/products/automotive-products/energy-power-management/can-transceivers/high-speed-can-transceiver-with-standby-mode:TJA1044 > > > where it states: > > The TJA1044T is specified for data rates up to 1 Mbit/s. Pending the > release of ISO11898-2:2016 including CAN FD and SAE-J2284-4/5, > additional timing parameters defining loop delay symmetry are specified > for the TJA1044GT and TJA1044GTK. This implementation enables reliable > communication in the CAN FD fast phase at data rates up to 5 Mbit/s. > > and > > TJA1044GT/TJA1044GTK > > - Timing guaranteed for data rates up to 5 Mbit/s > - Improved TXD to RXD propagation delay of 210 ns > >> I haven't followed the developments of transceivers, but with the above >> principle in mind, it's obvious that any transceiver allows higher >> bitrates during the data segment because the TX-to-RX line delay must >> not scale with the bitrate. >> In reality, maybe not all transceivers will mention this in their >> datasheet. >> >> So whether you call it 'max-arbitration-bitrate' & 'max-data-bitrate' >> or 'max-bitrate' & 'max-data-bitrate' does not really matter (I prefer >> 1st) but you will one day need 2 bitrates. > > The question to me is whether it is right option to specify two bitrates > OR to specify one maximum bitrate and provide a property that a CAN FD > capable propagation delay is available. > > E.g. > > max-bitrate > max-data-bitrate > > or > > max-bitrate > canfd-capable// CAN FD capable propagation delay available > > > I assume the optimized propagation delay is 'always on' as the > transceiver is not able to detect which kind of bits it is processing. > That's why I think providing two bitrates leads to a wrong view on the > transceiver. I agree with this. The transceiver is an analog device that needs to support faster switching frequency (FETs) including minimizing delay to support CAN-FD ie higher bitrate. From the transceiver perspective the bits for "arbitration" and "data" look exactly the same. Since it can't differentiate between the two (at the physical layer) then the actual limit isn't specific to which part/type of the CAN message is being sent. Rather its just a single overall max bitrate limit. > > Regards, > Oliver >
Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings
On 07/27/2017 01:47 PM, Oliver Hartkopp wrote: > On 07/26/2017 08:29 PM, Franklin S Cooper Jr wrote: >> > >> I'm fine with switching to using bitrate instead of speed. Kurk was >> originally the one that suggested to use the term arbitration and data >> since thats how the spec refers to it. Which I do agree with. But your >> right that in the drivers (struct can_priv) we just use bittiming and >> data_bittiming (CAN-FD timings). I don't think adding "fd" into the >> property name makes sense unless we are calling it something like >> "max-canfd-bitrate" which I would agree is the easiest to understand. >> >> So what is the preference if we end up sticking with two properties? >> Option 1 or 2? >> >> 1) >> max-bitrate >> max-data-bitrate >> >> 2) >> max-bitrate >> max-canfd-bitrate >> >> > > 1 > >>> A CAN transceiver is limited in bandwidth. But you only have one RX and >>> one TX line between the CAN controller and the CAN transceiver. The >>> transceiver does not know about CAN FD - it has just a physical(!) layer >>> with a limited bandwidth. This is ONE limitation. >>> >>> So I tend to specify only ONE 'max-bitrate' property for the >>> fixed-transceiver binding. >>> >>> The fact whether the CAN controller is CAN FD capable or not is provided >>> by the netlink configuration interface for CAN controllers. >> >> Part of the reasoning to have two properties is to indicate that you >> don't support CAN FD while limiting the "arbitration" bit rate. > > ?? > > It's a physical layer device which only has a bandwidth limitation. > The transceiver does not know about CAN FD. > >> With one >> property you can not determine this and end up having to make some >> assumptions that can quickly end up biting people. > > Despite the fact that the transceiver does not know anything about ISO > layer 2 (CAN/CAN FD) the properties should look like > > max-bitrate > canfd-capable > > then. > > But when the tranceiver is 'canfd-capable' agnostic, why provide a > property for it? > > Maybe I'm wrong but I still can't follow your argumentation ideas. Your right. I spoke to our CAN transceiver team and I finally get your points. So yes using "max-bitrate" alone is all we need. Sorry for the confusion and I'll create a new rev using this approach. > > Regards, > Oliver
Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate
Hi Kurt,
On 07/26/2017 03:04 PM, Kurt Van Dijck wrote:
> Hi,
>
> I know my response is late ...
>
>> Hi Oliver
>> On 07/20/2017 02:43 AM, Oliver Hartkopp wrote:
>>> Hi Franklin,
>>>
>>> On 07/20/2017 01:36 AM, Franklin S Cooper Jr wrote:
>>>
>>>> +#ifdef CONFIG_OF
>>>> +void of_transceiver_is_fixed(struct net_device *dev)
>>>> +{
>>>
>>> (..)
>>>
>>>> +}
>>>> +EXPORT_SYMBOL(of_transceiver_is_fixed);
>>>> +#endif
>>>
>>> I'm not sure about the naming here.
>>>
>>> As this is a CAN transceiver related option it should be named accordingly:
>
> I contest the the name too:
> 1) the can transceiver isn't fixed at all, it limited to the higher
> bitrates.
Its "possible" that this subnode may have additional properties beyond
bitrates in the future. But your right as of now it is specifically
addressing max bit rates. The naming of this function and subnode is
based on "fixed-link". So "fixed" is just implying that certain
properties can't be changed.
>
> 2) of_can_transceiver_is_fixed suggests to test if a transceiver is
> fixed, it does not suggest to load some properties from the device tree.
> of_can_load_transceiver looks way more clear to me.
I address this partially in my rev 2 that I've already sent. I'm now
using of_can_transceiver_fixed. The fact its of_ already implies it is
loading properties from device tree. So I don't think
of_can_load_transceiver really makes things clearer.
>
> That's my opinion.
> The important things, like the contents of the functions, look good.
You mind throwing your two cents in the thread for my latest patch?
Specifically the conversation regarding naming the properties. A couple
of people prefer to not use "arbitration" in one of the property names.
Currently I believe there are two options on property names that can be
used and I'm open to a majority vote on which one to go with.
>
> Kind regards,
> Kurt Van Dijck
>
Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings
On 07/26/2017 12:05 PM, Oliver Hartkopp wrote: > On 07/26/2017 06:41 PM, Andrew Lunn wrote: >> On Mon, Jul 24, 2017 at 06:05:19PM -0500, Franklin S Cooper Jr wrote: > >>> + >>> +Optional: >>> + max-arbitration-speed: a positive non 0 value that determines the max >>> +speed that CAN can run in non CAN-FD mode or during the >>> +arbitration phase in CAN-FD mode. >> >> Hi Franklin >> >> Since this and the next property are optional, it is good to document >> what happens when they are not in the DT blob. Also document what 0 >> means. The driver ignores values less than 0 with the exception being max-data-speed which supports a value of -1. Not sure what I'm documenting when the binding specifically says to use a positive non zero value. Its the same reason I don't document what happens if you give it a negative value. >> >>> + >>> + max-data-speed:a positive non 0 value that determines the max >>> data rate >>> +that can be used in CAN-FD mode. A value of -1 implies >>> +CAN-FD is not supported by the transceiver. >> >> -1 is ugly. I think it would be better to have a missing >> max-data-speed property indicate that CAN-FD is not supported. > Although this leads to your later point I don't think this is the right approach. Its an optional property. Not including the property should not assume it isn't supported. > Thanks Andrew! I had the same feeling about '-1' :-) Ok I'll go back to having 0 = not supported. Which will handle the documentation comment above. > >> And >> maybe put 'fd' into the property name. > > Good point. In fact the common naming to set bitrates for CAN(FD) > controllers are 'bitrate' and 'data bitrate'. > > 'speed' is not really a good word for that. I'm fine with switching to using bitrate instead of speed. Kurk was originally the one that suggested to use the term arbitration and data since thats how the spec refers to it. Which I do agree with. But your right that in the drivers (struct can_priv) we just use bittiming and data_bittiming (CAN-FD timings). I don't think adding "fd" into the property name makes sense unless we are calling it something like "max-canfd-bitrate" which I would agree is the easiest to understand. So what is the preference if we end up sticking with two properties? Option 1 or 2? 1) max-bitrate max-data-bitrate 2) max-bitrate max-canfd-bitrate > > Finally, @Franklin: > > A CAN transceiver is limited in bandwidth. But you only have one RX and > one TX line between the CAN controller and the CAN transceiver. The > transceiver does not know about CAN FD - it has just a physical(!) layer > with a limited bandwidth. This is ONE limitation. > > So I tend to specify only ONE 'max-bitrate' property for the > fixed-transceiver binding. > > The fact whether the CAN controller is CAN FD capable or not is provided > by the netlink configuration interface for CAN controllers. Part of the reasoning to have two properties is to indicate that you don't support CAN FD while limiting the "arbitration" bit rate. With one property you can not determine this and end up having to make some assumptions that can quickly end up biting people. > > Regards, > Oliver >
[PATCH 1/3] ARM: dts: am335x-evm: Enable NAND dma prefetch by default
Currently the default method of prefetch polled shows the highest possible read and write speed when minimal non NAND background activity is being done. But it is also very CPU intensive to reach these high speeds (CPU load of 99% via mtd performance tests). While DMA prefetch only uses 50% of the CPU to achieve around 23% less in top read and write performance. However, as the non NAND CPU load increases the read and write performance takes a large hit when using polled prefetch. Therefore, prefetch dma mode ends up outperforming prefetch polled in general "system level" test. So switch to using dma prefetch by default since it is likely what most users would prefer. Signed-off-by: Franklin S Cooper Jr --- arch/arm/boot/dts/am335x-evm.dts | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts index 1c37a7c..ddd8975 100644 --- a/arch/arm/boot/dts/am335x-evm.dts +++ b/arch/arm/boot/dts/am335x-evm.dts @@ -531,6 +531,7 @@ interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */ <1 IRQ_TYPE_NONE>; /* termcount */ rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>; /* gpmc_wait0 */ + ti,nand-xfer-type = "prefetch-dma"; ti,nand-ecc-opt = "bch8"; ti,elm-id = <&elm>; nand-bus-width = <8>; -- 2.10.0
[PATCH 2/3] ARM: dts: am437xx: Enable NAND dma prefetch by default
Currently the default method of prefetch polled shows the highest possible read and write speed when minimal non NAND background activity is being done. But it is also very CPU intensive to reach these high speeds (CPU load of 99% via mtd performance tests). While DMA prefetch only uses 50% of the CPU to achieve around 23% less in top read and write performance. However, as the non NAND CPU load increases the read and write performance takes a large hit when using polled prefetch. Therefore, prefetch dma mode ends up outperforming prefetch polled in general "system level" test. So switch to using dma prefetch by default since it is likely what most users would prefer. Signed-off-by: Franklin S Cooper Jr --- arch/arm/boot/dts/am437x-gp-evm.dts | 1 + arch/arm/boot/dts/am43x-epos-evm.dts | 1 + 2 files changed, 2 insertions(+) diff --git a/arch/arm/boot/dts/am437x-gp-evm.dts b/arch/arm/boot/dts/am437x-gp-evm.dts index 29a538e..a0a4ed0 100644 --- a/arch/arm/boot/dts/am437x-gp-evm.dts +++ b/arch/arm/boot/dts/am437x-gp-evm.dts @@ -842,6 +842,7 @@ interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */ <1 IRQ_TYPE_NONE>; /* termcount */ rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>; /* gpmc_wait0 */ + ti,nand-xfer-type = "prefetch-dma"; ti,nand-ecc-opt = "bch16"; ti,elm-id = <&elm>; nand-bus-width = <8>; diff --git a/arch/arm/boot/dts/am43x-epos-evm.dts b/arch/arm/boot/dts/am43x-epos-evm.dts index 54f40f3..9d276af 100644 --- a/arch/arm/boot/dts/am43x-epos-evm.dts +++ b/arch/arm/boot/dts/am43x-epos-evm.dts @@ -564,6 +564,7 @@ interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */ <1 IRQ_TYPE_NONE>; /* termcount */ rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>; /* gpmc_wait0 */ + ti,nand-xfer-type = "prefetch-dma"; ti,nand-ecc-opt = "bch16"; ti,elm-id = <&elm>; nand-bus-width = <8>; -- 2.10.0
[PATCH 0/3] ARM: dts: am335x-evm: am437xx: dra7xx: Enable NAND DMA prefetch by default
Enable NAND DMA prefetch by default for AM335x, AM437x and DRA7x evms that
include NAND.
The below commit went into additional details regarding performance numbers
seen between the two modes.
commit aa7abd312c11 ("mtd: nand: omap2: Support parsing dma channel information
from DT")
Franklin S Cooper Jr (3):
ARM: dts: am335x-evm: Enable NAND dma prefetch by default
ARM: dts: am437xx: Enable NAND dma prefetch by default
ARM: dts: dra7xx: Enable NAND dma prefetch by default
arch/arm/boot/dts/am335x-evm.dts| 1 +
arch/arm/boot/dts/am437x-gp-evm.dts | 1 +
arch/arm/boot/dts/am43x-epos-evm.dts| 1 +
arch/arm/boot/dts/dra7-evm.dts | 1 +
arch/arm/boot/dts/dra72-evm-common.dtsi | 1 +
5 files changed, 5 insertions(+)
--
2.10.0
[PATCH 3/3] ARM: dts: dra7xx: Enable NAND dma prefetch by default
Currently the default method of prefetch polled shows the highest possible read and write speed when minimal non NAND background activity is being done. But it is also very CPU intensive to reach these high speeds (CPU load of 99% via mtd performance tests). While DMA prefetch only uses 50% of the CPU to achieve around 23% less in top read and write performance. However, as the non NAND CPU load increases the read and write performance takes a large hit when using polled prefetch. Therefore, prefetch dma mode ends up outperforming prefetch polled in general "system level" test. So switch to using dma prefetch by default since it is likely what most users would prefer. Signed-off-by: Franklin S Cooper Jr --- arch/arm/boot/dts/dra7-evm.dts | 1 + arch/arm/boot/dts/dra72-evm-common.dtsi | 1 + 2 files changed, 2 insertions(+) diff --git a/arch/arm/boot/dts/dra7-evm.dts b/arch/arm/boot/dts/dra7-evm.dts index f47fc4d..7d55af9 100644 --- a/arch/arm/boot/dts/dra7-evm.dts +++ b/arch/arm/boot/dts/dra7-evm.dts @@ -556,6 +556,7 @@ interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */ <1 IRQ_TYPE_NONE>; /* termcount */ rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>; /* gpmc_wait0 pin */ + ti,nand-xfer-type = "prefetch-dma"; ti,nand-ecc-opt = "bch8"; ti,elm-id = <&elm>; nand-bus-width = <16>; diff --git a/arch/arm/boot/dts/dra72-evm-common.dtsi b/arch/arm/boot/dts/dra72-evm-common.dtsi index 8578054..1830483 100644 --- a/arch/arm/boot/dts/dra72-evm-common.dtsi +++ b/arch/arm/boot/dts/dra72-evm-common.dtsi @@ -311,6 +311,7 @@ interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */ <1 IRQ_TYPE_NONE>; /* termcount */ rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>; /* gpmc_wait0 pin */ + ti,nand-xfer-type = "prefetch-dma"; ti,nand-ecc-opt = "bch8"; ti,elm-id = <&elm>; nand-bus-width = <16>; -- 2.10.0
Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings
On 07/25/2017 11:32 AM, Oliver Hartkopp wrote:
>
>> + max-data-speed:a positive non 0 value that determines the max
>> data rate
>> +that can be used in CAN-FD mode. A value of -1 implies
>> +CAN-FD is not supported by the transceiver.
>> +
>> +Examples:
>
> (..)
>
>> +fixed-transceiver {
>> +max-data-speed = <(-1)>;
>
> Looks ugly IMHO.
>
> Why didn't you stay on '0' for 'not supported'??
Unless a driver specifically calls of_can_transceiver_fixed
priv->max_trans_data_speed will be by default 0. Therefore, all drivers
that support CAN-FD will claim that the transceiver indicates that it
isn't supported. So one option was to update every single driver to set
this property by default which I started to do but it end up becoming a
massive patch and it was risky in case I missed a driver which would of
resulted in major regressions. Its also problematic for new drivers that
miss this property or the many out of tree CAN drivers. The other option
was to create another variable to track to see if
of_can_transceiver_fixed was called but I didn't think that was the
better solution. So using signed values in DT is a bit ugly due to
syntax but was valid and I made sure I documented it so its clear.
>
> Regards,
> Oliver
>
[PATCH v2 4/4] can: m_can: Add call to of_can_transceiver_fixed
Add call to new generic functions that provides support via a binding to limit the arbitration rate and/or data rate imposed by the physical transceiver connected to the MCAN peripheral. Signed-off-by: Franklin S Cooper Jr --- drivers/net/can/m_can/m_can.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index f4947a7..bd75df1 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1649,6 +1649,8 @@ static int m_can_plat_probe(struct platform_device *pdev) devm_can_led_init(dev); + of_can_transceiver_fixed(dev); + dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n", KBUILD_MODNAME, dev->irq, priv->version); -- 2.10.0
[PATCH v2 3/4] can: m_can: Update documentation to mention new fixed transceiver binding
Add information regarding fixed transceiver binding. This is especially
important for MCAN since the IP allows CAN FD mode to run significantly
faster than what most transceivers are capable of.
Signed-off-by: Franklin S Cooper Jr
---
Version 2 changes:
Drop unit address
Documentation/devicetree/bindings/net/can/m_can.txt | 10 ++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
b/Documentation/devicetree/bindings/net/can/m_can.txt
index 9e33177..e4abd2c 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -43,6 +43,11 @@ Required properties:
Please refer to 2.4.1 Message RAM Configuration in
Bosch M_CAN user manual for details.
+Optional properties:
+- fixed-transceiver: Fixed-transceiver subnode describing maximum speed
+ that can be used for CAN and/or CAN-FD modes. See
+
Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
+ for details.
Example:
SoC dtsi:
m_can1: can@020e8000 {
@@ -64,4 +69,9 @@ Board dts:
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_m_can1>;
status = "enabled";
+
+ fixed-transceiver {
+ max-arbitration-speed = <100>;
+ max-data-speed = <500>;
+ };
};
--
2.10.0
[PATCH v2 0/4] can: Add new binding to limit bit rate used
Add a new generic binding that CAN drivers can use to specify the max arbitration and data bit rate supported by a transceiver. This is useful since in some instances the maximum speeds may be limited by the transceiver used. However, transceivers may not provide a means to determine this limitation at runtime. Therefore, create a new binding that mimics "fixed-link" that allows a user to hardcode the max speeds that can be used. Also add support for this new binding in the MCAN driver. Note this is an optional subnode so even if a driver adds support for parsing fixed-transceiver the user does not have to define it in their device tree. Version 2 changes: Rename function Define proper variable default Drop unit address Move check to changelink function Reword commit message Reword documentation Franklin S Cooper Jr (4): can: dev: Add support for limiting configured bitrate can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings can: m_can: Update documentation to mention new fixed transceiver binding can: m_can: Add call to of_can_transceiver_fixed .../bindings/net/can/fixed-transceiver.txt | 42 +++ .../devicetree/bindings/net/can/m_can.txt | 10 drivers/net/can/dev.c | 59 ++ drivers/net/can/m_can/m_can.c | 2 + include/linux/can/dev.h| 5 ++ 5 files changed, 118 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/can/fixed-transceiver.txt -- 2.10.0
[PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings
Add documentation to describe usage of the new fixed transceiver binding.
This new binding is applicable for any CAN device therefore it exists as
its own document.
Signed-off-by: Franklin S Cooper Jr
---
Version 2:
Tweak commit message working
Tweak wording for properties
Drop unit addressing
Give example if CAN-FD isn't supported
.../bindings/net/can/fixed-transceiver.txt | 42 ++
1 file changed, 42 insertions(+)
create mode 100644
Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
diff --git a/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
new file mode 100644
index 000..dc7e3eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
@@ -0,0 +1,42 @@
+Fixed transceiver Device Tree binding
+--
+
+CAN transceiver typically limits the max speed in standard CAN and CAN FD
+modes. Typically these limitations are static and the transceivers themselves
+provide no way to detect this limitation at runtime. For this situation,
+the "fixed-transceiver" node can be used.
+
+Properties:
+
+Optional:
+ max-arbitration-speed: a positive non 0 value that determines the max
+ speed that CAN can run in non CAN-FD mode or during the
+ arbitration phase in CAN-FD mode.
+
+ max-data-speed: a positive non 0 value that determines the max data rate
+ that can be used in CAN-FD mode. A value of -1 implies
+ CAN-FD is not supported by the transceiver.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+
+ fixed-transceiver {
+ max-arbitration-speed = <100>;
+ max-data-speed = <500>;
+ };
+ ...
+};
+
+Based on a CAN Transceiver that doesn't support CAN-FD. Only needed if the
+CAN IP supports CAN-FD but is using a transceiver that doesn't.
+
+m_can0 {
+
+ fixed-transceiver {
+ max-data-speed = <(-1)>;
+ };
+ ...
+};
--
2.10.0
[PATCH v2 1/4] can: dev: Add support for limiting configured bitrate
Various CAN or CAN-FD IP may be able to run at a faster rate than
what the transceiver the CAN node is connected to. This can lead to
unexpected errors. However, CAN transceivers typically have fixed
limitations and provide no means to discover these limitations at
runtime. Therefore, add support for a fixed-transceiver node that
can be reused by other CAN peripheral drivers to determine for both
CAN and CAN-FD what the max bitrate that can be used. If the user
tries to configure CAN to pass these maximum bitrates it will throw
an error.
Signed-off-by: Franklin S Cooper Jr
---
Version 2 changes:
Rename new function to of_can_transceiver_fixed
Use version of of_property_read that supports signed/negative values
Return error when user tries to use CAN-FD if the transceiver doesn't
support it (max-data-speed = -1).
drivers/net/can/dev.c | 59 +
include/linux/can/dev.h | 5 +
2 files changed, 64 insertions(+)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..c046631 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#define MOD_DESC "CAN device driver interface"
@@ -814,6 +815,41 @@ int open_candev(struct net_device *dev)
}
EXPORT_SYMBOL_GPL(open_candev);
+#ifdef CONFIG_OF
+void of_can_transceiver_fixed(struct net_device *dev)
+{
+ struct device_node *dn;
+ struct can_priv *priv = netdev_priv(dev);
+ int max_frequency;
+ struct device_node *np;
+
+ np = dev->dev.parent->of_node;
+
+ dn = of_get_child_by_name(np, "fixed-transceiver");
+ if (!dn)
+ return;
+
+ /* Value of 0 implies ignore max speed constraint */
+ max_frequency = 0;
+ of_property_read_s32(dn, "max-arbitration-speed", &max_frequency);
+
+ if (max_frequency >= 0)
+ priv->max_trans_arbitration_speed = max_frequency;
+ else
+ priv->max_trans_arbitration_speed = 0;
+
+ max_frequency = 0;
+
+ of_property_read_s32(dn, "max-data-speed", &max_frequency);
+
+ if (max_frequency >= -1)
+ priv->max_trans_data_speed = max_frequency;
+ else
+ priv->max_trans_data_speed = 0;
+}
+EXPORT_SYMBOL(of_can_transceiver_fixed);
+#endif
+
/*
* Common close function for cleanup before the device gets closed.
*
@@ -913,6 +949,14 @@ static int can_changelink(struct net_device *dev, struct
nlattr *tb[],
priv->bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->max_trans_arbitration_speed > 0 &&
+ bt.bitrate > priv->max_trans_arbitration_speed) {
+ netdev_err(dev, "arbitration bitrate surpasses
transceiver capabilities of %d bps\n",
+ priv->max_trans_arbitration_speed);
+ return -EINVAL;
+ }
+
memcpy(&priv->bittiming, &bt, sizeof(bt));
if (priv->do_set_bittiming) {
@@ -989,6 +1033,12 @@ static int can_changelink(struct net_device *dev, struct
nlattr *tb[],
if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
return -EOPNOTSUPP;
+ if ((priv->ctrlmode & CAN_CTRLMODE_FD) &&
+ priv->max_trans_data_speed == -1) {
+ netdev_err(dev, "canfd mode is not supported by
transceiver\n");
+ return -EINVAL;
+ }
+
memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
sizeof(dbt));
err = can_get_bittiming(dev, &dbt,
@@ -997,6 +1047,15 @@ static int can_changelink(struct net_device *dev, struct
nlattr *tb[],
priv->data_bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->max_trans_data_speed > 0 &&
+ (priv->ctrlmode & CAN_CTRLMODE_FD) &&
+ (dbt.bitrate > priv->max_trans_data_speed)) {
+ netdev_err(dev, "canfd data bitrate surpasses
transceiver capabilities of %d bps\n",
+ priv->max_trans_data_speed);
+ return -EINVAL;
+ }
+
memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
if (priv->do_set_data_bittiming) {
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 141b05a..926fc7e 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -47,6 +47,9 @@ struct can_priv {
unsigned int data_bitrat
[PATCH v2 1/3] can: m_can: Make hclk optional
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
---
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;
}
--
2.10.0
[PATCH v2 0/3] can: m_can: Add PM Runtime Support
Add PM runtime support to the MCAN driver. To support devices that don't use PM runtime leave the original clk calls in the driver. Perhaps in the future when it makes sense we can remove these non pm runtime clk calls. Version 2 changes: Used NULL instead of 0 for unused hclk handle Franklin S Cooper Jr (3): can: m_can: Make hclk optional can: m_can: Update documentation to indicate that hclk may be optional can: m_can: Add PM Runtime .../devicetree/bindings/net/can/m_can.txt | 3 ++- drivers/net/can/m_can/m_can.c | 22 -- 2 files changed, 22 insertions(+), 3 deletions(-) -- 2.10.0
[PATCH v2 3/3] can: m_can: Add PM Runtime
Add support for PM Runtime which is the new way to handle managing clocks.
However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
management approach in place.
PM_RUNTIME is required by OMAP based devices to handle clock management.
Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
to work with this driver.
Signed-off-by: Franklin S Cooper Jr
---
drivers/net/can/m_can/m_can.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index ea48e59..93e23f5 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
@@ -633,11 +634,15 @@ static int m_can_clk_start(struct m_can_priv *priv)
if (err)
clk_disable_unprepare(priv->hclk);
+ pm_runtime_get_sync(priv->device);
+
return err;
}
static void m_can_clk_stop(struct m_can_priv *priv)
{
+ pm_runtime_put_sync(priv->device);
+
clk_disable_unprepare(priv->cclk);
clk_disable_unprepare(priv->hclk);
}
@@ -1582,6 +1587,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
/* Enable clocks. Necessary to read Core Release in order to determine
* M_CAN version
*/
+ pm_runtime_enable(&pdev->dev);
+
ret = clk_prepare_enable(hclk);
if (ret)
goto disable_hclk_ret;
@@ -1626,6 +1633,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
*/
tx_fifo_size = mram_config_vals[7];
+ pm_runtime_get_sync(&pdev->dev);
+
/* allocate the m_can device */
dev = alloc_m_can_dev(pdev, addr, tx_fifo_size);
if (!dev) {
@@ -1670,6 +1679,7 @@ static int m_can_plat_probe(struct platform_device *pdev)
disable_hclk_ret:
clk_disable_unprepare(hclk);
failed_ret:
+ pm_runtime_put_sync(&pdev->dev);
return ret;
}
@@ -1726,6 +1736,9 @@ static int m_can_plat_remove(struct platform_device *pdev)
struct net_device *dev = platform_get_drvdata(pdev);
unregister_m_can_dev(dev);
+
+ pm_runtime_disable(&pdev->dev);
+
platform_set_drvdata(pdev, NULL);
free_m_can_dev(dev);
--
2.10.0
[PATCH v2 2/3] can: m_can: Update documentation to indicate that hclk may be optional
Update the documentation to reflect that hclk is now an optional clock. Signed-off-by: Franklin S Cooper Jr Acked-by: Rob Herring --- Documentation/devicetree/bindings/net/can/m_can.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt index 9e33177..2a0fe5b 100644 --- a/Documentation/devicetree/bindings/net/can/m_can.txt +++ b/Documentation/devicetree/bindings/net/can/m_can.txt @@ -12,7 +12,8 @@ Required properties: - interrupt-names : Should contain "int0" and "int1" - clocks : Clocks used by controller, should be host clock and CAN clock. -- clock-names : Should contain "hclk" and "cclk" +- clock-names : Should contain "hclk" and "cclk". For some socs hclk + may be optional. - pinctrl- : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt - pinctrl-names: Names corresponding to the numbered pinctrl states - bosch,mram-cfg : Message RAM configuration data. -- 2.10.0
Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate
On 07/20/2017 04:52 AM, Sergei Shtylyov wrote:
> Hello!
>
> On 7/20/2017 2:36 AM, Franklin S Cooper Jr wrote:
>
>> Various CAN or CAN-FD IP may be able to run at a faster rate than
>> what the transceiver the CAN node is connected to. This can lead to
>> unexpected errors. However, CAN transceivers typically have fixed
>> limitations and provide no means to discover these limitations at
>> runtime. Therefore, add support for a fixed-transceiver node that
>> can be reused by other CAN peripheral drivers to determine for both
>> CAN and CAN-FD what the max bitrate that can be used. If the user
>> tries to configure CAN to pass these maximum bitrates it will throw
>> an error.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> drivers/net/can/dev.c | 48
>>
>> include/linux/can/dev.h | 5 +
>> 2 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 365a8cc..fbab87d 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
> [...]
>> @@ -814,6 +830,38 @@ int open_candev(struct net_device *dev)
>> }
>> EXPORT_SYMBOL_GPL(open_candev);
>> +#ifdef CONFIG_OF
>> +void of_transceiver_is_fixed(struct net_device *dev)
>
>Strange name for a *void* function...
Ok I see what you mean since I'm not actually returning anything. I'll
go with of_can_transceiver_fixed based on your comment also Oliver's
suggestion.
>Also, I think 'struct net_device *' variables are typically called
> 'ndev'.
All other functions within this file uses struct net_device *dev. So I'm
just following the style currently used.
>
>> +{
>> +struct device_node *dn;
>> +struct can_priv *priv = netdev_priv(dev);
>> +u32 max_frequency;
>> +struct device_node *np;
>> +
>> +np = dev->dev.parent->of_node;
>> +
>> +/* New binding */
>> +dn = of_get_child_by_name(np, "fixed-transceiver");
>> +if (!dn)
>> +return;
>> +
>> +of_property_read_u32(dn, "max-arbitration-speed", &max_frequency);
>
>In case this function fails, 'max_frequency' will have no value --
> you'd better initialize it...
Thanks for catching this. Will fix.
>
>> +
>> +if (max_frequency > 0)
>> +priv->max_trans_arbitration_speed = max_frequency;
>> +else
>> +priv->max_trans_arbitration_speed = -1;
>> +
>> +of_property_read_u32(dn, "max-data-speed", &max_frequency);
>
>Again, when that function fails, the variable will keep the value
> from the previous call...
Will fix.
>
> [...]
>
> MBR, Sergei
Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate
Hi Oliver
On 07/20/2017 02:43 AM, Oliver Hartkopp wrote:
> Hi Franklin,
>
> On 07/20/2017 01:36 AM, Franklin S Cooper Jr wrote:
>
>> +#ifdef CONFIG_OF
>> +void of_transceiver_is_fixed(struct net_device *dev)
>> +{
>
> (..)
>
>> +}
>> +EXPORT_SYMBOL(of_transceiver_is_fixed);
>> +#endif
>
> I'm not sure about the naming here.
>
> As this is a CAN transceiver related option it should be named accordingly:
>
> E.g.
>
> can_transceiver_is_fixed
> of_can_transceiver_is_fixed
> ...
>
> Especially as it is defined in include/linux/can/dev.h
Thanks for the feedback. I'll go with of_can_transceiver_is_fixed
>
> Regards,
> Oliver
>
>
[PATCH 1/4] can: dev: Add support for limiting configured bitrate
Various CAN or CAN-FD IP may be able to run at a faster rate than
what the transceiver the CAN node is connected to. This can lead to
unexpected errors. However, CAN transceivers typically have fixed
limitations and provide no means to discover these limitations at
runtime. Therefore, add support for a fixed-transceiver node that
can be reused by other CAN peripheral drivers to determine for both
CAN and CAN-FD what the max bitrate that can be used. If the user
tries to configure CAN to pass these maximum bitrates it will throw
an error.
Signed-off-by: Franklin S Cooper Jr
---
drivers/net/can/dev.c | 48
include/linux/can/dev.h | 5 +
2 files changed, 53 insertions(+)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..fbab87d 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#define MOD_DESC "CAN device driver interface"
@@ -806,6 +807,21 @@ int open_candev(struct net_device *dev)
return -EINVAL;
}
+ if (priv->max_trans_arbitration_speed > 0 &&
+ priv->bittiming.bitrate > priv->max_trans_arbitration_speed) {
+ netdev_err(dev, "arbitration bitrate surpasses transceiver
capabilities of %d bps\n",
+ priv->max_trans_arbitration_speed);
+ return -EINVAL;
+ }
+
+ if (priv->max_trans_data_speed >= 0 &&
+ (priv->ctrlmode & CAN_CTRLMODE_FD) &&
+ (priv->data_bittiming.bitrate > priv->max_trans_data_speed)) {
+ netdev_err(dev, "canfd data bitrate surpasses transceiver
capabilities of %d bps\n",
+ priv->max_trans_data_speed);
+ return -EINVAL;
+ }
+
/* Switch carrier on if device was stopped while in bus-off state */
if (!netif_carrier_ok(dev))
netif_carrier_on(dev);
@@ -814,6 +830,38 @@ int open_candev(struct net_device *dev)
}
EXPORT_SYMBOL_GPL(open_candev);
+#ifdef CONFIG_OF
+void of_transceiver_is_fixed(struct net_device *dev)
+{
+ struct device_node *dn;
+ struct can_priv *priv = netdev_priv(dev);
+ u32 max_frequency;
+ struct device_node *np;
+
+ np = dev->dev.parent->of_node;
+
+ /* New binding */
+ dn = of_get_child_by_name(np, "fixed-transceiver");
+ if (!dn)
+ return;
+
+ of_property_read_u32(dn, "max-arbitration-speed", &max_frequency);
+
+ if (max_frequency > 0)
+ priv->max_trans_arbitration_speed = max_frequency;
+ else
+ priv->max_trans_arbitration_speed = -1;
+
+ of_property_read_u32(dn, "max-data-speed", &max_frequency);
+
+ if (max_frequency >= 0)
+ priv->max_trans_data_speed = max_frequency;
+ else
+ priv->max_trans_data_speed = -1;
+}
+EXPORT_SYMBOL(of_transceiver_is_fixed);
+#endif
+
/*
* Common close function for cleanup before the device gets closed.
*
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 141b05a..aec72b5 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -69,6 +69,9 @@ struct can_priv {
unsigned int echo_skb_max;
struct sk_buff **echo_skb;
+ unsigned int max_trans_arbitration_speed;
+ unsigned int max_trans_data_speed;
+
#ifdef CONFIG_CAN_LEDS
struct led_trigger *tx_led_trig;
char tx_led_trig_name[CAN_LED_NAME_SZ];
@@ -165,6 +168,8 @@ void can_put_echo_skb(struct sk_buff *skb, struct
net_device *dev,
unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
void can_free_echo_skb(struct net_device *dev, unsigned int idx);
+void of_transceiver_is_fixed(struct net_device *dev);
+
struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
struct sk_buff *alloc_canfd_skb(struct net_device *dev,
struct canfd_frame **cfd);
--
2.10.0
[PATCH 4/4] can: m_can: Add call to of_transceiver_is_fixed
Add call to new generic functions that provides support via a binding to limit the arbitration rate and/or data rate imposed by the physical transceiver connected to the MCAN peripheral. Signed-off-by: Franklin S Cooper Jr --- drivers/net/can/m_can/m_can.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index f4947a7..db1882c 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1649,6 +1649,8 @@ static int m_can_plat_probe(struct platform_device *pdev) devm_can_led_init(dev); + of_transceiver_is_fixed(dev); + dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n", KBUILD_MODNAME, dev->irq, priv->version); -- 2.10.0
