Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1701369 > unrolled thread

Re: [PATCH v3 14/16] switchtec_ntb: implement scratchpad registers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 14/16] switchtec_ntb: implement scratchpad registers Jon Mason <jdmason@kudzu.us> - 2017-08-01 21:20 +0200
    Re: [PATCH v3 14/16] switchtec_ntb: implement scratchpad registers Logan Gunthorpe <logang@deltatee.com> - 2017-08-02 00:30 +0200
      RE: [PATCH v3 14/16] switchtec_ntb: implement scratchpad registers "Allen Hubbe" <Allen.Hubbe@dell.com> - 2017-08-02 15:10 +0200
        Re: [PATCH v3 14/16] switchtec_ntb: implement scratchpad registers Jon Mason <jdmason@kudzu.us> - 2017-08-02 18:40 +0200
          Re: [PATCH v3 14/16] switchtec_ntb: implement scratchpad registers Logan Gunthorpe <logang@deltatee.com> - 2017-08-02 19:10 +0200

#1701369 — Re: [PATCH v3 14/16] switchtec_ntb: implement scratchpad registers

On Tue, Jul 25, 2017 at 02:57:51PM -0600, Logan Gunthorpe wrote:
> Seeing there is no dedicated hardware for this, we simply add
> these as entries in the shared memory window. Thus, we could support
> any number of them but 128 seems like enough, for now.

It would probaly be better if I remarked about the SPADs in the actual
patch about the SPADS :)

The whole point of using the SPADs in the NTB driver was to workaround
the problems establishing a connection between the two sides of the
NTB and where everything lives.  So, using a MW to get around the
SPADs is sort of backwards (and slightly funny).  I realize you are
trying to use the existing transport with minimal changes to enable
your hardware, and thus this makes logical sense to you.  However, if
the SPADs are not really needed, then we should either remove them
from the transport (or use them for something else).

Per my comment in the other patch, I'm amenable to take this series
as-is, assuming you are willing to address this design issue in the
near future.  Thoughts?

Thanks,
Jon

> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Stephen Bates <sbates@raithlin.com>
> Reviewed-by: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
> ---
>  drivers/ntb/hw/mscc/switchtec_ntb.c | 75 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ntb/hw/mscc/switchtec_ntb.c b/drivers/ntb/hw/mscc/switchtec_ntb.c
> index 3037ca730998..ca403ad2f7ac 100644
> --- a/drivers/ntb/hw/mscc/switchtec_ntb.c
> +++ b/drivers/ntb/hw/mscc/switchtec_ntb.c
> @@ -67,6 +67,7 @@ struct shared_mw {
>  	u32 link_sta;
>  	u32 partition_id;
>  	u64 mw_sizes[MAX_MWS];
> +	u32 spad[128];
>  };
>  
>  #define MAX_DIRECT_MW ARRAY_SIZE(((struct ntb_ctrl_regs *)(0))->bar_entry)
> @@ -455,22 +456,90 @@ static int switchtec_ntb_peer_db_set(struct ntb_dev *ntb, u64 db_bits)
>  
>  static int switchtec_ntb_spad_count(struct ntb_dev *ntb)
>  {
> -	return 0;
> +	struct switchtec_ntb *sndev = ntb_sndev(ntb);
> +
> +	return ARRAY_SIZE(sndev->self_shared->spad);
>  }
>  
>  static u32 switchtec_ntb_spad_read(struct ntb_dev *ntb, int idx)
>  {
> -	return 0;
> +	struct switchtec_ntb *sndev = ntb_sndev(ntb);
> +
> +	if (idx < 0 || idx >= ARRAY_SIZE(sndev->self_shared->spad))
> +		return 0;
> +
> +	if (!sndev->self_shared)
> +		return 0;
> +
> +	return sndev->self_shared->spad[idx];
>  }
>  
>  static int switchtec_ntb_spad_write(struct ntb_dev *ntb, int idx, u32 val)
>  {
> +	struct switchtec_ntb *sndev = ntb_sndev(ntb);
> +
> +	if (idx < 0 || idx >= ARRAY_SIZE(sndev->self_shared->spad))
> +		return -EINVAL;
> +
> +	if (!sndev->self_shared)
> +		return -EIO;
> +
> +	sndev->self_shared->spad[idx] = val;
> +
>  	return 0;
>  }
>  
> +static u32 switchtec_ntb_peer_spad_read(struct ntb_dev *ntb, int pidx,
> +					int sidx)
> +{
> +	struct switchtec_ntb *sndev = ntb_sndev(ntb);
> +
> +	if (pidx != NTB_DEF_PEER_IDX)
> +		return -EINVAL;
> +
> +	if (sidx < 0 || sidx >= ARRAY_SIZE(sndev->peer_shared->spad))
> +		return 0;
> +
> +	if (!sndev->peer_shared)
> +		return 0;
> +
> +	return ioread32(&sndev->peer_shared->spad[sidx]);
> +}
> +
>  static int switchtec_ntb_peer_spad_write(struct ntb_dev *ntb, int pidx,
>  					 int sidx, u32 val)
>  {
> +	struct switchtec_ntb *sndev = ntb_sndev(ntb);
> +
> +	if (pidx != NTB_DEF_PEER_IDX)
> +		return -EINVAL;
> +
> +	if (sidx < 0 || sidx >= ARRAY_SIZE(sndev->peer_shared->spad))
> +		return -EINVAL;
> +
> +	if (!sndev->peer_shared)
> +		return -EIO;
> +
> +	iowrite32(val, &sndev->peer_shared->spad[sidx]);
> +
> +	return 0;
> +}
> +
> +static int switchtec_ntb_peer_spad_addr(struct ntb_dev *ntb, int pidx,
> +					int sidx, phys_addr_t *spad_addr)
> +{
> +	struct switchtec_ntb *sndev = ntb_sndev(ntb);
> +	unsigned long offset;
> +
> +	if (pidx != NTB_DEF_PEER_IDX)
> +		return -EINVAL;
> +
> +	offset = (unsigned long)&sndev->peer_shared->spad[sidx] -
> +		(unsigned long)sndev->stdev->mmio;
> +
> +	if (spad_addr)
> +		*spad_addr = pci_resource_start(ntb->pdev, 0) + offset;
> +
>  	return 0;
>  }
>  
> @@ -496,7 +565,9 @@ static const struct ntb_dev_ops switchtec_ntb_ops = {
>  	.spad_count		= switchtec_ntb_spad_count,
>  	.spad_read		= switchtec_ntb_spad_read,
>  	.spad_write		= switchtec_ntb_spad_write,
> +	.peer_spad_read		= switchtec_ntb_peer_spad_read,
>  	.peer_spad_write	= switchtec_ntb_peer_spad_write,
> +	.peer_spad_addr		= switchtec_ntb_peer_spad_addr,
>  };
>  
>  static void switchtec_ntb_init_sndev(struct switchtec_ntb *sndev)
> -- 
> 2.11.0
> 

