Devicetree
 help / color / mirror / Atom feed
* Re: [RFC PATCH 1/2] usb: host: add a generic platform USB roothub driver
From: Martin Blumenstingl @ 2017-01-13 20:21 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-usb-u79uwXL29TY76Z2rM5mHXA,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
	felipe.balbi-VuQAYsv1563Yd54FQh9/CA, mark.rutland-5wv7dgnIgG8,
	mathias.nyman-ral2JQCrhuEAvxtiuMwx3w,
	vegard.nossum-QHcLZuEGTsvQT0dZR+AlfA,
	stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
	treding-DDmLM1+adcrQT0dZR+AlfA, geert-Td1EMuHUCqxL1ZNQvxDV9g,
	manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
	tony-4v6yS6AI5VpBDgjK7y7TUQ, ahaslam-rdvid1DuHRBWk0Htik3J/w
In-Reply-To: <20170113200850.6z3i3b762pkjwlcc@rob-hp-laptop>

Hi Rob,

On Fri, Jan 13, 2017 at 9:08 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Jan 11, 2017 at 04:29:46PM +0100, Martin Blumenstingl wrote:
>> Many SoC platforms have separate devices for the USB PHY which are
>> registered through the generic PHY framework. These PHYs have to be
>> enabled to make the USB controller actually work. They also have to be
>> disabled again on shutdown/suspend.
>>
>> Currently (at least) the following HCI platform drivers are using custom
>> code to obtain all PHYs via devicetree for the roothub/controller and
>> disable/enable them when required:
>> - ehci-platform.c has ehci_platform_power_{on,off}
>> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
>> - ohci-platform.c has ohci_platform_power_{on,off}
>>
>> These drivers are not using the generic devicetree USB device bindings
>> yet which were only introduced recently (documentation is available in
>> devicetree/bindings/usb/usb-device.txt).
>> With this new driver the usb2-phy and usb3-phy can be specified directly
>> in the child-node of the corresponding port of the roothub via
>> devicetree. This can be extended by not just parsing PHYs (some of the
>> other drivers listed above are for example also parsing a list of clocks
>> as well) when required.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> ---
>>  .../devicetree/bindings/usb/usb-roothub.txt        |  41 ++++++
>>  drivers/usb/host/Kconfig                           |   3 +
>>  drivers/usb/host/Makefile                          |   2 +
>>  drivers/usb/host/platform-roothub.c                | 146 +++++++++++++++++++++
>>  drivers/usb/host/platform-roothub.h                |  14 ++
>>  5 files changed, 206 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/usb-roothub.txt
>>  create mode 100644 drivers/usb/host/platform-roothub.c
>>  create mode 100644 drivers/usb/host/platform-roothub.h
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-roothub.txt b/Documentation/devicetree/bindings/usb/usb-roothub.txt
>> new file mode 100644
>> index 000000000000..96e152d3901c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/usb-roothub.txt
>> @@ -0,0 +1,41 @@
>> +Generic USB root-hub Properties
>> +
>> +similar to the USB device bindings (documented in usb-device.txt from the
>> +current directory) this provides support for configuring the root-hub.
>> +
>> +Required properties:
>> +- compatible: should be at least one of "usb1d6b,3", "usb1d6b,2"
>> +- reg: must be 0.
>> +- address-cells: must be 1
>> +- size-cells: must be 0
>
>> +- a sub-node per port supports the following properties:
>
> Make this another section with required and optional sections.
I can do that, but let's wait for the results if we want the PHYs to
be specified at the grand-child or child level

>> +  - reg: the port number on the root-hub (mandatory)
>> +  - phys: optional, from the *Generic PHY* bindings (mandatory needed when
>> +    phy-names is given)
>> +  - phy-names: optional, from the *Generic PHY* bindings; supported names
>> +    are "usb2-phy" or "usb3-phy"
>> +
>> +Example:
>> +     &usb1 {
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +
>> +             roothub@0 {
>> +                     compatible = "usb1d6b,3", "usb1d6b,2";
>
> Is this discoverable? IIRC, we had decided that ports on the root hub
> are just children of the USB controller node (rather than
> grandchildren). Why does that not work?
if I understand you correctly you are thinking of something like this:
&usb1 {
    ...cells...

    port@1 {
        reg = <1>;
        phys = <&phy2>
    }

    port@2 {
        reg = <2>;
        phys = <&phy2>
    }
}

in that case we need a way to differentiate between "actual device at
port 1" and "configuration for root-hub port 1".
in that example I also cannot specify a compatible string since I
don't know which device might be plugged into that port.

>> +                     #address-cells = <1>;
>> +                     #size-cells = <0>;
>> +                     reg = <0>;
>> +
>> +                     port@1 {
>
> Wouldn't this normally be 0 and 1. This should probably be usb-port
> rather than port to avoid OF graph overlap.
the USB subsystem starts counting at 1, there can never be a "device
with device-number 0" - so I think we should stay with 1 and 2 for a
2-port roothub.
I'm fine with changing the name to "usb-port" though

>> +                             reg = <1>;
>> +                             usb-phy = <&usb2_phy1>, <&usb3_phy1>;
>
> s/usb-phy/phys/
thanks for spotting this

>> +                             phy-names = "usb2-phy", "usb3-phy";
>> +                     };
>> +
>> +                     port@2 {
>> +                             reg = <2>;
>> +                             usb-phy = <&usb2_phy2>, <&usb3_phy2>;
>> +                             phy-names = "usb2-phy", "usb3-phy";
>> +                     };
>> +             };
>> +     }
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 2/4] dt-bindings: Add TI SCI PM Domains
From: Dave Gerlach @ 2017-01-13 20:28 UTC (permalink / raw)
  To: Rob Herring
  Cc: Nishanth Menon, devicetree@vger.kernel.org, Ulf Hansson,
	Santosh Shilimkar, linux-pm@vger.kernel.org, Lokesh Vutla,
	Keerthy, Kevin Hilman, Rafael J . Wysocki,
	linux-kernel@vger.kernel.org, Tero Kristo, Russell King,
	Sudeep Holla, linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAL_JsqL+L1oHAOv7tLevzbtRFSKRs9J01SgF=HR_vNNqNV+4+Q@mail.gmail.com>

On 01/13/2017 01:25 PM, Rob Herring wrote:
> On Thu, Jan 12, 2017 at 9:27 AM, Dave Gerlach <d-gerlach@ti.com> wrote:
>> Rob,
>>
>> On 01/11/2017 03:34 PM, Rob Herring wrote:
>>>
>>> On Mon, Jan 9, 2017 at 11:57 AM, Dave Gerlach <d-gerlach@ti.com> wrote:
>>>>
>>>> Rob,
>>>>
>>>> On 01/09/2017 11:50 AM, Rob Herring wrote:
>>>>>
>>>>>
>>>>> On Wed, Jan 04, 2017 at 02:55:34PM -0600, Dave Gerlach wrote:
>>>>>>
>>>>>>
>>>>>> Add a generic power domain implementation, TI SCI PM Domains, that
>>>>>> will hook into the genpd framework and allow the TI SCI protocol to
>>>>>> control device power states.
>>>>>>
>>>>>> Also, provide macros representing each device index as understood
>>>>>> by TI SCI to be used in the device node power-domain references.
>>>>>> These are identifiers for the K2G devices managed by the PMMC.
>>>>>>
>>>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>>>>>> ---
>>>>>> v2->v3:
>>>>>>         Update k2g_pds node docs to show it should be a child of pmmc
>>>>>> node.
>>>>>>         In early versions a phandle was used to point to pmmc and docs
>>>>>> still
>>>>>>         incorrectly showed this.
>>>>>>
>>>>>>  .../devicetree/bindings/soc/ti/sci-pm-domain.txt   | 59 ++++++++++++++
>>>>>>  MAINTAINERS                                        |  2 +
>>>>>>  include/dt-bindings/genpd/k2g.h                    | 90
>>>>>> ++++++++++++++++++++++
>>>>>>  3 files changed, 151 insertions(+)
>>>>>>  create mode 100644
>>>>>> Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>  create mode 100644 include/dt-bindings/genpd/k2g.h
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>> b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..4c9064e512cb
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>> @@ -0,0 +1,59 @@
>>>>>> +Texas Instruments TI-SCI Generic Power Domain
>>>>>> +---------------------------------------------
>>>>>> +
>>>>>> +Some TI SoCs contain a system controller (like the PMMC, etc...) that
>>>>>> is
>>>>>> +responsible for controlling the state of the IPs that are present.
>>>>>> +Communication between the host processor running an OS and the system
>>>>>> +controller happens through a protocol known as TI-SCI [1]. This pm
>>>>>> domain
>>>>>> +implementation plugs into the generic pm domain framework and makes
>>>>>> use
>>>>>> of
>>>>>> +the TI SCI protocol power on and off each device when needed.
>>>>>> +
>>>>>> +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>>>>> +
>>>>>> +PM Domain Node
>>>>>> +==============
>>>>>> +The PM domain node represents the global PM domain managed by the
>>>>>> PMMC,
>>>>>> +which in this case is the single implementation as documented by the
>>>>>> generic
>>>>>> +PM domain bindings in
>>>>>> Documentation/devicetree/bindings/power/power_domain.txt.
>>>>>> +Because this relies on the TI SCI protocol to communicate with the
>>>>>> PMMC
>>>>>> it
>>>>>> +must be a child of the pmmc node.
>>>>>> +
>>>>>> +Required Properties:
>>>>>> +--------------------
>>>>>> +- compatible: should be "ti,sci-pm-domain"
>>>>>> +- #power-domain-cells: Must be 0.
>>>>>> +
>>>>>> +Example (K2G):
>>>>>> +-------------
>>>>>> +       pmmc: pmmc {
>>>>>> +               compatible = "ti,k2g-sci";
>>>>>> +               ...
>>>>>> +
>>>>>> +               k2g_pds: k2g_pds {
>>>>>> +                       compatible = "ti,sci-pm-domain";
>>>>>> +                       #power-domain-cells = <0>;
>>>>>> +               };
>>>>>> +       };
>>>>>> +
>>>>>> +PM Domain Consumers
>>>>>> +===================
>>>>>> +Hardware blocks that require SCI control over their state must provide
>>>>>> +a reference to the sci-pm-domain they are part of and a unique device
>>>>>> +specific ID that identifies the device.
>>>>>> +
>>>>>> +Required Properties:
>>>>>> +--------------------
>>>>>> +- power-domains: phandle pointing to the corresponding PM domain node.
>>>>>> +- ti,sci-id: index representing the device id to be passed oevr SCI to
>>>>>> +            be used for device control.
>>>>>
>>>>>
>>>>>
>>>>> As I've already stated before, this goes in power-domain cells. When you
>>>>> have a single thing (i.e. node) that controls multiple things, then you
>>>>> you need to specify the ID for each of them in phandle args. This is how
>>>>> irqs, gpio, clocks, *everything* in DT works.
>>>>
>>>>
>>>>
>>>> You think the reasoning for doing it this way provided by both Ulf and
>>>> myself on v2 [1] is not valid then?
>>>>
>>>> From Ulf:
>>>>
>>>> To me, the TI SCI ID, is similar to a "conid" for any another "device
>>>> resource" (like clock, pinctrl, regulator etc) which we can describe
>>>> in DT and assign to a device node. The only difference here, is that
>>>> we don't have common API to fetch the resource (like clk_get(),
>>>> regulator_get()), but instead we fetches the device's resource from
>>>> SoC specific code, via genpd's device ->attach() callback.
>>>
>>>
>>> Sorry, but that sounds like a kernel problem to me and has nothing to
>>> do with DT bindings.
>>>
>>>> From me:
>>>>
>>>> Yes, you've pretty much hit it on the head. It is not an index into a
>>>> list
>>>> of genpds but rather identifies the device *within* a single genpd. It is
>>>> a
>>>> property specific to each device that resides in a ti-sci-genpd, not a
>>>> mapping describing which genpd the device belongs to. The generic power
>>>> domain binding is concerned with mapping the device to a specific genpd,
>>>> which is does fine for us, but we have a sub mapping for devices that
>>>> exist
>>>> inside a genpd which, we must describe as well, hence the ti,sci-id.
>>>>
>>>>
>>>> So to summarize, the genpd framework does interpret the phandle arg as an
>>>> index into multiple genpds, just as you've said other frameworks do, but
>>>> this is not what I am trying to do, we have multiple devices within this
>>>> *single* genpd, hence the need for the ti,sci-id property.
>>>
>>>
>>> Fix the genpd framework rather than work around it in DT.
>>
>>
>> I still disagree that this has nothing to do with DT bindings, as the
>> current DT binding represents something different already. I am trying to
>> extend it to give me additional information needed for our platforms. Are
>> you saying that we should break what the current DT binding already
>> represents to mean something else?
>
> No idea because what's the current binding? From the patch, looks like
> a new binding to me.

Yes, ti,sci-id is a new binding. I am referring to the current meaning 
of the "power-domains" binding, which is where you are asking this 
property to be added, in "power-domains" cells. This is documented here 
[1] in the kernel, although looking at it I must admit it is not very clear.

The power-domains cell represents an offset into an array of power 
domains, if you choose to use it. That's what the genpd framework is 
hard coded to interpret it as. This is correct, as it is an index into a 
static list of power domains, used to identify which power domain a 
device belongs to, which is exactly what the genpd framework itself is 
concerned with. This is already how it is used in the kernel today.

My ti,sci-id is not an index into a list of power domains, so it should 
not go in the power-domains cells and go against what the power-domains 
binding says that the cell expects. We have one single power domain, and 
the new ti,sci-id binding is not something the genpd framework itself is 
concerned with as it's our property to identify a device inside a power 
domain, not to identify which power domain it is associated with.

Regards,
Dave

[1] Documentation/devicetree/bindings/power/power_domain.txt

>
> Rob
>

^ permalink raw reply

* Re: [PATCH 4/4] ARM: dts: sun8i: add OTG function to Lichee Pi Zero
From: Bin Liu @ 2017-01-13 21:00 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: devicetree@vger.kernel.org, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, Kishon Vijay Abraham I,
	Chen-Yu Tsai, Icenowy Zheng, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20170112173938.5am5njwrbs6p62br@lukather>

