Re: [PATCH v2 1/2] accel/qaic: Add support for periodic timesync

2023-10-27 Thread Jeffrey Hugo

On 10/22/2023 5:06 AM, Stanislaw Gruszka wrote:

On Mon, Oct 16, 2023 at 11:01:13AM -0600, Jeffrey Hugo wrote:

From: Ajit Pal Singh 

Device and Host have a time synchronization mechanism that happens once
during boot when device is in SBL mode. After that, in mission-mode there
is no timesync. In an experiment after continuous operation, device time
drifted w.r.t. host by approximately 3 seconds per day. This drift leads
to mismatch in timestamp of device and Host logs. To correct this
implement periodic timesync in driver. This timesync is carried out via
QAIC_TIMESYNC_PERIODIC MHI channel.

Signed-off-by: Ajit Pal Singh 
Signed-off-by: Pranjal Ramajor Asha Kanojiya 
Reviewed-by: Jeffrey Hugo 
Reviewed-by: Carl Vanderlip 
Reviewed-by: Pranjal Ramajor Asha Kanojiya 
Signed-off-by: Jeffrey Hugo 

Reviewed-by: Stanislaw Gruszka 


@@ -586,8 +587,16 @@ static int __init qaic_init(void)
goto free_pci;
}
  
+	ret = qaic_timesync_init();

+   if (ret) {
+   pr_debug("qaic: qaic_timesync_init failed %d\n", ret);
+   goto free_mhi;

I would print at error level here. Or if timesync is optional do not error exit.


Good point.  timesync is optional so will not do error exit.




+#ifdef readq
+static u64 read_qtimer(const volatile void __iomem *addr)
+{
+   return readq(addr);
+}
+#else
+static u64 read_qtimer(const volatile void __iomem *addr)
+{
+   u64 low, high;
+
+   low = readl(addr);
+   high = readl(addr + sizeof(u32));
+   return low | (high << 32);
+}

If that's only for compile on 32-bit PowerPC, I think would be better
to limit supported architectures on Kconfig.


The issue was flagged on 32-bit PowerPC, but I concluded from a code 
review that the issue exists on any 32-bit architecture.  Given that 
this is an add-on card I'd prefer to support as many architectures as 
possible.


-Jeff



Re: [PATCH v2 1/2] accel/qaic: Add support for periodic timesync

2023-10-22 Thread Stanislaw Gruszka
On Mon, Oct 16, 2023 at 11:01:13AM -0600, Jeffrey Hugo wrote:
> From: Ajit Pal Singh 
> 
> Device and Host have a time synchronization mechanism that happens once
> during boot when device is in SBL mode. After that, in mission-mode there
> is no timesync. In an experiment after continuous operation, device time
> drifted w.r.t. host by approximately 3 seconds per day. This drift leads
> to mismatch in timestamp of device and Host logs. To correct this
> implement periodic timesync in driver. This timesync is carried out via
> QAIC_TIMESYNC_PERIODIC MHI channel.
> 
> Signed-off-by: Ajit Pal Singh 
> Signed-off-by: Pranjal Ramajor Asha Kanojiya 
> Reviewed-by: Jeffrey Hugo 
> Reviewed-by: Carl Vanderlip 
> Reviewed-by: Pranjal Ramajor Asha Kanojiya 
> Signed-off-by: Jeffrey Hugo 
Reviewed-by: Stanislaw Gruszka 

> @@ -586,8 +587,16 @@ static int __init qaic_init(void)
>   goto free_pci;
>   }
>  
> + ret = qaic_timesync_init();
> + if (ret) {
> + pr_debug("qaic: qaic_timesync_init failed %d\n", ret);
> + goto free_mhi;
I would print at error level here. Or if timesync is optional do not error 
exit. 

> +#ifdef readq
> +static u64 read_qtimer(const volatile void __iomem *addr)
> +{
> + return readq(addr);
> +}
> +#else
> +static u64 read_qtimer(const volatile void __iomem *addr)
> +{
> + u64 low, high;
> +
> + low = readl(addr);
> + high = readl(addr + sizeof(u32));
> + return low | (high << 32);
> +}
If that's only for compile on 32-bit PowerPC, I think would be better
to limit supported architectures on Kconfig. 

Regards
Stanislaw