Add initial support for Broadcom Cygnus SoC

Adds initial support for the Cygnus SoC based on Broadcom?s iProc series.

Reviewed-by Ray Jui <[email protected]>

Reviewed-by Desmond Liu <[email protected]>

Reviewed-by JD (Jiandong) Zheng <[email protected]>

Tested-by Jonathan Richardson <[email protected]>

Signed-off-by Jonathan Richardson <[email protected]>

---
 arch/arm/mach-bcm/Kconfig            |   34 +++++++
 arch/arm/mach-bcm/Makefile           |    3 +
 arch/arm/mach-bcm/board_bcm_cygnus.c |  166 ++++++++++++++++++++++++++++++++++

 3 files changed, 203 insertions(+)
 create mode 100644 arch/arm/mach-bcm/board_bcm_cygnus.c

arch/arm/mach-bcm/Kconfig +0 -0

index fc93800..58e0f20 100644

arch/arm/mach-bcm/Kconfig +23 -0

+	select GENERIC_CLOCKEVENTS
+	select ARM_GLOBAL_TIMER
+	select ARCH_REQUIRE_GPIOLIB
+	select ARM_AMBA
+	select PINCTRL
+	select DEBUG_UART_8250
+	help
+	  This enables support for systems based on Broadcom IPROC architected SoCs.
+	  The IPROC complex contains one or more ARM CPUs along with common
+	  core periperals. Application specific SoCs are created by adding a
+	  uArchitecture containing peripherals outside of the IPROC complex.
+	  Currently supported SoCs are Cygnus.
+
+menu "iProc SoC based Machine types"
+	depends on ARCH_BCM_IPROC
+
+	config ARCH_BCM_CYGNUS
+		bool "Support Broadcom Cygnus board"
+		select USB_ARCH_HAS_EHCI if USB_SUPPORT
+		help
+		  Support for Broadcom Cygnus SoC.
+endmenu
+
 config ARCH_BCM_MOBILE
 	bool "Broadcom Mobile SoC Support" if ARCH_MULTI_V7
 	select ARCH_REQUIRE_GPIOLIB

arch/arm/mach-bcm/Makefile +3 -0

index b19a396..dd14a10 100644
@@ -10,6 +10,9 @@
 # of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 
+# Cygnus
+obj-$(CONFIG_ARCH_BCM_CYGNUS) +=  board_bcm_cygnus.o
+
 # BCM281XX
 obj-$(CONFIG_ARCH_BCM_281XX)	+= board_bcm281xx.o
 

arch/arm/mach-bcm/board_bcm_cygnus.c new file +0 -0

new file mode 100644
index 0000000..d67555a

arch/arm/mach-bcm/board_bcm_cygnus.c new file +8 -0

+#define CRU_SIZE                  0x34
+#define CRU_CONTROL_OFFSET        0x0
+#define CRU_PWRDWN_EN_OFFSET      0x4
+#define CRU_PWRDWN_STATUS_OFFSET  0x8
+#define CRU_NEON0_HW_RESET  6
+#define CRU_CLAMP_ON_NEON0  20
+#define CRU_PWRONIN_NEON0   21
+#define CRU_PWRONOUT_NEON0  21

arch/arm/mach-bcm/board_bcm_cygnus.c new file +15 -0

+
+	/* De-assert the neon hardware block reset */
+	reg = readl(cru_base + CRU_CONTROL_OFFSET);
+	reg &= ~(1 << CRU_NEON0_HW_RESET);
+	writel(reg, cru_base + CRU_CONTROL_OFFSET);
+
+	/* Assert the power ON register bit */
+	reg = readl(cru_base + CRU_PWRDWN_EN_OFFSET);
+	reg |= (1 << CRU_PWRONIN_NEON0);
+	writel(reg, cru_base + CRU_PWRDWN_EN_OFFSET);
+
+	/*
+	 * Wait up to 10 usec in 1 usec increments for the
+	 * status register to acknowledge the power ON assert
+	 */

arch/arm/mach-bcm/board_bcm_cygnus.c new file +9 -0

+
+#ifdef CONFIG_NEON
+	bcm_cygnus_powerup_neon();
+#endif
+}
+
+/*
+ * Reset the system
+ */

Quoted 11 lines · Jonathan Richardson

Adds initial support for the Cygnus SoC based on Broadcom?s iProc series.

Reviewed-by Ray Jui <[email protected]>

Reviewed-by Desmond Liu <[email protected]>

Reviewed-by JD (Jiandong) Zheng <[email protected]>

Tested-by Jonathan Richardson <[email protected]>

Signed-off-by Jonathan Richardson <[email protected]>

---
 arch/arm/mach-bcm/Kconfig            |   34 +++++++
 arch/arm/mach-bcm/Makefile           |    3 +
 arch/arm/mach-bcm/board_bcm_cygnus.c |  166 ++++++++++++++++++++++++++++++++++

Is Cygnus an SoC or a board?

Quoted 12 lines

 3 files changed, 203 insertions(+)
 create mode 100644 arch/arm/mach-bcm/board_bcm_cygnus.c

arch/arm/mach-bcm/Kconfig +0 -0

index fc93800..58e0f20 100644
@@ -5,6 +5,40 @@ menuconfig ARCH_BCM
 
 if ARCH_BCM
 

arch/arm/mach-bcm/Kconfig menuconfig ARCH_BCM · line 12

Quoted 5 lines

arch/arm/mach-bcm/Kconfig +5 -0

+config ARCH_BCM_IPROC
+	bool "Broadcom ARMv7 iProc boards" if ARCH_MULTI_V7
+	select ARM_GIC
+	select CACHE_L2X0
+	select HAVE_ARM_SCU if SMP

I didn't spot any SMP code in this series.

arch/arm/mach-bcm/Kconfig menuconfig ARCH_BCM · line 18

Quoted 7 lines

arch/arm/mach-bcm/Kconfig +6 -0

+	select HAVE_ARM_TWD if LOCAL_TIMERS
+	select HAVE_CLK
+	select CLKSRC_OF
+	select CLKSRC_MMIO
+	select LOCAL_TIMERS if SMP
+	select GENERIC_CLOCKEVENTS_BUILD

You don't need to select this.

Quoted 41 lines

arch/arm/mach-bcm/Kconfig +23 -0

+	select GENERIC_CLOCKEVENTS
+	select ARM_GLOBAL_TIMER
+	select ARCH_REQUIRE_GPIOLIB
+	select ARM_AMBA
+	select PINCTRL
+	select DEBUG_UART_8250
+	help
+	  This enables support for systems based on Broadcom IPROC architected SoCs.
+	  The IPROC complex contains one or more ARM CPUs along with common
+	  core periperals. Application specific SoCs are created by adding a
+	  uArchitecture containing peripherals outside of the IPROC complex.
+	  Currently supported SoCs are Cygnus.
+
+menu "iProc SoC based Machine types"
+	depends on ARCH_BCM_IPROC
+
+	config ARCH_BCM_CYGNUS
+		bool "Support Broadcom Cygnus board"
+		select USB_ARCH_HAS_EHCI if USB_SUPPORT
+		help
+		  Support for Broadcom Cygnus SoC.
+endmenu
+
 config ARCH_BCM_MOBILE
 	bool "Broadcom Mobile SoC Support" if ARCH_MULTI_V7
 	select ARCH_REQUIRE_GPIOLIB

arch/arm/mach-bcm/Makefile +3 -0

index b19a396..dd14a10 100644
@@ -10,6 +10,9 @@
 # of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 
+# Cygnus
+obj-$(CONFIG_ARCH_BCM_CYGNUS) +=  board_bcm_cygnus.o
+
 # BCM281XX
 obj-$(CONFIG_ARCH_BCM_281XX)	+= board_bcm281xx.o
 

arch/arm/mach-bcm/board_bcm_cygnus.c line 7

Quoted 13 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +7 -0

