[PATCH 0/4] Misc. PowerPC MPC83xx fixes/cleanups

[PATCH 0/4] Misc. PowerPC MPC83xx fixes/cleanups

[J. Neuschäfer]

This patchset contains a few small fixes/cleanups for the MPC83xx
platform.

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
J. Neuschäfer (4):
      powerpc: mpc83xx: Fix timer value calculation
      powerpc: mpc83xx: Allow including initreg.h into multiple files
      powerpc: mpc83xx: Use defined constant for SPCR[TBEN]
      gpio: mpc8xxx: Preserve pre-init state of outputs

 arch/powerpc/cpu/mpc83xx/initreg/initreg.h |  8 ++++----
 arch/powerpc/cpu/mpc83xx/interrupts.c      |  3 ++-
 drivers/gpio/mpc8xxx_gpio.c                | 12 +++++++++++-
 drivers/timer/mpc83xx_timer.c              |  2 +-
 4 files changed, 18 insertions(+), 7 deletions(-)
---
base-commit: 9bc62c980d418f0a67632ab29cd3501072cdb6f0
change-id: 20241206-mpc83xx-misc-f438f7e054d8

Best regards,
-- 
J. Neuschäfer <j.ne@posteo.net>

[PATCH 0/4] Misc. PowerPC MPC83xx fixes/cleanups

[J. Neuschäfer via B4 Relay]

This patchset contains a few small fixes/cleanups for the MPC83xx
platform.

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
J. Neuschäfer (4):
      powerpc: mpc83xx: Fix timer value calculation
      powerpc: mpc83xx: Allow including initreg.h into multiple files
      powerpc: mpc83xx: Use defined constant for SPCR[TBEN]
      gpio: mpc8xxx: Preserve pre-init state of outputs

 arch/powerpc/cpu/mpc83xx/initreg/initreg.h |  8 ++++----
 arch/powerpc/cpu/mpc83xx/interrupts.c      |  3 ++-
 drivers/gpio/mpc8xxx_gpio.c                | 12 +++++++++++-
 drivers/timer/mpc83xx_timer.c              |  2 +-
 4 files changed, 18 insertions(+), 7 deletions(-)
---
base-commit: 9bc62c980d418f0a67632ab29cd3501072cdb6f0
change-id: 20241206-mpc83xx-misc-f438f7e054d8

Best regards,
-- 
J. Neuschäfer <j.ne@posteo.net>

[PATCH 1/4] powerpc: mpc83xx: Fix timer value calculation

[J. Neuschäfer]

TBU and TBL are specified as two 32-bit registers that form a 64-bit
value, but the calculation only shifted TBU by 16 bits.

Fix this by actually shifting 32 bits.

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
 drivers/timer/mpc83xx_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/timer/mpc83xx_timer.c b/drivers/timer/mpc83xx_timer.c
index 9da74479aaa678bfab861950a770b216b604c433..f92009e4ccc8256d4c5f4d11e414dfc141f5a043 100644
--- a/drivers/timer/mpc83xx_timer.c
+++ b/drivers/timer/mpc83xx_timer.c
@@ -206,7 +206,7 @@ static u64 mpc83xx_timer_get_count(struct udevice *dev)
 		tbl = mftb();
 	} while (tbu != mftbu());
 
-	return (tbu * 0x10000ULL) + tbl;
+	return (uint64_t)tbu << 32 | tbl;
 }
 
 static int mpc83xx_timer_probe(struct udevice *dev)

-- 
2.45.2

[PATCH 1/4] powerpc: mpc83xx: Fix timer value calculation

[J. Neuschäfer via B4 Relay]

From: "J. Neuschäfer" <j.ne@posteo.net>

TBU and TBL are specified as two 32-bit registers that form a 64-bit
value, but the calculation only shifted TBU by 16 bits.

Fix this by actually shifting 32 bits.

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
 drivers/timer/mpc83xx_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/timer/mpc83xx_timer.c b/drivers/timer/mpc83xx_timer.c
