[PATCH 1/2] media: ipu6: fix the wrong type cast and 64-bit division

[PATCH 1/2] media: ipu6: fix the wrong type cast and 64-bit division

[bingbu.cao]

From: Bingbu Cao <bingbu.cao@intel.com>

This patch fixes the build errors with `i386-allmodconfig`, the
errors are caused by wrong type cast and 64-bit division.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-cpd.c          | 6 +++---
 drivers/media/pci/intel/ipu6/ipu6-fw-com.c       | 8 ++++----
 drivers/media/pci/intel/ipu6/ipu6-isys-dwc-phy.c | 8 ++++----
 drivers/media/pci/intel/ipu6/ipu6-isys.c         | 2 +-
 drivers/media/pci/intel/ipu6/ipu6.c              | 3 ++-
 5 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-cpd.c b/drivers/media/pci/intel/ipu6/ipu6-cpd.c
index 715b21ab4b8e..9b46b91cfc1a 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-cpd.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-cpd.c
@@ -43,9 +43,9 @@
  * 63:56        55      54:48   47:32   31:24   23:0
  * Rsvd         Rsvd    Type    Version Rsvd    Size
  */
-#define PKG_DIR_SIZE_MASK	GENMASK(23, 0)
-#define PKG_DIR_VERSION_MASK	GENMASK(47, 32)
-#define PKG_DIR_TYPE_MASK	GENMASK(54, 48)
+#define PKG_DIR_SIZE_MASK	GENMASK_ULL(23, 0)
+#define PKG_DIR_VERSION_MASK	GENMASK_ULL(47, 32)
+#define PKG_DIR_TYPE_MASK	GENMASK_ULL(54, 48)
 
 static inline const struct ipu6_cpd_ent *ipu6_cpd_get_entry(const void *cpd,
 							    u8 idx)
