Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

From: Andreas Werner
Date: Mon Aug 08 2016 - 10:06:10 EST

Next message: Guodong Xu: "Re: WARNING: CPU: 0 PID: 1271 at drivers/mmc/core/core.c:991 mmc_release_host+0xa0/0xa8"
Previous message: Peter Zijlstra: "Re: [RFC PATCH 3/4] Make deadline max-heap faster avoiding real swaps on heapify ops."
In reply to: Andreas Werner: "Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver"
Next in thread: Wolfgang Grandegger: "Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Aug 08, 2016 at 02:28:39PM +0200, Wolfgang Grandegger wrote:
> Hello,
>
> Am 08.08.2016 um 13:39 schrieb Andreas Werner:
> >On Mon, Aug 08, 2016 at 11:27:25AM +0200, Wolfgang Grandegger wrote:
> >>Hello Andreas,
> >>
> >>a first quick review....
> >>
> >>Am 26.07.2016 um 11:16 schrieb Andreas Werner:
> >>>This CAN Controller is found on MEN Chameleon FPGAs.
> >>>
> >>>The driver/device supports the CAN2.0 specification.
> >>>There are 255 RX and 255 Tx buffer within the IP. The
> >>>pointer for the buffer are handled by HW to make the
> >>>access from within the driver as simple as possible.
> >>>
> >>>The driver also supports parameters to configure the
> >>>buffer level interrupt for RX/TX as well as a RX timeout
> >>>interrupt.
> >>>
> >>>With this configuration options, the driver/device
> >>>provides flexibility for different types of usecases.
> >>>
> >>>Signed-off-by: Andreas Werner <andreas.werner@xxxxxx>
> >>>---
> >>>drivers/net/can/Kconfig | 10 +
> >>>drivers/net/can/Makefile | 1 +
> >>>drivers/net/can/men_z192_can.c | 989 +++++++++++++++++++++++++++++++++++++++++
> >>>3 files changed, 1000 insertions(+)
> >>>create mode 100644 drivers/net/can/men_z192_can.c
>
> ---snip---
>
> >>>+/* Buffer level control values */
> >>>+#define MEN_Z192_MIN_BUF_LVL 0
> >>>+#define MEN_Z192_MAX_BUF_LVL 254
> >>>+#define MEN_Z192_RX_BUF_LVL_DEF 5
> >>>+#define MEN_Z192_TX_BUF_LVL_DEF 5
> >>>+#define MEN_Z192_RX_TOUT_MIN 0
> >>>+#define MEN_Z192_RX_TOUT_MAX 65535
> >>>+#define MEN_Z192_RX_TOUT_DEF 1000
> >>>+
> >>>+static int txlvl = MEN_Z192_TX_BUF_LVL_DEF;
> >>>+module_param(txlvl, int, S_IRUGO);
> >>>+MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default="
> >>>+ __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")");
> >>>+
> >>>+static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF;
> >>>+module_param(rxlvl, int, S_IRUGO);
> >>>+MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default="
> >>>+ __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")");
> >>>+
> >>
> >>What impact does the level have on the latency? Could you please add some
> >>comments.
> >
> >It has a impact on the latency.
> >rxlvl = 0 -> if one frame got received, a IRQ will be generated
> >rxlvl = 254 -> if 255 frames got received, a IRQ will be generated
>
> Well, what's your usecase for rxlvl > 0? For me it's not obvious what it can
> be good for. The application usually wants the message as soon as possible.
> Anyway, the default should be *0*. For RX and TX.
>

The HW provides such feature and the driver should be able to control it.
It was developed to control the IRQ load (like NAPI) by defining a level of the buffer
when the IRQ got asserted.

I aggree with you to set the default to "0" which is the main usecase.

> >>>+static int rx_timeout = MEN_Z192_RX_TOUT_DEF;
> >>>+module_param(rx_timeout, int, S_IRUGO);
> >>>+MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default="
> >>>+ __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")");
> >>
> >>Ditto. What is "rx_timeout" good for.
> >>
> >
> >The rx timeout is used im combination with the rxlvl to assert the
> >if the buffer level is not reached within this timeout.
>
> What event will the application receive in case of a timeout.
>

Its just to control the time when the RX IRQ will be asserted if the buffer
level is not reached.
Imagine if the rx_timeout is not existing and the rxlvl is set to 50 and
only 30 packets are received. The RX IRQ will be never asserted.

By defining the rx_timeout, we can control the time when the RX IRQ is asserted
if the buffer level is not reached.

The application does not receive any special signal, its just the RX IRQ.

> >Both, the timeout and the level are used to give the user as much
> >control over the latency and the IRQ handling as possible.
> >With this two options, the driver can be configured for different
> >use cases.
> >
> >I will add this as the comment to make it more clear.
>
> Even a bit more would be appreciated.
>

Sure...

