[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Eugeniy Paltsev]

Add option to set spi controller clock frequency via device tree
using standard clock bindings.
Old way of setting spi controller clock frequency (via implementation
of 'cm_get_spi_controller_clk_hz' function in platform specific code)
remains supported for backward compatibility.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
Changes v1->v2:
  * disable clock if we can't get the rate.
  * get rid of cm_get_spi_controller_clk_hz weak declaration.

 drivers/spi/designware_spi.c | 65 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 5aa507b..9eb5b1c 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -11,6 +11,7 @@
  */
 
 #include <common.h>
+#include <clk.h>
 #include <dm.h>
 #include <errno.h>
 #include <malloc.h>
@@ -18,7 +19,10 @@
 #include <fdtdec.h>
 #include <linux/compat.h>
 #include <asm/io.h>
+/* Only SOCFPGA_GEN5 and SOCFPGA_ARRIA10 uses their clock_manager functions */
+#if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
 #include <asm/arch/clock_manager.h>
+#endif
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -94,6 +98,7 @@ struct dw_spi_priv {
 	void __iomem *regs;
 	unsigned int freq;		/* Default frequency */
 	unsigned int mode;
+	unsigned long bus_clk_rate;
 
 	int bits_per_word;
 	u8 cs;			/* chip select pin */
@@ -176,14 +181,72 @@ static void spi_hw_init(struct dw_spi_priv *priv)
 	debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
 }
 
+static int dw_spi_of_get_clk(struct udevice *bus)
+{
+#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK)
+	struct dw_spi_priv *priv = dev_get_priv(bus);
+	struct clk clk;
+	int ret;
+
+	ret = clk_get_by_index(bus, 0, &clk);
+	if (ret)
+		return -EINVAL;
+
+	ret = clk_enable(&clk);
+	if (ret && ret != -ENOSYS)
+		return ret;
+
+	priv->bus_clk_rate = clk_get_rate(&clk);
+	if (!priv->bus_clk_rate) {
+		clk_disable(&clk);
+		return -EINVAL;
+	}
+
+	clk_free(&clk);
+
+	return 0;
+#else
+	return -ENOSYS;
+#endif
+}
+
+static int dw_spi_get_clk(struct udevice *bus)
+{
+	struct dw_spi_priv *priv = dev_get_priv(bus);
+
+	/* Firstly try to get clock frequency from device tree */
+	if (!dw_spi_of_get_clk(bus))
+		return 0;
+
+	/*
+	 * SOCFPGA_GEN5 and SOCFPGA_ARRIA10 uses cm_get_spi_controller_clk_hz
+	 * function (defined in asm/arch/clock_manager.h) to get spi controller
+	 * clock frequency. So in case of get clock frequency from device
+	 * tree failure rollback to cm_get_spi_controller_clk_hz
+	 */
+#if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
+	priv->bus_clk_rate = cm_get_spi_controller_clk_hz();
+#endif
+
+	if (!priv->bus_clk_rate)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int dw_spi_probe(struct udevice *bus)
 {
 	struct dw_spi_platdata *plat = dev_get_platdata(bus);
 	struct dw_spi_priv *priv = dev_get_priv(bus);
+	int ret;
 
 	priv->regs = plat->regs;
 	priv->freq = plat->frequency;
 
+	ret = dw_spi_get_clk(bus);
+	if (ret)
+		return ret;
+
 	/* Currently only bits_per_word == 8 supported */
 	priv->bits_per_word = 8;
 
@@ -369,7 +432,7 @@ static int dw_spi_set_speed(struct udevice *bus, uint speed)
 	spi_enable_chip(priv, 0);
 
 	/* clk_div doesn't support odd number */
-	clk_div = cm_get_spi_controller_clk_hz() / speed;
+	clk_div = priv->bus_clk_rate / speed;
 	clk_div = (clk_div + 1) & 0xfffe;
 	dw_writel(priv, DW_SPI_BAUDR, clk_div);
 
-- 
2.9.3

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Jagan Teki]

On Fri, Oct 13, 2017 at 8:48 PM, Eugeniy Paltsev
<Eugeniy.Paltsev@synopsys.com> wrote:
> Add option to set spi controller clock frequency via device tree
> using standard clock bindings.
> Old way of setting spi controller clock frequency (via implementation
> of 'cm_get_spi_controller_clk_hz' function in platform specific code)
> remains supported for backward compatibility.
>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
> Changes v1->v2:
>   * disable clock if we can't get the rate.
>   * get rid of cm_get_spi_controller_clk_hz weak declaration.
>
>  drivers/spi/designware_spi.c | 65 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> index 5aa507b..9eb5b1c 100644
> --- a/drivers/spi/designware_spi.c
> +++ b/drivers/spi/designware_spi.c
> @@ -11,6 +11,7 @@
>   */
>
>  #include <common.h>
> +#include <clk.h>
>  #include <dm.h>
>  #include <errno.h>
>  #include <malloc.h>
> @@ -18,7 +19,10 @@
>  #include <fdtdec.h>
>  #include <linux/compat.h>
>  #include <asm/io.h>
> +/* Only SOCFPGA_GEN5 and SOCFPGA_ARRIA10 uses their clock_manager functions */
> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>  #include <asm/arch/clock_manager.h>
> +#endif

How hard it is to make others to use clock manager? do you have any list?

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Eugeniy Paltsev]

Hi Jagan,

