[PATCH v3 0/4] pwm: Add Amlogic Meson SoC PWM Controller

[PATCH v3 0/4] pwm: Add Amlogic Meson SoC PWM Controller

[Neil Armstrong]

Add support for the PWM controller found in Amlogic Meson SoCs.
This controller provides a dual PWM output with 4 selectable clock source
and a two level divider to achieve a better PWM range.

Currently Meson8b and GXBB SoCs are supported.

Changes since v2 at http://lkml.kernel.org/r/1471718083-11516-1-git-send-email-narmstrong@baylibre.com :
- Switch to apply() callback only, this fixes the setup sequence
- Implement get_state
- Make clock parent names as DT match data

Changes since v1 at http://lkml.kernel.org/r/1466173784-15625-1-git-send-email-narmstrong@baylibre.com :
- fix meson8b dtsi

Neil Armstrong (4):
  pwm: Add support for Meson PWM Controller
  dt-bindings: pwm: Add bindings for Meson PWM Controller
  ARM64: dts: meson-gxbb: Add Meson GXBB PWM Controller nodes
  ARM: dts: meson8b: Add Meson8b PWM Controller nodes

 .../devicetree/bindings/pwm/pwm-meson.txt          |  21 +
 arch/arm/boot/dts/meson8b.dtsi                     |  21 +
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |  28 ++
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-meson.c                            | 528 +++++++++++++++++++++
 6 files changed, 608 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-meson.txt
 create mode 100644 drivers/pwm/pwm-meson.c

-- 
1.9.1

[PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Neil Armstrong]

Add support for the PWM controller found in the Amlogic SoCs.
This driver supports the Meson8b and GXBB SoCs.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/pwm/Kconfig     |   9 +
 drivers/pwm/Makefile    |   1 +
 drivers/pwm/pwm-meson.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 538 insertions(+)
 create mode 100644 drivers/pwm/pwm-meson.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 80a566a..bf01288 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -262,6 +262,15 @@ config PWM_LPSS_PLATFORM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-lpss-platform.
 
+config PWM_MESON
+	tristate "Amlogic Meson PWM driver"
+	depends on ARCH_MESON
+	help
+	  The platform driver for Amlogic Meson PWM controller.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-meson.
+
 config PWM_MTK_DISP
 	tristate "MediaTek display PWM driver"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index feef1dd..1194c54 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
 obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
 obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
+obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
 obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
