Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1436892 > unrolled thread

[PATCH 1/3] reset: socfpga: no need to store modrst_offset

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/3] reset: socfpga: no need to store modrst_offset Philipp Zabel <p.zabel@pengutronix.de> - 2016-07-05 12:20 +0200
    [PATCH 2/3] reset: sunxi: use readl/writel_relaxed Philipp Zabel <p.zabel@pengutronix.de> - 2016-07-05 12:20 +0200
    [PATCH 3/3] reset: socfpga: use readl/writel_relaxed Philipp Zabel <p.zabel@pengutronix.de> - 2016-07-05 12:20 +0200
      Re: [PATCH 3/3] reset: socfpga: use readl/writel_relaxed Arnd Bergmann <arnd@arndb.de> - 2016-07-05 13:20 +0200
        Re: [PATCH 3/3] reset: socfpga: use readl/writel_relaxed Philipp Zabel <p.zabel@pengutronix.de> - 2016-07-05 13:50 +0200
          Re: [PATCH 3/3] reset: socfpga: use readl/writel_relaxed Arnd Bergmann <arnd@arndb.de> - 2016-07-05 15:00 +0200
            Re: [PATCH 3/3] reset: socfpga: use readl/writel_relaxed Philipp Zabel <p.zabel@pengutronix.de> - 2016-07-05 15:30 +0200
    Re: [PATCH 1/3] reset: socfpga: no need to store modrst_offset Philipp Zabel <p.zabel@pengutronix.de> - 2016-07-06 18:30 +0200

#1436892 — [PATCH 1/3] reset: socfpga: no need to store modrst_offset

Since we can just add it to membase once, there is no need to store
modrst_offset separately, and to repeat the addition with every access.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/reset/reset-socfpga.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
index 12add9b..78ebf84 100644
--- a/drivers/reset/reset-socfpga.c
+++ b/drivers/reset/reset-socfpga.c
@@ -28,7 +28,6 @@
 struct socfpga_reset_data {
 	spinlock_t			lock;
 	void __iomem			*membase;
-	u32				modrst_offset;
 	struct reset_controller_dev	rcdev;
 };
 
@@ -45,9 +44,8 @@ static int socfpga_reset_assert(struct reset_controller_dev *rcdev,
 
 	spin_lock_irqsave(&data->lock, flags);
 
-	reg = readl(data->membase + data->modrst_offset + (bank * NR_BANKS));
-	writel(reg | BIT(offset), data->membase + data->modrst_offset +
-				 (bank * NR_BANKS));
+	reg = readl(data->membase + (bank * NR_BANKS));
+	writel(reg | BIT(offset), data->membase + (bank * NR_BANKS));
 	spin_unlock_irqrestore(&data->lock, flags);
 
 	return 0;
@@ -67,9 +65,8 @@ static int socfpga_reset_deassert(struct reset_controller_dev *rcdev,
 
 	spin_lock_irqsave(&data->lock, flags);
 
-	reg = readl(data->membase + data->modrst_offset + (bank * NR_BANKS));
-	writel(reg & ~BIT(offset), data->membase + data->modrst_offset +
-				  (bank * NR_BANKS));
+	reg = readl(data->membase + (bank * NR_BANKS));
+	writel(reg & ~BIT(offset), data->membase + (bank * NR_BANKS));
 
 	spin_unlock_irqrestore(&data->lock, flags);
 
@@ -85,7 +82,7 @@ static int socfpga_reset_status(struct reset_controller_dev *rcdev,
 	int offset = id % BITS_PER_LONG;
 	u32 reg;
 
-	reg = readl(data->membase + data->modrst_offset + (bank * NR_BANKS));
+	reg = readl(data->membase + (bank * NR_BANKS));
 
 	return !(reg & BIT(offset));
 }
@@ -102,6 +99,7 @@ static int socfpga_reset_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	u32 modrst_offset;
 
 	/*
 	 * The binding was mainlined without the required property.
@@ -122,10 +120,11 @@ static int socfpga_reset_probe(struct platform_device *pdev)
 	if (IS_ERR(data->membase))
 		return PTR_ERR(data->membase);
 
-	if (of_property_read_u32(np, "altr,modrst-offset", &data->modrst_offset)) {
+	if (of_property_read_u32(np, "altr,modrst-offset", &modrst_offset)) {
 		dev_warn(dev, "missing altr,modrst-offset property, assuming 0x10!\n");
-		data->modrst_offset = 0x10;
+		modrst_offset = 0x10;
 	}
+	data->membase += modrst_offset;
 
 	spin_lock_init(&data->lock);
 
-- 
2.8.1

[toc] | [next] | [standalone]

#1436896 — [PATCH 2/3] reset: sunxi: use readl/writel_relaxed

This just removes the rmb()/wmb() pair between register read and
write. Since no relevant reads follow the rmb and no relevant writes
precede the wmb, they should be safe to remove.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/reset/reset-sunxi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/reset/reset-sunxi.c b/drivers/reset/reset-sunxi.c
index 3080190..7a35950 100644
--- a/drivers/reset/reset-sunxi.c
+++ b/drivers/reset/reset-sunxi.c
@@ -41,8 +41,8 @@ static int sunxi_reset_assert(struct reset_controller_dev *rcdev,
 
 	spin_lock_irqsave(&data->lock, flags);
 
-	reg = readl(data->membase + (bank * 4));
-	writel(reg & ~BIT(offset), data->membase + (bank * 4));
+	reg = readl_relaxed(data->membase + (bank * 4));
+	writel_relaxed(reg & ~BIT(offset), data->membase + (bank * 4));
 
 	spin_unlock_irqrestore(&data->lock, flags);
 
@@ -62,8 +62,8 @@ static int sunxi_reset_deassert(struct reset_controller_dev *rcdev,
 
 	spin_lock_irqsave(&data->lock, flags);
 
-	reg = readl(data->membase + (bank * 4));
-	writel(reg | BIT(offset), data->membase + (bank * 4));
+	reg = readl_relaxed(data->membase + (bank * 4));
+	writel_relaxed(reg | BIT(offset), data->membase + (bank * 4));
 
 	spin_unlock_irqrestore(&data->lock, flags);
 
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1436897 — [PATCH 3/3] reset: socfpga: use readl/writel_relaxed

This just removes the rmb()/wmb() pair between register read and
write. Since no relevant reads follow the rmb and no relevant writes
precede the wmb, they should be safe to remove.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/reset/reset-socfpga.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
index 78ebf84..4a65128 100644
--- a/drivers/reset/reset-socfpga.c
+++ b/drivers/reset/reset-socfpga.c
@@ -44,8 +44,8 @@ static int socfpga_reset_assert(struct reset_controller_dev *rcdev,
 
 	spin_lock_irqsave(&data->lock, flags);
 
-	reg = readl(data->membase + (bank * NR_BANKS));
-	writel(reg | BIT(offset), data->membase + (bank * NR_BANKS));
+	reg = readl_relaxed(data->membase + (bank * NR_BANKS));
+	writel_relaxed(reg | BIT(offset), data->membase + (bank * NR_BANKS));
 	spin_unlock_irqrestore(&data->lock, flags);
 
 	return 0;
@@ -65,8 +65,8 @@ static int socfpga_reset_deassert(struct reset_controller_dev *rcdev,
 
 	spin_lock_irqsave(&data->lock, flags);
 
-	reg = readl(data->membase + (bank * NR_BANKS));
-	writel(reg & ~BIT(offset), data->membase + (bank * NR_BANKS));
+	reg = readl_relaxed(data->membase + (bank * NR_BANKS));
+	writel_relaxed(reg & ~BIT(offset), data->membase + (bank * NR_BANKS));
 
 	spin_unlock_irqrestore(&data->lock, flags);
 
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1436935 — Re: [PATCH 3/3] reset: socfpga: use readl/writel_relaxed

On Tuesday, July 5, 2016 12:17:52 PM CEST Philipp Zabel wrote:
> This just removes the rmb()/wmb() pair between register read and
> write. Since no relevant reads follow the rmb and no relevant writes
> precede the wmb, they should be safe to remove.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

We should only do this if you are fixing a bug (which you don't mention
in the changelog), or if you can show a relevant performance
improvement. Is this code ever used in a fast path? If it is,
wouldn't that indicate a problem in some driver?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1436949 — Re: [PATCH 3/3] reset: socfpga: use readl/writel_relaxed

Am Dienstag, den 05.07.2016, 13:20 +0200 schrieb Arnd Bergmann:
> On Tuesday, July 5, 2016 12:17:52 PM CEST Philipp Zabel wrote:
> > This just removes the rmb()/wmb() pair between register read and
> > write. Since no relevant reads follow the rmb and no relevant writes
> > precede the wmb, they should be safe to remove.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> We should only do this if you are fixing a bug (which you don't mention
> in the changelog), or if you can show a relevant performance
> improvement. Is this code ever used in a fast path? If it is,
> wouldn't that indicate a problem in some driver?

It does not fix a bug, and it's not about performance either. I'd like
to align code with the recently posted stm32 driver, to unify them in a
future patch.
Of course we can change the stm32 driver to use readl/writel instead of
the relaxed variants, it just seemed useless to have those barriers
between the read and write.

If anything, we'd need to try to make sure that the writel in assert
hits the hardware before the function returns, so that a
assert-delay-deassert doesn't accidentally spend half its delay with the
writel still in the store buffer, and we'd need a full barrier after the
writel in deassert so that there can be no successive reads from still
disabled IP cores.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1436973 — Re: [PATCH 3/3] reset: socfpga: use readl/writel_relaxed

On Tuesday, July 5, 2016 1:40:16 PM CEST Philipp Zabel wrote:
> Am Dienstag, den 05.07.2016, 13:20 +0200 schrieb Arnd Bergmann:
> > On Tuesday, July 5, 2016 12:17:52 PM CEST Philipp Zabel wrote:
> > > This just removes the rmb()/wmb() pair between register read and
> > > write. Since no relevant reads follow the rmb and no relevant writes
> > > precede the wmb, they should be safe to remove.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > We should only do this if you are fixing a bug (which you don't mention
> > in the changelog), or if you can show a relevant performance
> > improvement. Is this code ever used in a fast path? If it is,
> > wouldn't that indicate a problem in some driver?
> 
> It does not fix a bug, and it's not about performance either. I'd like
> to align code with the recently posted stm32 driver, to unify them in a
> future patch.
> Of course we can change the stm32 driver to use readl/writel instead of
> the relaxed variants, it just seemed useless to have those barriers
> between the read and write.

On stm32, there is no barrier because ARM_DMA_MEM_BUFFERABLE is not set.

I'd really prefer to just have readl/writel everywhere except in the
few places that are performance critical and have a comment explaining
why it's safe there, mainly to avoid having new developers blindly
add the relaxed accessors in drivers because they think it's the
normal coding style.

> If anything, we'd need to try to make sure that the writel in assert
> hits the hardware before the function returns, so that a
> assert-delay-deassert doesn't accidentally spend half its delay with the
> writel still in the store buffer, and we'd need a full barrier after the
> writel in deassert so that there can be no successive reads from still
> disabled IP cores.

In general, I think you need a readl() following the writel() to guarantee
that it has actually hit the hardware.

On ARM you often have just the CPU write buffer that needs to be flushed,
but if you have a PCI device or a more complex SoC, then a barriers doesn't
wait for a write to arrive at the device, it only ensures that a subsequent
write cannot arrive any earlier.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1437008 — Re: [PATCH 3/3] reset: socfpga: use readl/writel_relaxed

Am Dienstag, den 05.07.2016, 14:59 +0200 schrieb Arnd Bergmann:
> On Tuesday, July 5, 2016 1:40:16 PM CEST Philipp Zabel wrote:
> > Am Dienstag, den 05.07.2016, 13:20 +0200 schrieb Arnd Bergmann:
> > > On Tuesday, July 5, 2016 12:17:52 PM CEST Philipp Zabel wrote:
> > > > This just removes the rmb()/wmb() pair between register read and
> > > > write. Since no relevant reads follow the rmb and no relevant writes
> > > > precede the wmb, they should be safe to remove.
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > 
> > > We should only do this if you are fixing a bug (which you don't mention
> > > in the changelog), or if you can show a relevant performance
> > > improvement. Is this code ever used in a fast path? If it is,
> > > wouldn't that indicate a problem in some driver?
> > 
> > It does not fix a bug, and it's not about performance either. I'd like
> > to align code with the recently posted stm32 driver, to unify them in a
> > future patch.
> > Of course we can change the stm32 driver to use readl/writel instead of
> > the relaxed variants, it just seemed useless to have those barriers
> > between the read and write.
> 
> On stm32, there is no barrier because ARM_DMA_MEM_BUFFERABLE is not set.
> 
> I'd really prefer to just have readl/writel everywhere except in the
> few places that are performance critical and have a comment explaining
> why it's safe there, mainly to avoid having new developers blindly
> add the relaxed accessors in drivers because they think it's the
> normal coding style.

I get your point. I'll ask the stm32 developers to use non-relaxed
readl/writel then.

> > If anything, we'd need to try to make sure that the writel in assert
> > hits the hardware before the function returns, so that a
> > assert-delay-deassert doesn't accidentally spend half its delay with the
> > writel still in the store buffer, and we'd need a full barrier after the
> > writel in deassert so that there can be no successive reads from still
> > disabled IP cores.
> 
> In general, I think you need a readl() following the writel() to guarantee
> that it has actually hit the hardware.
>
> On ARM you often have just the CPU write buffer that needs to be flushed,
> but if you have a PCI device or a more complex SoC, then a barriers doesn't
> wait for a write to arrive at the device, it only ensures that a subsequent
> write cannot arrive any earlier.

Yes, exactly. Until now I have not considered this at all.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1437832

Am Mittwoch, den 06.07.2016, 11:06 -0500 schrieb Dinh Nguyen:
> On 07/05/2016 05:17 AM, Philipp Zabel wrote:
> > Since we can just add it to membase once, there is no need to store
> > modrst_offset separately, and to repeat the addition with every access.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/reset/reset-socfpga.c | 19 +++++++++----------
> >  1 file changed, 9 insertions(+), 10 deletions(-)
> > 
> 
> In case you want it:
> 
> Tested-by: Dinh Nguyen <dinguyen@opensource.altera.com>

I'll take it, thank you.

regards
Philipp

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web