[PATCH v1 0/3] spi: xilinx: Massage xilinx_spi.h

[PATCH v1 0/3] spi: xilinx: Massage xilinx_spi.h

[Andy Shevchenko]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/3] spi: xilinx: Fix kernel documentation in the xilinx_spi.h

[Andy Shevchenko]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/3] spi: xilinx: Fix kernel documentation in the xilinx_spi.h

[Michal Simek]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration

[Andy Shevchenko]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration

[Michal Simek]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration

[Andy Shevchenko]

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration

[Michal Simek]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration

[Andy Shevchenko]

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] spi: xilinx: Add necessary inclusion and forward declaration

[Michal Simek]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data

[Andy Shevchenko]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data

[Michal Simek]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data

[Andy Shevchenko]

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data

[Michal Simek]

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




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data

[Andy Shevchenko]

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data

[Michal Simek]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data

[Andy Shevchenko]

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data

[Andy Shevchenko]

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel