Message-ID: <20210409054118.GA7986@yilunxu-OptiPlex-7050>
Date:   Fri, 9 Apr 2021 13:41:18 +0800
From:   Xu Yilun <yilun.xu@...el.com>
To:     "Wu, Hao" <hao.wu@...el.com>
Cc:     Moritz Fischer <mdf@...nel.org>,
        "matthew.gerlach@...ux.intel.com" <matthew.gerlach@...ux.intel.com>,
        "trix@...hat.com" <trix@...hat.com>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jdelvare@...e.com" <jdelvare@...e.com>,
        "linux@...ck-us.net" <linux@...ck-us.net>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "russell.h.weight@...ux.intel.com" <russell.h.weight@...ux.intel.com>
Subject: Re: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

On Fri, Apr 09, 2021 at 12:02:47PM +0800, Wu, Hao wrote:
> > > > > > > > > +
> > > > > > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > > > > > > +{
> > > > > > > > > +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > > > > > > +
> > > > > > > > > +platform_device_unregister(aspi->altr_spi);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > > > > > > +
> > > > > > > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > > > > > > +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > > > > > > +{ }
> > > > > > > > > +};
> > > > > > > >
> > > > > > > > Maybe you can extend the Altera SPI driver with this part?
> > > > > > >
> > > > > > > The file, drivers/spi/spi-altera.c, already has platform MODULE_
> > related
> > > > > > > code.  Wouldn't moving this code to that file produce conflicts?
> > > > > >
> > > > > > I've seen other drivers support multiple busses, so it should be
> > > > > > possible, there might be nuances I'm missing in my brief look at this,
> > > > > > though.
> > > > > >
> > > > > > I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> > > > > > and the other one MODULE_DEVICE_TABLE(dfl, ...)
> > > > > >
> > > > > > See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
> > > > > > they might be guarding against what you describe with CONFIG_OF vs
> > > > > > CONFIG_ACPI)
> > > > > >
> > > > > > If that doesn't work we could split it up into
> > > > > >
> > > > > > altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> > > > > > or something of that sort?
> > > > > >
> > > > > > My point being, now that we have a bus, let's use it and develop drivers
> > > > > > according to the Linux device model where possible :)
> > > > >
> > > > > Agree. This does make sense from my side too. DFL core provides the
> > > > mechanism
> > > > > to enumerate different IPs on FPGA, but each function driver needs to go
> > to
> > > > > related subsystem for review.  : )
> > > > >
> > > > > I understand that for FPGA case, it may have some additional logics for
> > specific
> > > > > purposes based on common altera spi master IP, then additional code for
> > > >
> > > > I'm wondering if the additional logics are extensions for common spi-altera.
> > Like
> > > > the
> > > > SPI_CORE_PARAMETER register, it is not within the register space of
> > > > spi-altera,
> > > >
> > > >
> > > >   |   |      +-------------+
> > > >   |DFL|------| +--------+  |
> > > >   |BUS|      | |SPI CORE|  |
> > > >   |   |      | |PARAM   |  |
> > > >   |   |      | +--------+  |
> > > >   |   |      |             |
> > > >   |   |      | +--------+  |   +-------+
> > > >              | |Indirect|  |   |spi    |
> > > >              | |access  +--+---|altera |
> > > >              | |master  |  |   +-------+
> > > >              | +--------+  |
> > > >              +-------------+
> > > > > a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-
> > xxxx.c
> > > >
> > > > So is it proper we integrate this feature into spi-altera? Previously
> > > > we have merged the dfl-n3000-nios, its spi part is very similar as
> > > > this driver. The dfl-n3000-nios make the spi-altera as a sub device.
> > > > Could we borrow the idea, or could we just integrate this driver in
> > > > dfl-n3000-nios?
> > >
> > > Looks like those are enhancements of the IP. They can be applied even
> >
> > I don't think the extra registers are the enhancement of the IP. They
> > are not part of the IP because they are not within the IP's register
> > space. They are like some external way of describing the IP like
> > Devicetree or ACPI.
> 
> Why adding new registers can't be consider as enhancement, those
> changes serve the original IP and make it better, right? small mmio
> footprint and parameter registers?
> 
> >
> > > other buses are used, not only for DFL, like PCI device or platform device,
> > > right? then why not put related code together with the original IP?
> >
> > The code of devicetree or ACPI parsing are integrated in the IP drivers,
> > but for this case, it may not be proper for now, cause this style is not
> > formally introduced by any standard. IP specific parameters description
> > are not within the scope of DFL now.
> 
> Not sure if I get your point, but it's possible that we add some enhancements
> to one IP then driver could be simplified and doesn't need devicetree any more.
> For sure, it's IP specific thing, not the scope of DFL.
> 
> Then things become this: extension to IP to allow this IP to be used without
> device tree, so that this IP can be used in DFL or PCI or other buses without
> device tree?

It's good to extend an IP, but it needs a published SPEC and stable
register interfaces. For now, the spi-altera driver conforms to the
"SPI Core" chapter of the following spec:

https://www.intel.com/content/www/us/en/programmable/documentation/sfo1400787952932.html

There is no info about the core parameter register and this specific
indirect access bus. That's why I don't see these additional parts as
the enhancements to spi-altera. This DFL feature is like a wrapper for
the spi-altera sub device.

Thanks
Yilun

Powered by blists - more mailing lists