On Mon, 2017-10-16 at 22:37 +0530, Jagan Teki wrote:
> On Fri, Oct 13, 2017 at 8:48 PM, Eugeniy Paltsev
> <Eugeniy.Paltsev@synopsys.com> wrote:
> > Add option to set spi controller clock frequency via device tree
> > using standard clock bindings.
> > Old way of setting spi controller clock frequency (via implementation
> > of 'cm_get_spi_controller_clk_hz' function in platform specific code)
> > remains supported for backward compatibility.
> > 
> > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> > ---
> > Changes v1->v2:
> >   * disable clock if we can't get the rate.
> >   * get rid of cm_get_spi_controller_clk_hz weak declaration.
> > 
> >  drivers/spi/designware_spi.c | 65 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 64 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> > index 5aa507b..9eb5b1c 100644
> > --- a/drivers/spi/designware_spi.c
> > +++ b/drivers/spi/designware_spi.c
> > @@ -11,6 +11,7 @@
> >   */
> > 
> >  #include <common.h>
> > +#include <clk.h>
> >  #include <dm.h>
> >  #include <errno.h>
> >  #include <malloc.h>
> > @@ -18,7 +19,10 @@
> >  #include <fdtdec.h>
> >  #include <linux/compat.h>
> >  #include <asm/io.h>
> > +/* Only SOCFPGA_GEN5 and SOCFPGA_ARRIA10 uses their clock_manager functions */
> > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> >  #include <asm/arch/clock_manager.h>
> > +#endif
> 
> How hard it is to make others to use clock manager? do you have any list?

clock_manager.h is an old (and non-generic) way to deal with different clocks.
For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
cm_get_spi_controller_clk_hz function to deal with spi controller clock.

But today we have another, linux-like alternative: to bind clocks via device tree
and manipulate with clocks via generic functions provided by clk.h

In this patch I added option to get clock via device tree using standard bindings
and restrict clock_manager.h functions usage only to targets which still use it,
so new targets can simply bind clock via device tree and they do not need to
implement/define something in clock_manager.h

So we don't need to make others to use clock manager :)


> thanks!
-- 
 Eugeniy Paltsev

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Alexey Brodkin]

Hi Jagan,

> -----Original Message-----
> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
> Sent: Tuesday, October 17, 2017 4:33 PM
> To: jagannadh.teki at gmail.com
> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
> >
> > How hard it is to make others to use clock manager? do you have any list?
> 
> clock_manager.h is an old (and non-generic) way to deal with different clocks.
> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
> 
> But today we have another, linux-like alternative: to bind clocks via device tree
> and manipulate with clocks via generic functions provided by clk.h
> 
> In this patch I added option to get clock via device tree using standard bindings
> and restrict clock_manager.h functions usage only to targets which still use it,
> so new targets can simply bind clock via device tree and they do not need to
> implement/define something in clock_manager.h
> 
> So we don't need to make others to use clock manager :)

Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
generic clk framework?

Marek, any plans for that?

-Alexey

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Jagan Teki]

On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> Hi Jagan,
>
>> -----Original Message-----
>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>> Sent: Tuesday, October 17, 2017 4:33 PM
>> To: jagannadh.teki at gmail.com
>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>> >
>> > How hard it is to make others to use clock manager? do you have any list?
>>
>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>
>> But today we have another, linux-like alternative: to bind clocks via device tree
>> and manipulate with clocks via generic functions provided by clk.h
>>
>> In this patch I added option to get clock via device tree using standard bindings
>> and restrict clock_manager.h functions usage only to targets which still use it,
>> so new targets can simply bind clock via device tree and they do not need to
>> implement/define something in clock_manager.h
>>
>> So we don't need to make others to use clock manager :)
>
> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
> generic clk framework?

Yes, ie what exactly I thought of, thanks!

-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

On 10/17/2017 04:57 PM, Alexey Brodkin wrote:
> Hi Jagan,
> 
>> -----Original Message-----
>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>> Sent: Tuesday, October 17, 2017 4:33 PM
>> To: jagannadh.teki at gmail.com
>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>
>>> How hard it is to make others to use clock manager? do you have any list?
>>
>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>
>> But today we have another, linux-like alternative: to bind clocks via device tree
>> and manipulate with clocks via generic functions provided by clk.h
>>
>> In this patch I added option to get clock via device tree using standard bindings
>> and restrict clock_manager.h functions usage only to targets which still use it,
>> so new targets can simply bind clock via device tree and they do not need to
>> implement/define something in clock_manager.h
>>
>> So we don't need to make others to use clock manager :)
> 
> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
> generic clk framework?

That'd be real neat.

> Marek, any plans for that?

Patches welcome, +CC Dinh.

> -Alexey
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Eugeniy Paltsev]

On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
> > Hi Jagan,
> > 
> > > -----Original Message-----
> > > From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
> > > Sent: Tuesday, October 17, 2017 4:33 PM
> > > To: jagannadh.teki at gmail.com
> > > Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
> > > Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
> > > > 
> > > > How hard it is to make others to use clock manager? do you have any list?
> > > 
> > > clock_manager.h is an old (and non-generic) way to deal with different clocks.
> > > For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
> > > cm_get_spi_controller_clk_hz function to deal with spi controller clock.
> > > 
> > > But today we have another, linux-like alternative: to bind clocks via device tree
> > > and manipulate with clocks via generic functions provided by clk.h
> > > 
> > > In this patch I added option to get clock via device tree using standard bindings
> > > and restrict clock_manager.h functions usage only to targets which still use it,
> > > so new targets can simply bind clock via device tree and they do not need to
> > > implement/define something in clock_manager.h
> > > 
> > > So we don't need to make others to use clock manager :)
> > 
> > Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
> > generic clk framework?
> 
> Yes, ie what exactly I thought of, thanks!

