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

From: Andreas Werner
Date: Tue Aug 09 2016 - 02:10:49 EST

Next message: Andreas Werner: "Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver"
Previous message: Tomasz Nowicki: "Re: [RFC PATCH V5 3/5] PCI: Check platform specific ECAM quirks"
In reply to: Wolfgang Grandegger: "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 04:35:34PM +0200, Wolfgang Grandegger wrote:
> Am 08.08.2016 um 16:05 schrieb Andreas Werner:
> >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.
>
> Now I got it. After timeout an interrupt will be trigger regardless of the
> thresholds. The default settings should result in minimum latencies.
>

yes :-)
I will set the rx_timeout to 0 to get the minimum latency.

> >>>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.
>
> Well, s/driver/hardware/ ! Local loopback is the preferred mechanism.
>

Sure...

> >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.
>
> What does "MEN_Z192_TFLG_TXIF" signal?
>

It is not a "TX Done" IRQ, it is the tx buffer level IRQ.
The IRQ is triggered when the number of available tx buffer entries is as
configured with txlvl. (after the buffer was full)

Example:
txlvl = 0
tx buffer has 255 entries.

-> The IRQ is triggered as soon as 1 frame got transmitted (254 entries).

---

txlvl = 254
tx buffer has 255 entries.

-> The IRQ is triggered as soon as the buffer has one entry and it got transmitted

> >May be I can put and get the echo skb within the xmit function?
> >Does this make sense?
>
> It only makes sense if the driver knows when one or more transfers are done.
>

Then i do not think that I can use the txlvl IRQ in this case and need to use
the fallback mechanism.

> Wolfgang.

Next message: Andreas Werner: "Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver"
Previous message: Tomasz Nowicki: "Re: [RFC PATCH V5 3/5] PCI: Check platform specific ECAM quirks"
In reply to: Wolfgang Grandegger: "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 ]