This patch series enables support for PTP in AM65x SR1.0 devices.
This feature relies heavily on the Industrial Ethernet Peripheral
(IEP) hardware module, which implements a hardware counter through
which time is kept. This hardware block is the basis for exposing
a PTP hardware clock to userspace and for issuing timestamps for
incoming/outgoing packets, allowing for time synchronization.
The IEP also has compare registers that fire an interrupt when the
counter reaches the value stored in a compare register. This feature
allows us to support PPS events in the kernel.
The changes are separated into three patches:
- PATCH 01/03: Register SR1.0 devices with the IEP infrastructure to
expose a PHC clock to userspace, allowing time to be
adjusted using standard PTP tools. The code for issuing/
collecting packet timestamps is already present in the
current state of the driver, so only this needs to be
done.
- PATCH 02/03: Add support for IEP compare event/interrupt handling
to enable PPS events.
- PATCH 03/03: Add the interrupts to the IOT2050 device tree.
Currently every compare event generates two interrupts, the first
corresponding to the actual event and the second being a spurious
but otherwise harmless interrupt. The root cause of this has been
identified and has been solved in the platform's SDK. A forward port
of the SDK's patches also fixes the problem in upstream but is not
included here since it's upstreaming is out of the scope of this
series. If someone from TI would be willing to chime in and help
get the interrupt changes upstream that would be great!
Signed-off-by: Diogo Ivo <[email protected]>
---
Changes in v2:
- Collect Reviewed-by tags
- PATCH 01/03: Limit line length to 80 characters
- PATCH 02/03: Proceed with limited functionality if getting IRQ fails,
limit line length to 80 characters
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Diogo Ivo (3):
net: ti: icssg-prueth: Enable PTP timestamping support for SR1.0 devices
net: ti: icss-iep: Enable compare events
arm64: dts: ti: iot2050: Add IEP interrupts for SR1.0 devices
.../boot/dts/ti/k3-am65-iot2050-common-pg1.dtsi | 12 ++++
drivers/net/ethernet/ti/icssg/icss_iep.c | 74 ++++++++++++++++++++++
drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c | 51 ++++++++++++++-
3 files changed, 136 insertions(+), 1 deletion(-)
---
base-commit: 2f0e3f6a6824dfda2759225326d9c69203c06bc8
change-id: 20240529-iep-8bb4a3cb9068
Best regards,
--
Diogo Ivo <[email protected]>
The IEP module supports compare events, in which a value is written to a
hardware register and when the IEP counter reaches the written value an
interrupt is generated. Add handling for this interrupt in order to
support PPS events.
Reviewed-by: Jacob Keller <[email protected]>
Signed-off-by: Diogo Ivo <[email protected]>
---
drivers/net/ethernet/ti/icssg/icss_iep.c | 74 ++++++++++++++++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index 3025e9c18970..b076be9c527c 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -17,6 +17,7 @@
#include <linux/timekeeping.h>
#include <linux/interrupt.h>
#include <linux/of_irq.h>
+#include <linux/workqueue.h>
#include "icss_iep.h"
@@ -122,6 +123,7 @@ struct icss_iep {
int cap_cmp_irq;
u64 period;
u32 latch_enable;
+ struct work_struct work;
};
/**
@@ -571,6 +573,57 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
return ret;
}
+static void icss_iep_cap_cmp_work(struct work_struct *work)
+{
+ struct icss_iep *iep = container_of(work, struct icss_iep, work);
+ const u32 *reg_offs = iep->plat_data->reg_offs;
+ struct ptp_clock_event pevent;
+ unsigned int val;
+ u64 ns, ns_next;
+
+ spin_lock(&iep->irq_lock);
+
+ ns = readl(iep->base + reg_offs[ICSS_IEP_CMP1_REG0]);
+ if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT) {
+ val = readl(iep->base + reg_offs[ICSS_IEP_CMP1_REG1]);
+ ns |= (u64)val << 32;
+ }
+ /* set next event */
+ ns_next = ns + iep->period;
+ writel(lower_32_bits(ns_next),
+ iep->base + reg_offs[ICSS_IEP_CMP1_REG0]);
+ if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT)
+ writel(upper_32_bits(ns_next),
+ iep->base + reg_offs[ICSS_IEP_CMP1_REG1]);
+
+ pevent.pps_times.ts_real = ns_to_timespec64(ns);
+ pevent.type = PTP_CLOCK_PPSUSR;
+ pevent.index = 0;
+ ptp_clock_event(iep->ptp_clock, &pevent);
+ dev_dbg(iep->dev, "IEP:pps ts: %llu next:%llu:\n", ns, ns_next);
+
+ spin_unlock(&iep->irq_lock);
+}
+
+static irqreturn_t icss_iep_cap_cmp_irq(int irq, void *dev_id)
+{
+ struct icss_iep *iep = (struct icss_iep *)dev_id;
+ const u32 *reg_offs = iep->plat_data->reg_offs;
+ unsigned int val;
+
+ val = readl(iep->base + reg_offs[ICSS_IEP_CMP_STAT_REG]);
+ /* The driver only enables CMP1 */
+ if (val & BIT(1)) {
+ /* Clear the event */
+ writel(BIT(1), iep->base + reg_offs[ICSS_IEP_CMP_STAT_REG]);
+ if (iep->pps_enabled || iep->perout_enabled)
+ schedule_work(&iep->work);
+ return IRQ_HANDLED;
+ }
+
+ return IRQ_NONE;
+}
+
static int icss_iep_pps_enable(struct icss_iep *iep, int on)
{
struct ptp_clock_request rq;
@@ -602,6 +655,8 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
} else {
ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
+ if (iep->cap_cmp_irq)
+ cancel_work_sync(&iep->work);
}
if (!ret)
@@ -777,6 +832,8 @@ int icss_iep_init(struct icss_iep *iep, const struct icss_iep_clockops *clkops,
if (iep->ops && iep->ops->perout_enable) {
iep->ptp_info.n_per_out = 1;
iep->ptp_info.pps = 1;
+ } else if (iep->cap_cmp_irq) {
+ iep->ptp_info.pps = 1;
}
if (iep->ops && iep->ops->extts_enable)
@@ -817,6 +874,7 @@ static int icss_iep_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct icss_iep *iep;
struct clk *iep_clk;
+ int ret, irq;
iep = devm_kzalloc(dev, sizeof(*iep), GFP_KERNEL);
if (!iep)
@@ -827,6 +885,22 @@ static int icss_iep_probe(struct platform_device *pdev)
if (IS_ERR(iep->base))
return -ENODEV;
+ irq = platform_get_irq_byname_optional(pdev, "iep_cap_cmp");
+ if (irq == -EPROBE_DEFER)
+ return irq;
+
+ if (irq > 0) {
+ ret = devm_request_irq(dev, irq, icss_iep_cap_cmp_irq,
+ IRQF_TRIGGER_HIGH, "iep_cap_cmp", iep);
+ if (ret) {
+ dev_info(iep->dev, "cap_cmp irq request failed: %x\n",
+ ret);
+ } else {
+ iep->cap_cmp_irq = irq;
+ INIT_WORK(&iep->work, icss_iep_cap_cmp_work);
+ }
+ }
+
iep_clk = devm_clk_get(dev, NULL);
if (IS_ERR(iep_clk))
return PTR_ERR(iep_clk);
--
2.45.2
Add the interrupts needed for PTP Hardware Clock support via IEP
in SR1.0 devices.
Reviewed-by: Jacob Keller <[email protected]>
Signed-off-by: Diogo Ivo <[email protected]>
---
arch/arm64/boot/dts/ti/k3-am65-iot2050-common-pg1.dtsi | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common-pg1.dtsi b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common-pg1.dtsi
index ef7897763ef8..0a29ed172215 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common-pg1.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common-pg1.dtsi
@@ -73,3 +73,15 @@ &icssg0_eth {
"rx0", "rx1",
"rxmgm0", "rxmgm1";
};
+
+&icssg0_iep0 {
+ interrupt-parent = <&icssg0_intc>;
+ interrupts = <7 7 7>;
+ interrupt-names = "iep_cap_cmp";
+};
+
+&icssg0_iep1 {
+ interrupt-parent = <&icssg0_intc>;
+ interrupts = <56 8 8>;
+ interrupt-names = "iep_cap_cmp";
+};
--
2.45.2
Enable PTP support for AM65x SR1.0 devices by registering with the IEP
infrastructure in order to expose a PTP clock to userspace.
Reviewed-by: Jacob Keller <[email protected]>
Signed-off-by: Diogo Ivo <[email protected]>
---
drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c | 51 +++++++++++++++++++++++-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
index 7b3304bbd7fc..fa98bdb11ece 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
@@ -1011,16 +1011,44 @@ static int prueth_probe(struct platform_device *pdev)
dev_dbg(dev, "sram: pa %llx va %p size %zx\n", prueth->msmcram.pa,
prueth->msmcram.va, prueth->msmcram.size);
+ prueth->iep0 = icss_iep_get_idx(np, 0);
+ if (IS_ERR(prueth->iep0)) {
+ ret = dev_err_probe(dev, PTR_ERR(prueth->iep0),
+ "iep0 get failed\n");
+ goto free_pool;
+ }
+
+ prueth->iep1 = icss_iep_get_idx(np, 1);
+ if (IS_ERR(prueth->iep1)) {
+ ret = dev_err_probe(dev, PTR_ERR(prueth->iep1),
+ "iep1 get failed\n");
+ goto put_iep0;
+ }
+
+ ret = icss_iep_init(prueth->iep0, NULL, NULL, 0);
+ if (ret) {
+ dev_err_probe(dev, ret, "failed to init iep0\n");
+ goto put_iep;
+ }
+
+ ret = icss_iep_init(prueth->iep1, NULL, NULL, 0);
+ if (ret) {
+ dev_err_probe(dev, ret, "failed to init iep1\n");
+ goto exit_iep0;
+ }
+
if (eth0_node) {
ret = prueth_netdev_init(prueth, eth0_node);
if (ret) {
dev_err_probe(dev, ret, "netdev init %s failed\n",
eth0_node->name);
- goto free_pool;
+ goto exit_iep;
}
if (of_find_property(eth0_node, "ti,half-duplex-capable", NULL))
prueth->emac[PRUETH_MAC0]->half_duplex = 1;
+
+ prueth->emac[PRUETH_MAC0]->iep = prueth->iep0;
}
if (eth1_node) {
@@ -1033,6 +1061,8 @@ static int prueth_probe(struct platform_device *pdev)
if (of_find_property(eth1_node, "ti,half-duplex-capable", NULL))
prueth->emac[PRUETH_MAC1]->half_duplex = 1;
+
+ prueth->emac[PRUETH_MAC1]->iep = prueth->iep1;
}
/* register the network devices */
@@ -1091,6 +1121,19 @@ static int prueth_probe(struct platform_device *pdev)
prueth_netdev_exit(prueth, eth_node);
}
+exit_iep:
+ icss_iep_exit(prueth->iep1);
+exit_iep0:
+ icss_iep_exit(prueth->iep0);
+
+put_iep:
+ icss_iep_put(prueth->iep1);
+
+put_iep0:
+ icss_iep_put(prueth->iep0);
+ prueth->iep0 = NULL;
+ prueth->iep1 = NULL;
+
free_pool:
gen_pool_free(prueth->sram_pool,
(unsigned long)prueth->msmcram.va, msmc_ram_size);
@@ -1138,6 +1181,12 @@ static void prueth_remove(struct platform_device *pdev)
prueth_netdev_exit(prueth, eth_node);
}
+ icss_iep_exit(prueth->iep1);
+ icss_iep_exit(prueth->iep0);
+
+ icss_iep_put(prueth->iep1);
+ icss_iep_put(prueth->iep0);
+
gen_pool_free(prueth->sram_pool,
(unsigned long)prueth->msmcram.va,
MSMC_RAM_SIZE_SR1);
--
2.45.2
On 04.06.2024 15:15, Diogo Ivo wrote:
> Enable PTP support for AM65x SR1.0 devices by registering with the IEP
> infrastructure in order to expose a PTP clock to userspace.
>
> Reviewed-by: Jacob Keller <[email protected]>
> Signed-off-by: Diogo Ivo <[email protected]>
> ---
Reviewed-by: Wojciech Drewek <[email protected]>
> drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c | 51 +++++++++++++++++++++++-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
> index 7b3304bbd7fc..fa98bdb11ece 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
> @@ -1011,16 +1011,44 @@ static int prueth_probe(struct platform_device *pdev)
> dev_dbg(dev, "sram: pa %llx va %p size %zx\n", prueth->msmcram.pa,
> prueth->msmcram.va, prueth->msmcram.size);
>
> + prueth->iep0 = icss_iep_get_idx(np, 0);
> + if (IS_ERR(prueth->iep0)) {
> + ret = dev_err_probe(dev, PTR_ERR(prueth->iep0),
> + "iep0 get failed\n");
> + goto free_pool;
> + }
> +
> + prueth->iep1 = icss_iep_get_idx(np, 1);
> + if (IS_ERR(prueth->iep1)) {
> + ret = dev_err_probe(dev, PTR_ERR(prueth->iep1),
> + "iep1 get failed\n");
> + goto put_iep0;
> + }
> +
> + ret = icss_iep_init(prueth->iep0, NULL, NULL, 0);
> + if (ret) {
> + dev_err_probe(dev, ret, "failed to init iep0\n");
> + goto put_iep;
> + }
> +
> + ret = icss_iep_init(prueth->iep1, NULL, NULL, 0);
> + if (ret) {
> + dev_err_probe(dev, ret, "failed to init iep1\n");
> + goto exit_iep0;
> + }
> +
> if (eth0_node) {
> ret = prueth_netdev_init(prueth, eth0_node);
> if (ret) {
> dev_err_probe(dev, ret, "netdev init %s failed\n",
> eth0_node->name);
> - goto free_pool;
> + goto exit_iep;
> }
>
> if (of_find_property(eth0_node, "ti,half-duplex-capable", NULL))
> prueth->emac[PRUETH_MAC0]->half_duplex = 1;
> +
> + prueth->emac[PRUETH_MAC0]->iep = prueth->iep0;
> }
>
> if (eth1_node) {
> @@ -1033,6 +1061,8 @@ static int prueth_probe(struct platform_device *pdev)
>
> if (of_find_property(eth1_node, "ti,half-duplex-capable", NULL))
> prueth->emac[PRUETH_MAC1]->half_duplex = 1;
> +
> + prueth->emac[PRUETH_MAC1]->iep = prueth->iep1;
> }
>
> /* register the network devices */
> @@ -1091,6 +1121,19 @@ static int prueth_probe(struct platform_device *pdev)
> prueth_netdev_exit(prueth, eth_node);
> }
>
> +exit_iep:
> + icss_iep_exit(prueth->iep1);
> +exit_iep0:
> + icss_iep_exit(prueth->iep0);
> +
> +put_iep:
> + icss_iep_put(prueth->iep1);
> +
> +put_iep0:
> + icss_iep_put(prueth->iep0);
> + prueth->iep0 = NULL;
> + prueth->iep1 = NULL;
> +
> free_pool:
> gen_pool_free(prueth->sram_pool,
> (unsigned long)prueth->msmcram.va, msmc_ram_size);
> @@ -1138,6 +1181,12 @@ static void prueth_remove(struct platform_device *pdev)
> prueth_netdev_exit(prueth, eth_node);
> }
>
> + icss_iep_exit(prueth->iep1);
> + icss_iep_exit(prueth->iep0);
> +
> + icss_iep_put(prueth->iep1);
> + icss_iep_put(prueth->iep0);
> +
> gen_pool_free(prueth->sram_pool,
> (unsigned long)prueth->msmcram.va,
> MSMC_RAM_SIZE_SR1);
>
On 04.06.2024 15:15, Diogo Ivo wrote:
> The IEP module supports compare events, in which a value is written to a
> hardware register and when the IEP counter reaches the written value an
> interrupt is generated. Add handling for this interrupt in order to
> support PPS events.
>
> Reviewed-by: Jacob Keller <[email protected]>
> Signed-off-by: Diogo Ivo <[email protected]>
> ---
Reviewed-by: Wojciech Drewek <[email protected]>
> drivers/net/ethernet/ti/icssg/icss_iep.c | 74 ++++++++++++++++++++++++++++++++
> 1 file changed, 74 insertions(+)
>
> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
> index 3025e9c18970..b076be9c527c 100644
> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
> @@ -17,6 +17,7 @@
> #include <linux/timekeeping.h>
> #include <linux/interrupt.h>
> #include <linux/of_irq.h>
> +#include <linux/workqueue.h>
>
> #include "icss_iep.h"
>
> @@ -122,6 +123,7 @@ struct icss_iep {
> int cap_cmp_irq;
> u64 period;
> u32 latch_enable;
> + struct work_struct work;
> };
>
> /**
> @@ -571,6 +573,57 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
> return ret;
> }
>
> +static void icss_iep_cap_cmp_work(struct work_struct *work)
> +{
> + struct icss_iep *iep = container_of(work, struct icss_iep, work);
> + const u32 *reg_offs = iep->plat_data->reg_offs;
> + struct ptp_clock_event pevent;
> + unsigned int val;
> + u64 ns, ns_next;
> +
> + spin_lock(&iep->irq_lock);
> +
> + ns = readl(iep->base + reg_offs[ICSS_IEP_CMP1_REG0]);
> + if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT) {
> + val = readl(iep->base + reg_offs[ICSS_IEP_CMP1_REG1]);
> + ns |= (u64)val << 32;
> + }
> + /* set next event */
> + ns_next = ns + iep->period;
> + writel(lower_32_bits(ns_next),
> + iep->base + reg_offs[ICSS_IEP_CMP1_REG0]);
> + if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT)
> + writel(upper_32_bits(ns_next),
> + iep->base + reg_offs[ICSS_IEP_CMP1_REG1]);
> +
> + pevent.pps_times.ts_real = ns_to_timespec64(ns);
> + pevent.type = PTP_CLOCK_PPSUSR;
> + pevent.index = 0;
> + ptp_clock_event(iep->ptp_clock, &pevent);
> + dev_dbg(iep->dev, "IEP:pps ts: %llu next:%llu:\n", ns, ns_next);
> +
> + spin_unlock(&iep->irq_lock);
> +}
> +
> +static irqreturn_t icss_iep_cap_cmp_irq(int irq, void *dev_id)
> +{
> + struct icss_iep *iep = (struct icss_iep *)dev_id;
> + const u32 *reg_offs = iep->plat_data->reg_offs;
> + unsigned int val;
> +
> + val = readl(iep->base + reg_offs[ICSS_IEP_CMP_STAT_REG]);
> + /* The driver only enables CMP1 */
> + if (val & BIT(1)) {
> + /* Clear the event */
> + writel(BIT(1), iep->base + reg_offs[ICSS_IEP_CMP_STAT_REG]);
> + if (iep->pps_enabled || iep->perout_enabled)
> + schedule_work(&iep->work);
> + return IRQ_HANDLED;
> + }
> +
> + return IRQ_NONE;
> +}
> +
> static int icss_iep_pps_enable(struct icss_iep *iep, int on)
> {
> struct ptp_clock_request rq;
> @@ -602,6 +655,8 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
> ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
> } else {
> ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
> + if (iep->cap_cmp_irq)
> + cancel_work_sync(&iep->work);
> }
>
> if (!ret)
> @@ -777,6 +832,8 @@ int icss_iep_init(struct icss_iep *iep, const struct icss_iep_clockops *clkops,
> if (iep->ops && iep->ops->perout_enable) {
> iep->ptp_info.n_per_out = 1;
> iep->ptp_info.pps = 1;
> + } else if (iep->cap_cmp_irq) {
> + iep->ptp_info.pps = 1;
> }
>
> if (iep->ops && iep->ops->extts_enable)
> @@ -817,6 +874,7 @@ static int icss_iep_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct icss_iep *iep;
> struct clk *iep_clk;
> + int ret, irq;
>
> iep = devm_kzalloc(dev, sizeof(*iep), GFP_KERNEL);
> if (!iep)
> @@ -827,6 +885,22 @@ static int icss_iep_probe(struct platform_device *pdev)
> if (IS_ERR(iep->base))
> return -ENODEV;
>
> + irq = platform_get_irq_byname_optional(pdev, "iep_cap_cmp");
> + if (irq == -EPROBE_DEFER)
> + return irq;
> +
> + if (irq > 0) {
> + ret = devm_request_irq(dev, irq, icss_iep_cap_cmp_irq,
> + IRQF_TRIGGER_HIGH, "iep_cap_cmp", iep);
> + if (ret) {
> + dev_info(iep->dev, "cap_cmp irq request failed: %x\n",
> + ret);
> + } else {
> + iep->cap_cmp_irq = irq;
> + INIT_WORK(&iep->work, icss_iep_cap_cmp_work);
> + }
> + }
> +
> iep_clk = devm_clk_get(dev, NULL);
> if (IS_ERR(iep_clk))
> return PTR_ERR(iep_clk);
>
On Tue, 2024-06-04 at 14:15 +0100, Diogo Ivo wrote:
> @@ -571,6 +573,57 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
> return ret;
> }
>
> +static void icss_iep_cap_cmp_work(struct work_struct *work)
> +{
> + struct icss_iep *iep = container_of(work, struct icss_iep, work);
> + const u32 *reg_offs = iep->plat_data->reg_offs;
> + struct ptp_clock_event pevent;
> + unsigned int val;
> + u64 ns, ns_next;
> +
> + spin_lock(&iep->irq_lock);
'irq_lock' is always acquired with the irqsave variant, and here we are
in process context. This discrepancy would at least deserve a comment;
likely the above lock type is not correct.
Cheers,
Paolo
Hi Paolo,
On 6/6/24 11:32 AM, Paolo Abeni wrote:
> On Tue, 2024-06-04 at 14:15 +0100, Diogo Ivo wrote:
>> @@ -571,6 +573,57 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
>> return ret;
>> }
>>
>> +static void icss_iep_cap_cmp_work(struct work_struct *work)
>> +{
>> + struct icss_iep *iep = container_of(work, struct icss_iep, work);
>> + const u32 *reg_offs = iep->plat_data->reg_offs;
>> + struct ptp_clock_event pevent;
>> + unsigned int val;
>> + u64 ns, ns_next;
>> +
>> + spin_lock(&iep->irq_lock);
>
> 'irq_lock' is always acquired with the irqsave variant, and here we are
> in process context. This discrepancy would at least deserve a comment;
> likely the above lock type is not correct.
If my reasoning is correct I believe this variant is correct here. The
register accesses in the IRQ handler and icss_iep_cap_cmp_work() are
orthogonal, so there should be no need to guard against the IRQ handler
here. This is the case for the other places where the _irqsave() variant
is used, so using the _irqsave() variant is overkill there.
From my understanding this is a remnant of the SDK's version of the
driver, where all of the processing now done in icss_iep_cap_cmp_work()
was done directly in the IRQ handler, meaning that we had to guard
against some other thread calling icss_iep_ptp_enable() and accessing
for example ICSS_IEP_CMP1_REG0 concurrently. This can be seen in the
comment on line 114:
struct icss_iep {
...
spinlock_t irq_lock; /* CMP IRQ vs icss_iep_ptp_enable access */
...
};
For v3 I can add a comment with a condensed version of this argument in
icss_iep_cap_cmp_work().
With this said it should be possible to change this spinlock to a mutex as
well, since all the possibilities for concurrency happen outside of
interrupt context. I can add a patch to this series doing that if you
agree with my reasoning above and find it beneficial. For this some
comments from TI would also be good to have in case I missed something
or there is some other factor that I am not aware of.
Best regards,
Diogo
On Thu, 2024-06-06 at 14:28 +0100, Diogo Ivo wrote:
> On 6/6/24 11:32 AM, Paolo Abeni wrote:
> > On Tue, 2024-06-04 at 14:15 +0100, Diogo Ivo wrote:
> > > @@ -571,6 +573,57 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
> > > return ret;
> > > }
> > >
> > > +static void icss_iep_cap_cmp_work(struct work_struct *work)
> > > +{
> > > + struct icss_iep *iep = container_of(work, struct icss_iep, work);
> > > + const u32 *reg_offs = iep->plat_data->reg_offs;
> > > + struct ptp_clock_event pevent;
> > > + unsigned int val;
> > > + u64 ns, ns_next;
> > > +
> > > + spin_lock(&iep->irq_lock);
> >
> > 'irq_lock' is always acquired with the irqsave variant, and here we are
> > in process context. This discrepancy would at least deserve a comment;
> > likely the above lock type is not correct.
>
> If my reasoning is correct I believe this variant is correct here. The
> register accesses in the IRQ handler and icss_iep_cap_cmp_work() are
> orthogonal, so there should be no need to guard against the IRQ handler
> here. This is the case for the other places where the _irqsave() variant
> is used, so using the _irqsave() variant is overkill there.
>
> From my understanding this is a remnant of the SDK's version of the
> driver, where all of the processing now done in icss_iep_cap_cmp_work()
> was done directly in the IRQ handler, meaning that we had to guard
> against some other thread calling icss_iep_ptp_enable() and accessing
> for example ICSS_IEP_CMP1_REG0 concurrently. This can be seen in the
> comment on line 114:
>
> struct icss_iep {
> ...
> spinlock_t irq_lock; /* CMP IRQ vs icss_iep_ptp_enable access */
> ...
> };
>
> For v3 I can add a comment with a condensed version of this argument in
> icss_iep_cap_cmp_work().
Please have run with LOCKDEP enabled, I think it should splat with the
mix of plain spinlock and spinlock_irqsave this patch brings in.
> With this said it should be possible to change this spinlock to a mutex as
> well, since all the possibilities for concurrency happen outside of
> interrupt context. I can add a patch to this series doing that if you
> agree with my reasoning above and find it beneficial. For this some
> comments from TI would also be good to have in case I missed something
> or there is some other factor that I am not aware of.
It looks like that most critical section protected by iep->irq_lock are
already under ptp_clk_mutex protection. AFAICS all except the one
introduced by this patch.
If so, you could acquire such mutex even in icss_iep_cap_cmp_work() and
completely remove iep->irq_lock.
Cheers,
Paolo
On 6/6/24 2:49 PM, Paolo Abeni wrote:
> On Thu, 2024-06-06 at 14:28 +0100, Diogo Ivo wrote:
>> With this said it should be possible to change this spinlock to a mutex as
>> well, since all the possibilities for concurrency happen outside of
>> interrupt context. I can add a patch to this series doing that if you
>> agree with my reasoning above and find it beneficial. For this some
>> comments from TI would also be good to have in case I missed something
>> or there is some other factor that I am not aware of.
>
> It looks like that most critical section protected by iep->irq_lock are
> already under ptp_clk_mutex protection. AFAICS all except the one
> introduced by this patch.
>
> If so, you could acquire such mutex even in icss_iep_cap_cmp_work() and
> completely remove iep->irq_lock.
That is a much better approach, I'll do that and post v3.
Best regards,
Diogo