Re: [PATCH 2/2] mfd: Add Baikal-T1 Boot Controller driver

[Sergey Semin]

Hello Lee,

On Wed, Mar 25, 2020 at 10:09:40AM +0000, Lee Jones wrote:
> On Fri, 06 Mar 2020, Sergey.Semin@xxxxxxxxxxxxxxxxxxxx wrote:
> 
> > From: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > 
> > Baikal-T1 Boot Controller is an IP block embedded into the SoC and
> > responsible for the chip proper pre-initialization and further
> > booting up from some memory device. From the Linux kernel point of view
> > it's just a multi-functional device, which exports three physically mapped
> > ROMs and a single SPI controller.
> > 
> > Primarily the ROMs are intended to be used for transparent access of
> > the memory devices with system bootup code. First ROM device is an
> > embedded into the SoC firmware, which is supposed to be used just for
> > the chip debug and tests. Second ROM device provides a MMIO-based
> > access to an external SPI flash. Third ROM mirrors either the Internal
> > or SPI ROM region, depending on the state of the external BOOTCFG_{0,1}
> > chip pins selecting the system boot device.
> > 
> > External SPI flash can be also accessed by the DW APB SSI SPI controller
> > embedded into the Baikal-T1 Boot Controller. In this case the memory mapped
> > SPI flash region shouldn't be accessed.
> > 
> > Taking into account all the peculiarities described above, we created
> > an MFD-based driver for the Baikal-T1 controller. Aside from ordinary
> > OF-based sub-device registration it also provides a simple API to
> > serialize an access to the external SPI flash from either the MMIO-based
> > SPI interface or embedded SPI controller.
> 
> Not sure why this is being classified as an MFD.
> 
> This is clearly 'just' a memory device.
> 

Hm, I see this as a normal MFD device. The Boot controller provides a
set of physically mapped ROMs and a DW APB SSI-based embedded SPI
controller. Yes, the SPI controller is normally utilized to access an external
flash device, and at boot stage it is used for it. But still it's a SPI
controller which driver belongs to the kernel SPI subsystem. Moreover
nothing prevents a platform designer from using it together with custom
GPIO-based chip-selects to have additional devices on the SPI bus.

As I said the problem is that an SPI flash might be accessed either with
use of a physically mapped ROM or via the normal DW APB SPI controller.
These two interfaces can't be used simultaneously, because the ROM is
just an rtl state-machine, which is built to translate MMIO operations
through the SPI controller registers to an external SPI-nor at CS0 of
the interface. That's why first I need to make sure the interface is locked,
then being enabled, then the corresponding driver can use it, then get
to unlock. That's the point of having the __bt1_bc_spi_lock() and
bt1_bc_spi_unlock() methods exported from the driver.

I've got two drivers for MTD ROM and SPI controller based on that
methods. But I haven't submitted them yet, because they belong to two
different subsystems and I need to have this one being accepted first.

Recently I've sent an RFC regarding a different question, but it
concerns the Baikal-T1 system controller and the boot controller as being part
of it:

https://lkml.org/lkml/2020/3/22/393

Regards,
-Sergey

> > Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx>
> > Cc: Paul Burton <paulburton@xxxxxxxxxx>
> > Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> > ---
> >  drivers/mfd/Kconfig              |  13 ++
> >  drivers/mfd/Makefile             |   1 +
> >  drivers/mfd/bt1-boot-ctl.c       | 345 +++++++++++++++++++++++++++++++
> >  include/linux/mfd/bt1-boot-ctl.h |  46 +++++
> >  4 files changed, 405 insertions(+)
> >  create mode 100644 drivers/mfd/bt1-boot-ctl.c
> >  create mode 100644 include/linux/mfd/bt1-boot-ctl.h
> 
> [...]
> 
> > diff --git a/drivers/mfd/bt1-boot-ctl.c b/drivers/mfd/bt1-boot-ctl.c
> > new file mode 100644
> > index 000000000000..9e3cd47a2e7a
> > --- /dev/null
> > +++ b/drivers/mfd/bt1-boot-ctl.c
> > @@ -0,0 +1,345 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> > + *
> > + * Authors:
> > + *   Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > + *
> > + * Baikal-T1 Boot Controller driver.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +#include <linux/device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/clk.h>
> > +#include <linux/mutex.h>
> > +#include <linux/mfd/core.h>
> 
> Despite including the MFD API, I don't see it being used at all.
> 
> > +#include <linux/mfd/bt1-boot-ctl.h>
> 
> [...]
> 
> > +static inline u32 bc_read(void __iomem *reg)
> > +{
> > +	return readl(reg);
> > +}
> > +
> > +static inline void bc_write(void __iomem *reg, u32 data)
> > +{
> > +	writel(data, reg);
> > +}
> 
> Abstraction for the sake of abstraction is generally discouraged.
> 
> [...]
> 
> > +static int bc_register_devices(struct bt1_bc *bc)
> > +{
> > +	int ret;
> > +
> > +	ret = devm_of_platform_populate(bc->dev);
> > +	if (ret)
> > +		dev_err(bc->dev, "Failed to add sub-devices\n");
> > +
> > +	return ret;
> > +}
> 
> You can call devm_of_platform_populate() from anywhere.
> 
> Doesn't have to be an MFD.
> 
> [...]
> 
> -- 
> Lee Jones [æçæ]
> Linaro Services Technical Lead
> Linaro.org â Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog