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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi,

Volodymyr Babchuk <volodymyr_babchuk@epam.com> writes:

> Caleb Connolly <caleb.connolly@linaro.org> writes:
>
>> On 11/03/2024 18:23, Volodymyr Babchuk wrote:
>>> Hi Caleb,
>>> Caleb Connolly <caleb.connolly@linaro.org> writes:
>>> 
>>>> On 06/03/2024 21:24, Volodymyr Babchuk wrote:
>>>>>
>>>>> Hi Caleb,
>>>>>
>>>>> Caleb Connolly <caleb.connolly@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.

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.

Right now I have a working Xen that is able to boot Dom0 straight to
console, if anyone interested.

-- 
WBR, Volodymyr