* Re: [PATCH] powerpc/sysdev: Fix a mpic section mismatch for MPC85xx
From: Christian Engelmayer @ 2013-12-19 23:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Scott Wood; +Cc: linuxppc-dev, Kevin Hao
In-Reply-To: <1387152653.15730.175.camel@pasglop>
On Mon, 16 Dec 2013 11:10:53 +1100 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Sun, 2013-12-15 at 19:38 +0100, Christian Engelmayer wrote:
> > Moved arch/powerpc/sysdev/mpic.c : smp_mpic_probe() out of the __init section.
> > It is referenced by arch/powerpc/platforms/85xx/smp.c : smp_85xx_setup_cpu().
>
> I don't like this. The reference is not actually going to call into the
> code at all and as such is not an error, it's just a pointer comparison.
That's correct. I proposed it that way because on first sight I was concerned
that there is an address of an __init function assigned to a function pointer
within a non __initdata struct at all that can be compared against. However,
further usage of smp_ops->probe is currently safe of course and *_ops symbols
within .data are whitelisted to refer to init sections.
> If there is no way to silence the warning, then I'd suggest to use a
> global flag, something like mpc85xx_pic_type and test that instead
> of comparing the pointers.
I've seen that there is currently a patch proposed against
commit dc2c9c52b604f51b1416ed87ff54a1c77a1a8b5b
powerpc/85xx: Set up doorbells even with no mpic
that introduced the section causing the warning:
http://patchwork.ozlabs.org/patch/289214/
powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support
This patch also removes the affected pointer comparison and if accepted would
thus also silence this warning.
> > Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
> > ---
> > arch/powerpc/sysdev/mpic.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> > index 0e166ed..72c1e65 100644
> > --- a/arch/powerpc/sysdev/mpic.c
> > +++ b/arch/powerpc/sysdev/mpic.c
> > @@ -1924,7 +1924,7 @@ void smp_mpic_message_pass(int cpu, int msg)
> > msg * MPIC_INFO(CPU_IPI_DISPATCH_STRIDE), physmask);
> > }
> >
> > -int __init smp_mpic_probe(void)
> > +int smp_mpic_probe(void)
> > {
> > int nr_cpus;
> >
>
>
^ permalink raw reply
* [PATCH] powernv: eeh: fix possible buffer overrun in ioda_eeh_phb_diag()
From: Brian W Hart @ 2013-12-19 23:14 UTC (permalink / raw)
To: linuxppc-dev
PHB diagnostic buffer may be smaller than PAGE_SIZE, especially when
PAGE_SIZE > 4KB.
Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
---
arch/powerpc/platforms/powernv/eeh-ioda.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 02245ce..8184ef5 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -820,14 +820,15 @@ static void ioda_eeh_phb_diag(struct pci_controller *hose)
struct OpalIoPhbErrorCommon *common;
long rc;
- common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
- rc = opal_pci_get_phb_diag_data2(phb->opal_id, common, PAGE_SIZE);
+ rc = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
+ PNV_PCI_DIAG_BUF_SIZE);
if (rc != OPAL_SUCCESS) {
pr_warning("%s: Failed to get diag-data for PHB#%x (%ld)\n",
__func__, hose->global_number, rc);
return;
}
+ common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
switch (common->ioType) {
case OPAL_PHB_ERROR_DATA_TYPE_P7IOC:
ioda_eeh_p7ioc_phb_diag(hose, common);
--
1.8.3.1
^ permalink raw reply related
* [PATCH] powernv: eeh: add buffer for P7IOC hub error data
From: Brian W Hart @ 2013-12-19 23:18 UTC (permalink / raw)
To: linuxppc-dev
Prevent ioda_eeh_hub_diag() from clobbering itself when called by supplying
a buffer for P7IOC hub diagnostic data. Take care to inform OPAL of the
correct size for the buffer.
Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
---
I hope I've understood this correctly. It looks to me like
ioda_eeh_hub_data is effectively asking OPAL to clobber its own
text (via 'data') when it makes the call to retrieve the hub data.
Added a hub diagnostic structure per-phb. Perhaps the diagnostic
structure better belongs in the phb->diag union, but I wasn't sure whether
we'd need to carry the hub and PHB diag data at the same time.
arch/powerpc/platforms/powernv/eeh-ioda.c | 4 ++--
arch/powerpc/platforms/powernv/pci.h | 2 ++
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 8184ef5..dfd9134 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -636,8 +636,8 @@ static void ioda_eeh_hub_diag(struct pci_controller *hose)
struct OpalIoP7IOCErrorData *data;
long rc;
- data = (struct OpalIoP7IOCErrorData *)ioda_eeh_hub_diag;
- rc = opal_pci_get_hub_diag_data(phb->hub_id, data, PAGE_SIZE);
+ data = (struct OpalIoP7IOCErrorData *)&phb->p7ioc_err;
+ rc = opal_pci_get_hub_diag_data(phb->hub_id, data, sizeof *data);
if (rc != OPAL_SUCCESS) {
pr_warning("%s: Failed to get HUB#%llx diag-data (%ld)\n",
__func__, phb->hub_id, rc);
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 911c24e..d1b6d8c 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -177,6 +177,8 @@ struct pnv_phb {
unsigned char blob[PNV_PCI_DIAG_BUF_SIZE];
struct OpalIoP7IOCPhbErrorData p7ioc;
} diag;
+
+ struct OpalIoP7IOCErrorData p7ioc_err;
};
extern struct pci_ops pnv_pci_ops;
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] powernv: eeh: fix possible buffer overrun in ioda_eeh_phb_diag()
From: Gavin Shan @ 2013-12-20 1:35 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20131219231407.GA22418@oc3347516403.ibm.com>
On Thu, Dec 19, 2013 at 05:14:07PM -0600, Brian W Hart wrote:
>PHB diagnostic buffer may be smaller than PAGE_SIZE, especially when
>PAGE_SIZE > 4KB.
>
I think you're talking about that PAGE_SIZE could be configured
to have variable size (e.g. 4KB). So it's not safe to pass PAGE_SIZE
to OPAL API opal_pci_get_phb_diag_data2(). Instead, we should pass
PNV_PCI_DIAG_BUF_SIZE and it makes sense to me :-)
Also, it needs to be backported to stable kernel as well.
>Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
Acked-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>---
> arch/powerpc/platforms/powernv/eeh-ioda.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>index 02245ce..8184ef5 100644
>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>@@ -820,14 +820,15 @@ static void ioda_eeh_phb_diag(struct pci_controller *hose)
> struct OpalIoPhbErrorCommon *common;
> long rc;
>
>- common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
>- rc = opal_pci_get_phb_diag_data2(phb->opal_id, common, PAGE_SIZE);
>+ rc = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
>+ PNV_PCI_DIAG_BUF_SIZE);
> if (rc != OPAL_SUCCESS) {
> pr_warning("%s: Failed to get diag-data for PHB#%x (%ld)\n",
> __func__, hose->global_number, rc);
> return;
> }
>
>+ common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
> switch (common->ioType) {
> case OPAL_PHB_ERROR_DATA_TYPE_P7IOC:
> ioda_eeh_p7ioc_phb_diag(hose, common);
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH] powernv: eeh: add buffer for P7IOC hub error data
From: Gavin Shan @ 2013-12-20 1:45 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20131219231853.GB22418@oc3347516403.ibm.com>
On Thu, Dec 19, 2013 at 05:18:53PM -0600, Brian W Hart wrote:
>Prevent ioda_eeh_hub_diag() from clobbering itself when called by supplying
>a buffer for P7IOC hub diagnostic data. Take care to inform OPAL of the
>correct size for the buffer.
>
>Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
>---
>
>I hope I've understood this correctly. It looks to me like
>ioda_eeh_hub_data is effectively asking OPAL to clobber its own
>text (via 'data') when it makes the call to retrieve the hub data.
>
Yeah, we should have used following variable as HUB diag-data instead.
static char *hub_diag = NULL;
However, it's not safe to allocate page-sized buffer for "hub_diag".
>Added a hub diagnostic structure per-phb. Perhaps the diagnostic
>structure better belongs in the phb->diag union, but I wasn't sure whether
>we'd need to carry the hub and PHB diag data at the same time.
>
Please put hub diag-data to struct pnv_phb::diag since we don't need
carry hub and PHB diag-data at same time. With it, please remove
variable "hub_diag" as well.
> arch/powerpc/platforms/powernv/eeh-ioda.c | 4 ++--
> arch/powerpc/platforms/powernv/pci.h | 2 ++
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>index 8184ef5..dfd9134 100644
>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>@@ -636,8 +636,8 @@ static void ioda_eeh_hub_diag(struct pci_controller *hose)
> struct OpalIoP7IOCErrorData *data;
> long rc;
>
>- data = (struct OpalIoP7IOCErrorData *)ioda_eeh_hub_diag;
>- rc = opal_pci_get_hub_diag_data(phb->hub_id, data, PAGE_SIZE);
>+ data = (struct OpalIoP7IOCErrorData *)&phb->p7ioc_err;
>+ rc = opal_pci_get_hub_diag_data(phb->hub_id, data, sizeof *data);
> if (rc != OPAL_SUCCESS) {
> pr_warning("%s: Failed to get HUB#%llx diag-data (%ld)\n",
> __func__, phb->hub_id, rc);
>diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
>index 911c24e..d1b6d8c 100644
>--- a/arch/powerpc/platforms/powernv/pci.h
>+++ b/arch/powerpc/platforms/powernv/pci.h
>@@ -177,6 +177,8 @@ struct pnv_phb {
> unsigned char blob[PNV_PCI_DIAG_BUF_SIZE];
> struct OpalIoP7IOCPhbErrorData p7ioc;
> } diag;
>+
>+ struct OpalIoP7IOCErrorData p7ioc_err;
> };
>
> extern struct pci_ops pnv_pci_ops;
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH] powernv: eeh: fix possible buffer overrun in ioda_eeh_phb_diag()
From: Gavin Shan @ 2013-12-20 1:59 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev
In-Reply-To: <20131220013539.GA10795@shangw.(null)>
On Fri, Dec 20, 2013 at 09:35:39AM +0800, Gavin Shan wrote:
>On Thu, Dec 19, 2013 at 05:14:07PM -0600, Brian W Hart wrote:
>>PHB diagnostic buffer may be smaller than PAGE_SIZE, especially when
>>PAGE_SIZE > 4KB.
>>
>
>I think you're talking about that PAGE_SIZE could be configured
>to have variable size (e.g. 4KB). So it's not safe to pass PAGE_SIZE
>to OPAL API opal_pci_get_phb_diag_data2(). Instead, we should pass
>PNV_PCI_DIAG_BUF_SIZE and it makes sense to me :-)
>
>Also, it needs to be backported to stable kernel as well.
>
>>Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
>
>Acked-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>
Sorry, Brian. It has been fixed as part of the following commit, which
has been put into Ben's powerpc-next branch :-)
commit 93aef2a789778e7ec787179fc9b34ca4885a5ef3
161 static void ioda_eeh_phb_diag(struct pci_controller *hose)
162 {
163 struct pnv_phb *phb = hose->private_data;
164 - struct OpalIoPhbErrorCommon *common;
165 long rc;
166
167 - common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
168 - rc = opal_pci_get_phb_diag_data2(phb->opal_id, common, PAGE_SIZE);
169 + rc = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
170 + PNV_PCI_DIAG_BUF_SIZE);
>>---
>> arch/powerpc/platforms/powernv/eeh-ioda.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>index 02245ce..8184ef5 100644
>>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>@@ -820,14 +820,15 @@ static void ioda_eeh_phb_diag(struct pci_controller *hose)
>> struct OpalIoPhbErrorCommon *common;
>> long rc;
>>
>>- common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
>>- rc = opal_pci_get_phb_diag_data2(phb->opal_id, common, PAGE_SIZE);
>>+ rc = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
>>+ PNV_PCI_DIAG_BUF_SIZE);
>> if (rc != OPAL_SUCCESS) {
>> pr_warning("%s: Failed to get diag-data for PHB#%x (%ld)\n",
>> __func__, hose->global_number, rc);
>> return;
>> }
>>
>>+ common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
>> switch (common->ioType) {
>> case OPAL_PHB_ERROR_DATA_TYPE_P7IOC:
>> ioda_eeh_p7ioc_phb_diag(hose, common);
>
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH] powerpc: book3s: kvm: Use the saved dsisr and dar values
From: Aneesh Kumar K.V @ 2013-12-20 4:37 UTC (permalink / raw)
To: Alexander Graf
Cc: Paul Mackerras, linuxppc-dev@lists.ozlabs.org,
kvm-ppc@vger.kernel.org, kvm@vger.kernel.org mailing list
In-Reply-To: <CA6C7C49-C1EC-4277-9777-650026C9127D@suse.de>
Alexander Graf <agraf@suse.de> writes:
>> Am 19.12.2013 um 08:02 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>:
>>
>> Alexander Graf <agraf@suse.de> writes:
>>
>>>> On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>>>>
>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>>
>>>> Don't try to compute these values.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>>> ---
>>>>
>>>> NOTE: I am not sure why we were originally computing dsisr and dar. So may be
>>>> we need a variant of this patch. But with this and the additional patch
>>>> "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a Little Endian
>>>> PR guest to boot.
>>>
>>> It's quite easy to find out - git blame tells you all the history and points you to commit ca7f4203b.
>>>
>>> commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
>>> Author: Alexander Graf <agraf@suse.de>
>>> Date: Wed Mar 24 21:48:28 2010 +0100
>>>
>>> KVM: PPC: Implement alignment interrupt
>>>
>>> Mac OS X has some applications - namely the Finder - that require alignment
>>> interrupts to work properly. So we need to implement them.
>>>
>>> But the spec for 970 and 750 also looks different. While 750 requires the
>>> DSISR and DAR fields to reflect some instruction bits (DSISR) and the fault
>>> address (DAR), the 970 declares this as an optional feature. So we need
>>> to reconstruct DSISR and DAR manually.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>>
>>> Read this as "on 970, alignment interrupts don't give us DSISR and DAR of the faulting instruction" as otherwise I wouldn't have implemented it.
>>>
>>> So this is clearly a nack on this patch :).
>>
>> I can possibly do a if (cpu_has_feature(CPU_FTR_ARCH_201)). But do we need
>> to do that ? According to Paul we should always find DAR.
>
> Paul only mentioned DAR, not DSISR. Please verify whether 970 gives us a proper DAR value - we can then remove that part.
>
> But for DSISR I'm not convinced CPUs above 970 handle this
> correctly. So we would at least need a guest cpu check to find out
> whether the vcpu expects a working dsisr and emulate it then.
>
> I don't really fully understand the problem though. Why does the
> calculation break at all for you?
IIRC this was to get little endian PR setup to work. This is to avoid
handling new instructions, because in little endian mode we get
alignment interrupt for a larger instructon set
-aneesh
^ permalink raw reply
* [PATCH V4] powerpc/kernel/sysfs: cleanup set up macros for PMC/non-PMC SPRs
From: Madhavan Srinivasan @ 2013-12-20 5:01 UTC (permalink / raw)
To: benh; +Cc: michael, olof, Madhavan Srinivasan, linuxppc-dev
Currently PMC (Performance Monitor Counter) setup macros are used
for other SPRs. Since not all SPRs are PMC related, this patch
modifies the exisiting macro and uses it to setup both PMC and
non PMC SPRs accordingly.
V4 changes:
1) No logic changes, just rebase with upstream.
V3 changes:
1) No logic change, just renamed generic macro and removed #define for
empty string
2) Changes in the comment to explain better.
V2 changes:
1) Modified SYSFS_PMCSETUP to a generic macro with additional parameter
2) Added PMC and SPR macro to call the generic macro
3) Changes in the comment to explain better.
Acked-by: Michael Ellerman <michael@ellerman.id.au>
Acked-by: Olof Johansson <olof@lixom.net>
Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
arch/powerpc/kernel/sysfs.c | 72 +++++++++++++++++++++++--------------------
1 file changed, 38 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index b4e6676..cad777e 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -108,14 +108,14 @@ void ppc_enable_pmcs(void)
}
EXPORT_SYMBOL(ppc_enable_pmcs);
-#define SYSFS_PMCSETUP(NAME, ADDRESS) \
+#define __SYSFS_SPRSETUP(NAME, ADDRESS, EXTRA) \
static void read_##NAME(void *val) \
{ \
*(unsigned long *)val = mfspr(ADDRESS); \
} \
static void write_##NAME(void *val) \
{ \
- ppc_enable_pmcs(); \
+ EXTRA; \
mtspr(ADDRESS, *(unsigned long *)val); \
} \
static ssize_t show_##NAME(struct device *dev, \
@@ -140,6 +140,10 @@ static ssize_t __used \
return count; \
}
+#define SYSFS_PMCSETUP(NAME, ADDRESS) \
+ __SYSFS_SPRSETUP(NAME, ADDRESS, ppc_enable_pmcs())
+#define SYSFS_SPRSETUP(NAME, ADDRESS) \
+ __SYSFS_SPRSETUP(NAME, ADDRESS, )
/* Let's define all possible registers, we'll only hook up the ones
* that are implemented on the current processor
@@ -175,10 +179,10 @@ SYSFS_PMCSETUP(pmc7, SPRN_PMC7);
SYSFS_PMCSETUP(pmc8, SPRN_PMC8);
SYSFS_PMCSETUP(mmcra, SPRN_MMCRA);
-SYSFS_PMCSETUP(purr, SPRN_PURR);
-SYSFS_PMCSETUP(spurr, SPRN_SPURR);
-SYSFS_PMCSETUP(dscr, SPRN_DSCR);
-SYSFS_PMCSETUP(pir, SPRN_PIR);
+SYSFS_SPRSETUP(purr, SPRN_PURR);
+SYSFS_SPRSETUP(spurr, SPRN_SPURR);
+SYSFS_SPRSETUP(dscr, SPRN_DSCR);
+SYSFS_SPRSETUP(pir, SPRN_PIR);
/*
Lets only enable read for phyp resources and
@@ -249,34 +253,34 @@ SYSFS_PMCSETUP(pa6t_pmc3, SPRN_PA6T_PMC3);
SYSFS_PMCSETUP(pa6t_pmc4, SPRN_PA6T_PMC4);
SYSFS_PMCSETUP(pa6t_pmc5, SPRN_PA6T_PMC5);
#ifdef CONFIG_DEBUG_KERNEL
-SYSFS_PMCSETUP(hid0, SPRN_HID0);
-SYSFS_PMCSETUP(hid1, SPRN_HID1);
-SYSFS_PMCSETUP(hid4, SPRN_HID4);
-SYSFS_PMCSETUP(hid5, SPRN_HID5);
-SYSFS_PMCSETUP(ima0, SPRN_PA6T_IMA0);
-SYSFS_PMCSETUP(ima1, SPRN_PA6T_IMA1);
-SYSFS_PMCSETUP(ima2, SPRN_PA6T_IMA2);
-SYSFS_PMCSETUP(ima3, SPRN_PA6T_IMA3);
-SYSFS_PMCSETUP(ima4, SPRN_PA6T_IMA4);
-SYSFS_PMCSETUP(ima5, SPRN_PA6T_IMA5);
-SYSFS_PMCSETUP(ima6, SPRN_PA6T_IMA6);
-SYSFS_PMCSETUP(ima7, SPRN_PA6T_IMA7);
-SYSFS_PMCSETUP(ima8, SPRN_PA6T_IMA8);
-SYSFS_PMCSETUP(ima9, SPRN_PA6T_IMA9);
-SYSFS_PMCSETUP(imaat, SPRN_PA6T_IMAAT);
-SYSFS_PMCSETUP(btcr, SPRN_PA6T_BTCR);
-SYSFS_PMCSETUP(pccr, SPRN_PA6T_PCCR);
-SYSFS_PMCSETUP(rpccr, SPRN_PA6T_RPCCR);
-SYSFS_PMCSETUP(der, SPRN_PA6T_DER);
-SYSFS_PMCSETUP(mer, SPRN_PA6T_MER);
-SYSFS_PMCSETUP(ber, SPRN_PA6T_BER);
-SYSFS_PMCSETUP(ier, SPRN_PA6T_IER);
-SYSFS_PMCSETUP(sier, SPRN_PA6T_SIER);
-SYSFS_PMCSETUP(siar, SPRN_PA6T_SIAR);
-SYSFS_PMCSETUP(tsr0, SPRN_PA6T_TSR0);
-SYSFS_PMCSETUP(tsr1, SPRN_PA6T_TSR1);
-SYSFS_PMCSETUP(tsr2, SPRN_PA6T_TSR2);
-SYSFS_PMCSETUP(tsr3, SPRN_PA6T_TSR3);
+SYSFS_SPRSETUP(hid0, SPRN_HID0);
+SYSFS_SPRSETUP(hid1, SPRN_HID1);
+SYSFS_SPRSETUP(hid4, SPRN_HID4);
+SYSFS_SPRSETUP(hid5, SPRN_HID5);
+SYSFS_SPRSETUP(ima0, SPRN_PA6T_IMA0);
+SYSFS_SPRSETUP(ima1, SPRN_PA6T_IMA1);
+SYSFS_SPRSETUP(ima2, SPRN_PA6T_IMA2);
+SYSFS_SPRSETUP(ima3, SPRN_PA6T_IMA3);
+SYSFS_SPRSETUP(ima4, SPRN_PA6T_IMA4);
+SYSFS_SPRSETUP(ima5, SPRN_PA6T_IMA5);
+SYSFS_SPRSETUP(ima6, SPRN_PA6T_IMA6);
+SYSFS_SPRSETUP(ima7, SPRN_PA6T_IMA7);
+SYSFS_SPRSETUP(ima8, SPRN_PA6T_IMA8);
+SYSFS_SPRSETUP(ima9, SPRN_PA6T_IMA9);
+SYSFS_SPRSETUP(imaat, SPRN_PA6T_IMAAT);
+SYSFS_SPRSETUP(btcr, SPRN_PA6T_BTCR);
+SYSFS_SPRSETUP(pccr, SPRN_PA6T_PCCR);
+SYSFS_SPRSETUP(rpccr, SPRN_PA6T_RPCCR);
+SYSFS_SPRSETUP(der, SPRN_PA6T_DER);
+SYSFS_SPRSETUP(mer, SPRN_PA6T_MER);
+SYSFS_SPRSETUP(ber, SPRN_PA6T_BER);
+SYSFS_SPRSETUP(ier, SPRN_PA6T_IER);
+SYSFS_SPRSETUP(sier, SPRN_PA6T_SIER);
+SYSFS_SPRSETUP(siar, SPRN_PA6T_SIAR);
+SYSFS_SPRSETUP(tsr0, SPRN_PA6T_TSR0);
+SYSFS_SPRSETUP(tsr1, SPRN_PA6T_TSR1);
+SYSFS_SPRSETUP(tsr2, SPRN_PA6T_TSR2);
+SYSFS_SPRSETUP(tsr3, SPRN_PA6T_TSR3);
#endif /* CONFIG_DEBUG_KERNEL */
#endif /* HAS_PPC_PMC_PA6T */
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] powerpc: book3s: kvm: Use the saved dsisr and dar values
From: Alexander Graf @ 2013-12-20 6:38 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Paul Mackerras, linuxppc-dev@lists.ozlabs.org,
kvm-ppc@vger.kernel.org, kvm@vger.kernel.org mailing list
In-Reply-To: <8738lo9lzg.fsf@linux.vnet.ibm.com>
> Am 20.12.2013 um 05:37 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet=
.ibm.com>:
>=20
> Alexander Graf <agraf@suse.de> writes:
>=20
>>> Am 19.12.2013 um 08:02 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vn=
et.ibm.com>:
>>>=20
>>> Alexander Graf <agraf@suse.de> writes:
>>>=20
>>>>> On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm=
.com> wrote:
>>>>>=20
>>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>>>=20
>>>>> Don't try to compute these values.
>>>>>=20
>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>>>> ---
>>>>>=20
>>>>> NOTE: I am not sure why we were originally computing dsisr and dar. So=
may be
>>>>> we need a variant of this patch. But with this and the additional patc=
h
>>>>> "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a=
Little Endian
>>>>> PR guest to boot.
>>>>=20
>>>> It's quite easy to find out - git blame tells you all the history and p=
oints you to commit ca7f4203b.
>>>>=20
>>>> commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
>>>> Author: Alexander Graf <agraf@suse.de>
>>>> Date: Wed Mar 24 21:48:28 2010 +0100
>>>>=20
>>>> KVM: PPC: Implement alignment interrupt
>>>>=20
>>>> Mac OS X has some applications - namely the Finder - that require ali=
gnment
>>>> interrupts to work properly. So we need to implement them.
>>>>=20
>>>> But the spec for 970 and 750 also looks different. While 750 requires=
the
>>>> DSISR and DAR fields to reflect some instruction bits (DSISR) and the=
fault
>>>> address (DAR), the 970 declares this as an optional feature. So we ne=
ed
>>>> to reconstruct DSISR and DAR manually.
>>>>=20
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>>>=20
>>>> Read this as "on 970, alignment interrupts don't give us DSISR and DAR o=
f the faulting instruction" as otherwise I wouldn't have implemented it.
>>>>=20
>>>> So this is clearly a nack on this patch :).
>>>=20
>>> I can possibly do a if (cpu_has_feature(CPU_FTR_ARCH_201)). But do we ne=
ed
>>> to do that ? According to Paul we should always find DAR.
>>=20
>> Paul only mentioned DAR, not DSISR. Please verify whether 970 gives us a p=
roper DAR value - we can then remove that part.
>>=20
>> But for DSISR I'm not convinced CPUs above 970 handle this
>> correctly. So we would at least need a guest cpu check to find out
>> whether the vcpu expects a working dsisr and emulate it then.
>=20
>>=20
>> I don't really fully understand the problem though. Why does the
>> calculation break at all for you?
>=20
>=20
> IIRC this was to get little endian PR setup to work. This is to avoid
> handling new instructions, because in little endian mode we get
> alignment interrupt for a larger instructon set
Ok, please limit dar/dsisr calculation to book3s_32 vcpus then :).
Alex
>=20
> -aneesh
>=20
^ permalink raw reply
* RE: [PATCH 4/5] powerpc/fsl-booke: Add initial T208x QDS board support
From: Shengzhou.Liu @ 2013-12-20 6:42 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1387310222.10013.439.camel@snotra.buserror.net>
DQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IFdvb2QgU2NvdHQtQjA3NDIx
DQo+IFNlbnQ6IFdlZG5lc2RheSwgRGVjZW1iZXIgMTgsIDIwMTMgMzo1NyBBTQ0KPiBUbzogTGl1
IFNoZW5nemhvdS1CMzY2ODUNCj4gQ2M6IGxpbnV4cHBjLWRldkBsaXN0cy5vemxhYnMub3JnDQo+
IFN1YmplY3Q6IFJlOiBbUEFUQ0ggNC81XSBwb3dlcnBjL2ZzbC1ib29rZTogQWRkIGluaXRpYWwg
VDIwOHggUURTIGJvYXJkDQo+IHN1cHBvcnQNCj4gDQo+IE9uIFdlZCwgMjAxMy0xMi0xMSBhdCAx
OToxOSArMDgwMCwgU2hlbmd6aG91IExpdSB3cm90ZToNCj4gPiArCQlib2FyZGN0cmw6IGJvYXJk
LWNvbnRyb2xAMywwIHsNCj4gPiArCQkJI2FkZHJlc3MtY2VsbHMgPSA8MT47DQo+ID4gKwkJCSNz
aXplLWNlbGxzID0gPDE+Ow0KPiA+ICsJCQljb21wYXRpYmxlID0gImZzbCxmcGdhLXFpeGlzIjsN
Cj4gPiArCQkJcmVnID0gPDMgMCAweDMwMD47DQo+ID4gKwkJCXJhbmdlcyA9IDwwIDMgMCAweDMw
MD47DQo+ID4gKwkJfTsNCj4gDQo+IFdoeSBkbyB5b3UgaGF2ZSByYW5nZXMgYW5kICNhZGRyZXNz
LWNlbGxzLyNzaXplLWNlbGxzIGhlcmU/ICBXaGVuIHdvdWxkDQo+IHRoZXJlIGV2ZXIgYmUgYSBj
aGlsZCBub2RlPw0KDQpbU2hlbmd6aG91XSBUaGVyZSBhcmUgbXVsdGlwbGUgY2hpbGQgbm9kZXMg
Zm9yIHRoaXMoaW4gYSBzZXBhcmF0ZSBEUEFBLXJlbGF0ZWQgcGF0Y2gsIGJ1dCB3aG9sZSBEUEFB
IG1vZHVsZSBoYXMgbm90IGJlZW4gdXBzdHJlYW1lZCB5ZXQgc28gZmFyKQ0KDQo+IA0KPiA+ICsJ
fTsNCj4gPiArDQo+ID4gKwltZW1vcnkgew0KPiA+ICsJCWRldmljZV90eXBlID0gIm1lbW9yeSI7
DQo+ID4gKwl9Ow0KPiA+ICsNCj4gPiArCWRjc3I6IGRjc3JAZjAwMDAwMDAwIHsNCj4gPiArCQly
YW5nZXMgPSA8MHgwMDAwMDAwMCAweGYgMHgwMDAwMDAwMCAweDAxMDcyMDAwPjsNCj4gPiArCX07
DQo+ID4gKw0KPiA+ICsJc29jOiBzb2NAZmZlMDAwMDAwIHsNCj4gPiArCQlyYW5nZXMgPSA8MHgw
MDAwMDAwMCAweGYgMHhmZTAwMDAwMCAweDEwMDAwMDA+Ow0KPiA+ICsJCXJlZyA9IDwweGYgMHhm
ZTAwMDAwMCAwIDB4MDAwMDEwMDA+Ow0KPiA+ICsJCXNwaUAxMTAwMDAgew0KPiA+ICsJCQlmbGFz
aEAwIHsNCj4gPiArCQkJCSNhZGRyZXNzLWNlbGxzID0gPDE+Ow0KPiA+ICsJCQkJI3NpemUtY2Vs
bHMgPSA8MT47DQo+ID4gKwkJCQljb21wYXRpYmxlID0gInNwYW5zaW9uLHMyNXNsMTI4MDEiOw0K
PiA+ICsJCQkJcmVnID0gPDA+Ow0KPiA+ICsJCQkJc3BpLW1heC1mcmVxdWVuY3kgPSA8NDAwMDAw
MDA+OyAvKiBpbnB1dCBjbG9jayAqLw0KPiA+ICsJCQkJcGFydGl0aW9uQHUtYm9vdCB7DQo+ID4g
KwkJCQkJbGFiZWwgPSAiU1BJIFUtQm9vdCI7DQo+ID4gKwkJCQkJcmVnID0gPDB4MDAwMDAwMDAg
MHgwMDEwMDAwMD47DQo+ID4gKwkJCQkJcmVhZC1vbmx5Ow0KPiA+ICsJCQkJfTsNCj4gPiArCQkJ
CXBhcnRpdGlvbkBrZXJuZWwgew0KPiA+ICsJCQkJCWxhYmVsID0gIlNQSSBLZXJuZWwiOw0KPiA+
ICsJCQkJCXJlZyA9IDwweDAwMTAwMDAwIDB4MDA1MDAwMDA+Ow0KPiA+ICsJCQkJCXJlYWQtb25s
eTsNCj4gPiArCQkJCX07DQo+ID4gKwkJCQlwYXJ0aXRpb25AZHRiIHsNCj4gPiArCQkJCQlsYWJl
bCA9ICJTUEkgRFRCIjsNCj4gPiArCQkJCQlyZWcgPSA8MHgwMDYwMDAwMCAweDAwMTAwMDAwPjsN
Cj4gPiArCQkJCQlyZWFkLW9ubHk7DQo+ID4gKwkJCQl9Ow0KPiA+ICsJCQkJcGFydGl0aW9uQGZz
IHsNCj4gPiArCQkJCQlsYWJlbCA9ICJTUEkgRmlsZSBTeXN0ZW0iOw0KPiA+ICsJCQkJCXJlZyA9
IDwweDAwNzAwMDAwIDB4MDA5MDAwMDA+Ow0KPiA+ICsJCQkJfTsNCj4gDQo+IFRoZXNlIGFyZSBu
b3QgdmFsaWQgdW5pdCBhZGRyZXNzZXMgLS0gYW5kIHRoZSBrZXJuZWwvZHRiIHNob3VsZCBub3Qg
YmUgcmVhZC1vbmx5LiAgDQo+IERvIHlvdSByZWFsbHkgd2FudCB0byBkZWRpY2F0ZSBhIHdob2xl
IG1lYmlieXRlIHRvIHRoZSBkdGIsIGdpdmVuIHRoYXQNCj4gdGhlIGZsYXNoIGlzIG9ubHkgMTYg
TWlCIHRvdGFsPw0KPiANCj4gQWN0dWFsbHksIGxldCdzIHBsZWFzZSBqdXN0IHN0b3AgcHV0dGlu
ZyBwYXJ0aXRpb25zIGluIHRoZSBkdHMuICBFaXRoZXINCj4gdXNlIG10ZHBhcnRzIG9uIHRoZSBj
b21tYW5kIGxpbmUsIG9yIGhhdmUgVS1Cb290IGZpbGwgaW4gdGhlIHBhcnRpdGlvbg0KPiBpbmZv
ICh0aGVyZSBpcyBjb2RlIGluIFUtQm9vdCB0byBkbyB0aGlzIGJhc2VkIG9uIHRoZSBtdGRwYXJ0
cyBlbnYNCj4gdmFyaWFibGUpLg0KPiANCiANCltTaGVuZ3pob3VdIG9rYXksIHdpbGwgdXNlIG10
ZHBhcnRzIGluc3RlYWQgb2YgdGhpcyB3YXkuDQoNCg==
^ permalink raw reply
* Re: [v3, 3/7] powerpc: enable the relocatable support for the fsl booke 32bit kernel
From: Kevin Hao @ 2013-12-20 7:43 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
In-Reply-To: <20131218234825.GA6959@home.buserror.net>
[-- Attachment #1: Type: text/plain, Size: 2568 bytes --]
On Wed, Dec 18, 2013 at 05:48:25PM -0600, Scott Wood wrote:
> On Wed, Aug 07, 2013 at 09:18:31AM +0800, Kevin Hao wrote:
> > This is based on the codes in the head_44x.S. The difference is that
> > the init tlb size we used is 64M. With this patch we can only load the
> > kernel at address between memstart_addr ~ memstart_addr + 64M. We will
> > fix this restriction in the following patches.
>
> Which following patch fixes the restriction? With all seven patches
> applied, I was still only successful booting within 64M of memstart_addr.
There is bug in this patch series when booting above the 64M. It seems
that I missed to test this previously. Sorry for that. With the following
change I can boot the kernel at 0x5000000.
diff --git a/arch/powerpc/mm/fsl_booke_mmu.c b/arch/powerpc/mm/fsl_booke_mmu.c
index 048d716ae706..ce0c7d7db6c3 100644
--- a/arch/powerpc/mm/fsl_booke_mmu.c
+++ b/arch/powerpc/mm/fsl_booke_mmu.c
@@ -171,11 +171,10 @@ unsigned long calc_cam_sz(unsigned long ram, unsigned long virt,
return 1UL << camsize;
}
-unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx)
+static unsigned long map_mem_in_cams_addr(phys_addr_t phys, unsigned long virt,
+ unsigned long ram, int max_cam_idx)
{
int i;
- unsigned long virt = PAGE_OFFSET;
- phys_addr_t phys = memstart_addr;
unsigned long amount_mapped = 0;
/* Calculate CAM values */
@@ -195,6 +194,14 @@ unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx)
return amount_mapped;
}
+unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx)
+{
+ unsigned long virt = PAGE_OFFSET;
+ phys_addr_t phys = memstart_addr;
+
+ return map_mem_in_cams_addr(phys, virt, ram, max_cam_idx);
+}
+
#ifdef CONFIG_PPC32
#if defined(CONFIG_LOWMEM_CAM_NUM_BOOL) && (CONFIG_LOWMEM_CAM_NUM >= NUM_TLBCAMS)
@@ -289,7 +296,11 @@ notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start)
is_second_reloc = 1;
n = switch_to_as1();
/* map a 64M area for the second relocation */
- map_mem_in_cams(0x4000000UL, CONFIG_LOWMEM_CAM_NUM);
+ if (memstart_addr > start)
+ map_mem_in_cams(0x4000000, CONFIG_LOWMEM_CAM_NUM);
+ else
+ map_mem_in_cams_addr(start, PAGE_OFFSET - offset,
+ 0x4000000, CONFIG_LOWMEM_CAM_NUM);
restore_to_as0(n, offset, __va(dt_ptr));
/* We should never reach here */
panic("Relocation error");
I will do more test and then create a new spin to merge this change and rebase
on the latest kernel. Thanks for the review.
Kevin
>
> -Scott
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply related
* [PATCH v2] powerpc/512x: dts: disable MPC5125 usb module
From: Matteo Facchinetti @ 2013-12-20 9:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: gsi, agust
At the moment the USB controller's pin muxing is not setup
correctly and causes a kernel panic upon system startup, so
disable the USB1 device tree node in the MPC5125 tower board
dts file.
The USB controller is connected to an USB3320 ULPI transceiver
and the device tree should receive an update to reflect correct
dependencies and required initialization data before the USB1
node can get re-enabled.
Signed-off-by: Matteo Facchinetti <matteo.facchinetti@sirius-es.it>
---
v2:
* improve the text of the commit as suggested by Gerhard Sittig <gsi@denx.de>
* put the 'status = "disabled"' to the last line in the list of properties
* rewiew the comment related to USB1 device tree node
---
arch/powerpc/boot/dts/mpc5125twr.dts | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/powerpc/boot/dts/mpc5125twr.dts b/arch/powerpc/boot/dts/mpc5125twr.dts
index 806479f..2fa1d17 100644
--- a/arch/powerpc/boot/dts/mpc5125twr.dts
+++ b/arch/powerpc/boot/dts/mpc5125twr.dts
@@ -229,6 +229,10 @@
reg = <0xA000 0x1000>;
};
+ // disable USB1 port
+ // TODO:
+ // correct pinmux config and fix USB3320 ulpi dependency
+ // before re-enable it
usb@3000 {
compatible = "fsl,mpc5121-usb2-dr";
reg = <0x3000 0x400>;
@@ -239,6 +243,7 @@
phy_type = "ulpi";
clocks = <&clks MPC512x_CLK_USB1>;
clock-names = "ipg";
+ status = "disabled";
};
// 5125 PSCs are not 52xx or 5121 PSC compatible
--
1.8.3.2
^ permalink raw reply related
* [RFC] linux/pci: move pci_platform_pm_ops to linux/pci.h
From: Dongsheng Wang @ 2013-12-20 10:03 UTC (permalink / raw)
To: rjw, bhelgaas
Cc: linux-pm, roy.zang, Wang Dongsheng, linux-pci, scottwood,
linuxppc-dev
From: Wang Dongsheng <dongsheng.wang@freescale.com>
make Freescale platform use pci_platform_pm_ops struct.
Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
---
If device's not set power state, we will use this interface to put the
device's into the correct state.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9c91ecc..48f8b1a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -33,36 +33,6 @@ int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
#endif
int pci_probe_reset_function(struct pci_dev *dev);
-/**
- * struct pci_platform_pm_ops - Firmware PM callbacks
- *
- * @is_manageable: returns 'true' if given device is power manageable by the
- * platform firmware
- *
- * @set_state: invokes the platform firmware to set the device's power state
- *
- * @choose_state: returns PCI power state of given device preferred by the
- * platform; to be used during system-wide transitions from a
- * sleeping state to the working state and vice versa
- *
- * @sleep_wake: enables/disables the system wake up capability of given device
- *
- * @run_wake: enables/disables the platform to generate run-time wake-up events
- * for given device (the device's wake-up capability has to be
- * enabled by @sleep_wake for this feature to work)
- *
- * If given platform is generally capable of power managing PCI devices, all of
- * these callbacks are mandatory.
- */
-struct pci_platform_pm_ops {
- bool (*is_manageable)(struct pci_dev *dev);
- int (*set_state)(struct pci_dev *dev, pci_power_t state);
- pci_power_t (*choose_state)(struct pci_dev *dev);
- int (*sleep_wake)(struct pci_dev *dev, bool enable);
- int (*run_wake)(struct pci_dev *dev, bool enable);
-};
-
-int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
void pci_power_up(struct pci_dev *dev);
void pci_disable_enabled_device(struct pci_dev *dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1084a15..20e07b8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -365,6 +365,37 @@ struct pci_dev {
size_t romlen; /* Length of ROM if it's not from the BAR */
};
+/**
+ * struct pci_platform_pm_ops - Firmware PM callbacks
+ *
+ * @is_manageable: returns 'true' if given device is power manageable by the
+ * platform firmware
+ *
+ * @set_state: invokes the platform firmware to set the device's power state
+ *
+ * @choose_state: returns PCI power state of given device preferred by the
+ * platform; to be used during system-wide transitions from a
+ * sleeping state to the working state and vice versa
+ *
+ * @sleep_wake: enables/disables the system wake up capability of given device
+ *
+ * @run_wake: enables/disables the platform to generate run-time wake-up events
+ * for given device (the device's wake-up capability has to be
+ * enabled by @sleep_wake for this feature to work)
+ *
+ * If given platform is generally capable of power managing PCI devices, all of
+ * these callbacks are mandatory.
+ */
+struct pci_platform_pm_ops {
+ bool (*is_manageable)(struct pci_dev *dev);
+ int (*set_state)(struct pci_dev *dev, pci_power_t state);
+ pci_power_t (*choose_state)(struct pci_dev *dev);
+ int (*sleep_wake)(struct pci_dev *dev, bool enable);
+ int (*run_wake)(struct pci_dev *dev, bool enable);
+};
+
+int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
+
static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
{
#ifdef CONFIG_PCI_IOV
--
1.8.5
^ permalink raw reply related
* Re: [PATCH V4 07/10] powerpc, lib: Add new branch instruction analysis support functions
From: Anshuman Khandual @ 2013-12-20 10:06 UTC (permalink / raw)
To: Michael Ellerman
Cc: mikey, ak, linux-kernel, eranian, linuxppc-dev, acme, sukadev,
mingo
In-Reply-To: <52A6B028.3090001@linux.vnet.ibm.com>
On 12/10/2013 11:39 AM, Anshuman Khandual wrote:
> On 12/09/2013 11:51 AM, Michael Ellerman wrote:
>> On Wed, 2013-04-12 at 10:32:39 UTC, Anshuman Khandual wrote:
>>> Generic powerpc branch instruction analysis support added in the code
>>> patching library which will help the subsequent patch on SW based
>>> filtering of branch records in perf. This patch also converts and
>>> exports some of the existing local static functions through the header
>>> file to be used else where.
>>>
>>> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
>>> index a6f8c7a..8bab417 100644
>>> --- a/arch/powerpc/include/asm/code-patching.h
>>> +++ b/arch/powerpc/include/asm/code-patching.h
>>> @@ -22,6 +22,36 @@
>>> #define BRANCH_SET_LINK 0x1
>>> #define BRANCH_ABSOLUTE 0x2
>>>
>>> +#define XL_FORM_LR 0x4C000020
>>> +#define XL_FORM_CTR 0x4C000420
>>> +#define XL_FORM_TAR 0x4C000460
>>> +
>>> +#define BO_ALWAYS 0x02800000
>>> +#define BO_CTR 0x02000000
>>> +#define BO_CRBI_OFF 0x00800000
>>> +#define BO_CRBI_ON 0x01800000
>>> +#define BO_CRBI_HINT 0x00400000
>>> +
>>> +/* Forms of branch instruction */
>>> +int instr_is_branch_iform(unsigned int instr);
>>> +int instr_is_branch_bform(unsigned int instr);
>>> +int instr_is_branch_xlform(unsigned int instr);
>>> +
>>> +/* Classification of XL-form instruction */
>>> +int is_xlform_lr(unsigned int instr);
>>> +int is_xlform_ctr(unsigned int instr);
>>> +int is_xlform_tar(unsigned int instr);
>>> +
>>> +/* Branch instruction is a call */
>>> +int is_branch_link_set(unsigned int instr);
>>> +
>>> +/* BO field analysis (B-form or XL-form) */
>>> +int is_bo_always(unsigned int instr);
>>> +int is_bo_ctr(unsigned int instr);
>>> +int is_bo_crbi_off(unsigned int instr);
>>> +int is_bo_crbi_on(unsigned int instr);
>>> +int is_bo_crbi_hint(unsigned int instr);
>>
>>
>> I think this is the wrong API.
>>
>> We end up with all these micro checks, which don't actually encapsulate much,
>> and don't implement the logic perf needs. If we had another user for this level
>> of detail then it might make sense, but for a single user I think we're better
>> off just implementing the semantics it wants.
>>
>
> Having a comprehensive list of branch instruction analysis APIs which some other
> user can also use in the future does not make it wrong. Being more elaborate and
> detailed makes this one a better choice than the API you have suggested below.
>
>> So that would be something more like:
>>
>> bool instr_is_return_branch(unsigned int instr);
>> bool instr_is_conditional_branch(unsigned int instr);
>> bool instr_is_func_call(unsigned int instr);
>> bool instr_is_indirect_func_call(unsigned int instr);
>>
>>
>> These would then encapsulate something like the logic in your 8/10 patch. You
>> can hopefully also optimise the checking logic in each routine because you know
>> the exact semantics you're implementing.
Any ways, here is the patch which is will supersede the present patch for adding
required library functions. Hope this works.
commit 9d9f11a6b778b51732aaa0e7c9dea4be3385df56
Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Date: Fri Dec 20 13:46:15 2013 +0530
powerpc, lib: Add new branch analysis support functions
Generic powerpc branch analysis support added in the code patching
library which will help the subsequent patch on SW based filtering
of branch records in perf.
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index a6f8c7a..15700b5 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -22,6 +22,16 @@
#define BRANCH_SET_LINK 0x1
#define BRANCH_ABSOLUTE 0x2
+#define XL_FORM_LR 0x4C000020
+#define XL_FORM_CTR 0x4C000420
+#define XL_FORM_TAR 0x4C000460
+
+#define BO_ALWAYS 0x02800000
+#define BO_CTR 0x02000000
+#define BO_CRBI_OFF 0x00800000
+#define BO_CRBI_ON 0x01800000
+#define BO_CRBI_HINT 0x00400000
+
unsigned int create_branch(const unsigned int *addr,
unsigned long target, int flags);
unsigned int create_cond_branch(const unsigned int *addr,
@@ -49,4 +59,10 @@ static inline unsigned long ppc_function_entry(void *func)
#endif
}
+/* Perf branch filters */
+bool instr_is_return_branch(unsigned int instr);
+bool instr_is_conditional_branch(unsigned int instr);
+bool instr_is_func_call(unsigned int instr);
+bool instr_is_indirect_func_call(unsigned int instr);
+
#endif /* _ASM_POWERPC_CODE_PATCHING_H */
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 17e5b23..ad39c58 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -77,6 +77,7 @@ static unsigned int branch_opcode(unsigned int instr)
return (instr >> 26) & 0x3F;
}
+/* Forms of branch instruction */
static int instr_is_branch_iform(unsigned int instr)
{
return branch_opcode(instr) == 18;
@@ -87,6 +88,140 @@ static int instr_is_branch_bform(unsigned int instr)
return branch_opcode(instr) == 16;
}
+static int instr_is_branch_xlform(unsigned int instr)
+{
+ return branch_opcode(instr) == 19;
+}
+
+/* Classification of XL-form instruction */
+static int is_xlform_lr(unsigned int instr)
+{
+ return (instr & XL_FORM_LR) == XL_FORM_LR;
+}
+
+static int is_xlform_ctr(unsigned int instr)
+{
+ return (instr & XL_FORM_CTR) == XL_FORM_CTR;
+}
+
+static int is_xlform_tar(unsigned int instr)
+{
+ return (instr & XL_FORM_TAR) == XL_FORM_TAR;
+}
+
+/* BO field analysis (B-form or XL-form) */
+static int is_bo_always(unsigned int instr)
+{
+ return (instr & BO_ALWAYS) == BO_ALWAYS;
+}
+
+static int is_bo_ctr(unsigned int instr)
+{
+ return (instr & BO_CTR) == BO_CTR;
+}
+
+static int is_bo_crbi_off(unsigned int instr)
+{
+ return (instr & BO_CRBI_OFF) == BO_CRBI_OFF;
+}
+
+static int is_bo_crbi_on(unsigned int instr)
+{
+ return (instr & BO_CRBI_ON) == BO_CRBI_ON;
+}
+
+static int is_bo_crbi_hint(unsigned int instr)
+{
+ return (instr & BO_CRBI_HINT) == BO_CRBI_HINT;
+}
+
+/* Link bit is set */
+static int is_branch_link_set(unsigned int instr)
+{
+ return (instr & BRANCH_SET_LINK) == BRANCH_SET_LINK;
+}
+
+/* Perf branch filters */
+bool instr_is_return_branch(unsigned int instr)
+{
+ /*
+ * Conditional and unconditional branch to LR register
+ * without seting the link register.
+ */
+ if (is_xlform_lr(instr) && !is_branch_link_set(instr))
+ return true;
+
+ return false;
+}
+
+bool instr_is_conditional_branch(unsigned int instr)
+{
+ /* I-form instruction - excluded */
+ if (instr_is_branch_iform(instr))
+ return false;
+
+ /* B-form or XL-form instruction */
+ if (instr_is_branch_bform(instr) || instr_is_branch_xlform(instr)) {
+
+ /* Not branch always */
+ if (!is_bo_always(instr)) {
+
+ /* Conditional branch to CTR register */
+ if (is_bo_ctr(instr))
+ return false;
+
+ /* CR[BI] conditional branch with static hint */
+ if (is_bo_crbi_off(instr) || is_bo_crbi_on(instr)) {
+ if (is_bo_crbi_hint(instr))
+ return false;;
+ }
+ return true;
+ }
+ }
+ return false;
+}
+
+bool instr_is_func_call(unsigned int instr)
+{
+ /* LR should be set */
+ if (is_branch_link_set(instr))
+ return true;
+
+ return false;
+}
+
+bool instr_is_indirect_func_call(unsigned int instr)
+{
+ /* XL-form instruction */
+ if (instr_is_branch_xlform(instr)) {
+
+ /* LR should be set */
+ if (is_branch_link_set(instr)) {
+ /*
+ * Conditional and unconditional
+ * branch to CTR register.
+ */
+ if (is_xlform_ctr(instr))
+ return true;
+
+ /*
+ * Conditional and unconditional
+ * branch to LR register.
+ */
+ if (is_xlform_lr(instr))
+ return true;
+
+ /*
+ * Conditional and unconditional
+ * branch to TAR register.
+ */
+ if (is_xlform_tar(instr))
+ return true;
+ }
+ }
+ return false;
+}
+
int instr_is_relative_branch(unsigned int instr)
{
if (instr & BRANCH_ABSOLUTE)
^ permalink raw reply related
* Re: [PATCH V4 08/10] powerpc, perf: Enable SW filtering in branch stack sampling framework
From: Anshuman Khandual @ 2013-12-20 11:01 UTC (permalink / raw)
To: Michael Ellerman
Cc: mikey, ak, linux-kernel, eranian, linuxppc-dev, acme, sukadev,
mingo
In-Reply-To: <20131209062146.EABBE2C00C1@ozlabs.org>
On 12/09/2013 11:51 AM, Michael Ellerman wrote:
> On Wed, 2013-04-12 at 10:32:40 UTC, Anshuman Khandual wrote:
>> This patch enables SW based post processing of BHRB captured branches
>> to be able to meet more user defined branch filtration criteria in perf
>> branch stack sampling framework. These changes increase the number of
>> branch filters and their valid combinations on any powerpc64 server
>> platform with BHRB support. Find the summary of code changes here.
>>
>> (1) struct cpu_hw_events
>>
>> Introduced two new variables track various filter values and mask
>>
>> (a) bhrb_sw_filter Tracks SW implemented branch filter flags
>> (b) filter_mask Tracks both (SW and HW) branch filter flags
>
> The name 'filter_mask' doesn't mean much to me. I'd rather it was 'bhrb_filter'.
Done.
>
>
>> (2) Event creation
>>
>> Kernel will figure out supported BHRB branch filters through a PMU call
>> back 'bhrb_filter_map'. This function will find out how many of the
>> requested branch filters can be supported in the PMU HW. It will not
>> try to invalidate any branch filter combinations. Event creation will not
>> error out because of lack of HW based branch filters. Meanwhile it will
>> track the overall supported branch filters in the "filter_mask" variable.
>>
>> Once the PMU call back returns kernel will process the user branch filter
>> request against available SW filters while looking at the "filter_mask".
>> During this phase all the branch filters which are still pending from the
>> user requested list will have to be supported in SW failing which the
>> event creation will error out.
>>
>> (3) SW branch filter
>>
>> During the BHRB data capture inside the PMU interrupt context, each
>> of the captured 'perf_branch_entry.from' will be checked for compliance
>> with applicable SW branch filters. If the entry does not conform to the
>> filter requirements, it will be discarded from the final perf branch
>> stack buffer.
>>
>> (4) Supported SW based branch filters
>>
>> (a) PERF_SAMPLE_BRANCH_ANY_RETURN
>> (b) PERF_SAMPLE_BRANCH_IND_CALL
>> (c) PERF_SAMPLE_BRANCH_ANY_CALL
>> (d) PERF_SAMPLE_BRANCH_COND
>>
>> Please refer patch to understand the classification of instructions into
>> these branch filter categories.
>>
>> (5) Multiple branch filter semantics
>>
>> Book3 sever implementation follows the same OR semantics (as implemented in
>> x86) while dealing with multiple branch filters at any point of time. SW
>> branch filter analysis is carried on the data set captured in the PMU HW.
>> So the resulting set of data (after applying the SW filters) will inherently
>> be an AND with the HW captured set. Hence any combination of HW and SW branch
>> filters will be invalid. HW based branch filters are more efficient and faster
>> compared to SW implemented branch filters. So at first the PMU should decide
>> whether it can support all the requested branch filters itself or not. In case
>> it can support all the branch filters in an OR manner, we dont apply any SW
>> branch filter on top of the HW captured set (which is the final set). This
>> preserves the OR semantic of multiple branch filters as required. But in case
>> where the PMU cannot support all the requested branch filters in an OR manner,
>> it should not apply any it's filters and leave it upto the SW to handle them
>> all. Its the PMU code's responsibility to uphold this protocol to be able to
>> conform to the overall OR semantic of perf branch stack sampling framework.
>
>
> I'd prefer this level of commentary was in a block comment in the code. It's
> much more likely to be seen by a future hacker than here in the commit log.
>
I felt it was pretty big to be inside the code blocks. Though I have improved in-code
documentation substantially in the next version.
>
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 2de7d48..54d39a5 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -48,6 +48,8 @@ struct cpu_hw_events {
>>
>> /* BHRB bits */
>> u64 bhrb_hw_filter; /* BHRB HW branch filter */
>> + u64 bhrb_sw_filter; /* BHRB SW branch filter */
>> + u64 filter_mask; /* Branch filter mask */
>> int bhrb_users;
>> void *bhrb_context;
>> struct perf_branch_stack bhrb_stack;
>> @@ -400,6 +402,228 @@ static __u64 power_pmu_bhrb_to(u64 addr)
>> return target - (unsigned long)&instr + addr;
>> }
>>
>> +/*
>> + * Instruction opcode analysis
>> + *
>> + * Analyse instruction opcodes and classify them
>> + * into various branch filter options available.
>> + * This follows the standard semantics of OR which
>> + * means that instructions which conforms to `any`
>> + * of the requested branch filters get picked up.
>> + */
>> +static bool validate_instruction(unsigned int *addr, u64 bhrb_sw_filter)
>> +{
>
> "validate" is not a good name here. That implies that this routine identifies
> "valid" and "invalid" instructions - but that's not really correct.
>
Done.
validate_instruction --> check_instruction
> Also it's preferable to not use the same variable name for the local as for the
> cpuhw->bhrb_sw_filter global. Although technically it doesn't shadow the global
> it can still be confusing to a human, ie. me. A good name for the local would
> just be "sw_filter" because we know in this code that we're dealing with the
> BHRB.
>
Done.
local variable bhrb_sw_filter ---> sw_filter
>
>> + bool result = false;
>> +
>> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_ANY_RETURN) {
>> +
>> + /* XL-form instruction */
>> + if (instr_is_branch_xlform(*addr)) {
>> +
>> + /* LR should not be set */
>> + /*
>> + * Conditional and unconditional
>> + * branch to LR register.
>> + */
>> + if (is_xlform_lr(*addr))
>> + result = true;
>> + }
>> + }
>> + }
>
> is_xform_lr() implies instr_is_branch_xlform(), and once you get a hit you can
> short-circuit and exit the function, so this should boil down to just:
>
> if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_ANY_RETURN)
> if (is_xlform_lr(*addr) && !is_branch_link_set(*addr))
> return true;
>
Done
>
> Having said that I think it should move into a routine in code-patching as I
> said in the comments to the previous patch.
>
Done
>
>> +
>> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_IND_CALL) {
>> + /* XL-form instruction */
>> + if (instr_is_branch_xlform(*addr)) {
>> +
>> + /* LR should be set */
>> + if (is_branch_link_set(*addr)) {
>> + /*
>> + * Conditional and unconditional
>> + * branch to CTR.
>> + */
>> + if (is_xlform_ctr(*addr))
>> + result = true;
>> +
>> + /*
>> + * Conditional and unconditional
>> + * branch to LR.
>> + */
>> + if (is_xlform_lr(*addr))
>> + result = true;
>> +
>> + /*
>> + * Conditional and unconditional
>> + * branch to TAR.
>> + */
>> + if (is_xlform_tar(*addr))
>> + result = true;
>
> What other kind of XL-Form branch is there?
I am not sure. Do you know of any ?
>
>> + }
>> + }
>> + }
>
> The comments above all have a bogus leading space.
>
Rectified.
>> +
>> + /* Any-form branch */
>> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_ANY_CALL) {
>> + /* LR should be set */
>> + if (is_branch_link_set(*addr))
>> + result = true;
>
> Short circuit.
>
Rectified.
>> + }
>> +
>> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_COND) {
>> +
>> + /* I-form instruction - excluded */
>> + if (instr_is_branch_iform(*addr))
>> + goto out;
>> +
>> + /* B-form or XL-form instruction */
>> + if (instr_is_branch_bform(*addr) || instr_is_branch_xlform(*addr)) {
>> +
>> + /* Not branch always */
>> + if (!is_bo_always(*addr)) {
>> +
>> + /* Conditional branch to CTR register */
>> + if (is_bo_ctr(*addr))
>> + goto out;
>
> We might have discussed this but why not?
Did not get that, discuss what ?
>
>> +
>> + /* CR[BI] conditional branch with static hint */
>
> A conditional branch with a static hint is still a conditional branch?
>
No its not.
>> + if (is_bo_crbi_off(*addr) || is_bo_crbi_on(*addr)) {
>> + if (is_bo_crbi_hint(*addr))
>> + goto out;
>> + }
>> +
>> + result = true;
>> + }
>> + }
>> + }
>> +out:
>> + return result;
>> +}
>> +
>> +static bool check_instruction(u64 addr, u64 bhrb_sw_filter)
>> +{
>
>
> "check" is not a very descriptive name here, especially when "check" calls
> "validate".
>
> "filter" is also not good because a filter keeps some things and rejects others,
> and the directionality is not clear.
>
> I'd suggest "filter_selects_branch()" or just "keep_branch()".
>
keep_branch() now calls check_instruction()
>
>> + unsigned int instr;
>> + bool ret;
>> +
>> + if (bhrb_sw_filter == 0)
>> + return true;
>> +
>> + if (is_kernel_addr(addr)) {
>> + ret = validate_instruction((unsigned int *) addr, bhrb_sw_filter);
>
> No reason not to return directly here.
>
> That would then remove the need for an else block.
Done.
>
>> + } else {
>> + /*
>> + * Userspace address needs to be
>> + * copied first before analysis.
>> + */
>> + pagefault_disable();
>> + ret = __get_user_inatomic(instr, (unsigned int __user *)addr);
>
> I suspect you borrowed this incantation from the callchain code. Unlike that
> code you don't fallback to reading the page tables directly.
>
> I'd rather see the accessor in the callchain code made generic and have you
> call it here.
You have mentioned to take care of this issue yourself.
>
>> +
>> + /*
>> + * If the instruction could not be accessible
>> + * from user space, we still 'okay' the entry.
>> + */
>> + if (ret) {
>> + pagefault_enable();
>> + return true;
>> + }
>> + pagefault_enable();
>> + ret = validate_instruction(&instr, bhrb_sw_filter);
>
> No reason not to return directly here.
>
Done.
>> + }
>> + return ret;
>> +}
>> +
>> +/*
>> + * Validate whether all requested branch filters
>> + * are getting processed either in the PMU or in SW.
>> + */
>> +static int match_filters(u64 branch_sample_type, u64 filter_mask)
>
> I don't really understand why we have this routine?
>
> We should implement the filter in HW if we can, or in SW. Which filters can't we
> implement in SW?
>
As of now in POWER8, we implement all the filters either in HW or SW. But this framework
allows us to have a combined HW and SW branch filter implementation where PMU HW support
ORing of branch filters (which is not true for POWER8). This functions just runs a sanity
check to make sure that we got all branch filters covered either in HW or SW. BTW changed
name of the function from "match_filters" to all_filters_covered.
>> +{
>> + u64 x;
>> +
>> + if (filter_mask == PERF_SAMPLE_BRANCH_ANY)
>> + return true;
>> +
>> + for_each_branch_sample_type(x) {
>> + if (!(branch_sample_type & x))
>> + continue;
>> + /*
>> + * Privilege filter requests have been already
>> + * taken care during the base PMU configuration.
>> + */
>> + if (x == PERF_SAMPLE_BRANCH_USER)
>> + continue;
>> + if (x == PERF_SAMPLE_BRANCH_KERNEL)
>> + continue;
>> + if (x == PERF_SAMPLE_BRANCH_HV)
>> + continue;
>> +
>> + /*
>> + * Requested filter not available either
>> + * in PMU or in SW.
>> + */
>> + if (!(filter_mask & x))
>> + return false;
>> + }
>> + return true;
>> +}
>> +
>> +/*
>> + * Required SW based branch filters
>> + *
>> + * This is called after figuring out what all branch filters the
>> + * PMU HW supports for the requested branch filter set. Here we
>> + * will go through all the SW implemented branch filters one by
>> + * one and pick them up if its not already supported in the PMU.
>> + */
>> +static u64 branch_filter_map(u64 branch_sample_type, u64 pmu_bhrb_filter,
>> + u64 *filter_mask)
>
> Whitespace is foobar here ^
>
Will fix it.
> This function deals exclusively with the software filter IIUI, but the name
> doesn't indicate that in any way.
Correct, changed the name from branch_filter_map to bhrb_sw_filter_map which
will complement bhrb_filter_map used for figuring out PMU supported filters.
>
> As far as the logic goes, you return the software filter value, as well as
> mutating the *filter_mask. And in all cases you make the same modification to
> both. That seems very dubious.
>
yeah, thats right. Because we will use cpuhw->bhrb_sw_filter to apply SW filters on
branch records once they are captured from BHRB and cpuhw->bhrb_filter
(cpuhw->filter_mask before) to track the overall coverage of branch filters either
in HW or SW. While we modify bhrb_filter (filter mask before) inside this function,
it previous contains branch filters which is promised to be implemented by the PMU
for this session.
> Shouldn't this routine just setup the software filter, and leave the upper
> level code to deal with the HW & SW filter values?
>
bhrb_filter (filter_mask before) runs through two functions one after the other. First
one being PMU specific bhrb_filter_map to figure out available HW filters for the session
and then bhrb_sw_filter_map to figure out available SW filters for the session. There is
no high level code dealing with bhrb_filter mask.
>> +{
>> + u64 branch_sw_filter = 0;
>> +
>> + /* No branch filter requested */
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY) {
>> + WARN_ON(pmu_bhrb_filter != 0);
>> + WARN_ON(*filter_mask != PERF_SAMPLE_BRANCH_ANY);
>> + return branch_sw_filter;
>> + }
>> +
>> + /*
>> + * PMU supported branch filters must also be implemented in SW
>> + * in the event when the PMU is unable to process them for some
>> + * reason. This all those branch filters can be satisfied with
>> + * SW implemented filters. But right now, there is now way to
>> + * initimate the user about this decision.
>
> Please proof read these comments, I don't entirely follow this one.
>
> You say "must also be implemented in SW" - but I think it's actually "must be
> implemented in SW", ie. the HW is not "also" implementing the filter.
>
> You say "in the event when" but I think you just mean "when" - the word "event"
> has a particular meaning in this code so you should only use it for that if at
> all possible.
>
> I don't follow "This all those".
>
> You should just drop the last sentence, there is never going to be any way to
> notify the user that their filter is implemented in HW vs SW, that's an
> implementation detail.
>
Took care of these observations.
>> + */
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) {
>> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_ANY_CALL)) {
>> + branch_sw_filter |= PERF_SAMPLE_BRANCH_ANY_CALL;
>> + *filter_mask |= PERF_SAMPLE_BRANCH_ANY_CALL;
>> + }
>> + }
>> +
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_COND) {
>> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_COND)) {
>> + branch_sw_filter |= PERF_SAMPLE_BRANCH_COND;
>> + *filter_mask |= PERF_SAMPLE_BRANCH_COND;
>> + }
>> + }
>> +
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_RETURN) {
>> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_ANY_RETURN)) {
>> + branch_sw_filter |= PERF_SAMPLE_BRANCH_ANY_RETURN;
>> + *filter_mask |= PERF_SAMPLE_BRANCH_ANY_RETURN;
>> + }
>> + }
>> +
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL) {
>> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_IND_CALL)) {
>> + branch_sw_filter |= PERF_SAMPLE_BRANCH_IND_CALL;
>> + *filter_mask |= PERF_SAMPLE_BRANCH_IND_CALL;
>> + }
>> + }
>> +
>> + return branch_sw_filter;
>> +}
>> +
>> /* Processing BHRB entries */
>> void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
>> {
>> @@ -459,17 +683,29 @@ void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
>> addr = 0;
>> }
>> cpuhw->bhrb_entries[u_index].from = addr;
>> +
>> + if (!check_instruction(cpuhw->
>> + bhrb_entries[u_index].from,
>> + cpuhw->bhrb_sw_filter))
>> + u_index--;
>> } else {
>> /* Branches to immediate field
>> (ie I or B form) */
>> cpuhw->bhrb_entries[u_index].from = addr;
>> - cpuhw->bhrb_entries[u_index].to =
>> - power_pmu_bhrb_to(addr);
>> - cpuhw->bhrb_entries[u_index].mispred = pred;
>> - cpuhw->bhrb_entries[u_index].predicted = ~pred;
>> + if (check_instruction(cpuhw->
>> + bhrb_entries[u_index].from,
>> + cpuhw->bhrb_sw_filter)) {
>> + cpuhw->bhrb_entries[u_index].
>> + to = power_pmu_bhrb_to(addr);
>> + cpuhw->bhrb_entries[u_index].
>> + mispred = pred;
>> + cpuhw->bhrb_entries[u_index].
>> + predicted = ~pred;
>> + } else {
>> + u_index--;
>> + }
>> }
>> u_index++;
>
>
> This code was already in need of some unindentation, and now it's just
> ridiculous.
>
> To start with at the beginning of this routine we have:
>
> while (..) {
> if (!val)
> break;
> else {
> // Bulk of the logic
> ...
> }
> }
>
> That should almost always become:
>
> while (..) {
> if (!val)
> break;
>
> // Bulk of the logic
> ...
> }
>
>
> But in this case that's not enough. Please send a precursor patch which moves
> this logic out into a helper function.
>
Done
>
>> -
>> }
>> }
>> cpuhw->bhrb_stack.nr = u_index;
>> @@ -1255,7 +1491,11 @@ nocheck:
>> if (has_branch_stack(event)) {
>> power_pmu_bhrb_enable(event);
>> cpuhw->bhrb_hw_filter = ppmu->bhrb_filter_map(
>> - event->attr.branch_sample_type);
>> + event->attr.branch_sample_type,
>> + &cpuhw->filter_mask);
>> + cpuhw->bhrb_sw_filter = branch_filter_map
>> + (event->attr.branch_sample_type,
>> + cpuhw->bhrb_hw_filter, &cpuhw->filter_mask);
>> }
>>
>> perf_pmu_enable(event->pmu);
>> @@ -1637,10 +1877,16 @@ static int power_pmu_event_init(struct perf_event *event)
>> err = power_check_constraints(cpuhw, events, cflags, n + 1);
>>
>> if (has_branch_stack(event)) {
>> - cpuhw->bhrb_hw_filter = ppmu->bhrb_filter_map(
>> - event->attr.branch_sample_type);
>> -
>> - if(cpuhw->bhrb_hw_filter == -1)
>> + cpuhw->bhrb_hw_filter = ppmu->bhrb_filter_map
>> + (event->attr.branch_sample_type,
>> + &cpuhw->filter_mask);
>> + cpuhw->bhrb_sw_filter = branch_filter_map
>> + (event->attr.branch_sample_type,
>> + cpuhw->bhrb_hw_filter,
>> + &cpuhw->filter_mask);
>> +
>> + if(!match_filters(event->attr.branch_sample_type,
>> + cpuhw->filter_mask))
>> return -EOPNOTSUPP;
>
> The above two hunks look too similar for my liking.
Moved the SW filter check below the else block to make it common for both the type branches.
Wanted to save some cycles by not accessing the user space (power_pmu_bhrb_to) in case
we know that the "from" is not going to pass the SW branch filter check.
^ permalink raw reply
* Re: [PATCH V4 10/10] powerpc, perf: Cleanup SW branch filter list look up
From: Anshuman Khandual @ 2013-12-20 11:06 UTC (permalink / raw)
To: Michael Ellerman
Cc: mikey, ak, linux-kernel, eranian, linuxppc-dev, acme, sukadev,
mingo
In-Reply-To: <20131209062147.B874D2C00C6@ozlabs.org>
On 12/09/2013 11:51 AM, Michael Ellerman wrote:
> On Wed, 2013-04-12 at 10:32:42 UTC, Anshuman Khandual wrote:
>> This patch adds enumeration for all available SW branch filters
>> in powerpc book3s code and also streamlines the look for the
>> SW branch filter entries while trying to figure out which all
>> branch filters can be supported in SW.
>
> This appears to patch code that was only added in 8/10 ?
>
> Was there any reason not to do it the right way from the beginning?
No reason, merged this into the 8/10th patch. Working on the V5 of
this patchset. Will send out a draft V5 version for early review.
Regards
Anshuman
^ permalink raw reply
* [PATCH v3 REPOST 0/4] i2c : i2c-ibm-iic : use interrupts to perform the data transfer
From: jean-jacques hiblot @ 2013-12-20 15:12 UTC (permalink / raw)
To: wsa; +Cc: gregory.clement, jean-jacques hiblot, linuxppc-dev, linux-i2c
Hello Wolfram,
I'm reposting this patch set with the linux-ppc list in cc as the ibm I2c
controller is primarily (only?) available on ppc platforms.
This patch set aims to improve the performance of the driver for the IBM iic
controller by implementing the data transfer in the interrupt handler.
Using interrupts to trigger the data transfer reduces and make more
deterministic the latencies between indivdual bytes, and consequently reduces
the total transfer time,
In our test environement with multiple masters, this significantly reduces the
rate of i2c errors.
Changes since v2:
- cosmectics change (comments style, removed a hard-coded value)
- moved some parts from one patch to another
Changes since v1:
- split the patch in 4 individual patches. The code has been refactored a bit
to make the diff easier to read.
- changed some dev_dbg in dev_err or dev_warn when more appropriate
jean-jacques hiblot (4):
i2c: i2c-ibm-iic: cleanup.
i2c: i2c-ibm-iic: perform the transfer in the interrupt handler
i2c: i2c-ibm-iic: Implements transfer abortion
i2c: i2c-ibm-iic: Implements a polling mode
drivers/i2c/busses/i2c-ibm_iic.c | 490 +++++++++++++++++++++++----------------
drivers/i2c/busses/i2c-ibm_iic.h | 20 +-
2 files changed, 309 insertions(+), 201 deletions(-)
--
1.8.4.2
^ permalink raw reply
* [PATCH v3 REPOST 1/4] i2c: i2c-ibm-iic: cleanup.
From: jean-jacques hiblot @ 2013-12-20 15:12 UTC (permalink / raw)
To: wsa
Cc: gregory.clement, jean-jacques hiblot, linuxppc-dev, linux-i2c,
jean-jacques hiblot
In-Reply-To: <1387552376-12986-1-git-send-email-jjhiblot@traphandler.com>
From: jean-jacques hiblot <jjhiblot@gmail.com>
* removed unneeded 'volatile' qualifiers and casts
* use the dev_dbg, dev_err etc. instead of printk
* removed unneeded members for the driver's private data
* fixed the style of the multi-line comments
Signed-off-by: jean-jacques hiblot <jjhiblot@traphandler.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
drivers/i2c/busses/i2c-ibm_iic.c | 135 +++++++++++++++++++++------------------
drivers/i2c/busses/i2c-ibm_iic.h | 9 +--
2 files changed, 78 insertions(+), 66 deletions(-)
diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index ff3caa0..03ab756 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -68,22 +68,26 @@ MODULE_PARM_DESC(iic_force_fast, "Force fast mode (400 kHz)");
#undef DBG2
#endif
+# define ERR(dev, f, x...) dev_err(dev->adap.dev.parent, f, ##x)
+
#if DBG_LEVEL > 0
-# define DBG(f,x...) printk(KERN_DEBUG "ibm-iic" f, ##x)
+# define DBG(dev, f, x...) dev_dbg(dev->adap.dev.parent, f, ##x)
#else
-# define DBG(f,x...) ((void)0)
+# define DBG(dev, f, x...) ((void)0)
#endif
#if DBG_LEVEL > 1
-# define DBG2(f,x...) DBG(f, ##x)
+# define DBG2(dev, f, x...) DBG(dev, f, ##x)
#else
-# define DBG2(f,x...) ((void)0)
+# define DBG2(dev, f, x...) ((void)0)
#endif
#if DBG_LEVEL > 2
static void dump_iic_regs(const char* header, struct ibm_iic_private* dev)
{
- volatile struct iic_regs __iomem *iic = dev->vaddr;
- printk(KERN_DEBUG "ibm-iic%d: %s\n", dev->idx, header);
- printk(KERN_DEBUG
+ struct iic_regs __iomem *iic = dev->vaddr;
+ struct device *device = dev.adap.dev.parent;
+
+ dev_dbg(device, ": %s\n", header);
+ dev_dbg(device,
" cntl = 0x%02x, mdcntl = 0x%02x\n"
" sts = 0x%02x, extsts = 0x%02x\n"
" clkdiv = 0x%02x, xfrcnt = 0x%02x\n"
@@ -133,9 +137,9 @@ static inline void iic_interrupt_mode(struct ibm_iic_private* dev, int enable)
*/
static void iic_dev_init(struct ibm_iic_private* dev)
{
- volatile struct iic_regs __iomem *iic = dev->vaddr;
+ struct iic_regs __iomem *iic = dev->vaddr;
- DBG("%d: init\n", dev->idx);
+ DBG(dev, "init\n");
/* Clear master address */
out_8(&iic->lmadr, 0);
@@ -178,11 +182,11 @@ static void iic_dev_init(struct ibm_iic_private* dev)
*/
static void iic_dev_reset(struct ibm_iic_private* dev)
{
- volatile struct iic_regs __iomem *iic = dev->vaddr;
+ struct iic_regs __iomem *iic = dev->vaddr;
int i;
u8 dc;
- DBG("%d: soft reset\n", dev->idx);
+ DBG(dev, "soft reset\n");
DUMP_REGS("reset", dev);
/* Place chip in the reset state */
@@ -191,7 +195,7 @@ static void iic_dev_reset(struct ibm_iic_private* dev)
/* Check if bus is free */
dc = in_8(&iic->directcntl);
if (!DIRCTNL_FREE(dc)){
- DBG("%d: trying to regain bus control\n", dev->idx);
+ DBG(dev, "trying to regain bus control\n");
/* Try to set bus free state */
out_8(&iic->directcntl, DIRCNTL_SDAC | DIRCNTL_SCC);
@@ -226,7 +230,7 @@ static void iic_dev_reset(struct ibm_iic_private* dev)
*/
/* Wait for SCL and/or SDA to be high */
-static int iic_dc_wait(volatile struct iic_regs __iomem *iic, u8 mask)
+static int iic_dc_wait(struct iic_regs __iomem *iic, u8 mask)
{
unsigned long x = jiffies + HZ / 28 + 2;
while ((in_8(&iic->directcntl) & mask) != mask){
@@ -239,19 +243,18 @@ static int iic_dc_wait(volatile struct iic_regs __iomem *iic, u8 mask)
static int iic_smbus_quick(struct ibm_iic_private* dev, const struct i2c_msg* p)
{
- volatile struct iic_regs __iomem *iic = dev->vaddr;
+ struct iic_regs __iomem *iic = dev->vaddr;
const struct i2c_timings* t = &timings[dev->fast_mode ? 1 : 0];
u8 mask, v, sda;
int i, res;
/* Only 7-bit addresses are supported */
if (unlikely(p->flags & I2C_M_TEN)){
- DBG("%d: smbus_quick - 10 bit addresses are not supported\n",
- dev->idx);
+ DBG(dev, "smbus_quick - 10 bit addresses are not supported\n");
return -EINVAL;
}
- DBG("%d: smbus_quick(0x%02x)\n", dev->idx, p->addr);
+ DBG(dev, "smbus_quick(0x%02x)\n", p->addr);
/* Reset IIC interface */
out_8(&iic->xtcntlss, XTCNTLSS_SRST);
@@ -304,7 +307,7 @@ static int iic_smbus_quick(struct ibm_iic_private* dev, const struct i2c_msg* p)
ndelay(t->buf);
- DBG("%d: smbus_quick -> %s\n", dev->idx, res ? "NACK" : "ACK");
+ DBG(dev, "smbus_quick -> %s\n", res ? "NACK" : "ACK");
out:
/* Remove reset */
out_8(&iic->xtcntlss, 0);
@@ -314,7 +317,7 @@ out:
return res;
err:
- DBG("%d: smbus_quick - bus is stuck\n", dev->idx);
+ DBG(dev, "smbus_quick - bus is stuck\n");
res = -EREMOTEIO;
goto out;
}
@@ -325,10 +328,10 @@ err:
static irqreturn_t iic_handler(int irq, void *dev_id)
{
struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_id;
- volatile struct iic_regs __iomem *iic = dev->vaddr;
+ struct iic_regs __iomem *iic = dev->vaddr;
- DBG2("%d: irq handler, STS = 0x%02x, EXTSTS = 0x%02x\n",
- dev->idx, in_8(&iic->sts), in_8(&iic->extsts));
+ DBG2(dev, "irq handler, STS = 0x%02x, EXTSTS = 0x%02x\n",
+ in_8(&iic->sts), in_8(&iic->extsts));
/* Acknowledge IRQ and wakeup iic_wait_for_tc */
out_8(&iic->sts, STS_IRQA | STS_SCMP);
@@ -343,10 +346,10 @@ static irqreturn_t iic_handler(int irq, void *dev_id)
*/
static int iic_xfer_result(struct ibm_iic_private* dev)
{
- volatile struct iic_regs __iomem *iic = dev->vaddr;
+ struct iic_regs __iomem *iic = dev->vaddr;
if (unlikely(in_8(&iic->sts) & STS_ERR)){
- DBG("%d: xfer error, EXTSTS = 0x%02x\n", dev->idx,
+ DBG(dev, "xfer error, EXTSTS = 0x%02x\n",
in_8(&iic->extsts));
/* Clear errors and possible pending IRQs */
@@ -356,13 +359,14 @@ static int iic_xfer_result(struct ibm_iic_private* dev)
/* Flush master data buffer */
out_8(&iic->mdcntl, in_8(&iic->mdcntl) | MDCNTL_FMDB);
- /* Is bus free?
+ /*
+ * Is bus free?
* If error happened during combined xfer
* IIC interface is usually stuck in some strange
* state, the only way out - soft reset.
*/
if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){
- DBG("%d: bus is stuck, resetting\n", dev->idx);
+ DBG(dev, "bus is stuck, resetting\n");
iic_dev_reset(dev);
}
return -EREMOTEIO;
@@ -376,10 +380,10 @@ static int iic_xfer_result(struct ibm_iic_private* dev)
*/
static void iic_abort_xfer(struct ibm_iic_private* dev)
{
- volatile struct iic_regs __iomem *iic = dev->vaddr;
+ struct iic_regs __iomem *iic = dev->vaddr;
unsigned long x;
- DBG("%d: iic_abort_xfer\n", dev->idx);
+ DBG(dev, "iic_abort_xfer\n");
out_8(&iic->cntl, CNTL_HMT);
@@ -390,7 +394,7 @@ static void iic_abort_xfer(struct ibm_iic_private* dev)
x = jiffies + 2;
while ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){
if (time_after(jiffies, x)){
- DBG("%d: abort timeout, resetting...\n", dev->idx);
+ DBG(dev, "abort timeout, resetting...\n");
iic_dev_reset(dev);
return;
}
@@ -408,7 +412,7 @@ static void iic_abort_xfer(struct ibm_iic_private* dev)
*/
static int iic_wait_for_tc(struct ibm_iic_private* dev){
- volatile struct iic_regs __iomem *iic = dev->vaddr;
+ struct iic_regs __iomem *iic = dev->vaddr;
int ret = 0;
if (dev->irq >= 0){
@@ -417,9 +421,9 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){
!(in_8(&iic->sts) & STS_PT), dev->adap.timeout);
if (unlikely(ret < 0))
- DBG("%d: wait interrupted\n", dev->idx);
+ DBG(dev, "wait interrupted\n");
else if (unlikely(in_8(&iic->sts) & STS_PT)){
- DBG("%d: wait timeout\n", dev->idx);
+ DBG(dev, "wait timeout\n");
ret = -ETIMEDOUT;
}
}
@@ -429,13 +433,13 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){
while (in_8(&iic->sts) & STS_PT){
if (unlikely(time_after(jiffies, x))){
- DBG("%d: poll timeout\n", dev->idx);
+ DBG(dev, "poll timeout\n");
ret = -ETIMEDOUT;
break;
}
if (unlikely(signal_pending(current))){
- DBG("%d: poll interrupted\n", dev->idx);
+ DBG(dev, "poll interrupted\n");
ret = -ERESTARTSYS;
break;
}
@@ -448,7 +452,7 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){
else
ret = iic_xfer_result(dev);
- DBG2("%d: iic_wait_for_tc -> %d\n", dev->idx, ret);
+ DBG2(dev, "iic_wait_for_tc -> %d\n", ret);
return ret;
}
@@ -459,7 +463,7 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){
static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm,
int combined_xfer)
{
- volatile struct iic_regs __iomem *iic = dev->vaddr;
+ struct iic_regs __iomem *iic = dev->vaddr;
char* buf = pm->buf;
int i, j, loops, ret = 0;
int len = pm->len;
@@ -475,14 +479,14 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm,
if (!(cntl & CNTL_RW))
for (j = 0; j < count; ++j)
- out_8((void __iomem *)&iic->mdbuf, *buf++);
+ out_8(&iic->mdbuf, *buf++);
if (i < loops - 1)
cmd |= CNTL_CHT;
else if (combined_xfer)
cmd |= CNTL_RPST;
- DBG2("%d: xfer_bytes, %d, CNTL = 0x%02x\n", dev->idx, count, cmd);
+ DBG2(dev, "xfer_bytes, %d, CNTL = 0x%02x\n", count, cmd);
/* Start transfer */
out_8(&iic->cntl, cmd);
@@ -493,8 +497,8 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm,
if (unlikely(ret < 0))
break;
else if (unlikely(ret != count)){
- DBG("%d: xfer_bytes, requested %d, transferred %d\n",
- dev->idx, count, ret);
+ DBG(dev, "xfer_bytes, requested %d, transferred %d\n",
+ count, ret);
/* If it's not a last part of xfer, abort it */
if (combined_xfer || (i < loops - 1))
@@ -506,7 +510,7 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm,
if (cntl & CNTL_RW)
for (j = 0; j < count; ++j)
- *buf++ = in_8((void __iomem *)&iic->mdbuf);
+ *buf++ = in_8(&iic->mdbuf);
}
return ret > 0 ? 0 : ret;
@@ -517,10 +521,10 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm,
*/
static inline void iic_address(struct ibm_iic_private* dev, struct i2c_msg* msg)
{
- volatile struct iic_regs __iomem *iic = dev->vaddr;
+ struct iic_regs __iomem *iic = dev->vaddr;
u16 addr = msg->addr;
- DBG2("%d: iic_address, 0x%03x (%d-bit)\n", dev->idx,
+ DBG2(dev, "iic_address, 0x%03x (%d-bit)\n",
addr, msg->flags & I2C_M_TEN ? 10 : 7);
if (msg->flags & I2C_M_TEN){
@@ -553,46 +557,49 @@ static inline int iic_address_neq(const struct i2c_msg* p1,
static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
{
struct ibm_iic_private* dev = (struct ibm_iic_private*)(i2c_get_adapdata(adap));
- volatile struct iic_regs __iomem *iic = dev->vaddr;
+ struct iic_regs __iomem *iic = dev->vaddr;
int i, ret = 0;
- DBG2("%d: iic_xfer, %d msg(s)\n", dev->idx, num);
+ DBG2(dev, "iic_xfer, %d msg(s)\n", num);
if (!num)
return 0;
- /* Check the sanity of the passed messages.
+ /*
+ * Check the sanity of the passed messages.
* Uhh, generic i2c layer is more suitable place for such code...
*/
if (unlikely(iic_invalid_address(&msgs[0]))){
- DBG("%d: invalid address 0x%03x (%d-bit)\n", dev->idx,
+ DBG(dev, "invalid address 0x%03x (%d-bit)\n",
msgs[0].addr, msgs[0].flags & I2C_M_TEN ? 10 : 7);
return -EINVAL;
}
for (i = 0; i < num; ++i){
if (unlikely(msgs[i].len <= 0)){
if (num == 1 && !msgs[0].len){
- /* Special case for I2C_SMBUS_QUICK emulation.
+ /*
+ * Special case for I2C_SMBUS_QUICK emulation.
* IBM IIC doesn't support 0-length transactions
* so we have to emulate them using bit-banging.
*/
return iic_smbus_quick(dev, &msgs[0]);
}
- DBG("%d: invalid len %d in msg[%d]\n", dev->idx,
+ DBG(dev, "invalid len %d in msg[%d]\n",
msgs[i].len, i);
return -EINVAL;
}
if (unlikely(iic_address_neq(&msgs[0], &msgs[i]))){
- DBG("%d: invalid addr in msg[%d]\n", dev->idx, i);
+ DBG(dev, "invalid addr in msg[%d]\n", i);
return -EINVAL;
}
}
/* Check bus state */
if (unlikely((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE)){
- DBG("%d: iic_xfer, bus is not free\n", dev->idx);
+ DBG(dev, "iic_xfer, bus is not free\n");
- /* Usually it means something serious has happened.
+ /*
+ * Usually it means something serious has happened.
* We *cannot* have unfinished previous transfer
* so it doesn't make any sense to try to stop it.
* Probably we were not able to recover from the
@@ -603,7 +610,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
iic_dev_reset(dev);
if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){
- DBG("%d: iic_xfer, bus is still not free\n", dev->idx);
+ DBG(dev, "iic_xfer, bus is still not free\n");
return -EREMOTEIO;
}
}
@@ -635,15 +642,18 @@ static const struct i2c_algorithm iic_algo = {
/*
* Calculates IICx_CLCKDIV value for a specific OPB clock frequency
*/
-static inline u8 iic_clckdiv(unsigned int opb)
+static inline u8 iic_clckdiv(struct ibm_iic_private *dev, unsigned int opb)
{
- /* Compatibility kludge, should go away after all cards
+ struct device *device = dev->adap.dev.parent;
+
+ /*
+ * Compatibility kludge, should go away after all cards
* are fixed to fill correct value for opbfreq.
* Previous driver version used hardcoded divider value 4,
* it corresponds to OPB frequency from the range (40, 50] MHz
*/
if (!opb){
- printk(KERN_WARNING "ibm-iic: using compatibility value for OPB freq,"
+ dev_warn(device, "iic_clckdiv: using compatibility value for OPB freq,"
" fix your board specific setup\n");
opb = 50000000;
}
@@ -652,7 +662,7 @@ static inline u8 iic_clckdiv(unsigned int opb)
opb /= 1000000;
if (opb < 20 || opb > 150){
- printk(KERN_WARNING "ibm-iic: invalid OPB clock frequency %u MHz\n",
+ dev_warn(device, "iic_clckdiv: invalid OPB clock frequency %u MHz\n",
opb);
opb = opb < 20 ? 20 : 150;
}
@@ -670,16 +680,17 @@ static int iic_request_irq(struct platform_device *ofdev,
irq = irq_of_parse_and_map(np, 0);
if (!irq) {
- dev_err(&ofdev->dev, "irq_of_parse_and_map failed\n");
+ ERR(dev, "irq_of_parse_and_map failed\n");
return 0;
}
- /* Disable interrupts until we finish initialization, assumes
+ /*
+ * Disable interrupts until we finish initialization, assumes
* level-sensitive IRQ setup...
*/
iic_interrupt_mode(dev, 0);
if (request_irq(irq, iic_handler, 0, "IBM IIC", dev)) {
- dev_err(&ofdev->dev, "request_irq %d failed\n", irq);
+ ERR(dev, "request_irq %d failed\n", irq);
/* Fallback to the polling mode */
return 0;
}
@@ -717,7 +728,7 @@ static int iic_probe(struct platform_device *ofdev)
dev->irq = iic_request_irq(ofdev, dev);
if (!dev->irq)
- dev_warn(&ofdev->dev, "using polling mode\n");
+ dev_info(&ofdev->dev, "using polling mode\n");
/* Board specific settings */
if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
@@ -733,7 +744,7 @@ static int iic_probe(struct platform_device *ofdev)
}
}
- dev->clckdiv = iic_clckdiv(*freq);
+ dev->clckdiv = iic_clckdiv(dev, *freq);
dev_dbg(&ofdev->dev, "clckdiv = %d\n", dev->clckdiv);
/* Initialize IIC interface */
diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h
index fdaa482..1efce5d 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.h
+++ b/drivers/i2c/busses/i2c-ibm_iic.h
@@ -25,8 +25,10 @@
#include <linux/i2c.h>
struct iic_regs {
- u16 mdbuf;
- u16 sbbuf;
+ u8 mdbuf;
+ u8 reserved1;
+ u8 sbbuf;
+ u8 reserved2;
u8 lmadr;
u8 hmadr;
u8 cntl;
@@ -44,9 +46,8 @@ struct iic_regs {
struct ibm_iic_private {
struct i2c_adapter adap;
- volatile struct iic_regs __iomem *vaddr;
+ struct iic_regs __iomem *vaddr;
wait_queue_head_t wq;
- int idx;
int irq;
int fast_mode;
u8 clckdiv;
--
1.8.4.2
^ permalink raw reply related
* [PATCH v3 REPOST 2/4] i2c: i2c-ibm-iic: perform the transfer in the interrupt handler
From: jean-jacques hiblot @ 2013-12-20 15:12 UTC (permalink / raw)
To: wsa
Cc: gregory.clement, jean-jacques hiblot, linuxppc-dev, linux-i2c,
jean-jacques hiblot
In-Reply-To: <1387552376-12986-1-git-send-email-jjhiblot@traphandler.com>
From: jean-jacques hiblot <jjhiblot@gmail.com>
The current implementation uses the interrupt only to wakeup the process doing
the data transfer. While this is working, it introduces indesirable latencies.
This patch implements the data transfer in the interrupt handler. It keeps the
latency between individual bytes low and the jitter on the total transfer time
is reduced.
Note: transfer abortion and polling mode are not working and will be supported
in further patches
This was tested on a custom board built around a PPC460EX with several
different I2C devices (including EEPROMs and smart batteries)
Signed-off-by: jean-jacques hiblot <jjhiblot@traphandler.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
drivers/i2c/busses/i2c-ibm_iic.c | 230 ++++++++++++++++++++++++++++-----------
drivers/i2c/busses/i2c-ibm_iic.h | 8 ++
2 files changed, 177 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index 03ab756..a92e8f6 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -58,6 +58,8 @@ static bool iic_force_fast;
module_param(iic_force_fast, bool, 0);
MODULE_PARM_DESC(iic_force_fast, "Force fast mode (400 kHz)");
+#define FIFO_FLUSH_TIMEOUT 100
+
#define DBG_LEVEL 0
#ifdef DBG
@@ -125,6 +127,7 @@ static struct i2c_timings {
.high = 600,
.buf = 1300,
}};
+static int iic_xfer_bytes(struct ibm_iic_private *dev);
/* Enable/disable interrupt generation */
static inline void iic_interrupt_mode(struct ibm_iic_private* dev, int enable)
@@ -167,8 +170,8 @@ static void iic_dev_init(struct ibm_iic_private* dev)
/* Clear control register */
out_8(&iic->cntl, 0);
- /* Enable interrupts if possible */
- iic_interrupt_mode(dev, dev->irq >= 0);
+ /* Start with each individual interrupt masked*/
+ iic_interrupt_mode(dev, 0);
/* Set mode control */
out_8(&iic->mdcntl, MDCNTL_FMDB | MDCNTL_EINT | MDCNTL_EUBS
@@ -327,16 +330,8 @@ err:
*/
static irqreturn_t iic_handler(int irq, void *dev_id)
{
- struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_id;
- struct iic_regs __iomem *iic = dev->vaddr;
-
- DBG2(dev, "irq handler, STS = 0x%02x, EXTSTS = 0x%02x\n",
- in_8(&iic->sts), in_8(&iic->extsts));
-
- /* Acknowledge IRQ and wakeup iic_wait_for_tc */
- out_8(&iic->sts, STS_IRQA | STS_SCMP);
- wake_up_interruptible(&dev->wq);
-
+ struct ibm_iic_private *dev = (struct ibm_iic_private *) dev_id;
+ iic_xfer_bytes(dev);
return IRQ_HANDLED;
}
@@ -460,60 +455,129 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){
/*
* Low level master transfer routine
*/
-static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm,
- int combined_xfer)
+static int iic_xfer_bytes(struct ibm_iic_private *dev)
{
- struct iic_regs __iomem *iic = dev->vaddr;
- char* buf = pm->buf;
- int i, j, loops, ret = 0;
- int len = pm->len;
-
- u8 cntl = (in_8(&iic->cntl) & CNTL_AMD) | CNTL_PT;
- if (pm->flags & I2C_M_RD)
- cntl |= CNTL_RW;
+ struct i2c_msg *pm = &(dev->msgs[dev->current_msg]);
+ struct iic_regs *iic = dev->vaddr;
+ u8 cntl = in_8(&iic->cntl) & CNTL_AMD;
+ int count;
+ u32 status;
+ u32 ext_status;
+
+ /* First check the status register */
+ status = in_8(&iic->sts);
+ ext_status = in_8(&iic->extsts);
+
+ /* and acknowledge the interrupt */
+ out_8(&iic->extsts, EXTSTS_IRQP | EXTSTS_IRQD | EXTSTS_LA |
+ EXTSTS_ICT | EXTSTS_XFRA);
+ out_8(&iic->sts, STS_IRQA | STS_SCMP);
- loops = (len + 3) / 4;
- for (i = 0; i < loops; ++i, len -= 4){
- int count = len > 4 ? 4 : len;
- u8 cmd = cntl | ((count - 1) << CNTL_TCT_SHIFT);
+ if ((status & STS_ERR) ||
+ (ext_status & (EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA))) {
+ DBG(dev, "status 0x%x\n", status);
+ DBG(dev, "extended status 0x%x\n", ext_status);
+ if (status & STS_ERR)
+ ERR(dev, "Error detected\n");
+ if (ext_status & EXTSTS_LA)
+ DBG(dev, "Lost arbitration\n");
+ if (ext_status & EXTSTS_ICT)
+ ERR(dev, "Incomplete transfer\n");
+ if (ext_status & EXTSTS_XFRA)
+ ERR(dev, "Transfer aborted\n");
+
+ dev->status = -EIO;
+ dev->transfer_complete = 1;
+ complete(&dev->iic_compl);
+ return dev->status;
+ }
- if (!(cntl & CNTL_RW))
- for (j = 0; j < count; ++j)
- out_8(&iic->mdbuf, *buf++);
+ if (dev->msgs == NULL) {
+ DBG(dev, "spurious !!!!!\n");
+ dev->status = -EINVAL;
+ return dev->status;
+ }
- if (i < loops - 1)
- cmd |= CNTL_CHT;
- else if (combined_xfer)
- cmd |= CNTL_RPST;
+ /* check if there's any data to read from the fifo */
+ if (pm->flags & I2C_M_RD) {
+ while (dev->current_byte_rx < dev->current_byte) {
+ u8 *buf = pm->buf + dev->current_byte_rx;
+ *buf = in_8(&iic->mdbuf);
+ dev->current_byte_rx++;
+ DBG2(dev, "read 0x%x\n", *buf);
+ }
+ }
+ /* check if we must go with the next tranfer */
+ if (pm->len == dev->current_byte) {
+ DBG2(dev, "going to next transfer\n");
+ dev->current_byte = 0;
+ dev->current_byte_rx = 0;
+ dev->current_msg++;
+ if (dev->current_msg == dev->num_msgs) {
+ DBG2(dev, "end of transfer\n");
+ dev->transfer_complete = 1;
+ complete(&dev->iic_compl);
+ return dev->status;
+ }
+ pm++;
+ }
- DBG2(dev, "xfer_bytes, %d, CNTL = 0x%02x\n", count, cmd);
+ /* take care of the direction */
+ if (pm->flags & I2C_M_RD)
+ cntl |= CNTL_RW;
- /* Start transfer */
- out_8(&iic->cntl, cmd);
+ /*
+ * how much data are we going to transfer this time ?
+ * (up to 4 bytes at once)
+ */
+ count = pm->len - dev->current_byte;
+ count = (count > 4) ? 4 : count;
+ cntl |= (count - 1) << CNTL_TCT_SHIFT;
+
+ if ((pm->flags & I2C_M_RD) == 0) {
+ /* This is a write. we must fill the fifo with the data */
+ int i;
+ u8 *buf = pm->buf + dev->current_byte;
+
+ for (i = 0; i < count; i++) {
+ out_8(&iic->mdbuf, buf[i]);
+ DBG2(dev, "write : 0x%x\n", buf[i]);
+ }
+ }
- /* Wait for completion */
- ret = iic_wait_for_tc(dev);
+ /* will the current transfer complete with this chunk of data ? */
+ if (pm->len > dev->current_byte + 4) {
+ /*
+ * we're not done with the current transfer.
+ * Don't send a repeated start
+ */
+ cntl |= CNTL_CHT;
+ }
+ /*
+ * This transfer will be complete with this chunk of data
+ * BUT is this the last transfer of the list ?
+ */
+ else if (dev->current_msg != (dev->num_msgs-1)) {
+ /* not the last tranfer */
+ cntl |= CNTL_RPST; /* Do not send a STOP condition */
+ /* check if the NEXT transfer needs a repeated start */
+ if (pm[1].flags & I2C_M_NOSTART)
+ cntl |= CNTL_CHT;
+ }
- if (unlikely(ret < 0))
- break;
- else if (unlikely(ret != count)){
- DBG(dev, "xfer_bytes, requested %d, transferred %d\n",
- count, ret);
+ if ((cntl & CNTL_RPST) == 0)
+ DBG2(dev, "STOP required\n");
- /* If it's not a last part of xfer, abort it */
- if (combined_xfer || (i < loops - 1))
- iic_abort_xfer(dev);
+ if ((cntl & CNTL_CHT) == 0)
+ DBG2(dev, "next transfer will begin with START\n");
- ret = -EREMOTEIO;
- break;
- }
+ /* keep track of the amount of data transfered (RX and TX) */
+ dev->current_byte += count;
- if (cntl & CNTL_RW)
- for (j = 0; j < count; ++j)
- *buf++ = in_8(&iic->mdbuf);
- }
+ /* actually start the transfer of the current data chunk */
+ out_8(&iic->cntl, cntl | CNTL_PT);
- return ret > 0 ? 0 : ret;
+ return 0;
}
/*
@@ -614,19 +678,63 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
return -EREMOTEIO;
}
}
- else {
- /* Flush master data buffer (just in case) */
- out_8(&iic->mdcntl, in_8(&iic->mdcntl) | MDCNTL_FMDB);
+
+ dev->current_byte = dev->current_msg = dev->current_byte_rx = 0;
+ dev->transfer_complete = 0;
+ dev->status = 0;
+ dev->msgs = msgs;
+ dev->num_msgs = num;
+
+ /* clear the buffers */
+ out_8(&iic->mdcntl, MDCNTL_FMDB);
+ i = FIFO_FLUSH_TIMEOUT;
+ while (in_8(&iic->mdcntl) & MDCNTL_FMDB) {
+ if (i-- < 0) {
+ DBG(dev, "iic_xfer, unable to flush\n");
+ return -EREMOTEIO;
+ }
}
+ /* clear all interrupts masks */
+ out_8(&iic->intmsk, 0x0);
+ /* clear any status */
+ out_8(&iic->sts, STS_IRQA | STS_SCMP);
+ out_8(&iic->extsts, EXTSTS_IRQP | EXTSTS_IRQD | EXTSTS_LA |
+ EXTSTS_ICT | EXTSTS_XFRA);
+
/* Load slave address */
iic_address(dev, &msgs[0]);
- /* Do real transfer */
- for (i = 0; i < num && !ret; ++i)
- ret = iic_xfer_bytes(dev, &msgs[i], i < num - 1);
+ init_completion(&dev->iic_compl);
+
+ /* start the transfer */
+ ret = iic_xfer_bytes(dev);
+
+ if (ret == 0) {
+ /* enable the interrupts */
+ out_8(&iic->mdcntl, MDCNTL_EINT);
+ /* unmask the interrupts */
+ out_8(&iic->intmsk, INTRMSK_EIMTC | INTRMSK_EITA |
+ INTRMSK_EIIC | INTRMSK_EIHE);
+ /* wait for the transfer to complete */
+ ret = wait_for_completion_interruptible_timeout(
+ &dev->iic_compl, num * HZ);
+ /* mask the interrupts */
+ out_8(&iic->intmsk, 0);
+ }
- return ret < 0 ? ret : num;
+ if (ret == 0) {
+ ERR(dev, "transfer timed out\n");
+ ret = -ETIMEDOUT;
+ } else if (ret < 0) {
+ if (ret == -ERESTARTSYS)
+ ERR(dev, "transfer interrupted\n");
+ } else {
+ /* Transfer is complete */
+ ret = (dev->status) ? dev->status : num;
+ }
+
+ return ret;
}
static u32 iic_func(struct i2c_adapter *adap)
diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h
index 1efce5d..76c476a 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.h
+++ b/drivers/i2c/busses/i2c-ibm_iic.h
@@ -51,6 +51,14 @@ struct ibm_iic_private {
int irq;
int fast_mode;
u8 clckdiv;
+ struct i2c_msg *msgs;
+ int num_msgs;
+ int current_msg;
+ int current_byte;
+ int current_byte_rx;
+ int transfer_complete;
+ int status;
+ struct completion iic_compl;
};
/* IICx_CNTL register */
--
1.8.4.2
^ permalink raw reply related
* [PATCH v3 REPOST 3/4] i2c: i2c-ibm-iic: Implements transfer abortion
From: jean-jacques hiblot @ 2013-12-20 15:12 UTC (permalink / raw)
To: wsa
Cc: gregory.clement, jean-jacques hiblot, linuxppc-dev, linux-i2c,
jean-jacques hiblot
In-Reply-To: <1387552376-12986-1-git-send-email-jjhiblot@traphandler.com>
From: jean-jacques hiblot <jjhiblot@gmail.com>
Clean-up properly when a transfer fails for whatever reason.
Cancel the transfer when the process is signaled.
Signed-off-by: jean-jacques hiblot <jjhiblot@traphandler.com>
---
drivers/i2c/busses/i2c-ibm_iic.c | 146 ++++++++++-----------------------------
drivers/i2c/busses/i2c-ibm_iic.h | 2 +-
2 files changed, 36 insertions(+), 112 deletions(-)
diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index a92e8f6..857259e 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -336,120 +336,25 @@ static irqreturn_t iic_handler(int irq, void *dev_id)
}
/*
- * Get master transfer result and clear errors if any.
- * Returns the number of actually transferred bytes or error (<0)
- */
-static int iic_xfer_result(struct ibm_iic_private* dev)
-{
- struct iic_regs __iomem *iic = dev->vaddr;
-
- if (unlikely(in_8(&iic->sts) & STS_ERR)){
- DBG(dev, "xfer error, EXTSTS = 0x%02x\n",
- in_8(&iic->extsts));
-
- /* Clear errors and possible pending IRQs */
- out_8(&iic->extsts, EXTSTS_IRQP | EXTSTS_IRQD |
- EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA);
-
- /* Flush master data buffer */
- out_8(&iic->mdcntl, in_8(&iic->mdcntl) | MDCNTL_FMDB);
-
- /*
- * Is bus free?
- * If error happened during combined xfer
- * IIC interface is usually stuck in some strange
- * state, the only way out - soft reset.
- */
- if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){
- DBG(dev, "bus is stuck, resetting\n");
- iic_dev_reset(dev);
- }
- return -EREMOTEIO;
- }
- else
- return in_8(&iic->xfrcnt) & XFRCNT_MTC_MASK;
-}
-
-/*
* Try to abort active transfer.
*/
-static void iic_abort_xfer(struct ibm_iic_private* dev)
+static void iic_abort_xfer(struct ibm_iic_private *dev)
{
- struct iic_regs __iomem *iic = dev->vaddr;
- unsigned long x;
-
- DBG(dev, "iic_abort_xfer\n");
+ struct device *device = dev->adap.dev.parent;
+ unsigned long end;
- out_8(&iic->cntl, CNTL_HMT);
+ DBG(dev, "aborting transfer\n");
+ /* transfer should be aborted within 10ms */
+ end = jiffies + 10;
+ dev->abort = 1;
- /*
- * Wait for the abort command to complete.
- * It's not worth to be optimized, just poll (timeout >= 1 tick)
- */
- x = jiffies + 2;
- while ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){
- if (time_after(jiffies, x)){
- DBG(dev, "abort timeout, resetting...\n");
- iic_dev_reset(dev);
- return;
- }
+ while (time_after(end, jiffies) && !dev->transfer_complete)
schedule();
- }
- /* Just to clear errors */
- iic_xfer_result(dev);
-}
-
-/*
- * Wait for master transfer to complete.
- * It puts current process to sleep until we get interrupt or timeout expires.
- * Returns the number of transferred bytes or error (<0)
- */
-static int iic_wait_for_tc(struct ibm_iic_private* dev){
-
- struct iic_regs __iomem *iic = dev->vaddr;
- int ret = 0;
-
- if (dev->irq >= 0){
- /* Interrupt mode */
- ret = wait_event_interruptible_timeout(dev->wq,
- !(in_8(&iic->sts) & STS_PT), dev->adap.timeout);
-
- if (unlikely(ret < 0))
- DBG(dev, "wait interrupted\n");
- else if (unlikely(in_8(&iic->sts) & STS_PT)){
- DBG(dev, "wait timeout\n");
- ret = -ETIMEDOUT;
- }
- }
- else {
- /* Polling mode */
- unsigned long x = jiffies + dev->adap.timeout;
-
- while (in_8(&iic->sts) & STS_PT){
- if (unlikely(time_after(jiffies, x))){
- DBG(dev, "poll timeout\n");
- ret = -ETIMEDOUT;
- break;
- }
-
- if (unlikely(signal_pending(current))){
- DBG(dev, "poll interrupted\n");
- ret = -ERESTARTSYS;
- break;
- }
- schedule();
- }
+ if (!dev->transfer_complete) {
+ dev_err(device, "abort operation failed\n");
+ iic_dev_reset(dev);
}
-
- if (unlikely(ret < 0))
- iic_abort_xfer(dev);
- else
- ret = iic_xfer_result(dev);
-
- DBG2(dev, "iic_wait_for_tc -> %d\n", ret);
-
- return ret;
}
/*
@@ -473,6 +378,13 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev)
EXTSTS_ICT | EXTSTS_XFRA);
out_8(&iic->sts, STS_IRQA | STS_SCMP);
+ if (dev->status == -ECANCELED) {
+ DBG(dev, "abort completed\n");
+ dev->transfer_complete = 1;
+ complete(&dev->iic_compl);
+ return dev->status;
+ }
+
if ((status & STS_ERR) ||
(ext_status & (EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA))) {
DBG(dev, "status 0x%x\n", status);
@@ -577,7 +489,14 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev)
/* actually start the transfer of the current data chunk */
out_8(&iic->cntl, cntl | CNTL_PT);
- return 0;
+ /* The transfer must be aborted. */
+ if (dev->abort) {
+ DBG(dev, "aborting\n");
+ out_8(&iic->cntl, CNTL_HMT);
+ dev->status = -ECANCELED;
+ }
+
+ return dev->status;
}
/*
@@ -682,6 +601,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
dev->current_byte = dev->current_msg = dev->current_byte_rx = 0;
dev->transfer_complete = 0;
dev->status = 0;
+ dev->abort = 0;
dev->msgs = msgs;
dev->num_msgs = num;
@@ -719,8 +639,10 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
/* wait for the transfer to complete */
ret = wait_for_completion_interruptible_timeout(
&dev->iic_compl, num * HZ);
- /* mask the interrupts */
- out_8(&iic->intmsk, 0);
+ /*
+ * we don't mask the interrupts here because we may
+ * need them to abort the transfer gracefully
+ */
}
if (ret == 0) {
@@ -729,11 +651,15 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
} else if (ret < 0) {
if (ret == -ERESTARTSYS)
ERR(dev, "transfer interrupted\n");
+ iic_abort_xfer(dev);
} else {
/* Transfer is complete */
ret = (dev->status) ? dev->status : num;
}
+ /* mask the interrupts */
+ out_8(&iic->intmsk, 0);
+
return ret;
}
@@ -832,8 +758,6 @@ static int iic_probe(struct platform_device *ofdev)
goto error_cleanup;
}
- init_waitqueue_head(&dev->wq);
-
dev->irq = iic_request_irq(ofdev, dev);
if (!dev->irq)
dev_info(&ofdev->dev, "using polling mode\n");
diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h
index 76c476a..0ee28a9 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.h
+++ b/drivers/i2c/busses/i2c-ibm_iic.h
@@ -47,7 +47,6 @@ struct iic_regs {
struct ibm_iic_private {
struct i2c_adapter adap;
struct iic_regs __iomem *vaddr;
- wait_queue_head_t wq;
int irq;
int fast_mode;
u8 clckdiv;
@@ -58,6 +57,7 @@ struct ibm_iic_private {
int current_byte_rx;
int transfer_complete;
int status;
+ int abort;
struct completion iic_compl;
};
--
1.8.4.2
^ permalink raw reply related
* [PATCH v3 REPOST 4/4] i2c: i2c-ibm-iic: Implements a polling mode
From: jean-jacques hiblot @ 2013-12-20 15:12 UTC (permalink / raw)
To: wsa
Cc: gregory.clement, jean-jacques hiblot, linuxppc-dev, linux-i2c,
jean-jacques hiblot
In-Reply-To: <1387552376-12986-1-git-send-email-jjhiblot@traphandler.com>
From: jean-jacques hiblot <jjhiblot@gmail.com>
When no valid interrupt is defined for the controller, use polling to handle
the transfers.
The polling mode can also be forced with the "iic_force_poll" module parameter.
Signed-off-by: jean-jacques hiblot <jjhiblot@traphandler.com>
---
drivers/i2c/busses/i2c-ibm_iic.c | 91 ++++++++++++++++++++++++++++++++--------
drivers/i2c/busses/i2c-ibm_iic.h | 1 +
2 files changed, 74 insertions(+), 18 deletions(-)
diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index 857259e..aefe228 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -336,11 +336,45 @@ static irqreturn_t iic_handler(int irq, void *dev_id)
}
/*
+ * Polling used when interrupt can't be used
+ */
+static int poll_for_end_of_transfer(struct ibm_iic_private *dev, u32 timeout)
+{
+ struct iic_regs __iomem *iic = dev->vaddr;
+ u32 status;
+ unsigned long end = jiffies + timeout;
+
+ while ((dev->transfer_complete == 0) &&
+ time_after(end, jiffies) &&
+ !signal_pending(current)) {
+ status = in_8(&iic->sts);
+ /* check if the transfer is done or an error occured */
+ if ((status & (STS_ERR | STS_SCMP)) || !(status & STS_PT))
+ iic_xfer_bytes(dev);
+ /* The transfer is not complete,
+ * calling schedule relaxes the CPU load and allows to know
+ * if the process is being signaled (for abortion)
+ */
+ if (dev->transfer_complete == 0)
+ schedule();
+ }
+
+ if (signal_pending(current))
+ return -ERESTARTSYS;
+
+ if (dev->transfer_complete == 0)
+ return 0;
+
+ return 1;
+}
+
+/*
* Try to abort active transfer.
*/
static void iic_abort_xfer(struct ibm_iic_private *dev)
{
struct device *device = dev->adap.dev.parent;
+ struct iic_regs __iomem *iic = dev->vaddr;
unsigned long end;
DBG(dev, "aborting transfer\n");
@@ -348,8 +382,17 @@ static void iic_abort_xfer(struct ibm_iic_private *dev)
end = jiffies + 10;
dev->abort = 1;
- while (time_after(end, jiffies) && !dev->transfer_complete)
- schedule();
+ while (time_after(end, jiffies) && !dev->transfer_complete) {
+ u32 sts;
+ if (dev->use_polling) {
+ sts = in_8(&iic->sts);
+ /* check if the transfer is done or an error occured */
+ if ((sts & (STS_ERR | STS_SCMP)) || !(sts & STS_PT))
+ iic_xfer_bytes(dev);
+ }
+ if (dev->transfer_complete == 0)
+ schedule();
+ }
if (!dev->transfer_complete) {
dev_err(device, "abort operation failed\n");
@@ -381,7 +424,8 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev)
if (dev->status == -ECANCELED) {
DBG(dev, "abort completed\n");
dev->transfer_complete = 1;
- complete(&dev->iic_compl);
+ if (!dev->use_polling)
+ complete(&dev->iic_compl);
return dev->status;
}
@@ -400,7 +444,8 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev)
dev->status = -EIO;
dev->transfer_complete = 1;
- complete(&dev->iic_compl);
+ if (!dev->use_polling)
+ complete(&dev->iic_compl);
return dev->status;
}
@@ -428,7 +473,8 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev)
if (dev->current_msg == dev->num_msgs) {
DBG2(dev, "end of transfer\n");
dev->transfer_complete = 1;
- complete(&dev->iic_compl);
+ if (!dev->use_polling)
+ complete(&dev->iic_compl);
return dev->status;
}
pm++;
@@ -625,24 +671,29 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
/* Load slave address */
iic_address(dev, &msgs[0]);
- init_completion(&dev->iic_compl);
+ if (!dev->use_polling)
+ init_completion(&dev->iic_compl);
/* start the transfer */
ret = iic_xfer_bytes(dev);
if (ret == 0) {
- /* enable the interrupts */
- out_8(&iic->mdcntl, MDCNTL_EINT);
- /* unmask the interrupts */
- out_8(&iic->intmsk, INTRMSK_EIMTC | INTRMSK_EITA |
- INTRMSK_EIIC | INTRMSK_EIHE);
- /* wait for the transfer to complete */
- ret = wait_for_completion_interruptible_timeout(
- &dev->iic_compl, num * HZ);
- /*
- * we don't mask the interrupts here because we may
- * need them to abort the transfer gracefully
- */
+ if (dev->use_polling) {
+ ret = poll_for_end_of_transfer(dev, num * HZ);
+ } else {
+ /* enable the interrupts */
+ out_8(&iic->mdcntl, MDCNTL_EINT);
+ /* unmask the interrupts */
+ out_8(&iic->intmsk, INTRMSK_EIMTC | INTRMSK_EITA |
+ INTRMSK_EIIC | INTRMSK_EIHE);
+ /* wait for the transfer to complete */
+ ret = wait_for_completion_interruptible_timeout(
+ &dev->iic_compl, num * HZ);
+ /*
+ * we don't mask the interrupts here because we may
+ * need them to abort the transfer gracefully
+ */
+ }
}
if (ret == 0) {
@@ -709,6 +760,8 @@ static int iic_request_irq(struct platform_device *ofdev,
struct device_node *np = ofdev->dev.of_node;
int irq;
+ dev->use_polling = 1;
+
if (iic_force_poll)
return 0;
@@ -729,6 +782,8 @@ static int iic_request_irq(struct platform_device *ofdev,
return 0;
}
+ dev->use_polling = 0;
+
return irq;
}
diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h
index 0ee28a9..523cfc1 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.h
+++ b/drivers/i2c/busses/i2c-ibm_iic.h
@@ -58,6 +58,7 @@ struct ibm_iic_private {
int transfer_complete;
int status;
int abort;
+ int use_polling;
struct completion iic_compl;
};
--
1.8.4.2
^ permalink raw reply related
* Re: [RFC] linux/pci: move pci_platform_pm_ops to linux/pci.h
From: Bjorn Helgaas @ 2013-12-20 16:42 UTC (permalink / raw)
To: Dongsheng Wang
Cc: Linux PM list, roy.zang, Rafael J. Wysocki,
linux-pci@vger.kernel.org, Scott Wood, linuxppc-dev
In-Reply-To: <1387533832-34554-1-git-send-email-dongsheng.wang@freescale.com>
On Fri, Dec 20, 2013 at 3:03 AM, Dongsheng Wang
<dongsheng.wang@freescale.com> wrote:
> From: Wang Dongsheng <dongsheng.wang@freescale.com>
>
> make Freescale platform use pci_platform_pm_ops struct.
This changelog doesn't say anything about what the patch does.
I infer that you want to use pci_platform_pm_ops from some Freescale
code. This patch should be posted along with the patches that add
that Freescale code, so we can see how you intend to use it.
The existing use is in drivers/pci/pci-acpi.c, so it's possible that
your new use should be added in the same way, in drivers/pci, so we
don't have to make pci_platform_pm_ops part of the public PCI
interface in include/linux/pci.h.
That said, if Raphael thinks this makes sense, it's OK with me.
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> ---
>
> If device's not set power state, we will use this interface to put the
> device's into the correct state.
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9c91ecc..48f8b1a 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -33,36 +33,6 @@ int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
> #endif
> int pci_probe_reset_function(struct pci_dev *dev);
>
> -/**
> - * struct pci_platform_pm_ops - Firmware PM callbacks
> - *
> - * @is_manageable: returns 'true' if given device is power manageable by the
> - * platform firmware
> - *
> - * @set_state: invokes the platform firmware to set the device's power state
> - *
> - * @choose_state: returns PCI power state of given device preferred by the
> - * platform; to be used during system-wide transitions from a
> - * sleeping state to the working state and vice versa
> - *
> - * @sleep_wake: enables/disables the system wake up capability of given device
> - *
> - * @run_wake: enables/disables the platform to generate run-time wake-up events
> - * for given device (the device's wake-up capability has to be
> - * enabled by @sleep_wake for this feature to work)
> - *
> - * If given platform is generally capable of power managing PCI devices, all of
> - * these callbacks are mandatory.
> - */
> -struct pci_platform_pm_ops {
> - bool (*is_manageable)(struct pci_dev *dev);
> - int (*set_state)(struct pci_dev *dev, pci_power_t state);
> - pci_power_t (*choose_state)(struct pci_dev *dev);
> - int (*sleep_wake)(struct pci_dev *dev, bool enable);
> - int (*run_wake)(struct pci_dev *dev, bool enable);
> -};
> -
> -int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
> void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
> void pci_power_up(struct pci_dev *dev);
> void pci_disable_enabled_device(struct pci_dev *dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1084a15..20e07b8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -365,6 +365,37 @@ struct pci_dev {
> size_t romlen; /* Length of ROM if it's not from the BAR */
> };
>
> +/**
> + * struct pci_platform_pm_ops - Firmware PM callbacks
> + *
> + * @is_manageable: returns 'true' if given device is power manageable by the
> + * platform firmware
> + *
> + * @set_state: invokes the platform firmware to set the device's power state
> + *
> + * @choose_state: returns PCI power state of given device preferred by the
> + * platform; to be used during system-wide transitions from a
> + * sleeping state to the working state and vice versa
> + *
> + * @sleep_wake: enables/disables the system wake up capability of given device
> + *
> + * @run_wake: enables/disables the platform to generate run-time wake-up events
> + * for given device (the device's wake-up capability has to be
> + * enabled by @sleep_wake for this feature to work)
> + *
> + * If given platform is generally capable of power managing PCI devices, all of
> + * these callbacks are mandatory.
> + */
> +struct pci_platform_pm_ops {
> + bool (*is_manageable)(struct pci_dev *dev);
> + int (*set_state)(struct pci_dev *dev, pci_power_t state);
> + pci_power_t (*choose_state)(struct pci_dev *dev);
> + int (*sleep_wake)(struct pci_dev *dev, bool enable);
> + int (*run_wake)(struct pci_dev *dev, bool enable);
> +};
> +
> +int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
> +
> static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> {
> #ifdef CONFIG_PCI_IOV
> --
> 1.8.5
>
>
^ permalink raw reply
* Re: [PATCH] powernv: eeh: fix possible buffer overrun in ioda_eeh_phb_diag()
From: Brian W Hart @ 2013-12-20 18:15 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20131220015937.GA13868@shangw.(null)>
On Fri, Dec 20, 2013 at 09:59:37AM +0800, Gavin Shan wrote:
> On Fri, Dec 20, 2013 at 09:35:39AM +0800, Gavin Shan wrote:
> >On Thu, Dec 19, 2013 at 05:14:07PM -0600, Brian W Hart wrote:
> >>PHB diagnostic buffer may be smaller than PAGE_SIZE, especially when
> >>PAGE_SIZE > 4KB.
> >>
> >
> >I think you're talking about that PAGE_SIZE could be configured
> >to have variable size (e.g. 4KB). So it's not safe to pass PAGE_SIZE
> >to OPAL API opal_pci_get_phb_diag_data2(). Instead, we should pass
> >PNV_PCI_DIAG_BUF_SIZE and it makes sense to me :-)
Yeah, I noticed the problem because our test machine has PAGE_SIZE of
64K with the buffer only being 8K.
[...]
> Sorry, Brian. It has been fixed as part of the following commit, which
> has been put into Ben's powerpc-next branch :-)
Thank you!
^ permalink raw reply
* Re: [PATCH] powernv: eeh: add buffer for P7IOC hub error data
From: Brian W Hart @ 2013-12-20 18:17 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20131220014504.GB10795@shangw.(null)>
On Fri, Dec 20, 2013 at 09:45:04AM +0800, Gavin Shan wrote:
> On Thu, Dec 19, 2013 at 05:18:53PM -0600, Brian W Hart wrote:
> >Prevent ioda_eeh_hub_diag() from clobbering itself when called by supplying
> >a buffer for P7IOC hub diagnostic data. Take care to inform OPAL of the
> >correct size for the buffer.
> >
> >Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
> >---
> >
> >I hope I've understood this correctly. It looks to me like
> >ioda_eeh_hub_data is effectively asking OPAL to clobber its own
> >text (via 'data') when it makes the call to retrieve the hub data.
> >
>
> Yeah, we should have used following variable as HUB diag-data instead.
>
> static char *hub_diag = NULL;
>
> However, it's not safe to allocate page-sized buffer for "hub_diag".
>
> >Added a hub diagnostic structure per-phb. Perhaps the diagnostic
> >structure better belongs in the phb->diag union, but I wasn't sure whether
> >we'd need to carry the hub and PHB diag data at the same time.
> >
>
> Please put hub diag-data to struct pnv_phb::diag since we don't need
> carry hub and PHB diag-data at same time. With it, please remove
> variable "hub_diag" as well.
Thanks; will send another patch.
^ permalink raw reply
* [PATCH v2 0/9] cpuidle: rework device state count handling
From: Bartlomiej Zolnierkiewicz @ 2013-12-20 18:47 UTC (permalink / raw)
To: rjw
Cc: linux-samsung-soc, linux-pm, b.zolnierkie, daniel.lezcano,
linux-kernel, kyungmin.park, linuxppc-dev, lenb
Hi,
Some cpuidle drivers assume that cpuidle core will handle cases where
device->state_count is smaller than driver->state_count, unfortunately
currently this is untrue (device->state_count is used only for handling
cpuidle state sysfs entries and driver->state_count is used for all
other cases) and will not be fixed in the future as device->state_count
is planned to be removed [1].
This patchset fixes such drivers (ARM EXYNOS cpuidle driver and ACPI
cpuidle driver), removes superflous device->state_count initialization
from drivers for which device->state_count equals driver->state_count
(POWERPC pseries cpuidle driver and intel_idle driver) and finally
removes state_count field from struct cpuidle_device.
Additionaly (while at it) this patchset fixes C1E promotion disable
quirk handling (in intel_idle driver) and converts cpuidle drivers code
to use the common cpuidle_[un]register() routines (in POWERPC pseries
cpuidle driver and intel_idle driver).
[1] http://permalink.gmane.org/gmane.linux.power-management.general/36908
Reference to v1:
http://comments.gmane.org/gmane.linux.power-management.general/37390
Changes since v1:
- synced patch series with next-20131220
- added ACKs from Daniel Lezcano
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Bartlomiej Zolnierkiewicz (9):
ARM: EXYNOS: cpuidle: fix AFTR mode check
POWERPC: pseries: cpuidle: remove superfluous dev->state_count
initialization
POWERPC: pseries: cpuidle: use the common cpuidle_[un]register()
routines
ACPI / cpuidle: fix max idle state handling with hotplug CPU support
ACPI / cpuidle: remove dev->state_count setting
intel_idle: do C1E promotion disable quirk for hotplugged CPUs
intel_idle: remove superfluous dev->state_count initialization
intel_idle: use the common cpuidle_[un]register() routines
cpuidle: remove state_count field from struct cpuidle_device
arch/arm/mach-exynos/cpuidle.c | 8 +-
arch/powerpc/platforms/pseries/processor_idle.c | 59 +---------
drivers/acpi/processor_idle.c | 29 +++--
drivers/cpuidle/cpuidle.c | 3 -
drivers/cpuidle/sysfs.c | 5 +-
drivers/idle/intel_idle.c | 140 +++++-------------------
include/linux/cpuidle.h | 1 -
7 files changed, 51 insertions(+), 194 deletions(-)
--
1.8.2.3
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox