linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/3] spi: xilinx: Massage xilinx_spi.h
@ 2024-03-07 15:43 Andy Shevchenko
  2024-03-07 15:43 ` [PATCH v1 1/3] spi: xilinx: Fix kernel documentation in the xilinx_spi.h Andy Shevchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Andy Shevchenko @ 2024-03-07 15:43 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-arm-kernel, linux-kernel
  Cc: Mark Brown, Michal Simek

Fix kernel documentation and inclusion block, and dropping the size
of the num_chipselect.

Andy Shevchenko (3):
  spi: xilinx: Fix kernel documentation in the xilinx_spi.h
  spi: xilinx: Add necessary inclusion and forward declaration
  spi: xilinx: Make num_chipselect 8-bit in the struct
    xspi_platform_data

 include/linux/spi/xilinx_spi.h | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

-- 
2.43.0.rc1.1.gbec44491f096


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v1 1/3] spi: xilinx: Fix kernel documentation in the xilinx_spi.h
  2024-03-07 15:43 [PATCH v1 0/3] spi: xilinx: Massage xilinx_spi.h Andy Shevchenko
@ 2024-03-07 15:43 ` Andy Shevchenko
  2024-03-08  8:22   ` Michal Simek
  2024-03-07 15:43 ` [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration Andy Shevchenko
  2024-03-07 15:43 ` [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data Andy Shevchenko
  2 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-03-07 15:43 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-arm-kernel, linux-kernel
  Cc: Mark Brown, Michal Simek

While updating the data structure layout the kernel documentation
became outdated. Synchronize kernel documentation with the actual
data structure layout.

Fixes: 1dd46599f83a ("spi: xilinx: add force_irq for QSPI mode")
Fixes: 082339bc63cc ("spi: spi-xilinx: Add run run-time endian detection")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/spi/xilinx_spi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
index 3934ce789d87..fd6add419e94 100644
--- a/include/linux/spi/xilinx_spi.h
+++ b/include/linux/spi/xilinx_spi.h
@@ -5,10 +5,10 @@
 /**
  * struct xspi_platform_data - Platform data of the Xilinx SPI driver
  * @num_chipselect:	Number of chip select by the IP.
- * @little_endian:	If registers should be accessed little endian or not.
  * @bits_per_word:	Number of bits per word.
  * @devices:		Devices to add when the driver is probed.
  * @num_devices:	Number of devices in the devices array.
+ * @force_irq:		If set, forces QSPI transaction requirements.
  */
 struct xspi_platform_data {
 	u16 num_chipselect;
-- 
2.43.0.rc1.1.gbec44491f096


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration
  2024-03-07 15:43 [PATCH v1 0/3] spi: xilinx: Massage xilinx_spi.h Andy Shevchenko
  2024-03-07 15:43 ` [PATCH v1 1/3] spi: xilinx: Fix kernel documentation in the xilinx_spi.h Andy Shevchenko
@ 2024-03-07 15:43 ` Andy Shevchenko
  2024-03-08  8:21   ` Michal Simek
  2024-03-07 15:43 ` [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data Andy Shevchenko
  2 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-03-07 15:43 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-arm-kernel, linux-kernel
  Cc: Mark Brown, Michal Simek

xilinx_spi.h is mnissing inclusion and forward declaration, add them.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/spi/xilinx_spi.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
index fd6add419e94..4ba8f53ce570 100644
--- a/include/linux/spi/xilinx_spi.h
+++ b/include/linux/spi/xilinx_spi.h
@@ -2,6 +2,10 @@
 #ifndef __LINUX_SPI_XILINX_SPI_H
 #define __LINUX_SPI_XILINX_SPI_H
 
+#include <linux/types.h>
+
+struct spi_board_info;
+
 /**
  * struct xspi_platform_data - Platform data of the Xilinx SPI driver
  * @num_chipselect:	Number of chip select by the IP.
-- 
2.43.0.rc1.1.gbec44491f096


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data
  2024-03-07 15:43 [PATCH v1 0/3] spi: xilinx: Massage xilinx_spi.h Andy Shevchenko
  2024-03-07 15:43 ` [PATCH v1 1/3] spi: xilinx: Fix kernel documentation in the xilinx_spi.h Andy Shevchenko
  2024-03-07 15:43 ` [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration Andy Shevchenko
@ 2024-03-07 15:43 ` Andy Shevchenko
  2024-03-08  8:20   ` Michal Simek
  2 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-03-07 15:43 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-arm-kernel, linux-kernel
  Cc: Mark Brown, Michal Simek

There is no use for whole 16-bit for the number of chip select pins.
Drop it to 8 bits and reshuffle the data structure layout to avoid
unnecessary paddings.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/spi/xilinx_spi.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
index 4ba8f53ce570..a638ba2a55bd 100644
--- a/include/linux/spi/xilinx_spi.h
+++ b/include/linux/spi/xilinx_spi.h
@@ -8,18 +8,18 @@ struct spi_board_info;
 
 /**
  * struct xspi_platform_data - Platform data of the Xilinx SPI driver
+ * @force_irq:		If set, forces QSPI transaction requirements.
  * @num_chipselect:	Number of chip select by the IP.
  * @bits_per_word:	Number of bits per word.
- * @devices:		Devices to add when the driver is probed.
  * @num_devices:	Number of devices in the devices array.
- * @force_irq:		If set, forces QSPI transaction requirements.
+ * @devices:		Devices to add when the driver is probed.
  */
 struct xspi_platform_data {
-	u16 num_chipselect;
-	u8 bits_per_word;
-	struct spi_board_info *devices;
-	u8 num_devices;
 	bool force_irq;
+	u8 num_chipselect;
+	u8 bits_per_word;
+	u8 num_devices;
+	struct spi_board_info *devices;
 };
 
 #endif /* __LINUX_SPI_XILINX_SPI_H */
-- 
2.43.0.rc1.1.gbec44491f096


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data
  2024-03-07 15:43 ` [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data Andy Shevchenko
@ 2024-03-08  8:20   ` Michal Simek
  2024-03-08 13:31     ` Andy Shevchenko
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2024-03-08  8:20 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-arm-kernel, linux-kernel; +Cc: Mark Brown



On 3/7/24 16:43, Andy Shevchenko wrote:
> There is no use for whole 16-bit for the number of chip select pins.
> Drop it to 8 bits and reshuffle the data structure layout to avoid
> unnecessary paddings.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   include/linux/spi/xilinx_spi.h | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
> index 4ba8f53ce570..a638ba2a55bd 100644
> --- a/include/linux/spi/xilinx_spi.h
> +++ b/include/linux/spi/xilinx_spi.h
> @@ -8,18 +8,18 @@ struct spi_board_info;
>   
>   /**
>    * struct xspi_platform_data - Platform data of the Xilinx SPI driver
> + * @force_irq:		If set, forces QSPI transaction requirements.
>    * @num_chipselect:	Number of chip select by the IP.
>    * @bits_per_word:	Number of bits per word.
> - * @devices:		Devices to add when the driver is probed.
>    * @num_devices:	Number of devices in the devices array.
> - * @force_irq:		If set, forces QSPI transaction requirements.
> + * @devices:		Devices to add when the driver is probed.
>    */
>   struct xspi_platform_data {
> -	u16 num_chipselect;
> -	u8 bits_per_word;
> -	struct spi_board_info *devices;
> -	u8 num_devices;
>   	bool force_irq;
> +	u8 num_chipselect;
> +	u8 bits_per_word;
> +	u8 num_devices;

all above have 32bits. It means on 64bit cpu you have 32bit gap here.

> +	struct spi_board_info *devices;


It means this should be like this and then there is no gap between on 
32bit/64bit systems.

struct xspi_platform_data {
	struct spi_board_info *    devices;              /*     0     8 */
	bool                       force_irq;            /*     8     1 */
	u8                         num_chipselect;       /*     9     1 */
	u8                         bits_per_word;        /*    10     1 */
	u8                         num_devices;          /*    11     1 */

	/* size: 16, cachelines: 1, members: 5 */
	/* padding: 4 */
	/* last cacheline: 16 bytes */
};

Thanks,
Michal

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration
  2024-03-07 15:43 ` [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration Andy Shevchenko
@ 2024-03-08  8:21   ` Michal Simek
  2024-03-08 13:56     ` Andy Shevchenko
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2024-03-08  8:21 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-arm-kernel, linux-kernel; +Cc: Mark Brown



On 3/7/24 16:43, Andy Shevchenko wrote:
> xilinx_spi.h is mnissing inclusion and forward declaration, add them.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   include/linux/spi/xilinx_spi.h | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
> index fd6add419e94..4ba8f53ce570 100644
> --- a/include/linux/spi/xilinx_spi.h
> +++ b/include/linux/spi/xilinx_spi.h
> @@ -2,6 +2,10 @@
>   #ifndef __LINUX_SPI_XILINX_SPI_H
>   #define __LINUX_SPI_XILINX_SPI_H
>   
> +#include <linux/types.h>
> +
> +struct spi_board_info;
> +
>   /**
>    * struct xspi_platform_data - Platform data of the Xilinx SPI driver
>    * @num_chipselect:	Number of chip select by the IP.

Likely correct but forget how to check it with tools.

Thanks,
Michal


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 1/3] spi: xilinx: Fix kernel documentation in the xilinx_spi.h
  2024-03-07 15:43 ` [PATCH v1 1/3] spi: xilinx: Fix kernel documentation in the xilinx_spi.h Andy Shevchenko
@ 2024-03-08  8:22   ` Michal Simek
  0 siblings, 0 replies; 17+ messages in thread
From: Michal Simek @ 2024-03-08  8:22 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-arm-kernel, linux-kernel; +Cc: Mark Brown



On 3/7/24 16:43, Andy Shevchenko wrote:
> While updating the data structure layout the kernel documentation
> became outdated. Synchronize kernel documentation with the actual
> data structure layout.
> 
> Fixes: 1dd46599f83a ("spi: xilinx: add force_irq for QSPI mode")
> Fixes: 082339bc63cc ("spi: spi-xilinx: Add run run-time endian detection")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   include/linux/spi/xilinx_spi.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
> index 3934ce789d87..fd6add419e94 100644
> --- a/include/linux/spi/xilinx_spi.h
> +++ b/include/linux/spi/xilinx_spi.h
> @@ -5,10 +5,10 @@
>   /**
>    * struct xspi_platform_data - Platform data of the Xilinx SPI driver
>    * @num_chipselect:	Number of chip select by the IP.
> - * @little_endian:	If registers should be accessed little endian or not.
>    * @bits_per_word:	Number of bits per word.
>    * @devices:		Devices to add when the driver is probed.
>    * @num_devices:	Number of devices in the devices array.
> + * @force_irq:		If set, forces QSPI transaction requirements.
>    */
>   struct xspi_platform_data {
>   	u16 num_chipselect;

Reviewed-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data
  2024-03-08  8:20   ` Michal Simek
@ 2024-03-08 13:31     ` Andy Shevchenko
  2024-03-08 13:48       ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-03-08 13:31 UTC (permalink / raw)
  To: Michal Simek; +Cc: linux-spi, linux-arm-kernel, linux-kernel, Mark Brown

On Fri, Mar 08, 2024 at 09:20:23AM +0100, Michal Simek wrote:
> On 3/7/24 16:43, Andy Shevchenko wrote:

...

> >   struct xspi_platform_data {
> > -	u16 num_chipselect;
> > -	u8 bits_per_word;
> > -	struct spi_board_info *devices;
> > -	u8 num_devices;
> >   	bool force_irq;
> > +	u8 num_chipselect;
> > +	u8 bits_per_word;
> > +	u8 num_devices;
> 
> all above have 32bits. It means on 64bit cpu you have 32bit gap here.

> > +	struct spi_board_info *devices;

On all architectures? I mean do all 64-bit architecture ABIs _require_
the pointer to be aligned at 8-byte boundary? Even if so, the struct
itself can be aligned on 4-byte boundary.

> It means this should be like this and then there is no gap between on
> 32bit/64bit systems.
> 
> struct xspi_platform_data {
> 	struct spi_board_info *    devices;              /*     0     8 */
> 	bool                       force_irq;            /*     8     1 */
> 	u8                         num_chipselect;       /*     9     1 */
> 	u8                         bits_per_word;        /*    10     1 */
> 	u8                         num_devices;          /*    11     1 */
> 
> 	/* size: 16, cachelines: 1, members: 5 */
> 	/* padding: 4 */
> 	/* last cacheline: 16 bytes */
> };

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data
  2024-03-08 13:31     ` Andy Shevchenko
@ 2024-03-08 13:48       ` Michal Simek
  2024-03-08 13:55         ` Andy Shevchenko
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2024-03-08 13:48 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-spi, linux-arm-kernel, linux-kernel, Mark Brown



On 3/8/24 14:31, Andy Shevchenko wrote:
> On Fri, Mar 08, 2024 at 09:20:23AM +0100, Michal Simek wrote:
>> On 3/7/24 16:43, Andy Shevchenko wrote:
> 
> ...
> 
>>>    struct xspi_platform_data {
>>> -	u16 num_chipselect;
>>> -	u8 bits_per_word;
>>> -	struct spi_board_info *devices;
>>> -	u8 num_devices;
>>>    	bool force_irq;
>>> +	u8 num_chipselect;
>>> +	u8 bits_per_word;
>>> +	u8 num_devices;
>>
>> all above have 32bits. It means on 64bit cpu you have 32bit gap here.
> 
>>> +	struct spi_board_info *devices;
> 
> On all architectures? I mean do all 64-bit architecture ABIs _require_
> the pointer to be aligned at 8-byte boundary? Even if so, the struct
> itself can be aligned on 4-byte boundary.

I am not able to tell if toolchain enforce 8byte alignment by default/by setup 
on all 64bit systems.
I am using pahole to check this which was recommended by Greg in past which 
reports gap in the middle.

thanks,
Michal




^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data
  2024-03-08 13:48       ` Michal Simek
@ 2024-03-08 13:55         ` Andy Shevchenko
  2024-03-08 14:00           ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-03-08 13:55 UTC (permalink / raw)
  To: Michal Simek; +Cc: linux-spi, linux-arm-kernel, linux-kernel, Mark Brown

On Fri, Mar 08, 2024 at 02:48:04PM +0100, Michal Simek wrote:
> On 3/8/24 14:31, Andy Shevchenko wrote:
> > On Fri, Mar 08, 2024 at 09:20:23AM +0100, Michal Simek wrote:
> > > On 3/7/24 16:43, Andy Shevchenko wrote:

...

> > > >    struct xspi_platform_data {
> > > > -	u16 num_chipselect;
> > > > -	u8 bits_per_word;
> > > > -	struct spi_board_info *devices;
> > > > -	u8 num_devices;
> > > >    	bool force_irq;
> > > > +	u8 num_chipselect;
> > > > +	u8 bits_per_word;
> > > > +	u8 num_devices;
> > > 
> > > all above have 32bits. It means on 64bit cpu you have 32bit gap here.
> > 
> > > > +	struct spi_board_info *devices;
> > 
> > On all architectures? I mean do all 64-bit architecture ABIs _require_
> > the pointer to be aligned at 8-byte boundary? Even if so, the struct
> > itself can be aligned on 4-byte boundary.
> 
> I am not able to tell if toolchain enforce 8byte alignment by default/by
> setup on all 64bit systems.
> I am using pahole to check this which was recommended by Greg in past which
> reports gap in the middle.

I see, thanks for explanation.

Yes, it's likely that in some cases it will be a gap on 64-bit platforms, but
after this patch no gap on 32-bit. Do you still want me to reshuffle that as
you suggested?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration
  2024-03-08  8:21   ` Michal Simek
@ 2024-03-08 13:56     ` Andy Shevchenko
  2024-03-08 14:02       ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-03-08 13:56 UTC (permalink / raw)
  To: Michal Simek; +Cc: linux-spi, linux-arm-kernel, linux-kernel, Mark Brown

On Fri, Mar 08, 2024 at 09:21:32AM +0100, Michal Simek wrote:
> On 3/7/24 16:43, Andy Shevchenko wrote:
> > xilinx_spi.h is mnissing inclusion and forward declaration, add them.

...

> > diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
> >   #define __LINUX_SPI_XILINX_SPI_H
> > +#include <linux/types.h>
> > +
> > +struct spi_board_info;
> > +
> >   /**
> >    * struct xspi_platform_data - Platform data of the Xilinx SPI driver
> >    * @num_chipselect:	Number of chip select by the IP.
> 
> Likely correct but forget how to check it with tools.

I'm not sure which tools we have to check this.

The types.h is needed for uXX
The forward declaration for the pointer to the mentioned type.

All this has been derived from reading the file.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data
  2024-03-08 13:55         ` Andy Shevchenko
@ 2024-03-08 14:00           ` Michal Simek
  2024-03-08 15:01             ` Andy Shevchenko
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2024-03-08 14:00 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-spi, linux-arm-kernel, linux-kernel, Mark Brown



On 3/8/24 14:55, Andy Shevchenko wrote:
> On Fri, Mar 08, 2024 at 02:48:04PM +0100, Michal Simek wrote:
>> On 3/8/24 14:31, Andy Shevchenko wrote:
>>> On Fri, Mar 08, 2024 at 09:20:23AM +0100, Michal Simek wrote:
>>>> On 3/7/24 16:43, Andy Shevchenko wrote:
> 
> ...
> 
>>>>>     struct xspi_platform_data {
>>>>> -	u16 num_chipselect;
>>>>> -	u8 bits_per_word;
>>>>> -	struct spi_board_info *devices;
>>>>> -	u8 num_devices;
>>>>>     	bool force_irq;
>>>>> +	u8 num_chipselect;
>>>>> +	u8 bits_per_word;
>>>>> +	u8 num_devices;
>>>>
>>>> all above have 32bits. It means on 64bit cpu you have 32bit gap here.
>>>
>>>>> +	struct spi_board_info *devices;
>>>
>>> On all architectures? I mean do all 64-bit architecture ABIs _require_
>>> the pointer to be aligned at 8-byte boundary? Even if so, the struct
>>> itself can be aligned on 4-byte boundary.
>>
>> I am not able to tell if toolchain enforce 8byte alignment by default/by
>> setup on all 64bit systems.
>> I am using pahole to check this which was recommended by Greg in past which
>> reports gap in the middle.
> 
> I see, thanks for explanation.
> 
> Yes, it's likely that in some cases it will be a gap on 64-bit platforms, but
> after this patch no gap on 32-bit. Do you still want me to reshuffle that as
> you suggested?

Yes I would prefer to do that change when you are doing cleanup.

M

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration
  2024-03-08 13:56     ` Andy Shevchenko
@ 2024-03-08 14:02       ` Michal Simek
  2024-03-08 15:00         ` Andy Shevchenko
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2024-03-08 14:02 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-spi, linux-arm-kernel, linux-kernel, Mark Brown



On 3/8/24 14:56, Andy Shevchenko wrote:
> On Fri, Mar 08, 2024 at 09:21:32AM +0100, Michal Simek wrote:
>> On 3/7/24 16:43, Andy Shevchenko wrote:
>>> xilinx_spi.h is mnissing inclusion and forward declaration, add them.
> 
> ...
> 
>>> diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
>>>    #define __LINUX_SPI_XILINX_SPI_H
>>> +#include <linux/types.h>
>>> +
>>> +struct spi_board_info;
>>> +
>>>    /**
>>>     * struct xspi_platform_data - Platform data of the Xilinx SPI driver
>>>     * @num_chipselect:	Number of chip select by the IP.
>>
>> Likely correct but forget how to check it with tools.
> 
> I'm not sure which tools we have to check this.
> 
> The types.h is needed for uXX
> The forward declaration for the pointer to the mentioned type.
> 
> All this has been derived from reading the file.

No issue with it. But I am quite sure there was a tool which was able to find it 
out and report it. But forgot which one was it.

M

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration
  2024-03-08 14:02       ` Michal Simek
@ 2024-03-08 15:00         ` Andy Shevchenko
  2024-03-08 15:27           ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-03-08 15:00 UTC (permalink / raw)
  To: Michal Simek; +Cc: linux-spi, linux-arm-kernel, linux-kernel, Mark Brown

On Fri, Mar 08, 2024 at 03:02:01PM +0100, Michal Simek wrote:
> On 3/8/24 14:56, Andy Shevchenko wrote:
> > On Fri, Mar 08, 2024 at 09:21:32AM +0100, Michal Simek wrote:
> > > On 3/7/24 16:43, Andy Shevchenko wrote:
> > > > xilinx_spi.h is mnissing inclusion and forward declaration, add them.

...

> > > > diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
> > > >    #define __LINUX_SPI_XILINX_SPI_H
> > > > +#include <linux/types.h>
> > > > +
> > > > +struct spi_board_info;
> > > > +
> > > >    /**
> > > >     * struct xspi_platform_data - Platform data of the Xilinx SPI driver
> > > >     * @num_chipselect:	Number of chip select by the IP.
> > > 
> > > Likely correct but forget how to check it with tools.
> > 
> > I'm not sure which tools we have to check this.
> > 
> > The types.h is needed for uXX
> > The forward declaration for the pointer to the mentioned type.
> > 
> > All this has been derived from reading the file.
> 
> No issue with it. But I am quite sure there was a tool which was able to
> find it out and report it. But forgot which one was it.

You mean iwyu? If so, it needs a lot of tuning before being able to be used
in Linux kernel.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data
  2024-03-08 14:00           ` Michal Simek
@ 2024-03-08 15:01             ` Andy Shevchenko
  2024-03-08 15:01               ` Andy Shevchenko
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-03-08 15:01 UTC (permalink / raw)
  To: Michal Simek; +Cc: linux-spi, linux-arm-kernel, linux-kernel, Mark Brown

On Fri, Mar 08, 2024 at 03:00:55PM +0100, Michal Simek wrote:
> On 3/8/24 14:55, Andy Shevchenko wrote:
> > On Fri, Mar 08, 2024 at 02:48:04PM +0100, Michal Simek wrote:
> > > On 3/8/24 14:31, Andy Shevchenko wrote:
> > > > On Fri, Mar 08, 2024 at 09:20:23AM +0100, Michal Simek wrote:
> > > > > On 3/7/24 16:43, Andy Shevchenko wrote:

...

> > > > > >     struct xspi_platform_data {
> > > > > > -	u16 num_chipselect;
> > > > > > -	u8 bits_per_word;
> > > > > > -	struct spi_board_info *devices;
> > > > > > -	u8 num_devices;
> > > > > >     	bool force_irq;
> > > > > > +	u8 num_chipselect;
> > > > > > +	u8 bits_per_word;
> > > > > > +	u8 num_devices;
> > > > > 
> > > > > all above have 32bits. It means on 64bit cpu you have 32bit gap here.
> > > > 
> > > > > > +	struct spi_board_info *devices;
> > > > 
> > > > On all architectures? I mean do all 64-bit architecture ABIs _require_
> > > > the pointer to be aligned at 8-byte boundary? Even if so, the struct
> > > > itself can be aligned on 4-byte boundary.
> > > 
> > > I am not able to tell if toolchain enforce 8byte alignment by default/by
> > > setup on all 64bit systems.
> > > I am using pahole to check this which was recommended by Greg in past which
> > > reports gap in the middle.
> > 
> > I see, thanks for explanation.
> > 
> > Yes, it's likely that in some cases it will be a gap on 64-bit platforms, but
> > after this patch no gap on 32-bit. Do you still want me to reshuffle that as
> > you suggested?
> 
> Yes I would prefer to do that change when you are doing cleanup.

Can you give your tag? Then I prepare a new version with addressed comments
against last patch.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data
  2024-03-08 15:01             ` Andy Shevchenko
@ 2024-03-08 15:01               ` Andy Shevchenko
  0 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2024-03-08 15:01 UTC (permalink / raw)
  To: Michal Simek; +Cc: linux-spi, linux-arm-kernel, linux-kernel, Mark Brown

On Fri, Mar 08, 2024 at 05:01:20PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 08, 2024 at 03:00:55PM +0100, Michal Simek wrote:
> > On 3/8/24 14:55, Andy Shevchenko wrote:
> > > On Fri, Mar 08, 2024 at 02:48:04PM +0100, Michal Simek wrote:
> > > > On 3/8/24 14:31, Andy Shevchenko wrote:
> > > > > On Fri, Mar 08, 2024 at 09:20:23AM +0100, Michal Simek wrote:
> > > > > > On 3/7/24 16:43, Andy Shevchenko wrote:

...

> > > > > > >     struct xspi_platform_data {
> > > > > > > -	u16 num_chipselect;
> > > > > > > -	u8 bits_per_word;
> > > > > > > -	struct spi_board_info *devices;
> > > > > > > -	u8 num_devices;
> > > > > > >     	bool force_irq;
> > > > > > > +	u8 num_chipselect;
> > > > > > > +	u8 bits_per_word;
> > > > > > > +	u8 num_devices;
> > > > > > 
> > > > > > all above have 32bits. It means on 64bit cpu you have 32bit gap here.
> > > > > 
> > > > > > > +	struct spi_board_info *devices;
> > > > > 
> > > > > On all architectures? I mean do all 64-bit architecture ABIs _require_
> > > > > the pointer to be aligned at 8-byte boundary? Even if so, the struct
> > > > > itself can be aligned on 4-byte boundary.
> > > > 
> > > > I am not able to tell if toolchain enforce 8byte alignment by default/by
> > > > setup on all 64bit systems.
> > > > I am using pahole to check this which was recommended by Greg in past which
> > > > reports gap in the middle.
> > > 
> > > I see, thanks for explanation.
> > > 
> > > Yes, it's likely that in some cases it will be a gap on 64-bit platforms, but
> > > after this patch no gap on 32-bit. Do you still want me to reshuffle that as
> > > you suggested?
> > 
> > Yes I would prefer to do that change when you are doing cleanup.
> 
> Can you give your tag?

I mean for the other patch, not this one :-)

> Then I prepare a new version with addressed comments
> against last patch.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration
  2024-03-08 15:00         ` Andy Shevchenko
@ 2024-03-08 15:27           ` Michal Simek
  0 siblings, 0 replies; 17+ messages in thread
From: Michal Simek @ 2024-03-08 15:27 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-spi, linux-arm-kernel, linux-kernel, Mark Brown



On 3/8/24 16:00, Andy Shevchenko wrote:
> On Fri, Mar 08, 2024 at 03:02:01PM +0100, Michal Simek wrote:
>> On 3/8/24 14:56, Andy Shevchenko wrote:
>>> On Fri, Mar 08, 2024 at 09:21:32AM +0100, Michal Simek wrote:
>>>> On 3/7/24 16:43, Andy Shevchenko wrote:
>>>>> xilinx_spi.h is mnissing inclusion and forward declaration, add them.
> 
> ...
> 
>>>>> diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
>>>>>     #define __LINUX_SPI_XILINX_SPI_H
>>>>> +#include <linux/types.h>
>>>>> +
>>>>> +struct spi_board_info;
>>>>> +
>>>>>     /**
>>>>>      * struct xspi_platform_data - Platform data of the Xilinx SPI driver
>>>>>      * @num_chipselect:	Number of chip select by the IP.
>>>>
>>>> Likely correct but forget how to check it with tools.
>>>
>>> I'm not sure which tools we have to check this.
>>>
>>> The types.h is needed for uXX
>>> The forward declaration for the pointer to the mentioned type.
>>>
>>> All this has been derived from reading the file.
>>
>> No issue with it. But I am quite sure there was a tool which was able to
>> find it out and report it. But forgot which one was it.
> 
> You mean iwyu? If so, it needs a lot of tuning before being able to be used
> in Linux kernel.
> 

Acked-by; Michal Simek <michal.simek@amd.com>

Thanks,
Michal

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2024-03-08 15:28 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-07 15:43 [PATCH v1 0/3] spi: xilinx: Massage xilinx_spi.h Andy Shevchenko
2024-03-07 15:43 ` [PATCH v1 1/3] spi: xilinx: Fix kernel documentation in the xilinx_spi.h Andy Shevchenko
2024-03-08  8:22   ` Michal Simek
2024-03-07 15:43 ` [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration Andy Shevchenko
2024-03-08  8:21   ` Michal Simek
2024-03-08 13:56     ` Andy Shevchenko
2024-03-08 14:02       ` Michal Simek
2024-03-08 15:00         ` Andy Shevchenko
2024-03-08 15:27           ` Michal Simek
2024-03-07 15:43 ` [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data Andy Shevchenko
2024-03-08  8:20   ` Michal Simek
2024-03-08 13:31     ` Andy Shevchenko
2024-03-08 13:48       ` Michal Simek
2024-03-08 13:55         ` Andy Shevchenko
2024-03-08 14:00           ` Michal Simek
2024-03-08 15:01             ` Andy Shevchenko
2024-03-08 15:01               ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).