Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest
On Wed, Jul 11, 2018 at 06:37:02AM +, Robin Gong wrote:
>
> > -Original Message-
> > From: Vinod [mailto:vk...@kernel.org]
> > Sent: 2018年7月10日 23:33
> > To: Robin Gong
> > Cc: dan.j.willi...@intel.com; shawn...@kernel.org;
> > s.ha...@pengutronix.de; Fabio Estevam ;
> > li...@armlinux.org.uk; linux-arm-ker...@lists.infradead.org;
> > ker...@pengutronix.de; dmaeng...@vger.kernel.org;
> > linux-kernel@vger.kernel.org; dl-linux-imx
> > Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest
> >
> > On 11-07-18, 00:23, Robin Gong wrote:
> > > dmatest(memcpy) will never call dmaengine_slave_config before prep,
> >
> > and that should have been a hint to you that you should not expect that
> >
> > > so jobs in dmaengine_slave_config need to be moved into somewhere
> > > before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > > ->private as other common case like uart/audio/spi will always setup
> > > chan->private. Here check it to judge if it's dmatest case and do
> > > jobs in slave_config.
> >
> > and you should not do anything for dmatest. Supporting it means memcpy
> > implementation is not correct :)
> Okay, I will any word about dmatest here since memcpy assume no calling
> slave_config.
> >
> > >
> > > Signed-off-by: Robin Gong
> > > ---
> > > drivers/dma/imx-sdma.c | 37 -
> > > 1 file changed, 28 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> > > ed2267d..48f3749 100644
> > > --- a/drivers/dma/imx-sdma.c
> > > +++ b/drivers/dma/imx-sdma.c
> > > @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct
> > > dma_chan *chan) {
> > > struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > struct imx_dma_data *data = chan->private;
> > > + struct imx_dma_data default_data;
> > > int prio, ret;
> > >
> > > - if (!data)
> > > - return -EINVAL;
> > > + ret = clk_enable(sdmac->sdma->clk_ipg);
> > > + if (ret)
> > > + return ret;
> > > + ret = clk_enable(sdmac->sdma->clk_ahb);
> > > + if (ret)
> > > + goto disable_clk_ipg;
> > > + /*
> > > + * dmatest(memcpy) will never call dmaengine_slave_config before prep,
> > > + * so jobs in dmaengine_slave_config need to be moved into somewhere
> > > + * before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > > + * ->private as other common cases like uart/audio/spi will setup
> > > + * chan->private always. Here check it to judge if it's dmatest case
> > > + * and do jobs in slave_config.
> > > + */
> > > + if (!data) {
> > > + dev_warn(sdmac->sdma->dev, "dmatest is running?\n");
> >
> > why is that a warning!
> Current SDMA driver assume filter function to set chan->private with specific
> data
> (struct imx_dma_data dma_data)like below (sound/soc/fsl/fsl_asrc_dma.c):
> static bool filter(struct dma_chan *chan, void *param)
> {
> if (!imx_dma_is_general_purpose(chan))
> return false;
> chan->private = param;
> return true;
> }
>
> But in memcpy case, at lease dmatest case, no chan->private set in its filter
> function.
> So here take dmatest a special case and do some prepare jobs for memcpy. But
> if the
> Upper device driver call dma_request_channel() with their specific filter
> without
> 'chan->private' setting in the future. The warning message is a useful hint
> to them to
> Add 'chan->private' in filter function. Or doc it somewhere?
Instead of doing heuristics to guess whether we are doing memcpy you
could instead make memcpy the default when slave_config is not called,
i.e. drop the if (!data) check completely.
> >
> > > + sdmac->word_size = sdmac->sdma->dma_device.copy_align;
> > > + default_data.priority = 2;
> > > + default_data.peripheral_type = IMX_DMATYPE_MEMORY;
> > > + default_data.dma_request = 0;
> > > + default_data.dma_request2 = 0;
> > > + data = &default_data;
> > > +
> > > + sdma_config_ownership(sdmac, false, true, false);
> > > + sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY);
> > > + sdma_load_context(sdmac);
> > > + }
> >
> > this needs to be default for memcpy
The problem seems to be that we do not know whether we are doing memcpy
or not. Normally we get the information how a channel is to be
configured in dma_device->device_config, but this function is not called
in the memcpy case.
An alternative might also be to do the setup in
dma_device->device_prep_dma_memcpy.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v1] dma: imx-sdma: add virt-dma support
On Wed, May 23, 2018 at 10:26:23AM +, Robin Gong wrote:
> >
> > >
> > > + u32 bd_size_sum;
> > This variable is never used for anything.
> Yes, it's not for significative use but debug to see how many current
> bds used.
I am not convinced this is useful. The variable could easily be added back
by someone who debugs this driver. The code has to be changed anyway to
make use of this variable.
> > > @@ -632,7 +635,7 @@ static int sdma_run_channel0(struct sdma_engine
> > > *sdma)
> > > static int sdma_load_script(struct sdma_engine *sdma, void *buf,
> > > int size,
> > > u32 address)
> > > {
> > > - struct sdma_buffer_descriptor *bd0 = sdma->channel[0].bd;
> > > + struct sdma_buffer_descriptor *bd0 = sdma->bd0;
> > This change seems to be an orthogonal change. Please make this a
> > separate patch.
> It's something related with virtual dma support, because in virtual
> dma framework, all bds should be allocated dynamically if they used.
> but bd0 is a specail case since it's must and basic for load sdma
> firmware and context for other channels. So here alloc 'bd0' for other
> channels.
Well, it's somewhat related to virtual dma support, but that's not my
point. My point is that this patch is quite big and thus hard to review.
If we find ways to make it smaller and to split it up in multiple
patches then we should do so, because it makes it easier to review and
in case you break something here we raise the chance that a "git bisect"
lands on a smaller patch which is easier to understand.
Please try and make that a separate change. I haven't really looked into
it and it may not be possible due to reasons I haven't seen, but please
at least give it a try.
> >
> > >
> > > +
> > > + if (sdmac->desc)
> > > + sdmac->desc = NULL;
> > The test is unnecesary.
> This 'NULL' is meaningful in case that dma done interrupt come after
> terminate as you know sdma will actually stop after current transfer
> done.
The setting of the variable to NULL is ok, but the test is useless.
if (sdmac->desc)
sdmac->desc = NULL;
is equivalent to:
sdmac->desc = NULL;
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v1 07/15] spi: imx: remove ERR009165 workaround on i.mx6ul
On Tue, Apr 23, 2019 at 01:51:10PM +, Robin Gong wrote:
>ERR009165 fix on i.mx6ul and next chip, such as i.mx6ull/i.mx8mq/i.mx8mm.
> Remove workaround on those chips. Add new i.mx6ul type for that.
>
> Signed-off-by: Robin Gong
> ---
> drivers/spi/spi-imx.c | 39 +++
> 1 file changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index eb56eac..2e5e978 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -57,6 +57,7 @@ enum spi_imx_devtype {
> IMX35_CSPI, /* CSPI on all i.mx except above */
> IMX51_ECSPI,/* ECSPI on i.mx51 */
> IMX53_ECSPI,/* ECSPI on i.mx53 and later */
> + IMX6UL_ECSPI, /* ERR009165 fix from i.mx6ul */
> };
>
> struct spi_imx_data;
> @@ -128,7 +129,8 @@ static inline int is_imx35_cspi(struct spi_imx_data *d)
>
> static inline int is_imx51_ecspi(struct spi_imx_data *d)
> {
> - return d->devtype_data->devtype == IMX51_ECSPI;
> + return d->devtype_data->devtype == IMX51_ECSPI ||
> +d->devtype_data->devtype == IMX6UL_ECSPI;
> }
>
> static inline int is_imx53_ecspi(struct spi_imx_data *d)
> @@ -585,9 +587,16 @@ static int mx51_ecspi_prepare_transfer(struct
> spi_imx_data *spi_imx,
> ctrl |= mx51_ecspi_clkdiv(spi_imx, t->speed_hz, &clk);
> spi_imx->spi_bus_clk = clk;
>
> - /* ERR009165: work in XHC mode as PIO */
> - if (spi_imx->usedma)
> - ctrl &= ~MX51_ECSPI_CTRL_SMC;
> + /*
> + * ERR009165: work in XHC mode instead of SMC as PIO on the chips
> + * before i.mx6ul.
> + */
> + if (spi_imx->usedma) {
> + if (spi_imx->devtype_data->devtype == IMX6UL_ECSPI)
> + ctrl |= MX51_ECSPI_CTRL_SMC;
> + else
> + ctrl &= ~MX51_ECSPI_CTRL_SMC;
> + }
>
> writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
>
> @@ -615,6 +624,8 @@ static void mx51_setup_wml(struct spi_imx_data *spi_imx)
> {
> u32 tx_wml = 0;
>
> + if (spi_imx->devtype_data->devtype == IMX6UL_ECSPI)
> + tx_wml = spi_imx->wml;
This if clause has to be adjusted for every new SoC which doesn't have
this issue. Better introduce a has_err009165 boolean flag to the driver
data and set it where necessary.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v1 14/15] ARM: dts: imx6sll: correct ecspi/sdma compatible
On Tue, Apr 23, 2019 at 01:51:45PM +, Robin Gong wrote:
> Correct ecspi/sdma compatible since ecspi errata ERR009165
> not fixed on i.mx6sll chip.
>
> Signed-off-by: Robin Gong
> ---
> arch/arm/boot/dts/imx6sll.dtsi | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6sll.dtsi b/arch/arm/boot/dts/imx6sll.dtsi
> index 1b4899f..03f2103 100644
> --- a/arch/arm/boot/dts/imx6sll.dtsi
> +++ b/arch/arm/boot/dts/imx6sll.dtsi
> @@ -183,7 +183,7 @@
> };
>
> ecspi1: spi@2008000 {
> - compatible = "fsl,imx6ul-ecspi",
> "fsl,imx51-ecspi";
> + compatible ="fsl,imx51-ecspi";
Where is the fsl,imx6sll-ecspi compatible that would have prevented us
from changing this devicetree now? You should at least add it now.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