index 9da74479aaa678bfab861950a770b216b604c433..f92009e4ccc8256d4c5f4d11e414dfc141f5a043 100644
--- a/drivers/timer/mpc83xx_timer.c
+++ b/drivers/timer/mpc83xx_timer.c
@@ -206,7 +206,7 @@ static u64 mpc83xx_timer_get_count(struct udevice *dev)
 		tbl = mftb();
 	} while (tbu != mftbu());
 
-	return (tbu * 0x10000ULL) + tbl;
+	return (uint64_t)tbu << 32 | tbl;
 }
 
 static int mpc83xx_timer_probe(struct udevice *dev)

-- 
2.45.2

[PATCH 2/4] powerpc: mpc83xx: Allow including initreg.h into multiple files

[J. Neuschäfer]

Globals defined in headers can result in multiple-definition errors
while linking, if they are visible beyond the current translation unit.

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
 arch/powerpc/cpu/mpc83xx/initreg/initreg.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/cpu/mpc83xx/initreg/initreg.h b/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
index 63aa5c946696ee0368bb3453b40ff0110f0fbcfd..ea1176e7fe10dbb549125ab6b2706fee05a92734 100644
--- a/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
+++ b/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
@@ -13,7 +13,7 @@
 #define SPCR_TSECBDP_MASK	0x00000C00
 #define SPCR_TSECEP_MASK	0x00000300
 
-	const __be32 spcr_mask =
+	static const __be32 spcr_mask =
 #if defined(CONFIG_SPCR_OPT) && !defined(CONFIG_SPCR_OPT_UNSET)
 		SPCR_OPT_MASK |
 #endif
@@ -27,7 +27,7 @@
 		SPCR_TSEC2EP_MASK |
 #endif
 		0;
-	const __be32 spcr_val =
+	static const __be32 spcr_val =
 #if defined(CONFIG_SPCR_OPT) && !defined(CONFIG_SPCR_OPT_UNSET)
 		CONFIG_SPCR_OPT |
 #endif
@@ -42,7 +42,7 @@
 #endif
 		0;
 
-	const __be32 lcrr_mask =
+	static const __be32 lcrr_mask =
 #if defined(CONFIG_LCRR_DBYP) && !defined(CONFIG_LCRR_DBYP_UNSET)
 		LCRR_DBYP |
 #endif
@@ -60,7 +60,7 @@
 #endif
 		0;
 
-	const __be32 lcrr_val =
+	static const __be32 lcrr_val =
 #if defined(CONFIG_LCRR_DBYP) && !defined(CONFIG_LCRR_DBYP_UNSET)
 		CONFIG_LCRR_DBYP |
 #endif

-- 
2.45.2

[PATCH 2/4] powerpc: mpc83xx: Allow including initreg.h into multiple files

[J. Neuschäfer via B4 Relay]

From: "J. Neuschäfer" <j.ne@posteo.net>

Globals defined in headers can result in multiple-definition errors
while linking, if they are visible beyond the current translation unit.

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
 arch/powerpc/cpu/mpc83xx/initreg/initreg.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/cpu/mpc83xx/initreg/initreg.h b/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
index 63aa5c946696ee0368bb3453b40ff0110f0fbcfd..ea1176e7fe10dbb549125ab6b2706fee05a92734 100644
--- a/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
+++ b/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
@@ -13,7 +13,7 @@
 #define SPCR_TSECBDP_MASK	0x00000C00
 #define SPCR_TSECEP_MASK	0x00000300
 
-	const __be32 spcr_mask =
+	static const __be32 spcr_mask =
 #if defined(CONFIG_SPCR_OPT) && !defined(CONFIG_SPCR_OPT_UNSET)
 		SPCR_OPT_MASK |
 #endif
@@ -27,7 +27,7 @@
 		SPCR_TSEC2EP_MASK |
 #endif
 		0;
-	const __be32 spcr_val =
+	static const __be32 spcr_val =
 #if defined(CONFIG_SPCR_OPT) && !defined(CONFIG_SPCR_OPT_UNSET)
 		CONFIG_SPCR_OPT |
 #endif
@@ -42,7 +42,7 @@
 #endif
 		0;
 
-	const __be32 lcrr_mask =
+	static const __be32 lcrr_mask =
 #if defined(CONFIG_LCRR_DBYP) && !defined(CONFIG_LCRR_DBYP_UNSET)
 		LCRR_DBYP |
 #endif
@@ -60,7 +60,7 @@
 #endif
 		0;
 
-	const __be32 lcrr_val =
+	static const __be32 lcrr_val =
 #if defined(CONFIG_LCRR_DBYP) && !defined(CONFIG_LCRR_DBYP_UNSET)
 		CONFIG_LCRR_DBYP |
 #endif

-- 
2.45.2

[PATCH 3/4] powerpc: mpc83xx: Use defined constant for SPCR[TBEN]

[J. Neuschäfer]

To increase readability, use the defined constant instead of specifying
SPCR[TBEN] as a number.

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
 arch/powerpc/cpu/mpc83xx/interrupts.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/cpu/mpc83xx/interrupts.c b/arch/powerpc/cpu/mpc83xx/interrupts.c
index af517213f176cf59cc82b54dbe5247800a6c40e6..bb37a1332c877951e766a8891687dae3f05b6ef8 100644
--- a/arch/powerpc/cpu/mpc83xx/interrupts.c
+++ b/arch/powerpc/cpu/mpc83xx/interrupts.c
@@ -12,6 +12,7 @@
 #include <asm/global_data.h>
 #include <asm/processor.h>
 #include <asm/ptrace.h>
+#include "initreg/initreg.h"
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -29,7 +30,7 @@ void interrupt_init_cpu (unsigned *decrementer_count)
 
 	/* Enable e300 time base */
 
-	immr->sysconf.spcr |= 0x00400000;
+	immr->sysconf.spcr |= SPCR_TBEN_MASK;
 }
 
 /*

-- 
2.45.2

[PATCH 3/4] powerpc: mpc83xx: Use defined constant for SPCR[TBEN]

[J. Neuschäfer via B4 Relay]

From: "J. Neuschäfer" <j.ne@posteo.net>

To increase readability, use the defined constant instead of specifying
SPCR[TBEN] as a number.

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
 arch/powerpc/cpu/mpc83xx/interrupts.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/cpu/mpc83xx/interrupts.c b/arch/powerpc/cpu/mpc83xx/interrupts.c
index af517213f176cf59cc82b54dbe5247800a6c40e6..bb37a1332c877951e766a8891687dae3f05b6ef8 100644
--- a/arch/powerpc/cpu/mpc83xx/interrupts.c
+++ b/arch/powerpc/cpu/mpc83xx/interrupts.c
@@ -12,6 +12,7 @@
 #include <asm/global_data.h>
 #include <asm/processor.h>
 #include <asm/ptrace.h>
+#include "initreg/initreg.h"
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -29,7 +30,7 @@ void interrupt_init_cpu (unsigned *decrementer_count)
 
 	/* Enable e300 time base */
 
