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: K831olBq3McxU4fN5OJkkn09JKVMScltWgEXltlVKcLgq9fQobpeT9IJm67WtNWMDxr5aJEYOq
 6WWbeV0uMLog==
IronPort-SDR: qFWjlWgv9MLuFGGZ3NlQDRp5BGGLqlcfN1UROYNsySEZAex4UM0ICnhezXHg65RsLZpcqIA+S0
 lYBfhMlfNhhA==
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/AIAChFHwgAAQfwCAAAHe8IABIkcAgAAgaTCAACO1AIAADazA
Date:   Fri, 9 Apr 2021 07:20:09 +0000
Message-ID: <DM6PR11MB381916140583F3C31D66EFC385739@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>
 <DM6PR11MB381912BD810637E0CDA1885F85739@DM6PR11MB3819.namprd11.prod.outlook.com>
 <20210409054118.GA7986@yilunxu-OptiPlex-7050>
In-Reply-To: <20210409054118.GA7986@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: 3f93b4ec-bc57-4299-d234-08d8fb27e899
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Apr 2021 07:20:09.6107
 (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: wFQQwLPV82qAvOLyBpQaiLpsJjKV0Pv7Q9P0Wr+uAxV2y7XZcWWAvyaVAJYLwrnim+bmERvm9hbMxmQth+eHSg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5344
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> On Fri, Apr 09, 2021 at 12:02:47PM +0800, Wu, Hao wrote:
> > > > > > > > > > +
> > > > > > > > > > +static void dfl_spi_altera_remove(struct dfl_device *d=
fl_dev)
> > > > > > > > > > +{
> > > > > > > > > > +struct dfl_altera_spi *aspi =3D dev_get_drvdata(&dfl_d=
ev->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 MO=
DULE_
> > > related
> > > > > > > > code.  Wouldn't moving this code to that file produce confl=
icts?
> > > > > > >
> > > > > > > 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 exampl=
e
> (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 deve=
lop
> 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 ne=
eds to
> go
> > > to
> > > > > > related subsystem for review.  : )
> > > > > >
> > > > > > I understand that for FPGA case, it may have some additional lo=
gics 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 a=
ltera-spi-dfl-
> > > xxxx.c
> > > > >
> > > > > So is it proper we integrate this feature into spi-altera? Previo=
usly
> > > > > we have merged the dfl-n3000-nios, its spi part is very similar a=
s
> > > > > this driver. The dfl-n3000-nios make the spi-altera as a sub devi=
ce.
> > > > > 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 ev=
en
> > >
> > > 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 drive=
rs,
> > > but for this case, it may not be proper for now, cause this style is =
not
> > > formally introduced by any standard. IP specific parameters descripti=
on
> > > are not within the scope of DFL now.
> >
> > Not sure if I get your point, but it's possible that we add some enhanc=
ements
> > to one IP then driver could be simplified and doesn't need devicetree a=
ny 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 wi=
thout
> > device tree, so that this IP can be used in DFL or PCI or other buses w=
ithout
> > device tree?
>=20
> 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:
>=20
> https://www.intel.com/content/www/us/en/programmable/documentation/sf
> o1400787952932.html
>=20
> 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.

It really doesn't matter, even if you consider this as an new IP, it's stil=
l a SPI
Master, it's driver still need to be reviewed in drivers/spi subsystem. The
worst case is that we need to write a new spi-xxx.c driver, that's it.

From DFL part, DFL only can enumerate the common hardware resources,
but no good way to help thing like this, specific IP parameters (which hand=
le
by devicetree in platform driver). So for some IPs , they still need some
extensions to avoid such dependency (on device tree for parameters). I gues=
s
we may see more similar cases in the future.

Anyway, I think we reached agreement that for device drivers on DFL bus, it
needs to be reviewed in its own subsystem. : )

Thanks
Hao

>=20
> Thanks
> Yilun