new file mode 100644
index 0000000..d67555a
@@ -0,0 +1,166 @@
+/*
+ * Copyright 2014 Broadcom Corporation.  All rights reserved.
+ *
+ * Unless you and Broadcom execute a separate written software license
+ * agreement governing use of this software, this software is licensed to you
+ * under the terms of the GNU General Public License version 2, available at
+ * http://www.broadcom.com/licenses/GPLv2.php (the "GPL").

Why don't we point at the gnu.org copy as we do elsewhere?

Quoted 7 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +7 -0

+ */
+
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/clocksource.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>

arch/arm/mach-bcm/board_bcm_cygnus.c line 20

Quoted 6 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +6 -0

+#include <asm/mach/arch.h>
+#include <asm/mach/map.h>
+#include <asm/proc-fns.h>
+#include <asm/hardware/cache-l2x0.h>
+
+#define CRMU_MAIL_BOX1      0x03024028

Please don't hard code addresses in board files.

arch/arm/mach-bcm/board_bcm_cygnus.c line 28

Quoted 9 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +8 -0

+#define CRMU_SOFT_RESET_CMD 0xFFFFFFFF
+
+/* CRU_RESET register */
+static void * __iomem crmu_mail_box1_reg;
+
+#ifdef CONFIG_NEON
+
+#define CRU_BASE                  0x1800e000

Another hard coded address that needs to go.

Quoted 8 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +8 -0

+#define CRU_SIZE                  0x34
+#define CRU_CONTROL_OFFSET        0x0
+#define CRU_PWRDWN_EN_OFFSET      0x4
+#define CRU_PWRDWN_STATUS_OFFSET  0x8
+#define CRU_NEON0_HW_RESET  6
+#define CRU_CLAMP_ON_NEON0  20
+#define CRU_PWRONIN_NEON0   21
+#define CRU_PWRONOUT_NEON0  21

arch/arm/mach-bcm/board_bcm_cygnus.c line 46

Quoted 10 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +10 -0

+#define CRU_PWROKIN_NEON0   22
+#define CRU_PWROKOUT_NEON0  22
+#define CRU_STATUS_DELAY_NS 500
+#define CRU_MAX_RETRY_COUNT 10
+#define CRU_RETRY_INTVL_US  1
+
+/* Power up the NEON/VFPv3 block. */
+static void bcm_cygnus_powerup_neon(void)
+{
+	void * __iomem cru_base = ioremap_nocache(CRU_BASE, CRU_SIZE);

Why not plain ioremap?

arch/arm/mach-bcm/board_bcm_cygnus.c line 49

Quoted 3 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +3 -0

+	u32 reg, i;
+
+	BUG_ON(!cru_base);

This seems a little extreme. Can;t we continue without NEON?

Quoted 16 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +15 -0

+
+	/* De-assert the neon hardware block reset */
+	reg = readl(cru_base + CRU_CONTROL_OFFSET);
+	reg &= ~(1 << CRU_NEON0_HW_RESET);
+	writel(reg, cru_base + CRU_CONTROL_OFFSET);
+
+	/* Assert the power ON register bit */
+	reg = readl(cru_base + CRU_PWRDWN_EN_OFFSET);
+	reg |= (1 << CRU_PWRONIN_NEON0);
+	writel(reg, cru_base + CRU_PWRDWN_EN_OFFSET);
+
+	/*
+	 * Wait up to 10 usec in 1 usec increments for the
+	 * status register to acknowledge the power ON assert
+	 */

arch/arm/mach-bcm/board_bcm_cygnus.c line 74

Quoted 10 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +10 -0

+	for (i = 0; i < CRU_MAX_RETRY_COUNT; i++) {
+		reg = readl(cru_base + CRU_PWRDWN_STATUS_OFFSET);
+		if (reg & CRU_PWRONOUT_NEON0)
+			break;
+
+		udelay(CRU_RETRY_INTVL_US);
+	}
+
+	if (i == CRU_MAX_RETRY_COUNT)
+		panic("NEON power ON register not acknowledged\n");

We can't just disable NEON if we fail to enable the HW block?

[...]

arch/arm/mach-bcm/board_bcm_cygnus.c line 125

Quoted 7 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +7 -0

+static void __init bcm_cygnus_timer_init(void)
+{
+	/* Initialize all clocks declared in device tree */
+	of_clk_init(NULL);
+
+	clocksource_of_init();
+}

If you take a look at time_init in arch/arm/kernel/time.c you'll see this is redundant.

Get rid of this.

arch/arm/mach-bcm/board_bcm_cygnus.c line 133

Quoted 8 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +8 -0

+
+static void __init bcm_cygnus_init(void)
+{
+	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+
+	l2x0_of_init(0, ~0UL);
+
+	crmu_mail_box1_reg = ioremap_nocache(CRMU_MAIL_BOX1, SZ_4);

Why not plain ioremap?

arch/arm/mach-bcm/board_bcm_cygnus.c line 134

Quoted 2 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +1 -0

+	BUG_ON(!crmu_mail_box1_reg);

We only use this for reboot later on, so do we need to blow up so spectacularly in this case?

Quoted 15 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +14 -0

+
+#ifdef CONFIG_NEON
+	bcm_cygnus_powerup_neon();
+#endif
+}
+
+/*
+ * Reset the system
+ */
+void bcm_cygnus_restart(enum reboot_mode mode, const char *cmd)
+{
+	/* Send reset command to M0 via Mailbox. */
+	writel(CRMU_SOFT_RESET_CMD, crmu_mail_box1_reg);
+	iounmap(crmu_mail_box1_reg);

arch/arm/mach-bcm/board_bcm_cygnus.c line 153

Quoted 5 lines

arch/arm/mach-bcm/board_bcm_cygnus.c new file +5 -0

+
+	/* Wait for M0 to reset the chip. */
+	while (1)
+		cpu_do_idle();
+}

This doesn't have to live in the machine descriptor. It could be a separate driver.

Thanks,
Mark.

The iProc clock driver controls PLL's common across iProc chips. The

cygnus driver controls cygnus specific features and variations.

Reviewed-by Ray Jui <[email protected]>

Tested-by Jonathan Richardson <[email protected]>

Reviewed-by JD (Jiandong) Zheng <[email protected]>

Signed-off-by Jonathan Richardson <[email protected]>

---
 drivers/clk/Makefile         |    1 +
 drivers/clk/bcm/Makefile     |    2 +
 drivers/clk/bcm/clk-cygnus.c | 1179 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/bcm/clk-iproc.c  |  446 ++++++++++++++++
 4 files changed, 1628 insertions(+)
 create mode 100644 drivers/clk/bcm/clk-cygnus.c
 create mode 100644 drivers/clk/bcm/clk-iproc.c

drivers/clk/bcm/clk-cygnus.c new file +18 -0

+       if (state)
+               val |= 1 << offset;
+       else
+               val &= ~(1 << offset);
+
+       writel(val, clkgate_reg);
+}
+
+/*
+ * Powers on/off the MIPI GENPLL using CRMU_PLL_AON_CTRL register.
+ *
+ * @param state true to power on PLL, false to power off
+ */
+static void cygnus_mipi_genpll_poweron(void __iomem *pll_ctrl_reg,
+       bool state)
+{
+       u32 val;
+       u32 pll_ldo_on = ((1 << ASIU_MIPI_GENPLL_PWRON_SHIFT) |

drivers/clk/bcm/clk-cygnus.c new file +20 -0

+       if (state) {
+               val |= pll_ldo_on;
+               val &= ~(1 << ASIU_MIPI_GENPLL_ISO_IN_SHIFT);
+       } else {
+               val &= ~pll_ldo_on;
+               val |= 1 << ASIU_MIPI_GENPLL_ISO_IN_SHIFT;
+       }
+
+       writel(val, pll_ctrl_reg);
+}
+
+/*
+ * Powers on/off the audio PLL using CRMU_PLL_AON_CTRL register.
+ *
+ * @param state true to power on PLL, false to power off
+ */
+static void cygnus_audio_genpll_poweron(void __iomem *pll_ctrl_reg,
+       bool state)
+{
+       u32 val;

drivers/clk/bcm/clk-cygnus.c new file +10 -0

+       if (state) {
+               val |= pll_ldo_on;
+               val &= ~(1 << ASIU_AUDIO_GENPLL_ISO_IN);
+       } else {
+               val &= ~pll_ldo_on;
+               val |= 1 << ASIU_AUDIO_GENPLL_ISO_IN;
+       }
+
+       writel(val, pll_ctrl_reg);
+}

drivers/clk/bcm/clk-cygnus.c new file +21 -0

+static __init struct clk *cygnus_clock_init(struct device_node *node,
+       const struct clk_ops *ops)
+{
+       u32 channel = 0;
+       struct clk *clk;
+       struct cygnus_clk *cygnus_clk;
+       const char *clk_name = node->name;
+       const char *parent_name;
+       struct clk_init_data init;
+       int rc;
+
+       pr_debug("Clock name %s\n", node->name);
+
+       of_property_read_u32(node, "channel", &channel);
+       cygnus_clk = kzalloc(sizeof(*cygnus_clk), GFP_KERNEL);
+       if (WARN_ON(!cygnus_clk))
+               return NULL;
+
+       cygnus_clk->state = CLK_DISABLED;
+
+       /* Read base address from device tree and map to virtual address. */

drivers/clk/bcm/clk-cygnus.c new file +37 -0

+
+       /*
+        * Internal divider is optional and used for PLL derived clocks with
+        * hardcoded dividers.
+        */
+       cygnus_clk->internal_div = CLK_RATE_NO_DIV;
+       of_property_read_u32(node, "div", &cygnus_clk->internal_div);
+
+       init.name = clk_name;
+       init.ops = ops;
+       init.flags = CLK_GET_RATE_NOCACHE;
+       parent_name = of_clk_get_parent_name(node, 0);
+       init.parent_names = &parent_name;
+       init.num_parents = 1;
+
+       cygnus_clk->hw.init = &init;
+
+       clk = clk_register(NULL, &cygnus_clk->hw);
+       if (WARN_ON(IS_ERR(clk)))
+               goto err_unmap;
+
+       rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+       if (WARN_ON(IS_ERR_VALUE(rc)))
+               goto err_unregister;
+
+       rc = clk_register_clkdev(clk, clk_name, NULL);
+       if (WARN_ON(IS_ERR_VALUE(rc)))
+               goto err_provider;
+
+       return clk;
+
+err_provider:
+       of_clk_del_provider(node);
+
+err_unregister:
+       clk_unregister(clk);
+

Quoted 1 line · Jonathan Richardson

The iProc clock driver controls PLL's common across iProc chips. The

Nit: s/PLL's/PLLs/ (we aren't greengrocers [1]).

Quoted 15 lines

cygnus driver controls cygnus specific features and variations.

Reviewed-by Ray Jui <[email protected]>

Tested-by Jonathan Richardson <[email protected]>

Reviewed-by JD (Jiandong) Zheng <[email protected]>

Signed-off-by Jonathan Richardson <[email protected]>

---
 drivers/clk/Makefile         |    1 +
 drivers/clk/bcm/Makefile     |    2 +
 drivers/clk/bcm/clk-cygnus.c | 1179 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/bcm/clk-iproc.c  |  446 ++++++++++++++++
 4 files changed, 1628 insertions(+)
 create mode 100644 drivers/clk/bcm/clk-cygnus.c
 create mode 100644 drivers/clk/bcm/clk-iproc.c

[...]

drivers/clk/bcm/clk-cygnus.c line 173

Quoted 11 lines

drivers/clk/bcm/clk-cygnus.c new file +11 -0

+/*
+ * Enable clocks controlled through the top clock gating control.
+ *
+ * @param state true = enable clock, false = disable clock
+ */
+static void cygnus_clkgate_enable(void __iomem *clkgate_reg,
+       enum cygnus_top_clk_gating_ctrl_offsets offset, bool state)
+{
+       u32 val = readl(clkgate_reg);
+
+       /* Enable or disable the clock. */

This function is misnamed if it does both, and 'state' is not a very descriptive name.

Quoted 24 lines

drivers/clk/bcm/clk-cygnus.c new file +23 -0

+       if (state)
+               val |= 1 << offset;
+       else
+               val &= ~(1 << offset);
+
+       writel(val, clkgate_reg);
+}
+
+/*
+ * Powers on/off the MIPI GENPLL using CRMU_PLL_AON_CTRL register.
+ *
+ * @param state true to power on PLL, false to power off
+ */
+static void cygnus_mipi_genpll_poweron(void __iomem *pll_ctrl_reg,
+       bool state)
+{
+       u32 val;
+       u32 pll_ldo_on = ((1 << ASIU_MIPI_GENPLL_PWRON_SHIFT) |
+               (1 << ASIU_MIPI_GENPLL_PWRON_PLL_SHIFT) |
+               (1 << ASIU_MIPI_GENPLL_PWRON_BG_SHIFT)  |
+               (1 << ASIU_MIPI_GENPLL_PWRON_LDO_SHIFT));
+
+       val = readl(pll_ctrl_reg);

drivers/clk/bcm/clk-cygnus.c line 201

Quoted 5 lines

drivers/clk/bcm/clk-cygnus.c new file +5 -0

+
+       /*
+        * Set PLL on/off. Set input isolation mode to 1 when disabled, 0 when
+        * enabled.
+        */

As with cygnus_clkgate_enable, this function is misnamed and 'state' is a confusing parameter name.

Quoted 25 lines

drivers/clk/bcm/clk-cygnus.c new file +24 -0

+       if (state) {
+               val |= pll_ldo_on;
+               val &= ~(1 << ASIU_MIPI_GENPLL_ISO_IN_SHIFT);
+       } else {
+               val &= ~pll_ldo_on;
+               val |= 1 << ASIU_MIPI_GENPLL_ISO_IN_SHIFT;
+       }
+
+       writel(val, pll_ctrl_reg);
+}
+
+/*
+ * Powers on/off the audio PLL using CRMU_PLL_AON_CTRL register.
+ *
+ * @param state true to power on PLL, false to power off
+ */
+static void cygnus_audio_genpll_poweron(void __iomem *pll_ctrl_reg,
+       bool state)
+{
+       u32 val;
+       u32 pll_ldo_on = ((1 << ASIU_AUDIO_GENPLL_PWRON_PLL_SHIFT) |
+               (1 << ASIU_AUDIO_GENPLL_PWRON_BG_SHIFT) |
+               (1 << ASIU_AUDIO_GENPLL_PWRON_LDO_SHIFT));
+

drivers/clk/bcm/clk-cygnus.c line 201

Quoted 6 lines

drivers/clk/bcm/clk-cygnus.c new file +6 -0

+       val = readl(pll_ctrl_reg);
+
+       /*
+        * Set PLL on/off. Set input isolation mode to 1 when disabled, 0 when
+        * enabled.
+        */

Misnamed function, confusing parameter name.

Quoted 11 lines

drivers/clk/bcm/clk-cygnus.c new file +10 -0

+       if (state) {
+               val |= pll_ldo_on;
+               val &= ~(1 << ASIU_AUDIO_GENPLL_ISO_IN);
+       } else {
+               val &= ~pll_ldo_on;
+               val |= 1 << ASIU_AUDIO_GENPLL_ISO_IN;
+       }
+
+       writel(val, pll_ctrl_reg);
+}

[...]

Quoted 25 lines

drivers/clk/bcm/clk-cygnus.c new file +25 -0

+static __init struct clk *cygnus_clock_init(struct device_node *node,
+       const struct clk_ops *ops)
+{
+       u32 channel = 0;
+       struct clk *clk;
+       struct cygnus_clk *cygnus_clk;
+       const char *clk_name = node->name;
+       const char *parent_name;
+       struct clk_init_data init;
+       int rc;
+
+       pr_debug("Clock name %s\n", node->name);
+
+       of_property_read_u32(node, "channel", &channel);
+       cygnus_clk = kzalloc(sizeof(*cygnus_clk), GFP_KERNEL);
+       if (WARN_ON(!cygnus_clk))
+               return NULL;
+
+       cygnus_clk->state = CLK_DISABLED;
+
+       /* Read base address from device tree and map to virtual address. */
+       cygnus_clk->regs_base = of_iomap(node, CYGNUS_CLK_BASE_REG);
+       if (WARN_ON(!cygnus_clk->regs_base))
+               goto err_alloc;
+

drivers/clk/bcm/clk-cygnus.c line 647

Quoted 6 lines

drivers/clk/bcm/clk-cygnus.c new file +6 -0

+       /* Read optional base addresses for PLL control and clock gating. */
+       cygnus_clk->clock_gate_ctrl_reg = of_iomap(node,
+               CYGNUS_CLK_GATE_CTRL_REG);
+       cygnus_clk->pll_ctrl_reg = of_iomap(node, CYGNUS_PLL_CTRL_REG);
+
+       of_property_read_u32(node, "channel", &channel);

Why do we read this twice?

drivers/clk/bcm/clk-cygnus.c line 649

Quoted 3 lines

drivers/clk/bcm/clk-cygnus.c new file +2 -0

+       cygnus_clk->chan = channel;
+       of_property_read_string(node, "clock-output-names", &clk_name);

What happens if this is missing from the dt?

Quoted 38 lines

drivers/clk/bcm/clk-cygnus.c new file +37 -0

+
+       /*
+        * Internal divider is optional and used for PLL derived clocks with
+        * hardcoded dividers.
+        */
+       cygnus_clk->internal_div = CLK_RATE_NO_DIV;
+       of_property_read_u32(node, "div", &cygnus_clk->internal_div);
+
+       init.name = clk_name;
+       init.ops = ops;
+       init.flags = CLK_GET_RATE_NOCACHE;
+       parent_name = of_clk_get_parent_name(node, 0);
+       init.parent_names = &parent_name;
+       init.num_parents = 1;
+
+       cygnus_clk->hw.init = &init;
+
+       clk = clk_register(NULL, &cygnus_clk->hw);
+       if (WARN_ON(IS_ERR(clk)))
+               goto err_unmap;
+
+       rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+       if (WARN_ON(IS_ERR_VALUE(rc)))
+               goto err_unregister;
+
+       rc = clk_register_clkdev(clk, clk_name, NULL);
+       if (WARN_ON(IS_ERR_VALUE(rc)))
+               goto err_provider;
+
+       return clk;
+
+err_provider:
+       of_clk_del_provider(node);
+
+err_unregister:
+       clk_unregister(clk);
+

drivers/clk/bcm/clk-cygnus.c line 696

Quoted 10 lines

drivers/clk/bcm/clk-cygnus.c new file +10 -0

+err_unmap:
+       iounmap(cygnus_clk->regs_base);
+       iounmap(cygnus_clk->clock_gate_ctrl_reg);
+       iounmap(cygnus_clk->pll_ctrl_reg);
+
+err_alloc:
+       kfree(cygnus_clk);
+
+       return NULL;
+}

Thanks,
Mark.

[1] http://en.wikipedia.org/wiki/Apostrophe#Superfluous_apostrophes_.28.22greengrocers.27_apostrophes.22.29
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi,

This patchset contains initial support for Broadcom's Cygnus SoC based on our iProc architecture. Initial support is minimal and includes just the mach platform code, clock driver, and a basic device tree configuration. Peripheral drivers will be submitted soon, as will device tree configurations for other Cygnus board variants.

Quoted 7 lines · Jonathan Richardson

Hi,

This patchset contains initial support for Broadcom's Cygnus SoC based on our iProc architecture. Initial support is minimal and includes just the mach platform code, clock driver, and a basic device tree configuration. Peripheral drivers will be submitted soon, as will device tree configurations for other Cygnus board variants.

This SoC looks similar to the BCM5301X (Northstar) SoCs even some cores are at the same memory addresses. Does this SoC use an AXI bus with Broadcom Plugins at address 0x18000000 ?

The BCM5301X (Northstar) SoCs also has an iProc Clock Control Unit and I will try to use your driver for that SoC.

Is Broadcom Cygnus similar to BCM563XX or is it actually the same SoC?

Do you have some description of the hardware features of this SoC? Does it have PCIe, or Ethernet?

From the link above, it has Ethernet for sure, and certainly PCIe since

Quoted 16 lines · Hauke Mehrtens

Quoted 7 lines · Jonathan Richardson

Hi,

This patchset contains initial support for Broadcom's Cygnus SoC based on our iProc architecture. Initial support is minimal and includes just the mach platform code, clock driver, and a basic device tree configuration. Peripheral drivers will be submitted soon, as will device tree configurations for other Cygnus board variants.

This SoC looks similar to the BCM5301X (Northstar) SoCs even some cores are at the same memory addresses. Does this SoC use an AXI bus with Broadcom Plugins at address 0x18000000 ?

The BCM5301X (Northstar) SoCs also has an iProc Clock Control Unit and I will try to use your driver for that SoC.

Quoted 1 line

Is Broadcom Cygnus similar to BCM563XX or is it actually the same SoC?

According to this link:
http://lists.denx.de/pipermail/u-boot/2014-August/186086.html

these SoCs, in particular BCM58622 would belong to the StrataGX network processors:
http://www.broadcom.com/products/Processors/Home-and-Small-Business/BCM5862X-Series.

Pin compatible and software compatible products with previous generation BCM5301x products enable single design with easy upgradeable path

Quoted 2 lines

Do you have some description of the hardware features of this SoC? Does it have PCIe, or Ethernet?

From the link above, it has Ethernet for sure, and certainly PCIe since the Wi-Fi chips mentioned in the link above are PCIe chips.

It would be really good if all the work you and Rafal did was usable for the Cygnus SoCs.

Quoted 10 lines · Florian Fainelli

Quoted 9 lines · Hauke Mehrtens

Quoted 7 lines · Jonathan Richardson

Hi,

This patchset contains initial support for Broadcom's Cygnus SoC based on our iProc architecture. Initial support is minimal and includes just the mach platform code, clock driver, and a basic device tree configuration. Peripheral drivers will be submitted soon, as will device tree configurations for other Cygnus board variants.

Quoted 8 lines

Quoted 8 lines

This SoC looks similar to the BCM5301X (Northstar) SoCs even some cores are at the same memory addresses. Does this SoC use an AXI bus with Broadcom Plugins at address 0x18000000 ?

The BCM5301X (Northstar) SoCs also has an iProc Clock Control Unit and I will try to use your driver for that SoC.

Is Broadcom Cygnus similar to BCM563XX or is it actually the same SoC?

Quoted 7 lines

According to this link:
http://lists.denx.de/pipermail/u-boot/2014-August/186086.html

these SoCs, in particular BCM58622 would belong to the StrataGX network processors:
http://www.broadcom.com/products/Processors/Home-and-Small-Business/BCM5862X-Series.

Broadcom claims that this SoC is pin and software compatible to BCM5301X:

Quoted 6 lines

Pin compatible and software compatible products with previous generation BCM5301x products enable single design with easy upgradeable path

Quoted 2 lines

Do you have some description of the hardware features of this SoC? Does it have PCIe, or Ethernet?

Quoted 6 lines

From the link above, it has Ethernet for sure, and certainly PCIe since
the Wi-Fi chips mentioned in the link above are PCIe chips.

It would be really good if all the work you and Rafal did was usable for the Cygnus SoCs.

I also hope you can use much of the code and improve it. ;-)

