| Message ID | 20161202031641.GD6855@us.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Dec 2, 2016 at 2:16 PM, Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> wrote: > Balbir Singh [bsingharora@gmail.com] wrote: >> >> >> On 01/12/16 11:46, Sukadev Bhattiprolu wrote: >> > Oliver O'Halloran [oohall@gmail.com] wrote: >> >> Half the reason we have the DT is to avoid having to guess about what >> >> hardware is present based on things like the processor version. Is >> >> there a compelling reason not to create a DT node? >> > >> > Not really. I was keeping the code minimal. Will add a patch on top of this >> > set to create a device node for VAS under the xscom node. >> > >> >> That sounds reasonable, but I think it would be nice to discuss the design >> of the DT interface. I assume it won't be too late to revisit these patches then? > > With this change to my patchset: > > diff --git a/core/vas.c b/core/vas.c > index 48d9505..9107e6b 100644 > --- a/core/vas.c > +++ b/core/vas.c > @@ -372,6 +372,8 @@ static int init_one_chip(struct proc_chip *chip) > if (alloc_irqs(chip)) > goto out; > > + (void)dt_new(chip->devnode, "vas"); > + > prlog(PR_INFO, "VAS: Initialized chip %d\n", chip->id); > > return 0; > > I get: > > # ls /proc/device-tree/xscom@603fc00000000/vas/ > name phandle > > # od -xc /proc/device-tree/xscom@603fc00000000/vas/name > 0000000 v a s \0 > 7661 7300 > 0000004 > > # od -xc /proc/device-tree/xscom@603fc00000000/vas/phandle > 0000000 \0 \0 \0 $ > 0000 0024 > 0000004 > > 'vas' should show up under each chip of course. I can fold that change > into one of my patches, but do we need to do anything else at this point? I think it would be nice to add uvw/hvwc addresses/mask? number of windows/ size of windows, etc. I am not a DT expert, but it would be good to get broader opinion Balbir Singh
> > 'vas' should show up under each chip of course. I can fold that change > > into one of my patches, but do we need to do anything else at this point? > I think it would be nice to add uvw/hvwc addresses/mask? number of windows/ By "mask" do you mean the valid_wcm_mask? * * Use the following bitmask to identify valid offsets in the window * context. Refer to Table 3.2 in the VAS workbook for list of valid * offsets. */ uint64_t valid_wcm_mask = 0x00D71FFF13F7D77CULL; If so, we might be able to drop the whole init_windows() function from skiboot, since kernel will initialize the windows before each use. > size of windows, etc. I am not a DT expert, but it would be good to get broader > opinion I am not either and I have a TODO in the kernel patch to visit this later https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-November/150855.html The rest are all constants for Power9 and was not clear that the they need to be pushed through the device tree and parsed in the kernel. If any of them changed for Power10 we would have better understanding of the DT requirements and could deal with it then? Replacing the macros with code to get from DT will not affect the kernel or user API. Stewart, Mikey, please let us know your thoughts on this? Sukadev
On 09/12/16 10:34, Sukadev Bhattiprolu wrote: >>> 'vas' should show up under each chip of course. I can fold that change >>> into one of my patches, but do we need to do anything else at this point? > >> I think it would be nice to add uvw/hvwc addresses/mask? number of windows/ > > By "mask" do you mean the valid_wcm_mask? > > * > * Use the following bitmask to identify valid offsets in the window > * context. Refer to Table 3.2 in the VAS workbook for list of valid > * offsets. > */ > > uint64_t valid_wcm_mask = 0x00D71FFF13F7D77CULL; > > If so, we might be able to drop the whole init_windows() function from > skiboot, since kernel will initialize the windows before each use. > The idea is to have the design of DT is to allow the DT to abstract future hardware changes, such that the kernel will require little or no changes, IMHO. Balbir
diff --git a/core/vas.c b/core/vas.c index 48d9505..9107e6b 100644 --- a/core/vas.c +++ b/core/vas.c @@ -372,6 +372,8 @@ static int init_one_chip(struct proc_chip *chip) if (alloc_irqs(chip)) goto out; + (void)dt_new(chip->devnode, "vas"); + prlog(PR_INFO, "VAS: Initialized chip %d\n", chip->id); return 0;