Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: pinmux stm32F1 remap AFIO without changing the SWJ_CFG #38741

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

FRASTM
Copy link
Collaborator

@FRASTM FRASTM commented Sep 22, 2021

Change the SPI1_REMAP bit of the AFIO_MAPR of the stm32F1x soc
without changing the SWJ_CFG[2:0]: Serial wire JTAG configuration

fix #38354

Signed-off-by: Francois Ramu [email protected]

@github-actions github-actions bot added the platform: STM32 ST Micro STM32 label Sep 22, 2021
@FRASTM
Copy link
Collaborator Author

FRASTM commented Sep 23, 2021

OK,
going to change all remap bits of the AFIO_MAPR register,
It takes the same formula as the LL_GPIO_AF_EnableRemap_xxx without changing the AFIO_MAPR_SWJ_CFG bits.
Remapping on the AFIO_MAPR2 is not to change (LL_GPIO_AF_EnableRemap_xxx does not set the AFIO_MAPR_SWJ_CFG bits)

  • disable clock after AFIO setting

@FRASTM FRASTM marked this pull request as ready for review September 23, 2021 08:53
@FRASTM FRASTM requested a review from ABOSTM as a code owner September 23, 2021 08:53
drivers/pinmux/pinmux_stm32.c Outdated Show resolved Hide resolved
@FRASTM FRASTM changed the title drivers: pinmux stm32F1 remap spi1 without changing the SWJ_CFG drivers: pinmux stm32F1 remap AFIO without changing the SWJ_CFG Sep 23, 2021
@zephyrbot zephyrbot requested a review from mnkp September 23, 2021 15:14
/* PB8/PB9 */
LL_GPIO_AF_RemapPartial2_CAN1();
/* PB8/PB9 (CAN_REMAP = 0b10) */
MODIFY_REG(AFIO->MAPR, AFIO_MAPR_CAN_REMAP,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using CMSIS API to modify this register may cause instability:
as per reference manual:
image

So when modifying the register, it starts by reading the register, with undetermined value for SWJ_CFG,
this undetermined value is then written back to the register. Leading to undetermined behavior of SWJ

This is for me the reason why LL API, manipulates SWJ_CFG sytematically for AFIO_MAPR register and set an invalid SWJ_CFG value to have no effect.
So I think we should keep the original LL API.

Copy link
Collaborator Author

@FRASTM FRASTM Sep 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For remapping SPI1, we will have:

		/* enable remap */
#if defined(CONFIG_GPIO_STM32_SWJ_ENABLE)
	/* reset state */
	MODIFY_REG(AFIO->MAPR, (AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG),
			(AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG_RESET));
#elif defined(CONFIG_GPIO_STM32_SWJ_NONJTRST)
	/* released PB4 */
	MODIFY_REG(AFIO->MAPR, (AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG),
			(AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG_NOJNTRST));
#elif defined(CONFIG_GPIO_STM32_SWJ_NOJTAG)
	/* released PB4 PB3 PA15 */
	MODIFY_REG(AFIO->MAPR, (AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG),
			(AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG_JTAGDISABLE));
#elif defined(CONFIG_GPIO_STM32_SWJ_DISABLE)
	/* released PB4 PB3 PA13 PA14 PA15 */
	MODIFY_REG(AFIO->MAPR, (AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG),
			(AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG_DISABLE));
#endif

For disabling remap of SPI1, we will have:

		/* disable remap */
#if defined(CONFIG_GPIO_STM32_SWJ_ENABLE)
	/* reset state */
	MODIFY_REG(AFIO->MAPR, (AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG),
			AFIO_MAPR_SWJ_CFG_RESET);
#elif defined(CONFIG_GPIO_STM32_SWJ_NONJTRST)
	/* released PB4 */
	MODIFY_REG(AFIO->MAPR, (AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG),
			AFIO_MAPR_SWJ_CFG_NOJNTRST);
#elif defined(CONFIG_GPIO_STM32_SWJ_NOJTAG)
	/* released PB4 PB3 PA15 */
	MODIFY_REG(AFIO->MAPR, (AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG),
			AFIO_MAPR_SWJ_CFG_JTAGDISABLE);
#elif defined(CONFIG_GPIO_STM32_SWJ_DISABLE)
	/* released PB4 PB3 PA13 PA14 PA15 */
	MODIFY_REG(AFIO->MAPR, (AFIO_MAPR_SPI1_REMAP | AFIO_MAPR_SWJ_CFG),
			AFIO_MAPR_SWJ_CFG_DISABLE);
#endif	

@FRASTM
Copy link
Collaborator Author

FRASTM commented Sep 24, 2021

In the stm32F1x, AF remap and debug I/O configuration register (AFIO_MAPR) cannot read the SWJ_CFG bits. The modify register macro must apply the Serial wire JTAG configuration from the CONFIG and not from the MAPR register content.
This new version is redefining enable/disable remap macro depending on the the CONFIG_GPIO_STM32_SWJ_xxx position
and not from the MAPR register.

Comment on lines 157 to 227
#if defined(CONFIG_GPIO_STM32_SWJ_ENABLE)
/* reset state */
#define enable_remap(REMAP_PIN) MODIFY_REG(AFIO->MAPR,\
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG_RESET))
#elif defined(CONFIG_GPIO_STM32_SWJ_NONJTRST)
/* released PB4 */
#define enable_remap(REMAP_PIN) MODIFY_REG(AFIO->MAPR, \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG_NOJNTRST))
#elif defined(CONFIG_GPIO_STM32_SWJ_NOJTAG)
/* released PB4 PB3 PA15 */
#define enable_remap(REMAP_PIN) MODIFY_REG(AFIO->MAPR, \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG_JTAGDISABLE))
#elif defined(CONFIG_GPIO_STM32_SWJ_DISABLE)
/* released PB4 PB3 PA13 PA14 PA15 */
#define enable_remap(REMAP_PIN) MODIFY_REG(AFIO->MAPR, \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG_DISABLE))
#endif

