[v1,0/4] clk: qcom: Support for Low Power Audio Clocks on SC7180

[v1]
 * Add support for Retention of GDSCR.
 * Add YAML schema for LPASS clocks and clock IDs for LPASS.
 * Add clock driver for LPASS core clocks and GCC LPASS clock.

Taniya Das (4):
  clk: qcom: gdsc: Add support to enable retention of GSDCR
  dt-bindings: clock: Add YAML schemas for LPASS clocks on SC7180
  clk: qcom: gcc: Add support for GCC LPASS clock for SC7180
  clk: qcom: lpass: Add support for LPASS clock controller for SC7180

 .../bindings/clock/qcom,sc7180-lpasscorecc.yaml    |  81 ++++
 drivers/clk/qcom/Kconfig                           |   9 +
 drivers/clk/qcom/Makefile                          |   1 +
 drivers/clk/qcom/gcc-sc7180.c                      |  14 +
 drivers/clk/qcom/gdsc.c                            |  12 +
 drivers/clk/qcom/gdsc.h                            |   1 +
 drivers/clk/qcom/lpasscorecc-sc7180.c              | 479 +++++++++++++++++++++
 include/dt-bindings/clock/qcom,gcc-sc7180.h        |   1 +
 .../dt-bindings/clock/qcom,lpasscorecc-sc7180.h    |  28 ++
 9 files changed, 626 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,sc7180-lpasscorecc.yaml
 create mode 100644 drivers/clk/qcom/lpasscorecc-sc7180.c
 create mode 100644 include/dt-bindings/clock/qcom,lpasscorecc-sc7180.h

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Quoting Taniya Das (2020-03-27 12:48:02)
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index a250f59..cfe908f 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -28,6 +28,7 @@
>  /* CFG_GDSCR */
>  #define GDSC_POWER_UP_COMPLETE         BIT(16)
>  #define GDSC_POWER_DOWN_COMPLETE       BIT(15)
> +#define GDSC_RETAIN_FF_ENABLE          BIT(11)
>  #define CFG_GDSCR_OFFSET               0x4
> 
>  /* Wait 2^n CXO cycles between all states. Here, n=2 (4 cycles). */
> @@ -202,6 +203,14 @@ static inline void gdsc_assert_reset_aon(struct gdsc *sc)
>         regmap_update_bits(sc->regmap, sc->clamp_io_ctrl,
>                            GMEM_RESET_MASK, 0);
>  }
> +
> +static inline void gdsc_retain_ff_on(struct gdsc *sc)

Drop inline please.

> +{
> +       u32 mask = RETAIN_FF_ENABLE;

Is this supposed to be GDSC_RETAIN_FF_ENABLE?

> +
> +       regmap_update_bits(sc->regmap, sc->gdscr, mask, mask);
> +}
> +
>  static int gdsc_enable(struct generic_pm_domain *domain)
>  {
>         struct gdsc *sc = domain_to_gdsc(domain);
> @@ -254,6 +263,9 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>                 udelay(1);
>         }
> 
> +       if (sc->flags & RETAIN_FF_ENABLE)
> +               gdsc_retain_ff_on(sc);
> +
>         return 0;
>  }
> 
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index 64cdc8c..8604d44 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -49,6 +49,7 @@ struct gdsc {
>  #define AON_RESET      BIT(4)
>  #define POLL_CFG_GDSCR BIT(5)
>  #define ALWAYS_ON      BIT(6)
> +#define RETAIN_FF_ENABLE       BIT(7)

This is a flag, not a register bit presumably.

>         struct reset_controller_dev     *rcdev;
>         unsigned int                    *resets;
>         unsigned int                    reset_count;

Quoting Taniya Das (2020-03-27 12:48:04)
> @@ -2406,6 +2419,7 @@ static struct clk_regmap *gcc_sc7180_clocks[] = {
>         [GCC_MSS_NAV_AXI_CLK] = &gcc_mss_nav_axi_clk.clkr,
>         [GCC_MSS_Q6_MEMNOC_AXI_CLK] = &gcc_mss_q6_memnoc_axi_clk.clkr,
>         [GCC_MSS_SNOC_AXI_CLK] = &gcc_mss_snoc_axi_clk.clkr,
> +       [GCC_LPASS_CFG_NOC_SWAY_CLK] = &gcc_lpass_cfg_noc_sway_clk.clkr,

Sad to see that more defines are being added slowly to the header file.
It would be better to have all the defines there so the binding header
file isn't updated in small updates over months and months.

>  };
>

Hello Stephen,

Thanks for the review.

On 4/10/2020 1:36 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2020-03-27 12:48:02)
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index a250f59..cfe908f 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -28,6 +28,7 @@
>>   /* CFG_GDSCR */
>>   #define GDSC_POWER_UP_COMPLETE         BIT(16)
>>   #define GDSC_POWER_DOWN_COMPLETE       BIT(15)
>> +#define GDSC_RETAIN_FF_ENABLE          BIT(11)
>>   #define CFG_GDSCR_OFFSET               0x4
>>
>>   /* Wait 2^n CXO cycles between all states. Here, n=2 (4 cycles). */
>> @@ -202,6 +203,14 @@ static inline void gdsc_assert_reset_aon(struct gdsc *sc)
>>          regmap_update_bits(sc->regmap, sc->clamp_io_ctrl,
>>                             GMEM_RESET_MASK, 0);
>>   }
>> +
>> +static inline void gdsc_retain_ff_on(struct gdsc *sc)
> 
> Drop inline please.
> 

Will drop in the next patch.

>> +{
>> +       u32 mask = RETAIN_FF_ENABLE;
> 
> Is this supposed to be GDSC_RETAIN_FF_ENABLE?
> 

Will update in next patch.

>> +
>> +       regmap_update_bits(sc->regmap, sc->gdscr, mask, mask);
>> +}
>> +
>>   static int gdsc_enable(struct generic_pm_domain *domain)
>>   {
>>          struct gdsc *sc = domain_to_gdsc(domain);
>> @@ -254,6 +263,9 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>                  udelay(1);
>>          }
>>
>> +       if (sc->flags & RETAIN_FF_ENABLE)
>> +               gdsc_retain_ff_on(sc);
>> +
>>          return 0;
>>   }
>>
>> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
>> index 64cdc8c..8604d44 100644
>> --- a/drivers/clk/qcom/gdsc.h
>> +++ b/drivers/clk/qcom/gdsc.h
>> @@ -49,6 +49,7 @@ struct gdsc {
>>   #define AON_RESET      BIT(4)
>>   #define POLL_CFG_GDSCR BIT(5)
>>   #define ALWAYS_ON      BIT(6)
>> +#define RETAIN_FF_ENABLE       BIT(7)
> 
> This is a flag, not a register bit presumably.

Yes, it is a flag.

> 
>>          struct reset_controller_dev     *rcdev;
>>          unsigned int                    *resets;
>>          unsigned int                    reset_count;