Export the aemif_set_cs_timing() and aemif_check_cs_timing() symbols so
they can be used by other drivers
Add a mutex to protect the CS configuration register from concurrent
accesses between the AEMIF and its 'children'.
Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
drivers/memory/ti-aemif.c | 35 ++++++++++++---------------------
include/linux/memory/ti-aemif.h | 32 ++++++++++++++++++++++++++++++
2 files changed, 45 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 dae3441e0cd9..519283028eee 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -13,7 +13,9 @@
#include <linux/err.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/memory/ti-aemif.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
@@ -79,26 +81,6 @@
#define CONFIG_MASK (EW(EW_MAX) | SSTROBE(SSTROBE_MAX) | ASIZE_MAX)
-/**
- * struct aemif_cs_timings: structure to hold CS timings
- * @wstrobe: write strobe width, number of cycles - 1
- * @rstrobe: read strobe width, number of cycles - 1
- * @wsetup: write setup width, number of cycles - 1
- * @whold: write hold width, number of cycles - 1
- * @rsetup: read setup width, number of cycles - 1
- * @rhold: read hold width, number of cycles - 1
- * @ta: minimum turn around time, number of cycles - 1
- */
-struct aemif_cs_timings {
- u32 wstrobe;
- u32 rstrobe;
- u32 wsetup;
- u32 whold;
- u32 rsetup;
- u32 rhold;
- u32 ta;
-};
-
/**
* struct aemif_cs_data: structure to hold CS parameters
* @timings: timings configuration
@@ -123,6 +105,7 @@ struct aemif_cs_data {
* @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;
@@ -131,6 +114,7 @@ struct aemif_device {
u8 num_cs;
int cs_offset;
struct aemif_cs_data cs_data[NUM_CS];
+ struct mutex config_cs_lock;
};
/**
@@ -139,7 +123,7 @@ struct aemif_device {
*
* @return: 0 if the timing configuration is valid, negative error number otherwise.
*/
-static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
+int aemif_check_cs_timings(struct aemif_cs_timings *timings)
{
if (timings->ta > TA_MAX)
return -EINVAL;
@@ -164,6 +148,7 @@ static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
return 0;
}
+EXPORT_SYMBOL(aemif_check_cs_timings);
/**
* aemif_set_cs_timings - Set the timing configuration of a given chip select.
@@ -173,7 +158,7 @@ static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
*
* @return: 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 int offset;
u32 val, set;
@@ -195,13 +180,16 @@ static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_
offset = A1CR_OFFSET + cs * 4;
+ mutex_lock(&aemif->config_cs_lock);
val = readl(aemif->base + offset);
val &= ~TIMINGS_MASK;
val |= set;
writel(val, aemif->base + offset);
+ mutex_unlock(&aemif->config_cs_lock);
return 0;
}
+EXPORT_SYMBOL(aemif_set_cs_timings);
/**
* aemif_calc_rate - calculate timing data.
@@ -257,10 +245,12 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
if (data->enable_ss)
set |= ACR_SSTROBE_MASK;
+ mutex_lock(&aemif->config_cs_lock);
val = readl(aemif->base + offset);
val &= ~CONFIG_MASK;
val |= set;
writel(val, aemif->base + offset);
+ mutex_unlock(&aemif->config_cs_lock);
return aemif_set_cs_timings(aemif, data->cs - aemif->cs_offset, &data->timings);
}
@@ -399,6 +389,7 @@ static int aemif_probe(struct platform_device *pdev)
if (IS_ERR(aemif->base))
return PTR_ERR(aemif->base);
+ mutex_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..0640d30f6321
--- /dev/null
+++ b/include/linux/memory/ti-aemif.h
@@ -0,0 +1,32 @@
+/* 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);
+int aemif_check_cs_timings(struct aemif_cs_timings *timings);
+
+#endif // __TI_AEMIF_H
--
2.47.0On 15/11/2024 14:26, Bastien Curutchet wrote:
> return 0;
> }
> +EXPORT_SYMBOL(aemif_check_cs_timings);
>
> /**
> * aemif_set_cs_timings - Set the timing configuration of a given chip select.
> @@ -173,7 +158,7 @@ static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
> *
> * @return: 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 int offset;
> u32 val, set;
> @@ -195,13 +180,16 @@ static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_
>
> offset = A1CR_OFFSET + cs * 4;
>
> + mutex_lock(&aemif->config_cs_lock);
> val = readl(aemif->base + offset);
> val &= ~TIMINGS_MASK;
> val |= set;
> writel(val, aemif->base + offset);
> + mutex_unlock(&aemif->config_cs_lock);
>
> return 0;
> }
> +EXPORT_SYMBOL(aemif_set_cs_timings);
EXPORT_SYMBOL_GPL everywhere, these are quite specific to driver's
internals, so internal implementation.
Also, all of exported functions need to have correct kerneldoc but I
think they don't. At least missing(), so maybe rest also did not conform
to kernel doc style.
>
> /**
> * aemif_calc_rate - calculate timing data.
> @@ -257,10 +245,12 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
> if (data->enable_ss)
> set |= ACR_SSTROBE_MASK;
>
> + mutex_lock(&aemif->config_cs_lock);
> val = readl(aemif->base + offset);
> val &= ~CONFIG_MASK;
> val |= set;
> writel(val, aemif->base + offset);
> + mutex_unlock(&aemif->config_cs_lock);
>
> return aemif_set_cs_timings(aemif, data->cs - aemif->cs_offset, &data->timings);
> }
> @@ -399,6 +389,7 @@ static int aemif_probe(struct platform_device *pdev)
> if (IS_ERR(aemif->base))
> return PTR_ERR(aemif->base);
>
> + mutex_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..0640d30f6321
> --- /dev/null
> +++ b/include/linux/memory/ti-aemif.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __TI_AEMIF_H
> +#define __TI_AEMIF_H
Use some longer header guard, e.g. __MEMORY_TI_AEMIF_H
> +
> +/**
Best regards,
Krzysztof
Hi Krzysztof,
On 11/21/24 11:33 AM, Krzysztof Kozlowski wrote:
> On 15/11/2024 14:26, Bastien Curutchet wrote:
>> return 0;
>> }
>> +EXPORT_SYMBOL(aemif_check_cs_timings);
>>
>> /**
>> * aemif_set_cs_timings - Set the timing configuration of a given chip select.
>> @@ -173,7 +158,7 @@ static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
>> *
>> * @return: 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 int offset;
>> u32 val, set;
>> @@ -195,13 +180,16 @@ static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_
>>
>> offset = A1CR_OFFSET + cs * 4;
>>
>> + mutex_lock(&aemif->config_cs_lock);
>> val = readl(aemif->base + offset);
>> val &= ~TIMINGS_MASK;
>> val |= set;
>> writel(val, aemif->base + offset);
>> + mutex_unlock(&aemif->config_cs_lock);
>>
>> return 0;
>> }
>> +EXPORT_SYMBOL(aemif_set_cs_timings);
>
>
> EXPORT_SYMBOL_GPL everywhere, these are quite specific to driver's
> internals, so internal implementation.
Ok, I'll use EXPORT_SYMBOL_GPL in next iteration.
>
> Also, all of exported functions need to have correct kerneldoc but I
> think they don't. At least missing(), so maybe rest also did not conform
> to kernel doc style.
>
Do you know a tool that could help me checking the kernel doc style ? I
have no warning when running ```make W=1n drivers/memory/ti-aemif.o```.
The warnings I get when running ```make W=2 drivers/memory/ti-aemif.o```
aren't linked to these comments. And the output I get with
```./scripts/kernel-doc -export drivers/memory/ti-aemif.c``` seems fine
to me.
>>
>> /**
>> * aemif_calc_rate - calculate timing data.
>> @@ -257,10 +245,12 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
>> if (data->enable_ss)
>> set |= ACR_SSTROBE_MASK;
>>
>> + mutex_lock(&aemif->config_cs_lock);
>> val = readl(aemif->base + offset);
>> val &= ~CONFIG_MASK;
>> val |= set;
>> writel(val, aemif->base + offset);
>> + mutex_unlock(&aemif->config_cs_lock);
>>
>> return aemif_set_cs_timings(aemif, data->cs - aemif->cs_offset, &data->timings);
>> }
>> @@ -399,6 +389,7 @@ static int aemif_probe(struct platform_device *pdev)
>> if (IS_ERR(aemif->base))
>> return PTR_ERR(aemif->base);
>>
>> + mutex_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..0640d30f6321
>> --- /dev/null
>> +++ b/include/linux/memory/ti-aemif.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __TI_AEMIF_H
>> +#define __TI_AEMIF_H
>
>
> Use some longer header guard, e.g. __MEMORY_TI_AEMIF_H
>
Ok, I'll do it in next iteration.
Best regards,
Bastien
© 2016 - 2024 Red Hat, Inc.