/* enable remap : modify MAPR and keep the AFIO_MAPR_SWJ_CFG_x */
#if defined(CONFIG_GPIO_STM32_SWJ_ENABLE)
/* reset state */
#define enable_partial_remap(REMAP_PIN, PARTIAL_REMAP) \
MODIFY_REG(AFIO->MAPR, \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
(PARTIAL_REMAP | AFIO_MAPR_SWJ_CFG_RESET))
#elif defined(CONFIG_GPIO_STM32_SWJ_NONJTRST)
/* released PB4 */
#define enable_partial_remap(REMAP_PIN, PARTIAL_REMAP) \
MODIFY_REG(AFIO->MAPR, \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
(PARTIAL_REMAP | AFIO_MAPR_SWJ_CFG_NOJNTRST))
#elif defined(CONFIG_GPIO_STM32_SWJ_NOJTAG)
/* released PB4 PB3 PA15 */
#define enable_partial_remap(REMAP_PIN, PARTIAL_REMAP) \
MODIFY_REG(AFIO->MAPR, \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
(PARTIAL_REMAP | AFIO_MAPR_SWJ_CFG_JTAGDISABLE))
#elif defined(CONFIG_GPIO_STM32_SWJ_DISABLE)
/* released PB4 PB3 PA13 PA14 PA15 */
#define enable_partial_remap(REMAP_PIN, PARTIAL_REMAP) \
MODIFY_REG(AFIO->MAPR, \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
(PARTIAL_REMAP | AFIO_MAPR_SWJ_CFG_DISABLE))
#endif

/* disable remap : modify MAPR and keep the AFIO_MAPR_SWJ_CFG_x */
#if defined(CONFIG_GPIO_STM32_SWJ_ENABLE)
/* reset state */
#define disable_remap(REMAP_PIN) MODIFY_REG(AFIO->MAPR,\
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
AFIO_MAPR_SWJ_CFG_RESET)
#elif defined(CONFIG_GPIO_STM32_SWJ_NONJTRST)
/* released PB4 */
#define disable_remap(REMAP_PIN) MODIFY_REG(AFIO->MAPR, \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
AFIO_MAPR_SWJ_CFG_NOJNTRST)
#elif defined(CONFIG_GPIO_STM32_SWJ_NOJTAG)
/* released PB4 PB3 PA15 */
#define disable_remap(REMAP_PIN) MODIFY_REG(AFIO->MAPR, \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
AFIO_MAPR_SWJ_CFG_JTAGDISABLE)
#elif defined(CONFIG_GPIO_STM32_SWJ_DISABLE)
/* released PB4 PB3 PA13 PA14 PA15 */
#define disable_remap(REMAP_PIN) MODIFY_REG(AFIO->MAPR, \
(REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
AFIO_MAPR_SWJ_CFG_DISABLE)
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about something more factorized like:

if defined(CONFIG_GPIO_STM32_SWJ_ENABLE)
/* reset state */
#define Z_AFIO_REMAP AFIO_MAPR_SWJ_CFG_RESET
#elif defined(CONFIG_GPIO_STM32_SWJ_NONJTRST)
/* released PB4 */
#define Z_AFIO_REMAP AFIO_MAPR_SWJ_CFG_RESET AFIO_MAPR_SWJ_CFG_NOJNTRST
#elif defined(CONFIG_GPIO_STM32_SWJ_NOJTAG)
/* released PB4 PB3 PA15 */
#define Z_AFIO_REMAP AFIO_MAPR_SWJ_CFG_RESET AFIO_MAPR_SWJ_CFG_JTAGDISABLE
#elif defined(CONFIG_GPIO_STM32_SWJ_DISABLE)
/* released PB4 PB3 PA13 PA14 PA15 */
#define Z_AFIO_REMAP AFIO_MAPR_SWJ_CFG_RESET AFIO_MAPR_SWJ_CFG_DISABLE
#endif


#define enable_remap(REMAP_PIN) MODIFY_REG(AFIO->MAPR,\
				           (REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
				           (REMAP_PIN | Z_AFIO_REMAP))

#define enable_partial_remap(REMAP_PIN, PARTIAL_REMAP) \
				MODIFY_REG(AFIO->MAPR, \
					   (REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
					   (PARTIAL_REMAP | Z_AFIO_REMAP))

#define disable_remap(REMAP_PIN) MODIFY_REG(AFIO->MAPR,\
					    (REMAP_PIN | AFIO_MAPR_SWJ_CFG), \
					    Z_AFIO_REMAP)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should Z_AFIO_REMAP be defined based on the CONFIG_GPIO_STM32_SWJ_xxx defnition :

#if defined(CONFIG_GPIO_STM32_SWJ_ENABLE)
/* reset state */
#define Z_AFIO_REMAP AFIO_MAPR_SWJ_CFG_RESET
#elif defined(CONFIG_GPIO_STM32_SWJ_NONJTRST)
/* released PB4 */
#define Z_AFIO_REMAP AFIO_MAPR_SWJ_CFG_NOJNTRST
#elif defined(CONFIG_GPIO_STM32_SWJ_NOJTAG)
/* released PB4 PB3 PA15 */
#define Z_AFIO_REMAP AFIO_MAPR_SWJ_CFG_JTAGDISABLE
#elif defined(CONFIG_GPIO_STM32_SWJ_DISABLE)
/* released PB4 PB3 PA13 PA14 PA15 */
#define Z_AFIO_REMAP AFIO_MAPR_SWJ_CFG_DISABLE
#endif

@FRASTM
Copy link
Collaborator Author

FRASTM commented Sep 27, 2021

more factorized using Z_AFIO_REMAP

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remap part looks good

@@ -383,6 +383,8 @@ int stm32_dt_pinctrl_remap(const struct soc_gpio_pinctrl *pinctrl,
#endif
}

LL_APB2_GRP1_DisableClock(LL_APB2_GRP1_PERIPH_AFIO);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, AFIO clock is not taken into account in gpio clocks.
So disabling it, here will prevent any further GPIO programmation, such as button.
So I agree current situation is not clean, but I'd prefer we limit changes on that part right now (and postpone to V3.0.0)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, removing the commit

} else {
LL_GPIO_AF_DisableRemap_TIM3();
enable_partial_remap(AFIO_MAPR_TIM3_REMAP,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be disable_remap() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a partial remap on 2-bit TIM3_REMAP[1:0] (disable_remap is for 1-bit like SPI1_REMAP or I2C1_REMAP)

@erwango
Copy link
Member

erwango commented Sep 28, 2021

@ck-telecom, can you review, or at least test and see if fixes your issue ?

@ck-telecom
Copy link
Contributor

SPI1 remap works well with CONFIG_GPIO_STM32_SWJ_NOJTAG=y

Change the REMAP bits of the AFIO_MAPR of the stm32F1x soc
with local MACRO without changing the SWJ_CFG (write-only bits).
The serial wire JTAG configuration is taken from the Z_AFIO_REMAP
(value of the CONFIG_GPIO_STM32_SWJ_xxx))
and not read from the MAPR register.
It accesses to the MAPR register directly instead of LL functions.
Note that Remapping on the MAPR2 is not to change.

Signed-off-by: Francois Ramu <[email protected]>
@erwango erwango added this to the v3.0.0 milestone Oct 4, 2021
@erwango erwango requested review from nashif and cfriedt October 4, 2021 12:42
@cfriedt cfriedt merged commit e763061 into zephyrproject-rtos:main Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stm32: stm32f10x JTAG realated gpio repmap didn't works
6 participants