Re: [PATCH v2 net-next] drivers: net: davinci_mdio: prevent spurious timeout

[Sekhar Nori]

On Wednesday 09 May 2018 07:00 PM, Andrew Lunn wrote:
> On Wed, May 09, 2018 at 04:30:24PM +0530, Sekhar Nori wrote:
>> A well timed kernel preemption in the time_after() loop
>> in wait_for_idle() can result in a spurious timeout
>> error to be returned.
>>
>> Fix it by using readl_poll_timeout() which takes care of
>> this issue.
>>
>> Signed-off-by: Sekhar Nori <nsek...@ti.com>
>> ---
>> v2: use readl_poll_timeout() per suggestion from Andrew.
>>
>> The issue has not been personally observed by me, but has
>> been reported by users. Sending for next-next given the
>> non-critical nature. There is seems to be no easy way to
>> reproduce this.
>>
>>  drivers/net/ethernet/ti/davinci_mdio.c | 15 +--
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c 
>> b/drivers/net/ethernet/ti/davinci_mdio.c
>> index 3c33f4504d8e..d073432a5dbe 100644
>> --- a/drivers/net/ethernet/ti/davinci_mdio.c
>> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
>> @@ -34,6 +34,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -227,14 +228,16 @@ static inline int wait_for_user_access(struct 
>> davinci_mdio_data *data)
>>  static inline int wait_for_idle(struct davinci_mdio_data *data)
>>  {
>>  struct davinci_mdio_regs __iomem *regs = data->regs;
>> -unsigned long timeout = jiffies + msecs_to_jiffies(MDIO_TIMEOUT);
>> +u32 val, ret;
>>  
>> -while (time_after(timeout, jiffies)) {
>> -if (__raw_readl(>control) & CONTROL_IDLE)
>> -return 0;
>> +ret = readl_poll_timeout(>control, val, val & CONTROL_IDLE,
>> + 0, MDIO_TIMEOUT * 1000);
>> +if (ret) {
>> +dev_err(data->dev, "timed out waiting for idle\n");
>> +return ret;
>>  }
>> -dev_err(data->dev, "timed out waiting for idle\n");
>> -return -ETIMEDOUT;
>> +
>> +return 0;
>>  }
> 
> Hi Sekhar
> 
> You could simplify this to:
> 
>> +if (ret)
>> +dev_err(data->dev, "timed out waiting for idle\n");
>> +return ret;
> 
> Reviewed-by: Andrew Lunn <and...@lunn.ch>

Indeed. v3 sent.

Thanks,
Sekhar

[PATCH v3 next-next] drivers: net: davinci_mdio: prevent spurious timeout

[Sekhar Nori]

A well timed kernel preemption in the time_after() loop
in wait_for_idle() can result in a spurious timeout
error to be returned.

Fix it by using readl_poll_timeout() which takes care of
this issue.

Reviewed-by: Andrew Lunn <and...@lunn.ch>
Signed-off-by: Sekhar Nori <nsek...@ti.com>
---
v3: simplify return path based on comment from Andrew

The issue has not been personally observed by me, but has
been reported by users. Sending for next-next given the
non-critical nature. There is seems to be no easy way to
reproduce this.

 drivers/net/ethernet/ti/davinci_mdio.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c 
b/drivers/net/ethernet/ti/davinci_mdio.c
index 3c33f4504d8e..98a1c97fb95e 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -227,14 +228,14 @@ static inline int wait_for_user_access(struct 
davinci_mdio_data *data)
 static inline int wait_for_idle(struct davinci_mdio_data *data)
 {
struct davinci_mdio_regs __iomem *regs = data->regs;
-   unsigned long timeout = jiffies + msecs_to_jiffies(MDIO_TIMEOUT);
+   u32 val, ret;
 
-   while (time_after(timeout, jiffies)) {
-   if (__raw_readl(>control) & CONTROL_IDLE)
-   return 0;
-   }
-   dev_err(data->dev, "timed out waiting for idle\n");
-   return -ETIMEDOUT;
+   ret = readl_poll_timeout(>control, val, val & CONTROL_IDLE,
+0, MDIO_TIMEOUT * 1000);
+   if (ret)
+   dev_err(data->dev, "timed out waiting for idle\n");
+
+   return ret;
 }
 
 static int davinci_mdio_read(struct mii_bus *bus, int phy_id, int phy_reg)
-- 
2.16.2

[PATCH v2 net-next] drivers: net: davinci_mdio: prevent spurious timeout

[Sekhar Nori]

A well timed kernel preemption in the time_after() loop
in wait_for_idle() can result in a spurious timeout
error to be returned.

Fix it by using readl_poll_timeout() which takes care of
this issue.

Signed-off-by: Sekhar Nori <nsek...@ti.com>
---
v2: use readl_poll_timeout() per suggestion from Andrew.

The issue has not been personally observed by me, but has
been reported by users. Sending for next-next given the
non-critical nature. There is seems to be no easy way to
reproduce this.

 drivers/net/ethernet/ti/davinci_mdio.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c 
b/drivers/net/ethernet/ti/davinci_mdio.c
index 3c33f4504d8e..d073432a5dbe 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -227,14 +228,16 @@ static inline int wait_for_user_access(struct 
davinci_mdio_data *data)
 static inline int wait_for_idle(struct davinci_mdio_data *data)
 {
struct davinci_mdio_regs __iomem *regs = data->regs;
-   unsigned long timeout = jiffies + msecs_to_jiffies(MDIO_TIMEOUT);
+   u32 val, ret;
 
-   while (time_after(timeout, jiffies)) {
-   if (__raw_readl(>control) & CONTROL_IDLE)
-   return 0;
+   ret = readl_poll_timeout(>control, val, val & CONTROL_IDLE,
+0, MDIO_TIMEOUT * 1000);
+   if (ret) {
+   dev_err(data->dev, "timed out waiting for idle\n");
+   return ret;
}
-   dev_err(data->dev, "timed out waiting for idle\n");
-   return -ETIMEDOUT;
+
+   return 0;
 }
 
 static int davinci_mdio_read(struct mii_bus *bus, int phy_id, int phy_reg)
-- 
2.16.2

Re: [PATCH net-next] drivers: net: davinci_mdio: prevent sprious timeout

[Sekhar Nori]

On Tuesday 08 May 2018 06:18 PM, Andrew Lunn wrote:
> On Tue, May 08, 2018 at 01:56:38PM +0530, Sekhar Nori wrote:
>> A well timed kernel preemption in the time_after() loop
>> in wait_for_idle() can result in a spurious timeout
>> error to be returned.
>>
>> Fix it by checking for status of hardware before returning
>> timeout error.
>>
>> Signed-off-by: Sekhar Nori <nsek...@ti.com>
> 
> I've seen this with other drivers as well.
> 
> I suggest you make use of readx_poll_timeout(), or one of its
> cousins. They get this right.

Thanks for pointing me to these. I somehow thought these helpers are
only available with regmap.

Sending a version using readl_poll_timeout(). I know __raw_readl() is
used elsewhere in the driver, but I think that should be cleaned up
sometime to use readl_relaxed() at least. So not sure if there is
benefit in persisting with existing accessor.

Thanks,
Sekhar

[PATCH net-next] drivers: net: davinci_mdio: prevent sprious timeout

[Sekhar Nori]

A well timed kernel preemption in the time_after() loop
in wait_for_idle() can result in a spurious timeout
error to be returned.

Fix it by checking for status of hardware before returning
timeout error.

Signed-off-by: Sekhar Nori <nsek...@ti.com>
---
The issue has not been personally observed by me, but has
been reported by users. Sending for next-next given the
non-critical nature. There is seems to be no easy way to
reproduce this.

 drivers/net/ethernet/ti/davinci_mdio.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c 
b/drivers/net/ethernet/ti/davinci_mdio.c
index 3c33f4504d8e..4fbd04fd38cf 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -231,10 +231,16 @@ static inline int wait_for_idle(struct davinci_mdio_data 
*data)
 
while (time_after(timeout, jiffies)) {
if (__raw_readl(>control) & CONTROL_IDLE)
-   return 0;
+   goto out;
}
-   dev_err(data->dev, "timed out waiting for idle\n");
-   return -ETIMEDOUT;
+
+   if (!(__raw_readl(>control) & CONTROL_IDLE)) {
+   dev_err(data->dev, "timed out waiting for idle\n");
+   return -ETIMEDOUT;
+   }
+
+out:
+   return 0;
 }
 
 static int davinci_mdio_read(struct mii_bus *bus, int phy_id, int phy_reg)
-- 
2.16.2

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

On Thursday 19 October 2017 08:25 PM, 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

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

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?
> 
> 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.

I wonder whether even if a DT binding for default is provided, everyone
will end up setting it to 50% (since there is no way for them to predict
any better). In effect, I am suggesting using a hardcoded value of 50%
instead of introducing a binding without a clear need for it.

Note that I am only talking about there "offset" part of SSP here. The
transceiver loopback delay is calculated automatically by Bosch's MCAN
IP. But if there are other IPs which don't do that, then yes, that
should be a DT property IMO.

Thanks,
Sekhar

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

On Wednesday 18 October 2017 07:47 PM, Franklin S Cooper Jr wrote:
> 
> 
> 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

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

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 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 

Re: [v2,1/3] can: m_can: Make hclk optional

[Sekhar Nori]

On Thursday 21 September 2017 06:01 AM, Franklin S Cooper Jr wrote:
> 
> 
> On 08/24/2017 03:00 AM, Sekhar Nori wrote:
>> + some OMAP folks and Linux OMAP list
>>
>> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
>>> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
>>> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
>>> interface clock is managed by hwmod driver via pm_runtime_get and
>>> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
>>> and thus the driver shouldn't fail if this clock isn't found.
>>
>> I agree that hclk is defined as interface clock for M_CAN IP on DRA76x.
>>
>> However, there may be a need for the driver to know the value of hclk to
>> properly configure the RAM watchdog register which has a counter
>> counting down using hclk. Looks like the driver does not use the RAM
>> watchdog today. But if there is a need to configure it in future, it
>> might be a problem.
> 
> Honestly the RAM watchdog seems like a fundamental design problem.
> This RAM watchdog seems to be used in case a request to access the
> message ram is made but it hangs for what ever reason. Its even more
> complicated since the Message RAM is external to the MCAN IP so its
> implementation or how its handled probably differs from device to
> device. From example say you do have this error it isn't clear how you
> would recover from it. A logically answer would be to reset the entire
> IP but that also assumes that Message RAM will be reset along with the
> ip which likely depends on each SoC.
> 
> But if a readl/writel command hangs will the kernel eventually throw an
> error on its on or will the driver just hang? If it does hang can a
> driver in the ISR do something to properly terminate the driver or even
> recover from it?
>>
>> Is there a restriction in OMAP architecture against passing the
>> interface clock also in the 'clocks' property in DT. I have not tried it
>> myself, but wonder if you hit an issue that led to this patch.
> 
> No but not passing the interface clock is typical.

Okay, then it sounds like it will just be easier to pass the hclk too?

So it can be used if needed in future and also so that the driver can
stay the same as today.

Thanks,
Sekhar

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

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.

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_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_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);
>>  }
>>  
>>