Hauke

Hi,

This patchset contains initial support for Broadcom's Cygnus SoC based on our iProc architecture. Initial support is minimal and includes just the mach platform code, clock driver, and a basic device tree configuration. Peripheral drivers will be submitted soon, as will device tree configurations for other Cygnus board variants.

Changes from v1:
- Address code review comments as per previous responses.
- Copyright headers updated to remove Broadcom URL.
- mach platform code still contains hard coded adresses. These address are the same for all Cygnus variants. Could you please provide guidance on where they should go if you would still like them changed.

Thanks,
Jon

Jonathan Richardson (6):
ARM: cygnus: Initial support for Broadcom Cygnus SoC
clk: Clock driver support for Broadcom Cygnus SoC
dt-bindings: Document Broadcom Cygnus SoC and clock driver
ARM: dts: Enable Broadcom Cygnus SoC
ARM: cygnus defconfig : Initial defconfig for Broadcom Cygnus SoC
MAINTAINERS: Entry for Cygnus/iproc arm architecture and clock
drivers

  Documentation/devicetree/bindings/arm/cygnus.txt   |   12 +
  .../devicetree/bindings/clock/clk-cygnus.txt       |  121 ++
  .../devicetree/bindings/clock/clk-iproc.txt        |   48 +
  MAINTAINERS                                        |   21 +
  arch/arm/boot/dts/Makefile                         |    1 +
  arch/arm/boot/dts/bcm-cygnus.dtsi                  |  349 ++++++
  arch/arm/boot/dts/bcm911360_entphn.dts             |   22 +
  arch/arm/configs/bcm_cygnus_defconfig              |  223 ++++
  arch/arm/mach-bcm/Kconfig                          |   31 +
  arch/arm/mach-bcm/Makefile                         |    3 +
  arch/arm/mach-bcm/bcm_cygnus.c                     |  166 +++
  drivers/clk/Makefile                               |    1 +
  drivers/clk/bcm/Makefile                           |    2 +
  drivers/clk/bcm/clk-cygnus.c                       | 1186 ++++++++++++++++++++
  drivers/clk/bcm/clk-iproc.c                        |  451 ++++++++
  15 files changed, 2637 insertions(+)