new file mode 100644
index 0000000..56f3493
--- /dev/null
+++ b/drivers/pwm/pwm-meson.c
@@ -0,0 +1,528 @@
+/*
+ * This file is provided under a dual BSD/GPLv2 license.  When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ * Copyright (C) 2014 Amlogic, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ * The full GNU General Public License is included in this distribution
+ * in the file called COPYING.
+ *
+ * BSD LICENSE
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ * Copyright (C) 2014 Amlogic, Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ *   * Redistributions of source code must retain the above copyright
+ *     notice, this list of conditions and the following disclaimer.
+ *   * Redistributions in binary form must reproduce the above copyright
+ *     notice, this list of conditions and the following disclaimer in
+ *     the documentation and/or other materials provided with the
+ *     distribution.
+ *   * Neither the name of Intel Corporation nor the names of its
+ *     contributors may be used to endorse or promote products derived
+ *     from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk-provider.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/pwm.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+
+#define REG_PWM_A	0x0
+#define REG_PWM_B	0x4
+#define PWM_HIGH_SHIFT	16
+
+#define REG_MISC_AB	0x8
+#define MISC_B_CLK_EN	BIT(23)
+#define MISC_A_CLK_EN	BIT(15)
+#define MISC_CLK_DIV_MASK	0x7f
+#define MISC_B_CLK_DIV_SHIFT	16
+#define MISC_A_CLK_DIV_SHIFT	8
+#define MISC_B_CLK_SEL_SHIFT	6
+#define MISC_A_CLK_SEL_SHIFT	4
+#define MISC_CLK_SEL_WIDTH	2
+#define MISC_B_EN	BIT(1)
+#define MISC_A_EN	BIT(0)
+
+#define PWM_NUM		2
+
+static const unsigned int mux_reg_shifts[PWM_NUM] = {
+	MISC_A_CLK_SEL_SHIFT, MISC_B_CLK_SEL_SHIFT
+};
+
+enum pwm_channel {
+	PWM_A = 0,
+	PWM_B,
+};
+
+struct meson_pwm_channel {
+	unsigned int pwm_hi;
+	unsigned int pwm_lo;
+	u8 pwm_pre_div;
+	int period;
+	int duty;
+	struct pwm_state state;
+};
+
+struct meson_pwm_chip {
+	struct platform_device *pdev;
+	struct pwm_chip chip;
+	void __iomem *base;
+	u8 inverter_mask;
+	spinlock_t lock;
+	struct clk *clk_parents[PWM_NUM];
+	struct clk_mux mux[PWM_NUM];
+	struct clk *clk[PWM_NUM];
+};
+
+struct meson_pwm_data {
+	const char *const *parent_names;
+};
+
+#define to_meson_pwm_chip(chip) \
+	container_of(chip, struct meson_pwm_chip, chip)
+
+static int meson_pwm_calc(struct meson_pwm_chip *pwm_data,
+			  struct meson_pwm_channel *pwm_chan,
+			  unsigned int id,
+			  unsigned int duty_ns,
+			  unsigned int period_ns)
+{
+	unsigned int pwm_pre_div;
+	unsigned int pwm_cnt;
+	unsigned int pwm_duty_cnt;
+	unsigned long fin_freq = -1;
+	unsigned long fin_ns;
+	unsigned int i = 0;
+
+	if (duty_ns > period_ns)
+		return -EINVAL;
+
+	if ((~(pwm_data->inverter_mask >> id) & 0x1))
+		duty_ns = period_ns - duty_ns;
+
+	if (period_ns == pwm_chan->period && duty_ns == pwm_chan->duty)
+		return 0;
+
+	switch (id) {
+	case PWM_A:
+		fin_freq = clk_get_rate(pwm_data->clk[0]);
+		break;
+	case PWM_B:
+		fin_freq = clk_get_rate(pwm_data->clk[1]);
+		break;
+	}
+	if (fin_freq <= 0) {
+		dev_err(pwm_data->chip.dev, "invalid source clock frequency\n");
+		return -EINVAL;
+	}
+	dev_dbg(pwm_data->chip.dev, "fin_freq: %luHz\n", fin_freq);
+	fin_ns = NSEC_PER_SEC / fin_freq;
+
+	/* Calc pre_div with the period */
+	for (i = 0; i < MISC_CLK_DIV_MASK; i++) {
+		pwm_pre_div = i;
+		pwm_cnt = DIV_ROUND_CLOSEST(period_ns,
+				fin_ns * (pwm_pre_div + 1));
+		dev_dbg(pwm_data->chip.dev, "fin_ns=%lu pre_div=%d cnt=%d\n",
+				fin_ns, pwm_pre_div, pwm_cnt);
+		if (pwm_cnt <= 0xffff)
+			break;
+	}
+	if (i == MISC_CLK_DIV_MASK) {
+		dev_err(pwm_data->chip.dev, "Unable to get period pre_div");
+		return -EINVAL;
+	}
+	dev_dbg(pwm_data->chip.dev, "period_ns=%d pre_div=%d pwm_cnt=%d\n",
+		period_ns, pwm_pre_div, pwm_cnt);
+
+	if (duty_ns == period_ns) {
+		pwm_chan->pwm_pre_div = pwm_pre_div;
+		pwm_chan->pwm_hi = pwm_cnt;
+		pwm_chan->pwm_lo = 0;
+	} else if (duty_ns == 0) {
+		pwm_chan->pwm_pre_div = pwm_pre_div;
+		pwm_chan->pwm_hi = 0;
+		pwm_chan->pwm_lo = pwm_cnt;
+	} else {
+		/* Then check is we can have the duty with the same pre_div */
+		pwm_duty_cnt = DIV_ROUND_CLOSEST(duty_ns,
+					fin_ns * (pwm_pre_div + 1));
+		if (pwm_cnt > 0xffff) {
+			dev_err(pwm_data->chip.dev, "Unable to get duty period, differences are too high");
+			return -EINVAL;
+		}
+		dev_dbg(pwm_data->chip.dev, "duty_ns=%d pre_div=%d pwm_cnt=%d\n",
+			duty_ns, pwm_pre_div, pwm_duty_cnt);
+
+		pwm_chan->pwm_pre_div = pwm_pre_div;
+		pwm_chan->pwm_hi = pwm_duty_cnt;
+		pwm_chan->pwm_lo = pwm_cnt - pwm_chan->pwm_hi;
+	}
+
+	return 0;
+}
+
+static void meson_pwm_get_state(struct pwm_chip *chip,
+				struct pwm_device *pwm,
+				struct pwm_state *state)
+{
+	struct meson_pwm_chip *pwm_data = to_meson_pwm_chip(chip);
+	unsigned int id = pwm->hwpwm;
+
+	if (!state)
+		return;
+
+	switch (id) {
+	case PWM_A:
+		if ((readl(pwm_data->base + REG_MISC_AB) & MISC_A_EN))
+			state->enabled = true;
+		break;
+	case PWM_B:
+		if ((readl(pwm_data->base + REG_MISC_AB) & MISC_B_EN))
+			state->enabled = true;
+		break;
+	}
+}
+
+static int meson_pwm_request(struct pwm_chip *chip,
+			     struct pwm_device *pwm)
+{
+	struct meson_pwm_channel *pwm_chan;
+
+	pwm_chan = devm_kzalloc(chip->dev, sizeof(*pwm_chan), GFP_KERNEL);
+	if (!pwm_chan)
+		return -ENOMEM;
+
+	meson_pwm_get_state(chip, pwm, &pwm_chan->state);
+
+	pwm_set_chip_data(pwm, pwm_chan);
+
+	return 0;
+}
+
+static void meson_pwm_free(struct pwm_chip *chip,
+			   struct pwm_device *pwm)
+{
+	devm_kfree(chip->dev, pwm_get_chip_data(pwm));
+	pwm_set_chip_data(pwm, NULL);
+}
+
+static void meson_pwm_enable(struct meson_pwm_chip *pwm_data,
+			     struct meson_pwm_channel *pwm_chan,
+			     unsigned int id)
+{
+	switch (id) {
+	case PWM_A:
+		writel((readl(pwm_data->base + REG_MISC_AB) &
+			~(MISC_CLK_DIV_MASK << MISC_A_CLK_DIV_SHIFT)) |
+			((pwm_chan->pwm_pre_div << MISC_A_CLK_DIV_SHIFT) |
+			 MISC_A_CLK_EN),
+			pwm_data->base + REG_MISC_AB);
+
+		writel((pwm_chan->pwm_hi << PWM_HIGH_SHIFT) |
+		       (pwm_chan->pwm_lo),
+		       pwm_data->base + REG_PWM_A);
+
+		writel(readl(pwm_data->base + REG_MISC_AB) | MISC_A_EN,
+			pwm_data->base + REG_MISC_AB);
+		break;
+
+	case PWM_B:
+		writel((readl(pwm_data->base + REG_MISC_AB) &
+			~(MISC_CLK_DIV_MASK << MISC_B_CLK_DIV_SHIFT)) |
+			((pwm_chan->pwm_pre_div << MISC_B_CLK_DIV_SHIFT) |
+			 MISC_B_CLK_EN),
+			pwm_data->base + REG_MISC_AB);
+
+		writel((pwm_chan->pwm_hi << PWM_HIGH_SHIFT) |
+		       (pwm_chan->pwm_lo),
+		       pwm_data->base + REG_PWM_B);
+
+		writel(readl(pwm_data->base + REG_MISC_AB) | MISC_B_EN,
+			pwm_data->base + REG_MISC_AB);
+		break;
+
+	default:
+		break;
+	}
+}
+
+static void meson_pwm_disable(struct meson_pwm_chip *pwm_data, unsigned int id)
+{
+	switch (id) {
+	case PWM_A:
+		writel(readl(pwm_data->base + REG_MISC_AB) & ~MISC_A_EN,
+			pwm_data->base + REG_MISC_AB);
+		break;
+
+	case PWM_B:
+		writel(readl(pwm_data->base + REG_MISC_AB) & ~MISC_B_EN,
+			pwm_data->base + REG_MISC_AB);
+		break;
+
+	default:
+		break;
+	}
+}
+
+static int meson_pwm_apply(struct pwm_chip *chip,
+			   struct pwm_device *pwm,
+			   struct pwm_state *state)
+{
+	struct meson_pwm_chip *pwm_data = to_meson_pwm_chip(chip);
+	struct meson_pwm_channel *pwm_chan = pwm_get_chip_data(pwm);
+	unsigned int id = pwm->hwpwm;
+	unsigned long flags;
+	int ret = 0;
+
+	if (!state)
+		return -EINVAL;
+
+	spin_lock_irqsave(&pwm_data->lock, flags);
+
+	if (!state->enabled) {
+		meson_pwm_disable(pwm_data, id);
+		pwm_chan->state.enabled = false;
+
+		goto out;
+	}
+
+	if (state->period != pwm_chan->state.period ||
+	    state->duty_cycle != pwm_chan->state.duty_cycle ||
+	    state->polarity != pwm_chan->state.polarity) {
+		if (pwm_chan->state.enabled) {
+			meson_pwm_disable(pwm_data, id);
+			pwm_chan->state.enabled = false;
+		}
+
+		if (state->polarity != pwm_chan->state.polarity) {
+			if (state->polarity == PWM_POLARITY_NORMAL)
+				pwm_data->inverter_mask |= BIT(id);
+			else
+				pwm_data->inverter_mask &= ~BIT(id);
+		}
+
+		ret = meson_pwm_calc(pwm_data, pwm_chan, id,
+				     state->duty_cycle,
+				     state->period);
+		if (ret)
+			goto out;
+
+		pwm_chan->state.polarity = state->polarity;
+		pwm_chan->state.period = state->period;
+		pwm_chan->state.duty_cycle = state->duty_cycle;
+	}
+
+	if (state->enabled && !pwm_chan->state.enabled) {
+		meson_pwm_enable(pwm_data, pwm_chan, id);
+		pwm_chan->state.enabled = true;
+	}
+
+out:
+	spin_unlock_irqrestore(&pwm_data->lock, flags);
+
+	return ret;
+}
+
+static const struct pwm_ops meson_pwm_ops = {
+	.request	= meson_pwm_request,
+	.free		= meson_pwm_free,
+	.apply		= meson_pwm_apply,
+	.get_state	= meson_pwm_get_state,
+	.owner		= THIS_MODULE,
+};
+
+static const char *const pwm_meson8b_parent_names[] = {
+	"xtal", "vid_pll", "fclk_div4", "fclk_div3", NULL
+};
+
+static const struct meson_pwm_data pwm_meson8b_data = {
+	.parent_names = pwm_meson8b_parent_names,
+};
+
+static const char *const pwm_gxbb_parent_names[] = {
+	"xtal", "hdmi_pll", "fclk_div4", "fclk_div3", NULL
+};
+
+static const struct meson_pwm_data pwm_gxbb_data = {
+	.parent_names = pwm_gxbb_parent_names,
+};
+
+static const struct of_device_id meson_pwm_matches[] = {
+	{ .compatible = "amlogic,meson8b-pwm", .data = &pwm_meson8b_data },
+	{ .compatible = "amlogic,meson-gxbb-pwm", .data = &pwm_gxbb_data },
+	{},
+};
+MODULE_DEVICE_TABLE(of, meson_pwm_matches);
+
+static int meson_pwm_mux_init(struct meson_pwm_chip *chip,
+			      const struct meson_pwm_data *data)
+{
+	struct device *dev = &chip->pdev->dev;
+	struct clk_init_data init;
+	char clk_name[255];
+	int ret;
+	int i;
+
+	for (i = 0 ; i < PWM_NUM ; ++i) {
+		sprintf(clk_name, "%s#mux%d",
+				of_node_full_name(dev->of_node), i);
+
+		init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
+		init.ops = &clk_mux_ops;
+		init.flags = CLK_IS_BASIC;
+		init.parent_names = data->parent_names;
+		init.num_parents = 1 << MISC_CLK_SEL_WIDTH;
+
+		chip->mux[i].reg = chip->base + REG_MISC_AB;
+		chip->mux[i].shift = mux_reg_shifts[i];
+		chip->mux[i].mask = BIT(MISC_CLK_SEL_WIDTH) - 1;
+		chip->mux[i].flags = 0;
+		chip->mux[i].lock = &chip->lock;
+		chip->mux[i].table = NULL;
+		chip->mux[i].hw.init = &init;
+
+		chip->clk[i] = devm_clk_register(dev, &chip->mux[i].hw);
+		if (IS_ERR(chip->clk[i])) {
+			dev_err(dev, "Failed to register %s\n", clk_name);
+			return PTR_ERR(chip->clk[i]);
+		}
+
+		if (chip->clk_parents[i]) {
+			ret = clk_set_parent(chip->clk[i],
+					     chip->clk_parents[i]);
+			if (ret) {
+				dev_err(dev, "Failed to set %s parent\n",
+					clk_name);
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int meson_pwm_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct device *dev = &pdev->dev;
+	struct meson_pwm_chip *chip;
+	struct resource *res;
+	int ret;
+
+	match = of_match_node(meson_pwm_matches, dev->of_node);
+	if (!match)
+		return -ENODEV;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL)
+		return -ENOMEM;
+
+	chip->pdev = pdev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	chip->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(chip->base))
+		return PTR_ERR(chip->base);
+
+	chip->clk_parents[0] = devm_clk_get(dev, "clkin0");
+	if (IS_ERR(chip->clk_parents[0])) {
+		if (PTR_ERR(chip->clk_parents[0]) == -EPROBE_DEFER)
+			return PTR_ERR(chip->clk_parents[0]);
+		chip->clk_parents[0] = NULL;
+	}
+
+	chip->clk_parents[1] = devm_clk_get(dev, "clkin1");
+	if (IS_ERR(chip->clk_parents[1])) {
+		if (PTR_ERR(chip->clk_parents[1]) == -EPROBE_DEFER)
+			return PTR_ERR(chip->clk_parents[1]);
+		chip->clk_parents[1] = NULL;
+	}
+
+	ret = meson_pwm_mux_init(chip, match->data);
+	if (ret)
+		return ret;
+
+	clk_prepare_enable(chip->clk[0]);
+	clk_prepare_enable(chip->clk[1]);
+
+	chip->chip.dev = dev;
+	chip->chip.ops = &meson_pwm_ops;
+	chip->chip.base = -1;
+	chip->chip.npwm = PWM_NUM;
+	chip->chip.of_xlate = of_pwm_xlate_with_flags;
+	chip->chip.of_pwm_n_cells = 3;
+	chip->inverter_mask = BIT(PWM_NUM) - 1;
+
+	ret = pwmchip_add(&chip->chip);
+	if (ret < 0) {
+		dev_err(dev, "failed to register PWM chip\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+}
+
+static int meson_pwm_remove(struct platform_device *pdev)
+{
+	struct meson_pwm_chip *chip = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&chip->chip);
+}
+
+static struct platform_driver meson_pwm_driver = {
+	.driver		= {
+		.name	= "meson-pwm",
+		.of_match_table = of_match_ptr(meson_pwm_matches),
+	},
+	.probe		= meson_pwm_probe,
+	.remove		= meson_pwm_remove,
+};
+module_platform_driver(meson_pwm_driver);
+
+MODULE_ALIAS("platform:meson-pwm");
+MODULE_DESCRIPTION("Amlogic Meson PWM Generator driver");
+MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
1.9.1

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Martin Blumenstingl]

On Mon, Aug 22, 2016 at 5:36 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> Add support for the PWM controller found in the Amlogic SoCs.
> This driver supports the Meson8b and GXBB SoCs.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
I could enable the AP6330 SDIO wifi module with this on my
meson-gxbb-vega-s95-meta clone, which uses pwm_e to generate the LPO
clock for the wifi module (see [0] for the corresponding patches).
So thank you Neil, and:
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>


[0] https://github.com/xdarklight/linux/tree/meson-gxbb-integration-4.8-20160828

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

On Sun, Aug 28, 2016 at 06:33:03PM +0200, Martin Blumenstingl wrote:
> On Mon, Aug 22, 2016 at 5:36 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> > Add support for the PWM controller found in the Amlogic SoCs.
> > This driver supports the Meson8b and GXBB SoCs.
> >
> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> I could enable the AP6330 SDIO wifi module with this on my
> meson-gxbb-vega-s95-meta clone, which uses pwm_e to generate the LPO
> clock for the wifi module (see [0] for the corresponding patches).
> So thank you Neil, and:
> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Hi Martin,

As mentioned in another reply in this thread I ended up restructuring
the driver a little before applying it to the PWM tree. I don't /think/
there are any functional changes, so I left your Tested-by in place, but
it would be great if you could retest and confirm, just to make sure.

The driver is in the for-4.9/drivers branch of the PWM tree:

	https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-4.9/drivers

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Martin Blumenstingl]

Hi Thierry,

On Mon, Sep 5, 2016 at 11:53 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> As mentioned in another reply in this thread I ended up restructuring
> the driver a little before applying it to the PWM tree. I don't /think/
> there are any functional changes, so I left your Tested-by in place, but
> it would be great if you could retest and confirm, just to make sure.
>
> The driver is in the for-4.9/drivers branch of the PWM tree:
>
>         https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-4.9/drivers
I have tested it with the updated Neil sent today: [0]
This is still working fine for me (driving the LPO clock of an AP6330
SDIO wifi chip).

Please note that I don't have a scope to measure the actual signal,
but I guess it's fine since my AP6330 is happy.

So feel free to keep my:
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>


Regards,
Martin


[0] https://github.com/superna9999/linux/releases/tag/amlogic/v4.8/pwm-for-next-for-v4

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

On Mon, Aug 22, 2016 at 05:36:30PM +0200, Neil Armstrong wrote:
> Add support for the PWM controller found in the Amlogic SoCs.
> This driver supports the Meson8b and GXBB SoCs.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/pwm/Kconfig     |   9 +
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-meson.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 538 insertions(+)
>  create mode 100644 drivers/pwm/pwm-meson.c

Hi Neil,

sorry for taking so long to review this. I had actually started to write
a review email since I had noticed a couple of slight oddities about the
driver structure (primarily this was about how channel-specific data was
split between struct meson_pwm_channel and struct meson_pwm_chip), but I
ended up making some changes to the driver in order to see what my
suggestions would look like, and if they would indeed improve things.
But once I had done that, I thought it a bit pointless to make that into
review comments and decided to just push what I had done and ask you to
take a look, and if you had no objections to the changes take the driver
for a spin to see if it still worked as expected.

One other thing I noticed is that your ->get_state() implementation only
reads the enable state, but from the looks of it it should be possible
to read period and duty cycle information from hardware as well. I'm not
going to reject the driver for that reason, just saying that it'd be
good to have that implemented sometime in the future.

I've pushed my modifications to the driver to the linux-pwm repository:

	https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next

Alternatively you can also take a look at the for-4.9/drivers branch,
but they're currently the same thing.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Neil Armstrong]

On 09/05/2016 11:00 AM, Thierry Reding wrote:
> On Mon, Aug 22, 2016 at 05:36:30PM +0200, Neil Armstrong wrote:
>> Add support for the PWM controller found in the Amlogic SoCs.
>> This driver supports the Meson8b and GXBB SoCs.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/pwm/Kconfig     |   9 +
>>  drivers/pwm/Makefile    |   1 +
>>  drivers/pwm/pwm-meson.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 538 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-meson.c

Hi Thierry,
> Hi Neil,
> 
> sorry for taking so long to review this. I had actually started to write
> a review email since I had noticed a couple of slight oddities about the
> driver structure (primarily this was about how channel-specific data was
> split between struct meson_pwm_channel and struct meson_pwm_chip), but I
> ended up making some changes to the driver in order to see what my
> suggestions would look like, and if they would indeed improve things.
> But once I had done that, I thought it a bit pointless to make that into
> review comments and decided to just push what I had done and ask you to
> take a look, and if you had no objections to the changes take the driver
> for a spin to see if it still worked as expected.

Well, thanks ! I was wondering why it took so long, but the result look far best than what I achieved.

The road was very long since the original Amlogic driver...

I will try it out ASAP, but it looks very good for me.
Your changes seems quite obvious, and such rework was necessary.

> 
> One other thing I noticed is that your ->get_state() implementation only
> reads the enable state, but from the looks of it it should be possible
> to read period and duty cycle information from hardware as well. I'm not
> going to reject the driver for that reason, just saying that it'd be
> good to have that implemented sometime in the future.

Yes, it was delayed for later since it's not a functional feature, I will certainly push
an update with this later on.

> 
> I've pushed my modifications to the driver to the linux-pwm repository:
> 
> 	https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next
> 
> Alternatively you can also take a look at the for-4.9/drivers branch,
> but they're currently the same thing.

Great, I will give you a functional update ASAP.

> Thierry

Thanks for the review, aww, s/review/rework/ !
Neil
> 

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Neil Armstrong]