I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it 
manipulate with real hardware.
The only way to do it is to replace SOCFPGA* clock manager functions by real
clock driver.

And given I don't have mentioned hardware so I barely can help with
those improvements on SOCFPGA. That said if there're no short-term plans to
switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?

-- 
 Eugeniy Paltsev

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>> <Alexey.Brodkin@synopsys.com> wrote:
>>> Hi Jagan,
>>>
>>>> -----Original Message-----
>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>> To: jagannadh.teki at gmail.com
>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>
>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>
>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>
>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>
>>>> In this patch I added option to get clock via device tree using standard bindings
>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>> so new targets can simply bind clock via device tree and they do not need to
>>>> implement/define something in clock_manager.h
>>>>
>>>> So we don't need to make others to use clock manager :)
>>>
>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>> generic clk framework?
>>
>> Yes, ie what exactly I thought of, thanks!
> 
> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it 
> manipulate with real hardware.
> The only way to do it is to replace SOCFPGA* clock manager functions by real
> clock driver.
> 
> And given I don't have mentioned hardware so I barely can help with
> those improvements on SOCFPGA. That said if there're no short-term plans to
> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?

Wait for Dinh's reply ...

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Dinh Nguyen]

On 10/19/2017 10:51 AM, Marek Vasut wrote:
> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>> Hi Jagan,
>>>>
>>>>> -----Original Message-----
>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>> To: jagannadh.teki at gmail.com
>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>
>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>
>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>
>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>
>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>> implement/define something in clock_manager.h
>>>>>
>>>>> So we don't need to make others to use clock manager :)
>>>>
>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>> generic clk framework?
>>>
>>> Yes, ie what exactly I thought of, thanks!
>>
>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it 
>> manipulate with real hardware.
>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>> clock driver.
>>
>> And given I don't have mentioned hardware so I barely can help with
>> those improvements on SOCFPGA. That said if there're no short-term plans to
>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
> 
> Wait for Dinh's reply ...
> 

Honestly, I don't have too much time to work on this right now. So I
really don't when it can get done. But it'll go on my to-do list.

Dinh

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Eugeniy Paltsev]

On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
> 
> On 10/19/2017 10:51 AM, Marek Vasut wrote:
> > On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
> > > On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
> > > > On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
> > > > <Alexey.Brodkin@synopsys.com> wrote:
> > > > > Hi Jagan,
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
> > > > > > Sent: Tuesday, October 17, 2017 4:33 PM
> > > > > > To: jagannadh.teki at gmail.com
> > > > > > Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
> > > > > > Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
> > > > > > > 
> > > > > > > How hard it is to make others to use clock manager? do you have any list?
> > > > > > 
> > > > > > clock_manager.h is an old (and non-generic) way to deal with different clocks.
> > > > > > For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
> > > > > > cm_get_spi_controller_clk_hz function to deal with spi controller clock.
> > > > > > 
> > > > > > But today we have another, linux-like alternative: to bind clocks via device tree
> > > > > > and manipulate with clocks via generic functions provided by clk.h
> > > > > > 
> > > > > > In this patch I added option to get clock via device tree using standard bindings
> > > > > > and restrict clock_manager.h functions usage only to targets which still use it,
> > > > > > so new targets can simply bind clock via device tree and they do not need to
> > > > > > implement/define something in clock_manager.h
> > > > > > 
> > > > > > So we don't need to make others to use clock manager :)
> > > > > 
> > > > > Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
> > > > > generic clk framework?
> > > > 
> > > > Yes, ie what exactly I thought of, thanks!
> > > 
> > > I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
> > > SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it 
> > > manipulate with real hardware.
> > > The only way to do it is to replace SOCFPGA* clock manager functions by real
> > > clock driver.
> > > 
> > > And given I don't have mentioned hardware so I barely can help with
> > > those improvements on SOCFPGA. That said if there're no short-term plans to
> > > switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
> > 
> > Wait for Dinh's reply ...
> > 
> 
> Honestly, I don't have too much time to work on this right now. So I
> really don't when it can get done. But it'll go on my to-do list.
> 
> Dinh

Yep, thanks for your comments.

So, Jagan, 
given Dinh's reply, could you please apply this patch?

Thanks.
-- 
 Eugeniy Paltsev

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
>>
>> On 10/19/2017 10:51 AM, Marek Vasut wrote:
>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>>>> Hi Jagan,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>>>
>>>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>>>
>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>>>
>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>>>
>>>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>>>> implement/define something in clock_manager.h
>>>>>>>
>>>>>>> So we don't need to make others to use clock manager :)
>>>>>>
>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>>>> generic clk framework?
>>>>>
>>>>> Yes, ie what exactly I thought of, thanks!
>>>>
>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it 
>>>> manipulate with real hardware.
>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>>>> clock driver.
>>>>
>>>> And given I don't have mentioned hardware so I barely can help with
>>>> those improvements on SOCFPGA. That said if there're no short-term plans to
>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
>>>
>>> Wait for Dinh's reply ...
>>>
>>
>> Honestly, I don't have too much time to work on this right now. So I
>> really don't when it can get done. But it'll go on my to-do list.
>>
>> Dinh
> 
> Yep, thanks for your comments.
> 
> So, Jagan, 
> given Dinh's reply, could you please apply this patch?

