Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote: > Add the flag and corresponding documentation for PWM_USAGE_POWER. > > Cc: Rob Herring > Signed-off-by: Clemens Gruber > --- > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++ > include/dt-bindings/pwm/pwm.h | 1 + > 2 files changed, 4 insertions(+) Rob, what are your thoughts on this? I've been thinking about this some more and I'm having second thoughts about putting this into device tree because it doesn't actually describe a property of the PWM hardware but rather a use-case specific hint. It's a bit of a gray area because this is just part of the PWM specifier which already has use-case specific "configuration", such as the period and the polarity. Perhaps a better place for this is within the PWM API? We could add the same information into struct pwm_state and then consumers that don't care about specifics of the signal (such as pwm-backlight) can set that flag when they request a state to be applied. Thierry signature.asc Description: PGP signature
Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
On Fri, Apr 16, 2021 at 11:32:12AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
>
> On Thu, Apr 15, 2021 at 06:27:02PM +0200, Thierry Reding wrote:
> > On Tue, Apr 13, 2021 at 07:56:31PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Apr 13, 2021 at 01:51:15PM +0200, Thierry Reding wrote:
> > > > On Mon, Apr 12, 2021 at 06:27:23PM +0200, Uwe Kleine-König wrote:
> > > > > On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote:
> > > > > > Cc: Rob Herring
> > > > > > Signed-off-by: Clemens Gruber
> > > > > > ---
> > > > > > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++
> > > > > > include/dt-bindings/pwm/pwm.h | 1 +
> > > > > > 2 files changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt
> > > > > > b/Documentation/devicetree/bindings/pwm/pwm.txt
> > > > > > index 084886bd721e..fe3a28f887c0 100644
> > > > > > --- a/Documentation/devicetree/bindings/pwm/pwm.txt
> > > > > > +++ b/Documentation/devicetree/bindings/pwm/pwm.txt
> > > > > > @@ -46,6 +46,9 @@ period in nanoseconds.
> > > > > > Optionally, the pwm-specifier can encode a number of flags
> > > > > > (defined in
> > > > > > ) in a third cell:
> > > > > > - PWM_POLARITY_INVERTED: invert the PWM signal polarity
> > > > > > +- PWM_USAGE_POWER: Only care about the power output of the signal.
> > > > > > This
> > > > > > + allows drivers (if supported) to optimize the signals, for
> > > > > > example to
> > > > > > + improve EMI and reduce current spikes.
> > > > >
> > > > > IMHO there are too many open questions about which freedom this gives
> > > > > to
> > > > > the lowlevel driver. If the consumer requests .duty_cycle = 25ns +
> > > > > .period = 100ns, can the driver provide .duty_cycle = 25s + .period =
> > > > > 100s which nominally has the same power output? Let's not introduce
> > > > > more
> > > > > ambiguity than there already is.
> > > >
> > > > The freedom given to the driver should be to adjust the signal within
> > > > reasonable bounds. Changing the time unit by a factor of 10 is
> > > > not within reason, and I doubt anyone would interpret it that way, even
> > > > if we didn't document this at all.
> > >
> > > Please define a rule that allows to judge if any given implementation is
> > > correct or not. For the record neither "within reasonable bounds" nor "a
> > > factor of 10 is not within reason" is good enough.
> >
> > We haven't had any rules thus far and I have yet to see a single report
> > that drivers get this completely wrong. So "within reason", which I
> > think is what driver authors will do by default, is good enough in
> > practice.
>
> For me commit 11fc4edc483b ("pwm: bcm2835: Improve precision of PWM")
> indicates that there is a problem. Someone used the pwm-ir-tx driver on
> top of pwm-bcm2835. The former driver's expectation is that
>
> period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
> duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100);
> pwm_config(pwm, duty, period);
>
> yields a frequency near pwm_ir->carrier (minimizing
>
> abs(1 / implemented_freq - 1 / pwm_ir->carrier)
>
> (or should it minimize abs(implemented_freq - pwm_ir->carrier) instead?
> Not entirely sure.). The result was that the pwm-bcm2835 driver was
> changed to implement a different rounding. I took the time to look at
> the drivers in 11fc4edc483b^, with the following result:
>
> - pwm-ab8500.c: ignores period_ns in .config
> - pwm-atmel.c: rounds down period
> - pwm-atmel-hlcdc.c: rounds up period (unsure)
> - pwm-atmel-tcb.c: rounds down period (unsure)
> - pwm-bcm2835.c: rounds down period
> - pwm-bcm-iproc.c: rounds down period
> - pwm-bcm-kona.c: rounds down period
> - pwm-berlin.c: rounds down period
> - pwm-brcmstb.c: rounds down period
> - pwm-clps711x.c: doesn't support changing period (IMHO in a buggy way
>because the period in the dts is just overwritten)
> - pwm-crc: rounds down period
> - pwm-cros-ec.c: doesn't support changing period
> - pwm-ep93xx.c: rounds down period
> - pwm-fsl-ftm.c: rounds down period
Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
On Tue, Apr 13, 2021 at 07:56:31PM +0200, Uwe Kleine-König wrote: > On Tue, Apr 13, 2021 at 01:51:15PM +0200, Thierry Reding wrote: > > On Mon, Apr 12, 2021 at 06:27:23PM +0200, Uwe Kleine-König wrote: > > > On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote: > > > > Add the flag and corresponding documentation for PWM_USAGE_POWER. > > > > > > My concern here in the previous round was that PWM_USAGE_POWER isn't a > > > name that intuitively suggests its semantic. Do you disagree? > > > > I suggested PWM_USAGE_POWER because I think it accurately captures what > > we want here. > > > > > > Cc: Rob Herring > > > > Signed-off-by: Clemens Gruber > > > > --- > > > > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++ > > > > include/dt-bindings/pwm/pwm.h | 1 + > > > > 2 files changed, 4 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt > > > > b/Documentation/devicetree/bindings/pwm/pwm.txt > > > > index 084886bd721e..fe3a28f887c0 100644 > > > > --- a/Documentation/devicetree/bindings/pwm/pwm.txt > > > > +++ b/Documentation/devicetree/bindings/pwm/pwm.txt > > > > @@ -46,6 +46,9 @@ period in nanoseconds. > > > > Optionally, the pwm-specifier can encode a number of flags (defined in > > > > ) in a third cell: > > > > - PWM_POLARITY_INVERTED: invert the PWM signal polarity > > > > +- PWM_USAGE_POWER: Only care about the power output of the signal. This > > > > + allows drivers (if supported) to optimize the signals, for example to > > > > + improve EMI and reduce current spikes. > > > > > > IMHO there are too many open questions about which freedom this gives to > > > the lowlevel driver. If the consumer requests .duty_cycle = 25ns + > > > .period = 100ns, can the driver provide .duty_cycle = 25s + .period = > > > 100s which nominally has the same power output? Let's not introduce more > > > ambiguity than there already is. > > > > The freedom given to the driver should be to adjust the signal within > > reasonable bounds. Changing the time unit by a factor of 10 is > > not within reason, and I doubt anyone would interpret it that way, even > > if we didn't document this at all. > > Please define a rule that allows to judge if any given implementation is > correct or not. For the record neither "within reasonable bounds" nor "a > factor of 10 is not within reason" is good enough. We haven't had any rules thus far and I have yet to see a single report that drivers get this completely wrong. So "within reason", which I think is what driver authors will do by default, is good enough in practice. > This is not only important to be able to review drivers that implement > it, but also for consumers, because they should know what to expect. Again, consumers should expect that the PWM driver will do something that is within reasonable margins. If that ever ends up being wrong for a given use-case we may need to change that. But I don't think it's necessary to take out all flexibility if we don't have to. As long as things work fine there's no reason to make the rules any more strict. > > To be frank I think that quest of yours to try and rid the PWM API of > > all ambiguity is futile. > > I consider my quest about rounding reasonable. And I think this is > painful because when the PWM framework was introduced it was too much ad > hoc and the APIs were not thought through enough. And because I don't > want to have that repeated, I express my concerns here. Maybe try to look at this from another perspective. Maybe what you call adhoc API was actually deliberately designed this way. To be honest I don't know what the intentions were when the original PWM API was created, that was way before I took on maintenance of the PWM subsystem. The PWM framework adopted the existing API and there was no reason to change it because it worked just fine. And I still don't see a reason for the API to change. Like I said, if we ever run into a case where the current flexibility gets in the way and yields unpredictable or unusable results, then that's something we have to improve. But I don't think we should make any such changes if they're not necessary, because then we may end up making matters worse. Also, I think this actually corroborates the need for something like the usage flags in the PWM specifier. Currently drivers will do their best to generate a PWM signal that's as close as possible to the requested parameters. If that's not enough for a specific use-case, t
Re: [PATCH] drm/tegra: Fix shift overflow in tegra_shared_plane_atomic_update
On Thu, Apr 15, 2021 at 08:29:14AM -0700, Nathan Chancellor wrote:
> Clang warns:
>
> drivers/gpu/drm/tegra/hub.c:513:11: warning: shift count >= width of
> type [-Wshift-count-overflow]
> base |= BIT(39);
> ^~~
>
> BIT is unsigned long, which is 32-bit on ARCH=arm, hence the overflow
> warning. Switch to BIT_ULL, which is 64-bit and will not overflow.
>
> Fixes: 7b6f846785f4 ("drm/tegra: Support sector layout on Tegra194")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1351
> Signed-off-by: Nathan Chancellor
> ---
> drivers/gpu/drm/tegra/hub.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
This code never runs on 32-bit platforms, so another option would be to
not try and build this on 32-bit configurations either. But none of the
rest of the code is built conditionally, so fixing this is preferable.
Applied, thanks.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 03/30] drm/tegra: Don't register DP AUX channels before connectors
On Fri, Feb 19, 2021 at 04:52:59PM -0500, Lyude Paul wrote: > As pointed out by the documentation for drm_dp_aux_register(), > drm_dp_aux_init() should be used in situations where the AUX channel for a > display driver can potentially be registered before it's respective DRM > driver. This is the case with Tegra, since the DP aux channel exists as a > platform device instead of being a grandchild of the DRM device. > > Since we're about to add a backpointer to a DP AUX channel's respective DRM > device, let's fix this so that we don't potentially allow userspace to use > the AUX channel before we've associated it with it's DRM connector. > > Signed-off-by: Lyude Paul > --- > drivers/gpu/drm/tegra/dpaux.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c > index 105fb9cdbb3b..ea56c6ec25e4 100644 > --- a/drivers/gpu/drm/tegra/dpaux.c > +++ b/drivers/gpu/drm/tegra/dpaux.c > @@ -534,9 +534,7 @@ static int tegra_dpaux_probe(struct platform_device *pdev) > dpaux->aux.transfer = tegra_dpaux_transfer; > dpaux->aux.dev = >dev; > > - err = drm_dp_aux_register(>aux); > - if (err < 0) > - return err; > + drm_dp_aux_init(>aux); I just noticed that this change causes an error on some setups that I haven't seen before. The problem is that the SOR driver tries to grab a reference to the I2C device to make sure it doesn't go away while it has a pointer to it. However, since now the I2C adapter hasn't been registered yet, I get this: [ 15.013969] kobject: '(null)' (5c903e43): is not initialized, yet kobject_get() is being called. I recall that you wanted to make this change so that a backpointer to the DRM device could be added (I think that's patch 15 of the series), but I didn't see that patch get merged, so it's a bit difficult to try and fix this up. Has the situation changed? Do we no longer need the backpointer? If we still want it, what's the plan for merging the change? Should I work under the assumption that patch will make it in sometime and try to fix this on top of that? I'm thinking that perhaps we can move the I2C adapter registration into drm_dp_aux_init() since that's independent of the DRM device. It would also make a bit more sense from the Tegra driver's point of view where all devices would be created during the ->probe() path, and only during the ->init() path would the connection between DRM device and DRM DP AUX device be established. Thierry signature.asc Description: PGP signature
Re: Regression v5.12-rc3: net: stmmac: re-init rx buffers when mac resume back
On Wed, Apr 14, 2021 at 02:18:58AM +, Joakim Zhang wrote: > > > -Original Message- > > From: Thierry Reding > > Sent: 2021年4月14日 0:07 > > To: David S. Miller ; Jakub Kicinski > > Cc: Joakim Zhang ; Jon Hunter > > ; Giuseppe Cavallaro ; > > Alexandre Torgue ; Jose Abreu > > ; net...@vger.kernel.org; Linux Kernel Mailing List > > ; linux-tegra > > Subject: Re: Regression v5.12-rc3: net: stmmac: re-init rx buffers when mac > > resume back > > > > On Tue, Apr 13, 2021 at 12:13:01PM +, Joakim Zhang wrote: > > > > > > Hi Jon, > > > > > > > -Original Message- > > > > From: Jon Hunter > > > > Sent: 2021年4月13日 16:41 > > > > To: Joakim Zhang ; Giuseppe Cavallaro > > > > ; Alexandre Torgue > > > > ; Jose Abreu > > > > Cc: net...@vger.kernel.org; Linux Kernel Mailing List > > > > ; linux-tegra > > > > ; Jakub Kicinski > > > > Subject: Re: Regression v5.12-rc3: net: stmmac: re-init rx buffers > > > > when mac resume back > > > > > > > > > > > > On 01/04/2021 17:28, Jon Hunter wrote: > > > > > > > > > > On 31/03/2021 12:41, Joakim Zhang wrote: > > > > > > > > > > ... > > > > > > > > > >>> In answer to your question, resuming from suspend does work on > > > > >>> this board without your change. We have been testing > > > > >>> suspend/resume now on this board since Linux v5.8 and so we have > > > > >>> the ability to bisect such regressions. So it is clear to me > > > > >>> that this is the change that caused > > > > this, but I am not sure why. > > > > >> > > > > >> Yes, I know this issue is regression caused by my patch. I just > > > > >> want to > > > > analyze the potential reasons. Due to the code change only related > > > > to the page recycle and reallocate. > > > > >> So I guess if this page operate need IOMMU works when IOMMU is > > enabled. > > > > Could you help check if IOMMU driver resume before STMMAC? Our > > > > common desire is to find the root cause, right? > > > > > > > > > > > > > > > Yes of course that is the desire here indeed. I had assumed that > > > > > the suspend/resume order was good because we have never seen any > > > > > problems, but nonetheless it is always good to check. Using ftrace > > > > > I enabled tracing of the appropriate suspend/resume functions and > > > > > this is what I see ... > > > > > > > > > > # tracer: function > > > > > # > > > > > # entries-in-buffer/entries-written: 4/4 #P:6 > > > > > # > > > > > #_-=> irqs-off > > > > > # / _=> need-resched > > > > > # | / _---=> hardirq/softirq > > > > > # || / _--=> preempt-depth > > > > > # ||| / delay > > > > > # TASK-PID CPU# TIMESTAMP FUNCTION > > > > > # | | | | | > > > > > rtcwake-748 [000] ...1 536.700777: > > > > stmmac_pltfr_suspend <-platform_pm_suspend > > > > > rtcwake-748 [000] ...1 536.735532: > > > > arm_smmu_pm_suspend <-platform_pm_suspend > > > > > rtcwake-748 [000] ...1 536.757290: > > > > arm_smmu_pm_resume <-platform_pm_resume > > > > > rtcwake-748 [003] ...1 536.856771: > > > > stmmac_pltfr_resume <-platform_pm_resume > > > > > > > > > > > > > > > So I don't see any ordering issues that could be causing this. > > > > > > > > > > > > Another thing I have found is that for our platform, if the driver > > > > for the ethernet PHY (in this case broadcom PHY) is enabled, then it > > > > fails to resume but if I disable the PHY in the kernel > > > > configuration, then resume works. I have found that if I move the > > > > reinit of the RX buffers to before the startup of the phy, then it can > > > > resume > > OK with the PHY enabled. > > > > > >
Re: Regression v5.12-rc3: net: stmmac: re-init rx buffers when mac resume back
On Tue, Apr 13, 2021 at 12:13:01PM +, Joakim Zhang wrote:
>
> Hi Jon,
>
> > -Original Message-
> > From: Jon Hunter
> > Sent: 2021年4月13日 16:41
> > To: Joakim Zhang ; Giuseppe Cavallaro
> > ; Alexandre Torgue ;
> > Jose Abreu
> > Cc: net...@vger.kernel.org; Linux Kernel Mailing List
> > ; linux-tegra ;
> > Jakub Kicinski
> > Subject: Re: Regression v5.12-rc3: net: stmmac: re-init rx buffers when mac
> > resume back
> >
> >
> > On 01/04/2021 17:28, Jon Hunter wrote:
> > >
> > > On 31/03/2021 12:41, Joakim Zhang wrote:
> > >
> > > ...
> > >
> > >>> In answer to your question, resuming from suspend does work on this
> > >>> board without your change. We have been testing suspend/resume now
> > >>> on this board since Linux v5.8 and so we have the ability to bisect
> > >>> such regressions. So it is clear to me that this is the change that
> > >>> caused
> > this, but I am not sure why.
> > >>
> > >> Yes, I know this issue is regression caused by my patch. I just want to
> > analyze the potential reasons. Due to the code change only related to the
> > page
> > recycle and reallocate.
> > >> So I guess if this page operate need IOMMU works when IOMMU is enabled.
> > Could you help check if IOMMU driver resume before STMMAC? Our common
> > desire is to find the root cause, right?
> > >
> > >
> > > Yes of course that is the desire here indeed. I had assumed that the
> > > suspend/resume order was good because we have never seen any problems,
> > > but nonetheless it is always good to check. Using ftrace I enabled
> > > tracing of the appropriate suspend/resume functions and this is what I
> > > see ...
> > >
> > > # tracer: function
> > > #
> > > # entries-in-buffer/entries-written: 4/4 #P:6
> > > #
> > > #_-=> irqs-off
> > > # / _=> need-resched
> > > # | / _---=> hardirq/softirq
> > > # || / _--=> preempt-depth
> > > # ||| / delay
> > > # TASK-PID CPU# TIMESTAMP FUNCTION
> > > # | | | | |
> > > rtcwake-748 [000] ...1 536.700777:
> > stmmac_pltfr_suspend <-platform_pm_suspend
> > > rtcwake-748 [000] ...1 536.735532:
> > arm_smmu_pm_suspend <-platform_pm_suspend
> > > rtcwake-748 [000] ...1 536.757290:
> > arm_smmu_pm_resume <-platform_pm_resume
> > > rtcwake-748 [003] ...1 536.856771:
> > stmmac_pltfr_resume <-platform_pm_resume
> > >
> > >
> > > So I don't see any ordering issues that could be causing this.
> >
> >
> > Another thing I have found is that for our platform, if the driver for the
> > ethernet
> > PHY (in this case broadcom PHY) is enabled, then it fails to resume but if I
> > disable the PHY in the kernel configuration, then resume works. I have found
> > that if I move the reinit of the RX buffers to before the startup of the
> > phy, then
> > it can resume OK with the PHY enabled.
> >
> > Does the following work for you? Does your platform use a specific ethernet
> > PHY driver?
>
> I am also looking into this issue these days, we use the Realtek RTL8211FDI
> PHY, driver is drivers/net/phy/realtek.c.
>
> For our EQOS MAC integrated in our SoC, Rx side logic depends on RXC clock
> from PHY, so we need phylink_start before MAC.
>
> I will test below code change tomorrow to see if it can work at my side,
> since it is only re-init memory, need not RXC clock.
>
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 208cae344ffa..071d15d86dbe 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5416,19 +5416,20 @@ int stmmac_resume(struct device *dev)
> > return ret;
> > }
> > + rtnl_lock();
> > + mutex_lock(>lock);
> > + stmmac_reinit_rx_buffers(priv);
> > + mutex_unlock(>lock);
> > +
> > if (!device_may_wakeup(priv->device) || !priv->plat->pmt) {
> > - rtnl_lock();
> > phylink_start(priv->phylink);
> > /* We may have called phylink_speed_down before */
> > phylink_speed_up(priv->phylink);
> > - rtnl_unlock();
> > }
> > - rtnl_lock();
> > mutex_lock(>lock);
> > stmmac_reset_queues_param(priv);
> > - stmmac_reinit_rx_buffers(priv);
> > stmmac_free_tx_skbufs(priv);
> > stmmac_clear_descriptors(priv);
> >
> >
> > It is still not clear to us why the existing call to
> > stmmac_clear_descriptors() is not sufficient to fix your problem.
>
> During suspend/resume stress test, I found rx descriptor may not refill when
> system suspended, rx descriptor could be: 008 [0xc4310080]: 0x0 0x40
> 0x0 0x34010040.
> When system resume back,
Re: [PATCH] staging: media: tegra-vde: Align line break to match with the open parenthesis in file trace.h
On Mon, Apr 12, 2021 at 07:20:40PM -0300, Aline Santana Cordeiro wrote:
> Align line break to match with the open parenthesis.
> Issue detected by checkpatch.pl.
> It consequently solved a few end lines with a '(',
> issue also detected by checkpatch.pl
>
> Signed-off-by: Aline Santana Cordeiro
> ---
> drivers/staging/media/tegra-vde/trace.h | 111
> ++--
> 1 file changed, 50 insertions(+), 61 deletions(-)
Ugh... I'd say this is one case where checkpatch.pl really shouldn't be
complaining since this isn't a function call or declaration. It's more
like a code snippet written with macros, so I don't think the same rules
should apply.
Adding checkpatch folks (hence quoting in full): is there anything we
can do about this without too much effort? Or should we just accept this
the way it is?
Thierry
> diff --git a/drivers/staging/media/tegra-vde/trace.h
> b/drivers/staging/media/tegra-vde/trace.h
> index e571410..1fcc573 100644
> --- a/drivers/staging/media/tegra-vde/trace.h
> +++ b/drivers/staging/media/tegra-vde/trace.h
> @@ -11,79 +11,68 @@
> #include "vde.h"
>
> DECLARE_EVENT_CLASS(register_access,
> - TP_PROTO(struct tegra_vde *vde, void __iomem *base,
> - u32 offset, u32 value),
> - TP_ARGS(vde, base, offset, value),
> - TP_STRUCT__entry(
> - __string(hw_name, tegra_vde_reg_base_name(vde, base))
> - __field(u32, offset)
> - __field(u32, value)
> - ),
> - TP_fast_assign(
> - __assign_str(hw_name, tegra_vde_reg_base_name(vde, base));
> - __entry->offset = offset;
> - __entry->value = value;
> - ),
> - TP_printk("%s:0x%03x 0x%08x", __get_str(hw_name), __entry->offset,
> - __entry->value)
> + TP_PROTO(struct tegra_vde *vde, void __iomem *base,
> + u32 offset, u32 value),
> + TP_ARGS(vde, base, offset, value),
> + TP_STRUCT__entry(__string(hw_name,
> tegra_vde_reg_base_name(vde, base))
> + __field(u32, offset)
> + __field(u32, value)
> + ),
> + TP_fast_assign(__assign_str(hw_name,
> +
> tegra_vde_reg_base_name(vde, base));
> +__entry->offset = offset;
> +__entry->value = value;
> + ),
> + TP_printk("%s:0x%03x 0x%08x", __get_str(hw_name),
> __entry->offset,
> + __entry->value)
> );
>
> DEFINE_EVENT(register_access, vde_writel,
> - TP_PROTO(struct tegra_vde *vde, void __iomem *base,
> - u32 offset, u32 value),
> - TP_ARGS(vde, base, offset, value));
> + TP_PROTO(struct tegra_vde *vde, void __iomem *base,
> + u32 offset, u32 value),
> + TP_ARGS(vde, base, offset, value));
> DEFINE_EVENT(register_access, vde_readl,
> - TP_PROTO(struct tegra_vde *vde, void __iomem *base,
> - u32 offset, u32 value),
> - TP_ARGS(vde, base, offset, value));
> + TP_PROTO(struct tegra_vde *vde, void __iomem *base,
> + u32 offset, u32 value),
> + TP_ARGS(vde, base, offset, value));
>
> TRACE_EVENT(vde_setup_iram_entry,
> - TP_PROTO(unsigned int table, unsigned int row, u32 value, u32 aux_addr),
> - TP_ARGS(table, row, value, aux_addr),
> - TP_STRUCT__entry(
> - __field(unsigned int, table)
> - __field(unsigned int, row)
> - __field(u32, value)
> - __field(u32, aux_addr)
> - ),
> - TP_fast_assign(
> - __entry->table = table;
> - __entry->row = row;
> - __entry->value = value;
> - __entry->aux_addr = aux_addr;
> - ),
> - TP_printk("[%u][%u] = { 0x%08x (flags = \"%s\", frame_num = %u); 0x%08x
> }",
> - __entry->table, __entry->row, __entry->value,
> - __print_flags(__entry->value, " ", { (1 << 25), "B" }),
> - __entry->value & 0x7F, __entry->aux_addr)
> + TP_PROTO(unsigned int table, unsigned int row, u32 value, u32
> aux_addr),
> + TP_ARGS(table, row, value, aux_addr),
> + TP_STRUCT__entry(__field(unsigned int, table)
> + __field(unsigned int, row)
> + __field(u32, value)
> + __field(u32, aux_addr)
> + ),
> + TP_fast_assign(__entry->table = table;
> +__entry->row = row;
> +__entry->value = value;
> +__entry->aux_addr = aux_addr;
> + ),
> + TP_printk("[%u][%u] = { 0x%08x (flags =
Re: [PATCH v3 1/4] dt-bindings: pwm: convert pwm-rockchip.txt to YAML
On Mon, Apr 12, 2021 at 10:01:52PM +0200, Johan Jonker wrote: > Current dts files with 'pwm' nodes are manually verified. > In order to automate this process pwm-rockchip.txt > has to be converted to yaml. > > Signed-off-by: Johan Jonker > --- > For some SoC nodes this patch serie generates notifications > for undocumented "interrupts" properties shared between > PWM channels till there is consensus of what to do with it or > someone makes a solution for the whole PWM block. > > Changed V3: > fix mistake with compatibles introduced in V2 > Changed V2: > changed schema for clocks and clock-names > --- > .../devicetree/bindings/pwm/pwm-rockchip.txt | 27 --- > .../devicetree/bindings/pwm/pwm-rockchip.yaml | 88 > ++ > 2 files changed, 88 insertions(+), 27 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/pwm/pwm-rockchip.txt > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml Heiko, do you want to pick up patches 1 & 2 into your tree along with 3 & 4? If so: Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v3 2/4] dt-bindings: pwm: add more compatible strings to pwm-rockchip.yaml
On Mon, Apr 12, 2021 at 10:01:53PM +0200, Johan Jonker wrote: > The compatible strings below are already in use in the Rockchip > dtsi files, but were somehow never added to a document, so add > > "rockchip,rk3328-pwm" > > "rockchip,rk3036-pwm", "rockchip,rk2928-pwm" > > "rockchip,rk3368-pwm", "rockchip,rk3288-pwm" > "rockchip,rk3399-pwm", "rockchip,rk3288-pwm" > > "rockchip,px30-pwm", "rockchip,rk3328-pwm" > "rockchip,rk3308-pwm", "rockchip,rk3328-pwm" > > for pwm nodes to pwm-rockchip.yaml. > > Signed-off-by: Johan Jonker > --- > Note for rob+dt: > A tag was not added on purpose, because by the change of schema > for clocks and clock-names I add "rockchip,rk3328-pwm" to > the "if:", so strictly speaking V1 and (V2) V3 will not be the same. > Please have a look at it again. > > For some SoC nodes this patch serie generates notifications > for undocumented "interrupts" properties shared between > PWM channels till there is consensus of what to do with it or > someone makes a solution for the whole PWM block. > > Changed V3: > fix mistake with compatibles introduced in V2 > Changed V2: > changed schema for clocks and clock-names > --- > Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml | 12 > 1 file changed, 12 insertions(+) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v8 1/8] pwm: pca9685: Switch to atomic API
On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote: > Hi Uwe, > > On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-König wrote: > > Hello Clemens, > > > > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote: > > > On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-König wrote: > > > > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote: > > > > > The switch to the atomic API goes hand in hand with a few fixes to > > > > > previously experienced issues: > > > > > - The duty cycle is no longer lost after disable/enable (previously > > > > > the > > > > > OFF registers were cleared in disable and the user was required to > > > > > call config to restore the duty cycle settings) > > > > > - If one sets a period resulting in the same prescale register value, > > > > > the sleep and write to the register is now skipped > > > > > - Previously, only the full ON bit was toggled in GPIO mode (and full > > > > > OFF cleared if set to high), which could result in both full OFF and > > > > > full ON not being set and on=0, off=0, which is not allowed > > > > > according > > > > > to the datasheet > > > > > - The OFF registers were reset to 0 in probe, which could lead to the > > > > > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF) > > > > > > > > > > Signed-off-by: Clemens Gruber > > > > > --- > > > > > Changes since v7: > > > > > - Moved check for !state->enabled before prescaler configuration > > > > > - Removed unnecessary cast > > > > > - Use DIV_ROUND_DOWN in .apply > > > > > > > > > > Changes since v6: > > > > > - Order of a comparison switched for improved readability > > > > > > > > > > Changes since v5: > > > > > - Function documentation for set_duty > > > > > - Variable initializations > > > > > - Print warning if all LEDs channel > > > > > - Changed EOPNOTSUPP to EINVAL > > > > > - Improved error messages > > > > > - Register reset corrections moved to this patch > > > > > > > > > > Changes since v4: > > > > > - Patches split up > > > > > - Use a single set_duty function > > > > > - Improve readability / new macros > > > > > - Added a patch to restrict prescale changes to the first user > > > > > > > > > > Changes since v3: > > > > > - Refactoring: Extracted common functions > > > > > - Read prescale register value instead of caching it > > > > > - Return all zeros and disabled for "all LEDs" channel state > > > > > - Improved duty calculation / mapping to 0..4096 > > > > > > > > > > Changes since v2: > > > > > - Always set default prescale value in probe > > > > > - Simplified probe code > > > > > - Inlined functions with one callsite > > > > > > > > > > Changes since v1: > > > > > - Fixed a logic error > > > > > - Impoved PM runtime handling and fixed !CONFIG_PM > > > > > - Write default prescale reg value if invalid in probe > > > > > - Reuse full_off/_on functions throughout driver > > > > > - Use cached prescale value whenever possible > > > > > > > > > > drivers/pwm/pwm-pca9685.c | 259 > > > > > +- > > > > > 1 file changed, 89 insertions(+), 170 deletions(-) > > > > > > > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > > > > > index 4a55dc18656c..827b57ced3c2 100644 > > > > > --- a/drivers/pwm/pwm-pca9685.c > > > > > +++ b/drivers/pwm/pwm-pca9685.c > > > > > @@ -51,7 +51,6 @@ > > > > > #define PCA9685_PRESCALE_MAX 0xFF/* => min. frequency of 24 Hz */ > > > > > > > > > > #define PCA9685_COUNTER_RANGE4096 > > > > > -#define PCA9685_DEFAULT_PERIOD 500 /* Default period_ns = > > > > > 1/200 Hz */ > > > > > #define PCA9685_OSC_CLOCK_MHZ25 /* Internal oscillator > > > > > with 25 MHz */ > > > > > > > > > > #define PCA9685_NUMREGS 0xFF > > > > > @@ -71,10 +70,14 @@ > > > > > #define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N))) > > > > > #define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N))) > > > > > > > > > > +#define REG_ON_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_H > > > > > : LED_N_ON_H((C))) > > > > > +#define REG_ON_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_L > > > > > : LED_N_ON_L((C))) > > > > > +#define REG_OFF_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H > > > > > : LED_N_OFF_H((C))) > > > > > +#define REG_OFF_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L > > > > > : LED_N_OFF_L((C))) > > > > > > > > I'd like to see these named PCA9685_REG_ON_H etc. > > > > > > I did not use the prefix because the existing LED_N_ON/OFF_H/L also do > > > not have a prefix. If the prefix is mandatory, I think LED_N_.. should > > > also be prefixed, right? > > > > I'd like to seem the prefixed (and assume that Thierry doesn't agree). > > IMHO it's good style and even though it yields longer name usually it > > yields easier understandable code. (But this seems to be subjective.) > > I am not sure I want to also rename the existing LED_N_OFF stuff
Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
On Mon, Apr 12, 2021 at 06:27:23PM +0200, Uwe Kleine-König wrote: > On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote: > > Add the flag and corresponding documentation for PWM_USAGE_POWER. > > My concern here in the previous round was that PWM_USAGE_POWER isn't a > name that intuitively suggests its semantic. Do you disagree? I suggested PWM_USAGE_POWER because I think it accurately captures what we want here. > > Cc: Rob Herring > > Signed-off-by: Clemens Gruber > > --- > > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++ > > include/dt-bindings/pwm/pwm.h | 1 + > > 2 files changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt > > b/Documentation/devicetree/bindings/pwm/pwm.txt > > index 084886bd721e..fe3a28f887c0 100644 > > --- a/Documentation/devicetree/bindings/pwm/pwm.txt > > +++ b/Documentation/devicetree/bindings/pwm/pwm.txt > > @@ -46,6 +46,9 @@ period in nanoseconds. > > Optionally, the pwm-specifier can encode a number of flags (defined in > > ) in a third cell: > > - PWM_POLARITY_INVERTED: invert the PWM signal polarity > > +- PWM_USAGE_POWER: Only care about the power output of the signal. This > > + allows drivers (if supported) to optimize the signals, for example to > > + improve EMI and reduce current spikes. > > IMHO there are too many open questions about which freedom this gives to > the lowlevel driver. If the consumer requests .duty_cycle = 25ns + > .period = 100ns, can the driver provide .duty_cycle = 25s + .period = > 100s which nominally has the same power output? Let's not introduce more > ambiguity than there already is. The freedom given to the driver should be to adjust the signal within reasonable bounds. Changing the time unit by a factor of 10 is not within reason, and I doubt anyone would interpret it that way, even if we didn't document this at all. To be frank I think that quest of yours to try and rid the PWM API of all ambiguity is futile. I've been trying to be lenient because you seem motivated, but I think you're taking this too far. There are always going to be cases that aren't completely clear-cut and where drivers need the flexibility to cheat in order to be useful at all. If we get to a point where everything needs to be 100% accurate, the majority of the PWM controllers won't be usable at all. Don't let perfect be the enemy of good. Thierry signature.asc Description: PGP signature
Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
On Tue, Apr 13, 2021 at 01:38:05PM +0200, Uwe Kleine-König wrote: > Hello, > > On Mon, Apr 12, 2021 at 06:46:51PM +0200, Clemens Gruber wrote: > > On Mon, Apr 12, 2021 at 06:27:23PM +0200, Uwe Kleine-König wrote: > > > On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote: > > > > Add the flag and corresponding documentation for PWM_USAGE_POWER. > > > > > > My concern here in the previous round was that PWM_USAGE_POWER isn't a > > > name that intuitively suggests its semantic. Do you disagree? > > > > No. It is more abstract and requires documentation. But I also didn't > > want to waste too much time on discussing names, so I used Thierry's > > suggestion. > > If you introduce API thinking about the name before actually introducing > it is a good idea in general. (OK, the name doesn't become part of the > (binary) dt API, but we don't even agree about its semantic here.) > > And IMHO a bad name with a good documentation isn't good enough. > Otherwise we can better just agree on using plain numbers in the .dts > files. Plain numbers or not doesn't change anything. The meaning of the bit has to be defined. This has nothing to do with the symbolic name at all. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
On Fri, Apr 09, 2021 at 06:07:09PM +0900, Nobuhiro Iwamatsu wrote:
> Add driver for the PWM controller on Toshiba Visconti ARM SoC.
>
> Signed-off-by: Nobuhiro Iwamatsu
> ---
> drivers/pwm/Kconfig| 9 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-visconti.c | 193 +
> 3 files changed, 203 insertions(+)
> create mode 100644 drivers/pwm/pwm-visconti.c
Looks good, but I have a few minor comments, see below.
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 9a4f66ae8070..8ae68d6203fb 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -601,6 +601,15 @@ config PWM_TWL_LED
> To compile this driver as a module, choose M here: the module
> will be called pwm-twl-led.
>
> +config PWM_VISCONTI
> + tristate "Toshiba Visconti PWM support"
> + depends on ARCH_VISCONTI || COMPILE_TEST
> + help
> + PWM Subsystem driver support for Toshiba Visconti SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-visconti.
> +
> config PWM_VT8500
> tristate "vt8500 PWM support"
> depends on ARCH_VT8500 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 6374d3b1d6f3..d43b1e17e8e1 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -56,4 +56,5 @@ obj-$(CONFIG_PWM_TIECAP)+= pwm-tiecap.o
> obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
> obj-$(CONFIG_PWM_TWL)+= pwm-twl.o
> obj-$(CONFIG_PWM_TWL_LED)+= pwm-twl-led.o
> +obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o
> obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
> diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> new file mode 100644
> index ..ff4a5f5b0009
> --- /dev/null
> +++ b/drivers/pwm/pwm-visconti.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Toshiba Visconti pulse-width-modulation controller driver
> + *
> + * Copyright (c) 2020 TOSHIBA CORPORATION
> + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> + *
> + * Authors: Nobuhiro Iwamatsu
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Should be sorted alphabetically.
> +
> +#define PIPGM_PCSR(ch) (0x400 + 4 * (ch))
> +#define PIPGM_PDUT(ch) (0x420 + 4 * (ch))
> +#define PIPGM_PWMC(ch) (0x440 + 4 * (ch))
> +
> +#define PIPGM_PWMC_PWMACTBIT(5)
> +#define PIPGM_PWMC_CLK_MASK GENMASK(1, 0)
> +#define PIPGM_PWMC_POLARITY_MASK GENMASK(5, 5)
> +
> +struct visconti_pwm_chip {
> + struct pwm_chip chip;
> + void __iomem *base;
> +};
> +
> +#define to_visconti_chip(chip) \
> + container_of(chip, struct visconti_pwm_chip, chip)
I prefer these to be static inline functions because that tends to give
better error messages than macros. Also, that's what's primarily used in
the PWM drivers, even if there are a couple of outliers.
I'll go fix those up.
> +
> +static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct visconti_pwm_chip *priv = to_visconti_chip(chip);
> + u32 period, duty_cycle, pwmc0;
> +
> + dev_dbg(chip->dev, "%s: ch = %d en = %d p = 0x%llx d = 0x%llx\n",
> __func__,
> + pwm->hwpwm, state->enabled, state->period, state->duty_cycle);
Don't the trace points work for you?
> +
> + /*
> + * pwmc is a 2-bit divider for the input clock running at 1 MHz.
> + * When the settings of the PWM are modified, the new values are
> shadowed in hardware until
> + * the period register (PCSR) is written and the currently running
> period is completed. This
> + * way the hardware switches atomically from the old setting to the new.
> + * Also, disabling the hardware completes the currently running period
> and keeps the output
> + * at low level at all times.
> + */
> + if (!state->enabled) {
> + writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm));
> + return 0;
> + }
> +
> + /*
> + * The biggest period the hardware can provide is
> + * (0x << 3) * 1000 ns
> + * This value fits easily in an u32, so simplify the maths by
> + * capping the values to 32 bit integers.
> + */
> + if (state->period > (0x << 3) * 1000)
> + period = (0x << 3) * 1000;
> + else
> + period = state->period;
> +
> + if (state->duty_cycle > period)
> + duty_cycle = period;
> + else
> + duty_cycle = state->duty_cycle;
> +
> + /*
> + * The input clock runs fixed at 1 MHz, so we have only
> + * microsecond resolution and so can divide by
> + * NSEC_PER_SEC / CLKFREQ = 1000 without loosing precision.
> + */
> + period /= 1000;
> + duty_cycle /= 1000;
> +
> + if (!period)
> + /* period too small
Re: [PATCH v7 3/8] pwm: pca9685: Improve runtime PM behavior
On Tue, Apr 06, 2021 at 06:41:35PM +0200, Clemens Gruber wrote:
> The chip does not come out of POR in active state but in sleep state.
> To be sure (in case the bootloader woke it up) we force it to sleep in
> probe.
>
> On kernels without CONFIG_PM, we wake the chip in .probe and put it to
> sleep in .remove.
>
> Signed-off-by: Clemens Gruber
> ---
> Changes since v6:
> - Improved !CONFIG_PM handling (wake it up without putting it to sleep
> first)
>
> drivers/pwm/pwm-pca9685.c | 26 +++---
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index d4474c5ff96f..0bcec04b138a 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -474,13 +474,18 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> return ret;
> }
>
> - /* The chip comes out of power-up in the active state */
> - pm_runtime_set_active(>dev);
> - /*
> - * Enable will put the chip into suspend, which is what we
> - * want as all outputs are disabled at this point
> - */
> - pm_runtime_enable(>dev);
> + if (IS_ENABLED(CONFIG_PM)) {
This looks odd to me. I've seen similar constructs, but they usually go
something like this (I think):
pm_runtime_enable(>dev);
if (!pm_runtime_enabled(>dev)) {
/* resume device */
}
Which I guess in your would be somewhat the opposite and it wouldn't
actually resume the device but rather put it to sleep.
Perhaps something like this:
pm_runtime_enable(>dev);
if (pm_runtime_enabled(>dev)) {
pca9685_set_sleep_mode(pca, true);
pm_runtime_set_suspended(>dev);
} else {
/* wake the chip up on non-PM environments */
pca9685_set_sleep_mode(pca, false);
}
? I think that's slightly more correct than your original because it
takes into account things like sysfs power control and such. It also
doesn't rely on the config option alone but instead uses the runtime
PM API to achieve this more transparently.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v7 4/8] dt-bindings: pwm: Support new PWM_STAGGERING_ALLOWED flag
On Wed, Apr 07, 2021 at 07:33:57AM +0200, Uwe Kleine-König wrote: > On Tue, Apr 06, 2021 at 06:41:36PM +0200, Clemens Gruber wrote: > > Add the flag and corresponding documentation for the new PWM staggering > > mode feature. > > > > Cc: Rob Herring > > Signed-off-by: Clemens Gruber > > For the record, I don't like this and still prefer to make this > staggering explicit for the consumer by expanding struct pwm_state with > an .offset member to shift the active phase in the period. How are consumers supposed to know which offset to choose? And worse: how should consumers even know that the driver supports phase shifts? Thierry signature.asc Description: PGP signature
Re: [v3,PATCH 3/3] pwm: mtk_disp: implement .get_state()
On Tue, Apr 06, 2021 at 12:27:56PM +0200, Uwe Kleine-König wrote:
> On Tue, Apr 06, 2021 at 05:57:42PM +0800, Rex-BC Chen wrote:
> > implement get_state function for pwm-mtk-disp
> >
> > Signed-off-by: Rex-BC Chen
> > Signed-off-by: Jitao Shi
>
> Ideally you S-o-b line is the last one to show the order in which this
> patch went from one person to another.
>
> > ---
> > drivers/pwm/pwm-mtk-disp.c | 46 ++
> > 1 file changed, 46 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> > index 502228adf718..166e0a8ca703 100644
> > --- a/drivers/pwm/pwm-mtk-disp.c
> > +++ b/drivers/pwm/pwm-mtk-disp.c
> > @@ -179,8 +179,54 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip,
> > struct pwm_device *pwm,
> > return mtk_disp_pwm_enable(chip, state);
> > }
> >
> > +static void mtk_disp_pwm_get_state(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> > + u32 clk_div, period, high_width, con0, con1;
> > + u64 rate;
> > + int err;
> > +
> > + err = clk_prepare_enable(mdp->clk_main);
> > + if (err < 0) {
> > + dev_err(chip->dev, "Can't enable mdp->clk_main: %d\n", err);
> > + return;
> > + }
> > + err = clk_prepare_enable(mdp->clk_mm);
> > + if (err < 0) {
> > + dev_err(chip->dev, "Can't enable mdp->clk_mm: %d\n", err);
> > + clk_disable_unprepare(mdp->clk_main);
>
> As before: %pe please
According to the documentation %pe only works on pointers for which
IS_ERR() is true, so I'm not sure it can be used with plain integer
error codes.
Looks like there's a bunch of drivers that will do %pe and then use
ERR_PTR(err) to make this work, but to be honest, that seems like
jumping through hoops.
But I don't feel strongly either way, so choose whichever you want.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
On Tue, Apr 06, 2021 at 03:43:56PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Tue, Apr 06, 2021 at 01:16:31PM +0200, Thierry Reding wrote: > > On Tue, Apr 06, 2021 at 09:30:36AM +0200, Uwe Kleine-König wrote: > > > Given that lowlevel drivers usually cannot implement exactly what a > > > consumer requests with pwm_apply_state() there is some rounding involved. > > > > > > pwm_get_state() traditionally returned the setting that was requested most > > > recently by the consumer (opposed to what was actually implemented in > > > hardware in reply to the last request). To make this semantic obvious > > > rename the function. > > > > > > Signed-off-by: Uwe Kleine-König > > > --- > > > Documentation/driver-api/pwm.rst | 6 +++- > > > drivers/clk/clk-pwm.c | 2 +- > > > drivers/gpu/drm/i915/display/intel_panel.c | 4 +-- > > > drivers/input/misc/da7280.c| 2 +- > > > drivers/input/misc/pwm-beeper.c| 2 +- > > > drivers/input/misc/pwm-vibra.c | 4 +-- > > > drivers/pwm/core.c | 4 +-- > > > drivers/pwm/pwm-atmel-hlcdc.c | 2 +- > > > drivers/pwm/pwm-atmel.c| 2 +- > > > drivers/pwm/pwm-imx27.c| 2 +- > > > drivers/pwm/pwm-rockchip.c | 2 +- > > > drivers/pwm/pwm-stm32-lp.c | 4 +-- > > > drivers/pwm/pwm-sun4i.c| 2 +- > > > drivers/pwm/sysfs.c| 18 ++-- > > > drivers/regulator/pwm-regulator.c | 4 +-- > > > drivers/video/backlight/pwm_bl.c | 10 +++ > > > include/linux/pwm.h| 34 ++ > > > 17 files changed, 59 insertions(+), 45 deletions(-) > > > > Honestly, I don't think this is worth the churn. If you think people > > will easily get confused by this then a better solution might be to more > > explicitly document the pwm_get_state() function to say exactly what it > > returns. > > I'm not so optimistic that people become aware of the semantic just > because there is documentation describing it and I strongly believe that > a good name for functions is more important than accurate documentation. > > If you don't agree, what do you think about the updated wording in > Documentation/driver-api/pwm.rst? Yeah, that clarifies this a bit. I can apply that hunk of the patch separately. > > But there's no need to make life difficult for everyone by > > renaming this to something as cumbersome as this. > > I don't expect any merge conflicts (and if still a problem occurs > resolving should be trivial enough). So I obviously don't agree to your > weighing. I wasn't talking about merge conflicts but instead about the extra churn of changing all consumers and having to type all these extra characters for no benefit. Thierry signature.asc Description: PGP signature
Re: [PATCH v2] drm/panel: panel-dsi-cm: convert sysfs snprintf to sysfs_emit
On Thu, Apr 08, 2021 at 11:08:49PM +0800, Xuezhi zhang wrote: > On Thu, 8 Apr 2021 15:14:04 +0200 > Thierry Reding wrote: > > > On Thu, Apr 08, 2021 at 08:52:57AM +, Carlis wrote: > > > From: Xuezhi Zhang > > > > > > Fix the following coccicheck warning: > > > drivers/gpu/drm//panel/panel-dsi-cm.c:271:8-16: > > > WARNING: use scnprintf or sprintf > > > drivers/gpu/drm//panel/panel-dsi-cm.c:251:8-16: > > > WARNING: use scnprintf or sprintf > > > > > > Signed-off-by: Xuezhi Zhang > > > --- > > > v2: change snprint to snprintf in subject. > > > --- > > > drivers/gpu/drm/panel/panel-dsi-cm.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > Nit: I suspect you might've just resent this from some private email > > address, but it's kind of hard to tell because you haven't been using > > at least the same name in both email addresses. > > > > However, if you're forwarding this patch on behalf of somebody else > > you need to add your own Signed-off-by: line. > > > > Reviewed-by: Thierry Reding > > Hi, >the email address of llyz...@163.com is my private email address, >and zhangxue...@yulng.com is my company email address, and Carlis is >my English name ,Xuezhi Zhang is my Chinese name, i will use the >Chinese name to send the emails and patchs in the future. > > thanks, It's not a big deal, I'm just mentioning it because it can confuse people. Yes, using the same name in either case is usually a good way to make people realize what's going on. Thierry signature.asc Description: PGP signature
Re: [PATCH] pwm: raspberrypi-poe: Fix mailbox message initialization
On Fri, Apr 09, 2021 at 11:08:19AM +0200, Nicolas Saenz Julienne wrote:
> For testing purposes this driver might be built on big-endian
> architectures. So make sure we take that into account when populating
> structures that are to be passed to RPi4's mailbox.
>
> Reported-by: kernel test robot
> Fixes: 79caa362eab6 ("pwm: Add Raspberry Pi Firmware based PWM bus")
> Signed-off-by: Nicolas Saenz Julienne
> ---
>
> @arndb: This was just meged into the arm-soc tree some days ago. Should I
> prepare a second PR once it's been reviewed?
>
> drivers/pwm/pwm-raspberrypi-poe.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH] [v2] dmaengine: tegra20: Fix runtime PM imbalance on error
On Fri, Apr 09, 2021 at 04:28:05PM +0800, Dinghao Liu wrote: > pm_runtime_get_sync() will increase the runtime PM counter > even it returns an error. Thus a pairing decrement is needed > to prevent refcount leak. Fix this by replacing this API with > pm_runtime_resume_and_get(), which will not change the runtime > PM counter on error. > > Signed-off-by: Dinghao Liu > --- > > Changelog: > > v2: - Fix another similar case in tegra_dma_synchronize(). > --- > drivers/dma/tegra20-apb-dma.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Looks good: Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH -next] soc/tegra: fuse: add missing iounmap() on error in tegra_init_fuse()
On Fri, Apr 09, 2021 at 12:49:03PM +0800, Yang Yingliang wrote:
> Add the missing iounmap() before return from tegra_init_fuse()
> in the error handling case.
>
> Fixes: 9f94fadd75d3 ("soc/tegra: fuse: Register cell lookups for
> compatibility")
> Reported-by: Hulk Robot
> Signed-off-by: Yang Yingliang
> ---
> drivers/soc/tegra/fuse/fuse-tegra.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c
> b/drivers/soc/tegra/fuse/fuse-tegra.c
> index 94b60a692b51..bc8d70e6a676 100644
> --- a/drivers/soc/tegra/fuse/fuse-tegra.c
> +++ b/drivers/soc/tegra/fuse/fuse-tegra.c
> @@ -489,8 +489,10 @@ static int __init tegra_init_fuse(void)
> size_t size = sizeof(*fuse->lookups) * fuse->soc->num_lookups;
>
> fuse->lookups = kmemdup(fuse->soc->lookups, size, GFP_KERNEL);
> - if (!fuse->lookups)
> + if (!fuse->lookups) {
> + iounmap(fuse->base);
> return -ENOMEM;
> + }
>
> nvmem_add_cell_lookups(fuse->lookups, fuse->soc->num_lookups);
> }
I don't think that's a good idea. Given that this is an early_initcall,
the failure doesn't actually prevent the driver from loading. There are
other functions that rely on fuse->base staying around to access some of
the registers in that I/O memory region.
I suppose we could remove the -ENOMEM return there and instead just skip
registering the nvmem cell lookups, perhaps that would make this less
confusing.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v2] ata: ahci_tegra: call tegra_powergate_power_off only when PM domain is not present
On Thu, Apr 08, 2021 at 01:55:15PM -0700, Sowjanya Komatineni wrote: > This patch adds check to call legacy power domain API > tegra_powergate_power_off() only when PM domain is not present. > > This is a follow-up patch to Tegra186 AHCI support patch series. > --- > drivers/ata/ahci_tegra.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag
On Thu, Apr 08, 2021 at 07:36:37PM +0200, Uwe Kleine-König wrote: > On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote: > > On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote: > > > Yes, I think that's basically what this is saying. I think we're perhaps > > > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives > > > the impression that we're dealing with some provider-specific feature, > > > whereas what we really want to express is that the PWM doesn't care > > > exactly when the active cycle starts and based on that a provider that > > > can support it may optimize the EMI behavior. > > > > > > Maybe we can find a better name for this? Ultimately what this means is > > > that the consumer is primarily interested in the power output of the PWM > > > rather than the exact shape of the signal. So perhaps something like > > > PWM_USAGE_POWER would be more appropriate. > > > > Yes, although it would then no longer be obvious that this feature leads > > to improved EMI behavior, as long as we mention that in the docs, I > > think it's a good idea > > > > Maybe document it as follows? > > PWM_USAGE_POWER - Allow the driver to delay the start of the cycle > > for EMI improvements, as long as the power output stays the same > > I don't like both names, because for someone who is only halfway into > PWM stuff it is not understandable. Maybe ALLOW_PHASE_SHIFT? Heh... how's that any more understandable? > When a consumer is only interested in the power output than > > .period = 20 > .duty_cycle = 5 > > would also be an allowed response for the request > > .period = 200 > .duty_cycle = 50 > > and this is not what is in the focus here. Actually, that's *exactly* what's important here. From a consumer point of view the output power is the key in this case. The specifier is a description of a particular PWM in the consumer context. And the consumer not going to care what exactly the PWM controller might end up configuring to achieve best results. If the controller allows the phase shift to be changed and the constraints allow it, then that's great, but it isn't something that the consumer has to know if all it wants is that the power output is as requested. Put another way, the more generically we can describe the constraints or use cases, the more flexibility we get for drivers to fulfill those constraints. For example one controller might support phase shifting and use that for PWM_USAGE_POWER for better EMI behaviour. But another PWM controller may not support it. But it could perhaps want to optimize the PWM signal by reversing the polarity of one channel or whatever other mechanism there may be. If we add a flag such as ALLOW_PHASE_SHIFT, then only controllers that support programmable phase shift will be able to support this. If some other mechanism can also be used to support "equivalent power" use cases, that would have to be described as some other flag, which has essentially the same meaning. So you can get into a situation where you have multiple flags used for the same thing. Thierry signature.asc Description: PGP signature
Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag
On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote: > On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote: > > On Wed, Apr 07, 2021 at 11:34:03PM +0200, Uwe Kleine-König wrote: > > > On Wed, Apr 07, 2021 at 10:21:10PM +0200, Clemens Gruber wrote: > > > > On Wed, Apr 07, 2021 at 07:46:58AM +0200, Uwe Kleine-König wrote: > > > > > On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote: > > > > > > If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM > > > > > > driver > > > > > > may (if supported by the HW) delay the ON time of the channel > > > > > > relative > > > > > > to the channel number. > > > > > > This does not alter the duty cycle ratio and is only relevant for > > > > > > PWM > > > > > > chips with less prescalers than channels, which would otherwise > > > > > > assert > > > > > > multiple or even all enabled channels at the same time. > > > > > > > > > > > > If this feature is supported by the driver and the flag is set on > > > > > > multiple channels, their ON times are spread out to improve EMI and > > > > > > reduce current spikes. > > > > > > > > > > As said in reply to patch 4/8 already: I don't like this idea and > > > > > think this should be made explicit using a new offset member in struct > > > > > pwm_state instead. That's because I think that the wave form a PWM > > > > > generates should be (completely) defined by the consumer and not by a > > > > > mix between consumer and device tree. Also the consumer has no (sane) > > > > > way to determine if staggering is in use or not. > > > > > > > > I don't think offsets are ideal for this feature: It makes it more > > > > cumbersome for the user, because he has to allocate the offsets > > > > himself instead of a simple on/off switch. > > > > The envisioned usecase is: "I want better EMI behavior and don't care > > > > about the individual channels no longer being asserted at the exact same > > > > time". > > > > > > The formal thing is: "I want better EMI behavior and don't care if > > > periods start with the active phase, it might be anywhere, even over a > > > period boundary." Being asserted at the exact same time is just a detail > > > for the pca9685. > > > > > > > > One side effect (at least for the pca9685) is that when programming a > > > > > new duty cycle it takes a bit longer than without staggering until the > > > > > new setting is active. > > > > > > > > Yes, but it can be turned off if this is a problem, now even per-PWM. > > > > > > Yes and that is a good thing. (BTW: I'd call it per-PWM-consumer, but > > > details.) > > > > > > > > Another objection I have is that we already have some technical debt > > > > > because there are already two different types of drivers (.apply vs > > > > > .config+.set_polarity+.enable+.disable) and I would like to unify this > > > > > first before introducing new stuff. > > > > > > > > But there is already PWM_POLARITY_INVERTED, which can be set in the DT. > > > > I am only adding another flag. > > > > > > I understand your reasoning, and similar to "This diplay backlight needs > > > an inverted PWM (as a low duty-cycle results in a high brightness" this > > > semantic "This consumer doesn't care if the active cycle is anywhere in > > > the period". Hmm, maybe I just have to think about it a bit more to > > > become friends with that thought. > > > > Yes, I think that's basically what this is saying. I think we're perhaps > > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives > > the impression that we're dealing with some provider-specific feature, > > whereas what we really want to express is that the PWM doesn't care > > exactly when the active cycle starts and based on that a provider that > > can support it may optimize the EMI behavior. > > > > Maybe we can find a better name for this? Ultimately what this means is > > that the consumer is primarily interested in the power output of the PWM > > rather than the exact shape of the signal. So perhaps something like > > PWM_USAGE_POWER would be more appropriate. >
Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients
On Thu, Apr 08, 2021 at 02:42:42AM -0700, Nicolin Chen wrote: > On Mon, Mar 29, 2021 at 02:32:55AM +0300, Dmitry Osipenko wrote: > > All consumer-grade Android and Chromebook devices show a splash screen > > on boot and then display is left enabled when kernel is booted. This > > behaviour is unacceptable in a case of implicit IOMMU domains to which > > devices are attached during kernel boot since devices, like display > > controller, may perform DMA at that time. We can work around this problem > > by deferring the enable of SMMU translation for a specific devices, > > like a display controller, until the first IOMMU mapping is created, > > which works good enough in practice because by that time h/w is already > > stopped. > > > > Signed-off-by: Dmitry Osipenko > > For both patches: > Acked-by: Nicolin Chen > Tested-by: Nicolin Chen > > The WAR looks good to me. Perhaps Thierry would give some input. > > Another topic: > I think this may help work around the mc-errors, which we have > been facing on Tegra210 also when we enable IOMMU_DOMAIN_DMA. > (attached a test patch rebasing on these two) Ugh... that's exactly what I was afraid of. Now everybody is going to think that we can just work around this issue with driver-specific SMMU hacks... > However, GPU would also report errors using DMA domain: > > nouveau 5700.gpu: acr: firmware unavailable > nouveau 5700.gpu: pmu: firmware unavailable > nouveau 5700.gpu: gr: firmware unavailable > tegra-mc 70019000.memory-controller: gpusrd: read @0xfffbe200: > Security violation (TrustZone violation) > nouveau 5700.gpu: DRM: failed to create kernel channel, -22 > tegra-mc 70019000.memory-controller: gpusrd: read @0xfffad000: > Security violation (TrustZone violation) > nouveau 5700.gpu: fifo: SCHED_ERROR 20 [] > nouveau 5700.gpu: fifo: SCHED_ERROR 20 [] > > Looking at the address, seems that GPU allocated memory in 32-bit > physical address space behind SMMU, so a violation happened after > turning on DMA domain I guess... The problem with GPU is... extra complicated. You're getting these faults because you're enabling the IOMMU-backed DMA API, which then causes the Nouveau driver allocate buffers using the DMA API instead of explicitly allocating pages and then mapping them using the IOMMU API. However, there are additional patches needed to teach Nouveau about how to deal with SMMU and those haven't been merged yet. I've got prototypes of this, but before the whole framebuffer carveout passing work makes progress there's little sense in moving individual pieces forward. One more not to try and cut corners. We know what the right solution is, even if it takes a lot of work. I'm willing to ack this patch, or some version of it, but only as a way of working around things we have no realistic chance of fixing properly anymore. I still think it would be best if we could derive identity mappings from command-line arguments on these platforms because I think most of them will actually set that, and then the solution becomes at least uniform at the SMMU level. For Tegra210 I've already laid out a path to a solution that's going to be generic and extend to Tegra186 and later as well. Thierry signature.asc Description: PGP signature
Re: [PATCH] drm/panel: tpo-td043mtea1: convert sysfs snprintf to sysfs_emit
On Thu, Apr 08, 2021 at 08:31:18AM +, Carlis wrote: > From: Xuezhi Zhang > > Fix the following coccicheck warning: > drivers/gpu/drm//panel/panel-tpo-td043mtea1.c:217:8-16: > WARNING: use scnprintf or sprintf > drivers/gpu/drm//panel/panel-tpo-td043mtea1.c:189:8-16: > WARNING: use scnprintf or sprintf > > Signed-off-by: Xuezhi Zhang > --- > drivers/gpu/drm/panel/panel-tpo-td043mtea1.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v2] drm/panel: panel-dsi-cm: convert sysfs snprintf to sysfs_emit
On Thu, Apr 08, 2021 at 08:52:57AM +, Carlis wrote: > From: Xuezhi Zhang > > Fix the following coccicheck warning: > drivers/gpu/drm//panel/panel-dsi-cm.c:271:8-16: > WARNING: use scnprintf or sprintf > drivers/gpu/drm//panel/panel-dsi-cm.c:251:8-16: > WARNING: use scnprintf or sprintf > > Signed-off-by: Xuezhi Zhang > --- > v2: change snprint to snprintf in subject. > --- > drivers/gpu/drm/panel/panel-dsi-cm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Nit: I suspect you might've just resent this from some private email address, but it's kind of hard to tell because you haven't been using at least the same name in both email addresses. However, if you're forwarding this patch on behalf of somebody else you need to add your own Signed-off-by: line. Reviewed-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH] PCI: tegra: Fix runtime PM imbalance in pex_ep_event_pex_rst_deassert
On Thu, Apr 08, 2021 at 01:34:37PM +0100, Jon Hunter wrote:
>
> On 08/04/2021 08:26, Dinghao Liu wrote:
> > pm_runtime_get_sync() will increase the runtime PM counter
> > even it returns an error. Thus a pairing decrement is needed
> > to prevent refcount leak. Fix this by replacing this API with
> > pm_runtime_resume_and_get(), which will not change the runtime
> > PM counter on error.
> >
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/pci/controller/dwc/pcie-tegra194.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
> > b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index 6fa216e52d14..0e94190ca4e8 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -1645,7 +1645,7 @@ static void pex_ep_event_pex_rst_deassert(struct
> > tegra_pcie_dw *pcie)
> > if (pcie->ep_state == EP_STATE_ENABLED)
> > return;
> >
> > - ret = pm_runtime_get_sync(dev);
> > + ret = pm_runtime_resume_and_get(dev);
> > if (ret < 0) {
> > dev_err(dev, "Failed to get runtime sync for PCIe dev: %d\n",
> > ret);
> >
>
> There are two places in the driver where pm_runtime_get_sync() is called.
It looks like the second callsite has the proper cleanup code. Although
it might be nice to use pm_runtime_resume_and_get() there as well, and
adjust the cleanup path, to make this consistent.
In any case, this looks good to me:
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186
On Thu, Apr 08, 2021 at 02:25:19AM +0300, Dmitry Osipenko wrote:
> 08.04.2021 02:00, Sowjanya Komatineni пишет:
> >
> > On 4/7/21 3:57 PM, Sowjanya Komatineni wrote:
> >>
> >> On 4/7/21 2:36 PM, Dmitry Osipenko wrote:
> >>> 07.04.2021 04:25, Sowjanya Komatineni пишет:
> + if (!tegra->pdev->dev.pm_domain) {
> + ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
> + tegra->sata_clk,
> + tegra->sata_rst);
> + if (ret)
> + goto disable_regulators;
> + }
> >>> Hi,
> >>>
> >>> Why you haven't added condition for tegra_powergate_power_off()? I think
> >>> it should break GENPD and legacy PD API isn't not supported by T186
> >>> at all.
> >>>
> >>> I'm also not sure whether the power up/down sequence is correct using
> >>> GENPD.
> >>>
> >>> Moreover the driver doesn't support runtime PM, so GENPD should be
> >>> always off?
> >>
> >> This driver already using legacy PD API's so thought its supported and
> >> added power domain device check during powergate_sequence_power_up and
> >> yes same should apply for powergate_power_off as well. But if legacy
> >> PD is not supported by T186 then not sure why original driver even
> >> using these API's.
> >>
> >>
> > Sorry just took a look and driver supports T210 and prior tegra as well.
> > T210 and prior supports legacy PD and this check is applicable for
> > those. So we should add power domain device check for power off as well.
>
> You could fix it with a follow up patch. Please try to test that
> power-off works properly, at least try to unload the driver module and
> re-load it.
Agreed, this should have the same check as above for
tegra_powergate_power_off(). It currently works fine because on Tegra186
tegra_powergate_power_off() (and all the other legacy APIs for that
matter) will abort early since no power gates are implemented. The AHCI
driver doesn't check for errors, so this will just fail silently. It's
better to be symmetric, though, and add the check in both paths.
> > But for T186, we should have GENPD working once we add runtime PM
> > support to driver.
> >
> > Preetham/Thierry, Can you confirm where SATA is un powergated prior to
> > kernel?
> >
> >
> >> But as RPM is not implemented yet for this driver, GENPD will be OFF
> >> but SATA is not in power-gate by the time kernel starts and
> >> functionally works.
> >>
> >> But with RPM implementation, I guess we can do proper power gate on/off.
> >>
>
> I now recalled that GENPD turns ON all domains by default and then turns
> them OFF only when driver entered into the RPM-suspended state. This
> means that AHCI GENPD should be always-ON for T186, which should be okay
> if this doesn't break power sequences.
Yeah, the generic PM domain will just stay enabled after probe and until
remove. This does not impact the power sequences because they have to be
completely implemented in the power domains code anyway. With the legacy
API we used to need more rigorous sequences in the individual drivers,
but with generic PM domains none of that should be necessary, though it
also doesn't hurt, so some of the unnecessary clock enablement code is
kept for simplicity.
To be honest, I'm not sure if it's worth adding runtime PM support for
this driver. If this top-level layer has a way of getting notification
when no device was detected, then it might make some sense to turn off
the power domain and the regulators again, but I'm not sure if that's
the case. tegra_ahci_host_stop() seems like it might be usable for that
so yeah, that might work. We currently do turn off the powergate in that
case, so extending that power optimization to Tegra186 using runtime PM
makes sense.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag
On Wed, Apr 07, 2021 at 11:34:03PM +0200, Uwe Kleine-König wrote: > On Wed, Apr 07, 2021 at 10:21:10PM +0200, Clemens Gruber wrote: > > On Wed, Apr 07, 2021 at 07:46:58AM +0200, Uwe Kleine-König wrote: > > > On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote: > > > > If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver > > > > may (if supported by the HW) delay the ON time of the channel relative > > > > to the channel number. > > > > This does not alter the duty cycle ratio and is only relevant for PWM > > > > chips with less prescalers than channels, which would otherwise assert > > > > multiple or even all enabled channels at the same time. > > > > > > > > If this feature is supported by the driver and the flag is set on > > > > multiple channels, their ON times are spread out to improve EMI and > > > > reduce current spikes. > > > > > > As said in reply to patch 4/8 already: I don't like this idea and > > > think this should be made explicit using a new offset member in struct > > > pwm_state instead. That's because I think that the wave form a PWM > > > generates should be (completely) defined by the consumer and not by a > > > mix between consumer and device tree. Also the consumer has no (sane) > > > way to determine if staggering is in use or not. > > > > I don't think offsets are ideal for this feature: It makes it more > > cumbersome for the user, because he has to allocate the offsets > > himself instead of a simple on/off switch. > > The envisioned usecase is: "I want better EMI behavior and don't care > > about the individual channels no longer being asserted at the exact same > > time". > > The formal thing is: "I want better EMI behavior and don't care if > periods start with the active phase, it might be anywhere, even over a > period boundary." Being asserted at the exact same time is just a detail > for the pca9685. > > > > One side effect (at least for the pca9685) is that when programming a > > > new duty cycle it takes a bit longer than without staggering until the > > > new setting is active. > > > > Yes, but it can be turned off if this is a problem, now even per-PWM. > > Yes and that is a good thing. (BTW: I'd call it per-PWM-consumer, but > details.) > > > > Another objection I have is that we already have some technical debt > > > because there are already two different types of drivers (.apply vs > > > .config+.set_polarity+.enable+.disable) and I would like to unify this > > > first before introducing new stuff. > > > > But there is already PWM_POLARITY_INVERTED, which can be set in the DT. > > I am only adding another flag. > > I understand your reasoning, and similar to "This diplay backlight needs > an inverted PWM (as a low duty-cycle results in a high brightness" this > semantic "This consumer doesn't care if the active cycle is anywhere in > the period". Hmm, maybe I just have to think about it a bit more to > become friends with that thought. Yes, I think that's basically what this is saying. I think we're perhaps getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives the impression that we're dealing with some provider-specific feature, whereas what we really want to express is that the PWM doesn't care exactly when the active cycle starts and based on that a provider that can support it may optimize the EMI behavior. Maybe we can find a better name for this? Ultimately what this means is that the consumer is primarily interested in the power output of the PWM rather than the exact shape of the signal. So perhaps something like PWM_USAGE_POWER would be more appropriate. Come to think of it, a flag like that might even be useful to implement the common case of emulating inverted polarity with reversing the duty cycle. That is, if PWM_USAGE_POWER | PWM_POLARITY_INVERSED was specified and the PWM provider did not support polarity inversion, the inversion could still be implemented using emulation. Currently we push that logic down into consumers, but this could be a way to bring that up into drivers, or perhaps even the core. Thierry signature.asc Description: PGP signature
Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients
On Mon, Mar 29, 2021 at 02:32:55AM +0300, Dmitry Osipenko wrote:
> All consumer-grade Android and Chromebook devices show a splash screen
> on boot and then display is left enabled when kernel is booted. This
> behaviour is unacceptable in a case of implicit IOMMU domains to which
> devices are attached during kernel boot since devices, like display
> controller, may perform DMA at that time. We can work around this problem
> by deferring the enable of SMMU translation for a specific devices,
> like a display controller, until the first IOMMU mapping is created,
> which works good enough in practice because by that time h/w is already
> stopped.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/iommu/tegra-smmu.c | 71 ++
> 1 file changed, 71 insertions(+)
In general I do see why we would want to enable this. However, I think
this is a bad idea because it's going to proliferate the bad practice of
not describing things properly in device tree.
Whatever happened to the idea of creating identity mappings based on the
obscure tegra_fb_mem (or whatever it was called) command-line option? Is
that command-line not universally passed to the kernel from bootloaders
that initialize display?
That idealistic objection aside, this seems a bit over-engineered for
the hack that it is. See below.
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 602aab98c079..af1e4b5adb27 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -60,6 +60,8 @@ struct tegra_smmu_as {
> dma_addr_t pd_dma;
> unsigned id;
> u32 attr;
> + bool display_attached[2];
> + bool attached_devices_need_sync;
> };
>
> static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
> @@ -78,6 +80,10 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu,
> unsigned long offset)
> return readl(smmu->regs + offset);
> }
>
> +/* all Tegra SoCs use the same group IDs for displays */
> +#define SMMU_SWGROUP_DC 1
> +#define SMMU_SWGROUP_DCB 2
> +
> #define SMMU_CONFIG 0x010
> #define SMMU_CONFIG_ENABLE (1 << 0)
>
> @@ -253,6 +259,20 @@ static inline void smmu_flush(struct tegra_smmu *smmu)
> smmu_readl(smmu, SMMU_PTB_ASID);
> }
>
> +static int smmu_swgroup_to_display_id(unsigned int swgroup)
> +{
> + switch (swgroup) {
> + case SMMU_SWGROUP_DC:
> + return 0;
> +
> + case SMMU_SWGROUP_DCB:
> + return 1;
> +
> + default:
> + return -1;
> + }
> +}
> +
Why do we need to have this two-level mapping? Do we even need to care
about the specific swgroups IDs? Can we not just simply check at attach
time if the client that's being attached is a display client and then
set atteched_devices_need_sync = true?
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
On Thu, Apr 08, 2021 at 09:59:20AM +0200, Uwe Kleine-König wrote: > Hello Nobuhiro, > > On Thu, Apr 08, 2021 at 08:15:48AM +0900, Nobuhiro Iwamatsu wrote: > > > > + /* > > > > +* pwmc is a 2-bit divider for the input clock running at 1 MHz. > > > > +* When the settings of the PWM are modified, the new values > > > > are shadowed in hardware until > > > > +* the period register (PCSR) is written and the currently > > > > running period is completed. This > > > > +* way the hardware switches atomically from the old setting to > > > > the new. > > > > +* Also, disabling the hardware completes the currently running > > > > period and keeps the output > > > > +* at low level at all times. > > > > > > Did you just copy my optimal description or is your hardware really that > > > nice? > > > > Yes, this hardware works as you wrote. > > And I added about the state if the sinnal when this hardware disabled. > > > > > > > > Do you know scripts/checkpatch.pl? I bet it will tell you to limit your > > > lines to approx. 80 chars where sensible. > > > > Yes, I know. I ran scripts/checkpatch.pl before send patch. > > I understand that the number of characters per line has been changed to > > 100 characters. Does the pwm driver recommend 80 characters? > > For free-text comments I'd still recommend 80, yes. For code lines I'd > be indeed more lax, as a line break in function calls reduces readability. Let's not start making any special rules. It becomes impossible for anyone to keep track of those. If checkpatch doesn't complain for comments that exceed 80 characters, I will not reject based on that. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 0/3] Add AHCI support for Tegra186
On Wed, Apr 07, 2021 at 09:44:58AM -0600, Jens Axboe wrote: > On 4/6/21 7:25 PM, Sowjanya Komatineni wrote: > > Re-sending dt-binding and ahci_tegra driver patches as v4 as device > > tree patches from v3 are merged but not the AHCI Tegra driver. > > > > Missed to add Jens Axboe to mailing list in v3. Adding for v4. > > > > This series adds support for AHCI-compliant SATA to Tegra186 SoC. > > > > This series includes patches for > > - Converting text based dt-binding document to YAML. > > - Adding dt-bindings for Tegra186. > > - Adding Tegra186 support to Tegra AHCI driver. > > > > Delta between patch versions: > > [v4]: Same as v3 except removed device tree patches as they are > > merged. > > [v3]: fixed yaml example to pass dt_binding_check > > [v2]: v1 feedback related to yaml dt-binding. > > Removed conditional reset order in yaml and updated dts files > > to maintain same order for commonly available resets across > > Tegra124 thru Tegra186. > > Assuming the libata tree is the best way for this to go in, so I applied > it for 5.13. Perfect! Thanks Jens. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186
On Tue, Apr 06, 2021 at 06:25:31PM -0700, Sowjanya Komatineni wrote: > This patch adds support for AHCI-compliant Serial ATA controller > on Tegra186 SoC. > > Tegra186 does not have sata-oob reset. > Tegra186 SATA_NVOOB register filed COMMA_CNT position and width are > different compared to Tegra210 and prior. > > So, this patch adds a flag has_sata_oob_rst and tegra_ahci_regs to > SoC specific strcuture tegra_ahci_soc and updated their implementation > accordingly. > > Signed-off-by: Sowjanya Komatineni > --- > drivers/ata/ahci_tegra.c | 60 > +--- > 1 file changed, 47 insertions(+), 13 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v4 2/3] dt-binding: ata: tegra: Add dt-binding documentation for Tegra186
On Tue, Apr 06, 2021 at 06:25:30PM -0700, Sowjanya Komatineni wrote: > This patch adds dt-bindings documentation for Tegra186 AHCI > controller. > > Reviewed-by: Rob Herring > Signed-off-by: Sowjanya Komatineni > --- > .../devicetree/bindings/ata/nvidia,tegra-ahci.yaml | 38 > ++ > 1 file changed, 38 insertions(+) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v4 1/3] dt-bindings: ata: tegra: Convert binding documentation to YAML
On Tue, Apr 06, 2021 at 06:25:29PM -0700, Sowjanya Komatineni wrote: > This patch converts text based dt-binding document to YAML based > dt-binding document. > > Reviewed-by: Rob Herring > Signed-off-by: Sowjanya Komatineni > --- > .../devicetree/bindings/ata/nvidia,tegra-ahci.yaml | 138 > + > .../bindings/ata/nvidia,tegra124-ahci.txt | 44 --- > 2 files changed, 138 insertions(+), 44 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml > delete mode 100644 > Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH] media: Fix compilation error
On Wed, Apr 07, 2021 at 04:45:34PM +0300, Mikko Perttunen wrote: > On 7.4.2021 16.29, Hans Verkuil wrote: > > On 02/04/2021 09:40, Bixuan Cui wrote: > > > Fix the error: > > > > > > drivers/staging/media/tegra-video/vi.c:1180:4: > > > error: implicit declaration of function 'host1x_syncpt_free' > > > [-Werror,-Wimplicit-function-declaration] > > > > Against what tree is this being built? The mainline kernel doesn't have > > host1x_syncpt_put, only host1x_syncpt_free. > > This change was done only very recently, it's in linux-next and submitted > for 5.13. I missed this one host1x_syncpt_free call in vi.c, but Thierry has > already applied an equivalent patch on his end so the issue should be > resolved. Indeed, this should be fixed as of next-20210406. This is going in through the DRM tree in order to keep the change atomic and bisectible. Thierry signature.asc Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
On Tue, Apr 06, 2021 at 08:33:57AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
>
> On Wed, Mar 31, 2021 at 05:52:45PM +0200, Thierry Reding wrote:
> > On Wed, Mar 31, 2021 at 12:25:43PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Mar 22, 2021 at 09:34:21AM +0100, Thierry Reding wrote:
> > > > On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> > > > > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > > > > > Another point is the period: Sven suggested we do not read out the
> > > > > > period at all, as the PWM is disabled anyway (see above).
> > > > > > Is this acceptable?
> > > > >
> > > > > In my eyes consumers should consider the period value as "don't care"
> > > > > if
> > > > > the PWM is off. But this doesn't match reality (and maybe also it
> > > > > doesn't match Thierry's opinion). See for example the
> > > > > drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> > > > >
> > > > > pwm_get_state(mypwm, );
> > > > > state.enabled = true;
> > > > > pwm_apply_state(pb->pwm, );
> > > > >
> > > > > which breaks if .period is invalid. (OK, this isn't totally accurate
> > > > > because currently the .get_state callback has only little to do with
> > > > > pwm_get_state(), but you get the point.)
> > > >
> > > > The idea behind atomic states in the PWM API is to provide accurate
> > > > snapshots of a PWM channel's settings. It's not a representation of
> > > > the PWM channel's physical output, although in some cases they may
> > > > be the same.
> > >
> > > I think the pwm_state returned by .get_state() should be an accurate
> > > representation of the PWM channel's physical output.
> >
> > Yeah, like I said, in most cases that will be true. However, as
> > mentioned below, if there's no 1:1 correspondence between the settings
> > and what's actually coming out of the PWM, this isn't always possible.
> > But yes, it should always be as accurate as possible.
>
> It should always be true, not only in most cases. For any given PWM
> output you can provide a pwm_state that describes this output (unless
> you'd need a value bigger than u64 to describe it or a higher precision
> as pwm_state only uses integer values). So it's a 1:n relation: You
> should always be able to provide a pwm_state and in some cases there are
> more than one such states (and you should still provide one of them).
My point is that the correspondence between the pwm_state and what's
programmed to hardware can't always be read back to unambiguously give
the same result. As you mentioned there are these cases where the PWM
channel doesn't have a separate enable bit, in which case it must be
emulated by setting the duty cycle to 0.
In such cases it doesn't make sense to always rely on ->get_state() to
read back the logical state because it just can't. How is it supposed to
know from reading that 0 duty cycle whether that means the PWM is on or
off? So for the initial read-out we can't do any better so we have to
pick one. The easiest would probably be to just assume PWM disabled when
duty cycle is 0 and in most cases that will be just fine as the
resulting PWM will be the same regardless of which of the two options we
pick.
However, what I'm also saying is that once the consumer has called
pwm_apply_state(), there's no reason to read back the value from
hardware and potentially loose information about the state that isn't
contained in hardware. If the consumer has applied this state:
{
.period = 100,
.duty_cycle = 0,
.polarity = PWM_POLARITY_NORMAL,
.enabled = true,
}
then why would we want to have it call ->get_state() and turn this into:
{
.period = 100,
.duty_cycle = 0,
.polarity = PWM_POLARITY_NORMAL,
.enabled = false,
}
? If pwm_apply_state() has properly applied the first state there's no
reason for the consumer to continue using either its own cache or the
PWM framework's cache (via pwm_get_state()) and just toggle the bits as
necessary.
> > > > However, given that we want a snapshot of the currently configured
> > > > settings, we can't simply assume that there's a 1:1 correspondence and
> > > > then use shortcuts to simplify the hardware state representation because
> > > > it isn't going to be accurate.
> > >
> > > When we assume that pwm_get_state returns
Re: [PATCH 5.10 079/126] drm/tegra: sor: Grab runtime PM reference across reset
On Mon, Apr 05, 2021 at 05:42:21PM +0200, Pavel Machek wrote: > Hi! > > > However, these functions alone don't provide any guarantees at the > > system level. Drivers need to ensure that the only a single consumer has > > access to the reset at the same time. In order for the SOR to be able to > > exclusively access its reset, it must therefore ensure that the SOR > > power domain is not powered off by holding on to a runtime PM reference > > to that power domain across the reset assert/deassert operation. > > Yeah, but it should not leak the PM reference in the error handling. True. However the only reason why the code could realistically fail between pm_runtime_resume_and_get() and pm_runtime_put() is if we did not take the runtime PM reference (which would cause the call to reset_control_acquire() to fail). So the chances of this actually leaking are practically zero, so I didn't want to bloat this bugfix with what's essentially dead code. I can queue up your fix below for v5.14, though, since it's obviously more correct from a theoretical point of view. Thierry signature.asc Description: PGP signature
Re: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
On Tue, Apr 06, 2021 at 09:30:36AM +0200, Uwe Kleine-König wrote: > Given that lowlevel drivers usually cannot implement exactly what a > consumer requests with pwm_apply_state() there is some rounding involved. > > pwm_get_state() traditionally returned the setting that was requested most > recently by the consumer (opposed to what was actually implemented in > hardware in reply to the last request). To make this semantic obvious > rename the function. > > Signed-off-by: Uwe Kleine-König > --- > Documentation/driver-api/pwm.rst | 6 +++- > drivers/clk/clk-pwm.c | 2 +- > drivers/gpu/drm/i915/display/intel_panel.c | 4 +-- > drivers/input/misc/da7280.c| 2 +- > drivers/input/misc/pwm-beeper.c| 2 +- > drivers/input/misc/pwm-vibra.c | 4 +-- > drivers/pwm/core.c | 4 +-- > drivers/pwm/pwm-atmel-hlcdc.c | 2 +- > drivers/pwm/pwm-atmel.c| 2 +- > drivers/pwm/pwm-imx27.c| 2 +- > drivers/pwm/pwm-rockchip.c | 2 +- > drivers/pwm/pwm-stm32-lp.c | 4 +-- > drivers/pwm/pwm-sun4i.c| 2 +- > drivers/pwm/sysfs.c| 18 ++-- > drivers/regulator/pwm-regulator.c | 4 +-- > drivers/video/backlight/pwm_bl.c | 10 +++ > include/linux/pwm.h| 34 ++ > 17 files changed, 59 insertions(+), 45 deletions(-) Honestly, I don't think this is worth the churn. If you think people will easily get confused by this then a better solution might be to more explicitly document the pwm_get_state() function to say exactly what it returns. But there's no need to make life difficult for everyone by renaming this to something as cumbersome as this. Thierry signature.asc Description: PGP signature
Re: [PATCH v6 4/7] pwm: pca9685: Support staggered output ON times
On Thu, Apr 01, 2021 at 09:50:44AM +0200, Clemens Gruber wrote: > Hi Thierry, > > On Wed, Mar 31, 2021 at 06:21:32PM +0200, Thierry Reding wrote: > > On Wed, Mar 31, 2021 at 02:26:14PM +0200, Clemens Gruber wrote: > > > On Mon, Mar 29, 2021 at 08:02:06PM +0200, Uwe Kleine-König wrote: > > > > On Mon, Mar 29, 2021 at 07:16:38PM +0200, Clemens Gruber wrote: > > > > > On Mon, Mar 29, 2021 at 07:03:57PM +0200, Uwe Kleine-König wrote: > > > > > > On Mon, Mar 29, 2021 at 02:57:04PM +0200, Clemens Gruber wrote: > > > > > > > The PCA9685 supports staggered LED output ON times to minimize > > > > > > > current > > > > > > > surges and reduce EMI. > > > > > > > When this new option is enabled, the ON times of each channel are > > > > > > > delayed by channel number x counter range / 16, which avoids > > > > > > > asserting > > > > > > > all enabled outputs at the same counter value while still > > > > > > > maintaining > > > > > > > the configured duty cycle of each output. > > > > > > > > > > > > > > Signed-off-by: Clemens Gruber > > > > > > > > > > > > Is there a reason to not want this staggered output? If it never > > > > > > hurts I > > > > > > suggest to always stagger and drop the dt property. > > > > > > > > > > There might be applications where you want multiple outputs to assert > > > > > at > > > > > the same time / to be synchronized. > > > > > With staggered outputs mode always enabled, this would no longer be > > > > > possible as they are spread out according to their channel number. > > > > > > > > > > Not sure how often that usecase is required, but just enforcing the > > > > > staggered mode by default sounds risky to me. > > > > > > > > There is no such guarantee in the PWM framework, so I don't think we > > > > need to fear breaking setups. Thierry? > > > > > > Still, someone might rely on it? But let's wait for Thierry's opinion. > > > > There's currently no way to synchronize two PWM channels in the PWM > > framework. And given that each PWM channel is handled separately the > > programming for two channels will never happen atomically or even > > concurrently, so I don't see how you could run two PWMs completely > > synchronized to one another. > > As the PCA9685 has only one prescaler and one counter per chip, by > default, all PWMs enabled will go high at the same time. If they also > have the same duty cycle configured, they also go low at the same time. What happens if you enable one of them, it then goes high and then you enable the next one? Is the second one going to get enabled on the next period? Or will it start in the middle of the period? To truly enable them atomically, you'd have to ensure they all get enabled in basically the same write, right? Because otherwise you can still end up with just a subset enabled and the rest getting enabled only after the first period. > > Or did I misunderstand and it's only the start time of the rising edge > > that's shifted, but the signal will remain high for a full duty cycle > > after that and then go down and remain low for period - duty - offset? > > Yes, that's how it works. That's less problematic because the signal will remain a standard PWM, it's just shifted by some amount. Technically pwm_apply_state() must only return when the signal has been enabled, so very strictly speaking you'd have to wait for a given amount of time to ensure that's correct. But again, I doubt that any use-case would require you to be that deterministic. > > That's slightly better than the above in that it likely won't trip up > > any consumers. But it might still be worth to make this configurable per > > PWM (perhaps by specifying a third specifier cell, in addition to the > > period and flags, that defines the offset/phase of the signal). > > > > In both cases, doing this on a per-PWM basis will allow the consumer to > > specify that they're okay with staggered mode and you won't actually > > force it onto anyone. This effectively makes this opt-in and there will > > be no change for existing consumers. > > I agree that it should be opt-in, but I am not sure about doing it > per-pwm: > The reason why you'd want staggered mode is to reduce EMI or current > spikes and it is most effective if it is enabled for all PWMs. > > If it is specifi
Re: [PATCH RFT 2/2] i2c: tegra-bpmp: make some functions void
On Wed, Mar 31, 2021 at 09:51:41AM +0200, Wolfram Sang wrote: > They return 0 always, so save some lines and code. > > Signed-off-by: Wolfram Sang > --- > drivers/i2c/busses/i2c-tegra-bpmp.c | 20 > 1 file changed, 4 insertions(+), 16 deletions(-) Tested-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH RFT 1/2] i2c: tegra-bpmp: don't modify input variable in xlate_flags
On Wed, Mar 31, 2021 at 09:51:40AM +0200, Wolfram Sang wrote:
> Since commit bc1c2048abbe ("i2c: bpmp-tegra: Ignore unknown I2C_M
> flags") we don't need to mask out flags and can keep the input variable
> as is to save quite some lines.
>
> Signed-off-by: Wolfram Sang
> ---
> drivers/i2c/busses/i2c-tegra-bpmp.c | 32 -
> 1 file changed, 8 insertions(+), 24 deletions(-)
I've applied this to my local development tree and ran this on Tegra194,
didn't see any issues, so:
Tested-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH RFT 2/2] i2c: tegra-bpmp: make some functions void
On Wed, Mar 31, 2021 at 09:51:41AM +0200, Wolfram Sang wrote: > They return 0 always, so save some lines and code. > > Signed-off-by: Wolfram Sang > --- > drivers/i2c/busses/i2c-tegra-bpmp.c | 20 > 1 file changed, 4 insertions(+), 16 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH RFT 1/2] i2c: tegra-bpmp: don't modify input variable in xlate_flags
On Wed, Mar 31, 2021 at 09:51:40AM +0200, Wolfram Sang wrote:
> Since commit bc1c2048abbe ("i2c: bpmp-tegra: Ignore unknown I2C_M
> flags") we don't need to mask out flags and can keep the input variable
> as is to save quite some lines.
>
> Signed-off-by: Wolfram Sang
> ---
> drivers/i2c/busses/i2c-tegra-bpmp.c | 32 -
> 1 file changed, 8 insertions(+), 24 deletions(-)
Heh... good catch!
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH v6 4/7] pwm: pca9685: Support staggered output ON times
On Wed, Mar 31, 2021 at 02:26:14PM +0200, Clemens Gruber wrote: > On Mon, Mar 29, 2021 at 08:02:06PM +0200, Uwe Kleine-König wrote: > > On Mon, Mar 29, 2021 at 07:16:38PM +0200, Clemens Gruber wrote: > > > On Mon, Mar 29, 2021 at 07:03:57PM +0200, Uwe Kleine-König wrote: > > > > On Mon, Mar 29, 2021 at 02:57:04PM +0200, Clemens Gruber wrote: > > > > > The PCA9685 supports staggered LED output ON times to minimize current > > > > > surges and reduce EMI. > > > > > When this new option is enabled, the ON times of each channel are > > > > > delayed by channel number x counter range / 16, which avoids asserting > > > > > all enabled outputs at the same counter value while still maintaining > > > > > the configured duty cycle of each output. > > > > > > > > > > Signed-off-by: Clemens Gruber > > > > > > > > Is there a reason to not want this staggered output? If it never hurts I > > > > suggest to always stagger and drop the dt property. > > > > > > There might be applications where you want multiple outputs to assert at > > > the same time / to be synchronized. > > > With staggered outputs mode always enabled, this would no longer be > > > possible as they are spread out according to their channel number. > > > > > > Not sure how often that usecase is required, but just enforcing the > > > staggered mode by default sounds risky to me. > > > > There is no such guarantee in the PWM framework, so I don't think we > > need to fear breaking setups. Thierry? > > Still, someone might rely on it? But let's wait for Thierry's opinion. There's currently no way to synchronize two PWM channels in the PWM framework. And given that each PWM channel is handled separately the programming for two channels will never happen atomically or even concurrently, so I don't see how you could run two PWMs completely synchronized to one another. That said, it might be possible to implement something like this by coupling two or more PWMs. However, I think we will only ever be able to do this on a per-chip basis, because that's the only way we could guarantee that multiple PWMs can be programmed at the same time, or that they get enabled with the same write. Of course this all requires that the chip even supports that. Even if you enable two PWM channels within the same driver but with two consecutive register writes they will not be guaranteed to run synchronously. There'd have to be some special chip support to allow this to work. However, I'm a bit hesitant about this staggering output mode. From what I understand what's going to happen for these is basically that overall each PWM will be running at the requested duty cycle, but the on/off times will be evenly spread out over the whole period. In other words, the output *power* of the PWM signal will be the same as if the signal was a single on/off cycle. That's not technically a PWM signal as the PWM framework defines it. See the kerneldoc for enum pwm_polarity for what signals are expected to look like. So I agree that this is not something that should be enabled by default because if you've got a consumer that expects exactly one rising edge and one falling edge per period, they will get confused if you toggle multiple times during one period. If that's the case,you probably want to configure this on a per-PWM basis rather than for the entire chip because otherwise you could end up in a scenario where one PWM does *not* work with staggered output and the others do. I guess you could always make that decision up front, but do you always know what people may end up using these PWMs for? What if you have a board that breaks out one PWM on some general purpose pin header and people end up using it via sysfs. They would need have to recompile the DTB for the device if they wanted to enable or disable this staggered mode. Or did I misunderstand and it's only the start time of the rising edge that's shifted, but the signal will remain high for a full duty cycle after that and then go down and remain low for period - duty - offset? That's slightly better than the above in that it likely won't trip up any consumers. But it might still be worth to make this configurable per PWM (perhaps by specifying a third specifier cell, in addition to the period and flags, that defines the offset/phase of the signal). In both cases, doing this on a per-PWM basis will allow the consumer to specify that they're okay with staggered mode and you won't actually force it onto anyone. This effectively makes this opt-in and there will be no change for existing consumers. > > One reason we might not want staggering is if we have a consumer who > > cares about config transitions. (This however is moot it the hardware > > doesn't provide sane transitions even without staggering.) > > > > Did I already ask about races in this driver? I assume there is a > > free running counter and the ON and OFF registers just define where in > > the period the transitions happen, right? Given that changing ON and
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
On Wed, Mar 31, 2021 at 12:25:43PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
>
> On Mon, Mar 22, 2021 at 09:34:21AM +0100, Thierry Reding wrote:
> > On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> > > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > > > Another point is the period: Sven suggested we do not read out the
> > > > period at all, as the PWM is disabled anyway (see above).
> > > > Is this acceptable?
> > >
> > > In my eyes consumers should consider the period value as "don't care" if
> > > the PWM is off. But this doesn't match reality (and maybe also it
> > > doesn't match Thierry's opinion). See for example the
> > > drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> > >
> > > pwm_get_state(mypwm, );
> > > state.enabled = true;
> > > pwm_apply_state(pb->pwm, );
> > >
> > > which breaks if .period is invalid. (OK, this isn't totally accurate
> > > because currently the .get_state callback has only little to do with
> > > pwm_get_state(), but you get the point.)
> >
> > The idea behind atomic states in the PWM API is to provide accurate
> > snapshots of a PWM channel's settings. It's not a representation of
> > the PWM channel's physical output, although in some cases they may
> > be the same.
>
> I think the pwm_state returned by .get_state() should be an accurate
> representation of the PWM channel's physical output.
Yeah, like I said, in most cases that will be true. However, as
mentioned below, if there's no 1:1 correspondence between the settings
and what's actually coming out of the PWM, this isn't always possible.
But yes, it should always be as accurate as possible.
> > However, there's no 1:1 correspondence between those two. For example,
> > when looking purely at the physical output of a PWM it is in most cases
> > not possible to make the distinction between these two states:
> >
> > - duty: 0
> > period: 100
> >
> > - duty: 0
> > period: 200
> >
> > Because the output will be a constant 0 (or 1, depending on polarity).
>
> I agree that there isn't in all cases a unique pwm_state that formalizes
> the current output. That's because with .enabled=false the settings
> .duty_cycle and .period hardly matter for the output and when
> .duty_cycle = 0 or = .period the actual period also (mostly) doesn't
> matter.
>
> > However, given that we want a snapshot of the currently configured
> > settings, we can't simply assume that there's a 1:1 correspondence and
> > then use shortcuts to simplify the hardware state representation because
> > it isn't going to be accurate.
>
> When we assume that pwm_get_state returns the state of the hardware,
> relying on these values might be too optimistic. Consider a hardware
> (e.g. pwm-cros-ec) that has no disable switch. Disabling is modeled by
> .duty_cycle = 0. So after:
>
> pwm_apply_state(mypwm, { .period = 1000, .duty_cycle = 500, .enabled =
> false })
>
> doing:
>
> pwm_get_state(pwm, );
> mystate.enabled = true;
> pwm_apply_state(pwm, );
>
> the PWM stays configured with .duty_cycle = 0.
Uhm... no, that's not what should be happening. pwm_get_state() copies
the PWM channel's internal software state. It doesn't actually go and
read the hardware state. We only ever read the hardware state when the
PWM is requested. After that there should be no reason to ever read back
the hardware state again because the consumer owns the state and that
state must not change unless explicitly requested by the owner.
So in the above case, mystate.duty_cycle should be 500 after that call
to pwm_get_state(). The fact that the hardware can't explicitly disable
a PWM and needs to emulate that by setting duty-cycle = 0 is a driver
implementation detail and shouldn't leak to the consumer.
> There are also more delicate surprises. Consider a PWM that can
> implement all duty_cycles and periods with a resolution of 30 ns up to
> the consumers preferred period of 2000 ns and uses the usual round-down
> strategy. Consider the consumer wants to repeatedly switch between 75%
> and 50% relative duty cycle.
>
> When relying on .get_state, the first configuration is likely to still
> be 1500/2000. .get_state() then returns
>
> .duty_cycle = 1500
> .period = 1980
>
> and then to implement the 50% relative duty the resulting request is
>
> .duty_cycle = 990
> .period = 1980
>
> which can be implemented exactly. When then switching back to 75% the
> request is
>
> .duty_cycl
Re: [PATCH 13/17] ASoC: tegra: tegra20_das: align function prototypes
^ > sound/soc/tegra/tegra20_das.h:118:59: note: Function > 'tegra20_das_connect_dac_to_dap' argument 2 names different: > declaration 'dap_sel' definition 'dap'. > extern int tegra20_das_connect_dac_to_dap(int dac_id, int dap_sel); > ^ > sound/soc/tegra/tegra20_das.c:75:49: note: Function > 'tegra20_das_connect_dac_to_dap' argument 2 names different: > declaration 'dap_sel' definition 'dap'. > int tegra20_das_connect_dac_to_dap(int dac, int dap) > ^ > > Signed-off-by: Pierre-Louis Bossart > --- > sound/soc/tegra/tegra20_das.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH 12/17] ASoC: tegra: tegra20_das: clarify expression
On Fri, Mar 26, 2021 at 04:59:22PM -0500, Pierre-Louis Bossart wrote: > cppcheck warning: > > sound/soc/tegra/tegra20_das.c:64:60: style: Boolean result is used in > bitwise operation. Clarify expression with > parentheses. [clarifyCondition] > reg = otherdap << TEGRA20_DAS_DAP_CTRL_SEL_DAP_CTRL_SEL_P | >^ > sound/soc/tegra/tegra20_das.c:65:61: style: Boolean result is used in > bitwise operation. Clarify expression with > parentheses. [clarifyCondition] > > !!sdata2rx << TEGRA20_DAS_DAP_CTRL_SEL_DAP_SDATA2_TX_RX_P | > ^ > sound/soc/tegra/tegra20_das.c:66:61: style: Boolean result is used in > bitwise operation. Clarify expression with > parentheses. [clarifyCondition] > !!sdata1rx << TEGRA20_DAS_DAP_CTRL_SEL_DAP_SDATA1_TX_RX_P | > ^ > > Signed-off-by: Pierre-Louis Bossart > --- > sound/soc/tegra/tegra20_das.c | 8 ++++ > 1 file changed, 4 insertions(+), 4 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH] spi: dt-bindings: nvidia,tegra210-quad: Use documented compatible "jedec,spi-nor" in example
On Sat, Mar 27, 2021 at 03:33:57PM -0500, Rob Herring wrote: > The 'spi-nor' compatible used in the example is not documented. Use the > documented 'jedec,spi-nor' compatible instead. > > Cc: Mark Brown > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: linux-...@vger.kernel.org > Cc: linux-te...@vger.kernel.org > Signed-off-by: Rob Herring > --- > Documentation/devicetree/bindings/spi/nvidia,tegra210-quad.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v2 05/20] drm/dp: Add backpointer to drm_device in drm_dp_aux
On Fri, Mar 26, 2021 at 04:37:52PM -0400, Lyude Paul wrote: > This is something that we've wanted for a while now: the ability to > actually look up the respective drm_device for a given drm_dp_aux struct. > This will also allow us to transition over to using the drm_dbg_*() helpers > for debug message printing, as we'll finally have a drm_device to reference > for doing so. > > Note that there is one limitation with this - because some DP AUX adapters > exist as platform devices which are initialized independently of their > respective DRM devices, one cannot rely on drm_dp_aux->drm_dev to always be > non-NULL until drm_dp_aux_register() has been called. We make sure to point > this out in the documentation for struct drm_dp_aux. > > Signed-off-by: Lyude Paul > --- [...] > drivers/gpu/drm/tegra/dpaux.c | 1 + [...] Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v2 02/20] drm/tegra: Don't register DP AUX channels before connectors
On Fri, Mar 26, 2021 at 04:37:49PM -0400, Lyude Paul wrote: > As pointed out by the documentation for drm_dp_aux_register(), > drm_dp_aux_init() should be used in situations where the AUX channel for a > display driver can potentially be registered before it's respective DRM > driver. This is the case with Tegra, since the DP aux channel exists as a > platform device instead of being a grandchild of the DRM device. > > Since we're about to add a backpointer to a DP AUX channel's respective DRM > device, let's fix this so that we don't potentially allow userspace to use > the AUX channel before we've associated it with it's DRM connector. > > Signed-off-by: Lyude Paul > --- > drivers/gpu/drm/tegra/dpaux.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v2] ASoC: dt-bindings: nvidia,tegra210-ahub: Add missing child nodes
On Fri, Mar 26, 2021 at 01:50:03PM -0600, Rob Herring wrote: > The nvidia,tegra210-ahub binding is missing schema for child nodes. This > results in warnings if 'additionalProperties: false' is set (or when the > tools implement 'unevaluatedProperties' support). Add the child nodes > and reference their schema if one exists. > > Cc: Liam Girdwood > Cc: Mark Brown > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: Sameer Pujar > Cc: alsa-de...@alsa-project.org > Cc: linux-te...@vger.kernel.org > Signed-off-by: Rob Herring > --- > v2: > - Also add 'dspk' child node > > This patch ideally should be applied before this series[1]. > > [1] https://lore.kernel.org/r/20210323163634.877511-1-r...@kernel.org/ > --- > .../bindings/sound/nvidia,tegra210-ahub.yaml | 16 ++++ > 1 file changed, 16 insertions(+) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH] dt-bindings: Clean-up undocumented compatible strings
On Tue, Mar 16, 2021 at 01:49:18PM -0600, Rob Herring wrote: > Adding checks for undocumented compatible strings reveals a bunch of > warnings in the DT binding examples. Fix the cases which are typos, just > a mismatch between the schema and the example, or aren't documented at all. > In a couple of cases, fixing the compatible revealed some schema errors > which are fixed. > > There's a bunch of others remaining after this which have bindings, but > those aren't converted to schema yet. > > Cc: Stephen Boyd > Cc: Maxime Ripard > Cc: Thierry Reding > Cc: Sam Ravnborg > Cc: Vinod Koul > Cc: Alexandre Belloni > Cc: Jonathan Cameron > Cc: Pavel Machek > Cc: Kishon Vijay Abraham I > Cc: Sebastian Reichel > Cc: Mark Brown > Cc: Greg Kroah-Hartman > Cc: linux-...@vger.kernel.org > Cc: dmaeng...@vger.kernel.org > Cc: linux-...@lists.infradead.org > Cc: linux-...@vger.kernel.org > Cc: linux-l...@vger.kernel.org > Cc: linux...@vger.kernel.org > Cc: linux-ser...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Signed-off-by: Rob Herring > --- > .../clock/allwinner,sun4i-a10-pll1-clk.yaml | 2 +- > .../bindings/clock/milbeaut-clock.yaml| 12 + > .../bindings/display/brcm,bcm2835-dsi0.yaml | 6 - > .../bindings/display/panel/panel-dpi.yaml | 2 +- > .../devicetree/bindings/dma/qcom,gpi.yaml | 2 +- > .../devicetree/bindings/i3c/i3c.yaml | 7 ++--- > .../iio/adc/brcm,iproc-static-adc.yaml| 5 > .../iio/gyroscope/nxp,fxas21002c.yaml | 2 +- > .../bindings/iio/light/upisemi,us5182.yaml| 4 +-- > .../interrupt-controller/loongson,htpic.yaml | 2 +- > .../devicetree/bindings/leds/leds-lgm.yaml| 26 --- > .../bindings/phy/ti,phy-j721e-wiz.yaml| 2 +- > .../bindings/power/supply/cw2015_battery.yaml | 2 +- > .../bindings/power/supply/power-supply.yaml | 22 > .../devicetree/bindings/serial/serial.yaml| 2 +- > .../bindings/spi/amlogic,meson-gx-spicc.yaml | 4 +-- > .../bindings/spi/spi-controller.yaml | 21 --- > .../devicetree/bindings/spi/spi-mux.yaml | 8 ++ > .../devicetree/bindings/spi/st,stm32-spi.yaml | 6 - > 19 files changed, 58 insertions(+), 79 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [RFC PATCH 1/3] dt-bindings: display: simple: Add the panel on sc7180-trogdor-pompom
On Wed, Mar 17, 2021 at 06:53:04PM -0700, Rob Clark wrote: > On Wed, Mar 17, 2021 at 4:27 PM Matthias Kaehlcke wrote: > > > > On Tue, Mar 16, 2021 at 02:08:19PM -0700, Douglas Anderson wrote: > > > The sc7180-trogdor-pompom board might be attached to any number of a > > > pile of eDP panels. At the moment I'm told that the list might include: > > > - KD KD116N21-30NV-A010 > > > - KD KD116N09-30NH-A016 > > > - Starry 2081116HHD028001-51D > > > - Sharp LQ116M1JW10 > > > > > > It should be noted that while the EDID programmed in the first 3 > > > panels indicates that they should run with exactly the same timing (to > > > keep things simple), the 4th panel not only needs different timing but > > > has a different resolution. > > > > > > As is true in general with eDP panels, we can figure out which panel > > > we have and all the info needed to drive its pixel clock by reading > > > the EDID. However, we can do this only after we've powered the panel > > > on. Powering on the panels requires following the timing diagram in > > > each panel's datasheet which specifies delays between certain > > > actions. This means that, while we can be quite dynamic about handling > > > things we can't just totally skip out on describing the panel like we > > > could do if it was connected to an external-facing DP port. > > > > > > While the different panels have slightly different delays, it's > > > possible to come up with a set of unified delays that will work on all > > > the panels. From reading the datasheets: > > > * KD KD116N21-30NV-A010 and KD KD116N09-30NH-A016 > > > - HPD absent delay: 200 ms > > > - Unprepare delay: 150 ms (datasheet is confusing, might be 500 ms) > > > * Starry 2081116HHD028001-51D > > > - HPD absent delay: 100 ms > > > - Enable delay: (link training done till enable BL): 200 ms > > > - Unprepare delay: 500 ms > > > * Sharp LQ116M1JW10 > > > - HPD absent delay: 200 ms > > > - Unprepare delay: 500 ms > > > - Prepare to enable delay (power on till backlight): 100 ms > > > > > > Unified: > > > - HPD absent delay: 200 ms > > > - Unprepare delay: 500 ms > > > - Enable delay: 200 ms > > > > > > NOTE: in theory the only thing that we _really_ need unity on is the > > > "HPD absent delay" since once the panel asserts HPD we can read the > > > EDID and could make per-panel decisions if we wanted. > > > > > > Let's create a definition of "a panel that can be attached to pompom" > > > as a panel that provides a valid EDID and can work with the standard > > > pompom power sequencing. If more panels are later attached to pompom > > > then it's fine as long as they work in a compatible way. > > > > > > One might ask why we can't just use a generic string here and provide > > > the timings directly in the device tree file. As I understand it, > > > trying to describe generic power sequencing in the device tree is > > > frowned upon and the one instance (SD/MMC) is regarded as a mistake > > > that shouldn't be repeated. Specifying a power sequence per board (or > > > per board class) feels like a reasonable compromise. We're not trying > > > to define fully generic power sequence bindings but we can also take > > > advantage of the semi-probable properties of the attached device. > > > > > > NOTE: I believe that past instances of supporting this type of thing > > > have used the "white lie" approach. One representative panel was > > > listed in the device tree. The power sequencings of this > > > representative panel were OK to use across all panels that might be > > > attached and other differences were handled by EDID. This patch > > > attempts to set a new precedent and avoid the need for the white lie. > > > > > > Signed-off-by: Douglas Anderson > > > --- > > > > Sounds reasonable to me if DT maintainers can live with this abstract > > hardware definition. It's clearer than the 'white lie' approach. > > Yeah, it is a weird grey area between "discoverable" and "not > discoverable".. but I favor DT reflecting reality as much as > possible/feasible, so I think this is definity cleaner than "white > lies" This is practically no different than the "white lie". I suppose you could perhaps call it "more honest", if you want. The point remains that unless we describe exactly which panel we're dealing with, we ultimately have no way of properly quirking anything if we ever have to. Also, once we allow this kind of wildcard we can suddenly get into a situation where people might want to reuse this on something that's not at all a google-pompom board because the same particular power sequence happens to work on on some other board. Similarly I can imagine a situation where we could now have the same panel supported by multiple different wildcard compatible strings. How is that supposed to be any cleaner than what we have now? And I still keep wondering why bootloaders can't be taught about these kinds of things. We have in the past discussed various solutions where the bootloader could detect
Re: [PATCH v6 3/3] ARM: tegra: acer-a500: Add atmel,wakeup-method property
On Thu, Mar 25, 2021 at 11:15:54AM -0700, Dmitry Torokhov wrote: > On Thu, Mar 25, 2021 at 03:10:25PM +0100, Thierry Reding wrote: > > On Sun, Mar 21, 2021 at 03:40:28PM -0700, Dmitry Torokhov wrote: > > > On Tue, Mar 02, 2021 at 01:21:58PM +0300, Dmitry Osipenko wrote: > > > > Acer A500 uses Atmel Maxtouch 1386 touchscreen controller. This > > > > controller > > > > has WAKE line which could be connected to I2C clock lane, dedicated GPIO > > > > or fixed to HIGH level. Controller wakes up from a deep sleep when WAKE > > > > line is asserted low. Acer A500 has WAKE line connected to I2C clock and > > > > Linux device driver doesn't work property without knowing what wakeup > > > > method is used by h/w. > > > > > > > > Add atmel,wakeup-method property to the touchscreen node. > > > > > > > > Signed-off-by: Dmitry Osipenko > > > > > > Applied, thank you. > > > > I noticed that you had applied this as I was applying a different patch > > that touches the same area and it causes a conflict. In general I prefer > > to pick up all device tree changes into the Tegra tree, specifically to > > avoid such conflicts. > > > > That said, I didn't see an email from Stephen about this causing a > > conflict in linux-next, so perhaps it's fine. If this pops up again it > > might be worth considering to drop this from your tree so that I can > > resolve the conflict in the Tegra tree. > > Sorry about that, I went ahead and dropped the patch from my branch. Applied to the Tegra tree now. Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH v4 3/6] dt-bindings: power: tegra: Add binding for core power domain
On Wed, Mar 24, 2021 at 02:01:29AM +0300, Dmitry Osipenko wrote:
> 24.03.2021 01:48, Rob Herring пишет:
> > On Sun, Mar 14, 2021 at 07:48:07PM +0300, Dmitry Osipenko wrote:
> >> All NVIDIA Tegra SoCs have a core power domain where majority of hardware
> >> blocks reside. Add binding for the core power domain.
> >>
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> .../power/nvidia,tegra20-core-domain.yaml | 51 +++
> >> 1 file changed, 51 insertions(+)
> >> create mode 100644
> >> Documentation/devicetree/bindings/power/nvidia,tegra20-core-domain.yaml
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/power/nvidia,tegra20-core-domain.yaml
> >> b/Documentation/devicetree/bindings/power/nvidia,tegra20-core-domain.yaml
> >> new file mode 100644
> >> index ..4692489d780a
> >> --- /dev/null
> >> +++
> >> b/Documentation/devicetree/bindings/power/nvidia,tegra20-core-domain.yaml
> >> @@ -0,0 +1,51 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/power/nvidia,tegra20-core-domain.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: NVIDIA Tegra Core Power Domain
> >> +
> >> +maintainers:
> >> + - Dmitry Osipenko
> >> + - Jon Hunter
> >> + - Thierry Reding
> >> +
> >> +allOf:
> >> + - $ref: power-domain.yaml#
> >> +
> >> +properties:
> >> + compatible:
> >> +enum:
> >> + - nvidia,tegra20-core-domain
> >> + - nvidia,tegra30-core-domain
> >> +
> >> + operating-points-v2:
> >> +description:
> >> + Should contain level, voltages and opp-supported-hw property.
> >> + The supported-hw is a bitfield indicating SoC speedo or process
> >> + ID mask.
> >> +
> >> + "#power-domain-cells":
> >> +const: 0
> >> +
> >> + power-supply:
> >> +description:
> >> + Phandle to voltage regulator connected to the SoC Core power rail.
> >> +
> >> +required:
> >> + - compatible
> >> + - operating-points-v2
> >> + - "#power-domain-cells"
> >> + - power-supply
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> + - |
> >> +power-domain {
> >> +compatible = "nvidia,tegra20-core-domain";
> >> +operating-points-v2 = <_table>;
> >> +power-supply = <>;
> >> +#power-domain-cells = <0>;
> >
> > AFAICT, there's no way to access this 'hardware'?
> correct
To avoid exposing this "virtual" device in device tree, could this
instead be modelled as a child node of the PMC node? We already expose a
couple of generic power domains that way on Tegra210 and later, so
perhaps some of that infrastructure can be reused? I suppose given that
this is different from the standard powergate domains that we expose so
far, this may need a different implementation, but from a device tree
bindings point of view it could fit in with that.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v4 5/5] soc/tegra: pmc: Rate-limit error message about failed to acquire of reset
On Tue, Mar 02, 2021 at 03:25:02PM +0300, Dmitry Osipenko wrote:
> PMC domain could be easily bombarded with the enable requests if there is
> a problem in regards to acquiring reset control of a domain and kernel
> log will be flooded with the error message in this case. Hence rate-limit
> the message in order to prevent missing other important messages.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/soc/tegra/pmc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index bf29ea22480a..84ab27d85d92 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -868,8 +868,8 @@ static int tegra_genpd_power_off(struct generic_pm_domain
> *domain)
>
> err = reset_control_acquire(pg->reset);
> if (err < 0) {
> - dev_err(dev, "failed to acquire resets for PM domain %s: %d\n",
> - pg->genpd.name, err);
> + dev_err_ratelimited(dev, "failed to acquire resets for PM
> domain %s: %d\n",
> + pg->genpd.name, err);
That doesn't look right. This is a serious error condition that
shouldn't happen at all. Ever. If this shows up even once we've got a
serious bug somewhere and we need to fix it rather than "downplay" it
by ratelimiting these errors.
What's the exact use-case where you see this?
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v4 4/5] soc/tegra: pmc: Print out domain name when reset fails to acquire
On Tue, Mar 02, 2021 at 03:25:01PM +0300, Dmitry Osipenko wrote: > Print out domain name when reset fails to acquire for debugging purposes > and to make formatting of GENPD errors consistent in the driver. > > Tested-by: Peter Geis # Ouya T30 > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar # Ouya T30 > Signed-off-by: Dmitry Osipenko > --- > drivers/soc/tegra/pmc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 3/5] soc/tegra: pmc: Ensure that clock rates aren't too high
On Tue, Mar 02, 2021 at 03:25:00PM +0300, Dmitry Osipenko wrote:
> Switch all clocks of a power domain to a safe rate which is suitable
> for all possible voltages in order to ensure that hardware constraints
> aren't violated when power domain state toggles.
>
> Tested-by: Peter Geis # Ouya T30
> Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124
> Tested-by: Matt Merhar # Ouya T30
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/soc/tegra/pmc.c | 92 -
> 1 file changed, 90 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index f970b615ee27..a87645fac735 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -237,6 +237,7 @@ struct tegra_powergate {
> unsigned int id;
> struct clk **clks;
> unsigned int num_clks;
> + unsigned long *clk_rates;
> struct reset_control *reset;
> };
>
> @@ -641,6 +642,57 @@ static int __tegra_powergate_remove_clamping(struct
> tegra_pmc *pmc,
> return 0;
> }
>
> +static int tegra_powergate_prepare_clocks(struct tegra_powergate *pg)
> +{
> + unsigned long safe_rate = 100 * 1000 * 1000;
This seems a bit arbitrary. Where did you come up with that value?
I'm going to apply this to see how it fares in our testing.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v4 2/5] soc/tegra: pmc: Fix completion of power-gate toggling
On Tue, Mar 02, 2021 at 03:24:59PM +0300, Dmitry Osipenko wrote: > The SW-initiated power gate toggling is dropped by PMC if there is > contention with a HW-initiated toggling, i.e. when one of CPU cores is > gated by cpuidle driver. Software should retry the toggling after 10 > microseconds on Tegra20/30 SoCs, hence add the retrying. On Tegra114+ the > toggling method was changed in hardware, the TOGGLE_START bit indicates > whether PMC is busy or could accept the command to toggle, hence handle > that bit properly. > > The problem pops up after enabling dynamic power gating of 3d hardware, > where 3d power domain fails to turn on/off "randomly". > > The programming sequence and quirks are documented in TRMs, but PMC > driver obliviously re-used the Tegra20 logic for Tegra30+, which strikes > back now. The 10 microseconds and other timeouts aren't documented in TRM, > they are taken from downstream kernel. > > Link: > https://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commit;h=311dd1c318b70e93bcefec15456a10ff2b9eb0ff > Link: > https://nv-tegra.nvidia.com/gitweb/?p=linux-3.10.git;a=commit;h=7f36693c47cb23730a6b2822e0975be65fb0c51d > Tested-by: Peter Geis # Ouya T30 > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar # Ouya T30 > Signed-off-by: Dmitry Osipenko > --- > drivers/soc/tegra/pmc.c | 70 ++--- > 1 file changed, 65 insertions(+), 5 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 1/5] soc/tegra: pmc: Fix imbalanced clock disabling in error code path
On Tue, Mar 02, 2021 at 03:24:58PM +0300, Dmitry Osipenko wrote: > The tegra_powergate_power_up() has a typo in the error code path where it > will try to disable clocks twice, fix it. In practice that error never > happens, so this is a minor correction. > > Tested-by: Peter Geis # Ouya T30 > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar # Ouya T30 > Signed-off-by: Dmitry Osipenko > --- > drivers/soc/tegra/pmc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v6 3/3] ARM: tegra: acer-a500: Add atmel,wakeup-method property
On Sun, Mar 21, 2021 at 03:40:28PM -0700, Dmitry Torokhov wrote: > On Tue, Mar 02, 2021 at 01:21:58PM +0300, Dmitry Osipenko wrote: > > Acer A500 uses Atmel Maxtouch 1386 touchscreen controller. This controller > > has WAKE line which could be connected to I2C clock lane, dedicated GPIO > > or fixed to HIGH level. Controller wakes up from a deep sleep when WAKE > > line is asserted low. Acer A500 has WAKE line connected to I2C clock and > > Linux device driver doesn't work property without knowing what wakeup > > method is used by h/w. > > > > Add atmel,wakeup-method property to the touchscreen node. > > > > Signed-off-by: Dmitry Osipenko > > Applied, thank you. I noticed that you had applied this as I was applying a different patch that touches the same area and it causes a conflict. In general I prefer to pick up all device tree changes into the Tegra tree, specifically to avoid such conflicts. That said, I didn't see an email from Stephen about this causing a conflict in linux-next, so perhaps it's fine. If this pops up again it might be worth considering to drop this from your tree so that I can resolve the conflict in the Tegra tree. Thierry signature.asc Description: PGP signature
Re: [PATCH v1] soc/tegra: regulators: Fix locking up when voltage-spread is out of range
On Tue, Mar 02, 2021 at 04:18:00PM +0300, Dmitry Osipenko wrote:
> Fix voltage coupler lockup which happens when voltage-spread is out
> of range due to a bug in the code. The max-spread requirement shall be
> accounted when CPU regulator doesn't have consumers. This problem is
> observed on Tegra30 Ouya game console once system-wide DVFS is enabled
> in a device-tree.
>
> Fixes: 783807436f36 ("soc/tegra: regulators: Add regulators coupler for
> Tegra30")
> Cc: sta...@vger.kernel.org
> Reported-by: Peter Geis
> Tested-by: Peter Geis # Ouya T30
> Tested-by: Matt Merhar # Ouya T30
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/soc/tegra/regulators-tegra30.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v7 00/14] Tegra XHCI controller ELPG support
On Thu, Mar 25, 2021 at 11:45:16AM +0530, Vinod Koul wrote: > On 24-03-21, 14:32, Thierry Reding wrote: > > On Wed, Mar 24, 2021 at 01:39:32PM +0100, Thierry Reding wrote: > > > On Fri, Feb 05, 2021 at 05:22:29PM +0100, Greg KH wrote: > > > > On Fri, Feb 05, 2021 at 05:15:21PM +0100, Thierry Reding wrote: > > > > > On Wed, Jan 20, 2021 at 03:34:00PM +0800, JC Kuo wrote: > > > > > > Tegra XHCI controler can be placed in ELPG (Engine Level PowerGated) > > > > > > state for power saving when all of the connected USB devices are in > > > > > > suspended state. This patch series includes clk, phy and pmc changes > > > > > > that are required for properly place controller in ELPG and bring > > > > > > controller out of ELPG. > > > > > > > > > > > > JC Kuo (14): > > > > > > clk: tegra: Add PLLE HW power sequencer control > > > > > > clk: tegra: Don't enable PLLE HW sequencer at init > > > > > > phy: tegra: xusb: Move usb3 port init for Tegra210 > > > > > > phy: tegra: xusb: Rearrange UPHY init on Tegra210 > > > > > > phy: tegra: xusb: Add Tegra210 lane_iddq operation > > > > > > phy: tegra: xusb: Add sleepwalk and suspend/resume > > > > > > soc/tegra: pmc: Provide USB sleepwalk register map > > > > > > arm64: tegra210: XUSB PADCTL add "nvidia,pmc" prop > > > > > > dt-bindings: phy: tegra-xusb: Add nvidia,pmc prop > > > > > > phy: tegra: xusb: Add wake/sleepwalk for Tegra210 > > > > > > phy: tegra: xusb: Tegra210 host mode VBUS control > > > > > > phy: tegra: xusb: Add wake/sleepwalk for Tegra186 > > > > > > usb: host: xhci-tegra: Unlink power domain devices > > > > > > xhci: tegra: Enable ELPG for runtime/system PM > > > > > > > > > > > > .../phy/nvidia,tegra124-xusb-padctl.txt |1 + > > > > > > arch/arm64/boot/dts/nvidia/tegra210.dtsi |1 + > > > > > > drivers/clk/tegra/clk-pll.c | 12 - > > > > > > drivers/clk/tegra/clk-tegra210.c | 53 +- > > > > > > drivers/phy/tegra/xusb-tegra186.c | 558 - > > > > > > drivers/phy/tegra/xusb-tegra210.c | 1889 > > > > > > + > > > > > > drivers/phy/tegra/xusb.c | 92 +- > > > > > > drivers/phy/tegra/xusb.h | 22 +- > > > > > > drivers/soc/tegra/pmc.c | 94 + > > > > > > drivers/usb/host/xhci-tegra.c | 613 -- > > > > > > include/linux/clk/tegra.h |4 +- > > > > > > include/linux/phy/tegra/xusb.h| 10 +- > > > > > > 12 files changed, 2784 insertions(+), 565 deletions(-) > > > > > > > > > > > > v5 "phy: tegra: xusb: tegra210: Do not reset UPHY PLL" is moved > > > > > > into v6 "phy: tegra: xusb: Rearrange UPHY init on Tegra210" > > > > > > > > > > Mike, Stephen, > > > > > > > > > > could you guys take a look at the two clk patches here and give an > > > > > Acked-by? There's build-time dependencies throughout the series, so > > > > > it'd > > > > > be good if they can all go through either the PHY or USB trees. > > > > > > > > > > Kishon, Greg, > > > > > > > > > > any comments on these patches? Unfortunately, the USB patches in this > > > > > series have a build-time dependency on the PHY patches, so this should > > > > > all go through one tree. Since this all culminates in the XHCI driver, > > > > > merging this through the USB tree might be best, provided that Kishon > > > > > provides his Acked-by on the PHY patches. > > > > > > > > > > Alternatively, I can create a set of branches with the correct > > > > > dependencies and send out pull requests for the three subsystems if > > > > > that's preferrable. > > > > > > > > I have no objection for the usb patches to go through your tree as they > > > > are hardware-specific. > > > > > > Kishon, > > > > > > I haven't heard back from you on this y
Re: [PATCH v1] drm/tegra: dc: Don't set PLL clock to 0Hz
On Tue, Mar 02, 2021 at 04:15:06PM +0300, Dmitry Osipenko wrote: > RGB output doesn't allow to change parent clock rate of the display and > PCLK rate is set to 0Hz in this case. The tegra_dc_commit_state() shall > not set the display clock to 0Hz since this change propagates to the > parent clock. The DISP clock is defined as a NODIV clock by the tegra-clk > driver and all NODIV clocks use the CLK_SET_RATE_PARENT flag. > > This bug stayed unnoticed because by default PLLP is used as the parent > clock for the display controller and PLLP silently skips the erroneous 0Hz > rate changes because it always has active child clocks that don't permit > rate changes. The PLLP isn't acceptable for some devices that we want to > upstream (like Samsung Galaxy Tab and ASUS TF700T) due to a display panel > clock rate requirements that can't be fulfilled by using PLLP and then the > bug pops up in this case since parent clock is set to 0Hz, killing the > display output. > > Don't touch DC clock if pclk=0 in order to fix the problem. > > Signed-off-by: Dmitry Osipenko > --- > drivers/gpu/drm/tegra/dc.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 00/14] NVIDIA Tegra ARM32 device-tree improvements
On Tue, Mar 02, 2021 at 03:09:49PM +0300, Dmitry Osipenko wrote: > Hi, > > This series is partially factored out from [1] since the DT patches > could be applied separately. In addition I added couple more new > patches and implemented suggestion given by Daniel Lezcano to [1], > see "Specify all CPU cores as cooling devices" patches. > > [1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=221130 > > Please note that this patchset enables voltage scaling for a few boards, > but currently voltage scaling is limited in kernel by the regulator coupler > drivers, so it's safe to change the device-trees. Voltage scaling will > be fully unlocked once [1] will be merged. > > Changelog: > > v3: - Added new "Specify tps65911 as wakeup source" patch, which previously > was a part of series that added wakeup-source support to TPS65911 RTC > driver [1]. The RTC series was partially applied, excluding the DT > patch. > > [1] > https://lore.kernel.org/linux-rtc/20210120211603.18555-1-dig...@gmail.com/ > > v2: - The "acer-a500: Rename avdd to vdda of touchscreen node" patch > now shouldn't have merge conflicts with the upstream kernel since > v1 was based on a patch that adds a new atmel,wakeup-method property, > which is not supported by upstream yet. > > - Fixed unwrapped commit description in the "cardhu: Support CPU > frequency and voltage" patch. > > Dmitry Osipenko (14): > ARM: tegra: ventana: Support CPU and Core voltage scaling > ARM: tegra: ventana: Support CPU thermal throttling > ARM: tegra: cardhu: Support CPU frequency and voltage scaling on all > board variants > ARM: tegra: cardhu: Support CPU thermal throttling > ARM: tegra: paz00: Enable full voltage scaling ranges for CPU and Core > domains > ARM: tegra: acer-a500: Enable core voltage scaling > ARM: tegra: acer-a500: Reduce thermal throttling hysteresis to 0.2C > ARM: tegra: acer-a500: Specify all CPU cores as cooling devices > ARM: tegra: acer-a500: Rename avdd to vdda of touchscreen node > ARM: tegra: nexus7: Specify all CPU cores as cooling devices > ARM: tegra: ouya: Specify all CPU cores as cooling devices > ARM: tegra: Specify CPU suspend OPP in device-tree > ARM: tegra: Specify memory suspend OPP in device-tree > ARM: tegra: Specify tps65911 as wakeup source > > .../boot/dts/tegra124-peripherals-opp.dtsi| 5 ++ > .../boot/dts/tegra20-acer-a500-picasso.dts| 14 ++-- > arch/arm/boot/dts/tegra20-cpu-opp.dtsi| 2 + > arch/arm/boot/dts/tegra20-paz00.dts | 14 ++-- > .../arm/boot/dts/tegra20-peripherals-opp.dtsi | 1 + > arch/arm/boot/dts/tegra20-ventana.dts | 78 ++--- > arch/arm/boot/dts/tegra30-apalis.dtsi | 1 + > .../tegra30-asus-nexus7-grouper-common.dtsi | 14 +++- > .../tegra30-asus-nexus7-grouper-ti-pmic.dtsi | 1 + > arch/arm/boot/dts/tegra30-beaver.dts | 1 + > arch/arm/boot/dts/tegra30-cardhu-a04.dts | 48 --- > arch/arm/boot/dts/tegra30-cardhu.dtsi | 84 ++- > arch/arm/boot/dts/tegra30-colibri.dtsi| 1 + > arch/arm/boot/dts/tegra30-cpu-opp.dtsi| 3 + > arch/arm/boot/dts/tegra30-ouya.dts| 16 +++- > .../arm/boot/dts/tegra30-peripherals-opp.dtsi | 3 + > 16 files changed, 202 insertions(+), 84 deletions(-) All of these applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 09/14] ARM: tegra: acer-a500: Rename avdd to vdda of touchscreen node
On Tue, Mar 02, 2021 at 03:09:58PM +0300, Dmitry Osipenko wrote:
> Rename avdd supply to vdda of the touchscreen node. The old supply name
> was incorrect.
>
> Signed-off-by: Dmitry Osipenko
> ---
> arch/arm/boot/dts/tegra20-acer-a500-picasso.dts | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
This one didn't apply cleanly, but applying it manually was fine...
>
> diff --git a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> index 8a98e4a9d994..d852527db707 100644
> --- a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> +++ b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> @@ -449,7 +449,7 @@ touchscreen@4c {
>
> reset-gpios = < TEGRA_GPIO(Q, 7) GPIO_ACTIVE_LOW>;
>
> - avdd-supply = <_3v3_sys>;
> + vdda-supply = <_3v3_sys>;
> vdd-supply = <_3v3_sys>;
>
> atmel,wakeup-method = ;
Looks like this line is not upstream. Did I miss a patch somewhere?
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v7 00/14] Tegra XHCI controller ELPG support
On Wed, Mar 24, 2021 at 01:39:32PM +0100, Thierry Reding wrote: > On Fri, Feb 05, 2021 at 05:22:29PM +0100, Greg KH wrote: > > On Fri, Feb 05, 2021 at 05:15:21PM +0100, Thierry Reding wrote: > > > On Wed, Jan 20, 2021 at 03:34:00PM +0800, JC Kuo wrote: > > > > Tegra XHCI controler can be placed in ELPG (Engine Level PowerGated) > > > > state for power saving when all of the connected USB devices are in > > > > suspended state. This patch series includes clk, phy and pmc changes > > > > that are required for properly place controller in ELPG and bring > > > > controller out of ELPG. > > > > > > > > JC Kuo (14): > > > > clk: tegra: Add PLLE HW power sequencer control > > > > clk: tegra: Don't enable PLLE HW sequencer at init > > > > phy: tegra: xusb: Move usb3 port init for Tegra210 > > > > phy: tegra: xusb: Rearrange UPHY init on Tegra210 > > > > phy: tegra: xusb: Add Tegra210 lane_iddq operation > > > > phy: tegra: xusb: Add sleepwalk and suspend/resume > > > > soc/tegra: pmc: Provide USB sleepwalk register map > > > > arm64: tegra210: XUSB PADCTL add "nvidia,pmc" prop > > > > dt-bindings: phy: tegra-xusb: Add nvidia,pmc prop > > > > phy: tegra: xusb: Add wake/sleepwalk for Tegra210 > > > > phy: tegra: xusb: Tegra210 host mode VBUS control > > > > phy: tegra: xusb: Add wake/sleepwalk for Tegra186 > > > > usb: host: xhci-tegra: Unlink power domain devices > > > > xhci: tegra: Enable ELPG for runtime/system PM > > > > > > > > .../phy/nvidia,tegra124-xusb-padctl.txt |1 + > > > > arch/arm64/boot/dts/nvidia/tegra210.dtsi |1 + > > > > drivers/clk/tegra/clk-pll.c | 12 - > > > > drivers/clk/tegra/clk-tegra210.c | 53 +- > > > > drivers/phy/tegra/xusb-tegra186.c | 558 - > > > > drivers/phy/tegra/xusb-tegra210.c | 1889 + > > > > drivers/phy/tegra/xusb.c | 92 +- > > > > drivers/phy/tegra/xusb.h | 22 +- > > > > drivers/soc/tegra/pmc.c | 94 + > > > > drivers/usb/host/xhci-tegra.c | 613 -- > > > > include/linux/clk/tegra.h |4 +- > > > > include/linux/phy/tegra/xusb.h| 10 +- > > > > 12 files changed, 2784 insertions(+), 565 deletions(-) > > > > > > > > v5 "phy: tegra: xusb: tegra210: Do not reset UPHY PLL" is moved > > > > into v6 "phy: tegra: xusb: Rearrange UPHY init on Tegra210" > > > > > > Mike, Stephen, > > > > > > could you guys take a look at the two clk patches here and give an > > > Acked-by? There's build-time dependencies throughout the series, so it'd > > > be good if they can all go through either the PHY or USB trees. > > > > > > Kishon, Greg, > > > > > > any comments on these patches? Unfortunately, the USB patches in this > > > series have a build-time dependency on the PHY patches, so this should > > > all go through one tree. Since this all culminates in the XHCI driver, > > > merging this through the USB tree might be best, provided that Kishon > > > provides his Acked-by on the PHY patches. > > > > > > Alternatively, I can create a set of branches with the correct > > > dependencies and send out pull requests for the three subsystems if > > > that's preferrable. > > > > I have no objection for the usb patches to go through your tree as they > > are hardware-specific. > > Kishon, > > I haven't heard back from you on this yet. We missed v5.12 but I'd like > to get this into v5.13 since it's the last missing piece to get suspend > and resume working properly on a number of boards. > > Are you okay if I take this through the Tegra tree to satisfy the > interdependencies between clk, PHY and USB patches? I've already got > Acked-by's from the clock and USB maintainers. > > I want to tentatively take this into linux-next to give it a bit of soak > time before the ARM SoC -rc6 cut-off. Please let me know if you'd prefer > to apply these to your tree so I can back them out of the Tegra tree > again. Also adding Vinod for visibility and in case Kishon's too busy. Thierry signature.asc Description: PGP signature
Re: [PATCH v7 00/14] Tegra XHCI controller ELPG support
On Fri, Feb 05, 2021 at 05:22:29PM +0100, Greg KH wrote: > On Fri, Feb 05, 2021 at 05:15:21PM +0100, Thierry Reding wrote: > > On Wed, Jan 20, 2021 at 03:34:00PM +0800, JC Kuo wrote: > > > Tegra XHCI controler can be placed in ELPG (Engine Level PowerGated) > > > state for power saving when all of the connected USB devices are in > > > suspended state. This patch series includes clk, phy and pmc changes > > > that are required for properly place controller in ELPG and bring > > > controller out of ELPG. > > > > > > JC Kuo (14): > > > clk: tegra: Add PLLE HW power sequencer control > > > clk: tegra: Don't enable PLLE HW sequencer at init > > > phy: tegra: xusb: Move usb3 port init for Tegra210 > > > phy: tegra: xusb: Rearrange UPHY init on Tegra210 > > > phy: tegra: xusb: Add Tegra210 lane_iddq operation > > > phy: tegra: xusb: Add sleepwalk and suspend/resume > > > soc/tegra: pmc: Provide USB sleepwalk register map > > > arm64: tegra210: XUSB PADCTL add "nvidia,pmc" prop > > > dt-bindings: phy: tegra-xusb: Add nvidia,pmc prop > > > phy: tegra: xusb: Add wake/sleepwalk for Tegra210 > > > phy: tegra: xusb: Tegra210 host mode VBUS control > > > phy: tegra: xusb: Add wake/sleepwalk for Tegra186 > > > usb: host: xhci-tegra: Unlink power domain devices > > > xhci: tegra: Enable ELPG for runtime/system PM > > > > > > .../phy/nvidia,tegra124-xusb-padctl.txt |1 + > > > arch/arm64/boot/dts/nvidia/tegra210.dtsi |1 + > > > drivers/clk/tegra/clk-pll.c | 12 - > > > drivers/clk/tegra/clk-tegra210.c | 53 +- > > > drivers/phy/tegra/xusb-tegra186.c | 558 - > > > drivers/phy/tegra/xusb-tegra210.c | 1889 + > > > drivers/phy/tegra/xusb.c | 92 +- > > > drivers/phy/tegra/xusb.h | 22 +- > > > drivers/soc/tegra/pmc.c | 94 + > > > drivers/usb/host/xhci-tegra.c | 613 -- > > > include/linux/clk/tegra.h |4 +- > > > include/linux/phy/tegra/xusb.h| 10 +- > > > 12 files changed, 2784 insertions(+), 565 deletions(-) > > > > > > v5 "phy: tegra: xusb: tegra210: Do not reset UPHY PLL" is moved > > > into v6 "phy: tegra: xusb: Rearrange UPHY init on Tegra210" > > > > Mike, Stephen, > > > > could you guys take a look at the two clk patches here and give an > > Acked-by? There's build-time dependencies throughout the series, so it'd > > be good if they can all go through either the PHY or USB trees. > > > > Kishon, Greg, > > > > any comments on these patches? Unfortunately, the USB patches in this > > series have a build-time dependency on the PHY patches, so this should > > all go through one tree. Since this all culminates in the XHCI driver, > > merging this through the USB tree might be best, provided that Kishon > > provides his Acked-by on the PHY patches. > > > > Alternatively, I can create a set of branches with the correct > > dependencies and send out pull requests for the three subsystems if > > that's preferrable. > > I have no objection for the usb patches to go through your tree as they > are hardware-specific. Kishon, I haven't heard back from you on this yet. We missed v5.12 but I'd like to get this into v5.13 since it's the last missing piece to get suspend and resume working properly on a number of boards. Are you okay if I take this through the Tegra tree to satisfy the interdependencies between clk, PHY and USB patches? I've already got Acked-by's from the clock and USB maintainers. I want to tentatively take this into linux-next to give it a bit of soak time before the ARM SoC -rc6 cut-off. Please let me know if you'd prefer to apply these to your tree so I can back them out of the Tegra tree again. Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH] thermal: Fix a typo in the file soctherm.c
On Fri, Mar 05, 2021 at 07:23:20AM +0530, Bhaskar Chowdhury wrote: > > s/calibaration/calibration/ > > Signed-off-by: Bhaskar Chowdhury > --- > drivers/thermal/tegra/soctherm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: GTE - The hardware timestamping engine
On Tue, Mar 23, 2021 at 10:08:00AM +0100, Linus Walleij wrote: > On Mon, Mar 22, 2021 at 9:17 PM Dipen Patel wrote: > > > My follow-up concerns on both Linus's and Kent's feedback: > > > > 1. Please correct me if I am wrong, lineevent in the gpiolib* is only > > serves the userspace clients. > > 1.a What about kernel drivers wanting to use this feature for monitoring its > > GPIO lines, see gyroscope example somewhere below. In that regards, > > lineevent implementation is not sufficient. > > 1.b Are you also implying to extend lineevent implementation to kernel > > drivers? > > I was talking about lineevent because you mentioned things like > motors and robotics, and those things are traditionally not run in > kernelspace because they are not generic hardware that fit in the > kernel subsystems. > > Normally industrial automatic control tasks are run in a userspace > thread with some realtime priority. > > As Kent says, in-kernel events are exclusively using IRQ as > mechanism, and should be modeled as IRQs. Then the question > is how you join the timestamp with the IRQ. GPIO chips are > just some kind of irqchip in this regard, we reuse the irqchip > infrastructure in the kernel for all GPIO drivers that generate > "events" in response to state transitions on digital lines. One potential problem I see with this is that Kent's proposal, if I understand correctly, would supplant the original IRQ of a device with the GTE IRQ for the corresponding event. I'm not sure that's desirable because that would require modifying the device tree and would no longer accurately represent the hardware. Timestamping also sounds like something that drivers would want to opt into, and requiring people to update the device tree to achieve this just doesn't seem reasonable. This proposal would also only work if there's a 1:1 correspondence between hardware IRQ and GTE IRQ. However, as Dipen mentioned, the GTE events can be configured with a threshold, so a GTE IRQ might only trigger every, say, 5th hardware IRQ. I'm not sure if those are common use-cases, though. Obviously if we don't integrate this with IRQs directly, it becomes a bit more difficult to relate the captured timestamps to the events across subsystem boundaries. I'm not sure how this would be solved properly. If the events are sufficiently rare, and it's certain that none will be missed, then it should be possible to just pull a timestamp from the timestamp FIFO for each event. All of that said, I wonder if perhaps hierarchical IRQ domains can somehow be used for this. We did something similar on Tegra not too long ago for wake events, which are basically IRQs exposed by a parent IRQ chip that allows waking up from system sleep. There are some similarities between that and GTE in that the wake events also map to a subset of GPIOs and IRQs and provide additional functionalities on top. I managed to mess up the implementation and Marc stepped in to clean things up, so Cc'ing him since he's clearly more familiar with the topic than I am. Thierry signature.asc Description: PGP signature
Re: GTE - The hardware timestamping engine
On Mon, Mar 22, 2021 at 01:33:38PM -0700, Dipen Patel wrote: > Hi Richard, > > Thanks for your input and time. Please see below follow up. > > On 3/20/21 8:38 AM, Richard Cochran wrote: > > On Sat, Mar 20, 2021 at 01:44:20PM +0100, Arnd Bergmann wrote: > >> Adding Richard Cochran as well, for drivers/ptp/, he may be able to > >> identify whether this should be integrated into that framework in some > >> form. > > > > I'm not familiar with the GTE, but it sounds like it is a (free > > running?) clock with time stamping inputs. If so, then it could > > expose a PHC. That gets you functionality: > > > > - clock_gettime() and friends > > - comparison ioctl between GTE clock and CLOCK_REALTIME > > - time stamping channels with programmable input selection > > > GTE gets or rather records the timestamps from the TSC > (timestamp system coutner) so its not attached to GTE as any > one can access TSC, so not sure if we really need to implement PHC > and/or clock_* and friends for the GTE. I believe burden to find correlation > between various clock domains should be on the clients, consider below > example. I agree. My understanding is the the TSC is basically an SoC-wide clock that can be (and is) used by several hardware blocks. There's an interface for software to read out the value, but it's part of a block called TKE (time-keeping engine, if I recall correctly) that implements various clock sources and watchdog functionality. As a matter of fact, I recall typing up a driver for that at some point but I don't recall if I ever sent it out or what became of it. I can't find it upstream at least. Anyway, I think given that the GTE doesn't provide that clock itself but rather just a means of taking a snapshot of that clock and stamping certain events with that, it makes more sense to provide that clock from the TKE driver. Thierry signature.asc Description: PGP signature
Re: [PATCH] clk: tegra: fix old-style declaration
On Mon, Mar 22, 2021 at 10:50:41PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> With extra warnings enabled, gcc complains about a slightly odd
> prototype:
>
> drivers/clk/tegra/clk-dfll.c:1380:1: error: 'inline' is not at beginning of
> declaration [-Werror=old-style-declaration]
> 1380 | static void inline dfll_debug_init(struct tegra_dfll *td) { }
>
> Move the 'inline' keyword to the start of the line.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/clk/tegra/clk-dfll.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH v8 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
On Fri, Mar 12, 2021 at 01:24:54PM +0100, Nicolas Saenz Julienne wrote: > Adds support to control the PWM bus available in official Raspberry Pi > PoE HAT. Only RPi's co-processor has access to it, so commands have to > be sent through RPi's firmware mailbox interface. > > Signed-off-by: Nicolas Saenz Julienne > > --- > > Changes since v7: > - Remove unwarranted RPI_PWM_DEF_DUTY_REG usage > > Changes since v6: > - Use %pe > - Round divisions properly > - Use dev_err_probe() > - Pass check_patch > > Changes since v3: > - Rename compatible string to be more explicit WRT to bus's limitations > > Changes since v2: > - Use devm_rpi_firmware_get() > - Rename driver > - Small cleanups > > Changes since v1: > - Use default pwm bindings and get rid of xlate() function > - Correct spelling errors > - Correct apply() function > - Round values > - Fix divisions in arm32 mode > - Small cleanups > > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-raspberrypi-poe.c | 206 ++ > 3 files changed, 216 insertions(+) > create mode 100644 drivers/pwm/pwm-raspberrypi-poe.c Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote: > Hi Sven, > > On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote: > > Hi Clemens, > > > > On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber > > wrote: > > > > > > Ok, so you suggest we extend our get_state logic to deal with cases > > > like the following: > > > > Kind of. We can't control how other actors (bootloaders etc) program the > > chip. As far as I know, there are many, many different register settings > > that > > result in the same physical chip outputs. So if .probe() wants to preserve > > the > > existing chip settings, .get_state() has to be able to deal with every > > possible > > setting. Even invalid ones. > > Is the driver really responsible for bootloaders that program the chip > with invalid values? > The chip comes out of PoR with sane default values. If the bootloader of > a user messes them up, isn't that a bootloader problem instead of a > Linux kernel driver problem? It is ultimately a problem of the bootloader and where possible the bootloader should be fixed. However, fixing bootloaders sometimes isn't possible, or impractical, so the kernel has to be able to deal with hardware that's been badly programmed by the bootloader. Within reason, of course. Sometimes this can't be done in any other way than forcing a hard reset of the chip, but it should always be a last resort. > > In addition, .apply() cannot make any assumptions as to which bits are > > already set/cleared on the chip. Including preserved, invalid settings. > > > > This might get quite complex. > > > > However if we reset the chip in .probe() to a known state (a normalized > > state, > > in the mathematical sense), then both .get_state() and .apply() become > > much simpler. because they only need to deal with known, normalized states. > > Yes, I agree. This would however make it impossible to do a flicker-free > transition from bootloader to kernel, but that's not really a usecase I > have so I can live without it. > > Another point in favor of resetting is that the driver already does it. > Removing the reset of the OFF register may break some boards who rely on > that behaviour. > My version only extended the reset to include the ON register. > > > > > In short, it's a tradeoff between code complexity, and user friendliness/ > > features. > > > > Sven > > Thierry, Uwe, what's your take on this? > > Thierry: Would you accept it if we continue to reset the registers in > .probe? Yes, I think it's fine to continue to reset the registers since that's basically what the driver already does. It'd be great if you could follow up with a patch that removes the reset and leaves the hardware in whatever state the bootloader has set up. Then we can take that patch for a ride and see if there are any complains about it breaking. If there are we can always try to fix them, but as a last resort we can also revert, which then may be something we have to live with. But I think we should at least try to make this consistent with how other drivers do this so that people don't stumble over this particular driver's behaviour. Thierry signature.asc Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote: > Hi Clemens, > > On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber > wrote: > > > > Ok, so you suggest we extend our get_state logic to deal with cases > > like the following: > > Kind of. We can't control how other actors (bootloaders etc) program the > chip. As far as I know, there are many, many different register settings that > result in the same physical chip outputs. So if .probe() wants to preserve the > existing chip settings, .get_state() has to be able to deal with every > possible > setting. Even invalid ones. I said earlier that the PWM state is a snapshot of the current hardware settings and that's not entirely accurate because it isn't actually a complete representation of the hardware state. It's merely a representation of the PWM software state that's currently applied to the hardware. This is simpler from an API point of view than completely representing the actual hardware state, but it's also sufficient for most use-cases because we don't care about the exact programming as long as it yields the result represented by the atomic state. Although it's still vitally important that the amount of state that we have is accurately represented (i.e. duty-cycle/period values must not be collapsed to 0 when the PWM is off), otherwise the API isn't usable. One good thing that comes from this simplification is that it gives us a bit more flexibility in hardware readout because you can collapse a large amount of variation into the couple of values that we have. So if your bootloaders program weird values, you can canonicalize them as long as they still yield the same result. So roughly what should be guaranteed from an atomic API point of view is that doing the following is glitch-free and doesn't cause a change in the physical PWM signal: chip->ops->get_state(chip, pwm, ); pwm_apply_state(pwm, ); Ideally we'd even be able to, though we don't do it at present, to optimize that out as a no-op by comparing the new state with the current state and just not doing anything if they are equal. And just to clarify: glitch-free above means: to the extent possible. In some cases it might not be possible to set PWM hardware state in a completely glitch-free way. If so, there's not a lot we can do and it's better to do something even if it's not ideal. The rationale behind this is that nobody will select a chip that doesn't meet requirements to perform a given task, so it's highly unlikely that a chip that glitches during transitions will ever be used in a setup where it's required not to glitch. We should obviously always do our best to keep glitches to a minimum, but software can't change hardware... > In addition, .apply() cannot make any assumptions as to which bits are > already set/cleared on the chip. Including preserved, invalid settings. > > This might get quite complex. > > However if we reset the chip in .probe() to a known state (a normalized state, > in the mathematical sense), then both .get_state() and .apply() become > much simpler. because they only need to deal with known, normalized states. As was mentioned before, this does restrict the usability of the driver. In some cases you really want to avoid resetting the chip. But I'm also okay with leaving this as-is because it's the status quo. So what I'd propose is to take this forward and keep the reset during probe for now and then follow up with a separate, simple patch that removes the reset. That way we can easily back it out, or revert it, if it causes any breakage, but it won't hold up this series. Thierry signature.asc Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
On Thu, Jan 14, 2021 at 06:16:22PM +0100, Clemens Gruber wrote: > Hi, > > On Mon, Jan 11, 2021 at 09:35:32PM +0100, Uwe Kleine-König wrote: > > Hello, > > > > On Thu, Dec 17, 2020 at 06:43:04PM +0100, Clemens Gruber wrote: > > > On Wed, Dec 16, 2020 at 11:00:59PM -0500, Sven Van Asbroeck wrote: > > > > On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber > > > > wrote: > > > > > > > > > > Implements .get_state to read-out the current hardware state. > > > > > > > > > > > > > I am not convinced that we actually need this. > > > > > > > > Looking at the pwm core, .get_state() is only called right after > > > > .request(), > > > > to initialize the cached value of the state. The core then uses the > > > > cached > > > > value throughout, it'll never read out the h/w again, until the next > > > > .request(). > > > > > > > > In our case, we know that the state right after request is always > > > > disabled, > > > > because: > > > > - we disable all pwm channels on probe (in PATCH v5 4/7) > > > > - .free() disables the pwm channel > > > > > > > > Conclusion: .get_state() will always return "pwm disabled", so why do we > > > > bother reading out the h/w? > > > > > > If there are no plans for the PWM core to call .get_state more often in > > > the future, we could just read out the period and return 0 duty and > > > disabled. > > > > > > Thierry, Uwe, what's your take on this? > > > > I have some plans here. In the past I tried to implement them (see > > commit 01ccf903edd65f6421612321648fa5a7f4b7cb10), but this failed > > (commit 40a6b9a00930fd6b59aa2eb6135abc2efe5440c3). > > > > > > Of course, if we choose to leave the pwm enabled after .free(), then > > > > .get_state() can even be left out! Do we want that? Genuine question, I > > > > do > > > > not know the answer. > > > > > > I do not think we should leave it enabled after free. It is less > > > complicated if we know that unrequested channels are not in use. > > > > My position here is: A consumer should disable a PWM before calling > > pwm_put. The driver should however not enforce this and so should not > > modify the hardware state in .free(). > > > > Also .probe should not change the PWM configuration. > > I see. This would also allow PWMs initialized in the bootloader (e.g. > backlights) to stay on between the bootloader and Linux and avoid > flickering. Yes, that's precisely one of the reasons why we introduced the atomic API. One of the use-cases that led to the current design was that the kernel pwm-regulator on some platforms was causing devices to crash because the driver would reprogram the PWM and cause a glitch on the power supply for the CPUs. So it's crucial in some cases that the PWM driver don't touch the hardware state in ->probe(). If some drivers currently do so, that's something we should eventually change, but given that there haven't been any complaints yet, it likely means nothing breaks because of this, so we do have the luxury of not having to rush things. > If no one objects, I would then no longer reset period and duty cycles > in the driver (and for our projects, reset them in the bootloader code > to avoid leaving PWMs on after a kernel panic and watchdog reset, etc.) This isn't strictly necessary, but it's obviously something that's up to board designers/maintainers to decide. Thierry signature.asc Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
On Mon, Jan 11, 2021 at 09:35:32PM +0100, Uwe Kleine-König wrote: > Hello, > > On Thu, Dec 17, 2020 at 06:43:04PM +0100, Clemens Gruber wrote: > > On Wed, Dec 16, 2020 at 11:00:59PM -0500, Sven Van Asbroeck wrote: > > > On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber > > > wrote: > > > > > > > > Implements .get_state to read-out the current hardware state. > > > > > > > > > > I am not convinced that we actually need this. > > > > > > Looking at the pwm core, .get_state() is only called right after > > > .request(), > > > to initialize the cached value of the state. The core then uses the cached > > > value throughout, it'll never read out the h/w again, until the next > > > .request(). > > > > > > In our case, we know that the state right after request is always > > > disabled, > > > because: > > > - we disable all pwm channels on probe (in PATCH v5 4/7) > > > - .free() disables the pwm channel > > > > > > Conclusion: .get_state() will always return "pwm disabled", so why do we > > > bother reading out the h/w? > > > > If there are no plans for the PWM core to call .get_state more often in > > the future, we could just read out the period and return 0 duty and > > disabled. > > > > Thierry, Uwe, what's your take on this? > > I have some plans here. In the past I tried to implement them (see > commit 01ccf903edd65f6421612321648fa5a7f4b7cb10), but this failed > (commit 40a6b9a00930fd6b59aa2eb6135abc2efe5440c3). > > > > Of course, if we choose to leave the pwm enabled after .free(), then > > > .get_state() can even be left out! Do we want that? Genuine question, I do > > > not know the answer. > > > > I do not think we should leave it enabled after free. It is less > > complicated if we know that unrequested channels are not in use. > > My position here is: A consumer should disable a PWM before calling > pwm_put. The driver should however not enforce this and so should not > modify the hardware state in .free(). There had been discussions in the past about at least letting the PWM core warn about any PWMs that had been left enabled after pwm_put(). I still think that's worthwhile to do because it is consistent with how the rest of the kernel works (i.e. drivers are supposed to leave devices in a quiescent state when they relinquish control), and consumers are ultimately responsible for making sure they've cleaned up their resources. Most PWM drivers do a variant of this and assert a reset, disable clocks and/or release runtime PM references when removing the PWM chip on ->remove(), but that happens at a different time than pwm_put(), so I think it makes sense to nudge consumers in the right direction and WARN when they've left a PWM enabled when calling pwm_put(). If we do that, it shouldn't take very long for any violations to get fixed and then we don't have to enforce this in PWM drivers or the core. Thierry signature.asc Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote: > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote: > > Another point is the period: Sven suggested we do not read out the > > period at all, as the PWM is disabled anyway (see above). > > Is this acceptable? > > In my eyes consumers should consider the period value as "don't care" if > the PWM is off. But this doesn't match reality (and maybe also it > doesn't match Thierry's opinion). See for example the > drivers/video/backlight/pwm_bl.c driver which uses the idiom: > > pwm_get_state(mypwm, ); > state.enabled = true; > pwm_apply_state(pb->pwm, ); > > which breaks if .period is invalid. (OK, this isn't totally accurate > because currently the .get_state callback has only little to do with > pwm_get_state(), but you get the point.) The idea behind atomic states in the PWM API is to provide accurate snapshots of a PWM channel's settings. It's not a representation of the PWM channel's physical output, although in some cases they may be the same. However, there's no 1:1 correspondence between those two. For example, when looking purely at the physical output of a PWM it is in most cases not possible to make the distinction between these two states: - duty: 0 period: 100 - duty: 0 period: 200 Because the output will be a constant 0 (or 1, depending on polarity). However, given that we want a snapshot of the currently configured settings, we can't simply assume that there's a 1:1 correspondence and then use shortcuts to simplify the hardware state representation because it isn't going to be accurate. It is entirely expected that consumers will be able to use an existing atomic state, update it and then apply it without necessarily having to recompute the whole state. So what pwm-backlight is doing is expressly allowed (and in fact was one specific feature that we wanted to have in the atomic API). Similarly it's a valid use-case to do something like this: /* set duty cycle to 50% */ pwm_get_state(pwm, ); state.duty_cycle = state.period / 2; pwm_apply_state(pwm, ); which allows a consumer to do simple modifications without actually knowing what period has been configured. Some consumers just don't care about the period or don't even have a clue about what a good value would be (again, pwm-backlight would be one example). For some PWMs it may also not be possible to modify the period and if there's no explicit reason to do so, why should consumers be forced to even bother? All of that's out the window if we start taking shortcuts. If the PWM provider reads out the hardware state's PWM as "don't care", how is the consumer going to know what value to use? Yes, they can use things like pwm_get_args() to get the configuration from DT, but then what's the purpose of using states in the first place if consumers have to do all the tracking manually anyway? Thierry signature.asc Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote: > Hi everyone, > > happy new year, hope you are all well! > > On Thu, Dec 17, 2020 at 12:52:42PM -0500, Sven Van Asbroeck wrote: > > On Thu, Dec 17, 2020 at 12:43 PM Clemens Gruber > > wrote: > > > > > > > > Conclusion: .get_state() will always return "pwm disabled", so why do we > > > > bother reading out the h/w? > > > > > > If there are no plans for the PWM core to call .get_state more often in > > > the future, we could just read out the period and return 0 duty and > > > disabled. > > > > I'm not sure why we should even read out the period? > > When a channel is disabled, the period is not externally visible, > > therefore it's meaningless ? > > > > As far as I can tell, we can use this for .get_state(): > > memset(>state, 0, sizeof(pwm_state)); > > > > > > > > Thierry, Uwe, what's your take on this? > > I will continue working on this series in the upcoming weeks. > Feedback on the .get_state issue would be greatly appreciated. > > To summarize: > Is it OK for a driver to expect the chip->ops->get_state function to be > only called from the place in pwm core it is currently called from? > (Namely in pwm_device_request after chip->ops->request) > > If yes, we could always return a 0 duty cycle and disabled state, > because this is the state we left it in after .probe (and .free). > > However, if in the future, the pwm core adds additional calls to > chip->ops->get_state in other places, this could lead to problems. It's not safe in general to assume that this function will be called only at one specific point. If you implement the function, then it should do what it says (i.e. read the current hardware state), and not bother about when it might be called, or guess at the state that the PWM might be in. If you can't implement hardware readout, then that's fine (there are some devices for which no physical way exists to read out the current hardware state), but it doesn't sound like that's the problem here. > Another point is the period: Sven suggested we do not read out the > period at all, as the PWM is disabled anyway (see above). > Is this acceptable? No, if the PWM has separate bits for "enable" and "period", then they should be read separately. The hardware state isn't about representing what the currently configured output is, it's a representation of what the current settings of the PWM channel are. > And, if we never return anything but 0 in .get_state, should it be > implemented at all? Yes, not implementing .get_state() at all is better than lying. If you always return an all-zeros state, you're inevitably going to return the wrong result at some point in time. Thierry signature.asc Description: PGP signature
Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote: > On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber > wrote: > > > > I can initialize the values to 0 of course and check the file for other > > places with missing initializations. > > > > Or would it be better to check the return codes of regmap_read/write in > > such cases? I'm not sure. > > I think that checking the regmap_read/write return values is overkill > in this driver. These functions can't realistically fail, except if the i2c > bus is bad, i.e. h/w failure or intermittency. And that's an externality > which I believe we can ignore. I think there are (rare) occasions where it's fine to not check for errors, i.e. if you definitively know that calls can't fail. However, given that this uses regmap and you don't really know what's backing this, I think it's always better to err on the side of caution and properly check the return values. The fact that this can be externally caused is actually a reason why we shouldn't be ignoring any errors. If there's a chip that's hogging the I2C bus or if you've even just mistyped the I2C client's address in DT, it's better if the PWM driver tells you with an error message than if it is silently ignoring the errors and keeps you guessing at why the PWM isn't behaving the way it should. Granted, the error code isn't always going to pinpoint exactly what's going wrong, but for serious errors often the I2C bus driver will let you know with an extra error message. However, it's much easier to go looking for that error message if the PWM driver lets you know that something went wrong. Please just add full checking of regmap operations. Thierry signature.asc Description: PGP signature
Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
On Mon, Mar 15, 2021 at 01:36:31PM -0700, Nicolin Chen wrote:
> This patch dumps all active mapping entries from pagetable
> to a debugfs directory named "mappings".
>
> Attaching an example:
>
> SWGROUP: hc
> ASID: 0
> reg: 0x250
> PTB_ASID: 0xe0080004
> as->pd_dma: 0x80004000
> {
> [1023] 0xf008000b (1)
> {
> PTE RANGE | ATTR | PHYS | IOVA
> | SIZE
> [#1023, #1023] | 0x5 | 0x000111a8d000 |
> 0xf000 | 0x1000
> }
> }
> Total PDE count: 1
> Total PTE count: 1
>
> Tested-by: Dmitry Osipenko
> Reviewed-by: Dmitry Osipenko
> Signed-off-by: Nicolin Chen
> ---
>
> Changelog
> v5:
> * Fixed a typo in commit message
> * Splitted a long line into two lines
> * Rearranged variable defines by length
> * Added Tested-by and Reviewed-by from Dmitry
> v4: https://lkml.org/lkml/2021/3/14/429
> * Changed %d to %u for unsigned variables
> * Fixed print format mismatch warnings on ARM32
> v3: https://lkml.org/lkml/2021/3/14/30
> * Fixed PHYS and IOVA print formats
> * Changed variables to unsigned int type
> * Changed the table outputs to be compact
> v2: https://lkml.org/lkml/2021/3/9/1382
> * Expanded mutex range to the entire function
> * Added as->lock to protect pagetable walkthrough
> * Replaced devm_kzalloc with devm_kcalloc for group_debug
> * Added "PTE RANGE" and "SIZE" columns to group contiguous mappings
> * Dropped as->count check; added WARN_ON when as->count mismatches
> pd[pd_index]
> v1: https://lkml.org/lkml/2020/9/26/70
>
> drivers/iommu/tegra-smmu.c | 181 -
> 1 file changed, 176 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 97eb62f667d2..b728cae63314 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -19,6 +19,11 @@
> #include
> #include
>
> +struct tegra_smmu_group_debug {
> + const struct tegra_smmu_swgroup *group;
> + void *priv;
This always stores the address space, so why not make this:
struct tegra_smmu_as *as;
? While at it, perhaps throw in a const to make sure we don't modify
this structure in the debugfs code.
> +};
> +
> struct tegra_smmu_group {
> struct list_head list;
> struct tegra_smmu *smmu;
> @@ -47,6 +52,8 @@ struct tegra_smmu {
> struct dentry *debugfs;
>
> struct iommu_device iommu; /* IOMMU Core code handle */
> +
> + struct tegra_smmu_group_debug *group_debug;
> };
>
> struct tegra_smmu_as {
> @@ -152,6 +159,9 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu,
> unsigned long offset)
>
> #define SMMU_PDE_ATTR(SMMU_PDE_READABLE | SMMU_PDE_WRITABLE
> | \
>SMMU_PDE_NONSECURE)
> +#define SMMU_PTE_ATTR(SMMU_PTE_READABLE | SMMU_PTE_WRITABLE
> | \
> + SMMU_PTE_NONSECURE)
> +#define SMMU_PTE_ATTR_SHIFT (29)
No need for the parentheses here.
>
> static unsigned int iova_pd_index(unsigned long iova)
> {
> @@ -163,6 +173,12 @@ static unsigned int iova_pt_index(unsigned long iova)
> return (iova >> SMMU_PTE_SHIFT) & (SMMU_NUM_PTE - 1);
> }
>
> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int
> pt_index)
> +{
> + return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> +((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> +}
> +
> static bool smmu_dma_addr_valid(struct tegra_smmu *smmu, dma_addr_t addr)
> {
> addr >>= 12;
> @@ -334,7 +350,7 @@ static void tegra_smmu_domain_free(struct iommu_domain
> *domain)
> }
>
> static const struct tegra_smmu_swgroup *
> -tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
> +tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup, int
> *index)
> {
> const struct tegra_smmu_swgroup *group = NULL;
> unsigned int i;
> @@ -342,6 +358,8 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned
> int swgroup)
> for (i = 0; i < smmu->soc->num_swgroups; i++) {
> if (smmu->soc->swgroups[i].swgroup == swgroup) {
> group = >soc->swgroups[i];
> + if (index)
> + *index = i;
This doesn't look like the right place for this. And this also makes
things hard to follow because it passes out-of-band data in the index
parameter.
I'm thinking that this could benefit from a bit of refactoring where
we could for example embed struct tegra_smmu_group_debug into struct
tegra_smmu_group and then reference that when necessary instead of
carrying all that data in an orthogonal array. That should also make
it easier to track this.
Come to think of it, everything that's currently in your new struct
tegra_smmu_group_debug would be useful in struct tegra_smmu_group,
irrespective of debugfs support.
>
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
On Thu, Feb 18, 2021 at 02:07:02PM -0800, Nicolin Chen wrote:
> Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
> removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
> of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
> tegra_smmu_configure() that are typically done in the IOMMU core also.
>
> This approach works for both existing devices that have DT nodes and other
> devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
> upon testing. However, Page Fault errors are reported on tegra124-Nyan:
>
> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>EMEM address decode error (SMMU translation error [--S])
> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>Page fault (SMMU translation error [--S])
>
> After debugging, I found that the mentioned commit changed some function
> callback sequence of tegra-smmu's, resulting in enabling SMMU for display
> client before display driver gets initialized. I couldn't reproduce exact
> same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.
>
> Actually this Page Fault is a known issue, as on most of Tegra platforms,
> display gets enabled by the bootloader for the splash screen feature, so
> it keeps filling the framebuffer memory. A proper fix to this issue is to
> 1:1 linear map the framebuffer memory to IOVA space so the SMMU will have
> the same address as the physical address in its page table. Yet, Thierry
> has been working on the solution above for a year, and it hasn't merged.
>
> Therefore, let's partially revert the mentioned commit to fix the errors.
>
> The reason why we do a partial revert here is that we can still set priv
> in ->of_xlate() callback for PCI devices. Meanwhile, devices existing in
> DT, like display, will go through tegra_smmu_configure() at the stage of
> bus_set_iommu() when SMMU gets probed(), as what it did before we merged
> the mentioned commit.
>
> Once we have the linear map solution for framebuffer memory, this change
> can be cleaned away.
>
> [Big thank to Guillaume who reported and helped debugging/verification]
>
> Fixes: 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
> Reported-by: Guillaume Tucker
> Signed-off-by: Nicolin Chen
> ---
>
> Guillaume, would you please give a "Tested-by" to this change? Thanks!
>
> drivers/iommu/tegra-smmu.c | 72 +-
> 1 file changed, 71 insertions(+), 1 deletion(-)
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
[GIT PULL] pwm: Changes for v5.12-rc1
Hi Linus, The following changes since commit 6eefb79d6f5bc4086bd02c76f1072dd4a8d9d9f6: pwm: sun4i: Remove erroneous else branch (2020-12-17 14:23:49 +0100) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git tags/pwm/for-5.12-rc1 for you to fetch changes up to 9a9dd7e473517b68412fd2da3da8a4aeb4ecb38a: pwm: lpc18xx-sct: remove unneeded semicolon (2021-02-22 15:20:43 +0100) As I was generating the pull request I noticed that I forgot to fast- forward this to v5.11-rc1 after the last merge window. However, I did not think a last-minute rebase was appropriate. I did go back and ran my test builds on a rebase on top of v5.11-rc1 and everything checked out, so I think this is safe to merge. Also, linux-next would have caught any problems related to this. I'll make sure to properly roll forward the branch next time. Thanks, Thierry pwm: Changes for v5.12-rc1 The ZTE ZX platform is being removed, so the PWM driver is no longer needed and removed as well. Other than that this contains a small set of fixes and cleanups across a couple of drivers. Arnd Bergmann (1): pwm: Remove ZTE ZX driver Jeff LaBundy (1): pwm: iqs620a: Correct a stale state variable Simon South (5): pwm: rockchip: Enable APB clock during register access while probing pwm: rockchip: rockchip_pwm_probe(): Remove superfluous clk_unprepare() pwm: rockchip: Replace "bus clk" with "PWM clk" pwm: rockchip: Eliminate potential race condition when probing pwm: rockchip: Enable clock before calling clk_get_rate() Uwe Kleine-König (1): pwm: iqs620a: Fix overflow and optimize calculations Yang Li (1): pwm: lpc18xx-sct: remove unneeded semicolon Documentation/devicetree/bindings/pwm/pwm-zx.txt | 22 -- drivers/pwm/Kconfig | 10 - drivers/pwm/Makefile | 1 - drivers/pwm/pwm-iqs620a.c| 94 drivers/pwm/pwm-lpc18xx-sct.c| 2 +- drivers/pwm/pwm-rockchip.c | 32 ++- drivers/pwm/pwm-zx.c | 278 --- 7 files changed, 65 insertions(+), 374 deletions(-) delete mode 100644 Documentation/devicetree/bindings/pwm/pwm-zx.txt delete mode 100644 drivers/pwm/pwm-zx.c
Re: next/master bisection: baseline.login on r8a77960-ulcb
On Thu, Feb 25, 2021 at 11:14:57AM +, Robin Murphy wrote: > On 2021-02-25 11:09, Thierry Reding wrote: > > On Wed, Feb 24, 2021 at 10:39:42PM +0100, Heiko Thiery wrote: > > > Hi Christoph and all, > > > > > > On 23.02.21 10:56, Guillaume Tucker wrote: > > > > Hi Christoph, > > > > > > > > Please see the bisection report below about a boot failure on > > > > r8a77960-ulcb on next-20210222. > > > > > > > > Reports aren't automatically sent to the public while we're > > > > trialing new bisection features on kernelci.org but this one > > > > looks valid. > > > > > > > > The log shows a kernel panic, more details can be found here: > > > > > > > > https://kernelci.org/test/case/id/6034bde034504edc9faddd2c/ > > > > > > > > Please let us know if you need any help to debug the issue or try > > > > a fix on this platform. > > > > > > I am also seeing this problem on an iMX8MQ board and can help test if you > > > have a fix. > > > > This is also causing boot failures on Jetson AGX Xavier. The origin is > > slightly different from the above kernelci.org report, but the BUG_ON is > > the same: > > > > [2.650447] [ cut here ] > > [2.650588] kernel BUG at include/linux/iommu-helper.h:23! > > [2.650729] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP > > [2.654330] Modules linked in: > > [2.657474] CPU: 2 PID: 67 Comm: kworker/2:1 Not tainted > > 5.11.0-next-20210225-00025-gfd15609b3a81-dirty #120 > > [2.667367] Hardware name: NVIDIA Jetson AGX Xavier Developer Kit > > (DT) > > [2.674096] Workqueue: events deferred_probe_work_func > > [2.679169] pstate: 40400089 (nZcv daIf +PAN -UAO -TCO BTYPE=--) > > [2.684949] pc : find_slots.isra.0+0x118/0x2f0 > > [2.689494] lr : find_slots.isra.0+0x88/0x2f0 > > [2.693696] sp : 800011faf950 > > [2.697281] x29: 800011faf950 x28: 0001 > > [2.702537] x27: 0001 x26: > > [2.708131] x25: 0001 x24: 000105f03148 > > [2.713556] x23: 0001 x22: 800011559000 > > [2.718835] x21: 800011559a80 x20: edc0 > > [2.724493] x19: x18: 0020 > > [2.729770] x17: 0003ffd7d160 x16: 0068 > > [2.735173] x15: 80b43150 x14: > > [2.740944] x13: 82b5d791 x12: 0040 > > [2.746113] x11: a248 x10: > > [2.751882] x9 : x8 : 0003fed3 > > [2.757139] x7 : x6 : > > [2.762818] x5 : x4 : > > [2.767984] x3 : 0001e8303148 x2 : 8000 > > [2.773580] x1 : x0 : 001db800 > > [2.778662] Call trace: > > [2.781136] find_slots.isra.0+0x118/0x2f0 > > [2.785137] swiotlb_tbl_map_single+0x80/0x1b4 > > [2.789858] swiotlb_map+0x58/0x200 > > [2.793355] dma_direct_map_page+0x148/0x1c0 > > [2.797386] dma_map_page_attrs+0x2c/0x54 > > [2.801411] dw_pcie_host_init+0x40c/0x4c0 > > [2.805633] tegra_pcie_config_rp+0x7c/0x1f4 > > [2.810155] tegra_pcie_dw_probe+0x3d0/0x60c > > [2.814185] platform_probe+0x68/0xe0 > > [2.817688] really_probe+0xe4/0x4c0 > > [2.821362] driver_probe_device+0x58/0xc0 > > [2.825386] __device_attach_driver+0xa8/0x104 > > [2.829953] bus_for_each_drv+0x78/0xd0 > > [2.833434] __device_attach+0xdc/0x17c > > [2.837631] device_initial_probe+0x14/0x20 > > [2.841680] bus_probe_device+0x9c/0xa4 > > [2.845160] deferred_probe_work_func+0x74/0xb0 > > [2.849734] process_one_work+0x1cc/0x350 > > [2.853822] worker_thread+0x20c/0x3ac > > [2.858018] kthread+0x128/0x134 > > [2.860997] ret_from_fork+0x10/0x34 > > [2.864508] Code: ca180063 ea06007f 54fffee1 b50001e7 (d421) > > [2.870547] ---[ end trace e5c50bdcf12b316e ]--- > > [2.875087] note: kworker/2:1[67] exited with preempt_count 2 > > [2.880836] [ cut here ] > > > > I've confirmed that reverting the
Re: next/master bisection: baseline.login on r8a77960-ulcb
On Wed, Feb 24, 2021 at 10:39:42PM +0100, Heiko Thiery wrote:
> Hi Christoph and all,
>
> On 23.02.21 10:56, Guillaume Tucker wrote:
> > Hi Christoph,
> >
> > Please see the bisection report below about a boot failure on
> > r8a77960-ulcb on next-20210222.
> >
> > Reports aren't automatically sent to the public while we're
> > trialing new bisection features on kernelci.org but this one
> > looks valid.
> >
> > The log shows a kernel panic, more details can be found here:
> >
> >https://kernelci.org/test/case/id/6034bde034504edc9faddd2c/
> >
> > Please let us know if you need any help to debug the issue or try
> > a fix on this platform.
>
> I am also seeing this problem on an iMX8MQ board and can help test if you
> have a fix.
This is also causing boot failures on Jetson AGX Xavier. The origin is
slightly different from the above kernelci.org report, but the BUG_ON is
the same:
[2.650447] [ cut here ]
[2.650588] kernel BUG at include/linux/iommu-helper.h:23!
[2.650729] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[2.654330] Modules linked in:
[2.657474] CPU: 2 PID: 67 Comm: kworker/2:1 Not tainted
5.11.0-next-20210225-00025-gfd15609b3a81-dirty #120
[2.667367] Hardware name: NVIDIA Jetson AGX Xavier Developer Kit (DT)
[2.674096] Workqueue: events deferred_probe_work_func
[2.679169] pstate: 40400089 (nZcv daIf +PAN -UAO -TCO BTYPE=--)
[2.684949] pc : find_slots.isra.0+0x118/0x2f0
[2.689494] lr : find_slots.isra.0+0x88/0x2f0
[2.693696] sp : 800011faf950
[2.697281] x29: 800011faf950 x28: 0001
[2.702537] x27: 0001 x26:
[2.708131] x25: 0001 x24: 000105f03148
[2.713556] x23: 0001 x22: 800011559000
[2.718835] x21: 800011559a80 x20: edc0
[2.724493] x19: x18: 0020
[2.729770] x17: 0003ffd7d160 x16: 0068
[2.735173] x15: 80b43150 x14:
[2.740944] x13: 82b5d791 x12: 0040
[2.746113] x11: a248 x10:
[2.751882] x9 : x8 : 0003fed3
[2.757139] x7 : x6 :
[2.762818] x5 : x4 :
[2.767984] x3 : 0001e8303148 x2 : 8000
[2.773580] x1 : x0 : 001db800
[2.778662] Call trace:
[2.781136] find_slots.isra.0+0x118/0x2f0
[2.785137] swiotlb_tbl_map_single+0x80/0x1b4
[2.789858] swiotlb_map+0x58/0x200
[2.793355] dma_direct_map_page+0x148/0x1c0
[2.797386] dma_map_page_attrs+0x2c/0x54
[2.801411] dw_pcie_host_init+0x40c/0x4c0
[2.805633] tegra_pcie_config_rp+0x7c/0x1f4
[2.810155] tegra_pcie_dw_probe+0x3d0/0x60c
[2.814185] platform_probe+0x68/0xe0
[2.817688] really_probe+0xe4/0x4c0
[2.821362] driver_probe_device+0x58/0xc0
[2.825386] __device_attach_driver+0xa8/0x104
[2.829953] bus_for_each_drv+0x78/0xd0
[2.833434] __device_attach+0xdc/0x17c
[2.837631] device_initial_probe+0x14/0x20
[2.841680] bus_probe_device+0x9c/0xa4
[2.845160] deferred_probe_work_func+0x74/0xb0
[2.849734] process_one_work+0x1cc/0x350
[2.853822] worker_thread+0x20c/0x3ac
[2.858018] kthread+0x128/0x134
[2.860997] ret_from_fork+0x10/0x34
[2.864508] Code: ca180063 ea06007f 54fffee1 b50001e7 (d421)
[2.870547] ---[ end trace e5c50bdcf12b316e ]---
[2.875087] note: kworker/2:1[67] exited with preempt_count 2
[2.880836] [ cut here ]
I've confirmed that reverting the following commits makes the system
boot again:
47cfc5be1934 ("swiotlb: Validate bounce size in the sync/unmap path")
c6f50c7719e7 ("swiotlb: respect min_align_mask")
e952d9a1bc20 ("swiotlb: don't modify orig_addr in swiotlb_tbl_sync_single")
567d877f9a7d ("swiotlb: refactor swiotlb_tbl_map_single")
Let me know if I can help test any fixes for this.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v2 1/3] pwm: mtk_disp: clear the clock operations
On Sat, Jan 30, 2021 at 10:12:24PM +0800, Jitao Shi wrote: > Remove the clk_prepare from mtk_disp_pwm_probe. > Remove the clk_unprepare from mtk_disp_pwm_remove. > > Signed-off-by: Jitao Shi > --- > drivers/pwm/pwm-mtk-disp.c | 23 ++- > 1 file changed, 2 insertions(+), 21 deletions(-) It's not clear *why* you're doing this change. It's already obvious from the changes in this patch that you're removing the calls to clk_prepare() and clk_unprepare(), so instead of duplicating that information in the commit message, take this opportunity to describe why this change is needed. Without any further context, this would seem to just break operation of this chip because now these clocks are never enabled in the first place. Thierry signature.asc Description: PGP signature
Re: [PATCH] tty: serial: Add earlycon driver for Tegra Combined UART
On Mon, Feb 15, 2021 at 12:35:31PM +0200, Mikko Perttunen wrote: > On 2/15/21 12:25 PM, Thierry Reding wrote: > > On Sat, Feb 13, 2021 at 01:58:24PM +0200, Mikko Perttunen wrote: > > > Add an earlycon driver for platforms with TCU, namely Tegra194. > > > The driver is compatible with boot parameters passed by NVIDIA > > > boot chains. > > > > I'm not sure I understand the latter part of this description. What boot > > parameters is this compatible with? Looking at the setup function there > > doesn't seem to be anything out of the ordinary here, so I'm wondering > > if that's just confusing. If there's anything special, it might be worth > > specifically pointing out what that is. Perhaps both in the commit > > message and in a code comment, so it's properly documented. > > It's that the name of the driver 'tegra_comb_uart' matches what the boot > chain passes; and that OF_EARLYCON_DECLARE is not used. (OF_EARLYCON_DECLARE > cannot anyway be used due to the mailbox indirection in device tree). This is all not immediately obvious. Perhaps you can add more of this into the commit message and perhaps provide an example of how this would be used on the kernel command-line. You say "mailbox indirection" and looking at the implementation this does seem to use the mailbox's base address as a sort of TX FIFO, which I think is all good. However, I'm wondering if we couldn't somehow detect this all dynamically at runtime. Don't we have access to the device tree node at this point? If so, couldn't we parse all the necessary information from the DT instead of relying on the user providing the mailbox address on the command-line? I realize that this would all make things a bit more complicated in this driver, but at the same time it'd make life so much easier for users, so I think it's worth at least considering. To elaborate on this a bit, I think it'd be much more useful if users could specify something like this: earlycon=tegra-tcu rather than: earlycon=tegra_comb_uart,0xc15 Note that I'm not even sure if that's a correct address. It'd be even better if all of this can just be derived from the device tree. My recollection is that earlycon always needs to be explicitly enabled, but I thought it was also possible to derive which console to use from the /chose/stdout-path property in device tree. > > > Signed-off-by: Mikko Perttunen > > > --- > > > drivers/tty/serial/Kconfig | 12 + > > > drivers/tty/serial/Makefile | 1 + > > > drivers/tty/serial/tegra-tcu-earlycon.c | 72 + > > > 3 files changed, 85 insertions(+) > > > create mode 100644 drivers/tty/serial/tegra-tcu-earlycon.c > > > > > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > > > index 34a2899e69c0..d941785e3f46 100644 > > > --- a/drivers/tty/serial/Kconfig > > > +++ b/drivers/tty/serial/Kconfig > > > @@ -331,6 +331,18 @@ config SERIAL_TEGRA_TCU_CONSOLE > > > If unsure, say Y. > > > +config SERIAL_TEGRA_TCU_EARLYCON > > > + bool "Earlycon on NVIDIA Tegra Combined UART" > > > + depends on ARCH_TEGRA || COMPILE_TEST > > > + select SERIAL_EARLYCON > > > + select SERIAL_CORE_CONSOLE > > > + default y if SERIAL_TEGRA_TCU_CONSOLE > > > + help > > > + If you say Y here, TCU output will be supported during the earlycon > > > + phase of the boot. > > > + > > > + If unsure, say Y. > > > + > > > config SERIAL_MAX3100 > > > tristate "MAX3100 support" > > > depends on SPI > > > diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile > > > index b85d53f9e9ff..408144326fed 100644 > > > --- a/drivers/tty/serial/Makefile > > > +++ b/drivers/tty/serial/Makefile > > > @@ -72,6 +72,7 @@ obj-$(CONFIG_SERIAL_XILINX_PS_UART) += xilinx_uartps.o > > > obj-$(CONFIG_SERIAL_SIRFSOC) += sirfsoc_uart.o > > > obj-$(CONFIG_SERIAL_TEGRA) += serial-tegra.o > > > obj-$(CONFIG_SERIAL_TEGRA_TCU) += tegra-tcu.o > > > +obj-$(CONFIG_SERIAL_TEGRA_TCU_EARLYCON) += tegra-tcu-earlycon.o > > > obj-$(CONFIG_SERIAL_AR933X) += ar933x_uart.o > > > obj-$(CONFIG_SERIAL_EFM32_UART) += efm32-uart.o > > > obj-$(CONFIG_SERIAL_ARC)+= arc_uart.o > > > diff --git a/drivers/tty/serial/tegra-tcu-earlycon.c > > > b/drivers/tty/serial/tegra-tcu-earlycon.c > > > new file mode 100644 > > > index ..9decfbced0a7 > > > --- /dev/null > > > +++ b/drivers/tty/serial/tegra-tcu-earlycon.c &
Re: [PATCH] mailbox: tegra-hsp: Set lockdep class dynamically
On Wed, Feb 10, 2021 at 03:49:45PM +0200, Mikko Perttunen wrote: > On Tegra194, due to both BPMP and TCU using mailboxes, we get a > lockdep spew at boot. Both are using different instances of HSP, > so this is harmless. As such give each HSP instance a different > lockdep class. > > Signed-off-by: Mikko Perttunen > --- > drivers/mailbox/tegra-hsp.c | 15 +++ > 1 file changed, 15 insertions(+) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH -next] staging: nvec: minor coding style fix
On Fri, Feb 12, 2021 at 10:34:23AM +0300, Fatih Yildirim wrote: > Fix for the below coding style warning. > Warning: Move const after static - use 'static const int' > > Signed-off-by: Fatih Yildirim > --- > drivers/staging/nvec/nvec_power.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH] tty: serial: Add earlycon driver for Tegra Combined UART
On Sat, Feb 13, 2021 at 01:58:24PM +0200, Mikko Perttunen wrote:
> Add an earlycon driver for platforms with TCU, namely Tegra194.
> The driver is compatible with boot parameters passed by NVIDIA
> boot chains.
I'm not sure I understand the latter part of this description. What boot
parameters is this compatible with? Looking at the setup function there
doesn't seem to be anything out of the ordinary here, so I'm wondering
if that's just confusing. If there's anything special, it might be worth
specifically pointing out what that is. Perhaps both in the commit
message and in a code comment, so it's properly documented.
>
> Signed-off-by: Mikko Perttunen
> ---
> drivers/tty/serial/Kconfig | 12 +
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/tegra-tcu-earlycon.c | 72 +
> 3 files changed, 85 insertions(+)
> create mode 100644 drivers/tty/serial/tegra-tcu-earlycon.c
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 34a2899e69c0..d941785e3f46 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -331,6 +331,18 @@ config SERIAL_TEGRA_TCU_CONSOLE
>
> If unsure, say Y.
>
> +config SERIAL_TEGRA_TCU_EARLYCON
> + bool "Earlycon on NVIDIA Tegra Combined UART"
> + depends on ARCH_TEGRA || COMPILE_TEST
> + select SERIAL_EARLYCON
> + select SERIAL_CORE_CONSOLE
> + default y if SERIAL_TEGRA_TCU_CONSOLE
> + help
> + If you say Y here, TCU output will be supported during the earlycon
> + phase of the boot.
> +
> + If unsure, say Y.
> +
> config SERIAL_MAX3100
> tristate "MAX3100 support"
> depends on SPI
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index b85d53f9e9ff..408144326fed 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -72,6 +72,7 @@ obj-$(CONFIG_SERIAL_XILINX_PS_UART) += xilinx_uartps.o
> obj-$(CONFIG_SERIAL_SIRFSOC) += sirfsoc_uart.o
> obj-$(CONFIG_SERIAL_TEGRA) += serial-tegra.o
> obj-$(CONFIG_SERIAL_TEGRA_TCU) += tegra-tcu.o
> +obj-$(CONFIG_SERIAL_TEGRA_TCU_EARLYCON) += tegra-tcu-earlycon.o
> obj-$(CONFIG_SERIAL_AR933X) += ar933x_uart.o
> obj-$(CONFIG_SERIAL_EFM32_UART) += efm32-uart.o
> obj-$(CONFIG_SERIAL_ARC) += arc_uart.o
> diff --git a/drivers/tty/serial/tegra-tcu-earlycon.c
> b/drivers/tty/serial/tegra-tcu-earlycon.c
> new file mode 100644
> index ..9decfbced0a7
> --- /dev/null
> +++ b/drivers/tty/serial/tegra-tcu-earlycon.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2017-2021, NVIDIA CORPORATION. All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +
> +#define NUM_BYTES_FIELD_BIT 24
> +#define FLUSH_BIT26
This one seems to be unused.
> +#define INTR_TRIGGER_BIT 31
I wonder if this could somehow be integrated with the existing TCU
driver since we have these bits defined there already. And really this
is basically a skeleton version of the same driver.
> +/*
> + * This function splits the string to be printed (const char *s) into
> multiple
> + * packets. Each packet contains a max of 3 characters. Packets are sent to
> the
> + * SPE-based combined UART server for printing. Communication with SPE is
> done
> + * through mailbox registers which can generate interrupts for SPE.
> + */
> +static void early_tcu_write(struct console *console, const char *s, unsigned
> int count)
> +{
> + struct earlycon_device *device = console->data;
> + u8 __iomem *addr = device->port.membase;
> + u32 mbox_val = BIT(INTR_TRIGGER_BIT);
> + unsigned int i;
> +
> + /* Loop for processing each 3 char packet */
> + for (i = 0; i < count; i++) {
> + if (s[i] == '\n')
> + mbox_val = update_and_send_mbox(addr, mbox_val, '\r');
> + mbox_val = update_and_send_mbox(addr, mbox_val, s[i]);
> + }
> +
> + if ((mbox_val >> NUM_BYTES_FIELD_BIT) & 0x3) {
> + while (readl(addr) & BIT(INTR_TRIGGER_BIT))
> + cpu_relax();
> + writel(mbox_val, addr);
> + }
> +}
For example this function already exists in the Tegra TCU driver and
perhaps some of that could be refactored to work for both cases.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v7 00/14] Tegra XHCI controller ELPG support
On Wed, Jan 20, 2021 at 03:34:00PM +0800, JC Kuo wrote: > Tegra XHCI controler can be placed in ELPG (Engine Level PowerGated) > state for power saving when all of the connected USB devices are in > suspended state. This patch series includes clk, phy and pmc changes > that are required for properly place controller in ELPG and bring > controller out of ELPG. > > JC Kuo (14): > clk: tegra: Add PLLE HW power sequencer control > clk: tegra: Don't enable PLLE HW sequencer at init > phy: tegra: xusb: Move usb3 port init for Tegra210 > phy: tegra: xusb: Rearrange UPHY init on Tegra210 > phy: tegra: xusb: Add Tegra210 lane_iddq operation > phy: tegra: xusb: Add sleepwalk and suspend/resume > soc/tegra: pmc: Provide USB sleepwalk register map > arm64: tegra210: XUSB PADCTL add "nvidia,pmc" prop > dt-bindings: phy: tegra-xusb: Add nvidia,pmc prop > phy: tegra: xusb: Add wake/sleepwalk for Tegra210 > phy: tegra: xusb: Tegra210 host mode VBUS control > phy: tegra: xusb: Add wake/sleepwalk for Tegra186 > usb: host: xhci-tegra: Unlink power domain devices > xhci: tegra: Enable ELPG for runtime/system PM > > .../phy/nvidia,tegra124-xusb-padctl.txt |1 + > arch/arm64/boot/dts/nvidia/tegra210.dtsi |1 + > drivers/clk/tegra/clk-pll.c | 12 - > drivers/clk/tegra/clk-tegra210.c | 53 +- > drivers/phy/tegra/xusb-tegra186.c | 558 - > drivers/phy/tegra/xusb-tegra210.c | 1889 + > drivers/phy/tegra/xusb.c | 92 +- > drivers/phy/tegra/xusb.h | 22 +- > drivers/soc/tegra/pmc.c | 94 + > drivers/usb/host/xhci-tegra.c | 613 -- > include/linux/clk/tegra.h |4 +- > include/linux/phy/tegra/xusb.h| 10 +- > 12 files changed, 2784 insertions(+), 565 deletions(-) > > v5 "phy: tegra: xusb: tegra210: Do not reset UPHY PLL" is moved > into v6 "phy: tegra: xusb: Rearrange UPHY init on Tegra210" Mike, Stephen, could you guys take a look at the two clk patches here and give an Acked-by? There's build-time dependencies throughout the series, so it'd be good if they can all go through either the PHY or USB trees. Kishon, Greg, any comments on these patches? Unfortunately, the USB patches in this series have a build-time dependency on the PHY patches, so this should all go through one tree. Since this all culminates in the XHCI driver, merging this through the USB tree might be best, provided that Kishon provides his Acked-by on the PHY patches. Alternatively, I can create a set of branches with the correct dependencies and send out pull requests for the three subsystems if that's preferrable. Let me know how you want to handle these. Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH v2] pwm: sunxi: Add Allwinner SoC PWM controller driver
On Thu, Feb 04, 2021 at 02:54:36PM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Thu, Feb 04, 2021 at 02:38:40PM +0100, Thierry Reding wrote: > > All I'm saying is that I don't think we need to require everyone to > > adopt a prefix, especially if this hasn't been followed consistently, > > because then we don't actually gain anything. > > Is "we didn't do this consistently in the past" a reason to never > improve here? I hope it's not ... You're implying that consistently using a prefix is an improvement. I don't agree, so I don't see a need to introduce any new rules for this. Thierry signature.asc Description: PGP signature
Re: [PATCH v2] pwm: sunxi: Add Allwinner SoC PWM controller driver
On Wed, Feb 03, 2021 at 09:59:13PM +0100, Uwe Kleine-König wrote:
> Hello Thierry, hello Ban,
>
> On Wed, Feb 03, 2021 at 05:33:16PM +0100, Thierry Reding wrote:
> > On Wed, Feb 03, 2021 at 04:12:00PM +0100, Uwe Kleine-König wrote:
> > > On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> > [...]
> > > > +#define PWM_GET_CLK_OFFSET(chan) (0x20 + ((chan >> 1) * 0x4))
> > > > +#define PWM_CLK_APB_SCRBIT(7)
> > > > +#define PWM_DIV_M 0
> > > > +#define PWM_DIV_M_WIDTH0x4
> > > > +
> > > > +#define PWM_CLK_REG0x40
> > > > +#define PWM_CLK_GATING BIT(0)
> > > > +
> > > > +#define PWM_ENABLE_REG 0x80
> > > > +#define PWM_EN BIT(0)
> > > > +
> > > > +#define PWM_CTL_REG(chan) (0x100 + 0x20 * chan)
> > > > +#define PWM_ACT_STABIT(8)
> > > > +#define PWM_PRESCAL_K 0
> > > > +#define PWM_PRESCAL_K_WIDTH0x8
> > > > +
> > > > +#define PWM_PERIOD_REG(chan) (0x104 + 0x20 * chan)
> > > > +#define PWM_ENTIRE_CYCLE 16
> > > > +#define PWM_ENTIRE_CYCLE_WIDTH 0x10
> > > > +#define PWM_ACT_CYCLE 0
> > > > +#define PWM_ACT_CYCLE_WIDTH0x10
> > >
> > > Please use a driver specific prefix for these defines.
> >
> > These are nice and to the point, so I think it'd be fine to leave these
> > as-is. Unless of course if they conflict with something from the PWM
> > core, which I don't think any of these do.
>
> In my eyes PWM_CLK_REG, PWM_CLK_GATING, PWM_ENABLE_REG and PWM_EN are
> not so nice. pwm-sun4i.c has already PWM_EN, pwm-mtk-disp.c has
> DISP_PWM_EN, pwm-zx.c has ZX_PWM_EN, pwm-berlin.c has BERLIN_PWM_EN.
> Luckily most of them already use a prefix. So it doesn't conflict with
> the core, but might with other drivers.
"Conflicts with another driver" is not something that makes sense. By
definition drivers should be specific to a given device, so you better
make sure there's no namespace sharing between them.
I'm fine if somebody wants to use a prefix, but I'm also fine if they
don't use a prefix, provided that there are no conflicts, which should
be immediately obvious because it typically causes build errors.
All I'm saying is that I don't think we need to require everyone to
adopt a prefix, especially if this hasn't been followed consistently,
because then we don't actually gain anything.
> And also if you only care about
> conflicts with the core, using a prefix is the future-proof strategy.
It's not like these symbol names are carved in stone. If we ever
introduce a symbol in the PWM core that happens to conflicts with one or
more drivers, we can always fix the drivers at that point.
> For tools like ctags and cscope a unique name is great, too.
>
> Also comparing
>
> tmp |= BIT_CH(PWM_EN, pwm->hwpwm);
>
> with (say)
>
> tmp |= BIT_CH(SUN5I_PWM_PWM_EN, pwm->hwpwm);
>
> I prefer the latter as it is obvious that this is a driver specific
> constant.
I think the context makes it plenty clear that this is driver specific,
so the prefix is a bit redundant.
> Having said that, I'm also a fan of having the register name in the bit
> field define to simplify spotting errors like 4b75f8000361 ("serial:
> imx: Fix DCD reading").
>
> So looking at:
>
> + /* disable pwm controller */
> + tmp = sun50i_pwm_readl(sun50i_pwm, PWM_ENABLE_REG);
> + tmp &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> + sun50i_pwm_writel(sun50i_pwm, tmp, PWM_ENABLE_REG);
>
> my preferred version would be instead:
>
> + /* disable pwm controller */
> + enable = sun50i_pwm_readl(sun50i_pwm, SUN50I_REG_PWM_ENABLE);
> + enable &= ~SUN50I_REG_PWM_ENABLE_EN(pwm->hwpwm);
> + sun50i_pwm_writel(sun50i_pwm, enable, SUN50I_REG_PWM_ENABLE);
>
> where variable name, register name and bitfield name are obviously
> matching.
I don't have any particular objection to the above, but I've also seen
this kind of naming result in ridiculously long (as in almost impossible
to read) lines, so I think we need to strike the right balance. Given
that there aren't any rigorous rules for this, I don't think it's worth
requiring everyone to adopt some style that's not consistently followed
anyway.
> > > > +struct sun50i_pwm_data {
> > > > + unsigned int npwm;
> > > > +};
>
Re: [PATCH v2] pwm: sunxi: Add Allwinner SoC PWM controller driver
On Wed, Feb 03, 2021 at 04:12:00PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
[...]
> > +#define PWM_GET_CLK_OFFSET(chan) (0x20 + ((chan >> 1) * 0x4))
> > +#define PWM_CLK_APB_SCRBIT(7)
> > +#define PWM_DIV_M 0
> > +#define PWM_DIV_M_WIDTH0x4
> > +
> > +#define PWM_CLK_REG0x40
> > +#define PWM_CLK_GATING BIT(0)
> > +
> > +#define PWM_ENABLE_REG 0x80
> > +#define PWM_EN BIT(0)
> > +
> > +#define PWM_CTL_REG(chan) (0x100 + 0x20 * chan)
> > +#define PWM_ACT_STABIT(8)
> > +#define PWM_PRESCAL_K 0
> > +#define PWM_PRESCAL_K_WIDTH0x8
> > +
> > +#define PWM_PERIOD_REG(chan) (0x104 + 0x20 * chan)
> > +#define PWM_ENTIRE_CYCLE 16
> > +#define PWM_ENTIRE_CYCLE_WIDTH 0x10
> > +#define PWM_ACT_CYCLE 0
> > +#define PWM_ACT_CYCLE_WIDTH0x10
>
> Please use a driver specific prefix for these defines.
These are nice and to the point, so I think it'd be fine to leave these
as-is. Unless of course if they conflict with something from the PWM
core, which I don't think any of these do.
> > +struct sun50i_pwm_data {
> > + unsigned int npwm;
> > +};
> > +
> > +struct sun50i_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + struct reset_control *rst_clk;
> > + void __iomem *base;
> > + const struct sun50i_pwm_data *data;
> > +};
> > +
> > +static inline struct sun50i_pwm_chip *sun50i_pwm_from_chip(struct pwm_chip
> > *chip)
> > +{
> > + return container_of(chip, struct sun50i_pwm_chip, chip);
> > +}
I wanted to reply to Uwe's comment on v1 but you sent this before I got
around to it, so I'll mention it here. I recognize the usefulness of
having a common prefix for function names, though admittedly it's not
totally necessary in these drivers because these are all local symbols.
But it does makes things a bit consistent and helps when looking at
backtraces and such, so that's all good.
That said, I consider these casting helpers a bit of a special case
because they usually won't show up in backtraces, or anywhere else for
that matter. Traditionally these have been named to_*(), so it'd be
entirely consistent to keep doing that.
But I don't have any strong objections to this variant either, so pick
whichever you want. Although, please, nobody take that as a hint that
any existing helpers should be converted.
[...]
> > +static int sun50i_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct sun50i_pwm_chip *pwm;
>
> "pwm" isn't a good name, in PWM code this name is usually used for
> struct pwm_device pointers (and sometimes the global pwm id). I usually
> use "ddata" for driver data (and would have called "sun50i_pwm_chip"
> "sun50i_pwm_ddata" instead). Another common name is "priv".
This driver already declares sun50i_pwm_data for per-SoC data, so having
a struct sun50i_pwm_ddata would just be confusing. sun50i_pwm_chip is
consistent with what other drivers name this, so I think that's fine.
Agreed on "pwm" being a bad choice here, though.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v5 1/9] dt-bindings: clock: tegra: Add clock ID TEGRA210_CLK_QSPI_PM
On Mon, Dec 21, 2020 at 01:17:31PM -0800, Sowjanya Komatineni wrote: > Tegra210 QSPI clock output has divider DIV2_SEL which will be enabled > when using DDR interface mode. > > This patch adds clock ID for this to dt-binding. > > Acked-by: Rob Herring > Signed-off-by: Sowjanya Komatineni > --- > include/dt-bindings/clock/tegra210-car.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Hi Mark, It looks like you applied this patch along with the driver patches. Unfortunately, if I apply the DT updates without this patch, the DT files will fail to build because this symbol is missing. Since the TEGRA210_CLK_QSPI_PM symbol isn't used by the driver patches directly, would you mind dropping this so that I can pick it up into the Tegra tree along with the DT updates? I realize this is completely unobvious, so sorry for not noticing and bringing this up earlier. Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH] arm64: tegra: Enable Jetson-Xavier J512 USB host
On Tue, Jan 19, 2021 at 10:23:49AM +0800, JC Kuo wrote: > This commit enables USB host mode at J512 type-C port of Jetson-Xavier. > > Signed-off-by: JC Kuo > --- > .../arm64/boot/dts/nvidia/tegra194-p2888.dtsi | 8 +++ > .../boot/dts/nvidia/tegra194-p2972-.dts | 24 +-- > 2 files changed, 30 insertions(+), 2 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [RESEND PATCH v6 5/6] arm64: tegra: Audio graph header for Tegra210
On Tue, Jan 19, 2021 at 02:58:15PM +0530, Sameer Pujar wrote: > Expose a header which describes DT bindings required to use audio-graph > based sound card. All Tegra210 based platforms can include this header > and add platform specific information. Currently, from SoC point of view, > all links are exposed for ADMAIF, AHUB, I2S and DMIC components. > > Signed-off-by: Sameer Pujar > Reviewed-by: Jon Hunter > --- > .../boot/dts/nvidia/tegra210-audio-graph.dtsi | 153 > + > 1 file changed, 153 insertions(+) > create mode 100644 arch/arm64/boot/dts/nvidia/tegra210-audio-graph.dtsi Applied, thanks. Thierry signature.asc Description: PGP signature
