* Re: [PATCH 1/1] arm64: dts: rockchip: enable onboard spi flash for rock-3a
From: Jonas Karlman @ 2024-04-08 14:56 UTC (permalink / raw)
To: Chukun Pan, Heiko Stuebner
Cc: Conor Dooley, Krzysztof Kozlowski, Rob Herring, linux-arm-kernel,
linux-rockchip, linux-kernel, devicetree
In-Reply-To: <421ab43b-ff92-41c6-9904-3c7681c926be@kwiboo.se>
Hi again,
On 2024-04-08 16:44, Jonas Karlman wrote:
> Hi Chukun,
>
> On 2024-04-08 14:40, Chukun Pan wrote:
>> There is a mx25u12835f spi flash on this board, enable it.
>>
>> [ 2.525805] spi-nor spi4.0: mx25u12835f (16384 Kbytes)
>>
>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
>> ---
>> arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
>> index a5e974ea659e..d8738cc47c73 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
>> @@ -757,6 +757,18 @@ &sdmmc2 {
>> status = "okay";
>> };
>>
>> +&sfc {
>
> This is missing:
>
> #address-cells = <1>;
> #size-cells = <0>;
>
>> + status = "okay";
>> +
>> + flash@0 {
>> + compatible = "jedec,spi-nor";
>> + reg = <0x0>;
>> + spi-max-frequency = <50000000>;
>
> At least in U-Boot the spi clock only support 24, 100 or 200 mhz and I
> am pretty sure the spi flash support 100mhz, so I would suggest you test
> with 100mhz, same as used on other rk356x boards.
Sorry, looked at spi clock instead of sfc clock.
sfc clock support 24, 50, 75, 100, 125 and 150 mhz.
Regards,
Jonas
>
> For U-Boot I have used 24 mhz for rk356x boards not defining a flash@0
> node in linux device tree, such as the rock-3a board.
>
> Regards,
> Jonas
>
>> + spi-rx-bus-width = <4>;
>> + spi-tx-bus-width = <1>;
>> + };
>> +};
>> +
>> &tsadc {
>> rockchip,hw-tshut-mode = <1>;
>> rockchip,hw-tshut-polarity = <0>;
>
^ permalink raw reply
* Re: [PATCH 1/1] arm64: dts: rockchip: enable onboard spi flash for rock-3a
From: Jonas Karlman @ 2024-04-08 14:44 UTC (permalink / raw)
To: Chukun Pan, Heiko Stuebner
Cc: Conor Dooley, Krzysztof Kozlowski, Rob Herring, linux-arm-kernel,
linux-rockchip, linux-kernel, devicetree
In-Reply-To: <20240408124005.182565-1-amadeus@jmu.edu.cn>
Hi Chukun,
On 2024-04-08 14:40, Chukun Pan wrote:
> There is a mx25u12835f spi flash on this board, enable it.
>
> [ 2.525805] spi-nor spi4.0: mx25u12835f (16384 Kbytes)
>
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> ---
> arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> index a5e974ea659e..d8738cc47c73 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> @@ -757,6 +757,18 @@ &sdmmc2 {
> status = "okay";
> };
>
> +&sfc {
This is missing:
#address-cells = <1>;
#size-cells = <0>;
> + status = "okay";
> +
> + flash@0 {
> + compatible = "jedec,spi-nor";
> + reg = <0x0>;
> + spi-max-frequency = <50000000>;
At least in U-Boot the spi clock only support 24, 100 or 200 mhz and I
am pretty sure the spi flash support 100mhz, so I would suggest you test
with 100mhz, same as used on other rk356x boards.
For U-Boot I have used 24 mhz for rk356x boards not defining a flash@0
node in linux device tree, such as the rock-3a board.
Regards,
Jonas
> + spi-rx-bus-width = <4>;
> + spi-tx-bus-width = <1>;
> + };
> +};
> +
> &tsadc {
> rockchip,hw-tshut-mode = <1>;
> rockchip,hw-tshut-polarity = <0>;
^ permalink raw reply
* Re: [PATCH v2 05/11] spi: cadence-qspi: add FIFO depth detection quirk
From: Mark Brown @ 2024-04-08 14:51 UTC (permalink / raw)
To: Théo Lebrun
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Vaishnav Achath,
Thomas Bogendoerfer, Rob Herring, linux-spi, devicetree,
linux-kernel, linux-mips, Vladimir Kondratiev, Gregory CLEMENT,
Thomas Petazzoni, Tawfik Bayouk
In-Reply-To: <D0ETH1AG1ONG.1M1FPSZM69H0Z@bootlin.com>
[-- Attachment #1: Type: text/plain, Size: 2658 bytes --]
On Mon, Apr 08, 2024 at 04:38:56PM +0200, Théo Lebrun wrote:
> On Mon Apr 8, 2024 at 4:10 PM CEST, Mark Brown wrote:
> > On Fri, Apr 05, 2024 at 05:02:15PM +0200, Théo Lebrun wrote:
> > > + if (ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) {
> > > + cqspi->fifo_depth = fifo_depth;
> > > + dev_dbg(dev, "using FIFO depth of %u\n", fifo_depth);
> > > + } else if (fifo_depth != cqspi->fifo_depth) {
> > > + dev_warn(dev, "detected FIFO depth (%u) different from config (%u)\n",
> > > + fifo_depth, cqspi->fifo_depth);
> > > + }
> > It's not obvious to me that we should ignore an explicitly specified
> > property if the quirk is present
> DT value isn't expected for compatibles with CQSPI_DETECT_FIFO_DEPTH
> quirk, therefore we do not ignore a specified property. Bindings agree:
> prop is false with EyeQ5 compatible.
Sure, but it's not obvious that that is the most helpful or constructive
way to handle things.
> > - if anything I'd more expect to see
> > the new warning in that case, possibly with a higher severity if we're
> > saying that the quirk means we're more confident that the data reported
> > by the hardware is reliable. I think what I'd expect is that we always
> > use an explicitly specified depth (hopefully the user was specifying it
> > for a reason?).
> The goal was a simpler devicetree on Mobileye platform. This is why we
> add this behavior flag. You prefer the property to be always present?
> This is a only a nice-to-have, you tell me what you prefer.
I would prefer that the property is always optional, or only required on
platforms where we know that the depth isn't probeable.
> I wasn't sure all HW behaved in the same way wrt read-only bits in
> SRAMPARTITION, and I do not have access to other platforms exploiting
> this driver. This is why I kept behavior reserved for EyeQ5-integrated
> IP block.
Well, if there's such little confidence that the depth is reported then
we shouldn't be logging an error.
> > Pulling all the above together can we just drop the quirk and always do
> > the detection, or leave the quirk as just controlling the severity with
> > which we log any difference between detected and explicitly configured
> > depths?
> If we do not simplify devicetree, then I'd vote for dropping this patch
> entirely. Adding code for detecting such an edge-case doesn't sound
> useful. Especially since this kind of error should only occur to people
> adding new hardware support; those probably do not need a nice
> user-facing error message. What do you think?
I'm confused why you think dropping the patch is a good idea?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 08/17] clk: samsung: gs101: add support for cmu_hsi2
From: André Draszik @ 2024-04-08 14:49 UTC (permalink / raw)
To: Peter Griffin, mturquette, sboyd, robh, krzk+dt, conor+dt, vkoul,
kishon, alim.akhtar, avri.altman, bvanassche, s.nawrocki,
cw00.choi, jejb, martin.petersen, chanho61.park, ebiggers
Cc: linux-scsi, linux-phy, devicetree, linux-clk, linux-samsung-soc,
linux-kernel, linux-arm-kernel, tudor.ambarus, saravanak,
willmcvicker
In-Reply-To: <20240404122559.898930-9-peter.griffin@linaro.org>
Hi Pete,
On Thu, 2024-04-04 at 13:25 +0100, Peter Griffin wrote:
> CMU_HSI2 is the clock management unit used for the hsi2 block.
> HSI stands for High Speed Interface and as such it generates
> clocks for PCIe, UFS and MMC card.
>
> This patch adds support for the muxes, dividers, and gates in
> cmu_hsi2.
>
> CLK_GOUT_HSI2_HSI2_CMU_HSI2_PCLK is marked as CLK_IS_CRITICAL
> as disabling it leads to an immediate system hang.
>
> CLK_GOUT_HSI2_SYSREG_HSI2_PCLK is also marked CLK_IS_CRITICAL.
> A hang is not observed with fine grained clock control, but
> UFS IP does not function with syscon controlling this clock
> just around hsi2_sysreg register accesses.
Would it make sense to add this clock to the &ufs_0 node in the DTS
instead? Seems more natural than a clock that's constantly enabled?
> [...]
>
> Updated regex for clock name mangling
> sed \
> -e 's|^PLL_LOCKTIME_PLL_\([^_]\+\)|fout_\L\1_pll|' \
> \
> -e 's|^PLL_CON0_MUX_CLKCMU_\([^_]\+\)_\(.*\)|mout_\L\1_\2|' \
> -e 's|^PLL_CON0_PLL_\(.*\)|mout_pll_\L\1|' \
> -e 's|^CLK_CON_MUX_MUX_CLK_\(.*\)|mout_\L\1|' \
> -e '/^PLL_CON[1-4]_[^_]\+_/d' \
> -e '/^[^_]\+_CMU_[^_]\+_CONTROLLER_OPTION/d' \
> -e '/^CLKOUT_CON_BLK_[^_]\+_CMU_[^_]\+_CLKOUT0/d' \
> \
> -e 's|_IPCLKPORT||' \
> -e 's|_RSTNSYNC||' \
> -e 's|_G4X2_DWC_PCIE_CTL||' \
> -e 's|_G4X1_DWC_PCIE_CTL||' \
> -e 's|_PCIE_SUB_CTRL||' \
> -e 's|_INST_0||g' \
> -e 's|_LN05LPE||' \
> -e 's|_TM_WRAPPER||' \
> -e 's|_SF||' \
> \
> -e 's|^CLK_CON_DIV_DIV_CLK_\([^_]\+\)_\(.*\)|dout_\L\1_\2|' \
> \
> -e 's|^CLK_CON_BUF_CLKBUF_\([^_]\+\)_\(.*\)|gout_\L\1_\2|' \
> -e 's|^CLK_CON_GAT_CLK_BLK_\([^_]\+\)_UID_\(.*\)|gout_\L\1_\2|' \
> -e 's|^gout_[^_]\+_[^_]\+_cmu_\([^_]\+\)_pclk$|gout_\1_\1_pclk|' \
> -e 's|^CLK_CON_GAT_GOUT_BLK_\([^_]\+\)_UID_\(.*\)|gout_\L\1_\2|' \
> -e 's|^CLK_CON_GAT_CLK_\([^_]\+\)_\(.*\)|gout_\L\1_clk_\L\1_\2|' \
> \
> -e '/^\(DMYQCH\|PCH\|QCH\|QUEUE\)_/d'
Thank you for the updated regex.
> ---
> drivers/clk/samsung/clk-gs101.c | 558 +++++++++++++++++++++++
> include/dt-bindings/clock/google,gs101.h | 63 +++
> 2 files changed, 621 insertions(+)
>
> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
> index d065e343a85d..b9f84c7d5c22 100644
> --- a/drivers/clk/samsung/clk-gs101.c
> +++ b/drivers/clk/samsung/clk-gs101.c
> @@ -22,6 +22,7 @@
> #define CLKS_NR_MISC (CLK_GOUT_MISC_XIU_D_MISC_ACLK + 1)
> #define CLKS_NR_PERIC0 (CLK_GOUT_PERIC0_SYSREG_PERIC0_PCLK + 1)
> #define CLKS_NR_PERIC1 (CLK_GOUT_PERIC1_SYSREG_PERIC1_PCLK + 1)
> +#define CLKS_NR_HSI2 (CLK_GOUT_HSI2_XIU_P_HSI2_ACLK + 1)
>
> /* ---- CMU_TOP ------------------------------------------------------------- */
>
> @@ -3409,6 +3410,560 @@ static const struct samsung_cmu_info peric1_cmu_info __initconst = {
> .clk_name = "bus",
> };
>
> +/* ---- CMU_HSI2 ---------------------------------------------------------- */
This comment is shorter that all the other similar comments in this file.
> [...]
> +
> +PNAME(mout_hsi2_bus_user_p) = { "oscclk", "dout_cmu_hsi2_bus" };
> +PNAME(mout_hsi2_pcie_user_p) = { "oscclk", "dout_cmu_hsi2_pcie" };
> +PNAME(mout_hsi2_ufs_embd_user_p) = { "oscclk", "dout_cmu_hsi2_ufs_embd" };
> +PNAME(mout_hsi2_mmc_card_user_p) = { "oscclk", "dout_cmu_hsi2_mmc_card" };
Can you make these alphabetical, too, please, which would also match their usage
below:
> +
> +static const struct samsung_mux_clock hsi2_mux_clks[] __initconst = {
> + MUX(CLK_MOUT_HSI2_BUS_USER, "mout_hsi2_bus_user", mout_hsi2_bus_user_p,
> + PLL_CON0_MUX_CLKCMU_HSI2_BUS_USER, 4, 1),
> + MUX(CLK_MOUT_HSI2_MMC_CARD_USER, "mout_hsi2_mmc_card_user",
> + mout_hsi2_mmc_card_user_p, PLL_CON0_MUX_CLKCMU_HSI2_MMC_CARD_USER,
> + 4, 1),
> + MUX(CLK_MOUT_HSI2_PCIE_USER, "mout_hsi2_pcie_user",
> + mout_hsi2_pcie_user_p, PLL_CON0_MUX_CLKCMU_HSI2_PCIE_USER,
> + 4, 1),
> + MUX(CLK_MOUT_HSI2_UFS_EMBD_USER, "mout_hsi2_ufs_embd_user",
> + mout_hsi2_ufs_embd_user_p, PLL_CON0_MUX_CLKCMU_HSI2_UFS_EMBD_USER,
> + 4, 1),
> +};
> +
> +static const struct samsung_gate_clock hsi2_gate_clks[] __initconst = {
> +
Here and below: all these extra empty lines are not needed.
> + GATE(CLK_GOUT_HSI2_PCIE_GEN4_1_PCIE_003_PHY_REFCLK_IN,
> + "gout_hsi2_pcie_gen4_1_pcie_003_phy_refclk_in",
> + "mout_hsi2_pcie_user",
> + CLK_CON_GAT_CLK_BLK_HSI2_UID_PCIE_GEN4_1_IPCLKPORT_PCIE_003_PCIE_SUB_CTRL_INST_0_PHY_REFCLK_IN,
> + 21, 0, 0),
> +
> + GATE(CLK_GOUT_HSI2_PCIE_GEN4_1_PCIE_004_PHY_REFCLK_IN,
> + "gout_hsi2_pcie_gen4_1_pcie_004_phy_refclk_in",
> + "mout_hsi2_pcie_user",
> + CLK_CON_GAT_CLK_BLK_HSI2_UID_PCIE_GEN4_1_IPCLKPORT_PCIE_004_PCIE_SUB_CTRL_INST_0_PHY_REFCLK_IN,
> + 21, 0, 0),
> +
> + GATE(CLK_GOUT_HSI2_SSMT_PCIE_IA_GEN4A_1_ACLK,
> + "gout_hsi2_ssmt_pcie_ia_gen4a_1_aclk",
> + "mout_hsi2_bus_user",
The two strings fit on the same line.
> + CLK_CON_GAT_CLK_BLK_HSI2_UID_SSMT_PCIE_IA_GEN4A_1_IPCLKPORT_ACLK,
> + 21, 0, 0),
> +
> + GATE(CLK_GOUT_HSI2_SSMT_PCIE_IA_GEN4A_1_PCLK,
> + "gout_hsi2_ssmt_pcie_ia_gen4a_1_pclk",
> + "mout_hsi2_bus_user",
dito.
> [...]
> + /* Disabling this clock makes the system hang. Mark the clock as critical. */
> + GATE(CLK_GOUT_HSI2_HSI2_CMU_HSI2_PCLK,
> + "gout_hsi2_hsi2_cmu_hsi2_pclk", "mout_hsi2_bus_user",
> + CLK_CON_GAT_GOUT_BLK_HSI2_UID_HSI2_CMU_HSI2_IPCLKPORT_PCLK,
> + 21, CLK_IS_CRITICAL, 0),
I have a similar clock in USB, which also causes a hang if off, I wonder what we
could do better here.
Cheers,
Andre'
^ permalink raw reply
* Re: [PATCH v2 05/11] spi: cadence-qspi: add FIFO depth detection quirk
From: Théo Lebrun @ 2024-04-08 14:45 UTC (permalink / raw)
To: Théo Lebrun, Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Vaishnav Achath,
Thomas Bogendoerfer, Rob Herring, linux-spi, devicetree,
linux-kernel, linux-mips, Vladimir Kondratiev, Gregory CLEMENT,
Thomas Petazzoni, Tawfik Bayouk
In-Reply-To: <D0ETH1AG1ONG.1M1FPSZM69H0Z@bootlin.com>
Hello,
On Mon Apr 8, 2024 at 4:38 PM CEST, Théo Lebrun wrote:
> Hello,
>
> On Mon Apr 8, 2024 at 4:10 PM CEST, Mark Brown wrote:
> > On Fri, Apr 05, 2024 at 05:02:15PM +0200, Théo Lebrun wrote:
> >
> > > Use hardware ability to read the FIFO depth thanks to
> > > CQSPI_REG_SRAMPARTITION that is partially read-only. Keep current
> > > behavior identical for existing compatibles.
> >
> > The behaviour is not identical here - we now unconditionally probe the
> > FIFO depth on all hardware, the difference with the quirk is that we
> > will ignore any DT property specifying the depth.
>
> You are correct of course. Wording is incorrect. I wanted to highlight
> that FIFO depth does not change for existing HW and still relies as
> before on devicetree value.
>
> > > - if (of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
> > > + if (!(ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) &&
> > > + of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
> > > dev_err(dev, "couldn't determine fifo-depth\n");
> >
> > It's not obvious from just the code that we do handle having a FIFO
> > depth property and detection in the detection code, at least a comment
> > would be good.
>
> I see. Will add comment or rework code to make more straight forward, or
> both.
>
> > > +static void cqspi_controller_detect_fifo_depth(struct cqspi_st *cqspi)
> > > +{
> > > + const struct cqspi_driver_platdata *ddata = cqspi->ddata;
> > > + struct device *dev = &cqspi->pdev->dev;
> > > + u32 reg, fifo_depth;
> > > +
> > > + /*
> > > + * Bits N-1:0 are writable while bits 31:N are read as zero, with 2^N
> > > + * the FIFO depth.
> > > + */
> > > + writel(U32_MAX, cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> > > + reg = readl(cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> > > + fifo_depth = reg + 1;
> > > +
> > > + if (ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) {
> > > + cqspi->fifo_depth = fifo_depth;
> > > + dev_dbg(dev, "using FIFO depth of %u\n", fifo_depth);
> > > + } else if (fifo_depth != cqspi->fifo_depth) {
> > > + dev_warn(dev, "detected FIFO depth (%u) different from config (%u)\n",
> > > + fifo_depth, cqspi->fifo_depth);
> > > + }
> >
> > It's not obvious to me that we should ignore an explicitly specified
> > property if the quirk is present
>
> DT value isn't expected for compatibles with CQSPI_DETECT_FIFO_DEPTH
> quirk, therefore we do not ignore a specified property. Bindings agree:
> prop is false with EyeQ5 compatible.
>
> > - if anything I'd more expect to see
> > the new warning in that case, possibly with a higher severity if we're
> > saying that the quirk means we're more confident that the data reported
> > by the hardware is reliable. I think what I'd expect is that we always
> > use an explicitly specified depth (hopefully the user was specifying it
> > for a reason?).
>
> The goal was a simpler devicetree on Mobileye platform. This is why we
> add this behavior flag. You prefer the property to be always present?
> This is a only a nice-to-have, you tell me what you prefer.
>
> I wasn't sure all HW behaved in the same way wrt read-only bits in
> SRAMPARTITION, and I do not have access to other platforms exploiting
> this driver. This is why I kept behavior reserved for EyeQ5-integrated
> IP block.
>
> > Pulling all the above together can we just drop the quirk and always do
> > the detection, or leave the quirk as just controlling the severity with
> > which we log any difference between detected and explicitly configured
> > depths?
>
> If we do not simplify devicetree, then I'd vote for dropping this patch
> entirely. Adding code for detecting such an edge-case doesn't sound
> useful. Especially since this kind of error should only occur to people
> adding new hardware support; those probably do not need a nice
> user-facing error message. What do you think?
Option you hinted at on dt-bindings patch sounds nice to my ears:
- Optional devicetree property;
- If present, check HW value and warn if different;
- If absent, use HW value.
This makes for a nice devicetree and simplifies driver code by removing
one quirk.
Sorry for delayed second thought.
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v2 08/11] spi: cadence-qspi: add early busywait to cqspi_wait_for_bit()
From: Théo Lebrun @ 2024-04-08 14:42 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Vaishnav Achath,
Thomas Bogendoerfer, Rob Herring, linux-spi, devicetree,
linux-kernel, linux-mips, Vladimir Kondratiev, Gregory CLEMENT,
Thomas Petazzoni, Tawfik Bayouk
In-Reply-To: <1f7087ad-824e-47fe-9953-ed5152c8f18f@sirena.org.uk>
Hello,
On Mon Apr 8, 2024 at 4:16 PM CEST, Mark Brown wrote:
> On Fri, Apr 05, 2024 at 05:02:18PM +0200, Théo Lebrun wrote:
>
> > If the CQSPI_BUSYWAIT_EARLY quirk flag is on, call
> > readl_relaxed_poll_timeout() with no sleep at the start of
> > cqspi_wait_for_bit(). If its short timeout expires, a sleeping
> > readl_relaxed_poll_timeout() call takes the relay.
> >
> > Behavior is hidden behind a quirk flag to keep the previous behavior the
> > same on all platforms.
> >
> > The reason is to avoid hrtimer interrupts on the system. All read
> > operations take less than 100µs.
>
> Why would this be platform specific, this seems like a very standard
> optimisation technique?
It does not make sense if you know that all read operations take more
than 100µs. I preferred being conservative. If you confirm it makes
sense I'll remove the quirk.
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v2 02/11] spi: dt-bindings: cdns,qspi-nor: sort compatibles alphabetically
From: Théo Lebrun @ 2024-04-08 14:41 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Vaishnav Achath,
Thomas Bogendoerfer, Rob Herring, linux-spi, devicetree,
linux-kernel, linux-mips, Vladimir Kondratiev, Gregory CLEMENT,
Thomas Petazzoni, Tawfik Bayouk
In-Reply-To: <37d89b59-d94e-47aa-8841-f2758b8e18b2@sirena.org.uk>
Hello,
On Mon Apr 8, 2024 at 4:14 PM CEST, Mark Brown wrote:
> On Fri, Apr 05, 2024 at 05:02:12PM +0200, Théo Lebrun wrote:
> > Compatibles are ordered by date of addition.
> > Switch to (deterministic) alphabetical ordering.
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> > Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> > index 5509c126b1cf..e53d443c6f93 100644
> > --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> > +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> > @@ -79,13 +79,13 @@ properties:
> > - items:
> > - enum:
> > - amd,pensando-elba-qspi
> > - - mobileye,eyeq5-ospi
> > - - ti,k2g-qspi
> > - - ti,am654-ospi
> > - intel,lgm-qspi
> > - - xlnx,versal-ospi-1.0
> > - intel,socfpga-qspi
> > + - mobileye,eyeq5-ospi
> > - starfive,jh7110-qspi
> > + - ti,am654-ospi
> > + - ti,k2g-qspi
> > + - xlnx,versal-ospi-1.0
>
> In general it's better to sort trivial cleanup patches like this before
> new functionality in order to avoid spurious dependencies.
It wasn't clear to me if this patch was desired. I therefore put it
afterwards to avoid conflicts if "spi: dt-bindings: cdns,qspi-nor: add
mobileye,eyeq5-ospi compatible" was applied.
Now that I know it is desired, I'll move it first in the series.
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles
From: Herve Codina @ 2024-04-08 14:40 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring, Frank Rowand,
Shawn Guo, Wolfram Sang, Mark Brown, Geert Uytterhoeven,
Rob Herring, linux-kernel, devicetree, Luca Ceresoli,
Thomas Petazzoni, stable, Android Kernel Team
In-Reply-To: <CAGETcx-oMbjtgW-sqzP6GPuM9BwgQrYJawpui3QMf1A-ETHpvg@mail.gmail.com>
Hi Sarava,
On Fri, 22 Mar 2024 19:00:03 -0700
Saravana Kannan <saravanak@google.com> wrote:
> On Thu, Mar 21, 2024 at 4:59 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Hi Saravana,
> >
> > On Mon, 4 Mar 2024 23:14:13 -0800
> > Saravana Kannan <saravanak@google.com> wrote:
> >
> > ...
> > >
> > > Thanks for the example. Let me think about this a bit on how we could
> > > fix this and get back to you.
> > >
> > > Please do ping me if I don't get back in a week or two.
> > >
> >
> > This is my ping.
> > Do you move forward ?
>
> Thanks for the ping. I thought about it a bit. I think the right fix
> it to undo the overlay fix I had suggested to Geert and then make the
> overlay code call __fw_devlink_pickup_dangling_consumers() on the
> parent device of the top level overlay nodes that get added that don't
> have a device created for them.
>
> I'll try to wrap up a patch for this on Monday. But if you want to
> take a shot at this, that's ok too.
>
I didn't see anything on this topic. Maybe I missed the related modifications.
Did you move forward on that patch ?
Best regards,
Hervé
^ permalink raw reply
* Re: [PATCH v2 05/11] spi: cadence-qspi: add FIFO depth detection quirk
From: Théo Lebrun @ 2024-04-08 14:38 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Vaishnav Achath,
Thomas Bogendoerfer, Rob Herring, linux-spi, devicetree,
linux-kernel, linux-mips, Vladimir Kondratiev, Gregory CLEMENT,
Thomas Petazzoni, Tawfik Bayouk
In-Reply-To: <551bea0a-1c9e-4e04-87db-c643fdaee85e@sirena.org.uk>
Hello,
On Mon Apr 8, 2024 at 4:10 PM CEST, Mark Brown wrote:
> On Fri, Apr 05, 2024 at 05:02:15PM +0200, Théo Lebrun wrote:
>
> > Use hardware ability to read the FIFO depth thanks to
> > CQSPI_REG_SRAMPARTITION that is partially read-only. Keep current
> > behavior identical for existing compatibles.
>
> The behaviour is not identical here - we now unconditionally probe the
> FIFO depth on all hardware, the difference with the quirk is that we
> will ignore any DT property specifying the depth.
You are correct of course. Wording is incorrect. I wanted to highlight
that FIFO depth does not change for existing HW and still relies as
before on devicetree value.
> > - if (of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
> > + if (!(ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) &&
> > + of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
> > dev_err(dev, "couldn't determine fifo-depth\n");
>
> It's not obvious from just the code that we do handle having a FIFO
> depth property and detection in the detection code, at least a comment
> would be good.
I see. Will add comment or rework code to make more straight forward, or
both.
> > +static void cqspi_controller_detect_fifo_depth(struct cqspi_st *cqspi)
> > +{
> > + const struct cqspi_driver_platdata *ddata = cqspi->ddata;
> > + struct device *dev = &cqspi->pdev->dev;
> > + u32 reg, fifo_depth;
> > +
> > + /*
> > + * Bits N-1:0 are writable while bits 31:N are read as zero, with 2^N
> > + * the FIFO depth.
> > + */
> > + writel(U32_MAX, cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> > + reg = readl(cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> > + fifo_depth = reg + 1;
> > +
> > + if (ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) {
> > + cqspi->fifo_depth = fifo_depth;
> > + dev_dbg(dev, "using FIFO depth of %u\n", fifo_depth);
> > + } else if (fifo_depth != cqspi->fifo_depth) {
> > + dev_warn(dev, "detected FIFO depth (%u) different from config (%u)\n",
> > + fifo_depth, cqspi->fifo_depth);
> > + }
>
> It's not obvious to me that we should ignore an explicitly specified
> property if the quirk is present
DT value isn't expected for compatibles with CQSPI_DETECT_FIFO_DEPTH
quirk, therefore we do not ignore a specified property. Bindings agree:
prop is false with EyeQ5 compatible.
> - if anything I'd more expect to see
> the new warning in that case, possibly with a higher severity if we're
> saying that the quirk means we're more confident that the data reported
> by the hardware is reliable. I think what I'd expect is that we always
> use an explicitly specified depth (hopefully the user was specifying it
> for a reason?).
The goal was a simpler devicetree on Mobileye platform. This is why we
add this behavior flag. You prefer the property to be always present?
This is a only a nice-to-have, you tell me what you prefer.
I wasn't sure all HW behaved in the same way wrt read-only bits in
SRAMPARTITION, and I do not have access to other platforms exploiting
this driver. This is why I kept behavior reserved for EyeQ5-integrated
IP block.
> Pulling all the above together can we just drop the quirk and always do
> the detection, or leave the quirk as just controlling the severity with
> which we log any difference between detected and explicitly configured
> depths?
If we do not simplify devicetree, then I'd vote for dropping this patch
entirely. Adding code for detecting such an edge-case doesn't sound
useful. Especially since this kind of error should only occur to people
adding new hardware support; those probably do not need a nice
user-facing error message. What do you think?
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* [PATCH v2 2/2] iio: adc: Add support for AD4000
From: Marcelo Schmitt @ 2024-04-08 14:31 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt, marcelo.schmitt1
Cc: linux-iio, devicetree, linux-kernel
In-Reply-To: <cover.1712585500.git.marcelo.schmitt@analog.com>
Add support for AD4000 family of low noise, low power, high speed,
successive aproximation register (SAR) ADCs.
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad4000.c | 649 +++++++++++++++++++++++++++++++++++++++
4 files changed, 663 insertions(+)
create mode 100644 drivers/iio/adc/ad4000.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 5dfe118a5dd3..86aa96115f5a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1165,6 +1165,7 @@ L: linux-iio@vger.kernel.org
S: Supported
W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
+F: drivers/iio/adc/ad4000.c
ANALOG DEVICES INC AD4130 DRIVER
M: Cosmin Tanislav <cosmin.tanislav@analog.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..9c9d13d4b74f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -21,6 +21,18 @@ config AD_SIGMA_DELTA
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
+config AD4000
+ tristate "Analog Devices AD4000 ADC Driver"
+ depends on SPI
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say yes here to build support for Analog Devices AD4000 high speed
+ SPI analog to digital converters (ADC).
+
+ To compile this driver as a module, choose M here: the module will be
+ called ad4000.
+
config AD4130
tristate "Analog Device AD4130 ADC Driver"
depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index edb32ce2af02..aa52068d864b 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -6,6 +6,7 @@
# When adding new entries keep the list in alphabetical order
obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
+obj-$(CONFIG_AD4000) += ad4000.o
obj-$(CONFIG_AD4130) += ad4130.o
obj-$(CONFIG_AD7091R) += ad7091r-base.o
obj-$(CONFIG_AD7091R5) += ad7091r5.o
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
new file mode 100644
index 000000000000..7997d9d98743
--- /dev/null
+++ b/drivers/iio/adc/ad4000.c
@@ -0,0 +1,649 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD4000 SPI ADC driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ */
+#include <asm/unaligned.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+#include <linux/units.h>
+#include <linux/util_macros.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+#define AD400X_READ_COMMAND 0x54
+#define AD400X_WRITE_COMMAND 0x14
+
+/* AD4000 Configuration Register programmable bits */
+#define AD4000_STATUS BIT(4) /* Status bits output */
+#define AD4000_SPAN_COMP BIT(3) /* Input span compression */
+#define AD4000_HIGHZ BIT(2) /* High impedance mode */
+#define AD4000_TURBO BIT(1) /* Turbo mode */
+
+#define AD4000_TQUIET2_NS 60
+
+#define AD4000_18BIT_MSK GENMASK(31, 14)
+#define AD4000_20BIT_MSK GENMASK(31, 12)
+
+#define AD4000_DIFF_CHANNEL(_sign, _real_bits) \
+ { \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .differential = 1, \
+ .channel = 0, \
+ .channel2 = 1, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
+ .scan_type = { \
+ .sign = _sign, \
+ .realbits = _real_bits, \
+ .storagebits = _real_bits > 16 ? 32 : 16, \
+ .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
+ .endianness = IIO_BE, \
+ }, \
+ } \
+
+#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits) \
+ { \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = 0, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_OFFSET), \
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
+ .scan_type = { \
+ .sign = _sign, \
+ .realbits = _real_bits, \
+ .storagebits = _real_bits > 16 ? 32 : 16, \
+ .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
+ .endianness = IIO_BE, \
+ }, \
+ } \
+
+enum ad4000_ids {
+ ID_AD4000,
+ ID_AD4001,
+ ID_AD4002,
+ ID_AD4003,
+ ID_AD4004,
+ ID_AD4005,
+ ID_AD4006,
+ ID_AD4007,
+ ID_AD4008,
+ ID_AD4010,
+ ID_AD4011,
+ ID_AD4020,
+ ID_AD4021,
+ ID_AD4022,
+ ID_ADAQ4001,
+ ID_ADAQ4003,
+};
+
+struct ad4000_chip_info {
+ const char *dev_name;
+ struct iio_chan_spec chan_spec;
+};
+
+static const struct ad4000_chip_info ad4000_chips[] = {
+ [ID_AD4000] = {
+ .dev_name = "ad4000",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+ },
+ [ID_AD4001] = {
+ .dev_name = "ad4001",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+ },
+ [ID_AD4002] = {
+ .dev_name = "ad4002",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+ },
+ [ID_AD4003] = {
+ .dev_name = "ad4003",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+ [ID_AD4004] = {
+ .dev_name = "ad4004",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+ },
+ [ID_AD4005] = {
+ .dev_name = "ad4005",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+ },
+ [ID_AD4006] = {
+ .dev_name = "ad4006",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+ },
+ [ID_AD4007] = {
+ .dev_name = "ad4007",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+ [ID_AD4008] = {
+ .dev_name = "ad4008",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+ },
+ [ID_AD4010] = {
+ .dev_name = "ad4010",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+ },
+ [ID_AD4011] = {
+ .dev_name = "ad4011",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+ [ID_AD4020] = {
+ .dev_name = "ad4020",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+ },
+ [ID_AD4021] = {
+ .dev_name = "ad4021",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+ },
+ [ID_AD4022] = {
+ .dev_name = "ad4022",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+ },
+ [ID_ADAQ4001] = {
+ .dev_name = "adaq4001",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+ },
+ [ID_ADAQ4003] = {
+ .dev_name = "adaq4003",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+};
+
+enum ad4000_gains {
+ AD4000_0454_GAIN = 0,
+ AD4000_0909_GAIN = 1,
+ AD4000_1_GAIN = 2,
+ AD4000_1900_GAIN = 3,
+ AD4000_GAIN_LEN
+};
+
+/*
+ * Gains stored and computed as fractions to avoid introducing rounding errors.
+ */
+static const int ad4000_gains_frac[AD4000_GAIN_LEN][2] = {
+ [AD4000_0454_GAIN] = { 227, 500 },
+ [AD4000_0909_GAIN] = { 909, 1000 },
+ [AD4000_1_GAIN] = { 1, 1 },
+ [AD4000_1900_GAIN] = { 19, 10 },
+};
+
+struct ad4000_state {
+ struct spi_device *spi;
+ struct gpio_desc *cnv_gpio;
+ int vref;
+ bool status_bits;
+ bool span_comp;
+ bool turbo_mode;
+ bool high_z_mode;
+
+ enum ad4000_gains pin_gain;
+ int scale_tbl[AD4000_GAIN_LEN][2][2];
+
+ /*
+ * DMA (thus cache coherency maintenance) requires the
+ * transfer buffers to live in their own cache lines.
+ */
+ struct {
+ union {
+ u16 sample_buf16;
+ u32 sample_buf32;
+ } data;
+ s64 timestamp __aligned(8);
+ } scan;
+ __be16 tx_buf __aligned(IIO_DMA_MINALIGN);
+ __be16 rx_buf;
+};
+
+static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits,
+ const struct ad4000_chip_info *chip)
+{
+ int diff = chip->chan_spec.differential;
+ int val, val2, tmp0, tmp1, i;
+ u64 tmp2;
+
+ val2 = scale_bits;
+ for (i = 0; i < AD4000_GAIN_LEN; i++) {
+ val = st->vref / 1000;
+ /* Multiply by MILLI here to avoid losing precision */
+ val = mult_frac(val, ad4000_gains_frac[i][1] * MILLI,
+ ad4000_gains_frac[i][0]);
+ /* Would multiply by NANO here but we already multiplied by MILLI */
+ tmp2 = shift_right((u64)val * MICRO, val2);
+ tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1);
+ /* Store scale for when span compression is disabled */
+ st->scale_tbl[i][0][0] = tmp0; /* Integer part */
+ st->scale_tbl[i][0][1] = abs(tmp1); /* Fractional part */
+ /* Store scale for when span compression is enabled */
+ st->scale_tbl[i][1][0] = tmp0;
+ if (diff)
+ st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
+ else
+ st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
+ }
+}
+
+static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
+{
+ put_unaligned_be16(AD400X_WRITE_COMMAND << BITS_PER_BYTE | val,
+ &st->tx_buf);
+ return spi_write(st->spi, &st->tx_buf, 2);
+}
+
+static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
+{
+ struct spi_transfer t[] = {
+ {
+ .tx_buf = &st->tx_buf,
+ .rx_buf = &st->rx_buf,
+ .len = 2,
+ },
+ };
+ int ret;
+
+ put_unaligned_be16(AD400X_READ_COMMAND << BITS_PER_BYTE, &st->tx_buf);
+ ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
+ if (ret < 0)
+ return ret;
+
+ *val = get_unaligned_be16(&st->rx_buf);
+
+ return ret;
+}
+
+static int ad4000_read_sample(struct ad4000_state *st,
+ const struct iio_chan_spec *chan)
+{
+ struct spi_transfer t[] = {
+ {
+ .rx_buf = &st->scan.data,
+ .len = BITS_TO_BYTES(chan->scan_type.storagebits),
+ .delay = {
+ .value = AD4000_TQUIET2_NS,
+ .unit = SPI_DELAY_UNIT_NSECS,
+ },
+ },
+ };
+ int ret;
+
+ ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int ad4000_single_conversion(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan, int *val)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+ u32 sample;
+ int ret;
+
+ if (st->cnv_gpio)
+ gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_HIGH);
+
+ ret = ad4000_read_sample(st, chan);
+ if (ret)
+ return ret;
+
+ if (st->cnv_gpio)
+ gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_LOW);
+
+ if (chan->scan_type.storagebits > 16)
+ sample = get_unaligned_be32(&st->scan.data);
+ else
+ sample = get_unaligned_be16(&st->scan.data);
+
+ switch (chan->scan_type.realbits) {
+ case 16:
+ break;
+ case 18:
+ sample = FIELD_GET(AD4000_18BIT_MSK, sample);
+ break;
+ case 20:
+ sample = FIELD_GET(AD4000_20BIT_MSK, sample);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (chan->scan_type.sign == 's')
+ *val = sign_extend32(sample, chan->scan_type.realbits - 1);
+
+ return IIO_VAL_INT;
+}
+
+static int ad4000_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long info)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return ad4000_single_conversion(indio_dev, chan, val);
+ unreachable();
+ case IIO_CHAN_INFO_SCALE:
+ *val = st->scale_tbl[st->pin_gain][st->span_comp][0];
+ *val2 = st->scale_tbl[st->pin_gain][st->span_comp][1];
+ return IIO_VAL_INT_PLUS_NANO;
+ case IIO_CHAN_INFO_OFFSET:
+ *val = 0;
+ if (st->span_comp)
+ *val = mult_frac(st->vref / 1000, 1, 10);
+
+ return IIO_VAL_INT;
+ default:
+ break;
+ }
+
+ return -EINVAL;
+}
+
+static int ad4000_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long info)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_CHAN_INFO_SCALE:
+ *vals = (int *)st->scale_tbl[st->pin_gain];
+ *length = 2 * 2;
+ *type = IIO_VAL_INT_PLUS_NANO;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ return IIO_VAL_INT_PLUS_NANO;
+ default:
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
+ return -EINVAL;
+}
+
+static int ad4000_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val, int val2,
+ long mask)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+ unsigned int reg_val;
+ bool span_comp_en;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ ret = ad4000_read_reg(st, ®_val);
+ if (ret < 0)
+ return ret;
+
+ span_comp_en = (val2 == st->scale_tbl[st->pin_gain][1][1]);
+ reg_val &= ~AD4000_SPAN_COMP;
+ reg_val |= FIELD_PREP(AD4000_SPAN_COMP, span_comp_en);
+
+ ret = ad4000_write_reg(st, reg_val);
+ if (ret < 0)
+ return ret;
+
+ st->span_comp = span_comp_en;
+ return 0;
+ }
+ unreachable();
+ default:
+ break;
+ }
+
+ return -EINVAL;
+}
+
+static irqreturn_t ad4000_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct ad4000_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (st->cnv_gpio)
+ gpiod_set_value(st->cnv_gpio, GPIOD_OUT_HIGH);
+
+ ret = ad4000_read_sample(st, &indio_dev->channels[0]);
+ if (ret < 0)
+ goto err_out;
+
+ if (st->cnv_gpio)
+ gpiod_set_value(st->cnv_gpio, GPIOD_OUT_LOW);
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
+ iio_get_time_ns(indio_dev));
+
+err_out:
+ iio_trigger_notify_done(indio_dev->trig);
+ return IRQ_HANDLED;
+}
+
+static const struct iio_info ad4000_info = {
+ .read_raw = &ad4000_read_raw,
+ .read_avail = &ad4000_read_avail,
+ .write_raw = &ad4000_write_raw,
+ .write_raw_get_fmt = &ad4000_write_raw_get_fmt,
+};
+
+static void ad4000_config(struct ad4000_state *st)
+{
+ unsigned int reg_val;
+ int ret;
+
+ reg_val = FIELD_PREP(AD4000_TURBO, 1);
+
+ if (device_property_present(&st->spi->dev, "adi,high-z-input"))
+ reg_val |= FIELD_PREP(AD4000_HIGHZ, 1);
+
+ /*
+ * The ADC SDI pin might be connected to controller CS line in which
+ * case the write might fail. This, however, does not prevent the device
+ * from functioning even though in a configuration other than the
+ * requested one.
+ */
+ ret = ad4000_write_reg(st, reg_val);
+ if (ret < 0)
+ dev_dbg(&st->spi->dev, "Failed to config device\n");
+}
+
+static void ad4000_regulator_disable(void *reg)
+{
+ regulator_disable(reg);
+}
+
+static int ad4000_probe(struct spi_device *spi)
+{
+ const struct ad4000_chip_info *chip;
+ struct regulator *vref_reg;
+ struct iio_dev *indio_dev;
+ struct ad4000_state *st;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ chip = spi_get_device_match_data(spi);
+ if (!chip)
+ return -EINVAL;
+
+ st = iio_priv(indio_dev);
+ st->spi = spi;
+
+ ret = devm_regulator_get_enable(&spi->dev, "vdd");
+ if (ret)
+ return dev_err_probe(&spi->dev, ret, "Failed to enable VDD supply\n");
+
+ ret = devm_regulator_get_enable(&spi->dev, "vio");
+ if (ret)
+ return dev_err_probe(&spi->dev, ret, "Failed to enable VIO supply\n");
+
+ vref_reg = devm_regulator_get(&spi->dev, "ref");
+ if (IS_ERR(vref_reg))
+ return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
+ "Failed to get vref regulator\n");
+
+ ret = regulator_enable(vref_reg);
+ if (ret < 0)
+ return dev_err_probe(&spi->dev, ret,
+ "Failed to enable voltage regulator\n");
+
+ ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable, vref_reg);
+ if (ret)
+ return dev_err_probe(&spi->dev, ret,
+ "Failed to add regulator disable action\n");
+
+ st->vref = regulator_get_voltage(vref_reg);
+ if (st->vref < 0)
+ return dev_err_probe(&spi->dev, st->vref, "Failed to get vref\n");
+
+ st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv", GPIOD_OUT_HIGH);
+ if (IS_ERR(st->cnv_gpio)) {
+ if (PTR_ERR(st->cnv_gpio) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
+ "Failed to get CNV GPIO");
+ }
+
+ ad4000_config(st);
+
+ indio_dev->name = chip->dev_name;
+ indio_dev->info = &ad4000_info;
+ indio_dev->channels = &chip->chan_spec;
+ indio_dev->num_channels = 1;
+
+ st->pin_gain = AD4000_1_GAIN;
+ if (device_property_present(&spi->dev, "adi,gain-milli")) {
+ u32 val;
+
+ ret = device_property_read_u32(&spi->dev, "adi,gain-milli", &val);
+ if (ret)
+ return ret;
+
+ switch (val) {
+ case 454:
+ st->pin_gain = AD4000_0454_GAIN;
+ break;
+ case 909:
+ st->pin_gain = AD4000_0909_GAIN;
+ break;
+ case 1000:
+ st->pin_gain = AD4000_1_GAIN;
+ break;
+ case 1900:
+ st->pin_gain = AD4000_1900_GAIN;
+ break;
+ default:
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "Invalid firmware provided gain\n");
+ }
+ }
+
+ /*
+ * ADCs that output twos complement code have one less bit to express
+ * voltage magnitude.
+ */
+ if (chip->chan_spec.scan_type.sign == 's')
+ ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits - 1,
+ chip);
+ else
+ ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits,
+ chip);
+
+ ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+ &iio_pollfunc_store_time,
+ &ad4000_trigger_handler, NULL);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id ad4000_id[] = {
+ { "ad4000", (kernel_ulong_t)&ad4000_chips[ID_AD4000] },
+ { "ad4001", (kernel_ulong_t)&ad4000_chips[ID_AD4001] },
+ { "ad4002", (kernel_ulong_t)&ad4000_chips[ID_AD4002] },
+ { "ad4003", (kernel_ulong_t)&ad4000_chips[ID_AD4003] },
+ { "ad4004", (kernel_ulong_t)&ad4000_chips[ID_AD4004] },
+ { "ad4005", (kernel_ulong_t)&ad4000_chips[ID_AD4005] },
+ { "ad4006", (kernel_ulong_t)&ad4000_chips[ID_AD4006] },
+ { "ad4007", (kernel_ulong_t)&ad4000_chips[ID_AD4007] },
+ { "ad4008", (kernel_ulong_t)&ad4000_chips[ID_AD4008] },
+ { "ad4010", (kernel_ulong_t)&ad4000_chips[ID_AD4010] },
+ { "ad4011", (kernel_ulong_t)&ad4000_chips[ID_AD4011] },
+ { "ad4020", (kernel_ulong_t)&ad4000_chips[ID_AD4020] },
+ { "ad4021", (kernel_ulong_t)&ad4000_chips[ID_AD4021] },
+ { "ad4022", (kernel_ulong_t)&ad4000_chips[ID_AD4022] },
+ { "adaq4001", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4001] },
+ { "adaq4003", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4003] },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, ad4000_id);
+
+static const struct of_device_id ad4000_of_match[] = {
+ { .compatible = "adi,ad4000", .data = &ad4000_chips[ID_AD4000] },
+ { .compatible = "adi,ad4001", .data = &ad4000_chips[ID_AD4001] },
+ { .compatible = "adi,ad4002", .data = &ad4000_chips[ID_AD4002] },
+ { .compatible = "adi,ad4003", .data = &ad4000_chips[ID_AD4003] },
+ { .compatible = "adi,ad4004", .data = &ad4000_chips[ID_AD4004] },
+ { .compatible = "adi,ad4005", .data = &ad4000_chips[ID_AD4005] },
+ { .compatible = "adi,ad4006", .data = &ad4000_chips[ID_AD4006] },
+ { .compatible = "adi,ad4007", .data = &ad4000_chips[ID_AD4007] },
+ { .compatible = "adi,ad4008", .data = &ad4000_chips[ID_AD4008] },
+ { .compatible = "adi,ad4010", .data = &ad4000_chips[ID_AD4010] },
+ { .compatible = "adi,ad4011", .data = &ad4000_chips[ID_AD4011] },
+ { .compatible = "adi,ad4020", .data = &ad4000_chips[ID_AD4020] },
+ { .compatible = "adi,ad4021", .data = &ad4000_chips[ID_AD4021] },
+ { .compatible = "adi,ad4022", .data = &ad4000_chips[ID_AD4022] },
+ { .compatible = "adi,adaq4001", .data = &ad4000_chips[ID_ADAQ4001] },
+ { .compatible = "adi,adaq4003", .data = &ad4000_chips[ID_ADAQ4003] },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ad4000_of_match);
+
+static struct spi_driver ad4000_driver = {
+ .driver = {
+ .name = "ad4000",
+ .of_match_table = ad4000_of_match,
+ },
+ .probe = ad4000_probe,
+ .id_table = ad4000_id,
+};
+module_spi_driver(ad4000_driver);
+
+MODULE_AUTHOR("Mircea Caprioru <mircea.caprioru@analog.com>");
+MODULE_AUTHOR("Marcelo Schmitt <marcelo.schmitt@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD4000 ADC driver");
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000
From: Marcelo Schmitt @ 2024-04-08 14:31 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt, marcelo.schmitt1
Cc: linux-iio, devicetree, linux-kernel
In-Reply-To: <cover.1712585500.git.marcelo.schmitt@analog.com>
Add device tree documentation for AD4000 family of ADC devices.
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
.../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++++++++++++++
MAINTAINERS | 7 +
2 files changed, 208 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
new file mode 100644
index 000000000000..ca06afb5149e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
@@ -0,0 +1,201 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4000 and similar Analog to Digital Converters
+
+maintainers:
+ - Marcelo Schmitt <marcelo.schmitt@analog.com>
+
+description: |
+ Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
+ Specifications can be found at:
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - adi,ad4000
+ - adi,ad4001
+ - adi,ad4002
+ - adi,ad4003
+ - adi,ad4004
+ - adi,ad4005
+ - adi,ad4006
+ - adi,ad4007
+ - adi,ad4008
+ - adi,ad4010
+ - adi,ad4011
+ - adi,ad4020
+ - adi,ad4021
+ - adi,ad4022
+ - adi,adaq4001
+ - adi,adaq4003
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
+
+ spi-cpha: true
+
+ adi,spi-mode:
+ $ref: /schemas/types.yaml#/definitions/string
+ enum: [ single, chain ]
+ description: |
+ This property indicates the SPI wiring configuration.
+
+ When this property is omitted, it is assumed that the device is using what
+ the datasheet calls "4-wire mode". This is the conventional SPI mode used
+ when there are multiple devices on the same bus. In this mode, the CNV
+ line is used to initiate the conversion and the SDI line is connected to
+ CS on the SPI controller.
+
+ When this property is present, it indicates that the device is using one
+ of the following alternative wiring configurations:
+
+ * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
+ definition of 3-wire mode is NOT at all related to the standard
+ spi-3wire property!) This mode is often used when the ADC is the only
+ device on the bus. In this mode, SDI is tied to VIO, and the CNV line
+ can be connected to the CS line of the SPI controller or to a GPIO, in
+ which case the CS line of the controller is unused.
+ * chain: The datasheet calls this "chain mode". This mode is used to save
+ on wiring when multiple ADCs are used. In this mode, the SDI line of
+ one chip is tied to the SDO of the next chip in the chain and the SDI of
+ the last chip in the chain is tied to GND. Only the first chip in the
+ chain is connected to the SPI bus. The CNV line of all chips are tied
+ together. The CS line of the SPI controller can be used as the CNV line
+ only if it is active high.
+
+ '#daisy-chained-devices': true
+
+ vdd-supply:
+ description: A 1.8V supply that powers the chip (VDD).
+
+ vio-supply:
+ description:
+ A 1.8V to 5.5V supply for the digital inputs and outputs (VIO).
+
+ ref-supply:
+ description:
+ A 2.5 to 5V supply for the external reference voltage (REF).
+
+ cnv-gpios:
+ description:
+ The Convert Input (CNV). This input has multiple functions. It initiates
+ the conversions and selects the SPI mode of the device (chain or CS). In
+ 'single' mode, this property is omitted if the CNV pin is connected to the
+ CS line of the SPI controller. If 'single' mode is selected and this GPIO
+ is provided, it must be active low.
+ maxItems: 1
+
+ adi,high-z-input:
+ type: boolean
+ description:
+ High-Z mode allows the amplifier and RC filter in front of the ADC to be
+ chosen based on the signal bandwidth of interest, rather than the settling
+ requirements of the switched capacitor SAR ADC inputs.
+
+ adi,gain-milli:
+ description: |
+ The hardware gain applied to the ADC input (in milli units).
+ The gain provided by the ADC input scaler is defined by the hardware
+ connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-.
+ If not present, default to 1000 (no actual gain applied).
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [454, 909, 1000, 1900]
+ default: 1000
+
+ interrupts:
+ description:
+ The SDO pin can also function as a busy indicator. This node should be
+ connected to an interrupt that is triggered when the SDO line goes low
+ while the SDI line is high and the CNV line is low ('single' mode) or the
+ SDI line is low and the CNV line is high ('multi' mode); or when the SDO
+ line goes high while the SDI and CNV lines are high (chain mode),
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - spi-cpha
+ - vdd-supply
+ - vio-supply
+ - ref-supply
+
+allOf:
+ # in '4-wire' mode, cnv-gpios is required, for other modes it is optional
+ - if:
+ not:
+ required:
+ - adi,spi-mode
+ then:
+ required:
+ - cnv-gpios
+ # chain mode has lower SCLK max rate
+ - if:
+ required:
+ - adi,spi-mode
+ properties:
+ adi,spi-mode:
+ const: chain
+ then:
+ properties:
+ spi-max-frequency:
+ maximum: 50000000 # for VIO > 2.7 V, 40000000 for VIO > 1.7 V
+ required:
+ - '#daisy-chained-devices'
+ else:
+ properties:
+ '#daisy-chained-devices': false
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ /* Example for a AD devices */
+ adc@0 {
+ compatible = "adi,ad4020";
+ reg = <0>;
+ spi-cpha;
+ spi-max-frequency = <71000000>;
+ vdd-supply = <&supply_1_8V>;
+ vio-supply = <&supply_1_8V>;
+ ref-supply = <&supply_5V>;
+ cnv-gpios = <&gpio0 88 GPIO_ACTIVE_HIGH>;
+ };
+ };
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ /* Example for a ADAQ devices */
+ adc@0 {
+ compatible = "adi,adaq4003";
+ reg = <0>;
+ spi-cpha;
+ adi,spi-mode = "single";
+ spi-max-frequency = <80000000>;
+ vdd-supply = <&supply_1_8V>;
+ vio-supply = <&supply_1_8V>;
+ ref-supply = <&supply_5V>;
+ adi,high-z-input;
+ adi,gain-milli = <454>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index a7287cf44869..5dfe118a5dd3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1159,6 +1159,13 @@ W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
F: drivers/iio/dac/ad3552r.c
+ANALOG DEVICES INC AD4000 DRIVER
+M: Marcelo Schmitt <marcelo.schmitt@analog.com>
+L: linux-iio@vger.kernel.org
+S: Supported
+W: https://ez.analog.com/linux-software-drivers
+F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
+
ANALOG DEVICES INC AD4130 DRIVER
M: Cosmin Tanislav <cosmin.tanislav@analog.com>
L: linux-iio@vger.kernel.org
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/2] Add support for AD4000 series
From: Marcelo Schmitt @ 2024-04-08 14:30 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt, marcelo.schmitt1
Cc: linux-iio, devicetree, linux-kernel
This is more like an RFC patch set since configuration read/write is currently
buggy.
Change log v1 -> v2:
- Took device tree provided by David.
- Dropped ABI additions in favor of device tree properties.
- Set differential IIO channel subtype for differential ADCs.
- Set scan_type shift bits to mask out correct real bits from buffer.
- Added __aligned(8) to buffer timestamp.
- Used union to reduce buffer memory usage for 16-bit devices.
- Used SPI transfer functions rather than SPI message.
- Used c99 style structure initialization.
- Used iio_device_claim_direct_scoped().
- Removed unneeded pointer casts.
- Added other power supplies (VDD and VIO).
Link to v1: https://lore.kernel.org/linux-iio/cover.1711131830.git.marcelo.schmitt@analog.com/
Additional topics:
- Why there is no different handling for the different SPI wiring modes?
It looks like there is no need for different handling of "4-wire" and "3-wire"
modes.
If in "4-wire" (dt default mode), SDI is connected to SPI controller CS and
CNV is active high. We can activate the CNV GPIO then let the SPI controller
bring CS (connected to SDI) down when starting the transfer.
If in "3-wire" (dt single mode), if we have a CNV (active low) GPIO we activate
it and then proceed with with the transfer. If controller CS is connected to
CNV it works the same way.
I'm thinking it's better if we can support these devices in similar way
other SPI ADCs are supported. Does that make sense?
To me, the "3-wire" mode with controller CS to ADC CNV is what most resembles
conventional SPI. The only important distinction is that the
controller must be able to keep ADC SDI line high during conversions.
Although, while the spi-engine implementation provided to me can keep SDI up
during conversions, I'm not sure its a thing all SPI controllers can do.
I tried a raspberry pi 4 some time ago and it was leaving the SDI line low if
no tx buffer was provided. Even with a tx full of 1s the controller would
bring SDI down between each 8 bits of transfer.
Anyway, single-shot and buffered reads work with the spi-engine controller
with ADC in "3-wire"/single mode with controller CS line connected to ADC CNV
pin which is how I've been testing it.
- Why did not make vref regulator optional?
Other SAR ADCs I've seen needed a voltage reference otherwise they simply
could not provide any reasonable readings. Isn't it preferable to fail rather
than having a device that can't provide reliable data?
- Why did not split into AD and ADAQ patches?
The main difference between AD and ADAQ is the amplifier in front of the ADC.
If only supporting AD, we could probably avoid the scale table since it would
only have two possible values per ADC. But then the handling of span compression
scale would need refactoring to be in the scale table when adding ADAQ.
I'm not excited to implement something knowing it will need rework in the
following patch. Will do if required.
- Span compression and offset.
For non-differential ADCs, enabling the span compression requires an input offset.
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/AD4000-4004-4008.pdf
page 18
and
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
page 19
I updated the _offset attribute for those ADCs according to span compression
being enabled or not. Is it okay to have an attribute update cause an update to
another one?
Maybe also make the span compression a dt property and have it fixed after probe?
- Configuration register
Despite it doing single-shot and buffered captures, read and writes to the
configuration register are currently buggy. It is as if the register was
"floating". I tried setting up buffers like ad7768-1, adxl355_core, bma220_spi,
bma400_core, and mcp3911.
Thanks,
Marcelo
Marcelo Schmitt (2):
dt-bindings: iio: adc: Add AD4000
iio: adc: Add support for AD4000
.../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++
MAINTAINERS | 8 +
drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad4000.c | 649 ++++++++++++++++++
5 files changed, 871 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
create mode 100644 drivers/iio/adc/ad4000.c
--
2.43.0
^ permalink raw reply
* Re: [PATCH 3/4] arm64: dts: qcom: sc8280xp-lenovo-thinkpad-x13s: add USB-C orientation GPIOs
From: Konrad Dybcio @ 2024-04-08 14:26 UTC (permalink / raw)
To: Dmitry Baryshkov, Bjorn Andersson, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20240408-hdk-orientation-gpios-v1-3-8064ba43e52a@linaro.org>
On 4/8/24 04:34, Dmitry Baryshkov wrote:
> Define the USB-C orientation GPIOs so that the USB-C ports orientation
> is known without having to resort to the altmode notifications.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> index 15ae94c1602d..2806aa8ec497 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> @@ -100,6 +100,8 @@ pmic-glink {
>
> #address-cells = <1>;
> #size-cells = <0>;
> + orientation-gpios = <&tlmm 166 GPIO_ACTIVE_HIGH>,
> + <&tlmm 49 GPIO_ACTIVE_HIGH>;
These numbers do check out
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
^ permalink raw reply
* Re: [PATCH v2 08/11] spi: cadence-qspi: add early busywait to cqspi_wait_for_bit()
From: Mark Brown @ 2024-04-08 14:16 UTC (permalink / raw)
To: Théo Lebrun
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Vaishnav Achath,
Thomas Bogendoerfer, Rob Herring, linux-spi, devicetree,
linux-kernel, linux-mips, Vladimir Kondratiev, Gregory CLEMENT,
Thomas Petazzoni, Tawfik Bayouk
In-Reply-To: <20240405-cdns-qspi-mbly-v2-8-956679866d6d@bootlin.com>
[-- Attachment #1: Type: text/plain, Size: 612 bytes --]
On Fri, Apr 05, 2024 at 05:02:18PM +0200, Théo Lebrun wrote:
> If the CQSPI_BUSYWAIT_EARLY quirk flag is on, call
> readl_relaxed_poll_timeout() with no sleep at the start of
> cqspi_wait_for_bit(). If its short timeout expires, a sleeping
> readl_relaxed_poll_timeout() call takes the relay.
>
> Behavior is hidden behind a quirk flag to keep the previous behavior the
> same on all platforms.
>
> The reason is to avoid hrtimer interrupts on the system. All read
> operations take less than 100µs.
Why would this be platform specific, this seems like a very standard
optimisation technique?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 02/11] spi: dt-bindings: cdns,qspi-nor: sort compatibles alphabetically
From: Mark Brown @ 2024-04-08 14:14 UTC (permalink / raw)
To: Théo Lebrun
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Vaishnav Achath,
Thomas Bogendoerfer, Rob Herring, linux-spi, devicetree,
linux-kernel, linux-mips, Vladimir Kondratiev, Gregory CLEMENT,
Thomas Petazzoni, Tawfik Bayouk
In-Reply-To: <20240405-cdns-qspi-mbly-v2-2-956679866d6d@bootlin.com>
[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]
On Fri, Apr 05, 2024 at 05:02:12PM +0200, Théo Lebrun wrote:
> Compatibles are ordered by date of addition.
> Switch to (deterministic) alphabetical ordering.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
> Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> index 5509c126b1cf..e53d443c6f93 100644
> --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> @@ -79,13 +79,13 @@ properties:
> - items:
> - enum:
> - amd,pensando-elba-qspi
> - - mobileye,eyeq5-ospi
> - - ti,k2g-qspi
> - - ti,am654-ospi
> - intel,lgm-qspi
> - - xlnx,versal-ospi-1.0
> - intel,socfpga-qspi
> + - mobileye,eyeq5-ospi
> - starfive,jh7110-qspi
> + - ti,am654-ospi
> + - ti,k2g-qspi
> + - xlnx,versal-ospi-1.0
In general it's better to sort trivial cleanup patches like this before
new functionality in order to avoid spurious dependencies.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 01/11] spi: dt-bindings: cdns,qspi-nor: add mobileye,eyeq5-ospi compatible
From: Mark Brown @ 2024-04-08 14:13 UTC (permalink / raw)
To: Théo Lebrun
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Vaishnav Achath,
Thomas Bogendoerfer, Rob Herring, linux-spi, devicetree,
linux-kernel, linux-mips, Vladimir Kondratiev, Gregory CLEMENT,
Thomas Petazzoni, Tawfik Bayouk, Krzysztof Kozlowski
In-Reply-To: <20240405-cdns-qspi-mbly-v2-1-956679866d6d@bootlin.com>
[-- Attachment #1: Type: text/plain, Size: 538 bytes --]
On Fri, Apr 05, 2024 at 05:02:11PM +0200, Théo Lebrun wrote:
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: mobileye,eyeq5-ospi
> + then:
> + properties:
> + cdns,fifo-depth: false
> + else:
> + required:
> + - cdns,fifo-depth
My suggestions on the FIFO depth probe patch would mean this would turn
into making cdns,fifo-depth optional for everything. It certainly seems
like many instances of the hardware should support that anyway.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] dt-bindings: net: rockchip-dwmac: use rgmii-id in example
From: Dragan Simic @ 2024-04-08 14:13 UTC (permalink / raw)
To: Sascha Hauer
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
David Wu, netdev, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel, Andrew Lunn
In-Reply-To: <20240408-rockchip-dwmac-rgmii-id-binding-v1-1-3886d1a8bd54@pengutronix.de>
Hello Sascha,
On 2024-04-08 08:44, Sascha Hauer wrote:
> The dwmac supports specifying the RGMII clock delays, but it is
> recommended to use rgmii-id and to specify the delays in the phy node
> instead [1].
>
> Change the example accordingly to no longer promote this undesired
> setting.
>
> [1]
> https://lore.kernel.org/all/1a0de7b4-f0f7-4080-ae48-f5ffa9e76be3@lunn.ch/
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
I'd suggest that the following link is also added to the patch
description as a reference, because it contains rather useful
information:
https://lore.kernel.org/linux-rockchip/2973a2cb1f478031ae6d478c853c33ae@manjaro.org/
Otherwise,
Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> ---
> Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> index 70bbc4220e2ac..6bbe96e352509 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> @@ -137,8 +137,6 @@ examples:
> assigned-clock-parents = <&ext_gmac>;
>
> rockchip,grf = <&grf>;
> - phy-mode = "rgmii";
> + phy-mode = "rgmii-id";
> clock_in_out = "input";
> - tx_delay = <0x30>;
> - rx_delay = <0x10>;
> };
>
> ---
> base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f
> change-id: 20240408-rockchip-dwmac-rgmii-id-binding-6166af659845
>
> Best regards,
^ permalink raw reply
* Re: [PATCH v2 05/11] spi: cadence-qspi: add FIFO depth detection quirk
From: Mark Brown @ 2024-04-08 14:10 UTC (permalink / raw)
To: Théo Lebrun
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Vaishnav Achath,
Thomas Bogendoerfer, Rob Herring, linux-spi, devicetree,
linux-kernel, linux-mips, Vladimir Kondratiev, Gregory CLEMENT,
Thomas Petazzoni, Tawfik Bayouk
In-Reply-To: <20240405-cdns-qspi-mbly-v2-5-956679866d6d@bootlin.com>
[-- Attachment #1: Type: text/plain, Size: 2371 bytes --]
On Fri, Apr 05, 2024 at 05:02:15PM +0200, Théo Lebrun wrote:
> Use hardware ability to read the FIFO depth thanks to
> CQSPI_REG_SRAMPARTITION that is partially read-only. Keep current
> behavior identical for existing compatibles.
The behaviour is not identical here - we now unconditionally probe the
FIFO depth on all hardware, the difference with the quirk is that we
will ignore any DT property specifying the depth.
> - if (of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
> + if (!(ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) &&
> + of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
> dev_err(dev, "couldn't determine fifo-depth\n");
It's not obvious from just the code that we do handle having a FIFO
depth property and detection in the detection code, at least a comment
would be good.
> +static void cqspi_controller_detect_fifo_depth(struct cqspi_st *cqspi)
> +{
> + const struct cqspi_driver_platdata *ddata = cqspi->ddata;
> + struct device *dev = &cqspi->pdev->dev;
> + u32 reg, fifo_depth;
> +
> + /*
> + * Bits N-1:0 are writable while bits 31:N are read as zero, with 2^N
> + * the FIFO depth.
> + */
> + writel(U32_MAX, cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> + reg = readl(cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> + fifo_depth = reg + 1;
> +
> + if (ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) {
> + cqspi->fifo_depth = fifo_depth;
> + dev_dbg(dev, "using FIFO depth of %u\n", fifo_depth);
> + } else if (fifo_depth != cqspi->fifo_depth) {
> + dev_warn(dev, "detected FIFO depth (%u) different from config (%u)\n",
> + fifo_depth, cqspi->fifo_depth);
> + }
It's not obvious to me that we should ignore an explicitly specified
property if the quirk is present - if anything I'd more expect to see
the new warning in that case, possibly with a higher severity if we're
saying that the quirk means we're more confident that the data reported
by the hardware is reliable. I think what I'd expect is that we always
use an explicitly specified depth (hopefully the user was specifying it
for a reason?).
Pulling all the above together can we just drop the quirk and always do
the detection, or leave the quirk as just controlling the severity with
which we log any difference between detected and explicitly configured
depths?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] dt-bindings: net: rockchip-dwmac: use rgmii-id in example
From: Andrew Lunn @ 2024-04-08 14:02 UTC (permalink / raw)
To: Sascha Hauer
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
David Wu, netdev, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
In-Reply-To: <20240408-rockchip-dwmac-rgmii-id-binding-v1-1-3886d1a8bd54@pengutronix.de>
On Mon, Apr 08, 2024 at 08:44:10AM +0200, Sascha Hauer wrote:
> The dwmac supports specifying the RGMII clock delays, but it is
> recommended to use rgmii-id and to specify the delays in the phy node
> instead [1].
>
> Change the example accordingly to no longer promote this undesired
> setting.
>
> [1] https://lore.kernel.org/all/1a0de7b4-f0f7-4080-ae48-f5ffa9e76be3@lunn.ch/
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 0/5] Add parsing for Zimop ISA extension
From: Andrew Jones @ 2024-04-08 13:56 UTC (permalink / raw)
To: Clément Léger
Cc: Deepak Gupta, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Conor Dooley, Rob Herring, Krzysztof Kozlowski,
Anup Patel, Shuah Khan, Atish Patra, linux-doc, linux-riscv,
linux-kernel, devicetree, kvm, kvm-riscv, linux-kselftest
In-Reply-To: <89d4a24c-db24-487b-8c5c-bdc1fa2d42b4@rivosinc.com>
On Mon, Apr 08, 2024 at 01:19:39PM +0200, Clément Léger wrote:
>
>
> On 08/04/2024 13:03, Andrew Jones wrote:
> > On Mon, Apr 08, 2024 at 10:01:12AM +0200, Clément Léger wrote:
> >>
> >>
> >> On 05/04/2024 19:33, Deepak Gupta wrote:
> >>> On Fri, Apr 5, 2024 at 8:26 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >>>>
> >>>> On Thu, Apr 04, 2024 at 12:32:46PM +0200, Clément Léger wrote:
> >>>>> The Zimop ISA extension was ratified recently. This series adds support
> >>>>> for parsing it from riscv,isa, hwprobe export and kvm support for
> >>>>> Guest/VM.
> >>>>
> >>>> I'm not sure we need this. Zimop by itself isn't useful, so I don't know
> >>>> if we need to advertise it at all. When an extension comes along that
> >>>> redefines some MOPs, then we'll advertise that extension, but the fact
> >>>> Zimop is used for that extension is really just an implementation detail.
> >>>
> >>> Only situation I see this can be useful is this:--
> >>>
> >>> An implementer, implemented Zimops in CPU solely for the purpose that they can
> >>> run mainline distro & packages on their hardware and don't want to leverage any
> >>> feature which are built on top of Zimop.
> >>
> >> Yes, the rationale was that some binaries using extensions that overload
> >> MOPs could still be run. With Zimop exposed, the loader could determine
> >> if the binary can be executed without potentially crashing. We could
> >> also let the program run anyway but the execution could potentially
> >> crash unexpectedly, which IMHO is not really good for the user
> >> experience nor for debugging. I already think that the segfaults which
> >> happens when executing binaries that need some missing extension are not
> >> so easy to debug, so better add more guards.
> >
> > OK. It's only one more extension out of dozens, so I won't complain more,
>
> No worries, your point *is* valid since I'm not sure yet that the loader
> will actually do that one day.
>
> BTW, are you aware of any effort to make the elf dynamic loader
> "smarter" and actually check for needed extensions to be present rather
> than blindly running the elf and potentially catching SIGILL ?
Jeff Law told me a bit about FMV (function multiversioning). I don't know
much about this, but, from what he's told me, it sounds like there will be
an ifunc resolver which invokes hwprobe to determine which variants are
possible/best to use, so it should be possible to avoid SIGILL by always
having a basic variant.
Thanks,
drew
>
> Thanks,
>
> Clément
>
> > but I was thinking that binaries that use particular extensions would
> > check for those particular extensions (step 2), rather than Zimop.
> >
> > Thanks,
> > drew
> >
> >>
> >>>
> >>> As an example zicfilp and zicfiss are dependent on zimops. glibc can
> >>> do following
> >>>
> >>> 1) check elf header if binary was compiled with zicfiss and zicfilp,
> >>> if yes goto step 2, else goto step 6.
> >>> 2) check if zicfiss/zicfilp is available in hw via hwprobe, if yes
> >>> goto step 5. else goto step 3
> >>> 3) check if zimop is available via hwprobe, if yes goto step 6, else goto step 4
> >>
> >> I think you meant step 5 rather than step 6.
> >>
> >> Clément
> >>
> >>> 4) This binary won't be able to run successfully on this platform,
> >>> issue exit syscall. <-- termination
> >>> 5) issue prctl to enable shadow stack and landing pad for current task
> >>> <-- enable feature
> >>> 6) let the binary run <-- let the binary run because no harm can be done
^ permalink raw reply
* Re: [PATCH net-next v2 0/9] Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface
From: Parthiban.Veerasooran @ 2024-04-08 13:41 UTC (permalink / raw)
To: benjamin
Cc: netdev, devicetree, linux-kernel, linux-doc, Horatiu.Vultur,
Woojung.Huh, Nicolas.Ferre, UNGLinuxDriver, Thorsten.Kummermehr,
davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
conor+dt, corbet, Steen.Hegelund, rdunlap, horms, casper.casan,
andrew
In-Reply-To: <d495577d97a5e7aa809d3e9a90ef6057404782f1.camel@bigler.one>
Hi Benjamin,
On 04/04/24 3:10 am, Benjamin Bigler wrote:
> [Some people who received this message don't often get email from benjamin@bigler.one. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Parthiban,
>
> Sorry for the late answer, I was quite busy the last few days.
No problem.
>
> On Mon, 2024-03-25 at 13:24 +0000, Parthiban.Veerasooran@microchip.com wrote:
>> Hi Benjamin Bigler,
>>
>> Thank you for your testing and feedback. It would be really helpful to
>> bring the driver to a good shape. We really appreciate your efforts on this.
>>
>> On 24/03/24 5:25 pm, Benjamin Bigler wrote:
>>> [Some people who received this message don't often get email from benjamin@bigler.one. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi Parthiban
>>>
>>> I hope I send this in the right context as it is not related to just one patch or
>>> some specific code.
>>>
>>> I conducted UDP load testing using three i.MX8MM boards in conjunction with the
>>> LAN8651. The setup involved one board functioning as a server, which is just
>>> echoing back received data, while the remaining two boards acted as clients,
>>> sending UDP packets of different sizes in various bursts to the server.
>>> Due to hardware constraints, the SPI bus speed was limited to 15 MHz, which might
>>> have influenced the results.
>>>
>>> During the tests I experienced some issues:
>>>
>>> - The boards just start receiving after first sending something (ping another board).
>>> Some measurements showed that the irq stays asserted after init. This makes sense
>>> as far as I understand the chapter 7.7 of the specification, the irq is deasserted
>>> on reception of the first data header following CSn being asserted. As a workaround
>>> I trigger the thread at the end of oa_tc6_init.
>> It looks like the IRQ is asserted on RESET completion and expects a data
>> chunk from host to deassert the IRQ. I used to test the driver in RPI 4
>> using iperf3. For some reason I never faced this issue, may be when the
>> network device is being registered there might be some packet
>> transmission which leads to deliver a data chunk so that the IRQ is
>> deasserted. Thanks for the workaround. I think that would be the
>> solution to solve this issue. Adding the below lines in the end of the
>> function oa_tc6_init() will trigger the oa_tc6_spi_thread_handler() to
>> perform an empty data chunk transfer which will deassert the IRQ before
>> starting the actual data transfer.
>
> I have ipv6 disabled and use static ipv4 addresses. That could be the reason why on
> my side no packet is sent.
>
>>
>> /* oa_tc6_sw_reset_macphy() function resets and clears the MAC-PHY reset
>> * complete status. IRQ is also asserted on reset completion and it is
>> * remain asserted until MAC-PHY receives a data chunk. So performing an
>> * empty data chunk transmission will deassert the IRQ. Refer section
>> * 7.7 and 9.2.8.8 in the OPEN Alliance specification for more details.
>> */
>> tc6->int_flag = true;
>> wake_up_interruptible(&tc6->spi_wq);
>
> Perfect, thats the same I added and also works on my side.
>
>>>
>>> - If there is a lot of traffic, the receive buffer overflow error spams the log.
>>>
>>> - If there is a lot of traffic, I got various kernel panics in oa_tc6_update_rx_skb.
>>> Mostly because more data to rx_skb is added than allocated and sometimes because
>>> rx_skb is null in oa_tc6_update_rx_skb or oa_tc6_prcs_rx_frame_end. Some debugging
>>> with a logic analyzer showed that the chip is not behave correctly. There is more
>>> bytes between start_valid and end_valid than there should be. Also there
>>> seems to be 2 end_valid without a start_valid between. What is common is that the incorrect
>>> frame starts in a chunk where end_valid and start_valid is set.
>>> In my opinion its a problem in the chip (maybe related to the errata in the next point)
>>> but the driver should be resilent and just drop the packet and not cause a kernel panic.
>> Usually I run into this issue "receive buffer overflow" when I run RPI 4
>> with default cpu governor setting which is "ondemand". In this case,
>> even though if I set SPI clock speed as 15 MHz the RPI 4 core clock is
>> clocking down when it is idle which leads delivering half of the
>> configured SPI clock speed around 5.9 MHz. So the systems like RPI 4
>> need performance mode enabled to get the proper clock speed for SPI.
>> Refer below link for more details.
>>
>> https://github.com/raspberrypi/linux/issues/3381#issuecomment-1144723750
>>
>> I used to enable performance mode using the below command.
>>
>> echo performance | sudo tee
>> /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor > /dev/null
>>
>> So please ensure the SPI clock speed using a logic analyzer to get the
>> maximum throughput without receive buffer overflow.
>>
>> Of course, I agree that the driver should not crash in case of receive
>> buffer overflow. By referring your investigations, I understand that the
>> buffers in the MAC-PHY is being continuously overwritten again and again
>> as the host is very slow to read the data from the MAC-PHY buffers
>> through SPI which alters the descriptors. There might be two reasons why
>> we run into this situation.
>> 1. The host is busy doing something else and delays to initiate SPI even
>> though SPI clock speed is 15 MHz.
>> 2. The SPI clock speed is less than 15 MHz.
>
> Sorry there is a missunderstanding between us. The receive buffer overflow is not
> causing any harm except filling the log. In my setup I get in one day about 35000
> entries. I am not sure if its appropriate to log these errors.
>
> The SPI Frequency is at 14.8 MHz. If I just have 2 boards connected, I am not able
> to reproduce this. Only with 3 boards when 2 boards sends multiple big ethernet
> frames (1512 byte per Frame) to one, I get these log entries.
> The latency seems to be quite low, from IRQ to start reading first frame it takes
> always less than 500us. Also the boards are just running the udp test.
>
>>
>> I use the below iperf3 setup for my testing and never faced the driver
>> crash issue even though faced "receive buffer overflow" error when I run
>> RPI 4 with "ondemand" default mode.
>>
>> Node 0 - Raspberry Pi 4 with LAN8650 MAC-PHY
>> $ iperf3 -s
>> Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick
>> $ iperf3 -c 192.168.5.100 -u -b 10M -i 1 -t 0
>>
>> and vice versa.
>>
>> I never faced "receive buffer overflow" error when I run RPI 4 with
>> "performance" mode enabled and even though all the cores are stressed
>> using the below command,
>>
>> $ yes >/dev/null & yes >/dev/null & yes >/dev/null & yes >/dev/null &
>>
>> Can you share more details about your testing setup and applications you
>> use, so that I will try to reproduce the issue in my setup to debug the
>> driver?
>
> I use a internal tool which does some stress tests using udp. Unfortunately,
> I am not allowed to publish it, but a colleague works on a rust implementation,
> which we can publish, but its not fully ready yet.
> On one board the tool is running in server mode. It just echoes back the received
> data. On the 2 other boards the tool is running in client mode. It sends various
> sized udp-packets in different bursts and then checks if it receives the same
> data in the same order.
>
>
> The crashes only happens when ZARFE is not set (with Rev B0). When the crash
> happens, I see on the logic analyzer that there are more bytes than mtu + headers
> between the frame where start_valid is set and the frame where end_valid is set.
> Then this happens:
Thanks for all the above details. I will include this ZARFE fix in the
next version v4 which I am going to post soon.
>
> [ 437.155673] skbuff: skb_over_panic: text:ffff80007a8c2bd8 len:1600 put:64 head:ffff00000de28080
> data:ffff00000de280c0 tail:0x680 end:0x640 dev:eth1
> [ 437.168987] kernel BUG at net/core/skbuff.c:192!
> [ 437.173612] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
> [ 437.180407] Modules linked in: ppp_async crc_ccitt ppp_generic slhc lan865x oa_tc6 bec_infoo(O)
> tpm_tis_spi tpm_tis_core spi_imx imx_sdma
> [ 437.196016] CPU: 1 PID: 455 Comm: oa-tc6-spi-thre Tainted: G O 6.6.11-
> gce336e2c2bc3-dirty #1
> [ 437.205853] Hardware name: Toradex Verdin iMX8M Mini on FUMU (DT)
> [ 437.212820] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 437.219790] pc : skb_panic+0x58/0x5c
> [ 437.223376] lr : skb_panic+0x58/0x5c
> [ 437.226959] sp : ffff80008362bd90
> [ 437.230278] x29: ffff80008362bda0 x28: 0000000000000000 x27: ffff000001066878
> [ 437.237426] x26: 000000000000001e x25: 00000000000007f8 x24: ffff0000010cea80
> [ 437.244571] x23: 00000000f0f0f0f1 x22: 000000000000001f x21: 0000000000000000
> [ 437.251720] x20: ffff0000010ceaa8 x19: 000000003f20003f x18: ffffffffffffffff
> [ 437.258867] x17: ffff7ffffded9000 x16: ffff800080008000 x15: 073a0764076e0765
> [ 437.266015] x14: 0720073007380736 x13: ffff8000823d1f58 x12: 0000000000000534
> [ 437.273162] x11: 00000000000001bc x10: ffff800082429f58 x9 : ffff8000823d1f58
> [ 437.280310] x8 : 00000000ffffefff x7 : ffff800082429f58 x6 : 0000000000000000
> [ 437.287455] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 0000000000000000
> [ 437.294606] x2 : 0000000000000000 x1 : ffff000001223b00 x0 : 0000000000000087
> [ 437.301753] Call trace:
> [ 437.304203] skb_panic+0x58/0x5c
> [ 437.307436] skb_find_text+0x0/0xf0
> [ 437.310933] oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
> [ 437.316523] kthread+0x118/0x11c
> [ 437.319758] ret_from_fork+0x10/0x20
> [ 437.323343] Code: f90007e9 b940b908 f90003e8 97ca3c34 (d4210000)
> [ 437.329446] ---[ end trace 0000000000000000 ]---
>
>
> Sometimes there are 2 end_valid after eachother without a start_valid between.
> Then this happens:
>
> [ 469.737297] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000074
> [ 469.746137] Mem abort info:
> [ 469.748950] ESR = 0x0000000096000004
> [ 469.752709] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 469.758036] SET = 0, FnV = 0
> [ 469.761098] EA = 0, S1PTW = 0
> [ 469.764252] FSC = 0x04: level 0 translation fault
> [ 469.769144] Data abort info:
> [ 469.772033] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> [ 469.777529] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [ 469.782594] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [ 469.787921] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000043c32000
> [ 469.794377] [0000000000000074] pgd=0000000000000000, p4d=0000000000000000
> [ 469.801184] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> [ 469.807459] Modules linked in: ppp_async crc_ccitt ppp_generic slhc lan865x oa_tc6 bec_infoo(O)
> tpm_tis_spi tpm_tis_core spi_imx imx_sdma
> [ 469.823064] CPU: 2 PID: 456 Comm: oa-tc6-spi-thre Tainted: G O 6.6.11-
> g350ed394a6ca-dirty #1
> [ 469.832903] Hardware name: Toradex Verdin iMX8M Mini on FUMU (DT)
> [ 469.839871] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 469.846841] pc : skb_put+0xc/0x6c
> [ 469.850169] lr : oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
> [ 469.856106] sp : ffff80008376bdb0
> [ 469.859424] x29: ffff80008376bdb0 x28: 0000000000000000 x27: ffff00000194c080
> [ 469.866573] x26: 0000000000000000 x25: 0000000000000000 x24: ffff000001095c80
> [ 469.873720] x23: 00000000f0f0f0f1 x22: 000000000000001f x21: 0000000000000000
> [ 469.880870] x20: ffff000001095ca8 x19: 000000003f20003f x18: 0000000000000000
> [ 469.888023] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
> [ 469.895174] x14: 0000031acf8b86d8 x13: 0000000000000000 x12: 0000000000000000
> [ 469.902321] x11: 0000000000000002 x10: 0000000000000a60 x9 : ffff80008376b970
> [ 469.909467] x8 : ffff00007fb6e580 x7 : 000000000194b080 x6 : 0000000000000000
> [ 469.916616] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 000000000000fc80
> [ 469.923765] x2 : 0000000000000001 x1 : 0000000000000040 x0 : 0000000000000000
> [ 469.930915] Call trace:
> [ 469.933365] skb_put+0xc/0x6c
> [ 469.936342] oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
> [ 469.941929] kthread+0x118/0x11c
> [ 469.945166] ret_from_fork+0x10/0x20
> [ 469.948752] Code: d65f03c0 d503233f a9bf7bfd 910003fd (b9407406)
> [ 469.954854] ---[ end trace 0000000000000000 ]---
>
>
> If interested I can try to get a recording with the logic analyzer and send it to you.
I don't think it is needed.
>
> By the way in the other answer you attached a screenshot of the logic analyzer and you
> have a very nice HLA for oa_tc6. Are they open-source or are there any plans to publish them?
It is already available in the Microchip's github page for public.
Checkout the below link for the same.
https://github.com/MicrochipTech/oa-tc6-saleae-extension
>
>>>
>>> - Sometimes the chip stops working. It always asserts the irq but there is no data (rca=0)
>>> and also exst is not active. I found out that there is an errata (DS80001075) point s3
>>> that explains this. I set the ZARFE bit in CONFIG0. This also fixes the point above.
>>> The driver now works since about 2.5 weeks with various load with just one loss of frame
>>> error where I had to reboot the system after about 4 days.
>> It is good to hear that the driver works fine with the above changes. As
>> mentioned in the errata, this continuous interrupt issue is a known
>> issue with LAN8651 Rev.B0. Switching to LAN8651 Rev.B1 will solve this
>> issue and no need of any workaround. Setting ZARFE bit in the CONFIG0
>> will solve the continuous interrupt issue but don't know how the above
>> "receive buffer overflow" issue also solved. I think it is a good idea
>> to test with LAN8651 Rev.B1 without setting ZARFE bit once. It would be
>> interesting to see the result. I am always using LAN8651 Rev.B1 for my
>> testing.
>
> Unfortunately I just have LAN8651 Rev. B0 Chips. Are you sure that the Rev B1 has the
> issue fixed? The errata here says that B1 is affected too:
> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/Errata/LAN8650-1-Errata-80001075.pdf
As per my knowledge it is fixed in the Rev.B1 but as you said errata
says the issue persists in both revisions. Let me check internally and
get back to you on this. But it is always recommended to use Rev.B1
rather Rev.B0. If possible, I would suggest to use the latest one.
Best regards,
Parthiban V
>
>>
>> I should be able to reproduce the "receive buffer overflow" issue and
>> consequently kernel crash in my setup with LAN8651 Rev.B1 so that I can
>> investigate the issue further. As I am not able to reproduce in my RPI
>> 4, I need your support for the tests and applications you used in your
>> setup.
>>>
>>> Is there a reason why you removed the netdev watchdog which was active in v2?
>> When the timeout occurs, there is no further action except increasing
>> tx_errors. Not seeing this except USB-to-Ethernet which can be removed
>> unexpectedly. But this is SPI interface which will not be removed
>> unexpectedly as it is a platform device. That's why we removed this.
>>
>> Best regards,
>> Parthiban V
>>>
>>> Thanks,
>>> Benjamin Bigler
>>>
>>
>
> Thanks,
> Benjamin Bigler
>
^ permalink raw reply
* [PATCH v20 9/9] usb: dwc3: qcom: Add multiport suspend/resume support for wrapper
From: Krishna Kurapati @ 2024-04-08 13:29 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Bjorn Andersson, Wesley Cheng,
Konrad Dybcio, Greg Kroah-Hartman, Conor Dooley, Thinh Nguyen,
Felipe Balbi, Johan Hovold
Cc: devicetree, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap,
quic_jackp, Krishna Kurapati, Johan Hovold
In-Reply-To: <20240408132925.1880571-1-quic_kriskura@quicinc.com>
Power event IRQ is used for wakeup in cases:
a) where the controller is super speed capable and missing an
ss_phy interrupt.
b) where the GIC is not capable of detecting DP/DM hs phy irq's.
Power event IRQ stat register indicates whether high speed phy
entered and exited L2 successfully during suspend and resume.
Indicate the same for all ports of multiport.
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
---
drivers/usb/dwc3/dwc3-qcom.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 12182e0f8f45..d3e6d5d5e8bf 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -52,6 +52,13 @@
#define APPS_USB_AVG_BW 0
#define APPS_USB_PEAK_BW MBps_to_icc(40)
+static const u32 pwr_evnt_irq_stat_reg[DWC3_MAX_PORTS] = {
+ 0x58,
+ 0x1dc,
+ 0x228,
+ 0x238,
+};
+
struct dwc3_qcom_port {
int qusb2_phy_irq;
int dp_hs_phy_irq;
@@ -421,9 +428,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
if (qcom->is_suspended)
return 0;
- val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
- if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
- dev_err(qcom->dev, "HS-PHY not in L2\n");
+ for (i = 0; i < qcom->num_ports; i++) {
+ val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg[i]);
+ if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
+ dev_err(qcom->dev, "port-%d HS-PHY not in L2\n", i + 1);
+ }
for (i = qcom->num_clocks - 1; i >= 0; i--)
clk_disable_unprepare(qcom->clks[i]);
@@ -472,8 +481,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
/* Clear existing events from PHY related to L2 in/out */
- dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
- PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
+ for (i = 0; i < qcom->num_ports; i++) {
+ dwc3_qcom_setbits(qcom->qscratch_base,
+ pwr_evnt_irq_stat_reg[i],
+ PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
+ }
qcom->is_suspended = false;
--
2.34.1
^ permalink raw reply related
* [PATCH v20 8/9] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport
From: Krishna Kurapati @ 2024-04-08 13:29 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Bjorn Andersson, Wesley Cheng,
Konrad Dybcio, Greg Kroah-Hartman, Conor Dooley, Thinh Nguyen,
Felipe Balbi, Johan Hovold
Cc: devicetree, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap,
quic_jackp, Krishna Kurapati, Johan Hovold
In-Reply-To: <20240408132925.1880571-1-quic_kriskura@quicinc.com>
DWC3 Qcom wrapper currently supports only wakeup configuration
for single port controllers. Read speed of each port connected
to the controller and enable wakeup for each of them accordingly.
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
---
drivers/usb/dwc3/dwc3-qcom.c | 71 +++++++++++++++++++++---------------
1 file changed, 41 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 35eb338514bc..12182e0f8f45 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -57,6 +57,7 @@ struct dwc3_qcom_port {
int dp_hs_phy_irq;
int dm_hs_phy_irq;
int ss_phy_irq;
+ enum usb_device_speed usb2_speed;
};
struct dwc3_qcom {
@@ -68,7 +69,6 @@ struct dwc3_qcom {
struct reset_control *resets;
struct dwc3_qcom_port ports[DWC3_MAX_PORTS];
u8 num_ports;
- enum usb_device_speed usb2_speed;
struct extcon_dev *edev;
struct extcon_dev *host_edev;
@@ -307,7 +307,7 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
return dwc->xhci;
}
-static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
+static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, int port_index)
{
struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
struct usb_device *udev;
@@ -318,14 +318,8 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
*/
hcd = platform_get_drvdata(dwc->xhci);
- /*
- * It is possible to query the speed of all children of
- * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code
- * currently supports only 1 port per controller. So
- * this is sufficient.
- */
#ifdef CONFIG_USB
- udev = usb_hub_find_child(hcd->self.root_hub, 1);
+ udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
#else
udev = NULL;
#endif
@@ -356,26 +350,26 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
disable_irq_nosync(irq);
}
-static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
+static void dwc3_qcom_disable_port_interrupts(struct dwc3_qcom_port *port)
{
- dwc3_qcom_disable_wakeup_irq(qcom->ports[0].qusb2_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(port->qusb2_phy_irq);
- if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq);
- } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
- (qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq);
+ if (port->usb2_speed == USB_SPEED_LOW) {
+ dwc3_qcom_disable_wakeup_irq(port->dm_hs_phy_irq);
+ } else if ((port->usb2_speed == USB_SPEED_HIGH) ||
+ (port->usb2_speed == USB_SPEED_FULL)) {
+ dwc3_qcom_disable_wakeup_irq(port->dp_hs_phy_irq);
} else {
- dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq);
- dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(port->dp_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(port->dm_hs_phy_irq);
}
- dwc3_qcom_disable_wakeup_irq(qcom->ports[0].ss_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(port->ss_phy_irq);
}
-static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
+static void dwc3_qcom_enable_port_interrupts(struct dwc3_qcom_port *port)
{
- dwc3_qcom_enable_wakeup_irq(qcom->ports[0].qusb2_phy_irq, 0);
+ dwc3_qcom_enable_wakeup_irq(port->qusb2_phy_irq, 0);
/*
* Configure DP/DM line interrupts based on the USB2 device attached to
@@ -386,21 +380,37 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
* DP and DM lines as rising edge to detect HS/HS/LS device connect scenario.
*/
- if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq,
+ if (port->usb2_speed == USB_SPEED_LOW) {
+ dwc3_qcom_enable_wakeup_irq(port->dm_hs_phy_irq,
IRQ_TYPE_EDGE_FALLING);
- } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
- (qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq,
+ } else if ((port->usb2_speed == USB_SPEED_HIGH) ||
+ (port->usb2_speed == USB_SPEED_FULL)) {
+ dwc3_qcom_enable_wakeup_irq(port->dp_hs_phy_irq,
IRQ_TYPE_EDGE_FALLING);
} else {
- dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(port->dp_hs_phy_irq,
IRQ_TYPE_EDGE_RISING);
- dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(port->dm_hs_phy_irq,
IRQ_TYPE_EDGE_RISING);
}
- dwc3_qcom_enable_wakeup_irq(qcom->ports[0].ss_phy_irq, 0);
+ dwc3_qcom_enable_wakeup_irq(port->ss_phy_irq, 0);
+}
+
+static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
+{
+ int i;
+
+ for (i = 0; i < qcom->num_ports; i++)
+ dwc3_qcom_disable_port_interrupts(&qcom->ports[i]);
+}
+
+static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
+{
+ int i;
+
+ for (i = 0; i < qcom->num_ports; i++)
+ dwc3_qcom_enable_port_interrupts(&qcom->ports[i]);
}
static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
@@ -427,7 +437,8 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
* freezable workqueue.
*/
if (dwc3_qcom_is_host(qcom) && wakeup) {
- qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
+ for (i = 0; i < qcom->num_ports; i++)
+ qcom->ports[i].usb2_speed = dwc3_qcom_read_usb2_speed(qcom, i);
dwc3_qcom_enable_interrupts(qcom);
}
--
2.34.1
^ permalink raw reply related
* [PATCH v20 7/9] usb: dwc3: qcom: Refactor IRQ handling in glue driver
From: Krishna Kurapati @ 2024-04-08 13:29 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Bjorn Andersson, Wesley Cheng,
Konrad Dybcio, Greg Kroah-Hartman, Conor Dooley, Thinh Nguyen,
Felipe Balbi, Johan Hovold
Cc: devicetree, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap,
quic_jackp, Krishna Kurapati, Johan Hovold
In-Reply-To: <20240408132925.1880571-1-quic_kriskura@quicinc.com>
On multiport supported controllers, each port has its own DP/DM
and SS (if super speed capable) interrupts. As per the bindings,
their interrupt names differ from standard ones having "_x" added
as suffix (x indicates port number). Identify from the interrupt
names whether the controller is a multiport controller or not.
Refactor dwc3_qcom_setup_irq() call to parse multiport interrupts
along with non-multiport ones accordingly..
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
---
drivers/usb/dwc3/dwc3-qcom.c | 137 ++++++++++++++++++++++++++---------
1 file changed, 103 insertions(+), 34 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index cae5dab8fcfc..35eb338514bc 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -52,6 +52,13 @@
#define APPS_USB_AVG_BW 0
#define APPS_USB_PEAK_BW MBps_to_icc(40)
+struct dwc3_qcom_port {
+ int qusb2_phy_irq;
+ int dp_hs_phy_irq;
+ int dm_hs_phy_irq;
+ int ss_phy_irq;
+};
+
struct dwc3_qcom {
struct device *dev;
void __iomem *qscratch_base;
@@ -59,11 +66,8 @@ struct dwc3_qcom {
struct clk **clks;
int num_clocks;
struct reset_control *resets;
-
- int qusb2_phy_irq;
- int dp_hs_phy_irq;
- int dm_hs_phy_irq;
- int ss_phy_irq;
+ struct dwc3_qcom_port ports[DWC3_MAX_PORTS];
+ u8 num_ports;
enum usb_device_speed usb2_speed;
struct extcon_dev *edev;
@@ -354,24 +358,24 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
{
- dwc3_qcom_disable_wakeup_irq(qcom->qusb2_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->ports[0].qusb2_phy_irq);
if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq);
} else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
(qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq);
} else {
- dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
- dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq);
}
- dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->ports[0].ss_phy_irq);
}
static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
{
- dwc3_qcom_enable_wakeup_irq(qcom->qusb2_phy_irq, 0);
+ dwc3_qcom_enable_wakeup_irq(qcom->ports[0].qusb2_phy_irq, 0);
/*
* Configure DP/DM line interrupts based on the USB2 device attached to
@@ -383,20 +387,20 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
*/
if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq,
- IRQ_TYPE_EDGE_FALLING);
+ dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq,
+ IRQ_TYPE_EDGE_FALLING);
} else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
(qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq,
- IRQ_TYPE_EDGE_FALLING);
+ dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq,
+ IRQ_TYPE_EDGE_FALLING);
} else {
- dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq,
- IRQ_TYPE_EDGE_RISING);
- dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq,
- IRQ_TYPE_EDGE_RISING);
+ dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq,
+ IRQ_TYPE_EDGE_RISING);
+ dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq,
+ IRQ_TYPE_EDGE_RISING);
}
- dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0);
+ dwc3_qcom_enable_wakeup_irq(qcom->ports[0].ss_phy_irq, 0);
}
static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
@@ -517,42 +521,107 @@ static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq,
return ret;
}
-static int dwc3_qcom_setup_irq(struct platform_device *pdev)
+static int dwc3_qcom_setup_port_irq(struct platform_device *pdev, int port_index, bool is_multiport)
{
struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
+ const char *irq_name;
int irq;
int ret;
- irq = platform_get_irq_byname_optional(pdev, "qusb2_phy");
+ if (is_multiport)
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_%d", port_index + 1);
+ else
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_irq");
+ if (!irq_name)
+ return -ENOMEM;
+
+ irq = platform_get_irq_byname_optional(pdev, irq_name);
if (irq > 0) {
- ret = dwc3_qcom_request_irq(qcom, irq, "qusb2_phy");
+ ret = dwc3_qcom_request_irq(qcom, irq, irq_name);
if (ret)
return ret;
- qcom->qusb2_phy_irq = irq;
+ qcom->ports[port_index].dp_hs_phy_irq = irq;
}
- irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_irq");
+ if (is_multiport)
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dm_hs_phy_%d", port_index + 1);
+ else
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dm_hs_phy_irq");
+ if (!irq_name)
+ return -ENOMEM;
+
+ irq = platform_get_irq_byname_optional(pdev, irq_name);
if (irq > 0) {
- ret = dwc3_qcom_request_irq(qcom, irq, "dp_hs_phy_irq");
+ ret = dwc3_qcom_request_irq(qcom, irq, irq_name);
if (ret)
return ret;
- qcom->dp_hs_phy_irq = irq;
+ qcom->ports[port_index].dm_hs_phy_irq = irq;
}
- irq = platform_get_irq_byname_optional(pdev, "dm_hs_phy_irq");
+ if (is_multiport)
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ss_phy_%d", port_index + 1);
+ else
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ss_phy_irq");
+ if (!irq_name)
+ return -ENOMEM;
+
+ irq = platform_get_irq_byname_optional(pdev, irq_name);
if (irq > 0) {
- ret = dwc3_qcom_request_irq(qcom, irq, "dm_hs_phy_irq");
+ ret = dwc3_qcom_request_irq(qcom, irq, irq_name);
if (ret)
return ret;
- qcom->dm_hs_phy_irq = irq;
+ qcom->ports[port_index].ss_phy_irq = irq;
}
- irq = platform_get_irq_byname_optional(pdev, "ss_phy_irq");
+ if (is_multiport)
+ return 0;
+
+ irq = platform_get_irq_byname_optional(pdev, "qusb2_phy");
if (irq > 0) {
- ret = dwc3_qcom_request_irq(qcom, irq, "ss_phy_irq");
+ ret = dwc3_qcom_request_irq(qcom, irq, "qusb2_phy");
+ if (ret)
+ return ret;
+ qcom->ports[port_index].qusb2_phy_irq = irq;
+ }
+
+ return 0;
+}
+
+static int dwc3_qcom_find_num_ports(struct platform_device *pdev)
+{
+ char irq_name[14];
+ int port_num;
+ int irq;
+
+ irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_1");
+ if (irq <= 0)
+ return 1;
+
+ for (port_num = 2; port_num <= DWC3_MAX_PORTS; port_num++) {
+ sprintf(irq_name, "dp_hs_phy_%d", port_num);
+
+ irq = platform_get_irq_byname_optional(pdev, irq_name);
+ if (irq <= 0)
+ return port_num - 1;
+ }
+
+ return DWC3_MAX_PORTS;
+}
+
+static int dwc3_qcom_setup_irq(struct platform_device *pdev)
+{
+ struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
+ bool is_multiport;
+ int ret;
+ int i;
+
+ qcom->num_ports = dwc3_qcom_find_num_ports(pdev);
+ is_multiport = (qcom->num_ports > 1);
+
+ for (i = 0; i < qcom->num_ports; i++) {
+ ret = dwc3_qcom_setup_port_irq(pdev, i, is_multiport);
if (ret)
return ret;
- qcom->ss_phy_irq = irq;
}
return 0;
--
2.34.1
^ permalink raw reply related
* [PATCH v20 5/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport
From: Krishna Kurapati @ 2024-04-08 13:29 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Bjorn Andersson, Wesley Cheng,
Konrad Dybcio, Greg Kroah-Hartman, Conor Dooley, Thinh Nguyen,
Felipe Balbi, Johan Hovold
Cc: devicetree, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap,
quic_jackp, Krishna Kurapati, Johan Hovold
In-Reply-To: <20240408132925.1880571-1-quic_kriskura@quicinc.com>
Add the compatible string for SC8280 Multiport USB controller from
Qualcomm.
There are 4 power event irq interrupts supported by this controller
(one for each port of multiport). Added all the 4 as non-optional
interrupts for SC8280XP-MP
Also each port of multiport has one DP and oen DM IRQ. Add all DP/DM
IRQ's related to 4 ports of SC8280XP Teritiary controller.
Also added ss phy irq for both SS Ports.
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
Reviewed-by: Bjorn Andersson <andersson@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
---
.../devicetree/bindings/usb/qcom,dwc3.yaml | 34 +++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index 38a3404ec71b..f55f601c0329 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -30,6 +30,7 @@ properties:
- qcom,sc7180-dwc3
- qcom,sc7280-dwc3
- qcom,sc8280xp-dwc3
+ - qcom,sc8280xp-dwc3-mp
- qcom,sdm660-dwc3
- qcom,sdm670-dwc3
- qcom,sdm845-dwc3
@@ -282,6 +283,7 @@ allOf:
contains:
enum:
- qcom,sc8280xp-dwc3
+ - qcom,sc8280xp-dwc3-mp
- qcom,x1e80100-dwc3
then:
properties:
@@ -470,6 +472,38 @@ allOf:
- const: dm_hs_phy_irq
- const: ss_phy_irq
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sc8280xp-dwc3-mp
+ then:
+ properties:
+ interrupts:
+ minItems: 18
+ maxItems: 18
+ interrupt-names:
+ items:
+ - const: pwr_event_1
+ - const: pwr_event_2
+ - const: pwr_event_3
+ - const: pwr_event_4
+ - const: hs_phy_1
+ - const: hs_phy_2
+ - const: hs_phy_3
+ - const: hs_phy_4
+ - const: dp_hs_phy_1
+ - const: dm_hs_phy_1
+ - const: dp_hs_phy_2
+ - const: dm_hs_phy_2
+ - const: dp_hs_phy_3
+ - const: dm_hs_phy_3
+ - const: dp_hs_phy_4
+ - const: dm_hs_phy_4
+ - const: ss_phy_1
+ - const: ss_phy_2
+
additionalProperties: false
examples:
--
2.34.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox