LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Peng Fan <peng.fan@nxp.com>
To: Abel Vesa <abel.vesa@nxp.com>,
	Aisheng Dong <aisheng.dong@nxp.com>,
	Leonard Crestez <leonard.crestez@nxp.com>
Cc: Aisheng Dong <aisheng.dong@nxp.com>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	Leonard Crestez <leonard.crestez@nxp.com>,
	"sboyd@kernel.org" <sboyd@kernel.org>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	dl-linux-imx <linux-imx@nxp.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Anson Huang <anson.huang@nxp.com>,
	Daniel Baluta <daniel.baluta@nxp.com>,
	"aford173@gmail.com" <aford173@gmail.com>,
	Jacky Bai <ping.bai@nxp.com>, Jun Li <jun.li@nxp.com>,
	"l.stach@pengutronix.de" <l.stach@pengutronix.de>,
	"andrew.smirnov@gmail.com" <andrew.smirnov@gmail.com>,
	"agx@sigxcpu.org" <agx@sigxcpu.org>,
	"angus@akkea.ca" <angus@akkea.ca>,
	"heiko@sntech.de" <heiko@sntech.de>,
	Andy Duan <fugang.duan@nxp.com>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>
Subject: RE: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite clk
Date: Thu, 30 Apr 2020 12:56:28 +0000	[thread overview]
Message-ID: <DB6PR0402MB2760925E380A8E5907F6EF3788AA0@DB6PR0402MB2760.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20200430100055.6rhec5rwtz2yyqbl@fsr-ub1664-175>

Hi Abel, Aisheng, Leonard and all

> Subject: Re: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite
> clk
> 
....
> > > > +
> > > > +	return clk_mux_val_to_index(hw, mux->table, mux->flags, val); }
> > >
> > > You don't have to redefinition them if they're the same as clk_mux_ops.
> > > You have the access to clk_mux_ops.
> >
> > This will require export_symbol of clk_mux_ops callbacks.
> >
> 
> Maybe you can do here:
> 
> return clk_mux_ops.get_parent(hw);

Ok, will try this.

> 
> > >
> > > > +
> > > > +static int imx8m_clk_composite_mux_set_parent(struct clk_hw *hw,
> > > > +u8
> > > > +index) {
> > > > +	struct clk_mux *mux = to_clk_mux(hw);
> > > > +	u32 val = clk_mux_index_to_val(mux->table, mux->flags, index);
> > > > +	unsigned long flags = 0;
> > > > +	u32 reg;
> > > > +
> > > > +	if (mux->lock)
> > > > +		spin_lock_irqsave(mux->lock, flags);
> > > > +
> > > > +	reg = readl(mux->reg);
> > > > +	reg &= ~(mux->mask << mux->shift);
> > > > +	val = val << mux->shift;
> > > > +	reg |= val;
> > > > +	/* write twice to make sure SEL_A/B point the same mux */
> > > > +	writel(reg, mux->reg);
> > > > +	writel(reg, mux->reg);
> > >
> > > Why this affects both SEL_A/B?
> >
> > The internal counter will make sure both SEL_A/B point to the same
> > mux.
> >
> > > Very tricky and may worth more comments.
> >
> > Ah, I think RM should be clear about the target interface and
> > non-target interface.
> >
> > When you write once, saying use SEL_A, when you write the 2nd, the
> > hardware will use SEL_B, when you write 3rd, the hardware will use
> > SEL_A.
> > and ...
> >
> 
> This is a very interesting behavior from HW point of view.
> So every write changes the mux ?
> 
> Unless there is an ERRATA for this, we'll get a lot of pushback from upstream.

Let me share more details about this. Then if ok, I'll put in commit log and post
V2.

There is no errata, this is the hardware designed as is and it exist since i.MX7D.
i.MX8M and i.MX7D using same CCM root design.

It support target(smart) interface and normal interface. Target interface is exported
for programmer easy to configure ccm root. Normal interface is also
exported, but we not use it in our driver, because it will introduce more
complexity compared with target interface.

From RM:
"
The Target Interface is optimized to simplify software operation. Using this interface, all
clock roots are in the same program model with the same register bit field mapping. The
software does not handle the details of the clock slice and clock slice types. Software
writes the desired settings to the register, and the internal hardware logic generates a
required sequence to achieve the desired settings.
"

From i.MX8MN RM:
"
A requirement of the Target Interface's software is that the
target clock source is active.
"

We touch target interface, but hardware logic actually also need configure normal interface.

I draw a simple normal interface for core clock slice pic:

CLK[0-7] --------->SEL_A ----->-----CG---->|
         |                        \
         |                          mux-->post_podf-->
         V                        /                   
         |------>SEL_B------>---CG---->| /


The mux in the upper pic is not the target interface MUX, target interface MUX is
hiding SEL_A and SEL_B. When you choose clk[0-7], you are actually writing
SEL_A or SEL_B depends on the internal counter which will also control the
internal "mux".

Whether the hardware touch SEL_A or SEL_B, it requires the clock input to
SEL_A or SEL_B must be active. However SEL_A and SEL_B could have
different value. Saying SEL_A is clk1, SEL_B is clk4, the internal counter
controlled automatically by hardware logic choose SEL_A, then Linux will
disable clk4 because of no user. Now we write target MUX to choose clk5,
the internal counter will configure SEL_B to clk5 and switch to SEL_B,
however the previous SEL_B input clk4 is off, system hang, the hardware
requires SEL_B input clk4 is on, then hardware could configure SEL_B to
clk5.

That's why write twice to make sure the internal counter could select
SEL_A and SEL_B to same active input clk.

Please see whether this clarify the issue or not. I could post the upper
into commit log in V3. The fixes needs to be into 5.7 to avoid system
boot hang.

Thanks,
Peng.

> 
> > >
> > > Besides that, I'd like to see Abel's comments on this patch.
> >
> >
> > Abel,
> >
> >  Any comments?
> >
> > Thanks,
> > Peng.
> >
> > >
> > > Regards
> > > Aisheng
> > >
> > > > +
> > > > +	if (mux->lock)
> > > > +		spin_unlock_irqrestore(mux->lock, flags);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +imx8m_clk_composite_mux_determine_rate(struct clk_hw *hw,
> > > > +				       struct clk_rate_request *req) {
> > > > +	struct clk_mux *mux = to_clk_mux(hw);
> > > > +
> > > > +	return clk_mux_determine_rate_flags(hw, req, mux->flags); }
> > >
> > > Same as bove.
> > >
> > > > +
> > > > +
> > > > +const struct clk_ops imx8m_clk_composite_mux_ops = {
> > > > +	.get_parent = imx8m_clk_composite_mux_get_parent,
> > > > +	.set_parent = imx8m_clk_composite_mux_set_parent,
> > > > +	.determine_rate = imx8m_clk_composite_mux_determine_rate,
> > > > +};
> > > > +
> > > >  struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
> > > >  					const char * const *parent_names,
> > > >  					int num_parents, void __iomem *reg, @@
> -136,6
> > > > +193,7 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char
> > > > +*name,
> > > >  	struct clk_gate *gate = NULL;
> > > >  	struct clk_mux *mux = NULL;
> > > >  	const struct clk_ops *divider_ops;
> > > > +	const struct clk_ops *mux_ops;
> > > >
> > > >  	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> > > >  	if (!mux)
> > > > @@ -157,10 +215,12 @@ struct clk_hw
> > > > *imx8m_clk_hw_composite_flags(const char *name,
> > > >  		div->shift = PCG_DIV_SHIFT;
> > > >  		div->width = PCG_CORE_DIV_WIDTH;
> > > >  		divider_ops = &clk_divider_ops;
> > > > +		mux_ops = &imx8m_clk_composite_mux_ops;
> > > >  	} else {
> > > >  		div->shift = PCG_PREDIV_SHIFT;
> > > >  		div->width = PCG_PREDIV_WIDTH;
> > > >  		divider_ops = &imx8m_clk_composite_divider_ops;
> > > > +		mux_ops = &clk_mux_ops;
> > > >  	}
> > > >
> > > >  	div->lock = &imx_ccm_lock;
> > > > @@ -176,7 +236,7 @@ struct clk_hw
> > > *imx8m_clk_hw_composite_flags(const
> > > > char *name,
> > > >  	gate->lock = &imx_ccm_lock;
> > > >
> > > >  	hw = clk_hw_register_composite(NULL, name, parent_names,
> > > > num_parents,
> > > > -			mux_hw, &clk_mux_ops, div_hw,
> > > > +			mux_hw, mux_ops, div_hw,
> > > >  			divider_ops, gate_hw, &clk_gate_ops, flags);
> > > >  	if (IS_ERR(hw))
> > > >  		goto fail;
> > > > --
> > > > 2.16.4
> >

  reply	other threads:[~2020-04-30 12:56 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-12 10:19 [PATCH V2 00/10] clk: imx: fixes and improve for i.MX8M peng.fan
2020-03-12 10:19 ` [PATCH V2 01/10] arm64: dts: imx8m: assign clocks for A53 peng.fan
2020-04-26  3:51   ` Aisheng Dong
2020-03-12 10:19 ` [PATCH V2 02/10] clk: imx8m: drop clk_hw_set_parent " peng.fan
2020-04-26  3:54   ` Aisheng Dong
2020-03-12 10:19 ` [PATCH V2 03/10] clk: imx: imx8mp: fix pll mux bit peng.fan
2020-04-26  4:23   ` Aisheng Dong
2020-03-12 10:19 ` [PATCH V2 04/10] clk: imx8mp: Define gates for pll1/2 fixed dividers peng.fan
2020-04-26  4:29   ` Aisheng Dong
2020-03-12 10:19 ` [PATCH V2 05/10] clk: imx8mp: use imx8m_clk_hw_composite_core to simplify code peng.fan
2020-04-26  4:38   ` Aisheng Dong
2020-04-27  8:57     ` Peng Fan
2020-03-12 10:19 ` [PATCH V2 06/10] clk: imx8m: migrate A53 clk root to use composite core peng.fan
2020-04-26  4:43   ` Aisheng Dong
2020-04-27  8:58     ` Peng Fan
2020-03-12 10:19 ` [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite clk peng.fan
2020-04-24 19:29   ` Leonard Crestez
2020-04-27  9:15     ` Peng Fan
2020-04-27 19:34       ` Leonard Crestez
2020-04-28  1:08         ` Peng Fan
2020-04-26  5:08   ` Aisheng Dong
2020-04-27  9:11     ` Peng Fan
2020-04-30 10:00       ` Abel Vesa
2020-04-30 12:56         ` Peng Fan [this message]
2020-03-12 10:19 ` [PATCH V2 08/10] clk: imx: add imx8m_clk_hw_composite_bus peng.fan
2020-03-12 10:19 ` [PATCH V2 09/10] clk: imx: use imx8m_clk_hw_composite_bus for i.MX8M bus clk slice peng.fan
2020-03-12 10:19 ` [PATCH V2 10/10] clk: imx8mp: mark memrepair clock as critical peng.fan
2020-03-19 10:04 ` [PATCH V2 00/10] clk: imx: fixes and improve for i.MX8M Peng Fan
2020-04-18 13:45 ` Peng Fan
2020-04-24 19:30   ` Leonard Crestez

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=DB6PR0402MB2760925E380A8E5907F6EF3788AA0@DB6PR0402MB2760.eurprd04.prod.outlook.com \
    --to=peng.fan@nxp.com \
    --cc=abel.vesa@nxp.com \
    --cc=aford173@gmail.com \
    --cc=agx@sigxcpu.org \
    --cc=aisheng.dong@nxp.com \
    --cc=andrew.smirnov@gmail.com \
    --cc=angus@akkea.ca \
    --cc=anson.huang@nxp.com \
    --cc=daniel.baluta@nxp.com \
    --cc=festevam@gmail.com \
    --cc=fugang.duan@nxp.com \
    --cc=heiko@sntech.de \
    --cc=jun.li@nxp.com \
    --cc=kernel@pengutronix.de \
    --cc=l.stach@pengutronix.de \
    --cc=leonard.crestez@nxp.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ping.bai@nxp.com \
    --cc=s.hauer@pengutronix.de \
    --cc=sboyd@kernel.org \
    --cc=shawnguo@kernel.org \
    --subject='RE: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite clk' \
    /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

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).