Hi Thierry,

On 09/05/2016 11:00 AM, Thierry Reding wrote:
> On Mon, Aug 22, 2016 at 05:36:30PM +0200, Neil Armstrong wrote:
>> Add support for the PWM controller found in the Amlogic SoCs.
>> This driver supports the Meson8b and GXBB SoCs.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/pwm/Kconfig     |   9 +
>>  drivers/pwm/Makefile    |   1 +
>>  drivers/pwm/pwm-meson.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 538 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-meson.c
> 
> Hi Neil,
> 
> sorry for taking so long to review this. I had actually started to write
> a review email since I had noticed a couple of slight oddities about the
> driver structure (primarily this was about how channel-specific data was
> split between struct meson_pwm_channel and struct meson_pwm_chip), but I
> ended up making some changes to the driver in order to see what my
> suggestions would look like, and if they would indeed improve things.
> But once I had done that, I thought it a bit pointless to make that into
> review comments and decided to just push what I had done and ask you to
> take a look, and if you had no objections to the changes take the driver
> for a spin to see if it still worked as expected.

We re-run our tests and I found 2 bugs, the first one is in meson_pwm_enable(),
only the channel A was setup, the fix is :

static void meson_pwm_enable(...)
-	u32 value, clk_shift, clk_enable, enable;
+	u32 reg, value, clk_shift, clk_enable, enable;

 	switch (id) {
 	case 0:
[...]
+		reg = REG_PWM_A;
 		break;
 	case 1:
[...]
+		reg = REG_PWM_B;
 		break;
 	}
