Re: [PATCH v2] cxl: Enable an endpoint decoder type

[Dan Williams <dan.j.williams@intel.com>]

On Fri, Jul 16, 2021 at 4:37 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-07-09 16:56:48, Dan Williams wrote:
> > On Tue, Jul 6, 2021 at 9:01 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > CXL memory devices support HDM decoders. Currently, when a decoder is
> > > instantiated there is no knowledge of the type of decoder; only the
> > > underlying endpoint type is specified. In order to have the memory
> > > devices reuse the existing decoder creation infrastructure it is
> > > convenient to pass along the type of decoder on creation.
> > >
> > > The primary difference for an endpoint decoder is that it doesn't have
> > > dports, nor targets. The target is just the underlying media (with
> > > offset).
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > ---
> > >
> > > v2 Fixes target_type and stores the decoder type on instantiation
> >
> > This depends on the memdev driver series? It's not applying for me on
> > top of cxl.git#next.
> >
>
> Weird, it shouldn't. All my other branches depend on core-reorg work, where
> core.c isn't a thing anymore. Would you like to get this in shape before the
> core rework? I think git should dtrt, but it might be easier to merge that
> first.

Yeah, I think it logically makes sense to finish off the core
representation of ports before enabling them.

>
> > >
> > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > > index 196f260e2580..69acdd230f54 100644
> > > --- a/drivers/cxl/core.c
> > > +++ b/drivers/cxl/core.c
> > > @@ -493,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> > >                         .start = base,
> > >                         .end = base + len - 1,
> > >                 },
> > > +               .type = type,
> >
> > ...but there's already the dev->type?
>
> Good point. It's redundant.
>
> >
> > >                 .flags = flags,
> > >                 .interleave_ways = interleave_ways,
> > >                 .interleave_granularity = interleave_granularity,
> > > -               .target_type = type,
> > > +               .target_type = CXL_DEVICE_EXPANDER,
> >
> > In the cxl-switch case how to indicate that only type-2 targets are supported?
> >
> > I do think I misnamed the cxl_decoder_type. I also did not make it
> > clear that root decoders don't have a target type, they have a set of
> > flags indicating their restrictions, and unlike switch level decoders
> > they can support targeting both accelerators and expanders in the same
> > windows. I think the decoder can still be just the dev->type, but
> > there needs to be separate helpers for the 3 cases you have
> > identified. Something like the following where devm_cxl_add_decoder()
> > because private to the core:
> >
> > devm_cxl_add_platform_decoder(struct device *host, int nr_targets,
> >                               resource_size_t base, resource_size_t len,
> >                               int interleave_ways, int interleave_granularity,
> >                               unsigned long flags)
> > {
> >         return devm_cxl_add_decoder(host, NULL, nr_targets, base, len,
> >                                     interleave_ways, interleave_granularity,
> >                                     0, flags);
> > }
> >
> > devm_cxl_add_switch_decoder(struct device *host, struct cxl_port *port,
> >                             enum cxl_decoder_type type)
> > {
> >         return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0,
> >                                     CXL_DECODER_UNKNOWN, 0);
> > }
> >
> > devm_cxl_add_endpoint_decoder(struct device *host, struct cxl_port *port,
> >                               enum cxl_decoder_type type)
> > {
> >         return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0, type,
> >                                     CXL_DECODER_F_ENDPOINT);
> > }
> >
> > ...where 0 values are filled in by the decoder driver init or N/A.
> > Presumably the memdev driver calling devm_cxl_add_endpoint_decoder()
> > will know whether it is an expander or an accelerator. Although given
> > there are no CXL accelerator drivers on the horizon, maybe even that
> > degree of freedom can be hidden for now.
> >
> > Then the dev->type determination is:
> >
> > platform => no parent cxl_port
> > switch => parent cxl_port and flags does not have CXL_DECODER_F_ENDPOINT
> > endpoint =>  parent cxl_port and flags has CXL_DECODER_F_ENDPOINT
> >
>
> I'm definitely in favor of being more explicit.

The exported function names are not explicit enough?

The thought is that the baseline devm_cxl_add_port() becomes static
and private to the core.

> Are you opposed to having the
> helper set the type and getting rid of endpoint flag? I think it's a little
> non-idiomatic, but I do prefer it stylistically.

The endpoint flag is to disambiguate switches vs endpoints...

>
> ie.
> devm_cxl_add_endpoint_decoder(struct device *host, struct cxl_port *port,
>                               enum cxl_decoder_type type)

...but type is CXL_DECODER_{PLATFORM,SWITCH,ENDPOINT}, what does this
function do when it is passed anything CXL_DECODER_PLATFORM? My
original intention for 'type' was 'expander' vs 'accelerator' since
switch decoders exclusively support one or the other.

> {
>         struct cxl_decoder *cxld;
>
>         cxld = devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0, type, 0);
>         if (!ret)
>                 cxld->dev.type = &cxl_decoder_endpoint_type;

Unfortunately this is too late as dev->type is in use immediately upon
device_add().