Hi Laurent Pinchart,
Thanks for the feedback.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Saturday, September 7, 2024 1:00 AM
> Subject: Re: [PATCH] media: platform: rzg2l-cru: rzg2l-video: Set AXI burst max length
>
> Hi Biju,
>
> Thank you for the patch.
>
> On Thu, Sep 05, 2024 at 12:18:26PM +0100, Biju Das wrote:
> > As per the hardware manual section 35.2.3.26 'AXI Master Transfer
> > Setting Register for CRU Image Data;, it is mentioned that to improve
> > the transfer
>
> s/;/'/
Oops, Will fix this in next version.
>
> > performance of CRU, it is recommended to use AXILEN value '0xf' for
> > AXI burst max length setting for image data.
> >
> > Signed-off-by: Hien Huynh <hien.huynh.px@xxxxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index 374dc084717f..d17e3eac4177 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -52,6 +52,11 @@
> > #define AMnMBS 0x14c
> > #define AMnMBS_MBSTS 0x7
> >
> > +/* AXI Master Transfer Setting Register for CRU Image Data */
> > +#define AMnAXIATTR 0x158
> > +#define AMnAXIATTR_AXILEN_MASK GENMASK(3, 0)
> > +#define AMnAXIATTR_AXILEN (0xf)
> > +
> > /* AXI Master FIFO Pointer Register for CRU Image Data */
> > #define AMnFIFOPNTR 0x168
> > #define AMnFIFOPNTR_FIFOWPNTR GENMASK(7, 0)
> > @@ -278,6 +283,7 @@ static void rzg2l_cru_fill_hw_slot(struct
> > rzg2l_cru_dev *cru, int slot) static void
> > rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru) {
> > unsigned int slot;
> > + u32 amnaxiattr;
> >
> > /*
> > * Set image data memory banks.
> > @@ -287,6 +293,11 @@ static void rzg2l_cru_initialize_axi(struct
> > rzg2l_cru_dev *cru)
> >
> > for (slot = 0; slot < cru->num_buf; slot++)
> > rzg2l_cru_fill_hw_slot(cru, slot);
> > +
> > + /* Set AXI burst max length to recommended setting */
> > + amnaxiattr = rzg2l_cru_read(cru, AMnAXIATTR) & ~AMnAXIATTR_AXILEN_MASK;
> > + amnaxiattr |= AMnAXIATTR_AXILEN;
> > + rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
>
> It would be more efficient to just write
>
> rzg2l_cru_write(cru, AMnAXIATTR, AMnAXIATTR_AXILEN);
I thought about that. But then re-reading register description changed the mind because
of the below bits
{9,8} — 01b R Reserved:
When read, the initial value is read. When writing, be sure to write the initial value.
Operation is not guaranteed if a value other than the initial value is written.
{6,4} — 101b R Reserved:
When read, the initial value is read. When writing, be sure to write the initial value.
Operation is not guaranteed if a value other than the initial value is written.
Also, bits {27,24}, {22,16} and {13,12} -0b :
It is mentioned that
When read, the initial value is read. When writing, be sure to write the initial value.
operation is not guaranteed if a value other than the initial value is written.
Cheers,
Biju
>
> the hardware manual however doesn't make it clear if this is safe or not. The rest of the register is
> reserved, and writes as documented as ignored, but the reset value is non-zero. If it's not safe to
> write the reserved bits to 0,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> > }
> >
> > static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool
> > *input_is_yuv,
>
> --
> Regards,
>
> Laurent Pinchart
