diff mbox series

[v6,1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver

Message ID 20210504105940.100004-2-greentime.hu@sifive.com (mailing list archive)
State Awaiting Upstream, archived
Headers show
Series Add SiFive FU740 PCIe host controller driver support | expand

Commit Message

Greentime Hu May 4, 2021, 10:59 a.m. UTC 
We add pcie_aux clock in this patch so that pcie driver can use
clk_prepare_enable() and clk_disable_unprepare() to enable and disable
pcie_aux clock.

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/sifive/fu740-prci.c               | 11 +++++
 drivers/clk/sifive/fu740-prci.h               |  2 +-
 drivers/clk/sifive/sifive-prci.c              | 41 +++++++++++++++++++
 drivers/clk/sifive/sifive-prci.h              |  9 ++++
 include/dt-bindings/clock/sifive-fu740-prci.h |  1 +
 5 files changed, 63 insertions(+), 1 deletion(-)

Comments

Leon Romanovsky May 4, 2021, 12:24 p.m. UTC | #1 
On Tue, May 04, 2021 at 06:59:35PM +0800, Greentime Hu wrote:
> We add pcie_aux clock in this patch so that pcie driver can use
> clk_prepare_enable() and clk_disable_unprepare() to enable and disable
> pcie_aux clock.
> 
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> Acked-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  drivers/clk/sifive/fu740-prci.c               | 11 +++++
>  drivers/clk/sifive/fu740-prci.h               |  2 +-
>  drivers/clk/sifive/sifive-prci.c              | 41 +++++++++++++++++++
>  drivers/clk/sifive/sifive-prci.h              |  9 ++++
>  include/dt-bindings/clock/sifive-fu740-prci.h |  1 +
>  5 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
> index 764d1097aa51..53f6e00a03b9 100644
> --- a/drivers/clk/sifive/fu740-prci.c
> +++ b/drivers/clk/sifive/fu740-prci.c
> @@ -72,6 +72,12 @@ static const struct clk_ops sifive_fu740_prci_hfpclkplldiv_clk_ops = {
>  	.recalc_rate = sifive_prci_hfpclkplldiv_recalc_rate,

<...>

> +/* PCIE AUX clock APIs for enable, disable. */
> +int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw)

It should be bool

> +{
> +	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +	struct __prci_data *pd = pc->pd;
> +	u32 r;
> +
> +	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);
> +
> +	if (r & PRCI_PCIE_AUX_EN_MASK)
> +		return 1;
> +	else
> +		return 0;
> +}

and here simple "return r & PRCI_PCIE_AUX_EN_MASK;"

