Messages in this thread | | | Subject | Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver | From | Wolfgang Grandegger <> | Date | Mon, 8 Aug 2016 16:35:34 +0200 |
| |
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@men.de> >>>>> --- >>>>> 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.
>>> 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.
> 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?
> 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.
Wolfgang.
| |