* Re: [PATCH net-next 1/2] dt-bindings: net: dp83822: Add TI dp83822 phy
From: Dan Murphy @ 2020-05-14 19:38 UTC (permalink / raw)
To: Andrew Lunn
Cc: f.fainelli, hkallweit1, davem, robh, netdev, linux-kernel,
devicetree, Rob Herring
In-Reply-To: <20200514183912.GW499265@lunn.ch>
Andrew
On 5/14/20 1:39 PM, Andrew Lunn wrote:
> On Thu, May 14, 2020 at 12:30:54PM -0500, Dan Murphy wrote:
>> Add a dt binding for the TI dp83822 ethernet phy device.
>>
>> CC: Rob Herring <robh+dt@kernel.org>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>> .../devicetree/bindings/net/ti,dp83822.yaml | 49 +++++++++++++++++++
>> 1 file changed, 49 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> new file mode 100644
>> index 000000000000..60afd43ad3b6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> @@ -0,0 +1,49 @@
>> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
>> +# Copyright (C) 2020 Texas Instruments Incorporated
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/net/ti,dp83822.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: TI DP83822 ethernet PHY
>> +
>> +allOf:
>> + - $ref: "ethernet-controller.yaml#"
>> +
>> +maintainers:
>> + - Dan Murphy <dmurphy@ti.com>
>> +
>> +description: |
>> + The DP83822 is a low-power, single-port, 10/100 Mbps Ethernet PHY. It
>> + provides all of the physical layer functions needed to transmit and receive
>> + data over standard, twisted-pair cables or to connect to an external,
>> + fiber-optic transceiver. Additionally, the DP83822 provides flexibility to
>> + connect to a MAC through a standard MII, RMII, or RGMII interface
> Hi Dan
>
> You say 10/100 Mbps Ethernet PHY, but then list RGMII?
Copied from the data sheet.
>
>> +
>> + Specifications about the charger can be found at:
>> + http://www.ti.com/lit/ds/symlink/dp83822i.pdf
>> +
>> +properties:
>> + reg:
>> + maxItems: 1
>> +
>> + ti,signal-polarity-low:
>> + type: boolean
>> + description: |
>> + DP83822 PHY in Fiber mode only.
>> + Sets the DP83822 to detect a link drop condition when the signal goes
>> + high. If not set then link drop will occur when the signal goes low.
> Are we talking about the LOS line from the SFP cage? In the SFF/SFP
> binding we have:
>
> - los-gpios : GPIO phandle and a specifier of the Receiver Loss of Signal
> Indication input gpio signal, active (signal lost) high
>
> It would be nice to have a consistent naming.
> Is it required the LOS signal is connected to the PHY? Russell King
> has some patches which allows the Marvell PHY to be used as a media
> converter. In that setting, i think the SFP signals are connected to
> GPIOs not the PHY. The SFP core can then control the transmit disable,
> module insertion detection etc. So i'm wondering if you need a
> property to indicate the LOS is not connected to the PHY?
The LED_1 pin can be strapped to be an input to the chip for signal loss
detection. This is an optional feature of the PHY.
This property defines the polarity for the 822 LED_1/GPIO input pin.
The LOS is not required to be connected to the PHY. If the preferred
method is to use the SFP framework and Processor GPIOs then I can remove
this from the patch set.
And if a user would like to use the feature then they can add it.
Dan
>
> Andrew
^ permalink raw reply
* Re: [PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx
From: Dmitry Osipenko @ 2020-05-14 19:34 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Rob Herring, David Heidelberg, Jonghwa Lee, Chanwoo Choi,
Myungjoo Ham, Sumit Semwal, John Stultz, Vinay Simha BN,
Mika Westerberg, ramakrishna.pallala, open list:THERMAL,
linux-kernel@vger.kernel.org, devicetree
In-Reply-To: <20200509011406.hs7nj3g7f5pzetxp@earth.universe>
09.05.2020 04:14, Sebastian Reichel пишет:
> Hi,
>
> On Wed, Apr 15, 2020 at 06:30:02PM +0300, Dmitry Osipenko wrote:
>> 15.04.2020 17:27, Rob Herring пишет:
>>> On Fri, Apr 10, 2020 at 2:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> 10.04.2020 19:49, Rob Herring пишет:
>>>> ...
>>>>>> + summit,max-chg-curr:
>>>>>> + description: Maximum current for charging (in uA)
>>>>>> + allOf:
>>>>>> + - $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +
>>>>>> + summit,max-chg-volt:
>>>>>> + description: Maximum voltage for charging (in uV)
>>>>>> + allOf:
>>>>>> + - $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> + minimum: 3500000
>>>>>> + maximum: 4500000
>>>>>> +
>>>>>> + summit,pre-chg-curr:
>>>>>> + description: Pre-charging current for charging (in uA)
>>>>>> + allOf:
>>>>>> + - $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +
>>>>>> + summit,term-curr:
>>>>>> + description: Charging cycle termination current (in uA)
>>>>>> + allOf:
>>>>>> + - $ref: /schemas/types.yaml#/definitions/uint32
>>>> ...
>>>>> These are all properties of the battery attached and we have standard
>>>>> properties for some/all of these.
>>>>
>>>> Looks like only four properties seem to be matching the properties of
>>>> the battery.txt binding.
>>>>
>>>> Are you suggesting that these matching properties should be renamed
>>>> after the properties in battery.txt?
>>>
>>> Yes, and that there should be a battery node.
>>
>> Usually, it's a battery that has a phandle to the power-supply. Isn't it?
>
> There are two things: The infrastructure described by
> Documentation/devicetree/bindings/power/supply/power-supply.yaml is
> used for telling the operating system, that a battery is charged
> by some charger. This is done by adding a power-supplies = <&phandle>
> in the battery fuel gauge node referencing the charger and probably
> what you mean here.
>
> Then we have the infrastructure described by
> Documentation/devicetree/bindings/power/supply/battery.txt, which
> provides data about the battery cell. In an ideal world we would
> have only smart batteries providing this data, but we don't live
> in such a world. So what we currently have is a binding looking
> like this:
>
> bat: dumb-battery {
> compatible = "simple-battery";
>
> // data about battery cell(s)
> };
>
> fuel-gauge {
> // fuel-gauge specific data
>
> supplies = <&charger>;
> monitored-battery = <&bat>;
> };
>
> charger: charger {
> // charger specific data
>
> monitored-battery = <&bat>;
> };
>
> In an ideal world, charger should possibly reference fuel-gauge
> node, which could provide combined data. Right now we do not have
> the infrastructure for that, so it needs to directly reference
> the simple-battery node.
>
>>> Possibly you should add
>>> new properties battery.txt. It's curious that different properties are
>>> needed.
>>
>> I guess it should be possible to make all these properties generic.
>>
>> Sebastian, will you be okay if we will add all the required properties
>> to the power_supply_core?
>
> Extending battery.txt is possible when something is missing. As Rob
> mentioned quite a few are already described, though:
>
> summit,max-chg-curr => constant-charge-current-max-microamp
> summit,max-chg-volt => constant-charge-voltage-max-microvolt
> summit,pre-chg-curr => precharge-current-microamp
> summit,term-curr => charge-term-current-microamp
>
> I think at least the battery temperature limits are something, that
> should be added to the generic code.
>
>>> Ultimately, for a given battery technology I would expect
>>> there's a fixed set of properties needed to describe how to charge
>>> them.
>>
>> Please notice that the charger doesn't "only charge" the battery,
>> usually it also supplies power to the whole device.
>>
>> For example, when battery is fully-charged and charger is connected to
>> the power source (USB or mains), then battery may not draw any current
>> at all.
>
> It is also a question of how good the charging process should be.
> Technically I can charge a single cell Li-ion battery without
> knowing much, but it can reduce battery life and/or be very slow.
> It might even be dangerous, if charging is done at high
> temperatures. Also some of the properties in the battery binding
> are not about charging, but about gauging. Some devices basically
> have only options to measure voltage and voltage drop over a
> resistor and everything else must be done by the operating system.
>
>>> Perhaps some of these properties can just be derived from other
>>> properties and folks are just picking what a specific charger wants.
>>
>> Could be so, but I don't know for sure.
>
> I don't think we have things, that can be derived with a reasonable
> amount of effort in the existing simple-battery binding, except for
> energy-full-design-microwatt-hours & charge-full-design-microamp-hours.
>
>> Even if some properties could be derived from the others, it won't hurt
>> if we will specify everything explicitly in the device-tree.
>>
>>> Unfortunately, we have just a mess of stuff made up for each charger
>>> out there. I don't have the time nor the experience in this area to do
>>> much more than say do better.
>>
>> I don't think it's a mess in the kernel. For example, it's common that
>> embedded controllers are exposed to the system as "just a battery",
>> while in fact it's a combined charger + battery controller and the
>> charger parameters just couldn't be changed by SW.
>
> A good EC driver exposes a charger and a battery device, so that
> userspace can easily identify if a charger is connected.
>
>> In a case of the Nexus 7 devices, the battery controller and charger
>> controller are two separate entities in the system. The battery
>> controller (bq27541) only monitors status of the battery (charge level,
>> temperature and etc).
Hello Sebastian,
Thank you very much for the comments! We'll prepare the v2.
^ permalink raw reply
* Re: [PATCH v2 1/4] dt-bindings: clk: Add Baikal-T1 CCU PLLs binding
From: Rob Herring @ 2020-05-14 19:13 UTC (permalink / raw)
To: Serge Semin
Cc: Thomas Bogendoerfer, Stephen Boyd, Michael Turquette, Serge Semin,
Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann,
linux-mips, linux-clk, devicetree, linux-kernel
In-Reply-To: <20200506222300.30895-2-Sergey.Semin@baikalelectronics.ru>
On Thu, May 07, 2020 at 01:22:57AM +0300, Serge Semin wrote:
> Baikal-T1 Clocks Control Unit is responsible for transformation of a
> signal coming from an external oscillator into clocks of various
> frequencies to propagate them then to the corresponding clocks
> consumers (either individual IP-blocks or clock domains). In order
> to create a set of high-frequency clocks the external signal is
> firstly handled by the embedded into CCU PLLs. So the corresponding
> dts-node is just a normal clock-provider node with standard set of
> properties. Note as being part of the Baikal-T1 System Controller its
> DT node is supposed to be a child the system controller node.
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-mips@vger.kernel.org
>
> ---
>
> Changelog v2:
> - Rearrange the SoBs.
> - Discard comments in the bindings file header.
> - Add dual GPL/BSD license.
> - Add spaces around the ASCII-graphics in the binding description.
> - Remove reference to Documentation/devicetree/bindings/clock/clock-bindings.txt
> file.
> - Discard redundant object check against "/schemas/clock/clock.yaml#" schema.
> - Discard redundant descriptions of the "#clock-cells" property.
> - Remove "reg" property since from now the clock DT node is supposed to be
> a child of the syscon-compatible system controller node.
> - Remove "clock-output-names" property support.
> - Replace "additionalProperties: false" with "unevaluatedProperties: false".
> - Lowercase the nodes name in the examples.
> - Use "clock-controller" node name suffix in the examples.
> - Remove unnecessary comments in the clocks dt-bindings header file.
> ---
> .../bindings/clock/baikal,bt1-ccu-pll.yaml | 127 ++++++++++++++++++
> include/dt-bindings/clock/bt1-ccu.h | 16 +++
> 2 files changed, 143 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/baikal,bt1-ccu-pll.yaml
> create mode 100644 include/dt-bindings/clock/bt1-ccu.h
>
> diff --git a/Documentation/devicetree/bindings/clock/baikal,bt1-ccu-pll.yaml b/Documentation/devicetree/bindings/clock/baikal,bt1-ccu-pll.yaml
> new file mode 100644
> index 000000000000..571181758ef2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/baikal,bt1-ccu-pll.yaml
> @@ -0,0 +1,127 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/baikal,bt1-ccu-pll.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Baikal-T1 Clock Control Unit PLL
> +
> +maintainers:
> + - Serge Semin <fancer.lancer@gmail.com>
> +
> +description: |
> + Clocks Control Unit is the core of Baikal-T1 SoC System Controller
> + responsible for the chip subsystems clocking and resetting. The CCU is
> + connected with an external fixed rate oscillator, which signal is transformed
> + into clocks of various frequencies and then propagated to either individual
> + IP-blocks or to groups of blocks (clock domains). The transformation is done
> + by means of PLLs and gateable/non-gateable dividers embedded into the CCU.
> + It's logically divided into the next components:
> + 1) External oscillator (normally XTAL's 25 MHz crystal oscillator, but
> + in general can provide any frequency supported by the CCU PLLs).
> + 2) PLLs clocks generators (PLLs) - described in this binding file.
> + 3) AXI-bus clock dividers (AXI).
> + 4) System devices reference clock dividers (SYS).
> + which are connected with each other as shown on the next figure:
> +
> + +---------------+
> + | Baikal-T1 CCU |
> + | +----+------|- MIPS P5600 cores
> + | +-|PLLs|------|- DDR controller
> + | | +----+ |
> + +----+ | | | | |
> + |XTAL|--|-+ | | +---+-|
> + +----+ | | | +-|AXI|-|- AXI-bus
> + | | | +---+-|
> + | | | |
> + | | +----+---+-|- APB-bus
> + | +-------|SYS|-|- Low-speed Devices
> + | +---+-|- High-speed Devices
> + +---------------+
Are you going to just duplicate all this for each sub-block?
> +
> + Each CCU sub-block is represented as a separate dts-node and has an
> + individual driver to be bound with.
> +
> + In order to create signals of wide range frequencies the external oscillator
> + output is primarily connected to a set of CCU PLLs. There are five PLLs
> + to create a clock for the MIPS P5600 cores, the embedded DDR controller,
> + SATA, Ethernet and PCIe domains. The last three domains though named by the
> + biggest system interfaces in fact include nearly all of the rest SoC
> + peripherals. Each of the PLLs is based on True Circuits TSMC CLN28HPM core
> + with an interface wrapper (so called safe PLL' clocks switcher) to simplify
> + the PLL configuration procedure. The PLLs work as depicted on the next
> + diagram:
> +
> + +--------------------------+
> + | |
> + +-->+---+ +---+ +---+ | +---+ 0|\
> + CLKF--->|/NF|--->|PFD|...|VCO|-+->|/OD|--->| |
> + +---+ +->+---+ +---+ /->+---+ | |--->CLKOUT
> + CLKOD---------C----------------+ 1| |
> + +--------C--------------------------->|/
> + | | ^
> + Rclk-+->+---+ | |
> + CLKR--->|/NR|-+ |
> + +---+ |
> + BYPASS--------------------------------------+
> + BWADJ--->
> +
> + where Rclk is the reference clock coming from XTAL, NR - reference clock
> + divider, NF - PLL clock multiplier, OD - VCO output clock divider, CLKOUT -
> + output clock, BWADJ is the PLL bandwidth adjustment parameter. At this moment
> + the binding supports the PLL dividers configuration in accordance with a
> + requested rate, while bypassing and bandwidth adjustment settings can be
> + added in future if it gets to be necessary.
> +
> + The PLLs CLKOUT is then either directly connected with the corresponding
> + clocks consumer (like P5600 cores or DDR controller) or passed over a CCU
> + divider to create a signal required for the clock domain.
> +
> + The CCU PLL dts-node uses the common clock bindings with no custom
> + parameters. The list of exported clocks can be found in
> + 'include/dt-bindings/clock/bt1-ccu.h'. Since CCU PLL is a part of the
> + Baikal-T1 SoC System Controller its DT node is supposed to be a child of
> + later one.
The schema can and should express this. IOW, either move this into the
system controller schema or reference this ($ref) from it.
> +
> +properties:
> + compatible:
> + const: baikal,bt1-ccu-pll
> +
> + "#clock-cells":
> + const: 1
> +
> + clocks:
> + description: External reference clock
> + maxItems: 1
> +
> + clock-names:
> + const: ref_clk
> +
> +unevaluatedProperties: false
> +
> +required:
> + - compatible
> + - "#clock-cells"
> + - clocks
> + - clock-names
> +
> +examples:
> + # Clock Control Unit PLL node:
> + - |
> + clock-controller-pll {
> + compatible = "baikal,bt1-ccu-pll";
> + #clock-cells = <1>;
> +
> + clocks = <&clk25m>;
> + clock-names = "ref_clk";
If there's a register range within the system controller for the pll,
then add 'reg' even if Linux doesn't use it.
> + };
> + # Required external oscillator:
> + - |
> + clk25m: clock-oscillator-25m {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <25000000>;
> + clock-output-names = "clk25m";
> + };
> +...
> diff --git a/include/dt-bindings/clock/bt1-ccu.h b/include/dt-bindings/clock/bt1-ccu.h
> new file mode 100644
> index 000000000000..931a4bea67c0
> --- /dev/null
> +++ b/include/dt-bindings/clock/bt1-ccu.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> + *
> + * Baikal-T1 CCU clock indices
> + */
> +#ifndef __DT_BINDINGS_CLOCK_BT1_CCU_H
> +#define __DT_BINDINGS_CLOCK_BT1_CCU_H
> +
> +#define CCU_CPU_PLL 0
> +#define CCU_SATA_PLL 1
> +#define CCU_DDR_PLL 2
> +#define CCU_PCIE_PLL 3
> +#define CCU_ETH_PLL 4
> +
> +#endif /* __DT_BINDINGS_CLOCK_BT1_CCU_H */
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: phy: DP83822: Add ability to advertise Fiber connection
From: Andrew Lunn @ 2020-05-14 18:52 UTC (permalink / raw)
To: Dan Murphy
Cc: f.fainelli, hkallweit1, davem, robh, netdev, linux-kernel,
devicetree
In-Reply-To: <20200514173055.15013-3-dmurphy@ti.com>
> +static int dp83822_config_init(struct phy_device *phydev)
> +{
> + struct dp83822_private *dp83822 = phydev->priv;
> + int err = 0;
> +
> + if (dp83822->fx_enabled) {
> + linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
> + phydev->supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
> + phydev->advertising);
> +
> + /* Auto negotiation is not available in fiber mode */
> + phydev->autoneg = AUTONEG_DISABLE;
> + phydev->speed = SPEED_100;
> + phydev->duplex = DUPLEX_FULL;
Hi Dan
This is normally determined by reading the ability registers,
genphy_read_abilities(). When strapped to fibre mode, does it still
indicate all the usual copper capabilities, which it can not actually
do?
Andrew
^ permalink raw reply
* Re: [PATCH v3 3/7] dt-bindings: interrupt-controller: Convert mti,gic to DT schema
From: Rob Herring @ 2020-05-14 19:06 UTC (permalink / raw)
To: Serge Semin
Cc: Rob Herring, Ralf Baechle, Serge Semin, Arnd Bergmann, linux-mips,
devicetree, Thomas Gleixner, Daniel Lezcano, linux-rtc,
Marc Zyngier, Alexey Malahov, Alexandre Belloni, linux-kernel,
Jason Cooper, Thomas Bogendoerfer, Alessandro Zummo, Paul Burton
In-Reply-To: <20200506214107.25956-4-Sergey.Semin@baikalelectronics.ru>
On Thu, 7 May 2020 00:41:03 +0300, Serge Semin wrote:
> Modern device tree bindings are supposed to be created as YAML-files
> in accordance with DT schema. This commit replaces MIPS GIC legacy bare
> text binding with YAML file. As before the binding file states that the
> corresponding dts node is supposed to be compatible with MIPS Global
> Interrupt Controller indicated by the "mti,gic" compatible string and
> to provide a mandatory interrupt-controller and '#interrupt-cells'
> properties. There might be optional registers memory range,
> "mti,reserved-cpu-vectors" and "mti,reserved-ipi-vectors" properties
> specified.
>
> MIPS GIC also includes a free-running global timer, per-CPU count/compare
> timers, and a watchdog. Since currently the GIC Timer is only supported the
> DT schema expects an IRQ and clock-phandler charged timer sub-node with
> "mti,mips-gic-timer" compatible string.
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-mips@vger.kernel.org
> Cc: linux-rtc@vger.kernel.org
>
> ---
>
> I don't really know who is the corresponding driver maintainer, so I
> added Paul to the maintainers property since he used to be looking for the
> MIPS arch and Thomas looking after it now. Any idea what email should be
> specified there instead?
>
> Changelog v3:
> - Since timer sub-node has no unit-address, the node shouldn't be named
> with one. So alter the MIPS GIC bindings to have a pure "timer"
> sub-node.
> - Discard allOf: [ $ref: /schemas/interrupt-controller.yaml# ].
> - Since it's a conversion patch use GPL-2.0-only SPDX header.
> ---
> .../interrupt-controller/mips-gic.txt | 67 --------
> .../interrupt-controller/mti,gic.yaml | 148 ++++++++++++++++++
> 2 files changed, 148 insertions(+), 67 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mti,gic.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 2/7] dt-bindings: timer: Move snps,dw-apb-timer DT schema from rtc
From: Rob Herring @ 2020-05-14 19:04 UTC (permalink / raw)
To: Serge Semin
Cc: Alessandro Zummo, linux-mips, Daniel Lezcano, Ralf Baechle,
linux-kernel, Thomas Bogendoerfer, linux-rtc, Paul Burton,
Alexey Malahov, Alexandre Belloni, Serge Semin, Arnd Bergmann,
Thomas Gleixner, devicetree, Rob Herring
In-Reply-To: <20200506214107.25956-3-Sergey.Semin@baikalelectronics.ru>
On Thu, 7 May 2020 00:41:02 +0300, Serge Semin wrote:
> This binding file doesn't belong to the rtc seeing it's a pure timer
> with no rtc facilities like days/months/years counting and alarms.
> So move the YAML-file to the Documentation/devicetree/bindings/timer/
> directory.
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-mips@vger.kernel.org
> ---
> .../devicetree/bindings/{rtc => timer}/snps,dw-apb-timer.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> rename Documentation/devicetree/bindings/{rtc => timer}/snps,dw-apb-timer.yaml (96%)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v1.1 4/4] dt-bindings: display: bridge: renesas,lvds: Convert binding to YAML
From: Geert Uytterhoeven @ 2020-05-14 19:02 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Laurent Pinchart,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-Renesas, Rob Herring, DRI Development
In-Reply-To: <20200514151721.GF5955@pendragon.ideasonboard.com>
Hi Laurent,
On Thu, May 14, 2020 at 5:17 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thu, May 14, 2020 at 09:31:53AM +0200, Geert Uytterhoeven wrote:
> > On Thu, May 14, 2020 at 1:29 AM Laurent Pinchart wrote:
> > > Convert the Renesas R-Car LVDS encoder text binding to YAML.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Acked-by: Maxime Ripard <mripard@kernel.org>
> >
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.yaml
> >
> > > +examples:
> > > + - |
> > > + #include <dt-bindings/clock/renesas-cpg-mssr.h>
> > > + #include <dt-bindings/power/r8a7795-sysc.h>
> > > +
> > > + lvds@feb90000 {
> > > + compatible = "renesas,r8a7795-lvds";
> > > + reg = <0 0xfeb90000 0 0x14>;
> >
> > #{address,size}-cells = <1> for examples.
> > Applies to all nodes below, too.
>
> Why ?
See "[PATCH 5/5] dt-bindings: Fix incorrect 'reg' property sizes"
https://lore.kernel.org/linux-devicetree/20200512204543.22090-5-robh@kernel.org/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v5 3/5] remoteproc: qcom: Update PIL relocation info on load
From: kbuild test robot @ 2020-05-14 18:48 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Ohad Ben-Cohen, Rob Herring
Cc: kbuild-all, linux-arm-msm, linux-remoteproc, devicetree,
linux-kernel
In-Reply-To: <20200513055641.1413100-4-bjorn.andersson@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]
Hi Bjorn,
I love your patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v5.7-rc5 next-20200514]
[cannot apply to agross-msm/qcom/for-next remoteproc/for-next rpmsg/for-next hwspinlock/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Bjorn-Andersson/remoteproc-qcom-PIL-info-support/20200514-161851
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/remoteproc/qcom_q6v5_wcss.c: In function 'q6v5_wcss_load':
>> drivers/remoteproc/qcom_q6v5_wcss.c:433:2: error: implicit declaration of function 'qcom_pil_info_store' [-Werror=implicit-function-declaration]
433 | qcom_pil_info_store("wcnss", wcss->mem_reloc, wcss->mem_size);
| ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/qcom_pil_info_store +433 drivers/remoteproc/qcom_q6v5_wcss.c
420
421 static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw)
422 {
423 struct q6v5_wcss *wcss = rproc->priv;
424 int ret;
425
426 ret = qcom_mdt_load_no_init(wcss->dev, fw, rproc->firmware,
427 0, wcss->mem_region, wcss->mem_phys,
428 wcss->mem_size, &wcss->mem_reloc);
429 if (ret)
430 return ret;
431
432 /* Failures only affect post mortem debugging, so ignore return value */
> 433 qcom_pil_info_store("wcnss", wcss->mem_reloc, wcss->mem_size);
434
435 return ret;
436 }
437
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 71781 bytes --]
^ permalink raw reply
* Re: [PATCH v2 6/9] dt-bindings: serial: renesas,hscif: Document r8a7742 bindings
From: Rob Herring @ 2020-05-14 18:58 UTC (permalink / raw)
To: Lad Prabhakar
Cc: linux-kernel, Greg Kroah-Hartman, Marc Zyngier, linux-mmc,
Geert Uytterhoeven, Jason Cooper, Linus Walleij,
Bartosz Golaszewski, Ulf Hansson, Thomas Gleixner,
linux-renesas-soc, Magnus Damm, devicetree, linux-gpio,
Rob Herring, Prabhakar, linux-serial
In-Reply-To: <1588794695-27852-7-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com>
On Wed, 6 May 2020 20:51:32 +0100, Lad Prabhakar wrote:
> RZ/G1H (R8A7742) SoC also has the R-Car gen2 compatible HSCIF ports,
> so document the SoC specific bindings.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v1->v2:
> * No change
> ---
>
> Documentation/devicetree/bindings/serial/renesas,hscif.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH v2 5/9] dt-bindings: serial: renesas,scifb: Document r8a7742 bindings
From: Rob Herring @ 2020-05-14 18:58 UTC (permalink / raw)
To: Lad Prabhakar
Cc: linux-gpio, Prabhakar, Thomas Gleixner, linux-renesas-soc,
Marc Zyngier, Ulf Hansson, devicetree, linux-mmc, Linus Walleij,
Geert Uytterhoeven, Greg Kroah-Hartman, Magnus Damm, Rob Herring,
Jason Cooper, linux-serial, linux-kernel, Bartosz Golaszewski
In-Reply-To: <1588794695-27852-6-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com>
On Wed, 6 May 2020 20:51:31 +0100, Lad Prabhakar wrote:
> RZ/G1H (R8A7742) SoC also has the R-Car gen2 compatible SCIFB ports,
> so document the SoC specific bindings.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v1->v2:
> * No change
> ---
>
> Documentation/devicetree/bindings/serial/renesas,scifb.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH v2 4/9] dt-bindings: serial: renesas,scif: Document r8a7742 bindings
From: Rob Herring @ 2020-05-14 18:57 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Geert Uytterhoeven, Jason Cooper, Marc Zyngier, Ulf Hansson,
linux-kernel, linux-renesas-soc, Greg Kroah-Hartman, linux-mmc,
linux-serial, Thomas Gleixner, Rob Herring, Magnus Damm,
Linus Walleij, linux-gpio, Prabhakar, Bartosz Golaszewski,
devicetree
In-Reply-To: <1588794695-27852-5-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com>
On Wed, 6 May 2020 20:51:30 +0100, Lad Prabhakar wrote:
> RZ/G1H (R8A7742) SoC also has the R-Car gen2 compatible SCIF ports,
> so document the SoC specific bindings.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v1->v2:
> * No change
> ---
>
> Documentation/devicetree/bindings/serial/renesas,scif.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH v2 2/9] dt-bindings: irqchip: renesas-irqc: Document r8a7742 bindings
From: Rob Herring @ 2020-05-14 18:57 UTC (permalink / raw)
To: Lad Prabhakar
Cc: linux-kernel, Geert Uytterhoeven, Thomas Gleixner, Magnus Damm,
Bartosz Golaszewski, Jason Cooper, linux-renesas-soc, devicetree,
linux-gpio, Ulf Hansson, Marc Zyngier, linux-mmc, linux-serial,
Prabhakar, Linus Walleij, Greg Kroah-Hartman
In-Reply-To: <20200514184306.GA8929@bogus>
On Thu, May 14, 2020 at 01:43:06PM -0500, Rob Herring wrote:
> On Wed, 6 May 2020 20:51:28 +0100, Lad Prabhakar wrote:
> > Document SoC specific bindings for RZ/G1H (r8a7742) SoC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v1->v2:
> > * No change
> > ---
> >
> > .../devicetree/bindings/interrupt-controller/renesas,irqc.yaml | 1 +
> > 1 file changed, 1 insertion(+)
> >
>
> Acked-by: Rob Herring <robh@kernel.org>
I'll just apply this with the others that haven't been picked up.
Rob
^ permalink raw reply
* Re: [PATCH 09/14] pinctrl: ocelot: Add Sparx5 SoC support
From: kbuild test robot @ 2020-05-14 18:09 UTC (permalink / raw)
To: Lars Povlsen, SoC Team, Arnd Bergmann, Stephen Boyd,
Linus Walleij
Cc: kbuild-all, Lars Povlsen, Steen Hegelund,
Microchip Linux Driver Support, Olof Johansson, Michael Turquette,
devicetree, linux-clk, linux-gpio, linux-arm-kernel
In-Reply-To: <20200513125532.24585-10-lars.povlsen@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 1908 bytes --]
Hi Lars,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on pinctrl/devel clk/clk-next linus/master v5.7-rc5 next-20200512]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Lars-Povlsen/Adding-support-for-Microchip-Sparx5-SoC/20200514-163536
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> drivers/pinctrl/pinctrl-ocelot.c:28: warning: "clrsetbits" redefined
28 | #define clrsetbits(addr, clear, set) |
In file included from include/linux/io.h:13,
from include/linux/irq.h:20,
from include/linux/gpio/driver.h:7,
from drivers/pinctrl/pinctrl-ocelot.c:10:
arch/powerpc/include/asm/io.h:849: note: this is the location of the previous definition
849 | #define clrsetbits(type, addr, clear, set) |
vim +/clrsetbits +28 drivers/pinctrl/pinctrl-ocelot.c
27
> 28 #define clrsetbits(addr, clear, set) \
29 writel((readl(addr) & ~(clear)) | (set), (addr))
30
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66031 bytes --]
^ permalink raw reply
* Re: u-boot DT configuration node
From: Michal Simek @ 2020-05-14 18:46 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree@vger.kernel.org, u-boot@lists.denx.de, Tom Rini,
Loic Poulain
In-Reply-To: <CAL_JsqJksDAt7G_YfpB9P+5GLzf26M4CbcJ+YnjwekorNzSXtg@mail.gmail.com>
čt 14. 5. 2020 v 20:07 odesílatel Rob Herring <robh@kernel.org> napsal:
>
> On Thu, Apr 30, 2020 at 6:13 AM Michal Simek <michal.simek@xilinx.com> wrote:
> >
> > On 29. 04. 20 16:55, Rob Herring wrote:
> > > On Tue, Apr 28, 2020 at 8:51 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > >>
> > >> On 28. 04. 20 15:23, Rob Herring wrote:
> > >>> On Wed, Apr 1, 2020 at 4:23 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > >>>>
> > >>>> Hi Rob and others,
> > >>>>
> > >>>> for couple of years already u-boot is using config node in root DT for
> > >>>> u-boot configuration.
> > >>>>
> > >>>> Here is one example in u-boot source code.
> > >>>> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/exynos5250-spring.dts#L47
> > >>>>
> > >>>> And here is dt binding description
> > >>>> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/doc/device-tree-bindings/config.txt
> > >>>>
> > >>>> I was checking dt binding specification and there no such a thing
> > >>>> described there. It means I expect this is more adhoc u-boot solution.
> > >>>> We have reached the point where could be beneficial to put some u-boot
> > >>>> specific configurations to DT.
> > >>>>
> > >>>> Actually I have done similar thing some time ago too by using chosen
> > >>>> node and add xilinx specific property there to point to eeprom.
> > >>>> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/zynqmp-zcu102-revA.dts#L39
> > >>>
> > >>> In this case, I think an alias should be used as it's more of just a
> > >>> shortcut to finding a specific node.
> > >>
> > >> What alias name do you suggest to use?
> > >> We have systems where one i2c eeprom described based board and another
> > >> i2c eeprom describe bootable module. And I need to have shotcuts to both
> > >> of them.
> > >>
> > >> dt specification doesn't list any keywords for aliases but there is
> > >> generic name recommendation.
> > >
> > > I do want make aliases a registered list of names.
> > >
> > >> Based on keywords it should look like this.
> > >> eeprom0 = ...;
> > >> eeprom1 = ...;
> > >
> > > That was my initial thought, but maybe "nvmemX" to be a bit more generic.
> >
> > I am fine with that. It means that multiple eeproms and order will be
> > direct by alias number.
> > In past I wanted to use list but aliases number is also fine.
> >
> > >
> > >
> > >>>> I think it is a time to discuss it and do it properly.
> > >>>>
> > >>>> First of all my question is where we could list SW prefixes to make sure
> > >>>> that they are listed and everybody is aware about it. We have
> > >>>> vendor-prefixes and we should have a way to record also prefixes for sw
> > >>>> projects. U-Boot is using u-boot. Xen has file in the kernel with using
> > >>>> xen prefix. At least these two should be listed.
> > >>>
> > >>> Documentation/devicetree/bindings/vendor-prefixes.yaml.
> > >>
> > >> thx
> >
> > Sent a patch for it. Please review.
> > https://lore.kernel.org/linux-devicetree/85b8dc9e6288270bbfdf55f1c156dba160293f01.1588239081.git.michal.simek@xilinx.com/
> >
> >
> > >>>> Next my question is what is the recommended way to pass sw specific
> > >>>> parameters via DT? I think using chosen node is more appropriate then
> > >>>> adhoc config node. Or is there a better way how this should be done?
> > >>>
> > >>> /chosen
> > >>>
> > >>> For vendor specific things though I would be cautious. If they are
> > >>> settings for a specific device, then they probably belong in the
> > >>> device's node. Second, are they really vendor specific? What we don't
> > >>> want is each vendor doing the same thing in slightly different ways.
> > >>
> > >> For u-boot specific setting like - offsets it should be generic for
> > >> everybody. I was already talking to Loic that for saving u-boot
> > >> variables to QSPI we should be using MTD partition map and put there
> > >> maybe a flag to say that this is the location for storing them.
> > >
> > > I'd standardize on the partition name.
> >
> > ok. Documentation/devicetree/bindings/mtd/partition.txt?
> >
> > I have grep u-boot repo and I see these label names
> >
> > "NAND.u-boot";
> > "NAND.u-boot-env";
> > "NAND.u-boot-env.backup1";
> > "NAND.u-boot-spl-os";
> > "QSPI.u-boot";
> > "QSPI.u-boot-env";
> > "QSPI.u-boot-env.backup1";
> > "qspi-u-boot-img";
> > "qspi-u-boot-spl";
> > "QSPI.u-boot-spl-os";
> > "u-boot
> > "u-boot";
> > "u-boot-2";
> > "u-boot-2.backup1";
> > "u-boot.backup1";
> > "u-boot-env";
> > "u-boot-env.backup1";
> > "u-boot-spl";
> >
> > kernel is kind of similar
> > "alt-u-boot";
> > "alt-u-boot-env";
> > "NAND.u-boot";
> > "NAND.u-boot-env";
> > "NAND.u-boot-env.backup1";
> > "NAND.u-boot-spl-os";
> > "QSPI.u-boot";
> > "QSPI.u-boot-env";
> > "QSPI.u-boot-env.backup1";
> > "QSPI.u-boot-spl-os";
> > "u-boot
> > "u-boot";
> > "u-boot.backup1";
> > "u-boot-env";
> > "u-boot-env2";
> > "u-boot-env.backup1";
> > "u-boot-environment";
> > "u-boot-factory";
> > "u-boot-nand";
> > "u-boot-nor";
> > "u-boot-spi";
> > "u-boot-spl";
> >
> > It means it is mix of names. I think SPI cases are the most complicated
> > one because you can have multiple spi devices in the system and you
> > can't use the same name for registration.
> >
> > That's why I think that make sense to use an optional prefix as people
> > are using QSPI/NAND already. But not quite sure that using QSPI is
> > generic enough because you can have multiple QSPIs. Using alias name is
> > also not ideal because one simple change in aliases would require
> > changes in partition name/label.
> > Any better suggestion?
>
> Okay, that's a mess of names. I guess perhaps properties in /chosen
> pointing to data would work. Then you just have to update that
> property if you're switching partitions (using SPI vs. MMC or for A/B
> style partition switching). We should point to partitions rather than
> raw offsets though.
That means that when you deploy images this property doesn't need to be there
and then your firmware (in our case u-boot) can fill this property
based on your logic.
I definitely want to avoid cases where we would need to maintain
different DTs for
different mode which would bring more overhead.
We should document these u-boot properties in the u-boot project for sure.
But there could also be the reason to do it in Linux because likely
these properties
will get to Linux dtses. Would be good to get some feedback on this.
And if we should
document it in Linux, path and name suggestions would be welcome.
> One thing I'm not certain of is 'stdout-path' used a path string
> rather than phandle. I'm not sure why. It may predate phandles or that
> they weren't necessarily stable. Or just path string is more human
> readable. So phandle vs. path/alias is something to figure out.
That's not a problem I will figure out.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
^ permalink raw reply
* Re: [PATCH v2 2/9] dt-bindings: irqchip: renesas-irqc: Document r8a7742 bindings
From: Rob Herring @ 2020-05-14 18:43 UTC (permalink / raw)
To: Lad Prabhakar
Cc: linux-kernel, Geert Uytterhoeven, Thomas Gleixner, Rob Herring,
Magnus Damm, Bartosz Golaszewski, Jason Cooper, linux-renesas-soc,
devicetree, linux-gpio, Ulf Hansson, Marc Zyngier, linux-mmc,
linux-serial, Prabhakar, Linus Walleij, Greg Kroah-Hartman
In-Reply-To: <1588794695-27852-3-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com>
On Wed, 6 May 2020 20:51:28 +0100, Lad Prabhakar wrote:
> Document SoC specific bindings for RZ/G1H (r8a7742) SoC.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v1->v2:
> * No change
> ---
>
> .../devicetree/bindings/interrupt-controller/renesas,irqc.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH net-next 1/2] dt-bindings: net: dp83822: Add TI dp83822 phy
From: Andrew Lunn @ 2020-05-14 18:39 UTC (permalink / raw)
To: Dan Murphy
Cc: f.fainelli, hkallweit1, davem, robh, netdev, linux-kernel,
devicetree, Rob Herring
In-Reply-To: <20200514173055.15013-2-dmurphy@ti.com>
On Thu, May 14, 2020 at 12:30:54PM -0500, Dan Murphy wrote:
> Add a dt binding for the TI dp83822 ethernet phy device.
>
> CC: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> .../devicetree/bindings/net/ti,dp83822.yaml | 49 +++++++++++++++++++
> 1 file changed, 49 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/ti,dp83822.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> new file mode 100644
> index 000000000000..60afd43ad3b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
> +# Copyright (C) 2020 Texas Instruments Incorporated
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/net/ti,dp83822.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: TI DP83822 ethernet PHY
> +
> +allOf:
> + - $ref: "ethernet-controller.yaml#"
> +
> +maintainers:
> + - Dan Murphy <dmurphy@ti.com>
> +
> +description: |
> + The DP83822 is a low-power, single-port, 10/100 Mbps Ethernet PHY. It
> + provides all of the physical layer functions needed to transmit and receive
> + data over standard, twisted-pair cables or to connect to an external,
> + fiber-optic transceiver. Additionally, the DP83822 provides flexibility to
> + connect to a MAC through a standard MII, RMII, or RGMII interface
Hi Dan
You say 10/100 Mbps Ethernet PHY, but then list RGMII?
> +
> + Specifications about the charger can be found at:
> + http://www.ti.com/lit/ds/symlink/dp83822i.pdf
> +
> +properties:
> + reg:
> + maxItems: 1
> +
> + ti,signal-polarity-low:
> + type: boolean
> + description: |
> + DP83822 PHY in Fiber mode only.
> + Sets the DP83822 to detect a link drop condition when the signal goes
> + high. If not set then link drop will occur when the signal goes low.
Are we talking about the LOS line from the SFP cage? In the SFF/SFP
binding we have:
- los-gpios : GPIO phandle and a specifier of the Receiver Loss of Signal
Indication input gpio signal, active (signal lost) high
It would be nice to have a consistent naming.
Is it required the LOS signal is connected to the PHY? Russell King
has some patches which allows the Marvell PHY to be used as a media
converter. In that setting, i think the SFP signals are connected to
GPIOs not the PHY. The SFP core can then control the transmit disable,
module insertion detection etc. So i'm wondering if you need a
property to indicate the LOS is not connected to the PHY?
Andrew
^ permalink raw reply
* Re: [PATCH v2 03/20] dt-bindings: Add vendor prefix for Baikal Electronics, JSC
From: Rob Herring @ 2020-05-14 18:31 UTC (permalink / raw)
To: Sergey.Semin
Cc: Serge Semin, Linus Walleij, Ralf Baechle, Paul Burton,
Thomas Bogendoerfer, linux-mips, linux-pm, Sam Ravnborg,
Arnd Bergmann, Mark Brown, Heiko Stuebner, linux-kernel,
Maxime Ripard, Stephan Gerhold, Rob Herring, Alexey Malahov,
devicetree
In-Reply-To: <20200506174238.15385-4-Sergey.Semin@baikalelectronics.ru>
On Wed, 6 May 2020 20:42:21 +0300, wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>
> Add "BAIKAL ELECTRONICS, JSC" to the list of devicetree vendor prefixes
> as "baikal".
>
> Website: http://www.baikalelectronics.com
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-mips@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
>
> ---
>
> Changelog v2:
> - Fix author and SoB emails mismatch.
> - Add 'baikal' vendor prefix instead of ambiguous 'be'.
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
Applied, thanks!
^ permalink raw reply
* Re: [RFC v1 0/3] Add QTI QFPROM-Efuse driver support
From: Doug Anderson @ 2020-05-14 18:21 UTC (permalink / raw)
To: Ravi Kumar Bokka (Temp)
Cc: Srinivas Kandagatla, Rob Herring, LKML,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate,
c_rbokka, mkurumel
In-Reply-To: <fb7f601f-388f-8a77-bb22-e1398f90326f@codeaurora.org>
Hi,
On Wed, May 13, 2020 at 8:35 PM Ravi Kumar Bokka (Temp)
<rbokka@codeaurora.org> wrote:
>
> Hi doug,
>
> Thanks for your feedback. Please find below inline comments.
Probably the mailing list didn't see them. You responded with HTML
mail. Please be careful to only respond in plain text.
> Regards,
>
> Ravi Kumar.B
>
>
>
> On 5/13/2020 4:33 AM, Doug Anderson wrote:
>
> Hi,
>
> On Tue, May 12, 2020 at 11:18 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote:
>
> This patch series adds qfprom-efuse controller driver support.
>
> This driver can access the raw qfprom regions for fuse blowing.
>
> The current existed qfprom driver is only supports for cpufreq, thermal sensors
> drivers by read out calibration data, speed bins..etc which is stored by
> qfprom efuses.
>
> I don't understand the interaction between this driver and the
> existing "qcom,qfprom" driver. Can you please explain? Are they both
> acting on the same values and this one has write access? Are there
> two instances of the same hardware block and you're managing one of
> them with this new driver and one with thue old driver? Something
> else?
>
> [Ravi] Existing QFPROM driver in upstream kernel has limited support which is some hard coded mapping of id vs set of fuses and user can read those fuse with those id-bucket.
> That is simply reading Hw-registers and it doesn't involve any hardware programming sequence etc. Based on information given to us by QC-kernel team, existing driver was created to read calibration/sensor fuses and it is very basic/limited/fixed in functionalities and orthogonal to what we need to on Trogdor.
>
> Requirement for Trogdor fuse blow driver is different which allows to read/write almost whole fuse block and requires to follow HW programming guide. Both are completely separate and has no overlapping in terms of functionalities and capability. Please ignore the similarity of names of drivers, they are different in terms of functionalities and driver internals etc.
If they are targeting the same type of hardware IP block then, in the
very least, the bindings need to be the same. The bindings are
supposed to be describing the hardware.
Presumably if the underlying hardware is the same you should be able
to write one driver and it can just operate in some sort of
"read-only" mode if it's running somewhere it doesn't have access
permissions to actually change the fuses.
> Ravi Kumar Bokka (3):
> dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse
> drivers: nvmem: Add driver for QTI qfprom-efuse support
> arm64: dts: qcom: sc7180: Add qfprom-efuse
>
> .../devicetree/bindings/nvmem/qfprom-efuse.yaml | 40 ++
> arch/arm64/boot/dts/qcom/sc7180-idp.dts | 4 +
> arch/arm64/boot/dts/qcom/sc7180.dtsi | 9 +
> drivers/nvmem/Kconfig | 10 +
> drivers/nvmem/Makefile | 2 +
> drivers/nvmem/qfprom-efuse.c | 476 +++++++++++++++++++++
> 6 files changed, 541 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
> create mode 100644 drivers/nvmem/qfprom-efuse.c
>
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.
>
^ permalink raw reply
* Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support
From: Doug Anderson @ 2020-05-14 18:21 UTC (permalink / raw)
To: Ravi Kumar Bokka (Temp)
Cc: Srinivas Kandagatla, Rob Herring, LKML,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate,
c_rbokka, mkurumel
In-Reply-To: <14e1fa51-066c-6e1b-01a4-2103612de9e9@codeaurora.org>
Hi,
I notice that you didn't respond to any of my feedback [1], only
Srinivas's. Any reason why? It turns out that Srinivas had quite a
lot of the same feedback as I did, but please make sure you didn't
miss anything I suggested. More inline below.
On Thu, May 14, 2020 at 5:26 AM Ravi Kumar Bokka (Temp)
<rbokka@codeaurora.org> wrote:
>
> Hi Srinivas,
> Thanks for your feedback by giving review comments. Please find my
> inline comments.
>
>
> Regards,
> Ravi Kumar.B
>
> On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
> >
> >
> > On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
> >> This patch adds new driver for QTI qfprom-efuse controller. This
> >> driver can
> >> access the raw qfprom regions for fuse blowing.
> >
> > QTI?
>
> guidance I have received from internal Legal/LOST team is that the QCOM
> prefix needs to be changed to QTI everywhere it is used
I'll let Srinivas comment if he cares. I'm really not sure why a
legal team cares about the Kconfig name in a GPL-licensed Linux
kernel.
> >> The current existed qfprom driver is only supports for cpufreq,
> >> thermal sensors
> >> drivers by read out calibration data, speed bins..etc which is stored
> >> by qfprom efuses.
> >
> > Can you explain bit more about this QFPROM instance, Is this QFPROM part
> > of secure controller address space?
> > Is this closely tied to SoC or Secure controller version?
> >
> > Any reason why this can not be integrated into qfprom driver with
> > specific compatible.
> >
>
> QFPROM driver communicates with sec_controller address space however
> scope and functionalities of this driver is different and not limited as
> existing qfprom fuse Read-Only driver for specific “fuse buckets’ like
> cpufreq, thermal sensors etc. QFPROM fuse write driver in this patch
> requires specific sequence to write/blow fuses unlike other driver.
> Scope/functionalities are different and this is separate driver.
If the underlying IP blocks are the same it should be one driver and
it should just work in read-only mode for the other range of stuff.
> >> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> >> ---
> >> drivers/nvmem/Kconfig | 10 +
> >> drivers/nvmem/Makefile | 2 +
> >> drivers/nvmem/qfprom-efuse.c | 476
> >> +++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 488 insertions(+)
> >> create mode 100644 drivers/nvmem/qfprom-efuse.c
> >>
> > ...
> >
> >> diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
> >> new file mode 100644
> >> index 0000000..2e3c275
> >> --- /dev/null
> >> +++ b/drivers/nvmem/qfprom-efuse.c
> >> @@ -0,0 +1,476 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/device.h>
> >> +#include <linux/io.h>
> >> +#include <linux/iopoll.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mod_devicetable.h>
> >> +#include <linux/nvmem-provider.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regulator/consumer.h>
> >> +
> >> +#define QFPROM_BLOW_STATUS_BUSY 0x1
> >> +#define QFPROM_BLOW_STATUS_READY 0x0
> >> +
> >> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
> >> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
> >> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
> >> +
> >> +/* Amount of time required to hold charge to blow fuse in
> >> micro-seconds */
> >> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
> >> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
> >> +
> >> +#define QFPROM_ACCEL_OFFSET 0x044
> >> +
> >> +/**
> >> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
> >> + * platform data
> >> + *
> >> + * @name: qfprom-efuse compatible name
> >
> > ??
>
> Thanks for your feedback. I will address this change
>
> >> + * @fuse_blow_time_in_us: Should contain the wait time when doing the
> >> fuse blow
> >> + * @accel_value: Should contain qfprom accel value
> >> + * @accel_reset_value: The reset value of qfprom accel value
> >> + * @qfprom_blow_timer_value: The timer value of qfprom when doing
> >> efuse blow
> >> + * @qfprom_blow_reset_freq: The frequency required to set when fuse
> >> blowing
> >> + * is done
> >> + * @qfprom_blow_set_freq: The frequency required to set when we start
> >> the
> >> + * fuse blowing
> >> + * @qfprom_max_vol: max voltage required to set fuse blow
> >> + * @qfprom_min_vol: min voltage required to set fuse blow
> >
> > How specific are these values per SoC?
> >
>
> This voltage level may change based on SoC and/or fuse-hardware
> technology, it would change for SoC with different technology, hence we
> have kept it in SOC specific settings.
Generally I'd expect the SoC specific settings to be in the device
tree. Drivers don't need to specify this. Please respond to the
comments I posed in my review.
> >> + */
> >> +struct qfprom_efuse_platform_data {
> >> + const char *name;
> >> + u8 fuse_blow_time_in_us;
> >> + u32 accel_value;
> >> + u32 accel_reset_value;
> >> + u32 qfprom_blow_timer_value;
> >> + u32 qfprom_blow_reset_freq;
> >> + u32 qfprom_blow_set_freq;
> >> + u32 qfprom_max_vol;
> >> + u32 qfprom_min_vol;
> >> +};
> >> +
> >> +/**
> >> + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
> >> + *
> >> + * @qfpbase: iomapped memory space for qfprom base
> >> + * @qfpraw: iomapped memory space for qfprom raw fuse region
> >> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
> >> +
> >> + * @dev: qfprom device structure
> >> + * @secclk: clock supply
> >> + * @vcc: regulator supply
> >> +
> >> + * @qfpraw_start: qfprom raw fuse start region
> >> + * @qfpraw_end: qfprom raw fuse end region
> >> + * @qfprom_efuse_platform_data: qfprom platform data
> >> + */
> >> +struct qfprom_efuse_priv {
> >> + void __iomem *qfpbase;
> >> + void __iomem *qfpraw;
> >> + void __iomem *qfpmap;
> >
> > Why are these memory regions split? Can't you just have complete qfprom
> > area and add fixed offset for qfpraw within the driver?
> >
>
> Thanks for your feedback. I will address this change.
> I have separated this memory regions because to identify raw fuse
> regions separately and compare these raw fuse regions from the user
> given input.
How are you addressing? Can you go back to just having one range? If
you need to know where the raw fuse region is inside your range just
put the offset in the of_match data.
> >> + struct device *dev;
> >> + struct clk *secclk;
> >> + struct regulator *vcc;
> >> + resource_size_t qfpraw_start;
> >> + resource_size_t qfpraw_end;
> > Why do we need to check this range? as long as we set the nvmem_config
> > with correct range then you should not need this check.
> >
>
> There is no harm in this explicit check in QFPROM-fuse driver and based
> on internal review with our security team, this check is important to
> avoid dependency on other upper layer.
There is harm: it adds extra complexity. Please remove.
You are talking as if it was somehow important for this code to be the
same as the code on other OSes / in other contexts. It isn't. This
is a Linux driver and it should not be written to duplicate stuff that
Linux is already doing.
> >> + struct qfprom_efuse_platform_data efuse;
> > A pointer here should be good enough?
> >> +};
> >> +
> >
>
> Thanks for your feedback. I will address this change
>
> > ...
> >
> >> +/*
> >> + * sets the value of the blow timer, accel register and the clock
> >> + * and voltage settings
> >> + */
> >> +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv
> >> *priv)
> >> +{
> >> + int ret;
> >> +
> >> + ret = qfprom_disable_fuse_blowing(priv);
> >> + if (ret) {
> >> + dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
> >> + return ret;
> >> + }
> >
> > Why do we need to qfprom_disable_fuse_blowing() for every call to enable
> > it?
> >
> > Or are we missing some error handling in the caller?
> >
>
> We must disable/vote-off this QFPROM fuse power rail after blowing fuse,
> it is the safe and right approach as per hardware programming guide for
> fuse blowing process. Caller here is user space, can’t control
> fuse-power-rail or can’t be relied to follow the required process. There
> could also be unnecessary risk of leaving the vote/power-rail configured
> at specific level after blowing the fuse. As per hardware requirement,
> right after fuse blowing, we need to disable power rail.
Please remove your disable here. Though the user initiates the call
you can still rely on Linux to make sure two users aren't trying to
blow fuses at the same time. The Linux driver should always leave
things in a "disabled" state and you can rely on that.
...besides the only way you aren't hitting an underflow on the
regulator enable count is that you are constantly enabling over and
over again.
> >> +
> >> + writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
> >> + QFPROM_BLOW_TIMER_OFFSET);
> >> + writel(priv->efuse.accel_value, priv->qfpmap + QFPROM_ACCEL_OFFSET);
> >> +
> >> + ret = qfprom_set_clock_settings(priv);
> >> + if (ret) {
> >> + dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
> >> + return ret;
> >> + }
> >> +
> >> + ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
> >> + priv->efuse.qfprom_max_vol);
> >> + if (ret) {
> >> + dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
> >> + return ret;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >
> > <<
> >> +/*
> >> + * verifying to make sure address being written or read is from qfprom
> >> + * raw address range
> >> + */
> >> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
> >> + size_t bytes)
> >> +{
> >> + if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
> >> + ((reg + bytes) <= priv->qfpraw_end)) {
> >> + return 1;
> >> + }
> >> +
> >> + return 0;
> >> +}
> > >>
> > Above function is totally redundant, nvmem core already has checks for
> > this.
> >
>
> There is no harm in this explicit check in QFPROM-fuse driver and based
> on internal review with our security team, this check is important to
> avoid dependency on other upper layer.
Please remove. You are a Linux driver.
> >> +
> >> +/*
> >> + * API for reading from raw qfprom region
> >> + */
> >> +static int qfprom_efuse_reg_read(void *context, unsigned int reg,
> >> void *_val,
> >> + size_t bytes)
> >> +{
> >> + struct qfprom_efuse_priv *priv = context;
> >> + u32 *value = _val;
> >> + u32 align_check;
> >> + int i = 0, words = bytes / 4;
> >> +
> >> + dev_info(priv->dev,
> >> + "reading raw qfprom region offset: 0x%08x of size: %zd\n",
> >> + reg, bytes);
> >
> > In general there is lot of debug info across the code, do you really
> > need all this? Consider removing these!
> >
>
> Thanks for your feedback. I will address this change.
>
> >> +
> >> + if (bytes % 4 != 0x00) {
> >> + dev_err(priv->dev,
> >> + "Bytes: %zd to read should be word align\n",
> >> + bytes);
> >> + return -EINVAL;
> >> + }
> >
> > This word align check is also redundant once you set nvmem_config with
> > correct word_size.
> >
>
> I understand that there may be different approach to handle this. We
> have used this approach and tested this driver thoroughly. Unless there
> is technical limitation, changing this word_size would end up requiring
> re-writing write/read APIs and going through testing again, there is not
> much difference in either approach, we would like to keep this approach
> unless there is technical concern.
The driver isn't done until it lands in Linux. While it's important
to test the driver before posting upstream it is completely expected
and normal that then posting upstream you will be asked to change
things. After you change things you will need to re-test. The fact
that you already tested this the old way is not an excuse. Please
fix.
> >> +
> >> + if (!addr_in_qfprom_range(priv, reg, bytes)) {
> >> + dev_err(priv->dev,
> >> + "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
> >> + reg, bytes);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + align_check = (reg & 0xF);
> >> +
> >> + if (((align_check & ~3) == align_check) && value != NULL)
> >> + while (words--)
> >> + *value++ = readl(priv->qfpbase + reg + (i++ * 4));
> >> +
> >> + else
> >> + dev_err(priv->dev,
> >> + "Invalid input parameter 0x%08x fuse blow address\n",
> >> + reg);
> >> +
> >> + return 0;
> >> +}
> > ...
> >
> >> +
> >> +static int qfprom_efuse_probe(struct platform_device *pdev)
> >> +{
> >> + struct device *dev = &pdev->dev;
> >> + struct resource *qfpbase, *qfpraw, *qfpmap;
> >> + struct nvmem_device *nvmem;
> >> + struct nvmem_config *econfig;
> >> + struct qfprom_efuse_priv *priv;
> >> + const struct qfprom_efuse_platform_data *drvdata;
> >> + int ret;
> >> +
> >> + dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
> >> +
> >
> > too much debug!
> >
>
> Thanks for your feedback. I will address this change.
>
> >> + drvdata = of_device_get_match_data(&pdev->dev);
> >> + if (!drvdata)
> >> + return -EINVAL;
> > Unnecessary check as this driver will not be probed unless there is a
> > compatible match.
> >
>
> Thanks for your feedback. I will address this change.
>
> >
> >> +
> >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> + if (!priv)
> >> + return -ENOMEM;
> >> +
> >> + priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
> >> + priv->efuse.accel_value = drvdata->accel_value;
> >> + priv->efuse.accel_reset_value = drvdata->accel_reset_value;
> >> + priv->efuse.qfprom_blow_timer_value =
> >> drvdata->qfprom_blow_timer_value;
> >> + priv->efuse.qfprom_blow_reset_freq =
> >> drvdata->qfprom_blow_reset_freq;
> >> + priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
> >> + priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
> >> + priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
> >> + priv->dev = dev;
> >> +
> >> + qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +
> >> + priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
> >> + if (IS_ERR(priv->qfpbase)) {
> >> + ret = PTR_ERR(priv->qfpbase);
> >> + goto err;
> >> + }
> >> +
> >> + qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >> +
> >> + priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
> >> + if (IS_ERR(priv->qfpraw)) {
> >> + ret = PTR_ERR(priv->qfpraw);
> >> + goto err;
> >> + }
> >> +
> >> + priv->qfpraw_start = qfpraw->start - qfpbase->start;
> >> + priv->qfpraw_end = qfpraw->end - qfpbase->start;
> >> +
> >> + qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> >> +
> >> + priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
> >> + if (IS_ERR(priv->qfpmap)) {
> >> + ret = PTR_ERR(priv->qfpmap);
> >> + goto err;
> >> + }
> >> +
> >> + priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
> >
> > I see no reference to this regulator in dt bindings.
>
> This perameter kept in board specific file i.e., sc7180-idp.dts file
Yes, but it still needs to be in the bindings.
> >> + if (IS_ERR(priv->vcc)) {
> >> + ret = PTR_ERR(priv->vcc);
> >> + if (ret == -ENODEV)
> >> + ret = -EPROBE_DEFER;
> > Can you explain what is going on here?
> >
>
> As i took other drivers reference, i have kept this check.
Then other drivers are wrong. Please remove.
> >> +
> >> + goto err;
> >> + }
> >> +
> >> + priv->secclk = devm_clk_get(dev, "secclk");
> >> + if (IS_ERR(priv->secclk)) {
> >> + ret = PTR_ERR(priv->secclk);
> >> + if (ret != -EPROBE_DEFER)
> >> + dev_err(dev, "secclk error getting : %d\n", ret);
> >> + goto err;
> >> + }
> >> +
> >> + ret = clk_prepare_enable(priv->secclk);
> >> + if (ret) {
> >> + dev_err(dev, "clk_prepare_enable() failed\n");
> >> + goto err;
> >> + }
> >> +
> >> + econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
> >> + if (!econfig)
> > Why not disabling the clk here?
> >> + return -ENOMEM;
> >
>
> Thanks for your feedback. I will address this change.
>
> >> +
> >> + econfig->dev = dev;
> >> + econfig->name = "qfprom-efuse";
> >> + econfig->stride = 1;
> >> + econfig->word_size = 1;
> >> + econfig->reg_read = qfprom_efuse_reg_read;
> >> + econfig->reg_write = qfprom_efuse_reg_write;
> >> + econfig->size = resource_size(qfpraw);
> >> + econfig->priv = priv;
> >> +
> >> + nvmem = devm_nvmem_register(dev, econfig);
> >> +
> >> + return PTR_ERR_OR_ZERO(nvmem);
> > probably you should check the nvmem here before returning to disable the
> > clk properly.
> >
>
> Thanks for your feedback. I will address this change.
>
> >> +
> >> +err:
> >> + clk_disable_unprepare(priv->secclk);
> >> + return ret;
> >> +}
> >> +
> >> +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
> >> + .name = "sc7180-qfprom-efuse",
> > Redundant.
> >
>
> Thanks for your feedback. I will address this change.
>
> >> + .fuse_blow_time_in_us = 10,
> >> + .accel_value = 0xD10,
> >> + .accel_reset_value = 0x800,
> >> + .qfprom_blow_timer_value = 25,
> >> + .qfprom_blow_reset_freq = 19200000,
> >> + .qfprom_blow_set_freq = 4800000,
> >> + .qfprom_max_vol = 1904000,
> >> + .qfprom_min_vol = 1800000,
> >> +};
> >> +
> >> +static const struct of_device_id qfprom_efuse_of_match[] = {
> >> + {
> >> + .compatible = "qcom,sc7180-qfprom-efuse",
> >> + .data = &sc7180_qfp_efuse_data
> >> + },
> >> + {/* sentinel */},
> >> +};
> >> +
> >> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
> >> +
> >> +static struct platform_driver qfprom_efuse_driver = {
> >> + .probe = qfprom_efuse_probe,
> >> + .driver = {
> >> + .name = "sc7180-qfprom-efuse",
> >> + .of_match_table = qfprom_efuse_of_match,
> >> + },
> >> +};
> >> +
> >> +module_platform_driver(qfprom_efuse_driver);
> >> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
> >> +MODULE_LICENSE("GPL v2");
> >>
>
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of the Code Aurora Forum, hosted by the Linux Foundation.
[1] https://lore.kernel.org/r/CAD=FV=UZQRfBTbh2CLnwsRSpbXFf=8iF2MG20hdj47s42aP8HQ@mail.gmail.com
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props
From: Prashant Malani @ 2020-05-14 18:16 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Rob Herring, linux-kernel, twawrzynczak, Benson Leung,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Enric Balletbo i Serra, Guenter Roeck
In-Reply-To: <20200512134154.GC2085641@kuha.fi.intel.com>
Hi folks,
On Tue, May 12, 2020 at 04:41:54PM +0300, Heikki Krogerus wrote:
> Hi guys,
>
> On Mon, May 11, 2020 at 01:46:35PM -0700, Prashant Malani wrote:
> > Hi Rob,
> >
> > Thank you for reviewing the patch. Kindly see my comments inline:
> >
> > On Mon, May 11, 2020 at 02:28:00PM -0500, Rob Herring wrote:
> > > On Wed, Apr 22, 2020 at 03:22:39PM -0700, Prashant Malani wrote:
> > > > Add properties for mode, orientation and USB data role switches for
> > > > Type C connectors. When available, these will allow the Type C connector
> > > > class port driver to configure the various switches according to USB PD
> > > > information (like orientation, alt mode etc.) provided by the Chrome OS
> > > > EC controller.
> > > >
> > > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > > ---
> > > > .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
> > > > 1 file changed, 26 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > > index 6d7396ab8bee..b5814640aa32 100644
> > > > --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > > +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > > @@ -21,7 +21,21 @@ properties:
> > > > const: google,cros-ec-typec
> > > >
> > > > connector:
> > > > - $ref: /schemas/connector/usb-connector.yaml#
> > > > + allOf:
> > > > + - $ref: /schemas/connector/usb-connector.yaml#
> > > > + - type: object
> > > > + properties:
> > >
> > > These don't seem CrOS EC specific, so why document them as such.
> >
> > Are you referring to the "mode-switch", "orientation-switch" and
> > "usb-role-switch" properties? If so, then yes, they aren't Cros EC
> > specific. The Type C connector class framework requires the nodes to be
> > named like this, and the cros-ec-typec driver uses this framework, hence
> > the description here (the Type C connector class framework doesn't have
> > any bindings).
> >
> > Would it be better to add in the description string that Type Connector
> > class expects these switches to be named this way? :
> >
> > " Reference to a DT node for the USB Type C Multiplexer controlling the
> > data lines routing for this connector. This switch is assumed registered
> > with the Type C connector class framework, which requires it to be named
> > this way."
> > >
> > > > + mode-switch:
> > > > + description: Reference to a DT node for the USB Type C Multiplexer
> > > > + controlling the data lines routing for this connector.
> > >
> > > This is for alternate mode muxing I presume.
> >
> > Yes, that's right.
> > >
> > > We already have a mux-control binding. Why not use that here?
> >
> > Heikki might be able to offer more insight into why this is the case,
> > since the connector class framework seems to expect a phandle and for
> > the device driver to implement a "set" command. Heikki, would you happen to know?
>
> The mode-switch here would actually represent the "consumer" part in
> the mux-control bindings. So the mux-controls would describe the
> relationship between the "mode-switch" and the mux controller(s),
> while the mode-switch property describes the relationship between
> something like USB Type-C Port Manager (or this cros_ec function) and
> the "mux consumer".
>
Thanks for the explanation, Heikki.
Hi Rob,
Does the above explanation help clarify the usage here?
If so, shall I upload a new patch version with the additional text
(referencing Type C connector class framework) added to the *-switch
descriptions?
Best regards,
-Prashant
> > > > +
> > > > + orientation-switch:
> > > > + description: Reference to a DT node for the USB Type C orientation
> > > > + switch for this connector.
> > >
> > > What's in this node?
> >
> > Similar to the other "-switch", this will contain a phandle to a device
> > which can control orientation settings for the Type C Mux. The connector
> > class API assumes the switches are named this way. For example:
> >
> > orientation-switch:
> > https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/usb/typec/mux.c#L64
> >
> > mode-switch:
> > https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/usb/typec/mux.c#L258
> >
> > >
> > > > +
> > > > + usb-role-switch:
> > > > + description: Reference to a DT node for the USB Data role switch
> > > > + for this connector.
> > > >
> > > > required:
> > > > - compatible
> > > > @@ -49,6 +63,17 @@ examples:
> > > > data-role = "dual";
> > > > try-power-role = "source";
> > > > };
> > > > +
> > > > + connector@1 {
> > > > + compatible = "usb-c-connector";
> > > > + reg = <1>;
> > > > + power-role = "dual";
> > > > + data-role = "host";
> > > > + try-power-role = "source";
> > > > + mode-switch = <&typec_mux>;
> > > > + orientation-switch = <&typec_orientation_switch>;
> > > > + usb-role-switch = <&typec_mux>;
> > > > + };
> > > > };
> > > > };
> > > > };
> > > > --
> > > > 2.26.1.301.g55bc3eb7cb9-goog
> > > >
>
> thanks,
>
> --
> heikki
^ permalink raw reply
* Re: [PATCH v2 03/20] dt-bindings: Add vendor prefix for Baikal Electronics, JSC
From: Serge Semin @ 2020-05-14 18:13 UTC (permalink / raw)
To: Thomas Bogendoerfer, Rob Herring
Cc: Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle,
Arnd Bergmann, linux-mips, linux-pm, Rob Herring, Sam Ravnborg,
Maxime Ripard, Heiko Stuebner, Linus Walleij, Stephan Gerhold,
Mark Brown, devicetree, linux-kernel
In-Reply-To: <20200506174238.15385-4-Sergey.Semin@baikalelectronics.ru>
Rob,
Could you also take a look at this patch? There are several patchsets I've sent
which depend on the vendor-prefix it defines. So when you get to check those
patchsets DT files, the dt_binding_check will fail without it. Is it possible
somehow to pick this patch up from here and apply it before checking those
Baikal-T1-specific binding files?
-Sergey
On Wed, May 06, 2020 at 08:42:21PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>
> Add "BAIKAL ELECTRONICS, JSC" to the list of devicetree vendor prefixes
> as "baikal".
>
> Website: http://www.baikalelectronics.com
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-mips@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
>
> ---
>
> Changelog v2:
> - Fix author and SoB emails mismatch.
> - Add 'baikal' vendor prefix instead of ambiguous 'be'.
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index d3891386d671..674c0d07c0ad 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -139,6 +139,8 @@ patternProperties:
> description: Azoteq (Pty) Ltd
> "^azw,.*":
> description: Shenzhen AZW Technology Co., Ltd.
> + "^baikal,.*":
> + description: BAIKAL ELECTRONICS, JSC
> "^bananapi,.*":
> description: BIPAI KEJI LIMITED
> "^beacon,.*":
> --
> 2.25.1
>
^ permalink raw reply
* Re: u-boot DT configuration node
From: Rob Herring @ 2020-05-14 18:07 UTC (permalink / raw)
To: Michal Simek
Cc: devicetree@vger.kernel.org, u-boot@lists.denx.de, Tom Rini,
Loic Poulain
In-Reply-To: <fefb0e66-b82e-23ec-1e81-471d590df032@xilinx.com>
On Thu, Apr 30, 2020 at 6:13 AM Michal Simek <michal.simek@xilinx.com> wrote:
>
> On 29. 04. 20 16:55, Rob Herring wrote:
> > On Tue, Apr 28, 2020 at 8:51 AM Michal Simek <michal.simek@xilinx.com> wrote:
> >>
> >> On 28. 04. 20 15:23, Rob Herring wrote:
> >>> On Wed, Apr 1, 2020 at 4:23 AM Michal Simek <michal.simek@xilinx.com> wrote:
> >>>>
> >>>> Hi Rob and others,
> >>>>
> >>>> for couple of years already u-boot is using config node in root DT for
> >>>> u-boot configuration.
> >>>>
> >>>> Here is one example in u-boot source code.
> >>>> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/exynos5250-spring.dts#L47
> >>>>
> >>>> And here is dt binding description
> >>>> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/doc/device-tree-bindings/config.txt
> >>>>
> >>>> I was checking dt binding specification and there no such a thing
> >>>> described there. It means I expect this is more adhoc u-boot solution.
> >>>> We have reached the point where could be beneficial to put some u-boot
> >>>> specific configurations to DT.
> >>>>
> >>>> Actually I have done similar thing some time ago too by using chosen
> >>>> node and add xilinx specific property there to point to eeprom.
> >>>> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/zynqmp-zcu102-revA.dts#L39
> >>>
> >>> In this case, I think an alias should be used as it's more of just a
> >>> shortcut to finding a specific node.
> >>
> >> What alias name do you suggest to use?
> >> We have systems where one i2c eeprom described based board and another
> >> i2c eeprom describe bootable module. And I need to have shotcuts to both
> >> of them.
> >>
> >> dt specification doesn't list any keywords for aliases but there is
> >> generic name recommendation.
> >
> > I do want make aliases a registered list of names.
> >
> >> Based on keywords it should look like this.
> >> eeprom0 = ...;
> >> eeprom1 = ...;
> >
> > That was my initial thought, but maybe "nvmemX" to be a bit more generic.
>
> I am fine with that. It means that multiple eeproms and order will be
> direct by alias number.
> In past I wanted to use list but aliases number is also fine.
>
> >
> >
> >>>> I think it is a time to discuss it and do it properly.
> >>>>
> >>>> First of all my question is where we could list SW prefixes to make sure
> >>>> that they are listed and everybody is aware about it. We have
> >>>> vendor-prefixes and we should have a way to record also prefixes for sw
> >>>> projects. U-Boot is using u-boot. Xen has file in the kernel with using
> >>>> xen prefix. At least these two should be listed.
> >>>
> >>> Documentation/devicetree/bindings/vendor-prefixes.yaml.
> >>
> >> thx
>
> Sent a patch for it. Please review.
> https://lore.kernel.org/linux-devicetree/85b8dc9e6288270bbfdf55f1c156dba160293f01.1588239081.git.michal.simek@xilinx.com/
>
>
> >>>> Next my question is what is the recommended way to pass sw specific
> >>>> parameters via DT? I think using chosen node is more appropriate then
> >>>> adhoc config node. Or is there a better way how this should be done?
> >>>
> >>> /chosen
> >>>
> >>> For vendor specific things though I would be cautious. If they are
> >>> settings for a specific device, then they probably belong in the
> >>> device's node. Second, are they really vendor specific? What we don't
> >>> want is each vendor doing the same thing in slightly different ways.
> >>
> >> For u-boot specific setting like - offsets it should be generic for
> >> everybody. I was already talking to Loic that for saving u-boot
> >> variables to QSPI we should be using MTD partition map and put there
> >> maybe a flag to say that this is the location for storing them.
> >
> > I'd standardize on the partition name.
>
> ok. Documentation/devicetree/bindings/mtd/partition.txt?
>
> I have grep u-boot repo and I see these label names
>
> "NAND.u-boot";
> "NAND.u-boot-env";
> "NAND.u-boot-env.backup1";
> "NAND.u-boot-spl-os";
> "QSPI.u-boot";
> "QSPI.u-boot-env";
> "QSPI.u-boot-env.backup1";
> "qspi-u-boot-img";
> "qspi-u-boot-spl";
> "QSPI.u-boot-spl-os";
> "u-boot
> "u-boot";
> "u-boot-2";
> "u-boot-2.backup1";
> "u-boot.backup1";
> "u-boot-env";
> "u-boot-env.backup1";
> "u-boot-spl";
>
> kernel is kind of similar
> "alt-u-boot";
> "alt-u-boot-env";
> "NAND.u-boot";
> "NAND.u-boot-env";
> "NAND.u-boot-env.backup1";
> "NAND.u-boot-spl-os";
> "QSPI.u-boot";
> "QSPI.u-boot-env";
> "QSPI.u-boot-env.backup1";
> "QSPI.u-boot-spl-os";
> "u-boot
> "u-boot";
> "u-boot.backup1";
> "u-boot-env";
> "u-boot-env2";
> "u-boot-env.backup1";
> "u-boot-environment";
> "u-boot-factory";
> "u-boot-nand";
> "u-boot-nor";
> "u-boot-spi";
> "u-boot-spl";
>
> It means it is mix of names. I think SPI cases are the most complicated
> one because you can have multiple spi devices in the system and you
> can't use the same name for registration.
>
> That's why I think that make sense to use an optional prefix as people
> are using QSPI/NAND already. But not quite sure that using QSPI is
> generic enough because you can have multiple QSPIs. Using alias name is
> also not ideal because one simple change in aliases would require
> changes in partition name/label.
> Any better suggestion?
Okay, that's a mess of names. I guess perhaps properties in /chosen
pointing to data would work. Then you just have to update that
property if you're switching partitions (using SPI vs. MMC or for A/B
style partition switching). We should point to partitions rather than
raw offsets though.
One thing I'm not certain of is 'stdout-path' used a path string
rather than phandle. I'm not sure why. It may predate phandles or that
they weren't necessarily stable. Or just path string is more human
readable. So phandle vs. path/alias is something to figure out.
Rob
^ permalink raw reply
* Re: [PATCH v2 02/20] dt-bindings: bus: Add MIPS CDMM controller
From: Serge Semin @ 2020-05-14 18:05 UTC (permalink / raw)
To: Rob Herring
Cc: Serge Semin, Paul Burton, Thomas Bogendoerfer, linux-kernel,
Arnd Bergmann, Rob Herring, linux-pm, Ralf Baechle, devicetree,
Alexey Malahov, linux-mips
In-Reply-To: <20200514150943.GA30404@bogus>
On Thu, May 14, 2020 at 10:09:43AM -0500, Rob Herring wrote:
> On Wed, 6 May 2020 20:42:20 +0300, wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >
> > It's a Common Device Memory Map controller embedded into the MIPS IP
> > cores, which dts node is supposed to have compatible and reg properties.
> >
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: linux-mips@vger.kernel.org
> > Cc: linux-pm@vger.kernel.org
> >
> > ---
> >
> > Changelog v2:
> > - Lowercase the example hex'es.
> > ---
> > .../bindings/bus/mti,mips-cdmm.yaml | 35 +++++++++++++++++++
> > 1 file changed, 35 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/bus/mti,mips-cdmm.yaml
> >
>
> Reviewed-by: Rob Herring <robh@kernel.org>
Great! Thanks.
-Sergey
^ permalink raw reply
* Re: [PATCH v2 01/20] dt-bindings: power: Convert mti,mips-cpc to DT schema
From: Serge Semin @ 2020-05-14 18:04 UTC (permalink / raw)
To: Rob Herring
Cc: Serge Semin, devicetree, Alexey Malahov, Thomas Bogendoerfer,
linux-kernel, Paul Burton, linux-mips, Arnd Bergmann, Rob Herring,
linux-pm, Ralf Baechle
In-Reply-To: <20200514150903.GA28744@bogus>
On Thu, May 14, 2020 at 10:09:03AM -0500, Rob Herring wrote:
> On Wed, 6 May 2020 20:42:19 +0300, wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >
> > It's a Cluster Power Controller embedded into the MIPS IP cores.
> > Currently the corresponding dts node is supposed to have compatible
> > and reg properties.
> >
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: linux-pm@vger.kernel.org
> >
> > ---
> >
> > Changelog v2:
> > - Reword the changelog summary - use shorter version.
> > - Lowercase the example hex'es.
> > ---
> > .../bindings/power/mti,mips-cpc.txt | 8 -----
> > .../bindings/power/mti,mips-cpc.yaml | 35 +++++++++++++++++++
> > 2 files changed, 35 insertions(+), 8 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/power/mti,mips-cpc.txt
> > create mode 100644 Documentation/devicetree/bindings/power/mti,mips-cpc.yaml
> >
>
> Reviewed-by: Rob Herring <robh@kernel.org>
Great! Thanks.
-Sergey
^ permalink raw reply
* Re: [PATCH v4 06/10] dt-bindings: mtd: update STM32 FMC2 NAND controller documentation
From: Rob Herring @ 2020-05-14 17:55 UTC (permalink / raw)
To: Christophe Kerello
Cc: Miquèl Raynal, Richard Weinberger, Vignesh R, Mark Rutland,
Greg Kroah-Hartman, Boris Brezillon, MTD Maling List,
linux-kernel@vger.kernel.org, linux-stm32, devicetree,
Marek Vašut
In-Reply-To: <9ffc04cf-137f-5ee5-57ff-39a876abfb34@st.com>
On Thu, May 14, 2020 at 11:35 AM Christophe Kerello
<christophe.kerello@st.com> wrote:
>
> Hi Rob,
>
> On 5/14/20 5:00 PM, Rob Herring wrote:
> > On Wed, May 06, 2020 at 11:11:15AM +0200, Christophe Kerello wrote:
> >> These bindings can be used on SOCs where the FMC2 NAND controller is
> >> in standalone. In case that the FMC2 embeds 2 controllers (an external
> >> bus controller and a raw NAND controller), the register base and the
> >> clock will be defined in the parent node. It is the reason why the
> >> register base address and the clock are now optional.
> >>
> >> Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
> >> ---
> >> .../devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml | 19 ++++++++++---------
> >> 1 file changed, 10 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml b/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
> >> index b059267..68fac1a 100644
> >> --- a/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
> >> +++ b/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
> >> @@ -18,13 +18,15 @@ properties:
> >>
> >> reg:
> >> items:
> >> - - description: Registers
> >> + - description: Registers (optional)
> >
> > The only thing that can be optional are the last entries. You have to do
> > a 'oneOf' with 6 entries and 7 entries.
>
> Ok, so the way to describe the reg property in my case should be:
> reg:
> oneOf:
> - description: FMC2 embeds the NFC controller in standalone.
> items:
> - description: Registers
> - description: Chip select 0 data
> - description: Chip select 0 command
> - description: Chip select 0 address space
> - description: Chip select 1 data
> - description: Chip select 1 command
> - description: Chip select 1 address space
>
> - description: FMC2 embeds the NFC controller and the EBI
> controller.
> items:
> - description: Chip select 0 data
> - description: Chip select 0 command
> - description: Chip select 0 address space
> - description: Chip select 1 data
> - description: Chip select 1 command
> - description: Chip select 1 address space
>
> >
> > And where's your new compatible string for this different h/w?
>
> From NFC controller point of view, it is the same HW.
That's what everyone says until they have some quirk or integration
difference to handle.
> In the case that we have 2 controllers embedded, the register base is
> shared.
> The NFC driver will check at probe time the compatible string of its
> parent node.
> In case that it is "st,stm32mp1-fmc2-ebi", then the driver will find the
> register base in the parent node (EBI node), otherwise it will find it
> in the NFC node.
> Is it better to have 2 compatible strings (one for each reg description)
> than checking the parent's compatible string and have only one
> compatible string?
Why not just put the register base into the child node too? While
overlapping 'reg' regions for siblings is bad, it's fine for child
nodes. I guess since there are chip selects for the child nodes that
may not work here.
It doesn't hurt to have another compatible. You can always make the
old one a fallback. With different compatibles you can make sure reg
has the right number of entries.
Rob
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox