| Message ID | 20200812143324.2394375-0-mholenko@antmicro.com |
|---|---|
| Headers | show |
| Series | LiteX SoC controller and LiteUART serial driver | expand |
On Wed, Aug 12, 2020 at 02:33:46PM +0200, Mateusz Holenko wrote: > This patchset introduces support for LiteX SoC Controller > and LiteUART - serial device from LiteX SoC builder > (https://github.com/enjoy-digital/litex). > > In the following patchset I will add > a new mor1kx-based (OpenRISC) platform that > uses this device. > > Later I plan to extend this platform by > adding support for more devices from LiteX suite. Hello, as discussed offline I am planning to merge these via the OpenRISC tree during 5.10. If anyone has an issues let me know. The patches all have their reviews and look fine to me other than a few nit's on the soc controller patch. -Stafford
On Mon, Sep 14, 2020 at 12:33:11PM +0200, Mateusz Holenko wrote: > On Fri, Sep 11, 2020 at 2:57 AM Stafford Horne <shorne@gmail.com> wrote: > > > > On Wed, Aug 12, 2020 at 02:34:34PM +0200, Mateusz Holenko wrote: > > > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com> > > > > > > This commit adds driver for the FPGA-based LiteX SoC > > > Controller from LiteX SoC builder. > > > > > > Co-developed-by: Mateusz Holenko <mholenko@antmicro.com> > > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com> > > > Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com> > > > --- > > > + node = dev->of_node; > > > + if (!node) > > > + return -ENODEV; We return here without BUG() if the setup fails. > > > + > > > + soc_ctrl_dev = devm_kzalloc(dev, sizeof(*soc_ctrl_dev), GFP_KERNEL); > > > + if (!soc_ctrl_dev) > > > + return -ENOMEM; We return here without BUG() if we are out of memory. > > > + > > > + soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0); > > > + if (IS_ERR(soc_ctrl_dev->base)) > > > + return PTR_ERR(soc_ctrl_dev->base); Etc. > > > + > > > + result = litex_check_csr_access(soc_ctrl_dev->base); > > > + if (result) { > > > + // LiteX CSRs access is broken which means that > > > + // none of LiteX drivers will most probably > > > + // operate correctly > > The comment format here with // is not usually used in the kernel, but its not > > forbidded. Could you use the /* */ multiline style? > > Sure, I'll change the commenting style here. > > > > > > + BUG(); > > Instead of stopping the system with BUG, could we just do: > > > > return litex_check_csr_access(soc_ctrl_dev->base); > > > > We already have failure for NODEV/NOMEM so might as well not call BUG() here > > too. > > It's true that litex_check_csr_accessors() already generates error > codes that could be > returned directly. > The point of using BUG() macro here, however, is to stop booting the > system so that it's visible > (and impossible to miss for the user) that an unresolvable HW issue > was encountered. > > CSR-accessors - the litex_{g,s}et_reg() functions - are intended to be > used by other LiteX drivers > and it's very unlikely that those drivers would work properly after > the fail of litex_check_csr_accessors(). > Since in such case the UART driver will be affected too (no boot logs > and error messages visible to the user), > I thought it'll be easier to spot and debug the problem if the system > stopped in the BUG loop. > Perhaps there are other, more linux-friendly, ways of achieving a > similar goal - I'm open for suggestions. I see your point, but I thought if failed with an exit status above, we could do the same here. But I guess failing here means that something is really wrong as validation failed. Some points: - If we return here, the system will still boot but there will be no UART - If we bail with BUG(), here the system stops, and there is no UART - Both cases the user can connect with a debugger and read "dmesg", to see what is wrong, but BUG() does not print an error message on all architectures. We could also use: - WARN(1, "Failed to validate CSR registers, the system is probably broken."); If you want to keep BUG() it may be fine. I am not an expert on handling these type of bailout's so other input is appreciated. -Stafford
This patchset introduces support for LiteX SoC Controller and LiteUART - serial device from LiteX SoC builder (https://github.com/enjoy-digital/litex). In the following patchset I will add a new mor1kx-based (OpenRISC) platform that uses this device. Later I plan to extend this platform by adding support for more devices from LiteX suite. Changes in v10: - added casting to avoid sparse warnings in the SoC Controller's driver Changes in v9: - fixed the `reg` node notation in the DT example - added exporting of the `litex_set_reg`/`litex_get_reg` symbols Changes in v8: - fixed help messages in LiteUART's KConfig - removed dependency between LiteUART and LiteX SoC drivers - removed `litex_check_accessors()` helper function - added crashing (BUG) on the failed LiteX CSR access test Changes in v7: - added missing include directive in UART's driver Changes in v6: - changed accessors in SoC Controller's driver - reworked UART driver Changes in v5: - added Reviewed-by tag - removed custom accessors from SoC Controller's driver - fixed error checking in SoC Controller's driver Changes in v4: - fixed copyright headers - fixed SoC Controller's yaml - simplified SoC Controller's driver Changes in v3: - added Acked-by and Reviewed-by tags - introduced LiteX SoC Controller driver - removed endianness detection (handled now by LiteX SoC Controller driver) - modified litex.h header - DTS aliases for LiteUART made optional - renamed SERIAL_LITEUART_NR_PORTS to SERIAL_LITEUART_MAX_PORTS - changed PORT_LITEUART from 122 to 123 Changes in v2: - binding description rewritten to a yaml schema file - added litex.h header with common register access functions Filip Kokosinski (3): dt-bindings: vendor: add vendor prefix for LiteX dt-bindings: serial: document LiteUART bindings drivers/tty/serial: add LiteUART driver Pawel Czarnecki (2): dt-bindings: soc: document LiteX SoC Controller bindings drivers/soc/litex: add LiteX SoC Controller driver .../bindings/serial/litex,liteuart.yaml | 38 ++ .../soc/litex/litex,soc-controller.yaml | 39 ++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 9 + drivers/soc/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/litex/Kconfig | 15 + drivers/soc/litex/Makefile | 3 + drivers/soc/litex/litex_soc_ctrl.c | 194 +++++++++ drivers/tty/serial/Kconfig | 32 ++ drivers/tty/serial/Makefile | 1 + drivers/tty/serial/liteuart.c | 402 ++++++++++++++++++ include/linux/litex.h | 24 ++ 13 files changed, 761 insertions(+) create mode 100644 Documentation/devicetree/bindings/serial/litex,liteuart.yaml create mode 100644 Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml create mode 100644 drivers/soc/litex/Kconfig create mode 100644 drivers/soc/litex/Makefile create mode 100644 drivers/soc/litex/litex_soc_ctrl.c create mode 100644 drivers/tty/serial/liteuart.c create mode 100644 include/linux/litex.h