Re: PCIe host controller behind IOMMU on ARM

2015-11-11 Thread liviu.du...@arm.com
On Mon, Nov 09, 2015 at 12:32:13PM +, Phil Edworthy wrote:
> Hi Liviu, Will,
> 
> On 04 November 2015 15:19, Phil wrote:
> > On 04 November 2015 15:02, Liviu wrote:
> > > On Wed, Nov 04, 2015 at 02:48:38PM +, Phil Edworthy wrote:
> > > > Hi Liviu,
> > > >
> > > > On 04 November 2015 14:24, Liviu wrote:
> > > > > On Wed, Nov 04, 2015 at 01:57:48PM +, Phil Edworthy wrote:
> > > > > > Hi,
> > > > > >
> > > > > > I am trying to hook up a PCIe host controller that sits behind an 
> > > > > > IOMMU,
> > > > > > but having some problems.
> > > > > >
> > > > > > I'm using the pcie-rcar PCIe host controller and it works fine 
> > > > > > without
> > > > > > the IOMMU, and I can attach the IOMMU to the controller such that 
> > > > > > any
> > > calls
> > > > > > to dma_alloc_coherent made by the controller driver uses the
> > iommu_ops
> > > > > > version of dma_ops.
> > > > > >
> > > > > > However, I can't see how to make the endpoints to utilise the 
> > > > > > dma_ops
> > that
> > > > > > the controller uses. Shouldn't the endpoints inherit the dma_ops 
> > > > > > from the
> > > > > > controller?
> > > > >
> > > > > No, not directly.
> > > > >
> > > > > > Any pointers for this?
> > > > >
> > > > > You need to understand the process through which a driver for endpoint
> > get
> > > > > an address to be passed down to the device. Have a look at
> > > > > Documentation/DMA-API-HOWTO.txt, there is a nice explanation there.
> > > > > (Hint: EP driver needs to call dma_map_single).
> > > > >
> > > > > Also, you need to make sure that the bus address that ends up being 
> > > > > set
> > into
> > > > > the endpoint gets translated correctly by the host controller into an 
> > > > > address
> > > > > that the IOMMU can then translate into physical address.
> > > > Sure, though since this is bog standard Intel PCIe ethernet card which 
> > > > works
> > > > fine when the IOMMU is effectively unused, I don’t think there is a 
> > > > problem
> > > > with that.
> > > >
> > > > The driver for the PCIe controller sets up the IOMMU mapping ok when I
> > > > do a test call to dma_alloc_coherent() in the controller's driver. i.e. 
> > > > when I
> > > > do this, it ends up in arm_iommu_alloc_attrs(), which calls
> > > > __iommu_alloc_buffer() and __alloc_iova().
> > > >
> > > > When an endpoint driver allocates and maps a dma coherent buffer it
> > > > also needs to end up in arm_iommu_alloc_attrs(), but it doesn't.
> > >
> > > Why do you think that? Remember that the only thing attached to the IOMMU
> > is
> > > the
> > > host controller. The endpoint is on the PCIe bus, which gets a different
> > > translation
> > > that the IOMMU knows nothing about. If it helps you to visualise it 
> > > better, think
> > > of the host controller as another IOMMU device. It's the ops of the host
> > > controller
> > > that should be invoked, not the IOMMU's.
> > Ok, that makes sense. I'll have a think and poke it a bit more...

Hi Phil,

Not trying to ignore your email, but I thought this is more in Will's backyard.

> Somewhat related to this, since our PCIe controller HW is limited to
> 32-bit AXI address range, before trying to hook up the IOMMU I have
> tried to limit the dma_mask for PCI cards to DMA_BIT_MASK(32). The
> reason being that Linux uses a 1 to 1 mapping between PCI addresses
> and cpu (phys) addresses when there isn't an IOMMU involved, so I
> think that we need to limit the PCI address space used.

I think you're mixing things a bit or not explaining them very well. Having the
PCIe controller limited to 32-bit AXI does not mean that the PCIe bus cannot
carry 64-bit addresses. It depends on how they get translated by the host bridge
or its associated ATS block. I can't see why you can't have a setup where
the CPU addresses are 32-bit but the PCIe bus addresses are all 64-bit.
You just have to be careful on how you setup your mem64 ranges so that they 
don't
overlap with the 32-bit ranges when translated.

And no, you should not limit at the card driver the DMA_BIT_MASK() unless the
card is not capable of supporting more than 32-bit addresses.

> 
> Since pci_setup_device() sets up dma_mask, I added a bus notifier in the
> PCIe controller driver so I can change the mask, if needed, on the
> BUS_NOTIFY_BOUND_DRIVER action.
> However, I think there is the potential for card drivers to allocate and
> map buffers before the bus notifier get called. Additionally, I've seen
> drivers change their behaviour based on the success or failure of
> dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)), so the
> driver could, theoretically at least, operate in a way that is not
> compatible with a more restricted dma_mask (though I can't think
> of any way this would not work with hardware I've seen).
> 
> So, I think that using a bus notifier is the wrong way to go, but I don’t
> know what other options I have. Any suggestions?

I would first have a look at how the PCIe bus addresses are translated by the
host controller. 

Bes

Re: PCIe host controller behind IOMMU on ARM

2015-11-12 Thread liviu.du...@arm.com
On Thu, Nov 12, 2015 at 09:26:33AM +, Phil Edworthy wrote:
> Hi Liviu, Arnd,
> 
> On 11 November 2015 18:25, LIviu wrote:
> > On Mon, Nov 09, 2015 at 12:32:13PM +, Phil Edworthy wrote:
> > > Hi Liviu, Will,
> > >
> > > On 04 November 2015 15:19, Phil wrote:
> > > > On 04 November 2015 15:02, Liviu wrote:
> > > > > On Wed, Nov 04, 2015 at 02:48:38PM +, Phil Edworthy wrote:
> > > > > > Hi Liviu,
> > > > > >
> > > > > > On 04 November 2015 14:24, Liviu wrote:
> > > > > > > On Wed, Nov 04, 2015 at 01:57:48PM +, Phil Edworthy wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > I am trying to hook up a PCIe host controller that sits behind 
> > > > > > > > an
> > IOMMU,
> > > > > > > > but having some problems.
> > > > > > > >
> > > > > > > > I'm using the pcie-rcar PCIe host controller and it works fine 
> > > > > > > > without
> > > > > > > > the IOMMU, and I can attach the IOMMU to the controller such 
> > > > > > > > that
> > any
> > > > > calls
> > > > > > > > to dma_alloc_coherent made by the controller driver uses the
> > > > iommu_ops
> > > > > > > > version of dma_ops.
> > > > > > > >
> > > > > > > > However, I can't see how to make the endpoints to utilise the
> > dma_ops
> > > > that
> > > > > > > > the controller uses. Shouldn't the endpoints inherit the 
> > > > > > > > dma_ops from
> > the
> > > > > > > > controller?
> > > > > > >
> > > > > > > No, not directly.
> > > > > > >
> > > > > > > > Any pointers for this?
> > > > > > >
> > > > > > > You need to understand the process through which a driver for
> > endpoint
> > > > get
> > > > > > > an address to be passed down to the device. Have a look at
> > > > > > > Documentation/DMA-API-HOWTO.txt, there is a nice explanation 
> > > > > > > there.
> > > > > > > (Hint: EP driver needs to call dma_map_single).
> > > > > > >
> > > > > > > Also, you need to make sure that the bus address that ends up 
> > > > > > > being set
> > > > into
> > > > > > > the endpoint gets translated correctly by the host controller 
> > > > > > > into an
> > address
> > > > > > > that the IOMMU can then translate into physical address.
> > > > > > Sure, though since this is bog standard Intel PCIe ethernet card 
> > > > > > which
> > works
> > > > > > fine when the IOMMU is effectively unused, I don’t think there is a
> > problem
> > > > > > with that.
> > > > > >
> > > > > > The driver for the PCIe controller sets up the IOMMU mapping ok 
> > > > > > when I
> > > > > > do a test call to dma_alloc_coherent() in the controller's driver. 
> > > > > > i.e. when I
> > > > > > do this, it ends up in arm_iommu_alloc_attrs(), which calls
> > > > > > __iommu_alloc_buffer() and __alloc_iova().
> > > > > >
> > > > > > When an endpoint driver allocates and maps a dma coherent buffer it
> > > > > > also needs to end up in arm_iommu_alloc_attrs(), but it doesn't.
> > > > >
> > > > > Why do you think that? Remember that the only thing attached to the
> > IOMMU
> > > > is
> > > > > the
> > > > > host controller. The endpoint is on the PCIe bus, which gets a 
> > > > > different
> > > > > translation
> > > > > that the IOMMU knows nothing about. If it helps you to visualise it 
> > > > > better,
> > think
> > > > > of the host controller as another IOMMU device. It's the ops of the 
> > > > > host
> > > > > controller
> > > > > that should be invoked, not the IOMMU's.
> > > > Ok, that makes sense. I'll have a think and poke it a bit more...
> > 
> > Hi Phil,
> > 
> > Not trying to ignore your email, but I thought this is more in Will's 
> > backyard.
> > 
> > > Somewhat related to this, since our PCIe controller HW is limited to
> > > 32-bit AXI address range, before trying to hook up the IOMMU I have
> > > tried to limit the dma_mask for PCI cards to DMA_BIT_MASK(32). The
> > > reason being that Linux uses a 1 to 1 mapping between PCI addresses
> > > and cpu (phys) addresses when there isn't an IOMMU involved, so I
> > > think that we need to limit the PCI address space used.
> > 
> > I think you're mixing things a bit or not explaining them very well. Having 
> > the
> > PCIe controller limited to 32-bit AXI does not mean that the PCIe bus cannot
> > carry 64-bit addresses. It depends on how they get translated by the host 
> > bridge
> > or its associated ATS block. I can't see why you can't have a setup where
> > the CPU addresses are 32-bit but the PCIe bus addresses are all 64-bit.
> > You just have to be careful on how you setup your mem64 ranges so that they
> > don't
> > overlap with the 32-bit ranges when translated.
> From a HW point of view I agree that we can setup the PCI host bridge such 
> that
> it uses 64-bit PCI address, with 32-bit cpu addresses. Though in practice 
> doesn't
> this mean that the dma ops used by card drivers has to be provided by our PCI
> host bridge driver so we can apply the translation to those PCI addresses?

I thought all addresses that are set into the cards go through
pcibios_resource_to_bus() which give you the PCI address 

Re: PCIe host controller behind IOMMU on ARM

2015-11-04 Thread liviu.du...@arm.com
On Wed, Nov 04, 2015 at 01:57:48PM +, Phil Edworthy wrote:
> Hi,
> 
> I am trying to hook up a PCIe host controller that sits behind an IOMMU,
> but having some problems.
> 
> I'm using the pcie-rcar PCIe host controller and it works fine without
> the IOMMU, and I can attach the IOMMU to the controller such that any calls
> to dma_alloc_coherent made by the controller driver uses the iommu_ops
> version of dma_ops.
> 
> However, I can't see how to make the endpoints to utilise the dma_ops that
> the controller uses. Shouldn't the endpoints inherit the dma_ops from the
> controller? 

No, not directly.

> Any pointers for this?

You need to understand the process through which a driver for endpoint get
an address to be passed down to the device. Have a look at
Documentation/DMA-API-HOWTO.txt, there is a nice explanation there.
(Hint: EP driver needs to call dma_map_single).

Also, you need to make sure that the bus address that ends up being set into
the endpoint gets translated correctly by the host controller into an address
that the IOMMU can then translate into physical address.

Best regards,
Liviu


> 
> Thanks
> Phil
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: PCIe host controller behind IOMMU on ARM

2015-11-04 Thread liviu.du...@arm.com
On Wed, Nov 04, 2015 at 02:48:38PM +, Phil Edworthy wrote:
> Hi Liviu,
> 
> On 04 November 2015 14:24, Liviu wrote:
> > On Wed, Nov 04, 2015 at 01:57:48PM +, Phil Edworthy wrote:
> > > Hi,
> > >
> > > I am trying to hook up a PCIe host controller that sits behind an IOMMU,
> > > but having some problems.
> > >
> > > I'm using the pcie-rcar PCIe host controller and it works fine without
> > > the IOMMU, and I can attach the IOMMU to the controller such that any 
> > > calls
> > > to dma_alloc_coherent made by the controller driver uses the iommu_ops
> > > version of dma_ops.
> > >
> > > However, I can't see how to make the endpoints to utilise the dma_ops that
> > > the controller uses. Shouldn't the endpoints inherit the dma_ops from the
> > > controller?
> > 
> > No, not directly.
> > 
> > > Any pointers for this?
> > 
> > You need to understand the process through which a driver for endpoint get
> > an address to be passed down to the device. Have a look at
> > Documentation/DMA-API-HOWTO.txt, there is a nice explanation there.
> > (Hint: EP driver needs to call dma_map_single).
> > 
> > Also, you need to make sure that the bus address that ends up being set into
> > the endpoint gets translated correctly by the host controller into an 
> > address
> > that the IOMMU can then translate into physical address.
> Sure, though since this is bog standard Intel PCIe ethernet card which works
> fine when the IOMMU is effectively unused, I don’t think there is a problem
> with that.
> 
> The driver for the PCIe controller sets up the IOMMU mapping ok when I
> do a test call to dma_alloc_coherent() in the controller's driver. i.e. when I
> do this, it ends up in arm_iommu_alloc_attrs(), which calls
> __iommu_alloc_buffer() and __alloc_iova().
> 
> When an endpoint driver allocates and maps a dma coherent buffer it
> also needs to end up in arm_iommu_alloc_attrs(), but it doesn't.

Why do you think that? Remember that the only thing attached to the IOMMU is the
host controller. The endpoint is on the PCIe bus, which gets a different 
translation
that the IOMMU knows nothing about. If it helps you to visualise it better, 
think
of the host controller as another IOMMU device. It's the ops of the host 
controller
that should be invoked, not the IOMMU's.

Best regards,
Liviu

> 
> Thanks
> Phil

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Purpose of pci_remap_iospace

2016-07-13 Thread liviu.du...@arm.com
On Wed, Jul 13, 2016 at 08:11:56AM +, Bharat Kumar Gogada wrote:
> > Subject: Re: Purpose of pci_remap_iospace
> >
> > On Tuesday, July 12, 2016 6:57:10 AM CEST Bharat Kumar Gogada wrote:
> > > Hi,
> > >
> > > I have a query.
> > >
> > > Can any once explain the purpose of pci_remap_iospace function in root
> > port driver.
> > >
> > > What is its dependency with architecture ?
> > >
> > > Here is my understanding, the above API takes PCIe IO resource and its
> > > to be mapped CPU address from ranges property and remaps into virtual
> > address space.
> > >
> > > So my question is who uses this virtual addresses ?
> >
> > The inb()/outb() functions declared in asm/io.h
> >
> > > When End Point requests for IO BARs doesn't it get from the above
> > > resource range (first parameter of API) and do ioremap to access this
> > > region ?
> >
> > Device drivers generally do not ioremap() the I/O BARs but they use
> > inb()/outb() directly. They can also call pci_iomap() and do
> > ioread8()/iowrite8() on the pointer returned from that function, but
> > generally the call to pci_iomap() then returns a pointer into the virtual
> > address that is already mapped.
> >
> > > But why root complex driver is mapping this address region ?
> >
> > The PCI core does not know that the I/O space is memory mapped.
> > On x86 and a few others, I/O space is not memory mapped but requires the
> > use of special CPU instructions.
> >
> Thanks Arnd.
> 
> I'm facing issue in testing IO bars on our SoC.
> 
> I added following ranges in our device tree :
> ranges = <0x0100 0x 0x 0x 0xe000 0 0x0010 
>   //io
>  0x0200 0x 0xe010 0x 0xe010 0 
> 0x0ef0>;   //non prefetchabe memory
> 
> And I'm using above API to map the res and cpu physical address in my driver.
> 
> Kernel Boot log:
> [2.345294] nwl-pcie fd0e.pcie: Link is UP
> [2.345339] PCI host bridge /amba/pcie@fd0e ranges:
> [2.345356]   No bus range found for /amba/pcie@fd0e, using [bus 00-ff]
> [2.345382]IO 0xe000..0xe00f -> 0x
> [2.345401]   MEM 0xe010..0xeeff -> 0xe010
> [2.345498] nwl-pcie fd0e.pcie: PCI host bridge to bus :00
> [2.345517] pci_bus :00: root bus resource [bus 00-ff]
> [2.345533] pci_bus :00: root bus resource [io  0x-0xf]
> [2.345550] pci_bus :00: root bus resource [mem 0xe010-0xeeff]
> [2.345770] pci :00:00.0: cannot attach to SMMU, is it on the same bus?
> [2.345786] iommu: Adding device :00:00.0 to group 1
> [2.346142] pci :01:00.0: cannot attach to SMMU, is it on the same bus?
> [2.346158] iommu: Adding device :01:00.0 to group 1
> [2.346213] pci :00:00.0: BAR 8: assigned [mem 0xe010-0xe02f]
> [2.346234] pci :01:00.0: BAR 0: assigned [mem 0xe010-0xe01f 
> 64bit]
> [2.346268] pci :01:00.0: BAR 2: assigned [mem 0xe020-0xe02f 
> 64bit]
> [2.346300] pci :01:00.0: BAR 4: no space for [io  size 0x0040]

Can you try to print the value of ret in pci_assign_resource() when it is 
printing the above message?

I would tr debugging that function and the __pci_assign_resource() function to 
figure out
where it fails. Maybe due to IO region being 1MB?

Best regards,
Liviu

> [2.346316] pci :01:00.0: BAR 4: failed to assign [io  size 0x0040]
> [2.346333] pci :00:00.0: PCI bridge to [bus 01-0c]
> [2.346350] pci :00:00.0:   bridge window [mem 0xe010-0xe02f]
> 
> IO assignment fails.
> 
> On End Point:
> 01:00.0 Memory controller: Xilinx Corporation Device a024
> Subsystem: Xilinx Corporation Device 0007
> Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping- SERR- FastB2B- DisINTx-
> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- 
> SERR-  Interrupt: pin A routed to IRQ 224
> Region 0: Memory at e010 (64-bit, non-prefetchable) [disabled] 
> [size=1M]
> Region 2: Memory at e020 (64-bit, non-prefetchable) [disabled] 
> [size=1M]
> Region 4: I/O ports at  [disabled]
> 
> When I tested on x86 machine the same End Point I/O address is assigned, but 
> it is a IO port mapped address.
> 
> So my doubt is why the memory mapped IO addresses are not assigned to EP on 
> SoC ?
> 
> Do we need to have port mapped addresses on SoC also for PCI IO bars ?
> 
> Please let me know If I'm doing something wrong or missing something.
> 
> Thanks & Regards,
> Bharat
> 
> 
> 
> This email and any attachments are intended for the sole use of the named 
> recipient(s) and contain(s) confidential information that may be proprietary, 
> privileged or copyrighted under applicable law. If you are not the intended 
> recipient, do not read, copy, or forward this email message or any 
> attachments. Delete this email message and any attachments immediately.
> 
> --
> To unsubscribe f

Re: Purpose of pci_remap_iospace

2016-07-13 Thread liviu.du...@arm.com
On Wed, Jul 13, 2016 at 05:28:47PM +0200, Arnd Bergmann wrote:
> On Wednesday, July 13, 2016 3:16:21 PM CEST Bharat Kumar Gogada wrote:
> > 
> > > This has neither the PCI memory nor the I/O resource, it looks like you 
> > > never
> > > call pci_add_resource_offset() to start with, or maybe it fails for some
> > > reason.
> > 
> > I see that above API is used in ARM drivers, do we need to do it in ARM64 
> > also ?
> > 
> 
> Yes, all architectures need it.

of_pci_get_host_bridge_resources() calls it for him.

Liviu

> 
>   Arnd
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯


Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

2016-11-09 Thread liviu.du...@arm.com
On Wed, Nov 09, 2016 at 04:16:17PM +, Gabriele Paoloni wrote:
> Hi Liviu
> 
> Thanks for reviewing
> 

[removed some irrelevant part of discussion, avoid crazy formatting]

> > > +/**
> > > + * addr_is_indirect_io - check whether the input taddr is for
> > indirectIO.
> > > + * @taddr: the io address to be checked.
> > > + *
> > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > + */
> > > +int addr_is_indirect_io(u64 taddr)
> > > +{
> > > + if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <
> > taddr)
> > 
> > start >= taddr ?
> 
> Nope... if  (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
> then taddr is outside the range [start; end] and will return 0; otherwise
> it will return 1...

Oops, sorry, did not pay attention to the returned value. The check is
correct as it is, no need to change then.

> 
> > 
> > > + return 0;
> > > +
> > > + return 1;
> > > +}
> > >
> > >  BUILD_EXTIO(b, u8)
> > >
> > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > index 02b2903..cc2a05d 100644
> > > --- a/drivers/of/address.c
> > > +++ b/drivers/of/address.c
> > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > device_node *np)
> > >   return false;
> > >  }
> > >
> > > +
> > > +/*
> > > + * of_isa_indirect_io - get the IO address from some isa reg
> > property value.
> > > + *   For some isa/lpc devices, no ranges property in ancestor node.
> > > + *   The device addresses are described directly in their regs
> > property.
> > > + *   This fixup function will be called to get the IO address of
> > isa/lpc
> > > + *   devices when the normal of_translation failed.
> > > + *
> > > + * @parent:  points to the parent dts node;
> > > + * @bus: points to the of_bus which can be used to parse
> > address;
> > > + * @addr:the address from reg property;
> > > + * @na:  the address cell counter of @addr;
> > > + * @presult: store the address paresed from @addr;
> > > + *
> > > + * return 1 when successfully get the I/O address;
> > > + * 0 will return for some failures.
> > 
> > Bah, you are returning a signed int, why 0 for failure? Return a
> > negative value with
> > error codes. Otherwise change the return value into a bool.
> 
> Yes we'll move to bool
> 
> > 
> > > + */
> > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > + struct of_bus *bus, __be32 *addr,
> > > + int na, u64 *presult)
> > > +{
> > > + unsigned int flags;
> > > + unsigned int rlen;
> > > +
> > > + /* whether support indirectIO */
> > > + if (!indirect_io_enabled())
> > > + return 0;
> > > +
> > > + if (!of_bus_isa_match(parent))
> > > + return 0;
> > > +
> > > + flags = bus->get_flags(addr);
> > > + if (!(flags & IORESOURCE_IO))
> > > + return 0;
> > > +
> > > + /* there is ranges property, apply the normal translation
> > directly. */
> > 
> > s/there is ranges/if we have a 'ranges'/
> 
> Thanks for spotting this
> 
> > 
> > > + if (of_get_property(parent, "ranges", &rlen))
> > > + return 0;
> > > +
> > > + *presult = of_read_number(addr + 1, na - 1);
> > > + /* this fixup is only valid for specific I/O range. */
> > > + return addr_is_indirect_io(*presult);
> > > +}
> > > +
> > >  static int of_translate_one(struct device_node *parent, struct
> > of_bus *bus,
> > >   struct of_bus *pbus, __be32 *addr,
> > >   int na, int ns, int pna, const char *rprop)
> > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> > device_node *dev,
> > >   result = of_read_number(addr, na);
> > >   break;
> > >   }
> > > + /*
> > > +  * For indirectIO device which has no ranges property, get
> > > +  * the address from reg directly.
> > > +  */
> > > + if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
> > > + pr_debug("isa indirectIO matched(%s)..addr =
> > 0x%llx\n",
> > > + of_node_full_name(dev), result);
> > > + break;
> > > + }
> > >
> > >   /* Get new parent bus and counts */
> > >   pbus = of_match_bus(parent);
> > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> > device_node *dev,
> > >   if (taddr == OF_BAD_ADDR)
> > >   return -EINVAL;
> > >   memset(r, 0, sizeof(struct resource));
> > > - if (flags & IORESOURCE_IO) {
> > > + if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > >   unsigned long port;
> > > +
> > >   port = pci_address_to_pio(taddr);
> > >   if (port == (unsigned long)-1)
> > >   return -EINVAL;
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index ba34907..1a08511 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t
> > a

Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

2016-11-14 Thread liviu.du...@arm.com
On Mon, Nov 14, 2016 at 08:26:42AM +, Gabriele Paoloni wrote:
> Hi Liviu
> 

[snip]

> > > >
> > > > Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and
> > you
> > > > actually need another variable for "reserving" an area in the I/O
> > space
> > > > that can be used for physical addresses rather than I/O tokens.
> > > >
> > > > The one good example for using PCIBIOS_MIN_IO is when your
> > > > platform/architecture
> > > > does not support legacy ISA operations *at all*. In that case
> > someone
> > > > sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O
> > range
> > > > so that it doesn't get used. With Zhichang's patch you now start
> > > > forcing
> > > > those platforms to have a valid address below PCIBIOS_MIN_IO.
> > >
> > > But if PCIBIOS_MIN_IO is 0 then it means that all I/O space is to be
> > used
> > > by PCI controllers only...
> > 
> > Nope, that is not what it means. It means that PCI devices can see I/O
> > addresses
> > on the bus that start from 0. There never was any usage for non-PCI
> > controllers
> 
> So I am a bit confused...
> From http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> It seems that ISA buses operate on cpu I/O address range [0, 0xFFF].
> I thought that was the reason why for most architectures we have
> PCIBIOS_MIN_IO equal to 0x1000 (so I thought that ISA controllers
> usually use [0, PCIBIOS_MIN_IO - 1] )

First of all, cpu I/O addresses is an x86-ism. ARM architectures and others
 have no separate address space for I/O, it is all merged into one unified
address space. So, on arm/arm64 for example, PCIBIOS_MIN_IO = 0 could mean
that we don't care about ISA I/O because the platform does not support having
an ISA bus (e.g.).


> 
> For those architectures whose PCIBIOS_MIN_IO != 0x1000 probably
> they are not fully compliant or they cannot fully support an ISA
> controller...?

Exactly. Not fully compliant is a bit strong, as ISA is a legacy feature and
when it comes to PCI-e you are allowed to ignore it. Having PCIBIOS_MIN_IO != 
0x1000
is a way to signal that you don't fully support ISA.

> 
> As said before this series forbid IO tokens to be in [0, PCIBIOS_MIN_IO)
> to allow special ISA controllers to use that range with special
> accessors.
> Having a variable threshold would make life much more difficult
> as there would be a probe dependency between the PCI controller and
> the special ISA one (PCI to wait for the special ISA device to be
> probed and set the right threshold value from DT or ACPI table).
> 
> Instead using PCIBIOS_MIN_IO is easier and should not impose much
> constraint as [PCIBIOS_MIN_IO, IO_SPACE_LIMIT] is available to
> the PCI controller for I/O tokens...

What I am suggesting is to leave PCIBIOS_MIN_IO alone which still reserves
space for ISA controller and add a PCIBIOS_MIN_DIRECT_IO that will reserve
space for your direct address I/O on top of PCIBIOS_MIN_IO.

Best regards,
Liviu

> 
> Thanks
> 
> Gab
> 
> > when PCIBIOS_MIN_IO != 0. That is what Zhichang is trying to do now and
> > what
> > I think is not the right thing (and not enough anyway).
> > 
> > > so if you have a special bus device using
> > > an I/O range in this case should be a PCI controller...
> > 
> > That has always been the case. It is this series that wants to
> > introduce the
> > new meaning.
> > 
> > > i.e. I would
> > > expect it to fall back into the case of I/O tokens redirection rather
> > than
> > > physical addresses redirection (as mentioned below from my previous
> > reply).
> > > What do you think?
> > 
> > I think you have looked too much at the code *with* Zhichang's patches
> > applied.
> > Take a step back and look at how PCIBIOS_MIN_IO is used now, before you
> > apply
> > the patches. It is all about PCI addresses and there is no notion of
> > non-PCI
> > busses using PCI framework. Only platforms and architectures that try
> > to work
> > around some legacy standards (ISA) or HW restrictions.
> > 
> > Best regards,
> > Liviu
> > 
> > >
> > > Thanks
> > >
> > > Gab
> > >
> > >
> > > >
> > > > For the general case you also have to bear in mind that
> > PCIBIOS_MIN_IO
> > > > could
> > > > be zero. In that case, what is your "forbidden" range? [0, 0) ? So
> > it
> > > > makes
> > > > sense to add a new #define that should only be defined by those
> > > > architectures/
> > > > platforms that want to reserve on top of PCIBIOS_MIN_IO another
> > region
> > > > where I/O tokens can't be generated for.
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > >
> > > > > >
> > > > > > > > Your current version has
> > > > > > > >
> > > > > > > > if (arm64_extio_ops->pfout)
> > > > \
> > > > > > > > arm64_extio_ops->pfout(arm64_extio_ops-
> > > > >devpara,\
> > > > > > > >addr, value, sizeof(type));
> > > > \
> > > > > > > >
> > > > > > > > Instead, just subtract the start of the range from the
> > logical
> > > > > > > > port number to transform it back into a bus-local 

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

2016-11-11 Thread liviu.du...@arm.com
On Thu, Nov 10, 2016 at 04:06:40PM +, Gabriele Paoloni wrote:
> Hi Liviu
> 
> > -Original Message-
> > From: liviu.du...@arm.com [mailto:liviu.du...@arm.com]
> > Sent: 09 November 2016 16:51
> > To: Gabriele Paoloni
> > Cc: Yuanzhichang; catalin.mari...@arm.com; will.dea...@arm.com;
> > robh...@kernel.org; bhelg...@google.com; mark.rutl...@arm.com;
> > o...@lixom.net; a...@arndb.de; linux-arm-ker...@lists.infradead.org;
> > lorenzo.pieral...@arm.com; linux-kernel@vger.kernel.org; Linuxarm;
> > devicet...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> > ser...@vger.kernel.org; miny...@acm.org; b...@kernel.crashing.org;
> > zourongr...@gmail.com; John Garry; zhichang.yua...@gmail.com;
> > kant...@163.com; xuwei (O)
> > Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> > special ISA
> > 
> > On Wed, Nov 09, 2016 at 04:16:17PM +, Gabriele Paoloni wrote:
> > > Hi Liviu
> > >
> > > Thanks for reviewing
> > >
> > 
> > [removed some irrelevant part of discussion, avoid crazy formatting]
> > 
> > > > > +/**
> > > > > + * addr_is_indirect_io - check whether the input taddr is for
> > > > indirectIO.
> > > > > + * @taddr: the io address to be checked.
> > > > > + *
> > > > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > > > + */
> > > > > +int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > + if (arm64_extio_ops->start > taddr || arm64_extio_ops->end
> > <
> > > > taddr)
> > > >
> > > > start >= taddr ?
> > >
> > > Nope... if  (taddr < arm64_extio_ops->start || taddr >
> > arm64_extio_ops->end)
> > > then taddr is outside the range [start; end] and will return 0;
> > otherwise
> > > it will return 1...
> > 
> > Oops, sorry, did not pay attention to the returned value. The check is
> > correct as it is, no need to change then.
> > 
> > >
> > > >
> > > > > + return 0;
> > > > > +
> > > > > + return 1;
> > > > > +}
> > > > >
> > > > >  BUILD_EXTIO(b, u8)
> > > > >
> > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > > > index 02b2903..cc2a05d 100644
> > > > > --- a/drivers/of/address.c
> > > > > +++ b/drivers/of/address.c
> > > > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > > > device_node *np)
> > > > >   return false;
> > > > >  }
> > > > >
> > > > > +
> > > > > +/*
> > > > > + * of_isa_indirect_io - get the IO address from some isa reg
> > > > property value.
> > > > > + *   For some isa/lpc devices, no ranges property in ancestor
> > node.
> > > > > + *   The device addresses are described directly in their regs
> > > > property.
> > > > > + *   This fixup function will be called to get the IO address of
> > > > isa/lpc
> > > > > + *   devices when the normal of_translation failed.
> > > > > + *
> > > > > + * @parent:  points to the parent dts node;
> > > > > + * @bus: points to the of_bus which can be used to parse
> > > > address;
> > > > > + * @addr:the address from reg property;
> > > > > + * @na:  the address cell counter of @addr;
> > > > > + * @presult: store the address paresed from @addr;
> > > > > + *
> > > > > + * return 1 when successfully get the I/O address;
> > > > > + * 0 will return for some failures.
> > > >
> > > > Bah, you are returning a signed int, why 0 for failure? Return a
> > > > negative value with
> > > > error codes. Otherwise change the return value into a bool.
> > >
> > > Yes we'll move to bool
> > >
> > > >
> > > > > + */
> > > > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > > > + struct of_bus *bus, __be32 *addr,
> > > > > + int na, u64 *presult)
> > > > > +{
> > > > > + unsigned int flags;
> > > > > + unsigned int rlen;
> > > > > +
&

Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

2016-11-11 Thread liviu.du...@arm.com
Hi Arnd,

On Thu, Nov 10, 2016 at 05:07:21PM +0100, Arnd Bergmann wrote:
> On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
> > 
> > Where should we get the range from? For LPC we know that it is going
> > Work on anything that is not used by PCI I/O space, and this is 
> > why we use [0, PCIBIOS_MIN_IO]
> 
> It should be allocated the same way we allocate PCI config space
> segments. This is currently done with the io_range list in
> drivers/pci/pci.c, which isn't perfect but could be extended
> if necessary. Based on what others commented here, I'd rather
> make the differences between ISA/LPC and PCI I/O ranges smaller
> than larger.
> 
> > > Your current version has
> > > 
> > > if (arm64_extio_ops->pfout) \
> > > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > >addr, value, sizeof(type)); \
> > > 
> > > Instead, just subtract the start of the range from the logical
> > > port number to transform it back into a bus-local port number:
> > 
> > These accessors do not operate on IO tokens:
> > 
> > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
> > addr is not going to be an I/O token; in fact patch 2/3 imposes that
> > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO
> > we have free physical addresses that the accessors can operate on.
> 
> Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
> the logical I/O tokens, the purpose of that macro is really meant
> for allocating PCI I/O port numbers within the address space of
> one bus.
> 
> Note that it's equally likely that whichever next platform needs
> non-mapped I/O access like this actually needs them for PCI I/O space,
> and that will use it on addresses registered to a PCI host bridge.
> 
> If we separate the two steps:
> 
> a) assign a range of logical I/O port numbers to a bus

Except that currently when we add ranges to io_range_list we don't have
a bus number yet, because the parsing happens before the host bridge
has been created. Maybe register_io_range() can take a bus number as an
argument, but I'm not sure how we are going to use that in pci_pio_to_address()
or pci_address_to_pio().

Best regards,
Liviu

> b) register a set of helpers for redirecting logical I/O
>port to a helper function
> 
> then I think the code will get cleaner and more flexible.
> It should actually then be able to replace the powerpc
> specific implementation.
> 
>   Arnd

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯


Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

2016-11-11 Thread liviu.du...@arm.com
On Fri, Nov 11, 2016 at 01:39:35PM +, Gabriele Paoloni wrote:
> Hi Arnd
> 
> > -Original Message-
> > From: Arnd Bergmann [mailto:a...@arndb.de]
> > Sent: 10 November 2016 16:07
> > To: Gabriele Paoloni
> > Cc: linux-arm-ker...@lists.infradead.org; Yuanzhichang;
> > mark.rutl...@arm.com; devicet...@vger.kernel.org;
> > lorenzo.pieral...@arm.com; miny...@acm.org; linux-...@vger.kernel.org;
> > b...@kernel.crashing.org; John Garry; will.dea...@arm.com; linux-
> > ker...@vger.kernel.org; xuwei (O); Linuxarm; zourongr...@gmail.com;
> > robh...@kernel.org; kant...@163.com; linux-ser...@vger.kernel.org;
> > catalin.mari...@arm.com; o...@lixom.net; liviu.du...@arm.com;
> > bhelgaas@googl e.com; zhichang.yua...@gmail.com
> > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> > Hip06
> > 
> > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
> > >
> > > Where should we get the range from? For LPC we know that it is going
> > > Work on anything that is not used by PCI I/O space, and this is
> > > why we use [0, PCIBIOS_MIN_IO]
> > 
> > It should be allocated the same way we allocate PCI config space
> > segments. This is currently done with the io_range list in
> > drivers/pci/pci.c, which isn't perfect but could be extended
> > if necessary. Based on what others commented here, I'd rather
> > make the differences between ISA/LPC and PCI I/O ranges smaller
> > than larger.

Gabriele,

> 
> I am not sure this would make sense...
> 
> IMHO all the mechanism around io_range_list is needed to provide the
> "mapping" between I/O tokens and physical CPU addresses.
> 
> Currently the available tokens range from 0 to IO_SPACE_LIMIT.
> 
> As you know the I/O memory accessors operate on whatever
> __of_address_to_resource sets into the resource (start, end).
> 
> With this special device in place we cannot know if a resource is
> assigned with an I/O token or a physical address, unless we forbid
> the I/O tokens to be in a specific range.
> 
> So this is why we are changing the offsets of all the functions
> handling io_range_list (to make sure that a range is forbidden to
> the tokens and is available to the physical addresses).
> 
> We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO)
> because this is the maximum physical I/O range that a non PCI device
> can operate on and because we believe this does not impose much
> restriction on the available I/O token range; that now is 
> [PCIBIOS_MIN_IO, IO_SPACE_LIMIT].
> So we believe that the chosen forbidden range can accommodate
> any special ISA bus device with no much constraint on the rest
> of I/O tokens...

Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and you
actually need another variable for "reserving" an area in the I/O space
that can be used for physical addresses rather than I/O tokens.

The one good example for using PCIBIOS_MIN_IO is when your platform/architecture
does not support legacy ISA operations *at all*. In that case someone
sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O range
so that it doesn't get used. With Zhichang's patch you now start forcing
those platforms to have a valid address below PCIBIOS_MIN_IO.

For the general case you also have to bear in mind that PCIBIOS_MIN_IO could
be zero. In that case, what is your "forbidden" range? [0, 0) ? So it makes
sense to add a new #define that should only be defined by those architectures/
platforms that want to reserve on top of PCIBIOS_MIN_IO another region
where I/O tokens can't be generated for.

Best regards,
Liviu

> 
> > 
> > > > Your current version has
> > > >
> > > > if (arm64_extio_ops->pfout) \
> > > > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > > >addr, value, sizeof(type)); \
> > > >
> > > > Instead, just subtract the start of the range from the logical
> > > > port number to transform it back into a bus-local port number:
> > >
> > > These accessors do not operate on IO tokens:
> > >
> > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
> > > addr is not going to be an I/O token; in fact patch 2/3 imposes that
> > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to
> > PCIBIOS_MIN_IO
> > > we have free physical addresses that the accessors can operate on.
> > 
> > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
> > the logical I

Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

2016-11-11 Thread liviu.du...@arm.com
On Fri, Nov 11, 2016 at 03:53:53PM +, Gabriele Paoloni wrote:
> Hi Liviu

Hi Gabriele,

> 
> > -Original Message-
> > From: liviu.du...@arm.com [mailto:liviu.du...@arm.com]
> > Sent: 11 November 2016 14:46
> > To: Gabriele Paoloni
> > Cc: Arnd Bergmann; linux-arm-ker...@lists.infradead.org; Yuanzhichang;
> > mark.rutl...@arm.com; devicet...@vger.kernel.org;
> > lorenzo.pieral...@arm.com; miny...@acm.org; linux-...@vger.kernel.org;
> > b...@kernel.crashing.org; John Garry; will.dea...@arm.com; linux-
> > ker...@vger.kernel.org; xuwei (O); Linuxarm; zourongr...@gmail.com;
> > robh...@kernel.org; kant...@163.com; linux-ser...@vger.kernel.org;
> > catalin.mari...@arm.com; o...@lixom.net; bhelgaas@googl e.com;
> > zhichang.yua...@gmail.com
> > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> > Hip06
> > 
> > On Fri, Nov 11, 2016 at 01:39:35PM +, Gabriele Paoloni wrote:
> > > Hi Arnd
> > >
> > > > -Original Message-
> > > > From: Arnd Bergmann [mailto:a...@arndb.de]
> > > > Sent: 10 November 2016 16:07
> > > > To: Gabriele Paoloni
> > > > Cc: linux-arm-ker...@lists.infradead.org; Yuanzhichang;
> > > > mark.rutl...@arm.com; devicet...@vger.kernel.org;
> > > > lorenzo.pieral...@arm.com; miny...@acm.org; linux-
> > p...@vger.kernel.org;
> > > > b...@kernel.crashing.org; John Garry; will.dea...@arm.com; linux-
> > > > ker...@vger.kernel.org; xuwei (O); Linuxarm; zourongr...@gmail.com;
> > > > robh...@kernel.org; kant...@163.com; linux-ser...@vger.kernel.org;
> > > > catalin.mari...@arm.com; o...@lixom.net; liviu.du...@arm.com;
> > > > bhelgaas@googl e.com; zhichang.yua...@gmail.com
> > > > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> > > > Hip06
> > > >
> > > > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni
> > wrote:
> > > > >
> > > > > Where should we get the range from? For LPC we know that it is
> > going
> > > > > Work on anything that is not used by PCI I/O space, and this is
> > > > > why we use [0, PCIBIOS_MIN_IO]
> > > >
> > > > It should be allocated the same way we allocate PCI config space
> > > > segments. This is currently done with the io_range list in
> > > > drivers/pci/pci.c, which isn't perfect but could be extended
> > > > if necessary. Based on what others commented here, I'd rather
> > > > make the differences between ISA/LPC and PCI I/O ranges smaller
> > > > than larger.
> > 
> > Gabriele,
> > 
> > >
> > > I am not sure this would make sense...
> > >
> > > IMHO all the mechanism around io_range_list is needed to provide the
> > > "mapping" between I/O tokens and physical CPU addresses.
> > >
> > > Currently the available tokens range from 0 to IO_SPACE_LIMIT.
> > >
> > > As you know the I/O memory accessors operate on whatever
> > > __of_address_to_resource sets into the resource (start, end).
> > >
> > > With this special device in place we cannot know if a resource is
> > > assigned with an I/O token or a physical address, unless we forbid
> > > the I/O tokens to be in a specific range.
> > >
> > > So this is why we are changing the offsets of all the functions
> > > handling io_range_list (to make sure that a range is forbidden to
> > > the tokens and is available to the physical addresses).
> > >
> > > We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO)
> > > because this is the maximum physical I/O range that a non PCI device
> > > can operate on and because we believe this does not impose much
> > > restriction on the available I/O token range; that now is
> > > [PCIBIOS_MIN_IO, IO_SPACE_LIMIT].
> > > So we believe that the chosen forbidden range can accommodate
> > > any special ISA bus device with no much constraint on the rest
> > > of I/O tokens...
> > 
> > Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and you
> > actually need another variable for "reserving" an area in the I/O space
> > that can be used for physical addresses rather than I/O tokens.
> > 
> > The one good example for using PCIBIOS_MIN_IO is when your
> > platform/architecture
> > does not support legacy ISA operations *at all*. In that case someone
> > sets the PCIBIOS_MIN_IO to a non-zero value 

Re: [PATCH] drm/arcpgu: Get rid of "encoder-slave" property

2017-03-03 Thread liviu.du...@arm.com
On Fri, Mar 03, 2017 at 05:48:19PM +, Alexey Brodkin wrote:
> Hi Liviu,
> 
> On Fri, 2017-03-03 at 16:28 +, Liviu Dudau wrote:
> > On Fri, Mar 03, 2017 at 06:19:24PM +0300, Alexey Brodkin wrote:
> > > 
> > > - /* find the encoder node and initialize it */
> > > - encoder_node = of_parse_phandle(drm->dev->of_node, "encoder-slave", 0);
> > > - if (encoder_node) {
> > > - ret = arcpgu_drm_hdmi_init(drm, encoder_node);
> > > - of_node_put(encoder_node);
> > > + /* There is only one output port inside each device, find it */
> > > + port = of_graph_get_next_endpoint(pdev->dev.of_node, NULL);
> > > +
> > > + if (port) {
> > > + if (of_device_is_available(port))
> > > + encoder = of_graph_get_remote_port_parent(port);
> > > + of_node_put(port);
> > > + }
> > 
> > You must've been looking at some old version. Current version in -next uses
> > of_graph_get_remote_node() to replace all those lines you have added (see 
> > Rob
> > Herring's series to introduce of_graph_get_remote_node() function)
> 
> Hm, I'm not on Linus' master tree [1] and so I thought I was quite up to date 
> :)
> Still I made a check of linux-next and don't see any changes in
> "drivers/gpu/drm/arm" compared to Linus' tree.
> 
> [1] 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/arm?id=e4563f6ba71792c77aeccb2092cc23149b44e642
> [2] 
> http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/arm?id=e4563f6ba71792c77aeccb2092cc23149b44e642
> 
> Could you please clarify which exact tree did you mean?

Sorry, I thought the series got pulled by one of the DRM trees, but it looks 
like
I was wrong. I was carrying a private copy in my internal tree, waiting for the
moment when it got pulled into drm-next or drm-misc-next.

Rob, do you have an update on your series introducing 
of_graph_get_remote_node() ?

Best regards,
Liviu

> 
> Anyways I just tried to rebase my patch on top of linux-next tree and now
> video output is broken for me - I only see some garbage on top of the screen
> so I'll need to investigate it first before moving forward with stuff you
> proposed :)
> 
> -Alexey

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯


Re: [PATCH] drm/arcpgu: Get rid of "encoder-slave" property

2017-03-29 Thread liviu.du...@arm.com
On Wed, Mar 29, 2017 at 01:34:00PM +, Alexey Brodkin wrote:
> Hi Liviu, Rob,

Hi Alexey,

> 
> On Fri, 2017-03-03 at 18:21 +, liviu.du...@arm.com wrote:
> > On Fri, Mar 03, 2017 at 05:48:19PM +, Alexey Brodkin wrote:
> > > 
> > > Hi Liviu,
> > > 
> > > On Fri, 2017-03-03 at 16:28 +, Liviu Dudau wrote:
> > > > 
> > > > On Fri, Mar 03, 2017 at 06:19:24PM +0300, Alexey Brodkin wrote:
> > > > > 
> > > > > 
> > > > > - /* find the encoder node and initialize it */
> > > > > - encoder_node = of_parse_phandle(drm->dev->of_node, 
> > > > > "encoder-slave", 0);
> > > > > - if (encoder_node) {
> > > > > - ret = arcpgu_drm_hdmi_init(drm, encoder_node);
> > > > > - of_node_put(encoder_node);
> > > > > + /* There is only one output port inside each device, find it */
> > > > > + port = of_graph_get_next_endpoint(pdev->dev.of_node, NULL);
> > > > > +
> > > > > + if (port) {
> > > > > + if (of_device_is_available(port))
> > > > > + encoder = of_graph_get_remote_port_parent(port);
> > > > > + of_node_put(port);
> > > > > + }
> > > > 
> > > > You must've been looking at some old version. Current version in -next 
> > > > uses
> > > > of_graph_get_remote_node() to replace all those lines you have added 
> > > > (see Rob
> > > > Herring's series to introduce of_graph_get_remote_node() function)
> > > 
> > > Hm, I'm not on Linus' master tree [1] and so I thought I was quite up to 
> > > date :)
> > > Still I made a check of linux-next and don't see any changes in
> > > "drivers/gpu/drm/arm" compared to Linus' tree.
> > > 
> > > [1] 
> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__git.kernel.org_cgit_linux_kernel_git_torvalds_linux.git_commit_drivers_gpu_drm_arm-3Fid-3D
> > > e4563f6ba71792c77aeccb2092cc23149b44e642&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=SI66ngnnXy33ncb8m5H4La2
> > > T1SzSEiiP7hc_XsRahEc&s=uaswjVXcjYDrUosOkO_UpTMqJMWTT-LLPrg5JE6-t-8&e= 
> > > [2] 
> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__git.kernel.org_cgit_linux_kernel_git_next_linux-2Dnext.git_commit_drivers_gpu_drm_arm-3Fid
> > > -3De4563f6ba71792c77aeccb2092cc23149b44e642&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=SI66ngnnXy33ncb8m5H4
> > > La2T1SzSEiiP7hc_XsRahEc&s=hl9Y6s3K9LwLL1M2WnL3ODax_V-ZRh8k1iTiyctIqU4&e= 
> > > 
> > > Could you please clarify which exact tree did you mean?
> > 
> > Sorry, I thought the series got pulled by one of the DRM trees, but it 
> > looks like
> > I was wrong. I was carrying a private copy in my internal tree, waiting for 
> > the
> > moment when it got pulled into drm-next or drm-misc-next.
> > 
> > Rob, do you have an update on your series introducing 
> > of_graph_get_remote_node() ?
> 
> For some reason I cannot find any relevant commits in linux-next tree even 
> today.
> Could you please point me to either any random git tree with mentioned above 
> change or
> maybe just mailing list where this patch was sent?

Not sure why Rob hasn't added it to linux-next, but (according to Rob) this is 
the latest version:

https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=of-graph-helpers

Best regards,
Liviu

> 
> I'd like to implement the same fix in ARCPGU and call it a day finally.
> 
> -Alexey

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