Page MenuHomeFreeBSD

WIP: [PowerPC] PowerMac timebase sync for G4
AcceptedPublic

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

Details

Reviewers
jhibbits
Group Reviewers
PowerPC
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
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

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)

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
725

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 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.)

sys/powerpc/powermac/macio.c
725

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.

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?

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
. . .
  • 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.

sys/powerpc/powermac/platform_powermac.c
436

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.)

sys/powerpc/powermac/platform_powermac.c
436

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.

sys/powerpc/powermac/platform_powermac.c
436

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.)

sys/powerpc/powermac/platform_powermac.c
436

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.

Updating comments

sys/powerpc/aim/mp_cpudep.c
316 ↗(On Diff #85360)

This bit was resolved by it being removed in 921716186f121a2 which I just MFC'd to stable/13.

Rebase against latest code.

sys/conf/files.powerpc
220

Note for stable/13 testers: This part will conflict when applying the patch, please fix it up manually.

Seems to work fine on my G4.

This revision is now accepted and ready to land.Apr 23 2021, 8:12 PM