of_dma_request_slave_channel() failed ?

of_dma_request_slave_channel() failed ?

[Kuninori Morimoto]

Hi Renesas ML
Cc Rob

I noticed that Sound driver is using PIO mode
on latest kernel.

	...
	rcar_sound ec500000.sound: src[0] : probe error -11
	rcar_sound ec500000.sound: ssi[0] fallback to PIO mode
	rcar_sound ec500000.sound: src[1] : probe error -11
	rcar_sound ec500000.sound: ssi[1] fallback to PIO mode
	rcar_sound ec500000.sound: ssi[2] : probe error -11
	rcar_sound ec500000.sound: ssi[2] fallback to PIO mode
	rcar_sound ec500000.sound: ssi[3] : probe error -11
	rcar_sound ec500000.sound: ssi[3] fallback to PIO mode
	rcar_sound ec500000.sound: probed
	...

Sound can use DMA mode and PIO mode.
It automatically fallbacks to PIO mode if it can't use DMA.

I bisected this issue, and found the 1st bad commit

-----------------------------------------------------
commit ac6bbf0cdf4206c517ac9789814c23e372ebce4d
Author: Rob Herring <robh@kernel.org>
Date:   Mon Jul 9 09:41:52 2018 -0600

    iommu: Remove IOMMU_OF_DECLARE
    
    Now that we use the driver core to stop deferred probe for missing
    drivers, IOMMU_OF_DECLARE can be removed.
    
    This is slightly less optimal than having a list of built-in drivers in
    that we'll now defer probe twice before giving up. This shouldn't have a
    significant impact on boot times as past discussions about deferred
    probe have given no evidence of deferred probe having a substantial
    impact.
	...    
-----------------------------------------------------

In more detail, it seems it can't find DMA controller,
and of_dma_request_slave_channel() returns
-EPROBE_DEFER from this commit.

struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
					      const char *name)
{
	...
		ofdma = of_dma_find_controller(&dma_spec);

		if (ofdma) {
			chan = ofdma->of_dma_xlate(&dma_spec, ofdma);
		} else {
=>			ret_no_channel = -EPROBE_DEFER;
			chan = NULL;
		}
	...
=>	return ERR_PTR(ret_no_channel);
}

Is this known issue ?
Or sound shouldn't fallbacks to PIO mode ?

Best regards
---
Kuninori Morimoto

Re: of_dma_request_slave_channel() failed ?

[Geert Uytterhoeven]

Hi Morimoto-san,

CC asoc, dma, iommu

On Tue, Sep 4, 2018 at 8:06 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> Hi Renesas ML
> Cc Rob
>
> I noticed that Sound driver is using PIO mode
> on latest kernel.
>
>         ...
>         rcar_sound ec500000.sound: src[0] : probe error -11
>         rcar_sound ec500000.sound: ssi[0] fallback to PIO mode
>         rcar_sound ec500000.sound: src[1] : probe error -11
>         rcar_sound ec500000.sound: ssi[1] fallback to PIO mode
>         rcar_sound ec500000.sound: ssi[2] : probe error -11
>         rcar_sound ec500000.sound: ssi[2] fallback to PIO mode
>         rcar_sound ec500000.sound: ssi[3] : probe error -11
>         rcar_sound ec500000.sound: ssi[3] fallback to PIO mode
>         rcar_sound ec500000.sound: probed
>         ...
>
> Sound can use DMA mode and PIO mode.
> It automatically fallbacks to PIO mode if it can't use DMA.
>
> I bisected this issue, and found the 1st bad commit
>
> -----------------------------------------------------
> commit ac6bbf0cdf4206c517ac9789814c23e372ebce4d
> Author: Rob Herring <robh@kernel.org>
> Date:   Mon Jul 9 09:41:52 2018 -0600
>
>     iommu: Remove IOMMU_OF_DECLARE
>
>     Now that we use the driver core to stop deferred probe for missing
>     drivers, IOMMU_OF_DECLARE can be removed.
>
>     This is slightly less optimal than having a list of built-in drivers in
>     that we'll now defer probe twice before giving up. This shouldn't have a
>     significant impact on boot times as past discussions about deferred
>     probe have given no evidence of deferred probe having a substantial
>     impact.
>         ...
> -----------------------------------------------------
>
> In more detail, it seems it can't find DMA controller,
> and of_dma_request_slave_channel() returns
> -EPROBE_DEFER from this commit.
>
> struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
>                                               const char *name)
> {
>         ...
>                 ofdma = of_dma_find_controller(&dma_spec);
>
>                 if (ofdma) {
>                         chan = ofdma->of_dma_xlate(&dma_spec, ofdma);
>                 } else {
> =>                      ret_no_channel = -EPROBE_DEFER;
>                         chan = NULL;
>                 }
>         ...
> =>      return ERR_PTR(ret_no_channel);
> }
>
> Is this known issue ?
> Or sound shouldn't fallbacks to PIO mode ?

I assume you wrote commit 6c92d5a2744e2761 ("ASoC: rsnd: don't fallback
to PIO mode when -EPROBE_DEFER") to work around this?

While this got rid of the error messages, and postpones sound initialization
until the audio DMAC is available, it does mean that the driver will _never_
fall back to PIO.

I.e. if CONFIG_RCAR_DMAC=n, audio will fail to probe instead of falling
back to PIO.

With CONFIG_RCAR_DMAC=y:

    rcar-dmac e6700000.dma-controller: ignoring dependency for device,
assuming no driver
    rcar-dmac e7300000.dma-controller: ignoring dependency for device,
assuming no driver
    rcar-dmac e7310000.dma-controller: ignoring dependency for device,
assuming no driver
    rcar-dmac ec700000.dma-controller: ignoring dependency for device,
assuming no driver
    rcar-dmac ec720000.dma-controller: ignoring dependency for device,
assuming no driver

So it seems the audio DMAC is deferred a second time, before the iommu driver
probed.

    subsys_initcall(ipmmu_init); calls platform_driver_register
    module_platform_driver(rcar_dmac_driver);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: of_dma_request_slave_channel() failed ?

[Geert Uytterhoeven]

Hi Morimoto-san,

CC asoc, dma, iommu

On Tue, Sep 4, 2018 at 8:06 AM Kuninori Morimoto
<kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> Hi Renesas ML
> Cc Rob
>
> I noticed that Sound driver is using PIO mode
> on latest kernel.
>
>         ...
>         rcar_sound ec500000.sound: src[0] : probe error -11
>         rcar_sound ec500000.sound: ssi[0] fallback to PIO mode
>         rcar_sound ec500000.sound: src[1] : probe error -11
>         rcar_sound ec500000.sound: ssi[1] fallback to PIO mode
>         rcar_sound ec500000.sound: ssi[2] : probe error -11
>         rcar_sound ec500000.sound: ssi[2] fallback to PIO mode
>         rcar_sound ec500000.sound: ssi[3] : probe error -11
>         rcar_sound ec500000.sound: ssi[3] fallback to PIO mode
>         rcar_sound ec500000.sound: probed
>         ...
>
> Sound can use DMA mode and PIO mode.
> It automatically fallbacks to PIO mode if it can't use DMA.
>
> I bisected this issue, and found the 1st bad commit
>
> -----------------------------------------------------
> commit ac6bbf0cdf4206c517ac9789814c23e372ebce4d
> Author: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Date:   Mon Jul 9 09:41:52 2018 -0600
>
>     iommu: Remove IOMMU_OF_DECLARE
>
>     Now that we use the driver core to stop deferred probe for missing
>     drivers, IOMMU_OF_DECLARE can be removed.
>
>     This is slightly less optimal than having a list of built-in drivers in
>     that we'll now defer probe twice before giving up. This shouldn't have a
>     significant impact on boot times as past discussions about deferred
>     probe have given no evidence of deferred probe having a substantial
>     impact.
>         ...
> -----------------------------------------------------
>
> In more detail, it seems it can't find DMA controller,
> and of_dma_request_slave_channel() returns
> -EPROBE_DEFER from this commit.
>
> struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
>                                               const char *name)
> {
>         ...
>                 ofdma = of_dma_find_controller(&dma_spec);
>
>                 if (ofdma) {
>                         chan = ofdma->of_dma_xlate(&dma_spec, ofdma);
>                 } else {
> =>                      ret_no_channel = -EPROBE_DEFER;
>                         chan = NULL;
>                 }
>         ...
> =>      return ERR_PTR(ret_no_channel);
> }
>
> Is this known issue ?
> Or sound shouldn't fallbacks to PIO mode ?

I assume you wrote commit 6c92d5a2744e2761 ("ASoC: rsnd: don't fallback
to PIO mode when -EPROBE_DEFER") to work around this?

While this got rid of the error messages, and postpones sound initialization
until the audio DMAC is available, it does mean that the driver will _never_
fall back to PIO.

I.e. if CONFIG_RCAR_DMAC=n, audio will fail to probe instead of falling
back to PIO.

With CONFIG_RCAR_DMAC=y:

    rcar-dmac e6700000.dma-controller: ignoring dependency for device,
assuming no driver
    rcar-dmac e7300000.dma-controller: ignoring dependency for device,
assuming no driver
    rcar-dmac e7310000.dma-controller: ignoring dependency for device,
assuming no driver
    rcar-dmac ec700000.dma-controller: ignoring dependency for device,
assuming no driver
    rcar-dmac ec720000.dma-controller: ignoring dependency for device,
assuming no driver

So it seems the audio DMAC is deferred a second time, before the iommu driver
probed.

    subsys_initcall(ipmmu_init); calls platform_driver_register
    module_platform_driver(rcar_dmac_driver);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: of_dma_request_slave_channel() failed ?

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

On Tue, Sep 11, 2018 at 11:43:47AM +0200, Geert Uytterhoeven wrote:

> So it seems the audio DMAC is deferred a second time, before the iommu driver
> probed.

Shouldn't there be at least one more round of deferred probe triggers
after the IOMMU probes?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: of_dma_request_slave_channel() failed ?

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 254 bytes --]

On Tue, Sep 11, 2018 at 11:43:47AM +0200, Geert Uytterhoeven wrote:

> So it seems the audio DMAC is deferred a second time, before the iommu driver
> probed.

Shouldn't there be at least one more round of deferred probe triggers
after the IOMMU probes?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: of_dma_request_slave_channel() failed ?

[Kuninori Morimoto]

Hi Geert

Thank you for your feedback

> > -----------------------------------------------------
> > commit ac6bbf0cdf4206c517ac9789814c23e372ebce4d
> > Author: Rob Herring <robh@kernel.org>
> > Date:   Mon Jul 9 09:41:52 2018 -0600
> >
> >     iommu: Remove IOMMU_OF_DECLARE
> >
> >     Now that we use the driver core to stop deferred probe for missing
> >     drivers, IOMMU_OF_DECLARE can be removed.
> >
> >     This is slightly less optimal than having a list of built-in drivers in
> >     that we'll now defer probe twice before giving up. This shouldn't have a
> >     significant impact on boot times as past discussions about deferred
> >     probe have given no evidence of deferred probe having a substantial
> >     impact.
> >         ...
> > -----------------------------------------------------
(snip)
> I assume you wrote commit 6c92d5a2744e2761 ("ASoC: rsnd: don't fallback
> to PIO mode when -EPROBE_DEFER") to work around this?

Yes, it is work around for it.

> While this got rid of the error messages, and postpones sound initialization
> until the audio DMAC is available, it does mean that the driver will _never_
> fall back to PIO.
> 
> I.e. if CONFIG_RCAR_DMAC=n, audio will fail to probe instead of falling
> back to PIO.

If I focus only for sound here, the purpose of PIO mode is
checking basic HW connection, clock settings, etc.
Without DMAC support, it can't use many sound features.
And PIO mode is supporting "SSI" only.

If DT has SRC/CTU/DVC settings for sound,
it needs DMA support anyway.

&rcar_sound {
	...
	ports {
		rsnd_port0: port@0 {
			rsnd_endpoint0: endpoint {
				...
=>				playback = <&ssi0 &src0 &dvc0>;
			};
		};
	};
};

Before 6c92d5a2744e2761 patch, driver will forcibly ignore
non-SSI modules, and try to use PIO mode.
I'm not sure it is "kindly support" or "overkill support".

After this patch, it needs DMA, otherwise, probe will be failed.
DT shouldn't have non-SSI modules if you want to use PIO mode.

	+ /* use PIO mode */ 
	- playback = <&ssi0 &src0 &dvc0>;
	+ playback = <&ssi0>;

Re: of_dma_request_slave_channel() failed ?

[Kuninori Morimoto]

Hi Geert

Thank you for your feedback

> > -----------------------------------------------------
> > commit ac6bbf0cdf4206c517ac9789814c23e372ebce4d
> > Author: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Date:   Mon Jul 9 09:41:52 2018 -0600
> >
> >     iommu: Remove IOMMU_OF_DECLARE
> >
> >     Now that we use the driver core to stop deferred probe for missing
> >     drivers, IOMMU_OF_DECLARE can be removed.
> >
> >     This is slightly less optimal than having a list of built-in drivers in
> >     that we'll now defer probe twice before giving up. This shouldn't have a
> >     significant impact on boot times as past discussions about deferred
> >     probe have given no evidence of deferred probe having a substantial
> >     impact.
> >         ...
> > -----------------------------------------------------
(snip)
> I assume you wrote commit 6c92d5a2744e2761 ("ASoC: rsnd: don't fallback
> to PIO mode when -EPROBE_DEFER") to work around this?

Yes, it is work around for it.

> While this got rid of the error messages, and postpones sound initialization
> until the audio DMAC is available, it does mean that the driver will _never_
> fall back to PIO.
> 
> I.e. if CONFIG_RCAR_DMAC=n, audio will fail to probe instead of falling
> back to PIO.

If I focus only for sound here, the purpose of PIO mode is
checking basic HW connection, clock settings, etc.
Without DMAC support, it can't use many sound features.
And PIO mode is supporting "SSI" only.

If DT has SRC/CTU/DVC settings for sound,
it needs DMA support anyway.

&rcar_sound {
	...
	ports {
		rsnd_port0: port@0 {
			rsnd_endpoint0: endpoint {
				...
=>				playback = <&ssi0 &src0 &dvc0>;
			};
		};
	};
};

Before 6c92d5a2744e2761 patch, driver will forcibly ignore
non-SSI modules, and try to use PIO mode.
I'm not sure it is "kindly support" or "overkill support".

After this patch, it needs DMA, otherwise, probe will be failed.
DT shouldn't have non-SSI modules if you want to use PIO mode.

	+ /* use PIO mode */ 
	- playback = <&ssi0 &src0 &dvc0>;
	+ playback = <&ssi0>;

Re: of_dma_request_slave_channel() failed ?

[Geert Uytterhoeven]

Hi Morimoto-san,

On Thu, Sep 13, 2018 at 7:48 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> > > -----------------------------------------------------
> > > commit ac6bbf0cdf4206c517ac9789814c23e372ebce4d
> > > Author: Rob Herring <robh@kernel.org>
> > > Date:   Mon Jul 9 09:41:52 2018 -0600
> > >
> > >     iommu: Remove IOMMU_OF_DECLARE
> > >
> > >     Now that we use the driver core to stop deferred probe for missing
> > >     drivers, IOMMU_OF_DECLARE can be removed.
> > >
> > >     This is slightly less optimal than having a list of built-in drivers in
> > >     that we'll now defer probe twice before giving up. This shouldn't have a
> > >     significant impact on boot times as past discussions about deferred
> > >     probe have given no evidence of deferred probe having a substantial
> > >     impact.
> > >         ...
> > > -----------------------------------------------------
> (snip)
> > I assume you wrote commit 6c92d5a2744e2761 ("ASoC: rsnd: don't fallback
> > to PIO mode when -EPROBE_DEFER") to work around this?
>
> Yes, it is work around for it.
>
> > While this got rid of the error messages, and postpones sound initialization
> > until the audio DMAC is available, it does mean that the driver will _never_
> > fall back to PIO.
> >
> > I.e. if CONFIG_RCAR_DMAC=n, audio will fail to probe instead of falling
> > back to PIO.
>
> If I focus only for sound here, the purpose of PIO mode is
> checking basic HW connection, clock settings, etc.
> Without DMAC support, it can't use many sound features.
> And PIO mode is supporting "SSI" only.
>
> If DT has SRC/CTU/DVC settings for sound,
> it needs DMA support anyway.
>
> &rcar_sound {
>         ...
>         ports {
>                 rsnd_port0: port@0 {
>                         rsnd_endpoint0: endpoint {
>                                 ...
> =>                              playback = <&ssi0 &src0 &dvc0>;
>                         };
>                 };
>         };
> };
>
> Before 6c92d5a2744e2761 patch, driver will forcibly ignore
> non-SSI modules, and try to use PIO mode.
> I'm not sure it is "kindly support" or "overkill support".
>
> After this patch, it needs DMA, otherwise, probe will be failed.
> DT shouldn't have non-SSI modules if you want to use PIO mode.
>
>         + /* use PIO mode */
>         - playback = <&ssi0 &src0 &dvc0>;
>         + playback = <&ssi0>;
>

OK, so falling back to PIO was really a "best effort" fallback, and the user
should really enable DMA?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: of_dma_request_slave_channel() failed ?

[Geert Uytterhoeven]

Hi Morimoto-san,

On Thu, Sep 13, 2018 at 7:48 AM Kuninori Morimoto
<kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> > > -----------------------------------------------------
> > > commit ac6bbf0cdf4206c517ac9789814c23e372ebce4d
> > > Author: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > Date:   Mon Jul 9 09:41:52 2018 -0600
> > >
> > >     iommu: Remove IOMMU_OF_DECLARE
> > >
> > >     Now that we use the driver core to stop deferred probe for missing
> > >     drivers, IOMMU_OF_DECLARE can be removed.
> > >
> > >     This is slightly less optimal than having a list of built-in drivers in
> > >     that we'll now defer probe twice before giving up. This shouldn't have a
> > >     significant impact on boot times as past discussions about deferred
> > >     probe have given no evidence of deferred probe having a substantial
> > >     impact.
> > >         ...
> > > -----------------------------------------------------
> (snip)
> > I assume you wrote commit 6c92d5a2744e2761 ("ASoC: rsnd: don't fallback
> > to PIO mode when -EPROBE_DEFER") to work around this?
>
> Yes, it is work around for it.
>
> > While this got rid of the error messages, and postpones sound initialization
> > until the audio DMAC is available, it does mean that the driver will _never_
> > fall back to PIO.
> >
> > I.e. if CONFIG_RCAR_DMAC=n, audio will fail to probe instead of falling
> > back to PIO.
>
> If I focus only for sound here, the purpose of PIO mode is
> checking basic HW connection, clock settings, etc.
> Without DMAC support, it can't use many sound features.
> And PIO mode is supporting "SSI" only.
>
> If DT has SRC/CTU/DVC settings for sound,
> it needs DMA support anyway.
>
> &rcar_sound {
>         ...
>         ports {
>                 rsnd_port0: port@0 {
>                         rsnd_endpoint0: endpoint {
>                                 ...
> =>                              playback = <&ssi0 &src0 &dvc0>;
>                         };
>                 };
>         };
> };
>
> Before 6c92d5a2744e2761 patch, driver will forcibly ignore
> non-SSI modules, and try to use PIO mode.
> I'm not sure it is "kindly support" or "overkill support".
>
> After this patch, it needs DMA, otherwise, probe will be failed.
> DT shouldn't have non-SSI modules if you want to use PIO mode.
>
>         + /* use PIO mode */
>         - playback = <&ssi0 &src0 &dvc0>;
>         + playback = <&ssi0>;
>

OK, so falling back to PIO was really a "best effort" fallback, and the user
should really enable DMA?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: of_dma_request_slave_channel() failed ?

[Geert Uytterhoeven]

Hi Mark,

On Wed, Sep 12, 2018 at 5:51 PM Mark Brown <broonie@kernel.org> wrote:
> On Tue, Sep 11, 2018 at 11:43:47AM +0200, Geert Uytterhoeven wrote:
> > So it seems the audio DMAC is deferred a second time, before the iommu driver
> > probed.
>
> Shouldn't there be at least one more round of deferred probe triggers
> after the IOMMU probes?

My statement above was incorrect. Adding more debug info allowed to gain
more insight:

A. If CONFIG_IPMMU_VMSA=y, everything is fine:

  1. ipmmu probes,
  2. audio-dmac probes,
  3. audio probes, using DMA

B. If CONFIG_IPMMU_VMSA=n, the sound driver falls back to PIO, which
   Morimoto-san worked around by commit 6c92d5a2744e2761 ("ASoC: rsnd:
   don't fallback to PIO mode when -EPROBE_DEFER")[1]

  1. ipmmu is not probed: driver not enabled => OK
  2. audio-dmac is not probed: iommu not found
  3. audio fails to probe (3 times, hence just ignoring the first one is
     not sufficient):
        "of_dma_find_controller: can't find DMA controller
         /soc/dma-controller@ec700000"
        => fell back to PIO before [1]
        => -EPROBE_DEFER since [1]
  4. audio-dmac probes
      of_iommu_xlate() calls driver_deferred_probe_check_state(), leading
      to ("ignoring dependency for device, assuming no driver")

  If step 3 returned -EPROBE_DEFER, there's one additional step:
  5. audio reprobes successfully, using DMA.

While step 2 (DMA-capable device not probed because iommu not found)
is correct for devices with a builtin dmac, it causes issues with external
dmacs, as the dmac probe may be postponed after the DMA slave driver
probe.

C. Before commit ac6bbf0cdf4206c5 ("iommu: Remove IOMMU_OF_DECLARE"),
with CONFIG_IPMMU_VMSA=n:

  1. ipmmu is not probed: driver not enabled => OK
  2. audio-dmac probes => OK
      Missing IOMMU ignored, as per
          !ops && !of_iommu_driver_present(iommu_spec->np)
      in of_iommu_xlate(), which is really
          !ops && !of_match_node(&__iommu_of_table, np))
  3. audio probes, using DMA
      Note: if of_dma_find_controller() would have failed (e.g. missing
      dmac driver), the audio driver fell back to PIO here.

As per Morimoto-san's recent reply, audio really needs DMA, so the current
behavior of B.5. is OK. However, this may not be true for other devices
(e.g. SPI, i2c, serial, ...), where the dmac is optional.

The main issue is that if of_dma_find_controller() fails, a DMA slave driver
cannot distinguish between dmac not yet probed successfully, and dmac
driver not present.

However, as we rely more and more on probe deferral, this will cause more
issues in the future.

I guess the proper solution is to take into account dependencies described
in DT before probing devices, i.e. devices with phandles should always be
probed after the targets of these phandles?
Complication:
  - Circular dependencies,
  - Optional phandle targets (dmacs, oops).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: of_dma_request_slave_channel() failed ?

[Geert Uytterhoeven]

Hi Mark,

On Wed, Sep 12, 2018 at 5:51 PM Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Sep 11, 2018 at 11:43:47AM +0200, Geert Uytterhoeven wrote:
> > So it seems the audio DMAC is deferred a second time, before the iommu driver
> > probed.
>
> Shouldn't there be at least one more round of deferred probe triggers
> after the IOMMU probes?

My statement above was incorrect. Adding more debug info allowed to gain
more insight:

A. If CONFIG_IPMMU_VMSA=y, everything is fine:

  1. ipmmu probes,
  2. audio-dmac probes,
  3. audio probes, using DMA

B. If CONFIG_IPMMU_VMSA=n, the sound driver falls back to PIO, which
   Morimoto-san worked around by commit 6c92d5a2744e2761 ("ASoC: rsnd:
   don't fallback to PIO mode when -EPROBE_DEFER")[1]

  1. ipmmu is not probed: driver not enabled => OK
  2. audio-dmac is not probed: iommu not found
  3. audio fails to probe (3 times, hence just ignoring the first one is
     not sufficient):
        "of_dma_find_controller: can't find DMA controller
         /soc/dma-controller@ec700000"
        => fell back to PIO before [1]
        => -EPROBE_DEFER since [1]
  4. audio-dmac probes
      of_iommu_xlate() calls driver_deferred_probe_check_state(), leading
      to ("ignoring dependency for device, assuming no driver")

  If step 3 returned -EPROBE_DEFER, there's one additional step:
  5. audio reprobes successfully, using DMA.

While step 2 (DMA-capable device not probed because iommu not found)
is correct for devices with a builtin dmac, it causes issues with external
dmacs, as the dmac probe may be postponed after the DMA slave driver
probe.

C. Before commit ac6bbf0cdf4206c5 ("iommu: Remove IOMMU_OF_DECLARE"),
with CONFIG_IPMMU_VMSA=n:

  1. ipmmu is not probed: driver not enabled => OK
  2. audio-dmac probes => OK
      Missing IOMMU ignored, as per
          !ops && !of_iommu_driver_present(iommu_spec->np)
      in of_iommu_xlate(), which is really
          !ops && !of_match_node(&__iommu_of_table, np))
  3. audio probes, using DMA
      Note: if of_dma_find_controller() would have failed (e.g. missing
      dmac driver), the audio driver fell back to PIO here.

As per Morimoto-san's recent reply, audio really needs DMA, so the current
behavior of B.5. is OK. However, this may not be true for other devices
(e.g. SPI, i2c, serial, ...), where the dmac is optional.

The main issue is that if of_dma_find_controller() fails, a DMA slave driver
cannot distinguish between dmac not yet probed successfully, and dmac
driver not present.

However, as we rely more and more on probe deferral, this will cause more
issues in the future.

I guess the proper solution is to take into account dependencies described
in DT before probing devices, i.e. devices with phandles should always be
probed after the targets of these phandles?
Complication:
  - Circular dependencies,
  - Optional phandle targets (dmacs, oops).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: of_dma_request_slave_channel() failed ?

[Robin Murphy]

On 13/09/18 10:00, Geert Uytterhoeven wrote:
[...]
> The main issue is that if of_dma_find_controller() fails, a DMA slave driver
> cannot distinguish between dmac not yet probed successfully, and dmac
> driver not present.

...which is in fact the exact same problem that the IOMMU code has - 
might it make sense to give of_dma_request_slave_channel() similar 
(optional?) driver_deferred_probe_check_state() logic, i.e. "if my DMAC 
driver's not shown up by this point, assume it's not built-in and go on 
without it". Of course it is somewhat easier for IOMMU drivers as 
there's zero chance of those popping up as modules later on.

> However, as we rely more and more on probe deferral, this will cause more
> issues in the future.
> 
> I guess the proper solution is to take into account dependencies described
> in DT before probing devices, i.e. devices with phandles should always be
> probed after the targets of these phandles?
> Complication:
>    - Circular dependencies,
>    - Optional phandle targets (dmacs, oops).

Yeah, a proper dependency graph would be ideal, but it's certainly not 
trivial. There was an interesting proposal a couple of years ago of a 
half-way approach where various phandle-chasing routines could actively 
try to probe the target, but I think that got stymied on the fact that a 
single OF node may represent multiple different driver-level providers, 
so it's tricky to tell whether the *right* dependency has been satisfied 
without detailed knowledge of the individual bindings and driver 
implementations. That's probably still an issue for other approaches 
too, sadly.

Robin.

Re: of_dma_request_slave_channel() failed ?

[Robin Murphy]

On 13/09/18 10:00, Geert Uytterhoeven wrote:
[...]
> The main issue is that if of_dma_find_controller() fails, a DMA slave driver
> cannot distinguish between dmac not yet probed successfully, and dmac
> driver not present.

...which is in fact the exact same problem that the IOMMU code has - 
might it make sense to give of_dma_request_slave_channel() similar 
(optional?) driver_deferred_probe_check_state() logic, i.e. "if my DMAC 
driver's not shown up by this point, assume it's not built-in and go on 
without it". Of course it is somewhat easier for IOMMU drivers as 
there's zero chance of those popping up as modules later on.

> However, as we rely more and more on probe deferral, this will cause more
> issues in the future.
> 
> I guess the proper solution is to take into account dependencies described
> in DT before probing devices, i.e. devices with phandles should always be
> probed after the targets of these phandles?
> Complication:
>    - Circular dependencies,
>    - Optional phandle targets (dmacs, oops).

Yeah, a proper dependency graph would be ideal, but it's certainly not 
trivial. There was an interesting proposal a couple of years ago of a 
half-way approach where various phandle-chasing routines could actively 
try to probe the target, but I think that got stymied on the fact that a 
single OF node may represent multiple different driver-level providers, 
so it's tricky to tell whether the *right* dependency has been satisfied 
without detailed knowledge of the individual bindings and driver 
implementations. That's probably still an issue for other approaches 
too, sadly.

Robin.

Re: of_dma_request_slave_channel() failed ?

[Geert Uytterhoeven]

Hi Robin,

On Thu, Sep 13, 2018 at 12:12 PM Robin Murphy <robin.murphy@arm.com> wrote:
> On 13/09/18 10:00, Geert Uytterhoeven wrote:
> [...]
> > The main issue is that if of_dma_find_controller() fails, a DMA slave driver
> > cannot distinguish between dmac not yet probed successfully, and dmac
> > driver not present.
>
> ...which is in fact the exact same problem that the IOMMU code has -
> might it make sense to give of_dma_request_slave_channel() similar
> (optional?) driver_deferred_probe_check_state() logic, i.e. "if my DMAC
> driver's not shown up by this point, assume it's not built-in and go on
> without it". Of course it is somewhat easier for IOMMU drivers as
> there's zero chance of those popping up as modules later on.

It may solve the issue in some cases.  But only if the dmac is reprobed
before the DMA slave driver, which is not guaranteed.

BTW, it seems e.g. i2c and serial suffer from the same problem, and fall
back to PIO. However, these drivers try to obtain the DMA channel when
used, not during probe, so they start using DMA after the dmac has been
probed successfully.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: of_dma_request_slave_channel() failed ?

[Geert Uytterhoeven]

Hi Robin,

On Thu, Sep 13, 2018 at 12:12 PM Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> On 13/09/18 10:00, Geert Uytterhoeven wrote:
> [...]
> > The main issue is that if of_dma_find_controller() fails, a DMA slave driver
> > cannot distinguish between dmac not yet probed successfully, and dmac
> > driver not present.
>
> ...which is in fact the exact same problem that the IOMMU code has -
> might it make sense to give of_dma_request_slave_channel() similar
> (optional?) driver_deferred_probe_check_state() logic, i.e. "if my DMAC
> driver's not shown up by this point, assume it's not built-in and go on
> without it". Of course it is somewhat easier for IOMMU drivers as
> there's zero chance of those popping up as modules later on.

It may solve the issue in some cases.  But only if the dmac is reprobed
before the DMA slave driver, which is not guaranteed.

BTW, it seems e.g. i2c and serial suffer from the same problem, and fall
back to PIO. However, these drivers try to obtain the DMA channel when
used, not during probe, so they start using DMA after the dmac has been
probed successfully.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: of_dma_request_slave_channel() failed ?

[Kuninori Morimoto]

Hi Geert

> > Before 6c92d5a2744e2761 patch, driver will forcibly ignore
> > non-SSI modules, and try to use PIO mode.
> > I'm not sure it is "kindly support" or "overkill support".
> >
> > After this patch, it needs DMA, otherwise, probe will be failed.
> > DT shouldn't have non-SSI modules if you want to use PIO mode.
> >
> >         + /* use PIO mode */
> >         - playback = <&ssi0 &src0 &dvc0>;
> >         + playback = <&ssi0>;
> >
> 
> OK, so falling back to PIO was really a "best effort" fallback, and the user
> should really enable DMA?

Yeah, I'm thinking this is better.
PIO fallback is of course "nice to have" if possible.
But, because of this new iommu patch, keeping this feature
needs "big complicated patch", and we can get "small effect" I think.
Thus, I think this is the time to remove this feature.
Can you agree ?

Best regards
---
Kuninori Morimoto

Re: of_dma_request_slave_channel() failed ?

[Kuninori Morimoto]

Hi Geert

> > Before 6c92d5a2744e2761 patch, driver will forcibly ignore
> > non-SSI modules, and try to use PIO mode.
> > I'm not sure it is "kindly support" or "overkill support".
> >
> > After this patch, it needs DMA, otherwise, probe will be failed.
> > DT shouldn't have non-SSI modules if you want to use PIO mode.
> >
> >         + /* use PIO mode */
> >         - playback = <&ssi0 &src0 &dvc0>;
> >         + playback = <&ssi0>;
> >
> 
> OK, so falling back to PIO was really a "best effort" fallback, and the user
> should really enable DMA?

Yeah, I'm thinking this is better.
PIO fallback is of course "nice to have" if possible.
But, because of this new iommu patch, keeping this feature
needs "big complicated patch", and we can get "small effect" I think.
Thus, I think this is the time to remove this feature.
Can you agree ?

Best regards
---
Kuninori Morimoto

Re: of_dma_request_slave_channel() failed ?

[Kuninori Morimoto]

Hi Geert

> > ...which is in fact the exact same problem that the IOMMU code has -
> > might it make sense to give of_dma_request_slave_channel() similar
> > (optional?) driver_deferred_probe_check_state() logic, i.e. "if my DMAC
> > driver's not shown up by this point, assume it's not built-in and go on
> > without it". Of course it is somewhat easier for IOMMU drivers as
> > there's zero chance of those popping up as modules later on.
> 
> It may solve the issue in some cases.  But only if the dmac is reprobed
> before the DMA slave driver, which is not guaranteed.
> 
> BTW, it seems e.g. i2c and serial suffer from the same problem, and fall
> back to PIO. However, these drivers try to obtain the DMA channel when
> used, not during probe, so they start using DMA after the dmac has been
> probed successfully.

Actually sound driver get DMA channel when used.
But checking DMA availability when probe timing (= try to get, and release it soon).
sound driver side other approach is that it don't check it when probe.

Best regards
---
Kuninori Morimoto

Re: of_dma_request_slave_channel() failed ?

[Kuninori Morimoto]

Hi Geert

> > ...which is in fact the exact same problem that the IOMMU code has -
> > might it make sense to give of_dma_request_slave_channel() similar
> > (optional?) driver_deferred_probe_check_state() logic, i.e. "if my DMAC
> > driver's not shown up by this point, assume it's not built-in and go on
> > without it". Of course it is somewhat easier for IOMMU drivers as
> > there's zero chance of those popping up as modules later on.
> 
> It may solve the issue in some cases.  But only if the dmac is reprobed
> before the DMA slave driver, which is not guaranteed.
> 
> BTW, it seems e.g. i2c and serial suffer from the same problem, and fall
> back to PIO. However, these drivers try to obtain the DMA channel when
> used, not during probe, so they start using DMA after the dmac has been
> probed successfully.

Actually sound driver get DMA channel when used.
But checking DMA availability when probe timing (= try to get, and release it soon).
sound driver side other approach is that it don't check it when probe.

Best regards
---
Kuninori Morimoto

Re: of_dma_request_slave_channel() failed ?

[Geert Uytterhoeven]

Hi Morimoto-san,

On Fri, Sep 14, 2018 at 2:53 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> > > Before 6c92d5a2744e2761 patch, driver will forcibly ignore
> > > non-SSI modules, and try to use PIO mode.
> > > I'm not sure it is "kindly support" or "overkill support".
> > >
> > > After this patch, it needs DMA, otherwise, probe will be failed.
> > > DT shouldn't have non-SSI modules if you want to use PIO mode.
> > >
> > >         + /* use PIO mode */
> > >         - playback = <&ssi0 &src0 &dvc0>;
> > >         + playback = <&ssi0>;
> >
> > OK, so falling back to PIO was really a "best effort" fallback, and the user
> > should really enable DMA?
>
> Yeah, I'm thinking this is better.
> PIO fallback is of course "nice to have" if possible.
> But, because of this new iommu patch, keeping this feature
> needs "big complicated patch", and we can get "small effect" I think.
> Thus, I think this is the time to remove this feature.
> Can you agree ?

You're the rcar-sound expert ;-)
If you think there's not much value in keeping PIO fallback, it can be
removed.

We can change behavior once these dependency issues have been resolved
in a generic way.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: of_dma_request_slave_channel() failed ?

[Geert Uytterhoeven]

Hi Morimoto-san,

On Fri, Sep 14, 2018 at 2:53 AM Kuninori Morimoto
<kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> > > Before 6c92d5a2744e2761 patch, driver will forcibly ignore
> > > non-SSI modules, and try to use PIO mode.
> > > I'm not sure it is "kindly support" or "overkill support".
> > >
> > > After this patch, it needs DMA, otherwise, probe will be failed.
> > > DT shouldn't have non-SSI modules if you want to use PIO mode.
> > >
> > >         + /* use PIO mode */
> > >         - playback = <&ssi0 &src0 &dvc0>;
> > >         + playback = <&ssi0>;
> >
> > OK, so falling back to PIO was really a "best effort" fallback, and the user
> > should really enable DMA?
>
> Yeah, I'm thinking this is better.
> PIO fallback is of course "nice to have" if possible.
> But, because of this new iommu patch, keeping this feature
> needs "big complicated patch", and we can get "small effect" I think.
> Thus, I think this is the time to remove this feature.
> Can you agree ?

You're the rcar-sound expert ;-)
If you think there's not much value in keeping PIO fallback, it can be
removed.

We can change behavior once these dependency issues have been resolved
in a generic way.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds