Message ID | cover.1603354744.git.pisa@cmp.felk.cvut.cz |
---|---|
Headers | show |
Series | CTU CAN FD open-source IP core SocketCAN driver, PCI, platform integration and documentation | expand |
On Thu 2020-10-22 10:36:17, Pavel Pisa wrote: > The device-tree bindings for open-source/open-hardware CAN FD IP core > designed at the Czech Technical University in Prague. > > CTU CAN FD IP core and other CTU CAN bus related projects > listing and documentation page > > http://canbus.pages.fel.cvut.cz/ > > Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz> > Reviewed-by: Rob Herring <robh@kernel.org> 1,2: Acked-by: Pavel Machek <pavel@ucw.cz>
On Thu 2020-10-22 10:36:21, Pavel Pisa wrote: > CTU CAN FD IP core documentation based on Martin Jeřábek's diploma theses > Open-source and Open-hardware CAN FD Protocol Support > https://dspace.cvut.cz/handle/10467/80366 > . > --- > .../ctu/FSM_TXT_Buffer_user.png | Bin 0 -> 174807 bytes Maybe picture should stay on website, somewhere. It is rather big for kernel sources. > +About SocketCAN > +--------------- > + > +SocketCAN is a standard common interface for CAN devices in the Linux > +kernel. As the name suggests, the bus is accessed via sockets, similarly > +to common network devices. The reasoning behind this is in depth > +described in `Linux SocketCAN <https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/can.rst>`_. > +In short, it offers a > +natural way to implement and work with higher layer protocols over CAN, > +in the same way as, e.g., UDP/IP over Ethernet. Drop? Or at least link directly to the file in kernel tree? > +Device probe > +~~~~~~~~~~~~ > + > +Before going into detail about the structure of a CAN bus device driver, > +let's reiterate how the kernel gets to know about the device at all. > +Some buses, like PCI or PCIe, support device enumeration. That is, when > +the system boots, it discovers all the devices on the bus and reads > +their configuration. The kernel identifies the device via its vendor ID > +and device ID, and if there is a driver registered for this identifier > +combination, its probe method is invoked to populate the driver's > +instance for the given hardware. A similar situation goes with USB, only > +it allows for device hot-plug. > + > +The situation is different for peripherals which are directly embedded > +in the SoC and connected to an internal system bus (AXI, APB, Avalon, > +and others). These buses do not support enumeration, and thus the kernel > +has to learn about the devices from elsewhere. This is exactly what the > +Device Tree was made for. Dunno. Is it suitable? This is supposed to be ctu-can documentation, not "how hardware works" docs. > +Platform device driver > +^^^^^^^^^^^^^^^^^^^^^^ > + > +In the case of Zynq, the core is connected via the AXI system bus, which > +does not have enumeration support, and the device must be specified in > +Device Tree. This kind of devices is called *platform device* in the > +kernel and is handled by a *platform device driver*\ [1]_. > + > +A platform device driver provides the following things: > + > +- A *probe* function > + > +- A *remove* function > + > +- A table of *compatible* devices that the driver can handle > + > +The *probe* function is called exactly once when the device appears (or > +the driver is loaded, whichever happens later). If there are more > +devices handled by the same driver, the *probe* function is called for > +each one of them. Its role is to allocate and initialize resources > +required for handling the device, as well as set up low-level functions > +for the platform-independent layer, e.g., *read_reg* and *write_reg*. > +After that, the driver registers the device to a higher layer, in our > +case as a *network device*. > + > +The *remove* function is called when the device disappears, or the > +driver is about to be unloaded. It serves to free the resources > +allocated in *probe* and to unregister the device from higher layers. > + > +Finally, the table of *compatible* devices states which devices the > +driver can handle. The Device Tree entry ``compatible`` is matched > +against the tables of all *platform drivers*. And this is "how to write a kernel driver" documentation. Like, why not, but maybe it does not need to be in kernel tree, and certainly should be separate from real "what is ctucan and how to use it" docs. Best regards, Pavel
Hi! > +++ b/drivers/net/can/ctucanfd/Kconfig > @@ -21,4 +21,15 @@ config CAN_CTUCANFD_PCI > PCIe board with PiKRON.com designed transceiver riser shield is available > at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd . > > +config CAN_CTUCANFD_PLATFORM > + tristate "CTU CAN-FD IP core platform (FPGA, SoC) driver" > + depends on OF || COMPILE_TEST > + help This is likely wrong, as it can enable config of CAN_CTUCANFD=M, CAN_CTUCANFD_PLATFORM=y, right? > @@ -8,3 +8,6 @@ ctucanfd-y := ctu_can_fd.o ctu_can_fd_hw.o > > obj-$(CONFIG_CAN_CTUCANFD_PCI) += ctucanfd_pci.o > ctucanfd_pci-y := ctu_can_fd_pci.o > + > +obj-$(CONFIG_CAN_CTUCANFD_PLATFORM) += ctucanfd_platform.o > +ctucanfd_platform-y += ctu_can_fd_platform.o Can you simply add right object files directly? Best regards, Pavel
Hello Pavel, thanks for review. As for the documentation, my current intention is to keep/maintain the common driver documentation for CTU CAN FD site and kernel source. The standalone driver documentation http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/driver_doc/ctucanfd-driver.html when the driver documentation moves to https://www.kernel.org/doc/html/latest/ we may consider to drop standalone. But resource are limited and keeping and maintaining sync between slightly different files is error prone. I run manual kdiff3 updates of both RST files because references to sources are different. On Thursday 22 of October 2020 13:25:40 Pavel Machek wrote: > On Thu 2020-10-22 10:36:21, Pavel Pisa wrote: > > CTU CAN FD IP core documentation based on Martin Jeřábek's diploma theses > > Open-source and Open-hardware CAN FD Protocol Support > > https://dspace.cvut.cz/handle/10467/80366 > > . > > > > --- > > .../ctu/FSM_TXT_Buffer_user.png | Bin 0 -> 174807 bytes > > Maybe picture should stay on website, somewhere. It is rather big for > kernel sources. My sense is that it is proffered that documentation is self-contained without embedded references to "untrusted" third party sites. But we try to do something with it. I try reduce size or switch to SVG, our actual source is PDF prepared by Ondrej Ille as part of CTU CAN FD IP core architecture. I probably redraw image in inscape with little worse graphics style, fonts, smoothness etc. but in smaller and simpler SVG file size format. I expect that use of original PDF in vector form would not help much. > > +About SocketCAN > > +--------------- > > + > > +SocketCAN is a standard common interface for CAN devices in the Linux > > +kernel. As the name suggests, the bus is accessed via sockets, similarly > > +to common network devices. The reasoning behind this is in depth > > +described in `Linux SocketCAN > > <https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Doc > >umentation/networking/can.rst>`_. +In short, it offers a > > +natural way to implement and work with higher layer protocols over CAN, > > +in the same way as, e.g., UDP/IP over Ethernet. > > Drop? Or at least link directly to the file in kernel tree? Yes, this is another place where we need other diversion between standalone and kernel. I try to learn what is the right way to cross-reference Linux kernel manuals writtent n RST? If you can speedup it by hint/right done example I would be happy. > > +Device probe > > +~~~~~~~~~~~~ > > + > > +Before going into detail about the structure of a CAN bus device driver, > > +let's reiterate how the kernel gets to know about the device at all. > > +Some buses, like PCI or PCIe, support device enumeration. That is, when > > +the system boots, it discovers all the devices on the bus and reads > > +their configuration. The kernel identifies the device via its vendor ID > > +and device ID, and if there is a driver registered for this identifier > > +combination, its probe method is invoked to populate the driver's > > +instance for the given hardware. A similar situation goes with USB, only > > +it allows for device hot-plug. > > + > > +The situation is different for peripherals which are directly embedded > > +in the SoC and connected to an internal system bus (AXI, APB, Avalon, > > +and others). These buses do not support enumeration, and thus the kernel > > +has to learn about the devices from elsewhere. This is exactly what the > > +Device Tree was made for. > > Dunno. Is it suitable? This is supposed to be ctu-can documentation, > not "how hardware works" docs. I think that this text is fully valid for standalone driver documentation, I understand that in the kernel tree this can be replaced by references to right places if we locate them. > > +Platform device driver > > +^^^^^^^^^^^^^^^^^^^^^^ > > + > > +In the case of Zynq, the core is connected via the AXI system bus, which > > +does not have enumeration support, and the device must be specified in > > +Device Tree. This kind of devices is called *platform device* in the > > +kernel and is handled by a *platform device driver*\ [1]_. > > + > > +A platform device driver provides the following things: > > + > > +- A *probe* function > > + > > +- A *remove* function > > + > > +- A table of *compatible* devices that the driver can handle > > + > > +The *probe* function is called exactly once when the device appears (or > > +the driver is loaded, whichever happens later). If there are more > > +devices handled by the same driver, the *probe* function is called for > > +each one of them. Its role is to allocate and initialize resources > > +required for handling the device, as well as set up low-level functions > > +for the platform-independent layer, e.g., *read_reg* and *write_reg*. > > +After that, the driver registers the device to a higher layer, in our > > +case as a *network device*. > > + > > +The *remove* function is called when the device disappears, or the > > +driver is about to be unloaded. It serves to free the resources > > +allocated in *probe* and to unregister the device from higher layers. > > + > > +Finally, the table of *compatible* devices states which devices the > > +driver can handle. The Device Tree entry ``compatible`` is matched > > +against the tables of all *platform drivers*. > > And this is "how to write a kernel driver" documentation. Like, why > not, but maybe it does not need to be in kernel tree, and certainly > should be separate from real "what is ctucan and how to use it" docs. I agree, we try to find way how to separate the things and reference pieces already found in the kernel. But it probably means massive diversion of documentation sources which is yet another additional load during preparation for mainlining. So I probably drop updates of standalone documentation. Which can be problem for another university group which builds applications based on the core and needs to maintain and patch kernels now. Best wishes, Pavel -- Pavel Pisa phone: +420 603531357 e-mail: pisa@cmp.felk.cvut.cz Department of Control Engineering FEE CVUT Karlovo namesti 13, 121 35, Prague 2 university: http://dce.fel.cvut.cz/ personal: http://cmp.felk.cvut.cz/~pisa projects: https://www.openhub.net/accounts/ppisa CAN related:http://canbus.pages.fel.cvut.cz/
Hello Pavel, thanks for review. On Thursday 22 of October 2020 13:43:06 Pavel Machek wrote: > Hi! > > > +++ b/drivers/net/can/ctucanfd/Kconfig > > @@ -21,4 +21,15 @@ config CAN_CTUCANFD_PCI > > PCIe board with PiKRON.com designed transceiver riser shield is > > available at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd . > > > > +config CAN_CTUCANFD_PLATFORM > > + tristate "CTU CAN-FD IP core platform (FPGA, SoC) driver" > > + depends on OF || COMPILE_TEST > > + help > > This is likely wrong, as it can enable config of CAN_CTUCANFD=M, > CAN_CTUCANFD_PLATFORM=y, right? My original code has not || COMPILE_TEST alternative. But I have been asked to add it On Sunday 16 of August 2020 01:28:13 Randy Dunlap wrote: > Can this be > depends on OF || COMPILE_TEST > ? I have send discussion later that I am not sure if it is right but followed suggestion. If there is no other reply now, I would drop || COMPILE_TEST. I believe that then it is correct for regular use. I ma not sure about all consequences of COMPILE_TEST missing. > > @@ -8,3 +8,6 @@ ctucanfd-y := ctu_can_fd.o ctu_can_fd_hw.o > > > > obj-$(CONFIG_CAN_CTUCANFD_PCI) += ctucanfd_pci.o > > ctucanfd_pci-y := ctu_can_fd_pci.o > > + > > +obj-$(CONFIG_CAN_CTUCANFD_PLATFORM) += ctucanfd_platform.o > > +ctucanfd_platform-y += ctu_can_fd_platform.o > > Can you simply add right object files directly? This is more tough question. We have kept sources as ctu_can_fd.c, ctu_can_fd_hw.c etc. to produce final ctucanfd.ko which matches device tree entry etc. after name simplification now... So we move from underscores to ctucanfd on more places. So yes, we can rename ctu_can_fd.c to ctucanfd_drv.c + others keep final ctucanfd.ko and change to single file based objects ctucanfd_platform.c and ctucanfd_pci.c If you think that it worth to be redone, I would do that. It would disrupt sources history, may it be blames, merging etc... but I would invest effort into it if asked for. Best wishes, Pavel
Hi! > > > +++ b/drivers/net/can/ctucanfd/Kconfig > > > @@ -21,4 +21,15 @@ config CAN_CTUCANFD_PCI > > > PCIe board with PiKRON.com designed transceiver riser shield is > > > available at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd . > > > > > > +config CAN_CTUCANFD_PLATFORM > > > + tristate "CTU CAN-FD IP core platform (FPGA, SoC) driver" > > > + depends on OF || COMPILE_TEST > > > + help > > > > This is likely wrong, as it can enable config of CAN_CTUCANFD=M, > > CAN_CTUCANFD_PLATFORM=y, right? > > My original code has not || COMPILE_TEST alternative. > > But I have been asked to add it > > On Sunday 16 of August 2020 01:28:13 Randy Dunlap wrote: > > Can this be > > depends on OF || COMPILE_TEST > > ? > > I have send discussion later that I am not sure if it is right > but followed suggestion. If there is no other reply now, > I would drop || COMPILE_TEST. I believe that then it is correct > for regular use. I ma not sure about all consequences of COMPILE_TEST > missing. COMPILE_TEST is not a problem. But you need to make this depend on main CONFIG_ option to disallow CAN_CTUCANFD=M, CAN_CTUCANFD_PLATFORM=y combination. > > > @@ -8,3 +8,6 @@ ctucanfd-y := ctu_can_fd.o ctu_can_fd_hw.o > > > > > > obj-$(CONFIG_CAN_CTUCANFD_PCI) += ctucanfd_pci.o > > > ctucanfd_pci-y := ctu_can_fd_pci.o > > > + > > > +obj-$(CONFIG_CAN_CTUCANFD_PLATFORM) += ctucanfd_platform.o > > > +ctucanfd_platform-y += ctu_can_fd_platform.o > > > > Can you simply add right object files directly? > > This is more tough question. We have kept sources > as ctu_can_fd.c, ctu_can_fd_hw.c etc. to produce > final ctucanfd.ko which matches device tree entry etc. > after name simplification now... > So we move from underscores to ctucanfd on more places. > So yes, we can rename ctu_can_fd.c to ctucanfd_drv.c + others > keep final ctucanfd.ko and change to single file based objects > ctucanfd_platform.c and ctucanfd_pci.c > > If you think that it worth to be redone, I would do that. > It would disrupt sources history, may it be blames, merging > etc... but I would invest effort into it if asked for. git can handle renames. Or you can use the new names for module names...? Best regards, Pavel
Hello Ondrej and others, On Monday 26 of October 2020 14:38:59 Ondrej Ille wrote: > Hello Pavel and Pavel, > > first of all, Pavel (Machek) thank you for review, we appreciate it. > We will try to fix as much mistakes as possible. Please, see my comments > below. > > With Regards > Ondrej > > On Thu, Oct 22, 2020 at 10:22 PM Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote: ... > > > > +/** > > > > + * ctucan_start_xmit - Starts the transmission > > > > + * @skb: sk_buff pointer that contains data to be Txed > > > > + * @ndev: Pointer to net_device structure > > > > + * > > > > + * This function is invoked from upper layers to initiate > > > > transmission. > > > > > > This + * function uses the next available free txbuf and populates > > > > their > > > > > > fields to + * start the transmission. > > > > + * > > > > + * Return: %NETDEV_TX_OK on success and failure value on error > > > > + */ > > > > > > Based on other documentation, I'd expect this to return -ESOMETHING on > > > error, but it returns NETDEV_TX_BUSY. > > > > I add information about explicit error/need for postpone type. > > Changing description, OK. Pavel Pisa, but why did you change handling of > insertion > failure to TXT Buffer to return NETDEV_TX_BUSY and increment tx_dropped? > Is there some preference on what should the driver return in case of HW > error? > Also, couldnt we afford not to check return value of ctucan_hw_insert_frame > ? Is purpose of > driver to be fail-safe against HW bug which says "There is TX buffer free > in Status register", but in reality, > no TXT Buffer is free? > > If we look at when ctu_can_hw_insert_frame returns false, it is when: > 1. We attempt to insert to non-existent TXT buffer -> Under drivers > control to do rotation correctly. > 2. If cfg->len > CAN_FD_MAX_LEN. Couldnt this check be removed? > CAN_FD_MAX_LEN is > defined for Linux, so it is not OS agnostic... Also, is it possible > that driver will call insert with > cf->len > CAN_FD_MAX_LEN? > 3. When there is HW bug (as mentioned earlier). There are assertions in > RTL checking this situation > will not happend! > So maybe we dont even need to check return value of this function at all? I try to follow other drivers. So if everything is OK then return NETDEV_TX_OK. If there is no Tx buffer available then return NETDEV_TX_BUSY. Some retransmit or drop should be handled by NET core in such case. This situation should not appear in reality, because Tx queue should be stopped if there is no free Tx buffer and should not be reenable earlien than at least one is available. So this situation is bug in driver logic or NET core. If the check for CAN FD frame format fails then it is right to drop SKB and it is handled with NETDEV_TX_OK return in other drivers as well. Only statistic counter increments. If the Tx buffer selected by driver s in incorrect state then it is even more serious bug so alternative is to stop whole driver and report fatal error. > > > > + /* Check for Bus Error interrupt */ > > > > + if (isr.s.bei) { > > > > + netdev_info(ndev, " bus error"); > > > > > > Missing "if (dologerr)" here? > > > > Intention was to left this one to appear without rate limit, it is really > > indication of bigger problem. But on the other hand without dologerr > > would be quite short/unclear, but does not overflow the log buffers... > > We would discuss what to do with my colleagues, suggestions welcomed. > > I agree with adding "dologerror" check here. It is true that arbitration > lost is not really an > error, and Bus error (error frame), therefore Bus error has higher > "severity". Could we > maybe do it that both have "dologerr" condition, but arbitration lost uses > "netdev_dbg"? Arbitration lost should not be reported nor generate interrupt for usual can application setup. > > > > +static int ctucan_rx_poll(struct napi_struct *napi, int quota) > > > > +{ > > > > + struct net_device *ndev = napi->dev; > > > > + struct ctucan_priv *priv = netdev_priv(ndev); > > > > + int work_done = 0; > > > > + union ctu_can_fd_status status; > > > > + u32 framecnt; > > > > + > > > > + framecnt = ctucan_hw_get_rx_frame_count(&priv->p); > > > > + netdev_dbg(ndev, "rx_poll: %d frames in RX FIFO", framecnt); > > > > > > This will be rather noisy, right? > > > > It has use to debug during development but may be it should be removed > > or controlled by other option. > > Maybe again suppress by "net_ratelimit" ? I have removed this one and report only errors. ... > > https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/-/blob/master/spec/CTU > >/ip/CAN_FD_IP_Core/2.1/CAN_FD_IP_Core.2.1.xml > > > > Which I consider as good option which should be preserved. > > I prefer to have only singe source of infomation > > which is kept with rest in automatic sync. > > We are really trying only to use bitfields generated in ctu_can_fd_regs. > Whether it is bitfield or > mask, is up to debate, but we always use generated values. Linux driver is > only one part of it > all. The golden source (IP-XACT) is propagated to RTL, 2 different TB > packages, constant definitions, > generated documentation. This is the only way how to keep register map > consistent with limited > developer resources we have. If we corrupt this rule, we end up with 4 > different representations > of register maps: > 1. What Testebench thinks > 2. What is in RTL > 3. What is in documentation > 4. What driver sees. > and then we will never put it back together again... I think that it is even important example for others. And there is not listed use of the generated header files for functional emulation in QEMU. I have even plan to use generator to prepare RO/RW mask for QEMU and IP block emulation skeleton for QEMU automatically. This part has not been implemented during last bachelor thesis. That part was written manually but it is in QEMU mainline now. If there is strong preferences for macros than bitfields we add macros generation alternative. > > > > + { > > > > + union ctu_can_fd_int_stat imask; > > > > + > > > > + imask.u32 = 0xffffffff; > > > > + ctucan_hw_int_ena_clr(&priv->p, imask); > > > > + ctucan_hw_int_mask_set(&priv->p, imask); > > > > + } > > > > > > More like this. Plus avoid block here...? > > > > Blocks is to document that imask is really local for these > > two lines, no need to look for it elsewhere in the function. > > But I can move declaration to start of the function. > > I would also remove blocks here. Flattened > > > > +/** > > > > + * ctucan_close - Driver close routine > > > > + * @ndev: Pointer to net_device structure > > > > + * > > > > + * Return: 0 always > > > > + */ > > > > > > You see, this is better. No need to say "Driver close routine" > > > twice. Now, make the rest consistent :-). > > > > > > > +EXPORT_SYMBOL(ctucan_suspend); > > > > +EXPORT_SYMBOL(ctucan_resume); > > > > > > _GPL? > > > > Should we be so strict??? Ondrej Ille can provide his opinion there. > > Is it really necessary? If yes, then we can change it. I see no reason that it is necessary. Without GPL some vendor can come with CTU CAN FD integration on such platform, where integration code stays unavailable to users. But common driver part has to be made available. We can prevent such use of the driver code, but I see no big wind there or financial income instrument. ... > > Hmmm, we can add special rules to tools to skip some special cases > > but actual files exactly math what is in documentation and VHDL > > sources and registers implementation. See page 61 / PDF 67 of > > > > http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/Progdokum.pdf > > > > Yes there is still space for improvements but we need to have > > acceptable base for already running applications. > > Point taken. These are indeed ridiculous. I was thinking about adding some > skip rule > to the register map generator, but I am out of IP-XACT fields to represent > this thing. > Maybe I can use vendor extensions do to hide it? Or custom switch? Anyway, > the > same thing needs to be resolved if HW design has dedicated test registers > which > are for debug. I will think about some solution. I personally prefer rules without exception. Defined 32-bit fields over whole register are harmless and if narrowed or reorganized later then in can help. .... > > OK, I have invested lot of time after Marin Jerabek's submission of > > diploma theses to make code really documented etc.. I add there something > > even that it is really simple use of can_len2dlc. May it be, we can use > > that directly. It is Linux specific, but clean. > > Using can_len2dlc seems as right option for me... Done > > > > +// TODO: AL_CAPTURE and ERROR_CAPTURE > > > > Again colleagues remark for future work. For me, it is important > > basic function under GNU/Linux. > > Again, CAN be removed and moved to Issue tracker on Gitlab. Please add issue, I have removed this one already. > It is basically the same topic as above. We need to generate everything > from single > source, otherwise we are not able to keep all the targets (RTL, TB, driver, > docs) > consistent. Best wishes, Pavel