create mode 100644 Documentation/devicetree/bindings/arm/cygnus.txt create mode 100644 Documentation/devicetree/bindings/clock/clk-cygnus.txt create mode 100644 Documentation/devicetree/bindings/clock/clk-iproc.txt create mode 100644 arch/arm/boot/dts/bcm-cygnus.dtsi
create mode 100644 arch/arm/boot/dts/bcm911360_entphn.dts create mode 100644 arch/arm/configs/bcm_cygnus_defconfig create mode 100644 arch/arm/mach-bcm/bcm_cygnus.c
create mode 100644 drivers/clk/bcm/clk-cygnus.c
create mode 100644 drivers/clk/bcm/clk-iproc.c

Hi Matt/Christian (and others),

We would like to move forward in getting our base Cygnus support into the mainline kernel. From there we will be able to contribute additional drivers.

Is somebody willing to accept this pull request to get this patchset accepted by the open source community?

Regards,
Scott

Quoted 49 lines · Jonathan Richardson

Hi,

This patchset contains initial support for Broadcom's Cygnus SoC based on our
iProc architecture. Initial support is minimal and includes just the mach platform code, clock driver, and a basic device tree configuration. Peripheral
drivers will be submitted soon, as will device tree configurations for other
Cygnus board variants.

Changes from v1:
- Address code review comments as per previous responses.
- Copyright headers updated to remove Broadcom URL.
- mach platform code still contains hard coded adresses. These
address are
the same for all Cygnus variants. Could you please provide
guidance on where
they should go if you would still like them changed.

Thanks,
Jon

Jonathan Richardson (6):
ARM: cygnus: Initial support for Broadcom Cygnus SoC
clk: Clock driver support for Broadcom Cygnus SoC
dt-bindings: Document Broadcom Cygnus SoC and clock driver
ARM: dts: Enable Broadcom Cygnus SoC
ARM: cygnus defconfig : Initial defconfig for Broadcom Cygnus SoC
MAINTAINERS: Entry for Cygnus/iproc arm architecture and clock
drivers

  Documentation/devicetree/bindings/arm/cygnus.txt   |   12 +
  .../devicetree/bindings/clock/clk-cygnus.txt       |  121 ++
  .../devicetree/bindings/clock/clk-iproc.txt        |   48 +
  MAINTAINERS                                        |   21 +
  arch/arm/boot/dts/Makefile                         |    1 +
  arch/arm/boot/dts/bcm-cygnus.dtsi                  |  349 ++++++
  arch/arm/boot/dts/bcm911360_entphn.dts             |   22 +
  arch/arm/configs/bcm_cygnus_defconfig              |  223 ++++
  arch/arm/mach-bcm/Kconfig                          |   31 +
  arch/arm/mach-bcm/Makefile                         |    3 +
  arch/arm/mach-bcm/bcm_cygnus.c                     |  166 +++
  drivers/clk/Makefile                               |    1 +
  drivers/clk/bcm/Makefile                           |    2 +
  drivers/clk/bcm/clk-cygnus.c                       | 1186
++++++++++++++++++++
  drivers/clk/bcm/clk-iproc.c                        |  451 ++++++++
  15 files changed, 2637 insertions(+)

Quoted 12 lines

create mode 100644 Documentation/devicetree/bindings/arm/cygnus.txt create mode 100644
Documentation/devicetree/bindings/clock/clk-cygnus.txt
create mode 100644
Documentation/devicetree/bindings/clock/clk-iproc.txt
create mode 100644 arch/arm/boot/dts/bcm-cygnus.dtsi create mode 100644 arch/arm/boot/dts/bcm911360_entphn.dts create mode 100644 arch/arm/configs/bcm_cygnus_defconfig create mode 100644 arch/arm/mach-bcm/bcm_cygnus.c
create mode 100644 drivers/clk/bcm/clk-cygnus.c
create mode 100644 drivers/clk/bcm/clk-iproc.c

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Scott,

Quoted 8 lines · Scott Branden

Hi Matt/Christian (and others),

We would like to move forward in getting our base Cygnus support into the mainline kernel. From there we will be able to contribute additional drivers.

Is somebody willing to accept this pull request to get this patchset accepted by the open source community?

I am fine with taking the ARM patches and putting them at
http://github/brcm/linux.git, with C code in cygnus/board and the Device Tree files in cygnus/dt for instance?

The clock driver patch will have to go through Mike Turquette's tree unless he wants to delegate that one.

Quoted 74 lines

Regards,
Scott

Quoted 49 lines · Jonathan Richardson

Hi,

This patchset contains initial support for Broadcom's Cygnus SoC based on our
iProc architecture. Initial support is minimal and includes just the mach platform code, clock driver, and a basic device tree configuration. Peripheral
drivers will be submitted soon, as will device tree configurations for other
Cygnus board variants.

Changes from v1:
- Address code review comments as per previous responses.
- Copyright headers updated to remove Broadcom URL.
- mach platform code still contains hard coded adresses. These
address are
the same for all Cygnus variants. Could you please provide
guidance on where
they should go if you would still like them changed.

Thanks,
Jon

Jonathan Richardson (6):
ARM: cygnus: Initial support for Broadcom Cygnus SoC
clk: Clock driver support for Broadcom Cygnus SoC
dt-bindings: Document Broadcom Cygnus SoC and clock driver
ARM: dts: Enable Broadcom Cygnus SoC
ARM: cygnus defconfig : Initial defconfig for Broadcom Cygnus SoC
MAINTAINERS: Entry for Cygnus/iproc arm architecture and clock
drivers

  Documentation/devicetree/bindings/arm/cygnus.txt   |   12 +
  .../devicetree/bindings/clock/clk-cygnus.txt       |  121 ++
  .../devicetree/bindings/clock/clk-iproc.txt        |   48 +
  MAINTAINERS                                        |   21 +
  arch/arm/boot/dts/Makefile                         |    1 +
  arch/arm/boot/dts/bcm-cygnus.dtsi                  |  349 ++++++
  arch/arm/boot/dts/bcm911360_entphn.dts             |   22 +
  arch/arm/configs/bcm_cygnus_defconfig              |  223 ++++
  arch/arm/mach-bcm/Kconfig                          |   31 +
  arch/arm/mach-bcm/Makefile                         |    3 +
  arch/arm/mach-bcm/bcm_cygnus.c                     |  166 +++
  drivers/clk/Makefile                               |    1 +
  drivers/clk/bcm/Makefile                           |    2 +
  drivers/clk/bcm/clk-cygnus.c                       | 1186
