diff mbox series

[04/18] arm64: dts: renesas: beacon kit: Fix Audio Clock sources

Message ID 20201213183759.223246-5-aford173@gmail.com
State New
Headers show
Series arm64: dts: renesas: Cleanup Beacon Kit and support more SoC's | expand

Commit Message

Adam Ford Dec. 13, 2020, 6:37 p.m. UTC
The SoC was expecting two clock sources with different frequencies.
One to support 44.1KHz and one to support 48KHz.  With the newly added
ability to configure the programmably clock, configure both clocks.

Beacause the SoC is expecting a fixed clock/oscillator, it doesn't
attempt to get and enable the clock for audio_clk_a. The choice to
use a fixed-factor-clock was due to the fact that it will automatically
enable the programmable clock frequency without change any code.

Signed-off-by: Adam Ford <aford173@gmail.com>
---
 .../boot/dts/renesas/beacon-renesom-baseboard.dtsi    | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Geert Uytterhoeven Dec. 17, 2020, 10:54 a.m. UTC | #1
Hi Adam,

On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> wrote:
> The SoC was expecting two clock sources with different frequencies.

> One to support 44.1KHz and one to support 48KHz.  With the newly added

> ability to configure the programmably clock, configure both clocks.

>

> Beacause the SoC is expecting a fixed clock/oscillator, it doesn't

> attempt to get and enable the clock for audio_clk_a. The choice to

> use a fixed-factor-clock was due to the fact that it will automatically

> enable the programmable clock frequency without change any code.

>

> Signed-off-by: Adam Ford <aford173@gmail.com>


Thanks for your patch!

> --- a/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi

> +++ b/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi

> @@ -250,9 +250,12 @@ ss_ep: endpoint {

>  };

>

>  &audio_clk_a {

> -       clock-frequency = <24576000>;

> -       assigned-clocks = <&versaclock6_bb 4>;

> -       assigned-clock-rates = <24576000>;

> +       /delete-property/ clock-frequency;

> +       #clock-cells = <0>;

> +       compatible = "fixed-factor-clock";

> +       clock-mult = <1>;

> +       clock-div = <1>;

> +       clocks = <&versaclock6_bb 4>;

>  };


Shouldn't you override the clocks property in the rcar_sound node
instead, like is done in several other board DTS files (with cs2000)?

>