[...]
-	writel(value, meson->base + REG_PWM_A);
+	writel(value, meson->base + reg);

The second bug is in probe(), I understand the point to allocate dynamically the channels
and attach them to each pwm chip, but when calling meson_pwm_init_channels() we get an OOPS
because meson->chip.pwms[i] are allocated in pwmchip_add().
Moving meson_pwm_init_channels() would fix this, but in case of a clk PROBE_DEFER, we would need
to remove back the pwmchip, which is a quite a bad design decision....

The smartest fix I found was to allocate channels in probe, init them them attach them after pwmchip_add():

static int meson_pwm_init_channels(..., struct meson_pwm_channel *channels)
{
+	struct meson_pwm_channel *channels;
[...]
-	for (i = 0; i < meson->chip.npwm; i++) {
-		struct pwm_device *pwm = &meson->chip.pwms[i];
-		struct meson_pwm_channel *channel;
-
-		channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
-		if (!channel)
-			return -ENOMEM;
+	if (!channels)
+		return -EINVAL;

+	for (i = 0; i < meson->chip.npwm; i++) {
[...]
+		memset(&channels[i], 0, sizeof(struct meson_pwm_channel));
[...]
//Rename "channel->" into "channels[i]."//
[...]
-		pwm_set_chip_data(pwm, channel);
 	}

 	return 0;
}

+static void meson_pwm_add_channels_data(struct meson_pwm *meson,
+					struct meson_pwm_channel *channels)
+{
+	unsigned int i;
+
+	for (i = 0; i < meson->chip.npwm; i++)
+		pwm_set_chip_data(&meson->chip.pwms[i], &channels[i]);
+}

static int meson_pwm_probe(struct platform_device *pdev)
{
+	struct meson_pwm_channel *channels;
[...]
-	err = meson_pwm_init_channels(meson);
-	if (err < 0)
-		return err;
-
 	meson->chip.dev = &pdev->dev;
[...]
 	meson->chip.of_pwm_n_cells = 3;

+	channels = devm_kmalloc_array(&pdev->dev, 2, sizeof(*meson),
+				      GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	err = meson_pwm_init_channels(meson, channels);
+	if (err < 0)
+		return err;
+
 	err = pwmchip_add(&meson->chip);
[...]
+	meson_pwm_add_channels_data(meson, channels);
+
 	platform_set_drvdata(pdev, meson);

 	return 0;
}

The fix driver is in a separate branch, rebased on your for-next :
https://github.com/superna9999/linux/tree/amlogic/v4.8/pwm-for-next

and in a signed tag I can transform in a pull request if needed :
https://github.com/superna9999/linux/releases/tag/amlogic/v4.8/pwm-for-next-for-v4

[...]
> 
> I've pushed my modifications to the driver to the linux-pwm repository:
> 
> 	https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next
> 
> Alternatively you can also take a look at the for-4.9/drivers branch,
> but they're currently the same thing.
> 
> Thierry
> 

Thanks,
Neil

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5111 bytes --]

On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote:
> Hi Thierry,
> 
> On 09/05/2016 11:00 AM, Thierry Reding wrote:
> > On Mon, Aug 22, 2016 at 05:36:30PM +0200, Neil Armstrong wrote:
> >> Add support for the PWM controller found in the Amlogic SoCs.
> >> This driver supports the Meson8b and GXBB SoCs.
> >>
> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >> ---
> >>  drivers/pwm/Kconfig     |   9 +
> >>  drivers/pwm/Makefile    |   1 +
> >>  drivers/pwm/pwm-meson.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 538 insertions(+)
> >>  create mode 100644 drivers/pwm/pwm-meson.c
> > 
> > Hi Neil,
> > 
> > sorry for taking so long to review this. I had actually started to write
> > a review email since I had noticed a couple of slight oddities about the
> > driver structure (primarily this was about how channel-specific data was
> > split between struct meson_pwm_channel and struct meson_pwm_chip), but I
> > ended up making some changes to the driver in order to see what my
> > suggestions would look like, and if they would indeed improve things.
> > But once I had done that, I thought it a bit pointless to make that into
> > review comments and decided to just push what I had done and ask you to
> > take a look, and if you had no objections to the changes take the driver
> > for a spin to see if it still worked as expected.
> 
> We re-run our tests and I found 2 bugs, the first one is in meson_pwm_enable(),
> only the channel A was setup, the fix is :
> 
> static void meson_pwm_enable(...)
> -	u32 value, clk_shift, clk_enable, enable;
> +	u32 reg, value, clk_shift, clk_enable, enable;
> 
>  	switch (id) {
>  	case 0:
> [...]
> +		reg = REG_PWM_A;
>  		break;
>  	case 1:
> [...]
> +		reg = REG_PWM_B;
>  		break;
>  	}
> [...]
> -	writel(value, meson->base + REG_PWM_A);
> +	writel(value, meson->base + reg);

Ah indeed. Good catch.

> 
> The second bug is in probe(), I understand the point to allocate
> dynamically the channels and attach them to each pwm chip, but when
> calling meson_pwm_init_channels() we get an OOPS because
> meson->chip.pwms[i] are allocated in pwmchip_add(). Moving
> meson_pwm_init_channels() would fix this, but in case of a clk
> PROBE_DEFER, we would need to remove back the pwmchip, which is a
> quite a bad design decision....

Ah yes... that one again. I remember running into that a while ago with
some other driver. To be honest, I think that's a short-coming of the
PWM subsystem and the fix would be for PWM chip registration to be split
into two parts: pwm_chip_init() and pwm_chip_add(). That way, a chip
would be initialized using pwm_chip_init() where the pwms array would be
allocated, and pwm_chip_add() would register the chip with the system.

Currently a few drivers might be vulnerable to a race condition between
registration and implementation (i.e. PWM channels aren't fully set up
when they are exposed to users and sysfs).

> The smartest fix I found was to allocate channels in probe, init them
> them attach them after pwmchip_add():
> 
> static int meson_pwm_init_channels(..., struct meson_pwm_channel *channels)
> {
> +	struct meson_pwm_channel *channels;
> [...]
> -	for (i = 0; i < meson->chip.npwm; i++) {
> -		struct pwm_device *pwm = &meson->chip.pwms[i];
> -		struct meson_pwm_channel *channel;
> -
> -		channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> -		if (!channel)
> -			return -ENOMEM;
> +	if (!channels)
> +		return -EINVAL;
> 
> +	for (i = 0; i < meson->chip.npwm; i++) {
> [...]
> +		memset(&channels[i], 0, sizeof(struct meson_pwm_channel));
> [...]
> //Rename "channel->" into "channels[i]."//
> [...]
> -		pwm_set_chip_data(pwm, channel);
>  	}
> 
>  	return 0;
> }
> 
> +static void meson_pwm_add_channels_data(struct meson_pwm *meson,
> +					struct meson_pwm_channel *channels)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < meson->chip.npwm; i++)
> +		pwm_set_chip_data(&meson->chip.pwms[i], &channels[i]);
> +}
> 
> static int meson_pwm_probe(struct platform_device *pdev)
> {
> +	struct meson_pwm_channel *channels;
> [...]
> -	err = meson_pwm_init_channels(meson);
> -	if (err < 0)
> -		return err;
> -
>  	meson->chip.dev = &pdev->dev;
> [...]
>  	meson->chip.of_pwm_n_cells = 3;
> 
> +	channels = devm_kmalloc_array(&pdev->dev, 2, sizeof(*meson),
> +				      GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	err = meson_pwm_init_channels(meson, channels);
> +	if (err < 0)
> +		return err;
> +
>  	err = pwmchip_add(&meson->chip);
> [...]
> +	meson_pwm_add_channels_data(meson, channels);
> +
>  	platform_set_drvdata(pdev, meson);
> 
>  	return 0;
> }

That's the race I was talking about above. I suppose it's not too big an
issue since other drivers seem to manage, so I'm going to merge your
fixed driver.

Unless you feel like taking a stab at the pwm_chip_init()/pwm_chip_add()
split, in which case your driver would be the first to be race-free. =)

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Neil Armstrong]

