diff mbox

[v2,1/8] clk: imx: add common logic to detect early UART usage

Message ID 1441382419-4343-2-git-send-email-l.stach@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Lucas Stach Sept. 4, 2015, 4 p.m. UTC 
Both earlycon and eralyprintk depend on the bootloader setup UART
clocks being retained. This patch adds the common logic to detect such
situations and make the information available to the clock drivers, as
well as adding the facilities to disable those clocks at the end of
the kernel init.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/clk/imx/clk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/imx/clk.h |  1 +
 2 files changed, 47 insertions(+)

Comments

Shawn Guo Sept. 6, 2015, 2:21 p.m. UTC | #1 
On Fri, Sep 04, 2015 at 06:00:12PM +0200, Lucas Stach wrote:
> Both earlycon and eralyprintk depend on the bootloader setup UART
> clocks being retained. This patch adds the common logic to detect such
> situations and make the information available to the clock drivers, as
> well as adding the facilities to disable those clocks at the end of
> the kernel init.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/clk/imx/clk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/imx/clk.h |  1 +
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> index df12b5307175..3357e29e43ab 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -73,3 +73,49 @@ void imx_cscmr1_fixup(u32 *val)
>  	*val ^= CSCMR1_FIXUP;
>  	return;
>  }
> +
> +static int __initdata imx_keep_uart_clocks;

This gives a checkpatch warning as below.

WARNING: __initdata should be placed after imx_keep_uart_clocks
#27: FILE: drivers/clk/imx/clk.c:77:
+static int __initdata imx_keep_uart_clocks;

Shawn

