* Re: [PATCH v2, 3/3] arm64: dts: meson-g12b-ugoos-am6: add initial device-tree
From: Martin Blumenstingl @ 2019-08-24 21:29 UTC (permalink / raw)
To: Christian Hewitt
Cc: Mark Rutland, devicetree, Oleg Ivanov, Kevin Hilman, linux-kernel,
Rob Herring, linux-amlogic, linux-arm-kernel
In-Reply-To: <1566633850-9421-4-git-send-email-christianshewitt@gmail.com>
Hi Christian,
On Sat, Aug 24, 2019 at 10:06 AM Christian Hewitt
<christianshewitt@gmail.com> wrote:
>
> The Ugoos AM6 is based on the Amlogic W400 (G12B) reference design using the
> S922X chipset. Hardware specifications:
>
> - 2GB LPDDR4 RAM
> - 16GB eMMC storage
> - 10/100/1000 Base-T Ethernet using External RGMII PHY
> - 802.11 a/b/g/b/ac + BT 5.0 sdio wireless (Ampak 6398S)
> - HDMI 2.0 (4k@60p) video
> - Composite video + 2-channel audio output on 3.5mm jack
> - S/PDIF audio output
> - Aux input
> - 1x USB 3.0
> - 3x USB 2.0
> - 1x micro SD card slot
>
> The device-tree is laregly based on meson-g12b-odroid-n2 but with audio
typo -> largely
[...]
> + tflash_vdd: regulator-tflash_vdd {
> + compatible = "regulator-fixed";
> +
> + regulator-name = "TFLASH_VDD";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> +
> + gpio = <&gpio_ao GPIOAO_8 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
do we need regulator-always-on here as well, see [0]?
[...]
> + usb_pwr_en: regulator-usb_pwr_en {
> + compatible = "regulator-fixed";
> + regulator-name = "USB_PWR_EN";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + vin-supply = <&vcc_5v>;
> +
> + /* Connected to the microUSB port power enable */
> + gpio = <&gpio GPIOH_6 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + };
the photos I found don't show a micro USB port (but 3x USB A 2.0 and
1x USB A 3.0 - just like you mentioned in the patch description)
does this regulator exist?
[...]
> +ðmac {
> + pinctrl-0 = <ð_pins>, <ð_rgmii_pins>;
> + pinctrl-names = "default";
> + status = "okay";
> + phy-mode = "rgmii";
> + phy-handle = <&external_phy>;
> + amlogic,tx-delay-ns = <2>;
> +};
is the PHY reset GPIO not wired to GPIOZ_15 like on Odroid-N2 and X96 Max?
Martin
[0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts?id=dc7f2cb218b5ef65ab3d455a0e62d27e44075203
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH RFC v1 2/2] mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host
From: Martin Blumenstingl @ 2019-08-24 21:34 UTC (permalink / raw)
To: Ulf Hansson
Cc: Mark Rutland, DTML, jianxin.pan, linux-mmc@vger.kernel.org,
Linux Kernel Mailing List, Rob Herring,
open list:ARM/Amlogic Meson..., Linux ARM
In-Reply-To: <CAPDyKFoFQ_QvHD-+Mg_VAR5rqs3CM_h7dw25p81JTzE1Yz7d1A@mail.gmail.com>
Hi Ulf,
On Thu, Aug 22, 2019 at 3:53 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 8 Jul 2019 at 19:33, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> >
> > WiP - only partially working - see performance numbers.
> >
> > Odroid-C1 eMMC (HS-200):
> > Amlogic's vendor driver @ Linux 3.10:
> > 7781351936 bytes (7.8 GB) copied, 134.714 s, 57.8 MB/s
> > This driver:
> > 7781351936 bytes (7.8 GB, 7.2 GiB) copied, 189.02 s, 41.2 MB/s
> >
> > EC-100 eMMC (HS MMC):
> > Amlogic's vendor driver @ Linux 3.10:
> > 15762194432 bytes (16 GB) copied, 422.967 s, 37.3 MB/s
> > This driver:
> > 15762194432 bytes (16 GB, 15 GiB) copied, 9232.65 s, 1.7 MB/s
> >
> > 1) Amlogic's vendor driver does some magic with the divider:
> > clk_div = input_rate / clk_ios - !(input_rate%clk_ios);
> > if (!(clk_div & 0x01)) // if even number, turn it to an odd one
> > clk_div++;
> > It's not clear to me whether what the reason behind this is, what is
> > supposed to be achieved with this?
> >
> > 2) The hardcoded RX clock phases are taken from the vendor driver. It
> > seems that these are only valid when fclk_div3 is used as input
> > clock (however, there are four more inputs). It's not clear to me how
> > to calculate the RX clock phases in set_ios based on the input clock
> > and the ios rate.
> >
> > 3) The hardware supports a timeout IRQ but the max_busy_timeout is not
> > documented anywhere.
> >
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> Martin, overall this looks good to me. Once you moved from RFC to a
> formal patch I will check again, of course.
OK, great
in the meantime I got answers to my questions (off-list) from Jianxin.
also someone asked me (just this week) for the .dts patches so he
could test on his own board (I have them ready but didn't send them
yet)
unfortunately he ran into some data corruption on writing
I can reproduce it but I didn't have time to debug this yet
I'll send an updated version once I have resolved that - as non-RFC
> There are a couple of calls to readl_poll_timeout(), for different
> reasons, that I have some questions about, but we can discuss those in
> the next step.
sure.
feel free to ask now since I still have to debug that data corruption
problem as stated above
Martin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
From: Jernej Škrabec @ 2019-08-24 21:36 UTC (permalink / raw)
To: linux-sunxi, megous
Cc: Mark Rutland, Alessandro Zummo, Alexandre Belloni, devicetree,
Maxime Ripard, linux-kernel, Chen-Yu Tsai, Rob Herring,
linux-arm-kernel, linux-rtc
In-Reply-To: <20190824212746.a5pyilkrrvysjjbd@core.my.home>
Dne sobota, 24. avgust 2019 ob 23:27:46 CEST je Ondřej Jirman napisal(a):
> Hello Jernej,
>
> On Sat, Aug 24, 2019 at 11:09:49PM +0200, Jernej Škrabec wrote:
> > > Visually?
> > >
> > > That would explain why it doesn't work for you. The mainline RTC driver
> > > disables auto-switch feature, and if your board doesn't have a crystal
> > > for
> > > LOSC, RTC will not generate a clock for the RTC.
> > >
> > > H6's dtsi describes by default a situatiuon with external 32k crystal
> > > oscillator. See ext_osc32k node. That's incorrect for your board if it
> > > doesn't have the crystal. You need to fix this in the DTS for your board
> > > instead of patching the driver.
> >
> > I see that reparenting is supported, but I'm not sure how to fix that in
> > DT. Any suggestion?
>
> You may try removing the clocks property from rtc node.
I don't think this would work:
https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-sun6i.c#L246
>
> > > The driver has parent clock selection logic in case the LOSC crystal is
> > > not
> > > used.
> > >
> > > Your patch enables automatic detection of LOSC failure and RTC changes
> > > clock to LOSC automatically, despite what's described in the DTS. That
> > > may fix the issue, but is not the correct solution.
> > >
> > > Registers on my board look like this (external 32k osc is used) for
> > > reference:
> > >
> > > LOSC_CTRL_REG[7000000]: 8011
> > >
> > > KEY_FIELD ??? (0)
> > > LOSC_AUTO_SWT_BYPASS EN (1)
> > > LOSC_AUTO_SWT_EN DIS (0)
> > > EXT_LOSC_EN EN (1)
> > > EXT_LOSC_GSM LOW (0)
> > > BATTERY_DIR DISCHARGE (0)
> > > LOSC_SRC_SEL EXT32k (1)
> > >
> > > LOSC_AUTO_SWT_STA_REG[7000004]: 1
> > >
> > > EXT_LOSC_STA OK (0)
> > > LOSC_AUTO_SWT_PEND NOEFF (0)
> > > LOSC_SRC_SEL_STA EXT32K (1)
> >
> > In my case LOSC_CTRL_REG has value 0x4010 and LOSC_AUTO_SWT_STA_REG
> > has value 0x4, so there is issue with external crystal (it's missing) and
> > RTC switched to internal one.
> >
> > BTW, what's wrong with automatic switching? Why is it disabled?
>
> It always was disabled on mainline (bit 14 was set to 0 even before my
> patch). H6 just probably has another extra undocummented bit, that's needed
> to disables it properly.
>
> You probably don't want a glitch to switch your RTC from high-precision
> clock to a low precision one possibly without any indication in the
> userspace or a kernel log.
>
> Regardless of all this, DTS needs to have a correct description of the HW,
> which means if RTC module is not connected to the 32.757kHz crystal/clock,
> clocks property should be empty.
If we are talking about correct HW description, then clock property should
actually have possibility that two clocks are defined - one for internal RC
(always present) and one external crystal (optional). In such case I could
really just omit external clock and be done with it. But I'm not sure if such
solution is acceptable at this point.
Best regards,
Jernej
>
> regards,
> o.
>
> > Best regards,
> > Jernej
> >
> > > regards,
> > >
> > > o.
> > >
> > > > > The real issue probably is that the mainline driver is missing this:
> > > > >
> > > > > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-b
> > > > > sp#n
> > > > > 650
> > > >
> > > > Not sure what you mean by that. ext vs. int source selection?
> > > >
> > > >
> > > >
> > > > Best regards,
> > > > Jernej
> > > >
> > > > > regards,
> > > > >
> > > > > o.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v2 0/3] net: dsa: mt7530: Convert to PHYLINK and add support for port 5
From: Andrew Lunn @ 2019-08-24 22:15 UTC (permalink / raw)
To: David Miller
Cc: f.fainelli, frank-w, netdev, sean.wang, linux-mips, opensource,
linux-mediatek, john, matthias.bgg, vivien.didelot,
linux-arm-kernel
In-Reply-To: <20190824.141803.1656753287804303137.davem@davemloft.net>
65;5402;1cOn Sat, Aug 24, 2019 at 02:18:03PM -0700, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Fri, 23 Aug 2019 03:09:28 +0200
>
> > That would be Russell.
> >
> > We should try to improve MAINTAINER so that Russell King gets picked
> > by the get_maintainer script.
>
> Shoule he be added to the mt7530 entry?
Hi David
No. I think we need a phylink entry. And then make use of the K: line
format to list keywords. I hope that even though changes like this
don't touch any files listed as being part of phylink, they will match
the keyword and pickup Russell.
I need to do some testing and see if this actually works.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
From: Ondřej Jirman @ 2019-08-24 22:16 UTC (permalink / raw)
To: Jernej Škrabec
Cc: Mark Rutland, Alessandro Zummo, Alexandre Belloni, devicetree,
Maxime Ripard, linux-kernel, linux-sunxi, Rob Herring,
Chen-Yu Tsai, linux-arm-kernel, linux-rtc
In-Reply-To: <7913281.jYEbquIlsS@jernej-laptop>
On Sat, Aug 24, 2019 at 11:36:26PM +0200, Jernej Škrabec wrote:
> Dne sobota, 24. avgust 2019 ob 23:27:46 CEST je Ondřej Jirman napisal(a):
> > Hello Jernej,
> >
> > On Sat, Aug 24, 2019 at 11:09:49PM +0200, Jernej Škrabec wrote:
> > > > Visually?
> > > >
> > > > That would explain why it doesn't work for you. The mainline RTC driver
> > > > disables auto-switch feature, and if your board doesn't have a crystal
> > > > for
> > > > LOSC, RTC will not generate a clock for the RTC.
> > > >
> > > > H6's dtsi describes by default a situatiuon with external 32k crystal
> > > > oscillator. See ext_osc32k node. That's incorrect for your board if it
> > > > doesn't have the crystal. You need to fix this in the DTS for your board
> > > > instead of patching the driver.
> > >
> > > I see that reparenting is supported, but I'm not sure how to fix that in
> > > DT. Any suggestion?
> >
> > You may try removing the clocks property from rtc node.
>
> I don't think this would work:
> https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-sun6i.c#L246
Well, I don't know. There has to be some way to make it work, since the code
deals with it here:
https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-sun6i.c#L270
Number of parents for LOSC is calculated from the DT somehow. Maybne something
to do with the #clock-cells property?
Sorry I can't be of more help here.
> >
> > > > The driver has parent clock selection logic in case the LOSC crystal is
> > > > not
> > > > used.
> > > >
> > > > Your patch enables automatic detection of LOSC failure and RTC changes
> > > > clock to LOSC automatically, despite what's described in the DTS. That
> > > > may fix the issue, but is not the correct solution.
> > > >
> > > > Registers on my board look like this (external 32k osc is used) for
> > > > reference:
> > > >
> > > > LOSC_CTRL_REG[7000000]: 8011
> > > >
> > > > KEY_FIELD ??? (0)
> > > > LOSC_AUTO_SWT_BYPASS EN (1)
> > > > LOSC_AUTO_SWT_EN DIS (0)
> > > > EXT_LOSC_EN EN (1)
> > > > EXT_LOSC_GSM LOW (0)
> > > > BATTERY_DIR DISCHARGE (0)
> > > > LOSC_SRC_SEL EXT32k (1)
> > > >
> > > > LOSC_AUTO_SWT_STA_REG[7000004]: 1
> > > >
> > > > EXT_LOSC_STA OK (0)
> > > > LOSC_AUTO_SWT_PEND NOEFF (0)
> > > > LOSC_SRC_SEL_STA EXT32K (1)
> > >
> > > In my case LOSC_CTRL_REG has value 0x4010 and LOSC_AUTO_SWT_STA_REG
> > > has value 0x4, so there is issue with external crystal (it's missing) and
> > > RTC switched to internal one.
> > >
> > > BTW, what's wrong with automatic switching? Why is it disabled?
> >
> > It always was disabled on mainline (bit 14 was set to 0 even before my
> > patch). H6 just probably has another extra undocummented bit, that's needed
> > to disables it properly.
> >
> > You probably don't want a glitch to switch your RTC from high-precision
> > clock to a low precision one possibly without any indication in the
> > userspace or a kernel log.
> >
> > Regardless of all this, DTS needs to have a correct description of the HW,
> > which means if RTC module is not connected to the 32.757kHz crystal/clock,
> > clocks property should be empty.
>
> If we are talking about correct HW description, then clock property should
> actually have possibility that two clocks are defined - one for internal RC
> (always present) and one external crystal (optional). In such case I could
> really just omit external clock and be done with it. But I'm not sure if such
Internal RC is thought to be part of the RTC module, so it's not defined as an
input clock to the RTC module.
regards,
Ondrej
>
> Best regards,
> Jernej
>
> >
> > regards,
> > o.
> >
> > > Best regards,
> > > Jernej
> > >
> > > > regards,
> > > >
> > > > o.
> > > >
> > > > > > The real issue probably is that the mainline driver is missing this:
> > > > > >
> > > > > > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-b
> > > > > > sp#n
> > > > > > 650
> > > > >
> > > > > Not sure what you mean by that. ext vs. int source selection?
> > > > >
> > > > >
> > > > >
> > > > > Best regards,
> > > > > Jernej
> > > > >
> > > > > > regards,
> > > > > >
> > > > > > o.
>
>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v2 0/3] net: dsa: mt7530: Convert to PHYLINK and add support for port 5
From: Russell King - ARM Linux admin @ 2019-08-24 22:18 UTC (permalink / raw)
To: David Miller
Cc: andrew, f.fainelli, frank-w, netdev, sean.wang, linux-mips,
opensource, linux-mediatek, john, matthias.bgg, vivien.didelot,
linux-arm-kernel
In-Reply-To: <20190824.141803.1656753287804303137.davem@davemloft.net>
On Sat, Aug 24, 2019 at 02:18:03PM -0700, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Fri, 23 Aug 2019 03:09:28 +0200
>
> > That would be Russell.
> >
> > We should try to improve MAINTAINER so that Russell King gets picked
> > by the get_maintainer script.
>
> Shoule he be added to the mt7530 entry?
Probably some way to make MAINTAINERS pick up on phylink-containing
patches. Something like:
K: phylink
maybe?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v2 0/3] net: dsa: mt7530: Convert to PHYLINK and add support for port 5
From: Russell King - ARM Linux admin @ 2019-08-24 22:29 UTC (permalink / raw)
To: René van Dorst
Cc: Andrew Lunn, Florian Fainelli, Frank Wunderlich, netdev,
Sean Wang, linux-mips, David S . Miller, linux-mediatek,
John Crispin, Matthias Brugger, Vivien Didelot, linux-arm-kernel
In-Reply-To: <20190821144547.15113-1-opensource@vdorst.com>
On Wed, Aug 21, 2019 at 04:45:44PM +0200, René van Dorst wrote:
> 1. net: dsa: mt7530: Convert to PHYLINK API
> This patch converts mt7530 to PHYLINK API.
> 2. dt-bindings: net: dsa: mt7530: Add support for port 5
> 3. net: dsa: mt7530: Add support for port 5
> These 2 patches adding support for port 5 of the switch.
>
> v1->v2:
> * Mostly phylink improvements after review.
> rfc -> v1:
> * Mostly phylink improvements after review.
> * Drop phy isolation patches. Adds no value for now.
> René van Dorst (3):
> net: dsa: mt7530: Convert to PHYLINK API
> dt-bindings: net: dsa: mt7530: Add support for port 5
> net: dsa: mt7530: Add support for port 5
>
> .../devicetree/bindings/net/dsa/mt7530.txt | 218 ++++++++++
> drivers/net/dsa/mt7530.c | 371 +++++++++++++++---
> drivers/net/dsa/mt7530.h | 61 ++-
> 3 files changed, 577 insertions(+), 73 deletions(-)
Having looked through this set of patches, I don't see anything
from the phylink point of view that concerns me. So, for the
series from the phylink perspective:
Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
Thanks.
I did notice a dev_info() in patch 3 that you may like to consider
whether they should be printed at info level or debug level. You
may keep my ack on the patch when fixing that.
I haven't considered whether the patch passes davem's style
requirements for networking code; what I spotted did look like
the declarations were upside-down christmas tree.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v2 0/3] net: dsa: mt7530: Convert to PHYLINK and add support for port 5
From: Russell King - ARM Linux admin @ 2019-08-24 22:31 UTC (permalink / raw)
To: Andrew Lunn
Cc: f.fainelli, frank-w, netdev, sean.wang, linux-mips,
vivien.didelot, opensource, linux-mediatek, john, matthias.bgg,
David Miller, linux-arm-kernel
In-Reply-To: <20190824221519.GF8251@lunn.ch>
On Sun, Aug 25, 2019 at 12:15:19AM +0200, Andrew Lunn wrote:
> 65;5402;1cOn Sat, Aug 24, 2019 at 02:18:03PM -0700, David Miller wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > Date: Fri, 23 Aug 2019 03:09:28 +0200
> >
> > > That would be Russell.
> > >
> > > We should try to improve MAINTAINER so that Russell King gets picked
> > > by the get_maintainer script.
> >
> > Shoule he be added to the mt7530 entry?
>
> Hi David
>
> No. I think we need a phylink entry. And then make use of the K: line
> format to list keywords. I hope that even though changes like this
> don't touch any files listed as being part of phylink, they will match
> the keyword and pickup Russell.
Note that phylink itself is already covered by
"SFF/SFP/SFP+ MODULE SUPPORT"
but doesn't pick up on keywords.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: cleanup the dma_pgprot handling
From: Christoph Hellwig @ 2019-08-24 22:34 UTC (permalink / raw)
To: Paul Burton
Cc: Shawn Anastasio, Will Deacon, linux-m68k@lists.linux-m68k.org,
Catalin Marinas, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, Russell King,
linux-mips@vger.kernel.org, iommu@lists.linux-foundation.org,
Geert Uytterhoeven, James Hogan, Guan Xuetao, Christoph Hellwig,
linux-arm-kernel@lists.infradead.org, Robin Murphy
In-Reply-To: <20190823215759.zprrwotlbva46y33@pburton-laptop>
On Fri, Aug 23, 2019 at 09:58:04PM +0000, Paul Burton wrote:
> So I believe uncached & uncached accelerated are another case like that
> described above - they're 2 different CCAs but the same "access type",
> namely uncached.
>
> Section 4.9 then goes on to forbid mixing access types, but not CCAs.
>
> It would be nice if the precise mapping from CCA to access type was
> provided, but I don't see that anywhere. I can check with the
> architecture team to be sure, but to my knowledge we're fine to mix
> access via kseg1 (ie. uncached) & mappings with CCA=7 (uncached
> accelerated).
Ok. Looks like we can keep it then and I'll add a comment to the
code with the above reference.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH V5 1/5] iommu/amd: Remove unnecessary locking from AMD iommu driver
From: Christoph Hellwig @ 2019-08-24 22:41 UTC (permalink / raw)
To: Tom Murphy
Cc: Heiko Stuebner, virtualization, Matthias Brugger, Thierry Reding,
Will Deacon, Jean-Philippe Brucker, linux-samsung-soc,
Krzysztof Kozlowski, Jonathan Hunter, Christoph Hellwig,
linux-rockchip, Kukjin Kim, Andy Gross, linux-s390,
Gerald Schaefer, linux-arm-msm, linux-mediatek, linux-tegra,
linux-arm-kernel, Robin Murphy, Linux Kernel Mailing List, iommu,
David Woodhouse
In-Reply-To: <CALQxJussiGDzWFT1xhko6no5jZNOezWCFuJQUCr4XwH4NHri3Q@mail.gmail.com>
Thank for the explanation Tom. It might make sense to add a condensed
version of it to commit log for the next iteration.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] net: dsa: mt7530: Add support for port 5
From: David Miller @ 2019-08-24 23:19 UTC (permalink / raw)
To: opensource
Cc: andrew, f.fainelli, frank-w, netdev, sean.wang, linux-mips,
linux-mediatek, john, matthias.bgg, vivien.didelot,
linux-arm-kernel
In-Reply-To: <20190821144547.15113-4-opensource@vdorst.com>
From: René van Dorst <opensource@vdorst.com>
Date: Wed, 21 Aug 2019 16:45:47 +0200
> + dev_info(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
> + val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
This is debugging, at best. Please make this a debugging message or
remove it entirely.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/2] dma-contiguous: Use fallback alloc_pages for single pages
From: Christoph Hellwig @ 2019-08-25 1:10 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Ulf Hansson, Tony Lindgren, Catalin Marinas, Will Deacon,
Linux Kernel Mailing List, Max Filippov, Christoph Hellwig,
Marek Szyprowski, Stephen Rothwell, Joerg Roedel, Russell King,
Thierry Reding, linux-xtensa, Kees Cook, Nicolin Chen,
Andrew Morton, linux-arm-kernel, Chris Zankel, Wolfram Sang,
Robin Murphy, linux-mmc, Adrian Hunter, iommu, iamjoonsoo.kim,
David Woodhouse
In-Reply-To: <CAK7LNAQfYBCoChMV=MOwcUyVoqRkrPWs7DaWdzDqjBe18gGiAQ@mail.gmail.com>
On Fri, Aug 23, 2019 at 09:56:52PM +0900, Masahiro Yamada wrote:
> + linux-mmc, Ulf Hansson, Adrian Hunter,
>
>
> ADMA of SDHCI is not working
> since bd2e75633c8012fc8a7431c82fda66237133bf7e
Does it work for you with this commit:
http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/90ae409f9eb3bcaf38688f9ec22375816053a08e
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for ls1088a and ls2088a
From: Xiaowei Bao @ 2019-08-25 3:07 UTC (permalink / raw)
To: christophe leroy, Andrew Murray
Cc: mark.rutland@arm.com, Roy Zang, bhelgaas@google.com,
lorenzo.pieralisi@arm.co, arnd@arndb.de,
devicetree@vger.kernel.org, gregkh@linuxfoundation.org, Leo Li,
linux-kernel@vger.kernel.org, kishon@ti.com, M.h. Lian,
robh+dt@kernel.org, linux-arm-kernel@lists.infradead.org,
linux-pci@vger.kernel.org, jingoohan1@gmail.com,
shawnguo@kernel.org, gustavo.pimentel@synopsys.com,
linuxppc-dev@lists.ozlabs.org, Mingkai Hu
In-Reply-To: <89c90732-5e42-f87e-73b1-8d615355afc4@c-s.fr>
> -----Original Message-----
> From: christophe leroy <christophe.leroy@c-s.fr>
> Sent: 2019年8月24日 14:45
> To: Xiaowei Bao <xiaowei.bao@nxp.com>; Andrew Murray
> <andrew.murray@arm.com>
> Cc: mark.rutland@arm.com; Roy Zang <roy.zang@nxp.com>;
> lorenzo.pieralisi@arm.co; arnd@arndb.de; devicetree@vger.kernel.org;
> gregkh@linuxfoundation.org; linuxppc-dev@lists.ozlabs.org;
> linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; kishon@ti.com; M.h.
> Lian <minghuan.lian@nxp.com>; robh+dt@kernel.org;
> gustavo.pimentel@synopsys.com; jingoohan1@gmail.com;
> bhelgaas@google.com; Leo Li <leoyang.li@nxp.com>; shawnguo@kernel.org;
> Mingkai Hu <mingkai.hu@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> ls1088a and ls2088a
>
>
>
> Le 24/08/2019 à 02:18, Xiaowei Bao a écrit :
> >
> >
> >> -----Original Message-----
> >> From: Andrew Murray <andrew.murray@arm.com>
> >> Sent: 2019年8月23日 22:28
> >> To: Xiaowei Bao <xiaowei.bao@nxp.com>
> >> Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
> >> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com;
> >> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org;
> M.h.
> >> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy
> >> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com;
> >> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> >> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support
> >> for ls1088a and ls2088a
> >>
> >> On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> >>> Add PCIe EP mode support for ls1088a and ls2088a, there are some
> >>> difference between LS1 and LS2 platform, so refactor the code of the
> >>> EP driver.
> >>>
> >>> Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> >>> ---
> >>> v2:
> >>> - New mechanism for layerscape EP driver.
> >>
> >> Was there a v1 of this patch?
> >
> > Yes, but I don't know how to comments, ^_^
>
> As far as I can see, in the previous version of the series
> (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.ozlabs.org%2Fproject%2Flinuxppc-dev%2Flist%2F%3Fseries%3D125315
> %26state%3D*&data=02%7C01%7Cxiaowei.bao%40nxp.com%7C1befe9
> a67c8046f9535e08d7285eaab6%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C637022259387139020&sdata=p4wbycd04Z7qRUfAoZtwc
> UP7pR%2FuA3%2FjVcWMz6YyQVQ%3D&reserved=0),
> the 8/10 was something completely different, and I can't find any other patch
> in the series that could have been the v1 of this patch.
Thanks, I will correct it to v1 in next version patch.
>
> Christophe
>
> >
> >>
> >>>
> >>> drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> >>> ++++++++++++++++++++------
> >>> 1 file changed, 58 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> index 7ca5fe8..2a66f07 100644
> >>> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> @@ -20,27 +20,29 @@
> >>>
> >>> #define PCIE_DBI2_OFFSET 0x1000 /* DBI2 base address*/
> >>>
> >>> -struct ls_pcie_ep {
> >>> - struct dw_pcie *pci;
> >>> - struct pci_epc_features *ls_epc;
> >>> +#define to_ls_pcie_ep(x) dev_get_drvdata((x)->dev)
> >>> +
> >>> +struct ls_pcie_ep_drvdata {
> >>> + u32 func_offset;
> >>> + const struct dw_pcie_ep_ops *ops;
> >>> + const struct dw_pcie_ops *dw_pcie_ops;
> >>> };
> >>>
> >>> -#define to_ls_pcie_ep(x) dev_get_drvdata((x)->dev)
> >>> +struct ls_pcie_ep {
> >>> + struct dw_pcie *pci;
> >>> + struct pci_epc_features *ls_epc;
> >>> + const struct ls_pcie_ep_drvdata *drvdata; };
> >>>
> >>> static int ls_pcie_establish_link(struct dw_pcie *pci) {
> >>> return 0;
> >>> }
> >>>
> >>> -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> >>> +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> >>> .start_link = ls_pcie_establish_link, };
> >>>
> >>> -static const struct of_device_id ls_pcie_ep_of_match[] = {
> >>> - { .compatible = "fsl,ls-pcie-ep",},
> >>> - { },
> >>> -};
> >>> -
> >>> static const struct pci_epc_features*
> >>> ls_pcie_ep_get_features(struct dw_pcie_ep *ep) { @@ -82,10 +84,44
> >>> @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> >>> }
> >>> }
> >>>
> >>> -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> >>> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> >>> + u8 func_no)
> >>> +{
> >>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >>> + struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> >>> + u8 header_type;
> >>> +
> >>> + header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> >>> +
> >>> + if (header_type & (1 << 7))
> >>> + return pcie->drvdata->func_offset * func_no;
> >>> + else
> >>> + return 0;
> >>
> >> It looks like there isn't a PCI define for multi function, the
> >> nearest I could find was PCI_HEADER_TYPE_MULTIDEVICE in
> >> hotplug/ibmphp.h. A comment above the test might be helpful to explain
> the test.
> >
> > Yes, I have not find the PCI_HEADER_TYPE_MULTIDEVICE define. OK, I
> > will add The comments in next version patch.
> >
> >>
> >> As the ls_pcie_ep_drvdata structures are static, the unset
> >> .func_offset will be initialised to 0, so you could just drop the test above.
> >
> > OK, thanks
> >
> >>
> >> However something to the effect of the following may help spot
> >> misconfiguration:
> >>
> >> WARN_ON(func_no && !pcie->drvdata->func_offset); return
> >> pcie->drvdata->func_offset * func_no;
> >
> > Thanks a lot, this looks better.
> >
> >>
> >> The WARN is probably quite useful as if you are attempting to use
> >> non-zero functions and func_offset isn't set - then things may appear
> >> to work normally but actually will break horribly.
> >
> > got it, thanks.
> >
> >>
> >> Thanks,
> >>
> >> Andrew Murray
> >>
> >>> +}
> >>> +
> >>> +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> >>> .ep_init = ls_pcie_ep_init,
> >>> .raise_irq = ls_pcie_ep_raise_irq,
> >>> .get_features = ls_pcie_ep_get_features,
> >>> + .func_conf_select = ls_pcie_ep_func_conf_select, };
> >>> +
> >>> +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> >>> + .ops = &ls_pcie_ep_ops,
> >>> + .dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> >>> +
> >>> +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> >>> + .func_offset = 0x20000,
> >>> + .ops = &ls_pcie_ep_ops,
> >>> + .dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> >>> +
> >>> +static const struct of_device_id ls_pcie_ep_of_match[] = {
> >>> + { .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> >>> + { .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> >>> + { .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> >>> + { },
> >>> };
> >>>
> >>> static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@ -98,7
> >>> +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
> >>> int ret;
> >>>
> >>> ep = &pci->ep;
> >>> - ep->ops = &pcie_ep_ops;
> >>> + ep->ops = pcie->drvdata->ops;
> >>>
> >>> res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >> "addr_space");
> >>> if (!res)
> >>> @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> >> platform_device *pdev)
> >>> if (!ls_epc)
> >>> return -ENOMEM;
> >>>
> >>> - dbi_base = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> >> "regs");
> >>> - pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> >>> - if (IS_ERR(pci->dbi_base))
> >>> - return PTR_ERR(pci->dbi_base);
> >>> + pcie->drvdata = of_device_get_match_data(dev);
> >>>
> >>> - pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> >>> pci->dev = dev;
> >>> - pci->ops = &ls_pcie_ep_ops;
> >>> + pci->ops = pcie->drvdata->dw_pcie_ops;
> >>> +
> >>> pcie->pci = pci;
> >>>
> >>> ls_epc->linkup_notifier = false,
> >>> @@ -152,6 +185,13 @@ static int __init ls_pcie_ep_probe(struct
> >>> platform_device *pdev)
> >>>
> >>> pcie->ls_epc = ls_epc;
> >>>
> >>> + dbi_base = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> >> "regs");
> >>> + pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> >>> + if (IS_ERR(pci->dbi_base))
> >>> + return PTR_ERR(pci->dbi_base);
> >>> +
> >>> + pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> >>> +
> >>> platform_set_drvdata(pdev, pcie);
> >>>
> >>> ret = ls_add_pcie_ep(pcie, pdev);
> >>> --
> >>> 2.9.5
> >>>
>
> ---
> L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel
> antivirus Avast.
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> avast.com%2Fantivirus&data=02%7C01%7Cxiaowei.bao%40nxp.com%7
> C1befe9a67c8046f9535e08d7285eaab6%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C637022259387139020&sdata=JAYds7X%2FHVxgtrg
> e%2F%2FvnP84zdb2yReXcctQUiSLC11I%3D&reserved=0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 0/7] arm64: dts: meson: ir keymap updates
From: Christian Hewitt @ 2019-08-25 4:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
This series adds keymaps for several box/board vendor IR remote devices
to respective device-tree files. The keymaps were submitted in [0] and
have been queued for inclusion in Linux 5.4.
The Khadas remote change swaps the rc-geekbox keymap for rc-khadas. The
Geekbox branded remote was only sold for a brief period when VIM(1) was
a new device. The Khadas branded remote that replaced it exchanged the
Geekbox full-screen key for an Android mouse button using a different IR
keycode. The rc-khadas keymap supports the mouse button keycode and maps
it to KEY_MUTE.
[0] https://patchwork.kernel.org/project/linux-media/list/?series=160309
Christian Hewitt (7):
arm64: dts: meson-g12b-odroid-n2: add rc-odroid keymap
arm64: dts: meson-g12a-x96-max: add rc-x96max keymap
arm64: dts: meson-gxbb-wetek-hub: add rc-wetek-hub keymap
arm64: dts: meson-gxbb-wetek-play2: add rc-wetek-play2 keymap
arm64: dts: meson-gxl-s905x-khadas-vim: use rc-khadas keymap
arm64: dts: meson-gxl-s905w-tx3-mini: add rc-tx3mini keymap
arm64: dts: meson-gxm-khadas-vim2: use rc-khadas keymap
arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts | 1 +
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts | 1 +
arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts | 4 ++++
arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts | 4 ++++
arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 4 ++++
arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts | 2 +-
arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts | 2 +-
7 files changed, 16 insertions(+), 2 deletions(-)
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 1/7] arm64: dts: meson-g12b-odroid-n2: add rc-odroid keymap
From: Christian Hewitt @ 2019-08-25 4:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>
add the rc-odroid keymap to the ir node
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
index 81780ff..35cef76 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
@@ -314,6 +314,7 @@
status = "okay";
pinctrl-0 = <&remote_input_ao_pins>;
pinctrl-names = "default";
+ linux,rc-map-name = "rc-odroid";
};
/* SD card */
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 2/7] arm64: dts: meson-g12a-x96-max: add rc-x96max keymap
From: Christian Hewitt @ 2019-08-25 4:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>
add the rc-x96max keymap to the ir node
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts
index fe4013c..357d7dc 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts
@@ -277,6 +277,7 @@
status = "okay";
pinctrl-0 = <&remote_input_ao_pins>;
pinctrl-names = "default";
+ linux,rc-map-name = "rc-x96max";
};
&ext_mdio {
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 3/7] arm64: dts: meson-gxbb-wetek-hub: add rc-wetek-hub keymap
From: Christian Hewitt @ 2019-08-25 4:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>
add the rc-wetek-hub keymap to the ir node
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts
index 2bfe699..83b985b 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts
@@ -12,3 +12,7 @@
compatible = "wetek,hub", "amlogic,meson-gxbb";
model = "WeTek Hub";
};
+
+&ir {
+ linux,rc-map-name = "rc-wetek-hub";
+};
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 4/7] arm64: dts: meson-gxbb-wetek-play2: add rc-wetek-play2 keymap
From: Christian Hewitt @ 2019-08-25 4:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>
add the rc-wetek-play2 keymap to the ir node
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts
index 0038522..1d32d1f 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts
@@ -54,3 +54,7 @@
&usb1 {
status = "okay";
};
+
+&ir {
+ linux,rc-map-name = "rc-wetek-play2";
+};
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 5/7] arm64: dts: meson-gxl-s905x-khadas-vim: use rc-khadas keymap
From: Christian Hewitt @ 2019-08-25 4:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>
Swap to the rc-khadas keymap that maps the mouse button to KEY_MUTE.
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts
index 5499e8d..2a5cd30 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts
@@ -110,7 +110,7 @@
};
&ir {
- linux,rc-map-name = "rc-geekbox";
+ linux,rc-map-name = "rc-khadas";
};
&gpio_ao {
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 6/7] arm64: dts: meson-gxl-s905w-tx3-mini: add rc-tx3mini keymap
From: Christian Hewitt @ 2019-08-25 4:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>
add the rc-tx3mini keymap to the ir node
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts b/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts
index 789c819..dd729ac 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts
@@ -20,3 +20,7 @@
reg = <0x0 0x0 0x0 0x40000000>; /* 1 GiB or 2 GiB */
};
};
+
+&ir {
+ linux,rc-map-name = "rc-tanix-tx3mini";
+};
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 7/7] arm64: dts: meson-gxm-khadas-vim2: use rc-khadas keymap
From: Christian Hewitt @ 2019-08-25 4:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>
Swap to the rc-khadas keymap that maps the mouse button to KEY_MUTE.
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts b/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
index 989d33a..f25ddd1 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
@@ -299,7 +299,7 @@
status = "okay";
pinctrl-0 = <&remote_input_ao_pins>;
pinctrl-names = "default";
- linux,rc-map-name = "rc-geekbox";
+ linux,rc-map-name = "rc-khadas";
};
&pwm_AO_ab {
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH] input: keyboard: snvs_pwrkey: Send press and release event for i.MX6 S,DL and Q
From: Shawn Guo @ 2019-08-25 7:10 UTC (permalink / raw)
To: Robin van der Gracht
Cc: Mark Rutland, devicetree, Adam Ford, Sascha Hauer,
Dmitry Torokhov, linux-kernel, Rob Herring, NXP Linux Team,
Pengutronix Kernel Team, linux-input, Robin Gong, Fabio Estevam,
linux-arm-kernel
In-Reply-To: <20190823123002.10448-1-robin@protonic.nl>
On Fri, Aug 23, 2019 at 02:30:02PM +0200, Robin van der Gracht wrote:
> The older generation i.MX6 processors send a powerdown request interrupt if
> the powerkey is released before a hard shutdown (5 second press). This should
> allow software to bring down the SoC safely.
>
> For this driver to work as a regular powerkey with the older SoCs, we need to
> send a keypress AND release when we get the powerdown request interrupt.
>
> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> ---
> arch/arm/boot/dts/imx6qdl.dtsi | 2 +-
> arch/arm/boot/dts/imx6sll.dtsi | 2 +-
> arch/arm/boot/dts/imx6sx.dtsi | 2 +-
> arch/arm/boot/dts/imx6ul.dtsi | 2 +-
> arch/arm/boot/dts/imx7s.dtsi | 2 +-
Please have dts changes in a separate patch.
> drivers/input/keyboard/Kconfig | 2 +-
> drivers/input/keyboard/snvs_pwrkey.c | 59 +++++++++++++++++++++++-----
> 7 files changed, 56 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index b3a77bcf00d51..c10d12658743c 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -836,7 +836,7 @@
> };
>
> snvs_pwrkey: snvs-powerkey {
> - compatible = "fsl,sec-v4.0-pwrkey";
> + compatible = "fsl,imx6qdl-sec-v4.0-pwrkey";
> regmap = <&snvs>;
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx6sll.dtsi b/arch/arm/boot/dts/imx6sll.dtsi
> index 1b4899f0fcded..91c7d5bdcc359 100644
> --- a/arch/arm/boot/dts/imx6sll.dtsi
> +++ b/arch/arm/boot/dts/imx6sll.dtsi
> @@ -571,7 +571,7 @@
> };
>
> snvs_pwrkey: snvs-powerkey {
> - compatible = "fsl,sec-v4.0-pwrkey";
> + compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
> regmap = <&snvs>;
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
> index b16a123990a26..b6736db65350f 100644
> --- a/arch/arm/boot/dts/imx6sx.dtsi
> +++ b/arch/arm/boot/dts/imx6sx.dtsi
> @@ -733,7 +733,7 @@
> };
>
> snvs_pwrkey: snvs-powerkey {
> - compatible = "fsl,sec-v4.0-pwrkey";
> + compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
> regmap = <&snvs>;
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi
> index a7f6d1d58e20d..d4678c52b55db 100644
> --- a/arch/arm/boot/dts/imx6ul.dtsi
> +++ b/arch/arm/boot/dts/imx6ul.dtsi
> @@ -644,7 +644,7 @@
> };
>
> snvs_pwrkey: snvs-powerkey {
> - compatible = "fsl,sec-v4.0-pwrkey";
> + compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
> regmap = <&snvs>;
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index 106711d2c01b0..bb68c23beb199 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -604,7 +604,7 @@
> };
>
> snvs_pwrkey: snvs-powerkey {
> - compatible = "fsl,sec-v4.0-pwrkey";
> + compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
> regmap = <&snvs>;
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> linux,keycode = <KEY_POWER>;
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 7c4f19dab34fd..937e58da5ce17 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -436,7 +436,7 @@ config KEYBOARD_SNVS_PWRKEY
> depends on OF
> help
> This is the snvs powerkey driver for the Freescale i.MX application
> - processors that are newer than i.MX6 SX.
> + processors.
>
> To compile this driver as a module, choose M here; the
> module will be called snvs_pwrkey.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 5342d8d45f811..c321e5f2d1087 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -29,6 +29,11 @@
> #define DEBOUNCE_TIME 30
> #define REPEAT_INTERVAL 60
>
> +enum imx_snvs_hwtype {
> + IMX6SX_SNVS, /* i.MX6 SoloX and newer */
> + IMX6QDL_SNVS, /* i.MX6 Solo, DualLite adn Quad */
> +};
> +
> struct pwrkey_drv_data {
> struct regmap *snvs;
> int irq;
> @@ -37,6 +42,19 @@ struct pwrkey_drv_data {
> int wakeup;
> struct timer_list check_timer;
> struct input_dev *input;
> + enum imx_snvs_hwtype hwtype;
> +};
> +
> +static const struct platform_device_id imx_snvs_devtype[] = {
> + {
> + .name = "imx6sx-snvs-pwrkey",
> + .driver_data = IMX6SX_SNVS,
> + }, {
> + .name = "imx6qdl-snvs-pwrkey",
> + .driver_data = IMX6QDL_SNVS,
> + }, {
> + /* sentinel */
> + }
> };
>
> static void imx_imx_snvs_check_for_events(struct timer_list *t)
> @@ -67,13 +85,23 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
> {
> struct platform_device *pdev = dev_id;
> struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> + struct input_dev *input = pdata->input;
> u32 lp_status;
>
> - pm_wakeup_event(pdata->input->dev.parent, 0);
> + pm_wakeup_event(input->dev.parent, 0);
>
> regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> - if (lp_status & SNVS_LPSR_SPO)
> - mod_timer(&pdata->check_timer, jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> + if (lp_status & SNVS_LPSR_SPO) {
> + if (pdata->hwtype == IMX6QDL_SNVS) {
> + input_report_key(input, pdata->keycode, 1);
> + input_report_key(input, pdata->keycode, 0);
> + input_sync(input);
> + pm_relax(input->dev.parent);
> + } else {
> + mod_timer(&pdata->check_timer,
> + jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> + }
> + }
>
> /* clear SPO status */
> regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
> @@ -88,11 +116,24 @@ static void imx_snvs_pwrkey_act(void *pdata)
> del_timer_sync(&pd->check_timer);
> }
>
> +static const struct of_device_id imx_snvs_pwrkey_ids[] = {
> + {
> + .compatible = "fsl,imx6sx-sec-v4.0-pwrkey",
The new compatible should be documented.
> + .data = &imx_snvs_devtype[IMX6SX_SNVS],
> + }, {
> + .compatible = "fsl,imx6qdl-sec-v4.0-pwrkey",
> + .data = &imx_snvs_devtype[IMX6QDL_SNVS],
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
> +
> static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> {
> struct pwrkey_drv_data *pdata = NULL;
> struct input_dev *input = NULL;
> struct device_node *np;
> + const struct of_device_id *of_id;
> int error;
>
> /* Get SNVS register Page */
> @@ -100,6 +141,11 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> if (!np)
> return -ENODEV;
>
> + of_id = of_match_node(imx_snvs_pwrkey_ids, np);
> + if (!of_id)
> + return -ENODEV;
> + pdev->id_entry = of_id->data;
> +
> pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> if (!pdata)
> return -ENOMEM;
> @@ -116,6 +162,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> }
>
> pdata->wakeup = of_property_read_bool(np, "wakeup-source");
> + pdata->hwtype = pdev->id_entry->driver_data;
>
> pdata->irq = platform_get_irq(pdev, 0);
> if (pdata->irq < 0) {
> @@ -175,12 +222,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> return 0;
> }
>
> -static const struct of_device_id imx_snvs_pwrkey_ids[] = {
> - { .compatible = "fsl,sec-v4.0-pwrkey" },
The compatible should be kept for not breaking existing DTBs.
Shawn
> - { /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
> -
> static struct platform_driver imx_snvs_pwrkey_driver = {
> .driver = {
> .name = "snvs_pwrkey",
> --
> 2.20.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: dts: vf610-zii-scu4-aib: Configure IRQ line for GPIO expander
From: Shawn Guo @ 2019-08-25 7:15 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Cory Tusar, Fabio Estevam, Chris Healy, linux-arm-kernel,
linux-kernel
In-Reply-To: <20190824002703.13902-1-andrew.smirnov@gmail.com>
On Fri, Aug 23, 2019 at 05:27:03PM -0700, Andrey Smirnov wrote:
> Configure IRQ line for SX1503 GPIO expander. We already have
> appropriate pinmux entry and all that is missing is "interrupt-parent"
> and "interrupts" properties. Add them.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Cory Tusar <cory.tusar@zii.aero>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
Applied, thanks.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: dts: vf610-zii-scu4-aib: Use generic names for DT nodes
From: Shawn Guo @ 2019-08-25 7:18 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Cory Tusar, Fabio Estevam, Chris Healy, linux-arm-kernel,
linux-kernel
In-Reply-To: <20190824002747.14610-1-andrew.smirnov@gmail.com>
On Fri, Aug 23, 2019 at 05:27:47PM -0700, Andrey Smirnov wrote:
> The devicetree specification recommends using generic node names.
>
> Some ZII dts files already follow such recommendation, but some don't,
> so use generic node names for consistency among the ZII dts files.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Cory Tusar <cory.tusar@zii.aero>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
It doesn't apply to my branch.
Shawn
> ---
> arch/arm/boot/dts/vf610-zii-scu4-aib.dts | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/boot/dts/vf610-zii-scu4-aib.dts b/arch/arm/boot/dts/vf610-zii-scu4-aib.dts
> index 45a978defbdc..6edd682dbd29 100644
> --- a/arch/arm/boot/dts/vf610-zii-scu4-aib.dts
> +++ b/arch/arm/boot/dts/vf610-zii-scu4-aib.dts
> @@ -417,7 +417,7 @@
> pinctrl-0 = <&pinctrl_dspi1>;
> status = "okay";
>
> - spi-flash@0 {
> + flash@0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "jedec,spi-nor";
> @@ -430,7 +430,7 @@
> };
> };
>
> - spi-flash@1 {
> + flash@1 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "jedec,spi-nor";
> @@ -519,7 +519,7 @@
> #gpio-cells = <2>;
> };
>
> - lm75@48 {
> + temp-sensor@48 {
> compatible = "national,lm75";
> reg = <0x48>;
> };
> @@ -534,7 +534,7 @@
> reg = <0x52>;
> };
>
> - ds1682@6b {
> + elapsed-time-recorder@6b {
> compatible = "dallas,ds1682";
> reg = <0x6b>;
> };
> @@ -551,7 +551,7 @@
> reg = <0x38>;
> };
>
> - adt7411@4a {
> + adc@4a {
> compatible = "adi,adt7411";
> reg = <0x4a>;
> };
> @@ -563,7 +563,7 @@
> pinctrl-0 = <&pinctrl_i2c2>;
> status = "okay";
>
> - gpio9: sx1503q@20 {
> + gpio9: io-expander@20 {
> compatible = "semtech,sx1503q";
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_sx1503_20>;
> @@ -574,12 +574,12 @@
> interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
> };
>
> - lm75@4e {
> + temp-sensor@4e {
> compatible = "national,lm75";
> reg = <0x4e>;
> };
>
> - lm75@4f {
> + temp-sensor@4f {
> compatible = "national,lm75";
> reg = <0x4f>;
> };
> @@ -591,17 +591,17 @@
> reg = <0x23>;
> };
>
> - adt7411@4a {
> + adc@4a {
> compatible = "adi,adt7411";
> reg = <0x4a>;
> };
>
> - at24c08@54 {
> + eeprom@54 {
> compatible = "atmel,24c08";
> reg = <0x54>;
> };
>
> - tca9548@70 {
> + i2c-mux@70 {
> compatible = "nxp,pca9548";
> pinctrl-names = "default";
> #address-cells = <1>;
> @@ -639,7 +639,7 @@
> };
> };
>
> - tca9548@71 {
> + i2c-mux@71 {
> compatible = "nxp,pca9548";
> pinctrl-names = "default";
> reg = <0x71>;
> --
> 2.21.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Jerry-ch Chen @ 2019-08-25 9:18 UTC (permalink / raw)
To: Tomasz Figa
Cc: devicetree@vger.kernel.org, Sean Cheng (鄭昇弘),
Frederic Chen (陳俊元),
Rynn Wu (吳育恩), srv_heupstream, po-yang.huang,
suleiman@chromium.org, shik@chromium.org, jerry-ch.chen,
Jungo Lin (林明俊),
Sj Huang (黃信璋), yuzhao@chromium.org,
linux-mediatek@lists.infradead.org, zwisler@chromium.org,
hans.verkuil@cisco.com, matthias.bgg@gmail.com,
Christie Yu (游雅惠), mchehab@kernel.org,
laurent.pinchart+renesas@ideasonboard.com,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <20190802082815.GA203993@chromium.org>
Hi Tomasz,
On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote:
> Hi Jerry,
>
> On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote:
> > From: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> >
> > This patch adds the driver of Face Detection (FD) unit in
> > Mediatek camera system, providing face detection function.
> >
> > The mtk-isp directory will contain drivers for multiple IP
> > blocks found in Mediatek ISP system. It will include ISP Pass 1
> > driver (CAM), sensor interface driver, DIP driver and face
> > detection driver.
> >
> > Signed-off-by: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> > ---
> > drivers/media/platform/Makefile | 2 +
> > drivers/media/platform/mtk-isp/fd/Makefile | 5 +
> > drivers/media/platform/mtk-isp/fd/mtk_fd.h | 157 +++
> > drivers/media/platform/mtk-isp/fd/mtk_fd_40.c | 1259 +++++++++++++++++++++++++
> > include/uapi/linux/v4l2-controls.h | 4 +
> > 5 files changed, 1427 insertions(+)
> > create mode 100644 drivers/media/platform/mtk-isp/fd/Makefile
> > create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> >
>
> Thanks for the patch! I finally got a chance to fully review the code. Sorry
> for the delay. Please check my comments inline.
>
I appreciate your comments.
I've fixed most of the comments and verifying them,
Sorry for the delay, here is the reply.
>
> > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > index e6deb25..8b817cc 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -94,6 +94,8 @@ obj-$(CONFIG_VIDEO_MEDIATEK_MDP) += mtk-mdp/
> >
> > obj-$(CONFIG_VIDEO_MEDIATEK_JPEG) += mtk-jpeg/
> >
> > +obj-$(CONFIG_VIDEO_MEDIATEK_FD) += mtk-isp/fd/
> > +
> > obj-$(CONFIG_VIDEO_QCOM_CAMSS) += qcom/camss/
> >
> > obj-$(CONFIG_VIDEO_QCOM_VENUS) += qcom/venus/
> > diff --git a/drivers/media/platform/mtk-isp/fd/Makefile b/drivers/media/platform/mtk-isp/fd/Makefile
> > new file mode 100644
> > index 0000000..9b1c501
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +mtk-fd-objs += mtk_fd_40.o
> > +
> > +obj-$(CONFIG_VIDEO_MEDIATEK_FD) += mtk-fd.o
> > \ No newline at end of file
> > diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd.h b/drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > new file mode 100644
> > index 0000000..289999b
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > @@ -0,0 +1,157 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +#ifndef __MTK_FD_HW_H__
> > +#define __MTK_FD_HW_H__
> > +
> > +#include <linux/io.h>
> > +#include <linux/types.h>
> > +#include <linux/platform_device.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/videobuf2-v4l2.h>
> > +
> > +#define MTK_FD_OUTPUT_MIN_WIDTH 26U
> > +#define MTK_FD_OUTPUT_MIN_HEIGHT 26U
> > +#define MTK_FD_OUTPUT_MAX_WIDTH 640U
> > +#define MTK_FD_OUTPUT_MAX_HEIGHT 480U
> > +
> > +/* Control the user defined image widths and heights
> > + * to be scaled and performed face detection in FD HW.
> > + * MTK FD support up to 14 user defined image sizes to perform face detection.
> > + */
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_WIDTH (V4L2_CID_USER_MTK_FD_BASE + 1)
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT (V4L2_CID_USER_MTK_FD_BASE + 2)
> > +
> > +/* Control the numbers of user defined image sizes.
> > + * The default value is 0 which means user is not going
> > + * to define the specific image sizes.
> > + */
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_NUM (V4L2_CID_USER_MTK_FD_BASE + 3)
> > +
> > +/* Control the Face Pose to be detected.
> > + * Here describe the value as following:
> > + * {0, detect the front face with rotation from 0 to 270 degrees},
> > + * {1, detect the front face with rotation from 0 to 240 and 300 degrees},
> > + * {2, detect the front face with rotation from 0 to 240 and 330 degrees},
> > + * {3, detect the front face with rotation from 0 to 240 and left side face}.
> > + */
> > +#define V4L2_CID_MTK_FD_DETECT_POSE (V4L2_CID_USER_MTK_FD_BASE + 4)
> > +#define V4L2_CID_MTK_FD_DETECT_SPEEDUP (V4L2_CID_USER_MTK_FD_BASE + 5)
> > +#define V4L2_CID_MTK_FD_EXTRA_MODEL (V4L2_CID_USER_MTK_FD_BASE + 6)
> > +
> > +/* We reserve 16 controls for this driver. */
> > +#define V4L2_CID_MTK_FD_MAX 16
> > +
> > +#define ENABLE_FD 0x111
> > +#define FD_HW_ENABLE 0x4
> > +#define FD_INT_EN 0x15c
> > +#define FD_INT 0x168
> > +#define FD_RESULT 0x178
> > +#define FD_IRQ_MASK 0x001
> > +
> > +#define RS_MAX_BUF_SIZE 2288788
> > +#define FD_MAX_SPEEDUP 7
> > +#define FD_MAX_POSE_VAL 0xfffffffffffffff
> > +#define FD_DEF_POSE_VAL 0x3ff
> > +#define MAX_FD_SEL_NUM 1026
> > +
> > +/* The max. number of face sizes could be detected, for feature scaling */
> > +#define FACE_SIZE_NUM_MAX 14
> > +/* FACE_SIZE_NUM_MAX + 1, first scale for input image W/H */
> > +#define FD_SCALE_NUM 15
> > +
> > +enum fd_state {
> > + FD_ENQ,
> > + FD_CBD,
> > +};
> > +
> > +enum fd_img_format {
> > + FMT_VYUY = 2,
> > + FMT_UYVY,
> > + FMT_YVYU,
> > + FMT_YUYV,
> > + FMT_UNKNOWN
> > +};
>
> Please use #define macros for hardware/firmware interface definitions.
>
Ok, I will refine as following
#define MTK_FD_HW_FMT_VYUY 2
#define MTK_FD_HW_FMT_UYVY 3
#define MTK_FD_HW_FMT_YVYU 4
#define MTK_FD_HW_FMT_YUYV 5
#define MTK_FD_HW_FMT_UNKNOWN 6
> > +
> > +struct fd_buffer {
> > + __u32 scp_addr; /* used by SCP */
> > + __u32 dma_addr; /* used by DMA HW */
> > +} __packed;
> > +
> > +enum fd_scp_cmd {
> > + FD_CMD_INIT,
> > + FD_CMD_ENQUEUE,
> > +};
>
> Ditto.
Ok, I will refine as following
#define MTK_FD_IPI_CMD_INIT 0
#define MTK_FD_IPI_CMD_ENQUEUE 1
>
> > +
> > +struct fd_user_output {
> > + __u64 results[MAX_FD_SEL_NUM];
> > + __u16 number;
> > +};
> > +
> > +struct user_param {
> > + u8 fd_pose;
> > + u8 fd_speedup;
> > + u8 fd_extra_model;
> > + u8 scale_img_num;
> > + u8 src_img_fmt;
> > + __u16 scale_img_width[FD_SCALE_NUM];
> > + __u16 scale_img_height[FD_SCALE_NUM];
> > +} __packed;
> > +
> > +struct fd_hw_param {
> > + struct fd_buffer src_img;
> > + struct fd_buffer user_result;
> > + struct user_param user_param;
> > +} __packed;
> > +
> > +struct cmd_init_info {
> > + struct fd_buffer fd_manager;
> > + __u32 rs_dma_addr;
> > +} __packed;
> > +
> > +struct ipi_message {
> > + u8 cmd_id;
> > + union {
> > + struct cmd_init_info init_param;
> > + struct fd_hw_param hw_param;
> > + };
> > +} __packed;
> > +
> > +struct mtk_fd_hw {
> > + struct clk *fd_clk;
> > + struct rproc *rproc_handle;
> > + struct platform_device *scp_pdev;
> > + struct fd_buffer scp_mem;
> > + wait_queue_head_t wq;
> > + void __iomem *fd_base;
> > + atomic_t fd_user_cnt;
> > + enum fd_state state;
> > + u32 fd_irq_result;
> > + /* Ensure only one job in hw */
> > + struct mutex fd_hw_lock;
> > +};
> > +
> > +struct mtk_fd_dev {
> > + struct platform_device *pdev;
> > + struct v4l2_device v4l2_dev;
> > + struct v4l2_m2m_dev *m2m_dev;
> > + struct media_device mdev;
> > + struct video_device vfd;
> > + struct mtk_fd_hw fd_hw;
>
> Could we just put the contents of that struct directly here? That should
> simplify dereference chains in the code.
>
Ok, I will fix it.
> > + /* Lock for V4L2 operations */
> > + struct mutex vfd_lock;
> > +};
> > +
> > +struct mtk_fd_ctx {
> > + struct mtk_fd_dev *fd_dev;
> > + struct device *dev;
> > + struct v4l2_fh fh;
> > + struct v4l2_ctrl_handler hdl;
> > + struct v4l2_pix_format_mplane src_fmt;
> > + struct v4l2_meta_format dst_fmt;
> > + struct user_param user_param;
> > +};
> > +
> > +#endif/*__MTK_FD_HW_H__*/
> > diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c b/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> > new file mode 100644
> > index 0000000..246d3aa
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> > @@ -0,0 +1,1259 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +#include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_data/mtk_scp.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/remoteproc.h>
> > +#include <media/videobuf2-dma-contig.h>
> > +#include <media/videobuf2-v4l2.h>
> > +#include <linux/wait.h>
> > +#include <media/v4l2-ioctl.h>
> > +#include <media/v4l2-event.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-mem2mem.h>
> > +#include <media/videobuf2-core.h>
> > +
> > +#include "mtk_fd.h"
> > +
> > +static struct v4l2_meta_format fw_param_fmts[] = {
>
> const?
>
> Also, isn't this the buffer with the detected faces rather than params?
This struct will be removed. Yes, it's the buffer with detected faces.
>
> > + {
> > + .dataformat = V4L2_META_FMT_MTISP_PARAMS,
>
> This should use its own format.
Ok, I will define V4L2_META_FMT_MTFD_RESULT in videodev2.h
>
> > + .buffersize = 1024 * 30,
>
> Please define a C struct for this buffer and use sizeof() here.
>
Ok, that will be sizeof(struct fd_user_output);
> > + },
> > +};
>
> Actually, is there a reason to have this array at all if there is only 1
> format supported? I think we could just put the right constants directly in
> the code.
>
I agree, I'll remove this array and use the constants in the code.
> > +
> > +static const struct v4l2_pix_format_mplane in_img_fmts[] = {
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_VYUY,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
> > + },
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_YUYV,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
> > + },
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_YVYU,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
> > + },
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_UYVY,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
>
> If all the formats have the same values for a field, there is no reason to
> have the field initialized here. Just make initialize them to the constant
> values inside TRY_FMT.
>
Ok, I will do so.
> Hmm, are the interleaved YUV formats the only formats supported by this
> hardware? That would be quite unfortunate given the formats supported by the
> other IP blocks on this SoC use the more typical planar formats.
>
The supported YUV 1 plane formats are listed above.
The rest supported format is YUV 2 plane format, which should be
V4L2_PIX_FMT_NV16M.
For the in_img_fmts[], I will refine as following:
static const struct v4l2_pix_format_mplane mtk_fd_img_fmts[] = {
{
.pixelformat = V4L2_PIX_FMT_VYUY,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_YUYV,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_YVYU,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_UYVY,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_NV16M,
.num_planes = 2,
},
{
.pixelformat = V4L2_PIX_FMT_NV61M,
.num_planes = 2,
},
};
> > + },
> > +};
> > +
> > +#define NUM_FORMATS ARRAY_SIZE(in_img_fmts)
> > +
> > +static inline struct mtk_fd_dev *mtk_fd_hw_to_dev(struct mtk_fd_hw *fd_hw)
> > +{
> > + return container_of(fd_hw, struct mtk_fd_dev, fd_hw);
> > +}
> > +
> > +static inline struct mtk_fd_ctx *fh_to_ctx(struct v4l2_fh *fh)
> > +{
> > + return container_of(fh, struct mtk_fd_ctx, fh);
> > +}
> > +
> > +static int mtk_fd_load_scp(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > + phandle rproc_phandle;
> > + int ret;
> > +
> > + /* init scp */
> > + fd_hw->scp_pdev = scp_get_pdev(fd_dev->pdev);
> > + if (!fd_hw->scp_pdev) {
> > + dev_err(dev, "Failed to get scp device\n");
> > + return -ENODEV;
> > + }
> > +
> > + if (of_property_read_u32(fd_dev->pdev->dev.of_node, "mediatek,scp",
> > + &rproc_phandle)) {
> > + dev_err(dev, "Could not get scp device\n");
> > + return -EINVAL;
> > + }
> > +
> > + fd_hw->rproc_handle = rproc_get_by_phandle(rproc_phandle);
> > + if (!fd_hw->rproc_handle) {
> > + dev_err(dev, "Could not get FD's rproc_handle\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = rproc_boot(fd_hw->rproc_handle);
> > + if (ret < 0) {
> > + /**
> > + * Return 0 if downloading firmware successfully,
> > + * otherwise it is failed
> > + */
> > + dev_err(dev, "rproc_boot failed\n");
> > + return -ENODEV;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static dma_addr_t mtk_fd_hw_alloc_rs_dma_addr(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > + void *va;
> > + dma_addr_t dma_handle;
> > +
> > + va = dma_alloc_coherent(dev, RS_MAX_BUF_SIZE, &dma_handle, GFP_KERNEL);
>
> I see this allocated here, but I don't see this freed anywhere.
>
I will fix it in next version.
> > + if (!va) {
> > + dev_err(dev, "dma_alloc null va\n");
> > + return -ENOMEM;
> > + }
> > + memset(va, 0, RS_MAX_BUF_SIZE);
> > +
> > + return dma_handle;
> > +}
> > +
> > +static int mtk_fd_send_ipi_init(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct ipi_message fd_init_msg;
> > + dma_addr_t rs_dma_addr;
> > +
> > + fd_init_msg.cmd_id = FD_CMD_INIT;
> > +
> > + fd_init_msg.init_param.fd_manager.scp_addr = fd_hw->scp_mem.scp_addr;
> > + fd_init_msg.init_param.fd_manager.dma_addr = fd_hw->scp_mem.dma_addr;
> > +
> > + rs_dma_addr = mtk_fd_hw_alloc_rs_dma_addr(fd_hw);
> > + if (!rs_dma_addr)
> > + return -ENOMEM;
> > +
> > + fd_init_msg.init_param.rs_dma_addr = rs_dma_addr;
> > +
> > + return scp_ipi_send(fd_hw->scp_pdev, SCP_IPI_FD_CMD, &fd_init_msg,
> > + sizeof(fd_init_msg), 0);
> > +}
> > +
> > +static int mtk_fd_hw_enable(struct mtk_fd_hw *fd_hw)
> > +{
> > + int ret;
> > +
> > + ret = mtk_fd_load_scp(fd_hw);
> > + if (ret)
> > + return ret;
> > +
> > + ret = mtk_fd_send_ipi_init(fd_hw);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_hw_connect(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + s32 usercount;
> > +
> > + mutex_lock(&fd_hw->fd_hw_lock);
>
> First of all, we don't need a mutex here, because all the V4L2 ioctls are
> already running with the video device mutex.
Ok, I will remove the using of the mutex here.
>
> > + usercount = atomic_inc_return(&fd_hw->fd_user_cnt);
>
> If we already use a mutex, there is no point in using atomic types.
>
Ok, I will replace the atomic types with normal types
> > + if (usercount == 1) {
> > + pm_runtime_get_sync(&fd_dev->pdev->dev);
> > + if (mtk_fd_hw_enable(fd_hw)) {
> > + pm_runtime_put_sync(&fd_dev->pdev->dev);
> > + atomic_dec_return(&fd_hw->fd_user_cnt);
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > + return -EINVAL;
> > + }
> > + }
>
> This is a simple mem-to-mem device, so there is no reason to keep it active
> all the time it's streaming. Please just get the runtime PM counter when
> queuing a job to the hardware and release it when the job finishes.
>
> I guess we might still want to do the costly operations like rproc_boot()
> when we start streaming, though.
>
Do you mean by moving the pm_runtime_get/put stuff to the job execution
and job finish place?
the rproc_boot() operation will be done here.
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_wait_irq(struct mtk_fd_hw *fd_hw)
> > +{
> > + int timeout;
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > +
> > + timeout = wait_event_interruptible_timeout
> > + (fd_hw->wq, (fd_hw->fd_irq_result & FD_IRQ_MASK),
> > + usecs_to_jiffies(1000000));
> > + if (!timeout) {
> > + dev_err(dev, "%s timeout, %d\n", __func__,
> > + fd_hw->fd_irq_result);
> > + return -EAGAIN;
> > + } else if (!(fd_hw->fd_irq_result & FD_IRQ_MASK)) {
> > + dev_err(dev, "%s No IRQ mask:0x%8x\n",
> > + __func__, fd_hw->fd_irq_result);
> > + return -EINVAL;
> > + }
> > + fd_hw->fd_irq_result = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_fd_hw_disconnect(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + s32 usercount;
> > +
> > + mutex_lock(&fd_hw->fd_hw_lock);
> > + atomic_dec_return(&fd_hw->fd_user_cnt);
> > + usercount = atomic_read(&fd_hw->fd_user_cnt);
> > + if (usercount == 0) {
> > + if (fd_hw->state == FD_ENQ)
> > + mtk_fd_wait_irq(fd_hw);
>
> This shouldn't be possible to happen as the queues should be already stopped
> at this point.
>
Ok, I will remove it.
> > +
> > + pm_runtime_put_sync(&fd_dev->pdev->dev);
>
> Any reason to use pm_runtime_put_sync() over pm_runtime_put()?
>
No special reason to do so, the pm_runtime_put_sync here will be moved
to the place each job finished.
> > + rproc_shutdown(fd_hw->rproc_handle);
> > + rproc_put(fd_hw->rproc_handle);
> > + }
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +}
> > +
> > +static void mtk_fd_hw_job_finish(struct mtk_fd_hw *fd_hw,
> > + struct fd_hw_param *fd_param,
> > + enum vb2_buffer_state vb_state)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct mtk_fd_ctx *ctx;
> > + struct device *dev = &fd_dev->pdev->dev;
> > + struct vb2_buffer *src_vb, *dst_vb;
> > + struct vb2_v4l2_buffer *src_vbuf = NULL, *dst_vbuf = NULL;
> > +
> > + ctx = v4l2_m2m_get_curr_priv(fd_dev->m2m_dev);
> > + if (!ctx) {
> > + dev_err(dev, "Instance released before end of transaction\n");
> > + return;
> > + }
>
> This shouldn't be possible. I suspect you're seeing this because
> .stop_streaming is not implemented correctly. See my comment there.
>
Ok, I will remove this.
> > +
> > + src_vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + if (WARN_ON(!src_vb))
> > + return;
>
> This shouldn't be possible.
>
Ditto.
> > + src_vbuf = to_vb2_v4l2_buffer(src_vb);
> > +
> > + dst_vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > + if (WARN_ON(!dst_vb))
> > + return;
>
> Ditto.
>
Ditto.
> > + dst_vbuf = to_vb2_v4l2_buffer(dst_vb);
> > +
> > + dst_vbuf->vb2_buf.timestamp = src_vbuf->vb2_buf.timestamp;
> > + dst_vbuf->timecode = src_vbuf->timecode;
> > + dst_vbuf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> > + dst_vbuf->flags |= src_vbuf->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>
> We should be able to just use v4l2_m2m_buf_copy_metadata().
>
Ok, I will refine as:
v4l2_m2m_buf_copy_metadata(src_vbuf, dst_vbuf,
V4L2_BUF_FLAG_TSTAMP_SRC_MASK);
> > +
> > + v4l2_m2m_buf_done(src_vbuf, vb_state);
> > + v4l2_m2m_buf_done(dst_vbuf, vb_state);
> > + v4l2_m2m_job_finish(fd_dev->m2m_dev, ctx->fh.m2m_ctx);
> > +}
> > +
> > +static int mtk_fd_hw_job_exec(struct mtk_fd_hw *fd_hw,
> > + struct fd_hw_param *fd_param,
> > + void *output_vaddr)
> > +{
> > + struct fd_user_output *fd_output;
> > + struct ipi_message fd_ipi_msg;
> > + int ret;
> > + u32 num;
> > +
> > + if (fd_param->user_param.src_img_fmt == FMT_UNKNOWN)
> > + goto param_err;
>
> Is this possible?
>
Only if user set wrong format, I will remove this.
> > +
> > + mutex_lock(&fd_hw->fd_hw_lock);
> > + fd_hw->state = FD_ENQ;
>
> What is this state for?
>
It was for checking status, if a job is processing, the state is
FD_ENQ,
then we should wait for the last job to be done when pm_suspend().
> > + fd_ipi_msg.cmd_id = FD_CMD_ENQUEUE;
> > + memcpy(&fd_ipi_msg.hw_param, fd_param, sizeof(fd_ipi_msg.hw_param));
> > + ret = scp_ipi_send(fd_hw->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg,
> > + sizeof(fd_ipi_msg), 0);
> > + if (ret)
> > + goto buf_err;
> > +
> > + ret = mtk_fd_wait_irq(fd_hw);
> > + if (ret)
> > + goto buf_err;
>
> This function is called from device_run and it shouldn't be synchronous. It
> should only queue the job to the hardware to be handled asynchronously when
> it finishes.
>
Ok, I will fix it.
> > +
> > + num = readl(fd_hw->fd_base + FD_RESULT);
> > + /* Disable FD ISR */
> > + writel(0x0, fd_hw->fd_base + FD_INT_EN);
> > +
> > + fd_output = (struct fd_user_output *)output_vaddr;
> > + fd_output->number = num;
> > + fd_hw->state = FD_CBD;
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +
> > + mtk_fd_hw_job_finish(fd_hw, fd_param, VB2_BUF_STATE_DONE);
> > + return 0;
> > +
> > +buf_err:
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +param_err:
> > + mtk_fd_hw_job_finish(fd_hw, fd_param, VB2_BUF_STATE_ERROR);
> > + return ret;
> > +}
> > +
> > +static int mtk_fd_vb2_buf_out_validate(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> > +
> > + v4l2_buf->field = V4L2_FIELD_NONE;
>
> We need to fail with -EINVAL if v4l2_buf->field was different than
> V4L2_FIELD_ANY or _NONE.
>
Ok, I will refine as following:
if (v4l2_buf->field == V4L2_FIELD_ANY)
v4l2_buf->field = V4L2_FIELD_NONE;
if (v4l2_buf->field != V4L2_FIELD_NONE)
return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_vb2_buf_prepare(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > + struct vb2_queue *vq = vb->vb2_queue;
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct device *dev = ctx->dev;
> > + struct v4l2_pix_format_mplane *pixfmt;
> > +
> > + switch (vq->type) {
> > + case V4L2_BUF_TYPE_META_CAPTURE:
> > + if (vb2_plane_size(vb, 0) < ctx->dst_fmt.buffersize) {
> > + dev_err(dev, "meta size %d is too small\n");
>
> Please make this dev_dbg(), because userspace misbehavior must not trigger
> kernel error logs.
>
Ok, fixed.
> > + return -EINVAL;
> > + }
> > + break;
> > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > + pixfmt = &ctx->src_fmt;
> > +
> > + if (vbuf->field == V4L2_FIELD_ANY)
> > + vbuf->field = V4L2_FIELD_NONE;
> > +
> > + if (vb->num_planes != 1 || vbuf->field != V4L2_FIELD_NONE) {
> > + dev_err(dev, "plane or field %d not supported\n",
> > + vb->num_planes, vbuf->field);
>
> Ditto.
>
Done.
> > + return -EINVAL;
> > + }
> > + if (vb2_plane_size(vb, 0) < pixfmt->plane_fmt[0].sizeimage) {
> > + dev_err(dev, "plane %d is too small\n");
>
> Ditto.
>
Done.
> > + return -EINVAL;
> > + }
> > + break;
> > + default:
> > + dev_err(dev, "invalid queue type: %d\n", vq->type);
> > + return -EINVAL;
>
> We shouldn't need to handle this.
>
Ok, I will remove this.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_fd_vb2_buf_queue(struct vb2_buffer *vb)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +
> > + v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> > +}
> > +
> > +static int mtk_fd_vb2_queue_setup(struct vb2_queue *vq,
> > + unsigned int *num_buffers,
> > + unsigned int *num_planes,
> > + unsigned int sizes[],
> > + struct device *alloc_devs[])
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct device *dev = ctx->dev;
> > + unsigned int size;
> > +
> > + switch (vq->type) {
> > + case V4L2_BUF_TYPE_META_CAPTURE:
> > + size = ctx->dst_fmt.buffersize;
> > + break;
> > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > + size = ctx->src_fmt.plane_fmt[0].sizeimage;
> > + break;
> > + default:
> > + dev_err(dev, "invalid queue type: %d\n", vq->type);
>
> We should need to handle this.
>
Do you mean by giving a size instead of return -EINVAL?
> > + return -EINVAL;
> > + }
> > +
> > + if (!*num_planes) {
> > + *num_planes = 1;
> > + sizes[0] = size;
> > + }
>
> We need to handle the case when *num_planes is non-zero. We then need to
> check if it's 1 and *sizes >= size and fail with -EINVAL if not.
>
Ok, I will refine as following:
if (*num_planes) {
if (sizes[0] < size || *num_planes != 1)
return -EINVAL;
} else {
*num_planes = 1;
sizes[0] = size;
}
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_vb2_start_streaming(struct vb2_queue *vq, unsigned int count)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > +
> > + return mtk_fd_hw_connect(&ctx->fd_dev->fd_hw);
>
> This would be called twice for every context, once for the VIDEO_OUTPUT
> queue and once for the META_CAPTURE queue. Perhaps it would make sense to
> just do this for the VIDEO_OUTPUT queue?
>
Ok, I will refine as:
if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
return mtk_fd_hw_connect(&ctx->fd_dev->fd_hw);
else
return 0;
> > +}
> > +
> > +static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct vb2_buffer *vb;
>
> How do we guarantee here that the hardware isn't still accessing the buffers
> removed below?
>
Maybe we can check the driver state flag and aborting the unfinished
jobs?
(fd_hw->state == FD_ENQ)
> > +
> > + if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > + vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + else
> > + vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > +
> > + while (vb) {
> > + v4l2_m2m_buf_done(to_vb2_v4l2_buffer(vb), VB2_BUF_STATE_ERROR);
> > + if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > + vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + else
> > + vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > + }
>
> We can use v4l2_m2m_buf_remove(). Also we can move the call into the loop
> condition:
>
> while ((vb == v4l2_m2m_buf_remove(...)))
> v4l2_m2m_buf_done(...);
>
Ok, I will refine as following:
while ((vb = v4l2_m2m_buf_remove(V4L2_TYPE_IS_OUTPUT(vq->type)?
&m2m_ctx->out_q_ctx :
&m2m_ctx->cap_q_ctx)))
v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
> > +
> > + mtk_fd_hw_disconnect(&ctx->fd_dev->fd_hw);
>
> This should also probably be done only for 1 queue. Since the VIDEO_OUTPUT
> queue supports requestes, I'd consider it the master one.
>
Ok, only for output queue to trigger disconnect.
if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
mtk_fd_hw_disconnect(&ctx->fd_dev->fd_hw);
> > +}
> > +
> > +static void mtk_fd_vb2_request_complete(struct vb2_buffer *vb)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +
> > + v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
> > +}
> > +
> > +static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
> > + const struct v4l2_pix_format_mplane *sfmt)
> > +{
> > + dfmt->width = sfmt->width;
> > + dfmt->height = sfmt->height;
> > + dfmt->pixelformat = sfmt->pixelformat;
> > + dfmt->field = sfmt->field;
> > + dfmt->colorspace = sfmt->colorspace;
> > + dfmt->num_planes = sfmt->num_planes;
> > +
> > + /* Use default */
> > + dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > + dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > + dfmt->xfer_func =
> > + V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
> > + dfmt->plane_fmt[0].bytesperline = dfmt->width * 2;
> > + dfmt->plane_fmt[0].sizeimage =
> > + dfmt->height * dfmt->plane_fmt[0].bytesperline;
> > + memset(dfmt->reserved, 0, sizeof(dfmt->reserved));
> > +}
>
> Could we unify this function with mtk_fd_m2m_try_fmt_out_mp()? That function
> should be almost directly reusable for the default format initialization +/-
> the arguments passed to it.
>
Ok, I will try to reuse it as following:
static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
const struct v4l2_pix_format_mplane *sfmt)
{
dfmt->field = V4L2_FIELD_NONE;
dfmt->colorspace = V4L2_COLORSPACE_BT2020;
dfmt->num_planes = sfmt->num_planes;
dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
dfmt->xfer_func =
V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
/* Keep user setting as possible */
dfmt->width = clamp(dfmt->width,
MTK_FD_OUTPUT_MIN_WIDTH,
MTK_FD_OUTPUT_MAX_WIDTH);
dfmt->height = clamp(dfmt->height,
MTK_FD_OUTPUT_MIN_HEIGHT,
MTK_FD_OUTPUT_MAX_HEIGHT);
if (sfmt->num_planes == 2) {
/* NV16M and NV61M has 1 byte per pixel */
dfmt->plane_fmt[0].bytesperline = dfmt->width;
dfmt->plane_fmt[1].bytesperline = dfmt->width;
} else {
/* 2 bytes per pixel */
dfmt->plane_fmt[0].bytesperline = dfmt->width * 2;
}
dfmt->plane_fmt[0].sizeimage =
dfmt->height * dfmt->plane_fmt[0].bytesperline;
}
> > +
> > +static const struct v4l2_pix_format_mplane *mtk_fd_find_fmt(u32 format)
> > +{
> > + unsigned int i;
> > + const struct v4l2_pix_format_mplane *dev_fmt;
> > +
> > + for (i = 0; i < NUM_FORMATS; i++) {
> > + dev_fmt = &in_img_fmts[i];
> > + if (dev_fmt->pixelformat == format)
> > + return dev_fmt;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static int mtk_fd_m2m_querycap(struct file *file, void *fh,
> > + struct v4l2_capability *cap)
> > +{
> > + struct mtk_fd_dev *fd_dev = video_drvdata(file);
> > + struct device *dev = &fd_dev->pdev->dev;
> > +
> > + strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
>
> Please use dev_driver_string().
>
Done,
> > + strscpy(cap->card, fd_dev->vfd.name, sizeof(cap->card));
>
> I think we can also make this dev_driver_string().
>
Done.
> > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> > + dev_name(&fd_dev->pdev->dev));
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_enum_fmt_out_mp(struct file *file, void *fh,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < NUM_FORMATS; ++i) {
> > + if (i == f->index) {
> > + f->pixelformat = in_img_fmts[i].pixelformat;
> > + return 0;
> > + }
> > + }
>
> Why don't we just check if f->index is within the [0, ARRAY_SIZE()-1] bounds
> and then just use it to index the array directly?
>
I will refine as :
static int mtk_fd_m2m_enum_fmt_out_mp(struct file *file, void *fh,
struct v4l2_fmtdesc *f)
{
if ((f->index >= 0) && (f->index < NUM_FORMATS)) {
f->pixelformat = in_img_fmts[f->index].pixelformat;
return 0;
}
return -EINVAL;
}
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int mtk_fd_m2m_try_fmt_out_mp(struct file *file,
> > + void *fh,
> > + struct v4l2_format *f)
>
> I think we could just shorten the function prefixes to "mtk_fd_".
>
Do you mean by replace mtk_fd_m2m_* with mtk_fd_ ?
> > +{
> > + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > + const struct v4l2_pix_format_mplane *fmt;
> > +
> > + fmt = mtk_fd_find_fmt(pix_mp->pixelformat);
> > + if (!fmt) {
> > + /* Get default img fmt */
> > + fmt = &in_img_fmts[0];
> > + f->fmt.pix.pixelformat = fmt->pixelformat;
> > + }
> > +
> > + /* Use default */
> > + pix_mp->field = fmt->field;
> > + pix_mp->colorspace = fmt->colorspace;
> > + pix_mp->num_planes = fmt->num_planes;
> > + pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > + pix_mp->quantization = V4L2_QUANTIZATION_DEFAULT;
> > + pix_mp->xfer_func =
> > + V4L2_MAP_XFER_FUNC_DEFAULT(pix_mp->colorspace);
> > +
> > + /* Keep user setting as possible */
> > + pix_mp->width = clamp(pix_mp->width,
> > + MTK_FD_OUTPUT_MIN_WIDTH,
> > + MTK_FD_OUTPUT_MAX_WIDTH);
> > + pix_mp->height = clamp(pix_mp->height,
> > + MTK_FD_OUTPUT_MIN_HEIGHT,
> > + MTK_FD_OUTPUT_MAX_HEIGHT);
> > +
> > + pix_mp->plane_fmt[0].bytesperline = pix_mp->width * 2;
>
> Please add a comment explaining why this is always 2.
>
Done.
/* 2 bytes per pixel */
> > + pix_mp->plane_fmt[0].sizeimage =
> > + pix_mp->plane_fmt[0].bytesperline * pix_mp->height;
> > + memset(pix_mp->plane_fmt[0].reserved, 0,
> > + sizeof(pix_mp->plane_fmt[0].reserved));
>
> No need to zero this here. The core will handle it.
>
Ok, I will remove it.
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_g_fmt_out_mp(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_fd_ctx *ctx = fh_to_ctx(fh);
> > +
> > + f->fmt.pix_mp = ctx->src_fmt;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_s_fmt_out_mp(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_fd_ctx *ctx = fh_to_ctx(fh);
> > + struct vb2_queue *vq;
> > +
> > + /* Change not allowed if queue is streaming. */
> > + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > + if (vb2_is_streaming(vq) || vb2_is_busy(vq)) {
>
> vb2_is_busy() is a superset of vb2_is_streaming(), so it's enough to just
> check the former.
>
Ok, I will just check the former.
> > + dev_dbg(ctx->dev, "vb2_is_streaming or vb2_is_busy");
> > + return -EBUSY;
> > + }
> > +
> > + mtk_fd_m2m_try_fmt_out_mp(file, fh, f);
> > + ctx->src_fmt = f->fmt.pix_mp;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_enum_fmt_meta_cap(struct file *file, void *fh,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + if (f->index)
> > + return -EINVAL;
> > +
> > + strscpy(f->description, "Face detection result",
> > + sizeof(f->description));
> > + f->pixelformat = fw_param_fmts[0].dataformat;
> > + f->flags = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_g_fmt_meta_cap(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + f->fmt.meta.dataformat = fw_param_fmts[0].dataformat;
> > + f->fmt.meta.buffersize = fw_param_fmts[0].buffersize;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct vb2_ops mtk_fd_vb2_ops = {
> > + .queue_setup = mtk_fd_vb2_queue_setup,
> > + .buf_out_validate = mtk_fd_vb2_buf_out_validate,
> > + .buf_prepare = mtk_fd_vb2_buf_prepare,
> > + .buf_queue = mtk_fd_vb2_buf_queue,
> > + .start_streaming = mtk_fd_vb2_start_streaming,
> > + .stop_streaming = mtk_fd_vb2_stop_streaming,
> > + .wait_prepare = vb2_ops_wait_prepare,
> > + .wait_finish = vb2_ops_wait_finish,
> > + .buf_request_complete = mtk_fd_vb2_request_complete,
> > +};
> > +
> > +static const struct v4l2_ioctl_ops mtk_fd_v4l2_video_out_ioctl_ops = {
> > + .vidioc_querycap = mtk_fd_m2m_querycap,
> > + .vidioc_enum_fmt_vid_out_mplane = mtk_fd_m2m_enum_fmt_out_mp,
> > + .vidioc_g_fmt_vid_out_mplane = mtk_fd_m2m_g_fmt_out_mp,
> > + .vidioc_s_fmt_vid_out_mplane = mtk_fd_m2m_s_fmt_out_mp,
> > + .vidioc_try_fmt_vid_out_mplane = mtk_fd_m2m_try_fmt_out_mp,
> > + .vidioc_enum_fmt_meta_cap = mtk_fd_m2m_enum_fmt_meta_cap,
> > + .vidioc_g_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > + .vidioc_s_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > + .vidioc_try_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > + .vidioc_reqbufs = v4l2_m2m_ioctl_reqbufs,
> > + .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
> > + .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
> > + .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
> > + .vidioc_querybuf = v4l2_m2m_ioctl_querybuf,
> > + .vidioc_qbuf = v4l2_m2m_ioctl_qbuf,
> > + .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
> > + .vidioc_streamon = v4l2_m2m_ioctl_streamon,
> > + .vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
> > + .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> > + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> > +};
> > +
> > +static int
> > +mtk_fd_queue_init(void *priv, struct vb2_queue *src_vq,
> > + struct vb2_queue *dst_vq)
> > +{
> > + struct mtk_fd_ctx *ctx = priv;
> > + int ret;
> > +
> > + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > + src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> > + src_vq->supports_requests = true;
> > + src_vq->drv_priv = ctx;
> > + src_vq->ops = &mtk_fd_vb2_ops;
> > + src_vq->mem_ops = &vb2_dma_contig_memops;
> > + src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > + src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + src_vq->lock = &ctx->fd_dev->vfd_lock;
> > + src_vq->dev = ctx->fd_dev->v4l2_dev.dev;
> > +
> > + ret = vb2_queue_init(src_vq);
> > + if (ret)
> > + return ret;
> > +
> > + dst_vq->type = V4L2_BUF_TYPE_META_CAPTURE;
> > + dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> > + dst_vq->drv_priv = ctx;
> > + dst_vq->ops = &mtk_fd_vb2_ops;
> > + dst_vq->mem_ops = &vb2_dma_contig_memops;
> > + dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > + dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + dst_vq->lock = &ctx->fd_dev->vfd_lock;
> > + dst_vq->dev = ctx->fd_dev->v4l2_dev.dev;
> > +
> > + return vb2_queue_init(dst_vq);
> > +}
> > +
> > +static int mtk_fd_dev_g_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mtk_fd_ctx *ctx = ctrl->priv;
> > + int i;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_MTK_FD_SCALE_IMG_WIDTH:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctrl->p_new.p_u16[i] =
> > + ctx->user_param.scale_img_width[i];
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctrl->p_new.p_u16[i] =
> > + ctx->user_param.scale_img_height[i];
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_POSE:
> > + ctrl->val = ctx->user_param.fd_pose;
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_SPEEDUP:
> > + ctrl->val = ctx->user_param.fd_speedup;
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_NUM:
> > + ctrl->val = ctx->user_param.scale_img_num;
> > + break;
> > + case V4L2_CID_MTK_FD_EXTRA_MODEL:
> > + ctrl->val = ctx->user_param.fd_extra_model;
> > + break;
> > + default:
> > + dev_dbg(ctx->dev, "%s: unexpected control: 0x%x:%d",
> > + __func__, ctrl->id, ctrl->val);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_dev_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mtk_fd_ctx *ctx = ctrl->priv;
> > + int i;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_MTK_FD_SCALE_IMG_WIDTH:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctx->user_param.scale_img_width[i] =
> > + ctrl->p_new.p_u16[i];
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctx->user_param.scale_img_height[i] =
> > + ctrl->p_new.p_u16[i];
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_POSE:
> > + ctx->user_param.fd_pose = ctrl->val;
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_SPEEDUP:
> > + ctx->user_param.fd_speedup = ctrl->val;
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_NUM:
> > + ctx->user_param.scale_img_num = ctrl->val;
> > + break;
> > + case V4L2_CID_MTK_FD_EXTRA_MODEL:
> > + ctx->user_param.fd_extra_model = ctrl->val;
> > + break;
> > + default:
> > + dev_dbg(ctx->dev, "%s: unexpected control: 0x%x:%d",
> > + __func__, ctrl->id, ctrl->val);
> > + return -EINVAL;
>
> No need to handle this, as the framework will only pass the controls we
> registered earlier to this function.
>
Ok, I will remove this .
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops mtk_fd_dev_ctrl_ops = {
> > + .g_volatile_ctrl = mtk_fd_dev_g_ctrl,
>
> I don't see any volatile controls below. No need to implement this callback.
>
Ok, I will remove this .
> > + .s_ctrl = mtk_fd_dev_s_ctrl,
>
> I think you might not even need to implement this function (or just provide
> a dummy one that returns 0 if its required) if you just use the controls
> directly when preparing the job for the hardware. Check v4l2_ctrl_find().
>
I will remove the mtk_fd_dev_ctrl_ops, and use the following function to
get control values when preparing the job for HW,
static void mtk_fd_fill_user_param(struct user_param *user_param,
struct v4l2_ctrl_handler *hdl)
{
struct v4l2_ctrl *ctrl;
int i;
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_WIDTH);
if (ctrl)
for (i = 0; i < ctrl->elems; i++)
user_param->scale_img_width[i] = ctrl->p_new.p_u16[i];
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT);
if (ctrl)
for (i = 0; i < ctrl->elems; i++)
user_param->scale_img_height[i] = ctrl->p_new.p_u16[i];
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECT_POSE);
if (ctrl)
user_param->fd_pose = ctrl->val;
pr_info("pose:%d\n", user_param->fd_pose);
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECT_SPEEDUP);
if (ctrl)
user_param->fd_speedup = ctrl->val;
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_NUM);
if (ctrl)
user_param->scale_img_num = ctrl->val;
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_EXTRA_MODEL);
if (ctrl)
user_param->fd_extra_model = ctrl->val;
}
> > +};
> > +
> > +struct v4l2_ctrl_config mtk_fd_controls[] = {
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_SCALE_IMG_WIDTH,
> > + .name = "FD scale image widths",
> > + .type = V4L2_CTRL_TYPE_U16,
> > + .min = MTK_FD_OUTPUT_MIN_WIDTH,
> > + .max = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .step = 1,
> > + .def = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .dims = { FD_SCALE_NUM },
>
> Something wrong with indentation here.
>
Done.
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT,
> > + .name = "FD scale image heights",
> > + .type = V4L2_CTRL_TYPE_U16,
> > + .min = MTK_FD_OUTPUT_MIN_HEIGHT,
> > + .max = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .step = 1,
> > + .def = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .dims = { FD_SCALE_NUM },
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_SCALE_IMG_NUM,
> > + .name = "FD scale size counts",
> > + .type = V4L2_CTRL_TYPE_INTEGER,
> > + .min = 0,
> > + .max = FACE_SIZE_NUM_MAX,
> > + .step = 1,
> > + .def = 0,
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_DETECT_POSE,
> > + .name = "FD detect face pose",
> > + .type = V4L2_CTRL_TYPE_INTEGER,
> > + .min = 0,
> > + .max = 3,
> > + .step = 1,
> > + .def = 0,
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_DETECT_SPEEDUP,
> > + .name = "FD detection speedup",
> > + .type = V4L2_CTRL_TYPE_INTEGER,
> > + .min = 0,
> > + .max = FD_MAX_SPEEDUP,
> > + .step = 1,
> > + .def = 0,
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_EXTRA_MODEL,
> > + .name = "FD use extra model",
> > + .type = V4L2_CTRL_TYPE_BOOLEAN,
> > + .min = 0,
> > + .max = 1,
> > + .step = 1,
> > + .def = 0,
> > + },
> > +};
> > +
> > +static int mtk_fd_ctrls_setup(struct mtk_fd_ctx *ctx)
> > +{
> > + struct v4l2_ctrl_handler *hdl = &ctx->hdl;
> > + struct v4l2_ctrl *ctl;
> > + int i;
> > +
> > + v4l2_ctrl_handler_init(hdl, V4L2_CID_MTK_FD_MAX);
> > + if (hdl->error)
> > + return hdl->error;
> > +
> > + for (i = 0; i < ARRAY_SIZE(mtk_fd_controls); i++) {
> > + ctl = v4l2_ctrl_new_custom(hdl, &mtk_fd_controls[i], ctx);
> > + if (hdl->error) {
> > + v4l2_ctrl_handler_free(hdl);
> > + dev_err(ctx->dev, "Failed to register controls:%d", i);
> > + return hdl->error;
> > + }
> > + }
> > +
> > + ctx->fh.ctrl_handler = &ctx->hdl;
> > + v4l2_ctrl_handler_setup(hdl);
> > +
> > + return 0;
> > +}
> > +
> > +static unsigned int get_fd_img_fmt(unsigned int fourcc)
> > +{
> > + switch (fourcc) {
> > + case V4L2_PIX_FMT_VYUY:
> > + return FMT_VYUY;
> > + case V4L2_PIX_FMT_YUYV:
> > + return FMT_YUYV;
> > + case V4L2_PIX_FMT_YVYU:
> > + return FMT_YVYU;
> > + case V4L2_PIX_FMT_UYVY:
> > + return FMT_UYVY;
> > + default:
> > + return FMT_UNKNOWN;
> > + }
> > +}
> > +
> > +static void init_ctx_fmt(struct mtk_fd_ctx *ctx)
> > +{
> > + const struct v4l2_pix_format_mplane *fmt;
> > +
> > + /* Initialize M2M source fmt */
> > + fmt = &in_img_fmts[0];
> > + mtk_fd_fill_pixfmt_mp(&ctx->src_fmt, fmt);
> > +
> > + /* Initialize M2M destination fmt */
> > + ctx->dst_fmt.buffersize = fw_param_fmts[0].buffersize;
> > + ctx->dst_fmt.dataformat = fw_param_fmts[0].dataformat;
>
> Why not just make dst_fmt a pointer and assign &fw_params_fmt[0] there?
>
fw_params_fmt[] will be removed and assign the const value here.
ctx->dst_fmt.buffersize = sizeof(struct fd_user_output);
ctx->dst_fmt.dataformat = V4L2_META_FMT_MTFD_RESULT;
> > +}
> > +
> > +/*
> > + * V4L2 file operations.
> > + */
> > +static int mtk_vfd_open(struct file *filp)
> > +{
> > + struct mtk_fd_dev *fd_dev = video_drvdata(filp);
> > + struct video_device *vdev = video_devdata(filp);
> > + struct mtk_fd_ctx *ctx;
> > + int ret;
> > +
> > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx)
> > + return -ENOMEM;
> > +
> > + ctx->fd_dev = fd_dev;
> > + ctx->dev = &fd_dev->pdev->dev;
> > +
> > + v4l2_fh_init(&ctx->fh, vdev);
> > + filp->private_data = &ctx->fh;
> > +
> > + init_ctx_fmt(ctx);
> > +
> > + ret = mtk_fd_ctrls_setup(ctx);
> > + if (ret) {
> > + dev_err(ctx->dev, "Failed to set up controls:%d\n", ret);
> > + goto err_fh_ctrl;
> > + }
> > +
> > + ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(fd_dev->m2m_dev, ctx,
> > + &mtk_fd_queue_init);
> > + if (IS_ERR(ctx->fh.m2m_ctx)) {
> > + ret = PTR_ERR(ctx->fh.m2m_ctx);
> > + goto err_init_ctx;
> > + }
> > +
> > + v4l2_fh_add(&ctx->fh);
> > +
> > + return 0;
> > +
> > +err_init_ctx:
> > + v4l2_ctrl_handler_free(&ctx->hdl);
> > +err_fh_ctrl:
> > + v4l2_fh_exit(&ctx->fh);
> > + kfree(ctx);
> > +
> > + return ret;
> > +}
> > +
> > +static int mtk_vfd_release(struct file *filp)
> > +{
> > + struct mtk_fd_ctx *ctx = container_of(filp->private_data,
> > + struct mtk_fd_ctx, fh);
> > +
> > + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> > +
> > + v4l2_ctrl_handler_free(&ctx->hdl);
> > + v4l2_fh_del(&ctx->fh);
> > + v4l2_fh_exit(&ctx->fh);
> > +
> > + kfree(ctx);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_file_operations fd_video_fops = {
> > + .owner = THIS_MODULE,
> > + .open = mtk_vfd_open,
> > + .release = mtk_vfd_release,
> > + .poll = v4l2_m2m_fop_poll,
> > + .unlocked_ioctl = video_ioctl2,
> > + .mmap = v4l2_m2m_fop_mmap,
>
> We also need the compat_ioctl here for 32-bit userspace on 64-bit kernels.
>
Done.
> > +};
> > +
> > +static void mtk_fd_device_run(void *priv)
> > +{
> > + struct mtk_fd_ctx *ctx = priv;
> > + struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > + struct fd_hw_param fd_param;
> > + void *fd_result_vaddr;
> > +
> > + src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > +
> > + memset(&fd_param, 0, sizeof(fd_param));
> > +
> > + fd_param.src_img.dma_addr =
> > + vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
> > + fd_param.user_result.dma_addr =
> > + vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
> > + fd_result_vaddr = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> > +
> > + ctx->user_param.src_img_fmt = get_fd_img_fmt(ctx->src_fmt.pixelformat);
> > + memcpy(&fd_param.user_param, &ctx->user_param, sizeof(ctx->user_param));
> > +
> > + /* Complete request controls if any */
> > + v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, &ctx->hdl);
> > +
> > + mtk_fd_hw_job_exec(&ctx->fd_dev->fd_hw, &fd_param, fd_result_vaddr);
> > +}
> > +
> > +static struct v4l2_m2m_ops fd_m2m_ops = {
> > + .device_run = mtk_fd_device_run,
> > +};
> > +
> > +static int mtk_fd_request_validate(struct media_request *req)
> > +{
> > + unsigned int count;
> > +
> > + count = vb2_request_buffer_cnt(req);
> > + if (!count)
> > + return -ENOENT;
>
> Why -ENOENT?
>
Reference the return code in vb2_request_validate()
I consider refining as following:
static int mtk_fd_request_validate(struct media_request *req)
{
if (vb2_request_buffer_cnt(req) > 1)
return -EINVAL;
return vb2_request_validate(req);
}
or maybe I don't need to worry the request count greater than 1,
just remove this function and set vb2_request_validate as .req_validate
directly?
> > + else if (count > 1)
> > + return -EINVAL;
> > +
> > + return vb2_request_validate(req);
> > +}
> > +
> > +static const struct media_device_ops fd_m2m_media_ops = {
> > + .req_validate = mtk_fd_request_validate,
> > + .req_queue = v4l2_m2m_request_queue,
> > +};
> > +
> > +static int mtk_fd_video_device_register(struct mtk_fd_dev *fd_dev)
> > +{
> > + struct video_device *vfd = &fd_dev->vfd;
> > + struct v4l2_m2m_dev *m2m_dev = fd_dev->m2m_dev;
> > + struct device *dev = &fd_dev->pdev->dev;
> > + int function, ret;
> > +
> > + vfd->fops = &fd_video_fops;
> > + vfd->release = video_device_release;
> > + vfd->lock = &fd_dev->vfd_lock;
> > + vfd->v4l2_dev = &fd_dev->v4l2_dev;
> > + vfd->vfl_dir = VFL_DIR_M2M;
> > + vfd->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_OUTPUT_MPLANE |
> > + V4L2_CAP_META_CAPTURE;
> > + vfd->ioctl_ops = &mtk_fd_v4l2_video_out_ioctl_ops;
> > +
> > + strscpy(vfd->name, "MTK-FD-V4L2", sizeof(vfd->name));
>
> Please use dev_driver_string().
>
Done.
> > + video_set_drvdata(vfd, fd_dev);
> > +
> > + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> > + if (ret) {
> > + dev_err(dev, "Failed to register video device\n");
> > + goto err_free_dev;
> > + }
> > +
> > + function = MEDIA_ENT_F_PROC_VIDEO_DECODER;
>
> MEDIA_ENT_F_PROC_VIDEO_STATISTICS fits here much more.
>
> Also nit: Any reason to have this assigned to this intermediate variable
> rather than just directly passed to the function?
>
Too long for 80 columns, I will remove the intermediate variable and
refine as following:
ret = v4l2_m2m_register_media_controller(m2m_dev, vfd,
MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
> > + ret = v4l2_m2m_register_media_controller(m2m_dev, vfd, function);
> > + if (ret) {
> > + dev_err(dev, "Failed to init mem2mem media controller\n");
> > + goto err_unreg_video;
> > + }
> > + return 0;
> > +
> > +err_unreg_video:
> > + video_unregister_device(vfd);
> > +err_free_dev:
> > + video_device_release(vfd);
> > + return ret;
> > +}
> > +
> > +static int mtk_fd_dev_v4l2_init(struct mtk_fd_dev *fd_dev)
> > +{
> > + struct media_device *mdev = &fd_dev->mdev;
> > + struct device *dev = &fd_dev->pdev->dev;
> > + int ret;
> > +
> > + ret = v4l2_device_register(dev, &fd_dev->v4l2_dev);
> > + if (ret) {
> > + dev_err(dev, "Failed to register v4l2 device\n");
> > + return ret;
> > + }
> > +
> > + fd_dev->m2m_dev = v4l2_m2m_init(&fd_m2m_ops);
> > + if (IS_ERR(fd_dev->m2m_dev)) {
> > + dev_err(dev, "Failed to init mem2mem device\n");
> > + ret = PTR_ERR(fd_dev->m2m_dev);
> > + goto fail_m2m_dev;
>
> Please name the labels after the next clean-up step to be done, not the
> failure.
>
Ok, I will refine as following:
err_unreg_vdev:
v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
video_unregister_device(&fd_dev->vfd);
video_device_release(&fd_dev->vfd);
err_cleanup_mdev:
media_device_cleanup(mdev);
v4l2_m2m_release(fd_dev->m2m_dev);
err_unreg_v4l2_dev:
v4l2_device_unregister(&fd_dev->v4l2_dev);
> > + }
> > +
> > + mdev->dev = dev;
> > + strscpy(mdev->model, "MTK-FD-V4L2", sizeof(mdev->model));
>
> Could we just use dev_driver_string() here instead?
>
Done.
> > + snprintf(mdev->bus_info, sizeof(mdev->bus_info),
> > + "platform:%s", dev_name(dev));
> > + media_device_init(mdev);
> > + mdev->ops = &fd_m2m_media_ops;
> > + fd_dev->v4l2_dev.mdev = mdev;
> > +
> > + ret = mtk_fd_video_device_register(fd_dev);
> > + if (ret) {
> > + dev_err(dev, "Failed to register video device\n");
> > + goto err_vdev;
> > + }
> > +
> > + ret = media_device_register(mdev);
> > + if (ret) {
> > + dev_err(dev, "Failed to register mem2mem media device\n");
> > + goto fail_mdev;
> > + }
> > +
> > + return 0;
> > +
> > +fail_mdev:
> > + v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
> > + video_unregister_device(&fd_dev->vfd);
> > + video_device_release(&fd_dev->vfd);
> > +err_vdev:
> > + media_device_cleanup(mdev);
> > + v4l2_m2m_release(fd_dev->m2m_dev);
> > +fail_m2m_dev:
> > + v4l2_device_unregister(&fd_dev->v4l2_dev);
> > + return ret;
> > +}
> > +
> > +static void mtk_fd_dev_v4l2_release(struct mtk_fd_dev *fd_dev)
> > +{
> > + v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
> > + video_unregister_device(&fd_dev->vfd);
> > + video_device_release(&fd_dev->vfd);
> > + media_device_cleanup(&fd_dev->mdev);
> > + v4l2_m2m_release(fd_dev->m2m_dev);
> > + v4l2_device_unregister(&fd_dev->v4l2_dev);
> > +}
> > +
> > +static irqreturn_t mtk_fd_irq(int irq, void *data)
> > +{
> > + struct mtk_fd_hw *fd_hw = (struct mtk_fd_hw *)data;
> > +
> > + fd_hw->fd_irq_result = readl(fd_hw->fd_base + FD_INT);
>
> We should be able to just handle the job completion directly from here.
>
Ok, I will try to handle the job completion from here.
> > + wake_up_interruptible(&fd_hw->wq);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_fd_hw_get_scp_mem(struct mtk_fd_hw *fd_hw,
> > + struct fd_buffer *scp_mem)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > + dma_addr_t addr;
> > + u32 size;
> > +
> > + scp_mem->scp_addr = scp_get_reserve_mem_phys(SCP_FD_MEM_ID);
> > + size = scp_get_reserve_mem_size(SCP_FD_MEM_ID);
> > + if (!scp_mem->scp_addr || !size)
> > + return -EPROBE_DEFER;
>
> Why -EPROBE_DEFER? Should we have some way to distinguish between the SCP
> driver not being initialized yet and some other error?
>
Ok, I will fix it to -ENOMEM.
Not being initialized or some other error can be found during device
probe.
(will also load scp stuff there)
> > +
> > + /* get dma addr address */
> > + addr = dma_map_page_attrs(dev, phys_to_page(scp_mem->scp_addr), 0,
> > + size, DMA_BIDIRECTIONAL,
>
> Should this be really bidirectional?
>
I will fix it to DMA_TO_DEVICE
> > + DMA_ATTR_SKIP_CPU_SYNC);
> > + if (dma_mapping_error(dev, addr)) {
> > + scp_mem->scp_addr = 0;
> > + dev_err(dev, "Failed to map scp addr\n");
> > + return -ENOMEM;
> > + }
> > + scp_mem->dma_addr = addr;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_probe(struct platform_device *pdev)
> > +{
> > + struct mtk_fd_dev *fd_dev;
>
> nit: Perhaps just fd, to have shorter code?
>
Ok, I will refine as
struct mtk_fd_dev *fd;
> > + struct device *dev = &pdev->dev;
> > + struct mtk_fd_hw *fd_hw;
> > + struct resource *res;
> > + int irq;
> > + int ret;
> > +
> > + fd_dev = devm_kzalloc(&pdev->dev, sizeof(*fd_dev), GFP_KERNEL);
> > + if (!fd_dev)
> > + return -ENOMEM;
> > +
> > + dev_set_drvdata(dev, fd_dev);
> > + fd_hw = &fd_dev->fd_hw;
> > + fd_dev->pdev = pdev;
>
> Is pdev useful for anything? Perhaps you want to store &pdev->dev instead?
>
Yes, I will store that instead.
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(dev, "no IRQ:%d resource info\n", irq);
> > + return irq;
> > + }
> > + ret = devm_request_irq(dev, irq, mtk_fd_irq, IRQF_SHARED,
> > + dev_driver_string(dev),
> > + fd_hw);
>
> Why not just fd_dev?
>
Ok, I will use fd_dev here.
> > + if (ret) {
> > + dev_err(dev, "req_irq fail:%d\n", irq);
> > + return ret;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + fd_hw->fd_base = devm_ioremap_resource(dev, res);
> > + if (!fd_hw->fd_base) {
> > + dev_err(dev, "unable to get fd reg base\n");
> > + return PTR_ERR(fd_hw->fd_base);
> > + }
> > +
> > + fd_hw->fd_clk = devm_clk_get(dev, "fd");
> > + if (IS_ERR(fd_hw->fd_clk)) {
> > + dev_err(dev, "cannot get fd_clk_img_fd clock\n");
> > + return PTR_ERR(fd_hw->fd_clk);
> > + }
> > +
> > + ret = mtk_fd_hw_get_scp_mem(fd_hw, &fd_hw->scp_mem);
> > + if (ret) {
> > + dev_err(dev, "scp memory init failed: %d\n", ret);
>
> nit: Could we make the error messages a bit more consistent? For example
> "Failed to get IRQ resource (%d)", "Failed to request IRQ (%d)", "Failed to
> ... (%d)", etc.
>
Ok, I will fix it.
> > + return ret;
> > + }
> > +
> > + atomic_set(&fd_hw->fd_user_cnt, 0);
> > + init_waitqueue_head(&fd_hw->wq);
> > + mutex_init(&fd_dev->vfd_lock);
> > + mutex_init(&fd_hw->fd_hw_lock);
> > + pm_runtime_enable(dev);
> > +
> > + ret = mtk_fd_dev_v4l2_init(fd_dev);
> > + if (ret) {
> > + mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> > + mutex_destroy(&fd_dev->vfd_lock);
> > + pm_runtime_disable(&pdev->dev);
>
> Please move the clean-up to an error path on the bottom of the function.
>
Done.
> > + dev_err(dev, "v4l2 init failed: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_remove(struct platform_device *pdev)
> > +{
> > + struct mtk_fd_dev *fd_dev = dev_get_drvdata(&pdev->dev);
> > +
> > + mtk_fd_dev_v4l2_release(fd_dev);
> > + mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> > + mutex_destroy(&fd_dev->vfd_lock);
> > + pm_runtime_disable(&pdev->dev);
>
> The order of calls in remove should normally be the opposite to probe.
>
Ok, I will refine as following
mtk_fd_dev_v4l2_release(fd_dev);
pm_runtime_disable(&pdev->dev);
mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
mutex_destroy(&fd_dev->vfd_lock);
rproc_put(fd_dev->fd_hw.rproc_handle);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_suspend(struct device *dev)
> > +{
> > + struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
> > +
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
> > + /* suspend FD HW */
> > + writel(0x0, fd_dev->fd_hw.fd_base + FD_HW_ENABLE);
> > + writel(0x0, fd_dev->fd_hw.fd_base + FD_INT_EN);
> > + clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_resume(struct device *dev)
> > +{
> > + struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
> > + ret = clk_prepare_enable(fd_dev->fd_hw.fd_clk);
> > + if (ret < 0) {
> > + dev_dbg(dev, "open fd clk failed\n");
> > + clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> > + }
> > +
> > + /* resume FD HW */
> > + writel(ENABLE_FD, fd_dev->fd_hw.fd_base + FD_HW_ENABLE);
> > + if (fd_dev->fd_hw.state == FD_ENQ)
> > + writel(0x1, fd_dev->fd_hw.fd_base + FD_INT_EN);
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_fd_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mtk_fd_suspend, mtk_fd_resume)
> > + SET_RUNTIME_PM_OPS(mtk_fd_suspend, mtk_fd_resume, NULL)
>
> We need separate runtime and system PM ops. Their behavior is expected to be
> different.
>
> For runtime PM ops, you the functions should just unconditionally power on
> or power off the device.
>
> For system PM ops, suspend needs to make sure that no further job is queued
> to the hardware and that any job being already processed by the hardware is
> completed. Resume needs to resume the processing if there are any further
> jobs to be queued to the hardware.
>
Ok, for runtime suspend/resume I will add the following functions:
static int mtk_fd_runtime_suspend(struct device *dev)
{
struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
dev_dbg(dev, "%s:disable clock\n", __func__);
clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
return 0;
}
static int mtk_fd_runtime_resume(struct device *dev)
{
struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
int ret;
dev_dbg(dev, "%s:enable clock\n", __func__);
ret = clk_prepare_enable(fd_dev->fd_hw.fd_clk);
if (ret < 0) {
dev_err(dev, "Failed to open fd clk:%d\n", ret);
return ret;
}
return 0;
}
Best regards,
Jerry
> > +};
> > +
> > +static const struct of_device_id mtk_fd_of_ids[] = {
> > + { .compatible = "mediatek,mt8183-fd", },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_fd_of_ids);
> > +
> > +static struct platform_driver mtk_fd_driver = {
> > + .probe = mtk_fd_probe,
> > + .remove = mtk_fd_remove,
> > + .driver = {
> > + .name = "mtk-fd-4.0",
> > + .of_match_table = of_match_ptr(mtk_fd_of_ids),
> > + .pm = &mtk_fd_pm_ops,
> > + }
> > +};
> > +module_platform_driver(mtk_fd_driver);
> > +
> > +MODULE_DESCRIPTION("Mediatek FD driver");
> > +MODULE_LICENSE("GPL");
>
> GPL v2
>
> Best regards,
> Tomasz
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
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