simpleaudio: set sysclk before using it

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

simpleaudio: set sysclk before using it

Klemens Nanni-2

Feedback? Objections? OK?
 
diff c154c5b3e913ab7299483002bea9fb9782684007 274222c1624a27cde904e8964e7b663a3d0750d8
blob - 59cda075c0a8ec80308c145ca8dfab78f36816ef
blob + 0bbe4da7bf8ee03806130833b63cd3245b79f196
--- sys/dev/fdt/simpleaudio.c
+++ sys/dev/fdt/simpleaudio.c
@@ -300,24 +300,6 @@ simpleaudio_set_params(void *cookie, int setmode, int
  uint32_t rate;
  int error;
 
- dai = sc->sc_dai_cpu;
- hwif = dai->dd_hw_if;
- if (hwif->set_params) {
- error = hwif->set_params(dai->dd_cookie,
-    setmode, usemode, play, rec);
- if (error)
- return error;
- }
-
- dai = sc->sc_dai_codec;
- hwif = dai->dd_hw_if;
- if (hwif->set_params) {
- error = hwif->set_params(dai->dd_cookie,
-    setmode, usemode, play, rec);
- if (error)
- return error;
- }
-
  if (sc->sc_mclk_fs) {
  if (setmode & AUMODE_PLAY)
  rate = play->sample_rate * sc->sc_mclk_fs;
@@ -339,6 +321,24 @@ simpleaudio_set_params(void *cookie, int setmode, int
  }
  }
 
+ dai = sc->sc_dai_cpu;
+ hwif = dai->dd_hw_if;
+ if (hwif->set_params) {
+ error = hwif->set_params(dai->dd_cookie,
+    setmode, usemode, play, rec);
+ if (error)
+ return error;
+ }
+
+ dai = sc->sc_dai_codec;
+ hwif = dai->dd_hw_if;
+ if (hwif->set_params) {
+ error = hwif->set_params(dai->dd_cookie,
+    setmode, usemode, play, rec);
+ if (error)
+ return error;
+ }
+
  return 0;
 }
 


Reply | Threaded
Open this post in threaded view
|

Re: simpleaudio: set sysclk before using it

Mark Kettenis
> Date: Sun, 4 Apr 2021 21:08:09 +0200
> From: Klemens Nanni <[hidden email]>
>
> Feedback? Objections? OK?

Explanation?

Not sure what happened here, but the reply-to was completely garbled...


> diff c154c5b3e913ab7299483002bea9fb9782684007 274222c1624a27cde904e8964e7b663a3d0750d8
> blob - 59cda075c0a8ec80308c145ca8dfab78f36816ef
> blob + 0bbe4da7bf8ee03806130833b63cd3245b79f196
> --- sys/dev/fdt/simpleaudio.c
> +++ sys/dev/fdt/simpleaudio.c
> @@ -300,24 +300,6 @@ simpleaudio_set_params(void *cookie, int setmode, int
>   uint32_t rate;
>   int error;
>  
> - dai = sc->sc_dai_cpu;
> - hwif = dai->dd_hw_if;
> - if (hwif->set_params) {
> - error = hwif->set_params(dai->dd_cookie,
> -    setmode, usemode, play, rec);
> - if (error)
> - return error;
> - }
> -
> - dai = sc->sc_dai_codec;
> - hwif = dai->dd_hw_if;
> - if (hwif->set_params) {
> - error = hwif->set_params(dai->dd_cookie,
> -    setmode, usemode, play, rec);
> - if (error)
> - return error;
> - }
> -
>   if (sc->sc_mclk_fs) {
>   if (setmode & AUMODE_PLAY)
>   rate = play->sample_rate * sc->sc_mclk_fs;
> @@ -339,6 +321,24 @@ simpleaudio_set_params(void *cookie, int setmode, int
>   }
>   }
>  
> + dai = sc->sc_dai_cpu;
> + hwif = dai->dd_hw_if;
> + if (hwif->set_params) {
> + error = hwif->set_params(dai->dd_cookie,
> +    setmode, usemode, play, rec);
> + if (error)
> + return error;
> + }
> +
> + dai = sc->sc_dai_codec;
> + hwif = dai->dd_hw_if;
> + if (hwif->set_params) {
> + error = hwif->set_params(dai->dd_cookie,
> +    setmode, usemode, play, rec);
> + if (error)
> + return error;
> + }
> +
>   return 0;
>  }
>  
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: simpleaudio: set sysclk before using it

Klemens Nanni-2
On Sun, Apr 04, 2021 at 10:01:50PM +0200, Mark Kettenis wrote:
> > Date: Sun, 4 Apr 2021 21:08:09 +0200
> > From: Klemens Nanni <[hidden email]>
> >
> > Feedback? Objections? OK?
>
> Explanation?
>
> Not sure what happened here, but the reply-to was completely garbled...
Sorry, I must've crippled the body before sending.

