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 <[email protected]>
Signed-off-by: kuldip dwivedi <[email protected]>
---
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
@@ -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);
--
2.17.1
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 <[email protected]>
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/[email protected]
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: Friday, August 21, 2020 7:37 PM
> To: kuldip dwivedi <[email protected]>
> Cc: [email protected]; [email protected]; Qiang Zhao
> <[email protected]>; Pankaj Bansal <[email protected]>; Varun Sethi
> <[email protected]>; tanveer <[email protected]>
> 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 <[email protected]>
> > Sent: Friday, August 21, 2020 7:37 PM
> > To: kuldip dwivedi <[email protected]>
> > Cc: [email protected]; [email protected]; Qiang Zhao
> > <[email protected]>; Pankaj Bansal <[email protected]>; Varun Sethi
> > <[email protected]>; tanveer <[email protected]>
> > 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 <[email protected]>
> Signed-off-by: kuldip dwivedi <[email protected]>
> ---
> 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 <[email protected]> wrote:
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: 2020??8??24?? 19:25
> To: Vladimir Oltean <[email protected]>
> Cc: kuldip dwivedi <[email protected]>;
> [email protected]; [email protected]; Qiang Zhao
> <[email protected]>; Pankaj Bansal <[email protected]>; Varun Sethi
> <[email protected]>; Tanveer Alam <[email protected]>
> 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 Sat, Aug 22, 2020 at 23:21PM, Vladimir Oltean <[email protected]> wrote:
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: 2020??8??22?? 23:21
> To: kuldip dwivedi <[email protected]>
> Cc: Mark Brown <[email protected]>; [email protected];
> [email protected]; Qiang Zhao <[email protected]>; Pankaj
> Bansal <[email protected]>; Varun Sethi <[email protected]>; Tanveer
> Alam <[email protected]>
> 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 <[email protected]>
> > > Sent: Friday, August 21, 2020 7:37 PM
> > > To: kuldip dwivedi <[email protected]>
> > > Cc: [email protected]; [email protected]; Qiang
> > > Zhao <[email protected]>; Pankaj Bansal <[email protected]>;
> > > Varun Sethi <[email protected]>; tanveer
> > > <[email protected]>
> > > 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 08:19:41AM +0000, Qiang Zhao wrote:
> On Mon, Aug 24, 2020 at 19:25, Mark Brown <[email protected]> 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 Wed, Aug 26, 2020 at 11:10:49AM +0000, Qiang Zhao wrote:
> On Sat, Aug 22, 2020 at 23:21PM, Vladimir Oltean <[email protected]> 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 <[email protected]>
> Sent: Wednesday, August 26, 2020 8:18 PM
> To: Mark Brown <[email protected]>
> Cc: Qiang Zhao <[email protected]>; kuldip dwivedi
> <[email protected]>; [email protected]; linux-
> [email protected]; Pankaj Bansal <[email protected]>; Varun
Sethi
> <[email protected]>; Tanveer Alam <[email protected]>
> 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 <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On Wed, Aug 26, 2020 at 10:34 PM Andy Shevchenko
<[email protected]> wrote:
> On Sat, Aug 22, 2020 at 9:37 PM Vladimir Oltean <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On Wed, Aug 26, 2020 at 10:36:15PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 26, 2020 at 10:34 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Sat, Aug 22, 2020 at 9:37 PM Vladimir Oltean <[email protected]> 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 <[email protected]> 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.