Re: [PATCH 15/17] soc: fsl: cpm1: qmc: Handle timeslot entries at channel start() and stop()

2023-12-01 Thread Herve Codina
Hi Arnd,

On Wed, 29 Nov 2023 15:03:02 +0100
"Arnd Bergmann"  wrote:

> On Tue, Nov 28, 2023, at 15:08, Herve Codina wrote:
> > @@ -272,6 +274,8 @@ int qmc_chan_get_info(struct qmc_chan *chan, struct 
> > qmc_chan_info *info)
> > if (ret)
> > return ret;
> > 
> > +   spin_lock_irqsave(>ts_lock, flags);
> > +
> > info->mode = chan->mode;
> > info->rx_fs_rate = tsa_info.rx_fs_rate;
> > info->rx_bit_rate = tsa_info.rx_bit_rate;
> > @@ -280,6 +284,8 @@ int qmc_chan_get_info(struct qmc_chan *chan, struct 
> > qmc_chan_info *info)
> > info->tx_bit_rate = tsa_info.tx_bit_rate;
> > info->nb_rx_ts = hweight64(chan->rx_ts_mask);
> > 
> > +   spin_unlock_irqrestore(>ts_lock, flags);
> > +
> > return 0;
> >  }  
> 
> I would normally use spin_lock_irq() instead of spin_lock_irqsave()
> in functions that are only called outside of atomic context.

I would prefer to keep spin_lock_irqsave() here.
This function is part of the API and so, its quite difficult to ensure
that all calls (current and future) will be done outside of an atomic
context.

