From: Andre Przywara <andre.przywara@arm.com>
To: Philipp Zabel <p.zabel@pengutronix.de>, linux-kernel@vger.kernel.org
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>,
Alexandre Torgue <alexandre.torgue@st.com>,
Maxime Ripard <maxime.ripard@free-electrons.com>,
Chen-Yu Tsai <wens@csie.org>
Subject: Re: [PATCH 2/2] reset: simple: read back to make sure changes are applied
Date: Wed, 8 Mar 2017 14:04:27 +0000 [thread overview]
Message-ID: <c046c1c1-7a14-5052-fbf1-d1efab6c9d9f@arm.com> (raw)
In-Reply-To: <20170308095423.3948-2-p.zabel@pengutronix.de>
Hi,
On 08/03/17 09:54, Philipp Zabel wrote:
> Read back the register after setting or clearing a reset bit to make
> sure that the changes are applied to the reset controller hardware.
> Theoretically, this avoids the write to stay stuck in a store buffer
> during the delay of an assert-delay-deassert sequence, and makes sure
> that the reset really is asserted for the specified duration.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> drivers/reset/reset-simple.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index 2160e84fe216b..e32289bdaf0c6 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -33,13 +33,16 @@ static int reset_simple_clear(struct reset_controller_dev *rcdev,
> int reg_width = sizeof(u32);
> int bank = id / (reg_width * BITS_PER_BYTE);
> int offset = id % (reg_width * BITS_PER_BYTE);
> + void __iomem *addr = data->membase + (bank * reg_width);
> unsigned long flags;
> u32 reg;
>
> spin_lock_irqsave(&data->lock, flags);
>
> - reg = readl(data->membase + (bank * reg_width));
> - writel(reg & ~BIT(offset), data->membase + (bank * reg_width));
> + reg = readl(addr);
> + writel(reg & ~BIT(offset), addr);
> + /* Read back to make sure the write doesn't linger in a store buffer */
> + readl(addr);
Nit: "Read back to make sure the write doesn't linger in a store
buffer", sounds somewhat dodgy.
What about: "Read back to make sure the write has reached the device."?
And I wonder if we actually need to check the returned value to make
sure the device has actually changed the state? Or is this too paranoid?
Cheers,
Andre.
> spin_unlock_irqrestore(&data->lock, flags);
>
> @@ -53,13 +56,16 @@ static int reset_simple_set(struct reset_controller_dev *rcdev,
> int reg_width = sizeof(u32);
> int bank = id / (reg_width * BITS_PER_BYTE);
> int offset = id % (reg_width * BITS_PER_BYTE);
> + void __iomem *addr = data->membase + (bank * reg_width);
> unsigned long flags;
> u32 reg;
>
> spin_lock_irqsave(&data->lock, flags);
>
> - reg = readl(data->membase + (bank * reg_width));
> - writel(reg | BIT(offset), data->membase + (bank * reg_width));
> + reg = readl(addr);
> + writel(reg | BIT(offset), addr);
> + /* Read back to make sure the write doesn't linger in a store buffer */
> + readl(addr);
>
> spin_unlock_irqrestore(&data->lock, flags);
>
>
next prev parent reply other threads:[~2017-03-08 14:03 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-08 9:54 [PATCH 1/2] reset: add reset-simple to unify socfpga, stm32, and sunxi Philipp Zabel
2017-03-08 9:54 ` [PATCH 2/2] reset: simple: read back to make sure changes are applied Philipp Zabel
2017-03-08 14:04 ` Andre Przywara [this message]
2017-03-08 10:19 ` [PATCH 1/2] reset: add reset-simple to unify socfpga, stm32, and sunxi Andre Przywara
2017-03-08 11:05 ` Alexandre Torgue
2017-03-08 12:20 ` Philipp Zabel
2017-03-08 14:03 ` Andre Przywara
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=c046c1c1-7a14-5052-fbf1-d1efab6c9d9f@arm.com \
--to=andre.przywara@arm.com \
--cc=alexandre.torgue@st.com \
--cc=linux-kernel@vger.kernel.org \
--cc=maxime.ripard@free-electrons.com \
--cc=mcoquelin.stm32@gmail.com \
--cc=p.zabel@pengutronix.de \
--cc=wens@csie.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
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.