On 09/06/2016 11:07 AM, Thierry Reding wrote:
> On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote:
>> Hi Thierry,
>>
[...]
> 
>>
>> The second bug is in probe(), I understand the point to allocate
>> dynamically the channels and attach them to each pwm chip, but when
>> calling meson_pwm_init_channels() we get an OOPS because
>> meson->chip.pwms[i] are allocated in pwmchip_add(). Moving
>> meson_pwm_init_channels() would fix this, but in case of a clk
>> PROBE_DEFER, we would need to remove back the pwmchip, which is a
>> quite a bad design decision....
> 
> Ah yes... that one again. I remember running into that a while ago with
> some other driver. To be honest, I think that's a short-coming of the
> PWM subsystem and the fix would be for PWM chip registration to be split
> into two parts: pwm_chip_init() and pwm_chip_add(). That way, a chip
> would be initialized using pwm_chip_init() where the pwms array would be
> allocated, and pwm_chip_add() would register the chip with the system.
> 
> Currently a few drivers might be vulnerable to a race condition between
> registration and implementation (i.e. PWM channels aren't fully set up
> when they are exposed to users and sysfs).
> 
>> The smartest fix I found was to allocate channels in probe, init them
>> them attach them after pwmchip_add():
>>
[...]

> 
> That's the race I was talking about above. I suppose it's not too big an
> issue since other drivers seem to manage, so I'm going to merge your
> fixed driver.

