Re: [Qemu-ppc] [PATCH v2 5/6] PPC 85xx: Find PCI host controllers on ppce500 from device tree

[Alexander Graf]

On 06.02.2014, at 23:52, Scott Wood <address@hidden> wrote:

> On Thu, 2014-02-06 at 14:26 +0100, Alexander Graf wrote:
>> On 04.02.2014, at 03:47, Scott Wood <address@hidden> wrote:
>> 
>>> On Fri, 2014-01-31 at 12:16 +0100, Alexander Graf wrote:
>>>> The definition of our ppce500 PV machine is that every address is 
>>>> dynamically
>>>> determined through device tree bindings.
>>>> 
>>>> So don't hardcode where PCI devices are in our physical memory layout but 
>>>> instead
>>>> read them dynamically from the device tree we get passed on boot.
>>> 
>>> Would it be difficult to make the QEMU emulation properly implement
>>> access windows?
>> 
>> What are access windows? You mean BAR region offsets? Not too hard I
>> suppose, but it adds complexity which we were trying to avoid, no?
> 
> It would remove U-Boot complexity (unlike the LAW stuff where we just
> skip it) because we wouldn't need to care about QEMU's default settings.
> It should be easier to do than LAW support, and more useful (e.g. Linux
> currently programs this as well, we just get lucky that it misuses the
> device tree as configuration information).

What complexity would it remove? We would still need to find the configuration 
space for the access windows, configure them and then even go as far as 
modifying the original device tree so we expose the new windows.

> 
>>>> +{
>>>> +  int len;
>>>> +  const uint32_t *prop;
>>>> +
>>>> +  prop = fdt_getprop(fdt, node, property, &len);
>>>> +
>>>> +  if (!prop)
>>>> +          return defval;
>>>> +
>>>> +  if (len < ((off + num) * sizeof(uint32_t)))
>>>> +          panic("Invalid fdt");
>>>> +
>>>> +  prop += off;
>>>> +
>>>> +  switch (num) {
>>>> +  case 1:
>>>> +          return *prop;
>>>> +  case 2:
>>>> +          return *(const uint64_t *)prop;
>>>> +  }
>>>> +
>>> 
>>> What about this function is specific to qemu-ppce500?
>> 
>> Nothing. But the less common code I touch the less I can break.
> 
> The more that line of thought is applied, the uglier the codebase we'll
> end up with. :-)
> 
>> There seems to be an fdt helper framework that's only targeted at a few ARM
>> devices - not sure what to make of that.
> 
> Make use of whatever parts you can, and extend it with the missing bits
> you need.  There's also common/fdt_support.c which is definitely not
> just used by ARM.
> 
>>> +   panic("Invalid cell size");
>>> +}
>>> 
>>> s/cell size/cell count/
>>> 
>>>> +static uint32_t myfdt_one_cell(const void *fdt, int node, const char 
>>>> *property,
>>>> +                         uint32_t defval)
>>>> +{
>>>> +  return myfdt_readcells(fdt, node, property, 1, 0, defval);
>>>> +}
>>> 
>>> This looks a lot like fdt_getprop_u32_default(), except for "int node"
>>> versus path.
>> 
>> Well, I use it inside of a loop where I don't have the path :).
> 
> It still indicates where the proper place for code like this is. :-)
> 
>>>> +static void map_tlb1_io(ulong virt_addr, uint64_t phys_addr, uint64_t 
>>>> size)
>>>> +{
>>>> +  unsigned int max_cam, tsize_mask;
>>>> +  int i;
>>>> +
>>>> +  if ((mfspr(SPRN_MMUCFG) & MMUCFG_MAVN) == MMUCFG_MAVN_V1) {
>>>> +          /* Convert (4^max) kB to (2^max) bytes */
>>>> +          max_cam = ((mfspr(SPRN_TLB1CFG) >> 16) & 0xf) * 2 + 10;
>>>> +          tsize_mask = ~1U;
>>>> +  } else {
>>>> +          /* Convert (2^max) kB to (2^max) bytes */
>>>> +          max_cam = __ilog2(mfspr(SPRN_TLB1PS)) + 10;
>>>> +          tsize_mask = ~0U;
>>>> +  }
>>>> +
>>>> +  for (i = 0; size && i < 8; i++) {
>>>> +          int tlb_index = find_free_tlbcam();
>>>> +          u32 camsize = __ilog2_u64(size) & tsize_mask;
>>>> +          u32 align = __ilog2(virt_addr) & tsize_mask;
>>>> +          unsigned int tlb_size;
>>>> +
>>>> +          if (tlb_index == -1)
>>>> +                  break;
>>>> +
>>>> +          if (align == -2) align = max_cam;
>>> 
>>> -2?  Besides align being unsigned, if this is meant to handle the case
>>> where virt_addr is zero, that's undefined for __ilog2() (don't rely on
>>> it being implemented with cntlzw), and you're not handling the MMUv2
>>> case.
>> 
>> I merely copied this from tlb.c's setup_ddr_tlbs_phys() and adjusted it
>> slightly to let me choose the target virt address.
>> 
>> Would you prefer if I generalize setup_ddr_tlbs_phys() inside tlb.c and
>> export that function there?
> 
> Yes.
> 
> And maybe fix that align == -2 bug while you're at it. :-)

I'll leave that to you :P


Alex