Re: [PATCH v8 12/19] drivers: soc: ti: k3-ringacc: handle absence of tisci

[Sai Sree Kartheek Adivi <s-adivi@ti.com>]

On 15/09/26 12:22, Vignesh Raghavendra wrote:
>> Handle absence of tisci with direct register writes. This will support
>> platforms that do not have tisci firmware like AM62L.
>>
>> Signed-off-by: Sai Sree Kartheek Adivi <s-adivi@ti.com>
>>
>> diff --git a/drivers/soc/ti/k3-ringacc.c b/drivers/soc/ti/k3-ringacc.c
>> index 5966db4327b1..2a93e389fb8e 100644
>> --- a/drivers/soc/ti/k3-ringacc.c
>> +++ b/drivers/soc/ti/k3-ringacc.c
>> @@ -46,6 +46,53 @@ struct k3_ring_rt_regs {
>>  	u32	hwindx;
>>  };
>>  
>> +#define K3_RINGACC_RT_CFG_REGS_OFS	0x40
>> +#define K3_DMARING_CFG_ADDR_HI_MASK	GENMASK(3, 0)
>> +#define K3_DMARING_CFG_ASEL_SHIFT	16
>> +#define K3_DMARING_CFG_SIZE_MASK	GENMASK(15, 0)
>> +
>> +/**
>> + * struct k3_ring_cfg_regs - The RA Configuration Registers region
>> + *
>> + * @ba_lo: Ring Base Address Low Register
>> + * @ba_hi: Ring Base Address High Register
>> + * @size: Ring Size Register
>> + */
>> +struct k3_ring_cfg_regs {
>> +	u32	ba_lo;
>> +	u32	ba_hi;
>> +	u32	size;
>> +};
>> +
>> +#define K3_RINGACC_RT_INT_REGS_OFS		0x140
>> +#define K3_RINGACC_RT_INT_ENABLE_SET_COMPLETE	BIT(0)
>> +#define K3_RINGACC_RT_INT_ENABLE_SET_TR			BIT(2)
>> +
>> +/**
>> + * struct k3_ring_intr_regs {
>> + *
>> + * @enable_set: Ring Interrupt Enable Register
>> + * @resv_1: Reserved
>> + * @clr: Ring Interrupt Clear Register
>> + * @resv_2: Reserved
>> + * @status_set: Ring Interrupt Status Set Register
>> + * @resv_3: Reserved
>> + * @status: Ring Interrupt Status Register
>> + * @resv_4: Reserved
>> + * @status_masked: Ring Interrupt Status Masked Register
>> + */
>> +struct k3_ring_intr_regs {
>> +	u32	enable_set;
>> +	u32	resv_1;
>> +	u32	clr;
>> +	u32	resv_2;
>> +	u32	status_set;
>> +	u32	resv_3;
>> +	u32	status;
>> +	u32	resv_4;
>> +	u32	status_masked;
>> +};
>> +
>>  #define K3_RINGACC_RT_REGS_STEP			0x1000
>>  #define K3_DMARING_RT_REGS_STEP			0x2000
>>  #define K3_DMARING_RT_REGS_REVERSE_OFS		0x1000
>> @@ -139,6 +186,8 @@ struct k3_ring_state {
>>   * struct k3_ring - RA Ring descriptor
>>   *
>>   * @rt: Ring control/status registers
>> + * @cfg: Ring config registers
>> + * @intr: Ring interrupt registers
>>   * @fifos: Ring queues registers
>>   * @proxy: Ring Proxy Datapath registers
>>   * @ring_mem_dma: Ring buffer dma address
>> @@ -158,6 +207,8 @@ struct k3_ring_state {
>>   */
>>  struct k3_ring {
>>  	struct k3_ring_rt_regs __iomem *rt;
>> +	struct k3_ring_cfg_regs __iomem *cfg;
>> +	struct k3_ring_intr_regs __iomem *intr;
>>  	struct k3_ring_fifo_regs __iomem *fifos;
>>  	struct k3_ringacc_proxy_target_regs  __iomem *proxy;
>>  	dma_addr_t	ring_mem_dma;
>> @@ -467,15 +518,31 @@ static void k3_ringacc_ring_reset_sci(struct k3_ring *ring)
>>  	struct k3_ringacc *ringacc = ring->parent;
>>  	int ret;
>>  
>> -	ring_cfg.nav_id = ringacc->tisci_dev_id;
>> -	ring_cfg.index = ring->ring_id;
>> -	ring_cfg.valid_params = TI_SCI_MSG_VALUE_RM_RING_COUNT_VALID;
>> -	ring_cfg.count = ring->size;
>> +	if (!ringacc->tisci) {
>> +		u32 reg;
>>  
>> -	ret = ringacc->tisci_ring_ops->set_cfg(ringacc->tisci, &ring_cfg);
>> -	if (ret)
>> -		dev_err(ringacc->dev, "TISCI reset ring fail (%d) ring_idx %d\n",
>> -			ret, ring->ring_id);
>> +		if (!ring->cfg)
>> +			return;
>> +
>> +		reg = readl(&ring->cfg->size);
>> +		reg &= ~K3_DMARING_CFG_SIZE_MASK;
>> +		writel(reg, &ring->cfg->size);
>> +
>> +		/* Ensure the register clear operation completes before writing new value */
> how is this ensured? Should there be a check to read the reg until
> readback of K3_DMARING_CFG_SIZE_MASK field reads 0?
>
>> +		reg = readl(&ring->cfg->size);
>> +		reg |= ring->size;
>> +		writel(reg, &ring->cfg->size);
>> +	} else {
>> +		ring_cfg.nav_id = ringacc->tisci_dev_id;
>> +		ring_cfg.index = ring->ring_id;
>> +		ring_cfg.valid_params = TI_SCI_MSG_VALUE_RM_RING_COUNT_VALID;
>> +		ring_cfg.count = ring->size;
>> +
>> +		ret = ringacc->tisci_ring_ops->set_cfg(ringacc->tisci, &ring_cfg);
>> +		if (ret)
>> +			dev_err(ringacc->dev, "TISCI reset ring fail (%d) ring_idx %d\n",
>> +				ret, ring->ring_id);
>> +	}
>>  }
>>  
>>  void k3_ringacc_ring_reset(struct k3_ring *ring)
>> @@ -501,10 +568,25 @@ static void k3_ringacc_ring_reconfig_qmode_sci(struct k3_ring *ring,
>>  	ring_cfg.valid_params = TI_SCI_MSG_VALUE_RM_RING_MODE_VALID;
>>  	ring_cfg.mode = mode;
>>  
>> -	ret = ringacc->tisci_ring_ops->set_cfg(ringacc->tisci, &ring_cfg);
>> -	if (ret)
>> -		dev_err(ringacc->dev, "TISCI reconf qmode fail (%d) ring_idx %d\n",
>> -			ret, ring->ring_id);
>> +	if (!ringacc->tisci) {
>> +		u32 reg;
>> +
>> +		writel(ring_cfg.addr_lo, &ring->cfg->ba_lo);
>> +		writel((ring_cfg.addr_hi & K3_DMARING_CFG_ADDR_HI_MASK) +
>> +				(ring_cfg.asel << K3_DMARING_CFG_ASEL_SHIFT),
>> +				&ring->cfg->ba_hi);
>> +
>> +		reg = readl(&ring->cfg->size);
>> +		reg &= ~K3_DMARING_CFG_SIZE_MASK;
>> +		reg |= ring_cfg.count & K3_DMARING_CFG_SIZE_MASK;
>> +
>> +		writel(reg, &ring->cfg->size);
>> +	} else {
>> +		ret = ringacc->tisci_ring_ops->set_cfg(ringacc->tisci, &ring_cfg);
>> +		if (ret)
>> +			dev_err(ringacc->dev, "TISCI reconf qmode fail (%d) ring_idx %d\n",
>> +				ret, ring->ring_id);
>> +	}
>>  }
>>  
>>  void k3_ringacc_ring_reset_dma(struct k3_ring *ring, u32 occ)
>> @@ -576,10 +658,25 @@ static void k3_ringacc_ring_free_sci(struct k3_ring *ring)
>>  	ring_cfg.index = ring->ring_id;
>>  	ring_cfg.valid_params = TI_SCI_MSG_VALUE_RM_ALL_NO_ORDER;
>>  
>> -	ret = ringacc->tisci_ring_ops->set_cfg(ringacc->tisci, &ring_cfg);
>> -	if (ret)
>> -		dev_err(ringacc->dev, "TISCI ring free fail (%d) ring_idx %d\n",
>> -			ret, ring->ring_id);
>> +	if (!ringacc->tisci) {
>> +		u32 reg;
>> +
>> +		writel(ring_cfg.addr_lo, &ring->cfg->ba_lo);
>> +		writel((ring_cfg.addr_hi & K3_DMARING_CFG_ADDR_HI_MASK) +
>> +				(ring_cfg.asel << K3_DMARING_CFG_ASEL_SHIFT),
>> +				&ring->cfg->ba_hi);
>> +
>> +		reg = readl(&ring->cfg->size);
>> +		reg &= ~K3_DMARING_CFG_SIZE_MASK;
>> +		reg |= ring_cfg.count & K3_DMARING_CFG_SIZE_MASK;
>> +
>> +		writel(reg, &ring->cfg->size);
>> +	} else {
>> +		ret = ringacc->tisci_ring_ops->set_cfg(ringacc->tisci, &ring_cfg);
>> +		if (ret)
>> +			dev_err(ringacc->dev, "TISCI ring free fail (%d) ring_idx %d\n",
>> +				ret, ring->ring_id);
>> +	}
>>  }
>>  
>>  int k3_ringacc_ring_free(struct k3_ring *ring)
>> @@ -670,15 +767,37 @@ int k3_ringacc_get_ring_irq_num(struct k3_ring *ring)
>>  }
>>  EXPORT_SYMBOL_GPL(k3_ringacc_get_ring_irq_num);
>>  
>> +u32 k3_ringacc_ring_get_irq_status(struct k3_ring *ring)
>> +{
>> +	struct k3_ringacc *ringacc = ring->parent;
>> +	struct k3_ring *ring2 = &ringacc->rings[ring->ring_id];
> How is ring2 different than ring? Maybe need better name for ring2 here?

addressed this and your other comments in v9.

https://lore.kernel.org/dmaengine/20260922064902.2719979-1-s-adivi@ti.com/

>
>> +
>> +	if (!ring2->intr)
>> +		return 0;
>> +
>> +	return readl(&ring2->intr->status);
>> +}
>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_get_irq_status);
>> +
>> +void k3_ringacc_ring_clear_irq(struct k3_ring *ring)
>> +{
>> +	struct k3_ringacc *ringacc = ring->parent;
>> +	struct k3_ring *ring2 = &ringacc->rings[ring->ring_id];
>> +
> Same here
>
>> +	if (!ring2->intr)
>> +		return;
>> +
>> +	u32 status = readl(&ring2->intr->status);
> Not a good idea to clear all the IRQs unconditionally, this function 
> should ideally take a param of status to be cleared.
>