I'd really hate it to start seeing soc-specific ifdefs in drivers,
that's IMO not acceptable. A __weak override might be a temporary
solution I'd be willing to live with though.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Jagan Teki]

On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote:
> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
>>>
>>> On 10/19/2017 10:51 AM, Marek Vasut wrote:
>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>>>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>>>>> Hi Jagan,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>>>>
>>>>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>>>>
>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>>>>
>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>>>>
>>>>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>>>>> implement/define something in clock_manager.h
>>>>>>>>
>>>>>>>> So we don't need to make others to use clock manager :)
>>>>>>>
>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>>>>> generic clk framework?
>>>>>>
>>>>>> Yes, ie what exactly I thought of, thanks!
>>>>>
>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it
>>>>> manipulate with real hardware.
>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>>>>> clock driver.
>>>>>
>>>>> And given I don't have mentioned hardware so I barely can help with
>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to
>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
>>>>
>>>> Wait for Dinh's reply ...
>>>>
>>>
>>> Honestly, I don't have too much time to work on this right now. So I
>>> really don't when it can get done. But it'll go on my to-do list.
>>>
>>> Dinh
>>
>> Yep, thanks for your comments.
>>
>> So, Jagan,
>> given Dinh's reply, could you please apply this patch?
>
> I'd really hate it to start seeing soc-specific ifdefs in drivers,
> that's IMO not acceptable. A __weak override might be a temporary
> solution I'd be willing to live with though.

I would rather like to see some check on clock manager itself whether
CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use
__weak as well soc #ifdefs in driver.

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

On 10/24/2017 11:52 AM, Jagan Teki wrote:
> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote:
>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
>>>>
>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote:
>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>>>>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>>>>>> Hi Jagan,
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>>>>>
>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>>>>>
>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>>>>>
>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>>>>>
>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>>>>>> implement/define something in clock_manager.h
>>>>>>>>>
>>>>>>>>> So we don't need to make others to use clock manager :)
>>>>>>>>
>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>>>>>> generic clk framework?
>>>>>>>
>>>>>>> Yes, ie what exactly I thought of, thanks!
>>>>>>
>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it
>>>>>> manipulate with real hardware.
>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>>>>>> clock driver.
>>>>>>
>>>>>> And given I don't have mentioned hardware so I barely can help with
>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to
>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
>>>>>
>>>>> Wait for Dinh's reply ...
>>>>>
>>>>
>>>> Honestly, I don't have too much time to work on this right now. So I
>>>> really don't when it can get done. But it'll go on my to-do list.
>>>>
>>>> Dinh
>>>
>>> Yep, thanks for your comments.
>>>
>>> So, Jagan,
>>> given Dinh's reply, could you please apply this patch?
>>
>> I'd really hate it to start seeing soc-specific ifdefs in drivers,
>> that's IMO not acceptable. A __weak override might be a temporary
>> solution I'd be willing to live with though.
> 
> I would rather like to see some check on clock manager itself whether
> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use
> __weak as well soc #ifdefs in driver.

I don't understand what you mean by this , can you elucidate ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Eugeniy Paltsev]

On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote:
> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote:
> > On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
> > > On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
> > > > 
> > > > On 10/19/2017 10:51 AM, Marek Vasut wrote:
> > > > > On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
> > > > > > On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
> > > > > > > On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
> > > > > > > <Alexey.Brodkin@synopsys.com> wrote:
> > > > > > > > Hi Jagan,
> > > > > > > > 
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
> > > > > > > > > Sent: Tuesday, October 17, 2017 4:33 PM
> > > > > > > > > To: jagannadh.teki at gmail.com
> > > > > > > > > Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
> > > > > > > > > Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
> > > > > > > > > > 
> > > > > > > > > > How hard it is to make others to use clock manager? do you have any list?
> > > > > > > > > 
> > > > > > > > > clock_manager.h is an old (and non-generic) way to deal with different clocks.
> > > > > > > > > For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
> > > > > > > > > cm_get_spi_controller_clk_hz function to deal with spi controller clock.
> > > > > > > > > 
> > > > > > > > > But today we have another, linux-like alternative: to bind clocks via device tree
> > > > > > > > > and manipulate with clocks via generic functions provided by clk.h
> > > > > > > > > 
> > > > > > > > > In this patch I added option to get clock via device tree using standard bindings
> > > > > > > > > and restrict clock_manager.h functions usage only to targets which still use it,
> > > > > > > > > so new targets can simply bind clock via device tree and they do not need to
> > > > > > > > > implement/define something in clock_manager.h
> > > > > > > > > 
> > > > > > > > > So we don't need to make others to use clock manager :)
> > > > > > > > 
> > > > > > > > Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
> > > > > > > > generic clk framework?
> > > > > > > 
> > > > > > > Yes, ie what exactly I thought of, thanks!
> > > > > > 
> > > > > > I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
> > > > > > SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it
> > > > > > manipulate with real hardware.
> > > > > > The only way to do it is to replace SOCFPGA* clock manager functions by real
> > > > > > clock driver.
> > > > > > 
> > > > > > And given I don't have mentioned hardware so I barely can help with
> > > > > > those improvements on SOCFPGA. That said if there're no short-term plans to
> > > > > > switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
> > > > > 
> > > > > Wait for Dinh's reply ...
> > > > > 
> > > > 
> > > > Honestly, I don't have too much time to work on this right now. So I
> > > > really don't when it can get done. But it'll go on my to-do list.
> > > > 
> > > > Dinh
> > > 
> > > Yep, thanks for your comments.
> > > 
> > > So, Jagan,
> > > given Dinh's reply, could you please apply this patch?
> > 
> > I'd really hate it to start seeing soc-specific ifdefs in drivers,
> > that's IMO not acceptable. A __weak override might be a temporary
> > solution I'd be willing to live with though.
> 
> I would rather like to see some check on clock manager itself whether
> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use
> __weak as well soc #ifdefs in driver.
> 

