Page MenuHomeFreeBSD

Correct ranges in Armada38x dts
ClosedPublic

Authored by bsz_semihalf.com on Nov 19 2015, 12:20 PM.
Tags
Referenced Files
Unknown Object (File)
Sun, Mar 3, 12:07 AM
Unknown Object (File)
Sat, Mar 2, 11:05 PM
Unknown Object (File)
Feb 24 2024, 4:38 AM
Unknown Object (File)
Feb 5 2024, 7:58 PM
Unknown Object (File)
Dec 22 2023, 9:21 PM
Unknown Object (File)
Nov 13 2023, 1:37 PM
Unknown Object (File)
Nov 10 2023, 1:59 AM
Unknown Object (File)
Oct 28 2023, 6:54 PM

Details

Summary

Subject: [PATCH 03/34]

Ranges property of 'soc' node used two-cell addresses which resulted in
casting errors as simplebus resource allocation works with 32-bit u_long
variables. FDT ranges were simplified.

Obtained from: Semihalf
Sponsored by: Stormshield
Submitted by: Michal Stanek <mst@semihalf.com>

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bsz_semihalf.com retitled this revision from to Correct 'soc' nodes in armada38x dts files.
bsz_semihalf.com updated this object.
bsz_semihalf.com edited the test plan for this revision. (Show Details)
bsz_semihalf.com added reviewers: ian, imp.
bsz_semihalf.com set the repository for this revision to rS FreeBSD src repository - subversion.
bsz_semihalf.com added a project: ARM.
bsz_semihalf.com added a subscriber: zbb.
imp edited edge metadata.

In the previous review I suggested moving these. Disregard. If we're using non-standard FDTs, we need to keep them here.

I do wonder, though why the second change. simplebus will attach to any bus that has the compatible string... Which has caused some issues for me in the past...

This revision is now accepted and ready to land.Nov 19 2015, 2:43 PM
andrew requested changes to this revision.Dec 10 2015, 9:08 AM
andrew added a reviewer: andrew.

Why is this needed? i.e. Why are you not fixing the code?

This revision now requires changes to proceed.Dec 10 2015, 9:08 AM
In D4212#94393, @andrew wrote:

Why is this needed? i.e. Why are you not fixing the code?

Actually I looked into fdt_immr_addr() and found no reason to require the compatible string to be exactly "simple-bus" so I will modify that function and leave the original compatible string in FDT.
As for the 'ranges' type mismatch I think it is best to leave the change in FDT as I see no possible simple fix in the code. It would require changing the types of arguments in bus_alloc_resource() interface to avoid truncation of addresses.

In D4212#94393, @andrew wrote:

Why is this needed? i.e. Why are you not fixing the code?

Actually I looked into fdt_immr_addr() and found no reason to require the compatible string to be exactly "simple-bus" so I will modify that function and leave the original compatible string in FDT.
As for the 'ranges' type mismatch I think it is best to leave the change in FDT as I see no possible simple fix in the code. It would require changing the types of arguments in bus_alloc_resource() interface to avoid truncation of addresses.

I have tried several times to get rid of fdt_immr_addr() and run into roadblocks, but it really needs to be eliminated (and its functionality needs to be moved into arm/mv in some better way because it's mv-specific). I don't remember what problems I ran into, it was a long time ago.

If rman's 32-bit limitation is a part of the problem, that should be getting better soon... D4544 has changes to expand rman types from u_long to uintmax_t. It will take a while for that to get tested and committed, but we should revisit this issue once that's done.

In D4212#94393, @andrew wrote:

Why is this needed? i.e. Why are you not fixing the code?

Actually I looked into fdt_immr_addr() and found no reason to require the compatible string to be exactly "simple-bus" so I will modify that function and leave the original compatible string in FDT.

Do you still need to call fdt_immr_addr? No other SoCs need this and would be easy to restrict to the older Marvell SoCs.

In D4212#95771, @andrew wrote:
In D4212#94393, @andrew wrote:

Why is this needed? i.e. Why are you not fixing the code?

Actually I looked into fdt_immr_addr() and found no reason to require the compatible string to be exactly "simple-bus" so I will modify that function and leave the original compatible string in FDT.

Do you still need to call fdt_immr_addr? No other SoCs need this and would be easy to restrict to the older Marvell SoCs.

Yes, fdt_immr_addr() initializes fdt_immr_pa and fdt_immr_va which are needed in some parts of code common for all Marvell SoCs including Armada38x. Only Marvell code uses this function, except for sys/mips/nlm/xlp_machdep.c where it seems to have been added by mistake.

In D4212#95771, @andrew wrote:

Do you still need to call fdt_immr_addr? No other SoCs need this and would be easy to restrict to the older Marvell SoCs.

Yes, fdt_immr_addr() initializes fdt_immr_pa and fdt_immr_va which are needed in some parts of code common for all Marvell SoCs including Armada38x. Only Marvell code uses this function, except for sys/mips/nlm/xlp_machdep.c where it seems to have been added by mistake.

Is this common code called on Armada 38x? The only place that I can see is in platform_devmap_init, and I think this could be replaced with a local function to find the pa & size.

bsz_semihalf.com retitled this revision from Correct 'soc' nodes in armada38x dts files to Correct ranges in Armada38x dts; alter fdt_immr_addr().
bsz_semihalf.com updated this object.
bsz_semihalf.com edited edge metadata.

fdt_immr_addr modified instead of changing compatible.

In D4212#96118, @andrew wrote:
In D4212#95771, @andrew wrote:

Do you still need to call fdt_immr_addr? No other SoCs need this and would be easy to restrict to the older Marvell SoCs.

Yes, fdt_immr_addr() initializes fdt_immr_pa and fdt_immr_va which are needed in some parts of code common for all Marvell SoCs including Armada38x. Only Marvell code uses this function, except for sys/mips/nlm/xlp_machdep.c where it seems to have been added by mistake.

Is this common code called on Armada 38x? The only place that I can see is in platform_devmap_init, and I think this could be replaced with a local function to find the pa & size.

It is also required in decode_win_pcie_setup() which gets called on A38X as well.

bsz_semihalf.com retitled this revision from Correct ranges in Armada38x dts; alter fdt_immr_addr() to Correct ranges in Armada38x dts.
bsz_semihalf.com updated this object.

I decided to seperate DTS change and correcting strict compatibility requirement, as there are more calls we are affected by.

@andrew, the commit was reduced. Necessity of remaining change was explained by Michal above. Please let me know if it's acceptable for you.

This revision was automatically updated to reflect the committed changes.