From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 50115F364A6 for ; Thu, 9 Apr 2026 17:31:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=7ygPbcjQbk8WsUkKsvStvZAeAx3SckpX7RZUl7+fUFI=; b=nCL7mC5q/D9DII2uQCzR4nSXkT DlMECcb2BJyM0Bi8I006Ch5kYCs6mbHaFIYXt5YueU0R9M/bElfHY/9VfjboEhQkur9Fh4/0su9aE D56CFyRPWwPuy0WjpBx6yNL6TmUX1cYAbArgA3DO6Gtw47yAs2Wu8bp3FmGWZdPLRzNNJYqfl88zS LeIXTq+xqMZIXiBoSsdM8C3A/kaqUudUMmm8H/YEDilz7gyjZCfGHyuPSemMf5XUcJGcDjwYKWeDm ZmGX7THBy8MGigD+bFwTp22AnAos36RRQItWolyOIqgmF9/5E44Na3myWDebFqpMoi9GE8s2kJLrD zcMDfsHg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1wAtDQ-0000000B1a2-0wKg; Thu, 09 Apr 2026 17:30:52 +0000 Received: from leonov.paulk.fr ([185.233.101.22]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1wAtDM-0000000B1Z7-2m5q for linux-arm-kernel@lists.infradead.org; Thu, 09 Apr 2026 17:30:51 +0000 Received: from laika.paulk.fr (12.234.24.109.rev.sfr.net [109.24.234.12]) by leonov.paulk.fr (Postfix) with ESMTPS id B4F271F80044 for ; Thu, 9 Apr 2026 17:30:44 +0000 (UTC) Received: by laika.paulk.fr (Postfix, from userid 65534) id E5213B401D1; Thu, 9 Apr 2026 17:30:42 +0000 (UTC) Received: from shepard (unknown [192.168.1.1]) by laika.paulk.fr (Postfix) with ESMTPSA id 83915B401B9; Thu, 9 Apr 2026 17:30:41 +0000 (UTC) Date: Thu, 9 Apr 2026 19:30:38 +0200 From: Paul Kocialkowski To: Richard Genoud Cc: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Philipp Zabel , Thomas Petazzoni , John Stultz , Joao Schim , linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 2/4] pwm: sun50i: Add H616 PWM support Message-ID: References: <20260305091959.2530374-1-richard.genoud@bootlin.com> <20260305091959.2530374-3-richard.genoud@bootlin.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="33pU5tb6RmcLLlSB" Content-Disposition: inline In-Reply-To: <20260305091959.2530374-3-richard.genoud@bootlin.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260409_103049_065782_9EF3D6E0 X-CRM114-Status: GOOD ( 30.34 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --33pU5tb6RmcLLlSB Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Richard, On Thu 05 Mar 26, 10:19, Richard Genoud wrote: > +/* PWM IRQ Enable Register */ > +#define H616_PWM_IER 0x0 I think it would make more sense to keep the full register names from the manual after the suffix and stick to them. It makes things easier when comparing the code with documentation or the reference implementation. So something like SUN8I_PWM_PIER here. > + > +/* PWM IRQ Status Register */ > +#define H616_PWM_ISR 0x4 > + > +/* PWM Capture IRQ Enable Register */ > +#define H616_PWM_CIER 0x10 > + > +/* PWM Capture IRQ Status Register */ > +#define H616_PWM_CISR 0x14 > + > +/* PWMCC Pairs Clock Configuration Registers */ > +#define H616_PWM_XY_CLK_CR(pair) (0x20 + ((pair) * 0x4)) > +#define H616_PWM_XY_CLK_CR_SRC_SHIFT 7 > +#define H616_PWM_XY_CLK_CR_SRC_MASK 1 > +#define H616_PWM_XY_CLK_CR_GATE_BIT 4 > +#define H616_PWM_XY_CLK_CR_BYPASS_BIT(chan) ((chan) % 2 + 5) > +#define H616_PWM_XY_CLK_CR_DIV_M_SHIFT 0 > + > +/* PWMCC Pairs Dead Zone Control Registers */ > +#define H616_PWM_XY_DZ(pair) (0x30 + ((pair) * 0x4)) > + > +/* PWM Enable Register */ > +#define H616_PWM_ENR 0x40 > +#define H616_PWM_ENABLE(x) BIT(x) > + > +/* PWM Capture Enable Register */ > +#define H616_PWM_CER 0x44 > + > +/* PWM Control Register */ > +#define H616_PWM_CTRL_REG(chan) (0x60 + (chan) * 0x20) You're sometimes calling the register offset _REG and sometimes not. Both options are fine but you need to keep it consistent across the whole definitions. I would be enclined to not use it after using the register nam= es coming from the manual as suggested above. Also you're sometimes using "chan", sometimes "ch" for the argument to the register macros. This is inconsistent and you might as well just use "c" everywhere so it doesn't take too much space. > +#define H616_PWM_CTRL_PRESCAL_K_SHIFT 0 > +#define H616_PWM_CTRL_PRESCAL_K_WIDTH 8 > +#define H616_PWM_CTRL_ACTIVE_STATE BIT(8) > + > +/* PWM Period Register */ > +#define H616_PWM_PERIOD_REG(ch) (0x64 + (ch) * 0x20) > +#define H616_PWM_PERIOD_MASK GENMASK(31, 16) > +#define H616_PWM_DUTY_MASK GENMASK(15, 0) > +#define H616_PWM_REG_PERIOD(reg) (FIELD_GET(H616_PWM_PERIOD_MASK, reg) += 1) > +#define H616_PWM_REG_DUTY(reg) FIELD_GET(H616_PWM_DUTY_MASK, reg) > +#define H616_PWM_PERIOD(prd) FIELD_PREP(H616_PWM_PERIOD_MASK, (prd) - 1) > +#define H616_PWM_DUTY(dty) FIELD_PREP(H616_PWM_DUTY_MASK, dty) > +#define H616_PWM_PERIOD_MAX (FIELD_MAX(H616_PWM_PERIOD_MASK) + 1) Using REG as a prefix feels a bit confusing here. I would rather see: #define SUN8I_PWM_PPR(c) (0x64 + (c) * 0x20) #define SUN8I_PWM_PPR_PERIOD(p) FIELD_PREP(...) #define SUN8I_PWM_PPR_PERIOD_VALUE(r) FIELD_GET(...) #define SUN8I_PWN_PPR_PERIOD_MAX FIELD_MAX(...) #define SUN8I_PWM_PPR_DUTY(d) FIELD_PREP(...) #define SUN8I_PWM_PPR_DUTY_VALUE(r) FIELD_GET(...) > + > +/* PWM Count Register */ > +#define H616_PWM_CNT_REG(x) (0x68 + (x) * 0x20) > + > +/* PWM Capture Control Register */ > +#define H616_PWM_CCR(x) (0x6c + (x) * 0x20) > + > +/* PWM Capture Rise Lock Register */ > +#define H616_PWM_CRLR(x) (0x70 + (x) * 0x20) > + > +/* PWM Capture Fall Lock Register */ > +#define H616_PWM_CFLR(x) (0x74 + (x) * 0x20) > + > +#define H616_PWM_PAIR_IDX(chan) ((chan) >> 2) > + > +/* > + * Block diagram of the PWM clock controller: > + * > + * _____ ______ ________ > + * OSC24M --->| | | | | | > + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> H616_PWM_clock_src= _xy > + * |_____| |______| |________| > + * ________ > + * | | > + * +->| /div_k |---> H616_PWM_clock_x > + * | |________| > + * | ______ > + * | | | > + * +-->| Gate |----> H616_PWM_bypass_clock_x > + * | |______| > + * H616_PWM_clock_src_xy ----+ ________ > + * | | | > + * +->| /div_k |---> H616_PWM_clock_y > + * | |________| > + * | ______ > + * | | | > + * +-->| Gate |----> H616_PWM_bypass_clock_y > + * |______| > + * > + * NB: when the bypass is set, all the PWM logic is bypassed. > + * So, the duty cycle and polarity can't be modified (we just have a clo= ck). > + * The bypass in PWM mode is used to achieve a 1/2 relative duty cycle w= ith the > + * fastest clock. > + * > + * H616_PWM_clock_x/y serve for the PWM purpose. > + * H616_PWM_bypass_clock_x/y serve for the clock-provider purpose. > + * > + */ > + > +/* > + * Table used for /div_m (diviser before obtaining H616_PWM_clock_src_xy) > + * It's actually CLK_DIVIDER_POWER_OF_TWO, but limited to /256 > + */ > +#define CLK_TABLE_DIV_M_ENTRY(i) { \ > + .val =3D (i), .div =3D 1 << (i) \ > +} > + > +static const struct clk_div_table clk_table_div_m[] =3D { > + CLK_TABLE_DIV_M_ENTRY(0), > + CLK_TABLE_DIV_M_ENTRY(1), > + CLK_TABLE_DIV_M_ENTRY(2), > + CLK_TABLE_DIV_M_ENTRY(3), > + CLK_TABLE_DIV_M_ENTRY(4), > + CLK_TABLE_DIV_M_ENTRY(5), > + CLK_TABLE_DIV_M_ENTRY(6), > + CLK_TABLE_DIV_M_ENTRY(7), > + CLK_TABLE_DIV_M_ENTRY(8), > + { /* sentinel */ } > +}; > + > +#define H616_PWM_XY_SRC_GATE(_pair, _reg) \ > +struct clk_gate gate_xy_##_pair =3D { \ > + .reg =3D (void *)(_reg), \ > + .bit_idx =3D H616_PWM_XY_CLK_CR_GATE_BIT, \ > + .hw.init =3D &(struct clk_init_data){ \ > + .ops =3D &clk_gate_ops, \ > + } \ > +} > + > +#define H616_PWM_XY_SRC_MUX(_pair, _reg) \ > +struct clk_mux mux_xy_##_pair =3D { \ > + .reg =3D (void *)(_reg), \ > + .shift =3D H616_PWM_XY_CLK_CR_SRC_SHIFT, \ > + .mask =3D H616_PWM_XY_CLK_CR_SRC_MASK, \ > + .flags =3D CLK_MUX_ROUND_CLOSEST, \ > + .hw.init =3D &(struct clk_init_data){ \ > + .ops =3D &clk_mux_ops, \ > + } \ > +} > + > +#define H616_PWM_XY_SRC_DIV(_pair, _reg) \ > +struct clk_divider rate_xy_##_pair =3D { \ > + .reg =3D (void *)(_reg), \ > + .shift =3D H616_PWM_XY_CLK_CR_DIV_M_SHIFT, \ > + .table =3D clk_table_div_m, \ > + .hw.init =3D &(struct clk_init_data){ \ > + .ops =3D &clk_divider_ops, \ > + } \ > +} > + > +#define H616_PWM_X_DIV(_idx, _reg) \ > +struct clk_divider rate_x_##_idx =3D { \ > + .reg =3D (void *)(_reg), \ > + .shift =3D H616_PWM_CTRL_PRESCAL_K_SHIFT, \ > + .width =3D H616_PWM_CTRL_PRESCAL_K_WIDTH, \ > + .hw.init =3D &(struct clk_init_data){ \ > + .ops =3D &clk_divider_ops, \ > + } \ > +} > + > +#define H616_PWM_X_BYPASS_GATE(_idx) \ > +struct clk_gate gate_x_bypass_##_idx =3D { \ > + .reg =3D (void *)H616_PWM_ENR, \ > + .bit_idx =3D _idx, \ > + .hw.init =3D &(struct clk_init_data){ \ > + .ops =3D &clk_gate_ops, \ > + } \ > +} > + > +#define H616_PWM_XY_CLK_SRC(_pair, _reg) \ > + static H616_PWM_XY_SRC_MUX(_pair, _reg); \ > + static H616_PWM_XY_SRC_GATE(_pair, _reg); \ > + static H616_PWM_XY_SRC_DIV(_pair, _reg) > + > +#define H616_PWM_X_CLK(_idx) \ > + static H616_PWM_X_DIV(_idx, H616_PWM_CTRL_REG(_idx)) > + > +#define H616_PWM_X_BYPASS_CLK(_idx) \ > + H616_PWM_X_BYPASS_GATE(_idx) > + > +#define REF_CLK_XY_SRC(_pair) \ > + { \ > + .name =3D "pwm-clk-src" #_pair, \ > + .mux_hw =3D &mux_xy_##_pair.hw, \ > + .gate_hw =3D &gate_xy_##_pair.hw, \ > + .rate_hw =3D &rate_xy_##_pair.hw, \ > + } > + > +#define REF_CLK_X(_idx, _pair) \ > + { \ > + .name =3D "pwm-clk" #_idx, \ > + .parent_names =3D (const char *[]){ "pwm-clk-src" #_pair }, \ > + .num_parents =3D 1, \ > + .rate_hw =3D &rate_x_##_idx.hw, \ > + .flags =3D CLK_SET_RATE_PARENT, \ > + } > + > +#define REF_CLK_BYPASS(_idx, _pair) \ > + { \ > + .name =3D "pwm-clk-bypass" #_idx, \ > + .parent_names =3D (const char *[]){ "pwm-clk-src" #_pair }, \ > + .num_parents =3D 1, \ > + .gate_hw =3D &gate_x_bypass_##_idx.hw, \ > + .flags =3D CLK_SET_RATE_PARENT, \ > + } > + > +/* > + * H616_PWM_clock_src_xy generation: > + * _____ ______ ________ > + * OSC24M --->| | | | | | > + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> H616_PWM_clock_src= _xy > + * |_____| |______| |________| > + */ > +H616_PWM_XY_CLK_SRC(01, H616_PWM_XY_CLK_CR(0)); > +H616_PWM_XY_CLK_SRC(23, H616_PWM_XY_CLK_CR(1)); > +H616_PWM_XY_CLK_SRC(45, H616_PWM_XY_CLK_CR(2)); > + > +/* > + * H616_PWM_clock_x_div generation: > + * ________ > + * | | H616_PWM_clock_x/y > + * H616_PWM_clock_src_xy --->| /div_k |---------------> > + * |________| > + */ > +H616_PWM_X_CLK(0); > +H616_PWM_X_CLK(1); > +H616_PWM_X_CLK(2); > +H616_PWM_X_CLK(3); > +H616_PWM_X_CLK(4); > +H616_PWM_X_CLK(5); > + > +/* > + * H616_PWM_bypass_clock_xy generation: > + * ______ > + * | | > + * H616_PWM_clock_src_xy ---->| Gate |-------> H616_PWM_bypass_clock_x > + * |______| > + * > + * The gate is actually H616_PWM_ENR register. > + */ > +H616_PWM_X_BYPASS_CLK(0); > +H616_PWM_X_BYPASS_CLK(1); > +H616_PWM_X_BYPASS_CLK(2); > +H616_PWM_X_BYPASS_CLK(3); > +H616_PWM_X_BYPASS_CLK(4); > +H616_PWM_X_BYPASS_CLK(5); > + > +struct clk_pwm_data { > + const char *name; > + const char **parent_names; > + unsigned int num_parents; > + struct clk_hw *mux_hw; > + struct clk_hw *rate_hw; > + struct clk_hw *gate_hw; > + unsigned long flags; > +}; > + > +#define CLK_BYPASS(h616chip, ch) ((h616chip)->data->npwm + (ch)) > +#define CLK_XY_SRC_IDX(h616chip, ch) ((h616chip)->data->npwm * 2 + ((ch)= >> 1)) > +static struct clk_pwm_data pwmcc_data[] =3D { > + REF_CLK_X(0, 01), > + REF_CLK_X(1, 01), > + REF_CLK_X(2, 23), > + REF_CLK_X(3, 23), > + REF_CLK_X(4, 45), > + REF_CLK_X(5, 45), > + REF_CLK_BYPASS(0, 01), > + REF_CLK_BYPASS(1, 01), > + REF_CLK_BYPASS(2, 23), > + REF_CLK_BYPASS(3, 23), > + REF_CLK_BYPASS(4, 45), > + REF_CLK_BYPASS(5, 45), > + REF_CLK_XY_SRC(01), > + REF_CLK_XY_SRC(23), > + REF_CLK_XY_SRC(45), > + { /* sentinel */ } > +}; We'll probably need a way to tie these static definitions to a particular instance of the unit for a given chip. But I guess that can be done later when adding more chips to the driver. I'm not too versed in the clk and pwm APIs but the rest generally looks good to me. All the best, Paul --=20 Paul Kocialkowski, Independent contractor - sys-base - https://www.sys-base.io/ Free software developer - https://www.paulk.fr/ Expert in multimedia, graphics and embedded hardware support with Linux. --33pU5tb6RmcLLlSB Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEAbcMXZQMtj1fphLChP3B6o/ulQwFAmnX4j4ACgkQhP3B6o/u lQwdTQ//ch93yoRTG9gLChC5/R+m1lTPe1/tIOk1Vi4GIILWe8IA+aJSf72kNsRx vMFalFHp44nFK4GIRAfx+r9zrZ+mLR3lQM4wsxGp2K65pPrmciw5qSky1v0l1ZRC eCOafd2S2ZBYq0Epzb1xje7piuqZp2ZDgbrquUWMeE39PcpHcC/miVPWTnbGB/5w VBPFbIfLlMNvGcv57l0t3zK8X3HnK60Tjlaqdi/iKdRAewm77tM+5SgyeMU4P7yd BGjiJn3FzonBTt64mWGSyjW1vjNAhiPRHklOkAUvbfBdRotlXoTZZieokinELK5v 9/+38h/bpAlObLlIEmOCBAM4EPfGvVeX3iWIfiAoaxH7bh5jU6G5EuR4q+zWJRP8 yNCW8vyn6jgAlq9p9hVc8J72mUvL8s1qidgxnErW583/wKhQY6au92l5uZkYykLi 5eNF8jian3f/Hnh6ZweXfD2/SLFL4I+zO9Ez2jX3d4z748JrH9XiV6odQlyqHcYT duFqWWDNGD83wa+1/7yDeSN05u+Z/t3F56qJBycA/ESiOJneTHhccXnbNpKsLPpQ MeHraPw+D/tYUAQMxdeywU2HkVp704gGPOxOZn77ga6dSLr+As4eFa6eYgrDeD8R VJXulqovR4Ob8C0tyrWn8LYJTGR+HTngeHVWweyrJ6vtYqJcdCI= =v+2L -----END PGP SIGNATURE----- --33pU5tb6RmcLLlSB--