++++++++++++++++++++
  drivers/clk/bcm/clk-iproc.c                        |  451 ++++++++
  15 files changed, 2637 insertions(+)

Quoted 12 lines

create mode 100644 Documentation/devicetree/bindings/arm/cygnus.txt create mode 100644
Documentation/devicetree/bindings/clock/clk-cygnus.txt
create mode 100644
Documentation/devicetree/bindings/clock/clk-iproc.txt
create mode 100644 arch/arm/boot/dts/bcm-cygnus.dtsi create mode 100644 arch/arm/boot/dts/bcm911360_entphn.dts create mode 100644 arch/arm/configs/bcm_cygnus_defconfig create mode 100644 arch/arm/mach-bcm/bcm_cygnus.c
create mode 100644 drivers/clk/bcm/clk-cygnus.c
create mode 100644 drivers/clk/bcm/clk-iproc.c

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Scott, Jonathan,

Quoted 16 lines · Florian Fainelli

Hi Scott,

Quoted 8 lines · Scott Branden

Hi Matt/Christian (and others),

We would like to move forward in getting our base Cygnus support into the mainline kernel. From there we will be able to contribute additional drivers.

Is somebody willing to accept this pull request to get this patchset accepted by the open source community?

I am fine with taking the ARM patches and putting them at
http://github/brcm/linux.git, with C code in cygnus/board and the Device Tree files in cygnus/dt for instance?

Quoted 2 lines

The clock driver patch will have to go through Mike Turquette's tree unless he wants to delegate that one.

Could you resend patch 3 without the clock binding, patch 4 without the clocks node, and patch 6 without the drivers/clk changes? Once you do that, I can assemble these patches quickly and send a pull request which does only contain ARM SoC and DTS changes, and you can get the clock driver to be reviewed separately by its maintainer?

Thanks!

Quoted 75 lines

Quoted 74 lines

Regards,
Scott

Quoted 49 lines · Jonathan Richardson

Hi,

This patchset contains initial support for Broadcom's Cygnus SoC based on our
iProc architecture. Initial support is minimal and includes just the mach platform code, clock driver, and a basic device tree configuration. Peripheral
drivers will be submitted soon, as will device tree configurations for other
Cygnus board variants.

Changes from v1:
- Address code review comments as per previous responses.
- Copyright headers updated to remove Broadcom URL.
- mach platform code still contains hard coded adresses. These
address are
the same for all Cygnus variants. Could you please provide
guidance on where
they should go if you would still like them changed.

Thanks,
Jon

Jonathan Richardson (6):
ARM: cygnus: Initial support for Broadcom Cygnus SoC
clk: Clock driver support for Broadcom Cygnus SoC
dt-bindings: Document Broadcom Cygnus SoC and clock driver
ARM: dts: Enable Broadcom Cygnus SoC
ARM: cygnus defconfig : Initial defconfig for Broadcom Cygnus SoC
MAINTAINERS: Entry for Cygnus/iproc arm architecture and clock
drivers

  Documentation/devicetree/bindings/arm/cygnus.txt   |   12 +
  .../devicetree/bindings/clock/clk-cygnus.txt       |  121 ++
  .../devicetree/bindings/clock/clk-iproc.txt        |   48 +
  MAINTAINERS                                        |   21 +
  arch/arm/boot/dts/Makefile                         |    1 +
  arch/arm/boot/dts/bcm-cygnus.dtsi                  |  349 ++++++
  arch/arm/boot/dts/bcm911360_entphn.dts             |   22 +
  arch/arm/configs/bcm_cygnus_defconfig              |  223 ++++
  arch/arm/mach-bcm/Kconfig                          |   31 +
  arch/arm/mach-bcm/Makefile                         |    3 +
  arch/arm/mach-bcm/bcm_cygnus.c                     |  166 +++
  drivers/clk/Makefile                               |    1 +
  drivers/clk/bcm/Makefile                           |    2 +
  drivers/clk/bcm/clk-cygnus.c                       | 1186
++++++++++++++++++++
  drivers/clk/bcm/clk-iproc.c                        |  451 ++++++++
  15 files changed, 2637 insertions(+)

Quoted 12 lines

create mode 100644 Documentation/devicetree/bindings/arm/cygnus.txt create mode 100644
Documentation/devicetree/bindings/clock/clk-cygnus.txt
create mode 100644
Documentation/devicetree/bindings/clock/clk-iproc.txt
create mode 100644 arch/arm/boot/dts/bcm-cygnus.dtsi create mode 100644 arch/arm/boot/dts/bcm911360_entphn.dts create mode 100644 arch/arm/configs/bcm_cygnus_defconfig create mode 100644 arch/arm/mach-bcm/bcm_cygnus.c
create mode 100644 drivers/clk/bcm/clk-cygnus.c
create mode 100644 drivers/clk/bcm/clk-iproc.c

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 19 lines · Florian Fainelli

Scott, Jonathan,

Quoted 16 lines · Florian Fainelli

Hi Scott,

Quoted 8 lines · Scott Branden

Hi Matt/Christian (and others),

We would like to move forward in getting our base Cygnus support into the mainline kernel. From there we will be able to contribute additional drivers.

Is somebody willing to accept this pull request to get this patchset accepted by the open source community?

I am fine with taking the ARM patches and putting them at
http://github/brcm/linux.git, with C code in cygnus/board and the Device Tree files in cygnus/dt for instance?

Quoted 8 lines

Quoted 2 lines

The clock driver patch will have to go through Mike Turquette's tree unless he wants to delegate that one.

Could you resend patch 3 without the clock binding, patch 4 without the clocks node, and patch 6 without the drivers/clk changes? Once you do that, I can assemble these patches quickly and send a pull request which does only contain ARM SoC and DTS changes, and you can get the clock driver to be reviewed separately by its maintainer?

The serial driver needs a clock so probably better not to pull out the clock nodes. It's part of the core functionality we'd like in the initial Cygnus support. I didn't hear from Mike (cc'd), though Mark reviewed it.

If config is called when the pwm is disabled and there is nothing to do, the while loop to calculate duty cycle and period doesn't need to be done. The function now just returns if the pwm state is disabled.

drivers/pwm/pwm-bcm-kona.c +7 -2

@@ -207,13 +240,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
        struct kona_pwmc *kp = to_kona_pwmc(chip);
        unsigned int chan = pwm->hwpwm;
+       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+       /* Set smooth type to 0 and disable */
+       value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+       writel(value, kp->base + PWM_CONTROL_OFFSET);
 
        /* Simulate a disable by configuring for zero duty */
        writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
-       kona_pwmc_apply_settings(kp, chan);
+       writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
 
-       /* Wait for waveform to settle before gating off the clock */

Quoted 3 lines · Jonathan Richardson

If config is called when the pwm is disabled and there is nothing to do, the while loop to calculate duty cycle and period doesn't need to be done. The function now just returns if the pwm state is disabled.

It doesn't take long to figure out whether the duty and period are achievable.

If the caller specifies bad settings, why not return an error immediately?

Quoted 18 lines

drivers/pwm/pwm-bcm-kona.c +7 -2

@@ -207,13 +240,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
        struct kona_pwmc *kp = to_kona_pwmc(chip);
        unsigned int chan = pwm->hwpwm;
+       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+       /* Set smooth type to 0 and disable */
+       value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+       writel(value, kp->base + PWM_CONTROL_OFFSET);
 
        /* Simulate a disable by configuring for zero duty */
        writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
-       kona_pwmc_apply_settings(kp, chan);
+       writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
 
-       /* Wait for waveform to settle before gating off the clock */

drivers/pwm/pwm-bcm-kona.c static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) · line 262

Quoted 10 lines

drivers/pwm/pwm-bcm-kona.c +6 -1

-       ndelay(400);
+       /* Set prescale to 0 for this channel */
+       value = readl(kp->base + PRESCALE_OFFSET);
+       value &= ~PRESCALE_MASK(chan);
+       writel(value, kp->base + PRESCALE_OFFSET);
+
+       kona_pwmc_apply_settings(kp, chan);
 
        clk_disable_unprepare(kp->clk);
 }

