| Message ID | 20201213183759.223246-5-aford173@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | arm64: dts: renesas: Cleanup Beacon Kit and support more SoC's | expand |
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
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
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
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"; };
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(-)