Re: Fwd: DA850-evm MAC Address is random

[Sekhar Nori]

On Thursday 07 September 2017 03:11 AM, Adam Ford wrote:
> On Mon, Sep 4, 2017 at 11:42 PM, Sekhar Nori <nsek...@ti.com> wrote:
>> Hi Adam,
>>
>> On Wednesday 30 August 2017 11:08 AM, Sekhar Nori wrote:
>>>> I wonder if U-Boot isn't pushing something to Linux because it doesn't
>>>> appear to be running some of the da850 specific code even when I run
>>>> linux-next.  Can you tell me what verision of U-Boot you're using?
>>>> Other than using davinci_all_defconfig, did you change the
>>>> configuration at all?
>>
>>> I am using U-Boot 2017.01. Yes, the kernel was built using
>>> davinci_all_defconfig and no other config change. Before booting kernel,
>>> can you confirm that ethaddr is set in U-Boot environment? This is what
>>> fdt_fixup_ethernet() reads to fixup the FDT before boot.
>>>
>>> Here is my complete boot log with environment variable dump.
>>>
>>> http://pastebin.ubuntu.com/25430265/
>>
>> Were you able to get rid of the random mac address problem?
> 
> Not yet.  I haven't been able to rebuild Arago using TI's instructions
> on the Wiki.  I am not sure if it's a dependency issue or something
> else.  When I run Linux 4.13 using Buildroot as the rootfs, it does
> not appear to run da850_evm_m25p80_notify_add().  I am going to
> investigate whether or not da850_evm_init() is getting called.  I was
> wondering if you had some insight as to what calls that function?  It
> looks like it's defined as part of MACHINE_START(DAVINCI_DA850_EVM,
> "DaVinci DA850/OMAP-L138/AM18x EVM"), but I don't know how it gets
> called.

These functions are called only when booting using the legacy board file
method. From your logs before, you are booting using device tree. So
these functions are irrelevant.

Can you check if the mac address has been populated in the device-tree
by dumping it from /proc/device-tree/.../local-mac-address? That will
tell us if U-Boot is updating the mac address or not.

Thanks,
Sekhar

Re: Fwd: DA850-evm MAC Address is random

[Sekhar Nori]

Hi Adam,

On Wednesday 30 August 2017 11:08 AM, Sekhar Nori wrote:
>> I wonder if U-Boot isn't pushing something to Linux because it doesn't
>> appear to be running some of the da850 specific code even when I run
>> linux-next.  Can you tell me what verision of U-Boot you're using?
>> Other than using davinci_all_defconfig, did you change the
>> configuration at all?

> I am using U-Boot 2017.01. Yes, the kernel was built using
> davinci_all_defconfig and no other config change. Before booting kernel,
> can you confirm that ethaddr is set in U-Boot environment? This is what
> fdt_fixup_ethernet() reads to fixup the FDT before boot.
> 
> Here is my complete boot log with environment variable dump.
> 
> http://pastebin.ubuntu.com/25430265/

Were you able to get rid of the random mac address problem?

Thanks,
Sekhar

[PATCH] net: ti: cpsw-common: dont print error if ti_cm_get_macid() fails

[Sekhar Nori]

It is quite common for ti_cm_get_macid() to fail on some of the
platforms it is invoked on. They include any platform where
mac address is not part of SoC register space.

On these platforms, mac address is read and populated in
device-tree by bootloader. An example is TI DA850.

Downgrade the severity of message to "information", so it does
not spam logs when 'quiet' boot is desired.

Signed-off-by: Sekhar Nori <nsek...@ti.com>
---
 drivers/net/ethernet/ti/cpsw-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/cpsw-common.c 
b/drivers/net/ethernet/ti/cpsw-common.c
index 56ba411421f0..38d1cc557c11 100644
--- a/drivers/net/ethernet/ti/cpsw-common.c
+++ b/drivers/net/ethernet/ti/cpsw-common.c
@@ -96,7 +96,7 @@ int ti_cm_get_macid(struct device *dev, int slave, u8 
*mac_addr)
if (of_machine_is_compatible("ti,dra7"))
return davinci_emac_3517_get_macid(dev, 0x514, slave, mac_addr);
 
