Re: [PATCH] docs/cxl: fix some typos

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Sat, 22 Jun 2024 16:25:26 +0900
Hyeongtak Ji <hyeongtak.ji@gmail.com> wrote:

> Hello Jonathan,
> 
> Thank you for your response.
> 
> On Sat, Jun 22, 2024 at 1:10 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Wed, 19 Jun 2024 13:54:59 +0900
> > Hyeongtak Ji <hyeongtak.ji@gmail.com> wrote:
> >
> > Hi, some description would be good of how you caught these
> > (I'm guessing a close read).  
> 
> Just to confirm, are you suggesting that the patch should include a
> commit message?  I apologize for submitting the patch without any
> sufficient explanation.  However, I am not entirely sure if "how I
> found these typos" needs to be included in the commit message.  For
> your information, I discovered these typos because the ASCII art did
> not align with the explanations (yes, a close read).

Mostly just wanted a patch description that said something.
It can be useful to have even minimal info such as
"Found by inspection relative to the ASCII art above."
Helps guide if people should read closely or not!

Thanks,

Jonathan


> 
> >
> > Whilst checking this I did notice there are some errors in
> > the example bus numbering but that's a separate issue.
> >
> > Jonathan
> >
> >  
> > > Signed-off-by: Hyeongtak Ji <hyeongtak.ji@gmail.com>
> > > ---
> > >  docs/system/devices/cxl.rst | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/docs/system/devices/cxl.rst b/docs/system/devices/cxl.rst
> > > index 10a0e9bc9ff4..e2497e6a098b 100644
> > > --- a/docs/system/devices/cxl.rst
> > > +++ b/docs/system/devices/cxl.rst
> > > @@ -218,17 +218,17 @@ Notes:
> > >      A complex configuration here, might be to use the following HDM
> > >      decoders in HB0. HDM0 routes CFMW0 requests to RP0 and hence
> > >      part of CXL Type3 0. HDM1 routes CFMW0 requests from a
> > > -    different region of the CFMW0 PA range to RP2 and hence part
> > > +    different region of the CFMW0 PA range to RP1 and hence part  
> >
> > Good catch.
> >  
> > >      of CXL Type 3 1.  HDM2 routes yet another PA range from within
> > >      CFMW0 to be interleaved across RP0 and RP1, providing 2 way
> > >      interleave of part of the memory provided by CXL Type3 0 and
> > >      CXL Type 3 1. HDM3 routes those interleaved accesses from
> > >      CFMW1 that target HB0 to RP 0 and another part of the memory of
> > >      CXL Type 3 0 (as part of a 2 way interleave at the system level
> > > -    across for example CXL Type3 0 and CXL Type3 2.
> > > +    across for example CXL Type3 0 and CXL Type3 1).  
> > This one is wrong.  CFMW1 interleaves across both host bridges so we need
> > a device below HB0 and one below HB1, so CXL type3 2 is a possible choice
> > (could be CXL type3 3 as well, but that doesn't matter.)  
> 
> Oh, I misunderstood the original explanation.  I will correct it just by
> adding the missing parenthesis instead.
> 
> >  
> > >      HDM4 is used to enable system wide 4 way interleave across all
> > >      the present CXL type3 devices, by interleaving those (interleaved)
> > > -    requests that HB0 receives from from CFMW1 across RP 0 and
> > > +    requests that HB0 receives from CFMW1 across RP 0 and  
> > Good.
> >  
> > >      RP 1 and hence to yet more regions of the memory of the
> > >      attached Type3 devices.  Note this is a representative subset
> > >      of the full range of possible HDM decoder configurations in this  
> >  
> 
> I will send V2 with a decent explanation and the corrected typo fix.
> 
> Kind regards,
> Hyeongtak
> 
> On Sat, Jun 22, 2024 at 1:10 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Wed, 19 Jun 2024 13:54:59 +0900
> > Hyeongtak Ji <hyeongtak.ji@gmail.com> wrote:
> >
> > Hi, some description would be good of how you caught these
> > (I'm guessing a close read).
> >
> > Whilst checking this I did notice there are some errors in
> > the example bus numbering but that's a separate issue.
> >
> > Jonathan
> >
> >  
> > > Signed-off-by: Hyeongtak Ji <hyeongtak.ji@gmail.com>
> > > ---
> > >  docs/system/devices/cxl.rst | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/docs/system/devices/cxl.rst b/docs/system/devices/cxl.rst
> > > index 10a0e9bc9ff4..e2497e6a098b 100644
> > > --- a/docs/system/devices/cxl.rst
> > > +++ b/docs/system/devices/cxl.rst
> > > @@ -218,17 +218,17 @@ Notes:
> > >      A complex configuration here, might be to use the following HDM
> > >      decoders in HB0. HDM0 routes CFMW0 requests to RP0 and hence
> > >      part of CXL Type3 0. HDM1 routes CFMW0 requests from a
> > > -    different region of the CFMW0 PA range to RP2 and hence part
> > > +    different region of the CFMW0 PA range to RP1 and hence part  
> >
> > Good catch.
> >  
> > >      of CXL Type 3 1.  HDM2 routes yet another PA range from within
> > >      CFMW0 to be interleaved across RP0 and RP1, providing 2 way
> > >      interleave of part of the memory provided by CXL Type3 0 and
> > >      CXL Type 3 1. HDM3 routes those interleaved accesses from
> > >      CFMW1 that target HB0 to RP 0 and another part of the memory of
> > >      CXL Type 3 0 (as part of a 2 way interleave at the system level
> > > -    across for example CXL Type3 0 and CXL Type3 2.
> > > +    across for example CXL Type3 0 and CXL Type3 1).  
> > This one is wrong.  CFMW1 interleaves across both host bridges so we need
> > a device below HB0 and one below HB1, so CXL type3 2 is a possible choice
> > (could be CXL type3 3 as well, but that doesn't matter.)
> >  
> > >      HDM4 is used to enable system wide 4 way interleave across all
> > >      the present CXL type3 devices, by interleaving those (interleaved)
> > > -    requests that HB0 receives from from CFMW1 across RP 0 and
> > > +    requests that HB0 receives from CFMW1 across RP 0 and  
> > Good.
> >  
> > >      RP 1 and hence to yet more regions of the memory of the
> > >      attached Type3 devices.  Note this is a representative subset
> > >      of the full range of possible HDM decoder configurations in this  
> >