On Thu, Jan 12, 2017 at 06:39:38PM +0100, Maxime Ripard wrote:
> Hi Bin,
> 
> On Thu, Jan 12, 2017 at 08:50:14AM -0600, Bin Liu wrote:
> > On Wed, Jan 11, 2017 at 10:06:38PM +0100, Maxime Ripard wrote:
> > > On Wed, Jan 11, 2017 at 02:08:11PM -0600, Bin Liu wrote:
> > > > On Thu, Jan 12, 2017 at 03:55:33AM +0800, Icenowy Zheng wrote:
> > > > > 
> > > > > 
> > > > > 11.01.2017, 04:24, "Bin Liu" <b-liu@ti.com>:
> > > > > > On Tue, Jan 03, 2017 at 11:25:34PM +0800, Icenowy Zheng wrote:
> > > > > >>  Lichee Pi Zero features a USB OTG port.
> > > > > >>
> > > > > >>  Add support for it.
> > > > > >>
> > > > > >>  Note: in order to use the Host mode, the board must be powered via the
> > > > > >>  +5V and GND pins.
> > > > > >>
> > > > > >>  Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> > > > > >>  ---
> > > > > >>   arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts | 10 ++++++++++
> > > > > >>   1 file changed, 10 insertions(+)
> > > > > >>
> > > > > >>  diff --git a/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts b/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts
> > > > > >>  index 0099affc6ce3..3d9168cbaeca 100644
> > > > > >>  --- a/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts
> > > > > >>  +++ b/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts
> > > > > >>  @@ -71,3 +71,13 @@
> > > > > >>           pinctrl-names = "default";
> > > > > >>           status = "okay";
> > > > > >>   };
> > > > > >>  +
> > > > > >>  +&usb_otg {
> > > > > >>  + dr_mode = "otg";
> > > > > >
> > > > > > Why not set this default mode in dtsi instead?
> > > > > >
> > > > > > Regards,
> > > > > > -Bin.
> > > > > 
> > > > > There's possibly boards which do not have OTG functions.
> > > > 
> > > > That is board specific.
> > > 
> > > Exactly, and this is why it should be done in the board DT.
> > 
> > I am just suggesting based on the common practice. If a .dtsi exists for
> > a family, the .dtsi describes the device and common properties for all
> > possible boards, and each board .dts adds or overrides its specific
> > implementation. Kernel has many devices/boards done in this way - define
> > the default dr_mode in .dtsi.
> > 
> > In this case, I suggest to set the common dr_mode in .dtsi, then each
> > board .dts only overrides it if the implementation is different. 
> > 
> > > 
> > > The controller in the Allwinner SoCs do not handle directly the ID pin
> > > and VBUS, but rather rely on a GPIO to do so.
> > > 
> > > So boards with OTG will need setup anyway, at least to tell which
> > > GPIOs are used. There's no point in enforcing a default if it doesn't
> > > work by default.
> > 
> > Then define a default which supposes to work for most boards.
> > 
> > Why I suggest this, is because defining a default dr_mode which works
> > for most cases in dtsi could prevent a little surprise in MUSB function.
> > If someone designs a new board but forgets to define dr_mode in the new
> > board DT, the MUSB driver will default to org mode, which might not be
> > intended.
> 
> The point is that there is no sensible default. Some boards don't have
> an ID pin and no VBUS (peripheral), some don't have an ID pin but VBUS
> (host), and some have an ID pin but no controllable VBUS, some have an
> ID pin and a controllable VBUS, but we have no idea which GPIOs are
> used.
> 
> There's no way we can have something that works on most cases.

Ok, understood.

Regards,
-Bin.

^ permalink raw reply

* Re: [PATCH v3 02/24] ARM: dts: imx6qdl: Add compatible, clocks, irqs to MIPI CSI-2 node
From: Steve Longerbeam @ 2017-01-13 22:40 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: robh+dt, mark.rutland, shawnguo, kernel, fabio.estevam, linux,
	mchehab, hverkuil, nick, markus.heiser, laurent.pinchart+renesas,
	bparrot, geert, arnd, sudipm.mukherjee, minghsiu.tsai,
	tiffany.lin, jean-christophe.trotin, horms+renesas,
	niklas.soderlund+renesas, robert.jarzmik, songjun.wu,
	andrew-ct.chen, gregkh, devicetree, linux-kernel,
	linux-arm-kernel, linux-media, devel, Steve Longerbeam
In-Reply-To: <1484308678.31475.24.camel@pengutronix.de>



On 01/13/2017 03:57 AM, Philipp Zabel wrote:
> Am Freitag, den 06.01.2017, 18:11 -0800 schrieb Steve Longerbeam:
>> Add to the MIPI CSI2 receiver node: compatible string, interrupt sources,
>> clocks.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>   arch/arm/boot/dts/imx6qdl.dtsi | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
>> index 53e6e63..42926e9 100644
>> --- a/arch/arm/boot/dts/imx6qdl.dtsi
>> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
>> @@ -1125,7 +1125,14 @@
>>   			};
>>   
>>   			mipi_csi: mipi@021dc000 {
>> +				compatible = "fsl,imx6-mipi-csi2";
>>   				reg = <0x021dc000 0x4000>;
>> +				interrupts = <0 100 0x04>, <0 101 0x04>;
>> +				clocks = <&clks IMX6QDL_CLK_HSI_TX>,
>> +					 <&clks IMX6QDL_CLK_VIDEO_27M>,
>> +					 <&clks IMX6QDL_CLK_EIM_SEL>;
> I think the latter should be EIM_PODF

done.

>
>> +				clock-names = "dphy", "cfg", "pix";
> and I'm not sure dphy is the right name for this one. Is that the pll
> ref input?

I believe this naming came from FSL's mipi csi-2 driver. It is the "hsi_tx"
clock (presumably for the MIPI HSI controller) whose parents are selected
by the CDCDR register as PLL3_120M or PLL2_PFD2. I have no clue whether
this is indeed also used as the clock for the MIPI CSI-2 D-PHY, but 
according
to FSL naming convention it might be.


Steve

^ permalink raw reply

* Re: [PATCH v3 06/24] ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors
From: Steve Longerbeam @ 2017-01-13 23:04 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: mark.rutland, andrew-ct.chen, minghsiu.tsai, nick, songjun.wu,
	hverkuil, Steve Longerbeam, robert.jarzmik, devel, markus.heiser,
	laurent.pinchart+renesas, linux, geert, linux-media, devicetree,
	arnd, mchehab, bparrot, robh+dt, horms+renesas, tiffany.lin,
	linux-arm-kernel, niklas.soderlund+renesas, gregkh, linux-kernel,
	jean-christophe.trotin, kernel, fabio.estevam, shawnguo,
	sudipm.mukherjee
In-Reply-To: <1484309021.31475.29.camel@pengutronix.de>



On 01/13/2017 04:03 AM, Philipp Zabel wrote:
> Am Freitag, den 06.01.2017, 18:11 -0800 schrieb Steve Longerbeam:
>> Enables the OV5642 parallel-bus sensor, and the OV5640 MIPI CSI-2 sensor.
>> Both hang off the same i2c2 bus, so they require different (and non-
>> default) i2c slave addresses.
>>
>> The OV5642 connects to the parallel-bus mux input port on ipu1_csi0_mux.
>>
>> The OV5640 connects to the input port on the MIPI CSI-2 receiver on
>> mipi_csi. It is set to transmit over MIPI virtual channel 1.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>   arch/arm/boot/dts/imx6dl-sabrelite.dts   |   5 ++
>>   arch/arm/boot/dts/imx6q-sabrelite.dts    |   6 ++
>>   arch/arm/boot/dts/imx6qdl-sabrelite.dtsi | 118 +++++++++++++++++++++++++++++++
>>   3 files changed, 129 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx6dl-sabrelite.dts b/arch/arm/boot/dts/imx6dl-sabrelite.dts
>> index 0f06ca5..fec2524 100644
>> --- a/arch/arm/boot/dts/imx6dl-sabrelite.dts
>> +++ b/arch/arm/boot/dts/imx6dl-sabrelite.dts
>> @@ -48,3 +48,8 @@
>>   	model = "Freescale i.MX6 DualLite SABRE Lite Board";
>>   	compatible = "fsl,imx6dl-sabrelite", "fsl,imx6dl";
>>   };
>> +
>> +&ipu1_csi1_from_ipu1_csi1_mux {
>> +	data-lanes = <0 1>;
>> +	clock-lanes = <2>;
>> +};
>> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
>> index 66d10d8..9e2d26d 100644
>> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
>> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
>> @@ -52,3 +52,9 @@
>>   &sata {
>>   	status = "okay";
>>   };
>> +
>> +&ipu1_csi1_from_mipi_vc1 {
>> +	data-lanes = <0 1>;
>> +	clock-lanes = <2>;
>> +};
>> +
>> diff --git a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
>> index 795b5a5..bca9fed 100644
>> --- a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
>> +++ b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
>> @@ -39,6 +39,8 @@
>>    *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>    *     OTHER DEALINGS IN THE SOFTWARE.
>>    */
>> +
>> +#include <dt-bindings/clock/imx6qdl-clock.h>
>>   #include <dt-bindings/gpio/gpio.h>
>>   #include <dt-bindings/input/input.h>
>>   
>> @@ -96,6 +98,15 @@
>>   		};
>>   	};
>>   
>> +	mipi_xclk: mipi_xclk {
>> +		compatible = "pwm-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <22000000>;
>> +		clock-output-names = "mipi_pwm3";
>> +		pwms = <&pwm3 0 45>; /* 1 / 45 ns = 22 MHz */
>> +		status = "okay";
>> +	};
>> +
>>   	gpio-keys {
>>   		compatible = "gpio-keys";
>>   		pinctrl-names = "default";
>> @@ -220,6 +231,22 @@
>>   	};
>>   };
>>   
>> +&ipu1_csi0_from_ipu1_csi0_mux {
>> +	bus-width = <8>;
>> +	data-shift = <12>; /* Lines 19:12 used */
>> +	hsync-active = <1>;
>> +	vync-active = <1>;
>> +};
>> +
>> +&ipu1_csi0_mux_from_parallel_sensor {
>> +	remote-endpoint = <&ov5642_to_ipu1_csi0_mux>;
>> +};
>> +
>> +&ipu1_csi0 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_ipu1_csi0>;
>> +};
>> +
>>   &audmux {
>>   	pinctrl-names = "default";
>>   	pinctrl-0 = <&pinctrl_audmux>;
>> @@ -299,6 +326,52 @@
>>   	pinctrl-names = "default";
>>   	pinctrl-0 = <&pinctrl_i2c2>;
>>   	status = "okay";
>> +
>> +	ov5640: camera@40 {
>> +		compatible = "ovti,ov5640";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_ov5640>;
>> +		clocks = <&mipi_xclk>;
>> +		clock-names = "xclk";
>> +		reg = <0x40>;
>> +		xclk = <22000000>;
> This is superfluous, you can use clk_get_rate on mipi_xclk.

This property is actually there to tell the driver what to set the
rate to, with clk_set_rate(). So you are saying it would be better
to set the rate in the device tree and the driver should only
retrieve the rate?

Steve

^ permalink raw reply

* Re: [PATCH v3 15/24] media: Add userspace header file for i.MX
From: Steve Longerbeam @ 2017-01-13 23:13 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: robh+dt, mark.rutland, shawnguo, kernel, fabio.estevam, linux,
	mchehab, hverkuil, nick, markus.heiser, laurent.pinchart+renesas,
	bparrot, geert, arnd, sudipm.mukherjee, minghsiu.tsai,
	tiffany.lin, jean-christophe.trotin, horms+renesas,
	niklas.soderlund+renesas, robert.jarzmik, songjun.wu,
	andrew-ct.chen, gregkh, devicetree, linux-kernel,
	linux-arm-kernel, linux-media, devel, Steve Longerbeam
In-Reply-To: <1484309143.31475.31.camel@pengutronix.de>



On 01/13/2017 04:05 AM, Philipp Zabel wrote:
> Am Freitag, den 06.01.2017, 18:11 -0800 schrieb Steve Longerbeam:
>> This adds a header file for use by userspace programs wanting to interact
>> with the i.MX media driver. It defines custom v4l2 controls and events
>> generated by the i.MX v4l2 subdevices.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>   include/uapi/media/Kbuild |  1 +
>>   include/uapi/media/imx.h  | 30 ++++++++++++++++++++++++++++++
>>   2 files changed, 31 insertions(+)
>>   create mode 100644 include/uapi/media/imx.h
>>
>> diff --git a/include/uapi/media/Kbuild b/include/uapi/media/Kbuild
>> index aafaa5a..fa78958 100644
>> --- a/include/uapi/media/Kbuild
>> +++ b/include/uapi/media/Kbuild
>> @@ -1 +1,2 @@
>>   # UAPI Header export list
>> +header-y += imx.h
>> diff --git a/include/uapi/media/imx.h b/include/uapi/media/imx.h
>> new file mode 100644
>> index 0000000..2421d9c
>> --- /dev/null
>> +++ b/include/uapi/media/imx.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * Copyright (c) 2014-2015 Mentor Graphics Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the
>> + * License, or (at your option) any later version
>> + */
>> +
>> +#ifndef __UAPI_MEDIA_IMX_H__
>> +#define __UAPI_MEDIA_IMX_H__
>> +
>> +/*
>> + * events from the subdevs
>> + */
>> +#define V4L2_EVENT_IMX_CLASS          V4L2_EVENT_PRIVATE_START
>> +#define V4L2_EVENT_IMX_NFB4EOF        (V4L2_EVENT_IMX_CLASS + 1)
>> +#define V4L2_EVENT_IMX_EOF_TIMEOUT    (V4L2_EVENT_IMX_CLASS + 2)
>> +#define V4L2_EVENT_IMX_FRAME_INTERVAL (V4L2_EVENT_IMX_CLASS + 3)
> Aren't these generic enough to warrant common events? I would think
> there have to be other capture IP cores that can signal aborted frames
> or frame timeouts.

Yes, agreed. A frame capture timeout, or frame interval error, are
both generic concepts. At some point it would be great to make the
Frame Interval Monitor generally available under v4l2-core. As for the
EOF timeout event, I'll look into moving that into a generic V4L2 event.

Steve

^ permalink raw reply

* [PATCH] ARM: BCM5301X: Set 5 GHz wireless frequency limits on Netgear R8000
From: Rafał Miłecki @ 2017-01-13 23:58 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Arnd Bergmann, Rob Herring, Mark Rutland, Russell King,
	Hauke Mehrtens, Hante Meuleman,
	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Rafał Miłecki

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

Netgear R8000 is a tri-band home router. It has three BCM43602 chipsets
two of them for 5 GHz band. Both seem the same and their firmwares
report the same set of channels. The problem is due to hardware / board
design there are extra limitations that should be respected.

First PHY should be used for U-NII-2 and U-NII-3. Third PHY should be
used for U-NII-1. Using them in a different way may result in wireless
not working or in noticeably reduced performance. Basic version of this
info was provided by Broadcom employee, then it has been verified by me
using original vendor firmware (which has limitations hardcoded in UI).

This patch uses recently introduced ieee80211-freq-limit property to
describe these limitations at DT level.

Referencing PCIe devices in DT required specifying all related bridges.
Below you can see (a bit complex) PCI tree from R8000 that explains all
entries that I needed to put in DT.

0000:00:00.0		14e4:8012	Bridge Device
└─ 0000:01:00.0		14e4:aa52	Network Controller

0001:00:00.0		14e4:8012	Bridge Device
└─ 0001:01:00.0		10b5:8603	Bridge Device
   ├─ 0001:02:01.0	10b5:8603	Bridge Device
   │  └─ 0001:03:00.0	14e4:aa52	Network Controller
   ├─ 0001:02:02.0	10b5:8603	Bridge Device
   │  └─ 0001:04:00.0	14e4:aa52	Network Controller
   ├─ 0001:02:03.0	000d:0000	0x000000
   ├─ 0001:02:04.0	000d:0000	0x000000
   ├─ 0001:02:05.0	000d:0000	0x000000
   ├─ 0001:02:06.0	000d:0000	0x000000
   ├─ (...)
   ├─ 0001:02:1d.0	000d:0000	0x000000
   ├─ 0001:02:1e.0	000d:0000	0x000000
   └─ 0001:02:1f.0	000d:0000	0x000000

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
 arch/arm/boot/dts/bcm4709-netgear-r8000.dts | 48 +++++++++++++++++++++++++++++
 arch/arm/boot/dts/bcm5301x.dtsi             |  8 +++++
 2 files changed, 56 insertions(+)

diff --git a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts
index cd13534..8e39a84 100644
--- a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts
+++ b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts
@@ -108,6 +108,54 @@
 	};
 };
 
