| Message ID | 20240805084350.3895788-1-jonas@kwiboo.se |
|---|---|
| Headers | show |
| Series | rockchip: Skip serial pinctrl at pre-reloc phase | expand |
Hi Jonas, On 8/5/24 10:43 AM, Jonas Karlman wrote: > UART pinctrl for serial is typically applied multiple times: > - in external TPL or in TPL for DEBUG_UART in board_debug_uart_init() > - in SPL for DEBUG_UART in board_debug_uart_init() > - in SPL using pinctrl from DT > - in pre-reloc phase using pinctrl from DT > - after relocation using pinctrl from DT > > This series change bootph props for the UART pinctrl to not include them > in pre-reloc phase to save some boot time: > NACK. I feel like this is a hack for vendor trees only. 1) The Device Tree represents the hardware. We need the mux at all times. 2) This breaks users who want to have no serial in TPL/SPL but in proper (where you want it as soon as possible I assume, thus pre-reloc). 3) This likely also means if BL31 is configured to output on a different UART mux of the same controller for some reason (I've seen weird stuff), now U-Boot proper pre-reloc will not output anything as well. Adding Lukasz in Cc who's trying to fix U-Boot so that this is possible on PX30 (therefore, not just a theoretical use case I'm bringing up ;) ). > - Radxa ROCK Pi S (RK3308): ~80 ms > - Radxa ZERO 3W (RK3566): ~120 ms > - Radxa ROCK 5B (RK3588): ~150 ms > > Similar change has already been applied for RK3328 and RK3399 boards. > Then I believe this was a mistake. > The change for PX30 has not been runtime tested, and only include change > for uart2, not uart0 and uart5 used on two of the supported px30 boards. > This means we have uart2 pinctrl at all stages even though we make no use of it? I'm starting to wonder if this optimization isn't orthogonal to having as much support as possible via an <soc>-u-boot.dtsi which was done to make bring-up and maintenance easier. If you're really after optimization AND we keep the pinctrl for the debug uart (thus, not breaking 2) above), then what we want is cleaning up the default px30-u-boot.dtsi and move bootph properties to the board DTS instead? I was a bit against this "turn-key" solution wrt u-boot DT for rk3568/rk3588 but we went ahead, are we changing our mind now? Just to know what exactly is the direction we are planning to take. Cheers, Quentin
Hi Quentin, On 2024-08-05 12:23, Quentin Schulz wrote: > Hi Jonas, > > On 8/5/24 10:43 AM, Jonas Karlman wrote: >> UART pinctrl for serial is typically applied multiple times: >> - in external TPL or in TPL for DEBUG_UART in board_debug_uart_init() >> - in SPL for DEBUG_UART in board_debug_uart_init() >> - in SPL using pinctrl from DT >> - in pre-reloc phase using pinctrl from DT >> - after relocation using pinctrl from DT >> >> This series change bootph props for the UART pinctrl to not include them >> in pre-reloc phase to save some boot time: >> > > NACK. I feel like this is a hack for vendor trees only. I disagree, this just relaxes the bootph props currently enforced by <soc>-u-boot.dtsi files. For boards that has special need they can still add bootph-all or the bootph-some-ram prop to restore prior behavior. Also please do not associate me with any vendor, I am a hobbyist :-) > > 1) The Device Tree represents the hardware. We need the mux at all times. I do not think this changes the the description of the hardware in any way. The bootph props is currently already more of a representation of how software should behave than hardware. In general for Rockchip use of the bootph-some-ram prop (pre-relocation phase) does not fully make any sense, since we already have full SDRAM in SPL. If we where to fully follow the dtschema description for the bootph props, for Rockchip they should probably map to something like: TPL: - bootph-pre-sram: Enable this node when SRAM is not available. - bootph-pre-ram: Enable this node in the phase that sets up SDRAM. SPL: - bootph-some-ram: Enable this node in the phase that is run after SDRAM is working but before all of it is available. U-Boot pre-reloc: - no prop mapping, we have all SDRAM and can use all devices, ideally for Rockchip we should just do bare minimum to relocate without loading any driver and then move to main post-reloc phase. Instead they map to the different U-Boot software stages: - bootph-pre-sram: TPL - bootph-pre-ram: SPL - bootph-some-ram: U-Boot pre-reloc > 2) This breaks users who want to have no serial in TPL/SPL but in proper > (where you want it as soon as possible I assume, thus pre-reloc). Do you know of any such user/board? Adding the bootph-some-ram or bootph-all prop can easily be done in <board>-u-boot.dtsi for such user/board to restore current behavior. > 3) This likely also means if BL31 is configured to output on a different > UART mux of the same controller for some reason (I've seen weird stuff), > now U-Boot proper pre-reloc will not output anything as well. Same as for 2), adding the bootph-some-ram or bootph-all can and should be done at board level for boards that have special, or weird, requirements. It is also possible to add board specific code that call debug_uart_init() early if you want to ensure pinmux without adding the unnecessary delay that use of pinctrl in pre-reloc brings. > > Adding Lukasz in Cc who's trying to fix U-Boot so that this is possible > on PX30 (therefore, not just a theoretical use case I'm bringing up ;) ). > >> - Radxa ROCK Pi S (RK3308): ~80 ms >> - Radxa ZERO 3W (RK3566): ~120 ms >> - Radxa ROCK 5B (RK3588): ~150 ms >> >> Similar change has already been applied for RK3328 and RK3399 boards. >> > > Then I believe this was a mistake. My wording may have been misleading, when we added broad use of pinctrl props to SPL for rk3328/rk3399 we limited so that the uart pinctrl group was only enabled for TPL/SPL and not pre-reloc phase, because also including it at pre-reloc added 200-300 ms in extra boot time. This series tries to apply same findings on remaining Rockchip aarch64 boards. > >> The change for PX30 has not been runtime tested, and only include change >> for uart2, not uart0 and uart5 used on two of the supported px30 boards. >> > > This means we have uart2 pinctrl at all stages even though we make no > use of it? Yes, in my opinion the px30-u-boot.dtsi include lots of nodes that seem very board specific, some that probably should move to board specific u-boot.dtsi files. > > I'm starting to wonder if this optimization isn't orthogonal to having > as much support as possible via an <soc>-u-boot.dtsi which was done to > make bring-up and maintenance easier. If you're really after > optimization AND we keep the pinctrl for the debug uart (thus, not > breaking 2) above), then what we want is cleaning up the default > px30-u-boot.dtsi and move bootph properties to the board DTS instead? Agree, for other aarch64 socs I have already tried to cleanup <soc>-u-boot.dtsi to only contain what more than 70-80% of all supported boards may need. For rk3399 bob and kevin even use /delete-property/ to remove some of the common boothp props because all other rk3399 boards benefited from those common boothp props. I have no px30 board and the only rk3326 board I do have does not include the px30-u-boot.dtsi, so I will not try to do any cleanup of that file. > > I was a bit against this "turn-key" solution wrt u-boot DT for > rk3568/rk3588 but we went ahead, are we changing our mind now? Just to > know what exactly is the direction we are planning to take. Please elaborate, I do not understand what you mean by this. The noticeable effect of boot time was discovered when reworking rk3399-u-boot.dtsi, after rk3568/rk3588 u-boot.dtsi already had gained a bootph-all prop in its uart2m0_xfer node. This try to apply those findings on a few more SoCs. Regards, Jonas > > Cheers, > Quentin
Hi Jonas, On 8/5/24 2:22 PM, Jonas Karlman wrote: > Hi Quentin, > > On 2024-08-05 12:23, Quentin Schulz wrote: >> Hi Jonas, >> >> On 8/5/24 10:43 AM, Jonas Karlman wrote: >>> UART pinctrl for serial is typically applied multiple times: >>> - in external TPL or in TPL for DEBUG_UART in board_debug_uart_init() >>> - in SPL for DEBUG_UART in board_debug_uart_init() >>> - in SPL using pinctrl from DT >>> - in pre-reloc phase using pinctrl from DT >>> - after relocation using pinctrl from DT >>> >>> This series change bootph props for the UART pinctrl to not include them >>> in pre-reloc phase to save some boot time: >>> >> >> NACK. I feel like this is a hack for vendor trees only. > > I disagree, this just relaxes the bootph props currently enforced by > <soc>-u-boot.dtsi files. For boards that has special need they can > still add bootph-all or the bootph-some-ram prop to restore prior > behavior. > > Also please do not associate me with any vendor, I am a hobbyist :-) > Non-upstream = vendor for me :) >> >> 1) The Device Tree represents the hardware. We need the mux at all times. > > I do not think this changes the the description of the hardware in any > way. The bootph props is currently already more of a representation of > how software should behave than hardware. > > In general for Rockchip use of the bootph-some-ram prop (pre-relocation > phase) does not fully make any sense, since we already have full SDRAM > in SPL. > > If we where to fully follow the dtschema description for the bootph > props, for Rockchip they should probably map to something like: > > TPL: > - bootph-pre-sram: Enable this node when SRAM is not available. > - bootph-pre-ram: Enable this node in the phase that sets up SDRAM. > > SPL: > - bootph-some-ram: Enable this node in the phase that is run after SDRAM > is working but before all of it is available. > > U-Boot pre-reloc: > - no prop mapping, we have all SDRAM and can use all devices, ideally > for Rockchip we should just do bare minimum to relocate without > loading any driver and then move to main post-reloc phase. > > Instead they map to the different U-Boot software stages: > - bootph-pre-sram: TPL > - bootph-pre-ram: SPL > - bootph-some-ram: U-Boot pre-reloc > >> 2) This breaks users who want to have no serial in TPL/SPL but in proper >> (where you want it as soon as possible I assume, thus pre-reloc). > > Do you know of any such user/board? Adding the bootph-some-ram or > bootph-all prop can easily be done in <board>-u-boot.dtsi for such > user/board to restore current behavior. > Yes, Lukasz in Cc has a device that is doing that, he'll be fixing a few things related to that soon (tm). I would suggest to have one commit that basically moves bootph-all to all boards which currently include the -u-boot.dtsi where bootph-all is, and add bootph-pre-sram+pre-ram to -u-boot.dtsi. 0-sum change and then on a per-board basis remove this bootph-all? Basically, I'm not sure this is a sane default to have. For non-upstreamed boards, they can figure this out whenever they contribute. For upstreamed boards, I would aim at not possibly breaking stuff in the name of optimization? The issue I believe is that you wouldn't be able to tell if there are users today since this should be a Kconfig symbol selection (SPL/TPL_SERIAL for example). We could start having #ifdefs in -u-boot.dtsi for when $(SPL_TPL_)SERIAL is enabled if we wanted too. Not sure it's a welcomed added complexity though. >> 3) This likely also means if BL31 is configured to output on a different >> UART mux of the same controller for some reason (I've seen weird stuff), >> now U-Boot proper pre-reloc will not output anything as well. > > Same as for 2), adding the bootph-some-ram or bootph-all can and should > be done at board level for boards that have special, or weird, > requirements. > It's just super weird to me to enter some corner cases because it works in most cases today and is bringing ~100ms boot time decrease. > It is also possible to add board specific code that call > debug_uart_init() early if you want to ensure pinmux without adding the > unnecessary delay that use of pinctrl in pre-reloc brings. > >> >> Adding Lukasz in Cc who's trying to fix U-Boot so that this is possible >> on PX30 (therefore, not just a theoretical use case I'm bringing up ;) ). >> >>> - Radxa ROCK Pi S (RK3308): ~80 ms >>> - Radxa ZERO 3W (RK3566): ~120 ms >>> - Radxa ROCK 5B (RK3588): ~150 ms >>> >>> Similar change has already been applied for RK3328 and RK3399 boards. >>> >> >> Then I believe this was a mistake. > > My wording may have been misleading, when we added broad use of pinctrl props > to SPL for rk3328/rk3399 we limited so that the uart pinctrl group was only > enabled for TPL/SPL and not pre-reloc phase, because also including it at > pre-reloc added 200-300 ms in extra boot time. > > This series tries to apply same findings on remaining Rockchip aarch64 > boards. > What I'm complaining about in this patch series applies as well to the patches that are already merged if the intent and result are the same. >> >>> The change for PX30 has not been runtime tested, and only include change >>> for uart2, not uart0 and uart5 used on two of the supported px30 boards. >>> >> >> This means we have uart2 pinctrl at all stages even though we make no >> use of it? > > Yes, in my opinion the px30-u-boot.dtsi include lots of nodes that seem > very board specific, some that probably should move to board specific > u-boot.dtsi files. > That I would welcome. >> >> I'm starting to wonder if this optimization isn't orthogonal to having >> as much support as possible via an <soc>-u-boot.dtsi which was done to >> make bring-up and maintenance easier. If you're really after >> optimization AND we keep the pinctrl for the debug uart (thus, not >> breaking 2) above), then what we want is cleaning up the default >> px30-u-boot.dtsi and move bootph properties to the board DTS instead? > > Agree, for other aarch64 socs I have already tried to cleanup > <soc>-u-boot.dtsi to only contain what more than 70-80% of all supported > boards may need. For rk3399 bob and kevin even use /delete-property/ to > remove some of the common boothp props because all other rk3399 boards > benefited from those common boothp props. > > I have no px30 board and the only rk3326 board I do have does not > include the px30-u-boot.dtsi, so I will not try to do any cleanup of > that file. > Understood, this falls on someone else's shoulders then :) >> >> I was a bit against this "turn-key" solution wrt u-boot DT for >> rk3568/rk3588 but we went ahead, are we changing our mind now? Just to >> know what exactly is the direction we are planning to take. > > Please elaborate, I do not understand what you mean by this. > I looked a bit and could only find: https://lore.kernel.org/u-boot/9d7063f9-c1c9-681c-480b-4c88895e6ccf@theobroma-systems.com/ This doesn't really match what I remembered of an "argument" I believe I had with Kever about putting stuff **I** didn't need for my boards in <soc>-u-boot.dtsi of which the answer was "I prefer to have something that works with minimal work than something that is optimized and require more work for newer boards". But cannot find it anymore, so maybe it was all in my head :) Cheers, Quentin
Hi Quentin, On 2024/8/12 18:44, Quentin Schulz wrote: > Hi Jonas, > > On 8/5/24 2:22 PM, Jonas Karlman wrote: >> Hi Quentin, >> >> On 2024-08-05 12:23, Quentin Schulz wrote: >>> Hi Jonas, >>> >>> On 8/5/24 10:43 AM, Jonas Karlman wrote: >>>> UART pinctrl for serial is typically applied multiple times: >>>> - in external TPL or in TPL for DEBUG_UART in board_debug_uart_init() >>>> - in SPL for DEBUG_UART in board_debug_uart_init() >>>> - in SPL using pinctrl from DT >>>> - in pre-reloc phase using pinctrl from DT >>>> - after relocation using pinctrl from DT >>>> >>>> This series change bootph props for the UART pinctrl to not include >>>> them >>>> in pre-reloc phase to save some boot time: >>>> >>> >>> NACK. I feel like this is a hack for vendor trees only. >> >> I disagree, this just relaxes the bootph props currently enforced by >> <soc>-u-boot.dtsi files. For boards that has special need they can >> still add bootph-all or the bootph-some-ram prop to restore prior >> behavior. The pinctrl setting for the same module for multi times is obviously not a good idea, and for SPL, it used to work without pinctrl which is simple and fast and enough for use. Because in most case, people use the UART and storage with is the same as the bootrom used, so the IO has been already been initialized. This change works for most of the boards and for different design can customize for its usage, them the patch is acceptable, this is different default setting instead of hack. Thanks, - Kever >> >> Also please do not associate me with any vendor, I am a hobbyist :-) >> > > Non-upstream = vendor for me :) > >>> >>> 1) The Device Tree represents the hardware. We need the mux at all >>> times. >> >> I do not think this changes the the description of the hardware in any >> way. The bootph props is currently already more of a representation of >> how software should behave than hardware. >> >> In general for Rockchip use of the bootph-some-ram prop (pre-relocation >> phase) does not fully make any sense, since we already have full SDRAM >> in SPL. >> >> If we where to fully follow the dtschema description for the bootph >> props, for Rockchip they should probably map to something like: >> >> TPL: >> - bootph-pre-sram: Enable this node when SRAM is not available. >> - bootph-pre-ram: Enable this node in the phase that sets up SDRAM. >> >> SPL: >> - bootph-some-ram: Enable this node in the phase that is run after SDRAM >> is working but before all of it is available. >> >> U-Boot pre-reloc: >> - no prop mapping, we have all SDRAM and can use all devices, ideally >> for Rockchip we should just do bare minimum to relocate without >> loading any driver and then move to main post-reloc phase. >> >> Instead they map to the different U-Boot software stages: >> - bootph-pre-sram: TPL >> - bootph-pre-ram: SPL >> - bootph-some-ram: U-Boot pre-reloc >> >>> 2) This breaks users who want to have no serial in TPL/SPL but in >>> proper >>> (where you want it as soon as possible I assume, thus pre-reloc). >> >> Do you know of any such user/board? Adding the bootph-some-ram or >> bootph-all prop can easily be done in <board>-u-boot.dtsi for such >> user/board to restore current behavior. >> > > Yes, Lukasz in Cc has a device that is doing that, he'll be fixing a > few things related to that soon (tm). > > I would suggest to have one commit that basically moves bootph-all to > all boards which currently include the -u-boot.dtsi where bootph-all > is, and add bootph-pre-sram+pre-ram to -u-boot.dtsi. 0-sum change and > then on a per-board basis remove this bootph-all? Basically, I'm not > sure this is a sane default to have. For non-upstreamed boards, they > can figure this out whenever they contribute. For upstreamed boards, I > would aim at not possibly breaking stuff in the name of optimization? > > The issue I believe is that you wouldn't be able to tell if there are > users today since this should be a Kconfig symbol selection > (SPL/TPL_SERIAL for example). > > We could start having #ifdefs in -u-boot.dtsi for when > $(SPL_TPL_)SERIAL is enabled if we wanted too. Not sure it's a > welcomed added complexity though. > >>> 3) This likely also means if BL31 is configured to output on a >>> different >>> UART mux of the same controller for some reason (I've seen weird >>> stuff), >>> now U-Boot proper pre-reloc will not output anything as well. >> >> Same as for 2), adding the bootph-some-ram or bootph-all can and should >> be done at board level for boards that have special, or weird, >> requirements. >> > > It's just super weird to me to enter some corner cases because it > works in most cases today and is bringing ~100ms boot time decrease. > >> It is also possible to add board specific code that call >> debug_uart_init() early if you want to ensure pinmux without adding the >> unnecessary delay that use of pinctrl in pre-reloc brings. >> >>> >>> Adding Lukasz in Cc who's trying to fix U-Boot so that this is possible >>> on PX30 (therefore, not just a theoretical use case I'm bringing up >>> ;) ). >>> >>>> - Radxa ROCK Pi S (RK3308): ~80 ms >>>> - Radxa ZERO 3W (RK3566): ~120 ms >>>> - Radxa ROCK 5B (RK3588): ~150 ms >>>> >>>> Similar change has already been applied for RK3328 and RK3399 boards. >>>> >>> >>> Then I believe this was a mistake. >> >> My wording may have been misleading, when we added broad use of >> pinctrl props >> to SPL for rk3328/rk3399 we limited so that the uart pinctrl group >> was only >> enabled for TPL/SPL and not pre-reloc phase, because also including >> it at >> pre-reloc added 200-300 ms in extra boot time. >> >> This series tries to apply same findings on remaining Rockchip aarch64 >> boards. >> > > What I'm complaining about in this patch series applies as well to the > patches that are already merged if the intent and result are the same. > >>> >>>> The change for PX30 has not been runtime tested, and only include >>>> change >>>> for uart2, not uart0 and uart5 used on two of the supported px30 >>>> boards. >>>> >>> >>> This means we have uart2 pinctrl at all stages even though we make no >>> use of it? >> >> Yes, in my opinion the px30-u-boot.dtsi include lots of nodes that seem >> very board specific, some that probably should move to board specific >> u-boot.dtsi files. >> > > That I would welcome. > >>> >>> I'm starting to wonder if this optimization isn't orthogonal to having >>> as much support as possible via an <soc>-u-boot.dtsi which was done to >>> make bring-up and maintenance easier. If you're really after >>> optimization AND we keep the pinctrl for the debug uart (thus, not >>> breaking 2) above), then what we want is cleaning up the default >>> px30-u-boot.dtsi and move bootph properties to the board DTS instead? >> >> Agree, for other aarch64 socs I have already tried to cleanup >> <soc>-u-boot.dtsi to only contain what more than 70-80% of all supported >> boards may need. For rk3399 bob and kevin even use /delete-property/ to >> remove some of the common boothp props because all other rk3399 boards >> benefited from those common boothp props. >> >> I have no px30 board and the only rk3326 board I do have does not >> include the px30-u-boot.dtsi, so I will not try to do any cleanup of >> that file. >> > > Understood, this falls on someone else's shoulders then :) > >>> >>> I was a bit against this "turn-key" solution wrt u-boot DT for >>> rk3568/rk3588 but we went ahead, are we changing our mind now? Just to >>> know what exactly is the direction we are planning to take. >> >> Please elaborate, I do not understand what you mean by this. >> > > I looked a bit and could only find: > https://lore.kernel.org/u-boot/9d7063f9-c1c9-681c-480b-4c88895e6ccf@theobroma-systems.com/ > > > This doesn't really match what I remembered of an "argument" I believe > I had with Kever about putting stuff **I** didn't need for my boards > in <soc>-u-boot.dtsi of which the answer was "I prefer to have > something that works with minimal work than something that is > optimized and require more work for newer boards". But cannot find it > anymore, so maybe it was all in my head :) > > Cheers, > Quentin
Hi Kever, On 9/30/24 12:11 PM, Kever Yang wrote: > Hi Quentin, > > On 2024/8/12 18:44, Quentin Schulz wrote: >> Hi Jonas, >> >> On 8/5/24 2:22 PM, Jonas Karlman wrote: >>> Hi Quentin, >>> >>> On 2024-08-05 12:23, Quentin Schulz wrote: >>>> Hi Jonas, >>>> >>>> On 8/5/24 10:43 AM, Jonas Karlman wrote: >>>>> UART pinctrl for serial is typically applied multiple times: >>>>> - in external TPL or in TPL for DEBUG_UART in board_debug_uart_init() >>>>> - in SPL for DEBUG_UART in board_debug_uart_init() >>>>> - in SPL using pinctrl from DT >>>>> - in pre-reloc phase using pinctrl from DT >>>>> - after relocation using pinctrl from DT >>>>> >>>>> This series change bootph props for the UART pinctrl to not include >>>>> them >>>>> in pre-reloc phase to save some boot time: >>>>> >>>> >>>> NACK. I feel like this is a hack for vendor trees only. >>> >>> I disagree, this just relaxes the bootph props currently enforced by >>> <soc>-u-boot.dtsi files. For boards that has special need they can >>> still add bootph-all or the bootph-some-ram prop to restore prior >>> behavior. > > The pinctrl setting for the same module for multi times is obviously not > a good idea, > > and for SPL, it used to work without pinctrl which is simple and fast > and enough for use. > > Because in most case, people use the UART and storage with is the same > as the bootrom > > used, so the IO has been already been initialized. > Is the bootrom really initializing the UART? Are you sure you're not mixing bootrom and Rockchip's TPL blob? The latter, I know, the former, I'm surprised since it doesn't print anything as far as I am aware? > This change works for most of the boards and for different design can > customize for its > > usage, them the patch is acceptable, this is different default setting > instead of hack. > I disagree, this is working around the DT specs to make things faster by breaking some other usecases. I believe we shouldn't have to change the device tree when we change something in the defconfig for the board to still work. I would be fine with a bunch of ifdefs in -u-boot.dtsi to have the appropriate bootph- properties guarded by the appropriate CONFIG_SPL_SERIAL/CONFIG_TPL_SERIAL, e.g. something like (not tested) """ // Optimize boot time by avoiding to reconfigure pinctrl already configured by earlier stages for console #if IS_ENABLED(TPL_SERIAL) bootph-pre-sram; #elif IS_ENABLED(SPL_SERIAL) bootph-pre-ram; #else bootph-all; #endif """ I also don't know what we should be doing with ROCKCHIP_EXTERNAL_TPL as we should probably have bootph-pre-ram/bootph-all set if that option is set, in case one uses the default blob from Rockchip but a different UART controller or mux so that they have console starting from upstream U-Boot at least. What do you think? Cheers, Quentin