Re: [PATCH v10 3/5] drivers/soc/litex: add LiteX SoC Controller driver

[Mateusz Holenko <mholenko@antmicro.com>]

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>
> > ---
> ...
> > +static int litex_check_csr_access(void __iomem *reg_addr)
> > +{
> > +     unsigned long reg;
> > +
> > +     reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
> > +
> > +     if (reg != SCRATCH_REG_VALUE) {
> > +             panic("Scratch register read error! Expected: 0x%x but got: 0x%lx",
> > +                     SCRATCH_REG_VALUE, reg);
> > +             return -EINVAL;
> > +     }
> > +
> > +     litex_set_reg(reg_addr + SCRATCH_REG_OFF,
> > +             SCRATCH_REG_SIZE, SCRATCH_TEST_VALUE);
> > +     reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
> > +
> > +     if (reg != SCRATCH_TEST_VALUE) {
> > +             panic("Scratch register write error! Expected: 0x%x but got: 0x%lx",
> > +                     SCRATCH_TEST_VALUE, reg);
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* restore original value of the SCRATCH register */
> > +     litex_set_reg(reg_addr + SCRATCH_REG_OFF,
> > +             SCRATCH_REG_SIZE, SCRATCH_REG_VALUE);
> > +
> > +     /* Set flag for other drivers */
> What does this comment mean?

This is a leftover from the previous version of the patch
and shouldn't be there - sorry for that.
I'll remove it.

> > +     pr_info("LiteX SoC Controller driver initialized");
> > +
> > +     return 0;
> > +}
> > +
> > +struct litex_soc_ctrl_device {
> > +     void __iomem *base;
> > +};
> > +
> > +static const struct of_device_id litex_soc_ctrl_of_match[] = {
> > +     {.compatible = "litex,soc-controller"},
> > +     {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, litex_soc_ctrl_of_match);
> > +
> > +static int litex_soc_ctrl_probe(struct platform_device *pdev)
> > +{
> > +     int result;
> > +     struct device *dev;
> > +     struct device_node *node;
> > +     struct litex_soc_ctrl_device *soc_ctrl_dev;
> > +
> > +     dev = &pdev->dev;
> > +     node = dev->of_node;
> > +     if (!node)
> > +             return -ENODEV;
> > +
> > +     soc_ctrl_dev = devm_kzalloc(dev, sizeof(*soc_ctrl_dev), GFP_KERNEL);
> > +     if (!soc_ctrl_dev)
> > +             return -ENOMEM;
> > +
> > +     soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(soc_ctrl_dev->base))
> > +             return PTR_ERR(soc_ctrl_dev->base);
> > +
> > +     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.

> > +     }
> > +
> > +     return 0;
> > +}
> > +
>
> Other than that it looks ok to me.
>
> -Stafford

Thanks for the review!

Best,
Mateusz


--
Mateusz Holenko
Antmicro Ltd | www.antmicro.com
Roosevelta 22, 60-829 Poznan, Poland