Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_CAN driver to 2.6.35

["Masayuki Ohtake" <masa-korg@dsn.okisemi.com>]

----- Original Message ----- 
From: "Wolfgang Grandegger" <wg@grandegger.com>
To: "Masayuki Ohtake" <masa-korg@dsn.okisemi.com>
Cc: "Andrew Chih Howe" <andrew.chih.howe.khor@intel.com>; "Qi" <qi.wang@intel.com>; "ML netdev"
<netdev@vger.kernel.org>; <gregkh@suse.de>; "ML linux-kernel" <linux-kernel@vger.kernel.org>; "Wang, Yong Y"
<yong.y.wang@intel.com>; <socketcan-core@lists.berlios.de>; <arjan@linux.intel.com>; "David S. Miller"
<davem@davemloft.net>; "Christian Pellegrin" <chripell@fsfe.org>; "Samuel Ortiz" <sameo@linux.intel.com>
Sent: Thursday, September 02, 2010 3:51 AM
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_CAN driver to 2.6.35

> ...
> >>>>> - The values for the hw-specific bit-timing registers should be derived
> >>>>>   from the calculated values in "priv->can.bittiming":
> >>>>>
> >>>>>   http://lxr.linux.no/#linux+v2.6.35/include/linux/can/netlink.h#L17
> >>>>>
> >
> > I show current pch_can code below.
> >
> > +static int pch_set_bittiming(struct net_device *ndev)
> > +{
> > + struct pch_can_priv *priv = netdev_priv(ndev);
> > + struct pch_can_os *dev_can_os = priv->pch_can_os_p;
> > + const struct can_bittiming *bt = &priv->can.bittiming;
> >
> > Is the above TRUE, isn't it ?
>
> The code fragment looks good. In that function you should then *derive*
> the values of the bit-timing registers from the data fields of "bt". For
> the SJA1000, you find the code here:
>
> http://lxr.linux.no/#linux+v2.6.35/drivers/net/can/sja1000/sja1000.c#L202
>

I can't understand the your saying.
I think our can driver is implemented like your saying.

In function "pch_set_bittiming", get the value of the bit-timing registers from
the data fields of "bt" at "pch_can_set_baud_custom" or "pch_can_set_baud_simple".

Could you indicate in more detail ?


> >>>>> - The driver should handle state changes and communicate them to the
> >>>>>   user space via error messages, if possible.
> >>>>>
> > What's "state chage" mean ?
>
> Googling for "can bis states" returned:
>
>
http://www.softing.com/home/en/industrial-automation/products/can-bus/more-can-bus/error-handling/error-states.php?navanchor=3010510
>
> The CAN controller usually triggers an interrupt when the state changes,
> which allows the driver to track the CAN state and deliver that
> information to the user space.

I could understand your saying.
In our current code, our driver can detect state change, but doesn't notify to
can-core module or kennel protocol stack.
We will modify our driver to notify to these.

>
> >>>>> - The driver should report errors to the user space via error messages.
> >>>>>
> > Is the above mean using alloc_can_err_skb and set error info and notify to kernel with netif_rx ?
>
> Yes. Please search "Documentation/networking/can.txt" for "error frames"
> for further information.

I understand.
We will modify.

>
> >>>>> - Bus errors seem not to be handled properly.I'm missing can_bus_off().
> >>>>>   Does the controller recover from bus-off automatically?
> > No.
> > CAN driver recovers from Bus-off state.
>
> You mean: "It does *not" recover automatically"! Right?

I meant like below.
CAN-HW itself can't recover from bus-off state automatically.
Cooperate with CAN driver, CAN HW can do automatically.

>
> >
> >>>>>
> >>>>> - I see that the driver uses many TX and RX objects. How do you avoid
> >>>>>   out-of-order transmission and reception?
> >>>> What do you mean out-of-order RX and TX?
> >>>> Atom processor only supports in-order execution, and PCIe-based peripherals
> >>> can solve it with consumer-producer model. Actually IC designer will take care
> >>> of out-of-order PCIe CPLD transaction.
> >>>
> >>> I mean out-of-order transmission to or from the CAN bus. This is handled
> >>> by the CAN controller hardware. It has nothing to to with the processor.
> > Cannot avoid occurring rx or tx our-of-order.
>
> It is a *requirement* as Oliver already pointed out. It's easy to
> achieve if just one TX object is used but it might be tricky with more
> than one.

I agree with your indication.
We will modify so that our CAN driver has only one tx/rx each object.

Thanks, Ohtake(OKISemi)