Re: [PATCH v4 25/31] hw/arm/tegra241-cmdqv: Add reset handler

2026-05-08 Thread Eric Auger 



On 5/8/26 1:19 PM, Shameer Kolothum Thodi wrote:
>
>> -Original Message-
>> From: Eric Auger 
>> Sent: 07 May 2026 17:52
>> To: Shameer Kolothum Thodi ; qemu-
>> [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; [email protected]; Nicolin
>> Chen ; Nathan Chen ; Matt
>> Ochs ; Jiandi An ; Jason Gunthorpe
>> ; [email protected];
>> [email protected]; Krishnakant Jaju ;
>> [email protected]
>> Subject: Re: [PATCH v4 25/31] hw/arm/tegra241-cmdqv: Add reset handler
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 4/15/26 12:55 PM, Shameer Kolothum wrote:
>>> From: Nicolin Chen 
>>>
>>> Introduce a reset handler for the Tegra241 CMDQV and initialize its
>>> register state.
>>>
>>> Signed-off-by: Nicolin Chen 
>>> Signed-off-by: Shameer Kolothum 
>>> ---
>>>  hw/arm/tegra241-cmdqv.h |  2 ++
>>>  hw/arm/tegra241-cmdqv.c | 50
>> +
>>>  hw/arm/trace-events |  1 +
>>>  3 files changed, 53 insertions(+)
>>>
>>> diff --git a/hw/arm/tegra241-cmdqv.h b/hw/arm/tegra241-cmdqv.h
>>> index 2befa6205e..b2a444daef 100644
>>> --- a/hw/arm/tegra241-cmdqv.h
>>> +++ b/hw/arm/tegra241-cmdqv.h
>>> @@ -79,6 +79,8 @@ FIELD(CONFIG, CMDQ_MAX_CLK_BATCH, 4, 8)
>>>  FIELD(CONFIG, CMDQ_MAX_CMD_BATCH, 12, 8)
>>>  FIELD(CONFIG, CONS_DRAM_EN, 20, 1)
>>>
>>> +#define V_CONFIG_RESET 0x00020403
>>> +
>>>  REG32(PARAM, 0x4)
>>>  FIELD(PARAM, CMDQV_VER, 0, 4)
>>>  FIELD(PARAM, CMDQV_NUM_CMDQ_LOG2, 4, 4)
>>> diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
>>> index 9c2fc02b92..af68add2f0 100644
>>> --- a/hw/arm/tegra241-cmdqv.c
>>> +++ b/hw/arm/tegra241-cmdqv.c
>>> @@ -8,6 +8,7 @@
>>>   */
>>>
>>>  #include "qemu/osdep.h"
>>> +#include "qemu/error-report.h"
>>>  #include "qemu/log.h"
>>>
>>>  #include "hw/arm/smmuv3.h"
>>> @@ -645,8 +646,57 @@ free_viommu:
>>>  return false;
>>>  }
>>>
>>> +static void tegra241_cmdqv_init_regs(SMMUv3State *s, Tegra241CMDQV
>> *cmdqv)
>>> +{
>>> +int i;
>>> +
>>> +cmdqv->config = V_CONFIG_RESET;
>>> +cmdqv->param = FIELD_DP32(0, PARAM, CMDQV_VER, CMDQV_VER);
>> hw/core/registerfields.h:#define FIELD_DP32(storage, reg, field, val)
>> what does this 0?
> I think it is to make sure we have no garbage set for non-field bits.
> Starting from 0 guarantees non-field bits are clean.

OK thanks ;-)

Eric
>
>>> +cmdqv->param = FIELD_DP32(cmdqv->param, PARAM,
>> CMDQV_NUM_CMDQ_LOG2,
>>> +  CMDQV_NUM_CMDQ_LOG2);
>> I was a bit puzzled by the FIELD name being the value name but well.
>>> +cmdqv->param = FIELD_DP32(cmdqv->param, PARAM,
>> CMDQV_NUM_SID_PER_VI_LOG2,
>>> +  CMDQV_NUM_SID_PER_VI_LOG2);
>>> +trace_tegra241_cmdqv_init_regs(cmdqv->param);
>>> +cmdqv->status = R_STATUS_CMDQV_ENABLED_MASK;
>>> +for (i = 0; i < 2; i++) {
>>> +cmdqv->vi_err_map[i] = 0;
>>> +cmdqv->vi_int_mask[i] = 0;
>>> +cmdqv->cmdq_err_map[i] = 0;
>>> +}
>>> +cmdqv->cmdq_err_map[2] = 0;
>>> +cmdqv->cmdq_err_map[3] = 0;
>> the split looks pretty strange, ie. loop of 2 + continue with individual
>> setting. why don't you do the init of
>>
>> cmdqv->cmdq_err_map in the x4 loop below
> 😊. True, that does look strange. I will change.
>
> Thanks,
> Shameer

