| Message ID | 20220809172017.215412-3-aouledameur@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: amlogic: meson-spicc: Use pinctrl to drive CLK line when idle | expand |
On Tue 09 Aug 2022 at 19:20, Amjad Ouled-Ameur <aouledameur@baylibre.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@libre.computer> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.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 ? > @@ -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 ? > + }; > + }; > + > 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;
On 10/08/2022 10:52, Jerome Brunet wrote: > > On Tue 09 Aug 2022 at 19:20, Amjad Ouled-Ameur <aouledameur@baylibre.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@libre.computer> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.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; >
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 @@ -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; + }; + }; + 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;