Hi Anssi Hannula, Thanks for the patch... Please find response inline. <Snip> > On 07. 03. 19 13:52, Anssi Hannula wrote: > > Currently the xilinx_can xcan_chip_start() function, called from > > .ndo_open() and via CAN_MODE_START (bus-off restart), waits for the SR > > register to show the wanted operating state, with a 1 sec timeout. > > > > However, that register bit will only be set once the HW has observed > > 11 consecutive recessive bits (BusIdle) on the bus. > > > > If the bus will not see the 11 bits (e.g. it is stuck dominant), the > > function will timeout and return an error. > > If this was done as part of a scheduled restart from bus-off, the > > interface will stay in bus-off state forever even if the bus recovers > > later. > > If this was done during interface .ndo_open(), the interface will stay > > down. > > > > According to M_CAN and FLEXCAN documentation they also wait for 11 > > consecutive recessive bits, but their drivers do not seem to wait for > > that. > > > > To make the behavior consistent, modify xilinx_can to also not wait > > for the synchronization to complete. > > > > The only way for users to know for sure that the bus has been joined > > successfully is to see successfully received or transmitted frames. > > That does not seem optimal, but it is consistent with other drivers > > and we should have a properly working restart-ms with xilinx_can. > > > > Tested on ZynqMP with Xilinx CAN-FD 1.0. > > > > Signed-off-by: Anssi Hannula <anssi.hannula@xxxxxxxxxx> > > --- > > > > Not really sure if this way is the preferred way forward. > > > > One alternative way would be to keep the wait but log a warning and > > allow the function to succeed? > > > > But I believe something needs to be done, as it seems a reasonable > > expectation that if an interface is brought up with restart-ms, the > > interface should recover to a working state even after periods of bus > > being in whatever invalid state. > > > > > > The HW waiting is described for Xilinx HW in [1] "Register > > Configuration Sequence", for M_CAN in [2] 3.1.1 Software > > Initialization, for flexcan in [3] 34.4.10.1 Freeze mode. > > > > [1] > > > https://www.xilinx.com/support/documentation/ip_documentation/canfd/v2 > > _0/pg223-canfd.pdf [2] > > http://www.bosch- > semiconductors.com/media/ip_modules/pdf_2/m_can/mcan_ > > users_manual_v3212.pdf [3] > > https://www.nxp.com/docs/en/reference-manual/MC56F8458XRM.pdf > > > > > > drivers/net/can/xilinx_can.c | 13 +------------ > > 1 file changed, 1 insertion(+), 12 deletions(-) > > > > diff --git a/drivers/net/can/xilinx_can.c > > b/drivers/net/can/xilinx_can.c index 97d0933d9bd9..13abc9e1a7ed 100644 > > --- a/drivers/net/can/xilinx_can.c > > +++ b/drivers/net/can/xilinx_can.c > > @@ -400,9 +400,8 @@ static int xcan_set_bittiming(struct net_device > > *ndev) static int xcan_chip_start(struct net_device *ndev) { > > struct xcan_priv *priv = netdev_priv(ndev); > > - u32 reg_msr, reg_sr_mask; > > + u32 reg_msr; > > int err; > > - unsigned long timeout; > > u32 ier; > > > > /* Check if it is in reset mode */ > > @@ -428,10 +427,8 @@ static int xcan_chip_start(struct net_device *ndev) > > /* Check whether it is loopback mode or normal mode */ > > if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) { > > reg_msr = XCAN_MSR_LBACK_MASK; > > - reg_sr_mask = XCAN_SR_LBACK_MASK; > > } else { > > reg_msr = 0x0; > > - reg_sr_mask = XCAN_SR_NORMAL_MASK; > > } > > > > /* enable the first extended filter, if any, as cores with extended > > @@ -443,14 +440,6 @@ static int xcan_chip_start(struct net_device *ndev) > > priv->write_reg(priv, XCAN_MSR_OFFSET, reg_msr); > > priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK); > > > > - timeout = jiffies + XCAN_TIMEOUT; > > - while (!(priv->read_reg(priv, XCAN_SR_OFFSET) & reg_sr_mask)) { > > - if (time_after(jiffies, timeout)) { > > - netdev_warn(ndev, > > - "timed out for correct mode\n"); > > - return -ETIMEDOUT; > > - } > > - } Instead of deleting the timeout loop we can check for the bus off state here(SR Register -- Error Status fields). If the controller is in bus off state skip this timeout loop... Regards, Kedar. > > netdev_dbg(ndev, "status:#x%08x\n", > > priv->read_reg(priv, XCAN_SR_OFFSET)); > > > > > > Xilinx folks: Please take a look at this RFC. > > Thanks, > Michal