Actually I don't understand what do you mean.
Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h
include with #ifdefs as clock_manager.h is defined only for two 
targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10.

#if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
#include <asm/arch/clock_manager.h>
#endif

And I think it is better to add this #ifdef in driver than create empty 
clock_manager.h file for every new target which uses this driver.
-- 
 Eugeniy Paltsev

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote:
> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote:
>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote:
>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
>>>>>
>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote:
>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>>>>>>> Hi Jagan,
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>>>>>>
>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>>>>>>
>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>>>>>>
>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>>>>>>
>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>>>>>>> implement/define something in clock_manager.h
>>>>>>>>>>
>>>>>>>>>> So we don't need to make others to use clock manager :)
>>>>>>>>>
>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>>>>>>> generic clk framework?
>>>>>>>>
>>>>>>>> Yes, ie what exactly I thought of, thanks!
>>>>>>>
>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it
>>>>>>> manipulate with real hardware.
>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>>>>>>> clock driver.
>>>>>>>
>>>>>>> And given I don't have mentioned hardware so I barely can help with
>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to
>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
>>>>>>
>>>>>> Wait for Dinh's reply ...
>>>>>>
>>>>>
>>>>> Honestly, I don't have too much time to work on this right now. So I
>>>>> really don't when it can get done. But it'll go on my to-do list.
>>>>>
>>>>> Dinh
>>>>
>>>> Yep, thanks for your comments.
>>>>
>>>> So, Jagan,
>>>> given Dinh's reply, could you please apply this patch?
>>>
>>> I'd really hate it to start seeing soc-specific ifdefs in drivers,
>>> that's IMO not acceptable. A __weak override might be a temporary
>>> solution I'd be willing to live with though.
>>
>> I would rather like to see some check on clock manager itself whether
>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use
>> __weak as well soc #ifdefs in driver.
>>
> 
> Actually I don't understand what do you mean.
> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h
> include with #ifdefs as clock_manager.h is defined only for two 
> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10.
> 
> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> #include <asm/arch/clock_manager.h>
> #endif
> 
> And I think it is better to add this #ifdef in driver than create empty 
> clock_manager.h file for every new target which uses this driver.

If you have weak default implementation in the DW SPI driver of some
function to get the clock rate and override it in the socfpga clock
manager, then you don't need to use ifdefs .

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Jagan Teki]

On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote:
> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote:
>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote:
>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote:
>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
>>>>>>
>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote:
>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>>>>>>>> Hi Jagan,
>>>>>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>>>>>>>
>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>>>>>>>
>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>>>>>>>
>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>>>>>>>
>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>>>>>>>> implement/define something in clock_manager.h
>>>>>>>>>>>
>>>>>>>>>>> So we don't need to make others to use clock manager :)
>>>>>>>>>>
>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>>>>>>>> generic clk framework?
>>>>>>>>>
>>>>>>>>> Yes, ie what exactly I thought of, thanks!
>>>>>>>>
>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it
>>>>>>>> manipulate with real hardware.
>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>>>>>>>> clock driver.
>>>>>>>>
>>>>>>>> And given I don't have mentioned hardware so I barely can help with
>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to
>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
>>>>>>>
>>>>>>> Wait for Dinh's reply ...
>>>>>>>
>>>>>>
>>>>>> Honestly, I don't have too much time to work on this right now. So I
>>>>>> really don't when it can get done. But it'll go on my to-do list.
>>>>>>
>>>>>> Dinh
>>>>>
>>>>> Yep, thanks for your comments.
>>>>>
>>>>> So, Jagan,
>>>>> given Dinh's reply, could you please apply this patch?
>>>>
>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers,
>>>> that's IMO not acceptable. A __weak override might be a temporary
>>>> solution I'd be willing to live with though.
>>>
>>> I would rather like to see some check on clock manager itself whether
>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use
>>> __weak as well soc #ifdefs in driver.
>>>
>>
>> Actually I don't understand what do you mean.
>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h
>> include with #ifdefs as clock_manager.h is defined only for two
>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10.
>>
>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>> #include <asm/arch/clock_manager.h>
>> #endif
>>
>> And I think it is better to add this #ifdef in driver than create empty
>> clock_manager.h file for every new target which uses this driver.

Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used
in other IP's as well. we need to carefully adding these check and if
require add CLK for remaining drivers as well. the reason for adding
checks in clock-manager is it may be removable code if all use CLK.

--- a/arch/arm/mach-socfpga/clock_manager_arria10.c
+++ b/arch/arm/mach-socfpga/clock_manager_arria10.c
@@ -1066,10 +1066,17 @@ unsigned int cm_get_mmc_controller_clk_hz(void)
        return clk_hz / 4;
 }

+#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK)
+unsigned int cm_get_spi_controller_clk_hz(void)
+{
+       return 0;
+}
+#else
 unsigned int cm_get_spi_controller_clk_hz(void)
 {
        return cm_get_l4_noc_hz(CLKMGR_MAINPLL_NOCDIV_L4MPCLK_LSB);
 }
+#endif

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

