lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 10 Aug 2022 11:19:27 +0200 From: Neil Armstrong <narmstrong@...libre.com> To: Jerome Brunet <jbrunet@...libre.com>, Amjad Ouled-Ameur <aouledameur@...libre.com>, broonie@...nel.org Cc: linux-kernel@...r.kernel.org, linux-amlogic@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, linux-spi@...r.kernel.org, Da Xue <da@...re.computer> Subject: Re: [PATCH 2/2] spi: meson-spicc: Use pinctrl to drive CLK line when idle On 10/08/2022 10:52, Jerome Brunet wrote: > > On Tue 09 Aug 2022 at 19:20, Amjad Ouled-Ameur <aouledameur@...libre.com> wrote: > >> Between SPI transactions, all SPI pins are in HiZ state. When using the SS >> signal from the SPICC controller it's not an issue because when the >> transaction resumes all pins come back to the right state at the same time >> as SS. >> >> The problem is when we use CS as a GPIO. In fact, between the GPIO CS >> state change and SPI pins state change from idle, you can have a missing or >> spurious clock transition. >> >> Set a bias on the clock depending on the clock polarity requested before CS >> goes active, by passing a special "idle-low" and "idle-high" pinctrl state >> and setting the right state at a start of a message >> >> Reported-by: Da Xue <da@...re.computer> >> Signed-off-by: Neil Armstrong <narmstrong@...libre.com> >> Signed-off-by: Amjad Ouled-Ameur <aouledameur@...libre.com> >> --- >> arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 14 ++++++++ >> drivers/spi/spi-meson-spicc.c | 39 +++++++++++++++++++++- > > These 2 changes should not be in the same patch. > >> 2 files changed, 52 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi >> index c3ac531c4f84..04e9d0f1bde0 100644 >> --- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi >> +++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi > > Does the problem applies only the gxl ? not gxbb, g12, axg ? Only on GXL, starting from AXG the pins mode output state can be kept between bursts. > >> @@ -429,6 +429,20 @@ mux { >> }; >> }; >> >> + spi_idle_high_pins: spi-idle-high-pins { >> + mux { >> + groups = "spi_sclk"; >> + bias-pull-up; >> + }; >> + }; >> + >> + spi_idle_low_pins: spi-idle-low-pins { >> + mux { >> + groups = "spi_sclk"; >> + bias-pull-down; > > Would it be safer to properly drive the pin in push-pull mode ? > Like using gpio pinumux mode and output-high/output-low pinconf ? The pins mux must be kept in the SPI function, thus only a bias can be applied. > >> + }; >> + }; >> + >> spi_ss0_pins: spi-ss0 { >> mux { >> groups = "spi_ss0"; >> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c >> index 0bc7daa7afc8..d42171ee1d61 100644 >> --- a/drivers/spi/spi-meson-spicc.c >> +++ b/drivers/spi/spi-meson-spicc.c >> @@ -21,6 +21,7 @@ >> #include <linux/types.h> >> #include <linux/interrupt.h> >> #include <linux/reset.h> >> +#include <linux/pinctrl/consumer.h> >> >> /* >> * The Meson SPICC controller could support DMA based transfers, but is not >> @@ -166,14 +167,31 @@ struct meson_spicc_device { >> unsigned long tx_remain; >> unsigned long rx_remain; >> unsigned long xfer_remain; >> + struct pinctrl *pinctrl; >> + struct pinctrl_state *pins_idle_high; >> + struct pinctrl_state *pins_idle_low; >> }; >> >> static void meson_spicc_oen_enable(struct meson_spicc_device *spicc) >> { >> u32 conf; >> >> - if (!spicc->data->has_oen) >> + if (!spicc->data->has_oen) { >> + /* Try to get pinctrl states for idle high/low */ >> + spicc->pins_idle_high = pinctrl_lookup_state(spicc->pinctrl, >> + "idle-high"); >> + if (IS_ERR(spicc->pins_idle_high)) { >> + dev_warn(&spicc->pdev->dev, "can't get idle-high pinctrl\n"); >> + spicc->pins_idle_high = NULL; >> + } >> + spicc->pins_idle_low = pinctrl_lookup_state(spicc->pinctrl, >> + "idle-low"); >> + if (IS_ERR(spicc->pins_idle_low)) { >> + dev_warn(&spicc->pdev->dev, "can't get idle-low pinctrl\n"); >> + spicc->pins_idle_low = NULL; >> + } >> return; >> + } >> >> conf = readl_relaxed(spicc->base + SPICC_ENH_CTL0) | >> SPICC_ENH_MOSI_OEN | SPICC_ENH_CLK_OEN | SPICC_ENH_CS_OEN; >> @@ -438,6 +456,16 @@ static int meson_spicc_prepare_message(struct spi_master *master, >> else >> conf &= ~SPICC_POL; >> >> + if (!spicc->data->has_oen) { >> + if (spi->mode & SPI_CPOL) { >> + if (spicc->pins_idle_high) >> + pinctrl_select_state(spicc->pinctrl, spicc->pins_idle_high); >> + } else { >> + if (spicc->pins_idle_low) >> + pinctrl_select_state(spicc->pinctrl, spicc->pins_idle_low); >> + } >> + } >> + >> if (spi->mode & SPI_CPHA) >> conf |= SPICC_PHA; >> else >> @@ -482,6 +510,9 @@ static int meson_spicc_unprepare_transfer(struct spi_master *master) >> >> device_reset_optional(&spicc->pdev->dev); >> >> + if (!spicc->data->has_oen) >> + pinctrl_select_default_state(&spicc->pdev->dev); >> + >> return 0; >> } >> >> @@ -733,6 +764,12 @@ static int meson_spicc_probe(struct platform_device *pdev) >> goto out_core_clk; >> } >> >> + spicc->pinctrl = devm_pinctrl_get(&pdev->dev); >> + if (IS_ERR(spicc->pinctrl)) { >> + ret = PTR_ERR(spicc->pinctrl); >> + goto out_clk; >> + } >> + >> device_reset_optional(&pdev->dev); >> >> master->num_chipselect = 4; >
Powered by blists - more mailing lists