ok thanks !

> 
> Unless you feel like taking a stab at the pwm_chip_init()/pwm_chip_add()
> split, in which case your driver would be the first to be race-free. =)

Having he driver upstream is a priority, but having it completely race-free would be great!
I'll be happy to collaborate to a race-free pwmchip probe somehow !

But there is still a glitch, when pwmadd_chip() returns, pwm_get_chip_data(pwm) will still
return crap until meson_pwm_add_channels_data() is called in the following instructions...

The pwm_chip_init()/pwm_chip_add() would be the only solution !

> Thierry
> 

Thanks,
Neil

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

On Tue, Sep 06, 2016 at 11:14:45AM +0200, Neil Armstrong wrote:
> On 09/06/2016 11:07 AM, Thierry Reding wrote:
> > On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote:
> >> Hi Thierry,
> >>
> [...]
> > 
> >>
> >> The second bug is in probe(), I understand the point to allocate
> >> dynamically the channels and attach them to each pwm chip, but when
> >> calling meson_pwm_init_channels() we get an OOPS because
> >> meson->chip.pwms[i] are allocated in pwmchip_add(). Moving
> >> meson_pwm_init_channels() would fix this, but in case of a clk
> >> PROBE_DEFER, we would need to remove back the pwmchip, which is a
> >> quite a bad design decision....
> > 
> > Ah yes... that one again. I remember running into that a while ago with
> > some other driver. To be honest, I think that's a short-coming of the
> > PWM subsystem and the fix would be for PWM chip registration to be split
> > into two parts: pwm_chip_init() and pwm_chip_add(). That way, a chip
> > would be initialized using pwm_chip_init() where the pwms array would be
> > allocated, and pwm_chip_add() would register the chip with the system.
> > 
> > Currently a few drivers might be vulnerable to a race condition between
> > registration and implementation (i.e. PWM channels aren't fully set up
> > when they are exposed to users and sysfs).
> > 
> >> The smartest fix I found was to allocate channels in probe, init them
> >> them attach them after pwmchip_add():
> >>
> [...]
> 
> > 
> > That's the race I was talking about above. I suppose it's not too big an
> > issue since other drivers seem to manage, so I'm going to merge your
> > fixed driver.
> 
> ok thanks !

