| Message ID | 20200202125950.1825013-2-aford173@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/5] spi: fspi: enable fspi on imx8qxp and imx8mm | expand |
On Sun, Feb 2, 2020 at 10:00 AM Adam Ford <aford173@gmail.com> wrote: > > From: Han Xu <han.xu@nxp.com> > > Apply patch from NXP upstream repo to > dynamically allocate AHB memory as needed. The commit log could be improved here. What is the motivation for doing this? > + if (!f->ahb_addr) { > + dev_err(f->dev, "failed to alloc memory\n"); There is no need for this error message as the MM core will take care of it.
On Sun, Feb 2, 2020 at 10:39 AM Fabio Estevam <festevam@gmail.com> wrote: > > On Sun, Feb 2, 2020 at 10:00 AM Adam Ford <aford173@gmail.com> wrote: > > > > From: Han Xu <han.xu@nxp.com> > > > > Apply patch from NXP upstream repo to > > dynamically allocate AHB memory as needed. > > The commit log could be improved here. What is the motivation for doing this? My motivation is to get the flexspi on the i.MX8MM to work, and I did a list of the patches applied on the NXP branch to see what was applied on top of their 4.19 kernel and this patch series generated from that list. Most of the NXP commits are one-line commits, and I don't know the motivation for what's happening. NXP did it, and I know it works on the Flexspi driver. Maybe Han Xu can comment, since it's really his patch. adam > > > + if (!f->ahb_addr) { > > + dev_err(f->dev, "failed to alloc memory\n"); > > There is no need for this error message as the MM core will take care of it.
On Mon, Feb 03, 2020 at 04:53:34AM -0600, Adam Ford wrote: > My motivation is to get the flexspi on the i.MX8MM to work, and I did > a list of the patches applied on the NXP branch to see what was > applied on top of their 4.19 kernel and this patch series generated > from that list. Most of the NXP commits are one-line commits, and I > don't know the motivation for what's happening. NXP did it, and I > know it works on the Flexspi driver. Adding new compatibles and so on seems fine but the patches making random changes without explanation like the one for octal mode I just replied to are more worrying, do they work with older versions of the IP or in all use cases for example? I'd suggest cutting the initial patch series down to the bare minimum needed to get things working and then building on top of that if that's not already been done.
On Wed, Feb 12, 2020 at 6:07 AM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Feb 03, 2020 at 04:53:34AM -0600, Adam Ford wrote: > > > My motivation is to get the flexspi on the i.MX8MM to work, and I did > > a list of the patches applied on the NXP branch to see what was > > applied on top of their 4.19 kernel and this patch series generated > > from that list. Most of the NXP commits are one-line commits, and I > > don't know the motivation for what's happening. NXP did it, and I > > know it works on the Flexspi driver. > > Adding new compatibles and so on seems fine but the patches making > random changes without explanation like the one for octal mode I just > replied to are more worrying, do they work with older versions of the IP > or in all use cases for example? I'd suggest cutting the initial patch > series down to the bare minimum needed to get things working and then > building on top of that if that's not already been done. The original author was copied on the initial commit. I literally generated the patch from NXP's branch, added my notes, and pushed them to the mailing lists after testing them on the the Linux master branch. I am a bit disappointed that NXP's author hasn't responded to any of the comments or feedback. NXP knows their hardware and better understands the details as to what is happening and why. In any case, I'll try to scale the patch series back to just enough to get it working on the i.MX8M Mini. I'll expand a bit on the commit message based on what I've learned about the various in-implemented quirks and send a V2 series. adam
On Wed, Feb 12, 2020 at 07:08:49AM -0600, Adam Ford wrote: > The original author was copied on the initial commit. I literally > generated the patch from NXP's branch, added my notes, and pushed > them to the mailing lists after testing them on the the Linux master > branch. I am a bit disappointed that NXP's author hasn't responded > to any of the comments or feedback. NXP knows their hardware and Bear in mind that it's been the spring festival and there's been quite a bit of delay in getting back to work in China resulting from coronavirus stuff so hopefully it's just a delay in replying.
> -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Thursday, February 13, 2020 12:46 PM > To: Adam Ford <aford173@gmail.com> > Cc: Fabio Estevam <festevam@gmail.com>; linux-spi <linux-spi@vger.kernel.org>; > Han Xu <han.xu@nxp.com>; Yogesh Gaur <yogeshgaur.83@gmail.com>; Ashish > Kumar <ashish.kumar@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark > Rutland <mark.rutland@arm.com>; Shawn Guo <shawnguo@kernel.org>; Sascha > Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team > <kernel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>; open list:OPEN > FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > <devicetree@vger.kernel.org>; linux-kernel <linux-kernel@vger.kernel.org>; > moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE <linux-arm- > kernel@lists.infradead.org> > Subject: [EXT] Re: [PATCH V2 2/5] spi: fspi: dynamically alloc AHB memory > > On Wed, Feb 12, 2020 at 07:08:49AM -0600, Adam Ford wrote: > > > The original author was copied on the initial commit. I literally > > generated the patch from NXP's branch, added my notes, and pushed > > them to the mailing lists after testing them on the the Linux master > > branch. I am a bit disappointed that NXP's author hasn't responded > > to any of the comments or feedback. NXP knows their hardware and > > Bear in mind that it's been the spring festival and there's been quite a bit of delay > in getting back to work in China resulting from coronavirus stuff so hopefully it's > just a delay in replying. The FSPI is a shared IP with other NXP BU. We are debugging an issue may related to this patch. I will resend the patch set after the root cause found.
diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c index 00c7899428a1..23abf5ae318e 100644 --- a/drivers/spi/spi-nxp-fspi.c +++ b/drivers/spi/spi-nxp-fspi.c @@ -307,6 +307,7 @@ #define POLL_TOUT 5000 #define NXP_FSPI_MAX_CHIPSELECT 4 +#define NXP_FSPI_MIN_IOMAP SZ_4M struct nxp_fspi_devtype_data { unsigned int rxfifo; @@ -345,6 +346,8 @@ struct nxp_fspi { void __iomem *ahb_addr; u32 memmap_phy; u32 memmap_phy_size; + u32 memmap_start; + u32 memmap_len; struct clk *clk, *clk_en; struct device *dev; struct completion c; @@ -657,12 +660,35 @@ static void nxp_fspi_select_mem(struct nxp_fspi *f, struct spi_device *spi) f->selected = spi->chip_select; } -static void nxp_fspi_read_ahb(struct nxp_fspi *f, const struct spi_mem_op *op) +static int nxp_fspi_read_ahb(struct nxp_fspi *f, const struct spi_mem_op *op) { + u32 start = op->addr.val; u32 len = op->data.nbytes; + /* if necessary, ioremap before AHB read */ + if ((!f->ahb_addr) || start < f->memmap_start || + start + len > f->memmap_start + f->memmap_len) { + if (f->ahb_addr) + iounmap(f->ahb_addr); + + f->memmap_start = start; + f->memmap_len = len > NXP_FSPI_MIN_IOMAP ? + len : NXP_FSPI_MIN_IOMAP; + + f->ahb_addr = ioremap_wc(f->memmap_phy + f->memmap_start, + f->memmap_len); + + if (!f->ahb_addr) { + dev_err(f->dev, "failed to alloc memory\n"); + return -ENOMEM; + } + } + /* Read out the data directly from the AHB buffer. */ - memcpy_fromio(op->data.buf.in, (f->ahb_addr + op->addr.val), len); + memcpy_fromio(op->data.buf.in, + f->ahb_addr + start - f->memmap_start, len); + + return 0; } static void nxp_fspi_fill_txfifo(struct nxp_fspi *f, @@ -822,7 +848,7 @@ static int nxp_fspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) */ if (op->data.nbytes > (f->devtype_data->rxfifo - 4) && op->data.dir == SPI_MEM_DATA_IN) { - nxp_fspi_read_ahb(f, op); + err = nxp_fspi_read_ahb(f, op); } else { if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_OUT) nxp_fspi_fill_txfifo(f, op); @@ -992,9 +1018,8 @@ 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"); - f->ahb_addr = devm_ioremap_resource(dev, res); - if (IS_ERR(f->ahb_addr)) { - ret = PTR_ERR(f->ahb_addr); + if (IS_ERR(res)) { + ret = PTR_ERR(res); goto err_put_ctrl; } @@ -1073,6 +1098,9 @@ static int nxp_fspi_remove(struct platform_device *pdev) mutex_destroy(&f->lock); + if (f->ahb_addr) + iounmap(f->ahb_addr); + return 0; }