From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Stefan Agner <stefan@agner.ch>
Cc: Lukasz Majewski <lukma@denx.de>,
Thierry Reding <thierry.reding@gmail.com>,
Sascha Hauer <s.hauer@pengutronix.de>,
linux-pwm@vger.kernel.org,
Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>,
linux-kernel@vger.kernel.org,
Fabio Estevam <fabio.estevam@nxp.com>,
Fabio Estevam <festevam@gmail.com>,
Lothar Wassmann <LW@karo-electronics.de>,
kernel@pengutronix.de
Subject: Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2
Date: Tue, 3 Jan 2017 20:35:51 +0100 [thread overview]
Message-ID: <20170103203551.737e3a45@bbrezillon> (raw)
In-Reply-To: <7401088f38dc214449fe541e43185fda@agner.ch>
On Tue, 03 Jan 2017 09:29:40 -0800
Stefan Agner <stefan@agner.ch> wrote:
> On 2017-01-03 04:46, Boris Brezillon wrote:
> <snip>
> >> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
> >> > both agreed that most of the code was unneeded when all we want to do
> >> > is disable the PWM.
> >>
> >> So for the PATCH 7/11 we fix the issue with recalculating clocks
> >> when we want to disable PWM.
> >>
> >> if (state->enabled) {
> >> c = clk_get_rate(imx->clk_per);
> >> c *= state->period;
> >>
> >> do_div(c, 1000000000);
> >> period_cycles = c;
> >>
> >> prescale = period_cycles / 0x10000 + 1;
> >>
> >> period_cycles /= prescale;
> >> c = (unsigned long long)period_cycles *
> >> state->duty_cycle;
> >> do_div(c, state->period);
> >> duty_cycles = c;
> >>
> >> /*
> >> * According to imx pwm RM, the real period value
> >> * should be PERIOD value in PWMPR plus 2.
> >> */
> >> if (period_cycles > 2)
> >> period_cycles -= 2;
> >> else
> >> period_cycles = 0;
> >>
> >> /*
> >> * Enable the clock if the PWM is not already
> >> * enabled.
> >> */
> >> if (!cstate.enabled) {
> >> ret = clk_prepare_enable(imx->clk_per);
> >> if (ret)
> >> return ret;
> >> }
> >>
> >> /*
> >> * Wait for a free FIFO slot if the PWM is already
> >> * enabled, and flush the FIFO if the PWM was disabled
> >> * and is about to be enabled.
> >> */
> >> if (cstate.enabled)
> >> imx_pwm_wait_fifo_slot(chip, pwm);
> >> else
> >> imx_pwm_sw_reset(chip);
> >>
> >> writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> >> writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> >>
> >> writel(MX3_PWMCR_PRESCALER(prescale) |
> >> MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> >> MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
> >> MX3_PWMCR_EN,
> >> imx->mmio_base + MX3_PWMCR);
> >> } else {
> >>
> >> writel(0, imx->mmio_base + MX3_PWMCR);
> >>
> >> /* Disable the clock if the PWM is currently enabled. */
> >> if (cstate.enabled)
> >> clk_disable_unprepare(imx->clk_per);
> >> }
> >>
> >>
> >
> > Yep.
> >
>
> This looks like a good transformation of the current Patch 7, but once
> you merge my patch, it will look slightly different...
Yes. I think we should just unconditionally enable/disable the per_clk
at function entry/exit. The prepare_enable() call is almost free
when the clk is already enabled, so it's not like we're adding a huge
overhead by doing that.
>
> >>
> >> >
> >> > My concern was more about the way PWM changes are applied (->apply()
> >> > returns before the change is actually applied), but I agreed that it
> >> > could be fixed later on (if other people think it's really needed),
> >> > since the existing code already handles it this way.
> >>
> >> This is the issue with FIFO setting - but for now we do not deal with
> >> it.
> >
> > Exactly.
> >
> >>
> >> >
> >> > > No clear decision what to change until today when Stefan prepared
> >> > > separate (concise) patch (now I see what is the problem).
> >> > >
> >> >
> >> > The patch proposed by Stefan is addressing a different problem: the
> >> > periph clock has to be enabled before accessing registers.
> >>
> >> So for this reason Stefan's patch [1] always enable the clock no matter
> >> if PWM clock is generated or not.
> >
> > Yes.
> >
> >>
> >> >
> >> > >
> >> > > >
> >> > > > Same goes for the regression introduced in patch 2: I think it's
> >> > > > better to keep things bisectable on all platforms (even if it
> >> > > > appeared to work by chance on imx7, it did work before this
> >> > > > change).
> >> > >
> >> > > Could you be more specific about your idea to solve this problem?
> >> >
> >> > Stefan already provided a patch, I just think it should be fixed
> >> > before patch 2 to avoid breaking bisectibility.
> >>
> >> My idea is as follows:
> >>
> >> I will drop patch v2 (prepared by Sasha) and then squash Stefan's patch
> >> [1] to patch 7/11. The "old" ipg enable code will be removed with other
> >> not needed code during conversion.
> >
> > How about keeping patch 2 but enabling/disabling the periph clk
> > in imx_pwm_config() instead of completely dropping the enable/disable
> > clk sequence.
> >
> > In patch 7 you just add the logic we talked about earlier:
> > unconditionally enable the periph clk when entering the
> > imx_pwm_apply_v2() function and disable it before leaving the function.
> >
> > This way you can preserve bisectibility and still get rid of the ipg
> > clk.
> >
> > Stefan, what's your opinion?
>
> We will get rid of the ipg clocks anyway in patch 8 (which removes those
> functions completely).
>
> So I think Lukasz approach should be fine, just drop patch 2 and squash
> my patch into patch 7.
Well, the end result will be same (ipg_clk will be gone after patch 8),
but then it's hard to track why this clock suddenly disappeared.
I still think it's worth adding an extra commit explaining that
enabling the per_clk before accessing IP registers is needed on some
platforms (imx7), and that IPG clk is actually not required until we
start using it as a source for the PWM signal generation.
Maybe I'm the only one to think so. In this case, feel free to drop
patch 2.
Regards,
Boris
next prev parent reply other threads:[~2017-01-03 19:36 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-23 21:45 [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver Lukasz Majewski
2016-10-23 21:45 ` [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation Lukasz Majewski
2016-10-24 15:21 ` Boris Brezillon
2016-10-25 5:54 ` Sascha Hauer
2016-10-25 6:27 ` Boris Brezillon
2016-10-25 6:32 ` Sascha Hauer
2016-10-25 6:42 ` Boris Brezillon
2016-10-25 6:55 ` Lukasz Majewski
2016-10-23 21:45 ` [PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a separate function Lukasz Majewski
2016-10-24 15:23 ` Boris Brezillon
2016-10-24 21:02 ` Lukasz Majewski
2016-10-23 21:45 ` [PATCH 3/6] pwm: imx: Move PWMv2 wait for fifo slot " Lukasz Majewski
2016-10-24 15:23 ` Boris Brezillon
2016-10-23 21:45 ` [PATCH 4/6] pwm: imx: Provide atomic PWM support for IMXv2 PWM Lukasz Majewski
2016-10-24 15:25 ` Boris Brezillon
2016-10-23 21:45 ` [PATCH 5/6] pwm: imx: Remove redundant IMX PWMv2 code Lukasz Majewski
2016-10-24 15:27 ` Boris Brezillon
2016-10-23 21:45 ` [PATCH 6/6] pwm: imx: Introduce "polarity_supported" flag to PWMv2 driver Lukasz Majewski
2016-10-24 15:28 ` Boris Brezillon
2016-10-24 15:34 ` Boris Brezillon
2016-10-24 21:14 ` Lukasz Majewski
2016-10-25 6:37 ` Boris Brezillon
2016-10-25 6:58 ` Lukasz Majewski
2016-10-24 15:36 ` [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver Boris Brezillon
2016-10-24 21:26 ` Lukasz Majewski
2016-10-25 3:41 ` Stefan Agner
2016-10-25 7:07 ` Lukasz Majewski
2016-10-25 17:08 ` Fabio Estevam
2016-10-25 17:09 ` Fabio Estevam
2016-10-27 5:59 ` Lukasz Majewski
2016-12-26 22:55 ` [PATCH v3 RESEND 00/11] " Lukasz Majewski
2016-12-26 22:55 ` [PATCH v3 RESEND 01/11] pwm: print error messages with pr_err() instead of pr_debug() Lukasz Majewski
2016-12-26 22:55 ` [PATCH v3 RESEND 02/11] pwm: imx: remove ipg clock Lukasz Majewski
2016-12-27 7:26 ` Stefan Agner
2016-12-27 7:54 ` Lukasz Majewski
2016-12-28 22:01 ` Lukasz Majewski
2016-12-29 7:22 ` Stefan Agner
2016-12-29 10:06 ` Lukasz Majewski
2016-12-26 22:55 ` [PATCH v3 RESEND 03/11] pwm: imx: Add separate set of pwm ops for PWMv1 and PWMv2 Lukasz Majewski
2016-12-26 22:55 ` [PATCH v3 RESEND 04/11] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation Lukasz Majewski
2016-12-26 22:55 ` [PATCH v3 RESEND 05/11] pwm: imx: Move PWMv2 software reset code to a separate function Lukasz Majewski
2016-12-26 22:55 ` [PATCH v3 RESEND 06/11] pwm: imx: Move PWMv2 wait for fifo slot " Lukasz Majewski
2016-12-26 22:55 ` [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2 Lukasz Majewski
2016-12-29 16:21 ` Boris Brezillon
2016-12-29 16:45 ` Lukasz Majewski
2016-12-29 17:08 ` Boris Brezillon
2017-01-03 11:43 ` Lukasz Majewski
2017-01-03 12:46 ` Boris Brezillon
2017-01-03 17:29 ` Stefan Agner
2017-01-03 19:35 ` Boris Brezillon [this message]
2017-01-03 22:01 ` Lukasz Majewski
2017-01-03 22:18 ` Boris Brezillon
2017-01-03 22:46 ` Lukasz Majewski
2017-01-03 23:34 ` Boris Brezillon
2017-01-04 11:29 ` Lukasz Majewski
2016-12-26 22:55 ` [PATCH v3 RESEND 08/11] pwm: imx: Remove redundant i.MX PWMv2 code Lukasz Majewski
2016-12-26 22:55 ` [PATCH v3 RESEND 09/11] pwm: core: make the PWM_POLARITY flag in DTB optional Lukasz Majewski
2016-12-26 22:56 ` [PATCH v3 RESEND 10/11] pwm: imx: doc: Update imx-pwm.txt documentation entry Lukasz Majewski
2016-12-26 22:56 ` [PATCH v3 RESEND 11/11] pwm: imx: Add polarity inversion support to i.MX's PWMv2 Lukasz Majewski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170103203551.737e3a45@bbrezillon \
--to=boris.brezillon@free-electrons.com \
--cc=LW@karo-electronics.de \
--cc=bhuvanchandra.dv@toradex.com \
--cc=fabio.estevam@nxp.com \
--cc=festevam@gmail.com \
--cc=kernel@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=lukma@denx.de \
--cc=s.hauer@pengutronix.de \
--cc=stefan@agner.ch \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).