LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Eddie James <eajames@linux.ibm.com>
To: Eduardo Valentin <eduval@amazon.com>
Cc: linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org,
	arnd@arndb.de, robh+dt@kernel.org, mark.rutland@arm.com,
	devicetree@vger.kernel.org, joel@jms.id.au, andrew@aj.id.au
Subject: Re: [PATCH v3 5/8] drivers/soc: xdma: Add PCI device configuration sysfs
Date: Mon, 1 Jul 2019 13:38:56 -0500	[thread overview]
Message-ID: <358d0c5b-1473-bb6a-810d-45230b890a55@linux.ibm.com> (raw)
In-Reply-To: <20190531034502.GH17772@u40b0340c692b58f6553c.ant.amazon.com>


On 5/30/19 10:45 PM, Eduardo Valentin wrote:
> On Wed, May 29, 2019 at 01:10:05PM -0500, Eddie James wrote:
>> The AST2500 has two PCI devices embedded. The XDMA engine can use either
>> device to perform DMA transfers. Users need the capability to choose
>> which device to use. This commit therefore adds two sysfs files that
>> toggle the AST2500 and XDMA engine between the two PCI devices.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   drivers/soc/aspeed/aspeed-xdma.c | 103 +++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 100 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/soc/aspeed/aspeed-xdma.c b/drivers/soc/aspeed/aspeed-xdma.c
>> index 39f6545..ddd5e1e 100644
>> --- a/drivers/soc/aspeed/aspeed-xdma.c
>> +++ b/drivers/soc/aspeed/aspeed-xdma.c
>> @@ -143,6 +143,7 @@ struct aspeed_xdma {
>>   	void *cmdq_vga_virt;
>>   	struct gen_pool *vga_pool;
>>   
>> +	char pcidev[4];
>>   	struct miscdevice misc;
>>   };
>>   
>> @@ -165,6 +166,10 @@ struct aspeed_xdma_client {
>>   	SCU_PCIE_CONF_VGA_EN_IRQ | SCU_PCIE_CONF_VGA_EN_DMA |
>>   	SCU_PCIE_CONF_RSVD;
>>   
>> +static char *_pcidev = "vga";
>> +module_param_named(pcidev, _pcidev, charp, 0600);
>> +MODULE_PARM_DESC(pcidev, "Default PCI device used by XDMA engine for DMA ops");
>> +
>>   static void aspeed_scu_pcie_write(struct aspeed_xdma *ctx, u32 conf)
>>   {
>>   	u32 v = 0;
>> @@ -512,7 +517,7 @@ static int aspeed_xdma_release(struct inode *inode, struct file *file)
>>   	.release		= aspeed_xdma_release,
>>   };
>>   
>> -static int aspeed_xdma_init_mem(struct aspeed_xdma *ctx)
>> +static int aspeed_xdma_init_mem(struct aspeed_xdma *ctx, u32 conf)
>>   {
>>   	int rc;
>>   	u32 scu_conf = 0;
>> @@ -522,7 +527,7 @@ static int aspeed_xdma_init_mem(struct aspeed_xdma *ctx)
>>   	const u32 vga_sizes[4] = { 0x800000, 0x1000000, 0x2000000, 0x4000000 };
>>   	void __iomem *sdmc_base = ioremap(0x1e6e0000, 0x100);
>>   
>> -	aspeed_scu_pcie_write(ctx, aspeed_xdma_vga_pcie_conf);
>> +	aspeed_scu_pcie_write(ctx, conf);
>>   
>>   	regmap_read(ctx->scu, SCU_STRAP, &scu_conf);
>>   	ctx->vga_size = vga_sizes[FIELD_GET(SCU_STRAP_VGA_MEM, scu_conf)];
>> @@ -598,10 +603,91 @@ static int aspeed_xdma_init_mem(struct aspeed_xdma *ctx)
>>   	return rc;
>>   }
>>   
>> +static int aspeed_xdma_change_pcie_conf(struct aspeed_xdma *ctx, u32 conf)
>> +{
>> +	int rc;
>> +
>> +	mutex_lock(&ctx->start_lock);
>> +	rc = wait_event_interruptible_timeout(ctx->wait,
>> +					      !test_bit(XDMA_IN_PRG,
>> +							&ctx->flags),
>> +					      msecs_to_jiffies(1000));
>> +	if (rc < 0) {
>> +		mutex_unlock(&ctx->start_lock);
>> +		return -EINTR;
>> +	}
>> +
>> +	/* previous op didn't complete, wake up waiters anyway */
>> +	if (!rc)
>> +		wake_up_interruptible_all(&ctx->wait);
>> +
>> +	reset_control_assert(ctx->reset);
>> +	msleep(10);
>> +
>> +	aspeed_scu_pcie_write(ctx, conf);
>> +	msleep(10);
>> +
>> +	reset_control_deassert(ctx->reset);
>> +	msleep(10);
>> +
>> +	aspeed_xdma_init_eng(ctx);
>> +
>> +	mutex_unlock(&ctx->start_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static int aspeed_xdma_pcidev_to_conf(struct aspeed_xdma *ctx,
>> +				      const char *pcidev, u32 *conf)
>> +{
>> +	if (!strcasecmp(pcidev, "vga")) {
>> +		*conf = aspeed_xdma_vga_pcie_conf;
>> +		return 0;
>> +	}
>> +
>> +	if (!strcasecmp(pcidev, "bmc")) {
>> +		*conf = aspeed_xdma_bmc_pcie_conf;
>> +		return 0;
>> +	}
> strncasecmp()?


Yes, will change, and below.


>
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static ssize_t aspeed_xdma_show_pcidev(struct device *dev,
>> +				       struct device_attribute *attr,
>> +				       char *buf)
>> +{
>> +	struct aspeed_xdma *ctx = dev_get_drvdata(dev);
>> +
>> +	return snprintf(buf, PAGE_SIZE - 1, "%s", ctx->pcidev);
>> +}
>> +
>> +static ssize_t aspeed_xdma_store_pcidev(struct device *dev,
>> +					struct device_attribute *attr,
>> +					const char *buf, size_t count)
>> +{
>> +	u32 conf;
>> +	struct aspeed_xdma *ctx = dev_get_drvdata(dev);
>> +	int rc = aspeed_xdma_pcidev_to_conf(ctx, buf, &conf);
>> +
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = aspeed_xdma_change_pcie_conf(ctx, conf);
>> +	if (rc)
>> +		return rc;
>> +
>> +	strcpy(ctx->pcidev, buf);
> should we use strncpy() instead?
>
>> +	return count;
>> +}
>> +static DEVICE_ATTR(pcidev, 0644, aspeed_xdma_show_pcidev,
>> +		   aspeed_xdma_store_pcidev);
>> +
>>   static int aspeed_xdma_probe(struct platform_device *pdev)
>>   {
>>   	int irq;
>>   	int rc;
>> +	u32 conf;
>>   	struct resource *res;
>>   	struct device *dev = &pdev->dev;
>>   	struct aspeed_xdma *ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> @@ -657,7 +743,14 @@ static int aspeed_xdma_probe(struct platform_device *pdev)
>>   
>>   	msleep(10);
>>   
>> -	rc = aspeed_xdma_init_mem(ctx);
>> +	if (aspeed_xdma_pcidev_to_conf(ctx, _pcidev, &conf)) {
>> +		conf = aspeed_xdma_vga_pcie_conf;
>> +		strcpy(ctx->pcidev, "vga");
>> +	} else {
>> +		strcpy(ctx->pcidev, _pcidev);
>> +	}
> same...
>
>> +
>> +	rc = aspeed_xdma_init_mem(ctx, conf);
>>   	if (rc) {
>>   		reset_control_assert(ctx->reset);
>>   		return rc;
>> @@ -682,6 +775,8 @@ static int aspeed_xdma_probe(struct platform_device *pdev)
>>   		return rc;
>>   	}
>>   
>> +	device_create_file(dev, &dev_attr_pcidev);
> Should we consider using one of the default attributes here instead of device_create_file()?
> http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/


Doesn't seem to be any way to create attributes per device with that 
method. Setting the device->groups in probe() doesn't do it.


>
> BTW, was this ABI documented? Is this the same file documented in patch 2?


Patch 4, but yes.


>
>> +
>>   	return 0;
>>   }
>>   
>> @@ -689,6 +784,8 @@ static int aspeed_xdma_remove(struct platform_device *pdev)
>>   {
>>   	struct aspeed_xdma *ctx = platform_get_drvdata(pdev);
>>   
>> +	device_remove_file(ctx->dev, &dev_attr_pcidev);
>> +
>>   	misc_deregister(&ctx->misc);
>>   	gen_pool_free(ctx->vga_pool, (unsigned long)ctx->cmdq_vga_virt,
>>   		      XDMA_CMDQ_SIZE);
>> -- 
>> 1.8.3.1
>>


  reply	other threads:[~2019-07-01 18:39 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-29 18:10 [PATCH v3 0/8] drivers/soc: Add Aspeed XDMA Engine Driver Eddie James
2019-05-29 18:10 ` [PATCH v3 1/8] dt-bindings: soc: Add Aspeed XDMA engine binding documentation Eddie James
2019-05-30  5:30   ` Andrew Jeffery
2019-06-27 19:19     ` Eddie James
2019-05-29 18:10 ` [PATCH v3 2/8] drivers/soc: Add Aspeed XDMA Engine Driver Eddie James
2019-05-31  3:31   ` Eduardo Valentin
2019-06-28 15:43     ` Eddie James
2019-05-29 18:10 ` [PATCH v3 3/8] drivers/soc: xdma: Add user interface Eddie James
2019-05-31  3:51   ` Eduardo Valentin
2019-05-29 18:10 ` [PATCH v3 4/8] Documentation: ABI: Add aspeed-xdma sysfs documentation Eddie James
2019-05-29 18:10 ` [PATCH v3 5/8] drivers/soc: xdma: Add PCI device configuration sysfs Eddie James
2019-05-31  3:45   ` Eduardo Valentin
2019-07-01 18:38     ` Eddie James [this message]
2019-05-29 18:10 ` [PATCH v3 6/8] drivers/soc: xdma: Add debugfs entries Eddie James
2019-05-29 18:10 ` [PATCH v3 7/8] ARM: dts: aspeed: Add XDMA Engine Eddie James
2019-05-29 18:10 ` [PATCH v3 8/8] ARM: dts: aspeed: witherspoon: Enable " Eddie James

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=358d0c5b-1473-bb6a-810d-45230b890a55@linux.ibm.com \
    --to=eajames@linux.ibm.com \
    --cc=andrew@aj.id.au \
    --cc=arnd@arndb.de \
    --cc=devicetree@vger.kernel.org \
    --cc=eduval@amazon.com \
    --cc=joel@jms.id.au \
    --cc=linux-aspeed@lists.ozlabs.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v3 5/8] drivers/soc: xdma: Add PCI device configuration sysfs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).