>  &audio_clk_b {

> @@ -591,7 +594,7 @@ sound_pins: sound {

>         };

>

>         sound_clk_pins: sound_clk {

> -               groups = "audio_clk_a_a";

> +               groups = "audio_clk_a_a", "audio_clk_b_a";

>                 function = "audio_clk";

>         };


Yes, this part was definitely missing.

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
Adam Ford Dec. 17, 2020, 12:01 p.m. UTC | #2
On Thu, Dec 17, 2020 at 4:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Adam,

>

> On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> wrote:

> > The SoC was expecting two clock sources with different frequencies.

> > One to support 44.1KHz and one to support 48KHz.  With the newly added

> > ability to configure the programmably clock, configure both clocks.

> >

> > Beacause the SoC is expecting a fixed clock/oscillator, it doesn't

> > attempt to get and enable the clock for audio_clk_a. The choice to

> > use a fixed-factor-clock was due to the fact that it will automatically

> > enable the programmable clock frequency without change any code.

> >

> > Signed-off-by: Adam Ford <aford173@gmail.com>

>

> Thanks for your patch!

>

> > --- a/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi

> > +++ b/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi

> > @@ -250,9 +250,12 @@ ss_ep: endpoint {

> >  };

> >

> >  &audio_clk_a {

> > -       clock-frequency = <24576000>;

> > -       assigned-clocks = <&versaclock6_bb 4>;

> > -       assigned-clock-rates = <24576000>;

> > +       /delete-property/ clock-frequency;

> > +       #clock-cells = <0>;

> > +       compatible = "fixed-factor-clock";

> > +       clock-mult = <1>;

> > +       clock-div = <1>;

> > +       clocks = <&versaclock6_bb 4>;

> >  };

>

> Shouldn't you override the clocks property in the rcar_sound node

> instead, like is done in several other board DTS files (with cs2000)?

>


I guess there are multiple ways to do this.  Because the rcar_sound
was already expecting a reference to audio_clk_a, it seemed less
intrusive this way. The way I proposed, we can use the default
rcar_sound clocking and just change the audio_clk node to enable the
versaclock output.  The versaclock is driving the audio_clk_a
reference clock, so it seemed appropriate to put it there.

If you want me to change, I will.

> >

> >  &audio_clk_b {

> > @@ -591,7 +594,7 @@ sound_pins: sound {

> >         };

> >

> >         sound_clk_pins: sound_clk {

> > -               groups = "audio_clk_a_a";

> > +               groups = "audio_clk_a_a", "audio_clk_b_a";

> >                 function = "audio_clk";

> >         };

>

> Yes, this part was definitely missing.

>

> 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
Geert Uytterhoeven Dec. 18, 2020, 1:05 p.m. UTC | #3
Hi Adam,

On Thu, Dec 17, 2020 at 1:01 PM Adam Ford <aford173@gmail.com> wrote:
> On Thu, Dec 17, 2020 at 4:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> wrote:

> > > The SoC was expecting two clock sources with different frequencies.

> > > One to support 44.1KHz and one to support 48KHz.  With the newly added

> > > ability to configure the programmably clock, configure both clocks.

> > >

> > > Beacause the SoC is expecting a fixed clock/oscillator, it doesn't

> > > attempt to get and enable the clock for audio_clk_a. The choice to

> > > use a fixed-factor-clock was due to the fact that it will automatically

> > > enable the programmable clock frequency without change any code.

> > >

> > > Signed-off-by: Adam Ford <aford173@gmail.com>

> >

> > Thanks for your patch!

> >

> > > --- a/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi

> > > +++ b/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi

> > > @@ -250,9 +250,12 @@ ss_ep: endpoint {

> > >  };

> > >

> > >  &audio_clk_a {

> > > -       clock-frequency = <24576000>;

> > > -       assigned-clocks = <&versaclock6_bb 4>;

> > > -       assigned-clock-rates = <24576000>;

> > > +       /delete-property/ clock-frequency;

> > > +       #clock-cells = <0>;

> > > +       compatible = "fixed-factor-clock";

> > > +       clock-mult = <1>;

> > > +       clock-div = <1>;

> > > +       clocks = <&versaclock6_bb 4>;

> > >  };

> >

> > Shouldn't you override the clocks property in the rcar_sound node

> > instead, like is done in several other board DTS files (with cs2000)?

> >

>

> I guess there are multiple ways to do this.  Because the rcar_sound

> was already expecting a reference to audio_clk_a, it seemed less

> intrusive this way. The way I proposed, we can use the default

> rcar_sound clocking and just change the audio_clk node to enable the

> versaclock output.  The versaclock is driving the audio_clk_a

> reference clock, so it seemed appropriate to put it there.

>

> If you want me to change, I will.


Taking a fresh look at this, I start to like it.
What do other people think?

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
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi b/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
index 3c84e060c69f..5c09e64001cc 100644
--- a/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
+++ b/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
@@ -250,9 +250,12 @@  ss_ep: endpoint {
 };
 
 &audio_clk_a {
-	clock-frequency = <24576000>;
-	assigned-clocks = <&versaclock6_bb 4>;
-	assigned-clock-rates = <24576000>;
+	/delete-property/ clock-frequency;
+	#clock-cells = <0>;
+	compatible = "fixed-factor-clock";
+	clock-mult = <1>;
+	clock-div = <1>;
+	clocks = <&versaclock6_bb 4>;
 };
 
 &audio_clk_b {
@@ -591,7 +594,7 @@  sound_pins: sound {
 	};
 
 	sound_clk_pins: sound_clk {
-		groups = "audio_clk_a_a";
+		groups = "audio_clk_a_a", "audio_clk_b_a";
 		function = "audio_clk";
 	};