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

[Lee Jones <lee.jones@xxxxxxxxxx>]

On Tue, 18 Jun 2019, Geert Uytterhoeven wrote:
> On Tue, Jun 18, 2019 at 11:20 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > On Tue, 18 Jun 2019, Geert Uytterhoeven wrote:
> > > On Mon, Jun 17, 2019 at 11:31 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > > > On Fri, 14 Jun 2019, Sergei Shtylyov wrote:
> > > > > On 06/12/2019 12:05 PM, Lee Jones wrote:
> > > > > >> +static const struct mfd_cell rpcif_hf_ctlr = {
> > > > > >> +  .name = "rpcif-hyperflash",
> > > > > >> +};
> > > > > >> +
> > > > > >> +static const struct mfd_cell rpcif_spi_ctlr = {
> > > > > >> +  .name = "rpcif-spi",
> > > > > >> +};
> > > > > >
> > > > > > This looks like a very tenuous use of the MFD API.
> > > > > >
> > > > > > I suggest that this isn't actually an MFD at all.
> > > > >
> > > > >    The same hardware supports 2 different physical interfaces, hence
> > > > > the drivers have to comply to 2 different driver frameworks... sounded
> > > > > like MFD to me. :-)
> > > >
> > > > Not to me.
> > > >
> > > > This appears to be some kind of 'mode selector' for an MTD device.
> > >
> > > ... for either an SPI or MTD device.
> >
> > Okay, so I think I misunderstood the device.  I was under the
> > impression that it was a flash memory device where the only difference
> > was the interface by which it is controlled?
> >
> > > > Lots of drivers have multiple ways to control them - they are not all
> > > > MFDs.
> > >
> > > So where to reside the common part? drivers/platform/renesas/?
> >
> > That does not make sense, since this is not a platform controller.
> >
> > > > > [...]
> > > > > >> +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?

Do you have access to it?

Maybe someone who does can help with the magic number definitions.

> 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

  "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."

Looks like a flash device to me.

Can the SPI portion be used to connect generic SPI devices?

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