simpleaudio_set_params() calls set_params() which reads sysclk off the
"i2s_clk" property before it sets that very clock's dd_set_sysclk()
(in case there's multiplier specified).

Hence reverse the order so set_params() picks up the newly set rate.

The rate is still off on the Pinebook Pro, but I came across this when
reading the code and it seemed wrong, so I also checked NetBSD which did
the same with

sys/dev/fdt/ausoc.c r1.6
"Set sysclk rate at set_format time, so the link set_format callback can read the new sysclk"
https://github.com/NetBSD/src/commit/ac8f47d1e5f46949b081c8e9d95211cdfda1e327


Index: dev/fdt/simpleaudio.c
===================================================================
RCS file: /cvs/src/sys/dev/fdt/simpleaudio.c,v
retrieving revision 1.1
diff -u -p -r1.1 simpleaudio.c
--- dev/fdt/simpleaudio.c 10 Jun 2020 23:55:19 -0000 1.1
+++ dev/fdt/simpleaudio.c 4 Apr 2021 20:23:39 -0000
@@ -300,24 +300,6 @@ simpleaudio_set_params(void *cookie, int
  uint32_t rate;
  int error;
 
- dai = sc->sc_dai_cpu;
- hwif = dai->dd_hw_if;
- if (hwif->set_params) {
- error = hwif->set_params(dai->dd_cookie,
-    setmode, usemode, play, rec);
- if (error)
- return error;
- }
-
- dai = sc->sc_dai_codec;
- hwif = dai->dd_hw_if;
- if (hwif->set_params) {
- error = hwif->set_params(dai->dd_cookie,
-    setmode, usemode, play, rec);
- if (error)
- return error;
- }
-
  if (sc->sc_mclk_fs) {
  if (setmode & AUMODE_PLAY)
  rate = play->sample_rate * sc->sc_mclk_fs;
@@ -337,6 +319,24 @@ simpleaudio_set_params(void *cookie, int
  if (error)
  return error;
  }
+ }
+
+ dai = sc->sc_dai_cpu;
+ hwif = dai->dd_hw_if;
+ if (hwif->set_params) {
+ error = hwif->set_params(dai->dd_cookie,
+    setmode, usemode, play, rec);
+ if (error)
+ return error;
+ }
+
+ dai = sc->sc_dai_codec;
+ hwif = dai->dd_hw_if;
+ if (hwif->set_params) {
+ error = hwif->set_params(dai->dd_cookie,
+    setmode, usemode, play, rec);
+ if (error)
+ return error;
  }
 
  return 0;

Reply | Threaded
Open this post in threaded view
|

Re: simpleaudio: set sysclk before using it

Mark Kettenis
> Date: Sun, 4 Apr 2021 22:24:57 +0200
> From: Klemens Nanni <[hidden email]>
> Cc: Mark Kettenis <[hidden email]>
>
> On Sun, Apr 04, 2021 at 10:01:50PM +0200, Mark Kettenis wrote:
> > > Date: Sun, 4 Apr 2021 21:08:09 +0200
> > > From: Klemens Nanni <[hidden email]>
> > >
> > > Feedback? Objections? OK?
> >
> > Explanation?
> >
> > Not sure what happened here, but the reply-to was completely garbled...
> Sorry, I must've crippled the body before sending.
>
> simpleaudio_set_params() calls set_params() which reads sysclk off the
> "i2s_clk" property before it sets that very clock's dd_set_sysclk()
> (in case there's multiplier specified).
>
> Hence reverse the order so set_params() picks up the newly set rate.
>
> The rate is still off on the Pinebook Pro, but I came across this when
> reading the code and it seemed wrong, so I also checked NetBSD which did
> the same with
>
> sys/dev/fdt/ausoc.c r1.6
> "Set sysclk rate at set_format time, so the link set_format callback can read the new sysclk"
> https://github.com/NetBSD/src/commit/ac8f47d1e5f46949b081c8e9d95211cdfda1e327

OK. So NetBSD's _set_format() is the equivalent of our _set_params().
So changing the order makes us match how NetBSD does things.  That
makes some sense.

ok kettenis@, but give Patrick a chance to comment as well.


> Index: dev/fdt/simpleaudio.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/fdt/simpleaudio.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 simpleaudio.c
> --- dev/fdt/simpleaudio.c 10 Jun 2020 23:55:19 -0000 1.1
> +++ dev/fdt/simpleaudio.c 4 Apr 2021 20:23:39 -0000
> @@ -300,24 +300,6 @@ simpleaudio_set_params(void *cookie, int
>   uint32_t rate;
>   int error;
>  
> - dai = sc->sc_dai_cpu;
> - hwif = dai->dd_hw_if;
> - if (hwif->set_params) {
> - error = hwif->set_params(dai->dd_cookie,
> -    setmode, usemode, play, rec);
> - if (error)
> - return error;
> - }
> -
> - dai = sc->sc_dai_codec;
> - hwif = dai->dd_hw_if;
> - if (hwif->set_params) {
> - error = hwif->set_params(dai->dd_cookie,
> -    setmode, usemode, play, rec);
> - if (error)
> - return error;
> - }
> -
>   if (sc->sc_mclk_fs) {
>   if (setmode & AUMODE_PLAY)
>   rate = play->sample_rate * sc->sc_mclk_fs;
> @@ -337,6 +319,24 @@ simpleaudio_set_params(void *cookie, int
>   if (error)
>   return error;
>   }
> + }
> +
> + dai = sc->sc_dai_cpu;
> + hwif = dai->dd_hw_if;
> + if (hwif->set_params) {
> + error = hwif->set_params(dai->dd_cookie,
> +    setmode, usemode, play, rec);
> + if (error)
> + return error;
> + }
> +
> + dai = sc->sc_dai_codec;
> + hwif = dai->dd_hw_if;
> + if (hwif->set_params) {
> + error = hwif->set_params(dai->dd_cookie,
> +    setmode, usemode, play, rec);
> + if (error)
> + return error;
>   }
>  
>   return 0;
>

Reply | Threaded
Open this post in threaded view
|

Re: simpleaudio: set sysclk before using it

Patrick Wildt-3
Am Sun, Apr 04, 2021 at 11:17:54PM +0200 schrieb Mark Kettenis:

> > Date: Sun, 4 Apr 2021 22:24:57 +0200
> > From: Klemens Nanni <[hidden email]>
> > Cc: Mark Kettenis <[hidden email]>
> >
> > On Sun, Apr 04, 2021 at 10:01:50PM +0200, Mark Kettenis wrote:
> > > > Date: Sun, 4 Apr 2021 21:08:09 +0200
> > > > From: Klemens Nanni <[hidden email]>
> > > >
> > > > Feedback? Objections? OK?
> > >
> > > Explanation?
> > >
> > > Not sure what happened here, but the reply-to was completely garbled...
> > Sorry, I must've crippled the body before sending.
> >
> > simpleaudio_set_params() calls set_params() which reads sysclk off the
> > "i2s_clk" property before it sets that very clock's dd_set_sysclk()
> > (in case there's multiplier specified).
> >
> > Hence reverse the order so set_params() picks up the newly set rate.
> >
> > The rate is still off on the Pinebook Pro, but I came across this when
> > reading the code and it seemed wrong, so I also checked NetBSD which did
> > the same with
> >
> > sys/dev/fdt/ausoc.c r1.6
> > "Set sysclk rate at set_format time, so the link set_format callback can read the new sysclk"
> > https://github.com/NetBSD/src/commit/ac8f47d1e5f46949b081c8e9d95211cdfda1e327
>
> OK. So NetBSD's _set_format() is the equivalent of our _set_params().
> So changing the order makes us match how NetBSD does things.  That
> makes some sense.
>
> ok kettenis@, but give Patrick a chance to comment as well.
>

ok patrick@

>
> > Index: dev/fdt/simpleaudio.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/fdt/simpleaudio.c,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 simpleaudio.c
> > --- dev/fdt/simpleaudio.c 10 Jun 2020 23:55:19 -0000 1.1
> > +++ dev/fdt/simpleaudio.c 4 Apr 2021 20:23:39 -0000
> > @@ -300,24 +300,6 @@ simpleaudio_set_params(void *cookie, int
> >   uint32_t rate;
> >   int error;
> >  
> > - dai = sc->sc_dai_cpu;
> > - hwif = dai->dd_hw_if;
> > - if (hwif->set_params) {
> > - error = hwif->set_params(dai->dd_cookie,
> > -    setmode, usemode, play, rec);
> > - if (error)
> > - return error;
> > - }
> > -
> > - dai = sc->sc_dai_codec;
> > - hwif = dai->dd_hw_if;
> > - if (hwif->set_params) {
> > - error = hwif->set_params(dai->dd_cookie,
> > -    setmode, usemode, play, rec);
> > - if (error)
> > - return error;
> > - }
> > -
> >   if (sc->sc_mclk_fs) {
> >   if (setmode & AUMODE_PLAY)
> >   rate = play->sample_rate * sc->sc_mclk_fs;
> > @@ -337,6 +319,24 @@ simpleaudio_set_params(void *cookie, int
> >   if (error)
> >   return error;
> >   }
> > + }
> > +
> > + dai = sc->sc_dai_cpu;
> > + hwif = dai->dd_hw_if;
> > + if (hwif->set_params) {
> > + error = hwif->set_params(dai->dd_cookie,
> > +    setmode, usemode, play, rec);
> > + if (error)
> > + return error;
> > + }
> > +
> > + dai = sc->sc_dai_codec;
> > + hwif = dai->dd_hw_if;
> > + if (hwif->set_params) {
> > + error = hwif->set_params(dai->dd_cookie,
> > +    setmode, usemode, play, rec);
> > + if (error)
> > + return error;
> >   }
> >  
> >   return 0;
> >
>