diff mbox series

media: platform: rzg2l-cru: rzg2l-video: Set AXI burst max length

Message ID 20240905111828.159670-1-biju.das.jz@bp.renesas.com (mailing list archive)
State New
Headers
Series media: platform: rzg2l-cru: rzg2l-video: Set AXI burst max length |

Checks

Context Check Description
media-ci/media-ci fail Failed to cherry-pick patch on top of media-staging

Commit Message

Biju Das Sept. 5, 2024, 11:18 a.m. UTC 
  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
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@renesas.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c    | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Laurent Pinchart Sept. 6, 2024, 11:59 p.m. UTC | #1 
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/;/'/

> 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@renesas.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  .../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);

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@ideasonboard.com>

>  }
>  
>  static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
Biju Das Sept. 7, 2024, 7:41 a.m. UTC | #2 
Hi Laurent Pinchart,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 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@renesas.com>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  .../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@ideasonboard.com>
> 
> >  }
> >
> >  static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool
> > *input_is_yuv,
> 
> --
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart Sept. 7, 2024, 5:32 p.m. UTC | #3 
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@renesas.com>
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  .../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@ideasonboard.com>
> > 
> > >  }
> > >
> > >  static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool
> > > *input_is_yuv,
Biju Das Sept. 9, 2024, 5:45 a.m. UTC | #4 
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Saturday, September 7, 2024 6:33 PM
> Subject: Re: [PATCH] media: platform: rzg2l-cru: rzg2l-video: Set AXI burst max length
> 
> 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@renesas.com>
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > >  .../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.

Thanks Laurent.

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@ideasonboard.com>
> > >
> > > >  }
> > > >
> > > >  static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool
> > > > *input_is_yuv,
> 
> --
> Regards,
> 
> Laurent Pinchart
diff mbox series

Patch

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);
 }
 
 static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,