* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Suman Anna @ 2020-05-21 19:06 UTC (permalink / raw)
To: Bjorn Andersson
Cc: devicetree, Mathieu Poirier, Loic Pallardy, Lokesh Vutla,
linux-remoteproc, Arnaud Pouliquen, linux-kernel, Clement Leger,
Rob Herring, linux-arm-kernel
In-Reply-To: <20200521175421.GI408178@builder.lan>
Hi Bjorn,
On 5/21/20 12:54 PM, Bjorn Andersson wrote:
> On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
>
>> The current remoteproc core has supported only 32-bit remote
>> processors and as such some of the current resource structures
>> may not scale well for 64-bit remote processors, and would
>> require new versions of resource types. Each resource is currently
>> identified by a 32-bit type field. Introduce the concept of version
>> for these resource types by overloading this 32-bit type field
>> into two 16-bit version and type fields with the existing resources
>> behaving as version 0 thereby providing backward compatibility.
>>
>> The version field is passed as an additional argument to each of
>> the handler functions, and all the existing handlers are updated
>> accordingly. Each specific handler will be updated on a need basis
>> when a new version of the resource type is added.
>>
>
> I really would prefer that we add additional types for the new
> structures, neither side will be compatible with new versions without
> enhancements to their respective implementations anyways.
OK.
>
>> An alternate way would be to introduce the new types as completely
>> new resource types which would require additional customization of
>> the resource handlers based on the 32-bit or 64-bit mode of a remote
>> processor, and introduction of an additional mode flag to the rproc
>> structure.
>>
>
> What would this "mode" indicate? If it's version 0 or 1?
No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
loading handlers if the resource types need to be segregated accordingly.
>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++----------
>> drivers/remoteproc/remoteproc_debugfs.c | 17 ++++++++++-------
>> include/linux/remoteproc.h | 8 +++++++-
>> 3 files changed, 32 insertions(+), 18 deletions(-)
>>
> [..]
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 77788a4bb94e..526d3cb45e37 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -86,7 +86,13 @@ struct resource_table {
>> * this header, and it should be parsed according to the resource type.
>> */
>> struct fw_rsc_hdr {
>> - u32 type;
>> + union {
>> + u32 type;
>> + struct {
>> + u16 t;
>> + u16 v;
>> + } st;
>
> I see your "type" is little endian...
Yeah, definitely a draw-back if we want to support big-endian rprocs. Do
you have any remoteprocs following big-endian? All TI remoteprocs are
little-endian except for really old ones.
regards
Suman
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/3] ARM: pxa: remove Compulab pxa2xx boards
From: Boris Brezillon @ 2020-05-21 19:10 UTC (permalink / raw)
To: Mike Rapoport
Cc: linux-mtd, Robert Jarzmik, Linux ARM, Arnd Bergmann,
Miquel Raynal
In-Reply-To: <20200521190158.GL1118872@kernel.org>
On Thu, 21 May 2020 22:01:58 +0300
Mike Rapoport <rppt@kernel.org> wrote:
> On Thu, May 21, 2020 at 09:26:10AM +0200, Arnd Bergmann wrote:
> > On Thu, May 21, 2020 at 9:12 AM Mike Rapoport <rppt@kernel.org> wrote:
> > >
> > > On Wed, May 20, 2020 at 05:21:51PM +0200, Arnd Bergmann wrote:
> > > > On Wed, May 20, 2020 at 12:21 PM Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> > > > >
> > > > > As these boards have no more users nor testers, and patching them has
> > > > > become a burden, be that because of the PCI part or the MTD NAND
> > > > > support, let's remove them.
> > > > >
> > > > > The cm-x300 will for now remain and represent Compulab boards at its
> > > > > best in the PXA department.
> > >
> > > I think this series missed the cm-x270 NAND driver, so this could be
> > > PATCH 4/3 (not even compile tested).
> > >
> > > From 56a11987f82ef8b32b25dfc17b849f9bbbf03e4d Mon Sep 17 00:00:00 2001
> > > From: Mike Rapoport <rppt@linux.ibm.com>
> > > Date: Thu, 21 May 2020 10:09:47 +0300
> > > Subject: [PATCH] mtd: rawnand: remove CM-X270 NAND driver
> > >
> > > The cm-x270 board have been removed and theres is no point to keep this
> > > driver.
> > >
> > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> >
> > Thanks! Removing that driver was actually how the discussion started,
> > so we definitely want to do that. Boris earlier patch "mtd: rawnand: Get
> > rid of the cmx270 driver" did it by changing the board file, now your
> > patch is the right thing to do, and we should remember to do the same
> > for mbxfb.
>
> Arnd, will you pick this to the SoC tree or should I resend?
Actually I sent a v2 removing only the MTD driver. You should probably
sync with Miquel to make sure the same patch is not applied twice.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/2] hwrng: iproc-rng200 - Set the quality value
From: Lukasz Stelmach @ 2020-05-21 19:14 UTC (permalink / raw)
To: Stefan Wahren
Cc: Florian Fainelli, Herbert Xu, Scott Branden, Matthias Brugger,
Stephan Mueller, Greg Kroah-Hartman, Matt Mackall, linux-kernel,
Krzysztof Kozlowski, linux-samsung-soc, Bartlomiej Zolnierkiewicz,
Kukjin Kim, Arnd Bergmann, linux-crypto, Ray Jui,
bcm-kernel-feedback-list, Markus Elfring, linux-arm-kernel
In-Reply-To: <573b2eff-3c74-90e9-50fa-644264d0a0e5@i2se.com>
[-- Attachment #1.1: Type: text/plain, Size: 2205 bytes --]
It was <2020-05-21 czw 13:00>, when Stefan Wahren wrote:
> Hi Lukasz,
>
> Am 19.05.20 um 23:25 schrieb Łukasz Stelmach:
>> The value was estimaded with ea_iid[1] using on 10485760 bytes read from
>> the RNG via /dev/hwrng. The min-entropy value calculated using the most
>> common value estimate (NIST SP 800-90P[2], section 6.3.1) was 7.964464.
>
> could you please mention in the commit the used hardware
> implementation(s) of iproc-rng200 to get this quality?
>
> AFAIK there is still no public register description at least for the
> bcm2711. So is it safe to assume that the suggested quality applies to
> all possible configurations?
I've learnt that there is a post-processing unit in RNG200 that tests
the output of the noise generator and fills FIFO only with data that
passes FIPS tests. Unlike simmilar unit in Exynos, it cannot be disabled
or bypassed. Therefore, after Stephan Mueller's thorough explanations I
am considering dropping this patch in v3.
However, I stil am still not 100% convinced that it is impossible to
assign the quality a reasonable non-zero value in such case.
> Thanks
> Stefan
>
>>
>> [1] https://protect2.fireeye.com/url?k=da4735b2-87d99b28-da46befd-0cc47a336fae-e1c21080bc6ab1e4&q=1&u=https%3A%2F%2Fgithub.com%2Fusnistgov%2FSP800-90B_EntropyAssessment
>> [2] https://csrc.nist.gov/publications/detail/sp/800-90b/final
>>
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> ---
>> drivers/char/hw_random/iproc-rng200.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/char/hw_random/iproc-rng200.c b/drivers/char/hw_random/iproc-rng200.c
>> index 32d9fe61a225..95669ece050f 100644
>> --- a/drivers/char/hw_random/iproc-rng200.c
>> +++ b/drivers/char/hw_random/iproc-rng200.c
>> @@ -199,6 +199,7 @@ static int iproc_rng200_probe(struct platform_device *pdev)
>> priv->rng.read = iproc_rng200_read,
>> priv->rng.init = iproc_rng200_init,
>> priv->rng.cleanup = iproc_rng200_cleanup,
>> + priv->rng.quality = 1000,
>>
>> /* Register driver */
>> ret = devm_hwrng_register(dev, &priv->rng);
>
>
--
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Bjorn Andersson @ 2020-05-21 19:21 UTC (permalink / raw)
To: Suman Anna
Cc: devicetree, Mathieu Poirier, Loic Pallardy, Lokesh Vutla,
linux-remoteproc, Arnaud Pouliquen, linux-kernel, Clement Leger,
Rob Herring, linux-arm-kernel
In-Reply-To: <b338480e-c586-f988-f5b6-784551b7beb6@ti.com>
On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
> Hi Bjorn,
>
> On 5/21/20 12:54 PM, Bjorn Andersson wrote:
> > On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
> >
> > > The current remoteproc core has supported only 32-bit remote
> > > processors and as such some of the current resource structures
> > > may not scale well for 64-bit remote processors, and would
> > > require new versions of resource types. Each resource is currently
> > > identified by a 32-bit type field. Introduce the concept of version
> > > for these resource types by overloading this 32-bit type field
> > > into two 16-bit version and type fields with the existing resources
> > > behaving as version 0 thereby providing backward compatibility.
> > >
> > > The version field is passed as an additional argument to each of
> > > the handler functions, and all the existing handlers are updated
> > > accordingly. Each specific handler will be updated on a need basis
> > > when a new version of the resource type is added.
> > >
> >
> > I really would prefer that we add additional types for the new
> > structures, neither side will be compatible with new versions without
> > enhancements to their respective implementations anyways.
>
> OK.
>
> >
> > > An alternate way would be to introduce the new types as completely
> > > new resource types which would require additional customization of
> > > the resource handlers based on the 32-bit or 64-bit mode of a remote
> > > processor, and introduction of an additional mode flag to the rproc
> > > structure.
> > >
> >
> > What would this "mode" indicate? If it's version 0 or 1?
>
> No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
> loading handlers if the resource types need to be segregated accordingly.
>
Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
remote processor need different firmware from your 32-bit processor
anyways, if you want to support the wider resource? And you would pack
your firmware with the appropriate resource types?
Afaict the bit width of your remote processor, busses or memory is
unrelated to the choice of number of bits used to express things in the
resource table.
Regards,
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6] arm64: Expose FAR_EL1 tag bits in sigcontext
From: Eric W. Biederman @ 2020-05-21 19:24 UTC (permalink / raw)
To: Peter Collingbourne
Cc: Andrey Konovalov, Kevin Brodsky, Oleg Nesterov, Kostya Serebryany,
Linux ARM, Catalin Marinas, Vincenzo Frascino, Will Deacon,
Dave Martin, Evgenii Stepanov, Richard Henderson
In-Reply-To: <CAMn1gO6cgcP0O85BA_ire9j1L5zvN4i2JFRXO7R=MScXbmWG1g@mail.gmail.com>
Peter Collingbourne <pcc@google.com> writes:
> On Thu, May 21, 2020 at 5:39 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> Peter Collingbourne <pcc@google.com> writes:
>>
>> > On Wed, May 20, 2020 at 2:26 AM Dave Martin <Dave.Martin@arm.com> wrote:
>> >>
>> >> On Wed, May 20, 2020 at 09:55:03AM +0100, Will Deacon wrote:
>> >> > On Tue, May 19, 2020 at 03:00:12PM -0700, Peter Collingbourne wrote:
>> >> > > On Mon, May 18, 2020 at 2:53 AM Dave Martin <Dave.Martin@arm.com> wrote:
>> >> > > > On Thu, May 14, 2020 at 05:58:21PM -0700, Peter Collingbourne wrote:
>> >> > > > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
>> >> > > > > index baa88dc02e5c..5867f2fdbe64 100644
>> >> > > > > --- a/arch/arm64/kernel/signal.c
>> >> > > > > +++ b/arch/arm64/kernel/signal.c
>> >> > > > > @@ -648,6 +648,7 @@ static int setup_sigframe(struct
>> >> > > > > rt_sigframe_user_layout *user,
>> >> > > > > __put_user_error(ESR_MAGIC, &esr_ctx->head.magic, err);
>> >> > > > > __put_user_error(sizeof(*esr_ctx), &esr_ctx->head.size, err);
>> >> > > > > __put_user_error(current->thread.fault_code,
>> >> > > > > &esr_ctx->esr, err);
>> >> > > > > + current->thread.fault_code = 0;
>> >> > > >
>> >> > > > Perhaps, but we'd need to be careful. For example, can we run out of
>> >> > > > user stack before this and deliver a SIGSEGV, but with the old
>> >> > > > fault_code still set? Then we'd emit the old fault code with the
>> >> > > > new "can't deliver signal" signal, which doesn't make sense.
>> >> > > >
>> >> > > > Stuff may also go wrong with signal prioritisation.
>> >> > > >
>> >> > > > If a higher-priority signal (say SIGINT) comes in after a data abort
>> >> > > > enters the kernel but before the resulting SIGSEGV is dequeued for
>> >> > > > delivery, wouldn't we deliver SIGINT first, with the bogus fault code?
>> >> > > > With your change we'd then have cleared the fault code by the time we
>> >> > > > deliver the SIGSEGV it actually relates to, if I've understood right.
>> >> > > >
>> >> > > > Today, I think we just attach that fault code to every signal that's
>> >> > > > delivered until something overwrites or resets it, which means that
>> >> > > > a signal that needs fault_code gets it, at the expense of attaching
>> >> > > > it to a bunch of other random signals too.
>> >> > > >
>> >> > > >
>> >> > > > Checking the signal number and si_code might help us to know what we
>> >> > > > should be doing with fault_code. We need to have sure userspace can't
>> >> > > > trick us with a non kernel generated signal here. It would also be
>> >> > > > necessary to check how PTRACE_SETSIGINFO interacts with this.
>> >> > >
>> >> > > With these possible interactions in mind I think we should store the
>> >> > > fault code and fault address in kernel_siginfo instead of
>> >> > > thread_struct (and clear these fields when we receive a siginfo from
>> >> > > userspace, i.e. in copy_siginfo_from_user which is used by
>> >> > > ptrace(PTRACE_SETSIGINFO) among other places). That way, the
>> >> > > information is clearly associated with the signal itself and not the
>> >> > > thread, so we don't need to worry about our signal being delivered out
>> >> > > of order.
>> >> >
>> >> > Hmm, I can't see a way to do that that isn't horribly invasive in the core
>> >> > signal code. Can you?
>> >
>> > I think I've come up with a way that doesn't seem to be too invasive.
>> > See patch #1 of the series that I'm about to send out.
>> >
>> >> > But generally, I agree: the per-thread handling of fault_address and
>> >> > fault_code appears to be quite broken in the face of signal prioritisation
>> >> > and signals that don't correspond directly to hardware trap. It would be
>> >> > nice to have some tests for this...
>> >> >
>> >> > If we want to pile on more bodges, perhaps we could stash the signal number
>> >> > to which the fault_{address,code} relate, and then check that at delivery
>> >> > and clear on a match. I hate it.
>> >>
>> >> I agree with Daniel's suggestion in principle, but I was also concerned
>> >> about whether it would be too invasive elsewhere.
>> >>
>> >> Question though: does the core code take special care to make sure that
>> >> a force_sig cannot be outprioritised by a regular signal? If so,
>> >> perhaps we get away with it. I ask this, because the same same issue
>> >> may be hitting other arches otherwise.
>> >
>> > Not as far as I can tell. There does appear to be prioritisation for
>> > synchronous signals [1] but as far as I can tell nothing to
>> > distinguish one of these signals from one with the same signal number
>> > sent from userspace (e.g. via kill(2)).
>>
>> The si_code will differ between signals generated between userspace
>> and signals generated by the kernel.
>>
>> We do allow a little bit of ptrace and sending to yourself to spoof
>> kernel generated signals, for reasons of debugging and process migration
>> where an existing process needs to be reconstructed. But the defenses
>> should be strong enough you can assume that we reliably distinguish
>> between a signal from userspace and a signal from the kernel.
>
> So check for SIGBUS || SIGSEGV and one of the below si_codes, and only
> add the context in that case? Seems fragile to me, but I suppose I
> could live with it.
>
>> I don't fully follow what you are doing but this feels like the
>> kind of case where a new si_code has been defined as well as additional
>> fields in siginfo.
>
> There is no new si_code for this, the information will be exposed for
> several existing si_code types (BUS_ADRERR, BUS_ADRALN, BUS_MCEERR_AR,
> SEGV_ACCERR, SEGV_MAPERR), and possibly others in the future
> (particularly SEGV_MTESERR, which is part of the proposed MTE patch
> set). Note that we already have a union field for BUS_MCEERR_AR, and
> we may want to expose it for the other si_codes that already have
> union fields as well.
>
> That being said, taking a closer look at siginfo, I think we are in
> luck and we might be able to make this work in a reasonable way by
> reusing padding (see below).
>
>> In your patchset I really hate that you were going back to
>> force_sig_info, and filling out struct siginfo by hand. That is an
>> error prone pattern, and I have fixed enough bugs in the kernel to prove
>> that.
>
> To be fair, most of the callers are in helper functions that take
> explicit parameters similar to force_sig_fault et al, and the SIGILL
> one could easily be made that way as well.
>
>> I take exception to the idea that including the full address might break
>> userspace. That means typically means someone has been too lazy to look
>> and see what userspace is doing. When that userspace that might break
>> is the same userspace you are changing the kernel to serve that makes me
>> nervous. AKA the userspace that cares about this signal and how it is
>> represented in siginfo.
>
> It's not a matter of being lazy. This behaviour isn't just an accident
> but has been explicitly documented for years (see the
> tagged-pointers.rst file that I changed: "Non-zero tags are not
> preserved when delivering signals."), so users can reasonably rely on
> it. Furthermore we simply don't have visibility into the majority of
> userspace. For example, there are a lot of closed source Android apps
> out there, and who knows what signal handlers they're installing and
> how they're making use of the si_addr field on e.g. SEGV_MAPERR. We
> can't just change the documented semantics under their feet.
>
> It's also not the same userspace either. The userspace that's
> initially going to be consuming the new fields is in a part of the
> Android system that handles and reports crashes, and that's something
> that we control unlike all the apps.
>
> Finally, the userspace may need to know whether the tag bits were
> actually zero or whether they were just unavailable, otherwise
> userspace could for example produce a misleading crash report. Simply
> having the kernel set the top bits of si_addr wouldn't accomplish that
> due to the kernel's previous behaviour, hence the mask to let
> userspace know which bits are accurate.
>
>> A fix of one instance of SIGILL should not be included with a patch that
>> does something else, and really should come before everything else if
>> possible.
>
> Fair point. I can see if I can split that part out.
>
>> If this information really belongs in struct siginfo (as it sounds like)
>> please actually put the information in siginfo, and let userspace look
>> in siginfo to find it. struct siginfo is a union with plenty of space,
>> and plenty of si_codes.
>>
>> If this applies to multiple cases then it might be trickier but please
>> dig into the details, don't toss things into sigcontext just because
>> you can't figure out a clean design for reporting this.
>
> If we wanted this in siginfo, one idea that I had was to revert commit
> b68a68d3dcc15ebbf23cbe91af1abf57591bd96b and add unsigned char fields
> _addr_top_byte and _addr_top_byte_mask in the padding between
> _addr_lsb and the union (with comments on all the fields of course to
> say when they are filled in). I think that would work since we are
> already clearing padding in siginfo, one nice property of the new
> fields is that the zero values are correct in the case where the
> information isn't being exposed (so old kernels would already have the
> correct behaviour). That would only work on certain architectures
> (i.e. at least alignof(void*) >= 4) so I suppose it could have an
> #ifdef __aarch64__ around it.
Perhaps add a 4th padding member to the union inside of _sigfault, that
adds something like 4 unsigned long's worth of data, and then have your
fields after the union.
Is it quite a bit of work to gather that information from the
instructions that faulted? I am just checking that this work is really
makes sense.
What I really don't understand is how well this problem generalizes to
other architectures to tell if this is something other people need to
solve at some point as well.
Eric
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Suman Anna @ 2020-05-21 19:29 UTC (permalink / raw)
To: Bjorn Andersson
Cc: devicetree, Mathieu Poirier, Loic Pallardy, Lokesh Vutla,
linux-remoteproc, Arnaud Pouliquen, linux-kernel, Clement Leger,
Rob Herring, linux-arm-kernel
In-Reply-To: <20200521192146.GO408178@builder.lan>
On 5/21/20 2:21 PM, Bjorn Andersson wrote:
> On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
>
>> Hi Bjorn,
>>
>> On 5/21/20 12:54 PM, Bjorn Andersson wrote:
>>> On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
>>>
>>>> The current remoteproc core has supported only 32-bit remote
>>>> processors and as such some of the current resource structures
>>>> may not scale well for 64-bit remote processors, and would
>>>> require new versions of resource types. Each resource is currently
>>>> identified by a 32-bit type field. Introduce the concept of version
>>>> for these resource types by overloading this 32-bit type field
>>>> into two 16-bit version and type fields with the existing resources
>>>> behaving as version 0 thereby providing backward compatibility.
>>>>
>>>> The version field is passed as an additional argument to each of
>>>> the handler functions, and all the existing handlers are updated
>>>> accordingly. Each specific handler will be updated on a need basis
>>>> when a new version of the resource type is added.
>>>>
>>>
>>> I really would prefer that we add additional types for the new
>>> structures, neither side will be compatible with new versions without
>>> enhancements to their respective implementations anyways.
>>
>> OK.
>>
>>>
>>>> An alternate way would be to introduce the new types as completely
>>>> new resource types which would require additional customization of
>>>> the resource handlers based on the 32-bit or 64-bit mode of a remote
>>>> processor, and introduction of an additional mode flag to the rproc
>>>> structure.
>>>>
>>>
>>> What would this "mode" indicate? If it's version 0 or 1?
>>
>> No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
>> loading handlers if the resource types need to be segregated accordingly.
>>
>
> Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
> remote processor need different firmware from your 32-bit processor
> anyways, if you want to support the wider resource? And you would pack
> your firmware with the appropriate resource types?
Yes, that's correct.
>
> Afaict the bit width of your remote processor, busses or memory is
> unrelated to the choice of number of bits used to express things in the
> resource table.
I would have to add the new resource type to the loading_handlers right,
so it is a question of whether we want to impose any restrictions in
remoteproc core or not from supporting a certain resource type (eg: I
don't expect RSC_TRACE entries on 64-bit processors).
regards
Suman
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V8, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver
From: Tomasz Figa @ 2020-05-21 19:32 UTC (permalink / raw)
To: Dongchun Zhu
Cc: mark.rutland, drinkcat, andriy.shevchenko, srv_heupstream,
devicetree, linus.walleij, shengnan.wang, louis.kuo, bgolaszewski,
sj.huang, robh+dt, linux-mediatek, sakari.ailus, matthias.bgg,
bingbu.cao, mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20200509080627.23222-3-dongchun.zhu@mediatek.com>
Hi Dongchun,
On Sat, May 09, 2020 at 04:06:27PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for OV02A10 image sensor.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 13 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/ov02a10.c | 1094 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1109 insertions(+)
> create mode 100644 drivers/media/i2c/ov02a10.c
>
Thank you for the patch. Please see my comments inline.
[snip]
> +struct ov02a10 {
> + u32 eclk_freq;
> + u32 mipi_clock_tx_speed;
> +
> + struct clk *eclk;
> + struct gpio_desc *pd_gpio;
> + struct gpio_desc *n_rst_gpio;
> + struct regulator_bulk_data supplies[OV02A10_NUM_SUPPLIES];
> +
> + bool streaming;
> + bool upside_down;
> + bool mipi_clock_tx_speed_select_enable;
> + bool mipi_clock_hs_mode_enable;
> +
> + /*
> + * Serialize control access, get/set format, get selection
> + * and start streaming.
> + */
> + struct mutex mutex;
> + struct v4l2_subdev subdev;
> + struct media_pad pad;
> + struct v4l2_ctrl *anal_gain;
> + struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *hblank;
> + struct v4l2_ctrl *vblank;
> + struct v4l2_ctrl *test_pattern;
> + struct v4l2_mbus_framefmt fmt;
nit: Remove the tabs between types and names and use single spaces. As you
can see above, even tabs don't guarantee equal alignment. And they actually
make adding fields more difficult, because if a longer field is added, the
alignment breaks.
[snip]
> +static int ov02a10_read_smbus(struct ov02a10 *ov02a10, unsigned char reg,
> + unsigned char *val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(client, reg);
> +
nit: Unnecessary blank line.
> + if (ret < 0)
> + return ret;
> +
> + *val = (unsigned char)ret;
> +
> + return 0;
> +}
[snip]
> +static int __maybe_unused ov02a10_power_on(struct device *dev)
How is it possible that this function may be unused? If this driver allows
disabling runtime PM, then there is still a need to configure the clock,
regulator and GPIO. If not, there shouldn't be a case where this function
is unused.
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> + int ret;
> +
> + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> + gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
> +
> + ret = clk_prepare_enable(ov02a10->eclk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable eclk\n");
> + return ret;
> + }
> +
> + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable regulators\n");
> + goto disable_clk;
> + }
> + usleep_range(5000, 6000);
> +
> + gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
This is a "powerdown" GPIO. It must be set to 0 if the sensor is to be
powered on.
> + usleep_range(5000, 6000);
> +
> + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 1);
> + usleep_range(5000, 6000);
> +
> + ret = ov02a10_check_sensor_id(ov02a10);
> + if (ret)
> + goto disable_regulator;
> +
> + return 0;
> +
> +disable_regulator:
> + regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> +disable_clk:
> + clk_disable_unprepare(ov02a10->eclk);
> +
> + return ret;
> +}
> +
> +static int __maybe_unused ov02a10_power_off(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +
> + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> + clk_disable_unprepare(ov02a10->eclk);
> + gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
Similar comment as above. To power off the sensor, the "powerdown" GPIO
needs to be active, i.e. 1.
[snip]
> +/*
> + * ov02a10_set_exposure - Function called when setting exposure time
> + * @priv: Pointer to device structure
> + * @val: Variable for exposure time, in the unit of micro-second
> + *
> + * Set exposure time based on input value.
> + *
> + * Return: 0 on success
> + */
> +static int ov02a10_set_exposure(struct ov02a10 *ov02a10, int val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
How does this page switch work? According to the documentation I have, the
register allows selecting between a few different pages. However, there
should be two page pointers - one for the AP and the other for the sensor,
so that when the AP is programming page X, the sensor can have consistent
settings from page Y. But here we only set one register and always with
page 1.
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_H,
> + val >> OV02A10_EXP_SHIFT);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_L, val);
> + if (ret < 0)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
This patch defines REG_GLOBAL_EFFECTIVE to 0x01. I don't see such register
mentioned in the documentation.
> +}
> +
> +static int ov02a10_set_gain(struct ov02a10 *ov02a10, int val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN, val);
> + if (ret < 0)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
> +}
> +
> +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + u32 vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE;
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> + vts >> OV02A10_VTS_SHIFT);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts);
> + if (ret < 0)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
> +}
> +
> +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + if (pattern)
> + pattern = OV02A10_TEST_PATTERN_ENABLE;
Is this necessary? Our control can be 0 for disabled and 1 for color bars.
The latter is the same as the above macro.
[snip]
> +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + const struct ov02a10_mode *mode;
> + struct v4l2_ctrl_handler *handler;
> + struct v4l2_ctrl *ctrl;
> + u64 exposure_max;
> + u32 pixel_rate, h_blank;
> + int ret;
> +
> + handler = &ov02a10->ctrl_handler;
> + mode = ov02a10->cur_mode;
> + ret = v4l2_ctrl_handler_init(handler, 7);
> + if (ret)
> + return ret;
> +
> + handler->lock = &ov02a10->mutex;
> +
> + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0,
> + link_freq_menu_items);
> + if (ctrl)
> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + pixel_rate = to_pixel_rate(0);
> + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1,
> + pixel_rate);
> +
> + h_blank = mode->hts_def - mode->width;
> + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK,
> + h_blank, h_blank, 1, h_blank);
> + if (ov02a10->hblank)
> + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
Do we need to hold a pointer to this control? We don't seem to ever access
it anywhere else in the driver.
> + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_VBLANK, mode->vts_def -
> + mode->height,
> + OV02A10_VTS_MAX - mode->height, 1,
> + mode->vts_def - mode->height);
> +
Ditto.
> + exposure_max = mode->vts_def - 4;
> + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_EXPOSURE,
> + OV02A10_EXPOSURE_MIN,
> + exposure_max,
> + OV02A10_EXPOSURE_STEP,
> + mode->exp_def);
> +
> + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_ANALOGUE_GAIN,
> + OV02A10_GAIN_MIN,
> + OV02A10_GAIN_MAX,
> + OV02A10_GAIN_STEP,
> + OV02A10_GAIN_DEFAULT);
Ditto.
> +
> + ov02a10->test_pattern =
> + v4l2_ctrl_new_std_menu_items(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(ov02a10_test_pattern_menu) -
> + 1, 0, 0,
> + ov02a10_test_pattern_menu);
Ditto.
[snip]
> + ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH);
> + if (IS_ERR(ov02a10->pd_gpio)) {
> + ret = PTR_ERR(ov02a10->pd_gpio);
> + dev_err(dev, "failed to get powerdown-gpios %d\n", ret);
> + return ret;
> + }
> +
> + ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
I've asked a question about the right convention to define active low pins,
but I don't remember seeing an answer. We need to clarify this to avoid
confusion. Especially since the current binding documentation doesn't
mention anything about the sensor being active low. Let me also post a
comment to the binding patch.
> + if (IS_ERR(ov02a10->n_rst_gpio)) {
> + ret = PTR_ERR(ov02a10->n_rst_gpio);
> + dev_err(dev, "failed to get reset-gpios %d\n", ret);
> + return ret;
> + }
> +
> + for (i = 0; i < OV02A10_NUM_SUPPLIES; i++)
> + ov02a10->supplies[i].supply = ov02a10_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(dev, OV02A10_NUM_SUPPLIES,
> + ov02a10->supplies);
> + if (ret) {
> + dev_err(dev, "failed to get regulators\n");
> + return ret;
> + }
> +
> + mutex_init(&ov02a10->mutex);
> + ov02a10->cur_mode = &supported_modes[0];
> + ret = ov02a10_initialize_controls(ov02a10);
> + if (ret) {
> + dev_err(dev, "failed to initialize controls\n");
> + goto err_destroy_mutex;
> + }
> +
> + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops;
> + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad);
> + if (ret < 0) {
> + dev_err(dev, "failed to init entity pads: %d", ret);
> + goto err_free_handler;
> + }
> +
> + pm_runtime_enable(dev);
> + if (!pm_runtime_enabled(dev)) {
> + ret = ov02a10_power_on(dev);
> + if (ret < 0) {
> + dev_err(dev, "failed to power on: %d\n", ret);
> + goto err_clean_entity;
> + }
> + }
> +
> + ret = v4l2_async_register_subdev(&ov02a10->subdev);
> + if (ret) {
> + dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> + goto err_clean_entity;
> + }
> +
> + return 0;
> +
> +err_clean_entity:
Need to power off if !pm_runtime_enabled().
Best regards,
Tomasz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 04/15] PCI: brcmstb: Add compatibily of other chips
From: Jim Quinlan @ 2020-05-21 19:35 UTC (permalink / raw)
To: Nicolas Saenz Julienne
Cc: Rob Herring, Lorenzo Pieralisi,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list,
Florian Fainelli, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Bjorn Helgaas,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE
In-Reply-To: <5a52e39ce99214877e83104b8ea9f95c0d5b4e90.camel@suse.de>
On Wed, May 20, 2020 at 7:51 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Jim,
>
> On Tue, 2020-05-19 at 16:34 -0400, Jim Quinlan wrote:
> > From: Jim Quinlan <jquinlan@broadcom.com>
> >
> > Add in compatibility strings and code for three Broadcom STB chips.
> > Some of the register locations, shifts, and masks are different
> > for certain chips, requiring the use of different constants based
> > on of_id.
> >
> > We would like to add the following at this time to the match list
> > but we need to wait until the end of this patchset so that
> > everything works.
> >
> > { .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
> > { .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
> > { .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
> > { .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
> >
> > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
> > ---
> > drivers/pci/controller/pcie-brcmstb.c | 103 +++++++++++++++++++++++---
> > 1 file changed, 91 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c
> > b/drivers/pci/controller/pcie-brcmstb.c
> > index 73020b4ff090..c1cf4ea7d3d9 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -120,9 +120,8 @@
> > #define PCIE_EXT_SLOT_SHIFT 15
> > #define PCIE_EXT_FUNC_SHIFT 12
> >
> > -#define PCIE_RGR1_SW_INIT_1 0x9210
> > #define PCIE_RGR1_SW_INIT_1_PERST_MASK 0x1
> > -#define PCIE_RGR1_SW_INIT_1_INIT_MASK 0x2
> > +#define PCIE_RGR1_SW_INIT_1_PERST_SHIFT 0x0
> >
> > /* PCIe parameters */
> > #define BRCM_NUM_PCIE_OUT_WINS 0x4
> > @@ -152,6 +151,69 @@
> > #define SSC_STATUS_SSC_MASK 0x400
> > #define SSC_STATUS_PLL_LOCK_MASK 0x800
> >
> > +#define IDX_ADDR(pcie) \
> > + (pcie->reg_offsets[EXT_CFG_INDEX])
> > +#define DATA_ADDR(pcie) \
> > + (pcie->reg_offsets[EXT_CFG_DATA])
> > +#define PCIE_RGR1_SW_INIT_1(pcie) \
> > + (pcie->reg_offsets[RGR1_SW_INIT_1])
> > +
> > +enum {
> > + RGR1_SW_INIT_1,
> > + EXT_CFG_INDEX,
> > + EXT_CFG_DATA,
> > +};
> > +
> > +enum {
> > + RGR1_SW_INIT_1_INIT_MASK,
> > + RGR1_SW_INIT_1_INIT_SHIFT,
> > +};
> > +
> > +enum pcie_type {
> > + GENERIC,
> > + BCM7278,
> > +};
> > +
> > +struct pcie_cfg_data {
> > + const int *reg_field_info;
> > + const int *offsets;
> > + const enum pcie_type type;
> > +};
> > +
> > +static const int pcie_reg_field_info[] = {
> > + [RGR1_SW_INIT_1_INIT_MASK] = 0x2,
> > + [RGR1_SW_INIT_1_INIT_SHIFT] = 0x1,
> > +};
> > +
> > +static const int pcie_reg_field_info_bcm7278[] = {
> > + [RGR1_SW_INIT_1_INIT_MASK] = 0x1,
> > + [RGR1_SW_INIT_1_INIT_SHIFT] = 0x0,
> > +};
> > +
> > +static const int pcie_offsets[] = {
> > + [RGR1_SW_INIT_1] = 0x9210,
> > + [EXT_CFG_INDEX] = 0x9000,
> > + [EXT_CFG_DATA] = 0x9004,
> > +};
> > +
> > +static const struct pcie_cfg_data generic_cfg = {
> > + .reg_field_info = pcie_reg_field_info,
> > + .offsets = pcie_offsets,
> > + .type = GENERIC,
> > +};
> > +
> > +static const int pcie_offset_bcm7278[] = {
> > + [RGR1_SW_INIT_1] = 0xc010,
> > + [EXT_CFG_INDEX] = 0x9000,
> > + [EXT_CFG_DATA] = 0x9004,
> > +};
> > +
> > +static const struct pcie_cfg_data bcm7278_cfg = {
> > + .reg_field_info = pcie_reg_field_info_bcm7278,
> > + .offsets = pcie_offset_bcm7278,
> > + .type = BCM7278,
> > +};
>
> It's not essential, but if v2 is due I'd suggest factoring out the bcm2728
> specific structures above, and moving them to patch #15. This will keep a
> clearer division between the patch introducing the infrastructure and the one
> adding the support for a new device.
>
> > +
> > struct brcm_msi {
> > struct device *dev;
> > void __iomem *base;
> > @@ -176,6 +238,9 @@ struct brcm_pcie {
> > int gen;
> > u64 msi_target_addr;
> > struct brcm_msi *msi;
> > + const int *reg_offsets;
> > + const int *reg_field_info;
> > + enum pcie_type type;
> > };
> >
> > /*
> > @@ -602,20 +667,21 @@ static struct pci_ops brcm_pcie_ops = {
> >
> > static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32
> > val)
> > {
> > - u32 tmp;
> > + u32 tmp, mask = pcie->reg_field_info[RGR1_SW_INIT_1_INIT_MASK];
> > + u32 shift = pcie->reg_field_info[RGR1_SW_INIT_1_INIT_SHIFT];
>
> I don't think you need shift here, IIUC u32p_replace_bits() will take care of
> all the masking and shifting internally, moreover, you'd be able to drop the
> shift entry from reg_field_info.
I believe that u32p_replace_bits requires at least one of the value or
mask to be compile time constants to work and we don't have that here.
Regards,
Jim
>
> > - tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
> > - u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_INIT_MASK);
> > - writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);
> > + tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > + tmp = (tmp & ~mask) | ((val << shift) & mask);
> > + writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > }
>
> Regards,
> Nicolas
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V8, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Tomasz Figa @ 2020-05-21 19:35 UTC (permalink / raw)
To: Rob Herring
Cc: mark.rutland, devicetree, andriy.shevchenko, srv_heupstream,
linus.walleij, shengnan.wang, louis.kuo, bgolaszewski, sj.huang,
drinkcat, linux-mediatek, Dongchun Zhu, sakari.ailus,
matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20200511160207.GA32476@bogus>
Hi Rob,
On Mon, May 11, 2020 at 11:02:07AM -0500, Rob Herring wrote:
> On Sat, May 09, 2020 at 04:06:26PM +0800, Dongchun Zhu wrote:
> > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> > .../bindings/media/i2c/ovti,ov02a10.yaml | 184 +++++++++++++++++++++
> > MAINTAINERS | 7 +
> > 2 files changed, 191 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > new file mode 100644
> > index 0000000..5468d1b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > @@ -0,0 +1,184 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (c) 2020 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
> > +
> > +maintainers:
> > + - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > +
> > +description: |-
> > + The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
> > + image sensor, which is the latest production derived from Omnivision's CMOS
> > + image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
> > + @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
> > + sensor output is available via CSI-2 serial data output.
> > +
> > +properties:
> > + compatible:
> > + const: ovti,ov02a10
> > +
> > + reg:
> > + description: I2C device address
>
> Drop this. Nothing specific to this device.
>
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: top mux camtg clock
> > + - description: devider clock
>
> typo
>
> > +
> > + clock-names:
> > + items:
> > + - const: eclk
> > + - const: freq_mux
> > +
> > + clock-frequency:
> > + description:
> > + Frequency of the eclk clock in Hertz.
> > +
> > + dovdd-supply:
> > + description:
> > + Definition of the regulator used as interface power supply.
> > +
> > + avdd-supply:
> > + description:
> > + Definition of the regulator used as analog power supply.
> > +
> > + dvdd-supply:
> > + description:
> > + Definition of the regulator used as digital power supply.
> > +
> > + powerdown-gpios:
> > + maxItems: 1
> > +
> > + reset-gpios:
> > + maxItems: 1
I asked a question about defining GPIO polarities some time ago, but I
guess it slipped through.
The chip documentation calls the reset pin as "RST_PAD (low level
reset)". Where should the inversion be handled, in the driver or here,
by having the DT include a necessary flag in the specifier?
Best regards,
Tomasz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 04/19] spi: spi-mem: allow specifying a command's extension
From: Pratyush Yadav @ 2020-05-21 19:41 UTC (permalink / raw)
To: Boris Brezillon
Cc: Alexandre Belloni, Mason Yang, Vignesh Raghavendra, Tudor Ambarus,
Richard Weinberger, Sekhar Nori, linux-kernel, Ludovic Desroches,
Mark Brown, linux-mtd, Miquel Raynal, Matthias Brugger,
linux-mediatek, linux-spi, linux-arm-kernel
In-Reply-To: <20200521202256.5816eb32@collabora.com>
On 21/05/20 08:22PM, Boris Brezillon wrote:
> On Wed, 20 May 2020 22:00:38 +0530
> Pratyush Yadav <p.yadav@ti.com> wrote:
>
> > In xSPI mode, flashes expect 2-byte opcodes. The second byte is called
> > the "command extension". There can be 3 types of extensions in xSPI:
> > repeat, invert, and hex. When the extension type is "repeat", the same
> > opcode is sent twice. When it is "invert", the second byte is the
> > inverse of the opcode. When it is "hex" an additional opcode byte based
> > is sent with the command whose value can be anything.
> >
> > So, make opcode a 16-bit value and add a 'nbytes', similar to how
> > multiple address widths are handled.
> >
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> > include/linux/spi/spi-mem.h | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> > index e3dcb956bf61..731bb64c6ba6 100644
> > --- a/include/linux/spi/spi-mem.h
> > +++ b/include/linux/spi/spi-mem.h
> > @@ -69,6 +69,8 @@ enum spi_mem_data_dir {
> >
> > /**
> > * struct spi_mem_op - describes a SPI memory operation
> > + * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
> > + * sent MSB-first.
> > * @cmd.buswidth: number of IO lines used to transmit the command
> > * @cmd.opcode: operation opcode
> > * @cmd.dtr: whether the command opcode should be sent in DTR mode or not
> > @@ -94,9 +96,10 @@ enum spi_mem_data_dir {
> > */
> > struct spi_mem_op {
> > struct {
> > + u8 nbytes;
> > u8 buswidth;
> > u8 dtr : 1;
> > - u8 opcode;
> > + u16 opcode;
> > } cmd;
> >
> > struct {
>
> As mentioned in one of my previous review, you should patch the mxic
> driver before extending the opcode field:
IIUC, this patchset doesn't break original functionality of the driver.
It will work like before with 1-byte opcodes. So I don't think it is the
responsibility of this patchset to enhance the driver. It didn't work
before with 2-byte opcodes, it won't work now. IMO this should be a
separate, independent change.
> --->8---
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index 69491f3a515d..c3f4136a7c1d 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -356,6 +356,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
> int nio = 1, i, ret;
> u32 ss_ctrl;
> u8 addr[8];
> + u8 cmd[2];
>
> ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
> if (ret)
> @@ -393,7 +394,10 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
> writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
> mxic->regs + HC_CFG);
>
> - ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
> + for (i = 0; i < op->cmd.nbytes; i++)
> + cmd[i] = op->cmd.opcode >> (8 * (op->cmd.nbytes - i - 1));
> +
> + ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
> if (ret)
> goto out;
>
--
Regards,
Pratyush Yadav
Texas Instruments India
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/4] remoteproc: add support for a new 64-bit trace version
From: Suman Anna @ 2020-05-21 19:42 UTC (permalink / raw)
To: Bjorn Andersson
Cc: devicetree, Mathieu Poirier, Loic Pallardy, Lokesh Vutla,
linux-remoteproc, Arnaud Pouliquen, linux-kernel, Clement Leger,
Rob Herring, linux-arm-kernel
In-Reply-To: <20200521180417.GJ408178@builder.lan>
Hi Bjorn,
On 5/21/20 1:04 PM, Bjorn Andersson wrote:
> On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote:
>
>> Introduce a new trace entry resource structure that accommodates
>> a 64-bit device address to support 64-bit processors. This is to
>> be used using an overloaded version value of 1 in the upper 32-bits
>> of the previous resource type field. The new resource still uses
>> 32-bits for the length field (followed by a 32-bit reserved field,
>> so can be updated in the future), which is a sufficiently large
>> trace buffer size. A 32-bit padding field also had to be added
>> to align the device address on a 64-bit boundary, and match the
>> usage on the firmware side.
>>
>> The remoteproc debugfs logic also has been adjusted accordingly.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 40 ++++++++++++++++++++-----
>> drivers/remoteproc/remoteproc_debugfs.c | 37 ++++++++++++++++++-----
>> include/linux/remoteproc.h | 26 ++++++++++++++++
>> 3 files changed, 87 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 53bc37c508c6..b9a097990862 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -609,21 +609,45 @@ void rproc_vdev_release(struct kref *ref)
>> *
>> * Returns 0 on success, or an appropriate error code otherwise
>> */
>> -static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>> +static int rproc_handle_trace(struct rproc *rproc, void *rsc,
>> int offset, int avail, u16 ver)
>> {
>> struct rproc_debug_trace *trace;
>> struct device *dev = &rproc->dev;
>> + struct fw_rsc_trace *rsc1;
>> + struct fw_rsc_trace2 *rsc2;
>> char name[15];
>> + size_t rsc_size;
>> + u32 reserved;
>> + u64 da;
>> + u32 len;
>> +
>> + if (!ver) {
>
> This looks like a switch to me, but I also do think this looks rather
> crude, if you spin off the tail of this function and call it from a
> rproc_handle_trace() and rproc_handle_trace64() I believe this would be
> cleaner.
Yeah, ok. Will refactor for this in v2.
>
>> + rsc1 = (struct fw_rsc_trace *)rsc;
>> + rsc_size = sizeof(*rsc1);
>> + reserved = rsc1->reserved;
>> + da = rsc1->da;
>> + len = rsc1->len;
>> + } else if (ver == 1) {
>> + rsc2 = (struct fw_rsc_trace2 *)rsc;
>> + rsc_size = sizeof(*rsc2);
>> + reserved = rsc2->reserved;
>> + da = rsc2->da;
>> + len = rsc2->len;
>> + } else {
>> + dev_err(dev, "unsupported trace rsc version %d\n", ver);
>
> If we use "type" to describe your 64-bit-da-trace then this sanity check
> would have been taken care of by the core.
>
>> + return -EINVAL;
>> + }
>>
>> - if (sizeof(*rsc) > avail) {
>> + if (rsc_size > avail) {
>> dev_err(dev, "trace rsc is truncated\n");
>> return -EINVAL;
>> }
>>
>> /* make sure reserved bytes are zeroes */
>> - if (rsc->reserved) {
>> - dev_err(dev, "trace rsc has non zero reserved bytes\n");
>> + if (reserved) {
>> + dev_err(dev, "trace rsc has non zero reserved bytes, value = 0x%x\n",
>> + reserved);
>> return -EINVAL;
>> }
>>
>> @@ -632,8 +656,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>> return -ENOMEM;
>>
>> /* set the trace buffer dma properties */
>> - trace->trace_mem.len = rsc->len;
>> - trace->trace_mem.da = rsc->da;
>> + trace->trace_mem.len = len;
>> + trace->trace_mem.da = da;
>>
>> /* set pointer on rproc device */
>> trace->rproc = rproc;
>> @@ -652,8 +676,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>>
>> rproc->num_traces++;
>>
>> - dev_dbg(dev, "%s added: da 0x%x, len 0x%x\n",
>> - name, rsc->da, rsc->len);
>> + dev_dbg(dev, "%s added: da 0x%llx, len 0x%x\n",
>> + name, da, len);
>>
>> return 0;
>> }
>> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
>> index 3560eed7a360..ff43736db45a 100644
>> --- a/drivers/remoteproc/remoteproc_debugfs.c
>> +++ b/drivers/remoteproc/remoteproc_debugfs.c
>> @@ -192,7 +192,8 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>> struct resource_table *table = rproc->table_ptr;
>> struct fw_rsc_carveout *c;
>> struct fw_rsc_devmem *d;
>> - struct fw_rsc_trace *t;
>> + struct fw_rsc_trace *t1;
>> + struct fw_rsc_trace2 *t2;
>> struct fw_rsc_vdev *v;
>> int i, j;
>>
>> @@ -205,6 +206,7 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>> int offset = table->offset[i];
>> struct fw_rsc_hdr *hdr = (void *)table + offset;
>> void *rsc = (void *)hdr + sizeof(*hdr);
>> + u16 ver = hdr->st.v;
>>
>> switch (hdr->st.t) {
>> case RSC_CARVEOUT:
>> @@ -230,13 +232,32 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>> seq_printf(seq, " Name %s\n\n", d->name);
>> break;
>> case RSC_TRACE:
>> - t = rsc;
>> - seq_printf(seq, "Entry %d is of type %s\n",
>> - i, types[hdr->st.t]);
>> - seq_printf(seq, " Device Address 0x%x\n", t->da);
>> - seq_printf(seq, " Length 0x%x Bytes\n", t->len);
>> - seq_printf(seq, " Reserved (should be zero) [%d]\n", t->reserved);
>> - seq_printf(seq, " Name %s\n\n", t->name);
>> + if (ver == 0) {
>
> Again, this is a switch, here in a switch. Just defining a new
> RSC_TRACE64 type would reduce the amount of code here...
OK.
>
>> + t1 = rsc;
>> + seq_printf(seq, "Entry %d is version %d of type %s\n",
>> + i, ver, types[hdr->st.t]);
>> + seq_printf(seq, " Device Address 0x%x\n",
>> + t1->da);
>> + seq_printf(seq, " Length 0x%x Bytes\n",
>> + t1->len);
>> + seq_printf(seq, " Reserved (should be zero) [%d]\n",
>> + t1->reserved);
>> + seq_printf(seq, " Name %s\n\n", t1->name);
>> + } else if (ver == 1) {
>> + t2 = rsc;
>> + seq_printf(seq, "Entry %d is version %d of type %s\n",
>> + i, ver, types[hdr->st.t]);
>> + seq_printf(seq, " Device Address 0x%llx\n",
>> + t2->da);
>> + seq_printf(seq, " Length 0x%x Bytes\n",
>> + t2->len);
>> + seq_printf(seq, " Reserved (should be zero) [%d]\n",
>> + t2->reserved);
>> + seq_printf(seq, " Name %s\n\n", t2->name);
>> + } else {
>> + seq_printf(seq, "Entry %d is an unsupported version %d of type %s\n",
>> + i, ver, types[hdr->st.t]);
>> + }
>> break;
>> case RSC_VDEV:
>> v = rsc;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 526d3cb45e37..3b3bea42f8b1 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -243,6 +243,32 @@ struct fw_rsc_trace {
>> u8 name[32];
>> } __packed;
>>
>> +/**
>> + * struct fw_rsc_trace2 - trace buffer declaration supporting 64-bits
>> + * @padding: initial padding after type field for aligned 64-bit access
>> + * @da: device address (64-bit)
>> + * @len: length (in bytes)
>> + * @reserved: reserved (must be zero)
>> + * @name: human-readable name of the trace buffer
>> + *
>> + * This resource entry is an enhanced version of the fw_rsc_trace resourec entry
>> + * and the provides equivalent functionality but designed for 64-bit remote
>> + * processors.
>> + *
>> + * @da specifies the device address of the buffer, @len specifies
>> + * its size, and @name may contain a human readable name of the trace buffer.
>> + *
>> + * After booting the remote processor, the trace buffers are exposed to the
>> + * user via debugfs entries (called trace0, trace1, etc..).
>> + */
>> +struct fw_rsc_trace2 {
>
> Sounds more like fw_rsc_trace64 to me - in particular since the version
> of trace2 is 1...
Yeah, will rename this.
>
>> + u32 padding;
>> + u64 da;
>> + u32 len;
>> + u32 reserved;
>
> What's the purpose of this reserved field?
Partly to make sure the entire resource is aligned on an 8-byte, and
partly copied over from fw_rsc_trace entry. I guess 32-bits is already
large enough of a size for trace entries irrespective of 32-bit or
64-bit traces, so I doubt if we want to make the len field also a u64.
regards
Suman
>
> Regards,
> Bjorn
>
>> + u8 name[32];
>> +} __packed;
>> +
>> /**
>> * struct fw_rsc_vdev_vring - vring descriptor entry
>> * @da: device address
>> --
>> 2.23.0
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Bjorn Andersson @ 2020-05-21 19:41 UTC (permalink / raw)
To: Suman Anna
Cc: devicetree, Mathieu Poirier, Loic Pallardy, Lokesh Vutla,
linux-remoteproc, Arnaud Pouliquen, linux-kernel, Clement Leger,
Rob Herring, linux-arm-kernel
In-Reply-To: <57ae5678-fd0a-07a8-6165-a2cf7ccdef88@ti.com>
On Thu 21 May 12:29 PDT 2020, Suman Anna wrote:
> On 5/21/20 2:21 PM, Bjorn Andersson wrote:
> > On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
> >
> > > Hi Bjorn,
> > >
> > > On 5/21/20 12:54 PM, Bjorn Andersson wrote:
> > > > On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
> > > >
> > > > > The current remoteproc core has supported only 32-bit remote
> > > > > processors and as such some of the current resource structures
> > > > > may not scale well for 64-bit remote processors, and would
> > > > > require new versions of resource types. Each resource is currently
> > > > > identified by a 32-bit type field. Introduce the concept of version
> > > > > for these resource types by overloading this 32-bit type field
> > > > > into two 16-bit version and type fields with the existing resources
> > > > > behaving as version 0 thereby providing backward compatibility.
> > > > >
> > > > > The version field is passed as an additional argument to each of
> > > > > the handler functions, and all the existing handlers are updated
> > > > > accordingly. Each specific handler will be updated on a need basis
> > > > > when a new version of the resource type is added.
> > > > >
> > > >
> > > > I really would prefer that we add additional types for the new
> > > > structures, neither side will be compatible with new versions without
> > > > enhancements to their respective implementations anyways.
> > >
> > > OK.
> > >
> > > >
> > > > > An alternate way would be to introduce the new types as completely
> > > > > new resource types which would require additional customization of
> > > > > the resource handlers based on the 32-bit or 64-bit mode of a remote
> > > > > processor, and introduction of an additional mode flag to the rproc
> > > > > structure.
> > > > >
> > > >
> > > > What would this "mode" indicate? If it's version 0 or 1?
> > >
> > > No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
> > > loading handlers if the resource types need to be segregated accordingly.
> > >
> >
> > Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
> > remote processor need different firmware from your 32-bit processor
> > anyways, if you want to support the wider resource? And you would pack
> > your firmware with the appropriate resource types?
>
> Yes, that's correct.
>
> >
> > Afaict the bit width of your remote processor, busses or memory is
> > unrelated to the choice of number of bits used to express things in the
> > resource table.
>
> I would have to add the new resource type to the loading_handlers right, so
> it is a question of whether we want to impose any restrictions in remoteproc
> core or not from supporting a certain resource type (eg: I don't expect
> RSC_TRACE entries on 64-bit processors).
>
Right, but either you add support for new resource types to the
loading_handlers, or you add the version checks within each handler,
either way you will have to do some work to be compatible with new
versions.
Regarding what resources would be fit for a 64-bit processor probably
relates to many things, in particular the question of what we actually
mean when we say that a coprocessor is 64-bit. So I don't really see a
need for the remoteproc core to prevent someone to design their
system/firmware to have a 64-bit CPU being passed 32-bit addresses.
Regards,
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
From: Tomasz Figa @ 2020-05-21 19:51 UTC (permalink / raw)
To: Dongchun Zhu
Cc: mark.rutland, drinkcat, andriy.shevchenko, srv_heupstream,
devicetree, linus.walleij, shengnan.wang, louis.kuo, bgolaszewski,
sj.huang, robh+dt, linux-mediatek, sakari.ailus, matthias.bgg,
bingbu.cao, mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20200518132731.20855-3-dongchun.zhu@mediatek.com>
Hi Dongchun, Sakari,
On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
> control to set the desired focus via IIC serial interface.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 13 ++
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/dw9768.c | 515 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 530 insertions(+)
> create mode 100644 drivers/media/i2c/dw9768.c
[snip]
> +/*
> + * DW9768_AAC_PRESC_REG & DW9768_AAC_TIME_REG determine VCM operation time.
> + * If DW9768_AAC_PRESC_REG is 0x41, and DW9768_AAC_TIME_REG is 0x39, VCM mode
> + * would be AAC3, Operation Time would be 0.70xTvib, that is 8.40ms.
> + */
> +#define DW9768_MOVE_DELAY_US 8400
> +#define DW9768_STABLE_TIME_US 20000
These times are only valid with the specific settings mentioned in the
comment. If one sets different settings in DT, the driver would apply
incorrect delays. Rather than hardcoded, they should be computed based
on the configured values.
That said, I wonder if we're not digging too deep now. Sakari, do you
think we could take a step back, remove the optional DT properties and
just support the fixed values for now, so that we can get a basic driver
upstream first without doubling the effort?
> +
> +static const char * const dw9768_supply_names[] = {
> + "vin", /* I2C I/O interface power */
> + "vdd", /* VCM power */
> +};
> +
> +/* dw9768 device structure */
> +struct dw9768 {
> + struct regulator_bulk_data supplies[ARRAY_SIZE(dw9768_supply_names)];
> + struct v4l2_ctrl_handler ctrls;
> + struct v4l2_ctrl *focus;
> + struct v4l2_subdev sd;
> +
> + u32 aac_mode;
> + u32 aac_timing;
> + u32 clock_dividing_rate;
> + bool aac_mode_control_enable;
> + bool aact_cnt_select_enable;
> + bool clock_dividing_rate_select_enable;
nit: Separate types from names with just 1 space.
> +};
> +
> +static inline struct dw9768 *sd_to_dw9768(struct v4l2_subdev *subdev)
> +{
> + return container_of(subdev, struct dw9768, sd);
> +}
> +
> +struct regval_list {
> + u8 reg_num;
> + u8 value;
> +};
> +
> +static int dw9768_read_smbus(struct dw9768 *dw9768, unsigned char reg,
> + unsigned char *val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(client, reg);
> +
> + if (ret < 0)
> + return ret;
> +
> + *val = (unsigned char)ret;
> +
> + return 0;
> +}
Why do we need this function? Couldn't we just call
i2c_smbus_read_byte_data() directly?
[snip]
> +static int dw9768_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct dw9768 *dw9768;
> + unsigned int aac_mode_select;
> + unsigned int aac_timing_select;
> + unsigned int clock_dividing_rate_select;
> + unsigned int i;
> + int ret;
> +
> + dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> + if (!dw9768)
> + return -ENOMEM;
> +
> + v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> + dw9768->aac_mode_control_enable = false;
> + dw9768->aact_cnt_select_enable = false;
> + dw9768->clock_dividing_rate_select_enable = false;
devm_kzalloc() initializes the memory to zero, so no need to set anything
to false explicitly.
> +
> + /* Optional indication of AAC mode select */
> + ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
> + &aac_mode_select);
> +
> + if (!ret) {
> + dw9768->aac_mode_control_enable = true;
> + dw9768->aac_mode = aac_mode_select;
How about making aac_mode a signed int and assigning -1 by
default? Then we don't need two separate fields in the struct.
> + }
> +
> + /* Optional indication of VCM internal clock dividing rate select */
> + ret = fwnode_property_read_u32(dev_fwnode(dev),
> + "dongwoon,clock-dividing-rate",
> + &clock_dividing_rate_select);
> +
> + if (!ret) {
> + dw9768->clock_dividing_rate_select_enable = true;
> + dw9768->clock_dividing_rate = clock_dividing_rate_select;
Ditto.
> + }
> +
> + /* Optional indication of AAC Timing */
> + ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
> + &aac_timing_select);
> +
> + if (!ret) {
> + dw9768->aact_cnt_select_enable = true;
> + dw9768->aac_timing = aac_timing_select;
Ditto.
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
> + dw9768->supplies[i].supply = dw9768_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
> + dw9768->supplies);
> + if (ret) {
> + dev_err(dev, "failed to get regulators\n");
> + return ret;
> + }
> +
> + ret = dw9768_init_controls(dw9768);
> + if (ret)
> + goto entity_cleanup;
> +
> + dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + dw9768->sd.internal_ops = &dw9768_int_ops;
> +
> + ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
> + if (ret < 0)
> + goto entity_cleanup;
> +
> + dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> + pm_runtime_enable(dev);
> + if (!pm_runtime_enabled(dev)) {
> + ret = dw9768_runtime_resume(dev);
> + if (ret < 0) {
> + dev_err(dev, "failed to power on: %d\n", ret);
> + goto entity_cleanup;
> + }
> + }
> +
> + ret = v4l2_async_register_subdev(&dw9768->sd);
> + if (ret < 0)
> + goto entity_cleanup;
> +
> + return 0;
> +
> +entity_cleanup:
Need to power off if the code above powered on.
> + v4l2_ctrl_handler_free(&dw9768->ctrls);
> + media_entity_cleanup(&dw9768->sd.entity);
> + return ret;
> +}
> +
> +static int dw9768_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9768 *dw9768 = sd_to_dw9768(sd);
> +
> + pm_runtime_disable(&client->dev);
First the device must be unregistered from the userspace. Otherwise there
is a race condition that risks the userspace accessing the device while the
deinitialization is happening.
Best regards,
Tomasz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Suman Anna @ 2020-05-21 19:52 UTC (permalink / raw)
To: Bjorn Andersson
Cc: devicetree, Mathieu Poirier, Loic Pallardy, Lokesh Vutla,
linux-remoteproc, Arnaud Pouliquen, linux-kernel, Clement Leger,
Rob Herring, linux-arm-kernel
In-Reply-To: <20200521194116.GP408178@builder.lan>
On 5/21/20 2:41 PM, Bjorn Andersson wrote:
> On Thu 21 May 12:29 PDT 2020, Suman Anna wrote:
>
>> On 5/21/20 2:21 PM, Bjorn Andersson wrote:
>>> On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
>>>
>>>> Hi Bjorn,
>>>>
>>>> On 5/21/20 12:54 PM, Bjorn Andersson wrote:
>>>>> On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
>>>>>
>>>>>> The current remoteproc core has supported only 32-bit remote
>>>>>> processors and as such some of the current resource structures
>>>>>> may not scale well for 64-bit remote processors, and would
>>>>>> require new versions of resource types. Each resource is currently
>>>>>> identified by a 32-bit type field. Introduce the concept of version
>>>>>> for these resource types by overloading this 32-bit type field
>>>>>> into two 16-bit version and type fields with the existing resources
>>>>>> behaving as version 0 thereby providing backward compatibility.
>>>>>>
>>>>>> The version field is passed as an additional argument to each of
>>>>>> the handler functions, and all the existing handlers are updated
>>>>>> accordingly. Each specific handler will be updated on a need basis
>>>>>> when a new version of the resource type is added.
>>>>>>
>>>>>
>>>>> I really would prefer that we add additional types for the new
>>>>> structures, neither side will be compatible with new versions without
>>>>> enhancements to their respective implementations anyways.
>>>>
>>>> OK.
>>>>
>>>>>
>>>>>> An alternate way would be to introduce the new types as completely
>>>>>> new resource types which would require additional customization of
>>>>>> the resource handlers based on the 32-bit or 64-bit mode of a remote
>>>>>> processor, and introduction of an additional mode flag to the rproc
>>>>>> structure.
>>>>>>
>>>>>
>>>>> What would this "mode" indicate? If it's version 0 or 1?
>>>>
>>>> No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
>>>> loading handlers if the resource types need to be segregated accordingly.
>>>>
>>>
>>> Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
>>> remote processor need different firmware from your 32-bit processor
>>> anyways, if you want to support the wider resource? And you would pack
>>> your firmware with the appropriate resource types?
>>
>> Yes, that's correct.
>>
>>>
>>> Afaict the bit width of your remote processor, busses or memory is
>>> unrelated to the choice of number of bits used to express things in the
>>> resource table.
>>
>> I would have to add the new resource type to the loading_handlers right, so
>> it is a question of whether we want to impose any restrictions in remoteproc
>> core or not from supporting a certain resource type (eg: I don't expect
>> RSC_TRACE entries on 64-bit processors).
>>
>
> Right, but either you add support for new resource types to the
> loading_handlers, or you add the version checks within each handler,
> either way you will have to do some work to be compatible with new
> versions.
>
> Regarding what resources would be fit for a 64-bit processor probably
> relates to many things, in particular the question of what we actually
> mean when we say that a coprocessor is 64-bit. So I don't really see a
> need for the remoteproc core to prevent someone to design their
> system/firmware to have a 64-bit CPU being passed 32-bit addresses.
OK. In general, I have seen firmware developers get confused w.r.t the
resource types, that's why I was inclined to go with the restrictive
checking. Anyway, will rework the support as per the comments.
regards
Suman
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 02/12] ACPI/IORT: Make iort_get_device_domain IRQ domain agnostic
From: Bjorn Helgaas @ 2020-05-21 19:56 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Marc Zyngier, devicetree, linux-pci, Catalin Marinas,
Joerg Roedel, Hanjun Guo, Rafael J. Wysocki, Robin Murphy,
linux-acpi, iommu, Rob Herring, Makarand Pawagi, Sudeep Holla,
Bjorn Helgaas, Diana Craciun, Will Deacon, linux-arm-kernel,
Laurentiu Tudor
In-Reply-To: <20200521130008.8266-3-lorenzo.pieralisi@arm.com>
On Thu, May 21, 2020 at 01:59:58PM +0100, Lorenzo Pieralisi wrote:
> iort_get_device_domain() is PCI specific but it need not be,
> since it can be used to retrieve IRQ domain nexus of any kind
> by adding an irq_domain_bus_token input to it.
>
> Make it PCI agnostic by also renaming the requestor ID input
> to a more generic ID name.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Marc Zyngier <maz@kernel.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> # pci/msi.c
> ---
> drivers/acpi/arm64/iort.c | 14 +++++++-------
> drivers/pci/msi.c | 3 ++-
> include/linux/acpi_iort.h | 7 ++++---
> 3 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 7cfd77b5e6e8..8f2a961c1364 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -567,7 +567,6 @@ static struct acpi_iort_node *iort_find_dev_node(struct device *dev)
> node = iort_get_iort_node(dev->fwnode);
> if (node)
> return node;
> -
> /*
> * if not, then it should be a platform device defined in
> * DSDT/SSDT (with Named Component node in IORT)
> @@ -658,13 +657,13 @@ static int __maybe_unused iort_find_its_base(u32 its_id, phys_addr_t *base)
> /**
> * iort_dev_find_its_id() - Find the ITS identifier for a device
> * @dev: The device.
> - * @req_id: Device's requester ID
> + * @id: Device's ID
> * @idx: Index of the ITS identifier list.
> * @its_id: ITS identifier.
> *
> * Returns: 0 on success, appropriate error value otherwise
> */
> -static int iort_dev_find_its_id(struct device *dev, u32 req_id,
> +static int iort_dev_find_its_id(struct device *dev, u32 id,
> unsigned int idx, int *its_id)
> {
> struct acpi_iort_its_group *its;
> @@ -674,7 +673,7 @@ static int iort_dev_find_its_id(struct device *dev, u32 req_id,
> if (!node)
> return -ENXIO;
>
> - node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE);
> + node = iort_node_map_id(node, id, NULL, IORT_MSI_TYPE);
> if (!node)
> return -ENXIO;
>
> @@ -697,19 +696,20 @@ static int iort_dev_find_its_id(struct device *dev, u32 req_id,
> *
> * Returns: the MSI domain for this device, NULL otherwise
> */
> -struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id)
> +struct irq_domain *iort_get_device_domain(struct device *dev, u32 id,
> + enum irq_domain_bus_token bus_token)
> {
> struct fwnode_handle *handle;
> int its_id;
>
> - if (iort_dev_find_its_id(dev, req_id, 0, &its_id))
> + if (iort_dev_find_its_id(dev, id, 0, &its_id))
> return NULL;
>
> handle = iort_find_domain_token(its_id);
> if (!handle)
> return NULL;
>
> - return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
> + return irq_find_matching_fwnode(handle, bus_token);
> }
>
> static void iort_set_device_domain(struct device *dev,
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 6b43a5455c7a..74a91f52ecc0 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1558,7 +1558,8 @@ struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
> pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
> dom = of_msi_map_get_device_domain(&pdev->dev, rid);
> if (!dom)
> - dom = iort_get_device_domain(&pdev->dev, rid);
> + dom = iort_get_device_domain(&pdev->dev, rid,
> + DOMAIN_BUS_PCI_MSI);
> return dom;
> }
> #endif /* CONFIG_PCI_MSI_IRQ_DOMAIN */
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 8e7e2ec37f1b..08ec6bd2297f 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -29,7 +29,8 @@ struct fwnode_handle *iort_find_domain_token(int trans_id);
> #ifdef CONFIG_ACPI_IORT
> void acpi_iort_init(void);
> u32 iort_msi_map_rid(struct device *dev, u32 req_id);
> -struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
> +struct irq_domain *iort_get_device_domain(struct device *dev, u32 id,
> + enum irq_domain_bus_token bus_token);
> void acpi_configure_pmsi_domain(struct device *dev);
> int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
> /* IOMMU interface */
> @@ -40,8 +41,8 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head);
> static inline void acpi_iort_init(void) { }
> static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
> { return req_id; }
> -static inline struct irq_domain *iort_get_device_domain(struct device *dev,
> - u32 req_id)
> +static inline struct irq_domain *iort_get_device_domain(
> + struct device *dev, u32 id, enum irq_domain_bus_token bus_token)
> { return NULL; }
> static inline void acpi_configure_pmsi_domain(struct device *dev) { }
> /* IOMMU interface */
> --
> 2.26.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 08/12] of/irq: make of_msi_map_get_device_domain() bus agnostic
From: Bjorn Helgaas @ 2020-05-21 19:57 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: devicetree, Sudeep Holla, Catalin Marinas, Will Deacon,
Rafael J. Wysocki, Marc Zyngier, Joerg Roedel, Hanjun Guo,
linux-pci, Makarand Pawagi, linux-acpi, iommu, Rob Herring,
Diana Craciun, Bjorn Helgaas, Robin Murphy, linux-arm-kernel,
Laurentiu Tudor
In-Reply-To: <20200521130008.8266-9-lorenzo.pieralisi@arm.com>
On Thu, May 21, 2020 at 02:00:04PM +0100, Lorenzo Pieralisi wrote:
> From: Diana Craciun <diana.craciun@oss.nxp.com>
>
> of_msi_map_get_device_domain() is PCI specific but it need not be and
> can be easily changed to be bus agnostic in order to be used by other
> busses by adding an IRQ domain bus token as an input parameter.
>
> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> # pci/msi.c
> ---
> drivers/of/irq.c | 8 +++++---
> drivers/pci/msi.c | 2 +-
> include/linux/of_irq.h | 5 +++--
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index a296eaf52a5b..48a40326984f 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -613,18 +613,20 @@ u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in)
> * of_msi_map_get_device_domain - Use msi-map to find the relevant MSI domain
> * @dev: device for which the mapping is to be done.
> * @rid: Requester ID for the device.
> + * @bus_token: Bus token
> *
> * Walk up the device hierarchy looking for devices with a "msi-map"
> * property.
> *
> * Returns: the MSI domain for this device (or NULL on failure)
> */
> -struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 rid)
> +struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 id,
> + u32 bus_token)
> {
> struct device_node *np = NULL;
>
> - __of_msi_map_rid(dev, &np, rid);
> - return irq_find_matching_host(np, DOMAIN_BUS_PCI_MSI);
> + __of_msi_map_rid(dev, &np, id);
> + return irq_find_matching_host(np, bus_token);
> }
>
> /**
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 74a91f52ecc0..9532e1d12d3f 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1556,7 +1556,7 @@ struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
> u32 rid = pci_dev_id(pdev);
>
> pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
> - dom = of_msi_map_get_device_domain(&pdev->dev, rid);
> + dom = of_msi_map_get_device_domain(&pdev->dev, rid, DOMAIN_BUS_PCI_MSI);
> if (!dom)
> dom = iort_get_device_domain(&pdev->dev, rid,
> DOMAIN_BUS_PCI_MSI);
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index 1214cabb2247..7142a3722758 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -52,7 +52,8 @@ extern struct irq_domain *of_msi_get_domain(struct device *dev,
> struct device_node *np,
> enum irq_domain_bus_token token);
> extern struct irq_domain *of_msi_map_get_device_domain(struct device *dev,
> - u32 rid);
> + u32 id,
> + u32 bus_token);
> extern void of_msi_configure(struct device *dev, struct device_node *np);
> u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in);
> #else
> @@ -85,7 +86,7 @@ static inline struct irq_domain *of_msi_get_domain(struct device *dev,
> return NULL;
> }
> static inline struct irq_domain *of_msi_map_get_device_domain(struct device *dev,
> - u32 rid)
> + u32 id, u32 bus_token)
> {
> return NULL;
> }
> --
> 2.26.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v12 1/3] dt-bindings: i2c: npcm7xx: add NPCM I2C controller
From: robh @ 2020-05-21 19:58 UTC (permalink / raw)
To: Tali Perry
Cc: devicetree, benjaminfair, wsa, avifishman70, venture, openbmc,
kfting, brendanhiggins, ofery, linux-kernel, yuenn, robh+dt,
linux-i2c, andriy.shevchenko, linux-arm-kernel, tmaimon77
In-Reply-To: <20200521110910.45518-2-tali.perry1@gmail.com>
On Thu, 21 May 2020 14:09:08 +0300, Tali Perry wrote:
> Added device tree binding documentation for Nuvoton BMC
> NPCM I2C controller.
>
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> ---
> .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml | 62 +++++++++++++++++++
> 1 file changed, 62 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
Error: Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dts:22.28-29 syntax error
FATAL ERROR: Unable to parse input tree
scripts/Makefile.lib:312: recipe for target 'Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1295115
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
Please check and re-submit.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 5/5] dt-bindings: arm: fsl: add different Protonic boards
From: robh @ 2020-05-21 20:00 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Mark Rutland, devicetree, Shawn Guo, Sascha Hauer, linux-kernel,
Rob Herring, NXP Linux Team, Pengutronix Kernel Team,
Fabio Estevam, linux-arm-kernel
In-Reply-To: <20200520154116.12909-6-o.rempel@pengutronix.de>
On Wed, 20 May 2020 17:41:16 +0200, Oleksij Rempel wrote:
> Add Protonic PRTI6Q, WD2, RVT, VT7 boards.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> Documentation/devicetree/bindings/arm/fsl.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.
If a tag was not added on purpose, please state why and what changed.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 04/19] spi: spi-mem: allow specifying a command's extension
From: Pratyush Yadav @ 2020-05-21 20:03 UTC (permalink / raw)
To: Boris Brezillon
Cc: Alexandre Belloni, Mason Yang, Vignesh Raghavendra, Tudor Ambarus,
Richard Weinberger, Sekhar Nori, linux-kernel, Ludovic Desroches,
Mark Brown, linux-mtd, Miquel Raynal, Matthias Brugger,
linux-mediatek, linux-spi, linux-arm-kernel
In-Reply-To: <20200521194129.3lqdormmcs4zadh2@ti.com>
On 22/05/20 01:11AM, Pratyush Yadav wrote:
> On 21/05/20 08:22PM, Boris Brezillon wrote:
> > On Wed, 20 May 2020 22:00:38 +0530
> > Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > > In xSPI mode, flashes expect 2-byte opcodes. The second byte is called
> > > the "command extension". There can be 3 types of extensions in xSPI:
> > > repeat, invert, and hex. When the extension type is "repeat", the same
> > > opcode is sent twice. When it is "invert", the second byte is the
> > > inverse of the opcode. When it is "hex" an additional opcode byte based
> > > is sent with the command whose value can be anything.
> > >
> > > So, make opcode a 16-bit value and add a 'nbytes', similar to how
> > > multiple address widths are handled.
> > >
> > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > > ---
> > > include/linux/spi/spi-mem.h | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> > > index e3dcb956bf61..731bb64c6ba6 100644
> > > --- a/include/linux/spi/spi-mem.h
> > > +++ b/include/linux/spi/spi-mem.h
> > > @@ -69,6 +69,8 @@ enum spi_mem_data_dir {
> > >
> > > /**
> > > * struct spi_mem_op - describes a SPI memory operation
> > > + * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
> > > + * sent MSB-first.
> > > * @cmd.buswidth: number of IO lines used to transmit the command
> > > * @cmd.opcode: operation opcode
> > > * @cmd.dtr: whether the command opcode should be sent in DTR mode or not
> > > @@ -94,9 +96,10 @@ enum spi_mem_data_dir {
> > > */
> > > struct spi_mem_op {
> > > struct {
> > > + u8 nbytes;
> > > u8 buswidth;
> > > u8 dtr : 1;
> > > - u8 opcode;
> > > + u16 opcode;
> > > } cmd;
> > >
> > > struct {
> >
> > As mentioned in one of my previous review, you should patch the mxic
> > driver before extending the opcode field:
>
> IIUC, this patchset doesn't break original functionality of the driver.
> It will work like before with 1-byte opcodes. So I don't think it is the
> responsibility of this patchset to enhance the driver. It didn't work
> before with 2-byte opcodes, it won't work now. IMO this should be a
> separate, independent change.
Scratch that. Big/little endian issue. If you'd drop your Signed-off-by,
I'll write the commit message and patch it in.
> > --->8---
> > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> > index 69491f3a515d..c3f4136a7c1d 100644
> > --- a/drivers/spi/spi-mxic.c
> > +++ b/drivers/spi/spi-mxic.c
> > @@ -356,6 +356,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
> > int nio = 1, i, ret;
> > u32 ss_ctrl;
> > u8 addr[8];
> > + u8 cmd[2];
> >
> > ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
> > if (ret)
> > @@ -393,7 +394,10 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
> > writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
> > mxic->regs + HC_CFG);
> >
> > - ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
> > + for (i = 0; i < op->cmd.nbytes; i++)
> > + cmd[i] = op->cmd.opcode >> (8 * (op->cmd.nbytes - i - 1));
> > +
> > + ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
> > if (ret)
> > goto out;
> >
--
Regards,
Pratyush Yadav
Texas Instruments India
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 04/19] spi: spi-mem: allow specifying a command's extension
From: Boris Brezillon @ 2020-05-21 20:16 UTC (permalink / raw)
To: Pratyush Yadav
Cc: Alexandre Belloni, Mason Yang, Vignesh Raghavendra, Tudor Ambarus,
Richard Weinberger, Sekhar Nori, linux-kernel, Ludovic Desroches,
Mark Brown, linux-mtd, Miquel Raynal, Matthias Brugger,
linux-mediatek, linux-spi, linux-arm-kernel
In-Reply-To: <20200521200313.bnil6a4u3dhiizr4@ti.com>
On Fri, 22 May 2020 01:33:15 +0530
Pratyush Yadav <p.yadav@ti.com> wrote:
> On 22/05/20 01:11AM, Pratyush Yadav wrote:
> > On 21/05/20 08:22PM, Boris Brezillon wrote:
> > > On Wed, 20 May 2020 22:00:38 +0530
> > > Pratyush Yadav <p.yadav@ti.com> wrote:
> > >
> > > > In xSPI mode, flashes expect 2-byte opcodes. The second byte is called
> > > > the "command extension". There can be 3 types of extensions in xSPI:
> > > > repeat, invert, and hex. When the extension type is "repeat", the same
> > > > opcode is sent twice. When it is "invert", the second byte is the
> > > > inverse of the opcode. When it is "hex" an additional opcode byte based
> > > > is sent with the command whose value can be anything.
> > > >
> > > > So, make opcode a 16-bit value and add a 'nbytes', similar to how
> > > > multiple address widths are handled.
> > > >
> > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > > > ---
> > > > include/linux/spi/spi-mem.h | 5 ++++-
> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> > > > index e3dcb956bf61..731bb64c6ba6 100644
> > > > --- a/include/linux/spi/spi-mem.h
> > > > +++ b/include/linux/spi/spi-mem.h
> > > > @@ -69,6 +69,8 @@ enum spi_mem_data_dir {
> > > >
> > > > /**
> > > > * struct spi_mem_op - describes a SPI memory operation
> > > > + * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
> > > > + * sent MSB-first.
> > > > * @cmd.buswidth: number of IO lines used to transmit the command
> > > > * @cmd.opcode: operation opcode
> > > > * @cmd.dtr: whether the command opcode should be sent in DTR mode or not
> > > > @@ -94,9 +96,10 @@ enum spi_mem_data_dir {
> > > > */
> > > > struct spi_mem_op {
> > > > struct {
> > > > + u8 nbytes;
> > > > u8 buswidth;
> > > > u8 dtr : 1;
> > > > - u8 opcode;
> > > > + u16 opcode;
> > > > } cmd;
> > > >
> > > > struct {
> > >
> > > As mentioned in one of my previous review, you should patch the mxic
> > > driver before extending the opcode field:
> >
> > IIUC, this patchset doesn't break original functionality of the driver.
> > It will work like before with 1-byte opcodes. So I don't think it is the
> > responsibility of this patchset to enhance the driver. It didn't work
> > before with 2-byte opcodes, it won't work now. IMO this should be a
> > separate, independent change.
>
> Scratch that. Big/little endian issue. If you'd drop your Signed-off-by,
> I'll write the commit message and patch it in.
Just add a Suggested-by, that should be fine.
>
> > > --->8---
> > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> > > index 69491f3a515d..c3f4136a7c1d 100644
> > > --- a/drivers/spi/spi-mxic.c
> > > +++ b/drivers/spi/spi-mxic.c
> > > @@ -356,6 +356,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
> > > int nio = 1, i, ret;
> > > u32 ss_ctrl;
> > > u8 addr[8];
> > > + u8 cmd[2];
> > >
> > > ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
> > > if (ret)
> > > @@ -393,7 +394,10 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
> > > writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
> > > mxic->regs + HC_CFG);
> > >
> > > - ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
> > > + for (i = 0; i < op->cmd.nbytes; i++)
> > > + cmd[i] = op->cmd.opcode >> (8 * (op->cmd.nbytes - i - 1));
> > > +
> > > + ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
> > > if (ret)
> > > goto out;
> > >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RESEND PATCH v14 04/11] pwm: clps711x: Cast period to u32 before use as divisor
From: Guru Das Srinagesh @ 2020-05-21 20:25 UTC (permalink / raw)
To: Daniel Thompson
Cc: linux-arm-kernel, linux-pwm, Arnd Bergmann, David Collins,
Stephen Boyd, linux-kernel, Thierry Reding, Geert Uytterhoeven,
Dan Carpenter, Uwe Kleine-König, Joe Perches,
Subbaraman Narayanamurthy, Lee Jones, Guenter Roeck
In-Reply-To: <20200521101934.j5ivjky4e6byveut@holly.lan>
On Thu, May 21, 2020 at 11:19:34AM +0100, Daniel Thompson wrote:
> On Wed, May 20, 2020 at 03:55:57PM -0700, Guru Das Srinagesh wrote:
> > Since the PWM framework is switching struct pwm_args.period's datatype
> > to u64, prepare for this transition by typecasting it to u32.
> >
> > Also, since the dividend is still a 32-bit number, any divisor greater
> > than the numerator will cause the quotient to be zero, so return 0 in
> > that case to efficiently skip the division.
> >
> > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > ---
> > drivers/pwm/pwm-clps711x.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
> > index 924d39a..da771b1 100644
> > --- a/drivers/pwm/pwm-clps711x.c
> > +++ b/drivers/pwm/pwm-clps711x.c
> > @@ -43,7 +43,10 @@ static void clps711x_pwm_update_val(struct clps711x_chip *priv, u32 n, u32 v)
> > static unsigned int clps711x_get_duty(struct pwm_device *pwm, unsigned int v)
> > {
> > /* Duty cycle 0..15 max */
> > - return DIV_ROUND_CLOSEST(v * 0xf, pwm->args.period);
> > + if (pwm->args.period > (v * 0xf))
> > + return 0;
>
> This doesn't look right to me.
>
> DIV_ROUND_CLOSEST() does rounded division and the short circuit doesn't
> implement that.
My initial patch [1] was to simply use DIV64_U64_ROUND_CLOSEST(), but I
got review feedback to add a short-circuit (same thread, [2]). I feel
like I should skip the short-circuiting and type casting and simply just
use DIV64_U64_ROUND_CLOSEST() - what do you think?
[1] https://lore.kernel.org/lkml/587f9ccae68ad7e1ce97fa8da6037292af1a5095.1584473399.git.gurus@codeaurora.org/
[2] https://lore.kernel.org/lkml/CAK8P3a2Hi_AoRC3g7qKth4e_Y1jZrbBDhWUb3YPZm10FWMu-ig@mail.gmail.com/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
From: Wolfram Sang @ 2020-05-21 20:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: devicetree, Linux Kernel Mailing List, Tomer Maimon, Nancy Yuen,
avifishman70, Patrick Venture, OpenBMC Maillist, Brendan Higgins,
Ofer Yehielli, Tali Perry, kfting, Rob Herring, linux-i2c,
linux-arm-kernel, Benjamin Fair
In-Reply-To: <20200521145347.GO1634618@smile.fi.intel.com>
[-- Attachment #1.1: Type: text/plain, Size: 1148 bytes --]
> > > I wondered also about DEBUG_FS entries. I can see their value when
> > > developing the driver. But since this is done now, do they really help a
> > > user to debug a difficult case? I am not sure, and then I wonder if we
> > > should have that code in upstream. I am open for discussion, though.
> >
> > The user wanted to have health monitor implemented on top of the driver.
> > The user has 16 channels connected the multiple devices. All are operated
> > using various daemons in the system. Sometimes the slave devices are power down.
> > Therefor the user wanted to track the health status of the devices.
>
> Ah, then there are these options I have in mind (Wolfram, FYI as well!):
> 1) push with debugfs as a temporary solution and convert to devlink health protocol [1];
> 2) drop it and develop devlink_health solution;
> 3) push debugfs and wait if I²C will gain devlink health support
No need for 2). We can push it now and convert it later. That being
said, I wonder if [1] is suitable for this driver? Things like NACKs and
timeouts happen regularly on an I2C bus and are not a state of bad
health.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
From: Tali Perry @ 2020-05-21 20:47 UTC (permalink / raw)
To: Wolfram Sang
Cc: devicetree, Tomer Maimon, Nancy Yuen, avifishman70,
Patrick Venture, OpenBMC Maillist, Brendan Higgins, Ofer Yehielli,
Linux Kernel Mailing List, kfting, Rob Herring, linux-i2c,
Andy Shevchenko, linux-arm-kernel, Benjamin Fair
In-Reply-To: <20200521203758.GA20150@ninjato>
On Thu, May 21, 2020 at 11:37 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
>
> > > > I wondered also about DEBUG_FS entries. I can see their value when
> > > > developing the driver. But since this is done now, do they really help a
> > > > user to debug a difficult case? I am not sure, and then I wonder if we
> > > > should have that code in upstream. I am open for discussion, though.
> > >
> > > The user wanted to have health monitor implemented on top of the driver.
> > > The user has 16 channels connected the multiple devices. All are operated
> > > using various daemons in the system. Sometimes the slave devices are power down.
> > > Therefor the user wanted to track the health status of the devices.
> >
> > Ah, then there are these options I have in mind (Wolfram, FYI as well!):
> > 1) push with debugfs as a temporary solution and convert to devlink health protocol [1];
> > 2) drop it and develop devlink_health solution;
> > 3) push debugfs and wait if I²C will gain devlink health support
>
> No need for 2). We can push it now and convert it later. That being
> said, I wonder if [1] is suitable for this driver? Things like NACKs and
> timeouts happen regularly on an I2C bus and are not a state of bad
> health.
>
Agree, having a timeout every now and then is not an issue. The user
is interested
in cases when the number of timeouts\BER\nack\recovery are high.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6] arm64: Expose FAR_EL1 tag bits in sigcontext
From: Peter Collingbourne @ 2020-05-21 20:48 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Andrey Konovalov, Kevin Brodsky, Oleg Nesterov, Kostya Serebryany,
Linux ARM, Catalin Marinas, Vincenzo Frascino, Will Deacon,
Dave Martin, Evgenii Stepanov, Richard Henderson
In-Reply-To: <874ks9drb6.fsf@x220.int.ebiederm.org>
On Thu, May 21, 2020 at 12:28 PM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Peter Collingbourne <pcc@google.com> writes:
>
> > On Thu, May 21, 2020 at 5:39 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >>
> >> Peter Collingbourne <pcc@google.com> writes:
> >>
> >> > On Wed, May 20, 2020 at 2:26 AM Dave Martin <Dave.Martin@arm.com> wrote:
> >> >>
> >> >> On Wed, May 20, 2020 at 09:55:03AM +0100, Will Deacon wrote:
> >> >> > On Tue, May 19, 2020 at 03:00:12PM -0700, Peter Collingbourne wrote:
> >> >> > > On Mon, May 18, 2020 at 2:53 AM Dave Martin <Dave.Martin@arm.com> wrote:
> >> >> > > > On Thu, May 14, 2020 at 05:58:21PM -0700, Peter Collingbourne wrote:
> >> >> > > > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> >> >> > > > > index baa88dc02e5c..5867f2fdbe64 100644
> >> >> > > > > --- a/arch/arm64/kernel/signal.c
> >> >> > > > > +++ b/arch/arm64/kernel/signal.c
> >> >> > > > > @@ -648,6 +648,7 @@ static int setup_sigframe(struct
> >> >> > > > > rt_sigframe_user_layout *user,
> >> >> > > > > __put_user_error(ESR_MAGIC, &esr_ctx->head.magic, err);
> >> >> > > > > __put_user_error(sizeof(*esr_ctx), &esr_ctx->head.size, err);
> >> >> > > > > __put_user_error(current->thread.fault_code,
> >> >> > > > > &esr_ctx->esr, err);
> >> >> > > > > + current->thread.fault_code = 0;
> >> >> > > >
> >> >> > > > Perhaps, but we'd need to be careful. For example, can we run out of
> >> >> > > > user stack before this and deliver a SIGSEGV, but with the old
> >> >> > > > fault_code still set? Then we'd emit the old fault code with the
> >> >> > > > new "can't deliver signal" signal, which doesn't make sense.
> >> >> > > >
> >> >> > > > Stuff may also go wrong with signal prioritisation.
> >> >> > > >
> >> >> > > > If a higher-priority signal (say SIGINT) comes in after a data abort
> >> >> > > > enters the kernel but before the resulting SIGSEGV is dequeued for
> >> >> > > > delivery, wouldn't we deliver SIGINT first, with the bogus fault code?
> >> >> > > > With your change we'd then have cleared the fault code by the time we
> >> >> > > > deliver the SIGSEGV it actually relates to, if I've understood right.
> >> >> > > >
> >> >> > > > Today, I think we just attach that fault code to every signal that's
> >> >> > > > delivered until something overwrites or resets it, which means that
> >> >> > > > a signal that needs fault_code gets it, at the expense of attaching
> >> >> > > > it to a bunch of other random signals too.
> >> >> > > >
> >> >> > > >
> >> >> > > > Checking the signal number and si_code might help us to know what we
> >> >> > > > should be doing with fault_code. We need to have sure userspace can't
> >> >> > > > trick us with a non kernel generated signal here. It would also be
> >> >> > > > necessary to check how PTRACE_SETSIGINFO interacts with this.
> >> >> > >
> >> >> > > With these possible interactions in mind I think we should store the
> >> >> > > fault code and fault address in kernel_siginfo instead of
> >> >> > > thread_struct (and clear these fields when we receive a siginfo from
> >> >> > > userspace, i.e. in copy_siginfo_from_user which is used by
> >> >> > > ptrace(PTRACE_SETSIGINFO) among other places). That way, the
> >> >> > > information is clearly associated with the signal itself and not the
> >> >> > > thread, so we don't need to worry about our signal being delivered out
> >> >> > > of order.
> >> >> >
> >> >> > Hmm, I can't see a way to do that that isn't horribly invasive in the core
> >> >> > signal code. Can you?
> >> >
> >> > I think I've come up with a way that doesn't seem to be too invasive.
> >> > See patch #1 of the series that I'm about to send out.
> >> >
> >> >> > But generally, I agree: the per-thread handling of fault_address and
> >> >> > fault_code appears to be quite broken in the face of signal prioritisation
> >> >> > and signals that don't correspond directly to hardware trap. It would be
> >> >> > nice to have some tests for this...
> >> >> >
> >> >> > If we want to pile on more bodges, perhaps we could stash the signal number
> >> >> > to which the fault_{address,code} relate, and then check that at delivery
> >> >> > and clear on a match. I hate it.
> >> >>
> >> >> I agree with Daniel's suggestion in principle, but I was also concerned
> >> >> about whether it would be too invasive elsewhere.
> >> >>
> >> >> Question though: does the core code take special care to make sure that
> >> >> a force_sig cannot be outprioritised by a regular signal? If so,
> >> >> perhaps we get away with it. I ask this, because the same same issue
> >> >> may be hitting other arches otherwise.
> >> >
> >> > Not as far as I can tell. There does appear to be prioritisation for
> >> > synchronous signals [1] but as far as I can tell nothing to
> >> > distinguish one of these signals from one with the same signal number
> >> > sent from userspace (e.g. via kill(2)).
> >>
> >> The si_code will differ between signals generated between userspace
> >> and signals generated by the kernel.
> >>
> >> We do allow a little bit of ptrace and sending to yourself to spoof
> >> kernel generated signals, for reasons of debugging and process migration
> >> where an existing process needs to be reconstructed. But the defenses
> >> should be strong enough you can assume that we reliably distinguish
> >> between a signal from userspace and a signal from the kernel.
> >
> > So check for SIGBUS || SIGSEGV and one of the below si_codes, and only
> > add the context in that case? Seems fragile to me, but I suppose I
> > could live with it.
> >
> >> I don't fully follow what you are doing but this feels like the
> >> kind of case where a new si_code has been defined as well as additional
> >> fields in siginfo.
> >
> > There is no new si_code for this, the information will be exposed for
> > several existing si_code types (BUS_ADRERR, BUS_ADRALN, BUS_MCEERR_AR,
> > SEGV_ACCERR, SEGV_MAPERR), and possibly others in the future
> > (particularly SEGV_MTESERR, which is part of the proposed MTE patch
> > set). Note that we already have a union field for BUS_MCEERR_AR, and
> > we may want to expose it for the other si_codes that already have
> > union fields as well.
> >
> > That being said, taking a closer look at siginfo, I think we are in
> > luck and we might be able to make this work in a reasonable way by
> > reusing padding (see below).
> >
> >> In your patchset I really hate that you were going back to
> >> force_sig_info, and filling out struct siginfo by hand. That is an
> >> error prone pattern, and I have fixed enough bugs in the kernel to prove
> >> that.
> >
> > To be fair, most of the callers are in helper functions that take
> > explicit parameters similar to force_sig_fault et al, and the SIGILL
> > one could easily be made that way as well.
> >
> >> I take exception to the idea that including the full address might break
> >> userspace. That means typically means someone has been too lazy to look
> >> and see what userspace is doing. When that userspace that might break
> >> is the same userspace you are changing the kernel to serve that makes me
> >> nervous. AKA the userspace that cares about this signal and how it is
> >> represented in siginfo.
> >
> > It's not a matter of being lazy. This behaviour isn't just an accident
> > but has been explicitly documented for years (see the
> > tagged-pointers.rst file that I changed: "Non-zero tags are not
> > preserved when delivering signals."), so users can reasonably rely on
> > it. Furthermore we simply don't have visibility into the majority of
> > userspace. For example, there are a lot of closed source Android apps
> > out there, and who knows what signal handlers they're installing and
> > how they're making use of the si_addr field on e.g. SEGV_MAPERR. We
> > can't just change the documented semantics under their feet.
> >
> > It's also not the same userspace either. The userspace that's
> > initially going to be consuming the new fields is in a part of the
> > Android system that handles and reports crashes, and that's something
> > that we control unlike all the apps.
> >
> > Finally, the userspace may need to know whether the tag bits were
> > actually zero or whether they were just unavailable, otherwise
> > userspace could for example produce a misleading crash report. Simply
> > having the kernel set the top bits of si_addr wouldn't accomplish that
> > due to the kernel's previous behaviour, hence the mask to let
> > userspace know which bits are accurate.
> >
> >> A fix of one instance of SIGILL should not be included with a patch that
> >> does something else, and really should come before everything else if
> >> possible.
> >
> > Fair point. I can see if I can split that part out.
> >
> >> If this information really belongs in struct siginfo (as it sounds like)
> >> please actually put the information in siginfo, and let userspace look
> >> in siginfo to find it. struct siginfo is a union with plenty of space,
> >> and plenty of si_codes.
> >>
> >> If this applies to multiple cases then it might be trickier but please
> >> dig into the details, don't toss things into sigcontext just because
> >> you can't figure out a clean design for reporting this.
> >
> > If we wanted this in siginfo, one idea that I had was to revert commit
> > b68a68d3dcc15ebbf23cbe91af1abf57591bd96b and add unsigned char fields
> > _addr_top_byte and _addr_top_byte_mask in the padding between
> > _addr_lsb and the union (with comments on all the fields of course to
> > say when they are filled in). I think that would work since we are
> > already clearing padding in siginfo, one nice property of the new
> > fields is that the zero values are correct in the case where the
> > information isn't being exposed (so old kernels would already have the
> > correct behaviour). That would only work on certain architectures
> > (i.e. at least alignof(void*) >= 4) so I suppose it could have an
> > #ifdef __aarch64__ around it.
>
> Perhaps add a 4th padding member to the union inside of _sigfault, that
> adds something like 4 unsigned long's worth of data, and then have your
> fields after the union.
Maybe. I guess we could always add another union after my fields if we
end up needing another union member that is larger than the 4 unsigned
longs, which would be ugly but at least it would work. Reusing the
padding would avoid that but maybe it's not that likely that we'll
need that much.
> Is it quite a bit of work to gather that information from the
> instructions that faulted? I am just checking that this work is really
> makes sense.
I think so. At a glance there are hundreds of load and store
instructions on arm64 and we would need to know how to disassemble all
of them and recompute the si_addr from scratch (since the tag bits
could come from any of the registers used to compute the address). And
we really don't want to be doing this tricky stuff in a signal handler
where we've just crashed.
> What I really don't understand is how well this problem generalizes to
> other architectures to tell if this is something other people need to
> solve at some point as well.
An architecture with a feature similar to ARM's TBI or MTE may need
something like this as well, depending on whether they decide to
expose the tag bits in si_addr from the start (and if the feature is
similar to TBI it certainly seems like a reasonable choice to follow
arm64 for compatibility reasons). I would imagine that the main thing
that could vary between architectures is the number of bits involved,
which suggests making the fields arch-specific (or making them larger,
but that may be wasteful).
The only other architecture that I'm aware of with such a feature is
SPARC (whose ADI is similar to MTE). The documentation [1] seems to
suggest that the tag bits are available in si_addr but isn't very
specific.
Peter
[1] https://www.kernel.org/doc/Documentation/sparc/adi.rst
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
From: Wolfram Sang @ 2020-05-21 21:21 UTC (permalink / raw)
To: Andy Shevchenko
Cc: devicetree, linux-kernel, tmaimon77, yuenn, avifishman70, venture,
openbmc, brendanhiggins, ofery, Tali Perry, kfting, robh+dt,
linux-i2c, linux-arm-kernel, benjaminfair
In-Reply-To: <20200521143100.GA16812@ninjato>
[-- Attachment #1.1: Type: text/plain, Size: 192 bytes --]
> From a glimpse, this looks good to go. I will have a close look later
> today.
Phew, this driver is huge. I won't finish my review today, but I am
working on it and am maybe 2/3 through.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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