On 10/30/2017 07:04 AM, Jagan Teki wrote:
> On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote:
>> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote:
>>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote:
>>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote:
>>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
>>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
>>>>>>>
>>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote:
>>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>>>>>>>>> Hi Jagan,
>>>>>>>>>>>
>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>>>>>>>>
>>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>>>>>>>>
>>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>>>>>>>>
>>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>>>>>>>>
>>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>>>>>>>>> implement/define something in clock_manager.h
>>>>>>>>>>>>
>>>>>>>>>>>> So we don't need to make others to use clock manager :)
>>>>>>>>>>>
>>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>>>>>>>>> generic clk framework?
>>>>>>>>>>
>>>>>>>>>> Yes, ie what exactly I thought of, thanks!
>>>>>>>>>
>>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it
>>>>>>>>> manipulate with real hardware.
>>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>>>>>>>>> clock driver.
>>>>>>>>>
>>>>>>>>> And given I don't have mentioned hardware so I barely can help with
>>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to
>>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
>>>>>>>>
>>>>>>>> Wait for Dinh's reply ...
>>>>>>>>
>>>>>>>
>>>>>>> Honestly, I don't have too much time to work on this right now. So I
>>>>>>> really don't when it can get done. But it'll go on my to-do list.
>>>>>>>
>>>>>>> Dinh
>>>>>>
>>>>>> Yep, thanks for your comments.
>>>>>>
>>>>>> So, Jagan,
>>>>>> given Dinh's reply, could you please apply this patch?
>>>>>
>>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers,
>>>>> that's IMO not acceptable. A __weak override might be a temporary
>>>>> solution I'd be willing to live with though.
>>>>
>>>> I would rather like to see some check on clock manager itself whether
>>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use
>>>> __weak as well soc #ifdefs in driver.
>>>>
>>>
>>> Actually I don't understand what do you mean.
>>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h
>>> include with #ifdefs as clock_manager.h is defined only for two
>>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10.
>>>
>>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>> #include <asm/arch/clock_manager.h>
>>> #endif
>>>
>>> And I think it is better to add this #ifdef in driver than create empty
>>> clock_manager.h file for every new target which uses this driver.
> 
> Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used
> in other IP's as well. we need to carefully adding these check and if
> require add CLK for remaining drivers as well. the reason for adding
> checks in clock-manager is it may be removable code if all use CLK.

I do not understand what you're trying to tell me, but the patch below
makes no sense. What I would like to see is a weak function in the
driver and that function be overriden in the socfpga clock manager.

> --- a/arch/arm/mach-socfpga/clock_manager_arria10.c
> +++ b/arch/arm/mach-socfpga/clock_manager_arria10.c
> @@ -1066,10 +1066,17 @@ unsigned int cm_get_mmc_controller_clk_hz(void)
>         return clk_hz / 4;
>  }
> 
> +#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK)
> +unsigned int cm_get_spi_controller_clk_hz(void)
> +{
> +       return 0;
> +}
> +#else
>  unsigned int cm_get_spi_controller_clk_hz(void)
>  {
>         return cm_get_l4_noc_hz(CLKMGR_MAINPLL_NOCDIV_L4MPCLK_LSB);
>  }
> +#endif
> 
> thanks!
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Jagan Teki]

On Mon, Oct 30, 2017 at 4:24 PM, Marek Vasut <marex@denx.de> wrote:
> On 10/30/2017 07:04 AM, Jagan Teki wrote:
>> On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote:
>>>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote:
>>>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
>>>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
>>>>>>>>
>>>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote:
>>>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>>>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>>>>>>>>>> Hi Jagan,
>>>>>>>>>>>>
>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>>>>>>>>>
>>>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>>>>>>>>>
>>>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>>>>>>>>>
>>>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>>>>>>>>>> implement/define something in clock_manager.h
>>>>>>>>>>>>>
>>>>>>>>>>>>> So we don't need to make others to use clock manager :)
>>>>>>>>>>>>
>>>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>>>>>>>>>> generic clk framework?
>>>>>>>>>>>
>>>>>>>>>>> Yes, ie what exactly I thought of, thanks!
>>>>>>>>>>
>>>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>>>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it
>>>>>>>>>> manipulate with real hardware.
>>>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>>>>>>>>>> clock driver.
>>>>>>>>>>
>>>>>>>>>> And given I don't have mentioned hardware so I barely can help with
>>>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to
>>>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
>>>>>>>>>
>>>>>>>>> Wait for Dinh's reply ...
>>>>>>>>>
>>>>>>>>
>>>>>>>> Honestly, I don't have too much time to work on this right now. So I
>>>>>>>> really don't when it can get done. But it'll go on my to-do list.
>>>>>>>>
>>>>>>>> Dinh
>>>>>>>
>>>>>>> Yep, thanks for your comments.
>>>>>>>
>>>>>>> So, Jagan,
>>>>>>> given Dinh's reply, could you please apply this patch?
>>>>>>
>>>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers,
>>>>>> that's IMO not acceptable. A __weak override might be a temporary
>>>>>> solution I'd be willing to live with though.
>>>>>
>>>>> I would rather like to see some check on clock manager itself whether
>>>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use
>>>>> __weak as well soc #ifdefs in driver.
>>>>>
>>>>
>>>> Actually I don't understand what do you mean.
>>>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h
>>>> include with #ifdefs as clock_manager.h is defined only for two
>>>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10.
>>>>
>>>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>>> #include <asm/arch/clock_manager.h>
>>>> #endif
>>>>
>>>> And I think it is better to add this #ifdef in driver than create empty
>>>> clock_manager.h file for every new target which uses this driver.
>>
>> Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used
>> in other IP's as well. we need to carefully adding these check and if
>> require add CLK for remaining drivers as well. the reason for adding
>> checks in clock-manager is it may be removable code if all use CLK.
>
> I do not understand what you're trying to tell me, but the patch below
> makes no sense. What I would like to see is a weak function in the
> driver and that function be overriden in the socfpga clock manager.

