From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0C54415E98 for ; Mon, 25 Sep 2023 10:48:31 +0000 (UTC) Received: from ex01.ufhost.com (ex01.ufhost.com [61.152.239.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E2E02B8; Mon, 25 Sep 2023 03:48:28 -0700 (PDT) Received: from EXMBX165.cuchost.com (unknown [175.102.18.54]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "EXMBX165", Issuer "EXMBX165" (not verified)) by ex01.ufhost.com (Postfix) with ESMTP id 6D51324E277; Mon, 25 Sep 2023 18:48:27 +0800 (CST) Received: from EXMBX168.cuchost.com (172.16.6.78) by EXMBX165.cuchost.com (172.16.6.75) with Microsoft SMTP Server (TLS) id 15.0.1497.42; Mon, 25 Sep 2023 18:48:27 +0800 Received: from [192.168.120.47] (171.223.208.138) by EXMBX168.cuchost.com (172.16.6.78) with Microsoft SMTP Server (TLS) id 15.0.1497.42; Mon, 25 Sep 2023 18:48:26 +0800 Message-ID: <45b3f679-df20-a560-6fdc-70fc67d33188@starfivetech.com> Date: Mon, 25 Sep 2023 18:48:25 +0800 Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [PATCH v5 2/4] pwm: starfive: Add PWM driver support To: Emil Renner Berthing , , , , CC: Emil Renner Berthing , Rob Herring , Thierry Reding , Philipp Zabel , Krzysztof Kozlowski , Conor Dooley , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= , Hal Feng , Paul Walmsley , "Palmer Dabbelt" , Albert Ou References: <20230922092848.72664-1-william.qiu@starfivetech.com> <20230922092848.72664-3-william.qiu@starfivetech.com> Content-Language: en-US From: William Qiu In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [171.223.208.138] X-ClientProxiedBy: EXCAS062.cuchost.com (172.16.6.22) To EXMBX168.cuchost.com (172.16.6.78) X-YovoleRuleAgent: yovoleflag X-Spam-Status: No, score=-0.7 required=5.0 tests=BAYES_00,NICE_REPLY_A, RCVD_IN_DNSWL_BLOCKED,RCVD_IN_PSBL,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net On 2023/9/25 18:31, Emil Renner Berthing wrote: > William Qiu wrote: >> >> >> On 2023/9/23 20:08, Emil Renner Berthing wrote: >> > William Qiu wrote: >> >> Add Pulse Width Modulation driver support for StarFive >> >> JH7100 and JH7110 SoC. >> >> >> >> Co-developed-by: Hal Feng >> >> Signed-off-by: Hal Feng >> >> Signed-off-by: William Qiu >> >> --- >> >> MAINTAINERS | 7 ++ >> >> drivers/pwm/Kconfig | 9 ++ >> >> drivers/pwm/Makefile | 1 + >> >> drivers/pwm/pwm-starfive.c | 190 +++++++++++++++++++++++++++++++++++++ >> >> 4 files changed, 207 insertions(+) >> >> create mode 100644 drivers/pwm/pwm-starfive.c >> >> >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> >> index bf0f54c24f81..bc2155bd2712 100644 >> >> --- a/MAINTAINERS >> >> +++ b/MAINTAINERS >> >> @@ -20495,6 +20495,13 @@ F: drivers/pinctrl/starfive/pinctrl-starfive-jh71* >> >> F: include/dt-bindings/pinctrl/pinctrl-starfive-jh7100.h >> >> F: include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h >> >> >> >> +STARFIVE JH71X0 PWM DRIVERS >> >> +M: William Qiu >> >> +M: Hal Feng >> >> +S: Supported >> >> +F: Documentation/devicetree/bindings/pwm/starfive,jh7100-pwm.yaml >> >> +F: drivers/pwm/pwm-starfive-ptc.c >> >> + >> >> STARFIVE JH71X0 RESET CONTROLLER DRIVERS >> >> M: Emil Renner Berthing >> >> M: Hal Feng >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> >> index 8ebcddf91f7b..e2ee0169f6e4 100644 >> >> --- a/drivers/pwm/Kconfig >> >> +++ b/drivers/pwm/Kconfig >> >> @@ -569,6 +569,15 @@ config PWM_SPRD >> >> To compile this driver as a module, choose M here: the module >> >> will be called pwm-sprd. >> >> >> >> +config PWM_STARFIVE >> >> + tristate "StarFive PWM support" >> >> + depends on ARCH_STARFIVE || COMPILE_TEST >> >> + help >> >> + Generic PWM framework driver for StarFive SoCs. >> >> + >> >> + To compile this driver as a module, choose M here: the module >> >> + will be called pwm-starfive. >> >> + >> >> config PWM_STI >> >> tristate "STiH4xx PWM support" >> >> depends on ARCH_STI || COMPILE_TEST >> >> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile >> >> index c822389c2a24..93b954376873 100644 >> >> --- a/drivers/pwm/Makefile >> >> +++ b/drivers/pwm/Makefile >> >> @@ -52,6 +52,7 @@ obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o >> >> obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o >> >> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o >> >> obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o >> >> +obj-$(CONFIG_PWM_STARFIVE) += pwm-starfive.o >> >> obj-$(CONFIG_PWM_STI) += pwm-sti.o >> >> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o >> >> obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o >> >> diff --git a/drivers/pwm/pwm-starfive.c b/drivers/pwm/pwm-starfive.c >> > >> > Hi William, >> > >> > You never answered my questions about what PTC is short for and if there are >> > other PWMs on the JH7110. You just removed -ptc from the name of this file.. >> > >> Hi Emil, >> >> The PTC, short for PWM/TIMER/CONUTER, comes from OpenCore's ip, but only PWM >> mode is used in the JH7110. So the register still has the word "PTC". >> s the best way to change all the prefix to STARFIVE? > > I see. Yeah, since you're only using the P from PTC the PTC name doesn't make a > lot of sense anymore. I'd just call this whole driver > STARFIVE_PWM_/starfive_pwm_ consistently. > I see, I'll update it in next version. >> >> Best regards, >> William >> >> new file mode 100644 >> >> index 000000000000..d390349fc95d >> >> --- /dev/null >> >> +++ b/drivers/pwm/pwm-starfive.c >> >> @@ -0,0 +1,190 @@ >> >> +// SPDX-License-Identifier: GPL-2.0 >> >> +/* >> >> + * PWM driver for the StarFive JH71x0 SoC >> >> + * >> >> + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd. >> >> + */ >> >> + >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> + >> >> +/* Access PTC register (CNTR, HRC, LRC and CTRL) */ >> >> +#define REG_PTC_BASE_ADDR_SUB(base, N) ((base) + (((N) > 3) ? \ >> >> + (((N) % 4) * 0x10 + (1 << 15)) : ((N) * 0x10))) >> >> +#define REG_PTC_RPTC_CNTR(base, N) (REG_PTC_BASE_ADDR_SUB(base, N)) >> >> +#define REG_PTC_RPTC_HRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x4) >> >> +#define REG_PTC_RPTC_LRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x8) >> >> +#define REG_PTC_RPTC_CTRL(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0xC) >> > >> > ..but these defines >> > >> >> + >> >> +/* PTC_RPTC_CTRL register bits*/ >> >> +#define PTC_EN BIT(0) >> >> +#define PTC_ECLK BIT(1) >> >> +#define PTC_NEC BIT(2) >> >> +#define PTC_OE BIT(3) >> >> +#define PTC_SIGNLE BIT(4) >> >> +#define PTC_INTE BIT(5) >> >> +#define PTC_INT BIT(6) >> >> +#define PTC_CNTRRST BIT(7) >> >> +#define PTC_CAPTE BIT(8) >> > >> > ..and these defines are still prefixed with *PTC where I'd expect something like >> > STARFIVE_PWM_, and below structs and function names are also still >> > using starfive_pwm_ptc_ >> > where I'd expect starfive_pwm_. Please be consistant in your naming. >> > >> >> +struct starfive_pwm_ptc_device { >> >> + struct pwm_chip chip; >> >> + struct clk *clk; >> >> + struct reset_control *rst; >> >> + void __iomem *regs; >> >> + u32 clk_rate; /* PWM APB clock frequency */ >> >> +}; >> >> + >> >> +static inline struct starfive_pwm_ptc_device * >> >> +chip_to_starfive_ptc(struct pwm_chip *chip) >> >> + >> >> +{ >> >> + return container_of(chip, struct starfive_pwm_ptc_device, chip); >> >> +} >> >> + >> >> +static int starfive_pwm_ptc_get_state(struct pwm_chip *chip, >> >> + struct pwm_device *dev, >> >> + struct pwm_state *state) >> >> +{ >> >> + struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip); >> >> + u32 period_data, duty_data, ctrl_data; >> >> + >> >> + period_data = readl(REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm)); >> >> + duty_data = readl(REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm)); >> >> + ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm)); >> >> + >> >> + state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate); >> >> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate); >> >> + state->polarity = PWM_POLARITY_INVERSED; >> >> + state->enabled = (ctrl_data & PTC_EN) ? true : false; >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static int starfive_pwm_ptc_apply(struct pwm_chip *chip, >> >> + struct pwm_device *dev, >> >> + const struct pwm_state *state) >> >> +{ >> >> + struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip); >> >> + u32 period_data, duty_data, ctrl_data = 0; >> >> + >> >> + if (state->polarity != PWM_POLARITY_INVERSED) >> >> + return -EINVAL; >> >> + >> >> + period_data = DIV_ROUND_CLOSEST_ULL(state->period * pwm->clk_rate, >> >> + NSEC_PER_SEC); >> >> + duty_data = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pwm->clk_rate, >> >> + NSEC_PER_SEC); >> >> + >> >> + writel(period_data, REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm)); >> >> + writel(duty_data, REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm)); >> >> + writel(0, REG_PTC_RPTC_CNTR(pwm->regs, dev->hwpwm)); >> >> + >> >> + ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm)); >> >> + if (state->enabled) >> >> + writel(ctrl_data | PTC_EN | PTC_OE, REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm)); >> >> + else >> >> + writel(ctrl_data & ~(PTC_EN | PTC_OE), REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm)); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static const struct pwm_ops starfive_pwm_ptc_ops = { >> >> + .get_state = starfive_pwm_ptc_get_state, >> >> + .apply = starfive_pwm_ptc_apply, >> >> + .owner = THIS_MODULE, >> >> +}; >> >> + >> >> +static int starfive_pwm_ptc_probe(struct platform_device *pdev) >> >> +{ >> >> + struct device *dev = &pdev->dev; >> >> + struct starfive_pwm_ptc_device *pwm; >> >> + struct pwm_chip *chip; >> >> + int ret; >> >> + >> >> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL); >> >> + if (!pwm) >> >> + return -ENOMEM; >> >> + >> >> + chip = &pwm->chip; >> >> + chip->dev = dev; >> >> + chip->ops = &starfive_pwm_ptc_ops; >> >> + chip->npwm = 8; >> >> + chip->of_pwm_n_cells = 3; >> >> + >> >> + pwm->regs = devm_platform_ioremap_resource(pdev, 0); >> >> + if (IS_ERR(pwm->regs)) >> >> + return dev_err_probe(dev, PTR_ERR(pwm->regs), >> >> + "Unable to map IO resources\n"); >> >> + >> >> + pwm->clk = devm_clk_get_enabled(dev, NULL); >> >> + if (IS_ERR(pwm->clk)) >> >> + return dev_err_probe(dev, PTR_ERR(pwm->clk), >> >> + "Unable to get pwm's clock\n"); >> >> + >> >> + pwm->rst = devm_reset_control_get_exclusive(dev, NULL); >> >> + if (IS_ERR(pwm->rst)) >> >> + return dev_err_probe(dev, PTR_ERR(pwm->rst), >> >> + "Unable to get pwm's reset\n"); >> >> + >> >> + ret = reset_control_deassert(pwm->rst); >> >> + if (ret) { >> >> + dev_err(dev, "Failed to enable clock for pwm: %d\n", ret); >> >> + return ret; >> >> + } >> >> + >> >> + pwm->clk_rate = clk_get_rate(pwm->clk); >> >> + if (pwm->clk_rate <= 0) { >> >> + dev_warn(dev, "Failed to get APB clock rate\n"); >> >> + return -EINVAL; >> >> + } >> >> + >> >> + ret = devm_pwmchip_add(dev, chip); >> >> + if (ret < 0) { >> >> + dev_err(dev, "Cannot register PTC: %d\n", ret); >> >> + clk_disable_unprepare(pwm->clk); >> >> + reset_control_assert(pwm->rst); >> >> + return ret; >> >> + } >> >> + >> >> + platform_set_drvdata(pdev, pwm); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static int starfive_pwm_ptc_remove(struct platform_device *dev) >> >> +{ >> >> + struct starfive_pwm_ptc_device *pwm = platform_get_drvdata(dev); >> >> + >> >> + reset_control_assert(pwm->rst); >> >> + clk_disable_unprepare(pwm->clk); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static const struct of_device_id starfive_pwm_ptc_of_match[] = { >> >> + { .compatible = "starfive,jh7100-pwm" }, >> >> + { .compatible = "starfive,jh7110-pwm" }, >> >> + { /* sentinel */ } >> >> +}; >> >> +MODULE_DEVICE_TABLE(of, starfive_pwm_ptc_of_match); >> >> + >> >> +static struct platform_driver starfive_pwm_ptc_driver = { >> >> + .probe = starfive_pwm_ptc_probe, >> >> + .remove = starfive_pwm_ptc_remove, >> >> + .driver = { >> >> + .name = "pwm-starfive-ptc", >> > >> > Here >> > >> >> + .of_match_table = starfive_pwm_ptc_of_match, >> >> + }, >> >> +}; >> >> +module_platform_driver(starfive_pwm_ptc_driver); >> >> + >> >> +MODULE_AUTHOR("Jieqin Chen"); >> >> +MODULE_AUTHOR("Hal Feng "); >> >> +MODULE_DESCRIPTION("StarFive PWM PTC driver"); >> > >> > ..and here you're also still calling the driver PTC without explaining why. >> > >> >> +MODULE_LICENSE("GPL"); >> >> -- >> >> 2.34.1 >> >>