Re: [PATCH v4 2/3] dt-bindings: reset: imx7: Document usage on i.MX8MQ SoCs
On Thu, 2019-01-17 at 14:38 -0800, Andrey Smirnov wrote: [...] > > To be honest, I don't like these two, I'm not convinced anymore that > > they actually qualify as reset signals. To me it looks like this is > > something that the PCIe glue code should handle via syscon like i.MX6. > > Leonard, Lucas, what do you think? > > OK, one thing to keep in mind about this is that those bits are > already exposed for i.MX7D and I think (correct me if I am wrong) > there's no going back there. That's not a reason to repeat the same mistake for i.MX8QM, but at the moment I'm still trying to figure out if it actually was a mistake. > PCIe driver already has the code to use > those on i.MX7D and, due to high degree of similarity, i.MX8MQ > actually re-uses the same codepath (at least for > IMX8MQ_RESET_PCIE_CTRL_APPS_EN). We can always switch to i.MX6-like direct syscon/GPR manipulation and just drop the resets from DT. Since if this is done, it should be done for i.MX7 as well, I see no reason for this issue to hold up your i.MX8M changes. regards Philipp
Re: [PATCH v5 3/3] reset: imx7: Add support for i.MX8MQ IP block variant
Hi Andrey,
On Mon, 2019-01-21 at 18:10 -0800, Andrey Smirnov wrote:
> Add bits and pieces needed to support IP block variant found on
> i.MX8MQ SoCs.
>
> Cc: p.za...@pengutronix.de
> Cc: Fabio Estevam
> Cc: cphe...@gmail.com
> Cc: l.st...@pengutronix.de
> Cc: Leonard Crestez
> Cc: "A.s. Dong"
> Cc: Richard Zhu
> Cc: Rob Herring
> Cc: devicet...@vger.kernel.org
> Cc: linux-...@nxp.com
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov
[...]
> +static int imx8mq_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
^
> +{
> + return imx8mq_reset_set(rcdev, id, true);
> +}
> +
> +static int imx8mq_reset_deassert(struct reset_controller_dev *rcdev,
> +unsigned long id)
^
> +{
> + return imx8mq_reset_set(rcdev, id, false);
> +}
Thank you, applied all three to reset/next with a small whitespace
alignment fix.
regards
Philipp
Re: [PATCH reset-next 0/2] reset: brcmstb: Misc fixes
On Tue, 2019-01-22 at 16:33 -0800, Florian Fainelli wrote:
> Hi Philipp,
>
> These two patches fix some recent issues brought up by Paul and Randy,
> feel free to squash into c196cdc7659d ("reset: Add Broadcom STB SW_INIT
> reset controller driver") since this is only in reset/next and
> linux-next so far.
If you want this squashed, feel free to submit an updated version of the
original patches and I'll replace them. That way I don't have to decide
how to stitch (or drop) the commit messages.
thanks
Philipp
Re: [PATCH reset-next 2/2] reset: brcmstb: Fix 32-bit build with 64-bit resource_size_t
Hi Florian,
On Tue, 2019-01-22 at 16:33 -0800, Florian Fainelli wrote:
> On 32-bit architectures defining resource_size_t as 64-bit (because of
> PAE), we can run into a linker failure because of the modulo and the
> division against resource_size(), replace the two problematic operations
> with an alignment check on the register resource (instead of modulo),
> and the division with DIV_ROUND_CLOSEST_ULL().
>
> Reported-by: Randy Dunlap
> Fixes: c196cdc7659d ("reset: Add Broadcom STB SW_INIT reset controller
> driver")
> Signed-off-by: Florian Fainelli
> ---
> drivers/reset/reset-brcmstb.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/reset/reset-brcmstb.c b/drivers/reset/reset-brcmstb.c
> index 01ab1f71518b..c4cab8b5052d 100644
> --- a/drivers/reset/reset-brcmstb.c
> +++ b/drivers/reset/reset-brcmstb.c
> @@ -91,7 +91,8 @@ static int brcmstb_reset_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (resource_size(res) % SW_INIT_BANK_SIZE) {
> + if (!IS_ALIGNED(res->start, SW_INIT_BANK_SIZE) ||
> + !IS_AGLINED(resource_size(res), SW_INIT_BANK_SIZE)) {
Typo.
> dev_err(kdev, "incorrect register range\n");
> return -EINVAL;
> }
> @@ -103,7 +104,8 @@ static int brcmstb_reset_probe(struct platform_device
> *pdev)
> dev_set_drvdata(kdev, priv);
>
> priv->rcdev.owner = THIS_MODULE;
> - priv->rcdev.nr_resets = (resource_size(res) / SW_INIT_BANK_SIZE) * 32;
> + priv->rcdev.nr_resets = DIV_ROUND_CLOSEST_ULL(resource_size(res),
> + SW_INIT_BANK_SIZE) * 32;
This should be DIV_ROUND_DOWN_ULL.
regards
Philipp
Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel
On Mon, 2019-01-21 at 10:46 -0800, Steve Longerbeam wrote: > > On 1/21/19 10:43 AM, Steve Longerbeam wrote: > > > > > > On 1/21/19 3:49 AM, Philipp Zabel wrote: > > > Also ipu_smfc_disable is refcounted, so if the other CSI is capturing > > > simultaneously, this change has no effect. > > > > Sigh, you're right. Let me go back to disabling the CSI before the > > channel, the CSI enable/disable is not refcounted (it doesn't need to > > be since it is single use) so it doesn't have this problem. > > > > Should we drop this patch or keep it (with a big comment)? By only > > changing the disable order to "CSI then channel", the hang is reliably > > fixed from my and Gael's testing, but my concern is that by not > > disabling the SMFC before the channel, the SMFC could still empty its > > FIFO to the channel's internal FIFO and still create a hang. > > Well, as you said it will have no effect if both CSI's are streaming > with the SMFC, in which case the danger would still exist. Perhaps it > would be best to just drop this patch. Hm, if we can't guarantee the intended effect with this patch, and stopping the CSI first helps reliably, it's indeed better to just do that instead. regards Philipp
Re: [PATCH 4/4] media: imx: Don't register IPU subdevs/links if CSI port missing
On Sat, 2019-01-19 at 13:46 -0800, Steve Longerbeam wrote:
> The second IPU internal sub-devices were being registered and links
> to them created even when the second IPU is not present. This is wrong
> for i.MX6 S/DL and i.MX53 which have only a single IPU.
>
> Fixes: e130291212df5 ("[media] media: Add i.MX media core driver")
>
> Signed-off-by: Steve Longerbeam
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [PATCH 3/4] media: imx: Rename functions that add IPU-internal subdevs/links
On Sat, 2019-01-19 at 13:45 -0800, Steve Longerbeam wrote: > For the functions that add and remove the internal IPU subdevice > descriptors and links between them, rename them to make clear they > are the subdevs and links internal to the IPU. Also rename the > platform data structure for the internal IPU subdevices. > No functional changes. > > Signed-off-by: Steve Longerbeam Acked-by: Philipp Zabel regards Philipp
Re: [PATCH 1/4] media: imx: csi: Allow unknown nearest upstream entities
On Sat, 2019-01-19 at 13:45 -0800, Steve Longerbeam wrote:
> On i.MX6, the nearest upstream entity to the CSI can only be the
> CSI video muxes or the Synopsys DW MIPI CSI-2 receiver.
>
> However the i.MX53 has no CSI video muxes or a MIPI CSI-2 receiver.
> So allow for the nearest upstream entity to the CSI to be something
> other than those.
>
> Fixes: bf3cfaa712e5c ("media: staging/imx: get CSI bus type from nearest
> upstream entity")
>
> Signed-off-by: Steve Longerbeam
> Cc: sta...@vger.kernel.org
> ---
> drivers/staging/media/imx/imx-media-csi.c | 18 ++
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 555aa45e02e3..b9af7d3d4974 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -154,9 +154,10 @@ static inline bool requires_passthrough(struct
> v4l2_fwnode_endpoint *ep,
> /*
> * Parses the fwnode endpoint from the source pad of the entity
> * connected to this CSI. This will either be the entity directly
> - * upstream from the CSI-2 receiver, or directly upstream from the
> - * video mux. The endpoint is needed to determine the bus type and
> - * bus config coming into the CSI.
> + * upstream from the CSI-2 receiver, directly upstream from the
> + * video mux, or directly upstream from the CSI itself. The endpoint
> + * is needed to determine the bus type and bus config coming into
> + * the CSI.
> */
> static int csi_get_upstream_endpoint(struct csi_priv *priv,
>struct v4l2_fwnode_endpoint *ep)
> @@ -172,7 +173,8 @@ static int csi_get_upstream_endpoint(struct csi_priv
> *priv,
> if (!priv->src_sd)
> return -EPIPE;
>
> - src = >src_sd->entity;
> + sd = priv->src_sd;
> + src = >entity;
>
> if (src->function == MEDIA_ENT_F_VID_MUX) {
> /*
> @@ -186,6 +188,14 @@ static int csi_get_upstream_endpoint(struct csi_priv
> *priv,
> src = >entity;
> }
>
> + /*
> + * If the source is neither the video mux nor the CSI-2 receiver,
> + * get the source pad directly upstream from CSI itself.
> + */
> + if (src->function != MEDIA_ENT_F_VID_MUX &&
Will it work correctly if there's an external MUX connected to the CSI?
> + sd->grp_id != IMX_MEDIA_GRP_ID_CSI2)
> + src = >sd.entity;
> +
> /* get source pad of entity directly upstream from src */
> pad = imx_media_find_upstream_pad(priv->md, src, 0);
> if (IS_ERR(pad))
regards
Philipp
Re: [PATCH v2 2/2] reset: Add Broadcom STB SW_INIT reset controller driver
Hi Scott, On Wed, 2019-01-16 at 10:15 -0800, Scott Branden wrote: > On 2019-01-15 10:44 a.m., Florian Fainelli wrote: > > Add support for resetting blocks through the Linux reset controller > > subsystem when reset lines are provided through a SW_INIT-style reset > > controller on Broadcom STB SoCs. > > > > Signed-off-by: Florian Fainelli > > --- > > drivers/reset/Kconfig | 7 ++ > > drivers/reset/Makefile| 1 + > > drivers/reset/reset-brcmstb.c | 130 ++ > > 3 files changed, 138 insertions(+) > > create mode 100644 drivers/reset/reset-brcmstb.c > > > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > > index 2e01bd833ffd..1ca03c57e049 100644 > > --- a/drivers/reset/Kconfig > > +++ b/drivers/reset/Kconfig > > @@ -40,6 +40,13 @@ config RESET_BERLIN > > help > > This enables the reset controller driver for Marvell Berlin SoCs. > > > > +config RESET_BRCMSTB > > + bool "Broadcom STB reset controller" if COMPILE_TEST > > Could this even be: > > depends on ARCH_BRCMSTB || COMPILE_TEST I don't think this is necessary. Since the symbol is non-interactive if COMPILE_TEST is disabled, it just vanishes when both are disabled. > > + default ARCH_BRCMSTB And if it is actually needed, it defaults to the correct value. regards Philipp
Re: [PATCH v2 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
Hi Florian, On Tue, 2019-01-15 at 10:44 -0800, Florian Fainelli wrote: > Add a binding document for the Broadcom STB reset controller, also known > as SW_INIT-style reset controller. > > Signed-off-by: Florian Fainelli Thank you, both applied to reset/next. regards Philipp
Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel
Hi,
On Fri, 2019-01-18 at 17:04 -0800, Steve Longerbeam wrote:
> Disable the SMFC before disabling the IDMA channel, instead of after,
> in csi_idmac_unsetup().
>
> This fixes a complete system hard lockup on the SabreAuto when streaming
> from the ADV7180, by repeatedly sending a stream off immediately followed
> by stream on:
>
> while true; do v4l2-ctl -d4 --stream-mmap --stream-count=3; done
>
> Eventually this either causes the system lockup or EOF timeouts at all
> subsequent stream on, until a system reset.
>
> The lockup occurs when disabling the IDMA channel at stream off. Stopping
> the video data stream entering the IDMA channel before disabling the
> channel itself appears to be a reliable fix for the hard lockup. That can
> be done either by disabling the SMFC or the CSI before disabling the
> channel. Disabling the SMFC before the channel is the easiest solution,
> so do that.
>
> Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
>
> Suggested-by: Peter Seiderer
> Reported-by: Gaël PORTAY
> Signed-off-by: Steve Longerbeam
Gaël, could we get a Tested-by: for this as well?
> Cc: sta...@vger.kernel.org
> ---
> Changes in v3:
> - switch from disabling the CSI before the channel to disabling the
> SMFC before the channel.
> Changes in v2:
> - restore an empty line
> - Add Fixes: and Cc: stable
> ---
> drivers/staging/media/imx/imx-media-csi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index e18f58f56dfb..8610027eac97 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -560,8 +560,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
> static void csi_idmac_unsetup(struct csi_priv *priv,
> enum vb2_buffer_state state)
> {
> - ipu_idmac_disable_channel(priv->idmac_ch);
> ipu_smfc_disable(priv->smfc);
> + ipu_idmac_disable_channel(priv->idmac_ch);
Steve, do you have any theory why this helps? It's a bit weird to
disable the SMFC module while the DMA channel is still enabled. I think
this warrants a big comment, given that enable order is SMFC_EN before
IDMAC channel enable.
Also ipu_smfc_disable is refcounted, so if the other CSI is capturing
simultaneously, this change has no effect.
FWIW, I don't see any regressions though.
regards
Philipp
Re: linux-next: build failure after merge of the imx-drm tree
On Fri, 2019-01-18 at 14:19 +1100, Stephen Rothwell wrote:
> Hi Lucas,
>
> After merging the imx-drm tree, today's linux-next build (powerpc
> allyesconfig) failed like this:
>
> drivers/gpu/drm/imx/imx-tve.c: At top level:
> drivers/gpu/drm/imx/imx-tve.c:437:29: error: storage size of 'clk_tve_di_ops'
> isn't known
> static const struct clk_ops clk_tve_di_ops = {
> ^~
>
> Exposed by commit
>
> 97144d12df00 ("drm/imx: Allow building under COMPILE_TEST")
>
> I have reverted that commit for today.
Sorry, I have dropped the commit for now.
regards
Philipp
Re: [PATCH v2 1/2] media: imx: csi: Disable CSI immediately after last EOF
On Thu, 2019-01-17 at 12:49 -0800, Steve Longerbeam wrote:
> Disable the CSI immediately after receiving the last EOF before stream
> off (and thus before disabling the IDMA channel).
>
> This fixes a complete system hard lockup on the SabreAuto when streaming
> from the ADV7180, by repeatedly sending a stream off immediately followed
> by stream on:
>
> while true; do v4l2-ctl -d4 --stream-mmap --stream-count=3; done
>
> Eventually this either causes the system lockup or EOF timeouts at all
> subsequent stream on, until a system reset.
>
> The lockup occurs when disabling the IDMA channel at stream off. Disabling
> the CSI before disabling the IDMA channel appears to be a reliable fix for
> the hard lockup.
>
> Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
>
> Reported-by: Gaël PORTAY
> Signed-off-by: Steve Longerbeam
> Cc: sta...@vger.kernel.org
> ---
> Changes in v2:
> - restore an empty line
> - Add Fixes: and Cc: stable
> ---
> drivers/staging/media/imx/imx-media-csi.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index e18f58f56dfb..e0f6f88e2e70 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -681,6 +681,8 @@ static void csi_idmac_stop(struct csi_priv *priv)
> if (ret == 0)
> v4l2_warn(>sd, "wait last EOF timeout\n");
>
> + ipu_csi_disable(priv->csi);
> +
Can you add a short comment why this call is here? Since now
csi_idmac_stop is kind of a misnomer and symmetry with csi(_idmac)_start
is broken, I think this is a bit un-obvious.
Also note that now the error path of csi_start() will now call
ipu_csi_disable() while the CSI is disabled. This happens to work
because that just calls ipu_module_disable(), which is not refcounted.
> devm_free_irq(priv->dev, priv->eof_irq, priv);
> devm_free_irq(priv->dev, priv->nfb4eof_irq, priv);
>
> @@ -793,9 +795,9 @@ static void csi_stop(struct csi_priv *priv)
> /* stop the frame interval monitor */
> if (priv->fim)
> imx_media_fim_set_stream(priv->fim, NULL, false);
> + } else {
> + ipu_csi_disable(priv->csi);
> }
> -
> - ipu_csi_disable(priv->csi);
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [PATCH v4 1/3] reset: imx7: Add plubming to support multiple IP variants
On Wed, 2018-12-19 at 17:06 -0800, Andrey Smirnov wrote:
> In order to enable supporting i.MX8MQ with this driver, convert it to
> expect variant specific bits to be passed via driver data.
>
> Cc: p.za...@pengutronix.de
> Cc: Fabio Estevam
> Cc: cphe...@gmail.com
> Cc: l.st...@pengutronix.de
> Cc: Leonard Crestez
> Cc: "A.s. Dong"
> Cc: Richard Zhu
> Cc: Rob Herring
> Cc: devicet...@vger.kernel.org
> Cc: linux-...@nxp.com
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov
> ---
> drivers/reset/reset-imx7.c | 62 +++---
> 1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 77911fa8f31d..3a36d5863891 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -17,14 +17,29 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
>
> +struct imx7_src_signal {
> + unsigned int offset, bit;
> +};
> +
> +struct imx7_src;
> +
> +struct imx7_src_variant {
> + const struct imx7_src_signal *signals;
> + unsigned int signals_num;
> + unsigned int (*prepare)(struct imx7_src *imx7src, unsigned long id,
> + bool assert);
Instead of adding a function pointer indirection, I'd prefer separate
imx7_reset_ops and imx8m_reset_ops set by the variant, see below.
> +};
> +
> struct imx7_src {
> struct reset_controller_dev rcdev;
> struct regmap *regmap;
> + const struct imx7_src_variant *variant;
This could then replaced with a direct pointer to the respective signals
array.
> };
>
> enum imx7_src_registers {
> @@ -39,10 +54,6 @@ enum imx7_src_registers {
> SRC_DDRC_RCR= 0x1000,
> };
>
> -struct imx7_src_signal {
> - unsigned int offset, bit;
> -};
> -
> static const struct imx7_src_signal imx7_src_signals[IMX7_RESET_NUM] = {
> [IMX7_RESET_A7_CORE_POR_RESET0] = { SRC_A7RCR0, BIT(0) },
> [IMX7_RESET_A7_CORE_POR_RESET1] = { SRC_A7RCR0, BIT(1) },
> @@ -72,17 +83,11 @@ static const struct imx7_src_signal
> imx7_src_signals[IMX7_RESET_NUM] = {
> [IMX7_RESET_DDRC_CORE_RST] = { SRC_DDRC_RCR, BIT(1) },
> };
>
> -static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
> +static unsigned int
> +imx7_src_prepare(struct imx7_src *imx7src, unsigned long id, bool assert)
> {
> - return container_of(rcdev, struct imx7_src, rcdev);
> -}
> -
> -static int imx7_reset_set(struct reset_controller_dev *rcdev,
> - unsigned long id, bool assert)
> -{
> - struct imx7_src *imx7src = to_imx7_src(rcdev);
> - const struct imx7_src_signal *signal = _src_signals[id];
> - unsigned int value = assert ? signal->bit : 0;
> + const unsigned int bit = imx7src->variant->signals[id].bit;
> + unsigned int value = assert ? bit : 0;
>
> switch (id) {
> case IMX7_RESET_PCIEPHY:
> @@ -95,10 +100,32 @@ static int imx7_reset_set(struct reset_controller_dev
> *rcdev,
> break;
>
> case IMX7_RESET_PCIE_CTRL_APPS_EN:
> - value = (assert) ? 0 : signal->bit;
> + value = assert ? 0 : bit;
> break;
> }
>
> + return value;
> +}
Instead of having a common imx7_reset_set and then calling the custom
.prepare() through a function pointer, I'd suggest to have custom
imx7_reset_set and imx8m_reset_set functions that contain the code from
.prepare() and then call a common function to do the actual register
access.
> +
> +static const struct imx7_src_variant variant_imx7 = {
> + .signals = imx7_src_signals,
> + .signals_num = ARRAY_SIZE(imx7_src_signals),
> + .prepare = imx7_src_prepare,
> +};
> +
> +static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
> +{
> + return container_of(rcdev, struct imx7_src, rcdev);
> +}
> +
> +static int imx7_reset_set(struct reset_controller_dev *rcdev,
> + unsigned long id, bool assert)
> +{
> + struct imx7_src *imx7src = to_imx7_src(rcdev);
> + const struct imx7_src_variant *variant = imx7src->variant;
> + const struct imx7_src_signal *signal = >signals[id];
> + const unsigned int value = variant->prepare(imx7src, id, assert);
> +
> return regmap_update_bits(imx7src->regmap,
> signal->offset, signal->bit, value);
> }
> @@ -130,6 +157,7 @@ static int imx7_reset_probe(struct platform_device *pdev)
> if (!imx7src)
> return -ENOMEM;
>
> + imx7src->variant = of_device_get_match_data(dev);
> imx7src->regmap = syscon_node_to_regmap(dev->of_node);
> if (IS_ERR(imx7src->regmap)) {
> dev_err(dev, "Unable to get imx7-src regmap");
> @@ -138,7 +166,7 @@ static int imx7_reset_probe(struct platform_device *pdev)
> regmap_attach_dev(dev, imx7src->regmap, );
>
> imx7src->rcdev.owner
Re: [PATCH v4 2/3] dt-bindings: reset: imx7: Document usage on i.MX8MQ SoCs
Hi Andrey, sorry for the delay. Thank you for the update, apart from the comments below, the list now looks to be complete. On Wed, 2018-12-19 at 17:06 -0800, Andrey Smirnov wrote: > The driver now supports i.MX8MQ, so update bindings accordingly. > > Cc: p.za...@pengutronix.de > Cc: Fabio Estevam > Cc: cphe...@gmail.com > Cc: l.st...@pengutronix.de > Cc: Leonard Crestez > Cc: "A.s. Dong" > Cc: Richard Zhu > Cc: linux-...@nxp.com > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Reviewed-by: Rob Herring > Signed-off-by: Andrey Smirnov > --- > .../bindings/reset/fsl,imx7-src.txt | 7 +- > include/dt-bindings/reset/imx8mq-reset.h | 64 +++ > 2 files changed, 69 insertions(+), 2 deletions(-) > create mode 100644 include/dt-bindings/reset/imx8mq-reset.h > > diff --git a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt > b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt > index 1ab1d109318e..2ecf33815d18 100644 > --- a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt > +++ b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt > @@ -5,7 +5,9 @@ Please also refer to reset.txt in this directory for common > reset > controller binding usage. > > Required properties: > -- compatible: Should be "fsl,imx7d-src", "syscon" > +- compatible: > + - For i.MX7 SoCs should be "fsl,imx7d-src", "syscon" > + - For i.MX8MQ SoCs should be "fsl,imx8mq-src", "syscon" > - reg: should be register base and length as documented in the >datasheet > - interrupts: Should contain SRC interrupt > @@ -44,4 +46,5 @@ Example: > > > For list of all valid reset indicies see > - > + for i.MX7 and > + for i.MX8MQ > diff --git a/include/dt-bindings/reset/imx8mq-reset.h > b/include/dt-bindings/reset/imx8mq-reset.h > new file mode 100644 > index ..57c592498aa0 > --- /dev/null > +++ b/include/dt-bindings/reset/imx8mq-reset.h > @@ -0,0 +1,64 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2018 Zodiac Inflight Innovations > + * > + * Author: Andrey Smirnov > + */ > + > +#ifndef DT_BINDING_RESET_IMX8MQ_H > +#define DT_BINDING_RESET_IMX8MQ_H > + > +#define IMX8MQ_RESET_A53_CORE_POR_RESET0 0 > +#define IMX8MQ_RESET_A53_CORE_POR_RESET1 1 > +#define IMX8MQ_RESET_A53_CORE_POR_RESET2 2 > +#define IMX8MQ_RESET_A53_CORE_POR_RESET3 3 > +#define IMX8MQ_RESET_A53_CORE_RESET0 4 > +#define IMX8MQ_RESET_A53_CORE_RESET1 5 > +#define IMX8MQ_RESET_A53_CORE_RESET2 6 > +#define IMX8MQ_RESET_A53_CORE_RESET3 7 > +#define IMX8MQ_RESET_A53_DBG_RESET0 8 > +#define IMX8MQ_RESET_A53_DBG_RESET1 9 > +#define IMX8MQ_RESET_A53_DBG_RESET2 10 > +#define IMX8MQ_RESET_A53_DBG_RESET3 11 > +#define IMX8MQ_RESET_A53_ETM_RESET0 12 > +#define IMX8MQ_RESET_A53_ETM_RESET1 13 > +#define IMX8MQ_RESET_A53_ETM_RESET2 14 > +#define IMX8MQ_RESET_A53_ETM_RESET3 15 > +#define IMX8MQ_RESET_A53_SOC_DBG_RESET 16 > +#define IMX8MQ_RESET_A53_L2RESET 17 > +#define IMX8MQ_RESET_SW_NON_SCLR_M4C_RST 18 This might be an implementation detail. The reset line is (SW_?)M4C_RST. I suppose the self-clearing SW_M4C_RST bit could be used to implement .reset() functionality for the same line if needed. What about the self-clearing SW_M4P_RST bit? Has that been left out on purpose? > +#define IMX8MQ_RESET_OTG1_PHY_RESET 19 > +#define IMX8MQ_RESET_OTG2_PHY_RESET 20 > +#define IMX8MQ_RESET_MIPI_DSI_RESET_BYTE_N 21 > +#define IMX8MQ_RESET_MIPI_DSI_RESET_N22 > +#define IMX8MQ_RESET_MIPI_DIS_DPI_RESET_N23 > +#define IMX8MQ_RESET_MIPI_DIS_ESC_RESET_N24 > +#define IMX8MQ_RESET_MIPI_DIS_PCLK_RESET_N 25 > +#define IMX8MQ_RESET_PCIEPHY 26 > +#define IMX8MQ_RESET_PCIEPHY_PERST 27 > +#define IMX8MQ_RESET_PCIE_CTRL_APPS_EN 28 > +#define IMX8MQ_RESET_PCIE_CTRL_APPS_TURNOFF 29 To be honest, I don't like these two, I'm not convinced anymore that they actually qualify as reset signals. To me it looks like this is something that the PCIe glue code should handle via syscon like i.MX6. Leonard, Lucas, what do you think? > +#define IMX8MQ_RESET_HDMI_PHY_APB_RESET 30 > +#define IMX8MQ_RESET_DISP_RESET 31 > +#define IMX8MQ_RESET_GPU_RESET 32 > +#define IMX8MQ_RESET_VPU_RESET 33 > +#define IMX8MQ_RESET_PCIEPHY234 > +#define IMX8MQ_RESET_PCIEPHY2_PERST 35 > +#define IMX8MQ_RESET_PCIE2_CTRL_APPS_EN 36 > +#define IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF 37 Same issue as PCIe #1 > +#define IMX8MQ_RESET_MIPI_CSI1_CORE_RESET38 > +#define IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET 39 > +#define IMX8MQ_RESET_MIPI_CSI1_ESC_RESET 40 > +#define IMX8MQ_RESET_MIPI_CSI2_CORE_RESET41 > +#define
Re: [PATCH 2/4] drm/imx: imx-ldb: add missing of_node_puts
Hi Julia, On Sun, 2019-01-13 at 09:47 +0100, Julia Lawall wrote: > The device node iterators perform an of_node_get on each > iteration, so a jump out of the loop requires an of_node_put. > > Move the initialization channel->child = child; down to just > before the call to imx_ldb_register so that intervening failures > don't need to clear it. Add a label at the end of the function to > do all the of_node_puts. Thank you, I've applied the patch to the imx-drm/fixes branch. regards Philipp
Re: [PATCH] media: imx-csi: Input connections to CSI should be optional
On Wed, 2019-01-09 at 10:34 -0800, Steve Longerbeam wrote:
> Some imx platforms do not have fwnode connections to all CSI input
> ports, and should not be treated as an error. This includes the
> imx6q SabreAuto, which has no connections to ipu1_csi1 and ipu2_csi0.
> Return -ENOTCONN in imx_csi_parse_endpoint() so that v4l2-fwnode
> endpoint parsing will not treat an unconnected CSI input port as
> an error.
>
> Fixes: c893500a16baf ("media: imx: csi: Register a subdev notifier")
>
> Signed-off-by: Steve Longerbeam
Reviewed-by: Philipp Zabel
regards
Philipp
> ---
> drivers/staging/media/imx/imx-media-csi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 4223f8d418ae..30b1717982ae 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1787,7 +1787,7 @@ static int imx_csi_parse_endpoint(struct device *dev,
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd)
> {
> - return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
> + return fwnode_device_is_available(asd->match.fwnode) ? 0 : -ENOTCONN;
> }
>
> static int imx_csi_async_register(struct csi_priv *priv)
Re: [PATCH v6 12/12] media: imx.rst: Update doc to reflect fixes to interlaced capture
On Tue, 2019-01-08 at 16:15 -0800, Steve Longerbeam wrote: > Also add an example pipeline for unconverted capture with interweave > on SabreAuto. > > Cleanup some language in various places in the process. > > Signed-off-by: Steve Longerbeam Reviewed-by: Philipp Zabel regards Philipp
Re: [PATCH v6 05/12] media: imx-csi: Input connections to CSI should be optional
On Tue, 2019-01-08 at 16:15 -0800, Steve Longerbeam wrote:
> Some imx platforms do not have fwnode connections to all CSI input
> ports, and should not be treated as an error. This includes the
> imx6q SabreAuto, which has no connections to ipu1_csi1 and ipu2_csi0.
> Return -ENOTCONN in imx_csi_parse_endpoint() so that v4l2-fwnode
> endpoint parsing will not treat an unconnected endpoint as an error.
>
> Fixes: c893500a16baf ("media: imx: csi: Register a subdev notifier")
>
> Signed-off-by: Steve Longerbeam
> ---
> drivers/staging/media/imx/imx-media-csi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index e3a4f39dbf73..b276672cae1d 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1815,7 +1815,7 @@ static int imx_csi_parse_endpoint(struct device *dev,
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd)
> {
> - return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
> + return fwnode_device_is_available(asd->match.fwnode) ? 0 : -ENOTCONN;
> }
>
> static int imx_csi_async_register(struct csi_priv *priv)
Is this something that should be applied as a fix, separately from this
series?
regards
Philipp
Re: [PATCH v6 02/12] gpu: ipu-csi: Swap fields according to input/output field types
On Tue, 2019-01-08 at 16:15 -0800, Steve Longerbeam wrote: > The function ipu_csi_init_interface() was inverting the F-bit for > NTSC case, in the CCIR_CODE_1/2 registers. The result being that > for NTSC bottom-top field order, the CSI would swap fields and > capture in top-bottom order. > > Instead, base field swap on the field order of the input to the CSI, > and the field order of the requested output. If the input/output > fields are sequential but different, swap fields, otherwise do > not swap. This requires passing both the input and output mbus > frame formats to ipu_csi_init_interface(). > > Move this code to a new private function ipu_csi_set_bt_interlaced_codes() > that programs the CCIR_CODE_1/2 registers for interlaced BT.656 (and > possibly interlaced BT.1120 in the future). > > When detecting input video standard from the input frame width/height, > make sure to double height if input field type is alternate, since > in that case input height only includes lines for one field. > > Signed-off-by: Steve Longerbeam > Reviewed-by: Philipp Zabel Also Acked-by: Philipp Zabel to be merged via the media tree regards Philipp
[PATCH] MAINTAINERS: use include/linux/reset for reset controller related headers
The include/linux/reset directory currently contains one header with helper functions for Broadcom BCM63xx PMB, which can control reset lines to on-chip peripherals. Even though that driver doesn't use the reset controller framework, the the directory can be shared with other reset controller drivers that do. Signed-off-by: Philipp Zabel --- See for example https://patchwork.kernel.org/patch/10728595/ --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 32d76a90..826f7ea03f80 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12954,6 +12954,7 @@ F: drivers/reset/ F: Documentation/devicetree/bindings/reset/ F: include/dt-bindings/reset/ F: include/linux/reset.h +F: include/linux/reset/ F: include/linux/reset-controller.h RESTARTABLE SEQUENCES SUPPORT -- 2.20.1
Re: [PATCH 2/2] reset: Add Broadcom STB SW_INIT reset controller driver
Hi Florian,
On Thu, 2018-12-20 at 17:34 -0800, Florian Fainelli wrote:
> Add support for resetting blocks through the Linux reset controller
> subsystem when reset lines are provided through a SW_INIT-style reset
> controller on Broadcom STB SoCs.
>
> Signed-off-by: Florian Fainelli
Thank you, this looks mostly good to me. I just have a few small
nitpicks and I'm curious about the mdelays, see below.
> ---
> drivers/reset/Kconfig | 7 ++
> drivers/reset/Makefile| 1 +
> drivers/reset/reset-brcmstb.c | 121 ++
> 3 files changed, 129 insertions(+)
> create mode 100644 drivers/reset/reset-brcmstb.c
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 2e01bd833ffd..1ca03c57e049 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -40,6 +40,13 @@ config RESET_BERLIN
> help
> This enables the reset controller driver for Marvell Berlin SoCs.
>
> +config RESET_BRCMSTB
> + bool "Broadcom STB reset controller" if COMPILE_TEST
> + default ARCH_BRCMSTB
> + help
> + This enables the reset controller driver for Broadcom STB SoCs using
> + a SUN_TOP_CTRL_SW_INIT style controller.
> +
> config RESET_HSDK
> bool "Synopsys HSDK Reset Driver"
> depends on HAS_IOMEM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index dc7874df78d9..7395db2cb1dd 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
> obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
> obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
> obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
> +obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
> obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
> obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> diff --git a/drivers/reset/reset-brcmstb.c b/drivers/reset/reset-brcmstb.c
> new file mode 100644
> index ..17a0bcdd6c9a
> --- /dev/null
> +++ b/drivers/reset/reset-brcmstb.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Broadcom STB generic reset controller for SW_INIT style reset controller
> + *
> + * Copyright (C) 2018 Broadcom
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct brcmstb_reset {
> + void __iomem *base;
> + unsigned int n_words;
> + struct device *dev;
These two variables are not used anywhere.
> + struct reset_controller_dev rcdev;
> +};
> +
> +#define SW_INIT_SET 0x00
> +#define SW_INIT_CLEAR0x04
> +#define SW_INIT_STATUS 0x08
> +
> +#define SW_INIT_BIT(id) BIT((id) & 0x1f)
> +#define SW_INIT_BANK(id) (id >> 5)
Checkpatch suggests to use ((id) >> 5) here.
> +
> +#define SW_INIT_BANK_SIZE0x18
> +
> +static inline
> +struct brcmstb_reset *to_brcmstb(struct reset_controller_dev *rcdev)
> +{
> + return container_of(rcdev, struct brcmstb_reset, rcdev);
> +}
> +
> +static int brcmstb_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
> + struct brcmstb_reset *priv = to_brcmstb(rcdev);
> +
> + writel_relaxed(SW_INIT_BIT(id), priv->base + off + SW_INIT_SET);
> + msleep(10);
What is the purpose of the msleep(10)? Is it guaranteed that the writel
takes effect before the msleep, or could it be lingering in some store
buffer for (a part of) the duration? Also, checkpatch warns about this
being < 20 ms. You could increase the delay or use usleep_range.
> + return 0;
> +}
> +
> +static int brcmstb_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
> + struct brcmstb_reset *priv = to_brcmstb(rcdev);
> +
> + writel_relaxed(SW_INIT_BIT(id), priv->base + off + SW_INIT_CLEAR);
> + msleep(10);
Same as above, what has to be delayed for 10 ms after deasserting the
reset? Is this the same for all reset lines handled by this controller?
> +
> + return 0;
> +}
> +
> +static int brcmstb_reset_status(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
> + struct brcmstb_reset *priv = to_brcmstb(rcdev);
> +
> + return readl_relaxed(priv->base + off + SW_INIT_STATUS);
Should this be
+ return readl_relaxed(priv->base + off + SW_INIT_STATUS) &
+ SW_INIT_BANK(id);
i.e. do the SW_INIT_STATUS registers contain 32 status bits, one for
each reset line?
> +}
> +
> +static const struct reset_control_ops brcmstb_reset_ops = {
> + .assert = brcmstb_reset_assert,
> + .deassert = brcmstb_reset_deassert,
> + .status = brcmstb_reset_status,
> +};
> +
> +static int
Re: [PATCH v5 02/12] gpu: ipu-csi: Swap fields according to input/output field types
Hi Steve,
On Tue, 2018-10-16 at 17:00 -0700, Steve Longerbeam wrote:
> The function ipu_csi_init_interface() was inverting the F-bit for
> NTSC case, in the CCIR_CODE_1/2 registers. The result being that
> for NTSC bottom-top field order, the CSI would swap fields and
> capture in top-bottom order.
>
> Instead, base field swap on the field order of the input to the CSI,
> and the field order of the requested output. If the input/output
> fields are sequential but different, swap fields, otherwise do
> not swap. This requires passing both the input and output mbus
> frame formats to ipu_csi_init_interface().
>
> Move this code to a new private function ipu_csi_set_bt_interlaced_codes()
> that programs the CCIR_CODE_1/2 registers for interlaced BT.656 (and
> possibly interlaced BT.1120 in the future).
>
> When detecting input video standard from the input frame width/height,
> make sure to double height if input field type is alternate, since
> in that case input height only includes lines for one field.
>
> Signed-off-by: Steve Longerbeam
> ---
> Changes since v4:
> - Cleaned up some convoluted code in ipu_csi_init_interface(), suggested
> by Philipp Zabel.
> - Fixed a regression in csi_setup(), caught by Philipp.
> ---
> drivers/gpu/ipu-v3/ipu-csi.c | 119 +++---
> drivers/staging/media/imx/imx-media-csi.c | 17 +---
> include/video/imx-ipu-v3.h| 3 +-
> 3 files changed, 88 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-csi.c b/drivers/gpu/ipu-v3/ipu-csi.c
> index aa0e30a2ba18..4a15e513fa05 100644
> --- a/drivers/gpu/ipu-v3/ipu-csi.c
> +++ b/drivers/gpu/ipu-v3/ipu-csi.c
> @@ -325,6 +325,15 @@ static int mbus_code_to_bus_cfg(struct
> ipu_csi_bus_config *cfg, u32 mbus_code,
> return 0;
> }
>
> +/* translate alternate field mode based on given standard */
> +static inline enum v4l2_field
> +ipu_csi_translate_field(enum v4l2_field field, v4l2_std_id std)
> +{
> + return (field != V4L2_FIELD_ALTERNATE) ? field :
> + ((std & V4L2_STD_525_60) ?
> + V4L2_FIELD_SEQ_BT : V4L2_FIELD_SEQ_TB);
> +}
> +
> /*
> * Fill a CSI bus config struct from mbus_config and mbus_framefmt.
> */
> @@ -374,22 +383,75 @@ static int fill_csi_bus_cfg(struct ipu_csi_bus_config
> *csicfg,
> return 0;
> }
>
> +static int ipu_csi_set_bt_interlaced_codes(struct ipu_csi *csi,
> +struct v4l2_mbus_framefmt *infmt,
> +struct v4l2_mbus_framefmt *outfmt,
infmt and outfmt parameters could be const.
> +v4l2_std_id std)
> +{
> + enum v4l2_field infield, outfield;
> + bool swap_fields;
> +
> + /* get translated field type of input and output */
> + infield = ipu_csi_translate_field(infmt->field, std);
> + outfield = ipu_csi_translate_field(outfmt->field, std);
> +
> + /*
> + * Write the H-V-F codes the CSI will match against the
> + * incoming data for start/end of active and blanking
> + * field intervals. If input and output field types are
> + * sequential but not the same (one is SEQ_BT and the other
> + * is SEQ_TB), swap the F-bit so that the CSI will capture
> + * field 1 lines before field 0 lines.
> + */
> + swap_fields = (V4L2_FIELD_IS_SEQUENTIAL(infield) &&
> +V4L2_FIELD_IS_SEQUENTIAL(outfield) &&
> +infield != outfield);
> +
> + if (!swap_fields) {
> + /*
> + * Field0BlankEnd = 110, Field0BlankStart = 010
> + * Field0ActiveEnd = 100, Field0ActiveStart = 000
> + * Field1BlankEnd = 111, Field1BlankStart = 011
> + * Field1ActiveEnd = 101, Field1ActiveStart = 001
> + */
> + ipu_csi_write(csi, 0x40596 | CSI_CCIR_ERR_DET_EN,
> + CSI_CCIR_CODE_1);
> + ipu_csi_write(csi, 0xD07DF, CSI_CCIR_CODE_2);
> + } else {
> + dev_dbg(csi->ipu->dev, "capture field swap\n");
> +
> + /* same as above but with F-bit inverted */
> + ipu_csi_write(csi, 0xD07DF | CSI_CCIR_ERR_DET_EN,
> + CSI_CCIR_CODE_1);
> + ipu_csi_write(csi, 0x40596, CSI_CCIR_CODE_2);
> + }
> +
> + ipu_csi_write(csi, 0xFF, CSI_CCIR_CODE_3);
> +
> + return 0;
> +}
> +
> +
> int ipu_csi_init_interface(struct ipu_csi *csi,
> struct v4l2_mbus_config *mbus_cfg,
> -struct v4l2_mbus_framefmt *mbus_fmt)
> +
[PATCH 0/2] reset: socfpga, sunxi: declare _reset_init functions in header files
Hi, this moves the socpfga_reset_init function declaration into a header file that is included by both mach-socfpga and reset-socfpga code, as suggested by Stephen. The second patch does the same for sun6i_reset_init. Since the socfpga patch avoids a (trivial) merge conflict with the arm-soc tree, I'd like to apply this to the reset/next branch before sending a pull request. regards Philipp Philipp Zabel (2): reset: sunxi: declare sun6i_reset_init in a header file reset: socfpga: declare socfpga_reset_init in a header file arch/arm/mach-socfpga/socfpga.c | 3 +-- arch/arm/mach-sunxi/sunxi.c | 2 +- drivers/reset/reset-socfpga.c | 2 +- drivers/reset/reset-sunxi.c | 1 + include/linux/reset/socfpga.h | 7 +++ include/linux/reset/sunxi.h | 7 +++ 6 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 include/linux/reset/socfpga.h create mode 100644 include/linux/reset/sunxi.h -- 2.19.1
[PATCH 2/2] reset: socfpga: declare socfpga_reset_init in a header file
Avoid declaring extern functions in c files. To make sure function
definition and usage don't get out of sync, declare socfpga_reset_init
in a common header.
Suggested-by: Stephen Rothwell
Signed-off-by: Philipp Zabel
---
arch/arm/mach-socfpga/socfpga.c | 3 +--
drivers/reset/reset-socfpga.c | 2 +-
include/linux/reset/socfpga.h | 7 +++
3 files changed, 9 insertions(+), 3 deletions(-)
create mode 100644 include/linux/reset/socfpga.h
diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index cc64576c102b..685272fa9eb6 100644
--- a/arch/arm/mach-socfpga/socfpga.c
+++ b/arch/arm/mach-socfpga/socfpga.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -32,8 +33,6 @@ void __iomem *rst_manager_base_addr;
void __iomem *sdr_ctl_base_addr;
unsigned long socfpga_cpu1start_addr;
-extern void __init socfpga_reset_init(void);
-
void __init socfpga_sysmgr_init(void)
{
struct device_node *np;
diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
index 318cfc51c441..96953992c2bb 100644
--- a/drivers/reset/reset-socfpga.c
+++ b/drivers/reset/reset-socfpga.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -18,7 +19,6 @@
#include "reset-simple.h"
#define SOCFPGA_NR_BANKS 8
-void __init socfpga_reset_init(void);
static int a10_reset_init(struct device_node *np)
{
diff --git a/include/linux/reset/socfpga.h b/include/linux/reset/socfpga.h
new file mode 100644
index ..b11a2047c342
--- /dev/null
+++ b/include/linux/reset/socfpga.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_RESET_SOCFPGA_H__
+#define __LINUX_RESET_SOCFPGA_H__
+
+void __init socfpga_reset_init(void);
+
+#endif /* __LINUX_RESET_SOCFPGA_H__ */
--
2.19.1
[PATCH 1/2] reset: sunxi: declare sun6i_reset_init in a header file
Avoid declaring extern functions in c files. To make sure function
definition and usage don't get out of sync, declare sun6i_reset_init
in a common header.
Suggested-by: Stephen Rothwell
Signed-off-by: Philipp Zabel
---
arch/arm/mach-sunxi/sunxi.c | 2 +-
drivers/reset/reset-sunxi.c | 1 +
include/linux/reset/sunxi.h | 7 +++
3 files changed, 9 insertions(+), 1 deletion(-)
create mode 100644 include/linux/reset/sunxi.h
diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
index de4b0e932f22..88680e80c166 100644
--- a/arch/arm/mach-sunxi/sunxi.c
+++ b/arch/arm/mach-sunxi/sunxi.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
@@ -37,7 +38,6 @@ static const char * const sun6i_board_dt_compat[] = {
NULL,
};
-extern void __init sun6i_reset_init(void);
static void __init sun6i_timer_init(void)
{
of_clk_init(NULL);
diff --git a/drivers/reset/reset-sunxi.c b/drivers/reset/reset-sunxi.c
index db9a1a75523f..b06d724d8f21 100644
--- a/drivers/reset/reset-sunxi.c
+++ b/drivers/reset/reset-sunxi.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
#include
diff --git a/include/linux/reset/sunxi.h b/include/linux/reset/sunxi.h
new file mode 100644
index ..1ad7fffb413e
--- /dev/null
+++ b/include/linux/reset/sunxi.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_RESET_SUNXI_H__
+#define __LINUX_RESET_SUNXI_H__
+
+void __init sun6i_reset_init(void);
+
+#endif /* __LINUX_RESET_SUNXI_H__ */
--
2.19.1
Re: [PATCH v8 2/4] pps: descriptor-based gpio, capture-clear addition
Hi Tom,
On Sat, Nov 17, 2018 at 2:07 PM Tom Burkart wrote:
>
> This patch changes the GPIO access for the pps-gpio driver from the
> integer based ABI to the descriptor based ABI. It also adds the
> extraction of the device tree capture-clear option.
Is the capture-clear property documented in
Documentation/devicetree/bindings/pps/pps-gpio.txt?
If not, you should add a binding documentation patch.
> Signed-off-by: Tom Burkart
> ---
> drivers/pps/clients/pps-gpio.c | 80
> +-
> include/linux/pps-gpio.h | 3 +-
> 2 files changed, 41 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
> index 333ad7d5b45b..d2fbc91dc8fc 100644
> --- a/drivers/pps/clients/pps-gpio.c
> +++ b/drivers/pps/clients/pps-gpio.c
> @@ -31,7 +31,7 @@
> #include
> #include
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -41,9 +41,9 @@ struct pps_gpio_device_data {
> int irq;/* IRQ used as PPS source */
> struct pps_device *pps; /* PPS source device */
> struct pps_source_info info;/* PPS source information */
> + struct gpio_desc *gpio_pin; /* GPIO port descriptors */
> bool assert_falling_edge;
> bool capture_clear;
> - unsigned int gpio_pin;
> };
>
> /*
> @@ -61,18 +61,49 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void
> *data)
>
> info = data;
>
> - rising_edge = gpio_get_value(info->gpio_pin);
> + rising_edge = gpiod_get_value(info->gpio_pin);
> if ((rising_edge && !info->assert_falling_edge) ||
> (!rising_edge && info->assert_falling_edge))
> pps_event(info->pps, , PPS_CAPTUREASSERT, NULL);
> else if (info->capture_clear &&
> ((rising_edge && info->assert_falling_edge) ||
> -(!rising_edge && !info->assert_falling_edge)))
> + (!rising_edge && !info->assert_falling_edge)))
> pps_event(info->pps, , PPS_CAPTURECLEAR, NULL);
>
> return IRQ_HANDLED;
> }
>
> +static int pps_gpio_setup(struct platform_device *pdev)
> +{
> + struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
> + const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data;
> + struct device_node *np = pdev->dev.of_node;
> + int ret;
Unused variable?
> +
> + if (pdata) {
> + data->gpio_pin = pdata->gpio_pin;
> +
> + data->assert_falling_edge = pdata->assert_falling_edge;
> + data->capture_clear = pdata->capture_clear;
This is just a matter of personal taste, so feel free to ignore:
I would keep the pdata branch in pps_gpio_probe and call this function
pps_gpio_dt_setup, to reduce indentation of the OF branch.
> + } else {
> + data->gpio_pin = devm_gpiod_get(>dev,
> + NULL, /* request "gpios" */
> + GPIOD_IN);
> + if (IS_ERR(data->gpio_pin)) {
> + dev_err(>dev,
> + "failed to request PPS GPIO\n");
> + return PTR_ERR(data->gpio_pin);
> + }
> +
> + if (of_get_property(np, "assert-falling-edge", NULL))
> + data->assert_falling_edge = true;
> +
> + if (of_get_property(np, "capture-clear", NULL))
> + data->capture_clear = true;
Those two should use the of_property_read_bool wrapper.
> + }
> + return 0;
> +}
> +
> static unsigned long
> get_irqf_trigger_flags(const struct pps_gpio_device_data *data)
> {
> @@ -90,53 +121,23 @@ get_irqf_trigger_flags(const struct pps_gpio_device_data
> *data)
> static int pps_gpio_probe(struct platform_device *pdev)
> {
> struct pps_gpio_device_data *data;
> - const char *gpio_label;
> int ret;
> int pps_default_params;
> - const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data;
> - struct device_node *np = pdev->dev.of_node;
>
> /* allocate space for device info */
> data = devm_kzalloc(>dev, sizeof(struct pps_gpio_device_data),
Could use sizeof(*data) here. Otherwise,
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [PATCH v8 2/4] pps: descriptor-based gpio, capture-clear addition
Hi Tom,
On Sat, Nov 17, 2018 at 2:07 PM Tom Burkart wrote:
>
> This patch changes the GPIO access for the pps-gpio driver from the
> integer based ABI to the descriptor based ABI. It also adds the
> extraction of the device tree capture-clear option.
Is the capture-clear property documented in
Documentation/devicetree/bindings/pps/pps-gpio.txt?
If not, you should add a binding documentation patch.
> Signed-off-by: Tom Burkart
> ---
> drivers/pps/clients/pps-gpio.c | 80
> +-
> include/linux/pps-gpio.h | 3 +-
> 2 files changed, 41 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
> index 333ad7d5b45b..d2fbc91dc8fc 100644
> --- a/drivers/pps/clients/pps-gpio.c
> +++ b/drivers/pps/clients/pps-gpio.c
> @@ -31,7 +31,7 @@
> #include
> #include
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -41,9 +41,9 @@ struct pps_gpio_device_data {
> int irq;/* IRQ used as PPS source */
> struct pps_device *pps; /* PPS source device */
> struct pps_source_info info;/* PPS source information */
> + struct gpio_desc *gpio_pin; /* GPIO port descriptors */
> bool assert_falling_edge;
> bool capture_clear;
> - unsigned int gpio_pin;
> };
>
> /*
> @@ -61,18 +61,49 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void
> *data)
>
> info = data;
>
> - rising_edge = gpio_get_value(info->gpio_pin);
> + rising_edge = gpiod_get_value(info->gpio_pin);
> if ((rising_edge && !info->assert_falling_edge) ||
> (!rising_edge && info->assert_falling_edge))
> pps_event(info->pps, , PPS_CAPTUREASSERT, NULL);
> else if (info->capture_clear &&
> ((rising_edge && info->assert_falling_edge) ||
> -(!rising_edge && !info->assert_falling_edge)))
> + (!rising_edge && !info->assert_falling_edge)))
> pps_event(info->pps, , PPS_CAPTURECLEAR, NULL);
>
> return IRQ_HANDLED;
> }
>
> +static int pps_gpio_setup(struct platform_device *pdev)
> +{
> + struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
> + const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data;
> + struct device_node *np = pdev->dev.of_node;
> + int ret;
Unused variable?
> +
> + if (pdata) {
> + data->gpio_pin = pdata->gpio_pin;
> +
> + data->assert_falling_edge = pdata->assert_falling_edge;
> + data->capture_clear = pdata->capture_clear;
This is just a matter of personal taste, so feel free to ignore:
I would keep the pdata branch in pps_gpio_probe and call this function
pps_gpio_dt_setup, to reduce indentation of the OF branch.
> + } else {
> + data->gpio_pin = devm_gpiod_get(>dev,
> + NULL, /* request "gpios" */
> + GPIOD_IN);
> + if (IS_ERR(data->gpio_pin)) {
> + dev_err(>dev,
> + "failed to request PPS GPIO\n");
> + return PTR_ERR(data->gpio_pin);
> + }
> +
> + if (of_get_property(np, "assert-falling-edge", NULL))
> + data->assert_falling_edge = true;
> +
> + if (of_get_property(np, "capture-clear", NULL))
> + data->capture_clear = true;
Those two should use the of_property_read_bool wrapper.
> + }
> + return 0;
> +}
> +
> static unsigned long
> get_irqf_trigger_flags(const struct pps_gpio_device_data *data)
> {
> @@ -90,53 +121,23 @@ get_irqf_trigger_flags(const struct pps_gpio_device_data
> *data)
> static int pps_gpio_probe(struct platform_device *pdev)
> {
> struct pps_gpio_device_data *data;
> - const char *gpio_label;
> int ret;
> int pps_default_params;
> - const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data;
> - struct device_node *np = pdev->dev.of_node;
>
> /* allocate space for device info */
> data = devm_kzalloc(>dev, sizeof(struct pps_gpio_device_data),
Could use sizeof(*data) here. Otherwise,
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [PATCH 1/1] reset: imx7: Add support for i.MX8MQ
Hi Andrey,
thank you for the patch.
In general, sharing the lookup table with i.MX7 is fine iff it is a
strict superset. But I don't think that is the case (see below).
Even so, this will change if there ever is another i.MX7 or i.MX8M
variant that is also a superset of i.MX7, but not a superset of
i.MX8M. Also, just listing all resets in order of appearance would make
review a bit easier.
I don't like increasing IMX7_RESET_NUM though, i.MX8M should have its
own nr_resets.
On Sat, 2018-11-17 at 10:11 -0800, Andrey Smirnov wrote:
> SRC IP block used in i.MX8MQ is a superset of what is found in i.MX7D,
Is this true, though?
i.MX7 has SRC_ERCR at offset 0x14 and HSICPHY_RCR at offset 0x1c.
According to documentation, i.MX8M is missing those at least.
> so add all of the definitions necessary to support both.
>
> Cc: p.za...@pengutronix.de
> Cc: Fabio Estevam
> Cc: cphe...@gmail.com
> Cc: l.st...@pengutronix.de
> Cc: Leonard Crestez
> Cc: "A.s. Dong"
> Cc: Richard Zhu
> Cc: linux-...@nxp.com
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov
> ---
> drivers/reset/Kconfig | 2 +-
> drivers/reset/reset-imx7.c | 66 -
> include/dt-bindings/reset/imx7-reset.h | 15 +-
> include/dt-bindings/reset/imx8m-reset.h | 47 ++
> 4 files changed, 127 insertions(+), 3 deletions(-)
> create mode 100644 include/dt-bindings/reset/imx8m-reset.h
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index c21da9fe51ec..4909aab7401b 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -50,7 +50,7 @@ config RESET_HSDK
> config RESET_IMX7
> bool "i.MX7 Reset Driver" if COMPILE_TEST
> depends on HAS_IOMEM
> - default SOC_IMX7D
> + default SOC_IMX7D || SOC_IMX8MQ
> select MFD_SYSCON
> help
> This enables the reset controller driver for i.MX7 SoCs.
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 77911fa8f31d..dffad618f805 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -21,14 +21,16 @@
> #include
> #include
> #include
> +#include
>
> struct imx7_src {
> struct reset_controller_dev rcdev;
> struct regmap *regmap;
> };
>
> -enum imx7_src_registers {
> +enum imx_src_registers {
> SRC_A7RCR0 = 0x0004,
> + SRC_A53RCR0 = 0x0004,
> SRC_M4RCR = 0x000c,
> SRC_ERCR= 0x0014,
> SRC_HSICPHY_RCR = 0x001c,
> @@ -36,7 +38,9 @@ enum imx7_src_registers {
> SRC_USBOPHY2_RCR= 0x0024,
> SRC_MIPIPHY_RCR = 0x0028,
> SRC_PCIEPHY_RCR = 0x002c,
These aren't complete. I see at least SRC_HDMI_RCR, SRC_DISP_RCR,
SRC_GPU_RCR, and SRC_VPU_RCR missing.
> + SRC_PCIE2PHY_RCR= 0x0048,
And SRC_MIPIPHY1_RCR, SRC_MIPIPHY2_RCR. Please check the reference
manual and complete the list of registers that contain resets.
> SRC_DDRC_RCR= 0x1000,
> +
Whitespace.
> };
>
> struct imx7_src_signal {
> @@ -67,11 +71,67 @@ static const struct imx7_src_signal
> imx7_src_signals[IMX7_RESET_NUM] = {
> [IMX7_RESET_PCIEPHY]= { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) },
> [IMX7_RESET_PCIEPHY_PERST] = { SRC_PCIEPHY_RCR, BIT(3) },
> [IMX7_RESET_PCIE_CTRL_APPS_EN] = { SRC_PCIEPHY_RCR, BIT(6) },
> + [IMX7_RESET_PCIE_CTRL_APPS_CLK_REQ] = { SRC_PCIEPHY_RCR, BIT(4) },
Now I feel like CTRL_APPS_EN shouldn't have been added here either. But
while there I wasn't really sure about whether that is actually a valid
reset, I'm pretty sure that this one isn't.
I think that i.MX8M either needs a clock driver to control this bit and
the corresponding IOMUXC GPR bit to enable the PCIe refclk, or the PCIe
driver has to access this register via syscon. I really don't want to
see a clock enable signal described in the device tree as a reset.
> [IMX7_RESET_PCIE_CTRL_APPS_TURNOFF] = { SRC_PCIEPHY_RCR, BIT(11) },
> [IMX7_RESET_DDRC_PRST] = { SRC_DDRC_RCR, BIT(0) },
> [IMX7_RESET_DDRC_CORE_RST] = { SRC_DDRC_RCR, BIT(1) },
> +
> + [IMX8M_RESET_A53_CORE_POR_RESET2] = { SRC_A53RCR0, BIT(2) },
> + [IMX8M_RESET_A53_CORE_POR_RESET3] = { SRC_A53RCR0, BIT(3) },
> + [IMX8M_RESET_A53_CORE_RESET2] = { SRC_A53RCR0, BIT(6) },
> + [IMX8M_RESET_A53_CORE_RESET3] = { SRC_A53RCR0, BIT(7) },
Missing IMX8M_RESET_A53_DBG_RESET2,3.
> + [IMX8M_RESET_A53_ETM_RESET2] = { SRC_A53RCR0, BIT(14) },
> + [IMX8M_RESET_A53_ETM_RESET3] = { SRC_A53RCR0, BIT(15) },
Missing some reset registers here.
> + [IMX8M_RESET_PCIE2PHY] = { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) },
> + [IMX8M_RESET_PCIE2PHY_PERST] = { SRC_PCIE2PHY_RCR, BIT(3) },
> + [IMX8M_RESET_PCIE2_CTRL_APPS_EN] = { SRC_PCIE2PHY_RCR, BIT(6) },
> +
Re: [PATCH 1/1] reset: imx7: Add support for i.MX8MQ
Hi Andrey,
thank you for the patch.
In general, sharing the lookup table with i.MX7 is fine iff it is a
strict superset. But I don't think that is the case (see below).
Even so, this will change if there ever is another i.MX7 or i.MX8M
variant that is also a superset of i.MX7, but not a superset of
i.MX8M. Also, just listing all resets in order of appearance would make
review a bit easier.
I don't like increasing IMX7_RESET_NUM though, i.MX8M should have its
own nr_resets.
On Sat, 2018-11-17 at 10:11 -0800, Andrey Smirnov wrote:
> SRC IP block used in i.MX8MQ is a superset of what is found in i.MX7D,
Is this true, though?
i.MX7 has SRC_ERCR at offset 0x14 and HSICPHY_RCR at offset 0x1c.
According to documentation, i.MX8M is missing those at least.
> so add all of the definitions necessary to support both.
>
> Cc: p.za...@pengutronix.de
> Cc: Fabio Estevam
> Cc: cphe...@gmail.com
> Cc: l.st...@pengutronix.de
> Cc: Leonard Crestez
> Cc: "A.s. Dong"
> Cc: Richard Zhu
> Cc: linux-...@nxp.com
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov
> ---
> drivers/reset/Kconfig | 2 +-
> drivers/reset/reset-imx7.c | 66 -
> include/dt-bindings/reset/imx7-reset.h | 15 +-
> include/dt-bindings/reset/imx8m-reset.h | 47 ++
> 4 files changed, 127 insertions(+), 3 deletions(-)
> create mode 100644 include/dt-bindings/reset/imx8m-reset.h
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index c21da9fe51ec..4909aab7401b 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -50,7 +50,7 @@ config RESET_HSDK
> config RESET_IMX7
> bool "i.MX7 Reset Driver" if COMPILE_TEST
> depends on HAS_IOMEM
> - default SOC_IMX7D
> + default SOC_IMX7D || SOC_IMX8MQ
> select MFD_SYSCON
> help
> This enables the reset controller driver for i.MX7 SoCs.
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 77911fa8f31d..dffad618f805 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -21,14 +21,16 @@
> #include
> #include
> #include
> +#include
>
> struct imx7_src {
> struct reset_controller_dev rcdev;
> struct regmap *regmap;
> };
>
> -enum imx7_src_registers {
> +enum imx_src_registers {
> SRC_A7RCR0 = 0x0004,
> + SRC_A53RCR0 = 0x0004,
> SRC_M4RCR = 0x000c,
> SRC_ERCR= 0x0014,
> SRC_HSICPHY_RCR = 0x001c,
> @@ -36,7 +38,9 @@ enum imx7_src_registers {
> SRC_USBOPHY2_RCR= 0x0024,
> SRC_MIPIPHY_RCR = 0x0028,
> SRC_PCIEPHY_RCR = 0x002c,
These aren't complete. I see at least SRC_HDMI_RCR, SRC_DISP_RCR,
SRC_GPU_RCR, and SRC_VPU_RCR missing.
> + SRC_PCIE2PHY_RCR= 0x0048,
And SRC_MIPIPHY1_RCR, SRC_MIPIPHY2_RCR. Please check the reference
manual and complete the list of registers that contain resets.
> SRC_DDRC_RCR= 0x1000,
> +
Whitespace.
> };
>
> struct imx7_src_signal {
> @@ -67,11 +71,67 @@ static const struct imx7_src_signal
> imx7_src_signals[IMX7_RESET_NUM] = {
> [IMX7_RESET_PCIEPHY]= { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) },
> [IMX7_RESET_PCIEPHY_PERST] = { SRC_PCIEPHY_RCR, BIT(3) },
> [IMX7_RESET_PCIE_CTRL_APPS_EN] = { SRC_PCIEPHY_RCR, BIT(6) },
> + [IMX7_RESET_PCIE_CTRL_APPS_CLK_REQ] = { SRC_PCIEPHY_RCR, BIT(4) },
Now I feel like CTRL_APPS_EN shouldn't have been added here either. But
while there I wasn't really sure about whether that is actually a valid
reset, I'm pretty sure that this one isn't.
I think that i.MX8M either needs a clock driver to control this bit and
the corresponding IOMUXC GPR bit to enable the PCIe refclk, or the PCIe
driver has to access this register via syscon. I really don't want to
see a clock enable signal described in the device tree as a reset.
> [IMX7_RESET_PCIE_CTRL_APPS_TURNOFF] = { SRC_PCIEPHY_RCR, BIT(11) },
> [IMX7_RESET_DDRC_PRST] = { SRC_DDRC_RCR, BIT(0) },
> [IMX7_RESET_DDRC_CORE_RST] = { SRC_DDRC_RCR, BIT(1) },
> +
> + [IMX8M_RESET_A53_CORE_POR_RESET2] = { SRC_A53RCR0, BIT(2) },
> + [IMX8M_RESET_A53_CORE_POR_RESET3] = { SRC_A53RCR0, BIT(3) },
> + [IMX8M_RESET_A53_CORE_RESET2] = { SRC_A53RCR0, BIT(6) },
> + [IMX8M_RESET_A53_CORE_RESET3] = { SRC_A53RCR0, BIT(7) },
Missing IMX8M_RESET_A53_DBG_RESET2,3.
> + [IMX8M_RESET_A53_ETM_RESET2] = { SRC_A53RCR0, BIT(14) },
> + [IMX8M_RESET_A53_ETM_RESET3] = { SRC_A53RCR0, BIT(15) },
Missing some reset registers here.
> + [IMX8M_RESET_PCIE2PHY] = { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) },
> + [IMX8M_RESET_PCIE2PHY_PERST] = { SRC_PCIE2PHY_RCR, BIT(3) },
> + [IMX8M_RESET_PCIE2_CTRL_APPS_EN] = { SRC_PCIE2PHY_RCR, BIT(6) },
> +
Re: [PATCH 0/4] reset: uniphier: Rename from USB3 reset to glue reset and add AHCI reset support
On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote: > This series renames the reset control of core reset included in USB3 glue > layer with in the glue layer for generic peripherals to allow other devices > to use it. > > And this series adds support for the core reset included in AHCI glue layer. > > Kunihiko Hayashi (4): > dt-bindings: reset: uniphier: Replace the expression of USB3 with > generic peripherals > reset: uniphier-usb3: Rename to reset-uniphier-glue > dt-bindings: reset: uniphier: Add AHCI core reset description > reset: uniphier-glue: Add AHCI reset control support in glue layer Thank you, applied to reset/next. regards Philipp
Re: [PATCH 0/4] reset: uniphier: Rename from USB3 reset to glue reset and add AHCI reset support
On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote: > This series renames the reset control of core reset included in USB3 glue > layer with in the glue layer for generic peripherals to allow other devices > to use it. > > And this series adds support for the core reset included in AHCI glue layer. > > Kunihiko Hayashi (4): > dt-bindings: reset: uniphier: Replace the expression of USB3 with > generic peripherals > reset: uniphier-usb3: Rename to reset-uniphier-glue > dt-bindings: reset: uniphier: Add AHCI core reset description > reset: uniphier-glue: Add AHCI reset control support in glue layer Thank you, applied to reset/next. regards Philipp
Re: [PATCH v6 2/4] pps: descriptor-based gpio, capture-clear addition
Hi Tom, On Tue, Nov 13, 2018 at 5:08 AM Tom Burkart wrote: > > This patch changes the GPIO access for the pps-gpio driver from the > integer based API to the descriptor based API. It also adds > documentation for the device tree capture-clear option and > device tree capture-clear extraction. > > The change from integer based GPIO API to the descriptor based API > breaks backward compatibility for the devicetree. This is due to > the descriptor based API appending "-gpio" or "-gpios" (see > Documentation/gpio/base.txt.) Documentation/devicetree/bindings/gpio/gpio.txt says: "While a non-existent is considered valid for compatibility reasons (resolving to the "gpios" property), it is not allowed for new bindings." This is not a new binding, so there should be no reason to change it. gpiod_get() and friends support this by using the "gpios" property if they are passed a NULL con_id. of_find_gpio() in drivers/gpio/gpiolib-of.c contains the relevant code. regards Philipp
Re: [PATCH v6 2/4] pps: descriptor-based gpio, capture-clear addition
Hi Tom, On Tue, Nov 13, 2018 at 5:08 AM Tom Burkart wrote: > > This patch changes the GPIO access for the pps-gpio driver from the > integer based API to the descriptor based API. It also adds > documentation for the device tree capture-clear option and > device tree capture-clear extraction. > > The change from integer based GPIO API to the descriptor based API > breaks backward compatibility for the devicetree. This is due to > the descriptor based API appending "-gpio" or "-gpios" (see > Documentation/gpio/base.txt.) Documentation/devicetree/bindings/gpio/gpio.txt says: "While a non-existent is considered valid for compatibility reasons (resolving to the "gpios" property), it is not allowed for new bindings." This is not a new binding, so there should be no reason to change it. gpiod_get() and friends support this by using the "gpios" property if they are passed a NULL con_id. of_find_gpio() in drivers/gpio/gpiolib-of.c contains the relevant code. regards Philipp
Re: [PATCHv4] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Tue, 2018-11-13 at 12:50 -0600, Dinh Nguyen wrote:
> @@ -120,7 +120,8 @@ static const struct reset_simple_devdata
> reset_simple_active_low = {
> };
>
> static const struct of_device_id reset_simple_dt_ids[] = {
> - { .compatible = "altr,rst-mgr", .data = _simple_socfpga },
> + { .compatible = "altr,stratix10-rst-mgr", "altr,rst-mgr",
This should be
+ { .compatible = "altr,stratix10-rst-mgr",
instead. The rest looks good to me. If you are ok with this change, I
can fix it up when applying.
regards
Philipp
Re: [PATCHv4] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Tue, 2018-11-13 at 12:50 -0600, Dinh Nguyen wrote:
> @@ -120,7 +120,8 @@ static const struct reset_simple_devdata
> reset_simple_active_low = {
> };
>
> static const struct of_device_id reset_simple_dt_ids[] = {
> - { .compatible = "altr,rst-mgr", .data = _simple_socfpga },
> + { .compatible = "altr,stratix10-rst-mgr", "altr,rst-mgr",
This should be
+ { .compatible = "altr,stratix10-rst-mgr",
instead. The rest looks good to me. If you are ok with this change, I
can fix it up when applying.
regards
Philipp
Re: [PATCH][next] reset: fix null pointer dereference on dev by dev_name
Hi Colin,
On Wed, 2018-11-14 at 21:49 +, Colin King wrote:
> From: Colin Ian King
>
> The call to dev_name will dereference dev, however, dev is later
> being null checked, so there is a possibility of a null pointer
> dereference on dev by the call to dev_name. Fix this by null
> checking dev first before the call to dev_name
>
> Detected by CoverityScan, CID#1475475 ("Dereference before null check")
>
> Fixes: 2a6cb2b1d83b ("reset: Add reset_control_get_count()")
> Signed-off-by: Colin Ian King
Thank you, applied to reset/next.
regards
Philipp
Re: [PATCH][next] reset: fix null pointer dereference on dev by dev_name
Hi Colin,
On Wed, 2018-11-14 at 21:49 +, Colin King wrote:
> From: Colin Ian King
>
> The call to dev_name will dereference dev, however, dev is later
> being null checked, so there is a possibility of a null pointer
> dereference on dev by the call to dev_name. Fix this by null
> checking dev first before the call to dev_name
>
> Detected by CoverityScan, CID#1475475 ("Dereference before null check")
>
> Fixes: 2a6cb2b1d83b ("reset: Add reset_control_get_count()")
> Signed-off-by: Colin Ian King
Thank you, applied to reset/next.
regards
Philipp
Re: [PATCH 1/3] arm64: dts: stratix10: use "altr,stratix10-rst-mgr" binding
Hi Dinh,
On Mon, 2018-11-05 at 14:05 -0600, Dinh Nguyen wrote:
> From: Dinh Nguyen
>
> The standard reset-simple driver the uses the "altr,rst-mgr" binding is
> not getting initialized early enough in the boot process, so timers
> that the kernel needs are still left in reset. Thus an early
> reset driver was created. This early reset driver is only for the
> SoCFPGA 32-bit platform.
>
> The Stratix10 platform does not need any of the timers that in reset to
> boot, thus we don't need to early reset driver. Therefore, use the
> "altr,stratix10-rst-mgr" binding for the reset-simple platform driver on
> the Stratix10 platform.
>
> Also remove the "altr,modrst-offset" property because the driver no
> longer needs it.
>
> Signed-off-by: Dinh Nguyen
> ---
> arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> index 8253a1a9e985..5f0b18ae5007 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> @@ -308,9 +308,8 @@
>
> rst: rstmgr@ffd11000 {
> #reset-cells = <1>;
> - compatible = "altr,rst-mgr";
> + compatible = "altr,stratix10-rst-mgr", "altr,rst-mgr";
> reg = <0xffd11000 0x1000>;
> - altr,modrst-offset = <0x20>;
> };
>
> spi0: spi@ffda4000 {
Do you want me to pick this up as well, or just patches 2 and 3?
regards
Philipp
Re: [PATCH 3/3] ARM: socfpga: dts: document "altr,stratix10-rst-mgr" binding
Hi Dinh, On Mon, 2018-11-05 at 14:05 -0600, Dinh Nguyen wrote: > "altr,stratix10-rst-mgr" is used for the Stratix10 reset manager. > > Signed-off-by: Dinh Nguyen > --- > Documentation/devicetree/bindings/reset/socfpga-reset.txt | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/reset/socfpga-reset.txt > b/Documentation/devicetree/bindings/reset/socfpga-reset.txt > index 98c9f560e5c5..38fe34fd8b8a 100644 > --- a/Documentation/devicetree/bindings/reset/socfpga-reset.txt > +++ b/Documentation/devicetree/bindings/reset/socfpga-reset.txt > @@ -1,7 +1,8 @@ > Altera SOCFPGA Reset Manager > > Required properties: > -- compatible : "altr,rst-mgr" > +- compatible : "altr,rst-mgr" for (Cyclone5/Arria5/Arria10) > +"altr,stratix10-rst-mgr","altr,rst-mgr" for Stratix10 ARM64 SoC git grep '\(altr\|intel\),stratix10' currently only shows "intel,stratix10-clkmgr". Should this be "intel,stratix10-rst-mgr"? I think keeping "altr," is fine for consistency, just wanted to point it out. > - reg : Should contain 1 register ranges(address and length) > - altr,modrst-offset : Should contain the offset of the first modrst > register. > - #reset-cells: 1 regards Philipp
Re: [PATCH 1/3] arm64: dts: stratix10: use "altr,stratix10-rst-mgr" binding
Hi Dinh,
On Mon, 2018-11-05 at 14:05 -0600, Dinh Nguyen wrote:
> From: Dinh Nguyen
>
> The standard reset-simple driver the uses the "altr,rst-mgr" binding is
> not getting initialized early enough in the boot process, so timers
> that the kernel needs are still left in reset. Thus an early
> reset driver was created. This early reset driver is only for the
> SoCFPGA 32-bit platform.
>
> The Stratix10 platform does not need any of the timers that in reset to
> boot, thus we don't need to early reset driver. Therefore, use the
> "altr,stratix10-rst-mgr" binding for the reset-simple platform driver on
> the Stratix10 platform.
>
> Also remove the "altr,modrst-offset" property because the driver no
> longer needs it.
>
> Signed-off-by: Dinh Nguyen
> ---
> arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> index 8253a1a9e985..5f0b18ae5007 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> @@ -308,9 +308,8 @@
>
> rst: rstmgr@ffd11000 {
> #reset-cells = <1>;
> - compatible = "altr,rst-mgr";
> + compatible = "altr,stratix10-rst-mgr", "altr,rst-mgr";
> reg = <0xffd11000 0x1000>;
> - altr,modrst-offset = <0x20>;
> };
>
> spi0: spi@ffda4000 {
Do you want me to pick this up as well, or just patches 2 and 3?
regards
Philipp
Re: [PATCH 3/3] ARM: socfpga: dts: document "altr,stratix10-rst-mgr" binding
Hi Dinh, On Mon, 2018-11-05 at 14:05 -0600, Dinh Nguyen wrote: > "altr,stratix10-rst-mgr" is used for the Stratix10 reset manager. > > Signed-off-by: Dinh Nguyen > --- > Documentation/devicetree/bindings/reset/socfpga-reset.txt | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/reset/socfpga-reset.txt > b/Documentation/devicetree/bindings/reset/socfpga-reset.txt > index 98c9f560e5c5..38fe34fd8b8a 100644 > --- a/Documentation/devicetree/bindings/reset/socfpga-reset.txt > +++ b/Documentation/devicetree/bindings/reset/socfpga-reset.txt > @@ -1,7 +1,8 @@ > Altera SOCFPGA Reset Manager > > Required properties: > -- compatible : "altr,rst-mgr" > +- compatible : "altr,rst-mgr" for (Cyclone5/Arria5/Arria10) > +"altr,stratix10-rst-mgr","altr,rst-mgr" for Stratix10 ARM64 SoC git grep '\(altr\|intel\),stratix10' currently only shows "intel,stratix10-clkmgr". Should this be "intel,stratix10-rst-mgr"? I think keeping "altr," is fine for consistency, just wanted to point it out. > - reg : Should contain 1 register ranges(address and length) > - altr,modrst-offset : Should contain the offset of the first modrst > register. > - #reset-cells: 1 regards Philipp
Re: [PATCH trivial] reset: Improve reset controller kernel docs
Hi Geert, On Mon, 2018-10-08 at 13:15 +0200, Geert Uytterhoeven wrote: > Grammar and indentation fixes. > > Signed-off-by: Geert Uytterhoeven > --- > include/linux/reset.h | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/linux/reset.h b/include/linux/reset.h > index 29af6d6b2f4b8103..d01ea059e2beee6e 100644 > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -138,7 +138,7 @@ __must_check reset_control_get_exclusive(struct device > *dev, const char *id) > * > * Returns a struct reset_control or IS_ERR() condition containing errno. > * This function is intended for use with reset-controls which are shared > - * between hardware-blocks. > + * among hardware blocks. For now I have applied this patch without the s/between/among/, which I am not sure about. regards Philipp
Re: [PATCH trivial] reset: Improve reset controller kernel docs
Hi Geert, On Mon, 2018-10-08 at 13:15 +0200, Geert Uytterhoeven wrote: > Grammar and indentation fixes. > > Signed-off-by: Geert Uytterhoeven > --- > include/linux/reset.h | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/linux/reset.h b/include/linux/reset.h > index 29af6d6b2f4b8103..d01ea059e2beee6e 100644 > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -138,7 +138,7 @@ __must_check reset_control_get_exclusive(struct device > *dev, const char *id) > * > * Returns a struct reset_control or IS_ERR() condition containing errno. > * This function is intended for use with reset-controls which are shared > - * between hardware-blocks. > + * among hardware blocks. For now I have applied this patch without the s/between/among/, which I am not sure about. regards Philipp
Re: [PATCHv3 2/3] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Mon, 2018-11-05 at 14:05 -0600, Dinh Nguyen wrote:
> From: Dinh Nguyen
>
> Create a separate reset driver that uses the reset operations in
> reset-simple. The reset driver for the SoCFPGA platform needs to
> register early in order to be able bring online timers that needed
> early in the kernel bootup.
>
> We do not need this early reset driver for Stratix10, because on
> arm64, Linux does not need the timers are that in reset. Linux is
> able to run just fine with the internal armv8 timer. Thus, we use
> a new binding "altr,stratix10-rst-mgr" for the Stratix10 platform.
> The Stratix10 platform will continue to use the reset-simple platform
> driver, while the 32-bit platforms(Cyclone5/Arria5/Arria10) will use
> the early reset driver.
>
> Signed-off-by: Dinh Nguyen
> ---
> v3: use "altr,stratix10-rst-mgr" for Stratix10
> remove "altr,modrst-offset" from reset-simple
> v2: Do not build separate reset driver for STRATIX10
> fix warning: symbol 'socfpga_reset_init' was not declared. Should it be
> static?
> ---
> arch/arm/mach-socfpga/socfpga.c | 4 ++
> drivers/reset/Kconfig | 9 +++-
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-simple.c| 12 +
> drivers/reset/reset-socfpga.c | 88 +
> 5 files changed, 103 insertions(+), 11 deletions(-)
> create mode 100644 drivers/reset/reset-socfpga.c
>
> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
> index dde14f7bf2c3..cc64576c102b 100644
> --- a/arch/arm/mach-socfpga/socfpga.c
> +++ b/arch/arm/mach-socfpga/socfpga.c
> @@ -32,6 +32,8 @@ void __iomem *rst_manager_base_addr;
> void __iomem *sdr_ctl_base_addr;
> unsigned long socfpga_cpu1start_addr;
>
> +extern void __init socfpga_reset_init(void);
> +
> void __init socfpga_sysmgr_init(void)
> {
> struct device_node *np;
> @@ -64,6 +66,7 @@ static void __init socfpga_init_irq(void)
>
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void __init socfpga_arria10_init_irq(void)
> @@ -74,6 +77,7 @@ static void __init socfpga_arria10_init_irq(void)
> socfpga_init_arria10_l2_ecc();
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_arria10_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd)
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index c21da9fe51ec..5d7a3ba445aa 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -109,7 +109,7 @@ config RESET_QCOM_PDC
>
> config RESET_SIMPLE
> bool "Simple Reset Controller Driver" if COMPILE_TEST
> - default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI ||
> ARCH_ZX || ARCH_ASPEED
> + default ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX ||
> ARCH_ASPEED
> help
> This enables a simple reset controller driver for reset lines that
> that can be asserted and deasserted by toggling bits in a contiguous,
> @@ -128,6 +128,13 @@ config RESET_STM32MP157
> help
> This enables the RCC reset controller driver for STM32 MPUs.
>
> +config RESET_SOCFPGA
> + bool "SoCFPGA Reset Driver" if COMPILE_TEST && !ARCH_SOCFPGA
> + default ARCH_SOCFPGA && !ARCH_STRATIX10
I don't understand the "&& !ARCH_STRATIX10" part.
Isn't ARCH_SOCFPGA disabled anyway when ARCH_STRATIX10 is enabled?
> + select RESET_SIMPLE
> + help
> + This enables the reset driver for SoCFPGA.
> +
> config RESET_SUNXI
> bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
> default ARCH_SUNXI
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index d08e8b90046a..b14de32eb610 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
> obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o
> +obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index a91107fc9e27..f2e7a8c1be65 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -109,7 +109,7 @@ struct reset_simple_devdata {
> #define SOCFPGA_NR_BANKS 8
>
> static const struct reset_simple_devdata reset_simple_socfpga = {
> - .reg_offset = 0x10,
> + .reg_offset = 0x20,
> .nr_resets = SOCFPGA_NR_BANKS * 32,
> .status_active_low = true,
> };
> @@ -120,7 +120,7 @@ static const struct reset_simple_devdata
> reset_simple_active_low = {
> };
>
> static const struct of_device_id reset_simple_dt_ids[] = {
>
Re: [PATCHv3 2/3] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Mon, 2018-11-05 at 14:05 -0600, Dinh Nguyen wrote:
> From: Dinh Nguyen
>
> Create a separate reset driver that uses the reset operations in
> reset-simple. The reset driver for the SoCFPGA platform needs to
> register early in order to be able bring online timers that needed
> early in the kernel bootup.
>
> We do not need this early reset driver for Stratix10, because on
> arm64, Linux does not need the timers are that in reset. Linux is
> able to run just fine with the internal armv8 timer. Thus, we use
> a new binding "altr,stratix10-rst-mgr" for the Stratix10 platform.
> The Stratix10 platform will continue to use the reset-simple platform
> driver, while the 32-bit platforms(Cyclone5/Arria5/Arria10) will use
> the early reset driver.
>
> Signed-off-by: Dinh Nguyen
> ---
> v3: use "altr,stratix10-rst-mgr" for Stratix10
> remove "altr,modrst-offset" from reset-simple
> v2: Do not build separate reset driver for STRATIX10
> fix warning: symbol 'socfpga_reset_init' was not declared. Should it be
> static?
> ---
> arch/arm/mach-socfpga/socfpga.c | 4 ++
> drivers/reset/Kconfig | 9 +++-
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-simple.c| 12 +
> drivers/reset/reset-socfpga.c | 88 +
> 5 files changed, 103 insertions(+), 11 deletions(-)
> create mode 100644 drivers/reset/reset-socfpga.c
>
> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
> index dde14f7bf2c3..cc64576c102b 100644
> --- a/arch/arm/mach-socfpga/socfpga.c
> +++ b/arch/arm/mach-socfpga/socfpga.c
> @@ -32,6 +32,8 @@ void __iomem *rst_manager_base_addr;
> void __iomem *sdr_ctl_base_addr;
> unsigned long socfpga_cpu1start_addr;
>
> +extern void __init socfpga_reset_init(void);
> +
> void __init socfpga_sysmgr_init(void)
> {
> struct device_node *np;
> @@ -64,6 +66,7 @@ static void __init socfpga_init_irq(void)
>
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void __init socfpga_arria10_init_irq(void)
> @@ -74,6 +77,7 @@ static void __init socfpga_arria10_init_irq(void)
> socfpga_init_arria10_l2_ecc();
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_arria10_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd)
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index c21da9fe51ec..5d7a3ba445aa 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -109,7 +109,7 @@ config RESET_QCOM_PDC
>
> config RESET_SIMPLE
> bool "Simple Reset Controller Driver" if COMPILE_TEST
> - default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI ||
> ARCH_ZX || ARCH_ASPEED
> + default ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX ||
> ARCH_ASPEED
> help
> This enables a simple reset controller driver for reset lines that
> that can be asserted and deasserted by toggling bits in a contiguous,
> @@ -128,6 +128,13 @@ config RESET_STM32MP157
> help
> This enables the RCC reset controller driver for STM32 MPUs.
>
> +config RESET_SOCFPGA
> + bool "SoCFPGA Reset Driver" if COMPILE_TEST && !ARCH_SOCFPGA
> + default ARCH_SOCFPGA && !ARCH_STRATIX10
I don't understand the "&& !ARCH_STRATIX10" part.
Isn't ARCH_SOCFPGA disabled anyway when ARCH_STRATIX10 is enabled?
> + select RESET_SIMPLE
> + help
> + This enables the reset driver for SoCFPGA.
> +
> config RESET_SUNXI
> bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
> default ARCH_SUNXI
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index d08e8b90046a..b14de32eb610 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
> obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o
> +obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index a91107fc9e27..f2e7a8c1be65 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -109,7 +109,7 @@ struct reset_simple_devdata {
> #define SOCFPGA_NR_BANKS 8
>
> static const struct reset_simple_devdata reset_simple_socfpga = {
> - .reg_offset = 0x10,
> + .reg_offset = 0x20,
> .nr_resets = SOCFPGA_NR_BANKS * 32,
> .status_active_low = true,
> };
> @@ -120,7 +120,7 @@ static const struct reset_simple_devdata
> reset_simple_active_low = {
> };
>
> static const struct of_device_id reset_simple_dt_ids[] = {
>
Re: [PATCH 3/4] dt-bindings: reset: uniphier: Add AHCI core reset description
Hi, On Mon, 2018-11-12 at 21:02 +0900, Kunihiko Hayashi wrote: > Hi, > > Thank you for some comments and pointing out. > > On Sat, 10 Nov 2018 01:14:06 +0900 wrote: > > > On Sat, Nov 10, 2018 at 12:02 AM Philipp Zabel > > wrote: > > > > > > Hi Kunihiko, > > > > > > On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote: > > > > Add compatible strings for reset control of AHCI core implemented in > > > > UniPhier SoCs. The reset control belongs to AHCI glue layer. > > > > > > > > Signed-off-by: Kunihiko Hayashi > > > > --- > > > > Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt > > > > b/Documentation/devicetree/bindings/reset/uniphier-reset.txt > > > > index f63c511..ea00517 100644 > > > > --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt > > > > +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt > > > > @@ -133,6 +133,9 @@ Required properties: > > > > "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC USB3 > > > > "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC USB3 > > > > "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC USB3 > > > > +"socionext,uniphier-pro4-ahci-reset" - for Pro4 SoC AHCI > > > > +"socionext,uniphier-pxs2-ahci-reset" - for PXs2 SoC AHCI > > > > +"socionext,uniphier-pxs3-ahci-reset" - for PXs3 SoC AHCI > > > > > > Since the driver behaves identically for "socionext,uniphier-pro4-usb3- > > > reset" and "socionext,uniphier-pro4-ahci-reset", would it make sense to > > > add a common compatible? > > > > As far as I could guess, he just happened to find the same driver code > > could be reused for other hardware. Ok, in that case never mind. I just assumed that this could be a case of glue layer building blocks being reused by the hardware engineers, since the driver reuses the same clock names for USB3 and AHCI both on Pro4 and on PXs2. > > Theoretically, this can happen anywhere since > > a reset controller is just a set of registers > > each bit of which is connected to a reset line. > > > > If you added a super-generic compatible like "simple-reset", > > I would agree with > > "socionext,uniphier-pro4-usb3-reset", "simple-reset" > > since this is a pattern. > > I think it's more generic to define simple-reset with parent clock/reset > control without both SoC and device names. > > However, such parent clocks/resets strongly depends on SoC, > so I think it's difficut to lead generic definition in this case. > > If we add generic compatible string, I also add "simple-reset". There is no "simple-reset" binding definition. As soon as there are SoC specific clocks, it's not really generic anymore. > > However, > > "socionext,uniphier-pro4-glue-reset" is kind of a halfway house > > where it is SoC-specific, but still ambiguous. > > Surely, it might be hard to understand that pro4-glue-reset is SoC-specific > but for generic-device. I agree. > > > Something like: > > > "socionext,uniphier-pro4-usb3-reset", > > > "socionext,uniphier-pro4-glue-reset" - for USB3 SoC AHCI > > > "socionext,uniphier-pro4-ahci-reset", > > > "socionext,uniphier-pro4-glue-reset" - for Pro4 SoC AHCI > > > > > > That way if more places turn up where the glue layer reset is used, > > > you can add them without patching the driver every time. > > > > > > This is a trade-off between "patch the driver" > > and "potential change of the binding". > > Adding "glue-reset" is usable for devices having same parent clocks/resets as > usb3/ahci, and we can add the devices without patches. > > However, if the device needs other parent clocks/resets for the same SoC, > we can't add "glue-reset" and we might add patches for the device as a result. > In this case, the "glue-reset" will become difficult to understand. Then let's not bother with it. I made the suggestion without knowing the full picture. > > There is no real hardware like pro4-glue-reset. > > > > I am guessing this is a part of syscon or something, > > but I cannot find any explanation in a bigger picture. > > > > So, I cannot judge this further more. > > Since it's hard to lead the best result, > currently I'd like to suggest the current compatibles, That is fine with me. > with "simple-reset" if necessary. Not necessary. best regards Philipp
Re: [PATCH 3/4] dt-bindings: reset: uniphier: Add AHCI core reset description
Hi, On Mon, 2018-11-12 at 21:02 +0900, Kunihiko Hayashi wrote: > Hi, > > Thank you for some comments and pointing out. > > On Sat, 10 Nov 2018 01:14:06 +0900 wrote: > > > On Sat, Nov 10, 2018 at 12:02 AM Philipp Zabel > > wrote: > > > > > > Hi Kunihiko, > > > > > > On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote: > > > > Add compatible strings for reset control of AHCI core implemented in > > > > UniPhier SoCs. The reset control belongs to AHCI glue layer. > > > > > > > > Signed-off-by: Kunihiko Hayashi > > > > --- > > > > Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt > > > > b/Documentation/devicetree/bindings/reset/uniphier-reset.txt > > > > index f63c511..ea00517 100644 > > > > --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt > > > > +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt > > > > @@ -133,6 +133,9 @@ Required properties: > > > > "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC USB3 > > > > "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC USB3 > > > > "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC USB3 > > > > +"socionext,uniphier-pro4-ahci-reset" - for Pro4 SoC AHCI > > > > +"socionext,uniphier-pxs2-ahci-reset" - for PXs2 SoC AHCI > > > > +"socionext,uniphier-pxs3-ahci-reset" - for PXs3 SoC AHCI > > > > > > Since the driver behaves identically for "socionext,uniphier-pro4-usb3- > > > reset" and "socionext,uniphier-pro4-ahci-reset", would it make sense to > > > add a common compatible? > > > > As far as I could guess, he just happened to find the same driver code > > could be reused for other hardware. Ok, in that case never mind. I just assumed that this could be a case of glue layer building blocks being reused by the hardware engineers, since the driver reuses the same clock names for USB3 and AHCI both on Pro4 and on PXs2. > > Theoretically, this can happen anywhere since > > a reset controller is just a set of registers > > each bit of which is connected to a reset line. > > > > If you added a super-generic compatible like "simple-reset", > > I would agree with > > "socionext,uniphier-pro4-usb3-reset", "simple-reset" > > since this is a pattern. > > I think it's more generic to define simple-reset with parent clock/reset > control without both SoC and device names. > > However, such parent clocks/resets strongly depends on SoC, > so I think it's difficut to lead generic definition in this case. > > If we add generic compatible string, I also add "simple-reset". There is no "simple-reset" binding definition. As soon as there are SoC specific clocks, it's not really generic anymore. > > However, > > "socionext,uniphier-pro4-glue-reset" is kind of a halfway house > > where it is SoC-specific, but still ambiguous. > > Surely, it might be hard to understand that pro4-glue-reset is SoC-specific > but for generic-device. I agree. > > > Something like: > > > "socionext,uniphier-pro4-usb3-reset", > > > "socionext,uniphier-pro4-glue-reset" - for USB3 SoC AHCI > > > "socionext,uniphier-pro4-ahci-reset", > > > "socionext,uniphier-pro4-glue-reset" - for Pro4 SoC AHCI > > > > > > That way if more places turn up where the glue layer reset is used, > > > you can add them without patching the driver every time. > > > > > > This is a trade-off between "patch the driver" > > and "potential change of the binding". > > Adding "glue-reset" is usable for devices having same parent clocks/resets as > usb3/ahci, and we can add the devices without patches. > > However, if the device needs other parent clocks/resets for the same SoC, > we can't add "glue-reset" and we might add patches for the device as a result. > In this case, the "glue-reset" will become difficult to understand. Then let's not bother with it. I made the suggestion without knowing the full picture. > > There is no real hardware like pro4-glue-reset. > > > > I am guessing this is a part of syscon or something, > > but I cannot find any explanation in a bigger picture. > > > > So, I cannot judge this further more. > > Since it's hard to lead the best result, > currently I'd like to suggest the current compatibles, That is fine with me. > with "simple-reset" if necessary. Not necessary. best regards Philipp
Re: [PATCH 3/4] dt-bindings: reset: uniphier: Add AHCI core reset description
Hi Kunihiko, On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote: > Add compatible strings for reset control of AHCI core implemented in > UniPhier SoCs. The reset control belongs to AHCI glue layer. > > Signed-off-by: Kunihiko Hayashi > --- > Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt > b/Documentation/devicetree/bindings/reset/uniphier-reset.txt > index f63c511..ea00517 100644 > --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt > +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt > @@ -133,6 +133,9 @@ Required properties: > "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC USB3 > "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC USB3 > "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC USB3 > +"socionext,uniphier-pro4-ahci-reset" - for Pro4 SoC AHCI > +"socionext,uniphier-pxs2-ahci-reset" - for PXs2 SoC AHCI > +"socionext,uniphier-pxs3-ahci-reset" - for PXs3 SoC AHCI Since the driver behaves identically for "socionext,uniphier-pro4-usb3- reset" and "socionext,uniphier-pro4-ahci-reset", would it make sense to add a common compatible? Something like: "socionext,uniphier-pro4-usb3-reset", "socionext,uniphier-pro4-glue-reset" - for USB3 SoC AHCI "socionext,uniphier-pro4-ahci-reset", "socionext,uniphier-pro4-glue-reset" - for Pro4 SoC AHCI That way if more places turn up where the glue layer reset is used, you can add them without patching the driver every time. regards Philipp
Re: [PATCH 3/4] dt-bindings: reset: uniphier: Add AHCI core reset description
Hi Kunihiko, On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote: > Add compatible strings for reset control of AHCI core implemented in > UniPhier SoCs. The reset control belongs to AHCI glue layer. > > Signed-off-by: Kunihiko Hayashi > --- > Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt > b/Documentation/devicetree/bindings/reset/uniphier-reset.txt > index f63c511..ea00517 100644 > --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt > +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt > @@ -133,6 +133,9 @@ Required properties: > "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC USB3 > "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC USB3 > "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC USB3 > +"socionext,uniphier-pro4-ahci-reset" - for Pro4 SoC AHCI > +"socionext,uniphier-pxs2-ahci-reset" - for PXs2 SoC AHCI > +"socionext,uniphier-pxs3-ahci-reset" - for PXs3 SoC AHCI Since the driver behaves identically for "socionext,uniphier-pro4-usb3- reset" and "socionext,uniphier-pro4-ahci-reset", would it make sense to add a common compatible? Something like: "socionext,uniphier-pro4-usb3-reset", "socionext,uniphier-pro4-glue-reset" - for USB3 SoC AHCI "socionext,uniphier-pro4-ahci-reset", "socionext,uniphier-pro4-glue-reset" - for Pro4 SoC AHCI That way if more places turn up where the glue layer reset is used, you can add them without patching the driver every time. regards Philipp
Re: [PATCH] PCI: imx: Add imx6sx suspend/resume support
Hi Leonard,
On Wed, 2018-10-31 at 11:02 +, Leonard Crestez wrote:
> On 10/8/2018 8:38 PM, Leonard Crestez wrote:
> > Enable PCI suspend/resume support on imx6sx socs. This is similar to
> > imx7d with a few differences:
> >
> > * The PM_Turn_Off bit is exposed through an IOMUX GPR, like all other
> > pcie control bits on 6sx.
> > * The pcie_inbound_axi clk needs to be turned off in suspend. On resume
> > it is restored via resume -> deassert_core_reset -> enable_ref_clk.
> >
> > Most of the resume logic is shared with the initial reset after probe.
> >
> > Signed-off-by: Leonard Crestez
>
> This is a gentle reminder that this patch was posted ~3 weeks ago and
> there is yet no reply. It's a mostly straight-forward extension of imx7d
> pci suspend/resume support to a different SOC.
>
> Lucas/Philipp: can you please take a brief look?
This is a bit out of my wheelhouse, but I'll try my best.
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 40 ++---
> > include/linux/mfd/syscon/imx6q-iomuxc-gpr.h | 1 +
> > 2 files changed, 36 insertions(+), 5 deletions(-)
> >
> > Patch is again linux-next, meant to apply here:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/log/?h=pci/dwc
> >
> > This is quite an old patch, mostly did it to prove that imx7d suspend
> > support is indeed generic.
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 2cbef2d7c207..6171171db1fc 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -771,12 +771,29 @@ static void imx6_pcie_ltssm_disable(struct device
> > *dev)
> > }
> > }
> >
> > static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> > {
> > - reset_control_assert(imx6_pcie->turnoff_reset);
> > - reset_control_deassert(imx6_pcie->turnoff_reset);
> > + struct device *dev = imx6_pcie->pci->dev;
> > +
> > + /*
> > +* Some variants have a turnoff reset in DT while
> > +* others poke at iomuxc registers.
> > +*/
> > + if (imx6_pcie->turnoff_reset) {
> > + reset_control_assert(imx6_pcie->turnoff_reset);
> > + reset_control_deassert(imx6_pcie->turnoff_reset);
> > + } else if (imx6_pcie->variant == IMX6SX) {
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > + IMX6SX_GPR12_PCIE_PM_TURN_OFF,
> > + IMX6SX_GPR12_PCIE_PM_TURN_OFF);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > + IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0);
> > + } else {
> > + dev_err(dev, "PME_Turn_Off not implemented\n");
> > + return;
> > + }
I'd use switch(imx6_pcie->variant) for consistency with the other places
where different variants need to be handled separately.
> >
> > /*
> > * Components with an upstream port must respond to
> > * PME_Turn_Off with PME_TO_Ack but we can't check.
> > *
> > @@ -790,22 +807,35 @@ static void imx6_pcie_clk_disable(struct imx6_pcie
> > *imx6_pcie)
> > {
> > clk_disable_unprepare(imx6_pcie->pcie);
> > clk_disable_unprepare(imx6_pcie->pcie_phy);
> > clk_disable_unprepare(imx6_pcie->pcie_bus);
> >
> > - if (imx6_pcie->variant == IMX7D) {
> > + switch (imx6_pcie->variant) {
> > + case IMX6SX:
> > + clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > + break;
> > + case IMX7D:
> > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> >IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> >IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > + break;
> > + default:
> > + break;
> > }
This disables the ref clock. Should this be split into a separate
function imx6_pcie_disable_ref_clk() for symmetry with the already
existing imx6_pcie_enable_ref_clk() ?
That could then also be used to disable pcie_inbound_axi in the error
path of imx6_pcie_deassert_core_reset().
> > }
> >
> > +static inline bool imx6_pcie_supports_suspend(struct imx6_pcie *imx6_pcie)
> > +{
> > + return (imx6_pcie->variant == IMX7D ||
> > + imx6_pcie->variant == IMX6SX);
> > +}
> > +
> > static int imx6_pcie_suspend_noirq(struct device *dev)
> > {
> > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> >
> > - if (imx6_pcie->variant != IMX7D)
> > + if (!imx6_pcie_supports_suspend(imx6_pcie))
> > return 0;
> >
> > imx6_pcie_pm_turnoff(imx6_pcie);
> > imx6_pcie_clk_disable(imx6_pcie);
> > imx6_pcie_ltssm_disable(dev);
> > @@ -817,11 +847,11 @@ static int imx6_pcie_resume_noirq(struct device *dev)
> > {
> > int ret;
> > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > struct pcie_port *pp = _pcie->pci->pp;
> >
> > - if (imx6_pcie->variant != IMX7D)
> > + if
Re: [PATCH] PCI: imx: Add imx6sx suspend/resume support
Hi Leonard,
On Wed, 2018-10-31 at 11:02 +, Leonard Crestez wrote:
> On 10/8/2018 8:38 PM, Leonard Crestez wrote:
> > Enable PCI suspend/resume support on imx6sx socs. This is similar to
> > imx7d with a few differences:
> >
> > * The PM_Turn_Off bit is exposed through an IOMUX GPR, like all other
> > pcie control bits on 6sx.
> > * The pcie_inbound_axi clk needs to be turned off in suspend. On resume
> > it is restored via resume -> deassert_core_reset -> enable_ref_clk.
> >
> > Most of the resume logic is shared with the initial reset after probe.
> >
> > Signed-off-by: Leonard Crestez
>
> This is a gentle reminder that this patch was posted ~3 weeks ago and
> there is yet no reply. It's a mostly straight-forward extension of imx7d
> pci suspend/resume support to a different SOC.
>
> Lucas/Philipp: can you please take a brief look?
This is a bit out of my wheelhouse, but I'll try my best.
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 40 ++---
> > include/linux/mfd/syscon/imx6q-iomuxc-gpr.h | 1 +
> > 2 files changed, 36 insertions(+), 5 deletions(-)
> >
> > Patch is again linux-next, meant to apply here:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/log/?h=pci/dwc
> >
> > This is quite an old patch, mostly did it to prove that imx7d suspend
> > support is indeed generic.
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 2cbef2d7c207..6171171db1fc 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -771,12 +771,29 @@ static void imx6_pcie_ltssm_disable(struct device
> > *dev)
> > }
> > }
> >
> > static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> > {
> > - reset_control_assert(imx6_pcie->turnoff_reset);
> > - reset_control_deassert(imx6_pcie->turnoff_reset);
> > + struct device *dev = imx6_pcie->pci->dev;
> > +
> > + /*
> > +* Some variants have a turnoff reset in DT while
> > +* others poke at iomuxc registers.
> > +*/
> > + if (imx6_pcie->turnoff_reset) {
> > + reset_control_assert(imx6_pcie->turnoff_reset);
> > + reset_control_deassert(imx6_pcie->turnoff_reset);
> > + } else if (imx6_pcie->variant == IMX6SX) {
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > + IMX6SX_GPR12_PCIE_PM_TURN_OFF,
> > + IMX6SX_GPR12_PCIE_PM_TURN_OFF);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > + IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0);
> > + } else {
> > + dev_err(dev, "PME_Turn_Off not implemented\n");
> > + return;
> > + }
I'd use switch(imx6_pcie->variant) for consistency with the other places
where different variants need to be handled separately.
> >
> > /*
> > * Components with an upstream port must respond to
> > * PME_Turn_Off with PME_TO_Ack but we can't check.
> > *
> > @@ -790,22 +807,35 @@ static void imx6_pcie_clk_disable(struct imx6_pcie
> > *imx6_pcie)
> > {
> > clk_disable_unprepare(imx6_pcie->pcie);
> > clk_disable_unprepare(imx6_pcie->pcie_phy);
> > clk_disable_unprepare(imx6_pcie->pcie_bus);
> >
> > - if (imx6_pcie->variant == IMX7D) {
> > + switch (imx6_pcie->variant) {
> > + case IMX6SX:
> > + clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > + break;
> > + case IMX7D:
> > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> >IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> >IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > + break;
> > + default:
> > + break;
> > }
This disables the ref clock. Should this be split into a separate
function imx6_pcie_disable_ref_clk() for symmetry with the already
existing imx6_pcie_enable_ref_clk() ?
That could then also be used to disable pcie_inbound_axi in the error
path of imx6_pcie_deassert_core_reset().
> > }
> >
> > +static inline bool imx6_pcie_supports_suspend(struct imx6_pcie *imx6_pcie)
> > +{
> > + return (imx6_pcie->variant == IMX7D ||
> > + imx6_pcie->variant == IMX6SX);
> > +}
> > +
> > static int imx6_pcie_suspend_noirq(struct device *dev)
> > {
> > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> >
> > - if (imx6_pcie->variant != IMX7D)
> > + if (!imx6_pcie_supports_suspend(imx6_pcie))
> > return 0;
> >
> > imx6_pcie_pm_turnoff(imx6_pcie);
> > imx6_pcie_clk_disable(imx6_pcie);
> > imx6_pcie_ltssm_disable(dev);
> > @@ -817,11 +847,11 @@ static int imx6_pcie_resume_noirq(struct device *dev)
> > {
> > int ret;
> > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > struct pcie_port *pp = _pcie->pci->pp;
> >
> > - if (imx6_pcie->variant != IMX7D)
> > + if
Re: [PATCH v2 3/3] reset: reset-zynqmp: Adding support for Xilinx zynqmp reset controller.
On Fri, Oct 26, 2018 at 05:54:24PM +0530, Nava kishore Manne wrote:
[...]
> +static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> + int val, err;
> +
> + err = priv->eemi_ops->reset_get_status(ZYNQMP_RESET_ID + id, );
> + if (!err)
> + return err;
This should be:
if (err)
return err;
instead. Otherwise you end up always returning 0 on success ...
> +
> + return val;
... and uninitialized val on error.
regards
Philipp
Re: [PATCH v2 3/3] reset: reset-zynqmp: Adding support for Xilinx zynqmp reset controller.
On Fri, Oct 26, 2018 at 05:54:24PM +0530, Nava kishore Manne wrote:
[...]
> +static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> + int val, err;
> +
> + err = priv->eemi_ops->reset_get_status(ZYNQMP_RESET_ID + id, );
> + if (!err)
> + return err;
This should be:
if (err)
return err;
instead. Otherwise you end up always returning 0 on success ...
> +
> + return val;
... and uninitialized val on error.
regards
Philipp
Re: [PATCH 3/3] reset: reset-zynqmp: Adding support for Xilinx zynqmp reset controller.
Hi Nava,
On Sat, 2018-10-20 at 14:11 +0530, Nava kishore Manne wrote:
> Add a reset controller driver for Xilinx Zynq UltraScale+ MPSoC.
> The zynqmp reset-controller has the ability to reset lines
> connected to different blocks and peripheral in the Soc.
>
> Signed-off-by: Nava kishore Manne
> ---
> Changes for v1:
> -None.
I had comments on RFC v3 that are not addressed yet, see below.
> --- /dev/null
> +++ b/drivers/reset/reset-zynqmp.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Xilinx, Inc.
> + *
> + */
> +
> +#include
Unnecessary.
[...]
> +static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> + int val, err;
> +
> + err = priv->eemi_ops->reset_get_status(ZYNQMP_RESET_ID + id, );
> + if (!err)
> + return -EINVAL;
Should return error code, and only if there is an error:
if (err)
return err;
> + return val;
> +}
> +
> +static int zynqmp_reset_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> +
> + return priv->eemi_ops->reset_assert(ZYNQMP_RESET_ID + id,
> + PM_RESET_ACTION_PULSE);
> +}
> +
> +static struct reset_control_ops zynqmp_reset_ops = {
Should be const:
static const struct reset_control_ops zynqmp_reset_ops = {
> + .reset = zynqmp_reset_reset,
> + .assert = zynqmp_reset_assert,
> + .deassert = zynqmp_reset_deassert,
> + .status = zynqmp_reset_status,
> +};
> +
> +static int zynqmp_reset_probe(struct platform_device *pdev)
> +{
> + struct zynqmp_reset_data *priv;
> +
> + priv = devm_kzalloc(>dev,
> + sizeof(*priv), GFP_KERNEL);
Fits on one line:
priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
regards
Philipp
Re: [PATCH 3/3] reset: reset-zynqmp: Adding support for Xilinx zynqmp reset controller.
Hi Nava,
On Sat, 2018-10-20 at 14:11 +0530, Nava kishore Manne wrote:
> Add a reset controller driver for Xilinx Zynq UltraScale+ MPSoC.
> The zynqmp reset-controller has the ability to reset lines
> connected to different blocks and peripheral in the Soc.
>
> Signed-off-by: Nava kishore Manne
> ---
> Changes for v1:
> -None.
I had comments on RFC v3 that are not addressed yet, see below.
> --- /dev/null
> +++ b/drivers/reset/reset-zynqmp.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Xilinx, Inc.
> + *
> + */
> +
> +#include
Unnecessary.
[...]
> +static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> + int val, err;
> +
> + err = priv->eemi_ops->reset_get_status(ZYNQMP_RESET_ID + id, );
> + if (!err)
> + return -EINVAL;
Should return error code, and only if there is an error:
if (err)
return err;
> + return val;
> +}
> +
> +static int zynqmp_reset_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> +
> + return priv->eemi_ops->reset_assert(ZYNQMP_RESET_ID + id,
> + PM_RESET_ACTION_PULSE);
> +}
> +
> +static struct reset_control_ops zynqmp_reset_ops = {
Should be const:
static const struct reset_control_ops zynqmp_reset_ops = {
> + .reset = zynqmp_reset_reset,
> + .assert = zynqmp_reset_assert,
> + .deassert = zynqmp_reset_deassert,
> + .status = zynqmp_reset_status,
> +};
> +
> +static int zynqmp_reset_probe(struct platform_device *pdev)
> +{
> + struct zynqmp_reset_data *priv;
> +
> + priv = devm_kzalloc(>dev,
> + sizeof(*priv), GFP_KERNEL);
Fits on one line:
priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
regards
Philipp
Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support
Hi Maxime,
On Fri, 2018-10-12 at 15:46 +0200, Maxime Ripard wrote:
> On Fri, Oct 12, 2018 at 12:05:16PM +0200, Philipp Zabel wrote:
[...]
> > What I would like better would be to let the consumers keep their reset-
> > gpios bindings, but add an optional hold time override in the DT:
> >
> > c1 {
> > reset-gpios = < 15 GPIO_ACTIVE_LOW>;
> > reset-delays-us = <1>;
> > };
> >
> > c2 {
> > reset-gpios = < 15 GPIO_ACTIVE_LOW>;
> > reset-delays-us = <1>;
> > };
> >
> > The reset framework could create a reset_control backed by a gpio
> > instead of a rcdev. I have an unfinished patch for this, but without the
> > sharing requirement adding the reset framework abstraction between gpiod
> > and drivers never seemed really worth it.
>
> I don't remember the exact details of our past discussion, but I
> (still) don't really think that this would work well.
It has been a while :) Thanks for jumping back in.
> I see two main shortcomings with that approach:
>
> - I guess that the main reason you want to do that would be to have
> easy DT backward compatibility.
Yes, that is true. The other reason is that I'd like devices to have a
single binding, regardless of whether somebody decided to put them onto
a board with shared reset lines.
I'd find it hard to advocate for changing the thankfully common case
of device-exclusive reset gpios from:
some-device {
reset-gpios = < y>;
};
to:
rstc: reset-controller {
compatible = "gpio-reset";
reset-gpios < y>;
};
some-device {
resets = < 0>;
};
even for new bindings.
If the reset framework only supports the latter, and not the former,
drivers for devices which already have reset-gpios would have to handle
both reset_control and gpiod functionality, which I think should not be
necessary.
Note that this is not really an argument against a "gpio-reset"
controller driver, but an argument for reset-gpios support integrated
into the reset framework.
My argument against a "gpio-reset" device node in the DT is only that it
is basically a virtual device that would only be used to work around
missing reset-gpios support in the reset framework. Physically, this
"device" consists of no more than a few PCB traces.
> Yet, the addition of the
> reset-delay-us would break that compatibility, since drivers that
> would have been converted now need to have it somehow, but older
> DTs wouldn't have it
That is why such a property would have to be optional, and the drivers
would have to keep providing the reset delay themselves, as they
currently do when handling GPIOs directly.
It would only serve to override the driver default in case of additional
delay requirements due to board specifics (such as delay elements, or
shared resets).
> - There's so many variations of the reset-gpios property name that
> you wouldn't be able to cover all the cases anyhow, at least
> without breaking the compatibility (again).
This is true, and I do not have a solution for this. Especially for
those cases that don't use gpiod yet.
of_get_named_gpio(node, "reset-gpio")
is still quite common, but I'd really like on gpiolib for polarity
support.
> But I also see your point, and you're right that converting everyone
> to a gpio-reset node will not happen (even though I'd still really
> like to have that binding).
See above. I'd be a lot less reluctant to support this binding if
somebody could demonstrate a real gpio controlled reset controller
device of some kind. And even then I would not want to have to use
this device just to connect a single GPIO with a single reset input.
> What about having a function that would be called by the consumer to
> instantiate that reset controller from a GPIO for us, instead of
> trying to do it automatically? That function could take the property
> name that holds the GPIO, which would cover the second drawback, and
> the delay, which would cover the first.
I like this idea.
I'd like to avoid having to fall back from gpiod to of_get_named_gpio if
possible, so maybe we'd have to extend gpiolib-of.c with an
of_find_reset_gpio function, as is done for SPI and regulators already.
We probably would have to support delay ranges. I see a lot of
usleep_range calls between reset GPIO toggles in the drivers.
> And in order to cover shared GPIO reset lines, we could just look at
> the one already created by other drivers, and if one has been created,
> we just give the reference to that one instead of creating it.
>
> Does that make sense?
I'm not quite sure how to ma
Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support
Hi Maxime,
On Fri, 2018-10-12 at 15:46 +0200, Maxime Ripard wrote:
> On Fri, Oct 12, 2018 at 12:05:16PM +0200, Philipp Zabel wrote:
[...]
> > What I would like better would be to let the consumers keep their reset-
> > gpios bindings, but add an optional hold time override in the DT:
> >
> > c1 {
> > reset-gpios = < 15 GPIO_ACTIVE_LOW>;
> > reset-delays-us = <1>;
> > };
> >
> > c2 {
> > reset-gpios = < 15 GPIO_ACTIVE_LOW>;
> > reset-delays-us = <1>;
> > };
> >
> > The reset framework could create a reset_control backed by a gpio
> > instead of a rcdev. I have an unfinished patch for this, but without the
> > sharing requirement adding the reset framework abstraction between gpiod
> > and drivers never seemed really worth it.
>
> I don't remember the exact details of our past discussion, but I
> (still) don't really think that this would work well.
It has been a while :) Thanks for jumping back in.
> I see two main shortcomings with that approach:
>
> - I guess that the main reason you want to do that would be to have
> easy DT backward compatibility.
Yes, that is true. The other reason is that I'd like devices to have a
single binding, regardless of whether somebody decided to put them onto
a board with shared reset lines.
I'd find it hard to advocate for changing the thankfully common case
of device-exclusive reset gpios from:
some-device {
reset-gpios = < y>;
};
to:
rstc: reset-controller {
compatible = "gpio-reset";
reset-gpios < y>;
};
some-device {
resets = < 0>;
};
even for new bindings.
If the reset framework only supports the latter, and not the former,
drivers for devices which already have reset-gpios would have to handle
both reset_control and gpiod functionality, which I think should not be
necessary.
Note that this is not really an argument against a "gpio-reset"
controller driver, but an argument for reset-gpios support integrated
into the reset framework.
My argument against a "gpio-reset" device node in the DT is only that it
is basically a virtual device that would only be used to work around
missing reset-gpios support in the reset framework. Physically, this
"device" consists of no more than a few PCB traces.
> Yet, the addition of the
> reset-delay-us would break that compatibility, since drivers that
> would have been converted now need to have it somehow, but older
> DTs wouldn't have it
That is why such a property would have to be optional, and the drivers
would have to keep providing the reset delay themselves, as they
currently do when handling GPIOs directly.
It would only serve to override the driver default in case of additional
delay requirements due to board specifics (such as delay elements, or
shared resets).
> - There's so many variations of the reset-gpios property name that
> you wouldn't be able to cover all the cases anyhow, at least
> without breaking the compatibility (again).
This is true, and I do not have a solution for this. Especially for
those cases that don't use gpiod yet.
of_get_named_gpio(node, "reset-gpio")
is still quite common, but I'd really like on gpiolib for polarity
support.
> But I also see your point, and you're right that converting everyone
> to a gpio-reset node will not happen (even though I'd still really
> like to have that binding).
See above. I'd be a lot less reluctant to support this binding if
somebody could demonstrate a real gpio controlled reset controller
device of some kind. And even then I would not want to have to use
this device just to connect a single GPIO with a single reset input.
> What about having a function that would be called by the consumer to
> instantiate that reset controller from a GPIO for us, instead of
> trying to do it automatically? That function could take the property
> name that holds the GPIO, which would cover the second drawback, and
> the delay, which would cover the first.
I like this idea.
I'd like to avoid having to fall back from gpiod to of_get_named_gpio if
possible, so maybe we'd have to extend gpiolib-of.c with an
of_find_reset_gpio function, as is done for SPI and regulators already.
We probably would have to support delay ranges. I see a lot of
usleep_range calls between reset GPIO toggles in the drivers.
> And in order to cover shared GPIO reset lines, we could just look at
> the one already created by other drivers, and if one has been created,
> we just give the reference to that one instead of creating it.
>
> Does that make sense?
I'm not quite sure how to ma
Re: [PATCHv2] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Wed, 2018-10-17 at 10:02 -0500, Dinh Nguyen wrote:
[...]
> > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > > index 13d28fdbdbb5..f10de5ce4753 100644
> > > --- a/drivers/reset/Kconfig
> > > +++ b/drivers/reset/Kconfig
> > > @@ -100,7 +100,7 @@ config RESET_QCOM_AOSS
> > >
> > > config RESET_SIMPLE
> > > bool "Simple Reset Controller Driver" if COMPILE_TEST
> > > - default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI ||
> > > ARCH_ZX || ARCH_ASPEED
> > > + default ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX ||
> > > ARCH_ASPEED
> > > help
> > > This enables a simple reset controller driver for reset lines that
> > > that can be asserted and deasserted by toggling bits in a contiguous,
> > > @@ -119,6 +119,13 @@ config RESET_STM32MP157
> > > help
> > > This enables the RCC reset controller driver for STM32 MPUs.
> > >
> > > +config RESET_SOCFPGA
> > > + bool "SoCFPGA Reset Driver" if COMPILE_TEST && !ARCH_SOCFPGA
> > > + default ARCH_SOCFPGA && !ARCH_STRATIX10
> > > + select RESET_SIMPLE
[...]
> > > +static const struct of_device_id socfpga_early_reset_dt_ids[]
> > > __initconst = {
> > > + { .compatible = "altr,rst-mgr", },
> >
> > That doesn't seem right. If you don't remove the altr,rst-mgr compatible
> > from reset-simple.c anymore, we suddenly have two device drivers for the
> > same compatible.
>
> You're right, and that's why I am not building reset-simple for
> ARCH_SOCFPGA anymore. I am only building reset-simple for ARCH_STRATIX10.
RESET_SOCFPGA still selects RESET_SIMPLE, so reset-simple should be
built in both cases.
> > (Also I liked removing altr,modrst-offset from reset-simple.c)
>
> I can remove it since it's just 0x20 of ARCH_STRATIX10.
As long as there is another driver handling that compatible,
altr,rst-mgr support should be removed from the platform driver in
reset-simple.c altogether.
> > Would there be any issue with calling socfpga_reset_init() on Stratix10
> > as well?
>
> I don't see any place in the arm64 common code where I can do this.
This is one of the reasons why there should always be a SoC specific
compatible as well as the generic one. If the device trees contained
something like
compatible = "altr,socfpga-a10-rst-mgr", "altr,rst-mgr";
and
compatible = "altr,socfpga-s10-rst-mgr", "altr,rst-mgr";
we could now easily match the specific compatibles for the different
cases (s10 in reset-simple, a10 in reset-socfpga).
I suppose one way to handle this with the shared compatible would be to
add a platform driver for s10 to reset-socfpga.c, but register it only
if socfpga_reset_init() was not called earlier.
regards
Philipp
Re: [PATCHv2] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Wed, 2018-10-17 at 10:02 -0500, Dinh Nguyen wrote:
[...]
> > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > > index 13d28fdbdbb5..f10de5ce4753 100644
> > > --- a/drivers/reset/Kconfig
> > > +++ b/drivers/reset/Kconfig
> > > @@ -100,7 +100,7 @@ config RESET_QCOM_AOSS
> > >
> > > config RESET_SIMPLE
> > > bool "Simple Reset Controller Driver" if COMPILE_TEST
> > > - default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI ||
> > > ARCH_ZX || ARCH_ASPEED
> > > + default ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX ||
> > > ARCH_ASPEED
> > > help
> > > This enables a simple reset controller driver for reset lines that
> > > that can be asserted and deasserted by toggling bits in a contiguous,
> > > @@ -119,6 +119,13 @@ config RESET_STM32MP157
> > > help
> > > This enables the RCC reset controller driver for STM32 MPUs.
> > >
> > > +config RESET_SOCFPGA
> > > + bool "SoCFPGA Reset Driver" if COMPILE_TEST && !ARCH_SOCFPGA
> > > + default ARCH_SOCFPGA && !ARCH_STRATIX10
> > > + select RESET_SIMPLE
[...]
> > > +static const struct of_device_id socfpga_early_reset_dt_ids[]
> > > __initconst = {
> > > + { .compatible = "altr,rst-mgr", },
> >
> > That doesn't seem right. If you don't remove the altr,rst-mgr compatible
> > from reset-simple.c anymore, we suddenly have two device drivers for the
> > same compatible.
>
> You're right, and that's why I am not building reset-simple for
> ARCH_SOCFPGA anymore. I am only building reset-simple for ARCH_STRATIX10.
RESET_SOCFPGA still selects RESET_SIMPLE, so reset-simple should be
built in both cases.
> > (Also I liked removing altr,modrst-offset from reset-simple.c)
>
> I can remove it since it's just 0x20 of ARCH_STRATIX10.
As long as there is another driver handling that compatible,
altr,rst-mgr support should be removed from the platform driver in
reset-simple.c altogether.
> > Would there be any issue with calling socfpga_reset_init() on Stratix10
> > as well?
>
> I don't see any place in the arm64 common code where I can do this.
This is one of the reasons why there should always be a SoC specific
compatible as well as the generic one. If the device trees contained
something like
compatible = "altr,socfpga-a10-rst-mgr", "altr,rst-mgr";
and
compatible = "altr,socfpga-s10-rst-mgr", "altr,rst-mgr";
we could now easily match the specific compatibles for the different
cases (s10 in reset-simple, a10 in reset-socfpga).
I suppose one way to handle this with the shared compatible would be to
add a platform driver for s10 to reset-socfpga.c, but register it only
if socfpga_reset_init() was not called earlier.
regards
Philipp
Re: [PATCHv2] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Thu, 2018-10-11 at 08:52 -0500, Dinh Nguyen wrote:
> Create a separate reset driver that uses the reset operations in
> reset-simple. The reset driver for the SoCFPGA platform needs to
> register early in order to be able bring online timers that needed
> early in the kernel bootup.
>
> We do not need this early reset driver for Stratix10, because on
> arm64, Linux does not need the timers are that in reset. Linux is
> able to run just fine with the internal armv8 timer.
>
> Signed-off-by: Dinh Nguyen
> ---
> v2: Do not build separate reset driver for STRATIX10
> fix warning: symbol 'socfpga_reset_init' was not declared. Should
> it be static?
> ---
> arch/arm/mach-socfpga/socfpga.c | 4 ++
> drivers/reset/Kconfig | 9 +++-
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-socfpga.c | 88 +
> 4 files changed, 101 insertions(+), 1 deletion(-)
> create mode 100644 drivers/reset/reset-socfpga.c
>
> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
> index dde14f7bf2c3..cc64576c102b 100644
> --- a/arch/arm/mach-socfpga/socfpga.c
> +++ b/arch/arm/mach-socfpga/socfpga.c
> @@ -32,6 +32,8 @@ void __iomem *rst_manager_base_addr;
> void __iomem *sdr_ctl_base_addr;
> unsigned long socfpga_cpu1start_addr;
>
> +extern void __init socfpga_reset_init(void);
> +
> void __init socfpga_sysmgr_init(void)
> {
> struct device_node *np;
> @@ -64,6 +66,7 @@ static void __init socfpga_init_irq(void)
>
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void __init socfpga_arria10_init_irq(void)
> @@ -74,6 +77,7 @@ static void __init socfpga_arria10_init_irq(void)
> socfpga_init_arria10_l2_ecc();
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_arria10_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd)
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 13d28fdbdbb5..f10de5ce4753 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -100,7 +100,7 @@ config RESET_QCOM_AOSS
>
> config RESET_SIMPLE
> bool "Simple Reset Controller Driver" if COMPILE_TEST
> - default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI ||
> ARCH_ZX || ARCH_ASPEED
> + default ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX ||
> ARCH_ASPEED
> help
> This enables a simple reset controller driver for reset lines that
> that can be asserted and deasserted by toggling bits in a contiguous,
> @@ -119,6 +119,13 @@ config RESET_STM32MP157
> help
> This enables the RCC reset controller driver for STM32 MPUs.
>
> +config RESET_SOCFPGA
> + bool "SoCFPGA Reset Driver" if COMPILE_TEST && !ARCH_SOCFPGA
> + default ARCH_SOCFPGA && !ARCH_STRATIX10
> + select RESET_SIMPLE
> + help
> + This enables the reset driver for SoCFPGA.
> +
> config RESET_SUNXI
> bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
> default ARCH_SUNXI
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 4243c38228e2..d09bb41273f6 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
> obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o
> +obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
> new file mode 100644
> index ..b92769861d2b
> --- /dev/null
> +++ b/drivers/reset/reset-socfpga.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018, Intel Corporation
> + * Copied from reset-sunxi.c
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "reset-simple.h"
> +
> +#define SOCFPGA_NR_BANKS 8
> +void __init socfpga_reset_init(void);
> +
> +static int a10_reset_init(struct device_node *np)
> +{
> + struct reset_simple_data *data;
> + struct resource res;
> + resource_size_t size;
> + int ret;
> + u32 reg_offset = 0x10;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + ret = of_address_to_resource(np, 0, );
> + if (ret)
> + goto err_alloc;
> +
> + size = resource_size();
> + if (!request_mem_region(res.start, size, np->name)) {
> + ret = -EBUSY;
> + goto err_alloc;
> + }
> +
> + data->membase =
Re: [PATCHv2] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Thu, 2018-10-11 at 08:52 -0500, Dinh Nguyen wrote:
> Create a separate reset driver that uses the reset operations in
> reset-simple. The reset driver for the SoCFPGA platform needs to
> register early in order to be able bring online timers that needed
> early in the kernel bootup.
>
> We do not need this early reset driver for Stratix10, because on
> arm64, Linux does not need the timers are that in reset. Linux is
> able to run just fine with the internal armv8 timer.
>
> Signed-off-by: Dinh Nguyen
> ---
> v2: Do not build separate reset driver for STRATIX10
> fix warning: symbol 'socfpga_reset_init' was not declared. Should
> it be static?
> ---
> arch/arm/mach-socfpga/socfpga.c | 4 ++
> drivers/reset/Kconfig | 9 +++-
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-socfpga.c | 88 +
> 4 files changed, 101 insertions(+), 1 deletion(-)
> create mode 100644 drivers/reset/reset-socfpga.c
>
> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
> index dde14f7bf2c3..cc64576c102b 100644
> --- a/arch/arm/mach-socfpga/socfpga.c
> +++ b/arch/arm/mach-socfpga/socfpga.c
> @@ -32,6 +32,8 @@ void __iomem *rst_manager_base_addr;
> void __iomem *sdr_ctl_base_addr;
> unsigned long socfpga_cpu1start_addr;
>
> +extern void __init socfpga_reset_init(void);
> +
> void __init socfpga_sysmgr_init(void)
> {
> struct device_node *np;
> @@ -64,6 +66,7 @@ static void __init socfpga_init_irq(void)
>
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void __init socfpga_arria10_init_irq(void)
> @@ -74,6 +77,7 @@ static void __init socfpga_arria10_init_irq(void)
> socfpga_init_arria10_l2_ecc();
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_arria10_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd)
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 13d28fdbdbb5..f10de5ce4753 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -100,7 +100,7 @@ config RESET_QCOM_AOSS
>
> config RESET_SIMPLE
> bool "Simple Reset Controller Driver" if COMPILE_TEST
> - default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI ||
> ARCH_ZX || ARCH_ASPEED
> + default ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX ||
> ARCH_ASPEED
> help
> This enables a simple reset controller driver for reset lines that
> that can be asserted and deasserted by toggling bits in a contiguous,
> @@ -119,6 +119,13 @@ config RESET_STM32MP157
> help
> This enables the RCC reset controller driver for STM32 MPUs.
>
> +config RESET_SOCFPGA
> + bool "SoCFPGA Reset Driver" if COMPILE_TEST && !ARCH_SOCFPGA
> + default ARCH_SOCFPGA && !ARCH_STRATIX10
> + select RESET_SIMPLE
> + help
> + This enables the reset driver for SoCFPGA.
> +
> config RESET_SUNXI
> bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
> default ARCH_SUNXI
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 4243c38228e2..d09bb41273f6 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
> obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o
> +obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
> new file mode 100644
> index ..b92769861d2b
> --- /dev/null
> +++ b/drivers/reset/reset-socfpga.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018, Intel Corporation
> + * Copied from reset-sunxi.c
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "reset-simple.h"
> +
> +#define SOCFPGA_NR_BANKS 8
> +void __init socfpga_reset_init(void);
> +
> +static int a10_reset_init(struct device_node *np)
> +{
> + struct reset_simple_data *data;
> + struct resource res;
> + resource_size_t size;
> + int ret;
> + u32 reg_offset = 0x10;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + ret = of_address_to_resource(np, 0, );
> + if (ret)
> + goto err_alloc;
> +
> + size = resource_size();
> + if (!request_mem_region(res.start, size, np->name)) {
> + ret = -EBUSY;
> + goto err_alloc;
> + }
> +
> + data->membase =
Re: [PATCH v2] ARC: HSDK: improve reset driver
Hi Eugeniy,
thank you for the update.
On Fri, 2018-09-28 at 19:28 +0300, Eugeniy Paltsev wrote:
> As for today HSDK reset driver implements only
> .reset() callback.
>
> In case of driver which implements one of standard
> reset controller usage pattern
> (call *_deassert() in probe(), call *_assert() in remove())
> that leads to inoperability of this reset driver.
>
> Improve HSDK reset driver by calling .reset() callback inside of
> .deassert() callback to avoid each reset controller
> user adaptation for work with both reset methods
> (reset() and {.assert() & .deassert()} pair)
>
> Signed-off-by: Eugeniy Paltsev
> ---
> Changes v1->v2:
> * Don't call hsdk_reset_reset in .assert callback.
>
> drivers/reset/reset-hsdk.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/reset/reset-hsdk.c b/drivers/reset/reset-hsdk.c
> index 8bce391c6943..399440f197c5 100644
> --- a/drivers/reset/reset-hsdk.c
> +++ b/drivers/reset/reset-hsdk.c
> @@ -84,8 +84,21 @@ static int hsdk_reset_reset(struct reset_controller_dev
> *rcdev,
> return ret;
> }
>
> +static int hsdk_reset_dummy(struct reset_controller_dev *rcd, unsigned long
> id)
> +{
> + return 0;
> +}
> +
> +/*
> + * Doing real reset from .assert isn't necessary/useful here. So we pass
> + * 'hsdk_reset_dummy' to .assert callback to prevent -ENOTSUPP returning by
> + * reset_control_assert() to make happy drivers which check
> + * reset_control_{assert | deassert} return status.
> + */
> static const struct reset_control_ops hsdk_reset_ops = {
> .reset = hsdk_reset_reset,
> + .assert = hsdk_reset_dummy,
Is this part really necessary though? reset_control_assert already
returns 0 in shared mode, so the only thing this does is make
reset_control_assert return 0 instead of -ENOTSUPP in exclusive mode.
The documentation states that calling reset_control_assert "on an
exclusive reset controller guarantees that the reset will be asserted."
Since this is clearly not the case with this driver, it is appropriate
to keep returning an error in this case.
If there is a driver that requests an exclusive reset control, calls
reset_control_assert, and then checks the error value to see whether
asserting the reset succeeded, it should be made aware that
we couldn't actually assert the reset line as requested. If the driver
can continue operation even though the reset line was not asserted,
it should ignore the error.
So if you need to hide this error, I'd like to know the actual case that
is fixed by this, to see if we can't fix it in a better way.
> + .deassert = hsdk_reset_reset,
This part is fine.
> };
>
> static int hsdk_reset_probe(struct platform_device *pdev)
regards
Philipp
Re: [PATCH v2] ARC: HSDK: improve reset driver
Hi Eugeniy,
thank you for the update.
On Fri, 2018-09-28 at 19:28 +0300, Eugeniy Paltsev wrote:
> As for today HSDK reset driver implements only
> .reset() callback.
>
> In case of driver which implements one of standard
> reset controller usage pattern
> (call *_deassert() in probe(), call *_assert() in remove())
> that leads to inoperability of this reset driver.
>
> Improve HSDK reset driver by calling .reset() callback inside of
> .deassert() callback to avoid each reset controller
> user adaptation for work with both reset methods
> (reset() and {.assert() & .deassert()} pair)
>
> Signed-off-by: Eugeniy Paltsev
> ---
> Changes v1->v2:
> * Don't call hsdk_reset_reset in .assert callback.
>
> drivers/reset/reset-hsdk.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/reset/reset-hsdk.c b/drivers/reset/reset-hsdk.c
> index 8bce391c6943..399440f197c5 100644
> --- a/drivers/reset/reset-hsdk.c
> +++ b/drivers/reset/reset-hsdk.c
> @@ -84,8 +84,21 @@ static int hsdk_reset_reset(struct reset_controller_dev
> *rcdev,
> return ret;
> }
>
> +static int hsdk_reset_dummy(struct reset_controller_dev *rcd, unsigned long
> id)
> +{
> + return 0;
> +}
> +
> +/*
> + * Doing real reset from .assert isn't necessary/useful here. So we pass
> + * 'hsdk_reset_dummy' to .assert callback to prevent -ENOTSUPP returning by
> + * reset_control_assert() to make happy drivers which check
> + * reset_control_{assert | deassert} return status.
> + */
> static const struct reset_control_ops hsdk_reset_ops = {
> .reset = hsdk_reset_reset,
> + .assert = hsdk_reset_dummy,
Is this part really necessary though? reset_control_assert already
returns 0 in shared mode, so the only thing this does is make
reset_control_assert return 0 instead of -ENOTSUPP in exclusive mode.
The documentation states that calling reset_control_assert "on an
exclusive reset controller guarantees that the reset will be asserted."
Since this is clearly not the case with this driver, it is appropriate
to keep returning an error in this case.
If there is a driver that requests an exclusive reset control, calls
reset_control_assert, and then checks the error value to see whether
asserting the reset succeeded, it should be made aware that
we couldn't actually assert the reset line as requested. If the driver
can continue operation even though the reset line was not asserted,
it should ignore the error.
So if you need to hide this error, I'd like to know the actual case that
is fixed by this, to see if we can't fix it in a better way.
> + .deassert = hsdk_reset_reset,
This part is fine.
> };
>
> static int hsdk_reset_probe(struct platform_device *pdev)
regards
Philipp
Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support
Hi Cheng-yi,
[adding Maxime, devicetree to Cc:, the old discussion about GPIO resets
in [4] has never been resolved]
On Tue, 2018-10-09 at 21:46 +0800, Cheng-yi Chiang wrote:
> +reset controller maintainer Philipp
>
> Hi Mark,
> Sorry for the late reply. It took me a while to investigate reset
> controller and its possible usage. I would like to figure out the
> proper way of reset handling because it is common to have a shared
> reset line for two max98927 codecs for left and right channels.
> Without supporting this usage, a simple reset-gpios change for single
> codec would not be useful, and perhaps will be duplicated if reset
> controller is a better way.
>
> Hi Philipp,
> I would like to seek your advice about whether we can use reset
> controller to support the use case where multiple devices share the
> same reset line.
>
> Let me summarize the use case:
> There are two max98927 codecs sharing the same reset line.
> The reset line is controlled by a GPIO.
> The goal is to toggle reset line once and only once.
>
> There was a similar scenario in tlv320aic3x codec driver [1].
> A static list is used in the codec driver so probe function can know
> whether it is needed to toggle reset line.
> Mark pointed out that it is not suitable to handle the shared reset
> line inside codec driver.
> A point is that this only works for multiple devices using the same
> device driver.
> He suggested to look into reset controller so I searched through the
> usage for common reset line.
>
> Here I found a shared reset line use case [2].
> With the patch [2], reset_control_reset can support this “reset once
> and for all” use case.
This patch was applied as 7da33a37b48f. So the reset framework has
support for shared reset controls where only the first reset request
actually causes a reset pulse, and all others are no-ops.
> However, I found that there are some missing pieces:
>
> Let’s assume there are two codecs c1 and c2 sharing a reset line
> controlled by GPIO.
>
> c1’s spec:
> Hold time: The minimum time to assert the reset line is t_a1.
> Release time: The minimum time to access the chip after deassert of
> the reset line is t_d1.
>
> c2’s spec:
> Hold time: The minimum time to assert the reset line is t_a2.
> Release time: The minimum time to access the chip after deassert of
> the reset line is t_d2.
>
> For both c1 and c2 to work properly, we need a reset controller that
> can assert the reset line for
> T = max(t_a1, t_a2).
>
> 1. We need reset controller to support GPIO.
I still don't like the idea of describing a separate gpio reset
controller "device" in DT very much, as this is really just a software
abstraction, not actual hardware. For example:
gpio_reset: reset-controller {
compatible = "gpio-reset";
reset-gpios = < 15 GPIO_ACTIVE_LOW>,
<
16 GPIO_ACTIVE_HIGH>;
reset-delays-us = <1>, <500>;
};
c1 {
resets = <_reset 0>; /* maps to < 15 ...> */
};
c2 {
resets = <_reset 0>;
};
What I would like better would be to let the consumers keep their reset-
gpios bindings, but add an optional hold time override in the DT:
c1 {
reset-gpios = < 15 GPIO_ACTIVE_LOW>;
reset-delays-us = <1>;
};
c2 {
reset-gpios = < 15 GPIO_ACTIVE_LOW>;
re
set-delays-us = <1>;
};
The reset framework could create a reset_control backed by a gpio
instead of a rcdev. I have an unfinished patch for this, but without the
sharing requirement adding the reset framework abstraction between gpiod
and drivers never seemed really worth it.
> 2. We need to specify hold time T to reset controller in device tree
> so it knows that it needs hold reset line for that long in its
> implementation of reset_control_reset.
Agreed. Ideally, I'd like to make this optional, and have the drivers
continue to provide the hold time if they think they know it.
> 3. Assuming we have 1 and 2 in place. In codec driver of c1, it can
> call reset_control_reset and wait for t_a1 + t_d1. In codec driver of
> c2, it can call reset_control_reset and wait for t_a2 + t_d2.
The reset framework should wait for the configured assertion time,
max(t_a1, t_a2). The drivers only should only have to wait for
t_d1 / t_d2 after reset_control_reset returns.
> We need to wait for hold time + release time because
> triggered_count is increased before reset ops is called. When the
> second driver finds that triggered_count is 1 and skip the real reset
> operation, reset ops might just begin doing its work a short time ago.
That is a good point. Maybe the reset framework should just wait for the
hold time even for the second reset. Another possibility would be to
serialize them with a mutex.
> I am not sure whether we would need a flag in reset controller to
> mark that "reset is done".
Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support
Hi Cheng-yi,
[adding Maxime, devicetree to Cc:, the old discussion about GPIO resets
in [4] has never been resolved]
On Tue, 2018-10-09 at 21:46 +0800, Cheng-yi Chiang wrote:
> +reset controller maintainer Philipp
>
> Hi Mark,
> Sorry for the late reply. It took me a while to investigate reset
> controller and its possible usage. I would like to figure out the
> proper way of reset handling because it is common to have a shared
> reset line for two max98927 codecs for left and right channels.
> Without supporting this usage, a simple reset-gpios change for single
> codec would not be useful, and perhaps will be duplicated if reset
> controller is a better way.
>
> Hi Philipp,
> I would like to seek your advice about whether we can use reset
> controller to support the use case where multiple devices share the
> same reset line.
>
> Let me summarize the use case:
> There are two max98927 codecs sharing the same reset line.
> The reset line is controlled by a GPIO.
> The goal is to toggle reset line once and only once.
>
> There was a similar scenario in tlv320aic3x codec driver [1].
> A static list is used in the codec driver so probe function can know
> whether it is needed to toggle reset line.
> Mark pointed out that it is not suitable to handle the shared reset
> line inside codec driver.
> A point is that this only works for multiple devices using the same
> device driver.
> He suggested to look into reset controller so I searched through the
> usage for common reset line.
>
> Here I found a shared reset line use case [2].
> With the patch [2], reset_control_reset can support this “reset once
> and for all” use case.
This patch was applied as 7da33a37b48f. So the reset framework has
support for shared reset controls where only the first reset request
actually causes a reset pulse, and all others are no-ops.
> However, I found that there are some missing pieces:
>
> Let’s assume there are two codecs c1 and c2 sharing a reset line
> controlled by GPIO.
>
> c1’s spec:
> Hold time: The minimum time to assert the reset line is t_a1.
> Release time: The minimum time to access the chip after deassert of
> the reset line is t_d1.
>
> c2’s spec:
> Hold time: The minimum time to assert the reset line is t_a2.
> Release time: The minimum time to access the chip after deassert of
> the reset line is t_d2.
>
> For both c1 and c2 to work properly, we need a reset controller that
> can assert the reset line for
> T = max(t_a1, t_a2).
>
> 1. We need reset controller to support GPIO.
I still don't like the idea of describing a separate gpio reset
controller "device" in DT very much, as this is really just a software
abstraction, not actual hardware. For example:
gpio_reset: reset-controller {
compatible = "gpio-reset";
reset-gpios = < 15 GPIO_ACTIVE_LOW>,
<
16 GPIO_ACTIVE_HIGH>;
reset-delays-us = <1>, <500>;
};
c1 {
resets = <_reset 0>; /* maps to < 15 ...> */
};
c2 {
resets = <_reset 0>;
};
What I would like better would be to let the consumers keep their reset-
gpios bindings, but add an optional hold time override in the DT:
c1 {
reset-gpios = < 15 GPIO_ACTIVE_LOW>;
reset-delays-us = <1>;
};
c2 {
reset-gpios = < 15 GPIO_ACTIVE_LOW>;
re
set-delays-us = <1>;
};
The reset framework could create a reset_control backed by a gpio
instead of a rcdev. I have an unfinished patch for this, but without the
sharing requirement adding the reset framework abstraction between gpiod
and drivers never seemed really worth it.
> 2. We need to specify hold time T to reset controller in device tree
> so it knows that it needs hold reset line for that long in its
> implementation of reset_control_reset.
Agreed. Ideally, I'd like to make this optional, and have the drivers
continue to provide the hold time if they think they know it.
> 3. Assuming we have 1 and 2 in place. In codec driver of c1, it can
> call reset_control_reset and wait for t_a1 + t_d1. In codec driver of
> c2, it can call reset_control_reset and wait for t_a2 + t_d2.
The reset framework should wait for the configured assertion time,
max(t_a1, t_a2). The drivers only should only have to wait for
t_d1 / t_d2 after reset_control_reset returns.
> We need to wait for hold time + release time because
> triggered_count is increased before reset ops is called. When the
> second driver finds that triggered_count is 1 and skip the real reset
> operation, reset ops might just begin doing its work a short time ago.
That is a good point. Maybe the reset framework should just wait for the
hold time even for the second reset. Another possibility would be to
serialize them with a mutex.
> I am not sure whether we would need a flag in reset controller to
> mark that "reset is done".
Re: [PATCH] reset: Fix potential use-after-free in __of_reset_control_get()
On Mon, 2018-10-08 at 15:12 +0200, Geert Uytterhoeven wrote: > Hi Philipp, > > On Mon, Oct 8, 2018 at 2:56 PM Philipp Zabel wrote: > > On Mon, 2018-10-08 at 13:14 +0200, Geert Uytterhoeven wrote: > > > Calling of_node_put() decreases the reference count of a device tree > > > object, and may free some data. > > > > > > However, the of_phandle_args structure embedding it is passed to > > > reset_controller_dev.of_xlate() after that, so it may still be accessed. > > > > > > Move the call to of_node_put() down to fix this. > > > > > > Signed-off-by: Geert Uytterhoeven > > > --- > > > drivers/reset/core.c | 15 --- > > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > > > index 225e34c56b94a2e3..bc9df10d31b4bae1 100644 > > > --- a/drivers/reset/core.c > > > +++ b/drivers/reset/core.c > > > @@ -496,27 +496,28 @@ struct reset_control *__of_reset_control_get(struct > > > device_node *node, [...] > > > /* reset_list_mutex also protects the rcdev's reset_control list */ > > > rstc = __reset_control_get_internal(rcdev, rstc_id, shared); > > > > > > +out: > > > + of_node_put(args.np); > > > mutex_unlock(_list_mutex); > > > > Thank you for the patch. I'd like to move of_node_put after mutex_unlock > > for symmetry. If you agree, I can switch the two when applying. > > No objection, thanks! Applied to reset/next with that change. regards Philipp
Re: [PATCH] reset: Fix potential use-after-free in __of_reset_control_get()
On Mon, 2018-10-08 at 15:12 +0200, Geert Uytterhoeven wrote: > Hi Philipp, > > On Mon, Oct 8, 2018 at 2:56 PM Philipp Zabel wrote: > > On Mon, 2018-10-08 at 13:14 +0200, Geert Uytterhoeven wrote: > > > Calling of_node_put() decreases the reference count of a device tree > > > object, and may free some data. > > > > > > However, the of_phandle_args structure embedding it is passed to > > > reset_controller_dev.of_xlate() after that, so it may still be accessed. > > > > > > Move the call to of_node_put() down to fix this. > > > > > > Signed-off-by: Geert Uytterhoeven > > > --- > > > drivers/reset/core.c | 15 --- > > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > > > index 225e34c56b94a2e3..bc9df10d31b4bae1 100644 > > > --- a/drivers/reset/core.c > > > +++ b/drivers/reset/core.c > > > @@ -496,27 +496,28 @@ struct reset_control *__of_reset_control_get(struct > > > device_node *node, [...] > > > /* reset_list_mutex also protects the rcdev's reset_control list */ > > > rstc = __reset_control_get_internal(rcdev, rstc_id, shared); > > > > > > +out: > > > + of_node_put(args.np); > > > mutex_unlock(_list_mutex); > > > > Thank you for the patch. I'd like to move of_node_put after mutex_unlock > > for symmetry. If you agree, I can switch the two when applying. > > No objection, thanks! Applied to reset/next with that change. regards Philipp
Re: [PATCH] reset: Fix potential use-after-free in __of_reset_control_get()
Hi Geert,
On Mon, 2018-10-08 at 13:14 +0200, Geert Uytterhoeven wrote:
> Calling of_node_put() decreases the reference count of a device tree
> object, and may free some data.
>
> However, the of_phandle_args structure embedding it is passed to
> reset_controller_dev.of_xlate() after that, so it may still be accessed.
>
> Move the call to of_node_put() down to fix this.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> drivers/reset/core.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 225e34c56b94a2e3..bc9df10d31b4bae1 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -496,27 +496,28 @@ struct reset_control *__of_reset_control_get(struct
> device_node *node,
> break;
> }
> }
> - of_node_put(args.np);
>
> if (!rcdev) {
> - mutex_unlock(_list_mutex);
> - return ERR_PTR(-EPROBE_DEFER);
> + rstc = ERR_PTR(-EPROBE_DEFER);
> + goto out;
> }
>
> if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) {
> - mutex_unlock(_list_mutex);
> - return ERR_PTR(-EINVAL);
> + rstc = ERR_PTR(-EINVAL);
> + goto out;
> }
>
> rstc_id = rcdev->of_xlate(rcdev, );
> if (rstc_id < 0) {
> - mutex_unlock(_list_mutex);
> - return ERR_PTR(rstc_id);
> + rstc = ERR_PTR(rstc_id);
> + goto out;
> }
>
> /* reset_list_mutex also protects the rcdev's reset_control list */
> rstc = __reset_control_get_internal(rcdev, rstc_id, shared);
>
> +out:
> + of_node_put(args.np);
> mutex_unlock(_list_mutex);
Thank you for the patch. I'd like to move of_node_put after mutex_unlock
for symmetry. If you agree, I can switch the two when applying.
regards
Philipp
Re: [PATCH] reset: Fix potential use-after-free in __of_reset_control_get()
Hi Geert,
On Mon, 2018-10-08 at 13:14 +0200, Geert Uytterhoeven wrote:
> Calling of_node_put() decreases the reference count of a device tree
> object, and may free some data.
>
> However, the of_phandle_args structure embedding it is passed to
> reset_controller_dev.of_xlate() after that, so it may still be accessed.
>
> Move the call to of_node_put() down to fix this.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> drivers/reset/core.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 225e34c56b94a2e3..bc9df10d31b4bae1 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -496,27 +496,28 @@ struct reset_control *__of_reset_control_get(struct
> device_node *node,
> break;
> }
> }
> - of_node_put(args.np);
>
> if (!rcdev) {
> - mutex_unlock(_list_mutex);
> - return ERR_PTR(-EPROBE_DEFER);
> + rstc = ERR_PTR(-EPROBE_DEFER);
> + goto out;
> }
>
> if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) {
> - mutex_unlock(_list_mutex);
> - return ERR_PTR(-EINVAL);
> + rstc = ERR_PTR(-EINVAL);
> + goto out;
> }
>
> rstc_id = rcdev->of_xlate(rcdev, );
> if (rstc_id < 0) {
> - mutex_unlock(_list_mutex);
> - return ERR_PTR(rstc_id);
> + rstc = ERR_PTR(rstc_id);
> + goto out;
> }
>
> /* reset_list_mutex also protects the rcdev's reset_control list */
> rstc = __reset_control_get_internal(rcdev, rstc_id, shared);
>
> +out:
> + of_node_put(args.np);
> mutex_unlock(_list_mutex);
Thank you for the patch. I'd like to move of_node_put after mutex_unlock
for symmetry. If you agree, I can switch the two when applying.
regards
Philipp
Re: [PATCH trivial] reset: Improve reset controller kernel docs
Hi Geert, On Mon, 2018-10-08 at 13:15 +0200, Geert Uytterhoeven wrote: > Grammar and indentation fixes. > > Signed-off-by: Geert Uytterhoeven > --- > include/linux/reset.h | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/linux/reset.h b/include/linux/reset.h > index 29af6d6b2f4b8103..d01ea059e2beee6e 100644 > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -138,7 +138,7 @@ __must_check reset_control_get_exclusive(struct device > *dev, const char *id) > * > * Returns a struct reset_control or IS_ERR() condition containing errno. > * This function is intended for use with reset-controls which are shared > - * between hardware-blocks. > + * among hardware blocks. To my ears "between" sounds better, because it evokes the idea that each hardware block individually must take care not to disturb any of the others (like sharing a bike between a few riders, instead of among them). Also, in many cases the sharing occurs between just two hardware blocks, which makes "among" sound weird. Of course I'm not a native speaker, so maybe I'm completely wrong. The other fixes I agree with. regards Philipp
Re: [PATCH trivial] reset: Improve reset controller kernel docs
Hi Geert, On Mon, 2018-10-08 at 13:15 +0200, Geert Uytterhoeven wrote: > Grammar and indentation fixes. > > Signed-off-by: Geert Uytterhoeven > --- > include/linux/reset.h | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/linux/reset.h b/include/linux/reset.h > index 29af6d6b2f4b8103..d01ea059e2beee6e 100644 > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -138,7 +138,7 @@ __must_check reset_control_get_exclusive(struct device > *dev, const char *id) > * > * Returns a struct reset_control or IS_ERR() condition containing errno. > * This function is intended for use with reset-controls which are shared > - * between hardware-blocks. > + * among hardware blocks. To my ears "between" sounds better, because it evokes the idea that each hardware block individually must take care not to disturb any of the others (like sharing a bike between a few riders, instead of among them). Also, in many cases the sharing occurs between just two hardware blocks, which makes "among" sound weird. Of course I'm not a native speaker, so maybe I'm completely wrong. The other fixes I agree with. regards Philipp
Re: [PATCH] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh, On Fri, 2018-10-05 at 15:23 -0500, Dinh Nguyen wrote: > Hi Philipp, > > I apologize, but I just realized that I forgot to test this patch > against the SoCFPGA ARM64 platform. I just tested against that platform > and this patch is preventing that board from booting. > > I need to redo this patch. > > If its not too late, can you remove this patch from reset/next? That's fine, I've dropped the patch from reset/next. regards Philipp
Re: [PATCH] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh, On Fri, 2018-10-05 at 15:23 -0500, Dinh Nguyen wrote: > Hi Philipp, > > I apologize, but I just realized that I forgot to test this patch > against the SoCFPGA ARM64 platform. I just tested against that platform > and this patch is preventing that board from booting. > > I need to redo this patch. > > If its not too late, can you remove this patch from reset/next? That's fine, I've dropped the patch from reset/next. regards Philipp
Re: [PATCH] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Fri, 2018-10-05 at 10:17 -0500, Dinh Nguyen wrote:
[...]
> > > +static int a10_reset_init(struct device_node *np)
> > > +{
> > > + struct reset_simple_data *data;
> > > + struct resource res;
> > > + resource_size_t size;
> > > + int ret;
> > > + u32 reg_offset = 0;
> >
> > ... now it is 0x0.
> > I think this should be
> >
> > u32 reg_offset = 0x10;
> >
> > to avoid breaking SocFPGA with ancient device trees.
[...]
> > Could we keep the SOCFPGA_NR_BANKS #define to reduce the amount of magic
> > numbers?
> >
> > I can make both changes when applying if you want.
> >
>
> If you don't mind, that would be great!
Done, applied to reset/next with the above changes.
I get the following build warning:
drivers/reset/reset-socfpga.c:81:13: warning: symbol 'socfpga_reset_init' was
not declared. Should it be static?
It would be nice to put the declaration into a header in
include/soc/socfpga (or wherever is more appropriate) at some point.
regards
Philipp
Re: [PATCH] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Fri, 2018-10-05 at 10:17 -0500, Dinh Nguyen wrote:
[...]
> > > +static int a10_reset_init(struct device_node *np)
> > > +{
> > > + struct reset_simple_data *data;
> > > + struct resource res;
> > > + resource_size_t size;
> > > + int ret;
> > > + u32 reg_offset = 0;
> >
> > ... now it is 0x0.
> > I think this should be
> >
> > u32 reg_offset = 0x10;
> >
> > to avoid breaking SocFPGA with ancient device trees.
[...]
> > Could we keep the SOCFPGA_NR_BANKS #define to reduce the amount of magic
> > numbers?
> >
> > I can make both changes when applying if you want.
> >
>
> If you don't mind, that would be great!
Done, applied to reset/next with the above changes.
I get the following build warning:
drivers/reset/reset-socfpga.c:81:13: warning: symbol 'socfpga_reset_init' was
not declared. Should it be static?
It would be nice to put the declaration into a header in
include/soc/socfpga (or wherever is more appropriate) at some point.
regards
Philipp
Re: [PATCH v2 4/4] PCI: imx: Add PME_Turn_Off support
Hi Lorenzo, On Fri, 2018-10-05 at 11:29 +0100, Lorenzo Pieralisi wrote: > On Thu, Oct 04, 2018 at 04:34:30PM +, Leonard Crestez wrote: > > When the root complex suspends it must send a PME_Turn_Off TLP. > > Implement this by asserting the "turnoff" reset. > > > > On imx7d this functionality is part of the SRC and exposed through the > > linux reset-controller subsystem. On imx6 equivalent bits are in the > > IOMUXC GPR area which the imx6-pcie driver accesses directly. > > Nit: I am merging the series, please define what SRC and GPR are so > that I can update the commit log accordingly, it must be clear to > anyone reading it. SRC is the System Reset Controller, which controls reset signals to all kinds of devices, among them the PCIe PHY. IOMUXC GPR is a General Purpose Register range in the IOMUXC (pinmux controller) that is used to control various signals all over the SoC. regards Philipp
Re: [PATCH v2 4/4] PCI: imx: Add PME_Turn_Off support
Hi Lorenzo, On Fri, 2018-10-05 at 11:29 +0100, Lorenzo Pieralisi wrote: > On Thu, Oct 04, 2018 at 04:34:30PM +, Leonard Crestez wrote: > > When the root complex suspends it must send a PME_Turn_Off TLP. > > Implement this by asserting the "turnoff" reset. > > > > On imx7d this functionality is part of the SRC and exposed through the > > linux reset-controller subsystem. On imx6 equivalent bits are in the > > IOMUXC GPR area which the imx6-pcie driver accesses directly. > > Nit: I am merging the series, please define what SRC and GPR are so > that I can update the commit log accordingly, it must be clear to > anyone reading it. SRC is the System Reset Controller, which controls reset signals to all kinds of devices, among them the PCIe PHY. IOMUXC GPR is a General Purpose Register range in the IOMUXC (pinmux controller) that is used to control various signals all over the SoC. regards Philipp
Re: [PATCH v4 03/11] gpu: ipu-v3: Add planar support to interlaced scan
On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote:
> To support interlaced scan with planar formats, cpmem SLUV must
> be programmed with the correct chroma line stride. For full and
> partial planar 4:2:2 (YUV422P, NV16), chroma line stride must
> be doubled. For full and partial planar 4:2:0 (YUV420, YVU420, NV12),
> chroma line stride must _not_ be doubled, since a single chroma line
> is shared by two luma lines.
>
> Signed-off-by: Steve Longerbeam
> ---
> drivers/gpu/ipu-v3/ipu-cpmem.c | 26 +++--
> drivers/staging/media/imx/imx-ic-prpencvf.c | 3 ++-
> drivers/staging/media/imx/imx-media-csi.c | 3 ++-
> include/video/imx-ipu-v3.h | 3 ++-
> 4 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
> index a9d2501500a1..d41df8034c5b 100644
> --- a/drivers/gpu/ipu-v3/ipu-cpmem.c
> +++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
> @@ -273,9 +273,10 @@ void ipu_cpmem_set_uv_offset(struct ipuv3_channel *ch,
> u32 u_off, u32 v_off)
> }
> EXPORT_SYMBOL_GPL(ipu_cpmem_set_uv_offset);
>
> -void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride)
> +void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride,
> +u32 pixelformat)
> {
> - u32 ilo, sly;
> + u32 ilo, sly, sluv;
>
> if (stride < 0) {
> stride = -stride;
> @@ -286,9 +287,30 @@ void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch,
> int stride)
>
> sly = (stride * 2) - 1;
>
> + switch (pixelformat) {
> + case V4L2_PIX_FMT_YUV420:
> + case V4L2_PIX_FMT_YVU420:
> + sluv = stride / 2 - 1;
> + break;
> + case V4L2_PIX_FMT_NV12:
> + sluv = stride - 1;
> + break;
> + case V4L2_PIX_FMT_YUV422P:
> + sluv = stride - 1;
> + break;
> + case V4L2_PIX_FMT_NV16:
> + sluv = stride * 2 - 1;
> + break;
> + default:
> + sluv = 0;
> + break;
> + }
> +
> ipu_ch_param_write_field(ch, IPU_FIELD_SO, 1);
> ipu_ch_param_write_field(ch, IPU_FIELD_ILO, ilo);
> ipu_ch_param_write_field(ch, IPU_FIELD_SLY, sly);
> + if (sluv)
> + ipu_ch_param_write_field(ch, IPU_FIELD_SLUV, sluv);
> };
> EXPORT_SYMBOL_GPL(ipu_cpmem_interlaced_scan);
[...]
Reviewed-by: Philipp Zabel
and
Acked-by: Philipp Zabel
to be merged with the rest of the series via the media tree. I'll take
care not to introduce nontrivial conflicts in imx-drm.
regards
Philipp
Re: [PATCH v4 03/11] gpu: ipu-v3: Add planar support to interlaced scan
On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote:
> To support interlaced scan with planar formats, cpmem SLUV must
> be programmed with the correct chroma line stride. For full and
> partial planar 4:2:2 (YUV422P, NV16), chroma line stride must
> be doubled. For full and partial planar 4:2:0 (YUV420, YVU420, NV12),
> chroma line stride must _not_ be doubled, since a single chroma line
> is shared by two luma lines.
>
> Signed-off-by: Steve Longerbeam
> ---
> drivers/gpu/ipu-v3/ipu-cpmem.c | 26 +++--
> drivers/staging/media/imx/imx-ic-prpencvf.c | 3 ++-
> drivers/staging/media/imx/imx-media-csi.c | 3 ++-
> include/video/imx-ipu-v3.h | 3 ++-
> 4 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
> index a9d2501500a1..d41df8034c5b 100644
> --- a/drivers/gpu/ipu-v3/ipu-cpmem.c
> +++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
> @@ -273,9 +273,10 @@ void ipu_cpmem_set_uv_offset(struct ipuv3_channel *ch,
> u32 u_off, u32 v_off)
> }
> EXPORT_SYMBOL_GPL(ipu_cpmem_set_uv_offset);
>
> -void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride)
> +void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride,
> +u32 pixelformat)
> {
> - u32 ilo, sly;
> + u32 ilo, sly, sluv;
>
> if (stride < 0) {
> stride = -stride;
> @@ -286,9 +287,30 @@ void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch,
> int stride)
>
> sly = (stride * 2) - 1;
>
> + switch (pixelformat) {
> + case V4L2_PIX_FMT_YUV420:
> + case V4L2_PIX_FMT_YVU420:
> + sluv = stride / 2 - 1;
> + break;
> + case V4L2_PIX_FMT_NV12:
> + sluv = stride - 1;
> + break;
> + case V4L2_PIX_FMT_YUV422P:
> + sluv = stride - 1;
> + break;
> + case V4L2_PIX_FMT_NV16:
> + sluv = stride * 2 - 1;
> + break;
> + default:
> + sluv = 0;
> + break;
> + }
> +
> ipu_ch_param_write_field(ch, IPU_FIELD_SO, 1);
> ipu_ch_param_write_field(ch, IPU_FIELD_ILO, ilo);
> ipu_ch_param_write_field(ch, IPU_FIELD_SLY, sly);
> + if (sluv)
> + ipu_ch_param_write_field(ch, IPU_FIELD_SLUV, sluv);
> };
> EXPORT_SYMBOL_GPL(ipu_cpmem_interlaced_scan);
[...]
Reviewed-by: Philipp Zabel
and
Acked-by: Philipp Zabel
to be merged with the rest of the series via the media tree. I'll take
care not to introduce nontrivial conflicts in imx-drm.
regards
Philipp
Re: [PATCH v3 0/6] Add support for PDC Global on SDM845 SoCs
Hi Sibi, Bjorn, On Thu, 2018-10-04 at 23:57 +0530, Sibi Sankar wrote: > On 09/04/2018 01:06 AM, Bjorn Andersson wrote: [...] > > Philipp, there's no compile time dependencies between the PDC and > > remoteproc patches in this series. Will you take these two through your > > tree and I'll take the remaining four through the remoteproc tree? > > > > Regards, > > Bjorn > >Hey Phillip, > > Will the PDC driver make the cut for reset/next this time for the > 4.20rc? I have applied patches 1 and 2 to reset/next. I'll send a pull request early next week. regards Philipp
Re: [PATCH v3 0/6] Add support for PDC Global on SDM845 SoCs
Hi Sibi, Bjorn, On Thu, 2018-10-04 at 23:57 +0530, Sibi Sankar wrote: > On 09/04/2018 01:06 AM, Bjorn Andersson wrote: [...] > > Philipp, there's no compile time dependencies between the PDC and > > remoteproc patches in this series. Will you take these two through your > > tree and I'll take the remaining four through the remoteproc tree? > > > > Regards, > > Bjorn > >Hey Phillip, > > Will the PDC driver make the cut for reset/next this time for the > 4.20rc? I have applied patches 1 and 2 to reset/next. I'll send a pull request early next week. regards Philipp
Re: [PATCH v2 1/4] reset: imx7: Add PCIE_CTRL_APPS_TURNOFF
Hi Leonard,
On Thu, 2018-10-04 at 16:34 +, Leonard Crestez wrote:
> This is required for the imx pci driver to send the PME_Turn_Off TLP.
>
> Signed-off-by: Leonard Crestez
> Acked-by: Rob Herring
> ---
> drivers/reset/reset-imx7.c | 1 +
> include/dt-bindings/reset/imx7-reset.h | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 97d9f08271c5..77911fa8f31d 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -65,10 +65,11 @@ static const struct imx7_src_signal
> imx7_src_signals[IMX7_RESET_NUM] = {
> [IMX7_RESET_MIPI_PHY_MRST] = { SRC_MIPIPHY_RCR, BIT(1) },
> [IMX7_RESET_MIPI_PHY_SRST] = { SRC_MIPIPHY_RCR, BIT(2) },
> [IMX7_RESET_PCIEPHY]= { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) },
> [IMX7_RESET_PCIEPHY_PERST] = { SRC_PCIEPHY_RCR, BIT(3) },
> [IMX7_RESET_PCIE_CTRL_APPS_EN] = { SRC_PCIEPHY_RCR, BIT(6) },
> + [IMX7_RESET_PCIE_CTRL_APPS_TURNOFF] = { SRC_PCIEPHY_RCR, BIT(11) },
> [IMX7_RESET_DDRC_PRST] = { SRC_DDRC_RCR, BIT(0) },
> [IMX7_RESET_DDRC_CORE_RST] = { SRC_DDRC_RCR, BIT(1) },
> };
>
> static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
> diff --git a/include/dt-bindings/reset/imx7-reset.h
> b/include/dt-bindings/reset/imx7-reset.h
> index 63948170c7b2..31b3f87dde9a 100644
> --- a/include/dt-bindings/reset/imx7-reset.h
> +++ b/include/dt-bindings/reset/imx7-reset.h
> @@ -54,9 +54,11 @@
> */
> #define IMX7_RESET_PCIE_CTRL_APPS_EN 22
> #define IMX7_RESET_DDRC_PRST 23
> #define IMX7_RESET_DDRC_CORE_RST 24
>
> -#define IMX7_RESET_NUM 25
> +#define IMX7_RESET_PCIE_CTRL_APPS_TURNOFF 25
> +
> +#define IMX7_RESET_NUM 26
>
> #endif
This is contained enough to be merged with the rest of the series,
patches 1 and 2:
Acked-by: Philipp Zabel
Let me know if I should pick them up instead.
regards
Philipp
Re: [PATCH v2 1/4] reset: imx7: Add PCIE_CTRL_APPS_TURNOFF
Hi Leonard,
On Thu, 2018-10-04 at 16:34 +, Leonard Crestez wrote:
> This is required for the imx pci driver to send the PME_Turn_Off TLP.
>
> Signed-off-by: Leonard Crestez
> Acked-by: Rob Herring
> ---
> drivers/reset/reset-imx7.c | 1 +
> include/dt-bindings/reset/imx7-reset.h | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 97d9f08271c5..77911fa8f31d 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -65,10 +65,11 @@ static const struct imx7_src_signal
> imx7_src_signals[IMX7_RESET_NUM] = {
> [IMX7_RESET_MIPI_PHY_MRST] = { SRC_MIPIPHY_RCR, BIT(1) },
> [IMX7_RESET_MIPI_PHY_SRST] = { SRC_MIPIPHY_RCR, BIT(2) },
> [IMX7_RESET_PCIEPHY]= { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) },
> [IMX7_RESET_PCIEPHY_PERST] = { SRC_PCIEPHY_RCR, BIT(3) },
> [IMX7_RESET_PCIE_CTRL_APPS_EN] = { SRC_PCIEPHY_RCR, BIT(6) },
> + [IMX7_RESET_PCIE_CTRL_APPS_TURNOFF] = { SRC_PCIEPHY_RCR, BIT(11) },
> [IMX7_RESET_DDRC_PRST] = { SRC_DDRC_RCR, BIT(0) },
> [IMX7_RESET_DDRC_CORE_RST] = { SRC_DDRC_RCR, BIT(1) },
> };
>
> static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
> diff --git a/include/dt-bindings/reset/imx7-reset.h
> b/include/dt-bindings/reset/imx7-reset.h
> index 63948170c7b2..31b3f87dde9a 100644
> --- a/include/dt-bindings/reset/imx7-reset.h
> +++ b/include/dt-bindings/reset/imx7-reset.h
> @@ -54,9 +54,11 @@
> */
> #define IMX7_RESET_PCIE_CTRL_APPS_EN 22
> #define IMX7_RESET_DDRC_PRST 23
> #define IMX7_RESET_DDRC_CORE_RST 24
>
> -#define IMX7_RESET_NUM 25
> +#define IMX7_RESET_PCIE_CTRL_APPS_TURNOFF 25
> +
> +#define IMX7_RESET_NUM 26
>
> #endif
This is contained enough to be merged with the rest of the series,
patches 1 and 2:
Acked-by: Philipp Zabel
Let me know if I should pick them up instead.
regards
Philipp
Re: [PATCH 4/4] PCI: imx: Add PME_Turn_Off support
On Thu, 2018-10-04 at 13:20 +, Leonard Crestez wrote:
> On Thu, 2018-10-04 at 10:59 +0200, Lucas Stach wrote:
> > Am Montag, den 01.10.2018, 22:53 +0300 schrieb Leonard Crestez:
> > > When the root complex suspends it must send a PME_Turn_Off TLP.
> > > Implement this by asserting the "turnoff" reset.
> > >
> > > +static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> > > +{
> > > + reset_control_assert(imx6_pcie->turnoff_reset);
> > > + reset_control_deassert(imx6_pcie->turnoff_reset);
> >
> > I'm a bit surprised to see no timing requirements here. I would have
> > expected that there is a minimum time from asserting the reset, so the
> > turnoff message gets transmitted to the EP before the clocks are
> > stopped.
>
> According to the PCI standard after PME_Turn_Off is sent all components
> must respond with PME_TO_Ack. There doesn't seem to be any bit exposed
> to check if acks were received but the standard recommends a 1-10ms
> timeout after which you should proceed anyway.
>
> It seems the NXP vendor tree has an udelay(1000) which I missed, it
> seems like an acceptable solution. Or maybe it should be msleep(10)?
Maybe usleep_range(1000, 1) ?
regards
Philipp
Re: [PATCH 4/4] PCI: imx: Add PME_Turn_Off support
On Thu, 2018-10-04 at 13:20 +, Leonard Crestez wrote:
> On Thu, 2018-10-04 at 10:59 +0200, Lucas Stach wrote:
> > Am Montag, den 01.10.2018, 22:53 +0300 schrieb Leonard Crestez:
> > > When the root complex suspends it must send a PME_Turn_Off TLP.
> > > Implement this by asserting the "turnoff" reset.
> > >
> > > +static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> > > +{
> > > + reset_control_assert(imx6_pcie->turnoff_reset);
> > > + reset_control_deassert(imx6_pcie->turnoff_reset);
> >
> > I'm a bit surprised to see no timing requirements here. I would have
> > expected that there is a minimum time from asserting the reset, so the
> > turnoff message gets transmitted to the EP before the clocks are
> > stopped.
>
> According to the PCI standard after PME_Turn_Off is sent all components
> must respond with PME_TO_Ack. There doesn't seem to be any bit exposed
> to check if acks were received but the standard recommends a 1-10ms
> timeout after which you should proceed anyway.
>
> It seems the NXP vendor tree has an udelay(1000) which I missed, it
> seems like an acceptable solution. Or maybe it should be msleep(10)?
Maybe usleep_range(1000, 1) ?
regards
Philipp
Re: [PATCH trivial] reset: Grammar s/more then once/more than once/
On Wed, 2018-09-26 at 15:20 +0200, Geert Uytterhoeven wrote: > Signed-off-by: Geert Uytterhoeven > --- > include/linux/reset.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/reset.h b/include/linux/reset.h > index 09732c36f3515a1e..29af6d6b2f4b8103 100644 > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -116,7 +116,7 @@ static inline int device_reset_optional(struct device > *dev) > * @id: reset line name > * > * Returns a struct reset_control or IS_ERR() condition containing errno. > - * If this function is called more then once for the same reset_control it > will > + * If this function is called more than once for the same reset_control it > will > * return -EBUSY. > * > * See reset_control_get_shared for details on shared references to Thank you. Applied to reset/next, with a line of commit description added. regards Philipp
Re: [PATCH] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Mon, 2018-09-17 at 09:50 -0500, Dinh Nguyen wrote:
> Create a separate reset driver that uses the reset operations in reset-simple.
> The reset driver for the SoCFPGA platform needs to register early in order to
> be able bring online timers that needed early in the kernel bootup.
>
> Signed-off-by: Dinh Nguyen
> ---
> arch/arm/mach-socfpga/socfpga.c | 4 ++
> drivers/reset/Kconfig | 7
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-simple.c| 17 -
> drivers/reset/reset-socfpga.c | 85
> +
> 5 files changed, 97 insertions(+), 17 deletions(-)
> create mode 100644 drivers/reset/reset-socfpga.c
>
> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
> index dde14f7..cc64576 100644
> --- a/arch/arm/mach-socfpga/socfpga.c
> +++ b/arch/arm/mach-socfpga/socfpga.c
> @@ -32,6 +32,8 @@ void __iomem *rst_manager_base_addr;
> void __iomem *sdr_ctl_base_addr;
> unsigned long socfpga_cpu1start_addr;
>
> +extern void __init socfpga_reset_init(void);
> +
> void __init socfpga_sysmgr_init(void)
> {
> struct device_node *np;
> @@ -64,6 +66,7 @@ static void __init socfpga_init_irq(void)
>
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void __init socfpga_arria10_init_irq(void)
> @@ -74,6 +77,7 @@ static void __init socfpga_arria10_init_irq(void)
> socfpga_init_arria10_l2_ecc();
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_arria10_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd)
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 13d28fd..dcc5f1d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -119,6 +119,13 @@ config RESET_STM32MP157
> help
> This enables the RCC reset controller driver for STM32 MPUs.
>
> +config RESET_SOCFPGA
> + bool "SoCFPGA Reset Driver" if COMPILE_TEST && !ARCH_SOCFPGA
> + default ARCH_SOCFPGA
> + select RESET_SIMPLE
> + help
> + This enables the reset driver for SoCFPGA.
> +
> config RESET_SUNXI
> bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
> default ARCH_SUNXI
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 4243c38..d09bb41 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
> obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o
> +obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index a91107f..483824f 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -106,21 +106,12 @@ struct reset_simple_devdata {
> bool status_active_low;
> };
>
> -#define SOCFPGA_NR_BANKS 8
> -
> -static const struct reset_simple_devdata reset_simple_socfpga = {
> - .reg_offset = 0x10,
Before, the default reg_offset for DTs missing the altr,modrst-offset
property was 0x10 ...
> - .nr_resets = SOCFPGA_NR_BANKS * 32,
> - .status_active_low = true,
> -};
> -
[...]
> +static int a10_reset_init(struct device_node *np)
> +{
> + struct reset_simple_data *data;
> + struct resource res;
> + resource_size_t size;
> + int ret;
> + u32 reg_offset = 0;
... now it is 0x0.
I think this should be
u32 reg_offset = 0x10;
to avoid breaking SocFPGA with ancient device trees.
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + ret = of_address_to_resource(np, 0, );
> + if (ret)
> + goto err_alloc;
> +
> + size = resource_size();
> + if (!request_mem_region(res.start, size, np->name)) {
> + ret = -EBUSY;
> + goto err_alloc;
> + }
> +
> + data->membase = ioremap(res.start, size);
> + if (!data->membase) {
> + ret = -ENOMEM;
> + goto err_alloc;
> + }
> +
> + if (of_property_read_u32(np, "altr,modrst-offset", _offset))
> + pr_warn("missing altr,modrst-offset property, assuming 0x0\n");
> + data->membase += reg_offset;
> +
> + spin_lock_init(>lock);
> +
> + data->rcdev.owner = THIS_MODULE;
> + data->rcdev.nr_resets = 32 * 8;
Could we keep the SOCFPGA_NR_BANKS #define to reduce the amount of magic
numbers?
I can make both changes when applying if you want.
regards
Philipp
Re: [PATCH trivial] reset: Grammar s/more then once/more than once/
On Wed, 2018-09-26 at 15:20 +0200, Geert Uytterhoeven wrote: > Signed-off-by: Geert Uytterhoeven > --- > include/linux/reset.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/reset.h b/include/linux/reset.h > index 09732c36f3515a1e..29af6d6b2f4b8103 100644 > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -116,7 +116,7 @@ static inline int device_reset_optional(struct device > *dev) > * @id: reset line name > * > * Returns a struct reset_control or IS_ERR() condition containing errno. > - * If this function is called more then once for the same reset_control it > will > + * If this function is called more than once for the same reset_control it > will > * return -EBUSY. > * > * See reset_control_get_shared for details on shared references to Thank you. Applied to reset/next, with a line of commit description added. regards Philipp
Re: [PATCH] reset: socfpga: add an early reset driver for SoCFPGA
Hi Dinh,
On Mon, 2018-09-17 at 09:50 -0500, Dinh Nguyen wrote:
> Create a separate reset driver that uses the reset operations in reset-simple.
> The reset driver for the SoCFPGA platform needs to register early in order to
> be able bring online timers that needed early in the kernel bootup.
>
> Signed-off-by: Dinh Nguyen
> ---
> arch/arm/mach-socfpga/socfpga.c | 4 ++
> drivers/reset/Kconfig | 7
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-simple.c| 17 -
> drivers/reset/reset-socfpga.c | 85
> +
> 5 files changed, 97 insertions(+), 17 deletions(-)
> create mode 100644 drivers/reset/reset-socfpga.c
>
> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
> index dde14f7..cc64576 100644
> --- a/arch/arm/mach-socfpga/socfpga.c
> +++ b/arch/arm/mach-socfpga/socfpga.c
> @@ -32,6 +32,8 @@ void __iomem *rst_manager_base_addr;
> void __iomem *sdr_ctl_base_addr;
> unsigned long socfpga_cpu1start_addr;
>
> +extern void __init socfpga_reset_init(void);
> +
> void __init socfpga_sysmgr_init(void)
> {
> struct device_node *np;
> @@ -64,6 +66,7 @@ static void __init socfpga_init_irq(void)
>
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void __init socfpga_arria10_init_irq(void)
> @@ -74,6 +77,7 @@ static void __init socfpga_arria10_init_irq(void)
> socfpga_init_arria10_l2_ecc();
> if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> socfpga_init_arria10_ocram_ecc();
> + socfpga_reset_init();
> }
>
> static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd)
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 13d28fd..dcc5f1d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -119,6 +119,13 @@ config RESET_STM32MP157
> help
> This enables the RCC reset controller driver for STM32 MPUs.
>
> +config RESET_SOCFPGA
> + bool "SoCFPGA Reset Driver" if COMPILE_TEST && !ARCH_SOCFPGA
> + default ARCH_SOCFPGA
> + select RESET_SIMPLE
> + help
> + This enables the reset driver for SoCFPGA.
> +
> config RESET_SUNXI
> bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
> default ARCH_SUNXI
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 4243c38..d09bb41 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
> obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o
> +obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index a91107f..483824f 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -106,21 +106,12 @@ struct reset_simple_devdata {
> bool status_active_low;
> };
>
> -#define SOCFPGA_NR_BANKS 8
> -
> -static const struct reset_simple_devdata reset_simple_socfpga = {
> - .reg_offset = 0x10,
Before, the default reg_offset for DTs missing the altr,modrst-offset
property was 0x10 ...
> - .nr_resets = SOCFPGA_NR_BANKS * 32,
> - .status_active_low = true,
> -};
> -
[...]
> +static int a10_reset_init(struct device_node *np)
> +{
> + struct reset_simple_data *data;
> + struct resource res;
> + resource_size_t size;
> + int ret;
> + u32 reg_offset = 0;
... now it is 0x0.
I think this should be
u32 reg_offset = 0x10;
to avoid breaking SocFPGA with ancient device trees.
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + ret = of_address_to_resource(np, 0, );
> + if (ret)
> + goto err_alloc;
> +
> + size = resource_size();
> + if (!request_mem_region(res.start, size, np->name)) {
> + ret = -EBUSY;
> + goto err_alloc;
> + }
> +
> + data->membase = ioremap(res.start, size);
> + if (!data->membase) {
> + ret = -ENOMEM;
> + goto err_alloc;
> + }
> +
> + if (of_property_read_u32(np, "altr,modrst-offset", _offset))
> + pr_warn("missing altr,modrst-offset property, assuming 0x0\n");
> + data->membase += reg_offset;
> +
> + spin_lock_init(>lock);
> +
> + data->rcdev.owner = THIS_MODULE;
> + data->rcdev.nr_resets = 32 * 8;
Could we keep the SOCFPGA_NR_BANKS #define to reduce the amount of magic
numbers?
I can make both changes when applying if you want.
regards
Philipp
Re: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls
Hi Geert,
On Wed, 2018-09-19 at 17:24 +0200, Geert Uytterhoeven wrote:
> On Wed, Sep 19, 2018 at 4:58 PM Philipp Zabel wrote:
[...]
> > I consider requesting exclusive access to a shared reset line a misuse
> > of the API. Are there such cases? Can they be fixed?
>
> I guess there are plenty.
I did a cursory search for drivers that request exclusive reset controls
for resets that have multiple phandle references in the corresponding
DT. So far I have found none.
> Whether a line is shared or dedicated depends on SoC integration.
>
> The issue is that a driver requesting exclusive access has no way to know
> if the reset line is dedicated to its device or not. If no other
> driver requested the reset control (most drivers don't use reset
> controls), it will succeed.
True. It would be great to have a way to make sure an exclusive request
for a shared reset line never succeeds.
> > > Sometimes a driver needs to reset a specific hardware block, and be 100%
> > > sure it has no impact on other hardware blocks. This is e.g. the case
> > > for virtualization with device pass-through, where the host wants to
> > > reset any exported device before and after exporting it for use by the
> > > guest, for isolation.
> > >
> > > Hence a new flag for dedicated resets is added to the internal methods,
> > > with a new public reset_control_get_dedicated() method, to obtain an
> > > exclusive handle to a reset that is dedicated to one specific hardware
> > > block.
> >
> > I'm not sure a new flag is necessary, this is what exclusive resets
> > should be.
>
> So perhaps the check should be done for the existing exclusive resets
> instead, without adding a new flag?
That would be my preference, if possible.
> > Also I fear there will be confusion about the difference between
> > exclusive (refering to the reset control) and dedicated (refering to
> > the reset line) reset controls.
>
> Indeed, exclusive has multiple meanings here:
> 1. Exclusive vs. shared access to the reset control,
> 2. Reset line is dedicated to a single device, or shared with multiple
> devices.
2. is the more important factor, and that's what I have in mind when
talking about exclusive vs. shared resets. Admittedly, the kernel doc
comments only mention 1.
> If we can simplify (exclusive == dedicated, 1.shared == 2.shared), life
> can become simpler.
Well, it still has to be possible for drivers to request 1.shared
control over a dedicated reset line, just because the same driver may
work on multiple SoCs, only some of which have that reset line 2.shared.
But if we could make sure that exclusive requests are only possible for
dedicated reset lines, I'd be happier.
> > > - I think __device_reset() should call __reset_control_get() with
> > > dedicated=true. However, that will impact existing users,
> >
> > Which ones? And how?
>
> I didn't actually check which drivers.
> If a reset is not dedicated, device_reset{,_optional}() will suddenly
> start to fail if
> a reset turns out to be not dedicated.
> Well, currently the device will be reset multiple times, so people would
> already have noticed...
Exactly. Naive exclusive control, as currently implemented, is bound to
fail if the reset line is shared. I am not aware of any cases where this
currently happens.
Of course there could always be those fragile cases where something just
works by accident and lucky timing.
[...]
> > I want to hear the device tree maintainers' opinion about this.
> > I'd very much like to have such a check for exclusive resets, but my
> > understanding is that we are not allowed to make the assumption that
> > other nodes' "reset" properties follow the common reset signal device
> > tree bindings.
> >
> > Maybe this is something that should be checked in a device tree
> > validation step?
>
> We already have SoCs where reset lines are shared among multiple on-chip
> devices. So dtc validation won't work, and a runtime check is needed.
Right, the dtb would have to be validated against driver code to get the
information whether a given phandle might be requested exclusively at
some point.
It would be better if this could be checked at runtime.
regards
Philipp
Re: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls
Hi Geert,
On Wed, 2018-09-19 at 17:24 +0200, Geert Uytterhoeven wrote:
> On Wed, Sep 19, 2018 at 4:58 PM Philipp Zabel wrote:
[...]
> > I consider requesting exclusive access to a shared reset line a misuse
> > of the API. Are there such cases? Can they be fixed?
>
> I guess there are plenty.
I did a cursory search for drivers that request exclusive reset controls
for resets that have multiple phandle references in the corresponding
DT. So far I have found none.
> Whether a line is shared or dedicated depends on SoC integration.
>
> The issue is that a driver requesting exclusive access has no way to know
> if the reset line is dedicated to its device or not. If no other
> driver requested the reset control (most drivers don't use reset
> controls), it will succeed.
True. It would be great to have a way to make sure an exclusive request
for a shared reset line never succeeds.
> > > Sometimes a driver needs to reset a specific hardware block, and be 100%
> > > sure it has no impact on other hardware blocks. This is e.g. the case
> > > for virtualization with device pass-through, where the host wants to
> > > reset any exported device before and after exporting it for use by the
> > > guest, for isolation.
> > >
> > > Hence a new flag for dedicated resets is added to the internal methods,
> > > with a new public reset_control_get_dedicated() method, to obtain an
> > > exclusive handle to a reset that is dedicated to one specific hardware
> > > block.
> >
> > I'm not sure a new flag is necessary, this is what exclusive resets
> > should be.
>
> So perhaps the check should be done for the existing exclusive resets
> instead, without adding a new flag?
That would be my preference, if possible.
> > Also I fear there will be confusion about the difference between
> > exclusive (refering to the reset control) and dedicated (refering to
> > the reset line) reset controls.
>
> Indeed, exclusive has multiple meanings here:
> 1. Exclusive vs. shared access to the reset control,
> 2. Reset line is dedicated to a single device, or shared with multiple
> devices.
2. is the more important factor, and that's what I have in mind when
talking about exclusive vs. shared resets. Admittedly, the kernel doc
comments only mention 1.
> If we can simplify (exclusive == dedicated, 1.shared == 2.shared), life
> can become simpler.
Well, it still has to be possible for drivers to request 1.shared
control over a dedicated reset line, just because the same driver may
work on multiple SoCs, only some of which have that reset line 2.shared.
But if we could make sure that exclusive requests are only possible for
dedicated reset lines, I'd be happier.
> > > - I think __device_reset() should call __reset_control_get() with
> > > dedicated=true. However, that will impact existing users,
> >
> > Which ones? And how?
>
> I didn't actually check which drivers.
> If a reset is not dedicated, device_reset{,_optional}() will suddenly
> start to fail if
> a reset turns out to be not dedicated.
> Well, currently the device will be reset multiple times, so people would
> already have noticed...
Exactly. Naive exclusive control, as currently implemented, is bound to
fail if the reset line is shared. I am not aware of any cases where this
currently happens.
Of course there could always be those fragile cases where something just
works by accident and lucky timing.
[...]
> > I want to hear the device tree maintainers' opinion about this.
> > I'd very much like to have such a check for exclusive resets, but my
> > understanding is that we are not allowed to make the assumption that
> > other nodes' "reset" properties follow the common reset signal device
> > tree bindings.
> >
> > Maybe this is something that should be checked in a device tree
> > validation step?
>
> We already have SoCs where reset lines are shared among multiple on-chip
> devices. So dtc validation won't work, and a runtime check is needed.
Right, the dtb would have to be validated against driver code to get the
information whether a given phandle might be requested exclusively at
some point.
It would be better if this could be checked at runtime.
regards
Philipp
Re: [PATCH v2 6/8] drm/imx: support handling bridge timings bus flags
On Wed, 2018-09-12 at 11:32 -0700, Stefan Agner wrote: > A bridge might require specific settings for the pixel data on > the bus. On which bus? The bridge has input and output. > Copy the bus flags from the bridge timings if a bridge > is in use. > > Signed-off-by: Stefan Agner > --- > drivers/gpu/drm/imx/parallel-display.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/imx/parallel-display.c > b/drivers/gpu/drm/imx/parallel-display.c > index aefd04e18f93..7798a0621df7 100644 > --- a/drivers/gpu/drm/imx/parallel-display.c > +++ b/drivers/gpu/drm/imx/parallel-display.c > @@ -239,6 +239,9 @@ static int imx_pd_bind(struct device *dev, struct device > *master, void *data) > if (ret && ret != -ENODEV) > return ret; > > + if (imxpd->bridge && imxpd->bridge->timings) > + imxpd->bus_flags = imxpd->bridge->timings->input_bus_flags; Oh, ok. I'd also specify input bus in the commit message. > + > imxpd->dev = dev; > > ret = imx_pd_register(drm, imxpd); regards Philipp
Re: [PATCH v2 6/8] drm/imx: support handling bridge timings bus flags
On Wed, 2018-09-12 at 11:32 -0700, Stefan Agner wrote: > A bridge might require specific settings for the pixel data on > the bus. On which bus? The bridge has input and output. > Copy the bus flags from the bridge timings if a bridge > is in use. > > Signed-off-by: Stefan Agner > --- > drivers/gpu/drm/imx/parallel-display.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/imx/parallel-display.c > b/drivers/gpu/drm/imx/parallel-display.c > index aefd04e18f93..7798a0621df7 100644 > --- a/drivers/gpu/drm/imx/parallel-display.c > +++ b/drivers/gpu/drm/imx/parallel-display.c > @@ -239,6 +239,9 @@ static int imx_pd_bind(struct device *dev, struct device > *master, void *data) > if (ret && ret != -ENODEV) > return ret; > > + if (imxpd->bridge && imxpd->bridge->timings) > + imxpd->bus_flags = imxpd->bridge->timings->input_bus_flags; Oh, ok. I'd also specify input bus in the commit message. > + > imxpd->dev = dev; > > ret = imx_pd_register(drm, imxpd); regards Philipp
Re: [RFC] reset: make reset controller driver initialize early
Hi Dinh, On Mon, 2018-09-10 at 14:59 -0500, Dinh Nguyen wrote: > Hi Philipp, > > I need to make the reset controller on the SoCFPGA platform initialize > early. What does early mean in this context? > I have one solution which is similar to what reset-sunxi is > doing, making the "altr,rst-mgr" initialize early in a separate > reset-socfpga.c but using the reset-simple operations. That's what I would have gone for as well. > I'm guessing SoCFPGA may not be the only platform that needs the reset > driver early, would it make sense to have a solution in reset-simple > that is capable of an early init? I suppose you could share sunxi_reset_init if you pass it a struct reset_simple_devdata as an argument. regards Philipp
Re: [RFC] reset: make reset controller driver initialize early
Hi Dinh, On Mon, 2018-09-10 at 14:59 -0500, Dinh Nguyen wrote: > Hi Philipp, > > I need to make the reset controller on the SoCFPGA platform initialize > early. What does early mean in this context? > I have one solution which is similar to what reset-sunxi is > doing, making the "altr,rst-mgr" initialize early in a separate > reset-socfpga.c but using the reset-simple operations. That's what I would have gone for as well. > I'm guessing SoCFPGA may not be the only platform that needs the reset > driver early, would it make sense to have a solution in reset-simple > that is capable of an early init? I suppose you could share sunxi_reset_init if you pass it a struct reset_simple_devdata as an argument. regards Philipp
Re: [RFC PATCH v3 1/3] firmware: xilinx: Add reset API's
Hi,
On Wed, 2018-09-05 at 12:39 +0530, Nava kishore Manne wrote:
> This Patch Adds reset API's to support release, assert
> and status functionalities by using firmware interface.
>
> Signed-off-by: Nava kishore Manne
> ---
> Changes for v3:
> -None.
> Changes for v2:
> -New Patch.
>
> drivers/firmware/xilinx/zynqmp.c | 40 +++
> include/linux/firmware/xlnx-zynqmp.h | 136
> +++
> 2 files changed, 176 insertions(+)
>
> diff --git a/drivers/firmware/xilinx/zynqmp.c
> b/drivers/firmware/xilinx/zynqmp.c
> index 7ccedf0..639c72f 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -447,6 +447,44 @@ static int zynqmp_pm_clock_getparent(u32 clock_id, u32
> *parent_id)
> return ret;
> }
>
> +/**
> + * zynqmp_pm_reset_assert - Request setting of reset (1 - assert, 0 -
> release)
> + * @reset: Reset to be configured
> + * @assert_flag: Flag stating should reset be asserted (1) or
> + * released (0)
> + *
> + * Return: Returns status, either success or error+reason
> + */
> +static int zynqmp_pm_reset_assert(const enum zynqmp_pm_reset reset,
> + const enum zynqmp_pm_reset_action assert_flag)
> +{
> + return zynqmp_pm_invoke_fn(PM_RESET_ASSERT, reset, assert_flag,
> +0, 0, NULL);
> +}
> +
> +/**
> + * zynqmp_pm_reset_get_status - Get status of the reset
> + * @reset: Reset whose status should be returned
> + * @status: Returned status
> + *
> + * Return: Returns status, either success or error+reason
> + */
> +static int zynqmp_pm_reset_get_status(const enum zynqmp_pm_reset reset,
> + u32 *status)
> +{
> + u32 ret_payload[PAYLOAD_ARG_CNT];
> + int ret;
> +
> + if (!status)
> + return -EINVAL;
> +
> + ret = zynqmp_pm_invoke_fn(PM_RESET_GET_STATUS, reset, 0,
> + 0, 0, ret_payload);
> + *status = ret_payload[1];
It doesn't really matter here, but in general I'd skip writing output
arguments in case of error.
For all I know, the result returned in ret_payload could be undefined.
regards
Philipp
Re: [RFC PATCH v3 1/3] firmware: xilinx: Add reset API's
Hi,
On Wed, 2018-09-05 at 12:39 +0530, Nava kishore Manne wrote:
> This Patch Adds reset API's to support release, assert
> and status functionalities by using firmware interface.
>
> Signed-off-by: Nava kishore Manne
> ---
> Changes for v3:
> -None.
> Changes for v2:
> -New Patch.
>
> drivers/firmware/xilinx/zynqmp.c | 40 +++
> include/linux/firmware/xlnx-zynqmp.h | 136
> +++
> 2 files changed, 176 insertions(+)
>
> diff --git a/drivers/firmware/xilinx/zynqmp.c
> b/drivers/firmware/xilinx/zynqmp.c
> index 7ccedf0..639c72f 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -447,6 +447,44 @@ static int zynqmp_pm_clock_getparent(u32 clock_id, u32
> *parent_id)
> return ret;
> }
>
> +/**
> + * zynqmp_pm_reset_assert - Request setting of reset (1 - assert, 0 -
> release)
> + * @reset: Reset to be configured
> + * @assert_flag: Flag stating should reset be asserted (1) or
> + * released (0)
> + *
> + * Return: Returns status, either success or error+reason
> + */
> +static int zynqmp_pm_reset_assert(const enum zynqmp_pm_reset reset,
> + const enum zynqmp_pm_reset_action assert_flag)
> +{
> + return zynqmp_pm_invoke_fn(PM_RESET_ASSERT, reset, assert_flag,
> +0, 0, NULL);
> +}
> +
> +/**
> + * zynqmp_pm_reset_get_status - Get status of the reset
> + * @reset: Reset whose status should be returned
> + * @status: Returned status
> + *
> + * Return: Returns status, either success or error+reason
> + */
> +static int zynqmp_pm_reset_get_status(const enum zynqmp_pm_reset reset,
> + u32 *status)
> +{
> + u32 ret_payload[PAYLOAD_ARG_CNT];
> + int ret;
> +
> + if (!status)
> + return -EINVAL;
> +
> + ret = zynqmp_pm_invoke_fn(PM_RESET_GET_STATUS, reset, 0,
> + 0, 0, ret_payload);
> + *status = ret_payload[1];
It doesn't really matter here, but in general I'd skip writing output
arguments in case of error.
For all I know, the result returned in ret_payload could be undefined.
regards
Philipp
Re: [RFC PATCH v3 3/3] reset: reset-zynqmp: Adding support for Xilinx zynqmp reset controller.
Hi,
thank you for the patch. I have a few comments below:
On Wed, 2018-09-05 at 12:39 +0530, Nava kishore Manne wrote:
> Add a reset controller driver for Xilinx Zynq UltraScale+ MPSoC.
> The zynqmp reset-controller has the ability to reset lines
> connected to different blocks and peripheral in the Soc.
>
> Signed-off-by: Nava kishore Manne
> ---
> Changes for v3:
> -None.
> Changes for v2:
> -Moved eemi_ops into a priv struct as suggested
>by philipp.
>
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-zynqmp.c | 115
> +++
> 2 files changed, 116 insertions(+)
> create mode 100644 drivers/reset/reset-zynqmp.c
>
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index c1261dc..27e4a33 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -21,4 +21,5 @@ obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
> obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
> +obj-$(CONFIG_ARCH_ZYNQMP) += reset-zynqmp.o
>
> diff --git a/drivers/reset/reset-zynqmp.c b/drivers/reset/reset-zynqmp.c
> new file mode 100644
> index 000..f908492
> --- /dev/null
> +++ b/drivers/reset/reset-zynqmp.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Xilinx, Inc.
> + *
> + */
> +
> +#include
I think including io.h is not necessary.
[...]
> +static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> + int val, err;
> +
> + err = priv->eemi_ops->reset_get_status(ZYNQMP_RESET_ID + id, );
> + if (!err)
> + return -EINVAL;
This looks like it should be
if (err)
return err;
instead.
[...]
> +static struct reset_control_ops zynqmp_reset_ops = {
static const struct reset_control_ops zynqmp_reset_ops = {
> + .reset = zynqmp_reset_reset,
> + .assert = zynqmp_reset_assert,
> + .deassert = zynqmp_reset_deassert,
> + .status = zynqmp_reset_status,
> +};
> +
> +static int zynqmp_reset_probe(struct platform_device *pdev)
> +{
> + struct zynqmp_reset_data *priv;
> +
> + priv = devm_kzalloc(>dev,
> + sizeof(*priv), GFP_KERNEL);
This should fit on one line.
regards
Philipp
Re: [RFC PATCH v3 3/3] reset: reset-zynqmp: Adding support for Xilinx zynqmp reset controller.
Hi,
thank you for the patch. I have a few comments below:
On Wed, 2018-09-05 at 12:39 +0530, Nava kishore Manne wrote:
> Add a reset controller driver for Xilinx Zynq UltraScale+ MPSoC.
> The zynqmp reset-controller has the ability to reset lines
> connected to different blocks and peripheral in the Soc.
>
> Signed-off-by: Nava kishore Manne
> ---
> Changes for v3:
> -None.
> Changes for v2:
> -Moved eemi_ops into a priv struct as suggested
>by philipp.
>
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-zynqmp.c | 115
> +++
> 2 files changed, 116 insertions(+)
> create mode 100644 drivers/reset/reset-zynqmp.c
>
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index c1261dc..27e4a33 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -21,4 +21,5 @@ obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
> obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
> +obj-$(CONFIG_ARCH_ZYNQMP) += reset-zynqmp.o
>
> diff --git a/drivers/reset/reset-zynqmp.c b/drivers/reset/reset-zynqmp.c
> new file mode 100644
> index 000..f908492
> --- /dev/null
> +++ b/drivers/reset/reset-zynqmp.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Xilinx, Inc.
> + *
> + */
> +
> +#include
I think including io.h is not necessary.
[...]
> +static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> + int val, err;
> +
> + err = priv->eemi_ops->reset_get_status(ZYNQMP_RESET_ID + id, );
> + if (!err)
> + return -EINVAL;
This looks like it should be
if (err)
return err;
instead.
[...]
> +static struct reset_control_ops zynqmp_reset_ops = {
static const struct reset_control_ops zynqmp_reset_ops = {
> + .reset = zynqmp_reset_reset,
> + .assert = zynqmp_reset_assert,
> + .deassert = zynqmp_reset_deassert,
> + .status = zynqmp_reset_status,
> +};
> +
> +static int zynqmp_reset_probe(struct platform_device *pdev)
> +{
> + struct zynqmp_reset_data *priv;
> +
> + priv = devm_kzalloc(>dev,
> + sizeof(*priv), GFP_KERNEL);
This should fit on one line.
regards
Philipp