I'm trying to avoid __weak or #ifdef on driver instead make changes in
clock manager which will remove in future(once all moved with CLK)

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

On 10/30/2017 12:36 PM, Jagan Teki wrote:
> On Mon, Oct 30, 2017 at 4:24 PM, Marek Vasut <marex@denx.de> wrote:
>> On 10/30/2017 07:04 AM, Jagan Teki wrote:
>>> On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote:
>>>>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote:
>>>>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
>>>>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
>>>>>>>>>
>>>>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote:
>>>>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>>>>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>>>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>>>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>>>>>>>>>>> Hi Jagan,
>>>>>>>>>>>>>
>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>>>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>>>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>>>>>>>>>>> implement/define something in clock_manager.h
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So we don't need to make others to use clock manager :)
>>>>>>>>>>>>>
>>>>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>>>>>>>>>>> generic clk framework?
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, ie what exactly I thought of, thanks!
>>>>>>>>>>>
>>>>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>>>>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it
>>>>>>>>>>> manipulate with real hardware.
>>>>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>>>>>>>>>>> clock driver.
>>>>>>>>>>>
>>>>>>>>>>> And given I don't have mentioned hardware so I barely can help with
>>>>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to
>>>>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
>>>>>>>>>>
>>>>>>>>>> Wait for Dinh's reply ...
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Honestly, I don't have too much time to work on this right now. So I
>>>>>>>>> really don't when it can get done. But it'll go on my to-do list.
>>>>>>>>>
>>>>>>>>> Dinh
>>>>>>>>
>>>>>>>> Yep, thanks for your comments.
>>>>>>>>
>>>>>>>> So, Jagan,
>>>>>>>> given Dinh's reply, could you please apply this patch?
>>>>>>>
>>>>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers,
>>>>>>> that's IMO not acceptable. A __weak override might be a temporary
>>>>>>> solution I'd be willing to live with though.
>>>>>>
>>>>>> I would rather like to see some check on clock manager itself whether
>>>>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use
>>>>>> __weak as well soc #ifdefs in driver.
>>>>>>
>>>>>
>>>>> Actually I don't understand what do you mean.
>>>>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h
>>>>> include with #ifdefs as clock_manager.h is defined only for two
>>>>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10.
>>>>>
>>>>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>>>> #include <asm/arch/clock_manager.h>
>>>>> #endif
>>>>>
>>>>> And I think it is better to add this #ifdef in driver than create empty
>>>>> clock_manager.h file for every new target which uses this driver.
>>>
>>> Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used
>>> in other IP's as well. we need to carefully adding these check and if
>>> require add CLK for remaining drivers as well. the reason for adding
>>> checks in clock-manager is it may be removable code if all use CLK.
>>
>> I do not understand what you're trying to tell me, but the patch below
>> makes no sense. What I would like to see is a weak function in the
>> driver and that function be overriden in the socfpga clock manager.
> 
> I'm trying to avoid __weak or #ifdef on driver instead make changes in
> clock manager which will remove in future(once all moved with CLK)

Well, if you do the proposed change, socfpga will break. The __weak is
the least possible evil here and I can live with that for now.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Jagan Teki]

On Mon, Oct 30, 2017 at 5:12 PM, Marek Vasut <marex@denx.de> wrote:
> On 10/30/2017 12:36 PM, Jagan Teki wrote:
>> On Mon, Oct 30, 2017 at 4:24 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 10/30/2017 07:04 AM, Jagan Teki wrote:
>>>> On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote:
>>>>> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote:
>>>>>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote:
>>>>>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
>>>>>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
>>>>>>>>>>
>>>>>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote:
>>>>>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>>>>>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>>>>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>>>>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>>>>>>>>>>>> Hi Jagan,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>>>>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>>>>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>>>>>>>>>>>> implement/define something in clock_manager.h
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So we don't need to make others to use clock manager :)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>>>>>>>>>>>> generic clk framework?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes, ie what exactly I thought of, thanks!
>>>>>>>>>>>>
>>>>>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>>>>>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it
>>>>>>>>>>>> manipulate with real hardware.
>>>>>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>>>>>>>>>>>> clock driver.
>>>>>>>>>>>>
>>>>>>>>>>>> And given I don't have mentioned hardware so I barely can help with
>>>>>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to
>>>>>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
>>>>>>>>>>>
>>>>>>>>>>> Wait for Dinh's reply ...
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Honestly, I don't have too much time to work on this right now. So I
>>>>>>>>>> really don't when it can get done. But it'll go on my to-do list.
>>>>>>>>>>
>>>>>>>>>> Dinh
>>>>>>>>>
>>>>>>>>> Yep, thanks for your comments.
>>>>>>>>>
>>>>>>>>> So, Jagan,
>>>>>>>>> given Dinh's reply, could you please apply this patch?
>>>>>>>>
>>>>>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers,
>>>>>>>> that's IMO not acceptable. A __weak override might be a temporary
>>>>>>>> solution I'd be willing to live with though.
>>>>>>>
>>>>>>> I would rather like to see some check on clock manager itself whether
>>>>>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use
>>>>>>> __weak as well soc #ifdefs in driver.
>>>>>>>
>>>>>>
>>>>>> Actually I don't understand what do you mean.
>>>>>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h
>>>>>> include with #ifdefs as clock_manager.h is defined only for two
>>>>>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10.
>>>>>>
>>>>>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>>>>> #include <asm/arch/clock_manager.h>
>>>>>> #endif
>>>>>>
>>>>>> And I think it is better to add this #ifdef in driver than create empty
>>>>>> clock_manager.h file for every new target which uses this driver.
>>>>
>>>> Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used
>>>> in other IP's as well. we need to carefully adding these check and if
>>>> require add CLK for remaining drivers as well. the reason for adding
>>>> checks in clock-manager is it may be removable code if all use CLK.
>>>
>>> I do not understand what you're trying to tell me, but the patch below
>>> makes no sense. What I would like to see is a weak function in the
>>> driver and that function be overriden in the socfpga clock manager.
>>
>> I'm trying to avoid __weak or #ifdef on driver instead make changes in
>> clock manager which will remove in future(once all moved with CLK)
>
> Well, if you do the proposed change, socfpga will break. The __weak is
> the least possible evil here and I can live with that for now.

