* [PATCH v2] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay
From: Javi Merino @ 2016-12-05 10:09 UTC (permalink / raw)
To: Javier Martinez Canillas, Sakari Ailus
Cc: linux-media, linux-kernel, devicetree, Javi Merino,
Mauro Carvalho Chehab
In asds configured with V4L2_ASYNC_MATCH_OF, the v4l2 subdev can be
part of a devicetree overlay, for example:
&media_bridge {
...
my_port: port@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
ep: endpoint@0 {
remote-endpoint = <&camera0>;
};
};
};
/ {
fragment@0 {
target = <&i2c0>;
__overlay__ {
my_cam {
compatible = "foo,bar";
port {
camera0: endpoint {
remote-endpoint = <&my_port>;
...
};
};
};
};
};
};
Each time the overlay is applied, its of_node pointer will be
different. We are not interested in matching the pointer, what we
want to match is that the path is the one we are expecting. Change to
use of_node_cmp() so that we continue matching after the overlay has
been removed and reapplied.
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Javier Martinez Canillas <javier@osg.samsung.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Javi Merino <javi.merino@kernel.org>
---
drivers/media/v4l2-core/v4l2-async.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 5bada20..d33a17c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
{
- return sd->of_node == asd->match.of.node;
+ return !of_node_cmp(of_node_full_name(sd->of_node),
+ of_node_full_name(asd->match.of.node));
}
static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 2/8] dt-bindings: document the STM32 RTC bindings
From: Amelie DELAUNAY @ 2016-12-05 10:14 UTC (permalink / raw)
To: Alexandre Belloni
Cc: a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
Alexandre TORGUE, linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org,
rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Gabriel FERNANDEZ
In-Reply-To: <20161205100630.n3gldznf524pucjm-m++hUPXGwpdeoWH0uzbU5w@public.gmane.org>
Hi Alexandre,
Thanks for reviewing
On 12/05/2016 11:06 AM, Alexandre Belloni wrote:
> Hi,
>
> On 02/12/2016 at 15:09:55 +0100, Amelie Delaunay wrote :
>> This patch adds documentation of device tree bindings for the STM32 RTC.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>
>> ---
>> .../devicetree/bindings/rtc/st,stm32-rtc.txt | 31 ++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt b/Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
>> new file mode 100644
>> index 0000000..4578838
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
>> @@ -0,0 +1,31 @@
>> +STM32 Real Time Clock
>> +
>> +Required properties:
>> +- compatible: "st,stm32-rtc".
>> +- reg: address range of rtc register set.
>> +- clocks: reference to the clock entry ck_rtc.
>> +- clock-names: name of the clock used. Should be "ck_rtc".
>
> Is this name really useful?
You're right, not useful.
>
>> +- interrupt-parent: phandle for the interrupt controller.
>> +- interrupts: rtc alarm interrupt.
>> +- interrupt-names: rtc alarm interrupt name, should be "alarm".
>
> Same comment, is this name really useful?
Ditto.
>
>
Best regards,
Amelie
--
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH v1 2/2] crypto: mediatek - add DT bindings documentation
From: Matthias Brugger @ 2016-12-05 10:18 UTC (permalink / raw)
To: Ryder Lee, Herbert Xu, David S. Miller
Cc: devicetree, linux-mediatek, linux-kernel, linux-crypto,
linux-arm-kernel, Sean Wang, Roy Luo
In-Reply-To: <1480921284-45827-3-git-send-email-ryder.lee@mediatek.com>
On 05/12/16 08:01, Ryder Lee wrote:
> Add DT bindings documentation for the crypto driver
>
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
> .../devicetree/bindings/crypto/mediatek-crypto.txt | 32 ++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/crypto/mediatek-crypto.txt
>
> diff --git a/Documentation/devicetree/bindings/crypto/mediatek-crypto.txt b/Documentation/devicetree/bindings/crypto/mediatek-crypto.txt
> new file mode 100644
> index 0000000..8b1db08
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/mediatek-crypto.txt
> @@ -0,0 +1,32 @@
> +MediaTek cryptographic accelerators
> +
> +Required properties:
> +- compatible: Should be "mediatek,mt7623-crypto"
Do you know how big the difference is between the crypto engine for
mt7623/mt2701/mt8521p in comparison, let's say mt8173 or mt6797?
Do this SoCs have a crypot engine? If so and they are quite similar, we
might think of adding a mtk-crypto binding and add soc specific bindings.
Regards,
Matthias
> +- reg: Address and length of the register set for the device
> +- interrupts: Should contain the five crypto engines interrupts in numeric
> + order. These are global system and four descriptor rings.
> +- clocks: the clock used by the core
> +- clock-names: the names of the clock listed in the clocks property. These are
> + "ethif", "cryp"
> +- power-domains: Must contain a reference to the PM domain.
> +
> +
> +Optional properties:
> +- interrupt-parent: Should be the phandle for the interrupt controller
> + that services interrupts for this device
> +
> +
> +Example:
> + crypto: crypto@1b240000 {
> + compatible = "mediatek,mt7623-crypto";
> + reg = <0 0x1b240000 0 0x20000>;
> + interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 83 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 84 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 91 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 97 IRQ_TYPE_LEVEL_LOW>;
> + clocks = <&topckgen CLK_TOP_ETHIF_SEL>,
> + <ðsys CLK_ETHSYS_CRYPTO>;
> + clock-names = "ethif","cryp";
> + power-domains = <&scpsys MT2701_POWER_DOMAIN_ETH>;
> + };
>
^ permalink raw reply
* Re: [RFC PATCH 2/2] Documentation: devictree: Add macb mdio bindings
From: Nicolas Ferre @ 2016-12-05 10:23 UTC (permalink / raw)
To: Harini Katakam, Rob Herring
Cc: Harini Katakam, David Miller, Pawel Moll, Mark Rutland,
ijc+devicetree@hellion.org.uk, Kumar Gala, Boris Brezillon,
alexandre.belloni, netdev, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, michals@xilinx.com
In-Reply-To: <CAFcVECJRbgAxQMbu7jABHjkiicD1Ysp7WmDMoUzs2P5qn26uqw@mail.gmail.com>
Le 05/12/2016 à 03:55, Harini Katakam a écrit :
> Hi Rob,
>
>
> Thanks for the review.
> On Sun, Dec 4, 2016 at 3:05 AM, Rob Herring <robh@kernel.org> wrote:
>> On Mon, Nov 28, 2016 at 03:19:27PM +0530, Harini Katakam wrote:
> <snip>
>>> +Required properties:
>>> +- compatible: Should be "cdns,macb-mdio"
>>
>> Only one version ever? This needs more specific compatible strings.
>>
>
> This is part of the Cadence MAC in a way.
> I can use revision number from the Cadence spec I was working with.
> But it is not necessarily specific that version.
Yes it is:
you must specify the first precise SoC needing/implementing it:
"cdns,zynqmp-gem-mdio" or "xlnx,zynq-gem-mdio" or anything looking like
this.
So, if an Atmel implementation is slightly different, we can still use
our own "atmel,sama5d2-gem-mdio" or "cdns,sama5d2-gem-mdio"
compatibility string.
On the other hand, if it's strictly the same, we can use the
"xlnx,zynq-gem-mdio" compatibility without any problem...
> I'll take care of the other comments in the next version.
>
> Regards,
> Harini
>
--
Nicolas Ferre
^ permalink raw reply
* Re: [PATCH 06/11] ARM: dts: imx: Add imx6sll EVK board dts support
From: Fabio Estevam @ 2016-12-05 10:25 UTC (permalink / raw)
To: Bai Ping
Cc: Mark Rutland, devicetree@vger.kernel.org, Philipp Zabel,
Michael Turquette, Daniel Lezcano, Stephen Boyd, linux-clk,
linux-gpio@vger.kernel.org, robh+dt@kernel.org, Sascha Hauer,
Fabio Estevam, Thomas Gleixner, Shawn Guo, Linus Walleij,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1480660774-25055-7-git-send-email-ping.bai@nxp.com>
On Fri, Dec 2, 2016 at 4:39 AM, Bai Ping <ping.bai@nxp.com> wrote:
> Add basic dts support for imx6sll EVK baoard.
s/baord/board
> + battery: max8903@0 {
> + compatible = "fsl,max8903-charger";
> + pinctrl-names = "default";
> + dok_input = <&gpio4 13 1>;
> + uok_input = <&gpio4 13 1>;
> + chg_input = <&gpio4 15 1>;
> + flt_input = <&gpio4 14 1>;
> + fsl,dcm_always_high;
> + fsl,dc_valid;
> + fsl,adc_disable;
These properties do not exist in mainline, please remove them.
> + status = "okay";
> + };
> +
> + pxp_v4l2_out {
> + compatible = "fsl,imx6sl-pxp-v4l2";
> + status = "okay";
> + };
We don't have pxp support in mainline kernel, please remove it.
> +
> + regulators {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <0>;
Please remove it and place the regulator nodes directly as below:
> +
> + reg_usb_otg1_vbus: regulator@0 {
> + compatible = "regulator-fixed";
> + reg = <0>;
> + regulator-name = "usb_otg1_vbus";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + gpio = <&gpio4 0 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + };
reg_usb_otg1_vbus: regulator-usb-otg1-vbus {
compatible = "regulator-fixed";
regulator-name = "usb_otg1_vbus";
regulator-min-microvolt = <5000000>;
regulator-max-microvolt = <5000000>;
gpio = <&gpio4 0 GPIO_ACTIVE_HIGH>;
enable-active-high;
};
> +&cpu0 {
> + arm-supply = <&sw1a_reg>;
> + soc-supply = <&sw1c_reg>;
> +};
This is only for LDO bypass mode, right? We don't support LDO-bypass
in mainline.
> +&gpc {
> + fsl,ldo-bypass = <1>;
We don't support ldo-bypass in mainline, please remove it.
> +&pxp {
> + status = "okay";
> +};
We don't support PXP in mainline, please remove it.
^ permalink raw reply
* Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings
From: Thierry Reding @ 2016-12-05 10:50 UTC (permalink / raw)
To: Lee Jones
Cc: mark.rutland, devicetree, linaro-kernel, lars, alexandre.torgue,
linux-pwm, linux-iio, pmeerw, arnaud.pouliquen, linux-kernel,
robh+dt, linux-arm-kernel, Benjamin Gaignard, knaack.h,
gerald.baeza, fabrice.gasnier, linus.walleij, jic23,
Benjamin Gaignard
In-Reply-To: <20161205083535.GB14004@dell>
[-- Attachment #1.1: Type: text/plain, Size: 4619 bytes --]
On Mon, Dec 05, 2016 at 08:35:35AM +0000, Lee Jones wrote:
> On Mon, 05 Dec 2016, Thierry Reding wrote:
>
> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> > > Define bindings for pwm-stm32
> > >
> > > version 2:
> > > - use parameters instead of compatible of handle the hardware configuration
> > >
> > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> > > ---
> > > .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++++++++++++++++++++++
> > > 1 file changed, 38 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > > new file mode 100644
> > > index 0000000..575b9fb
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > > @@ -0,0 +1,38 @@
> > > +STMicroelectronics PWM driver bindings for STM32
> >
> > Technically this bindings describe devices, so "driver binding" is a
> > somewhat odd wording. Perhaps:
> >
> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> >
> > ?
> >
> > > +
> > > +Must be a sub-node of STM32 general purpose timer driver
> > > +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
> >
> > Again, "driver parent node" is odd. Perhaps:
> >
> > Must be a sub-node of an STM32 General Purpose Timer device tree
> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
> > the parent node.
> >
> > ?
> >
> > > +Required parameters:
> > > +- compatible: Must be "st,stm32-pwm"
> > > +- pinctrl-names: Set to "default".
> > > +- pinctrl-0: List of phandles pointing to pin configuration nodes
> > > + for PWM module.
> > > + For Pinctrl properties, please refer to [1].
> >
> > Your indentation and capitalization are inconsistent. Also, please refer
> > to the pinctrl bindings by relative path and inline, rather than as a
> > footnote reference.
> >
> > > +
> > > +Optional parameters:
> > > +- st,breakinput: Set if the hardware have break input capabilities
> > > +- st,breakinput-polarity: Set break input polarity. Default is 0
> > > + The value define the active polarity:
> > > + - 0 (active LOW)
> > > + - 1 (active HIGH)
> >
> > Could we fold these into a single property? If st,breakinput-polarity is
> > not present it could simply mean that there is no break input, and if it
> > is present you don't have to rely on a default.
> >
> > > +- st,pwm-num-chan: Number of available PWM channels. Default is 0.
> >
> > The pwm- prefix is rather redundant since the node is already named pwm.
> > Why not simply st,channels? Or simply channels, since it's not really
> > anything specific to this hardware.
> >
> > Come to think of it, might be worth having a discussion with our DT
> > gurus about what their stance is on using the # as prefix for numbers
> > (such as in #address-cells or #size-cells). This could be #channels to
> > mark it more explicitly as representing a count.
>
> Unfortunately that ship has sailed.
>
> st,pwm-num-chan already exists (with your blessing). It's usually
I think I did at the time object, though very mildly. The property here
is somewhat different, though. For one this is a PWM specific node, so
the pwm- prefix is completely redundant. Also for pwm-sti where you had
introduced st,pwm-num-chan, the property denoted how many PWM channels
vs. capture channels (st,capture-num-chan) the device was supposed to
use. Here there are only one type of channels.
> suggested to reuse exiting properties when writing new bindings.
Given the above I think this case is different. Further my understanding
is that the desire to reuse existing properties is primarily for generic
properties. Vendor specific properties are always going to have to be
defined in the specific bindings, so it doesn't matter very much whether
they are reused or not.
Lastly, I think st,pwm-num-chan is not optimal, and while it isn't very
bad either, I do believe that when we see ways of improving things then
we should do so, regardless of whether existing ways to describe things
already exist. Especially if it comes at no additional cost.
All of that said, this is my opinion and if everybody thinks that the
st,pwm-num-chan is the better choice I'll merge it. Anyway, we'll need
the Acked-by from one of the device tree bindings maintainers and I'd
like to see at least an attempt at a discussion.
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
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] i2c: rk3x: keep i2c irq ON in suspend
From: Heiko Stuebner @ 2016-12-05 10:54 UTC (permalink / raw)
To: David Wu
Cc: wsa-z923LK4zBo2bacvFa/9K2g, dianders-F7+t8E8rja9g9hUCZPvPmw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480924979-13450-1-git-send-email-david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Hi David,
Am Montag, 5. Dezember 2016, 16:02:59 CET schrieb David Wu:
> During suspend there may still be some i2c access happening.
> And if we don't keep i2c irq ON, there may be i2c access timeout if
> i2c is in irq mode of operation.
can you describe the issue you're trying to fix a bit more please?
I.e. I'd think the i2c-core does suspend i2c-client devices first, so that
these should be able to finish up their ongoing transfers and not start any
new ones instead?
Your irq can still happen slightly after the system started going to actually
sleep, so to me it looks like you just widened the window where irqs can be
handled. Especially as your irq could also just simply stem from the start
state, so you cannot even be sure if your transaction actually is finished.
So to me it looks like the i2c-connected device driver should be fixed instead?
In the past I solved this for example in the zforce_ts driver [0] to
prevent i2c transfers from happening during suspend.
Heiko
[0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/zforce_ts.c
>
> Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> drivers/i2c/busses/i2c-rk3x.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index df22066..67af32a 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -1261,7 +1261,7 @@ static int rk3x_i2c_probe(struct platform_device
> *pdev) }
>
> ret = devm_request_irq(&pdev->dev, irq, rk3x_i2c_irq,
> - 0, dev_name(&pdev->dev), i2c);
> + IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c);
> if (ret < 0) {
> dev_err(&pdev->dev, "cannot request IRQ\n");
> return ret;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board
From: Icenowy Zheng @ 2016-12-05 11:01 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mark Rutland, devicetree@vger.kernel.org, Vishnu Patekar,
Arnd Bergmann, linux-doc@vger.kernel.org, André Przywara,
Jonathan Corbet, linux-kernel, Russell King, Hans de Goede,
Chen-Yu Tsai, "linux-arm-kernel@lists.infradead.org"
In-Reply-To: <20161205094023.mtymfvpmca4x3ohh@lukather>
05.12.2016, 17:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> On Mon, Dec 05, 2016 at 04:59:44PM +0800, Icenowy Zheng wrote:
>> 2016年12月5日 16:52于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
>> >
>> > On Fri, Dec 02, 2016 at 10:22:30PM +0800, Icenowy Zheng wrote:
>> > >
>> > >
>> > > 01.12.2016, 17:36, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
>> > > > On Mon, Nov 28, 2016 at 12:29:07AM +0000, André Przywara wrote:
>> > > >> > Something more interesting happened.
>> > > >> >
>> > > >> > Xunlong made a add-on board for Orange Pi Zero, which exposes the
>> > > >> > two USB Controllers exported at expansion bus as USB Type-A
>> > > >> > connectors.
>> > > >> >
>> > > >> > Also it exposes a analog A/V jack and a microphone.
>> > > >> >
>> > > >> > Should I enable {e,o}hci{2.3} in the device tree?
>> > > >>
>> > > >> Actually we should do this regardless of this extension board. The USB
>> > > >> pins are not multiplexed and are exposed on user accessible pins (just
>> > > >> not soldered, but that's a detail), so I think they qualify for DT
>> > > >> enablement. And even if a user can't use them, it doesn't hurt to have
>> > > >> them (since they are not multiplexed).
>> > > >
>> > > > My main concern about this is that we'll leave regulators enabled by
>> > > > default, for a minority of users. And that minority will prevent to do
>> > > > a proper power management when the times come since we'll have to keep
>> > > > that behaviour forever.
>> > >
>> > > I think these users can add a 'fdt set /xxx/xxx status "disabled" ' .
>> >
>> > You can't ask that from the majority of users. These users will take
>> > debian or fedora, install it, and expect everything to work
>> > properly. I would make the opposite argument actually. If someone is
>> > knowledgeable enough to solder the USB pins a connector, then (s)he'll
>> > be able to make that u-boot call.
>>
>> Now (s)he do not need soldering.
>>
>> (S)he needs only paying $1.99 more to Xunlong to get the expansion
>> board, and insert it on the OPi Zero.
>
> Which is going to require an overlay anyway, so we could have the USB
> bits in there too.
If so, I think the [PATCH -next v3 2/2] is ready to be merged ;-)
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
_______________________________________________
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 v3 4/7] PWM: add pwm driver for stm32 plaftorm
From: Benjamin Gaignard @ 2016-12-05 11:02 UTC (permalink / raw)
To: Thierry Reding
Cc: Lee Jones, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, Mark Rutland,
alexandre.torgue-qxv4g6HH51o, devicetree-u79uwXL29TY76Z2rM5mHXA,
Linux Kernel Mailing List, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
Jonathan Cameron, knaack.h-Mmb7MZpHnFY, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen, Linus Walleij,
Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <20161205072357.GB18069-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org>
2016-12-05 8:23 GMT+01:00 Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> On Fri, Dec 02, 2016 at 11:17:19AM +0100, Benjamin Gaignard wrote:
>> This driver add support for pwm driver on stm32 platform.
>
> "adds". Also please use PWM in prose because it's an abbreviation.
>
>> The SoC have multiple instances of the hardware IP and each
>> of them could have small differences: number of channels,
>> complementary output, counter register size...
>> Use DT parameters to handle those differentes configuration
>
> "different configurations"
ok
>
>>
>> version 2:
>> - only keep one comptatible
>> - use DT paramaters to discover hardware block configuration
>
> "parameters"
ok
>
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
>> ---
>> drivers/pwm/Kconfig | 8 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-stm32.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 294 insertions(+)
>> create mode 100644 drivers/pwm/pwm-stm32.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index bf01288..a89fdba 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -388,6 +388,14 @@ config PWM_STI
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-sti.
>>
>> +config PWM_STM32
>> + bool "STMicroelectronics STM32 PWM"
>> + depends on ARCH_STM32
>> + depends on OF
>> + select MFD_STM32_GP_TIMER
>
> Should that be a "depends on"?
I think select is fine because the wanted feature is PWM not the mfd part
>
>> + help
>> + Generic PWM framework driver for STM32 SoCs.
>> +
>> config PWM_STMPE
>> bool "STMPE expander PWM export"
>> depends on MFD_STMPE
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 1194c54..5aa9308 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
>> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
>> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
>> obj-$(CONFIG_PWM_STI) += pwm-sti.o
>> +obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
>> obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
>> obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
>> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
>> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
>> new file mode 100644
>> index 0000000..a362f63
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-stm32.c
>> @@ -0,0 +1,285 @@
>> +/*
>> + * Copyright (C) STMicroelectronics 2016
>> + * Author: Gerald Baeza <gerald.baeza-qxv4g6HH51o@public.gmane.org>
>
> Could use a blank line between the above. Also, please use a single
> space after : for consistency.
ok
>
>> + * License terms: GNU General Public License (GPL), version 2
>
> Here too.
OK
>
>> + *
>> + * Inspired by timer-stm32.c from Maxime Coquelin
>> + * pwm-atmel.c from Bo Shen
>> + */
>> +
>> +#include <linux/of.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>
> Please sort these alphabetically.
ok
>> +
>> +#include <linux/mfd/stm32-gptimer.h>
>> +
>> +#define DRIVER_NAME "stm32-pwm"
>> +
>> +#define CAP_COMPLEMENTARY BIT(0)
>> +#define CAP_32BITS_COUNTER BIT(1)
>> +#define CAP_BREAKINPUT BIT(2)
>> +#define CAP_BREAKINPUT_POLARITY BIT(3)
>
> Just make these boolean. Makes the conditionals a lot simpler to read.
I will do that for v4
>> +
>> +struct stm32_pwm_dev {
>
> No need for the _dev suffix.
>
>> + struct device *dev;
>> + struct clk *clk;
>> + struct regmap *regmap;
>> + struct pwm_chip chip;
>
> It's slightly more efficient to put this as first field because then
> to_stm32_pwm() becomes a no-op.
Ok I will remove it
>
>> + int caps;
>> + int npwm;
>
> unsigned int, please.
>
>> + u32 polarity;
>
> Maybe use a prefix here to stress that it is the polarity of the
> complementary output. Otherwise one might take it for the PWM signal's
> polarity that's already part of the PWM state.
I will rename it
>
>> +};
>> +
>> +#define to_stm32_pwm_dev(x) container_of(chip, struct stm32_pwm_dev, chip)
>
> Please turn this into a static inline.
with putting struct pwm_chip as first filed I will just cast the structure
>> +
>> +static u32 __active_channels(struct stm32_pwm_dev *pwm_dev)
>
> No need for a __ prefix.
I wlll remove it
>
>> +{
>> + u32 ccer;
>> +
>> + regmap_read(pwm_dev->regmap, TIM_CCER, &ccer);
>> +
>> + return ccer & TIM_CCER_CCXE;
>> +}
>> +
>> +static int write_ccrx(struct stm32_pwm_dev *dev, struct pwm_device *pwm,
>> + u32 ccr)
>
> u32 value, perhaps? I first mistook this to be a register offset.
OK
>
>> +{
>> + switch (pwm->hwpwm) {
>> + case 0:
>> + return regmap_write(dev->regmap, TIM_CCR1, ccr);
>> + case 1:
>> + return regmap_write(dev->regmap, TIM_CCR2, ccr);
>> + case 2:
>> + return regmap_write(dev->regmap, TIM_CCR3, ccr);
>> + case 3:
>> + return regmap_write(dev->regmap, TIM_CCR4, ccr);
>> + }
>> + return -EINVAL;
>> +}
>> +
>> +static int stm32_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> + int duty_ns, int period_ns)
>
> Please implement this as an atomic PWM driver, I don't want new drivers
> to use the legacy callbacks.
Ok I will just use .apply ops.
>
>> +{
>> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
>
> I think something like "stm", or "priv" would be more appropriate here.
> If you ever need access to a struct device, you'll be hard-pressed to
> find a good name for it.
I will use priv
>
>> + unsigned long long prd, div, dty;
>> + int prescaler = 0;
>
> If this can never be negative, please make it unsigned int.
>
>> + u32 max_arr = 0xFFFF, ccmr, mask, shift, bdtr;
>> +
>> + if (dev->caps & CAP_32BITS_COUNTER)
>> + max_arr = 0xFFFFFFFF;
>
> I prefer lower-case hexadecimal digits.
I have found a way to remove this code
>
>> +
>> + /* Period and prescaler values depends of clock rate */
>
> "depend on"
fixed
>
>> + div = (unsigned long long)clk_get_rate(dev->clk) * period_ns;
>> +
>> + do_div(div, NSEC_PER_SEC);
>> + prd = div;
>> +
>> + while (div > max_arr) {
>> + prescaler++;
>> + div = prd;
>> + do_div(div, (prescaler + 1));
>> + }
>> + prd = div;
>
> Blank line after blocks, please.
fixed
>
>> +
>> + if (prescaler > MAX_TIM_PSC) {
>> + dev_err(chip->dev, "prescaler exceeds the maximum value\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* All channels share the same prescaler and counter so
>> + * when two channels are active at the same we can't change them
>> + */
>
> This isn't proper block comment style. Also, please use all of the
> columns at your disposal.
done
>
>> + if (__active_channels(dev) & ~(1 << pwm->hwpwm * 4)) {
>> + u32 psc, arr;
>> +
>> + regmap_read(dev->regmap, TIM_PSC, &psc);
>> + regmap_read(dev->regmap, TIM_ARR, &arr);
>> +
>> + if ((psc != prescaler) || (arr != prd - 1))
>> + return -EINVAL;
>
> Maybe -EBUSY to differentiate from other error cases?
done
>
>> + }
>> +
>> + regmap_write(dev->regmap, TIM_PSC, prescaler);
>> + regmap_write(dev->regmap, TIM_ARR, prd - 1);
>> + regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
>> +
>> + /* Calculate the duty cycles */
>> + dty = prd * duty_ns;
>> + do_div(dty, period_ns);
>> +
>> + write_ccrx(dev, pwm, dty);
>> +
>> + /* Configure output mode */
>> + shift = (pwm->hwpwm & 0x1) * 8;
>> + ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
>> + mask = 0xFF << shift;
>> +
>> + if (pwm->hwpwm & 0x2)
>
> This looks as though TIM_CCMR1 is used for channels 0 and 1, while
> TIM_CCMR2 is used for channels 2 and 3. Wouldn't it be more natural to
> make the conditional above:
>
> if (pwm->hwpwm >= 2)
>
> ? Or perhaps better yet:
>
> if (pwm->hwpwm < 2)
> /* update TIM_CCMR1 */
> else
> /* update TIM_CCMR2 */
I will code it that, thanks.
>
> The other alternative, which might make the code slightly more readable,
> would be to get rid of all these conditionals by parameterizing the
> offsets per PWM channel. The PWM subsystem has a means of storing per-
> channel chip-specific data (see pwm_{set,get}_chip_data()). It might be
> a little overkill for this particular driver, given that the number of
> conditionals is fairly small.
>
>> + regmap_update_bits(dev->regmap, TIM_CCMR2, mask, ccmr);
>> + else
>> + regmap_update_bits(dev->regmap, TIM_CCMR1, mask, ccmr);
>> +
>> + if (!(dev->caps & CAP_BREAKINPUT))
>> + return 0;
>
> If you make these capabilities boolean, it becomes much more readable,
> especially for negations:
Yes I will fix that
>
> if (!dev->has_breakinput)
>
>> +
>> + bdtr = TIM_BDTR_MOE | TIM_BDTR_AOE;
>> +
>> + if (dev->caps & CAP_BREAKINPUT_POLARITY)
>> + bdtr |= TIM_BDTR_BKE;
>> +
>> + if (dev->polarity)
>> + bdtr |= TIM_BDTR_BKP;
>> +
>> + regmap_update_bits(dev->regmap, TIM_BDTR,
>> + TIM_BDTR_MOE | TIM_BDTR_AOE |
>> + TIM_BDTR_BKP | TIM_BDTR_BKE,
>> + bdtr);
>> +
>> + return 0;
>> +}
>> +
>> +static int stm32_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> + enum pwm_polarity polarity)
>> +{
>> + u32 mask;
>> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
>> +
>> + mask = TIM_CCER_CC1P << (pwm->hwpwm * 4);
>> + if (dev->caps & CAP_COMPLEMENTARY)
>> + mask |= TIM_CCER_CC1NP << (pwm->hwpwm * 4);
>> +
>> + regmap_update_bits(dev->regmap, TIM_CCER, mask,
>> + polarity == PWM_POLARITY_NORMAL ? 0 : mask);
>> +
>> + return 0;
>> +}
>> +
>> +static int stm32_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + u32 mask;
>> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
>> +
>> + clk_enable(dev->clk);
>> +
>> + /* Enable channel */
>> + mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
>> + if (dev->caps & CAP_COMPLEMENTARY)
>> + mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
>> +
>> + regmap_update_bits(dev->regmap, TIM_CCER, mask, mask);
>> +
>> + /* Make sure that registers are updated */
>> + regmap_update_bits(dev->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
>> +
>> + /* Enable controller */
>> + regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
>> +
>> + return 0;
>> +}
>> +
>> +static void stm32_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + u32 mask;
>> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
>> +
>> + /* Disable channel */
>> + mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
>> + if (dev->caps & CAP_COMPLEMENTARY)
>> + mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
>> +
>> + regmap_update_bits(dev->regmap, TIM_CCER, mask, 0);
>> +
>> + /* When all channels are disabled, we can disable the controller */
>> + if (!__active_channels(dev))
>> + regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>> +
>> + clk_disable(dev->clk);
>> +}
>
> All of the above can be folded into the ->apply() hook for atomic PWM
> support.
>
> Also, in the above you use clk_enable() in the ->enable() callback and
> clk_disable() in ->disable(). If you need the clock to access registers
> you'll have to enabled it in the others as well because there are no
> guarantees that configuration will only happen between ->enable() and
> ->disable(). Atomic PWM simplifies this a bit, but you still need to be
> careful about when to enable the clock in your ->apply() hook.
I have used regmap functions enable/disable clk for me when accessing to
the registers.
I only have to take care of clk enable/disable when PWM state change.
>
>> +
>> +static const struct pwm_ops stm32pwm_ops = {
>> + .config = stm32_pwm_config,
>> + .set_polarity = stm32_pwm_set_polarity,
>> + .enable = stm32_pwm_enable,
>> + .disable = stm32_pwm_disable,
>> +};
>
> You need to set the .owner field as well.
OK and I will remove legacy ops to use only apply.
>
>> +
>> +static int stm32_pwm_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + struct stm32_gptimer_dev *mfd = dev_get_drvdata(pdev->dev.parent);
>> + struct stm32_pwm_dev *pwm;
>> + int ret;
>> +
>> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
>> + if (!pwm)
>> + return -ENOMEM;
>> +
>> + pwm->regmap = mfd->regmap;
>> + pwm->clk = mfd->clk;
>> +
>> + if (!pwm->regmap || !pwm->clk)
>> + return -EINVAL;
>> +
>> + if (of_property_read_bool(np, "st,complementary"))
>> + pwm->caps |= CAP_COMPLEMENTARY;
>> +
>> + if (of_property_read_bool(np, "st,32bits-counter"))
>> + pwm->caps |= CAP_32BITS_COUNTER;
>> +
>> + if (of_property_read_bool(np, "st,breakinput"))
>> + pwm->caps |= CAP_BREAKINPUT;
>> +
>> + if (!of_property_read_u32(np, "st,breakinput-polarity", &pwm->polarity))
>> + pwm->caps |= CAP_BREAKINPUT_POLARITY;
>> +
>> + of_property_read_u32(np, "st,pwm-num-chan", &pwm->npwm);
>> +
>> + pwm->chip.base = -1;
>> + pwm->chip.dev = dev;
>> + pwm->chip.ops = &stm32pwm_ops;
>> + pwm->chip.npwm = pwm->npwm;
>> +
>> + ret = pwmchip_add(&pwm->chip);
>> + if (ret < 0)
>> + return ret;
>> +
>> + platform_set_drvdata(pdev, pwm);
>> +
>> + return 0;
>> +}
>> +
>> +static int stm32_pwm_remove(struct platform_device *pdev)
>> +{
>> + struct stm32_pwm_dev *pwm = platform_get_drvdata(pdev);
>> + int i;
>
> unsigned int, please.
OK
>
>> +
>> + for (i = 0; i < pwm->npwm; i++)
>> + pwm_disable(&pwm->chip.pwms[i]);
>> +
>> + pwmchip_remove(&pwm->chip);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id stm32_pwm_of_match[] = {
>> + {
>> + .compatible = "st,stm32-pwm",
>> + },
>
> The above can be collapsed into a single line.
OK
>
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_pwm_of_match);
>> +
>> +static struct platform_driver stm32_pwm_driver = {
>> + .probe = stm32_pwm_probe,
>> + .remove = stm32_pwm_remove,
>> + .driver = {
>> + .name = DRIVER_NAME,
>> + .of_match_table = stm32_pwm_of_match,
>> + },
>> +};
>
> Please don't use tabs for padding within the structure definition since
> it doesn't align properly anyway.
OK
>
>> +module_platform_driver(stm32_pwm_driver);
>> +
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>> +MODULE_DESCRIPTION("STMicroelectronics STM32 PWM driver");
>> +MODULE_LICENSE("GPL");
>
> According to the header comment this should be "GPL v2".
done
>
> Thanks,
> Thierry
^ permalink raw reply
* Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings
From: Benjamin Gaignard @ 2016-12-05 11:08 UTC (permalink / raw)
To: Thierry Reding
Cc: Lee Jones, robh+dt, Mark Rutland, alexandre.torgue, devicetree,
Linux Kernel Mailing List, linux-pwm, Jonathan Cameron, knaack.h,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
linux-arm-kernel, Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen,
Linus Walleij, Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <20161205065350.GA18069@ulmo.ba.sec>
2016-12-05 7:53 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
>> Define bindings for pwm-stm32
>>
>> version 2:
>> - use parameters instead of compatible of handle the hardware configuration
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>> .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> new file mode 100644
>> index 0000000..575b9fb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> @@ -0,0 +1,38 @@
>> +STMicroelectronics PWM driver bindings for STM32
>
> Technically this bindings describe devices, so "driver binding" is a
> somewhat odd wording. Perhaps:
>
> STMicroelectronics STM32 General Purpose Timer PWM bindings
>
> ?
done
>
>> +
>> +Must be a sub-node of STM32 general purpose timer driver
>> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
>
> Again, "driver parent node" is odd. Perhaps:
>
> Must be a sub-node of an STM32 General Purpose Timer device tree
> node. See ../mfd/stm32-general-purpose-timer.txt for details about
> the parent node.
>
> ?
done
>
>> +Required parameters:
>> +- compatible: Must be "st,stm32-pwm"
>> +- pinctrl-names: Set to "default".
>> +- pinctrl-0: List of phandles pointing to pin configuration nodes
>> + for PWM module.
>> + For Pinctrl properties, please refer to [1].
>
> Your indentation and capitalization are inconsistent. Also, please refer
> to the pinctrl bindings by relative path and inline, rather than as a
> footnote reference.
OK
>
>> +
>> +Optional parameters:
>> +- st,breakinput: Set if the hardware have break input capabilities
>> +- st,breakinput-polarity: Set break input polarity. Default is 0
>> + The value define the active polarity:
>> + - 0 (active LOW)
>> + - 1 (active HIGH)
>
> Could we fold these into a single property? If st,breakinput-polarity is
> not present it could simply mean that there is no break input, and if it
> is present you don't have to rely on a default.
I need to know if I have to activate breakinput feature and on which level
I will rewrite it like that:
Optional parameters:
- st,breakinput-polarity-high: Set if break input polarity is active
on high level.
- st,breakinput-polarity-high: Set if break input polarity is active
on low level.
>> +- st,pwm-num-chan: Number of available PWM channels. Default is 0.
>
> The pwm- prefix is rather redundant since the node is already named pwm.
> Why not simply st,channels? Or simply channels, since it's not really
> anything specific to this hardware.
>
> Come to think of it, might be worth having a discussion with our DT
> gurus about what their stance is on using the # as prefix for numbers
> (such as in #address-cells or #size-cells). This could be #channels to
> mark it more explicitly as representing a count.
>
>> +- st,32bits-counter: Set if the hardware have a 32 bits counter
>> +- st,complementary: Set if the hardware have complementary output channels
>
> "hardware has" and also maybe mention explicitly that this is a boolean
> property. Otherwise people might be left wondering what it should be set
> to. Or maybe word this differently to imply that it's boolean:
>
> - st,32bits-counter: if present, the hardware has a 32 bit counter
> - st,complementary: if present, the hardware has a complementary
> output channel
I found a way to detect, at probe time, the number of channels, counter size,
break input capability and complementary channels so I will remove
"st,breakinput", "st,32bits-counter", "st,complementary" and "st,pwm-num-chan"
parameters
>
> Thierry
^ permalink raw reply
* Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings
From: Thierry Reding @ 2016-12-05 11:23 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Lee Jones, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, Mark Rutland,
alexandre.torgue-qxv4g6HH51o, devicetree-u79uwXL29TY76Z2rM5mHXA,
Linux Kernel Mailing List, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
Jonathan Cameron, knaack.h-Mmb7MZpHnFY, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen, Linus Walleij,
Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <CA+M3ks4-JQNGYtdiXNj1J701x0njKD0_bf4yN1mLSjEcT2Z=6A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 5365 bytes --]
On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 7:53 GMT+01:00 Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> >> Define bindings for pwm-stm32
> >>
> >> version 2:
> >> - use parameters instead of compatible of handle the hardware configuration
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
> >> ---
> >> .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++++++++++++++++++++++
> >> 1 file changed, 38 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> new file mode 100644
> >> index 0000000..575b9fb
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> @@ -0,0 +1,38 @@
> >> +STMicroelectronics PWM driver bindings for STM32
> >
> > Technically this bindings describe devices, so "driver binding" is a
> > somewhat odd wording. Perhaps:
> >
> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> >
> > ?
> done
>
> >
> >> +
> >> +Must be a sub-node of STM32 general purpose timer driver
> >> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
> >
> > Again, "driver parent node" is odd. Perhaps:
> >
> > Must be a sub-node of an STM32 General Purpose Timer device tree
> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
> > the parent node.
> >
> > ?
>
> done
>
> >
> >> +Required parameters:
> >> +- compatible: Must be "st,stm32-pwm"
> >> +- pinctrl-names: Set to "default".
> >> +- pinctrl-0: List of phandles pointing to pin configuration nodes
> >> + for PWM module.
> >> + For Pinctrl properties, please refer to [1].
> >
> > Your indentation and capitalization are inconsistent. Also, please refer
> > to the pinctrl bindings by relative path and inline, rather than as a
> > footnote reference.
>
> OK
>
> >
> >> +
> >> +Optional parameters:
> >> +- st,breakinput: Set if the hardware have break input capabilities
> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
> >> + The value define the active polarity:
> >> + - 0 (active LOW)
> >> + - 1 (active HIGH)
> >
> > Could we fold these into a single property? If st,breakinput-polarity is
> > not present it could simply mean that there is no break input, and if it
> > is present you don't have to rely on a default.
>
> I need to know if I have to activate breakinput feature and on which level
> I will rewrite it like that:
> Optional parameters:
> - st,breakinput-polarity-high: Set if break input polarity is active
> on high level.
> - st,breakinput-polarity-high: Set if break input polarity is active
> on low level.
How is that different from a single property:
Optional properties:
- st,breakinput-polarity: If present, a break input is available
for the channel. In that case the property value denotes the
polarity of the break input:
- 0: active low
- 1: active high
?
> > The pwm- prefix is rather redundant since the node is already named pwm.
> > Why not simply st,channels? Or simply channels, since it's not really
> > anything specific to this hardware.
> >
> > Come to think of it, might be worth having a discussion with our DT
> > gurus about what their stance is on using the # as prefix for numbers
> > (such as in #address-cells or #size-cells). This could be #channels to
> > mark it more explicitly as representing a count.
> >
> >> +- st,32bits-counter: Set if the hardware have a 32 bits counter
> >> +- st,complementary: Set if the hardware have complementary output channels
> >
> > "hardware has" and also maybe mention explicitly that this is a boolean
> > property. Otherwise people might be left wondering what it should be set
> > to. Or maybe word this differently to imply that it's boolean:
> >
> > - st,32bits-counter: if present, the hardware has a 32 bit counter
> > - st,complementary: if present, the hardware has a complementary
> > output channel
>
> I found a way to detect, at probe time, the number of channels, counter size,
> break input capability and complementary channels so I will remove
> "st,breakinput", "st,32bits-counter", "st,complementary" and "st,pwm-num-chan"
> parameters
Oh hey, that's very neat. I suppose in that case my comment above about
the break input polarity is somewhat obsoleted. Still I think you won't
need two properties. Instead you can follow what other similar
properties have done: choose a default (often low-active) and have a
single optional property to override the default (often high-active).
In your case:
- st,breakinput-active-high: Some channels have a break input,
whose polarity will be active low by default. If this
property is present, the channel will be configured with an
active high polarity for the break input.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v3 4/7] PWM: add pwm driver for stm32 plaftorm
From: Thierry Reding @ 2016-12-05 11:30 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Mark Rutland, devicetree, Lars-Peter Clausen, alexandre.torgue,
linux-pwm, linux-iio, Linus Walleij, Arnaud Pouliquen,
Linux Kernel Mailing List, robh+dt, linux-arm-kernel,
Peter Meerwald-Stadler, knaack.h, Gerald Baeza, Fabrice Gasnier,
Lee Jones, Linaro Kernel Mailman List, Jonathan Cameron,
Benjamin Gaignard
In-Reply-To: <CA+M3ks7_utdw1c0A0m+RzuPTfvFAv5d8HoQvQDzXwHR8M6RKJA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 4037 bytes --]
On Mon, Dec 05, 2016 at 12:02:45PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 8:23 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> > On Fri, Dec 02, 2016 at 11:17:19AM +0100, Benjamin Gaignard wrote:
> >> This driver add support for pwm driver on stm32 platform.
> >
> > "adds". Also please use PWM in prose because it's an abbreviation.
> >
> >> The SoC have multiple instances of the hardware IP and each
> >> of them could have small differences: number of channels,
> >> complementary output, counter register size...
> >> Use DT parameters to handle those differentes configuration
> >
> > "different configurations"
>
> ok
>
> >
> >>
> >> version 2:
> >> - only keep one comptatible
> >> - use DT paramaters to discover hardware block configuration
> >
> > "parameters"
>
> ok
>
> >
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> >> ---
> >> drivers/pwm/Kconfig | 8 ++
> >> drivers/pwm/Makefile | 1 +
> >> drivers/pwm/pwm-stm32.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 294 insertions(+)
> >> create mode 100644 drivers/pwm/pwm-stm32.c
> >>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index bf01288..a89fdba 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -388,6 +388,14 @@ config PWM_STI
> >> To compile this driver as a module, choose M here: the module
> >> will be called pwm-sti.
> >>
> >> +config PWM_STM32
> >> + bool "STMicroelectronics STM32 PWM"
> >> + depends on ARCH_STM32
> >> + depends on OF
> >> + select MFD_STM32_GP_TIMER
> >
> > Should that be a "depends on"?
>
> I think select is fine because the wanted feature is PWM not the mfd part
I think in that case you may want to hide the MFD Kconfig option. See
Documentation/kbuild/kconfig-language.txt (notes about select) for the
details.
[...]
> >> +};
> >> +
> >> +#define to_stm32_pwm_dev(x) container_of(chip, struct stm32_pwm_dev, chip)
> >
> > Please turn this into a static inline.
>
> with putting struct pwm_chip as first filed I will just cast the structure
Don't do that, please. container_of() is still preferred because it is
correct and won't break even if you (or somebody else) changes the order
in the future. The fact that it might be optimized away is a detail, or
a micro-optimization. Force-casting is a bad idea because it won't catch
errors if for some reason the field doesn't remain in the first position
forever.
> >> +static void stm32_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> +{
> >> + u32 mask;
> >> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
> >> +
> >> + /* Disable channel */
> >> + mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
> >> + if (dev->caps & CAP_COMPLEMENTARY)
> >> + mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
> >> +
> >> + regmap_update_bits(dev->regmap, TIM_CCER, mask, 0);
> >> +
> >> + /* When all channels are disabled, we can disable the controller */
> >> + if (!__active_channels(dev))
> >> + regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_CEN, 0);
> >> +
> >> + clk_disable(dev->clk);
> >> +}
> >
> > All of the above can be folded into the ->apply() hook for atomic PWM
> > support.
> >
> > Also, in the above you use clk_enable() in the ->enable() callback and
> > clk_disable() in ->disable(). If you need the clock to access registers
> > you'll have to enabled it in the others as well because there are no
> > guarantees that configuration will only happen between ->enable() and
> > ->disable(). Atomic PWM simplifies this a bit, but you still need to be
> > careful about when to enable the clock in your ->apply() hook.
>
> I have used regmap functions enable/disable clk for me when accessing to
> the registers.
> I only have to take care of clk enable/disable when PWM state change.
Okay, that's fine then.
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
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 1/3] ARM: dts: imx6: Add Savageboard common file
From: Fabio Estevam @ 2016-12-05 11:36 UTC (permalink / raw)
To: Milo Kim
Cc: Shawn Guo, Sascha Hauer, Fabio Estevam,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel
In-Reply-To: <20161205010729.7047-2-woogyom.kim-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Sun, Dec 4, 2016 at 11:07 PM, Milo Kim <woogyom.kim-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> + regulators {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reg_3p3v: regulator@0 {
> + compatible = "regulator-fixed";
> + reg = <0>;
> + regulator-name = "3P3V";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-always-on;
> + };
Please remove the regulators container and put the regulator node
directly as follows:
reg_3p3v: regulator-3p3v {
compatible = "regulator-fixed";
regulator-name = "3P3V";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
regulator-always-on;
}
> + };
> +};
> +
> +&clks {
> + assigned-clocks = <&clks IMX6QDL_CLK_LDB_DI0_SEL>,
> + <&clks IMX6QDL_CLK_LDB_DI1_SEL>;
> + assigned-clock-parents = <&clks IMX6QDL_CLK_PLL3_USB_OTG>,
> + <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
> +};
> +
> +&fec {
> + phy-mode = "rgmii";
> + phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_HIGH>;
I think you meant
phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
> +&iomuxc {
> + savageboard {
> + pinctrl_emmc: emmcgrp {
> + fsl,pins = <
> + MX6QDL_PAD_SD4_CMD__SD4_CMD 0x17059
> + MX6QDL_PAD_SD4_CLK__SD4_CLK 0x10059
> + MX6QDL_PAD_SD4_DAT0__SD4_DATA0 0x17059
> + MX6QDL_PAD_SD4_DAT1__SD4_DATA1 0x17059
> + MX6QDL_PAD_SD4_DAT2__SD4_DATA2 0x17059
> + MX6QDL_PAD_SD4_DAT3__SD4_DATA3 0x17059
> + MX6QDL_PAD_SD4_DAT4__SD4_DATA4 0x17059
> + MX6QDL_PAD_SD4_DAT5__SD4_DATA5 0x17059
> + MX6QDL_PAD_SD4_DAT6__SD4_DATA6 0x17059
> + MX6QDL_PAD_SD4_DAT7__SD4_DATA7 0x17059
> + >;
> + };
You can remove the savegeboard level. Please check
arch/arm/boot/dts/imx6q-tbs2910.dts.
iomux usually go as the last node of the dts file.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/5] spi: Add basic support for Armada 3700 SPI Controller
From: Mark Brown @ 2016-12-05 12:05 UTC (permalink / raw)
To: Romain Perier
Cc: Mark Rutland, Andrew Lunn, Jason Cooper, Pawel Moll, devicetree,
Ian Campbell, linux-spi, Nadav Haklai, Rob Herring, Kumar Gala,
Gregory Clement, xigu, dingwei, Thomas Petazzoni,
linux-arm-kernel, Sebastian Hesselbarth
In-Reply-To: <20161201102719.4291-2-romain.perier@free-electrons.com>
[-- Attachment #1.1: Type: text/plain, Size: 1933 bytes --]
On Thu, Dec 01, 2016 at 11:27:15AM +0100, Romain Perier wrote:
> +config SPI_ARMADA_3700
> + tristate "Marvell Armada 3700 SPI Controller"
> + depends on ARCH_MVEBU && OF
Why does this not have a COMPILE_TEST dependency?
> + /* Reset SPI unit */
> + val = spireg_read(a3700_spi, A3700_SPI_IF_CFG_REG);
> + val |= A3700_SPI_SRST;
> + spireg_write(a3700_spi, A3700_SPI_IF_CFG_REG, val);
> +
> + for (i = 0; i < A3700_SPI_TIMEOUT; i++)
> + udelay(1);
Why not just do a single udelay()?
> +static irqreturn_t a3700_spi_interrupt(int irq, void *dev_id)
> +{
> + struct spi_master *master = dev_id;
> + struct a3700_spi *a3700_spi;
> + u32 cause;
> +
> + a3700_spi = spi_master_get_devdata(master);
> +
> + /* Get interrupt causes */
> + cause = spireg_read(a3700_spi, A3700_SPI_INT_STAT_REG);
> +
> + /* mask and acknowledge the SPI interrupts */
> + spireg_write(a3700_spi, A3700_SPI_INT_MASK_REG, 0);
> + spireg_write(a3700_spi, A3700_SPI_INT_STAT_REG, cause);
> +
> + /* Wake up the transfer */
> + if (a3700_spi->wait_mask & cause)
> + complete(&a3700_spi->done);
> +
> + return IRQ_HANDLED;
> +}
This reports that we handled an interrupt even if we did not in fact
handle an interrupt. It's also a bit dodgy that it doesn't check what
the interrupt was but that's less serious.
> + master->bus_num = (pdev->id != -1) ? pdev->id : 0;
At most this should be just setting the bus number to pdev->id like
other drivers do.
> + ret = clk_prepare_enable(spi->clk);
> + if (ret) {
> + dev_err(dev, "could not prepare clk: %d\n", ret);
> + goto error;
> + }
I'd expect the hardware prepare/unprepare to be managing the enable and
disable of the clock in order to save a little power.
> + dev_info(dev, "Marvell Armada 3700 SPI Controller at 0x%08lx, irq %d\n",
> + (unsigned long)res->start, spi->irq);
This is just adding noise to the boot, remove it. It's not telling us
anything we read from the hardware or anything.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
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 v3 3/7] PWM: add pwm-stm32 DT bindings
From: Benjamin Gaignard @ 2016-12-05 12:12 UTC (permalink / raw)
To: Thierry Reding
Cc: Mark Rutland, devicetree, Lars-Peter Clausen, alexandre.torgue,
linux-pwm, linux-iio, Linus Walleij, Arnaud Pouliquen,
Linux Kernel Mailing List, robh+dt, linux-arm-kernel,
Peter Meerwald-Stadler, knaack.h, Gerald Baeza, Fabrice Gasnier,
Lee Jones, Linaro Kernel Mailman List, Jonathan Cameron,
Benjamin Gaignard
In-Reply-To: <20161205112347.GF19891@ulmo.ba.sec>
2016-12-05 12:23 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
>> 2016-12-05 7:53 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
>> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
>> >> Define bindings for pwm-stm32
>> >>
>> >> version 2:
>> >> - use parameters instead of compatible of handle the hardware configuration
>> >>
>> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> >> ---
>> >> .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++++++++++++++++++++++
>> >> 1 file changed, 38 insertions(+)
>> >> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >> new file mode 100644
>> >> index 0000000..575b9fb
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >> @@ -0,0 +1,38 @@
>> >> +STMicroelectronics PWM driver bindings for STM32
>> >
>> > Technically this bindings describe devices, so "driver binding" is a
>> > somewhat odd wording. Perhaps:
>> >
>> > STMicroelectronics STM32 General Purpose Timer PWM bindings
>> >
>> > ?
>> done
>>
>> >
>> >> +
>> >> +Must be a sub-node of STM32 general purpose timer driver
>> >> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
>> >
>> > Again, "driver parent node" is odd. Perhaps:
>> >
>> > Must be a sub-node of an STM32 General Purpose Timer device tree
>> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
>> > the parent node.
>> >
>> > ?
>>
>> done
>>
>> >
>> >> +Required parameters:
>> >> +- compatible: Must be "st,stm32-pwm"
>> >> +- pinctrl-names: Set to "default".
>> >> +- pinctrl-0: List of phandles pointing to pin configuration nodes
>> >> + for PWM module.
>> >> + For Pinctrl properties, please refer to [1].
>> >
>> > Your indentation and capitalization are inconsistent. Also, please refer
>> > to the pinctrl bindings by relative path and inline, rather than as a
>> > footnote reference.
>>
>> OK
>>
>> >
>> >> +
>> >> +Optional parameters:
>> >> +- st,breakinput: Set if the hardware have break input capabilities
>> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
>> >> + The value define the active polarity:
>> >> + - 0 (active LOW)
>> >> + - 1 (active HIGH)
>> >
>> > Could we fold these into a single property? If st,breakinput-polarity is
>> > not present it could simply mean that there is no break input, and if it
>> > is present you don't have to rely on a default.
>>
>> I need to know if I have to activate breakinput feature and on which level
>> I will rewrite it like that:
>> Optional parameters:
>> - st,breakinput-polarity-high: Set if break input polarity is active
>> on high level.
>> - st,breakinput-polarity-high: Set if break input polarity is active
>> on low level.
>
> How is that different from a single property:
>
> Optional properties:
> - st,breakinput-polarity: If present, a break input is available
> for the channel. In that case the property value denotes the
> polarity of the break input:
> - 0: active low
> - 1: active high
>
> ?
For break input feature I need two information: do I have to active it
and if activated
on which level.
I have two solutions:
- one parameter with a value (0 or 1) -> st,breakinput-polarity
- two parameters without value -> st,breakinput-active-high and
st,breakinput-active-low
By default break input feature is disabled
>
>> > The pwm- prefix is rather redundant since the node is already named pwm.
>> > Why not simply st,channels? Or simply channels, since it's not really
>> > anything specific to this hardware.
>> >
>> > Come to think of it, might be worth having a discussion with our DT
>> > gurus about what their stance is on using the # as prefix for numbers
>> > (such as in #address-cells or #size-cells). This could be #channels to
>> > mark it more explicitly as representing a count.
>> >
>> >> +- st,32bits-counter: Set if the hardware have a 32 bits counter
>> >> +- st,complementary: Set if the hardware have complementary output channels
>> >
>> > "hardware has" and also maybe mention explicitly that this is a boolean
>> > property. Otherwise people might be left wondering what it should be set
>> > to. Or maybe word this differently to imply that it's boolean:
>> >
>> > - st,32bits-counter: if present, the hardware has a 32 bit counter
>> > - st,complementary: if present, the hardware has a complementary
>> > output channel
>>
>> I found a way to detect, at probe time, the number of channels, counter size,
>> break input capability and complementary channels so I will remove
>> "st,breakinput", "st,32bits-counter", "st,complementary" and "st,pwm-num-chan"
>> parameters
>
> Oh hey, that's very neat. I suppose in that case my comment above about
> the break input polarity is somewhat obsoleted. Still I think you won't
> need two properties. Instead you can follow what other similar
> properties have done: choose a default (often low-active) and have a
> single optional property to override the default (often high-active).
>
> In your case:
>
> - st,breakinput-active-high: Some channels have a break input,
> whose polarity will be active low by default. If this
> property is present, the channel will be configured with an
> active high polarity for the break input.
>
> Thierry
--
Benjamin Gaignard
Graphic Study Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
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 v3 3/7] PWM: add pwm-stm32 DT bindings
From: Thierry Reding @ 2016-12-05 12:21 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Lee Jones, robh+dt, Mark Rutland, alexandre.torgue, devicetree,
Linux Kernel Mailing List, linux-pwm, Jonathan Cameron, knaack.h,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
linux-arm-kernel, Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen,
Linus Walleij, Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <CA+M3ks5j2e-EQEf74YSduboNnCiWC7+e_PD5aAz5XdFA1tGztQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4599 bytes --]
On Mon, Dec 05, 2016 at 01:12:25PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 12:23 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> > On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
> >> 2016-12-05 7:53 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> >> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> >> >> Define bindings for pwm-stm32
> >> >>
> >> >> version 2:
> >> >> - use parameters instead of compatible of handle the hardware configuration
> >> >>
> >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> >> >> ---
> >> >> .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++++++++++++++++++++++
> >> >> 1 file changed, 38 insertions(+)
> >> >> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >> new file mode 100644
> >> >> index 0000000..575b9fb
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >> @@ -0,0 +1,38 @@
> >> >> +STMicroelectronics PWM driver bindings for STM32
> >> >
> >> > Technically this bindings describe devices, so "driver binding" is a
> >> > somewhat odd wording. Perhaps:
> >> >
> >> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> >> >
> >> > ?
> >> done
> >>
> >> >
> >> >> +
> >> >> +Must be a sub-node of STM32 general purpose timer driver
> >> >> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
> >> >
> >> > Again, "driver parent node" is odd. Perhaps:
> >> >
> >> > Must be a sub-node of an STM32 General Purpose Timer device tree
> >> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
> >> > the parent node.
> >> >
> >> > ?
> >>
> >> done
> >>
> >> >
> >> >> +Required parameters:
> >> >> +- compatible: Must be "st,stm32-pwm"
> >> >> +- pinctrl-names: Set to "default".
> >> >> +- pinctrl-0: List of phandles pointing to pin configuration nodes
> >> >> + for PWM module.
> >> >> + For Pinctrl properties, please refer to [1].
> >> >
> >> > Your indentation and capitalization are inconsistent. Also, please refer
> >> > to the pinctrl bindings by relative path and inline, rather than as a
> >> > footnote reference.
> >>
> >> OK
> >>
> >> >
> >> >> +
> >> >> +Optional parameters:
> >> >> +- st,breakinput: Set if the hardware have break input capabilities
> >> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
> >> >> + The value define the active polarity:
> >> >> + - 0 (active LOW)
> >> >> + - 1 (active HIGH)
> >> >
> >> > Could we fold these into a single property? If st,breakinput-polarity is
> >> > not present it could simply mean that there is no break input, and if it
> >> > is present you don't have to rely on a default.
> >>
> >> I need to know if I have to activate breakinput feature and on which level
> >> I will rewrite it like that:
> >> Optional parameters:
> >> - st,breakinput-polarity-high: Set if break input polarity is active
> >> on high level.
> >> - st,breakinput-polarity-high: Set if break input polarity is active
> >> on low level.
> >
> > How is that different from a single property:
> >
> > Optional properties:
> > - st,breakinput-polarity: If present, a break input is available
> > for the channel. In that case the property value denotes the
> > polarity of the break input:
> > - 0: active low
> > - 1: active high
> >
> > ?
>
> For break input feature I need two information: do I have to active it
> and if activated
> on which level.
> I have two solutions:
> - one parameter with a value (0 or 1) -> st,breakinput-polarity
> - two parameters without value -> st,breakinput-active-high and
> st,breakinput-active-low
>
> By default break input feature is disabled
Right, what I was suggesting is that you use the first solution because
it's the typical way to use for tristate options. It's also much easier
to test for in practice because for the second solution you have to
parse two properties before you know whether it is active or not.
The second is typically the solution for required properties where only
one of the properties is used to override the default.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT
From: Hans Verkuil @ 2016-12-05 12:27 UTC (permalink / raw)
To: Laurent Pinchart, Sakari Ailus
Cc: Kevin Hilman, linux-media, devicetree, Sekhar Nori, Axel Haslam,
Bartosz Gołaszewski, Alexandre Bailon, David Lechner
In-Reply-To: <2453889.B9pO7dWgEo@avalon>
On 12/01/2016 10:16 AM, Laurent Pinchart wrote:
> Hello,
>
> On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
>> On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
>>> Sakari Ailus <sakari.ailus@iki.fi> writes:
>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>> Sakari Ailus <sakari.ailus@iki.fi> writes:
>>>>>> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>>>>>>> Allow getting of subdevs from DT ports and endpoints.
>>>>>>>
>>>>>>> The _get_pdata() function was larely inspired by (i.e. stolen from)
>>>>>>> am437x-vpfe.c
>>>>>>>
>>>>>>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>>>>>>> ---
>>>>>>>
>>>>>>> drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
>>>>>>> include/media/davinci/vpif_types.h
>>>>>>> | 9 +-
>>>>>>> 2 files changed, 133 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>> b/drivers/media/platform/davinci/vpif_capture.c index
>>>>>>> 94ee6cf03f02..47a4699157e7 100644
>>>>>>> --- a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>>>>>>> @@ -26,6 +26,8 @@
>>>>>>> #include <linux/slab.h>
>>>>>>>
>>>>>>> #include <media/v4l2-ioctl.h>
>>>>>>> +#include <media/v4l2-of.h>
>>>>>>> +#include <media/i2c/tvp514x.h>
>>>>>>
>>>>>> Do you need this header?
>>>>>
>>>>> Yes, based on discussion with Hans, since there is no DT binding for
>>>>> selecting the input pins of the TVP514x, I have to select it in the
>>>>> driver, so I need the defines from this header. More on this below...
>
> That's really ugly :-( The problem should be fixed properly instead of adding
> one more offender.
Do you have time for that, Laurent? I don't. Until that time we just need to
make do with this workaround.
>
>>>>>>> #include "vpif.h"
>>>>>>> #include "vpif_capture.h"
>>>>>>> @@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
>>>>>>>
>>>>>>> vpif_dbg(2, debug, "vpif_input_to_subdev\n");
>>>>>>>
>>>>>>> + if (!chan_cfg)
>>>>>>> + return -1;
>>>>>>> + if (input_index >= chan_cfg->input_count)
>>>>>>> + return -1;
>>>>>>> subdev_name = chan_cfg->inputs[input_index].subdev_name;
>>>>>>> if (subdev_name == NULL)
>>>>>>> return -1;
>>>>>>> @@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
>>>>>>> /* loop through the sub device list to get the sub device info
>>>>>>> */
>>>>>>> for (i = 0; i < vpif_cfg->subdev_count; i++) {
>>>>>>> subdev_info = &vpif_cfg->subdev_info[i];
>>>>>>> - if (!strcmp(subdev_info->name, subdev_name))
>>>>>>> + if (subdev_info && !strcmp(subdev_info->name,
>>>>>>> subdev_name))
>>>>>>> return i;
>>>>>>> }
>>>>>>> return -1;
>>>>>>> @@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct
>>>>>>> v4l2_async_notifier *notifier,> >> >>
>>>>>>> {
>>>>>>> int i;
>>>>>>>
>>>>>>> + for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
>>>>>>> + struct v4l2_async_subdev *_asd = vpif_obj.config
>>>>>>> ->asd[i];
>>>>>>> + const struct device_node *node = _asd->match.of.node;
>>>>>>> +
>>>>>>> + if (node == subdev->of_node) {
>>>>>>> + vpif_obj.sd[i] = subdev;
>>>>>>> + vpif_obj.config->chan_config
>>>>>>> ->inputs[i].subdev_name =
>>>>>>> + (char *)subdev->of_node->full_name;
>
> Can subdev_name be made const instead of blindly casting the full_name pointer
> ? If not this is probably unsafe, and if yes it should be done :-)
>
>>>>>>> + vpif_dbg(2, debug,
>>>>>>> + "%s: setting input %d subdev_name =
>>>>>>> %s\n",
>>>>>>> + __func__, i, subdev->of_node
>>>>>>> ->full_name);
>>>>>>> + return 0;
>>>>>>> + }
>>>>>>> + }
>>>>>>> +
>>>>>>> for (i = 0; i < vpif_obj.config->subdev_count; i++)
>>>>>>> if (!strcmp(vpif_obj.config->subdev_info[i].name,
>>>>>>> subdev->name)) {
>>>>>>> @@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
>>>>>>> v4l2_async_notifier *notifier)
>>>>>>> return vpif_probe_complete();
>>>>>>> }
>>>>>>>
>>>>>>> +static struct vpif_capture_config *
>>>>>>> +vpif_capture_get_pdata(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> + struct device_node *endpoint = NULL;
>>>>>>> + struct v4l2_of_endpoint bus_cfg;
>>>>>>> + struct vpif_capture_config *pdata;
>>>>>>> + struct vpif_subdev_info *sdinfo;
>>>>>>> + struct vpif_capture_chan_config *chan;
>>>>>>> + unsigned int i;
>>>>>>> +
>>>>>>> + dev_dbg(&pdev->dev, "vpif_get_pdata\n");
>>>>>>> +
>>>>>>> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>>>>>>> + return pdev->dev.platform_data;
>>>>>>> +
>>>>>>> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>>>>>> + if (!pdata)
>>>>>>> + return NULL;
>>>>>>> + pdata->subdev_info =
>>>>>>> + devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
>>>>>>> + VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
>>>>>>> +
>>>>>>> + if (!pdata->subdev_info)
>>>>>>> + return NULL;
>>>>>>> + dev_dbg(&pdev->dev, "%s\n", __func__);
>>>>>>> +
>>>>>>> + for (i = 0; ; i++) {
>>>>>>> + struct device_node *rem;
>>>>>>> + unsigned int flags;
>>>>>>> + int err;
>>>>>>> +
>>>>>>> + endpoint = of_graph_get_next_endpoint(pdev
>>>>>>> ->dev.of_node,
>>>>>>> + endpoint);
>>>>>>> + if (!endpoint)
>>>>>>> + break;
>>>>>>> +
>>>>>>> + sdinfo = &pdata->subdev_info[i];
>>>>>>
>>>>>> subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.
>>>>>
>>>>> Right, I need to make the loop only go for a max of
>>>>> VPIF_CAPTURE_MAX_CHANNELS iterations.
>>>>>
>>>>>>> + chan = &pdata->chan_config[i];
>>>>>>> + chan->inputs = devm_kzalloc(&pdev->dev,
>>>>>>> + sizeof(*chan->inputs) *
>>>>>>> + VPIF_DISPLAY_MAX_CHANNELS,
>>>>>>> + GFP_KERNEL);
>>>>>>> +
>>>>>>> + chan->input_count++;
>>>>>>> + chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;
>>>>>>
>>>>>> I wonder what's the purpose of using index i on this array as well.
>>>>>
>>>>> The number of endpoints in DT is the number of input channels
>>>>> configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)
>>>>>
>>>>>> If you use that to access a corresponding entry in a different array,
>>>>>> I'd just create a struct that contains the port configuration and the
>>>>>> async sub-device. The omap3isp driver does that, for instance; see
>>>>>> isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if
>>>>>> you're interested. Up to you.
>>>>>
>>>>> OK, I'll have a look at that driver. The goal here with this series is
>>>>> just to get this working with DT, but also not break the existing
>>>>> legacy platform_device support, so I'm trying not to mess with the
>>>>> driver-interal data structures too much.
>>>>
>>>> Ack.
>>>>
>>>>>>> + chan->inputs[i].input.std = V4L2_STD_ALL;
>>>>>>> + chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
>>>>>>> +
>>>>>>> + /* FIXME: need a new property? ch0:composite ch1:
>>>>>>> s-video */
>>>>>>> + if (i == 0)
>>>>>>
>>>>>> Can you assume that the first endopoint has got a particular kind of
>>>>>> input? What if it's not connected?
>>>>>
>>>>> On all the boards I know of (there aren't many using this SoC), it's a
>>>>> safe assumption.
>>>>>
>>>>>> If this is a different physical port (not in the meaning another) in
>>>>>> the device, I'd use the reg property for this. Please see
>>>>>> Documentation/devicetree/bindings/media/video-interfaces.txt .
>>>>>
>>>>> My understanding (which is admittedly somewhat fuzzy) of the TVP514x is
>>>>> that it's not physically a different port. Instead, it's just telling
>>>>> the TVP514x which pin(s) will be active inputs (and what kind of signal
>>>>> will be present.)
>>>>>
>>>>> I'm open to a better way to describe this input select from DT, but
>>>>> based on what I heard from Hans, there isn't currently a good way to do
>>>>> that except for in the driver:
>>>>> (c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)
>>>>>
>>>>> Based on further discussion in that thread, it sounds like there may be
>>>>> a way forward coming soon, and I'll be glad to switch to that when it
>>>>> arrives.
>
> I'm afraid I have to disappoint Hans here, I don't have code for that yet.
>
>>>> I'm not sure that properly supporting connectors will provide any help
>>>> here.
>>>>
>>>> Looking at the s_routing() API, it's the calling driver that has to be
>>>> aware of sub-device specific function parameters. As such it's not a
>>>> very good idea to require that a driver is aware of the value range of
>>>> another driver's parameter. I wonder if a simple enumeration interface
>>>> would help here --- if I understand correctly, the purpose is just to
>>>> provide a way to choose the input using VIDIOC_S_INPUT.
>>>>
>>>> I guess that's somehow ok as long as you have no other combinations of
>>>> these devices but this is hardly future-proof. (And certainly not a
>>>> problem created by this patch.)
>>>
>>> Yeah, this is far from future proof.
>>>
>>>> It'd be still nice to fix that as presumably we don't have the option of
>>>> reworking how we expect the device tree to look like.
>>>
>>> Agreed.
>>>
>>> I'm just hoping someone can shed som light on "how we expect the device
>>> tree to look". ;)
>>
>> :-)
>>
>> For the tvp514x, do you need more than a single endpoint on the receiver
>> side? Does the input that's selected affect the bus parameters?
>>
>> If it doesn't, you could create a custom endpoint property for the possible
>> input values. The s_routing() really should be fixed though, but that could
>> be postponed I guess. There are quite a few drivers using it.
>
> There's two ways to look at s_routing() in my opinion, as the calling driver
> should really not hardcode any knowledge specific to a particular subdev. We
> can either have the calling driver discover the possible routing options at
> runtime through the subdev API, or modify the s_routing() API.
>
Some historical perspective: s_routing was added well before the device tree
was ever used for ARM. And at that time the vast majority of drivers were PCI
or USB drivers, very few platform drivers existed (and those typically used
sensors, not video receivers).
Before s_routing existed the situation was even worse.
Basically what s_routing does is a poor-man's device tree entry, telling the
subdev how to route video or audio from connector to the output of the chip.
Typically the card tables in PCI or USB drivers contain the correct arguments
for s_routing. Of course, today we'd do that with the DT, but that was not an
option years ago.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v3 1/2] ARM: dts: da850-lcdk: add the dumb-vga-dac node
From: Tomi Valkeinen @ 2016-12-05 12:49 UTC (permalink / raw)
To: Bartosz Golaszewski, Kevin Hilman, Michael Turquette, Sekhar Nori,
Rob Herring, Frank Rowand, Mark Rutland, Peter Ujfalusi,
Russell King
Cc: linux-devicetree, LKML, linux-drm, Jyri Sarha, arm-soc,
Laurent Pinchart
In-Reply-To: <1480420624-23544-2-git-send-email-bgolaszewski@baylibre.com>
[-- Attachment #1.1.1: Type: text/plain, Size: 3213 bytes --]
On 29/11/16 13:57, Bartosz Golaszewski wrote:
> Add the dumb-vga-dac node to the board DT together with corresponding
> ports and vga connector. This allows to retrieve the edid info from
> the display automatically.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> arch/arm/boot/dts/da850-lcdk.dts | 58 ++++++++++++++++++++++++++++++++++++++++
> arch/arm/boot/dts/da850.dtsi | 17 ++++++++++++
> 2 files changed, 75 insertions(+)
>
> diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
> index 711b9ad..d864f11 100644
> --- a/arch/arm/boot/dts/da850-lcdk.dts
> +++ b/arch/arm/boot/dts/da850-lcdk.dts
> @@ -50,6 +50,53 @@
> system-clock-frequency = <24576000>;
> };
> };
> +
> + vga_bridge {
> + compatible = "dumb-vga-dac";
> + pinctrl-names = "default";
> + pinctrl-0 = <&lcd_pins>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> +
> + vga_bridge_in: endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&display_out_vga>;
> + };
> + };
> +
> + port@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> +
> + vga_bridge_out: endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&vga_con_in>;
> + };
> + };
> + };
> + };
> +
> + vga {
> + compatible = "vga-connector";
> +
> + ddc-i2c-bus = <&i2c0>;
> +
> + port {
> + vga_con_in: endpoint {
> + remote-endpoint = <&vga_bridge_out>;
> + };
> + };
> + };
> };
>
> &pmx_core {
> @@ -235,3 +282,14 @@
> &memctrl {
> status = "okay";
> };
> +
> +&display {
> + status = "okay";
> +};
> +
> +&display_out {
> + display_out_vga: endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&vga_bridge_in>;
> + };
> +};
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index 4070619..5f4ba2e 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -454,6 +454,23 @@
> reg = <0x213000 0x1000>;
> interrupts = <52>;
> status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + display_in: port@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> + };
> +
> + display_out: port@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> + };
> + };
> };
It's a bit difficult to follow this as there's been so many patches
going around. But I take the above is the LCDC node in the base da850
dtsi file? In that case, what is the display_in supposed to present?
It's the first node in the "display chain", so it has no input.
Also, don't touch da850.dtsi here, just add the "ports" node in the
da850-lcdk.dts file.
If the da850.dtsi has not been merged yet, I'd change the name of the
lcdc node to something else than "display". It's rather vague. If it's
named "lcdc", reading da850-lcdk.dts becomes much easier, as you'll
refer to "lcdc".
Tomi
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay
From: Javier Martinez Canillas @ 2016-12-05 13:13 UTC (permalink / raw)
To: Javi Merino, Sakari Ailus
Cc: linux-media, linux-kernel, devicetree, Mauro Carvalho Chehab
In-Reply-To: <1480932596-4108-1-git-send-email-javi.merino@kernel.org>
Hello Javi,
On 12/05/2016 07:09 AM, Javi Merino wrote:
> In asds configured with V4L2_ASYNC_MATCH_OF, the v4l2 subdev can be
> part of a devicetree overlay, for example:
>
> &media_bridge {
> ...
> my_port: port@0 {
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0>;
> ep: endpoint@0 {
> remote-endpoint = <&camera0>;
> };
> };
> };
>
> / {
> fragment@0 {
> target = <&i2c0>;
> __overlay__ {
> my_cam {
> compatible = "foo,bar";
> port {
> camera0: endpoint {
> remote-endpoint = <&my_port>;
> ...
> };
> };
> };
> };
> };
> };
>
> Each time the overlay is applied, its of_node pointer will be
> different. We are not interested in matching the pointer, what we
> want to match is that the path is the one we are expecting. Change to
> use of_node_cmp() so that we continue matching after the overlay has
> been removed and reapplied.
>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Javi Merino <javi.merino@kernel.org>
> ---
I already reviewed v1 but you didn't carry the tag. So again:
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
^ permalink raw reply
* RE: [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation
From: Punnaiah Choudary Kalluri @ 2016-12-05 13:24 UTC (permalink / raw)
To: Marek Vasut, dwmw2@infradead.org, computersforpeace@gmail.com,
boris.brezillon@free-electrons.com, richard@nod.at,
cyrille.pitchen@atmel.com, robh+dt@kernel.org,
mark.rutland@arm.com
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Michal Simek, kalluripunnaiahchoudary@gmail.com, kpc528@gmail.com,
linux-mtd@lists.infradead.org
In-Reply-To: <74842d92-840d-a7c2-fb1b-ddab1ac2cf42@gmail.com>
Hi Marek,
Thanks for the review.
> -----Original Message-----
> From: Marek Vasut [mailto:marek.vasut@gmail.com]
> Sent: Monday, December 05, 2016 9:56 AM
> To: Punnaiah Choudary Kalluri <punnaia@xilinx.com>;
> dwmw2@infradead.org; computersforpeace@gmail.com;
> boris.brezillon@free-electrons.com; richard@nod.at;
> cyrille.pitchen@atmel.com; robh+dt@kernel.org; mark.rutland@arm.com
> Cc: linux-kernel@vger.kernel.org; linux-mtd@lists.infradead.org;
> devicetree@vger.kernel.org; Michal Simek <michals@xilinx.com>;
> kalluripunnaiahchoudary@gmail.com; kpc528@gmail.com; Punnaiah
> Choudary Kalluri <punnaia@xilinx.com>
> Subject: Re: [PATCH v6 1/2] mtd: arasan: Add device tree binding
> documentation
>
> On 12/05/2016 05:11 AM, Punnaiah Choudary Kalluri wrote:
> > This patch adds the dts binding document for arasan nand flash
> > controller.
> >
> > Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> > Acked-by: Rob Herring <robh@kernel.org>
> > ---
> > changes in v6:
> > - Removed num-cs property
> > - Separated nandchip from nand controller
> > changes in v5:
> > - None
> > Changes in v4:
> > - Added num-cs property
> > - Added clock support
> > Changes in v3:
> > - None
> > Changes in v2:
> > - None
> > ---
> > .../devicetree/bindings/mtd/arasan_nfc.txt | 38
> ++++++++++++++++++++++
> > 1 file changed, 38 insertions(+)
> > create mode 100644
> Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> > new file mode 100644
> > index 0000000..dcbe7ad
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> > @@ -0,0 +1,38 @@
> > +Arasan Nand Flash Controller with ONFI 3.1 support
>
> Arasan NAND Flash ...
>
> > +Required properties:
> > +- compatible: Should be "arasan,nfc-v3p10"
>
> This v3p10 looks like version 3 patchlevel 10, but shouldn't we have
> some fallback option which doesn't encode IP version in the compat
> string ?
>
This is a third-party IP and v3p10 is the version that we are using in our SOC.
Also this IP doesn’t have the IP version information in the register space to
read dynamically and having generic compatible name. So, any new versions
can be added through of_match_table with config data inside the driver.
> Also, shouldn't quirks be handled by DT props instead of effectively
> encoding them into the compatible string ?
>
I feel the above mentioned method will be more appropriate rather than defining
the quirks through DT properties.
I will fix all other comments
Regards,
Punnaiah
> > +- reg: Memory map for module access
> > +- interrupt-parent: Interrupt controller the interrupt is routed through
> > +- interrupts: Should contain the interrupt for the device
> > +- clock-name: List of input clocks - "clk_sys", "clk_flash"
> > + (See clock bindings for details)
> > +- clocks: Clock phandles (see clock bindings for details)
> > +
> > +Optional properties:
> > +- arasan,has-mdma: Enables Dma support
>
> 'Enables DMA support' , with DMA in caps.
>
> > +for nand partition information please refer the below file
>
> For NAND ...
>
> > +Documentation/devicetree/bindings/mtd/partition.txt
> > +
> > +Example:
> > + nand0: nand@ff100000 {
> > + compatible = "arasan,nfc-v3p10"
> > + reg = <0x0 0xff100000 0x1000>;
> > + clock-name = "clk_sys", "clk_flash"
> > + clocks = <&misc_clk &misc_clk>;
> > + interrupt-parent = <&gic>;
> > + interrupts = <0 14 4>;
> > + arasan,has-mdma;
> > + #address-cells = <1>;
> > + #size-cells = <0>
> > +
> > + nand@0 {
> > + reg = <0>
> > + partition@0 {
> > + label = "filesystem";
> > + reg = <0x0 0x0 0x1000000>;
> > + };
> > + (...)
> > + };
> > + };
> >
>
>
> --
> Best regards,
> Marek Vasut
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply
* [PATCH v2 0/4] net: hix5hd2_gmac: add tx sg feature and reset/clock control signals
From: Dongpo Li @ 2016-12-05 13:27 UTC (permalink / raw)
To: robh+dt, mark.rutland, mturquette, sboyd, linux, zhangfei.gao,
yisen.zhuang, salil.mehta, davem, arnd, andrew
Cc: xuejiancheng, benjamin.chenhao, caizhiyong, netdev, devicetree,
linux-kernel, Dongpo Li
The "hix5hd2" is SoC name, add the generic ethernet driver compatible string.
The "hisi-gemac-v1" is the basic version and "hisi-gemac-v2" adds
the SG/TXCSUM/TSO/UFO features.
This patch set only adds the SG(scatter-gather) driver for transmitting,
the drivers of other features will be submitted later.
Add the MAC reset control signals and clock signals.
We make these signals optional to be backward compatible with
the hix5hd2 SoC.
Changes in v2:
- Make the compatible string changes be a separate patch and
the most specific string come first than the generic string
as advised by Rob.
- Make the MAC reset control signals and clock signals optional
to be backward compatible with the hix5hd2 SoC.
- Change the compatible string and give the clock a specific name
in hix5hd2 dts file.
Dongpo Li (4):
net: hix5hd2_gmac: add generic compatible string
net: hix5hd2_gmac: add tx scatter-gather feature
net: hix5hd2_gmac: add reset control and clock signals
ARM: dts: hix5hd2: add gmac generic compatible and clock names
.../bindings/net/hisilicon-hix5hd2-gmac.txt | 27 +-
arch/arm/boot/dts/hisi-x5hd2.dtsi | 6 +-
drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 352 +++++++++++++++++++--
3 files changed, 352 insertions(+), 33 deletions(-)
--
2.8.2
^ permalink raw reply
* [PATCH v2 1/4] net: hix5hd2_gmac: add generic compatible string
From: Dongpo Li @ 2016-12-05 13:27 UTC (permalink / raw)
To: robh+dt, mark.rutland, mturquette, sboyd, linux, zhangfei.gao,
yisen.zhuang, salil.mehta, davem, arnd, andrew
Cc: xuejiancheng, benjamin.chenhao, caizhiyong, netdev, devicetree,
linux-kernel, Dongpo Li
In-Reply-To: <1480944481-118803-1-git-send-email-lidongpo@hisilicon.com>
The "hix5hd2" is SoC name, add the generic ethernet driver name.
The "hisi-gemac-v1" is the basic version and "hisi-gemac-v2" adds
the SG/TXCSUM/TSO/UFO features.
Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
---
.../devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt | 9 +++++++--
drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 15 +++++++++++----
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt b/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
index 75d398b..75920f0 100644
--- a/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
+++ b/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
@@ -1,7 +1,12 @@
Hisilicon hix5hd2 gmac controller
Required properties:
-- compatible: should be "hisilicon,hix5hd2-gmac".
+- compatible: should contain one of the following SoC strings:
+ * "hisilicon,hix5hd2-gemac"
+ * "hisilicon,hi3798cv200-gemac"
+ and one of the following version string:
+ * "hisilicon,hisi-gemac-v1"
+ * "hisilicon,hisi-gemac-v2"
- reg: specifies base physical address(s) and size of the device registers.
The first region is the MAC register base and size.
The second region is external interface control register.
@@ -20,7 +25,7 @@ Required properties:
Example:
gmac0: ethernet@f9840000 {
- compatible = "hisilicon,hix5hd2-gmac";
+ compatible = "hisilicon,hix5hd2-gemac", "hisilicon,hisi-gemac-v1";
reg = <0xf9840000 0x1000>,<0xf984300c 0x4>;
interrupts = <0 71 4>;
#address-cells = <1>;
diff --git a/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c b/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
index e69a6be..27cb2e6 100644
--- a/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
+++ b/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
@@ -189,6 +189,10 @@
#define dma_cnt(n) ((n) >> 5)
#define dma_byte(n) ((n) << 5)
+#define HW_CAP_TSO BIT(0)
+#define GEMAC_V1 0
+#define GEMAC_V2 (GEMAC_V1 | HW_CAP_TSO)
+
struct hix5hd2_desc {
__le32 buff_addr;
__le32 cmd;
@@ -1021,7 +1025,10 @@ static int hix5hd2_dev_remove(struct platform_device *pdev)
}
static const struct of_device_id hix5hd2_of_match[] = {
- {.compatible = "hisilicon,hix5hd2-gmac",},
+ { .compatible = "hisilicon,hisi-gemac-v1", .data = (void *)GEMAC_V1 },
+ { .compatible = "hisilicon,hisi-gemac-v2", .data = (void *)GEMAC_V2 },
+ { .compatible = "hisilicon,hix5hd2-gemac", .data = (void *)GEMAC_V1 },
+ { .compatible = "hisilicon,hi3798cv200-gemac", .data = (void *)GEMAC_V2 },
{},
};
@@ -1029,7 +1036,7 @@ MODULE_DEVICE_TABLE(of, hix5hd2_of_match);
static struct platform_driver hix5hd2_dev_driver = {
.driver = {
- .name = "hix5hd2-gmac",
+ .name = "hisi-gemac",
.of_match_table = hix5hd2_of_match,
},
.probe = hix5hd2_dev_probe,
@@ -1038,6 +1045,6 @@ static struct platform_driver hix5hd2_dev_driver = {
module_platform_driver(hix5hd2_dev_driver);
-MODULE_DESCRIPTION("HISILICON HIX5HD2 Ethernet driver");
+MODULE_DESCRIPTION("HISILICON Gigabit Ethernet MAC driver");
MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("platform:hix5hd2-gmac");
+MODULE_ALIAS("platform:hisi-gemac");
--
2.8.2
^ permalink raw reply related
* [PATCH v2 2/4] net: hix5hd2_gmac: add tx scatter-gather feature
From: Dongpo Li @ 2016-12-05 13:27 UTC (permalink / raw)
To: robh+dt, mark.rutland, mturquette, sboyd, linux, zhangfei.gao,
yisen.zhuang, salil.mehta, davem, arnd, andrew
Cc: xuejiancheng, benjamin.chenhao, caizhiyong, netdev, devicetree,
linux-kernel, Dongpo Li
In-Reply-To: <1480944481-118803-1-git-send-email-lidongpo@hisilicon.com>
"hisi-gemac-v2" adds the SG/TXCSUM/TSO/UFO features.
This patch only adds the SG(scatter-gather) driver for transmitting,
the drivers of other features will be submitted later.
Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
---
drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 198 ++++++++++++++++++++++++--
1 file changed, 187 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c b/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
index 27cb2e6..18af55b 100644
--- a/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
+++ b/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
@@ -11,6 +11,7 @@
#include <linux/interrupt.h>
#include <linux/etherdevice.h>
#include <linux/platform_device.h>
+#include <linux/of_device.h>
#include <linux/of_net.h>
#include <linux/of_mdio.h>
#include <linux/clk.h>
@@ -183,6 +184,8 @@
#define DESC_DATA_LEN_OFF 16
#define DESC_BUFF_LEN_OFF 0
#define DESC_DATA_MASK 0x7ff
+#define DESC_SG BIT(30)
+#define DESC_FRAGS_NUM_OFF 11
/* DMA descriptor ring helpers */
#define dma_ring_incr(n, s) (((n) + 1) & ((s) - 1))
@@ -192,6 +195,7 @@
#define HW_CAP_TSO BIT(0)
#define GEMAC_V1 0
#define GEMAC_V2 (GEMAC_V1 | HW_CAP_TSO)
+#define HAS_CAP_TSO(hw_cap) ((hw_cap) & HW_CAP_TSO)
struct hix5hd2_desc {
__le32 buff_addr;
@@ -205,6 +209,27 @@ struct hix5hd2_desc_sw {
unsigned int size;
};
+struct hix5hd2_sg_desc_ring {
+ struct sg_desc *desc;
+ dma_addr_t phys_addr;
+};
+
+struct frags_info {
+ __le32 addr;
+ __le32 size;
+};
+
+/* hardware supported max skb frags num */
+#define SG_MAX_SKB_FRAGS 17
+struct sg_desc {
+ __le32 total_len;
+ __le32 resvd0;
+ __le32 linear_addr;
+ __le32 linear_len;
+ /* reserve one more frags for memory alignment */
+ struct frags_info frags[SG_MAX_SKB_FRAGS + 1];
+};
+
#define QUEUE_NUMS 4
struct hix5hd2_priv {
struct hix5hd2_desc_sw pool[QUEUE_NUMS];
@@ -212,6 +237,7 @@ struct hix5hd2_priv {
#define rx_bq pool[1]
#define tx_bq pool[2]
#define tx_rq pool[3]
+ struct hix5hd2_sg_desc_ring tx_ring;
void __iomem *base;
void __iomem *ctrl_base;
@@ -225,6 +251,7 @@ struct hix5hd2_priv {
struct device_node *phy_node;
phy_interface_t phy_mode;
+ unsigned long hw_cap;
unsigned int speed;
unsigned int duplex;
@@ -515,6 +542,27 @@ static int hix5hd2_rx(struct net_device *dev, int limit)
return num;
}
+static void hix5hd2_clean_sg_desc(struct hix5hd2_priv *priv,
+ struct sk_buff *skb, u32 pos)
+{
+ struct sg_desc *desc;
+ dma_addr_t addr;
+ u32 len;
+ int i;
+
+ desc = priv->tx_ring.desc + pos;
+
+ addr = le32_to_cpu(desc->linear_addr);
+ len = le32_to_cpu(desc->linear_len);
+ dma_unmap_single(priv->dev, addr, len, DMA_TO_DEVICE);
+
+ for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+ addr = le32_to_cpu(desc->frags[i].addr);
+ len = le32_to_cpu(desc->frags[i].size);
+ dma_unmap_page(priv->dev, addr, len, DMA_TO_DEVICE);
+ }
+}
+
static void hix5hd2_xmit_reclaim(struct net_device *dev)
{
struct sk_buff *skb;
@@ -542,8 +590,15 @@ static void hix5hd2_xmit_reclaim(struct net_device *dev)
pkts_compl++;
bytes_compl += skb->len;
desc = priv->tx_rq.desc + pos;
- addr = le32_to_cpu(desc->buff_addr);
- dma_unmap_single(priv->dev, addr, skb->len, DMA_TO_DEVICE);
+
+ if (skb_shinfo(skb)->nr_frags) {
+ hix5hd2_clean_sg_desc(priv, skb, pos);
+ } else {
+ addr = le32_to_cpu(desc->buff_addr);
+ dma_unmap_single(priv->dev, addr, skb->len,
+ DMA_TO_DEVICE);
+ }
+
priv->tx_skb[pos] = NULL;
dev_consume_skb_any(skb);
pos = dma_ring_incr(pos, TX_DESC_NUM);
@@ -604,12 +659,66 @@ static irqreturn_t hix5hd2_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static u32 hix5hd2_get_desc_cmd(struct sk_buff *skb, unsigned long hw_cap)
+{
+ u32 cmd = 0;
+
+ if (HAS_CAP_TSO(hw_cap)) {
+ if (skb_shinfo(skb)->nr_frags)
+ cmd |= DESC_SG;
+ cmd |= skb_shinfo(skb)->nr_frags << DESC_FRAGS_NUM_OFF;
+ } else {
+ cmd |= DESC_FL_FULL |
+ ((skb->len & DESC_DATA_MASK) << DESC_BUFF_LEN_OFF);
+ }
+
+ cmd |= (skb->len & DESC_DATA_MASK) << DESC_DATA_LEN_OFF;
+ cmd |= DESC_VLD_BUSY;
+
+ return cmd;
+}
+
+static int hix5hd2_fill_sg_desc(struct hix5hd2_priv *priv,
+ struct sk_buff *skb, u32 pos)
+{
+ struct sg_desc *desc;
+ dma_addr_t addr;
+ int ret;
+ int i;
+
+ desc = priv->tx_ring.desc + pos;
+
+ desc->total_len = cpu_to_le32(skb->len);
+ addr = dma_map_single(priv->dev, skb->data, skb_headlen(skb),
+ DMA_TO_DEVICE);
+ if (unlikely(dma_mapping_error(priv->dev, addr)))
+ return -EINVAL;
+ desc->linear_addr = cpu_to_le32(addr);
+ desc->linear_len = cpu_to_le32(skb_headlen(skb));
+
+ for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+ skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+ int len = frag->size;
+
+ addr = skb_frag_dma_map(priv->dev, frag, 0, len, DMA_TO_DEVICE);
+ ret = dma_mapping_error(priv->dev, addr);
+ if (unlikely(ret))
+ return -EINVAL;
+ desc->frags[i].addr = cpu_to_le32(addr);
+ desc->frags[i].size = cpu_to_le32(len);
+ }
+
+ return 0;
+}
+
static int hix5hd2_net_xmit(struct sk_buff *skb, struct net_device *dev)
{
struct hix5hd2_priv *priv = netdev_priv(dev);
struct hix5hd2_desc *desc;
dma_addr_t addr;
u32 pos;
+ u32 cmd;
+ int ret;
/* software write pointer */
pos = dma_cnt(readl_relaxed(priv->base + TX_BQ_WR_ADDR));
@@ -620,18 +729,31 @@ static int hix5hd2_net_xmit(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_BUSY;
}
- addr = dma_map_single(priv->dev, skb->data, skb->len, DMA_TO_DEVICE);
- if (dma_mapping_error(priv->dev, addr)) {
- dev_kfree_skb_any(skb);
- return NETDEV_TX_OK;
- }
-
desc = priv->tx_bq.desc + pos;
+
+ cmd = hix5hd2_get_desc_cmd(skb, priv->hw_cap);
+ desc->cmd = cpu_to_le32(cmd);
+
+ if (skb_shinfo(skb)->nr_frags) {
+ ret = hix5hd2_fill_sg_desc(priv, skb, pos);
+ if (unlikely(ret)) {
+ dev_kfree_skb_any(skb);
+ dev->stats.tx_dropped++;
+ return NETDEV_TX_OK;
+ }
+ addr = priv->tx_ring.phys_addr + pos * sizeof(struct sg_desc);
+ } else {
+ addr = dma_map_single(priv->dev, skb->data, skb->len,
+ DMA_TO_DEVICE);
+ if (unlikely(dma_mapping_error(priv->dev, addr))) {
+ dev_kfree_skb_any(skb);
+ dev->stats.tx_dropped++;
+ return NETDEV_TX_OK;
+ }
+ }
desc->buff_addr = cpu_to_le32(addr);
+
priv->tx_skb[pos] = skb;
- desc->cmd = cpu_to_le32(DESC_VLD_BUSY | DESC_FL_FULL |
- (skb->len & DESC_DATA_MASK) << DESC_DATA_LEN_OFF |
- (skb->len & DESC_DATA_MASK) << DESC_BUFF_LEN_OFF);
/* ensure desc updated */
wmb();
@@ -866,10 +988,40 @@ static int hix5hd2_init_hw_desc_queue(struct hix5hd2_priv *priv)
return -ENOMEM;
}
+static int hix5hd2_init_sg_desc_queue(struct hix5hd2_priv *priv)
+{
+ struct sg_desc *desc;
+ dma_addr_t phys_addr;
+
+ desc = (struct sg_desc *)dma_alloc_coherent(priv->dev,
+ TX_DESC_NUM * sizeof(struct sg_desc),
+ &phys_addr, GFP_KERNEL);
+ if (!desc)
+ return -ENOMEM;
+
+ priv->tx_ring.desc = desc;
+ priv->tx_ring.phys_addr = phys_addr;
+
+ return 0;
+}
+
+static void hix5hd2_destroy_sg_desc_queue(struct hix5hd2_priv *priv)
+{
+ if (priv->tx_ring.desc) {
+ dma_free_coherent(priv->dev,
+ TX_DESC_NUM * sizeof(struct sg_desc),
+ priv->tx_ring.desc, priv->tx_ring.phys_addr);
+ priv->tx_ring.desc = NULL;
+ }
+}
+
+static const struct of_device_id hix5hd2_of_match[];
+
static int hix5hd2_dev_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct device_node *node = dev->of_node;
+ const struct of_device_id *of_id = NULL;
struct net_device *ndev;
struct hix5hd2_priv *priv;
struct resource *res;
@@ -887,6 +1039,13 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
priv->dev = dev;
priv->netdev = ndev;
+ of_id = of_match_device(hix5hd2_of_match, dev);
+ if (!of_id) {
+ ret = -EINVAL;
+ goto out_free_netdev;
+ }
+ priv->hw_cap = (unsigned long)of_id->data;
+
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
priv->base = devm_ioremap_resource(dev, res);
if (IS_ERR(priv->base)) {
@@ -976,11 +1135,24 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
ndev->ethtool_ops = &hix5hd2_ethtools_ops;
SET_NETDEV_DEV(ndev, dev);
+ if (HAS_CAP_TSO(priv->hw_cap))
+ ndev->hw_features |= NETIF_F_SG;
+
+ ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
+ ndev->vlan_features |= ndev->features;
+
ret = hix5hd2_init_hw_desc_queue(priv);
if (ret)
goto out_phy_node;
netif_napi_add(ndev, &priv->napi, hix5hd2_poll, NAPI_POLL_WEIGHT);
+
+ if (HAS_CAP_TSO(priv->hw_cap)) {
+ ret = hix5hd2_init_sg_desc_queue(priv);
+ if (ret)
+ goto out_destroy_queue;
+ }
+
ret = register_netdev(priv->netdev);
if (ret) {
netdev_err(ndev, "register_netdev failed!");
@@ -992,6 +1164,8 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
return ret;
out_destroy_queue:
+ if (HAS_CAP_TSO(priv->hw_cap))
+ hix5hd2_destroy_sg_desc_queue(priv);
netif_napi_del(&priv->napi);
hix5hd2_destroy_hw_desc_queue(priv);
out_phy_node:
@@ -1016,6 +1190,8 @@ static int hix5hd2_dev_remove(struct platform_device *pdev)
mdiobus_unregister(priv->bus);
mdiobus_free(priv->bus);
+ if (HAS_CAP_TSO(priv->hw_cap))
+ hix5hd2_destroy_sg_desc_queue(priv);
hix5hd2_destroy_hw_desc_queue(priv);
of_node_put(priv->phy_node);
cancel_work_sync(&priv->tx_timeout_task);
--
2.8.2
^ permalink raw reply related
* [PATCH v2 3/4] net: hix5hd2_gmac: add reset control and clock signals
From: Dongpo Li @ 2016-12-05 13:28 UTC (permalink / raw)
To: robh+dt, mark.rutland, mturquette, sboyd, linux, zhangfei.gao,
yisen.zhuang, salil.mehta, davem, arnd, andrew
Cc: xuejiancheng, benjamin.chenhao, caizhiyong, netdev, devicetree,
linux-kernel, Dongpo Li
In-Reply-To: <1480944481-118803-1-git-send-email-lidongpo@hisilicon.com>
Add three reset control signals, "mac_core_rst", "mac_ifc_rst" and
"phy_rst".
The following diagram explained how the reset signals work.
SoC
|-----------------------------------------------------
| ------ |
| | cpu | |
| ------ |
| | |
| ------------ AMBA bus |
| GMAC | |
| ---------------------- |
| ------------- mac_core_rst | -------------- | |
| |clock and |-------------->| mac core | | |
| |reset | | -------------- | |
| |generator |---- | | | |
| ------------- | | ---------------- | |
| | ---------->| mac interface | | |
| | mac_ifc_rst | ---------------- | |
| | | | | |
| | | ------------------ | |
| |phy_rst | | RGMII interface | | |
| | | ------------------ | |
| | ---------------------- |
|----------|------------------------------------------|
| |
| ----------
|--------------------- |PHY chip |
----------
The "mac_core_rst" represents "mac core reset signal", it resets
the mac core including packet processing unit, descriptor processing unit,
tx engine, rx engine, control unit.
The "mac_ifc_rst" represents "mac interface reset signal", it resets
the mac interface. The mac interface unit connects mac core and
data interface like MII/RMII/RGMII. After we set a new value of
interface mode, we must reset mac interface to reload the new mode value.
The "mac_core_rst" and "mac_ifc_rst" are both optional to be
backward compatible with the hix5hd2 SoC.
The "phy_rst" represents "phy reset signal", it does a hardware reset
on the PHY chip. This reset signal is optional if the PHY can work well
without the hardware reset.
Add one more clock signal, the existing is MAC core clock,
and the new one is MAC interface clock.
The MAC interface clock is optional to be backward compatible with
the hix5hd2 SoC.
Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
---
.../bindings/net/hisilicon-hix5hd2-gmac.txt | 20 ++-
drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 139 +++++++++++++++++++--
2 files changed, 144 insertions(+), 15 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt b/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
index 75920f0..063c02d 100644
--- a/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
+++ b/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
@@ -17,6 +17,16 @@ Required properties:
- phy-handle: see ethernet.txt [1].
- mac-address: see ethernet.txt [1].
- clocks: clock phandle and specifier pair.
+- clock-names: contain the clock name "mac_core"(required) and "mac_ifc"(optional).
+- resets: should contain the phandle to the MAC core reset signal(optional),
+ the MAC interface reset signal(optional)
+ and the PHY reset signal(optional).
+- reset-names: contain the reset signal name "mac_core"(optional),
+ "mac_ifc"(optional) and "phy"(optional).
+- hisilicon,phy-reset-delays-us: triplet of delays if PHY reset signal given.
+ The 1st cell is reset pre-delay in micro seconds.
+ The 2nd cell is reset pulse in micro seconds.
+ The 3rd cell is reset post-delay in micro seconds.
- PHY subnode: inherits from phy binding [2]
@@ -25,15 +35,19 @@ Required properties:
Example:
gmac0: ethernet@f9840000 {
- compatible = "hisilicon,hix5hd2-gemac", "hisilicon,hisi-gemac-v1";
+ compatible = "hisilicon,hi3798cv200-gemac", "hisilicon,hisi-gemac-v2";
reg = <0xf9840000 0x1000>,<0xf984300c 0x4>;
interrupts = <0 71 4>;
#address-cells = <1>;
#size-cells = <0>;
- phy-mode = "mii";
+ phy-mode = "rgmii";
phy-handle = <&phy2>;
mac-address = [00 00 00 00 00 00];
- clocks = <&clock HIX5HD2_MAC0_CLK>;
+ clocks = <&crg HISTB_ETH0_MAC_CLK>, <&crg HISTB_ETH0_MACIF_CLK>;
+ clock-names = "mac_core", "mac_ifc";
+ resets = <&crg 0xcc 8>, <&crg 0xcc 10>, <&crg 0xcc 12>;
+ reset-names = "mac_core", "mac_ifc", "phy";
+ hisilicon,phy-reset-delays-us = <10000 10000 30000>;
phy2: ethernet-phy@2 {
reg = <2>;
diff --git a/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c b/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
index 18af55b..ee7e9ce 100644
--- a/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
+++ b/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
@@ -14,6 +14,7 @@
#include <linux/of_device.h>
#include <linux/of_net.h>
#include <linux/of_mdio.h>
+#include <linux/reset.h>
#include <linux/clk.h>
#include <linux/circ_buf.h>
@@ -197,6 +198,15 @@
#define GEMAC_V2 (GEMAC_V1 | HW_CAP_TSO)
#define HAS_CAP_TSO(hw_cap) ((hw_cap) & HW_CAP_TSO)
+#define PHY_RESET_DELAYS_PROPERTY "hisilicon,phy-reset-delays-us"
+
+enum phy_reset_delays {
+ PRE_DELAY,
+ PULSE,
+ POST_DELAY,
+ DELAYS_NUM,
+};
+
struct hix5hd2_desc {
__le32 buff_addr;
__le32 cmd;
@@ -255,12 +265,26 @@ struct hix5hd2_priv {
unsigned int speed;
unsigned int duplex;
- struct clk *clk;
+ struct clk *mac_core_clk;
+ struct clk *mac_ifc_clk;
+ struct reset_control *mac_core_rst;
+ struct reset_control *mac_ifc_rst;
+ struct reset_control *phy_rst;
+ u32 phy_reset_delays[DELAYS_NUM];
struct mii_bus *bus;
struct napi_struct napi;
struct work_struct tx_timeout_task;
};
+static inline void hix5hd2_mac_interface_reset(struct hix5hd2_priv *priv)
+{
+ if (!priv->mac_ifc_rst)
+ return;
+
+ reset_control_assert(priv->mac_ifc_rst);
+ reset_control_deassert(priv->mac_ifc_rst);
+}
+
static void hix5hd2_config_port(struct net_device *dev, u32 speed, u32 duplex)
{
struct hix5hd2_priv *priv = netdev_priv(dev);
@@ -293,6 +317,7 @@ static void hix5hd2_config_port(struct net_device *dev, u32 speed, u32 duplex)
if (duplex)
val |= GMAC_FULL_DUPLEX;
writel_relaxed(val, priv->ctrl_base);
+ hix5hd2_mac_interface_reset(priv);
writel_relaxed(BIT_MODE_CHANGE_EN, priv->base + MODE_CHANGE_EN);
if (speed == SPEED_1000)
@@ -807,16 +832,26 @@ static int hix5hd2_net_open(struct net_device *dev)
struct phy_device *phy;
int ret;
- ret = clk_prepare_enable(priv->clk);
+ ret = clk_prepare_enable(priv->mac_core_clk);
+ if (ret < 0) {
+ netdev_err(dev, "failed to enable mac core clk %d\n", ret);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(priv->mac_ifc_clk);
if (ret < 0) {
- netdev_err(dev, "failed to enable clk %d\n", ret);
+ clk_disable_unprepare(priv->mac_core_clk);
+ netdev_err(dev, "failed to enable mac ifc clk %d\n", ret);
return ret;
}
phy = of_phy_connect(dev, priv->phy_node,
&hix5hd2_adjust_link, 0, priv->phy_mode);
- if (!phy)
+ if (!phy) {
+ clk_disable_unprepare(priv->mac_ifc_clk);
+ clk_disable_unprepare(priv->mac_core_clk);
return -ENODEV;
+ }
phy_start(phy);
hix5hd2_hw_init(priv);
@@ -847,7 +882,8 @@ static int hix5hd2_net_close(struct net_device *dev)
phy_disconnect(dev->phydev);
}
- clk_disable_unprepare(priv->clk);
+ clk_disable_unprepare(priv->mac_ifc_clk);
+ clk_disable_unprepare(priv->mac_core_clk);
return 0;
}
@@ -1015,6 +1051,48 @@ static void hix5hd2_destroy_sg_desc_queue(struct hix5hd2_priv *priv)
}
}
+static inline void hix5hd2_mac_core_reset(struct hix5hd2_priv *priv)
+{
+ if (!priv->mac_core_rst)
+ return;
+
+ reset_control_assert(priv->mac_core_rst);
+ reset_control_deassert(priv->mac_core_rst);
+}
+
+static void hix5hd2_sleep_us(u32 time_us)
+{
+ u32 time_ms;
+
+ if (!time_us)
+ return;
+
+ time_ms = DIV_ROUND_UP(time_us, 1000);
+ if (time_ms < 20)
+ usleep_range(time_us, time_us + 500);
+ else
+ msleep(time_ms);
+}
+
+static void hix5hd2_phy_reset(struct hix5hd2_priv *priv)
+{
+ /* To make sure PHY hardware reset success,
+ * we must keep PHY in deassert state first and
+ * then complete the hardware reset operation
+ */
+ reset_control_deassert(priv->phy_rst);
+ hix5hd2_sleep_us(priv->phy_reset_delays[PRE_DELAY]);
+
+ reset_control_assert(priv->phy_rst);
+ /* delay some time to ensure reset ok,
+ * this depends on PHY hardware feature
+ */
+ hix5hd2_sleep_us(priv->phy_reset_delays[PULSE]);
+ reset_control_deassert(priv->phy_rst);
+ /* delay some time to ensure later MDIO access */
+ hix5hd2_sleep_us(priv->phy_reset_delays[POST_DELAY]);
+}
+
static const struct of_device_id hix5hd2_of_match[];
static int hix5hd2_dev_probe(struct platform_device *pdev)
@@ -1060,23 +1138,55 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
goto out_free_netdev;
}
- priv->clk = devm_clk_get(&pdev->dev, NULL);
- if (IS_ERR(priv->clk)) {
- netdev_err(ndev, "failed to get clk\n");
+ priv->mac_core_clk = devm_clk_get(&pdev->dev, "mac_core");
+ if (IS_ERR(priv->mac_core_clk)) {
+ netdev_err(ndev, "failed to get mac core clk\n");
ret = -ENODEV;
goto out_free_netdev;
}
- ret = clk_prepare_enable(priv->clk);
+ ret = clk_prepare_enable(priv->mac_core_clk);
if (ret < 0) {
- netdev_err(ndev, "failed to enable clk %d\n", ret);
+ netdev_err(ndev, "failed to enable mac core clk %d\n", ret);
goto out_free_netdev;
}
+ priv->mac_ifc_clk = devm_clk_get(&pdev->dev, "mac_ifc");
+ if (IS_ERR(priv->mac_ifc_clk))
+ priv->mac_ifc_clk = NULL;
+
+ ret = clk_prepare_enable(priv->mac_ifc_clk);
+ if (ret < 0) {
+ netdev_err(ndev, "failed to enable mac ifc clk %d\n", ret);
+ goto out_disable_mac_core_clk;
+ }
+
+ priv->mac_core_rst = devm_reset_control_get(dev, "mac_core");
+ if (IS_ERR(priv->mac_core_rst))
+ priv->mac_core_rst = NULL;
+ hix5hd2_mac_core_reset(priv);
+
+ priv->mac_ifc_rst = devm_reset_control_get(dev, "mac_ifc");
+ if (IS_ERR(priv->mac_ifc_rst))
+ priv->mac_ifc_rst = NULL;
+
+ priv->phy_rst = devm_reset_control_get(dev, "phy");
+ if (IS_ERR(priv->phy_rst)) {
+ priv->phy_rst = NULL;
+ } else {
+ ret = of_property_read_u32_array(node,
+ PHY_RESET_DELAYS_PROPERTY,
+ priv->phy_reset_delays,
+ DELAYS_NUM);
+ if (ret)
+ goto out_disable_clk;
+ hix5hd2_phy_reset(priv);
+ }
+
bus = mdiobus_alloc();
if (bus == NULL) {
ret = -ENOMEM;
- goto out_free_netdev;
+ goto out_disable_clk;
}
bus->priv = priv;
@@ -1159,7 +1269,8 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
goto out_destroy_queue;
}
- clk_disable_unprepare(priv->clk);
+ clk_disable_unprepare(priv->mac_ifc_clk);
+ clk_disable_unprepare(priv->mac_core_clk);
return ret;
@@ -1174,6 +1285,10 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
mdiobus_unregister(bus);
err_free_mdio:
mdiobus_free(bus);
+out_disable_clk:
+ clk_disable_unprepare(priv->mac_ifc_clk);
+out_disable_mac_core_clk:
+ clk_disable_unprepare(priv->mac_core_clk);
out_free_netdev:
free_netdev(ndev);
--
2.8.2
^ permalink raw reply related
* [PATCH v2 4/4] ARM: dts: hix5hd2: add gmac generic compatible and clock names
From: Dongpo Li @ 2016-12-05 13:28 UTC (permalink / raw)
To: robh+dt, mark.rutland, mturquette, sboyd, linux, zhangfei.gao,
yisen.zhuang, salil.mehta, davem, arnd, andrew
Cc: xuejiancheng, benjamin.chenhao, caizhiyong, netdev, devicetree,
linux-kernel, Dongpo Li
In-Reply-To: <1480944481-118803-1-git-send-email-lidongpo@hisilicon.com>
Add gmac generic compatible and clock names.
Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
---
arch/arm/boot/dts/hisi-x5hd2.dtsi | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/hisi-x5hd2.dtsi b/arch/arm/boot/dts/hisi-x5hd2.dtsi
index fdcc23d..0da76c5 100644
--- a/arch/arm/boot/dts/hisi-x5hd2.dtsi
+++ b/arch/arm/boot/dts/hisi-x5hd2.dtsi
@@ -436,18 +436,20 @@
};
gmac0: ethernet@1840000 {
- compatible = "hisilicon,hix5hd2-gmac";
+ compatible = "hisilicon,hix5hd2-gemac", "hisilicon,hisi-gemac-v1";
reg = <0x1840000 0x1000>,<0x184300c 0x4>;
interrupts = <0 71 4>;
clocks = <&clock HIX5HD2_MAC0_CLK>;
+ clock-names = "mac_core";
status = "disabled";
};
gmac1: ethernet@1841000 {
- compatible = "hisilicon,hix5hd2-gmac";
+ compatible = "hisilicon,hix5hd2-gemac", "hisilicon,hisi-gemac-v1";
reg = <0x1841000 0x1000>,<0x1843010 0x4>;
interrupts = <0 72 4>;
clocks = <&clock HIX5HD2_MAC1_CLK>;
+ clock-names = "mac_core";
status = "disabled";
};
--
2.8.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox