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 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/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: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
On 10/18/2017 08:24 AM, Sekhar Nori wrote: > Hi Marc, > > On Wednesday 18 October 2017 06:14 PM, Marc Kleine-Budde wrote: >> On 09/21/2017 02:48 AM, Franklin S Cooper Jr wrote: >>> >>> >>> On 09/20/2017 04:37 PM, Mario Hüttel wrote: >>>> >>>> >>>> On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote: >>>>> Hi Wenyou, >>>>> >>>>> On 09/17/2017 10:47 PM, Yang, Wenyou wrote: >>>>>> >>>>>> On 2017/9/14 13:06, Sekhar Nori wrote: >>>>>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote: >>>>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote: >>>>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only >>>>>>>>> resulted in errors. Scoping the signals I noticed that only a single >>>>>>>>> bit >>>>>>>>> was being transmitted and with a bit more investigation realized the >>>>>>>>> actual >>>>>>>>> MCAN IP would go back to initialization mode automatically. >>>>>>>>> >>>>>>>>> It appears this issue is due to the MCAN needing to use the >>>>>>>>> Transmitter >>>>>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this >>>>>>>>> mode is used the User's Guide indicates that the Transmitter Delay >>>>>>>>> Compensation Offset register should be set. The document mentions >>>>>>>>> that this >>>>>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq). >>>>>>>>> >>>>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document >>>>>>>>> indicates >>>>>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps. >>>>>>>>> Therefore, only enable this mode and only set TDCO when the data bit >>>>>>>>> rate >>>>>>>>> is above 2.5 Mbps. >>>>>>>>> >>>>>>>>> Signed-off-by: Franklin S Cooper Jr >>>>>>>>> --- >>>>>>>>> I'm pretty surprised that this hasn't been implemented already since >>>>>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP >>>>>>>>> supports up to 10 Mbps. >>>>>>>>> >>>>>>>>> So it will be nice to get comments from users of this driver to >>>>>>>>> understand >>>>>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this >>>>>>>>> patch. >>>>>>>>> If they haven't what did they do to get around it if they needed >>>>>>>>> higher >>>>>>>>> speeds. >>>>>>>>> >>>>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to >>>>>>>>> insure >>>>>>>>> everything still works at 5 Mbps which is the max speed of my CAN >>>>>>>>> transceiver. >>>>>>>> ping. Anyone has any thoughts on this? >>>>>>> I added Dong who authored the m_can driver and Wenyou who added the only >>>>>>> in-kernel user of the driver for any help. >>>>>> I tested it on SAMA5D2 Xplained board both with and without this patch, >>>>>> both work with the 4M bps data bit rate. >>>>> Thank you for testing this out. Its interesting that you have been able >>>>> to use higher speeds without this patch. What is the CAN transceiver >>>>> being used on the SAMA5D2 Xplained board? I tried looking at the >>>>> schematic but it seems the CAN signals are used on an extension board >>>>> which I can't find the schematic for. Also do you mind sharing your test >>>>> setup? Were you doing a short point to point test? >>>>> >>>>> Thank You, >>>>> Franklin >>>> Hello Franklin, >>>> >>>> your patch definitely makes sense. >>>> >>>> I forgot the TDC in my patches because it was not present in the >>>> previous driver versions and because I didn'
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: [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: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
On 09/20/2017 04:37 PM, Mario Hüttel wrote: > > > On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote: >> Hi Wenyou, >> >> On 09/17/2017 10:47 PM, Yang, Wenyou wrote: >>> >>> On 2017/9/14 13:06, Sekhar Nori wrote: >>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote: >>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote: >>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only >>>>>> resulted in errors. Scoping the signals I noticed that only a single >>>>>> bit >>>>>> was being transmitted and with a bit more investigation realized the >>>>>> actual >>>>>> MCAN IP would go back to initialization mode automatically. >>>>>> >>>>>> It appears this issue is due to the MCAN needing to use the Transmitter >>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this >>>>>> mode is used the User's Guide indicates that the Transmitter Delay >>>>>> Compensation Offset register should be set. The document mentions >>>>>> that this >>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq). >>>>>> >>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document >>>>>> indicates >>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps. >>>>>> Therefore, only enable this mode and only set TDCO when the data bit >>>>>> rate >>>>>> is above 2.5 Mbps. >>>>>> >>>>>> Signed-off-by: Franklin S Cooper Jr >>>>>> --- >>>>>> I'm pretty surprised that this hasn't been implemented already since >>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP >>>>>> supports up to 10 Mbps. >>>>>> >>>>>> So it will be nice to get comments from users of this driver to >>>>>> understand >>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this >>>>>> patch. >>>>>> If they haven't what did they do to get around it if they needed higher >>>>>> speeds. >>>>>> >>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to >>>>>> insure >>>>>> everything still works at 5 Mbps which is the max speed of my CAN >>>>>> transceiver. >>>>> ping. Anyone has any thoughts on this? >>>> I added Dong who authored the m_can driver and Wenyou who added the only >>>> in-kernel user of the driver for any help. >>> I tested it on SAMA5D2 Xplained board both with and without this patch, >>> both work with the 4M bps data bit rate. >> Thank you for testing this out. Its interesting that you have been able >> to use higher speeds without this patch. What is the CAN transceiver >> being used on the SAMA5D2 Xplained board? I tried looking at the >> schematic but it seems the CAN signals are used on an extension board >> which I can't find the schematic for. Also do you mind sharing your test >> setup? Were you doing a short point to point test? >> >> Thank You, >> Franklin > Hello Franklin, > > your patch definitely makes sense. > > I forgot the TDC in my patches because it was not present in the > previous driver versions and because I didn't encounter any > problems when testing it myself. > > The error is highly dependent on the hardware (transceiver) setup. > So it is definitely possible that some people don't encounter errors > without your patch. So the Transmission Delay Compensation feature Value register is suppose to take into consideration the transceiver delay automatically and add the value of TDCO on top of that. So why would TDCO be dependent on the transceiver? I've heard conflicting things regarding TDC so any clarification on what actually impacts it would be appreciated. Also part of the issue I'm having is how can we properly configure TDCO? Configuring TDCO is essentially figuring out what Secondary Sample Point to use. However, it is unclear what value to set SSP to and which use cases a given SSP will work or doesn't work. I've seen various recommendations from Bosch on choosing SSP but ultimately it seems they suggestion "real world testing" to come up with a proper value. Not setting TDCO causes problems for my device and improperly setting TDCO causes problems for my device. So its likely any value I use could end up breaking something for someone else. Currently I leaning to a DT property that can be used for setting SSP. Perhaps use a generic default value and allow individuals to override it via DT? > > Could you clarify what you meant with >> Scoping the signals I noticed that only a single bit was being transmitted > > Do you mean one data bit (high bit rate) or did the core already fail > in the arbitration phase? A single bit during the arbitration phase. Essentially the Start of Frame bit is sent and then nothing else and the IP resets. > > There is also another aspect that can lead to errors: > > If the CAN clock 'cclk' is above the frequency of the interface/logic > clock 'hclk', the clock domain crossing of the CAN messages can't > work properly. However, I will throw this topic as an extra e-mail into > the round. > > Mario > > >
Re: [v2,3/3] can: m_can: Add PM Runtime
On 08/24/2017 03:30 AM, Sekhar Nori wrote:
> + OMAP mailing list
>
> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
>> Add support for PM Runtime which is the new way to handle managing clocks.
>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>> management approach in place.
>
> Since hclk is the interface clock, I think at a minimum there needs to
> be an assumption that pm_runtime_get_sync() will enable that clock and
> make the module ready for access.
>
> The only in-kernel user of this driver seems to be an atmel SoC. I am
> ccing folks who added support for M_CAN on that SoC to see if hclk
> management can be left to pm_runtime*() on their SoC.
>
> If they are okay, I think its a good to atleast drop explicit hclk
> enable disable in the driver. That way, we avoid double enable/disable
> of interface clock (hclk).
Wenyou,
Do you foresee this being a problem on your board? If not I can send a
v3 removing these clk_enable and clk_disable calls and it would be great
if you can test it out and provide your tested by.
>
>>
>> PM_RUNTIME is required by OMAP based devices to handle clock management.
>> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
>> to work with this driver.
>>
>> Signed-off-by: Franklin S Cooper Jr <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,3/3] can: m_can: Add PM Runtime
On 08/24/2017 03:30 AM, Sekhar Nori wrote:
> + OMAP mailing list
>
> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
>> Add support for PM Runtime which is the new way to handle managing clocks.
>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>> management approach in place.
>
> Since hclk is the interface clock, I think at a minimum there needs to
> be an assumption that pm_runtime_get_sync() will enable that clock and
> make the module ready for access.
>
> The only in-kernel user of this driver seems to be an atmel SoC. I am
> ccing folks who added support for M_CAN on that SoC to see if hclk
> management can be left to pm_runtime*() on their SoC.
>
> If they are okay, I think its a good to atleast drop explicit hclk
> enable disable in the driver. That way, we avoid double enable/disable
> of interface clock (hclk).
Wenyou,
Do you foresee this being a problem on your board? If not I can send a
v3 removing these clk_enable and clk_disable calls and it would be great
if you can test it out and provide your tested by.
>
>>
>> PM_RUNTIME is required by OMAP based devices to handle clock management.
>> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
>> to work with this driver.
>>
>> Signed-off-by: Franklin S Cooper Jr
>
> Thanks,
> Sekhar
>
>> ---
>> drivers/net/can/m_can/m_can.c | 13 +
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index ea48e59..93e23f5 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -23,6 +23,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -633,11 +634,15 @@ static int m_can_clk_start(struct m_can_priv *priv)
>> if (err)
>> clk_disable_unprepare(priv->hclk);
>>
>> +pm_runtime_get_sync(priv->device);
>> +
>> return err;
>> }
>>
>> static void m_can_clk_stop(struct m_can_priv *priv)
>> {
>> +pm_runtime_put_sync(priv->device);
>> +
>> clk_disable_unprepare(priv->cclk);
>> clk_disable_unprepare(priv->hclk);
>> }
>> @@ -1582,6 +1587,8 @@ static int m_can_plat_probe(struct platform_device
>> *pdev)
>> /* Enable clocks. Necessary to read Core Release in order to determine
>> * M_CAN version
>> */
>> +pm_runtime_enable(>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: [v2,1/3] can: m_can: Make hclk optional
On 08/24/2017 03:00 AM, Sekhar Nori wrote:
> + some OMAP folks and Linux OMAP list
>
> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
>> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
>> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
>> interface clock is managed by hwmod driver via pm_runtime_get and
>> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
>> and thus the driver shouldn't fail if this clock isn't found.
>
> I agree that hclk is defined as interface clock for M_CAN IP on DRA76x.
>
> However, there may be a need for the driver to know the value of hclk to
> properly configure the RAM watchdog register which has a counter
> counting down using hclk. Looks like the driver does not use the RAM
> watchdog today. But if there is a need to configure it in future, it
> might be a problem.
Honestly the RAM watchdog seems like a fundamental design problem.
This RAM watchdog seems to be used in case a request to access the
message ram is made but it hangs for what ever reason. Its even more
complicated since the Message RAM is external to the MCAN IP so its
implementation or how its handled probably differs from device to
device. From example say you do have this error it isn't clear how you
would recover from it. A logically answer would be to reset the entire
IP but that also assumes that Message RAM will be reset along with the
ip which likely depends on each SoC.
But if a readl/writel command hangs will the kernel eventually throw an
error on its on or will the driver just hang? If it does hang can a
driver in the ISR do something to properly terminate the driver or even
recover from it?
>
> Is there a restriction in OMAP architecture against passing the
> interface clock also in the 'clocks' property in DT. I have not tried it
> myself, but wonder if you hit an issue that led to this patch.
No but not passing the interface clock is typical.
>
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Version 2 changes:
>> Used NULL instead of 0 for unused hclk handle
>>
>> drivers/net/can/m_can/m_can.c | 9 +++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index f4947a7..ea48e59 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device
>> *pdev)
>> hclk = devm_clk_get(>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: [PATCH v2 1/3] can: m_can: Make hclk optional
On 09/20/2017 05:00 PM, Mario Hüttel wrote:
>> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
>> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
>> interface clock is managed by hwmod driver via pm_runtime_get and
>> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
>> and thus the driver shouldn't fail if this clock isn't found.
> I also agree in this point.
> However, there's a problem I want to point out:
>
> The M_CAN can only function correctly, if the condition
> 'hclk >= cclk' holds true.
>
> The internal clock domain crossing can fail if this condition
> is violated.
>
> I thought about adding the condition to the driver to ensure
> correct operation. But I had some problems:
>
> 1. Determine the clock rates:
> The devices you've mentioned above don't have an assigned
> hclk. Is there still a possibility to get the clock frequency?
I believe interface clocks via hwmod aren't exposed to drivers. So the
only way to get access to the clock frequency is to add this interface
clock to dt.
>
> 2. What to do if 'hclk < cclk'?
> Is there a general way to process such an error? - I think not.
> Is a simple warning in case of an error enough?
>
> Is there a way of ensuring that the condition is always met at all?
>
> I think it is quite unlikely that the condition is violated, because
> devices that actually run Linux usually have (bus) clock rates that
> are high enough to ensure the correct operation.
>
> Would it be safe to just ignore this possible fault?
I think alot of peripherals have similar constraints when there is an
interface and functional clock. However, I haven't seen drivers verify
that this kind of constraint is properly met. Personally I think its a
valid fault but one that can be ignored from the driver perspective.
>
> Regards
>
> Mario
>
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Version 2 changes:
>> Used NULL instead of 0 for unused hclk handle
>>
>> drivers/net/can/m_can/m_can.c | 9 +++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index f4947a7..ea48e59 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device
>> *pdev)
>> hclk = devm_clk_get(>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
Hi Wenyou,
On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>
>
> On 2017/9/14 13:06, Sekhar Nori wrote:
>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>
>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>> bit
>>>> was being transmitted and with a bit more investigation realized the
>>>> actual
>>>> MCAN IP would go back to initialization mode automatically.
>>>>
>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>> Compensation Offset register should be set. The document mentions
>>>> that this
>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>
>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>> indicates
>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>> rate
>>>> is above 2.5 Mbps.
>>>>
>>>> Signed-off-by: Franklin S Cooper Jr
>>>> ---
>>>> I'm pretty surprised that this hasn't been implemented already since
>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>> supports up to 10 Mbps.
>>>>
>>>> So it will be nice to get comments from users of this driver to
>>>> understand
>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>> patch.
>>>> If they haven't what did they do to get around it if they needed higher
>>>> speeds.
>>>>
>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>> insure
>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>> transceiver.
>>> ping. Anyone has any thoughts on this?
>> I added Dong who authored the m_can driver and Wenyou who added the only
>> in-kernel user of the driver for any help.
> I tested it on SAMA5D2 Xplained board both with and without this patch,
> both work with the 4M bps data bit rate.
Thank you for testing this out. Its interesting that you have been able
to use higher speeds without this patch. What is the CAN transceiver
being used on the SAMA5D2 Xplained board? I tried looking at the
schematic but it seems the CAN signals are used on an extension board
which I can't find the schematic for. Also do you mind sharing your test
setup? Were you doing a short point to point test?
Thank You,
Franklin
>
>>
>> Thanks,
>> Sekhar
>>
>>>> drivers/net/can/m_can/m_can.c | 24 +++-
>>>> 1 file changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/can/m_can/m_can.c
>>>> b/drivers/net/can/m_can/m_can.c
>>>> index f4947a7..720e073 100644
>>>> --- a/drivers/net/can/m_can/m_can.c
>>>> +++ b/drivers/net/can/m_can/m_can.c
>>>> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
>>>> #define DBTP_DSJW_SHIFT0
>>>> #define DBTP_DSJW_MASK(0xf << DBTP_DSJW_SHIFT)
>>>> +/* Transmitter Delay Compensation Register (TDCR) */
>>>> +#define TDCR_TDCO_SHIFT8
>>>> +#define TDCR_TDCO_MASK(0x7F << TDCR_TDCO_SHIFT)
>>>> +#define TDCR_TDCF_SHIFT0
>>>> +#define TDCR_TDCF_MASK(0x7F << TDCR_TDCO_SHIFT)
>>>> +
>>>> /* Test Register (TEST) */
>>>> #define TEST_LBCKBIT(4)
>>>> @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct
>>>> net_device *dev)
>>>> const struct can_bittiming *dbt = >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);
> }
>
>
Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
> resulted in errors. Scoping the signals I noticed that only a single bit
> was being transmitted and with a bit more investigation realized the actual
> MCAN IP would go back to initialization mode automatically.
>
> It appears this issue is due to the MCAN needing to use the Transmitter
> Delay Compensation Mode as defined in the MCAN User's Guide. When this
> mode is used the User's Guide indicates that the Transmitter Delay
> Compensation Offset register should be set. The document mentions that this
> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>
> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
> Therefore, only enable this mode and only set TDCO when the data bit rate
> is above 2.5 Mbps.
>
> Signed-off-by: Franklin S Cooper Jr
> ---
> I'm pretty surprised that this hasn't been implemented already since
> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
> supports up to 10 Mbps.
>
> So it will be nice to get comments from users of this driver to understand
> if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
> If they haven't what did they do to get around it if they needed higher
> speeds.
>
> Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
> everything still works at 5 Mbps which is the max speed of my CAN
> transceiver.
ping. Anyone has any thoughts on this?
>
> drivers/net/can/m_can/m_can.c | 24 +++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index f4947a7..720e073 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
> #define DBTP_DSJW_SHIFT 0
> #define DBTP_DSJW_MASK (0xf << DBTP_DSJW_SHIFT)
>
> +/* Transmitter Delay Compensation Register (TDCR) */
> +#define TDCR_TDCO_SHIFT 8
> +#define TDCR_TDCO_MASK (0x7F << TDCR_TDCO_SHIFT)
> +#define TDCR_TDCF_SHIFT 0
> +#define TDCR_TDCF_MASK (0x7F << TDCR_TDCO_SHIFT)
> +
> /* Test Register (TEST) */
> #define TEST_LBCKBIT(4)
>
> @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
> const struct can_bittiming *dbt = >can.data_bittiming;
> u16 brp, sjw, tseg1, tseg2;
> u32 reg_btp;
> + u32 enable_tdc = 0;
> + u32 tdco;
>
> brp = bt->brp - 1;
> sjw = bt->sjw - 1;
> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
> sjw = dbt->sjw - 1;
> tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
> tseg2 = dbt->phase_seg2 - 1;
> +
> + /* TDC is only needed for bitrates beyond 2.5 MBit/s
> + * Specified in the "Bit Time Requirements for CAN FD" document
> + */
> + if (dbt->bitrate > 250) {
> + enable_tdc = DBTP_TDC;
> + /* Equation based on Bosch's M_CAN User Manual's
> + * Transmitter Delay Compensation Section
> + */
> + tdco = priv->can.clock.freq / (dbt->bitrate * 2);
> + m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
> + }
> +
> reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
> (tseg1 << DBTP_DTSEG1_SHIFT) |
> - (tseg2 << DBTP_DTSEG2_SHIFT);
> + (tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
> +
> m_can_write(priv, M_CAN_DBTP, reg_btp);
> }
>
>
[PATCH v4 1/2] i2c: davinci: Add PM Runtime Support
66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
unlike other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
Version 4 changes:
Fix wording on error message
Fix additional error path
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path
drivers/i2c/busses/i2c-davinci.c | 67 +---
1 file changed, 55 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..a2d9f7c 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
/* - global defines --- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000/* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem*base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(>xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,13 +821,24 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(>dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(>dev, mem);
if (IS_ERR(dev->base)) {
- r = PTR_ERR(dev->base);
- goto err_unuse_clocks;
+ return PTR_ERR(dev->base);
+ }
+
+ pm_runtime_set_autosuspend_delay(dev->dev,
+DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ pm_runtime_enable(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
+ pm_runtime_put_noidle(dev->dev);
+ return r;
}
i2c_davinci_init(dev);
@@ -849,27 +879,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
return r;
}
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(>adapter);
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ ret = pm_runtime_get_sync(>dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(>dev);
+ return ret;
+ }
[PATCH v4 1/2] i2c: davinci: Add PM Runtime Support
66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
unlike other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.
Signed-off-by: Franklin S Cooper Jr
---
Version 4 changes:
Fix wording on error message
Fix additional error path
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path
drivers/i2c/busses/i2c-davinci.c | 67 +---
1 file changed, 55 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..a2d9f7c 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
/* - global defines --- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000/* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem*base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(>xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,13 +821,24 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(>dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(>dev, mem);
if (IS_ERR(dev->base)) {
- r = PTR_ERR(dev->base);
- goto err_unuse_clocks;
+ return PTR_ERR(dev->base);
+ }
+
+ pm_runtime_set_autosuspend_delay(dev->dev,
+DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ pm_runtime_enable(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
+ pm_runtime_put_noidle(dev->dev);
+ return r;
}
i2c_davinci_init(dev);
@@ -849,27 +879,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
return r;
}
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(>adapter);
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ ret = pm_runtime_get_sync(>dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(>dev);
+ return ret;
+ }
davinci_i2c_write_reg
[PATCH v4 2/2] dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com> Acked-by: Rob Herring <r...@kernel.org> Acked-by: Sekhar Nori <nsek...@ti.com> --- Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt index 5b123e0..64e6e65 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt @@ -6,6 +6,18 @@ davinci/keystone i2c interface contains. Required properties: - compatible: "ti,davinci-i2c" or "ti,keystone-i2c"; - reg : Offset and length of the register set for the device +- clocks: I2C functional clock phandle. + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the I2C device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Recommended properties : - interrupts : standard interrupt property. -- 2.9.4.dirty
[PATCH v4 2/2] dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr Acked-by: Rob Herring Acked-by: Sekhar Nori --- Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt index 5b123e0..64e6e65 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt @@ -6,6 +6,18 @@ davinci/keystone i2c interface contains. Required properties: - compatible: "ti,davinci-i2c" or "ti,keystone-i2c"; - reg : Offset and length of the register set for the device +- clocks: I2C functional clock phandle. + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the I2C device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Recommended properties : - interrupts : standard interrupt property. -- 2.9.4.dirty
[PATCH v4 0/2] i2c: davinci: Add PM Runtime Support needed by 66AK2G
Add I2C support to 66AK2G. Primary requirement is to add PM Runtime support to the driver. This has been tested on following platforms by performing simple i2c test such as i2c detect and reading on board i2c devices: K2G GP evm OMAPL138 K2L GP EVM and boot tested on: K2E GP EVM K2HK GP EVM Version 2 changes: Moved ordering of pm runtime calls Version 3 changes: Dropped first patch since it was pulled in. Remove clk = NULL statements Fix error path Version 4 changes: Fix typo in commit message Fix error path Franklin S Cooper Jr (2): i2c: davinci: Add PM Runtime Support dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property .../devicetree/bindings/i2c/i2c-davinci.txt| 12 drivers/i2c/busses/i2c-davinci.c | 67 ++ 2 files changed, 67 insertions(+), 12 deletions(-) -- 2.9.4.dirty
[PATCH v4 0/2] i2c: davinci: Add PM Runtime Support needed by 66AK2G
Add I2C support to 66AK2G. Primary requirement is to add PM Runtime support to the driver. This has been tested on following platforms by performing simple i2c test such as i2c detect and reading on board i2c devices: K2G GP evm OMAPL138 K2L GP EVM and boot tested on: K2E GP EVM K2HK GP EVM Version 2 changes: Moved ordering of pm runtime calls Version 3 changes: Dropped first patch since it was pulled in. Remove clk = NULL statements Fix error path Version 4 changes: Fix typo in commit message Fix error path Franklin S Cooper Jr (2): i2c: davinci: Add PM Runtime Support dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property .../devicetree/bindings/i2c/i2c-davinci.txt| 12 drivers/i2c/busses/i2c-davinci.c | 67 ++ 2 files changed, 67 insertions(+), 12 deletions(-) -- 2.9.4.dirty
Re: [PATCH v3 1/2] i2c: davinci: Add PM Runtime Support
On 09/11/2017 06:29 AM, Sekhar Nori wrote:
> On Friday 08 September 2017 11:24 PM, Franklin S Cooper Jr wrote:
>> 66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
>> like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
>
> unlike ?
>
>> is required to insure the power domain used by the specific I2C instance is
>> properly turned on along with its functional clock.
>>
>> Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
>> ---
>> Version 3 changes:
>> Remove several statements that set clk to NULL
>> Fix error path
>>
>> drivers/i2c/busses/i2c-davinci.c | 64
>> +---
>> 1 file changed, 53 insertions(+), 11 deletions(-)
>
>> @@ -802,12 +821,22 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> dev->clk = devm_clk_get(>dev, NULL);
>> if (IS_ERR(dev->clk))
>> return PTR_ERR(dev->clk);
>> -clk_prepare_enable(dev->clk);
>>
>> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> dev->base = devm_ioremap_resource(>dev, mem);
>> if (IS_ERR(dev->base)) {
>> -r = PTR_ERR(dev->base);
>> +return PTR_ERR(dev->base);
>> +}
>> +
>> +pm_runtime_set_autosuspend_delay(dev->dev,
>> + DAVINCI_I2C_PM_TIMEOUT);
>> +pm_runtime_use_autosuspend(dev->dev);
>> +
>> +pm_runtime_enable(dev->dev);
>> +
>> +r = pm_runtime_get_sync(dev->dev);
>> +if (r < 0) {
>> +dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
>> goto err_unuse_clocks;
>
> You end up doing a pm_runtime_put_sync() on failure here, instead of
> pm_runtime_put_noidle() like rest of the patch.
>
> May be handle this failure here instead of relying on the goto path.
Ok
>
>> }
>>
>> @@ -849,27 +878,40 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> if (r)
>> goto err_unuse_clocks;
>>
>> +pm_runtime_mark_last_busy(dev->dev);
>> +pm_runtime_put_autosuspend(dev->dev);
>> +
>> return 0;
>>
>> err_unuse_clocks:
>> -clk_disable_unprepare(dev->clk);
>> -dev->clk = NULL;
>> +pm_runtime_dont_use_autosuspend(dev->dev);
>> +pm_runtime_put_sync(dev->dev);
>> +pm_runtime_disable(dev->dev);
>> +
>> return r;
>> }
>
> Thanks,
> Sekhar
>
Re: [PATCH v3 1/2] i2c: davinci: Add PM Runtime Support
On 09/11/2017 06:29 AM, Sekhar Nori wrote:
> On Friday 08 September 2017 11:24 PM, Franklin S Cooper Jr wrote:
>> 66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
>> like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
>
> unlike ?
>
>> is required to insure the power domain used by the specific I2C instance is
>> properly turned on along with its functional clock.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Version 3 changes:
>> Remove several statements that set clk to NULL
>> Fix error path
>>
>> drivers/i2c/busses/i2c-davinci.c | 64
>> +---
>> 1 file changed, 53 insertions(+), 11 deletions(-)
>
>> @@ -802,12 +821,22 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> dev->clk = devm_clk_get(>dev, NULL);
>> if (IS_ERR(dev->clk))
>> return PTR_ERR(dev->clk);
>> -clk_prepare_enable(dev->clk);
>>
>> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> dev->base = devm_ioremap_resource(>dev, mem);
>> if (IS_ERR(dev->base)) {
>> -r = PTR_ERR(dev->base);
>> +return PTR_ERR(dev->base);
>> +}
>> +
>> +pm_runtime_set_autosuspend_delay(dev->dev,
>> + DAVINCI_I2C_PM_TIMEOUT);
>> +pm_runtime_use_autosuspend(dev->dev);
>> +
>> +pm_runtime_enable(dev->dev);
>> +
>> +r = pm_runtime_get_sync(dev->dev);
>> +if (r < 0) {
>> +dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
>> goto err_unuse_clocks;
>
> You end up doing a pm_runtime_put_sync() on failure here, instead of
> pm_runtime_put_noidle() like rest of the patch.
>
> May be handle this failure here instead of relying on the goto path.
Ok
>
>> }
>>
>> @@ -849,27 +878,40 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> if (r)
>> goto err_unuse_clocks;
>>
>> +pm_runtime_mark_last_busy(dev->dev);
>> +pm_runtime_put_autosuspend(dev->dev);
>> +
>> return 0;
>>
>> err_unuse_clocks:
>> -clk_disable_unprepare(dev->clk);
>> -dev->clk = NULL;
>> +pm_runtime_dont_use_autosuspend(dev->dev);
>> +pm_runtime_put_sync(dev->dev);
>> +pm_runtime_disable(dev->dev);
>> +
>> return r;
>> }
>
> Thanks,
> Sekhar
>
[PATCH 3/3] ARM: dts: keystone-k2g-evm: add bindings for SPI NOR flash
From: Murali Karicheri <m-kariche...@ti.com>
K2G EVM has n25q128a13 SPI NOR flash on SPI1. Enable SPI1 in the DT
node as well as add a subnode for the SPI NOR.
Signed-off-by: Murali Karicheri <m-kariche...@ti.com>
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
arch/arm/boot/dts/keystone-k2g-evm.dts | 37 ++
1 file changed, 37 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g-evm.dts
b/arch/arm/boot/dts/keystone-k2g-evm.dts
index f462f10..d5c43c7 100644
--- a/arch/arm/boot/dts/keystone-k2g-evm.dts
+++ b/arch/arm/boot/dts/keystone-k2g-evm.dts
@@ -81,6 +81,16 @@
K2G_CORE_IOPAD(0x1110) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* mmc1_cmd.mmc1_cmd */
>;
};
+
+ spi1_pins: pinmux_spi1_pins {
+ pinctrl-single,pins = <
+ K2G_CORE_IOPAD(0x11a4) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_scs0.spi1_scs0 */
+ K2G_CORE_IOPAD(0x11ac) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_clk.spi1_clk */
+ K2G_CORE_IOPAD(0x11b0) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_miso.spi1_miso */
+ K2G_CORE_IOPAD(0x11b4) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_mosi.spi1_mosi */
+ >;
+ };
+
};
{
@@ -112,3 +122,30 @@
memory-region = <_common_memory>;
status = "okay";
};
+
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+ status = "okay";
+
+ spi_nor: flash@0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "jedec,spi-nor";
+ spi-max-frequency = <500>;
+ m25p,fast-read;
+ reg = <0>;
+
+ partition@0 {
+ label = "u-boot-spl";
+ reg = <0x0 0x10>;
+ read-only;
+ };
+
+ partition@1 {
+ label = "misc";
+ reg = <0x10 0xf0>;
+ };
+ };
+};
+
--
2.9.4.dirty
[PATCH 1/3] dt-bindings: spi: spi-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com> --- Documentation/devicetree/bindings/spi/spi-davinci.txt | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt index f5916c9..1925277 100644 --- a/Documentation/devicetree/bindings/spi/spi-davinci.txt +++ b/Documentation/devicetree/bindings/spi/spi-davinci.txt @@ -24,6 +24,16 @@ Required properties: based on a specific SoC configuration. - interrupts: interrupt number mapped to CPU. - clocks: spi clk phandle + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the SPI device id + value. This property is as per the binding, Optional: - cs-gpios: gpio chip selects -- 2.9.4.dirty
[PATCH 3/3] ARM: dts: keystone-k2g-evm: add bindings for SPI NOR flash
From: Murali Karicheri
K2G EVM has n25q128a13 SPI NOR flash on SPI1. Enable SPI1 in the DT
node as well as add a subnode for the SPI NOR.
Signed-off-by: Murali Karicheri
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g-evm.dts | 37 ++
1 file changed, 37 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g-evm.dts
b/arch/arm/boot/dts/keystone-k2g-evm.dts
index f462f10..d5c43c7 100644
--- a/arch/arm/boot/dts/keystone-k2g-evm.dts
+++ b/arch/arm/boot/dts/keystone-k2g-evm.dts
@@ -81,6 +81,16 @@
K2G_CORE_IOPAD(0x1110) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* mmc1_cmd.mmc1_cmd */
>;
};
+
+ spi1_pins: pinmux_spi1_pins {
+ pinctrl-single,pins = <
+ K2G_CORE_IOPAD(0x11a4) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_scs0.spi1_scs0 */
+ K2G_CORE_IOPAD(0x11ac) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_clk.spi1_clk */
+ K2G_CORE_IOPAD(0x11b0) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_miso.spi1_miso */
+ K2G_CORE_IOPAD(0x11b4) (BUFFER_CLASS_B | PULL_DISABLE |
MUX_MODE0) /* spi1_mosi.spi1_mosi */
+ >;
+ };
+
};
{
@@ -112,3 +122,30 @@
memory-region = <_common_memory>;
status = "okay";
};
+
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+ status = "okay";
+
+ spi_nor: flash@0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "jedec,spi-nor";
+ spi-max-frequency = <500>;
+ m25p,fast-read;
+ reg = <0>;
+
+ partition@0 {
+ label = "u-boot-spl";
+ reg = <0x0 0x10>;
+ read-only;
+ };
+
+ partition@1 {
+ label = "misc";
+ reg = <0x10 0xf0>;
+ };
+ };
+};
+
--
2.9.4.dirty
[PATCH 1/3] dt-bindings: spi: spi-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr --- Documentation/devicetree/bindings/spi/spi-davinci.txt | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt index f5916c9..1925277 100644 --- a/Documentation/devicetree/bindings/spi/spi-davinci.txt +++ b/Documentation/devicetree/bindings/spi/spi-davinci.txt @@ -24,6 +24,16 @@ Required properties: based on a specific SoC configuration. - interrupts: interrupt number mapped to CPU. - clocks: spi clk phandle + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the SPI device id + value. This property is as per the binding, Optional: - cs-gpios: gpio chip selects -- 2.9.4.dirty
[PATCH 0/3] ARM: dts: keystone-k2g-evm: Add SPI nodes and enable SPI1 on K2G EVM
Update the binding document to indicate the required requirements for 66AK2G. In addition add the proper SPI nodes for 66AK2G. For K2G EVM enable SPI1 which is used for the on board SPI NOR. Franklin S Cooper Jr (1): dt-bindings: spi: spi-davinci: Update binding for 66AK2Gx pwr dm property Murali Karicheri (1): ARM: dts: keystone-k2g-evm: add bindings for SPI NOR flash Vitaly Andrianov (1): ARM: dts: keystone-k2g: Add SPI nodes .../devicetree/bindings/spi/spi-davinci.txt| 10 + arch/arm/boot/dts/keystone-k2g-evm.dts | 37 + arch/arm/boot/dts/keystone-k2g.dtsi| 48 ++ 3 files changed, 95 insertions(+) -- 2.9.4.dirty
[PATCH 0/3] ARM: dts: keystone-k2g-evm: Add SPI nodes and enable SPI1 on K2G EVM
Update the binding document to indicate the required requirements for 66AK2G. In addition add the proper SPI nodes for 66AK2G. For K2G EVM enable SPI1 which is used for the on board SPI NOR. Franklin S Cooper Jr (1): dt-bindings: spi: spi-davinci: Update binding for 66AK2Gx pwr dm property Murali Karicheri (1): ARM: dts: keystone-k2g-evm: add bindings for SPI NOR flash Vitaly Andrianov (1): ARM: dts: keystone-k2g: Add SPI nodes .../devicetree/bindings/spi/spi-davinci.txt| 10 + arch/arm/boot/dts/keystone-k2g-evm.dts | 37 + arch/arm/boot/dts/keystone-k2g.dtsi| 48 ++ 3 files changed, 95 insertions(+) -- 2.9.4.dirty
[PATCH 2/3] ARM: dts: keystone-k2g: Add SPI nodes
From: Vitaly Andrianov <vita...@ti.com>
Add nodes for the various SPI instances.
Signed-off-by: Vitaly Andrianov <vita...@ti.com>
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
arch/arm/boot/dts/keystone-k2g.dtsi | 48 +
1 file changed, 48 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index 826b286..9f9e9e1 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -343,5 +343,53 @@
clock-names = "fck", "mmchsdb_fck";
status = "disabled";
};
+
+ spi0: spi@21805400 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21805400 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <_pds 0x0010>;
+ clocks = <_clks 0x0010 0>;
+ };
+
+ spi1: spi@21805800 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21805800 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <_pds 0x0011>;
+ clocks = <_clks 0x0011 0>;
+ };
+
+ spi2: spi@21805c00 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21805C00 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <_pds 0x0012>;
+ clocks = <_clks 0x0012 0>;
+ };
+
+ spi3: spi@21806000 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21806000 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <_pds 0x0013>;
+ clocks = <_clks 0x0013 0>;
+ };
};
};
--
2.9.4.dirty
[PATCH 2/3] ARM: dts: keystone-k2g: Add SPI nodes
From: Vitaly Andrianov
Add nodes for the various SPI instances.
Signed-off-by: Vitaly Andrianov
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g.dtsi | 48 +
1 file changed, 48 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index 826b286..9f9e9e1 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -343,5 +343,53 @@
clock-names = "fck", "mmchsdb_fck";
status = "disabled";
};
+
+ spi0: spi@21805400 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21805400 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <_pds 0x0010>;
+ clocks = <_clks 0x0010 0>;
+ };
+
+ spi1: spi@21805800 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21805800 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <_pds 0x0011>;
+ clocks = <_clks 0x0011 0>;
+ };
+
+ spi2: spi@21805c00 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21805C00 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <_pds 0x0012>;
+ clocks = <_clks 0x0012 0>;
+ };
+
+ spi3: spi@21806000 {
+ compatible = "ti,keystone-spi";
+ reg = <0x21806000 0x200>;
+ num-cs = <4>;
+ ti,davinci-spi-intr-line = <0>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-domains = <_pds 0x0013>;
+ clocks = <_clks 0x0013 0>;
+ };
};
};
--
2.9.4.dirty
[PATCH v2 2/2] ARM: dts: k2g-evm: Enable USB 0 and 1
From: Roger Quadros <rog...@ti.com>
Enable USB 0 which will be used as a host port and USB 1 which will be
used in peripheral mode.
Signed-off-by: Roger Quadros <rog...@ti.com>
Signed-off-by: Murali Karicheri <m-kariche...@ti.com>
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
arch/arm/boot/dts/keystone-k2g-evm.dts | 26 ++
1 file changed, 26 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g-evm.dts
b/arch/arm/boot/dts/keystone-k2g-evm.dts
index 61883cb..4f56295 100644
--- a/arch/arm/boot/dts/keystone-k2g-evm.dts
+++ b/arch/arm/boot/dts/keystone-k2g-evm.dts
@@ -41,3 +41,29 @@
pinctrl-0 = <_pins>;
status = "okay";
};
+
+_usb0 {
+ status = "okay";
+};
+
+_phy {
+ status = "okay";
+};
+
+ {
+ dr_mode = "host";
+ status = "okay";
+};
+
+_usb1 {
+ status = "okay";
+};
+
+_phy {
+ status = "okay";
+};
+
+ {
+ dr_mode = "peripheral";
+ status = "okay";
+};
--
2.9.4.dirty
[PATCH v2 2/2] ARM: dts: k2g-evm: Enable USB 0 and 1
From: Roger Quadros
Enable USB 0 which will be used as a host port and USB 1 which will be
used in peripheral mode.
Signed-off-by: Roger Quadros
Signed-off-by: Murali Karicheri
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g-evm.dts | 26 ++
1 file changed, 26 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g-evm.dts
b/arch/arm/boot/dts/keystone-k2g-evm.dts
index 61883cb..4f56295 100644
--- a/arch/arm/boot/dts/keystone-k2g-evm.dts
+++ b/arch/arm/boot/dts/keystone-k2g-evm.dts
@@ -41,3 +41,29 @@
pinctrl-0 = <_pins>;
status = "okay";
};
+
+_usb0 {
+ status = "okay";
+};
+
+_phy {
+ status = "okay";
+};
+
+ {
+ dr_mode = "host";
+ status = "okay";
+};
+
+_usb1 {
+ status = "okay";
+};
+
+_phy {
+ status = "okay";
+};
+
+ {
+ dr_mode = "peripheral";
+ status = "okay";
+};
--
2.9.4.dirty
[PATCH v2 1/2] ARM: dts: k2g: Add USB instances
From: Vitaly Andrianov <vita...@ti.com>
Add nodes for both USB instances supported by 66AK2G.
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
arch/arm/boot/dts/keystone-k2g.dtsi | 56 +
1 file changed, 56 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index a789f75..c8f3b93 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -139,5 +139,61 @@
interrupts = ,
;
};
+
+ usb0_phy: usb-phy@0 {
+ compatible = "usb-nop-xceiv";
+ status = "disabled";
+ };
+
+ keystone_usb0: keystone-dwc3@268 {
+ compatible = "ti,keystone-dwc3";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x268 0x1>;
+ interrupts = ;
+ ranges;
+ dma-coherent;
+ dma-ranges;
+ status = "disabled";
+ power-domains = <_pds 0x0016>;
+
+ usb0: usb@269 {
+ compatible = "snps,dwc3";
+ reg = <0x269 0x1>;
+ interrupts = ;
+ maximum-speed = "high-speed";
+ dr_mode = "otg";
+ usb-phy = <_phy>;
+ status = "disabled";
+ };
+ };
+
+ usb1_phy: usb-phy@1 {
+ compatible = "usb-nop-xceiv";
+ status = "disabled";
+ };
+
+ keystone_usb1: keystone-dwc3@258 {
+ compatible = "ti,keystone-dwc3";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x258 0x1>;
+ interrupts = ;
+ ranges;
+ dma-coherent;
+ dma-ranges;
+ status = "disabled";
+ power-domains = <_pds 0x0017>;
+
+ usb1: usb@259 {
+ compatible = "snps,dwc3";
+ reg = <0x259 0x1>;
+ interrupts = ;
+ maximum-speed = "high-speed";
+ dr_mode = "otg";
+ usb-phy = <_phy>;
+ status = "disabled";
+ };
+ };
};
};
--
2.9.4.dirty
[PATCH v2 1/2] ARM: dts: k2g: Add USB instances
From: Vitaly Andrianov
Add nodes for both USB instances supported by 66AK2G.
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g.dtsi | 56 +
1 file changed, 56 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index a789f75..c8f3b93 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -139,5 +139,61 @@
interrupts = ,
;
};
+
+ usb0_phy: usb-phy@0 {
+ compatible = "usb-nop-xceiv";
+ status = "disabled";
+ };
+
+ keystone_usb0: keystone-dwc3@268 {
+ compatible = "ti,keystone-dwc3";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x268 0x1>;
+ interrupts = ;
+ ranges;
+ dma-coherent;
+ dma-ranges;
+ status = "disabled";
+ power-domains = <_pds 0x0016>;
+
+ usb0: usb@269 {
+ compatible = "snps,dwc3";
+ reg = <0x269 0x1>;
+ interrupts = ;
+ maximum-speed = "high-speed";
+ dr_mode = "otg";
+ usb-phy = <_phy>;
+ status = "disabled";
+ };
+ };
+
+ usb1_phy: usb-phy@1 {
+ compatible = "usb-nop-xceiv";
+ status = "disabled";
+ };
+
+ keystone_usb1: keystone-dwc3@258 {
+ compatible = "ti,keystone-dwc3";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x258 0x1>;
+ interrupts = ;
+ ranges;
+ dma-coherent;
+ dma-ranges;
+ status = "disabled";
+ power-domains = <_pds 0x0017>;
+
+ usb1: usb@259 {
+ compatible = "snps,dwc3";
+ reg = <0x259 0x1>;
+ interrupts = ;
+ maximum-speed = "high-speed";
+ dr_mode = "otg";
+ usb-phy = <_phy>;
+ status = "disabled";
+ };
+ };
};
};
--
2.9.4.dirty
[PATCH v2 0/2] ARM: dts: keystone-k2g-evm: Add USB nodes and enable USB on K2G EVM
Add USB nodes to 66AK2G dtsi. Also enable the two USB nodes that are on the K2G EVM. Roger Quadros (1): ARM: dts: k2g-evm: Enable USB 0 and 1 Vitaly Andrianov (1): ARM: dts: k2g: Add USB instances arch/arm/boot/dts/keystone-k2g-evm.dts | 26 arch/arm/boot/dts/keystone-k2g.dtsi| 56 ++ 2 files changed, 82 insertions(+) -- 2.9.4.dirty
[PATCH v2 0/2] ARM: dts: keystone-k2g-evm: Add USB nodes and enable USB on K2G EVM
Add USB nodes to 66AK2G dtsi. Also enable the two USB nodes that are on the K2G EVM. Roger Quadros (1): ARM: dts: k2g-evm: Enable USB 0 and 1 Vitaly Andrianov (1): ARM: dts: k2g: Add USB instances arch/arm/boot/dts/keystone-k2g-evm.dts | 26 arch/arm/boot/dts/keystone-k2g.dtsi| 56 ++ 2 files changed, 82 insertions(+) -- 2.9.4.dirty
[PATCH v2 2/2] ARM: dts: keystone-k2g-evm: Add I2C EEPROM DT entry
From: Murali Karicheri <m-kariche...@ti.com>
K2G EVM has an onboard I2C EEPROM connected to I2C0. This patch adds
the necessary DT entry for the AT24CM01 EEPROM.
Signed-off-by: Murali Karicheri <m-kariche...@ti.com>
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
arch/arm/boot/dts/keystone-k2g-evm.dts | 19 +++
1 file changed, 19 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g-evm.dts
b/arch/arm/boot/dts/keystone-k2g-evm.dts
index f462f10..a6ad5fc 100644
--- a/arch/arm/boot/dts/keystone-k2g-evm.dts
+++ b/arch/arm/boot/dts/keystone-k2g-evm.dts
@@ -81,6 +81,14 @@
K2G_CORE_IOPAD(0x1110) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* mmc1_cmd.mmc1_cmd */
>;
};
+
+ i2c0_pins: pinmux_i2c0_pins {
+ pinctrl-single,pins = <
+ K2G_CORE_IOPAD(0x137c) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* i2c0_scl.i2c0_scl */
+ K2G_CORE_IOPAD(0x1380) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* i2c0_sda.i2c0_sda */
+ >;
+ };
+
};
{
@@ -112,3 +120,14 @@
memory-region = <_common_memory>;
status = "okay";
};
+
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+ status = "okay";
+
+ eeprom@50 {
+ compatible = "atmel,24c1024";
+ reg = <0x50>;
+ };
+};
--
2.9.4.dirty
[PATCH v2 2/2] ARM: dts: keystone-k2g-evm: Add I2C EEPROM DT entry
From: Murali Karicheri
K2G EVM has an onboard I2C EEPROM connected to I2C0. This patch adds
the necessary DT entry for the AT24CM01 EEPROM.
Signed-off-by: Murali Karicheri
Signed-off-by: Franklin S Cooper Jr
---
arch/arm/boot/dts/keystone-k2g-evm.dts | 19 +++
1 file changed, 19 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g-evm.dts
b/arch/arm/boot/dts/keystone-k2g-evm.dts
index f462f10..a6ad5fc 100644
--- a/arch/arm/boot/dts/keystone-k2g-evm.dts
+++ b/arch/arm/boot/dts/keystone-k2g-evm.dts
@@ -81,6 +81,14 @@
K2G_CORE_IOPAD(0x1110) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* mmc1_cmd.mmc1_cmd */
>;
};
+
+ i2c0_pins: pinmux_i2c0_pins {
+ pinctrl-single,pins = <
+ K2G_CORE_IOPAD(0x137c) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* i2c0_scl.i2c0_scl */
+ K2G_CORE_IOPAD(0x1380) (BUFFER_CLASS_B | PIN_PULLUP |
MUX_MODE0)/* i2c0_sda.i2c0_sda */
+ >;
+ };
+
};
{
@@ -112,3 +120,14 @@
memory-region = <_common_memory>;
status = "okay";
};
+
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+ status = "okay";
+
+ eeprom@50 {
+ compatible = "atmel,24c1024";
+ reg = <0x50>;
+ };
+};
--
2.9.4.dirty
[PATCH v2 0/2] ARM: dts: keystone-k2g-evm: Add I2C nodes and enable I2C0 on K2G EVM
Add I2C DT nodes for 66AK2G. Also enable I2C0 in K2G EVM which is needed to access the I2C EEPROM. Version 2 changes: No real changes. Split patches into their own patchset Murali Karicheri (1): ARM: dts: keystone-k2g-evm: Add I2C EEPROM DT entry Vitaly Andrianov (1): ARM: dts: keystone-k2g: Add I2C nodes arch/arm/boot/dts/keystone-k2g-evm.dts | 19 ++ arch/arm/boot/dts/keystone-k2g.dtsi| 36 ++ 2 files changed, 55 insertions(+) -- 2.9.4.dirty
[PATCH v2 1/2] ARM: dts: keystone-k2g: Add I2C nodes
From: Vitaly Andrianov <vita...@ti.com>
Add nodes for the various I2C instances.
Signed-off-by: Vitaly Andrianov <vita...@ti.com>
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
Reviewed-by: Grygorii Strashko <grygorii.stras...@ti.com>
---
arch/arm/boot/dts/keystone-k2g.dtsi | 36
1 file changed, 36 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index 826b286..8a98d76 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -28,6 +28,9 @@
aliases {
serial0 =
+ i2c0 =
+ i2c1 =
+ i2c2 =
rproc0 =
};
@@ -133,6 +136,39 @@
clocks = <_clks 0x0009 1>;
};
+ i2c0: i2c@253 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x0253 0x400>;
+ clocks = <_clks 0x003a 0>;
+ power-domains = <_pds 0x003a>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c1: i2c@2530400 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x02530400 0x400>;
+ clocks = <_clks 0x003b 0>;
+ power-domains = <_pds 0x003b>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c2: i2c@2530800 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x02530800 0x400>;
+ clocks = <_clks 0x003c 0>;
+ power-domains = <_pds 0x003c>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
kirq0: keystone_irq@026202a0 {
compatible = "ti,keystone-irq";
interrupts = ;
--
2.9.4.dirty
[PATCH v2 0/2] ARM: dts: keystone-k2g-evm: Add I2C nodes and enable I2C0 on K2G EVM
Add I2C DT nodes for 66AK2G. Also enable I2C0 in K2G EVM which is needed to access the I2C EEPROM. Version 2 changes: No real changes. Split patches into their own patchset Murali Karicheri (1): ARM: dts: keystone-k2g-evm: Add I2C EEPROM DT entry Vitaly Andrianov (1): ARM: dts: keystone-k2g: Add I2C nodes arch/arm/boot/dts/keystone-k2g-evm.dts | 19 ++ arch/arm/boot/dts/keystone-k2g.dtsi| 36 ++ 2 files changed, 55 insertions(+) -- 2.9.4.dirty
[PATCH v2 1/2] ARM: dts: keystone-k2g: Add I2C nodes
From: Vitaly Andrianov
Add nodes for the various I2C instances.
Signed-off-by: Vitaly Andrianov
Signed-off-by: Franklin S Cooper Jr
Reviewed-by: Grygorii Strashko
---
arch/arm/boot/dts/keystone-k2g.dtsi | 36
1 file changed, 36 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi
b/arch/arm/boot/dts/keystone-k2g.dtsi
index 826b286..8a98d76 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -28,6 +28,9 @@
aliases {
serial0 =
+ i2c0 =
+ i2c1 =
+ i2c2 =
rproc0 =
};
@@ -133,6 +136,39 @@
clocks = <_clks 0x0009 1>;
};
+ i2c0: i2c@253 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x0253 0x400>;
+ clocks = <_clks 0x003a 0>;
+ power-domains = <_pds 0x003a>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c1: i2c@2530400 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x02530400 0x400>;
+ clocks = <_clks 0x003b 0>;
+ power-domains = <_pds 0x003b>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c2: i2c@2530800 {
+ compatible = "ti,keystone-i2c";
+ reg = <0x02530800 0x400>;
+ clocks = <_clks 0x003c 0>;
+ power-domains = <_pds 0x003c>;
+ interrupts = ;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
kirq0: keystone_irq@026202a0 {
compatible = "ti,keystone-irq";
interrupts = ;
--
2.9.4.dirty
[PATCH v3 0/2] ARM: dts: keystone-k2g: Add I2C support for 66AK2G
Add I2C support to 66AK2G. Primary requirement is to add PM Runtime support to the driver. This has been tested on following platforms by performing simple i2c test such as i2c detect and reading on board i2c devices: K2G GP evm OMAPL138 K2L GP EVM and boot tested on: K2E GP EVM K2HK GP EVM Version 2 changes: Moved ordering of pm runtime calls Version 3 changes: Dropped first patch since it was pulled in. Remove clk = NULL statements Fix error path Franklin S Cooper Jr (2): i2c: davinci: Add PM Runtime Support dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property .../devicetree/bindings/i2c/i2c-davinci.txt| 12 drivers/i2c/busses/i2c-davinci.c | 64 ++ 2 files changed, 65 insertions(+), 11 deletions(-) -- 2.9.4.dirty
[PATCH v3 0/2] ARM: dts: keystone-k2g: Add I2C support for 66AK2G
Add I2C support to 66AK2G. Primary requirement is to add PM Runtime support to the driver. This has been tested on following platforms by performing simple i2c test such as i2c detect and reading on board i2c devices: K2G GP evm OMAPL138 K2L GP EVM and boot tested on: K2E GP EVM K2HK GP EVM Version 2 changes: Moved ordering of pm runtime calls Version 3 changes: Dropped first patch since it was pulled in. Remove clk = NULL statements Fix error path Franklin S Cooper Jr (2): i2c: davinci: Add PM Runtime Support dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property .../devicetree/bindings/i2c/i2c-davinci.txt| 12 drivers/i2c/busses/i2c-davinci.c | 64 ++ 2 files changed, 65 insertions(+), 11 deletions(-) -- 2.9.4.dirty
[PATCH v3 1/2] i2c: davinci: Add PM Runtime Support
66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path
drivers/i2c/busses/i2c-davinci.c | 64 +---
1 file changed, 53 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..57612cb 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
/* - global defines --- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000/* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem*base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(>xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,12 +821,22 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(>dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(>dev, mem);
if (IS_ERR(dev->base)) {
- r = PTR_ERR(dev->base);
+ return PTR_ERR(dev->base);
+ }
+
+ pm_runtime_set_autosuspend_delay(dev->dev,
+DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ pm_runtime_enable(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
goto err_unuse_clocks;
}
@@ -849,27 +878,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
return r;
}
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(>adapter);
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ ret = pm_runtime_get_sync(>dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(>dev);
+ return ret;
+ }
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0);
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+
[PATCH v3 1/2] i2c: davinci: Add PM Runtime Support
66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.
Signed-off-by: Franklin S Cooper Jr
---
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path
drivers/i2c/busses/i2c-davinci.c | 64 +---
1 file changed, 53 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..57612cb 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
/* - global defines --- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000/* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem*base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(>xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,12 +821,22 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(>dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(>dev, mem);
if (IS_ERR(dev->base)) {
- r = PTR_ERR(dev->base);
+ return PTR_ERR(dev->base);
+ }
+
+ pm_runtime_set_autosuspend_delay(dev->dev,
+DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ pm_runtime_enable(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
goto err_unuse_clocks;
}
@@ -849,27 +878,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
return r;
}
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(>adapter);
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ ret = pm_runtime_get_sync(>dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(>dev);
+ return ret;
+ }
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0);
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
return
[PATCH v3 2/2] dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com> Acked-by: Rob Herring <r...@kernel.org> Acked-by: Sekhar Nori <nsek...@ti.com> --- Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt index 5b123e0..64e6e65 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt @@ -6,6 +6,18 @@ davinci/keystone i2c interface contains. Required properties: - compatible: "ti,davinci-i2c" or "ti,keystone-i2c"; - reg : Offset and length of the register set for the device +- clocks: I2C functional clock phandle. + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the I2C device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Recommended properties : - interrupts : standard interrupt property. -- 2.9.4.dirty
[PATCH v3 2/2] dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr Acked-by: Rob Herring Acked-by: Sekhar Nori --- Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt index 5b123e0..64e6e65 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt @@ -6,6 +6,18 @@ davinci/keystone i2c interface contains. Required properties: - compatible: "ti,davinci-i2c" or "ti,keystone-i2c"; - reg : Offset and length of the register set for the device +- clocks: I2C functional clock phandle. + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the I2C device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Recommended properties : - interrupts : standard interrupt property. -- 2.9.4.dirty
Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support
On 08/23/2017 01:34 AM, Sekhar Nori wrote:
> On Monday 21 August 2017 10:29 PM, Franklin S Cooper Jr wrote:
>>
>>
>> On 08/21/2017 07:20 AM, Sekhar Nori wrote:
>>> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>>>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>>>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>>>> power domains needed by UART are on. Keep legacy clk api calls since other
>>>> users of this driver may not support PM runtime.
>>
>> There are a significant amount of users across a wide variety of
>> architectures and boards that I have no means to properly test to insure
>> I'm avoiding regressions. My current approach which I've seen other
>> drivers in the past use when in similar situations allows things to work
>> without regressions.
>
> Since the users are all device-tree users, my hope is they can be
> tracked by looking at the DT files and the maintainers be copied on this
> patch to make sure nothing breaks for them. I understand its not a
> trivial list, but its finite :)
>
>>>
>>> Do we really have users like that? And even if there are, cant they use
>>> PM clock handling support available in drivers/base/power/clock_ops.c ?
>>
>> I don't see any current defconfig that enables CONFIG_PM_CLK and only a
>
> Thats because its a non-user-selectable def_bool. Most configs should
> have it enabled after the 'make config' step.
>
>> handful of instances where functions from clock_ops.c are actually used.
>> I don't quite understand what your suggestion is but in general I'm
>> concerned since any approach to move everyone to different apis is
>> rather risky especially for a critical driver.
>>
>> If I'm missing your point please let me know.
>
> I do agree we dont want to break anyone. But at the same time, this
> patch redundantly enables/disables clock for most known (and possible
> future) users without no clear indication of who benefits from such
> redundancy.
>
> If we ignore this now, the problem will only get worse as time passes. I
> suggest biting the bullet now, attempt to reach-out as many affected
> parties as possible but switch to pm_runtime once and for all.
>
>>
>>>
>>> The clock enable support itself was added pretty "recently" - about 5
>>> years back with 0bbeb3c3e84b ("of serial port driver - add
>>> clk_get_rate() support"). So I doubt any really legacy platforms relied
>>> on clock support being there. It was added by Murali, I assume for
>>> Keystone devices. Keystone devices can work with runtime PM using the PM
>>> clock support pointed to above.
>>
>> You might be right but I can't be confident that it is indeed the case.
>> But its possible that at some point people will start having problems if
>> they try to use this driver for other UART instances. The currently
>> could not be aware of an issue because of the bootloader powering things
>> for them or even that different UART instances could be apart of a non
>> always on power domain.
>
> I dont think you got my point there. I have no disagreement that
> pm_runtime support is needed. In fact, it should have been that way when
> clock enable/disable was added. But thats history now.
>
>>> Perhaps linux-arm-kernel list should be copied on this submission too,
>>> since most users of this driver are likely to be there on that list.
>>>
>>
>> Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
>> bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
>> PowerPC, MIPS, Xtensa and Arc.>
>> Looking at dts that enable some of the compatible it is still a
>> combination of quite a bit of architectures.
>
> Right, I do think there are quite a few users. But they should still be
> reachable and provide testing inputs for the change.
>
>> The current approach I've taken should be safe for all users of this
>> driver which. I have no issue going another approach as long as its
>> understood that I'm fairly limited in what I can test when you take into
>> account the large number of users of this driver.
>
> Understood that you are limited by hardware you possess. But we can
> probably reach out to as many folks affected by this as possible so we
> get a fair chance at doing the "right thing" without breaking anyone.
I guess I don't understand the acceptable criteria when we can go
forward and make this change for something that impacts so many users
across so many architectures and SoC famili
Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support
On 08/23/2017 01:34 AM, Sekhar Nori wrote:
> On Monday 21 August 2017 10:29 PM, Franklin S Cooper Jr wrote:
>>
>>
>> On 08/21/2017 07:20 AM, Sekhar Nori wrote:
>>> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>>>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>>>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>>>> power domains needed by UART are on. Keep legacy clk api calls since other
>>>> users of this driver may not support PM runtime.
>>
>> There are a significant amount of users across a wide variety of
>> architectures and boards that I have no means to properly test to insure
>> I'm avoiding regressions. My current approach which I've seen other
>> drivers in the past use when in similar situations allows things to work
>> without regressions.
>
> Since the users are all device-tree users, my hope is they can be
> tracked by looking at the DT files and the maintainers be copied on this
> patch to make sure nothing breaks for them. I understand its not a
> trivial list, but its finite :)
>
>>>
>>> Do we really have users like that? And even if there are, cant they use
>>> PM clock handling support available in drivers/base/power/clock_ops.c ?
>>
>> I don't see any current defconfig that enables CONFIG_PM_CLK and only a
>
> Thats because its a non-user-selectable def_bool. Most configs should
> have it enabled after the 'make config' step.
>
>> handful of instances where functions from clock_ops.c are actually used.
>> I don't quite understand what your suggestion is but in general I'm
>> concerned since any approach to move everyone to different apis is
>> rather risky especially for a critical driver.
>>
>> If I'm missing your point please let me know.
>
> I do agree we dont want to break anyone. But at the same time, this
> patch redundantly enables/disables clock for most known (and possible
> future) users without no clear indication of who benefits from such
> redundancy.
>
> If we ignore this now, the problem will only get worse as time passes. I
> suggest biting the bullet now, attempt to reach-out as many affected
> parties as possible but switch to pm_runtime once and for all.
>
>>
>>>
>>> The clock enable support itself was added pretty "recently" - about 5
>>> years back with 0bbeb3c3e84b ("of serial port driver - add
>>> clk_get_rate() support"). So I doubt any really legacy platforms relied
>>> on clock support being there. It was added by Murali, I assume for
>>> Keystone devices. Keystone devices can work with runtime PM using the PM
>>> clock support pointed to above.
>>
>> You might be right but I can't be confident that it is indeed the case.
>> But its possible that at some point people will start having problems if
>> they try to use this driver for other UART instances. The currently
>> could not be aware of an issue because of the bootloader powering things
>> for them or even that different UART instances could be apart of a non
>> always on power domain.
>
> I dont think you got my point there. I have no disagreement that
> pm_runtime support is needed. In fact, it should have been that way when
> clock enable/disable was added. But thats history now.
>
>>> Perhaps linux-arm-kernel list should be copied on this submission too,
>>> since most users of this driver are likely to be there on that list.
>>>
>>
>> Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
>> bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
>> PowerPC, MIPS, Xtensa and Arc.>
>> Looking at dts that enable some of the compatible it is still a
>> combination of quite a bit of architectures.
>
> Right, I do think there are quite a few users. But they should still be
> reachable and provide testing inputs for the change.
>
>> The current approach I've taken should be safe for all users of this
>> driver which. I have no issue going another approach as long as its
>> understood that I'm fairly limited in what I can test when you take into
>> account the large number of users of this driver.
>
> Understood that you are limited by hardware you possess. But we can
> probably reach out to as many folks affected by this as possible so we
> get a fair chance at doing the "right thing" without breaking anyone.
I guess I don't understand the acceptable criteria when we can go
forward and make this change for something that impacts so many users
across so many architectures and SoC famili
Re: [PATCH v2 2/3] i2c: davinci: Add PM Runtime Support
On 08/21/2017 04:05 AM, Sekhar Nori wrote:
> On Thursday 17 August 2017 03:47 AM, Franklin S Cooper Jr wrote:
>
>> @@ -802,7 +821,6 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> dev->clk = devm_clk_get(>dev, NULL);
>> if (IS_ERR(dev->clk))
>> return PTR_ERR(dev->clk);
>> -clk_prepare_enable(dev->clk);
>>
>> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> dev->base = devm_ioremap_resource(>dev, mem);
>> @@ -811,6 +829,18 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> goto err_unuse_clocks;
>
> This goto is wrong now. There is no need to unwind the pm_runtime setup
> on a devm_ioremap_resource() failure. You can just return error here.
Ok
>
>> }
>>
>> +pm_runtime_set_autosuspend_delay(dev->dev,
>> + DAVINCI_I2C_PM_TIMEOUT);
>> +pm_runtime_use_autosuspend(dev->dev);
>> +
>> +pm_runtime_enable(dev->dev);
>> +
>> +r = pm_runtime_get_sync(dev->dev);
>> +if (r < 0) {
>> +dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
>> +goto err_unuse_clocks;
>> +}
>> +
>> i2c_davinci_init(dev);
>>
>> r = devm_request_irq(>dev, dev->irq, i2c_davinci_isr, 0,
>> @@ -849,10 +879,16 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> if (r)
>> goto err_unuse_clocks;
>>
>> +pm_runtime_mark_last_busy(dev->dev);
>> +pm_runtime_put_autosuspend(dev->dev);
>> +
>> return 0;
>>
>> err_unuse_clocks:
>> -clk_disable_unprepare(dev->clk);
>> +pm_runtime_dont_use_autosuspend(dev->dev);
>> +pm_runtime_put_sync(dev->dev);
>> +pm_runtime_disable(dev->dev);
>> +
>> dev->clk = NULL;
>
> This null setting of clk seems quite bogus and can be cleaned-up.
Do you mean that I should just remove this line?
>
>> return r;
>> }
>
> Thanks,
> Sekhar
>
Re: [PATCH v2 2/3] i2c: davinci: Add PM Runtime Support
On 08/21/2017 04:05 AM, Sekhar Nori wrote:
> On Thursday 17 August 2017 03:47 AM, Franklin S Cooper Jr wrote:
>
>> @@ -802,7 +821,6 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> dev->clk = devm_clk_get(>dev, NULL);
>> if (IS_ERR(dev->clk))
>> return PTR_ERR(dev->clk);
>> -clk_prepare_enable(dev->clk);
>>
>> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> dev->base = devm_ioremap_resource(>dev, mem);
>> @@ -811,6 +829,18 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> goto err_unuse_clocks;
>
> This goto is wrong now. There is no need to unwind the pm_runtime setup
> on a devm_ioremap_resource() failure. You can just return error here.
Ok
>
>> }
>>
>> +pm_runtime_set_autosuspend_delay(dev->dev,
>> + DAVINCI_I2C_PM_TIMEOUT);
>> +pm_runtime_use_autosuspend(dev->dev);
>> +
>> +pm_runtime_enable(dev->dev);
>> +
>> +r = pm_runtime_get_sync(dev->dev);
>> +if (r < 0) {
>> +dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
>> +goto err_unuse_clocks;
>> +}
>> +
>> i2c_davinci_init(dev);
>>
>> r = devm_request_irq(>dev, dev->irq, i2c_davinci_isr, 0,
>> @@ -849,10 +879,16 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> if (r)
>> goto err_unuse_clocks;
>>
>> +pm_runtime_mark_last_busy(dev->dev);
>> +pm_runtime_put_autosuspend(dev->dev);
>> +
>> return 0;
>>
>> err_unuse_clocks:
>> -clk_disable_unprepare(dev->clk);
>> +pm_runtime_dont_use_autosuspend(dev->dev);
>> +pm_runtime_put_sync(dev->dev);
>> +pm_runtime_disable(dev->dev);
>> +
>> dev->clk = NULL;
>
> This null setting of clk seems quite bogus and can be cleaned-up.
Do you mean that I should just remove this line?
>
>> return r;
>> }
>
> Thanks,
> Sekhar
>
Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support
On 08/21/2017 07:20 AM, Sekhar Nori wrote:
> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>> power domains needed by UART are on. Keep legacy clk api calls since other
>> users of this driver may not support PM runtime.
There are a significant amount of users across a wide variety of
architectures and boards that I have no means to properly test to insure
I'm avoiding regressions. My current approach which I've seen other
drivers in the past use when in similar situations allows things to work
without regressions.
>
> Do we really have users like that? And even if there are, cant they use
> PM clock handling support available in drivers/base/power/clock_ops.c ?
I don't see any current defconfig that enables CONFIG_PM_CLK and only a
handful of instances where functions from clock_ops.c are actually used.
I don't quite understand what your suggestion is but in general I'm
concerned since any approach to move everyone to different apis is
rather risky especially for a critical driver.
If I'm missing your point please let me know.
>
> The clock enable support itself was added pretty "recently" - about 5
> years back with 0bbeb3c3e84b ("of serial port driver - add
> clk_get_rate() support"). So I doubt any really legacy platforms relied
> on clock support being there. It was added by Murali, I assume for
> Keystone devices. Keystone devices can work with runtime PM using the PM
> clock support pointed to above.
You might be right but I can't be confident that it is indeed the case.
But its possible that at some point people will start having problems if
they try to use this driver for other UART instances. The currently
could not be aware of an issue because of the bootloader powering things
for them or even that different UART instances could be apart of a non
always on power domain.
>
> Perhaps linux-arm-kernel list should be copied on this submission too,
> since most users of this driver are likely to be there on that list.
>
Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
PowerPC, MIPS, Xtensa and Arc.
Looking at dts that enable some of the compatible it is still a
combination of quite a bit of architectures.
The current approach I've taken should be safe for all users of this
driver which. I have no issue going another approach as long as its
understood that I'm fairly limited in what I can test when you take into
account the large number of users of this driver.
>>
>> Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
>
> Thanks,
> Sekhar
>
Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support
On 08/21/2017 07:20 AM, Sekhar Nori wrote:
> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>> power domains needed by UART are on. Keep legacy clk api calls since other
>> users of this driver may not support PM runtime.
There are a significant amount of users across a wide variety of
architectures and boards that I have no means to properly test to insure
I'm avoiding regressions. My current approach which I've seen other
drivers in the past use when in similar situations allows things to work
without regressions.
>
> Do we really have users like that? And even if there are, cant they use
> PM clock handling support available in drivers/base/power/clock_ops.c ?
I don't see any current defconfig that enables CONFIG_PM_CLK and only a
handful of instances where functions from clock_ops.c are actually used.
I don't quite understand what your suggestion is but in general I'm
concerned since any approach to move everyone to different apis is
rather risky especially for a critical driver.
If I'm missing your point please let me know.
>
> The clock enable support itself was added pretty "recently" - about 5
> years back with 0bbeb3c3e84b ("of serial port driver - add
> clk_get_rate() support"). So I doubt any really legacy platforms relied
> on clock support being there. It was added by Murali, I assume for
> Keystone devices. Keystone devices can work with runtime PM using the PM
> clock support pointed to above.
You might be right but I can't be confident that it is indeed the case.
But its possible that at some point people will start having problems if
they try to use this driver for other UART instances. The currently
could not be aware of an issue because of the bootloader powering things
for them or even that different UART instances could be apart of a non
always on power domain.
>
> Perhaps linux-arm-kernel list should be copied on this submission too,
> since most users of this driver are likely to be there on that list.
>
Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
PowerPC, MIPS, Xtensa and Arc.
Looking at dts that enable some of the compatible it is still a
combination of quite a bit of architectures.
The current approach I've taken should be safe for all users of this
driver which. I have no issue going another approach as long as its
understood that I'm fairly limited in what I can test when you take into
account the large number of users of this driver.
>>
>> Signed-off-by: Franklin S Cooper Jr
>
> Thanks,
> Sekhar
>
[PATCH v5 2/4] dt-bindings: can: can-transceiver: Document new binding
Add documentation to describe usage of the new can-transceiver binding.
This new binding is applicable for any CAN device therefore it exists as
its own document.
Signed-off-by: Franklin S Cooper Jr <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 2/4] dt-bindings: can: can-transceiver: Document new binding
Add documentation to describe usage of the new can-transceiver binding.
This new binding is applicable for any CAN device therefore it exists as
its own document.
Signed-off-by: Franklin S Cooper Jr
Acked-by: Rob Herring
---
Version 5 changes:
Remove @ symbol from can-transceiver example
.../bindings/net/can/can-transceiver.txt | 24 ++
1 file changed, 24 insertions(+)
create mode 100644
Documentation/devicetree/bindings/net/can/can-transceiver.txt
diff --git a/Documentation/devicetree/bindings/net/can/can-transceiver.txt
b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
new file mode 100644
index 000..0011f53
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
@@ -0,0 +1,24 @@
+Generic CAN transceiver Device Tree binding
+--
+
+CAN transceiver typically limits the max speed in standard CAN and CAN FD
+modes. Typically these limitations are static and the transceivers themselves
+provide no way to detect this limitation at runtime. For this situation,
+the "can-transceiver" node can be used.
+
+Required Properties:
+ max-bitrate: a positive non 0 value that determines the max
+ speed that CAN/CAN-FD can run. Any other value
+ will be ignored.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+
+ can-transceiver {
+ max-bitrate = <500>;
+ };
+ ...
+};
--
2.9.4.dirty
[PATCH v5 4/4] can: m_can: Add call to of_can_transceiver
Add call to new generic functions that provides support via a binding to limit the arbitration rate and/or data rate imposed by the physical transceiver connected to the MCAN peripheral. Signed-off-by: Franklin S Cooper Jr <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 4/4] can: m_can: Add call to of_can_transceiver
Add call to new generic functions that provides support via a binding to limit the arbitration rate and/or data rate imposed by the physical transceiver connected to the MCAN peripheral. Signed-off-by: Franklin S Cooper Jr --- drivers/net/can/m_can/m_can.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index f4947a7..f72116e 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1649,6 +1649,8 @@ static int m_can_plat_probe(struct platform_device *pdev) devm_can_led_init(dev); + of_can_transceiver(dev); + dev_info(>dev, "%s device registered (irq=%d, version=%d)\n", KBUILD_MODNAME, dev->irq, priv->version); -- 2.9.4.dirty
[PATCH v5 0/4] can: Support transceiver based bit rate limit
Add a new generic binding that CAN drivers can be used to specify the max bit rate supported by a transceiver. This is useful since in some instances since the maximum speeds may be limited by the transceiver used. However, transceivers may not provide a means to determine this limitation at runtime. Therefore, create a new binding that mimics "fixed-link" that allows a user to hardcode the max speeds that can be used. Also add support for this new binding in the MCAN driver. Note this is an optional subnode so even if a driver adds support for parsing can-transceiver the user does not have to define it in their device tree. Version 5 changes: Got rid of is_bitrate_limited Removed @ symbol from can-transceiver binding Version 4 changes: Switch from fixed-transceiver to can-transceiver Drop unit address that snuck back in again. Indicate that can-transceiver is a subnode and not a property in documentation Version 3 changes: Switch from having two "max bitrates" to one universal bitrate. Version 2 changes: Rename function Define proper variable default Drop unit address Move check to changelink function Reword commit message Reword documentation Franklin S Cooper Jr (4): can: dev: Add support for limiting configured bitrate dt-bindings: can: can-transceiver: Document new binding dt-bindings: can: m_can: Document new can transceiver binding can: m_can: Add call to of_can_transceiver .../bindings/net/can/can-transceiver.txt | 24 + .../devicetree/bindings/net/can/m_can.txt | 9 + drivers/net/can/dev.c | 39 ++ drivers/net/can/m_can/m_can.c | 2 ++ include/linux/can/dev.h| 4 +++ 5 files changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt -- 2.9.4.dirty
[PATCH v5 0/4] can: Support transceiver based bit rate limit
Add a new generic binding that CAN drivers can be used to specify the max bit rate supported by a transceiver. This is useful since in some instances since the maximum speeds may be limited by the transceiver used. However, transceivers may not provide a means to determine this limitation at runtime. Therefore, create a new binding that mimics "fixed-link" that allows a user to hardcode the max speeds that can be used. Also add support for this new binding in the MCAN driver. Note this is an optional subnode so even if a driver adds support for parsing can-transceiver the user does not have to define it in their device tree. Version 5 changes: Got rid of is_bitrate_limited Removed @ symbol from can-transceiver binding Version 4 changes: Switch from fixed-transceiver to can-transceiver Drop unit address that snuck back in again. Indicate that can-transceiver is a subnode and not a property in documentation Version 3 changes: Switch from having two "max bitrates" to one universal bitrate. Version 2 changes: Rename function Define proper variable default Drop unit address Move check to changelink function Reword commit message Reword documentation Franklin S Cooper Jr (4): can: dev: Add support for limiting configured bitrate dt-bindings: can: can-transceiver: Document new binding dt-bindings: can: m_can: Document new can transceiver binding can: m_can: Add call to of_can_transceiver .../bindings/net/can/can-transceiver.txt | 24 + .../devicetree/bindings/net/can/m_can.txt | 9 + drivers/net/can/dev.c | 39 ++ drivers/net/can/m_can/m_can.c | 2 ++ include/linux/can/dev.h| 4 +++ 5 files changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt -- 2.9.4.dirty
[PATCH v5 3/4] dt-bindings: can: m_can: Document new can transceiver binding
Add information regarding can-transceiver binding. This is especially
important for MCAN since the IP allows CAN FD mode to run significantly
faster than what most transceivers are capable of.
Signed-off-by: Franklin S Cooper Jr <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 3/4] dt-bindings: can: m_can: Document new can transceiver binding
Add information regarding can-transceiver binding. This is especially
important for MCAN since the IP allows CAN FD mode to run significantly
faster than what most transceivers are capable of.
Signed-off-by: Franklin S Cooper Jr
Acked-by: Rob Herring
---
Remove @ symbol from can-transceiver example
Documentation/devicetree/bindings/net/can/m_can.txt | 9 +
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
b/Documentation/devicetree/bindings/net/can/m_can.txt
index 9e33177..0bdae2a 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -43,6 +43,11 @@ Required properties:
Please refer to 2.4.1 Message RAM Configuration in
Bosch M_CAN user manual for details.
+Optional Subnode:
+- can-transceiver : Can-transceiver subnode describing maximum speed
+ that can be used for CAN/CAN-FD modes. See
+
Documentation/devicetree/bindings/net/can/can-transceiver.txt
+ for details.
Example:
SoC dtsi:
m_can1: can@020e8000 {
@@ -64,4 +69,8 @@ Board dts:
pinctrl-names = "default";
pinctrl-0 = <_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
[PATCH v5 1/4] can: dev: Add support for limiting configured bitrate
Various CAN or CAN-FD IP may be able to run at a faster rate than
what the transceiver the CAN node is connected to. This can lead to
unexpected errors. However, CAN transceivers typically have fixed
limitations and provide no means to discover these limitations at
runtime. Therefore, add support for a can-transceiver node that
can be reused by other CAN peripheral drivers to determine for both
CAN and CAN-FD what the max bitrate that can be used. If the user
tries to configure CAN to pass these maximum bitrates it will throw
an error.
Signed-off-by: Franklin S Cooper Jr
---
Version 5 changes:
Set values for some variables at the very top.
Remove usage of is_bitrate_limited
drivers/net/can/dev.c | 39 +++
include/linux/can/dev.h | 4
2 files changed, 43 insertions(+)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..d0e5b46 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#define MOD_DESC "CAN device driver interface"
@@ -814,6 +815,30 @@ int open_candev(struct net_device *dev)
}
EXPORT_SYMBOL_GPL(open_candev);
+#ifdef CONFIG_OF
+/*
+ * Common function that can be used to understand the limitation of
+ * a transceiver when it provides no means to determine these limitations
+ * at runtime.
+ */
+void of_can_transceiver(struct net_device *dev)
+{
+ struct device_node *dn;
+ struct can_priv *priv = netdev_priv(dev);
+ struct device_node *np = dev->dev.parent->of_node;
+ int ret;
+
+ dn = of_get_child_by_name(np, "can-transceiver");
+ if (!dn)
+ return;
+
+ ret = of_property_read_u32(dn, "max-bitrate", >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
[RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
During test transmitting using CAN-FD at high bitrates (4 Mbps) only
resulted in errors. Scoping the signals I noticed that only a single bit
was being transmitted and with a bit more investigation realized the actual
MCAN IP would go back to initialization mode automatically.
It appears this issue is due to the MCAN needing to use the Transmitter
Delay Compensation Mode as defined in the MCAN User's Guide. When this
mode is used the User's Guide indicates that the Transmitter Delay
Compensation Offset register should be set. The document mentions that this
register should be set to (1/dbitrate)/2*(Func Clk Freq).
Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
that this TDC mode is only needed for data bit rates above 2.5 Mbps.
Therefore, only enable this mode and only set TDCO when the data bit rate
is above 2.5 Mbps.
Signed-off-by: Franklin S Cooper Jr
---
I'm pretty surprised that this hasn't been implemented already since
the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
supports up to 10 Mbps.
So it will be nice to get comments from users of this driver to understand
if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
If they haven't what did they do to get around it if they needed higher
speeds.
Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
everything still works at 5 Mbps which is the max speed of my CAN
transceiver.
drivers/net/can/m_can/m_can.c | 24 +++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f4947a7..720e073 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -126,6 +126,12 @@ enum m_can_mram_cfg {
#define DBTP_DSJW_SHIFT0
#define DBTP_DSJW_MASK (0xf << DBTP_DSJW_SHIFT)
+/* Transmitter Delay Compensation Register (TDCR) */
+#define TDCR_TDCO_SHIFT8
+#define TDCR_TDCO_MASK (0x7F << TDCR_TDCO_SHIFT)
+#define TDCR_TDCF_SHIFT0
+#define TDCR_TDCF_MASK (0x7F << TDCR_TDCO_SHIFT)
+
/* Test Register (TEST) */
#define TEST_LBCK BIT(4)
@@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
const struct can_bittiming *dbt = >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
Hi,
On 08/10/2017 10:29 AM, Marc Kleine-Budde wrote:
> Hello,
>
> sorry for stepping in late
>
> On 08/10/2017 02:59 AM, Franklin S Cooper Jr wrote:
>> Various CAN or CAN-FD IP may be able to run at a faster rate than
>> what the transceiver the CAN node is connected to. This can lead to
>> unexpected errors. However, CAN transceivers typically have fixed
>> limitations and provide no means to discover these limitations at
>> runtime. Therefore, add support for a can-transceiver node that
>> can be reused by other CAN peripheral drivers to determine for both
>> CAN and CAN-FD what the max bitrate that can be used. If the user
>> tries to configure CAN to pass these maximum bitrates it will throw
>> an error.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Version 4 changes:
>> Used can-transceiver instead of fixed-transceiver.
>>
>> drivers/net/can/dev.c | 50
>> +
>> include/linux/can/dev.h | 5 +
>> 2 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 365a8cc..372108f 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -27,6 +27,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>>
>> #define MOD_DESC "CAN device driver interface"
>> @@ -814,6 +815,39 @@ int open_candev(struct net_device *dev)
>> }
>> EXPORT_SYMBOL_GPL(open_candev);
>>
>> +#ifdef CONFIG_OF
>> +/*
>> + * Common function that can be used to understand the limitation of
>> + * a transceiver when it provides no means to determine these limitations
>> + * at runtime.
>> + */
>> +void of_can_transceiver(struct net_device *dev)
>> +{
>> +struct device_node *dn;
>> +struct can_priv *priv = netdev_priv(dev);
>> +struct device_node *np;
>> +unsigned int max_bitrate;
>> +int ret;
>> +
>> +np = dev->dev.parent->of_node;
>> +
>> +dn = of_get_child_by_name(np, "can-transceiver");
>> +if (!dn)
>> +return;
>> +
>> +max_bitrate = 0;
>> +ret = of_property_read_u32(dn, "max-bitrate", _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 v4 1/4] can: dev: Add support for limiting configured bitrate
On 08/10/2017 05:05 AM, Sergei Shtylyov wrote:
> Hello!
>
> On 8/10/2017 3:59 AM, Franklin S Cooper Jr wrote:
>
>> Various CAN or CAN-FD IP may be able to run at a faster rate than
>> what the transceiver the CAN node is connected to. This can lead to
>> unexpected errors. However, CAN transceivers typically have fixed
>> limitations and provide no means to discover these limitations at
>> runtime. Therefore, add support for a can-transceiver node that
>> can be reused by other CAN peripheral drivers to determine for both
>> CAN and CAN-FD what the max bitrate that can be used. If the user
>> tries to configure CAN to pass these maximum bitrates it will throw
>> an error.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Version 4 changes:
>> Used can-transceiver instead of fixed-transceiver.
>>
>> drivers/net/can/dev.c | 50
>> +
>> include/linux/can/dev.h | 5 +
>> 2 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 365a8cc..372108f 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
> [...]
>> @@ -814,6 +815,39 @@ int open_candev(struct net_device *dev)
>> }
>> EXPORT_SYMBOL_GPL(open_candev);
>> +#ifdef CONFIG_OF
>> +/*
>> + * Common function that can be used to understand the limitation of
>> + * a transceiver when it provides no means to determine these
>> limitations
>> + * at runtime.
>> + */
>> +void of_can_transceiver(struct net_device *dev)
>> +{
>> +struct device_node *dn;
>> +struct can_priv *priv = netdev_priv(dev);
>> +struct device_node *np;
>> +unsigned int max_bitrate;
>> +int ret;
>> +
>> +np = dev->dev.parent->of_node;
>
>I'd do that as an initializer.
Ok
>
>> +
>> +dn = of_get_child_by_name(np, "can-transceiver");
>> +if (!dn)
>> +return;
>> +
>> +max_bitrate = 0;
>> +ret = of_property_read_u32(dn, "max-bitrate", _bitrate);
>
>I'd initialize max_bitrate to 0 as iff of_property_read_u32() fails,
> it'll leave the variable unset...>
>> +
>> +if (max_bitrate > 0) {
>
>You risk checking unset variable here.
Four lines up I am already setting max_bitrate to a default value of 0.
>
>> +priv->max_bitrate = max_bitrate;
>> +priv->is_bitrate_limited = true;
>> +} else if (ret != -EINVAL) {
>> +netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
>> +}
>> +}
>> +EXPORT_SYMBOL(of_can_transceiver);
>> +#endif
>> +
>> /*
>>* Common close function for cleanup before the device gets closed.
>>*
> [...]
>
> MBR, Sergei
Re: [PATCH v2 1/2] usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver
On 08/17/2017 07:04 AM, Felipe Balbi wrote: > > Hi, > > Franklin S Cooper Jr <fcoo...@ti.com> writes: >> For 66AK2Gx there is a requirement to use PM Runtime to properly manage >> clocks and the power domains. Therefore, add PM runtime support. Remove >> legacy clock api's calls since other users of this driver worked without >> these clock apis calls. >> >> Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com> >> --- > > I already have the previous version of this in my tree. Seems like the > only difference was on binding document, right? Do I need to change > anything in my 'next' and/or 'testing/next' branches? Correct the binding document is the only difference. Both of your branches looks good. >
Re: [PATCH v2 1/2] usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver
On 08/17/2017 07:04 AM, Felipe Balbi wrote: > > Hi, > > Franklin S Cooper Jr writes: >> For 66AK2Gx there is a requirement to use PM Runtime to properly manage >> clocks and the power domains. Therefore, add PM runtime support. Remove >> legacy clock api's calls since other users of this driver worked without >> these clock apis calls. >> >> Signed-off-by: Franklin S Cooper Jr >> --- > > I already have the previous version of this in my tree. Seems like the > only difference was on binding document, right? Do I need to change > anything in my 'next' and/or 'testing/next' branches? Correct the binding document is the only difference. Both of your branches looks good. >
[PATCH v2 0/3] ARM: dts: keystone-k2g: Add I2C support for 66AK2G
Add I2C support to 66AK2G. Primary requirement is to add PM Runtime support to the driver. This has been tested on following platforms by performing simple i2c test such as i2c detect and reading on board i2c devices: K2G GP evm OMAPL138 K2L GP EVM and boot tested on: K2E GP EVM K2HK GP EVM Version 2 changes: Moved ordering of pm runtime calls Franklin S Cooper Jr (3): i2c: davinci: Preserve return value of devm_clk_get i2c: davinci: Add PM Runtime Support dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property .../devicetree/bindings/i2c/i2c-davinci.txt| 12 drivers/i2c/busses/i2c-davinci.c | 64 +++--- 2 files changed, 67 insertions(+), 9 deletions(-) -- 2.9.4.dirty
[PATCH v2 0/3] ARM: dts: keystone-k2g: Add I2C support for 66AK2G
Add I2C support to 66AK2G. Primary requirement is to add PM Runtime support to the driver. This has been tested on following platforms by performing simple i2c test such as i2c detect and reading on board i2c devices: K2G GP evm OMAPL138 K2L GP EVM and boot tested on: K2E GP EVM K2HK GP EVM Version 2 changes: Moved ordering of pm runtime calls Franklin S Cooper Jr (3): i2c: davinci: Preserve return value of devm_clk_get i2c: davinci: Add PM Runtime Support dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property .../devicetree/bindings/i2c/i2c-davinci.txt| 12 drivers/i2c/busses/i2c-davinci.c | 64 +++--- 2 files changed, 67 insertions(+), 9 deletions(-) -- 2.9.4.dirty
[PATCH v2 2/3] i2c: davinci: Add PM Runtime Support
66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
Version 2 changes:
Move initial calls to pm runtime autosuspend before pm_runtime_enable
drivers/i2c/busses/i2c-davinci.c | 62 ++--
1 file changed, 54 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..6b1930d 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
/* - global defines --- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000/* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem*base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(>xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,7 +821,6 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(>dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(>dev, mem);
@@ -811,6 +829,18 @@ static int davinci_i2c_probe(struct platform_device *pdev)
goto err_unuse_clocks;
}
+ pm_runtime_set_autosuspend_delay(dev->dev,
+DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ pm_runtime_enable(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
+ goto err_unuse_clocks;
+ }
+
i2c_davinci_init(dev);
r = devm_request_irq(>dev, dev->irq, i2c_davinci_isr, 0,
@@ -849,10 +879,16 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
dev->clk = NULL;
return r;
}
@@ -860,16 +896,26 @@ static int davinci_i2c_probe(struct platform_device *pdev)
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(>adapter);
- clk_disable_unprepare(dev->clk);
+ ret = pm_runtime_get_sync(>dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(>dev);
+ return ret;
+ }
+
dev->clk =
[PATCH v2 2/3] i2c: davinci: Add PM Runtime Support
66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.
Signed-off-by: Franklin S Cooper Jr
---
Version 2 changes:
Move initial calls to pm runtime autosuspend before pm_runtime_enable
drivers/i2c/busses/i2c-davinci.c | 62 ++--
1 file changed, 54 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..6b1930d 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
/* - global defines --- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000/* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem*base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(>xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,7 +821,6 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(>dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(>dev, mem);
@@ -811,6 +829,18 @@ static int davinci_i2c_probe(struct platform_device *pdev)
goto err_unuse_clocks;
}
+ pm_runtime_set_autosuspend_delay(dev->dev,
+DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ pm_runtime_enable(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
+ goto err_unuse_clocks;
+ }
+
i2c_davinci_init(dev);
r = devm_request_irq(>dev, dev->irq, i2c_davinci_isr, 0,
@@ -849,10 +879,16 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
dev->clk = NULL;
return r;
}
@@ -860,16 +896,26 @@ static int davinci_i2c_probe(struct platform_device *pdev)
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(>adapter);
- clk_disable_unprepare(dev->clk);
+ ret = pm_runtime_get_sync(>dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(>dev);
+ return ret;
+ }
+
dev->clk = NULL;
davinci_i2c
[PATCH v2 3/3] dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com> Acked-by: Rob Herring <r...@kernel.org> --- Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt index 5b123e0..64e6e65 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt @@ -6,6 +6,18 @@ davinci/keystone i2c interface contains. Required properties: - compatible: "ti,davinci-i2c" or "ti,keystone-i2c"; - reg : Offset and length of the register set for the device +- clocks: I2C functional clock phandle. + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the I2C device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Recommended properties : - interrupts : standard interrupt property. -- 2.9.4.dirty
[PATCH v2 3/3] dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm property
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G unique clocks property usage. Signed-off-by: Franklin S Cooper Jr Acked-by: Rob Herring --- Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt index 5b123e0..64e6e65 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt @@ -6,6 +6,18 @@ davinci/keystone i2c interface contains. Required properties: - compatible: "ti,davinci-i2c" or "ti,keystone-i2c"; - reg : Offset and length of the register set for the device +- clocks: I2C functional clock phandle. + For 66AK2G this property should be set per binding, + Documentation/devicetree/bindings/clock/ti,sci-clk.txt + +SoC-specific Required Properties: + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the I2C device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Recommended properties : - interrupts : standard interrupt property. -- 2.9.4.dirty
[PATCH v2 1/3] i2c: davinci: Preserve return value of devm_clk_get
The i2c driver can run into driver dependency issues if its loaded before a clock driver it depends on. Therefore, EPROBE_DEFER may be returned by devm_clk_get and should be returned in probe to allow the kernel to reprobe the driver at a later time. This patch allows the error value returned by devm_clk_get to be passed through and not overwritten. Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com> Reviewed-by: Grygorii Strashko <grygorii.stras...@ti.com> --- drivers/i2c/busses/i2c-davinci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index 33ca9a3..b8c4353 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -801,7 +801,7 @@ static int davinci_i2c_probe(struct platform_device *pdev) dev->clk = devm_clk_get(>dev, NULL); if (IS_ERR(dev->clk)) - return -ENODEV; + return PTR_ERR(dev->clk); clk_prepare_enable(dev->clk); mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); -- 2.9.4.dirty
[PATCH v2 1/3] i2c: davinci: Preserve return value of devm_clk_get
The i2c driver can run into driver dependency issues if its loaded before a clock driver it depends on. Therefore, EPROBE_DEFER may be returned by devm_clk_get and should be returned in probe to allow the kernel to reprobe the driver at a later time. This patch allows the error value returned by devm_clk_get to be passed through and not overwritten. Signed-off-by: Franklin S Cooper Jr Reviewed-by: Grygorii Strashko --- drivers/i2c/busses/i2c-davinci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index 33ca9a3..b8c4353 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -801,7 +801,7 @@ static int davinci_i2c_probe(struct platform_device *pdev) dev->clk = devm_clk_get(>dev, NULL); if (IS_ERR(dev->clk)) - return -ENODEV; + return PTR_ERR(dev->clk); clk_prepare_enable(dev->clk); mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); -- 2.9.4.dirty
[PATCH v2 2/2] dt-bindings: usb: keystone-usb: Update bindings pm and clocks properties
Update various properties to properly indicate their requirement depending on the SoC. Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com> --- Version 2: Clarify clock requirements in binding document Documentation/devicetree/bindings/usb/keystone-usb.txt | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/usb/keystone-usb.txt b/Documentation/devicetree/bindings/usb/keystone-usb.txt index 60527d3..2d1bef1 100644 --- a/Documentation/devicetree/bindings/usb/keystone-usb.txt +++ b/Documentation/devicetree/bindings/usb/keystone-usb.txt @@ -12,8 +12,21 @@ Required properties: MPU. - ranges: allows valid 1:1 translation between child's address space and parent's address space. - - clocks: Clock IDs array as required by the controller. - - clock-names: names of clocks correseponding to IDs in the clock property. + +SoC-specific Required Properties: +The following are mandatory properties for Keystone 2 66AK2HK, 66AK2L and 66AK2E +SoCs only: + +- clocks: Clock ID for USB functional clock. +- clock-names: Must be "usb". + + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the USB device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Sub-nodes: The dwc3 core should be added as subnode to Keystone DWC3 glue. -- 2.9.4.dirty
[PATCH v2 2/2] dt-bindings: usb: keystone-usb: Update bindings pm and clocks properties
Update various properties to properly indicate their requirement depending on the SoC. Signed-off-by: Franklin S Cooper Jr --- Version 2: Clarify clock requirements in binding document Documentation/devicetree/bindings/usb/keystone-usb.txt | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/usb/keystone-usb.txt b/Documentation/devicetree/bindings/usb/keystone-usb.txt index 60527d3..2d1bef1 100644 --- a/Documentation/devicetree/bindings/usb/keystone-usb.txt +++ b/Documentation/devicetree/bindings/usb/keystone-usb.txt @@ -12,8 +12,21 @@ Required properties: MPU. - ranges: allows valid 1:1 translation between child's address space and parent's address space. - - clocks: Clock IDs array as required by the controller. - - clock-names: names of clocks correseponding to IDs in the clock property. + +SoC-specific Required Properties: +The following are mandatory properties for Keystone 2 66AK2HK, 66AK2L and 66AK2E +SoCs only: + +- clocks: Clock ID for USB functional clock. +- clock-names: Must be "usb". + + +The following are mandatory properties for Keystone 2 66AK2G SoCs only: + +- power-domains: Should contain a phandle to a PM domain provider node + and an args specifier containing the USB device id + value. This property is as per the binding, + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt Sub-nodes: The dwc3 core should be added as subnode to Keystone DWC3 glue. -- 2.9.4.dirty
[PATCH v2 0/2] ARM: dts: k2g: Add support for USB instances on 66AK2G
Add support for 66AK2G usb instances. However, the driver needs to be updated to support PM_RUNTIME. This update has been validated to work on K2L and boot tested on K2HK and K2E. Version 2: Clarify clock properties requirements in binding document Franklin S Cooper Jr (2): usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver dt-bindings: usb: keystone-usb: Update bindings pm and clocks properties .../devicetree/bindings/usb/keystone-usb.txt | 17 +++-- drivers/usb/dwc3/dwc3-keystone.c | 22 ++ 2 files changed, 25 insertions(+), 14 deletions(-) -- 2.9.4.dirty
[PATCH v2 0/2] ARM: dts: k2g: Add support for USB instances on 66AK2G
Add support for 66AK2G usb instances. However, the driver needs to be updated to support PM_RUNTIME. This update has been validated to work on K2L and boot tested on K2HK and K2E. Version 2: Clarify clock properties requirements in binding document Franklin S Cooper Jr (2): usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver dt-bindings: usb: keystone-usb: Update bindings pm and clocks properties .../devicetree/bindings/usb/keystone-usb.txt | 17 +++-- drivers/usb/dwc3/dwc3-keystone.c | 22 ++ 2 files changed, 25 insertions(+), 14 deletions(-) -- 2.9.4.dirty
[PATCH v2 1/2] usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver
For 66AK2Gx there is a requirement to use PM Runtime to properly manage
clocks and the power domains. Therefore, add PM runtime support. Remove
legacy clock api's calls since other users of this driver worked without
these clock apis calls.
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
drivers/usb/dwc3/dwc3-keystone.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
index 12ee23f..d2ed952 100644
--- a/drivers/usb/dwc3/dwc3-keystone.c
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -15,7 +15,6 @@
* GNU General Public License for more details.
*/
-#include
#include
#include
#include
@@ -23,6 +22,7 @@
#include
#include
#include
+#include
/* USBSS register offsets */
#define USBSS_REVISION 0x
@@ -41,7 +41,6 @@
struct dwc3_keystone {
struct device *dev;
- struct clk *clk;
void __iomem*usbss;
};
@@ -106,17 +105,13 @@ static int kdwc3_probe(struct platform_device *pdev)
if (IS_ERR(kdwc->usbss))
return PTR_ERR(kdwc->usbss);
- kdwc->clk = devm_clk_get(kdwc->dev, "usb");
- if (IS_ERR(kdwc->clk)) {
- dev_err(kdwc->dev, "unable to get usb clock\n");
- return PTR_ERR(kdwc->clk);
- }
+ pm_runtime_enable(kdwc->dev);
- error = clk_prepare_enable(kdwc->clk);
+ error = pm_runtime_get_sync(kdwc->dev);
if (error < 0) {
- dev_err(kdwc->dev, "unable to enable usb clock, error %d\n",
+ dev_err(kdwc->dev, "pm_runtime_get_sync failed, error %d\n",
error);
- return error;
+ goto err_irq;
}
irq = platform_get_irq(pdev, 0);
@@ -147,7 +142,8 @@ static int kdwc3_probe(struct platform_device *pdev)
err_core:
kdwc3_disable_irqs(kdwc);
err_irq:
- clk_disable_unprepare(kdwc->clk);
+ pm_runtime_put_sync(kdwc->dev);
+ pm_runtime_disable(kdwc->dev);
return error;
}
@@ -167,7 +163,9 @@ static int kdwc3_remove(struct platform_device *pdev)
kdwc3_disable_irqs(kdwc);
device_for_each_child(>dev, NULL, kdwc3_remove_core);
- clk_disable_unprepare(kdwc->clk);
+ pm_runtime_put_sync(kdwc->dev);
+ pm_runtime_disable(kdwc->dev);
+
platform_set_drvdata(pdev, NULL);
return 0;
--
2.9.4.dirty
[PATCH v2 1/2] usb: dwc3: keystone: Add PM_RUNTIME Support to DWC3 Keystone USB driver
For 66AK2Gx there is a requirement to use PM Runtime to properly manage
clocks and the power domains. Therefore, add PM runtime support. Remove
legacy clock api's calls since other users of this driver worked without
these clock apis calls.
Signed-off-by: Franklin S Cooper Jr
---
drivers/usb/dwc3/dwc3-keystone.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
index 12ee23f..d2ed952 100644
--- a/drivers/usb/dwc3/dwc3-keystone.c
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -15,7 +15,6 @@
* GNU General Public License for more details.
*/
-#include
#include
#include
#include
@@ -23,6 +22,7 @@
#include
#include
#include
+#include
/* USBSS register offsets */
#define USBSS_REVISION 0x
@@ -41,7 +41,6 @@
struct dwc3_keystone {
struct device *dev;
- struct clk *clk;
void __iomem*usbss;
};
@@ -106,17 +105,13 @@ static int kdwc3_probe(struct platform_device *pdev)
if (IS_ERR(kdwc->usbss))
return PTR_ERR(kdwc->usbss);
- kdwc->clk = devm_clk_get(kdwc->dev, "usb");
- if (IS_ERR(kdwc->clk)) {
- dev_err(kdwc->dev, "unable to get usb clock\n");
- return PTR_ERR(kdwc->clk);
- }
+ pm_runtime_enable(kdwc->dev);
- error = clk_prepare_enable(kdwc->clk);
+ error = pm_runtime_get_sync(kdwc->dev);
if (error < 0) {
- dev_err(kdwc->dev, "unable to enable usb clock, error %d\n",
+ dev_err(kdwc->dev, "pm_runtime_get_sync failed, error %d\n",
error);
- return error;
+ goto err_irq;
}
irq = platform_get_irq(pdev, 0);
@@ -147,7 +142,8 @@ static int kdwc3_probe(struct platform_device *pdev)
err_core:
kdwc3_disable_irqs(kdwc);
err_irq:
- clk_disable_unprepare(kdwc->clk);
+ pm_runtime_put_sync(kdwc->dev);
+ pm_runtime_disable(kdwc->dev);
return error;
}
@@ -167,7 +163,9 @@ static int kdwc3_remove(struct platform_device *pdev)
kdwc3_disable_irqs(kdwc);
device_for_each_child(>dev, NULL, kdwc3_remove_core);
- clk_disable_unprepare(kdwc->clk);
+ pm_runtime_put_sync(kdwc->dev);
+ pm_runtime_disable(kdwc->dev);
+
platform_set_drvdata(pdev, NULL);
return 0;
--
2.9.4.dirty
[PATCH v3] serial: 8250_of: Add basic PM runtime support
66AK2G UART instances are not apart of the ALWAYS_ON power domain.
Therefore, pm_runtime calls must be made to properly insure the appropriate
power domains needed by UART are on. Keep legacy clk api calls since other
users of this driver may not support PM runtime.
Signed-off-by: Franklin S Cooper Jr <fcoo...@ti.com>
---
Version 3 changes:
Commit message updated
Drop extra parenthesis
Version 2 changes:
Fix build error
Build tested using allmodconfig
drivers/tty/serial/8250/8250_of.c | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_of.c
b/drivers/tty/serial/8250/8250_of.c
index 6c5a8ca..be90c16 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -65,6 +66,10 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
int ret;
memset(port, 0, sizeof *port);
+
+ pm_runtime_enable(>dev);
+ pm_runtime_get_sync(>dev);
+
if (of_property_read_u32(np, "clock-frequency", )) {
/* Get clk rate through clk driver if present */
@@ -72,12 +77,13 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
if (IS_ERR(info->clk)) {
dev_warn(>dev,
"clk or clock-frequency not defined\n");
- return PTR_ERR(info->clk);
+ ret = PTR_ERR(info->clk);
+ goto err_pmruntime;
}
ret = clk_prepare_enable(info->clk);
if (ret < 0)
- return ret;
+ goto err_pmruntime;
clk = clk_get_rate(info->clk);
}
@@ -170,8 +176,10 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
err_dispose:
irq_dispose_mapping(port->irq);
err_unprepare:
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ clk_disable_unprepare(info->clk);
+err_pmruntime:
+ pm_runtime_put_sync(>dev);
+ pm_runtime_disable(>dev);
return ret;
}
@@ -227,8 +235,9 @@ static int of_platform_serial_probe(struct platform_device
*ofdev)
return 0;
err_dispose:
irq_dispose_mapping(port8250.port.irq);
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ pm_runtime_put_sync(>dev);
+ pm_runtime_disable(>dev);
+ clk_disable_unprepare(info->clk);
err_free:
kfree(info);
return ret;
@@ -244,8 +253,9 @@ static int of_platform_serial_remove(struct platform_device
*ofdev)
serial8250_unregister_port(info->line);
reset_control_assert(info->rst);
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ pm_runtime_put_sync(>dev);
+ pm_runtime_disable(>dev);
+ clk_disable_unprepare(info->clk);
kfree(info);
return 0;
}
@@ -259,9 +269,10 @@ static int of_serial_suspend(struct device *dev)
serial8250_suspend_port(info->line);
- if (info->clk && (!uart_console(port) || console_suspend_enabled))
+ if (!uart_console(port) || console_suspend_enabled) {
+ pm_runtime_put_sync(dev);
clk_disable_unprepare(info->clk);
-
+ }
return 0;
}
@@ -271,8 +282,10 @@ static int of_serial_resume(struct device *dev)
struct uart_8250_port *port8250 = serial8250_get_port(info->line);
struct uart_port *port = >port;
- if (info->clk && (!uart_console(port) || console_suspend_enabled))
+ if (!uart_console(port) || console_suspend_enabled) {
+ pm_runtime_get_sync(dev);
clk_prepare_enable(info->clk);
+ }
serial8250_resume_port(info->line);
--
2.9.4.dirty
[PATCH v3] serial: 8250_of: Add basic PM runtime support
66AK2G UART instances are not apart of the ALWAYS_ON power domain.
Therefore, pm_runtime calls must be made to properly insure the appropriate
power domains needed by UART are on. Keep legacy clk api calls since other
users of this driver may not support PM runtime.
Signed-off-by: Franklin S Cooper Jr
---
Version 3 changes:
Commit message updated
Drop extra parenthesis
Version 2 changes:
Fix build error
Build tested using allmodconfig
drivers/tty/serial/8250/8250_of.c | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_of.c
b/drivers/tty/serial/8250/8250_of.c
index 6c5a8ca..be90c16 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -65,6 +66,10 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
int ret;
memset(port, 0, sizeof *port);
+
+ pm_runtime_enable(>dev);
+ pm_runtime_get_sync(>dev);
+
if (of_property_read_u32(np, "clock-frequency", )) {
/* Get clk rate through clk driver if present */
@@ -72,12 +77,13 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
if (IS_ERR(info->clk)) {
dev_warn(>dev,
"clk or clock-frequency not defined\n");
- return PTR_ERR(info->clk);
+ ret = PTR_ERR(info->clk);
+ goto err_pmruntime;
}
ret = clk_prepare_enable(info->clk);
if (ret < 0)
- return ret;
+ goto err_pmruntime;
clk = clk_get_rate(info->clk);
}
@@ -170,8 +176,10 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
err_dispose:
irq_dispose_mapping(port->irq);
err_unprepare:
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ clk_disable_unprepare(info->clk);
+err_pmruntime:
+ pm_runtime_put_sync(>dev);
+ pm_runtime_disable(>dev);
return ret;
}
@@ -227,8 +235,9 @@ static int of_platform_serial_probe(struct platform_device
*ofdev)
return 0;
err_dispose:
irq_dispose_mapping(port8250.port.irq);
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ pm_runtime_put_sync(>dev);
+ pm_runtime_disable(>dev);
+ clk_disable_unprepare(info->clk);
err_free:
kfree(info);
return ret;
@@ -244,8 +253,9 @@ static int of_platform_serial_remove(struct platform_device
*ofdev)
serial8250_unregister_port(info->line);
reset_control_assert(info->rst);
- if (info->clk)
- clk_disable_unprepare(info->clk);
+ pm_runtime_put_sync(>dev);
+ pm_runtime_disable(>dev);
+ clk_disable_unprepare(info->clk);
kfree(info);
return 0;
}
@@ -259,9 +269,10 @@ static int of_serial_suspend(struct device *dev)
serial8250_suspend_port(info->line);
- if (info->clk && (!uart_console(port) || console_suspend_enabled))
+ if (!uart_console(port) || console_suspend_enabled) {
+ pm_runtime_put_sync(dev);
clk_disable_unprepare(info->clk);
-
+ }
return 0;
}
@@ -271,8 +282,10 @@ static int of_serial_resume(struct device *dev)
struct uart_8250_port *port8250 = serial8250_get_port(info->line);
struct uart_port *port = >port;
- if (info->clk && (!uart_console(port) || console_suspend_enabled))
+ if (!uart_console(port) || console_suspend_enabled) {
+ pm_runtime_get_sync(dev);
clk_prepare_enable(info->clk);
+ }
serial8250_resume_port(info->line);
--
2.9.4.dirty
Re: [PATCH v2 3/4] can: m_can: Update documentation to mention new fixed transceiver binding
Hi Rob,
On 08/03/2017 12:07 PM, Rob Herring wrote:
> On Mon, Jul 24, 2017 at 06:05:20PM -0500, Franklin S Cooper Jr wrote:
>> Add information regarding fixed transceiver binding. This is especially
>> important for MCAN since the IP allows CAN FD mode to run significantly
>> faster than what most transceivers are capable of.
>>
>> Signed-off-by: Franklin S Cooper Jr <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
>>
Re: [PATCH v2 3/4] can: m_can: Update documentation to mention new fixed transceiver binding
Hi Rob,
On 08/03/2017 12:07 PM, Rob Herring wrote:
> On Mon, Jul 24, 2017 at 06:05:20PM -0500, Franklin S Cooper Jr wrote:
>> Add information regarding fixed transceiver binding. This is especially
>> important for MCAN since the IP allows CAN FD mode to run significantly
>> faster than what most transceivers are capable of.
>>
>> Signed-off-by: Franklin S Cooper Jr
>> ---
>> Version 2 changes:
>> Drop unit address
>>
>> Documentation/devicetree/bindings/net/can/m_can.txt | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>> index 9e33177..e4abd2c 100644
>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>> @@ -43,6 +43,11 @@ Required properties:
>>Please refer to 2.4.1 Message RAM Configuration in
>>Bosch M_CAN user manual for details.
>>
>> +Optional properties:
>> +- fixed-transceiver : Fixed-transceiver subnode describing maximum speed
>
> This is a node, not a property. Sub nodes should have their own section.
Fixed in my v4 that I just sent.
>
>> + that can be used for CAN and/or CAN-FD modes. See
>> +
>> Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>> + for details.
>> Example:
>> SoC dtsi:
>> m_can1: can@020e8000 {
>> @@ -64,4 +69,9 @@ Board dts:
>> pinctrl-names = "default";
>> pinctrl-0 = <_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
[PATCH v4 4/4] can: m_can: Add call to of_can_transceiver
Add call to new generic functions that provides support via a binding to limit the arbitration rate and/or data rate imposed by the physical transceiver connected to the MCAN peripheral. Signed-off-by: Franklin S Cooper Jr --- drivers/net/can/m_can/m_can.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index f4947a7..f72116e 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1649,6 +1649,8 @@ static int m_can_plat_probe(struct platform_device *pdev) devm_can_led_init(dev); + of_can_transceiver(dev); + dev_info(>dev, "%s device registered (irq=%d, version=%d)\n", KBUILD_MODNAME, dev->irq, priv->version); -- 2.9.4.dirty
[PATCH v4 0/4] can: Support transceiver based bit rate limit
Add a new generic binding that CAN drivers can be used to specify the max bit rate supported by a transceiver. This is useful since in some instances since the maximum speeds may be limited by the transceiver used. However, transceivers may not provide a means to determine this limitation at runtime. Therefore, create a new binding that mimics "fixed-link" that allows a user to hardcode the max speeds that can be used. Also add support for this new binding in the MCAN driver. Note this is an optional subnode so even if a driver adds support for parsing can-transceiver the user does not have to define it in their device tree. Version 4 changes: Switch from fixed-transceiver to can-transceiver Drop unit address that snuck back in again. Indicate that can-transceiver is a subnode and not a property in documentation Version 3 changes: Switch from having two "max bitrates" to one universal bitrate. Version 2 changes: Rename function Define proper variable default Drop unit address Move check to changelink function Reword commit message Reword documentation Franklin S Cooper Jr (4): can: dev: Add support for limiting configured bitrate dt-bindings: can: can-transceiver: Document new binding dt-bindings: can: m_can: Document new can transceiver binding can: m_can: Add call to of_can_transceiver .../bindings/net/can/can-transceiver.txt | 24 +++ .../devicetree/bindings/net/can/m_can.txt | 9 drivers/net/can/dev.c | 50 ++ drivers/net/can/m_can/m_can.c | 2 + include/linux/can/dev.h| 5 +++ 5 files changed, 90 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt -- 2.9.4.dirty
[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