-   dev_err(dev, "incompatible machine/device type for reading mac 
address\n");
+   dev_info(dev, "incompatible machine/device type for reading mac 
address\n");
return -ENOENT;
 }
 EXPORT_SYMBOL_GPL(ti_cm_get_macid);
-- 
2.9.0

Re: Fwd: DA850-evm MAC Address is random

[Sekhar Nori]

On Wednesday 30 August 2017 06:19 AM, Adam Ford wrote:
> On Tue, Aug 29, 2017 at 10:20 AM, Adam Ford <aford...@gmail.com> wrote:
>> On Tue, Aug 29, 2017 at 10:16 AM, Sekhar Nori <nsek...@ti.com> wrote:
>>> On Tuesday 29 August 2017 05:32 PM, Adam Ford wrote:
>>>> On Tue, Aug 29, 2017 at 6:42 AM, Sekhar Nori <nsek...@ti.com> wrote:
>>>>> On Tuesday 29 August 2017 03:53 PM, Adam Ford wrote:
>>>>>> On Tue, Aug 29, 2017 at 3:23 AM, Sekhar Nori <nsek...@ti.com> wrote:
>>>>>>> On Tuesday 29 August 2017 02:42 AM, Tony Lindgren wrote:
>>>>>>>> * Adam Ford <aford...@gmail.com> [170828 13:33]:
>>>>>>>>> On Mon, Aug 28, 2017 at 1:54 PM, Grygorii Strashko
>>>>>>>>> <grygorii.stras...@ti.com> wrote:
>>>>>>>>>> Cc: Sekhar
>>>>>>>>>>
>>>>>>>>>> On 08/28/2017 10:32 AM, Adam Ford wrote:
>>>>>>>>>>>
>>>>>>>>>>> The davinvi_emac MAC address seems to attempt a call to
>>>>>>>>>>> ti_cm_get_macid in cpsw-common.c but it returns the message
>>>>>>>>>>> 'davinci_emac davinci_emac.1: incompatible machine/device type for
>>>>>>>>>>> reading mac address ' and then generates a random MAC address.
>>>>>>>>>>>
>>>>>>>>>>> The function appears to lookup varions boards using
>>>>>>>>>>> 'of_machine_is_compaible' and supports dm8148, am33xx, am3517, 
>>>>>>>>>>> dm816,
>>>>>>>>>>> am4372 and dra7.  I don't see the ti,davinci-dm6467-emac which is
>>>>>>>>>>> what's shown in the da850 device tree.
>>>>>>>>>>>
>>>>>>>>>>> Is there a patch somewhere for supporting the da850-evm?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Not sure if MAC address can be read from Control module.
>>>>>>>>>> May be Sekhar can say more?
>>>>>>>>>
>>>>>>>>> My understanding is that the MAC address is programmed by Logic PD
>>>>>>>>> into the SPI flash.  The Bootloader reads this from either SPI or its
>>>>>>>>> env variables.  Looking at the partition info listed in the
>>>>>>>>> da850-evm.dts file, it appears as if they've reserved space for it.
>>>>>>>>> Unfortunately, I don't see any code that reads it out.  I was hoping
>>>>>>>
>>>>>>> This code is present in U-Boot sources at
>>>>>>> board/davinci/da8xxevm/da850evm.c. See the function get_mac_addr() and
>>>>>>> its usage in misc_init_r().
>>>>>>>
>>>>>>>>> there might be a way to just pass cmdline parameter from the
>>>>>>>>> bootloader to the kernel to accept the MAC address.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> If not, is there a way to pass the MAC address from U-Boot to the
>>>>>>>>>>> driver so it doesn't generate a random MAC?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> "local-mac-address" dt porp
>>>>>>>>>
>>>>>>>>> The downside here, is that we'd have to have the Bootloader modify the
>>>>>>>>> device tree.
>>>>>>>>
>>>>>>>> That piece of code exists somewhere in u-boot already. Note how
>>>>>>>
>>>>>>> Yes, it is fdt_fixup_ethernet() and its usage is in common/image-fdt.c.
>>>>>>>
>>>>>>>> we are populating the mac address for USB Ethernet drivers in
>>>>>>>> u-boot and then the Ethernet driver code parses it. See commit
>>>>>>>> 055d31de7158 ("ARM: omap3: beagleboard-xm: dt: Add ethernet to
>>>>>>>> the device tree") for some more information.
>>>>>>>>
>>>>>>>> I think u-boot needs the ethernet alias for finding the interface.
>>>>>>>
>>>>>>&

Re: Fwd: DA850-evm MAC Address is random

[Sekhar Nori]

On Tuesday 29 August 2017 05:32 PM, Adam Ford wrote:
> On Tue, Aug 29, 2017 at 6:42 AM, Sekhar Nori <nsek...@ti.com> wrote:
>> On Tuesday 29 August 2017 03:53 PM, Adam Ford wrote:
>>> On Tue, Aug 29, 2017 at 3:23 AM, Sekhar Nori <nsek...@ti.com> wrote:
>>>> On Tuesday 29 August 2017 02:42 AM, Tony Lindgren wrote:
>>>>> * Adam Ford <aford...@gmail.com> [170828 13:33]:
>>>>>> On Mon, Aug 28, 2017 at 1:54 PM, Grygorii Strashko
>>>>>> <grygorii.stras...@ti.com> wrote:
>>>>>>> Cc: Sekhar
>>>>>>>
>>>>>>> On 08/28/2017 10:32 AM, Adam Ford wrote:
>>>>>>>>
>>>>>>>> The davinvi_emac MAC address seems to attempt a call to
>>>>>>>> ti_cm_get_macid in cpsw-common.c but it returns the message
>>>>>>>> 'davinci_emac davinci_emac.1: incompatible machine/device type for
>>>>>>>> reading mac address ' and then generates a random MAC address.
>>>>>>>>
>>>>>>>> The function appears to lookup varions boards using
>>>>>>>> 'of_machine_is_compaible' and supports dm8148, am33xx, am3517, dm816,
>>>>>>>> am4372 and dra7.  I don't see the ti,davinci-dm6467-emac which is
>>>>>>>> what's shown in the da850 device tree.
>>>>>>>>
>>>>>>>> Is there a patch somewhere for supporting the da850-evm?
>>>>>>>
>>>>>>>
>>>>>>> Not sure if MAC address can be read from Control module.
>>>>>>> May be Sekhar can say more?
>>>>>>
>>>>>> My understanding is that the MAC address is programmed by Logic PD
>>>>>> into the SPI flash.  The Bootloader reads this from either SPI or its
>>>>>> env variables.  Looking at the partition info listed in the
>>>>>> da850-evm.dts file, it appears as if they've reserved space for it.
>>>>>> Unfortunately, I don't see any code that reads it out.  I was hoping
>>>>
>>>> This code is present in U-Boot sources at
>>>> board/davinci/da8xxevm/da850evm.c. See the function get_mac_addr() and
>>>> its usage in misc_init_r().
>>>>
>>>>>> there might be a way to just pass cmdline parameter from the
>>>>>> bootloader to the kernel to accept the MAC address.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> If not, is there a way to pass the MAC address from U-Boot to the
>>>>>>>> driver so it doesn't generate a random MAC?
>>>>>>>
>>>>>>>
>>>>>>> "local-mac-address" dt porp
>>>>>>
>>>>>> The downside here, is that we'd have to have the Bootloader modify the
>>>>>> device tree.
>>>>>
>>>>> That piece of code exists somewhere in u-boot already. Note how
>>>>
>>>> Yes, it is fdt_fixup_ethernet() and its usage is in common/image-fdt.c.
>>>>
>>>>> we are populating the mac address for USB Ethernet drivers in
>>>>> u-boot and then the Ethernet driver code parses it. See commit
>>>>> 055d31de7158 ("ARM: omap3: beagleboard-xm: dt: Add ethernet to
>>>>> the device tree") for some more information.
>>>>>
>>>>> I think u-boot needs the ethernet alias for finding the interface.
>>>>
>>>> That's exactly what was missing. I have sent a patch for fixing that and
>>>> copied you there.
>>>
>>> Thanks for doing that.
>>>
>>>>
>>>> Adam, if I can get your Tested-by, I will make an attempt to send it for
>>>> v4.13 itself.
>>>
>>> I will test it.  Do need to run some instruction or do something
>>> special in U-Boot to pass this in the proper place for the kernel to
>>> pull it?  Tony's patch reference showed
>>> command for fdt set, but I am not sure I fully understand the
>>> parameters that went along with that.
>>
>> Nope, just applying the patch and booting the with the new dtb should
>> result in the random mac address going away.
> 
> Unfortunately, I am not seeing any change with the patch (at least
> with Kernel 4.12.9 from stable).
> 
> netconsole: network logging started
> davinci_emac davinci_emac.1: incompatible machine/device type for
> reading mac address
> davinci_emac davinci_emac.1: using random MAC addr: ee:74:c3:3a:15:be
> 
> Looking at the source for cpsw-common.c function ti_cm_get_macid()
> doesn't have a case for the  ti,davinci-dm6467-emac so I wonder if
> there might be more to it.

Hmm, it did solve the issue for me when I tried latest -next. And
reverting the patch brought back the random mac address usage. Could you
try latest mainline or -next?

Meanwhile let me see whats going on with the observations you have.

Thanks,
Sekhar

Re: Fwd: DA850-evm MAC Address is random

[Sekhar Nori]

On Tuesday 29 August 2017 03:53 PM, Adam Ford wrote:
> On Tue, Aug 29, 2017 at 3:23 AM, Sekhar Nori <nsek...@ti.com> wrote:
>> On Tuesday 29 August 2017 02:42 AM, Tony Lindgren wrote:
>>> * Adam Ford <aford...@gmail.com> [170828 13:33]:
>>>> On Mon, Aug 28, 2017 at 1:54 PM, Grygorii Strashko
>>>> <grygorii.stras...@ti.com> wrote:
>>>>> Cc: Sekhar
>>>>>
>>>>> On 08/28/2017 10:32 AM, Adam Ford wrote:
>>>>>>
>>>>>> The davinvi_emac MAC address seems to attempt a call to
>>>>>> ti_cm_get_macid in cpsw-common.c but it returns the message
>>>>>> 'davinci_emac davinci_emac.1: incompatible machine/device type for
>>>>>> reading mac address ' and then generates a random MAC address.
>>>>>>
>>>>>> The function appears to lookup varions boards using
>>>>>> 'of_machine_is_compaible' and supports dm8148, am33xx, am3517, dm816,
>>>>>> am4372 and dra7.  I don't see the ti,davinci-dm6467-emac which is
>>>>>> what's shown in the da850 device tree.
>>>>>>
>>>>>> Is there a patch somewhere for supporting the da850-evm?
>>>>>
>>>>>
>>>>> Not sure if MAC address can be read from Control module.
>>>>> May be Sekhar can say more?
>>>>
>>>> My understanding is that the MAC address is programmed by Logic PD
>>>> into the SPI flash.  The Bootloader reads this from either SPI or its
>>>> env variables.  Looking at the partition info listed in the
>>>> da850-evm.dts file, it appears as if they've reserved space for it.
>>>> Unfortunately, I don't see any code that reads it out.  I was hoping
>>
>> This code is present in U-Boot sources at
>> board/davinci/da8xxevm/da850evm.c. See the function get_mac_addr() and
>> its usage in misc_init_r().
>>
>>>> there might be a way to just pass cmdline parameter from the
>>>> bootloader to the kernel to accept the MAC address.
>>>>
>>>>>
>>>>>>
>>>>>> If not, is there a way to pass the MAC address from U-Boot to the
>>>>>> driver so it doesn't generate a random MAC?
>>>>>
>>>>>
>>>>> "local-mac-address" dt porp
>>>>
>>>> The downside here, is that we'd have to have the Bootloader modify the
>>>> device tree.
>>>
>>> That piece of code exists somewhere in u-boot already. Note how
>>
>> Yes, it is fdt_fixup_ethernet() and its usage is in common/image-fdt.c.
>>
>>> we are populating the mac address for USB Ethernet drivers in
>>> u-boot and then the Ethernet driver code parses it. See commit
>>> 055d31de7158 ("ARM: omap3: beagleboard-xm: dt: Add ethernet to
>>> the device tree") for some more information.
>>>
>>> I think u-boot needs the ethernet alias for finding the interface.
>>
>> That's exactly what was missing. I have sent a patch for fixing that and
>> copied you there.
> 
> Thanks for doing that.
> 
>>
>> Adam, if I can get your Tested-by, I will make an attempt to send it for
>> v4.13 itself.
> 
> I will test it.  Do need to run some instruction or do something
> special in U-Boot to pass this in the proper place for the kernel to
> pull it?  Tony's patch reference showed
> command for fdt set, but I am not sure I fully understand the
> parameters that went along with that.

Nope, just applying the patch and booting the with the new dtb should
result in the random mac address going away.

Thanks,
Sekhar

Re: Fwd: DA850-evm MAC Address is random

[Sekhar Nori]

On Tuesday 29 August 2017 02:42 AM, Tony Lindgren wrote:
> * Adam Ford  [170828 13:33]:
>> On Mon, Aug 28, 2017 at 1:54 PM, Grygorii Strashko
>>  wrote:
>>> Cc: Sekhar
>>>
>>> On 08/28/2017 10:32 AM, Adam Ford wrote:

 The davinvi_emac MAC address seems to attempt a call to
 ti_cm_get_macid in cpsw-common.c but it returns the message
 'davinci_emac davinci_emac.1: incompatible machine/device type for
 reading mac address ' and then generates a random MAC address.

 The function appears to lookup varions boards using
 'of_machine_is_compaible' and supports dm8148, am33xx, am3517, dm816,
 am4372 and dra7.  I don't see the ti,davinci-dm6467-emac which is
 what's shown in the da850 device tree.

 Is there a patch somewhere for supporting the da850-evm?
>>>
>>>
>>> Not sure if MAC address can be read from Control module.
>>> May be Sekhar can say more?
>>
>> My understanding is that the MAC address is programmed by Logic PD
>> into the SPI flash.  The Bootloader reads this from either SPI or its
>> env variables.  Looking at the partition info listed in the
>> da850-evm.dts file, it appears as if they've reserved space for it.
>> Unfortunately, I don't see any code that reads it out.  I was hoping

This code is present in U-Boot sources at
board/davinci/da8xxevm/da850evm.c. See the function get_mac_addr() and
its usage in misc_init_r().

>> there might be a way to just pass cmdline parameter from the
>> bootloader to the kernel to accept the MAC address.
>>
>>>

 If not, is there a way to pass the MAC address from U-Boot to the
 driver so it doesn't generate a random MAC?
>>>
>>>
>>> "local-mac-address" dt porp
>>
>> The downside here, is that we'd have to have the Bootloader modify the
>> device tree.
> 
> That piece of code exists somewhere in u-boot already. Note how

Yes, it is fdt_fixup_ethernet() and its usage is in common/image-fdt.c.

> we are populating the mac address for USB Ethernet drivers in
> u-boot and then the Ethernet driver code parses it. See commit
> 055d31de7158 ("ARM: omap3: beagleboard-xm: dt: Add ethernet to
> the device tree") for some more information.
> 
> I think u-boot needs the ethernet alias for finding the interface.

That's exactly what was missing. I have sent a patch for fixing that and
copied you there.

Adam, if I can get your Tested-by, I will make an attempt to send it for
v4.13 itself.

Thanks,
Sekhar

Re: [v2,3/3] can: m_can: Add PM Runtime

[Sekhar Nori]

+ 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).

> 
> 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

[Sekhar Nori]

+ some OMAP folks and Linux OMAP list

On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
> interface clock is managed by hwmod driver via pm_runtime_get and
> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
> and thus the driver shouldn't fail if this clock isn't found.

I agree that hclk is defined as interface clock for M_CAN IP on DRA76x.

However, there may be a need for the driver to know the value of hclk to
properly configure the RAM watchdog register which has a counter
counting down using hclk. Looks like the driver does not use the RAM
watchdog today. But if there is a need to configure it in future, it
might be a problem.

Is there a restriction in OMAP architecture against passing the
interface clock also in the 'clocks' property in DT. I have not tried it
myself, but wonder if you hit an issue that led to this patch.

> 
> Signed-off-by: Franklin S Cooper Jr 
> ---
> 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 0/2] net: phy: dp83867: workaround incorrect RX_CTRL pin strap

[Sekhar Nori]

Hi Andrew,

On Tuesday 04 July 2017 08:17 PM, Andrew Lunn wrote:
> On Tue, Jul 04, 2017 at 04:23:22PM +0530, Sekhar Nori wrote:
>> Hi,
>>
>> This patch series adds workaround for incorrect RX_CTRL pin strap
>> setting that can be found on some TI boards.
> 
> Hi Sekhar
> 
> Do you plan to post some DT patches to make use of this? It would be
> good to seem them as well.

Actually I ended up posting those prior to this series (done in error)
and have promised Tony a resend once this is accepted.

http://www.spinics.net/lists/devicetree/msg183487.html

I will Cc the same audience on the resend.

Thanks,
Sekhar

[PATCH 2/2] net: phy: dp83867: add workaround for incorrect RX_CTRL pin strap

[Sekhar Nori]

From: Murali Karicheri <m-kariche...@ti.com>

The data manual for DP83867IR/CR, SNLS484E[1], revised march 2017,
advises that strapping RX_DV/RX_CTRL pin in mode 1 and 2 is not
supported (see note below Table 5 (4-Level Strap Pins)).

There are some boards which have the pin strapped this way and need
software workaround suggested by the data manual. Bit[7] of
Configuration Register 4 (address 0x0031) must be cleared to 0. This
ensures proper operation of the PHY.

Implement driver support for device-tree property meant to advertise
the wrong strapping.

[1] http://www.ti.com/lit/ds/snls484e/snls484e.pdf

Signed-off-by: Murali Karicheri <m-kariche...@ti.com>
[nsek...@ti.com: rebase to mainline, code simplification]
Signed-off-by: Sekhar Nori <nsek...@ti.com>
---
 drivers/net/phy/dp83867.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index b57f20e552ba..c1ab976cc800 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -91,6 +91,7 @@ struct dp83867_private {
int fifo_depth;
int io_impedance;
int port_mirroring;
+   bool rxctrl_strap_quirk;
 };
 
 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -164,6 +165,9 @@ static int dp83867_of_init(struct phy_device *phydev)
else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN;
 
+   dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node,
+   "ti,dp83867-rxctrl-strap-quirk");
+
ret = of_property_read_u32(of_node, "ti,rx-internal-delay",
   >rx_id_delay);
if (ret &&
@@ -214,6 +218,13 @@ static int dp83867_config_init(struct phy_device *phydev)
dp83867 = (struct dp83867_private *)phydev->priv;
}
 
+   /* RX_DV/RX_CTRL strapped in mode 1 or mode 2 workaround */
+   if (dp83867->rxctrl_strap_quirk) {
+   val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4);
+   val &= ~BIT(7);
+   phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4, val);
+   }
+
if (phy_interface_is_rgmii(phydev)) {
val = phy_read(phydev, MII_DP83867_PHYCTRL);
if (val < 0)
-- 
2.9.0

[PATCH 1/2] dt-bindings: phy: dp83867: provide a workaround for incorrect RX_CTRL pin strap

[Sekhar Nori]

From: Murali Karicheri <m-kariche...@ti.com>

The data manual for DP83867IR/CR, SNLS484E[1], revised march 2017,
advises that strapping RX_DV/RX_CTRL pin in mode 1 and 2 is not
supported (see note below Table 5 (4-Level Strap Pins)).

It further advises that if a board has this pin strapped in mode 1 and
mode 2, then to ensure proper operation of the PHY, a software workaround
must be implemented.

Since it is not possible to detect in software if RX_DV/RX_CTRL pin is
incorrectly strapped, add a device-tree property for the board to
advertise this and allow corrective action in software.

[1] http://www.ti.com/lit/ds/snls484e/snls484e.pdf

Signed-off-by: Murali Karicheri <m-kariche...@ti.com>
[nsek...@ti.com: rebase to mainline, split documentation into separate patch]
Signed-off-by: Sekhar Nori <nsek...@ti.com>
---
 Documentation/devicetree/bindings/net/ti,dp83867.txt | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt 
b/Documentation/devicetree/bindings/net/ti,dp83867.txt
index afe9630a5e7d..02c4353b5cf2 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
@@ -18,6 +18,13 @@ Optional property:
- ti,max-output-impedance - MAC Interface Impedance control to set
the programmable output impedance to
maximum value (70 ohms).
+   - ti,dp83867-rxctrl-strap-quirk - This denotes the fact that the
+   board has RX_DV/RX_CTRL pin strapped in
+   mode 1 or 2. To ensure PHY operation,
+   there are specific actions that
+   software needs to take when this pin is
+   strapped in these modes. See data manual
+   for details.
 
 Note: ti,min-output-impedance and ti,max-output-impedance are mutually
   exclusive. When both properties are present ti,max-output-impedance
-- 
2.9.0

[PATCH 0/2] net: phy: dp83867: workaround incorrect RX_CTRL pin strap

[Sekhar Nori]

Hi,

This patch series adds workaround for incorrect RX_CTRL pin strap
setting that can be found on some TI boards.

This is required to be complaint to PHY datamanual specification.

Murali Karicheri (2):
  dt-bindings: phy: dp83867: provide a workaround for incorrect RX_CTRL
pin strap
  net: phy: dp83867: add workaround for incorrect RX_CTRL pin strap

 Documentation/devicetree/bindings/net/ti,dp83867.txt |  7 +++
 drivers/net/phy/dp83867.c| 11 +++
 2 files changed, 18 insertions(+)

-- 
2.9.0

[PATCH] MAINTAINERS: update entry for TI's CPSW driver

[Sekhar Nori]

Mugunthan V N, who was reviewing TI's CPSW driver patches is
not working for TI anymore and wont be reviewing patches for
that driver.

Drop Mugunthan as the maintiainer for this driver.

Grygorii continues to be a reviewer. Dave Miller applies the
patches directly and adding a maintainer is actually
misleading since get_maintainer.pl script stops suggesting
that Dave Miller be copied.

Signed-off-by: Sekhar Nori <nsek...@ti.com>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 860dacbb5be3..c6d89f004e25 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12684,7 +12684,6 @@ F:  drivers/clk/ti/
 F: include/linux/clk/ti.h
 
 TI ETHERNET SWITCH DRIVER (CPSW)
-M: Mugunthan V N <mugunthan...@ti.com>
 R: Grygorii Strashko <grygorii.stras...@ti.com>
 L: linux-o...@vger.kernel.org
 L: netdev@vger.kernel.org
-- 
2.9.0

[PATCH] net: ethernet: ti: cpsw: fix race condition during open()

[Sekhar Nori]

TI's cpsw driver handles both OF and non-OF case for phy
connect. Unfortunately of_phy_connect() returns NULL on
error while phy_connect() returns ERR_PTR().

To handle this, cpsw_slave_open() overrides the return value
from phy_connect() to make it NULL or error.

This leaves a small window, where cpsw_adjust_link() may be
invoked for a slave while slave->phy pointer is temporarily
set to -ENODEV (or some other error) before it is finally set
to NULL.

_cpsw_adjust_link() only handles the NULL case, and an oops
results when ERR_PTR() is seen by it.

Note that cpsw_adjust_link() checks PHY status for each
slave whenever it is invoked. It can so happen that even
though phy_connect() for a given slave returns error,
_cpsw_adjust_link() is still called for that slave because
the link status of another slave changed.

Fix this by using a temporary pointer to store return value
of {of_}phy_connect() and do a one-time write to slave->phy.

Reviewed-by: Grygorii Strashko <grygorii.stras...@ti.com>
Reported-by: Yan Liu <yan-...@ti.com>
Signed-off-by: Sekhar Nori <nsek...@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 9f3d9c67e3fe..5a4faa4489d0 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1267,6 +1267,7 @@ static void soft_reset_slave(struct cpsw_slave *slave)
 static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
 {
u32 slave_port;
+   struct phy_device *phy;
struct cpsw_common *cpsw = priv->cpsw;
 
soft_reset_slave(slave);
@@ -1300,27 +1301,28 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
   1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
 
if (slave->data->phy_node) {
-   slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
+   phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
-   if (!slave->phy) {
+   if (!phy) {
dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
slave->data->phy_node->full_name,
slave->slave_num);
return;
}
} else {
-   slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
+   phy = phy_connect(priv->ndev, slave->data->phy_id,
 _adjust_link, slave->data->phy_if);
-   if (IS_ERR(slave->phy)) {
+   if (IS_ERR(phy)) {
dev_err(priv->dev,
"phy \"%s\" not found on slave %d, err %ld\n",
slave->data->phy_id, slave->slave_num,
-   PTR_ERR(slave->phy));
-   slave->phy = NULL;
+   PTR_ERR(phy));
return;
}
}
 
+   slave->phy = phy;
+
phy_attached_info(slave->phy);
 
phy_start(slave->phy);
-- 
2.9.0

Re: [PATCH 2/2] ARM: dts: am335x-icev2: Add CPSW ethernet0 and ethernet1

[Sekhar Nori]

On Tuesday 14 March 2017 12:54 AM, Grygorii Strashko wrote:
> 
> 
> On 03/13/2017 08:42 AM, Roger Quadros wrote:
>> Enable the 2 ethernet ports as CPSW ports in dual-mac mode
>>
>> Signed-off-by: Roger Quadros <rog...@ti.com>
>> [nsek...@ti.com: use AM33XX_IOPAD()]
>> Signed-off-by: Sekhar Nori <nsek...@ti.com>
>> ---
>>  arch/arm/boot/dts/am335x-icev2.dts | 113
>> +
>>  1 file changed, 113 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/am335x-icev2.dts
>> b/arch/arm/boot/dts/am335x-icev2.dts
>> index a2ad076..cc343b0 100644
>> --- a/arch/arm/boot/dts/am335x-icev2.dts
>> +++ b/arch/arm/boot/dts/am335x-icev2.dts
>> @@ -201,6 +201,69 @@
>>  AM33XX_IOPAD(0x938, PIN_OUTPUT_PULLUP | MUX_MODE1) /*
>> (L16) gmii1_rxd2.uart3_txd */
>>  >;
>>  };
>> +
> 
>>
>>   {
>> @@ -350,3 +413,53 @@
>>  pinctrl-0 = <_pins_default>;
>>  status = "okay";
>>  };
>> +
>> + {
>> +p4 {
>> +gpio-hog;
>> +gpios = <4 GPIO_ACTIVE_HIGH>;
>> +output-high;
>> +line-name = "PR1_MII_CTRL";
>> +};
>> +
>> +p10 {
>> +gpio-hog;
>> +gpios = <10 GPIO_ACTIVE_HIGH>;
>> +/* ETH1 mux: Low for MII-PRU, high for RMII-CPSW */
>> +output-high;
>> +line-name = "MUX_MII_CTL1";
>> +};
>> +};
>> +
>> +_emac0 {
>> +phy_id = <_mdio>, <1>;
> 
> this is deprecated definition.
> pls, use phy-handle.

Perhaps cpsw driver should warn about using deprecated DT properties.

Thanks,
Sekhar

Re: next-20170110 build: 1 failures 4 warnings (next-20170110)

[Sekhar Nori]

On Wednesday 11 January 2017 01:51 AM, Stephen Rothwell wrote:
> Hi Mark,
> 
> On Tue, 10 Jan 2017 18:16:07 + Mark Brown  wrote:
>>
>> On Tue, Jan 10, 2017 at 07:21:32AM +, Build bot for Mark Brown wrote:
>>
>> Today's -next fails to build an arm allmodconfig due to:
>>
>>> arm-allmodconfig
>>> ../drivers/net/ethernet/ti/netcp_core.c:1951:28: error: initialization from 
>>> incompatible pointer type [-Werror=incompatible-pointer-types]  
>>
>> caused by 6a8162e99ef344 (net: netcp: store network statistics in 64
>> bits).  It's assigning the function
>>
>>   static struct rtnl_link_stats64 *
>>   netcp_get_stats(struct net_device *ndev, struct rtnl_link_stats64 *stats)
>>
>> to ndo_get_stats64 which expects a function returning void.
> 
> Yes, but only because commit bc1f44709cf2 ("net: make ndo_get_stats64 a
> void function") entered the net-next tree on the same day ... so it
> needs a followup fixup patch for this new usage.

Keerthy sent a patch fixing this yesterday. Looks like he will have to
spin another version though.

https://patchwork.ozlabs.org/patch/713224/

Thanks,
Sekhar

Re: [net-next PATCH 1/3] net: phy: dp83867: Add documentation for optional impedance control

[Sekhar Nori]

Nishanth,

On Wednesday 20 July 2016 09:03 PM, Nishanth Menon wrote:
> On 07/20/2016 09:56 AM, Mugunthan V N wrote:
>> Add documention of ti,impedance-control which can be used to
>> correct MAC impedance mismatch using phy extended registers.
>>
>> Signed-off-by: Mugunthan V N 
>> ---
>>   Documentation/devicetree/bindings/net/ti,dp83867.txt | 7 +++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt
>> b/Documentation/devicetree/bindings/net/ti,dp83867.txt
>> index 5d21141..531ea3c5 100644
>> --- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
>> +++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
>> @@ -9,6 +9,13 @@ Required properties:
>>   - ti,fifo-depth - Transmitt FIFO depth- see
>> dt-bindings/net/ti-dp83867.h
>>   for applicable values
>>
>> +Optional property:
>> +- ti,impedance-control - MAC Interface Impedance control to vary the
>> + output impedance with an approximate range
>> + from 35-70 ohms in 32 steps. Value range can
>> + be 0x0 to 0x1f. Lowest impedance value is
>> + 0x1f and highest impedance being 0x0.
>> +
> 
> Should'nt you be using the impedance values of 35 to 70 as the valid
> values here? that would be the hardware description, and the values to
> program corresponding to those are 0x00 to 0x1f. Right?
> 
> Rob: is'nt that the right way to do it?

Agree that that is usually the right way to do it. I believe this case
is bit peculiar though. Here is the extract from the datasheet[1] about
how the register in question works.

"
Output impedance approximate range from 35-70 ohms in 32 steps.
Lowest being 0 and highest being 0. Range and Step size
will vary with process.

Default is set to 50 ohms by trim. But the default register value can
vary by process. Non default values of MAC I/O impedance can be
used based on trace impedance. Mismatch between device and
trace impedance can cause voltage overshoot and undershoot.
"

So clearly, there is no easy correspondence that the hardware guarantees
between output impedance ohmage and the register value programmed. Only
couple of things are guaranteed.

1) Programming a value of 0 gives approximately 35 ohms
2) Programming a value of 0x1F gives approximately 70 ohms
3) Default value of the register gives 50 ohms (the default value could
vary by device).
4) Programming a value in between will give some ohmage in the
approximate range 35-70 (curve is unknown).

