Page MenuHomeFreeBSD
Differential D5069

Add FDT bus capabilities to ThunderX PCI driver
ClosedPublic
Actions

Authored by zbb on Jan 25 2016, 3:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 7, 1:56 PM
Unknown Object (File)
Fri, Mar 14, 11:32 PM
Unknown Object (File)
Wed, Mar 12, 7:31 AM
Unknown Object (File)
Mon, Mar 10, 7:18 AM
Unknown Object (File)
Jan 27 2025, 1:41 AM
Unknown Object (File)
Jan 27 2025, 1:29 AM
Unknown Object (File)
Jan 27 2025, 1:27 AM
Unknown Object (File)
Jan 26 2025, 5:24 PM
View All Files
Subscribers
emaste
arm64

Details

Reviewers
wma
andrew
imp
• ian
Commits
rS294993: Add FDT bus capabilities to ThunderX PCI driver
Summary

New ThunderX firmware incorporates modified DTB that presents
different device hierarchy. In the new device tree, MDIO
devices are below two additional buses that oddly hang on
PCI bridge.

Diff Detail

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

Event Timeline

zbb updated this revision to Diff 12688.Jan 25 2016, 3:47 PM
zbb retitled this revision from to Add FDT bus capabilities to ThunderX PCI driver.
zbb updated this object.
zbb edited the test plan for this revision. (Show Details)
zbb added reviewers: andrew, imp, • ian, wma.
zbb set the repository for this revision to rS FreeBSD src repository - subversion.
zbb added a subscriber: arm64.
Herald added a subscriber: emaste. · View Herald TranscriptJan 25 2016, 3:47 PM
zbb added a child revision: D5070: Support new MDIO hierarchy in ThunderX DTB.Jan 25 2016, 3:49 PM
zbb added a parent revision: D5067: Divide ThunderX PCIe driver to general and FDT part.
wma edited edge metadata.Jan 26 2016, 5:26 AM

Looks good to me, but someone else should approve.

