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 any bittiming related setup. >>&g
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
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 <fcoo...@ti.com> >>>>>>>>> --- >>>>>>>>> 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
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 <fcoo...@ti.com> >>>>>> --- >>>>>> 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 <fcoo...@ti.com>
>
> 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(>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(>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(>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(>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 <fcoo...@ti.com>
>> ---
>> Version 2 changes:
>> Used NULL instead of 0 for unused hclk handle
>>
>> drivers/net/can/m_can/m_can.c | 9 +++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index f4947a7..ea48e59 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device
>> *pdev)
>> hclk = devm_clk_get(>dev, "hclk");
>> cclk = devm_clk_get(>dev, "cclk");
>>
>> -if (IS_ERR(hclk) || IS_ERR(cclk)) {
>> -dev_err(>dev, "no clock found\n");
>> +if (IS_ERR(hclk)) {
>> +dev_warn(>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(>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 <fcoo...@ti.com>
>> ---
>> Version 2 changes:
>> Used NULL instead of 0 for unused hclk handle
>>
>> drivers/net/can/m_can/m_can.c | 9 +++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index f4947a7..ea48e59 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device
>> *pdev)
>> hclk = devm_clk_get(>dev, "hclk");
>> cclk = devm_clk_get(>dev, "cclk");
>>
>> -if (IS_ERR(hclk) || IS_ERR(cclk)) {
>> -dev_err(>dev, "no clock found\n");
>> +if (IS_ERR(hclk)) {
>> +dev_warn(>dev, "hclk could not be found\n");
>> +hclk = NULL;
>> +}
>> +
>> +if (IS_ERR(cclk)) {
>> +dev_err(>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 <fcoo...@ti.com>
>>>> ---
>>>> 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 = >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 <fcoo...@ti.com>
> ---
> 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 = >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 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 <fcoo...@ti.com>
Acked-by: Rob Herring <r...@kernel.org>
---
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 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 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 <fcoo...@ti.com> --- 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(>dev, "%s device registered (irq=%d, version=%d)\n", KBUILD_MODNAME, dev->irq, priv->version); -- 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 <fcoo...@ti.com>
Acked-by: Rob Herring <r...@kernel.org>
---
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 = <_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 <fcoo...@ti.com>
---
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", >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(>bittiming, , 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(>data_bittiming, , 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 <fcoo...@ti.com>
---
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 = >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 <fcoo...@ti.com>
>> ---
>> 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", _bitrate);
>
> IIRC the last argument is only modified in case of no error, so what about:
> ret = of_property_read_u32(dn, "max-bitrate",
> >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(>bittiming, , 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 <fcoo...@ti.com>
>> ---
>> 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", _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 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 <fcoo...@ti.com>
>> ---
>> 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 = <_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 <fcoo...@ti.com> --- 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(>dev, "%s device registered (irq=%d, version=%d)\n", KBUILD_MODNAME, dev->irq, priv->version); -- 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 <fcoo...@ti.com>
>>>>> ---
>>>>>.../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 <fcoo...@ti.com>
---
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 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
[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 <fcoo...@ti.com>
---
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", _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(>bittiming, , 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(>data_bittiming, , 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
[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 <fcoo...@ti.com>
---
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 = <_m_can1>;
status = "enabled";
+
+ can-transceiver@ {
+ max-bitrate = <500>;
+ };
};
--
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
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 <fcoo...@ti.com>
>>>> ---
>>>>.../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 <fcoo...@ti.com>
>> ---
>> 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 = <_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 <fcoo...@ti.com>
>> ---
>> .../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 <fcoo...@ti.com> --- 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(>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 <fcoo...@ti.com>
---
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", _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(>bittiming, , 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(>data_bittiming, , 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 <fcoo...@ti.com>
---
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 = <_m_can1>;
status = "enabled";
+
+ 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 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 <fcoo...@ti.com>
---
.../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 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 <fcoo...@ti.com> --- 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 <lokeshvu...@ti.com>
Add nodes for the two DCAN instances included in 66AK2G
Signed-off-by: Lokesh Vutla <lokeshvu...@ti.com>
[d-gerl...@ti.com: add power-domains and clock information]
Signed-off-by: Dave Gerlach <d-gerl...@ti.com>
[fcoo...@ti.com: update subject and commit message. Misc minor updates]
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
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 = <_pds 0x0008>;
+ clocks = <_clks 0x0008 1>;
+ };
+
+ dcan1: can@0260B400 {
+ compatible = "ti,am4372-d_can", "ti,am3352-d_can";
+ reg = <0x0260B400 0x200>;
+ interrupts = ;
+ status = "disabled";
+ power-domains = <_pds 0x0009>;
+ clocks = <_clks 0x0009 1>;
+ };
+
kirq0: keystone_irq@026202a0 {
compatible = "ti,keystone-irq";
interrupts = ;
--
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 <fcoo...@ti.com> --- 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
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 >
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 <fcoo...@ti.com> --- 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(>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 <fcoo...@ti.com>
---
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 = <_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 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 <fcoo...@ti.com>
---
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", _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", _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(>bittiming, , 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(, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
sizeof(dbt));
err = can_get_bittiming(dev, ,
@@ -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(>data_bittiming, , 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_bitrate_const_cnt;
struct can_clock clock;
[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 <fcoo...@ti.com>
---
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/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 <fcoo...@ti.com>
---
Version 2 changes:
Used NULL instead of 0 for unused hclk handle
drivers/net/can/m_can/m_can.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f4947a7..ea48e59 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device *pdev)
hclk = devm_clk_get(>dev, "hclk");
cclk = devm_clk_get(>dev, "cclk");
- if (IS_ERR(hclk) || IS_ERR(cclk)) {
- dev_err(>dev, "no clock found\n");
+ if (IS_ERR(hclk)) {
+ dev_warn(>dev, "hclk could not be found\n");
+ hclk = NULL;
+ }
+
+ if (IS_ERR(cclk)) {
+ dev_err(>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 <fcoo...@ti.com>
---
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(>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(>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(>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(>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 <fcoo...@ti.com> Acked-by: Rob Herring <r...@kernel.org> --- 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 <fcoo...@ti.com>
>> ---
>> 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", _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", _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 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. 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_transceiver_is_fixed .../bindings/net/can/fixed-transceiver.txt | 31 ++ .../devicetree/bindings/net/can/m_can.txt | 10 + drivers/net/can/dev.c | 48 ++ drivers/net/can/m_can/m_can.c | 2 + include/linux/can/dev.h| 5 +++ 5 files changed, 96 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/can/fixed-transceiver.txt -- 2.10.0
[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 <fcoo...@ti.com>
---
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", _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", _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 <fcoo...@ti.com> --- 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(>dev, "%s device registered (irq=%d, version=%d)\n", KBUILD_MODNAME, dev->irq, priv->version); -- 2.10.0
[PATCH 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 <fcoo...@ti.com>
---
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..4440e4b 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 = <_m_can1>;
status = "enabled";
+
+ fixed-transceiver@0 {
+ max-arbitration-speed = <100>;
+ max-data-speed = <500>;
+ };
};
--
2.10.0
[PATCH 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 exist as
its own document.
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
.../bindings/net/can/fixed-transceiver.txt | 31 ++
1 file changed, 31 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..7c093c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
@@ -0,0 +1,31 @@
+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 value non 0 value that determines the max
+speed CAN can run in non CAN-FD mode or during the
+arbitration phase in CAN-FD mode.
+
+ max-data-speed:a positive value that determines the max data rate
+that can be used in CAN-FD mode. A value of 0
+implies CAN-FD is not supported by the transceiver.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+
+ fixed-transceiver@0 {
+ max-arbitration-speed = <100>;
+ max-data-speed = <500>;
+ };
+ ...
+};
--
2.10.0
[PATCH 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 <fcoo...@ti.com>
---
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..7fe9145 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(>dev, "hclk");
cclk = devm_clk_get(>dev, "cclk");
- if (IS_ERR(hclk) || IS_ERR(cclk)) {
- dev_err(>dev, "no clock found\n");
+ if (IS_ERR(hclk)) {
+ dev_warn(>dev, "can't find hclk\n");
+ hclk = 0;
+ }
+
+ if (IS_ERR(cclk)) {
+ dev_err(>dev, "cclk could not be found\n");
ret = -ENODEV;
goto failed_ret;
}
--
2.10.0
[PATCH 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 <fcoo...@ti.com>
---
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 7fe9145..eb45cd5 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(>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(>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(>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(>dev);
+
platform_set_drvdata(pdev, NULL);
free_m_can_dev(dev);
--
2.10.0
[PATCH 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 <fcoo...@ti.com> --- 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
[PATCH 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. 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
Re: CAN-FD Transceiver Limitations
Hi Kurt,
On 06/30/2017 03:09 AM, Kurt Van Dijck wrote:
>> On 06/29/2017 05:36 PM, Kurt Van Dijck wrote:
>>
>> mcan@0 {
>> ...
>> fixed-transceiver {
>>max-canfd-speed = <2000>
>> };
>> ...
>> };
>>>
>>> Since when would a transceiver support different speeds for CAN & CANFD?
>>
>> When I say CAN I'm referring to CAN 2.0 specification which mentioned
>> speeds upto 1 Mbit/s. While CAN FD supports higher bitrates.
>
> linux-can is not necessarily restricted to CAN 2.0B?
>
>>
>>> No transceivers were available, but they are now.
>>> I see no datalink problem applying 2MBit for regular CAN with apropriate
>>> physical layer, and CAN does not predefine the physical layer
>>> (advise != define).
>>>
>>> IMHO,
>>> fixed-transceiver {
>>> max-arbitration-speed = <200>
>>> max-data-speed = <400>
>>> };
>>> is way better to describe the hardware.
>>> Regular CAN chips would not consider max-data-speed...
>>
>> What is arbitration speed?
>
> CANFD remains similar during the arbitration phase (when the CAN id is
> sent on the wire), and after that allows to switch to a higher 'data'
> speed because the round-trip wire restrictions during arbitration
> don't apply anymore.
>
>>
>> Also if I understand you correctly then I agree drivers for traditional
>> CAN wouldn't care about this subnode. Although it may be helpful for
>> max-data-speed to become max-canfd-speed or something along those lines.
>> Just so the property's purpose is clear.
>
> Transceivers exist that don't support 1MB either.
> naming the speeds max-arbitration-speed and max-data-speed makes this
> OF nodes usable for that kind of CAN 2.0 restrtications too.
>
> Of course, CAN 2.0 chips only consider max-arbitration-speed as that
> applies to the whole wire bitstream, where as CANFD considers both.
>
> What I understand of your proposal is that max-arbitration-speed is
> 'fixed to 1MB anyway', and that assumption has been proven not
> universally applicable with CAN2.0 transceivers already.
>
> I found the name 'max-canfd-speed' a bit dubious as CANFD relies on
> 'flexible datarate'. transceivers may not necessarily support the same
> speed for both arbitration and data.
> So I propose to replace it with 'max-data-speed'
Thanks for the explanation. Makes sense.
>
> Kind regards,
> Kurt
>
>>>
>>> Kind regards,
>>> Kurt
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CAN-FD Transceiver Limitations
On 06/29/2017 05:36 PM, Kurt Van Dijck wrote:
mcan@0 {
...
fixed-transceiver {
max-canfd-speed = <2000>
};
...
};
>
> Since when would a transceiver support different speeds for CAN & CANFD?
When I say CAN I'm referring to CAN 2.0 specification which mentioned
speeds upto 1 Mbit/s. While CAN FD supports higher bitrates.
> No transceivers were available, but they are now.
> I see no datalink problem applying 2MBit for regular CAN with apropriate
> physical layer, and CAN does not predefine the physical layer
> (advise != define).
>
> IMHO,
> fixed-transceiver {
> max-arbitration-speed = <200>
> max-data-speed = <400>
> };
> is way better to describe the hardware.
> Regular CAN chips would not consider max-data-speed...
What is arbitration speed?
Also if I understand you correctly then I agree drivers for traditional
CAN wouldn't care about this subnode. Although it may be helpful for
max-data-speed to become max-canfd-speed or something along those lines.
Just so the property's purpose is clear.
>
> Kind regards,
> Kurt
>
Re: CAN-FD Transceiver Limitations
+device tree mailing list
Hi Andrew
On 06/29/2017 10:41 AM, Andrew Lunn wrote:
>> Transceivers for CAN are not apart of any model. Traditional CAN didn't
>> have a problem because all transceivers from my understanding supported
>> the maximum speed of 1 Mbps defined by the spec. However, with the
>> introduction of CAN Flexible Datarate mode it seems that for
>> transceivers that supported CAN-FD the maximum supported speeds vary.
>
> So transceivers are dumb devices, nothing to configure, so no need to
> have a driver for them.
>
>> Now that I think of it
>> you also can't determine if the transceiver supports CAN-FD in the first
>> place. IP that supports CAN-FD is backwards compatible with standard
>> CAN. Therefore, its feasible that you may even use a transceiver that
>> doesn't support CAN-FD. So I would think something like the below would
>> be needed.
>>
>> mcan@0 {
>> ...
>> fixed-transceiver {
>>max-canfd-speed = <2000>
>> };
>> ...
>> };
>
> Are there likely to be other transceiver properties? Adding a subnode
> may not make sense if this is going to be the only property.
>
> Also, 2KHz is not very fast :-)
>
> Taking a quick look in Documentation/devicetree/bindings/net/can, it
> seems a bit of a wild west. No standardization, no central binding
> which CAN drivers are expected to support, etc. This sounds like a
> generic problem, not an mcan problem. So document this property
> centrally, implement the parsing of it centrally, etc, to encourage
> other CAN drivers to use it, rather than re-invent the wheel.
As of now its the only property that I think is needed. In general from
past experience and threads it seems that people are fairly adamant
about having DT mimic hardware as closely as possible. So since from a
hardware perspective the transceiver is an external device that is
connected to the CAN IP it would make sense for a subnode to be used to
model it. But either way works for me. If the device tree folks do not
care for subnode to be created then I can just add a property.
Also I agree that attempting to make this optional property/subnode
generic to all of CAN would be preferable. Another not sure if its
feasible yet without standardization being first forced across all CAN
drivers.
>
> Andrew
>
Re: CAN-FD Transceiver Limitations
Hi Andrew
On 06/29/2017 09:21 AM, Andrew Lunn wrote:
> On Wed, Jun 28, 2017 at 05:14:42PM -0500, Franklin S Cooper Jr wrote:
>> Hi All,
>>
>> The various CAN transceivers I've seen that support CAN-FD appear to be
>> fairly limited in terms of their supported max speed. I've seen some
>> transceivers that only support upto 2 Mbps while others support up to 5
>> Mbps. This is a problem when the SoC's CAN IP can support even higher
>> values than the transceiver.
>>
>> Ideally I would think the MCAN driver should at the very least know what
>> the maximum speed supported by the transceiver it is connected to.
>> Therefore, either throwing an error if a request for a speed above the
>> transceiver capability or lower the requested speed to what ever the
>> transceiver is capability of doing.
>
> Hi Franklin
>
>> In either case I do not know if it makes sense to add a DT property
>> within the MCAN driver or create another subnode that contains this
>> information. For example I see some ethernet drivers support
>> "fixed-link" subnode which is trying to solve a similar issue.
>
> Hi Franklin
>
> I would say fixed-link is trying to solve a different issue. You use
> fixed-link when you don't have a PHY connected to the Ethernet MAC. A
> MAC driver will normally tell the PHY driver what speeds its supports,
> and ask the PHY to negotiate a speed both the MAC and PHY supports
> with the peer device. The MAC then gets told of the resulting speed.
> If this PHY does not exist, you cannot ask it to perform auto-neg, you
> have no idea what the peer is capable of. Hence you add a virtual PHY
> using fixed-link, which always returns the same speed. This works
> because if you don't have a PHY, the MAC is generally connected to
> another MAC on the same board, typically an Ethernet switch. Hence the
> speed is a board property and fixed.
>
> You appear to have a different issue. I don't know the CAN
> subsystem. Is the transceiver part of the model? Is there an API
> between the CAN-MAC and the CAN-transceiver? It sounds like you need
> to add an API call from the MAC driver to the transceiver driver to
> ask it what it is capable of. I don't see this as being a DT property.
> The transceiver should know its own capabilities. And you have to
> think about non-DT systems, e.g. CAN devices on USB dongles.
Transceivers for CAN are not apart of any model. Traditional CAN didn't
have a problem because all transceivers from my understanding supported
the maximum speed of 1 Mbps defined by the spec. However, with the
introduction of CAN Flexible Datarate mode it seems that for
transceivers that supported CAN-FD the maximum supported speeds vary.
Unfortunately, their is no way to communicate with the transceiver to
understand its capabilities. You don't program it or configure a
transceiver. It simply has a fixed configuration. Now that I think of it
you also can't determine if the transceiver supports CAN-FD in the first
place. IP that supports CAN-FD is backwards compatible with standard
CAN. Therefore, its feasible that you may even use a transceiver that
doesn't support CAN-FD. So I would think something like the below would
be needed.
mcan@0 {
...
fixed-transceiver {
max-canfd-speed = <2000>
};
...
};
So the mcan driver can then check for this subnode and if found read in
the max canfd speed that is supported. A value of 0 could be used to
indicate that CAN-FD isn't supported by the transceiver.
>
> Andrew
>
CAN-FD Transceiver Limitations
Hi All, The various CAN transceivers I've seen that support CAN-FD appear to be fairly limited in terms of their supported max speed. I've seen some transceivers that only support upto 2 Mbps while others support up to 5 Mbps. This is a problem when the SoC's CAN IP can support even higher values than the transceiver. Ideally I would think the MCAN driver should at the very least know what the maximum speed supported by the transceiver it is connected to. Therefore, either throwing an error if a request for a speed above the transceiver capability or lower the requested speed to what ever the transceiver is capability of doing. In either case I do not know if it makes sense to add a DT property within the MCAN driver or create another subnode that contains this information. For example I see some ethernet drivers support "fixed-link" subnode which is trying to solve a similar issue. Should I go with that approach? If so would it make sense to reuse fixed-link even though majority of its properties aren't applicable? Or should I create something similar such as fixed-can-transceiver?
Re: Boot failure when using NFS on OMAP based evms
On 04/06/2016 09:22 PM, Willem de Bruijn wrote:
> On Wed, Apr 6, 2016 at 7:12 PM, Franklin S Cooper Jr. <fcoo...@ti.com> wrote:
>> Hi All,
>>
>> Currently linux-next is failing to boot via NFS on my AM335x GP evm,
>> AM437x GP evm and Beagle X15. I bisected the problem down to the commit
>> "udp: remove headers from UDP packets before queueing".
>>
>> I had to revert the following three commits to get things working again:
>>
>> e6afc8ace6dd5cef5e812f26c72579da8806f5ac
>> udp: remove headers from UDP packets before queueing
>>
>> 627d2d6b550094d88f9e518e15967e7bf906ebbf
>> udp: enable MSG_PEEK at non-zero offset
>>
>> b9bb53f3836f4eb2bdeb3447be11042bd29c2408
>> sock: convert sk_peek_offset functions to WRITE_ONCE
>>
>
> Thanks for the report, and apologies for breaking your configuration.
> I had missed that sunrpc can dequeue skbs from a udp receive
> queue and makes assumptions about the layout of those packets. rxrpc
> does the same. From what I can tell so far, those are the only two
> protocols that do this. I have verified that the following fixes rxrpc for me
>
> --- a/net/rxrpc/ar-input.c
> +++ b/net/rxrpc/ar-input.c
> @@ -612,9 +612,9 @@ int rxrpc_extract_header(struct rxrpc_skb_priv
> *sp, struct sk_buff *skb)
> struct rxrpc_wire_header whdr;
>
> /* dig out the RxRPC connection details */
> - if (skb_copy_bits(skb, sizeof(struct udphdr), , sizeof(whdr)) <
> 0)
> + if (skb_copy_bits(skb, 0, , sizeof(whdr)) < 0)
> return -EBADMSG;
> - if (!pskb_pull(skb, sizeof(struct udphdr) + sizeof(whdr)))
> + if (!pskb_pull(skb, sizeof(whdr)))
> BUG();
>
> I have not yet been able to reproduce the sunrpc/nfs issue, but I
> suspect that the following might fix it. I will try to create an NFS
> setup.
>
> diff --git a/net/sunrpc/socklib.c b/net/sunrpc/socklib.c
> index 2df87f7..8ab40ba 100644
> --- a/net/sunrpc/socklib.c
> +++ b/net/sunrpc/socklib.c
> @@ -155,7 +155,7 @@ int csum_partial_copy_to_xdr(struct xdr_buf *xdr,
> struct sk_buff *skb)
> struct xdr_skb_reader desc;
>
> desc.skb = skb;
> - desc.offset = sizeof(struct udphdr);
> + desc.offset = 0;
> desc.count = skb->len - desc.offset;
>
> if (skb_csum_unnecessary(skb))
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 1413cdc..71d6072 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -617,7 +617,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
> svsk->sk_sk->sk_stamp = skb->tstamp;
> set_bit(XPT_DATA, >sk_xprt.xpt_flags); /* there may be
> more data... */
>
> - len = skb->len - sizeof(struct udphdr);
> + len = skb->len;
> rqstp->rq_arg.len = len;
>
> rqstp->rq_prot = IPPROTO_UDP;
> @@ -641,8 +641,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
> skb_free_datagram_locked(svsk->sk_sk, skb);
> } else {
> /* we can use it in-place */
> - rqstp->rq_arg.head[0].iov_base = skb->data +
> - sizeof(struct udphdr);
> + rqstp->rq_arg.head[0].iov_base = skb->data;
> rqstp->rq_arg.head[0].iov_len = len;
> if (skb_checksum_complete(skb))
> goto out_free;
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 65e7595..c1fc7b2 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -995,15 +995,14 @@ static void xs_udp_data_read_skb(struct rpc_xprt *xprt,
> u32 _xid;
> __be32 *xp;
>
> - repsize = skb->len - sizeof(struct udphdr);
> + repsize = skb->len;
> if (repsize < 4) {
> dprintk("RPC: impossible RPC reply size %d!\n",
> repsize);
> return;
> }
>
>
>
> /* Copy the XID from the skb... */
> - xp = skb_header_pointer(skb, sizeof(struct udphdr),
> - sizeof(_xid), &_xid);
> + xp = skb_header_pointer(skb, 0, sizeof(_xid), &_xid);
> if (xp == NULL)
> return;
>
Thank you for your quick response. I verified with all of the above
suggested changes that NFS works again on my 3 evms.
Boot failure when using NFS on OMAP based evms
Hi All, Currently linux-next is failing to boot via NFS on my AM335x GP evm, AM437x GP evm and Beagle X15. I bisected the problem down to the commit "udp: remove headers from UDP packets before queueing". I had to revert the following three commits to get things working again: e6afc8ace6dd5cef5e812f26c72579da8806f5ac udp: remove headers from UDP packets before queueing 627d2d6b550094d88f9e518e15967e7bf906ebbf udp: enable MSG_PEEK at non-zero offset b9bb53f3836f4eb2bdeb3447be11042bd29c2408 sock: convert sk_peek_offset functions to WRITE_ONCE I'm using omap2plus_defconfig for my config. Below bootlogs are from my AM335x GP evm: Working http://pastebin.ubuntu.com/15661989/ (Linux-next 3 patches reverted) Failing http://pastebin.ubuntu.com/15661318/ (Linux-next)
Re: Keystone 2 boards boot failure
On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>
> On 02/02/2016 05:26 PM, Arnd Bergmann wrote:
>> On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
>>> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
>>>> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
>>>>> Yes. Here is a boot log on the latest master with the below
>>>>> three patches reverted.
>>>>> http://pastebin.com/W7RWSHpE (Working)
>>>>>
>>>>> I reverted these three patches. The two latest patches seem
>>>>> to be trying to correct/expand upon the last patch on this list.
>>>>>
>>>>> commit 958d104e3d40eef5148c402887138f6594ff7e1e
>>>>> netcp: fix regression in receive processing
>>>>>
>>>>> commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
>>>>> netcp: add more __le32 annotations
>>>>>
>>>>> commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
>>>>> netcp: try to reduce type confusion in descriptors
>>>>>
>>>> The middle patch should have no effect on generated code, so I'm ignoring
>>>> that for now.
>>>>
>>>> The next thing to rule out is an endianess bug. I assume you
>>>> are running this on with a little-endian kernel, correct? If
>>>> you are running big-endian, the base assumption that the driver needs
>>>> to swap the data was flawed and that portion needs to be done.
>>>>
>>>> If you are running little-endian 32-bit, please try the partial
>>>> revert below, which just undoes the attempt to make it work with
>>>> 64-bit kernels.
>>> Keystone 2 devices are little-endian 32-bit devices.
>> I meant the kernel you are running on it, not the hardware.
>> You should always be able to run both a big-endian kernel and
>> a littl-endian kernel on any ARMv7 machine, and a couple of
>> platforms use 64-bit physical addresses even on 32-bit machines
>> (with the normal 32-bit instruction set).
> I'm not sure if Keystone 2 devices support this or if we
> have support for this. I'll have to double check.
>> I wasn't completely sure if there are already keystone-derived
>> products with 64-bit CPU cores, but I guess the driver would
>> fail really badly on those (with or without the patch).
>>
>>> This partial revert fixes the boot problem for me.
>> Ok.
>>
>>
>> I tried to create a smaller version and stumbled over
>> a typo, maybe that's the whole problem. Can you try this one:
>>
>> diff --git a/drivers/net/ethernet/ti/netcp_core.c
>> b/drivers/net/ethernet/ti/netcp_core.c
>> index c61d66d38634..8490804416dd 100644
>> --- a/drivers/net/ethernet/ti/netcp_core.c
>> +++ b/drivers/net/ethernet/ti/netcp_core.c
>> @@ -167,7 +167,7 @@ static void set_pad_info(u32 pad0, u32 pad1, u32 pad2,
>> struct knav_dma_desc *des
>> {
>> desc->pad[0] = cpu_to_le32(pad0);
>> desc->pad[1] = cpu_to_le32(pad1);
>> -desc->pad[2] = cpu_to_le32(pad1);
>> +desc->pad[2] = cpu_to_le32(pad2);
>> }
>>
>> static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>>
>>
>> Arnd
> So only making this change on the latest master with no
> other changes I see the boot problem again.
+Grygorii
Re: Keystone 2 boards boot failure
On 02/03/2016 08:21 AM, Grygorii Strashko wrote:
> Hi Arnd,
>
> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
>> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>>> On 02/02/2016 05:26 PM, Arnd Bergmann wrote:
>>>> On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
>>>>> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
>>>>>> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
>>>>>>> Yes. Here is a boot log on the latest master with the below
>>>>>>> three patches reverted.
>>>>>>> http://pastebin.com/W7RWSHpE (Working)
>>>>>>>
>>>>>>> I reverted these three patches. The two latest patches seem
>>>>>>> to be trying to correct/expand upon the last patch on this list.
>>>>>>>
>>>>>>> commit 958d104e3d40eef5148c402887138f6594ff7e1e
>>>>>>> netcp: fix regression in receive processing
>>>>>>>
>>>>>>> commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
>>>>>>> netcp: add more __le32 annotations
>>>>>>>
>>>>>>> commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
>>>>>>> netcp: try to reduce type confusion in descriptors
>>>>>>>
>>>>>> The middle patch should have no effect on generated code, so I'm ignoring
>>>>>> that for now.
>>>>>>
>>>>>> The next thing to rule out is an endianess bug. I assume you
>>>>>> are running this on with a little-endian kernel, correct? If
>>>>>> you are running big-endian, the base assumption that the driver needs
>>>>>> to swap the data was flawed and that portion needs to be done.
>>>>>>
>>>>>> If you are running little-endian 32-bit, please try the partial
>>>>>> revert below, which just undoes the attempt to make it work with
>>>>>> 64-bit kernels.
>>>>> Keystone 2 devices are little-endian 32-bit devices.
>>>> I meant the kernel you are running on it, not the hardware.
>>>> You should always be able to run both a big-endian kernel and
>>>> a littl-endian kernel on any ARMv7 machine, and a couple of
>>>> platforms use 64-bit physical addresses even on 32-bit machines
>>>> (with the normal 32-bit instruction set).
>>> I'm not sure if Keystone 2 devices support this or if we
>>> have support for this. I'll have to double check.
>>>> I wasn't completely sure if there are already keystone-derived
>>>> products with 64-bit CPU cores, but I guess the driver would
>>>> fail really badly on those (with or without the patch).
>>>>
>>>>> This partial revert fixes the boot problem for me.
>>>> Ok.
>>>>
>>>>
>>>> I tried to create a smaller version and stumbled over
>>>> a typo, maybe that's the whole problem. Can you try this one:
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/netcp_core.c
>>>> b/drivers/net/ethernet/ti/netcp_core.c
>>>> index c61d66d38634..8490804416dd 100644
>>>> --- a/drivers/net/ethernet/ti/netcp_core.c
>>>> +++ b/drivers/net/ethernet/ti/netcp_core.c
>>>> @@ -167,7 +167,7 @@ static void set_pad_info(u32 pad0, u32 pad1, u32 pad2,
>>>> struct knav_dma_desc *des
>>>> {
>>>>desc->pad[0] = cpu_to_le32(pad0);
>>>>desc->pad[1] = cpu_to_le32(pad1);
>>>> - desc->pad[2] = cpu_to_le32(pad1);
>>>> + desc->pad[2] = cpu_to_le32(pad2);
>>>> }
>>>>
>>>> static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>>>>
>>>>
>>> So only making this change on the latest master with no
>>> other changes I see the boot problem again.
> yep. I can confirm that.
>
> Also, I'm today came up with the similar fix that you've proposed before in
> this thread.
> So, Could we move forward this way?
>
>
> From 8280895f01b33edba303e7374431bef47630f26c Mon Sep 17 00:00:00 2001
> From: Grygorii Strashko <grygorii.stras...@ti.com>
> Date: Wed, 3 Feb 2016 15:11:40 +0200
> Subject: [PATCH] net: ti: netcp: restore get/set_pad_info() functionality
>
> The commit 899077791403 ("netcp: try to reduce type confusion in descriptors")
> introduces a regression in Kernel 4.5-rc1 as it breaks
> get/set_pad_info() functionality.
>
> The TI NETCP driver
Re: Keystone 2 boards boot failure
On 02/02/2016 02:41 PM, Arnd Bergmann wrote: > On Tuesday 02 February 2016 10:50:41 Franklin S Cooper Jr. wrote: >> Latest mainline is currently failing to boot for Keystone 2 >> Hawking but I'm assuming its true for other Keystone 2 >> boards. Bisect shows that this issue popped up after the >> patch "netcp: try to reduce type confusion in descriptors" >> (commit 89907779) was introduced. There was another patch >> "netcp: fix regression in receive processing" that seems to >> fix some bugs that the prior patch introduced however it >> still did not resolve the boot failure and was documented as >> not being tested. >> >> Should we revert these commits or does anyone have any >> suggestions on how to fix these failures? I would be more >> than happy to test any fix. >> > Have you tried to see if a revert fixes the problem on a > current kernel? Yes. Here is a boot log on the latest master with the below three patches reverted. http://pastebin.com/W7RWSHpE (Working) I reverted these three patches. The two latest patches seem to be trying to correct/expand upon the last patch on this list. commit 958d104e3d40eef5148c402887138f6594ff7e1e netcp: fix regression in receive processing commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8 netcp: add more __le32 annotations commit 899077791403ff7a2d8cfaa87bd1a82d729463e2 netcp: try to reduce type confusion in descriptors > > Arnd
Re: Keystone 2 boards boot failure
On 02/02/2016 05:26 PM, Arnd Bergmann wrote:
> On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
>> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
>>> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
>>>> Yes. Here is a boot log on the latest master with the below
>>>> three patches reverted.
>>>> http://pastebin.com/W7RWSHpE (Working)
>>>>
>>>> I reverted these three patches. The two latest patches seem
>>>> to be trying to correct/expand upon the last patch on this list.
>>>>
>>>> commit 958d104e3d40eef5148c402887138f6594ff7e1e
>>>> netcp: fix regression in receive processing
>>>>
>>>> commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
>>>> netcp: add more __le32 annotations
>>>>
>>>> commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
>>>> netcp: try to reduce type confusion in descriptors
>>>>
>>> The middle patch should have no effect on generated code, so I'm ignoring
>>> that for now.
>>>
>>> The next thing to rule out is an endianess bug. I assume you
>>> are running this on with a little-endian kernel, correct? If
>>> you are running big-endian, the base assumption that the driver needs
>>> to swap the data was flawed and that portion needs to be done.
>>>
>>> If you are running little-endian 32-bit, please try the partial
>>> revert below, which just undoes the attempt to make it work with
>>> 64-bit kernels.
>> Keystone 2 devices are little-endian 32-bit devices.
> I meant the kernel you are running on it, not the hardware.
> You should always be able to run both a big-endian kernel and
> a littl-endian kernel on any ARMv7 machine, and a couple of
> platforms use 64-bit physical addresses even on 32-bit machines
> (with the normal 32-bit instruction set).
I'm not sure if Keystone 2 devices support this or if we
have support for this. I'll have to double check.
>
> I wasn't completely sure if there are already keystone-derived
> products with 64-bit CPU cores, but I guess the driver would
> fail really badly on those (with or without the patch).
>
>> This partial revert fixes the boot problem for me.
> Ok.
>
>
> I tried to create a smaller version and stumbled over
> a typo, maybe that's the whole problem. Can you try this one:
>
> diff --git a/drivers/net/ethernet/ti/netcp_core.c
> b/drivers/net/ethernet/ti/netcp_core.c
> index c61d66d38634..8490804416dd 100644
> --- a/drivers/net/ethernet/ti/netcp_core.c
> +++ b/drivers/net/ethernet/ti/netcp_core.c
> @@ -167,7 +167,7 @@ static void set_pad_info(u32 pad0, u32 pad1, u32 pad2,
> struct knav_dma_desc *des
> {
> desc->pad[0] = cpu_to_le32(pad0);
> desc->pad[1] = cpu_to_le32(pad1);
> - desc->pad[2] = cpu_to_le32(pad1);
> + desc->pad[2] = cpu_to_le32(pad2);
> }
>
> static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>
>
> Arnd
So only making this change on the latest master with no
other changes I see the boot problem again.
Keystone 2 boards boot failure
Hi All, Latest mainline is currently failing to boot for Keystone 2 Hawking but I'm assuming its true for other Keystone 2 boards. Bisect shows that this issue popped up after the patch "netcp: try to reduce type confusion in descriptors" (commit 89907779) was introduced. There was another patch "netcp: fix regression in receive processing" that seems to fix some bugs that the prior patch introduced however it still did not resolve the boot failure and was documented as not being tested. Should we revert these commits or does anyone have any suggestions on how to fix these failures? I would be more than happy to test any fix. Bootlogs: Bootlog 4.5.0-rc2: http://pastebin.com/bsH4u656 (Failed) Bootlog 4.5.0-rc1: http://pastebin.com/MwY0eS7x (Failed) Bootlog 4.4: http://pastebin.com/yfBND9Vi (Passed)
