lkml.org 
[lkml]   [2014]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5] can: xilinx CAN controller support.
    On 03/11/2014 03:08 PM, Appana Durga Kedareswara Rao wrote:

    >>>>> + struct napi_struct napi;
    >>>>> + u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
    >>>>> + void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
    >>>>> + u32 val);
    >>>>> + struct net_device *dev;
    >>>>> + void __iomem *reg_base;
    >>>>> + unsigned long irq_flags;
    >>>>> + struct clk *aperclk;
    >>>>> + struct clk *devclk;
    >>>>
    >>>> Please rename the clock variables to match the names in the DT.
    >>>>
    >>> The clock names are different for axi CAN and CANPS case.
    >>> So will make them as busclk and devclk Are you ok with this?
    >>
    >> Why not "ref_clk" and "aper_clk" as used in the DT?
    >>
    > One of the comments I got from the Soren(sorenb@xilinx.com)
    > Is the clock-names must match the data sheet.
    > If I Modify the clock names then it is different names for AXI CAN
    > and CANPS case.

    Sorry, my faul, I thought the names are already these from the
    datasheet. As Sören pointed out please use 's_axi_aclk' and
    'can_clk' for the DT and for the the variable names in the private
    struct, too.

    The 'official' name of the ip core seems to be axi_can, should we rename
    the driver? I suspect, that Michal wants to keep xilinx in the name for
    marketing reasons :P

    [...]

    >>>>> +/**
    >>>>> + * xcan_chip_start - This the drivers start routine
    >>>>> + * @ndev: Pointer to net_device structure
    >>>>> + *
    >>>>> + * This is the drivers start routine.
    >>>>> + * Based on the State of the CAN device it puts
    >>>>> + * the CAN device into a proper mode.
    >>>>> + *
    >>>>> + * Return: 0 on success and failure value on error */ static int
    >>>>> +xcan_chip_start(struct net_device *ndev) {
    >>>>> + struct xcan_priv *priv = netdev_priv(ndev);
    >>>>> + u32 err;
    >>>>> + unsigned long timeout;
    >>>>> +
    >>>>> + /* Check if it is in reset mode */
    >>>>> + err = set_reset_mode(ndev);
    >>>>> + if (err < 0)
    >>>>> + return err;
    >>>>> +
    >>>>> + err = xcan_set_bittiming(ndev);
    >>>>> + if (err < 0)
    >>>>> + return err;
    >>>>> +
    >>>>> + /* Enable interrupts */
    >>>>> + priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);
    >>>>> +
    >>>>> + /* Check whether it is loopback mode or normal mode */
    >>>>> + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
    >>>>> + /* Put device into loopback mode */
    >>>>> + priv->write_reg(priv, XCAN_MSR_OFFSET,
    >>>> XCAN_MSR_LBACK_MASK);
    >>>>> + else
    >>>>> + /* The device is in normal mode */
    >>>>> + priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
    >>>>> +
    >>>>> + if (priv->can.state == CAN_STATE_STOPPED) {
    >>>>
    >>>> Your device is in stopped mode, add you go though set_reset_mode()
    >>>> already.
    >>>
    >>> Will remove this if condition I putted this condition here because in
    >>> set_reset_mode we are putting the Device state in Stopped state.
    >>
    >> So the device is in CAN_STATE_STOPPED, as this code just went through
    >> set_reset_mode() some lines ago. so it makes no sense to check if it
    >> (still) is (the code runs serialized here).
    >>
    >
    > After removing the if condition and the line(CAN_STATE_STOPPED from
    > the set_reset_mode) the code for chip_start is below It is better to
    > check whether it is really Configured in the loopback or normal mode
    > That's why kept the loops as it is.
    >
    > static int xcan_chip_start(struct net_device *ndev)
    > {
    > struct xcan_priv *priv = netdev_priv(ndev);
    > u32 err;
    > unsigned long timeout;
    >
    > /* Check if it is in reset mode */
    > err = set_reset_mode(ndev);
    > if (err < 0)
    > return err;
    >
    > err = xcan_set_bittiming(ndev);
    > if (err < 0)
    > return err;
    >
    > /* Enable interrupts */
    > priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);
    >
    > /* Check whether it is loopback mode or normal mode */
    > if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
    > /* Put device into loopback mode */
    > priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_LBACK_MASK);
    > else
    > /* The device is in normal mode */
    > priv->write_reg(priv, XCAN_MSR_OFFSET, 0);

    What about using two temp variables? Like this (untested, though):

    u32 reg_msg;
    u32 reg_sr_mask;

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

    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, "time out waiting for correct mode\n")
    return -ETIMEDOUT;
    }
    usleep_range(500, 10000);
    }

    >
    > priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
    > timeout = jiffies + XCAN_TIMEOUT;
    > if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
    > while ((priv->read_reg(priv, XCAN_SR_OFFSET)
    > & XCAN_SR_LBACK_MASK) == 0) {
    > if (time_after(jiffies, timeout)) {
    > netdev_warn(ndev,
    > "timedout for loopback mode\n");
    > return -ETIMEDOUT;
    > }
    > usleep_range(500, 10000);
    > }
    > } else {
    > while ((priv->read_reg(priv, XCAN_SR_OFFSET)
    > & XCAN_SR_NORMAL_MASK) == 0) {
    > if (time_after(jiffies, timeout)) {
    > netdev_warn(ndev,
    > "timedout for normal mode\n");
    > return -ETIMEDOUT;
    > }
    > usleep_range(500, 10000);
    > }
    > }
    > netdev_dbg(ndev, "status:#x%08x\n",
    > priv->read_reg(priv, XCAN_SR_OFFSET));
    >
    > priv->can.state = CAN_STATE_ERROR_ACTIVE;
    > return 0;
    > }

    Marc

    --
    Pengutronix e.K. | Marc Kleine-Budde |
    Industrial Linux Solutions | Phone: +49-231-2826-924 |
    Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
    Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2014-03-11 16:02    [W:0.040 / U:6.672 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site