-	immr->sysconf.spcr |= 0x00400000;
+	immr->sysconf.spcr |= SPCR_TBEN_MASK;
 }
 
 /*

-- 
2.45.2

[PATCH 4/4] gpio: mpc8xxx: Preserve pre-init state of outputs

[J. Neuschäfer]

The mpc8xxx_gpio driver contains a workaround for certain chips
where the previously written state of outputs cannot be read back
from the GPIO data (GPDAT) register (MPC8572/MPC8536). This workaround
consists of tracking the state of GPDAT in a "shadow register" (i.e. a
software variable). The shadow register is initialized to zero.

This results in a problem w.r.t. outputs that are configured to a
high (1) state before U-Boot runs, but not touched by U-Boot itself:
Due to the zero-initialization, these GPIOs end up being set to zero,
the first time that any other output is set.

To avoid such issues initialize the GPDAT shadow register to the value
previously held by any outputs, if possible. On MPC8572/MPC8536 this
should make no difference, i.e. the shadow register should be
initialized to zero on these chips.

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
 drivers/gpio/mpc8xxx_gpio.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/mpc8xxx_gpio.c b/drivers/gpio/mpc8xxx_gpio.c
index e9bd38f162c10c76460bc63f93c03a711cb4d38e..709d04017d154ecae1e8d16d7c35ced521157c4d 100644
--- a/drivers/gpio/mpc8xxx_gpio.c
+++ b/drivers/gpio/mpc8xxx_gpio.c
@@ -204,7 +204,17 @@ static int mpc8xxx_gpio_plat_to_priv(struct udevice *dev)
 		return -ENOMEM;
 
 	priv->gpio_count = plat->ngpios;
-	priv->dat_shadow = 0;
+
+	/*
+	 * On platforms that do support reading back output values, we want to
+	 * try preserving them, so that we don't accidentally set unrelated
+	 * GPIOs to zero in mpc8xxx_gpio_set_value.
+	 */
+	if (priv->little_endian)
+		priv->dat_shadow = in_le32(&priv->base->gpdat) & in_le32(&priv->base->gpdir);
+	else
+		priv->dat_shadow = in_be32(&priv->base->gpdat) & in_be32(&priv->base->gpdir);
+
 
 	priv->type = driver_data;
 

-- 
2.45.2

[PATCH 4/4] gpio: mpc8xxx: Preserve pre-init state of outputs

[J. Neuschäfer via B4 Relay]

From: "J. Neuschäfer" <j.ne@posteo.net>

The mpc8xxx_gpio driver contains a workaround for certain chips
where the previously written state of outputs cannot be read back
from the GPIO data (GPDAT) register (MPC8572/MPC8536). This workaround
consists of tracking the state of GPDAT in a "shadow register" (i.e. a
software variable). The shadow register is initialized to zero.

This results in a problem w.r.t. outputs that are configured to a
high (1) state before U-Boot runs, but not touched by U-Boot itself:
Due to the zero-initialization, these GPIOs end up being set to zero,
the first time that any other output is set.

To avoid such issues initialize the GPDAT shadow register to the value
previously held by any outputs, if possible. On MPC8572/MPC8536 this
should make no difference, i.e. the shadow register should be
initialized to zero on these chips.

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
 drivers/gpio/mpc8xxx_gpio.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/mpc8xxx_gpio.c b/drivers/gpio/mpc8xxx_gpio.c
index e9bd38f162c10c76460bc63f93c03a711cb4d38e..709d04017d154ecae1e8d16d7c35ced521157c4d 100644
--- a/drivers/gpio/mpc8xxx_gpio.c
+++ b/drivers/gpio/mpc8xxx_gpio.c
@@ -204,7 +204,17 @@ static int mpc8xxx_gpio_plat_to_priv(struct udevice *dev)
 		return -ENOMEM;
 
 	priv->gpio_count = plat->ngpios;
-	priv->dat_shadow = 0;
+
+	/*
+	 * On platforms that do support reading back output values, we want to
+	 * try preserving them, so that we don't accidentally set unrelated
+	 * GPIOs to zero in mpc8xxx_gpio_set_value.
+	 */
+	if (priv->little_endian)
+		priv->dat_shadow = in_le32(&priv->base->gpdat) & in_le32(&priv->base->gpdir);
+	else
+		priv->dat_shadow = in_be32(&priv->base->gpdat) & in_be32(&priv->base->gpdir);
+
 
 	priv->type = driver_data;
 

-- 
2.45.2

Re: [PATCH 0/4] Misc. PowerPC MPC83xx fixes/cleanups

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On Sun, Dec 15, 2024 at 04:18:12PM +0100, J. Neuschäfer via B4 Relay wrote:

> This patchset contains a few small fixes/cleanups for the MPC83xx
> platform.
> 
> Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
> ---
> J. Neuschäfer (4):
>       powerpc: mpc83xx: Fix timer value calculation
>       powerpc: mpc83xx: Allow including initreg.h into multiple files
>       powerpc: mpc83xx: Use defined constant for SPCR[TBEN]
>       gpio: mpc8xxx: Preserve pre-init state of outputs
> 
>  arch/powerpc/cpu/mpc83xx/initreg/initreg.h |  8 ++++----
>  arch/powerpc/cpu/mpc83xx/interrupts.c      |  3 ++-
>  drivers/gpio/mpc8xxx_gpio.c                | 12 +++++++++++-
>  drivers/timer/mpc83xx_timer.c              |  2 +-
>  4 files changed, 18 insertions(+), 7 deletions(-)

Sinan, do you have time to look at these patches by chance?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/4] powerpc: mpc83xx: Allow including initreg.h into multiple files

[Sinan Akman]

Hi

On 2024-12-15 10:18, J. Neuschäfer via B4 Relay wrote:
> From: "J. Neuschäfer"<j.ne@posteo.net>
>
> Globals defined in headers can result in multiple-definition errors
> while linking, if they are visible beyond the current translation unit.

Is this happening anywhere ? It seems you introduced this in your other
patch :
[PATCH 3/4] powerpc: mpc83xx: Use defined constant for SPCR[TBEN]

If this is the only place you are seeing the problem, can we not protect
that the file
is not included multiple times ?

>
> Signed-off-by: J. Neuschäfer<j.ne@posteo.net>
> ---
>   arch/powerpc/cpu/mpc83xx/initreg/initreg.h | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/cpu/mpc83xx/initreg/initreg.h b/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
> index 63aa5c946696ee0368bb3453b40ff0110f0fbcfd..ea1176e7fe10dbb549125ab6b2706fee05a92734 100644
> --- a/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
> +++ b/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
> @@ -13,7 +13,7 @@
>   #define SPCR_TSECBDP_MASK	0x00000C00
>   #define SPCR_TSECEP_MASK	0x00000300
>
> -	const __be32 spcr_mask =
> +	static const __be32 spcr_mask =
>   #if defined(CONFIG_SPCR_OPT) && !defined(CONFIG_SPCR_OPT_UNSET)
>   		SPCR_OPT_MASK |
>   #endif
> @@ -27,7 +27,7 @@
>   		SPCR_TSEC2EP_MASK |
>   #endif
>   		0;
> -	const __be32 spcr_val =
> +	static const __be32 spcr_val =
>   #if defined(CONFIG_SPCR_OPT) && !defined(CONFIG_SPCR_OPT_UNSET)
>   		CONFIG_SPCR_OPT |
>   #endif
> @@ -42,7 +42,7 @@
>   #endif
>   		0;
>
> -	const __be32 lcrr_mask =
> +	static const __be32 lcrr_mask =
>   #if defined(CONFIG_LCRR_DBYP) && !defined(CONFIG_LCRR_DBYP_UNSET)
>   		LCRR_DBYP |
>   #endif
> @@ -60,7 +60,7 @@
>   #endif
>   		0;
>
> -	const __be32 lcrr_val =
> +	static const __be32 lcrr_val =
>   #if defined(CONFIG_LCRR_DBYP) && !defined(CONFIG_LCRR_DBYP_UNSET)
>   		CONFIG_LCRR_DBYP |
>   #endif
>

Re: [PATCH 4/4] gpio: mpc8xxx: Preserve pre-init state of outputs

[Sinan Akman]

Hi

On 2024-12-15 10:18, J. Neuschäfer via B4 Relay wrote:
> From: "J. Neuschäfer"<j.ne@posteo.net>
>
> The mpc8xxx_gpio driver contains a workaround for certain chips
> where the previously written state of outputs cannot be read back
> from the GPIO data (GPDAT) register (MPC8572/MPC8536). This workaround
> consists of tracking the state of GPDAT in a "shadow register" (i.e. a
> software variable). The shadow register is initialized to zero.
>
> This results in a problem w.r.t. outputs that are configured to a
> high (1) state before U-Boot runs, but not touched by U-Boot itself:
> Due to the zero-initialization, these GPIOs end up being set to zero,
> the first time that any other output is set.
>
> To avoid such issues initialize the GPDAT shadow register to the value
> previously held by any outputs, if possible. On MPC8572/MPC8536 this
> should make no difference,

  Did you test this on a MPC83xx board ?

