Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Robin Murphy]

On 31/01/18 09:37, Arnd Bergmann wrote:

On Wed, Jan 31, 2018 at 8:29 AM, Maxime Ripard
 wrote:

Hi Thierry,

On Tue, Jan 30, 2018 at 11:01:50AM +0100, Thierry Reding wrote:

On Tue, Jan 30, 2018 at 10:59:16AM +0100, Thierry Reding wrote:

On Tue, Jan 30, 2018 at 10:24:48AM +0100, Arnd Bergmann wrote:

On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
 wrote:

On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:

On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
 wrote:

On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
 wrote:

On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:




At one point we had discussed adding a 'dma-masters' property that
lists all the buses on which a device can be a dma master, and
the respective properties of those masters (iommu, coherency,
offset, ...).

IIRC at the time we decided that we could live without that complexity,
but perhaps we cannot.


Are you talking about this ?
https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41

It doesn't seem to be related to that issue to me. And in our
particular cases, all the devices are DMA masters, the RAM is just
mapped to another address.


No, that's not the one I was thinking of. The idea at the time was much
more generic, and not limited to dma engines. I don't recall the details,
but I think that Thierry was either involved or made the proposal at the
time.


Yeah, I vaguely remember discussing something like this before. A quick
search through my inbox yielded these two threads, mostly related to
IOMMU but I think there were some mentions about dma-ranges and so on as
well. I'll have to dig deeper into those threads to refresh my memories,
but I won't get around to it until later today.

If someone wants to read up on this in the meantime, here are the links:

 https://lkml.org/lkml/2014/4/27/346
 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257200.html

 From a quick glance the issue of dma-ranges was something that we hand-
waved at the time.


Also found this, which seems to be relevant as well:

   
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252715.html

Adding Dave.


Thanks for the pointers, I started to read through it.

I guess we have to come up with two solutions here: a short term one
to address the users we already have in the kernel properly, and a
long term one where we could use that discussion as a starting point.

For the short term one, could we just set the device dma_pfn_offset to
PHYS_OFFSET at probe time, and use our dma_addr_t directly later on,
or would this also cause some issues?


That would certainly be an improvement over the current version,
it keeps the hack more localized. That's fine with me. Note that both
PHYS_OFFSET and dma_pfn_offset have architecture specific
meanings and they could in theory change, so ideally we'd do that
fixup somewhere in arch/arm/mach-sunxi/ at boot time before the
driver gets probed, but this wouldn't work on arm64 if we need it
there too.


dma_pfn_offset certainly *shouldn't* be architecture-specific; it's just 
historically not been used consistently everywhere. That should now be 
addressed by Christoph's big dma_direct_ops cleanup (now merged) which 
fills in the places it was being missed in generic code. From quickly 
skimming this thread, it sounds like that is really should be sufficient 
for this particular hardware - if all its DMA goes through an 
interconnect which makes RAM appear at 0, then you give it "dma-ranges = 
<0 PHYS_OFFSET size>;" and things should 'just work' provided the DMA 
API is otherwise being used correctly.


If different devices have differing DMA master address maps (such that 
you can't just place a single dma-ranges on your "soc" node) then the 
trick is to wrap them in intermediate "simple-bus" nodes with a 
straight-though "ranges;" and the appropriate "dma-ranges = ...". Yes, 
it's a bit of a bodge, but then pretending AXI/APB interconnects are 
buses in the sense that DT assumes is already a bit of a bodge.



For the long term plan, from what I read from the discussion, it's
mostly centered around IOMMU indeed, and we don't have that. What we
would actually need is to break the assumption that the DMA "parent"
bus is the DT node's parent bus.

And I guess this could be done quite easily by adding a dma-parent
property with a phandle to the bus controller, that would have a
dma-ranges property of its own with the proper mapping described
there. It should be simple enough to support, but is there anything
that could prevent something like that to work properly (such as
preventing further IOMMU-related developments that were described in
those mail threads).


I've thought about it a little bit now. A dma-parent property would nicely
solve two problems:

- a device on a memory mapped 

Re: [linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

On Thu, Feb 01, 2018 at 11:34:43AM +, Liviu Dudau wrote:
> On Thu, Feb 01, 2018 at 10:20:28AM +0100, Arnd Bergmann wrote:
> > On Thu, Feb 1, 2018 at 9:32 AM, Maxime Ripard
> >  wrote:
> > > On Wed, Jan 31, 2018 at 02:47:53PM +, Liviu Dudau wrote:
> > >> On Wed, Jan 31, 2018 at 08:42:12AM +0100, Maxime Ripard wrote:
> > >> > On Wed, Jan 31, 2018 at 03:08:08AM +, Liviu Dudau wrote:
> > >> > > On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
> > >>
> > >> Yeah, sorry, my threading of the discussion was broken and I've seen
> > >> the rest of the thread after I have replied. My bad!
> > >>
> > >> >
> > >> > In our case, the bus where the device is attached will not do the
> > >> > address translations, and shouldn't.
> > >>
> > >> In my view, the bus is already doing address translation at physical
> > >> level, AFAIU it remaps the memory to zero.
> > >
> > > Not really. It uses a separate bus with a different mapping for the
> > > DMA accesses (and only the DMA accesses). The AXI (or AHB, I'm not
> > > sure, but, well, the "registers" bus) doesn't remap anything in
> > > itself, and we only describe this one usually in our DTs.
> 
> I was actually thinking about the DMA bus (AXI bus, most likely), not the
> "registers" bus (yes, usually APB or AHB). The DMA bus is the one that does
> the implicit remapping for the addresses it uses, if I understood you 
> correctly.
> 
> > Exactly, the DT model fundamentally assumes that each a device is
> > connected to exactly one bus, so we make up a device *tree* rather
> > than a non-directed mesh topology that you might see in modern
> > SoCs.
> 
> I think you are right, but we also have the registers property for a device 
> node
> and that can be used for describing the "registers" bus. Now, it is possible
> that some driver code gets confused between accessing the device registers
> (which in Arm world happens through an APB or AHB bus) and the device doing 
> system
> read/writes which usually happends through an AXI (or for very old systems, 
> AHB) bus.
> 
> For the sake of making sure we are talking about the same thing and in hope
> that Maxime or Yong can give a more detailed picture of this device

Keep in mind that this part is heavily under-documented to us, and
this is mostly information collected through testing and reading
through the various vendor trees.

As far as I know, a few devices (Display Engine, hardware codec, the
CSI driver that spawned this discussion) are connected to the memory
through a proprietary bus that does the remapping. The registers part
is connected to an AHB bus.

> I'll re-iterate what a lot of devices in the Arm world look like
> nowadays:
> 
> - they have a bus for accessing the "registers" of the device, for controlling
>   the behaviour of that device. Inside the SoC, that happens through the
>   APB bus and it has a separate clock. The CPU has a view of those registers
>   through some mapping in the address space that has been backed by the 
> hardware
>   engineers at design time and in DT we express that through the "registers" 
> property,
>   plus the "apb_clk" for most of the bindings. In DT world we express the 
> mapping
>   vis-a-vis the parent bus by using the "ranges" property.

We do have that.

> - they have a high speed bus for doing data transfers. Inside the SoC that
>   happens through an AXI or more modern CCI interconnect bus. The CPU does not
>   have a direct view on those transfers, but by using IOMMUs, SMMUs or simple
>   bus mastering capabilities it can gain knowledge of the state of the 
> transfers
>   and/or influence the target memory.

Part of that bus controller allows to probe the bus bandwidth and / or
set limits for the various controllers connected to it, so it seems to
match that description.

>   In the DT world, we use the "dma-ranges" property like you say to
>   express the translations that happen on that bus.

Right, except that the way dma-ranges is parsed at the moment is that
it will look on the parent node for dma-ranges. In this case, the
parent in the DT will be our AHB bus, and not all the devices
connected on that AHB bus that do DMA go through that DMA bus with the
different mapping. So setting it there isn't an option.

> Maxime/Yong: does your device have more than one AXI bus for doing
> transfers?

Not as far as I know.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Arnd Bergmann]

On Thu, Feb 1, 2018 at 4:29 PM, Maxime Ripard
 wrote:
> On Wed, Jan 31, 2018 at 10:37:37AM +0100, Arnd Bergmann wrote:
>> On Wed, Jan 31, 2018 at 8:29 AM, Maxime Ripard
>
>> I can think of a couple of other problems that may or may not be
>> relevant in the future that would require a more complex solution:
>>
>> - a device that is a bus master on more than one bus, e.g. a
>>   DMA engine that can copy between the CPU address space and
>>   another memory controller that is not visible to the CPU
>>
>> - a device that is connected to main memory both through an IOMMU
>>   and directly through its parent bus, and the device itself is in
>>   control over which of the two it uses (usually the IOMMU would
>>   contol whether a device is bypassing translation)
>>
>> - a device that has a single DMA address space with some form
>>   of non-linear mapping to one or more parent buses. Some of these
>>   can be expressed using the parent's dma-ranges properties, but
>>   our code currently only looks at the first entry in dma-ranges.
>
> As far as I know, we're in neither of these cases.

The point here was more about the general question of where we are
heading with the complexity of finding the right DMA settings. It's
already too complicated for anyone to fully understand what is
going on with DMA masks, offset, coherency etc when we look
at the existing DT bindings. Adding more complexity makes it
worse, so if anyone else is in need of a solution for the issues
above, we should try to accommodate their needs at the same time
to avoid adding more complexity now and again later on if we
can come up with a way that works for everyone now.

 Arnd

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

On Wed, Jan 31, 2018 at 10:37:37AM +0100, Arnd Bergmann wrote:
> On Wed, Jan 31, 2018 at 8:29 AM, Maxime Ripard
>  wrote:
> > Hi Thierry,
> >
> > On Tue, Jan 30, 2018 at 11:01:50AM +0100, Thierry Reding wrote:
> >> On Tue, Jan 30, 2018 at 10:59:16AM +0100, Thierry Reding wrote:
> >> > On Tue, Jan 30, 2018 at 10:24:48AM +0100, Arnd Bergmann wrote:
> >> > > On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
> >> > >  wrote:
> >> > > > On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
> >> > > >> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
> >> > > >>  wrote:
> >> > > >> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
> >> > > >> >  wrote:
> >> > > >> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
> >> > >
> >> > > >>
> >> > > >> At one point we had discussed adding a 'dma-masters' property that
> >> > > >> lists all the buses on which a device can be a dma master, and
> >> > > >> the respective properties of those masters (iommu, coherency,
> >> > > >> offset, ...).
> >> > > >>
> >> > > >> IIRC at the time we decided that we could live without that 
> >> > > >> complexity,
> >> > > >> but perhaps we cannot.
> >> > > >
> >> > > > Are you talking about this ?
> >> > > > https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41
> >> > > >
> >> > > > It doesn't seem to be related to that issue to me. And in our
> >> > > > particular cases, all the devices are DMA masters, the RAM is just
> >> > > > mapped to another address.
> >> > >
> >> > > No, that's not the one I was thinking of. The idea at the time was much
> >> > > more generic, and not limited to dma engines. I don't recall the 
> >> > > details,
> >> > > but I think that Thierry was either involved or made the proposal at 
> >> > > the
> >> > > time.
> >> >
> >> > Yeah, I vaguely remember discussing something like this before. A quick
> >> > search through my inbox yielded these two threads, mostly related to
> >> > IOMMU but I think there were some mentions about dma-ranges and so on as
> >> > well. I'll have to dig deeper into those threads to refresh my memories,
> >> > but I won't get around to it until later today.
> >> >
> >> > If someone wants to read up on this in the meantime, here are the links:
> >> >
> >> > https://lkml.org/lkml/2014/4/27/346
> >> > 
> >> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257200.html
> >> >
> >> > From a quick glance the issue of dma-ranges was something that we hand-
> >> > waved at the time.
> >>
> >> Also found this, which seems to be relevant as well:
> >>
> >>   
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252715.html
> >>
> >> Adding Dave.
> >
> > Thanks for the pointers, I started to read through it.
> >
> > I guess we have to come up with two solutions here: a short term one
> > to address the users we already have in the kernel properly, and a
> > long term one where we could use that discussion as a starting point.
> >
> > For the short term one, could we just set the device dma_pfn_offset to
> > PHYS_OFFSET at probe time, and use our dma_addr_t directly later on,
> > or would this also cause some issues?
> 
> That would certainly be an improvement over the current version,
> it keeps the hack more localized. That's fine with me.

Ok, we'll do that in that driver and convert the existing drivers
then.

> Note that both PHYS_OFFSET and dma_pfn_offset have architecture
> specific meanings and they could in theory change, so ideally we'd
> do that fixup somewhere in arch/arm/mach-sunxi/ at boot time before
> the driver gets probed, but this wouldn't work on arm64 if we need
> it there too.

Unfortunately, we do :/

> > For the long term plan, from what I read from the discussion, it's
> > mostly centered around IOMMU indeed, and we don't have that. What we
> > would actually need is to break the assumption that the DMA "parent"
> > bus is the DT node's parent bus.
> >
> > And I guess this could be done quite easily by adding a dma-parent
> > property with a phandle to the bus controller, that would have a
> > dma-ranges property of its own with the proper mapping described
> > there. It should be simple enough to support, but is there anything
> > that could prevent something like that to work properly (such as
> > preventing further IOMMU-related developments that were described in
> > those mail threads).
> 
> I've thought about it a little bit now. A dma-parent property would nicely
> solve two problems:
> 
> - a device on a memory mapped control bus that is a bus master on
>   a different bus. This is the case we are talking about here AFAICT
> 
> - a device that is on a different kind of bus (i2c, spi, usb, ...) but also
>   happens to be a dma master on another bus. I suspect we have
>   some of these today and they work by accident because 

Re: [linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Liviu Dudau]

On Thu, Feb 01, 2018 at 10:20:28AM +0100, Arnd Bergmann wrote:
> On Thu, Feb 1, 2018 at 9:32 AM, Maxime Ripard
>  wrote:
> > On Wed, Jan 31, 2018 at 02:47:53PM +, Liviu Dudau wrote:
> >> On Wed, Jan 31, 2018 at 08:42:12AM +0100, Maxime Ripard wrote:
> >> > On Wed, Jan 31, 2018 at 03:08:08AM +, Liviu Dudau wrote:
> >> > > On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
> >>
> >> Yeah, sorry, my threading of the discussion was broken and I've seen
> >> the rest of the thread after I have replied. My bad!
> >>
> >> >
> >> > In our case, the bus where the device is attached will not do the
> >> > address translations, and shouldn't.
> >>
> >> In my view, the bus is already doing address translation at physical
> >> level, AFAIU it remaps the memory to zero.
> >
> > Not really. It uses a separate bus with a different mapping for the
> > DMA accesses (and only the DMA accesses). The AXI (or AHB, I'm not
> > sure, but, well, the "registers" bus) doesn't remap anything in
> > itself, and we only describe this one usually in our DTs.

I was actually thinking about the DMA bus (AXI bus, most likely), not the
"registers" bus (yes, usually APB or AHB). The DMA bus is the one that does
the implicit remapping for the addresses it uses, if I understood you correctly.

> 
> Exactly, the DT model fundamentally assumes that each a device is
> connected to exactly one bus, so we make up a device *tree* rather
> than a non-directed mesh topology that you might see in modern
> SoCs.

I think you are right, but we also have the registers property for a device node
and that can be used for describing the "registers" bus. Now, it is possible
that some driver code gets confused between accessing the device registers
(which in Arm world happens through an APB or AHB bus) and the device doing 
system
read/writes which usually happends through an AXI (or for very old systems, 
AHB) bus.

For the sake of making sure we are talking about the same thing and in hope
that Maxime or Yong can give a more detailed picture of this device, I'll
re-iterate what a lot of devices in the Arm world look like nowadays:

- they have a bus for accessing the "registers" of the device, for controlling
  the behaviour of that device. Inside the SoC, that happens through the
  APB bus and it has a separate clock. The CPU has a view of those registers
  through some mapping in the address space that has been backed by the hardware
  engineers at design time and in DT we express that through the "registers" 
property,
  plus the "apb_clk" for most of the bindings. In DT world we express the 
mapping
  vis-a-vis the parent bus by using the "ranges" property.

- they have a high speed bus for doing data transfers. Inside the SoC that
  happens through an AXI or more modern CCI interconnect bus. The CPU does not
  have a direct view on those transfers, but by using IOMMUs, SMMUs or simple
  bus mastering capabilities it can gain knowledge of the state of the transfers
  and/or influence the target memory. In the DT world, we use the "dma-ranges"
  property like you say to express the translations that happen on that bus.

Maxime/Yong: does your device have more than one AXI bus for doing transfers?


> 
> The "dma-ranges" property was introduced when this first started
> falling apart and we got devices that had a different translation
> in DMA direction compared to control direction (i.e. the "ranges"
> property), but that still assumed that every device on a given bus
> had the same view of DMA space.
> 
> With just "dma-ranges", we could easy deal with a topology where
> each DMA master on an AXI bus sees main memory at address
> zero but the CPU sees the same memory at a high address while
> seeing the MMIO ranges at a low address.
> 
> What we cannot represent is multiple masters on the same
> AXI bus that use a different translation. Making up arbitrary
> intermediate buses would get this to work, but would likely
> cause other problems in the future when we do something
> else that relies on having a correct representation of the
> hierarchy of all the AXI/AHB/APB buses in the system, such
> as doing per-bus bandwidth allocation for child devices or
> anything else that requires configuring the bus bridge itself.

Agree we cannot express multiple masters on the same AXI bus having
different translations. Maybe we need to try to make things look more like
PCI where the child busses can have their own view of the world as long as
they don't try to communicate upwards to their parents or sideways to sibling
busses?


> 
> >> What you (we?) need is a simple bus driver that registers the
> >> correct virt_to_bus()/bus_to_virt() hooks for the device that do
> >> this translation at the DMA API level as well.
> >
> > Like I said, this only impact DMA transfers, and not the registers
> > accesses. We have other devices sitting on the same bus that do not
> > perform DMA accesses through that special memory bus 

Re: [linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Arnd Bergmann]

On Thu, Feb 1, 2018 at 9:32 AM, Maxime Ripard
 wrote:
> On Wed, Jan 31, 2018 at 02:47:53PM +, Liviu Dudau wrote:
>> On Wed, Jan 31, 2018 at 08:42:12AM +0100, Maxime Ripard wrote:
>> > On Wed, Jan 31, 2018 at 03:08:08AM +, Liviu Dudau wrote:
>> > > On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
>>
>> Yeah, sorry, my threading of the discussion was broken and I've seen
>> the rest of the thread after I have replied. My bad!
>>
>> >
>> > In our case, the bus where the device is attached will not do the
>> > address translations, and shouldn't.
>>
>> In my view, the bus is already doing address translation at physical
>> level, AFAIU it remaps the memory to zero.
>
> Not really. It uses a separate bus with a different mapping for the
> DMA accesses (and only the DMA accesses). The AXI (or AHB, I'm not
> sure, but, well, the "registers" bus) doesn't remap anything in
> itself, and we only describe this one usually in our DTs.

Exactly, the DT model fundamentally assumes that each a device is
connected to exactly one bus, so we make up a device *tree* rather
than a non-directed mesh topology that you might see in modern
SoCs.

The "dma-ranges" property was introduced when this first started
falling apart and we got devices that had a different translation
in DMA direction compared to control direction (i.e. the "ranges"
property), but that still assumed that every device on a given bus
had the same view of DMA space.

With just "dma-ranges", we could easy deal with a topology where
each DMA master on an AXI bus sees main memory at address
zero but the CPU sees the same memory at a high address while
seeing the MMIO ranges at a low address.

What we cannot represent is multiple masters on the same
AXI bus that use a different translation. Making up arbitrary
intermediate buses would get this to work, but would likely
cause other problems in the future when we do something
else that relies on having a correct representation of the
hierarchy of all the AXI/AHB/APB buses in the system, such
as doing per-bus bandwidth allocation for child devices or
anything else that requires configuring the bus bridge itself.

>> What you (we?) need is a simple bus driver that registers the
>> correct virt_to_bus()/bus_to_virt() hooks for the device that do
>> this translation at the DMA API level as well.
>
> Like I said, this only impact DMA transfers, and not the registers
> accesses. We have other devices sitting on the same bus that do not
> perform DMA accesses through that special memory bus and will not have
> that mapping changed.

virt_to_bus()/bus_to_virt() don't actually exist on modern platforms any
more, but when they did, they were only about dma access, not
mmio access, so they would correspond to what we do with
'dma-ranges' rather than 'ranges'.

Arnd

Re: [linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

On Wed, Jan 31, 2018 at 02:47:53PM +, Liviu Dudau wrote:
> On Wed, Jan 31, 2018 at 08:42:12AM +0100, Maxime Ripard wrote:
> > On Wed, Jan 31, 2018 at 03:08:08AM +, Liviu Dudau wrote:
> > > On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
> > > > Hi Maxime,
> > > > 
> > > > On Fri, 26 Jan 2018 09:46:58 +0800
> > > > Yong  wrote:
> > > > 
> > > > > Hi Maxime,
> > > > > 
> > > > > Do you have any experience in solving this problem?
> > > > > It seems the PHYS_OFFSET maybe undeclared when the ARCH is not arm.
> > > > 
> > > > Got it.
> > > > Should I add 'depends on ARM' in Kconfig?
> > > 
> > > No, I don't think you should do that, you should fix the code.
> > > 
> > > The dma_addr_t addr that you've got is ideally coming from 
> > > dma_alloc_coherent(),
> > > in which case the addr is already "suitable" for use by the device 
> > > (because the
> > > bus where the device is attached to does all the address translations).
> > 
> > Like we're discussing in that other part of the thread with Thierry
> > and Arnd, things are slightly more complicated than that :)
> 
> Yeah, sorry, my threading of the discussion was broken and I've seen
> the rest of the thread after I have replied. My bad!
> 
> > 
> > In our case, the bus where the device is attached will not do the
> > address translations, and shouldn't.
> 
> In my view, the bus is already doing address translation at physical
> level, AFAIU it remaps the memory to zero.

Not really. It uses a separate bus with a different mapping for the
DMA accesses (and only the DMA accesses). The AXI (or AHB, I'm not
sure, but, well, the "registers" bus) doesn't remap anything in
itself, and we only describe this one usually in our DTs.

> What you (we?) need is a simple bus driver that registers the
> correct virt_to_bus()/bus_to_virt() hooks for the device that do
> this translation at the DMA API level as well.

Like I said, this only impact DMA transfers, and not the registers
accesses. We have other devices sitting on the same bus that do not
perform DMA accesses through that special memory bus and will not have
that mapping changed.

> > > If you apply PHYS_OFFSET forcefully to it you might get unexpected
> > > results.
> > 
> > Out of curiosity, what would be these unexpected results?
> 
> If in the future (or a parallel world setup) the device is sitting
> behind an IOMMU, the addr value might well be smaller than
> PHYS_OFFSET and you will under-wrap, possibly starting to hit kernel
> physical addresses (or anything sitting at the top of the physical
> memory).
> 
> From my time playing with IOMMUs and PCI domains, I've learned to
> treat the dma_addr_t as a cookie value and never try to do
> arithmetics with it.

I've never worked with PCI or IOMMU, so I tend to overlook them, but
yeah, you're right :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Liviu Dudau]

On Wed, Jan 31, 2018 at 08:42:12AM +0100, Maxime Ripard wrote:
> Hi Liviu,

Hi Maxime,

> 
> On Wed, Jan 31, 2018 at 03:08:08AM +, Liviu Dudau wrote:
> > On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
> > > Hi Maxime,
> > > 
> > > On Fri, 26 Jan 2018 09:46:58 +0800
> > > Yong  wrote:
> > > 
> > > > Hi Maxime,
> > > > 
> > > > Do you have any experience in solving this problem?
> > > > It seems the PHYS_OFFSET maybe undeclared when the ARCH is not arm.
> > > 
> > > Got it.
> > > Should I add 'depends on ARM' in Kconfig?
> > 
> > No, I don't think you should do that, you should fix the code.
> > 
> > The dma_addr_t addr that you've got is ideally coming from 
> > dma_alloc_coherent(),
> > in which case the addr is already "suitable" for use by the device (because 
> > the
> > bus where the device is attached to does all the address translations).
> 
> Like we're discussing in that other part of the thread with Thierry
> and Arnd, things are slightly more complicated than that :)

