Re: [PATCH 09/28] media: ti-vpe: cal: Add PPI context

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 18/04/2021 15:17, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, Apr 12, 2021 at 02:34:38PM +0300, Tomi Valkeinen wrote:
>> CAL has 8 PPI contexts per PHY, which are used to tag the incoming data.
>> The current driver only uses the first PPI, but we need to support all
>> of them to implement multi-stream support.
>>
>> Add a ppi_ctx field to cal_ctx, which indicates which of the 8 PPI
>> contexts is used for the particular cal_ctx. Also clean up the PPI
>> context register macros to take the PPI context number as a parameter.
> 
> As far as I can tell, the TRMs don't mention "PPI contexts". PPI stands
> for PHY Protocol Interface, it does identify a particular physical
> interface. Would it be better to rename ppi_ctx to csi2_ctx ?  This
> would match the register names too.

At least DRA76 TRM says, for example, these:

"Number of contexts for PPI interface #0"
"The PPI context IDs of interleaved streams must match."
"Each PPI FSM has 8 copies of the CAL_CSI2_CTXy_l (y = [0 to 7],
CAL_CSI2_CTX0_l through CAL_CSI2_CTX7_l) state registers (that is, 
contexts)."

It's true changing ppi_ctx to csi2_ctx would match the register names 
neatly. But then again, the TRM doesn't mention CSI2 context, as far as 
I can tell.

However, I think csi2_ctx is more understandable than ppi_ctx. So 
perhaps I'll change it.

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/platform/ti-vpe/cal.c      | 10 ++++++----
>>   drivers/media/platform/ti-vpe/cal.h      |  1 +
>>   drivers/media/platform/ti-vpe/cal_regs.h | 18 ++----------------
>>   3 files changed, 9 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
>> index 3d57aedbee0a..c550eeb27e79 100644
>> --- a/drivers/media/platform/ti-vpe/cal.c
>> +++ b/drivers/media/platform/ti-vpe/cal.c
>> @@ -294,7 +294,7 @@ static void cal_ctx_csi2_config(struct cal_ctx *ctx)
>>   {
>>   	u32 val;
>>   
>> -	val = cal_read(ctx->cal, CAL_CSI2_CTX0(ctx->index));
>> +	val = cal_read(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx));
>>   	cal_set_field(&val, ctx->cport, CAL_CSI2_CTX_CPORT_MASK);
>>   	/*
>>   	 * DT type: MIPI CSI-2 Specs
>> @@ -310,9 +310,10 @@ static void cal_ctx_csi2_config(struct cal_ctx *ctx)
>>   	cal_set_field(&val, CAL_CSI2_CTX_ATT_PIX, CAL_CSI2_CTX_ATT_MASK);
>>   	cal_set_field(&val, CAL_CSI2_CTX_PACK_MODE_LINE,
>>   		      CAL_CSI2_CTX_PACK_MODE_MASK);
>> -	cal_write(ctx->cal, CAL_CSI2_CTX0(ctx->index), val);
>> -	ctx_dbg(3, ctx, "CAL_CSI2_CTX0(%d) = 0x%08x\n", ctx->index,
>> -		cal_read(ctx->cal, CAL_CSI2_CTX0(ctx->index)));
>> +	cal_write(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx), val);
>> +	ctx_dbg(3, ctx, "CAL_CSI2_CTX%d(%d) = 0x%08x\n",
>> +		ctx->phy->instance, ctx->ppi_ctx,
>> +		cal_read(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx)));
>>   }
>>   
>>   static void cal_ctx_pix_proc_config(struct cal_ctx *ctx)
>> @@ -854,6 +855,7 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst)
>>   	ctx->cal = cal;
>>   	ctx->phy = cal->phy[inst];
>>   	ctx->index = inst;
>> +	ctx->ppi_ctx = inst;
> 
> To avoid a functional change in this patch, should this be = 0 ?

Define "functional change" =). The context used may be different after 
this patch, but there's no change in how the HW functions. I changed it 
to 'inst' here, instead of keeping it as 0, as this works fine for both 
the legacy and new functionality and thus I don't need to remember to 
change it later.

I should mention this in the commit desc, though.

  Tomi