From: Philipp Zabel <p.zabel@pengutronix.de>
To: Florian Fainelli <f.fainelli@gmail.com>, linux-kernel@vger.kernel.org
Cc: Mark Rutland <mark.rutland@arm.com>,
Rob Herring <robh@kernel.org>, Jim Quinlan <im2101024@gmail.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>, Jim Quinlan <jim2101024@gmail.com>,
"maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE"
<bcm-kernel-feedback-list@broadcom.com>,
"moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE"
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 2/2] reset: Add Broadcom STB RESCAL reset controller
Date: Thu, 12 Dec 2019 11:01:04 +0100 [thread overview]
Message-ID: <9fd20d18a8773cd8886d9cb0424791b2f79b3e25.camel@pengutronix.de> (raw)
In-Reply-To: <469c7b73-b028-1691-d5f0-0ceb3007da1c@gmail.com>
On Wed, 2019-12-11 at 10:12 -0800, Florian Fainelli wrote:
>
> On 12/11/2019 1:48 AM, Philipp Zabel wrote:
> > > +#define BRCM_RESCAL_START 0
> > > +#define BRCM_RESCAL_START_BIT BIT(0)
> > > +#define BRCM_RESCAL_CTRL 4
> > > +#define BRCM_RESCAL_STATUS 8
> > > +#define BRCM_RESCAL_STATUS_BIT BIT(0)
> >
> > Is there any reason the start bit is indented but the status bit is not?
>
> This is a convention we have tried to adopt to denote the definition
> from a register word address/offset versus the definition for bits
> within that register word.
That's fine, consider indenting BRCM_RESCAL_STATUS_BIT as well, then.
[...]
> > > + reg = readl(base + BRCM_RESCAL_START);
> > > + writel(reg | BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START);
> > > + reg = readl(base + BRCM_RESCAL_START);
> > > + if (!(reg & BRCM_RESCAL_START_BIT)) {
> > > + dev_err(data->dev, "failed to start sata/pcie rescal\n");
Is this something that can actually happen?
[...]
> > > + reg = readl(base + BRCM_RESCAL_START);
> > > + writel(reg ^ BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START);
> >
> > Please use &= ~BRCM_RESCAL_START_BIT instead.
>
> I think the idea was to avoid unconditionally clearing it, but based on
> the documentation, I don't see this being harmful, Jim?
Unless the bit is self-clearing, I can't see how this XOR could ever set
the bit instead of clearing it.
And even if it would, I don't understand how that can be indented.
Wouldn't that restart the reset/calibration sequence?
> > > + reg = readl(base + BRCM_RESCAL_START);
> > > + dev_dbg(data->dev, "sata/pcie rescal success\n");
> > > +
> > > + return 0;
> > > +}
> >
> > This whole function looks a lot like it doesn't just deassert a reset
> > line, but actually issues a complete reset procedure of some kind. Do
> > you have some insight on what actually happens in the hardware when the
> > start bit is triggered? I suspect this should be implemented with the
> > .reset operation.
>
> This hardware block is controlling the reset and calibration process of
> the SATA/PCIe combo PHY analog front end, but is not technically part of
> the PCIe or SATA PHY proper, it stands on its own, both functionally and
> from a register space perspective. The motivation for modelling this as
> a reset controller is that it does a reset (and a calibration) and this
> is a shared reset line among 2/3 instances of another block. If you
> think we should model this differently, please let us know.
Thank you for the explanation. I agree the "reset and calibration
sequence" property is close enough to a pure reset sequence to warrant
describing this as as reset controller.
The correct way would be to use the .reset callback though, if you can
have the drivers use reset_control_reset().
regards
Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-12-12 10:01 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-10 19:59 [PATCH 0/2] reset: Add Broadcom STB RESCAL reset controller Florian Fainelli
2019-12-10 19:59 ` [PATCH 1/2] dt-bindings: reset: Document BCM7216 " Florian Fainelli
2019-12-19 20:40 ` Rob Herring
2019-12-19 20:52 ` Florian Fainelli
2019-12-10 19:59 ` [PATCH 2/2] reset: Add Broadcom STB " Florian Fainelli
2019-12-11 9:48 ` Philipp Zabel
2019-12-11 18:12 ` Florian Fainelli
2019-12-12 10:01 ` Philipp Zabel [this message]
2019-12-30 19:05 ` Florian Fainelli
2020-01-02 11:23 ` Philipp Zabel
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=9fd20d18a8773cd8886d9cb0424791b2f79b3e25.camel@pengutronix.de \
--to=p.zabel@pengutronix.de \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=devicetree@vger.kernel.org \
--cc=f.fainelli@gmail.com \
--cc=im2101024@gmail.com \
--cc=jim2101024@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=robh@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
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).