On 19/03/2024 04:57, Anjelique Melendez wrote:
>
>
> On 3/14/2024 2:20 PM, Krzysztof Kozlowski wrote:
>> On 14/03/2024 21:04, Anjelique Melendez wrote:
>>> Update the Qualcomm Technologies, Inc. PMIC GPIO binding documentation
>>> to include compatible strings for PMIH010x and PMD802x PMICs.
>>>
>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>> ---
>>> .../bindings/pinctrl/qcom,pmic-gpio.yaml | 20 +++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
>>> index 2b17d244f051..5cc04c016b25 100644
>>> --- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
>>> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
>>> @@ -57,10 +57,12 @@ properties:
>>> - qcom,pma8084-gpio
>>> - qcom,pmc8180-gpio
>>> - qcom,pmc8180c-gpio
>>> + - qcom,pmd802x-gpio
>>
>> Is the "x" some sort of wildcard or actual PMIC model/version name?
>> Wildcards are in general discouraged.
>>
>
> "x" is being used as a wildcard here so can update with actual PMIC version
> in next version.
>
Then please drop it also in all future submissions, as asked by writing
bindings.
Best regards,
Krzysztof
On 17/03/2024 16:52, Jan Dakinevich wrote: > > > On 3/15/24 13:22, Jerome Brunet wrote: >> >> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >> >>> On 15/03/2024 00:21, Jan Dakinevich wrote: >>>> This option allow to redefine the rate of DSP system clock. >>> >>> And why is it suitable for bindings? Describe the hardware, not what you >>> want to do in the driver. >>> >>>> >>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>> --- >>>> Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>> index df21dd72fc65..d2f23a59a6b6 100644 >>>> --- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>> +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>> @@ -40,6 +40,10 @@ properties: >>>> resets: >>>> maxItems: 1 >>>> >>>> + sysrate: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: redefine rate of DSP system clock >>> >>> No vendor prefix, so is it a generic property? Also, missing unit >>> suffix, but more importantly I don't understand why this is a property >>> of hardware. >> >> +1. >> >> The appropriate way to set rate of the clock before the driver take over >> is 'assigned-rate', if you need to customize this for different >> platform. >> > > It would be great, but it doesn't work. Below, is what I want to see: > > assigned-clocks = > <&clkc_audio AUD2_CLKID_PDM_SYSCLK_SEL>, > <&clkc_audio AUD2_CLKID_PDM_SYSCLK_DIV>; > assigned-clock-parents = > <&clkc_pll CLKID_FCLK_DIV3>, > <0>; > assigned-clock-rates = > <0>, > <256000000>; > > But regardles of this declaration, PDM's driver unconditionally sets That's driver's problem. You do not change bindings, just because your driver behaves differently. Just fix driver. > sysclk'rate to 250MHz and throws away everything that was configured > before, reparents audio2_pdm_sysclk_mux to hifi_pll and changes > hifi_pll's rate. > > This value 250MHz is declared here: > > static const struct axg_pdm_cfg axg_pdm_config = { > .filters = &axg_default_filters, > .sys_rate = 250000000, > }; > > The property 'sysrate' is intended to redefine hardcoded 'sys_rate' > value in 'axg_pdm_config'. What does it have to do with bindings? Change driver if you are not happy how it operates. Best regards, Krzysztof
On 17/03/2024 17:35, Jan Dakinevich wrote:
>
>
> On 3/17/24 19:27, Krzysztof Kozlowski wrote:
>> On 17/03/2024 16:55, Jan Dakinevich wrote:
>>>
>>>
>>> On 3/15/24 13:00, Krzysztof Kozlowski wrote:
>>>> On 15/03/2024 00:21, Jan Dakinevich wrote:
>>>>> This option allow to redefine the rate of DSP system clock.
>>>>
>>>> And why is it suitable for bindings? Describe the hardware, not what you
>>>> want to do in the driver.
>>>>
>>>
>>> What do you mean? I am adding some new property and should describe it
>>> in dt-bindinds. Isn't it?
>>
>> No, if the property is not suitable for bindings, you should not add it
>> in the first place. So again: explain what sort of hardware, not driver,
>> problem you are solving here, so we can understand why do you need new
>> property. Otherwise use existing properties or no properties, because we
>> do not define all possible clocks in the bindings.
>>
>> Let's be clear: with such commit msg explanation as you have, my answer
>> is: no, driver should set clock frequency and you do not need this
>> property at all.
>>
>
> Could you please take a look on answer to "Jerome Brunet
> <jbrunet@baylibre.com>"'s message on the same thread. There, I am trying
> to explain what I am solving by this commit.
How is this answer here? You asked "What do you mean", so apparently you
did not understand why I am responding and why you cannot just document
whatever you wish, because that "whatever you wish" is not correct. I
explained that but now you respond that I should read other part of
emails. Really?
So again, do you understand that commit msg should provide rationale why
you think this describes hardware and why this is suitable for bindings?
Best regards,
Krzysztof
On 3/14/2024 2:20 PM, Krzysztof Kozlowski wrote:
> On 14/03/2024 21:04, Anjelique Melendez wrote:
>> Update the Qualcomm Technologies, Inc. PMIC GPIO binding documentation
>> to include compatible strings for PMIH010x and PMD802x PMICs.
>>
>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> ---
>> .../bindings/pinctrl/qcom,pmic-gpio.yaml | 20 +++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
>> index 2b17d244f051..5cc04c016b25 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
>> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
>> @@ -57,10 +57,12 @@ properties:
>> - qcom,pma8084-gpio
>> - qcom,pmc8180-gpio
>> - qcom,pmc8180c-gpio
>> + - qcom,pmd802x-gpio
>
> Is the "x" some sort of wildcard or actual PMIC model/version name?
> Wildcards are in general discouraged.
>
"x" is being used as a wildcard here so can update with actual PMIC version
in next version.
Thanks,
Anjelique
Let's start from the end: > No - Looks to me you just have two clock controllers you are trying force into one. > Again, this shows 2 devices. The one related to your 'map0' should request AUD2_CLKID_AUDIOTOP as input and enable it right away. Most of fishy workarounds that you commented is caused the fact the mmio of this clock controller is divided into two parts. Compare it with axg-audio driver, things that was part of contigous memory region (like pdm) here are moved to second region. Is this enough to make a guess that these are two devices? Concerning AUD2_CLKID_AUDIOTOP clock, as it turned out, it must be enabled before enabling of clocks from second region too. That is AUD2_CLKID_AUDIOTOP clock feeds both parts of this clock controller. On 3/15/24 12:20, Jerome Brunet wrote: > > On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> This controller provides clocks and reset functionality for audio >> peripherals on Amlogic A1 SoC family. >> >> The driver is almost identical to 'axg-audio', however it would be better >> to keep it separate due to following reasons: >> >> - significant amount of bits has another definition. I will bring there >> a mess of new defines with A1_ suffixes. >> >> - registers of this controller are located in two separate regions. It >> will give a lot of complications for 'axg-audio' to support this. >> >> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> --- >> drivers/clk/meson/Kconfig | 13 + >> drivers/clk/meson/Makefile | 1 + >> drivers/clk/meson/a1-audio.c | 556 +++++++++++++++++++++++++++++++++++ >> drivers/clk/meson/a1-audio.h | 58 ++++ >> 4 files changed, 628 insertions(+) >> create mode 100644 drivers/clk/meson/a1-audio.c >> create mode 100644 drivers/clk/meson/a1-audio.h >> >> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig >> index d6a2fa5f7e88..80c4a18c83d2 100644 >> --- a/drivers/clk/meson/Kconfig >> +++ b/drivers/clk/meson/Kconfig >> @@ -133,6 +133,19 @@ config COMMON_CLK_A1_PERIPHERALS >> device, A1 SoC Family. Say Y if you want A1 Peripherals clock >> controller to work. >> >> +config COMMON_CLK_A1_AUDIO >> + tristate "Amlogic A1 SoC Audio clock controller support" >> + depends on ARM64 >> + select COMMON_CLK_MESON_REGMAP >> + select COMMON_CLK_MESON_CLKC_UTILS >> + select COMMON_CLK_MESON_PHASE >> + select COMMON_CLK_MESON_SCLK_DIV >> + select COMMON_CLK_MESON_AUDIO_RSTC >> + help >> + Support for the Audio clock controller on Amlogic A113L based >> + device, A1 SoC Family. Say Y if you want A1 Audio clock controller >> + to work. >> + >> config COMMON_CLK_G12A >> tristate "G12 and SM1 SoC clock controllers support" >> depends on ARM64 >> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile >> index 88d94921a4dc..4968fc7ad555 100644 >> --- a/drivers/clk/meson/Makefile >> +++ b/drivers/clk/meson/Makefile >> @@ -20,6 +20,7 @@ obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o >> obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o >> obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o >> obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o >> +obj-$(CONFIG_COMMON_CLK_A1_AUDIO) += a1-audio.o >> obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o >> obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o >> obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o >> diff --git a/drivers/clk/meson/a1-audio.c b/drivers/clk/meson/a1-audio.c >> new file mode 100644 >> index 000000000000..6039116c93ba >> --- /dev/null >> +++ b/drivers/clk/meson/a1-audio.c >> @@ -0,0 +1,556 @@ >> +// SPDX-License-Identifier: (GPL-2.0 OR MIT) >> +/* >> + * Copyright (c) 2024, SaluteDevices. All Rights Reserved. >> + * >> + * Author: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/clk-provider.h> >> +#include <linux/init.h> >> +#include <linux/of_device.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> +#include <linux/reset.h> >> +#include <linux/reset-controller.h> >> +#include <linux/slab.h> >> + >> +#include "meson-clkc-utils.h" >> +#include "meson-audio-rstc.h" >> +#include "clk-regmap.h" >> +#include "clk-phase.h" >> +#include "sclk-div.h" >> +#include "a1-audio.h" >> + >> +#define AUDIO_PDATA(_name) \ >> + ((const struct clk_parent_data[]) { { .hw = &(_name).hw } }) > > Not a fan - yet another level of macro. > >> + >> +#define AUDIO_MUX(_name, _reg, _mask, _shift, _pdata) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct clk_regmap_mux_data){ \ >> + .offset = AUDIO_REG_OFFSET(_reg), \ >> + .mask = (_mask), \ >> + .shift = (_shift), \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &clk_regmap_mux_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = ARRAY_SIZE(_pdata), \ >> + .flags = CLK_SET_RATE_PARENT, \ >> + }, \ >> +} >> + >> +#define AUDIO_DIV(_name, _reg, _shift, _width, _pdata) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct clk_regmap_div_data){ \ >> + .offset = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift), \ >> + .width = (_width), \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &clk_regmap_divider_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = 1, \ >> + .flags = CLK_SET_RATE_PARENT, \ >> + }, \ >> +} >> + >> +#define AUDIO_GATE(_name, _reg, _bit, _pdata) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct clk_regmap_gate_data){ \ >> + .offset = AUDIO_REG_OFFSET(_reg), \ >> + .bit_idx = (_bit), \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &clk_regmap_gate_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = 1, \ >> + .flags = CLK_SET_RATE_PARENT, \ >> + }, \ >> +} >> + >> +#define AUDIO_SCLK_DIV(_name, _reg, _div_shift, _div_width, \ >> + _hi_shift, _hi_width, _pdata, _set_rate_parent) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct meson_sclk_div_data) { \ >> + .div = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_div_shift), \ >> + .width = (_div_width), \ >> + }, \ >> + .hi = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_hi_shift), \ >> + .width = (_hi_width), \ >> + }, \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &meson_sclk_div_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = 1, \ >> + .flags = (_set_rate_parent) ? CLK_SET_RATE_PARENT : 0, \ > > Does not help readeability. Just pass the flag as axg-audio does. > >> + }, \ >> +} >> + >> +#define AUDIO_TRIPHASE(_name, _reg, _width, _shift0, _shift1, _shift2, \ >> + _pdata) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct meson_clk_triphase_data) { \ >> + .ph0 = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift0), \ >> + .width = (_width), \ >> + }, \ >> + .ph1 = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift1), \ >> + .width = (_width), \ >> + }, \ >> + .ph2 = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift2), \ >> + .width = (_width), \ >> + }, \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &meson_clk_triphase_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = 1, \ >> + .flags = CLK_SET_RATE_PARENT | CLK_DUTY_CYCLE_PARENT, \ >> + }, \ >> +} >> + >> +#define AUDIO_SCLK_WS(_name, _reg, _width, _shift_ph, _shift_ws, \ >> + _pdata) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct meson_sclk_ws_inv_data) { \ >> + .ph = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift_ph), \ >> + .width = (_width), \ >> + }, \ >> + .ws = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift_ws), \ >> + .width = (_width), \ >> + }, \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &meson_sclk_ws_inv_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = 1, \ >> + .flags = CLK_SET_RATE_PARENT | CLK_DUTY_CYCLE_PARENT, \ >> + }, \ >> +} > > All the above does essentially the same things as the macro of > axg-audio, to some minor differences. Yet it is another set to maintain. > Except one thing... Here I keep memory identifier to which this clock belongs: .map = AUDIO_REG_MAP(_reg), It is workaround, but ->map the only common field in clk_regmap that could be used for this purpose. > I'd much prefer if you put the axg-audio macro in a header a re-used > those. There would a single set to maintain. You may then specialize the > included in the driver C file, to avoid redundant parameters > > Rework axg-audio to use clk_parent_data if you must, but not in the same > series please. > >> + >> +static const struct clk_parent_data a1_pclk_pdata[] = { >> + { .fw_name = "pclk", }, >> +}; >> + >> +AUDIO_GATE(audio_ddr_arb, AUDIO_CLK_GATE_EN0, 0, a1_pclk_pdata); >> +AUDIO_GATE(audio_tdmin_a, AUDIO_CLK_GATE_EN0, 1, a1_pclk_pdata); >> +AUDIO_GATE(audio_tdmin_b, AUDIO_CLK_GATE_EN0, 2, a1_pclk_pdata); >> +AUDIO_GATE(audio_tdmin_lb, AUDIO_CLK_GATE_EN0, 3, a1_pclk_pdata); >> +AUDIO_GATE(audio_loopback, AUDIO_CLK_GATE_EN0, 4, a1_pclk_pdata); >> +AUDIO_GATE(audio_tdmout_a, AUDIO_CLK_GATE_EN0, 5, a1_pclk_pdata); >> +AUDIO_GATE(audio_tdmout_b, AUDIO_CLK_GATE_EN0, 6, a1_pclk_pdata); >> +AUDIO_GATE(audio_frddr_a, AUDIO_CLK_GATE_EN0, 7, a1_pclk_pdata); >> +AUDIO_GATE(audio_frddr_b, AUDIO_CLK_GATE_EN0, 8, a1_pclk_pdata); >> +AUDIO_GATE(audio_toddr_a, AUDIO_CLK_GATE_EN0, 9, a1_pclk_pdata); >> +AUDIO_GATE(audio_toddr_b, AUDIO_CLK_GATE_EN0, 10, a1_pclk_pdata); >> +AUDIO_GATE(audio_spdifin, AUDIO_CLK_GATE_EN0, 11, a1_pclk_pdata); >> +AUDIO_GATE(audio_resample, AUDIO_CLK_GATE_EN0, 12, a1_pclk_pdata); >> +AUDIO_GATE(audio_eqdrc, AUDIO_CLK_GATE_EN0, 13, a1_pclk_pdata); >> +AUDIO_GATE(audio_audiolocker, AUDIO_CLK_GATE_EN0, 14, a1_pclk_pdata); > This is what I mean by redundant parameter ^ > Yep. I could define something like AUDIO_PCLK_GATE(). >> + >> +AUDIO_GATE(audio2_ddr_arb, AUDIO2_CLK_GATE_EN0, 0, a1_pclk_pdata); >> +AUDIO_GATE(audio2_pdm, AUDIO2_CLK_GATE_EN0, 1, a1_pclk_pdata); >> +AUDIO_GATE(audio2_tdmin_vad, AUDIO2_CLK_GATE_EN0, 2, a1_pclk_pdata); >> +AUDIO_GATE(audio2_toddr_vad, AUDIO2_CLK_GATE_EN0, 3, a1_pclk_pdata); >> +AUDIO_GATE(audio2_vad, AUDIO2_CLK_GATE_EN0, 4, a1_pclk_pdata); >> +AUDIO_GATE(audio2_audiotop, AUDIO2_CLK_GATE_EN0, 7, a1_pclk_pdata); >> + >> +static const struct clk_parent_data a1_mst_pdata[] = { >> + { .fw_name = "dds_in" }, >> + { .fw_name = "fclk_div2" }, >> + { .fw_name = "fclk_div3" }, >> + { .fw_name = "hifi_pll" }, >> + { .fw_name = "xtal" }, >> +}; >> + >> +#define AUDIO_MST_MCLK(_name, _reg) \ >> + AUDIO_MUX(_name##_mux, (_reg), 0x7, 24, a1_mst_pdata); \ >> + AUDIO_DIV(_name##_div, (_reg), 0, 16, \ >> + AUDIO_PDATA(_name##_mux)); \ >> + AUDIO_GATE(_name, (_reg), 31, AUDIO_PDATA(_name##_div)) >> + >> +AUDIO_MST_MCLK(audio_mst_a_mclk, AUDIO_MCLK_A_CTRL); >> +AUDIO_MST_MCLK(audio_mst_b_mclk, AUDIO_MCLK_B_CTRL); >> +AUDIO_MST_MCLK(audio_mst_c_mclk, AUDIO_MCLK_C_CTRL); >> +AUDIO_MST_MCLK(audio_mst_d_mclk, AUDIO_MCLK_D_CTRL); >> +AUDIO_MST_MCLK(audio_spdifin_clk, AUDIO_CLK_SPDIFIN_CTRL); >> +AUDIO_MST_MCLK(audio_eqdrc_clk, AUDIO_CLK_EQDRC_CTRL); >> + >> +AUDIO_MUX(audio_resample_clk_mux, AUDIO_CLK_RESAMPLE_CTRL, 0xf, 24, >> + a1_mst_pdata); >> +AUDIO_DIV(audio_resample_clk_div, AUDIO_CLK_RESAMPLE_CTRL, 0, 8, >> + AUDIO_PDATA(audio_resample_clk_mux)); >> +AUDIO_GATE(audio_resample_clk, AUDIO_CLK_RESAMPLE_CTRL, 31, >> + AUDIO_PDATA(audio_resample_clk_div)); >> + >> +AUDIO_MUX(audio_locker_in_clk_mux, AUDIO_CLK_LOCKER_CTRL, 0xf, 8, >> + a1_mst_pdata); >> +AUDIO_DIV(audio_locker_in_clk_div, AUDIO_CLK_LOCKER_CTRL, 0, 8, >> + AUDIO_PDATA(audio_locker_in_clk_mux)); >> +AUDIO_GATE(audio_locker_in_clk, AUDIO_CLK_LOCKER_CTRL, 15, >> + AUDIO_PDATA(audio_locker_in_clk_div)); >> + >> +AUDIO_MUX(audio_locker_out_clk_mux, AUDIO_CLK_LOCKER_CTRL, 0xf, 24, >> + a1_mst_pdata); >> +AUDIO_DIV(audio_locker_out_clk_div, AUDIO_CLK_LOCKER_CTRL, 16, 8, >> + AUDIO_PDATA(audio_locker_out_clk_mux)); >> +AUDIO_GATE(audio_locker_out_clk, AUDIO_CLK_LOCKER_CTRL, 31, >> + AUDIO_PDATA(audio_locker_out_clk_div)); >> + >> +AUDIO_MST_MCLK(audio2_vad_mclk, AUDIO2_MCLK_VAD_CTRL); >> +AUDIO_MST_MCLK(audio2_vad_clk, AUDIO2_CLK_VAD_CTRL); >> +AUDIO_MST_MCLK(audio2_pdm_dclk, AUDIO2_CLK_PDMIN_CTRL0); >> +AUDIO_MST_MCLK(audio2_pdm_sysclk, AUDIO2_CLK_PDMIN_CTRL1); >> + >> +#define AUDIO_MST_SCLK(_name, _reg0, _reg1, _pdata) \ >> + AUDIO_GATE(_name##_pre_en, (_reg0), 31, (_pdata)); \ >> + AUDIO_SCLK_DIV(_name##_div, (_reg0), 20, 10, 0, 0, \ >> + AUDIO_PDATA(_name##_pre_en), true); \ >> + AUDIO_GATE(_name##_post_en, (_reg0), 30, \ >> + AUDIO_PDATA(_name##_div)); \ >> + AUDIO_TRIPHASE(_name, (_reg1), 1, 0, 2, 4, \ >> + AUDIO_PDATA(_name##_post_en)) >> + > > Again, I'm not a fan of this many levels of macro. I can live with it > but certainly don't want the burden of reviewing and maintaining for > clock driver. AXG / G12 and A1 are obviously closely related, so make it common. > >> +#define AUDIO_MST_LRCLK(_name, _reg0, _reg1, _pdata) \ >> + AUDIO_SCLK_DIV(_name##_div, (_reg0), 0, 10, 10, 10, \ >> + (_pdata), false); \ >> + AUDIO_TRIPHASE(_name, (_reg1), 1, 1, 3, 5, \ >> + AUDIO_PDATA(_name##_div)) >> + >> +AUDIO_MST_SCLK(audio_mst_a_sclk, AUDIO_MST_A_SCLK_CTRL0, AUDIO_MST_A_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_a_mclk)); >> +AUDIO_MST_SCLK(audio_mst_b_sclk, AUDIO_MST_B_SCLK_CTRL0, AUDIO_MST_B_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_b_mclk)); >> +AUDIO_MST_SCLK(audio_mst_c_sclk, AUDIO_MST_C_SCLK_CTRL0, AUDIO_MST_C_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_c_mclk)); >> +AUDIO_MST_SCLK(audio_mst_d_sclk, AUDIO_MST_D_SCLK_CTRL0, AUDIO_MST_D_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_d_mclk)); >> + >> +AUDIO_MST_LRCLK(audio_mst_a_lrclk, AUDIO_MST_A_SCLK_CTRL0, AUDIO_MST_A_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_a_sclk_post_en)); >> +AUDIO_MST_LRCLK(audio_mst_b_lrclk, AUDIO_MST_B_SCLK_CTRL0, AUDIO_MST_B_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_b_sclk_post_en)); >> +AUDIO_MST_LRCLK(audio_mst_c_lrclk, AUDIO_MST_C_SCLK_CTRL0, AUDIO_MST_C_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_c_sclk_post_en)); >> +AUDIO_MST_LRCLK(audio_mst_d_lrclk, AUDIO_MST_D_SCLK_CTRL0, AUDIO_MST_D_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_d_sclk_post_en)); >> + >> +static const struct clk_parent_data a1_mst_sclk_pdata[] = { >> + { .hw = &audio_mst_a_sclk.hw }, >> + { .hw = &audio_mst_b_sclk.hw }, >> + { .hw = &audio_mst_c_sclk.hw }, >> + { .hw = &audio_mst_d_sclk.hw }, >> + { .fw_name = "slv_sclk0" }, >> + { .fw_name = "slv_sclk1" }, >> + { .fw_name = "slv_sclk2" }, >> + { .fw_name = "slv_sclk3" }, >> + { .fw_name = "slv_sclk4" }, >> + { .fw_name = "slv_sclk5" }, >> + { .fw_name = "slv_sclk6" }, >> + { .fw_name = "slv_sclk7" }, >> + { .fw_name = "slv_sclk8" }, >> + { .fw_name = "slv_sclk9" }, >> +}; >> + >> +static const struct clk_parent_data a1_mst_lrclk_pdata[] = { >> + { .hw = &audio_mst_a_lrclk.hw }, >> + { .hw = &audio_mst_b_lrclk.hw }, >> + { .hw = &audio_mst_c_lrclk.hw }, >> + { .hw = &audio_mst_d_lrclk.hw }, >> + { .fw_name = "slv_lrclk0" }, >> + { .fw_name = "slv_lrclk1" }, >> + { .fw_name = "slv_lrclk2" }, >> + { .fw_name = "slv_lrclk3" }, >> + { .fw_name = "slv_lrclk4" }, >> + { .fw_name = "slv_lrclk5" }, >> + { .fw_name = "slv_lrclk6" }, >> + { .fw_name = "slv_lrclk7" }, >> + { .fw_name = "slv_lrclk8" }, >> + { .fw_name = "slv_lrclk9" }, >> +}; >> + >> +#define AUDIO_TDM_SCLK(_name, _reg) \ >> + AUDIO_MUX(_name##_mux, (_reg), 0xf, 24, a1_mst_sclk_pdata); \ >> + AUDIO_GATE(_name##_pre_en, (_reg), 31, \ >> + AUDIO_PDATA(_name##_mux)); \ >> + AUDIO_GATE(_name##_post_en, (_reg), 30, \ >> + AUDIO_PDATA(_name##_pre_en)); \ >> + AUDIO_SCLK_WS(_name, (_reg), 1, 29, 28, \ >> + AUDIO_PDATA(_name##_post_en)) >> + >> +#define AUDIO_TDM_LRCLK(_name, _reg) \ >> + AUDIO_MUX(_name, (_reg), 0xf, 20, a1_mst_lrclk_pdata) >> + >> +AUDIO_TDM_SCLK(audio_tdmin_a_sclk, AUDIO_CLK_TDMIN_A_CTRL); >> +AUDIO_TDM_SCLK(audio_tdmin_b_sclk, AUDIO_CLK_TDMIN_B_CTRL); >> +AUDIO_TDM_SCLK(audio_tdmin_lb_sclk, AUDIO_CLK_TDMIN_LB_CTRL); >> +AUDIO_TDM_SCLK(audio_tdmout_a_sclk, AUDIO_CLK_TDMOUT_A_CTRL); >> +AUDIO_TDM_SCLK(audio_tdmout_b_sclk, AUDIO_CLK_TDMOUT_B_CTRL); >> + >> +AUDIO_TDM_LRCLK(audio_tdmin_a_lrclk, AUDIO_CLK_TDMIN_A_CTRL); >> +AUDIO_TDM_LRCLK(audio_tdmin_b_lrclk, AUDIO_CLK_TDMIN_B_CTRL); >> +AUDIO_TDM_LRCLK(audio_tdmin_lb_lrclk, AUDIO_CLK_TDMIN_LB_CTRL); >> +AUDIO_TDM_LRCLK(audio_tdmout_a_lrclk, AUDIO_CLK_TDMOUT_A_CTRL); >> +AUDIO_TDM_LRCLK(audio_tdmout_b_lrclk, AUDIO_CLK_TDMOUT_B_CTRL); >> + >> +static struct clk_hw *a1_audio_hw_clks[] = { >> + [AUD_CLKID_DDR_ARB] = &audio_ddr_arb.hw, >> + [AUD_CLKID_TDMIN_A] = &audio_tdmin_a.hw, >> + [AUD_CLKID_TDMIN_B] = &audio_tdmin_b.hw, >> + [AUD_CLKID_TDMIN_LB] = &audio_tdmin_lb.hw, >> + [AUD_CLKID_LOOPBACK] = &audio_loopback.hw, >> + [AUD_CLKID_TDMOUT_A] = &audio_tdmout_a.hw, >> + [AUD_CLKID_TDMOUT_B] = &audio_tdmout_b.hw, >> + [AUD_CLKID_FRDDR_A] = &audio_frddr_a.hw, >> + [AUD_CLKID_FRDDR_B] = &audio_frddr_b.hw, >> + [AUD_CLKID_TODDR_A] = &audio_toddr_a.hw, >> + [AUD_CLKID_TODDR_B] = &audio_toddr_b.hw, >> + [AUD_CLKID_SPDIFIN] = &audio_spdifin.hw, >> + [AUD_CLKID_RESAMPLE] = &audio_resample.hw, >> + [AUD_CLKID_EQDRC] = &audio_eqdrc.hw, >> + [AUD_CLKID_LOCKER] = &audio_audiolocker.hw, >> + [AUD_CLKID_MST_A_MCLK_SEL] = &audio_mst_a_mclk_mux.hw, >> + [AUD_CLKID_MST_A_MCLK_DIV] = &audio_mst_a_mclk_div.hw, >> + [AUD_CLKID_MST_A_MCLK] = &audio_mst_a_mclk.hw, >> + [AUD_CLKID_MST_B_MCLK_SEL] = &audio_mst_b_mclk_mux.hw, >> + [AUD_CLKID_MST_B_MCLK_DIV] = &audio_mst_b_mclk_div.hw, >> + [AUD_CLKID_MST_B_MCLK] = &audio_mst_b_mclk.hw, >> + [AUD_CLKID_MST_C_MCLK_SEL] = &audio_mst_c_mclk_mux.hw, >> + [AUD_CLKID_MST_C_MCLK_DIV] = &audio_mst_c_mclk_div.hw, >> + [AUD_CLKID_MST_C_MCLK] = &audio_mst_c_mclk.hw, >> + [AUD_CLKID_MST_D_MCLK_SEL] = &audio_mst_d_mclk_mux.hw, >> + [AUD_CLKID_MST_D_MCLK_DIV] = &audio_mst_d_mclk_div.hw, >> + [AUD_CLKID_MST_D_MCLK] = &audio_mst_d_mclk.hw, >> + [AUD_CLKID_RESAMPLE_CLK_SEL] = &audio_resample_clk_mux.hw, >> + [AUD_CLKID_RESAMPLE_CLK_DIV] = &audio_resample_clk_div.hw, >> + [AUD_CLKID_RESAMPLE_CLK] = &audio_resample_clk.hw, >> + [AUD_CLKID_LOCKER_IN_CLK_SEL] = &audio_locker_in_clk_mux.hw, >> + [AUD_CLKID_LOCKER_IN_CLK_DIV] = &audio_locker_in_clk_div.hw, >> + [AUD_CLKID_LOCKER_IN_CLK] = &audio_locker_in_clk.hw, >> + [AUD_CLKID_LOCKER_OUT_CLK_SEL] = &audio_locker_out_clk_mux.hw, >> + [AUD_CLKID_LOCKER_OUT_CLK_DIV] = &audio_locker_out_clk_div.hw, >> + [AUD_CLKID_LOCKER_OUT_CLK] = &audio_locker_out_clk.hw, >> + [AUD_CLKID_SPDIFIN_CLK_SEL] = &audio_spdifin_clk_mux.hw, >> + [AUD_CLKID_SPDIFIN_CLK_DIV] = &audio_spdifin_clk_div.hw, >> + [AUD_CLKID_SPDIFIN_CLK] = &audio_spdifin_clk.hw, >> + [AUD_CLKID_EQDRC_CLK_SEL] = &audio_eqdrc_clk_mux.hw, >> + [AUD_CLKID_EQDRC_CLK_DIV] = &audio_eqdrc_clk_div.hw, >> + [AUD_CLKID_EQDRC_CLK] = &audio_eqdrc_clk.hw, >> + [AUD_CLKID_MST_A_SCLK_PRE_EN] = &audio_mst_a_sclk_pre_en.hw, >> + [AUD_CLKID_MST_A_SCLK_DIV] = &audio_mst_a_sclk_div.hw, >> + [AUD_CLKID_MST_A_SCLK_POST_EN] = &audio_mst_a_sclk_post_en.hw, >> + [AUD_CLKID_MST_A_SCLK] = &audio_mst_a_sclk.hw, >> + [AUD_CLKID_MST_B_SCLK_PRE_EN] = &audio_mst_b_sclk_pre_en.hw, >> + [AUD_CLKID_MST_B_SCLK_DIV] = &audio_mst_b_sclk_div.hw, >> + [AUD_CLKID_MST_B_SCLK_POST_EN] = &audio_mst_b_sclk_post_en.hw, >> + [AUD_CLKID_MST_B_SCLK] = &audio_mst_b_sclk.hw, >> + [AUD_CLKID_MST_C_SCLK_PRE_EN] = &audio_mst_c_sclk_pre_en.hw, >> + [AUD_CLKID_MST_C_SCLK_DIV] = &audio_mst_c_sclk_div.hw, >> + [AUD_CLKID_MST_C_SCLK_POST_EN] = &audio_mst_c_sclk_post_en.hw, >> + [AUD_CLKID_MST_C_SCLK] = &audio_mst_c_sclk.hw, >> + [AUD_CLKID_MST_D_SCLK_PRE_EN] = &audio_mst_d_sclk_pre_en.hw, >> + [AUD_CLKID_MST_D_SCLK_DIV] = &audio_mst_d_sclk_div.hw, >> + [AUD_CLKID_MST_D_SCLK_POST_EN] = &audio_mst_d_sclk_post_en.hw, >> + [AUD_CLKID_MST_D_SCLK] = &audio_mst_d_sclk.hw, >> + [AUD_CLKID_MST_A_LRCLK_DIV] = &audio_mst_a_lrclk_div.hw, >> + [AUD_CLKID_MST_A_LRCLK] = &audio_mst_a_lrclk.hw, >> + [AUD_CLKID_MST_B_LRCLK_DIV] = &audio_mst_b_lrclk_div.hw, >> + [AUD_CLKID_MST_B_LRCLK] = &audio_mst_b_lrclk.hw, >> + [AUD_CLKID_MST_C_LRCLK_DIV] = &audio_mst_c_lrclk_div.hw, >> + [AUD_CLKID_MST_C_LRCLK] = &audio_mst_c_lrclk.hw, >> + [AUD_CLKID_MST_D_LRCLK_DIV] = &audio_mst_d_lrclk_div.hw, >> + [AUD_CLKID_MST_D_LRCLK] = &audio_mst_d_lrclk.hw, >> + [AUD_CLKID_TDMIN_A_SCLK_SEL] = &audio_tdmin_a_sclk_mux.hw, >> + [AUD_CLKID_TDMIN_A_SCLK_PRE_EN] = &audio_tdmin_a_sclk_pre_en.hw, >> + [AUD_CLKID_TDMIN_A_SCLK_POST_EN] = &audio_tdmin_a_sclk_post_en.hw, >> + [AUD_CLKID_TDMIN_A_SCLK] = &audio_tdmin_a_sclk.hw, >> + [AUD_CLKID_TDMIN_A_LRCLK] = &audio_tdmin_a_lrclk.hw, >> + [AUD_CLKID_TDMIN_B_SCLK_SEL] = &audio_tdmin_b_sclk_mux.hw, >> + [AUD_CLKID_TDMIN_B_SCLK_PRE_EN] = &audio_tdmin_b_sclk_pre_en.hw, >> + [AUD_CLKID_TDMIN_B_SCLK_POST_EN] = &audio_tdmin_b_sclk_post_en.hw, >> + [AUD_CLKID_TDMIN_B_SCLK] = &audio_tdmin_b_sclk.hw, >> + [AUD_CLKID_TDMIN_B_LRCLK] = &audio_tdmin_b_lrclk.hw, >> + [AUD_CLKID_TDMIN_LB_SCLK_SEL] = &audio_tdmin_lb_sclk_mux.hw, >> + [AUD_CLKID_TDMIN_LB_SCLK_PRE_EN] = &audio_tdmin_lb_sclk_pre_en.hw, >> + [AUD_CLKID_TDMIN_LB_SCLK_POST_EN] = &audio_tdmin_lb_sclk_post_en.hw, >> + [AUD_CLKID_TDMIN_LB_SCLK] = &audio_tdmin_lb_sclk.hw, >> + [AUD_CLKID_TDMIN_LB_LRCLK] = &audio_tdmin_lb_lrclk.hw, >> + [AUD_CLKID_TDMOUT_A_SCLK_SEL] = &audio_tdmout_a_sclk_mux.hw, >> + [AUD_CLKID_TDMOUT_A_SCLK_PRE_EN] = &audio_tdmout_a_sclk_pre_en.hw, >> + [AUD_CLKID_TDMOUT_A_SCLK_POST_EN] = &audio_tdmout_a_sclk_post_en.hw, >> + [AUD_CLKID_TDMOUT_A_SCLK] = &audio_tdmout_a_sclk.hw, >> + [AUD_CLKID_TDMOUT_A_LRCLK] = &audio_tdmout_a_lrclk.hw, >> + [AUD_CLKID_TDMOUT_B_SCLK_SEL] = &audio_tdmout_b_sclk_mux.hw, >> + [AUD_CLKID_TDMOUT_B_SCLK_PRE_EN] = &audio_tdmout_b_sclk_pre_en.hw, >> + [AUD_CLKID_TDMOUT_B_SCLK_POST_EN] = &audio_tdmout_b_sclk_post_en.hw, >> + [AUD_CLKID_TDMOUT_B_SCLK] = &audio_tdmout_b_sclk.hw, >> + [AUD_CLKID_TDMOUT_B_LRCLK] = &audio_tdmout_b_lrclk.hw, >> + >> + [AUD2_CLKID_DDR_ARB] = &audio2_ddr_arb.hw, >> + [AUD2_CLKID_PDM] = &audio2_pdm.hw, >> + [AUD2_CLKID_TDMIN_VAD] = &audio2_tdmin_vad.hw, >> + [AUD2_CLKID_TODDR_VAD] = &audio2_toddr_vad.hw, >> + [AUD2_CLKID_VAD] = &audio2_vad.hw, >> + [AUD2_CLKID_AUDIOTOP] = &audio2_audiotop.hw, >> + [AUD2_CLKID_VAD_MCLK_SEL] = &audio2_vad_mclk_mux.hw, >> + [AUD2_CLKID_VAD_MCLK_DIV] = &audio2_vad_mclk_div.hw, >> + [AUD2_CLKID_VAD_MCLK] = &audio2_vad_mclk.hw, >> + [AUD2_CLKID_VAD_CLK_SEL] = &audio2_vad_clk_mux.hw, >> + [AUD2_CLKID_VAD_CLK_DIV] = &audio2_vad_clk_div.hw, >> + [AUD2_CLKID_VAD_CLK] = &audio2_vad_clk.hw, >> + [AUD2_CLKID_PDM_DCLK_SEL] = &audio2_pdm_dclk_mux.hw, >> + [AUD2_CLKID_PDM_DCLK_DIV] = &audio2_pdm_dclk_div.hw, >> + [AUD2_CLKID_PDM_DCLK] = &audio2_pdm_dclk.hw, >> + [AUD2_CLKID_PDM_SYSCLK_SEL] = &audio2_pdm_sysclk_mux.hw, >> + [AUD2_CLKID_PDM_SYSCLK_DIV] = &audio2_pdm_sysclk_div.hw, >> + [AUD2_CLKID_PDM_SYSCLK] = &audio2_pdm_sysclk.hw, >> +}; >> + >> +static struct meson_clk_hw_data a1_audio_clks = { >> + .hws = a1_audio_hw_clks, >> + .num = ARRAY_SIZE(a1_audio_hw_clks), >> +}; >> + >> +static struct regmap *a1_audio_map(struct platform_device *pdev, >> + unsigned int index) >> +{ >> + char name[32]; >> + const struct regmap_config cfg = { >> + .reg_bits = 32, >> + .val_bits = 32, >> + .reg_stride = 4, >> + .name = name, > > Not necessary > This implementation uses two regmaps, and this field allow to avoid errors like this: [ 0.145530] debugfs: Directory 'fe050000.audio-clock-controller' with parent 'regmap' already present! >> + }; >> + void __iomem *base; >> + >> + base = devm_platform_ioremap_resource(pdev, index); >> + if (IS_ERR(base)) >> + return base; >> + >> + scnprintf(name, sizeof(name), "%d", index); >> + return devm_regmap_init_mmio(&pdev->dev, base, &cfg); >> +} > > That is overengineered. Please keep it simple. Declare the regmap_config > as static const global, and do it like axg-audio please. > This only reason why it is not "static const" because I need to set unique name for each regmap. >> + >> +static int a1_register_clk(struct platform_device *pdev, >> + struct regmap *map0, struct regmap *map1, >> + struct clk_hw *hw) >> +{ >> + struct clk_regmap *clk = container_of(hw, struct clk_regmap, hw); >> + >> + if (!hw) >> + return 0; >> + >> + switch ((unsigned long)clk->map) { >> + case AUDIO_RANGE_0: >> + clk->map = map0; >> + break; >> + case AUDIO_RANGE_1: >> + clk->map = map1; >> + break; > > ... fishy > >> + default: >> + WARN_ON(1); >> + return -EINVAL; >> + } >> + >> + return devm_clk_hw_register(&pdev->dev, hw); >> +} >> + >> +static int a1_audio_clkc_probe(struct platform_device *pdev) >> +{ >> + struct regmap *map0, *map1; >> + struct clk *clk; >> + unsigned int i; >> + int ret; >> + >> + clk = devm_clk_get_enabled(&pdev->dev, "pclk"); >> + if (WARN_ON(IS_ERR(clk))) >> + return PTR_ERR(clk); >> + >> + map0 = a1_audio_map(pdev, 0); >> + if (IS_ERR(map0)) >> + return PTR_ERR(map0); >> + >> + map1 = a1_audio_map(pdev, 1); >> + if (IS_ERR(map1)) >> + return PTR_ERR(map1); > > No - Looks to me you just have two clock controllers you are trying > force into one. > See the begining. >> + >> + /* >> + * Register and enable AUD2_CLKID_AUDIOTOP clock first. Unless >> + * it is enabled any read/write to 'map0' hangs the CPU. >> + */ >> + >> + ret = a1_register_clk(pdev, map0, map1, >> + a1_audio_clks.hws[AUD2_CLKID_AUDIOTOP]); >> + if (ret) >> + return ret; >> + >> + ret = clk_prepare_enable(a1_audio_clks.hws[AUD2_CLKID_AUDIOTOP]->clk); >> + if (ret) >> + return ret; > > Again, this shows 2 devices. The one related to your 'map0' should > request AUD2_CLKID_AUDIOTOP as input and enable it right away. > See the begining. >> + >> + for (i = 0; i < a1_audio_clks.num; i++) { >> + if (i == AUD2_CLKID_AUDIOTOP) >> + continue; >> + >> + ret = a1_register_clk(pdev, map0, map1, a1_audio_clks.hws[i]); >> + if (ret) >> + return ret; >> + } >> + >> + ret = devm_of_clk_add_hw_provider(&pdev->dev, meson_clk_hw_get, >> + &a1_audio_clks); >> + if (ret) >> + return ret; >> + >> + BUILD_BUG_ON((unsigned long)AUDIO_REG_MAP(AUDIO_SW_RESET0) != >> + AUDIO_RANGE_0); > > Why is that necessary ? > A little paranoia. Here AUDIO_SW_RESET0 is handled as map0's register, and I want to assert it. >> + return meson_audio_rstc_register(&pdev->dev, map0, >> + AUDIO_REG_OFFSET(AUDIO_SW_RESET0), 32); >> +} >> + >> +static const struct of_device_id a1_audio_clkc_match_table[] = { >> + { .compatible = "amlogic,a1-audio-clkc", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, a1_audio_clkc_match_table); >> + >> +static struct platform_driver a1_audio_clkc_driver = { >> + .probe = a1_audio_clkc_probe, >> + .driver = { >> + .name = "a1-audio-clkc", >> + .of_match_table = a1_audio_clkc_match_table, >> + }, >> +}; >> +module_platform_driver(a1_audio_clkc_driver); >> + >> +MODULE_DESCRIPTION("Amlogic A1 Audio Clock driver"); >> +MODULE_AUTHOR("Jan Dakinevich <jan.dakinevich@salutedevices.com>"); >> +MODULE_LICENSE("GPL"); >> diff --git a/drivers/clk/meson/a1-audio.h b/drivers/clk/meson/a1-audio.h >> new file mode 100644 >> index 000000000000..f994e87276cd >> --- /dev/null >> +++ b/drivers/clk/meson/a1-audio.h >> @@ -0,0 +1,58 @@ >> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */ >> +/* >> + * Copyright (c) 2024, SaluteDevices. All Rights Reserved. >> + * >> + * Author: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> + */ >> + >> +#ifndef __A1_AUDIO_H >> +#define __A1_AUDIO_H >> + >> +#define AUDIO_RANGE_0 0xa >> +#define AUDIO_RANGE_1 0xb >> +#define AUDIO_RANGE_SHIFT 16 >> + >> +#define AUDIO_REG(_range, _offset) \ >> + (((_range) << AUDIO_RANGE_SHIFT) + (_offset)) >> + >> +#define AUDIO_REG_OFFSET(_reg) \ >> + ((_reg) & ((1 << AUDIO_RANGE_SHIFT) - 1)) >> + >> +#define AUDIO_REG_MAP(_reg) \ >> + ((void *)((_reg) >> AUDIO_RANGE_SHIFT)) > > That is seriouly overengineered. > The following are offset. Just write what they are. > This is all in order to keep range's identifier together with offset and then use it to store the identifier in clk_regmaps. > There is not reason to put that into a header. It is only going to be > used by a single driver. > >> + >> +#define AUDIO_CLK_GATE_EN0 AUDIO_REG(AUDIO_RANGE_0, 0x000) >> +#define AUDIO_MCLK_A_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x008) >> +#define AUDIO_MCLK_B_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x00c) >> +#define AUDIO_MCLK_C_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x010) >> +#define AUDIO_MCLK_D_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x014) >> +#define AUDIO_MCLK_E_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x018) >> +#define AUDIO_MCLK_F_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x01c) >> +#define AUDIO_SW_RESET0 AUDIO_REG(AUDIO_RANGE_0, 0x028) >> +#define AUDIO_MST_A_SCLK_CTRL0 AUDIO_REG(AUDIO_RANGE_0, 0x040) >> +#define AUDIO_MST_A_SCLK_CTRL1 AUDIO_REG(AUDIO_RANGE_0, 0x044) >> +#define AUDIO_MST_B_SCLK_CTRL0 AUDIO_REG(AUDIO_RANGE_0, 0x048) >> +#define AUDIO_MST_B_SCLK_CTRL1 AUDIO_REG(AUDIO_RANGE_0, 0x04c) >> +#define AUDIO_MST_C_SCLK_CTRL0 AUDIO_REG(AUDIO_RANGE_0, 0x050) >> +#define AUDIO_MST_C_SCLK_CTRL1 AUDIO_REG(AUDIO_RANGE_0, 0x054) >> +#define AUDIO_MST_D_SCLK_CTRL0 AUDIO_REG(AUDIO_RANGE_0, 0x058) >> +#define AUDIO_MST_D_SCLK_CTRL1 AUDIO_REG(AUDIO_RANGE_0, 0x05c) >> +#define AUDIO_CLK_TDMIN_A_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x080) >> +#define AUDIO_CLK_TDMIN_B_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x084) >> +#define AUDIO_CLK_TDMIN_LB_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x08c) >> +#define AUDIO_CLK_TDMOUT_A_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x090) >> +#define AUDIO_CLK_TDMOUT_B_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x094) >> +#define AUDIO_CLK_SPDIFIN_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x09c) >> +#define AUDIO_CLK_RESAMPLE_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x0a4) >> +#define AUDIO_CLK_LOCKER_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x0a8) >> +#define AUDIO_CLK_EQDRC_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x0c0) >> + >> +#define AUDIO2_CLK_GATE_EN0 AUDIO_REG(AUDIO_RANGE_1, 0x00c) >> +#define AUDIO2_MCLK_VAD_CTRL AUDIO_REG(AUDIO_RANGE_1, 0x040) >> +#define AUDIO2_CLK_VAD_CTRL AUDIO_REG(AUDIO_RANGE_1, 0x044) >> +#define AUDIO2_CLK_PDMIN_CTRL0 AUDIO_REG(AUDIO_RANGE_1, 0x058) >> +#define AUDIO2_CLK_PDMIN_CTRL1 AUDIO_REG(AUDIO_RANGE_1, 0x05c) >> + >> +#include <dt-bindings/clock/amlogic,a1-audio-clkc.h> >> + >> +#endif /* __A1_AUDIO_H */ > > -- Best regards Jan Dakinevich
在 2024/3/18 22:33, kernel test robot 写道: > tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master > branch HEAD: 2e93f143ca010a5013528e1cfdc895f024fe8c21 Add linux-next specific files for 20240318 > > Error/Warning ids grouped by kconfigs: > > gcc_recent_errors > |-- arc-allmodconfig > | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead > |-- arc-allyesconfig > | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead > |-- arm-allmodconfig > | |-- arch-arm-mach-omap2-prm33xx.c:warning:expecting-prototype-for-am33xx_prm_global_warm_sw_reset().-Prototype-was-for-am33xx_prm_global_sw_reset()-instead > | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and > | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead > |-- arm-allyesconfig > | |-- arch-arm-mach-omap2-prm33xx.c:warning:expecting-prototype-for-am33xx_prm_global_warm_sw_reset().-Prototype-was-for-am33xx_prm_global_sw_reset()-instead > | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and > | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead > |-- arm64-defconfig > | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead > |-- csky-allmodconfig > | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and > | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead > |-- csky-allyesconfig > | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and > | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead Hi, Richard, I sent out the warning fix patch in https://patchwork.ozlabs.org/project/linux-mtd/patch/20240227024204.1080739-1-chengzhihao1@huawei.com/
On Mon, Mar 18, 2024 at 06:38:20PM +0530, Mukesh Ojha wrote:
>
>
> On 3/3/2024 12:55 AM, Bjorn Andersson wrote:
> > On Tue, Feb 27, 2024 at 09:23:06PM +0530, Mukesh Ojha wrote:
> > > qcom_scm_is_available() gives wrong indication if __scm
> > > is initialized but __scm->dev is not.
> > >
> > > Fix this appropriately by making sure if __scm is
> > > initialized and then it is associated with its
> > > device.
> > >
> >
> > This seems like a bug fix, and should as such have a Fixes: tag and
> > probably Cc: stable@vger.kernel.org
> >
> > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > ---
> > > drivers/firmware/qcom/qcom_scm.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > > index 6c252cddd44e..6f14254c0c10 100644
> > > --- a/drivers/firmware/qcom/qcom_scm.c
> > > +++ b/drivers/firmware/qcom/qcom_scm.c
> > > @@ -1859,6 +1859,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
> > > if (!scm)
> > > return -ENOMEM;
> > > + scm->dev = &pdev->dev;
> > > ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr);
> > > if (ret < 0)
> > > return ret;
> > > @@ -1895,7 +1896,6 @@ static int qcom_scm_probe(struct platform_device *pdev)
> > > return ret;
> > > __scm = scm;
> > > - __scm->dev = &pdev->dev;
> >
> > Is it sufficient to just move the line up, or do we need a barrier of
> > some sort here?
>
> Would be good to use, smp_mb() before the assignment
> __scm = scm
> along with moving below line
> __scm->dev = &pdev->dev
>
Full memory barrier is not needed here. store variant is sufficient.
WRITE_ONCE() + smp_store_release() will fit here no?
Thanks,
Pavan
On 3/18/24 13:55, Jerome Brunet wrote: > > On Sun 17 Mar 2024 at 18:52, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> On 3/15/24 13:22, Jerome Brunet wrote: >>> >>> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 15/03/2024 00:21, Jan Dakinevich wrote: >>>>> This option allow to redefine the rate of DSP system clock. >>>> >>>> And why is it suitable for bindings? Describe the hardware, not what you >>>> want to do in the driver. >>>> >>>>> >>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>>> --- >>>>> Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>> index df21dd72fc65..d2f23a59a6b6 100644 >>>>> --- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>> +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>> @@ -40,6 +40,10 @@ properties: >>>>> resets: >>>>> maxItems: 1 >>>>> >>>>> + sysrate: >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + description: redefine rate of DSP system clock >>>> >>>> No vendor prefix, so is it a generic property? Also, missing unit >>>> suffix, but more importantly I don't understand why this is a property >>>> of hardware. >>> >>> +1. >>> >>> The appropriate way to set rate of the clock before the driver take over >>> is 'assigned-rate', if you need to customize this for different >>> platform. >>> >> >> It would be great, but it doesn't work. Below, is what I want to see: >> >> assigned-clocks = >> <&clkc_audio AUD2_CLKID_PDM_SYSCLK_SEL>, >> <&clkc_audio AUD2_CLKID_PDM_SYSCLK_DIV>; >> assigned-clock-parents = >> <&clkc_pll CLKID_FCLK_DIV3>, >> <0>; >> assigned-clock-rates = >> <0>, >> <256000000>; >> >> But regardles of this declaration, PDM's driver unconditionally sets >> sysclk'rate to 250MHz and throws away everything that was configured >> before, reparents audio2_pdm_sysclk_mux to hifi_pll and changes >> hifi_pll's rate. >> >> This value 250MHz is declared here: >> >> static const struct axg_pdm_cfg axg_pdm_config = { >> .filters = &axg_default_filters, >> .sys_rate = 250000000, >> }; >> >> The property 'sysrate' is intended to redefine hardcoded 'sys_rate' >> value in 'axg_pdm_config'. > > What is stopping you from removing that from the driver and adding > assigned-rate to 250M is the existing platform ? > Ok, in next version I will try to remove this unconditional setting of rate that spoils my clock hierarchy. >> >>> Then you don't have to deal with it in the device driver. >>> >>>> >>>> Best regards, >>>> Krzysztof >>> >>> > > -- Best regards Jan Dakinevich
On 3/18/24 15:19, Jerome Brunet wrote: > > On Mon 18 Mar 2024 at 11:55, Jerome Brunet <jbrunet@baylibre.com> wrote: > >> On Sun 17 Mar 2024 at 18:52, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: >> >>> On 3/15/24 13:22, Jerome Brunet wrote: >>>> >>>> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>>> On 15/03/2024 00:21, Jan Dakinevich wrote: >>>>>> This option allow to redefine the rate of DSP system clock. >>>>> >>>>> And why is it suitable for bindings? Describe the hardware, not what you >>>>> want to do in the driver. >>>>> >>>>>> >>>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>>>> --- >>>>>> Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>>> index df21dd72fc65..d2f23a59a6b6 100644 >>>>>> --- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>>> +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>>> @@ -40,6 +40,10 @@ properties: >>>>>> resets: >>>>>> maxItems: 1 >>>>>> >>>>>> + sysrate: >>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>> + description: redefine rate of DSP system clock >>>>> >>>>> No vendor prefix, so is it a generic property? Also, missing unit >>>>> suffix, but more importantly I don't understand why this is a property >>>>> of hardware. >>>> >>>> +1. >>>> >>>> The appropriate way to set rate of the clock before the driver take over >>>> is 'assigned-rate', if you need to customize this for different >>>> platform. >>>> >>> >>> It would be great, but it doesn't work. Below, is what I want to see: >>> >>> assigned-clocks = >>> <&clkc_audio AUD2_CLKID_PDM_SYSCLK_SEL>, >>> <&clkc_audio AUD2_CLKID_PDM_SYSCLK_DIV>; >>> assigned-clock-parents = >>> <&clkc_pll CLKID_FCLK_DIV3>, >>> <0>; >>> assigned-clock-rates = >>> <0>, >>> <256000000>; >>> >>> But regardles of this declaration, PDM's driver unconditionally sets >>> sysclk'rate to 250MHz and throws away everything that was configured >>> before, reparents audio2_pdm_sysclk_mux to hifi_pll and changes >>> hifi_pll's rate. >>> >>> This value 250MHz is declared here: >>> >>> static const struct axg_pdm_cfg axg_pdm_config = { >>> .filters = &axg_default_filters, >>> .sys_rate = 250000000, >>> }; >>> >>> The property 'sysrate' is intended to redefine hardcoded 'sys_rate' >>> value in 'axg_pdm_config'. >> >> What is stopping you from removing that from the driver and adding >> assigned-rate to 250M is the existing platform ? > > ... Also, considering how PDM does work, I'm not sure I get the point of > the doing all this to go from 250MHz to 256Mhz. > The point is to use fclk_div3 clock as source for PDM's sysclock and keep hiff_pll clock free for TDM. Because, I can get 256MHz from any hifi_pll and fclk_div3, but only hifi_pll is able to provide accurate 48kHz (after several divider). > PDM value is sampled at ~75% of the half period. That clock basically > feeds a counter and the threshold is adjusted based on the clock rate. > > So there is no need to change the rate. Changing it is only necessary > when the captured audio rate is extremely slow (<8kHz) and the counter > may overflow. The driver already adjust this automatically. > > So changing the input rate from 250MHz to 256MHz should not make any > difference. > Thank you for the explanation. >> >>> >>>> Then you don't have to deal with it in the device driver. >>>> >>>>> >>>>> Best regards, >>>>> Krzysztof >>>> >>>> > > -- Best regards Jan Dakinevich
On 3/18/24 13:46, Jerome Brunet wrote: > > On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> A1's internal codec is very close to t9015. The main difference, that it >> has ADC. This commit introduces support for capturing from it. > > This is mis-leading. > > It does not look like the change is A1 specific but rather a extension > of the support for t9015. It also mixes several different topics like line > configuration, capture support, etc ... > First, it is not only extentsion. Some bits are changed comparing to existing t9015, so new compatible string is still required. Second, I don't know anything about about ADC in t9015 on other SoCs and even don't sure that it exist there (may be I am inattentive, but I'm unable to find audio input pin on sm1/g12a's pinout). > Again, the t9015 changes should be a separated series from the rest, and > there should be one patch per topic. > > As Mark, if something is meant to be configured based on the HW layout, > then there a good change a kcontrol is not appropriate, and this should > rather be part of the platform description, like DT. > > It was also suggested here: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/meson/t9015.c?h=v6.8#n298 > Ok. By the way, on a1 LINEOUT_CFG would have another value. >> >> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> --- >> sound/soc/meson/t9015.c | 259 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 259 insertions(+) >> >> diff --git a/sound/soc/meson/t9015.c b/sound/soc/meson/t9015.c >> index 48f6767bd858..365955bfeb78 100644 >> --- a/sound/soc/meson/t9015.c >> +++ b/sound/soc/meson/t9015.c >> @@ -19,16 +19,33 @@ >> #define LOLP_EN 3 >> #define DACR_EN 4 >> #define DACL_EN 5 >> +#define ADCR_EN 6 >> +#define ADCL_EN 7 >> +#define PGAR_ZCD_EN 8 >> +#define PGAL_ZCD_EN 9 >> +#define PGAR_EN 10 >> +#define PGAL_EN 11 >> +#define ADCR_INV 16 >> +#define ADCL_INV 17 >> +#define ADCR_SRC 18 >> +#define ADCL_SRC 19 >> #define DACR_INV 20 >> #define DACL_INV 21 >> #define DACR_SRC 22 >> #define DACL_SRC 23 >> +#define ADC_DEM_EN 26 >> +#define ADC_FILTER_MODE 28 >> +#define ADC_FILTER_EN 29 >> #define REFP_BUF_EN BIT(12) >> #define BIAS_CURRENT_EN BIT(13) >> #define VMID_GEN_FAST BIT(14) >> #define VMID_GEN_EN BIT(15) >> #define I2S_MODE BIT(30) >> #define VOL_CTRL0 0x04 >> +#define PGAR_VC 0 >> +#define PGAL_VC 8 >> +#define ADCR_VC 16 >> +#define ADCL_VC 24 >> #define GAIN_H 31 >> #define GAIN_L 23 >> #define VOL_CTRL1 0x08 >> @@ -46,6 +63,28 @@ >> #define LOLN_POL 8 >> #define LOLP_POL 12 >> #define POWER_CFG 0x10 >> +#define LINEIN_CFG 0x14 >> +#define MICBIAS_LEVEL 0 >> +#define MICBIAS_EN 3 >> +#define PGAR_CTVMN 8 >> +#define PGAR_CTVMP 9 >> +#define PGAL_CTVMN 10 >> +#define PGAL_CTVMP 11 >> +#define PGAR_CTVIN 12 >> +#define PGAR_CTVIP 13 >> +#define PGAL_CTVIN 14 >> +#define PGAL_CTVIP 15 >> + >> +#define PGAR_MASK (BIT(PGAR_CTVMP) | BIT(PGAR_CTVMN) | \ >> + BIT(PGAR_CTVIP) | BIT(PGAR_CTVIN)) >> +#define PGAR_DIFF (BIT(PGAR_CTVIP) | BIT(PGAR_CTVIN)) >> +#define PGAR_POSITIVE (BIT(PGAR_CTVIP) | BIT(PGAR_CTVMN)) >> +#define PGAR_NEGATIVE (BIT(PGAR_CTVIN) | BIT(PGAR_CTVMP)) >> +#define PGAL_MASK (BIT(PGAL_CTVMP) | BIT(PGAL_CTVMN) | \ >> + BIT(PGAL_CTVIP) | BIT(PGAL_CTVIN)) >> +#define PGAL_DIFF (BIT(PGAL_CTVIP) | BIT(PGAL_CTVIN)) >> +#define PGAL_POSITIVE (BIT(PGAL_CTVIP) | BIT(PGAL_CTVMN)) >> +#define PGAL_NEGATIVE (BIT(PGAL_CTVIN) | BIT(PGAL_CTVMP)) >> >> struct t9015 { >> struct regulator *avdd; >> @@ -103,6 +142,31 @@ static struct snd_soc_dai_driver t9015_dai = { >> .ops = &t9015_dai_ops, >> }; >> >> +static struct snd_soc_dai_driver a1_t9015_dai = { >> + .name = "t9015-hifi", >> + .playback = { >> + .stream_name = "Playback", >> + .channels_min = 1, >> + .channels_max = 2, >> + .rates = SNDRV_PCM_RATE_8000_96000, >> + .formats = (SNDRV_PCM_FMTBIT_S8 | >> + SNDRV_PCM_FMTBIT_S16_LE | >> + SNDRV_PCM_FMTBIT_S20_LE | >> + SNDRV_PCM_FMTBIT_S24_LE), >> + }, >> + .capture = { >> + .stream_name = "Capture", >> + .channels_min = 1, >> + .channels_max = 2, >> + .rates = SNDRV_PCM_RATE_8000_96000, >> + .formats = (SNDRV_PCM_FMTBIT_S8 | >> + SNDRV_PCM_FMTBIT_S16_LE | >> + SNDRV_PCM_FMTBIT_S20_LE | >> + SNDRV_PCM_FMTBIT_S24_LE), >> + }, >> + .ops = &t9015_dai_ops, >> +}; >> + >> static const DECLARE_TLV_DB_MINMAX_MUTE(dac_vol_tlv, -9525, 0); >> >> static const char * const ramp_rate_txt[] = { "Fast", "Slow" }; >> @@ -179,6 +243,166 @@ static const struct snd_soc_dapm_route t9015_dapm_routes[] = { >> { "LOLP", NULL, "Left+ Driver", }, >> }; >> >> +static const char * const a1_right_driver_txt[] = { "None", "Right DAC", >> + "Left DAC Inverted" }; >> +static const unsigned int a1_right_driver_values[] = { 0, 2, 4 }; >> + >> +static const char * const a1_left_driver_txt[] = { "None", "Left DAC", >> + "Right DAC Inverted" }; >> +static const unsigned int a1_left_driver_values[] = { 0, 2, 4 }; >> + >> +static SOC_VALUE_ENUM_SINGLE_DECL(a1_right_driver, LINEOUT_CFG, 12, 0x7, >> + a1_right_driver_txt, a1_right_driver_values); >> +static SOC_VALUE_ENUM_SINGLE_DECL(a1_left_driver, LINEOUT_CFG, 4, 0x7, >> + a1_left_driver_txt, a1_left_driver_values); >> + >> +static const struct snd_kcontrol_new a1_right_driver_mux = >> + SOC_DAPM_ENUM("Right Driver+ Source", a1_right_driver); >> +static const struct snd_kcontrol_new a1_left_driver_mux = >> + SOC_DAPM_ENUM("Left Driver+ Source", a1_left_driver); >> + >> +static const DECLARE_TLV_DB_MINMAX_MUTE(a1_adc_vol_tlv, -29625, 0); >> +static const DECLARE_TLV_DB_MINMAX_MUTE(a1_adc_pga_vol_tlv, -1200, 0); >> + >> +static const char * const a1_adc_right_txt[] = { "Right", "Left" }; >> +static SOC_ENUM_SINGLE_DECL(a1_adc_right, BLOCK_EN, ADCR_SRC, a1_adc_right_txt); >> + >> +static const char * const a1_adc_left_txt[] = { "Left", "Right" }; >> +static SOC_ENUM_SINGLE_DECL(a1_adc_left, BLOCK_EN, ADCL_SRC, a1_adc_left_txt); >> + >> +static const struct snd_kcontrol_new a1_adc_right_mux = >> + SOC_DAPM_ENUM("ADC Right Source", a1_adc_right); >> +static const struct snd_kcontrol_new a1_adc_left_mux = >> + SOC_DAPM_ENUM("ADC Left Source", a1_adc_left); >> + >> +static const char * const a1_adc_filter_mode_txt[] = { "Voice", "HiFi"}; >> +static SOC_ENUM_SINGLE_DECL(a1_adc_filter_mode, BLOCK_EN, ADC_FILTER_MODE, >> + a1_adc_filter_mode_txt); >> + >> +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 }; >> +static SOC_VALUE_ENUM_SINGLE_DECL(a1_adc_mic_bias_level, >> + LINEIN_CFG, MICBIAS_LEVEL, 0x7, >> + a1_adc_mic_bias_level_txt, >> + a1_adc_mic_bias_level_values); >> + >> +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 }; >> + >> +static SOC_VALUE_ENUM_SINGLE_DECL(a1_adc_pga_right, LINEIN_CFG, 0, PGAR_MASK, >> + a1_adc_pga_txt, a1_adc_pga_right_values); >> +static SOC_VALUE_ENUM_SINGLE_DECL(a1_adc_pga_left, LINEIN_CFG, 0, PGAL_MASK, >> + a1_adc_pga_txt, a1_adc_pga_left_values); >> + >> +static const struct snd_kcontrol_new a1_adc_pga_right_mux = >> + SOC_DAPM_ENUM("ADC PGA Right Source", a1_adc_pga_right); >> +static const struct snd_kcontrol_new a1_adc_pga_left_mux = >> + SOC_DAPM_ENUM("ADC PGA Left Source", a1_adc_pga_left); >> + >> +static const struct snd_kcontrol_new a1_t9015_snd_controls[] = { >> + /* Volume Controls */ >> + SOC_ENUM("Playback Channel Mode", mono_enum), >> + SOC_SINGLE("Playback Switch", VOL_CTRL1, DAC_SOFT_MUTE, 1, 1), >> + SOC_DOUBLE_TLV("Playback Volume", VOL_CTRL1, DACL_VC, DACR_VC, >> + 0xff, 0, dac_vol_tlv), >> + >> + /* Ramp Controls */ >> + SOC_ENUM("Ramp Rate", ramp_rate_enum), >> + SOC_SINGLE("Volume Ramp Switch", VOL_CTRL1, VC_RAMP_MODE, 1, 0), >> + SOC_SINGLE("Mute Ramp Switch", VOL_CTRL1, MUTE_MODE, 1, 0), >> + SOC_SINGLE("Unmute Ramp Switch", VOL_CTRL1, UNMUTE_MODE, 1, 0), >> + >> + /* ADC Controls */ >> + SOC_DOUBLE_TLV("ADC Volume", VOL_CTRL0, ADCL_VC, ADCR_VC, >> + 0x7f, 0, a1_adc_vol_tlv), >> + SOC_SINGLE("ADC Filter Switch", BLOCK_EN, ADC_FILTER_EN, 1, 0), >> + SOC_ENUM("ADC Filter Mode", a1_adc_filter_mode), >> + SOC_SINGLE("ADC Mic Bias Switch", LINEIN_CFG, MICBIAS_EN, 1, 0), >> + SOC_ENUM("ADC Mic Bias Level", a1_adc_mic_bias_level), >> + SOC_SINGLE("ADC DEM Switch", BLOCK_EN, ADC_DEM_EN, 1, 0), >> + SOC_DOUBLE_TLV("ADC PGA Volume", VOL_CTRL0, PGAR_VC, PGAL_VC, >> + 0x1f, 0, a1_adc_pga_vol_tlv), >> + SOC_DOUBLE("ADC PGA Zero Cross-detection Switch", BLOCK_EN, >> + PGAL_ZCD_EN, PGAR_ZCD_EN, 1, 0), >> +}; >> + >> +static const struct snd_soc_dapm_widget a1_t9015_dapm_widgets[] = { >> + SND_SOC_DAPM_AIF_IN("Right IN", NULL, 0, SND_SOC_NOPM, 0, 0), >> + SND_SOC_DAPM_AIF_IN("Left IN", NULL, 0, SND_SOC_NOPM, 0, 0), >> + SND_SOC_DAPM_MUX("Right DAC Sel", SND_SOC_NOPM, 0, 0, >> + &t9015_right_dac_mux), >> + SND_SOC_DAPM_MUX("Left DAC Sel", SND_SOC_NOPM, 0, 0, >> + &t9015_left_dac_mux), >> + SND_SOC_DAPM_DAC("Right DAC", NULL, BLOCK_EN, DACR_EN, 0), >> + SND_SOC_DAPM_DAC("Left DAC", NULL, BLOCK_EN, DACL_EN, 0), >> + SND_SOC_DAPM_MUX("Right+ Driver Sel", SND_SOC_NOPM, 0, 0, >> + &a1_right_driver_mux), >> + SND_SOC_DAPM_MUX("Left+ Driver Sel", SND_SOC_NOPM, 0, 0, >> + &a1_left_driver_mux), >> + SND_SOC_DAPM_OUT_DRV("Right+ Driver", BLOCK_EN, LORP_EN, 0, NULL, 0), >> + SND_SOC_DAPM_OUT_DRV("Left+ Driver", BLOCK_EN, LOLP_EN, 0, NULL, 0), >> + SND_SOC_DAPM_OUTPUT("LORP"), >> + SND_SOC_DAPM_OUTPUT("LOLP"), >> + >> + SND_SOC_DAPM_INPUT("ADC IN Right"), >> + SND_SOC_DAPM_INPUT("ADC IN Left"), >> + SND_SOC_DAPM_MUX("ADC PGA Right Sel", SND_SOC_NOPM, 0, 0, >> + &a1_adc_pga_right_mux), >> + SND_SOC_DAPM_MUX("ADC PGA Left Sel", SND_SOC_NOPM, 0, 0, >> + &a1_adc_pga_left_mux), >> + SND_SOC_DAPM_PGA("ADC PGA Right", BLOCK_EN, PGAR_EN, 0, NULL, 0), >> + SND_SOC_DAPM_PGA("ADC PGA Left", BLOCK_EN, PGAL_EN, 0, NULL, 0), >> + SND_SOC_DAPM_ADC("ADC Right", NULL, BLOCK_EN, ADCR_EN, 0), >> + SND_SOC_DAPM_ADC("ADC Left", NULL, BLOCK_EN, ADCL_EN, 0), >> + SND_SOC_DAPM_MUX("ADC Right Sel", SND_SOC_NOPM, 0, 0, &a1_adc_right_mux), >> + SND_SOC_DAPM_MUX("ADC Left Sel", SND_SOC_NOPM, 0, 0, &a1_adc_left_mux), >> + SND_SOC_DAPM_AIF_OUT("ADC OUT Right", NULL, 0, SND_SOC_NOPM, 0, 0), >> + SND_SOC_DAPM_AIF_OUT("ADC OUT Left", NULL, 0, SND_SOC_NOPM, 0, 0), >> +}; >> + >> +static const struct snd_soc_dapm_route a1_t9015_dapm_routes[] = { >> + { "Right IN", NULL, "Playback" }, >> + { "Left IN", NULL, "Playback" }, >> + { "Right DAC Sel", "Right", "Right IN" }, >> + { "Right DAC Sel", "Left", "Left IN" }, >> + { "Left DAC Sel", "Right", "Right IN" }, >> + { "Left DAC Sel", "Left", "Left IN" }, >> + { "Right DAC", NULL, "Right DAC Sel" }, >> + { "Left DAC", NULL, "Left DAC Sel" }, >> + { "Right+ Driver Sel", "Right DAC", "Right DAC" }, >> + { "Right+ Driver Sel", "Left DAC Inverted", "Right DAC" }, >> + { "Left+ Driver Sel", "Left DAC", "Left DAC" }, >> + { "Left+ Driver Sel", "Right DAC Inverted", "Left DAC" }, >> + { "Right+ Driver", NULL, "Right+ Driver Sel" }, >> + { "Left+ Driver", NULL, "Left+ Driver Sel" }, >> + { "LORP", NULL, "Right+ Driver", }, >> + { "LOLP", NULL, "Left+ Driver", }, >> + >> + { "ADC PGA Right Sel", "Differential", "ADC IN Right" }, >> + { "ADC PGA Right Sel", "Positive", "ADC IN Right" }, >> + { "ADC PGA Right Sel", "Negative", "ADC IN Right" }, >> + { "ADC PGA Left Sel", "Differential", "ADC IN Left" }, >> + { "ADC PGA Left Sel", "Positive", "ADC IN Left" }, >> + { "ADC PGA Left Sel", "Negative", "ADC IN Left" }, >> + { "ADC PGA Right", NULL, "ADC PGA Right Sel" }, >> + { "ADC PGA Left", NULL, "ADC PGA Left Sel" }, >> + { "ADC Right", NULL, "ADC PGA Right" }, >> + { "ADC Left", NULL, "ADC PGA Left" }, >> + { "ADC Right Sel", "Right", "ADC Right" }, >> + { "ADC Right Sel", "Left", "ADC Left" }, >> + { "ADC Left Sel", "Right", "ADC Right" }, >> + { "ADC Left Sel", "Left", "ADC Left" }, >> + { "ADC OUT Right", NULL, "ADC Right Sel" }, >> + { "ADC OUT Left", NULL, "ADC Left Sel" }, >> + { "Capture", NULL, "ADC OUT Right" }, >> + { "Capture", NULL, "ADC OUT Left" }, >> +}; >> + >> static int t9015_set_bias_level(struct snd_soc_component *component, >> enum snd_soc_bias_level level) >> { >> @@ -241,6 +465,18 @@ static int t9015_component_probe(struct snd_soc_component *component) >> return 0; >> } >> >> +static int a1_t9015_component_probe(struct snd_soc_component *component) >> +{ >> + /* >> + * This configuration was stealed from original Amlogic's driver to >> + * reproduce the behavior of the driver more accurately. However, it is >> + * not known for certain what it actually affects. >> + */ >> + snd_soc_component_write(component, POWER_CFG, 0x00010000); >> + >> + return 0; >> +} >> + >> static const struct snd_soc_component_driver t9015_codec_driver = { >> .probe = t9015_component_probe, >> .set_bias_level = t9015_set_bias_level, >> @@ -254,6 +490,19 @@ static const struct snd_soc_component_driver t9015_codec_driver = { >> .endianness = 1, >> }; >> >> +static const struct snd_soc_component_driver a1_t9015_codec_driver = { >> + .probe = a1_t9015_component_probe, >> + .set_bias_level = t9015_set_bias_level, >> + .controls = a1_t9015_snd_controls, >> + .num_controls = ARRAY_SIZE(a1_t9015_snd_controls), >> + .dapm_widgets = a1_t9015_dapm_widgets, >> + .num_dapm_widgets = ARRAY_SIZE(a1_t9015_dapm_widgets), >> + .dapm_routes = a1_t9015_dapm_routes, >> + .num_dapm_routes = ARRAY_SIZE(a1_t9015_dapm_routes), >> + .suspend_bias_off = 1, >> + .endianness = 1, >> +}; >> + >> static int t9015_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -315,11 +564,21 @@ static const struct t9015_match_data t9015_match_data = { >> .max_register = POWER_CFG, >> }; >> >> +static const struct t9015_match_data a1_t9015_match_data = { >> + .component_drv = &a1_t9015_codec_driver, >> + .dai_drv = &a1_t9015_dai, >> + .max_register = LINEIN_CFG, >> +}; >> + >> static const struct of_device_id t9015_ids[] __maybe_unused = { >> { >> .compatible = "amlogic,t9015", >> .data = &t9015_match_data, >> }, >> + { >> + .compatible = "amlogic,t9015-a1", >> + .data = &a1_t9015_match_data, >> + }, >> { } >> }; >> MODULE_DEVICE_TABLE(of, t9015_ids); > > -- Best regards Jan Dakinevich
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.
--
Best regards
Jan Dakinevich
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), >>>> }, >>> >>> > > -- Best regards Jan Dakinevich
Is there any implicit or expected guarantee that after having returned from gpiod_set_value that the GPIO will reflect the new value externally? Some drivers that leverage GPIO to emulate buses, like i2c-gpio, may be relying on this to be true in order to make a "stable" clock. I was glancing at https://github.com/raspberrypi/linux/issues/5554 where someone ran into an issue and it looks like, at least on the RPi 4 platform, that there may need to be some "flush" mechanism to guarantee that a GPIO has been written out. If it's the responsibility of gpio_chip->set to do this, then I'm guessing the pinctrl driver may need to be updated, but that does incur a performance hit for every GPIO write. If it's up to the bus emulator to do this, short of sampling the pin, should there be some API or mechanism to assist with flushing writes out? Is there already a mechanism to do this? Thanks, -Vincent
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master branch HEAD: 2e93f143ca010a5013528e1cfdc895f024fe8c21 Add linux-next specific files for 20240318 Error/Warning ids grouped by kconfigs: gcc_recent_errors |-- arc-allmodconfig | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- arc-allyesconfig | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- arm-allmodconfig | |-- arch-arm-mach-omap2-prm33xx.c:warning:expecting-prototype-for-am33xx_prm_global_warm_sw_reset().-Prototype-was-for-am33xx_prm_global_sw_reset()-instead | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- arm-allyesconfig | |-- arch-arm-mach-omap2-prm33xx.c:warning:expecting-prototype-for-am33xx_prm_global_warm_sw_reset().-Prototype-was-for-am33xx_prm_global_sw_reset()-instead | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- arm64-defconfig | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- csky-allmodconfig | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- csky-allyesconfig | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- i386-allyesconfig | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- i386-randconfig-001-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- i386-randconfig-004-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- i386-randconfig-011-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- i386-randconfig-141-20240318 | `-- fs-exfat-dir.c-__exfat_get_dentry_set()-warn:missing-unwind-goto |-- loongarch-allmodconfig | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- m68k-allmodconfig | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- m68k-allyesconfig | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- microblaze-allmodconfig | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- microblaze-allyesconfig | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- mips-allyesconfig | |-- (.ref.text):relocation-truncated-to-fit:R_MIPS_26-against-start_secondary | |-- (.text):relocation-truncated-to-fit:R_MIPS_26-against-kernel_entry | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- nios2-allmodconfig | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- nios2-allyesconfig | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- openrisc-allyesconfig | |-- (.head.text):relocation-truncated-to-fit:R_OR1K_INSN_REL_26-against-no-symbol | |-- (.text):relocation-truncated-to-fit:R_OR1K_INSN_REL_26-against-no-symbol | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | |-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead | `-- main.c:(.text):relocation-truncated-to-fit:R_OR1K_INSN_REL_26-against-symbol-__muldi3-defined-in-.text-section-in-..-lib-gcc-or1k-linux-..-libgcc.a(_muldi3.o) |-- parisc-allmodconfig | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- parisc-allyesconfig | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- parisc-randconfig-001-20240318 | `-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and |-- parisc-randconfig-r113-20240318 | `-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and |-- powerpc-allmodconfig | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- powerpc-randconfig-002-20240318 | `-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and |-- powerpc64-randconfig-002-20240318 | `-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and |-- powerpc64-randconfig-003-20240318 | `-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and |-- riscv-randconfig-001-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- sh-sdk7786_defconfig | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- sparc-randconfig-001-20240318 | |-- (.head.text):relocation-truncated-to-fit:R_SPARC_WDISP22-against-init.text | `-- (.init.text):relocation-truncated-to-fit:R_SPARC_WDISP22-against-symbol-leon_smp_cpu_startup-defined-in-.text-section-in-arch-sparc-kernel-trampoline_32.o |-- sparc-randconfig-002-20240318 | |-- (.head.text):relocation-truncated-to-fit:R_SPARC_WDISP22-against-init.text | `-- (.init.text):relocation-truncated-to-fit:R_SPARC_WDISP22-against-symbol-leon_smp_cpu_startup-defined-in-.text-section-in-arch-sparc-kernel-trampoline_32.o |-- sparc64-allmodconfig | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- sparc64-allyesconfig | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- sparc64-randconfig-001-20240318 | `-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and |-- sparc64-randconfig-002-20240318 | |-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- sparc64-randconfig-r131-20240318 | `-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and |-- x86_64-buildonly-randconfig-002-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- x86_64-buildonly-randconfig-003-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- x86_64-buildonly-randconfig-005-20240318 | `-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and |-- x86_64-randconfig-072-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- x86_64-randconfig-073-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- x86_64-randconfig-076-20240318 | `-- drivers-gpu-drm-amd-amdgpu-amdgpu_vcn.c:warning:.bin-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and `-- x86_64-randconfig-161-20240318 |-- drivers-pinctrl-pinctrl-aw9523.c-aw9523_gpio_get_multiple()-error:uninitialized-symbol-ret-. `-- drivers-pinctrl-pinctrl-aw9523.c-aw9523_pconf_set()-error:uninitialized-symbol-rc-. clang_recent_errors |-- arm-defconfig | |-- arch-arm-mach-omap2-prm33xx.c:warning:expecting-prototype-for-am33xx_prm_global_warm_sw_reset().-Prototype-was-for-am33xx_prm_global_sw_reset()-instead | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- arm-randconfig-002-20240318 | `-- arch-arm-mach-omap2-prm33xx.c:warning:expecting-prototype-for-am33xx_prm_global_warm_sw_reset().-Prototype-was-for-am33xx_prm_global_sw_reset()-instead |-- arm-randconfig-003-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- hexagon-randconfig-r121-20240318 | |-- fs-libfs.c:sparse:sparse:Using-plain-integer-as-NULL-pointer | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- mips-randconfig-r111-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- powerpc-allyesconfig | |-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead | `-- kernel-bpf-bpf_struct_ops.c:warning:bitwise-operation-between-different-enumeration-types-(-enum-bpf_type_flag-and-enum-bpf_reg_type-) |-- powerpc-randconfig-001-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead |-- riscv-allmodconfig | |-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead | `-- kernel-bpf-bpf_struct_ops.c:warning:bitwise-operation-between-different-enumeration-types-(-enum-bpf_type_flag-and-enum-bpf_reg_type-) |-- riscv-allyesconfig | |-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead | `-- kernel-bpf-bpf_struct_ops.c:warning:bitwise-operation-between-different-enumeration-types-(-enum-bpf_type_flag-and-enum-bpf_reg_type-) |-- s390-defconfig | `-- kernel-bpf-bpf_struct_ops.c:warning:bitwise-operation-between-different-enumeration-types-(-enum-bpf_type_flag-and-enum-bpf_reg_type-) |-- x86_64-randconfig-002-20240318 | `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead `-- x86_64-randconfig-074-20240318 `-- fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead elapsed time: 729m configs tested: 179 configs skipped: 4 tested configs: alpha allnoconfig gcc alpha allyesconfig gcc alpha defconfig gcc arc allmodconfig gcc arc allnoconfig gcc arc allyesconfig gcc arc defconfig gcc arc randconfig-001-20240318 gcc arc randconfig-002-20240318 gcc arm allmodconfig gcc arm allnoconfig clang arm allyesconfig gcc arm defconfig clang arm omap1_defconfig gcc arm randconfig-001-20240318 clang arm randconfig-002-20240318 clang arm randconfig-003-20240318 clang arm randconfig-004-20240318 gcc arm s5pv210_defconfig gcc arm shmobile_defconfig gcc arm64 allnoconfig gcc arm64 defconfig gcc arm64 randconfig-001-20240318 gcc arm64 randconfig-002-20240318 gcc arm64 randconfig-003-20240318 clang arm64 randconfig-004-20240318 gcc csky allmodconfig gcc csky allnoconfig gcc csky allyesconfig gcc csky defconfig gcc csky randconfig-001-20240318 gcc csky randconfig-002-20240318 gcc hexagon allmodconfig clang hexagon allnoconfig clang hexagon allyesconfig clang hexagon defconfig clang hexagon randconfig-001-20240318 clang hexagon randconfig-002-20240318 clang i386 allmodconfig gcc i386 allnoconfig gcc i386 allyesconfig gcc i386 buildonly-randconfig-001-20240318 gcc i386 buildonly-randconfig-002-20240318 clang i386 buildonly-randconfig-003-20240318 clang i386 buildonly-randconfig-004-20240318 clang i386 buildonly-randconfig-005-20240318 gcc i386 buildonly-randconfig-006-20240318 gcc i386 defconfig clang i386 randconfig-001-20240318 gcc i386 randconfig-002-20240318 clang i386 randconfig-003-20240318 gcc i386 randconfig-004-20240318 gcc i386 randconfig-005-20240318 gcc i386 randconfig-006-20240318 gcc i386 randconfig-011-20240318 gcc i386 randconfig-012-20240318 clang i386 randconfig-013-20240318 clang i386 randconfig-014-20240318 clang i386 randconfig-015-20240318 clang i386 randconfig-016-20240318 gcc loongarch allmodconfig gcc loongarch allnoconfig gcc loongarch defconfig gcc loongarch randconfig-001-20240318 gcc loongarch randconfig-002-20240318 gcc m68k allmodconfig gcc m68k allnoconfig gcc m68k allyesconfig gcc m68k defconfig gcc m68k m5208evb_defconfig gcc m68k m5249evb_defconfig gcc microblaze allmodconfig gcc microblaze allnoconfig gcc microblaze allyesconfig gcc microblaze defconfig gcc mips allnoconfig gcc mips allyesconfig gcc mips bcm47xx_defconfig clang mips bmips_be_defconfig gcc mips lemote2f_defconfig gcc mips rt305x_defconfig clang nios2 allmodconfig gcc nios2 allnoconfig gcc nios2 allyesconfig gcc nios2 defconfig gcc nios2 randconfig-001-20240318 gcc nios2 randconfig-002-20240318 gcc openrisc allnoconfig gcc openrisc allyesconfig gcc openrisc defconfig gcc parisc allmodconfig gcc parisc allnoconfig gcc parisc allyesconfig gcc parisc defconfig gcc parisc randconfig-001-20240318 gcc parisc randconfig-002-20240318 gcc parisc64 defconfig gcc powerpc allmodconfig gcc powerpc allnoconfig gcc powerpc allyesconfig clang powerpc motionpro_defconfig clang powerpc ps3_defconfig gcc powerpc randconfig-001-20240318 clang powerpc randconfig-002-20240318 gcc powerpc randconfig-003-20240318 clang powerpc redwood_defconfig clang powerpc wii_defconfig gcc powerpc64 randconfig-001-20240318 clang powerpc64 randconfig-002-20240318 gcc powerpc64 randconfig-003-20240318 gcc riscv allmodconfig clang riscv allnoconfig gcc riscv allyesconfig clang riscv defconfig clang riscv randconfig-001-20240318 gcc riscv randconfig-002-20240318 gcc s390 allmodconfig clang s390 allnoconfig clang s390 allyesconfig gcc s390 defconfig clang s390 randconfig-001-20240318 clang s390 randconfig-002-20240318 clang sh allmodconfig gcc sh allnoconfig gcc sh allyesconfig gcc sh defconfig gcc sh edosk7760_defconfig gcc sh randconfig-001-20240318 gcc sh randconfig-002-20240318 gcc sh sdk7786_defconfig gcc sh sh7757lcr_defconfig gcc sparc alldefconfig gcc sparc allmodconfig gcc sparc allnoconfig gcc sparc defconfig gcc sparc64 allmodconfig gcc sparc64 allyesconfig gcc sparc64 defconfig gcc sparc64 randconfig-001-20240318 gcc sparc64 randconfig-002-20240318 gcc um allmodconfig clang um allnoconfig clang um allyesconfig gcc um defconfig clang um i386_defconfig gcc um randconfig-001-20240318 gcc um randconfig-002-20240318 clang um x86_64_defconfig clang x86_64 allnoconfig clang x86_64 allyesconfig clang x86_64 buildonly-randconfig-001-20240318 gcc x86_64 buildonly-randconfig-002-20240318 gcc x86_64 buildonly-randconfig-003-20240318 gcc x86_64 buildonly-randconfig-004-20240318 gcc x86_64 buildonly-randconfig-005-20240318 gcc x86_64 buildonly-randconfig-006-20240318 gcc x86_64 defconfig gcc x86_64 randconfig-001-20240318 gcc x86_64 randconfig-002-20240318 clang x86_64 randconfig-003-20240318 clang x86_64 randconfig-004-20240318 clang x86_64 randconfig-005-20240318 clang x86_64 randconfig-006-20240318 gcc x86_64 randconfig-011-20240318 clang x86_64 randconfig-012-20240318 clang x86_64 randconfig-013-20240318 gcc x86_64 randconfig-014-20240318 gcc x86_64 randconfig-015-20240318 clang x86_64 randconfig-016-20240318 gcc x86_64 randconfig-071-20240318 gcc x86_64 randconfig-072-20240318 gcc x86_64 randconfig-073-20240318 gcc x86_64 randconfig-074-20240318 clang x86_64 randconfig-075-20240318 gcc x86_64 randconfig-076-20240318 gcc x86_64 rhel-8.3-rust clang xtensa allnoconfig gcc xtensa randconfig-001-20240318 gcc xtensa randconfig-002-20240318 gcc -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Lockdep detects a possible deadlock as listed below. This is because it detects the IA55 interrupt controller .irq_eoi() API is called from interrupt context while configuration-specific API (e.g., .irq_enable()) could be called from process context on resume path (by calling rzg2l_gpio_irq_restore()). To avoid this, protect the call of rzg2l_gpio_irq_enable() with spin_lock_irqsave()/spin_unlock_irqrestore(). With this the same approach that is available in __setup_irq() is mimicked to pinctrl IRQ resume function. Below is the lockdep report: WARNING: inconsistent lock state 6.8.0-rc5-next-20240219-arm64-renesas-00030-gb17a289abf1f #90 Not tainted -------------------------------- inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage. str_rwdt_t_001./159 [HC0[0]:SC0[0]:HE1:SE1] takes: ffff00000b001d70 (&rzg2l_irqc_data->lock){?...}-{2:2}, at: rzg2l_irqc_irq_enable+0x60/0xa4 {IN-HARDIRQ-W} state was registered at: lock_acquire+0x1e0/0x310 _raw_spin_lock+0x44/0x58 rzg2l_irqc_eoi+0x2c/0x130 irq_chip_eoi_parent+0x18/0x20 rzg2l_gpio_irqc_eoi+0xc/0x14 handle_fasteoi_irq+0x134/0x230 generic_handle_domain_irq+0x28/0x3c gic_handle_irq+0x4c/0xbc call_on_irq_stack+0x24/0x34 do_interrupt_handler+0x78/0x7c el1_interrupt+0x30/0x5c el1h_64_irq_handler+0x14/0x1c el1h_64_irq+0x64/0x68 _raw_spin_unlock_irqrestore+0x34/0x70 __setup_irq+0x4d4/0x6b8 request_threaded_irq+0xe8/0x1a0 request_any_context_irq+0x60/0xb8 devm_request_any_context_irq+0x74/0x104 gpio_keys_probe+0x374/0xb08 platform_probe+0x64/0xcc really_probe+0x140/0x2ac __driver_probe_device+0x74/0x124 driver_probe_device+0x3c/0x15c __driver_attach+0xec/0x1c4 bus_for_each_dev+0x70/0xcc driver_attach+0x20/0x28 bus_add_driver+0xdc/0x1d0 driver_register+0x5c/0x118 __platform_driver_register+0x24/0x2c gpio_keys_init+0x18/0x20 do_one_initcall+0x70/0x290 kernel_init_freeable+0x294/0x504 kernel_init+0x20/0x1cc ret_from_fork+0x10/0x20 irq event stamp: 69071 hardirqs last enabled at (69071): [<ffff800080e0dafc>] _raw_spin_unlock_irqrestore+0x6c/0x70 hardirqs last disabled at (69070): [<ffff800080e0cfec>] _raw_spin_lock_irqsave+0x7c/0x80 softirqs last enabled at (67654): [<ffff800080010614>] __do_softirq+0x494/0x4dc softirqs last disabled at (67645): [<ffff800080015238>] ____do_softirq+0xc/0x14 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(&rzg2l_irqc_data->lock); <Interrupt> lock(&rzg2l_irqc_data->lock); *** DEADLOCK *** 4 locks held by str_rwdt_t_001./159: #0: ffff00000b10f3f0 (sb_writers#4){.+.+}-{0:0}, at: vfs_write+0x1a4/0x35c #1: ffff00000e43ba88 (&of->mutex){+.+.}-{3:3}, at: kernfs_fop_write_iter+0xe8/0x1a8 #2: ffff00000aa21dc8 (kn->active#40){.+.+}-{0:0}, at: kernfs_fop_write_iter+0xf0/0x1a8 #3: ffff80008179d970 (system_transition_mutex){+.+.}-{3:3}, at: pm_suspend+0x9c/0x278 stack backtrace: CPU: 0 PID: 159 Comm: str_rwdt_t_001. Not tainted 6.8.0-rc5-next-20240219-arm64-renesas-00030-gb17a289abf1f #90 Hardware name: Renesas SMARC EVK version 2 based on r9a08g045s33 (DT) Call trace: dump_backtrace+0x94/0xe8 show_stack+0x14/0x1c dump_stack_lvl+0x88/0xc4 dump_stack+0x14/0x1c print_usage_bug.part.0+0x294/0x348 mark_lock+0x6b0/0x948 __lock_acquire+0x750/0x20b0 lock_acquire+0x1e0/0x310 _raw_spin_lock+0x44/0x58 rzg2l_irqc_irq_enable+0x60/0xa4 irq_chip_enable_parent+0x1c/0x34 rzg2l_gpio_irq_enable+0xc4/0xd8 rzg2l_pinctrl_resume_noirq+0x4cc/0x520 pm_generic_resume_noirq+0x28/0x3c genpd_finish_resume+0xc0/0xdc genpd_resume_noirq+0x14/0x1c dpm_run_callback+0x34/0x90 device_resume_noirq+0xa8/0x268 dpm_noirq_resume_devices+0x13c/0x160 dpm_resume_noirq+0xc/0x1c suspend_devices_and_enter+0x2c8/0x570 pm_suspend+0x1ac/0x278 state_store+0x88/0x124 kobj_attr_store+0x14/0x24 sysfs_kf_write+0x48/0x6c kernfs_fop_write_iter+0x118/0x1a8 vfs_write+0x270/0x35c ksys_write+0x64/0xec __arm64_sys_write+0x18/0x20 invoke_syscall+0x44/0x108 el0_svc_common.constprop.0+0xb4/0xd4 do_el0_svc+0x18/0x20 el0_svc+0x3c/0xb8 el0t_64_sync_handler+0xb8/0xbc el0t_64_sync+0x14c/0x150 Fixes: 254203f9a94c ("pinctrl: renesas: rzg2l: Add suspend/resume support") Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> --- Changes in v2: - used raw_spin_lock_irqsave()/raw_spin_unlock_irqrestore() drivers/pinctrl/renesas/pinctrl-rzg2l.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c index eb5a8c654260..93916553bcc7 100644 --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c @@ -2063,8 +2063,17 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl) continue; } - if (!irqd_irq_disabled(data)) + if (!irqd_irq_disabled(data)) { + unsigned long flags; + + /* + * This has to be atomically executed to protect against a concurrent + * interrupt. + */ + raw_spin_lock_irqsave(&pctrl->lock.rlock, flags); rzg2l_gpio_irq_enable(data); + raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags); + } } } -- 2.39.2
[-- Attachment #1: Type: text/plain, Size: 773 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #1: Type: text/plain, Size: 437 bytes --] On Sun, Mar 17, 2024 at 06:19:36PM +0300, Jan Dakinevich wrote: > On 3/15/24 16:33, Mark Brown wrote: > > If the maximum register is 0 how does the regmap have a stride? > reg_stride inherited from existing code. Apparently, it was meaningless > even before my modifications (the hardware has single register > regardless of max_register declaration) and it should be dropped. But, > is it okay to remove it in the same commit? Sure. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
On 3/3/2024 12:55 AM, Bjorn Andersson wrote: > On Tue, Feb 27, 2024 at 09:23:06PM +0530, Mukesh Ojha wrote: >> qcom_scm_is_available() gives wrong indication if __scm >> is initialized but __scm->dev is not. >> >> Fix this appropriately by making sure if __scm is >> initialized and then it is associated with its >> device. >> > > This seems like a bug fix, and should as such have a Fixes: tag and > probably Cc: stable@vger.kernel.org > >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >> --- >> drivers/firmware/qcom/qcom_scm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c >> index 6c252cddd44e..6f14254c0c10 100644 >> --- a/drivers/firmware/qcom/qcom_scm.c >> +++ b/drivers/firmware/qcom/qcom_scm.c >> @@ -1859,6 +1859,7 @@ static int qcom_scm_probe(struct platform_device *pdev) >> if (!scm) >> return -ENOMEM; >> >> + scm->dev = &pdev->dev; >> ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr); >> if (ret < 0) >> return ret; >> @@ -1895,7 +1896,6 @@ static int qcom_scm_probe(struct platform_device *pdev) >> return ret; >> >> __scm = scm; >> - __scm->dev = &pdev->dev; > > Is it sufficient to just move the line up, or do we need a barrier of > some sort here? Would be good to use, smp_mb() before the assignment __scm = scm along with moving below line __scm->dev = &pdev->dev somewhere up. -Mukesh > > Regards, > Bjorn > >> >> init_completion(&__scm->waitq_comp); >> >> -- >> 2.43.0.254.ga26002b62827 >>
On Mon 18 Mar 2024 at 11:55, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Sun 17 Mar 2024 at 18:52, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> On 3/15/24 13:22, Jerome Brunet wrote: >>> >>> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 15/03/2024 00:21, Jan Dakinevich wrote: >>>>> This option allow to redefine the rate of DSP system clock. >>>> >>>> And why is it suitable for bindings? Describe the hardware, not what you >>>> want to do in the driver. >>>> >>>>> >>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>>> --- >>>>> Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>> index df21dd72fc65..d2f23a59a6b6 100644 >>>>> --- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>> +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>> @@ -40,6 +40,10 @@ properties: >>>>> resets: >>>>> maxItems: 1 >>>>> >>>>> + sysrate: >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + description: redefine rate of DSP system clock >>>> >>>> No vendor prefix, so is it a generic property? Also, missing unit >>>> suffix, but more importantly I don't understand why this is a property >>>> of hardware. >>> >>> +1. >>> >>> The appropriate way to set rate of the clock before the driver take over >>> is 'assigned-rate', if you need to customize this for different >>> platform. >>> >> >> It would be great, but it doesn't work. Below, is what I want to see: >> >> assigned-clocks = >> <&clkc_audio AUD2_CLKID_PDM_SYSCLK_SEL>, >> <&clkc_audio AUD2_CLKID_PDM_SYSCLK_DIV>; >> assigned-clock-parents = >> <&clkc_pll CLKID_FCLK_DIV3>, >> <0>; >> assigned-clock-rates = >> <0>, >> <256000000>; >> >> But regardles of this declaration, PDM's driver unconditionally sets >> sysclk'rate to 250MHz and throws away everything that was configured >> before, reparents audio2_pdm_sysclk_mux to hifi_pll and changes >> hifi_pll's rate. >> >> This value 250MHz is declared here: >> >> static const struct axg_pdm_cfg axg_pdm_config = { >> .filters = &axg_default_filters, >> .sys_rate = 250000000, >> }; >> >> The property 'sysrate' is intended to redefine hardcoded 'sys_rate' >> value in 'axg_pdm_config'. > > What is stopping you from removing that from the driver and adding > assigned-rate to 250M is the existing platform ? ... Also, considering how PDM does work, I'm not sure I get the point of the doing all this to go from 250MHz to 256Mhz. PDM value is sampled at ~75% of the half period. That clock basically feeds a counter and the threshold is adjusted based on the clock rate. So there is no need to change the rate. Changing it is only necessary when the captured audio rate is extremely slow (<8kHz) and the counter may overflow. The driver already adjust this automatically. So changing the input rate from 250MHz to 256MHz should not make any difference. > >> >>> Then you don't have to deal with it in the device driver. >>> >>>> >>>> Best regards, >>>> Krzysztof >>> >>> -- Jerome
Hello, Currently I am implementing a pinctrl driver for the Renesas RZ/V2H SoC. I will be reusing the RZ/G2L pinctrl driver for this. On the RZ/V2H SoC there are a couple of settings (like output-impedance) which vary depending on what power rail it's connected to. For example, for the output impedance there are 4 groups of pins. - Group1: Impedance is 150Ω / 75Ω / 38Ω / 25Ω (at 3.3 V) 130Ω / 65Ω / 33Ω / 22Ω (at 1.8 V) - Group 2: Impedance is 50Ω / 40Ω / 33Ω / 25Ω (at 1.8V) - Group 3: Impedance is 150Ω / 75Ω / 37.5Ω / 25Ω (at 3.3 V) 130Ω / 65Ω / 33Ω / 22Ω (at 1.8 V) - Group 4: Impedance is 110Ω / 55Ω / 30Ω / 20Ω (at 1.8 V) 150Ω / 75Ω / 38Ω / 25Ω (at 1.2 V) The power rails connected to these pin groups will be connected to PMIC. Below are the options have been explored, Option#1 - Passing the power rail information from the PMIC to PFC (pinctrl driver) so that pinctrl driver can read the voltage level and set the values accordingly. Here we will be using the PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS to get/set values Pros: • Debugfs can show the value in ohms Cons: • Race condition at boot between pfc, i2c, and pmic • Late time of probing • Impossible to validate dt-bindings correctly • Manual doesn't say that pfc has access to the power rails, this could be a challenge With option #1 I am currently using fixed regulators but I see an issue when we add a PMIC driver with regulators for example if i2c pinmux (to which pmic is connected) that itself requires output-impedance setting. Option#2 - Specify the voltage in the pinmux/pins child node alongside the output impedance (using power-source property) Pros: • both driver and bindings can validate the settings Cons: • the figure of the voltage supplied will have been replicated, as it would be listed in the corresponding power regulator, but also in the definition of the pin Option#3 - Have an IP specific compatible ("renesas,v2h-output-impedance") with value 1, 2, 4 or 6 (which indicates x1, x2, x4, x6 strength) Pros: • Very simple to support and validate. • The user cannot really get this wrong Cons: • new proprietary property • we would not be using the output impedance property offered by the subsystem Please share your thoughts what could be the best approach to add pinctrl support for RZ/V2H Cheers, Prabhakar
On Sun 17 Mar 2024 at 18:52, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > On 3/15/24 13:22, Jerome Brunet wrote: >> >> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >> >>> On 15/03/2024 00:21, Jan Dakinevich wrote: >>>> This option allow to redefine the rate of DSP system clock. >>> >>> And why is it suitable for bindings? Describe the hardware, not what you >>> want to do in the driver. >>> >>>> >>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>> --- >>>> Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>> index df21dd72fc65..d2f23a59a6b6 100644 >>>> --- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>> +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>> @@ -40,6 +40,10 @@ properties: >>>> resets: >>>> maxItems: 1 >>>> >>>> + sysrate: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: redefine rate of DSP system clock >>> >>> No vendor prefix, so is it a generic property? Also, missing unit >>> suffix, but more importantly I don't understand why this is a property >>> of hardware. >> >> +1. >> >> The appropriate way to set rate of the clock before the driver take over >> is 'assigned-rate', if you need to customize this for different >> platform. >> > > It would be great, but it doesn't work. Below, is what I want to see: > > assigned-clocks = > <&clkc_audio AUD2_CLKID_PDM_SYSCLK_SEL>, > <&clkc_audio AUD2_CLKID_PDM_SYSCLK_DIV>; > assigned-clock-parents = > <&clkc_pll CLKID_FCLK_DIV3>, > <0>; > assigned-clock-rates = > <0>, > <256000000>; > > But regardles of this declaration, PDM's driver unconditionally sets > sysclk'rate to 250MHz and throws away everything that was configured > before, reparents audio2_pdm_sysclk_mux to hifi_pll and changes > hifi_pll's rate. > > This value 250MHz is declared here: > > static const struct axg_pdm_cfg axg_pdm_config = { > .filters = &axg_default_filters, > .sys_rate = 250000000, > }; > > The property 'sysrate' is intended to redefine hardcoded 'sys_rate' > value in 'axg_pdm_config'. What is stopping you from removing that from the driver and adding assigned-rate to 250M is the existing platform ? > >> Then you don't have to deal with it in the device driver. >> >>> >>> Best regards, >>> Krzysztof >> >> -- Jerome
On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > A1's internal codec is very close to t9015. The main difference, that it > has ADC. This commit introduces support for capturing from it. This is mis-leading. It does not look like the change is A1 specific but rather a extension of the support for t9015. It also mixes several different topics like line configuration, capture support, etc ... Again, the t9015 changes should be a separated series from the rest, and there should be one patch per topic. As Mark, if something is meant to be configured based on the HW layout, then there a good change a kcontrol is not appropriate, and this should rather be part of the platform description, like DT. It was also suggested here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/meson/t9015.c?h=v6.8#n298 > > Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> > --- > sound/soc/meson/t9015.c | 259 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 259 insertions(+) > > diff --git a/sound/soc/meson/t9015.c b/sound/soc/meson/t9015.c > index 48f6767bd858..365955bfeb78 100644 > --- a/sound/soc/meson/t9015.c > +++ b/sound/soc/meson/t9015.c > @@ -19,16 +19,33 @@ > #define LOLP_EN 3 > #define DACR_EN 4 > #define DACL_EN 5 > +#define ADCR_EN 6 > +#define ADCL_EN 7 > +#define PGAR_ZCD_EN 8 > +#define PGAL_ZCD_EN 9 > +#define PGAR_EN 10 > +#define PGAL_EN 11 > +#define ADCR_INV 16 > +#define ADCL_INV 17 > +#define ADCR_SRC 18 > +#define ADCL_SRC 19 > #define DACR_INV 20 > #define DACL_INV 21 > #define DACR_SRC 22 > #define DACL_SRC 23 > +#define ADC_DEM_EN 26 > +#define ADC_FILTER_MODE 28 > +#define ADC_FILTER_EN 29 > #define REFP_BUF_EN BIT(12) > #define BIAS_CURRENT_EN BIT(13) > #define VMID_GEN_FAST BIT(14) > #define VMID_GEN_EN BIT(15) > #define I2S_MODE BIT(30) > #define VOL_CTRL0 0x04 > +#define PGAR_VC 0 > +#define PGAL_VC 8 > +#define ADCR_VC 16 > +#define ADCL_VC 24 > #define GAIN_H 31 > #define GAIN_L 23 > #define VOL_CTRL1 0x08 > @@ -46,6 +63,28 @@ > #define LOLN_POL 8 > #define LOLP_POL 12 > #define POWER_CFG 0x10 > +#define LINEIN_CFG 0x14 > +#define MICBIAS_LEVEL 0 > +#define MICBIAS_EN 3 > +#define PGAR_CTVMN 8 > +#define PGAR_CTVMP 9 > +#define PGAL_CTVMN 10 > +#define PGAL_CTVMP 11 > +#define PGAR_CTVIN 12 > +#define PGAR_CTVIP 13 > +#define PGAL_CTVIN 14 > +#define PGAL_CTVIP 15 > + > +#define PGAR_MASK (BIT(PGAR_CTVMP) | BIT(PGAR_CTVMN) | \ > + BIT(PGAR_CTVIP) | BIT(PGAR_CTVIN)) > +#define PGAR_DIFF (BIT(PGAR_CTVIP) | BIT(PGAR_CTVIN)) > +#define PGAR_POSITIVE (BIT(PGAR_CTVIP) | BIT(PGAR_CTVMN)) > +#define PGAR_NEGATIVE (BIT(PGAR_CTVIN) | BIT(PGAR_CTVMP)) > +#define PGAL_MASK (BIT(PGAL_CTVMP) | BIT(PGAL_CTVMN) | \ > + BIT(PGAL_CTVIP) | BIT(PGAL_CTVIN)) > +#define PGAL_DIFF (BIT(PGAL_CTVIP) | BIT(PGAL_CTVIN)) > +#define PGAL_POSITIVE (BIT(PGAL_CTVIP) | BIT(PGAL_CTVMN)) > +#define PGAL_NEGATIVE (BIT(PGAL_CTVIN) | BIT(PGAL_CTVMP)) > > struct t9015 { > struct regulator *avdd; > @@ -103,6 +142,31 @@ static struct snd_soc_dai_driver t9015_dai = { > .ops = &t9015_dai_ops, > }; > > +static struct snd_soc_dai_driver a1_t9015_dai = { > + .name = "t9015-hifi", > + .playback = { > + .stream_name = "Playback", > + .channels_min = 1, > + .channels_max = 2, > + .rates = SNDRV_PCM_RATE_8000_96000, > + .formats = (SNDRV_PCM_FMTBIT_S8 | > + SNDRV_PCM_FMTBIT_S16_LE | > + SNDRV_PCM_FMTBIT_S20_LE | > + SNDRV_PCM_FMTBIT_S24_LE), > + }, > + .capture = { > + .stream_name = "Capture", > + .channels_min = 1, > + .channels_max = 2, > + .rates = SNDRV_PCM_RATE_8000_96000, > + .formats = (SNDRV_PCM_FMTBIT_S8 | > + SNDRV_PCM_FMTBIT_S16_LE | > + SNDRV_PCM_FMTBIT_S20_LE | > + SNDRV_PCM_FMTBIT_S24_LE), > + }, > + .ops = &t9015_dai_ops, > +}; > + > static const DECLARE_TLV_DB_MINMAX_MUTE(dac_vol_tlv, -9525, 0); > > static const char * const ramp_rate_txt[] = { "Fast", "Slow" }; > @@ -179,6 +243,166 @@ static const struct snd_soc_dapm_route t9015_dapm_routes[] = { > { "LOLP", NULL, "Left+ Driver", }, > }; > > +static const char * const a1_right_driver_txt[] = { "None", "Right DAC", > + "Left DAC Inverted" }; > +static const unsigned int a1_right_driver_values[] = { 0, 2, 4 }; > + > +static const char * const a1_left_driver_txt[] = { "None", "Left DAC", > + "Right DAC Inverted" }; > +static const unsigned int a1_left_driver_values[] = { 0, 2, 4 }; > + > +static SOC_VALUE_ENUM_SINGLE_DECL(a1_right_driver, LINEOUT_CFG, 12, 0x7, > + a1_right_driver_txt, a1_right_driver_values); > +static SOC_VALUE_ENUM_SINGLE_DECL(a1_left_driver, LINEOUT_CFG, 4, 0x7, > + a1_left_driver_txt, a1_left_driver_values); > + > +static const struct snd_kcontrol_new a1_right_driver_mux = > + SOC_DAPM_ENUM("Right Driver+ Source", a1_right_driver); > +static const struct snd_kcontrol_new a1_left_driver_mux = > + SOC_DAPM_ENUM("Left Driver+ Source", a1_left_driver); > + > +static const DECLARE_TLV_DB_MINMAX_MUTE(a1_adc_vol_tlv, -29625, 0); > +static const DECLARE_TLV_DB_MINMAX_MUTE(a1_adc_pga_vol_tlv, -1200, 0); > + > +static const char * const a1_adc_right_txt[] = { "Right", "Left" }; > +static SOC_ENUM_SINGLE_DECL(a1_adc_right, BLOCK_EN, ADCR_SRC, a1_adc_right_txt); > + > +static const char * const a1_adc_left_txt[] = { "Left", "Right" }; > +static SOC_ENUM_SINGLE_DECL(a1_adc_left, BLOCK_EN, ADCL_SRC, a1_adc_left_txt); > + > +static const struct snd_kcontrol_new a1_adc_right_mux = > + SOC_DAPM_ENUM("ADC Right Source", a1_adc_right); > +static const struct snd_kcontrol_new a1_adc_left_mux = > + SOC_DAPM_ENUM("ADC Left Source", a1_adc_left); > + > +static const char * const a1_adc_filter_mode_txt[] = { "Voice", "HiFi"}; > +static SOC_ENUM_SINGLE_DECL(a1_adc_filter_mode, BLOCK_EN, ADC_FILTER_MODE, > + a1_adc_filter_mode_txt); > + > +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 }; > +static SOC_VALUE_ENUM_SINGLE_DECL(a1_adc_mic_bias_level, > + LINEIN_CFG, MICBIAS_LEVEL, 0x7, > + a1_adc_mic_bias_level_txt, > + a1_adc_mic_bias_level_values); > + > +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 }; > + > +static SOC_VALUE_ENUM_SINGLE_DECL(a1_adc_pga_right, LINEIN_CFG, 0, PGAR_MASK, > + a1_adc_pga_txt, a1_adc_pga_right_values); > +static SOC_VALUE_ENUM_SINGLE_DECL(a1_adc_pga_left, LINEIN_CFG, 0, PGAL_MASK, > + a1_adc_pga_txt, a1_adc_pga_left_values); > + > +static const struct snd_kcontrol_new a1_adc_pga_right_mux = > + SOC_DAPM_ENUM("ADC PGA Right Source", a1_adc_pga_right); > +static const struct snd_kcontrol_new a1_adc_pga_left_mux = > + SOC_DAPM_ENUM("ADC PGA Left Source", a1_adc_pga_left); > + > +static const struct snd_kcontrol_new a1_t9015_snd_controls[] = { > + /* Volume Controls */ > + SOC_ENUM("Playback Channel Mode", mono_enum), > + SOC_SINGLE("Playback Switch", VOL_CTRL1, DAC_SOFT_MUTE, 1, 1), > + SOC_DOUBLE_TLV("Playback Volume", VOL_CTRL1, DACL_VC, DACR_VC, > + 0xff, 0, dac_vol_tlv), > + > + /* Ramp Controls */ > + SOC_ENUM("Ramp Rate", ramp_rate_enum), > + SOC_SINGLE("Volume Ramp Switch", VOL_CTRL1, VC_RAMP_MODE, 1, 0), > + SOC_SINGLE("Mute Ramp Switch", VOL_CTRL1, MUTE_MODE, 1, 0), > + SOC_SINGLE("Unmute Ramp Switch", VOL_CTRL1, UNMUTE_MODE, 1, 0), > + > + /* ADC Controls */ > + SOC_DOUBLE_TLV("ADC Volume", VOL_CTRL0, ADCL_VC, ADCR_VC, > + 0x7f, 0, a1_adc_vol_tlv), > + SOC_SINGLE("ADC Filter Switch", BLOCK_EN, ADC_FILTER_EN, 1, 0), > + SOC_ENUM("ADC Filter Mode", a1_adc_filter_mode), > + SOC_SINGLE("ADC Mic Bias Switch", LINEIN_CFG, MICBIAS_EN, 1, 0), > + SOC_ENUM("ADC Mic Bias Level", a1_adc_mic_bias_level), > + SOC_SINGLE("ADC DEM Switch", BLOCK_EN, ADC_DEM_EN, 1, 0), > + SOC_DOUBLE_TLV("ADC PGA Volume", VOL_CTRL0, PGAR_VC, PGAL_VC, > + 0x1f, 0, a1_adc_pga_vol_tlv), > + SOC_DOUBLE("ADC PGA Zero Cross-detection Switch", BLOCK_EN, > + PGAL_ZCD_EN, PGAR_ZCD_EN, 1, 0), > +}; > + > +static const struct snd_soc_dapm_widget a1_t9015_dapm_widgets[] = { > + SND_SOC_DAPM_AIF_IN("Right IN", NULL, 0, SND_SOC_NOPM, 0, 0), > + SND_SOC_DAPM_AIF_IN("Left IN", NULL, 0, SND_SOC_NOPM, 0, 0), > + SND_SOC_DAPM_MUX("Right DAC Sel", SND_SOC_NOPM, 0, 0, > + &t9015_right_dac_mux), > + SND_SOC_DAPM_MUX("Left DAC Sel", SND_SOC_NOPM, 0, 0, > + &t9015_left_dac_mux), > + SND_SOC_DAPM_DAC("Right DAC", NULL, BLOCK_EN, DACR_EN, 0), > + SND_SOC_DAPM_DAC("Left DAC", NULL, BLOCK_EN, DACL_EN, 0), > + SND_SOC_DAPM_MUX("Right+ Driver Sel", SND_SOC_NOPM, 0, 0, > + &a1_right_driver_mux), > + SND_SOC_DAPM_MUX("Left+ Driver Sel", SND_SOC_NOPM, 0, 0, > + &a1_left_driver_mux), > + SND_SOC_DAPM_OUT_DRV("Right+ Driver", BLOCK_EN, LORP_EN, 0, NULL, 0), > + SND_SOC_DAPM_OUT_DRV("Left+ Driver", BLOCK_EN, LOLP_EN, 0, NULL, 0), > + SND_SOC_DAPM_OUTPUT("LORP"), > + SND_SOC_DAPM_OUTPUT("LOLP"), > + > + SND_SOC_DAPM_INPUT("ADC IN Right"), > + SND_SOC_DAPM_INPUT("ADC IN Left"), > + SND_SOC_DAPM_MUX("ADC PGA Right Sel", SND_SOC_NOPM, 0, 0, > + &a1_adc_pga_right_mux), > + SND_SOC_DAPM_MUX("ADC PGA Left Sel", SND_SOC_NOPM, 0, 0, > + &a1_adc_pga_left_mux), > + SND_SOC_DAPM_PGA("ADC PGA Right", BLOCK_EN, PGAR_EN, 0, NULL, 0), > + SND_SOC_DAPM_PGA("ADC PGA Left", BLOCK_EN, PGAL_EN, 0, NULL, 0), > + SND_SOC_DAPM_ADC("ADC Right", NULL, BLOCK_EN, ADCR_EN, 0), > + SND_SOC_DAPM_ADC("ADC Left", NULL, BLOCK_EN, ADCL_EN, 0), > + SND_SOC_DAPM_MUX("ADC Right Sel", SND_SOC_NOPM, 0, 0, &a1_adc_right_mux), > + SND_SOC_DAPM_MUX("ADC Left Sel", SND_SOC_NOPM, 0, 0, &a1_adc_left_mux), > + SND_SOC_DAPM_AIF_OUT("ADC OUT Right", NULL, 0, SND_SOC_NOPM, 0, 0), > + SND_SOC_DAPM_AIF_OUT("ADC OUT Left", NULL, 0, SND_SOC_NOPM, 0, 0), > +}; > + > +static const struct snd_soc_dapm_route a1_t9015_dapm_routes[] = { > + { "Right IN", NULL, "Playback" }, > + { "Left IN", NULL, "Playback" }, > + { "Right DAC Sel", "Right", "Right IN" }, > + { "Right DAC Sel", "Left", "Left IN" }, > + { "Left DAC Sel", "Right", "Right IN" }, > + { "Left DAC Sel", "Left", "Left IN" }, > + { "Right DAC", NULL, "Right DAC Sel" }, > + { "Left DAC", NULL, "Left DAC Sel" }, > + { "Right+ Driver Sel", "Right DAC", "Right DAC" }, > + { "Right+ Driver Sel", "Left DAC Inverted", "Right DAC" }, > + { "Left+ Driver Sel", "Left DAC", "Left DAC" }, > + { "Left+ Driver Sel", "Right DAC Inverted", "Left DAC" }, > + { "Right+ Driver", NULL, "Right+ Driver Sel" }, > + { "Left+ Driver", NULL, "Left+ Driver Sel" }, > + { "LORP", NULL, "Right+ Driver", }, > + { "LOLP", NULL, "Left+ Driver", }, > + > + { "ADC PGA Right Sel", "Differential", "ADC IN Right" }, > + { "ADC PGA Right Sel", "Positive", "ADC IN Right" }, > + { "ADC PGA Right Sel", "Negative", "ADC IN Right" }, > + { "ADC PGA Left Sel", "Differential", "ADC IN Left" }, > + { "ADC PGA Left Sel", "Positive", "ADC IN Left" }, > + { "ADC PGA Left Sel", "Negative", "ADC IN Left" }, > + { "ADC PGA Right", NULL, "ADC PGA Right Sel" }, > + { "ADC PGA Left", NULL, "ADC PGA Left Sel" }, > + { "ADC Right", NULL, "ADC PGA Right" }, > + { "ADC Left", NULL, "ADC PGA Left" }, > + { "ADC Right Sel", "Right", "ADC Right" }, > + { "ADC Right Sel", "Left", "ADC Left" }, > + { "ADC Left Sel", "Right", "ADC Right" }, > + { "ADC Left Sel", "Left", "ADC Left" }, > + { "ADC OUT Right", NULL, "ADC Right Sel" }, > + { "ADC OUT Left", NULL, "ADC Left Sel" }, > + { "Capture", NULL, "ADC OUT Right" }, > + { "Capture", NULL, "ADC OUT Left" }, > +}; > + > static int t9015_set_bias_level(struct snd_soc_component *component, > enum snd_soc_bias_level level) > { > @@ -241,6 +465,18 @@ static int t9015_component_probe(struct snd_soc_component *component) > return 0; > } > > +static int a1_t9015_component_probe(struct snd_soc_component *component) > +{ > + /* > + * This configuration was stealed from original Amlogic's driver to > + * reproduce the behavior of the driver more accurately. However, it is > + * not known for certain what it actually affects. > + */ > + snd_soc_component_write(component, POWER_CFG, 0x00010000); > + > + return 0; > +} > + > static const struct snd_soc_component_driver t9015_codec_driver = { > .probe = t9015_component_probe, > .set_bias_level = t9015_set_bias_level, > @@ -254,6 +490,19 @@ static const struct snd_soc_component_driver t9015_codec_driver = { > .endianness = 1, > }; > > +static const struct snd_soc_component_driver a1_t9015_codec_driver = { > + .probe = a1_t9015_component_probe, > + .set_bias_level = t9015_set_bias_level, > + .controls = a1_t9015_snd_controls, > + .num_controls = ARRAY_SIZE(a1_t9015_snd_controls), > + .dapm_widgets = a1_t9015_dapm_widgets, > + .num_dapm_widgets = ARRAY_SIZE(a1_t9015_dapm_widgets), > + .dapm_routes = a1_t9015_dapm_routes, > + .num_dapm_routes = ARRAY_SIZE(a1_t9015_dapm_routes), > + .suspend_bias_off = 1, > + .endianness = 1, > +}; > + > static int t9015_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -315,11 +564,21 @@ static const struct t9015_match_data t9015_match_data = { > .max_register = POWER_CFG, > }; > > +static const struct t9015_match_data a1_t9015_match_data = { > + .component_drv = &a1_t9015_codec_driver, > + .dai_drv = &a1_t9015_dai, > + .max_register = LINEIN_CFG, > +}; > + > static const struct of_device_id t9015_ids[] __maybe_unused = { > { > .compatible = "amlogic,t9015", > .data = &t9015_match_data, > }, > + { > + .compatible = "amlogic,t9015-a1", > + .data = &a1_t9015_match_data, > + }, > { } > }; > MODULE_DEVICE_TABLE(of, t9015_ids); -- Jerome
Summon Hans and Sakari for the ideas and for heads up (MIPI case can be affected as well). On Fri, Mar 15, 2024 at 12:39:03AM +0000, Hamish Martin wrote: > On Thu, 2024-03-14 at 15:18 +0200, andriy.shevchenko@linux.intel.com > wrote: > > On Thu, Mar 14, 2024 at 01:13:31AM +0000, Hamish Martin wrote: > > > On Wed, 2024-03-13 at 13:26 +0200, Andy Shevchenko wrote: ... > > > Removing the setting of the handle to invalid may be the right fix > > > but > > > I don't feel I know the code well enough to make a decision on > > > that. It > > > certainly doesn't resolve things immediately - I saw ref counting > > > warnings output. > > > > Not removing, but moving to the better place? > > Can you share warnings, though? > > > For context here is the current call chain that leads to > acpi_gpiochip_remove(): > > acpi_gpiochip_remove+0x32/0x1a0 > gpiochip_remove+0x39/0x140 > devres_release_group+0xe6/0x160 > i2c_device_remove+0x2d/0x80 > device_release_driver_internal+0x19a/0x200 > bus_remove_device+0xbf/0x100 > device_del+0x157/0x490 > ? __pfx_device_match_fwnode+0x10/0x10 > ? srso_return_thunk+0x5/0x5f > device_unregister+0xd/0x30 > i2c_acpi_notify+0x10e/0x160 > notifier_call_chain+0x58/0xd0 > blocking_notifier_call_chain+0x3a/0x60 > acpi_device_del_work_fn+0x85/0x1d0 > process_one_work+0x134/0x2f0 > worker_thread+0x2f0/0x410 > ? __pfx_worker_thread+0x10/0x10 > kthread+0xe3/0x110 > ? __pfx_kthread+0x10/0x10 > ret_from_fork+0x2f/0x50 > ? __pfx_kthread+0x10/0x10 > ret_from_fork_asm+0x1b/0x30 Right, and with invalid handle it can't process further. And note, that _pointer_ is valid, the content become unavailable. > I removed the setting of adev->handle = INVALID_ACPI_HANDLE from > acpi_scan_drop_device() and shifted it to just after the call to > blocking_notifier_call_chain() in acpi_device_del_work_fn(). > With that it seems things progress further with the call to > acpi_get_data() in acpi_gpiochip_remove() succeeding now. However, > later in acpi_gpiochip_free_regions() we hit this error: > > pca953x i2c-PRP0001:03: Failed to remove GPIO OpRegion handler > > We also get these errors: > ACPI Warning: Obj 00000000ba6a9600, Reference Count is already zero, > cannot decrement > (20230628/utdelete-422) Right, because of ACPICA (not even ACPI glue layer!) the callbacks are called when namespace node (which is acpi_handle) is being removed. I spend a few hours to understand the history of the invalidation of the handle. TBH, it looks like a hack to me, but its presence seems necessary to avoid racing with the hotplug work. It's a lot of functions that run asynchronously there and the validness of some objects is questionable. Here are the commits in question: d783156ea384 ("ACPI / scan: Define non-empty device removal handler") c27b2c33b621 ("ACPI / hotplug: Introduce common hotplug function acpi_device_hotplug()") (It doesn't mean they are bad, it means that this requires more investigation.) That said, from my perspective what should be done/clarified - the scope of ACPI data (Can we use it outside of ACPICA/ACPI glue layer or not?) - clarify in the documentation the scope / life time of the ACPI objects from the ACPICA perspective (Is it already done? Where?) - remove that invalidation hack and find better solution for the race avoidance > > P.S. > > I'm not an expert in ACPICA and low lever of ACPI glue layer in the Linux > > kernel, perhaps Rafael can suggest something better. > > > OK, thanks for your help. Hopefully Rafael can add something to the > discssion. Added more people to harvest the ideas. -- With Best Regards, Andy Shevchenko
On Sun 17 Mar 2024 at 18:19, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:
> On 3/15/24 16:33, Mark Brown wrote:
>> On Fri, Mar 15, 2024 at 02:21:45AM +0300, Jan Dakinevich wrote:
>>
>>> static const struct regmap_config g12a_toacodec_regmap_cfg = {
>>> - .reg_bits = 32,
>>> - .val_bits = 32,
>>> - .reg_stride = 4,
>>> + .reg_bits = 32,
>>> + .val_bits = 32,
>>> + .reg_stride = 4,
>>> + .max_register = TOACODEC_CTRL0,
>>> + .max_register_is_0 = true,
>>
>> If the maximum register is 0 how does the regmap have a stride?
>
> reg_stride inherited from existing code. Apparently, it was meaningless
> even before my modifications (the hardware has single register
> regardless of max_register declaration) and it should be dropped. But,
> is it okay to remove it in the same commit?
Yes it has a single register, for now. Still the stride is 4.
And assuming the mmio region passed from DT is correct, I'm not sure the
hunk is useful at all.
--
Jerome
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), >>> }, >> >> -- Jerome