| Message ID | 20200812143324.2394375-0-mholenko@antmicro.com |
|---|---|
| Headers | show |
| Series | LiteX SoC controller and LiteUART serial driver | expand |
On Mon, Sep 14, 2020 at 3:24 PM Stafford Horne <shorne@gmail.com> wrote: > > 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. You are totally right - this is not consistent. We should probably either trigger BUG() in each case or don't bother at all. > > > > > + > > > > + 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. I don't have a strong opinion about using BUG() here - I just thought it would be easier for the user. If this is, however, not how linux typically works, I'm ok with reworking this part. > -Stafford Best, Mateusz -- Mateusz Holenko Antmicro Ltd | www.antmicro.com Roosevelta 22, 60-829 Poznan, Poland
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