diff mbox series

[v2,4/7] libpdbg/p8chip.c: release special wakeups for P8

Message ID 20190312014920.25368-5-npiggin@gmail.com
State Accepted
Headers show
Series sreset support for P8 systems | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (deb577949a3505064f471e7b7c692e37c38ec8a4)
snowpatch_ozlabs/build-multiarch success Test build-multiarch on branch master

Commit Message

Nicholas Piggin March 12, 2019, 1:49 a.m. UTC 
This copies the special wakeup release logic from p9chip.c

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 libpdbg/p8chip.c | 67 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 9 deletions(-)

Comments

Alistair Popple March 26, 2019, 1:10 a.m. UTC | #1 
Hi Nick,

> +static void p8_core_release(struct pdbg_target *target)
> +{
> +	struct pdbg_target *child;
> +	struct core *core = target_to_core(target);
> +	enum pdbg_target_status status;
> +
> +	usleep(1); /* enforce small delay before and after it is cleared */
> +
> +	/* Probe and release all threads to ensure release_spwkup is up to
> +	 * date */
> +	pdbg_for_each_target("thread", target, child) {
> +		status = pdbg_target_status(child);
> +
> +		/* This thread has already been release so should have set
> +		 * release_spwkup to false if it was quiesced, */
> +		if (status == PDBG_TARGET_RELEASED)
> +			continue;
> +
> +		status = pdbg_target_probe(child);
> +		if (status != PDBG_TARGET_ENABLED)
> +			continue;
> +
> +		/* Release the thread to ensure release_spwkup is updated. */
> +		pdbg_target_release(child);
> +	}

So the intent of this loop is ensure all threads have their release methods 
called prior to doing the release for the core? The pdbg core should ensure 
that is the case - ie. pdbg_target_release(core) should release all the child 
threads first which I think makes this code redundant.

- Alistair

> +
> +	if (!core->release_spwkup)
> +		return;
> +
> +	deassert_special_wakeup(core);
> +	usleep(10000);
> +}
> +
>  static struct core p8_core = {
>  	.target = {
>  		.name = "POWER8 Core",
>  		.compatible = "ibm,power8-core",
>  		.class = "core",
>  		.probe = p8_core_probe,
> +		.release = p8_core_release,
>  	},
>  };
>  DECLARE_HW_UNIT(p8_core);
Nicholas Piggin March 26, 2019, 6:01 a.m. UTC | #2 
Alistair Popple's on March 26, 2019 11:10 am:
> Hi Nick,
> 
>> +static void p8_core_release(struct pdbg_target *target)
>> +{
>> +	struct pdbg_target *child;
>> +	struct core *core = target_to_core(target);
>> +	enum pdbg_target_status status;
>> +
>> +	usleep(1); /* enforce small delay before and after it is cleared */
>> +
>> +	/* Probe and release all threads to ensure release_spwkup is up to
>> +	 * date */
>> +	pdbg_for_each_target("thread", target, child) {
>> +		status = pdbg_target_status(child);
>> +
>> +		/* This thread has already been release so should have set
>> +		 * release_spwkup to false if it was quiesced, */
>> +		if (status == PDBG_TARGET_RELEASED)
>> +			continue;
>> +
>> +		status = pdbg_target_probe(child);
>> +		if (status != PDBG_TARGET_ENABLED)
>> +			continue;
>> +
>> +		/* Release the thread to ensure release_spwkup is updated. */
>> +		pdbg_target_release(child);
>> +	}
> 
> So the intent of this loop is ensure all threads have their release methods 
> called prior to doing the release for the core?

Yes, from memory.

> The pdbg core should ensure 
> that is the case - ie. pdbg_target_release(core) should release all the child 
> threads first which I think makes this code redundant.
> 

Okay, this is taken from P9 code, so maybe we could remove the 
redundant bits from both in a later patch?

Thanks,
Nick
diff mbox series

Patch

diff --git a/libpdbg/p8chip.c b/libpdbg/p8chip.c
index b93e953..71e6569 100644
--- a/libpdbg/p8chip.c
+++ b/libpdbg/p8chip.c
@@ -163,16 +163,11 @@  static int assert_special_wakeup(struct core *chip)
 	return 0;
 }
 
-#if 0
-/* TODO: Work out when to do this. */
-static int deassert_special_wakeup(struct core *chip)
+static void deassert_special_wakeup(struct core *chip)
 {
 	/* Assert special wakeup to prevent low power states */
-	CHECK_ERR(pib_write(&chip->target, PMSPCWKUPFSP_REG, 0));
-
-	return 0;
+	pib_write(&chip->target, PMSPCWKUPFSP_REG, 0);
 }
-#endif
 
 static struct thread_state get_thread_status(struct thread *thread)
 {
@@ -318,6 +313,8 @@  static int p8_thread_stop(struct thread *thread)
 	val |= PPC_BIT(8) >> thread->id;
 	CHECK_ERR(pib_write(&chip->target, THREAD_ACTIVE_REG, val));
 
+	thread->status = get_thread_status(thread);
+
 	return 0;
 }
 
@@ -336,6 +333,8 @@  static int p8_thread_start(struct thread *thread)
 	val |= PPC_BIT(thread->id);
 	CHECK_ERR(pib_write(&chip->target, THREAD_ACTIVE_REG, val));
 
+	thread->status = get_thread_status(thread);
+
 	return 0;
 }
 
@@ -366,7 +365,7 @@  static int p8_ram_setup(struct thread *thread)
 			return 1;
 
 		tmp = target_to_thread(target);
-		if (!(get_thread_status(tmp).quiesced))
+		if (!tmp->status.quiesced)
 			return 1;
 	}
 
@@ -487,6 +486,16 @@  static int p8_thread_probe(struct pdbg_target *target)
 	return 0;
 }
 
+static void p8_thread_release(struct pdbg_target *target)
+{
+	struct core *core = target_to_core(pdbg_target_require_parent("core", target));
+	struct thread *thread = target_to_thread(target);
+
+	if (thread->status.quiesced)
+		/* this thread is still quiesced so don't release spwkup */
+		core->release_spwkup = false;
+}
+
 static int p8_get_hid0(struct pdbg_target *chip, uint64_t *value)
 {
 	CHECK_ERR(pib_read(chip, HID0_REG, value));
@@ -534,6 +543,7 @@  static struct thread p8_thread = {
 		.compatible = "ibm,power8-thread",
 		.class = "thread",
 		.probe = p8_thread_probe,
+		.release = p8_thread_release,
 	},
 	.step = p8_thread_step,
 	.start = p8_thread_start,
@@ -562,16 +572,55 @@  static int p8_core_probe(struct pdbg_target *target)
 	if (!GETFIELD(PPC_BIT(0), value))
 		return -1;
 
-	assert_special_wakeup(core);
+	if (assert_special_wakeup(core))
+		return -1;
+
+	/* Child threads will set this to false if they are released while quiesced */
+	core->release_spwkup = true;
+
 	return 0;
 }
 
+static void p8_core_release(struct pdbg_target *target)
+{
+	struct pdbg_target *child;
+	struct core *core = target_to_core(target);
+	enum pdbg_target_status status;
+
+	usleep(1); /* enforce small delay before and after it is cleared */
+
+	/* Probe and release all threads to ensure release_spwkup is up to
+	 * date */
+	pdbg_for_each_target("thread", target, child) {
+		status = pdbg_target_status(child);
+
+		/* This thread has already been release so should have set
+		 * release_spwkup to false if it was quiesced, */
+		if (status == PDBG_TARGET_RELEASED)
+			continue;
+
+		status = pdbg_target_probe(child);
+		if (status != PDBG_TARGET_ENABLED)
+			continue;
+
+		/* Release the thread to ensure release_spwkup is updated. */
+		pdbg_target_release(child);
+	}
+
+	if (!core->release_spwkup)
+		return;
+
+	deassert_special_wakeup(core);
+	usleep(10000);
+}
+
 static struct core p8_core = {
 	.target = {
 		.name = "POWER8 Core",
 		.compatible = "ibm,power8-core",
 		.class = "core",
 		.probe = p8_core_probe,
+		.release = p8_core_release,
 	},
 };
 DECLARE_HW_UNIT(p8_core);