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

[Volodymyr Babchuk]

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.

>>>> +
>>>> +       /* 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.

(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.
>> 
>>>> +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.
>
> 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.

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

[...]

-- 
WBR, Volodymyr