* Re: [PATCH] dt-bindings: pinctrl: stm32: Fix missing 'clocks' property in examples
From: Linus Walleij @ 2019-08-02 22:31 UTC (permalink / raw)
To: Rob Herring
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, linux-kernel@vger.kernel.org, Maxime Coquelin,
Alexandre Torgue, open list:GPIO SUBSYSTEM, linux-stm32
In-Reply-To: <20190716215618.29757-1-robh@kernel.org>
On Tue, Jul 16, 2019 at 11:56 PM Rob Herring <robh@kernel.org> wrote:
> Now that examples are validated against the DT schema, an error with
> required 'clocks' property missing is exposed:
>
> Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.example.dt.yaml: \
> pinctrl@40020000: gpio@0: 'clocks' is a required property
> Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.example.dt.yaml: \
> pinctrl@50020000: gpio@1000: 'clocks' is a required property
> Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.example.dt.yaml: \
> pinctrl@50020000: gpio@2000: 'clocks' is a required property
>
> Add the missing 'clocks' properties to the examples to fix the errors.
>
> Fixes: 2c9239c125f0 ("dt-bindings: pinctrl: Convert stm32 pinctrl bindings to json-schema")
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: linux-gpio@vger.kernel.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Signed-off-by: Rob Herring <robh@kernel.org>
This seems to already be upstream, but I have no memory of applying it.
Less work for me :)
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 2/2] gpio: mpc8xxx: Add ls1028a device specify function.
From: Linus Walleij @ 2019-08-02 22:19 UTC (permalink / raw)
To: Hui Song
Cc: Shawn Guo, Li Yang, Rob Herring, Mark Rutland,
Bartosz Golaszewski, Linux ARM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org, open list:GPIO SUBSYSTEM
In-Reply-To: <20190718094902.15562-2-hui.song_1@nxp.com>
On Thu, Jul 18, 2019 at 11:58 AM Hui Song <hui.song_1@nxp.com> wrote:
> From: Song Hui <hui.song_1@nxp.com>
>
> There is a device specify register(named GPIO_IBE)
> on ls1028a need to enable in initial stage.
>
> Signed-off-by: Song Hui <hui.song_1@nxp.com>
Patch applied.
As noted on patch 1/2, send a separate patch to add the
device tree bindings in Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2 4/6] irqchip/irq-pruss-intc: Add helper functions to configure internal mapping
From: Suman Anna @ 2019-08-02 21:26 UTC (permalink / raw)
To: David Lechner, Marc Zyngier, Thomas Gleixner, Jason Cooper
Cc: Rob Herring, Tony Lindgren, Andrew F. Davis, Roger Quadros,
Lokesh Vutla, Grygorii Strashko, Sekhar Nori, Murali Karicheri,
devicetree, linux-omap, linux-arm-kernel, linux-kernel
In-Reply-To: <1ac233f6-f3a3-6cec-9ad2-49e985fdfaca@lechnology.com>
Hi David,
On 8/1/19 1:31 PM, David Lechner wrote:
> On 8/1/19 12:10 PM, Suman Anna wrote:
>> Hi Marc,
>>
>> On 8/1/19 3:45 AM, Marc Zyngier wrote:
>>> On 31/07/2019 23:41, Suman Anna wrote:
>>>> The PRUSS INTC receives a number of system input interrupt
>>>> source events and supports individual control configuration and
>>>> hardware prioritization. These input events can be mapped to
>>>> some output interrupt lines through 2 levels of many-to-one
>>>> mapping i.e. events to channel mapping and channels to output
>>>> interrupts.
>>>>
>>>> This mapping information is provided through the PRU firmware
>>>> that is loaded onto a PRU core/s or through the device tree
>>>> node of the PRU application. The mapping is configured by the
>>>> PRU remoteproc driver, and is setup before the PRU core is
>>>> started and cleaned up after the PRU core is stopped. This
>>>> event mapping configuration logic programs the Channel Map
>>>> Registers (CMRx) and Host-Interrupt Map Registers (HMRx) only
>>>> when a new program is being loaded/started and the same events
>>>> and interrupt channels are reset to zero when stopping a PRU.
>>>>
>>>> Add two helper functions: pruss_intc_configure() &
>>>> pruss_intc_unconfigure() that the PRU remoteproc driver can use
>>>> to configure the PRUSS INTC.
>>>
>>> So let me see if I correctly understand this: this adds yet
>>> another firmware description parser, with a private interface to
>>> another (undisclosed?) driver, bypassing the standard irqchip
>>> configuration mechanism. It sounds great, doesn't it?
>>>
>>> What I cannot really infer from this message (-ETOOMUCHJARGON) is
>>> what interrupts this affects:
>>>
>>> - Interrupts from random devices to the PRUSS? - Interrupts from
>>> the PRUSS to the host? - Something else?
>>
>> The interrupt sources (called system events) can be from internal
>> PRUSS peripherals, SoC-level peripherals or just software
>> triggering (limited to some events).
>>
>> So, the PRUSS INTC behaves as a funnel and is both an interrupt
>> router and multiplexer. The INTC itself is part of the PRUSS, and
>> all PRU application related interrupts/events that need to trigger
>> an interrupt to either the PRU cores or other host processors (like
>> DSP, ARM) have to go through this INTC, and routed out to a limited
>> number of output interrupts that are then connected to different
>> processors.
>>
>> The split of interrupt handling between a PRU and its peer host
>> processor will be a application design choice (We can implement
>> soft IPs like UARTs, ADCs, I2Cs etc using PRUs). Some of the input
>> events themselves are multiplexed and controlled by a single MMR
>> (outside of INTC) that feeds different sets of events into the
>> INTC. The MMR configuration is outside of scope of this driver and
>> will depend on the application/client driver being run.
>>
>>>
>>> When does this happen? Under control of what? It isn't even clear
>>> why this is part of this irqchip driver.
>>
>> The mapping configuration is per PRU application and firmware, and
>> is done in line with acquiring and release a PRU which is treated
>> as an exclusive resource. We establish the mapping for all events
>> through this driver including the events that are to be routed to
>> PRUs. This is done to save the tiny/limited Instruction RAM space
>> that PRUs have.
>>
>> We have designed this as an irqchip driver (instead of some custom
>> SoC driver exporting custom functions) to use standard Linux
>> semantics/irq API and better integrate with Linux DT, but we need
>> some semantics for establishing the routing at runtime depending on
>> the PRU client driver we are running. The exported functions will
>> be called only by the PRU remoteproc driver during a
>> pru_rproc_get()/pru_rproc_put(), and are transparent to PRU client
>> drivers.
>>
>> Please also see the discussion from v1 [1] on why we can't use an
>> extended number of interrupt-cells infrastructure for achieving
>> this.
>>
>> [1] https://patchwork.kernel.org/patch/11034563/
>>
>>
>>> Depending what this does, there may be ways to fit it into the
>>> standard interrupt configuration framework. After all, we already
>>> have standard interfaces to route interrupts to virtual CPUs,
>>> effectively passing full control of an interrupt to another
>>> entity. If you squint hard enough, your PRUSS can fit that
>>> description.
>>
>> Yeah, I am open to suggestions if there is a better way of doing
>> this.
>
> Hi Suman,
>
> Can you explain more about the use case where one PRU system event
> is mapped to multiple host events?
On AM335x, for example, we have 64 events out of which events 16 to 31
are not sourced by any peripherals and are used for general purpose
signaling between a PRU0/PRU1 core and any external host processor (like
an ARM). So, different applications/drivers implementing a different
soft IP like a Soft UART, Soft I2C, ADC etc will need to be using among
these generic set to achieve their various interrupts / signaling logic
between the corresponding ARM driver and the PRU firmware implementing
the soft IP.
Following are some existing usage examples on AM335x within TI SDKs
(tuples of <system_event intr_channel output_interrupt>
1. A Soft UART implementing 3 ports per PRU:
PRU0: <21, 2, 2>, <22, 3, 3>, <23, 4, 4>
PRU1: <24, 5, 5>, <25, 6, 6>, <26, 7, 7>;
2. A Dual-EMAC PRU Ethernet usecase using one PRU per ethernet port
utilizing the MDIO, MII_TI sub-modules within PRUSS:
PRU0: {42, 0, 0}, {20, 2, 2}, {22, 4, 4}, {26, 6, 6}, {41, 7, 8},
PRU1: {54, 1, 1}, {21, 3, 3}, {23, 5, 5}, {53, 8, 9}, {27, 9, 7},
Some of the above PRU Ethernet ones are generic events while the others
are tied to specific MII_RT interrupt events. A different mapping is
used when both the ethernet ports and PRUs are being used to achieve a
Switch functionality.
Point is different applications might use mapping differently as per
their firmware and driver/application design and their split across one
or more PRUs (design by contract). And we need to set this up at runtime
when the application driver is getting run. We will have either the Soft
UART or the Ethernet running at a time depending on the end goal desired
> I have an idea that we can use multiple struct irq_domains to make
> this work in the existing IRQ framework, but it would be helpful to
> know more about the bigger picture first.
Yeah, would be great if there is a way this can be solved without having
to introduce additional API.
regards
Suman
>
>>
>> regards Suman
>>
>>>
>>> If that doesn't work, then we need to make the IRQ framework grok
>>> that kind of requirement (hence my request for clarification).
>>> But I'm strongly opposed to inventing a SoC-private way of
>>> configuring interrupts behind the kernel's back.
>>>
>>> Thanks,
>>>
>>> M.
>>>
>>
>
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni @ 2019-08-02 21:18 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <73cd521b-782c-7fb2-d904-ae8b07927d47@gmail.com>
On 8/2/19 2:15 PM, Dmitry Osipenko wrote:
> 02.08.2019 23:32, Sowjanya Komatineni пишет:
>> On 8/2/19 1:17 PM, Dmitry Osipenko wrote:
>>> 02.08.2019 23:13, Dmitry Osipenko пишет:
>>>> 02.08.2019 21:33, Sowjanya Komatineni пишет:
>>>>> On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
>>>>>> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>>>>>>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>>>>>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>>>>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>>>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>> This patch implements save and restore context for
>>>>>>>>>>>>>>>>>>> peripheral
>>>>>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock
>>>>>>>>>>>>>>>>>>> ops, and
>>>>>>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>>>>>>> and on resume they are restored to have clocks back to
>>>>>>>>>>>>>>>>>>> same
>>>>>>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni
>>>>>>>>>>>>>>>>>>> <skomatineni@nvidia.com>
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>>> @@ -60,11 +60,44 @@
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>> +static int
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +static void
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> static const struct clk_ops
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>>>>>>> .is_enabled =
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>>>>>>> .recalc_rate =
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>>> struct clk
>>>>>>>>>>>>>>>>>>> *tegra_clk_register_periph_fixed(const
>>>>>>>>>>>>>>>>>>> char
>>>>>>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static
>>>>>>>>>>>>>>>>>>> DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>>>>>>> readl_relaxed(gate->clk_base +
>>>>>>>>>>>>>>>>>>> (gate->regs->rst_reg))
>>>>>>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct
>>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct
>>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>> +static int clk_periph_save_context(struct
>>>>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>>>>>>> Could you please point to where the divider's
>>>>>>>>>>>>>>>>>> save_context()
>>>>>>>>>>>>>>>>>> happens?
>>>>>>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers
>>>>>>>>>>>>>>>>> context
>>>>>>>>>>>>>>>>> because
>>>>>>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>>>>>>> restoring
>>>>>>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>>>>>>> generic helper to get the index instead of storing it
>>>>>>>>>>>>>>>>> manually.
>>>>>>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> All existing drivers are using directly get_parent() from
>>>>>>>>>>>>>>>> clk_mux
>>>>>>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> To have this more generic w.r.t save/restore context
>>>>>>>>>>>>>>>> point of
>>>>>>>>>>>>>>>> view,
>>>>>>>>>>>>>>>> probably instead of implementing new get_parent_index
>>>>>>>>>>>>>>>> helper,
>>>>>>>>>>>>>>>> I think
>>>>>>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>>>>>>> clk_mux to
>>>>>>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>>>>>>> restore_context.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops
>>>>>>>>>>>>>>> to be
>>>>>>>>>>>>>>> more
>>>>>>>>>>>>>>> generic w.r.t save/restore context rather than
>>>>>>>>>>>>>>> get_parent_index
>>>>>>>>>>>>>>> API.
>>>>>>>>>>>>>>> Please confirm if you agree.
>>>>>>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>>>>>>> helper for
>>>>>>>>>>>>>> the generic clk_gate, seems something similar could be done
>>>>>>>>>>>>>> for the
>>>>>>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the
>>>>>>>>>>>>>> parent
>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>>>>>>> by 'restoring' helper for generic clk_gate, are you
>>>>>>>>>>>>> referring to
>>>>>>>>>>>>> clk_gate_restore_context API?
>>>>>>>>>>>> Yes.
>>>>>>>>>>>>
>>>>>>>>>>>>> clk_gate_restore_context is API that's any clk drivers can
>>>>>>>>>>>>> use for
>>>>>>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>>>>>>
>>>>>>>>>>>>> But clk-periph is directly using generic clk_mux ops from
>>>>>>>>>>>>> clk_mux
>>>>>>>>>>>>> so I
>>>>>>>>>>>>> think we should add .restore_context to clk_mux_ops and then
>>>>>>>>>>>>> during
>>>>>>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>>>>>>> generic
>>>>>>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>>>>>>> function
>>>>>>>>>>>> that any driver could use in order to restore the clock's
>>>>>>>>>>>> parent.
>>>>>>>>>>>>
>>>>>>>>>>>> The clk-periph restoring also includes case of combining divider
>>>>>>>>>>>> and
>>>>>>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>>>>>>> as well.
>>>>>>>>>>>>
>>>>>>>>>>>> It also looks like you could actually use the
>>>>>>>>>>>> clk_gate_restore_context()
>>>>>>>>>>>> instead of manually saving the clock's enable-state, couldn't
>>>>>>>>>>>> you?
>>>>>>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API
>>>>>>>>>>> rather
>>>>>>>>>>> than
>>>>>>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>>>>>>> clk_periph
>>>>>>>>>>> restore.
>>>>>>>>>>>
>>>>>>> digging thru looks like for clk_periph source restore instead of
>>>>>>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>>>>>>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent
>>>>>>> anyway.
>>>>>>> Will do this for periph clk mux
>>>>>>>>>>> Reg clk_gate, looks like we cant use generic
>>>>>>>>>>> clk_gate_restore_context
>>>>>>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>>>>>>> clk_periph_enable/disable in clk-periph-gate also updated
>>>>>>>>>>> refcnt and
>>>>>>>>>>> depending on that actual enable/disable is set.
>>>>>>>>>>>
>>>>>>>>>>> During suspend, peripherals that are already enabled have their
>>>>>>>>>>> refcnt >
>>>>>>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>>>>>>> enable/disable callback.
>>>>>>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>>>>>>> save_context, wouldn't that work?
>>>>>>>>>>
>>>>>>>> gate->enable_refcnt is within clk-periph-gate which gets updated
>>>>>>>> when
>>>>>>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>>>>>>> But actual enable_count used in clk_gate_restore_context is the one
>>>>>>>> which gets updated with in the clk core enable/disable functions
>>>>>>>> which
>>>>>>>> invokes these callbacks. Depending on this enable_count in clk
>>>>>>>> core it
>>>>>>>> invokes enable/disable.
>>>>>>>>
>>>>>>>> So, this will cause mismatch if we handle refcnt during save/restore
>>>>>>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>>>>>>> clk_gate_restore_context is based on enable_count from clk core.
>>>>>>>>
>>>>>>>>>>> Also to align exact reset state along with CLK (like for case
>>>>>>>>>>> where
>>>>>>>>>>> CLK
>>>>>>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>>>>>>> I'm wondering whether instead of saving/restoring reset-state of
>>>>>>>>>> every
>>>>>>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET
>>>>>>>>>> register.
>>>>>>>>>> Couldn't you?
>>>>>>>>> Thats what I was doing in first version of patch. But later as we
>>>>>>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>>>>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>>>>>>> after source restore
>>>>>>>> Also, to align both CLK & RST to the exact state of register, doing
>>>>>>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>>>>>>> source restore for peripheral clock, seems cleaner to avoid any
>>>>>>>> misconfiguration b/w rst & clk settings.
>>>>>>>>
>>>>>> It looks to me that it is very wasteful to store/restore each
>>>>>> individual
>>>>>> gate and reset state, also given that some of them are shared. I think
>>>>>> that the gates and resets should be restored separately for the
>>>>>> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
>>>>> clk_periph_fixed_disable just disables clock only without deasserting
>>>>> the corresponding peripheral.
>>>>>
>>>>> corresponding peripheral drivers can also issue reset assert/deassert
>>>>> thru reset_control_assert/deassert.
>>>>>
>>>>> So, we will not get the actual state of clk and rst unless we read and
>>>>> save state of reset and clock separately during save_context.
>>>>>
>>>>> Currently patch is already using custom
>>>>> tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
>>>>> Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
>>>>> register settings instead of individual peripheral bits?
>>>> Yes, I'm suggesting to do a complete ungate/reset handling of the
>>>> devices in a separate function. All enabling/deassertion will be done in
>>>> a single hop, hence using 7us delay and four u32 words, which is much
>>>> nicer IMHO.
>>> Actually six words, three for CLKs and three for RSTs.
>> OK, So with separate function doing complete register save/restore for
>> clk & rst, we can't do this thru clk_ops save/restore as clk_ops
>> save_restore happens per peripheral wise. So if we decide to do this,
>> then this should be invoked in clk-tegra210 driver suspend/resume.
> Yes, per-clock save/restore should be used for setting rate and parent.
> The ungating and resetting could be done separately to keep things cleaner.
>
OK, Will move back to register wise save/restore for clk_enb/rst_dev in
next version.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Dmitry Osipenko @ 2019-08-02 21:15 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <c6e1d744-3a7a-fe1b-2c86-a3d49f022232@nvidia.com>
02.08.2019 23:32, Sowjanya Komatineni пишет:
>
> On 8/2/19 1:17 PM, Dmitry Osipenko wrote:
>> 02.08.2019 23:13, Dmitry Osipenko пишет:
>>> 02.08.2019 21:33, Sowjanya Komatineni пишет:
>>>> On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
>>>>> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>>>>>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>>>>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>>>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>> This patch implements save and restore context for
>>>>>>>>>>>>>>>>>> peripheral
>>>>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock
>>>>>>>>>>>>>>>>>> ops, and
>>>>>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>>>>>> and on resume they are restored to have clocks back to
>>>>>>>>>>>>>>>>>> same
>>>>>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni
>>>>>>>>>>>>>>>>>> <skomatineni@nvidia.com>
>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>> @@ -60,11 +60,44 @@
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>> +static int
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +static void
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> static const struct clk_ops
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>>>>>> .is_enabled =
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>>>>>> .recalc_rate =
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>> struct clk
>>>>>>>>>>>>>>>>>> *tegra_clk_register_periph_fixed(const
>>>>>>>>>>>>>>>>>> char
>>>>>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static
>>>>>>>>>>>>>>>>>> DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>>>>>> readl_relaxed(gate->clk_base +
>>>>>>>>>>>>>>>>>> (gate->regs->rst_reg))
>>>>>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct
>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct
>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>> +static int clk_periph_save_context(struct
>>>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>>>>>> Could you please point to where the divider's
>>>>>>>>>>>>>>>>> save_context()
>>>>>>>>>>>>>>>>> happens?
>>>>>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers
>>>>>>>>>>>>>>>> context
>>>>>>>>>>>>>>>> because
>>>>>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>>>>>> restoring
>>>>>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>>>>>> generic helper to get the index instead of storing it
>>>>>>>>>>>>>>>> manually.
>>>>>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> All existing drivers are using directly get_parent() from
>>>>>>>>>>>>>>> clk_mux
>>>>>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> To have this more generic w.r.t save/restore context
>>>>>>>>>>>>>>> point of
>>>>>>>>>>>>>>> view,
>>>>>>>>>>>>>>> probably instead of implementing new get_parent_index
>>>>>>>>>>>>>>> helper,
>>>>>>>>>>>>>>> I think
>>>>>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>>>>>> clk_mux to
>>>>>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>>>>>> restore_context.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops
>>>>>>>>>>>>>> to be
>>>>>>>>>>>>>> more
>>>>>>>>>>>>>> generic w.r.t save/restore context rather than
>>>>>>>>>>>>>> get_parent_index
>>>>>>>>>>>>>> API.
>>>>>>>>>>>>>> Please confirm if you agree.
>>>>>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>>>>>> helper for
>>>>>>>>>>>>> the generic clk_gate, seems something similar could be done
>>>>>>>>>>>>> for the
>>>>>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the
>>>>>>>>>>>>> parent
>>>>>>>>>>>>> clock
>>>>>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>>>>>> by 'restoring' helper for generic clk_gate, are you
>>>>>>>>>>>> referring to
>>>>>>>>>>>> clk_gate_restore_context API?
>>>>>>>>>>> Yes.
>>>>>>>>>>>
>>>>>>>>>>>> clk_gate_restore_context is API that's any clk drivers can
>>>>>>>>>>>> use for
>>>>>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>>>>>
>>>>>>>>>>>> But clk-periph is directly using generic clk_mux ops from
>>>>>>>>>>>> clk_mux
>>>>>>>>>>>> so I
>>>>>>>>>>>> think we should add .restore_context to clk_mux_ops and then
>>>>>>>>>>>> during
>>>>>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>>>>>> generic
>>>>>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>>>>>> function
>>>>>>>>>>> that any driver could use in order to restore the clock's
>>>>>>>>>>> parent.
>>>>>>>>>>>
>>>>>>>>>>> The clk-periph restoring also includes case of combining divider
>>>>>>>>>>> and
>>>>>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>>>>>> as well.
>>>>>>>>>>>
>>>>>>>>>>> It also looks like you could actually use the
>>>>>>>>>>> clk_gate_restore_context()
>>>>>>>>>>> instead of manually saving the clock's enable-state, couldn't
>>>>>>>>>>> you?
>>>>>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API
>>>>>>>>>> rather
>>>>>>>>>> than
>>>>>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>>>>>> clk_periph
>>>>>>>>>> restore.
>>>>>>>>>>
>>>>>> digging thru looks like for clk_periph source restore instead of
>>>>>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>>>>>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent
>>>>>> anyway.
>>>>>> Will do this for periph clk mux
>>>>>>>>>> Reg clk_gate, looks like we cant use generic
>>>>>>>>>> clk_gate_restore_context
>>>>>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>>>>>> clk_periph_enable/disable in clk-periph-gate also updated
>>>>>>>>>> refcnt and
>>>>>>>>>> depending on that actual enable/disable is set.
>>>>>>>>>>
>>>>>>>>>> During suspend, peripherals that are already enabled have their
>>>>>>>>>> refcnt >
>>>>>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>>>>>> enable/disable callback.
>>>>>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>>>>>> save_context, wouldn't that work?
>>>>>>>>>
>>>>>>> gate->enable_refcnt is within clk-periph-gate which gets updated
>>>>>>> when
>>>>>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>>>>>> But actual enable_count used in clk_gate_restore_context is the one
>>>>>>> which gets updated with in the clk core enable/disable functions
>>>>>>> which
>>>>>>> invokes these callbacks. Depending on this enable_count in clk
>>>>>>> core it
>>>>>>> invokes enable/disable.
>>>>>>>
>>>>>>> So, this will cause mismatch if we handle refcnt during save/restore
>>>>>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>>>>>> clk_gate_restore_context is based on enable_count from clk core.
>>>>>>>
>>>>>>>>>> Also to align exact reset state along with CLK (like for case
>>>>>>>>>> where
>>>>>>>>>> CLK
>>>>>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>>>>>> I'm wondering whether instead of saving/restoring reset-state of
>>>>>>>>> every
>>>>>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET
>>>>>>>>> register.
>>>>>>>>> Couldn't you?
>>>>>>>> Thats what I was doing in first version of patch. But later as we
>>>>>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>>>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>>>>>> after source restore
>>>>>>> Also, to align both CLK & RST to the exact state of register, doing
>>>>>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>>>>>> source restore for peripheral clock, seems cleaner to avoid any
>>>>>>> misconfiguration b/w rst & clk settings.
>>>>>>>
>>>>> It looks to me that it is very wasteful to store/restore each
>>>>> individual
>>>>> gate and reset state, also given that some of them are shared. I think
>>>>> that the gates and resets should be restored separately for the
>>>>> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
>>>> clk_periph_fixed_disable just disables clock only without deasserting
>>>> the corresponding peripheral.
>>>>
>>>> corresponding peripheral drivers can also issue reset assert/deassert
>>>> thru reset_control_assert/deassert.
>>>>
>>>> So, we will not get the actual state of clk and rst unless we read and
>>>> save state of reset and clock separately during save_context.
>>>>
>>>> Currently patch is already using custom
>>>> tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
>>>> Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
>>>> register settings instead of individual peripheral bits?
>>> Yes, I'm suggesting to do a complete ungate/reset handling of the
>>> devices in a separate function. All enabling/deassertion will be done in
>>> a single hop, hence using 7us delay and four u32 words, which is much
>>> nicer IMHO.
>> Actually six words, three for CLKs and three for RSTs.
>
> OK, So with separate function doing complete register save/restore for
> clk & rst, we can't do this thru clk_ops save/restore as clk_ops
> save_restore happens per peripheral wise. So if we decide to do this,
> then this should be invoked in clk-tegra210 driver suspend/resume.
Yes, per-clock save/restore should be used for setting rate and parent.
The ungating and resetting could be done separately to keep things cleaner.
^ permalink raw reply
* Re: [PATCH V6 14/21] clk: tegra210: Add suspend and resume support
From: Sowjanya Komatineni @ 2019-08-02 20:39 UTC (permalink / raw)
To: Stephen Boyd, Dmitry Osipenko, jason, jonathanh, linus.walleij,
marc.zyngier, mark.rutland, stefan, tglx, thierry.reding
Cc: pdeschrijver, pgaikwad, linux-clk, linux-gpio, jckuo, josephl,
talho, linux-tegra, linux-kernel, mperttunen, spatra, robh+dt,
devicetree
In-Reply-To: <20190802175119.1E401217F5@mail.kernel.org>
On 8/2/19 10:51 AM, Stephen Boyd wrote:
> Quoting Dmitry Osipenko (2019-07-22 00:12:17)
>> 22.07.2019 10:09, Dmitry Osipenko пишет:
>>> 22.07.2019 9:52, Sowjanya Komatineni пишет:
>>>> On 7/21/19 11:10 PM, Dmitry Osipenko wrote:
>>>>> 22.07.2019 1:45, Sowjanya Komatineni пишет:
>>>>>> On 7/21/19 2:38 PM, Dmitry Osipenko wrote:
>>>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>>>>> @@ -2853,9 +2859,8 @@ static int tegra210_enable_pllu(void)
>>>>>>>> reg |= PLL_ENABLE;
>>>>>>>> writel(reg, clk_base + PLLU_BASE);
>>>>>>>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
>>>>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
>>>>>>>> - if (!(reg & PLL_BASE_LOCK)) {
>>>>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
>>>>>>>> + if (ret) {
>>>>>>> Why this is needed? Was there a bug?
>>>>>>>
>>>>>> during resume pllu init is needed and to use same terga210_init_pllu,
>>>>>> poll_timeout_atomic can't be used as its ony for atomic context.
>>>>>>
>>>>>> So changed to use wait_for_mask which should work in both cases.
>>>>> Atomic variant could be used from any context, not sure what do you
>>>>> mean. The 'atomic' part only means that function won't cause scheduling
>>>>> and that's it.
>>>> Sorry, replied incorrect. readx_poll_timeout_atomic uses ktime_get() and
>>>> during resume timekeeping suspend/resume happens later than clock
>>>> suspend/resume. So using tegra210_wait_for_mask.
>>>>
>>>> both timekeeping and clk-tegra210 drivers are registered as syscore but
>>>> not ordered.
>>> Okay, thank you for the clarification.
>>>
>>> [snip]
>>>
>> You should remove the 'iopoll.h' then, since it's not used anymore.
> And also add a comment to this location in the code because it's
> non-obvious that we can't use iopoll here.
>
Actually added comment during function usage instead of during include
as iopoll.h is removed.
Will add additional comment in include section as well highlighting
reason for removal of iopoll.h
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni @ 2019-08-02 20:37 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <92e95688-1984-9967-d47c-57380466a0f2@gmail.com>
On 8/2/19 1:20 PM, Dmitry Osipenko wrote:
> 02.08.2019 21:43, Sowjanya Komatineni пишет:
>> On 8/2/19 5:32 AM, Dmitry Osipenko wrote:
>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>> This patch implements save and restore context for peripheral fixed
>>>> clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
>>>> peripheral clock ops.
>>>>
>>>> During system suspend, core power goes off and looses the settings
>>>> of the Tegra CAR controller registers.
>>>>
>>>> So during suspend entry clock and reset state of peripherals is saved
>>>> and on resume they are restored to have clocks back to same rate and
>>>> state as before suspend.
>>>>
>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>> ++++++++++++++++++++++++++++++++
>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>> +++++++++++++++++++++++++++++++++
>>>> drivers/clk/tegra/clk-periph.c | 37
>>>> ++++++++++++++++++++++++++++++++++++
>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28 +++++++++++++++++++++++++++
>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>> 5 files changed, 138 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>> index c088e7a280df..21b24530fa00 100644
>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>> @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct clk_hw
>>>> *hw,
>>>> return (unsigned long)rate;
>>>> }
>>>> +static int tegra_clk_periph_fixed_save_context(struct clk_hw *hw)
>>>> +{
>>>> + struct tegra_clk_periph_fixed *fixed =
>>>> to_tegra_clk_periph_fixed(hw);
>>>> + u32 mask = 1 << (fixed->num % 32);
>>> This could be BIT(fixed->num % 32).
>>>
>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>> fixed->regs->enb_reg) &
>>>> + mask;
>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>> fixed->regs->rst_reg) &
>>>> + mask;
>>> The enb_ctx/rst_ctx are booleans, while you assigning an integer value
>>> here. You're getting away here because bool is an 32bit unsigned int,
>>> but you shouldn't rely on it and always explicitly convert to a bool.
>>>
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void tegra_clk_periph_fixed_restore_context(struct clk_hw *hw)
>>>> +{
>>>> + struct tegra_clk_periph_fixed *fixed =
>>>> to_tegra_clk_periph_fixed(hw);
>>>> + u32 mask = 1 << (fixed->num % 32);
>>>> +
>>>> + if (fixed->enb_ctx)
>>>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_set_reg);
>>>> + else
>>>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_clr_reg);
>>>> +
>>>> + udelay(2);
>>> Will be better to read out and compare the hardware's state with the
>>> restored one, then bail out if the state is unchanged.
>>>
>>> Shouldn't it be fence_udelay()?
>>>
>>>> + if (!fixed->rst_ctx) {
>>>> + udelay(5); /* reset propogation delay */
>>> Why delaying is done before the writing to the reset register?
>> During SC7 exit, peripheral reset state is set to POR state. So some
>> peripherals will already be in reset state and making sure of
>> propagation delay before releasing from reset.
>>
>> It should be rst_clr_reg. will fix in next rev
>>
>>>> + writel_relaxed(mask, fixed->base + fixed->regs->rst_reg);
>>> I'm not quite sure what's going on here, this looks wrong.
>>>
>>> 1. rst_reg points to RST_DEVICES_x
>>> 2. Each bit of RST_DEVICES_x represents the reset-assertion state of
>>> each individual device
>>> 3. By writing to rst_reg, all (!) devices are deasserted, except the one
>>> device which corresponds to the mask
>>> 4. The reset is asserted for a single device, while !fixed->rst_ctx
>>> means that it actually should be deasserted (?)
>>>
>>> Apparently you should use rst_set_reg / rst_clr_reg.
>> Yes, It should be rst_clr_reg. will fix in next rev
>>>> + }
>>> What about the case where rst_ctx=true?
>> ON SC7 exit, state of RST_DEV will be POR state where most peripherals
>> will already be in reset state.
>>
>> Few of them which are not in reset state in POR values are those that
>> need to stay de-asserted across the boot states anyway.
> Okay, sounds reasonable.
>
> BTW, it would be nice if you could add a brief clarifying comment to the
> code for each of the questions asked during of the review.
OK, Will add comments in code ...
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni @ 2019-08-02 20:32 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <90268663-e5a7-4715-bd1a-31644c2fe9ab@gmail.com>
On 8/2/19 1:17 PM, Dmitry Osipenko wrote:
> 02.08.2019 23:13, Dmitry Osipenko пишет:
>> 02.08.2019 21:33, Sowjanya Komatineni пишет:
>>> On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
>>>> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>>>>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>>>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>> This patch implements save and restore context for
>>>>>>>>>>>>>>>>> peripheral
>>>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock
>>>>>>>>>>>>>>>>> ops, and
>>>>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>>>>> and on resume they are restored to have clocks back to same
>>>>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>> @@ -60,11 +60,44 @@
>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>> +static int tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +static void tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> static const struct clk_ops tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>>>>> .is_enabled = tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>>>>> .recalc_rate = tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_fixed(const
>>>>>>>>>>>>>>>>> char
>>>>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>>>>> readl_relaxed(gate->clk_base +
>>>>>>>>>>>>>>>>> (gate->regs->rst_reg))
>>>>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct clk_hw
>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct clk_hw
>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>> +static int clk_periph_save_context(struct clk_hw *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>>>>> Could you please point to where the divider's save_context()
>>>>>>>>>>>>>>>> happens?
>>>>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers
>>>>>>>>>>>>>>> context
>>>>>>>>>>>>>>> because
>>>>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>>>>> restoring
>>>>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>>>>> generic helper to get the index instead of storing it
>>>>>>>>>>>>>>> manually.
>>>>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> All existing drivers are using directly get_parent() from
>>>>>>>>>>>>>> clk_mux
>>>>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> To have this more generic w.r.t save/restore context point of
>>>>>>>>>>>>>> view,
>>>>>>>>>>>>>> probably instead of implementing new get_parent_index helper,
>>>>>>>>>>>>>> I think
>>>>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>>>>> clk_mux to
>>>>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>>>>> restore_context.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops to be
>>>>>>>>>>>>> more
>>>>>>>>>>>>> generic w.r.t save/restore context rather than get_parent_index
>>>>>>>>>>>>> API.
>>>>>>>>>>>>> Please confirm if you agree.
>>>>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>>>>> helper for
>>>>>>>>>>>> the generic clk_gate, seems something similar could be done
>>>>>>>>>>>> for the
>>>>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the
>>>>>>>>>>>> parent
>>>>>>>>>>>> clock
>>>>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>>>>> by 'restoring' helper for generic clk_gate, are you referring to
>>>>>>>>>>> clk_gate_restore_context API?
>>>>>>>>>> Yes.
>>>>>>>>>>
>>>>>>>>>>> clk_gate_restore_context is API that's any clk drivers can use for
>>>>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>>>>
>>>>>>>>>>> But clk-periph is directly using generic clk_mux ops from clk_mux
>>>>>>>>>>> so I
>>>>>>>>>>> think we should add .restore_context to clk_mux_ops and then
>>>>>>>>>>> during
>>>>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>>>>> generic
>>>>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>>>>> function
>>>>>>>>>> that any driver could use in order to restore the clock's parent.
>>>>>>>>>>
>>>>>>>>>> The clk-periph restoring also includes case of combining divider
>>>>>>>>>> and
>>>>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>>>>> as well.
>>>>>>>>>>
>>>>>>>>>> It also looks like you could actually use the
>>>>>>>>>> clk_gate_restore_context()
>>>>>>>>>> instead of manually saving the clock's enable-state, couldn't you?
>>>>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API rather
>>>>>>>>> than
>>>>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>>>>> clk_periph
>>>>>>>>> restore.
>>>>>>>>>
>>>>> digging thru looks like for clk_periph source restore instead of
>>>>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>>>>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent anyway.
>>>>> Will do this for periph clk mux
>>>>>>>>> Reg clk_gate, looks like we cant use generic
>>>>>>>>> clk_gate_restore_context
>>>>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>>>>> clk_periph_enable/disable in clk-periph-gate also updated refcnt and
>>>>>>>>> depending on that actual enable/disable is set.
>>>>>>>>>
>>>>>>>>> During suspend, peripherals that are already enabled have their
>>>>>>>>> refcnt >
>>>>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>>>>> enable/disable callback.
>>>>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>>>>> save_context, wouldn't that work?
>>>>>>>>
>>>>>> gate->enable_refcnt is within clk-periph-gate which gets updated when
>>>>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>>>>> But actual enable_count used in clk_gate_restore_context is the one
>>>>>> which gets updated with in the clk core enable/disable functions which
>>>>>> invokes these callbacks. Depending on this enable_count in clk core it
>>>>>> invokes enable/disable.
>>>>>>
>>>>>> So, this will cause mismatch if we handle refcnt during save/restore
>>>>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>>>>> clk_gate_restore_context is based on enable_count from clk core.
>>>>>>
>>>>>>>>> Also to align exact reset state along with CLK (like for case where
>>>>>>>>> CLK
>>>>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>>>>> I'm wondering whether instead of saving/restoring reset-state of
>>>>>>>> every
>>>>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET
>>>>>>>> register.
>>>>>>>> Couldn't you?
>>>>>>> Thats what I was doing in first version of patch. But later as we
>>>>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>>>>> after source restore
>>>>>> Also, to align both CLK & RST to the exact state of register, doing
>>>>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>>>>> source restore for peripheral clock, seems cleaner to avoid any
>>>>>> misconfiguration b/w rst & clk settings.
>>>>>>
>>>> It looks to me that it is very wasteful to store/restore each individual
>>>> gate and reset state, also given that some of them are shared. I think
>>>> that the gates and resets should be restored separately for the
>>>> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
>>> clk_periph_fixed_disable just disables clock only without deasserting
>>> the corresponding peripheral.
>>>
>>> corresponding peripheral drivers can also issue reset assert/deassert
>>> thru reset_control_assert/deassert.
>>>
>>> So, we will not get the actual state of clk and rst unless we read and
>>> save state of reset and clock separately during save_context.
>>>
>>> Currently patch is already using custom
>>> tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
>>> Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
>>> register settings instead of individual peripheral bits?
>> Yes, I'm suggesting to do a complete ungate/reset handling of the
>> devices in a separate function. All enabling/deassertion will be done in
>> a single hop, hence using 7us delay and four u32 words, which is much
>> nicer IMHO.
> Actually six words, three for CLKs and three for RSTs.
OK, So with separate function doing complete register save/restore for
clk & rst, we can't do this thru clk_ops save/restore as clk_ops
save_restore happens per peripheral wise. So if we decide to do this,
then this should be invoked in clk-tegra210 driver suspend/resume.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Dmitry Osipenko @ 2019-08-02 20:20 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <614e3fec-cfa2-9e49-6130-d6de253acf03@nvidia.com>
02.08.2019 21:43, Sowjanya Komatineni пишет:
>
> On 8/2/19 5:32 AM, Dmitry Osipenko wrote:
>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>> This patch implements save and restore context for peripheral fixed
>>> clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
>>> peripheral clock ops.
>>>
>>> During system suspend, core power goes off and looses the settings
>>> of the Tegra CAR controller registers.
>>>
>>> So during suspend entry clock and reset state of peripherals is saved
>>> and on resume they are restored to have clocks back to same rate and
>>> state as before suspend.
>>>
>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>> ++++++++++++++++++++++++++++++++
>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>> +++++++++++++++++++++++++++++++++
>>> drivers/clk/tegra/clk-periph.c | 37
>>> ++++++++++++++++++++++++++++++++++++
>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28 +++++++++++++++++++++++++++
>>> drivers/clk/tegra/clk.h | 6 ++++++
>>> 5 files changed, 138 insertions(+)
>>>
>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>> index c088e7a280df..21b24530fa00 100644
>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>> @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct clk_hw
>>> *hw,
>>> return (unsigned long)rate;
>>> }
>>> +static int tegra_clk_periph_fixed_save_context(struct clk_hw *hw)
>>> +{
>>> + struct tegra_clk_periph_fixed *fixed =
>>> to_tegra_clk_periph_fixed(hw);
>>> + u32 mask = 1 << (fixed->num % 32);
>> This could be BIT(fixed->num % 32).
>>
>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>> fixed->regs->enb_reg) &
>>> + mask;
>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>> fixed->regs->rst_reg) &
>>> + mask;
>> The enb_ctx/rst_ctx are booleans, while you assigning an integer value
>> here. You're getting away here because bool is an 32bit unsigned int,
>> but you shouldn't rely on it and always explicitly convert to a bool.
>>
>>> + return 0;
>>> +}
>>> +
>>> +static void tegra_clk_periph_fixed_restore_context(struct clk_hw *hw)
>>> +{
>>> + struct tegra_clk_periph_fixed *fixed =
>>> to_tegra_clk_periph_fixed(hw);
>>> + u32 mask = 1 << (fixed->num % 32);
>>> +
>>> + if (fixed->enb_ctx)
>>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_set_reg);
>>> + else
>>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_clr_reg);
>>> +
>>> + udelay(2);
>> Will be better to read out and compare the hardware's state with the
>> restored one, then bail out if the state is unchanged.
>>
>> Shouldn't it be fence_udelay()?
>>
>>> + if (!fixed->rst_ctx) {
>>> + udelay(5); /* reset propogation delay */
>> Why delaying is done before the writing to the reset register?
>
> During SC7 exit, peripheral reset state is set to POR state. So some
> peripherals will already be in reset state and making sure of
> propagation delay before releasing from reset.
>
> It should be rst_clr_reg. will fix in next rev
>
>>
>>> + writel_relaxed(mask, fixed->base + fixed->regs->rst_reg);
>> I'm not quite sure what's going on here, this looks wrong.
>>
>> 1. rst_reg points to RST_DEVICES_x
>> 2. Each bit of RST_DEVICES_x represents the reset-assertion state of
>> each individual device
>> 3. By writing to rst_reg, all (!) devices are deasserted, except the one
>> device which corresponds to the mask
>> 4. The reset is asserted for a single device, while !fixed->rst_ctx
>> means that it actually should be deasserted (?)
>>
>> Apparently you should use rst_set_reg / rst_clr_reg.
> Yes, It should be rst_clr_reg. will fix in next rev
>>> + }
>> What about the case where rst_ctx=true?
>
> ON SC7 exit, state of RST_DEV will be POR state where most peripherals
> will already be in reset state.
>
> Few of them which are not in reset state in POR values are those that
> need to stay de-asserted across the boot states anyway.
Okay, sounds reasonable.
BTW, it would be nice if you could add a brief clarifying comment to the
code for each of the questions asked during of the review.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Dmitry Osipenko @ 2019-08-02 20:17 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <a64472fd-46b7-5ff9-3140-11f71d5f88ff@gmail.com>
02.08.2019 23:13, Dmitry Osipenko пишет:
> 02.08.2019 21:33, Sowjanya Komatineni пишет:
>>
>> On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
>>> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>>>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>> This patch implements save and restore context for
>>>>>>>>>>>>>>>> peripheral
>>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock
>>>>>>>>>>>>>>>> ops, and
>>>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>>>> and on resume they are restored to have clocks back to same
>>>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>> @@ -60,11 +60,44 @@
>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>> +static int tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +static void tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> static const struct clk_ops tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>>>> .is_enabled = tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>>>> .recalc_rate = tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_fixed(const
>>>>>>>>>>>>>>>> char
>>>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>>>> readl_relaxed(gate->clk_base +
>>>>>>>>>>>>>>>> (gate->regs->rst_reg))
>>>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct clk_hw
>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct clk_hw
>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>> +static int clk_periph_save_context(struct clk_hw *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>>>> Could you please point to where the divider's save_context()
>>>>>>>>>>>>>>> happens?
>>>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers
>>>>>>>>>>>>>> context
>>>>>>>>>>>>>> because
>>>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>>>> restoring
>>>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>>>> generic helper to get the index instead of storing it
>>>>>>>>>>>>>> manually.
>>>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>>>
>>>>>>>>>>>>> All existing drivers are using directly get_parent() from
>>>>>>>>>>>>> clk_mux
>>>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>>>
>>>>>>>>>>>>> To have this more generic w.r.t save/restore context point of
>>>>>>>>>>>>> view,
>>>>>>>>>>>>> probably instead of implementing new get_parent_index helper,
>>>>>>>>>>>>> I think
>>>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>>>> clk_mux to
>>>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>>>> restore_context.
>>>>>>>>>>>>>
>>>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops to be
>>>>>>>>>>>> more
>>>>>>>>>>>> generic w.r.t save/restore context rather than get_parent_index
>>>>>>>>>>>> API.
>>>>>>>>>>>> Please confirm if you agree.
>>>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>>>> helper for
>>>>>>>>>>> the generic clk_gate, seems something similar could be done
>>>>>>>>>>> for the
>>>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the
>>>>>>>>>>> parent
>>>>>>>>>>> clock
>>>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>>>> by 'restoring' helper for generic clk_gate, are you referring to
>>>>>>>>>> clk_gate_restore_context API?
>>>>>>>>> Yes.
>>>>>>>>>
>>>>>>>>>> clk_gate_restore_context is API that's any clk drivers can use for
>>>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>>>
>>>>>>>>>> But clk-periph is directly using generic clk_mux ops from clk_mux
>>>>>>>>>> so I
>>>>>>>>>> think we should add .restore_context to clk_mux_ops and then
>>>>>>>>>> during
>>>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>>>> generic
>>>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>>>> function
>>>>>>>>> that any driver could use in order to restore the clock's parent.
>>>>>>>>>
>>>>>>>>> The clk-periph restoring also includes case of combining divider
>>>>>>>>> and
>>>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>>>> as well.
>>>>>>>>>
>>>>>>>>> It also looks like you could actually use the
>>>>>>>>> clk_gate_restore_context()
>>>>>>>>> instead of manually saving the clock's enable-state, couldn't you?
>>>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API rather
>>>>>>>> than
>>>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>>>> clk_periph
>>>>>>>> restore.
>>>>>>>>
>>>> digging thru looks like for clk_periph source restore instead of
>>>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>>>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent anyway.
>>>> Will do this for periph clk mux
>>>>>>>> Reg clk_gate, looks like we cant use generic
>>>>>>>> clk_gate_restore_context
>>>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>>>> clk_periph_enable/disable in clk-periph-gate also updated refcnt and
>>>>>>>> depending on that actual enable/disable is set.
>>>>>>>>
>>>>>>>> During suspend, peripherals that are already enabled have their
>>>>>>>> refcnt >
>>>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>>>> enable/disable callback.
>>>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>>>> save_context, wouldn't that work?
>>>>>>>
>>>>> gate->enable_refcnt is within clk-periph-gate which gets updated when
>>>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>>>> But actual enable_count used in clk_gate_restore_context is the one
>>>>> which gets updated with in the clk core enable/disable functions which
>>>>> invokes these callbacks. Depending on this enable_count in clk core it
>>>>> invokes enable/disable.
>>>>>
>>>>> So, this will cause mismatch if we handle refcnt during save/restore
>>>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>>>> clk_gate_restore_context is based on enable_count from clk core.
>>>>>
>>>>>>>> Also to align exact reset state along with CLK (like for case where
>>>>>>>> CLK
>>>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>>>> I'm wondering whether instead of saving/restoring reset-state of
>>>>>>> every
>>>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET
>>>>>>> register.
>>>>>>> Couldn't you?
>>>>>> Thats what I was doing in first version of patch. But later as we
>>>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>>>> after source restore
>>>>>
>>>>> Also, to align both CLK & RST to the exact state of register, doing
>>>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>>>> source restore for peripheral clock, seems cleaner to avoid any
>>>>> misconfiguration b/w rst & clk settings.
>>>>>
>>> It looks to me that it is very wasteful to store/restore each individual
>>> gate and reset state, also given that some of them are shared. I think
>>> that the gates and resets should be restored separately for the
>>> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
>> clk_periph_fixed_disable just disables clock only without deasserting
>> the corresponding peripheral.
>>
>> corresponding peripheral drivers can also issue reset assert/deassert
>> thru reset_control_assert/deassert.
>>
>> So, we will not get the actual state of clk and rst unless we read and
>> save state of reset and clock separately during save_context.
>>
>> Currently patch is already using custom
>> tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
>> Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
>> register settings instead of individual peripheral bits?
>
> Yes, I'm suggesting to do a complete ungate/reset handling of the
> devices in a separate function. All enabling/deassertion will be done in
> a single hop, hence using 7us delay and four u32 words, which is much
> nicer IMHO.
Actually six words, three for CLKs and three for RSTs.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Dmitry Osipenko @ 2019-08-02 20:13 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <314b5572-4113-d5c5-5956-1a55555a573c@nvidia.com>
02.08.2019 21:33, Sowjanya Komatineni пишет:
>
> On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
>> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>> This patch implements save and restore context for
>>>>>>>>>>>>>>> peripheral
>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock
>>>>>>>>>>>>>>> ops, and
>>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>>> and on resume they are restored to have clocks back to same
>>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>> @@ -60,11 +60,44 @@
>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> +static int tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +static void tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> static const struct clk_ops tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>>> .is_enabled = tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>>> .recalc_rate = tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_fixed(const
>>>>>>>>>>>>>>> char
>>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>>> readl_relaxed(gate->clk_base +
>>>>>>>>>>>>>>> (gate->regs->rst_reg))
>>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct clk_hw
>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct clk_hw
>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> +static int clk_periph_save_context(struct clk_hw *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>>> Could you please point to where the divider's save_context()
>>>>>>>>>>>>>> happens?
>>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers
>>>>>>>>>>>>> context
>>>>>>>>>>>>> because
>>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>>> restoring
>>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>>> generic helper to get the index instead of storing it
>>>>>>>>>>>>> manually.
>>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>>
>>>>>>>>>>>> All existing drivers are using directly get_parent() from
>>>>>>>>>>>> clk_mux
>>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>>
>>>>>>>>>>>> To have this more generic w.r.t save/restore context point of
>>>>>>>>>>>> view,
>>>>>>>>>>>> probably instead of implementing new get_parent_index helper,
>>>>>>>>>>>> I think
>>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>>> clk_mux to
>>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>>
>>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>>> restore_context.
>>>>>>>>>>>>
>>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops to be
>>>>>>>>>>> more
>>>>>>>>>>> generic w.r.t save/restore context rather than get_parent_index
>>>>>>>>>>> API.
>>>>>>>>>>> Please confirm if you agree.
>>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>>> helper for
>>>>>>>>>> the generic clk_gate, seems something similar could be done
>>>>>>>>>> for the
>>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the
>>>>>>>>>> parent
>>>>>>>>>> clock
>>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>>> by 'restoring' helper for generic clk_gate, are you referring to
>>>>>>>>> clk_gate_restore_context API?
>>>>>>>> Yes.
>>>>>>>>
>>>>>>>>> clk_gate_restore_context is API that's any clk drivers can use for
>>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>>
>>>>>>>>> But clk-periph is directly using generic clk_mux ops from clk_mux
>>>>>>>>> so I
>>>>>>>>> think we should add .restore_context to clk_mux_ops and then
>>>>>>>>> during
>>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>>> generic
>>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>>> function
>>>>>>>> that any driver could use in order to restore the clock's parent.
>>>>>>>>
>>>>>>>> The clk-periph restoring also includes case of combining divider
>>>>>>>> and
>>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>>> as well.
>>>>>>>>
>>>>>>>> It also looks like you could actually use the
>>>>>>>> clk_gate_restore_context()
>>>>>>>> instead of manually saving the clock's enable-state, couldn't you?
>>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API rather
>>>>>>> than
>>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>>> clk_periph
>>>>>>> restore.
>>>>>>>
>>> digging thru looks like for clk_periph source restore instead of
>>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent anyway.
>>> Will do this for periph clk mux
>>>>>>> Reg clk_gate, looks like we cant use generic
>>>>>>> clk_gate_restore_context
>>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>>> clk_periph_enable/disable in clk-periph-gate also updated refcnt and
>>>>>>> depending on that actual enable/disable is set.
>>>>>>>
>>>>>>> During suspend, peripherals that are already enabled have their
>>>>>>> refcnt >
>>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>>> enable/disable callback.
>>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>>> save_context, wouldn't that work?
>>>>>>
>>>> gate->enable_refcnt is within clk-periph-gate which gets updated when
>>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>>> But actual enable_count used in clk_gate_restore_context is the one
>>>> which gets updated with in the clk core enable/disable functions which
>>>> invokes these callbacks. Depending on this enable_count in clk core it
>>>> invokes enable/disable.
>>>>
>>>> So, this will cause mismatch if we handle refcnt during save/restore
>>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>>> clk_gate_restore_context is based on enable_count from clk core.
>>>>
>>>>>>> Also to align exact reset state along with CLK (like for case where
>>>>>>> CLK
>>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>>> I'm wondering whether instead of saving/restoring reset-state of
>>>>>> every
>>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET
>>>>>> register.
>>>>>> Couldn't you?
>>>>> Thats what I was doing in first version of patch. But later as we
>>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>>> after source restore
>>>>
>>>> Also, to align both CLK & RST to the exact state of register, doing
>>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>>> source restore for peripheral clock, seems cleaner to avoid any
>>>> misconfiguration b/w rst & clk settings.
>>>>
>> It looks to me that it is very wasteful to store/restore each individual
>> gate and reset state, also given that some of them are shared. I think
>> that the gates and resets should be restored separately for the
>> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
> clk_periph_fixed_disable just disables clock only without deasserting
> the corresponding peripheral.
>
> corresponding peripheral drivers can also issue reset assert/deassert
> thru reset_control_assert/deassert.
>
> So, we will not get the actual state of clk and rst unless we read and
> save state of reset and clock separately during save_context.
>
> Currently patch is already using custom
> tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
> Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
> register settings instead of individual peripheral bits?
Yes, I'm suggesting to do a complete ungate/reset handling of the
devices in a separate function. All enabling/deassertion will be done in
a single hop, hence using 7us delay and four u32 words, which is much
nicer IMHO.
^ permalink raw reply
* Re: [PATCH v16 1/2] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
From: Sergei Shtylyov @ 2019-08-02 20:02 UTC (permalink / raw)
To: Mason Yang, broonie, robh+dt, mark.rutland, linux-kernel,
linux-spi, linux-renesas-soc, Geert Uytterhoeven, devicetree
Cc: juliensu, Simon Horman, lee.jones, marek.vasut, miquel.raynal
In-Reply-To: <1564539258-16313-2-git-send-email-masonccyang@mxic.com.tw>
Hello!
On 07/31/2019 05:14 AM, Mason Yang wrote:
> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.
>
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
[...]
> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
> new file mode 100644
> index 0000000..648d14e
> --- /dev/null
> +++ b/drivers/spi/spi-renesas-rpc.c
> @@ -0,0 +1,754 @@
[...]
> +static void rpc_spi_hw_init(struct rpc_spi *rpc)
> +{
> + //
> + // NOTE: The 0x260 are undocumented bits, but they must be set.
> + // RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
> + // 0x0 : the delay is biggest,
> + // 0x1 : the delay is 2nd biggest,
> + // On H3 ES1.x, the value should be 0, while on others,
> + // the value should be 6.
> + //
> + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
> + RPC_PHYCNT_STRTIM(6) | 0x260);
> +
> + //
> + // NOTE: The 0x1511144 are undocumented bits, but they must be set
> + // for RPC_PHYOFFSET1.
> + // The 0x31 are undocumented bits, but they must be set
> + // for RPC_PHYOFFSET2.
> + //
> + regmap_write(rpc->regmap, RPC_PHYOFFSET1, RPC_PHYOFFSET1_DDRTMG(3) |
> + 0x1511144);
> + regmap_write(rpc->regmap, RPC_PHYOFFSET2, 0x31 |
> + RPC_PHYOFFSET2_OCTTMG(4));
> + regmap_write(rpc->regmap, RPC_SSLDR, RPC_SSLDR_SPNDL(7) |
> + RPC_SSLDR_SLNDL(7) | RPC_SSLDR_SCKDL(7));
> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> + RPC_CMNCR_BSZ(0));
> +}
[...]
> +static int rpc_spi_io_xfer(struct rpc_spi *rpc,
> + const void *tx_buf, void *rx_buf)
> +{
[...]
> +err_out:
> + return reset_control_reset(rpc->rstc);
Don't toy need to call rpc_spi_hw_init(( here? The reset would spoil
the PHY/etc register setup otherwise...
[...]
MBR, Sergei
^ permalink raw reply
* [PATCH 6/6] arm64: defconfig: Enable configs for S32V234
From: Stefan-gabriel Mirea @ 2019-08-02 19:47 UTC (permalink / raw)
To: corbet@lwn.net, robh+dt@kernel.org, mark.rutland@arm.com,
gregkh@linuxfoundation.org, catalin.marinas@arm.com,
will@kernel.org, shawnguo@kernel.org, Leo Li
Cc: jslaby@suse.com, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Cosmin Stefan Stoica
In-Reply-To: <20190802194702.30249-1-stefan-gabriel.mirea@nxp.com>
From: Mihaela Martinas <Mihaela.Martinas@freescale.com>
Enable support for the S32V234 SoC, including the previously added UART
driver.
Signed-off-by: Mihaela Martinas <Mihaela.Martinas@freescale.com>
Signed-off-by: Adrian.Nitu <adrian.nitu@freescale.com>
Signed-off-by: Stoica Cosmin-Stefan <cosmin.stoica@nxp.com>
Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
---
arch/arm64/configs/defconfig | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 0e58ef02880c..bb5aa95a8455 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -48,6 +48,7 @@ CONFIG_ARCH_MXC=y
CONFIG_ARCH_QCOM=y
CONFIG_ARCH_RENESAS=y
CONFIG_ARCH_ROCKCHIP=y
+CONFIG_ARCH_S32=y
CONFIG_ARCH_SEATTLE=y
CONFIG_ARCH_STRATIX10=y
CONFIG_ARCH_SYNQUACER=y
@@ -347,6 +348,8 @@ CONFIG_SERIAL_XILINX_PS_UART=y
CONFIG_SERIAL_XILINX_PS_UART_CONSOLE=y
CONFIG_SERIAL_FSL_LPUART=y
CONFIG_SERIAL_FSL_LPUART_CONSOLE=y
+CONFIG_SERIAL_FSL_LINFLEXUART=y
+CONFIG_SERIAL_FSL_LINFLEXUART_CONSOLE=y
CONFIG_SERIAL_MVEBU_UART=y
CONFIG_SERIAL_DEV_BUS=y
CONFIG_VIRTIO_CONSOLE=y
--
2.22.0
^ permalink raw reply related
* [PATCH 5/6] tty: serial: Add linflexuart driver for S32V234
From: Stefan-gabriel Mirea @ 2019-08-02 19:47 UTC (permalink / raw)
To: corbet@lwn.net, robh+dt@kernel.org, mark.rutland@arm.com,
gregkh@linuxfoundation.org, catalin.marinas@arm.com,
will@kernel.org, shawnguo@kernel.org, Leo Li
Cc: jslaby@suse.com, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Cosmin Stefan Stoica,
Larisa Ileana Grigore
In-Reply-To: <20190802194702.30249-1-stefan-gabriel.mirea@nxp.com>
Introduce support for LINFlex driver, based on:
- the version of Freescale LPUART driver after commit b3e3bf2ef2c7 ("Merge
4.0-rc7 into tty-next");
- commit abf1e0a98083 ("tty: serial: fsl_lpuart: lock port on console
write").
In this basic version, the driver can be tested using initramfs and relies
on the clocks and pin muxing set up by U-Boot.
Remarks concerning the earlycon support:
- LinFlexD does not allow character transmissions in the INIT mode (see
section 47.4.2.1 in the reference manual[1]). Therefore, a mutual
exclusion between the first linflex_setup_watermark/linflex_set_termios
executions and linflex_earlycon_putchar was employed and the characters
normally sent to earlycon during initialization are kept in a buffer and
sent afterwards.
- Empirically, character transmission is also forbidden within the last 1-2
ms before entering the INIT mode, so we use an explicit timeout
(PREINIT_DELAY) between linflex_earlycon_putchar and the first call to
linflex_setup_watermark.
- U-Boot currently uses the UART FIFO mode, while this driver makes the
transition to the buffer mode. Therefore, the earlycon putchar function
matches the U-Boot behavior before initializations and the Linux behavior
after.
[1] https://www.nxp.com/webapp/Download?colCode=S32V234RM
Signed-off-by: Stoica Cosmin-Stefan <cosmin.stoica@nxp.com>
Signed-off-by: Adrian.Nitu <adrian.nitu@freescale.com>
Signed-off-by: Larisa Grigore <Larisa.Grigore@nxp.com>
Signed-off-by: Ana Nedelcu <B56683@freescale.com>
Signed-off-by: Mihaela Martinas <Mihaela.Martinas@freescale.com>
Signed-off-by: Matthew Nunez <matthew.nunez@nxp.com>
[stefan-gabriel.mirea@nxp.com: Reduced for upstreaming and implemented
earlycon support]
Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
---
.../admin-guide/kernel-parameters.txt | 6 +
drivers/tty/serial/Kconfig | 15 +
drivers/tty/serial/Makefile | 1 +
drivers/tty/serial/fsl_linflexuart.c | 956 ++++++++++++++++++
include/uapi/linux/serial_core.h | 3 +
5 files changed, 981 insertions(+)
create mode 100644 drivers/tty/serial/fsl_linflexuart.c
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 46b826fcb5ad..4d545732aadc 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1090,6 +1090,12 @@
the framebuffer, pass the 'ram' option so that it is
mapped with the correct attributes.
+ linflex,<addr>
+ Use early console provided by Freescale LinFlex UART
+ serial driver for NXP S32V234 SoCs. A valid base
+ address must be provided, and the serial port must
+ already be setup and configured.
+
earlyprintk= [X86,SH,ARM,M68k,S390]
earlyprintk=vga
earlyprintk=sclp
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index fd385c8c53a5..b4fa2f7c96bd 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1452,6 +1452,21 @@ config SERIAL_FSL_LPUART_CONSOLE
If you have enabled the lpuart serial port on the Freescale SoCs,
you can make it the console by answering Y to this option.
+config SERIAL_FSL_LINFLEXUART
+ tristate "Freescale linflexuart serial port support"
+ select SERIAL_CORE
+ help
+ Support for the on-chip linflexuart on some Freescale SOCs.
+
+config SERIAL_FSL_LINFLEXUART_CONSOLE
+ bool "Console on Freescale linflexuart serial port"
+ depends on SERIAL_FSL_LINFLEXUART=y
+ select SERIAL_CORE_CONSOLE
+ select SERIAL_EARLYCON
+ help
+ If you have enabled the linflexuart serial port on the Freescale
+ SoCs, you can make it the console by answering Y to this option.
+
config SERIAL_CONEXANT_DIGICOLOR
tristate "Conexant Digicolor CX92xxx USART serial port support"
depends on OF
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index 7cd7cabfa6c4..7a3d52a453b7 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_SERIAL_EFM32_UART) += efm32-uart.o
obj-$(CONFIG_SERIAL_ARC) += arc_uart.o
obj-$(CONFIG_SERIAL_RP2) += rp2.o
obj-$(CONFIG_SERIAL_FSL_LPUART) += fsl_lpuart.o
+obj-$(CONFIG_SERIAL_FSL_LINFLEXUART) += fsl_linflexuart.o
obj-$(CONFIG_SERIAL_CONEXANT_DIGICOLOR) += digicolor-usart.o
obj-$(CONFIG_SERIAL_MEN_Z135) += men_z135_uart.o
obj-$(CONFIG_SERIAL_SPRD) += sprd_serial.o
diff --git a/drivers/tty/serial/fsl_linflexuart.c b/drivers/tty/serial/fsl_linflexuart.c
new file mode 100644
index 000000000000..7c4e6872b360
--- /dev/null
+++ b/drivers/tty/serial/fsl_linflexuart.c
@@ -0,0 +1,956 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Freescale linflexuart serial port driver
+ *
+ * Copyright 2012-2016 Freescale Semiconductor, Inc.
+ * Copyright 2017-2018 NXP
+ */
+
+#if defined(CONFIG_SERIAL_FSL_LINFLEXUART_CONSOLE) && \
+ defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
+#include <linux/console.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+#include <linux/tty_flip.h>
+#include <linux/delay.h>
+
+/* All registers are 32-bit width */
+
+#define LINCR1 0x0000 /* LIN control register */
+#define LINIER 0x0004 /* LIN interrupt enable register */
+#define LINSR 0x0008 /* LIN status register */
+#define LINESR 0x000C /* LIN error status register */
+#define UARTCR 0x0010 /* UART mode control register */
+#define UARTSR 0x0014 /* UART mode status register */
+#define LINTCSR 0x0018 /* LIN timeout control status register */
+#define LINOCR 0x001C /* LIN output compare register */
+#define LINTOCR 0x0020 /* LIN timeout control register */
+#define LINFBRR 0x0024 /* LIN fractional baud rate register */
+#define LINIBRR 0x0028 /* LIN integer baud rate register */
+#define LINCFR 0x002C /* LIN checksum field register */
+#define LINCR2 0x0030 /* LIN control register 2 */
+#define BIDR 0x0034 /* Buffer identifier register */
+#define BDRL 0x0038 /* Buffer data register least significant */
+#define BDRM 0x003C /* Buffer data register most significant */
+#define IFER 0x0040 /* Identifier filter enable register */
+#define IFMI 0x0044 /* Identifier filter match index */
+#define IFMR 0x0048 /* Identifier filter mode register */
+#define GCR 0x004C /* Global control register */
+#define UARTPTO 0x0050 /* UART preset timeout register */
+#define UARTCTO 0x0054 /* UART current timeout register */
+
+/*
+ * Register field definitions
+ */
+
+#define LINFLEXD_LINCR1_INIT BIT(0)
+#define LINFLEXD_LINCR1_MME BIT(4)
+#define LINFLEXD_LINCR1_BF BIT(7)
+
+#define LINFLEXD_LINSR_LINS_INITMODE BIT(12)
+#define LINFLEXD_LINSR_LINS_MASK (0xF << 12)
+
+#define LINFLEXD_LINIER_SZIE BIT(15)
+#define LINFLEXD_LINIER_OCIE BIT(14)
+#define LINFLEXD_LINIER_BEIE BIT(13)
+#define LINFLEXD_LINIER_CEIE BIT(12)
+#define LINFLEXD_LINIER_HEIE BIT(11)
+#define LINFLEXD_LINIER_FEIE BIT(8)
+#define LINFLEXD_LINIER_BOIE BIT(7)
+#define LINFLEXD_LINIER_LSIE BIT(6)
+#define LINFLEXD_LINIER_WUIE BIT(5)
+#define LINFLEXD_LINIER_DBFIE BIT(4)
+#define LINFLEXD_LINIER_DBEIETOIE BIT(3)
+#define LINFLEXD_LINIER_DRIE BIT(2)
+#define LINFLEXD_LINIER_DTIE BIT(1)
+#define LINFLEXD_LINIER_HRIE BIT(0)
+
+#define LINFLEXD_UARTCR_OSR_MASK (0xF << 24)
+#define LINFLEXD_UARTCR_OSR(uartcr) (((uartcr) \
+ & LINFLEXD_UARTCR_OSR_MASK) >> 24)
+
+#define LINFLEXD_UARTCR_ROSE BIT(23)
+
+#define LINFLEXD_UARTCR_RFBM BIT(9)
+#define LINFLEXD_UARTCR_TFBM BIT(8)
+#define LINFLEXD_UARTCR_WL1 BIT(7)
+#define LINFLEXD_UARTCR_PC1 BIT(6)
+
+#define LINFLEXD_UARTCR_RXEN BIT(5)
+#define LINFLEXD_UARTCR_TXEN BIT(4)
+#define LINFLEXD_UARTCR_PC0 BIT(3)
+
+#define LINFLEXD_UARTCR_PCE BIT(2)
+#define LINFLEXD_UARTCR_WL0 BIT(1)
+#define LINFLEXD_UARTCR_UART BIT(0)
+
+#define LINFLEXD_UARTSR_SZF BIT(15)
+#define LINFLEXD_UARTSR_OCF BIT(14)
+#define LINFLEXD_UARTSR_PE3 BIT(13)
+#define LINFLEXD_UARTSR_PE2 BIT(12)
+#define LINFLEXD_UARTSR_PE1 BIT(11)
+#define LINFLEXD_UARTSR_PE0 BIT(10)
+#define LINFLEXD_UARTSR_RMB BIT(9)
+#define LINFLEXD_UARTSR_FEF BIT(8)
+#define LINFLEXD_UARTSR_BOF BIT(7)
+#define LINFLEXD_UARTSR_RPS BIT(6)
+#define LINFLEXD_UARTSR_WUF BIT(5)
+#define LINFLEXD_UARTSR_4 BIT(4)
+
+#define LINFLEXD_UARTSR_TO BIT(3)
+
+#define LINFLEXD_UARTSR_DRFRFE BIT(2)
+#define LINFLEXD_UARTSR_DTFTFF BIT(1)
+#define LINFLEXD_UARTSR_NF BIT(0)
+#define LINFLEXD_UARTSR_PE (LINFLEXD_UARTSR_PE0 |\
+ LINFLEXD_UARTSR_PE1 |\
+ LINFLEXD_UARTSR_PE2 |\
+ LINFLEXD_UARTSR_PE3)
+
+#define LINFLEX_LDIV_MULTIPLIER (16)
+
+#define DRIVER_NAME "fsl-linflexuart"
+#define DEV_NAME "ttyLF"
+#define UART_NR 4
+
+#define EARLYCON_BUFFER_INITIAL_CAP 8
+
+#define PREINIT_DELAY 2000 /* us */
+
+static const struct of_device_id linflex_dt_ids[] = {
+ {
+ .compatible = "fsl,s32-linflexuart",
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, linflex_dt_ids);
+
+#ifdef CONFIG_SERIAL_FSL_LINFLEXUART_CONSOLE
+static struct uart_port *earlycon_port;
+static bool linflex_earlycon_same_instance;
+static spinlock_t init_lock;
+static bool during_init;
+
+static struct {
+ char *content;
+ unsigned int len, cap;
+} earlycon_buf;
+#endif
+
+static void linflex_stop_tx(struct uart_port *port)
+{
+ unsigned long ier;
+
+ ier = readl(port->membase + LINIER);
+ ier &= ~(LINFLEXD_LINIER_DTIE);
+ writel(ier, port->membase + LINIER);
+}
+
+static void linflex_stop_rx(struct uart_port *port)
+{
+ unsigned long ier;
+
+ ier = readl(port->membase + LINIER);
+ writel(ier & ~LINFLEXD_LINIER_DRIE, port->membase + LINIER);
+}
+
+static inline void linflex_transmit_buffer(struct uart_port *sport)
+{
+ struct circ_buf *xmit = &sport->state->xmit;
+ unsigned char c;
+ unsigned long status;
+
+ while (!uart_circ_empty(xmit)) {
+ c = xmit->buf[xmit->tail];
+ writeb(c, sport->membase + BDRL);
+
+ /* Waiting for data transmission completed. */
+ while (((status = readl(sport->membase + UARTSR)) &
+ LINFLEXD_UARTSR_DTFTFF) !=
+ LINFLEXD_UARTSR_DTFTFF)
+ ;
+
+ xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+ sport->icount.tx++;
+
+ writel(status | LINFLEXD_UARTSR_DTFTFF,
+ sport->membase + UARTSR);
+ }
+
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(sport);
+
+ if (uart_circ_empty(xmit))
+ linflex_stop_tx(sport);
+}
+
+static void linflex_start_tx(struct uart_port *port)
+{
+ unsigned long ier;
+
+ linflex_transmit_buffer(port);
+ ier = readl(port->membase + LINIER);
+ writel(ier | LINFLEXD_LINIER_DTIE, port->membase + LINIER);
+}
+
+static irqreturn_t linflex_txint(int irq, void *dev_id)
+{
+ struct uart_port *sport = dev_id;
+ struct circ_buf *xmit = &sport->state->xmit;
+ unsigned long flags;
+ unsigned long status;
+
+ spin_lock_irqsave(&sport->lock, flags);
+
+ if (sport->x_char) {
+ writeb(sport->x_char, sport->membase + BDRL);
+
+ /* waiting for data transmission completed */
+ while (((status = readl(sport->membase + UARTSR)) &
+ LINFLEXD_UARTSR_DTFTFF) != LINFLEXD_UARTSR_DTFTFF)
+ ;
+
+ writel(status | LINFLEXD_UARTSR_DTFTFF,
+ sport->membase + UARTSR);
+
+ goto out;
+ }
+
+ if (uart_circ_empty(xmit) || uart_tx_stopped(sport)) {
+ linflex_stop_tx(sport);
+ goto out;
+ }
+
+ linflex_transmit_buffer(sport);
+
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(sport);
+
+out:
+ spin_unlock_irqrestore(&sport->lock, flags);
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t linflex_rxint(int irq, void *dev_id)
+{
+ struct uart_port *sport = dev_id;
+ unsigned int flg;
+ struct tty_port *port = &sport->state->port;
+ unsigned long flags, status;
+ unsigned char rx;
+
+ spin_lock_irqsave(&sport->lock, flags);
+
+ status = readl(sport->membase + UARTSR);
+ while (status & LINFLEXD_UARTSR_RMB) {
+ rx = readb(sport->membase + BDRM);
+ flg = TTY_NORMAL;
+ sport->icount.rx++;
+
+ if (status & (LINFLEXD_UARTSR_BOF | LINFLEXD_UARTSR_SZF |
+ LINFLEXD_UARTSR_FEF | LINFLEXD_UARTSR_PE)) {
+ if (status & LINFLEXD_UARTSR_SZF)
+ status |= LINFLEXD_UARTSR_SZF;
+ if (status & LINFLEXD_UARTSR_BOF)
+ status |= LINFLEXD_UARTSR_BOF;
+ if (status & LINFLEXD_UARTSR_FEF)
+ status |= LINFLEXD_UARTSR_FEF;
+ if (status & LINFLEXD_UARTSR_PE)
+ status |= LINFLEXD_UARTSR_PE;
+ }
+
+ writel(status | LINFLEXD_UARTSR_RMB | LINFLEXD_UARTSR_DRFRFE,
+ sport->membase + UARTSR);
+ status = readl(sport->membase + UARTSR);
+
+ if (uart_handle_sysrq_char(sport, (unsigned char)rx))
+ continue;
+
+#ifdef SUPPORT_SYSRQ
+ sport->sysrq = 0;
+#endif
+ tty_insert_flip_char(port, rx, flg);
+ }
+
+ spin_unlock_irqrestore(&sport->lock, flags);
+
+ tty_flip_buffer_push(port);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t linflex_int(int irq, void *dev_id)
+{
+ struct uart_port *sport = dev_id;
+ unsigned long status;
+
+ status = readl(sport->membase + UARTSR);
+
+ if (status & LINFLEXD_UARTSR_DRFRFE)
+ linflex_rxint(irq, dev_id);
+ if (status & LINFLEXD_UARTSR_DTFTFF)
+ linflex_txint(irq, dev_id);
+
+ return IRQ_HANDLED;
+}
+
+/* return TIOCSER_TEMT when transmitter is not busy */
+static unsigned int linflex_tx_empty(struct uart_port *port)
+{
+ unsigned long status;
+
+ status = readl(port->membase + UARTSR) & LINFLEXD_UARTSR_DTFTFF;
+
+ return status ? TIOCSER_TEMT : 0;
+}
+
+static unsigned int linflex_get_mctrl(struct uart_port *port)
+{
+ return 0;
+}
+
+static void linflex_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+}
+
+static void linflex_break_ctl(struct uart_port *port, int break_state)
+{
+}
+
+static void linflex_setup_watermark(struct uart_port *sport)
+{
+ unsigned long cr, ier, cr1;
+
+ /* Disable transmission/reception */
+ ier = readl(sport->membase + LINIER);
+ ier &= ~(LINFLEXD_LINIER_DRIE | LINFLEXD_LINIER_DTIE);
+ writel(ier, sport->membase + LINIER);
+
+ cr = readl(sport->membase + UARTCR);
+ cr &= ~(LINFLEXD_UARTCR_RXEN | LINFLEXD_UARTCR_TXEN);
+ writel(cr, sport->membase + UARTCR);
+
+ /* Enter initialization mode by setting INIT bit */
+
+ /* set the Linflex in master mode and activate by-pass filter */
+ cr1 = LINFLEXD_LINCR1_BF | LINFLEXD_LINCR1_MME
+ | LINFLEXD_LINCR1_INIT;
+ writel(cr1, sport->membase + LINCR1);
+
+ /* wait for init mode entry */
+ while ((readl(sport->membase + LINSR)
+ & LINFLEXD_LINSR_LINS_MASK)
+ != LINFLEXD_LINSR_LINS_INITMODE)
+ ;
+
+ /*
+ * UART = 0x1; - Linflex working in UART mode
+ * TXEN = 0x1; - Enable transmission of data now
+ * RXEn = 0x1; - Receiver enabled
+ * WL0 = 0x1; - 8 bit data
+ * PCE = 0x0; - No parity
+ */
+
+ /* set UART bit to allow writing other bits */
+ writel(LINFLEXD_UARTCR_UART, sport->membase + UARTCR);
+
+ cr = (LINFLEXD_UARTCR_RXEN | LINFLEXD_UARTCR_TXEN |
+ LINFLEXD_UARTCR_WL0 | LINFLEXD_UARTCR_UART);
+
+ writel(cr, sport->membase + UARTCR);
+
+ cr1 &= ~(LINFLEXD_LINCR1_INIT);
+
+ writel(cr1, sport->membase + LINCR1);
+
+ ier = readl(sport->membase + LINIER);
+ ier |= LINFLEXD_LINIER_DRIE;
+ ier |= LINFLEXD_LINIER_DTIE;
+
+ writel(ier, sport->membase + LINIER);
+}
+
+static int linflex_startup(struct uart_port *port)
+{
+ int ret = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ linflex_setup_watermark(port);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ ret = devm_request_irq(port->dev, port->irq, linflex_int, 0,
+ DRIVER_NAME, port);
+
+ return ret;
+}
+
+static void linflex_shutdown(struct uart_port *port)
+{
+ unsigned long ier;
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ /* disable interrupts */
+ ier = readl(port->membase + LINIER);
+ ier &= ~(LINFLEXD_LINIER_DRIE | LINFLEXD_LINIER_DTIE);
+ writel(ier, port->membase + LINIER);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ devm_free_irq(port->dev, port->irq, port);
+}
+
+static void
+linflex_set_termios(struct uart_port *port, struct ktermios *termios,
+ struct ktermios *old)
+{
+ unsigned long flags;
+ unsigned long cr, old_cr, cr1;
+ unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
+
+ cr = readl(port->membase + UARTCR);
+ old_cr = cr;
+
+ /* Enter initialization mode by setting INIT bit */
+ cr1 = readl(port->membase + LINCR1);
+ cr1 |= LINFLEXD_LINCR1_INIT;
+ writel(cr1, port->membase + LINCR1);
+
+ /* wait for init mode entry */
+ while ((readl(port->membase + LINSR)
+ & LINFLEXD_LINSR_LINS_MASK)
+ != LINFLEXD_LINSR_LINS_INITMODE)
+ ;
+
+ /*
+ * only support CS8 and CS7, and for CS7 must enable PE.
+ * supported mode:
+ * - (7,e/o,1)
+ * - (8,n,1)
+ * - (8,e/o,1)
+ */
+ /* enter the UART into configuration mode */
+
+ while ((termios->c_cflag & CSIZE) != CS8 &&
+ (termios->c_cflag & CSIZE) != CS7) {
+ termios->c_cflag &= ~CSIZE;
+ termios->c_cflag |= old_csize;
+ old_csize = CS8;
+ }
+
+ if ((termios->c_cflag & CSIZE) == CS7) {
+ /* Word length: WL1WL0:00 */
+ cr = old_cr & ~LINFLEXD_UARTCR_WL1 & ~LINFLEXD_UARTCR_WL0;
+ }
+
+ if ((termios->c_cflag & CSIZE) == CS8) {
+ /* Word length: WL1WL0:01 */
+ cr = (old_cr | LINFLEXD_UARTCR_WL0) & ~LINFLEXD_UARTCR_WL1;
+ }
+
+ if (termios->c_cflag & CMSPAR) {
+ if ((termios->c_cflag & CSIZE) != CS8) {
+ termios->c_cflag &= ~CSIZE;
+ termios->c_cflag |= CS8;
+ }
+ /* has a space/sticky bit */
+ cr |= LINFLEXD_UARTCR_WL0;
+ }
+
+ if (termios->c_cflag & CSTOPB)
+ termios->c_cflag &= ~CSTOPB;
+
+ /* parity must be enabled when CS7 to match 8-bits format */
+ if ((termios->c_cflag & CSIZE) == CS7)
+ termios->c_cflag |= PARENB;
+
+ if ((termios->c_cflag & PARENB)) {
+ cr |= LINFLEXD_UARTCR_PCE;
+ if (termios->c_cflag & PARODD)
+ cr = (cr | LINFLEXD_UARTCR_PC0) &
+ (~LINFLEXD_UARTCR_PC1);
+ else
+ cr = cr & (~LINFLEXD_UARTCR_PC1 &
+ ~LINFLEXD_UARTCR_PC0);
+ } else {
+ cr &= ~LINFLEXD_UARTCR_PCE;
+ }
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ port->read_status_mask = 0;
+
+ if (termios->c_iflag & INPCK)
+ port->read_status_mask |= (LINFLEXD_UARTSR_FEF |
+ LINFLEXD_UARTSR_PE0 |
+ LINFLEXD_UARTSR_PE1 |
+ LINFLEXD_UARTSR_PE2 |
+ LINFLEXD_UARTSR_PE3);
+ if (termios->c_iflag & (IGNBRK | BRKINT | PARMRK))
+ port->read_status_mask |= LINFLEXD_UARTSR_FEF;
+
+ /* characters to ignore */
+ port->ignore_status_mask = 0;
+ if (termios->c_iflag & IGNPAR)
+ port->ignore_status_mask |= LINFLEXD_UARTSR_PE;
+ if (termios->c_iflag & IGNBRK) {
+ port->ignore_status_mask |= LINFLEXD_UARTSR_PE;
+ /*
+ * if we're ignoring parity and break indicators,
+ * ignore overruns too (for real raw support).
+ */
+ if (termios->c_iflag & IGNPAR)
+ port->ignore_status_mask |= LINFLEXD_UARTSR_BOF;
+ }
+
+ writel(cr, port->membase + UARTCR);
+
+ cr1 &= ~(LINFLEXD_LINCR1_INIT);
+
+ writel(cr1, port->membase + LINCR1);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static const char *linflex_type(struct uart_port *port)
+{
+ return "FSL_LINFLEX";
+}
+
+static void linflex_release_port(struct uart_port *port)
+{
+ /* nothing to do */
+}
+
+static int linflex_request_port(struct uart_port *port)
+{
+ return 0;
+}
+
+/* configure/auto-configure the port */
+static void linflex_config_port(struct uart_port *port, int flags)
+{
+ if (flags & UART_CONFIG_TYPE)
+ port->type = PORT_LINFLEXUART;
+}
+
+static int linflex_verify_port(struct uart_port *port,
+ struct serial_struct *ser)
+{
+ if ((ser->type != PORT_UNKNOWN && ser->type != PORT_LINFLEXUART) ||
+ (port->irq != ser->irq) ||
+ (ser->io_type != UPIO_MEM) ||
+ (port->iobase != ser->port) ||
+ (ser->hub6 != 0))
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct uart_ops linflex_pops = {
+ .tx_empty = linflex_tx_empty,
+ .set_mctrl = linflex_set_mctrl,
+ .get_mctrl = linflex_get_mctrl,
+ .stop_tx = linflex_stop_tx,
+ .start_tx = linflex_start_tx,
+ .stop_rx = linflex_stop_rx,
+ .break_ctl = linflex_break_ctl,
+ .startup = linflex_startup,
+ .shutdown = linflex_shutdown,
+ .set_termios = linflex_set_termios,
+ .type = linflex_type,
+ .request_port = linflex_request_port,
+ .release_port = linflex_release_port,
+ .config_port = linflex_config_port,
+ .verify_port = linflex_verify_port,
+};
+
+static struct uart_port *linflex_ports[UART_NR];
+
+#ifdef CONFIG_SERIAL_FSL_LINFLEXUART_CONSOLE
+static void linflex_console_putchar(struct uart_port *port, int ch)
+{
+ unsigned long cr;
+
+ cr = readl(port->membase + UARTCR);
+
+ writeb(ch, port->membase + BDRL);
+
+ if (!(cr & LINFLEXD_UARTCR_TFBM))
+ while ((readl(port->membase + UARTSR) &
+ LINFLEXD_UARTSR_DTFTFF)
+ != LINFLEXD_UARTSR_DTFTFF)
+ ;
+ else
+ while (readl(port->membase + UARTSR) &
+ LINFLEXD_UARTSR_DTFTFF)
+ ;
+
+ if (!(cr & LINFLEXD_UARTCR_TFBM)) {
+ writel((readl(port->membase + UARTSR) |
+ LINFLEXD_UARTSR_DTFTFF),
+ port->membase + UARTSR);
+ }
+}
+
+static void linflex_earlycon_putchar(struct uart_port *port, int ch)
+{
+ unsigned long flags;
+ char *ret;
+
+ if (!linflex_earlycon_same_instance) {
+ linflex_console_putchar(port, ch);
+ return;
+ }
+
+ spin_lock_irqsave(&init_lock, flags);
+ if (!during_init)
+ goto outside_init;
+
+ if (earlycon_buf.len >= 1 << CONFIG_LOG_BUF_SHIFT)
+ goto init_release;
+
+ if (!earlycon_buf.cap) {
+ earlycon_buf.content = kmalloc(EARLYCON_BUFFER_INITIAL_CAP,
+ GFP_ATOMIC);
+ earlycon_buf.cap = earlycon_buf.content ?
+ EARLYCON_BUFFER_INITIAL_CAP : 0;
+ } else if (earlycon_buf.len == earlycon_buf.cap) {
+ ret = krealloc(earlycon_buf.content, earlycon_buf.cap << 1,
+ GFP_ATOMIC);
+ if (ret) {
+ earlycon_buf.content = ret;
+ earlycon_buf.cap <<= 1;
+ }
+ }
+
+ if (earlycon_buf.len < earlycon_buf.cap)
+ earlycon_buf.content[earlycon_buf.len++] = ch;
+
+ goto init_release;
+
+outside_init:
+ linflex_console_putchar(port, ch);
+init_release:
+ spin_unlock_irqrestore(&init_lock, flags);
+}
+
+static void linflex_string_write(struct uart_port *sport, const char *s,
+ unsigned int count)
+{
+ unsigned long cr, ier = 0;
+
+ ier = readl(sport->membase + LINIER);
+ linflex_stop_tx(sport);
+
+ cr = readl(sport->membase + UARTCR);
+ cr |= (LINFLEXD_UARTCR_TXEN);
+ writel(cr, sport->membase + UARTCR);
+
+ uart_console_write(sport, s, count, linflex_console_putchar);
+
+ writel(ier, sport->membase + LINIER);
+}
+
+static void
+linflex_console_write(struct console *co, const char *s, unsigned int count)
+{
+ struct uart_port *sport = linflex_ports[co->index];
+ unsigned long flags;
+ int locked = 1;
+
+ if (sport->sysrq)
+ locked = 0;
+ else if (oops_in_progress)
+ locked = spin_trylock_irqsave(&sport->lock, flags);
+ else
+ spin_lock_irqsave(&sport->lock, flags);
+
+ linflex_string_write(sport, s, count);
+
+ if (locked)
+ spin_unlock_irqrestore(&sport->lock, flags);
+}
+
+/*
+ * if the port was already initialised (eg, by a boot loader),
+ * try to determine the current setup.
+ */
+static void __init
+linflex_console_get_options(struct uart_port *sport, int *parity, int *bits)
+{
+ unsigned long cr;
+
+ cr = readl(sport->membase + UARTCR);
+ cr &= LINFLEXD_UARTCR_RXEN | LINFLEXD_UARTCR_TXEN;
+
+ if (!cr)
+ return;
+
+ /* ok, the port was enabled */
+
+ *parity = 'n';
+ if (cr & LINFLEXD_UARTCR_PCE) {
+ if (cr & LINFLEXD_UARTCR_PC0)
+ *parity = 'o';
+ else
+ *parity = 'e';
+ }
+
+ if ((cr & LINFLEXD_UARTCR_WL0) && ((cr & LINFLEXD_UARTCR_WL1) == 0)) {
+ if (cr & LINFLEXD_UARTCR_PCE)
+ *bits = 9;
+ else
+ *bits = 8;
+ }
+}
+
+static int __init linflex_console_setup(struct console *co, char *options)
+{
+ struct uart_port *sport;
+ int baud = 115200;
+ int bits = 8;
+ int parity = 'n';
+ int flow = 'n';
+ int ret;
+ int i;
+ unsigned long flags;
+ /*
+ * check whether an invalid uart number has been specified, and
+ * if so, search for the first available port that does have
+ * console support.
+ */
+ if (co->index == -1 || co->index >= ARRAY_SIZE(linflex_ports))
+ co->index = 0;
+
+ sport = linflex_ports[co->index];
+ if (!sport)
+ return -ENODEV;
+
+ if (options)
+ uart_parse_options(options, &baud, &parity, &bits, &flow);
+ else
+ linflex_console_get_options(sport, &parity, &bits);
+
+ if (earlycon_port && sport->mapbase == earlycon_port->mapbase) {
+ linflex_earlycon_same_instance = true;
+
+ spin_lock_irqsave(&init_lock, flags);
+ during_init = true;
+ spin_unlock_irqrestore(&init_lock, flags);
+
+ /* Workaround for character loss or output of many invalid
+ * characters, when INIT mode is entered shortly after a
+ * character has just been printed.
+ */
+ udelay(PREINIT_DELAY);
+ }
+
+ linflex_setup_watermark(sport);
+
+ ret = uart_set_options(sport, co, baud, parity, bits, flow);
+
+ if (!linflex_earlycon_same_instance)
+ goto done;
+
+ spin_lock_irqsave(&init_lock, flags);
+
+ /* Emptying buffer */
+ if (earlycon_buf.len) {
+ for (i = 0; i < earlycon_buf.len; i++)
+ linflex_console_putchar(earlycon_port,
+ earlycon_buf.content[i]);
+
+ kfree(earlycon_buf.content);
+ earlycon_buf.len = 0;
+ }
+
+ during_init = false;
+ spin_unlock_irqrestore(&init_lock, flags);
+
+done:
+ return ret;
+}
+
+static struct uart_driver linflex_reg;
+static struct console linflex_console = {
+ .name = DEV_NAME,
+ .write = linflex_console_write,
+ .device = uart_console_device,
+ .setup = linflex_console_setup,
+ .flags = CON_PRINTBUFFER,
+ .index = -1,
+ .data = &linflex_reg,
+};
+
+static void linflex_earlycon_write(struct console *con, const char *s,
+ unsigned int n)
+{
+ struct earlycon_device *dev = con->data;
+
+ uart_console_write(&dev->port, s, n, linflex_earlycon_putchar);
+}
+
+static int __init linflex_early_console_setup(struct earlycon_device *device,
+ const char *options)
+{
+ if (!device->port.membase)
+ return -ENODEV;
+
+ device->con->write = linflex_earlycon_write;
+ earlycon_port = &device->port;
+
+ return 0;
+}
+
+OF_EARLYCON_DECLARE(linflex, "fsl,s32v234-linflexuart",
+ linflex_early_console_setup);
+
+#define LINFLEX_CONSOLE (&linflex_console)
+#else
+#define LINFLEX_CONSOLE NULL
+#endif
+
+static struct uart_driver linflex_reg = {
+ .owner = THIS_MODULE,
+ .driver_name = DRIVER_NAME,
+ .dev_name = DEV_NAME,
+ .nr = ARRAY_SIZE(linflex_ports),
+ .cons = LINFLEX_CONSOLE,
+};
+
+static int linflex_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct uart_port *sport;
+ struct resource *res;
+ int ret;
+
+ sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
+ if (!sport)
+ return -ENOMEM;
+
+ ret = of_alias_get_id(np, "serial");
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret);
+ return ret;
+ }
+ if (ret >= UART_NR) {
+ dev_err(&pdev->dev, "driver limited to %d serial ports\n",
+ UART_NR);
+ return -ENOMEM;
+ }
+
+ sport->line = ret;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENODEV;
+
+ sport->mapbase = res->start;
+ sport->membase = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(sport->membase))
+ return PTR_ERR(sport->membase);
+
+ sport->dev = &pdev->dev;
+ sport->type = PORT_LINFLEXUART;
+ sport->iotype = UPIO_MEM;
+ sport->irq = platform_get_irq(pdev, 0);
+ sport->ops = &linflex_pops;
+ sport->flags = UPF_BOOT_AUTOCONF;
+
+ linflex_ports[sport->line] = sport;
+
+ platform_set_drvdata(pdev, sport);
+
+ ret = uart_add_one_port(&linflex_reg, sport);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int linflex_remove(struct platform_device *pdev)
+{
+ struct uart_port *sport = platform_get_drvdata(pdev);
+
+ uart_remove_one_port(&linflex_reg, sport);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int linflex_suspend(struct device *dev)
+{
+ struct uart_port *sport = dev_get_drvdata(dev);
+
+ uart_suspend_port(&linflex_reg, sport);
+
+ return 0;
+}
+
+static int linflex_resume(struct device *dev)
+{
+ struct uart_port *sport = dev_get_drvdata(dev);
+
+ uart_resume_port(&linflex_reg, sport);
+
+ return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(linflex_pm_ops, linflex_suspend, linflex_resume);
+
+static struct platform_driver linflex_driver = {
+ .probe = linflex_probe,
+ .remove = linflex_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ .owner = THIS_MODULE,
+ .of_match_table = linflex_dt_ids,
+ .pm = &linflex_pm_ops,
+ },
+};
+
+static int __init linflex_serial_init(void)
+{
+ int ret;
+
+ ret = uart_register_driver(&linflex_reg);
+ if (ret)
+ return ret;
+
+ ret = platform_driver_register(&linflex_driver);
+ if (ret)
+ uart_unregister_driver(&linflex_reg);
+
+#ifdef CONFIG_SERIAL_FSL_LINFLEXUART_CONSOLE
+ spin_lock_init(&init_lock);
+#endif
+
+ return ret;
+}
+
+static void __exit linflex_serial_exit(void)
+{
+ platform_driver_unregister(&linflex_driver);
+ uart_unregister_driver(&linflex_reg);
+}
+
+module_init(linflex_serial_init);
+module_exit(linflex_serial_exit);
+
+MODULE_DESCRIPTION("Freescale linflex serial port driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 5642c05e0da0..25a3dead4473 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -293,4 +293,7 @@
/* SiFive UART */
#define PORT_SIFIVE_V0 120
+/* Freescale Linflex UART */
+#define PORT_LINFLEXUART 121
+
#endif /* _UAPILINUX_SERIAL_CORE_H */
--
2.22.0
^ permalink raw reply related
* [PATCH 4/6] dt-bindings: serial: Document Freescale LINFlex UART
From: Stefan-gabriel Mirea @ 2019-08-02 19:47 UTC (permalink / raw)
To: corbet@lwn.net, robh+dt@kernel.org, mark.rutland@arm.com,
gregkh@linuxfoundation.org, catalin.marinas@arm.com,
will@kernel.org, shawnguo@kernel.org, Leo Li
Cc: jslaby@suse.com, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Cosmin Stefan Stoica,
Larisa Ileana Grigore
In-Reply-To: <20190802194702.30249-1-stefan-gabriel.mirea@nxp.com>
From: Stoica Cosmin-Stefan <cosmin.stoica@nxp.com>
Add documentation for the serial communication interface module (LINFlex),
found in two instances on S32V234.
Signed-off-by: Stoica Cosmin-Stefan <cosmin.stoica@nxp.com>
Signed-off-by: Larisa Grigore <Larisa.Grigore@nxp.com>
Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
---
.../bindings/serial/fsl,s32-linflexuart.txt | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.txt
diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.txt b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.txt
new file mode 100644
index 000000000000..957ffeaca9f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.txt
@@ -0,0 +1,24 @@
+* Freescale Linflex UART
+
+The LINFlexD controller implements several LIN protocol versions, as well as
+support for full-duplex UART communication through 8-bit and 9-bit frames. The
+Linflex UART driver enables operation only in UART mode.
+
+See chapter 47 ("LINFlexD") in the reference manual[1].
+
+Required properties:
+- compatible :
+ - "fsl,s32-linflexuart" for linflex configured in uart mode which
+ is compatible with the one integrated on S32V234 SoC
+- reg : Address and length of the register set for the device
+- interrupts : Should contain uart interrupt
+
+Example:
+uart0:serial@40053000 {
+ compatible = "fsl,s32-linflexuart";
+ reg = <0x0 0x40053000 0x0 0x1000>;
+ interrupts = <0 59 4>;
+ status = "disabled";
+};
+
+[1] https://www.nxp.com/webapp/Download?colCode=S32V234RM
--
2.22.0
^ permalink raw reply related
* [PATCH 3/6] arm64: dts: fsl: Add device tree for S32V234-EVB
From: Stefan-gabriel Mirea @ 2019-08-02 19:47 UTC (permalink / raw)
To: corbet@lwn.net, robh+dt@kernel.org, mark.rutland@arm.com,
gregkh@linuxfoundation.org, catalin.marinas@arm.com,
will@kernel.org, shawnguo@kernel.org, Leo Li
Cc: jslaby@suse.com, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Cosmin Stefan Stoica,
Dan Nica, Larisa Ileana Grigore
In-Reply-To: <20190802194702.30249-1-stefan-gabriel.mirea@nxp.com>
From: Stoica Cosmin-Stefan <cosmin.stoica@nxp.com>
Add initial version of device tree for S32V234-EVB, including nodes for the
4 Cortex-A53 cores, AIPS bus with UART modules, ARM architected timer and
Generic Interrupt Controller (GIC).
Keep SoC level separate from board level to let future boards with this SoC
share common properties, while the dts files will keep board-dependent
properties.
Signed-off-by: Stoica Cosmin-Stefan <cosmin.stoica@nxp.com>
Signed-off-by: Mihaela Martinas <Mihaela.Martinas@freescale.com>
Signed-off-by: Dan Nica <dan.nica@nxp.com>
Signed-off-by: Larisa Grigore <Larisa.Grigore@nxp.com>
Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
---
arch/arm64/boot/dts/freescale/Makefile | 2 +
.../boot/dts/freescale/fsl-s32v234-evb.dts | 20 +++
.../arm64/boot/dts/freescale/fsl-s32v234.dtsi | 130 ++++++++++++++++++
3 files changed, 152 insertions(+)
create mode 100644 arch/arm64/boot/dts/freescale/fsl-s32v234-evb.dts
create mode 100644 arch/arm64/boot/dts/freescale/fsl-s32v234.dtsi
diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
index c043aca66572..3af29b58a833 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -26,3 +26,5 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb
+
+dtb-$(CONFIG_ARCH_S32) += fsl-s32v234-evb.dtb
diff --git a/arch/arm64/boot/dts/freescale/fsl-s32v234-evb.dts b/arch/arm64/boot/dts/freescale/fsl-s32v234-evb.dts
new file mode 100644
index 000000000000..9b3983402998
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/fsl-s32v234-evb.dts
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2015-2016 Freescale Semiconductor, Inc.
+ * Copyright 2016-2017 NXP
+ */
+
+/dts-v1/;
+#include "fsl-s32v234.dtsi"
+
+/ {
+ compatible = "fsl,s32v234-evb", "fsl,s32v234";
+};
+
+&uart0 {
+ status = "okay";
+};
+
+&uart1 {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/freescale/fsl-s32v234.dtsi b/arch/arm64/boot/dts/freescale/fsl-s32v234.dtsi
new file mode 100644
index 000000000000..6d686d3ba997
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/fsl-s32v234.dtsi
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2015-2016 Freescale Semiconductor, Inc.
+ * Copyright 2016-2018 NXP
+ */
+
+/memreserve/ 0x80000000 0x00010000;
+
+/ {
+ model = "Freescale S32V234";
+ compatible = "fsl,s32v234";
+ interrupt-parent = <&gic>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ aliases {
+ serial0 = &uart0;
+ serial1 = &uart1;
+ };
+
+ cpus {
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ cpu0: cpu@0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x0 0x0>;
+ enable-method = "spin-table";
+ cpu-release-addr = <0x0 0x80000000>;
+ next-level-cache = <&cluster0_l2_cache>;
+ };
+ cpu1: cpu@1 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x0 0x1>;
+ enable-method = "spin-table";
+ cpu-release-addr = <0x0 0x80000000>;
+ next-level-cache = <&cluster0_l2_cache>;
+ };
+ cpu2: cpu@100 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x0 0x100>;
+ enable-method = "spin-table";
+ cpu-release-addr = <0x0 0x80000000>;
+ next-level-cache = <&cluster1_l2_cache>;
+ };
+ cpu3: cpu@101 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x0 0x101>;
+ enable-method = "spin-table";
+ cpu-release-addr = <0x0 0x80000000>;
+ next-level-cache = <&cluster1_l2_cache>;
+ };
+
+ cluster0_l2_cache: l2-cache0 {
+ compatible = "cache";
+ };
+
+ cluster1_l2_cache: l2-cache1 {
+ compatible = "cache";
+ };
+ };
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ compatible = "simple-bus";
+ interrupt-parent = <&gic>;
+ ranges;
+
+ aips0: aips-bus@40000000 {
+ compatible = "simple-bus";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ interrupt-parent = <&gic>;
+ reg = <0x0 0x40000000 0x0 0x7D000>;
+ ranges;
+
+ uart0: serial@40053000 {
+ compatible = "fsl,s32-linflexuart";
+ reg = <0x0 0x40053000 0x0 0x1000>;
+ interrupts = <0 59 1>;
+ status = "disabled";
+ };
+ };
+
+ aips1: aips-bus@40080000 {
+ compatible = "simple-bus";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ interrupt-parent = <&gic>;
+ reg = <0x0 0x40080000 0x0 0x70000>;
+ ranges;
+
+ uart1: serial@400bc000 {
+ compatible = "fsl,s32-linflexuart";
+ reg = <0x0 0x400bc000 0x0 0x1000>;
+ interrupts = <0 60 1>;
+ status = "disabled";
+ };
+ };
+ };
+
+ timer {
+ compatible = "arm,armv8-timer";
+ interrupts = <1 13 0xf08>,
+ <1 14 0xf08>,
+ <1 11 0xf08>,
+ <1 10 0xf08>;
+ /* clock-frequency might be modified by u-boot, depending on the
+ * chip version.
+ */
+ clock-frequency = <10000000>;
+ };
+
+ gic: interrupt-controller@7d001000 {
+ compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
+ #interrupt-cells = <3>;
+ #address-cells = <0>;
+ interrupt-controller;
+ reg = <0 0x7d001000 0 0x1000>,
+ <0 0x7d002000 0 0x2000>,
+ <0 0x7d004000 0 0x2000>,
+ <0 0x7d006000 0 0x2000>;
+ interrupts = <1 9 0xf04>;
+ };
+};
--
2.22.0
^ permalink raw reply related
* [PATCH 2/6] arm64: Introduce config for S32
From: Stefan-gabriel Mirea @ 2019-08-02 19:47 UTC (permalink / raw)
To: corbet@lwn.net, robh+dt@kernel.org, mark.rutland@arm.com,
gregkh@linuxfoundation.org, catalin.marinas@arm.com,
will@kernel.org, shawnguo@kernel.org, Leo Li
Cc: jslaby@suse.com, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Cosmin Stefan Stoica
In-Reply-To: <20190802194702.30249-1-stefan-gabriel.mirea@nxp.com>
From: Mihaela Martinas <Mihaela.Martinas@freescale.com>
Add configuration option for the Freescale S32 platform family in
Kconfig.platforms. For starters, the only SoC supported will be Treerunner
(S32V234), with a single execution target: the S32V234-EVB (rev 29288)
board.
Signed-off-by: Mihaela Martinas <Mihaela.Martinas@freescale.com>
Signed-off-by: Stoica Cosmin-Stefan <cosmin.stoica@nxp.com>
Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
---
arch/arm64/Kconfig.platforms | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 4778c775de1b..a9a6152d37eb 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -210,6 +210,11 @@ config ARCH_ROCKCHIP
This enables support for the ARMv8 based Rockchip chipsets,
like the RK3368.
+config ARCH_S32
+ bool "Freescale S32 SoC Family"
+ help
+ This enables support for the Freescale S32 family of processors.
+
config ARCH_SEATTLE
bool "AMD Seattle SoC Family"
help
--
2.22.0
^ permalink raw reply related
* [PATCH 1/6] dt-bindings: arm: fsl: Add the S32V234-EVB board
From: Stefan-gabriel Mirea @ 2019-08-02 19:47 UTC (permalink / raw)
To: corbet@lwn.net, robh+dt@kernel.org, mark.rutland@arm.com,
gregkh@linuxfoundation.org, catalin.marinas@arm.com,
will@kernel.org, shawnguo@kernel.org, Leo Li
Cc: jslaby@suse.com, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Eddy Petrisor
In-Reply-To: <20190802194702.30249-1-stefan-gabriel.mirea@nxp.com>
From: Eddy Petrișor <eddy.petrisor@nxp.com>
Add entry for the NXP S32V234 Customer Evaluation Board to the board/SoC
bindings.
Signed-off-by: Eddy Petrișor <eddy.petrisor@nxp.com>
Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
---
Documentation/devicetree/bindings/arm/fsl.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
index 7294ac36f4c0..104d60a11177 100644
--- a/Documentation/devicetree/bindings/arm/fsl.yaml
+++ b/Documentation/devicetree/bindings/arm/fsl.yaml
@@ -309,4 +309,10 @@ properties:
- fsl,ls2088a-rdb
- const: fsl,ls2088a
+ - description: S32V234 Customer Evaluation Board
+ items:
+ - enum:
+ - fsl,s32v234-evb
+ - const: fsl,s32v234
+
...
--
2.22.0
^ permalink raw reply related
* [PATCH 0/6] Add initial support for S32V234-EVB
From: Stefan-gabriel Mirea @ 2019-08-02 19:47 UTC (permalink / raw)
To: corbet@lwn.net, robh+dt@kernel.org, mark.rutland@arm.com,
gregkh@linuxfoundation.org, catalin.marinas@arm.com,
will@kernel.org, shawnguo@kernel.org, Leo Li
Cc: jslaby@suse.com, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Hello,
NXP's S32V234[1] ("Treerunner") vision microprocessors are targeted for
high-performance, computationally intensive vision and sensor fusion
applications that require automotive safety levels. They include leading
edge Camera Vision modules like APEX-2, ISP and GPU. The S32V234-EVB and
S32V234-SBC boards are available for customer evaluation.
The following patch series introduces minimal enablement support for the
NXP S32V234-EVB2[2] board, which leverages most of the SoC capabilities.
The series includes a driver for operating the on-chip LINFlexD controller
in UART mode.
In the future, we aim to submit multiple drivers upstream, which can be
found in the kernel of our Auto Linux BSP[3] ("ALB"), starting with basic
pinmuxing, clock and uSDHC drivers.
For validation, you can use the U-Boot bootloader in the ALB[4], which we
build and test with our patched version of the Linaro GCC 6.3.1 2017.05
toolchain for ARM 64-bit, with sources available on [5].
[1] https://www.nxp.com/products/processors-and-microcontrollers/arm-based-processors-and-mcus/s32-automotive-platform/vision-processor-for-front-and-surround-view-camera-machine-learning-and-sensor-fusion:S32V234
[2] https://www.nxp.com/support/developer-resources/evaluation-and-development-boards/ultra-reliable-dev-platforms/s32v-mpus-platforms/s32v-vision-and-sensor-fusion-evaluation-system:S32V234EVB
[3] https://source.codeaurora.org/external/autobsps32/linux/
[4] https://source.codeaurora.org/external/autobsps32/u-boot/
[5] https://source.codeaurora.org/external/s32ds/compiler/gcc/
Eddy Petrișor (1):
dt-bindings: arm: fsl: Add the S32V234-EVB board
Mihaela Martinas (2):
arm64: Introduce config for S32
arm64: defconfig: Enable configs for S32V234
Stefan-Gabriel Mirea (1):
tty: serial: Add linflexuart driver for S32V234
Stoica Cosmin-Stefan (2):
arm64: dts: fsl: Add device tree for S32V234-EVB
dt-bindings: serial: Document Freescale LINFlex UART
.../admin-guide/kernel-parameters.txt | 6 +
.../devicetree/bindings/arm/fsl.yaml | 6 +
.../bindings/serial/fsl,s32-linflexuart.txt | 24 +
arch/arm64/Kconfig.platforms | 5 +
arch/arm64/boot/dts/freescale/Makefile | 2 +
.../boot/dts/freescale/fsl-s32v234-evb.dts | 20 +
.../arm64/boot/dts/freescale/fsl-s32v234.dtsi | 130 +++
arch/arm64/configs/defconfig | 3 +
drivers/tty/serial/Kconfig | 15 +
drivers/tty/serial/Makefile | 1 +
drivers/tty/serial/fsl_linflexuart.c | 956 ++++++++++++++++++
include/uapi/linux/serial_core.h | 3 +
12 files changed, 1171 insertions(+)
create mode 100644 Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.txt
create mode 100644 arch/arm64/boot/dts/freescale/fsl-s32v234-evb.dts
create mode 100644 arch/arm64/boot/dts/freescale/fsl-s32v234.dtsi
create mode 100644 drivers/tty/serial/fsl_linflexuart.c
--
2.22.0
^ permalink raw reply
* Re: [PATCH v4 4/4] net: phy: realtek: configure RTL8211E LEDs
From: Matthias Kaehlcke @ 2019-08-02 19:40 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190802181840.GP2099@lunn.ch>
Hi Andrew,
On Fri, Aug 02, 2019 at 08:18:40PM +0200, Andrew Lunn wrote:
> On Thu, Aug 01, 2019 at 12:07:59PM -0700, Matthias Kaehlcke wrote:
> > Configure the RTL8211E LEDs behavior when the device tree property
> > 'realtek,led-modes' is specified.
note to self: update commit message
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>
> Hi Matthias
>
> I was more thinking of adding a new driver call to the PHY driver API,
> to configure an LED. Something like
>
> rtl8211e_config_leds(phydev, int led, struct phy_led_config cfg);
I guess it sould be singular ('_config_led') if it configures a single
LED.
> It would be called by the phylib core after config_init(). But also,
> thinking ahead to generic linux LED support, it could be called later
> to reconfigure the LEDs to use a different trigger. The standard LED
> sysfs interface would be used.
I'll look into the phylib part.
Thanks
Matthias
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni @ 2019-08-02 18:43 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <c703b4fc-9ebb-0fd4-11de-80974b5c3842@gmail.com>
On 8/2/19 5:32 AM, Dmitry Osipenko wrote:
> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>> This patch implements save and restore context for peripheral fixed
>> clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
>> peripheral clock ops.
>>
>> During system suspend, core power goes off and looses the settings
>> of the Tegra CAR controller registers.
>>
>> So during suspend entry clock and reset state of peripherals is saved
>> and on resume they are restored to have clocks back to same rate and
>> state as before suspend.
>>
>> Acked-by: Thierry Reding <treding@nvidia.com>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>> drivers/clk/tegra/clk-periph-fixed.c | 33 ++++++++++++++++++++++++++++++++
>> drivers/clk/tegra/clk-periph-gate.c | 34 +++++++++++++++++++++++++++++++++
>> drivers/clk/tegra/clk-periph.c | 37 ++++++++++++++++++++++++++++++++++++
>> drivers/clk/tegra/clk-sdmmc-mux.c | 28 +++++++++++++++++++++++++++
>> drivers/clk/tegra/clk.h | 6 ++++++
>> 5 files changed, 138 insertions(+)
>>
>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c b/drivers/clk/tegra/clk-periph-fixed.c
>> index c088e7a280df..21b24530fa00 100644
>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>> @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct clk_hw *hw,
>> return (unsigned long)rate;
>> }
>>
>> +static int tegra_clk_periph_fixed_save_context(struct clk_hw *hw)
>> +{
>> + struct tegra_clk_periph_fixed *fixed = to_tegra_clk_periph_fixed(hw);
>> + u32 mask = 1 << (fixed->num % 32);
> This could be BIT(fixed->num % 32).
>
>> + fixed->enb_ctx = readl_relaxed(fixed->base + fixed->regs->enb_reg) &
>> + mask;
>> + fixed->rst_ctx = readl_relaxed(fixed->base + fixed->regs->rst_reg) &
>> + mask;
> The enb_ctx/rst_ctx are booleans, while you assigning an integer value
> here. You're getting away here because bool is an 32bit unsigned int,
> but you shouldn't rely on it and always explicitly convert to a bool.
>
>> + return 0;
>> +}
>> +
>> +static void tegra_clk_periph_fixed_restore_context(struct clk_hw *hw)
>> +{
>> + struct tegra_clk_periph_fixed *fixed = to_tegra_clk_periph_fixed(hw);
>> + u32 mask = 1 << (fixed->num % 32);
>> +
>> + if (fixed->enb_ctx)
>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_set_reg);
>> + else
>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_clr_reg);
>> +
>> + udelay(2);
> Will be better to read out and compare the hardware's state with the
> restored one, then bail out if the state is unchanged.
>
> Shouldn't it be fence_udelay()?
>
>> + if (!fixed->rst_ctx) {
>> + udelay(5); /* reset propogation delay */
> Why delaying is done before the writing to the reset register?
During SC7 exit, peripheral reset state is set to POR state. So some
peripherals will already be in reset state and making sure of
propagation delay before releasing from reset.
It should be rst_clr_reg. will fix in next rev
>
>> + writel_relaxed(mask, fixed->base + fixed->regs->rst_reg);
> I'm not quite sure what's going on here, this looks wrong.
>
> 1. rst_reg points to RST_DEVICES_x
> 2. Each bit of RST_DEVICES_x represents the reset-assertion state of
> each individual device
> 3. By writing to rst_reg, all (!) devices are deasserted, except the one
> device which corresponds to the mask
> 4. The reset is asserted for a single device, while !fixed->rst_ctx
> means that it actually should be deasserted (?)
>
> Apparently you should use rst_set_reg / rst_clr_reg.
Yes, It should be rst_clr_reg. will fix in next rev
>> + }
> What about the case where rst_ctx=true?
ON SC7 exit, state of RST_DEV will be POR state where most peripherals
will already be in reset state.
Few of them which are not in reset state in POR values are those that
need to stay de-asserted across the boot states anyway.
>
>> +}
>> @@ -517,6 +517,8 @@ struct tegra_clk_periph_gate {
>> int clk_num;
>> int *enable_refcnt;
>> const struct tegra_clk_periph_regs *regs;
>> + bool clk_state_ctx;
>> + bool rst_state_ctx;
>> };
>>
>> #define to_clk_periph_gate(_hw) \
>> @@ -543,6 +545,8 @@ struct tegra_clk_periph_fixed {
>> unsigned int mul;
>> unsigned int div;
>> unsigned int num;
>> + bool enb_ctx;
>> + bool rst_ctx;
>> };
> I'd expect these to be bool:1.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni @ 2019-08-02 18:33 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <8bca50b2-a78c-c6b1-6547-4cec98a3e9cb@gmail.com>
On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>> This patch implements save and restore context for peripheral
>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>> and on resume they are restored to have clocks back to same
>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>> @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> +static int tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +static void tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> static const struct clk_ops tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>> .is_enabled = tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>> .recalc_rate = tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>> };
>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_fixed(const char
>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>> readl_relaxed(gate->clk_base + (gate->regs->rst_reg))
>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct clk_hw
>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct clk_hw
>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>> };
>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> +static int clk_periph_save_context(struct clk_hw *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>> Could you please point to where the divider's save_context()
>>>>>>>>>>>>> happens?
>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers context
>>>>>>>>>>>> because
>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>> restoring
>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>
>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>> generic helper to get the index instead of storing it manually.
>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>
>>>>>>>>>>> All existing drivers are using directly get_parent() from clk_mux
>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>
>>>>>>>>>>> To have this more generic w.r.t save/restore context point of
>>>>>>>>>>> view,
>>>>>>>>>>> probably instead of implementing new get_parent_index helper,
>>>>>>>>>>> I think
>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>> clk_mux to
>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>
>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>> restore_context.
>>>>>>>>>>>
>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops to be
>>>>>>>>>> more
>>>>>>>>>> generic w.r.t save/restore context rather than get_parent_index
>>>>>>>>>> API.
>>>>>>>>>> Please confirm if you agree.
>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>> helper for
>>>>>>>>> the generic clk_gate, seems something similar could be done for the
>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the parent
>>>>>>>>> clock
>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>> by 'restoring' helper for generic clk_gate, are you referring to
>>>>>>>> clk_gate_restore_context API?
>>>>>>> Yes.
>>>>>>>
>>>>>>>> clk_gate_restore_context is API that's any clk drivers can use for
>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>
>>>>>>>> But clk-periph is directly using generic clk_mux ops from clk_mux
>>>>>>>> so I
>>>>>>>> think we should add .restore_context to clk_mux_ops and then during
>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>> generic
>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>> function
>>>>>>> that any driver could use in order to restore the clock's parent.
>>>>>>>
>>>>>>> The clk-periph restoring also includes case of combining divider and
>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>> as well.
>>>>>>>
>>>>>>> It also looks like you could actually use the
>>>>>>> clk_gate_restore_context()
>>>>>>> instead of manually saving the clock's enable-state, couldn't you?
>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API rather
>>>>>> than
>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>> clk_periph
>>>>>> restore.
>>>>>>
>> digging thru looks like for clk_periph source restore instead of
>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent anyway.
>> Will do this for periph clk mux
>>>>>> Reg clk_gate, looks like we cant use generic clk_gate_restore_context
>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>> clk_periph_enable/disable in clk-periph-gate also updated refcnt and
>>>>>> depending on that actual enable/disable is set.
>>>>>>
>>>>>> During suspend, peripherals that are already enabled have their
>>>>>> refcnt >
>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>> enable/disable callback.
>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>> save_context, wouldn't that work?
>>>>>
>>> gate->enable_refcnt is within clk-periph-gate which gets updated when
>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>> But actual enable_count used in clk_gate_restore_context is the one
>>> which gets updated with in the clk core enable/disable functions which
>>> invokes these callbacks. Depending on this enable_count in clk core it
>>> invokes enable/disable.
>>>
>>> So, this will cause mismatch if we handle refcnt during save/restore
>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>> clk_gate_restore_context is based on enable_count from clk core.
>>>
>>>>>> Also to align exact reset state along with CLK (like for case where
>>>>>> CLK
>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>> I'm wondering whether instead of saving/restoring reset-state of every
>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET register.
>>>>> Couldn't you?
>>>> Thats what I was doing in first version of patch. But later as we
>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>> after source restore
>>>
>>> Also, to align both CLK & RST to the exact state of register, doing
>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>> source restore for peripheral clock, seems cleaner to avoid any
>>> misconfiguration b/w rst & clk settings.
>>>
> It looks to me that it is very wasteful to store/restore each individual
> gate and reset state, also given that some of them are shared. I think
> that the gates and resets should be restored separately for the
> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
clk_periph_fixed_disable just disables clock only without deasserting
the corresponding peripheral.
corresponding peripheral drivers can also issue reset assert/deassert
thru reset_control_assert/deassert.
So, we will not get the actual state of clk and rst unless we read and
save state of reset and clock separately during save_context.
Currently patch is already using custom
tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
register settings instead of individual peripheral bits?
^ permalink raw reply
* Re: [PATCH v4 1/4] dt-bindings: net: phy: Add subnode for LED configuration
From: Matthias Kaehlcke @ 2019-08-02 18:27 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190802165755.GM2099@lunn.ch>
On Fri, Aug 02, 2019 at 06:57:55PM +0200, Andrew Lunn wrote:
> On Thu, Aug 01, 2019 at 12:07:56PM -0700, Matthias Kaehlcke wrote:
> > The LED behavior of some Ethernet PHYs is configurable. Add an
> > optional 'leds' subnode with a child node for each LED to be
> > configured. The binding aims to be compatible with the common
> > LED binding (see devicetree/bindings/leds/common.txt).
> >
> > A LED can be configured to be 'on' when a link with a certain speed
> > is active, or to blink on RX/TX activity. For the configuration to
> > be effective it needs to be supported by the hardware and the
> > corresponding PHY driver.
> >
> > Suggested-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v4:
> > - patch added to the series
> > ---
> > .../devicetree/bindings/net/ethernet-phy.yaml | 47 +++++++++++++++++++
> > 1 file changed, 47 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > index f70f18ff821f..81c5aacc89a5 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > @@ -153,6 +153,38 @@ properties:
> > Delay after the reset was deasserted in microseconds. If
> > this property is missing the delay will be skipped.
> >
> > +patternProperties:
> > + "^leds$":
> > + type: object
> > + description:
> > + Subnode with configuration of the PHY LEDs.
> > +
> > + patternProperties:
> > + "^led@[0-9]+$":
> > + type: object
> > + description:
> > + Subnode with the configuration of a single PHY LED.
> > +
> > + properties:
> > + reg:
> > + description:
> > + The ID number of the LED, typically corresponds to a hardware ID.
> > + $ref: "/schemas/types.yaml#/definitions/uint32"
> > +
> > + linux,default-trigger:
> > + description:
> > + This parameter, if present, is a string specifying the trigger
> > + assigned to the LED. Supported triggers are:
> > + "phy_link_10m_active" - LED will be on when a 10Mb/s link is active
> > + "phy_link_100m_active" - LED will be on when a 100Mb/s link is active
> > + "phy_link_1g_active" - LED will be on when a 1Gb/s link is active
> > + "phy_link_10g_active" - LED will be on when a 10Gb/s link is active
> > + "phy_activity" - LED will blink when data is received or transmitted
>
> Matthias
>
> We should think a bit more about these names.
>
> I can see in future needing 1G link, but it blinks off when there is
> active traffic? So phy_link_1g_active could be confusing, and very similar to
> phy_link_1g_activity?
agreed, the 'active' vs' 'activity' can be confusing, let's avoid that.
> So maybe
> > + "phy_link_10m" - LED will be solid on when a 10Mb/s link is active
> > + "phy_link_100m" - LED will be solid on when a 100Mb/s link is active
> > + "phy_link_1g" - LED will be solid on when a 1Gb/s link is active
>
> etc.
>
> And then in the future we can have
>
> "phy_link_1g_activity' - LED will be on when 1Gbp/s
> link is active and blink off
> with activity.
sounds good to me
> What other use cases do we have? I don't want to support everything,
> but we should be able to represent the most common modes without the
> names getting too confusing.
Initially I planned to support to configure a LED to be solid for
multiple link speeds, however that could become a bit messy with the
string based triggers, unless we limit the possible combinations. My
expertise in network land is limited, so I'm not sure if that's an
important/realistic use case.
^ permalink raw reply
* Re: [PATCH v4 4/4] net: phy: realtek: configure RTL8211E LEDs
From: Andrew Lunn @ 2019-08-02 18:18 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190801190759.28201-5-mka@chromium.org>
On Thu, Aug 01, 2019 at 12:07:59PM -0700, Matthias Kaehlcke wrote:
> Configure the RTL8211E LEDs behavior when the device tree property
> 'realtek,led-modes' is specified.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Hi Matthias
I was more thinking of adding a new driver call to the PHY driver API,
to configure an LED. Something like
rtl8211e_config_leds(phydev, int led, struct phy_led_config cfg);
It would be called by the phylib core after config_init(). But also,
thinking ahead to generic linux LED support, it could be called later
to reconfigure the LEDs to use a different trigger. The standard LED
sysfs interface would be used.
Andrew
^ permalink raw reply
* Re: [PATCH v4 2/4] net: phy: Add function to retrieve LED configuration from the DT
From: Matthias Kaehlcke @ 2019-08-02 17:59 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190802163810.GL2099@lunn.ch>
On Fri, Aug 02, 2019 at 06:38:10PM +0200, Andrew Lunn wrote:
> On Thu, Aug 01, 2019 at 12:07:57PM -0700, Matthias Kaehlcke wrote:
> > Add a phylib function for retrieving PHY LED configuration that
> > is specified in the device tree using the generic binding. LEDs
> > can be configured to be 'on' for a certain link speed or to blink
> > when there is TX/RX activity.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v4:
> > - patch added to the series
> > ---
> > drivers/net/phy/phy_device.c | 50 ++++++++++++++++++++++++++++++++++++
> > include/linux/phy.h | 15 +++++++++++
> > 2 files changed, 65 insertions(+)
> >
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 6b5cb87f3866..b4b48de45712 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -2188,6 +2188,56 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
> > return phydrv->config_intr && phydrv->ack_interrupt;
> > }
> >
> > +int of_get_phy_led_cfg(struct phy_device *phydev, int led,
> > + struct phy_led_config *cfg)
> > +{
> > + struct device_node *np, *child;
> > + const char *trigger;
> > + int ret;
> > +
> > + if (!IS_ENABLED(CONFIG_OF_MDIO))
> > + return -ENOENT;
> > +
> > + np = of_find_node_by_name(phydev->mdio.dev.of_node, "leds");
> > + if (!np)
> > + return -ENOENT;
> > +
> > + for_each_child_of_node(np, child) {
> > + u32 val;
> > +
> > + if (!of_property_read_u32(child, "reg", &val)) {
> > + if (val == (u32)led)
> > + break;
> > + }
> > + }
>
> Hi Matthias
>
> This is leaking references to np and child. In the past we have not
> cared about this too much, but we are now getting patches adding the
> missing releases. So it would be good to fix this.
Good point, I'll fix it in the next revision.
Thanks
Matthias
^ 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