Differential D29136

WIP: [PowerPC] PowerMac timebase sync for G4
ClosedPublic
Actions

Authored by bdragon on Mar 8 2021, 11:02 PM.

Details

Reviewers

jhibbits

Group Reviewers

PowerPC

Commits

rGc583b0258728: [PowerPC] PowerMac timebase sync for G4

Summary

Note: This still needs testing on an SMP G4 machine that has a timebase-enable property on /cpus/PowerPC,G4@0 -- I did that part blind because my own machine is too old to have that.

Todo:

Implement various G5 timebase controls.
Print out platform code on unknown G5s so we can collect it.
Change API to be give/take pairs like Linux does so it's possible to do a software sync protocol.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

bdragon created this revision.Mar 8 2021, 11:02 PM

Herald added subscribers: alfredo, luporl. · View Herald TranscriptMar 8 2021, 11:02 PM

bdragon requested review of this revision.Mar 8 2021, 11:02 PM

bdragon added inline comments.Mar 8 2021, 11:24 PM

sys/powerpc/aim/mp_cpudep.c
316 ↗	(On Diff #85360)	This seems pretty janky btw. Resetting the timebase to 0 seems like it would break all sorts of stuff when doing suspend/resume. We should probably look into this at some point and restore the timebase in a more disciplined manner (even on uniprocessor)

jhibbits added inline comments.Mar 9 2021, 1:27 AM

sys/powerpc/aim/mp_cpudep.c
316 ↗	(On Diff #85360)	Yeah, may as well just #ifndef powerpc64 this, since it's only really going to be used on G4, and do a direct 'mttb(0)'. It will be properly restored later on.
sys/powerpc/powermac/macio.c
726	Is 'frozen' to mean that it's already frozen, or that you want to freeze it? I'd rename it 'freeze' in that case, or 'thaw' if it's already frozen.

In case it helps for some G4 dual socket contexts (captured while the machine happens
to be up and accessible). Turns out each of the cpus has a timebase-enable listed, but
they are numerically equal on the machine in question.

. . .
  model:
    50 6f 77 65 72 4d 61 63 33 2c 36 00 
    'PowerMac3,6'
. . .
  Node 0x1f4: cpus
    phandle:
      ff 88 34 00 
      '\M^?\M^H4'
    name:
      63 70 75 73 00 
      'cpus'
    #address-cells:
      00 00 00 01 
    #size-cells:
      00 00 00 00 
    #cpus:
      00 00 00 02 
. . .
    Node 0x274: PowerPC,G4
      phandle:
        ff 88 36 d8 
      name:
        50 6f 77 65 72 50 43 2c 47 34 00 00 
      device_type:
        63 70 75 00 
        'cpu'
      reg:
        00 00 00 00 
      cpu-version:
        80 01 03 03 
. . .
      timebase-enable:
        00 00 00 73 
. . .
    Node 0x934: PowerPC,G4
      phandle:
        ff 88 4b 08 
      name:
        50 6f 77 65 72 50 43 2c 47 34 00 00 
      device_type:
        63 70 75 00 
        'cpu'
      reg:
        00 00 00 01 
      cpu-version:
        80 01 03 03 
. . .
      timebase-enable:
        00 00 00 73 
. . .

In D29136#652512, @markmi_dsl-only.net wrote:

In case it helps for some G4 dual socket contexts (captured while the machine happens
to be up and accessible). Turns out each of the cpus has a timebase-enable listed, but
they are numerically equal on the machine in question.

...

timebase-enable:
  00 00 00 73

Yep. 0x73 is the "expected" value for this. It is the GPIO base offset of 0x6a plus the offset to the timebase control of 0x9. As far as I know, that's how all G4s are wired, but we respect the property in case there is a model out there that is wired differently.

The value is the same on all cores because the timebase clock is enabled or disabled globally. The TBEN input signal line on all processors are tied together and controlled by the GPIO, so they all turn on and off at the same time.

The reason for freezing the timebase in the first place is so we can take advantage of this property to ensure the same value is in all cores' timebase registers without having to worry about memory latency and execution differences. We can freeze the timebase, update the registers to all have the same value, and then unfreezing the timebase will ensure that the timebase will continue to be synchronized between all cores, because all cores had the same value and were also reenabled at the same time.

You can tell if this new code is kicking in for G4 by checking dmesg for a "macio0: GPIO timebase control at 0x73" line. If you have this line, you should be good to go.

Thanks for the confirmation that machines that do have the timebase-enable property have the value I was expecting (my own G4 dual processor is too old to have the property, so it uses the hardcoded fallback default, which is also 0x73.)

bdragon added inline comments.Mar 9 2021, 2:20 AM

sys/powerpc/powermac/macio.c
726	good point. Will change to "freeze"

I'll remind of the old issue that the pre-exisiting code's use of platform_smp_timebase_sync does
not follow a synchronize everything at once protocol everywhere, sometimes using the ap
argument being 1 without any matching ap being 0. This invalidated one of jhibbits patches in
the past. (I'm writing from vague memory here. So be careful from reading too much detail.)

Looking shows 4 uses of platform_smp_timebase_sync still seems to have all 4 of:

static void cpu_mp_unleash(void *dummy) does: platform_smp_timebase_sync(ap_timebase, 0)
       void machdep_ap_bootstrap(void)  does: platform_smp_timebase_sync(ap_timebase, 1)
       void cpu_sleep()                 does: platform_smp_timebase_sync(timebase, 0)
       void cpudep_ap_setup()           does: platform_smp_timebase_sync(0, 1)

From what I can tell both cpudep_ap_setup() usage and the cpu_sleep() usage will not work
for this proposed update: wrong outer level protocol for what the update requires of such a
code protocol. (But I'm no expert.)

jhibbits had written at one time:

I do intend to shortly (next few days) clean up the one additional platform_timebase_sync()
user (cpu_sleep()) to not use it, and handle it directly, since the code in cpu_sleep() is actually
mpc74xx-specific, it's not generic at all. So once that's made explicit, a proper rendezvous
sync should be done in platform_powermac.c instead.

I had then asked if he was also removing the usage from cpudep_ap_setup(), not just the usage from cpu_sleep().
But I think he agreed at the time that part of the platform_smp_timebase_sync usage protocol was messed
up and needed rework, even if I got the cpudep_ap_setup() part wrong.

bdragon added inline comments.Mar 9 2021, 2:45 AM

sys/powerpc/aim/mp_cpudep.c
316 ↗	(On Diff #85360)	fwiw, in cpu_sleep(), for things to work at all on SMP, we would need to IPI the other processors into doing the sync with us. Is PMU frequency control ever enabled for SMP machines, or are we safe there?

jhibbits added inline comments.Mar 9 2021, 3:09 AM

sys/powerpc/aim/mp_cpudep.c
316 ↗	(On Diff #85360)	PMU frequency control is, to the best of my knowledge, only on PowerBooks, which are UP.

FYI: G5 2-sockets/2-cores-each basic info follows.

But https://bugs.freebsd.org/bugzilla/attachment.cgi?id=223108 has a xz of a full ofwdump -ap output.

. . .
  model:
    50 6f 77 65 72 4d 61 63 31 31 2c 32 00 
    'PowerMac11,2'
. . .
  Node 0x238: cpus
    phandle:
      ff 89 d3 f0 
    name:
      63 70 75 73 00 
      'cpus'
    #address-cells:
      00 00 00 01 
    #size-cells:
      00 00 00 00 
    #interrupt-cells:
      00 00 00 02 
    platform-cpu-timebase:
      ff 99 1d 78 
. . .
  Node 0x7518: ht
. . .
    Node 0x84dc: pci
. . .
      Node 0x8690: mac-io
. . .
        Node 0x8b64: gpio
. . .
          Node 0x91f4: timebase-enable
            phandle:
              ff 99 1d 78 
            name:
              74 69 6d 65 62 61 73 65 2d 65 6e 61 62 6c 65 00 
              'timebase-enable'
            device_type:
              67 70 69 6f 00 
              'gpio'
            reg:
              00 00 00 26 
            built-in:
            compatible:
              74 69 6d 65 62 61 73 65 2d 65 6e 61 62 6c 65 00 67 70 69 6f 
              33 30 00 67 70 69 6f 00 00 
            interrupts:
              00 00 00 49 00 3d 00 03 
            interrupt-parent:
              ff 98 14 88 
            platform-do-cpu-timebase:
              ff 89 d3 f0 08 00 00 00 00 00 00 01 00 00 00 00 00 00 00 04 
          Node 0x92dc: amp-mute
. . .

mikael added a subscriber: mikael.Mar 9 2021, 8:18 AM

s/frozen/freeze/
Add G5 handling for "platform function that is just a GPIO write" style timebase control like PowerMac11,2.

Untested as I don't have the hardware, but in theory should be correct unless I missed something silly.

bdragon marked an inline comment as done.Mar 11 2021, 6:30 PM

markmi_dsl-only.net added inline comments.Mar 11 2021, 9:00 PM

sys/powerpc/powermac/platform_powermac.c
437	I still do not see how the code that involves: void cpu_sleep() does: platform_smp_timebase_sync(timebase, 0) void cpudep_ap_setup() does: platform_smp_timebase_sync(0, 1) is supposed to work with these sort of cpu_done vs. mp_ncpus loops. (The two ap_timebase uses of platform_smp_timebase_sync make sense to me, at least as of when I last looked at the details, including cpu_done starting with an appropriate value.)

markmi_dsl-only.net added inline comments.Mar 12 2021, 12:19 AM

sys/powerpc/powermac/platform_powermac.c
437	In case it is unclear: sys/powerpc/aim/aim_machdep.c: platform_smp_timebase_sync(timebase, 0); sys/powerpc/aim/mp_cpudep.c: platform_smp_timebase_sync(0, 1); So the usage in question is AIM specific.

bdragon added inline comments.Mar 12 2021, 4:11 PM

sys/powerpc/powermac/platform_powermac.c
437	Yeah, I'm aware. cpu_sleep() is misnamed and is only used on PMU machines, which are never SMP according to jhibbits. And the cpudep_ap_setup() one I am planning on just changing to mttb(0);. It's tied to suspend/resume support, which is not possible to support in SMP currently (the code has some low level assumptions with static variables that totally fall apart under SMP, and attempting to suspend/resume on SMP will screw up the pcpu data, etc. We would have to implement a lookup table to map the pcpu data to cores after resume.)

markmi_dsl-only.net added inline comments.Mar 13 2021, 6:57 AM

sys/powerpc/powermac/platform_powermac.c
437	I looked up some old notes. According to them (summarized): cpudep_ap_setup happens during cpu_reset_handler before machdep_ap_bootstrap is called (which does platform_smp_timebase_sync as well). That leads to double counting in cpu_done and prevents the code structure proposed from working as is. So it would appear that before testing this code, the mttb(0) replacement in cpudep_ap_setup should be in place already in order to avoid the double counting in cpu_done.