linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Maxime Ripard <mripard@kernel.org>
Cc: "Rob Herring" <robh+dt@kernel.org>,
	"Frank Rowand" <frowand.list@gmail.com>,
	linux-sunxi@googlegroups.com, "Chen-Yu Tsai" <wens@csie.org>,
	"Jernej Škrabec" <jernej.skrabec@siol.net>,
	"Mark Rutland" <mark.rutland@arm.com>,
	linux-pwm@vger.kernel.org,
	devicetree <devicetree@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	kernel@pengutronix.de,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
Date: Wed, 31 Jul 2019 08:52:30 +0200	[thread overview]
Message-ID: <20190731065230.mqbtn5sfoxrkevw5@pengutronix.de> (raw)
In-Reply-To: <20190730170601.a7ei43wku6jsjanu@flea>

On Tue, Jul 30, 2019 at 07:06:01PM +0200, Maxime Ripard wrote:
> On Tue, Jul 30, 2019 at 10:09:00AM +0200, Uwe Kleine-König wrote:
> > Hello Rob and Frank,
> >
> > Maxime and Jernej on one side and me on the other cannot agree about a
> > detail in the change to the bindings here. I'm trying to objectively
> > summarize the situation for you to help deciding what is the right thing
> > to do here.
> >
> > TLDR: The sun4i pwm driver is extended to support a new variant of that
> > device on the H6 SoC. Compared to the earlier supported variants
> > allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an
> > additional clock.
> >
> > The two positions are:
> >
> >  - We need a new compatible because only then the driver and/or the dt
> >    schema checker can check that each "allwinner,sun50i-h6-pwm" device
> >    has a reset property and a "bus" clock; and the earlier variants
> >    don't.
> 
> There is two topics here, really. The binding itself really must have
> those properties as required.
> 
> You had an analogy before that we shouldn't really do that, since it
> would be verification and that it would be similar to checking whether
> the register range was right. This analogy isn't correct, a better one
> would be checking that the register range exists in the first place.

The relevant difference is that *all* devices supported by the driver
have to have a register range. Compared to that only a subset of the
devices have to have a bus clock.

> Indeed, if you don't have a register range, you have no register to
> write to, and that's a showstopper for any driver. I'm pretty sure we
> all agree on that. But on those SoCs, like Chen-Yu said, having no
> resets or clocks properties result in an equally bad result: either
> any write to that device is completely ignored (missing reset), or the
> system completely (and silently) locks up (missing bus clock).
> 
> We *have* to catch that somehow and not let anything like that happen.

IIUC both the clock and the reset stuff is SoC specific, so it's the
same for all machines with the H6, right? So assuming this is correctly
contained in the h6.dtsi, in which cases can this go wrong? I only see
the cases that the dts author includes the wrong dtsi or overrides
stuff. In the first case a non-working PWM is probably one of the
smaller problems and the second is something we're not really able to
catch.

But even if each machine's dts author has to get this right, I don't
think the dts schema is the right place to assert this.

> That being said, we can't really validate that the clock pointed is
> the right one, just like we can't really check that the register range
> is the proper one. Or rather, we could, but it's completely
> impractical and we do agree on that as well.
> 
> Having the bus clock and reset line being required however is only a
> few lines in the binding though, and is very practical.
> 
> >  - The driver can be simpler and the device specific knowledge is only
> >    in a single place (the dt) if the device tree is considered valid and
> >    a reset property and "bus" clock is used iff it's provided in the
> >    device tree without additional comparison for the compatible.
> 
> And now we have the discussion on how it's implemented in a
> driver. Since it's pretty cheap to implement (only a couple of lines:
> two for the if block, one for the additional field in the structure,
> one for each SoC using that) and have huge benefits (not silently
> locking up the system at boot), then I'd say we should go for it.

Right, it's only a few lines. Still it (IMHO needlessly) complicates the
driver. From the driver's POV the device tree defines the
characteristics of the device and if the dts defines an h6-pwm without a
bus clock then maybe this is the PWM on the next generation SoC that
doesn't need it. And maybe you're happy in a few year's time when you
don't have to touch the driver again for this next generation SoC
because the driver is not only simpler but also flexible enough to
handle the new PWM without adaptions.

All in all I don't care much about the dt schema stuff, I want to keep
the driver simple. So if we agree that the schema ensures that the h6
pwms have a reset and a bus clock and we just use reset_get_optional and
clk_get_optional that's a compromise I can agree to.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

  reply	other threads:[~2019-07-31  6:52 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-26 18:40 [PATCH 0/6] pwm: sun4i: Add support for Allwinner H6 Jernej Skrabec
2019-07-26 18:40 ` [PATCH 1/6] dt-bindings: pwm: allwinner: Add H6 PWM description Jernej Skrabec
2019-07-27 10:42   ` Maxime Ripard
2019-07-26 18:40 ` [PATCH 2/6] pwm: sun4i: Add a quirk for reset line Jernej Skrabec
2019-07-27 10:42   ` Maxime Ripard
2019-07-29  6:36   ` Uwe Kleine-König
2019-07-29  6:43     ` Chen-Yu Tsai
2019-07-29  7:12       ` Uwe Kleine-König
2019-07-29 10:18         ` Philipp Zabel
2019-07-29 16:37         ` Maxime Ripard
2019-07-29 18:20           ` Uwe Kleine-König
2019-07-26 18:40 ` [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock Jernej Skrabec
2019-07-27 10:46   ` Maxime Ripard
2019-07-27 14:15     ` Jernej Škrabec
2019-07-27 14:27     ` Chen-Yu Tsai
2019-07-29  6:38   ` Uwe Kleine-König
2019-07-29 15:48     ` Jernej Škrabec
2019-07-29 16:14       ` Uwe Kleine-König
2019-07-29 16:45         ` Maxime Ripard
2019-07-29 19:04           ` Uwe Kleine-König
2019-07-26 18:40 ` [PATCH 4/6] pwm: sun4i: Add support for H6 PWM Jernej Skrabec
2019-07-29  6:40   ` Uwe Kleine-König
2019-07-29 15:55     ` Jernej Škrabec
2019-07-29 16:07       ` Uwe Kleine-König
2019-07-29 16:09         ` [linux-sunxi] " Chen-Yu Tsai
2019-07-29 16:24           ` Uwe Kleine-König
2019-07-29 16:40             ` Jernej Škrabec
2019-07-29 18:40               ` Uwe Kleine-König
2019-07-29 18:46                 ` Jernej Škrabec
2019-07-29 18:51                   ` Uwe Kleine-König
2019-07-29 22:04                     ` Jernej Škrabec
2019-07-30  8:09                       ` Uwe Kleine-König
2019-07-30  8:32                         ` Chen-Yu Tsai
2019-07-30 17:06                         ` Maxime Ripard
2019-07-31  6:52                           ` Uwe Kleine-König [this message]
2019-08-12  9:56                             ` Maxime Ripard
2019-08-12 10:47                               ` Uwe Kleine-König
2019-08-12 10:51                                 ` Jernej Škrabec
2019-07-26 18:40 ` [PATCH 5/6] pwm: sun4i: Add support to output source clock directly Jernej Skrabec
2019-07-27 10:50   ` Maxime Ripard
2019-07-27 14:28     ` Jernej Škrabec
2019-07-27 14:54       ` [linux-sunxi] " Chen-Yu Tsai
2019-07-29  7:06   ` Uwe Kleine-König
2019-07-29 16:16     ` Jernej Škrabec
2019-07-29 16:29       ` Uwe Kleine-König
2019-07-26 18:40 ` [PATCH 6/6] arm64: dts: allwinner: h6: Add PWM node Jernej Skrabec
2019-07-27 10:51   ` Maxime Ripard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190731065230.mqbtn5sfoxrkevw5@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=devicetree@vger.kernel.org \
    --cc=frowand.list@gmail.com \
    --cc=jernej.skrabec@siol.net \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=mark.rutland@arm.com \
    --cc=mripard@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).