linux-kernel - RE: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM6PR11MB381912BD810637E0CDA1885F85739@DM6PR11MB3819.namprd11.prod.outlook.com>
Date:   Fri, 9 Apr 2021 04:02:47 +0000
From:   "Wu, Hao" <hao.wu@...el.com>
To:     "Xu, Yilun" <yilun.xu@...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

> > > > > > > > +
> > > > > > > > +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?

Thanks
Hao

> 
> >
> > The reason I suggested that function drivers which use DFL bus, still need
> > to go to related subsystem, because we know DFL quite well but may
> > not be the experts for every subsystem (e.g. SPI), and every IPs (e.g.
> > Altera SPI Master). Altera SPI Master driver maintainer could give more
> > valuable suggestions on your question if this feature can be integrated
> > into spi-altera or not. : )
> 
> I agree that we put drivers to their related subsystem. I'm just
> wondering if drivers/spi is the proper domain for it. Anyway getting
> some inputs from SPI maintainers is a good suggestion.
> 
> Thanks,
> Yilun
> 
> >
> > Hao
> >
> > >
> > > Thanks,
> > > Yilun
> > >
> > > >
> > > > Thanks
> > > > Hao
> > > >
> > > > >
> > > > > Cheers,
> > > > > Moritz