[PATCH 2/3] arm64: dts: ti: Introduce J722S family of SoCs
Andrew Davis
afd at ti.com
Thu Dec 14 07:43:43 PST 2023
On 12/14/23 6:27 AM, Nishanth Menon wrote:
> On 14:07-20231214, Vaishnav Achath wrote:
> [..]
>>> Trim this down to what is different from AM62P?
>>>
>>
>> Thanks for the review, I will trim this down in next revision, but the above is
>> just a summary of the main features of this SoC, pointing to AM62P feature set
>> here seems confusing to me. why does a new user/developer using J722S need to be
>> aware of the existence of AM62P to just understand a high level summary about
>> this device?
>
> Since this is a reuse device. Helps with review and focus on deltas.
>
> [...]
>
>>>> + l2_0: l2-cache0 {
>>>> + compatible = "cache";
>>>> + cache-unified;
>>>> + cache-level = <2>;
>>>> + cache-size = <0x80000>;
>>>> + cache-line-size = <64>;
>>>> + cache-sets = <512>;
>>>> + };
>>>
>>> ^^ this is a duplication of am62p5.dtsi? what about the spins with
>>> different CPUs enabled?
>>>
>>
>> Yes it is a duplicate, as of now we are not aware of plan for spins with cores
>> disabled, so just followed the pattern followed for other Jacinto devices
>> (J721e, J7200, J721s2, J784s4).
>
> None of the devices have been as close a reuse device as this has been.
I'd argue J721e and J7200 are more similar in terms of reuse. It was a
mistake to model them as simple super/subsets of each other, only causes
confusion later. Let's keep at least this top level file, we will end up
using it more as more features/deltas are enabled/found.
> errata in one carries over to the other etc.. There are definitely some
> differences - for example: itap/otap delays for mmc those would be
> unique on this device, but overrides with documentation will make
> perfect sense there.
>
> [..]
>>>> + cbass_wakeup: bus at b00000 {
>>>> + compatible = "simple-bus";
>>>> + #address-cells = <2>;
>>>> + #size-cells = <2>;
>>>> + ranges = <0x00 0x00b00000 0x00 0x00b00000 0x00 0x00002400>, /* VTM */
>>>> + <0x00 0x2b000000 0x00 0x2b000000 0x00 0x00300400>, /* Peripheral Window */
>>>> + <0x00 0x43000000 0x00 0x43000000 0x00 0x00020000>, /* WKUP CTRL MMR */
>>>> + <0x00 0x78000000 0x00 0x78000000 0x00 0x00008000>, /* DM R5 ATCM*/
>>>> + <0x00 0x78100000 0x00 0x78100000 0x00 0x00008000>; /* DM R5 BTCM*/
>>>> + bootph-all;
>>>
>>> Not in the bus nodes.. only in the leaf nodes please.
>>>
>>>
>>> Also what is wrong with expanding the am62p to include the
>>> missing bus segments instead of duplicating all of them?
>>>
>>
>> We can do that, but the same would be true for AM625, AM62A and AM62P families
>> and also for (J721E, J7200), since all these SoCs introduced new dtsi instead of
>> resuing existing ones, I thought there would be some valid reason to have
>> separate dtsi and just followed the same pattern, please let know if we need to
>> reuse from AM62P here also.
>
> See above.
>
> [...]
>
>>
>>>> + };
>>>> + };
>>>> +
>>>> + #include "k3-am62p-thermal.dtsi"
>>>
>>> Is this correct?
>>>
>>
>> I think it would be preferred to have a duplicate for J722S here as users may
>> want to customize the trip points as per system and doing that without affecting
>> AM62P would be preferred, but the initial information here would be same for J722S.
>
> Please check with your hardware teams if the am62p SoC level thermal
> behavior will be similar to j722s or will it be different. If it is
> estimated to be different, then you'd prefer to hold off introducing
> the trips till the characterization provides you with the data.
>
>>
>>>> +};
>>>> +
>>>> +/*
>>>> + * Include peripherals for each bus segment derived
>>>> + * from AM62P and overrides specific to J722S.
>>>> + */
>>>> +#include "k3-am62p-main.dtsi"
>>>> +#include "k3-am62p-mcu.dtsi"
>>>> +#include "k3-am62p-wakeup.dtsi"
>>>> +
>>>> +/* Main domain overrides */
>>>> +
>>>> +&cpsw3g {
>>>> + status = "disabled";
>>>
>>> here and rest: Why disabled?
>>
>> These nodes seems to have dependencies that are not merged already and would
>> throw errors during boot, AM62P SoC dtsi seems to have these enabled by default
>> without these dependencies met and have errors like below during boot (seen on
>> AM62P with 6.7.0-rc4-next-20231211).
>>
>> [ 17.869092] platform 8000000.ethernet: deferred probe pending:
>> am65-cpsw-nuss: Failed to request tx dma channel
>
> OK - lets get the dependencies sorted out before merging then. I though
> all the dependencies were indicated in the cover letter.
>
>>
>>>> +};
>>>> +
>>>> +&inta_main_dmss {
>>>> + ti,interrupt-ranges = <5 69 35>;
>>>> +};
>>>> +
>>>> +&mailbox0_cluster0 {
>>>> + status = "disabled";
>>>> +};
>>>> +
>>>> +&mailbox0_cluster1 {
>>>> + status = "disabled";
>>>> +};
>>>> +
>>>> +&mailbox0_cluster2 {
>>>> + status = "disabled";
>>>> +};
>>>> +
>>>> +&mailbox0_cluster3 {
>>>> + status = "disabled";
>>>> +};
>>>> +
>>
>> [ 7.198470] omap-mailbox 29020000.mailbox: no available mbox devices found
>> [ 7.210411] omap-mailbox 29030000.mailbox: no available mbox devices found
>
> You might want to dig into why..
>
>>
>>>> +&oc_sram {
>>>> + reg = <0x00 0x70000000 0x00 0x40000>;
>>>> + ranges = <0x00 0x00 0x70000000 0x40000>;
>>>> +};
>>>> +
>>>> +/* MCU domain overrides */
>>>> +
>>>> +&mcu_r5fss0 {
>>>> + status = "disabled";
>>>> +};
>>
>> [ 7.492406] platform 79000000.r5f: configured R5F for remoteproc mode
>> [ 7.499887] platform 79000000.r5f: device does not have reserved memory
>> regions, ret = -22
>> [ 7.508271] k3_r5_rproc bus at f0000:bus at 4000000:r5fss at 79000000: reserved memory
>> init failed, ret = -22
>> [ 7.517549] remoteproc remoteproc0: releasing 79000000.r5f
>> [ 7.523338] k3_r5_rproc bus at f0000:bus at 4000000:r5fss at 79000000:
>> k3_r5_cluster_rproc_init failed, ret = -22
>> [ 7.532993] k3_r5_rproc: probe of bus at f0000:bus at 4000000:r5fss at 79000000 failed
>> with error -22
>
> Yes, and the approach should rather be to disable the remote procs in
> the board or at the SoC dtsi in a consistent manner. I had previously
> suggested to do that SoC level (which means at am62p dtsi) since the remoteprocs have direct
> dependency on how the memory layouts are partitioned in board.dts - but
> i had asked folks working on remote procs to do that consistently across
> SoCs. I don't see that having been done so far.
>
I fixed this for a couple SoCs way back last year (7e48b665100ee), seems
folks kept adding mailboxes/rprocs un-disabled in the base .dtbi for
new SoCs anyway :( This needs fixed in AM62p .dtsi first, then these
disables can be removed from here.
Andrew
>>
>>>> +
>>>> +/* wakeup domain overrides */
>>>> +
>>>> +&wkup_r5fss0 {
>>>> + status = "disabled";
>>>> +};
>>
>> [ 7.576576] platform 78000000.r5f: configured R5F for IPC-only mode
>> [ 7.605535] platform 78000000.r5f: device does not have reserved memory
>> regions, ret = -22
>> [ 7.613942] k3_r5_rproc bus at f0000:bus at b00000:r5fss at 78000000: reserved memory
>> init failed, ret = -22
>> [ 7.635990] remoteproc remoteproc0: releasing 78000000.r5f
>> [ 7.648639] k3_r5_rproc bus at f0000:bus at b00000:r5fss at 78000000:
>> k3_r5_cluster_rproc_init failed, ret = -22
>> [ 7.664043] k3_r5_rproc: probe of bus at f0000:bus at b00000:r5fss at 78000000 failed
>> with error -22
>
> See above.
>
>>
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-pinctrl.h b/arch/arm64/boot/dts/ti/k3-pinctrl.h
>>>> index 2a4e0e084d69..591be4489f37 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-pinctrl.h
>>>> +++ b/arch/arm64/boot/dts/ti/k3-pinctrl.h
>>>> @@ -59,6 +59,9 @@
>>>> #define J721S2_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
>>>> #define J721S2_WKUP_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
>>>>
>>>> +#define J722S_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
>>>> +#define J722S_MCU_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
>>>> +
>>>> #define J784S4_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
>>>> #define J784S4_WKUP_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
>>>>
>>>> --
>>>> 2.17.1
>>>>
>>
>> 0 - AM62P Boot logs (next-20231211) -
>> https://gist.github.com/vaishnavachath/7143da253bc708a1e60a13fe081d3914
>>
>> To avoid these errors I am keeping these disabled, can we add support for these
>> also as part of the initial support addition series? then there is no need for
>> disabling these.
>
> Fix them, then introduce j722s.
>
More information about the linux-arm-kernel
mailing list