Yeah, sorry, my threading of the discussion was broken and I've seen the rest 
of the 
thread after I have replied. My bad!

> 
> In our case, the bus where the device is attached will not do the
> address translations, and shouldn't.

In my view, the bus is already doing address translation at physical level, 
AFAIU it
remaps the memory to zero. What you (we?) need is a simple bus driver that 
registers
the correct virt_to_bus()/bus_to_virt() hooks for the device that do this 
translation
at the DMA API level as well.

> 
> > If you apply PHYS_OFFSET forcefully to it you might get unexpected
> > results.
> 
> Out of curiosity, what would be these unexpected results?

If in the future (or a parallel world setup) the device is sitting behind an 
IOMMU, the
addr value might well be smaller than PHYS_OFFSET and you will under-wrap, 
possibly
starting to hit kernel physical addresses (or anything sitting at the top of 
the physical
memory).

>From my time playing with IOMMUs and PCI domains, I've learned to treat the 
>dma_addr_t as
a cookie value and never try to do arithmetics with it.

Best regards,
Liviu

> 
> Thanks!
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> 
> -- 
> You received this message because you are subscribed to the Google Groups 
> "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to linux-sunxi+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Arnd Bergmann]

On Wed, Jan 31, 2018 at 8:29 AM, Maxime Ripard
 wrote:
> Hi Thierry,
>
> On Tue, Jan 30, 2018 at 11:01:50AM +0100, Thierry Reding wrote:
>> On Tue, Jan 30, 2018 at 10:59:16AM +0100, Thierry Reding wrote:
>> > On Tue, Jan 30, 2018 at 10:24:48AM +0100, Arnd Bergmann wrote:
>> > > On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
>> > >  wrote:
>> > > > On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
>> > > >> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
>> > > >>  wrote:
>> > > >> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
>> > > >> >  wrote:
>> > > >> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
>> > >
>> > > >>
>> > > >> At one point we had discussed adding a 'dma-masters' property that
>> > > >> lists all the buses on which a device can be a dma master, and
>> > > >> the respective properties of those masters (iommu, coherency,
>> > > >> offset, ...).
>> > > >>
>> > > >> IIRC at the time we decided that we could live without that 
>> > > >> complexity,
>> > > >> but perhaps we cannot.
>> > > >
>> > > > Are you talking about this ?
>> > > > https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41
>> > > >
>> > > > It doesn't seem to be related to that issue to me. And in our
>> > > > particular cases, all the devices are DMA masters, the RAM is just
>> > > > mapped to another address.
>> > >
>> > > No, that's not the one I was thinking of. The idea at the time was much
>> > > more generic, and not limited to dma engines. I don't recall the details,
>> > > but I think that Thierry was either involved or made the proposal at the
>> > > time.
>> >
>> > Yeah, I vaguely remember discussing something like this before. A quick
>> > search through my inbox yielded these two threads, mostly related to
>> > IOMMU but I think there were some mentions about dma-ranges and so on as
>> > well. I'll have to dig deeper into those threads to refresh my memories,
>> > but I won't get around to it until later today.
>> >
>> > If someone wants to read up on this in the meantime, here are the links:
>> >
>> > https://lkml.org/lkml/2014/4/27/346
>> > 
>> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257200.html
>> >
>> > From a quick glance the issue of dma-ranges was something that we hand-
>> > waved at the time.
>>
>> Also found this, which seems to be relevant as well:
>>
>>   
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252715.html
>>
>> Adding Dave.
>
> Thanks for the pointers, I started to read through it.
>
> I guess we have to come up with two solutions here: a short term one
> to address the users we already have in the kernel properly, and a
> long term one where we could use that discussion as a starting point.
>
> For the short term one, could we just set the device dma_pfn_offset to
> PHYS_OFFSET at probe time, and use our dma_addr_t directly later on,
> or would this also cause some issues?

That would certainly be an improvement over the current version,
it keeps the hack more localized. That's fine with me. Note that both
PHYS_OFFSET and dma_pfn_offset have architecture specific
meanings and they could in theory change, so ideally we'd do that
fixup somewhere in arch/arm/mach-sunxi/ at boot time before the
driver gets probed, but this wouldn't work on arm64 if we need it
there too.

> For the long term plan, from what I read from the discussion, it's
> mostly centered around IOMMU indeed, and we don't have that. What we
> would actually need is to break the assumption that the DMA "parent"
> bus is the DT node's parent bus.
>
> And I guess this could be done quite easily by adding a dma-parent
> property with a phandle to the bus controller, that would have a
> dma-ranges property of its own with the proper mapping described
> there. It should be simple enough to support, but is there anything
> that could prevent something like that to work properly (such as
> preventing further IOMMU-related developments that were described in
> those mail threads).

I've thought about it a little bit now. A dma-parent property would nicely
solve two problems:

- a device on a memory mapped control bus that is a bus master on
  a different bus. This is the case we are talking about here AFAICT

- a device that is on a different kind of bus (i2c, spi, usb, ...) but also
  happens to be a dma master on another bus. I suspect we have
  some of these today and they work by accident because we set the
  dma_mask and dma_map_ops quite liberally in the DT probe code,
  but it really shouldn't work according to our bindings. We may also
  have drivers that work around the issue by forcing the correct dma
  mask and map_ops today, which makes them work but is rather
  fragile.

I can think of a couple of other problems that may or may not be
relevant in the future that 

Re: [linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi Liviu,

On Wed, Jan 31, 2018 at 03:08:08AM +, Liviu Dudau wrote:
> On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
> > Hi Maxime,
> > 
> > On Fri, 26 Jan 2018 09:46:58 +0800
> > Yong  wrote:
> > 
> > > Hi Maxime,
> > > 
> > > Do you have any experience in solving this problem?
> > > It seems the PHYS_OFFSET maybe undeclared when the ARCH is not arm.
> > 
> > Got it.
> > Should I add 'depends on ARM' in Kconfig?
> 
> No, I don't think you should do that, you should fix the code.
> 
> The dma_addr_t addr that you've got is ideally coming from 
> dma_alloc_coherent(),
> in which case the addr is already "suitable" for use by the device (because 
> the
> bus where the device is attached to does all the address translations).

Like we're discussing in that other part of the thread with Thierry
and Arnd, things are slightly more complicated than that :)

In our case, the bus where the device is attached will not do the
address translations, and shouldn't.

> If you apply PHYS_OFFSET forcefully to it you might get unexpected
> results.

Out of curiosity, what would be these unexpected results?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi Thierry,

On Tue, Jan 30, 2018 at 11:01:50AM +0100, Thierry Reding wrote:
> On Tue, Jan 30, 2018 at 10:59:16AM +0100, Thierry Reding wrote:
> > On Tue, Jan 30, 2018 at 10:24:48AM +0100, Arnd Bergmann wrote:
> > > On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
> > >  wrote:
> > > > On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
> > > >> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
> > > >>  wrote:
> > > >> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
> > > >> >  wrote:
> > > >> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
> > > 
> > > >>
> > > >> At one point we had discussed adding a 'dma-masters' property that
> > > >> lists all the buses on which a device can be a dma master, and
> > > >> the respective properties of those masters (iommu, coherency,
> > > >> offset, ...).
> > > >>
> > > >> IIRC at the time we decided that we could live without that complexity,
> > > >> but perhaps we cannot.
> > > >
> > > > Are you talking about this ?
> > > > https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41
> > > >
> > > > It doesn't seem to be related to that issue to me. And in our
> > > > particular cases, all the devices are DMA masters, the RAM is just
> > > > mapped to another address.
> > > 
> > > No, that's not the one I was thinking of. The idea at the time was much
> > > more generic, and not limited to dma engines. I don't recall the details,
> > > but I think that Thierry was either involved or made the proposal at the
> > > time.
> > 
> > Yeah, I vaguely remember discussing something like this before. A quick
> > search through my inbox yielded these two threads, mostly related to
> > IOMMU but I think there were some mentions about dma-ranges and so on as
> > well. I'll have to dig deeper into those threads to refresh my memories,
> > but I won't get around to it until later today.
> > 
> > If someone wants to read up on this in the meantime, here are the links:
> > 
> > https://lkml.org/lkml/2014/4/27/346
> > 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257200.html
> > 
> > From a quick glance the issue of dma-ranges was something that we hand-
> > waved at the time.
> 
> Also found this, which seems to be relevant as well:
> 
>   
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252715.html
> 
> Adding Dave.

Thanks for the pointers, I started to read through it.

I guess we have to come up with two solutions here: a short term one
to address the users we already have in the kernel properly, and a
long term one where we could use that discussion as a starting point.

For the short term one, could we just set the device dma_pfn_offset to
PHYS_OFFSET at probe time, and use our dma_addr_t directly later on,
or would this also cause some issues?

For the long term plan, from what I read from the discussion, it's
mostly centered around IOMMU indeed, and we don't have that. What we
would actually need is to break the assumption that the DMA "parent"
bus is the DT node's parent bus.

And I guess this could be done quite easily by adding a dma-parent
property with a phandle to the bus controller, that would have a
dma-ranges property of its own with the proper mapping described
there. It should be simple enough to support, but is there anything
that could prevent something like that to work properly (such as
preventing further IOMMU-related developments that were described in
those mail threads).

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Chen-Yu Tsai]

On Wed, Jan 31, 2018 at 11:08 AM, Liviu Dudau  wrote:
> On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
>> Hi Maxime,
>>
>> On Fri, 26 Jan 2018 09:46:58 +0800
>> Yong  wrote:
>>
>> > Hi Maxime,
>> >
>> > Do you have any experience in solving this problem?
>> > It seems the PHYS_OFFSET maybe undeclared when the ARCH is not arm.
>>
>> Got it.
>> Should I add 'depends on ARM' in Kconfig?
>
> No, I don't think you should do that, you should fix the code.
>
> The dma_addr_t addr that you've got is ideally coming from 
> dma_alloc_coherent(),
> in which case the addr is already "suitable" for use by the device (because 
> the
> bus where the device is attached to does all the address translations). If you
> apply PHYS_OFFSET forcefully to it you might get unexpected results.

As explained in the thread, the dma_addr_t address is based on the kernel
and processor's viewpoint, which has DRAM at an offset. This particular
peripheral (and some others, such as display and video decoder) on Allwinner
platforms do DMA on the separate memory bus directly, which does _not_
have that memory offset. This is specific to our hardware. And also mentioned
is that there is no sensible representation in the device tree that would
allow the DMA API to do proper address translation.

Just throwing it out there, maybe we could do a dummy IOMMU that does the
simple translation of (addr - PHYS_OFFSET)? Still I'm not sure if the device
tree representation would be sane.

ChenYu

>>
>> >
>> > On Fri, 26 Jan 2018 08:04:18 +0800
>> > kbuild test robot  wrote:
>> >
>> > > Hi Yong,
>> > >
>> > > I love your patch! Yet something to improve:
>> > >
>> > > [auto build test ERROR on linuxtv-media/master]
>> > > [also build test ERROR on v4.15-rc9 next-20180119]
>> > > [if your patch is applied to the wrong git tree, please drop us a note 
>> > > to help improve the system]
>> > >
>> > > url:
>> > > https://github.com/0day-ci/linux/commits/Yong-Deng/dt-bindings-media-Add-Allwinner-V3s-Camera-Sensor-Interface-CSI/20180126-054511
>> > > base:   git://linuxtv.org/media_tree.git master
>> > > config: i386-allmodconfig (attached as .config)
>> > > compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
>> > > reproduce:
>> > > # save the attached .config to linux build tree
>> > > make ARCH=i386
>> > >
>> > > All errors (new ones prefixed by >>):
>> > >
>> > >drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c: In function 
>> > > 'sun6i_csi_update_buf_addr':
>> > > >> drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: error: 
>> > > >> 'PHYS_OFFSET' undeclared (first use in this function); did you mean 
>> > > >> 'PAGE_OFFSET'?
>> > >  dma_addr_t bus_addr = addr - PHYS_OFFSET;
>> > >   ^~~
>> > >   PAGE_OFFSET
>> > >drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: note: each 
>> > > undeclared identifier is reported only once for each function it appears 
>> > > in
>> > >
>> > > vim +567 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
>> > >
>> > >562
>> > >563void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, 
>> > > dma_addr_t addr)
>> > >564{
>> > >565struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
>> > >566/* transform physical address to bus address */
>> > >  > 567dma_addr_t bus_addr = addr - PHYS_OFFSET;
>> > >568
>> > >569regmap_write(sdev->regmap, CSI_CH_F0_BUFA_REG,
>> > >570 (bus_addr + sdev->planar_offset[0]) >> 2);
>> > >571if (sdev->planar_offset[1] != -1)
>> > >572regmap_write(sdev->regmap, CSI_CH_F1_BUFA_REG,
>> > >573 (bus_addr + 
>> > > sdev->planar_offset[1]) >> 2);
>> > >574if (sdev->planar_offset[2] != -1)
>> > >575regmap_write(sdev->regmap, CSI_CH_F2_BUFA_REG,
>> > >576 (bus_addr + 
>> > > sdev->planar_offset[2]) >> 2);
>> > >577}
>> > >578
>> > >
>> > > ---
>> > > 0-DAY kernel test infrastructureOpen Source Technology 
>> > > Center
>> > > https://lists.01.org/pipermail/kbuild-all   Intel 
>> > > Corporation
>> >
>> >
>> > Thanks,
>> > Yong
>>
>>
>> Thanks,
>> Yong
>>
>> --
>> You received this message because you are subscribed to the Google Groups 
>> "linux-sunxi" group.
>> To unsubscribe from this group and stop receiving emails from it, send an 
>> email to linux-sunxi+unsubscr...@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.

Re: [linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Liviu Dudau]

On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
> Hi Maxime,
> 
> On Fri, 26 Jan 2018 09:46:58 +0800
> Yong  wrote:
> 
> > Hi Maxime,
> > 
> > Do you have any experience in solving this problem?
> > It seems the PHYS_OFFSET maybe undeclared when the ARCH is not arm.
> 
> Got it.
> Should I add 'depends on ARM' in Kconfig?

No, I don't think you should do that, you should fix the code.

The dma_addr_t addr that you've got is ideally coming from dma_alloc_coherent(),
in which case the addr is already "suitable" for use by the device (because the
bus where the device is attached to does all the address translations). If you
apply PHYS_OFFSET forcefully to it you might get unexpected results.

Best regards,
Liviu

> 
> > 
> > On Fri, 26 Jan 2018 08:04:18 +0800
> > kbuild test robot  wrote:
> > 
> > > Hi Yong,
> > > 
> > > I love your patch! Yet something to improve:
> > > 
> > > [auto build test ERROR on linuxtv-media/master]
> > > [also build test ERROR on v4.15-rc9 next-20180119]
> > > [if your patch is applied to the wrong git tree, please drop us a note to 
> > > help improve the system]
> > > 
> > > url:
> > > https://github.com/0day-ci/linux/commits/Yong-Deng/dt-bindings-media-Add-Allwinner-V3s-Camera-Sensor-Interface-CSI/20180126-054511
> > > base:   git://linuxtv.org/media_tree.git master
> > > config: i386-allmodconfig (attached as .config)
> > > compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
> > > reproduce:
> > > # save the attached .config to linux build tree
> > > make ARCH=i386 
> > > 
> > > All errors (new ones prefixed by >>):
> > > 
> > >drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c: In function 
> > > 'sun6i_csi_update_buf_addr':
> > > >> drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: error: 
> > > >> 'PHYS_OFFSET' undeclared (first use in this function); did you mean 
> > > >> 'PAGE_OFFSET'?
> > >  dma_addr_t bus_addr = addr - PHYS_OFFSET;
> > >   ^~~
> > >   PAGE_OFFSET
> > >drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: note: each 
> > > undeclared identifier is reported only once for each function it appears 
> > > in
> > > 
> > > vim +567 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > 
> > >562
> > >563void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, 
> > > dma_addr_t addr)
> > >564{
> > >565struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > >566/* transform physical address to bus address */
> > >  > 567dma_addr_t bus_addr = addr - PHYS_OFFSET;
> > >568
> > >569regmap_write(sdev->regmap, CSI_CH_F0_BUFA_REG,
> > >570 (bus_addr + sdev->planar_offset[0]) >> 2);
> > >571if (sdev->planar_offset[1] != -1)
> > >572regmap_write(sdev->regmap, CSI_CH_F1_BUFA_REG,
> > >573 (bus_addr + 
> > > sdev->planar_offset[1]) >> 2);
> > >574if (sdev->planar_offset[2] != -1)
> > >575regmap_write(sdev->regmap, CSI_CH_F2_BUFA_REG,
> > >576 (bus_addr + 
> > > sdev->planar_offset[2]) >> 2);
> > >577}
> > >578
> > > 
> > > ---
> > > 0-DAY kernel test infrastructureOpen Source Technology 
> > > Center
> > > https://lists.01.org/pipermail/kbuild-all   Intel 
> > > Corporation
> > 
> > 
> > Thanks,
> > Yong
> 
> 
> Thanks,
> Yong
> 
> -- 
> You received this message because you are subscribed to the Google Groups 
> "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to linux-sunxi+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Thierry Reding]

On Tue, Jan 30, 2018 at 10:24:48AM +0100, Arnd Bergmann wrote:
> On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
>  wrote:
> > On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
> >> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
> >>  wrote:
> >> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
> >> >  wrote:
> >> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
> 
> >>
> >> At one point we had discussed adding a 'dma-masters' property that
> >> lists all the buses on which a device can be a dma master, and
> >> the respective properties of those masters (iommu, coherency,
> >> offset, ...).
> >>
> >> IIRC at the time we decided that we could live without that complexity,
> >> but perhaps we cannot.
> >
> > Are you talking about this ?
> > https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41
> >
> > It doesn't seem to be related to that issue to me. And in our
> > particular cases, all the devices are DMA masters, the RAM is just
> > mapped to another address.
> 
> No, that's not the one I was thinking of. The idea at the time was much
> more generic, and not limited to dma engines. I don't recall the details,
> but I think that Thierry was either involved or made the proposal at the
> time.

Yeah, I vaguely remember discussing something like this before. A quick
search through my inbox yielded these two threads, mostly related to
IOMMU but I think there were some mentions about dma-ranges and so on as
well. I'll have to dig deeper into those threads to refresh my memories,
but I won't get around to it until later today.

If someone wants to read up on this in the meantime, here are the links:

https://lkml.org/lkml/2014/4/27/346

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257200.html

From a quick glance the issue of dma-ranges was something that we hand-
waved at the time.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Thierry Reding]

On Tue, Jan 30, 2018 at 10:59:16AM +0100, Thierry Reding wrote:
> On Tue, Jan 30, 2018 at 10:24:48AM +0100, Arnd Bergmann wrote:
> > On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
> >  wrote:
> > > On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
> > >> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
> > >>  wrote:
> > >> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
> > >> >  wrote:
> > >> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
> > 
> > >>
> > >> At one point we had discussed adding a 'dma-masters' property that
> > >> lists all the buses on which a device can be a dma master, and
> > >> the respective properties of those masters (iommu, coherency,
> > >> offset, ...).
> > >>
> > >> IIRC at the time we decided that we could live without that complexity,
> > >> but perhaps we cannot.
> > >
> > > Are you talking about this ?
> > > https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41
> > >
> > > It doesn't seem to be related to that issue to me. And in our
> > > particular cases, all the devices are DMA masters, the RAM is just
> > > mapped to another address.
> > 
> > No, that's not the one I was thinking of. The idea at the time was much
> > more generic, and not limited to dma engines. I don't recall the details,
> > but I think that Thierry was either involved or made the proposal at the
> > time.
> 
> Yeah, I vaguely remember discussing something like this before. A quick
> search through my inbox yielded these two threads, mostly related to
> IOMMU but I think there were some mentions about dma-ranges and so on as
> well. I'll have to dig deeper into those threads to refresh my memories,
> but I won't get around to it until later today.
> 
> If someone wants to read up on this in the meantime, here are the links:
> 
>   https://lkml.org/lkml/2014/4/27/346
>   
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257200.html
> 
> From a quick glance the issue of dma-ranges was something that we hand-
> waved at the time.
> 
> Thierry

Also found this, which seems to be relevant as well:


http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252715.html

Adding Dave.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Arnd Bergmann]

On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
 wrote:
> On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
>> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
>>  wrote:
>> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
>> >  wrote:
>> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:

>>
>> At one point we had discussed adding a 'dma-masters' property that
>> lists all the buses on which a device can be a dma master, and
>> the respective properties of those masters (iommu, coherency,
>> offset, ...).
>>
>> IIRC at the time we decided that we could live without that complexity,
>> but perhaps we cannot.
>
> Are you talking about this ?
> https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41
>
> It doesn't seem to be related to that issue to me. And in our
> particular cases, all the devices are DMA masters, the RAM is just
> mapped to another address.

No, that's not the one I was thinking of. The idea at the time was much
more generic, and not limited to dma engines. I don't recall the details,
but I think that Thierry was either involved or made the proposal at the
time.

   Arnd

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi,

On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
>  wrote:
> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
> >  wrote:
> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
> >> However, in DT systems, that
> >> field is filled only with the parent's node dma-ranges property. In
> >> our case, and since the DT parenthood is based on the "control" bus,
> >> and not the "data" bus, our parent node would be the AXI bus, and not
> >> the memory bus that enforce those constraints.
> >>
> >> And other devices doing DMA through regular DMA accesses won't have
> >> that mapping, so we definitely shouldn't enforce it for all the
> >> devices there, but only the one connected to the separate memory bus.
> >>
> >> tl; dr: the DT is not really an option to store that info.
> >>
> >> I suggested setting dma_pfn_offset at probe, but Arnd didn't seem too
> >> fond of that approach either at the time.
> >>
> >> So, well, I guess we could do better. We just have no idea how :)
> >
> > Usually of Arnd cannot suggest something smart, neither can I :(
> >
> > If some aspect of the memory layout of the system *really* doesn't
> > fit into DT because of the assumptions that DT is doing about
> > how systems work, we have a problem.
> >
> > Is the problem that DT's model assumes that devices have one and
> > only one data port to the system memory, and that is how it
> > populates the Linux devices?
> >
> > I guess, if nothing else works, I would use auxdata in the board file.
> > It is our definitive last resort when DT doesn't hold.
> >
> > I.e. I would go into struct of_dev_auxdata
> > (from ) and add a
> > dma_pfn_offset field that gets set into the device's dma_pfn_offset
> > in drivers/of/platform.c overriding anything else and use that to hammer
> > it down in the boardfile.
> >
> > But I bet some DT people are going to have other ideas.
> 
> At one point we had discussed adding a 'dma-masters' property that
> lists all the buses on which a device can be a dma master, and
> the respective properties of those masters (iommu, coherency,
> offset, ...).
>
> IIRC at the time we decided that we could live without that complexity,
> but perhaps we cannot.

Are you talking about this ?
https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41

It doesn't seem to be related to that issue to me. And in our
particular cases, all the devices are DMA masters, the RAM is just
mapped to another address.

> Another local hack that we can do here is to have two separate
> device nodes and let the device driver bind to one device and then
> look up the other one through a phandle to look up a platform_device
> for it, which it can then use with the DMA mapping interface.

We have multiple devices doing that, a couple of them already merged
(mostly on the display side). I'd rather not do that.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Arnd Bergmann]

On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
 wrote:
> On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
>  wrote:
>> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
>> However, in DT systems, that
>> field is filled only with the parent's node dma-ranges property. In
>> our case, and since the DT parenthood is based on the "control" bus,
>> and not the "data" bus, our parent node would be the AXI bus, and not
>> the memory bus that enforce those constraints.
>>
>> And other devices doing DMA through regular DMA accesses won't have
>> that mapping, so we definitely shouldn't enforce it for all the
>> devices there, but only the one connected to the separate memory bus.
>>
>> tl; dr: the DT is not really an option to store that info.
>>
>> I suggested setting dma_pfn_offset at probe, but Arnd didn't seem too
>> fond of that approach either at the time.
>>
>> So, well, I guess we could do better. We just have no idea how :)
>
> Usually of Arnd cannot suggest something smart, neither can I :(
>
> If some aspect of the memory layout of the system *really* doesn't
> fit into DT because of the assumptions that DT is doing about
> how systems work, we have a problem.
>
> Is the problem that DT's model assumes that devices have one and
> only one data port to the system memory, and that is how it
> populates the Linux devices?
>
> I guess, if nothing else works, I would use auxdata in the board file.
> It is our definitive last resort when DT doesn't hold.
>
> I.e. I would go into struct of_dev_auxdata
> (from ) and add a
> dma_pfn_offset field that gets set into the device's dma_pfn_offset
> in drivers/of/platform.c overriding anything else and use that to hammer
> it down in the boardfile.
>
> But I bet some DT people are going to have other ideas.

At one point we had discussed adding a 'dma-masters' property that
lists all the buses on which a device can be a dma master, and
the respective properties of those masters (iommu, coherency,
offset, ...).

IIRC at the time we decided that we could live without that complexity,
but perhaps we cannot.

Another local hack that we can do here is to have two separate
device nodes and let the device driver bind to one device and then
look up the other one through a phandle to look up a platform_device
for it, which it can then use with the DMA mapping interface.

  Arnd

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Linus Walleij]

On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
 wrote:
> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
>> > +void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
>> > +{
>> > +   struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
>> > +   /* transform physical address to bus address */
>> > +   dma_addr_t bus_addr = addr - PHYS_OFFSET;
>>
>> I am sorry if this is an unjustified drive-by comment. Maybe you
>> have already investigate other ways to do this.
>
> It's definitely not unjustified :)
>
>> Accessing PHYS_OFFSET directly seems unintuitive and not good
>> practice.
>>
>> But normally an dma_addr_t only comes from some function inside
>>  such as: dma_alloc_coherent() for a contigous
>> buffer which is coherent in physical memory, or from some buffer <=
>> 64KB that is switching ownership between device and CPU explicitly
>> with dma_map* or so. Did you check with Documentation/DMA-API.txt?
>
> So, I've discussed this with Arnd a month ago or so, because I'm not
> really fond of the current approach but we haven't found better way to
> do it yet.
>
> The issue is that all the DMA accesses are done not through the main
> AXI bus, but through a separate bus dedicated for memory accesses,
> where the RAM is mapped at the address 0. So the CPU and DMA devices
> have a different mapping for the RAM.

Aha, I see the problem ... but since the dma_addr_t is supposed
to actually be the address the DMA controller sees, something is
apparently broken.

> I guess we could address this by using the field dma_pfn_offset that
> seems to be used in similar situations.

That does seem like the right thing to do (to me).

> However, in DT systems, that
> field is filled only with the parent's node dma-ranges property. In
> our case, and since the DT parenthood is based on the "control" bus,
> and not the "data" bus, our parent node would be the AXI bus, and not
> the memory bus that enforce those constraints.
>
> And other devices doing DMA through regular DMA accesses won't have
> that mapping, so we definitely shouldn't enforce it for all the
> devices there, but only the one connected to the separate memory bus.
>
> tl; dr: the DT is not really an option to store that info.
>
> I suggested setting dma_pfn_offset at probe, but Arnd didn't seem too
> fond of that approach either at the time.
>
> So, well, I guess we could do better. We just have no idea how :)

Usually of Arnd cannot suggest something smart, neither can I :(

If some aspect of the memory layout of the system *really* doesn't
fit into DT because of the assumptions that DT is doing about
how systems work, we have a problem.

Is the problem that DT's model assumes that devices have one and
only one data port to the system memory, and that is how it
populates the Linux devices?

I guess, if nothing else works, I would use auxdata in the board file.
It is our definitive last resort when DT doesn't hold.

I.e. I would go into struct of_dev_auxdata
(from ) and add a
dma_pfn_offset field that gets set into the device's dma_pfn_offset
in drivers/of/platform.c overriding anything else and use that to hammer
it down in the boardfile.

But I bet some DT people are going to have other ideas.

Yours,
Linus Walleij

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi Linus,

On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
> > +void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
> > +{
> > +   struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > +   /* transform physical address to bus address */
> > +   dma_addr_t bus_addr = addr - PHYS_OFFSET;
> 
> I am sorry if this is an unjustified drive-by comment. Maybe you
> have already investigate other ways to do this.

It's definitely not unjustified :)

> Accessing PHYS_OFFSET directly seems unintuitive and not good
> practice.
> 
> But normally an dma_addr_t only comes from some function inside
>  such as: dma_alloc_coherent() for a contigous
> buffer which is coherent in physical memory, or from some buffer <=
> 64KB that is switching ownership between device and CPU explicitly
> with dma_map* or so. Did you check with Documentation/DMA-API.txt?

So, I've discussed this with Arnd a month ago or so, because I'm not
really fond of the current approach but we haven't found better way to
do it yet.

The issue is that all the DMA accesses are done not through the main
AXI bus, but through a separate bus dedicated for memory accesses,
where the RAM is mapped at the address 0. So the CPU and DMA devices
have a different mapping for the RAM.

I guess we could address this by using the field dma_pfn_offset that
seems to be used in similar situations. However, in DT systems, that
field is filled only with the parent's node dma-ranges property. In
our case, and since the DT parenthood is based on the "control" bus,
and not the "data" bus, our parent node would be the AXI bus, and not
the memory bus that enforce those constraints.

And other devices doing DMA through regular DMA accesses won't have
that mapping, so we definitely shouldn't enforce it for all the
devices there, but only the one connected to the separate memory bus.

tl; dr: the DT is not really an option to store that info.

I suggested setting dma_pfn_offset at probe, but Arnd didn't seem too
fond of that approach either at the time.

So, well, I guess we could do better. We just have no idea how :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi,

On Sun, Jan 28, 2018 at 10:19:03AM +0800, Yong wrote:
> Hi Maxime,
> 
> On Fri, 26 Jan 2018 09:10:00 +0100
> Maxime Ripard  wrote:
> 
> > On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
> > > Hi Maxime,
> > > 
> > > On Fri, 26 Jan 2018 09:46:58 +0800
> > > Yong  wrote:
> > > 
> > > > Hi Maxime,
> > > > 
> > > > Do you have any experience in solving this problem?
> > > > It seems the PHYS_OFFSET maybe undeclared when the ARCH is not arm.
> > > 
> > > Got it.
> > > Should I add 'depends on ARM' in Kconfig?
> > 
> > Yes, or even better a depends on MACH_SUNXI :)
> 
> Do you mean ARCH_SUNXI?

Yeah, sorry :)

> ARCH_SUNXI is alreay there. In the early version, my Kconfig is like this:
> 
>   depends on ARCH_SUNXI
> 
> But Hans suggest me to change this to:
> 
>   depends on ARCH_SUNXI || COMPILE_TEST
> 
> to allow this driver to be compiled on e.g. Intel for compile testing.
> 
> Should we get rid of COMPILE_TEST?

Yes, it cannot be compiled as is on anything but ARM and a few
architectures. Or maybe something like || (COMPILE_TEST && ARM) if
that makes sense?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi,

On Sat, 27 Jan 2018 17:14:26 +0100
Linus Walleij  wrote:

> On Tue, Jan 23, 2018 at 9:18 AM, Yong Deng  wrote:
> 
> > Allwinner V3s SoC features two CSI module. CSI0 is used for MIPI CSI-2
> > interface and CSI1 is used for parallel interface. This is not
> > documented in datasheet but by test and guess.
> >
> > This patch implement a v4l2 framework driver for it.
> >
> > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > ISP's support are not included in this patch.
> >
> > Tested-by: Maxime Ripard 
> > Signed-off-by: Yong Deng 
> 
> This is cool stuff :)
> 
> > +void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
> > +{
> > +   struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > +   /* transform physical address to bus address */
> > +   dma_addr_t bus_addr = addr - PHYS_OFFSET;
> 
> I am sorry if this is an unjustified drive-by comment. Maybe you
> have already investigate other ways to do this.
> 
> Accessing PHYS_OFFSET directly seems unintuitive
> and not good practice.
> 
> But normally an dma_addr_t only comes from some
> function inside  such as:
> dma_alloc_coherent() for a contigous buffer which is coherent
> in physical memory, or from some buffer <= 64KB that
> is switching ownership between device and CPU explicitly
> with dma_map* or so. Did you check with
> Documentation/DMA-API.txt?

The dma_addr_t here comes from v4l2 vb2 and it's already 'mapped'.

Maybe the dma-mapping code of sunXi does not do conversion between 
device and CPU. I'm not familiar with this. 

Maxime, do you have any idea about this?
Can we get bus address directly from dma_alloc_coherent or dma_map*
at the system layer but not doing the conversion per driver?

Yong

Re: [linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi Maxime,

On Fri, 26 Jan 2018 09:10:00 +0100
Maxime Ripard  wrote:

> On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
> > Hi Maxime,
> > 
> > On Fri, 26 Jan 2018 09:46:58 +0800
> > Yong  wrote:
> > 
> > > Hi Maxime,
> > > 
> > > Do you have any experience in solving this problem?
> > > It seems the PHYS_OFFSET maybe undeclared when the ARCH is not arm.
> > 
> > Got it.
> > Should I add 'depends on ARM' in Kconfig?
> 
> Yes, or even better a depends on MACH_SUNXI :)

Do you mean ARCH_SUNXI?

ARCH_SUNXI is alreay there. In the early version, my Kconfig is like this:

depends on ARCH_SUNXI

But Hans suggest me to change this to:

depends on ARCH_SUNXI || COMPILE_TEST

to allow this driver to be compiled on e.g. Intel for compile testing.

Should we get rid of COMPILE_TEST?

Yong

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Linus Walleij]

On Tue, Jan 23, 2018 at 9:18 AM, Yong Deng  wrote:

> Allwinner V3s SoC features two CSI module. CSI0 is used for MIPI CSI-2
> interface and CSI1 is used for parallel interface. This is not
> documented in datasheet but by test and guess.
>
> This patch implement a v4l2 framework driver for it.
>
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
>
> Tested-by: Maxime Ripard 
> Signed-off-by: Yong Deng 

This is cool stuff :)

> +void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
> +{
> +   struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> +   /* transform physical address to bus address */
> +   dma_addr_t bus_addr = addr - PHYS_OFFSET;

I am sorry if this is an unjustified drive-by comment. Maybe you
have already investigate other ways to do this.

Accessing PHYS_OFFSET directly seems unintuitive
and not good practice.

But normally an dma_addr_t only comes from some
function inside  such as:
dma_alloc_coherent() for a contigous buffer which is coherent
in physical memory, or from some buffer <= 64KB that
is switching ownership between device and CPU explicitly
with dma_map* or so. Did you check with
Documentation/DMA-API.txt?

Yours,
Linus Walleij

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[kbuild test robot]

Hi Yong,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.15-rc9 next-20180126]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Yong-Deng/dt-bindings-media-Add-Allwinner-V3s-Camera-Sensor-Interface-CSI/20180126-054511
base:   git://linuxtv.org/media_tree.git master
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c: In function 
'sun6i_csi_update_buf_addr':
>> drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: error: 
>> 'PHYS_OFFSET' undeclared (first use in this function)
 dma_addr_t bus_addr = addr - PHYS_OFFSET;
  ^~~
   drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: note: each 
undeclared identifier is reported only once for each function it appears in

vim +/PHYS_OFFSET +567 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c

   562  
   563  void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
   564  {
   565  struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
   566  /* transform physical address to bus address */
 > 567  dma_addr_t bus_addr = addr - PHYS_OFFSET;
   568  
   569  regmap_write(sdev->regmap, CSI_CH_F0_BUFA_REG,
   570   (bus_addr + sdev->planar_offset[0]) >> 2);
   571  if (sdev->planar_offset[1] != -1)
   572  regmap_write(sdev->regmap, CSI_CH_F1_BUFA_REG,
   573   (bus_addr + sdev->planar_offset[1]) >> 2);
   574  if (sdev->planar_offset[2] != -1)
   575  regmap_write(sdev->regmap, CSI_CH_F2_BUFA_REG,
   576   (bus_addr + sdev->planar_offset[2]) >> 2);
   577  }
   578  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

On Fri, Jan 26, 2018 at 11:00:41AM +0800, Yong wrote:
> Hi Maxime,
> 
> On Fri, 26 Jan 2018 09:46:58 +0800
> Yong  wrote:
> 
> > Hi Maxime,
> > 
> > Do you have any experience in solving this problem?
> > It seems the PHYS_OFFSET maybe undeclared when the ARCH is not arm.
> 
> Got it.
> Should I add 'depends on ARM' in Kconfig?

Yes, or even better a depends on MACH_SUNXI :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi Maxime,

On Fri, 26 Jan 2018 09:46:58 +0800
Yong  wrote:

> Hi Maxime,
> 
> Do you have any experience in solving this problem?
> It seems the PHYS_OFFSET maybe undeclared when the ARCH is not arm.

Got it.
Should I add 'depends on ARM' in Kconfig?

> 
> On Fri, 26 Jan 2018 08:04:18 +0800
> kbuild test robot  wrote:
> 
> > Hi Yong,
> > 
> > I love your patch! Yet something to improve:
> > 
> > [auto build test ERROR on linuxtv-media/master]
> > [also build test ERROR on v4.15-rc9 next-20180119]
> > [if your patch is applied to the wrong git tree, please drop us a note to 
> > help improve the system]
> > 
> > url:
> > https://github.com/0day-ci/linux/commits/Yong-Deng/dt-bindings-media-Add-Allwinner-V3s-Camera-Sensor-Interface-CSI/20180126-054511
> > base:   git://linuxtv.org/media_tree.git master
> > config: i386-allmodconfig (attached as .config)
> > compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
> > reproduce:
> > # save the attached .config to linux build tree
> > make ARCH=i386 
> > 
> > All errors (new ones prefixed by >>):
> > 
> >drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c: In function 
> > 'sun6i_csi_update_buf_addr':
> > >> drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: error: 
> > >> 'PHYS_OFFSET' undeclared (first use in this function); did you mean 
> > >> 'PAGE_OFFSET'?
> >  dma_addr_t bus_addr = addr - PHYS_OFFSET;
> >   ^~~
> >   PAGE_OFFSET
> >drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: note: each 
> > undeclared identifier is reported only once for each function it appears in
> > 
> > vim +567 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > 
> >562  
> >563  void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, 
> > dma_addr_t addr)
> >564  {
> >565  struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> >566  /* transform physical address to bus address */
> >  > 567  dma_addr_t bus_addr = addr - PHYS_OFFSET;
> >568  
> >569  regmap_write(sdev->regmap, CSI_CH_F0_BUFA_REG,
> >570   (bus_addr + sdev->planar_offset[0]) >> 2);
> >571  if (sdev->planar_offset[1] != -1)
> >572  regmap_write(sdev->regmap, CSI_CH_F1_BUFA_REG,
> >573   (bus_addr + 
> > sdev->planar_offset[1]) >> 2);
> >574  if (sdev->planar_offset[2] != -1)
> >575  regmap_write(sdev->regmap, CSI_CH_F2_BUFA_REG,
> >576   (bus_addr + 
> > sdev->planar_offset[2]) >> 2);
> >577  }
> >578  
> > 
> > ---
> > 0-DAY kernel test infrastructureOpen Source Technology 
> > Center
> > https://lists.01.org/pipermail/kbuild-all   Intel 
> > Corporation
> 
> 
> Thanks,
> Yong


Thanks,
Yong

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi Maxime,

Do you have any experience in solving this problem?
It seems the PHYS_OFFSET maybe undeclared when the ARCH is not arm.

On Fri, 26 Jan 2018 08:04:18 +0800
kbuild test robot  wrote:

> Hi Yong,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linuxtv-media/master]
> [also build test ERROR on v4.15-rc9 next-20180119]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Yong-Deng/dt-bindings-media-Add-Allwinner-V3s-Camera-Sensor-Interface-CSI/20180126-054511
> base:   git://linuxtv.org/media_tree.git master
> config: i386-allmodconfig (attached as .config)
> compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c: In function 
> 'sun6i_csi_update_buf_addr':
> >> drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: error: 
> >> 'PHYS_OFFSET' undeclared (first use in this function); did you mean 
> >> 'PAGE_OFFSET'?
>  dma_addr_t bus_addr = addr - PHYS_OFFSET;
>   ^~~
>   PAGE_OFFSET
>drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: note: each 
> undeclared identifier is reported only once for each function it appears in
> 
> vim +567 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> 
>562
>563void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, 
> dma_addr_t addr)
>564{
>565struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
>566/* transform physical address to bus address */
>  > 567dma_addr_t bus_addr = addr - PHYS_OFFSET;
>568
>569regmap_write(sdev->regmap, CSI_CH_F0_BUFA_REG,
>570 (bus_addr + sdev->planar_offset[0]) >> 2);
>571if (sdev->planar_offset[1] != -1)
>572regmap_write(sdev->regmap, CSI_CH_F1_BUFA_REG,
>573 (bus_addr + 
> sdev->planar_offset[1]) >> 2);
>574if (sdev->planar_offset[2] != -1)
>575regmap_write(sdev->regmap, CSI_CH_F2_BUFA_REG,
>576 (bus_addr + 
> sdev->planar_offset[2]) >> 2);
>577}
>578
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation


Thanks,
Yong

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[kbuild test robot]

Hi Yong,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.15-rc9 next-20180119]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Yong-Deng/dt-bindings-media-Add-Allwinner-V3s-Camera-Sensor-Interface-CSI/20180126-054511
base:   git://linuxtv.org/media_tree.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c: In function 
'sun6i_csi_update_buf_addr':
>> drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: error: 
>> 'PHYS_OFFSET' undeclared (first use in this function); did you mean 
>> 'PAGE_OFFSET'?
 dma_addr_t bus_addr = addr - PHYS_OFFSET;
  ^~~
  PAGE_OFFSET
   drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:567:31: note: each 
undeclared identifier is reported only once for each function it appears in

vim +567 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c

   562  
   563  void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
   564  {
   565  struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
   566  /* transform physical address to bus address */
 > 567  dma_addr_t bus_addr = addr - PHYS_OFFSET;
   568  
   569  regmap_write(sdev->regmap, CSI_CH_F0_BUFA_REG,
   570   (bus_addr + sdev->planar_offset[0]) >> 2);
   571  if (sdev->planar_offset[1] != -1)
   572  regmap_write(sdev->regmap, CSI_CH_F1_BUFA_REG,
   573   (bus_addr + sdev->planar_offset[1]) >> 2);
   574  if (sdev->planar_offset[2] != -1)
   575  regmap_write(sdev->regmap, CSI_CH_F2_BUFA_REG,
   576   (bus_addr + sdev->planar_offset[2]) >> 2);
   577  }
   578  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Yong Deng]

Allwinner V3s SoC features two CSI module. CSI0 is used for MIPI CSI-2
interface and CSI1 is used for parallel interface. This is not
documented in datasheet but by test and guess.

This patch implement a v4l2 framework driver for it.

Currently, the driver only support the parallel interface. MIPI-CSI2,
ISP's support are not included in this patch.

Tested-by: Maxime Ripard 
Signed-off-by: Yong Deng 
---
 MAINTAINERS|   8 +
 drivers/media/platform/Kconfig |   1 +
 drivers/media/platform/Makefile|   2 +
 drivers/media/platform/sunxi/sun6i-csi/Kconfig |   9 +
 drivers/media/platform/sunxi/sun6i-csi/Makefile|   3 +
 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c | 908 +
 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h | 143 
 .../media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h | 196 +
 .../media/platform/sunxi/sun6i-csi/sun6i_video.c   | 753 +
 .../media/platform/sunxi/sun6i-csi/sun6i_video.h   |  53 ++
 10 files changed, 2076 insertions(+)
 create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Kconfig
 create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Makefile
 create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
 create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
 create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h
 create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
 create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 9501403..b792fe5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3783,6 +3783,14 @@ M:   Jaya Kumar 
 S: Maintained
 F: sound/pci/cs5535audio/
 
+CSI DRIVERS FOR ALLWINNER V3s
+M: Yong Deng 
+L: linux-media@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/platform/sunxi/sun6i-csi/
+F: Documentation/devicetree/bindings/media/sun6i-csi.txt
+
 CW1200 WLAN driver
 M: Solomon Peachy 
 S: Maintained
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index fd0c998..41017e3 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -150,6 +150,7 @@ source "drivers/media/platform/am437x/Kconfig"
 source "drivers/media/platform/xilinx/Kconfig"
 source "drivers/media/platform/rcar-vin/Kconfig"
 source "drivers/media/platform/atmel/Kconfig"
+source "drivers/media/platform/sunxi/sun6i-csi/Kconfig"
 
 config VIDEO_TI_CAL
tristate "TI CAL (Camera Adaptation Layer) driver"
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 003b0bb..e6e9ce7 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -97,3 +97,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)+= 
qcom/camss-8x16/
 obj-$(CONFIG_VIDEO_QCOM_VENUS) += qcom/venus/
 
 obj-y  += meson/
+
+obj-$(CONFIG_VIDEO_SUN6I_CSI)  += sunxi/sun6i-csi/
diff --git a/drivers/media/platform/sunxi/sun6i-csi/Kconfig 
b/drivers/media/platform/sunxi/sun6i-csi/Kconfig
new file mode 100644
index 000..314188a
--- /dev/null
+++ b/drivers/media/platform/sunxi/sun6i-csi/Kconfig
@@ -0,0 +1,9 @@
+config VIDEO_SUN6I_CSI
+   tristate "Allwinner V3s Camera Sensor Interface driver"
+   depends on VIDEO_V4L2 && COMMON_CLK && VIDEO_V4L2_SUBDEV_API && HAS_DMA
+   depends on ARCH_SUNXI || COMPILE_TEST
+   select VIDEOBUF2_DMA_CONTIG
+   select REGMAP_MMIO
+   select V4L2_FWNODE
+   ---help---
+  Support for the Allwinner Camera Sensor Interface Controller on V3s.
diff --git a/drivers/media/platform/sunxi/sun6i-csi/Makefile 
b/drivers/media/platform/sunxi/sun6i-csi/Makefile
new file mode 100644
index 000..213cb6b
--- /dev/null
+++ b/drivers/media/platform/sunxi/sun6i-csi/Makefile
@@ -0,0 +1,3 @@
+sun6i-csi-y += sun6i_video.o sun6i_csi.o
+
+obj-$(CONFIG_VIDEO_SUN6I_CSI) += sun6i-csi.o
diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c 
b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
new file mode 100644
index 000..9c341f0
--- /dev/null
+++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
@@ -0,0 +1,908 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2011-2018 Magewell Electronics Co., Ltd. (Nanjing)
+ * All rights reserved.
+ * Author: Yong Deng 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "sun6i_csi.h"
+#include "sun6i_csi_reg.h"
+
+#define MODULE_NAME"sun6i-csi"
+
+struct sun6i_csi_dev {
+   struct sun6i_csicsi;
+   struct device