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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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
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
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
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
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
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