| Message ID | 20221004-up-aml-fix-spi-v3-2-89de126fd163@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: amlogic: meson-spicc: Use pinctrl to drive CLK line when idle | expand |
Hi Amjad, On Wed, Oct 19, 2022 at 4:03 PM Amjad Ouled-Ameur <aouledameur@baylibre.com> wrote: [...] > 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; > + }; > + }; > + We typically have the .dts{,i} changes in a separate patch. I suggest doing the same here. I also have two questions about this part: - why are these not referenced by the SPICC controller node? - is there a particular reason why meson-gxl.dtsi is updated but meson-gxbb.dtsi is not? (my understanding is that GXBB is also lacking hardware OEN support) Best regards, Martin
On 19/10/2022 22:50, Martin Blumenstingl wrote: > Hi Amjad, > > On Wed, Oct 19, 2022 at 4:03 PM Amjad Ouled-Ameur > <aouledameur@baylibre.com> wrote: > [...] >> 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; >> + }; >> + }; >> + > We typically have the .dts{,i} changes in a separate patch. I suggest > doing the same here. > I also have two questions about this part: > - why are these not referenced by the SPICC controller node? Because it's up to the board to use or not those states, if some pull-up/downs are already present on the lines no need to use those special states. > - is there a particular reason why meson-gxl.dtsi is updated but > meson-gxbb.dtsi is not? (my understanding is that GXBB is also lacking > hardware OEN support) Good question indeed, so indeed they should be added in meson-gxbb.dtsi in a separate patch. > > > Best regards, > Martin Thanks, Neil
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 bad201510a99..ffea38e2339c 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 @@ -167,6 +168,9 @@ 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; }; #define pow2_clk_to_spicc(_div) container_of(_div, struct meson_spicc_device, pow2_div) @@ -175,8 +179,22 @@ 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; @@ -441,6 +459,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 @@ -487,6 +515,9 @@ static int meson_spicc_unprepare_transfer(struct spi_master *master) /* Set default configuration, keeping datarate field */ writel_relaxed(conf, spicc->base + SPICC_CONREG); + if (!spicc->data->has_oen) + pinctrl_select_default_state(&spicc->pdev->dev); + return 0; } @@ -798,6 +829,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;