(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@xxxxxxxxxxxxx>
> > > > > ---
> > > > > 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__ */
> > > > >
> > > >
> > > >
> >
> >
>
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-rockchip
