[PATCH v2 8/8] board: add support for Qualcomm SA8155P-ADP board

Thu Mar 7 08:56:43 CET 2024

On Thu, 7 Mar 2024 at 02:54, Volodymyr Babchuk
<Volodymyr_Babchuk at epam.com> wrote:
>
>
> Hi Caleb,
>
> Caleb Connolly <caleb.connolly at linaro.org> writes:
>
> [...]
> >>>> +};
> >>>> +
> >>>> +&tlmm {
> >>>> +       /* U-Boot pinctrl driver does not understand multiple tiles */
> >>>> +       reg = <0x0 0x03000000 0x0 0x1000000>;
> >>>> +       /delete-property/ reg-names;
> >>>
> >>> This won't be needed if we can make the tiles offset in the pinctrl
> >>> driver compatible:
> >>>
> >>> #define WEST   0x00000000
> >>> #define EAST   0x00400000
> >>> #define NORTH  0x00800000
> >>> #define SOUTH  0x00C00000
> >>
> >> Hmm, I assume that in this case pinctrl driver should map all the four
> >> tiles independently? Are there guarantees in U-Boot that four separate
> >> memory regions will be mapped into virtual memory with the same relative
> >> positions? Linux clearly don't make such guarantees.
> >
> > U-Boot doesn't use virtual addresses on arm platforms, it only goes as
> > far as reading the address from DT, nothing else, so this is totally
> > fine and is how the other SoCs do it.
>
> For me it looks like we are depending on implementation details
> knowledge. I.e MMU API does not provide such guarantees, but drivers
> know how ARM MMU code is working internally and drivers depend on
> exactly this behavior. But if you are saying that it is totally fine,
> I'll rework the patch. No big deal. Actually, I already tried this and
> it is working fine.

I agree doing it properly via mapping pinctrl tiles individually is
the appropriate approach. BTW, the current implementation just relies
on 1:1 VA to PA mapping in U-Boot, it just avoids deviation from
upstream DTS. If you are willing to change msm_pinctrl_probe() to
parse tiles properly then I would be happy to review that.

>
> >>>> +
> >>>> +       /* U-Boot ethernet driver wants to drive reset as GPIO */
> >>>> +       /delete-node/ phy-reset-pins;
> >>>
> >>> I suppose this is not needed as phy-reset-pins also configures the pin
> >>> as GPIO only.
> >>>
> >> Well, yes. This also puzzles me up, but for some reason it stops working
> >> if I leave this node intact. Looks like I need to look at this deeper
> >> before posting the next version.
> >
> > Possibly the pinconf defined in the phy-reset-pins node causes U-Boot to
> > misbehave, can you check if this patch fixes it (there is a bug in the
> > line "return msm_gpio_direction_input(dev, gpio);", it should become
> > just "msm_gpio_direction_input(dev, gpio);").
> >
> > I had the exact same issue with the gpio-regulator driver and this was
> > the solution I ended up going with.
> >
> > https://urldefense.com/v3/__https://lore.kernel.org/u-boot/20240131-b4-qcom-livetree-v1-7-4071c0787db0@linaro.org/__;!!GF_29dbcQIUBPA!xFhZe7DKgRbr63sirEJLuH-B0AnGs7jvx8tdJPKLTgFuZ3I3_zpVml7l23G-_vJO_JiUR-wUO4GMPJFcE-8p50H3pf7nbxit$
> > [lore[.]kernel[.]org]
>
> It is exactly this. With your patch I don't need to /delete-node/
> anymore. I'll add a comment in the cover message that this series are
> depended on your patch.
>

+1

> (and sorry for the mangled link. It is our corporate mail server doing)
>
>
> >>
> >>>> +};
> >>>> diff --git a/board/qualcomm/sa8155p-adp/MAINTAINERS b/board/qualcomm/sa8155p-adp/MAINTAINERS
> >>>> new file mode 100644
> >>>> index 0000000000..03fac84f51
> >>>> --- /dev/null
> >>>> +++ b/board/qualcomm/sa8155p-adp/MAINTAINERS
> >>>> @@ -0,0 +1,5 @@
> >>>> +Qualcomm SA8155P Automotive Development Platform
> >>>> +M:     Volodymyr Babchuk <volodymyr_babchuk at epam.com>
> >>>> +S:     Maintained
> >>>> +F:     board/qualcomm/sa8155p-adp/
> >>>> +F:     configs/sa8155p-adp_defconfig
> >>>> diff --git a/configs/sa8155p_adp_defconfig b/configs/sa8155p_adp_defconfig
> >>>> new file mode 100644
> >>>> index 0000000000..b6969767f8
> >>>> --- /dev/null
> >>>> +++ b/configs/sa8155p_adp_defconfig
> >>>> @@ -0,0 +1,35 @@
> >>>> +CONFIG_ARM=y
> >>>> +CONFIG_SKIP_LOWLEVEL_INIT=y
> >>>> +CONFIG_COUNTER_FREQUENCY=19000000
> >>>> +CONFIG_POSITION_INDEPENDENT=y
> >>>> +CONFIG_ENABLE_ARM_SOC_BOOT0_HOOK=y
> >>>> +CONFIG_ARCH_SNAPDRAGON=y
> >>>> +CONFIG_TEXT_BASE=0x85710000
> >>>
> >>> Being position independent shouldn't require a hardcoded U-Boot text
> >>> base. Can you try if we can get rid of this?
> >>>
> >>
> >> Well, it is required if we want to load U-Boot instead of hyp.mbn. We
> >> need correct addresses in the ELF file so Qualcomm loader will not
> >> reject it right away.
> >>

I see the hard dependency due to prior stage Qcom bootloaders.

> >>>> +CONFIG_DEFAULT_DEVICE_TREE="qcom/sa8155p-adp"
> >>>> +CONFIG_IDENT_STRING="\nQualcomm SA8155P-ADP"
> >>>> +CONFIG_SYS_LOAD_ADDR=0x85710000
> >>>
> >>> Ditto.
> >>>
> >>>> +CONFIG_REMAKE_ELF=y
> >>>> +CONFIG_BOOTDELAY=3
> >>>> +CONFIG_SYS_CBSIZE=512
> >>>> +# CONFIG_DISPLAY_CPUINFO is not set
> >>>> +CONFIG_HUSH_PARSER=y
> >>>> +CONFIG_OF_UPSTREAM=y
> >>>> +CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> >>>> +CONFIG_NET_RANDOM_ETHADDR=y
> >>>> +CONFIG_CLK=y
> >>>> +CONFIG_CLK_QCOM_SM8150=y
> >>>> +CONFIG_MSM_GPIO=y
> >>>> +CONFIG_PHY_MICREL=y
> >>>> +CONFIG_PHY_MICREL_KSZ90X1=y
> >>>> +CONFIG_DM_MDIO=y
> >>>> +CONFIG_DM_ETH_PHY=y
> >>>> +CONFIG_DWC_ETH_QOS=y
> >>>> +CONFIG_DWC_ETH_QOS_QCOM=y
> >>>> +CONFIG_PHY=y
> >>>> +CONFIG_PINCTRL=y
> >>>> +CONFIG_PINCONF=y
> >>>> +CONFIG_PINCTRL_QCOM_SM8150=y
> >>>> +CONFIG_POWER_DOMAIN=y
> >>>> +CONFIG_MSM_GENI_SERIAL=y
> >>>> +CONFIG_SPMI_MSM=y
> >>>> +CONFIG_LMB_MAX_REGIONS=64
> >>>
> >>> Apart from above, I think this platform should be able to reuse
> >>> qcom_defconfig as you can find most of the config options there. Can
> >>> you try to reuse it?
> >>
> >> Honestly, the whole reason I am porting U-Boot to this platform is
> >> because I want to run Xen on it. And to run Xen, I need to run U-Boot in
> >> EL2. And to do this I need u-boot.elf with "correct" load address and
> >> entry point.
> >>
> >> I am planning to publish and upstream Xen patches as well (once I finish
> >> them). And it will be really nice if Xen users will be able use
> >> mainline U-Boot to boot Xen.

That's fair and I am happy for it to be a separate defconfig.

> >
> > I would like to enable the SM8150 drivers in qcom_defconfig (for
> > chainloading and supporting other platforms). But I'm totally fine with
> > having a separate defconfig for this board with this configuration.

Probably the common defconfig is up for renaming:
s/qcom_defconfig/qcom_chainload_defconfig/ since that seems to be the
only configuration we can support with a common defconfig.

>
> Yes, this is a good approach. I'll do this.
>

So you can probably drop CONFIG_POSITION_INDEPENDENT=y from this
defconfig and rather use the common defconfig for chainloaded
configuration.

-Sumit

> [...]

>
> --
> WBR, Volodymyr