| Message ID | 20190726184045.14669-6-jernej.skrabec@siol.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: sun4i: Add support for Allwinner H6 | expand |
On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote: > PWM core has an option to bypass whole logic and output unchanged source > clock as PWM output. This is achieved by enabling bypass bit. > > Note that when bypass is enabled, no other setting has any meaning, not > even enable bit. > > This mode of operation is needed to achieve high enough frequency to > serve as clock source for AC200 chip, which is integrated into same > package as H6 SoC. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> It doesn't seem to be available on the A10 (at least) though. The A13 seem to have it, so you should probably check that, and make that conditional to the compatible if not available on all of them. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Dne sobota, 27. julij 2019 ob 12:50:08 CEST je Maxime Ripard napisal(a): > On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote: > > PWM core has an option to bypass whole logic and output unchanged source > > clock as PWM output. This is achieved by enabling bypass bit. > > > > Note that when bypass is enabled, no other setting has any meaning, not > > even enable bit. > > > > This mode of operation is needed to achieve high enough frequency to > > serve as clock source for AC200 chip, which is integrated into same > > package as H6 SoC. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > It doesn't seem to be available on the A10 (at least) though. The A13 > seem to have it, so you should probably check that, and make that > conditional to the compatible if not available on all of them. Ok, can you suggest the name for the quirk? "has_bypass" is suspiciously similar to "has_prescaler_bypass". Also, how to name these sun4i_pwm_data structures? Now that there are (will be) three new quirks, name of the structure would be just too long, like "sun50i_pwm_dual_prescaler_bypass_clk_rst_bypass". Best regards, Jernej > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Sat, Jul 27, 2019 at 10:28 PM Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Dne sobota, 27. julij 2019 ob 12:50:08 CEST je Maxime Ripard napisal(a): > > On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote: > > > PWM core has an option to bypass whole logic and output unchanged source > > > clock as PWM output. This is achieved by enabling bypass bit. > > > > > > Note that when bypass is enabled, no other setting has any meaning, not > > > even enable bit. > > > > > > This mode of operation is needed to achieve high enough frequency to > > > serve as clock source for AC200 chip, which is integrated into same > > > package as H6 SoC. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > It doesn't seem to be available on the A10 (at least) though. The A13 > > seem to have it, so you should probably check that, and make that > > conditional to the compatible if not available on all of them. > > Ok, can you suggest the name for the quirk? "has_bypass" is suspiciously > similar to "has_prescaler_bypass". has_direct_mod_clk_output? > Also, how to name these sun4i_pwm_data structures? Now that there are (will > be) three new quirks, name of the structure would be just too long, like > "sun50i_pwm_dual_prescaler_bypass_clk_rst_bypass". Just use the SoC model. Any later ones that have the same quirks will likely use the same compatible string anyway. ChenYu
On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote: > PWM core has an option to bypass whole logic and output unchanged source > clock as PWM output. This is achieved by enabling bypass bit. > > Note that when bypass is enabled, no other setting has any meaning, not > even enable bit. > > This mode of operation is needed to achieve high enough frequency to > serve as clock source for AC200 chip, which is integrated into same > package as H6 SoC. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/pwm/pwm-sun4i.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > index 9e0eca79ff88..848cff26f385 100644 > --- a/drivers/pwm/pwm-sun4i.c > +++ b/drivers/pwm/pwm-sun4i.c > @@ -120,6 +120,19 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip, > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > + /* > + * PWM chapter in H6 manual has a diagram which explains that if bypass > + * bit is set, no other setting has any meaning. Even more, experiment > + * proved that also enable bit is ignored in this case. > + */ > + if (val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) { > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate); > + state->duty_cycle = state->period / 2; > + state->polarity = PWM_POLARITY_NORMAL; > + state->enabled = true; > + return; > + } > + > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) && > sun4i_pwm->data->has_prescaler_bypass) > prescaler = 1; > @@ -211,7 +224,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > { > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > struct pwm_state cstate; > - u32 ctrl; > + u32 ctrl, clk_rate; > + bool bypass; > int ret; > unsigned int delay_us; > unsigned long now; > @@ -226,6 +240,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > } > } > > + /* > + * Although it would make much more sense to check for bypass in > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled". > + * Period is allowed to be rounded up or down. > + */ Every driver seems to implement rounding the way its driver considers it sensible. @Thierry: This is another patch where it would be good to have a global directive about how rounding is supposed to work to provide the users an reliable and uniform way to work with PWMs. > + clk_rate = clk_get_rate(sun4i_pwm->clk); > + bypass = (state->period == NSEC_PER_SEC / clk_rate || > + state->period == DIV_ROUND_UP(NSEC_PER_SEC, clk_rate)) && > + state->enabled; Not sure if the compiler is clever enough to notice the obvious optimisation with this code construct, but you can write this test in a more clever way which has zero instead of up to two divisions. Something like: bypass = ((state->period * clk_rate >= NSEC_PER_SEC && state->period * clk_rate < NSEC_PER_SEC + clk_rate) && state->enabled); In the commit log you write the motivation for using bypass is that it allows to implement higher frequency then with the "normal" operation. As you don't skip calculating the normal parameters requesting such a high-frequency setting either errors out or doesn't catch the impossible request. In both cases there is something to fix. > + > spin_lock(&sun4i_pwm->ctrl_lock); > ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > @@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm); > } > > + if (bypass) > + ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm); > + else > + ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm); > + Does switching on (or off) the bypass bit complete the currently running period? > sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG); > > spin_unlock(&sun4i_pwm->ctrl_lock); Best regards Uwe
Hi Uwe, Dne ponedeljek, 29. julij 2019 ob 09:06:05 CEST je Uwe Kleine-König napisal(a): > On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote: > > PWM core has an option to bypass whole logic and output unchanged source > > clock as PWM output. This is achieved by enabling bypass bit. > > > > Note that when bypass is enabled, no other setting has any meaning, not > > even enable bit. > > > > This mode of operation is needed to achieve high enough frequency to > > serve as clock source for AC200 chip, which is integrated into same > > package as H6 SoC. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/pwm/pwm-sun4i.c | 31 ++++++++++++++++++++++++++++++- > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > index 9e0eca79ff88..848cff26f385 100644 > > --- a/drivers/pwm/pwm-sun4i.c > > +++ b/drivers/pwm/pwm-sun4i.c > > @@ -120,6 +120,19 @@ static void sun4i_pwm_get_state(struct pwm_chip > > *chip, > > > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > > > + /* > > + * PWM chapter in H6 manual has a diagram which explains that if bypass > > + * bit is set, no other setting has any meaning. Even more, experiment > > + * proved that also enable bit is ignored in this case. > > + */ > > + if (val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) { > > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate); > > + state->duty_cycle = state->period / 2; > > + state->polarity = PWM_POLARITY_NORMAL; > > + state->enabled = true; > > + return; > > + } > > + > > > > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) && > > > > sun4i_pwm->data->has_prescaler_bypass) > > > > prescaler = 1; > > > > @@ -211,7 +224,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, > > struct pwm_device *pwm,> > > { > > > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > > struct pwm_state cstate; > > > > - u32 ctrl; > > + u32 ctrl, clk_rate; > > + bool bypass; > > > > int ret; > > unsigned int delay_us; > > unsigned long now; > > > > @@ -226,6 +240,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, > > struct pwm_device *pwm,> > > } > > > > } > > > > + /* > > + * Although it would make much more sense to check for bypass in > > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled". > > + * Period is allowed to be rounded up or down. > > + */ > > Every driver seems to implement rounding the way its driver considers it > sensible. @Thierry: This is another patch where it would be good to have > a global directive about how rounding is supposed to work to provide the > users an reliable and uniform way to work with PWMs. > > > + clk_rate = clk_get_rate(sun4i_pwm->clk); > > + bypass = (state->period == NSEC_PER_SEC / clk_rate || > > + state->period == DIV_ROUND_UP(NSEC_PER_SEC, clk_rate)) && > > + state->enabled; > > Not sure if the compiler is clever enough to notice the obvious > optimisation with this code construct, but you can write this test in a > more clever way which has zero instead of up to two divisions. Something > like: > > bypass = ((state->period * clk_rate >= NSEC_PER_SEC && > state->period * clk_rate < NSEC_PER_SEC + clk_rate) && > state->enabled); > > In the commit log you write the motivation for using bypass is that it > allows to implement higher frequency then with the "normal" operation. > As you don't skip calculating the normal parameters requesting such a > high-frequency setting either errors out or doesn't catch the impossible > request. In both cases there is something to fix. It's the latter, otherwise it wouldn't work for my case. I'll fix the check and skip additional logic. > > > + > > > > spin_lock(&sun4i_pwm->ctrl_lock); > > ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > > > @@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, > > struct pwm_device *pwm,> > > ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm); > > > > } > > > > + if (bypass) > > + ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm); > > + else > > + ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm); > > + > > Does switching on (or off) the bypass bit complete the currently running > period? I don't really know. If I understand correctly, it just bypasses PWM logic completely, so I would say it doesn't complete the currently running period. Take a look at chapter 3.9.2 http://linux-sunxi.org/ File:Allwinner_H6_V200_User_Manual_V1.1.pdf Best regards, Jernej > > > sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG); > > > > spin_unlock(&sun4i_pwm->ctrl_lock); > > Best regards > Uwe
Hello, On Mon, Jul 29, 2019 at 06:16:55PM +0200, Jernej Škrabec wrote: > Dne ponedeljek, 29. julij 2019 ob 09:06:05 CEST je Uwe Kleine-König > napisal(a): > > On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote: > > > PWM core has an option to bypass whole logic and output unchanged source > > > clock as PWM output. This is achieved by enabling bypass bit. > > > > > > Note that when bypass is enabled, no other setting has any meaning, not > > > even enable bit. > > > > > > This mode of operation is needed to achieve high enough frequency to > > > serve as clock source for AC200 chip, which is integrated into same > > > package as H6 SoC. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > drivers/pwm/pwm-sun4i.c | 31 ++++++++++++++++++++++++++++++- > > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > index 9e0eca79ff88..848cff26f385 100644 > > > --- a/drivers/pwm/pwm-sun4i.c > > > +++ b/drivers/pwm/pwm-sun4i.c > > > @@ -120,6 +120,19 @@ static void sun4i_pwm_get_state(struct pwm_chip > > > *chip, > > > > > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > > > > > + /* > > > + * PWM chapter in H6 manual has a diagram which explains that if bypass > > > + * bit is set, no other setting has any meaning. Even more, experiment > > > + * proved that also enable bit is ignored in this case. > > > + */ > > > + if (val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) { > > > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate); > > > + state->duty_cycle = state->period / 2; > > > + state->polarity = PWM_POLARITY_NORMAL; > > > + state->enabled = true; > > > + return; > > > + } > > > + > > > > > > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) && > > > > > > sun4i_pwm->data->has_prescaler_bypass) > > > > > > prescaler = 1; > > > > > > @@ -211,7 +224,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, > > > struct pwm_device *pwm,> > > > { > > > > > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > > > struct pwm_state cstate; > > > > > > - u32 ctrl; > > > + u32 ctrl, clk_rate; > > > + bool bypass; > > > > > > int ret; > > > unsigned int delay_us; > > > unsigned long now; > > > > > > @@ -226,6 +240,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, > > > struct pwm_device *pwm,> > > > } > > > > > > } > > > > > > + /* > > > + * Although it would make much more sense to check for bypass in > > > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled". > > > + * Period is allowed to be rounded up or down. > > > + */ > > > > Every driver seems to implement rounding the way its driver considers it > > sensible. @Thierry: This is another patch where it would be good to have > > a global directive about how rounding is supposed to work to provide the > > users an reliable and uniform way to work with PWMs. > > > > > + clk_rate = clk_get_rate(sun4i_pwm->clk); > > > + bypass = (state->period == NSEC_PER_SEC / clk_rate || > > > + state->period == DIV_ROUND_UP(NSEC_PER_SEC, clk_rate)) && > > > + state->enabled; > > > > Not sure if the compiler is clever enough to notice the obvious > > optimisation with this code construct, but you can write this test in a > > more clever way which has zero instead of up to two divisions. Something > > like: > > > > bypass = ((state->period * clk_rate >= NSEC_PER_SEC && > > state->period * clk_rate < NSEC_PER_SEC + clk_rate) && > > state->enabled); > > > > In the commit log you write the motivation for using bypass is that it > > allows to implement higher frequency then with the "normal" operation. > > As you don't skip calculating the normal parameters requesting such a > > high-frequency setting either errors out or doesn't catch the impossible > > request. In both cases there is something to fix. > > It's the latter, otherwise it wouldn't work for my case. I'll fix the check and > skip additional logic. Great. > > > + > > > > > > spin_lock(&sun4i_pwm->ctrl_lock); > > > ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > > > > > @@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, > > > struct pwm_device *pwm,> > > > ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm); > > > > > > } > > > > > > + if (bypass) > > > + ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm); > > > + else > > > + ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm); > > > + > > > > Does switching on (or off) the bypass bit complete the currently running > > period? > > I don't really know. If I understand correctly, it just bypasses PWM logic > completely, so I would say it doesn't complete the currently running period. This is a bug. It's part of the promise of the PWM API that started periods are completed. Please at least document this limitation at the top of the driver. drivers/pwm/pwm-sifive.c has an example you might want to use as a template. Best regards Uwe
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c index 9e0eca79ff88..848cff26f385 100644 --- a/drivers/pwm/pwm-sun4i.c +++ b/drivers/pwm/pwm-sun4i.c @@ -120,6 +120,19 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip, val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); + /* + * PWM chapter in H6 manual has a diagram which explains that if bypass + * bit is set, no other setting has any meaning. Even more, experiment + * proved that also enable bit is ignored in this case. + */ + if (val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) { + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate); + state->duty_cycle = state->period / 2; + state->polarity = PWM_POLARITY_NORMAL; + state->enabled = true; + return; + } + if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) && sun4i_pwm->data->has_prescaler_bypass) prescaler = 1; @@ -211,7 +224,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, { struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); struct pwm_state cstate; - u32 ctrl; + u32 ctrl, clk_rate; + bool bypass; int ret; unsigned int delay_us; unsigned long now; @@ -226,6 +240,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, } } + /* + * Although it would make much more sense to check for bypass in + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled". + * Period is allowed to be rounded up or down. + */ + clk_rate = clk_get_rate(sun4i_pwm->clk); + bypass = (state->period == NSEC_PER_SEC / clk_rate || + state->period == DIV_ROUND_UP(NSEC_PER_SEC, clk_rate)) && + state->enabled; + spin_lock(&sun4i_pwm->ctrl_lock); ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); @@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm); } + if (bypass) + ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm); + else + ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm); + sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG); spin_unlock(&sun4i_pwm->ctrl_lock);
PWM core has an option to bypass whole logic and output unchanged source clock as PWM output. This is achieved by enabling bypass bit. Note that when bypass is enabled, no other setting has any meaning, not even enable bit. This mode of operation is needed to achieve high enough frequency to serve as clock source for AC200 chip, which is integrated into same package as H6 SoC. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/pwm/pwm-sun4i.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)