Re: [PATCH RFC 2/2] mfd: add Renesas RPC-IF driver

[Lee Jones <lee.jones@xxxxxxxxxx>]

On Tue, 18 Jun 2019, Sergei Shtylyov wrote:

> On 06/18/2019 12:57 PM, Lee Jones wrote:
> 
> [...]
> >>>>>>>> +static int rpcif_probe(struct platform_device *pdev)
> >>>>>>>> +{
> >>>>>>>> +  struct device_node *flash;
> >>>>>>>> +  const struct mfd_cell *cell;
> >>>>>>>> +  struct resource *res;
> >>>>>>>> +  void __iomem *base;
> >>>>>>>> +  struct rpcif *rpc;
> >>>>>>>> +
> >>>>>>>> +  flash = of_get_next_child(pdev->dev.of_node, NULL);
> >>>>>>>> +  if (!flash) {
> >>>>>>>> +          dev_warn(&pdev->dev, "no flash node found\n");
> >>>>>>>> +          return -ENODEV;
> >>>>>>>> +  }
> >>>>>>>> +
> >>>>>>>> +  if (of_device_is_compatible(flash, "jedec,spi-nor")) {
> >>>>>>>> +          cell = &rpcif_spi_ctlr;
> >>>>>>>> +  } else if (of_device_is_compatible(flash, "cfi-flash")) {
> >>>>>>>> +          cell = &rpcif_hf_ctlr;
> >>>>>>>> +  } else {
> >>>>>>>> +          dev_warn(&pdev->dev, "unknown flash type\n");
> >>>>>>>> +          return -ENODEV;
> >>>>>>>> +  }
> >>>>>>>
> >>>>>>> Why not let DT choose which device to probe?
> >>>>>>
> >>>>>>    It's the DT that decides here. How else would you imagine that?
> >>>>>> It's the same hardware, just the different physical busses that it
> >>>>>> commands...
> >>>>>
> >>>>> DT is not deciding here.  This driver is deciding based on the
> >>>>> information contained in the tables - very different thing.
> >>>>>
> >>>>> Why not just let the OF framework probe the correct device i.e. let it
> >>>>> parse the 2 compatible strings and let it match the correct driver for
> >>>>> the device?
> >>>>
> >>>> The OF framework matches against the RPC-IF node, which is a single
> >>>> hardware type, hence has a fixed compatible value.
> >>>> The mode depends on the subnode in DT, which is something the OF
> >>>> framework doesn't match against, so the driver itself has to check the
> >>>> subnode's compatible value.
> >>>
> >>> I can see how it has been implemented.
> >>>
> >>> It is that which I was questioning.
> >>>
> >>>> DT describes hardware, not software (Linux subsystem boundary) policy.
> >>>
> >>> So is an RPC-IF a real hardware device.  Can you share the datasheet?
> >>
> >> Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
> >> not yet public.
> > 
> > When will it be public?
> 
>    Ask Renesas. :-)
> 
> > Do you have access to it?
> 
>    We do...
> 
> > Maybe someone who does can help with the magic number definitions.
> 
>    Not very likely. :-)
> 
> >> However, a very similar hardware block is present in the RZ/A2M SoC.
> >> Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
> >> User’s Manual: Hardware", which you can download from
> >> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents
> 
>    Also, there's RZ/A1 where a previous version of this hardware seems to be used, see chapter 17
> (SPI Multi I/O Bus Controller) of the RZ/A1H/M manual, downloadable from:
> https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz/rza/rza1h.html#documents
> 
> >   "The SPI multi I/O bus controller enables the direct connection of
> >    serial flash, OctaFlashTM, XccelaTM flash, or HyperFlashTM memory
> >    devices to this LSI chip.> 
> >    This module allows the connected serial flash, OctaFlashTM, XccelaTM
> >    flash, or HyperFlashTM memory devices to be accessed by reading the
> >    external address space, or using Manual mode to transmit and receive
> >    data."
> 
>    For contrast, RZ/A1 didn't yet support OctaFlash and HyperFlash, and R-Car gen3 RPC-IF doesn't
> support Xccella yet...
> 
> > Looks like a flash device to me.
> 
>    More like a multi-protocol flash controller, with the real flash chip connected
> to it.

Right, this has been my point from the start.

It's a flash controller.  Sure, you can access it in different ways,
but it's still *just* a flash controller and thus not a true MFD.

Surely this whole thing, including the shared portion should live in
one of the memory related subsystems?

This is not the first device people have tried to shove in MFD, that
is really *just* an <X> device, able to be controlled via different
protocols.

MFD is for registering child devices of chips which offer genuine
cross-subsystem functionality.  It is not designed for mode selecting,
or as a place to shove shared code just because a better location
doesn't appear to exist.

Also, ramming it into drivers/platform/<vendor> is not correct either,
since this is not a platform controller driver either.

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