> i.e. the shadow register should be
> initialized to zero on these chips.
>
> Signed-off-by: J. Neuschäfer<j.ne@posteo.net>
> ---
>   drivers/gpio/mpc8xxx_gpio.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/mpc8xxx_gpio.c b/drivers/gpio/mpc8xxx_gpio.c
> index e9bd38f162c10c76460bc63f93c03a711cb4d38e..709d04017d154ecae1e8d16d7c35ced521157c4d 100644
> --- a/drivers/gpio/mpc8xxx_gpio.c
> +++ b/drivers/gpio/mpc8xxx_gpio.c
> @@ -204,7 +204,17 @@ static int mpc8xxx_gpio_plat_to_priv(struct udevice *dev)
>   		return -ENOMEM;
>
>   	priv->gpio_count = plat->ngpios;
> -	priv->dat_shadow = 0;
> +
> +	/*
> +	 * On platforms that do support reading back output values, we want to
> +	 * try preserving them, so that we don't accidentally set unrelated
> +	 * GPIOs to zero in mpc8xxx_gpio_set_value.
> +	 */
> +	if (priv->little_endian)
> +		priv->dat_shadow = in_le32(&priv->base->gpdat) & in_le32(&priv->base->gpdir);
> +	else
> +		priv->dat_shadow = in_be32(&priv->base->gpdat) & in_be32(&priv->base->gpdir);
> +
>
>   	priv->type = driver_data;
>
>

Re: [PATCH 1/4] powerpc: mpc83xx: Fix timer value calculation

[Sinan Akman]

On 2024-12-15 10:18, J. Neuschäfer via B4 Relay wrote:
> From: "J. Neuschäfer"<j.ne@posteo.net>
>
> TBU and TBL are specified as two 32-bit registers that form a 64-bit
> value, but the calculation only shifted TBU by 16 bits.
>
> Fix this by actually shifting 32 bits.
>
> Signed-off-by: J. Neuschäfer<j.ne@posteo.net>
> ---
>   drivers/timer/mpc83xx_timer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/timer/mpc83xx_timer.c b/drivers/timer/mpc83xx_timer.c
> index 9da74479aaa678bfab861950a770b216b604c433..f92009e4ccc8256d4c5f4d11e414dfc141f5a043 100644
> --- a/drivers/timer/mpc83xx_timer.c
> +++ b/drivers/timer/mpc83xx_timer.c
> @@ -206,7 +206,7 @@ static u64 mpc83xx_timer_get_count(struct udevice *dev)
>   		tbl = mftb();
>   	} while (tbu != mftbu());
>
> -	return (tbu * 0x10000ULL) + tbl;
> +	return (uint64_t)tbu << 32 | tbl;
>   }
>
>   static int mpc83xx_timer_probe(struct udevice *dev)
>

Reviewed-by: Sinan Akman <sinan@writeme.com>

Re: [PATCH 3/4] powerpc: mpc83xx: Use defined constant for SPCR[TBEN]

[Sinan Akman]

On 2024-12-15 10:18, J. Neuschäfer via B4 Relay wrote:
> From: "J. Neuschäfer"<j.ne@posteo.net>
>
> To increase readability, use the defined constant instead of specifying
> SPCR[TBEN] as a number.
>
> Signed-off-by: J. Neuschäfer<j.ne@posteo.net>
> ---
>   arch/powerpc/cpu/mpc83xx/interrupts.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/cpu/mpc83xx/interrupts.c b/arch/powerpc/cpu/mpc83xx/interrupts.c
> index af517213f176cf59cc82b54dbe5247800a6c40e6..bb37a1332c877951e766a8891687dae3f05b6ef8 100644
> --- a/arch/powerpc/cpu/mpc83xx/interrupts.c
> +++ b/arch/powerpc/cpu/mpc83xx/interrupts.c
> @@ -12,6 +12,7 @@
>   #include <asm/global_data.h>
>   #include <asm/processor.h>
>   #include <asm/ptrace.h>
> +#include "initreg/initreg.h"
>
>   DECLARE_GLOBAL_DATA_PTR;
>
> @@ -29,7 +30,7 @@ void interrupt_init_cpu (unsigned *decrementer_count)
>
>   	/* Enable e300 time base */
>
> -	immr->sysconf.spcr |= 0x00400000;
> +	immr->sysconf.spcr |= SPCR_TBEN_MASK;
>   }
>
>   /*
>

Reviewed-by: Sinan Akman <sinan@writeme.com>

Re: [PATCH 2/4] powerpc: mpc83xx: Allow including initreg.h into multiple files

[J. Neuschäfer]

On Wed, Dec 18, 2024 at 02:18:48PM -0500, Sinan Akman wrote:
> Hi
> 
> On 2024-12-15 10:18, J. Neuschäfer via B4 Relay wrote:
> > From: "J. Neuschäfer"<j.ne@posteo.net>
> > 
> > Globals defined in headers can result in multiple-definition errors
> > while linking, if they are visible beyond the current translation unit.
> 
> Is this happening anywhere ? It seems you introduced this in your other
> patch :
> [PATCH 3/4] powerpc: mpc83xx: Use defined constant for SPCR[TBEN]

Right, I should have mentioned it: The problem appears only due to the
next patch.

> If this is the only place you are seeing the problem, can we not protect
> that the file is not included multiple times ?

The problem here is the inclusion of initreg.h into multiple .c files,
rather than multiple times into the same .c file. I don't think there's
an easy guard pattern against that (the proprocessor won't help here).

I think the benefit of being able to use these constants is big enough
to justify this change.


Thanks for your review,
 -- jn



> 
> > 
> > Signed-off-by: J. Neuschäfer<j.ne@posteo.net>
> > ---
> >   arch/powerpc/cpu/mpc83xx/initreg/initreg.h | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/powerpc/cpu/mpc83xx/initreg/initreg.h b/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
> > index 63aa5c946696ee0368bb3453b40ff0110f0fbcfd..ea1176e7fe10dbb549125ab6b2706fee05a92734 100644
> > --- a/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
> > +++ b/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
> > @@ -13,7 +13,7 @@
> >   #define SPCR_TSECBDP_MASK	0x00000C00
> >   #define SPCR_TSECEP_MASK	0x00000300
> > 
> > -	const __be32 spcr_mask =
> > +	static const __be32 spcr_mask =
[...]

Re: [PATCH 4/4] gpio: mpc8xxx: Preserve pre-init state of outputs

[J. Neuschäfer]

On Wed, Dec 18, 2024 at 02:20:21PM -0500, Sinan Akman wrote:
> Hi
> 
> On 2024-12-15 10:18, J. Neuschäfer via B4 Relay wrote:
> > From: "J. Neuschäfer"<j.ne@posteo.net>
> > 
> > The mpc8xxx_gpio driver contains a workaround for certain chips
> > where the previously written state of outputs cannot be read back
> > from the GPIO data (GPDAT) register (MPC8572/MPC8536). This workaround
> > consists of tracking the state of GPDAT in a "shadow register" (i.e. a
> > software variable). The shadow register is initialized to zero.
> > 
> > This results in a problem w.r.t. outputs that are configured to a
> > high (1) state before U-Boot runs, but not touched by U-Boot itself:
> > Due to the zero-initialization, these GPIOs end up being set to zero,
> > the first time that any other output is set.
> > 
> > To avoid such issues initialize the GPDAT shadow register to the value
> > previously held by any outputs, if possible. On MPC8572/MPC8536 this
> > should make no difference,
> 
>  Did you test this on a MPC83xx board ?

Yes, on MPC8314E. I'll mention it in the commit message.

I did not test it on MPC85xx, though.


 -- jn

Re: [PATCH 4/4] gpio: mpc8xxx: Preserve pre-init state of outputs

[Sinan Akman]

On 2024-12-19 08:06, J. Neuschäfer wrote:
> On Wed, Dec 18, 2024 at 02:20:21PM -0500, Sinan Akman wrote:
>> Hi
>>
>> On 2024-12-15 10:18, J. Neuschäfer via B4 Relay wrote:
>>> From: "J. Neuschäfer"<j.ne@posteo.net>
>>>
>>> The mpc8xxx_gpio driver contains a workaround for certain chips
>>> where the previously written state of outputs cannot be read back
>>> from the GPIO data (GPDAT) register (MPC8572/MPC8536). This workaround
>>> consists of tracking the state of GPDAT in a "shadow register" (i.e. a
>>> software variable). The shadow register is initialized to zero.
>>>
>>> This results in a problem w.r.t. outputs that are configured to a
>>> high (1) state before U-Boot runs, but not touched by U-Boot itself:
>>> Due to the zero-initialization, these GPIOs end up being set to zero,
>>> the first time that any other output is set.
>>>
>>> To avoid such issues initialize the GPDAT shadow register to the value
>>> previously held by any outputs, if possible. On MPC8572/MPC8536 this
>>> should make no difference,
>>
>>   Did you test this on a MPC83xx board ?
>
> Yes, on MPC8314E. I'll mention it in the commit message.

yes please do, thanks.

Reviewed-by: Sinan Akman  <sinan@writeme.com>

>
> I did not test it on MPC85xx, though.
>
>
>   -- jn

Re: [PATCH 2/4] powerpc: mpc83xx: Allow including initreg.h into multiple files

[Sinan Akman]

On 2024-12-19 07:55, J. Neuschäfer wrote:
> On Wed, Dec 18, 2024 at 02:18:48PM -0500, Sinan Akman wrote:
>> Hi
>>
>> On 2024-12-15 10:18, J. Neuschäfer via B4 Relay wrote:
>>> From: "J. Neuschäfer"<j.ne@posteo.net>
>>>
>>> Globals defined in headers can result in multiple-definition errors
>>> while linking, if they are visible beyond the current translation unit.
>>
>> Is this happening anywhere ? It seems you introduced this in your other
>> patch :
>> [PATCH 3/4] powerpc: mpc83xx: Use defined constant for SPCR[TBEN]
>
> Right, I should have mentioned it: The problem appears only due to the
> next patch.
>
>> If this is the only place you are seeing the problem, can we not protect
>> that the file is not included multiple times ?
>
> The problem here is the inclusion of initreg.h into multiple .c files,
> rather than multiple times into the same .c file. I don't think there's
> an easy guard pattern against that (the proprocessor won't help here).
>
> I think the benefit of being able to use these constants is big enough
> to justify this change.

we'll go with this for now, thanks.

Reviewed-by: Sinan Akman  <sinan@writeme.com>

>
>
> Thanks for your review,
>   -- jn
>
>
>
>>
>>>
>>> Signed-off-by: J. Neuschäfer<j.ne@posteo.net>
>>> ---
>>>    arch/powerpc/cpu/mpc83xx/initreg/initreg.h | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/powerpc/cpu/mpc83xx/initreg/initreg.h b/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
>>> index 63aa5c946696ee0368bb3453b40ff0110f0fbcfd..ea1176e7fe10dbb549125ab6b2706fee05a92734 100644
>>> --- a/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
>>> +++ b/arch/powerpc/cpu/mpc83xx/initreg/initreg.h
>>> @@ -13,7 +13,7 @@
>>>    #define SPCR_TSECBDP_MASK	0x00000C00
>>>    #define SPCR_TSECEP_MASK	0x00000300
>>>
>>> -	const __be32 spcr_mask =
>>> +	static const __be32 spcr_mask =
> [...]