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@xxxxxxxxxxxxxxxx>
---
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
