[PATCH 0/4] Misc. PowerPC MPC83xx fixes/cleanups
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,
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)
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äferj.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
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
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äferj.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
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äferj.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 =
[...]
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äferj.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 =
[...]
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; }
/*
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äferj.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
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;
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äferj.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;
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
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
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?
participants (4)
-
J. Neuschäfer -
J. Neuschäfer via B4 Relay -
Sinan Akman -
Tom Rini