+&pcie0 {
+	#address-cells = <3>;
+	#size-cells = <2>;
+
+	bridge@0,0,0 {
+		reg = <0x0000 0 0 0 0>;
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+
+		wifi@0,1,0 {
+			reg = <0x0000 0 0 0 0>;
+			ieee80211-freq-limit = <5735000 5835000>;
+		};
+	};
+};
+
+&pcie1 {
+	#address-cells = <3>;
+	#size-cells = <2>;
+
+	bridge@1,0,0 {
+		reg = <0x0000 0 0 0 0>;
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+
+		bridge@1,1,0 {
+			reg = <0x0000 0 0 0 0>;
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			bridge@1,2,2 {
+				reg = <0x1000 0 0 0 0>;
+
+				#address-cells = <3>;
+				#size-cells = <2>;
+
+				wifi@1,4,0 {
+					reg = <0x0000 0 0 0 0>;
+					ieee80211-freq-limit = <5170000 5730000>;
+				};
+			};
+		};
+	};
+};
+
 &usb2 {
 	vcc-gpio = <&chipcommon 0 GPIO_ACTIVE_HIGH>;
 };
diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
index a4614c9..4fbb089 100644
--- a/arch/arm/boot/dts/bcm5301x.dtsi
+++ b/arch/arm/boot/dts/bcm5301x.dtsi
@@ -243,6 +243,14 @@
 			#gpio-cells = <2>;
 		};
 
+		pcie0: pcie@12000 {
+			reg = <0x00012000 0x1000>;
+		};
+
+		pcie1: pcie@13000 {
+			reg = <0x00013000 0x1000>;
+		};
+
 		usb2: usb2@21000 {
 			reg = <0x00021000 0x1000>;
 
-- 
2.10.1

--
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 related

* Re: [PATCH v3 2/4] dt-bindings: Add TI SCI PM Domains
From: Rob Herring @ 2017-01-14  2:40 UTC (permalink / raw)
  To: Dave Gerlach
  Cc: Ulf Hansson, Rafael J . Wysocki, Kevin Hilman,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Nishanth Menon, Keerthy, Russell King, Tero Kristo, Sudeep Holla,
	Santosh Shilimkar, Lokesh Vutla
In-Reply-To: <84d7d49b-933b-8b26-f18a-3a5054738cb1-l0cyMroinI0@public.gmane.org>

On Fri, Jan 13, 2017 at 2:28 PM, Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org> wrote:
> On 01/13/2017 01:25 PM, Rob Herring wrote:
>>
>> On Thu, Jan 12, 2017 at 9:27 AM, Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org> wrote:
>>>
>>> Rob,
>>>
>>> On 01/11/2017 03:34 PM, Rob Herring wrote:
>>>>
>>>>
>>>> On Mon, Jan 9, 2017 at 11:57 AM, Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org> wrote:
>>>>>
>>>>>
>>>>> Rob,
>>>>>
>>>>> On 01/09/2017 11:50 AM, Rob Herring wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Jan 04, 2017 at 02:55:34PM -0600, Dave Gerlach wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Add a generic power domain implementation, TI SCI PM Domains, that
>>>>>>> will hook into the genpd framework and allow the TI SCI protocol to
>>>>>>> control device power states.
>>>>>>>
>>>>>>> Also, provide macros representing each device index as understood
>>>>>>> by TI SCI to be used in the device node power-domain references.
>>>>>>> These are identifiers for the K2G devices managed by the PMMC.
>>>>>>>
>>>>>>> Signed-off-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
>>>>>>> Signed-off-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
>>>>>>> ---
>>>>>>> v2->v3:
>>>>>>>         Update k2g_pds node docs to show it should be a child of pmmc
>>>>>>> node.
>>>>>>>         In early versions a phandle was used to point to pmmc and
>>>>>>> docs
>>>>>>> still
>>>>>>>         incorrectly showed this.
>>>>>>>
>>>>>>>  .../devicetree/bindings/soc/ti/sci-pm-domain.txt   | 59
>>>>>>> ++++++++++++++
>>>>>>>  MAINTAINERS                                        |  2 +
>>>>>>>  include/dt-bindings/genpd/k2g.h                    | 90
>>>>>>> ++++++++++++++++++++++
>>>>>>>  3 files changed, 151 insertions(+)
>>>>>>>  create mode 100644
>>>>>>> Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>>  create mode 100644 include/dt-bindings/genpd/k2g.h
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>> b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..4c9064e512cb
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>> @@ -0,0 +1,59 @@
>>>>>>> +Texas Instruments TI-SCI Generic Power Domain
>>>>>>> +---------------------------------------------
>>>>>>> +
>>>>>>> +Some TI SoCs contain a system controller (like the PMMC, etc...)
>>>>>>> that
>>>>>>> is
>>>>>>> +responsible for controlling the state of the IPs that are present.
>>>>>>> +Communication between the host processor running an OS and the
>>>>>>> system
>>>>>>> +controller happens through a protocol known as TI-SCI [1]. This pm
>>>>>>> domain
>>>>>>> +implementation plugs into the generic pm domain framework and makes
>>>>>>> use
>>>>>>> of
>>>>>>> +the TI SCI protocol power on and off each device when needed.
>>>>>>> +
>>>>>>> +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>>>>>> +
>>>>>>> +PM Domain Node
>>>>>>> +==============
>>>>>>> +The PM domain node represents the global PM domain managed by the
>>>>>>> PMMC,
>>>>>>> +which in this case is the single implementation as documented by the
>>>>>>> generic
>>>>>>> +PM domain bindings in
>>>>>>> Documentation/devicetree/bindings/power/power_domain.txt.
>>>>>>> +Because this relies on the TI SCI protocol to communicate with the
>>>>>>> PMMC
>>>>>>> it
>>>>>>> +must be a child of the pmmc node.
>>>>>>> +
>>>>>>> +Required Properties:
>>>>>>> +--------------------
>>>>>>> +- compatible: should be "ti,sci-pm-domain"
>>>>>>> +- #power-domain-cells: Must be 0.
>>>>>>> +
>>>>>>> +Example (K2G):
>>>>>>> +-------------
>>>>>>> +       pmmc: pmmc {
>>>>>>> +               compatible = "ti,k2g-sci";
>>>>>>> +               ...
>>>>>>> +
>>>>>>> +               k2g_pds: k2g_pds {
>>>>>>> +                       compatible = "ti,sci-pm-domain";
>>>>>>> +                       #power-domain-cells = <0>;
>>>>>>> +               };
>>>>>>> +       };
>>>>>>> +
>>>>>>> +PM Domain Consumers
>>>>>>> +===================
>>>>>>> +Hardware blocks that require SCI control over their state must
>>>>>>> provide
>>>>>>> +a reference to the sci-pm-domain they are part of and a unique
>>>>>>> device
>>>>>>> +specific ID that identifies the device.
>>>>>>> +
>>>>>>> +Required Properties:
>>>>>>> +--------------------
>>>>>>> +- power-domains: phandle pointing to the corresponding PM domain
>>>>>>> node.
>>>>>>> +- ti,sci-id: index representing the device id to be passed oevr SCI
>>>>>>> to
>>>>>>> +            be used for device control.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> As I've already stated before, this goes in power-domain cells. When
>>>>>> you
>>>>>> have a single thing (i.e. node) that controls multiple things, then
>>>>>> you
>>>>>> you need to specify the ID for each of them in phandle args. This is
>>>>>> how
>>>>>> irqs, gpio, clocks, *everything* in DT works.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> You think the reasoning for doing it this way provided by both Ulf and
>>>>> myself on v2 [1] is not valid then?
>>>>>
>>>>> From Ulf:
>>>>>
>>>>> To me, the TI SCI ID, is similar to a "conid" for any another "device
>>>>> resource" (like clock, pinctrl, regulator etc) which we can describe
>>>>> in DT and assign to a device node. The only difference here, is that
>>>>> we don't have common API to fetch the resource (like clk_get(),
>>>>> regulator_get()), but instead we fetches the device's resource from
>>>>> SoC specific code, via genpd's device ->attach() callback.
>>>>
>>>>
>>>>
>>>> Sorry, but that sounds like a kernel problem to me and has nothing to
>>>> do with DT bindings.
>>>>
>>>>> From me:
>>>>>
>>>>> Yes, you've pretty much hit it on the head. It is not an index into a
>>>>> list
>>>>> of genpds but rather identifies the device *within* a single genpd. It
>>>>> is
>>>>> a
>>>>> property specific to each device that resides in a ti-sci-genpd, not a
>>>>> mapping describing which genpd the device belongs to. The generic power
>>>>> domain binding is concerned with mapping the device to a specific
>>>>> genpd,
>>>>> which is does fine for us, but we have a sub mapping for devices that
>>>>> exist
>>>>> inside a genpd which, we must describe as well, hence the ti,sci-id.
>>>>>
>>>>>
>>>>> So to summarize, the genpd framework does interpret the phandle arg as
>>>>> an
>>>>> index into multiple genpds, just as you've said other frameworks do,
>>>>> but
>>>>> this is not what I am trying to do, we have multiple devices within
>>>>> this
>>>>> *single* genpd, hence the need for the ti,sci-id property.
>>>>
>>>>
>>>>
>>>> Fix the genpd framework rather than work around it in DT.
>>>
>>>
>>>
>>> I still disagree that this has nothing to do with DT bindings, as the
>>> current DT binding represents something different already. I am trying to
>>> extend it to give me additional information needed for our platforms. Are
>>> you saying that we should break what the current DT binding already
>>> represents to mean something else?
>>
>>
>> No idea because what's the current binding? From the patch, looks like
>> a new binding to me.
>
>
> Yes, ti,sci-id is a new binding. I am referring to the current meaning of
> the "power-domains" binding, which is where you are asking this property to
> be added, in "power-domains" cells. This is documented here [1] in the
> kernel, although looking at it I must admit it is not very clear.
>
> The power-domains cell represents an offset into an array of power domains,
> if you choose to use it. That's what the genpd framework is hard coded to
> interpret it as. This is correct, as it is an index into a static list of
> power domains, used to identify which power domain a device belongs to,
> which is exactly what the genpd framework itself is concerned with. This is
> already how it is used in the kernel today.

Strictly speaking, the cells are purely for the interpretation of the
phandle they are associated with. If some controller wants to have 20
cells, then it could assuming a good reason. The reality is we tend to
align the meaning of the cells. If genpd is interpreting the cells and
not letting the driver for the power domain controller interpret them,
then still, genpd needs to be fixed.

IIRC, initially it was said genpd required 0 cells, hence my confusion.

> My ti,sci-id is not an index into a list of power domains, so it should not
> go in the power-domains cells and go against what the power-domains binding
> says that the cell expects. We have one single power domain, and the new
> ti,sci-id binding is not something the genpd framework itself is concerned
> with as it's our property to identify a device inside a power domain, not to
> identify which power domain it is associated with.

What is the id used for? I can understand why you need to know what
power domain a device is in (as power-domains identifies), but not
what devices are in a power domain.

Rob
--
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 1/2] of: base: add support to find the level of the last cache
From: Rob Herring @ 2017-01-14  2:45 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Catalin Marinas, Will Deacon,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tan Xiaojun,
	Mark Rutland
In-Reply-To: <1484245772-31511-1-git-send-email-sudeep.holla-5wv7dgnIgG8@public.gmane.org>

On Thu, Jan 12, 2017 at 12:29 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
> It is useful to have helper function just to get the number of cache
> levels for a given logical cpu. We can obtain the same by just checking
> the level at which the last cache is present. This patch adds support
> to find the level of the last cache for a given cpu.
>
> It will be used on ARM64 platform where the device tree provides the
> information for the additional non-architected/transparent/external
> last level caches that are not integrated with the processors.
>
> Suggested-by: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/of/base.c  | 27 +++++++++++++++++++++++++++
>  include/linux/of.h |  1 +
>  2 files changed, 28 insertions(+)
>
> v1->v2:
>         - Moved to using "cache-level" in the last level cache instead
>           of counting through all the nodes as suggested by Rob
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d4bea3c797d6..c1128a077aea 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -25,6 +25,7 @@
>  #include <linux/cpu.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
> @@ -2268,6 +2269,32 @@ struct device_node *of_find_next_cache_node(const struct device_node *np)
>  }
>
>  /**
> + * of_find_last_cache_level - Find the level at which the last cache is
> + *             present for the given logical cpu
> + *
> + * @cpu: cpu number(logical index) for which the last cache level is needed
> + *
> + * Returns the the level at which the last cache is present. It is exactly
> + * same as  the total number of cache levels for the given logical cpu.
> + */
> +int of_find_last_cache_level(unsigned int cpu)
> +{
> +       int cache_level = 0;
> +       struct device_node *prev = NULL, *np = of_cpu_device_node_get(cpu);
> +
> +       while (np) {
> +               prev = np;
> +               of_node_put(np);
> +               np = of_find_next_cache_node(np);
> +       }
> +
> +       if (prev)

Probably don't need this check. Otherwise,

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

> +               of_property_read_u32(prev, "cache-level", &cache_level);
> +
> +       return cache_level;
> +}
> +
> +/**
>   * of_graph_parse_endpoint() - parse common endpoint node properties
>   * @node: pointer to endpoint device_node
>   * @endpoint: pointer to the OF endpoint data structure
> diff --git a/include/linux/of.h b/include/linux/of.h
> index d72f01009297..21e6323de0f3 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -280,6 +280,7 @@ extern struct device_node *of_get_child_by_name(const struct device_node *node,
>
>  /* cache lookup */
>  extern struct device_node *of_find_next_cache_node(const struct device_node *);
> +extern int of_find_last_cache_level(unsigned int cpu);
>  extern struct device_node *of_find_node_with_property(
>         struct device_node *from, const char *prop_name);
>
> --
> 2.7.4
>
--
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 1/2] dt-bindings: document renesas-ostm timer
From: Chris Brandt @ 2017-01-14  3:30 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Mark Rutland, Simon Horman, Magnus Damm,
	Russell King, Daniel Lezcano, Thomas Gleixner, Geert Uytterhoeven,
	devicetree@vger.kernel.org, Linux-Renesas
In-Reply-To: <CAMuHMdX_hx2M8Q4y7gptJvad4cbb+M54DZVCmbhunKuk4esbGA@mail.gmail.com>

Hi Geert,

Thank you for your review.


On Friday, January 13, 2017, Geert Uytterhoeven wrote:
> > +The OSTM comes with 2 independent channels.
> > +We will use the first channel (OSTM0) as a free running clocksource
> > +and the second channel (OSTM1) as a interrupt driven clock event.
> > +
> > +Additionally we will use the clocksource channel (OTSM0) for the
> > +system schedule timer sched_clock().
> 
> The above two sentences are software policy, not hardware description.
> Hence they do not belong in the DT bindings document.
> You can move them to the commit description, though.

OK.


> > +Required Properties:
> > +
> > +  - compatible: must be one or more of the following:
> > +    - "renesas,ostm-r7s72100" for the r7s72100 OSTM
> 
> Please use "renesas,r7s72100-ostm" instead, to match current practices.

If I look at the current r7s72100.dtsi:

compatible = "renesas,r7s72100-cpg-clocks", "renesas,rz-cpg-clocks";
compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
compatible = "renesas,scif-r7s72100", "renesas,scif";
compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz";
compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
compatible = "renesas,mtu2-r7s72100", "renesas,mtu2";
compatible = "renesas,mmcif-r7s72100", "renesas,sh-mmcif";
compatible = "renesas,sdhi-r7s72100";

Is "renesas,xxx-r7s7210" the old way, and "renesas,r7s72100-xxx" is the new way??



> > +  - reg: base address and length of the registers block for each timer
> channel.
> > +    There should be 2 sets of addresses, one for each channel.
> > +
> > +  - interrupts: interrupt specifiers for the timers. There should be 2
> > +    interupts, one for each channel.
> > +
> > +  - clocks: a list of phandle + clock-specifier pairs, one for each
> entry
> > +    channel. There should be 2 sets, one for each channel.
> 
> Are the channels truly independent? If yes, I think it's better to have
> two separate device nodes, one for each channel.
> Each channel has its own module clock, so using separate devices means
> Runtime PM can manage both channels through their module clocks as soon as
> you add a "power-domains" property pointing to the clock domain controller.


Yes, technically they are independent channels.
The way the driver is currently written, 1 instance of the driver uses 2 channels
for different things. Ch0 will be set up as a 'clocksource', and ch1 will be set up
as a 'clock event'.

As in:

static int __init ostm_timer_init(struct ostm_device *ostm)
{
	int ret = 0;

	/* ostm0 will be clock source */
	ret = ostm_init_clksrc(ostm);
	if (ret)
		goto err;

	/* use ostm0 as system scheduling clock */
	ret = ostm_init_sched_clock(&ostm->clksrc);
	if (ret)
		goto err;

	/* ostm1 will be clock event */
	ret = ostm_init_clkevt(ostm);
err:
	return ret;
}



Do you think it would be better if a driver instance only does 1 thing: Either
'clocksource' or 'clock event'??
Then, I would make 2 ostm nodes and pass in the mode I would like it operate in?

For example:

&ostm0 {
	mode = "clocksource";
	status = "okay";
};

&ostm1 {
	mode = "clock-event";
	status = "okay";
};



Thank you,
Chris

^ permalink raw reply

* [PATCH v6 0/3] dmaengine: xilinx_dma: Bug fixes
From: Kedareswara rao Appana @ 2017-01-14  5:35 UTC (permalink / raw)
  To: robh+dt, mark.rutland, dan.j.williams, vinod.koul, michal.simek,
	soren.brinkmann, appanad, moritz.fischer, laurent.pinchart, luis,
	Jose.Abreu
  Cc: dmaengine, linux-arm-kernel, linux-kernel, devicetree

This patch series fixes below bugs in DMA and VDMA IP's
---> Do not start VDMA until frame buffer is processed by the h/w
---> Fix bug in Multi frame sotres handling in VDMA
---> Fix issues w.r.to multi frame descriptors submit with AXI DMA S2MM(recv) Side.

Kedareswara rao Appana (3):
  dmaengine: xilinx_dma: Check for channel idle state before submitting
    dma descriptor
  dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in
    vdma
  dmaengine: xilinx_dma: Fix race condition in the driver for multiple
    descriptor scenario

 .../devicetree/bindings/dma/xilinx/xilinx_dma.txt  |   2 +
 drivers/dma/xilinx/xilinx_dma.c                    | 270 ++++++++++++---------
 2 files changed, 161 insertions(+), 111 deletions(-)

-- 
2.1.2

^ permalink raw reply

* [PATCH v6 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor
From: Kedareswara rao Appana @ 2017-01-14  5:35 UTC (permalink / raw)
  To: robh+dt, mark.rutland, dan.j.williams, vinod.koul, michal.simek,
	soren.brinkmann, appanad, moritz.fischer, laurent.pinchart, luis,
	Jose.Abreu
  Cc: dmaengine, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <1484372155-19423-1-git-send-email-appanad@xilinx.com>

Add variable for checking channel idle state to ensure that dma descriptor is not
Submitted when DMA engine is in progress.

This will avoids the pollling for a bit in the status register to know
Dma state in the driver hot path.

Reviewed-by: Jose Abreu <joabreu@synopsys.com>
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Changes for v6:
---> Updated commit message as suggested by Vinod.
---> Added Channel idle variable description in the driver
     as suggested by Vinod.
Changes for v5:
---> None.
Changes for v4:
---> None.
Changes for v3:
---> None.
Changes for v2:
---> Add idle check in the reset as suggested by Jose Abreu
---> Removed xilinx_dma_is_running/xilinx_dma_is_idle checks
    in the driver and used common idle checks across the driver
    as suggested by Laurent Pinchart.

 drivers/dma/xilinx/xilinx_dma.c | 61 +++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 39 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 8288fe4..5eeea57 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -321,6 +321,7 @@ struct xilinx_dma_tx_descriptor {
  * @cyclic: Check for cyclic transfers.
  * @genlock: Support genlock mode
  * @err: Channel has errors
+ * @idle: Check for channel idle
  * @tasklet: Cleanup work after irq
  * @config: Device configuration info
  * @flush_on_fsync: Flush on Frame sync
@@ -351,6 +352,7 @@ struct xilinx_dma_chan {
 	bool cyclic;
 	bool genlock;
 	bool err;
+	bool idle;
 	struct tasklet_struct tasklet;
 	struct xilinx_vdma_config config;
 	bool flush_on_fsync;
@@ -920,32 +922,6 @@ static enum dma_status xilinx_dma_tx_status(struct dma_chan *dchan,
 }
 
 /**
- * xilinx_dma_is_running - Check if DMA channel is running
- * @chan: Driver specific DMA channel
- *
- * Return: '1' if running, '0' if not.
- */
-static bool xilinx_dma_is_running(struct xilinx_dma_chan *chan)
-{
-	return !(dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
-		 XILINX_DMA_DMASR_HALTED) &&
-		(dma_ctrl_read(chan, XILINX_DMA_REG_DMACR) &
-		 XILINX_DMA_DMACR_RUNSTOP);
-}
-
-/**
- * xilinx_dma_is_idle - Check if DMA channel is idle
- * @chan: Driver specific DMA channel
- *
- * Return: '1' if idle, '0' if not.
- */
-static bool xilinx_dma_is_idle(struct xilinx_dma_chan *chan)
-{
-	return dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
-		XILINX_DMA_DMASR_IDLE;
-}
-
-/**
  * xilinx_dma_halt - Halt DMA channel
  * @chan: Driver specific DMA channel
  */
@@ -966,6 +942,7 @@ static void xilinx_dma_halt(struct xilinx_dma_chan *chan)
 			chan, dma_ctrl_read(chan, XILINX_DMA_REG_DMASR));
 		chan->err = true;
 	}
+	chan->idle = true;
 }
 
 /**
@@ -1007,6 +984,9 @@ static void xilinx_vdma_start_transfer(struct xilinx_dma_chan *chan)
 	if (chan->err)
 		return;
 
+	if (!chan->idle)
+		return;
+
 	if (list_empty(&chan->pending_list))
 		return;
 
@@ -1018,13 +998,6 @@ static void xilinx_vdma_start_transfer(struct xilinx_dma_chan *chan)
 	tail_segment = list_last_entry(&tail_desc->segments,
 				       struct xilinx_vdma_tx_segment, node);
 
-	/* If it is SG mode and hardware is busy, cannot submit */
-	if (chan->has_sg && xilinx_dma_is_running(chan) &&
-	    !xilinx_dma_is_idle(chan)) {
-		dev_dbg(chan->dev, "DMA controller still busy\n");
-		return;
-	}
-
 	/*
 	 * If hardware is idle, then all descriptors on the running lists are
 	 * done, start new transfers
@@ -1110,6 +1083,7 @@ static void xilinx_vdma_start_transfer(struct xilinx_dma_chan *chan)
 		vdma_desc_write(chan, XILINX_DMA_REG_VSIZE, last->hw.vsize);
 	}
 
+	chan->idle = false;
 	if (!chan->has_sg) {
 		list_del(&desc->node);
 		list_add_tail(&desc->node, &chan->active_list);
@@ -1136,6 +1110,9 @@ static void xilinx_cdma_start_transfer(struct xilinx_dma_chan *chan)
 	if (chan->err)
 		return;
 
+	if (!chan->idle)
+		return;
+
 	if (list_empty(&chan->pending_list))
 		return;
 
@@ -1181,6 +1158,7 @@ static void xilinx_cdma_start_transfer(struct xilinx_dma_chan *chan)
 
 	list_splice_tail_init(&chan->pending_list, &chan->active_list);
 	chan->desc_pendingcount = 0;
+	chan->idle = false;
 }
 
 /**
@@ -1196,15 +1174,11 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan)
 	if (chan->err)
 		return;
 
-	if (list_empty(&chan->pending_list))
+	if (!chan->idle)
 		return;
 
-	/* If it is SG mode and hardware is busy, cannot submit */
-	if (chan->has_sg && xilinx_dma_is_running(chan) &&
-	    !xilinx_dma_is_idle(chan)) {
-		dev_dbg(chan->dev, "DMA controller still busy\n");
+	if (list_empty(&chan->pending_list))
 		return;
-	}
 
 	head_desc = list_first_entry(&chan->pending_list,
 				     struct xilinx_dma_tx_descriptor, node);
@@ -1302,6 +1276,7 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan)
 
 	list_splice_tail_init(&chan->pending_list, &chan->active_list);
 	chan->desc_pendingcount = 0;
+	chan->idle = false;
 }
 
 /**
@@ -1366,6 +1341,7 @@ static int xilinx_dma_reset(struct xilinx_dma_chan *chan)
 	}
 
 	chan->err = false;
+	chan->idle = true;
 
 	return err;
 }
@@ -1447,6 +1423,7 @@ static irqreturn_t xilinx_dma_irq_handler(int irq, void *data)
 	if (status & XILINX_DMA_DMASR_FRM_CNT_IRQ) {
 		spin_lock(&chan->lock);
 		xilinx_dma_complete_descriptor(chan);
+		chan->idle = true;
 		chan->start_transfer(chan);
 		spin_unlock(&chan->lock);
 	}
@@ -2327,6 +2304,12 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
 	chan->has_sg = xdev->has_sg;
 	chan->desc_pendingcount = 0x0;
 	chan->ext_addr = xdev->ext_addr;
+	/* This variable enusres that descripotrs are not
+	 * Submited when dma engine is in progress. This variable is
+	 * Added to avoid pollling for a bit in the status register to
+	 * Know dma state in the driver hot path.
+	 */
+	chan->idle = true;
 
 	spin_lock_init(&chan->lock);
 	INIT_LIST_HEAD(&chan->pending_list);
-- 
2.1.2

^ permalink raw reply related

* [PATCH v6 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma
From: Kedareswara rao Appana @ 2017-01-14  5:35 UTC (permalink / raw)
  To: robh+dt, mark.rutland, dan.j.williams, vinod.koul, michal.simek,
	soren.brinkmann, appanad, moritz.fischer, laurent.pinchart, luis,
	Jose.Abreu
  Cc: dmaengine, linux-arm-kernel, linux-kernel, devicetree
In-Reply-To: <1484372155-19423-1-git-send-email-appanad@xilinx.com>

When VDMA is configured for more than one frame in the h/w.
For example h/w is configured for n number of frames, user
Submits n number of frames and triggered the DMA using issue_pending API.

In the current driver flow we are submitting one frame at a time,
But we should submit all the n number of frames at one time
As the h/w is configured for n number of frames.

This patch fixes this issue.

Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Jose Abreu <joabreu@synopsys.com>
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Changes for v6:
---> Added Rob Acked-by
---> Updated commit message as suggested by Vinod.
Changes for v5:
---> Updated xlnx,fstore-config property to xlnx,fstore-enable
     and updated description as suggested by Rob.
Changes for v4:
---> Add Check for framestore configuration on Transmit case as well
     as suggested by Jose Abreu.
---> Modified the dev_dbg checks to dev_warn checks as suggested
     by Jose Abreu.
Changes for v3:
---> Added Checks for frame store configuration. If frame store
     Configuration is not present at the h/w level and user
     Submits less frames added debug prints in the driver as relevant.
Changes for v2:
---> Fixed race conditions in the driver as suggested by Jose Abreu
---> Fixed unnecessray if else checks in the vdma_start_transfer
     as suggested by Laurent Pinchart.

 .../devicetree/bindings/dma/xilinx/xilinx_dma.txt  |  2 +
 drivers/dma/xilinx/xilinx_dma.c                    | 78 +++++++++++++++-------
 2 files changed, 57 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
index a2b8bfa..e951c09 100644
--- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
+++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
@@ -66,6 +66,8 @@ Optional child node properties:
 Optional child node properties for VDMA:
 - xlnx,genlock-mode: Tells Genlock synchronization is
 	enabled/disabled in hardware.
+- xlnx,fstore-enable: boolean; if defined, it indicates that controller
+	supports frame store configuration.
 Optional child node properties for AXI DMA:
 -dma-channels: Number of dma channels in child node.
 
diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 5eeea57..edb5b71 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -322,6 +322,7 @@ struct xilinx_dma_tx_descriptor {
  * @genlock: Support genlock mode
  * @err: Channel has errors
  * @idle: Check for channel idle
+ * @has_fstoreen: Check for frame store configuration
  * @tasklet: Cleanup work after irq
  * @config: Device configuration info
  * @flush_on_fsync: Flush on Frame sync
@@ -353,6 +354,7 @@ struct xilinx_dma_chan {
 	bool genlock;
 	bool err;
 	bool idle;
+	bool has_fstoreen;
 	struct tasklet_struct tasklet;
 	struct xilinx_vdma_config config;
 	bool flush_on_fsync;
@@ -990,6 +992,27 @@ static void xilinx_vdma_start_transfer(struct xilinx_dma_chan *chan)
 	if (list_empty(&chan->pending_list))
 		return;
 
+	/*
+	 * Note: When VDMA is built with default h/w configuration
+	 * User should submit frames upto H/W configured.
+	 * If users submits less than h/w configured
+	 * VDMA engine tries to write to a invalid location
+	 * Results undefined behaviour/memory corruption.
+	 *
+	 * If user would like to submit frames less than h/w capable
+	 * On S2MM side please enable debug info 13 at the h/w level
+	 * On MM2S side please enable debug info 6 at the h/w level
+	 * It will allows the frame buffers numbers to be modified at runtime.
+	 */
+	if (!chan->has_fstoreen &&
+	     chan->desc_pendingcount < chan->num_frms) {
+		dev_warn(chan->dev, "Frame Store Configuration is not enabled at the\n");
+		dev_warn(chan->dev, "H/w level enable Debug info 13 or 6 at the h/w level\n");
+		dev_warn(chan->dev, "OR Submit the frames upto h/w Capable\n\r");
+
+		return;
+	}
+
 	desc = list_first_entry(&chan->pending_list,
 				struct xilinx_dma_tx_descriptor, node);
 	tail_desc = list_last_entry(&chan->pending_list,
@@ -1052,25 +1075,38 @@ static void xilinx_vdma_start_transfer(struct xilinx_dma_chan *chan)
 	if (chan->has_sg) {
 		dma_ctrl_write(chan, XILINX_DMA_REG_TAILDESC,
 				tail_segment->phys);
+		list_splice_tail_init(&chan->pending_list, &chan->active_list);
+		chan->desc_pendingcount = 0;
 	} else {
 		struct xilinx_vdma_tx_segment *segment, *last = NULL;
-		int i = 0;
+		int i = 0, j = 0;
 
 		if (chan->desc_submitcount < chan->num_frms)
 			i = chan->desc_submitcount;
 
-		list_for_each_entry(segment, &desc->segments, node) {
-			if (chan->ext_addr)
-				vdma_desc_write_64(chan,
-					XILINX_VDMA_REG_START_ADDRESS_64(i++),
-					segment->hw.buf_addr,
-					segment->hw.buf_addr_msb);
-			else
-				vdma_desc_write(chan,
-					XILINX_VDMA_REG_START_ADDRESS(i++),
-					segment->hw.buf_addr);
-
-			last = segment;
+		for (j = 0; j < chan->num_frms; ) {
+			list_for_each_entry(segment, &desc->segments, node) {
+				if (chan->ext_addr)
+					vdma_desc_write_64(chan,
+					  XILINX_VDMA_REG_START_ADDRESS_64(i++),
+					  segment->hw.buf_addr,
+					  segment->hw.buf_addr_msb);
+				else
+					vdma_desc_write(chan,
+					    XILINX_VDMA_REG_START_ADDRESS(i++),
+					    segment->hw.buf_addr);
+
+				last = segment;
+			}
+			list_del(&desc->node);
+			list_add_tail(&desc->node, &chan->active_list);
+			j++;
+			if (list_empty(&chan->pending_list) ||
+			    (i == chan->num_frms))
+				break;
+			desc = list_first_entry(&chan->pending_list,
+						struct xilinx_dma_tx_descriptor,
+						node);
 		}
 
 		if (!last)
@@ -1081,20 +1117,14 @@ static void xilinx_vdma_start_transfer(struct xilinx_dma_chan *chan)
 		vdma_desc_write(chan, XILINX_DMA_REG_FRMDLY_STRIDE,
 				last->hw.stride);
 		vdma_desc_write(chan, XILINX_DMA_REG_VSIZE, last->hw.vsize);
-	}
 
-	chan->idle = false;
-	if (!chan->has_sg) {
-		list_del(&desc->node);
-		list_add_tail(&desc->node, &chan->active_list);
-		chan->desc_submitcount++;
-		chan->desc_pendingcount--;
+		chan->desc_submitcount += j;
+		chan->desc_pendingcount -= j;
 		if (chan->desc_submitcount == chan->num_frms)
 			chan->desc_submitcount = 0;
-	} else {
-		list_splice_tail_init(&chan->pending_list, &chan->active_list);
-		chan->desc_pendingcount = 0;
 	}
+
+	chan->idle = false;
 }
 
 /**
@@ -1342,6 +1372,7 @@ static int xilinx_dma_reset(struct xilinx_dma_chan *chan)
 
 	chan->err = false;
 	chan->idle = true;
+	chan->desc_submitcount = 0;
 
 	return err;
 }
@@ -2320,6 +2351,7 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
 	has_dre = of_property_read_bool(node, "xlnx,include-dre");
 
 	chan->genlock = of_property_read_bool(node, "xlnx,genlock-mode");
+	chan->has_fstoreen = of_property_read_bool(node, "xlnx,fstore-enable");
 
 	err = of_property_read_u32(node, "xlnx,datawidth", &value);
 	if (err) {
-- 
2.1.2

^ permalink raw reply related

* [PATCH v6 3/3] dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario
From: Kedareswara rao Appana @ 2017-01-14  5:35 UTC (permalink / raw)
  To: robh+dt, mark.rutland, dan.j.williams, vinod.koul, michal.simek,
	soren.brinkmann, appanad, moritz.fischer, laurent.pinchart, luis,
	Jose.Abreu
  Cc: dmaengine, linux-arm-kernel, linux-kernel, devicetree
In-Reply-To: <1484372155-19423-1-git-send-email-appanad@xilinx.com>

As per AXI DMA spec the software must not move the tail pointer to a location
That has not been updated (next descriptor field of the h/w descriptor
Should always point to a valid address).

When user submits multiple descriptors on the recv side, with the
Current driver flow the last buffer descriptor next descriptor field
Points to a invalid location, resulting the invalid data or errors in the
DMA engine.

This patch fixes this issue by creating a Buffer Descritpor Chain during
Channel allocation itself and use those Buffer Descriptors.

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Changes for v6:
---> Updated Commit message as suggested by Vinod.
Changes for v5:
---> None.
Changes for v4:
---> None.
Changes for v3:
---> None.
Changes for v2:
---> None.

 drivers/dma/xilinx/xilinx_dma.c | 133 +++++++++++++++++++++++++---------------
 1 file changed, 83 insertions(+), 50 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index edb5b71..c5cd935 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -163,6 +163,7 @@
 #define XILINX_DMA_BD_SOP		BIT(27)
 #define XILINX_DMA_BD_EOP		BIT(26)
 #define XILINX_DMA_COALESCE_MAX		255
+#define XILINX_DMA_NUM_DESCS		255
 #define XILINX_DMA_NUM_APP_WORDS	5
 
 /* Multi-Channel DMA Descriptor offsets*/
@@ -310,6 +311,7 @@ struct xilinx_dma_tx_descriptor {
  * @pending_list: Descriptors waiting
  * @active_list: Descriptors ready to submit
  * @done_list: Complete descriptors
+ * @free_seg_list: Free descriptors
  * @common: DMA common channel
  * @desc_pool: Descriptors pool
  * @dev: The dma device
@@ -331,7 +333,9 @@ struct xilinx_dma_tx_descriptor {
  * @desc_submitcount: Descriptor h/w submitted count
  * @residue: Residue for AXI DMA
  * @seg_v: Statically allocated segments base
+ * @seg_p: Physical allocated segments base
  * @cyclic_seg_v: Statically allocated segment base for cyclic transfers
+ * @cyclic_seg_p: Physical allocated segments base for cyclic dma
  * @start_transfer: Differentiate b/w DMA IP's transfer
  */
 struct xilinx_dma_chan {
@@ -342,6 +346,7 @@ struct xilinx_dma_chan {
 	struct list_head pending_list;
 	struct list_head active_list;
 	struct list_head done_list;
+	struct list_head free_seg_list;
 	struct dma_chan common;
 	struct dma_pool *desc_pool;
 	struct device *dev;
@@ -363,7 +368,9 @@ struct xilinx_dma_chan {
 	u32 desc_submitcount;
 	u32 residue;
 	struct xilinx_axidma_tx_segment *seg_v;
+	dma_addr_t seg_p;
 	struct xilinx_axidma_tx_segment *cyclic_seg_v;
+	dma_addr_t cyclic_seg_p;
 	void (*start_transfer)(struct xilinx_dma_chan *chan);
 	u16 tdest;
 };
@@ -569,17 +576,31 @@ static struct xilinx_axidma_tx_segment *
 xilinx_axidma_alloc_tx_segment(struct xilinx_dma_chan *chan)
 {
 	struct xilinx_axidma_tx_segment *segment;
-	dma_addr_t phys;
-
-	segment = dma_pool_zalloc(chan->desc_pool, GFP_ATOMIC, &phys);
-	if (!segment)
-		return NULL;
+	unsigned long flags;
 
-	segment->phys = phys;
+	spin_lock_irqsave(&chan->lock, flags);
+	if (!list_empty(&chan->free_seg_list)) {
+		segment = list_first_entry(&chan->free_seg_list,
+					   struct xilinx_axidma_tx_segment,
+					   node);
+		list_del(&segment->node);
+	}
+	spin_unlock_irqrestore(&chan->lock, flags);
 
 	return segment;
 }
 
+static void xilinx_dma_clean_hw_desc(struct xilinx_axidma_desc_hw *hw)
+{
+	u32 next_desc = hw->next_desc;
+	u32 next_desc_msb = hw->next_desc_msb;
+
+	memset(hw, 0, sizeof(struct xilinx_axidma_desc_hw));
+
+	hw->next_desc = next_desc;
+	hw->next_desc_msb = next_desc_msb;
+}
+
 /**
  * xilinx_dma_free_tx_segment - Free transaction segment
  * @chan: Driver specific DMA channel
@@ -588,7 +609,9 @@ xilinx_axidma_alloc_tx_segment(struct xilinx_dma_chan *chan)
 static void xilinx_dma_free_tx_segment(struct xilinx_dma_chan *chan,
 				struct xilinx_axidma_tx_segment *segment)
 {
-	dma_pool_free(chan->desc_pool, segment, segment->phys);
+	xilinx_dma_clean_hw_desc(&segment->hw);
+
+	list_add_tail(&segment->node, &chan->free_seg_list);
 }
 
 /**
@@ -713,16 +736,26 @@ static void xilinx_dma_free_descriptors(struct xilinx_dma_chan *chan)
 static void xilinx_dma_free_chan_resources(struct dma_chan *dchan)
 {
 	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
+	unsigned long flags;
 
 	dev_dbg(chan->dev, "Free all channel resources.\n");
 
 	xilinx_dma_free_descriptors(chan);
+
 	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
-		xilinx_dma_free_tx_segment(chan, chan->cyclic_seg_v);
-		xilinx_dma_free_tx_segment(chan, chan->seg_v);
+		spin_lock_irqsave(&chan->lock, flags);
+		INIT_LIST_HEAD(&chan->free_seg_list);
+		spin_unlock_irqrestore(&chan->lock, flags);
+
+		/* Free Memory that is allocated for cyclic DMA Mode */
+		dma_free_coherent(chan->dev, sizeof(*chan->cyclic_seg_v),
+				  chan->cyclic_seg_v, chan->cyclic_seg_p);
+	}
+
+	if (chan->xdev->dma_config->dmatype != XDMA_TYPE_AXIDMA) {
+		dma_pool_destroy(chan->desc_pool);
+		chan->desc_pool = NULL;
 	}
-	dma_pool_destroy(chan->desc_pool);
-	chan->desc_pool = NULL;
 }
 
 /**
@@ -805,6 +838,7 @@ static void xilinx_dma_do_tasklet(unsigned long data)
 static int xilinx_dma_alloc_chan_resources(struct dma_chan *dchan)
 {
 	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
+	int i;
 
 	/* Has this channel already been allocated? */
 	if (chan->desc_pool)
@@ -815,11 +849,30 @@ static int xilinx_dma_alloc_chan_resources(struct dma_chan *dchan)
 	 * for meeting Xilinx VDMA specification requirement.
 	 */
 	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
-		chan->desc_pool = dma_pool_create("xilinx_dma_desc_pool",
-				   chan->dev,
-				   sizeof(struct xilinx_axidma_tx_segment),
-				   __alignof__(struct xilinx_axidma_tx_segment),
-				   0);
+		/* Allocate the buffer descriptors. */
+		chan->seg_v = dma_zalloc_coherent(chan->dev,
+						  sizeof(*chan->seg_v) *
+						  XILINX_DMA_NUM_DESCS,
+						  &chan->seg_p, GFP_KERNEL);
+		if (!chan->seg_v) {
+			dev_err(chan->dev,
+				"unable to allocate channel %d descriptors\n",
+				chan->id);
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < XILINX_DMA_NUM_DESCS; i++) {
+			chan->seg_v[i].hw.next_desc =
+			lower_32_bits(chan->seg_p + sizeof(*chan->seg_v) *
+				((i + 1) % XILINX_DMA_NUM_DESCS));
+			chan->seg_v[i].hw.next_desc_msb =
+			upper_32_bits(chan->seg_p + sizeof(*chan->seg_v) *
+				((i + 1) % XILINX_DMA_NUM_DESCS));
+			chan->seg_v[i].phys = chan->seg_p +
+				sizeof(*chan->seg_v) * i;
+			list_add_tail(&chan->seg_v[i].node,
+				      &chan->free_seg_list);
+		}
 	} else if (chan->xdev->dma_config->dmatype == XDMA_TYPE_CDMA) {
 		chan->desc_pool = dma_pool_create("xilinx_cdma_desc_pool",
 				   chan->dev,
@@ -834,7 +887,8 @@ static int xilinx_dma_alloc_chan_resources(struct dma_chan *dchan)
 				     0);
 	}
 
-	if (!chan->desc_pool) {
+	if (!chan->desc_pool &&
+	    (chan->xdev->dma_config->dmatype != XDMA_TYPE_AXIDMA)) {
 		dev_err(chan->dev,
 			"unable to allocate channel %d descriptor pool\n",
 			chan->id);
@@ -843,22 +897,20 @@ static int xilinx_dma_alloc_chan_resources(struct dma_chan *dchan)
 
 	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
 		/*
-		 * For AXI DMA case after submitting a pending_list, keep
-		 * an extra segment allocated so that the "next descriptor"
-		 * pointer on the tail descriptor always points to a
-		 * valid descriptor, even when paused after reaching taildesc.
-		 * This way, it is possible to issue additional
-		 * transfers without halting and restarting the channel.
-		 */
-		chan->seg_v = xilinx_axidma_alloc_tx_segment(chan);
-
-		/*
 		 * For cyclic DMA mode we need to program the tail Descriptor
 		 * register with a value which is not a part of the BD chain
 		 * so allocating a desc segment during channel allocation for
 		 * programming tail descriptor.
 		 */
-		chan->cyclic_seg_v = xilinx_axidma_alloc_tx_segment(chan);
+		chan->cyclic_seg_v = dma_zalloc_coherent(chan->dev,
+					sizeof(*chan->cyclic_seg_v),
+					&chan->cyclic_seg_p, GFP_KERNEL);
+		if (!chan->cyclic_seg_v) {
+			dev_err(chan->dev,
+				"unable to allocate desc segment for cyclic DMA\n");
+			return -ENOMEM;
+		}
+		chan->cyclic_seg_v->phys = chan->cyclic_seg_p;
 	}
 
 	dma_cookie_init(dchan);
@@ -1198,7 +1250,7 @@ static void xilinx_cdma_start_transfer(struct xilinx_dma_chan *chan)
 static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan)
 {
 	struct xilinx_dma_tx_descriptor *head_desc, *tail_desc;
-	struct xilinx_axidma_tx_segment *tail_segment, *old_head, *new_head;
+	struct xilinx_axidma_tx_segment *tail_segment;
 	u32 reg;
 
 	if (chan->err)
@@ -1217,21 +1269,6 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan)
 	tail_segment = list_last_entry(&tail_desc->segments,
 				       struct xilinx_axidma_tx_segment, node);
 
-	if (chan->has_sg && !chan->xdev->mcdma) {
-		old_head = list_first_entry(&head_desc->segments,
-					struct xilinx_axidma_tx_segment, node);
-		new_head = chan->seg_v;
-		/* Copy Buffer Descriptor fields. */
-		new_head->hw = old_head->hw;
-
-		/* Swap and save new reserve */
-		list_replace_init(&old_head->node, &new_head->node);
-		chan->seg_v = old_head;
-
-		tail_segment->hw.next_desc = chan->seg_v->phys;
-		head_desc->async_tx.phys = new_head->phys;
-	}
-
 	reg = dma_ctrl_read(chan, XILINX_DMA_REG_DMACR);
 
 	if (chan->desc_pendingcount <= XILINX_DMA_COALESCE_MAX) {
@@ -1729,7 +1766,7 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
 {
 	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
 	struct xilinx_dma_tx_descriptor *desc;
-	struct xilinx_axidma_tx_segment *segment = NULL, *prev = NULL;
+	struct xilinx_axidma_tx_segment *segment = NULL;
 	u32 *app_w = (u32 *)context;
 	struct scatterlist *sg;
 	size_t copy;
@@ -1780,10 +1817,6 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
 					       XILINX_DMA_NUM_APP_WORDS);
 			}
 
-			if (prev)
-				prev->hw.next_desc = segment->phys;
-
-			prev = segment;
 			sg_used += copy;
 
 			/*
@@ -1797,7 +1830,6 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
 	segment = list_first_entry(&desc->segments,
 				   struct xilinx_axidma_tx_segment, node);
 	desc->async_tx.phys = segment->phys;
-	prev->hw.next_desc = segment->phys;
 
 	/* For the last DMA_MEM_TO_DEV transfer, set EOP */
 	if (chan->direction == DMA_MEM_TO_DEV) {
@@ -2346,6 +2378,7 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
 	INIT_LIST_HEAD(&chan->pending_list);
 	INIT_LIST_HEAD(&chan->done_list);
 	INIT_LIST_HEAD(&chan->active_list);
+	INIT_LIST_HEAD(&chan->free_seg_list);
 
 	/* Retrieve the channel properties from the device tree */
 	has_dre = of_property_read_bool(node, "xlnx,include-dre");
-- 
2.1.2

^ permalink raw reply related

* Re: [PATCH v3 6/7] devicetree: power: bq27xxx: add monitored battery documentation
From: Matt Ranostay @ 2017-01-14  6:07 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Sebastian Reichel,
	Tony Lindgren
In-Reply-To: <20170113172821.noogiax7bqwhkoev@rob-hp-laptop>

On Fri, Jan 13, 2017 at 9:28 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Jan 10, 2017 at 10:20:02PM -0800, Matt Ranostay wrote:
>> Depends-On: http://marc.info/?l=linux-pm&m=148392292830015&w=2
>> Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/power/supply/bq27xxx.txt | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/bq27xxx.txt b/Documentation/devicetree/bindings/power/supply/bq27xxx.txt
>> index b0c95ef63e68..0472a2db0f13 100644
>> --- a/Documentation/devicetree/bindings/power/supply/bq27xxx.txt
>> +++ b/Documentation/devicetree/bindings/power/supply/bq27xxx.txt
>> @@ -28,9 +28,17 @@ Required properties:
>>   * "ti,bq27621" - BQ27621
>>  - reg: integer, i2c address of the device.
>>
>> +Optional properties:
>> +- monitored-battery: phandle of battery information devicetree node
>
> We need a common way to describe charger/monitor to battery connections,
> not yet another way. The battery and power supply related bindings are a
> bit of a mess from what I've looked at.

Sebastian, your thoughts here?

Thanks,

Matt

>
> Rob
--
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

* [PATCH v1 1/3] dt: bindings: add documentation for zx2967 family reset controller
From: Baoyou Xie @ 2017-01-14  7:05 UTC (permalink / raw)
  To: jun.nie, p.zabel, robh+dt, mark.rutland
  Cc: devicetree, xie.baoyou, linux-kernel, chen.chaokai, wang.qiang01,
	baoyou.xie, shawnguo, linux-arm-kernel

This patch adds dt-binding documentation for zx2967 family
reset controller.

Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
---
 .../devicetree/bindings/reset/zte,zx2967-reset.txt   | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/zte,zx2967-reset.txt

diff --git a/Documentation/devicetree/bindings/reset/zte,zx2967-reset.txt b/Documentation/devicetree/bindings/reset/zte,zx2967-reset.txt
new file mode 100644
index 0000000..22d590e
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/zte,zx2967-reset.txt
@@ -0,0 +1,20 @@
+ZTE zx2967 SoCs Reset Controller
+=======================================
+
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+Required properties:
+- compatible: should be one of the following.
+	* zte,zx296718-reset
+- reg: physical base address of the controller and length of memory mapped
+	region.
+- #reset-cells: must be 1.
+
+example:
+
+	toprst: reset@1461060 {
+		compatible = "zte,zx296718-reset";
+		reg = <0x01461060 0x8>;
+		#reset-cells = <1>;
+	};
-- 
2.7.4

^ permalink raw reply related

* [PATCH v1 2/3] MAINTAINERS: add zx2967 reset controller driver to ARM ZTE architecture
From: Baoyou Xie @ 2017-01-14  7:05 UTC (permalink / raw)
  To: jun.nie, p.zabel, robh+dt, mark.rutland
  Cc: devicetree, xie.baoyou, linux-kernel, chen.chaokai, wang.qiang01,
	baoyou.xie, shawnguo, linux-arm-kernel
In-Reply-To: <1484377530-30635-1-git-send-email-baoyou.xie@linaro.org>

Add the zx2967 reset controller driver as maintained by ARM ZTE
architecture maintainers, as they're parts of the core IP.

Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2793808..08f8155 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1980,10 +1980,12 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 F:	arch/arm/mach-zx/
 F:	drivers/clk/zte/
+F:	drivers/reset/reset-zx2967.c
 F:	drivers/soc/zte/
 F:	drivers/thermal/zx*
 F:	Documentation/devicetree/bindings/arm/zte.txt
 F:	Documentation/devicetree/bindings/clock/zx296702-clk.txt
+F:	Documentation/devicetree/bindings/reset/zte,zx2967-reset.txt
 F:	Documentation/devicetree/bindings/soc/zte/
 F:	Documentation/devicetree/bindings/thermal/zx*
 F:	include/dt-bindings/soc/zx*.h
-- 
2.7.4

^ permalink raw reply related

* [PATCH v1 3/3] reset: zx2967: add reset controller driver for ZTE's zx2967 family
From: Baoyou Xie @ 2017-01-14  7:05 UTC (permalink / raw)
  To: jun.nie, p.zabel, robh+dt, mark.rutland
  Cc: devicetree, xie.baoyou, linux-kernel, chen.chaokai, wang.qiang01,
	baoyou.xie, shawnguo, linux-arm-kernel
In-Reply-To: <1484377530-30635-1-git-send-email-baoyou.xie@linaro.org>

This patch adds reset controller driver for ZTE's zx2967 family.

Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
---
 drivers/reset/Kconfig        |   6 ++
 drivers/reset/Makefile       |   1 +
 drivers/reset/reset-zx2967.c | 136 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+)
 create mode 100644 drivers/reset/reset-zx2967.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 172dc96..972d077 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -92,6 +92,12 @@ config RESET_ZYNQ
 	help
 	  This enables the reset controller driver for Xilinx Zynq SoCs.
 
+config RESET_ZX2967
+	bool "ZX2967 Reset Driver"
+	depends on ARCH_ZX || COMPILE_TEST
+	help
+	  This enables the reset controller driver for ZTE zx2967 family.
+
 source "drivers/reset/sti/Kconfig"
 source "drivers/reset/hisilicon/Kconfig"
 source "drivers/reset/tegra/Kconfig"
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 13b346e..807b77b 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o
 obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
 obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
+obj-$(CONFIG_RESET_ZX2967) += reset-zx2967.o
diff --git a/drivers/reset/reset-zx2967.c b/drivers/reset/reset-zx2967.c
new file mode 100644
index 0000000..63f9c41
--- /dev/null
+++ b/drivers/reset/reset-zx2967.c
@@ -0,0 +1,136 @@
+/*
+ * ZTE's zx2967 family thermal sensor driver
+ *
+ * Copyright (C) 2017 ZTE Ltd.
+ *
+ * Author: Baoyou Xie <baoyou.xie@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+
+struct zx2967_reset {
+	void __iomem			*reg_base;
+	spinlock_t			lock;
+	struct reset_controller_dev	rcdev;
+};
+
+static int zx2967_reset_assert(struct reset_controller_dev *rcdev,
+			   unsigned long id)
+{
+	struct zx2967_reset *reset = NULL;
+	int bank = id / 32;
+	int offset = id % 32;
+	unsigned int reg;
+	unsigned long flags;
+
+	reset = container_of(rcdev, struct zx2967_reset, rcdev);
+
+	spin_lock_irqsave(&reset->lock, flags);
+
+	reg = readl(reset->reg_base + (bank * 4));
+	writel(reg & ~BIT(offset), reset->reg_base + (bank * 4));
+	reg = readl(reset->reg_base + (bank * 4));
+
+	spin_unlock_irqrestore(&reset->lock, flags);
+
+	return 0;
+}
+
+static int zx2967_reset_deassert(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct zx2967_reset *reset = NULL;
+	int bank = id / 32;
+	int offset = id % 32;
+	unsigned int reg;
+	unsigned long flags;
+
+	reset = container_of(rcdev, struct zx2967_reset, rcdev);
+
+	spin_lock_irqsave(&reset->lock, flags);
+
+	reg = readl(reset->reg_base + (bank * 4));
+	writel(reg | BIT(offset), reset->reg_base + (bank * 4));
+	reg = readl(reset->reg_base + (bank * 4));
+
+	spin_unlock_irqrestore(&reset->lock, flags);
+
+	return 0;
+}
+
+static struct reset_control_ops zx2967_reset_ops = {
+	.assert		= zx2967_reset_assert,
+	.deassert	= zx2967_reset_deassert,
+};
+
+static int zx2967_reset_probe(struct platform_device *pdev)
+{
+	struct zx2967_reset *reset;
+	struct resource *res;
+
+	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
+	if (!reset)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reset->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(reset->reg_base))
+		return PTR_ERR(reset->reg_base);
+
+	spin_lock_init(&reset->lock);
+
+	reset->rcdev.owner = THIS_MODULE;
+	reset->rcdev.nr_resets = resource_size(res) * 8;
+	reset->rcdev.ops = &zx2967_reset_ops;
+	reset->rcdev.of_node = pdev->dev.of_node;
+
+	dev_info(&pdev->dev, "reset controller cnt:%d",
+		  reset->rcdev.nr_resets);
+
+	return reset_controller_register(&reset->rcdev);
+}
+
+static int zx2967_reset_remove(struct platform_device *pdev)
+{
+	struct zx2967_reset *reset = platform_get_drvdata(pdev);
+
+	reset_controller_unregister(&reset->rcdev);
+
+	return 0;
+}
+
+static const struct of_device_id zx2967_reset_dt_ids[] = {
+	 { .compatible = "zte,zx296718-reset", },
+	 {},
+};
+MODULE_DEVICE_TABLE(of, zx2967_reset_dt_ids);
+
+static struct platform_driver zx2967_reset_driver = {
+	.probe	= zx2967_reset_probe,
+	.remove	= zx2967_reset_remove,
+	.driver = {
+		.name		= "zx2967-reset",
+		.of_match_table	= zx2967_reset_dt_ids,
+	},
+};
+
+static int __init zx2967_reset_init(void)
+{
+	return platform_driver_register(&zx2967_reset_driver);
+}
+arch_initcall(zx2967_reset_init);
+
+static void __exit zx2967_reset_exit(void)
+{
+	platform_driver_unregister(&zx2967_reset_driver);
+}
+module_exit(zx2967_reset_exit);
+
+MODULE_AUTHOR("Baoyou Xie <baoyou.xie@linaro.org>");
+MODULE_DESCRIPTION("ZTE zx2967 Reset Controller Driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

^ permalink raw reply related

* Re: [PATCHv4 3/5] pinctrl: mvebu: pinctrl driver for 98DX3236 SoC
From: Chris Packham @ 2017-01-14  7:50 UTC (permalink / raw)
  To: Sebastian Hesselbarth, linux-arm-kernel@lists.infradead.org
  Cc: Kalyan Kinthada, Linus Walleij, Rob Herring, Mark Rutland,
	Thomas Petazzoni, Laxman Dewangan, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <2fca7ac2-9a88-19b1-28c5-5192b9c4a2bc@gmail.com>

On 13/01/17 22:54, Sebastian Hesselbarth wrote:
> On 13.01.2017 10:12, Chris Packham wrote:
>> From: Kalyan Kinthada <kalyan.kinthada@alliedtelesis.co.nz>
>>
>> This pinctrl driver supports the 98DX3236, 98DX3336 and 98DX4251 SoCs
>> from Marvell.
>>
>> Signed-off-by: Kalyan Kinthada <kalyan.kinthada@alliedtelesis.co.nz>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> ---
>>
>> Notes:
>>     Changes in v2:
>>     - include sdio support for the 98DX4251
>>     Changes in v3:
>>     - None
>>     Changes in v4:
>>     - Correct some discrepencies between binding and driver.
>
> Well, unfortunately I still see differences between the "gpio" in
> the binding and "gpo" in the driver.
>
> Please go back to that list I sent you yesterday and fix them all.
>

I think you may have missed my initial reply [1]. Or I have missed your 
response to it. Long story short "gpo" is intentional because some of 
those pins can't be used as inputs. But if you still want me to change 
it I will.

[1] - https://lkml.org/lkml/2017/1/12/117


^ permalink raw reply

* Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node
From: Boris Brezillon @ 2017-01-14  8:29 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Matthias Brugger, Guochun Mao, David Woodhouse, Brian Norris,
	Richard Weinberger, Cyrille Pitchen, Rob Herring, Mark Rutland,
	Russell King, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <0fafcd8d-cf99-de6b-728f-5e3637810b68-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On Fri, 13 Jan 2017 18:33:40 +0100
Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On 01/13/2017 05:56 PM, Boris Brezillon wrote:
> > On Fri, 13 Jan 2017 17:44:12 +0100
> > Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >   
> >> On 01/13/2017 05:28 PM, Boris Brezillon wrote:  
> >>> On Fri, 13 Jan 2017 17:13:55 +0100
> >>> Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>     
> >>>> On 01/13/2017 04:12 PM, Matthias Brugger wrote:    
> >>>>>
> >>>>>
> >>>>> On 13/01/17 15:17, Boris Brezillon wrote:      
> >>>>>> On Fri, 13 Jan 2017 15:13:29 +0800
> >>>>>> Guochun Mao <guochun.mao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >>>>>>      
> >>>>>>> Add Mediatek nor flash node.
> >>>>>>>
> >>>>>>> Signed-off-by: Guochun Mao <guochun.mao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> >>>>>>> ---
> >>>>>>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
> >>>>>>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
> >>>>>>>  2 files changed, 37 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>> b/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>> index 082ca88..85e5ae8 100644
> >>>>>>> --- a/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>> @@ -24,6 +24,31 @@
> >>>>>>>      };
> >>>>>>>  };
> >>>>>>>
> >>>>>>> +&nor_flash {
> >>>>>>> +    pinctrl-names = "default";
> >>>>>>> +    pinctrl-0 = <&nor_pins_default>;
> >>>>>>> +    status = "okay";
> >>>>>>> +    flash@0 {
> >>>>>>> +        compatible = "jedec,spi-nor";
> >>>>>>> +        reg = <0>;
> >>>>>>> +    };
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +&pio {
> >>>>>>> +    nor_pins_default: nor {
> >>>>>>> +        pins1 {
> >>>>>>> +            pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
> >>>>>>> +                 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
> >>>>>>> +                 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
> >>>>>>> +                 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
> >>>>>>> +                 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
> >>>>>>> +                 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
> >>>>>>> +            drive-strength = <MTK_DRIVE_4mA>;
> >>>>>>> +            bias-pull-up;
> >>>>>>> +        };
> >>>>>>> +    };
> >>>>>>> +};
> >>>>>>> +
> >>>>>>>  &uart0 {
> >>>>>>>      status = "okay";
> >>>>>>>  };
> >>>>>>> diff --git a/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>> b/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>> index bdf8954..1eefce4 100644
> >>>>>>> --- a/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>> +++ b/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>> @@ -227,6 +227,18 @@
> >>>>>>>          status = "disabled";
> >>>>>>>      };
> >>>>>>>
> >>>>>>> +    nor_flash: spi@11014000 {
> >>>>>>> +        compatible = "mediatek,mt2701-nor",
> >>>>>>> +                 "mediatek,mt8173-nor";      
> >>>>>>
> >>>>>> Why define both here? Is "mediatek,mt8173-nor" really providing a
> >>>>>> subset of the features supported by "mediatek,mt2701-nor"?
> >>>>>>      
> >>>>>
> >>>>> I think even if the ip block is the same, we should provide both
> >>>>> bindings, just in case in the future we find out that mt2701 has some
> >>>>> hidden bug, feature or bug-feature. This way even if we update the
> >>>>> driver, we stay compatible with older device tree blobs in the wild.
> >>>>>
> >>>>> We can drop the mt2701-nor in the bindings definition if you want.     
> >>>
> >>> Oh, sorry, I misunderstood. What I meant is that if you want to
> >>> list/support all possible compatibles, maybe you should just put one
> >>> compatible in your DT and patch your driver (+ binding doc) to define
> >>> all of them.    
> >>
> >> Uh, what ? I lost you here :-)

I mean adding a new entry in the mtk_nor_of_ids table (in
mtk-quadspi.c) so that the mediatek,mt2701-nor compatible string can be
matched directly, and you won't need to define 2 compatible strings in
your device tree.

> >>  
> >>>> This exactly. We should have a DT compat in the form:
> >>>> compatible = "vendor,<soc>-block", "vendor,<oldest-compat-soc>-block";
> >>>> Then if we find a problem in the future, we can match on the
> >>>> "vendor,<soc>-block" and still support the old DTs.    
> >>>
> >>> Not sure it's only in term of whose IP appeared first. My understanding
> >>> is that it's a way to provide inheritance. For example:
> >>>
> >>> 	"<soc-vendor>,<ip-version>", "<ip-vendor>,<ip-version>";
> >>>
> >>> or
> >>>
> >>> 	"<soc-vendor>,<full-featured-ip-version>","<soc-vendor>,<basic-feature-ip-version>";
> >>>
> >>> BTW, which one is the oldest between mt8173 and mt2701? :-)    
> >>
> >> And that's another thing and I agree with you, but I don't think that's
> >> what we're discussing in this thread. But (!), OT, I think we should
> >> codify the rules in Documentation/ . This discussion came up multiple
> >> times recently.
> >>
> >> And my question still stands, what do we put into the DT here, IMO
> >> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";  
> > 
> > I'd say
> > 
> > 	compatible = "mediatek,mt8173-nor";
> > 
> > because both compatible are referring to very specific IP version. It's
> > not the same as  
> 
> But then you don't have the ability to handle a block in this particular
> SoC in case there's a bug found in it in the future,
> so IMO it should be:
> 
> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";

Sorry again, I meant

	compatible = "mediatek,mt2701-nor";

> 
> > 	compatible = "mediatek,mt8173-nor", "mediatek,mt81xx-nor";  
> 
> This doesn't look right, since here we add two new compatibles ...

That was just an example to describe how compatible inheritance works
(at least that's my understanding of it), it does not apply to this
particular use case.

> 
> > where you clearly have a generic compatible which is overloaded by a
> > specific one.
> > 
> > But anyway, I'm not the one taking the decision here, let's wait for DT
> > maintainers reviews.
> >   
> >> and what goes into the binding document ? I guess both too ?  
> > 
> > If both exist, they should be both documented.
> >   
> 
> 

--
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 v4 1/2] iio: adc: Add Maxim MAX11100 driver
From: Jonathan Cameron @ 2017-01-14 10:22 UTC (permalink / raw)
  To: Jacopo Mondi, wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
	magnus.damm-Re5JQEeQqe8AvxtiuMwx3w, knaack.h-Mmb7MZpHnFY,
	lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	marek.vasut-Re5JQEeQqe8AvxtiuMwx3w, geert-Td1EMuHUCqxL1ZNQvxDV9g,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1484301038-16386-2-git-send-email-jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>

On 13/01/17 09:50, Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
> 
> Add iio driver for Maxim MAX11100 single-channel ADC.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
> Tested-by: Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
Small issue with DMA using SPI controllers and the location of buffer
in your read.

Otherwise looks good to me.

Jonathan
> ---
>  drivers/iio/adc/Kconfig    |   9 +++
>  drivers/iio/adc/Makefile   |   1 +
>  drivers/iio/adc/max11100.c | 187 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 197 insertions(+)
>  create mode 100644 drivers/iio/adc/max11100.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 38bc319..c32bc7a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -307,6 +307,15 @@ config MAX1027
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1027.
>  
> +config MAX11100
> +	tristate "Maxim max11100 ADC driver"
> +	depends on SPI_MASTER
> +	help
> +	  Say yes here to build support for Maxim max11100 SPI ADC
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max11100.
> +
>  config MAX1363
>  	tristate "Maxim max1363 ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d36c4be..5684369 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_MAX1027) += max1027.o
> +obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
> new file mode 100644
> index 0000000..78e2a45
> --- /dev/null
> +++ b/drivers/iio/adc/max11100.c
> @@ -0,0 +1,187 @@
> +/*
> + * iio/adc/max11100.c
> + * Maxim max11100 ADC Driver with IIO interface
> + *
> + * Copyright (C) 2016 Renesas Electronics Corporation
> + * Copyright (C) 2016 Jacopo Mondi
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/driver.h>
> +
> +/*
> + * LSB is the ADC single digital step
> + * 1 LSB = (vref_mv / 2 ^ 16)
> + *
> + * LSB is used to calculate analog voltage value
> + * from the number of ADC steps count
> + *
> + * Ain = (count * LSB)
> + */
> +#define MAX11100_LSB_DIV		(1 << 16)
> +
> +struct max11100_state {
> +	const struct max11100_chip_desc *desc;
> +	struct regulator *vref_reg;
> +	struct spi_device *spi;
> +};
> +
> +static struct iio_chan_spec max11100_channels[] = {
> +	{ /* [0] */
> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};
> +
> +static struct max11100_chip_desc {
> +	unsigned int num_chan;
> +	const struct iio_chan_spec *channels;
> +} max11100_desc = {
> +	.num_chan = ARRAY_SIZE(max11100_channels),
> +	.channels = max11100_channels,
> +};
> +
> +static int max11100_read_single(struct iio_dev *indio_dev, int *val)
> +{
> +	int ret;
> +	struct max11100_state *state = iio_priv(indio_dev);
> +	uint8_t buffer[3];
> +
> +	ret = spi_read(state->spi, buffer, sizeof(buffer));
Buffer needs to be __cacheline_aligned to safely use spi_read on spi
controllers that use DMA.

Two easy ways of doing this, either allocate buffer using kmalloc or
take advantage of the fact that spi_priv() is cache line aligned and
stick you buffer at the end of your private structure and mark it
__cacheline_aligned to ensure it also appropriately aligned in it's own
cacheline.

> +	if (ret) {
> +		dev_err(&indio_dev->dev, "SPI transfer failed\n");
> +		return ret;
> +	}
> +
> +	/* the first 8 bits sent out from ADC must be 0s */
> +	if (buffer[0]) {
> +		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
> +		return -EINVAL;
> +	}
> +
> +	*val = (buffer[1] << 8) | buffer[2];
> +
> +	return 0;
> +}
> +
> +static int max11100_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long info)
> +{
> +	int ret, vref_uv;
> +	struct max11100_state *state = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = max11100_read_single(indio_dev, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		vref_uv = regulator_get_voltage(state->vref_reg);
> +		if (vref_uv < 0)
> +			/* dummy regulator "get_voltage" returns -EINVAL */
> +			return -EINVAL;
> +
> +		*val =  vref_uv / 1000;
> +		*val2 = MAX11100_LSB_DIV;
> +		return IIO_VAL_FRACTIONAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info max11100_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = max11100_read_raw,
> +};
> +
> +static int max11100_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct iio_dev *indio_dev;
> +	struct max11100_state *state;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->desc = &max11100_desc;
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->dev.of_node = spi->dev.of_node;
> +	indio_dev->info = &max11100_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = state->desc->channels;
> +	indio_dev->num_channels = state->desc->num_chan;
> +
> +	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(state->vref_reg))
> +		return PTR_ERR(state->vref_reg);
> +
> +	ret = regulator_enable(state->vref_reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto disable_regulator;
> +
> +	return 0;
> +
> +disable_regulator:
> +	regulator_disable(state->vref_reg);
> +
> +	return ret;
> +}
> +
> +static int max11100_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct max11100_state *state = iio_priv(indio_dev);
> +
> +	regulator_disable(state->vref_reg);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id max11100_ids[] = {
> +	{.compatible = "maxim,max11100"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, max11100_ids);
> +
> +static struct spi_driver max11100_driver = {
> +	.driver = {
> +		.name	= "max11100",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(max11100_ids),
> +	},
> +	.probe		= max11100_probe,
> +	.remove		= max11100_remove,
> +};
> +
> +module_spi_driver(max11100_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>");
> +MODULE_DESCRIPTION("Maxim max11100 ADC Driver");
> +MODULE_LICENSE("GPL v2");
> 

--
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 5/6] arm: dts: mt2701: Add ethernet device node.
From: John Crispin @ 2017-01-14 10:32 UTC (permalink / raw)
  To: Erin Lo, Matthias Brugger
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Sean Wang,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1484296978-18572-6-git-send-email-erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

Hi Erin,

small comment inline

On 13/01/2017 09:42, Erin Lo wrote:
> From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> 
> Add ethernet device node for MT2701.
> 
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Erin Lo <erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  arch/arm/boot/dts/mt2701-evb.dts | 40 ++++++++++++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/mt2701.dtsi    | 22 ++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/mt2701-evb.dts b/arch/arm/boot/dts/mt2701-evb.dts
> index a483798..40abd3b 100644
> --- a/arch/arm/boot/dts/mt2701-evb.dts
> +++ b/arch/arm/boot/dts/mt2701-evb.dts
> @@ -28,7 +28,47 @@
>  	status = "okay";
>  };
>  
> +&eth {
> +	mac-address = [00 00 00 00 00 00];
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gmac1_pins>;
> +	gmac1: mac@1 {
> +		compatible = "mediatek,eth-mac";
> +		reg = <1>;
> +		phy-handle = <&phy5>;
> +	};
> +
> +	mdio-bus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		phy5: ethernet-phy@5 {
> +			reg = <5>;
> +			phy-mode = "rgmii-rxid";
> +		};
> +	};
> +};
> +
>  &pio {
> +	gmac1_pins: eth@0 {
> +		pins_eth {
> +			pinmux = <MT2701_PIN_275_MDC__FUNC_MDC>,
> +				 <MT2701_PIN_276_MDIO__FUNC_MDIO>,
> +				 <MT2701_PIN_262_G2_TXEN__FUNC_G2_TXEN>,
> +				 <MT2701_PIN_263_G2_TXD3__FUNC_G2_TXD3>,
> +				 <MT2701_PIN_264_G2_TXD2__FUNC_G2_TXD2>,
> +				 <MT2701_PIN_265_G2_TXD1__FUNC_G2_TXD1>,
> +				 <MT2701_PIN_266_G2_TXD0__FUNC_G2_TXD0>,
> +				 <MT2701_PIN_267_G2_TXC__FUNC_G2_TXC>,
> +				 <MT2701_PIN_268_G2_RXC__FUNC_G2_RXC>,
> +				 <MT2701_PIN_269_G2_RXD0__FUNC_G2_RXD0>,
> +				 <MT2701_PIN_270_G2_RXD1__FUNC_G2_RXD1>,
> +				 <MT2701_PIN_271_G2_RXD2__FUNC_G2_RXD2>,
> +				 <MT2701_PIN_272_G2_RXD3__FUNC_G2_RXD3>,
> +				 <MT2701_PIN_274_G2_RXDV__FUNC_G2_RXDV>;
> +		};
> +	};
> +
>  	spi_pins_a: spi0@0 {
>  		pins_spi {
>  			pinmux = <MT2701_PIN_53_SPI0_CSN__FUNC_SPI0_CS>,
> diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
> index 4f52019..3847f70 100644
> --- a/arch/arm/boot/dts/mt2701.dtsi
> +++ b/arch/arm/boot/dts/mt2701.dtsi
> @@ -381,6 +381,28 @@
>  		#clock-cells = <1>;
>  	};
>  
> +	eth: ethernet@1b100000 {
> +		compatible = "mediatek,mt7623-eth";
> +		reg = <0 0x1b100000 0 0x20000>;
> +		interrupts = <GIC_SPI 200 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 199 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 198 IRQ_TYPE_LEVEL_LOW>;
> +		clocks = <&topckgen CLK_TOP_ETHIF_SEL>,
> +			 <&apmixedsys CLK_APMIXED_TRGPLL>,
> +			 <&ethsys CLK_ETHSYS_ESW>,
> +			 <&ethsys CLK_ETHSYS_GP2>,
> +			 <&ethsys CLK_ETHSYS_GP1>;
> +		clock-names = "ethif", "trgpll", "esw", "gp2", "gp1";
> +		power-domains = <&scpsys MT2701_POWER_DOMAIN_ETH>;
> +		resets = <&watchdog MT2701_TOPRGU_ETHDMA_RST>;

are you sure this is correct ? on mt7623 we point the reset at ethsys
and not the watchdog.

	John

> +		reset-names = "eth";
> +		mediatek,ethsys = <&ethsys>;
> +		mediatek,pctl = <&syscfg_pctl_a>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		status = "disabled";
> +	};
> +
>  	bdpsys: syscon@1c000000 {
>  		compatible = "mediatek,mt2701-bdpsys", "syscon";
>  		reg = <0 0x1c000000 0 0x1000>;
> 
--
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] iio: max5481: Add support for Maxim digital potentiometers
From: Jonathan Cameron @ 2017-01-14 10:38 UTC (permalink / raw)
  To: Slawomir Stepien, linux-iio-u79uwXL29TY76Z2rM5mHXA
  Cc: cristina.moraru09-Re5JQEeQqe8AvxtiuMwx3w,
	matthew.weber-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR,
	maury.anderson-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Rutland,
	Rob Herring
In-Reply-To: <20170108185733.GA8511-SwUeJysX96B82hYKe6nXyg@public.gmane.org>

On 08/01/17 18:57, Slawomir Stepien wrote:
> From: Matt Weber <matthew.weber-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR@public.gmane.org>
> 
> Add implementation for Maxim Integrated 5481, 5482, 5483,
> and 5484 digital potentiometer devices.
> 
> Datasheet:
> http://datasheets.maximintegrated.com/en/ds/MAX5481-MAX5484.pdf
> 
> Signed-off-by: Maury Anderson <maury.anderson-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR@public.gmane.org>
> Signed-off-by: Matthew Weber <matthew.weber-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR@public.gmane.org>
> Signed-off-by: Slawomir Stepien <sst-IjDXvh/HVVUAvxtiuMwx3w@public.gmane.org>
As this has device tree bindings it should have gone to linux-devicetree,
Rob and Mark (maintainers of bindings).

Spi buffers for spi_write need to be cacheline aligned.  See below
for roughly why.

Jonathan
> ---
> 
> This is my resubmission of this patch after original authors decided not to
> pursuit it inclusion into kernel.
> 
> Tested using signal analyzer.
> 
> Changes since v1:
> * removed not needed '``' and 'c' chars
> * includes are now sorted
> * added coma to last item in enum max5481_variant
> * removed maxpos from struct max5481_cfg
> * max5481_CHANNEL is no MAX5481_CHANNEL and it does not have 'ch' argument
> * max5481_write_cmd is now based around switch
> * removed not needed cast in max5481_write_cmd
> * wpier state is saved after iio_device_unregister
> * changed names in spi_device_id and acpi_device_id to be equal to names in of_device_id
> 
> ---
>  .../bindings/iio/potentiometer/max5481.txt         |  23 +++
>  drivers/iio/potentiometer/Kconfig                  |  11 ++
>  drivers/iio/potentiometer/Makefile                 |   1 +
>  drivers/iio/potentiometer/max5481.c                | 215 +++++++++++++++++++++
>  4 files changed, 250 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/max5481.txt
>  create mode 100644 drivers/iio/potentiometer/max5481.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/max5481.txt b/Documentation/devicetree/bindings/iio/potentiometer/max5481.txt
> new file mode 100644
> index 000000000000..6a91b106e076
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/potentiometer/max5481.txt
> @@ -0,0 +1,23 @@
> +* Maxim Linear-Taper Digital Potentiometer MAX5481-MAX5484
> +
> +The node for this driver must be a child node of a SPI controller, hence
> +all mandatory properties described in
> +
> +        Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +must be specified.
> +
> +Required properties:
> +	- compatible:  	Must be one of the following, depending on the
> +			model:
> +			"maxim,max5481"
> +			"maxim,max5482"
> +			"maxim,max5483"
> +			"maxim,max5484"
> +
> +Example:
> +max548x: max548x@0 {
> +	compatible = "maxim,max5482";
> +	spi-max-frequency = <7000000>;
> +	reg = <0>; /* chip-select */
> +};
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index 2e9da1cf3297..8bf282510be6 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -15,6 +15,17 @@ config DS1803
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ds1803.
>  
> +config MAX5481
> +        tristate "Maxim MAX5481-MAX5484 Digital Potentiometer driver"
> +        depends on SPI
> +        help
> +          Say yes here to build support for the Maxim
> +          MAX5481, MAX5482, MAX5483, MAX5484 digital potentiometer
> +          chips.
> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called max5481.
> +
>  config MAX5487
>          tristate "Maxim MAX5487/MAX5488/MAX5489 Digital Potentiometer driver"
>          depends on SPI
> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> index 8adb58f38c0b..2260d40e0936 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -4,6 +4,7 @@
>  
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_DS1803) += ds1803.o
> +obj-$(CONFIG_MAX5481) += max5481.o
>  obj-$(CONFIG_MAX5487) += max5487.o
>  obj-$(CONFIG_MCP4131) += mcp4131.o
>  obj-$(CONFIG_MCP4531) += mcp4531.o
> diff --git a/drivers/iio/potentiometer/max5481.c b/drivers/iio/potentiometer/max5481.c
> new file mode 100644
> index 000000000000..59ced74b0252
> --- /dev/null
> +++ b/drivers/iio/potentiometer/max5481.c
> @@ -0,0 +1,215 @@
> +/*
> + * Maxim Integrated MAX5481-MAX5484 digital potentiometer driver
> + * Copyright 2016 Rockwell Collins
> + *
> + * Datasheet:
> + * http://datasheets.maximintegrated.com/en/ds/MAX5481-MAX5484.pdf
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the gnu general public license version 2 as
> + * published by the free software foundation.
> + *
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +
> +/* write wiper reg */
> +#define MAX5481_WRITE_WIPER (0 << 4)
> +/* copy wiper reg to NV reg */
> +#define MAX5481_COPY_AB_TO_NV (2 << 4)
> +/* copy NV reg to wiper reg */
> +#define MAX5481_COPY_NV_TO_AB (3 << 4)
> +
> +#define MAX5481_MAX_POS    1023
> +
> +enum max5481_variant {
> +	max5481,
> +	max5482,
> +	max5483,
> +	max5484,
> +};
> +
> +struct max5481_cfg {
> +	int kohms;
> +};
> +
> +static const struct max5481_cfg max5481_cfg[] = {
> +	[max5481] = { .kohms =  10, },
> +	[max5482] = { .kohms =  50, },
> +	[max5483] = { .kohms =  10, },
> +	[max5484] = { .kohms =  50, },
> +};
> +
> +struct max5481_data {
> +	struct spi_device *spi;
> +	const struct max5481_cfg *cfg;
> +};
> +
> +#define MAX5481_CHANNEL {					\
> +	.type = IIO_RESISTANCE,					\
> +	.indexed = 1,						\
> +	.output = 1,						\
> +	.channel = 0,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +}
> +
> +static const struct iio_chan_spec max5481_channels[] = {
> +	MAX5481_CHANNEL,
> +};
> +
> +static int max5481_write_cmd(struct spi_device *spi, u8 cmd, u16 val)
> +{
> +	/* SPI Format from MAX5481-MAX5484 (19-3708; Rev 5; 4/10) pg 15 */
> +	u8 msg[3];
It's clearly one of those days - same issue in two drivers in a row. :(
Still I can refine me response ;)

There are requirements for buffers passed directly to spi_read / spi_write.
They get passed to spi_sync which calls into the spi master drivers.
SPI master drivers are explicitly allowed to directly use this buffer
in dma. On most modern platforms it is fine to do DMA from any location...

However, cacheline corruption comes in here. There is no guarantee that
the SPI controller won't write back to this address, as it it will be
bypassing the processor whilst doing this, that can result in a difference
in other parts of the cacheline between what is in the cache and what is
in main memory.  At the end of the dma transfer any changes elsewhere in
the cacheline can be wiped out as result. (or something like that ;)

Anyhow, two solutions.  Either allocate the memory in it's own cacheline
which will naturally happen if you allocate on the heap using kmalloc
or use the fact we carefully align the iio_priv memory to be cacheline
aligned. This means that if you stick a __cacheline_aligned buffer at the
end of your iio_priv structure it was also be in it's own cacheline.

Not doing this is the source of really hard to track down bugs!
> +
> +	msg[0] = cmd;
> +
> +	switch (cmd) {
> +	case MAX5481_WRITE_WIPER:
> +		msg[1] = val >> 2;
> +		msg[2] = (val & 0x3) << 6;
> +		return spi_write(spi, msg, ARRAY_SIZE(msg));
> +
> +	case MAX5481_COPY_AB_TO_NV:
> +	case MAX5481_COPY_NV_TO_AB:
> +		return spi_write(spi, msg, sizeof(u8));
> +
> +	default:
> +		return -EIO;
> +	}
> +}
> +
> +static int max5481_read_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int *val, int *val2, long mask)
> +{
> +	struct max5481_data *data = iio_priv(indio_dev);
> +
> +	if (mask != IIO_CHAN_INFO_SCALE)
> +		return -EINVAL;
> +
> +	*val = 1000 * data->cfg->kohms;
> +	*val2 = MAX5481_MAX_POS;
> +
> +	return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int max5481_write_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int val, int val2, long mask)
> +{
> +	struct max5481_data *data = iio_priv(indio_dev);
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	if (val < 0 || val > MAX5481_MAX_POS)
> +		return -EINVAL;
> +
> +	return max5481_write_cmd(data->spi, MAX5481_WRITE_WIPER, val);
> +}
> +
> +static const struct iio_info max5481_info = {
> +	.read_raw = max5481_read_raw,
> +	.write_raw = max5481_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int max5481_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct max5481_data *data;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&spi->dev, indio_dev);
> +	data = iio_priv(indio_dev);
> +
> +	data->spi = spi;
> +	data->cfg = &max5481_cfg[id->driver_data];
> +
> +	indio_dev->name = id->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	/* variant specific configuration */
> +	indio_dev->info = &max5481_info;
> +	indio_dev->channels = max5481_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max5481_channels);
> +
> +	/* restore wiper from NV */
> +	ret = max5481_write_cmd(data->spi, MAX5481_COPY_NV_TO_AB, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	return iio_device_register(indio_dev);
> +}
> +
> +static int max5481_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	/* save wiper reg to NV reg */
> +	return max5481_write_cmd(spi, MAX5481_COPY_AB_TO_NV, 0);
> +}
> +
> +static const struct spi_device_id max5481_id_table[] = {
> +	{ "max5481", max5481 },
> +	{ "max5482", max5482 },
> +	{ "max5483", max5483 },
> +	{ "max5484", max5484 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, max5481_id_table);
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id max5481_match[] = {
> +	{ .compatible = "maxim,max5481", .data = &max5481_cfg[max5481] },
> +	{ .compatible = "maxim,max5482", .data = &max5481_cfg[max5482] },
> +	{ .compatible = "maxim,max5483", .data = &max5481_cfg[max5483] },
> +	{ .compatible = "maxim,max5484", .data = &max5481_cfg[max5484] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max5481_match);
> +#endif
> +
> +#if defined(CONFIG_ACPI)
> +static const struct acpi_device_id max5481_acpi_match[] = {
> +	{ "max5481", max5481 },
> +	{ "max5482", max5482 },
> +	{ "max5483", max5483 },
> +	{ "max5484", max5484 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, max5481_acpi_match);
> +#endif
> +
> +static struct spi_driver max5481_driver = {
> +	.driver = {
> +		.name  = "max5481",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(max5481_match),
> +		.acpi_match_table = ACPI_PTR(max5481_acpi_match),
> +	},
> +	.probe = max5481_probe,
> +	.remove = max5481_remove,
> +	.id_table = max5481_id_table,
> +};
> +
> +module_spi_driver(max5481_driver);
> +
> +MODULE_AUTHOR("Maury Anderson <maury.anderson-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR@public.gmane.org>");
> +MODULE_DESCRIPTION("max5481 SPI driver");
> +MODULE_LICENSE("GPL v2");
> 

^ permalink raw reply

* Re: [PATCH v4] iio:temperature: Add support for TI TMP007 sensor
From: Jonathan Cameron @ 2017-01-14 10:48 UTC (permalink / raw)
  To: Rob Herring, Manivannan Sadhasivam
  Cc: pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170110053603.n6ihrg3r6z4244g5@rob-hp-laptop>

On 10/01/17 05:36, Rob Herring wrote:
> On Mon, Jan 09, 2017 at 08:25:26AM +0530, Manivannan Sadhasivam wrote:
>> This patch adds support for TI TMP007 - 16 bit IR thermopile sensor with integrated Math engine.
>> Sensor takes care of calculating the object temperature with the help of calibrated constants stored in non-volatile memory,
>> thereby reducing the calculation overhead.
>>
>> Signed-off-by: Manivannan Sadhasivam <manivannanece23-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> Changes since v3:
>>
>> 1. Removed caching of status register
>> 2. Modified comments for data validity
>> 3. Added possible address combinations in dTree bindings file
>>
>>  .../devicetree/bindings/iio/temperature/tmp007.txt |  27 ++
> 
> This could be documented under trivial-devices.txt, but doesn't hurt to 
> have it here.
> 
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Thanks as ever Rob - I don't say that often enough!

Anyhow, nice little driver. Applied to the togreg branch of iio.git and
pushed out as testing for the autobuilders to see if they can find anything
we missed.

Was a bit messy going in with some fuzz, but superficially looks fine.

Thanks,

Jonathan
> 
>>  drivers/iio/temperature/Kconfig                    |  10 +
>>  drivers/iio/temperature/Makefile                   |   1 +
>>  drivers/iio/temperature/tmp007.c                   | 345 +++++++++++++++++++++
>>  4 files changed, 383 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/tmp007.txt
>>  create mode 100644 drivers/iio/temperature/tmp007.c
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" 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] iio: max5481: Add support for Maxim digital potentiometers
From: Slawomir Stepien @ 2017-01-14 11:42 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	cristina.moraru09-Re5JQEeQqe8AvxtiuMwx3w,
	matthew.weber-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR,
	maury.anderson-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Rutland,
	Rob Herring
In-Reply-To: <ee973aa1-c881-6bc8-c61a-9f36b8598f5f-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

On Jan 14, 2017 10:38, Jonathan Cameron wrote:
> As this has device tree bindings it should have gone to linux-devicetree,
> Rob and Mark (maintainers of bindings).

I will add them to CC on the new version.

> Spi buffers for spi_write need to be cacheline aligned.  See below
> for roughly why.

I can recall that from my previous submissions... I made the same mistake.
Thank you once again for explaining that. I hope this will imprint in my mind
from now on.

Thank you!

> Jonathan
> > ---
> > +static int max5481_write_cmd(struct spi_device *spi, u8 cmd, u16 val)
> > +{
> > +	/* SPI Format from MAX5481-MAX5484 (19-3708; Rev 5; 4/10) pg 15 */
> > +	u8 msg[3];
> It's clearly one of those days - same issue in two drivers in a row. :(
> Still I can refine me response ;)
> 
> There are requirements for buffers passed directly to spi_read / spi_write.
> They get passed to spi_sync which calls into the spi master drivers.
> SPI master drivers are explicitly allowed to directly use this buffer
> in dma. On most modern platforms it is fine to do DMA from any location...
> 
> However, cacheline corruption comes in here. There is no guarantee that
> the SPI controller won't write back to this address, as it it will be
> bypassing the processor whilst doing this, that can result in a difference
> in other parts of the cacheline between what is in the cache and what is
> in main memory.  At the end of the dma transfer any changes elsewhere in
> the cacheline can be wiped out as result. (or something like that ;)
> 
> Anyhow, two solutions.  Either allocate the memory in it's own cacheline
> which will naturally happen if you allocate on the heap using kmalloc
> or use the fact we carefully align the iio_priv memory to be cacheline
> aligned. This means that if you stick a __cacheline_aligned buffer at the
> end of your iio_priv structure it was also be in it's own cacheline.
> 
> Not doing this is the source of really hard to track down bugs!
> > +
> > +	msg[0] = cmd;
> > +
> > +	switch (cmd) {
> > +	case MAX5481_WRITE_WIPER:
> > +		msg[1] = val >> 2;
> > +		msg[2] = (val & 0x3) << 6;
> > +		return spi_write(spi, msg, ARRAY_SIZE(msg));
> > +
> > +	case MAX5481_COPY_AB_TO_NV:
> > +	case MAX5481_COPY_NV_TO_AB:
> > +		return spi_write(spi, msg, sizeof(u8));
> > +
> > +	default:
> > +		return -EIO;
> > +	}
> > +}

-- 
Slawomir Stepien

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox