All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brian Norris <briannorris@chromium.org>
To: Heiko Stuebner <heiko@sntech.de>
Cc: Arnd Bergmann <arnd@arndb.de>,
	shawn.lin@rock-chips.com, Doug Anderson <dianders@chromium.org>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	Olof Johansson <olof@lixom.net>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup
Date: Thu, 9 Mar 2017 15:28:54 -0800	[thread overview]
Message-ID: <20170309232852.GA15605@google.com> (raw)
In-Reply-To: <2206970.JAebETMOV6@phil>

To throw my unwanted 2-cents in:

On Mon, Feb 27, 2017 at 06:30:08AM +0100, Heiko Stuebner wrote:
> Am Sonntag, 29. Januar 2017, 16:41:46 CET schrieb Olof Johansson:
> > On Sun, Jan 29, 2017 at 3:36 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > Or, if you're firmly deciding to keep updating kernel code for all
> > SoCs, could also just add one platform quirk file in
> > drivers/soc/rockchip with the postcore_initcall that matches toplevel
> > compatible per SoC, finds the device node, maps the address range,
> > sets the value.
> > 
> > That'll give you a place for other platform quirks, if they ever come
> > up. In one place, even if the other quirks might be in other register
> > ranges.
> 
> I would very much prefer going this route, as I really don't trust the GRF to 
> keep conforming to any devicetree binding (or anything at all) and this way, 
> stuff is completely reversable / redoable if the need arises (hopefully not) 
> and we don't have to live with an inadequate bindings forever :-) .

Agreed, FWIW (though I'm not sure why a top-level chip driver is better
than a GRF driver). Rockchip's GRF is basically "designed" (that's
probably not an accurate word) to need updating on every chip revision.
I'm also not very confident in any given binding around it. DT bindings
are notoriously hard to modify after they've been accepted, and I don't
see much actual value in foolishly codifying every bit of GRF in the
device tree bindings.

I think this pattern applies elsewhere too -- for instance, on the
Type-C PHY controller [1], which currently defies the above and instead
tries to list each GRF offset it needs in the DT; this is, IMO, quite
unscalable. We're already nearly breaking backward compatibility, and
we've only supported one SoC there!!

Brian

[1] https://patchwork.kernel.org/patch/9566079/
    [PATCH 3/4] phy: rockchip-typec: support DP phy switch

WARNING: multiple messages have this Message-ID (diff)
From: briannorris@chromium.org (Brian Norris)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup
Date: Thu, 9 Mar 2017 15:28:54 -0800	[thread overview]
Message-ID: <20170309232852.GA15605@google.com> (raw)
In-Reply-To: <2206970.JAebETMOV6@phil>

To throw my unwanted 2-cents in:

On Mon, Feb 27, 2017 at 06:30:08AM +0100, Heiko Stuebner wrote:
> Am Sonntag, 29. Januar 2017, 16:41:46 CET schrieb Olof Johansson:
> > On Sun, Jan 29, 2017 at 3:36 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > Or, if you're firmly deciding to keep updating kernel code for all
> > SoCs, could also just add one platform quirk file in
> > drivers/soc/rockchip with the postcore_initcall that matches toplevel
> > compatible per SoC, finds the device node, maps the address range,
> > sets the value.
> > 
> > That'll give you a place for other platform quirks, if they ever come
> > up. In one place, even if the other quirks might be in other register
> > ranges.
> 
> I would very much prefer going this route, as I really don't trust the GRF to 
> keep conforming to any devicetree binding (or anything at all) and this way, 
> stuff is completely reversable / redoable if the need arises (hopefully not) 
> and we don't have to live with an inadequate bindings forever :-) .

Agreed, FWIW (though I'm not sure why a top-level chip driver is better
than a GRF driver). Rockchip's GRF is basically "designed" (that's
probably not an accurate word) to need updating on every chip revision.
I'm also not very confident in any given binding around it. DT bindings
are notoriously hard to modify after they've been accepted, and I don't
see much actual value in foolishly codifying every bit of GRF in the
device tree bindings.

I think this pattern applies elsewhere too -- for instance, on the
Type-C PHY controller [1], which currently defies the above and instead
tries to list each GRF offset it needs in the DT; this is, IMO, quite
unscalable. We're already nearly breaking backward compatibility, and
we've only supported one SoC there!!

Brian

[1] https://patchwork.kernel.org/patch/9566079/
    [PATCH 3/4] phy: rockchip-typec: support DP phy switch

  reply	other threads:[~2017-03-09 23:28 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-15 22:38 [PATCH v2 0/3] Rockchip: generalize GRF setup Heiko Stuebner
2016-11-15 22:38 ` Heiko Stuebner
     [not found] ` <20161115223900.30728-1-heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
2016-11-15 22:38   ` [PATCH v2 1/3] dt-bindings: add used but undocumented rockchip grf compatible values Heiko Stuebner
2016-11-15 22:38     ` Heiko Stuebner
2016-11-15 22:38   ` [PATCH v2 2/3] soc: rockchip: add driver handling grf setup Heiko Stuebner
2016-11-15 22:38     ` Heiko Stuebner
     [not found]     ` <20161115223900.30728-3-heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
2016-11-16  9:33       ` Shawn Lin
2016-11-16  9:33         ` Shawn Lin
     [not found]         ` <95cd052e-7e0d-d13b-fedb-21f7b49d17bb-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-11-16  9:58           ` Heiko Stuebner
2016-11-16  9:58             ` Heiko Stuebner
2016-11-17  1:38             ` Shawn Lin
2016-11-17  1:38               ` Shawn Lin
     [not found]               ` <a01a1a8c-b01b-4335-b69c-bb99dec9d6d6-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-11-17  9:12                 ` Heiko Stuebner
2016-11-17  9:12                   ` Heiko Stuebner
2016-11-16 21:50       ` Doug Anderson
2016-11-16 21:50         ` Doug Anderson
2017-01-29 22:40     ` Olof Johansson
2017-01-29 22:40       ` Olof Johansson
     [not found]       ` <CAOesGMjq3uPqaWjMkEkqXs9W90XTwXknjheZNdYsOaGu4fw9_g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-01-29 23:36         ` Heiko Stuebner
2017-01-29 23:36           ` Heiko Stuebner
2017-01-30  0:41           ` Olof Johansson
2017-01-30  0:41             ` Olof Johansson
     [not found]             ` <CAOesGMj_RdxVDzirT97KLkXRsyVcFh3Z8jZLNjn0ehAv0Qhmhg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-02-27  5:30               ` Heiko Stuebner
2017-02-27  5:30                 ` Heiko Stuebner
2017-03-09 23:28                 ` Brian Norris [this message]
2017-03-09 23:28                   ` Brian Norris
2016-11-15 22:39   ` [PATCH v2 3/3] ARM: rockchip: drop rk3288 jtag/mmc switch handling Heiko Stuebner
2016-11-15 22:39     ` Heiko Stuebner

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=20170309232852.GA15605@google.com \
    --to=briannorris@chromium.org \
    --cc=arnd@arndb.de \
    --cc=dianders@chromium.org \
    --cc=heiko@sntech.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=olof@lixom.net \
    --cc=shawn.lin@rock-chips.com \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.