I've mentioned this before but I will say it again, when the smooth and trigger bit are both low, the output is constant high.

If you look at the PWM output on a scope you will see it go high for 400 ns during your disable even if the duty prior to the disable was zero.

How are you testing your proposed changes?

Thanks,
Tim Kryger

The config procedure doesn't follow the spec which periodically results in failing to enable the output signal. This happens one in ten or twenty attempts. Following the spec and adding a 400ns delay in the appropriate locations resolves this problem. It also ensures that the signal transition is smooth.

If config is called when the pwm is disabled and there is nothing to do, the while loop to calculate duty cycle and period doesn't need to be done. The function now just returns if the pwm state is disabled.

The disable procedure now also follows the spec to ensure a smooth transition. Not following the spec can cause non-smooth transitions.

The enable procedure now clears the enabled bit if enabling failed. Enabling can fail if an invalid duty cycle and period is set. This prevents the sysfs interface from reporting the pwm is enabled after a failed call to enable.

Reviewed-by Arun Ramamurthy <[email protected]>

Reviewed-by Scott Branden <[email protected]>

Tested-by Scott Branden <[email protected]>

Signed-off-by Jonathan Richardson <[email protected]>

---
 drivers/pwm/pwm-bcm-kona.c |   91 ++++++++++++++++++++++++++++++++------------
 1 file changed, 67 insertions(+), 24 deletions(-)

drivers/pwm/pwm-bcm-kona.c +5 -4

index 02bc048..a831bb2 100644
@@ -80,15 +80,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 {
        unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-       /* Clear trigger bit but set smooth bit to maintain old output */
-       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
-       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
-       writel(value, kp->base + PWM_CONTROL_OFFSET);
+       /*
+        * There must be a min 400ns delay between clearing enable and setting
+        * it. Failing to do this may result in no PWM signal.
+        */
+       ndelay(400);

drivers/pwm/pwm-bcm-kona.c +34 -12

@@ -121,31 +128,60 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
                dc = div64_u64(val, div);
 
                /* If duty_ns or period_ns are not achievable then return */
-               if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+               if (pc < PERIOD_COUNT_MIN) {
+                       dev_warn(chip->dev,
+                               "%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
+                               __func__, chan, period_ns, pc, prescale);
                        return -EINVAL;
+               }
+
+               /* If duty_ns is not achievable then return */
+               if (dc < DUTY_CYCLE_HIGH_MIN) {
+                       if (0 != duty_ns) {
+                               dev_warn(chip->dev,
+                                       "%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
+                                       __func__, chan, duty_ns, dc, prescale);
+                       }
+                       return -EINVAL;
+               }
 
                /* If pc and dc are in bounds, the calculation is done */
                if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
                        break;
 
                /* Otherwise, increase prescale and recalculate pc and dc */
-               if (++prescale > PRESCALE_MAX)
+               if (++prescale > PRESCALE_MAX) {
+                       dev_warn(chip->dev,
+                               "%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
+                               __func__, chan, prescale, PRESCALE_MAX,
+                               period_ns, duty_ns);
                        return -EINVAL;
+               }
        }
 
-       /* If the PWM channel is enabled, write the settings to the HW */
-       if (test_bit(PWMF_ENABLED, &pwm->flags)) {
-               value = readl(kp->base + PRESCALE_OFFSET);
-               value &= ~PRESCALE_MASK(chan);
-               value |= prescale << PRESCALE_SHIFT(chan);
-               writel(value, kp->base + PRESCALE_OFFSET);
+       dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
+               chan, pc, dc, prescale);
 
-               writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
+       value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-               writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+       /*
+        * Clear trigger bit but set smooth bit to maintain old
+        * output.
+        */
+       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+       writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-               kona_pwmc_apply_settings(kp, chan);
-       }
+       value = readl(kp->base + PRESCALE_OFFSET);
+       value &= ~PRESCALE_MASK(chan);
+       value |= prescale << PRESCALE_SHIFT(chan);

drivers/pwm/pwm-bcm-kona.c +1 -0

@@ -190,12 +221,14 @@ static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
        ret = clk_prepare_enable(kp->clk);
        if (ret < 0) {
+               clear_bit(PWMF_ENABLED, &pwm->flags);

drivers/pwm/pwm-bcm-kona.c +6 -0

@@ -207,13 +240,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
        struct kona_pwmc *kp = to_kona_pwmc(chip);
        unsigned int chan = pwm->hwpwm;
+       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+       /* Set smooth type to 1 and disable */
+       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+       writel(value, kp->base + PWM_CONTROL_OFFSET);
 
        /* Simulate a disable by configuring for zero duty */

drivers/pwm/pwm-bcm-kona.c +2 -1

+
+       kona_pwmc_apply_settings(kp, chan);
 
        clk_disable_unprepare(kp->clk);
 }
--

1.7.9.5

Quoted 5 lines · Jonathan Richardson

The config procedure doesn't follow the spec which periodically results in failing to enable the output signal. This happens one in ten or twenty attempts. Following the spec and adding a 400ns delay in the appropriate locations resolves this problem. It also ensures that the signal transition is smooth.

This patch does not result in smooth transitions.

Please ensure that your commit message reflects the code.

Quoted 14 lines

If config is called when the pwm is disabled and there is nothing to do, the while loop to calculate duty cycle and period doesn't need to be done. The function now just returns if the pwm state is disabled.

The disable procedure now also follows the spec to ensure a smooth transition. Not following the spec can cause non-smooth transitions.

The enable procedure now clears the enabled bit if enabling failed. Enabling can fail if an invalid duty cycle and period is set. This prevents the sysfs interface from reporting the pwm is enabled after a failed call to enable.

Quoted 4 lines

Reviewed-by Arun Ramamurthy <[email protected]>

Reviewed-by Scott Branden <[email protected]>

Tested-by Scott Branden <[email protected]>

Signed-off-by Jonathan Richardson <[email protected]>

Normally when people send a multi-part patch series they include a cover letter that describes the overall purpose of the changes with a description of changes introduced in each revision of the series.

Please follow this convention.

Quoted 22 lines

---
 drivers/pwm/pwm-bcm-kona.c |   91 ++++++++++++++++++++++++++++++++------------
 1 file changed, 67 insertions(+), 24 deletions(-)

drivers/pwm/pwm-bcm-kona.c +5 -4

index 02bc048..a831bb2 100644
@@ -80,15 +80,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 {
        unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-       /* Clear trigger bit but set smooth bit to maintain old output */
-       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
-       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
-       writel(value, kp->base + PWM_CONTROL_OFFSET);
+       /*
+        * There must be a min 400ns delay between clearing enable and setting
+        * it. Failing to do this may result in no PWM signal.
+        */
+       ndelay(400);

drivers/pwm/pwm-bcm-kona.c static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan) · line 97

Quoted 10 lines

drivers/pwm/pwm-bcm-kona.c +3 -0

 
        /* Set trigger bit and clear smooth bit to apply new settings */
        value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
        value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
        writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+       /* PWMOUT_ENABLE must be held high for at least 400 ns. */
+       ndelay(400);
 }
 

Can you try to get a better understanding of what these timing requirements actually are?

When I wrote this driver, the documentation simply stated that if the trigger bit didn't stay high for 400 ns then the new settings would not get applied.

It wasn't essential that we wait 400 ns here because the only time when the trigger bit would be lowered is when we were about to load even newer settings which meant we didn't care if the previous settings were lost.

Quoted 6 lines

drivers/pwm/pwm-bcm-kona.c +0 -0

 static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,

@@ -99,6 +103,9 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
        unsigned long prescale = PRESCALE_MIN, pc, dc;
        unsigned int value, chan = pwm->hwpwm;
 

drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 111

Quoted 6 lines

drivers/pwm/pwm-bcm-kona.c +3 -0

