Thread (17 messages) 17 messages, 4 authors, 2016-09-13

RE: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

From: Y.B. Lu <hidden>
Date: 2016-09-13 07:57:17
Also in: linux-arm-kernel, linux-clk, linux-i2c, linux-iommu, linux-mmc, linuxppc-dev, lkml, netdev 

-----Original Message-----
From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
owner@vger.kernel.org] On Behalf Of Scott Wood
Sent: Tuesday, September 13, 2016 7:25 AM
To: Y.B. Lu; linux-mmc@vger.kernel.org; ulf.hansson@linaro.org; Arnd
Bergmann
Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
clk@vger.kernel.org; linux-i2c@vger.kernel.org; iommu@lists.linux-
foundation.org; netdev@vger.kernel.org; Mark Rutland; Rob Herring;
Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh
Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B. Xie
Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

On Mon, 2016-09-12 at 06:39 +0000, Y.B. Lu wrote:
quoted
Hi Scott,

Thanks for your review :)
See my comment inline.
quoted
-----Original Message-----
From: Scott Wood [mailto:oss@buserror.net]
Sent: Friday, September 09, 2016 11:47 AM
To: Y.B. Lu; linux-mmc@vger.kernel.org; ulf.hansson@linaro.org; Arnd
Bergmann
Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org;
linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
linux- clk@vger.kernel.org; linux-i2c@vger.kernel.org;
iommu@lists.linux- foundation.org; netdev@vger.kernel.org; Mark
Rutland; Rob Herring; Russell King; Jochen Friedrich; Joerg Roedel;
Claudiu Manoil; Bhupesh Sharma; Qiang Zhao; Kumar Gala; Santosh
Shilimkar; Leo Li; X.B. Xie
Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ
platforms

On Tue, 2016-09-06 at 16:28 +0800, Yangbo Lu wrote:
quoted
The global utilities block controls power management, I/O device
enabling, power-onreset(POR) configuration monitoring, alternate
function selection for multiplexed signals,and clock control.

This patch adds a driver to manage and access global utilities
block.
quoted
quoted
quoted
Initially only reading SVR and registering soc device are supported.
Other guts accesses, such as reading RCW, should eventually be
moved into this driver as well.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Signed-off-by: Scott Wood <oss@buserror.net>
Don't put my signoff on patches that I didn't put it on myself.
Definitely don't put mine *after* yours on patches that were last
modified by you.

If you want to mention that the soc_id encoding was my suggestion,
then do so explicitly.
[Lu Yangbo-B47093] I found your 'signoff' on this patch at below link.
http://patchwork.ozlabs.org/patch/649211/

So, let me just change the order in next version ?
Signed-off-by: Scott Wood <oss@buserror.net>
Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
No.  This isn't my patch so my signoff shouldn't be on it.
[Lu Yangbo-B47093] Ok, will remove it.

quoted
[Lu Yangbo-B47093] It's a good idea to move die into .family I think.
In my opinion, it's better to keep svr and name in soc_id just like
your suggestion above.
quoted
	{
		.soc_id = "svr:0x85490010,name:T1023E,",
		.family = "QorIQ T1024",
	}
The user probably don’t like to learn the svr value. What they want is
just to match the soc they use.
It's convenient to use name+rev for them to match a soc.
What the user should want 99% of the time is to match the die (plus
revision), not the soc.

quoted
Regarding shrinking the table, I think it's hard to use svr+mask.
Because I find many platforms use different masks.
We couldn’t know the mask according svr value.
The mask would be part of the table:

{
	{
		.die = "T1024",
		.svr = 0x85400000,
		.mask = 0xfff00000,
	},
	{
		.die = "T1040",
		.svr = 0x85200000,
		.mask = 0xfff00000,
	},
	{
		.die = "LS1088A",
		.svr = 0x87030000,
		.mask = 0xffff0000,
	},
	...
}

There's a small risk that we get the mask wrong and a different die is
created that matches an existing table, but it doesn't seem too likely,
and can easily be fixed with a kernel update if it happens.
[Lu Yangbo-B47093] You mean we will not define soc device attribute for each soc and we will define attribute for each die instead, right?
If so, when we want to match a specific soc we need to use its svr value in code. If it's acceptable, I can try in next version.

BTW, aren't ls2080a and ls2085a the same die?  And is there no non-E
version of LS2080A/LS2040A?
[Lu Yangbo-B47093] I checked all the svr values in chip errata doc "Revision level to part marking cross-reference" table.
I found ls2080a and ls2085a were in two separate doc. And I didn’t find non-E version of LS2080A/LS2040A in chip errata doc.
Do you know is there any other doc we can confirm this?

quoted
quoted
quoted
+	do {
+		if (!matches->soc_id)
+			return NULL;
+		if (glob_match(svr_match, matches->soc_id))
+			break;
+	} while (matches++);
Are you expecting "matches++" to ever evaluate as false?
[Lu Yangbo-B47093] Yes, this is used to match the soc we use in
qoriq_soc array until getting true.
We need to get the name and die information defined in array.
I'm not asking whether the glob_match will ever return true.  I'm saying
that "matches++" will never become NULL.
[Lu Yangbo-B47093] The matches++ will never become NULL while it will return NULL after matching for all the members in array.

quoted
quoted
quoted
+	/* Register soc device */
+	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
+	if (!soc_dev_attr) {
+		ret = -ENOMEM;
+		goto out_unmap;
+	}
Couldn't this be statically allocated?
[Lu Yangbo-B47093] Do you mean we define this struct statically ?

static struct soc_device_attribute soc_dev_attr;
Yes.
[Lu Yangbo-B47093] It's ok to define it statically. Is there any need to do that?

quoted
quoted
quoted
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		ret = -ENODEV;
Why are you changing the error code?
[Lu Yangbo-B47093] What error code should we use ? :)
ret = PTR_ERR(soc_dev);
[Lu Yangbo-B47093] Ok.. will do that.

+	}
quoted
quoted
quoted
+	return 0;
+out:
+	kfree(soc_dev_attr->machine);
+	kfree(soc_dev_attr->family);
+	kfree(soc_dev_attr->soc_id);
+	kfree(soc_dev_attr->revision);
+	kfree(soc_dev_attr);
+out_unmap:
+	iounmap(guts->regs);
+out_free:
+	kfree(guts);
devm
[Lu Yangbo-B47093] What's the devm meaning here :)
If you allocate these with devm_kzalloc(), devm_kasprintf(),
devm_kstrdup(), etc. then they will be freed automatically when the
device is unbound.

quoted
quoted
quoted
+static int fsl_guts_remove(struct platform_device *dev) {
+	kfree(soc_dev_attr->machine);
+	kfree(soc_dev_attr->family);
+	kfree(soc_dev_attr->soc_id);
+	kfree(soc_dev_attr->revision);
+	kfree(soc_dev_attr);
+	soc_device_unregister(soc_dev);
+	iounmap(guts->regs);
+	kfree(guts);
+	return 0;
+}
Don't free the memory before you unregister the device that uses it
(moot if you use devm).
[Lu Yangbo-B47093] The soc.c driver mentions that.
Ensure soc_dev->attr is freed prior to calling soc_device_unregister.
That comment is wrong.  Freeing the memory first creates a race condition
that could result in accessing freed memory, if something accesses the
soc device in parallel with unbinding.
[Lu Yangbo-B47093] Ok, will unregister the device first. Thanks.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org More majordomo info at
http://vger.kernel.org/majordomo-info.html
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
  



    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help