Yes, __weak can be possible evil here but since I'm planning to apply
this in next version, I'm hoping possible change that shouldn't effect
the driver. I see some __weak's last from many releases that doesn't
resolve properly and they are still __weak functions only.

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

On 10/31/2017 09:27 AM, Jagan Teki wrote:
> On Mon, Oct 30, 2017 at 5:12 PM, Marek Vasut <marex@denx.de> wrote:
>> On 10/30/2017 12:36 PM, Jagan Teki wrote:
>>> On Mon, Oct 30, 2017 at 4:24 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On 10/30/2017 07:04 AM, Jagan Teki wrote:
>>>>> On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote:
>>>>>>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote:
>>>>>>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote:
>>>>>>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote:
>>>>>>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote:
>>>>>>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote:
>>>>>>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin
>>>>>>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>>>>>>>>>>>>> Hi Jagan,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>>>>>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>>>>>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
>>>>>>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks.
>>>>>>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides
>>>>>>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree
>>>>>>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings
>>>>>>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it,
>>>>>>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to
>>>>>>>>>>>>>>>> implement/define something in clock_manager.h
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So we don't need to make others to use clock manager :)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to
>>>>>>>>>>>>>>> generic clk framework?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yes, ie what exactly I thought of, thanks!
>>>>>>>>>>>>>
>>>>>>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and
>>>>>>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it
>>>>>>>>>>>>> manipulate with real hardware.
>>>>>>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real
>>>>>>>>>>>>> clock driver.
>>>>>>>>>>>>>
>>>>>>>>>>>>> And given I don't have mentioned hardware so I barely can help with
>>>>>>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to
>>>>>>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs?
>>>>>>>>>>>>
>>>>>>>>>>>> Wait for Dinh's reply ...
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Honestly, I don't have too much time to work on this right now. So I
>>>>>>>>>>> really don't when it can get done. But it'll go on my to-do list.
>>>>>>>>>>>
>>>>>>>>>>> Dinh
>>>>>>>>>>
>>>>>>>>>> Yep, thanks for your comments.
>>>>>>>>>>
>>>>>>>>>> So, Jagan,
>>>>>>>>>> given Dinh's reply, could you please apply this patch?
>>>>>>>>>
>>>>>>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers,
>>>>>>>>> that's IMO not acceptable. A __weak override might be a temporary
>>>>>>>>> solution I'd be willing to live with though.
>>>>>>>>
>>>>>>>> I would rather like to see some check on clock manager itself whether
>>>>>>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use
>>>>>>>> __weak as well soc #ifdefs in driver.
>>>>>>>>
>>>>>>>
>>>>>>> Actually I don't understand what do you mean.
>>>>>>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h
>>>>>>> include with #ifdefs as clock_manager.h is defined only for two
>>>>>>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10.
>>>>>>>
>>>>>>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>>>>>> #include <asm/arch/clock_manager.h>
>>>>>>> #endif
>>>>>>>
>>>>>>> And I think it is better to add this #ifdef in driver than create empty
>>>>>>> clock_manager.h file for every new target which uses this driver.
>>>>>
>>>>> Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used
>>>>> in other IP's as well. we need to carefully adding these check and if
>>>>> require add CLK for remaining drivers as well. the reason for adding
>>>>> checks in clock-manager is it may be removable code if all use CLK.
>>>>
>>>> I do not understand what you're trying to tell me, but the patch below
>>>> makes no sense. What I would like to see is a weak function in the
>>>> driver and that function be overriden in the socfpga clock manager.
>>>
>>> I'm trying to avoid __weak or #ifdef on driver instead make changes in
>>> clock manager which will remove in future(once all moved with CLK)
>>
>> Well, if you do the proposed change, socfpga will break. The __weak is
>> the least possible evil here and I can live with that for now.
> 
> Yes, __weak can be possible evil here but since I'm planning to apply
> this in next version, I'm hoping possible change that shouldn't effect
> the driver. I see some __weak's last from many releases that doesn't
> resolve properly and they are still __weak functions only.

The idea here would be to have the default __weak implementation use
clock framework in the driver and override it only for socfpga in the
socfpga clock manager.

So for this patch, that's a NAK, I'd like a V3 with the __weak.

-- 
Best regards,
Marek Vasut