RE: [PATCH v4 25/31] hw/arm/tegra241-cmdqv: Add reset handler

2026-05-08 Thread Shameer Kolothum Thodi 


> -Original Message-
> From: Eric Auger 
> Sent: 07 May 2026 17:52
> To: Shameer Kolothum Thodi ; qemu-
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected]; Nicolin
> Chen ; Nathan Chen ; Matt
> Ochs ; Jiandi An ; Jason Gunthorpe
> ; [email protected];
> [email protected]; Krishnakant Jaju ;
> [email protected]
> Subject: Re: [PATCH v4 25/31] hw/arm/tegra241-cmdqv: Add reset handler
> 
> External email: Use caution opening links or attachments
> 
> 
> On 4/15/26 12:55 PM, Shameer Kolothum wrote:
> > From: Nicolin Chen 
> >
> > Introduce a reset handler for the Tegra241 CMDQV and initialize its
> > register state.
> >
> > Signed-off-by: Nicolin Chen 
> > Signed-off-by: Shameer Kolothum 
> > ---
> >  hw/arm/tegra241-cmdqv.h |  2 ++
> >  hw/arm/tegra241-cmdqv.c | 50
> +
> >  hw/arm/trace-events |  1 +
> >  3 files changed, 53 insertions(+)
> >
> > diff --git a/hw/arm/tegra241-cmdqv.h b/hw/arm/tegra241-cmdqv.h
> > index 2befa6205e..b2a444daef 100644
> > --- a/hw/arm/tegra241-cmdqv.h
> > +++ b/hw/arm/tegra241-cmdqv.h
> > @@ -79,6 +79,8 @@ FIELD(CONFIG, CMDQ_MAX_CLK_BATCH, 4, 8)
> >  FIELD(CONFIG, CMDQ_MAX_CMD_BATCH, 12, 8)
> >  FIELD(CONFIG, CONS_DRAM_EN, 20, 1)
> >
> > +#define V_CONFIG_RESET 0x00020403
> > +
> >  REG32(PARAM, 0x4)
> >  FIELD(PARAM, CMDQV_VER, 0, 4)
> >  FIELD(PARAM, CMDQV_NUM_CMDQ_LOG2, 4, 4)
> > diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
> > index 9c2fc02b92..af68add2f0 100644
> > --- a/hw/arm/tegra241-cmdqv.c
> > +++ b/hw/arm/tegra241-cmdqv.c
> > @@ -8,6 +8,7 @@
> >   */
> >
> >  #include "qemu/osdep.h"
> > +#include "qemu/error-report.h"
> >  #include "qemu/log.h"
> >
> >  #include "hw/arm/smmuv3.h"
> > @@ -645,8 +646,57 @@ free_viommu:
> >  return false;
> >  }
> >
> > +static void tegra241_cmdqv_init_regs(SMMUv3State *s, Tegra241CMDQV
> *cmdqv)
> > +{
> > +int i;
> > +
> > +cmdqv->config = V_CONFIG_RESET;
> > +cmdqv->param = FIELD_DP32(0, PARAM, CMDQV_VER, CMDQV_VER);
> hw/core/registerfields.h:#define FIELD_DP32(storage, reg, field, val)
> what does this 0?

I think it is to make sure we have no garbage set for non-field bits.
Starting from 0 guarantees non-field bits are clean.

> > +cmdqv->param = FIELD_DP32(cmdqv->param, PARAM,
> CMDQV_NUM_CMDQ_LOG2,
> > +  CMDQV_NUM_CMDQ_LOG2);
> I was a bit puzzled by the FIELD name being the value name but well.
> > +cmdqv->param = FIELD_DP32(cmdqv->param, PARAM,
> CMDQV_NUM_SID_PER_VI_LOG2,
> > +  CMDQV_NUM_SID_PER_VI_LOG2);
> > +trace_tegra241_cmdqv_init_regs(cmdqv->param);
> > +cmdqv->status = R_STATUS_CMDQV_ENABLED_MASK;
> > +for (i = 0; i < 2; i++) {
> > +cmdqv->vi_err_map[i] = 0;
> > +cmdqv->vi_int_mask[i] = 0;
> > +cmdqv->cmdq_err_map[i] = 0;
> > +}
> > +cmdqv->cmdq_err_map[2] = 0;
> > +cmdqv->cmdq_err_map[3] = 0;
> the split looks pretty strange, ie. loop of 2 + continue with individual
> setting. why don't you do the init of
> 
> cmdqv->cmdq_err_map in the x4 loop below

😊. True, that does look strange. I will change.

Thanks,
Shameer

Re: [PATCH v4 25/31] hw/arm/tegra241-cmdqv: Add reset handler

2026-05-07 Thread Eric Auger 



On 4/15/26 12:55 PM, Shameer Kolothum wrote:
> From: Nicolin Chen 
>
> Introduce a reset handler for the Tegra241 CMDQV and initialize its
> register state.
>
> Signed-off-by: Nicolin Chen 
> Signed-off-by: Shameer Kolothum 
> ---
>  hw/arm/tegra241-cmdqv.h |  2 ++
>  hw/arm/tegra241-cmdqv.c | 50 +
>  hw/arm/trace-events |  1 +
>  3 files changed, 53 insertions(+)
>
> diff --git a/hw/arm/tegra241-cmdqv.h b/hw/arm/tegra241-cmdqv.h
> index 2befa6205e..b2a444daef 100644
> --- a/hw/arm/tegra241-cmdqv.h
> +++ b/hw/arm/tegra241-cmdqv.h
> @@ -79,6 +79,8 @@ FIELD(CONFIG, CMDQ_MAX_CLK_BATCH, 4, 8)
>  FIELD(CONFIG, CMDQ_MAX_CMD_BATCH, 12, 8)
>  FIELD(CONFIG, CONS_DRAM_EN, 20, 1)
>  
> +#define V_CONFIG_RESET 0x00020403
please add a comment to explain what 

0x00020403 value means

Eric

> +
>  REG32(PARAM, 0x4)
>  FIELD(PARAM, CMDQV_VER, 0, 4)
>  FIELD(PARAM, CMDQV_NUM_CMDQ_LOG2, 4, 4)
> diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
> index 9c2fc02b92..af68add2f0 100644
> --- a/hw/arm/tegra241-cmdqv.c
> +++ b/hw/arm/tegra241-cmdqv.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qemu/log.h"
>  
>  #include "hw/arm/smmuv3.h"
> @@ -645,8 +646,57 @@ free_viommu:
>  return false;
>  }
>  
> +static void tegra241_cmdqv_init_regs(SMMUv3State *s, Tegra241CMDQV *cmdqv)
> +{
> +int i;
> +
> +cmdqv->config = V_CONFIG_RESET;
> +cmdqv->param = FIELD_DP32(0, PARAM, CMDQV_VER, CMDQV_VER);
> +cmdqv->param = FIELD_DP32(cmdqv->param, PARAM, CMDQV_NUM_CMDQ_LOG2,
> +  CMDQV_NUM_CMDQ_LOG2);
> +cmdqv->param = FIELD_DP32(cmdqv->param, PARAM, CMDQV_NUM_SID_PER_VI_LOG2,
> +  CMDQV_NUM_SID_PER_VI_LOG2);
> +trace_tegra241_cmdqv_init_regs(cmdqv->param);
> +cmdqv->status = R_STATUS_CMDQV_ENABLED_MASK;
> +for (i = 0; i < 2; i++) {
> +cmdqv->vi_err_map[i] = 0;
> +cmdqv->vi_int_mask[i] = 0;
> +cmdqv->cmdq_err_map[i] = 0;
> +}
> +cmdqv->cmdq_err_map[2] = 0;
> +cmdqv->cmdq_err_map[3] = 0;
> +cmdqv->vintf_config = 0;
> +cmdqv->vintf_status = 0;
> +for (i = 0; i < 4; i++) {
> +cmdqv->vintf_cmdq_err_map[i] = 0;
> +}
> +for (i = 0; i < TEGRA241_CMDQV_MAX_CMDQ; i++) {
> +cmdqv->cmdq_alloc_map[i] = 0;
> +cmdqv->vcmdq_cons_indx[i] = 0;
> +cmdqv->vcmdq_prod_indx[i] = 0;
> +cmdqv->vcmdq_config[i] = 0;
> +cmdqv->vcmdq_status[i] = 0;
> +cmdqv->vcmdq_gerror[i] = 0;
> +cmdqv->vcmdq_gerrorn[i] = 0;
> +cmdqv->vcmdq_base[i] = 0;
> +cmdqv->vcmdq_cons_indx_base[i] = 0;
> +}
> +}
> +
>  static void tegra241_cmdqv_reset(SMMUv3State *s)
>  {
> +SMMUv3AccelState *accel = s->s_accel;
> +Tegra241CMDQV *cmdqv = accel->cmdqv;
> +
> +if (!cmdqv) {
> +return;
> +}
> +
> +tegra241_cmdqv_guest_unmap_vintf_page0(cmdqv);
> +tegra241_cmdqv_munmap_vintf_page0(cmdqv, NULL);
> +tegra241_cmdqv_free_all_vcmdq(cmdqv);
> +
> +tegra241_cmdqv_init_regs(s, cmdqv);
>  }
>  
>  static const MemoryRegionOps mmio_cmdqv_ops = {
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index fd6441bfa7..6f602b9eda 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -76,6 +76,7 @@ smmuv3_accel_install_ste(uint32_t vsid, const char * type, 
> uint32_t hwpt_id) "vS
>  tegra241_cmdqv_read_mmio(uint64_t offset, uint64_t val, unsigned size) 
> "offset: 0x%"PRIx64" val: 0x%"PRIx64" size: 0x%x"
>  tegra241_cmdqv_write_mmio(uint64_t offset, uint64_t val, unsigned size) 
> "offset: 0x%"PRIx64" val: 0x%"PRIx64" size: 0x%x"
>  tegra241_cmdqv_err_map(uint32_t map3, uint32_t map2, uint32_t map1, uint32_t 
> map0) "hw irq received. error (hex) maps: %04X:%04X:%04X:%04X"
> +tegra241_cmdqv_init_regs(uint32_t param) "hw info received. param: 0x%04X"
>  
>  # strongarm.c
>  strongarm_uart_update_parameters(const char *label, int speed, char parity, 
> int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"

Re: [PATCH v4 25/31] hw/arm/tegra241-cmdqv: Add reset handler

2026-05-07 Thread Eric Auger 



On 4/15/26 12:55 PM, Shameer Kolothum wrote:
> From: Nicolin Chen 
>
> Introduce a reset handler for the Tegra241 CMDQV and initialize its
> register state.
>
> Signed-off-by: Nicolin Chen 
> Signed-off-by: Shameer Kolothum 
> ---
>  hw/arm/tegra241-cmdqv.h |  2 ++
>  hw/arm/tegra241-cmdqv.c | 50 +
>  hw/arm/trace-events |  1 +
>  3 files changed, 53 insertions(+)
>
> diff --git a/hw/arm/tegra241-cmdqv.h b/hw/arm/tegra241-cmdqv.h
> index 2befa6205e..b2a444daef 100644
> --- a/hw/arm/tegra241-cmdqv.h
> +++ b/hw/arm/tegra241-cmdqv.h
> @@ -79,6 +79,8 @@ FIELD(CONFIG, CMDQ_MAX_CLK_BATCH, 4, 8)
>  FIELD(CONFIG, CMDQ_MAX_CMD_BATCH, 12, 8)
>  FIELD(CONFIG, CONS_DRAM_EN, 20, 1)
>  
> +#define V_CONFIG_RESET 0x00020403
> +
>  REG32(PARAM, 0x4)
>  FIELD(PARAM, CMDQV_VER, 0, 4)
>  FIELD(PARAM, CMDQV_NUM_CMDQ_LOG2, 4, 4)
> diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
> index 9c2fc02b92..af68add2f0 100644
> --- a/hw/arm/tegra241-cmdqv.c
> +++ b/hw/arm/tegra241-cmdqv.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qemu/log.h"
>  
>  #include "hw/arm/smmuv3.h"
> @@ -645,8 +646,57 @@ free_viommu:
>  return false;
>  }
>  
> +static void tegra241_cmdqv_init_regs(SMMUv3State *s, Tegra241CMDQV *cmdqv)
> +{
> +int i;
> +
> +cmdqv->config = V_CONFIG_RESET;
> +cmdqv->param = FIELD_DP32(0, PARAM, CMDQV_VER, CMDQV_VER);
hw/core/registerfields.h:#define FIELD_DP32(storage, reg, field, val)
what does this 0?
> +cmdqv->param = FIELD_DP32(cmdqv->param, PARAM, CMDQV_NUM_CMDQ_LOG2,
> +  CMDQV_NUM_CMDQ_LOG2);
I was a bit puzzled by the FIELD name being the value name but well.
> +cmdqv->param = FIELD_DP32(cmdqv->param, PARAM, CMDQV_NUM_SID_PER_VI_LOG2,
> +  CMDQV_NUM_SID_PER_VI_LOG2);
> +trace_tegra241_cmdqv_init_regs(cmdqv->param);
> +cmdqv->status = R_STATUS_CMDQV_ENABLED_MASK;
> +for (i = 0; i < 2; i++) {
> +cmdqv->vi_err_map[i] = 0;
> +cmdqv->vi_int_mask[i] = 0;
> +cmdqv->cmdq_err_map[i] = 0;
> +}
> +cmdqv->cmdq_err_map[2] = 0;
> +cmdqv->cmdq_err_map[3] = 0;
the split looks pretty strange, ie. loop of 2 + continue with individual
setting. why don't you do the init of 

cmdqv->cmdq_err_map in the x4 loop below

> +cmdqv->vintf_config = 0;
> +cmdqv->vintf_status = 0;
> +for (i = 0; i < 4; i++) {
> +cmdqv->vintf_cmdq_err_map[i] = 0;
> +}
> +for (i = 0; i < TEGRA241_CMDQV_MAX_CMDQ; i++) {
> +cmdqv->cmdq_alloc_map[i] = 0;
> +cmdqv->vcmdq_cons_indx[i] = 0;
> +cmdqv->vcmdq_prod_indx[i] = 0;
> +cmdqv->vcmdq_config[i] = 0;
> +cmdqv->vcmdq_status[i] = 0;
> +cmdqv->vcmdq_gerror[i] = 0;
> +cmdqv->vcmdq_gerrorn[i] = 0;
> +cmdqv->vcmdq_base[i] = 0;
> +cmdqv->vcmdq_cons_indx_base[i] = 0;
> +}
> +}
> +
>  static void tegra241_cmdqv_reset(SMMUv3State *s)
>  {
> +SMMUv3AccelState *accel = s->s_accel;
> +Tegra241CMDQV *cmdqv = accel->cmdqv;
> +
> +if (!cmdqv) {
> +return;
> +}
> +
> +tegra241_cmdqv_guest_unmap_vintf_page0(cmdqv);
> +tegra241_cmdqv_munmap_vintf_page0(cmdqv, NULL);
> +tegra241_cmdqv_free_all_vcmdq(cmdqv);
> +
> +tegra241_cmdqv_init_regs(s, cmdqv);
>  }
>  
>  static const MemoryRegionOps mmio_cmdqv_ops = {
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index fd6441bfa7..6f602b9eda 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -76,6 +76,7 @@ smmuv3_accel_install_ste(uint32_t vsid, const char * type, 
> uint32_t hwpt_id) "vS
>  tegra241_cmdqv_read_mmio(uint64_t offset, uint64_t val, unsigned size) 
> "offset: 0x%"PRIx64" val: 0x%"PRIx64" size: 0x%x"
>  tegra241_cmdqv_write_mmio(uint64_t offset, uint64_t val, unsigned size) 
> "offset: 0x%"PRIx64" val: 0x%"PRIx64" size: 0x%x"
>  tegra241_cmdqv_err_map(uint32_t map3, uint32_t map2, uint32_t map1, uint32_t 
> map0) "hw irq received. error (hex) maps: %04X:%04X:%04X:%04X"
> +tegra241_cmdqv_init_regs(uint32_t param) "hw info received. param: 0x%04X"
>  
>  # strongarm.c
>  strongarm_uart_update_parameters(const char *label, int speed, char parity, 
> int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"
Thanks

Eric