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

[Caleb Connolly]

On 06/03/2024 21:24, Volodymyr Babchuk 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.
> 
>>>>> +
>>>>> +       /* 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.

Please can you split the power domain and clock patches into a separate
series? As I'd like to depend on them for the next revision of my
series, and we'd otherwise have a cyclical dependency.
> 
> (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.
> 
> [...]
> 

-- 
// Caleb (they/them)