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

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

[Masayuki Ohtake]

Sorry, for late response.
----- Original Message ----- 
From: "Wang, Qi" <qi.wang@intel.com>
To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com>
Cc: "Khor, Andrew Chih Howe" <andrew.chih.howe.khor@intel.com>; <gregkh@suse.de>; <arjan@linux.intel.com>; "Wang, Yong
Y" <yong.y.wang@intel.com>; "Wolfgang Grandegger" <wg@grandegger.com>
Sent: Thursday, August 12, 2010 6:00 PM
Subject: RE: [MeeGo-Dev][PATCH] Topcliff: Update PCH_CAN driver to 2.6.35


> Hi Ohtak-san,
>
> Would you please answer the question from Wolfgang?
>
> Best Regards,
> Qi.
>
> > -----Original Message-----
> > From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> > Sent: Thursday, August 12, 2010 4:55 PM
> > To: Wang, Qi
> > Cc: Masayuki Ohtak; Khor, Andrew Chih Howe; gregkh@suse.de;
> > arjan@linux.intel.com; Wang, Yong Y
> > Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_CAN driver to 2.6.35
> >
> > On 08/12/2010 03:29 AM, Wang, Qi wrote:
> > >> -----Original Message-----
> > >> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> > >> Sent: Wednesday, August 11, 2010 8:31 PM
> > >> To: Masayuki Ohtak
> > >> Cc: meego-dev@meego.com; socketcan-core@lists.berlios.de;
> > >> netdev@vger.kernel.org; Khor, Andrew Chih Howe; gregkh@suse.de;
> > >> arjan@linux.intel.com; Wang, Qi; Wang, Yong Y
> > >> Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_CAN driver to 2.6.35
> > >>
> > >> Hello,
> > >>
> > >> On 08/11/2010 02:25 AM, Masayuki Ohtak wrote:
> > >>> CAN driver of Topcliff PCH
> > >>>
> > >>> Topcliff PCH is the platform controller hub that is going to be used in
> > >>> Intel's upcoming general embedded platform. All IO peripherals in
> > >>> Topcliff PCH are actually devices sitting on AMBA bus.
> > >>
> > >> That's interesting. Where can I/we find more information about this CAN
> > >> controller, e.g. data-sheets. It seems to have a few interesting
> > >> features (message scheduler, etc.).
> > >
> > > I remove all the maillist, and show you something about this platform.
> >
> > Like Greg, I also prefer the public discussion.
> >
> > >
> > > Topcliff PCH is connected with Tunnelcreak (A kind of ATOM-based processor,
> > which integrate Memory controller, GFX and RC). And Topcliff is a kind of
> > stand-alone ARM-based processor, so it has AMBA-based peripherals, such as
> > CAN, GBE, I2C.
> > >
> > > Topcliff connected with Tunnelcreak via PCIe x1 lane. And PCH_PHUB works
> > as a gateway, which transform PCIe transaction to AMBA transaction, and vice
> > versa.
> > >
> > > The datasheet of those two chips aren't open now.
> >
> > Thanks for the info.
> >
> > >>> Topcliff PCH has CAN I/F. This driver enables CAN function.
> > >>>
> > >>> Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com>
> > >>
> > >> Thanks for your contribution. Unfortunately, there are many issues,
> > >> especially the driver is not yet conform with the Socket-CAN driver API:
> > >>
> > >> - My first observation was:
> > >>
> > >>   $ wc -l pch_can.c
> > >>   4076 pch_can.c
> > >>   $ grep dev_dbg pch_can.c | wc -l
> > >>   143
> > >>
> > >>   That's a lot of code, mainly debugging code, I think. This needs to
> > >>   be cleaned up sooner than later. dev_dbg's should be restricted to a
> > >>   few useful for the real users.

We will reduce dev_dbg code.

> > >>
> > >> - 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 driver should handle state changes and communicate them to the
> > >>   user space via error messages, if possible.
> > >>
What's "state chage" mean ?

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


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

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

Thanks, Ohtake(OKISemi)

out-of-order tx objects - was Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_CAN driver to 2.6.35

[Oliver Hartkopp]

Hello Ohtake,

i'm only picking up the out-of-order issue here ...

On 01.09.2010 09:45, Masayuki Ohtake wrote:

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

The CAN frames that are sent by the local system have to touch the wire in the
same order as you put them into the socket as user.

An example:

You send CAN-frames with CAN Identifiers via can-raw sockets

0x123
0x456
0x010

and of course you expect these frames sent in exactly this order.
This is easy to archive as you get each of these CAN frames out of the
netdevices tx-queue in this order.

The problems comes up when you use multiple "tx objects" of your CAN
controller. Some CAN controllers have a "magic intelligence" that leads to
some re-ordering according to the CAN-Identifiers priority.

E.g.

0x123
0x456
0x010

leads to

0x123
0x010
0x456

because you pushed these CAN-frames into different tx objects and the CAN
controller sorts them on its own by priority. For CAN newbies: A lower CAN-ID
value has a higher priority in arbitration on the *physical* layer.

In the example above the CAN controller checks after sending the 0x123 CAN
frame if there's a priority order in the tx objects that are ready to send,
and selects 0x010 ...

This behaviour might be interesting in single-user/single-application
environments - but it is just wrong in multi-user systems where several
different applications use the (same) CAN bus. Standard networking behaviour.

Please ensure that the CAN frame order given by the tx-queue of the Linux
kernel netdevice infrastructure is send on the wire in the same order.

This usually leads to the use of ONLY ONE tx object of your CAN controller.
Probably you can use two tx objects to increase the throughput if this does
not have any impact to the frame order. See your controllers documentation for
details about this tx object behaviour.

I hope this makes the question of Wolfgang a bit clearer :-)

Regards,
Oliver

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

[Wolfgang Grandegger]

Hello,

On 09/01/2010 09:45 AM, Masayuki Ohtake wrote:
> Sorry, for late response.
> ----- Original Message ----- 
> From: "Wang, Qi" <qi.wang@intel.com>
> To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com>
> Cc: "Khor, Andrew Chih Howe" <andrew.chih.howe.khor@intel.com>; <gregkh@suse.de>; <arjan@linux.intel.com>; "Wang, Yong
> Y" <yong.y.wang@intel.com>; "Wolfgang Grandegger" <wg@grandegger.com>
> Sent: Thursday, August 12, 2010 6:00 PM
> 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

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

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

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

Wolfgang.

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

[Masayuki Ohtake]

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

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

[Wolfgang Grandegger]

On 09/02/2010 05:19 AM, Masayuki Ohtake wrote:
> ----- 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".

Please *remove* "pch_can_set_baud_custom" or "pch_can_set_baud_simple"
and use the fields of "const struct can_bittiming *bt" *directly*:

  /* Getting the appropriate register value. */
  reg_val = (((bt->brp & MSK_BITT_BRP) << BIT_BITT_BRP) |
	    ((bt->prop_seg + bt->phase_seg1 - 1) << BIT_BITT_TSEG1) |
            ...

> Could you indicate in more detail ?

Please have a closer look to the link mentioned above.

>>>>>>> - 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 code is your friend. Please have a more detailed look to the SJA1000
driver, e.g. search it for "state".

Wolfgang.