Re: [U-Boot] [PATCH v1 15/19] spi: mvebu_a3700_spi: Use Armada 37xx clk driver for SPI clock frequency

[Marek Behún]

On Wed, 21 Mar 2018 10:37:33 +0100
Stefan Roese  wrote:

> Please add Jagan (on Cc) to Cc for SPI driver related changes.
Will do with next version

> Wouldn't it be easier to "select" CLK_ARMADA_3720 here instead? No
> changes to the config files necessary this way.

okay :)

> checkpatch will most likely complain about missing space in the line
> above.

yes, thanks

> Perhaps its better to check on "prescale >
> MVEBU_SPI_A3700_CLK_PRESCALE_MASK" instead of silently removing the
> potential additional bits.

The code just above ensures that prescale is never greater than 0x1f
which also is prescale_mask. The compiler should optimize the logical
and away.

I am using constants instead of macros in that code because when I
tried to put macros there with names like
MVEBU_SPI_A3700_CLK_PRESCALE_(MAX|HALF) or what not, the code was
uglier and the name of the macro does not explain much...

Marek
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v1 15/19] spi: mvebu_a3700_spi: Use Armada 37xx clk driver for SPI clock frequency

[Stefan Roese]

Please add Jagan (on Cc) to Cc for SPI driver related changes.

Please find some comments below...

On 07.03.2018 22:52, Marek Behún wrote:
> Since now we have driver for clocks on Armada 37xx, use it to determine
> SQF clock frequency for the SPI driver.
> 
> Also change the default config files for Armada 37xx devices so that
> the clock driver is enabled by default, otherwise the SPI driver cannot
> be enabled.
> 
> Signed-off-by: Marek Behun 
> ---
>   arch/arm/dts/armada-37xx.dtsi   |  4 +--
>   configs/mvebu_db-88f3720_defconfig  |  3 ++
>   configs/mvebu_espressobin-88f3720_defconfig |  3 ++
>   drivers/spi/Kconfig |  1 +
>   drivers/spi/mvebu_a3700_spi.c   | 52 
> -
>   5 files changed, 37 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/dts/armada-37xx.dtsi b/arch/arm/dts/armada-37xx.dtsi
> index e848812fca..c254c0aded 100644
> --- a/arch/arm/dts/armada-37xx.dtsi
> +++ b/arch/arm/dts/armada-37xx.dtsi
> @@ -281,8 +281,8 @@
>   #address-cells = <1>;
>   #size-cells = <0>;
>   #clock-cells = <0>;
> - clock-frequency = <16>;
> - spi-max-frequency = <4>;
> + spi-max-frequency = <5000>;
> + clocks = <_periph_clk 7>;
>   status = "disabled";
>   };
>   
> diff --git a/configs/mvebu_db-88f3720_defconfig 
> b/configs/mvebu_db-88f3720_defconfig
> index 338d764d84..c8ca06e428 100644
> --- a/configs/mvebu_db-88f3720_defconfig
> +++ b/configs/mvebu_db-88f3720_defconfig
> @@ -33,6 +33,9 @@ CONFIG_DM_GPIO=y
>   # CONFIG_MVEBU_GPIO is not set
>   CONFIG_DM_I2C=y
>   CONFIG_MISC=y
> +CONFIG_CLK=y
> +CONFIG_CLK_MVEBU=y
> +CONFIG_CLK_ARMADA_3720=y
>   CONFIG_DM_MMC=y
>   CONFIG_MMC_SDHCI=y
>   CONFIG_MMC_SDHCI_SDMA=y
> diff --git a/configs/mvebu_espressobin-88f3720_defconfig 
> b/configs/mvebu_espressobin-88f3720_defconfig
> index 28005e6131..5f449d34ea 100644
> --- a/configs/mvebu_espressobin-88f3720_defconfig
> +++ b/configs/mvebu_espressobin-88f3720_defconfig
> @@ -30,6 +30,9 @@ CONFIG_SCSI_AHCI=y
>   CONFIG_BLOCK_CACHE=y
>   CONFIG_DM_I2C=y
>   CONFIG_MISC=y
> +CONFIG_CLK=y
> +CONFIG_CLK_MVEBU=y
> +CONFIG_CLK_ARMADA_3720=y
>   CONFIG_DM_MMC=y
>   CONFIG_MMC_SDHCI=y
>   CONFIG_MMC_SDHCI_SDMA=y
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 235a8c7d73..4ea94a5f35 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -94,6 +94,7 @@ config ICH_SPI
>   
>   config MVEBU_A3700_SPI
>   bool "Marvell Armada 3700 SPI driver"
> + depends on CLK_ARMADA_3720

Wouldn't it be easier to "select" CLK_ARMADA_3720 here instead? No
changes to the config files necessary this way.

>   help
> Enable the Marvell Armada 3700 SPI driver. This driver can be
> used to access the SPI NOR flash on platforms embedding this
> diff --git a/drivers/spi/mvebu_a3700_spi.c b/drivers/spi/mvebu_a3700_spi.c
> index d1708a8d56..19e854945b 100644
> --- a/drivers/spi/mvebu_a3700_spi.c
> +++ b/drivers/spi/mvebu_a3700_spi.c
> @@ -10,6 +10,7 @@
>   #include 
>   #include 
>   #include 
> +#include 
>   #include 
>   #include 
>   
> @@ -22,9 +23,8 @@ DECLARE_GLOBAL_DATA_PTR;
>   #define MVEBU_SPI_A3700_CLK_POL BIT(7)
>   #define MVEBU_SPI_A3700_FIFO_EN BIT(17)
>   #define MVEBU_SPI_A3700_SPI_EN_0BIT(16)
> -#define MVEBU_SPI_A3700_CLK_PRESCALE_BIT 0
> -#define MVEBU_SPI_A3700_CLK_PRESCALE_MASK\
> - (0x1f << MVEBU_SPI_A3700_CLK_PRESCALE_BIT)
> +#define MVEBU_SPI_A3700_CLK_PRESCALE_MASK0x1f
> +
>   
>   /* SPI registers */
>   struct spi_reg {
> @@ -36,8 +36,7 @@ struct spi_reg {
>   
>   struct mvebu_spi_platdata {
>   struct spi_reg *spireg;
> - unsigned int frequency;
> - unsigned int clock;
> + struct clk clk;
>   };
>   
>   static void spi_cs_activate(struct spi_reg *reg, int cs)
> @@ -178,17 +177,18 @@ static int mvebu_spi_set_speed(struct udevice *bus, 
> uint hz)
>   {
>   struct mvebu_spi_platdata *plat = dev_get_platdata(bus);
>   struct spi_reg *reg = plat->spireg;
> - u32 data;
> + u32 data, prescale;
>   
>   data = readl(>cfg);
>   
> - /* Set Prescaler */
> - data &= ~MVEBU_SPI_A3700_CLK_PRESCALE_MASK;
> + prescale = DIV_ROUND_UP(clk_get_rate(>clk), hz);
> + if (prescale > 31)
> + prescale = 0x1f;
> + else if (prescale > 15)
> + prescale = 0x10 + (prescale + 1)/2;

checkpatch will most likely complain about missing space in the line
above.

>   
> - /* Calculate Prescaler = (spi_input_freq / spi_max_freq) */
> - if (hz > plat->frequency)
> - hz = plat->frequency;
> - data |= plat->clock / hz;
> + data &= ~MVEBU_SPI_A3700_CLK_PRESCALE_MASK;
> + 

[U-Boot] [PATCH v1 15/19] spi: mvebu_a3700_spi: Use Armada 37xx clk driver for SPI clock frequency

[Marek Behún]

Since now we have driver for clocks on Armada 37xx, use it to determine
SQF clock frequency for the SPI driver.

Also change the default config files for Armada 37xx devices so that
the clock driver is enabled by default, otherwise the SPI driver cannot
be enabled.

Signed-off-by: Marek Behun 
---
 arch/arm/dts/armada-37xx.dtsi   |  4 +--
 configs/mvebu_db-88f3720_defconfig  |  3 ++
 configs/mvebu_espressobin-88f3720_defconfig |  3 ++
 drivers/spi/Kconfig |  1 +
 drivers/spi/mvebu_a3700_spi.c   | 52 -
 5 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/arch/arm/dts/armada-37xx.dtsi b/arch/arm/dts/armada-37xx.dtsi
index e848812fca..c254c0aded 100644
--- a/arch/arm/dts/armada-37xx.dtsi
+++ b/arch/arm/dts/armada-37xx.dtsi
@@ -281,8 +281,8 @@
#address-cells = <1>;
#size-cells = <0>;
#clock-cells = <0>;
-   clock-frequency = <16>;
-   spi-max-frequency = <4>;
+   spi-max-frequency = <5000>;
+   clocks = <_periph_clk 7>;
status = "disabled";
};
 
diff --git a/configs/mvebu_db-88f3720_defconfig 
b/configs/mvebu_db-88f3720_defconfig
index 338d764d84..c8ca06e428 100644
--- a/configs/mvebu_db-88f3720_defconfig
+++ b/configs/mvebu_db-88f3720_defconfig
@@ -33,6 +33,9 @@ CONFIG_DM_GPIO=y
 # CONFIG_MVEBU_GPIO is not set
 CONFIG_DM_I2C=y
 CONFIG_MISC=y
+CONFIG_CLK=y
+CONFIG_CLK_MVEBU=y
+CONFIG_CLK_ARMADA_3720=y
 CONFIG_DM_MMC=y
 CONFIG_MMC_SDHCI=y
 CONFIG_MMC_SDHCI_SDMA=y
diff --git a/configs/mvebu_espressobin-88f3720_defconfig 
b/configs/mvebu_espressobin-88f3720_defconfig
index 28005e6131..5f449d34ea 100644
--- a/configs/mvebu_espressobin-88f3720_defconfig
+++ b/configs/mvebu_espressobin-88f3720_defconfig
@@ -30,6 +30,9 @@ CONFIG_SCSI_AHCI=y
 CONFIG_BLOCK_CACHE=y
 CONFIG_DM_I2C=y
 CONFIG_MISC=y
+CONFIG_CLK=y
+CONFIG_CLK_MVEBU=y
+CONFIG_CLK_ARMADA_3720=y
 CONFIG_DM_MMC=y
 CONFIG_MMC_SDHCI=y
 CONFIG_MMC_SDHCI_SDMA=y
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 235a8c7d73..4ea94a5f35 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -94,6 +94,7 @@ config ICH_SPI
 
 config MVEBU_A3700_SPI
bool "Marvell Armada 3700 SPI driver"
+   depends on CLK_ARMADA_3720
help
  Enable the Marvell Armada 3700 SPI driver. This driver can be
  used to access the SPI NOR flash on platforms embedding this
diff --git a/drivers/spi/mvebu_a3700_spi.c b/drivers/spi/mvebu_a3700_spi.c
index d1708a8d56..19e854945b 100644
--- a/drivers/spi/mvebu_a3700_spi.c
+++ b/drivers/spi/mvebu_a3700_spi.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -22,9 +23,8 @@ DECLARE_GLOBAL_DATA_PTR;
 #define MVEBU_SPI_A3700_CLK_POLBIT(7)
 #define MVEBU_SPI_A3700_FIFO_ENBIT(17)
 #define MVEBU_SPI_A3700_SPI_EN_0   BIT(16)
-#define MVEBU_SPI_A3700_CLK_PRESCALE_BIT   0
-#define MVEBU_SPI_A3700_CLK_PRESCALE_MASK  \
-   (0x1f << MVEBU_SPI_A3700_CLK_PRESCALE_BIT)
+#define MVEBU_SPI_A3700_CLK_PRESCALE_MASK  0x1f
+
 
 /* SPI registers */
 struct spi_reg {
@@ -36,8 +36,7 @@ struct spi_reg {
 
 struct mvebu_spi_platdata {
struct spi_reg *spireg;
-   unsigned int frequency;
-   unsigned int clock;
+   struct clk clk;
 };
 
 static void spi_cs_activate(struct spi_reg *reg, int cs)
@@ -178,17 +177,18 @@ static int mvebu_spi_set_speed(struct udevice *bus, uint 
hz)
 {
struct mvebu_spi_platdata *plat = dev_get_platdata(bus);
struct spi_reg *reg = plat->spireg;
-   u32 data;
+   u32 data, prescale;
 
data = readl(>cfg);
 
-   /* Set Prescaler */
-   data &= ~MVEBU_SPI_A3700_CLK_PRESCALE_MASK;
+   prescale = DIV_ROUND_UP(clk_get_rate(>clk), hz);
+   if (prescale > 31)
+   prescale = 0x1f;
+   else if (prescale > 15)
+   prescale = 0x10 + (prescale + 1)/2;
 
-   /* Calculate Prescaler = (spi_input_freq / spi_max_freq) */
-   if (hz > plat->frequency)
-   hz = plat->frequency;
-   data |= plat->clock / hz;
+   data &= ~MVEBU_SPI_A3700_CLK_PRESCALE_MASK;
+   data |= prescale & MVEBU_SPI_A3700_CLK_PRESCALE_MASK;
 
writel(data, >cfg);
 
@@ -252,21 +252,24 @@ static int mvebu_spi_probe(struct udevice *bus)
 static int mvebu_spi_ofdata_to_platdata(struct udevice *bus)
 {
struct mvebu_spi_platdata *plat = dev_get_platdata(bus);
+   int ret;
 
plat->spireg = (struct spi_reg *)devfdt_get_addr(bus);
 
-   /*
-* FIXME
-* Right now, mvebu does not have a clock infrastructure in U-Boot
-* which should be used to query the input clock to the SPI
-*