I've made a few tiny changes (reg -> offset, temporary variable to track
&channels[i], ...) and pushed it all out. Hopefully that now fixes any
of the remaining issues.

> > Unless you feel like taking a stab at the pwm_chip_init()/pwm_chip_add()
> > split, in which case your driver would be the first to be race-free. =)
> 
> Having he driver upstream is a priority, but having it completely
> race-free would be great! I'll be happy to collaborate to a race-free
> pwmchip probe somehow !

Fair enough. I'll do some prototyping and keep you in the loop if I come
up with something that I think will do.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[jbrunet]

On Tue, 2016-09-06 at 12:04 +0200, Thierry Reding wrote:
> On Tue, Sep 06, 2016 at 11:14:45AM +0200, Neil Armstrong wrote:
> > 
> > On 09/06/2016 11:07 AM, Thierry Reding wrote:
> > > 
> > > On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote:
> > > > 
> > > > Hi Thierry,
> > > > 
> > [...]
> > > 
> > > 
> > > > 
> > > > 
> > > > The second bug is in probe(), I understand the point to
> > > > allocate
> > > > dynamically the channels and attach them to each pwm chip, but
> > > > when
> > > > calling meson_pwm_init_channels() we get an OOPS because
> > > > meson->chip.pwms[i] are allocated in pwmchip_add(). Moving
> > > > meson_pwm_init_channels() would fix this, but in case of a clk
> > > > PROBE_DEFER, we would need to remove back the pwmchip, which is
> > > > a
> > > > quite a bad design decision....
> > > 
> > > Ah yes... that one again. I remember running into that a while
> > > ago with
> > > some other driver. To be honest, I think that's a short-coming of
> > > the
> > > PWM subsystem and the fix would be for PWM chip registration to
> > > be split
> > > into two parts: pwm_chip_init() and pwm_chip_add(). That way, a
> > > chip
> > > would be initialized using pwm_chip_init() where the pwms array
> > > would be
> > > allocated, and pwm_chip_add() would register the chip with the
> > > system.
> > > 
> > > Currently a few drivers might be vulnerable to a race condition
> > > between
> > > registration and implementation (i.e. PWM channels aren't fully
> > > set up
> > > when they are exposed to users and sysfs).
> > > 
> > > > 
> > > > The smartest fix I found was to allocate channels in probe,
> > > > init them
> > > > them attach them after pwmchip_add():
> > > > 
> > [...]
> > 
> > > 
> > > 
> > > That's the race I was talking about above. I suppose it's not too
> > > big an
> > > issue since other drivers seem to manage, so I'm going to merge
> > > your
> > > fixed driver.
> > 
> > ok thanks !
> 
> I've made a few tiny changes (reg -> offset, temporary variable to
> track
> &channels[i], ...) and pushed it all out. Hopefully that now fixes
> any
> of the remaining issues.
> 
> > 
> > > 
> > > Unless you feel like taking a stab at the
> > > pwm_chip_init()/pwm_chip_add()
> > > split, in which case your driver would be the first to be race-
> > > free. =)
> > 
> > Having he driver upstream is a priority, but having it completely
> > race-free would be great! I'll be happy to collaborate to a race-
> > free
> > pwmchip probe somehow !
> 
> Fair enough. I'll do some prototyping and keep you in the loop if I
> come
> up with something that I think will do.
> 
> Thierry

Hi Thierry,

I have tested the latest version on the P200 (S905), channels E and F.
It works as expected.

Regards

Tested-by: Jerome Brunet <jbrunet@baylibre.com>

[PATCH v3 2/4] dt-bindings: pwm: Add bindings for Meson PWM Controller

[Neil Armstrong]

