Hi Biju,
On Sat, Sep 07, 2024 at 07:41:24AM +0000, Biju Das wrote:
> On Saturday, September 7, 2024 1:00 AM, Laurent Pinchart wrote:
> > 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.
Let's keep the code as-is then. I'll fix the typo in (and reflow) the
commit message myself, no need to resubmit.
> > 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
