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