Add bindings for the Amlogic PWM Controller in Meson8b and GXBB SoCs.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 Documentation/devicetree/bindings/pwm/pwm-meson.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-meson.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-meson.txt b/Documentation/devicetree/bindings/pwm/pwm-meson.txt
new file mode 100644
index 0000000..e06aed0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-meson.txt
@@ -0,0 +1,21 @@
+Amlogic Meson PWM Controller
+=================================
+
+Required properties:
+- compatible: Shall contain "amlogic,meson8b-pwm" or "amlogic,meson-gxbb-pwm".
+- #pwm-cells: Should be 3. See pwm.txt in this directory for a description of
+  the cells format.
+
+Optional properties:
+- clocks: Could contain one or two parents clocks phandle for each of the two pwm channels.
+- clock-names: Could contain at least the "clkin0" and/or "clkin1" names.
+
+Example:
+pwm_ab: pwm@8550 {
+	compatible = "amlogic,meson-gxbb-pwm";
+	reg = <0x0 0x08550 0x0 0x10>;
+	#pwm-cells = <3>;
+	status = "disabled";
+	clocks = <&xtal>, <&xtal>;
+	clock-names = "clkin0", "clkin1";
+}
-- 
1.9.1

[PATCH v3 3/4] ARM64: dts: meson-gxbb: Add Meson GXBB PWM Controller nodes

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index e502c24..e3f4d36 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -153,6 +153,27 @@
 				status = "disabled";
 			};
 
+			pwm_ab: pwm@8550 {
+				compatible = "amlogic,meson-gxbb-pwm";
+				reg = <0x0 0x08550 0x0 0x10>;
+				#pwm-cells = <3>;
+				status = "disabled";
+			};
+
+			pwm_cd: pwm@8650 {
+				compatible = "amlogic,meson-gxbb-pwm";
+				reg = <0x0 0x08650 0x0 0x10>;
+				#pwm-cells = <3>;
+				status = "disabled";
+			};
+
+			pwm_ef: pwm@86c0 {
+				compatible = "amlogic,meson-gxbb-pwm";
+				reg = <0x0 0x086c0 0x0 0x10>;
+				#pwm-cells = <3>;
+				status = "disabled";
+			};
+
 			uart_C: serial@8700 {
 				compatible = "amlogic,meson-uart";
 				reg = <0x0 0x8700 0x0 0x14>;
@@ -212,6 +233,13 @@
 				clocks = <&xtal>;
 				status = "disabled";
 			};
+
+			pwm_ab_AO: pwm@550 {
+				compatible = "amlogic,meson-gxbb-pwm";
+				reg = <0x0 0x0550 0x0 0x10>;
+				#pwm-cells = <3>;
+				status = "disabled";
+			};
 		};
 
 		periphs: periphs@c8834000 {
-- 
1.9.1

Re: [PATCH v3 3/4] ARM64: dts: meson-gxbb: Add Meson GXBB PWM Controller nodes

[Martin Blumenstingl]

On Mon, Aug 22, 2016 at 5:36 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> index e502c24..e3f4d36 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> @@ -153,6 +153,27 @@
>                                 status = "disabled";
>                         };
>
> +                       pwm_ab: pwm@8550 {
> +                               compatible = "amlogic,meson-gxbb-pwm";
> +                               reg = <0x0 0x08550 0x0 0x10>;
> +                               #pwm-cells = <3>;
> +                               status = "disabled";
> +                       };
> +
> +                       pwm_cd: pwm@8650 {
> +                               compatible = "amlogic,meson-gxbb-pwm";
> +                               reg = <0x0 0x08650 0x0 0x10>;
> +                               #pwm-cells = <3>;
> +                               status = "disabled";
> +                       };
> +
> +                       pwm_ef: pwm@86c0 {
> +                               compatible = "amlogic,meson-gxbb-pwm";
> +                               reg = <0x0 0x086c0 0x0 0x10>;
> +                               #pwm-cells = <3>;
> +                               status = "disabled";
> +                       };
I have successfully tested pwm_e on my meson-gxbb-vega-s95-meta clone
(see [0] for the corresponding patches), thus:
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

[0] https://github.com/xdarklight/linux/tree/meson-gxbb-integration-4.8-20160828

[PATCH v3 4/4] ARM: dts: meson8b: Add Meson8b PWM Controller nodes

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 arch/arm/boot/dts/meson8b.dtsi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index fc4080d..41fd536 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -162,6 +162,27 @@
 			reg = <0xc1108000 0x4>, <0xc1104000 0x460>;
 		};
 
+		pwm_ab: pwm@8550 {
+			compatible = "amlogic,meson8b-pwm";
+			reg = <0xc1108550 0x10>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm_cd: pwm@8650 {
+			compatible = "amlogic,meson8b-pwm";
+			reg = <0xc1108650 0x10>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm_ef: pwm@86c0 {
+			compatible = "amlogic,meson8b-pwm";
+			reg = <0xc11086c0 0x10>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		pinctrl_cbus: pinctrl@c1109880 {
 			compatible = "amlogic,meson8b-cbus-pinctrl";
 			reg = <0xc1109880 0x10>;
-- 
1.9.1

Re: [PATCH v3 0/4] pwm: Add Amlogic Meson SoC PWM Controller

[Kevin Hilman]

Neil Armstrong <narmstrong@baylibre.com> writes:

> Add support for the PWM controller found in Amlogic Meson SoCs.
> This controller provides a dual PWM output with 4 selectable clock source
> and a two level divider to achieve a better PWM range.
>
> Currently Meson8b and GXBB SoCs are supported.
>
> Changes since v2 at http://lkml.kernel.org/r/1471718083-11516-1-git-send-email-narmstrong@baylibre.com :
> - Switch to apply() callback only, this fixes the setup sequence
> - Implement get_state
> - Make clock parent names as DT match data
>
> Changes since v1 at http://lkml.kernel.org/r/1466173784-15625-1-git-send-email-narmstrong@baylibre.com :
> - fix meson8b dtsi
>
> Neil Armstrong (4):
>   pwm: Add support for Meson PWM Controller
>   dt-bindings: pwm: Add bindings for Meson PWM Controller

Looks like Thierry has picked up these two into his for-4.9/drivers
branch...

>   ARM64: dts: meson-gxbb: Add Meson GXBB PWM Controller nodes
>   ARM: dts: meson8b: Add Meson8b PWM Controller nodes

... so I've picked up these into the amlogic tree.

Kevin