diff mbox series

[3/6] soc: qcom: ocmem: make iface clock optional

Message ID 20230506-msm8226-ocmem-v1-3-3e24e2724f01@z3ntu.xyz
State New
Headers
Series Add MSM8226 OCMEM support plus some extra OCMEM driver fixes |

Commit Message

Luca Weiss May 7, 2023, 9:12 a.m. UTC 
  Some platforms such as msm8226 do not have an iface clk. Since clk_bulk
APIs don't offer to a way to treat some clocks as optional simply add
core_clk and iface_clk members to our drvdata.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 drivers/soc/qcom/ocmem.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

Comments

Konrad Dybcio May 8, 2023, 7:37 a.m. UTC | #1 
On 7.05.2023 11:12, Luca Weiss wrote:
> Some platforms such as msm8226 do not have an iface clk. Since clk_bulk
> APIs don't offer to a way to treat some clocks as optional simply add
> core_clk and iface_clk members to our drvdata.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
I think I don't see anything wrong in here!

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/soc/qcom/ocmem.c | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
> index a11a955a1327..6235065d3bc9 100644
> --- a/drivers/soc/qcom/ocmem.c
> +++ b/drivers/soc/qcom/ocmem.c
> @@ -54,6 +54,8 @@ struct ocmem {
>  	const struct ocmem_config *config;
>  	struct resource *memory;
>  	void __iomem *mmio;
> +	struct clk *core_clk;
> +	struct clk *iface_clk;
>  	unsigned int num_ports;
>  	unsigned int num_macros;
>  	bool interleaved;
> @@ -91,16 +93,6 @@ struct ocmem {
>  #define OCMEM_PSGSC_CTL_MACRO2_MODE(val)	FIELD_PREP(0x00000700, (val))
>  #define OCMEM_PSGSC_CTL_MACRO3_MODE(val)	FIELD_PREP(0x00007000, (val))
>  
> -#define OCMEM_CLK_CORE_IDX			0
> -static struct clk_bulk_data ocmem_clks[] = {
> -	{
> -		.id = "core",
> -	},
> -	{
> -		.id = "iface",
> -	},
> -};
> -
>  static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32 data)
>  {
>  	writel(data, ocmem->mmio + reg);
> @@ -316,9 +308,15 @@ static int ocmem_dev_probe(struct platform_device *pdev)
>  	ocmem->dev = dev;
>  	ocmem->config = device_get_match_data(dev);
>  
> -	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(ocmem_clks), ocmem_clks);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "Unable to get clocks\n");
> +	ocmem->core_clk = devm_clk_get(dev, "core");
> +	if (IS_ERR(ocmem->core_clk))
> +		return dev_err_probe(dev, PTR_ERR(ocmem->core_clk),
> +				     "Unable to get core clock\n");
> +
> +	ocmem->iface_clk = devm_clk_get_optional(dev, "iface");
> +	if (IS_ERR(ocmem->iface_clk))
> +		return dev_err_probe(dev, PTR_ERR(ocmem->iface_clk),
> +				     "Unable to get iface clock\n");
>  
>  	ocmem->mmio = devm_platform_ioremap_resource_byname(pdev, "ctrl");
>  	if (IS_ERR(ocmem->mmio))
> @@ -333,11 +331,15 @@ static int ocmem_dev_probe(struct platform_device *pdev)
>  	}
>  
>  	/* The core clock is synchronous with graphics */
> -	WARN_ON(clk_set_rate(ocmem_clks[OCMEM_CLK_CORE_IDX].clk, 1000) < 0);
> +	WARN_ON(clk_set_rate(ocmem->core_clk, 1000) < 0);
> +
> +	ret = clk_prepare_enable(ocmem->core_clk);
> +	if (ret)
> +		return dev_err_probe(ocmem->dev, ret, "Failed to enable core clock\n");
>  
> -	ret = clk_bulk_prepare_enable(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> +	ret = clk_prepare_enable(ocmem->iface_clk);
>  	if (ret)
> -		return dev_err_probe(ocmem->dev, ret, "Failed to enable clocks\n");
> +		return dev_err_probe(ocmem->dev, ret, "Failed to enable iface clock\n");
>  
>  	if (qcom_scm_restore_sec_cfg_available()) {
>  		dev_dbg(dev, "configuring scm\n");
> @@ -396,13 +398,17 @@ static int ocmem_dev_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_clk_disable:
> -	clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> +	clk_disable_unprepare(ocmem->core_clk);
> +	clk_disable_unprepare(ocmem->iface_clk);
>  	return ret;
>  }
>  
>  static int ocmem_dev_remove(struct platform_device *pdev)
>  {
> -	clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> +	struct ocmem *ocmem = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(ocmem->core_clk);
> +	clk_disable_unprepare(ocmem->iface_clk);
>  
>  	return 0;
>  }
>
Dmitry Baryshkov May 8, 2023, 11:34 a.m. UTC | #2 
On 07/05/2023 12:12, Luca Weiss wrote:
> Some platforms such as msm8226 do not have an iface clk. Since clk_bulk
> APIs don't offer to a way to treat some clocks as optional simply add
> core_clk and iface_clk members to our drvdata.

What about using devm_clk_bulk_get_optional()? I think it would be 
simpler this way.

> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>   drivers/soc/qcom/ocmem.c | 42 ++++++++++++++++++++++++------------------
>   1 file changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
> index a11a955a1327..6235065d3bc9 100644
> --- a/drivers/soc/qcom/ocmem.c
> +++ b/drivers/soc/qcom/ocmem.c
> @@ -54,6 +54,8 @@ struct ocmem {
>   	const struct ocmem_config *config;
>   	struct resource *memory;
>   	void __iomem *mmio;
> +	struct clk *core_clk;
> +	struct clk *iface_clk;
>   	unsigned int num_ports;
>   	unsigned int num_macros;
>   	bool interleaved;
> @@ -91,16 +93,6 @@ struct ocmem {
>   #define OCMEM_PSGSC_CTL_MACRO2_MODE(val)	FIELD_PREP(0x00000700, (val))
>   #define OCMEM_PSGSC_CTL_MACRO3_MODE(val)	FIELD_PREP(0x00007000, (val))
>   
> -#define OCMEM_CLK_CORE_IDX			0
> -static struct clk_bulk_data ocmem_clks[] = {
> -	{
> -		.id = "core",
> -	},
> -	{
> -		.id = "iface",
> -	},
> -};
> -
>   static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32 data)
>   {
>   	writel(data, ocmem->mmio + reg);
> @@ -316,9 +308,15 @@ static int ocmem_dev_probe(struct platform_device *pdev)
>   	ocmem->dev = dev;
>   	ocmem->config = device_get_match_data(dev);
>   
> -	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(ocmem_clks), ocmem_clks);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "Unable to get clocks\n");
> +	ocmem->core_clk = devm_clk_get(dev, "core");
> +	if (IS_ERR(ocmem->core_clk))
> +		return dev_err_probe(dev, PTR_ERR(ocmem->core_clk),
> +				     "Unable to get core clock\n");
> +
> +	ocmem->iface_clk = devm_clk_get_optional(dev, "iface");
> +	if (IS_ERR(ocmem->iface_clk))
> +		return dev_err_probe(dev, PTR_ERR(ocmem->iface_clk),
> +				     "Unable to get iface clock\n");
>   
>   	ocmem->mmio = devm_platform_ioremap_resource_byname(pdev, "ctrl");
>   	if (IS_ERR(ocmem->mmio))
> @@ -333,11 +331,15 @@ static int ocmem_dev_probe(struct platform_device *pdev)
>   	}
>   
>   	/* The core clock is synchronous with graphics */
> -	WARN_ON(clk_set_rate(ocmem_clks[OCMEM_CLK_CORE_IDX].clk, 1000) < 0);
> +	WARN_ON(clk_set_rate(ocmem->core_clk, 1000) < 0);
> +
> +	ret = clk_prepare_enable(ocmem->core_clk);
> +	if (ret)
> +		return dev_err_probe(ocmem->dev, ret, "Failed to enable core clock\n");
>   
> -	ret = clk_bulk_prepare_enable(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> +	ret = clk_prepare_enable(ocmem->iface_clk);
>   	if (ret)
> -		return dev_err_probe(ocmem->dev, ret, "Failed to enable clocks\n");
> +		return dev_err_probe(ocmem->dev, ret, "Failed to enable iface clock\n");
>   
>   	if (qcom_scm_restore_sec_cfg_available()) {
>   		dev_dbg(dev, "configuring scm\n");
> @@ -396,13 +398,17 @@ static int ocmem_dev_probe(struct platform_device *pdev)
>   	return 0;
>   
>   err_clk_disable:
> -	clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> +	clk_disable_unprepare(ocmem->core_clk);
> +	clk_disable_unprepare(ocmem->iface_clk);
>   	return ret;
>   }
>   
>   static int ocmem_dev_remove(struct platform_device *pdev)
>   {
> -	clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> +	struct ocmem *ocmem = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(ocmem->core_clk);
> +	clk_disable_unprepare(ocmem->iface_clk);
>   
>   	return 0;
>   }
>
Luca Weiss May 9, 2023, 4:47 p.m. UTC | #3 
On Montag, 8. Mai 2023 13:34:23 CEST Dmitry Baryshkov wrote:
> On 07/05/2023 12:12, Luca Weiss wrote:
> > Some platforms such as msm8226 do not have an iface clk. Since clk_bulk
> > APIs don't offer to a way to treat some clocks as optional simply add
> > core_clk and iface_clk members to our drvdata.
> 
> What about using devm_clk_bulk_get_optional()? I think it would be
> simpler this way.

Using that function both clocks would be optional which may or may not be a 
bad idea. Not sure how much binding yaml and/or driver should try and catch 
bad usages of the driver.

But honestly the current usage of the bulk API seems a bit clunky, we have a 
static array of clocks that we use (not in struct ocmem for some reason) and 
then we refer to the core clock by index? Feels better to just have the two 
clock references in the device struct and then we're good.

Let me know.

Regards
Luca

> 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> > 
> >   drivers/soc/qcom/ocmem.c | 42 ++++++++++++++++++++++++------------------
> >   1 file changed, 24 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
> > index a11a955a1327..6235065d3bc9 100644
> > --- a/drivers/soc/qcom/ocmem.c
> > +++ b/drivers/soc/qcom/ocmem.c
> > @@ -54,6 +54,8 @@ struct ocmem {
> > 
> >   	const struct ocmem_config *config;
> >   	struct resource *memory;
> >   	void __iomem *mmio;
> > 
> > +	struct clk *core_clk;
> > +	struct clk *iface_clk;
> > 
> >   	unsigned int num_ports;
> >   	unsigned int num_macros;
> >   	bool interleaved;
> > 
> > @@ -91,16 +93,6 @@ struct ocmem {
> > 
> >   #define OCMEM_PSGSC_CTL_MACRO2_MODE(val)	FIELD_PREP(0x00000700, 
(val))
> >   #define OCMEM_PSGSC_CTL_MACRO3_MODE(val)	FIELD_PREP(0x00007000, 
(val))
> > 
> > -#define OCMEM_CLK_CORE_IDX			0
> > -static struct clk_bulk_data ocmem_clks[] = {
> > -	{
> > -		.id = "core",
> > -	},
> > -	{
> > -		.id = "iface",
> > -	},
> > -};
> > -
> > 
> >   static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32 data)
> >   {
> >   
> >   	writel(data, ocmem->mmio + reg);
> > 
> > @@ -316,9 +308,15 @@ static int ocmem_dev_probe(struct platform_device
> > *pdev)> 
> >   	ocmem->dev = dev;
> >   	ocmem->config = device_get_match_data(dev);
> > 
> > -	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > -	if (ret)
> > -		return dev_err_probe(dev, ret, "Unable to get clocks\n");
> > +	ocmem->core_clk = devm_clk_get(dev, "core");
> > +	if (IS_ERR(ocmem->core_clk))
> > +		return dev_err_probe(dev, PTR_ERR(ocmem->core_clk),
> > +				     "Unable to get core clock\n");
> > +
> > +	ocmem->iface_clk = devm_clk_get_optional(dev, "iface");
> > +	if (IS_ERR(ocmem->iface_clk))
> > +		return dev_err_probe(dev, PTR_ERR(ocmem->iface_clk),
> > +				     "Unable to get iface clock\n");
> > 
> >   	ocmem->mmio = devm_platform_ioremap_resource_byname(pdev, "ctrl");
> >   	if (IS_ERR(ocmem->mmio))
> > 
> > @@ -333,11 +331,15 @@ static int ocmem_dev_probe(struct platform_device
> > *pdev)> 
> >   	}
> >   	
> >   	/* The core clock is synchronous with graphics */
> > 
> > -	WARN_ON(clk_set_rate(ocmem_clks[OCMEM_CLK_CORE_IDX].clk, 1000) < 0);
> > +	WARN_ON(clk_set_rate(ocmem->core_clk, 1000) < 0);
> > +
> > +	ret = clk_prepare_enable(ocmem->core_clk);
> > +	if (ret)
> > +		return dev_err_probe(ocmem->dev, ret, "Failed to enable 
core clock\n");
> > 
> > -	ret = clk_bulk_prepare_enable(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > +	ret = clk_prepare_enable(ocmem->iface_clk);
> > 
> >   	if (ret)
> > 
> > -		return dev_err_probe(ocmem->dev, ret, "Failed to enable 
clocks\n");
> > +		return dev_err_probe(ocmem->dev, ret, "Failed to enable 
iface
> > clock\n");
> > 
> >   	if (qcom_scm_restore_sec_cfg_available()) {
> >   	
> >   		dev_dbg(dev, "configuring scm\n");
> > 
> > @@ -396,13 +398,17 @@ static int ocmem_dev_probe(struct platform_device
> > *pdev)> 
> >   	return 0;
> >   
> >   err_clk_disable:
> > -	clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > +	clk_disable_unprepare(ocmem->core_clk);
> > +	clk_disable_unprepare(ocmem->iface_clk);
> > 
> >   	return ret;
> >   
> >   }
> >   
> >   static int ocmem_dev_remove(struct platform_device *pdev)
> >   {
> > 
> > -	clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > +	struct ocmem *ocmem = platform_get_drvdata(pdev);
> > +
> > +	clk_disable_unprepare(ocmem->core_clk);
> > +	clk_disable_unprepare(ocmem->iface_clk);
> > 
> >   	return 0;
> >   
> >   }
Dmitry Baryshkov May 9, 2023, 5:08 p.m. UTC | #4 
On Tue, 9 May 2023 at 19:47, Luca Weiss <luca@z3ntu.xyz> wrote:
>
> On Montag, 8. Mai 2023 13:34:23 CEST Dmitry Baryshkov wrote:
> > On 07/05/2023 12:12, Luca Weiss wrote:
> > > Some platforms such as msm8226 do not have an iface clk. Since clk_bulk
> > > APIs don't offer to a way to treat some clocks as optional simply add
> > > core_clk and iface_clk members to our drvdata.
> >
> > What about using devm_clk_bulk_get_optional()? I think it would be
> > simpler this way.
>
> Using that function both clocks would be optional which may or may not be a
> bad idea. Not sure how much binding yaml and/or driver should try and catch
> bad usages of the driver.

The generic rule is that we should not validate the DT unless required
(e.g. because of the possibility of legacy DT which used other
bindings or contained less information).

> But honestly the current usage of the bulk API seems a bit clunky, we have a
> static array of clocks that we use (not in struct ocmem for some reason) and
> then we refer to the core clock by index? Feels better to just have the two
> clock references in the device struct and then we're good.
>
> Let me know.
>
> Regards
> Luca
>
> >
> > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > > ---
> > >
> > >   drivers/soc/qcom/ocmem.c | 42 ++++++++++++++++++++++++------------------
> > >   1 file changed, 24 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
> > > index a11a955a1327..6235065d3bc9 100644
> > > --- a/drivers/soc/qcom/ocmem.c
> > > +++ b/drivers/soc/qcom/ocmem.c
> > > @@ -54,6 +54,8 @@ struct ocmem {
> > >
> > >     const struct ocmem_config *config;
> > >     struct resource *memory;
> > >     void __iomem *mmio;
> > >
> > > +   struct clk *core_clk;
> > > +   struct clk *iface_clk;
> > >
> > >     unsigned int num_ports;
> > >     unsigned int num_macros;
> > >     bool interleaved;
> > >
> > > @@ -91,16 +93,6 @@ struct ocmem {
> > >
> > >   #define OCMEM_PSGSC_CTL_MACRO2_MODE(val)  FIELD_PREP(0x00000700,
> (val))
> > >   #define OCMEM_PSGSC_CTL_MACRO3_MODE(val)  FIELD_PREP(0x00007000,
> (val))
> > >
> > > -#define OCMEM_CLK_CORE_IDX                 0
> > > -static struct clk_bulk_data ocmem_clks[] = {
> > > -   {
> > > -           .id = "core",
> > > -   },
> > > -   {
> > > -           .id = "iface",
> > > -   },
> > > -};
> > > -
> > >
> > >   static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32 data)
> > >   {
> > >
> > >     writel(data, ocmem->mmio + reg);
> > >
> > > @@ -316,9 +308,15 @@ static int ocmem_dev_probe(struct platform_device
> > > *pdev)>
> > >     ocmem->dev = dev;
> > >     ocmem->config = device_get_match_data(dev);
> > >
> > > -   ret = devm_clk_bulk_get(dev, ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > > -   if (ret)
> > > -           return dev_err_probe(dev, ret, "Unable to get clocks\n");
> > > +   ocmem->core_clk = devm_clk_get(dev, "core");
> > > +   if (IS_ERR(ocmem->core_clk))
> > > +           return dev_err_probe(dev, PTR_ERR(ocmem->core_clk),
> > > +                                "Unable to get core clock\n");
> > > +
> > > +   ocmem->iface_clk = devm_clk_get_optional(dev, "iface");
> > > +   if (IS_ERR(ocmem->iface_clk))
> > > +           return dev_err_probe(dev, PTR_ERR(ocmem->iface_clk),
> > > +                                "Unable to get iface clock\n");
> > >
> > >     ocmem->mmio = devm_platform_ioremap_resource_byname(pdev, "ctrl");
> > >     if (IS_ERR(ocmem->mmio))
> > >
> > > @@ -333,11 +331,15 @@ static int ocmem_dev_probe(struct platform_device
> > > *pdev)>
> > >     }
> > >
> > >     /* The core clock is synchronous with graphics */
> > >
> > > -   WARN_ON(clk_set_rate(ocmem_clks[OCMEM_CLK_CORE_IDX].clk, 1000) < 0);
> > > +   WARN_ON(clk_set_rate(ocmem->core_clk, 1000) < 0);
> > > +
> > > +   ret = clk_prepare_enable(ocmem->core_clk);
> > > +   if (ret)
> > > +           return dev_err_probe(ocmem->dev, ret, "Failed to enable
> core clock\n");
> > >
> > > -   ret = clk_bulk_prepare_enable(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > > +   ret = clk_prepare_enable(ocmem->iface_clk);
> > >
> > >     if (ret)
> > >
> > > -           return dev_err_probe(ocmem->dev, ret, "Failed to enable
> clocks\n");
> > > +           return dev_err_probe(ocmem->dev, ret, "Failed to enable
> iface
> > > clock\n");
> > >
> > >     if (qcom_scm_restore_sec_cfg_available()) {
> > >
> > >             dev_dbg(dev, "configuring scm\n");
> > >
> > > @@ -396,13 +398,17 @@ static int ocmem_dev_probe(struct platform_device
> > > *pdev)>
> > >     return 0;
> > >
> > >   err_clk_disable:
> > > -   clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > > +   clk_disable_unprepare(ocmem->core_clk);
> > > +   clk_disable_unprepare(ocmem->iface_clk);
> > >
> > >     return ret;
> > >
> > >   }
> > >
> > >   static int ocmem_dev_remove(struct platform_device *pdev)
> > >   {
> > >
> > > -   clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > > +   struct ocmem *ocmem = platform_get_drvdata(pdev);
> > > +
> > > +   clk_disable_unprepare(ocmem->core_clk);
> > > +   clk_disable_unprepare(ocmem->iface_clk);
> > >
> > >     return 0;
> > >
> > >   }
>
>
>
>
Luca Weiss May 9, 2023, 9:41 p.m. UTC | #5 
On Dienstag, 9. Mai 2023 19:08:50 CEST Dmitry Baryshkov wrote:
> On Tue, 9 May 2023 at 19:47, Luca Weiss <luca@z3ntu.xyz> wrote:
> > On Montag, 8. Mai 2023 13:34:23 CEST Dmitry Baryshkov wrote:
> > > On 07/05/2023 12:12, Luca Weiss wrote:
> > > > Some platforms such as msm8226 do not have an iface clk. Since
> > > > clk_bulk
> > > > APIs don't offer to a way to treat some clocks as optional simply add
> > > > core_clk and iface_clk members to our drvdata.
> > > 
> > > What about using devm_clk_bulk_get_optional()? I think it would be
> > > simpler this way.
> > 
> > Using that function both clocks would be optional which may or may not be
> > a
> > bad idea. Not sure how much binding yaml and/or driver should try and
> > catch
> > bad usages of the driver.
> 
> The generic rule is that we should not validate the DT unless required
> (e.g. because of the possibility of legacy DT which used other
> bindings or contained less information).

Got it.

But since in this driver we use one of the clocks for setting clock rate I'd
keep using the two separate struct clk as I've done in this patch if you don't
mind too much.

Regards
Luca

> 
> > But honestly the current usage of the bulk API seems a bit clunky, we have
> > a static array of clocks that we use (not in struct ocmem for some
> > reason) and then we refer to the core clock by index? Feels better to
> > just have the two clock references in the device struct and then we're
> > good.
> > 
> > Let me know.
> > 
> > Regards
> > Luca
> > 
> > > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > > > ---
> > > > 
> > > >   drivers/soc/qcom/ocmem.c | 42
> > > >   ++++++++++++++++++++++++------------------
> > > >   1 file changed, 24 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
> > > > index a11a955a1327..6235065d3bc9 100644
> > > > --- a/drivers/soc/qcom/ocmem.c
> > > > +++ b/drivers/soc/qcom/ocmem.c
> > > > @@ -54,6 +54,8 @@ struct ocmem {
> > > > 
> > > >     const struct ocmem_config *config;
> > > >     struct resource *memory;
> > > >     void __iomem *mmio;
> > > > 
> > > > +   struct clk *core_clk;
> > > > +   struct clk *iface_clk;
> > > > 
> > > >     unsigned int num_ports;
> > > >     unsigned int num_macros;
> > > >     bool interleaved;
> > > > 
> > > > @@ -91,16 +93,6 @@ struct ocmem {
> > > > 
> > > >   #define OCMEM_PSGSC_CTL_MACRO2_MODE(val)  FIELD_PREP(0x00000700,
> > 
> > (val))
> > 
> > > >   #define OCMEM_PSGSC_CTL_MACRO3_MODE(val)  FIELD_PREP(0x00007000,
> > 
> > (val))
> > 
> > > > -#define OCMEM_CLK_CORE_IDX                 0
> > > > -static struct clk_bulk_data ocmem_clks[] = {
> > > > -   {
> > > > -           .id = "core",
> > > > -   },
> > > > -   {
> > > > -           .id = "iface",
> > > > -   },
> > > > -};
> > > > -
> > > > 
> > > >   static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32
> > > >   data)
> > > >   {
> > > >   
> > > >     writel(data, ocmem->mmio + reg);
> > > > 
> > > > @@ -316,9 +308,15 @@ static int ocmem_dev_probe(struct platform_device
> > > > *pdev)>
> > > > 
> > > >     ocmem->dev = dev;
> > > >     ocmem->config = device_get_match_data(dev);
> > > > 
> > > > -   ret = devm_clk_bulk_get(dev, ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > > > -   if (ret)
> > > > -           return dev_err_probe(dev, ret, "Unable to get clocks\n");
> > > > +   ocmem->core_clk = devm_clk_get(dev, "core");
> > > > +   if (IS_ERR(ocmem->core_clk))
> > > > +           return dev_err_probe(dev, PTR_ERR(ocmem->core_clk),
> > > > +                                "Unable to get core clock\n");
> > > > +
> > > > +   ocmem->iface_clk = devm_clk_get_optional(dev, "iface");
> > > > +   if (IS_ERR(ocmem->iface_clk))
> > > > +           return dev_err_probe(dev, PTR_ERR(ocmem->iface_clk),
> > > > +                                "Unable to get iface clock\n");
> > > > 
> > > >     ocmem->mmio = devm_platform_ioremap_resource_byname(pdev, "ctrl");
> > > >     if (IS_ERR(ocmem->mmio))
> > > > 
> > > > @@ -333,11 +331,15 @@ static int ocmem_dev_probe(struct
> > > > platform_device
> > > > *pdev)>
> > > > 
> > > >     }
> > > >     
> > > >     /* The core clock is synchronous with graphics */
> > > > 
> > > > -   WARN_ON(clk_set_rate(ocmem_clks[OCMEM_CLK_CORE_IDX].clk, 1000) <
> > > > 0);
> > > > +   WARN_ON(clk_set_rate(ocmem->core_clk, 1000) < 0);
> > > > +
> > > > +   ret = clk_prepare_enable(ocmem->core_clk);
> > > > +   if (ret)
> > > > +           return dev_err_probe(ocmem->dev, ret, "Failed to enable
> > 
> > core clock\n");
> > 
> > > > -   ret = clk_bulk_prepare_enable(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > > > +   ret = clk_prepare_enable(ocmem->iface_clk);
> > > > 
> > > >     if (ret)
> > > > 
> > > > -           return dev_err_probe(ocmem->dev, ret, "Failed to enable
> > 
> > clocks\n");
> > 
> > > > +           return dev_err_probe(ocmem->dev, ret, "Failed to enable
> > 
> > iface
> > 
> > > > clock\n");
> > > > 
> > > >     if (qcom_scm_restore_sec_cfg_available()) {
> > > >     
> > > >             dev_dbg(dev, "configuring scm\n");
> > > > 
> > > > @@ -396,13 +398,17 @@ static int ocmem_dev_probe(struct
> > > > platform_device
> > > > *pdev)>
> > > > 
> > > >     return 0;
> > > >   
> > > >   err_clk_disable:
> > > > -   clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > > > +   clk_disable_unprepare(ocmem->core_clk);
> > > > +   clk_disable_unprepare(ocmem->iface_clk);
> > > > 
> > > >     return ret;
> > > >   
> > > >   }
> > > >   
> > > >   static int ocmem_dev_remove(struct platform_device *pdev)
> > > >   {
> > > > 
> > > > -   clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
> > > > +   struct ocmem *ocmem = platform_get_drvdata(pdev);
> > > > +
> > > > +   clk_disable_unprepare(ocmem->core_clk);
> > > > +   clk_disable_unprepare(ocmem->iface_clk);
> > > > 
> > > >     return 0;
> > > >   
> > > >   }
Konrad Dybcio May 9, 2023, 10:32 p.m. UTC | #6 
On 9.05.2023 23:41, Luca Weiss wrote:
> On Dienstag, 9. Mai 2023 19:08:50 CEST Dmitry Baryshkov wrote:
>> On Tue, 9 May 2023 at 19:47, Luca Weiss <luca@z3ntu.xyz> wrote:
>>> On Montag, 8. Mai 2023 13:34:23 CEST Dmitry Baryshkov wrote:
>>>> On 07/05/2023 12:12, Luca Weiss wrote:
>>>>> Some platforms such as msm8226 do not have an iface clk. Since
>>>>> clk_bulk
>>>>> APIs don't offer to a way to treat some clocks as optional simply add
>>>>> core_clk and iface_clk members to our drvdata.
>>>>
>>>> What about using devm_clk_bulk_get_optional()? I think it would be
>>>> simpler this way.
>>>
>>> Using that function both clocks would be optional which may or may not be
>>> a
>>> bad idea. Not sure how much binding yaml and/or driver should try and
>>> catch
>>> bad usages of the driver.
>>
>> The generic rule is that we should not validate the DT unless required
>> (e.g. because of the possibility of legacy DT which used other
>> bindings or contained less information).
> 
> Got it.
> 
> But since in this driver we use one of the clocks for setting clock rate I'd
> keep using the two separate struct clk as I've done in this patch if you don't
> mind too much.
I'd also advocate for 2x struct clk, using bulk here is like trying to
spread butter with a fork, it works, but has holes..

Konrad
> 
> Regards
> Luca
> 
>>
>>> But honestly the current usage of the bulk API seems a bit clunky, we have
>>> a static array of clocks that we use (not in struct ocmem for some
>>> reason) and then we refer to the core clock by index? Feels better to
>>> just have the two clock references in the device struct and then we're
>>> good.
>>>
>>> Let me know.
>>>
>>> Regards
>>> Luca
>>>
>>>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>>>>> ---
>>>>>
>>>>>   drivers/soc/qcom/ocmem.c | 42
>>>>>   ++++++++++++++++++++++++------------------
>>>>>   1 file changed, 24 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
>>>>> index a11a955a1327..6235065d3bc9 100644
>>>>> --- a/drivers/soc/qcom/ocmem.c
>>>>> +++ b/drivers/soc/qcom/ocmem.c
>>>>> @@ -54,6 +54,8 @@ struct ocmem {
>>>>>
>>>>>     const struct ocmem_config *config;
>>>>>     struct resource *memory;
>>>>>     void __iomem *mmio;
>>>>>
>>>>> +   struct clk *core_clk;
>>>>> +   struct clk *iface_clk;
>>>>>
>>>>>     unsigned int num_ports;
>>>>>     unsigned int num_macros;
>>>>>     bool interleaved;
>>>>>
>>>>> @@ -91,16 +93,6 @@ struct ocmem {
>>>>>
>>>>>   #define OCMEM_PSGSC_CTL_MACRO2_MODE(val)  FIELD_PREP(0x00000700,
>>>
>>> (val))
>>>
>>>>>   #define OCMEM_PSGSC_CTL_MACRO3_MODE(val)  FIELD_PREP(0x00007000,
>>>
>>> (val))
>>>
>>>>> -#define OCMEM_CLK_CORE_IDX                 0
>>>>> -static struct clk_bulk_data ocmem_clks[] = {
>>>>> -   {
>>>>> -           .id = "core",
>>>>> -   },
>>>>> -   {
>>>>> -           .id = "iface",
>>>>> -   },
>>>>> -};
>>>>> -
>>>>>
>>>>>   static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32
>>>>>   data)
>>>>>   {
>>>>>   
>>>>>     writel(data, ocmem->mmio + reg);
>>>>>
>>>>> @@ -316,9 +308,15 @@ static int ocmem_dev_probe(struct platform_device
>>>>> *pdev)>
>>>>>
>>>>>     ocmem->dev = dev;
>>>>>     ocmem->config = device_get_match_data(dev);
>>>>>
>>>>> -   ret = devm_clk_bulk_get(dev, ARRAY_SIZE(ocmem_clks), ocmem_clks);
>>>>> -   if (ret)
>>>>> -           return dev_err_probe(dev, ret, "Unable to get clocks\n");
>>>>> +   ocmem->core_clk = devm_clk_get(dev, "core");
>>>>> +   if (IS_ERR(ocmem->core_clk))
>>>>> +           return dev_err_probe(dev, PTR_ERR(ocmem->core_clk),
>>>>> +                                "Unable to get core clock\n");
>>>>> +
>>>>> +   ocmem->iface_clk = devm_clk_get_optional(dev, "iface");
>>>>> +   if (IS_ERR(ocmem->iface_clk))
>>>>> +           return dev_err_probe(dev, PTR_ERR(ocmem->iface_clk),
>>>>> +                                "Unable to get iface clock\n");
>>>>>
>>>>>     ocmem->mmio = devm_platform_ioremap_resource_byname(pdev, "ctrl");
>>>>>     if (IS_ERR(ocmem->mmio))
>>>>>
>>>>> @@ -333,11 +331,15 @@ static int ocmem_dev_probe(struct
>>>>> platform_device
>>>>> *pdev)>
>>>>>
>>>>>     }
>>>>>     
>>>>>     /* The core clock is synchronous with graphics */
>>>>>
>>>>> -   WARN_ON(clk_set_rate(ocmem_clks[OCMEM_CLK_CORE_IDX].clk, 1000) <
>>>>> 0);
>>>>> +   WARN_ON(clk_set_rate(ocmem->core_clk, 1000) < 0);
>>>>> +
>>>>> +   ret = clk_prepare_enable(ocmem->core_clk);
>>>>> +   if (ret)
>>>>> +           return dev_err_probe(ocmem->dev, ret, "Failed to enable
>>>
>>> core clock\n");
>>>
>>>>> -   ret = clk_bulk_prepare_enable(ARRAY_SIZE(ocmem_clks), ocmem_clks);
>>>>> +   ret = clk_prepare_enable(ocmem->iface_clk);
>>>>>
>>>>>     if (ret)
>>>>>
>>>>> -           return dev_err_probe(ocmem->dev, ret, "Failed to enable
>>>
>>> clocks\n");
>>>
>>>>> +           return dev_err_probe(ocmem->dev, ret, "Failed to enable
>>>
>>> iface
>>>
>>>>> clock\n");
>>>>>
>>>>>     if (qcom_scm_restore_sec_cfg_available()) {
>>>>>     
>>>>>             dev_dbg(dev, "configuring scm\n");
>>>>>
>>>>> @@ -396,13 +398,17 @@ static int ocmem_dev_probe(struct
>>>>> platform_device
>>>>> *pdev)>
>>>>>
>>>>>     return 0;
>>>>>   
>>>>>   err_clk_disable:
>>>>> -   clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
>>>>> +   clk_disable_unprepare(ocmem->core_clk);
>>>>> +   clk_disable_unprepare(ocmem->iface_clk);
>>>>>
>>>>>     return ret;
>>>>>   
>>>>>   }
>>>>>   
>>>>>   static int ocmem_dev_remove(struct platform_device *pdev)
>>>>>   {
>>>>>
>>>>> -   clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
>>>>> +   struct ocmem *ocmem = platform_get_drvdata(pdev);
>>>>> +
>>>>> +   clk_disable_unprepare(ocmem->core_clk);
>>>>> +   clk_disable_unprepare(ocmem->iface_clk);
>>>>>
>>>>>     return 0;
>>>>>   
>>>>>   }
> 
> 
> 
>
diff mbox series

Patch

diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
index a11a955a1327..6235065d3bc9 100644
--- a/drivers/soc/qcom/ocmem.c
+++ b/drivers/soc/qcom/ocmem.c
@@ -54,6 +54,8 @@  struct ocmem {
 	const struct ocmem_config *config;
 	struct resource *memory;
 	void __iomem *mmio;
+	struct clk *core_clk;
+	struct clk *iface_clk;
 	unsigned int num_ports;
 	unsigned int num_macros;
 	bool interleaved;
@@ -91,16 +93,6 @@  struct ocmem {
 #define OCMEM_PSGSC_CTL_MACRO2_MODE(val)	FIELD_PREP(0x00000700, (val))
 #define OCMEM_PSGSC_CTL_MACRO3_MODE(val)	FIELD_PREP(0x00007000, (val))
 
-#define OCMEM_CLK_CORE_IDX			0
-static struct clk_bulk_data ocmem_clks[] = {
-	{
-		.id = "core",
-	},
-	{
-		.id = "iface",
-	},
-};
-
 static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32 data)
 {
 	writel(data, ocmem->mmio + reg);
@@ -316,9 +308,15 @@  static int ocmem_dev_probe(struct platform_device *pdev)
 	ocmem->dev = dev;
 	ocmem->config = device_get_match_data(dev);
 
-	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(ocmem_clks), ocmem_clks);
-	if (ret)
-		return dev_err_probe(dev, ret, "Unable to get clocks\n");
+	ocmem->core_clk = devm_clk_get(dev, "core");
+	if (IS_ERR(ocmem->core_clk))
+		return dev_err_probe(dev, PTR_ERR(ocmem->core_clk),
+				     "Unable to get core clock\n");
+
+	ocmem->iface_clk = devm_clk_get_optional(dev, "iface");
+	if (IS_ERR(ocmem->iface_clk))
+		return dev_err_probe(dev, PTR_ERR(ocmem->iface_clk),
+				     "Unable to get iface clock\n");
 
 	ocmem->mmio = devm_platform_ioremap_resource_byname(pdev, "ctrl");
 	if (IS_ERR(ocmem->mmio))
@@ -333,11 +331,15 @@  static int ocmem_dev_probe(struct platform_device *pdev)
 	}
 
 	/* The core clock is synchronous with graphics */
-	WARN_ON(clk_set_rate(ocmem_clks[OCMEM_CLK_CORE_IDX].clk, 1000) < 0);
+	WARN_ON(clk_set_rate(ocmem->core_clk, 1000) < 0);
+
+	ret = clk_prepare_enable(ocmem->core_clk);
+	if (ret)
+		return dev_err_probe(ocmem->dev, ret, "Failed to enable core clock\n");
 
-	ret = clk_bulk_prepare_enable(ARRAY_SIZE(ocmem_clks), ocmem_clks);
+	ret = clk_prepare_enable(ocmem->iface_clk);
 	if (ret)
-		return dev_err_probe(ocmem->dev, ret, "Failed to enable clocks\n");
+		return dev_err_probe(ocmem->dev, ret, "Failed to enable iface clock\n");
 
 	if (qcom_scm_restore_sec_cfg_available()) {
 		dev_dbg(dev, "configuring scm\n");
@@ -396,13 +398,17 @@  static int ocmem_dev_probe(struct platform_device *pdev)
 	return 0;
 
 err_clk_disable:
-	clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
+	clk_disable_unprepare(ocmem->core_clk);
+	clk_disable_unprepare(ocmem->iface_clk);
 	return ret;
 }
 
 static int ocmem_dev_remove(struct platform_device *pdev)
 {
-	clk_bulk_disable_unprepare(ARRAY_SIZE(ocmem_clks), ocmem_clks);
+	struct ocmem *ocmem = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(ocmem->core_clk);
+	clk_disable_unprepare(ocmem->iface_clk);
 
 	return 0;
 }