Currently NXP fspi driver has support of DT only. Adding ACPI
support to the driver so that it can be used by UEFI firmware
booting in ACPI mode. This driver will be probed if any firmware
will expose HID "NXP0009" in DSDT table.
Signed-off-by: kuldip dwivedi <[email protected]>
---
Notes:
1. Add ACPI match table
2. Change the DT specific APIs to device property APIs
so that same API can be used in DT and ACPi mode.
3. Omit clock configuration part - in ACPI world, the firmware
is responsible for clock maintenance.
4. This patch is tested on LX2160A platform
drivers/spi/spi-nxp-fspi.c | 66 +++++++++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 19 deletions(-)
diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index 1ccda82da206..acdb186ddfb2 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -3,7 +3,8 @@
/*
* NXP FlexSPI(FSPI) controller driver.
*
- * Copyright 2019 NXP.
+ * Copyright 2019-2020 NXP
+ * Copyright 2020 Puresoftware Ltd.
*
* FlexSPI is a flexsible SPI host controller which supports two SPI
* channels and up to 4 external devices. Each channel supports
@@ -30,6 +31,7 @@
* Frieder Schrempf <[email protected]>
*/
+#include <linux/acpi.h>
#include <linux/bitops.h>
#include <linux/clk.h>
#include <linux/completion.h>
@@ -563,6 +565,9 @@ static int nxp_fspi_clk_prep_enable(struct nxp_fspi *f)
{
int ret;
+ if (is_acpi_node(f->dev->fwnode))
+ return 0;
+
ret = clk_prepare_enable(f->clk_en);
if (ret)
return ret;
@@ -576,10 +581,15 @@ static int nxp_fspi_clk_prep_enable(struct nxp_fspi *f)
return 0;
}
-static void nxp_fspi_clk_disable_unprep(struct nxp_fspi *f)
+static int nxp_fspi_clk_disable_unprep(struct nxp_fspi *f)
{
+ if (is_acpi_node(f->dev->fwnode))
+ return 0;
+
clk_disable_unprepare(f->clk);
clk_disable_unprepare(f->clk_en);
+
+ return 0;
}
/*
@@ -900,6 +910,8 @@ static int nxp_fspi_default_setup(struct nxp_fspi *f)
return ret;
/* Reset the module */
+ fspi_writel(f, FSPI_MCR0_SWRST, (base + FSPI_MCR0));
+
/* w1c register, wait unit clear */
ret = fspi_readl_poll_tout(f, f->iobase + FSPI_MCR0,
FSPI_MCR0_SWRST, 0, POLL_TOUT, false);
@@ -1001,7 +1013,7 @@ static int nxp_fspi_probe(struct platform_device *pdev)
f = spi_controller_get_devdata(ctlr);
f->dev = dev;
- f->devtype_data = of_device_get_match_data(dev);
+ f->devtype_data = device_get_match_data(dev);
if (!f->devtype_data) {
ret = -ENODEV;
goto err_put_ctrl;
@@ -1011,6 +1023,8 @@ static int nxp_fspi_probe(struct platform_device *pdev)
/* find the resources - configuration register address space */
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "fspi_base");
+ if (!res)
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
f->iobase = devm_ioremap_resource(dev, res);
if (IS_ERR(f->iobase)) {
ret = PTR_ERR(f->iobase);
@@ -1020,8 +1034,11 @@ static int nxp_fspi_probe(struct platform_device *pdev)
/* find the resources - controller memory mapped space */
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "fspi_mmap");
if (!res) {
- ret = -ENODEV;
- goto err_put_ctrl;
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res) {
+ ret = -ENODEV;
+ goto err_put_ctrl;
+ }
}
/* assign memory mapped starting address and mapped size. */
@@ -1029,22 +1046,24 @@ static int nxp_fspi_probe(struct platform_device *pdev)
f->memmap_phy_size = resource_size(res);
/* find the clocks */
- f->clk_en = devm_clk_get(dev, "fspi_en");
- if (IS_ERR(f->clk_en)) {
- ret = PTR_ERR(f->clk_en);
- goto err_put_ctrl;
- }
+ if (dev_of_node(&pdev->dev)) {
+ f->clk_en = devm_clk_get(dev, "fspi_en");
+ if (IS_ERR(f->clk_en)) {
+ ret = PTR_ERR(f->clk_en);
+ goto err_put_ctrl;
+ }
- f->clk = devm_clk_get(dev, "fspi");
- if (IS_ERR(f->clk)) {
- ret = PTR_ERR(f->clk);
- goto err_put_ctrl;
- }
+ f->clk = devm_clk_get(dev, "fspi");
+ if (IS_ERR(f->clk)) {
+ ret = PTR_ERR(f->clk);
+ goto err_put_ctrl;
+ }
- ret = nxp_fspi_clk_prep_enable(f);
- if (ret) {
- dev_err(dev, "can not enable the clock\n");
- goto err_put_ctrl;
+ ret = nxp_fspi_clk_prep_enable(f);
+ if (ret) {
+ dev_err(dev, "can not enable the clock\n");
+ goto err_put_ctrl;
+ }
}
/* find the irq */
@@ -1127,6 +1146,14 @@ static const struct of_device_id nxp_fspi_dt_ids[] = {
};
MODULE_DEVICE_TABLE(of, nxp_fspi_dt_ids);
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id nxp_fspi_acpi_ids[] = {
+ { "NXP0009", .driver_data = (kernel_ulong_t)&lx2160a_data, },
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, nxp_fspi_acpi_ids);
+#endif
+
static const struct dev_pm_ops nxp_fspi_pm_ops = {
.suspend = nxp_fspi_suspend,
.resume = nxp_fspi_resume,
@@ -1136,6 +1163,7 @@ static struct platform_driver nxp_fspi_driver = {
.driver = {
.name = "nxp-fspi",
.of_match_table = nxp_fspi_dt_ids,
+ .acpi_match_table = ACPI_PTR(nxp_fspi_acpi_ids),
.pm = &nxp_fspi_pm_ops,
},
.probe = nxp_fspi_probe,
--
2.17.1
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: Wednesday, September 9, 2020 5:32 PM
> To: kuldip dwivedi <[email protected]>
> Cc: Ashish Kumar <[email protected]>; Yogesh Gaur
> <[email protected]>; [email protected]; linux-
> [email protected]; Varun Sethi <[email protected]>; Arokia Samy
> <[email protected]>
> Subject: Re: [PATCH v1] spi: spi-nxp-fspi: Add ACPI support
>
> On Tue, Sep 08, 2020 at 11:32:27AM +0530, kuldip dwivedi wrote:
>
> This appears to be v2 not v1?
This is separate Patch so v1 should be OK here. Earlier one was related to
DSPI.
https://lore.kernel.org/linux-spi/[email protected]/T/#t
>
> > Currently NXP fspi driver has support of DT only. Adding ACPI support
> > to the driver so that it can be used by UEFI firmware booting in ACPI
> > mode. This driver will be probed if any firmware will expose HID
> > "NXP0009" in DSDT table.
>
> As I said on your previous version:
>
> | 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, NXP is aware.
>
> Please don't ignore review comments, people are generally making them
for a
> reason and are likely to have the same concerns if issues remain
unaddressed.
> Having to repeat the same comments can get repetitive and make people
question
> the value of time spent reviewing. If you disagree with the review
comments
> that's fine but you need to reply and discuss your concerns so that the
reviewer
> can understand your decisions.
This is new Patch for different IP (FSPI) and scenario is different from
DSPI driver.
>
> > @@ -900,6 +910,8 @@ static int nxp_fspi_default_setup(struct nxp_fspi
*f)
> > return ret;
> >
> > /* Reset the module */
> > + fspi_writel(f, FSPI_MCR0_SWRST, (base + FSPI_MCR0));
> > +
> > /* w1c register, wait unit clear */
> > ret = fspi_readl_poll_tout(f, f->iobase + FSPI_MCR0,
> > FSPI_MCR0_SWRST, 0, POLL_TOUT, false);
>
> Why are you adding this reset? How is it connected to adding ACPI
support - it
> looks like it should be a separate patch.
I observed a kernel panic in setting up the driver, and this fixed the
issue.
Hi Kuldeep Dwivedi,
> -----Original Message-----
> From: kuldip dwivedi <[email protected]>
> Sent: Tuesday, September 8, 2020 11:32 AM
> To: Ashish Kumar <[email protected]>; Yogesh Gaur
> <[email protected]>; Mark Brown <[email protected]>; linux-
> [email protected]; [email protected]
> Cc: Varun Sethi <[email protected]>; Arokia Samy <[email protected]>;
> kuldip dwivedi <[email protected]>
> Subject: [EXT] [PATCH v1] spi: spi-nxp-fspi: Add ACPI support
>
> Caution: EXT Email
>
> Currently NXP fspi driver has support of DT only. Adding ACPI
> support to the driver so that it can be used by UEFI firmware
> booting in ACPI mode. This driver will be probed if any firmware
> will expose HID "NXP0009" in DSDT table.
>
> Signed-off-by: kuldip dwivedi <[email protected]>
Does these change affects non-ACPI FSPI driver in Linux? What test case were run to verify the same?
> ---
>
> Notes:
> 1. Add ACPI match table
> 2. Change the DT specific APIs to device property APIs
> so that same API can be used in DT and ACPi mode.
> 3. Omit clock configuration part - in ACPI world, the firmware
> is responsible for clock maintenance.
> 4. This patch is tested on LX2160A platform
>
> drivers/spi/spi-nxp-fspi.c | 66 +++++++++++++++++++++++++++-----------
> 1 file changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 1ccda82da206..acdb186ddfb2 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
> @@ -3,7 +3,8 @@
> /*
> * NXP FlexSPI(FSPI) controller driver.
> *
> - * Copyright 2019 NXP.
> + * Copyright 2019-2020 NXP
Why Update NXP copyright?
> + * Copyright 2020 Puresoftware Ltd.
> *
> * FlexSPI is a flexsible SPI host controller which supports two SPI
> * channels and up to 4 external devices. Each channel supports
> @@ -30,6 +31,7 @@
> * Frieder Schrempf <[email protected]>
> */
>
> +#include <linux/acpi.h>
> #include <linux/bitops.h>
> #include <linux/clk.h>
> #include <linux/completion.h>
> @@ -563,6 +565,9 @@ static int nxp_fspi_clk_prep_enable(struct nxp_fspi
> *f)
> {
> int ret;
>
> + if (is_acpi_node(f->dev->fwnode))
> + return 0;
> +
> ret = clk_prepare_enable(f->clk_en);
> if (ret)
> return ret;
> @@ -576,10 +581,15 @@ static int nxp_fspi_clk_prep_enable(struct
> nxp_fspi *f)
> return 0;
> }
>
> -static void nxp_fspi_clk_disable_unprep(struct nxp_fspi *f)
> +static int nxp_fspi_clk_disable_unprep(struct nxp_fspi *f)
> {
> + if (is_acpi_node(f->dev->fwnode))
> + return 0;
> +
> clk_disable_unprepare(f->clk);
> clk_disable_unprepare(f->clk_en);
> +
> + return 0;
> }
>
> /*
> @@ -900,6 +910,8 @@ static int nxp_fspi_default_setup(struct nxp_fspi *f)
> return ret;
>
> /* Reset the module */
> + fspi_writel(f, FSPI_MCR0_SWRST, (base + FSPI_MCR0));
> +
Why is this SW reset needed now? This will alter nxp_fspi_resume() function as well.
> /* w1c register, wait unit clear */
> ret = fspi_readl_poll_tout(f, f->iobase + FSPI_MCR0,
> FSPI_MCR0_SWRST, 0, POLL_TOUT, false);
> @@ -1001,7 +1013,7 @@ static int nxp_fspi_probe(struct platform_device
> *pdev)
>
> f = spi_controller_get_devdata(ctlr);
> f->dev = dev;
> - f->devtype_data = of_device_get_match_data(dev);
> + f->devtype_data = device_get_match_data(dev);
> if (!f->devtype_data) {
> ret = -ENODEV;
> goto err_put_ctrl;
> @@ -1011,6 +1023,8 @@ static int nxp_fspi_probe(struct platform_device
> *pdev)
>
> /* find the resources - configuration register address space */
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "fspi_base");
> + if (!res)
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
Why is this needed ?, _byname() will get you fspi_base value.
> f->iobase = devm_ioremap_resource(dev, res);
> if (IS_ERR(f->iobase)) {
> ret = PTR_ERR(f->iobase);
> @@ -1020,8 +1034,11 @@ static int nxp_fspi_probe(struct platform_device
> *pdev)
> /* find the resources - controller memory mapped space */
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "fspi_mmap");
> if (!res) {
> - ret = -ENODEV;
> - goto err_put_ctrl;
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
Why is this needed? _byname() will get you fspi_mmap.
If fspi_mmap is not present then fetch 1st IORESOURE_MEM ?
Regards
Ashish
> + if (!res) {
> + ret = -ENODEV;
> + goto err_put_ctrl;
> + }
> }
>
> /* assign memory mapped starting address and mapped size. */
> @@ -1029,22 +1046,24 @@ static int nxp_fspi_probe(struct platform_device
> *pdev)
> f->memmap_phy_size = resource_size(res);
>
> /* find the clocks */
> - f->clk_en = devm_clk_get(dev, "fspi_en");
> - if (IS_ERR(f->clk_en)) {
> - ret = PTR_ERR(f->clk_en);
> - goto err_put_ctrl;
> - }
> + if (dev_of_node(&pdev->dev)) {
> + f->clk_en = devm_clk_get(dev, "fspi_en");
> + if (IS_ERR(f->clk_en)) {
> + ret = PTR_ERR(f->clk_en);
> + goto err_put_ctrl;
> + }
>
> - f->clk = devm_clk_get(dev, "fspi");
> - if (IS_ERR(f->clk)) {
> - ret = PTR_ERR(f->clk);
> - goto err_put_ctrl;
> - }
> + f->clk = devm_clk_get(dev, "fspi");
> + if (IS_ERR(f->clk)) {
> + ret = PTR_ERR(f->clk);
> + goto err_put_ctrl;
> + }
>
> - ret = nxp_fspi_clk_prep_enable(f);
> - if (ret) {
> - dev_err(dev, "can not enable the clock\n");
> - goto err_put_ctrl;
> + ret = nxp_fspi_clk_prep_enable(f);
> + if (ret) {
> + dev_err(dev, "can not enable the clock\n");
> + goto err_put_ctrl;
> + }
> }
>
> /* find the irq */
> @@ -1127,6 +1146,14 @@ static const struct of_device_id nxp_fspi_dt_ids[]
> = {
> };
> MODULE_DEVICE_TABLE(of, nxp_fspi_dt_ids);
>
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id nxp_fspi_acpi_ids[] = {
> + { "NXP0009", .driver_data = (kernel_ulong_t)&lx2160a_data, },
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, nxp_fspi_acpi_ids);
> +#endif
> +
> static const struct dev_pm_ops nxp_fspi_pm_ops = {
> .suspend = nxp_fspi_suspend,
> .resume = nxp_fspi_resume,
> @@ -1136,6 +1163,7 @@ static struct platform_driver nxp_fspi_driver = {
> .driver = {
> .name = "nxp-fspi",
> .of_match_table = nxp_fspi_dt_ids,
> + .acpi_match_table = ACPI_PTR(nxp_fspi_acpi_ids),
> .pm = &nxp_fspi_pm_ops,
> },
> .probe = nxp_fspi_probe,
> --
> 2.17.1
On Tue, Sep 08, 2020 at 11:32:27AM +0530, kuldip dwivedi wrote:
This appears to be v2 not v1?
> Currently NXP fspi driver has support of DT only. Adding ACPI
> support to the driver so that it can be used by UEFI firmware
> booting in ACPI mode. This driver will be probed if any firmware
> will expose HID "NXP0009" in DSDT table.
As I said on your previous version:
| Does NXP know about this ID assignment from their namespace? ACPI IDs
| should be namespaced by whoever's assigning the ID to avoid collisions.
Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed. Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing. If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.
> @@ -900,6 +910,8 @@ static int nxp_fspi_default_setup(struct nxp_fspi *f)
> return ret;
>
> /* Reset the module */
> + fspi_writel(f, FSPI_MCR0_SWRST, (base + FSPI_MCR0));
> +
> /* w1c register, wait unit clear */
> ret = fspi_readl_poll_tout(f, f->iobase + FSPI_MCR0,
> FSPI_MCR0_SWRST, 0, POLL_TOUT, false);
Why are you adding this reset? How is it connected to adding ACPI
support - it looks like it should be a separate patch.
> -----Original Message-----
> From: Ashish Kumar <[email protected]>
> Sent: Wednesday, September 9, 2020 4:57 PM
> To: kuldip dwivedi <[email protected]>; Yogesh Gaur
> <[email protected]>; Mark Brown <[email protected]>; linux-
> [email protected]; [email protected]
> Cc: Varun Sethi <[email protected]>; Arokia Samy <[email protected]>
> Subject: RE: [EXT] [PATCH v1] spi: spi-nxp-fspi: Add ACPI support
>
> Hi Kuldeep Dwivedi,
Don't mind but It's Kuldip not Kuldeep
>
> > -----Original Message-----
> > From: kuldip dwivedi <[email protected]>
> > Sent: Tuesday, September 8, 2020 11:32 AM
> > To: Ashish Kumar <[email protected]>; Yogesh Gaur
> > <[email protected]>; Mark Brown <[email protected]>; linux-
> > [email protected]; [email protected]
> > Cc: Varun Sethi <[email protected]>; Arokia Samy <[email protected]>;
> > kuldip dwivedi <[email protected]>
> > Subject: [EXT] [PATCH v1] spi: spi-nxp-fspi: Add ACPI support
> >
> > Caution: EXT Email
> >
> > Currently NXP fspi driver has support of DT only. Adding ACPI support
> > to the driver so that it can be used by UEFI firmware booting in ACPI
> > mode. This driver will be probed if any firmware will expose HID
> > "NXP0009" in DSDT table.
> >
> > Signed-off-by: kuldip dwivedi <[email protected]>
> Does these change affects non-ACPI FSPI driver in Linux? What test case
were run
> to verify the same?
I have verified on both DT and ACPI mode with below method. In this method
we
Used mtd utility to write some content and then read from same address.
Address 0x3000000 has been selected to avoid any data loss of existing
UEFI variables.
root@localhost:/mnt# ls
1.c EFI i2c_utils mtd_debug mtdinfo reportfreq64
root@localhost:/mnt# cat 1.c
1234567890987654321
root@localhost:/mnt#
root@localhost:/mnt# ./mtdinfo
Count of MTD devices: 2
Present MTD devices: mtd0, mtd1
Sysfs interface supported: yes
root@localhost:/mnt# ./mtd_debug erase /dev/mtd1 0x3000000 0x1000
Erased 4096 bytes from address 0x03000000 in flash
root@localhost:/mnt# ./mtd_debug write /dev/mtd1 0x3000000 20 1.c
Copied 20 bytes from 1.c to address 0x03000000 in flash
root@lOcalhost:/mnt# ./mtd_debug read /dev/mtd1 0x3000000 20 2.c
Copied 20 bytes from address 0x03000000 in flash tO 2.c
root@localhost:/mnt# cat 2.c
1234567890987654321
> > ---
> >
> > Notes:
> > 1. Add ACPI match table
> > 2. Change the DT specific APIs to device property APIs
> > so that same API can be used in DT and ACPi mode.
> > 3. Omit clock configuration part - in ACPI world, the firmware
> > is responsible for clock maintenance.
> > 4. This patch is tested on LX2160A platform
> >
> > drivers/spi/spi-nxp-fspi.c | 66
> > +++++++++++++++++++++++++++-----------
> > 1 file changed, 47 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> > index 1ccda82da206..acdb186ddfb2 100644
> > --- a/drivers/spi/spi-nxp-fspi.c
> > +++ b/drivers/spi/spi-nxp-fspi.c
> > @@ -3,7 +3,8 @@
> > /*
> > * NXP FlexSPI(FSPI) controller driver.
> > *
> > - * Copyright 2019 NXP.
> > + * Copyright 2019-2020 NXP
> Why Update NXP copyright?
This is asked by NXP only
>
> > + * Copyright 2020 Puresoftware Ltd.
> > *
> > * FlexSPI is a flexsible SPI host controller which supports two SPI
> > * channels and up to 4 external devices. Each channel supports @@
> > -30,6 +31,7 @@
> > * Frieder Schrempf <[email protected]>
> > */
> >
> > +#include <linux/acpi.h>
> > #include <linux/bitops.h>
> > #include <linux/clk.h>
> > #include <linux/completion.h>
> > @@ -563,6 +565,9 @@ static int nxp_fspi_clk_prep_enable(struct
> > nxp_fspi
> > *f)
> > {
> > int ret;
> >
> > + if (is_acpi_node(f->dev->fwnode))
> > + return 0;
> > +
> > ret = clk_prepare_enable(f->clk_en);
> > if (ret)
> > return ret;
> > @@ -576,10 +581,15 @@ static int nxp_fspi_clk_prep_enable(struct
> > nxp_fspi *f)
> > return 0;
> > }
> >
> > -static void nxp_fspi_clk_disable_unprep(struct nxp_fspi *f)
> > +static int nxp_fspi_clk_disable_unprep(struct nxp_fspi *f)
> > {
> > + if (is_acpi_node(f->dev->fwnode))
> > + return 0;
> > +
> > clk_disable_unprepare(f->clk);
> > clk_disable_unprepare(f->clk_en);
> > +
> > + return 0;
> > }
> >
> > /*
> > @@ -900,6 +910,8 @@ static int nxp_fspi_default_setup(struct nxp_fspi
*f)
> > return ret;
> >
> > /* Reset the module */
> > + fspi_writel(f, FSPI_MCR0_SWRST, (base + FSPI_MCR0));
> > +
> Why is this SW reset needed now? This will alter nxp_fspi_resume()
function as
> well.
I observed a kernel panic during setting up the driver in
nxp_fspi_default_setup function in ACPI boot and this is fixed.
>
> > /* w1c register, wait unit clear */
> > ret = fspi_readl_poll_tout(f, f->iobase + FSPI_MCR0,
> > FSPI_MCR0_SWRST, 0, POLL_TOUT,
> > false); @@ -1001,7 +1013,7 @@ static int nxp_fspi_probe(struct
> > platform_device
> > *pdev)
> >
> > f = spi_controller_get_devdata(ctlr);
> > f->dev = dev;
> > - f->devtype_data = of_device_get_match_data(dev);
> > + f->devtype_data = device_get_match_data(dev);
> > if (!f->devtype_data) {
> > ret = -ENODEV;
> > goto err_put_ctrl;
> > @@ -1011,6 +1023,8 @@ static int nxp_fspi_probe(struct platform_device
> > *pdev)
> >
> > /* find the resources - configuration register address space
*/
> > res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "fspi_base");
> > + if (!res)
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> Why is this needed ?, _byname() will get you fspi_base value.
In ACPI we can not pass resource value by name where as we have to pass
By index. So in 0th Index there is FSPI_BASE and in 1st index FSPIMM_BASE.
For reference Please see , Line :23 and Line:24
https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platfor
ms/tree/Platform/NXP/LX2160aRdbPkg/AcpiTables/Dsdt/FSPI.asl?h=LX2160_UEFI_
ACPI_EAR3
>
> > f->iobase = devm_ioremap_resource(dev, res);
> > if (IS_ERR(f->iobase)) {
> > ret = PTR_ERR(f->iobase); @@ -1020,8 +1034,11 @@
> > static int nxp_fspi_probe(struct platform_device
> > *pdev)
> > /* find the resources - controller memory mapped space */
> > res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "fspi_mmap");
> > if (!res) {
> > - ret = -ENODEV;
> > - goto err_put_ctrl;
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> Why is this needed? _byname() will get you fspi_mmap.
> If fspi_mmap is not present then fetch 1st IORESOURE_MEM ?
In ACPI we can not pass resource value by name where as we have to pass
By index. So in 0th Index there is FSPI_BASE and in 1st index FSPIMM_BASE.
For reference Please see , Line :23 and Line:24
https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platfor
ms/tree/Platform/NXP/LX2160aRdbPkg/AcpiTables/Dsdt/FSPI.asl?h=LX2160_UEFI_
ACPI_EAR3
>
> Regards
> Ashish
> > + if (!res) {
> > + ret = -ENODEV;
> > + goto err_put_ctrl;
> > + }
> > }
> >
> > /* assign memory mapped starting address and mapped size. */
> > @@ -1029,22 +1046,24 @@ static int nxp_fspi_probe(struct
> > platform_device
> > *pdev)
> > f->memmap_phy_size = resource_size(res);
> >
> > /* find the clocks */
> > - f->clk_en = devm_clk_get(dev, "fspi_en");
> > - if (IS_ERR(f->clk_en)) {
> > - ret = PTR_ERR(f->clk_en);
> > - goto err_put_ctrl;
> > - }
> > + if (dev_of_node(&pdev->dev)) {
> > + f->clk_en = devm_clk_get(dev, "fspi_en");
> > + if (IS_ERR(f->clk_en)) {
> > + ret = PTR_ERR(f->clk_en);
> > + goto err_put_ctrl;
> > + }
> >
> > - f->clk = devm_clk_get(dev, "fspi");
> > - if (IS_ERR(f->clk)) {
> > - ret = PTR_ERR(f->clk);
> > - goto err_put_ctrl;
> > - }
> > + f->clk = devm_clk_get(dev, "fspi");
> > + if (IS_ERR(f->clk)) {
> > + ret = PTR_ERR(f->clk);
> > + goto err_put_ctrl;
> > + }
> >
> > - ret = nxp_fspi_clk_prep_enable(f);
> > - if (ret) {
> > - dev_err(dev, "can not enable the clock\n");
> > - goto err_put_ctrl;
> > + ret = nxp_fspi_clk_prep_enable(f);
> > + if (ret) {
> > + dev_err(dev, "can not enable the clock\n");
> > + goto err_put_ctrl;
> > + }
> > }
> >
> > /* find the irq */
> > @@ -1127,6 +1146,14 @@ static const struct of_device_id
> > nxp_fspi_dt_ids[] = { }; MODULE_DEVICE_TABLE(of, nxp_fspi_dt_ids);
> >
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id nxp_fspi_acpi_ids[] = {
> > + { "NXP0009", .driver_data = (kernel_ulong_t)&lx2160a_data, },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(acpi, nxp_fspi_acpi_ids); #endif
> > +
> > static const struct dev_pm_ops nxp_fspi_pm_ops = {
> > .suspend = nxp_fspi_suspend,
> > .resume = nxp_fspi_resume,
> > @@ -1136,6 +1163,7 @@ static struct platform_driver nxp_fspi_driver =
{
> > .driver = {
> > .name = "nxp-fspi",
> > .of_match_table = nxp_fspi_dt_ids,
> > + .acpi_match_table = ACPI_PTR(nxp_fspi_acpi_ids),
> > .pm = &nxp_fspi_pm_ops,
> > },
> > .probe = nxp_fspi_probe,
> > --
> > 2.17.1
Thanks,
On Wed, Sep 09, 2020 at 06:29:10PM +0530, Kuldip Dwivedi wrote:
> > | 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, NXP is aware.
Can anyone from NXP confirm this?
> > Please don't ignore review comments, people are generally making them
> for a
> > reason and are likely to have the same concerns if issues remain
> This is new Patch for different IP (FSPI) and scenario is different from
> DSPI driver.
If a generic issue like this exists with one patch you should expect
that exactly the same issue is going to come up with other very similar
patches and therefore ensure they are addressed so people don't feel
like you are ignoring them.
> > > /* Reset the module */
> > > + fspi_writel(f, FSPI_MCR0_SWRST, (base + FSPI_MCR0));
> > > +
> > Why are you adding this reset? How is it connected to adding ACPI
> support - it
> > looks like it should be a separate patch.
> I observed a kernel panic in setting up the driver, and this fixed the
> issue.
At the very least this would need to be called out in the changelog but
like I say it should really be a separate patch.