| Message ID | 4ceda3a9d68263b4e0dfe66521a46f40b2e502f7.1597518433.git.ppisa@pikron.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | CTU CAN FD open-source IP core SocketCAN driver, PCI, platform integration and documentation | expand |
On 8/15/20 12:43 PM, Pavel Pisa wrote: > diff --git a/drivers/net/can/ctucanfd/Kconfig b/drivers/net/can/ctucanfd/Kconfig > index e1636373628a..a8c9cc38f216 100644 > --- a/drivers/net/can/ctucanfd/Kconfig > +++ 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 Can this be depends on OF || COMPILE_TEST ? > + help > + The core has been tested together with OpenCores SJA1000 > + modified to be CAN FD frames tolerant on MicroZed Zynq based > + MZ_APO education kits designed by Petr Porazil from PiKRON.com > + company. FPGA design https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top. > + The kit description at the Computer Architectures course pages > + https://cw.fel.cvut.cz/b182/courses/b35apo/documentation/mz_apo/start . > + > endif
Hello Randy and Rob, thanks much for review, I have corrected FPGA spelling and binding YAML license. On Sunday 16 of August 2020 01:28:13 Randy Dunlap wrote: > On 8/15/20 12:43 PM, Pavel Pisa wrote: > > diff --git a/drivers/net/can/ctucanfd/Kconfig > > b/drivers/net/can/ctucanfd/Kconfig index e1636373628a..a8c9cc38f216 > > 100644 > > --- a/drivers/net/can/ctucanfd/Kconfig > > +++ 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 > > Can this be > depends on OF || COMPILE_TEST > ? I am not sure for this change. Is it ensured/documented somewhere that header files provide dummy definition such way, that OF drivers builds even if OF support is disabled? If I remember well, CTU CAN FD OF module build fails if attempted in the frame of native x86_64 build where OF has been disabled. Does COMPILE_TEST ensure that such build succeeds. As for the next steps, I expect that without any review of Marc Kleine-Budde or Wolfgang Grandegger from initial attempt for submission from February 2019, we are at the end of the road now. If there is confirmed preference, I would shorten license headers in the C files, but I am not sure if SPDX-License-Identifier is recognized by copyright law and because code and CTU CAN FD IP can be used outside of Linux kernel by others, we would like to keep legally binding preamble. It is reduced by not listing address to obtain complete GPL-2.0 from anyway. And change of preamble requires to update main repository, because header files are generated from IP core IPXACT definition by Python based tools. I am aware of only one other suggestion not followed yet and it is separation of part of ctucan_tx_interrupt() function into new one suggested by Pavel Machek. I agree that function length of 108 lines is big. When blank lines are removed we are on 68 lines and 28 lines are switch statement. The function consist of two nested loops. External one required to ensure no lost interrupt when edge triggered delivery or implementation is used. For me personally, it is more readable in the actual format then to separate and propagate local variables to another function. And particular function code received only formatting and ctu_can_fd_ -> ctucan_hw_ rename in past year so it is tested many/many times by manual PCI test and automated Zynq tests. Each of the following pipelines which contains two jobs ands by test of FPGA design and driver build and tests on real HW https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top/pipelines You can go through years of the testing and development back. So I have even tendency to not shuffle code which does not result in indisputable better readability and breaks more than year of unmodified code successful (pass) test result line and confidence. Because I understand that you all are loaded a lot I expect that after ACK/review-by by Rob, there is no need to send v6 to devicetree@vger.kernel.org I am not sure about cross-post to netdev@vger.kernel.org linux-kernel@vger.kernel.org when the progress is stuck on linux-can@vger.kernel.org Problem is that linux-can seems to eat core driver patch, probably because it is too long. Thanks to all for patience and if somebody does want to be loaded by minor updates, resends and pings to linux-can, send me note to not bother you again. Thanks for your time, Pavel PS: I would be available on Drew Fustini's LPC 2020 BoF: upstream drivers for open source FPGA SoC peripherals today. If there is interrest I can provide some information and show some overview and results.
On 8/25/20 2:25 AM, Pavel Pisa wrote: > Hello Randy and Rob, > > thanks much for review, I have corrected FPGA spelling > and binding YAML license. > > On Sunday 16 of August 2020 01:28:13 Randy Dunlap wrote: >> On 8/15/20 12:43 PM, Pavel Pisa wrote: >>> diff --git a/drivers/net/can/ctucanfd/Kconfig >>> b/drivers/net/can/ctucanfd/Kconfig index e1636373628a..a8c9cc38f216 >>> 100644 >>> --- a/drivers/net/can/ctucanfd/Kconfig >>> +++ 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 >> >> Can this be >> depends on OF || COMPILE_TEST >> ? > > I am not sure for this change. Is it ensured/documented somewhere that > header files provide dummy definition such way, that OF drivers builds > even if OF support is disabled? If I remember well, CTU CAN FD OF > module build fails if attempted in the frame of native x86_64 > build where OF has been disabled. Does COMPILE_TEST ensure that > such build succeeds. > COMPILE_TEST won't ensure anything. OTOH, <linux/of.h> has lots of stubs for handling the case of CONFIG_OF not being enabled.
diff --git a/drivers/net/can/ctucanfd/Kconfig b/drivers/net/can/ctucanfd/Kconfig index e1636373628a..a8c9cc38f216 100644 --- a/drivers/net/can/ctucanfd/Kconfig +++ 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 + help + The core has been tested together with OpenCores SJA1000 + modified to be CAN FD frames tolerant on MicroZed Zynq based + MZ_APO education kits designed by Petr Porazil from PiKRON.com + company. FPGA design https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top. + The kit description at the Computer Architectures course pages + https://cw.fel.cvut.cz/b182/courses/b35apo/documentation/mz_apo/start . + endif diff --git a/drivers/net/can/ctucanfd/Makefile b/drivers/net/can/ctucanfd/Makefile index eb945260952d..a77ca72d534e 100644 --- a/drivers/net/can/ctucanfd/Makefile +++ b/drivers/net/can/ctucanfd/Makefile @@ -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 diff --git a/drivers/net/can/ctucanfd/ctu_can_fd_platform.c b/drivers/net/can/ctucanfd/ctu_can_fd_platform.c new file mode 100644 index 000000000000..c35b16b8566b --- /dev/null +++ b/drivers/net/can/ctucanfd/ctu_can_fd_platform.c @@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/******************************************************************************* + * + * CTU CAN FD IP Core + * + * Copyright (C) 2015-2018 Ondrej Ille <ondrej.ille@gmail.com> FEE CTU + * Copyright (C) 2018-2020 Ondrej Ille <ondrej.ille@gmail.com> self-funded + * Copyright (C) 2018-2019 Martin Jerabek <martin.jerabek01@gmail.com> FEE CTU + * Copyright (C) 2018-2020 Pavel Pisa <pisa@cmp.felk.cvut.cz> FEE CTU/self-funded + * + * Project advisors: + * Jiri Novak <jnovak@fel.cvut.cz> + * Pavel Pisa <pisa@cmp.felk.cvut.cz> + * + * Department of Measurement (http://meas.fel.cvut.cz/) + * Faculty of Electrical Engineering (http://www.fel.cvut.cz) + * Czech Technical University (http://www.cvut.cz/) + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + ******************************************************************************/ + +#include <linux/module.h> +#include <linux/netdevice.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> + +#include "ctu_can_fd.h" + +#define DRV_NAME "ctucanfd" + +static void ctucan_platform_set_drvdata(struct device *dev, + struct net_device *ndev) +{ + struct platform_device *pdev = container_of(dev, struct platform_device, + dev); + + platform_set_drvdata(pdev, ndev); +} + +/** + * ctucan_platform_probe - Platform registration call + * @pdev: Handle to the platform device structure + * + * This function does all the memory allocation and registration for the CAN + * device. + * + * Return: 0 on success and failure value on error + */ +static int ctucan_platform_probe(struct platform_device *pdev) +{ + struct resource *res; /* IO mem resources */ + struct device *dev = &pdev->dev; + void __iomem *addr; + int ret; + unsigned int ntxbufs; + int irq; + + /* Get the virtual base address for the device */ + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + addr = devm_ioremap_resource(dev, res); + if (IS_ERR(addr)) { + dev_err(dev, "Cannot remap address.\n"); + ret = PTR_ERR(addr); + goto err; + } + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(dev, "Cannot find interrupt.\n"); + ret = irq; + goto err; + } + + /* Number of tx bufs might be change in HW for future. If so, + * it will be passed as property via device tree + */ + ntxbufs = 4; + ret = ctucan_probe_common(dev, addr, irq, ntxbufs, 0, + 1, ctucan_platform_set_drvdata); + + if (ret < 0) + platform_set_drvdata(pdev, NULL); + +err: + return ret; +} + +/** + * ctucan_platform_remove - Unregister the device after releasing the resources + * @pdev: Handle to the platform device structure + * + * This function frees all the resources allocated to the device. + * Return: 0 always + */ +static int ctucan_platform_remove(struct platform_device *pdev) +{ + struct net_device *ndev = platform_get_drvdata(pdev); + struct ctucan_priv *priv = netdev_priv(ndev); + + netdev_dbg(ndev, "ctucan_remove"); + + unregister_candev(ndev); + pm_runtime_disable(&pdev->dev); + netif_napi_del(&priv->napi); + free_candev(ndev); + + return 0; +} + +static SIMPLE_DEV_PM_OPS(ctucan_platform_pm_ops, ctucan_suspend, ctucan_resume); + +/* Match table for OF platform binding */ +static const struct of_device_id ctucan_of_match[] = { + { .compatible = "ctu,ctucanfd-2", }, + { .compatible = "ctu,ctucanfd", }, + { /* end of list */ }, +}; +MODULE_DEVICE_TABLE(of, ctucan_of_match); + +static struct platform_driver ctucanfd_driver = { + .probe = ctucan_platform_probe, + .remove = ctucan_platform_remove, + .driver = { + .name = DRV_NAME, + .pm = &ctucan_platform_pm_ops, + .of_match_table = ctucan_of_match, + }, +}; + +module_platform_driver(ctucanfd_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Martin Jerabek"); +MODULE_DESCRIPTION("CTU CAN FD for platform");