From: Philipp Zabel <p.zabel@pengutronix.de>
To: Jun Nie <jun.nie@linaro.org>
Cc: Michael Turquette <mturquette@baylibre.com>,
sboyd@kernel.org, Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Wei Xu <xuwei5@hisilicon.com>,
opensource@jilayne.com, swinslow@gmail.com, allison@lohutok.net,
yuehaibing@huawei.com, tglx@linutronix.de,
linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, xuejiancheng@hisilicon.com
Subject: Re: [PATCH 2/3] reset: hisilicon: Extend reset operation type
Date: Tue, 03 Dec 2019 15:15:28 +0100 [thread overview]
Message-ID: <5ca8ff65b8489195ef5349cd2d074d412c9d0036.camel@pengutronix.de> (raw)
In-Reply-To: <CABymUCNDZSH+mB9TyyUBwgRu-qTRbgUv89va2HuBs4VeJWn6uA@mail.gmail.com>
Hi Jun,
On Tue, 2019-12-03 at 11:53 +0800, Jun Nie wrote:
[...]
> @@ -48,13 +56,33 @@ static int hisi_reset_assert(struct reset_controller_dev *rcdev,
> > > u32 offset, reg;
> > > u8 bit;
> > >
> > > + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> > > + if (flags & HISI_ASSERT_NONE)
> > > + return -ENOTSUPP; /* assert not supported for this reset */
> >
> > As long as .assert and .deassert are the only implemented operations for
> > this reset controller, this does not make any sense to me. Are there
> > resets that can only be deasserted?
>
> Some reset is asserted on power on automatically. So only .deassert is needed.
But if the bit was set/cleared again after being deasserted, would it
assert the reset line? Basically, I wonder if those bits are write-once
or not.
[...]
> > > + if (flags & HISI_ASSERT_SET)
> > > + return readl_poll_timeout(rstc->membase + offset,
> > > + reg, reg & BIT(bit), 0, 5000);
> > > + else
> > > + return readl_poll_timeout(rstc->membase + offset,
> > > + reg, !(reg & BIT(bit)),
> > > + 0, 5000);
> >
> > And this is effectively dead code. Do you really have hardware that
> > asserts or asserts a reset line in reaction to a read access?
> >
> > Should HISI_ASSERT_POLL and HISI_DEASSERT_POLL be mutually exclusive?
This is missing an explanation.
[...]
> > > + writel(reg, rstc->membase + offset);
> > >
> > > spin_unlock_irqrestore(&rstc->lock, flags);
> > >
> > > @@ -69,13 +97,33 @@ static int hisi_reset_deassert(struct reset_controller_dev *rcdev,
> > > u32 offset, reg;
> > > u8 bit;
> > >
> > > + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> > > + if (flags & HISI_DEASSERT_NONE)
> > > + return -ENOTSUPP; /* deassert not supported for this reset */
> > > +
> >
> > Are there resets that can only ever be asserted?
>
> I do not know yet. Only extend this driver with all combination in logic.
If this is not used, let's leave it out.
[...]
> > > @@ -103,7 +151,7 @@ struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev)
> > > rstc->rcdev.owner = THIS_MODULE;
> > > rstc->rcdev.ops = &hisi_reset_ops;
> > > rstc->rcdev.of_node = pdev->dev.of_node;
> > > - rstc->rcdev.of_reset_n_cells = 2;
> > > + rstc->rcdev.of_reset_n_cells = 3;
> >
> > This breaks current device trees (before patch 3). You can make sure
> > device trees with #reset-cells = <2> keep working by parsing the #reset-
> > cells and setting this value to 2 for old DTs.
>
> All current dts file are modified accordingly. But existing dtb binary support
> is an issue. Do you have any suggestion?
Since this is for a new SoC, you should keep using of_reset_n_cells = 2
for the current SoCs and only set it to 3 for a new compatible, for
example using of_device_get_match_data().
regards
Philipp
next prev parent reply other threads:[~2019-12-03 14:15 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-02 14:45 [PATCH 0/3] Extend Hisilicon reset type Jun Nie
2019-12-02 14:45 ` [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc Jun Nie
2019-12-02 17:04 ` Philipp Zabel
2019-12-03 3:11 ` Jun Nie
2019-12-03 14:12 ` Philipp Zabel
2019-12-02 14:45 ` [PATCH 2/3] reset: hisilicon: Extend reset operation type Jun Nie
2019-12-02 17:04 ` Philipp Zabel
2019-12-03 3:53 ` Jun Nie
2019-12-03 14:15 ` Philipp Zabel [this message]
2019-12-04 6:09 ` Jun Nie
2019-12-02 14:45 ` [PATCH 3/3] ARM: dts: Update reset for hi3519 and hi3798cv200 Jun Nie
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=5ca8ff65b8489195ef5349cd2d074d412c9d0036.camel@pengutronix.de \
--to=p.zabel@pengutronix.de \
--cc=allison@lohutok.net \
--cc=devicetree@vger.kernel.org \
--cc=jun.nie@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mturquette@baylibre.com \
--cc=opensource@jilayne.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=swinslow@gmail.com \
--cc=tglx@linutronix.de \
--cc=xuejiancheng@hisilicon.com \
--cc=xuwei5@hisilicon.com \
--cc=yuehaibing@huawei.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 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).