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

[Caleb Connolly]

On 19/03/2024 21:18, Volodymyr Babchuk wrote:
> 
> Hi,
> 
> Volodymyr Babchuk <volodymyr_babchuk at epam.com> writes:
> 
>> Caleb Connolly <caleb.connolly at linaro.org> writes:
>>
>>> On 11/03/2024 18:23, Volodymyr Babchuk wrote:
>>>> Hi Caleb,
>>>> Caleb Connolly <caleb.connolly at linaro.org> writes:
>>>>
>>>>> 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.
>>>> Of course.
>>>> As I understood, you are interested in "clk: qcom: clear div mask
>>>> before
>>>> assigning a new divider" and "clk: qcom: add support for power domains
>>>> uclass", correct?
>>>
>>> Yes.
>>
>> Okay, I'll send it today.
>>
>>> I tried the power domain stuff out on SMD845 today and ran into
>>> quite a few issues. Specifically as a lot of the devices reference the
>>> rpmhpd power domain which we don't support (and don't *need* to
>>> support) in U-Boot. I'm not sure what the best way forward will be for
>>> this. Maybe a "nop" power domain driver?
>>
>> Are you sure that they are not required?
>>
>> "nop" power domain always is the option. Especially if it prints some
>> warning about an unknown device. I had quite a lot of issues with clock and
>> pin drivers that silently ignore unknown devices...
>>
>>> Do you have the same issues on sm8150?
>>
>> Yes and no. No, because I was lucky so far and devices I tried to use in
>> U-Boot does not require rpmhpd. Looking at DTS, I may only encounter
>> issues with sdhc_2, which requires rpmhpd for some reason. Also UFS
>> requires clock from rpmhcc.
>>
>> And "yes", because I have found root cause for my troubles with UFS in
>> Linux kernel, when I am skipping hyp.mbn. This is not strictly related
>> to U-Boot, but you may be interested in this: apparently Qualcomm's
>> hypervisor enables access to RPM (maybe brings it out of reset?). cmd-db
>> shared memory region can't be accessed if I skip the hypervisor and try
>> to boot directly into Linux. So now I am looking for ways to enable it.
> 
> I want to share the solution, in case if someone got the same
> problem. It all boiled down to correct memory attributes. Qualcomm's
> hypervisor mapped cmd-db shared memory as non-cached in Stage 2
> translation table, while Linux maps it as cacheable in Stage 1. Thus,
> the final memory attribute was "non-cacheable".
> 
> Xen, on other hand, used default mapping which is normal cacheable
> memory. And of course this lead to cacheable accesses to cmd-db. For
> some reason this caused the hardware error, which manifested as a secure
> interrupt (while I expected SError at most), which in turn led to
> endless loop somewhere in TZ.

ohh, nice find! I guess if Linux started sending bogus data to the rpmh
this could definitely lead to some XPU violation or another excuse for a
secure interrupt...
> 
> I am going to fix this by applying the correct mappings in the Linux
> cmd-db driver. Plan B is to have workaround in Xen, but I really want to
> avoid this.

I agree this is definitely a Linux bug, it should make sense to fix it
there.
> 
> Right now I have a working Xen that is able to boot Dom0 straight to
> console, if anyone interested.

Awesome! Yeah I'd love to take a look :D
> 

-- 
// Caleb (they/them)