> +
> +int sifive_prci_pcie_aux_clock_enable(struct clk_hw *hw)
> +{
> +	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +	struct __prci_data *pd = pc->pd;
> +	u32 r __maybe_unused;
> +
> +	if (sifive_prci_pcie_aux_clock_is_enabled(hw))
> +		return 0;

You actually call to this new function only once, put your
__prci_readl() here.

Thanks
Bjorn Helgaas May 4, 2021, 4:23 p.m. UTC | #2 
On Tue, May 04, 2021 at 03:24:19PM +0300, Leon Romanovsky wrote:
> On Tue, May 04, 2021 at 06:59:35PM +0800, Greentime Hu wrote:
> > We add pcie_aux clock in this patch so that pcie driver can use
> > clk_prepare_enable() and clk_disable_unprepare() to enable and disable
> > pcie_aux clock.
> > 
> > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > ---
> >  drivers/clk/sifive/fu740-prci.c               | 11 +++++
> >  drivers/clk/sifive/fu740-prci.h               |  2 +-
> >  drivers/clk/sifive/sifive-prci.c              | 41 +++++++++++++++++++
> >  drivers/clk/sifive/sifive-prci.h              |  9 ++++
> >  include/dt-bindings/clock/sifive-fu740-prci.h |  1 +
> >  5 files changed, 63 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
> > index 764d1097aa51..53f6e00a03b9 100644
> > --- a/drivers/clk/sifive/fu740-prci.c
> > +++ b/drivers/clk/sifive/fu740-prci.c
> > @@ -72,6 +72,12 @@ static const struct clk_ops sifive_fu740_prci_hfpclkplldiv_clk_ops = {
> >  	.recalc_rate = sifive_prci_hfpclkplldiv_recalc_rate,
> 
> <...>
> 
> > +/* PCIE AUX clock APIs for enable, disable. */
> > +int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw)
> 
> It should be bool

It's used via this function pointer:

  struct clk_ops {
    int             (*is_enabled)(struct clk_hw *hw);

so I think "int" is actually appropriate here.

There are some weird/interesting bool vs int usages nearby, though:

  "bool __is_clk_gate_enabled()" goes to some trouble to convert
  int to bool ("return (reg_val & bit_mask) != 0;"), and then
  kona_peri_clk_is_enabled() converts the bool back to int ("return
  is_clk_gate_enabled(bcm_clk->ccu, gate) ? 1 : 0;").

  "int lpc32xx_clk_gate_is_enabled()" actually returns a bool that is
  implicitly converted to int.

  Many *_is_enabled() functions return !!(...) where !! is an
  int-to-bool conversion that is arguably unnecessary and again
  results in an implicit conversion to int.

I don't see any *problems* with any of these; it just seems like a
little more mental effort to think about all the explicit and implicit
conversions going on.

> > +int sifive_prci_pcie_aux_clock_enable(struct clk_hw *hw)
> > +{
> > +	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> > +	struct __prci_data *pd = pc->pd;
> > +	u32 r __maybe_unused;
> > +
> > +	if (sifive_prci_pcie_aux_clock_is_enabled(hw))
> > +		return 0;
> 
> You actually call to this new function only once, put your
> __prci_readl() here.

Both sifive_prci_pcie_aux_clock_enable() and
sifive_prci_pcie_aux_clock_is_enabled() are used via the clk_ops
function pointers.

Maybe sifive_prci_pcie_aux_clock_is_enabled() could be replaced by the
__prci_readl() here, but I don't know enough about clk_ops internals
to know.

Bjorn
Leon Romanovsky May 4, 2021, 6:12 p.m. UTC | #3 
On Tue, May 04, 2021 at 11:23:31AM -0500, Bjorn Helgaas wrote:
> On Tue, May 04, 2021 at 03:24:19PM +0300, Leon Romanovsky wrote:
> > On Tue, May 04, 2021 at 06:59:35PM +0800, Greentime Hu wrote:
> > > We add pcie_aux clock in this patch so that pcie driver can use
> > > clk_prepare_enable() and clk_disable_unprepare() to enable and disable
> > > pcie_aux clock.
> > > 
> > > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > > ---
> > >  drivers/clk/sifive/fu740-prci.c               | 11 +++++
> > >  drivers/clk/sifive/fu740-prci.h               |  2 +-
> > >  drivers/clk/sifive/sifive-prci.c              | 41 +++++++++++++++++++
> > >  drivers/clk/sifive/sifive-prci.h              |  9 ++++
> > >  include/dt-bindings/clock/sifive-fu740-prci.h |  1 +
> > >  5 files changed, 63 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
> > > index 764d1097aa51..53f6e00a03b9 100644
> > > --- a/drivers/clk/sifive/fu740-prci.c
> > > +++ b/drivers/clk/sifive/fu740-prci.c
> > > @@ -72,6 +72,12 @@ static const struct clk_ops sifive_fu740_prci_hfpclkplldiv_clk_ops = {
> > >  	.recalc_rate = sifive_prci_hfpclkplldiv_recalc_rate,
> > 
> > <...>
> > 
> > > +/* PCIE AUX clock APIs for enable, disable. */
> > > +int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw)
> > 
> > It should be bool
> 
> It's used via this function pointer:
> 
>   struct clk_ops {
>     int             (*is_enabled)(struct clk_hw *hw);
> 
> so I think "int" is actually appropriate here.

Ahh, sorry, I missed that assignment.

> 
> There are some weird/interesting bool vs int usages nearby, though:
> 
>   "bool __is_clk_gate_enabled()" goes to some trouble to convert
>   int to bool ("return (reg_val & bit_mask) != 0;"), and then
>   kona_peri_clk_is_enabled() converts the bool back to int ("return
>   is_clk_gate_enabled(bcm_clk->ccu, gate) ? 1 : 0;").
> 
>   "int lpc32xx_clk_gate_is_enabled()" actually returns a bool that is
>   implicitly converted to int.
> 
>   Many *_is_enabled() functions return !!(...) where !! is an
>   int-to-bool conversion that is arguably unnecessary and again
>   results in an implicit conversion to int.
> 
> I don't see any *problems* with any of these; it just seems like a
> little more mental effort to think about all the explicit and implicit
> conversions going on.

The code is written once but read many times and I can't agree with
your that examples given by you are not the *problems*. They clearly
says "the API is not great and easily can be improved".

Driver authors struggled to write bool-to-int conversion, it is very
optimistic view that they won't struggle to read code too.

Thanks
Bjorn Helgaas May 4, 2021, 6:45 p.m. UTC | #4 
On Tue, May 04, 2021 at 09:12:57PM +0300, Leon Romanovsky wrote:
> On Tue, May 04, 2021 at 11:23:31AM -0500, Bjorn Helgaas wrote:

> > There are some weird/interesting bool vs int usages nearby, though:
> > 
> >   "bool __is_clk_gate_enabled()" goes to some trouble to convert
> >   int to bool ("return (reg_val & bit_mask) != 0;"), and then
> >   kona_peri_clk_is_enabled() converts the bool back to int ("return
> >   is_clk_gate_enabled(bcm_clk->ccu, gate) ? 1 : 0;").
> > 
> >   "int lpc32xx_clk_gate_is_enabled()" actually returns a bool that is
> >   implicitly converted to int.
> > 
> >   Many *_is_enabled() functions return !!(...) where !! is an
> >   int-to-bool conversion that is arguably unnecessary and again
> >   results in an implicit conversion to int.
> > 
> > I don't see any *problems* with any of these; it just seems like a
> > little more mental effort to think about all the explicit and implicit
> > conversions going on.
> 
> The code is written once but read many times and I can't agree with
> your that examples given by you are not the *problems*. They clearly
> says "the API is not great and easily can be improved".

I certainly agree that it's easier for readers if the style is
consistent.  I just meant I didn't see anything that's an actual bug.  

Bjorn
Leon Romanovsky May 5, 2021, 5:22 a.m. UTC | #5 
On Tue, May 04, 2021 at 01:45:55PM -0500, Bjorn Helgaas wrote:
> On Tue, May 04, 2021 at 09:12:57PM +0300, Leon Romanovsky wrote:
> > On Tue, May 04, 2021 at 11:23:31AM -0500, Bjorn Helgaas wrote:
> 
> > > There are some weird/interesting bool vs int usages nearby, though:
> > > 
> > >   "bool __is_clk_gate_enabled()" goes to some trouble to convert
> > >   int to bool ("return (reg_val & bit_mask) != 0;"), and then
> > >   kona_peri_clk_is_enabled() converts the bool back to int ("return
> > >   is_clk_gate_enabled(bcm_clk->ccu, gate) ? 1 : 0;").
> > > 
> > >   "int lpc32xx_clk_gate_is_enabled()" actually returns a bool that is
> > >   implicitly converted to int.
> > > 
> > >   Many *_is_enabled() functions return !!(...) where !! is an
> > >   int-to-bool conversion that is arguably unnecessary and again
> > >   results in an implicit conversion to int.
> > > 
> > > I don't see any *problems* with any of these; it just seems like a
> > > little more mental effort to think about all the explicit and implicit
> > > conversions going on.
> > 
> > The code is written once but read many times and I can't agree with
> > your that examples given by you are not the *problems*. They clearly
> > says "the API is not great and easily can be improved".
> 
> I certainly agree that it's easier for readers if the style is
> consistent.  I just meant I didn't see anything that's an actual bug.  

No one said that it is a bug. My comment is better seen as s suggestion
to the maintainers on how other subsystems keep their code base clean
and up-to date.

Once "the problem" is spotted, the submitter is asked to fix it globally
including fixing other drivers if needed, before "new feature" can be merged.

Of course, there are exceptions from this rule, but they are rare and
usually given to the people who has proven record.

Thanks

> 
> Bjorn
diff mbox series

Patch

diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
index 764d1097aa51..53f6e00a03b9 100644
--- a/drivers/clk/sifive/fu740-prci.c
+++ b/drivers/clk/sifive/fu740-prci.c
@@ -72,6 +72,12 @@  static const struct clk_ops sifive_fu740_prci_hfpclkplldiv_clk_ops = {
 	.recalc_rate = sifive_prci_hfpclkplldiv_recalc_rate,
 };
 
+static const struct clk_ops sifive_fu740_prci_pcie_aux_clk_ops = {
+	.enable = sifive_prci_pcie_aux_clock_enable,
+	.disable = sifive_prci_pcie_aux_clock_disable,
+	.is_enabled = sifive_prci_pcie_aux_clock_is_enabled,
+};
+
 /* List of clock controls provided by the PRCI */
 struct __prci_clock __prci_init_clocks_fu740[] = {
 	[PRCI_CLK_COREPLL] = {
@@ -120,4 +126,9 @@  struct __prci_clock __prci_init_clocks_fu740[] = {
 		.parent_name = "hfpclkpll",
 		.ops = &sifive_fu740_prci_hfpclkplldiv_clk_ops,
 	},
+	[PRCI_CLK_PCIE_AUX] = {
+		.name = "pcie_aux",
+		.parent_name = "hfclk",
+		.ops = &sifive_fu740_prci_pcie_aux_clk_ops,
+	},
 };
diff --git a/drivers/clk/sifive/fu740-prci.h b/drivers/clk/sifive/fu740-prci.h
index 13ef971f7764..511a0bf7ba2b 100644
--- a/drivers/clk/sifive/fu740-prci.h
+++ b/drivers/clk/sifive/fu740-prci.h
@@ -9,7 +9,7 @@ 
 
 #include "sifive-prci.h"
 
-#define NUM_CLOCK_FU740	8
+#define NUM_CLOCK_FU740	9
 
 extern struct __prci_clock __prci_init_clocks_fu740[NUM_CLOCK_FU740];
 
diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
index 1490b01ce629..9997a3fa4a38 100644
--- a/drivers/clk/sifive/sifive-prci.c
+++ b/drivers/clk/sifive/sifive-prci.c
@@ -453,6 +453,47 @@  void sifive_prci_hfpclkpllsel_use_hfpclkpll(struct __prci_data *pd)
 	r = __prci_readl(pd, PRCI_HFPCLKPLLSEL_OFFSET);	/* barrier */
 }
 
+/* PCIE AUX clock APIs for enable, disable. */
+int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_data *pd = pc->pd;
+	u32 r;
+
+	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);
+
+	if (r & PRCI_PCIE_AUX_EN_MASK)
+		return 1;
+	else
+		return 0;
+}
+
+int sifive_prci_pcie_aux_clock_enable(struct clk_hw *hw)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_data *pd = pc->pd;
+	u32 r __maybe_unused;
+
+	if (sifive_prci_pcie_aux_clock_is_enabled(hw))
+		return 0;
+
+	__prci_writel(1, PRCI_PCIE_AUX_OFFSET, pd);
+	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);	/* barrier */
+
+	return 0;
+}
+
+void sifive_prci_pcie_aux_clock_disable(struct clk_hw *hw)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_data *pd = pc->pd;
+	u32 r __maybe_unused;
+
+	__prci_writel(0, PRCI_PCIE_AUX_OFFSET, pd);
+	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);	/* barrier */
+
+}
+
 /**
  * __prci_register_clocks() - register clock controls in the PRCI
  * @dev: Linux struct device
diff --git a/drivers/clk/sifive/sifive-prci.h b/drivers/clk/sifive/sifive-prci.h
index dbdbd1722688..022c67cf053c 100644
--- a/drivers/clk/sifive/sifive-prci.h
+++ b/drivers/clk/sifive/sifive-prci.h
@@ -67,6 +67,11 @@ 
 #define PRCI_DDRPLLCFG1_CKE_SHIFT	31
 #define PRCI_DDRPLLCFG1_CKE_MASK	(0x1 << PRCI_DDRPLLCFG1_CKE_SHIFT)
 
+/* PCIEAUX */
+#define PRCI_PCIE_AUX_OFFSET		0x14
+#define PRCI_PCIE_AUX_EN_SHIFT		0
+#define PRCI_PCIE_AUX_EN_MASK		(0x1 << PRCI_PCIE_AUX_EN_SHIFT)
+
 /* GEMGXLPLLCFG0 */
 #define PRCI_GEMGXLPLLCFG0_OFFSET	0x1c
 #define PRCI_GEMGXLPLLCFG0_DIVR_SHIFT	0
@@ -296,4 +301,8 @@  unsigned long sifive_prci_tlclksel_recalc_rate(struct clk_hw *hw,
 unsigned long sifive_prci_hfpclkplldiv_recalc_rate(struct clk_hw *hw,
 						   unsigned long parent_rate);
 
+int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw);
+int sifive_prci_pcie_aux_clock_enable(struct clk_hw *hw);
+void sifive_prci_pcie_aux_clock_disable(struct clk_hw *hw);
+
 #endif /* __SIFIVE_CLK_SIFIVE_PRCI_H */
diff --git a/include/dt-bindings/clock/sifive-fu740-prci.h b/include/dt-bindings/clock/sifive-fu740-prci.h
index cd7706ea5677..7899b7fee7db 100644
--- a/include/dt-bindings/clock/sifive-fu740-prci.h
+++ b/include/dt-bindings/clock/sifive-fu740-prci.h
@@ -19,5 +19,6 @@ 
 #define PRCI_CLK_CLTXPLL	       5
 #define PRCI_CLK_TLCLK		       6
 #define PRCI_CLK_PCLK		       7
+#define PRCI_CLK_PCIE_AUX	       8
 
 #endif	/* __DT_BINDINGS_CLOCK_SIFIVE_FU740_PRCI_H */