| Message ID | 20190325093947.32633-13-kishon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for PCIe RC and EP mode in TI's AM654 SoC | expand |
On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote: > hook_fault_code is an ARM32 specific API for hooking into data abort. > Since pci-keystone.c will be used for AM65X platforms which is an > ARM64 platform, Hi Kishon, How is the problem plugged by the fault hook fixed on ARM64 ? Thanks, Lorenzo > allow hook_fault_code to be compiled only for ARM32. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/pci/controller/dwc/pci-keystone.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index dfe54553d832..93296d434f40 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) > return ret; > } > > +#ifdef CONFIG_ARM > /* > * When a PCI device does not exist during config cycles, keystone host gets a > * bus error instead of returning 0xffffffff. This handler always returns 0 > @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr, > > return 0; > } > +#endif > > static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie) > { > @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) > if (ret < 0) > return ret; > > +#ifdef CONFIG_ARM > /* > * PCIe access errors that result into OCP errors are caught by ARM as > * "External aborts" > */ > hook_fault_code(17, ks_pcie_fault, SIGBUS, 0, > "Asynchronous external abort"); > +#endif > > ks_pcie_start_link(pci); > dw_pcie_wait_for_link(pci); > -- > 2.17.1 >
Hi Lorenzo, On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote: > On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote: >> hook_fault_code is an ARM32 specific API for hooking into data abort. >> Since pci-keystone.c will be used for AM65X platforms which is an >> ARM64 platform, > > Hi Kishon, > > How is the problem plugged by the fault hook fixed on ARM64 ? At least in AM654 platform, I don't see a bus error when PCIe device is not connected but returns 0xffffffff. So there is no necessary for hook_fault_code in AM654 platform. Thanks Kishon > > Thanks, > Lorenzo > >> allow hook_fault_code to be compiled only for ARM32. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> drivers/pci/controller/dwc/pci-keystone.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >> index dfe54553d832..93296d434f40 100644 >> --- a/drivers/pci/controller/dwc/pci-keystone.c >> +++ b/drivers/pci/controller/dwc/pci-keystone.c >> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) >> return ret; >> } >> >> +#ifdef CONFIG_ARM >> /* >> * When a PCI device does not exist during config cycles, keystone host gets a >> * bus error instead of returning 0xffffffff. This handler always returns 0 >> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr, >> >> return 0; >> } >> +#endif >> >> static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie) >> { >> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) >> if (ret < 0) >> return ret; >> >> +#ifdef CONFIG_ARM >> /* >> * PCIe access errors that result into OCP errors are caught by ARM as >> * "External aborts" >> */ >> hook_fault_code(17, ks_pcie_fault, SIGBUS, 0, >> "Asynchronous external abort"); >> +#endif >> >> ks_pcie_start_link(pci); >> dw_pcie_wait_for_link(pci); >> -- >> 2.17.1 >>
On Fri, Apr 12, 2019 at 02:20:06PM +0530, Kishon Vijay Abraham I wrote: > Hi Lorenzo, > > On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote: > > On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote: > >> hook_fault_code is an ARM32 specific API for hooking into data abort. > >> Since pci-keystone.c will be used for AM65X platforms which is an > >> ARM64 platform, > > > > Hi Kishon, > > > > How is the problem plugged by the fault hook fixed on ARM64 ? > > At least in AM654 platform, I don't see a bus error when PCIe device is not > connected but returns 0xffffffff. So there is no necessary for hook_fault_code > in AM654 platform. ... which needs to be explained somewhere, either in the commit message or comments in the file, so that years later when someone has to look at this code, they know why it is the way that it is. When writing commits, please think about whether there is enough information to understand the change in ten years time. > > Thanks > Kishon > > > > > Thanks, > > Lorenzo > > > >> allow hook_fault_code to be compiled only for ARM32. > >> > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >> --- > >> drivers/pci/controller/dwc/pci-keystone.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > >> index dfe54553d832..93296d434f40 100644 > >> --- a/drivers/pci/controller/dwc/pci-keystone.c > >> +++ b/drivers/pci/controller/dwc/pci-keystone.c > >> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) > >> return ret; > >> } > >> > >> +#ifdef CONFIG_ARM > >> /* > >> * When a PCI device does not exist during config cycles, keystone host gets a > >> * bus error instead of returning 0xffffffff. This handler always returns 0 > >> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr, > >> > >> return 0; > >> } > >> +#endif > >> > >> static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie) > >> { > >> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) > >> if (ret < 0) > >> return ret; > >> > >> +#ifdef CONFIG_ARM > >> /* > >> * PCIe access errors that result into OCP errors are caught by ARM as > >> * "External aborts" > >> */ > >> hook_fault_code(17, ks_pcie_fault, SIGBUS, 0, > >> "Asynchronous external abort"); > >> +#endif > >> > >> ks_pcie_start_link(pci); > >> dw_pcie_wait_for_link(pci); > >> -- > >> 2.17.1 > >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi Russell, On 12/04/19 3:01 PM, Russell King - ARM Linux admin wrote: > On Fri, Apr 12, 2019 at 02:20:06PM +0530, Kishon Vijay Abraham I wrote: >> Hi Lorenzo, >> >> On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote: >>> On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote: >>>> hook_fault_code is an ARM32 specific API for hooking into data abort. >>>> Since pci-keystone.c will be used for AM65X platforms which is an >>>> ARM64 platform, >>> >>> Hi Kishon, >>> >>> How is the problem plugged by the fault hook fixed on ARM64 ? >> >> At least in AM654 platform, I don't see a bus error when PCIe device is not >> connected but returns 0xffffffff. So there is no necessary for hook_fault_code >> in AM654 platform. > > ... which needs to be explained somewhere, either in the commit > message or comments in the file, so that years later when someone > has to look at this code, they know why it is the way that it is. > > When writing commits, please think about whether there is enough > information to understand the change in ten years time. sure, I'll fix this one. Lorenzo, let me know if you want me to resend with updated commit log. Thanks Kishon > >> >> Thanks >> Kishon >> >>> >>> Thanks, >>> Lorenzo >>> >>>> allow hook_fault_code to be compiled only for ARM32. >>>> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> --- >>>> drivers/pci/controller/dwc/pci-keystone.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >>>> index dfe54553d832..93296d434f40 100644 >>>> --- a/drivers/pci/controller/dwc/pci-keystone.c >>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c >>>> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) >>>> return ret; >>>> } >>>> >>>> +#ifdef CONFIG_ARM >>>> /* >>>> * When a PCI device does not exist during config cycles, keystone host gets a >>>> * bus error instead of returning 0xffffffff. This handler always returns 0 >>>> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr, >>>> >>>> return 0; >>>> } >>>> +#endif >>>> >>>> static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie) >>>> { >>>> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) >>>> if (ret < 0) >>>> return ret; >>>> >>>> +#ifdef CONFIG_ARM >>>> /* >>>> * PCIe access errors that result into OCP errors are caught by ARM as >>>> * "External aborts" >>>> */ >>>> hook_fault_code(17, ks_pcie_fault, SIGBUS, 0, >>>> "Asynchronous external abort"); >>>> +#endif >>>> >>>> ks_pcie_start_link(pci); >>>> dw_pcie_wait_for_link(pci); >>>> -- >>>> 2.17.1 >>>> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >
On Fri, Apr 12, 2019 at 02:20:06PM +0530, Kishon Vijay Abraham I wrote: > Hi Lorenzo, > > On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote: > > On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote: > >> hook_fault_code is an ARM32 specific API for hooking into data abort. > >> Since pci-keystone.c will be used for AM65X platforms which is an > >> ARM64 platform, > > > > Hi Kishon, > > > > How is the problem plugged by the fault hook fixed on ARM64 ? > > At least in AM654 platform, I don't see a bus error when PCIe device > is not connected but returns 0xffffffff. So there is no necessary for > hook_fault_code in AM654 platform. That can't have much to do with ARM32<->ARM64, it is rather a platform integration issue AFAICS. Russell has a point, this has to be documented I can do it for you but I need additional information. Thanks, Lorenzo > Thanks > Kishon > > > > > Thanks, > > Lorenzo > > > >> allow hook_fault_code to be compiled only for ARM32. > >> > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >> --- > >> drivers/pci/controller/dwc/pci-keystone.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > >> index dfe54553d832..93296d434f40 100644 > >> --- a/drivers/pci/controller/dwc/pci-keystone.c > >> +++ b/drivers/pci/controller/dwc/pci-keystone.c > >> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) > >> return ret; > >> } > >> > >> +#ifdef CONFIG_ARM > >> /* > >> * When a PCI device does not exist during config cycles, keystone host gets a > >> * bus error instead of returning 0xffffffff. This handler always returns 0 > >> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr, > >> > >> return 0; > >> } > >> +#endif > >> > >> static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie) > >> { > >> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) > >> if (ret < 0) > >> return ret; > >> > >> +#ifdef CONFIG_ARM > >> /* > >> * PCIe access errors that result into OCP errors are caught by ARM as > >> * "External aborts" > >> */ > >> hook_fault_code(17, ks_pcie_fault, SIGBUS, 0, > >> "Asynchronous external abort"); > >> +#endif > >> > >> ks_pcie_start_link(pci); > >> dw_pcie_wait_for_link(pci); > >> -- > >> 2.17.1 > >>
Hi Lorenzo, On 12/04/19 4:41 PM, Lorenzo Pieralisi wrote: > On Fri, Apr 12, 2019 at 02:20:06PM +0530, Kishon Vijay Abraham I wrote: >> Hi Lorenzo, >> >> On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote: >>> On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote: >>>> hook_fault_code is an ARM32 specific API for hooking into data abort. >>>> Since pci-keystone.c will be used for AM65X platforms which is an >>>> ARM64 platform, >>> >>> Hi Kishon, >>> >>> How is the problem plugged by the fault hook fixed on ARM64 ? >> >> At least in AM654 platform, I don't see a bus error when PCIe device >> is not connected but returns 0xffffffff. So there is no necessary for >> hook_fault_code in AM654 platform. > > That can't have much to do with ARM32<->ARM64, it is rather a platform > integration issue AFAICS. Russell has a point, this has to be documented > I can do it for you but I need additional information. Right now only ARM32 exports hook_fault_code which was used by K2G (ARM32 platform) and since AM654 (ARM64 platform) uses the same driver, it'll result in compilation error. Also AM654 doesn't require hook_fault_code (or something similar) because it doesn't throw bus error on accessing PCIe address space when PCI device is not connected). Are you looking for some other information? Thanks Kishon > > Thanks, > Lorenzo > >> Thanks >> Kishon >> >>> >>> Thanks, >>> Lorenzo >>> >>>> allow hook_fault_code to be compiled only for ARM32. >>>> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> --- >>>> drivers/pci/controller/dwc/pci-keystone.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >>>> index dfe54553d832..93296d434f40 100644 >>>> --- a/drivers/pci/controller/dwc/pci-keystone.c >>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c >>>> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) >>>> return ret; >>>> } >>>> >>>> +#ifdef CONFIG_ARM >>>> /* >>>> * When a PCI device does not exist during config cycles, keystone host gets a >>>> * bus error instead of returning 0xffffffff. This handler always returns 0 >>>> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr, >>>> >>>> return 0; >>>> } >>>> +#endif >>>> >>>> static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie) >>>> { >>>> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) >>>> if (ret < 0) >>>> return ret; >>>> >>>> +#ifdef CONFIG_ARM >>>> /* >>>> * PCIe access errors that result into OCP errors are caught by ARM as >>>> * "External aborts" >>>> */ >>>> hook_fault_code(17, ks_pcie_fault, SIGBUS, 0, >>>> "Asynchronous external abort"); >>>> +#endif >>>> >>>> ks_pcie_start_link(pci); >>>> dw_pcie_wait_for_link(pci); >>>> -- >>>> 2.17.1 >>>>
On Fri, Apr 12, 2019 at 04:59:36PM +0530, Kishon Vijay Abraham I wrote: > Hi Lorenzo, > > On 12/04/19 4:41 PM, Lorenzo Pieralisi wrote: > > On Fri, Apr 12, 2019 at 02:20:06PM +0530, Kishon Vijay Abraham I wrote: > >> Hi Lorenzo, > >> > >> On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote: > >>> On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote: > >>>> hook_fault_code is an ARM32 specific API for hooking into data abort. > >>>> Since pci-keystone.c will be used for AM65X platforms which is an > >>>> ARM64 platform, > >>> > >>> Hi Kishon, > >>> > >>> How is the problem plugged by the fault hook fixed on ARM64 ? > >> > >> At least in AM654 platform, I don't see a bus error when PCIe device > >> is not connected but returns 0xffffffff. So there is no necessary for > >> hook_fault_code in AM654 platform. > > > > That can't have much to do with ARM32<->ARM64, it is rather a platform > > integration issue AFAICS. Russell has a point, this has to be documented > > I can do it for you but I need additional information. > > Right now only ARM32 exports hook_fault_code which was used by K2G (ARM32 > platform) and since AM654 (ARM64 platform) uses the same driver, it'll result > in compilation error. Also AM654 doesn't require hook_fault_code (or something > similar) because it doesn't throw bus error on accessing PCIe address space > when PCI device is not connected). > > Are you looking for some other information? No thanks it was to confirm my understanding, it is really not an ARM<->ARM64 issue, I will try to phrase it in a way that is more explicit. Lorenzo > > Thanks > Kishon > > > > Thanks, > > Lorenzo > > > >> Thanks > >> Kishon > >> > >>> > >>> Thanks, > >>> Lorenzo > >>> > >>>> allow hook_fault_code to be compiled only for ARM32. > >>>> > >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >>>> --- > >>>> drivers/pci/controller/dwc/pci-keystone.c | 4 ++++ > >>>> 1 file changed, 4 insertions(+) > >>>> > >>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > >>>> index dfe54553d832..93296d434f40 100644 > >>>> --- a/drivers/pci/controller/dwc/pci-keystone.c > >>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c > >>>> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) > >>>> return ret; > >>>> } > >>>> > >>>> +#ifdef CONFIG_ARM > >>>> /* > >>>> * When a PCI device does not exist during config cycles, keystone host gets a > >>>> * bus error instead of returning 0xffffffff. This handler always returns 0 > >>>> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr, > >>>> > >>>> return 0; > >>>> } > >>>> +#endif > >>>> > >>>> static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie) > >>>> { > >>>> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) > >>>> if (ret < 0) > >>>> return ret; > >>>> > >>>> +#ifdef CONFIG_ARM > >>>> /* > >>>> * PCIe access errors that result into OCP errors are caught by ARM as > >>>> * "External aborts" > >>>> */ > >>>> hook_fault_code(17, ks_pcie_fault, SIGBUS, 0, > >>>> "Asynchronous external abort"); > >>>> +#endif > >>>> > >>>> ks_pcie_start_link(pci); > >>>> dw_pcie_wait_for_link(pci); > >>>> -- > >>>> 2.17.1 > >>>>
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index dfe54553d832..93296d434f40 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) return ret; } +#ifdef CONFIG_ARM /* * When a PCI device does not exist during config cycles, keystone host gets a * bus error instead of returning 0xffffffff. This handler always returns 0 @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr, return 0; } +#endif static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie) { @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) if (ret < 0) return ret; +#ifdef CONFIG_ARM /* * PCIe access errors that result into OCP errors are caught by ARM as * "External aborts" */ hook_fault_code(17, ks_pcie_fault, SIGBUS, 0, "Asynchronous external abort"); +#endif ks_pcie_start_link(pci); dw_pcie_wait_for_link(pci);
hook_fault_code is an ARM32 specific API for hooking into data abort. Since pci-keystone.c will be used for AM65X platforms which is an ARM64 platform, allow hook_fault_code to be compiled only for ARM32. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- drivers/pci/controller/dwc/pci-keystone.c | 4 ++++ 1 file changed, 4 insertions(+)