Given this, I am not sure how one can convert a given user supplied ohms
values to a reasonable register value. Clearly, the register is supposed
to be programmed by experimentation, not calculation.

That said, we could take another approach. At least for the current
issue we are trying to address, we only need to configure the register
to max value. And given the nature of the register, I am pretty sure
that is what most people will end up doing.

So, may be we have two properties:

ti,max-output-impedance
ti,min-output-imepdance

which configure the impedance to maximum and minimum values
respectively. If, in future, someone needs to configure a value in
between, he can introduce a more granular property which when then
override whatever else is specified.

Regards,
Sekhar

[1] http://www.ti.com/lit/ds/symlink/dp83867ir.pdf

Re: [net-next PATCH 1/3] drivers: net: cpsw: add am335x errata workarround for interrutps

[Sekhar Nori]

Hi Mugunthan,

On Wednesday 12 August 2015 03:22 PM, Mugunthan V N wrote:
 +static const struct of_device_id cpsw_of_mtable[] = {
 + { .compatible = ti,cpsw, .data = cpsw_devtype[CPSW], },

 + { .compatible = ti,am335x-cpsw, .data = cpsw_devtype[AM335X_CPSW], },
 + { .compatible = ti,am4372-cpsw, .data = cpsw_devtype[AM4372_CPSW], },
 + { .compatible = ti,dra7-cpsw, .data = cpsw_devtype[DRA7_CPSW], },

I do not see documentation added for these compatibles. Since the series
is already applied, can you send additional patches adding documentation?

Thanks,
Sekhar
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html