Re: [RFC 5/5] doc_rst: media: New SDR formats SC16, SC18 & SC20
Antti, Hans, ping ? Please see below. On Friday 04 Nov 2016 09:23:29 Ramesh Shanmugasundaram wrote: > > On 11/02/2016 10:58 PM, Laurent Pinchart wrote: > >> On Wednesday 02 Nov 2016 09:00:00 Ramesh Shanmugasundaram wrote: > > On Wednesday 12 Oct 2016 15:10:29 Ramesh Shanmugasundaram wrote: > > > >> This patch adds documentation for the three new SDR formats > >> > >> V4L2_SDR_FMT_SCU16BE > >> V4L2_SDR_FMT_SCU18BE > >> V4L2_SDR_FMT_SCU20BE > >>> > >>> [snip] > >>> > >> + > >> + - start + 0: > >> + > >> + - I'\ :sub:`0[D13:D6]` > >> + > >> + - I'\ :sub:`0[D5:D0]` > >> + > >> +- .. row 2 > >> + > >> + - start + buffer_size/2: > >> + > >> + - Q'\ :sub:`0[D13:D6]` > >> + > >> + - Q'\ :sub:`0[D5:D0]` > > > > > > > > The format looks planar, does it use one V4L2 plane (as does NV12) > > or two V4L2 planes (as does NV12M) ? Same question for the other > > formats. > > Thank you for bringing up this topic. This is one of the key design > dilemma. > > The I & Q data for these three SDR formats comes from two different > DMA channels and hence two separate pointers -> we could say it is > v4l2 multi- planar. Right now, I am making it look like a single > plane by presenting the data in one single buffer ptr. > > For e.g. multi-planar SC16 format would look something like this > > <32bits--> > <--I(14 bit data) + 2bit status--16bit padded zeros--> : start0 + 0 > <--I(14 bit data) + 2bit status--16bit padded zeros--> : start0 + 4 > ... > <--Q(14 bit data) + 2bit status--16bit padded zeros--> : start1 + 0 > <--Q(14 bit data) + 2bit status--16bit padded zeros--> : start1 + 4 > > My concerns are > > 1) These formats are not a standard as the video "Image Formats". > These formats are possible when we use DRIF + MAX2175 combination. > If we interface with a different tuner vendor, the above format(s) > MAY/MAY NOT be re-usable. We do not know at this point. This is the > main open item for discussion in the cover letter. > >> > >> If the formats are really device-specific then they should be > >> documented accordingly and not made generic. > >> > 2) MPLANE support within V4L2 seems specific to video. Please > correct me if this is wrong interpretation. > > - struct v4l2_format contains v4l2_sdr_format and > v4l2_pix_format_mplane as members of union. Should I create a new > v4l2_sdr_format_mplane? If I have to use v4l2_pix_format_mplane most > of the video specific members would be unused (it would be similar > to using v4l2_pix_format itself instead of v4l2_sdr_format)? > >> > >> I have no answer to that question as I'm not familiar with SDR. Antti, > >> you've added v4l2_sdr_format to the API, what's your opinion ? Hans, > >> as you've acked the patch, your input would be appreciated as well. > > > > If I understood correctly this hardware provides I and Q samples via > > different channels and driver now combines those channels as a sequential > > IQ sample pairs. > > The driver combines the two buffer ptrs and present as one single buffer. > For a buffer of size 200 > > ptr + 0 : I I I I ... I > ptr + 100 : Q Q Q Q ... Q > > > I have never seen any other than hw which provides IQ IQ IQ IQ ... IQ. > > There are some modes where this h/w combo can also do IQ IQ IQ pattern. > Those modes are not added in the RFC patchset. > > > This is > > I I I I ... I > > Q Q Q Q ... Q > > I am not very familiar with planars, but it sounds like it is correct > > approach. So I think should be added rather than emulate packet > > sequential format. > > My understanding of V4L2 MPLANE constructs is limited to a quick code read > only. At this point MPLANE support seems specific to video. SDR is defined > as separate format like v4l2_pix_format. Questions would be - should we > define new SDR_MPLANE? or merge SDR format with pix format & reuse existing > MPLANE with some SDR extensions (if possible)? These seem big design > decisions. Any suggestions please? > > For my use case, MPLANE support does not seem to add significant benefit > except it may be syntactically correct. I am doing cyclic DMA with a small > set of h/w buffers and copying each stage to one mmapped vmalloc vb2_buffer > at two offsets. If we add MPLANE support, it can be two non-contiguous > buffer pointers. > > - The above decision (accomodate SDR & MPLANE) needs to be > propagated across the framework. Is this the preferred approach? > > It goes back to point (1). As of today, the change set for this > combo (DRIF+MAX2175) introduces new SDR formats only. Should it add > further SDR+MPLANE support to the framework as well? > > I would appreciate your suggestion
Re: [PATCH v4 00/14] ARM: dts: r8a779x: use demuxer for I2C
Hi Wolfram, On Thu, Nov 10, 2016 at 4:57 PM, Wolfram Sang wrote: > >> I think postponing merge of patches is one thing but I don't think >> dropping them forever is a long term solution. I think we should have > > Just to make sure because you wrote 'them': I talked about one patch of > the series, not the whole series, namely DVFS. Because Lager is the only > board we have where you can multiplex DVFS between IIC and I2C, no GPIO > option here. All other SoCs have dedicated pins for DVFS which you can't > mux to anything else at all. Sorry I meant "one patch". As a workaround it is fine, but dropping one patch still does not change the way the hardware is designed. Being able to change the I2C master controller between the on-chip I2C device and the on-chip IIC device is still something that the hardware supports but we cannot seem to do without further software development. Thanks, / magnus
Re: [PATCH 4/5] doc_rst: media: New SDR formats SC16, SC18 & SC20
Hi Ramesh,
Thank you for the patch.
On Wednesday 09 Nov 2016 15:44:43 Ramesh Shanmugasundaram wrote:
> This patch adds documentation for the three new SDR formats
>
> V4L2_SDR_FMT_SCU16BE
> V4L2_SDR_FMT_SCU18BE
> V4L2_SDR_FMT_SCU20BE
>
> Signed-off-by: Ramesh Shanmugasundaram
>
> ---
> .../media/uapi/v4l/pixfmt-sdr-scu16be.rst | 80 ++
> .../media/uapi/v4l/pixfmt-sdr-scu18be.rst | 80 ++
> .../media/uapi/v4l/pixfmt-sdr-scu20be.rst | 80 ++
> Documentation/media/uapi/v4l/sdr-formats.rst | 3 +
> 4 files changed, 243 insertions(+)
> create mode 100644 Documentation/media/uapi/v4l/pixfmt-sdr-scu16be.rst
> create mode 100644 Documentation/media/uapi/v4l/pixfmt-sdr-scu18be.rst
> create mode 100644 Documentation/media/uapi/v4l/pixfmt-sdr-scu20be.rst
>
> diff --git a/Documentation/media/uapi/v4l/pixfmt-sdr-scu16be.rst
> b/Documentation/media/uapi/v4l/pixfmt-sdr-scu16be.rst new file mode 100644
> index 000..7525378
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/pixfmt-sdr-scu16be.rst
> @@ -0,0 +1,80 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _V4L2-SDR-FMT-SCU16BE:
> +
> +**
> +V4L2_SDR_FMT_SCU16BE ('SC16')
> +**
> +
> +Sliced complex unsigned 16-bit big endian IQ sample
> +
> +Description
> +===
> +
> +This format contains a sequence of complex number samples. Each complex
> +number consist of two parts called In-phase and Quadrature (IQ). Both I
> +and Q are represented as a 16 bit unsigned big endian number stored in
> +32 bit space. The remaining unused bits within the 32 bit space will be
> +padded with 0. I value starts first and Q value starts at an offset
> +equalling half of the buffer size (i.e.) offset = buffersize/2. Out of
> +the 16 bits, bit 15:2 (14 bit) is data and bit 1:0 (2 bit) can be any
> +value.
I've pinged Antti and Hans regarding single buffer vs. multiplanar, let's try
to reach an agreement there.
> +
> +**Byte Order.**
> +Each cell is one byte.
> +
> +.. flat-table::
> +:header-rows: 1
> +:stub-columns: 0
> +
> +* - Offset:
> +
In the meantime, you can remove all the blank lines between table rows :-)
> + - Byte B0
> +
> + - Byte B1
> +
> + - Byte B2
> +
> + - Byte B3
[snip]
--
Regards,
Laurent Pinchart
Re: [RFC 1/5] media: i2c: max2175: Add MAX2175 support
Hi Ramesh,
On Friday 21 Oct 2016 14:49:30 Ramesh Shanmugasundaram wrote:
> > On Wednesday 12 Oct 2016 15:10:25 Ramesh Shanmugasundaram wrote:
> >> This patch adds driver support for MAX2175 chip. This is Maxim
> >> Integrated's RF to Bits tuner front end chip designed for
> >> software-defined radio solutions. This driver exposes the tuner as a
> >> sub-device instance with standard and custom controls to configure the
> >> device.
> >>
> >> Signed-off-by: Ramesh Shanmugasundaram
> >> ---
> >>
> >> .../devicetree/bindings/media/i2c/max2175.txt | 60 +
> >> drivers/media/i2c/Kconfig |4 +
> >> drivers/media/i2c/Makefile |2 +
> >> drivers/media/i2c/max2175/Kconfig |8 +
> >> drivers/media/i2c/max2175/Makefile |4 +
> >> drivers/media/i2c/max2175/max2175.c| 1624 +
> >> drivers/media/i2c/max2175/max2175.h| 124 ++
> >> 7 files changed, 1826 insertions(+)
> >> create mode 100644
> >> Documentation/devicetree/bindings/media/i2c/max2175.txt
> >> create mode 100644 drivers/media/i2c/max2175/Kconfig create mode
> >> 100644 drivers/media/i2c/max2175/Makefile
> >> create mode 100644 drivers/media/i2c/max2175/max2175.c
> >> create mode 100644 drivers/media/i2c/max2175/max2175.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/max2175.txt
> >> b/Documentation/devicetree/bindings/media/i2c/max2175.txt new file
> >> mode 100644
> >> index 000..2250d5f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/max2175.txt
> >> @@ -0,0 +1,60 @@
[snip]
> >> +Optional properties:
> >> +
> >> +- maxim,slave: empty property indicates this is a slave of
> >> another
> >> + master tuner. This is used to define two tuners in
> >> + diversity mode (1 master, 1 slave). By default each
> >> + tuner is an individual master.
> >
> > Would it be useful to make that property a phandle to the master tuner, to
> > give drivers a way to access the master ? I haven't checked whether there
> > could be use cases for that.
>
> As of now, I cannot find any use case for it from the datasheet. In future,
> we could add if such need arise.
My point is that making the maxim,slave property a phandle now would allow
handling such future cases without any change to the DT bindings.
[snip]
> > > diff --git a/drivers/media/i2c/max2175/max2175.c
> > > b/drivers/media/i2c/max2175/max2175.c new file mode 100644 index
> > > 000..71b60c2
> > > --- /dev/null
> > > +++ b/drivers/media/i2c/max2175/max2175.c
[snip]
> > > +static int max2175_poll_csm_ready(struct max2175_ctx *ctx) {
> > > + return max2175_poll_timeout(ctx, 69, 1, 1, 0, 50);
> >
> > Please define macros for register addresses and values, this is just
> > unreadable.
>
> The Tuner provider is unwilling to disclose all register details. I agree on
> the readability issue with this restriction but this is somewhat true for
> some sensitive IPs in the media subsystem.
Is it the case that you don't have access to the information, or that you have
been forbidden to disclose them by the tuner manufacturer ?
> > > +}
[snip]
> > > +static int max2175_set_lo_freq(struct max2175_ctx *ctx, u64 lo_freq)
> > > +{
> > > + int ret;
> > > + u32 lo_mult;
> > > + u64 scaled_lo_freq;
> > > + const u64 scale_factor = 100ULL;
> > > + u64 scaled_npf, scaled_integer, scaled_fraction;
> > > + u32 frac_desired, int_desired;
> > > + u8 loband_bits, vcodiv_bits;
> >
> > Do you really support frequencies above 4GHz ?
>
> Nope.
>
> If not most of the 64-bit
>
> > values could be stored in 32 bits.
>
> The 64bit variables are needed to extract the fractional part (upto 6 digit
> precision) out of floating point divisions (original user space code).
OK. The code would be more efficient if you made the scaling factor a power of
two though. 1048576 could be a good value.
> >> +
> >> + scaled_lo_freq = lo_freq;
> >> + /* Scale to larger number for precision */
> >> + scaled_lo_freq = scaled_lo_freq * scale_factor * 100;
I just noticed that you could write the two lines as
scaled_lo_freq = lo_freq * scale_factor * 100;
By the way, why do you multiply by 100 here, and...
> >> + mxm_dbg(ctx, "scaled lo_freq %llu lo_freq %llu\n",
> >> + scaled_lo_freq, lo_freq);
> >> +
> >> + if (MAX2175_IS_BAND_AM(ctx)) {
> >> + if (max2175_get_bit(ctx, 5, 7) == 0)
> >> + loband_bits = 0;
> >> + vcodiv_bits = 0;
> >> + lo_mult = 16;
> >> + } else if (max2175_get_bits(ctx, 5, 1, 0) == MAX2175_BAND_FM) {
> >> + if (lo_freq <= 7470) {
> >> + loband_bits = 0;
> >> + vcodiv_bits = 0;
> >> + lo_mult = 16;
> >> + } else if ((lo_freq > 7470) && (lo_freq <= 11000)) {
> >
> > No need for the inner
Re: [PATCH v2] spi: rspi: rewrites the name of the function
Hi Mark. Thanks for your reply. On 11/09/2016 11:05 PM, Mark Brown wrote: On Tue, Nov 08, 2016 at 09:35:46AM +0900, Cao Minh Hiep wrote: From: Hiep Cao Minh This patch rewrites the name of rspi_pio_transfer_in_or_out function. This doesn't apply against current code, please check and resend. It's ok, we don't need this patch any more because of the Arnd Bergmann's "spi: rspi: avoid uninitialized variable access" patch. Thanks, Jinzai Solution Inc, Hiep.
Re: [PATCH 5/5] media: platform: rcar_drif: Add DRIF support
Hi Ramesh, Thank you for the patch. On Wednesday 09 Nov 2016 15:44:44 Ramesh Shanmugasundaram wrote: > This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3 SoCs. > The driver exposes each instance of DRIF as a V4L2 SDR device. A DRIF > device represents a channel and each channel can have one or two > sub-channels respectively depending on the target board. > > DRIF supports only Rx functionality. It receives samples from a RF > frontend tuner chip it is interfaced with. The combination of DRIF and the > tuner device, which is registered as a sub-device, determines the receive > sample rate and format. > > In order to be compliant as a V4L2 SDR device, DRIF needs to bind with > the tuner device, which can be provided by a third party vendor. DRIF acts > as a slave device and the tuner device acts as a master transmitting the > samples. The driver allows asynchronous binding of a tuner device that > is registered as a v4l2 sub-device. The driver can learn about the tuner > it is interfaced with based on port endpoint properties of the device in > device tree. The V4L2 SDR device inherits the controls exposed by the > tuner device. > > The device can also be configured to use either one or both of the data > pins at runtime based on the master (tuner) configuration. > > Signed-off-by: Ramesh Shanmugasundaram > --- > .../devicetree/bindings/media/renesas,drif.txt | 136 ++ > drivers/media/platform/Kconfig | 25 + > drivers/media/platform/Makefile|1 + > drivers/media/platform/rcar_drif.c | 1574 > 4 files changed, 1736 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/renesas,drif.txt > create mode 100644 drivers/media/platform/rcar_drif.c > > diff --git a/Documentation/devicetree/bindings/media/renesas,drif.txt > b/Documentation/devicetree/bindings/media/renesas,drif.txt new file mode > 100644 > index 000..d65368a > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt > @@ -0,0 +1,136 @@ > +Renesas R-Car Gen3 Digital Radio Interface controller (DRIF) > + > + > +R-Car Gen3 DRIF is a serial slave device. It interfaces with a master > +device as shown below > + > ++-++-+ > +| |-SCK--->|CLK | > +| Master|-SS>|SYNC DRIFn (slave) | > +| |-SD0--->|D0 | > +| |-SD1--->|D1 | > ++-++-+ > + > +Each DRIF channel (drifn) consists of two sub-channels (drifn0 & drifn1). > +The sub-channels are like two individual channels in itself that share the > +common CLK & SYNC. Each sub-channel has it's own dedicated resources like > +irq, dma channels, address space & clock. > + > +The device tree model represents the channel and each of it's sub-channel > +as a separate node. The parent channel ties the sub-channels together with > +their phandles. > + > +Required properties of a sub-channel: > +- > +- compatible: "renesas,r8a7795-drif" if DRIF controller is a part of > R8A7795 SoC. > + "renesas,rcar-gen3-drif" for a generic R-Car Gen3 compatible > device. > + When compatible with the generic version, nodes must list the > + SoC-specific version corresponding to the platform first > + followed by the generic version. > +- reg: offset and length of that sub-channel. > +- interrupts: associated with that sub-channel. > +- clocks: phandle and clock specifier of that sub-channel. > +- clock-names: clock input name string: "fck". > +- dmas: phandles to the DMA channel of that sub-channel. > +- dma-names: names of the DMA channel: "rx". > + > +Optional properties of a sub-channel: > +- > +- power-domains: phandle to the respective power domain. > + > +Required properties of a channel: > +- > +- pinctrl-0: pin control group to be used for this channel. > +- pinctrl-names: must be "default". > +- sub-channels : phandles to the two sub-channels. > + > +Optional properties of a channel: > +- > +- port: child port node of a channel that defines the local and remote > +endpoints. The remote endpoint is assumed to be a tuner subdevice > + endpoint. > +- renesas,syncmd : sync mode > + 0 (Frame start sync pulse mode. 1-bit width pulse > + indicates start of a frame) > + 1 (L/R sync or I2S mode) (default) > +- renesas,lsb-first: empty property indicates lsb bit is received > first. > + When not defined msb bit is received first (default) Shouldn't those two properties be
Re: [PATCH v2 0/7] soc: renesas: Identify SoC and register with the SoC bus
Hi Ulf,
On Wed, Nov 9, 2016 at 10:12 PM, Arnd Bergmann wrote:
> On Wednesday, November 9, 2016 6:19:06 PM CET Geert Uytterhoeven wrote:
>> On Wed, Nov 9, 2016 at 5:56 PM, Arnd Bergmann wrote:
>> > On Wednesday, November 9, 2016 2:34:33 PM CET Geert Uytterhoeven wrote:
>> >> > And Samsung.
>> >> > Shall I create the immutable branch now?
>> >>
>> >> Arnd: are you happy with the new patches and changes?
>> >
>> > I still had some comments for patch 7, but that shouldn't stop
>> > you from creating a branch for the first three so everyone can
>> > build on top of that.
>>
>> Thanks!
>>
>> What about patch [4/7]?
>> Haven't you received it? Your address was in the To-line for all 7 patches.
>
> Ok, I see it now, looks good. That should be included as well then.
Thanks, I've created the branch/tag :
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git
branch soc-device-match
signed tag soc-device-match-tag1
In the mean time, Ulf has applied the first two patches to mmc/next, on top
of lots of MMC work :-(
Ulf, as this is not only a dependency for Freescale/NXP (for sdhci-of-esdhc),
but also for Samsung and Renesas, would it still be possible to replace these
two commits
8b82c17a8ae533d6 base: soc: introduce soc_device_match() interface
6fa350172b098f0f base: soc: Check for NULL SoC device attributes
by a merge of soc-device-match-tag1?
You can find more info in the full thread at
https://www.spinics.net/lists/devicetree/msg148558.html
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v4 00/14] ARM: dts: r8a779x: use demuxer for I2C
Hi Wolfram, On Wed, Nov 09, 2016 at 09:59:54AM +0100, Wolfram Sang wrote: > Hi Simon, > > > I have tested these patches on alt, gose, lager and koelsch. > > Wow, that was quick. Thank you! > > > The switching part seems to work fine, in so far as my test script > > succeeds. However, it seems that some IP blocks are not able to handle > > this switching. In particular I needed to disable VIDEO_RCAR_VIN and > > REGULATOR_DA9210 to avoid errors shown in the logs below. > > Yes. Probably we should activate the shiny new DEBUG_TEST_DRIVER_REMOVE > and if that passes, we should be safe. > > > My suggestion is to drop the following patches until those problems > > can be sorted out, most likely via driver updates. > > > > ARM: dts: alt: use demuxer for I2C1 > > ARM: dts: gose: use demuxer for I2C2 > > ARM: dts: lager: use demuxer for IIC2/I2C2 > > ARM: dts: lager: use demuxer for IIC3/I2C3 > > ARM: dts: koelsch: use demuxer for I2C2 > > OK. I'll try to have a look at those drivers nonetheless, because > rebasing these patches is a bit of a hazzle once new i2c slaves were > added to the busses. But I'll juest resend the patches along with my > fixes if I really can find the time. > > > I am not in a position to test silk or porter at this time. > > But by the same reasoning above I wonder if the following should > > be dropped for now. > > > > ARM: dts: gose: use demuxer for I2C2 > > I assume you mean 'porter' here. > > > ARM: dts: silk: use demuxer for I2C1 As per our discussion on IRC this morning I have queued up the following. We can revisit the remaining patches once the issues described above are resolved one way or another. ARM: dts: alt: use demuxer for I2C4 ARM: dts: gose: use demuxer for I2C4 ARM: dts: koelsch: use demuxer for I2C4 ARM: dts: koelsch: use demuxer for I2C1 ARM: dts: lager: use demuxer for IIC1/I2C1 ARM: dts: lager: rename and reindex i2cexio
Re: [PATCH v4 01/14] ARM: dts: lager: rename and reindex i2cexio
On Mon, Nov 07, 2016 at 02:12:05PM +0100, Geert Uytterhoeven wrote: > On Sun, Nov 6, 2016 at 9:20 PM, Wolfram Sang > wrote: > > From: Simon Horman > > > > The rename from i2cexio to i2cexio0 is in in preparation for adding > > double "in" > > > i2cexio1 which will use the dmuxer for IIC1/I2C1. > > demuxer > > > The reindexing from i2c8 to i2c10 is to allow space for grouping of > > additional GPIO buses to added by follow-up patches to support demuxing of > > to be added > > > other i2c buses. > > > > Also note that fallback to GPIO is not provided by the hardware for > > IIC0/I2C0. Thanks Geert, I have fixed up the commit message as per your suggestions.
Re: [PATCH v2 7/7] soc: renesas: Identify SoC and register with the SoC bus
Hi Arnd,
Thanks for your comments!
On Wed, Nov 9, 2016 at 5:55 PM, Arnd Bergmann wrote:
> On Monday, October 31, 2016 12:30:55 PM CET Geert Uytterhoeven wrote:
>> v2:
>> - Drop SoC families and family names; use fixed "Renesas" instead,
>
> I think I'd rather have seen the family names left in there, but it's
> not important, so up to you.
They're not useful for matching, as family names may change anytime, and don't
always say much about the hardware capabilities.
E.g. SH-Mobile -> R-Mobile -> R-Car | RZ/A | RZ/G
Some SH-Mobile (even some R-Car) parts are SuperH only, others have ARM and
SuperH.
At least the SoC part numbers are stable (hmm, sh73a0 == r8a73a0).
>> - Use "renesas,prr" and "renesas,cccr" device nodes in DT if
>> available, else fall back to hardcoded addresses for compatibility
>> with existing DTBs,
>
> I only see patches 2, 3, 5, and 7 in my inbox, so I'm lacking the DT
> binding for these, among other things.
I understand you've received them in the mean time?
> It does seem wrong to have a device node for a specific register though.
> Shouldn't the node be for the block of registers that these are inside
> of?
On R-Mobile APE6, R-Car Gen2 and Gen3, PRR is a lone register.
On R-Car Gen1, it's not even documented (and doesn't exist on all parts).
On SH-Mobile/R-Mobile, CCCR may be part of the HPB/APB register block, which
we further don't touch at all.
On R-Car Gen2, it's not documented, but does exist.
BTW, see why I'd prefer not to have it in DT at all, and go for KISS in code
we can change at any time? To avoid mistakes we have to keep on supporting
forever.
>> - Don't register the SoC bus if the chip ID register is missing,
>
> Why? My objection was to hardcoding the register, not to registering
> the device? I think I'd rather see the device registered with an
> empty revision string.
If there's no chip ID register, there's no reason to use soc_device_match(),
as we can always look at a compatible value. All SoCs listed in this driver
have a chip ID register.
if you want me to register the soc_bus for those SoCs regardless, I want to
re-add r7s72100 (RZ/A) and r8a7778 (R-Car M1A), who don't have chip ID
registers ;-)
>> +#define CCCR 0xe600101c /* Common Chip Code Register */
>> +#define PRR0xff44 /* Product Register */
>> +#define PRR3 0xfff00044 /* Product Register on R-Car Gen3 */
>> +
>> +static const struct of_device_id renesas_socs[] __initconst = {
>> +#ifdef CONFIG_ARCH_R8A73A4
>> + { .compatible = "renesas,r8a73a4", .data = (void *)PRR, },
>> +#endif
>> +#ifdef CONFIG_ARCH_R8A7740
>> + { .compatible = "renesas,r8a7740", .data = (void *)CCCR, },
>> +#endif
>
> My preference here would be to list the register address only for
> SoCs that are known to need them, while also having .dtb files that
> don't have the nodes.
Even if drivers don't have to handle differences, there's been a long
outstanding request to print SoC revision information during bootup
(E.g. "Does it still work on ES1.0?"). Hence that covers all SoCs.
>> +static int __init renesas_soc_init(void)
>> +{
>> + struct soc_device_attribute *soc_dev_attr;
>> + const struct of_device_id *match;
>> + void __iomem *chipid = NULL;
>> + struct soc_device *soc_dev;
>> + struct device_node *np;
>> + unsigned int product;
>> +
>> + np = of_find_matching_node_and_match(NULL, renesas_socs, &match);
>> + if (!np)
>> + return -ENODEV;
>> +
>> + of_node_put(np);
>> +
>> + /* Try PRR first, then CCCR, then hardcoded fallback */
>> + np = of_find_compatible_node(NULL, NULL, "renesas,prr");
>> + if (!np)
>> + np = of_find_compatible_node(NULL, NULL, "renesas,cccr");
>> + if (np) {
>> + chipid = of_iomap(np, 0);
>> + of_node_put(np);
>> + } else if (match->data) {
>> + chipid = ioremap((uintptr_t)match->data, 4);
>> + }
>> + if (!chipid)
>>
>
> Here, I'd turn the order around and look for the DT nodes of the
> devices first. Only if they are not found, look at the compatible
> string of the root node. No need to search for a node though,
> you know which one it is when you look for a compatible =
> "renesas,r8a73a4".
"renesas,r8a73a4" is the root node, not the device, so it does not have the
"reg" property for reading the chip ID?
There is no SoC part number in the "renesas,prr" and "renesas,cccr" nodes.
Hence I always need to look at the root nodes.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 0/7] soc: renesas: Identify SoC and register with the SoC bus
On Thu, Nov 10, 2016 at 10:22 AM, Geert Uytterhoeven
wrote:
> Thanks, I've created the branch/tag :
>
> git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git
> branch soc-device-match
> signed tag soc-device-match-tag1
Tested by kbuild test robot:
https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git
soc-device-match
da65a1589dacc7ec44ea0557a14d70a39d991f32 base: soc: Provide a
dummy implementation of soc_device_match()
elapsed time: 101m
configs tested: 85
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 0/7] soc: renesas: Identify SoC and register with the SoC bus
On 10 November 2016 at 10:22, Geert Uytterhoeven wrote: > Hi Ulf, > > On Wed, Nov 9, 2016 at 10:12 PM, Arnd Bergmann wrote: >> On Wednesday, November 9, 2016 6:19:06 PM CET Geert Uytterhoeven wrote: >>> On Wed, Nov 9, 2016 at 5:56 PM, Arnd Bergmann wrote: >>> > On Wednesday, November 9, 2016 2:34:33 PM CET Geert Uytterhoeven wrote: >>> >> > And Samsung. >>> >> > Shall I create the immutable branch now? >>> >> >>> >> Arnd: are you happy with the new patches and changes? >>> > >>> > I still had some comments for patch 7, but that shouldn't stop >>> > you from creating a branch for the first three so everyone can >>> > build on top of that. >>> >>> Thanks! >>> >>> What about patch [4/7]? >>> Haven't you received it? Your address was in the To-line for all 7 patches. >> >> Ok, I see it now, looks good. That should be included as well then. > > Thanks, I've created the branch/tag : > > git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git > branch soc-device-match > signed tag soc-device-match-tag1 > > In the mean time, Ulf has applied the first two patches to mmc/next, on top > of lots of MMC work :-( No worries! :-) > > Ulf, as this is not only a dependency for Freescale/NXP (for sdhci-of-esdhc), > but also for Samsung and Renesas, would it still be possible to replace these > two commits > > 8b82c17a8ae533d6 base: soc: introduce soc_device_match() interface > 6fa350172b098f0f base: soc: Check for NULL SoC device attributes > > by a merge of soc-device-match-tag1? Yes, I will take care of it during the day. Kind regards Uffe
Re: [PATCH v2 7/7] soc: renesas: Identify SoC and register with the SoC bus
On Thursday, November 10, 2016 11:19:20 AM CET Geert Uytterhoeven wrote:
> Hi Arnd,
>
> Thanks for your comments!
>
> On Wed, Nov 9, 2016 at 5:55 PM, Arnd Bergmann wrote:
> > On Monday, October 31, 2016 12:30:55 PM CET Geert Uytterhoeven wrote:
> >> v2:
> >> - Drop SoC families and family names; use fixed "Renesas" instead,
> >
> > I think I'd rather have seen the family names left in there, but it's
> > not important, so up to you.
>
> They're not useful for matching, as family names may change anytime, and don't
> always say much about the hardware capabilities.
> E.g. SH-Mobile -> R-Mobile -> R-Car | RZ/A | RZ/G
> Some SH-Mobile (even some R-Car) parts are SuperH only, others have ARM and
> SuperH.
>
> At least the SoC part numbers are stable (hmm, sh73a0 == r8a73a0).
I think the marketing names are much more useful for humans looking
at the sysfs files than the kernel doing matching on, but both use
cases are important.
> >> - Use "renesas,prr" and "renesas,cccr" device nodes in DT if
> >> available, else fall back to hardcoded addresses for compatibility
> >> with existing DTBs,
> >
> > I only see patches 2, 3, 5, and 7 in my inbox, so I'm lacking the DT
> > binding for these, among other things.
>
> I understand you've received them in the mean time?
Yes, I found them in my inbox later, not sure why I didn't see them
at first.
> > It does seem wrong to have a device node for a specific register though.
> > Shouldn't the node be for the block of registers that these are inside
> > of?
>
> On R-Mobile APE6, R-Car Gen2 and Gen3, PRR is a lone register.
> On R-Car Gen1, it's not even documented (and doesn't exist on all parts).
It just seems odd to have it at address 0xff44 when all the other
devices are at page-aligned addresses. Do you mean that accessing
0xff40 or 0xff48 will result in a bus-level exception for a
missing register and just 0xff44 is actually valid for access,
or is it just the only thing that is documented?
> On SH-Mobile/R-Mobile, CCCR may be part of the HPB/APB register block, which
> we further don't touch at all.
> On R-Car Gen2, it's not documented, but does exist.
This is where the family names would come in handy ;-) I now have
no idea which chip(s) you are referring to.
If you know the name of the register block, just put it into DT with
that name. The driver can trivially add the right offset.
> >> - Don't register the SoC bus if the chip ID register is missing,
> >
> > Why? My objection was to hardcoding the register, not to registering
> > the device? I think I'd rather see the device registered with an
> > empty revision string.
>
> If there's no chip ID register, there's no reason to use soc_device_match(),
> as we can always look at a compatible value. All SoCs listed in this driver
> have a chip ID register.
But you may still have user space tools looking into sysfs, e.g. to
figure out how to install a kernel that the boot loader can find,
or which hardware specific distro packages to install.
> if you want me to register the soc_bus for those SoCs regardless, I want to
> re-add r7s72100 (RZ/A) and r8a7778 (R-Car M1A), who don't have chip ID
> registers ;-)
Right. Just don't encode too much knowledge about the SoCs into the
driver, so we are prepared for adding new ones: We should still look
for the registers in DT on all chips.
> >> +#define CCCR 0xe600101c /* Common Chip Code Register */
> >> +#define PRR0xff44 /* Product Register */
> >> +#define PRR3 0xfff00044 /* Product Register on R-Car Gen3 */
> >> +
> >> +static const struct of_device_id renesas_socs[] __initconst = {
> >> +#ifdef CONFIG_ARCH_R8A73A4
> >> + { .compatible = "renesas,r8a73a4", .data = (void *)PRR, },
> >> +#endif
> >> +#ifdef CONFIG_ARCH_R8A7740
> >> + { .compatible = "renesas,r8a7740", .data = (void *)CCCR, },
> >> +#endif
> >
> > My preference here would be to list the register address only for
> > SoCs that are known to need them, while also having .dtb files that
> > don't have the nodes.
>
> Even if drivers don't have to handle differences, there's been a long
> outstanding request to print SoC revision information during bootup
> (E.g. "Does it still work on ES1.0?"). Hence that covers all SoCs.
Ok, fair enough.
> >> +static int __init renesas_soc_init(void)
> >> +{
> >> + struct soc_device_attribute *soc_dev_attr;
> >> + const struct of_device_id *match;
> >> + void __iomem *chipid = NULL;
> >> + struct soc_device *soc_dev;
> >> + struct device_node *np;
> >> + unsigned int product;
> >> +
> >> + np = of_find_matching_node_and_match(NULL, renesas_socs, &match);
> >> + if (!np)
> >> + return -ENODEV;
> >> +
> >> + of_node_put(np);
> >> +
> >> + /* Try PRR first, then CCCR, then hardcoded fallback */
> >> + np = of_find_compatible_node(NULL, NULL, "renesas,prr");
> >> + if (!np)
> >> + np = o
Re: [PATCH v6 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V6
On Thu, Oct 20, 2016 at 08:35:33AM +0900, Magnus Damm wrote: > iommu/ipmmu-vmsa: IPMMU multi-arch update V6 > > [PATCH v6 01/07] iommu/ipmmu-vmsa: Remove platform data handling > [PATCH v6 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for > context > [PATCH v6 03/07] iommu/ipmmu-vmsa: Break out utlb parsing code > [PATCH v6 04/07] iommu/ipmmu-vmsa: Break out domain allocation code > [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops > [PATCH v6 06/07] iommu/ipmmu-vmsa: ARM and ARM64 archdata access > [PATCH v6 07/07] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency Applied to arm/renesas, thanks.
Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
On Fri, Oct 21, 2016 at 06:52:53PM +0100, Robin Murphy wrote:
> > -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> > -{
> > - if (type != IOMMU_DOMAIN_UNMANAGED)
> > - return NULL;
>
> I *think* that if we did the initial check thus:
>
> if (type != IOMMU_DOMAIN_UNMANAGED ||
> (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
> return NULL;
>
> it shouldn't be necessary to split the function at all - we then just
> wrap the {get,put}_cookie() bits in "if (type == IOMMU_DOMAIN_DMA)" and
> in the 32-bit ARM case they just don't run as that can never be true.
This would be a good improvement. Magnus, Robin, can either of you send
a follow-on patch to implement this suggestion? I have applied these
patches to my arm/renesas branch (not pushed yet). The patch can be
based on it.
Joerg
Re: [PATCH v8 0/6] UHS-I SDR-104 support for sh_mobile_sdhi
On Thu, Nov 03, 2016 at 03:15:58PM +0100, Simon Horman wrote: > Hi, > > this series is based on work by Ai Kyuse to add UHS-I SDR-104 support for > sh_mobile_sdhi. It builds on work by Shinobu Uehara, Rob Taylor, William > Towle and Ian Molton, Ben Hutchings, Wolfram Sang and others to add UHS-I > SDR-50 support to the same driver. > > It is based on the next branch of the mmc tree. > > To aid review the following git branch is provided: > * https:://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git > topic/sdr104-driver-v8 > > Overview of changes since v7: > * Correct inverted logic in sh_mobile_sdhi_hw_reset > * Correct value of SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN > > Please see http://elinux.org/Tests:SD-SDHI-SDR104 for indicative tests > results. Successfully tested on a Lager Board with a SanDisk card and a Samsung card which used to cause issues before. Tested-by: Wolfram Sang Ulf, I think this series is ready to go for v4.10. All issues we know of are related to pinmuxing and will be solved elsewhere. We currently have hacks for that so it is quite clear the issues are not caused by this series. signature.asc Description: PGP signature
Re: [PATCH v8 0/3] ARM, arm64: renesas: Enable UHS-I SDR-104
On Thu, Nov 03, 2016 at 04:07:22PM +0100, Simon Horman wrote: > Hi, > > this series enables SDHI UHS-I SDR-104 on: > * r8a7790/lager > * r8a7791/koelsch > * r8a7794/alt > > It is based on renesas-next-20161102-v4.9-rc1. For the whole series: Reviewed-by: Wolfram Sang For Lager: Tested-by: Wolfram Sang signature.asc Description: PGP signature
Re: [PATCH v4] clk: renesas: cpg-mssr: add common R-Car Gen2 support
Hi Sergei,
On Tue, Nov 8, 2016 at 10:17 PM, Sergei Shtylyov
wrote:
> Add the common R-Car Gen2 (and RZ/G) Clock Pulse Generator / Module Standby
> and Software Reset support code, using the CPG/MSSR driver core.
>
> Based on the proof-of-concept R8A7791 CPG/MSSR patch by Geert Uytterhoeven
> .
>
> Signed-off-by: Sergei Shtylyov
> Reviewed-by: Geert Uytterhoeven
>
> ---
> This patch is against the 'clk-next' branch of CLK group's 'linux.git' repo.
>
> Changes in version 4:
> - added a static variable for the PLL0 divisor and a parameter of the same
> kind
> to rcar_gen2_cpg_init(), used that variable in
> rcar_gen2_cpg_clk_register().
Thanks, I've verified correct operation by using your r8a7743 driver on
r8a7791/koelsch, and comparing /sys/kernel/debug/clk/clk_summary.
> Changes in version 3:
> - removed the divisor logic from the PLL0 related code;
> - also mentioned R-Car V2H as not having the PLL0CR register.
>
> Changes in version 2:
> - added support for non-existing PLL0CR;
> - removed the function reading the mode pins;
> - added/used the #define's for PLL0CR.STC;
> - used CPG_FRQCRC_ZFC_SHIFT to #define CPG_FRQCRC_ZFC_MASK;
> - removed rcar_gen2_read_modemr();
> - added Geert's tag.
>
> drivers/clk/renesas/rcar-gen2-cpg.c | 370
>
> drivers/clk/renesas/rcar-gen2-cpg.h | 43
> 2 files changed, 413 insertions(+)
>
> Index: linux/drivers/clk/renesas/rcar-gen2-cpg.c
> ===
> --- /dev/null
> +++ linux/drivers/clk/renesas/rcar-gen2-cpg.c
> +static u32 cpg_mode __initdata;
cpg_mode is never set.
> +
> +struct clk * __init rcar_gen2_cpg_clk_register(struct device *dev,
> + const struct cpg_core_clk
> *core,
> + const struct cpg_mssr_info
> *info,
> + struct clk **clks,
> + void __iomem *base)
> +{
> + switch (core->type) {
> + case CLK_TYPE_GEN2_LB:
> + div = cpg_mode & BIT(18) ? 36 : 24;
Hence div may be incorrect here (it wasn't for me)...
> + case CLK_TYPE_GEN2_QSPI:
> + div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2) ?
> + 8 : 10;
... and here (it was for me).
> +int __init rcar_gen2_cpg_init(const struct rcar_gen2_cpg_pll_config *config,
> + unsigned int pll0_div)
I'll add a "u32 mode" parameter here, and pass it from
r8a774[35]_cpg_mssr_init().
> +{
> + cpg_pll_config = config;
> + cpg_pll0_div = pll0_div;
cpg_mode = mode;
No need to resend, fixing it up myself.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v6] clk: renesas: cpg-mssr: add R8A7743 support
Hi Sergei,
On Tue, Nov 8, 2016 at 10:21 PM, Sergei Shtylyov
wrote:
> Add RZ/G1M (R8A7743) Clock Pulse Generator / Module Standby and Software
> Reset support, using the CPG/MSSR driver core and the common R-Car Gen2
> (and RZ/G) code.
>
> Based on the proof-of-concept R8A7791 CPG/MSSR patch by Geert Uytterhoeven
> .
>
> Signed-off-by: Sergei Shtylyov
> Acked-by: Rob Herring
> Reviewed-by: Geert Uytterhoeven
>
> ---
> This patch is against the 'clk-next' branch of CLK group's 'linux.git' repo.
> It depends on the common R-Car gen2 (and RZ/G) support just posted.
>
> Changes in version 6:
> - passed the PLL0 divisor to rcar_gen2_cpg_init();
> - undid the version 5 changes.
Thanks!
Will queue with the following trivial changes:
- pass cpg_mode to rcar_gen2_cpg_init(),
- sort clock names in DT bindings,
- reformat tables for easier comparison with other clock drivers.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v4] clk: renesas: cpg-mssr: add R8A7745 support
On Tue, Nov 8, 2016 at 10:25 PM, Sergei Shtylyov
wrote:
> Add RZ/G1E (R8A7745) Clock Pulse Generator / Module Standby and Software
> Reset support, using the CPG/MSSR driver core and the common R-Car Gen2
> (and RZ/G) code.
>
> Based on the proof-of-concept R8A7791 CPG/MSSR patch by Geert Uytterhoeven
> .
>
> Signed-off-by: Sergei Shtylyov
> Reviewed-by: Geert Uytterhoeven
>
> ---
> This patch is against the 'clk-next' branch of CLK group's 'linux.git' repo
> plus the R8A7743 clock driver patch. It depends on the common R-Car gen2 (and
> RZ/G) support just posted.
>
> Changes in version 4:
> - changed the Z2 clock's divisor to 1;
> - passed the PLL0 divisor to rcar_gen2_cpg_init();
> - renamed the ACP clock to CPEX;
> - removed the thermal module clock.
Thanks!
Will queue with the following trivial changes:
- pass cpg_mode to rcar_gen2_cpg_init(),
- sort clock names in DT bindings,
- reformat tables for easier comparison with other clock drivers.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v4 00/14] ARM: dts: r8a779x: use demuxer for I2C
Hi Simon and Wolfram, Following up with my findings based on our IRC discussion. On 2016-11-09 09:44:07 +0100, Simon Horman wrote: > Hi Wolfram, > > On Sun, Nov 06, 2016 at 09:20:18PM +0100, Wolfram Sang wrote: > > So, here is the newest series for using the I2C demuxer on Gen2 boards. > > Initially done by Simon. The intention of this series is to extend use of > > the > > demuxer for I2C on the lager, koelsch, porter, koelsch, alt and silk boards > > to > > cover all I2C IP blocks that are either already used or exposed via an EXIO > > connector. > > > > I tested this on a Lager board where I could successfully switch between > > I2C, > > IIC, and GPIO on I2C2. > > > > Simon, can you test with your script on the other boards? If all works, I'll > > pick up the i2c patch for 4.9, so the DTS changes should be fine for 4.10. > > > > The branch is here: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git > > renesas/ip-switch-rework > > I have tested these patches on alt, gose, lager and koelsch. > The switching part seems to work fine, in so far as my test script > succeeds. However, it seems that some IP blocks are not able to handle > this switching. In particular I needed to disable VIDEO_RCAR_VIN and > REGULATOR_DA9210 to avoid errors shown in the logs below. I tested the unbind/bind cycle we talked about, I'm testing on Koelsch using renesas-drivers-2016-11-08-v4.9-rc4 and shmobile_defconifg with the following additions: # needed for systemd init... CONFIG_CGROUPS=y CONFIG_SECCOMP=y # needed to grab large frame sizes CONFIG_CMA=y CONFIG_CMA_SIZE_MBYTES=64 # unrelated but is need for the HDMI input CONFIG_VIDEO_ADV7604=y qv4l2 -d /dev/video26 # video works fine echo 2-0020 > /sys/bus/i2c/drivers/adv7180/unbind # Any attempt to open /dev/video26 fails with EBUSY, this is expect # when it do not have all devices available. echo 2-0020 > /sys/bus/i2c/drivers/adv7180/bind qv4l2 -d /dev/video26 # video works fine again The VIN driver issue I mention on IRC was my fault, I tested an old build which did not contain a fix for rcar-vin Gen2. So the conclusion is that the adv7180 and rcar-vin in renesas-drivers-2016-11-08-v4.9-rc4 can handle the rebind cycle. But looking at the boot logs it looks like you are using an older version of the rcar-vin driver, see bellow. > > My suggestion is to drop the following patches until those problems > can be sorted out, most likely via driver updates. > > ARM: dts: alt: use demuxer for I2C1 > ARM: dts: gose: use demuxer for I2C2 > ARM: dts: lager: use demuxer for IIC2/I2C2 > ARM: dts: lager: use demuxer for IIC3/I2C3 > ARM: dts: koelsch: use demuxer for I2C2 > > I am not in a position to test silk or porter at this time. > But by the same reasoning above I wonder if the following should > be dropped for now. > > ARM: dts: gose: use demuxer for I2C2 > ARM: dts: silk: use demuxer for I2C1 > > Some boot logs follow: > > board: koelsch > config: shmobile_defconfig; VIDEO_RCAR_VIN not set > # ./exercise-i2c-demux.sh > ./exercise-i2c-demux.sh > I2C Demux: i2c-12. Master: 1:/i2c-9 (1) > [ 60.782245] i2c-gpio i2c-9: using pins 796 (SDA) and 795 (SCL) > I2C Demux: i2c-12. Master: 0:/i2c@e6518000 (0) > [ 66.812087] i2c-rcar e6518000.i2c: probed > I2C Demux: i2c-13. Master: 1:/i2c-10 (1) > [ 72.853085] i2c-gpio i2c-10: using pins 941 (SDA) and 940 (SCL) > [ 72.860037] adv7180 13-0020: chip found @ 0x20 (i2c-demux (master i2c-10)) > [ 72.876990] i2c i2c-10: sendbytes: NAK bailout. > [ 72.913992] at24 13-0050: 256 byte 24c02 EEPROM, writable, 16 bytes/write > I2C Demux: i2c-13. Master: 0:/i2c@e653 (0) > [ 78.942593] i2c-rcar e653.i2c: probed > [ 78.947595] adv7180 13-0020: chip found @ 0x20 (i2c-demux (master i2c-2)) > [ 78.979763] at24 13-0050: 256 byte 24c02 EEPROM, writable, 16 bytes/write > I2C Demux: i2c-14. Master: 1:/i2c-11 (1) > [ 85.022244] i2c-gpio i2c-11: using pins 794 (SDA) and 793 (SCL) > I2C Demux: i2c-14. Master: 0:/i2c@e652 (0) > [ 91.052892] i2c-rcar e652.i2c: probed > > board: koelsch > config: shmobile_defconfig > # ./exercise-i2c-demux.sh > I2C Demux: i2c-12. Master: 1:/i2c-9 (1) > [ 44.742363] i2c-gpio i2c-9: using pins 796 (SDA) and 795 (SCL) > I2C Demux: i2c-12. Master: 0:/i2c@e6518000 (0) > [ 51.591786] i2c-rcar e6518000.i2c: probed > I2C Demux: i2c-13. Master: 1:/i2c-10 (1) > [ 57.630256] rcar-vin e6ef1000.video: Removing video25 This line tells me you are using the rcar-vin without the Gen3 patches. Part of Gen3 enablement is to rework the bind/unbind handling to be smarter since there are more subdevices involved. Can you rerun the tests on top of renesas-drivers-2016-11-08-v4.9-rc4? That branch contains the VIN Gen3 patches. > [ 57.638391] i2c-gpio i2c-10: using pins 941 (SDA) and 940 (SCL) > [ 57.646081] adv7180 13-0020: chip found @ 0x20 (i2c-demux (master i2c-10)) > [ 57.663185] i2c i2c-10: sendbytes: NAK bailout. > [ 57.682324] kobject (ee422
Re: [PATCH v4 00/14] ARM: dts: r8a779x: use demuxer for I2C
Niklas, all, > Yes, this is a bug in the rcar-vin driver which is addressed in the Gen3 > patches. However I'm not sure those patches will make it to v4.10, not > much review from the V4L2 side yet (Geert and Sergei have had a few > comments so there will at lest be one more iteration). > > If this is a big blocker I can try and break out the fix from the Gen3 > patch series, but I fear even that series would be quiet large since a > rework on the whole logic is needed to fix this (maybe a workaround can > be figure out to fix this until Gen3 patches are accepted). Let me know > what you think. Thanks for your investigation and heads up! I think we should do the following: * upstream the patches with no issues for v4.10 (Simon started this already) * rebase the demux branch on top of the one you mentioned with the vin fixes and retest * if all works in renesas-drivers, then just wait until vin patches are upstream and then we upstream the hdmi-demux-patches And orthogonal to that: * keep discussing what to do with DFVS (postponed to next week for me because of other duties) Sounds good? Wolfram signature.asc Description: PGP signature
Re: [PATCH v4 14/14] i2c: mux: demux-pinctrl: make drivers with no pinctrl work again
On Sun, Nov 06, 2016 at 09:20:32PM +0100, Wolfram Sang wrote: > Some drivers like i2c-gpio do not have dedicated pinctrl states. They > broke when error checking for pinctrl was added. Detect them now, and in > their case, simply skip over pinctrl configuration. > > Signed-off-by: Wolfram Sang Applied to for-current with Geert's typo fix, thanks! signature.asc Description: PGP signature
Re: [PATCH v8 0/6] UHS-I SDR-104 support for sh_mobile_sdhi
On 10 November 2016 at 12:45, Wolfram Sang wrote: > On Thu, Nov 03, 2016 at 03:15:58PM +0100, Simon Horman wrote: >> Hi, >> >> this series is based on work by Ai Kyuse to add UHS-I SDR-104 support for >> sh_mobile_sdhi. It builds on work by Shinobu Uehara, Rob Taylor, William >> Towle and Ian Molton, Ben Hutchings, Wolfram Sang and others to add UHS-I >> SDR-50 support to the same driver. >> >> It is based on the next branch of the mmc tree. >> >> To aid review the following git branch is provided: >> * https:://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git >> topic/sdr104-driver-v8 >> >> Overview of changes since v7: >> * Correct inverted logic in sh_mobile_sdhi_hw_reset >> * Correct value of SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN >> >> Please see http://elinux.org/Tests:SD-SDHI-SDR104 for indicative tests >> results. > > Successfully tested on a Lager Board with a SanDisk card and a Samsung > card which used to cause issues before. > > Tested-by: Wolfram Sang > > Ulf, I think this series is ready to go for v4.10. All issues we know of > are related to pinmuxing and will be solved elsewhere. We currently have > hacks for that so it is quite clear the issues are not caused by this > series. > Thanks, applied for next! I did have to fix conflict and also to resolve some checkpatch warnings - but no worries this time! Kind regards Uffe
Re: [git pull] clk: renesas: Updates for v4.10 (take one)
On 11/07, Geert Uytterhoeven wrote: > clk: renesas: Updates for v4.10 (take one) > > - SYS-DMAC, (H)SCIF, I2C, DRIF, and graphics related clocks for R-Car > M3-W, > - Minor fixes and cleanups. > > Thanks for pulling! > > Geert Uytterhoeven (4): > clk: renesas: cpg-mssr: Always use readl()/writel() > clk: renesas: rcar-gen3-cpg: Always use readl()/writel() > clk: renesas: cpg-mssr: Fix inverted debug check Hm.. I already applied this one to clk-next. If you're going to apply patches can you please indicate if they've been picked up and/or not send them To: Mike and I, please? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v6 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context
Hi Magnus,
Thank you for the patch.
On Thursday 20 Oct 2016 08:35:53 Magnus Damm wrote:
> From: Magnus Damm
>
> Introduce a bitmap for context handing and convert the
> interrupt routine to handle all registered contexts.
>
> At this point the number of contexts are still limited.
>
> Also remove the use of the ARM specific mapping variable
> from ipmmu_irq() to allow compile on ARM64.
>
> Signed-off-by: Magnus Damm
> Reviewed-by: Joerg Roedel
> ---
>
> Changes since V5:
> - None
>
> Changes since V4:
> - None
>
> Changes since V3:
> - None
>
> Changes since V2: (Thanks again to Laurent!)
> - Introduce a spinlock together with the bitmap and domain array.
> - Break out code into separate functions for alloc and free.
> - Perform free after (instead of before) configuring hardware registers.
> - Use the spinlock to protect the domain array in the interrupt handler.
>
> Changes since V1: (Thanks to Laurent for feedback!)
> - Use simple find_first_zero()/set_bit()/clear_bit() for context
> management. - For allocation rely on spinlock held when calling
> ipmmu_domain_init_context() - For test/free use atomic bitops
> - Return IRQ_HANDLED if any of the contexts generated interrupts
>
> drivers/iommu/ipmmu-vmsa.c | 76 +++--
> 1 file changed, 66 insertions(+), 10 deletions(-)
>
> --- 0004/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:48:23.770607110 +0900
> @@ -8,6 +8,7 @@
> * the Free Software Foundation; version 2 of the License.
> */
>
> +#include
> #include
> #include
> #include
> @@ -26,12 +27,17 @@
>
> #include "io-pgtable.h"
>
> +#define IPMMU_CTX_MAX 1
> +
> struct ipmmu_vmsa_device {
> struct device *dev;
> void __iomem *base;
> struct list_head list;
>
> unsigned int num_utlbs;
> + spinlock_t lock;/* Protects ctx and domains[]
*/
> + DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> + struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
>
> struct dma_iommu_mapping *mapping;
> };
> @@ -293,9 +299,29 @@ static struct iommu_gather_ops ipmmu_gat
> * Domain/Context Management
> */
>
> +static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu,
> + struct ipmmu_vmsa_domain *domain)
Nitpicking, I'd name this ipmmu_domain_alloc_context() as the driver uses the
alloc abbreviation already.
> +{
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&mmu->lock, flags);
> +
> + ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX);
> + if (ret != IPMMU_CTX_MAX) {
> + mmu->domains[ret] = domain;
> + set_bit(ret, mmu->ctx);
> + }
How about returning a negative error code on error instead of IPMMU_CTX_MAX ?
I think it would make the API clearer, avoiding the need to think about
special error handling for this function.
Having said that, I find that the init/alloc and destroy/free function names
don't carry a very clear semantic. Given the size of the alloc and free
functions, how about inlining them in their single caller ?
> +
> + spin_unlock_irqrestore(&mmu->lock, flags);
> +
> + return ret;
> +}
> +
> static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> {
> u64 ttbr;
> + int ret;
>
> /*
>* Allocate the page table operations.
> @@ -325,10 +351,15 @@ static int ipmmu_domain_init_context(str
> return -EINVAL;
>
> /*
> - * TODO: When adding support for multiple contexts, find an unused
> - * context.
> + * Find an unused context.
>*/
The comment now holds on one line.
> - domain->context_id = 0;
> + ret = ipmmu_domain_allocate_context(domain->mmu, domain);
> + if (ret == IPMMU_CTX_MAX) {
> + free_io_pgtable_ops(domain->iop);
> + return -EBUSY;
> + }
> +
> + domain->context_id = ret;
>
> /* TTBR0 */
> ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
> @@ -370,6 +401,19 @@ static int ipmmu_domain_init_context(str
> return 0;
> }
>
> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> + unsigned int context_id)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mmu->lock, flags);
> +
> + clear_bit(context_id, mmu->ctx);
> + mmu->domains[context_id] = NULL;
> +
> + spin_unlock_irqrestore(&mmu->lock, flags);
> +}
> +
> static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
> {
> /*
> @@ -380,6 +424,7 @@ static void ipmmu_domain_destroy_context
>*/
> ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
> ipmmu_tlb_sync(domain);
> + ipmmu_domain_free_context(domain->mmu, domain->context_id);
> }
>
> /*
> ---
> -- @@ -437,16 +482,25 @@ static irqreturn_t ipmmu_domain_irq(stru
> sta
Re: [PATCH v6 03/07] iommu/ipmmu-vmsa: Break out utlb parsing code
Hi Magnus,
Thank you for the patch.
On Thursday 20 Oct 2016 08:36:03 Magnus Damm wrote:
> From: Magnus Damm
>
> Break out the utlb parsing code and dev_data allocation into a
> separate function. This is preparation for future code sharing.
>
> Signed-off-by: Magnus Damm
> Reviewed-by: Joerg Roedel
> ---
>
> Changes since V5:
> - None
>
> Changes since V4:
> - Dropped hunk with fix to apply on top of:
>b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device
>
> Changes since V3:
> - Initialize "mmu" to NULL, check before accessing
> - Removed group parameter from ipmmu_init_platform_device()
>
> Changes since V2:
> - Included this new patch from the following series:
>[PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
> - Reworked code to fit on top on previous two patches in current series.
>
> drivers/iommu/ipmmu-vmsa.c | 58 ++---
> 1 file changed, 37 insertions(+), 21 deletions(-)
>
> --- 0006/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:49:34.580607110 +0900
> @@ -647,22 +647,15 @@ static int ipmmu_find_utlbs(struct ipmmu
> return 0;
> }
>
> -static int ipmmu_add_device(struct device *dev)
> +static int ipmmu_init_platform_device(struct device *dev)
The device doesn't have to be a platform device, how about calling this
ipmmu_init_device_archdata() ?
> {
> struct ipmmu_vmsa_archdata *archdata;
> struct ipmmu_vmsa_device *mmu;
> - struct iommu_group *group = NULL;
> unsigned int *utlbs;
> unsigned int i;
> int num_utlbs;
> int ret = -ENODEV;
>
> - if (dev->archdata.iommu) {
> - dev_warn(dev, "IOMMU driver already assigned to device %s\n",
> - dev_name(dev));
> - return -EINVAL;
> - }
> -
> /* Find the master corresponding to the device. */
>
> num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
> @@ -699,6 +692,36 @@ static int ipmmu_add_device(struct devic
> }
> }
>
> + archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
> + if (!archdata) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + archdata->mmu = mmu;
> + archdata->utlbs = utlbs;
> + archdata->num_utlbs = num_utlbs;
> + dev->archdata.iommu = archdata;
> + return 0;
> +
> +error:
> + kfree(utlbs);
> + return ret;
> +}
> +
> +static int ipmmu_add_device(struct device *dev)
> +{
> + struct ipmmu_vmsa_archdata *archdata;
> + struct ipmmu_vmsa_device *mmu = NULL;
> + struct iommu_group *group;
> + int ret;
> +
> + if (dev->archdata.iommu) {
> + dev_warn(dev, "IOMMU driver already assigned to device %s\n",
> + dev_name(dev));
> + return -EINVAL;
> + }
> +
> /* Create a device group and add the device to it. */
> group = iommu_group_alloc();
> if (IS_ERR(group)) {
> @@ -716,16 +739,9 @@ static int ipmmu_add_device(struct devic
> goto error;
> }
>
> - archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
> - if (!archdata) {
> - ret = -ENOMEM;
> + ret = ipmmu_init_platform_device(dev);
> + if (ret < 0)
> goto error;
> - }
> -
> - archdata->mmu = mmu;
> - archdata->utlbs = utlbs;
> - archdata->num_utlbs = num_utlbs;
> - dev->archdata.iommu = archdata;
>
> /*
>* Create the ARM mapping, used by the ARM DMA mapping core to
allocate
> @@ -736,6 +752,8 @@ static int ipmmu_add_device(struct devic
>* - Make the mapping size configurable ? We currently use a 2GB
mapping
>* at a 1GB offset to ensure that NULL VAs will fault.
>*/
> + archdata = dev->archdata.iommu;
> + mmu = archdata->mmu;
> if (!mmu->mapping) {
> struct dma_iommu_mapping *mapping;
>
> @@ -760,10 +778,8 @@ static int ipmmu_add_device(struct devic
> return 0;
>
> error:
> - arm_iommu_release_mapping(mmu->mapping);
> -
> - kfree(dev->archdata.iommu);
> - kfree(utlbs);
> + if (mmu)
> + arm_iommu_release_mapping(mmu->mapping);
Looking at the surrounding code, shouldn't the mapping only be destroyed here
if it has been created by the same call to the function ? If there's an
existing mapping for the IPMMU instance and the arm_iommu_attach_device() call
fails, releasing the mapping seems to be a bug. As the bug isn't introduced by
this patch, we can fix it separately, but if you end up resubmitting due to
the first comment you could also add a fix.
>
> dev->archdata.iommu = NULL;
--
Regards,
Laurent Pinchart
Re: [PATCH v6 04/07] iommu/ipmmu-vmsa: Break out domain allocation code
Hi Magnus,
Thank you for the patch.
On Thursday 20 Oct 2016 08:36:13 Magnus Damm wrote:
> From: Magnus Damm
>
> Break out the domain allocation code into a separate function.
> This is preparation for future code sharing.
>
> Signed-off-by: Magnus Damm
> Reviewed-by: Joerg Roedel
This looks good to me,
Reviewed-by: Laurent Pinchart
(assuming my review of the next patches in the series won't make me conclude
that this patch isn't needed :-))
> ---
>
> Changes since V5:
> - None
>
> Changes since V4:
> - None
>
> Changes since V3:
> - None
>
> Changes since V2:
> - Included this new patch as-is from the following series:
>[PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
>
> drivers/iommu/ipmmu-vmsa.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> --- 0008/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:56:59.220607110 +0900
> @@ -507,13 +507,10 @@ static irqreturn_t ipmmu_irq(int irq, vo
> * IOMMU Operations
> */
>
> -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> +static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)
> {
> struct ipmmu_vmsa_domain *domain;
>
> - if (type != IOMMU_DOMAIN_UNMANAGED)
> - return NULL;
> -
> domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> if (!domain)
> return NULL;
> @@ -523,6 +520,14 @@ static struct iommu_domain *ipmmu_domain
> return &domain->io_domain;
> }
>
> +static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> +{
> + if (type != IOMMU_DOMAIN_UNMANAGED)
> + return NULL;
> +
> + return __ipmmu_domain_alloc(type);
> +}
> +
> static void ipmmu_domain_free(struct iommu_domain *io_domain)
> {
> struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
--
Regards,
Laurent Pinchart
Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
Hello,
On Thursday 10 Nov 2016 12:42:06 Joerg Roedel wrote:
> On Fri, Oct 21, 2016 at 06:52:53PM +0100, Robin Murphy wrote:
> > > -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> > > -{
> > > - if (type != IOMMU_DOMAIN_UNMANAGED)
> > > - return NULL;
> >
> > I *think* that if we did the initial check thus:
> > if (type != IOMMU_DOMAIN_UNMANAGED ||
> >
> > (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
> >
> > return NULL;
> >
> > it shouldn't be necessary to split the function at all - we then just
> > wrap the {get,put}_cookie() bits in "if (type == IOMMU_DOMAIN_DMA)" and
> > in the 32-bit ARM case they just don't run as that can never be true.
>
> This would be a good improvement. Magnus, Robin, can either of you send
> a follow-on patch to implement this suggestion? I have applied these
> patches to my arm/renesas branch (not pushed yet). The patch can be
> based on it.
I like the suggestion too, a patch is on its way.
Joerg, as I've sent a few comments about the other patches (sorry for the late
review, I got delayed by KS and LPC), the follow-up patch should probably be
squashed into this one when Magnus addresses my comments. Could you please
hold off pushing the arm/renesas branch until Magnus replies to this ?
--
Regards,
Laurent Pinchart
Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
Hi Magnus,
Thank you for the patch.
On Thursday 20 Oct 2016 08:36:23 Magnus Damm wrote:
> From: Magnus Damm
>
> Introduce an alternative set of iommu_ops suitable for 64-bit ARM
> as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. Also adjust the
> Kconfig to depend on ARM or IOMMU_DMA.
>
> Signed-off-by: Magnus Damm
> ---
>
> Changes since V5:
> - Made domain allocation/free code more consistent - thanks Joerg!
>
> Changes since V4:
> - Added Kconfig hunk to depend on ARM or IOMMU_DMA
>
> Changes since V3:
> - Removed group parameter from ipmmu_init_platform_device()
>
> Changes since V2:
> - Included this new patch from the following series:
>[PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
> - Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
> - Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
> - of_xlate() is now used without #ifdefs
> - Made sure code compiles on both 32-bit and 64-bit ARM.
>
> drivers/iommu/Kconfig |1
> drivers/iommu/ipmmu-vmsa.c | 122 ++---
> 2 files changed, 115 insertions(+), 8 deletions(-)
>
> --- 0001/drivers/iommu/Kconfig
> +++ work/drivers/iommu/Kconfig2016-10-20 08:16:40.980607110 +0900
> @@ -274,6 +274,7 @@ config EXYNOS_IOMMU_DEBUG
>
> config IPMMU_VMSA
> bool "Renesas VMSA-compatible IPMMU"
> + depends on ARM || IOMMU_DMA
> depends on ARM_LPAE
> depends on ARCH_RENESAS || COMPILE_TEST
> select IOMMU_API
> --- 0006/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c 2016-10-20 08:16:48.440607110 +0900
> @@ -10,6 +10,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -22,8 +23,10 @@
> #include
> #include
>
> +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
> #include
> #include
> +#endif
>
> #include "io-pgtable.h"
>
> @@ -520,14 +523,6 @@ static struct iommu_domain *__ipmmu_doma
> return &domain->io_domain;
> }
>
> -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> -{
> - if (type != IOMMU_DOMAIN_UNMANAGED)
> - return NULL;
> -
> - return __ipmmu_domain_alloc(type);
> -}
> -
> static void ipmmu_domain_free(struct iommu_domain *io_domain)
> {
> struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
> @@ -714,6 +709,8 @@ error:
> return ret;
> }
>
> +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
> +
> static int ipmmu_add_device(struct device *dev)
> {
> struct ipmmu_vmsa_archdata *archdata;
> @@ -807,6 +804,14 @@ static void ipmmu_remove_device(struct d
> dev->archdata.iommu = NULL;
> }
>
> +static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> +{
> + if (type != IOMMU_DOMAIN_UNMANAGED)
> + return NULL;
> +
> + return __ipmmu_domain_alloc(type);
> +}
> +
> static const struct iommu_ops ipmmu_ops = {
> .domain_alloc = ipmmu_domain_alloc,
> .domain_free = ipmmu_domain_free,
> @@ -821,6 +826,105 @@ static const struct iommu_ops ipmmu_ops
> .pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
> };
>
> +#endif /* !CONFIG_ARM && CONFIG_IOMMU_DMA */
> +
> +#ifdef CONFIG_IOMMU_DMA
> +
> +static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
> +{
> + struct iommu_domain *io_domain = NULL;
> +
> + switch (type) {
> + case IOMMU_DOMAIN_UNMANAGED:
> + io_domain = __ipmmu_domain_alloc(type);
> + break;
> +
> + case IOMMU_DOMAIN_DMA:
> + io_domain = __ipmmu_domain_alloc(type);
> + if (io_domain)
> + iommu_get_dma_cookie(io_domain);
> + break;
> + }
> +
> + return io_domain;
> +}
> +
> +static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
> +{
> + switch (io_domain->type) {
> + case IOMMU_DOMAIN_DMA:
> + iommu_put_dma_cookie(io_domain);
> + /* fall-through */
> + default:
> + ipmmu_domain_free(io_domain);
> + break;
> + }
> +}
> +
> +static int ipmmu_add_device_dma(struct device *dev)
> +{
> + struct iommu_group *group;
> +
> + /* only accept devices with iommus property */
Nitpicking, comments in the driver are capitalized and end with a period.
> + if (of_count_phandle_with_args(dev->of_node, "iommus",
> +"#iommu-cells") < 0)
> + return -ENODEV;
> +
> + group = iommu_group_get_for_dev(dev);
> + if (IS_ERR(group))
> + return PTR_ERR(group);
> +
> + return 0;
> +}
> +
> +static void ipmmu_remove_device_dma(struct device *dev)
> +{
> + iommu_group_remove_device(dev);
> +}
> +
> +static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
> +{
> + struct iommu_group *group;
> + int ret;
> +
> + group = generic_device_group(dev);
> + if (IS_ERR(group))
> + return group;
> +
> + ret = ipmmu_init_platform_device(dev);
> + if
Re: [PATCH v6 06/07] iommu/ipmmu-vmsa: ARM and ARM64 archdata access
Hello,
On Friday 21 Oct 2016 18:32:54 Robin Murphy wrote:
> On 20/10/16 00:36, Magnus Damm wrote:
> > From: Magnus Damm
> >
> > Not all architectures have an iommu member in their archdata, so
> > use #ifdefs support build wit COMPILE_TEST on any architecture.
>
> As an alternative to this we could now use iommu_fwspec in place of the
> custom ipmmu_vmsa_archdata - that's deliberately
> architecture-independent. I converted the Mediatek drivers[1] as an
> example to stand separately from the big SMMU rework, as those are the
> ones I'm most familiar with, but it looks like the IPMMU is a similarly
> perfect fit.
>
> Of course, that could always come afterwards as a cleanup - I wouldn't
> consider it a blocker at this point - but it does look like it might
> simplify some of the code being moved around in this series if it were
> to be done beforehand.
I like the suggestion, it looks much cleaner. It would also allow implementing
.of_xlate() in a cleaner fashion. I don't think it needs to block this series,
but if Magnus ends up sending a v7 to address all the other small comments, it
would be worth a shot.
> [1]:http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14577.ht
> ml
> > Signed-off-by: Magnus Damm
> > Reviewed-by: Joerg Roedel
> > ---
> >
> > Changes since V5:
> > - None
> >
> > Changes since V4:
> > - None
> >
> > Changes since V3:
> > - New patch
> >
> > drivers/iommu/ipmmu-vmsa.c | 37 +++--
> > 1 file changed, 27 insertions(+), 10 deletions(-)
> >
> > --- 0012/drivers/iommu/ipmmu-vmsa.c
> > +++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:59:21.690607110 +0900
> > @@ -70,6 +70,25 @@ static struct ipmmu_vmsa_domain *to_vmsa
> >
> > return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
> >
> > }
> >
> > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
> > +{
> > + return dev->archdata.iommu;
> > +}
> > +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata
> > *p) +{
> > + dev->archdata.iommu = p;
> > +}
> > +#else
> > +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
> > +{
> > + return NULL;
> > +}
> > +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata
> > *p) +{
> > +}
> > +#endif
> > +
> >
> > #define TLB_LOOP_TIMEOUT 100 /* 100us */
> >
> > /*
> >
> > ->
> > @@ -539,7 +558,7 @@ static void ipmmu_domain_free(struct iom
> >
> > static int ipmmu_attach_device(struct iommu_domain *io_domain,
> >
> >struct device *dev)
> >
> > {
> >
> > - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> > + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
> >
> > struct ipmmu_vmsa_device *mmu = archdata->mmu;
> > struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
> > unsigned long flags;
> >
> > @@ -581,7 +600,7 @@ static int ipmmu_attach_device(struct io
> >
> > static void ipmmu_detach_device(struct iommu_domain *io_domain,
> >
> > struct device *dev)
> >
> > {
> >
> > - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> > + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
> >
> > struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
> > unsigned int i;
> >
> > @@ -701,7 +720,7 @@ static int ipmmu_init_platform_device(st
> >
> > archdata->mmu = mmu;
> > archdata->utlbs = utlbs;
> > archdata->num_utlbs = num_utlbs;
> >
> > - dev->archdata.iommu = archdata;
> > + set_archdata(dev, archdata);
> >
> > return 0;
> >
> > error:
> > @@ -713,12 +732,11 @@ error:
> > static int ipmmu_add_device(struct device *dev)
> > {
> >
> > - struct ipmmu_vmsa_archdata *archdata;
> >
> > struct ipmmu_vmsa_device *mmu = NULL;
> > struct iommu_group *group;
> > int ret;
> >
> > - if (dev->archdata.iommu) {
> > + if (to_archdata(dev)) {
> >
> > dev_warn(dev, "IOMMU driver already assigned to device %s\n",
> >
> > dev_name(dev));
> >
> > return -EINVAL;
> >
> > @@ -754,8 +772,7 @@ static int ipmmu_add_device(struct devic
> >
> > * - Make the mapping size configurable ? We currently use a 2GB
mapping
> > * at a 1GB offset to ensure that NULL VAs will fault.
> > */
> >
> > - archdata = dev->archdata.iommu;
> > - mmu = archdata->mmu;
> > + mmu = to_archdata(dev)->mmu;
> >
> > if (!mmu->mapping) {
> >
> > struct dma_iommu_mapping *mapping;
> >
> > @@ -783,7 +800,7 @@ error:
> > if (mmu)
> >
> > arm_iommu_release_mapping(mmu->mapping);
> >
> > - dev->archdata.iommu = NULL;
> > + set_archdata(dev, NULL);
> >
> > if (!IS_ERR_OR_NULL(group))
> >
> >
Re: The failure summary report of GEN2 for linux stable v4.8
Hello Truong,
On Friday 21 Oct 2016 14:20:24 Laurent Pinchart wrote:
> On Friday 21 Oct 2016 19:53:47 Xuan Truong Nguyen wrote:
> > Hello Laurent.
> >
> > > The document mentions that you have used shmobile_defconfig. However,
> > > that configuration in v4.8 doesn't enable CONFIG_CMA, which causes at
> > > least the DU driver to fail probing the device. I thus assume you have
> > > modified the configuration to enable CMA. The fact that
> > > shmobile_defconfig doesn't include the uvcvideo driver used by test 14
> > > seems to confirm local modifications to the configuration.
> > > Issue 8 ("Lager: bad image quality when shown on HDMI display") can't be
> > > reproduced on my system when displaying a test pattern with the modetest
> > > application. Could you please share more information on how to reproduce
> > > the problem ?
> >
> > We retested this issue No 8 again with the CONFIG_CMA enabled but the
> > result is the same. you could see it by the error.jpg image in
> > attachments. the display device used is Panasonic TH-L19C3 (refer the
> > device.jpg). we also send you my configs file, too (lager_du.config).
>
> Thank you for the information, I'll try to reproduce the problem here with
> your kernel configuration.
I've been able to reproduce the problem here, and at this point I'm not sure
what goes wrong. I've tested older kernels up to the point where HDMI support
for Lager was introduced, and the bug was present in all of them. I'll try to
fix it, but I have other higher priority tasks at the moment.
> > just one more thing, with CONFIG_CMA enabled, the driver fails on
> > probing if we boot the board without the HDMI cable inserted in advance.
> > we have to insert the cable before booting the board. and not all the
> > display work with current HDMI driver (as our LG display will not work)
> > we are using the mainline upstream version 4.8
>
> Interesting, I'll check that too.
I haven't had time to check this yet.
> > if you need any information to debug, please lets us know.
>
> Could you please tell me what you're using to test the display ?
--
Regards,
Laurent Pinchart
Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
Hi Robin,
On Friday 21 Oct 2016 18:52:53 Robin Murphy wrote:
> On 20/10/16 00:36, Magnus Damm wrote:
> > From: Magnus Damm
> >
> > Introduce an alternative set of iommu_ops suitable for 64-bit ARM
> > as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. Also adjust the
> > Kconfig to depend on ARM or IOMMU_DMA.
> >
> > Signed-off-by: Magnus Damm
> > ---
> >
> > Changes since V5:
> > - Made domain allocation/free code more consistent - thanks Joerg!
> >
> > Changes since V4:
> > - Added Kconfig hunk to depend on ARM or IOMMU_DMA
> >
> > Changes since V3:
> > - Removed group parameter from ipmmu_init_platform_device()
> >
> > Changes since V2:
> >
> > - Included this new patch from the following series:
> >[PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
> >
> > - Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
> > - Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
> > - of_xlate() is now used without #ifdefs
> > - Made sure code compiles on both 32-bit and 64-bit ARM.
> >
> > drivers/iommu/Kconfig |1
> > drivers/iommu/ipmmu-vmsa.c | 122 ---
> > 2 files changed, 115 insertions(+), 8 deletions(-)
[snip]
> > --- 0006/drivers/iommu/ipmmu-vmsa.c
> > +++ work/drivers/iommu/ipmmu-vmsa.c 2016-10-20 08:16:48.440607110 +0900
[snip]
> > -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> > -{
> > - if (type != IOMMU_DOMAIN_UNMANAGED)
> > - return NULL;
>
> I *think* that if we did the initial check thus:
>
> if (type != IOMMU_DOMAIN_UNMANAGED ||
> (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
> return NULL;
I assume you meant
if (type != IOMMU_DOMAIN_UNMANAGED &&
(!IS_ENABLED(CONFIG_IOMMU_DMA) || type != IOMMU_DOMAIN_DMA))
return NULL;
But how about just
if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA))
return NULL;
as type will never be set to IOMMU_DOMAIN_DMA on ARM32 ?
> it shouldn't be necessary to split the function at all - we then just
> wrap the {get,put}_cookie() bits in "if (type == IOMMU_DOMAIN_DMA)" and
> in the 32-bit ARM case they just don't run as that can never be true.
>
> > -
> > - return __ipmmu_domain_alloc(type);
> > -}
> > -
--
Regards,
Laurent Pinchart
[PATCH] iommu/ipmmu-vmsa: Unify domain alloc/free implementations
The ARM and ARM64 implementations of the IOMMU domain alloc/free
operations can be unified with the correct combination of type checking,
as the domain type can never be IOMMU_DOMAIN_DMA on 32-bit ARM due to
the driver calling iommu_group_get_for_dev() on ARM64 only. Do so.
Signed-off-by: Laurent Pinchart
---
drivers/iommu/ipmmu-vmsa.c | 58 +++---
1 file changed, 14 insertions(+), 44 deletions(-)
Hi Magnus,
This cleans up patch 5/7, making patch 4/7 unnecessary. I proposed squashing
4/7, 5/7 and this one in v7.
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 11550ac90d65..827aa72aaf28 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -529,16 +529,22 @@ static irqreturn_t ipmmu_irq(int irq, void *dev)
* IOMMU Operations
*/
-static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)
+static struct iommu_domain *ipmmu_domain_alloc(unsigned int type)
{
struct ipmmu_vmsa_domain *domain;
+ if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+ return NULL;
+
domain = kzalloc(sizeof(*domain), GFP_KERNEL);
if (!domain)
return NULL;
spin_lock_init(&domain->lock);
+ if (type == IOMMU_DOMAIN_DMA)
+ iommu_get_dma_cookie(&domain->io_domain);
+
return &domain->io_domain;
}
@@ -546,6 +552,9 @@ static void ipmmu_domain_free(struct iommu_domain
*io_domain)
{
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
+ if (io_domain->type)
+ iommu_put_dma_cookie(io_domain);
+
/*
* Free the domain resources. We assume that all devices have already
* been detached.
@@ -821,14 +830,6 @@ static void ipmmu_remove_device(struct device *dev)
set_archdata(dev, NULL);
}
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;
-
- return __ipmmu_domain_alloc(type);
-}
-
static const struct iommu_ops ipmmu_ops = {
.domain_alloc = ipmmu_domain_alloc,
.domain_free = ipmmu_domain_free,
@@ -847,42 +848,11 @@ static const struct iommu_ops ipmmu_ops = {
#ifdef CONFIG_IOMMU_DMA
-static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
-{
- struct iommu_domain *io_domain = NULL;
-
- switch (type) {
- case IOMMU_DOMAIN_UNMANAGED:
- io_domain = __ipmmu_domain_alloc(type);
- break;
-
- case IOMMU_DOMAIN_DMA:
- io_domain = __ipmmu_domain_alloc(type);
- if (io_domain)
- iommu_get_dma_cookie(io_domain);
- break;
- }
-
- return io_domain;
-}
-
-static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
-{
- switch (io_domain->type) {
- case IOMMU_DOMAIN_DMA:
- iommu_put_dma_cookie(io_domain);
- /* fall-through */
- default:
- ipmmu_domain_free(io_domain);
- break;
- }
-}
-
static int ipmmu_add_device_dma(struct device *dev)
{
struct iommu_group *group;
- /* only accept devices with iommus property */
+ /* Only accept devices with an iommus property. */
if (of_count_phandle_with_args(dev->of_node, "iommus",
"#iommu-cells") < 0)
return -ENODEV;
@@ -920,13 +890,13 @@ static struct iommu_group *ipmmu_device_group_dma(struct
device *dev)
static int ipmmu_of_xlate_dma(struct device *dev,
struct of_phandle_args *spec)
{
- /* dummy callback to satisfy of_iommu_configure() */
+ /* Dummy callback to satisfy of_iommu_configure(). */
return 0;
}
static const struct iommu_ops ipmmu_ops = {
- .domain_alloc = ipmmu_domain_alloc_dma,
- .domain_free = ipmmu_domain_free_dma,
+ .domain_alloc = ipmmu_domain_alloc,
+ .domain_free = ipmmu_domain_free,
.attach_dev = ipmmu_attach_device,
.detach_dev = ipmmu_detach_device,
.map = ipmmu_map,
--
Regards,
Laurent Pinchart
Re: The failure summary report of GEN2 for linux stable v4.8
Hi Laurent
please check this when you have time.
If you need any information, let us know.
thanks and best regard
JINSO/Truong
On 2016年11月11日 10:31, Laurent Pinchart wrote:
Hello Truong,
On Friday 21 Oct 2016 14:20:24 Laurent Pinchart wrote:
On Friday 21 Oct 2016 19:53:47 Xuan Truong Nguyen wrote:
Hello Laurent.
The document mentions that you have used shmobile_defconfig. However,
that configuration in v4.8 doesn't enable CONFIG_CMA, which causes at
least the DU driver to fail probing the device. I thus assume you have
modified the configuration to enable CMA. The fact that
shmobile_defconfig doesn't include the uvcvideo driver used by test 14
seems to confirm local modifications to the configuration.
Issue 8 ("Lager: bad image quality when shown on HDMI display") can't be
reproduced on my system when displaying a test pattern with the modetest
application. Could you please share more information on how to reproduce
the problem ?
We retested this issue No 8 again with the CONFIG_CMA enabled but the
result is the same. you could see it by the error.jpg image in
attachments. the display device used is Panasonic TH-L19C3 (refer the
device.jpg). we also send you my configs file, too (lager_du.config).
Thank you for the information, I'll try to reproduce the problem here with
your kernel configuration.
I've been able to reproduce the problem here, and at this point I'm not sure
what goes wrong. I've tested older kernels up to the point where HDMI support
for Lager was introduced, and the bug was present in all of them. I'll try to
fix it, but I have other higher priority tasks at the moment.
just one more thing, with CONFIG_CMA enabled, the driver fails on
probing if we boot the board without the HDMI cable inserted in advance.
we have to insert the cable before booting the board. and not all the
display work with current HDMI driver (as our LG display will not work)
we are using the mainline upstream version 4.8
Interesting, I'll check that too.
I haven't had time to check this yet.
if you need any information to debug, please lets us know.
Could you please tell me what you're using to test the display ?
Re: [RFC 5/5] doc_rst: media: New SDR formats SC16, SC18 & SC20
Hello On 11/10/2016 10:08 AM, Laurent Pinchart wrote: Antti, Hans, ping ? Please see below. On Friday 04 Nov 2016 09:23:29 Ramesh Shanmugasundaram wrote: On 11/02/2016 10:58 PM, Laurent Pinchart wrote: On Wednesday 02 Nov 2016 09:00:00 Ramesh Shanmugasundaram wrote: On Wednesday 12 Oct 2016 15:10:29 Ramesh Shanmugasundaram wrote: This patch adds documentation for the three new SDR formats V4L2_SDR_FMT_SCU16BE V4L2_SDR_FMT_SCU18BE V4L2_SDR_FMT_SCU20BE [snip] + + - start + 0: + + - I'\ :sub:`0[D13:D6]` + + - I'\ :sub:`0[D5:D0]` + +- .. row 2 + + - start + buffer_size/2: + + - Q'\ :sub:`0[D13:D6]` + + - Q'\ :sub:`0[D5:D0]` The format looks planar, does it use one V4L2 plane (as does NV12) or two V4L2 planes (as does NV12M) ? Same question for the other formats. Thank you for bringing up this topic. This is one of the key design dilemma. The I & Q data for these three SDR formats comes from two different DMA channels and hence two separate pointers -> we could say it is v4l2 multi- planar. Right now, I am making it look like a single plane by presenting the data in one single buffer ptr. For e.g. multi-planar SC16 format would look something like this <32bits--> <--I(14 bit data) + 2bit status--16bit padded zeros--> : start0 + 0 <--I(14 bit data) + 2bit status--16bit padded zeros--> : start0 + 4 ... <--Q(14 bit data) + 2bit status--16bit padded zeros--> : start1 + 0 <--Q(14 bit data) + 2bit status--16bit padded zeros--> : start1 + 4 My concerns are 1) These formats are not a standard as the video "Image Formats". These formats are possible when we use DRIF + MAX2175 combination. If we interface with a different tuner vendor, the above format(s) MAY/MAY NOT be re-usable. We do not know at this point. This is the main open item for discussion in the cover letter. If the formats are really device-specific then they should be documented accordingly and not made generic. 2) MPLANE support within V4L2 seems specific to video. Please correct me if this is wrong interpretation. - struct v4l2_format contains v4l2_sdr_format and v4l2_pix_format_mplane as members of union. Should I create a new v4l2_sdr_format_mplane? If I have to use v4l2_pix_format_mplane most of the video specific members would be unused (it would be similar to using v4l2_pix_format itself instead of v4l2_sdr_format)? I have no answer to that question as I'm not familiar with SDR. Antti, you've added v4l2_sdr_format to the API, what's your opinion ? Hans, as you've acked the patch, your input would be appreciated as well. If I understood correctly this hardware provides I and Q samples via different channels and driver now combines those channels as a sequential IQ sample pairs. The driver combines the two buffer ptrs and present as one single buffer. For a buffer of size 200 ptr + 0 : I I I I ... I ptr + 100 : Q Q Q Q ... Q I have never seen any other than hw which provides IQ IQ IQ IQ ... IQ. There are some modes where this h/w combo can also do IQ IQ IQ pattern. Those modes are not added in the RFC patchset. This is I I I I ... I Q Q Q Q ... Q I am not very familiar with planars, but it sounds like it is correct approach. So I think should be added rather than emulate packet sequential format. My understanding of V4L2 MPLANE constructs is limited to a quick code read only. At this point MPLANE support seems specific to video. SDR is defined as separate format like v4l2_pix_format. Questions would be - should we define new SDR_MPLANE? or merge SDR format with pix format & reuse existing MPLANE with some SDR extensions (if possible)? These seem big design decisions. Any suggestions please? struct v4l2_format contains union that has own format definition for video, video mplane and sdr (+many others). Basically on api there is own definitions for each type, so I think possible sdr mplane should be similarly own types and definitions. For my use case, MPLANE support does not seem to add significant benefit except it may be syntactically correct. I am doing cyclic DMA with a small set of h/w buffers and copying each stage to one mmapped vmalloc vb2_buffer at two offsets. If we add MPLANE support, it can be two non-contiguous buffer pointers. If there is no clear idea about need of mplane then that's also fine for me. And whole mplane concept is new for me. I have never played with any v4l2 video formats nor mplane video formats. I would still like to hear what Hans think about adding mplane. - The above decision (accomodate SDR & MPLANE) needs to be propagated across the framework. Is this the preferred approach? It goes back to point (1). As of today, the change set for this combo (DRIF+MAX2175) introduces new SDR formats only. Should it add further SDR+MPLANE support to the framework as well? I would appreciate your suggestions on this regard. regards Antti -- http://palosaari.fi/
Re: [PATCH 2/5] media: i2c: max2175: Add MAX2175 support
Hello
On 11/09/2016 05:44 PM, Ramesh Shanmugasundaram wrote:
+static int max2175_set_lo_freq(struct max2175 *ctx, u64 lo_freq)
+{
+ u64 scaled_lo_freq, scaled_npf, scaled_integer, scaled_fraction;
+ u32 frac_desired, int_desired, lo_mult = 1;
+ const u32 scale_factor = 100U;
+ u8 loband_bits = 0, vcodiv_bits = 0;
+ enum max2175_band band;
+ int ret;
+
+ /* Scale to larger number for precision */
+ scaled_lo_freq = lo_freq * scale_factor * 100;
+ band = max2175_read_bits(ctx, 5, 1, 0);
+
+ mxm_dbg(ctx, "set_lo_freq: scaled lo_freq %llu lo_freq %llu band %d\n",
+ scaled_lo_freq, lo_freq, band);
+
+ switch (band) {
+ case MAX2175_BAND_AM:
+ if (max2175_read_bit(ctx, 5, 7) == 0)
+ lo_mult = 16;
else is lo_mult = 1. No idea if it is correct, but sounds very small
output divider for low freq like am band. And on the other-hand local
oscillator output divider, which I expect this to be, is usually 2 or more.
+ break;
+ case MAX2175_BAND_FM:
+ if (lo_freq <= 7470) {
+ lo_mult = 16;
No meaning as you set it later 8.
+ } else if (lo_freq > 7470 && lo_freq <= 11000) {
+ loband_bits = 1;
+ } else {
+ loband_bits = 1;
+ vcodiv_bits = 3;
+ }
+ lo_mult = 8;
+ break;
+ case MAX2175_BAND_VHF:
+ if (lo_freq <= 21000) {
+ loband_bits = 2;
+ vcodiv_bits = 2;
+ } else {
+ loband_bits = 2;
+ vcodiv_bits = 1;
+ }
+ lo_mult = 4;
+ break;
+ default:
+ loband_bits = 3;
+ vcodiv_bits = 2;
+ lo_mult = 2;
+ break;
+ }
+
+ if (band == MAX2175_BAND_L)
+ scaled_npf = div_u64(div_u64(scaled_lo_freq, ctx->xtal_freq),
+lo_mult);
+ else
+ scaled_npf = div_u64(scaled_lo_freq, ctx->xtal_freq) * lo_mult;
+
+ scaled_npf = div_u64(scaled_npf, 100);
+ scaled_integer = div_u64(scaled_npf, scale_factor) * scale_factor;
+ int_desired = div_u64(scaled_npf, scale_factor);
+ scaled_fraction = scaled_npf - scaled_integer;
+ frac_desired = div_u64(scaled_fraction << 20, scale_factor);
+
+ /* Check CSM is not busy */
+ ret = max2175_poll_csm_ready(ctx);
+ if (ret)
+ return ret;
+
+ mxm_dbg(ctx, "loband %u vcodiv %u lo_mult %u scaled_npf %llu\n",
+ loband_bits, vcodiv_bits, lo_mult, scaled_npf);
+ mxm_dbg(ctx, "scaled int %llu frac %llu desired int %u frac %u\n",
+ scaled_integer, scaled_fraction, int_desired, frac_desired);
+
+ /* Write the calculated values to the appropriate registers */
+ max2175_write(ctx, 1, int_desired);
+ max2175_write_bits(ctx, 2, 3, 0, (frac_desired >> 16) & 0xf);
+ max2175_write(ctx, 3, frac_desired >> 8);
+ max2175_write(ctx, 4, frac_desired);
+ max2175_write_bits(ctx, 5, 3, 2, loband_bits);
+ max2175_write_bits(ctx, 6, 7, 6, vcodiv_bits);
+ return ret;
+}
That synthesizer config is hard to understand. It seems to be
fractional-N, with configurable N, K and output divider - like a school
book example.
++
v|
Fref ++ +---+ +--+
--> | PD | --> | VCO | --> | /N.F |
++ +---+ +--+
|
|
v
+---+ Fout
| /Rout | -->
+---+
I made following look-up table in order to understand it:
band lo freq band vcodiv div_out
AM < 50000 0 16 // reg 5 bit 7 ?
FM < 74700 0 16
FM < 110001 0 8
FM < 160001 3 8
VHF < 210002 2 4
VHF < 62 1 4
L <203 2 2
"vcodiv" looks unrelated to synth calculation, dunno what it is.
One which makes calculation very complex looking is that it is based of
floating point calculus. On integer mathematics you should replace
fractional part with fractional modulus (usually letter "K" is used for
fractional modulus on PLL calc).
So that ends up something like:
1) select suitable lo output divider from desired output frequency
2) calculate vco frequency
3) convert vco frequency to N and K
* N = Fvco/Fref
* K = Fvco%Fref
4) convert K to control word (looks like << 20)
5) program values
Result should be calculus without scaling.
regards
Antti
--
htt
Re: [PATCH v8 0/6] UHS-I SDR-104 support for sh_mobile_sdhi
On Thu, Nov 10, 2016 at 11:21:08PM +0100, Ulf Hansson wrote: > On 10 November 2016 at 12:45, Wolfram Sang wrote: > > On Thu, Nov 03, 2016 at 03:15:58PM +0100, Simon Horman wrote: > >> Hi, > >> > >> this series is based on work by Ai Kyuse to add UHS-I SDR-104 support for > >> sh_mobile_sdhi. It builds on work by Shinobu Uehara, Rob Taylor, William > >> Towle and Ian Molton, Ben Hutchings, Wolfram Sang and others to add UHS-I > >> SDR-50 support to the same driver. > >> > >> It is based on the next branch of the mmc tree. > >> > >> To aid review the following git branch is provided: > >> * https:://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git > >> topic/sdr104-driver-v8 > >> > >> Overview of changes since v7: > >> * Correct inverted logic in sh_mobile_sdhi_hw_reset > >> * Correct value of SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN > >> > >> Please see http://elinux.org/Tests:SD-SDHI-SDR104 for indicative tests > >> results. > > > > Successfully tested on a Lager Board with a SanDisk card and a Samsung > > card which used to cause issues before. > > > > Tested-by: Wolfram Sang > > > > Ulf, I think this series is ready to go for v4.10. All issues we know of > > are related to pinmuxing and will be solved elsewhere. We currently have > > hacks for that so it is quite clear the issues are not caused by this > > series. > > > > Thanks, applied for next! > > I did have to fix conflict and also to resolve some checkpatch > warnings - but no worries this time! Thanks, sorry for letting those slip through.