> +static struct clk __initdata ***imx_uart_clocks;
> +
> +static int __init imx_keep_uart_clocks_param(char *str)
> +{
> +	imx_keep_uart_clocks = 1;
> +
> +	return 0;
> +}
> +__setup_param("earlycon", imx_keep_uart_earlycon,
> +	      imx_keep_uart_clocks_param, 0);
> +__setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> +	      imx_keep_uart_clocks_param, 0);
> +
> +void __init imx_register_uart_clocks(struct clk **clks[])
> +{
> +	if (imx_keep_uart_clocks) {
> +		int i;
> +
> +		imx_uart_clocks = clks;
> +		for (i = 0;; i++) {
> +			if (imx_uart_clocks[i])
> +				clk_prepare_enable(*imx_uart_clocks[i]);
> +			else
> +				break;
> +		}
> +	}
> +}
> +
> +static int __init imx_clk_disable_uart(void)
> +{
> +	if (imx_keep_uart_clocks) {
> +		int i;
> +
> +		for (i = 0;; i++) {
> +			if (imx_uart_clocks[i])
> +				clk_disable_unprepare(*imx_uart_clocks[i]);
> +			else
> +				break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +late_initcall_sync(imx_clk_disable_uart);
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> index 1049b0c7d818..c4e2e282e892 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -7,6 +7,7 @@
>  extern spinlock_t imx_ccm_lock;
>  
>  void imx_check_clocks(struct clk *clks[], unsigned int count);
> +void imx_register_uart_clocks(struct clk **clks[]);
>  
>  extern void imx_cscmr1_fixup(u32 *val);
>  
> -- 
> 2.5.0
>
Uwe Kleine-König Sept. 7, 2015, 9:06 a.m. UTC | #2 
Hello Lucas,

On Fri, Sep 04, 2015 at 06:00:12PM +0200, Lucas Stach wrote:
> Both earlycon and eralyprintk depend on the bootloader setup UART
> clocks being retained. This patch adds the common logic to detect such
> situations and make the information available to the clock drivers, as
> well as adding the facilities to disable those clocks at the end of
> the kernel init.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/clk/imx/clk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/imx/clk.h |  1 +
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> index df12b5307175..3357e29e43ab 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -73,3 +73,49 @@ void imx_cscmr1_fixup(u32 *val)
>  	*val ^= CSCMR1_FIXUP;
>  	return;
>  }
> +
> +static int __initdata imx_keep_uart_clocks;
> +static struct clk __initdata ***imx_uart_clocks;
> +
> +static int __init imx_keep_uart_clocks_param(char *str)
> +{
> +	imx_keep_uart_clocks = 1;
> +
> +	return 0;
> +}
> +__setup_param("earlycon", imx_keep_uart_earlycon,
> +	      imx_keep_uart_clocks_param, 0);
> +__setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> +	      imx_keep_uart_clocks_param, 0);
> +
> +void __init imx_register_uart_clocks(struct clk **clks[])

const struct clk **clks[]?

I wonder why you need an array of pointers to pointers of clocks. Isn't
one indirection less possible and more easy?

> +{
> +	if (imx_keep_uart_clocks) {
> +		int i;
> +
> +		imx_uart_clocks = clks;
> +		for (i = 0;; i++) {
> +			if (imx_uart_clocks[i])
> +				clk_prepare_enable(*imx_uart_clocks[i]);
> +			else
> +				break;
> +		}

I would have written this as:

	for (i = 0; imx_uart_clocks[i]; ++i)
		clk_prepare_enable(*imx_uart_clocks[i]);

but I guess that's a matter of taste.

Best regards
Uwe
Lucas Stach Sept. 7, 2015, 9:15 a.m. UTC | #3 
Am Montag, den 07.09.2015, 11:06 +0200 schrieb Uwe Kleine-König:
> Hello Lucas,
> 
> On Fri, Sep 04, 2015 at 06:00:12PM +0200, Lucas Stach wrote:
> > Both earlycon and eralyprintk depend on the bootloader setup UART
> > clocks being retained. This patch adds the common logic to detect such
> > situations and make the information available to the clock drivers, as
> > well as adding the facilities to disable those clocks at the end of
> > the kernel init.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/clk/imx/clk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/clk/imx/clk.h |  1 +
> >  2 files changed, 47 insertions(+)
> > 
> > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> > index df12b5307175..3357e29e43ab 100644
> > --- a/drivers/clk/imx/clk.c
> > +++ b/drivers/clk/imx/clk.c
> > @@ -73,3 +73,49 @@ void imx_cscmr1_fixup(u32 *val)
> >  	*val ^= CSCMR1_FIXUP;
> >  	return;
> >  }
> > +
> > +static int __initdata imx_keep_uart_clocks;
> > +static struct clk __initdata ***imx_uart_clocks;
> > +
> > +static int __init imx_keep_uart_clocks_param(char *str)
> > +{
> > +	imx_keep_uart_clocks = 1;
> > +
> > +	return 0;
> > +}
> > +__setup_param("earlycon", imx_keep_uart_earlycon,
> > +	      imx_keep_uart_clocks_param, 0);
> > +__setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> > +	      imx_keep_uart_clocks_param, 0);
> > +
> > +void __init imx_register_uart_clocks(struct clk **clks[])
> 
> const struct clk **clks[]?
> 
Yep, will do.

> I wonder why you need an array of pointers to pointers of clocks. Isn't
> one indirection less possible and more easy?
> 
No, this is not easily possible if we want to use the auto-discarding of
the clocks array and a simple static initialization. The clock pointers
are only set up after the clock drivers probe routine has run, so if I
want to point to the actual clocks I would need to dynamically populate
the array and either dynamically allocate it, or would need to pre-size
the static array.

So this added indirection and slight complication of the the common code
cuts out some lines of code and chances to get things wrong in the
individual clock drivers. So I think the trade off I made here is
justified.

> > +{
> > +	if (imx_keep_uart_clocks) {
> > +		int i;
> > +
> > +		imx_uart_clocks = clks;
> > +		for (i = 0;; i++) {
> > +			if (imx_uart_clocks[i])
> > +				clk_prepare_enable(*imx_uart_clocks[i]);
> > +			else
> > +				break;
> > +		}
> 
> I would have written this as:
> 
> 	for (i = 0; imx_uart_clocks[i]; ++i)
> 		clk_prepare_enable(*imx_uart_clocks[i]);
> 
> but I guess that's a matter of taste.
> 
Philipp agrees with you. ;) But I like the more explicit style a bit
more.

Regards,
Lucas
Uwe Kleine-König Sept. 7, 2015, 9:50 a.m. UTC | #4 
Hallo Lucas,

On Mon, Sep 07, 2015 at 11:15:54AM +0200, Lucas Stach wrote:
> Am Montag, den 07.09.2015, 11:06 +0200 schrieb Uwe Kleine-König:
> > I wonder why you need an array of pointers to pointers of clocks. Isn't
> > one indirection less possible and more easy?
> > 
> No, this is not easily possible if we want to use the auto-discarding of
> the clocks array and a simple static initialization. The clock pointers
> are only set up after the clock drivers probe routine has run, so if I
> want to point to the actual clocks I would need to dynamically populate
> the array and either dynamically allocate it, or would need to pre-size
> the static array.
Ah right. An alternative would be to just store the indices, but I don't
think this is nicer because then you additionally need a reference to
the clk array. You don't happen to have this anyhow, do you?

Best regards
Uwe
diff mbox

Patch

diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
index df12b5307175..3357e29e43ab 100644
--- a/drivers/clk/imx/clk.c
+++ b/drivers/clk/imx/clk.c
@@ -73,3 +73,49 @@  void imx_cscmr1_fixup(u32 *val)
 	*val ^= CSCMR1_FIXUP;
 	return;
 }
+
+static int __initdata imx_keep_uart_clocks;
+static struct clk __initdata ***imx_uart_clocks;
+
+static int __init imx_keep_uart_clocks_param(char *str)
+{
+	imx_keep_uart_clocks = 1;
+
+	return 0;
+}
+__setup_param("earlycon", imx_keep_uart_earlycon,
+	      imx_keep_uart_clocks_param, 0);
+__setup_param("earlyprintk", imx_keep_uart_earlyprintk,
+	      imx_keep_uart_clocks_param, 0);
+
+void __init imx_register_uart_clocks(struct clk **clks[])
+{
+	if (imx_keep_uart_clocks) {
+		int i;
+
+		imx_uart_clocks = clks;
+		for (i = 0;; i++) {
+			if (imx_uart_clocks[i])
+				clk_prepare_enable(*imx_uart_clocks[i]);
+			else
+				break;
+		}
+	}
+}
+
+static int __init imx_clk_disable_uart(void)
+{
+	if (imx_keep_uart_clocks) {
+		int i;
+
+		for (i = 0;; i++) {
+			if (imx_uart_clocks[i])
+				clk_disable_unprepare(*imx_uart_clocks[i]);
+			else
+				break;
+		}
+	}
+
+	return 0;
+}
+late_initcall_sync(imx_clk_disable_uart);
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index 1049b0c7d818..c4e2e282e892 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -7,6 +7,7 @@ 
 extern spinlock_t imx_ccm_lock;
 
 void imx_check_clocks(struct clk *clks[], unsigned int count);
+void imx_register_uart_clocks(struct clk **clks[]);
 
 extern void imx_cscmr1_fixup(u32 *val);