+       if (!test_bit(PWMF_ENABLED, &pwm->flags))
+               return 0;
+
        /*
         * Find period count, duty count and prescale to suit duty_ns and
         * period_ns. This is done according to formulas described below:

Like I said last time, this is wrong. You need to sanity check the requested period and duty cycle here.

You can't just return success immediately when the channel isn't enabled.

Quoted 72 lines

drivers/pwm/pwm-bcm-kona.c +41 -12

@@ -121,31 +128,60 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
                dc = div64_u64(val, div);
 
                /* If duty_ns or period_ns are not achievable then return */
-               if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+               if (pc < PERIOD_COUNT_MIN) {
+                       dev_warn(chip->dev,
+                               "%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
+                               __func__, chan, period_ns, pc, prescale);
                        return -EINVAL;
+               }
+
+               /* If duty_ns is not achievable then return */
+               if (dc < DUTY_CYCLE_HIGH_MIN) {
+                       if (0 != duty_ns) {
+                               dev_warn(chip->dev,
+                                       "%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
+                                       __func__, chan, duty_ns, dc, prescale);
+                       }
+                       return -EINVAL;
+               }
 
                /* If pc and dc are in bounds, the calculation is done */
                if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
                        break;
 
                /* Otherwise, increase prescale and recalculate pc and dc */
-               if (++prescale > PRESCALE_MAX)
+               if (++prescale > PRESCALE_MAX) {
+                       dev_warn(chip->dev,
+                               "%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
+                               __func__, chan, prescale, PRESCALE_MAX,
+                               period_ns, duty_ns);
                        return -EINVAL;
+               }
        }
 
-       /* If the PWM channel is enabled, write the settings to the HW */
-       if (test_bit(PWMF_ENABLED, &pwm->flags)) {
-               value = readl(kp->base + PRESCALE_OFFSET);
-               value &= ~PRESCALE_MASK(chan);
-               value |= prescale << PRESCALE_SHIFT(chan);
-               writel(value, kp->base + PRESCALE_OFFSET);
+       dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
+               chan, pc, dc, prescale);
 
-               writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
+       value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-               writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+       /*
+        * Clear trigger bit but set smooth bit to maintain old
+        * output.
+        */
+       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+       writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-               kona_pwmc_apply_settings(kp, chan);
-       }
+       value = readl(kp->base + PRESCALE_OFFSET);
+       value &= ~PRESCALE_MASK(chan);
+       value |= prescale << PRESCALE_SHIFT(chan);
+       writel(value, kp->base + PRESCALE_OFFSET);
+
+       writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
+
+       writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+
+       kona_pwmc_apply_settings(kp, chan);
 

drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 187

Quoted 2 lines

drivers/pwm/pwm-bcm-kona.c +0 -0

        return 0;
 }

There is no requirement to lower the trigger bit prior to writing the period and duty registers.

This change is unnecessary. The old sequence was fine.

Quoted 12 lines

drivers/pwm/pwm-bcm-kona.c +0 -5

@@ -173,11 +209,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 
        writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-       kona_pwmc_apply_settings(kp, chan);
-
-       /* Wait for waveform to settle before gating off the clock */
-       ndelay(400);
-
        clk_disable_unprepare(kp->clk);
 

drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, · line 214

Quoted 1 line

drivers/pwm/pwm-bcm-kona.c +0 -0

        return 0;

I'm not completely convinced that changing the polarity behavior is beneficial.

Please mention this change in your commit description and provide appropriate justification.

Quoted 10 lines

drivers/pwm/pwm-bcm-kona.c +1 -0

@@ -190,12 +221,14 @@ static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
        ret = clk_prepare_enable(kp->clk);
        if (ret < 0) {
+               clear_bit(PWMF_ENABLED, &pwm->flags);
                dev_err(chip->dev, "failed to enable clock: %d\n", ret);
                return ret;
        }
 

drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm) · line 234

Quoted 6 lines

drivers/pwm/pwm-bcm-kona.c +1 -0

        ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
        if (ret < 0) {
+               clear_bit(PWMF_ENABLED, &pwm->flags);
                clk_disable_unprepare(kp->clk);
                return ret;
        }

Don't try to fix this in the Kona PWM driver, fix it in the PWM core.

Quoted 17 lines

drivers/pwm/pwm-bcm-kona.c +7 -1

@@ -207,13 +240,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
        struct kona_pwmc *kp = to_kona_pwmc(chip);
        unsigned int chan = pwm->hwpwm;
+       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+       /* Set smooth type to 1 and disable */
+       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+       writel(value, kp->base + PWM_CONTROL_OFFSET);
 
        /* Simulate a disable by configuring for zero duty */
        writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
-       kona_pwmc_apply_settings(kp, chan);
+       writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
 

drivers/pwm/pwm-bcm-kona.c static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) · line 257

Quoted 6 lines

drivers/pwm/pwm-bcm-kona.c +4 -2

-       /* Wait for waveform to settle before gating off the clock */
-       ndelay(400);
+       /* Set prescale to 0 for this channel */
+       value = readl(kp->base + PRESCALE_OFFSET);
+       value &= ~PRESCALE_MASK(chan);
+       writel(value, kp->base + PRESCALE_OFFSET);

Can you explain why it is necessary to change the prescale here?

Quoted 9 lines

drivers/pwm/pwm-bcm-kona.c +2 -1

+
+       kona_pwmc_apply_settings(kp, chan);
 
        clk_disable_unprepare(kp->clk);
 }
--

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Add initial version of the Broadcom touchscreen driver.

drivers/input/touchscreen/bcm_iproc_tsc.c +6 -0

+static int get_tsc_config(struct device_node *np, struct iproc_ts_priv *priv)
+{
+	int ret;
+	u32 val;
+	struct device *dev = &priv->pdev->dev;
+

Quoted 1 line · Jonathan Richardson

Add initial version of the Broadcom touchscreen driver.

trivia:

drivers/input/touchscreen/bcm_iproc_tsc.c line 88

Quoted 11 lines

drivers/input/touchscreen/bcm_iproc_tsc.c +9 -0

 
+/* Bit values for REGCTL2 */
+#define TS_CONTROLLER_EN_BIT    (1 << 16)
+#define TS_CONTROLLER_AVGDATA_SHIFT 8
+#define TS_CONTROLLER_AVGDATA_MASK (0x7 << TS_CONTROLLER_AVGDATA_SHIFT)
+#define TS_CONTROLLER_PWR_LDO   (1<<5)
+#define TS_CONTROLLER_PWR_ADC   (1<<4)
+#define TS_CONTROLLER_PWR_BGP   (1<<3)
+#define TS_CONTROLLER_PWR_TS    (1<<2)
+#define TS_WIRE_MODE_BIT        (1<<1)

Be nicer to use the same spacing around <<
or maybe use the BIT macro.

[]

Quoted 6 lines

drivers/input/touchscreen/bcm_iproc_tsc.c +6 -0

+static int get_tsc_config(struct device_node *np, struct iproc_ts_priv *priv)
+{
+	int ret;
+	u32 val;
+	struct device *dev = &priv->pdev->dev;
+

drivers/input/touchscreen/bcm_iproc_tsc.c line 378

Quoted 10 lines

drivers/input/touchscreen/bcm_iproc_tsc.c +10 -0

+	priv->cfg_params = default_config;
+
+	ret = of_property_read_u32(np, "scanning_period", &val);
+	if (ret >= 0) {
+		if ((1 <= val) && (val <= 256))
+			priv->cfg_params.scanning_period = val;
+		else {
+			dev_err(dev, "scanning_period must be [1-256]");
+			return -EINVAL;
+		}

ret is never used so I'd probably remove it
from all these blocks.

It's probably be nicer to invert the logic ald
remove the else.

There's a missing terminating newline too.

Something like:

	if (of_property_read_u32(np, "scanning_period", &val) >= 0) {
		if (val < 1 || val > 256) {
			dev_err(dev, "scanning_period must be [1-256]\n");
			return -EINVAL;
		}
		priv->cfg_params.scanning_period = val;
	}

etc...

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks Joe. I'll send out a new patch set with your suggestions/fixes shortly.