> 
> > +static int qmc_chan_start_rx(struct qmc_chan *chan);
> > +
> >  int qmc_chan_stop(struct qmc_chan *chan, int direction)
> >  {  
> ... 
> > -static void qmc_chan_start_rx(struct qmc_chan *chan)
> > +static int qmc_setup_chan_trnsync(struct qmc *qmc, struct qmc_chan *chan);
> > +
> > +static int qmc_chan_start_rx(struct qmc_chan *chan)
> >  {  
> 
> Can you reorder the static functions in a way that avoids the
> forward declarations?

Yes, sure.
I will do that in the next iteration.

Thanks for the review,

Best regards,
Hervé


Re: [PATCH 15/17] soc: fsl: cpm1: qmc: Handle timeslot entries at channel start() and stop()

2023-11-29 Thread Arnd Bergmann
On Tue, Nov 28, 2023, at 15:08, Herve Codina wrote:
> @@ -272,6 +274,8 @@ int qmc_chan_get_info(struct qmc_chan *chan, struct 
> qmc_chan_info *info)
>   if (ret)
>   return ret;
> 
> + spin_lock_irqsave(>ts_lock, flags);
> +
>   info->mode = chan->mode;
>   info->rx_fs_rate = tsa_info.rx_fs_rate;
>   info->rx_bit_rate = tsa_info.rx_bit_rate;
> @@ -280,6 +284,8 @@ int qmc_chan_get_info(struct qmc_chan *chan, struct 
> qmc_chan_info *info)
>   info->tx_bit_rate = tsa_info.tx_bit_rate;
>   info->nb_rx_ts = hweight64(chan->rx_ts_mask);
> 
> + spin_unlock_irqrestore(>ts_lock, flags);
> +
>   return 0;
>  }

I would normally use spin_lock_irq() instead of spin_lock_irqsave()
in functions that are only called outside of atomic context.

> +static int qmc_chan_start_rx(struct qmc_chan *chan);
> +
>  int qmc_chan_stop(struct qmc_chan *chan, int direction)
>  {
... 
> -static void qmc_chan_start_rx(struct qmc_chan *chan)
> +static int qmc_setup_chan_trnsync(struct qmc *qmc, struct qmc_chan *chan);
> +
> +static int qmc_chan_start_rx(struct qmc_chan *chan)
>  {

Can you reorder the static functions in a way that avoids the
forward declarations?

 Arnd


[PATCH 15/17] soc: fsl: cpm1: qmc: Handle timeslot entries at channel start() and stop()

2023-11-28 Thread Herve Codina
In order to support runtime timeslot route changes, enable the
channel timeslot entries at channel start() and disable them at
channel stop().

Signed-off-by: Herve Codina 
Reviewed-by: Christophe Leroy 
---
 drivers/soc/fsl/qe/qmc.c | 175 ---
 1 file changed, 163 insertions(+), 12 deletions(-)

diff --git a/drivers/soc/fsl/qe/qmc.c b/drivers/soc/fsl/qe/qmc.c
index e651b3bba1ca..e56aea5803bf 100644
--- a/drivers/soc/fsl/qe/qmc.c
+++ b/drivers/soc/fsl/qe/qmc.c
@@ -177,6 +177,7 @@ struct qmc_chan {
struct qmc *qmc;
void __iomem *s_param;
enum qmc_mode mode;
+   spinlock_t  ts_lock; /* Protect timeslots */
u64 tx_ts_mask_avail;
u64 tx_ts_mask;
u64 rx_ts_mask_avail;
@@ -265,6 +266,7 @@ static void qmc_setbits32(void __iomem *addr, u32 set)
 int qmc_chan_get_info(struct qmc_chan *chan, struct qmc_chan_info *info)
 {
struct tsa_serial_info tsa_info;
+   unsigned long flags;
int ret;
 
/* Retrieve info from the TSA related serial */
@@ -272,6 +274,8 @@ int qmc_chan_get_info(struct qmc_chan *chan, struct 
qmc_chan_info *info)
if (ret)
return ret;
 
+   spin_lock_irqsave(>ts_lock, flags);
+
info->mode = chan->mode;
info->rx_fs_rate = tsa_info.rx_fs_rate;
info->rx_bit_rate = tsa_info.rx_bit_rate;
@@ -280,6 +284,8 @@ int qmc_chan_get_info(struct qmc_chan *chan, struct 
qmc_chan_info *info)
info->tx_bit_rate = tsa_info.tx_bit_rate;
info->nb_rx_ts = hweight64(chan->rx_ts_mask);
 
+   spin_unlock_irqrestore(>ts_lock, flags);
+
return 0;
 }
 EXPORT_SYMBOL(qmc_chan_get_info);
@@ -683,6 +689,40 @@ static int qmc_chan_setup_tsa_32tx(struct qmc_chan *chan, 
const struct tsa_seria
return 0;
 }
 
+static int qmc_chan_setup_tsa_tx(struct qmc_chan *chan, bool enable)
+{
+   struct tsa_serial_info info;
+   int ret;
+
+   /* Retrieve info from the TSA related serial */
+   ret = tsa_serial_get_info(chan->qmc->tsa_serial, );
+   if (ret)
+   return ret;
+
+   /* Setup entries */
+   if (chan->qmc->is_tsa_64rxtx)
+   return qmc_chan_setup_tsa_64rxtx(chan, , enable);
+
+   return qmc_chan_setup_tsa_32tx(chan, , enable);
+}
+
+static int qmc_chan_setup_tsa_rx(struct qmc_chan *chan, bool enable)
+{
+   struct tsa_serial_info info;
+   int ret;
+
+   /* Retrieve info from the TSA related serial */
+   ret = tsa_serial_get_info(chan->qmc->tsa_serial, );
+   if (ret)
+   return ret;
+
+   /* Setup entries */
+   if (chan->qmc->is_tsa_64rxtx)
+   return qmc_chan_setup_tsa_64rxtx(chan, , enable);
+
+   return qmc_chan_setup_tsa_32rx(chan, , enable);
+}
+
 static int qmc_chan_setup_tsa(struct qmc_chan *chan, bool enable)
 {
struct tsa_serial_info info;
@@ -719,6 +759,12 @@ static int qmc_chan_stop_rx(struct qmc_chan *chan)
 
spin_lock_irqsave(>rx_lock, flags);
 
+   if (chan->is_rx_stopped) {
+   /* The channel is already stopped -> simply return ok */
+   ret = 0;
+   goto end;
+   }
+
/* Send STOP RECEIVE command */
ret = qmc_chan_command(chan, 0x0);
if (ret) {
@@ -729,6 +775,15 @@ static int qmc_chan_stop_rx(struct qmc_chan *chan)
 
chan->is_rx_stopped = true;
 
+   if (!chan->qmc->is_tsa_64rxtx || chan->is_tx_stopped) {
+   ret = qmc_chan_setup_tsa_rx(chan, false);
+   if (ret) {
+   dev_err(chan->qmc->dev, "chan %u: Disable tsa entries 
failed (%d)\n",
+   chan->id, ret);
+   goto end;
+   }
+   }
+
 end:
spin_unlock_irqrestore(>rx_lock, flags);
return ret;
@@ -741,6 +796,12 @@ static int qmc_chan_stop_tx(struct qmc_chan *chan)
 
spin_lock_irqsave(>tx_lock, flags);
 
+   if (chan->is_tx_stopped) {
+   /* The channel is already stopped -> simply return ok */
+   ret = 0;
+   goto end;
+   }
+
/* Send STOP TRANSMIT command */
ret = qmc_chan_command(chan, 0x1);
if (ret) {
@@ -751,37 +812,82 @@ static int qmc_chan_stop_tx(struct qmc_chan *chan)
 
chan->is_tx_stopped = true;
 
+   if (!chan->qmc->is_tsa_64rxtx || chan->is_rx_stopped) {
+   ret = qmc_chan_setup_tsa_tx(chan, false);
+   if (ret) {
+   dev_err(chan->qmc->dev, "chan %u: Disable tsa entries 
failed (%d)\n",
+   chan->id, ret);
+   goto end;
+   }
+   }
+
 end:
spin_unlock_irqrestore(>tx_lock, flags);
return ret;
 }
 
+static int qmc_chan_start_rx(struct qmc_chan *chan);
+
 int qmc_chan_stop(struct qmc_chan *chan, int direction)
 {
-   int ret;
+   bool is_rx_rollback_needed = false;
+   unsigned long flags;
+