Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: lzmKkN/NwN8Nx2QxIBMtlzk2YROhYDMlQ87fIx3xpEEmhgBUPdsP6AsaO3fzxpKnHUB7YNfgQ9
 gulnspWrqMFA==
IronPort-SDR: jSWcHMmqzar3fEh5DPjL+H/8KuVscNmygxYFk8cLFK2KWB18i/fG1wdSYF2Gw55NH1v6YP0OhC
 ppyCB8dbdhNg==
From:   "Wu, Hao" <hao.wu@intel.com>
To:     "Xu, Yilun" <yilun.xu@intel.com>
CC:     Moritz Fischer <mdf@kernel.org>,
        "matthew.gerlach@linux.intel.com" <matthew.gerlach@linux.intel.com>,
        "trix@redhat.com" <trix@redhat.com>,
        "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "jdelvare@suse.com" <jdelvare@suse.com>,
        "linux@roeck-us.net" <linux@roeck-us.net>,
        "lee.jones@linaro.org" <lee.jones@linaro.org>,
        "linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
        "russell.h.weight@linux.intel.com" <russell.h.weight@linux.intel.com>
Subject: RE: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master
Thread-Topic: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master
Thread-Index: AQHXKnapzaN+e8jiOUasbNG/fZUbfKqmp0KAgAEBK4CAAAt/AIAChFHwgAAQfwCAAAHe8IABIkcAgAAgaTA=
Date:   Fri, 9 Apr 2021 04:02:47 +0000
Message-ID: <DM6PR11MB381912BD810637E0CDA1885F85739@DM6PR11MB3819.namprd11.prod.outlook.com>
References: <20210405235301.187542-1-matthew.gerlach@linux.intel.com>
 <20210405235301.187542-3-matthew.gerlach@linux.intel.com>
 <YGuvFYvJTMPPm2Jy@epycbox.lan>
 <alpine.DEB.2.22.394.2104060847030.208844@rhweight-WRK1>
 <YGyQdN9uS/niyFDP@epycbox.lan>
 <DM6PR11MB3819E0FC4F735C72746CE54785749@DM6PR11MB3819.namprd11.prod.outlook.com>
 <20210408081152.GA2713@yilunxu-OptiPlex-7050>
 <DM6PR11MB3819317A55FD7798E778EC4D85749@DM6PR11MB3819.namprd11.prod.outlook.com>
 <20210409013730.GB2713@yilunxu-OptiPlex-7050>
In-Reply-To: <20210409013730.GB2713@yilunxu-OptiPlex-7050>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: fab9c03a-23ce-4f67-02f2-08d8fb0c5654
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Apr 2021 04:02:47.8221
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: nf894HLxEBBcNbNDtlVSqu34UF5ceCbqrwpaq8UYziSKhEDH7s+mv6eCaw6Jon/eezG6oFlEbjU3rhJA2eYDUQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB3916
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > > > > > > > +
> > > > > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_d=
ev)
> > > > > > > > +{
> > > > > > > > +struct dfl_altera_spi *aspi =3D 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[] =3D=
 {
> > > > > > > > +{ 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 (t=
hough
> > > > > they might be guarding against what you describe with CONFIG_OF v=
s
> > > > > 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 alter=
a-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
>=20
> 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?

>=20
> > other buses are used, not only for DFL, like PCI device or platform dev=
ice,
> > right? then why not put related code together with the original IP?
>=20
> 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 enhancemen=
ts
to one IP then driver could be simplified and doesn't need devicetree any m=
ore.
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 withou=
t
device tree, so that this IP can be used in DFL or PCI or other buses witho=
ut
device tree?

Thanks
Hao

>=20
> >
> > The reason I suggested that function drivers which use DFL bus, still n=
eed
> > 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. : )
>=20
> 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.
>=20
> Thanks,
> Yilun
>=20
> >
> > Hao
> >
> > >
> > > Thanks,
> > > Yilun
> > >
> > > >
> > > > Thanks
> > > > Hao
> > > >
> > > > >
> > > > > Cheers,
> > > > > Moritz