| Message ID | 1456392739-30336-1-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 25 February 2016 at 09:32, Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > to match structure MemoryRegionOps definition > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > And there seems are some out of date content in this file, like > "cpu_register_io_memory()" mentioned in the last line, which is removed several > years ago in commit 97161e177. I am sure how to modify it, So I take the > liberty to CC the guys in the cc-list(get list from get_maintainer.pl).k The part that says # - .old_mmio can be used to ease porting from code using # cpu_register_io_memory(). It should not be used in new code. could be changed to say "has been used to ease porting from code that used to use cpu_register_io_memory()" if you like. The information here is not wrong, really, just the tense. (There are just 32 uses of old_mmio left in the codebase, which hopefully we may eventually get rid of.) > docs/memory.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/docs/memory.txt b/docs/memory.txt > index 8745f76..1a3ad622 100644 > --- a/docs/memory.txt > +++ b/docs/memory.txt > @@ -297,7 +297,7 @@ various constraints can be supplied to control how these callbacks are called: > - .valid.min_access_size, .valid.max_access_size define the access sizes > (in bytes) which the device accepts; accesses outside this range will > have device and bus specific behaviour (ignored, or machine check) > - - .valid.aligned specifies that the device only accepts naturally aligned > + - .valid.unaligned specifies that the device only accepts naturally aligned > accesses. Unaligned accesses invoke device and bus specific behaviour. This doesn't look like the right change, because (a) a field named unaligned which you set true to specifiy that unaligned accesses are invalid would be very confusing and (b) the comment in the header file says that 'valid.unaligned' means that the device does support unaligned accesses. > - .impl.min_access_size, .impl.max_access_size define the access sizes > (in bytes) supported by the *implementation*; other access sizes will be We also seem to be missing documentation of valid.accepts. thanks -- PMM
On 02/25/2016 06:00 PM, Peter Maydell wrote: > On 25 February 2016 at 09:32, Cao jin <caoj.fnst@cn.fujitsu.com> wrote: >> to match structure MemoryRegionOps definition >> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> And there seems are some out of date content in this file, like >> "cpu_register_io_memory()" mentioned in the last line, which is removed several >> years ago in commit 97161e177. I am sure how to modify it, So I take the >> liberty to CC the guys in the cc-list(get list from get_maintainer.pl).k > Hi peter, Thanks for your quick response. Can`t believe I miss a "not" in my word..."I am not sure how to modify it" so I CC you guys:)
On 02/25/2016 06:00 PM, Peter Maydell wrote: > On 25 February 2016 at 09:32, Cao jin <caoj.fnst@cn.fujitsu.com> wrote: >> diff --git a/docs/memory.txt b/docs/memory.txt >> index 8745f76..1a3ad622 100644 >> --- a/docs/memory.txt >> +++ b/docs/memory.txt >> @@ -297,7 +297,7 @@ various constraints can be supplied to control how these callbacks are called: >> - .valid.min_access_size, .valid.max_access_size define the access sizes >> (in bytes) which the device accepts; accesses outside this range will >> have device and bus specific behaviour (ignored, or machine check) >> - - .valid.aligned specifies that the device only accepts naturally aligned >> + - .valid.unaligned specifies that the device only accepts naturally aligned >> accesses. Unaligned accesses invoke device and bus specific behaviour. > > This doesn't look like the right change, because (a) a field named > unaligned which you set true to specifiy that unaligned accesses > are invalid would be very confusing and (b) the comment in the > header file says that 'valid.unaligned' means that the device does > support unaligned accesses. > the reason that I think it is a typo is: from the pattern ".xxx.yyy", this section looks like a explanation(or detailed comment) of struct MemoryRegionOps`s fields, isn`t it? If yes, all the others match with the structure, except this /.valid.aligned/ I admit that, the description like ".valid.unaligned specifies that the device only accepts naturally aligned accesses" looks very confusing. But from the only caller memory_region_access_valid(), I think the original maybe not quite good? how about this: /.valid.unaligned specifies that the device accepts unaligned accesses. If false, Unaligned accesses invoke device and bus specific behaviour/
On 25 February 2016 at 13:30, Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > > > On 02/25/2016 06:00 PM, Peter Maydell wrote: >> >> On 25 February 2016 at 09:32, Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > > >>> diff --git a/docs/memory.txt b/docs/memory.txt >>> index 8745f76..1a3ad622 100644 >>> --- a/docs/memory.txt >>> +++ b/docs/memory.txt >>> @@ -297,7 +297,7 @@ various constraints can be supplied to control how >>> these callbacks are called: >>> - .valid.min_access_size, .valid.max_access_size define the access >>> sizes >>> (in bytes) which the device accepts; accesses outside this range >>> will >>> have device and bus specific behaviour (ignored, or machine check) >>> - - .valid.aligned specifies that the device only accepts naturally >>> aligned >>> + - .valid.unaligned specifies that the device only accepts naturally >>> aligned >>> accesses. Unaligned accesses invoke device and bus specific >>> behaviour. >> >> >> This doesn't look like the right change, because (a) a field named >> unaligned which you set true to specifiy that unaligned accesses >> are invalid would be very confusing and (b) the comment in the >> header file says that 'valid.unaligned' means that the device does >> support unaligned accesses. >> > > the reason that I think it is a typo is: from the pattern ".xxx.yyy", this > section looks like a explanation(or detailed comment) of struct > MemoryRegionOps`s fields, isn`t it? If yes, all the others match with the > structure, except this /.valid.aligned/ You're certainly right that the documentation as it stands is wrong. I don't think this is a simple typo though. Probably what happened was that when the API was being designed it started off with an 'unaligned' field, and then later the field name and semantics were changed but the docs weren't updated to match. My point was that just changing the name without looking at the behaviour we're actually implementing isn't correct. > I admit that, the description like ".valid.unaligned specifies that the > device only accepts naturally aligned accesses" looks very confusing. But > from the only caller memory_region_access_valid(), I think the original > maybe not quite good? how about this: > /.valid.unaligned specifies that the device accepts unaligned accesses. If > false, Unaligned accesses invoke device and bus specific behaviour/ I would suggest: - .valid.unaligned specifies that the *device being modelled* supports unaligned accesses; if false, unaligned accesses will invoke the appropriate bus or CPU specific behaviour. (the idea being to make the text parallel to the existing .impl.unaligned docs so it's clear what the difference is.) thanks -- PMM
On 02/25/2016 10:24 PM, Peter Maydell wrote: > On 25 February 2016 at 13:30, Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > > You're certainly right that the documentation as it stands is wrong. > I don't think this is a simple typo though. Probably what happened > was that when the API was being designed it started off with an > 'unaligned' field, and then later the field name and semantics > were changed but the docs weren't updated to match. My point was > that just changing the name without looking at the behaviour we're > actually implementing isn't correct. > Totally agree with the point. >> I admit that, the description like ".valid.unaligned specifies that the >> device only accepts naturally aligned accesses" looks very confusing. But >> from the only caller memory_region_access_valid(), I think the original >> maybe not quite good? how about this: > >> /.valid.unaligned specifies that the device accepts unaligned accesses. If >> false, Unaligned accesses invoke device and bus specific behaviour/ > > I would suggest: > > - .valid.unaligned specifies that the *device being modelled* > supports unaligned accesses; if false, unaligned accesses will > invoke the appropriate bus or CPU specific behaviour. > Does better. I want to pack this into a trivial patch first, because the original is not easy to read & understand > (the idea being to make the text parallel to the existing > .impl.unaligned docs so it's clear what the difference is.) > > thanks > -- PMM > > > . >
diff --git a/docs/memory.txt b/docs/memory.txt index 8745f76..1a3ad622 100644 --- a/docs/memory.txt +++ b/docs/memory.txt @@ -297,7 +297,7 @@ various constraints can be supplied to control how these callbacks are called: - .valid.min_access_size, .valid.max_access_size define the access sizes (in bytes) which the device accepts; accesses outside this range will have device and bus specific behaviour (ignored, or machine check) - - .valid.aligned specifies that the device only accepts naturally aligned + - .valid.unaligned specifies that the device only accepts naturally aligned accesses. Unaligned accesses invoke device and bus specific behaviour. - .impl.min_access_size, .impl.max_access_size define the access sizes (in bytes) supported by the *implementation*; other access sizes will be
to match structure MemoryRegionOps definition Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- And there seems are some out of date content in this file, like "cpu_register_io_memory()" mentioned in the last line, which is removed several years ago in commit 97161e177. I am sure how to modify it, So I take the liberty to CC the guys in the cc-list(get list from get_maintainer.pl). docs/memory.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)