* uImage target support on arm64
From: Russell King - ARM Linux @ 2018-05-24 21:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CA+Kvs9=x4kxkj-amni+E_KVjH2qjq_mXU88Dr-x5NNyz64gfuA@mail.gmail.com>
On Thu, May 24, 2018 at 11:17:19PM +0300, Ramon Fried wrote:
> On Thu, May 24, 2018 at 10:34 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> > Hi Ramon,
> >
> > On Thu, May 24, 2018 at 10:05:15PM +0300, Ramon Fried wrote:
> >> I've noticed that it's not supported.
> >> Is it on purpose ?
> >
> > Yes. The 32bit load address in the uImage header in pretty limited when
> > applied to 64bit ARM64. Even for ARM zImage is the preferred kernel format for
> > quite some time now, since it allows flexible load address, as well as
> > multi-platform kernels.
> Hi Baruch.
> I though that in terms of U-boot, the new FIT image is the preferred
> kernel format.
u-boot keeps inventing new image formats for itself that are specific
to u-boot. Not every boot loader is u-boot.
For 32-bit ARM, zImage has _always_ since day one been the preferred
format.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Ulf Hansson @ 2018-05-24 21:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <96169281-49b2-121f-0b7e-f54aae66db91@nvidia.com>
[...]
>>>
>>> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>>> * @dev: Device to attach.
>>> * @index: The index of the PM domain.
>>>
>>> This naming and description is a bit misleading, because really it is not
>>> attaching the device that is passed, but creating a new device to attach
>>> a
>>> PM domain to. So we should consider renaming and changing the description
>>> and indicate that users need to link the device.
>>
>>
>> I picked the name to be consistent with the existing
>> genpd_dev_pm_attach(). Do you have a better suggestion?
>
>
> Well, it appears to get more of a 'get' function and so I don't see why we
> could not have 'genpd_dev_get_by_id()' and then we could have a
> genpd_dev_put() as well (which would call genpd_dev_pm_detach).
>
>> I agree, some details is missing to the description, let me try to
>> improve it. Actually, I was trying to follow existing descriptions
>> from genpd_dev_pm_attach(), so perhaps that also needs a little
>> update.
>>
>> However, do note that, neither genpd_dev_pm_attach() or
>> genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
>> rather only by the driver core. So description may not be so
>> important.
>>
>> In regards to good descriptions, for sure the API added in patch9,
>> dev_pm_domain_attach_by_id(), needs a good one, as this is what
>> drivers should be using.
>
>
> OK. Same appears to apply here to the description as I mentioned above.
> Still seems to be more of a 'get' than an attach. So I wonder if it should
> be dev_pm_domain_get_by_id() instead?
Regarding "get" vs "attach", I suggest we continue to discuss that in
patch 9. Whatever is decided, $subject patch needs to follow.
>
>>> Finally, how is a PM domain attached via calling
>>> genpd_dev_pm_attach_by_id()
>>> detached?
>>
>>
>> Via the existing genpd_dev_pm_detach(), according to what I have
>> described in the change log. I clarify the description in regards to
>> this as well.
>
>
> OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that you
> said 'although we need to extend it to cover cleanup of the earlier
> registered device, via calling device_unregister().' So if we do this then
> that would be fine.
Let me clarify the changelog. It's not a to-do, as it's already done
as part of $subject patch.
So I guess we are in agreement that we don't need another API to deal
with detach?
Kind regards
Uffe
^ permalink raw reply
* [PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains
From: Ulf Hansson @ 2018-05-24 21:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1c21d18e-954a-f3a8-9817-0117b7cb7e4f@nvidia.com>
On 24 May 2018 at 17:48, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 18/05/18 11:31, Ulf Hansson wrote:
>>
>> The existing dev_pm_domain_attach() function, allows a single PM domain to
>> be attached per device. To be able to support devices that are partitioned
>> across multiple PM domains, let's introduce a new interface,
>> dev_pm_domain_attach_by_id().
>>
>> The dev_pm_domain_attach_by_id() returns a new allocated struct device
>> with
>> the corresponding attached PM domain. This enables for example a driver to
>> operate on the new device from a power management point of view. The
>> driver
>> may then also benefit from using the received device, to set up so called
>> device-links towards its original device. Depending on the situation,
>> these
>> links may then be dynamically changed.
>>
>> The new interface is typically called by drivers during their probe phase,
>> in case they manages devices which uses multiple PM domains. If that is
>> the
>> case, the driver also becomes responsible of managing the detaching of the
>> PM domains, which typically should be done at the remove phase. Detaching
>> is done by calling the existing dev_pm_domain_detach() function and for
>> each of the received devices from dev_pm_domain_attach_by_id().
>>
>> Note, currently its only genpd that supports multiple PM domains per
>> device, but dev_pm_domain_attach_by_id() can easily by extended to cover
>> other PM domain types, if/when needed.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>> drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++-
>> include/linux/pm_domain.h | 7 +++++++
>> 2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>> index 7ae62b6..d3db974 100644
>> --- a/drivers/base/power/common.c
>> +++ b/drivers/base/power/common.c
>> @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool
>> power_on)
>> EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>> /**
>> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains.
>
>
> Isn't this more of a 'get'?
I don't think so, at least according to the common understanding of
how we use get and put APIs.
For example, clk_get() returns a cookie to a clk, which you then can
do a hole bunch of different clk specific operations on.
This is different, there are no specific PM domain operations the
caller can or should do. Instead the idea is to keep drivers more or
less transparent, still using runtime PM as before. The only care the
caller need to take is to use device links, which BTW isn't a PM
domain specific thing.
>
>> + * @index: The index of the PM domain.
>> + * @dev: Device to attach.
>
>
> Isn't this just the device associated with the PM domain we are getting?
Correct, but please note, as stated above, I don't think it's a good
idea to return a special PM domain cookie, because we don't want the
caller to run special PM domain operations.
>
>> + *
>> + * As @dev may only be attached to a single PM domain, the backend PM
>> domain
>> + * provider should create a virtual device to attach instead. As
>> attachment
>> + * succeeds, the ->detach() callback in the struct dev_pm_domain should
>> be
>> + * assigned by the corresponding backend attach function.
>> + *
>> + * This function should typically be invoked from drivers during probe
>> phase.
>> + * Especially for those that manages devices which requires power
>> management
>> + * through more than one PM domain.
>> + *
>> + * Callers must ensure proper synchronization of this function with power
>> + * management callbacks.
>> + *
>> + * Returns the virtual attached device in case successfully attached PM
>> domain,
>> + * NULL in case @dev don't need a PM domain, else a PTR_ERR().
>
>
> Should this be 'NULL in the case where the @dev already has a power-domain'?
Yes, I think so. The intent is to be consistent with how
dev_pm_domain_attach() works and according to the latest changes I did
for it. It's queued for 4.18, please have a look in Rafael's tree and
you will see.
>
>> + */
>> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
>> + unsigned int index)
>> +{
>> + if (dev->pm_domain)
>
>
> I wonder if this is worthy of a ...
>
> if (WARN_ON(dev->pm_domain))
>
>> + return NULL;
>
>
> Don't we consider this an error case? I wonder why not return PTR_ERR here
> as well? This would be consistent with dev_pm_domain_attach().
Please see above comment.
Kind regards
Uffe
^ permalink raw reply
* [PATCH v2 1/8] driver core: make deferring probe after init optional
From: Rob Herring @ 2018-05-24 20:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524190031.GB31019@kroah.com>
On Thu, May 24, 2018 at 2:00 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote:
>> Deferred probe will currently wait forever on dependent devices to probe,
>> but sometimes a driver will never exist. It's also not always critical for
>> a driver to exist. Platforms can rely on default configuration from the
>> bootloader or reset defaults for things such as pinctrl and power domains.
>> This is often the case with initial platform support until various drivers
>> get enabled. There's at least 2 scenarios where deferred probe can render
>> a platform broken. Both involve using a DT which has more devices and
>> dependencies than the kernel supports. The 1st case is a driver may be
>> disabled in the kernel config. The 2nd case is the kernel version may
>> simply not have the dependent driver. This can happen if using a newer DT
>> (provided by firmware perhaps) with a stable kernel version.
>>
>> Subsystems or drivers may opt-in to this behavior by calling
>> driver_deferred_probe_check_init_done() instead of just returning
>> -EPROBE_DEFER. They may use additional information from DT or kernel's
>> config to decide whether to continue to defer probe or not.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>> drivers/base/dd.c | 17 +++++++++++++++++
>> include/linux/device.h | 2 ++
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index c9f54089429b..d6034718da6f 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -226,6 +226,16 @@ void device_unblock_probing(void)
>> driver_deferred_probe_trigger();
>> }
>>
>> +int driver_deferred_probe_check_init_done(struct device *dev, bool optional)
>> +{
>> + if (optional && initcalls_done) {
>
> Wait, what's the "optional" mess here?
My intent was that subsystems just always call this function and never
return EPROBE_DEFER themselves. Then the driver core can make
decisions as to what to do (such as the timeout added in the next
patch). Or it can print common error/debug messages. So optional is a
hint to allow subsystems per device control.
>
> The caller knows this value, so why do you need to even pass it in here?
Because regardless of the value, we always stop deferring when/if we
hit the timeout and the caller doesn't know about the timeout. If we
get rid of it, we'd need functions for both init done and for deferred
timeout.
> And bool values that are not obvious are horrid. I had to go look this
> up when reading the later patches that just passed "true" in this
> variable as I had no idea what that meant.
Perhaps inverting it and calling it "keep_deferring" would be better.
However, the flag is ignored if we have timed out.
>
> So as-is, no, this isn't ok, sorry.
>
> And at the least, this needs some kerneldoc to explain it :)
That part is easy enough to fix.
Rob
^ permalink raw reply
* [PATCH] ARM: pxa3xx: enable external wakeup pins
From: Daniel Mack @ 2018-05-24 20:40 UTC (permalink / raw)
To: linux-arm-kernel
The PXA3xx SoCs feature dedicated pins for wakeup functionality. These pins
have no alternate functions, so let's always enable them as wakeup source on
DT enabled boards. The WAKEUP1 pin is only available on PXA320.
Signed-off-by: Daniel Mack <daniel@zonque.org>
---
arch/arm/mach-pxa/pxa3xx.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
index 2a5044dd463e..9e05f92cd97a 100644
--- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -442,6 +442,10 @@ static int __init pxa3xx_init(void)
pxa3xx_init_pm();
+ enable_irq_wake(IRQ_WAKEUP0);
+ if (cpu_is_pxa320())
+ enable_irq_wake(IRQ_WAKEUP1);
+
register_syscore_ops(&pxa_irq_syscore_ops);
register_syscore_ops(&pxa3xx_mfp_syscore_ops);
--
2.14.3
^ permalink raw reply related
* [PATCH 3/6] ASoC: ams_delta: use GPIO lookup table
From: Janusz Krzysztofik @ 2018-05-24 20:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180523185244.GT98604@atomide.com>
On Wednesday, May 23, 2018 8:52:44 PM CEST Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [180521 10:07]:
> > On Fri, May 18, 2018 at 11:09:51PM +0200, Janusz Krzysztofik wrote:
> > > Now as the Amstrad Delta board provides GPIO lookup tables, switch from
> > > GPIO numbers to GPIO descriptors and use the table to locate required
> > > GPIO pins.
> >
> > Acked-by: Mark Brown <broonie@kernel.org>
>
Hi Tony,
> Thanks applying patches 1 and 3 of this series into omap-for-v4.18/soc.
> It's kind of getting late for v4.18, but let's see if we can still make
> it.
Thank you.
> Seems the others can be applied to the driver trees after no more
> comments, then once all that is done we can apply the last patch
> in this series.
I'll be submitting v2 of 5/6 (nand) very soon. 4/6 (lcd) is still waiting for
Tomi to respond. I hope there will be no issues with it. Howevver, regarding
2/6 - serio - I have to work more on that to satisfy Dmitry's comments. So
let's forget about 6/6 for now and I'll resubmit it again when we are ready
for that. Meanwhile, I'm going to submit a few more patches against the board
init file to complete migration to GPIO descriptors so dynamic allocation of
GPIO numbers to ams-delta latches will be possible.
Thanks,
Janusz
^ permalink raw reply
* [linux-sunxi] Re: [PATCH 05/15] drm/sun4i: Add TCON TOP driver
From: Jernej Škrabec @ 2018-05-24 20:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524084351.x4ugbbsz3mqv6fh7@flea>
Hi,
Dne ?etrtek, 24. maj 2018 ob 10:43:51 CEST je Maxime Ripard napisal(a):
> Hi,
>
> On Mon, May 21, 2018 at 05:15:15PM +0200, Jernej ?krabec wrote:
> > > > + /*
> > > > + * Default register values might have some reserved bits set,
which
> > > > + * prevents TCON TOP from working properly. Set them to 0 here.
> > > > + */
> > > > + writel(0, tcon_top->regs + TCON_TOP_PORT_SEL_REG);
> > > > + writel(0, tcon_top->regs + TCON_TOP_GATE_SRC_REG);
> > > > +
> > > > + for (i = 0; i < CLK_NUM; i++) {
> > > > + const char *parent_name = "bus-tcon-top";
> > >
> > > I guess retrieving the parent's clock name at runtime would be more
> > > flexible.
> >
> > It is, but will it ever be anything else?
>
> Probably not, but when the complexity is exactly the same (using
> __clk_get_name), we'd better use the more appropriate solution. If we
> ever need to change that clock name, or to use the driver with an SoC
> that wouldn't have the same clock name for whatever reason, it will
> just work.
>
> > > > + struct clk_init_data init;
> > > > + struct clk_gate *gate;
> > > > +
> > > > + gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
> > > > + if (!gate) {
> > > > + ret = -ENOMEM;
> > > > + goto err_disable_clock;
> > > > + }
> > > > +
> > > > + init.name = gates[i].name;
> > > > + init.ops = &clk_gate_ops;
> > > > + init.flags = CLK_IS_BASIC;
> > > > + init.parent_names = &parent_name;
> > > > + init.num_parents = 1;
> > > > +
> > > > + gate->reg = tcon_top->regs + TCON_TOP_GATE_SRC_REG;
> > > > + gate->bit_idx = gates[i].bit;
> > > > + gate->lock = &tcon_top->reg_lock;
> > > > + gate->hw.init = &init;
> > > > +
> > > > + ret = devm_clk_hw_register(dev, &gate->hw);
> > > > + if (ret)
> > > > + goto err_disable_clock;
> > >
> > > Isn't it what clk_hw_register_gate is doing?
> >
> > Almost, but not exactly. My goal was to use devm_* functions, so there is
> > no need to do any special cleanup.
>
> Is it the only difference? If so, you can just create a
> devm_clk_hw_register gate.
I checked around and it seems that in clk core there are only non devm_*
helpers like clk_hw_register_gate() for some reason. I guess I'll just use
that and manually unregister all the clocks in cleanup function.
Best regards,
Jernej
^ permalink raw reply
* [PATCH v2 1/8] driver core: make deferring probe after init optional
From: Rob Herring @ 2018-05-24 20:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524181834.GF4828@sirena.org.uk>
On Thu, May 24, 2018 at 1:18 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote:
>
>> Subsystems or drivers may opt-in to this behavior by calling
>> driver_deferred_probe_check_init_done() instead of just returning
>> -EPROBE_DEFER. They may use additional information from DT or kernel's
>> config to decide whether to continue to defer probe or not.
>
> Should userspace have some involvement in this decision? It knows if
> it's got any intention of loading modules for example. Kernel config
> checks might be good enough, though it's going to be a pain to work out
> if the relevant driver is built as a module for example.
I looked into whether we could hook into uevents in some way. If we
knew when all the coldplug events had been handled, that would be
sufficient. But it doesn't look to me like we can tell when that
happens with the uevent netlink socket. I think about the only thing
we can tell is if userspace has opened a socket. I'm not all that
familiar with how the whole sequence works, so other opinions on this
would be helpful.
Also, for this to work with serial consoles, we have to make the
decision before we get to userspace. I couldn't get systemd to create
serial gettys either if we deferred later. There's some dependence on
/dev/console, but I didn't get to the bottom of it.
Rob
^ permalink raw reply
* uImage target support on arm64
From: Ramon Fried @ 2018-05-24 20:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524193449.jql27apudwaevwpf@tarshish>
On Thu, May 24, 2018 at 10:34 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> Hi Ramon,
>
> On Thu, May 24, 2018 at 10:05:15PM +0300, Ramon Fried wrote:
>> I've noticed that it's not supported.
>> Is it on purpose ?
>
> Yes. The 32bit load address in the uImage header in pretty limited when
> applied to 64bit ARM64. Even for ARM zImage is the preferred kernel format for
> quite some time now, since it allows flexible load address, as well as
> multi-platform kernels.
Hi Baruch.
I though that in terms of U-boot, the new FIT image is the preferred
kernel format.
Thanks,
Ramon.
>
>> If not I'll be happy to add support for: make uImage
>> for arm64 targets.
>
> baruch
>
> --
> http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
> - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
^ permalink raw reply
* [reset-control] How to initialize hardware state with the shared reset line?
From: Martin Blumenstingl @ 2018-05-24 20:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526997879.3671.27.camel@pengutronix.de>
Hi Philipp,
On Tue, May 22, 2018 at 4:04 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Martin,
>
> On Mon, 2018-05-21 at 12:40 +0200, Martin Blumenstingl wrote:
>> Hello,
>>
>> On Mon, May 21, 2018 at 3:27 AM, Masahiro Yamada
>> <yamada.masahiro@socionext.com> wrote:
>> > Hi.
>> >
>> >
>> > 2018-05-20 19:57 GMT+09:00 Martin Blumenstingl
>> > <martin.blumenstingl@googlemail.com>:
>> > > Hi,
>> > >
>> > > On Thu, May 10, 2018 at 11:16 AM, Masahiro Yamada
>> > > <yamada.masahiro@socionext.com> wrote:
>> > > [snip]
>> > > > I may be missing something, but
>> > > > one solution might be reset hogging on the
>> > > > reset provider side. This allows us to describe
>> > > > the initial state of reset lines in the reset controller.
>> > > >
>> > > > The idea for "reset-hog" is similar to:
>> > > > - "gpio-hog" defined in
>> > > > Documentation/devicetree/bindings/gpio/gpio.txt
>> > > > - "assigned-clocks" defined in
>> > > > Documetation/devicetree/bindings/clock/clock-bindings.txt
>> > > >
>> > > >
>> > > >
>> > > > For example,
>> > > >
>> > > > reset-controller {
>> > > > ....
>> > > >
>> > > > line_a {
>> > > > reset-hog;
>> > > > resets = <1>;
>> > > > reset-assert;
>> > > > };
>> > > > }
>> > > >
>> > > >
>> > > > When the reset controller is registered,
>> > > > the reset ID '1' is asserted.
>> > > >
>> > > >
>> > > > So, all reset consumers that share the reset line '1'
>> > > > will start from the asserted state
>> > > > (i.e. defined state machine state).
>> > >
>> > > I wonder if a "reset hog" can be board specific:
>> > > - GPIO hogs are definitely board specific (meson-gxbb-odroidc2.dts for
>> > > example uses it to take the USB hub out of reset)
>> > > - assigned-clock-parents (and the like) can also be board specific (I
>> > > made up a use-case since I don't know of any actual examples: board A
>> > > uses an external XTAL while board B uses some other internal
>> > > clock-source because it doesn't have an external XTAL)
>> > >
>> > > however, can reset lines be board specific? or in other words: do we
>> > > need to describe them in device-tree?
>> >
>> > Indeed.
>> >
>> > I did not come up with board-specific cases.
>> >
>> > The problem we are discussing is SoC-specific,
>> > and reset-controller drivers are definitely SoC-specific.
>> >
>> > So, I think the initial state can be coded in drivers instead of DT.
>>
>> OK, let's also hear Philipp's (reset framework maintainer) opinion on this
>
> I'd like to know if there are other SoC families besides Amlogic Meson
> that potentially could have this problem and about how many of the
> resets that are documented in include/dt-bindings/reset/amlogic,meson*
> we are actually talking. Are all of those initially deasserted and none
> of the connected peripherals have power-on reset mechanisms?
I cannot speak for other SoC families besides Amlogic
Meson8/Meson8b/Meson8m2 and GX (disclaimer: I am a community
contributor, I don't have access to Amlogic's internal datasheets - my
knowledge is based on their public datasheets, their GPL kernel/u-boot
sources and trial and error)
it seems that at least "some" (but I don't know the exact number)
resets are de-asserted by the bootloader
Amlogic's u-boot for example also enables all gate clocks by default
I CC'ed the Amlogic mailing list because I'm not sure if everyone
working on that SoC family is watching the linux-arm-kernel mailing
list
>> > > we could extend struct reset_controller_dev (= reset controller
>> > > driver) if they are not board specific:
>> > > - either assert all reset lines by default except if they are listed
>> > > in a new field (may break backwards compatibility, requires testing of
>> > > all reset controller drivers)
>> >
>> > This is quite simple, but I am afraid there are some cases where the forcible
>> > reset-assert is not preferred.
>> >
>> > For example, the earlycon. When we use earlycon, we would expect it has been
>> > initialized by a boot-loader, or something.
>> > If it is reset-asserted on the while, the console output
>> > will not be good.
>>
>> indeed, so let's skip this idea
>
> Maybe we should at first add initial reset assertion to the Meson driver
> on a case by case bases?
this seems simple enough to test it - we can still generalize this
later on (either by adding support to the reset framework, DT bindings
or something else)
> We can't add required reset hog DT bindings to the Meson reset
> controller anyway without breaking DT backwards compatibility.
>
>> > > - specify a list of reset lines and their desired state (or to keep it
>> > > easy: specify a list of reset lines that should be asserted)
>> > > (I must admit that this is basically your idea but the definition is
>> > > moved from device-tree to the reset controller driver)
>> >
>> > Yes, I think the list of "reset line ID" and "init state" pairs
>> > would be nicer.
>>
>> $ grep -R "of_reset_n_cells = [^1]" drivers/reset/
>> drivers/reset/reset-berlin.c: priv->rcdev.of_reset_n_cells = 2;
>> drivers/reset/hisilicon/reset-hi3660.c: rc->rst.of_reset_n_cells = 2;
>> drivers/reset/reset-ti-sci.c: data->rcdev.of_reset_n_cells = 2;
>> drivers/reset/reset-lantiq.c: priv->rcdev.of_reset_n_cells = 2;
>>
>> everything else uses only one reset cell
>> from the lantiq reset dt-binding documentation: "The first cell takes
>> the reset set bit and the second cell takes the status bit."
>>
>> I'm not sure what to do with drivers that specify != 1 reset-cell
>> though if we use a simple "init state pair"
>> maybe Philipp can share his opinion on this one as well
>
> See above, so far I am not convinced (either way) whether this should be
> described in the DT at all.
>
>> > > any "chip" specific differences could be expressed by using a
>> > > different of_device_id
>> > >
>> > > one the other hand: your "reset hog" solution looks fine to me if
>> > > reset lines can be board specific
>> > >
>> > > > From the discussion with Martin Blumenstingl
>> > > > (https://lkml.org/lkml/2018/4/28/115),
>> > > > the problem for Amlogic is that
>> > > > the reset line is "de-asserted" by default.
>> > > > If so, the 'reset-hog' would fix the problem,
>> > > > and DWC3 driver would be able to use
>> > > > shared, level reset, I think.
>> > >
>> > > I think you are right: if we could control the initial state then we
>> > > should be able to use level resets
>> >
>> >
>> > Even further, can we drop the shared reset_control_reset() support, maybe?
>> > (in other words, revert commit 7da33a37b48f11)
>>
>> I believe we need to keep this if there's hardware out there:
>> - where the reset controller only supports reset pulses
>> - at least one reset line is shared between multiple devices
>>
>> I didn't have a closer look at the Amlogic Meson6 SoC yet, but I think
>> it matches above criteria. as far as I know:
>> - the USB situation there is similar to Meson8b (USB controllers and
>> PHYs share a reset line)
>> - it uses an older reset controller IP block which does not support
>> level resets (only reset pulses)
>
> See my answer to Masahiro's first mail. I think somebody suggested in
> the past to add a fallback from the deassert to the reset op. I think
> this is something that should work in this case.
this is an interesting idea - it should work for Meson6 (in case
mainline ever gains support for this old SoC)
Regards
Martin
^ permalink raw reply
* [PATCH v3 2/3] arm64: dts: renesas: draak: Describe CVBS input
From: jacopo mondi @ 2018-05-24 19:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1728709.thk7gXDxlo@avalon>
HI Laurent,
On Mon, May 21, 2018 at 04:10:34PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Monday, 21 May 2018 15:33:40 EEST jacopo mondi wrote:
> > On Mon, May 21, 2018 at 01:54:55PM +0300, Laurent Pinchart wrote:
> > > On Monday, 21 May 2018 12:57:05 EEST jacopo mondi wrote:
> > >> On Fri, May 18, 2018 at 06:12:15PM +0300, Laurent Pinchart wrote:
> > >>> On Friday, 18 May 2018 17:47:57 EEST Jacopo Mondi wrote:
> > >>>> Describe CVBS video input through analog video decoder ADV7180
> > >>>> connected to video input interface VIN4.
> > >>>>
> > >>>> The video input signal path is shared with HDMI video input, and
> > >>>> selected by on-board switches SW-53 and SW-54 with CVBS input
> > >>>> selected by the default switches configuration.
> > >>>>
> > >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >>>> Reviewed-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
> > >>>>
> > >>>> ---
> > >>>> v2 -> v3:
> > >>>> - Add comment to describe the shared input video path
> > >>>> - Add my SoB and Niklas' R-b tags
> > >>>> ---
> > >>>>
> > >>>> arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 42 ++++++++++++++++++
> > >>>> 1 file changed, 42 insertions(+)
> > >>>>
> > >>>> diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> > >>>> b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts index
> > >>>> 9d73de8..95745fc
> > >>>> 100644
> > >>>> --- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> > >>>> +++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> > >>>> @@ -142,6 +142,11 @@
> > >>>> groups = "usb0";
> > >>>> function = "usb0";
> > >>>> };
> > >>>> +
> > >>>> + vin4_pins_cvbs: vin4 {
> > >>>> + groups = "vin4_data8", "vin4_sync", "vin4_clk";
> > >>>> + function = "vin4";
> > >>>> + };
> > >>>> };
> > >>>>
> > >>>> &i2c0 {
> > >>>> @@ -154,6 +159,23 @@
> > >>>> reg = <0x50>;
> > >>>> pagesize = <8>;
> > >>>> };
> > >>>> +
> > >>>> + analog-video at 20 {
> > >>>> + compatible = "adi,adv7180";
> > >>>> + reg = <0x20>;
> > >>>> +
> > >>>> + port {
> > >>>
> > >>> The adv7180 DT bindings document the output port as 3 or 6
> > >>> (respectively for the CP and ST versions of the chip). You should thus
> > >>> number the port. Apart from that the patch looks good.
> > >>
> > >> I admit I have barely copied this from Gen-2 boards DTS, but reading
> > >> the driver code and binding description again, I think this is
> > >> correct, as the output port numbering and mandatory input port (which
> > >> is missing here) only apply to adv7180cp/st chip versions.
> > >>
> > >> Here we describe plain adv7180, no need to number output ports afaict.
> > >
> > > Indeed, my bad.
> > >
> > > Shouldn't you however use "adi,adv7180cp" as that's the chip we are using
> > > ?
> > > The "adi,adv7180" has no port documented in its DT bindings, so it
> > > shouldn't have any port node at all.
> >
> > I'm a bit confused here.
> >
> > The only Gen-2 board using the "adi,adv7180cp" compatible string is
> > Gose, which is also the only one I can get schematics for. That board
> > indeed feature an ADV7180WBCP32Z, as the Draak does. I cannot get
> > schematics for any other Gen-2 board, to compare the ADV7180 variant
> > installed there. If anyone can confirm that all other Gen-2 board uses
> > a different version (or that all other Gen-2 board DTS file use a
> > wrong compatible string value), I'll re-send this with a different
> > compatible value and proper port nodes numbering.
>
> Other Gen2 boards use a ADV7180WBCP32Z as well. The issue here isn't that the
> chip you're trying to support is different. The DT bindings that were
> initially written for the adi,adv7180 didn't have port nodes. When it was time
> to add them, we realized that two variants of the chip existed with different
> connectivity. We have thus added two new compatible strings to differentiate
> them, with different port numbers. The old compatible string should be
> deprecated in favour of the new ones.
>
Ack, thanks for explaining.
I don't have any Gen2 board here, but if someone is willing to test, I
can change the Gen2 bindings to use the new version in a follow-up
series.
Thanks
j
> > >>>> + /*
> > >>>> + * The VIN4 video input path is shared between
> > >>>> + * CVBS and HDMI inputs through SW[49-54] switches.
> > >>>> + *
> > >>>> + * CVBS is the default selection, link it to VIN4 here.
> > >>>> + */
> > >>>> + adv7180_out: endpoint {
> > >>>> + remote-endpoint = <&vin4_in>;
> > >>>> + };
> > >>>> + };
> > >>>> + };
> > >>>> };
> > >>>>
> > >>>> &i2c1 {
> > >>>> @@ -246,3 +268,23 @@
> > >>>> timeout-sec = <60>;
> > >>>> status = "okay";
> > >>>> };
> >>>> +
> > >>>> +&vin4 {
> > >>>> + pinctrl-0 = <&vin4_pins_cvbs>;
> > >>>> + pinctrl-names = "default";
> > >>>> +
> > >>>> + status = "okay";
> > >>>> +
> > >>>> + ports {
> > >>>> + #address-cells = <1>;
> > >>>> + #size-cells = <0>;
> > >>>> +
> > >>>> + port at 0 {
> > >>>> + reg = <0>;
> > >>>> +
> > >>>> + vin4_in: endpoint {
> > >>>> + remote-endpoint = <&adv7180_out>;
> > >>>> + };
> > >>>> + };
> > >>>> + };
> > >>>> +};
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180524/7c99b1ef/attachment.sig>
^ permalink raw reply
* [PATCH 10/11] coresight: tmc-etr: Add transparent buffer management
From: Mathieu Poirier @ 2018-05-24 19:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526661567-4578-11-git-send-email-suzuki.poulose@arm.com>
On Fri, May 18, 2018 at 05:39:26PM +0100, Suzuki K Poulose wrote:
> At the moment we always use contiguous memory for TMC ETR tracing
> when used from sysfs. The size of the buffer is fixed at boot time
> and can only be changed by modifiying the DT. With the introduction
> of SG support we could support really large buffers in that mode.
> This patch abstracts the buffer used for ETR to switch between a
> contiguous buffer or a SG table depending on the availability of
> the memory.
>
> This also enables the sysfs mode to use the ETR in SG mode depending
> on configured the trace buffer size. Also, since ETR will use the
> new infrastructure to manage the buffer, we can get rid of some
> of the members in the tmc_drvdata and clean up the fields a bit.
Upon first reading this changelog I thought this patch does way too many things
but after looking at the content it isn't the case. We could try to split the
patch by moving the introduction of the SG operations to another patch but it
would save about 60 lines, which is hardly worth it.
As it stands now it is alsmost guaranted other reviewers will ask you to split
your work. Perhaps rephrasing the changelog to concentrate on the global idea
of what the patch does will help (just my personnal opinion).
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-tmc-etr.c | 450 +++++++++++++++++++-----
> drivers/hwtracing/coresight/coresight-tmc.h | 57 ++-
> 2 files changed, 418 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 7ab0fd1..143afba 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -17,10 +17,18 @@
>
> #include <linux/coresight.h>
> #include <linux/dma-mapping.h>
> +#include <linux/iommu.h>
> #include <linux/slab.h>
> #include "coresight-priv.h"
> #include "coresight-tmc.h"
>
> +struct etr_flat_buf {
> + struct device *dev;
> + dma_addr_t daddr;
> + void *vaddr;
> + size_t size;
> +};
> +
> /*
> * The TMC ETR SG has a page size of 4K. The SG table contains pointers
> * to 4KB buffers. However, the OS may use a PAGE_SIZE different from
> @@ -541,7 +549,7 @@ static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table)
> * @size - Total size of the data buffer
> * @pages - Optional list of page virtual address
> */
> -static struct etr_sg_table __maybe_unused *
> +static struct etr_sg_table *
> tmc_init_etr_sg_table(struct device *dev, int node,
> unsigned long size, void **pages)
> {
> @@ -573,16 +581,307 @@ tmc_init_etr_sg_table(struct device *dev, int node,
> return etr_table;
> }
>
> +/*
> + * tmc_etr_alloc_flat_buf: Allocate a contiguous DMA buffer.
> + */
> +static int tmc_etr_alloc_flat_buf(struct tmc_drvdata *drvdata,
> + struct etr_buf *etr_buf, int node,
> + void **pages)
> +{
> + struct etr_flat_buf *flat_buf;
> +
> + /* We cannot reuse existing pages for flat buf */
> + if (pages)
> + return -EINVAL;
Perfect.
> +
> + flat_buf = kzalloc(sizeof(*flat_buf), GFP_KERNEL);
> + if (!flat_buf)
> + return -ENOMEM;
> +
> + flat_buf->vaddr = dma_alloc_coherent(drvdata->dev, etr_buf->size,
> + &flat_buf->daddr, GFP_KERNEL);
Indendation of the second line.
> + if (!flat_buf->vaddr) {
> + kfree(flat_buf);
> + return -ENOMEM;
> + }
> +
> + flat_buf->size = etr_buf->size;
> + flat_buf->dev = drvdata->dev;
> + etr_buf->hwaddr = flat_buf->daddr;
> + etr_buf->mode = ETR_MODE_FLAT;
> + etr_buf->private = flat_buf;
> + return 0;
> +}
> +
> +static void tmc_etr_free_flat_buf(struct etr_buf *etr_buf)
> +{
> + struct etr_flat_buf *flat_buf = etr_buf->private;
> +
> + if (flat_buf && flat_buf->daddr)
> + dma_free_coherent(flat_buf->dev, flat_buf->size,
> + flat_buf->vaddr, flat_buf->daddr);
> + kfree(flat_buf);
> +}
> +
> +static void tmc_etr_sync_flat_buf(struct etr_buf *etr_buf, u64 rrp, u64 rwp)
> +{
> + /*
> + * Adjust the buffer to point to the beginning of the trace data
> + * and update the available trace data.
> + */
> + etr_buf->offset = rrp - etr_buf->hwaddr;
> + if (etr_buf->full)
> + etr_buf->len = etr_buf->size;
> + else
> + etr_buf->len = rwp - rrp;
> +}
> +
> +static ssize_t tmc_etr_get_data_flat_buf(struct etr_buf *etr_buf,
> + u64 offset, size_t len, char **bufpp)
> +{
> + struct etr_flat_buf *flat_buf = etr_buf->private;
> +
> + *bufpp = (char *)flat_buf->vaddr + offset;
> + /*
> + * tmc_etr_buf_get_data already adjusts the length to handle
> + * buffer wrapping around.
> + */
> + return len;
> +}
> +
> +static const struct etr_buf_operations etr_flat_buf_ops = {
> + .alloc = tmc_etr_alloc_flat_buf,
> + .free = tmc_etr_free_flat_buf,
> + .sync = tmc_etr_sync_flat_buf,
> + .get_data = tmc_etr_get_data_flat_buf,
> +};
> +
> +/*
> + * tmc_etr_alloc_sg_buf: Allocate an SG buf @etr_buf. Setup the parameters
> + * appropriately.
> + */
> +static int tmc_etr_alloc_sg_buf(struct tmc_drvdata *drvdata,
> + struct etr_buf *etr_buf, int node,
> + void **pages)
> +{
> + struct etr_sg_table *etr_table;
> +
> + etr_table = tmc_init_etr_sg_table(drvdata->dev, node,
> + etr_buf->size, pages);
> + if (IS_ERR(etr_table))
> + return -ENOMEM;
> + etr_buf->hwaddr = etr_table->hwaddr;
> + etr_buf->mode = ETR_MODE_ETR_SG;
> + etr_buf->private = etr_table;
> + return 0;
> +}
> +
> +static void tmc_etr_free_sg_buf(struct etr_buf *etr_buf)
> +{
> + struct etr_sg_table *etr_table = etr_buf->private;
> +
> + if (etr_table) {
> + tmc_free_sg_table(etr_table->sg_table);
> + kfree(etr_table);
> + }
> +}
> +
> +static ssize_t tmc_etr_get_data_sg_buf(struct etr_buf *etr_buf, u64 offset,
> + size_t len, char **bufpp)
> +{
> + struct etr_sg_table *etr_table = etr_buf->private;
> +
> + return tmc_sg_table_get_data(etr_table->sg_table, offset, len, bufpp);
> +}
> +
> +static void tmc_etr_sync_sg_buf(struct etr_buf *etr_buf, u64 rrp, u64 rwp)
> +{
> + long r_offset, w_offset;
> + struct etr_sg_table *etr_table = etr_buf->private;
> + struct tmc_sg_table *table = etr_table->sg_table;
> +
> + /* Convert hw address to offset in the buffer */
> + r_offset = tmc_sg_get_data_page_offset(table, rrp);
> + if (r_offset < 0) {
> + dev_warn(table->dev,
> + "Unable to map RRP %llx to offset\n", rrp);
> + etr_buf->len = 0;
> + return;
> + }
> +
> + w_offset = tmc_sg_get_data_page_offset(table, rwp);
> + if (w_offset < 0) {
> + dev_warn(table->dev,
> + "Unable to map RWP %llx to offset\n", rwp);
> + etr_buf->len = 0;
> + return;
> + }
> +
> + etr_buf->offset = r_offset;
> + if (etr_buf->full)
> + etr_buf->len = etr_buf->size;
> + else
> + etr_buf->len = ((w_offset < r_offset) ? etr_buf->size : 0) +
> + w_offset - r_offset;
> + tmc_sg_table_sync_data_range(table, r_offset, etr_buf->len);
> +}
> +
> +static const struct etr_buf_operations etr_sg_buf_ops = {
> + .alloc = tmc_etr_alloc_sg_buf,
> + .free = tmc_etr_free_sg_buf,
> + .sync = tmc_etr_sync_sg_buf,
> + .get_data = tmc_etr_get_data_sg_buf,
> +};
> +
> +static const struct etr_buf_operations *etr_buf_ops[] = {
> + [ETR_MODE_FLAT] = &etr_flat_buf_ops,
> + [ETR_MODE_ETR_SG] = &etr_sg_buf_ops,
> +};
> +
> +static inline int tmc_etr_mode_alloc_buf(int mode,
> + struct tmc_drvdata *drvdata,
> + struct etr_buf *etr_buf, int node,
> + void **pages)
> +{
> + int rc;
> +
> + switch (mode) {
> + case ETR_MODE_FLAT:
> + case ETR_MODE_ETR_SG:
> + rc = etr_buf_ops[mode]->alloc(drvdata, etr_buf, node, pages);
> + if (!rc)
> + etr_buf->ops = etr_buf_ops[mode];
> + return rc;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +/*
> + * tmc_alloc_etr_buf: Allocate a buffer use by ETR.
> + * @drvdata : ETR device details.
> + * @size : size of the requested buffer.
> + * @flags : Required properties for the buffer.
> + * @node : Node for memory allocations.
> + * @pages : An optional list of pages.
> + */
> +static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata,
> + ssize_t size, int flags,
> + int node, void **pages)
> +{
> + int rc = -ENOMEM;
> + bool has_etr_sg, has_iommu;
> + struct etr_buf *etr_buf;
> +
> + has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG);
> + has_iommu = iommu_get_domain_for_dev(drvdata->dev);
> +
> + etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL);
> + if (!etr_buf)
> + return ERR_PTR(-ENOMEM);
> +
> + etr_buf->size = size;
> +
> + /*
> + * If we have to use an existing list of pages, we cannot reliably
> + * use a contiguous DMA memory (even if we have an IOMMU). Otherwise,
> + * we use the contiguous DMA memory if at least one of the following
> + * conditions is true:
> + * a) The ETR cannot use Scatter-Gather.
> + * b) we have a backing IOMMU
> + * c) The requested memory size is smaller (< 1M).
> + *
> + * Fallback to available mechanisms.
> + *
> + */
> + if (!pages &&
> + (!has_etr_sg || has_iommu || size < SZ_1M))
> + rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata,
> + etr_buf, node, pages);
> + if (rc && has_etr_sg)
> + rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata,
> + etr_buf, node, pages);
> + if (rc) {
> + kfree(etr_buf);
> + return ERR_PTR(rc);
> + }
> +
> + return etr_buf;
> +}
> +
> +static void tmc_free_etr_buf(struct etr_buf *etr_buf)
> +{
> + WARN_ON(!etr_buf->ops || !etr_buf->ops->free);
> + etr_buf->ops->free(etr_buf);
> + kfree(etr_buf);
> +}
> +
> +/*
> + * tmc_etr_buf_get_data: Get the pointer the trace data at @offset
> + * with a maximum of @len bytes.
> + * Returns: The size of the linear data available @pos, with *bufpp
> + * updated to point to the buffer.
> + */
> +static ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
> + u64 offset, size_t len, char **bufpp)
> +{
> + /* Adjust the length to limit this transaction to end of buffer */
> + len = (len < (etr_buf->size - offset)) ? len : etr_buf->size - offset;
> +
> + return etr_buf->ops->get_data(etr_buf, (u64)offset, len, bufpp);
> +}
> +
> +static inline s64
> +tmc_etr_buf_insert_barrier_packet(struct etr_buf *etr_buf, u64 offset)
> +{
> + ssize_t len;
> + char *bufp;
> +
> + len = tmc_etr_buf_get_data(etr_buf, offset,
> + CORESIGHT_BARRIER_PKT_SIZE, &bufp);
> + if (WARN_ON(len <= CORESIGHT_BARRIER_PKT_SIZE))
> + return -EINVAL;
> + coresight_insert_barrier_packet(bufp);
> + return offset + CORESIGHT_BARRIER_PKT_SIZE;
> +}
> +
> +/*
> + * tmc_sync_etr_buf: Sync the trace buffer availability with drvdata.
> + * Makes sure the trace data is synced to the memory for consumption.
> + * @etr_buf->offset will hold the offset to the beginning of the trace data
> + * within the buffer, with @etr_buf->len bytes to consume.
> + */
> +static void tmc_sync_etr_buf(struct tmc_drvdata *drvdata)
> +{
> + struct etr_buf *etr_buf = drvdata->etr_buf;
> + u64 rrp, rwp;
> + u32 status;
> +
> + rrp = tmc_read_rrp(drvdata);
> + rwp = tmc_read_rwp(drvdata);
> + status = readl_relaxed(drvdata->base + TMC_STS);
> + etr_buf->full = status & TMC_STS_FULL;
> +
> + WARN_ON(!etr_buf->ops || !etr_buf->ops->sync);
> +
> + etr_buf->ops->sync(etr_buf, rrp, rwp);
> +
> + /* Insert barrier packets at the beginning, if there was an overflow */
> + if (etr_buf->full)
> + tmc_etr_buf_insert_barrier_packet(etr_buf, etr_buf->offset);
> +}
> +
> static void tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
> {
> u32 axictl, sts;
> + struct etr_buf *etr_buf = drvdata->etr_buf;
>
> CS_UNLOCK(drvdata->base);
>
> /* Wait for TMCSReady bit to be set */
> tmc_wait_for_tmcready(drvdata);
>
> - writel_relaxed(drvdata->size / 4, drvdata->base + TMC_RSZ);
> + writel_relaxed(etr_buf->size / 4, drvdata->base + TMC_RSZ);
> writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
>
> axictl = readl_relaxed(drvdata->base + TMC_AXICTL);
> @@ -595,16 +894,22 @@ static void tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
> axictl |= TMC_AXICTL_ARCACHE_OS;
> }
>
> + if (etr_buf->mode == ETR_MODE_ETR_SG) {
> + if (WARN_ON(!tmc_etr_has_cap(drvdata, TMC_ETR_SG)))
> + return;
> + axictl |= TMC_AXICTL_SCT_GAT_MODE;
> + }
> +
> writel_relaxed(axictl, drvdata->base + TMC_AXICTL);
> - tmc_write_dba(drvdata, drvdata->paddr);
> + tmc_write_dba(drvdata, etr_buf->hwaddr);
> /*
> * If the TMC pointers must be programmed before the session,
> * we have to set it properly (i.e, RRP/RWP to base address and
> * STS to "not full").
> */
> if (tmc_etr_has_cap(drvdata, TMC_ETR_SAVE_RESTORE)) {
> - tmc_write_rrp(drvdata, drvdata->paddr);
> - tmc_write_rwp(drvdata, drvdata->paddr);
> + tmc_write_rrp(drvdata, etr_buf->hwaddr);
> + tmc_write_rwp(drvdata, etr_buf->hwaddr);
> sts = readl_relaxed(drvdata->base + TMC_STS) & ~TMC_STS_FULL;
> writel_relaxed(sts, drvdata->base + TMC_STS);
> }
> @@ -620,63 +925,53 @@ static void tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
> }
>
> /*
> - * Return the available trace data in the buffer @pos, with a maximum
> - * limit of @len, also updating the @bufpp on where to find it.
> + * Return the available trace data in the buffer (starts at etr_buf->offset,
> + * limited by etr_buf->len) from @pos, with a maximum limit of @len,
> + * also updating the @bufpp on where to find it. Since the trace data
> + * starts at anywhere in the buffer, depending on the RRP, we adjust the
> + * @len returned to handle buffer wrapping around.
> */
> ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata,
> loff_t pos, size_t len, char **bufpp)
> {
> + s64 offset;
> ssize_t actual = len;
> - char *bufp = drvdata->buf + pos;
> - char *bufend = (char *)(drvdata->vaddr + drvdata->size);
> -
> - /* Adjust the len to available size @pos */
> - if (pos + actual > drvdata->len)
> - actual = drvdata->len - pos;
> + struct etr_buf *etr_buf = drvdata->etr_buf;
>
> + if (pos + actual > etr_buf->len)
> + actual = etr_buf->len - pos;
> if (actual <= 0)
> return actual;
>
> - /*
> - * Since we use a circular buffer, with trace data starting
> - * @drvdata->buf, possibly anywhere in the buffer @drvdata->vaddr,
> - * wrap the current @pos to within the buffer.
> - */
> - if (bufp >= bufend)
> - bufp -= drvdata->size;
> - /*
> - * For simplicity, avoid copying over a wrapped around buffer.
> - */
> - if ((bufp + actual) > bufend)
> - actual = bufend - bufp;
> - *bufpp = bufp;
> - return actual;
> + /* Compute the offset from which we read the data */
> + offset = etr_buf->offset + pos;
> + if (offset >= etr_buf->size)
> + offset -= etr_buf->size;
> + return tmc_etr_buf_get_data(etr_buf, offset, actual, bufpp);
> }
>
> -static void tmc_etr_dump_hw(struct tmc_drvdata *drvdata)
> +static struct etr_buf *
> +tmc_etr_setup_sysfs_buf(struct tmc_drvdata *drvdata)
> {
> - u32 val;
> - u64 rwp;
> + return tmc_alloc_etr_buf(drvdata, drvdata->size,
> + 0, cpu_to_node(0), NULL);
> +}
>
> - rwp = tmc_read_rwp(drvdata);
> - val = readl_relaxed(drvdata->base + TMC_STS);
> +static void
> +tmc_etr_free_sysfs_buf(struct etr_buf *buf)
> +{
> + if (buf)
> + tmc_free_etr_buf(buf);
> +}
>
> - /*
> - * Adjust the buffer to point to the beginning of the trace data
> - * and update the available trace data.
> - */
> - if (val & TMC_STS_FULL) {
> - drvdata->buf = drvdata->vaddr + rwp - drvdata->paddr;
> - drvdata->len = drvdata->size;
> - coresight_insert_barrier_packet(drvdata->buf);
> - } else {
> - drvdata->buf = drvdata->vaddr;
> - drvdata->len = rwp - drvdata->paddr;
> - }
> +static void tmc_etr_sync_sysfs_buf(struct tmc_drvdata *drvdata)
> +{
> + tmc_sync_etr_buf(drvdata);
> }
>
> static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
> {
> +
> CS_UNLOCK(drvdata->base);
>
> tmc_flush_and_stop(drvdata);
> @@ -685,7 +980,8 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
> * read before the TMC is disabled.
> */
> if (drvdata->mode == CS_MODE_SYSFS)
> - tmc_etr_dump_hw(drvdata);
> + tmc_etr_sync_sysfs_buf(drvdata);
> +
> tmc_disable_hw(drvdata);
>
> CS_LOCK(drvdata->base);
> @@ -696,34 +992,31 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
> int ret = 0;
> bool used = false;
> unsigned long flags;
> - void __iomem *vaddr = NULL;
> - dma_addr_t paddr;
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + struct etr_buf *new_buf = NULL, *free_buf = NULL;
>
>
> /*
> - * If we don't have a buffer release the lock and allocate memory.
> - * Otherwise keep the lock and move along.
> + * If we are enabling the ETR from disabled state, we need to make
> + * sure we have a buffer with the right size. The etr_buf is not reset
> + * immediately after we stop the tracing in SYSFS mode as we wait for
> + * the user to collect the data. We may be able to reuse the existing
> + * buffer, provided the size matches. Any allocation has to be done
> + * with the lock released.
> */
> spin_lock_irqsave(&drvdata->spinlock, flags);
> - if (!drvdata->vaddr) {
> + if (!drvdata->etr_buf || (drvdata->etr_buf->size != drvdata->size)) {
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
> -
> - /*
> - * Contiguous memory can't be allocated while a spinlock is
> - * held. As such allocate memory here and free it if a buffer
> - * has already been allocated (from a previous session).
> - */
> - vaddr = dma_alloc_coherent(drvdata->dev, drvdata->size,
> - &paddr, GFP_KERNEL);
> - if (!vaddr)
> - return -ENOMEM;
> + /* Allocate memory with the spinlock released */
> + free_buf = new_buf = tmc_etr_setup_sysfs_buf(drvdata);
> + if (IS_ERR(new_buf))
> + return PTR_ERR(new_buf);
>
> /* Let's try again */
> spin_lock_irqsave(&drvdata->spinlock, flags);
> }
>
> - if (drvdata->reading) {
> + if (drvdata->reading || drvdata->mode == CS_MODE_PERF) {
> ret = -EBUSY;
> goto out;
> }
> @@ -731,21 +1024,20 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
> /*
> * In sysFS mode we can have multiple writers per sink. Since this
> * sink is already enabled no memory is needed and the HW need not be
> - * touched.
> + * touched, even if the buffer size has changed.
> */
> if (drvdata->mode == CS_MODE_SYSFS)
> goto out;
>
> /*
> - * If drvdata::buf == NULL, use the memory allocated above.
> - * Otherwise a buffer still exists from a previous session, so
> - * simply use that.
> + * If we don't have a buffer or it doesn't match the requested size,
> + * use the memory allocated above. Otherwise reuse it.
> */
> - if (drvdata->buf == NULL) {
> + if (!drvdata->etr_buf ||
> + (new_buf && drvdata->etr_buf->size != new_buf->size)) {
> used = true;
With the introduction of variable free_buf, this is no longer needed.
> - drvdata->vaddr = vaddr;
> - drvdata->paddr = paddr;
> - drvdata->buf = drvdata->vaddr;
> + free_buf = drvdata->etr_buf;
> + drvdata->etr_buf = new_buf;
> }
>
> drvdata->mode = CS_MODE_SYSFS;
> @@ -754,8 +1046,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> /* Free memory outside the spinlock if need be */
> - if (!used && vaddr)
> - dma_free_coherent(drvdata->dev, drvdata->size, vaddr, paddr);
> + if (free_buf)
> + tmc_etr_free_sysfs_buf(free_buf);
>
> if (!ret)
> dev_info(drvdata->dev, "TMC-ETR enabled\n");
> @@ -834,8 +1126,8 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
> goto out;
> }
>
> - /* If drvdata::buf is NULL the trace data has been read already */
> - if (drvdata->buf == NULL) {
> + /* If drvdata::etr_buf is NULL the trace data has been read already */
> + if (drvdata->etr_buf == NULL) {
As I pointed out during my last review this hunk doesn't apply on my next
branch and as such can't review this part.
> ret = -EINVAL;
> goto out;
> }
> @@ -854,8 +1146,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
> int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
> {
> unsigned long flags;
> - dma_addr_t paddr;
> - void __iomem *vaddr = NULL;
> + struct etr_buf *etr_buf = NULL;
>
> /* config types are set a boot time and never change */
> if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
> @@ -876,17 +1167,16 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
> * The ETR is not tracing and the buffer was just read.
> * As such prepare to free the trace buffer.
> */
> - vaddr = drvdata->vaddr;
> - paddr = drvdata->paddr;
> - drvdata->buf = drvdata->vaddr = NULL;
> + etr_buf = drvdata->etr_buf;
> + drvdata->etr_buf = NULL;
> }
>
> drvdata->reading = false;
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> /* Free allocated memory out side of the spinlock */
> - if (vaddr)
> - dma_free_coherent(drvdata->dev, drvdata->size, vaddr, paddr);
> + if (etr_buf)
> + tmc_free_etr_buf(etr_buf);
>
> return 0;
> }
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 19a765c..c00643c 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -55,6 +55,7 @@
> #define TMC_STS_TMCREADY_BIT 2
> #define TMC_STS_FULL BIT(0)
> #define TMC_STS_TRIGGERED BIT(1)
> +
> /*
> * TMC_AXICTL - 0x110
> *
> @@ -134,6 +135,35 @@ enum tmc_mem_intf_width {
> #define CORESIGHT_SOC_600_ETR_CAPS \
> (TMC_ETR_SAVE_RESTORE | TMC_ETR_AXI_ARCACHE)
>
> +enum etr_mode {
> + ETR_MODE_FLAT, /* Uses contiguous flat buffer */
> + ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */
> +};
> +
> +struct etr_buf_operations;
> +
> +/**
> + * struct etr_buf - Details of the buffer used by ETR
> + * @mode : Mode of the ETR buffer, contiguous, Scatter Gather etc.
> + * @full : Trace data overflow
> + * @size : Size of the buffer.
> + * @hwaddr : Address to be programmed in the TMC:DBA{LO,HI}
> + * @offset : Offset of the trace data in the buffer for consumption.
> + * @len : Available trace data @buf (may round up to the beginning).
> + * @ops : ETR buffer operations for the mode.
> + * @private : Backend specific information for the buf
> + */
> +struct etr_buf {
> + enum etr_mode mode;
> + bool full;
> + ssize_t size;
> + dma_addr_t hwaddr;
> + unsigned long offset;
> + s64 len;
> + const struct etr_buf_operations *ops;
> + void *private;
> +};
> +
> /**
> * struct tmc_drvdata - specifics associated to an TMC component
> * @base: memory mapped base address for this component.
> @@ -141,11 +171,10 @@ enum tmc_mem_intf_width {
> * @csdev: component vitals needed by the framework.
> * @miscdev: specifics to handle "/dev/xyz.tmc" entry.
> * @spinlock: only one at a time pls.
> - * @buf: area of memory where trace data get sent.
> - * @paddr: DMA start location in RAM.
> - * @vaddr: virtual representation of @paddr.
> - * @size: trace buffer size.
> - * @len: size of the available trace.
> + * @buf: Snapshot of the trace data for ETF/ETB.
> + * @etr_buf: details of buffer used in TMC-ETR
> + * @len: size of the available trace for ETF/ETB.
> + * @size: trace buffer size for this TMC (common for all modes).
> * @mode: how this TMC is being used.
> * @config_type: TMC variant, must be of type @tmc_config_type.
> * @memwidth: width of the memory interface databus, in bytes.
> @@ -160,11 +189,12 @@ struct tmc_drvdata {
> struct miscdevice miscdev;
> spinlock_t spinlock;
> bool reading;
> - char *buf;
> - dma_addr_t paddr;
> - void __iomem *vaddr;
> - u32 size;
> + union {
> + char *buf; /* TMC ETB */
> + struct etr_buf *etr_buf; /* TMC ETR */
> + };
> u32 len;
> + u32 size;
> u32 mode;
> enum tmc_config_type config_type;
> enum tmc_mem_intf_width memwidth;
> @@ -172,6 +202,15 @@ struct tmc_drvdata {
> u32 etr_caps;
> };
>
> +struct etr_buf_operations {
> + int (*alloc)(struct tmc_drvdata *drvdata, struct etr_buf *etr_buf,
> + int node, void **pages);
> + void (*sync)(struct etr_buf *etr_buf, u64 rrp, u64 rwp);
> + ssize_t (*get_data)(struct etr_buf *etr_buf, u64 offset, size_t len,
> + char **bufpp);
> + void (*free)(struct etr_buf *etr_buf);
> +};
> +
> /**
> * struct tmc_pages - Collection of pages used for SG.
> * @nr_pages: Number of pages in the list.
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH v2 2/8] driver core: add a deferred probe timeout
From: Greg Kroah-Hartman @ 2018-05-24 19:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAL_Jsq+wMJ+9JTGFGSsSW2Vww_mvAkzc3ujmftgbpQxDTrLHag@mail.gmail.com>
On Thu, May 24, 2018 at 02:45:48PM -0500, Rob Herring wrote:
> On Thu, May 24, 2018 at 2:01 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, May 24, 2018 at 12:50:18PM -0500, Rob Herring wrote:
> >> Deferring probe can wait forever on dependencies that may never appear
> >> for a variety of reasons. This can be difficult to debug especially if
> >> the console has dependencies or userspace fails to boot to a shell. Add
> >> a timeout to retry probing without possibly optional dependencies and to
> >> dump out the deferred probe pending list after retrying.
> >>
> >> This mechanism is intended for debug purposes. It won't work for the
> >> console which needs to be enabled before userspace starts. However, if
> >> the console's dependencies are resolved, then the kernel log will be
> >> printed (as opposed to no output).
> >>
> >> Signed-off-by: Rob Herring <robh@kernel.org>
> >> ---
> >> .../admin-guide/kernel-parameters.txt | 7 +++++
> >> drivers/base/dd.c | 28 ++++++++++++++++++-
> >> 2 files changed, 34 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >> index 11fc28ecdb6d..dd3f40b34a24 100644
> >> --- a/Documentation/admin-guide/kernel-parameters.txt
> >> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -809,6 +809,13 @@
> >> Defaults to the default architecture's huge page size
> >> if not specified.
> >>
> >> + deferred_probe_timeout=
> >> + [KNL] Set a timeout in seconds for deferred probe to
> >> + give up waiting on dependencies to probe. Only specific
> >> + dependencies (subsystems or drivers) that have opted in
> >> + will be ignored. This option also dumps out devices
> >> + still on the deferred probe list after retrying.
> >
> > Doesn't sound like a debugging-only option. I can see devices enabling
> > this when they figure out that's the only way their platform can boot :)
>
> Here's some rope...
>
> No doubt it can be abused. So are you saying don't call it a debug
> option or hide it behind a config option? And for the latter, what's
> one that distros don't just turn on?
If it is a debug option, make it obvious it's only for debugging. The
commit log here says it's a debug option, but your documentation does
not say that at all, and that's what people will read.
Well, they might read it, probably not. But at least give us something
to point at when they mess things up :)
thanks,
greg k-h
^ permalink raw reply
* [RESEND PATCH] ARM: dts: imx6q: Use correct SDMA script for SPI5 core
From: Sean Nyekjaer @ 2018-05-24 19:46 UTC (permalink / raw)
To: linux-arm-kernel
According to the reference manual the shp_2_mcu / mcu_2_shp
scripts must be used for devices connected through the SPBA.
This fixes an issue we saw with DMA transfers.
Sometimes the SPI controller RX FIFO was not empty after a DMA
transfer and the driver got stuck in the next PIO transfer when
it read one word more than expected.
commit dd4b487b32a35 ("ARM: dts: imx6: Use correct SDMA script
for SPI cores") is fixing the same issue but only for SPI1 - 4.
Fixes: 677940258dd8e ("ARM: dts: imx6q: enable dma for ecspi5")
Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
---
arch/arm/boot/dts/imx6q.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index ae7b3f107893..5185300cc11f 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -96,7 +96,7 @@
clocks = <&clks IMX6Q_CLK_ECSPI5>,
<&clks IMX6Q_CLK_ECSPI5>;
clock-names = "ipg", "per";
- dmas = <&sdma 11 7 1>, <&sdma 12 7 2>;
+ dmas = <&sdma 11 8 1>, <&sdma 12 8 2>;
dma-names = "rx", "tx";
status = "disabled";
};
--
2.17.0
^ permalink raw reply related
* [PATCH v2 2/8] driver core: add a deferred probe timeout
From: Rob Herring @ 2018-05-24 19:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524190126.GC31019@kroah.com>
On Thu, May 24, 2018 at 2:01 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, May 24, 2018 at 12:50:18PM -0500, Rob Herring wrote:
>> Deferring probe can wait forever on dependencies that may never appear
>> for a variety of reasons. This can be difficult to debug especially if
>> the console has dependencies or userspace fails to boot to a shell. Add
>> a timeout to retry probing without possibly optional dependencies and to
>> dump out the deferred probe pending list after retrying.
>>
>> This mechanism is intended for debug purposes. It won't work for the
>> console which needs to be enabled before userspace starts. However, if
>> the console's dependencies are resolved, then the kernel log will be
>> printed (as opposed to no output).
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>> .../admin-guide/kernel-parameters.txt | 7 +++++
>> drivers/base/dd.c | 28 ++++++++++++++++++-
>> 2 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 11fc28ecdb6d..dd3f40b34a24 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -809,6 +809,13 @@
>> Defaults to the default architecture's huge page size
>> if not specified.
>>
>> + deferred_probe_timeout=
>> + [KNL] Set a timeout in seconds for deferred probe to
>> + give up waiting on dependencies to probe. Only specific
>> + dependencies (subsystems or drivers) that have opted in
>> + will be ignored. This option also dumps out devices
>> + still on the deferred probe list after retrying.
>
> Doesn't sound like a debugging-only option. I can see devices enabling
> this when they figure out that's the only way their platform can boot :)
Here's some rope...
No doubt it can be abused. So are you saying don't call it a debug
option or hide it behind a config option? And for the latter, what's
one that distros don't just turn on?
Rob
^ permalink raw reply
* [PATCH v2 1/8] driver core: make deferring probe after init optional
From: Rob Herring @ 2018-05-24 19:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524185639.GA31019@kroah.com>
On Thu, May 24, 2018 at 1:56 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote:
>> Deferred probe will currently wait forever on dependent devices to probe,
>> but sometimes a driver will never exist. It's also not always critical for
>> a driver to exist. Platforms can rely on default configuration from the
>> bootloader or reset defaults for things such as pinctrl and power domains.
>> This is often the case with initial platform support until various drivers
>> get enabled. There's at least 2 scenarios where deferred probe can render
>> a platform broken. Both involve using a DT which has more devices and
>> dependencies than the kernel supports. The 1st case is a driver may be
>> disabled in the kernel config. The 2nd case is the kernel version may
>> simply not have the dependent driver. This can happen if using a newer DT
>> (provided by firmware perhaps) with a stable kernel version.
>>
>> Subsystems or drivers may opt-in to this behavior by calling
>> driver_deferred_probe_check_init_done() instead of just returning
>> -EPROBE_DEFER. They may use additional information from DT or kernel's
>> config to decide whether to continue to defer probe or not.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>> drivers/base/dd.c | 17 +++++++++++++++++
>> include/linux/device.h | 2 ++
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index c9f54089429b..d6034718da6f 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -226,6 +226,16 @@ void device_unblock_probing(void)
>> driver_deferred_probe_trigger();
>> }
>>
>> +int driver_deferred_probe_check_init_done(struct device *dev, bool optional)
>> +{
>> + if (optional && initcalls_done) {
>> + dev_WARN(dev, "ignoring dependency for device, assuming no driver");
>
> You really only need dev_warn(), here, right?
No, the screaming is on purpose.
Bjorn had concerns about debugging/supporting subtle problems that
could stem from this. Such as electrical settings not quite right that
cause intermittent errors.
Rob
^ permalink raw reply
* uImage target support on arm64
From: Baruch Siach @ 2018-05-24 19:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CA+Kvs9=mgW4Z=UcBZZLL82fJ=c--OXF3=B2Zbho+CfMTr8Wk-Q@mail.gmail.com>
Hi Ramon,
On Thu, May 24, 2018 at 10:05:15PM +0300, Ramon Fried wrote:
> I've noticed that it's not supported.
> Is it on purpose ?
Yes. The 32bit load address in the uImage header in pretty limited when
applied to 64bit ARM64. Even for ARM zImage is the preferred kernel format for
quite some time now, since it allows flexible load address, as well as
multi-platform kernels.
> If not I'll be happy to add support for: make uImage
> for arm64 targets.
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
^ permalink raw reply
* [PATCH 4/4] arm64: cpufeature: always log KPTI setting on boot
From: Mark Langsdorf @ 2018-05-24 19:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524190932.32118-1-mlangsdo@redhat.com>
Always log KPTI setting at boot time, whether or not KPTI was forced
by a kernel parameter.
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
---
arch/arm64/kernel/cpufeature.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 697a6ef..e50bf3c 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -889,16 +889,20 @@ static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
__pti_enabled = __kpti_forced > 0;
pr_info_once("kernel page table isolation forced %s by %s\n",
__pti_enabled ? "ON" : "OFF", str);
+ } else {
+ /* Useful for KASLR robustness */
+ if (IS_ENABLED(CONFIG_RANDOMIZE_BASE))
+ __pti_enabled = true;
+ /* Don't force KPTI for CPUs that are not vulnerable */
+ else if (is_midr_in_range_list(read_cpuid_id(), kpti_safe_list))
+ __pti_enabled = false;
+ /* Defer to CPU feature registers */
+ else
+ __pti_enabled = !has_cpuid_feature(entry, scope);
+
+ pr_info_once("kernel page table isolation %s by %s\n",
+ __pti_enabled ? "ON" : "OFF", str);
}
- /* Useful for KASLR robustness */
- else if (IS_ENABLED(CONFIG_RANDOMIZE_BASE))
- __pti_enabled = true;
- /* Don't force KPTI for CPUs that are not vulnerable */
- else if (is_midr_in_range_list(read_cpuid_id(), kpti_safe_list))
- __pti_enabled = false;
- /* Defer to CPU feature registers */
- else
- __pti_enabled = !has_cpuid_feature(entry, scope);
return __pti_enabled;
}
--
2.9.5
^ permalink raw reply related
* [PATCH 3/4] arm64: cpufeature: add debugfs interface for KPTI
From: Mark Langsdorf @ 2018-05-24 19:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524190932.32118-1-mlangsdo@redhat.com>
Add a debugfs interface to check if KPTI is enabled or disabled:
cat /sys/kernel/debug/arm64/pti_enabled
Slightly rework unmap_kernel_at_el0 logic to simplify calculating if
KPTI is enabled.
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
---
arch/arm64/kernel/cpufeature.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 7c5d8712..697a6ef 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -24,6 +24,7 @@
#include <linux/stop_machine.h>
#include <linux/types.h>
#include <linux/mm.h>
+#include <linux/debugfs.h>
#include <asm/cpu.h>
#include <asm/cpufeature.h>
#include <asm/cpu_ops.h>
@@ -860,6 +861,7 @@ static bool has_cache_dic(const struct arm64_cpu_capabilities *entry,
#ifdef CONFIG_UNMAP_KERNEL_AT_EL0
static int __kpti_forced; /* 0: not forced, >0: forced on, <0: forced off */
+static bool __pti_enabled;
static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
int scope)
@@ -884,21 +886,21 @@ static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
/* Forced? */
if (__kpti_forced) {
+ __pti_enabled = __kpti_forced > 0;
pr_info_once("kernel page table isolation forced %s by %s\n",
- __kpti_forced > 0 ? "ON" : "OFF", str);
- return __kpti_forced > 0;
+ __pti_enabled ? "ON" : "OFF", str);
}
-
/* Useful for KASLR robustness */
- if (IS_ENABLED(CONFIG_RANDOMIZE_BASE))
- return true;
-
+ else if (IS_ENABLED(CONFIG_RANDOMIZE_BASE))
+ __pti_enabled = true;
/* Don't force KPTI for CPUs that are not vulnerable */
- if (is_midr_in_range_list(read_cpuid_id(), kpti_safe_list))
- return false;
-
+ else if (is_midr_in_range_list(read_cpuid_id(), kpti_safe_list))
+ __pti_enabled = false;
/* Defer to CPU feature registers */
- return !has_cpuid_feature(entry, scope);
+ else
+ __pti_enabled = !has_cpuid_feature(entry, scope);
+
+ return __pti_enabled;
}
static void
@@ -947,6 +949,14 @@ static int __init force_nokpti(char *arg)
return 0;
}
early_param("nopti", force_nokpti);
+
+static int __init create_kpti_enabled(void)
+{
+ debugfs_create_bool("pti_enabled", S_IRUSR,
+ arch_debugfs_dir, &__pti_enabled);
+ return 0;
+}
+late_initcall(create_kpti_enabled);
#endif /* CONFIG_UNMAP_KERNEL_AT_EL0 */
#ifdef CONFIG_ARM64_HW_AFDBM
--
2.9.5
^ permalink raw reply related
* [PATCH 2/4] arm64: kdebugfs: create arm64 debugfs directory
From: Mark Langsdorf @ 2018-05-24 19:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524190932.32118-1-mlangsdo@redhat.com>
Create debugfs directory for arm64, like the existing one for x86.
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
---
arch/arm64/kernel/Makefile | 3 ++-
arch/arm64/kernel/kdebugfs.c | 27 +++++++++++++++++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kernel/kdebugfs.c
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index bf825f3..a48b298 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -18,7 +18,8 @@ arm64-obj-y := debug-monitors.o entry.o irq.o fpsimd.o \
hyp-stub.o psci.o cpu_ops.o insn.o \
return_address.o cpuinfo.o cpu_errata.o \
cpufeature.o alternative.o cacheinfo.o \
- smp.o smp_spin_table.o topology.o smccc-call.o
+ smp.o smp_spin_table.o topology.o smccc-call.o \
+ kdebugfs.o
extra-$(CONFIG_EFI) := efi-entry.o
diff --git a/arch/arm64/kernel/kdebugfs.c b/arch/arm64/kernel/kdebugfs.c
new file mode 100644
index 0000000..e1e3332
--- /dev/null
+++ b/arch/arm64/kernel/kdebugfs.c
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Architecture specific debugfs files
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/export.h>
+#include <linux/init.h>
+
+struct dentry *arch_debugfs_dir;
+EXPORT_SYMBOL(arch_debugfs_dir);
+
+static int __init arch_kdebugfs_init(void)
+{
+ int error = 0;
+
+ arch_debugfs_dir = debugfs_create_dir("arm64", NULL);
+ if (IS_ERR(arch_debugfs_dir)) {
+ arch_debugfs_dir = NULL;
+ return -ENOMEM;
+ }
+
+ return error;
+}
+postcore_initcall(arch_kdebugfs_init);
--
2.9.5
^ permalink raw reply related
* [PATCH 1/4] arm64: capabilities: add nopti command line argument
From: Mark Langsdorf @ 2018-05-24 19:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524190932.32118-1-mlangsdo@redhat.com>
The x86 kernel and the documentation use 'nopti' as the kernel command
line argument to disable kernel page table isolation, so add nopti to
the arm64 kernel for compatibility.
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
---
Documentation/admin-guide/kernel-parameters.txt | 6 +++---
arch/arm64/kernel/cpufeature.c | 11 ++++++++++-
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index f2040d4..a987725 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3342,8 +3342,8 @@
pt. [PARIDE]
See Documentation/blockdev/paride.txt.
- pti= [X86_64] Control Page Table Isolation of user and
- kernel address spaces. Disabling this feature
+ pti= [X86_64, ARM64] Control Page Table Isolation of user
+ and kernel address spaces. Disabling this feature
removes hardening, but improves performance of
system calls and interrupts.
@@ -3354,7 +3354,7 @@
Not specifying this option is equivalent to pti=auto.
- nopti [X86_64]
+ nopti [X86_64, ARM64]
Equivalent to pti=off
pty.legacy_count=
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 9d1b06d..7c5d8712 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -934,10 +934,19 @@ static int __init parse_kpti(char *str)
if (ret)
return ret;
- __kpti_forced = enabled ? 1 : -1;
+ if (!__kpti_forced)
+ __kpti_forced = enabled ? 1 : -1;
return 0;
}
__setup("kpti=", parse_kpti);
+
+/* for compatibility with documentation and x86 nopti command line arg */
+static int __init force_nokpti(char *arg)
+{
+ __kpti_forced = -1;
+ return 0;
+}
+early_param("nopti", force_nokpti);
#endif /* CONFIG_UNMAP_KERNEL_AT_EL0 */
#ifdef CONFIG_ARM64_HW_AFDBM
--
2.9.5
^ permalink raw reply related
* [PATCH 0/4] arm64: align KPTI interface with x86
From: Mark Langsdorf @ 2018-05-24 19:09 UTC (permalink / raw)
To: linux-arm-kernel
ARM64 supports KPTI, but is missing some user interface features such as
a debugfs entry compared to the x86 implementation.
Add the nopti argument, update the documentation so that ARM64 as well
as x86 supports nopti and kpti, and add a debugfs entry to check the
status of the kpti on a running machine.
--Mark Langsdorf
^ permalink raw reply
* uImage target support on arm64
From: Ramon Fried @ 2018-05-24 19:05 UTC (permalink / raw)
To: linux-arm-kernel
Hi.
I've noticed that it's not supported.
Is it on purpose ?
If not I'll be happy to add support for: make uImage
for arm64 targets.
Warm regards,
Ramon.
^ permalink raw reply
* [PATCH v2 2/8] driver core: add a deferred probe timeout
From: Greg Kroah-Hartman @ 2018-05-24 19:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524175024.19874-3-robh@kernel.org>
On Thu, May 24, 2018 at 12:50:18PM -0500, Rob Herring wrote:
> Deferring probe can wait forever on dependencies that may never appear
> for a variety of reasons. This can be difficult to debug especially if
> the console has dependencies or userspace fails to boot to a shell. Add
> a timeout to retry probing without possibly optional dependencies and to
> dump out the deferred probe pending list after retrying.
>
> This mechanism is intended for debug purposes. It won't work for the
> console which needs to be enabled before userspace starts. However, if
> the console's dependencies are resolved, then the kernel log will be
> printed (as opposed to no output).
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> .../admin-guide/kernel-parameters.txt | 7 +++++
> drivers/base/dd.c | 28 ++++++++++++++++++-
> 2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 11fc28ecdb6d..dd3f40b34a24 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -809,6 +809,13 @@
> Defaults to the default architecture's huge page size
> if not specified.
>
> + deferred_probe_timeout=
> + [KNL] Set a timeout in seconds for deferred probe to
> + give up waiting on dependencies to probe. Only specific
> + dependencies (subsystems or drivers) that have opted in
> + will be ignored. This option also dumps out devices
> + still on the deferred probe list after retrying.
Doesn't sound like a debugging-only option. I can see devices enabling
this when they figure out that's the only way their platform can boot :)
thanks,
greg k-h
^ permalink raw reply
* [PATCH v2 1/8] driver core: make deferring probe after init optional
From: Greg Kroah-Hartman @ 2018-05-24 19:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524175024.19874-2-robh@kernel.org>
On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote:
> Deferred probe will currently wait forever on dependent devices to probe,
> but sometimes a driver will never exist. It's also not always critical for
> a driver to exist. Platforms can rely on default configuration from the
> bootloader or reset defaults for things such as pinctrl and power domains.
> This is often the case with initial platform support until various drivers
> get enabled. There's at least 2 scenarios where deferred probe can render
> a platform broken. Both involve using a DT which has more devices and
> dependencies than the kernel supports. The 1st case is a driver may be
> disabled in the kernel config. The 2nd case is the kernel version may
> simply not have the dependent driver. This can happen if using a newer DT
> (provided by firmware perhaps) with a stable kernel version.
>
> Subsystems or drivers may opt-in to this behavior by calling
> driver_deferred_probe_check_init_done() instead of just returning
> -EPROBE_DEFER. They may use additional information from DT or kernel's
> config to decide whether to continue to defer probe or not.
>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> drivers/base/dd.c | 17 +++++++++++++++++
> include/linux/device.h | 2 ++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index c9f54089429b..d6034718da6f 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -226,6 +226,16 @@ void device_unblock_probing(void)
> driver_deferred_probe_trigger();
> }
>
> +int driver_deferred_probe_check_init_done(struct device *dev, bool optional)
> +{
> + if (optional && initcalls_done) {
Wait, what's the "optional" mess here?
The caller knows this value, so why do you need to even pass it in here?
And bool values that are not obvious are horrid. I had to go look this
up when reading the later patches that just passed "true" in this
variable as I had no idea what that meant.
So as-is, no, this isn't ok, sorry.
And at the least, this needs some kerneldoc to explain it :)
thanks,
greg k-h
^ 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