Message-ID: <20200424145047.GE8414@dell>
Date:   Fri, 24 Apr 2020 15:50:47 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Christophe Kerello <christophe.kerello@...com>
Cc:     Marek Vasut <marex@...x.de>, miquel.raynal@...tlin.com,
        richard@....at, vigneshr@...com, robh+dt@...nel.org,
        mark.rutland@....com, tony@...mide.com,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        devicetree@...r.kernel.org,
        Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [PATCH v2 02/12] mfd: stm32-fmc2: add STM32 FMC2 controller
 driver

On Fri, 24 Apr 2020, Christophe Kerello wrote:

> 
> 
> On 4/24/20 1:06 PM, Marek Vasut wrote:
> > On 4/24/20 12:50 PM, Lee Jones wrote:
> > > On Fri, 24 Apr 2020, Marek Vasut wrote:
> > > 
> > > > On 4/24/20 9:45 AM, Lee Jones wrote:
> > > > > On Wed, 15 Apr 2020, Christophe Kerello wrote:
> > > > > 
> > > > > > The driver adds the support for the STMicroelectronics FMC2 controller
> > > > > > found on STM32MP SOCs.
> > > > > > 
> > > > > > The FMC2 functional block makes the interface with: synchronous and
> > > > > > asynchronous static memories (such as PSNOR, PSRAM or other
> > > > > > memory-mapped peripherals) and NAND flash memories.
> > > > > > 
> > > > > > Signed-off-by: Christophe Kerello <christophe.kerello@...com>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > >   - remove ops from stm32_fmc2 structure
> > > > > >   - add 2 APIs to manage FMC2 enable/disable
> > > > > >   - add 2 APIs to manage FMC2 NWAIT shared signal
> > > > > > 
> > > > > >   drivers/mfd/Kconfig            |  12 +++
> > > > > >   drivers/mfd/Makefile           |   1 +
> > > > > >   drivers/mfd/stm32-fmc2.c       | 136 +++++++++++++++++++++++++
> > > > > >   include/linux/mfd/stm32-fmc2.h | 225 +++++++++++++++++++++++++++++++++++++++++
> > > > > >   4 files changed, 374 insertions(+)
> > > > > >   create mode 100644 drivers/mfd/stm32-fmc2.c
> > > > > >   create mode 100644 include/linux/mfd/stm32-fmc2.h
> > > > > > 
> > > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > > > index 2b20329..5260582 100644
> > > > > > --- a/drivers/mfd/Kconfig
> > > > > > +++ b/drivers/mfd/Kconfig
> > > > > > @@ -1922,6 +1922,18 @@ config MFD_ROHM_BD71828
> > > > > >   	  Also included is a Coulomb counter, a real-time clock (RTC), and
> > > > > >   	  a 32.768 kHz clock gate.
> > > > > > +config MFD_STM32_FMC2
> > > > > > +	tristate "Support for FMC2 controllers on STM32MP SoCs"
> > > > > > +	depends on MACH_STM32MP157 || COMPILE_TEST
> > > > > > +	select MFD_CORE
> > > > > > +	select REGMAP
> > > > > > +	select REGMAP_MMIO
> > > > > > +	help
> > > > > > +	  Select this option to enable STM32 FMC2 driver used for FMC2 External
> > > > > > +	  Bus Interface controller and FMC2 NAND flash controller. This driver
> > > > > > +	  provides core support for the STM32 FMC2 controllers, in order to use
> > > > > > +	  the actual functionality of the device other drivers must be enabled.
> > > > > 
> > > > > Not sure how many times I have to say this before people stop
> > > > > attempting to pass these kinds of relationships off as MFDs:
> > > > > 
> > > > > A memory device and its bus is not an MFD.  In a similar vain to the
> > > > > thousands of USB, I2C, SPI, PCI and the like devices that aren't MFDs
> > > > > either.
> > > > > 
> > > > > Please find another way to associate your device with its bus.
> > > > 
> > > > This FMC2 is however an IP which can either operate external devices
> > > > (like ethernet chip on this parallel bus) or external flashes (like NOR
> > > > and NAND chips).
> > > 
> > > I'm sure that it *can*.  Although that's not its main purpose.
> > 
> > I use it to operate KSZ8851-16MLL ethernet chip, which has async bus
> > interface. Linux just didn't have support for that mode of operation
> > thus far and the FMC was used to operate NANDs and NORs only. This
> > series, or rather, the first three patches in this series, add support
> > for operating other bus devices, like this ethernet controller.
> > 
> > > The
> > > clue is in the nomenclature ("Flexible *Memory* Controller").  Nor is
> > > it how the device is being used in this submission:
> > > 
> > >    "The FMC2 functional block makes the interface with: synchronous and
> > >     asynchronous static memories (such as PSNOR, PSRAM or other
> > >     memory-mapped peripherals) and NAND flash memories."
> > > 
> > > As I mentioned, this is just another memory device and its bus.
> > 
> > I don't think it's _just_ a memory controller, it's more universal than
> > that, see above. Note that SRAM interface basically boils down to
> > anything which has external parallel bus, e.g. Davicom DM9000, that
> > KSZ8851-16MLL etc.
> > 
> > > > Can you provide a suggestion how this should be handled, if not as MFD?
> > > > It seems to me, that this is a Multi-Function Device .
> > > 
> > > Simply move it into the MTD or Memory subsystems and set up the
> > > dependencies via Kconfig.
> > > 
> > > > If this discussion is a recurring topic, is there some documentation
> > > > which explains how such devices should be handled ?
> > > 
> > > Not that I'm aware of.
> > 
> > I see.
> > 
> 
> Hi Lee, Marek,
> 
> I will move this source code in the FMC2 bus driver. I think that I should
> be able to manage the 2 controllers with 2 drivers (the FMC2 bus driver and
> the FMC2 raw NAND driver). I will have to modify some part of the proposed
> bindings, and some updates will have to be done in the FMC2 bus driver. All
> these modifications will be part of V3.

That sounds like a very reasonable solution.

>From a purist's PoV, the bindings should not change.  As they should
describe the hardware, rather than the implementation.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists