Export the aemif_set_cs_timing() symbol so it can be used by other
drivers
Add a spinlock to protect the CS configuration register from concurrent
accesses.
Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
drivers/memory/ti-aemif.c | 35 ++++++++++++---------------------
include/linux/memory/ti-aemif.h | 31 +++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 22 deletions(-)
create mode 100644 include/linux/memory/ti-aemif.h
diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
index 8d27b2513b2c..4587095aa703 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -13,10 +13,12 @@
#include <linux/err.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/memory/ti-aemif.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/spinlock.h>
#define TA_SHIFT 2
#define RHOLD_SHIFT 4
@@ -107,27 +109,6 @@ struct aemif_cs_data {
u8 asize;
};
-/**
- * struct aemif_cs_timings: structure to hold cs timing configuration
- * values are expressed in number of clock cycles - 1
- * @ta: minimum turn around time
- * @rhold: read hold width
- * @rstrobe: read strobe width
- * @rsetup: read setup width
- * @whold: write hold width
- * @wstrobe: write strobe width
- * @wsetup: write setup width
- */
-struct aemif_cs_timings {
- u32 ta;
- u32 rhold;
- u32 rstrobe;
- u32 rsetup;
- u32 whold;
- u32 wstrobe;
- u32 wsetup;
-};
-
/**
* struct aemif_device: structure to hold device data
* @base: base address of AEMIF registers
@@ -136,6 +117,7 @@ struct aemif_cs_timings {
* @num_cs: number of assigned chip-selects
* @cs_offset: start number of cs nodes
* @cs_data: array of chip-select settings
+ * @config_cs_lock: lock used to access CS configuration
*/
struct aemif_device {
void __iomem *base;
@@ -144,6 +126,7 @@ struct aemif_device {
u8 num_cs;
int cs_offset;
struct aemif_cs_data cs_data[NUM_CS];
+ spinlock_t config_cs_lock;
};
/**
@@ -154,8 +137,9 @@ struct aemif_device {
*
* Returns 0 on success, else negative errno.
*/
-static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_cs_timings *timings)
+int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_cs_timings *timings)
{
+ unsigned long flags;
unsigned int offset;
u32 val, set;
@@ -176,13 +160,16 @@ static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_
offset = A1CR_OFFSET + cs * 4;
+ spin_lock_irqsave(&aemif->config_cs_lock, flags);
val = readl(aemif->base + offset);
val &= ~TIMINGS_MASK;
val |= set;
writel(val, aemif->base + offset);
+ spin_unlock_irqrestore(&aemif->config_cs_lock, flags);
return 0;
}
+EXPORT_SYMBOL(aemif_set_cs_timings);
/**
* aemif_calc_rate - calculate timing data.
@@ -231,6 +218,7 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
struct aemif_cs_data *data = &aemif->cs_data[csnum];
unsigned long clk_rate = aemif->clk_rate;
struct aemif_cs_timings timings;
+ unsigned long flags;
unsigned offset;
u32 set, val;
@@ -250,10 +238,12 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
if (data->enable_ss)
set |= ACR_SSTROBE_MASK;
+ spin_lock_irqsave(&aemif->config_cs_lock, flags);
val = readl(aemif->base + offset);
val &= ~CONFIG_MASK;
val |= set;
writel(val, aemif->base + offset);
+ spin_unlock_irqrestore(&aemif->config_cs_lock, flags);
return aemif_set_cs_timings(aemif, data->cs - aemif->cs_offset, &timings);
}
@@ -396,6 +386,7 @@ static int aemif_probe(struct platform_device *pdev)
if (IS_ERR(aemif->base))
return PTR_ERR(aemif->base);
+ spin_lock_init(&aemif->config_cs_lock);
if (np) {
/*
* For every controller device node, there is a cs device node
diff --git a/include/linux/memory/ti-aemif.h b/include/linux/memory/ti-aemif.h
new file mode 100644
index 000000000000..809f0a68605a
--- /dev/null
+++ b/include/linux/memory/ti-aemif.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __TI_AEMIF_H
+#define __TI_AEMIF_H
+
+/**
+ * struct aemif_cs_timings: structure to hold cs timing configuration
+ * values are expressed in number of clock cycles - 1
+ * @ta: minimum turn around time
+ * @rhold: read hold width
+ * @rstrobe: read strobe width
+ * @rsetup: read setup width
+ * @whold: write hold width
+ * @wstrobe: write strobe width
+ * @wsetup: write setup width
+ */
+struct aemif_cs_timings {
+ u32 ta;
+ u32 rhold;
+ u32 rstrobe;
+ u32 rsetup;
+ u32 whold;
+ u32 wstrobe;
+ u32 wsetup;
+};
+
+struct aemif_device;
+
+int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_cs_timings *timings);
+
+#endif // __TI_AEMIF_H
--
2.47.0Hello Bastien, On 06/11/2024 at 09:55:03 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote: > Export the aemif_set_cs_timing() symbol so it can be used by other > drivers > > Add a spinlock to protect the CS configuration register from concurrent > accesses. What concurrent accesses are you trying to protect yourself against? I fail to see the use case, but TBH I haven't tried hard enough maybe. Also, what justifies the use of a spin-lock in this case? Thanks, Miquèl
Hi Miquèl, On 11/11/24 8:21 PM, Miquel Raynal wrote: > > Hello Bastien, > > On 06/11/2024 at 09:55:03 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote: > >> Export the aemif_set_cs_timing() symbol so it can be used by other >> drivers >> >> Add a spinlock to protect the CS configuration register from concurrent >> accesses. > > What concurrent accesses are you trying to protect yourself against? > I fail to see the use case, but TBH I haven't tried hard enough maybe. > The register that handles the CS configuration belongs to the AEMIF component but it will also be accessed by the AEMIF 'children' with the aemif_set_cs_timing() function. So far, concurrent accesses shouldn't occur because the AEMIF configures the CS timings only once and does so before probing its 'children'; but I don't know how things can evolve in the future. > Also, what justifies the use of a spin-lock in this case? > TBH, I always struggle to choose between mutexes and spin-locks. I chose spin-lock because it only protects one memory-mapped register so I think it doesn't worth going to sleep when waiting for the lock. Best regards, Bastien
Hi Bastien, On 12/11/2024 at 10:13:38 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote: > Hi Miquèl, > > On 11/11/24 8:21 PM, Miquel Raynal wrote: >> Hello Bastien, >> On 06/11/2024 at 09:55:03 +01, Bastien Curutchet >> <bastien.curutchet@bootlin.com> wrote: >> >>> Export the aemif_set_cs_timing() symbol so it can be used by other >>> drivers >>> >>> Add a spinlock to protect the CS configuration register from concurrent >>> accesses. >> What concurrent accesses are you trying to protect yourself against? >> I fail to see the use case, but TBH I haven't tried hard enough maybe. >> > > The register that handles the CS configuration belongs to the AEMIF > component but it will also be accessed by the AEMIF 'children' with the > aemif_set_cs_timing() function. So far, concurrent accesses shouldn't > occur because the AEMIF configures the CS timings only once and does so > before probing its 'children'; but I don't know how things can evolve in > the future. Okay, and I guess we can have more than one children, in this case indeed we need serialization. >> Also, what justifies the use of a spin-lock in this case? >> > > TBH, I always struggle to choose between mutexes and spin-locks. I chose > spin-lock because it only protects one memory-mapped register so I think > it doesn't worth going to sleep when waiting for the lock. In general, I believe in a path that is not a hot-path and if you can sleep, you should go for a mutex. Cheers, Miquèl
© 2016 - 2024 Red Hat, Inc.