sys/arm64/cavium/thunder_pcie_common.c
48 ↗(On Diff #12688)

Change to: 'Thunder PCie driver memory' or anything specifying which memory it's all about.

zbb updated this revision to Diff 12721.Jan 26 2016, 1:07 PM
zbb edited edge metadata.
zbb marked an inline comment as done.
andrew added inline comments.Jan 26 2016, 5:52 PM
sys/arm64/cavium/thunder_pcie.c
408 ↗(On Diff #12721)

Why do you need this ifndef?

sys/arm64/cavium/thunder_pcie_fdt.c
270–271 ↗(On Diff #12721)

Be warned that you may need to change this because of D5075

279 ↗(On Diff #12721)

When does this return < 0?

zbb added inline comments.Jan 27 2016, 2:15 PM
sys/arm64/cavium/thunder_pcie.c
408 ↗(On Diff #12721)

Hmm. Won't bus_alloc_resource and bus_release_resource here interfere with the same in thunder_pcie_fdt.c?

sys/arm64/cavium/thunder_pcie_fdt.c
270–271 ↗(On Diff #12721)

OK I will keep that in mind, thanks!

279 ↗(On Diff #12721)

You are right. This will be either == 0 or > 0.
I must have mistaken it with OF_<smthing> class of functions. I will fix that in the next iteration.

zbb updated this revision to Diff 12786.Jan 28 2016, 11:43 AM
zbb marked 4 inline comments as done.
andrew added inline comments.Jan 28 2016, 12:26 PM
sys/arm64/cavium/thunder_pcie.c
408 ↗(On Diff #12786)

If you've overridden a function it should be used. We rely on this, e.g. to override the device_attach function.

sys/arm64/cavium/thunder_pcie_fdt.c
270–271 ↗(On Diff #12786)

It has now been committed, you will need to update the type.

zbb marked an inline comment as done.Jan 28 2016, 12:29 PM
zbb added inline comments.
sys/arm64/cavium/thunder_pcie_fdt.c
270–271 ↗(On Diff #12786)

It is done in the current diff. Did you check?

zbb marked an inline comment as done.Jan 28 2016, 12:29 PM
zbb added inline comments.
sys/arm64/cavium/thunder_pcie.c
408 ↗(On Diff #12786)

OK

andrew added inline comments.Jan 28 2016, 12:35 PM
sys/arm64/cavium/thunder_pcie_fdt.c
270–271 ↗(On Diff #12786)

Which diff? I don't see it here.

zbb added inline comments.Jan 28 2016, 12:37 PM
sys/arm64/cavium/thunder_pcie_fdt.c
270–271 ↗(On Diff #12786)

Agr sorry my bad. Didn't upload this one.
I will upload with the removed FDT above in a minute.

zbb updated this revision to Diff 12789.Jan 28 2016, 1:11 PM
zbb marked 6 inline comments as done.
Closed by commit rS294993: Add FDT bus capabilities to ThunderX PCI driver (authored by zbb). · Explain WhyJan 28 2016, 3:41 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
head/
sys/
arm64/
cavium/
9 lines
7 lines
3 lines
181 lines

Diff 12799

View Options

head/sys/arm64/cavium/thunder_pcie.c

Show First 20 Lines • Show All 1,096 Lines • ▼ Show 20 Lines case 0: /* item name */
break; break;
} }
remain = byte2; remain = byte2;
if (vpd_nextbyte(&vrs, &byte2)) { if (vpd_nextbyte(&vrs, &byte2)) {
state = -2; state = -2;
break; break;
} }
remain |= byte2 << 8; remain |= byte2 << 8;
if (remain > (0x7f*4 - vrs.off)) {
kibUnsubmitted
Not Done
Inline Actions

I suspect this was an attempt to handle the F bit from the vpd cap address register. It should be 0x7fff instead of 0x7f*4, then.

kib: I suspect this was an attempt to handle the F bit from the vpd cap address register. It should…
jhbUnsubmitted
Not Done
Inline Actions

Or to also avoid inaddressible locations. However, the "short" case would also need the same check since you could have a "short" tag at the end of the VPD area that could potentially overflow.

I would perhaps move this down to the bottom of this case before the 'switch (name)' (line 1115 in the old code in this review) so it tests both cases and code it something like:

if (vrs.off + remain - vrs.bytesinval > 0x8000) {
    pci_printf(cfg, "VPD data overflow, remain %#x\n", remain);
    state = -1;
    break;
}

The missing break in the old code isn't great either as you could potentially malloc() a really large value.

jhb: Or to also avoid inaddressible locations. However, the "short" case would also need the same…
state = -1;
pci_printf(cfg,
"invalid VPD data, remain %#x\n",
remain);
}
name = byte & 0x7f; name = byte & 0x7f;
} else { } else {
remain = byte & 0x7; remain = byte & 0x7;
name = (byte >> 3) & 0xf; name = (byte >> 3) & 0xf;
}
if (vrs.off + remain - vrs.bytesinval > 0x8000) {
pci_printf(cfg,
"VPD data overflow, remain %#x\n", remain);
state = -1;
break;
} }
switch (name) { switch (name) {
case 0x2: /* String */ case 0x2: /* String */
cfg->vpd.vpd_ident = malloc(remain + 1, cfg->vpd.vpd_ident = malloc(remain + 1,
M_DEVBUF, M_WAITOK); M_DEVBUF, M_WAITOK);
i = 0; i = 0;
state = 1; state = 1;
break; break;
▲ Show 20 Lines • Show All 5,502 Lines • Show Last 20 Lines
View Options

head/sys/arm64/cavium/thunder_pcie_common.h

Show First 20 Lines • Show All 1,096 Lines • ▼ Show 20 Lines case 0: /* item name */
break; break;
} }
remain = byte2; remain = byte2;
if (vpd_nextbyte(&vrs, &byte2)) { if (vpd_nextbyte(&vrs, &byte2)) {
state = -2; state = -2;
break; break;
} }
remain |= byte2 << 8; remain |= byte2 << 8;
if (remain > (0x7f*4 - vrs.off)) {
kibUnsubmitted
Not Done
Inline Actions

I suspect this was an attempt to handle the F bit from the vpd cap address register. It should be 0x7fff instead of 0x7f*4, then.

kib: I suspect this was an attempt to handle the F bit from the vpd cap address register. It should…
jhbUnsubmitted
Not Done
Inline Actions

Or to also avoid inaddressible locations. However, the "short" case would also need the same check since you could have a "short" tag at the end of the VPD area that could potentially overflow.

I would perhaps move this down to the bottom of this case before the 'switch (name)' (line 1115 in the old code in this review) so it tests both cases and code it something like:

if (vrs.off + remain - vrs.bytesinval > 0x8000) {
    pci_printf(cfg, "VPD data overflow, remain %#x\n", remain);
    state = -1;
    break;
}

The missing break in the old code isn't great either as you could potentially malloc() a really large value.

jhb: Or to also avoid inaddressible locations. However, the "short" case would also need the same…
state = -1;
pci_printf(cfg,
"invalid VPD data, remain %#x\n",
remain);
}
name = byte & 0x7f; name = byte & 0x7f;
} else { } else {
remain = byte & 0x7; remain = byte & 0x7;
name = (byte >> 3) & 0xf; name = (byte >> 3) & 0xf;
}
if (vrs.off + remain - vrs.bytesinval > 0x8000) {
pci_printf(cfg,
"VPD data overflow, remain %#x\n", remain);
state = -1;
break;
} }
switch (name) { switch (name) {
case 0x2: /* String */ case 0x2: /* String */
cfg->vpd.vpd_ident = malloc(remain + 1, cfg->vpd.vpd_ident = malloc(remain + 1,
M_DEVBUF, M_WAITOK); M_DEVBUF, M_WAITOK);
i = 0; i = 0;
state = 1; state = 1;
break; break;
▲ Show 20 Lines • Show All 5,502 Lines • Show Last 20 Lines
View Options

head/sys/arm64/cavium/thunder_pcie_common.c

Show First 20 Lines • Show All 1,096 Lines • ▼ Show 20 Lines case 0: /* item name */
break; break;
} }
remain = byte2; remain = byte2;
if (vpd_nextbyte(&vrs, &byte2)) { if (vpd_nextbyte(&vrs, &byte2)) {
state = -2; state = -2;
break; break;
} }
remain |= byte2 << 8; remain |= byte2 << 8;
if (remain > (0x7f*4 - vrs.off)) {
kibUnsubmitted
Not Done
Inline Actions

I suspect this was an attempt to handle the F bit from the vpd cap address register. It should be 0x7fff instead of 0x7f*4, then.

kib: I suspect this was an attempt to handle the F bit from the vpd cap address register. It should…
jhbUnsubmitted
Not Done
Inline Actions

Or to also avoid inaddressible locations. However, the "short" case would also need the same check since you could have a "short" tag at the end of the VPD area that could potentially overflow.

I would perhaps move this down to the bottom of this case before the 'switch (name)' (line 1115 in the old code in this review) so it tests both cases and code it something like:

if (vrs.off + remain - vrs.bytesinval > 0x8000) {
    pci_printf(cfg, "VPD data overflow, remain %#x\n", remain);
    state = -1;
    break;
}

The missing break in the old code isn't great either as you could potentially malloc() a really large value.

jhb: Or to also avoid inaddressible locations. However, the "short" case would also need the same…
state = -1;
pci_printf(cfg,
"invalid VPD data, remain %#x\n",
remain);
}
name = byte & 0x7f; name = byte & 0x7f;
} else { } else {
remain = byte & 0x7; remain = byte & 0x7;
name = (byte >> 3) & 0xf; name = (byte >> 3) & 0xf;
}
if (vrs.off + remain - vrs.bytesinval > 0x8000) {
pci_printf(cfg,
"VPD data overflow, remain %#x\n", remain);
state = -1;
break;
} }
switch (name) { switch (name) {
case 0x2: /* String */ case 0x2: /* String */
cfg->vpd.vpd_ident = malloc(remain + 1, cfg->vpd.vpd_ident = malloc(remain + 1,
M_DEVBUF, M_WAITOK); M_DEVBUF, M_WAITOK);
i = 0; i = 0;
state = 1; state = 1;
break; break;
▲ Show 20 Lines • Show All 5,502 Lines • Show Last 20 Lines
View Options

head/sys/arm64/cavium/thunder_pcie_fdt.c

Show First 20 Lines • Show All 1,096 Lines • ▼ Show 20 Lines case 0: /* item name */
break; break;
} }
remain = byte2; remain = byte2;
if (vpd_nextbyte(&vrs, &byte2)) { if (vpd_nextbyte(&vrs, &byte2)) {
state = -2; state = -2;
break; break;
} }
remain |= byte2 << 8; remain |= byte2 << 8;
if (remain > (0x7f*4 - vrs.off)) {
kibUnsubmitted
Not Done
Inline Actions

I suspect this was an attempt to handle the F bit from the vpd cap address register. It should be 0x7fff instead of 0x7f*4, then.

kib: I suspect this was an attempt to handle the F bit from the vpd cap address register. It should…
jhbUnsubmitted
Not Done
Inline Actions

Or to also avoid inaddressible locations. However, the "short" case would also need the same check since you could have a "short" tag at the end of the VPD area that could potentially overflow.

I would perhaps move this down to the bottom of this case before the 'switch (name)' (line 1115 in the old code in this review) so it tests both cases and code it something like:

if (vrs.off + remain - vrs.bytesinval > 0x8000) {
    pci_printf(cfg, "VPD data overflow, remain %#x\n", remain);
    state = -1;
    break;
}

The missing break in the old code isn't great either as you could potentially malloc() a really large value.

jhb: Or to also avoid inaddressible locations. However, the "short" case would also need the same…
state = -1;
pci_printf(cfg,
"invalid VPD data, remain %#x\n",
remain);
}
name = byte & 0x7f; name = byte & 0x7f;
} else { } else {
remain = byte & 0x7; remain = byte & 0x7;
name = (byte >> 3) & 0xf; name = (byte >> 3) & 0xf;
}
if (vrs.off + remain - vrs.bytesinval > 0x8000) {
pci_printf(cfg,
"VPD data overflow, remain %#x\n", remain);
state = -1;
break;
} }
switch (name) { switch (name) {
case 0x2: /* String */ case 0x2: /* String */
cfg->vpd.vpd_ident = malloc(remain + 1, cfg->vpd.vpd_ident = malloc(remain + 1,
M_DEVBUF, M_WAITOK); M_DEVBUF, M_WAITOK);
i = 0; i = 0;
state = 1; state = 1;
break; break;
▲ Show 20 Lines • Show All 5,502 Lines • Show Last 20 Lines