diff --git a/drivers/media/pci/intel/ipu6/ipu6-fw-com.c b/drivers/media/pci/intel/ipu6/ipu6-fw-com.c
index 0b33fe9e703d..6da2aca69bd3 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-fw-com.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-fw-com.c
@@ -30,7 +30,7 @@
 
 /* Shared structure between driver and FW - do not modify */
 struct ipu6_fw_sys_queue {
-	u64 host_address;
+	uintptr_t host_address;
 	u32 vied_address;
 	u32 size;
 	u32 token_size;
@@ -40,7 +40,7 @@ struct ipu6_fw_sys_queue {
 } __packed;
 
 struct ipu6_fw_sys_queue_res {
-	u64 host_address;
+	uintptr_t host_address;
 	u32 vied_address;
 	u32 reg;
 } __packed;
@@ -242,7 +242,7 @@ void *ipu6_fw_com_prepare(struct ipu6_fw_com_cfg *cfg,
 	/* initialize input queues */
 	offset += specific_size;
 	res.reg = SYSCOM_QPR_BASE_REG;
-	res.host_address = (u64)(ctx->dma_buffer + offset);
+	res.host_address = (uintptr_t)(ctx->dma_buffer + offset);
 	res.vied_address = ctx->dma_addr + offset;
 	for (i = 0; i < cfg->num_input_queues; i++)
 		ipu6_sys_queue_init(ctx->input_queue + i,
@@ -251,7 +251,7 @@ void *ipu6_fw_com_prepare(struct ipu6_fw_com_cfg *cfg,
 
 	/* initialize output queues */
 	offset += sizeinput;
-	res.host_address = (u64)(ctx->dma_buffer + offset);
+	res.host_address = (uintptr_t)(ctx->dma_buffer + offset);
 	res.vied_address = ctx->dma_addr + offset;
 	for (i = 0; i < cfg->num_output_queues; i++) {
 		ipu6_sys_queue_init(ctx->output_queue + i,
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-dwc-phy.c b/drivers/media/pci/intel/ipu6/ipu6-isys-dwc-phy.c
index 1715275e6776..1cfc9a45bcea 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-dwc-phy.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-dwc-phy.c
@@ -67,8 +67,8 @@ static void dwc_dphy_write(struct ipu6_isys *isys, u32 phy_id, u32 addr,
 	void __iomem *isys_base = isys->pdata->base;
 	void __iomem *base = isys_base + IPU6_DWC_DPHY_BASE(phy_id);
 
-	dev_dbg(dev, "write: reg 0x%lx = data 0x%x", base + addr - isys_base,
-		data);
+	dev_dbg(dev, "write: reg 0x%lx = data 0x%x",
+		(ulong)(base + addr - isys_base), data);
 	writel(data, base + addr);
 }
 
@@ -80,8 +80,8 @@ static u32 dwc_dphy_read(struct ipu6_isys *isys, u32 phy_id, u32 addr)
 	u32 data;
 
 	data = readl(base + addr);
-	dev_dbg(dev, "read: reg 0x%lx = data 0x%x", base + addr - isys_base,
-		data);
+	dev_dbg(dev, "read: reg 0x%lx = data 0x%x",
+		(ulong)(base + addr - isys_base), data);
 
 	return data;
 }
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
index c4aff2e2009b..51c1a567eff8 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
@@ -576,7 +576,7 @@ void update_watermark_setting(struct ipu6_isys *isys)
 	}
 
 	enable_iwake(isys, true);
-	calc_fill_time_us = max_sram_size / isys_pb_datarate_mbs;
+	calc_fill_time_us = div64_u64(max_sram_size, isys_pb_datarate_mbs);
 
 	if (isys->pdata->ipdata->enhanced_iwake) {
 		ltr = isys->pdata->ipdata->ltr;
diff --git a/drivers/media/pci/intel/ipu6/ipu6.c b/drivers/media/pci/intel/ipu6/ipu6.c
index 7fb707d35309..797d11d1d3b1 100644
--- a/drivers/media/pci/intel/ipu6/ipu6.c
+++ b/drivers/media/pci/intel/ipu6/ipu6.c
@@ -247,7 +247,8 @@ ipu6_pkg_dir_configure_spc(struct ipu6_device *isp,
 		dma_addr = sg_dma_address(isp->psys->fw_sgt.sgl);
 
 	pg_offset = server_fw_addr - dma_addr;
-	prog = (struct ipu6_cell_program *)((u64)isp->cpd_fw->data + pg_offset);
+	prog = (struct ipu6_cell_program *)((uintptr_t)isp->cpd_fw->data +
+					    pg_offset);
 	spc_base = base + prog->regs_addr;
 	if (spc_base != (base + hw_variant->spc_offset))
 		dev_warn(&isp->pdev->dev,
-- 
2.34.1

[PATCH 2/2] media: ipu6: remove redundant dependency in Kconfig

[bingbu.cao]

From: Bingbu Cao <bingbu.cao@intel.com>

IPU6 driver simply depends on X86, X86 and 64BIT cover the
X86_64, redundant X86_64 dependency in Kconfig could be removed.

Fixes: c70281cc83d6 ("media: intel/ipu6: add Kconfig and Makefile")
Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/media/pci/intel/ipu6/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/intel/ipu6/Kconfig b/drivers/media/pci/intel/ipu6/Kconfig
index 49e4fb696573..624d8e44c53b 100644
--- a/drivers/media/pci/intel/ipu6/Kconfig
+++ b/drivers/media/pci/intel/ipu6/Kconfig
@@ -2,7 +2,7 @@ config VIDEO_INTEL_IPU6
 	tristate "Intel IPU6 driver"
 	depends on ACPI || COMPILE_TEST
 	depends on VIDEO_DEV
-	depends on X86 && X86_64 && HAS_DMA
+	depends on X86 && HAS_DMA
 	depends on IPU_BRIDGE || !IPU_BRIDGE
 	#
 	# This driver incorrectly tries to override the dma_ops.  It should
-- 
2.34.1

Re: [PATCH 1/2] media: ipu6: fix the wrong type cast and 64-bit division

[Andy Shevchenko]

On Tue, Oct 08, 2024 at 02:19:15PM +0800, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> This patch fixes the build errors with `i386-allmodconfig`, the
> errors are caused by wrong type cast and 64-bit division.

Thanks for the change, my comments below.

...

>  /* Shared structure between driver and FW - do not modify */
>  struct ipu6_fw_sys_queue {
> -	u64 host_address;
> +	uintptr_t host_address;

Okay, in the given semantic this probably should be phys_addr_t.
BUT, is this address somehow is going to be used by IPU6 hardware?
If "yes", the type shall not be changed.

Looking at types used I hope the answer is "no", otherwise the types
in the structures should be  properly choose WRT endianess (and what
__packed is doing here? Is it part of the protocol?).

>  	u32 vied_address;
>  	u32 size;
>  	u32 token_size;
> @@ -40,7 +40,7 @@ struct ipu6_fw_sys_queue {
>  } __packed;
>  
>  struct ipu6_fw_sys_queue_res {
> -	u64 host_address;
> +	uintptr_t host_address;

Ditto.

>  	u32 vied_address;
>  	u32 reg;
>  } __packed;

...

> -	dev_dbg(dev, "write: reg 0x%lx = data 0x%x", base + addr - isys_base,
> -		data);
> +	dev_dbg(dev, "write: reg 0x%lx = data 0x%x",
> +		(ulong)(base + addr - isys_base), data);

No, one should use proper specifiers for this. And what the heck 'ulong' is?
Where is it being defined?

...

> -	dev_dbg(dev, "read: reg 0x%lx = data 0x%x", base + addr - isys_base,
> -		data);
> +	dev_dbg(dev, "read: reg 0x%lx = data 0x%x",
> +		(ulong)(base + addr - isys_base), data);

Ditto.

...

>  	pg_offset = server_fw_addr - dma_addr;
> -	prog = (struct ipu6_cell_program *)((u64)isp->cpd_fw->data + pg_offset);
> +	prog = (struct ipu6_cell_program *)((uintptr_t)isp->cpd_fw->data +
> +					    pg_offset);

Side Q: What are the alignment requirements for the prog pointer?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] media: ipu6: remove redundant dependency in Kconfig

[Andy Shevchenko]

On Tue, Oct 08, 2024 at 02:19:16PM +0800, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> IPU6 driver simply depends on X86, X86 and 64BIT cover the
> X86_64, redundant X86_64 dependency in Kconfig could be removed.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Please, fix my email address in the other tag to be @linux,intel.com.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] media: ipu6: fix the wrong type cast and 64-bit division

[Bingbu Cao]

Andy,

Thanks for the comments.

On 10/8/24 11:23 PM, Andy Shevchenko wrote:
> On Tue, Oct 08, 2024 at 02:19:15PM +0800, bingbu.cao@intel.com wrote:
>> From: Bingbu Cao <bingbu.cao@intel.com>
>>
>> This patch fixes the build errors with `i386-allmodconfig`, the
>> errors are caused by wrong type cast and 64-bit division.
> 
> Thanks for the change, my comments below.
> 
> ...
> 
>>  /* Shared structure between driver and FW - do not modify */
>>  struct ipu6_fw_sys_queue {
>> -	u64 host_address;
>> +	uintptr_t host_address;
> 
> Okay, in the given semantic this probably should be phys_addr_t.
> BUT, is this address somehow is going to be used by IPU6 hardware?
> If "yes", the type shall not be changed.
> 
> Looking at types used I hope the answer is "no", otherwise the types
> in the structures should be  properly choose WRT endianess (and what
> __packed is doing here? Is it part of the protocol?).

You are correct, the type here should not be changed.

> 
>>  	u32 vied_address;
>>  	u32 size;
>>  	u32 token_size;
>> @@ -40,7 +40,7 @@ struct ipu6_fw_sys_queue {
>>  } __packed;
>>  
>>  struct ipu6_fw_sys_queue_res {
>> -	u64 host_address;
>> +	uintptr_t host_address;
> 
> Ditto.
> 
>>  	u32 vied_address;
>>  	u32 reg;
>>  } __packed;
> 
> ...
> 
>> -	dev_dbg(dev, "write: reg 0x%lx = data 0x%x", base + addr - isys_base,
>> -		data);
>> +	dev_dbg(dev, "write: reg 0x%lx = data 0x%x",
>> +		(ulong)(base + addr - isys_base), data);
> 
> No, one should use proper specifiers for this. And what the heck 'ulong' is?
> Where is it being defined?
> 
> ...
> 
>> -	dev_dbg(dev, "read: reg 0x%lx = data 0x%x", base + addr - isys_base,
>> -		data);
>> +	dev_dbg(dev, "read: reg 0x%lx = data 0x%x",
>> +		(ulong)(base + addr - isys_base), data);
> 
> Ditto.
> 
> ...
> 
>>  	pg_offset = server_fw_addr - dma_addr;
>> -	prog = (struct ipu6_cell_program *)((u64)isp->cpd_fw->data + pg_offset);
>> +	prog = (struct ipu6_cell_program *)((uintptr_t)isp->cpd_fw->data +
>> +					    pg_offset);
> 
> Side Q: What are the alignment requirements for the prog pointer?

It should be 64 bytes alignment.

> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH 1/2] media: ipu6: fix the wrong type cast and 64-bit division

[Andy Shevchenko]

On Wed, Oct 09, 2024 at 01:06:32PM +0800, Bingbu Cao wrote:
> On 10/8/24 11:23 PM, Andy Shevchenko wrote:
> > On Tue, Oct 08, 2024 at 02:19:15PM +0800, bingbu.cao@intel.com wrote:
> >> From: Bingbu Cao <bingbu.cao@intel.com>

...

> >>  /* Shared structure between driver and FW - do not modify */
> >>  struct ipu6_fw_sys_queue {
> >> -	u64 host_address;
> >> +	uintptr_t host_address;
> > 
> > Okay, in the given semantic this probably should be phys_addr_t.
> > BUT, is this address somehow is going to be used by IPU6 hardware?
> > If "yes", the type shall not be changed.
> > 
> > Looking at types used I hope the answer is "no", otherwise the types
> > in the structures should be  properly choose WRT endianess (and what
> > __packed is doing here? Is it part of the protocol?).
> 
> You are correct, the type here should not be changed.

Okay, so perhaps for now we can amend the code to work around 32-bit
compilation issues while leaving the type the same. Ideally this struct should
be modified to follow endianess (all those types with __le prefixes), but
that is another story.

> >>  	u32 vied_address;
> >>  	u32 size;
> >>  	u32 token_size;

> >>  } __packed;

...

> > Side Q: What are the alignment requirements for the prog pointer?
> 
> It should be 64 bytes alignment.

Okay, so in this case it's good to cast like this. Perhaps a comment should be
added at some point, because it sounds like I already asked the very same
question a while ago.

-- 
With Best Regards,
Andy Shevchenko