| Message ID | 20240307154510.3795380-4-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: xilinx: Massage xilinx_spi.h | expand |
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
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 */ > };
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?
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.
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.
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 */
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(-)