* Re: [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Rob Herring @ 2019-07-16 15:04 UTC (permalink / raw)
To: Joel Stanley
Cc: devicetree, Linus Walleij, Andrew Jeffery, linux-aspeed,
open list:GPIO SUBSYSTEM, Linux ARM
In-Reply-To: <CACPK8Xdz98CQzgE2KCjz8eOhPtx=H8jTe1hVT7LvP77U_gGASQ@mail.gmail.com>
On Mon, Jul 15, 2019 at 5:17 PM Joel Stanley <joel@jms.id.au> wrote:
>
> On Mon, 15 Jul 2019 at 22:37, Rob Herring <robh@kernel.org> wrote:
> >
> > The Aspeed pinctl schema have errors in the 'compatible' schema:
> >
> > Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> > properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed', 'g4-pinctrl'] has non-unique elements
> > Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> > properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed', 'g5-pinctrl'] has non-unique elements
> >
> > Flow style sequences have to be quoted if the vales contain ','. Fix
> > this by using the more common one line per entry formatting.
>
> >
> > properties:
> > compatible:
> > - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> > + enum:
> > + - aspeed,ast2400-pinctrl
> > + - aspeed,g4-pinctrl
>
> Thanks for the fix. However, we've standardised on the first form for
> all of our device trees, so we can drop the second compatible string
> from the bindings.
Doing that would introduce validation warnings until the dts file is
updated. So we still need this change until that happens.
Rob
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 16:50 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <d908d3a2-3013-7f92-0852-115f428d1c5f@gmail.com>
On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
> 16.07.2019 11:06, Peter De Schrijver пишет:
>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>> OK, Will add to CPUFreq driver...
>>>>> The other thing that also need attention is that T124 CPUFreq driver
>>>>> implicitly relies on DFLL driver to be probed first, which is icky.
>>>>>
>>>> Should I add check for successful dfll clk register explicitly in
>>>> CPUFreq driver probe and defer till dfll clk registers?
> Probably you should use the "device links". See [1][2] for the example.
>
> [1]
> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>
> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>
> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
> use of_find_device_by_node() to get the DFLL's device, see [3].
>
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
Will go thru and add...
>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
>>>
>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>> integrated with DVFS control logic with the regulator. We will not switch
>>> CPU to other clock sources once we switched to DFLL. Because the CPU has
>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table you see
>>> in the driver.). We shouldn't reparent it to other sources with unknew
>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>> open-loop mode but different sources.
> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
> rate before switching to PLLP in order to have a proper CPU voltage.
PLLP freq is safe to work for any CPU voltage. So no need to enforce
DFLL freq to PLLP rate before changing CCLK_G source to PLLP during suspend
>>> And I don't exactly understand why we need to switch to PLLP in CPU idle
>>> driver. Just keep it on CL-DVFS mode all the time.
>>>
>>> In SC7 entry, the dfll suspend function moves it the open-loop mode. That's
>>> all. The sc7-entryfirmware will handle the rest of the sequence to turn off
>>> the CPU power.
>>>
>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>> resuming to the kernel, we re-init DFLL, restore the CPU clock policy (CPU
>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
> The DFLL is re-inited after switching CCLK to DFLL parent during of the
> early clocks-state restoring by CaR driver. Hence instead of having odd
> hacks in the CaR driver, it is much nicer to have a proper
> suspend-resume sequencing of the device drivers. In this case CPUFreq
> driver is the driver that enables DFLL and switches CPU to that clock
> source, which means that this driver is also should be responsible for
> management of the DFLL's state during of suspend/resume process. If
> CPUFreq driver disables DFLL during suspend and re-enables it during
> resume, then looks like the CaR driver hacks around DFLL are not needed.
>
>>> The DFLL part looks good to me. BTW, change the patch subject to "Add
>>> suspend-resume support" seems more appropriate to me.
>>>
>> To clarify this, the sequences for DFLL use are as follows (assuming all
>> required DFLL hw configuration has been done)
>>
>> Switch to DFLL:
>> 0) Save current parent and frequency
>> 1) Program DFLL to open loop mode
>> 2) Enable DFLL
>> 3) Change cclk_g parent to DFLL
>> For OVR regulator:
>> 4) Change PWM output pin from tristate to output
>> 5) Enable DFLL PWM output
>> For I2C regulator:
>> 4) Enable DFLL I2C output
>> 6) Program DFLL to closed loop mode
>>
>> Switch away from DFLL:
>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any vdd_cpu voltage
>> 1) Program DFLL to open loop mode
>>
>> For OVR regulator:
>> 2) Change PWM output pin from output to tristate: vdd_cpu will go back
>> to hardwired boot voltage.
>> 3) Disable DFLL PWM output
>>
>> For I2C regulator:
>> 2) Program vdd_cpu regulator voltage to the boot voltage
>> 3) Disable DFLL I2C output
>>
>> 4) Reprogram parent saved in step 0 of 'Switch to DFLL' to the saved
>> frequency
>> 5) Change cclk_g parent to saved parent
>> 6) Disable DFLL
This is the same sequence currently implemented. But dfll suspend/resume
calls are thru Tegra210 clock driver.
Dmitry wants to have dfll suspend/resume along with CCLK_G restore to
happen from CPUFreq driver pm_ops rather than tegra210 clock driver or
tegra dfll driver.
Will move it to CPUFreq driver...
> Thanks!
^ permalink raw reply
* Re: [PATCH] gpio: of: Break out OF-only code
From: Andy Shevchenko @ 2019-07-16 17:00 UTC (permalink / raw)
To: Linus Walleij; +Cc: linux-gpio, Bartosz Golaszewski
In-Reply-To: <20190716123918.26004-1-linus.walleij@linaro.org>
On Tue, Jul 16, 2019 at 02:39:18PM +0200, Linus Walleij wrote:
> The core gpiolib should not contain any OF/device tree-only
> code. Try to break out the main part of it and push it down
> into the optional gpiolib-of.c part of the library.
>
> Create a local gpiolib-of.h header and move stuff around a
> bit to get a clean cut.
Good one!
One question below, though.
> drivers/gpio/gpiolib-of.c | 114 ++++++++++++++++++++++++++++++++++++++
> drivers/gpio/gpiolib.c | 113 +++----------------------------------
> drivers/gpio/gpiolib.h | 27 ---------
> 3 files changed, 122 insertions(+), 132 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 2bc0bcd7a410..00ed60304cd4 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -21,6 +21,34 @@
> #include <linux/gpio/machine.h>
>
> #include "gpiolib.h"
> +#include "gpiolib-of.h"
Did I miss something? Where is the file?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 18:19 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <72b5df8c-8acb-d0d0-ebcf-b406e8404973@nvidia.com>
On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>
> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>> OK, Will add to CPUFreq driver...
>>>>>> The other thing that also need attention is that T124 CPUFreq driver
>>>>>> implicitly relies on DFLL driver to be probed first, which is icky.
>>>>>>
>>>>> Should I add check for successful dfll clk register explicitly in
>>>>> CPUFreq driver probe and defer till dfll clk registers?
>> Probably you should use the "device links". See [1][2] for the example.
>>
>> [1]
>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>
>>
>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>
>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>
>> [3]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>
> Will go thru and add...
>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
>>>>
>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>> integrated with DVFS control logic with the regulator. We will not
>>>> switch
>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>> CPU has
>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
>>>> you see
>>>> in the driver.). We shouldn't reparent it to other sources with unknew
>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>>> open-loop mode but different sources.
>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
>> rate before switching to PLLP in order to have a proper CPU voltage.
>
> PLLP freq is safe to work for any CPU voltage. So no need to enforce
> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
> suspend
>
Sorry, please ignore my above comment. During suspend, need to change
CCLK_G source to PLLP when dfll is in closed loop mode first and then
dfll need to be set to open loop.
>>>> And I don't exactly understand why we need to switch to PLLP in CPU
>>>> idle
>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>
>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>> mode. That's
>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>> turn off
>>>> the CPU power.
>>>>
>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>> policy (CPU
>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>> The DFLL is re-inited after switching CCLK to DFLL parent during of the
>> early clocks-state restoring by CaR driver. Hence instead of having odd
>> hacks in the CaR driver, it is much nicer to have a proper
>> suspend-resume sequencing of the device drivers. In this case CPUFreq
>> driver is the driver that enables DFLL and switches CPU to that clock
>> source, which means that this driver is also should be responsible for
>> management of the DFLL's state during of suspend/resume process. If
>> CPUFreq driver disables DFLL during suspend and re-enables it during
>> resume, then looks like the CaR driver hacks around DFLL are not needed.
>>
>>>> The DFLL part looks good to me. BTW, change the patch subject to "Add
>>>> suspend-resume support" seems more appropriate to me.
>>>>
>>> To clarify this, the sequences for DFLL use are as follows (assuming
>>> all
>>> required DFLL hw configuration has been done)
>>>
>>> Switch to DFLL:
>>> 0) Save current parent and frequency
>>> 1) Program DFLL to open loop mode
>>> 2) Enable DFLL
>>> 3) Change cclk_g parent to DFLL
>>> For OVR regulator:
>>> 4) Change PWM output pin from tristate to output
>>> 5) Enable DFLL PWM output
>>> For I2C regulator:
>>> 4) Enable DFLL I2C output
>>> 6) Program DFLL to closed loop mode
>>>
>>> Switch away from DFLL:
>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>> vdd_cpu voltage
>>> 1) Program DFLL to open loop mode
>>>
I see during switch away from DFLL (suspend), cclk_g parent is not
changed to PLLP before changing dfll to open loop mode.
Will add this ...
>>> For OVR regulator:
>>> 2) Change PWM output pin from output to tristate: vdd_cpu will go back
>>> to hardwired boot voltage.
>>> 3) Disable DFLL PWM output
>>>
>>> For I2C regulator:
>>> 2) Program vdd_cpu regulator voltage to the boot voltage
>>> 3) Disable DFLL I2C output
>>>
>>> 4) Reprogram parent saved in step 0 of 'Switch to DFLL' to the saved
>>> frequency
>>> 5) Change cclk_g parent to saved parent
>>> 6) Disable DFLL
>
> This is the same sequence currently implemented. But dfll
> suspend/resume calls are thru Tegra210 clock driver.
>
> Dmitry wants to have dfll suspend/resume along with CCLK_G restore to
> happen from CPUFreq driver pm_ops rather than tegra210 clock driver or
> tegra dfll driver.
>
> Will move it to CPUFreq driver...
>
Thanks!
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 18:25 UTC (permalink / raw)
To: Sowjanya Komatineni, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <2b701832-5548-7c83-7c17-05cc2f1470c8@nvidia.com>
16.07.2019 21:19, Sowjanya Komatineni пишет:
>
> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>
>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>> OK, Will add to CPUFreq driver...
>>>>>>> The other thing that also need attention is that T124 CPUFreq driver
>>>>>>> implicitly relies on DFLL driver to be probed first, which is icky.
>>>>>>>
>>>>>> Should I add check for successful dfll clk register explicitly in
>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>> Probably you should use the "device links". See [1][2] for the example.
>>>
>>> [1]
>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>
>>>
>>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>
>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>
>>> [3]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>
>> Will go thru and add...
Looks like I initially confused this case with getting orphaned clock.
I'm now seeing that the DFLL driver registers the clock and then
clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
probed, hence everything should be fine as-is and there is no real need
for the 'device link'. Sorry for the confusion!
>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
>>>>>
>>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>>> integrated with DVFS control logic with the regulator. We will not
>>>>> switch
>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>> CPU has
>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
>>>>> you see
>>>>> in the driver.). We shouldn't reparent it to other sources with unknew
>>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>>>> open-loop mode but different sources.
>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
>>> rate before switching to PLLP in order to have a proper CPU voltage.
>>
>> PLLP freq is safe to work for any CPU voltage. So no need to enforce
>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>> suspend
>>
> Sorry, please ignore my above comment. During suspend, need to change
> CCLK_G source to PLLP when dfll is in closed loop mode first and then
> dfll need to be set to open loop.
Okay.
>>>>> And I don't exactly understand why we need to switch to PLLP in CPU
>>>>> idle
>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>
>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>> mode. That's
>>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>>> turn off
>>>>> the CPU power.
>>>>>
>>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>> policy (CPU
>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>> The DFLL is re-inited after switching CCLK to DFLL parent during of the
>>> early clocks-state restoring by CaR driver. Hence instead of having odd
>>> hacks in the CaR driver, it is much nicer to have a proper
>>> suspend-resume sequencing of the device drivers. In this case CPUFreq
>>> driver is the driver that enables DFLL and switches CPU to that clock
>>> source, which means that this driver is also should be responsible for
>>> management of the DFLL's state during of suspend/resume process. If
>>> CPUFreq driver disables DFLL during suspend and re-enables it during
>>> resume, then looks like the CaR driver hacks around DFLL are not needed.
>>>
>>>>> The DFLL part looks good to me. BTW, change the patch subject to "Add
>>>>> suspend-resume support" seems more appropriate to me.
>>>>>
>>>> To clarify this, the sequences for DFLL use are as follows (assuming
>>>> all
>>>> required DFLL hw configuration has been done)
>>>>
>>>> Switch to DFLL:
>>>> 0) Save current parent and frequency
>>>> 1) Program DFLL to open loop mode
>>>> 2) Enable DFLL
>>>> 3) Change cclk_g parent to DFLL
>>>> For OVR regulator:
>>>> 4) Change PWM output pin from tristate to output
>>>> 5) Enable DFLL PWM output
>>>> For I2C regulator:
>>>> 4) Enable DFLL I2C output
>>>> 6) Program DFLL to closed loop mode
>>>>
>>>> Switch away from DFLL:
>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>>> vdd_cpu voltage
>>>> 1) Program DFLL to open loop mode
>>>>
> I see during switch away from DFLL (suspend), cclk_g parent is not
> changed to PLLP before changing dfll to open loop mode.
>
> Will add this ...
The CPUFreq driver switches parent to PLLP during the probe, similar
should be done on suspend.
I'm also wondering if it's always safe to switch to PLLP in the probe.
If CPU is running on a lower freq than PLLP, then some other more
appropriate intermediate parent should be selected.
>>>> For OVR regulator:
>>>> 2) Change PWM output pin from output to tristate: vdd_cpu will go back
>>>> to hardwired boot voltage.
>>>> 3) Disable DFLL PWM output
>>>>
>>>> For I2C regulator:
>>>> 2) Program vdd_cpu regulator voltage to the boot voltage
>>>> 3) Disable DFLL I2C output
>>>>
>>>> 4) Reprogram parent saved in step 0 of 'Switch to DFLL' to the saved
>>>> frequency
>>>> 5) Change cclk_g parent to saved parent
>>>> 6) Disable DFLL
>>
>> This is the same sequence currently implemented. But dfll
>> suspend/resume calls are thru Tegra210 clock driver.
>>
>> Dmitry wants to have dfll suspend/resume along with CCLK_G restore to
>> happen from CPUFreq driver pm_ops rather than tegra210 clock driver or
>> tegra dfll driver.
>>
>> Will move it to CPUFreq driver...
>>
> Thanks!
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 18:30 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <76e341be-6f38-2bc1-048e-1aa6883f9b88@gmail.com>
On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>> The other thing that also need attention is that T124 CPUFreq driver
>>>>>>>> implicitly relies on DFLL driver to be probed first, which is icky.
>>>>>>>>
>>>>>>> Should I add check for successful dfll clk register explicitly in
>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>> Probably you should use the "device links". See [1][2] for the example.
>>>>
>>>> [1]
>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>
>>>>
>>>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>
>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>
>>>> [3]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>
>>> Will go thru and add...
> Looks like I initially confused this case with getting orphaned clock.
> I'm now seeing that the DFLL driver registers the clock and then
> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
> probed, hence everything should be fine as-is and there is no real need
> for the 'device link'. Sorry for the confusion!
>
>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
>>>>>>
>>>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>>>> integrated with DVFS control logic with the regulator. We will not
>>>>>> switch
>>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>>> CPU has
>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
>>>>>> you see
>>>>>> in the driver.). We shouldn't reparent it to other sources with unknew
>>>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>>>>> open-loop mode but different sources.
>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
>>>> rate before switching to PLLP in order to have a proper CPU voltage.
>>> PLLP freq is safe to work for any CPU voltage. So no need to enforce
>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>>> suspend
>>>
>> Sorry, please ignore my above comment. During suspend, need to change
>> CCLK_G source to PLLP when dfll is in closed loop mode first and then
>> dfll need to be set to open loop.
> Okay.
>
>>>>>> And I don't exactly understand why we need to switch to PLLP in CPU
>>>>>> idle
>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>
>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>> mode. That's
>>>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>>>> turn off
>>>>>> the CPU power.
>>>>>>
>>>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>> policy (CPU
>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>>> The DFLL is re-inited after switching CCLK to DFLL parent during of the
>>>> early clocks-state restoring by CaR driver. Hence instead of having odd
>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>> suspend-resume sequencing of the device drivers. In this case CPUFreq
>>>> driver is the driver that enables DFLL and switches CPU to that clock
>>>> source, which means that this driver is also should be responsible for
>>>> management of the DFLL's state during of suspend/resume process. If
>>>> CPUFreq driver disables DFLL during suspend and re-enables it during
>>>> resume, then looks like the CaR driver hacks around DFLL are not needed.
>>>>
>>>>>> The DFLL part looks good to me. BTW, change the patch subject to "Add
>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>
>>>>> To clarify this, the sequences for DFLL use are as follows (assuming
>>>>> all
>>>>> required DFLL hw configuration has been done)
>>>>>
>>>>> Switch to DFLL:
>>>>> 0) Save current parent and frequency
>>>>> 1) Program DFLL to open loop mode
>>>>> 2) Enable DFLL
>>>>> 3) Change cclk_g parent to DFLL
>>>>> For OVR regulator:
>>>>> 4) Change PWM output pin from tristate to output
>>>>> 5) Enable DFLL PWM output
>>>>> For I2C regulator:
>>>>> 4) Enable DFLL I2C output
>>>>> 6) Program DFLL to closed loop mode
>>>>>
>>>>> Switch away from DFLL:
>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>>>> vdd_cpu voltage
>>>>> 1) Program DFLL to open loop mode
>>>>>
>> I see during switch away from DFLL (suspend), cclk_g parent is not
>> changed to PLLP before changing dfll to open loop mode.
>>
>> Will add this ...
> The CPUFreq driver switches parent to PLLP during the probe, similar
> should be done on suspend.
>
> I'm also wondering if it's always safe to switch to PLLP in the probe.
> If CPU is running on a lower freq than PLLP, then some other more
> appropriate intermediate parent should be selected.
>
CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
so switching to PLL_P during CPUFreq probe prior to dfll clock enable
should be safe.
>>>>> For OVR regulator:
>>>>> 2) Change PWM output pin from output to tristate: vdd_cpu will go back
>>>>> to hardwired boot voltage.
>>>>> 3) Disable DFLL PWM output
>>>>>
>>>>> For I2C regulator:
>>>>> 2) Program vdd_cpu regulator voltage to the boot voltage
>>>>> 3) Disable DFLL I2C output
>>>>>
>>>>> 4) Reprogram parent saved in step 0 of 'Switch to DFLL' to the saved
>>>>> frequency
>>>>> 5) Change cclk_g parent to saved parent
>>>>> 6) Disable DFLL
>>> This is the same sequence currently implemented. But dfll
>>> suspend/resume calls are thru Tegra210 clock driver.
>>>
>>> Dmitry wants to have dfll suspend/resume along with CCLK_G restore to
>>> happen from CPUFreq driver pm_ops rather than tegra210 clock driver or
>>> tegra dfll driver.
>>>
>>> Will move it to CPUFreq driver...
>>>
>> Thanks!
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 18:43 UTC (permalink / raw)
To: Sowjanya Komatineni, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <0706576a-ce61-1cf3-bed1-05f54a1e2489@nvidia.com>
16.07.2019 21:30, Sowjanya Komatineni пишет:
>
> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>> The other thing that also need attention is that T124 CPUFreq
>>>>>>>>> driver
>>>>>>>>> implicitly relies on DFLL driver to be probed first, which is
>>>>>>>>> icky.
>>>>>>>>>
>>>>>>>> Should I add check for successful dfll clk register explicitly in
>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>> Probably you should use the "device links". See [1][2] for the
>>>>> example.
>>>>>
>>>>> [1]
>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>
>>>>>
>>>>>
>>>>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>
>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
>>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>>
>>>>> [3]
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>
>>>>>
>>>> Will go thru and add...
>> Looks like I initially confused this case with getting orphaned clock.
>> I'm now seeing that the DFLL driver registers the clock and then
>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
>> probed, hence everything should be fine as-is and there is no real need
>> for the 'device link'. Sorry for the confusion!
>>
>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL
>>>>>>> part.
>>>>>>>
>>>>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>>>>> integrated with DVFS control logic with the regulator. We will not
>>>>>>> switch
>>>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>>>> CPU has
>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
>>>>>>> you see
>>>>>>> in the driver.). We shouldn't reparent it to other sources with
>>>>>>> unknew
>>>>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>>>>>> open-loop mode but different sources.
>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
>>>>> rate before switching to PLLP in order to have a proper CPU voltage.
>>>> PLLP freq is safe to work for any CPU voltage. So no need to enforce
>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>>>> suspend
>>>>
>>> Sorry, please ignore my above comment. During suspend, need to change
>>> CCLK_G source to PLLP when dfll is in closed loop mode first and then
>>> dfll need to be set to open loop.
>> Okay.
>>
>>>>>>> And I don't exactly understand why we need to switch to PLLP in CPU
>>>>>>> idle
>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>
>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>>> mode. That's
>>>>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>>>>> turn off
>>>>>>> the CPU power.
>>>>>>>
>>>>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>>> policy (CPU
>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>>>> The DFLL is re-inited after switching CCLK to DFLL parent during of
>>>>> the
>>>>> early clocks-state restoring by CaR driver. Hence instead of having
>>>>> odd
>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>> suspend-resume sequencing of the device drivers. In this case CPUFreq
>>>>> driver is the driver that enables DFLL and switches CPU to that clock
>>>>> source, which means that this driver is also should be responsible for
>>>>> management of the DFLL's state during of suspend/resume process. If
>>>>> CPUFreq driver disables DFLL during suspend and re-enables it during
>>>>> resume, then looks like the CaR driver hacks around DFLL are not
>>>>> needed.
>>>>>
>>>>>>> The DFLL part looks good to me. BTW, change the patch subject to
>>>>>>> "Add
>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>
>>>>>> To clarify this, the sequences for DFLL use are as follows (assuming
>>>>>> all
>>>>>> required DFLL hw configuration has been done)
>>>>>>
>>>>>> Switch to DFLL:
>>>>>> 0) Save current parent and frequency
>>>>>> 1) Program DFLL to open loop mode
>>>>>> 2) Enable DFLL
>>>>>> 3) Change cclk_g parent to DFLL
>>>>>> For OVR regulator:
>>>>>> 4) Change PWM output pin from tristate to output
>>>>>> 5) Enable DFLL PWM output
>>>>>> For I2C regulator:
>>>>>> 4) Enable DFLL I2C output
>>>>>> 6) Program DFLL to closed loop mode
>>>>>>
>>>>>> Switch away from DFLL:
>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>>>>> vdd_cpu voltage
>>>>>> 1) Program DFLL to open loop mode
>>>>>>
>>> I see during switch away from DFLL (suspend), cclk_g parent is not
>>> changed to PLLP before changing dfll to open loop mode.
>>>
>>> Will add this ...
>> The CPUFreq driver switches parent to PLLP during the probe, similar
>> should be done on suspend.
>>
>> I'm also wondering if it's always safe to switch to PLLP in the probe.
>> If CPU is running on a lower freq than PLLP, then some other more
>> appropriate intermediate parent should be selected.
>>
> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
> should be safe.
AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
which CCLKG supports, the PLLP_OUT4.
Probably, realistically, CPU is always running off a fast PLLX during
boot, but I'm wondering what may happen on KEXEC. I guess ideally
CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
on a reboot, but likely that there are other clock-related problems as
well that may break KEXEC and thus it is not very important at the moment.
[snip]
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 19:26 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <5b2945c5-fcb2-2ac0-2bf2-df869dc9c713@gmail.com>
On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>> The other thing that also need attention is that T124 CPUFreq
>>>>>>>>>> driver
>>>>>>>>>> implicitly relies on DFLL driver to be probed first, which is
>>>>>>>>>> icky.
>>>>>>>>>>
>>>>>>>>> Should I add check for successful dfll clk register explicitly in
>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>>> Probably you should use the "device links". See [1][2] for the
>>>>>> example.
>>>>>>
>>>>>> [1]
>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>
>>>>>>
>>>>>>
>>>>>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>
>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
>>>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>>>
>>>>>> [3]
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>
>>>>>>
>>>>> Will go thru and add...
>>> Looks like I initially confused this case with getting orphaned clock.
>>> I'm now seeing that the DFLL driver registers the clock and then
>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
>>> probed, hence everything should be fine as-is and there is no real need
>>> for the 'device link'. Sorry for the confusion!
>>>
>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL
>>>>>>>> part.
>>>>>>>>
>>>>>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>>>>>> integrated with DVFS control logic with the regulator. We will not
>>>>>>>> switch
>>>>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>>>>> CPU has
>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
>>>>>>>> you see
>>>>>>>> in the driver.). We shouldn't reparent it to other sources with
>>>>>>>> unknew
>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
>>>>>>>> open-loop mode but different sources.
>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
>>>>>> rate before switching to PLLP in order to have a proper CPU voltage.
>>>>> PLLP freq is safe to work for any CPU voltage. So no need to enforce
>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>>>>> suspend
>>>>>
>>>> Sorry, please ignore my above comment. During suspend, need to change
>>>> CCLK_G source to PLLP when dfll is in closed loop mode first and then
>>>> dfll need to be set to open loop.
>>> Okay.
>>>
>>>>>>>> And I don't exactly understand why we need to switch to PLLP in CPU
>>>>>>>> idle
>>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>>
>>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>>>> mode. That's
>>>>>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>>>>>> turn off
>>>>>>>> the CPU power.
>>>>>>>>
>>>>>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
>>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>>>> policy (CPU
>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent during of
>>>>>> the
>>>>>> early clocks-state restoring by CaR driver. Hence instead of having
>>>>>> odd
>>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>>> suspend-resume sequencing of the device drivers. In this case CPUFreq
>>>>>> driver is the driver that enables DFLL and switches CPU to that clock
>>>>>> source, which means that this driver is also should be responsible for
>>>>>> management of the DFLL's state during of suspend/resume process. If
>>>>>> CPUFreq driver disables DFLL during suspend and re-enables it during
>>>>>> resume, then looks like the CaR driver hacks around DFLL are not
>>>>>> needed.
>>>>>>
>>>>>>>> The DFLL part looks good to me. BTW, change the patch subject to
>>>>>>>> "Add
>>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>>
>>>>>>> To clarify this, the sequences for DFLL use are as follows (assuming
>>>>>>> all
>>>>>>> required DFLL hw configuration has been done)
>>>>>>>
>>>>>>> Switch to DFLL:
>>>>>>> 0) Save current parent and frequency
>>>>>>> 1) Program DFLL to open loop mode
>>>>>>> 2) Enable DFLL
>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>> For OVR regulator:
>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>> 5) Enable DFLL PWM output
>>>>>>> For I2C regulator:
>>>>>>> 4) Enable DFLL I2C output
>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>
>>>>>>> Switch away from DFLL:
>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>>>>>> vdd_cpu voltage
>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>
>>>> I see during switch away from DFLL (suspend), cclk_g parent is not
>>>> changed to PLLP before changing dfll to open loop mode.
>>>>
>>>> Will add this ...
>>> The CPUFreq driver switches parent to PLLP during the probe, similar
>>> should be done on suspend.
>>>
>>> I'm also wondering if it's always safe to switch to PLLP in the probe.
>>> If CPU is running on a lower freq than PLLP, then some other more
>>> appropriate intermediate parent should be selected.
>>>
>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
>> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
>> should be safe.
> AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
> which CCLKG supports, the PLLP_OUT4.
>
> Probably, realistically, CPU is always running off a fast PLLX during
> boot, but I'm wondering what may happen on KEXEC. I guess ideally
> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
> on a reboot, but likely that there are other clock-related problems as
> well that may break KEXEC and thus it is not very important at the moment.
>
> [snip]
During bootup CPUG sources from PLL_X. By PLL_P source above I meant
PLL_P_OUT4.
As per clock policies, PLL_X is always used for high freq like >800Mhz
and for low frequency it will be sourced from PLLP.
^ permalink raw reply
* [PATCH] spi: gpio: Add SPI_MASTER_GPIO_SS flag
From: Linus Walleij @ 2019-07-16 20:46 UTC (permalink / raw)
To: Mark Brown, linux-spi; +Cc: linux-gpio, Linus Walleij, Andrey Smirnov
The GPIO SPI master has some code in its local CS
callback to set the initial sck GPIO value. This was
lost in the commit converting it to use SPI core
GPIO handling as this callback isn't called if the
internal GPIO handling is active.
Add the special SPI_MASTER_GPIO_SS to ascertain it
gets called anyway so we get the initial SCK setting
right. There is some platform provided GPIO handling
there as well but this will be skipped as the cs_gpios
will be NULL.
My test targets seem not to care about the initial
SCK value so I am uncertain if this is a regression,
but to preserve the previous semantic we better do
this.
Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
Fixes: 249e2632dcd0 ("spi: gpio: Don't request CS GPIO in DT use-case")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/spi/spi-gpio.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c
index eca9d52ecf65..9eb82150666e 100644
--- a/drivers/spi/spi-gpio.c
+++ b/drivers/spi/spi-gpio.c
@@ -410,6 +410,12 @@ static int spi_gpio_probe(struct platform_device *pdev)
bb = &spi_gpio->bitbang;
bb->master = master;
+ /*
+ * There is some additional business, apart from driving the CS GPIO
+ * line, that we need to do on selection. This makes the local
+ * callback for chipselect always get called.
+ */
+ master->flags |= SPI_MASTER_GPIO_SS;
bb->chipselect = spi_gpio_chipselect;
bb->set_line_direction = spi_gpio_set_direction;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 20:47 UTC (permalink / raw)
To: Sowjanya Komatineni, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <ef63f72a-db03-ef28-a371-e578f351c713@nvidia.com>
16.07.2019 22:26, Sowjanya Komatineni пишет:
>
> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>> The other thing that also need attention is that T124 CPUFreq
>>>>>>>>>>> driver
>>>>>>>>>>> implicitly relies on DFLL driver to be probed first, which is
>>>>>>>>>>> icky.
>>>>>>>>>>>
>>>>>>>>>> Should I add check for successful dfll clk register explicitly in
>>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>>>> Probably you should use the "device links". See [1][2] for the
>>>>>>> example.
>>>>>>>
>>>>>>> [1]
>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> [2]
>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>
>>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails.
>>>>>>> And
>>>>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>>>>
>>>>>>> [3]
>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> Will go thru and add...
>>>> Looks like I initially confused this case with getting orphaned clock.
>>>> I'm now seeing that the DFLL driver registers the clock and then
>>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
>>>> probed, hence everything should be fine as-is and there is no real need
>>>> for the 'device link'. Sorry for the confusion!
>>>>
>>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL
>>>>>>>>> part.
>>>>>>>>>
>>>>>>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>>>>>>> integrated with DVFS control logic with the regulator. We will not
>>>>>>>>> switch
>>>>>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>>>>>> CPU has
>>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP
>>>>>>>>> table
>>>>>>>>> you see
>>>>>>>>> in the driver.). We shouldn't reparent it to other sources with
>>>>>>>>> unknew
>>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow
>>>>>>>>> switching to
>>>>>>>>> open-loop mode but different sources.
>>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to
>>>>>>> PLLP's
>>>>>>> rate before switching to PLLP in order to have a proper CPU voltage.
>>>>>> PLLP freq is safe to work for any CPU voltage. So no need to enforce
>>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>>>>>> suspend
>>>>>>
>>>>> Sorry, please ignore my above comment. During suspend, need to change
>>>>> CCLK_G source to PLLP when dfll is in closed loop mode first and then
>>>>> dfll need to be set to open loop.
>>>> Okay.
>>>>
>>>>>>>>> And I don't exactly understand why we need to switch to PLLP in
>>>>>>>>> CPU
>>>>>>>>> idle
>>>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>>>
>>>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>>>>> mode. That's
>>>>>>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>>>>>>> turn off
>>>>>>>>> the CPU power.
>>>>>>>>>
>>>>>>>>> In SC7 resume, the warmboot code will handle the sequence to
>>>>>>>>> turn on
>>>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P.
>>>>>>>>> After
>>>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>>>>> policy (CPU
>>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent during of
>>>>>>> the
>>>>>>> early clocks-state restoring by CaR driver. Hence instead of having
>>>>>>> odd
>>>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>>>> suspend-resume sequencing of the device drivers. In this case
>>>>>>> CPUFreq
>>>>>>> driver is the driver that enables DFLL and switches CPU to that
>>>>>>> clock
>>>>>>> source, which means that this driver is also should be
>>>>>>> responsible for
>>>>>>> management of the DFLL's state during of suspend/resume process. If
>>>>>>> CPUFreq driver disables DFLL during suspend and re-enables it during
>>>>>>> resume, then looks like the CaR driver hacks around DFLL are not
>>>>>>> needed.
>>>>>>>
>>>>>>>>> The DFLL part looks good to me. BTW, change the patch subject to
>>>>>>>>> "Add
>>>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>>>
>>>>>>>> To clarify this, the sequences for DFLL use are as follows
>>>>>>>> (assuming
>>>>>>>> all
>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>
>>>>>>>> Switch to DFLL:
>>>>>>>> 0) Save current parent and frequency
>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>> 2) Enable DFLL
>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>> For OVR regulator:
>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>> For I2C regulator:
>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>
>>>>>>>> Switch away from DFLL:
>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>>>>>>> vdd_cpu voltage
>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>
>>>>> I see during switch away from DFLL (suspend), cclk_g parent is not
>>>>> changed to PLLP before changing dfll to open loop mode.
>>>>>
>>>>> Will add this ...
>>>> The CPUFreq driver switches parent to PLLP during the probe, similar
>>>> should be done on suspend.
>>>>
>>>> I'm also wondering if it's always safe to switch to PLLP in the probe.
>>>> If CPU is running on a lower freq than PLLP, then some other more
>>>> appropriate intermediate parent should be selected.
>>>>
>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
>>> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
>>> should be safe.
>> AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
>> which CCLKG supports, the PLLP_OUT4.
>>
>> Probably, realistically, CPU is always running off a fast PLLX during
>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
>> on a reboot, but likely that there are other clock-related problems as
>> well that may break KEXEC and thus it is not very important at the
>> moment.
>>
>> [snip]
>
> During bootup CPUG sources from PLL_X. By PLL_P source above I meant
> PLL_P_OUT4.
>
> As per clock policies, PLL_X is always used for high freq like >800Mhz
> and for low frequency it will be sourced from PLLP.
Alright, then please don't forget to pre-initialize PLLP_OUT4 rate to a
reasonable value using tegra_clk_init_table or assigned-clocks.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 21:12 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <27641e30-fdd1-e53a-206d-71e1f23343fd@gmail.com>
On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>> The other thing that also need attention is that T124 CPUFreq
>>>>>>>>>>>> driver
>>>>>>>>>>>> implicitly relies on DFLL driver to be probed first, which is
>>>>>>>>>>>> icky.
>>>>>>>>>>>>
>>>>>>>>>>> Should I add check for successful dfll clk register explicitly in
>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>>>>> Probably you should use the "device links". See [1][2] for the
>>>>>>>> example.
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> [2]
>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>
>>>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails.
>>>>>>>> And
>>>>>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>>>>>
>>>>>>>> [3]
>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> Will go thru and add...
>>>>> Looks like I initially confused this case with getting orphaned clock.
>>>>> I'm now seeing that the DFLL driver registers the clock and then
>>>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
>>>>> probed, hence everything should be fine as-is and there is no real need
>>>>> for the 'device link'. Sorry for the confusion!
>>>>>
>>>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL
>>>>>>>>>> part.
>>>>>>>>>>
>>>>>>>>>> As you know it, the DFLL clock is one of the CPU clock sources and
>>>>>>>>>> integrated with DVFS control logic with the regulator. We will not
>>>>>>>>>> switch
>>>>>>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>>>>>>> CPU has
>>>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP
>>>>>>>>>> table
>>>>>>>>>> you see
>>>>>>>>>> in the driver.). We shouldn't reparent it to other sources with
>>>>>>>>>> unknew
>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow
>>>>>>>>>> switching to
>>>>>>>>>> open-loop mode but different sources.
>>>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to
>>>>>>>> PLLP's
>>>>>>>> rate before switching to PLLP in order to have a proper CPU voltage.
>>>>>>> PLLP freq is safe to work for any CPU voltage. So no need to enforce
>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>>>>>>> suspend
>>>>>>>
>>>>>> Sorry, please ignore my above comment. During suspend, need to change
>>>>>> CCLK_G source to PLLP when dfll is in closed loop mode first and then
>>>>>> dfll need to be set to open loop.
>>>>> Okay.
>>>>>
>>>>>>>>>> And I don't exactly understand why we need to switch to PLLP in
>>>>>>>>>> CPU
>>>>>>>>>> idle
>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>>>>
>>>>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>>>>>> mode. That's
>>>>>>>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
>>>>>>>>>> turn off
>>>>>>>>>> the CPU power.
>>>>>>>>>>
>>>>>>>>>> In SC7 resume, the warmboot code will handle the sequence to
>>>>>>>>>> turn on
>>>>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P.
>>>>>>>>>> After
>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>>>>>> policy (CPU
>>>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent during of
>>>>>>>> the
>>>>>>>> early clocks-state restoring by CaR driver. Hence instead of having
>>>>>>>> odd
>>>>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>>>>> suspend-resume sequencing of the device drivers. In this case
>>>>>>>> CPUFreq
>>>>>>>> driver is the driver that enables DFLL and switches CPU to that
>>>>>>>> clock
>>>>>>>> source, which means that this driver is also should be
>>>>>>>> responsible for
>>>>>>>> management of the DFLL's state during of suspend/resume process. If
>>>>>>>> CPUFreq driver disables DFLL during suspend and re-enables it during
>>>>>>>> resume, then looks like the CaR driver hacks around DFLL are not
>>>>>>>> needed.
>>>>>>>>
>>>>>>>>>> The DFLL part looks good to me. BTW, change the patch subject to
>>>>>>>>>> "Add
>>>>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>>>>
>>>>>>>>> To clarify this, the sequences for DFLL use are as follows
>>>>>>>>> (assuming
>>>>>>>>> all
>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>
>>>>>>>>> Switch to DFLL:
>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>> 2) Enable DFLL
>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>> For OVR regulator:
>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>> For I2C regulator:
>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>
>>>>>>>>> Switch away from DFLL:
>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any
>>>>>>>>> vdd_cpu voltage
>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>
>>>>>> I see during switch away from DFLL (suspend), cclk_g parent is not
>>>>>> changed to PLLP before changing dfll to open loop mode.
>>>>>>
>>>>>> Will add this ...
>>>>> The CPUFreq driver switches parent to PLLP during the probe, similar
>>>>> should be done on suspend.
>>>>>
>>>>> I'm also wondering if it's always safe to switch to PLLP in the probe.
>>>>> If CPU is running on a lower freq than PLLP, then some other more
>>>>> appropriate intermediate parent should be selected.
>>>>>
>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
>>>> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
>>>> should be safe.
>>> AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
>>> which CCLKG supports, the PLLP_OUT4.
>>>
>>> Probably, realistically, CPU is always running off a fast PLLX during
>>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>>> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
>>> on a reboot, but likely that there are other clock-related problems as
>>> well that may break KEXEC and thus it is not very important at the
>>> moment.
>>>
>>> [snip]
>> During bootup CPUG sources from PLL_X. By PLL_P source above I meant
>> PLL_P_OUT4.
>>
>> As per clock policies, PLL_X is always used for high freq like >800Mhz
>> and for low frequency it will be sourced from PLLP.
> Alright, then please don't forget to pre-initialize PLLP_OUT4 rate to a
> reasonable value using tegra_clk_init_table or assigned-clocks.
PLLP_OUT4 rate update is not needed as it is safe to run at 408Mhz
because it is below fmax @ Vmin
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 21:21 UTC (permalink / raw)
To: Sowjanya Komatineni, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <10c4b9a2-a857-d124-c22d-7fd71a473079@nvidia.com>
17.07.2019 0:12, Sowjanya Komatineni пишет:
>
> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>> The other thing that also need attention is that T124 CPUFreq
>>>>>>>>>>>>> driver
>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed first, which is
>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>
>>>>>>>>>>>> Should I add check for successful dfll clk register
>>>>>>>>>>>> explicitly in
>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>>>>>> Probably you should use the "device links". See [1][2] for the
>>>>>>>>> example.
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> [2]
>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>
>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails.
>>>>>>>>> And
>>>>>>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>>>>>>
>>>>>>>>> [3]
>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>> Will go thru and add...
>>>>>> Looks like I initially confused this case with getting orphaned
>>>>>> clock.
>>>>>> I'm now seeing that the DFLL driver registers the clock and then
>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
>>>>>> probed, hence everything should be fine as-is and there is no real
>>>>>> need
>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>
>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL
>>>>>>>>>>> part.
>>>>>>>>>>>
>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU clock
>>>>>>>>>>> sources and
>>>>>>>>>>> integrated with DVFS control logic with the regulator. We
>>>>>>>>>>> will not
>>>>>>>>>>> switch
>>>>>>>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>>>>>>>> CPU has
>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP
>>>>>>>>>>> table
>>>>>>>>>>> you see
>>>>>>>>>>> in the driver.). We shouldn't reparent it to other sources with
>>>>>>>>>>> unknew
>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow
>>>>>>>>>>> switching to
>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to
>>>>>>>>> PLLP's
>>>>>>>>> rate before switching to PLLP in order to have a proper CPU
>>>>>>>>> voltage.
>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no need to
>>>>>>>> enforce
>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>>>>>>>> suspend
>>>>>>>>
>>>>>>> Sorry, please ignore my above comment. During suspend, need to
>>>>>>> change
>>>>>>> CCLK_G source to PLLP when dfll is in closed loop mode first and
>>>>>>> then
>>>>>>> dfll need to be set to open loop.
>>>>>> Okay.
>>>>>>
>>>>>>>>>>> And I don't exactly understand why we need to switch to PLLP in
>>>>>>>>>>> CPU
>>>>>>>>>>> idle
>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>>>>>
>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>>>>>>> mode. That's
>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest of the
>>>>>>>>>>> sequence to
>>>>>>>>>>> turn off
>>>>>>>>>>> the CPU power.
>>>>>>>>>>>
>>>>>>>>>>> In SC7 resume, the warmboot code will handle the sequence to
>>>>>>>>>>> turn on
>>>>>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P.
>>>>>>>>>>> After
>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>>>>>>> policy (CPU
>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent
>>>>>>>>> during of
>>>>>>>>> the
>>>>>>>>> early clocks-state restoring by CaR driver. Hence instead of
>>>>>>>>> having
>>>>>>>>> odd
>>>>>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>>>>>> suspend-resume sequencing of the device drivers. In this case
>>>>>>>>> CPUFreq
>>>>>>>>> driver is the driver that enables DFLL and switches CPU to that
>>>>>>>>> clock
>>>>>>>>> source, which means that this driver is also should be
>>>>>>>>> responsible for
>>>>>>>>> management of the DFLL's state during of suspend/resume
>>>>>>>>> process. If
>>>>>>>>> CPUFreq driver disables DFLL during suspend and re-enables it
>>>>>>>>> during
>>>>>>>>> resume, then looks like the CaR driver hacks around DFLL are not
>>>>>>>>> needed.
>>>>>>>>>
>>>>>>>>>>> The DFLL part looks good to me. BTW, change the patch subject to
>>>>>>>>>>> "Add
>>>>>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>>>>>
>>>>>>>>>> To clarify this, the sequences for DFLL use are as follows
>>>>>>>>>> (assuming
>>>>>>>>>> all
>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>
>>>>>>>>>> Switch to DFLL:
>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>> For OVR regulator:
>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>> For I2C regulator:
>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>
>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for
>>>>>>>>>> any
>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>
>>>>>>> I see during switch away from DFLL (suspend), cclk_g parent is not
>>>>>>> changed to PLLP before changing dfll to open loop mode.
>>>>>>>
>>>>>>> Will add this ...
>>>>>> The CPUFreq driver switches parent to PLLP during the probe, similar
>>>>>> should be done on suspend.
>>>>>>
>>>>>> I'm also wondering if it's always safe to switch to PLLP in the
>>>>>> probe.
>>>>>> If CPU is running on a lower freq than PLLP, then some other more
>>>>>> appropriate intermediate parent should be selected.
>>>>>>
>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher
>>>>> rate
>>>>> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
>>>>> should be safe.
>>>> AFAIK, PLLX could run at ~200MHz. There is also a divided output of
>>>> PLLP
>>>> which CCLKG supports, the PLLP_OUT4.
>>>>
>>>> Probably, realistically, CPU is always running off a fast PLLX during
>>>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>>>> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
>>>> on a reboot, but likely that there are other clock-related problems as
>>>> well that may break KEXEC and thus it is not very important at the
>>>> moment.
>>>>
>>>> [snip]
>>> During bootup CPUG sources from PLL_X. By PLL_P source above I meant
>>> PLL_P_OUT4.
>>>
>>> As per clock policies, PLL_X is always used for high freq like >800Mhz
>>> and for low frequency it will be sourced from PLLP.
>> Alright, then please don't forget to pre-initialize PLLP_OUT4 rate to a
>> reasonable value using tegra_clk_init_table or assigned-clocks.
>
> PLLP_OUT4 rate update is not needed as it is safe to run at 408Mhz
> because it is below fmax @ Vmin
So even 204MHz CVB entries are having the same voltage as 408MHz,
correct? It's not instantly obvious to me from the DFLL driver's code
where the fmax @ Vmin is defined, I see that there is the min_millivolts
and frequency entries starting from 204MHZ defined per-table.
^ permalink raw reply
* [PATCH] spi: gpio: Use core CS GPIO handling for everything
From: Linus Walleij @ 2019-07-16 21:24 UTC (permalink / raw)
To: Mark Brown, linux-spi; +Cc: linux-gpio, Linus Walleij, Andrey Smirnov
The SPI core can handle all CS GPIOs without any problem.
As can be seen from the code in spi_get_gpio_descs()
the core will just get a bunch of chip selects from
the device.
When using GPIO descriptors from a board file, the
process is not any different.
However we need to be carefule about one thing:
devm_gpiod_get_index_optional() passes GPIOD_OUT_LOW
in the core and GPIOD_OUT_HIGH in the spi-gpio driver.
This is because the driver assumes the polarity
inversion is not handled by the core.
We need to revisit commit 9b00bc7b901f
("spi: spi-gpio: Rewrite to use GPIO descriptors")
and make sure all passed descriptors are flagged as
active low for the core, because now they are passed
in as active high and inverted by the driver instead.
The device tree core will enforce active low on
CS gpios, but board files are not that forgiving.
Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
arch/arm/mach-pxa/cm-x300.c | 2 +-
arch/arm/mach-s3c24xx/mach-jive.c | 4 +-
arch/arm/mach-s3c24xx/mach-qt2410.c | 2 +-
arch/arm/mach-s3c64xx/mach-smartq.c | 2 +-
arch/mips/alchemy/devboards/db1000.c | 2 +-
arch/mips/jz4740/board-qi_lb60.c | 2 +-
drivers/misc/eeprom/digsy_mtc_eeprom.c | 2 +-
drivers/spi/spi-gpio.c | 59 ++------------------------
8 files changed, 12 insertions(+), 63 deletions(-)
diff --git a/arch/arm/mach-pxa/cm-x300.c b/arch/arm/mach-pxa/cm-x300.c
index 425855f456f2..60ebc5f4041e 100644
--- a/arch/arm/mach-pxa/cm-x300.c
+++ b/arch/arm/mach-pxa/cm-x300.c
@@ -362,7 +362,7 @@ static struct gpiod_lookup_table cm_x300_spi_gpiod_table = {
GPIO_LOOKUP("gpio-pxa", GPIO_LCD_DOUT,
"miso", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("gpio-pxa", GPIO_LCD_CS,
- "cs", GPIO_ACTIVE_HIGH),
+ "cs", GPIO_ACTIVE_LOW),
{ },
},
};
diff --git a/arch/arm/mach-s3c24xx/mach-jive.c b/arch/arm/mach-s3c24xx/mach-jive.c
index 885e8f12e4b9..66c38472111d 100644
--- a/arch/arm/mach-s3c24xx/mach-jive.c
+++ b/arch/arm/mach-s3c24xx/mach-jive.c
@@ -406,7 +406,7 @@ static struct gpiod_lookup_table jive_lcdspi_gpiod_table = {
GPIO_LOOKUP("GPIOB", 8,
"mosi", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("GPIOB", 7,
- "cs", GPIO_ACTIVE_HIGH),
+ "cs", GPIO_ACTIVE_LOW),
{ },
},
};
@@ -431,7 +431,7 @@ static struct gpiod_lookup_table jive_wm8750_gpiod_table = {
GPIO_LOOKUP("GPIOB", 9,
"mosi", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("GPIOH", 10,
- "cs", GPIO_ACTIVE_HIGH),
+ "cs", GPIO_ACTIVE_LOW),
{ },
},
};
diff --git a/arch/arm/mach-s3c24xx/mach-qt2410.c b/arch/arm/mach-s3c24xx/mach-qt2410.c
index 5d48e5b6e738..61fbd3694257 100644
--- a/arch/arm/mach-s3c24xx/mach-qt2410.c
+++ b/arch/arm/mach-s3c24xx/mach-qt2410.c
@@ -214,7 +214,7 @@ static struct gpiod_lookup_table qt2410_spi_gpiod_table = {
GPIO_LOOKUP("GPIOG", 5,
"miso", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("GPIOB", 5,
- "cs", GPIO_ACTIVE_HIGH),
+ "cs", GPIO_ACTIVE_LOW),
{ },
},
};
diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c b/arch/arm/mach-s3c64xx/mach-smartq.c
index 951208f168e7..f81a85c75247 100644
--- a/arch/arm/mach-s3c64xx/mach-smartq.c
+++ b/arch/arm/mach-s3c64xx/mach-smartq.c
@@ -225,7 +225,7 @@ static struct gpiod_lookup_table smartq_lcd_control_gpiod_table = {
GPIO_LOOKUP("GPIOM", 3,
"miso", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("GPIOM", 0,
- "cs", GPIO_ACTIVE_HIGH),
+ "cs", GPIO_ACTIVE_LOW),
{ },
},
};
diff --git a/arch/mips/alchemy/devboards/db1000.c b/arch/mips/alchemy/devboards/db1000.c
index 2c52ee27b4f2..aa3b5b3d81aa 100644
--- a/arch/mips/alchemy/devboards/db1000.c
+++ b/arch/mips/alchemy/devboards/db1000.c
@@ -424,7 +424,7 @@ static struct gpiod_lookup_table db1100_spi_gpiod_table = {
GPIO_LOOKUP("alchemy-gpio2", 7,
"miso", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("alchemy-gpio2", 10,
- "cs", GPIO_ACTIVE_HIGH),
+ "cs", GPIO_ACTIVE_LOW),
{ },
},
};
diff --git a/arch/mips/jz4740/board-qi_lb60.c b/arch/mips/jz4740/board-qi_lb60.c
index 071e9d94eea7..9ea53398c0f1 100644
--- a/arch/mips/jz4740/board-qi_lb60.c
+++ b/arch/mips/jz4740/board-qi_lb60.c
@@ -328,7 +328,7 @@ static struct gpiod_lookup_table qi_lb60_spigpio_gpio_table = {
GPIO_LOOKUP("GPIOC", 22,
"mosi", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("GPIOC", 21,
- "cs", GPIO_ACTIVE_HIGH),
+ "cs", GPIO_ACTIVE_LOW),
{ },
},
};
diff --git a/drivers/misc/eeprom/digsy_mtc_eeprom.c b/drivers/misc/eeprom/digsy_mtc_eeprom.c
index f1f766b70965..c058eaff0385 100644
--- a/drivers/misc/eeprom/digsy_mtc_eeprom.c
+++ b/drivers/misc/eeprom/digsy_mtc_eeprom.c
@@ -69,7 +69,7 @@ static struct gpiod_lookup_table eeprom_spi_gpiod_table = {
GPIO_LOOKUP("gpio@b00", GPIO_EEPROM_DO,
"miso", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("gpio@b00", GPIO_EEPROM_CS,
- "cs", GPIO_ACTIVE_HIGH),
+ "cs", GPIO_ACTIVE_LOW),
{ },
},
};
diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c
index 9eb82150666e..4309832221b1 100644
--- a/drivers/spi/spi-gpio.c
+++ b/drivers/spi/spi-gpio.c
@@ -35,7 +35,6 @@ struct spi_gpio {
struct gpio_desc *sck;
struct gpio_desc *miso;
struct gpio_desc *mosi;
- struct gpio_desc **cs_gpios;
};
/*----------------------------------------------------------------------*/
@@ -203,37 +202,6 @@ static void spi_gpio_chipselect(struct spi_device *spi, int is_active)
/* set initial clock line level */
if (is_active)
gpiod_set_value_cansleep(spi_gpio->sck, spi->mode & SPI_CPOL);
-
- /* Drive chip select line, if we have one */
- if (spi_gpio->cs_gpios) {
- struct gpio_desc *cs = spi_gpio->cs_gpios[spi->chip_select];
-
- /* SPI chip selects are normally active-low */
- gpiod_set_value_cansleep(cs, (spi->mode & SPI_CS_HIGH) ? is_active : !is_active);
- }
-}
-
-static int spi_gpio_setup(struct spi_device *spi)
-{
- struct gpio_desc *cs;
- int status = 0;
- struct spi_gpio *spi_gpio = spi_to_spi_gpio(spi);
-
- /*
- * The CS GPIOs have already been
- * initialized from the descriptor lookup.
- */
- if (spi_gpio->cs_gpios) {
- cs = spi_gpio->cs_gpios[spi->chip_select];
- if (!spi->controller_state && cs)
- status = gpiod_direction_output(cs,
- !(spi->mode & SPI_CS_HIGH));
- }
-
- if (!status)
- status = spi_bitbang_setup(spi);
-
- return status;
}
static int spi_gpio_set_direction(struct spi_device *spi, bool output)
@@ -264,11 +232,6 @@ static int spi_gpio_set_direction(struct spi_device *spi, bool output)
return 0;
}
-static void spi_gpio_cleanup(struct spi_device *spi)
-{
- spi_bitbang_cleanup(spi);
-}
-
/*
* It can be convenient to use this driver with pins that have alternate
* functions associated with a "native" SPI controller if a driver for that
@@ -324,8 +287,6 @@ static int spi_gpio_probe_pdata(struct platform_device *pdev,
{
struct device *dev = &pdev->dev;
struct spi_gpio_platform_data *pdata = dev_get_platdata(dev);
- struct spi_gpio *spi_gpio = spi_master_get_devdata(master);
- int i;
#ifdef GENERIC_BITBANG
if (!pdata || !pdata->num_chipselect)
@@ -335,20 +296,8 @@ static int spi_gpio_probe_pdata(struct platform_device *pdev,
* The master needs to think there is a chipselect even if not
* connected
*/
- master->num_chipselect = pdata->num_chipselect ?: 1;
-
- spi_gpio->cs_gpios = devm_kcalloc(dev, master->num_chipselect,
- sizeof(*spi_gpio->cs_gpios),
- GFP_KERNEL);
- if (!spi_gpio->cs_gpios)
- return -ENOMEM;
-
- for (i = 0; i < master->num_chipselect; i++) {
- spi_gpio->cs_gpios[i] = devm_gpiod_get_index(dev, "cs", i,
- GPIOD_OUT_HIGH);
- if (IS_ERR(spi_gpio->cs_gpios[i]))
- return PTR_ERR(spi_gpio->cs_gpios[i]);
- }
+ master->num_chipselect = pdata->num_chipselect ? : 1;
+ master->use_gpio_descriptors = true;
return 0;
}
@@ -405,8 +354,8 @@ static int spi_gpio_probe(struct platform_device *pdev)
}
master->bus_num = pdev->id;
- master->setup = spi_gpio_setup;
- master->cleanup = spi_gpio_cleanup;
+ master->setup = spi_bitbang_setup;
+ master->cleanup = spi_bitbang_cleanup;
bb = &spi_gpio->bitbang;
bb->master = master;
--
2.21.0
^ permalink raw reply related
* [PATCH 2/3 v3] ARM: dts: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-16 21:24 UTC (permalink / raw)
To: Bartosz Golaszewski, Joel Stanley, Andrew Jeffery, Linus Walleij
Cc: Hongwei Zhang, linux-gpio, linux-arm-kernel, linux-aspeed,
linux-kernel
Add SGPIO driver support for Aspeed AST2500 SoC.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
drivers/gpio/sgpio-aspeed.c | 487 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 487 insertions(+)
create mode 100644 drivers/gpio/sgpio-aspeed.c
diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
new file mode 100644
index 0000000..ade2cb7
--- /dev/null
+++ b/drivers/gpio/sgpio-aspeed.c
@@ -0,0 +1,487 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2019 American Megatrends International LLC.
+ *
+ * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/gpio/aspeed.h>
+#include <linux/hashtable.h>
+#include <linux/bitfield.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/gpio.h>
+
+#define MAX_NR_SGPIO 80
+
+#define ASPEED_SGPIO_CTRL 0x54
+
+#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
+#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
+#define ASPEED_SGPIO_ENABLE BIT(0)
+
+// default sgpio direction is input.
+static uint32_t sgpio_dir_val[3] = {0xffffffff, 0xffffffff, 0xffffffff };
+
+struct aspeed_sgpio {
+ struct gpio_chip chip;
+ struct clk *pclk;
+ spinlock_t lock;
+ void __iomem *base;
+ int irq;
+};
+
+struct aspeed_sgpio_bank {
+ uint16_t val_regs;
+ uint16_t rdata_reg;
+ uint16_t irq_regs;
+ const char names[4][3];
+};
+
+/*
+ * Note: The "value" register returns the input value sampled on the
+ * line even when the GPIO is configured as an output. Since
+ * that input goes through synchronizers, writing, then reading
+ * back may not return the written value right away.
+ *
+ * The "rdata" register returns the content of the write latch
+ * and thus can be used to read back what was last written
+ * reliably.
+ */
+static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
+ {
+ .val_regs = 0x0000,
+ .rdata_reg = 0x0070,
+ .irq_regs = 0x0004,
+ .names = { "A", "B", "C", "D" },
+ },
+ {
+ .val_regs = 0x001C,
+ .rdata_reg = 0x0074,
+ .irq_regs = 0x0020,
+ .names = { "E", "F", "G", "H" },
+ },
+ {
+ .val_regs = 0x0038,
+ .rdata_reg = 0x0078,
+ .irq_regs = 0x003C,
+ .names = { "I", "J" },
+ },
+};
+
+enum aspeed_sgpio_reg {
+ reg_val,
+ reg_rdata,
+ reg_irq_enable,
+ reg_irq_type0,
+ reg_irq_type1,
+ reg_irq_type2,
+ reg_irq_status,
+};
+
+#define GPIO_VAL_VALUE 0x00
+#define GPIO_VAL_DIR 0x04
+#define GPIO_IRQ_ENABLE 0x00
+#define GPIO_IRQ_TYPE0 0x04
+#define GPIO_IRQ_TYPE1 0x08
+#define GPIO_IRQ_TYPE2 0x0C
+#define GPIO_IRQ_STATUS 0x10
+
+/* This will be resolved at compile time */
+static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
+ const struct aspeed_sgpio_bank *bank,
+ const enum aspeed_sgpio_reg reg)
+{
+ switch (reg) {
+ case reg_val:
+ return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
+ case reg_rdata:
+ return gpio->base + bank->rdata_reg;
+ case reg_irq_enable:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
+ case reg_irq_type0:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
+ case reg_irq_type1:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
+ case reg_irq_type2:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
+ case reg_irq_status:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
+ default:
+ /* acturally if code runs to here, it's an error case */
+ BUG_ON(1);
+ }
+}
+
+#define GPIO_BANK(x) ((x) >> 5)
+#define GPIO_OFFSET(x) ((x) & 0x1f)
+#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
+
+static const struct aspeed_sgpio_bank *to_bank(unsigned int offset)
+{
+ unsigned int bank = GPIO_BANK(offset);
+
+ WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
+ return &aspeed_sgpio_banks[bank];
+}
+
+static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ const struct aspeed_sgpio_bank *bank = to_bank(offset);
+
+ if (sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset))
+ return !!(ioread32(bank_reg(gpio, bank, reg_val)) & GPIO_BIT(offset));
+ else
+ return !!(ioread32(bank_reg(gpio, bank, reg_rdata)) & GPIO_BIT(offset));
+
+}
+
+static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ const struct aspeed_sgpio_bank *bank = to_bank(offset);
+ unsigned long flags;
+ void __iomem *addr;
+ u32 reg = 0;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_val);
+
+ if (val)
+ reg |= GPIO_BIT(offset);
+ else
+ reg &= ~GPIO_BIT(offset);
+
+ iowrite32(reg, addr);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset)
+{
+ sgpio_dir_val[GPIO_BANK(offset)] |= GPIO_BIT(offset);
+ return 0;
+}
+
+static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
+{
+ sgpio_dir_val[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
+ return 0;
+}
+
+static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ return sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset);
+
+}
+
+static inline int irqd_to_aspeed_sgpio_data(struct irq_data *d,
+ struct aspeed_sgpio **gpio,
+ const struct aspeed_sgpio_bank **bank,
+ u32 *bit, int *offset)
+{
+ struct aspeed_sgpio *internal;
+
+ *offset = irqd_to_hwirq(d);
+
+ internal = irq_data_get_irq_chip_data(d);
+
+ *gpio = internal;
+ *bank = to_bank(*offset);
+ *bit = GPIO_BIT(*offset);
+
+ return 0;
+}
+
+static void aspeed_sgpio_irq_ack(struct irq_data *d)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *status_addr;
+ int rc, offset;
+ u32 bit;
+
+ rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+ if (rc)
+ return;
+
+ status_addr = bank_reg(gpio, bank, reg_irq_status);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ iowrite32(bit, status_addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ u32 reg, bit;
+ void __iomem *addr;
+ int rc, offset;
+
+ rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+ if (rc)
+ return;
+
+ addr = bank_reg(gpio, bank, reg_irq_enable);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ reg = ioread32(addr);
+ if (set)
+ reg |= bit;
+ else
+ reg &= ~bit;
+
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_mask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, false);
+}
+
+static void aspeed_sgpio_irq_unmask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, true);
+}
+
+static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
+{
+ u32 type0 = 0;
+ u32 type1 = 0;
+ u32 type2 = 0;
+ u32 bit, reg;
+ const struct aspeed_sgpio_bank *bank;
+ irq_flow_handler_t handler;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *addr;
+ int rc, offset;
+
+ rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+ if (rc)
+ return -EINVAL;
+
+ switch (type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_EDGE_BOTH:
+ type2 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_RISING:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_FALLING:
+ handler = handle_edge_irq;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_LEVEL_LOW:
+ type1 |= bit;
+ handler = handle_level_irq;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_irq_type0);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type0;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type1);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type1;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type2);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type2;
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ irq_set_handler_locked(d, handler);
+
+ return 0;
+}
+
+static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
+{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *ic = irq_desc_get_chip(desc);
+ struct aspeed_sgpio *data = gpiochip_get_data(gc);
+ unsigned int i, p, girq;
+ unsigned long reg;
+
+ chained_irq_enter(ic, desc);
+
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
+
+ reg = ioread32(bank_reg(data, bank, reg_irq_status));
+
+ for_each_set_bit(p, ®, 32) {
+ girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
+ generic_handle_irq(girq);
+ }
+
+ }
+
+ chained_irq_exit(ic, desc);
+}
+
+static struct irq_chip aspeed_sgpio_irqchip = {
+ .name = "aspeed-sgpio",
+ .irq_ack = aspeed_sgpio_irq_ack,
+ .irq_mask = aspeed_sgpio_irq_mask,
+ .irq_unmask = aspeed_sgpio_irq_unmask,
+ .irq_set_type = aspeed_sgpio_set_type,
+};
+
+static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
+ struct platform_device *pdev)
+{
+ int rc, i;
+ const struct aspeed_sgpio_bank *bank;
+
+ rc = platform_get_irq(pdev, 0);
+ if (rc < 0)
+ return rc;
+
+ gpio->irq = rc;
+
+ /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* disable irq enable bits */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
+ /* clear status bits */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
+ }
+
+ rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
+ 0, handle_bad_irq, IRQ_TYPE_NONE);
+ if (rc) {
+ dev_info(&pdev->dev, "Could not add irqchip\n");
+ return rc;
+ }
+
+ gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
+ gpio->irq, aspeed_sgpio_irq_handler);
+
+ /* set IRQ settings and Enable Interrupt */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* set falling or level-low irq */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
+ /* trigger type is edge */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
+ /* dual edge trigger mode. */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
+ /* enable irq */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
+ }
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_sgpio_of_table[] = {
+ { .compatible = "aspeed,ast2400-sgpio" },
+ { .compatible = "aspeed,ast2500-sgpio" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
+
+static int __init aspeed_sgpio_probe(struct platform_device *pdev)
+{
+ struct aspeed_sgpio *gpio;
+ struct resource *res;
+ u32 nr_gpios, sgpio_freq;
+ int rc;
+ u16 sgpio_clk_div;
+ unsigned long apb_freq;
+
+ gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ gpio->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(gpio->base))
+ return PTR_ERR(gpio->base);
+
+ rc = of_property_read_u32(pdev->dev.of_node, "nr-gpios", &nr_gpios);
+ if ((rc < 0) || (nr_gpios > MAX_NR_SGPIO))
+ nr_gpios = MAX_NR_SGPIO;
+
+ rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Could not read bus-frequency property\n");
+ sgpio_freq = 12000000;
+ }
+
+ gpio->pclk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(gpio->pclk)) {
+ dev_err(&pdev->dev, "devm_clk_get failed\n");
+ return PTR_ERR(gpio->pclk);
+ }
+
+ apb_freq = clk_get_rate(gpio->pclk);
+ sgpio_clk_div = 2 * ((apb_freq % sgpio_freq == 0) ?
+ (apb_freq / sgpio_freq) - 1 : (apb_freq / sgpio_freq));
+ iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
+ FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
+ ASPEED_SGPIO_ENABLE,
+ gpio->base + ASPEED_SGPIO_CTRL);
+
+ spin_lock_init(&gpio->lock);
+
+ gpio->chip.parent = &pdev->dev;
+ gpio->chip.ngpio = nr_gpios;
+ gpio->chip.direction_input = aspeed_sgpio_dir_in;
+ gpio->chip.direction_output = aspeed_sgpio_dir_out;
+ gpio->chip.get_direction = aspeed_sgpio_get_direction;
+ gpio->chip.request = NULL;
+ gpio->chip.free = NULL;
+ gpio->chip.get = aspeed_sgpio_get;
+ gpio->chip.set = aspeed_sgpio_set;
+ gpio->chip.set_config = NULL;
+ gpio->chip.label = dev_name(&pdev->dev);
+ gpio->chip.base = ARCH_NR_GPIOS - MAX_NR_SGPIO;
+
+ rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+ if (rc < 0)
+ return rc;
+
+ return aspeed_sgpio_setup_irqs(gpio, pdev);
+}
+
+static struct platform_driver aspeed_sgpio_driver = {
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = aspeed_sgpio_of_table,
+ },
+};
+
+module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
+MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
+MODULE_LICENSE("GPL");
--
2.7.4
>
thanks Anrew, please review v3 at above and also inline comments at below.
>
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Wednesday, July 10, 2019 9:46 PM
> To: Hongwei Zhang; Bartosz Golaszewski; Joel Stanley; Linus Walleij
> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3 v2] ARM: dts: aspeed: Add SGPIO driver
>
>
>
> On Thu, 11 Jul 2019, at 00:56, Hongwei Zhang wrote:
> > Add SGPIO driver support for Aspeed AST2500 SoC.
> >
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > ---
> > drivers/gpio/sgpio-aspeed.c | 450
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 450 insertions(+)
> > create mode 100644 drivers/gpio/sgpio-aspeed.c
> >
> > diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
> > new file mode 100644 index 0000000..0743d22
> > --- /dev/null
> > +++ b/drivers/gpio/sgpio-aspeed.c
> > @@ -0,0 +1,450 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2019 American Megatrends International LLC.
> > + *
> > + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in> */
> > +
> > +#include <linux/gpio/driver.h>
> > +#include <linux/gpio/aspeed.h>
> > +#include <linux/hashtable.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/string.h>
> > +
> > +#define NR_SGPIO 80
> > +
> > +struct aspeed_sgpio {
> > + struct gpio_chip chip;
> > + spinlock_t lock;
> > + void __iomem *base;
> > + int irq;
> > +};
> > +
> > +struct aspeed_sgpio_bank {
> > + uint16_t val_regs;
> > + uint16_t rdata_reg;
> > + uint16_t irq_regs;
> > + const char names[4][3];
> > +};
> > +
> > +/*
> > + * Note: The "value" register returns the input value sampled on the
> > + * line even when the GPIO is configured as an output. Since
> > + * that input goes through synchronizers, writing, then reading
> > + * back may not return the written value right away.
> > + *
> > + * The "rdata" register returns the content of the write latch
> > + * and thus can be used to read back what was last written
> > + * reliably.
> > + */
> > +
> > +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> > + {
> > + .val_regs = 0x0000,
> > + .rdata_reg = 0x0070,
> > + .irq_regs = 0x0004,
> > + .names = { "A", "B", "C", "D" },
> > + },
> > + {
> > + .val_regs = 0x001C,
> > + .rdata_reg = 0x0074,
> > + .irq_regs = 0x0020,
> > + .names = { "E", "F", "G", "H" },
> > + },
> > + {
> > + .val_regs = 0x0038,
> > + .rdata_reg = 0x0078,
> > + .irq_regs = 0x003C,
> > + .names = { "I", "J" },
> > + },
> > +};
> > +
> > +enum aspeed_sgpio_reg {
> > + reg_val,
> > + reg_rdata,
> > + reg_irq_enable,
> > + reg_irq_type0,
> > + reg_irq_type1,
> > + reg_irq_type2,
> > + reg_irq_status,
> > +};
> > +
> > +#define GPIO_VAL_VALUE 0x00
> > +#define GPIO_VAL_DIR 0x04
> > +#define GPIO_IRQ_ENABLE 0x00
> > +#define GPIO_IRQ_TYPE0 0x04
> > +#define GPIO_IRQ_TYPE1 0x08
> > +#define GPIO_IRQ_TYPE2 0x0C
> > +#define GPIO_IRQ_STATUS 0x10
> > +
> > +/* This will be resolved at compile time */ static inline void
> > +__iomem *bank_reg(struct aspeed_sgpio *gpio,
> > + const struct aspeed_sgpio_bank *bank,
> > + const enum aspeed_sgpio_reg reg) {
> > + switch (reg) {
> > + case reg_val:
> > + return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> > + case reg_rdata:
> > + return gpio->base + bank->rdata_reg;
> > + case reg_irq_enable:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
> > + case reg_irq_type0:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
> > + case reg_irq_type1:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
> > + case reg_irq_type2:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
> > + case reg_irq_status:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
> > + default:
> > + /* acturally if code runs to here, it's an error case */
> > + WARN_ON(reg);
> > + return gpio->base;
> > + }
> > +}
> > +
updated to use BUG_ON(1), please see v3.
> > +#define GPIO_BANK(x) ((x) >> 5)
> > +#define GPIO_OFFSET(x) ((x) & 0x1f)
> > +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
> > +
> > +static const struct aspeed_sgpio_bank *to_bank(unsigned int offset) {
> > + unsigned int bank = GPIO_BANK(offset);
> > +
> > + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
> > + return &aspeed_sgpio_banks[bank];
> > +}
> > +
> > +static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int
> > +offset) {
> > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > + const struct aspeed_sgpio_bank *bank = to_bank(offset);
> > +
> > + return !!(ioread32(bank_reg(gpio, bank, reg_val)) &
> > +GPIO_BIT(offset)); }
> > +
> > +static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int
> > offset, int val)
> > +{
> > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > + const struct aspeed_sgpio_bank *bank = to_bank(offset);
> > + unsigned long flags;
> > + void __iomem *addr;
> > + u32 reg = 0;
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + addr = bank_reg(gpio, bank, reg_val);
> > +
> > + if (val)
> > + reg |= GPIO_BIT(offset);
> > + else
> > + reg &= ~GPIO_BIT(offset);
> > +
> > + iowrite32(reg, addr);
> > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > +
> > +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int
> > offset)
> > +{
> > + /* By default all SGPIO Pins are input */
> > + return 0;
> > +}
> > +
> > +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int
> > offset, int val)
> > +{
> > + return 0;
> > +}
> > +
> > +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned
> > int offset)
> > +{
> > + /* By default all SGPIO Pins are input */
> > + return 1;
> > +
> > +}
>
> Please see my follow-up reply on v1 that helps clarify what we should do with the
> dir_in()/dir_out()/get_direction() implementations. The implementation here will confuse everything in
> the stack above it.
>
updated, please see v3.
> > +
> > +static inline int irqd_to_aspeed_sgpio_data(struct irq_data *d,
> > + struct aspeed_sgpio **gpio,
> > + const struct aspeed_sgpio_bank **bank,
> > + u32 *bit, int *offset)
> > +{
> > + struct aspeed_sgpio *internal;
> > +
> > + *offset = irqd_to_hwirq(d);
> > +
> > + internal = irq_data_get_irq_chip_data(d);
> > +
> > + *gpio = internal;
> > + *bank = to_bank(*offset);
> > + *bit = GPIO_BIT(*offset);
> > +
> > + return 0;
> > +}
> > +
> > +static void aspeed_sgpio_irq_ack(struct irq_data *d) {
> > + const struct aspeed_sgpio_bank *bank;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + void __iomem *status_addr;
> > + int rc, offset;
> > + u32 bit;
> > +
> > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > + if (rc)
> > + return;
> > +
> > + status_addr = bank_reg(gpio, bank, reg_irq_status);
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + iowrite32(bit, status_addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > +
> > +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set) {
> > + const struct aspeed_sgpio_bank *bank;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + u32 reg, bit;
> > + void __iomem *addr;
> > + int rc, offset;
> > +
> > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > + if (rc)
> > + return;
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_enable);
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + reg = ioread32(addr);
> > + if (set)
> > + reg |= bit;
> > + else
> > + reg &= ~bit;
> > +
> > + iowrite32(reg, addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > +
> > +static void aspeed_sgpio_irq_mask(struct irq_data *d) {
> > + aspeed_sgpio_irq_set_mask(d, false); }
> > +
> > +static void aspeed_sgpio_irq_unmask(struct irq_data *d) {
> > + aspeed_sgpio_irq_set_mask(d, true);
> > +}
> > +
> > +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int
> > +type) {
> > + u32 type0 = 0;
> > + u32 type1 = 0;
> > + u32 type2 = 0;
> > + u32 bit, reg;
> > + const struct aspeed_sgpio_bank *bank;
> > + irq_flow_handler_t handler;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + void __iomem *addr;
> > + int rc, offset;
> > +
> > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > + if (rc)
> > + return -EINVAL;
> > +
> > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > + case IRQ_TYPE_EDGE_BOTH:
> > + type2 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_EDGE_RISING:
> > + type0 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_EDGE_FALLING:
> > + handler = handle_edge_irq;
> > + break;
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + type0 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_LEVEL_LOW:
> > + type1 |= bit;
> > + handler = handle_level_irq;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type0);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type0;
> > + iowrite32(reg, addr);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type1);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type1;
> > + iowrite32(reg, addr);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type2);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type2;
> > + iowrite32(reg, addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + irq_set_handler_locked(d, handler);
> > +
> > + return 0;
> > +}
> > +
> > +static void aspeed_sgpio_irq_handler(struct irq_desc *desc) {
> > + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> > + struct irq_chip *ic = irq_desc_get_chip(desc);
> > + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> > + unsigned int i, p, girq;
> > + unsigned long reg;
> > +
> > + chained_irq_enter(ic, desc);
> > +
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> > +
> > + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> > +
> > + for_each_set_bit(p, ®, 32) {
> > + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> > + generic_handle_irq(girq);
> > + }
> > +
> > + }
> > +
> > + chained_irq_exit(ic, desc);
> > +}
> > +
> > +static struct irq_chip aspeed_sgpio_irqchip = {
> > + .name = "aspeed-sgpio",
> > + .irq_ack = aspeed_sgpio_irq_ack,
> > + .irq_mask = aspeed_sgpio_irq_mask,
> > + .irq_unmask = aspeed_sgpio_irq_unmask,
> > + .irq_set_type = aspeed_sgpio_set_type,
> > +};
> > +
> > +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
> > + struct platform_device *pdev)
> > +{
> > + int rc, i;
> > + const struct aspeed_sgpio_bank *bank;
> > +
> > + rc = platform_get_irq(pdev, 0);
> > + if (rc < 0)
> > + return rc;
> > +
> > + gpio->irq = rc;
> > +
> > + /* Disable IRQ and clear Interrupt status registers for all SPGIO
> > Pins. */
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + bank = &aspeed_sgpio_banks[i];
> > + /* disable irq enable bits */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
> > + /* clear status bits */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
> > + }
> > +
> > + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
> > + 0, handle_bad_irq, IRQ_TYPE_NONE);
> > + if (rc) {
> > + dev_info(&pdev->dev, "Could not add irqchip\n");
> > + return rc;
> > + }
> > +
> > + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
> > + gpio->irq, aspeed_sgpio_irq_handler);
> > +
> > + /* set IRQ settings and Enable Interrupt */
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + bank = &aspeed_sgpio_banks[i];
> > + /* set falling or level-low irq */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
> > + /* trigger type is edge */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
> > + /* dual edge trigger mode. */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
> > + /* enable irq */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int aspeed_sgpio_request(struct gpio_chip *chip, unsigned int
> > offset)
> > +{
> > + return (offset < NR_SGPIO);
> > +}
>
> I don't think this request() implementation is helpful, especially as it stands in the face of needing to pull
> the number of GPIOs to serialise from the devicetree.
>
> request() is an optional callback, lets just drop it.
>
updated, please see v3.
> > +
> > +static const struct of_device_id aspeed_sgpio_of_table[] = {
> > + { .compatible = "aspeed,ast2400-sgpio" },
> > + { .compatible = "aspeed,ast2500-sgpio" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
> > +
> > +static int __init aspeed_sgpio_probe(struct platform_device *pdev) {
> > + struct aspeed_sgpio *gpio;
> > + struct resource *res;
> > + int rc;
> > +
> > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> > + if (!gpio)
> > + return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + gpio->base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(gpio->base))
> > + return PTR_ERR(gpio->base);
> > +
> > + spin_lock_init(&gpio->lock);
> > +
> > + gpio->chip.parent = &pdev->dev;
> > + gpio->chip.ngpio = NR_SGPIO;
> > + gpio->chip.direction_input = aspeed_sgpio_dir_in;
> > + gpio->chip.direction_output = aspeed_sgpio_dir_out;
> > + gpio->chip.get_direction = aspeed_sgpio_get_direction;
> > + gpio->chip.request = aspeed_sgpio_request;
> > + gpio->chip.free = NULL;
> > + gpio->chip.get = aspeed_sgpio_get;
> > + gpio->chip.set = aspeed_sgpio_set;
> > + gpio->chip.set_config = NULL;
> > + gpio->chip.label = dev_name(&pdev->dev);
> > + gpio->chip.base = -1;
> > +
> > + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> > + if (rc < 0)
> > + return rc;
> > +
> > + return aspeed_sgpio_setup_irqs(gpio, pdev); }
> > +
> > +static struct platform_driver aspeed_sgpio_driver = {
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .of_match_table = aspeed_sgpio_of_table,
> > + },
> > +};
> > +
> > +module_platform_driver_probe(aspeed_sgpio_driver,
> > +aspeed_sgpio_probe); MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.7.4
> >
> >
^ permalink raw reply related
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 21:35 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <fd8bad73-464b-54f1-be94-fe3ac8b23e6e@gmail.com>
On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>> The other thing that also need attention is that T124 CPUFreq
>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed first, which is
>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Should I add check for successful dfll clk register
>>>>>>>>>>>>> explicitly in
>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>>>>>>> Probably you should use the "device links". See [1][2] for the
>>>>>>>>>> example.
>>>>>>>>>>
>>>>>>>>>> [1]
>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> [2]
>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>
>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails.
>>>>>>>>>> And
>>>>>>>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>>>>>>>
>>>>>>>>>> [3]
>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Will go thru and add...
>>>>>>> Looks like I initially confused this case with getting orphaned
>>>>>>> clock.
>>>>>>> I'm now seeing that the DFLL driver registers the clock and then
>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
>>>>>>> probed, hence everything should be fine as-is and there is no real
>>>>>>> need
>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>
>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL
>>>>>>>>>>>> part.
>>>>>>>>>>>>
>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU clock
>>>>>>>>>>>> sources and
>>>>>>>>>>>> integrated with DVFS control logic with the regulator. We
>>>>>>>>>>>> will not
>>>>>>>>>>>> switch
>>>>>>>>>>>> CPU to other clock sources once we switched to DFLL. Because the
>>>>>>>>>>>> CPU has
>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP
>>>>>>>>>>>> table
>>>>>>>>>>>> you see
>>>>>>>>>>>> in the driver.). We shouldn't reparent it to other sources with
>>>>>>>>>>>> unknew
>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow
>>>>>>>>>>>> switching to
>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to
>>>>>>>>>> PLLP's
>>>>>>>>>> rate before switching to PLLP in order to have a proper CPU
>>>>>>>>>> voltage.
>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no need to
>>>>>>>>> enforce
>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
>>>>>>>>> suspend
>>>>>>>>>
>>>>>>>> Sorry, please ignore my above comment. During suspend, need to
>>>>>>>> change
>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop mode first and
>>>>>>>> then
>>>>>>>> dfll need to be set to open loop.
>>>>>>> Okay.
>>>>>>>
>>>>>>>>>>>> And I don't exactly understand why we need to switch to PLLP in
>>>>>>>>>>>> CPU
>>>>>>>>>>>> idle
>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>>>>>>
>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>>>>>>>> mode. That's
>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest of the
>>>>>>>>>>>> sequence to
>>>>>>>>>>>> turn off
>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>
>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the sequence to
>>>>>>>>>>>> turn on
>>>>>>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P.
>>>>>>>>>>>> After
>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent
>>>>>>>>>> during of
>>>>>>>>>> the
>>>>>>>>>> early clocks-state restoring by CaR driver. Hence instead of
>>>>>>>>>> having
>>>>>>>>>> odd
>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>>>>>>> suspend-resume sequencing of the device drivers. In this case
>>>>>>>>>> CPUFreq
>>>>>>>>>> driver is the driver that enables DFLL and switches CPU to that
>>>>>>>>>> clock
>>>>>>>>>> source, which means that this driver is also should be
>>>>>>>>>> responsible for
>>>>>>>>>> management of the DFLL's state during of suspend/resume
>>>>>>>>>> process. If
>>>>>>>>>> CPUFreq driver disables DFLL during suspend and re-enables it
>>>>>>>>>> during
>>>>>>>>>> resume, then looks like the CaR driver hacks around DFLL are not
>>>>>>>>>> needed.
>>>>>>>>>>
>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the patch subject to
>>>>>>>>>>>> "Add
>>>>>>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>>>>>>
>>>>>>>>>>> To clarify this, the sequences for DFLL use are as follows
>>>>>>>>>>> (assuming
>>>>>>>>>>> all
>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>
>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>
>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for
>>>>>>>>>>> any
>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>
>>>>>>>> I see during switch away from DFLL (suspend), cclk_g parent is not
>>>>>>>> changed to PLLP before changing dfll to open loop mode.
>>>>>>>>
>>>>>>>> Will add this ...
>>>>>>> The CPUFreq driver switches parent to PLLP during the probe, similar
>>>>>>> should be done on suspend.
>>>>>>>
>>>>>>> I'm also wondering if it's always safe to switch to PLLP in the
>>>>>>> probe.
>>>>>>> If CPU is running on a lower freq than PLLP, then some other more
>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>
>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher
>>>>>> rate
>>>>>> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
>>>>>> should be safe.
>>>>> AFAIK, PLLX could run at ~200MHz. There is also a divided output of
>>>>> PLLP
>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>
>>>>> Probably, realistically, CPU is always running off a fast PLLX during
>>>>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>>>>> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
>>>>> on a reboot, but likely that there are other clock-related problems as
>>>>> well that may break KEXEC and thus it is not very important at the
>>>>> moment.
>>>>>
>>>>> [snip]
>>>> During bootup CPUG sources from PLL_X. By PLL_P source above I meant
>>>> PLL_P_OUT4.
>>>>
>>>> As per clock policies, PLL_X is always used for high freq like >800Mhz
>>>> and for low frequency it will be sourced from PLLP.
>>> Alright, then please don't forget to pre-initialize PLLP_OUT4 rate to a
>>> reasonable value using tegra_clk_init_table or assigned-clocks.
>> PLLP_OUT4 rate update is not needed as it is safe to run at 408Mhz
>> because it is below fmax @ Vmin
> So even 204MHz CVB entries are having the same voltage as 408MHz,
> correct? It's not instantly obvious to me from the DFLL driver's code
> where the fmax @ Vmin is defined, I see that there is the min_millivolts
> and frequency entries starting from 204MHZ defined per-table.
Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will work at
Vmin voltage and PLLP max is 408Mhz.
^ permalink raw reply
* Re: [PATCH] gpio: don't WARN() on NULL descs if gpiolib is disabled
From: Linus Walleij @ 2019-07-16 21:46 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: open list:GPIO SUBSYSTEM, linux-kernel@vger.kernel.org,
Bartosz Golaszewski, Claus H . Stovgaard
In-Reply-To: <CAMRc=MfB9R70QDqtjG5a5Roq1roeL78Ss5noytrY-7P=tY1OHA@mail.gmail.com>
On Tue, Jul 9, 2019 at 4:20 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> wt., 9 lip 2019 o 15:30 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> > I was thinking something like this in the stubs:
> >
> > gpiod_get[_index]() {
> > return POISON;
> > }
> >
> > gpiod_get[_index]_optional() {
> > return NULL;
> > }
>
> This is already being done.
Ah it is.
> > This way all gpiod_get() and optional calls are properly
> > handled and the semantic that only _optional calls
> > can return NULL is preserved. (Your patch would
> > violate this.)
> >
>
> Maybe I'm missing something, but I don't quite see how my patch
> violates this behavior. :(
I missed that we actually do pass a poison from the strict
*get functions, mea culpa.
Let's apply this, will you send me a pull request or shall I
just try to apply it?
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH 2/3 v3] dt-bindings: gpio: aspeed: Add SGPIO support
From: Hongwei Zhang @ 2019-07-16 21:48 UTC (permalink / raw)
To: Joel Stanley, Andrew Jeffery, Linus Walleij, devicetree
Cc: Hongwei Zhang, Rob Herring, Mark Rutland, Bartosz Golaszewski,
linux-aspeed, linux-kernel, linux-arm-kernel, linux-gpio
Add bindings to support SGPIO on AST2400 or AST2500.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
.../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
1 file changed, 55 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
new file mode 100644
index 0000000..8c3a747
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
@@ -0,0 +1,55 @@
+Aspeed SGPIO controller Device Tree Bindings
+-------------------------------------------
+
+This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80 full
+featured Serial GPIOs. Each of the Serial GPIO pins can be programmed to
+support the following options:
+- Support interrupt option for each input port and various interrupt
+ sensitivity option (level-high, level-low, edge-high, edge-low)
+- Support reset tolerance option for each output port
+- Directly connected to APB bus and its shift clock is from APB bus clock
+ divided by a programmable value.
+- Co-work with external signal-chained TTL components (74LV165/74LV595)
+
+
+Required properties:
+
+- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
+
+- #gpio-cells : Should be two
+ - First cell is the GPIO line number
+ - Second cell is used to specify optional
+ parameters (unused)
+
+- reg : Address and length of the register set for the device
+- gpio-controller : Marks the device node as a GPIO controller.
+- interrupts : Interrupt specifier (see interrupt bindings for
+ details)
+
+- interrupt-controller : Mark the GPIO controller as an interrupt-controller
+
+- nr-gpios : number of GPIO pins to serialise.
+ (should be multiple of 8, up to 80 pins; 0 if not used)
+
+- clocks : A phandle to the APB clock for SGPM clock division
+
+- bus-frequency : SGPM CLK frequency, derived from APB bus clock by a programmable devisor
+
+
+The sgpio and interrupt properties are further described in their respective bindings documentation:
+
+- Documentation/devicetree/bindings/sgpio/gpio.txt
+- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+
+ Example:
+ sgpio@1e780200 {
+ #gpio-cells = <2>;
+ compatible = "aspeed,ast2500-sgpio";
+ gpio-controller;
+ interrupts = <40>;
+ reg = <0x1e780200 0x0100>;
+ clocks = <&syscon ASPEED_CLK_APB>;
+ interrupt-controller;
+ nr-gpios = <8>;
+ bus-frequency = <12000000>;
+ };
--
2.7.4
Thanks Andrew, please see above v3 and inline comments at below.
--Hongwei
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Sunday, July 14, 2019 10:25 PM
> To: Hongwei Zhang; Joel Stanley; Linus Walleij; devicetree@vger.kernel.org
> Cc: Rob Herring; Mark Rutland; Bartosz Golaszewski; linux-aspeed@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-gpio@vger.kernel.org
> Subject: Re: [PATCH 2/3 v2] dt-bindings: gpio: aspeed: Add SGPIO support
>
> Hello Hongwei,
>
> On Sat, 13 Jul 2019, at 05:44, Hongwei Zhang wrote:
> > Add bindings to support SGPIO on AST2400 or AST2500.
> >
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > ---
> > .../devicetree/bindings/gpio/sgpio-aspeed.txt | 43 ++++++++++++++++++++++
> > 1 file changed, 43 insertions(+)
> > create mode 100755
> > Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > new file mode 100755
> > index 0000000..3ae2b79
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > @@ -0,0 +1,43 @@
> > +Aspeed SGPIO controller Device Tree Bindings
> > +-------------------------------------------
> > +
> > +Required properties:
> > +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> > +
> > +- #gpio-cells : Should be two
> > + - First cell is the GPIO line number
> > + - Second cell is used to specify optional
> > + parameters (unused)
> > +
> > +- reg : Address and length of the register set for the device
> > +- gpio-controller : Marks the device node as a GPIO controller.
> > +- interrupts : Interrupt specifier (see interrupt bindings for
> > + details)
> > +
> > +- interrupt-controller : Mark the GPIO controller as an
> > interrupt-controller
> > +
> > +- nr-gpios : number of GPIO pins to serialise. (should be multiple of
> > 8, up to 80 pins)
> > + if not specified, defaults to 80.
>
> This appears to be a statement about the driver implementation, but bindings documents are about
> describing hardware. Reading the datasheet it actually appears the ASPEED SGPIO hardware comes up
> in what is "technically" a forbidden state (equivalent to `nr-gpios = <0>;`), though the device is also
> disabled at this point, so it's probably moot. The point is the true default value from a hardware
> perspective is 0, not 80, so if we're going to talk about default values, 0 would be more appropriate.
> However:
>
> You've also listed nr-gpios under the "Required properties" header, but the description suggests it's
> optional. It's either one or the other, please lets be clear about it. On that front, lets make it nr-gpios
> *not* optional (i.e. make it
> required) thus force the specification of how many SGPIOs we want to emit on the bus. This value is
> coupled to the platform design, so I don't think there's ever a scenario where we want nr-gpios to take a
> default value.
>
Added some descriptions and updated nr-gpios, please see v3.
> > +
> > +- clocks : A phandle to the APB clock for SGPM clock
> > division
> > +
> > +- bus-frequency : SGPM CLK frequency, if not specified defaults to 1
> > MHz
>
> Again here with the default value - SGPM CLK period is derived from PCLK by the expression `period =
> PCLK * 2 *(GPIO254[31:16] + 1)`, where GPIO254's initialisation state is `GPIO254[31:16] = 0`, which
> gives a default SGPM bus frequency of PCLK / 2. This is likely not going to be 1MHz (more like ~12MHz).
>
> Lets just make the property required. That way we avoid any ambiguity about the bus frequency and
> thus don't need words about defaults that turn out to be about the driver, not about the hardware.
>
updated, please see v3.
> Finally, when updating patches in response to feedback, please send the full series again, and bump the
> series version number. That way people can review a coherent set of patches and not have to hunt
> around and (fail to) collate the correct combination. It makes it easier to say "Reviewed-by:" on your
> patches :)
>
> Cheers,
>
> Andrew
^ permalink raw reply related
* Re: WARNING in gpio_to_desc
From: Linus Walleij @ 2019-07-16 21:52 UTC (permalink / raw)
To: syzbot, Johan Hovold, Vincent Cuissard
Cc: Andrey Konovalov, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
linux-kernel@vger.kernel.org, linux-usb, syzkaller-bugs
In-Reply-To: <000000000000a55d7d058d51ad4f@google.com>
On Wed, Jul 10, 2019 at 1:07 PM syzbot
<syzbot+cf35b76f35e068a1107f@syzkaller.appspotmail.com> wrote:
> HEAD commit: 7829a896 usb-fuzzer: main usb gadget fuzzer driver
(...)
> __gpio_set_value include/asm-generic/gpio.h:104 [inline]
> gpio_set_value include/linux/gpio.h:71 [inline]
> nfcmrvl_chip_halt+0x4e/0x70 drivers/nfc/nfcmrvl/main.c:259
> nfcmrvl_nci_register_dev+0x2d4/0x378 drivers/nfc/nfcmrvl/main.c:176
> nfcmrvl_probe+0x4e9/0x5e0 drivers/nfc/nfcmrvl/usb.c:344
This bug is somewhere in the drivers/nfc/nfcmrvl* code handling
GPIOs.
It should be converted to GPIO descriptors and fixed up, see
drivers/gpio/TODO for details on how to do this.
Johan/Vincent, tell me if you want me to forward the full fuzzing
robot crash dump.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH] dt-bindings: pinctrl: stm32: Fix missing 'clocks' property in examples
From: Rob Herring @ 2019-07-16 21:56 UTC (permalink / raw)
To: Linus Walleij
Cc: devicetree, linux-arm-kernel, linux-kernel, Maxime Coquelin,
Alexandre Torgue, linux-gpio, linux-stm32
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>
---
.../devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml
index 3ac5d2088e49..91d3e78b3395 100644
--- a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml
@@ -197,6 +197,7 @@ required:
examples:
- |
#include <dt-bindings/pinctrl/stm32-pinfunc.h>
+ #include <dt-bindings/mfd/stm32f4-rcc.h>
//Example 1
pinctrl@40020000 {
#address-cells = <1>;
@@ -210,6 +211,7 @@ examples:
#gpio-cells = <2>;
reg = <0x0 0x400>;
resets = <&reset_ahb1 0>;
+ clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOA)>;
st,bank-name = "GPIOA";
};
};
@@ -227,6 +229,7 @@ examples:
#gpio-cells = <2>;
reg = <0x1000 0x400>;
resets = <&reset_ahb1 0>;
+ clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOB)>;
st,bank-name = "GPIOB";
gpio-ranges = <&pinctrl 0 0 16>;
};
@@ -236,6 +239,7 @@ examples:
#gpio-cells = <2>;
reg = <0x2000 0x400>;
resets = <&reset_ahb1 0>;
+ clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOC)>;
st,bank-name = "GPIOC";
ngpios = <5>;
gpio-ranges = <&pinctrl 0 16 3>,
--
2.20.1
^ permalink raw reply related
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 22:00 UTC (permalink / raw)
To: Sowjanya Komatineni, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <0ee06d1a-310d-59f7-0aa6-b688b33447f5@nvidia.com>
17.07.2019 0:35, Sowjanya Komatineni пишет:
>
> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>> The other thing that also need attention is that T124
>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed first,
>>>>>>>>>>>>>>> which is
>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Should I add check for successful dfll clk register
>>>>>>>>>>>>>> explicitly in
>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>>>>>>>> Probably you should use the "device links". See [1][2] for the
>>>>>>>>>>> example.
>>>>>>>>>>>
>>>>>>>>>>> [1]
>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> [2]
>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add()
>>>>>>>>>>> fails.
>>>>>>>>>>> And
>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>>>>>>>>
>>>>>>>>>>> [3]
>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> Will go thru and add...
>>>>>>>> Looks like I initially confused this case with getting orphaned
>>>>>>>> clock.
>>>>>>>> I'm now seeing that the DFLL driver registers the clock and then
>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
>>>>>>>> probed, hence everything should be fine as-is and there is no real
>>>>>>>> need
>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>
>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the
>>>>>>>>>>>>> DFLL
>>>>>>>>>>>>> part.
>>>>>>>>>>>>>
>>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU clock
>>>>>>>>>>>>> sources and
>>>>>>>>>>>>> integrated with DVFS control logic with the regulator. We
>>>>>>>>>>>>> will not
>>>>>>>>>>>>> switch
>>>>>>>>>>>>> CPU to other clock sources once we switched to DFLL.
>>>>>>>>>>>>> Because the
>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP
>>>>>>>>>>>>> table
>>>>>>>>>>>>> you see
>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to other sources
>>>>>>>>>>>>> with
>>>>>>>>>>>>> unknew
>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow
>>>>>>>>>>>>> switching to
>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to
>>>>>>>>>>> PLLP's
>>>>>>>>>>> rate before switching to PLLP in order to have a proper CPU
>>>>>>>>>>> voltage.
>>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no need to
>>>>>>>>>> enforce
>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP
>>>>>>>>>> during
>>>>>>>>>> suspend
>>>>>>>>>>
>>>>>>>>> Sorry, please ignore my above comment. During suspend, need to
>>>>>>>>> change
>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop mode first and
>>>>>>>>> then
>>>>>>>>> dfll need to be set to open loop.
>>>>>>>> Okay.
>>>>>>>>
>>>>>>>>>>>>> And I don't exactly understand why we need to switch to
>>>>>>>>>>>>> PLLP in
>>>>>>>>>>>>> CPU
>>>>>>>>>>>>> idle
>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest of the
>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>> turn off
>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the sequence to
>>>>>>>>>>>>> turn on
>>>>>>>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P.
>>>>>>>>>>>>> After
>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop
>>>>>>>>>>>>> mode.
>>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent
>>>>>>>>>>> during of
>>>>>>>>>>> the
>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence instead of
>>>>>>>>>>> having
>>>>>>>>>>> odd
>>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>>>>>>>> suspend-resume sequencing of the device drivers. In this case
>>>>>>>>>>> CPUFreq
>>>>>>>>>>> driver is the driver that enables DFLL and switches CPU to that
>>>>>>>>>>> clock
>>>>>>>>>>> source, which means that this driver is also should be
>>>>>>>>>>> responsible for
>>>>>>>>>>> management of the DFLL's state during of suspend/resume
>>>>>>>>>>> process. If
>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and re-enables it
>>>>>>>>>>> during
>>>>>>>>>>> resume, then looks like the CaR driver hacks around DFLL are not
>>>>>>>>>>> needed.
>>>>>>>>>>>
>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the patch
>>>>>>>>>>>>> subject to
>>>>>>>>>>>>> "Add
>>>>>>>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>>>>>>>
>>>>>>>>>>>> To clarify this, the sequences for DFLL use are as follows
>>>>>>>>>>>> (assuming
>>>>>>>>>>>> all
>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>
>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>
>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for
>>>>>>>>>>>> any
>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>
>>>>>>>>> I see during switch away from DFLL (suspend), cclk_g parent is not
>>>>>>>>> changed to PLLP before changing dfll to open loop mode.
>>>>>>>>>
>>>>>>>>> Will add this ...
>>>>>>>> The CPUFreq driver switches parent to PLLP during the probe,
>>>>>>>> similar
>>>>>>>> should be done on suspend.
>>>>>>>>
>>>>>>>> I'm also wondering if it's always safe to switch to PLLP in the
>>>>>>>> probe.
>>>>>>>> If CPU is running on a lower freq than PLLP, then some other more
>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>
>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher
>>>>>>> rate
>>>>>>> so switching to PLL_P during CPUFreq probe prior to dfll clock
>>>>>>> enable
>>>>>>> should be safe.
>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a divided output of
>>>>>> PLLP
>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>
>>>>>> Probably, realistically, CPU is always running off a fast PLLX during
>>>>>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>>>>>> CPUFreq driver should also have a 'shutdown' callback to teardown
>>>>>> DFLL
>>>>>> on a reboot, but likely that there are other clock-related
>>>>>> problems as
>>>>>> well that may break KEXEC and thus it is not very important at the
>>>>>> moment.
>>>>>>
>>>>>> [snip]
>>>>> During bootup CPUG sources from PLL_X. By PLL_P source above I meant
>>>>> PLL_P_OUT4.
>>>>>
>>>>> As per clock policies, PLL_X is always used for high freq like >800Mhz
>>>>> and for low frequency it will be sourced from PLLP.
>>>> Alright, then please don't forget to pre-initialize PLLP_OUT4 rate to a
>>>> reasonable value using tegra_clk_init_table or assigned-clocks.
>>> PLLP_OUT4 rate update is not needed as it is safe to run at 408Mhz
>>> because it is below fmax @ Vmin
>> So even 204MHz CVB entries are having the same voltage as 408MHz,
>> correct? It's not instantly obvious to me from the DFLL driver's code
>> where the fmax @ Vmin is defined, I see that there is the min_millivolts
>> and frequency entries starting from 204MHZ defined per-table.
> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will work at
> Vmin voltage and PLLP max is 408Mhz.
Thank you for the clarification. It would be good to have that commented
in the code as well.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 22:06 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <cedfafd0-4114-0821-0c4b-efc17c213449@gmail.com>
On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>> The other thing that also need attention is that T124
>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed first,
>>>>>>>>>>>>>>>> which is
>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Should I add check for successful dfll clk register
>>>>>>>>>>>>>>> explicitly in
>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>>>>>>>>> Probably you should use the "device links". See [1][2] for the
>>>>>>>>>>>> example.
>>>>>>>>>>>>
>>>>>>>>>>>> [1]
>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> [2]
>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add()
>>>>>>>>>>>> fails.
>>>>>>>>>>>> And
>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's device, see [3].
>>>>>>>>>>>>
>>>>>>>>>>>> [3]
>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>> Will go thru and add...
>>>>>>>>> Looks like I initially confused this case with getting orphaned
>>>>>>>>> clock.
>>>>>>>>> I'm now seeing that the DFLL driver registers the clock and then
>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
>>>>>>>>> probed, hence everything should be fine as-is and there is no real
>>>>>>>>> need
>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>
>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the
>>>>>>>>>>>>>> DFLL
>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU clock
>>>>>>>>>>>>>> sources and
>>>>>>>>>>>>>> integrated with DVFS control logic with the regulator. We
>>>>>>>>>>>>>> will not
>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>> CPU to other clock sources once we switched to DFLL.
>>>>>>>>>>>>>> Because the
>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP
>>>>>>>>>>>>>> table
>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to other sources
>>>>>>>>>>>>>> with
>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow
>>>>>>>>>>>>>> switching to
>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to
>>>>>>>>>>>> PLLP's
>>>>>>>>>>>> rate before switching to PLLP in order to have a proper CPU
>>>>>>>>>>>> voltage.
>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no need to
>>>>>>>>>>> enforce
>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP
>>>>>>>>>>> during
>>>>>>>>>>> suspend
>>>>>>>>>>>
>>>>>>>>>> Sorry, please ignore my above comment. During suspend, need to
>>>>>>>>>> change
>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop mode first and
>>>>>>>>>> then
>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>> Okay.
>>>>>>>>>
>>>>>>>>>>>>>> And I don't exactly understand why we need to switch to
>>>>>>>>>>>>>> PLLP in
>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop
>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest of the
>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the sequence to
>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P.
>>>>>>>>>>>>>> After
>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop
>>>>>>>>>>>>>> mode.
>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent
>>>>>>>>>>>> during of
>>>>>>>>>>>> the
>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence instead of
>>>>>>>>>>>> having
>>>>>>>>>>>> odd
>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>>>>>>>>> suspend-resume sequencing of the device drivers. In this case
>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>> driver is the driver that enables DFLL and switches CPU to that
>>>>>>>>>>>> clock
>>>>>>>>>>>> source, which means that this driver is also should be
>>>>>>>>>>>> responsible for
>>>>>>>>>>>> management of the DFLL's state during of suspend/resume
>>>>>>>>>>>> process. If
>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and re-enables it
>>>>>>>>>>>> during
>>>>>>>>>>>> resume, then looks like the CaR driver hacks around DFLL are not
>>>>>>>>>>>> needed.
>>>>>>>>>>>>
>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the patch
>>>>>>>>>>>>>> subject to
>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> To clarify this, the sequences for DFLL use are as follows
>>>>>>>>>>>>> (assuming
>>>>>>>>>>>>> all
>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>
>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>
>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for
>>>>>>>>>>>>> any
>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>
>>>>>>>>>> I see during switch away from DFLL (suspend), cclk_g parent is not
>>>>>>>>>> changed to PLLP before changing dfll to open loop mode.
>>>>>>>>>>
>>>>>>>>>> Will add this ...
>>>>>>>>> The CPUFreq driver switches parent to PLLP during the probe,
>>>>>>>>> similar
>>>>>>>>> should be done on suspend.
>>>>>>>>>
>>>>>>>>> I'm also wondering if it's always safe to switch to PLLP in the
>>>>>>>>> probe.
>>>>>>>>> If CPU is running on a lower freq than PLLP, then some other more
>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>
>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher
>>>>>>>> rate
>>>>>>>> so switching to PLL_P during CPUFreq probe prior to dfll clock
>>>>>>>> enable
>>>>>>>> should be safe.
>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a divided output of
>>>>>>> PLLP
>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>
>>>>>>> Probably, realistically, CPU is always running off a fast PLLX during
>>>>>>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>>>>>>> CPUFreq driver should also have a 'shutdown' callback to teardown
>>>>>>> DFLL
>>>>>>> on a reboot, but likely that there are other clock-related
>>>>>>> problems as
>>>>>>> well that may break KEXEC and thus it is not very important at the
>>>>>>> moment.
>>>>>>>
>>>>>>> [snip]
>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source above I meant
>>>>>> PLL_P_OUT4.
>>>>>>
>>>>>> As per clock policies, PLL_X is always used for high freq like >800Mhz
>>>>>> and for low frequency it will be sourced from PLLP.
>>>>> Alright, then please don't forget to pre-initialize PLLP_OUT4 rate to a
>>>>> reasonable value using tegra_clk_init_table or assigned-clocks.
>>>> PLLP_OUT4 rate update is not needed as it is safe to run at 408Mhz
>>>> because it is below fmax @ Vmin
>>> So even 204MHz CVB entries are having the same voltage as 408MHz,
>>> correct? It's not instantly obvious to me from the DFLL driver's code
>>> where the fmax @ Vmin is defined, I see that there is the min_millivolts
>>> and frequency entries starting from 204MHZ defined per-table.
>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will work at
>> Vmin voltage and PLLP max is 408Mhz.
> Thank you for the clarification. It would be good to have that commented
> in the code as well.
OK, Will add...
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 2:18 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <707c4679-fde6-1714-ced0-dcf7ca8380a9@nvidia.com>
On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
>
> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>>> The other thing that also need attention is that T124
>>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed first,
>>>>>>>>>>>>>>>>> which is
>>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Should I add check for successful dfll clk register
>>>>>>>>>>>>>>>> explicitly in
>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>>>>>>>>>> Probably you should use the "device links". See [1][2] for
>>>>>>>>>>>>> the
>>>>>>>>>>>>> example.
>>>>>>>>>>>>>
>>>>>>>>>>>>> [1]
>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> [2]
>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add()
>>>>>>>>>>>>> fails.
>>>>>>>>>>>>> And
>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's device, see
>>>>>>>>>>>>> [3].
>>>>>>>>>>>>>
>>>>>>>>>>>>> [3]
>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>> Will go thru and add...
>>>>>>>>>> Looks like I initially confused this case with getting orphaned
>>>>>>>>>> clock.
>>>>>>>>>> I'm now seeing that the DFLL driver registers the clock and then
>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL
>>>>>>>>>> driver is
>>>>>>>>>> probed, hence everything should be fine as-is and there is no
>>>>>>>>>> real
>>>>>>>>>> need
>>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>>
>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the
>>>>>>>>>>>>>>> DFLL
>>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU clock
>>>>>>>>>>>>>>> sources and
>>>>>>>>>>>>>>> integrated with DVFS control logic with the regulator. We
>>>>>>>>>>>>>>> will not
>>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>>> CPU to other clock sources once we switched to DFLL.
>>>>>>>>>>>>>>> Because the
>>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB
>>>>>>>>>>>>>>> or OPP
>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to other sources
>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow
>>>>>>>>>>>>>>> switching to
>>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL
>>>>>>>>>>>>> freq to
>>>>>>>>>>>>> PLLP's
>>>>>>>>>>>>> rate before switching to PLLP in order to have a proper CPU
>>>>>>>>>>>>> voltage.
>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no need to
>>>>>>>>>>>> enforce
>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP
>>>>>>>>>>>> during
>>>>>>>>>>>> suspend
>>>>>>>>>>>>
>>>>>>>>>>> Sorry, please ignore my above comment. During suspend, need to
>>>>>>>>>>> change
>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop mode first
>>>>>>>>>>> and
>>>>>>>>>>> then
>>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>>> Okay.
>>>>>>>>>>
>>>>>>>>>>>>>>> And I don't exactly understand why we need to switch to
>>>>>>>>>>>>>>> PLLP in
>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it the
>>>>>>>>>>>>>>> open-loop
>>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest of the
>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the
>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And leave it on
>>>>>>>>>>>>>>> PLL_P.
>>>>>>>>>>>>>>> After
>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU
>>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop
>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent
>>>>>>>>>>>>> during of
>>>>>>>>>>>>> the
>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence instead of
>>>>>>>>>>>>> having
>>>>>>>>>>>>> odd
>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>>>>>>>>>> suspend-resume sequencing of the device drivers. In this case
>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>> driver is the driver that enables DFLL and switches CPU to
>>>>>>>>>>>>> that
>>>>>>>>>>>>> clock
>>>>>>>>>>>>> source, which means that this driver is also should be
>>>>>>>>>>>>> responsible for
>>>>>>>>>>>>> management of the DFLL's state during of suspend/resume
>>>>>>>>>>>>> process. If
>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and re-enables it
>>>>>>>>>>>>> during
>>>>>>>>>>>>> resume, then looks like the CaR driver hacks around DFLL
>>>>>>>>>>>>> are not
>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the patch
>>>>>>>>>>>>>>> subject to
>>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use are as follows
>>>>>>>>>>>>>> (assuming
>>>>>>>>>>>>>> all
>>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is
>>>>>>>>>>>>>> ok for
>>>>>>>>>>>>>> any
>>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>
>>>>>>>>>>> I see during switch away from DFLL (suspend), cclk_g parent
>>>>>>>>>>> is not
>>>>>>>>>>> changed to PLLP before changing dfll to open loop mode.
>>>>>>>>>>>
>>>>>>>>>>> Will add this ...
>>>>>>>>>> The CPUFreq driver switches parent to PLLP during the probe,
>>>>>>>>>> similar
>>>>>>>>>> should be done on suspend.
>>>>>>>>>>
>>>>>>>>>> I'm also wondering if it's always safe to switch to PLLP in the
>>>>>>>>>> probe.
>>>>>>>>>> If CPU is running on a lower freq than PLLP, then some other
>>>>>>>>>> more
>>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>>
>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at
>>>>>>>>> higher
>>>>>>>>> rate
>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to dfll clock
>>>>>>>>> enable
>>>>>>>>> should be safe.
>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a divided
>>>>>>>> output of
>>>>>>>> PLLP
>>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>>
>>>>>>>> Probably, realistically, CPU is always running off a fast PLLX
>>>>>>>> during
>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>>>>>>>> CPUFreq driver should also have a 'shutdown' callback to teardown
>>>>>>>> DFLL
>>>>>>>> on a reboot, but likely that there are other clock-related
>>>>>>>> problems as
>>>>>>>> well that may break KEXEC and thus it is not very important at the
>>>>>>>> moment.
>>>>>>>>
>>>>>>>> [snip]
>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source above I
>>>>>>> meant
>>>>>>> PLL_P_OUT4.
>>>>>>>
>>>>>>> As per clock policies, PLL_X is always used for high freq like
>>>>>>> >800Mhz
>>>>>>> and for low frequency it will be sourced from PLLP.
>>>>>> Alright, then please don't forget to pre-initialize PLLP_OUT4
>>>>>> rate to a
>>>>>> reasonable value using tegra_clk_init_table or assigned-clocks.
>>>>> PLLP_OUT4 rate update is not needed as it is safe to run at 408Mhz
>>>>> because it is below fmax @ Vmin
>>>> So even 204MHz CVB entries are having the same voltage as 408MHz,
>>>> correct? It's not instantly obvious to me from the DFLL driver's code
>>>> where the fmax @ Vmin is defined, I see that there is the
>>>> min_millivolts
>>>> and frequency entries starting from 204MHZ defined per-table.
>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will work at
>>> Vmin voltage and PLLP max is 408Mhz.
>> Thank you for the clarification. It would be good to have that commented
>> in the code as well.
> OK, Will add...
Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend happens
very early even before disabling non-boot CPUs and also need to export
clock driver APIs to CPUFreq.
Was thinking of below way of implementing this...
Clock DFLL driver Suspend:
- Save CPU clock policy registers, and Perform dfll suspend
which sets in open loop mode
CPU Freq driver Suspend: does nothing
Clock DFLL driver Resume:
- Re-init DFLL, Set in Open-Loop mode, restore CPU Clock policy
registers which actually sets source to DFLL along with other
CPU Policy register restore.
CPU Freq driver Resume:
- do clk_prepare_enable which acutally sets DFLL in Closed loop
mode
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 2:35 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <c6c0a205-c083-fd46-361c-175bd8840c6e@nvidia.com>
On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:
>
> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
>>
>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
>>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>>>> The other thing that also need attention is that T124
>>>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed first,
>>>>>>>>>>>>>>>>>> which is
>>>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Should I add check for successful dfll clk register
>>>>>>>>>>>>>>>>> explicitly in
>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers?
>>>>>>>>>>>>>> Probably you should use the "device links". See [1][2]
>>>>>>>>>>>>>> for the
>>>>>>>>>>>>>> example.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add()
>>>>>>>>>>>>>> fails.
>>>>>>>>>>>>>> And
>>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's device,
>>>>>>>>>>>>>> see [3].
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [3]
>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Will go thru and add...
>>>>>>>>>>> Looks like I initially confused this case with getting orphaned
>>>>>>>>>>> clock.
>>>>>>>>>>> I'm now seeing that the DFLL driver registers the clock and
>>>>>>>>>>> then
>>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL
>>>>>>>>>>> driver is
>>>>>>>>>>> probed, hence everything should be fine as-is and there is
>>>>>>>>>>> no real
>>>>>>>>>>> need
>>>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>>>
>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the
>>>>>>>>>>>>>>>> DFLL
>>>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU clock
>>>>>>>>>>>>>>>> sources and
>>>>>>>>>>>>>>>> integrated with DVFS control logic with the regulator. We
>>>>>>>>>>>>>>>> will not
>>>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>>>> CPU to other clock sources once we switched to DFLL.
>>>>>>>>>>>>>>>> Because the
>>>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB
>>>>>>>>>>>>>>>> or OPP
>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to other sources
>>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow
>>>>>>>>>>>>>>>> switching to
>>>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL
>>>>>>>>>>>>>> freq to
>>>>>>>>>>>>>> PLLP's
>>>>>>>>>>>>>> rate before switching to PLLP in order to have a proper CPU
>>>>>>>>>>>>>> voltage.
>>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no need to
>>>>>>>>>>>>> enforce
>>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP
>>>>>>>>>>>>> during
>>>>>>>>>>>>> suspend
>>>>>>>>>>>>>
>>>>>>>>>>>> Sorry, please ignore my above comment. During suspend, need to
>>>>>>>>>>>> change
>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop mode
>>>>>>>>>>>> first and
>>>>>>>>>>>> then
>>>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>>>> Okay.
>>>>>>>>>>>
>>>>>>>>>>>>>>>> And I don't exactly understand why we need to switch to
>>>>>>>>>>>>>>>> PLLP in
>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the time.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it the
>>>>>>>>>>>>>>>> open-loop
>>>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest of the
>>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the
>>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And leave it on
>>>>>>>>>>>>>>>> PLL_P.
>>>>>>>>>>>>>>>> After
>>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore the
>>>>>>>>>>>>>>>> CPU clock
>>>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop
>>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent
>>>>>>>>>>>>>> during of
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence instead of
>>>>>>>>>>>>>> having
>>>>>>>>>>>>>> odd
>>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a proper
>>>>>>>>>>>>>> suspend-resume sequencing of the device drivers. In this
>>>>>>>>>>>>>> case
>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>> driver is the driver that enables DFLL and switches CPU
>>>>>>>>>>>>>> to that
>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>> source, which means that this driver is also should be
>>>>>>>>>>>>>> responsible for
>>>>>>>>>>>>>> management of the DFLL's state during of suspend/resume
>>>>>>>>>>>>>> process. If
>>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and
>>>>>>>>>>>>>> re-enables it
>>>>>>>>>>>>>> during
>>>>>>>>>>>>>> resume, then looks like the CaR driver hacks around DFLL
>>>>>>>>>>>>>> are not
>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the patch
>>>>>>>>>>>>>>>> subject to
>>>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>>>> suspend-resume support" seems more appropriate to me.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use are as follows
>>>>>>>>>>>>>>> (assuming
>>>>>>>>>>>>>>> all
>>>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is
>>>>>>>>>>>>>>> ok for
>>>>>>>>>>>>>>> any
>>>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>
>>>>>>>>>>>> I see during switch away from DFLL (suspend), cclk_g parent
>>>>>>>>>>>> is not
>>>>>>>>>>>> changed to PLLP before changing dfll to open loop mode.
>>>>>>>>>>>>
>>>>>>>>>>>> Will add this ...
>>>>>>>>>>> The CPUFreq driver switches parent to PLLP during the probe,
>>>>>>>>>>> similar
>>>>>>>>>>> should be done on suspend.
>>>>>>>>>>>
>>>>>>>>>>> I'm also wondering if it's always safe to switch to PLLP in the
>>>>>>>>>>> probe.
>>>>>>>>>>> If CPU is running on a lower freq than PLLP, then some other
>>>>>>>>>>> more
>>>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>>>
>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at
>>>>>>>>>> higher
>>>>>>>>>> rate
>>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to dfll clock
>>>>>>>>>> enable
>>>>>>>>>> should be safe.
>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a divided
>>>>>>>>> output of
>>>>>>>>> PLLP
>>>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>>>
>>>>>>>>> Probably, realistically, CPU is always running off a fast PLLX
>>>>>>>>> during
>>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>>>>>>>>> CPUFreq driver should also have a 'shutdown' callback to teardown
>>>>>>>>> DFLL
>>>>>>>>> on a reboot, but likely that there are other clock-related
>>>>>>>>> problems as
>>>>>>>>> well that may break KEXEC and thus it is not very important at
>>>>>>>>> the
>>>>>>>>> moment.
>>>>>>>>>
>>>>>>>>> [snip]
>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source above I
>>>>>>>> meant
>>>>>>>> PLL_P_OUT4.
>>>>>>>>
>>>>>>>> As per clock policies, PLL_X is always used for high freq like
>>>>>>>> >800Mhz
>>>>>>>> and for low frequency it will be sourced from PLLP.
>>>>>>> Alright, then please don't forget to pre-initialize PLLP_OUT4
>>>>>>> rate to a
>>>>>>> reasonable value using tegra_clk_init_table or assigned-clocks.
>>>>>> PLLP_OUT4 rate update is not needed as it is safe to run at 408Mhz
>>>>>> because it is below fmax @ Vmin
>>>>> So even 204MHz CVB entries are having the same voltage as 408MHz,
>>>>> correct? It's not instantly obvious to me from the DFLL driver's code
>>>>> where the fmax @ Vmin is defined, I see that there is the
>>>>> min_millivolts
>>>>> and frequency entries starting from 204MHZ defined per-table.
>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will work at
>>>> Vmin voltage and PLLP max is 408Mhz.
>>> Thank you for the clarification. It would be good to have that
>>> commented
>>> in the code as well.
>> OK, Will add...
>
> Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend happens
> very early even before disabling non-boot CPUs and also need to export
> clock driver APIs to CPUFreq.
>
> Was thinking of below way of implementing this...
>
>
> Clock DFLL driver Suspend:
>
> - Save CPU clock policy registers, and Perform dfll suspend
> which sets in open loop mode
>
> CPU Freq driver Suspend: does nothing
>
>
> Clock DFLL driver Resume:
>
> - Re-init DFLL, Set in Open-Loop mode, restore CPU Clock
> policy registers which actually sets source to DFLL along with other
> CPU Policy register restore.
>
> CPU Freq driver Resume:
>
> - do clk_prepare_enable which acutally sets DFLL in Closed
> loop mode
>
>
> Adding one more note: Switching CPU Clock to PLLP is not needed as CPU
> CLock can be from dfll in open-loop mode as DFLL is not disabled
> anywhere throught the suspend/resume path and SC7 entry FW and Warm
> boot code will switch CPU source to PLLP.
^ permalink raw reply
* Re: [PATCH 2/3 v3] ARM: dts: aspeed: Add SGPIO driver
From: Andrew Jeffery @ 2019-07-17 3:25 UTC (permalink / raw)
To: Hongwei Zhang, Bartosz Golaszewski, Joel Stanley, Linus Walleij
Cc: linux-gpio, linux-arm-kernel, linux-aspeed, linux-kernel
In-Reply-To: <1563312271-17509-1-git-send-email-hongweiz@ami.com>
Hello Hongwei,
Please send patches and feedback on prior iterations separately. Please send the
output of `git format-patch ...`directly; format-patch spits the patch out in email
form ready to go and can be fed straight to `git send-email`.
On Wed, 17 Jul 2019, at 06:54, Hongwei Zhang wrote:
> Add SGPIO driver support for Aspeed AST2500 SoC.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
> drivers/gpio/sgpio-aspeed.c | 487 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 487 insertions(+)
> create mode 100644 drivers/gpio/sgpio-aspeed.c
>
> diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
> new file mode 100644
> index 0000000..ade2cb7
> --- /dev/null
> +++ b/drivers/gpio/sgpio-aspeed.c
> @@ -0,0 +1,487 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2019 American Megatrends International LLC.
> + *
> + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in>
> + */
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/aspeed.h>
linux/gpio/aspeed.h is specific to the parallel GPIO driver, please drop
this include.
> +#include <linux/hashtable.h>
> +#include <linux/bitfield.h>
> +#include <linux/init.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pinctrl/consumer.h>
This driver doesn't have any direct interaction with pinctrl, so I think
we can remove this header
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +#include <linux/gpio.h>
> +
> +#define MAX_NR_SGPIO 80
> +
> +#define ASPEED_SGPIO_CTRL 0x54
> +
> +#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
> +#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
> +#define ASPEED_SGPIO_ENABLE BIT(0)
> +
> +// default sgpio direction is input.
> +static uint32_t sgpio_dir_val[3] = {0xffffffff, 0xffffffff, 0xffffffff
> };
Why not make it a member of struct aspeed_sgpio (below)? I'd prefer
we encode the comment in the variable name as well, e.g.
sgpio_dir_in`- this way when reading the code that uses it we know
which bit state means what (set is input, clear is output).
> +
> +struct aspeed_sgpio {
> + struct gpio_chip chip;
> + struct clk *pclk;
> + spinlock_t lock;
> + void __iomem *base;
> + int irq;
> +};
> +
> +struct aspeed_sgpio_bank {
> + uint16_t val_regs;
> + uint16_t rdata_reg;
> + uint16_t irq_regs;
> + const char names[4][3];
> +};
> +
> +/*
> + * Note: The "value" register returns the input value sampled on the
> + * line even when the GPIO is configured as an output. Since
> + * that input goes through synchronizers, writing, then reading
> + * back may not return the written value right away.
The paragraph above is somewhat specific to the parallel GPIO driver.
It would be good to rework it for the context of the SGPIO driver.
Documenting the split of the "value" and "rdata" register is a good
thing.
> + *
> + * The "rdata" register returns the content of the write latch
> + * and thus can be used to read back what was last written
> + * reliably.
> + */
> +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> + {
> + .val_regs = 0x0000,
> + .rdata_reg = 0x0070,
> + .irq_regs = 0x0004,
> + .names = { "A", "B", "C", "D" },
> + },
> + {
> + .val_regs = 0x001C,
> + .rdata_reg = 0x0074,
> + .irq_regs = 0x0020,
> + .names = { "E", "F", "G", "H" },
> + },
> + {
> + .val_regs = 0x0038,
> + .rdata_reg = 0x0078,
> + .irq_regs = 0x003C,
> + .names = { "I", "J" },
> + },
> +};
> +
> +enum aspeed_sgpio_reg {
> + reg_val,
> + reg_rdata,
> + reg_irq_enable,
> + reg_irq_type0,
> + reg_irq_type1,
> + reg_irq_type2,
> + reg_irq_status,
> +};
> +
> +#define GPIO_VAL_VALUE 0x00
> +#define GPIO_VAL_DIR 0x04
> +#define GPIO_IRQ_ENABLE 0x00
> +#define GPIO_IRQ_TYPE0 0x04
> +#define GPIO_IRQ_TYPE1 0x08
> +#define GPIO_IRQ_TYPE2 0x0C
> +#define GPIO_IRQ_STATUS 0x10
> +
> +/* This will be resolved at compile time */
> +static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
> + const struct aspeed_sgpio_bank *bank,
> + const enum aspeed_sgpio_reg reg)
> +{
> + switch (reg) {
> + case reg_val:
> + return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> + case reg_rdata:
> + return gpio->base + bank->rdata_reg;
> + case reg_irq_enable:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
> + case reg_irq_type0:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
> + case reg_irq_type1:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
> + case reg_irq_type2:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
> + case reg_irq_status:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
> + default:
> + /* acturally if code runs to here, it's an error case */
> + BUG_ON(1);
> + }
> +}
> +
> +#define GPIO_BANK(x) ((x) >> 5)
> +#define GPIO_OFFSET(x) ((x) & 0x1f)
> +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
> +
> +static const struct aspeed_sgpio_bank *to_bank(unsigned int offset)
> +{
> + unsigned int bank = GPIO_BANK(offset);
> +
> + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
> + return &aspeed_sgpio_banks[bank];
> +}
> +
> +static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> + const struct aspeed_sgpio_bank *bank = to_bank(offset);
> +
> + if (sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset))
> + return !!(ioread32(bank_reg(gpio, bank, reg_val)) &
> GPIO_BIT(offset));
> + else
> + return !!(ioread32(bank_reg(gpio, bank, reg_rdata)) &
> GPIO_BIT(offset));
We don't need the else because we return from the body of the true case,
and this could be written in a less redundant fashion. Also we need to do
the read under gpio.lock for consistency with aspeed_sgpio_set().
enum aspeed_sgpio_reg from;
unsigned long flags;
bool input;
int rc;
...
spin_lock_irqsave(&gpio->lock, flags);
input = sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset);
from = input ? reg_val : reg_rdata;
rc = !!(ioread32(bank_reg(gpio, bank, from)) & GPIO_BIT(offset));
spin_unlock_irqrestore(&gpio->lock, flags);
return rc;
> +
> +}
> +
> +static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int
> offset, int val)
> +{
> + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> + const struct aspeed_sgpio_bank *bank = to_bank(offset);
> + unsigned long flags;
> + void __iomem *addr;
> + u32 reg = 0;
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + addr = bank_reg(gpio, bank, reg_val);
> +
> + if (val)
> + reg |= GPIO_BIT(offset);
> + else
> + reg &= ~GPIO_BIT(offset);
> +
> + iowrite32(reg, addr);
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int
> offset)
> +{
> + sgpio_dir_val[GPIO_BANK(offset)] |= GPIO_BIT(offset);
Also do all manipulations of sgpio_dir_val under the spinlock.
> + return 0;
> +}
> +
> +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int
> offset, int val)
> +{
> + sgpio_dir_val[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
Again here.
> + return 0;
> +}
> +
> +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned
> int offset)
> +{
> + return sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset);
Again here.
> +
> +}
> +
> +static inline int irqd_to_aspeed_sgpio_data(struct irq_data *d,
> + struct aspeed_sgpio **gpio,
> + const struct aspeed_sgpio_bank **bank,
> + u32 *bit, int *offset)
> +{
> + struct aspeed_sgpio *internal;
> +
> + *offset = irqd_to_hwirq(d);
> +
> + internal = irq_data_get_irq_chip_data(d);
> +
> + *gpio = internal;
> + *bank = to_bank(*offset);
> + *bit = GPIO_BIT(*offset);
> +
> + return 0;
It looks like this function could be a void function instead, and we
could eliminate error checking from the callsites. If you're feeling
paranoid you could `WARN_ON(!internal);` after the call to
`irq_data_get_irq_chip_data(d)`.
> +}
> +
> +static void aspeed_sgpio_irq_ack(struct irq_data *d)
> +{
> + const struct aspeed_sgpio_bank *bank;
> + struct aspeed_sgpio *gpio;
> + unsigned long flags;
> + void __iomem *status_addr;
> + int rc, offset;
> + u32 bit;
> +
> + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> + if (rc)
> + return;
> +
> + status_addr = bank_reg(gpio, bank, reg_irq_status);
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + iowrite32(bit, status_addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
> +{
> + const struct aspeed_sgpio_bank *bank;
> + struct aspeed_sgpio *gpio;
> + unsigned long flags;
> + u32 reg, bit;
> + void __iomem *addr;
> + int rc, offset;
> +
> + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> + if (rc)
> + return;
> +
> + addr = bank_reg(gpio, bank, reg_irq_enable);
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + reg = ioread32(addr);
> + if (set)
> + reg |= bit;
> + else
> + reg &= ~bit;
> +
> + iowrite32(reg, addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +static void aspeed_sgpio_irq_mask(struct irq_data *d)
> +{
> + aspeed_sgpio_irq_set_mask(d, false);
> +}
> +
> +static void aspeed_sgpio_irq_unmask(struct irq_data *d)
> +{
> + aspeed_sgpio_irq_set_mask(d, true);
> +}
> +
> +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
> +{
> + u32 type0 = 0;
> + u32 type1 = 0;
> + u32 type2 = 0;
> + u32 bit, reg;
> + const struct aspeed_sgpio_bank *bank;
> + irq_flow_handler_t handler;
> + struct aspeed_sgpio *gpio;
> + unsigned long flags;
> + void __iomem *addr;
> + int rc, offset;
> +
> + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> + if (rc)
> + return -EINVAL;
> +
> + switch (type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_EDGE_BOTH:
> + type2 |= bit;
> + /* fall through */
> + case IRQ_TYPE_EDGE_RISING:
> + type0 |= bit;
> + /* fall through */
> + case IRQ_TYPE_EDGE_FALLING:
> + handler = handle_edge_irq;
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + type0 |= bit;
> + /* fall through */
> + case IRQ_TYPE_LEVEL_LOW:
> + type1 |= bit;
> + handler = handle_level_irq;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + addr = bank_reg(gpio, bank, reg_irq_type0);
> + reg = ioread32(addr);
> + reg = (reg & ~bit) | type0;
> + iowrite32(reg, addr);
> +
> + addr = bank_reg(gpio, bank, reg_irq_type1);
> + reg = ioread32(addr);
> + reg = (reg & ~bit) | type1;
> + iowrite32(reg, addr);
> +
> + addr = bank_reg(gpio, bank, reg_irq_type2);
> + reg = ioread32(addr);
> + reg = (reg & ~bit) | type2;
> + iowrite32(reg, addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +
> + irq_set_handler_locked(d, handler);
> +
> + return 0;
> +}
> +
> +static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + struct irq_chip *ic = irq_desc_get_chip(desc);
> + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> + unsigned int i, p, girq;
> + unsigned long reg;
> +
> + chained_irq_enter(ic, desc);
> +
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> +
> + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> +
> + for_each_set_bit(p, ®, 32) {
> + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> + generic_handle_irq(girq);
> + }
> +
> + }
> +
> + chained_irq_exit(ic, desc);
> +}
> +
> +static struct irq_chip aspeed_sgpio_irqchip = {
> + .name = "aspeed-sgpio",
> + .irq_ack = aspeed_sgpio_irq_ack,
> + .irq_mask = aspeed_sgpio_irq_mask,
> + .irq_unmask = aspeed_sgpio_irq_unmask,
> + .irq_set_type = aspeed_sgpio_set_type,
> +};
> +
> +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
> + struct platform_device *pdev)
> +{
> + int rc, i;
> + const struct aspeed_sgpio_bank *bank;
> +
> + rc = platform_get_irq(pdev, 0);
> + if (rc < 0)
> + return rc;
> +
> + gpio->irq = rc;
> +
> + /* Disable IRQ and clear Interrupt status registers for all SPGIO
> Pins. */
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> + bank = &aspeed_sgpio_banks[i];
> + /* disable irq enable bits */
> + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
> + /* clear status bits */
> + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
> + }
> +
> + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
> + 0, handle_bad_irq, IRQ_TYPE_NONE);
> + if (rc) {
> + dev_info(&pdev->dev, "Could not add irqchip\n");
> + return rc;
> + }
> +
> + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
> + gpio->irq, aspeed_sgpio_irq_handler);
> +
> + /* set IRQ settings and Enable Interrupt */
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> + bank = &aspeed_sgpio_banks[i];
> + /* set falling or level-low irq */
> + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
> + /* trigger type is edge */
> + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
> + /* dual edge trigger mode. */
> + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
> + /* enable irq */
> + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id aspeed_sgpio_of_table[] = {
> + { .compatible = "aspeed,ast2400-sgpio" },
> + { .compatible = "aspeed,ast2500-sgpio" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
> +
> +static int __init aspeed_sgpio_probe(struct platform_device *pdev)
> +{
> + struct aspeed_sgpio *gpio;
> + struct resource *res;
> + u32 nr_gpios, sgpio_freq;
> + int rc;
> + u16 sgpio_clk_div;
Lets make this a u32 as it will help error detection below.
> + unsigned long apb_freq;
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + gpio->base = devm_ioremap_resource(&pdev->dev, res);
Please use devm_platform_ioremap_resource() here.
> + if (IS_ERR(gpio->base))
> + return PTR_ERR(gpio->base);
> +
> + rc = of_property_read_u32(pdev->dev.of_node, "nr-gpios", &nr_gpios);
> + if ((rc < 0) || (nr_gpios > MAX_NR_SGPIO))
> + nr_gpios = MAX_NR_SGPIO;
This is an error state, not something we should paper over. This should be
`return -EINVAL;`
> +
> + rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency",
> &sgpio_freq);
> + if (rc < 0) {
> + dev_err(&pdev->dev, "Could not read bus-frequency property\n");
> + sgpio_freq = 12000000;
Again, I suggested previously that this is a required property, not optional.
As such there should not be fall-back code here. This is another case of
`return -EINVAL;`.
> + }
> +
> + gpio->pclk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(gpio->pclk)) {
> + dev_err(&pdev->dev, "devm_clk_get failed\n");
> + return PTR_ERR(gpio->pclk);
> + }
> +
> + apb_freq = clk_get_rate(gpio->pclk);
> + sgpio_clk_div = 2 * ((apb_freq % sgpio_freq == 0) ?
> + (apb_freq / sgpio_freq) - 1 : (apb_freq / sgpio_freq));
This calculation seems overly complex and possibly incorrect (need to
multiply the denominator or divide the result, not multiply the result)?
From the datasheet, the SGPM clock period calculation is:
period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
rearranging:
period = 2 * (GPIO254[31:16] + 1) / PCLK
Converting back to bus frequency:
frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
Which rearranges to:
frequency = PCLK / (2 * (GPIO254[31:16] + 1))
Extracting GPIO254[31:16] in terms of PCLK / frequency from above:
frequency * 2 * (GPIO254[31:16] + 1) = PCLK
And so:
GPIO254[31:16] = PCLK / (frequency * 2) - 1
From that, the code should look something like:
if (sgpio_freq == 0)
return -EINVAL;
sgpio_clk_div = apb_freq / (sgpio_freq * 2) - 1;
if (sgpio_clk_div > (1 << 16) - 1)
return -EINVAL;
This seems to work at the extremes (sgpio_clk_div = 0 and
sgpio_clk_div = 65535), and we get 32766.99 on a round-trip of
the divider value 32768, which if we truncate gives an error of 0.023Hz
with an APB of 24.75MHz (value reported from one of our boards).
Andrew
> + iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
> + FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
> + ASPEED_SGPIO_ENABLE,
> + gpio->base + ASPEED_SGPIO_CTRL);
> +
> + spin_lock_init(&gpio->lock);
> +
> + gpio->chip.parent = &pdev->dev;
> + gpio->chip.ngpio = nr_gpios;
> + gpio->chip.direction_input = aspeed_sgpio_dir_in;
> + gpio->chip.direction_output = aspeed_sgpio_dir_out;
> + gpio->chip.get_direction = aspeed_sgpio_get_direction;
> + gpio->chip.request = NULL;
> + gpio->chip.free = NULL;
> + gpio->chip.get = aspeed_sgpio_get;
> + gpio->chip.set = aspeed_sgpio_set;
> + gpio->chip.set_config = NULL;
> + gpio->chip.label = dev_name(&pdev->dev);
> + gpio->chip.base = ARCH_NR_GPIOS - MAX_NR_SGPIO;
> +
> + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> + if (rc < 0)
> + return rc;
> +
> + return aspeed_sgpio_setup_irqs(gpio, pdev);
> +}
> +
> +static struct platform_driver aspeed_sgpio_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = aspeed_sgpio_of_table,
> + },
> +};
> +
> +module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
> +MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
> >
>
> thanks Anrew, please review v3 at above and also inline comments at below.
>
> >
> > From: Andrew Jeffery <andrew@aj.id.au>
> > Sent: Wednesday, July 10, 2019 9:46 PM
> > To: Hongwei Zhang; Bartosz Golaszewski; Joel Stanley; Linus Walleij
> > Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/3 v2] ARM: dts: aspeed: Add SGPIO driver
> >
> >
> >
> > On Thu, 11 Jul 2019, at 00:56, Hongwei Zhang wrote:
> > > Add SGPIO driver support for Aspeed AST2500 SoC.
> > >
> > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > > ---
> > > drivers/gpio/sgpio-aspeed.c | 450
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 450 insertions(+)
> > > create mode 100644 drivers/gpio/sgpio-aspeed.c
> > >
> > > diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
> > > new file mode 100644 index 0000000..0743d22
> > > --- /dev/null
> > > +++ b/drivers/gpio/sgpio-aspeed.c
> > > @@ -0,0 +1,450 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2019 American Megatrends International LLC.
> > > + *
> > > + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in> */
> > > +
> > > +#include <linux/gpio/driver.h>
> > > +#include <linux/gpio/aspeed.h>
> > > +#include <linux/hashtable.h>
> > > +#include <linux/init.h>
> > > +#include <linux/io.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pinctrl/consumer.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/string.h>
> > > +
> > > +#define NR_SGPIO 80
> > > +
> > > +struct aspeed_sgpio {
> > > + struct gpio_chip chip;
> > > + spinlock_t lock;
> > > + void __iomem *base;
> > > + int irq;
> > > +};
> > > +
> > > +struct aspeed_sgpio_bank {
> > > + uint16_t val_regs;
> > > + uint16_t rdata_reg;
> > > + uint16_t irq_regs;
> > > + const char names[4][3];
> > > +};
> > > +
> > > +/*
> > > + * Note: The "value" register returns the input value sampled on the
> > > + * line even when the GPIO is configured as an output. Since
> > > + * that input goes through synchronizers, writing, then reading
> > > + * back may not return the written value right away.
> > > + *
> > > + * The "rdata" register returns the content of the write latch
> > > + * and thus can be used to read back what was last written
> > > + * reliably.
> > > + */
> > > +
> > > +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> > > + {
> > > + .val_regs = 0x0000,
> > > + .rdata_reg = 0x0070,
> > > + .irq_regs = 0x0004,
> > > + .names = { "A", "B", "C", "D" },
> > > + },
> > > + {
> > > + .val_regs = 0x001C,
> > > + .rdata_reg = 0x0074,
> > > + .irq_regs = 0x0020,
> > > + .names = { "E", "F", "G", "H" },
> > > + },
> > > + {
> > > + .val_regs = 0x0038,
> > > + .rdata_reg = 0x0078,
> > > + .irq_regs = 0x003C,
> > > + .names = { "I", "J" },
> > > + },
> > > +};
> > > +
> > > +enum aspeed_sgpio_reg {
> > > + reg_val,
> > > + reg_rdata,
> > > + reg_irq_enable,
> > > + reg_irq_type0,
> > > + reg_irq_type1,
> > > + reg_irq_type2,
> > > + reg_irq_status,
> > > +};
> > > +
> > > +#define GPIO_VAL_VALUE 0x00
> > > +#define GPIO_VAL_DIR 0x04
> > > +#define GPIO_IRQ_ENABLE 0x00
> > > +#define GPIO_IRQ_TYPE0 0x04
> > > +#define GPIO_IRQ_TYPE1 0x08
> > > +#define GPIO_IRQ_TYPE2 0x0C
> > > +#define GPIO_IRQ_STATUS 0x10
> > > +
> > > +/* This will be resolved at compile time */ static inline void
> > > +__iomem *bank_reg(struct aspeed_sgpio *gpio,
> > > + const struct aspeed_sgpio_bank *bank,
> > > + const enum aspeed_sgpio_reg reg) {
> > > + switch (reg) {
> > > + case reg_val:
> > > + return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> > > + case reg_rdata:
> > > + return gpio->base + bank->rdata_reg;
> > > + case reg_irq_enable:
> > > + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
> > > + case reg_irq_type0:
> > > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
> > > + case reg_irq_type1:
> > > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
> > > + case reg_irq_type2:
> > > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
> > > + case reg_irq_status:
> > > + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
> > > + default:
> > > + /* acturally if code runs to here, it's an error case */
> > > + WARN_ON(reg);
> > > + return gpio->base;
> > > + }
> > > +}
> > > +
>
> updated to use BUG_ON(1), please see v3.
>
> > > +#define GPIO_BANK(x) ((x) >> 5)
> > > +#define GPIO_OFFSET(x) ((x) & 0x1f)
> > > +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
> > > +
> > > +static const struct aspeed_sgpio_bank *to_bank(unsigned int offset) {
> > > + unsigned int bank = GPIO_BANK(offset);
> > > +
> > > + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
> > > + return &aspeed_sgpio_banks[bank];
> > > +}
> > > +
> > > +static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int
> > > +offset) {
> > > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > > + const struct aspeed_sgpio_bank *bank = to_bank(offset);
> > > +
> > > + return !!(ioread32(bank_reg(gpio, bank, reg_val)) &
> > > +GPIO_BIT(offset)); }
> > > +
> > > +static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int
> > > offset, int val)
> > > +{
> > > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > > + const struct aspeed_sgpio_bank *bank = to_bank(offset);
> > > + unsigned long flags;
> > > + void __iomem *addr;
> > > + u32 reg = 0;
> > > +
> > > + spin_lock_irqsave(&gpio->lock, flags);
> > > +
> > > + addr = bank_reg(gpio, bank, reg_val);
> > > +
> > > + if (val)
> > > + reg |= GPIO_BIT(offset);
> > > + else
> > > + reg &= ~GPIO_BIT(offset);
> > > +
> > > + iowrite32(reg, addr);
> > > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > > +
> > > +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int
> > > offset)
> > > +{
> > > + /* By default all SGPIO Pins are input */
> > > + return 0;
> > > +}
> > > +
> > > +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int
> > > offset, int val)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned
> > > int offset)
> > > +{
> > > + /* By default all SGPIO Pins are input */
> > > + return 1;
> > > +
> > > +}
> >
> > Please see my follow-up reply on v1 that helps clarify what we should do with the
> > dir_in()/dir_out()/get_direction() implementations. The implementation here will confuse everything in
> > the stack above it.
> >
>
> updated, please see v3.
>
> > > +
> > > +static inline int irqd_to_aspeed_sgpio_data(struct irq_data *d,
> > > + struct aspeed_sgpio **gpio,
> > > + const struct aspeed_sgpio_bank **bank,
> > > + u32 *bit, int *offset)
> > > +{
> > > + struct aspeed_sgpio *internal;
> > > +
> > > + *offset = irqd_to_hwirq(d);
> > > +
> > > + internal = irq_data_get_irq_chip_data(d);
> > > +
> > > + *gpio = internal;
> > > + *bank = to_bank(*offset);
> > > + *bit = GPIO_BIT(*offset);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void aspeed_sgpio_irq_ack(struct irq_data *d) {
> > > + const struct aspeed_sgpio_bank *bank;
> > > + struct aspeed_sgpio *gpio;
> > > + unsigned long flags;
> > > + void __iomem *status_addr;
> > > + int rc, offset;
> > > + u32 bit;
> > > +
> > > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > > + if (rc)
> > > + return;
> > > +
> > > + status_addr = bank_reg(gpio, bank, reg_irq_status);
> > > +
> > > + spin_lock_irqsave(&gpio->lock, flags);
> > > +
> > > + iowrite32(bit, status_addr);
> > > +
> > > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > > +
> > > +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set) {
> > > + const struct aspeed_sgpio_bank *bank;
> > > + struct aspeed_sgpio *gpio;
> > > + unsigned long flags;
> > > + u32 reg, bit;
> > > + void __iomem *addr;
> > > + int rc, offset;
> > > +
> > > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > > + if (rc)
> > > + return;
> > > +
> > > + addr = bank_reg(gpio, bank, reg_irq_enable);
> > > +
> > > + spin_lock_irqsave(&gpio->lock, flags);
> > > +
> > > + reg = ioread32(addr);
> > > + if (set)
> > > + reg |= bit;
> > > + else
> > > + reg &= ~bit;
> > > +
> > > + iowrite32(reg, addr);
> > > +
> > > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > > +
> > > +static void aspeed_sgpio_irq_mask(struct irq_data *d) {
> > > + aspeed_sgpio_irq_set_mask(d, false); }
> > > +
> > > +static void aspeed_sgpio_irq_unmask(struct irq_data *d) {
> > > + aspeed_sgpio_irq_set_mask(d, true);
> > > +}
> > > +
> > > +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int
> > > +type) {
> > > + u32 type0 = 0;
> > > + u32 type1 = 0;
> > > + u32 type2 = 0;
> > > + u32 bit, reg;
> > > + const struct aspeed_sgpio_bank *bank;
> > > + irq_flow_handler_t handler;
> > > + struct aspeed_sgpio *gpio;
> > > + unsigned long flags;
> > > + void __iomem *addr;
> > > + int rc, offset;
> > > +
> > > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > > + if (rc)
> > > + return -EINVAL;
> > > +
> > > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > > + case IRQ_TYPE_EDGE_BOTH:
> > > + type2 |= bit;
> > > + /* fall through */
> > > + case IRQ_TYPE_EDGE_RISING:
> > > + type0 |= bit;
> > > + /* fall through */
> > > + case IRQ_TYPE_EDGE_FALLING:
> > > + handler = handle_edge_irq;
> > > + break;
> > > + case IRQ_TYPE_LEVEL_HIGH:
> > > + type0 |= bit;
> > > + /* fall through */
> > > + case IRQ_TYPE_LEVEL_LOW:
> > > + type1 |= bit;
> > > + handler = handle_level_irq;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + spin_lock_irqsave(&gpio->lock, flags);
> > > +
> > > + addr = bank_reg(gpio, bank, reg_irq_type0);
> > > + reg = ioread32(addr);
> > > + reg = (reg & ~bit) | type0;
> > > + iowrite32(reg, addr);
> > > +
> > > + addr = bank_reg(gpio, bank, reg_irq_type1);
> > > + reg = ioread32(addr);
> > > + reg = (reg & ~bit) | type1;
> > > + iowrite32(reg, addr);
> > > +
> > > + addr = bank_reg(gpio, bank, reg_irq_type2);
> > > + reg = ioread32(addr);
> > > + reg = (reg & ~bit) | type2;
> > > + iowrite32(reg, addr);
> > > +
> > > + spin_unlock_irqrestore(&gpio->lock, flags);
> > > +
> > > + irq_set_handler_locked(d, handler);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void aspeed_sgpio_irq_handler(struct irq_desc *desc) {
> > > + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> > > + struct irq_chip *ic = irq_desc_get_chip(desc);
> > > + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> > > + unsigned int i, p, girq;
> > > + unsigned long reg;
> > > +
> > > + chained_irq_enter(ic, desc);
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > > + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> > > +
> > > + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> > > +
> > > + for_each_set_bit(p, ®, 32) {
> > > + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> > > + generic_handle_irq(girq);
> > > + }
> > > +
> > > + }
> > > +
> > > + chained_irq_exit(ic, desc);
> > > +}
> > > +
> > > +static struct irq_chip aspeed_sgpio_irqchip = {
> > > + .name = "aspeed-sgpio",
> > > + .irq_ack = aspeed_sgpio_irq_ack,
> > > + .irq_mask = aspeed_sgpio_irq_mask,
> > > + .irq_unmask = aspeed_sgpio_irq_unmask,
> > > + .irq_set_type = aspeed_sgpio_set_type,
> > > +};
> > > +
> > > +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
> > > + struct platform_device *pdev)
> > > +{
> > > + int rc, i;
> > > + const struct aspeed_sgpio_bank *bank;
> > > +
> > > + rc = platform_get_irq(pdev, 0);
> > > + if (rc < 0)
> > > + return rc;
> > > +
> > > + gpio->irq = rc;
> > > +
> > > + /* Disable IRQ and clear Interrupt status registers for all SPGIO
> > > Pins. */
> > > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > > + bank = &aspeed_sgpio_banks[i];
> > > + /* disable irq enable bits */
> > > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
> > > + /* clear status bits */
> > > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
> > > + }
> > > +
> > > + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
> > > + 0, handle_bad_irq, IRQ_TYPE_NONE);
> > > + if (rc) {
> > > + dev_info(&pdev->dev, "Could not add irqchip\n");
> > > + return rc;
> > > + }
> > > +
> > > + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
> > > + gpio->irq, aspeed_sgpio_irq_handler);
> > > +
> > > + /* set IRQ settings and Enable Interrupt */
> > > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > > + bank = &aspeed_sgpio_banks[i];
> > > + /* set falling or level-low irq */
> > > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
> > > + /* trigger type is edge */
> > > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
> > > + /* dual edge trigger mode. */
> > > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
> > > + /* enable irq */
> > > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int aspeed_sgpio_request(struct gpio_chip *chip, unsigned int
> > > offset)
> > > +{
> > > + return (offset < NR_SGPIO);
> > > +}
> >
> > I don't think this request() implementation is helpful, especially as it stands in the face of needing to pull
> > the number of GPIOs to serialise from the devicetree.
> >
> > request() is an optional callback, lets just drop it.
> >
>
> updated, please see v3.
>
> > > +
> > > +static const struct of_device_id aspeed_sgpio_of_table[] = {
> > > + { .compatible = "aspeed,ast2400-sgpio" },
> > > + { .compatible = "aspeed,ast2500-sgpio" },
> > > + {}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
> > > +
> > > +static int __init aspeed_sgpio_probe(struct platform_device *pdev) {
> > > + struct aspeed_sgpio *gpio;
> > > + struct resource *res;
> > > + int rc;
> > > +
> > > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> > > + if (!gpio)
> > > + return -ENOMEM;
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + gpio->base = devm_ioremap_resource(&pdev->dev, res);
> > > + if (IS_ERR(gpio->base))
> > > + return PTR_ERR(gpio->base);
> > > +
> > > + spin_lock_init(&gpio->lock);
> > > +
> > > + gpio->chip.parent = &pdev->dev;
> > > + gpio->chip.ngpio = NR_SGPIO;
> > > + gpio->chip.direction_input = aspeed_sgpio_dir_in;
> > > + gpio->chip.direction_output = aspeed_sgpio_dir_out;
> > > + gpio->chip.get_direction = aspeed_sgpio_get_direction;
> > > + gpio->chip.request = aspeed_sgpio_request;
> > > + gpio->chip.free = NULL;
> > > + gpio->chip.get = aspeed_sgpio_get;
> > > + gpio->chip.set = aspeed_sgpio_set;
> > > + gpio->chip.set_config = NULL;
> > > + gpio->chip.label = dev_name(&pdev->dev);
> > > + gpio->chip.base = -1;
> > > +
> > > + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> > > + if (rc < 0)
> > > + return rc;
> > > +
> > > + return aspeed_sgpio_setup_irqs(gpio, pdev); }
> > > +
> > > +static struct platform_driver aspeed_sgpio_driver = {
> > > + .driver = {
> > > + .name = KBUILD_MODNAME,
> > > + .of_match_table = aspeed_sgpio_of_table,
> > > + },
> > > +};
> > > +
> > > +module_platform_driver_probe(aspeed_sgpio_driver,
> > > +aspeed_sgpio_probe); MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 2.7.4
> > >
> > >
>
^ permalink raw reply
* Re: [PATCH 2/3 v3] dt-bindings: gpio: aspeed: Add SGPIO support
From: Andrew Jeffery @ 2019-07-17 3:49 UTC (permalink / raw)
To: Hongwei Zhang, Joel Stanley, Linus Walleij, devicetree
Cc: Rob Herring, Mark Rutland, Bartosz Golaszewski, linux-aspeed,
linux-kernel, linux-arm-kernel, linux-gpio
In-Reply-To: <1563313711-17961-1-git-send-email-hongweiz@ami.com>
Hello Hongwei,
On Wed, 17 Jul 2019, at 07:18, Hongwei Zhang wrote:
> Add bindings to support SGPIO on AST2400 or AST2500.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
> .../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> new file mode 100644
> index 0000000..8c3a747
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> @@ -0,0 +1,55 @@
> +Aspeed SGPIO controller Device Tree Bindings
> +-------------------------------------------
> +
> +This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80
> full
> +featured Serial GPIOs. Each of the Serial GPIO pins can be programmed
> to
> +support the following options:
> +- Support interrupt option for each input port and various interrupt
> + sensitivity option (level-high, level-low, edge-high, edge-low)
> +- Support reset tolerance option for each output port
> +- Directly connected to APB bus and its shift clock is from APB bus
> clock
> + divided by a programmable value.
> +- Co-work with external signal-chained TTL components (74LV165/74LV595)
Nice description.
> +
> +
> +Required properties:
> +
> +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> +
> +- #gpio-cells : Should be two
> + - First cell is the GPIO line number
> + - Second cell is used to specify optional
> + parameters (unused)
> +
> +- reg : Address and length of the register set for the device
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- interrupts : Interrupt specifier (see interrupt bindings for
> + details)
> +
> +- interrupt-controller : Mark the GPIO controller as an
> interrupt-controller
> +
> +- nr-gpios : number of GPIO pins to serialise.
> + (should be multiple of 8, up to 80 pins; 0 if not used)
It's unclear to me what you mean by "0 if not used" here. The property is
required, so its description in a devicetree should always have a non-zero
value of `status = "okay";`, as 0 is an invalid value according to the
datasheet (sensibly so). If `status = "disabled";` then it doesn't really
matter, which makes the comment not terribly useful.
> +
> +- clocks : A phandle to the APB clock for SGPM clock
> division
> +
> +- bus-frequency : SGPM CLK frequency, derived from APB bus clock by a
> programmable devisor
I'd leave off the parent clock information. Practically speaking it's probably
always going to be the APB clock, but who knows. From a devicetree writer's
perspective they just want to say "make it 7MHz" or whatever speed they,
and it shouldn't matter too much how we get there.
Finally, as mentioned on the driver patch, please send v4 without the history
at the bottom.
Cheers,
Andrew
> +
> +
> +The sgpio and interrupt properties are further described in their
> respective bindings documentation:
> +
> +- Documentation/devicetree/bindings/sgpio/gpio.txt
> +- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +
> + Example:
> + sgpio@1e780200 {
> + #gpio-cells = <2>;
> + compatible = "aspeed,ast2500-sgpio";
> + gpio-controller;
> + interrupts = <40>;
> + reg = <0x1e780200 0x0100>;
> + clocks = <&syscon ASPEED_CLK_APB>;
> + interrupt-controller;
> + nr-gpios = <8>;
> + bus-frequency = <12000000>;
> + };
> --
> 2.7.4
>
>
> Thanks Andrew, please see above v3 and inline comments at below.
> --Hongwei
>
> > From: Andrew Jeffery <andrew@aj.id.au>
> > Sent: Sunday, July 14, 2019 10:25 PM
> > To: Hongwei Zhang; Joel Stanley; Linus Walleij; devicetree@vger.kernel.org
> > Cc: Rob Herring; Mark Rutland; Bartosz Golaszewski; linux-aspeed@lists.ozlabs.org; linux-
> > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-gpio@vger.kernel.org
> > Subject: Re: [PATCH 2/3 v2] dt-bindings: gpio: aspeed: Add SGPIO support
> >
> > Hello Hongwei,
> >
> > On Sat, 13 Jul 2019, at 05:44, Hongwei Zhang wrote:
> > > Add bindings to support SGPIO on AST2400 or AST2500.
> > >
> > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > > ---
> > > .../devicetree/bindings/gpio/sgpio-aspeed.txt | 43 ++++++++++++++++++++++
> > > 1 file changed, 43 insertions(+)
> > > create mode 100755
> > > Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > new file mode 100755
> > > index 0000000..3ae2b79
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > @@ -0,0 +1,43 @@
> > > +Aspeed SGPIO controller Device Tree Bindings
> > > +-------------------------------------------
> > > +
> > > +Required properties:
> > > +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> > > +
> > > +- #gpio-cells : Should be two
> > > + - First cell is the GPIO line number
> > > + - Second cell is used to specify optional
> > > + parameters (unused)
> > > +
> > > +- reg : Address and length of the register set for the device
> > > +- gpio-controller : Marks the device node as a GPIO controller.
> > > +- interrupts : Interrupt specifier (see interrupt bindings for
> > > + details)
> > > +
> > > +- interrupt-controller : Mark the GPIO controller as an
> > > interrupt-controller
> > > +
> > > +- nr-gpios : number of GPIO pins to serialise. (should be multiple of
> > > 8, up to 80 pins)
> > > + if not specified, defaults to 80.
> >
> > This appears to be a statement about the driver implementation, but bindings documents are about
> > describing hardware. Reading the datasheet it actually appears the ASPEED SGPIO hardware comes up
> > in what is "technically" a forbidden state (equivalent to `nr-gpios = <0>;`), though the device is also
> > disabled at this point, so it's probably moot. The point is the true default value from a hardware
> > perspective is 0, not 80, so if we're going to talk about default values, 0 would be more appropriate.
> > However:
> >
> > You've also listed nr-gpios under the "Required properties" header, but the description suggests it's
> > optional. It's either one or the other, please lets be clear about it. On that front, lets make it nr-gpios
> > *not* optional (i.e. make it
> > required) thus force the specification of how many SGPIOs we want to emit on the bus. This value is
> > coupled to the platform design, so I don't think there's ever a scenario where we want nr-gpios to take a
> > default value.
> >
>
> Added some descriptions and updated nr-gpios, please see v3.
>
> > > +
> > > +- clocks : A phandle to the APB clock for SGPM clock
> > > division
> > > +
> > > +- bus-frequency : SGPM CLK frequency, if not specified defaults to 1
> > > MHz
> >
> > Again here with the default value - SGPM CLK period is derived from PCLK by the expression `period =
> > PCLK * 2 *(GPIO254[31:16] + 1)`, where GPIO254's initialisation state is `GPIO254[31:16] = 0`, which
> > gives a default SGPM bus frequency of PCLK / 2. This is likely not going to be 1MHz (more like ~12MHz).
> >
> > Lets just make the property required. That way we avoid any ambiguity about the bus frequency and
> > thus don't need words about defaults that turn out to be about the driver, not about the hardware.
> >
>
> updated, please see v3.
>
> > Finally, when updating patches in response to feedback, please send the full series again, and bump the
> > series version number. That way people can review a coherent set of patches and not have to hunt
> > around and (fail to) collate the correct combination. It makes it easier to say "Reviewed-by:" on your
> > patches :)
> >
> > Cheers,
> >
> > Andrew
>
^ 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