[toc] | [next] | [standalone]

#1701546

On 01/08/17 01:10 PM, Jon Mason wrote:
> It would probaly be better if I remarked about the SPADs in the actual
> patch about the SPADS :)
> 
> The whole point of using the SPADs in the NTB driver was to workaround
> the problems establishing a connection between the two sides of the
> NTB and where everything lives.  So, using a MW to get around the
> SPADs is sort of backwards (and slightly funny).  I realize you are
> trying to use the existing transport with minimal changes to enable
> your hardware, and thus this makes logical sense to you.  However, if
> the SPADs are not really needed, then we should either remove them
> from the transport (or use them for something else).
>
> Per my comment in the other patch, I'm amenable to take this series
> as-is, assuming you are willing to address this design issue in the
> near future.  Thoughts?

Yes, I agree. I'd be willing to help but it seems the clients are
written the way they are for the other drivers, so it's their needs
(which I'm not fully aware of) that have to be considered.

I've also made all the other changes you sent as well as the file rename
Dave requested. Once I see the bug fix patch you were going to pull hit
ntb-next I'll rebase, test and resubmit.

Thanks,

Logan

[toc] | [prev] | [next] | [standalone]

#1702094

From: Logan Gunthorpe
> On 01/08/17 01:10 PM, Jon Mason wrote:
> > It would probaly be better if I remarked about the SPADs in the actual
> > patch about the SPADS :)
> >
> > The whole point of using the SPADs in the NTB driver was to workaround
> > the problems establishing a connection between the two sides of the
> > NTB and where everything lives.  So, using a MW to get around the
> > SPADs is sort of backwards (and slightly funny).  I realize you are
> > trying to use the existing transport with minimal changes to enable
> > your hardware, and thus this makes logical sense to you.  However, if
> > the SPADs are not really needed, then we should either remove them
> > from the transport (or use them for something else).
> >
> > Per my comment in the other patch, I'm amenable to take this series
> > as-is, assuming you are willing to address this design issue in the
> > near future.  Thoughts?
> 
> Yes, I agree. I'd be willing to help but it seems the clients are
> written the way they are for the other drivers, so it's their needs
> (which I'm not fully aware of) that have to be considered.

The proposed change, removing use of spads from transport, would not affect ntrdma.

> I've also made all the other changes you sent as well as the file rename
> Dave requested. Once I see the bug fix patch you were going to pull hit
> ntb-next I'll rebase, test and resubmit.
> 
> Thanks,
> 
> Logan

[toc] | [prev] | [next] | [standalone]

#1702259

On Wed, Aug 2, 2017 at 9:06 AM, Allen Hubbe <Allen.Hubbe@dell.com> wrote:
> From: Logan Gunthorpe
>> On 01/08/17 01:10 PM, Jon Mason wrote:
>> > It would probaly be better if I remarked about the SPADs in the actual
>> > patch about the SPADS :)
>> >
>> > The whole point of using the SPADs in the NTB driver was to workaround
>> > the problems establishing a connection between the two sides of the
>> > NTB and where everything lives.  So, using a MW to get around the
>> > SPADs is sort of backwards (and slightly funny).  I realize you are
>> > trying to use the existing transport with minimal changes to enable
>> > your hardware, and thus this makes logical sense to you.  However, if
>> > the SPADs are not really needed, then we should either remove them
>> > from the transport (or use them for something else).
>> >
>> > Per my comment in the other patch, I'm amenable to take this series
>> > as-is, assuming you are willing to address this design issue in the
>> > near future.  Thoughts?
>>
>> Yes, I agree. I'd be willing to help but it seems the clients are
>> written the way they are for the other drivers, so it's their needs
>> (which I'm not fully aware of) that have to be considered.
>
> The proposed change, removing use of spads from transport, would not affect ntrdma.

After a long-ish conversation on IRC, the way we want to go forward
would be to provide 2 ways of communicating prior to the MW's being
setup:  SPADs and Message Registers.  The HW driver would make
available whichever ones are supported by the hardware.  The transport
can see which of those are available in the HW driver, select the
appropriate one, and use it to setup the NTB connection, MW, etc.
similar to how the SPADs are being used today.  This would allow for
any current clients to work unmodified, and would require minimal
changes to the existing transport layer.

Since this is outside the scope of this series, per my email yesterday
allowing the SPAD workaround, we should start up another thread on the
NTB mailing list and flesh out the details and any benefits/drawbacks.
Then we, as a community, can make the changes necessary to the drivers
and transport to get this working more optimally.

Thanks,
Jon

>> I've also made all the other changes you sent as well as the file rename
>> Dave requested. Once I see the bug fix patch you were going to pull hit
>> ntb-next I'll rebase, test and resubmit.
>>
>> Thanks,
>>
>> Logan
>

[toc] | [prev] | [next] | [standalone]

#1702276

On 02/08/17 10:32 AM, Jon Mason wrote:
> After a long-ish conversation on IRC, the way we want to go forward
> would be to provide 2 ways of communicating prior to the MW's being
> setup:  SPADs and Message Registers.  The HW driver would make
> available whichever ones are supported by the hardware.  The transport
> can see which of those are available in the HW driver, select the
> appropriate one, and use it to setup the NTB connection, MW, etc.
> similar to how the SPADs are being used today.  This would allow for
> any current clients to work unmodified, and would require minimal
> changes to the existing transport layer.

I'm open to this, but I have a couple points:

1) I think there needs to be an ntb library or similar so the client
just has to say "communicate this setup data to the peer X". We don't
need every client replicating the decision to use spads or msgs or whatever.

2) In my experience, message registers are a bit of a pain to send
chunks of data like is being done with spads in the existing clients.
(Some time ago, I originally tried emulating spads with message
registers and it did not work well.)  You can only send 32bits at a time
and you have to have a signal indicating the other side is finished with
it before sending another message. I think they are best used just for
signaling infrequent events where if you loose a message it's not an issue.

One thought I had was that maybe we need to just push this decision into
the drivers. ie if there's an ntb api just for communicating setup data,
the driver could then determine the most appropriate way to communicate
it for the hardware (whether via spads, messages or a special memory
window). However, the difficulty with this is that the driver would have
to communicate if it's using spad resources it would otherwise advertise
to the clients.

Though, maybe we don't need spads in the ntb api. Maybe they are the
wrong abstraction. All the existing clients only use them for setup
data, so if we dropped the spad api in favor of a setup data api, the
driver could then just make the decision on how best to communicate it.

Anyway, just my thoughts.

Logan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web