Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
From: Ezequiel Garcia <ezequiel@collabora.com>
To: "Heiko Stübner" <heiko@sntech.de>, kever.yang@rock-chips.com
Cc: "Mylène Josserand" <mylene.josserand@collabora.com>,
	linux@armlinux.org.uk, mturquette@baylibre.com, sboyd@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, kernel@collabora.com,
	linux-clk@vger.kernel.org
Subject: Re: [PATCH 1/2] ARM: Rockchip: Handle rk3288/rk3288w revision
Date: Thu, 05 Mar 2020 08:35:47 -0300
Message-ID: <506e1aeda3ed14276734f22f320b0ceb6a4b06b0.camel@collabora.com> (raw)
In-Reply-To: <8b993ec8ae9dccc6a8633cfe5e5b6d40d58ffbae.camel@collabora.com>

(Adding Kever to the loop)

On Thu, 2020-03-05 at 08:32 -0300, Ezequiel Garcia wrote:
> On Thu, 2020-03-05 at 01:51 +0100, Heiko Stübner wrote:
> > Hi Ezequiel,
> > 
> > Am Donnerstag, 5. März 2020, 01:03:30 CET schrieb Ezequiel Garcia:
> > > Hi Heiko,
> > > 
> > > On Wed, 2020-03-04 at 11:59 +0100, Heiko Stübner wrote:
> > > > Hi,
> > > > 
> > > > Am Montag, 2. März 2020, 16:57:02 CET schrieb Mylène Josserand:
> > > > > Determine which revision of rk3288 by checking the HDMI version.
> > > > > According to the Rockchip BSP kernel, on rk3288w, the HDMI
> > > > > revision equals 0x1A which is not the case for the rk3288 [1].
> > > > > 
> > > > > As these SOC have some differences, the new function
> > > > > 'soc_is_rk3288w' will help us to know on which revision
> > > > > we are.
> > > > 
> > > > what happened to just having a different compatible in the dts?
> > > > Aka doing a 
> > > > 
> > > > rk3288w.dtsi with
> > > > 
> > > > #include "rk3288.dtsi"
> > > > 
> > > > &cru {
> > > > 	compatible = "rockchip,rk3288w-cru";
> > > > }
> > > > 
> > > 
> > > I guess you have something like this in mind:
> > > 
> > > static void __init rk3288_clk_init(struct device_node *np)
> > > {
> > >         __rk3288_clk_init(np, RK3288_SOC_REV_RK3288W);
> > > }
> > > CLK_OF_DECLARE(rk3288_cru, "rockchip,rk3288-cru", rk3288_clk_init);
> > > 
> > > static void __init rk3288w_clk_init(struct device_node *np)
> > > {
> > >         __rk3288_clk_init(np, RK3288_SOC_REV_RK3288);
> > > }
> > > CLK_OF_DECLARE(rk3288_cru, "rockchip,rk3288w-cru", rk3288w_clk_init);
> > > 
> > > And the rest is mostly untouched, except the revision is
> > > no longer queried and is now passed by the DT?
> > 
> > Essentially yes, but I guess I was more thinking along the lines of
> > the rk3188/rk3066a/rk3188a (drivers/clk/rockchip/clk-rk3188.c)
> > 
> > 
> > > This would be cleaner for the kernel, with the obvious
> > > drawback being that you now have to maintain
> > > another DTS.
> > 
> > Right now we would end up with the pretty minimal devicetree
> > having just that cru. So not very invasive.
> > 
> > 
> > > This could be an inconvenience. I believe
> > > RK3288W is meant as a direct replacement for RK3288,
> > > so folks building products would expect to just use
> > > RK3288W, and not really bother with passing a
> > > different DTS or what not.
> > 
> > Not sure I follow. As below, I don't think boards will magically switch
> > between soc variants, so a boards devicetree should just include
> > the variant - especially as I don't really know how many
> > additional new boards we will see with it (rk3288 being quite old itself).
> > 
> 
> It's not about new boards, any company does sells some RK32888-based product,
> will eventually want to produce some more.
> 
> If I understand correctly, RK3288W is the SoC that Rockchip is now offering,
> and not RK3288 anymore.
> 
> So, if you have to produce another batch of _the same RK3288_ product,
> you'll have to use RK3288W. In other words, they would "just switch"
> between SoC variants.
> 
> In fact, such a case motivates these patches :-)
> 
> > > > I somehow don't expect boards to just switch between soc variants
> > > > on the fly.
> > > > 
> > > 
> > > While I agree they are nasty, quirks like this
> > > are not uncommon.
> > > 
> > > > Also, doing things in mach-rockchip is not very future-proof:
> > > > 
> > > 
> > > There is actually no reason to keep this in mach-rockchip, right?
> > > 
> > > The quirk could be placed in other places. For instance,
> > > directly in the clock driver.
> > 
> > Mapping the hdmi controller inside the clock driver to read some "random"
> > register that hopefully indicates an (undocumented) distinction between soc
> > variants.
> > 
> > Somehow just having that minimal devicetree for the "w" sounds
> > way cleaner ;-) .
> > 
> > 
> > It's definitly cool to have support for the rk3288w but I don't like
> > adding hacks for something that is after all some sort of niche product.
> > 
> 
> Why do you think it's a niche product?
> 
> Thanks,
> Ezequiel
> 
> > Heiko
> > 
> > 
> > > > (1) having random soc-specific APIs spanning the kernel feels wrong,
> > > >     especially as at some point it might not be contained to our own special
> > > >     drivers like the cru. I cannot really see people being enthusiastic if
> > > >     something like this would be needed in say the core Analogix-DP bridge ;-)
> > > > (2) I guess the rk3288w will not be the last soc doing this and on arm64 you
> > > >     can't do it that way, as there is no mach-rockchip there
> > > > 
> > > > So my personal preference would really would be just a specific compatible
> > > > for affected ip blocks.
> > > > 
> > > > Heiko
> > > > 
> > > > > [1]:https://github.com/rockchip-linux/u-boot/blob/f992fe3334aa5090acb448261982628b5a3d37a5/arch/arm/include/asm/arch-rockchip/cpu.h#L30..L34
> > > > > 
> > > > > Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
> > > > > ---
> > > > >  arch/arm/mach-rockchip/rockchip.c | 45 +++++++++++++++++++++++++++++++
> > > > >  include/soc/rockchip/revision.h   | 22 +++++++++++++++
> > > > >  2 files changed, 67 insertions(+)
> > > > >  create mode 100644 include/soc/rockchip/revision.h
> > > > > 
> > > > > diff --git a/arch/arm/mach-rockchip/rockchip.c b/arch/arm/mach-rockchip/rockchip.c
> > > > > index f9797a2b5d0d..b907ba390093 100644
> > > > > --- a/arch/arm/mach-rockchip/rockchip.c
> > > > > +++ b/arch/arm/mach-rockchip/rockchip.c
> > > > > @@ -9,12 +9,14 @@
> > > > >  #include <linux/kernel.h>
> > > > >  #include <linux/init.h>
> > > > >  #include <linux/io.h>
> > > > > +#include <linux/of_address.h>
> > > > >  #include <linux/of_platform.h>
> > > > >  #include <linux/irqchip.h>
> > > > >  #include <linux/clk-provider.h>
> > > > >  #include <linux/clocksource.h>
> > > > >  #include <linux/mfd/syscon.h>
> > > > >  #include <linux/regmap.h>
> > > > > +#include <soc/rockchip/revision.h>
> > > > >  #include <asm/mach/arch.h>
> > > > >  #include <asm/mach/map.h>
> > > > >  #include <asm/hardware/cache-l2x0.h>
> > > > > @@ -22,6 +24,49 @@
> > > > >  #include "pm.h"
> > > > >  
> > > > >  #define RK3288_TIMER6_7_PHYS 0xff810000
> > > > > +#define RK3288_HDMI_REV_REG	0x04
> > > > > +#define RK3288W_HDMI_REV	0x1A
> > > > > +
> > > > > +static const struct of_device_id rk3288_dt_hdmi_match[] __initconst = {
> > > > > +	{ .compatible = "rockchip,rk3288-dw-hdmi" },
> > > > > +	{ }
> > > > > +};
> > > > > +
> > > > > +int rk3288_get_revision(void)
> > > > > +{
> > > > > +	static int revision = RK3288_SOC_REV_UNKNOWN;
> > > > > +	struct device_node *dn;
> > > > > +	void __iomem *hdmi_base;
> > > > > +
> > > > > +	if (revision != RK3288_SOC_REV_UNKNOWN)
> > > > > +		return revision;
> > > > > +
> > > > > +	dn = of_find_matching_node(NULL, rk3288_dt_hdmi_match);
> > > > > +	if (!dn) {
> > > > > +		pr_err("%s: Couldn't find HDMI node\n", __func__);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	hdmi_base = of_iomap(dn, 0);
> > > > > +	of_node_put(dn);
> > > > > +
> > > > > +	if (!hdmi_base) {
> > > > > +		pr_err("%s: Couldn't map %pOF regs\n", __func__,
> > > > > +		       hdmi_base);
> > > > > +		return -ENXIO;
> > > > > +	}
> > > > > +
> > > > > +	if (readl_relaxed(hdmi_base + RK3288_HDMI_REV_REG) ==
> > > > > +	    RK3288W_HDMI_REV)
> > > > > +		revision = RK3288_SOC_REV_RK3288W;
> > > > > +	else
> > > > > +		revision = RK3288_SOC_REV_RK3288;
> > > > > +
> > > > > +	iounmap(hdmi_base);
> > > > > +
> > > > > +	return revision;
> > > > > +}
> > > > > +EXPORT_SYMBOL(rk3288_get_revision);
> > > > >  
> > > > >  static void __init rockchip_timer_init(void)
> > > > >  {
> > > > > diff --git a/include/soc/rockchip/revision.h b/include/soc/rockchip/revision.h
> > > > > new file mode 100644
> > > > > index 000000000000..226419c60af0
> > > > > --- /dev/null
> > > > > +++ b/include/soc/rockchip/revision.h
> > > > > @@ -0,0 +1,22 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > > +/*
> > > > > + * Copyright 2020 Collabora
> > > > > + */
> > > > > +
> > > > > +#ifndef __SOC_ROCKCHIP_REVISION_H__
> > > > > +#define __SOC_ROCKCHIP_REVISION_H__
> > > > > +
> > > > > +enum rk3288_soc_revision {
> > > > > +	RK3288_SOC_REV_UNKNOWN,
> > > > > +	RK3288_SOC_REV_RK3288,
> > > > > +	RK3288_SOC_REV_RK3288W,
> > > > > +};
> > > > > +
> > > > > +int rk3288_get_revision(void);
> > > > > +
> > > > > +static inline bool soc_is_rk3288w(void)
> > > > > +{
> > > > > +	return rk3288_get_revision() == RK3288_SOC_REV_RK3288W;
> > > > > +}
> > > > > +
> > > > > +#endif /* __SOC_ROCKCHIP_REVISION_H__ */
> > > > > 
> > > > 
> > > > 
> > 
> > 
> 
> 



  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-02 15:57 [PATCH 0/2] ARM: Add Rockchip rk3288w support Mylène Josserand
2020-03-02 15:57 ` [PATCH 1/2] ARM: Rockchip: Handle rk3288/rk3288w revision Mylène Josserand
2020-03-04 10:59   ` Heiko Stübner
2020-03-05  0:03     ` Ezequiel Garcia
2020-03-05  0:51       ` Heiko Stübner
2020-03-05 11:32         ` Ezequiel Garcia
2020-03-05 11:35           ` Ezequiel Garcia [this message]
2020-03-06  2:44             ` Kever Yang
2020-03-06 10:30               ` Ezequiel Garcia
2020-03-06 10:45     ` Geert Uytterhoeven
2020-03-26 13:50       ` Mylene Josserand
2020-03-26 15:31         ` Geert Uytterhoeven
2020-03-27  6:20           ` Mylene Josserand
2020-03-02 15:57 ` [PATCH 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w Mylène Josserand
2020-03-04  5:34   ` kbuild test robot

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=506e1aeda3ed14276734f22f320b0ceb6a4b06b0.camel@collabora.com \
    --to=ezequiel@collabora.com \
    --cc=heiko@sntech.de \
    --cc=kernel@collabora.com \
    --cc=kever.yang@rock-chips.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=linux@armlinux.org.uk \
    --cc=mturquette@baylibre.com \
    --cc=mylene.josserand@collabora.com \
    --cc=sboyd@kernel.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

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git