| Message ID | 20200821131029.11440-1-kuldip.dwivedi@puresoftware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: spi-fsl-dspi: Add ACPI support | expand |
On Fri, Aug 21, 2020 at 06:40:29PM +0530, kuldip dwivedi wrote: > +static const struct acpi_device_id fsl_dspi_acpi_ids[] = { > + { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], }, > + {}, > +}; > +MODULE_DEVICE_TABLE(acpi, fsl_dspi_acpi_ids); Does NXP know about this ID assignment from their namespace? ACPI IDs should be namespaced by whoever's assigning the ID to avoid collisions. > - ret = of_property_read_u32(np, "spi-num-chipselects", &cs_num); > + if (is_acpi_node(pdev->dev.fwnode)) > + ret = device_property_read_u32(&pdev->dev, > + "spi-num-chipselects", &cs_num); > + else > + ret = of_property_read_u32(np, > + "spi-num-chipselects", &cs_num); The whole point with the device property API is that it works with both DT and ACPI without needing separate parsing, though in this case I'm wondering why we'd need to specify this in an ACPI system at all? > - of_property_read_u32(np, "bus-num", &bus_num); > + if (is_acpi_node(pdev->dev.fwnode)) { > + ret = device_property_read_u32(&pdev->dev, > + "bus-num", &bus_num); This is a bad idea for DT and I can't understand why you'd carry it over for ACPI - why would an ACPI system ever care about this? It's Linux internal at the best of times. It looks like you've done this by simply adding these device property alternatives for every DT property. This isn't how that API is intended to be used and suggests that this isn't a thought through, idiomatic ACPI binding. I'd suggest looking at the Synquacer driver for an example of how this would normally be done, I'd expect your ACPI code to look very much like theirs.
Hi kuldip,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on spi/for-next]
[also build test WARNING on v5.9-rc1 next-20200821]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/kuldip-dwivedi/spi-spi-fsl-dspi-Add-ACPI-support/20200821-211355
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: arm64-randconfig-r006-20200820 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project b587ca93be114d07ec3bf654add97d7872325281)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/spi/spi-fsl-dspi.c:1075:36: warning: unused variable 'fsl_dspi_acpi_ids' [-Wunused-const-variable]
static const struct acpi_device_id fsl_dspi_acpi_ids[] = {
^
1 warning generated.
# https://github.com/0day-ci/linux/commit/00b7c46d88c9150bd8225fce2b7b95e186514e10
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review kuldip-dwivedi/spi-spi-fsl-dspi-Add-ACPI-support/20200821-211355
git checkout 00b7c46d88c9150bd8225fce2b7b95e186514e10
vim +/fsl_dspi_acpi_ids +1075 drivers/spi/spi-fsl-dspi.c
1074
> 1075 static const struct acpi_device_id fsl_dspi_acpi_ids[] = {
1076 { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], },
1077 {},
1078 };
1079 MODULE_DEVICE_TABLE(acpi, fsl_dspi_acpi_ids);
1080
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Friday, August 21, 2020 7:37 PM > To: kuldip dwivedi <kuldip.dwivedi@puresoftware.com> > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; Qiang Zhao > <qiang.zhao@nxp.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; Varun Sethi > <V.Sethi@nxp.com>; tanveer <tanveer.alam@puresoftware.com> > Subject: Re: [PATCH] spi: spi-fsl-dspi: Add ACPI support > > On Fri, Aug 21, 2020 at 06:40:29PM +0530, kuldip dwivedi wrote: > > > +static const struct acpi_device_id fsl_dspi_acpi_ids[] = { > > + { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(acpi, fsl_dspi_acpi_ids); > > Does NXP know about this ID assignment from their namespace? ACPI IDs should > be namespaced by whoever's assigning the ID to avoid collisions. Yes, I got HID from NXP only. > > > - ret = of_property_read_u32(np, "spi-num-chipselects", &cs_num); > > + if (is_acpi_node(pdev->dev.fwnode)) > > + ret = device_property_read_u32(&pdev->dev, > > + "spi-num-chipselects", &cs_num); > > + else > > + ret = of_property_read_u32(np, > > + "spi-num-chipselects", &cs_num); > > The whole point with the device property API is that it works with both DT and ACPI > without needing separate parsing, though in this case I'm wondering why we'd > need to specify this in an ACPI system at all? Understood. Will take care in v2 PATCH > > > - of_property_read_u32(np, "bus-num", &bus_num); > > + if (is_acpi_node(pdev->dev.fwnode)) { > > + ret = device_property_read_u32(&pdev->dev, > > + "bus-num", &bus_num); > > This is a bad idea for DT and I can't understand why you'd carry it over for ACPI - > why would an ACPI system ever care about this? It's Linux internal at the best of > times. Will take care in v2 PATCH > > It looks like you've done this by simply adding these device property alternatives > for every DT property. This isn't how that API is intended to be used and suggests > that this isn't a thought through, idiomatic ACPI binding. I'd suggest looking at the > Synquacer driver for an example of how this would normally be done, I'd expect > your ACPI code to look very much like theirs.
On Sat, Aug 22, 2020 at 07:37:25PM +0530, Kuldip Dwivedi wrote: > > -----Original Message----- > > From: Mark Brown <broonie@kernel.org> > > Sent: Friday, August 21, 2020 7:37 PM > > To: kuldip dwivedi <kuldip.dwivedi@puresoftware.com> > > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; Qiang Zhao > > <qiang.zhao@nxp.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; Varun Sethi > > <V.Sethi@nxp.com>; tanveer <tanveer.alam@puresoftware.com> > > Subject: Re: [PATCH] spi: spi-fsl-dspi: Add ACPI support > > > > On Fri, Aug 21, 2020 at 06:40:29PM +0530, kuldip dwivedi wrote: > > > > > +static const struct acpi_device_id fsl_dspi_acpi_ids[] = { > > > + { "NXP0005", .driver_data = > (kernel_ulong_t)&devtype_data[LS2085A], }, > > > + {}, > > > +}; > > > +MODULE_DEVICE_TABLE(acpi, fsl_dspi_acpi_ids); > > > > Does NXP know about this ID assignment from their namespace? ACPI > > IDs should be namespaced by whoever's assigning the ID to avoid > > collisions. > Yes, I got HID from NXP only. > > > > > - ret = of_property_read_u32(np, "spi-num-chipselects", > &cs_num); > > > + if (is_acpi_node(pdev->dev.fwnode)) > > > + ret = device_property_read_u32(&pdev->dev, > > > + "spi-num-chipselects", &cs_num); > > > + else > > > + ret = of_property_read_u32(np, > > > + "spi-num-chipselects", &cs_num); > > > > The whole point with the device property API is that it works with > > both DT and ACPI without needing separate parsing, though in this > > case I'm wondering why we'd need to specify this in an ACPI system > > at all? > Understood. Will take care in v2 PATCH > > IMO there is zero reason for the existence of the "spi-num-chipselects" property even for DT. We should deprecate it (start ignoring it in existing device tree deployments) and populate struct fsl_dspi_devtype_data with that info based on SoC compatible string. > > > - of_property_read_u32(np, "bus-num", &bus_num); > > > + if (is_acpi_node(pdev->dev.fwnode)) { > > > + ret = device_property_read_u32(&pdev->dev, > > > + "bus-num", > &bus_num); > > > > This is a bad idea for DT and I can't understand why you'd carry it > > over for ACPI - why would an ACPI system ever care about this? It's > > Linux internal at the best of times. > Will take care in v2 PATCH Yes, definitely bloatware from the old days. I think this driver needs the existing device tree bindings rethought a little bit before mindlessly porting them to ACPI. > > > > It looks like you've done this by simply adding these device > > property alternatives for every DT property. This isn't how that > > API is intended to be used and suggests that this isn't a thought > > through, idiomatic ACPI binding. I'd suggest looking at the > > Synquacer driver for an example of how this would normally be done, > > I'd expect your ACPI code to look very much like theirs. Speaking of which, on what SPI peripherals was this tested? I am not sure how other controllers deal with this, but DSPI has, by default, no CS-to-SCK and a SCK-to-CS delays. Those must be explicitly requested through the custom "fsl,spi-cs-sck-delay" and "fsl,spi-sck-cs-delay" DT bindings for each individual SPI peripheral. Some peripherals just don't work when the CS-to-SCK and SCK-to-CS delays are zero, and I don't see the ACPI variant of the driver attempting to read those properties, hence the question. Thanks, -Vladimir
On Fri, Aug 21, 2020 at 06:40:29PM +0530, kuldip dwivedi wrote: > Currently fsl DSPI driver has support of DT only. Adding ACPI > support to the drive so that it can be used by UEFI firmware > boot in ACPI mode. This driver will be probed if any firmware > will expose HID "NXP0005" in DSDT table. > > Signed-off-by: tanveer <tanveer.alam@puresoftware.com> > Signed-off-by: kuldip dwivedi <kuldip.dwivedi@puresoftware.com> > --- > drivers/spi/spi-fsl-dspi.c | 91 +++++++++++++++++++++++++++++--------- > 1 file changed, 69 insertions(+), 22 deletions(-) > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c > index 91c6affe139c..7100a8a0a30e 100644 > --- a/drivers/spi/spi-fsl-dspi.c > +++ b/drivers/spi/spi-fsl-dspi.c > @@ -1070,6 +1072,12 @@ static void dspi_cleanup(struct spi_device *spi) > kfree(chip); > } > > +static const struct acpi_device_id fsl_dspi_acpi_ids[] = { > + { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], }, > + {}, > +}; > +MODULE_DEVICE_TABLE(acpi, fsl_dspi_acpi_ids); > + Just noticed this now. So for device tree, spi-fsl-dspi supports the following compatibles: fsl,vf610-dspi fsl,ls1021a-v1.0-dspi fsl,ls1012a-dspi fsl,ls1028a-dspi fsl,ls1043a-dspi fsl,ls1046a-dspi fsl,ls2080a-dspi fsl,ls2085a-dspi fsl,lx2160a-dspi Depending on the compatible string, the driver knows whether to use DMA or XSPI mode, and what the FIFO size is. Now, of course not all of the above SoCs are going to support ACPI, but it is reasonable to expect that more than one will. And in that case, the driver should still be able to know on what SoC it's running, because for example LS1043A doesn't support DMA, and LS2085A doesn't support XSPI. How is this dealt with in ACPI? Thanks, -Vladimir
On Sat, Aug 22, 2020 at 06:21:18PM +0300, Vladimir Oltean wrote: > On Sat, Aug 22, 2020 at 07:37:25PM +0530, Kuldip Dwivedi wrote: > > > The whole point with the device property API is that it works with > > > both DT and ACPI without needing separate parsing, though in this > > > case I'm wondering why we'd need to specify this in an ACPI system > > > at all? > > Understood. Will take care in v2 PATCH > IMO there is zero reason for the existence of the "spi-num-chipselects" > property even for DT. We should deprecate it (start ignoring it in > existing device tree deployments) and populate struct > fsl_dspi_devtype_data with that info based on SoC compatible string. Yes, it's a legacy from bad board file conversions and shouldn't be used at all.
On Mon, Aug 24, 2020 at 19:25, Mark Brown <broonie@kernel.org> wrote: > -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: 2020年8月24日 19:25 > To: Vladimir Oltean <olteanv@gmail.com> > Cc: kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; > linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; Qiang Zhao > <qiang.zhao@nxp.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; Varun Sethi > <V.Sethi@nxp.com>; Tanveer Alam <tanveer.alam@puresoftware.com> > Subject: Re: [PATCH] spi: spi-fsl-dspi: Add ACPI support > > On Sat, Aug 22, 2020 at 06:21:18PM +0300, Vladimir Oltean wrote: > > On Sat, Aug 22, 2020 at 07:37:25PM +0530, Kuldip Dwivedi wrote: > > > > > The whole point with the device property API is that it works with > > > > both DT and ACPI without needing separate parsing, though in this > > > > case I'm wondering why we'd need to specify this in an ACPI system > > > > at all? > > > > Understood. Will take care in v2 PATCH > > > IMO there is zero reason for the existence of the "spi-num-chipselects" > > property even for DT. We should deprecate it (start ignoring it in > > existing device tree deployments) and populate struct > > fsl_dspi_devtype_data with that info based on SoC compatible string. > > Yes, it's a legacy from bad board file conversions and shouldn't be used at all. I saw a lot of driver assign spi_controller -> num_chipselect directly, should we do like that? BR Qiang Zhao
On Wed, Aug 26, 2020 at 08:19:41AM +0000, Qiang Zhao wrote: > On Mon, Aug 24, 2020 at 19:25, Mark Brown <broonie@kernel.org> wrote: > > Yes, it's a legacy from bad board file conversions and shouldn't be used at all. > I saw a lot of driver assign spi_controller -> num_chipselect directly, should we do like that? Yes, you should know the number of chip selects based on the controller.
On Sat, Aug 22, 2020 at 23:21PM, Vladimir Oltean <olteanv@gmail.com> wrote: > -----Original Message----- > From: Vladimir Oltean <olteanv@gmail.com> > Sent: 2020年8月22日 23:21 > To: kuldip dwivedi <kuldip.dwivedi@puresoftware.com> > Cc: Mark Brown <broonie@kernel.org>; linux-spi@vger.kernel.org; > linux-kernel@vger.kernel.org; Qiang Zhao <qiang.zhao@nxp.com>; Pankaj > Bansal <pankaj.bansal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Tanveer > Alam <tanveer.alam@puresoftware.com> > Subject: Re: [PATCH] spi: spi-fsl-dspi: Add ACPI support > > On Sat, Aug 22, 2020 at 07:37:25PM +0530, Kuldip Dwivedi wrote: > > > -----Original Message----- > > > From: Mark Brown <broonie@kernel.org> > > > Sent: Friday, August 21, 2020 7:37 PM > > > To: kuldip dwivedi <kuldip.dwivedi@puresoftware.com> > > > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; Qiang > > > Zhao <qiang.zhao@nxp.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; > > > Varun Sethi <V.Sethi@nxp.com>; tanveer > > > <tanveer.alam@puresoftware.com> > > > Subject: Re: [PATCH] spi: spi-fsl-dspi: Add ACPI support > > > > > > On Fri, Aug 21, 2020 at 06:40:29PM +0530, kuldip dwivedi wrote: > > > > > > > +static const struct acpi_device_id fsl_dspi_acpi_ids[] = { > > > > + { "NXP0005", .driver_data = > > (kernel_ulong_t)&devtype_data[LS2085A], }, > > > > + {}, > > > > +}; > > > > +MODULE_DEVICE_TABLE(acpi, fsl_dspi_acpi_ids); > > > > > > Does NXP know about this ID assignment from their namespace? ACPI > > > IDs should be namespaced by whoever's assigning the ID to avoid > > > collisions. > > Yes, I got HID from NXP only. > > > > > > > - ret = of_property_read_u32(np, "spi-num-chipselects", > > &cs_num); > > > > + if (is_acpi_node(pdev->dev.fwnode)) > > > > + ret = device_property_read_u32(&pdev->dev, > > > > + "spi-num-chipselects", &cs_num); > > > > + else > > > > + ret = of_property_read_u32(np, > > > > + "spi-num-chipselects", &cs_num); > > > > > > The whole point with the device property API is that it works with > > > both DT and ACPI without needing separate parsing, though in this > > > case I'm wondering why we'd need to specify this in an ACPI system > > > at all? > > Understood. Will take care in v2 PATCH > > > > > IMO there is zero reason for the existence of the "spi-num-chipselects" > property even for DT. We should deprecate it (start ignoring it in existing device > tree deployments) and populate struct fsl_dspi_devtype_data with that info > based on SoC compatible string. > > > > > - of_property_read_u32(np, "bus-num", &bus_num); > > > > + if (is_acpi_node(pdev->dev.fwnode)) { > > > > + ret = device_property_read_u32(&pdev->dev, > > > > + "bus-num", > > &bus_num); > > > > > > This is a bad idea for DT and I can't understand why you'd carry it > > > over for ACPI - why would an ACPI system ever care about this? It's > > > Linux internal at the best of times. > > Will take care in v2 PATCH > > Yes, definitely bloatware from the old days. I think this driver needs the existing > device tree bindings rethought a little bit before mindlessly porting them to > ACPI. Could you give more details? Best Regards Qiang Zhao
On Wed, Aug 26, 2020 at 11:10:49AM +0000, Qiang Zhao wrote: > On Sat, Aug 22, 2020 at 23:21PM, Vladimir Oltean <olteanv@gmail.com> wrote: > > Yes, definitely bloatware from the old days. I think this driver needs the existing > > device tree bindings rethought a little bit before mindlessly porting them to > > ACPI. > > Could you give more details? > > Best Regards > Qiang Zhao Yes. This driver has some device tree bindings. Some thought need to be given as to which one of those is necessary for a functional ACPI setup, and which one isn't. For example: - fsl,spi-cs-sck-delay and fsl,spi-sck-cs-delay are many times necessary. I don't see an attempt to read something equivalent to those in this patch, or to do something about those, otherwise, in case a peripheral needs special treatment. If we want to do something like e.g. deprecate these bindings and just set up a large enough CS-to-SCK and SCK-to-CS delay to make every peripheral happy, in order to not carry this binding over to ACPI, at least we should establish that and do it now, so that the DT code can benefit from that as well. - The bus-num property was made optional by Sascha Hauer in commit 29d2daf2c33c ("spi: spi-fsl-dspi: Make bus-num property optional"). I think this is because he couldn't just remove it completely. But that doesn't mean we should carry it over to ACPI. The SPI core should know to allocate a bus_num dynamically (using IDR, or by looking at aliases) if we just set spi->bus_num = -1. - The spi-num-chipselects can be deduced from compatible string and bus number, and therefore we can avoid carrying it over to ACPI. But again, DT should have this logic first, and then ACPI can be added. - The compatible string plays an integral part in the functionality of the spi-fsl-dspi driver. I want to see a solution for ACPI where the driver knows on which SoC it's running on. Otherwise it doesn't know what are the silicon parameters of the DSPI module (XSPI present or not, DMA present or not, FIFO depth). I don't see that now. I just see something hardcoded for: { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], } Thanks, -Vladimir
On Wed, Aug 26, 2020 at 02:47:58PM +0300, Vladimir Oltean wrote: > - The compatible string plays an integral part in the functionality of > the spi-fsl-dspi driver. I want to see a solution for ACPI where the > driver knows on which SoC it's running on. Otherwise it doesn't know > what are the silicon parameters of the DSPI module (XSPI present or > not, DMA present or not, FIFO depth). I don't see that now. I just see > something hardcoded for: > { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], } Based on some other stuff I've seen with ACPI on NXP stuff it looks like they're following the same scheme but only caring about that one SoC for the time being.
On Wed, Aug 26, 2020 at 03:23:12PM +0100, Mark Brown wrote: > On Wed, Aug 26, 2020 at 02:47:58PM +0300, Vladimir Oltean wrote: > > > - The compatible string plays an integral part in the functionality of > > the spi-fsl-dspi driver. I want to see a solution for ACPI where the > > driver knows on which SoC it's running on. Otherwise it doesn't know > > what are the silicon parameters of the DSPI module (XSPI present or > > not, DMA present or not, FIFO depth). I don't see that now. I just see > > something hardcoded for: > > { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], } > > Based on some other stuff I've seen with ACPI on NXP stuff it looks like > they're following the same scheme but only caring about that one SoC for > the time being. So, no argument about caring only about ACPI on one particular SoC for the time being, but there's a big difference between a solution that works for N=1 and one that works for N=2... Showing my ignorance here, but is there something equivalent to of_machine_is_compatible() for ACPI? Thanks, -Vladimir
> -----Original Message----- > From: Vladimir Oltean <olteanv@gmail.com> > Sent: Wednesday, August 26, 2020 8:18 PM > To: Mark Brown <broonie@kernel.org> > Cc: Qiang Zhao <qiang.zhao@nxp.com>; kuldip dwivedi > <kuldip.dwivedi@puresoftware.com>; linux-spi@vger.kernel.org; linux- > kernel@vger.kernel.org; Pankaj Bansal <pankaj.bansal@nxp.com>; Varun Sethi > <V.Sethi@nxp.com>; Tanveer Alam <tanveer.alam@puresoftware.com> > Subject: Re: [PATCH] spi: spi-fsl-dspi: Add ACPI support > > On Wed, Aug 26, 2020 at 03:23:12PM +0100, Mark Brown wrote: > > On Wed, Aug 26, 2020 at 02:47:58PM +0300, Vladimir Oltean wrote: > > > > > - The compatible string plays an integral part in the functionality of > > > the spi-fsl-dspi driver. I want to see a solution for ACPI where the > > > driver knows on which SoC it's running on. Otherwise it doesn't know > > > what are the silicon parameters of the DSPI module (XSPI present or > > > not, DMA present or not, FIFO depth). I don't see that now. I just see > > > something hardcoded for: > > > { "NXP0005", .driver_data = > > > (kernel_ulong_t)&devtype_data[LS2085A], } > > > > Based on some other stuff I've seen with ACPI on NXP stuff it looks > > like they're following the same scheme but only caring about that one > > SoC for the time being. > > So, no argument about caring only about ACPI on one particular SoC for the time > being, but there's a big difference between a solution that works for N=1 and one > that works for N=2... > > Showing my ignorance here, but is there something equivalent to > of_machine_is_compatible() for ACPI? Just a query, Can't we use meaningful HID for different SoC just like compatible strings in DT ? In this way Silicon parameters can also be added in fsl_dspi_devtype_data structure , which is already exist in driver > > Thanks, > -Vladimir
On Wed, Aug 26, 2020 at 08:43:20PM +0530, Kuldip Dwivedi wrote: > Just a query, Can't we use meaningful HID for different SoC just like > compatible strings in DT ? > In this way Silicon parameters can also be added in > fsl_dspi_devtype_data structure , which is already exist in driver I don't know, is that the preferred way? I don't even know if NXP0005 is made up or if it's written down somewhere in the PNP ID registry. NXP0006 seems to be assigned to the MDIO controller already, so the list of _HID values for the DSPI controller would be discontiguous at best, as well as ever-growing. Again, I'm just raising the concern, if somebody comes in and declares that as "not a problem", then ok. In the ACPI spec there's also a _HRV (Hardware Revision) object, which comes as a simple DWORD. We could use acpi_evaluate_integer() to read that, and use it as index into the array of fsl_dspi_devtype_data, if we declare that as ABI within the driver (and new SoCs would be added only at the end of the enum). Then we could use the NXP0005 _HID for everything DSPI. Again, maybe somebody could chime in and guide us on what's preferable. Thanks, -Vladimir
On Wed, Aug 26, 2020 at 05:47:44PM +0300, Vladimir Oltean wrote: > On Wed, Aug 26, 2020 at 03:23:12PM +0100, Mark Brown wrote: > > On Wed, Aug 26, 2020 at 02:47:58PM +0300, Vladimir Oltean wrote: > > > { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], } > > Based on some other stuff I've seen with ACPI on NXP stuff it looks like > > they're following the same scheme but only caring about that one SoC for > > the time being. > So, no argument about caring only about ACPI on one particular SoC for > the time being, but there's a big difference between a solution that > works for N=1 and one that works for N=2... > Showing my ignorance here, but is there something equivalent to > of_machine_is_compatible() for ACPI? The NXP0005 is the ACPI equivalent of a compatible (comprehensibility is not ACPI's forte) and they're tying driver data to it there.
On Wed, Aug 26, 2020 at 07:09:50PM +0300, Vladimir Oltean wrote: > I don't even know if NXP0005 is made up or if it's written down > somewhere in the PNP ID registry. NXP0006 seems to be assigned to the Well, any ID is made up to some extent. I am concerned about the allocation of IDs too, it needs to be coordinated with NXP since it's being allocated from their namespace, but unfortunately in general there's no sensible way to go from a chip/feature name to an ACPI name due to ACPI's specification mechanism. There's also not any kind of central list of IDs. > In the ACPI spec there's also a _HRV (Hardware Revision) object, which > comes as a simple DWORD. We could use acpi_evaluate_integer() to read > that, and use it as index into the array of fsl_dspi_devtype_data, if > we declare that as ABI within the driver (and new SoCs would be added > only at the end of the enum). Then we could use the NXP0005 _HID for > everything DSPI. That's not something that it's particularly idiomatic to actually use in ACPI and you end up with the same namespacing problem assigning IDs so I'm not sure it makes life any better.
On Wed, Aug 26, 2020 at 06:02:05PM +0100, Mark Brown wrote: > On Wed, Aug 26, 2020 at 07:09:50PM +0300, Vladimir Oltean wrote: > > > I don't even know if NXP0005 is made up or if it's written down > > somewhere in the PNP ID registry. NXP0006 seems to be assigned to the > > Well, any ID is made up to some extent. I am concerned about the > allocation of IDs too, it needs to be coordinated with NXP since it's > being allocated from their namespace, but unfortunately in general > there's no sensible way to go from a chip/feature name to an ACPI name > due to ACPI's specification mechanism. There's also not any kind of > central list of IDs. > > > In the ACPI spec there's also a _HRV (Hardware Revision) object, which > > comes as a simple DWORD. We could use acpi_evaluate_integer() to read > > that, and use it as index into the array of fsl_dspi_devtype_data, if > > we declare that as ABI within the driver (and new SoCs would be added > > only at the end of the enum). Then we could use the NXP0005 _HID for > > everything DSPI. > > That's not something that it's particularly idiomatic to actually use in > ACPI and you end up with the same namespacing problem assigning IDs so > I'm not sure it makes life any better. So what's the idiomatic thing to do in this case, allocate the first free PNP ID now for DSPI controller on LS2085A, then another one for DSPI on LX2160A later, etc etc? Thanks, -Vladimir
On Wed, Aug 26, 2020 at 05:55:52PM +0100, Mark Brown wrote: > On Wed, Aug 26, 2020 at 05:47:44PM +0300, Vladimir Oltean wrote: > > On Wed, Aug 26, 2020 at 03:23:12PM +0100, Mark Brown wrote: > > > On Wed, Aug 26, 2020 at 02:47:58PM +0300, Vladimir Oltean wrote: > > > > > { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], } > > > > Based on some other stuff I've seen with ACPI on NXP stuff it looks like > > > they're following the same scheme but only caring about that one SoC for > > > the time being. > > > So, no argument about caring only about ACPI on one particular SoC for > > the time being, but there's a big difference between a solution that > > works for N=1 and one that works for N=2... > > > Showing my ignorance here, but is there something equivalent to > > of_machine_is_compatible() for ACPI? > > The NXP0005 is the ACPI equivalent of a compatible (comprehensibility is > not ACPI's forte) and they're tying driver data to it there. Where I was trying to get here is that we could have a single _HID for the DSPI controller, and corroborate that with the ACPI equivalent of of_machine_is_compatible("fsl,ls2085a") at driver probe time before assigning the driver data. Thanks, -Vladimir
On Wed, Aug 26, 2020 at 09:30:44PM +0300, Vladimir Oltean wrote: > On Wed, Aug 26, 2020 at 06:02:05PM +0100, Mark Brown wrote: > > That's not something that it's particularly idiomatic to actually use in > > ACPI and you end up with the same namespacing problem assigning IDs so > > I'm not sure it makes life any better. > So what's the idiomatic thing to do in this case, allocate the first > free PNP ID now for DSPI controller on LS2085A, then another one for > DSPI on LX2160A later, etc etc? AFAICT yes, assuming you don't make it look like a PCI device and enumerate that way which is more how these things more normally end up getting done.
On Wed, Aug 26, 2020 at 09:33:38PM +0300, Vladimir Oltean wrote: > On Wed, Aug 26, 2020 at 05:55:52PM +0100, Mark Brown wrote: > > > On Wed, Aug 26, 2020 at 03:23:12PM +0100, Mark Brown wrote: > > > Showing my ignorance here, but is there something equivalent to > > > of_machine_is_compatible() for ACPI? > > The NXP0005 is the ACPI equivalent of a compatible (comprehensibility is > > not ACPI's forte) and they're tying driver data to it there. > Where I was trying to get here is that we could have a single _HID for > the DSPI controller, and corroborate that with the ACPI equivalent of > of_machine_is_compatible("fsl,ls2085a") at driver probe time before > assigning the driver data. Oh, there isn't an ACPI equivalent of that. DMI information doesn't capture anything useful about the chipset, only the system as a whole (assuming it's usefully filled). See for example sound/soc/intel/common/soc-intel-quirks.h
On Sat, Aug 22, 2020 at 9:37 PM Vladimir Oltean <olteanv@gmail.com> wrote: > On Fri, Aug 21, 2020 at 06:40:29PM +0530, kuldip dwivedi wrote: > Just noticed this now. > So for device tree, spi-fsl-dspi supports the following compatibles: > > fsl,vf610-dspi > fsl,ls1021a-v1.0-dspi > fsl,ls1012a-dspi > fsl,ls1028a-dspi > fsl,ls1043a-dspi > fsl,ls1046a-dspi > fsl,ls2080a-dspi > fsl,ls2085a-dspi > fsl,lx2160a-dspi > > Depending on the compatible string, the driver knows whether to use DMA > or XSPI mode, and what the FIFO size is. > > Now, of course not all of the above SoCs are going to support ACPI, but > it is reasonable to expect that more than one will. And in that case, > the driver should still be able to know on what SoC it's running, > because for example LS1043A doesn't support DMA, and LS2085A doesn't > support XSPI. > > How is this dealt with in ACPI? Theoretically you may declare your HID in the same / similar way as PRP0001 and use same compatible strings and all other DT properties (when they make sense and not duplicate ACPI functionality). But better if ACPI people can tell you (I Cc'ed Rafael and ACPI mailing list) if it is gonna work.
On Wed, Aug 26, 2020 at 10:34 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, Aug 22, 2020 at 9:37 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Fri, Aug 21, 2020 at 06:40:29PM +0530, kuldip dwivedi wrote: > > > Just noticed this now. > > So for device tree, spi-fsl-dspi supports the following compatibles: > > > > fsl,vf610-dspi > > fsl,ls1021a-v1.0-dspi > > fsl,ls1012a-dspi > > fsl,ls1028a-dspi > > fsl,ls1043a-dspi > > fsl,ls1046a-dspi > > fsl,ls2080a-dspi > > fsl,ls2085a-dspi > > fsl,lx2160a-dspi > > > > Depending on the compatible string, the driver knows whether to use DMA > > or XSPI mode, and what the FIFO size is. FIFO size can be read from the property (or better if you can derive it directly from HW, like DesignWare does). DMA is just defined by FixedDMA resources (your platform with DMA provides them, otherwise no such resources). > > Now, of course not all of the above SoCs are going to support ACPI, but > > it is reasonable to expect that more than one will. And in that case, > > the driver should still be able to know on what SoC it's running, > > because for example LS1043A doesn't support DMA, and LS2085A doesn't > > support XSPI. > > > > How is this dealt with in ACPI? > > Theoretically you may declare your HID in the same / similar way as > PRP0001 and use same compatible strings and all other DT properties > (when they make sense and not duplicate ACPI functionality). > But better if ACPI people can tell you (I Cc'ed Rafael and ACPI > mailing list) if it is gonna work.
On Wed, Aug 26, 2020 at 10:36:15PM +0300, Andy Shevchenko wrote: > On Wed, Aug 26, 2020 at 10:34 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Sat, Aug 22, 2020 at 9:37 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > > On Fri, Aug 21, 2020 at 06:40:29PM +0530, kuldip dwivedi wrote: > > > > > Just noticed this now. > > > So for device tree, spi-fsl-dspi supports the following compatibles: > > > > > > fsl,vf610-dspi > > > fsl,ls1021a-v1.0-dspi > > > fsl,ls1012a-dspi > > > fsl,ls1028a-dspi > > > fsl,ls1043a-dspi > > > fsl,ls1046a-dspi > > > fsl,ls2080a-dspi > > > fsl,ls2085a-dspi > > > fsl,lx2160a-dspi > > > > > > Depending on the compatible string, the driver knows whether to use DMA > > > or XSPI mode, and what the FIFO size is. > > FIFO size can be read from the property My personal preference is for the driver to hold the expert information about the hardware parameters, and not the device tree. Today you need to know only about this set of parameters, tomorrow you need something new, but you also need to work with old DT blobs... It's a mess. > (or better if you can derive it directly from HW, like DesignWare > does). Nope, can't do that. This IP block doesn't even have an ID or revision register. > DMA is just defined by FixedDMA resources (your platform with DMA > provides them, otherwise no such resources). > This is a case of knowing that tomatoes are fruit, but being wise enough to not put them in a fruit salad. I would not be happy to see this driver make the decision to use DMA based just on the presence of DMA channels in the firmware blob. On some platforms, there are DMA channels but due to an erratum they don't work. While on other platforms, using the DMA channels would simply cause a performance downgrade. Same thing about interrupts, in fact. The firmware blob tells the driver what the interrupt line is, but it doesn't mean that the driver has to use it. Thanks, -Vladimir
On Wed, Aug 26, 2020 at 10:34:04PM +0300, Andy Shevchenko wrote: > On Sat, Aug 22, 2020 at 9:37 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Fri, Aug 21, 2020 at 06:40:29PM +0530, kuldip dwivedi wrote: > > > Just noticed this now. > > So for device tree, spi-fsl-dspi supports the following compatibles: > > > > fsl,vf610-dspi > > fsl,ls1021a-v1.0-dspi > > fsl,ls1012a-dspi > > fsl,ls1028a-dspi > > fsl,ls1043a-dspi > > fsl,ls1046a-dspi > > fsl,ls2080a-dspi > > fsl,ls2085a-dspi > > fsl,lx2160a-dspi > > > > Depending on the compatible string, the driver knows whether to use DMA > > or XSPI mode, and what the FIFO size is. > > > > Now, of course not all of the above SoCs are going to support ACPI, but > > it is reasonable to expect that more than one will. And in that case, > > the driver should still be able to know on what SoC it's running, > > because for example LS1043A doesn't support DMA, and LS2085A doesn't > > support XSPI. > > > > How is this dealt with in ACPI? > > Theoretically you may declare your HID in the same / similar way as > PRP0001 and use same compatible strings and all other DT properties > (when they make sense and not duplicate ACPI functionality). > But better if ACPI people can tell you (I Cc'ed Rafael and ACPI > mailing list) if it is gonna work. > Something doesn't look right about PRP0001, what's the catch?
On Wed, Aug 26, 2020 at 10:56:49PM +0300, Vladimir Oltean wrote: > On Wed, Aug 26, 2020 at 10:36:15PM +0300, Andy Shevchenko wrote: > > FIFO size can be read from the property > My personal preference is for the driver to hold the expert information > about the hardware parameters, and not the device tree. Today you need > to know only about this set of parameters, tomorrow you need something > new, but you also need to work with old DT blobs... It's a mess. I strongly agree, it is much better to just know what the hardware is and set any properties based off that so we don't need to update firmwares to take advantage of additional features or fix quirks that are discovered.
On Wed, Aug 26, 2020 at 11:41:08PM +0300, Vladimir Oltean wrote: > On Wed, Aug 26, 2020 at 10:34:04PM +0300, Andy Shevchenko wrote: > > Theoretically you may declare your HID in the same / similar way as > > PRP0001 and use same compatible strings and all other DT properties > > (when they make sense and not duplicate ACPI functionality). > > But better if ACPI people can tell you (I Cc'ed Rafael and ACPI > > mailing list) if it is gonna work. > Something doesn't look right about PRP0001, what's the catch? Microsoft decided not to implement support for it in Windows, it's essentially there for embedded style x86 platforms running Linux so they don't need to reimplement so many wheels and can just reuse existing DT bindings but it causes problems if you want to run Windows (and possibly some of the enterprise Linux distros, I can't remember if any of them had concerns about it) on the platform.
On Wed, Aug 26, 2020 at 09:45:47PM +0100, Mark Brown wrote: > On Wed, Aug 26, 2020 at 11:41:08PM +0300, Vladimir Oltean wrote: > > On Wed, Aug 26, 2020 at 10:34:04PM +0300, Andy Shevchenko wrote: > > > > Theoretically you may declare your HID in the same / similar way as > > > PRP0001 and use same compatible strings and all other DT properties > > > (when they make sense and not duplicate ACPI functionality). > > > But better if ACPI people can tell you (I Cc'ed Rafael and ACPI > > > mailing list) if it is gonna work. > > > Something doesn't look right about PRP0001, what's the catch? > > Microsoft decided not to implement support for it in Windows, it's > essentially there for embedded style x86 platforms running Linux so they > don't need to reimplement so many wheels and can just reuse existing DT > bindings but it causes problems if you want to run Windows (and possibly > some of the enterprise Linux distros, I can't remember if any of them > had concerns about it) on the platform. So if a silicon vendor doesn't care about Windows, what incentive does it have to even register an official ACPI/PNP ID for its devices?
On Thu, Aug 27, 2020 at 12:06:57AM +0300, Vladimir Oltean wrote: > On Wed, Aug 26, 2020 at 09:45:47PM +0100, Mark Brown wrote: > > On Wed, Aug 26, 2020 at 11:41:08PM +0300, Vladimir Oltean wrote: > > > Something doesn't look right about PRP0001, what's the catch? > > Microsoft decided not to implement support for it in Windows, it's > > essentially there for embedded style x86 platforms running Linux so they > > don't need to reimplement so many wheels and can just reuse existing DT > > bindings but it causes problems if you want to run Windows (and possibly > > some of the enterprise Linux distros, I can't remember if any of them > > had concerns about it) on the platform. > So if a silicon vendor doesn't care about Windows, what incentive does > it have to even register an official ACPI/PNP ID for its devices? Not that there's any registration process or anything, there's some namespacing but that's it, but the main thing would just be keeping the ACPI bindings and DT bindings separate. ACPI has some strong opinions on how systems are built and described so while you can use the PRP0001 stuff to parse DT bindings on an ACPI system it doesn't alway fit well, and there are some things where you just plain shouldn't use PRP0001 since the ACPI and DT models for that sort of device diverge so strongly.
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index 91c6affe139c..7100a8a0a30e 100644 --- a/drivers/spi/spi-fsl-dspi.c +++ b/drivers/spi/spi-fsl-dspi.c @@ -2,10 +2,12 @@ // // Copyright 2013 Freescale Semiconductor, Inc. // Copyright 2020 NXP +// Copyright 2020 Puresoftware Ltd. // // Freescale DSPI driver // This file contains a driver for the Freescale DSPI +#include <linux/acpi.h> #include <linux/clk.h> #include <linux/delay.h> #include <linux/dmaengine.h> @@ -1070,6 +1072,12 @@ static void dspi_cleanup(struct spi_device *spi) kfree(chip); } +static const struct acpi_device_id fsl_dspi_acpi_ids[] = { + { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], }, + {}, +}; +MODULE_DEVICE_TABLE(acpi, fsl_dspi_acpi_ids); + static const struct of_device_id fsl_dspi_dt_ids[] = { { .compatible = "fsl,vf610-dspi", @@ -1272,6 +1280,7 @@ static int dspi_probe(struct platform_device *pdev) struct resource *res; void __iomem *base; bool big_endian; + u32 clk_rate; ctlr = spi_alloc_master(&pdev->dev, sizeof(struct fsl_dspi)); if (!ctlr) @@ -1300,20 +1309,41 @@ static int dspi_probe(struct platform_device *pdev) big_endian = true; } else { - ret = of_property_read_u32(np, "spi-num-chipselects", &cs_num); + if (is_acpi_node(pdev->dev.fwnode)) + ret = device_property_read_u32(&pdev->dev, + "spi-num-chipselects", &cs_num); + else + ret = of_property_read_u32(np, + "spi-num-chipselects", &cs_num); if (ret < 0) { dev_err(&pdev->dev, "can't get spi-num-chipselects\n"); goto out_ctlr_put; } ctlr->num_chipselect = cs_num; - of_property_read_u32(np, "bus-num", &bus_num); + if (is_acpi_node(pdev->dev.fwnode)) { + ret = device_property_read_u32(&pdev->dev, + "bus-num", &bus_num); + if (ret < 0) { + dev_err(&pdev->dev, "can't get bus-num\n"); + goto out_ctlr_put; + } + } else { + of_property_read_u32(np, "bus-num", &bus_num); + } ctlr->bus_num = bus_num; - if (of_property_read_bool(np, "spi-slave")) - ctlr->slave = true; + if (!is_acpi_node(pdev->dev.fwnode)) { + if (of_property_read_bool(np, "spi-slave")) + ctlr->slave = true; + } + + if (is_acpi_node(pdev->dev.fwnode)) + dspi->devtype_data = device_get_match_data(&pdev->dev); + else + dspi->devtype_data = + of_device_get_match_data(&pdev->dev); - dspi->devtype_data = of_device_get_match_data(&pdev->dev); if (!dspi->devtype_data) { dev_err(&pdev->dev, "can't get devtype_data\n"); ret = -EFAULT; @@ -1367,15 +1397,18 @@ static int dspi_probe(struct platform_device *pdev) } } - dspi->clk = devm_clk_get(&pdev->dev, "dspi"); - if (IS_ERR(dspi->clk)) { - ret = PTR_ERR(dspi->clk); - dev_err(&pdev->dev, "unable to get clock\n"); - goto out_ctlr_put; + if (!is_acpi_node(pdev->dev.fwnode)) { + dspi->clk = devm_clk_get(&pdev->dev, "dspi"); + if (IS_ERR(dspi->clk)) { + ret = PTR_ERR(dspi->clk); + dev_err(&pdev->dev, "unable to get clock\n"); + goto out_ctlr_put; + } + + ret = clk_prepare_enable(dspi->clk); + if (ret) + goto out_ctlr_put; } - ret = clk_prepare_enable(dspi->clk); - if (ret) - goto out_ctlr_put; ret = dspi_init(dspi); if (ret) @@ -1408,8 +1441,21 @@ static int dspi_probe(struct platform_device *pdev) } } - ctlr->max_speed_hz = - clk_get_rate(dspi->clk) / dspi->devtype_data->max_clock_factor; + if (is_acpi_node(pdev->dev.fwnode)) { + ret = device_property_read_u32(&pdev->dev, + "clock-frequency", &clk_rate); + if (ret < 0) { + dev_err(&pdev->dev, "can't get clock-frequency\n"); + goto out_ctlr_put; + } + + ctlr->max_speed_hz = + clk_rate / dspi->devtype_data->max_clock_factor; + } else { + clk_rate = clk_get_rate(dspi->clk); + ctlr->max_speed_hz = + clk_rate / dspi->devtype_data->max_clock_factor; + } if (dspi->devtype_data->trans_mode != DSPI_DMA_MODE) ctlr->ptp_sts_supported = true; @@ -1465,13 +1511,14 @@ static void dspi_shutdown(struct platform_device *pdev) } static struct platform_driver fsl_dspi_driver = { - .driver.name = DRIVER_NAME, - .driver.of_match_table = fsl_dspi_dt_ids, - .driver.owner = THIS_MODULE, - .driver.pm = &dspi_pm, - .probe = dspi_probe, - .remove = dspi_remove, - .shutdown = dspi_shutdown, + .driver.name = DRIVER_NAME, + .driver.of_match_table = fsl_dspi_dt_ids, + .driver.acpi_match_table = ACPI_PTR(fsl_dspi_acpi_ids), + .driver.owner = THIS_MODULE, + .driver.pm = &dspi_pm, + .probe = dspi_probe, + .remove = dspi_remove, + .shutdown = dspi_shutdown, }; module_platform_driver(fsl_dspi_driver);