>
> ---snip---
>
> >>>+static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr)
> >>>+{
> >>>+ struct net_device_stats *stats = &ndev->stats;
> >>>+ struct men_z192 *priv = netdev_priv(ndev);
> >>>+ struct men_z192_cf_buf __iomem *cf_buf;
> >>>+ struct can_frame *cf;
> >>>+ struct sk_buff *skb;
> >>>+ u32 cf_offset;
> >>>+ u32 length;
> >>>+ u32 data;
> >>>+ u32 id;
> >>>+
> >>>+ skb = alloc_can_skb(ndev, &cf);
> >>>+ if (unlikely(!skb)) {
> >>>+ stats->rx_dropped++;
> >>>+ return 0;
> >>>+ }
> >>>+
> >>>+ cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr;
> >>>+
> >>>+ cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset;
> >>>+ length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN;
> >>>+ id = readl(&cf_buf->can_id);
> >>>+
> >>>+ if (id & MEN_Z192_CFBUF_IDE) {
> >>>+ /* Extended frame */
> >>>+ cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3;
> >>>+ cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >>
> >>>+ MEN_Z192_CFBUF_ID2_SHIFT;
> >>>+
> >>>+ cf->can_id |= CAN_EFF_FLAG;
> >>>+
> >>>+ if (id & MEN_Z192_CFBUF_E_RTR)
> >>>+ cf->can_id |= CAN_RTR_FLAG;
> >>>+ } else {
> >>>+ /* Standard frame */
> >>>+ cf->can_id = (id & MEN_Z192_CFBUF_ID1) >>
> >>>+ MEN_Z192_CFBUF_ID1_SHIFT;
> >>>+
> >>>+ if (id & MEN_Z192_CFBUF_S_RTR)
> >>>+ cf->can_id |= CAN_RTR_FLAG;
> >>>+ }
> >>>+
> >>>+ cf->can_dlc = get_can_dlc(length);
> >>>+
> >>>+ /* remote transmission request frame
> >>>+ * contains no data field even if the
> >>>+ * data length is set to a value > 0
> >>>+ */
> >>>+ if (!(cf->can_id & CAN_RTR_FLAG)) {
> >>>+ if (cf->can_dlc > 0) {
> >>>+ data = readl(&cf_buf->data[0]);
> >>>+ *(__be32 *)cf->data = cpu_to_be32(data);
> >>
> >>Do you really need the extra copy?
> >>
> >>>+ }
> >>>+ if (cf->can_dlc > 4) {
> >>>+ data = readl(&cf_buf->data[1]);
> >>>+ *(__be32 *)(cf->data + 4) = cpu_to_be32(data);
> >>
> >>Ditto.
> >
> >No its not really needed. I thought its more clean and more readable than
> >putting this in one line withouth the copy.
>
> It should be fast in the first place.
>

Ok, will change that.

[...]

>
> >>>+static int men_z192_set_mode(struct net_device *ndev, enum can_mode mode)
> >>>+{
> >>>+ int ret;
> >>>+
> >>>+ switch (mode) {
> >>>+ case CAN_MODE_START:
> >>>+ ret = men_z192_start(ndev);
> >>>+ if (ret)
> >>>+ return ret;
> >>
> >>"if (ret)" means always an error. Therefore s/ret/err/ is clearer. Here and
> >>in many other places.
> >>
> >
> >Yes and no. I think its a general question about the naming of those variables.
> >I will check all the variables in the driver if it really makes sense
> >to rename it.
> >
> >For my opinion, "ret" is more generic. But you are right, "err" would be more
> >readable in some places.
>
> if (err)
>
> makes immediately clear that it's an error case. ret is more general, e.g.
> for the return value of read/write:
>
> if (ret < 0)
> error-case
> else if (ret == 0)
> end-of-file
> else
> btyes-read
>
> Just my personal preference to make the code more readable.

Ok, I will think about it.

>
> >>>+
> >>>+ netif_wake_queue(ndev);
> >>>+ break;
> >>>+ default:
> >>>+ return -EOPNOTSUPP;
> >>>+ }
> >>>+
> >>>+ return 0;
> >>>+static int men_z192_probe(struct mcb_device *mdev,
> >>>+ const struct mcb_device_id *id)
> >>>+{
> >>>+ struct device *dev = &mdev->dev;
> >>>+ struct men_z192 *priv;
> >>>+ struct net_device *ndev;
> >>>+ void __iomem *dev_base;
> >>>+ struct resource *mem;
> >>>+ u32 timebase;
> >>>+ int ret = 0;
> >>>+ int irq;
> >>>+
> >>>+ mem = mcb_request_mem(mdev, dev_name(dev));
> >>>+ if (IS_ERR(mem)) {
> >>>+ dev_err(dev, "failed to request device memory");
> >>>+ return PTR_ERR(mem);
> >>>+ }
> >>>+
> >>>+ dev_base = ioremap(mem->start, resource_size(mem));
> >>>+ if (!dev_base) {
> >>>+ dev_err(dev, "failed to ioremap device memory");
> >>>+ ret = -ENXIO;
> >>>+ goto out_release;
> >>>+ }
> >>>+
> >>>+ irq = mcb_get_irq(mdev);
> >>>+ if (irq <= 0) {
> >>>+ ret = -ENODEV;
> >>>+ goto out_unmap;
> >>>+ }
> >>>+
> >>>+ ndev = alloc_candev(sizeof(struct men_z192), 1);
> >>
> >>You specify here one echo_skb but it's not used anywhere. Local loopback
> >>seems not to be implemented.
> >>
> >
> >Agree with you, will set it to "0".
>
> No, the local loopback is mandetory!
>

Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c
it is not mandatory. In the Documentation/networking/can.txt
there is also a "should" and a fallback mechnism if the driver
does not support the local loopback.
I'm currently ok with this fallback mechanism.

Anyway I am not sure that the driver can handle the echo skb correctly.
If i understand it correctly, the can_get_echo_skb() is normally called
on a "TX done IRQ" to get the skb and loop it back.
I do not have such a "TX done IRQ" and have not implemented implemented
and added the local looback.

May be I can put and get the echo skb within the xmit function?
Does this make sense?

Regards
Andy

> Wolfgang.

Next message: Guodong Xu: "Re: WARNING: CPU: 0 PID: 1271 at drivers/mmc/core/core.c:991 mmc_release_host+0xa0/0xa8"
Previous message: Peter Zijlstra: "Re: [RFC PATCH 3/4] Make deadline max-heap faster avoiding real swaps on heapify ops."
In reply to: Andreas Werner: "Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver"
Next in thread: Wolfgang Grandegger: "Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]