| Message ID | 20240314232201.2102178-1-jan.dakinevich@salutedevices.com |
|---|---|
| Headers | show |
| Series | Introduce support of audio for Amlogic A1 SoC family | expand |
On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > Existing values were insufficient to produce accurate clock for audio > devices. New values are safe and most suitable to produce 48000Hz sample > rate. The hifi pll is not about 48k only. I see no reason to restrict the PLL to a single setting. You've provided no justification why the PLL driver can't reach the same setting for 48k. The setting below is just the crude part. the fine tuning is done done with the frac parameter so I doubt this provides a more accurate rate. > > Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> > --- > drivers/clk/meson/a1-pll.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c > index 4325e8a6a3ef..00e06d03445b 100644 > --- a/drivers/clk/meson/a1-pll.c > +++ b/drivers/clk/meson/a1-pll.c > @@ -74,9 +74,9 @@ static struct clk_regmap fixed_pll = { > }, > }; > > -static const struct pll_mult_range hifi_pll_mult_range = { > - .min = 32, > - .max = 64, > +static const struct pll_params_table hifi_pll_params_table[] = { > + PLL_PARAMS(128, 5), > + { }, > }; > > static const struct reg_sequence hifi_init_regs[] = { > @@ -124,7 +124,7 @@ static struct clk_regmap hifi_pll = { > .shift = 6, > .width = 1, > }, > - .range = &hifi_pll_mult_range, > + .table = hifi_pll_params_table, > .init_regs = hifi_init_regs, > .init_count = ARRAY_SIZE(hifi_init_regs), > },
On 15/03/2024 00:21, Jan Dakinevich wrote: > Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> You must provide commit messages. A nit, subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > --- > .../bindings/clock/amlogic,a1-audio-clkc.yaml | 83 ++++++++++++ > .../dt-bindings/clock/amlogic,a1-audio-clkc.h | 122 ++++++++++++++++++ > .../reset/amlogic,meson-a1-audio-reset.h | 29 +++++ > 3 files changed, 234 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > create mode 100644 include/dt-bindings/clock/amlogic,a1-audio-clkc.h > create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h > > diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > new file mode 100644 > index 000000000000..c76cad4da493 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > @@ -0,0 +1,83 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/amlogic,a1-audio-clkc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Amlogic A1 Audio Clock Control Unit and Reset Controller > + > +maintainers: > + - Neil Armstrong <neil.armstrong@linaro.org> > + - Jerome Brunet <jbrunet@baylibre.com> > + - Jan Dakinevich <jan.dakinevich@salutedevices.com> > + > +properties: > + compatible: > + const: amlogic,a1-audio-clkc > + > + '#clock-cells': > + const: 1 > + > + '#reset-cells': > + const: 1 > + > + reg: > + minItems: 2 Drop > + maxItems: 2 > + > + clocks: > + items: > + - description: input main peripheral bus clock > + - description: input dds_in > + - description: input fixed pll div2 > + - description: input fixed pll div3 > + - description: input hifi_pll > + - description: input oscillator (usually at 24MHz) > + > + clock-names: > + items: > + - const: pclk > + - const: dds_in > + - const: fclk_div2 > + - const: fclk_div3 > + - const: hifi_pll > + - const: xtal > + > +required: > + - compatible > + - '#clock-cells' > + - '#reset-cells' > + - reg > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/amlogic,a1-pll-clkc.h> > + #include <dt-bindings/clock/amlogic,a1-peripherals-clkc.h> > + audio { > + #address-cells = <2>; > + #size-cells = <2>; > + > + clkc_audio: audio-clock-controller@0 { > + compatible = "amlogic,a1-audio-clkc"; > + reg = <0x0 0xfe050000 0x0 0xb0>, Messed indentayion. Use 4 spaces for example indentation. > + <0x0 0xfe054800 0x0 0x20>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + clocks = <&clkc_periphs CLKID_AUDIO>, > + <&clkc_periphs CLKID_DDS_IN>, > + <&clkc_pll CLKID_FCLK_DIV2>, > + <&clkc_pll CLKID_FCLK_DIV3>, > + <&clkc_pll CLKID_HIFI_PLL>,
On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > This series includes the following: > > - new audio clock and reset controller data and adaptation for it of existing > code (patches 0001..0004); > > - adaptation of existing audio components for A1 Soc (patches 0005..0021); > > - handy cosmetics for dai-link naming (patches 0022..0023); > > - integration of audio devices into common trees (patch 0024); > > - audio support bring up on Amlogic ad402 reference board (patch 0025). This > patch is not actually checked on real hardware (because all ad402 that we had > were burned out). This patch is based on ad402's schematics and on experience > with our own hardware (which is very close to reference board); > > Dmitry Rokosov (2): > ASoC: dt-bindings: meson: introduce link-name optional property > ASoC: meson: implement link-name optional property in meson card utils > > Jan Dakinevich (23): > clk: meson: a1: restrict an amount of 'hifi_pll' params > clk: meson: axg: move reset controller's code to separate module > dt-bindings: clock: meson: add A1 audio clock and reset controller > bindings > clk: meson: a1: add the audio clock controller driver > ASoC: meson: codec-glue: add support for capture stream > ASoC: meson: g12a-toacodec: fix "Lane Select" width > ASoC: meson: g12a-toacodec: rework the definition of bits > ASoC: dt-bindings: meson: g12a-toacodec: add support for A1 SoC family > ASoC: meson: g12a-toacodec: add support for A1 SoC family > ASoC: meson: t9015: prepare to adding new platforms > ASoC: dt-bindings: meson: t9015: add support for A1 SoC family > ASoC: meson: t9015: add support for A1 SoC family > ASoC: dt-bindings: meson: axg-pdm: document 'sysrate' property > ASoC: meson: axg-pdm: introduce 'sysrate' property > pinctrl/meson: fix typo in PDM's pin name > ASoC: dt-bindings: meson: meson-axg-audio-arb: claim support of A1 SoC > family > ASoC: dt-bindings: meson: axg-fifo: claim support of A1 SoC family > ASoC: dt-bindings: meson: axg-pdm: claim support of A1 SoC family > ASoC: dt-bindings: meson: axg-sound-card: claim support of A1 SoC > family > ASoC: dt-bindings: meson: axg-tdm-formatters: claim support of A1 SoC > family > ASoC: dt-bindings: meson: axg-tdm-iface: claim support of A1 SoC > family > arm64: dts: meson: a1: add audio devices > arm64: dts: ad402: enable audio I'm sorry but a 25 patches series is just way too big, especially when spamming multiple sub systems. Please try to make one series per sub systems and general topic, I see at least * A1 audio clocks * G12 acodec fix * Acodec rework * PDM * pinctrl * arm64 I did not review all but I think I've made enough comments to keep you busy for a while > > .../bindings/clock/amlogic,a1-audio-clkc.yaml | 83 +++ > .../reset/amlogic,meson-axg-audio-arb.yaml | 10 +- > .../bindings/sound/amlogic,axg-fifo.yaml | 8 + > .../bindings/sound/amlogic,axg-pdm.yaml | 5 + > .../sound/amlogic,axg-sound-card.yaml | 12 +- > .../sound/amlogic,axg-tdm-formatters.yaml | 22 +- > .../bindings/sound/amlogic,axg-tdm-iface.yaml | 6 +- > .../bindings/sound/amlogic,g12a-toacodec.yaml | 1 + > .../bindings/sound/amlogic,gx-sound-card.yaml | 6 + > .../bindings/sound/amlogic,t9015.yaml | 4 +- > .../arm64/boot/dts/amlogic/meson-a1-ad402.dts | 126 ++++ > arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 471 +++++++++++++++ > drivers/clk/meson/Kconfig | 18 + > drivers/clk/meson/Makefile | 2 + > drivers/clk/meson/a1-audio.c | 556 ++++++++++++++++++ > drivers/clk/meson/a1-audio.h | 58 ++ > drivers/clk/meson/a1-pll.c | 8 +- > drivers/clk/meson/axg-audio.c | 95 +-- > drivers/clk/meson/meson-audio-rstc.c | 109 ++++ > drivers/clk/meson/meson-audio-rstc.h | 12 + > drivers/pinctrl/meson/pinctrl-meson-a1.c | 6 +- > .../dt-bindings/clock/amlogic,a1-audio-clkc.h | 122 ++++ > .../reset/amlogic,meson-a1-audio-reset.h | 29 + > .../dt-bindings/sound/meson-g12a-toacodec.h | 5 + > sound/soc/meson/axg-pdm.c | 10 +- > sound/soc/meson/g12a-toacodec.c | 298 ++++++++-- > sound/soc/meson/meson-card-utils.c | 12 +- > sound/soc/meson/meson-codec-glue.c | 174 ++++-- > sound/soc/meson/meson-codec-glue.h | 23 + > sound/soc/meson/t9015.c | 326 +++++++++- > 30 files changed, 2394 insertions(+), 223 deletions(-) > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > create mode 100644 drivers/clk/meson/a1-audio.c > create mode 100644 drivers/clk/meson/a1-audio.h > create mode 100644 drivers/clk/meson/meson-audio-rstc.c > create mode 100644 drivers/clk/meson/meson-audio-rstc.h > create mode 100644 include/dt-bindings/clock/amlogic,a1-audio-clkc.h > create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h
On Fri, Mar 15, 2024 at 02:21:48AM +0300, Jan Dakinevich wrote: > +static const char * const a1_adc_mic_bias_level_txt[] = { "2.0V", "2.1V", > + "2.3V", "2.5V", "2.8V" }; > +static const unsigned int a1_adc_mic_bias_level_values[] = { 0, 1, 2, 3, 7 }; Why would this be varied at runtime rather than being something fixed when the system is designed? > +static const char * const a1_adc_pga_txt[] = { "None", "Differential", > + "Positive", "Negative" }; > +static const unsigned int a1_adc_pga_right_values[] = { 0, PGAR_DIFF, > + PGAR_POSITIVE, PGAR_NEGATIVE }; > +static const unsigned int a1_adc_pga_left_values[] = { 0, PGAL_DIFF, > + PGAL_POSITIVE, PGAL_NEGATIVE }; Similarly here. > + SOC_SINGLE("ADC Mic Bias Switch", LINEIN_CFG, MICBIAS_EN, 1, 0), > + SOC_ENUM("ADC Mic Bias Level", a1_adc_mic_bias_level), Why would micbias be user controlled rather than a DAPM widget as normal?
On Fri, 15 Mar 2024 02:21:36 +0300, Jan Dakinevich wrote: > This series includes the following: > > - new audio clock and reset controller data and adaptation for it of existing > code (patches 0001..0004); > > - adaptation of existing audio components for A1 Soc (patches 0005..0021); > > - handy cosmetics for dai-link naming (patches 0022..0023); > > - integration of audio devices into common trees (patch 0024); > > - audio support bring up on Amlogic ad402 reference board (patch 0025). This > patch is not actually checked on real hardware (because all ad402 that we had > were burned out). This patch is based on ad402's schematics and on experience > with our own hardware (which is very close to reference board); > > Dmitry Rokosov (2): > ASoC: dt-bindings: meson: introduce link-name optional property > ASoC: meson: implement link-name optional property in meson card utils > > Jan Dakinevich (23): > clk: meson: a1: restrict an amount of 'hifi_pll' params > clk: meson: axg: move reset controller's code to separate module > dt-bindings: clock: meson: add A1 audio clock and reset controller > bindings > clk: meson: a1: add the audio clock controller driver > ASoC: meson: codec-glue: add support for capture stream > ASoC: meson: g12a-toacodec: fix "Lane Select" width > ASoC: meson: g12a-toacodec: rework the definition of bits > ASoC: dt-bindings: meson: g12a-toacodec: add support for A1 SoC family > ASoC: meson: g12a-toacodec: add support for A1 SoC family > ASoC: meson: t9015: prepare to adding new platforms > ASoC: dt-bindings: meson: t9015: add support for A1 SoC family > ASoC: meson: t9015: add support for A1 SoC family > ASoC: dt-bindings: meson: axg-pdm: document 'sysrate' property > ASoC: meson: axg-pdm: introduce 'sysrate' property > pinctrl/meson: fix typo in PDM's pin name > ASoC: dt-bindings: meson: meson-axg-audio-arb: claim support of A1 SoC > family > ASoC: dt-bindings: meson: axg-fifo: claim support of A1 SoC family > ASoC: dt-bindings: meson: axg-pdm: claim support of A1 SoC family > ASoC: dt-bindings: meson: axg-sound-card: claim support of A1 SoC > family > ASoC: dt-bindings: meson: axg-tdm-formatters: claim support of A1 SoC > family > ASoC: dt-bindings: meson: axg-tdm-iface: claim support of A1 SoC > family > arm64: dts: meson: a1: add audio devices > arm64: dts: ad402: enable audio > > .../bindings/clock/amlogic,a1-audio-clkc.yaml | 83 +++ > .../reset/amlogic,meson-axg-audio-arb.yaml | 10 +- > .../bindings/sound/amlogic,axg-fifo.yaml | 8 + > .../bindings/sound/amlogic,axg-pdm.yaml | 5 + > .../sound/amlogic,axg-sound-card.yaml | 12 +- > .../sound/amlogic,axg-tdm-formatters.yaml | 22 +- > .../bindings/sound/amlogic,axg-tdm-iface.yaml | 6 +- > .../bindings/sound/amlogic,g12a-toacodec.yaml | 1 + > .../bindings/sound/amlogic,gx-sound-card.yaml | 6 + > .../bindings/sound/amlogic,t9015.yaml | 4 +- > .../arm64/boot/dts/amlogic/meson-a1-ad402.dts | 126 ++++ > arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 471 +++++++++++++++ > drivers/clk/meson/Kconfig | 18 + > drivers/clk/meson/Makefile | 2 + > drivers/clk/meson/a1-audio.c | 556 ++++++++++++++++++ > drivers/clk/meson/a1-audio.h | 58 ++ > drivers/clk/meson/a1-pll.c | 8 +- > drivers/clk/meson/axg-audio.c | 95 +-- > drivers/clk/meson/meson-audio-rstc.c | 109 ++++ > drivers/clk/meson/meson-audio-rstc.h | 12 + > drivers/pinctrl/meson/pinctrl-meson-a1.c | 6 +- > .../dt-bindings/clock/amlogic,a1-audio-clkc.h | 122 ++++ > .../reset/amlogic,meson-a1-audio-reset.h | 29 + > .../dt-bindings/sound/meson-g12a-toacodec.h | 5 + > sound/soc/meson/axg-pdm.c | 10 +- > sound/soc/meson/g12a-toacodec.c | 298 ++++++++-- > sound/soc/meson/meson-card-utils.c | 12 +- > sound/soc/meson/meson-codec-glue.c | 174 ++++-- > sound/soc/meson/meson-codec-glue.h | 23 + > sound/soc/meson/t9015.c | 326 +++++++++- > 30 files changed, 2394 insertions(+), 223 deletions(-) > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > create mode 100644 drivers/clk/meson/a1-audio.c > create mode 100644 drivers/clk/meson/a1-audio.h > create mode 100644 drivers/clk/meson/meson-audio-rstc.c > create mode 100644 drivers/clk/meson/meson-audio-rstc.h > create mode 100644 include/dt-bindings/clock/amlogic,a1-audio-clkc.h > create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h > > -- > 2.34.1 > > > My bot found new DTB warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade New warnings running 'make CHECK_DTBS=y amlogic/meson-a1-ad402.dtb' for 20240314232201.2102178-1-jan.dakinevich@salutedevices.com: arch/arm64/boot/dts/amlogic/meson-a1-ad402.dtb: audio-controller@4800: 'power-domains' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/sound/amlogic,t9015.yaml# arch/arm64/boot/dts/amlogic/meson-a1-ad402.dtb: audio-controller@1000: Unevaluated properties are not allowed ('power-domains' was unexpected) from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-pdm.yaml#
Hi Jan! On 15/03/2024 00:21, Jan Dakinevich wrote: > This series includes the following: > > - new audio clock and reset controller data and adaptation for it of existing > code (patches 0001..0004); > > - adaptation of existing audio components for A1 Soc (patches 0005..0021); > > - handy cosmetics for dai-link naming (patches 0022..0023); > > - integration of audio devices into common trees (patch 0024); > > - audio support bring up on Amlogic ad402 reference board (patch 0025). This > patch is not actually checked on real hardware (because all ad402 that we had > were burned out). This patch is based on ad402's schematics and on experience > with our own hardware (which is very close to reference board); Thanks for your serie, it's nice you're working on upstreaming this feature. In my opinion it's fine to have a "big" initial serie if you're not sure if your changes are ok, but next time add the RFC tag so we know it's not a final changeset and you seek advices. Overall the code is clean and your patch order makes sense if they were meant to be applied by a single maintainer, but in this case it will be split into multiple subsystems so it's better to split them as Jerome explained to ease review and the maintainers process. Don't hesitate discussing with us in the #linux-amlogic IRC channel on Libera.Chat, the goal is to reduce the number of patch version and ease the review and maintainance process. Concerning the link-name property, I think it should be done afterwards since it's not necessary to support audio on A1, and I think it could be extended to other SoC boards (which would be a great feature). Neil > > Dmitry Rokosov (2): > ASoC: dt-bindings: meson: introduce link-name optional property > ASoC: meson: implement link-name optional property in meson card utils > > Jan Dakinevich (23): > clk: meson: a1: restrict an amount of 'hifi_pll' params > clk: meson: axg: move reset controller's code to separate module > dt-bindings: clock: meson: add A1 audio clock and reset controller > bindings > clk: meson: a1: add the audio clock controller driver > ASoC: meson: codec-glue: add support for capture stream > ASoC: meson: g12a-toacodec: fix "Lane Select" width > ASoC: meson: g12a-toacodec: rework the definition of bits > ASoC: dt-bindings: meson: g12a-toacodec: add support for A1 SoC family > ASoC: meson: g12a-toacodec: add support for A1 SoC family > ASoC: meson: t9015: prepare to adding new platforms > ASoC: dt-bindings: meson: t9015: add support for A1 SoC family > ASoC: meson: t9015: add support for A1 SoC family > ASoC: dt-bindings: meson: axg-pdm: document 'sysrate' property > ASoC: meson: axg-pdm: introduce 'sysrate' property > pinctrl/meson: fix typo in PDM's pin name > ASoC: dt-bindings: meson: meson-axg-audio-arb: claim support of A1 SoC > family > ASoC: dt-bindings: meson: axg-fifo: claim support of A1 SoC family > ASoC: dt-bindings: meson: axg-pdm: claim support of A1 SoC family > ASoC: dt-bindings: meson: axg-sound-card: claim support of A1 SoC > family > ASoC: dt-bindings: meson: axg-tdm-formatters: claim support of A1 SoC > family > ASoC: dt-bindings: meson: axg-tdm-iface: claim support of A1 SoC > family > arm64: dts: meson: a1: add audio devices > arm64: dts: ad402: enable audio > > .../bindings/clock/amlogic,a1-audio-clkc.yaml | 83 +++ > .../reset/amlogic,meson-axg-audio-arb.yaml | 10 +- > .../bindings/sound/amlogic,axg-fifo.yaml | 8 + > .../bindings/sound/amlogic,axg-pdm.yaml | 5 + > .../sound/amlogic,axg-sound-card.yaml | 12 +- > .../sound/amlogic,axg-tdm-formatters.yaml | 22 +- > .../bindings/sound/amlogic,axg-tdm-iface.yaml | 6 +- > .../bindings/sound/amlogic,g12a-toacodec.yaml | 1 + > .../bindings/sound/amlogic,gx-sound-card.yaml | 6 + > .../bindings/sound/amlogic,t9015.yaml | 4 +- > .../arm64/boot/dts/amlogic/meson-a1-ad402.dts | 126 ++++ > arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 471 +++++++++++++++ > drivers/clk/meson/Kconfig | 18 + > drivers/clk/meson/Makefile | 2 + > drivers/clk/meson/a1-audio.c | 556 ++++++++++++++++++ > drivers/clk/meson/a1-audio.h | 58 ++ > drivers/clk/meson/a1-pll.c | 8 +- > drivers/clk/meson/axg-audio.c | 95 +-- > drivers/clk/meson/meson-audio-rstc.c | 109 ++++ > drivers/clk/meson/meson-audio-rstc.h | 12 + > drivers/pinctrl/meson/pinctrl-meson-a1.c | 6 +- > .../dt-bindings/clock/amlogic,a1-audio-clkc.h | 122 ++++ > .../reset/amlogic,meson-a1-audio-reset.h | 29 + > .../dt-bindings/sound/meson-g12a-toacodec.h | 5 + > sound/soc/meson/axg-pdm.c | 10 +- > sound/soc/meson/g12a-toacodec.c | 298 ++++++++-- > sound/soc/meson/meson-card-utils.c | 12 +- > sound/soc/meson/meson-codec-glue.c | 174 ++++-- > sound/soc/meson/meson-codec-glue.h | 23 + > sound/soc/meson/t9015.c | 326 +++++++++- > 30 files changed, 2394 insertions(+), 223 deletions(-) > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > create mode 100644 drivers/clk/meson/a1-audio.c > create mode 100644 drivers/clk/meson/a1-audio.h > create mode 100644 drivers/clk/meson/meson-audio-rstc.c > create mode 100644 drivers/clk/meson/meson-audio-rstc.h > create mode 100644 include/dt-bindings/clock/amlogic,a1-audio-clkc.h > create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h >
On 3/15/24 11:58, Jerome Brunet wrote: > > On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> Existing values were insufficient to produce accurate clock for audio >> devices. New values are safe and most suitable to produce 48000Hz sample >> rate. > > The hifi pll is not about 48k only. I see no reason to restrict the PLL > to a single setting. > > You've provided no justification why the PLL driver can't reach the same > setting for 48k. The setting below is just the crude part. the fine > tuning is done done with the frac parameter so I doubt this provides a > more accurate rate. > You are right, it is not about 48k only. However, there are two issues. First, indeed, I could just extend the range of multipliers to 1..255. But I am unsure if hifi_pll is able to handle whole range of mulptipliers. The value 128 is taken from Amlogic's branch, and I am pretty sure that it works. Second, unfortunately frac parameter currently doesn't work. When frac is used enabling of hifi_pll fails in meson_clk_pll_wait_lock(). I see it when try to use 44100Hz and multipliers' range is set to 1..255. So, support of other rates than 48k requires extra effort. >> >> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> --- >> drivers/clk/meson/a1-pll.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c >> index 4325e8a6a3ef..00e06d03445b 100644 >> --- a/drivers/clk/meson/a1-pll.c >> +++ b/drivers/clk/meson/a1-pll.c >> @@ -74,9 +74,9 @@ static struct clk_regmap fixed_pll = { >> }, >> }; >> >> -static const struct pll_mult_range hifi_pll_mult_range = { >> - .min = 32, >> - .max = 64, >> +static const struct pll_params_table hifi_pll_params_table[] = { >> + PLL_PARAMS(128, 5), >> + { }, >> }; >> >> static const struct reg_sequence hifi_init_regs[] = { >> @@ -124,7 +124,7 @@ static struct clk_regmap hifi_pll = { >> .shift = 6, >> .width = 1, >> }, >> - .range = &hifi_pll_mult_range, >> + .table = hifi_pll_params_table, >> .init_regs = hifi_init_regs, >> .init_count = ARRAY_SIZE(hifi_init_regs), >> }, > >
On 3/15/24 16:36, Mark Brown wrote: > On Fri, Mar 15, 2024 at 02:21:48AM +0300, Jan Dakinevich wrote: > >> +static const char * const a1_adc_mic_bias_level_txt[] = { "2.0V", "2.1V", >> + "2.3V", "2.5V", "2.8V" }; >> +static const unsigned int a1_adc_mic_bias_level_values[] = { 0, 1, 2, 3, 7 }; > > Why would this be varied at runtime rather than being something fixed > when the system is designed? > >> +static const char * const a1_adc_pga_txt[] = { "None", "Differential", >> + "Positive", "Negative" }; >> +static const unsigned int a1_adc_pga_right_values[] = { 0, PGAR_DIFF, >> + PGAR_POSITIVE, PGAR_NEGATIVE }; >> +static const unsigned int a1_adc_pga_left_values[] = { 0, PGAL_DIFF, >> + PGAL_POSITIVE, PGAL_NEGATIVE }; > > Similarly here. > Both mic bias and ADC's input mode depends on schematics and should be configurable. What is the better way to give access to these parameters? Device tree? >> + SOC_SINGLE("ADC Mic Bias Switch", LINEIN_CFG, MICBIAS_EN, 1, 0), >> + SOC_ENUM("ADC Mic Bias Level", a1_adc_mic_bias_level), > > Why would micbias be user controlled rather than a DAPM widget as > normal? Yes, I could use SND_SOC_DAPM_SUPPLY, but it supports only raw values, and doesn't supports enums. Here, I want to use enum to restrict possible values, because only these values mentioned in the documentation that I have.
Hello Neil, On Fri, Mar 15, 2024 at 05:53:05PM +0100, Neil Armstrong wrote: > Hi Jan! > > On 15/03/2024 00:21, Jan Dakinevich wrote: > > This series includes the following: > > > > - new audio clock and reset controller data and adaptation for it of existing > > code (patches 0001..0004); > > > > - adaptation of existing audio components for A1 Soc (patches 0005..0021); > > > > - handy cosmetics for dai-link naming (patches 0022..0023); > > > > - integration of audio devices into common trees (patch 0024); > > > > - audio support bring up on Amlogic ad402 reference board (patch 0025). This > > patch is not actually checked on real hardware (because all ad402 that we had > > were burned out). This patch is based on ad402's schematics and on experience > > with our own hardware (which is very close to reference board); > > Thanks for your serie, it's nice you're working on upstreaming this feature. > > In my opinion it's fine to have a "big" initial serie if you're not sure > if your changes are ok, but next time add the RFC tag so we know it's not > a final changeset and you seek advices. > > Overall the code is clean and your patch order makes sense if they were meant > to be applied by a single maintainer, but in this case it will be split > into multiple subsystems so it's better to split them as Jerome explained > to ease review and the maintainers process. > > Don't hesitate discussing with us in the #linux-amlogic IRC channel > on Libera.Chat, the goal is to reduce the number of patch version and > ease the review and maintainance process. > > Concerning the link-name property, I think it should be done afterwards > since it's not necessary to support audio on A1, and I think it could > be extended to other SoC boards (which would be a great feature). If you don't mind, I will send this change in a separate patch series. Although I don't have support for all boards in the linux-amlogic, I can test it on some Khadas and Odroid boards on my side. I will prepare link names for them. > > > > Dmitry Rokosov (2): > > ASoC: dt-bindings: meson: introduce link-name optional property > > ASoC: meson: implement link-name optional property in meson card utils > > > > Jan Dakinevich (23): > > clk: meson: a1: restrict an amount of 'hifi_pll' params > > clk: meson: axg: move reset controller's code to separate module > > dt-bindings: clock: meson: add A1 audio clock and reset controller > > bindings > > clk: meson: a1: add the audio clock controller driver > > ASoC: meson: codec-glue: add support for capture stream > > ASoC: meson: g12a-toacodec: fix "Lane Select" width > > ASoC: meson: g12a-toacodec: rework the definition of bits > > ASoC: dt-bindings: meson: g12a-toacodec: add support for A1 SoC family > > ASoC: meson: g12a-toacodec: add support for A1 SoC family > > ASoC: meson: t9015: prepare to adding new platforms > > ASoC: dt-bindings: meson: t9015: add support for A1 SoC family > > ASoC: meson: t9015: add support for A1 SoC family > > ASoC: dt-bindings: meson: axg-pdm: document 'sysrate' property > > ASoC: meson: axg-pdm: introduce 'sysrate' property > > pinctrl/meson: fix typo in PDM's pin name > > ASoC: dt-bindings: meson: meson-axg-audio-arb: claim support of A1 SoC > > family > > ASoC: dt-bindings: meson: axg-fifo: claim support of A1 SoC family > > ASoC: dt-bindings: meson: axg-pdm: claim support of A1 SoC family > > ASoC: dt-bindings: meson: axg-sound-card: claim support of A1 SoC > > family > > ASoC: dt-bindings: meson: axg-tdm-formatters: claim support of A1 SoC > > family > > ASoC: dt-bindings: meson: axg-tdm-iface: claim support of A1 SoC > > family > > arm64: dts: meson: a1: add audio devices > > arm64: dts: ad402: enable audio > > > > .../bindings/clock/amlogic,a1-audio-clkc.yaml | 83 +++ > > .../reset/amlogic,meson-axg-audio-arb.yaml | 10 +- > > .../bindings/sound/amlogic,axg-fifo.yaml | 8 + > > .../bindings/sound/amlogic,axg-pdm.yaml | 5 + > > .../sound/amlogic,axg-sound-card.yaml | 12 +- > > .../sound/amlogic,axg-tdm-formatters.yaml | 22 +- > > .../bindings/sound/amlogic,axg-tdm-iface.yaml | 6 +- > > .../bindings/sound/amlogic,g12a-toacodec.yaml | 1 + > > .../bindings/sound/amlogic,gx-sound-card.yaml | 6 + > > .../bindings/sound/amlogic,t9015.yaml | 4 +- > > .../arm64/boot/dts/amlogic/meson-a1-ad402.dts | 126 ++++ > > arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 471 +++++++++++++++ > > drivers/clk/meson/Kconfig | 18 + > > drivers/clk/meson/Makefile | 2 + > > drivers/clk/meson/a1-audio.c | 556 ++++++++++++++++++ > > drivers/clk/meson/a1-audio.h | 58 ++ > > drivers/clk/meson/a1-pll.c | 8 +- > > drivers/clk/meson/axg-audio.c | 95 +-- > > drivers/clk/meson/meson-audio-rstc.c | 109 ++++ > > drivers/clk/meson/meson-audio-rstc.h | 12 + > > drivers/pinctrl/meson/pinctrl-meson-a1.c | 6 +- > > .../dt-bindings/clock/amlogic,a1-audio-clkc.h | 122 ++++ > > .../reset/amlogic,meson-a1-audio-reset.h | 29 + > > .../dt-bindings/sound/meson-g12a-toacodec.h | 5 + > > sound/soc/meson/axg-pdm.c | 10 +- > > sound/soc/meson/g12a-toacodec.c | 298 ++++++++-- > > sound/soc/meson/meson-card-utils.c | 12 +- > > sound/soc/meson/meson-codec-glue.c | 174 ++++-- > > sound/soc/meson/meson-codec-glue.h | 23 + > > sound/soc/meson/t9015.c | 326 +++++++++- > > 30 files changed, 2394 insertions(+), 223 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > > create mode 100644 drivers/clk/meson/a1-audio.c > > create mode 100644 drivers/clk/meson/a1-audio.h > > create mode 100644 drivers/clk/meson/meson-audio-rstc.c > > create mode 100644 drivers/clk/meson/meson-audio-rstc.h > > create mode 100644 include/dt-bindings/clock/amlogic,a1-audio-clkc.h > > create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h > > >
On Sun 17 Mar 2024 at 17:17, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > On 3/15/24 11:58, Jerome Brunet wrote: >> >> On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: >> >>> Existing values were insufficient to produce accurate clock for audio >>> devices. New values are safe and most suitable to produce 48000Hz sample >>> rate. >> >> The hifi pll is not about 48k only. I see no reason to restrict the PLL >> to a single setting. >> > You've provided no justification why the PLL driver can't reach the same >> setting for 48k. The setting below is just the crude part. the fine >> tuning is done done with the frac parameter so I doubt this provides a >> more accurate rate. >> > > You are right, it is not about 48k only. However, there are two issues. > > First, indeed, I could just extend the range of multipliers to 1..255. Why 1..255 ? This is not what I'm pointing out According to the datasheet - the range is 32 - 64, as currently set in the driver. The change you have provided request a multipler of 128/5 = 25,6 If you put assigned-rate = 614400000 in DT, I see no reason can find the same solution on its own. > But I am unsure if hifi_pll is able to handle whole range of > mulptipliers. The value 128 is taken from Amlogic's branch, and I am > pretty sure that it works. > > Second, unfortunately frac parameter currently doesn't work. When frac > is used enabling of hifi_pll fails in meson_clk_pll_wait_lock(). I see > it when try to use 44100Hz and multipliers' range is set to 1..255. So, > support of other rates than 48k requires extra effort. Then your change is even more problematic because it certainly does not disable frac ... which you say is broken. That parameter should be removed with a proper comment explaining why you are disabling it. That type a limitation / known issue should be mentionned in your change. > >>> >>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>> --- >>> drivers/clk/meson/a1-pll.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c >>> index 4325e8a6a3ef..00e06d03445b 100644 >>> --- a/drivers/clk/meson/a1-pll.c >>> +++ b/drivers/clk/meson/a1-pll.c >>> @@ -74,9 +74,9 @@ static struct clk_regmap fixed_pll = { >>> }, >>> }; >>> >>> -static const struct pll_mult_range hifi_pll_mult_range = { >>> - .min = 32, >>> - .max = 64, >>> +static const struct pll_params_table hifi_pll_params_table[] = { >>> + PLL_PARAMS(128, 5), >>> + { }, >>> }; >>> >>> static const struct reg_sequence hifi_init_regs[] = { >>> @@ -124,7 +124,7 @@ static struct clk_regmap hifi_pll = { >>> .shift = 6, >>> .width = 1, >>> }, >>> - .range = &hifi_pll_mult_range, >>> + .table = hifi_pll_params_table, >>> .init_regs = hifi_init_regs, >>> .init_count = ARRAY_SIZE(hifi_init_regs), >>> }, >> >>
On Sun, Mar 17, 2024 at 07:27:14PM +0300, Jan Dakinevich wrote: > Both mic bias and ADC's input mode depends on schematics and should be > configurable. What is the better way to give access to these parameters? > Device tree? Yes. > >> + SOC_SINGLE("ADC Mic Bias Switch", LINEIN_CFG, MICBIAS_EN, 1, 0), > >> + SOC_ENUM("ADC Mic Bias Level", a1_adc_mic_bias_level), > > Why would micbias be user controlled rather than a DAPM widget as > > normal? > Yes, I could use SND_SOC_DAPM_SUPPLY, but it supports only raw values, > and doesn't supports enums. Here, I want to use enum to restrict > possible values, because only these values mentioned in the > documentation that I have. A supply is an on/off switch not an enum. Users should not be selecting values at all.
On 3/18/24 13:17, Jerome Brunet wrote: > > On Sun 17 Mar 2024 at 17:17, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> On 3/15/24 11:58, Jerome Brunet wrote: >>> >>> On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: >>> >>>> Existing values were insufficient to produce accurate clock for audio >>>> devices. New values are safe and most suitable to produce 48000Hz sample >>>> rate. >>> >>> The hifi pll is not about 48k only. I see no reason to restrict the PLL >>> to a single setting. >>>> You've provided no justification why the PLL driver can't reach the same >>> setting for 48k. The setting below is just the crude part. the fine >>> tuning is done done with the frac parameter so I doubt this provides a >>> more accurate rate. >>> >> >> You are right, it is not about 48k only. However, there are two issues. >> >> First, indeed, I could just extend the range of multipliers to 1..255. > > Why 1..255 ? This is not what I'm pointing out > > According to the datasheet - the range is 32 - 64, as currently > set in the driver. > Could you point where in the doc the range 32..64 is documented? Documentation that I have may be not so complete, but I don't see there any mention about it. Anyway, range 32..64 of multipliers is not enough to produce accurate clock, and a need 128 for 48kHz. > The change you have provided request a multipler of 128/5 = 25,6 > If you put assigned-rate = 614400000 in DT, I see no reason can find the > same solution on its own. > The reasoning is following. I don't know why 32..64 range was declared for this clock, and whether it would be safe to extend it and include 128, which is required for 48kHz. But I know, that multiplier=128 is safe and works fine (together divider=5). >> But I am unsure if hifi_pll is able to handle whole range of >> mulptipliers. The value 128 is taken from Amlogic's branch, and I am >> pretty sure that it works. > >> >> Second, unfortunately frac parameter currently doesn't work. When frac >> is used enabling of hifi_pll fails in meson_clk_pll_wait_lock(). I see >> it when try to use 44100Hz and multipliers' range is set to 1..255. So, >> support of other rates than 48k requires extra effort. > > Then your change is even more problematic because it certainly does not > disable frac ... which you say is broken. > > That parameter should be removed with a proper comment explaining why > you are disabling it. That type a limitation / known issue should be > mentionned in your change. > Handling of frac should not be removed, it should be fixed to achieve another rates. But that is not the goal of this commit. >> >>>> >>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>> --- >>>> drivers/clk/meson/a1-pll.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c >>>> index 4325e8a6a3ef..00e06d03445b 100644 >>>> --- a/drivers/clk/meson/a1-pll.c >>>> +++ b/drivers/clk/meson/a1-pll.c >>>> @@ -74,9 +74,9 @@ static struct clk_regmap fixed_pll = { >>>> }, >>>> }; >>>> >>>> -static const struct pll_mult_range hifi_pll_mult_range = { >>>> - .min = 32, >>>> - .max = 64, >>>> +static const struct pll_params_table hifi_pll_params_table[] = { >>>> + PLL_PARAMS(128, 5), >>>> + { }, >>>> }; >>>> >>>> static const struct reg_sequence hifi_init_regs[] = { >>>> @@ -124,7 +124,7 @@ static struct clk_regmap hifi_pll = { >>>> .shift = 6, >>>> .width = 1, >>>> }, >>>> - .range = &hifi_pll_mult_range, >>>> + .table = hifi_pll_params_table, >>>> .init_regs = hifi_init_regs, >>>> .init_count = ARRAY_SIZE(hifi_init_regs), >>>> }, >>> >>> > >
On 3/18/24 16:48, Mark Brown wrote: > On Sun, Mar 17, 2024 at 07:27:14PM +0300, Jan Dakinevich wrote: > >> Both mic bias and ADC's input mode depends on schematics and should be >> configurable. What is the better way to give access to these parameters? >> Device tree? > > Yes. > >>>> + SOC_SINGLE("ADC Mic Bias Switch", LINEIN_CFG, MICBIAS_EN, 1, 0), >>>> + SOC_ENUM("ADC Mic Bias Level", a1_adc_mic_bias_level), > >>> Why would micbias be user controlled rather than a DAPM widget as >>> normal? > >> Yes, I could use SND_SOC_DAPM_SUPPLY, but it supports only raw values, >> and doesn't supports enums. Here, I want to use enum to restrict >> possible values, because only these values mentioned in the >> documentation that I have. > > A supply is an on/off switch not an enum. Users should not be selecting > values at all. Ok. For me it is great if I am free to move these kcontrols to device tree.
On Tue 19 Mar 2024 at 01:35, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > On 3/18/24 13:17, Jerome Brunet wrote: >> >> On Sun 17 Mar 2024 at 17:17, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: >> >>> On 3/15/24 11:58, Jerome Brunet wrote: >>>> >>>> On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: >>>> >>>>> Existing values were insufficient to produce accurate clock for audio >>>>> devices. New values are safe and most suitable to produce 48000Hz sample >>>>> rate. >>>> >>>> The hifi pll is not about 48k only. I see no reason to restrict the PLL >>>> to a single setting. >>>>> You've provided no justification why the PLL driver can't reach the same >>>> setting for 48k. The setting below is just the crude part. the fine >>>> tuning is done done with the frac parameter so I doubt this provides a >>>> more accurate rate. >>>> >>> >>> You are right, it is not about 48k only. However, there are two issues. >>> >>> First, indeed, I could just extend the range of multipliers to 1..255. >> >> Why 1..255 ? This is not what I'm pointing out >> >> According to the datasheet - the range is 32 - 64, as currently >> set in the driver. >> > > Could you point where in the doc the range 32..64 is documented? > Documentation that I have may be not so complete, but I don't see there > any mention about it. > > Anyway, range 32..64 of multipliers is not enough to produce accurate > clock, and a need 128 for 48kHz. A1 datasheet v0.4 - Section 7.6.3.2 > >> The change you have provided request a multipler of 128/5 = 25,6 >> If you put assigned-rate = 614400000 in DT, I see no reason can find the >> same solution on its own. >> > > The reasoning is following. I don't know why 32..64 range was declared > for this clock, and whether it would be safe to extend it and include > 128, which is required for 48kHz. But I know, that multiplier=128 is > safe and works fine (together divider=5). You have not answer my remark. Mainline does not do everything like the AML SDK does. Saying you are copying it because you know it works (in your opinion) is not good enough. I'm telling you that your hack is not necessary and so far, you have not demonstrated that it is. Also the multiplier range in m/n, not m alone. > >>> But I am unsure if hifi_pll is able to handle whole range of >>> mulptipliers. The value 128 is taken from Amlogic's branch, and I am >>> pretty sure that it works. >> >>> >>> Second, unfortunately frac parameter currently doesn't work. When frac >>> is used enabling of hifi_pll fails in meson_clk_pll_wait_lock(). I see >>> it when try to use 44100Hz and multipliers' range is set to 1..255. So, >>> support of other rates than 48k requires extra effort. >> >> Then your change is even more problematic because it certainly does not >> disable frac ... which you say is broken. >> >> That parameter should be removed with a proper comment explaining why >> you are disabling it. That type a limitation / known issue should be >> mentionned in your change. >> > > Handling of frac should not be removed, it should be fixed to achieve > another rates. But that is not the goal of this commit. You argued that frac was broken and that was partly why you introduced this work around. I'm telling you this approach is incorrect. So either : * Remove frac for now, until it is fixed, because it is broken and add comment clearly explaining that quirk. * Or fix it now. Your choice. > > >>> >>>>> >>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>>> --- >>>>> drivers/clk/meson/a1-pll.c | 8 ++++---- >>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c >>>>> index 4325e8a6a3ef..00e06d03445b 100644 >>>>> --- a/drivers/clk/meson/a1-pll.c >>>>> +++ b/drivers/clk/meson/a1-pll.c >>>>> @@ -74,9 +74,9 @@ static struct clk_regmap fixed_pll = { >>>>> }, >>>>> }; >>>>> >>>>> -static const struct pll_mult_range hifi_pll_mult_range = { >>>>> - .min = 32, >>>>> - .max = 64, >>>>> +static const struct pll_params_table hifi_pll_params_table[] = { >>>>> + PLL_PARAMS(128, 5), >>>>> + { }, >>>>> }; >>>>> >>>>> static const struct reg_sequence hifi_init_regs[] = { >>>>> @@ -124,7 +124,7 @@ static struct clk_regmap hifi_pll = { >>>>> .shift = 6, >>>>> .width = 1, >>>>> }, >>>>> - .range = &hifi_pll_mult_range, >>>>> + .table = hifi_pll_params_table, >>>>> .init_regs = hifi_init_regs, >>>>> .init_count = ARRAY_SIZE(hifi_init_regs), >>>>> }, >>>> >>>> >> >>
On Tue, Mar 19, 2024 at 09:21:27AM +0100, Jerome Brunet wrote: > > On Tue 19 Mar 2024 at 01:35, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > > > On 3/18/24 13:17, Jerome Brunet wrote: > >> > >> On Sun 17 Mar 2024 at 17:17, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> > >>> On 3/15/24 11:58, Jerome Brunet wrote: > >>>> > >>>> On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >>>> > >>>>> Existing values were insufficient to produce accurate clock for audio > >>>>> devices. New values are safe and most suitable to produce 48000Hz sample > >>>>> rate. > >>>> > >>>> The hifi pll is not about 48k only. I see no reason to restrict the PLL > >>>> to a single setting. > >>>>> You've provided no justification why the PLL driver can't reach the same > >>>> setting for 48k. The setting below is just the crude part. the fine > >>>> tuning is done done with the frac parameter so I doubt this provides a > >>>> more accurate rate. > >>>> > >>> > >>> You are right, it is not about 48k only. However, there are two issues. > >>> > >>> First, indeed, I could just extend the range of multipliers to 1..255. > >> > >> Why 1..255 ? This is not what I'm pointing out > >> > >> According to the datasheet - the range is 32 - 64, as currently > >> set in the driver. > >> > > > > Could you point where in the doc the range 32..64 is documented? > > Documentation that I have may be not so complete, but I don't see there > > any mention about it. > > > > Anyway, range 32..64 of multipliers is not enough to produce accurate > > clock, and a need 128 for 48kHz. > > A1 datasheet v0.4 - Section 7.6.3.2 > > > > >> The change you have provided request a multipler of 128/5 = 25,6 > >> If you put assigned-rate = 614400000 in DT, I see no reason can find the > >> same solution on its own. > >> > > > > The reasoning is following. I don't know why 32..64 range was declared > > for this clock, and whether it would be safe to extend it and include > > 128, which is required for 48kHz. But I know, that multiplier=128 is > > safe and works fine (together divider=5). > > You have not answer my remark. > Mainline does not do everything like the AML SDK does. Saying you are > copying it because you know it works (in your opinion) is not good > enough. > > I'm telling you that your hack is not necessary and so far, you have not > demonstrated that it is. > > Also the multiplier range in m/n, not m alone. > > > > >>> But I am unsure if hifi_pll is able to handle whole range of > >>> mulptipliers. The value 128 is taken from Amlogic's branch, and I am > >>> pretty sure that it works. > >> > >>> > >>> Second, unfortunately frac parameter currently doesn't work. When frac > >>> is used enabling of hifi_pll fails in meson_clk_pll_wait_lock(). I see > >>> it when try to use 44100Hz and multipliers' range is set to 1..255. So, > >>> support of other rates than 48k requires extra effort. > >> > >> Then your change is even more problematic because it certainly does not > >> disable frac ... which you say is broken. > >> > >> That parameter should be removed with a proper comment explaining why > >> you are disabling it. That type a limitation / known issue should be > >> mentionned in your change. > >> > > > > Handling of frac should not be removed, it should be fixed to achieve > > another rates. But that is not the goal of this commit. > > You argued that frac was broken and that was partly why you introduced > this work around. I'm telling you this approach is incorrect. > > So either : > * Remove frac for now, until it is fixed, because it is broken and add > comment clearly explaining that quirk. > * Or fix it now. > > Your choice. > > > > > > >>> > >>>>> > >>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> > >>>>> --- > >>>>> drivers/clk/meson/a1-pll.c | 8 ++++---- > >>>>> 1 file changed, 4 insertions(+), 4 deletions(-) > >>>>> > >>>>> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c > >>>>> index 4325e8a6a3ef..00e06d03445b 100644 > >>>>> --- a/drivers/clk/meson/a1-pll.c > >>>>> +++ b/drivers/clk/meson/a1-pll.c > >>>>> @@ -74,9 +74,9 @@ static struct clk_regmap fixed_pll = { > >>>>> }, > >>>>> }; > >>>>> > >>>>> -static const struct pll_mult_range hifi_pll_mult_range = { > >>>>> - .min = 32, > >>>>> - .max = 64, > >>>>> +static const struct pll_params_table hifi_pll_params_table[] = { > >>>>> + PLL_PARAMS(128, 5), > >>>>> + { }, > >>>>> }; > >>>>> > >>>>> static const struct reg_sequence hifi_init_regs[] = { > >>>>> @@ -124,7 +124,7 @@ static struct clk_regmap hifi_pll = { > >>>>> .shift = 6, > >>>>> .width = 1, > >>>>> }, > >>>>> - .range = &hifi_pll_mult_range, > >>>>> + .table = hifi_pll_params_table, > >>>>> .init_regs = hifi_init_regs, > >>>>> .init_count = ARRAY_SIZE(hifi_init_regs), > >>>>> }, > >>>> > >>>> > >> > >> > > > -- > Jerome BTW, here Amlogic already mentioned all possible output audio rates for which hifipll can be used: https://lore.kernel.org/all/1569411888-98116-1-git-send-email-jian.hu@amlogic.com/T/#md7083b4f851ab97dfce43f8f6a3b266eb49ed329 ``` The audio working frequency are 44.1khz, 48khz and 192khz. 614.4M can meet the three frequency. after the hifi pll, there are two dividers in Audio clock. 614.4M/3200 = 192khz 614.4M/12800 = 48khz 614,4M/13932 = 44.0999khz ```
This series includes the following: - new audio clock and reset controller data and adaptation for it of existing code (patches 0001..0004); - adaptation of existing audio components for A1 Soc (patches 0005..0021); - handy cosmetics for dai-link naming (patches 0022..0023); - integration of audio devices into common trees (patch 0024); - audio support bring up on Amlogic ad402 reference board (patch 0025). This patch is not actually checked on real hardware (because all ad402 that we had were burned out). This patch is based on ad402's schematics and on experience with our own hardware (which is very close to reference board); Dmitry Rokosov (2): ASoC: dt-bindings: meson: introduce link-name optional property ASoC: meson: implement link-name optional property in meson card utils Jan Dakinevich (23): clk: meson: a1: restrict an amount of 'hifi_pll' params clk: meson: axg: move reset controller's code to separate module dt-bindings: clock: meson: add A1 audio clock and reset controller bindings clk: meson: a1: add the audio clock controller driver ASoC: meson: codec-glue: add support for capture stream ASoC: meson: g12a-toacodec: fix "Lane Select" width ASoC: meson: g12a-toacodec: rework the definition of bits ASoC: dt-bindings: meson: g12a-toacodec: add support for A1 SoC family ASoC: meson: g12a-toacodec: add support for A1 SoC family ASoC: meson: t9015: prepare to adding new platforms ASoC: dt-bindings: meson: t9015: add support for A1 SoC family ASoC: meson: t9015: add support for A1 SoC family ASoC: dt-bindings: meson: axg-pdm: document 'sysrate' property ASoC: meson: axg-pdm: introduce 'sysrate' property pinctrl/meson: fix typo in PDM's pin name ASoC: dt-bindings: meson: meson-axg-audio-arb: claim support of A1 SoC family ASoC: dt-bindings: meson: axg-fifo: claim support of A1 SoC family ASoC: dt-bindings: meson: axg-pdm: claim support of A1 SoC family ASoC: dt-bindings: meson: axg-sound-card: claim support of A1 SoC family ASoC: dt-bindings: meson: axg-tdm-formatters: claim support of A1 SoC family ASoC: dt-bindings: meson: axg-tdm-iface: claim support of A1 SoC family arm64: dts: meson: a1: add audio devices arm64: dts: ad402: enable audio .../bindings/clock/amlogic,a1-audio-clkc.yaml | 83 +++ .../reset/amlogic,meson-axg-audio-arb.yaml | 10 +- .../bindings/sound/amlogic,axg-fifo.yaml | 8 + .../bindings/sound/amlogic,axg-pdm.yaml | 5 + .../sound/amlogic,axg-sound-card.yaml | 12 +- .../sound/amlogic,axg-tdm-formatters.yaml | 22 +- .../bindings/sound/amlogic,axg-tdm-iface.yaml | 6 +- .../bindings/sound/amlogic,g12a-toacodec.yaml | 1 + .../bindings/sound/amlogic,gx-sound-card.yaml | 6 + .../bindings/sound/amlogic,t9015.yaml | 4 +- .../arm64/boot/dts/amlogic/meson-a1-ad402.dts | 126 ++++ arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 471 +++++++++++++++ drivers/clk/meson/Kconfig | 18 + drivers/clk/meson/Makefile | 2 + drivers/clk/meson/a1-audio.c | 556 ++++++++++++++++++ drivers/clk/meson/a1-audio.h | 58 ++ drivers/clk/meson/a1-pll.c | 8 +- drivers/clk/meson/axg-audio.c | 95 +-- drivers/clk/meson/meson-audio-rstc.c | 109 ++++ drivers/clk/meson/meson-audio-rstc.h | 12 + drivers/pinctrl/meson/pinctrl-meson-a1.c | 6 +- .../dt-bindings/clock/amlogic,a1-audio-clkc.h | 122 ++++ .../reset/amlogic,meson-a1-audio-reset.h | 29 + .../dt-bindings/sound/meson-g12a-toacodec.h | 5 + sound/soc/meson/axg-pdm.c | 10 +- sound/soc/meson/g12a-toacodec.c | 298 ++++++++-- sound/soc/meson/meson-card-utils.c | 12 +- sound/soc/meson/meson-codec-glue.c | 174 ++++-- sound/soc/meson/meson-codec-glue.h | 23 + sound/soc/meson/t9015.c | 326 +++++++++- 30 files changed, 2394 insertions(+), 223 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml create mode 100644 drivers/clk/meson/a1-audio.c create mode 100644 drivers/clk/meson/a1-audio.h create mode 100644 drivers/clk/meson/meson-audio-rstc.c create mode 100644 drivers/clk/meson/meson-audio-rstc.h create mode 100644 include/dt-bindings/clock/amlogic,a1-audio-clkc.h create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h