Re: [PATCH v5 0/6] Introduce ZONE_CMA
Joonsoo Kimwrites: > On Tue, Aug 30, 2016 at 04:09:37PM +0530, Aneesh Kumar K.V wrote: >> Joonsoo Kim writes: >> >> > 2016-08-29 18:27 GMT+09:00 Aneesh Kumar K.V >> > : >> >> js1...@gmail.com writes: >> >> >> >>> From: Joonsoo Kim >> >>> >> >>> Hello, >> >>> >> >>> Changes from v4 >> >>> o Rebase on next-20160825 >> >>> o Add general fix patch for lowmem reserve >> >>> o Fix lowmem reserve ratio >> >>> o Fix zone span optimizaion per Vlastimil >> >>> o Fix pageset initialization >> >>> o Change invocation timing on cma_init_reserved_areas() >> >> >> >> I don't see much information regarding how we interleave between >> >> ZONE_CMA and other zones for movable allocation. Is that explained in >> >> any of the patch ? The fair zone allocator got removed by >> >> e6cbd7f2efb433d717af72aa8510a9db6f7a7e05 >> > >> > Interleaving would not work since the fair zone allocator policy is >> > removed. >> > I don't think that it's a big problem because it is just matter of >> > timing to fill >> > up the memory. Eventually, memory on ZONE_CMA will be fully used in >> > any case. >> >> Does that mean a CMA allocation will now be slower because in most case we >> will need to reclaim ? The zone list will now have ZONE_CMA in the >> beginning right ? > > ZONE_CMA will be used first but I don't think that CMA allocation will > be slower. In most case, memory would be fully used (usually > by page cache). So, we need reclaim or migration in any case. Considering that the upstream kernel doesn't allow migration of THP pages, this would mean that migrate will fail in most case if we have THP enabled and the THP allocation request got satisfied via ZONE_CMA. Isn't that going to be a problem ? -aneesh
Re: [PATCH v5 0/6] Introduce ZONE_CMA
Joonsoo Kim writes: > On Tue, Aug 30, 2016 at 04:09:37PM +0530, Aneesh Kumar K.V wrote: >> Joonsoo Kim writes: >> >> > 2016-08-29 18:27 GMT+09:00 Aneesh Kumar K.V >> > : >> >> js1...@gmail.com writes: >> >> >> >>> From: Joonsoo Kim >> >>> >> >>> Hello, >> >>> >> >>> Changes from v4 >> >>> o Rebase on next-20160825 >> >>> o Add general fix patch for lowmem reserve >> >>> o Fix lowmem reserve ratio >> >>> o Fix zone span optimizaion per Vlastimil >> >>> o Fix pageset initialization >> >>> o Change invocation timing on cma_init_reserved_areas() >> >> >> >> I don't see much information regarding how we interleave between >> >> ZONE_CMA and other zones for movable allocation. Is that explained in >> >> any of the patch ? The fair zone allocator got removed by >> >> e6cbd7f2efb433d717af72aa8510a9db6f7a7e05 >> > >> > Interleaving would not work since the fair zone allocator policy is >> > removed. >> > I don't think that it's a big problem because it is just matter of >> > timing to fill >> > up the memory. Eventually, memory on ZONE_CMA will be fully used in >> > any case. >> >> Does that mean a CMA allocation will now be slower because in most case we >> will need to reclaim ? The zone list will now have ZONE_CMA in the >> beginning right ? > > ZONE_CMA will be used first but I don't think that CMA allocation will > be slower. In most case, memory would be fully used (usually > by page cache). So, we need reclaim or migration in any case. Considering that the upstream kernel doesn't allow migration of THP pages, this would mean that migrate will fail in most case if we have THP enabled and the THP allocation request got satisfied via ZONE_CMA. Isn't that going to be a problem ? -aneesh
RE: [PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration
> Subject: Re: [PATCH v3 kernel 0/7] Extend virtio-balloon for fast > (de)inflating > & fast live migration > > 2016-08-08 14:35 GMT+08:00 Liang Li: > > This patch set contains two parts of changes to the virtio-balloon. > > > > One is the change for speeding up the inflating & deflating process, > > the main idea of this optimization is to use bitmap to send the page > > information to host instead of the PFNs, to reduce the overhead of > > virtio data transmission, address translation and madvise(). This can > > help to improve the performance by about 85%. > > > > Another change is for speeding up live migration. By skipping process > > guest's free pages in the first round of data copy, to reduce needless > > data processing, this can help to save quite a lot of CPU cycles and > > network bandwidth. We put guest's free page information in bitmap and > > send it to host with the virt queue of virtio-balloon. For an idle 8GB > > guest, this can help to shorten the total live migration time from > > 2Sec to about 500ms in the 10Gbps network environment. > > I just read the slides of this feature for recent kvm forum, the cloud > providers more care about live migration downtime to avoid customers' > perception than total time, however, this feature will increase downtime > when acquire the benefit of reducing total time, maybe it will be more > acceptable if there is no downside for downtime. > > Regards, > Wanpeng Li In theory, there is no factor that will increase the downtime. There is no additional operation and no more data copy during the stop and copy stage. But in the test, the downtime increases and this can be reproduced. I think the busy network line maybe the reason for this. With this optimization, a huge amount of data is written to the socket in a shorter time, so some of the write operation may need to wait. Without this optimization, zero page checking takes more time, the network is not so busy. If the guest is not an idle one, I think the gap of the downtime will not so obvious. Anyway, the downtime is still less than the max_down_time set by the user. Thanks! Liang
RE: [PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration
> Subject: Re: [PATCH v3 kernel 0/7] Extend virtio-balloon for fast > (de)inflating > & fast live migration > > 2016-08-08 14:35 GMT+08:00 Liang Li : > > This patch set contains two parts of changes to the virtio-balloon. > > > > One is the change for speeding up the inflating & deflating process, > > the main idea of this optimization is to use bitmap to send the page > > information to host instead of the PFNs, to reduce the overhead of > > virtio data transmission, address translation and madvise(). This can > > help to improve the performance by about 85%. > > > > Another change is for speeding up live migration. By skipping process > > guest's free pages in the first round of data copy, to reduce needless > > data processing, this can help to save quite a lot of CPU cycles and > > network bandwidth. We put guest's free page information in bitmap and > > send it to host with the virt queue of virtio-balloon. For an idle 8GB > > guest, this can help to shorten the total live migration time from > > 2Sec to about 500ms in the 10Gbps network environment. > > I just read the slides of this feature for recent kvm forum, the cloud > providers more care about live migration downtime to avoid customers' > perception than total time, however, this feature will increase downtime > when acquire the benefit of reducing total time, maybe it will be more > acceptable if there is no downside for downtime. > > Regards, > Wanpeng Li In theory, there is no factor that will increase the downtime. There is no additional operation and no more data copy during the stop and copy stage. But in the test, the downtime increases and this can be reproduced. I think the busy network line maybe the reason for this. With this optimization, a huge amount of data is written to the socket in a shorter time, so some of the write operation may need to wait. Without this optimization, zero page checking takes more time, the network is not so busy. If the guest is not an idle one, I think the gap of the downtime will not so obvious. Anyway, the downtime is still less than the max_down_time set by the user. Thanks! Liang
Greetings
You are a recipient to Mrs Julie Leach Donation of $3 million USD. Contact (julieleac...@gmail.com) for claims.
Greetings
You are a recipient to Mrs Julie Leach Donation of $3 million USD. Contact (julieleac...@gmail.com) for claims.
Re: [PATCH V4] leds: trigger: Introduce an USB port trigger
On 31 August 2016 at 21:00, Rafał Miłeckiwrote: > On 31 August 2016 at 20:23, Alan Stern wrote: >> On Tue, 30 Aug 2016, Rafał Miłecki wrote: >>> Not really as it won't cover some pretty common use cases. Many home >>> routers have few USB ports (2-5) and only 1 USB LED. It has to be >>> possible to assign few USB ports to a single LED (trigger). That way >>> LED should be turned on (and kept on) if there is at least 1 USB >>> device connected. You obviously can't do: >>> echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger >>> >>> This was already brought up by Rob (who mentioned CPU trigger) and I >>> replied him pretty much the same way in: >>> https://lkml.org/lkml/2016/7/29/38 >>> (reply starts with "Anyway, the serious limitation I see"). >> >> The code for a bunch of triggers must already be written. What would >> the user do if he wanted to flash a single LED in response to both >> CPU activity and MTD activity? If not >> >> echo "cpu mtd" >/sys/class/leds/foo/trigger >> >> then what? > > Well, it sounds like a new feature then. Shall we add an extra API > with a request function for turning LED on? It could internally count > how many requests were raised and keep LED on as long as there is at > least 1 left. I guess we should implement it in trigger "subsystem" > (if I can call it so). Does it sound like a good plan? I'm pretty sure noone ever planned to have more than 1 trigger assigned to a single LED. I just realized there will be a problem with proposed solution: sysfs files conflict. Consider 2 existing triggers for a moment: 1) oneshot: it creates following sysfs files: /sys/class/leds/foo/delay_on /sys/class/leds/foo/delay_off /sys/class/leds/foo/invert /sys/class/leds/foo/shot 2) timer: it creates following sysfs files: /sys/class/leds/foo/delay_on /sys/class/leds/foo/delay_off Activating both of them will probably cause a WARNING in sysfs. They can't coexist :( We should probably have per-trigger subdirs, e.g.: /sys/class/leds/foo/trigger-oneshot/delay_on /sys/class/leds/foo/trigger-oneshot/delay_off /sys/class/leds/foo/trigger-oneshot/invert /sys/class/leds/foo/trigger-oneshot/shot /sys/class/leds/foo/trigger-timer/delay_on /sys/class/leds/foo/trigger-timer/delay_off but implementing it now would break the ABI. One workaround I can see is doing triggers V2, they: 1) Would put sysfs files in /sys/class/leds/foo/trigger-bar/ 2) Use new API for *requesting* LED to be on/off 3) There would be a counter of requests in V2 API 4) Multiple triggers V2 would be allowed to be used (assigned) at the same time -- Rafał
Re: [PATCH V4] leds: trigger: Introduce an USB port trigger
On 31 August 2016 at 21:00, Rafał Miłecki wrote: > On 31 August 2016 at 20:23, Alan Stern wrote: >> On Tue, 30 Aug 2016, Rafał Miłecki wrote: >>> Not really as it won't cover some pretty common use cases. Many home >>> routers have few USB ports (2-5) and only 1 USB LED. It has to be >>> possible to assign few USB ports to a single LED (trigger). That way >>> LED should be turned on (and kept on) if there is at least 1 USB >>> device connected. You obviously can't do: >>> echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger >>> >>> This was already brought up by Rob (who mentioned CPU trigger) and I >>> replied him pretty much the same way in: >>> https://lkml.org/lkml/2016/7/29/38 >>> (reply starts with "Anyway, the serious limitation I see"). >> >> The code for a bunch of triggers must already be written. What would >> the user do if he wanted to flash a single LED in response to both >> CPU activity and MTD activity? If not >> >> echo "cpu mtd" >/sys/class/leds/foo/trigger >> >> then what? > > Well, it sounds like a new feature then. Shall we add an extra API > with a request function for turning LED on? It could internally count > how many requests were raised and keep LED on as long as there is at > least 1 left. I guess we should implement it in trigger "subsystem" > (if I can call it so). Does it sound like a good plan? I'm pretty sure noone ever planned to have more than 1 trigger assigned to a single LED. I just realized there will be a problem with proposed solution: sysfs files conflict. Consider 2 existing triggers for a moment: 1) oneshot: it creates following sysfs files: /sys/class/leds/foo/delay_on /sys/class/leds/foo/delay_off /sys/class/leds/foo/invert /sys/class/leds/foo/shot 2) timer: it creates following sysfs files: /sys/class/leds/foo/delay_on /sys/class/leds/foo/delay_off Activating both of them will probably cause a WARNING in sysfs. They can't coexist :( We should probably have per-trigger subdirs, e.g.: /sys/class/leds/foo/trigger-oneshot/delay_on /sys/class/leds/foo/trigger-oneshot/delay_off /sys/class/leds/foo/trigger-oneshot/invert /sys/class/leds/foo/trigger-oneshot/shot /sys/class/leds/foo/trigger-timer/delay_on /sys/class/leds/foo/trigger-timer/delay_off but implementing it now would break the ABI. One workaround I can see is doing triggers V2, they: 1) Would put sysfs files in /sys/class/leds/foo/trigger-bar/ 2) Use new API for *requesting* LED to be on/off 3) There would be a counter of requests in V2 API 4) Multiple triggers V2 would be allowed to be used (assigned) at the same time -- Rafał
RE: [PATCH 3/3] PCI: Xilinx NWL PCIe: Fix Error for multi function device for legacy interrupts.
> Hi Bharat, > > @@ -561,7 +561,7 @@ static int nwl_pcie_init_irq_domain(struct > > nwl_pcie > *pcie) > > } > > > > pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node, > > - INTX_NUM, > > + INTX_NUM + 1, > > _domain_ops, > > pcie); > > This feels like the wrong thing to do. You have INTX_NUM irqs, so > the domain allocation should reflect this. On the other hand, the > way the driver currently deals with mappings is quite broken > (consistently adding 1 to > >> the HW interrupt). > > >>> Hi Marc, > >>> > >>> Without above change I get following crash in kernel while booting. > >>> > >>> [2.441684] error: hwirq 0x4 is too large for dummy > >>> > >>> [2.441694] [ cut here ] > >>> > >>> [2.441698] WARNING: at kernel/irq/irqdomain.c:344 > >>> > >>> [2.441702] Modules linked in: > >>> > >>> [2.441706] > >>> > >>> [2.441714] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0 #8 > >>> > >>> [2.441718] Hardware name: xlnx,zynqmp (DT) > >>> > >>> [2.441723] task: ffc071886b80 ti: ffc071888000 task.ti: > >> ffc071888000 > >>> > >>> [2.441732] PC is at irq_domain_associate+0x138/0x1c0 > >>> > >>> [2.441738] LR is at irq_domain_associate+0x138/0x1c0 > >>> > >>> In kernel/irq/irqdomain.c function irq_domain_associate > >>> > >>> if (WARN(hwirq >= domain->hwirq_max, > >>> "error: hwirq 0x%x is too large for %s\n", (int)hwirq, > >>> domain- > >name)) > >>> return -EINVAL; > >>> > >>> Here the hwirq and hwirq_max are equal to 4 without the above > >>> condition > >> (INTX_NUM + 1) due to which crash is coming. > >>> This is happening as the legacy interrupts are starting from 1 (INTA). > >> > >> I understood that. I'm still persisting in saying that you have the wrong > >> fix. > >> > >> Your domain should always allocate many interrupts as you have > >> interrupt sources. These interrupts (hwirq) should be numbered from 0 to > >> (n- > 1). > > > > Agreed, but here comes the problem the hwirq for legacy interrupts > > will start at 0x1 to 0x4 (INTA to INTD) and these values are as per > > PCIe specification for legacy interrupts. So these cannot be numbered > > from 0. So when 0x4 (INTD) for a multi-function device comes the crash > > occurs. > > So who provides this hwirq? Who calls irq_domain_associate() with hwirq set to > 4? > PCIe subsystem invokes pcibios_add_device function in arch/arm64/kernel/pci.c for every pci device. The purpose of this function is to assign dev->irq using of_irq_parse_and_map_pci. of_irq_parse_and_map_pci invokes of_irq_parse_pci where it reads PCI_INTERRUPT_PIN from configuration space and saves it in parameter of struct of_phandle_args. This structure is passed to irq_create_of_mapping where it invokes irq_create_fwspec_mapping. irq_create_fwspec_mapping invokes irq_domain_translate and gets hwirq, here the above saved PCI_INTERRUPT_PIN value is assigned to hwirq (*hwirq = fwspec->param[0]). And then using this hwirq irq_create_mapping -> irq_domain_associate were invoked and mapping is created for virtual irq with this hwirq. So for any end point PCI_INTERRUPT_PIN value starts from 0x1 to 0x4 and so hwirq starts from 0x1 to 0x4. So the values are more generic w.r.t to protocol, that's why hwirq will range from 0x1 to 0x4. And then if you check pcie-altera.c they are doing this adding one in their handler and while creating legacy domain. Thanks & Regards, Bharat
RE: [PATCH 3/3] PCI: Xilinx NWL PCIe: Fix Error for multi function device for legacy interrupts.
> Hi Bharat, > > @@ -561,7 +561,7 @@ static int nwl_pcie_init_irq_domain(struct > > nwl_pcie > *pcie) > > } > > > > pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node, > > - INTX_NUM, > > + INTX_NUM + 1, > > _domain_ops, > > pcie); > > This feels like the wrong thing to do. You have INTX_NUM irqs, so > the domain allocation should reflect this. On the other hand, the > way the driver currently deals with mappings is quite broken > (consistently adding 1 to > >> the HW interrupt). > > >>> Hi Marc, > >>> > >>> Without above change I get following crash in kernel while booting. > >>> > >>> [2.441684] error: hwirq 0x4 is too large for dummy > >>> > >>> [2.441694] [ cut here ] > >>> > >>> [2.441698] WARNING: at kernel/irq/irqdomain.c:344 > >>> > >>> [2.441702] Modules linked in: > >>> > >>> [2.441706] > >>> > >>> [2.441714] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0 #8 > >>> > >>> [2.441718] Hardware name: xlnx,zynqmp (DT) > >>> > >>> [2.441723] task: ffc071886b80 ti: ffc071888000 task.ti: > >> ffc071888000 > >>> > >>> [2.441732] PC is at irq_domain_associate+0x138/0x1c0 > >>> > >>> [2.441738] LR is at irq_domain_associate+0x138/0x1c0 > >>> > >>> In kernel/irq/irqdomain.c function irq_domain_associate > >>> > >>> if (WARN(hwirq >= domain->hwirq_max, > >>> "error: hwirq 0x%x is too large for %s\n", (int)hwirq, > >>> domain- > >name)) > >>> return -EINVAL; > >>> > >>> Here the hwirq and hwirq_max are equal to 4 without the above > >>> condition > >> (INTX_NUM + 1) due to which crash is coming. > >>> This is happening as the legacy interrupts are starting from 1 (INTA). > >> > >> I understood that. I'm still persisting in saying that you have the wrong > >> fix. > >> > >> Your domain should always allocate many interrupts as you have > >> interrupt sources. These interrupts (hwirq) should be numbered from 0 to > >> (n- > 1). > > > > Agreed, but here comes the problem the hwirq for legacy interrupts > > will start at 0x1 to 0x4 (INTA to INTD) and these values are as per > > PCIe specification for legacy interrupts. So these cannot be numbered > > from 0. So when 0x4 (INTD) for a multi-function device comes the crash > > occurs. > > So who provides this hwirq? Who calls irq_domain_associate() with hwirq set to > 4? > PCIe subsystem invokes pcibios_add_device function in arch/arm64/kernel/pci.c for every pci device. The purpose of this function is to assign dev->irq using of_irq_parse_and_map_pci. of_irq_parse_and_map_pci invokes of_irq_parse_pci where it reads PCI_INTERRUPT_PIN from configuration space and saves it in parameter of struct of_phandle_args. This structure is passed to irq_create_of_mapping where it invokes irq_create_fwspec_mapping. irq_create_fwspec_mapping invokes irq_domain_translate and gets hwirq, here the above saved PCI_INTERRUPT_PIN value is assigned to hwirq (*hwirq = fwspec->param[0]). And then using this hwirq irq_create_mapping -> irq_domain_associate were invoked and mapping is created for virtual irq with this hwirq. So for any end point PCI_INTERRUPT_PIN value starts from 0x1 to 0x4 and so hwirq starts from 0x1 to 0x4. So the values are more generic w.r.t to protocol, that's why hwirq will range from 0x1 to 0x4. And then if you check pcie-altera.c they are doing this adding one in their handler and while creating legacy domain. Thanks & Regards, Bharat
Re: [PATCH 1/4] kernel: add a helper to get an owning user namespace for a namespace
On Wed, Aug 31, 2016 at 01:38:35PM -0700, Andrey Vagin wrote: > On Tue, Aug 30, 2016 at 7:56 PM, Serge E. Hallynwrote: > > On Fri, Aug 26, 2016 at 04:08:08PM -0700, Andrei Vagin wrote: > >> +struct ns_common *ns_get_owner(struct ns_common *ns) > >> +{ > >> + struct user_namespace *my_user_ns = current_user_ns(); > >> + struct user_namespace *owner, *p; > >> + > >> + /* See if the owner is in the current user namespace */ > >> + owner = p = ns->ops->get_owner(ns); > >> + for (;;) { > >> + if (!p) > >> + return ERR_PTR(-EPERM); > >> + if (p == my_user_ns) > >> + break; > >> + p = p->parent; > >> + } > >> + > >> + return _user_ns(owner)->ns; > > > > get_user_ns() bumps the owner's refcount. I don't see where > > this is being dropped, especially when ns_ioctl() uses it in > > the next patch. > > It is dropped in __ns_get_path if a namespace has a dentry, otherwise > it is dropped from nsfs_evict. > > static void *__ns_get_path(struct path *path, struct ns_common *ns) > |return -EPERM; > ... > ns->ops->put(ns); > | > got_it: > |/* See if the owner is in the current user namespace > */ > path->mnt = mnt; > |owner = p = ns->ops->get_owner(ns); > path->dentry = dentry; > |for (;;) { > return NULL; > ... > > static void nsfs_evict(struct inode *inode) > | > { > |if (!ns_capable(user_ns, CAP_SYS_ADMIN)) > struct ns_common *ns = inode->i_private; > |return -EPERM; > clear_inode(inode); > | > ns->ops->put(ns); > |cred = prepare_creds(); > } Gotcha, thanks.
Re: [PATCH 1/4] kernel: add a helper to get an owning user namespace for a namespace
On Wed, Aug 31, 2016 at 01:38:35PM -0700, Andrey Vagin wrote: > On Tue, Aug 30, 2016 at 7:56 PM, Serge E. Hallyn wrote: > > On Fri, Aug 26, 2016 at 04:08:08PM -0700, Andrei Vagin wrote: > >> +struct ns_common *ns_get_owner(struct ns_common *ns) > >> +{ > >> + struct user_namespace *my_user_ns = current_user_ns(); > >> + struct user_namespace *owner, *p; > >> + > >> + /* See if the owner is in the current user namespace */ > >> + owner = p = ns->ops->get_owner(ns); > >> + for (;;) { > >> + if (!p) > >> + return ERR_PTR(-EPERM); > >> + if (p == my_user_ns) > >> + break; > >> + p = p->parent; > >> + } > >> + > >> + return _user_ns(owner)->ns; > > > > get_user_ns() bumps the owner's refcount. I don't see where > > this is being dropped, especially when ns_ioctl() uses it in > > the next patch. > > It is dropped in __ns_get_path if a namespace has a dentry, otherwise > it is dropped from nsfs_evict. > > static void *__ns_get_path(struct path *path, struct ns_common *ns) > |return -EPERM; > ... > ns->ops->put(ns); > | > got_it: > |/* See if the owner is in the current user namespace > */ > path->mnt = mnt; > |owner = p = ns->ops->get_owner(ns); > path->dentry = dentry; > |for (;;) { > return NULL; > ... > > static void nsfs_evict(struct inode *inode) > | > { > |if (!ns_capable(user_ns, CAP_SYS_ADMIN)) > struct ns_common *ns = inode->i_private; > |return -EPERM; > clear_inode(inode); > | > ns->ops->put(ns); > |cred = prepare_creds(); > } Gotcha, thanks.
Re: [PATCH v5 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY
On Wednesday 31 August 2016 07:38 PM, Heiko Stübner wrote: > Hi, > > Am Samstag, 20. August 2016, 10:53:37 schrieb Shawn Lin: >> This patch to add a generic PHY driver for rockchip PCIe PHY. >> Access the PHY via registers provided by GRF (general register >> files) module. >> >> Signed-off-by: Shawn Lin> > seems I'm late to the party, but when looking if I can apply the pcie- > devicetree patches, I found that the phy is still pending. > > Apart from some error-message nitpicks below, this looks ok to me. I don't > know enough about the actual pci phy part though. > > Kishon, is this on your radar? yes.. can the nipicks be fixed and posted asap? Thanks Kishon > > [...] > >> +static int rockchip_pcie_phy_power_off(struct phy *phy) >> +{ >> +struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); >> +int err = 0; >> + >> +err = reset_control_assert(rk_phy->phy_rst); >> +if (err) { >> +pr_err("assert phy_rst err %d\n", err); > > dev_err(phy->dev, ...) > > probably the same for all other pr_err invocations > > >> +return err; >> +} >> + >> +return 0; >> +} > > [...] > >> +static const struct of_device_id rockchip_pcie_phy_dt_ids[] = { >> +{ >> +.compatible = "rockchip,rk3399-pcie-phy", >> +.data = _pcie_data, >> +}, >> +{} >> +}; >> + >> +MODULE_DEVICE_TABLE(of, rockchip_pcie_phy_dt_ids); >> + >> +static int rockchip_pcie_phy_probe(struct platform_device *pdev) >> +{ >> +struct device *dev = >dev; >> +struct rockchip_pcie_phy *rk_phy; >> +struct phy *generic_phy; >> +struct phy_provider *phy_provider; >> +struct regmap *grf; >> +const struct of_device_id *of_id; >> + >> +grf = syscon_node_to_regmap(dev->parent->of_node); >> +if (IS_ERR(grf)) { >> +dev_err(dev, "Missing rockchip,grf property\n"); > > dev_err(dev, "Cannot find GRF syscon\n"); > > > Heiko >
Re: [PATCH v5 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY
On Wednesday 31 August 2016 07:38 PM, Heiko Stübner wrote: > Hi, > > Am Samstag, 20. August 2016, 10:53:37 schrieb Shawn Lin: >> This patch to add a generic PHY driver for rockchip PCIe PHY. >> Access the PHY via registers provided by GRF (general register >> files) module. >> >> Signed-off-by: Shawn Lin > > seems I'm late to the party, but when looking if I can apply the pcie- > devicetree patches, I found that the phy is still pending. > > Apart from some error-message nitpicks below, this looks ok to me. I don't > know enough about the actual pci phy part though. > > Kishon, is this on your radar? yes.. can the nipicks be fixed and posted asap? Thanks Kishon > > [...] > >> +static int rockchip_pcie_phy_power_off(struct phy *phy) >> +{ >> +struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); >> +int err = 0; >> + >> +err = reset_control_assert(rk_phy->phy_rst); >> +if (err) { >> +pr_err("assert phy_rst err %d\n", err); > > dev_err(phy->dev, ...) > > probably the same for all other pr_err invocations > > >> +return err; >> +} >> + >> +return 0; >> +} > > [...] > >> +static const struct of_device_id rockchip_pcie_phy_dt_ids[] = { >> +{ >> +.compatible = "rockchip,rk3399-pcie-phy", >> +.data = _pcie_data, >> +}, >> +{} >> +}; >> + >> +MODULE_DEVICE_TABLE(of, rockchip_pcie_phy_dt_ids); >> + >> +static int rockchip_pcie_phy_probe(struct platform_device *pdev) >> +{ >> +struct device *dev = >dev; >> +struct rockchip_pcie_phy *rk_phy; >> +struct phy *generic_phy; >> +struct phy_provider *phy_provider; >> +struct regmap *grf; >> +const struct of_device_id *of_id; >> + >> +grf = syscon_node_to_regmap(dev->parent->of_node); >> +if (IS_ERR(grf)) { >> +dev_err(dev, "Missing rockchip,grf property\n"); > > dev_err(dev, "Cannot find GRF syscon\n"); > > > Heiko >
Re: [PATCH v2] arm64: defconfig: enable common modules for power management
Hi Catalin, Will, On Thu, Sep 01, 2016 at 12:51:09PM +0800, Leo Yan wrote: > Enable common modules for power management; one is to enable > CPUFREQ_DT driver; the driver is used by many platforms by passing OPP > table from device tree. > > Also enables thermal related drivers. Firstly we need enable > configuration CPU_THERMAL for CPU cooling device driver, this will bind > thermal zone with CPU cooling device; and enable 'power allocator' > thermal governor. This patch is an updated version for [1] with enabling "power allocator". Sorry for regression. [1] http://archive.arm.linux.org.uk/lurker/message/20160831.085017.a42c57fe.en.html Thanks, Leo Yan > > Signed-off-by: Leo Yan> --- > arch/arm64/configs/defconfig | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index eadf485..c4f5948 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -82,6 +82,7 @@ CONFIG_COMPAT=y > CONFIG_CPU_IDLE=y > CONFIG_ARM_CPUIDLE=y > CONFIG_CPU_FREQ=y > +CONFIG_CPUFREQ_DT=y > CONFIG_ARM_BIG_LITTLE_CPUFREQ=y > CONFIG_ARM_SCPI_CPUFREQ=y > CONFIG_NET=y > @@ -252,6 +253,8 @@ CONFIG_SENSORS_INA2XX=m > CONFIG_SENSORS_ARM_SCPI=y > CONFIG_THERMAL=y > CONFIG_THERMAL_EMULATION=y > +CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y > +CONFIG_CPU_THERMAL=y > CONFIG_EXYNOS_THERMAL=y > CONFIG_WATCHDOG=y > CONFIG_RENESAS_WDT=y > -- > 1.9.1 >
Re: [PATCH v2] arm64: defconfig: enable common modules for power management
Hi Catalin, Will, On Thu, Sep 01, 2016 at 12:51:09PM +0800, Leo Yan wrote: > Enable common modules for power management; one is to enable > CPUFREQ_DT driver; the driver is used by many platforms by passing OPP > table from device tree. > > Also enables thermal related drivers. Firstly we need enable > configuration CPU_THERMAL for CPU cooling device driver, this will bind > thermal zone with CPU cooling device; and enable 'power allocator' > thermal governor. This patch is an updated version for [1] with enabling "power allocator". Sorry for regression. [1] http://archive.arm.linux.org.uk/lurker/message/20160831.085017.a42c57fe.en.html Thanks, Leo Yan > > Signed-off-by: Leo Yan > --- > arch/arm64/configs/defconfig | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index eadf485..c4f5948 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -82,6 +82,7 @@ CONFIG_COMPAT=y > CONFIG_CPU_IDLE=y > CONFIG_ARM_CPUIDLE=y > CONFIG_CPU_FREQ=y > +CONFIG_CPUFREQ_DT=y > CONFIG_ARM_BIG_LITTLE_CPUFREQ=y > CONFIG_ARM_SCPI_CPUFREQ=y > CONFIG_NET=y > @@ -252,6 +253,8 @@ CONFIG_SENSORS_INA2XX=m > CONFIG_SENSORS_ARM_SCPI=y > CONFIG_THERMAL=y > CONFIG_THERMAL_EMULATION=y > +CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y > +CONFIG_CPU_THERMAL=y > CONFIG_EXYNOS_THERMAL=y > CONFIG_WATCHDOG=y > CONFIG_RENESAS_WDT=y > -- > 1.9.1 >
[PATCH v2] arm64: defconfig: enable common modules for power management
Enable common modules for power management; one is to enable CPUFREQ_DT driver; the driver is used by many platforms by passing OPP table from device tree. Also enables thermal related drivers. Firstly we need enable configuration CPU_THERMAL for CPU cooling device driver, this will bind thermal zone with CPU cooling device; and enable 'power allocator' thermal governor. Signed-off-by: Leo Yan--- arch/arm64/configs/defconfig | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index eadf485..c4f5948 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -82,6 +82,7 @@ CONFIG_COMPAT=y CONFIG_CPU_IDLE=y CONFIG_ARM_CPUIDLE=y CONFIG_CPU_FREQ=y +CONFIG_CPUFREQ_DT=y CONFIG_ARM_BIG_LITTLE_CPUFREQ=y CONFIG_ARM_SCPI_CPUFREQ=y CONFIG_NET=y @@ -252,6 +253,8 @@ CONFIG_SENSORS_INA2XX=m CONFIG_SENSORS_ARM_SCPI=y CONFIG_THERMAL=y CONFIG_THERMAL_EMULATION=y +CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y +CONFIG_CPU_THERMAL=y CONFIG_EXYNOS_THERMAL=y CONFIG_WATCHDOG=y CONFIG_RENESAS_WDT=y -- 1.9.1
[PATCH v2] arm64: defconfig: enable common modules for power management
Enable common modules for power management; one is to enable CPUFREQ_DT driver; the driver is used by many platforms by passing OPP table from device tree. Also enables thermal related drivers. Firstly we need enable configuration CPU_THERMAL for CPU cooling device driver, this will bind thermal zone with CPU cooling device; and enable 'power allocator' thermal governor. Signed-off-by: Leo Yan --- arch/arm64/configs/defconfig | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index eadf485..c4f5948 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -82,6 +82,7 @@ CONFIG_COMPAT=y CONFIG_CPU_IDLE=y CONFIG_ARM_CPUIDLE=y CONFIG_CPU_FREQ=y +CONFIG_CPUFREQ_DT=y CONFIG_ARM_BIG_LITTLE_CPUFREQ=y CONFIG_ARM_SCPI_CPUFREQ=y CONFIG_NET=y @@ -252,6 +253,8 @@ CONFIG_SENSORS_INA2XX=m CONFIG_SENSORS_ARM_SCPI=y CONFIG_THERMAL=y CONFIG_THERMAL_EMULATION=y +CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y +CONFIG_CPU_THERMAL=y CONFIG_EXYNOS_THERMAL=y CONFIG_WATCHDOG=y CONFIG_RENESAS_WDT=y -- 1.9.1
Re: [PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration
2016-08-08 14:35 GMT+08:00 Liang Li: > This patch set contains two parts of changes to the virtio-balloon. > > One is the change for speeding up the inflating & deflating process, > the main idea of this optimization is to use bitmap to send the page > information to host instead of the PFNs, to reduce the overhead of > virtio data transmission, address translation and madvise(). This can > help to improve the performance by about 85%. > > Another change is for speeding up live migration. By skipping process > guest's free pages in the first round of data copy, to reduce needless > data processing, this can help to save quite a lot of CPU cycles and > network bandwidth. We put guest's free page information in bitmap and > send it to host with the virt queue of virtio-balloon. For an idle 8GB > guest, this can help to shorten the total live migration time from 2Sec > to about 500ms in the 10Gbps network environment. I just read the slides of this feature for recent kvm forum, the cloud providers more care about live migration downtime to avoid customers' perception than total time, however, this feature will increase downtime when acquire the benefit of reducing total time, maybe it will be more acceptable if there is no downside for downtime. Regards, Wanpeng Li
Re: [PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration
2016-08-08 14:35 GMT+08:00 Liang Li : > This patch set contains two parts of changes to the virtio-balloon. > > One is the change for speeding up the inflating & deflating process, > the main idea of this optimization is to use bitmap to send the page > information to host instead of the PFNs, to reduce the overhead of > virtio data transmission, address translation and madvise(). This can > help to improve the performance by about 85%. > > Another change is for speeding up live migration. By skipping process > guest's free pages in the first round of data copy, to reduce needless > data processing, this can help to save quite a lot of CPU cycles and > network bandwidth. We put guest's free page information in bitmap and > send it to host with the virt queue of virtio-balloon. For an idle 8GB > guest, this can help to shorten the total live migration time from 2Sec > to about 500ms in the 10Gbps network environment. I just read the slides of this feature for recent kvm forum, the cloud providers more care about live migration downtime to avoid customers' perception than total time, however, this feature will increase downtime when acquire the benefit of reducing total time, maybe it will be more acceptable if there is no downside for downtime. Regards, Wanpeng Li
Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
Hi, On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xuwrote: >> This is fine to pick up _only_ if you don't care about suspend/resume. >> If you care about suspend/resume then someone needs to first write a >> patch that will re-init all "corecfg" values after power is turned on. > > > Do you mean corecfg_clockmultiplier and corecfg_baseclkfreq, if yes, we > don't need to strore/re-init it after resume. > corecfg_clockmultiplier is only used to fetch host->clk_mul, and > host->clk_mul has been a fixed value at run-time, unless driver unbind. > The same as corecfg_clockmultiplier, corecfg_baseclkfreq is used to check > the xin_clk at probe time, we don't reference it at run-time. > BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works > fine. I guess I don't actually know how the corecfg_clockmultiplier and corecfg_baseclkfreq fields are actually used, but I presume that they actually do something useful and aren't used to just communicate back to software? I know that: 1. If I don't pick this patch and I suspend/resume, corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after suspend / resume. 2. If I do pick this patch and I suspend/resume, corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after suspend/resume (tested by reading /dev/mem directly from userspace after suspend/resume). Are you saying that it is unimportant that corecfg_clockmultiplier and corecfg_baseclkfreq are wrong? >> Technically I think this should probably use "pm runtime" and not >> normal suspend/resume hooks. Any time we end up pm runtime suspended >> then I think our power will go off (because of genpd?) and we need to >> restore values. > > > I understand your consideration. BUT genpd is in charge of on/off pd if the > corresponding device node has "power-domains" property. RPM is unnecessary > for this situation, we will not use autosuspend, right? > > @shawn, what's your opinion? I haven't dug. If Runtime PM isn't enabled for sdhci-of-arasan then I guess we can just worry about suspend/resume, though. -Doug
Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
Hi, On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu wrote: >> This is fine to pick up _only_ if you don't care about suspend/resume. >> If you care about suspend/resume then someone needs to first write a >> patch that will re-init all "corecfg" values after power is turned on. > > > Do you mean corecfg_clockmultiplier and corecfg_baseclkfreq, if yes, we > don't need to strore/re-init it after resume. > corecfg_clockmultiplier is only used to fetch host->clk_mul, and > host->clk_mul has been a fixed value at run-time, unless driver unbind. > The same as corecfg_clockmultiplier, corecfg_baseclkfreq is used to check > the xin_clk at probe time, we don't reference it at run-time. > BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works > fine. I guess I don't actually know how the corecfg_clockmultiplier and corecfg_baseclkfreq fields are actually used, but I presume that they actually do something useful and aren't used to just communicate back to software? I know that: 1. If I don't pick this patch and I suspend/resume, corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after suspend / resume. 2. If I do pick this patch and I suspend/resume, corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after suspend/resume (tested by reading /dev/mem directly from userspace after suspend/resume). Are you saying that it is unimportant that corecfg_clockmultiplier and corecfg_baseclkfreq are wrong? >> Technically I think this should probably use "pm runtime" and not >> normal suspend/resume hooks. Any time we end up pm runtime suspended >> then I think our power will go off (because of genpd?) and we need to >> restore values. > > > I understand your consideration. BUT genpd is in charge of on/off pd if the > corresponding device node has "power-domains" property. RPM is unnecessary > for this situation, we will not use autosuspend, right? > > @shawn, what's your opinion? I haven't dug. If Runtime PM isn't enabled for sdhci-of-arasan then I guess we can just worry about suspend/resume, though. -Doug
Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support
On 08/31/2016 08:39 PM, Shawn Lin wrote: Hi Guenter, Thanks for your review, and I think it still not too late for nitpicking as it isn't merged to next branch. :) We have amend the code a bit, so probably we fixed some of the minor issues against V10. But some of them are really personal taste, if you still insist on that, I will be ok to modify them. Take it as suggestions; I won't insist on anything. Guenter
Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support
On 08/31/2016 08:39 PM, Shawn Lin wrote: Hi Guenter, Thanks for your review, and I think it still not too late for nitpicking as it isn't merged to next branch. :) We have amend the code a bit, so probably we fixed some of the minor issues against V10. But some of them are really personal taste, if you still insist on that, I will be ok to modify them. Take it as suggestions; I won't insist on anything. Guenter
Re: [patch v3.18+ regression fix] sched: Further improve spurious CPU_IDLE active migrations
On Wed, 2016-08-31 at 17:52 +0200, Vincent Guittot wrote: > On 31 August 2016 at 12:36, Mike Galbraithwrote: > > On Wed, 2016-08-31 at 12:18 +0200, Mike Galbraith wrote: > > > On Wed, 2016-08-31 at 12:01 +0200, Peter Zijlstra wrote: > > > > > > So 43f4d66637bc ("sched: Improve sysbench performance by fixing spurious > > > > active migration") 's +1 made sense in that its a tie breaker. If you > > > > have 3 tasks on 2 groups, one group will have to have 2 tasks, and > > > > bouncing the one task around just isn't going to help _anything_. > > > > > > Yeah, but frequently tasks don't come in ones, so, you end up with an > > > endless tug of war between LB ripping communicating buddies apart, and > > > select_idle_sibling() pulling them back together.. bouncing cow > > > syndrome. > > > > replacing +1 by +2 fixes this use case that involves 2 threads but > similar behavior can happen with 3 tasks on system with 4 cores per MC > as an example > > IIUC, you have on > - one side, periodic load balance that spreads the 2 tasks in the system > - on the other side, wake up path that moves the task back in the same MC. Yup. > Isn't your regression more linked to spurious migration than where the > task is scheduled ? I don't see any direct relation between the client > and the server in this netperf test, isn't it ? netperf 4360 [004] 1207.865265: sched:sched_wakeup: netserver:4361 [120] success=1 CPU:002 netperf 4360 [004] 1207.865274: sched:sched_wakeup: netserver:4361 [120] success=1 CPU:002 netperf 4360 [004] 1207.865280: sched:sched_wakeup: netserver:4361 [120] success=1 CPU:002 netserver 4361 [002] 1207.865313: sched:sched_wakeup: netperf:4360 [120] success=1 CPU:004 netperf 4360 [004] 1207.865340: sched:sched_wakeup: kworker/u16:4:89 [120] success=1 CPU:000 netperf 4360 [004] 1207.865345: sched:sched_wakeup: kworker/u16:5:90 [120] success=1 CPU:006 netperf 4360 [004] 1207.865355: sched:sched_wakeup: kworker/u16:5:90 [120] success=1 CPU:006 netperf 4360 [004] 1207.865357: sched:sched_wakeup: kworker/u16:4:89 [120] success=1 CPU:000 netperf 4360 [004] 1207.865369: sched:sched_wakeup: netserver:4361 [120] success=1 CPU:002 netserver 4361 [002] 1207.865377: sched:sched_wakeup: netperf:4360 [120] success=1 CPU:004 netperf 4360 [004] 1207.865476: sched:sched_wakeup: perf:4359 [120] success=1 CPU:003 It's not limited to this load, anything at all that is communicating will do the same on these or similar processors. This trying to be perfect looks like a booboo to me, as we are now specifically asking our left hand undo what our right hand did to crank up throughput. For the diagnosed processor at least, one of those hands definitely wants to be slapped. This doesn't seem to be an issue for L3 equipped CPUs, but perhaps is for some even modern processors, dunno (the boxen where regression was detected are far from new). > we could either remove the condition which tries to keep an even > number of tasks in each group until busiest group becomes overloaded > but it means that unrelated tasks may have to share same resources > or we could try to prevent the migration at wake up. I was looking at > wake_affine which seems to choose local cpu when both prev and local > cpu are idle. I wonder if local cpu is really a better choice when > both are idle I don't see a great alternative to turning it off off the top of my head, at least for processors with multiple LLCs. Yeah, unrelated tasks could end up sharing a cache needlessly, but will that hurt as badly as tasks not munching tasty hot data definitely does? -Mike
Re: [patch v3.18+ regression fix] sched: Further improve spurious CPU_IDLE active migrations
On Wed, 2016-08-31 at 17:52 +0200, Vincent Guittot wrote: > On 31 August 2016 at 12:36, Mike Galbraith wrote: > > On Wed, 2016-08-31 at 12:18 +0200, Mike Galbraith wrote: > > > On Wed, 2016-08-31 at 12:01 +0200, Peter Zijlstra wrote: > > > > > > So 43f4d66637bc ("sched: Improve sysbench performance by fixing spurious > > > > active migration") 's +1 made sense in that its a tie breaker. If you > > > > have 3 tasks on 2 groups, one group will have to have 2 tasks, and > > > > bouncing the one task around just isn't going to help _anything_. > > > > > > Yeah, but frequently tasks don't come in ones, so, you end up with an > > > endless tug of war between LB ripping communicating buddies apart, and > > > select_idle_sibling() pulling them back together.. bouncing cow > > > syndrome. > > > > replacing +1 by +2 fixes this use case that involves 2 threads but > similar behavior can happen with 3 tasks on system with 4 cores per MC > as an example > > IIUC, you have on > - one side, periodic load balance that spreads the 2 tasks in the system > - on the other side, wake up path that moves the task back in the same MC. Yup. > Isn't your regression more linked to spurious migration than where the > task is scheduled ? I don't see any direct relation between the client > and the server in this netperf test, isn't it ? netperf 4360 [004] 1207.865265: sched:sched_wakeup: netserver:4361 [120] success=1 CPU:002 netperf 4360 [004] 1207.865274: sched:sched_wakeup: netserver:4361 [120] success=1 CPU:002 netperf 4360 [004] 1207.865280: sched:sched_wakeup: netserver:4361 [120] success=1 CPU:002 netserver 4361 [002] 1207.865313: sched:sched_wakeup: netperf:4360 [120] success=1 CPU:004 netperf 4360 [004] 1207.865340: sched:sched_wakeup: kworker/u16:4:89 [120] success=1 CPU:000 netperf 4360 [004] 1207.865345: sched:sched_wakeup: kworker/u16:5:90 [120] success=1 CPU:006 netperf 4360 [004] 1207.865355: sched:sched_wakeup: kworker/u16:5:90 [120] success=1 CPU:006 netperf 4360 [004] 1207.865357: sched:sched_wakeup: kworker/u16:4:89 [120] success=1 CPU:000 netperf 4360 [004] 1207.865369: sched:sched_wakeup: netserver:4361 [120] success=1 CPU:002 netserver 4361 [002] 1207.865377: sched:sched_wakeup: netperf:4360 [120] success=1 CPU:004 netperf 4360 [004] 1207.865476: sched:sched_wakeup: perf:4359 [120] success=1 CPU:003 It's not limited to this load, anything at all that is communicating will do the same on these or similar processors. This trying to be perfect looks like a booboo to me, as we are now specifically asking our left hand undo what our right hand did to crank up throughput. For the diagnosed processor at least, one of those hands definitely wants to be slapped. This doesn't seem to be an issue for L3 equipped CPUs, but perhaps is for some even modern processors, dunno (the boxen where regression was detected are far from new). > we could either remove the condition which tries to keep an even > number of tasks in each group until busiest group becomes overloaded > but it means that unrelated tasks may have to share same resources > or we could try to prevent the migration at wake up. I was looking at > wake_affine which seems to choose local cpu when both prev and local > cpu are idle. I wonder if local cpu is really a better choice when > both are idle I don't see a great alternative to turning it off off the top of my head, at least for processors with multiple LLCs. Yeah, unrelated tasks could end up sharing a cache needlessly, but will that hurt as badly as tasks not munching tasty hot data definitely does? -Mike
Re: [PATCH v3 0/5] net/usb: asix driver improvements
From: robert.f...@collabora.com Date: Mon, 29 Aug 2016 09:32:14 -0400 > This is a resubmission of v3, since the netdev > mailinlist was not sent the previous submission. > > This series improves power management of the asix driver. ... Series applied, thanks.
Re: [PATCH v3 0/5] net/usb: asix driver improvements
From: robert.f...@collabora.com Date: Mon, 29 Aug 2016 09:32:14 -0400 > This is a resubmission of v3, since the netdev > mailinlist was not sent the previous submission. > > This series improves power management of the asix driver. ... Series applied, thanks.
Re: [PATCH] mISDN: mark symbols static where possible
Three different patches all with the same Subject line, so I can't apply this stuff. You must make the subject lines unique so that someone reading the "git shortlog" can tell what is different in each change.
Re: [PATCH] mISDN: mark symbols static where possible
Three different patches all with the same Subject line, so I can't apply this stuff. You must make the subject lines unique so that someone reading the "git shortlog" can tell what is different in each change.
[PATCH 1/1] ARM: dts: sun8i: Add dts file for the NanoPi NEO SBC
From: James PettigrewThe NanoPi NEO is a minimal H3 based SBC. It comes with 256/512M RAM, a micro SD slot, 10/100Mbit ethernet and a single USB-A port. Signed-off-by: James Pettigrew --- arch/arm/boot/dts/Makefile| 1 + arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts | 126 ++ 2 files changed, 127 insertions(+) create mode 100644 arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile index 7dd4a07..a35f986 100644 --- a/arch/arm/boot/dts/Makefile +++ b/arch/arm/boot/dts/Makefile @@ -792,6 +792,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \ sun8i-a83t-allwinner-h8homlet-v2.dtb \ sun8i-a83t-cubietruck-plus.dtb \ sun8i-h3-bananapi-m2-plus.dtb \ + sun8i-h3-nanopi-neo.dtb \ sun8i-h3-orangepi-2.dtb \ sun8i-h3-orangepi-lite.dtb \ sun8i-h3-orangepi-one.dtb \ diff --git a/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts b/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts new file mode 100644 index 000..3a9eb19 --- /dev/null +++ b/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts @@ -0,0 +1,126 @@ +/* + * Copyright (C) 2016 James Pettigrew + * + * This file is dual-licensed: you can use it either under the terms + * of the GPL or the X11 license, at your option. Note that this dual + * licensing only applies to this file, and not this project as a + * whole. + * + * a) This file is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of the + * License, or (at your option) any later version. + * + * This file is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * Or, alternatively, + * + * b) Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation + * files (the "Software"), to deal in the Software without + * restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following + * conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + +/dts-v1/; +#include "sun8i-h3.dtsi" +#include "sunxi-common-regulators.dtsi" + +#include +#include +#include + +/ { + model = "FriendlyARM NanoPi NEO"; + compatible = "friendlyarm,nanopi-neo", "allwinner,sun8i-h3"; + + aliases { + serial0 = + }; + + chosen { + stdout-path = "serial0:115200n8"; + }; + + leds { + compatible = "gpio-leds"; + pinctrl-names = "default"; + pinctrl-0 = <_opc>, <_r_opc>; + + pwr_led { + label = "nanopi:green:pwr"; + gpios = <_pio 0 10 GPIO_ACTIVE_HIGH>; + default-state = "on"; + }; + + status_led { + label = "nanopi:blue:status"; + gpios = < 0 10 GPIO_ACTIVE_HIGH>; + }; + }; +}; + + { + status = "okay"; +}; + + { + pinctrl-names = "default"; + pinctrl-0 = <_pins_a>, <_cd_pin>; + vmmc-supply = <_vcc3v3>; + bus-width = <4>; + cd-gpios = < 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */ + cd-inverted; + status = "okay"; +}; + + { + status = "okay"; +}; + + { + leds_opc: led_pins@0 { + allwinner,pins = "PA10"; + allwinner,function = "gpio_out"; + allwinner,drive = ; + allwinner,pull = ; + }; +}; + +_pio { + leds_r_opc: led_pins@0 { + allwinner,pins = "PL10"; + allwinner,function = "gpio_out"; + allwinner,drive = ; + allwinner,pull = ; + }; +}; + + { + pinctrl-names = "default"; + pinctrl-0 = <_pins_a>; + status = "okay"; +}; + + { + /* USB VBUS is
[PATCH 1/1] ARM: dts: sun8i: Add dts file for the NanoPi NEO SBC
From: James Pettigrew The NanoPi NEO is a minimal H3 based SBC. It comes with 256/512M RAM, a micro SD slot, 10/100Mbit ethernet and a single USB-A port. Signed-off-by: James Pettigrew --- arch/arm/boot/dts/Makefile| 1 + arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts | 126 ++ 2 files changed, 127 insertions(+) create mode 100644 arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile index 7dd4a07..a35f986 100644 --- a/arch/arm/boot/dts/Makefile +++ b/arch/arm/boot/dts/Makefile @@ -792,6 +792,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \ sun8i-a83t-allwinner-h8homlet-v2.dtb \ sun8i-a83t-cubietruck-plus.dtb \ sun8i-h3-bananapi-m2-plus.dtb \ + sun8i-h3-nanopi-neo.dtb \ sun8i-h3-orangepi-2.dtb \ sun8i-h3-orangepi-lite.dtb \ sun8i-h3-orangepi-one.dtb \ diff --git a/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts b/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts new file mode 100644 index 000..3a9eb19 --- /dev/null +++ b/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts @@ -0,0 +1,126 @@ +/* + * Copyright (C) 2016 James Pettigrew + * + * This file is dual-licensed: you can use it either under the terms + * of the GPL or the X11 license, at your option. Note that this dual + * licensing only applies to this file, and not this project as a + * whole. + * + * a) This file is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of the + * License, or (at your option) any later version. + * + * This file is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * Or, alternatively, + * + * b) Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation + * files (the "Software"), to deal in the Software without + * restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following + * conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + +/dts-v1/; +#include "sun8i-h3.dtsi" +#include "sunxi-common-regulators.dtsi" + +#include +#include +#include + +/ { + model = "FriendlyARM NanoPi NEO"; + compatible = "friendlyarm,nanopi-neo", "allwinner,sun8i-h3"; + + aliases { + serial0 = + }; + + chosen { + stdout-path = "serial0:115200n8"; + }; + + leds { + compatible = "gpio-leds"; + pinctrl-names = "default"; + pinctrl-0 = <_opc>, <_r_opc>; + + pwr_led { + label = "nanopi:green:pwr"; + gpios = <_pio 0 10 GPIO_ACTIVE_HIGH>; + default-state = "on"; + }; + + status_led { + label = "nanopi:blue:status"; + gpios = < 0 10 GPIO_ACTIVE_HIGH>; + }; + }; +}; + + { + status = "okay"; +}; + + { + pinctrl-names = "default"; + pinctrl-0 = <_pins_a>, <_cd_pin>; + vmmc-supply = <_vcc3v3>; + bus-width = <4>; + cd-gpios = < 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */ + cd-inverted; + status = "okay"; +}; + + { + status = "okay"; +}; + + { + leds_opc: led_pins@0 { + allwinner,pins = "PA10"; + allwinner,function = "gpio_out"; + allwinner,drive = ; + allwinner,pull = ; + }; +}; + +_pio { + leds_r_opc: led_pins@0 { + allwinner,pins = "PL10"; + allwinner,function = "gpio_out"; + allwinner,drive = ; + allwinner,pull = ; + }; +}; + + { + pinctrl-names = "default"; + pinctrl-0 = <_pins_a>; + status = "okay"; +}; + + { + /* USB VBUS is always on */ + status = "okay"; +}; -- 1.9.1
Re: [PATCH 2/8] writeback: add wbc_to_write_flags()
On 08/31/2016 05:32 PM, Omar Sandoval wrote: On Wed, Aug 31, 2016 at 11:05:45AM -0600, Jens Axboe wrote: Add wbc_to_write_flags(), which returns the write modifier flags to use, based on a struct writeback_control. No functional changes in this patch, but it prepares us for factoring other wbc fields for write type. Signed-off-by: Jens AxboeReviewed-by: Jan Kara [snip] diff --git a/include/linux/writeback.h b/include/linux/writeback.h index fc1e16c25a29..e1fc25172397 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -100,6 +100,14 @@ struct writeback_control { #endif }; +static inline int wbc_to_write_flags(struct writeback_control *wbc) +{ + if (wbc->sync_mode == WB_SYNC_ALL) + return WRITE_SYNC; + + return WRITE; I think this should be `return 0;` after the op/flags split. WRITE == 1, so this would get interpreted as REQ_FAILFAST_DEV in bi_opf. Good catch, thanks! Fixed up. -- Jens Axboe
Re: [PATCH 2/8] writeback: add wbc_to_write_flags()
On 08/31/2016 05:32 PM, Omar Sandoval wrote: On Wed, Aug 31, 2016 at 11:05:45AM -0600, Jens Axboe wrote: Add wbc_to_write_flags(), which returns the write modifier flags to use, based on a struct writeback_control. No functional changes in this patch, but it prepares us for factoring other wbc fields for write type. Signed-off-by: Jens Axboe Reviewed-by: Jan Kara [snip] diff --git a/include/linux/writeback.h b/include/linux/writeback.h index fc1e16c25a29..e1fc25172397 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -100,6 +100,14 @@ struct writeback_control { #endif }; +static inline int wbc_to_write_flags(struct writeback_control *wbc) +{ + if (wbc->sync_mode == WB_SYNC_ALL) + return WRITE_SYNC; + + return WRITE; I think this should be `return 0;` after the op/flags split. WRITE == 1, so this would get interpreted as REQ_FAILFAST_DEV in bi_opf. Good catch, thanks! Fixed up. -- Jens Axboe
Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu
Few commits and patch changed according to Greg's comments. Regards Alex >From 186c534b0b8b9649fbfce05b0b4f90f764c571a4 Mon Sep 17 00:00:00 2001 From: Alex ShiDate: Tue, 16 Aug 2016 15:29:01 +0800 Subject: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu Adding /sys/devices/system/cpu/cpux/power/pm_qos_resume_latency_us for each of cpus. The pm_qos_resume_latency usage defined in Documentation/ABI/testing/sysfs-devices-power The cpu-dma PM QoS constraint impacts all the cpus in the system. There is no way to let the user to choose a PM QoS constraint per cpu. The following patch exposes to the userspace a per cpu based sysfs file in order to let the userspace to change the value of the PM QoS latency constraint. This change is inoperative in its form and the cpuidle governors have to take into account the per cpu latency constraint in addition to the global cpu-dma latency constraint in order to operate properly. Signed-off-by: Alex Shi To: linux-kernel@vger.kernel.org To: Greg Kroah-Hartman Cc: linux...@vger.kernel.org Cc: Ulf Hansson Cc: Daniel Lezcano --- drivers/base/cpu.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c index 4c28e1a..2c3b359 100644 --- a/drivers/base/cpu.c +++ b/drivers/base/cpu.c @@ -17,6 +17,7 @@ #include #include #include +#include #include "base.h" @@ -376,6 +377,7 @@ int register_cpu(struct cpu *cpu, int num) per_cpu(cpu_sys_devices, num) = >dev; register_cpu_under_node(num, cpu_to_node(num)); + dev_pm_qos_expose_latency_limit(>dev, 0); return 0; } -- 2.8.1.101.g72d917a
Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu
Few commits and patch changed according to Greg's comments. Regards Alex >From 186c534b0b8b9649fbfce05b0b4f90f764c571a4 Mon Sep 17 00:00:00 2001 From: Alex Shi Date: Tue, 16 Aug 2016 15:29:01 +0800 Subject: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu Adding /sys/devices/system/cpu/cpux/power/pm_qos_resume_latency_us for each of cpus. The pm_qos_resume_latency usage defined in Documentation/ABI/testing/sysfs-devices-power The cpu-dma PM QoS constraint impacts all the cpus in the system. There is no way to let the user to choose a PM QoS constraint per cpu. The following patch exposes to the userspace a per cpu based sysfs file in order to let the userspace to change the value of the PM QoS latency constraint. This change is inoperative in its form and the cpuidle governors have to take into account the per cpu latency constraint in addition to the global cpu-dma latency constraint in order to operate properly. Signed-off-by: Alex Shi To: linux-kernel@vger.kernel.org To: Greg Kroah-Hartman Cc: linux...@vger.kernel.org Cc: Ulf Hansson Cc: Daniel Lezcano --- drivers/base/cpu.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c index 4c28e1a..2c3b359 100644 --- a/drivers/base/cpu.c +++ b/drivers/base/cpu.c @@ -17,6 +17,7 @@ #include #include #include +#include #include "base.h" @@ -376,6 +377,7 @@ int register_cpu(struct cpu *cpu, int num) per_cpu(cpu_sys_devices, num) = >dev; register_cpu_under_node(num, cpu_to_node(num)); + dev_pm_qos_expose_latency_limit(>dev, 0); return 0; } -- 2.8.1.101.g72d917a
Re: [PATCH 5/5] net: axienet: constify ethtool_ops structures
From: Julia LawallDate: Thu, 1 Sep 2016 00:21:23 +0200 > Check for ethtool_ops structures that are only stored in the ethtool_ops > field of a net_device structure or passed as the second argument to > netdev_set_default_ethtool_ops. These contexts are declared const, so > ethtool_ops structures that have these properties can be declared as const > also. > > The semantic patch that makes this change is as follows: > (http://coccinelle.lip6.fr/) ... > Suggested-by: Stephen Hemminger > > Signed-off-by: Julia Lawall Applied.
Re: [PATCH 5/5] net: axienet: constify ethtool_ops structures
From: Julia Lawall Date: Thu, 1 Sep 2016 00:21:23 +0200 > Check for ethtool_ops structures that are only stored in the ethtool_ops > field of a net_device structure or passed as the second argument to > netdev_set_default_ethtool_ops. These contexts are declared const, so > ethtool_ops structures that have these properties can be declared as const > also. > > The semantic patch that makes this change is as follows: > (http://coccinelle.lip6.fr/) ... > Suggested-by: Stephen Hemminger > > Signed-off-by: Julia Lawall Applied.
Re: [PATCH 1/5] net: mediatek: constify ethtool_ops structures
From: Julia LawallDate: Thu, 1 Sep 2016 00:21:19 +0200 > Check for ethtool_ops structures that are only stored in the ethtool_ops > field of a net_device structure or passed as the second argument to > netdev_set_default_ethtool_ops. These contexts are declared const, so > ethtool_ops structures that have these properties can be declared as const > also. ... > Suggested-by: Stephen Hemminger > > Signed-off-by: Julia Lawall Applied.
Re: [PATCH 1/5] net: mediatek: constify ethtool_ops structures
From: Julia Lawall Date: Thu, 1 Sep 2016 00:21:19 +0200 > Check for ethtool_ops structures that are only stored in the ethtool_ops > field of a net_device structure or passed as the second argument to > netdev_set_default_ethtool_ops. These contexts are declared const, so > ethtool_ops structures that have these properties can be declared as const > also. ... > Suggested-by: Stephen Hemminger > > Signed-off-by: Julia Lawall Applied.
Re: [PATCH 4/5] r8152: constify ethtool_ops structures
From: Julia LawallDate: Thu, 1 Sep 2016 00:21:22 +0200 > Check for ethtool_ops structures that are only stored in the ethtool_ops > field of a net_device structure or passed as the second argument to > netdev_set_default_ethtool_ops. These contexts are declared const, so > ethtool_ops structures that have these properties can be declared as const > also. > > The semantic patch that makes this change is as follows: > (http://coccinelle.lip6.fr/) ... > Suggested-by: Stephen Hemminger > > Signed-off-by: Julia Lawall Applied.
Re: [PATCH 4/5] r8152: constify ethtool_ops structures
From: Julia Lawall Date: Thu, 1 Sep 2016 00:21:22 +0200 > Check for ethtool_ops structures that are only stored in the ethtool_ops > field of a net_device structure or passed as the second argument to > netdev_set_default_ethtool_ops. These contexts are declared const, so > ethtool_ops structures that have these properties can be declared as const > also. > > The semantic patch that makes this change is as follows: > (http://coccinelle.lip6.fr/) ... > Suggested-by: Stephen Hemminger > > Signed-off-by: Julia Lawall Applied.
Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu
On 09/01/2016 11:39 AM, Alex Shi wrote: > User can set values on each of cpu, like limit 100ms on cpu0, that means > the cpu0 response time should be in 100ms in possible idle. It similar > with DMA_LATENCY, but that request is for all cpu. This is just for > particular cpu, like a interrupt pined CPU. Sorry for typo! s/100ms/100us/ > > echo 100 > /sys/devices/system/cpu/cpu0/power/pm_qos_resume_latency_us >> >
Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu
On 09/01/2016 11:39 AM, Alex Shi wrote: > User can set values on each of cpu, like limit 100ms on cpu0, that means > the cpu0 response time should be in 100ms in possible idle. It similar > with DMA_LATENCY, but that request is for all cpu. This is just for > particular cpu, like a interrupt pined CPU. Sorry for typo! s/100ms/100us/ > > echo 100 > /sys/devices/system/cpu/cpu0/power/pm_qos_resume_latency_us >> >
Re: [PATCH 04/13] perf/core: Extend perf_output_sample_regs() to include perf_arch_regs
On Tuesday 30 August 2016 09:41 PM, Nilay Vaish wrote: On 28 August 2016 at 16:00, Madhavan Srinivasanwrote: diff --git a/kernel/events/core.c b/kernel/events/core.c index 274288819829..e16bf4d057d1 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -5371,16 +5371,24 @@ u64 __attribute__((weak)) perf_arch_reg_value(struct perf_arch_regs *regs, static void perf_output_sample_regs(struct perf_output_handle *handle, - struct pt_regs *regs, u64 mask) + struct perf_regs *regs, u64 mask) { int bit; DECLARE_BITMAP(_mask, 64); + u64 arch_regs_mask = regs->arch_regs_mask; bitmap_from_u64(_mask, mask); for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) { u64 val; - val = perf_reg_value(regs, bit); + val = perf_reg_value(regs->regs, bit); + perf_output_put(handle, val); + } + + bitmap_from_u64(_mask, arch_regs_mask); + for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) { + u64 val; + val = perf_arch_reg_value(regs->arch_regs, bit); perf_output_put(handle, val); } } @@ -5792,7 +5800,7 @@ void perf_output_sample(struct perf_output_handle *handle, if (abi) { u64 mask = event->attr.sample_regs_user; perf_output_sample_regs(handle, - data->regs_user.regs, + >regs_user, mask); } } @@ -5827,7 +5835,7 @@ void perf_output_sample(struct perf_output_handle *handle, u64 mask = event->attr.sample_regs_intr; perf_output_sample_regs(handle, - data->regs_intr.regs, + >regs_intr, mask); } } -- 2.7.4 I would like to suggest a slightly different version. Would it make more sense to have something like following: I agree we are outputting two different structures, but since we use the INTR_REG infrastructure to dump the arch pmu registers, I preferred to extend perf_output_sample_regs. But I guess I can break it up. Maddy @@ -5792,7 +5800,7 @@ void perf_output_sample(struct perf_output_handle *handle, if (abi) { u64 mask = event->attr.sample_regs_user; perf_output_sample_regs(handle, data->regs_user.regs, mask); } + + if (arch_regs_mask) { + perf_output_pmu_regs(handle, data->regs_users.arch_regs, arch_regs_mask); + } } Somehow I don't like outputting the two sets of registers through the same function call. -- Nilay
Re: [PATCH 04/13] perf/core: Extend perf_output_sample_regs() to include perf_arch_regs
On Tuesday 30 August 2016 09:41 PM, Nilay Vaish wrote: On 28 August 2016 at 16:00, Madhavan Srinivasan wrote: diff --git a/kernel/events/core.c b/kernel/events/core.c index 274288819829..e16bf4d057d1 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -5371,16 +5371,24 @@ u64 __attribute__((weak)) perf_arch_reg_value(struct perf_arch_regs *regs, static void perf_output_sample_regs(struct perf_output_handle *handle, - struct pt_regs *regs, u64 mask) + struct perf_regs *regs, u64 mask) { int bit; DECLARE_BITMAP(_mask, 64); + u64 arch_regs_mask = regs->arch_regs_mask; bitmap_from_u64(_mask, mask); for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) { u64 val; - val = perf_reg_value(regs, bit); + val = perf_reg_value(regs->regs, bit); + perf_output_put(handle, val); + } + + bitmap_from_u64(_mask, arch_regs_mask); + for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) { + u64 val; + val = perf_arch_reg_value(regs->arch_regs, bit); perf_output_put(handle, val); } } @@ -5792,7 +5800,7 @@ void perf_output_sample(struct perf_output_handle *handle, if (abi) { u64 mask = event->attr.sample_regs_user; perf_output_sample_regs(handle, - data->regs_user.regs, + >regs_user, mask); } } @@ -5827,7 +5835,7 @@ void perf_output_sample(struct perf_output_handle *handle, u64 mask = event->attr.sample_regs_intr; perf_output_sample_regs(handle, - data->regs_intr.regs, + >regs_intr, mask); } } -- 2.7.4 I would like to suggest a slightly different version. Would it make more sense to have something like following: I agree we are outputting two different structures, but since we use the INTR_REG infrastructure to dump the arch pmu registers, I preferred to extend perf_output_sample_regs. But I guess I can break it up. Maddy @@ -5792,7 +5800,7 @@ void perf_output_sample(struct perf_output_handle *handle, if (abi) { u64 mask = event->attr.sample_regs_user; perf_output_sample_regs(handle, data->regs_user.regs, mask); } + + if (arch_regs_mask) { + perf_output_pmu_regs(handle, data->regs_users.arch_regs, arch_regs_mask); + } } Somehow I don't like outputting the two sets of registers through the same function call. -- Nilay
[PATCH 6/7] serial: 8250_fintek: Add F81866 Support
Fintek F81866 is a LPC to 6 UARTs SuperIO. It has fully functional UARTs likes F81216H. It's also need check the IRQ mode with system assigned, but the configuration is not the same with F81216 series. F81866 IRQ Mode setting: 0xf0 Bit1: IRQ_MODE0 Bit0: Share mode (always on) 0xf6 Bit3: IRQ_MODE1 Level/Low: IRQ_MODE0:0, IRQ_MODE1:0 Edge/High: IRQ_MODE0:1, IRQ_MODE1:0 The following list is brief descriptions of F81866: F81866 (1010) 9Bit/High baud rate(not implements with mainline) RS485, 128Bytes FIFO (implemented) Signed-off-by: Ji-Ze Hong (Peter Hong)--- drivers/tty/serial/8250/8250_fintek.c | 75 --- 1 file changed, 69 insertions(+), 6 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index a62cfdd..1194e57 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -21,6 +21,7 @@ #define EXIT_KEY 0xAA #define CHIP_ID1 0x20 #define CHIP_ID2 0x21 +#define CHIP_ID_F81866 0x1010 #define CHIP_ID_F81216AD 0x1602 #define CHIP_ID_F81216H 0x0501 #define CHIP_ID_F81216 0x0802 @@ -50,6 +51,26 @@ #define RXFTHR_MODE_MASK (BIT(5) | BIT(4)) #define RXFTHR_MODE_4X BIT(5) +#define F81216_LDN_LOW 0x0 +#define F81216_LDN_HIGH0x4 + +/* + * F81866 registers + * + * The IRQ setting mode of F81866 is not the same with F81216 series. + * Level/Low: IRQ_MODE0:0, IRQ_MODE1:0 + * Edge/High: IRQ_MODE0:1, IRQ_MODE1:0 + */ +#define F81866_IRQ_MODE0xf0 +#define F81866_IRQ_SHARE BIT(0) +#define F81866_IRQ_MODE0 BIT(1) + +#define F81866_FIFO_CTRL FIFO_CTRL +#define F81866_IRQ_MODE1 BIT(3) + +#define F81866_LDN_LOW 0x10 +#define F81866_LDN_HIGH0x16 + struct fintek_8250 { u16 pid; u16 base_port; @@ -109,6 +130,7 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata) chip |= sio_read_reg(pdata, CHIP_ID2) << 8; switch (chip) { + case CHIP_ID_F81866: case CHIP_ID_F81216AD: case CHIP_ID_F81216H: case CHIP_ID_F81216: @@ -121,6 +143,26 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata) return 0; } +static int fintek_8250_get_ldn_range(struct fintek_8250 *pdata, int *min, +int *max) +{ + switch (pdata->pid) { + case CHIP_ID_F81866: + *min = F81866_LDN_LOW; + *max = F81866_LDN_HIGH; + return 0; + + case CHIP_ID_F81216AD: + case CHIP_ID_F81216H: + case CHIP_ID_F81216: + *min = F81216_LDN_LOW; + *max = F81216_LDN_HIGH; + return 0; + } + + return -ENODEV; +} + static int fintek_8250_rs485_config(struct uart_port *port, struct serial_rs485 *rs485) { @@ -175,6 +217,7 @@ static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata) default: /* Default 16Bytes FIFO */ return; + case CHIP_ID_F81866: case CHIP_ID_F81216H: /* 128Bytes FIFO */ sio_write_mask_reg(pdata, FIFO_CTRL, FIFO_MODE_MASK | RXFTHR_MODE_MASK, @@ -186,9 +229,27 @@ static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata) static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level) { sio_write_reg(pdata, LDN, pdata->index); - sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE); - sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK, - is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); + + switch (pdata->pid) { + case CHIP_ID_F81866: + sio_write_mask_reg(pdata, F81866_FIFO_CTRL, F81866_IRQ_MODE1, + 0); + sio_write_mask_reg(pdata, F81866_IRQ_MODE, F81866_IRQ_SHARE, + F81866_IRQ_SHARE); + sio_write_mask_reg(pdata, F81866_IRQ_MODE, + F81866_IRQ_MODE0, + is_level ? 0 : F81866_IRQ_MODE0); + break; + + case CHIP_ID_F81216AD: + case CHIP_ID_F81216H: + case CHIP_ID_F81216: + sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, + IRQ_SHARE); + sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK, + is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); + break; + } } static int find_base_port(struct fintek_8250 *pdata, u16 io_address, @@ -198,7 +259,7 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address, static const u8 keys[] = {0x77, 0xa0, 0x87, 0x67}; struct irq_data *irq_data; bool level_mode = false; -
[PATCH 6/7] serial: 8250_fintek: Add F81866 Support
Fintek F81866 is a LPC to 6 UARTs SuperIO. It has fully functional UARTs likes F81216H. It's also need check the IRQ mode with system assigned, but the configuration is not the same with F81216 series. F81866 IRQ Mode setting: 0xf0 Bit1: IRQ_MODE0 Bit0: Share mode (always on) 0xf6 Bit3: IRQ_MODE1 Level/Low: IRQ_MODE0:0, IRQ_MODE1:0 Edge/High: IRQ_MODE0:1, IRQ_MODE1:0 The following list is brief descriptions of F81866: F81866 (1010) 9Bit/High baud rate(not implements with mainline) RS485, 128Bytes FIFO (implemented) Signed-off-by: Ji-Ze Hong (Peter Hong) --- drivers/tty/serial/8250/8250_fintek.c | 75 --- 1 file changed, 69 insertions(+), 6 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index a62cfdd..1194e57 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -21,6 +21,7 @@ #define EXIT_KEY 0xAA #define CHIP_ID1 0x20 #define CHIP_ID2 0x21 +#define CHIP_ID_F81866 0x1010 #define CHIP_ID_F81216AD 0x1602 #define CHIP_ID_F81216H 0x0501 #define CHIP_ID_F81216 0x0802 @@ -50,6 +51,26 @@ #define RXFTHR_MODE_MASK (BIT(5) | BIT(4)) #define RXFTHR_MODE_4X BIT(5) +#define F81216_LDN_LOW 0x0 +#define F81216_LDN_HIGH0x4 + +/* + * F81866 registers + * + * The IRQ setting mode of F81866 is not the same with F81216 series. + * Level/Low: IRQ_MODE0:0, IRQ_MODE1:0 + * Edge/High: IRQ_MODE0:1, IRQ_MODE1:0 + */ +#define F81866_IRQ_MODE0xf0 +#define F81866_IRQ_SHARE BIT(0) +#define F81866_IRQ_MODE0 BIT(1) + +#define F81866_FIFO_CTRL FIFO_CTRL +#define F81866_IRQ_MODE1 BIT(3) + +#define F81866_LDN_LOW 0x10 +#define F81866_LDN_HIGH0x16 + struct fintek_8250 { u16 pid; u16 base_port; @@ -109,6 +130,7 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata) chip |= sio_read_reg(pdata, CHIP_ID2) << 8; switch (chip) { + case CHIP_ID_F81866: case CHIP_ID_F81216AD: case CHIP_ID_F81216H: case CHIP_ID_F81216: @@ -121,6 +143,26 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata) return 0; } +static int fintek_8250_get_ldn_range(struct fintek_8250 *pdata, int *min, +int *max) +{ + switch (pdata->pid) { + case CHIP_ID_F81866: + *min = F81866_LDN_LOW; + *max = F81866_LDN_HIGH; + return 0; + + case CHIP_ID_F81216AD: + case CHIP_ID_F81216H: + case CHIP_ID_F81216: + *min = F81216_LDN_LOW; + *max = F81216_LDN_HIGH; + return 0; + } + + return -ENODEV; +} + static int fintek_8250_rs485_config(struct uart_port *port, struct serial_rs485 *rs485) { @@ -175,6 +217,7 @@ static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata) default: /* Default 16Bytes FIFO */ return; + case CHIP_ID_F81866: case CHIP_ID_F81216H: /* 128Bytes FIFO */ sio_write_mask_reg(pdata, FIFO_CTRL, FIFO_MODE_MASK | RXFTHR_MODE_MASK, @@ -186,9 +229,27 @@ static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata) static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level) { sio_write_reg(pdata, LDN, pdata->index); - sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE); - sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK, - is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); + + switch (pdata->pid) { + case CHIP_ID_F81866: + sio_write_mask_reg(pdata, F81866_FIFO_CTRL, F81866_IRQ_MODE1, + 0); + sio_write_mask_reg(pdata, F81866_IRQ_MODE, F81866_IRQ_SHARE, + F81866_IRQ_SHARE); + sio_write_mask_reg(pdata, F81866_IRQ_MODE, + F81866_IRQ_MODE0, + is_level ? 0 : F81866_IRQ_MODE0); + break; + + case CHIP_ID_F81216AD: + case CHIP_ID_F81216H: + case CHIP_ID_F81216: + sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, + IRQ_SHARE); + sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK, + is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); + break; + } } static int find_base_port(struct fintek_8250 *pdata, u16 io_address, @@ -198,7 +259,7 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address, static const u8 keys[] = {0x77, 0xa0, 0x87, 0x67}; struct irq_data *irq_data; bool level_mode = false; - int i, j, k; + int i, j,
[PATCH 7/7] serial: 8250_fintek: Add F81865 Support
Fintek F81865 is a LPC to 6 UARTs SuperIO. It has less functional UARTs likes F81866. It's also need check the IRQ mode with system assigned, but the configuration is not the same with F81216 series. F81865 IRQ Mode setting: 0xf0 Bit1: IRQ_MODE0 Bit0: Share mode (always on) Level/Low: IRQ_MODE0:0 Edge/High: IRQ_MODE0:1 The following list is brief descriptions of F81865: F81865 (0704) 9Bit(not implements with mainline) RS485(implemented) Signed-off-by: Ji-Ze Hong (Peter Hong)--- drivers/tty/serial/8250/8250_fintek.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index 1194e57..3fb9ab6 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -21,6 +21,7 @@ #define EXIT_KEY 0xAA #define CHIP_ID1 0x20 #define CHIP_ID2 0x21 +#define CHIP_ID_F81865 0x0407 #define CHIP_ID_F81866 0x1010 #define CHIP_ID_F81216AD 0x1602 #define CHIP_ID_F81216H 0x0501 @@ -130,6 +131,7 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata) chip |= sio_read_reg(pdata, CHIP_ID2) << 8; switch (chip) { + case CHIP_ID_F81865: case CHIP_ID_F81866: case CHIP_ID_F81216AD: case CHIP_ID_F81216H: @@ -147,6 +149,7 @@ static int fintek_8250_get_ldn_range(struct fintek_8250 *pdata, int *min, int *max) { switch (pdata->pid) { + case CHIP_ID_F81865: case CHIP_ID_F81866: *min = F81866_LDN_LOW; *max = F81866_LDN_HIGH; @@ -231,9 +234,12 @@ static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level) sio_write_reg(pdata, LDN, pdata->index); switch (pdata->pid) { + case CHIP_ID_F81865: case CHIP_ID_F81866: - sio_write_mask_reg(pdata, F81866_FIFO_CTRL, F81866_IRQ_MODE1, - 0); + if (pdata->pid == CHIP_ID_F81866) + sio_write_mask_reg(pdata, F81866_FIFO_CTRL, + F81866_IRQ_MODE1, 0); + sio_write_mask_reg(pdata, F81866_IRQ_MODE, F81866_IRQ_SHARE, F81866_IRQ_SHARE); sio_write_mask_reg(pdata, F81866_IRQ_MODE, @@ -312,6 +318,7 @@ static void fintek_8250_set_rs485_handler(struct uart_8250_port *uart) default: /* No RS485 Auto direction functional */ break; + case CHIP_ID_F81865: case CHIP_ID_F81866: case CHIP_ID_F81216AD: case CHIP_ID_F81216H: -- 1.9.1
[PATCH 1/7] serial: 8250_fintek: Refactoring read/write method
If we need to access SuperIO registers, It should write register offset to base_addr and read/write value to base_addr + 1 to perform read/write. We can make it more simply with write/read functions. This patch add sio_read_reg()/sio_write_reg()/sio_write_mask_reg() to reduce SuperIO register operation with lot of outb()/inb(). Signed-off-by: Ji-Ze Hong (Peter Hong)--- drivers/tty/serial/8250/8250_fintek.c | 73 ++- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index 0facc78..2ae846e 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -49,6 +49,27 @@ struct fintek_8250 { u8 key; }; +static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg) +{ + outb(reg, pdata->base_port + ADDR_PORT); + return inb(pdata->base_port + DATA_PORT); +} + +static void sio_write_reg(struct fintek_8250 *pdata, u8 reg, u8 data) +{ + outb(reg, pdata->base_port + ADDR_PORT); + outb(data, pdata->base_port + DATA_PORT); +} + +static void sio_write_mask_reg(struct fintek_8250 *pdata, u8 reg, u8 mask, + u8 data) +{ + u8 tmp; + + tmp = (sio_read_reg(pdata, reg) & ~mask) | (mask & data); + sio_write_reg(pdata, reg, tmp); +} + static int fintek_8250_enter_key(u16 base_port, u8 key) { if (!request_muxed_region(base_port, 2, "8250_fintek")) @@ -66,22 +87,18 @@ static void fintek_8250_exit_key(u16 base_port) release_region(base_port + ADDR_PORT, 2); } -static int fintek_8250_check_id(u16 base_port) +static int fintek_8250_check_id(struct fintek_8250 *pdata) { u16 chip; - outb(VENDOR_ID1, base_port + ADDR_PORT); - if (inb(base_port + DATA_PORT) != VENDOR_ID1_VAL) + if (sio_read_reg(pdata, VENDOR_ID1) != VENDOR_ID1_VAL) return -ENODEV; - outb(VENDOR_ID2, base_port + ADDR_PORT); - if (inb(base_port + DATA_PORT) != VENDOR_ID2_VAL) + if (sio_read_reg(pdata, VENDOR_ID2) != VENDOR_ID2_VAL) return -ENODEV; - outb(CHIP_ID1, base_port + ADDR_PORT); - chip = inb(base_port + DATA_PORT); - outb(CHIP_ID2, base_port + ADDR_PORT); - chip |= inb(base_port + DATA_PORT) << 8; + chip = sio_read_reg(pdata, CHIP_ID1); + chip |= sio_read_reg(pdata, CHIP_ID2) << 8; if (chip != CHIP_ID_0 && chip != CHIP_ID_1) return -ENODEV; @@ -128,10 +145,8 @@ static int fintek_8250_rs485_config(struct uart_port *port, if (fintek_8250_enter_key(pdata->base_port, pdata->key)) return -EBUSY; - outb(LDN, pdata->base_port + ADDR_PORT); - outb(pdata->index, pdata->base_port + DATA_PORT); - outb(RS485, pdata->base_port + ADDR_PORT); - outb(config, pdata->base_port + DATA_PORT); + sio_write_reg(pdata, LDN, pdata->index); + sio_write_reg(pdata, RS485, config); fintek_8250_exit_key(pdata->base_port); port->rs485 = *rs485; @@ -147,10 +162,12 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address) for (i = 0; i < ARRAY_SIZE(addr); i++) { for (j = 0; j < ARRAY_SIZE(keys); j++) { + pdata->base_port = addr[i]; + pdata->key = keys[j]; if (fintek_8250_enter_key(addr[i], keys[j])) continue; - if (fintek_8250_check_id(addr[i])) { + if (fintek_8250_check_id(pdata)) { fintek_8250_exit_key(addr[i]); continue; } @@ -158,19 +175,13 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address) for (k = 0; k < 4; k++) { u16 aux; - outb(LDN, addr[i] + ADDR_PORT); - outb(k, addr[i] + DATA_PORT); - - outb(IO_ADDR1, addr[i] + ADDR_PORT); - aux = inb(addr[i] + DATA_PORT); - outb(IO_ADDR2, addr[i] + ADDR_PORT); - aux |= inb(addr[i] + DATA_PORT) << 8; + sio_write_reg(pdata, LDN, k); + aux = sio_read_reg(pdata, IO_ADDR1); + aux |= sio_read_reg(pdata, IO_ADDR2) << 8; if (aux != io_address) continue; fintek_8250_exit_key(addr[i]); - pdata->key = keys[j]; - pdata->base_port = addr[i]; pdata->index = k; return 0; @@ -186,24 +197,16 @@ static int
[PATCH 2/7] serial: 8250_fintek: Set IRQ Mode when port probed
Set IRQ Mode when port probed in find_base_port() It should hold the IO port premission via fintek_8250_enter_key() and release via fintek_8250_exit_key() when we configure the SuperIO. This patch will move all SuperIO configure operations to find_base_port() to reduce fintek_8250_enter_key()/fintek_8250_exit_key() usage. Suggested-by: Ricardo Ribalda DelgadoSigned-off-by: Ji-Ze Hong (Peter Hong) --- drivers/tty/serial/8250/8250_fintek.c | 36 ++- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index 2ae846e..cd8903f 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -49,6 +49,9 @@ struct fintek_8250 { u8 key; }; +static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, +bool level_mode); + static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg) { outb(reg, pdata->base_port + ADDR_PORT); @@ -154,10 +157,13 @@ static int fintek_8250_rs485_config(struct uart_port *port, return 0; } -static int find_base_port(struct fintek_8250 *pdata, u16 io_address) +static int find_base_port(struct fintek_8250 *pdata, u16 io_address, + unsigned int irq) { static const u16 addr[] = {0x4e, 0x2e}; static const u8 keys[] = {0x77, 0xa0, 0x87, 0x67}; + struct irq_data *irq_data; + bool level_mode = false; int i, j, k; for (i = 0; i < ARRAY_SIZE(addr); i++) { @@ -181,9 +187,16 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address) if (aux != io_address) continue; - fintek_8250_exit_key(addr[i]); pdata->index = k; + irq_data = irq_get_irq_data(irq); + if (irq_data) + level_mode = + irqd_is_level_type(irq_data); + + fintek_8250_set_irq_mode(pdata, level_mode); + fintek_8250_exit_key(addr[i]); + return 0; } @@ -194,31 +207,20 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address) return -ENODEV; } -static int fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool level_mode) +static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level) { - int status; - - status = fintek_8250_enter_key(pdata->base_port, pdata->key); - if (status) - return status; - sio_write_reg(pdata, LDN, pdata->index); sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE); sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK, - level_mode ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); - - fintek_8250_exit_key(pdata->base_port); - return 0; + is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); } int fintek_8250_probe(struct uart_8250_port *uart) { struct fintek_8250 *pdata; struct fintek_8250 probe_data; - struct irq_data *irq_data = irq_get_irq_data(uart->port.irq); - bool level_mode = irqd_is_level_type(irq_data); - if (find_base_port(_data, uart->port.iobase)) + if (find_base_port(_data, uart->port.iobase, uart->port.irq)) return -ENODEV; pdata = devm_kzalloc(uart->port.dev, sizeof(*pdata), GFP_KERNEL); @@ -229,5 +231,5 @@ int fintek_8250_probe(struct uart_8250_port *uart) uart->port.rs485_config = fintek_8250_rs485_config; uart->port.private_data = pdata; - return fintek_8250_set_irq_mode(pdata, level_mode); + return 0; } -- 1.9.1
[PATCH 4/7] serial: 8250_fintek: Rearrange function
We change the position of fintek_8250_set_irq_mode() above the find_base_port() to eliminate the prototype define. Signed-off-by: Ji-Ze Hong (Peter Hong)--- drivers/tty/serial/8250/8250_fintek.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index 5625203..921f742 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -56,9 +56,6 @@ struct fintek_8250 { u8 key; }; -static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, -bool level_mode); - static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg) { outb(reg, pdata->base_port + ADDR_PORT); @@ -179,6 +176,14 @@ static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata) } } +static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level) +{ + sio_write_reg(pdata, LDN, pdata->index); + sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE); + sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK, + is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); +} + static int find_base_port(struct fintek_8250 *pdata, u16 io_address, unsigned int irq) { @@ -230,14 +235,6 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address, return -ENODEV; } -static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level) -{ - sio_write_reg(pdata, LDN, pdata->index); - sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE); - sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK, - is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); -} - int fintek_8250_probe(struct uart_8250_port *uart) { struct fintek_8250 *pdata; -- 1.9.1
[PATCH 5/7] serial: 8250_fintek: Add F81216 Support
Fintek F81216 is a LPC to 4 UARTs device. It's the F81216 series but support less functional than F81216AD/F81216H The following list is brief descriptions of F81216 series: F81216H (0105) 9Bit/High baud rate(not implements with mainline) RS485, 128Bytes FIFO (implemented) F81216AD (0216) 9Bit(not implements with mainline) RS485(implemented) F81216 (0208) basically 16550A Signed-off-by: Ji-Ze Hong (Peter Hong)--- drivers/tty/serial/8250/8250_fintek.c | 26 -- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index 921f742..a62cfdd 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -23,6 +23,7 @@ #define CHIP_ID2 0x21 #define CHIP_ID_F81216AD 0x1602 #define CHIP_ID_F81216H 0x0501 +#define CHIP_ID_F81216 0x0802 #define VENDOR_ID1 0x23 #define VENDOR_ID1_VAL 0x19 #define VENDOR_ID2 0x24 @@ -107,8 +108,14 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata) chip = sio_read_reg(pdata, CHIP_ID1); chip |= sio_read_reg(pdata, CHIP_ID2) << 8; - if (chip != CHIP_ID_F81216AD && chip != CHIP_ID_F81216H) + switch (chip) { + case CHIP_ID_F81216AD: + case CHIP_ID_F81216H: + case CHIP_ID_F81216: + break; + default: return -ENODEV; + } pdata->pid = chip; return 0; @@ -235,6 +242,21 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address, return -ENODEV; } +static void fintek_8250_set_rs485_handler(struct uart_8250_port *uart) +{ + struct fintek_8250 *pdata = uart->port.private_data; + + switch (pdata->pid) { + default: /* No RS485 Auto direction functional */ + break; + + case CHIP_ID_F81216AD: + case CHIP_ID_F81216H: + uart->port.rs485_config = fintek_8250_rs485_config; + break; + } +} + int fintek_8250_probe(struct uart_8250_port *uart) { struct fintek_8250 *pdata; @@ -248,8 +270,8 @@ int fintek_8250_probe(struct uart_8250_port *uart) return -ENOMEM; memcpy(pdata, _data, sizeof(probe_data)); - uart->port.rs485_config = fintek_8250_rs485_config; uart->port.private_data = pdata; + fintek_8250_set_rs485_handler(uart); return 0; } -- 1.9.1
[PATCH 7/7] serial: 8250_fintek: Add F81865 Support
Fintek F81865 is a LPC to 6 UARTs SuperIO. It has less functional UARTs likes F81866. It's also need check the IRQ mode with system assigned, but the configuration is not the same with F81216 series. F81865 IRQ Mode setting: 0xf0 Bit1: IRQ_MODE0 Bit0: Share mode (always on) Level/Low: IRQ_MODE0:0 Edge/High: IRQ_MODE0:1 The following list is brief descriptions of F81865: F81865 (0704) 9Bit(not implements with mainline) RS485(implemented) Signed-off-by: Ji-Ze Hong (Peter Hong) --- drivers/tty/serial/8250/8250_fintek.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index 1194e57..3fb9ab6 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -21,6 +21,7 @@ #define EXIT_KEY 0xAA #define CHIP_ID1 0x20 #define CHIP_ID2 0x21 +#define CHIP_ID_F81865 0x0407 #define CHIP_ID_F81866 0x1010 #define CHIP_ID_F81216AD 0x1602 #define CHIP_ID_F81216H 0x0501 @@ -130,6 +131,7 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata) chip |= sio_read_reg(pdata, CHIP_ID2) << 8; switch (chip) { + case CHIP_ID_F81865: case CHIP_ID_F81866: case CHIP_ID_F81216AD: case CHIP_ID_F81216H: @@ -147,6 +149,7 @@ static int fintek_8250_get_ldn_range(struct fintek_8250 *pdata, int *min, int *max) { switch (pdata->pid) { + case CHIP_ID_F81865: case CHIP_ID_F81866: *min = F81866_LDN_LOW; *max = F81866_LDN_HIGH; @@ -231,9 +234,12 @@ static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level) sio_write_reg(pdata, LDN, pdata->index); switch (pdata->pid) { + case CHIP_ID_F81865: case CHIP_ID_F81866: - sio_write_mask_reg(pdata, F81866_FIFO_CTRL, F81866_IRQ_MODE1, - 0); + if (pdata->pid == CHIP_ID_F81866) + sio_write_mask_reg(pdata, F81866_FIFO_CTRL, + F81866_IRQ_MODE1, 0); + sio_write_mask_reg(pdata, F81866_IRQ_MODE, F81866_IRQ_SHARE, F81866_IRQ_SHARE); sio_write_mask_reg(pdata, F81866_IRQ_MODE, @@ -312,6 +318,7 @@ static void fintek_8250_set_rs485_handler(struct uart_8250_port *uart) default: /* No RS485 Auto direction functional */ break; + case CHIP_ID_F81865: case CHIP_ID_F81866: case CHIP_ID_F81216AD: case CHIP_ID_F81216H: -- 1.9.1
[PATCH 1/7] serial: 8250_fintek: Refactoring read/write method
If we need to access SuperIO registers, It should write register offset to base_addr and read/write value to base_addr + 1 to perform read/write. We can make it more simply with write/read functions. This patch add sio_read_reg()/sio_write_reg()/sio_write_mask_reg() to reduce SuperIO register operation with lot of outb()/inb(). Signed-off-by: Ji-Ze Hong (Peter Hong) --- drivers/tty/serial/8250/8250_fintek.c | 73 ++- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index 0facc78..2ae846e 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -49,6 +49,27 @@ struct fintek_8250 { u8 key; }; +static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg) +{ + outb(reg, pdata->base_port + ADDR_PORT); + return inb(pdata->base_port + DATA_PORT); +} + +static void sio_write_reg(struct fintek_8250 *pdata, u8 reg, u8 data) +{ + outb(reg, pdata->base_port + ADDR_PORT); + outb(data, pdata->base_port + DATA_PORT); +} + +static void sio_write_mask_reg(struct fintek_8250 *pdata, u8 reg, u8 mask, + u8 data) +{ + u8 tmp; + + tmp = (sio_read_reg(pdata, reg) & ~mask) | (mask & data); + sio_write_reg(pdata, reg, tmp); +} + static int fintek_8250_enter_key(u16 base_port, u8 key) { if (!request_muxed_region(base_port, 2, "8250_fintek")) @@ -66,22 +87,18 @@ static void fintek_8250_exit_key(u16 base_port) release_region(base_port + ADDR_PORT, 2); } -static int fintek_8250_check_id(u16 base_port) +static int fintek_8250_check_id(struct fintek_8250 *pdata) { u16 chip; - outb(VENDOR_ID1, base_port + ADDR_PORT); - if (inb(base_port + DATA_PORT) != VENDOR_ID1_VAL) + if (sio_read_reg(pdata, VENDOR_ID1) != VENDOR_ID1_VAL) return -ENODEV; - outb(VENDOR_ID2, base_port + ADDR_PORT); - if (inb(base_port + DATA_PORT) != VENDOR_ID2_VAL) + if (sio_read_reg(pdata, VENDOR_ID2) != VENDOR_ID2_VAL) return -ENODEV; - outb(CHIP_ID1, base_port + ADDR_PORT); - chip = inb(base_port + DATA_PORT); - outb(CHIP_ID2, base_port + ADDR_PORT); - chip |= inb(base_port + DATA_PORT) << 8; + chip = sio_read_reg(pdata, CHIP_ID1); + chip |= sio_read_reg(pdata, CHIP_ID2) << 8; if (chip != CHIP_ID_0 && chip != CHIP_ID_1) return -ENODEV; @@ -128,10 +145,8 @@ static int fintek_8250_rs485_config(struct uart_port *port, if (fintek_8250_enter_key(pdata->base_port, pdata->key)) return -EBUSY; - outb(LDN, pdata->base_port + ADDR_PORT); - outb(pdata->index, pdata->base_port + DATA_PORT); - outb(RS485, pdata->base_port + ADDR_PORT); - outb(config, pdata->base_port + DATA_PORT); + sio_write_reg(pdata, LDN, pdata->index); + sio_write_reg(pdata, RS485, config); fintek_8250_exit_key(pdata->base_port); port->rs485 = *rs485; @@ -147,10 +162,12 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address) for (i = 0; i < ARRAY_SIZE(addr); i++) { for (j = 0; j < ARRAY_SIZE(keys); j++) { + pdata->base_port = addr[i]; + pdata->key = keys[j]; if (fintek_8250_enter_key(addr[i], keys[j])) continue; - if (fintek_8250_check_id(addr[i])) { + if (fintek_8250_check_id(pdata)) { fintek_8250_exit_key(addr[i]); continue; } @@ -158,19 +175,13 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address) for (k = 0; k < 4; k++) { u16 aux; - outb(LDN, addr[i] + ADDR_PORT); - outb(k, addr[i] + DATA_PORT); - - outb(IO_ADDR1, addr[i] + ADDR_PORT); - aux = inb(addr[i] + DATA_PORT); - outb(IO_ADDR2, addr[i] + ADDR_PORT); - aux |= inb(addr[i] + DATA_PORT) << 8; + sio_write_reg(pdata, LDN, k); + aux = sio_read_reg(pdata, IO_ADDR1); + aux |= sio_read_reg(pdata, IO_ADDR2) << 8; if (aux != io_address) continue; fintek_8250_exit_key(addr[i]); - pdata->key = keys[j]; - pdata->base_port = addr[i]; pdata->index = k; return 0; @@ -186,24 +197,16 @@ static int find_base_port(struct fintek_8250 *pdata,
[PATCH 2/7] serial: 8250_fintek: Set IRQ Mode when port probed
Set IRQ Mode when port probed in find_base_port() It should hold the IO port premission via fintek_8250_enter_key() and release via fintek_8250_exit_key() when we configure the SuperIO. This patch will move all SuperIO configure operations to find_base_port() to reduce fintek_8250_enter_key()/fintek_8250_exit_key() usage. Suggested-by: Ricardo Ribalda Delgado Signed-off-by: Ji-Ze Hong (Peter Hong) --- drivers/tty/serial/8250/8250_fintek.c | 36 ++- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index 2ae846e..cd8903f 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -49,6 +49,9 @@ struct fintek_8250 { u8 key; }; +static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, +bool level_mode); + static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg) { outb(reg, pdata->base_port + ADDR_PORT); @@ -154,10 +157,13 @@ static int fintek_8250_rs485_config(struct uart_port *port, return 0; } -static int find_base_port(struct fintek_8250 *pdata, u16 io_address) +static int find_base_port(struct fintek_8250 *pdata, u16 io_address, + unsigned int irq) { static const u16 addr[] = {0x4e, 0x2e}; static const u8 keys[] = {0x77, 0xa0, 0x87, 0x67}; + struct irq_data *irq_data; + bool level_mode = false; int i, j, k; for (i = 0; i < ARRAY_SIZE(addr); i++) { @@ -181,9 +187,16 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address) if (aux != io_address) continue; - fintek_8250_exit_key(addr[i]); pdata->index = k; + irq_data = irq_get_irq_data(irq); + if (irq_data) + level_mode = + irqd_is_level_type(irq_data); + + fintek_8250_set_irq_mode(pdata, level_mode); + fintek_8250_exit_key(addr[i]); + return 0; } @@ -194,31 +207,20 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address) return -ENODEV; } -static int fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool level_mode) +static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level) { - int status; - - status = fintek_8250_enter_key(pdata->base_port, pdata->key); - if (status) - return status; - sio_write_reg(pdata, LDN, pdata->index); sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE); sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK, - level_mode ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); - - fintek_8250_exit_key(pdata->base_port); - return 0; + is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); } int fintek_8250_probe(struct uart_8250_port *uart) { struct fintek_8250 *pdata; struct fintek_8250 probe_data; - struct irq_data *irq_data = irq_get_irq_data(uart->port.irq); - bool level_mode = irqd_is_level_type(irq_data); - if (find_base_port(_data, uart->port.iobase)) + if (find_base_port(_data, uart->port.iobase, uart->port.irq)) return -ENODEV; pdata = devm_kzalloc(uart->port.dev, sizeof(*pdata), GFP_KERNEL); @@ -229,5 +231,5 @@ int fintek_8250_probe(struct uart_8250_port *uart) uart->port.rs485_config = fintek_8250_rs485_config; uart->port.private_data = pdata; - return fintek_8250_set_irq_mode(pdata, level_mode); + return 0; } -- 1.9.1
[PATCH 4/7] serial: 8250_fintek: Rearrange function
We change the position of fintek_8250_set_irq_mode() above the find_base_port() to eliminate the prototype define. Signed-off-by: Ji-Ze Hong (Peter Hong) --- drivers/tty/serial/8250/8250_fintek.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index 5625203..921f742 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -56,9 +56,6 @@ struct fintek_8250 { u8 key; }; -static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, -bool level_mode); - static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg) { outb(reg, pdata->base_port + ADDR_PORT); @@ -179,6 +176,14 @@ static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata) } } +static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level) +{ + sio_write_reg(pdata, LDN, pdata->index); + sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE); + sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK, + is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); +} + static int find_base_port(struct fintek_8250 *pdata, u16 io_address, unsigned int irq) { @@ -230,14 +235,6 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address, return -ENODEV; } -static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level) -{ - sio_write_reg(pdata, LDN, pdata->index); - sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE); - sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK, - is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH); -} - int fintek_8250_probe(struct uart_8250_port *uart) { struct fintek_8250 *pdata; -- 1.9.1
[PATCH 5/7] serial: 8250_fintek: Add F81216 Support
Fintek F81216 is a LPC to 4 UARTs device. It's the F81216 series but support less functional than F81216AD/F81216H The following list is brief descriptions of F81216 series: F81216H (0105) 9Bit/High baud rate(not implements with mainline) RS485, 128Bytes FIFO (implemented) F81216AD (0216) 9Bit(not implements with mainline) RS485(implemented) F81216 (0208) basically 16550A Signed-off-by: Ji-Ze Hong (Peter Hong) --- drivers/tty/serial/8250/8250_fintek.c | 26 -- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index 921f742..a62cfdd 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -23,6 +23,7 @@ #define CHIP_ID2 0x21 #define CHIP_ID_F81216AD 0x1602 #define CHIP_ID_F81216H 0x0501 +#define CHIP_ID_F81216 0x0802 #define VENDOR_ID1 0x23 #define VENDOR_ID1_VAL 0x19 #define VENDOR_ID2 0x24 @@ -107,8 +108,14 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata) chip = sio_read_reg(pdata, CHIP_ID1); chip |= sio_read_reg(pdata, CHIP_ID2) << 8; - if (chip != CHIP_ID_F81216AD && chip != CHIP_ID_F81216H) + switch (chip) { + case CHIP_ID_F81216AD: + case CHIP_ID_F81216H: + case CHIP_ID_F81216: + break; + default: return -ENODEV; + } pdata->pid = chip; return 0; @@ -235,6 +242,21 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address, return -ENODEV; } +static void fintek_8250_set_rs485_handler(struct uart_8250_port *uart) +{ + struct fintek_8250 *pdata = uart->port.private_data; + + switch (pdata->pid) { + default: /* No RS485 Auto direction functional */ + break; + + case CHIP_ID_F81216AD: + case CHIP_ID_F81216H: + uart->port.rs485_config = fintek_8250_rs485_config; + break; + } +} + int fintek_8250_probe(struct uart_8250_port *uart) { struct fintek_8250 *pdata; @@ -248,8 +270,8 @@ int fintek_8250_probe(struct uart_8250_port *uart) return -ENOMEM; memcpy(pdata, _data, sizeof(probe_data)); - uart->port.rs485_config = fintek_8250_rs485_config; uart->port.private_data = pdata; + fintek_8250_set_rs485_handler(uart); return 0; } -- 1.9.1
Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support
Hi Guenter, Thanks for your review, and I think it still not too late for nitpicking as it isn't merged to next branch. :) We have amend the code a bit, so probably we fixed some of the minor issues against V10. But some of them are really personal taste, if you still insist on that, I will be ok to modify them. I will push patch to fix them after your feedback. :) On 2016/9/1 2:17, Guenter Roeck wrote: On Fri, Aug 19, 2016 at 09:34:58AM +0800, Shawn Lin wrote: [...] + rockchip_pcie_enable_interrupts(port); + There is no matching disable_interrupts call in error handling after this. Is that ok ? From test, probably ok since the genpd will be off and all of them wil be cleared. Also, is it ok to enable interrupts this early (before the rest of the initialization is complete) ? The training starts, so we some client irq should be handled now, and we already register isr. + err = rockchip_pcie_init_irq_domain(port); + if (err < 0) + goto err_vpcie; + + err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff, + , _base); + if (err) + goto err_vpcie; + + err = devm_request_pci_bus_resources(dev, ); + if (err) + goto err_vpcie; + + /* Get the I/O and memory ranges from DT */ + resource_list_for_each_entry(win, ) { + switch (resource_type(win->res)) { + case IORESOURCE_IO: + io = win->res; + io->name = "I/O"; + io_size = resource_size(io); + io_bus_addr = io->start - win->offset; + err = pci_remap_iospace(io, io_base); + if (err) { + dev_warn(port->dev, "error %d: failed to map resource %pR\n", +err, io); + continue; + } + break; + case IORESOURCE_MEM: + mem = win->res; + mem->name = "MEM"; + mem_size = resource_size(mem); + mem_bus_addr = mem->start - win->offset; + break; + case IORESOURCE_BUS: + busn = win->res; + break; + default: + continue; + } + } + + if (mem_size) While strictly speaking not needed, I would recommend { } here to improve readability. + for (reg_no = 0; reg_no < (mem_size >> 20); reg_no++) { This doesn't support mem sizes smaller than 1 << 20 (and clips the size if it is not aligned to 1M). Is this intentional ? yes, we don't support. + err = rockchip_pcie_prog_ob_atu(port, reg_no + 1, + AXI_WRAPPER_MEM_WRITE, + 20 - 1, + mem_bus_addr + + (reg_no << 20), + 0); + if (err) { + dev_err(dev, "program RC mem outbound ATU failed\n"); + goto err_vpcie; + } + } + err = rockchip_pcie_prog_ob_atu(port, + reg_no + 1 + offset, + AXI_WRAPPER_IO_WRITE, + 20 - 1, + io_bus_addr + + (reg_no << 20), + 0); + if (err) { + dev_err(dev, "program RC io outbound ATU failed\n"); + goto err_vpcie; + } + } + + pci_bus_add_devices(bus); + + dev_warn(dev, "only 32-bit config accesses supported; smaller writes may corrupt adjacent RW1C fields\n"); + A warning which is always displayed seems to be a bit useless. If this is a concern, maybe the driver should provide its own config space access functions and map smaller accesses into 32 bit accesses. But isn't that already done ? No, that is just for normal code path. When users comfigure it via some user-space tool, it is sane to make them know this limitation. So we was suggested to do this. + return err; + +err_vpcie: + if (!IS_ERR(port->vpcie3v3)) + regulator_disable(port->vpcie3v3); + if (!IS_ERR(port->vpcie1v8)) + regulator_disable(port->vpcie1v8); + if (!IS_ERR(port->vpcie0v9)) +
[PATCH 3/7] serial: 8250_fintek: Set maximum FIFO of F81216H
The Fintek F81216H had maximum 128Bytes FIFO, but some BIOS configurated as normal 16Bytes FIFO. This patch will set 128Bytes FIFO and trigger level multiplier as 4x when F81216H detected. Default 16550A trigger level is 8Bytes. When this patch applied, the trigger level will change to 8Byte x 4 = 32Byte. It can be reduce the RX incoming interrupts. Signed-off-by: Ji-Ze Hong (Peter Hong)--- drivers/tty/serial/8250/8250_fintek.c | 29 ++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index cd8903f..5625203 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -21,8 +21,8 @@ #define EXIT_KEY 0xAA #define CHIP_ID1 0x20 #define CHIP_ID2 0x21 -#define CHIP_ID_0 0x1602 -#define CHIP_ID_1 0x0501 +#define CHIP_ID_F81216AD 0x1602 +#define CHIP_ID_F81216H 0x0501 #define VENDOR_ID1 0x23 #define VENDOR_ID1_VAL 0x19 #define VENDOR_ID2 0x24 @@ -43,7 +43,14 @@ #define RXW4C_IRA BIT(3) #define TXW4C_IRA BIT(2) +#define FIFO_CTRL 0xF6 +#define FIFO_MODE_MASK (BIT(1) | BIT(0)) +#define FIFO_MODE_128 (BIT(1) | BIT(0)) +#define RXFTHR_MODE_MASK (BIT(5) | BIT(4)) +#define RXFTHR_MODE_4X BIT(5) + struct fintek_8250 { + u16 pid; u16 base_port; u8 index; u8 key; @@ -103,9 +110,10 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata) chip = sio_read_reg(pdata, CHIP_ID1); chip |= sio_read_reg(pdata, CHIP_ID2) << 8; - if (chip != CHIP_ID_0 && chip != CHIP_ID_1) + if (chip != CHIP_ID_F81216AD && chip != CHIP_ID_F81216H) return -ENODEV; + pdata->pid = chip; return 0; } @@ -157,6 +165,20 @@ static int fintek_8250_rs485_config(struct uart_port *port, return 0; } +static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata) +{ + switch (pdata->pid) { + default: /* Default 16Bytes FIFO */ + return; + + case CHIP_ID_F81216H: /* 128Bytes FIFO */ + sio_write_mask_reg(pdata, FIFO_CTRL, + FIFO_MODE_MASK | RXFTHR_MODE_MASK, + FIFO_MODE_128 | RXFTHR_MODE_4X); + break; + } +} + static int find_base_port(struct fintek_8250 *pdata, u16 io_address, unsigned int irq) { @@ -195,6 +217,7 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address, irqd_is_level_type(irq_data); fintek_8250_set_irq_mode(pdata, level_mode); + fintek_8250_set_max_fifo(pdata); fintek_8250_exit_key(addr[i]); return 0; -- 1.9.1
[PATCH 0/7] serial: 8250_fintek: Fix the IRQ mode and code refactoring
The following patches will fix the Fintek LPC to UARTs IRQ mode mismatch issue, code refactoring and this series patches should follow by patch rename IRQ_MODE macro on 'commit 87a713c8ffca ("8250/fintek: rename IRQ_MODE macro")'. with tty-linus branch. Some BIOS only use _OSI("Linux") to distinguish between Linux & Windows. Apply Level/Low to UART trigger mode if Windows, Edge/High otherwise. But since 2.6.23 the mainline kernel no longer returns true for _OSI(“Linux”). The BIOS ASL should avoid to use _OSI() to check system type and use ACPI MADT override IRQ mode instead. We'll try to refactoring the source code more readable with SuperIO read/write register functions. Ricardo Ribalda Delgado suggest fixing a potential NULL pointer dereference with applied patch fix the mismatched IRQ mode on 'commit 4da22f1418cb ("serial: 8250_fintek: fix the mismatched IRQ mode")'. had been implemented with 8250_fintek: Set IRQ Mode when port probed. Ji-Ze Hong (Peter Hong) (7): serial: 8250_fintek: Refactoring read/write method serial: 8250_fintek: Set IRQ Mode when port probed serial: 8250_fintek: Set maximum FIFO of F81216H serial: 8250_fintek: Rearrange function serial: 8250_fintek: Add F81216 Support serial: 8250_fintek: Add F81866 Support serial: 8250_fintek: Add F81865 Support drivers/tty/serial/8250/8250_fintek.c | 233 +- 1 file changed, 175 insertions(+), 58 deletions(-) -- 1.9.1
Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support
Hi Guenter, Thanks for your review, and I think it still not too late for nitpicking as it isn't merged to next branch. :) We have amend the code a bit, so probably we fixed some of the minor issues against V10. But some of them are really personal taste, if you still insist on that, I will be ok to modify them. I will push patch to fix them after your feedback. :) On 2016/9/1 2:17, Guenter Roeck wrote: On Fri, Aug 19, 2016 at 09:34:58AM +0800, Shawn Lin wrote: [...] + rockchip_pcie_enable_interrupts(port); + There is no matching disable_interrupts call in error handling after this. Is that ok ? From test, probably ok since the genpd will be off and all of them wil be cleared. Also, is it ok to enable interrupts this early (before the rest of the initialization is complete) ? The training starts, so we some client irq should be handled now, and we already register isr. + err = rockchip_pcie_init_irq_domain(port); + if (err < 0) + goto err_vpcie; + + err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff, + , _base); + if (err) + goto err_vpcie; + + err = devm_request_pci_bus_resources(dev, ); + if (err) + goto err_vpcie; + + /* Get the I/O and memory ranges from DT */ + resource_list_for_each_entry(win, ) { + switch (resource_type(win->res)) { + case IORESOURCE_IO: + io = win->res; + io->name = "I/O"; + io_size = resource_size(io); + io_bus_addr = io->start - win->offset; + err = pci_remap_iospace(io, io_base); + if (err) { + dev_warn(port->dev, "error %d: failed to map resource %pR\n", +err, io); + continue; + } + break; + case IORESOURCE_MEM: + mem = win->res; + mem->name = "MEM"; + mem_size = resource_size(mem); + mem_bus_addr = mem->start - win->offset; + break; + case IORESOURCE_BUS: + busn = win->res; + break; + default: + continue; + } + } + + if (mem_size) While strictly speaking not needed, I would recommend { } here to improve readability. + for (reg_no = 0; reg_no < (mem_size >> 20); reg_no++) { This doesn't support mem sizes smaller than 1 << 20 (and clips the size if it is not aligned to 1M). Is this intentional ? yes, we don't support. + err = rockchip_pcie_prog_ob_atu(port, reg_no + 1, + AXI_WRAPPER_MEM_WRITE, + 20 - 1, + mem_bus_addr + + (reg_no << 20), + 0); + if (err) { + dev_err(dev, "program RC mem outbound ATU failed\n"); + goto err_vpcie; + } + } + err = rockchip_pcie_prog_ob_atu(port, + reg_no + 1 + offset, + AXI_WRAPPER_IO_WRITE, + 20 - 1, + io_bus_addr + + (reg_no << 20), + 0); + if (err) { + dev_err(dev, "program RC io outbound ATU failed\n"); + goto err_vpcie; + } + } + + pci_bus_add_devices(bus); + + dev_warn(dev, "only 32-bit config accesses supported; smaller writes may corrupt adjacent RW1C fields\n"); + A warning which is always displayed seems to be a bit useless. If this is a concern, maybe the driver should provide its own config space access functions and map smaller accesses into 32 bit accesses. But isn't that already done ? No, that is just for normal code path. When users comfigure it via some user-space tool, it is sane to make them know this limitation. So we was suggested to do this. + return err; + +err_vpcie: + if (!IS_ERR(port->vpcie3v3)) + regulator_disable(port->vpcie3v3); + if (!IS_ERR(port->vpcie1v8)) + regulator_disable(port->vpcie1v8); + if (!IS_ERR(port->vpcie0v9)) +
[PATCH 3/7] serial: 8250_fintek: Set maximum FIFO of F81216H
The Fintek F81216H had maximum 128Bytes FIFO, but some BIOS configurated as normal 16Bytes FIFO. This patch will set 128Bytes FIFO and trigger level multiplier as 4x when F81216H detected. Default 16550A trigger level is 8Bytes. When this patch applied, the trigger level will change to 8Byte x 4 = 32Byte. It can be reduce the RX incoming interrupts. Signed-off-by: Ji-Ze Hong (Peter Hong) --- drivers/tty/serial/8250/8250_fintek.c | 29 ++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c index cd8903f..5625203 100644 --- a/drivers/tty/serial/8250/8250_fintek.c +++ b/drivers/tty/serial/8250/8250_fintek.c @@ -21,8 +21,8 @@ #define EXIT_KEY 0xAA #define CHIP_ID1 0x20 #define CHIP_ID2 0x21 -#define CHIP_ID_0 0x1602 -#define CHIP_ID_1 0x0501 +#define CHIP_ID_F81216AD 0x1602 +#define CHIP_ID_F81216H 0x0501 #define VENDOR_ID1 0x23 #define VENDOR_ID1_VAL 0x19 #define VENDOR_ID2 0x24 @@ -43,7 +43,14 @@ #define RXW4C_IRA BIT(3) #define TXW4C_IRA BIT(2) +#define FIFO_CTRL 0xF6 +#define FIFO_MODE_MASK (BIT(1) | BIT(0)) +#define FIFO_MODE_128 (BIT(1) | BIT(0)) +#define RXFTHR_MODE_MASK (BIT(5) | BIT(4)) +#define RXFTHR_MODE_4X BIT(5) + struct fintek_8250 { + u16 pid; u16 base_port; u8 index; u8 key; @@ -103,9 +110,10 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata) chip = sio_read_reg(pdata, CHIP_ID1); chip |= sio_read_reg(pdata, CHIP_ID2) << 8; - if (chip != CHIP_ID_0 && chip != CHIP_ID_1) + if (chip != CHIP_ID_F81216AD && chip != CHIP_ID_F81216H) return -ENODEV; + pdata->pid = chip; return 0; } @@ -157,6 +165,20 @@ static int fintek_8250_rs485_config(struct uart_port *port, return 0; } +static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata) +{ + switch (pdata->pid) { + default: /* Default 16Bytes FIFO */ + return; + + case CHIP_ID_F81216H: /* 128Bytes FIFO */ + sio_write_mask_reg(pdata, FIFO_CTRL, + FIFO_MODE_MASK | RXFTHR_MODE_MASK, + FIFO_MODE_128 | RXFTHR_MODE_4X); + break; + } +} + static int find_base_port(struct fintek_8250 *pdata, u16 io_address, unsigned int irq) { @@ -195,6 +217,7 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address, irqd_is_level_type(irq_data); fintek_8250_set_irq_mode(pdata, level_mode); + fintek_8250_set_max_fifo(pdata); fintek_8250_exit_key(addr[i]); return 0; -- 1.9.1
[PATCH 0/7] serial: 8250_fintek: Fix the IRQ mode and code refactoring
The following patches will fix the Fintek LPC to UARTs IRQ mode mismatch issue, code refactoring and this series patches should follow by patch rename IRQ_MODE macro on 'commit 87a713c8ffca ("8250/fintek: rename IRQ_MODE macro")'. with tty-linus branch. Some BIOS only use _OSI("Linux") to distinguish between Linux & Windows. Apply Level/Low to UART trigger mode if Windows, Edge/High otherwise. But since 2.6.23 the mainline kernel no longer returns true for _OSI(“Linux”). The BIOS ASL should avoid to use _OSI() to check system type and use ACPI MADT override IRQ mode instead. We'll try to refactoring the source code more readable with SuperIO read/write register functions. Ricardo Ribalda Delgado suggest fixing a potential NULL pointer dereference with applied patch fix the mismatched IRQ mode on 'commit 4da22f1418cb ("serial: 8250_fintek: fix the mismatched IRQ mode")'. had been implemented with 8250_fintek: Set IRQ Mode when port probed. Ji-Ze Hong (Peter Hong) (7): serial: 8250_fintek: Refactoring read/write method serial: 8250_fintek: Set IRQ Mode when port probed serial: 8250_fintek: Set maximum FIFO of F81216H serial: 8250_fintek: Rearrange function serial: 8250_fintek: Add F81216 Support serial: 8250_fintek: Add F81866 Support serial: 8250_fintek: Add F81865 Support drivers/tty/serial/8250/8250_fintek.c | 233 +- 1 file changed, 175 insertions(+), 58 deletions(-) -- 1.9.1
Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu
>> @@ -376,6 +377,8 @@ int register_cpu(struct cpu *cpu, int num) >> >> per_cpu(cpu_sys_devices, num) = >dev; >> register_cpu_under_node(num, cpu_to_node(num)); >> +if (dev_pm_qos_expose_latency_limit(>dev, 0)) >> +pr_debug("CPU%d: add resume latency failed\n", num); > > Why is this a debug message? And you have a struct device, why not use > it? > Looks no one care about the dev_pm_qos_expose_latency_limit's return value, so this debug message like gilding the lily. will remove it. > Also, what userspace changes does this require, or affect? Any new > sysfs files? User can set values on each of cpu, like limit 100ms on cpu0, that means the cpu0 response time should be in 100ms in possible idle. It similar with DMA_LATENCY, but that request is for all cpu. This is just for particular cpu, like a interrupt pined CPU. echo 100 > /sys/devices/system/cpu/cpu0/power/pm_qos_resume_latency_us > > thanks, > > greg k-h >
Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu
>> @@ -376,6 +377,8 @@ int register_cpu(struct cpu *cpu, int num) >> >> per_cpu(cpu_sys_devices, num) = >dev; >> register_cpu_under_node(num, cpu_to_node(num)); >> +if (dev_pm_qos_expose_latency_limit(>dev, 0)) >> +pr_debug("CPU%d: add resume latency failed\n", num); > > Why is this a debug message? And you have a struct device, why not use > it? > Looks no one care about the dev_pm_qos_expose_latency_limit's return value, so this debug message like gilding the lily. will remove it. > Also, what userspace changes does this require, or affect? Any new > sysfs files? User can set values on each of cpu, like limit 100ms on cpu0, that means the cpu0 response time should be in 100ms in possible idle. It similar with DMA_LATENCY, but that request is for all cpu. This is just for particular cpu, like a interrupt pined CPU. echo 100 > /sys/devices/system/cpu/cpu0/power/pm_qos_resume_latency_us > > thanks, > > greg k-h >
Re: [RFC PATCH V2 3/3] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers
在 2016/9/1 6:56, Rafael J. Wysocki 写道: On Wednesday, August 31, 2016 07:48:14 PM Dongdong Liu wrote: Add specific quirks for PCI config space accessors.This involves: 1. New initialization call hisi_pcie_acpi_init() to get RC config resource with hardcoded range address and setup ecam mapping. 2. New entry in common quirk array. Signed-off-by: Dongdong LiuSigned-off-by: Gabriele Paoloni Well, what exactly is the ACPI support you're adding? Is it the ECAM part only or is there anything more to it? Hi Rafael, thanks for replying. Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access. In our case we cannot use the standard MCFG object to pass the RC itself config space addresses. The more discuss information can be found: https://lkml.org/lkml/2016/2/22/1087 [...] I have looked into this and in our case we cannot use the standard MCFG object to pass the RC config space addresses. The reason is that in our HW we have the config base addresses of the root complex ports that are less than 0x10 byte distant one from the other as we only map the first 0x1 bytes. Now the MCFG acpi framework always fix the MCFG resource size to 0x10 for each bus; therefore if we pass our RC addresses through MCFG we end up with a resource conflict. To give you a practical example we are in a situation where we have: port0: [0xb008 - 0xb008 + 0x1] port1: [0xb009 - 0xb009 + 0x1] port2: [0xb00A - 0xb00A + 0x1] port3: [0xb00B - 0xb00B + 0x1] So if we pass the base addresses through MCFG the resources will overlap as MCFG will consider 0x10 size for each base address of the root complex (only the RC bus uses that address) So far I do not see many option other than using _DSD to pass these RC config base addresses. Thanks and Regards Gab [...] and https://patchwork.kernel.org/patch/9178791/ [...] Furthermore, I suspect we do not even need a way to pass the non-ECAM compliant config space resources to the OS (ie we can't change FW anymore anyway in some platforms) so the quirks hooks are likely to hardcode the required config space addresses for the respective MCFG match. .. Thanks ! Lorenzo [...] So we hard code with our RC itself config resource. Thanks Dongdong Thanks, Rafael .
Re: [RFC PATCH V2 3/3] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers
在 2016/9/1 6:56, Rafael J. Wysocki 写道: On Wednesday, August 31, 2016 07:48:14 PM Dongdong Liu wrote: Add specific quirks for PCI config space accessors.This involves: 1. New initialization call hisi_pcie_acpi_init() to get RC config resource with hardcoded range address and setup ecam mapping. 2. New entry in common quirk array. Signed-off-by: Dongdong Liu Signed-off-by: Gabriele Paoloni Well, what exactly is the ACPI support you're adding? Is it the ECAM part only or is there anything more to it? Hi Rafael, thanks for replying. Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access. In our case we cannot use the standard MCFG object to pass the RC itself config space addresses. The more discuss information can be found: https://lkml.org/lkml/2016/2/22/1087 [...] I have looked into this and in our case we cannot use the standard MCFG object to pass the RC config space addresses. The reason is that in our HW we have the config base addresses of the root complex ports that are less than 0x10 byte distant one from the other as we only map the first 0x1 bytes. Now the MCFG acpi framework always fix the MCFG resource size to 0x10 for each bus; therefore if we pass our RC addresses through MCFG we end up with a resource conflict. To give you a practical example we are in a situation where we have: port0: [0xb008 - 0xb008 + 0x1] port1: [0xb009 - 0xb009 + 0x1] port2: [0xb00A - 0xb00A + 0x1] port3: [0xb00B - 0xb00B + 0x1] So if we pass the base addresses through MCFG the resources will overlap as MCFG will consider 0x10 size for each base address of the root complex (only the RC bus uses that address) So far I do not see many option other than using _DSD to pass these RC config base addresses. Thanks and Regards Gab [...] and https://patchwork.kernel.org/patch/9178791/ [...] Furthermore, I suspect we do not even need a way to pass the non-ECAM compliant config space resources to the OS (ie we can't change FW anymore anyway in some platforms) so the quirks hooks are likely to hardcode the required config space addresses for the respective MCFG match. .. Thanks ! Lorenzo [...] So we hard code with our RC itself config resource. Thanks Dongdong Thanks, Rafael .
Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
在 2016/9/1 10:29, Ziyuan Xu 写道: Hi, On 2016年09月01日 01:42, Doug Anderson wrote: Hi, On Sun, Aug 28, 2016 at 8:25 PM, Shawn Linwrote: On 2016/8/29 10:50, Elaine Zhang wrote: On 08/27/2016 11:05 PM, Shawn Lin wrote: On 2016/8/27 21:41, Ziyuan Xu wrote: Control power domain for eMMC via genpd to reduce power consumption. Signed-off-by: Elaine Zhang Signed-off-by: Ziyuan Xu It looks nice to me. But this should be merged after applying that[0] as your patch will break bind/unbind test for sdhci-of-arasan on rk3399 without it[0]. Moreover, Elaine should make sure that upstreamed rockchip power domain stuff would not off pd for emmc, *otherwise*, I should update my patch to make sure we update clkmul every time when doing suspend 2 resume.. Forgot to say: If use pd, Although there is no call to power odd the pd_emmc, it will be power off when the system doing suspend 2 resume. (Because the system call __device_suspend_noirq->pm_genpd_suspend_noirq->rockchip_pd_power_off) Thanks for explaining this. I checked the code a bit and actually I don't need to updata clkmul since it was recorded, although it is still reset to 0x10 reading from syscon. So for that, we can now pick it up without waiting for my sdhci-of-arasan's update. Reviewed-by: Shawn Lin This is fine to pick up _only_ if you don't care about suspend/resume. If you care about suspend/resume then someone needs to first write a patch that will re-init all "corecfg" values after power is turned on. Do you mean corecfg_clockmultiplier and corecfg_baseclkfreq, if yes, we don't need to strore/re-init it after resume. corecfg_clockmultiplier is only used to fetch host->clk_mul, and host->clk_mul has been a fixed value at run-time, unless driver unbind. The same as corecfg_clockmultiplier, corecfg_baseclkfreq is used to check the xin_clk at probe time, we don't reference it at run-time. correct. That is why we don't need to update it even we power off pd. BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works fine. Technically I think this should probably use "pm runtime" and not normal suspend/resume hooks. Any time we end up pm runtime suspended then I think our power will go off (because of genpd?) and we need to restore values. I understand your consideration. BUT genpd is in charge of on/off pd if the corresponding device node has "power-domains" property. RPM is unnecessary for this situation, we will not use autosuspend, right? That is irrelevant to rpm as what we need is just to control genpd here. If we need to support rmp, we need some extra patches to do it, including genpd off, clk-disable, etc... @shawn, what's your opinion? I'm not sure if this should be done in a generic way where we try to save and restore all values in the "sdhci_arasan_soc_ctl_map" or if we should try to be smarter... I am prone to keep it as-is, unless we see a stronge requirement for coming users who argue that they have some extra corecfg_* need to recovery due to whatever reason.:) If we see something like this: if(of_device_is_compatible(A)) update X else if (of_device_is_compatible(B)) update Y . Then we could consider trying to save & restrore all the values. -Doug ___ Linux-rockchip mailing list linux-rockc...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip -- Best Regards Shawn Lin
Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
在 2016/9/1 10:29, Ziyuan Xu 写道: Hi, On 2016年09月01日 01:42, Doug Anderson wrote: Hi, On Sun, Aug 28, 2016 at 8:25 PM, Shawn Lin wrote: On 2016/8/29 10:50, Elaine Zhang wrote: On 08/27/2016 11:05 PM, Shawn Lin wrote: On 2016/8/27 21:41, Ziyuan Xu wrote: Control power domain for eMMC via genpd to reduce power consumption. Signed-off-by: Elaine Zhang Signed-off-by: Ziyuan Xu It looks nice to me. But this should be merged after applying that[0] as your patch will break bind/unbind test for sdhci-of-arasan on rk3399 without it[0]. Moreover, Elaine should make sure that upstreamed rockchip power domain stuff would not off pd for emmc, *otherwise*, I should update my patch to make sure we update clkmul every time when doing suspend 2 resume.. Forgot to say: If use pd, Although there is no call to power odd the pd_emmc, it will be power off when the system doing suspend 2 resume. (Because the system call __device_suspend_noirq->pm_genpd_suspend_noirq->rockchip_pd_power_off) Thanks for explaining this. I checked the code a bit and actually I don't need to updata clkmul since it was recorded, although it is still reset to 0x10 reading from syscon. So for that, we can now pick it up without waiting for my sdhci-of-arasan's update. Reviewed-by: Shawn Lin This is fine to pick up _only_ if you don't care about suspend/resume. If you care about suspend/resume then someone needs to first write a patch that will re-init all "corecfg" values after power is turned on. Do you mean corecfg_clockmultiplier and corecfg_baseclkfreq, if yes, we don't need to strore/re-init it after resume. corecfg_clockmultiplier is only used to fetch host->clk_mul, and host->clk_mul has been a fixed value at run-time, unless driver unbind. The same as corecfg_clockmultiplier, corecfg_baseclkfreq is used to check the xin_clk at probe time, we don't reference it at run-time. correct. That is why we don't need to update it even we power off pd. BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works fine. Technically I think this should probably use "pm runtime" and not normal suspend/resume hooks. Any time we end up pm runtime suspended then I think our power will go off (because of genpd?) and we need to restore values. I understand your consideration. BUT genpd is in charge of on/off pd if the corresponding device node has "power-domains" property. RPM is unnecessary for this situation, we will not use autosuspend, right? That is irrelevant to rpm as what we need is just to control genpd here. If we need to support rmp, we need some extra patches to do it, including genpd off, clk-disable, etc... @shawn, what's your opinion? I'm not sure if this should be done in a generic way where we try to save and restore all values in the "sdhci_arasan_soc_ctl_map" or if we should try to be smarter... I am prone to keep it as-is, unless we see a stronge requirement for coming users who argue that they have some extra corecfg_* need to recovery due to whatever reason.:) If we see something like this: if(of_device_is_compatible(A)) update X else if (of_device_is_compatible(B)) update Y . Then we could consider trying to save & restrore all the values. -Doug ___ Linux-rockchip mailing list linux-rockc...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip -- Best Regards Shawn Lin
Re: [PATCH 00/13] Add support for perf_arch_regs
On Tuesday 30 August 2016 09:31 PM, Nilay Vaish wrote: On 28 August 2016 at 16:00, Madhavan Srinivasanwrote: Patchset to extend PERF_SAMPLE_REGS_INTR to include platform specific PMU registers. Patchset applies cleanly on tip:perf/core branch It's a perennial request from hardware folks to be able to see the raw values of the pmu registers. Partly it's so that they can verify perf is doing what they want, and some of it is that they're interested in some of the more obscure info that isn't plumbed out through other perf interfaces. Over the years internally we have used various hack to get the requested data out but this is an attempt to use a somewhat standard mechanism (using PERF_SAMPLE_REGS_INTR). This would also be helpful for those of us working on the perf hardware backends, to be able to verify that we're programming things correctly, without resorting to debug printks etc. Mechanism proposed: 1)perf_regs structure is extended with a perf_arch_regs structure which each arch/ can populate with their specific platform registers to sample on each perf interrupt and an arch_regs_mask variable, which is for perf tool to know about the perf_arch_regs that are supported. 2)perf/core func perf_sample_regs_intr() extended to update the perf_arch_regs structure and the perf_arch_reg_mask. Set of new support functions added perf_get_arch_regs_mask() and perf_get_arch_reg() to aid the updates from arch/ side. 3) perf/core funcs perf_prepare_sample() and perf_output_sample() are extended to support the update for the perf_arch_regs_mask and perf_arch_regs in the sample 4)perf/core func perf_output_sample_regs() extended to dump the arch_regs to the output sample. 5)Finally, perf tool side is updated to include a new element "arch_regs_mask" in the "struct regs_dump", event sample funcs and print functions are updated to support perf_arch_regs. I read the patch series and I have one suggestion to make. I think we should not use 'arch regs' to refer to these pmu registers. I think Reason is that they are arch specific pmu regs. But I guess we can go with pmu_regs also. And having a "pregs" as option to list in -I? will be fine? (patch 13 in the patch series) Maddy architectural registers typically refer to the ones that hold the state of the process. Can we replace arch_regs by pmu_regs, or some other choice? Thanks Nilay
Re: [PATCH 00/13] Add support for perf_arch_regs
On Tuesday 30 August 2016 09:31 PM, Nilay Vaish wrote: On 28 August 2016 at 16:00, Madhavan Srinivasan wrote: Patchset to extend PERF_SAMPLE_REGS_INTR to include platform specific PMU registers. Patchset applies cleanly on tip:perf/core branch It's a perennial request from hardware folks to be able to see the raw values of the pmu registers. Partly it's so that they can verify perf is doing what they want, and some of it is that they're interested in some of the more obscure info that isn't plumbed out through other perf interfaces. Over the years internally we have used various hack to get the requested data out but this is an attempt to use a somewhat standard mechanism (using PERF_SAMPLE_REGS_INTR). This would also be helpful for those of us working on the perf hardware backends, to be able to verify that we're programming things correctly, without resorting to debug printks etc. Mechanism proposed: 1)perf_regs structure is extended with a perf_arch_regs structure which each arch/ can populate with their specific platform registers to sample on each perf interrupt and an arch_regs_mask variable, which is for perf tool to know about the perf_arch_regs that are supported. 2)perf/core func perf_sample_regs_intr() extended to update the perf_arch_regs structure and the perf_arch_reg_mask. Set of new support functions added perf_get_arch_regs_mask() and perf_get_arch_reg() to aid the updates from arch/ side. 3) perf/core funcs perf_prepare_sample() and perf_output_sample() are extended to support the update for the perf_arch_regs_mask and perf_arch_regs in the sample 4)perf/core func perf_output_sample_regs() extended to dump the arch_regs to the output sample. 5)Finally, perf tool side is updated to include a new element "arch_regs_mask" in the "struct regs_dump", event sample funcs and print functions are updated to support perf_arch_regs. I read the patch series and I have one suggestion to make. I think we should not use 'arch regs' to refer to these pmu registers. I think Reason is that they are arch specific pmu regs. But I guess we can go with pmu_regs also. And having a "pregs" as option to list in -I? will be fine? (patch 13 in the patch series) Maddy architectural registers typically refer to the ones that hold the state of the process. Can we replace arch_regs by pmu_regs, or some other choice? Thanks Nilay
○Hello
Hello This is very surprising, Bike, brand, guitar, camera, TV, telephone 50% discount and free shipping www .slooone .com
○Hello
Hello This is very surprising, Bike, brand, guitar, camera, TV, telephone 50% discount and free shipping www .slooone .com
Re: [PATCH v2 2/7] dts: sun8i-h3: add pinmux definitions for i2c0/i2c1
On Thu, Sep 1, 2016 at 3:30 AM,wrote: > From: Jorik Jonker > > This adds proper pinmux definitions for i2c0 and i2c1. Although H3 has a third > i2c controller, these are not exposed on my boards. If someone actually has a > H3 board with an exposed i2c2, they could add the third. > > Signed-off-by: Jorik Jonker > --- > arch/arm/boot/dts/sun8i-h3.dtsi | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi > index 7740748..0637b95 100644 > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -327,6 +327,20 @@ > interrupt-controller; > #interrupt-cells = <3>; > > + i2c0_pins_a: i2c0@0 { > + allwinner,pins = "PA11", "PA12"; > + allwinner,function = "i2c0"; > + allwinner,drive = ; > + allwinner,pull = ; > + }; > + > + i2c1_pins_a: i2c1@0 { These pinmuxes are the only ones possible for each peripheral. Please drop the _a suffix and the @0 address for both of them. ChenYu > + allwinner,pins = "PA18", "PA19"; > + allwinner,function = "i2c1"; > + allwinner,drive = ; > + allwinner,pull = ; > + }; > + > mmc0_pins_a: mmc0@0 { > allwinner,pins = "PF0", "PF1", "PF2", "PF3", > "PF4", "PF5"; > -- > 2.7.4 >
Re: [PATCH v2 2/7] dts: sun8i-h3: add pinmux definitions for i2c0/i2c1
On Thu, Sep 1, 2016 at 3:30 AM, wrote: > From: Jorik Jonker > > This adds proper pinmux definitions for i2c0 and i2c1. Although H3 has a third > i2c controller, these are not exposed on my boards. If someone actually has a > H3 board with an exposed i2c2, they could add the third. > > Signed-off-by: Jorik Jonker > --- > arch/arm/boot/dts/sun8i-h3.dtsi | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi > index 7740748..0637b95 100644 > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -327,6 +327,20 @@ > interrupt-controller; > #interrupt-cells = <3>; > > + i2c0_pins_a: i2c0@0 { > + allwinner,pins = "PA11", "PA12"; > + allwinner,function = "i2c0"; > + allwinner,drive = ; > + allwinner,pull = ; > + }; > + > + i2c1_pins_a: i2c1@0 { These pinmuxes are the only ones possible for each peripheral. Please drop the _a suffix and the @0 address for both of them. ChenYu > + allwinner,pins = "PA18", "PA19"; > + allwinner,function = "i2c1"; > + allwinner,drive = ; > + allwinner,pull = ; > + }; > + > mmc0_pins_a: mmc0@0 { > allwinner,pins = "PF0", "PF1", "PF2", "PF3", > "PF4", "PF5"; > -- > 2.7.4 >
[PATCH] ftrace: Handle TRACE_BPUTS in print_graph_comment
It missed to handle TRACE_BPUTS so messages recorded by trace_bputs() will be shown with symbol info unnecessarily. You can see it with the trace_printk sample code: # cd /sys/kernel/tracing/ # echo sys_sync > set_graph_function # echo 1 > options/sym-offset # echo function_graph > current_tracer Note that the sys_sync filter was there to prevent recording other functions and the sym-offset option was needed since the first message was called from a module init function so kallsyms doesn't have the symbol and omitted in the output. # cd ~/build/kernel # insmod samples/trace_printk/trace-printk.ko # cd - # head trace Before: # tracer: function_graph # # CPU DURATION FUNCTION CALLS # | | | | | | | 1) | /* 0xa0002000: This is a static string that will use trace_bputs */ 1) | /* This is a dynamic string that will use trace_puts */ 1) | /* trace_printk_irq_work+0x5/0x7b [trace_printk]: (irq) This is a static string that will use trace_bputs */ 1) | /* (irq) This is a dynamic string that will use trace_puts */ 1) | /* (irq) This is a static string that will use trace_bprintk() */ 1) | /* (irq) This is a dynamic string that will use trace_printk */ After: # tracer: function_graph # # CPU DURATION FUNCTION CALLS # | | | | | | | 1) | /* This is a static string that will use trace_bputs */ 1) | /* This is a dynamic string that will use trace_puts */ 1) | /* (irq) This is a static string that will use trace_bputs */ 1) | /* (irq) This is a dynamic string that will use trace_puts */ 1) | /* (irq) This is a static string that will use trace_bprintk() */ 1) | /* (irq) This is a dynamic string that will use trace_printk */ Signed-off-by: Namhyung Kim--- kernel/trace/trace_functions_graph.c | 5 + 1 file changed, 5 insertions(+) diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c index 9c7ffa4df5a8..4e480e870474 100644 --- a/kernel/trace/trace_functions_graph.c +++ b/kernel/trace/trace_functions_graph.c @@ -1182,6 +1182,11 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent, trace_seq_puts(s, "/* "); switch (iter->ent->type) { + case TRACE_BPUTS: + ret = trace_print_bputs_msg_only(iter); + if (ret != TRACE_TYPE_HANDLED) + return ret; + break; case TRACE_BPRINT: ret = trace_print_bprintk_msg_only(iter); if (ret != TRACE_TYPE_HANDLED) -- 2.9.3
[PATCH] ftrace: Handle TRACE_BPUTS in print_graph_comment
It missed to handle TRACE_BPUTS so messages recorded by trace_bputs() will be shown with symbol info unnecessarily. You can see it with the trace_printk sample code: # cd /sys/kernel/tracing/ # echo sys_sync > set_graph_function # echo 1 > options/sym-offset # echo function_graph > current_tracer Note that the sys_sync filter was there to prevent recording other functions and the sym-offset option was needed since the first message was called from a module init function so kallsyms doesn't have the symbol and omitted in the output. # cd ~/build/kernel # insmod samples/trace_printk/trace-printk.ko # cd - # head trace Before: # tracer: function_graph # # CPU DURATION FUNCTION CALLS # | | | | | | | 1) | /* 0xa0002000: This is a static string that will use trace_bputs */ 1) | /* This is a dynamic string that will use trace_puts */ 1) | /* trace_printk_irq_work+0x5/0x7b [trace_printk]: (irq) This is a static string that will use trace_bputs */ 1) | /* (irq) This is a dynamic string that will use trace_puts */ 1) | /* (irq) This is a static string that will use trace_bprintk() */ 1) | /* (irq) This is a dynamic string that will use trace_printk */ After: # tracer: function_graph # # CPU DURATION FUNCTION CALLS # | | | | | | | 1) | /* This is a static string that will use trace_bputs */ 1) | /* This is a dynamic string that will use trace_puts */ 1) | /* (irq) This is a static string that will use trace_bputs */ 1) | /* (irq) This is a dynamic string that will use trace_puts */ 1) | /* (irq) This is a static string that will use trace_bprintk() */ 1) | /* (irq) This is a dynamic string that will use trace_printk */ Signed-off-by: Namhyung Kim --- kernel/trace/trace_functions_graph.c | 5 + 1 file changed, 5 insertions(+) diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c index 9c7ffa4df5a8..4e480e870474 100644 --- a/kernel/trace/trace_functions_graph.c +++ b/kernel/trace/trace_functions_graph.c @@ -1182,6 +1182,11 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent, trace_seq_puts(s, "/* "); switch (iter->ent->type) { + case TRACE_BPUTS: + ret = trace_print_bputs_msg_only(iter); + if (ret != TRACE_TYPE_HANDLED) + return ret; + break; case TRACE_BPRINT: ret = trace_print_bprintk_msg_only(iter); if (ret != TRACE_TYPE_HANDLED) -- 2.9.3
Re: [PATCH v2 3/7] dts: sun8i-h3: add i2c0/i2c1 SoC peripherals
On Thu, Sep 1, 2016 at 3:30 AM,wrote: > From: Jorik Jonker > > This enables the i2c0/i2c1 peripherals of the SoC. There is actually a third > controller, but I do not have a board on hands on which i2c2 is exposed in > such > a way that I can verify that it works. If they are listed in the manual, and the interrupts, clocks, resets, pins all exist, that is good enough for me. > > Signed-off-by: Jorik Jonker Acked-by: Chen-Yu Tsai
Re: [PATCH v2 3/7] dts: sun8i-h3: add i2c0/i2c1 SoC peripherals
On Thu, Sep 1, 2016 at 3:30 AM, wrote: > From: Jorik Jonker > > This enables the i2c0/i2c1 peripherals of the SoC. There is actually a third > controller, but I do not have a board on hands on which i2c2 is exposed in > such > a way that I can verify that it works. If they are listed in the manual, and the interrupts, clocks, resets, pins all exist, that is good enough for me. > > Signed-off-by: Jorik Jonker Acked-by: Chen-Yu Tsai
Re: [PATCH] arm64: Improve kprobes test for atomic sequence
Hi Dave, On Wed, 31 Aug 2016 16:52:22 -0400 David Longwrote: > From: "David A. Long" > > Kprobes searches backwards a finite number of instructions to determine if > there is an attempt to probe a load/store exclusive sequence. It stops when > it hits the maximum number of instructions or a load or store exclusive. Hmm, so on aarch64, we can not put a kprobe between load exclusive and store exclusive, because kprobe always breaks the atomicity, am I correct? If so, what happen if any branch in the sequence? e.g. load-ex (do something) l1: store-ex ... load-ex (do something) branch l1; > However this means it can run up past the beginning of the function and > start looking at literal constants. This has been shown to cause a false > positive and blocks insertion of the probe. To fix this add a test to see > if the typical: > > "stp x29, x30, [sp, #n]!" > > instruction beginning a function gets hit. This also improves efficiency by > not testing code that is not part of the function. There is some > possibility that a function will not begin with this instruction, in which > case the fixed code will behave no worse than before. If the function boundary is the problem, why you wouldn't use kallsyms information as I did in can_optimize()@arch/x86/kernel/kprobes/opt.c ? /* Lookup symbol including addr */ if (!kallsyms_lookup_size_offset(paddr, , )) return 0; With this call, symbol start address is (paddr - offset) and end address is (paddr - offset + size). Thank you, > > There could also be the case that the stp instruction is found further in > the body of the function, which could theoretically allow probing of an > atomic squence. The likelihood of this seems low, and this would not be the > only aspect of kprobes where the user needs to be careful to avoid > problems. > > Signed-off-by: David A. Long > --- > arch/arm64/kernel/probes/decode-insn.c | 25 ++--- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/probes/decode-insn.c > b/arch/arm64/kernel/probes/decode-insn.c > index 37e47a9..248e820 100644 > --- a/arch/arm64/kernel/probes/decode-insn.c > +++ b/arch/arm64/kernel/probes/decode-insn.c > @@ -122,16 +122,28 @@ arm_probe_decode_insn(kprobe_opcode_t insn, struct > arch_specific_insn *asi) > static bool __kprobes > is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t > *scan_end) > { > + const u32 stp_x29_x30_sp_pre = 0xa9807bfd; > + const u32 stp_ignore_index_mask = 0xffc07fff; > + u32 instruction = le32_to_cpu(*scan_start); > + > while (scan_start > scan_end) { > /* > - * atomic region starts from exclusive load and ends with > - * exclusive store. > + * Atomic region starts from exclusive load and ends with > + * exclusive store. If we hit a "stp x29, x30, [sp, #n]!" > + * assume it is the beginning of the function and end the > + * search. This helps avoid false positives from literal > + * constants that look like a load-exclusive, in addition > + * to being more efficient. >*/ > - if (aarch64_insn_is_store_ex(le32_to_cpu(*scan_start))) > + if ((instruction & stp_ignore_index_mask) == stp_x29_x30_sp_pre) > return false; > - else if (aarch64_insn_is_load_ex(le32_to_cpu(*scan_start))) > - return true; > + > scan_start--; > + instruction = le32_to_cpu(*scan_start); > + if (aarch64_insn_is_store_ex(instruction)) > + return false; > + else if (aarch64_insn_is_load_ex(instruction)) > + return true; > } > > return false; > @@ -142,7 +154,6 @@ arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct > arch_specific_insn *asi) > { > enum kprobe_insn decoded; > kprobe_opcode_t insn = le32_to_cpu(*addr); > - kprobe_opcode_t *scan_start = addr - 1; > kprobe_opcode_t *scan_end = addr - MAX_ATOMIC_CONTEXT_SIZE; > #if defined(CONFIG_MODULES) && defined(MODULES_VADDR) > struct module *mod; > @@ -167,7 +178,7 @@ arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct > arch_specific_insn *asi) > decoded = arm_probe_decode_insn(insn, asi); > > if (decoded == INSN_REJECTED || > - is_probed_address_atomic(scan_start, scan_end)) > + is_probed_address_atomic(addr, scan_end)) > return INSN_REJECTED; > > return decoded; > -- > 2.5.0 > -- Masami Hiramatsu
Re: [PATCH] arm64: Improve kprobes test for atomic sequence
Hi Dave, On Wed, 31 Aug 2016 16:52:22 -0400 David Long wrote: > From: "David A. Long" > > Kprobes searches backwards a finite number of instructions to determine if > there is an attempt to probe a load/store exclusive sequence. It stops when > it hits the maximum number of instructions or a load or store exclusive. Hmm, so on aarch64, we can not put a kprobe between load exclusive and store exclusive, because kprobe always breaks the atomicity, am I correct? If so, what happen if any branch in the sequence? e.g. load-ex (do something) l1: store-ex ... load-ex (do something) branch l1; > However this means it can run up past the beginning of the function and > start looking at literal constants. This has been shown to cause a false > positive and blocks insertion of the probe. To fix this add a test to see > if the typical: > > "stp x29, x30, [sp, #n]!" > > instruction beginning a function gets hit. This also improves efficiency by > not testing code that is not part of the function. There is some > possibility that a function will not begin with this instruction, in which > case the fixed code will behave no worse than before. If the function boundary is the problem, why you wouldn't use kallsyms information as I did in can_optimize()@arch/x86/kernel/kprobes/opt.c ? /* Lookup symbol including addr */ if (!kallsyms_lookup_size_offset(paddr, , )) return 0; With this call, symbol start address is (paddr - offset) and end address is (paddr - offset + size). Thank you, > > There could also be the case that the stp instruction is found further in > the body of the function, which could theoretically allow probing of an > atomic squence. The likelihood of this seems low, and this would not be the > only aspect of kprobes where the user needs to be careful to avoid > problems. > > Signed-off-by: David A. Long > --- > arch/arm64/kernel/probes/decode-insn.c | 25 ++--- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/probes/decode-insn.c > b/arch/arm64/kernel/probes/decode-insn.c > index 37e47a9..248e820 100644 > --- a/arch/arm64/kernel/probes/decode-insn.c > +++ b/arch/arm64/kernel/probes/decode-insn.c > @@ -122,16 +122,28 @@ arm_probe_decode_insn(kprobe_opcode_t insn, struct > arch_specific_insn *asi) > static bool __kprobes > is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t > *scan_end) > { > + const u32 stp_x29_x30_sp_pre = 0xa9807bfd; > + const u32 stp_ignore_index_mask = 0xffc07fff; > + u32 instruction = le32_to_cpu(*scan_start); > + > while (scan_start > scan_end) { > /* > - * atomic region starts from exclusive load and ends with > - * exclusive store. > + * Atomic region starts from exclusive load and ends with > + * exclusive store. If we hit a "stp x29, x30, [sp, #n]!" > + * assume it is the beginning of the function and end the > + * search. This helps avoid false positives from literal > + * constants that look like a load-exclusive, in addition > + * to being more efficient. >*/ > - if (aarch64_insn_is_store_ex(le32_to_cpu(*scan_start))) > + if ((instruction & stp_ignore_index_mask) == stp_x29_x30_sp_pre) > return false; > - else if (aarch64_insn_is_load_ex(le32_to_cpu(*scan_start))) > - return true; > + > scan_start--; > + instruction = le32_to_cpu(*scan_start); > + if (aarch64_insn_is_store_ex(instruction)) > + return false; > + else if (aarch64_insn_is_load_ex(instruction)) > + return true; > } > > return false; > @@ -142,7 +154,6 @@ arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct > arch_specific_insn *asi) > { > enum kprobe_insn decoded; > kprobe_opcode_t insn = le32_to_cpu(*addr); > - kprobe_opcode_t *scan_start = addr - 1; > kprobe_opcode_t *scan_end = addr - MAX_ATOMIC_CONTEXT_SIZE; > #if defined(CONFIG_MODULES) && defined(MODULES_VADDR) > struct module *mod; > @@ -167,7 +178,7 @@ arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct > arch_specific_insn *asi) > decoded = arm_probe_decode_insn(insn, asi); > > if (decoded == INSN_REJECTED || > - is_probed_address_atomic(scan_start, scan_end)) > + is_probed_address_atomic(addr, scan_end)) > return INSN_REJECTED; > > return decoded; > -- > 2.5.0 > -- Masami Hiramatsu
Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
Hi, On 2016年09月01日 01:42, Doug Anderson wrote: Hi, On Sun, Aug 28, 2016 at 8:25 PM, Shawn Linwrote: On 2016/8/29 10:50, Elaine Zhang wrote: On 08/27/2016 11:05 PM, Shawn Lin wrote: On 2016/8/27 21:41, Ziyuan Xu wrote: Control power domain for eMMC via genpd to reduce power consumption. Signed-off-by: Elaine Zhang Signed-off-by: Ziyuan Xu It looks nice to me. But this should be merged after applying that[0] as your patch will break bind/unbind test for sdhci-of-arasan on rk3399 without it[0]. Moreover, Elaine should make sure that upstreamed rockchip power domain stuff would not off pd for emmc, *otherwise*, I should update my patch to make sure we update clkmul every time when doing suspend 2 resume.. Forgot to say: If use pd, Although there is no call to power odd the pd_emmc, it will be power off when the system doing suspend 2 resume. (Because the system call __device_suspend_noirq->pm_genpd_suspend_noirq->rockchip_pd_power_off) Thanks for explaining this. I checked the code a bit and actually I don't need to updata clkmul since it was recorded, although it is still reset to 0x10 reading from syscon. So for that, we can now pick it up without waiting for my sdhci-of-arasan's update. Reviewed-by: Shawn Lin This is fine to pick up _only_ if you don't care about suspend/resume. If you care about suspend/resume then someone needs to first write a patch that will re-init all "corecfg" values after power is turned on. Do you mean corecfg_clockmultiplier and corecfg_baseclkfreq, if yes, we don't need to strore/re-init it after resume. corecfg_clockmultiplier is only used to fetch host->clk_mul, and host->clk_mul has been a fixed value at run-time, unless driver unbind. The same as corecfg_clockmultiplier, corecfg_baseclkfreq is used to check the xin_clk at probe time, we don't reference it at run-time. BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works fine. Technically I think this should probably use "pm runtime" and not normal suspend/resume hooks. Any time we end up pm runtime suspended then I think our power will go off (because of genpd?) and we need to restore values. I understand your consideration. BUT genpd is in charge of on/off pd if the corresponding device node has "power-domains" property. RPM is unnecessary for this situation, we will not use autosuspend, right? @shawn, what's your opinion? I'm not sure if this should be done in a generic way where we try to save and restore all values in the "sdhci_arasan_soc_ctl_map" or if we should try to be smarter... -Doug ___ Linux-rockchip mailing list linux-rockc...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
Hi, On 2016年09月01日 01:42, Doug Anderson wrote: Hi, On Sun, Aug 28, 2016 at 8:25 PM, Shawn Lin wrote: On 2016/8/29 10:50, Elaine Zhang wrote: On 08/27/2016 11:05 PM, Shawn Lin wrote: On 2016/8/27 21:41, Ziyuan Xu wrote: Control power domain for eMMC via genpd to reduce power consumption. Signed-off-by: Elaine Zhang Signed-off-by: Ziyuan Xu It looks nice to me. But this should be merged after applying that[0] as your patch will break bind/unbind test for sdhci-of-arasan on rk3399 without it[0]. Moreover, Elaine should make sure that upstreamed rockchip power domain stuff would not off pd for emmc, *otherwise*, I should update my patch to make sure we update clkmul every time when doing suspend 2 resume.. Forgot to say: If use pd, Although there is no call to power odd the pd_emmc, it will be power off when the system doing suspend 2 resume. (Because the system call __device_suspend_noirq->pm_genpd_suspend_noirq->rockchip_pd_power_off) Thanks for explaining this. I checked the code a bit and actually I don't need to updata clkmul since it was recorded, although it is still reset to 0x10 reading from syscon. So for that, we can now pick it up without waiting for my sdhci-of-arasan's update. Reviewed-by: Shawn Lin This is fine to pick up _only_ if you don't care about suspend/resume. If you care about suspend/resume then someone needs to first write a patch that will re-init all "corecfg" values after power is turned on. Do you mean corecfg_clockmultiplier and corecfg_baseclkfreq, if yes, we don't need to strore/re-init it after resume. corecfg_clockmultiplier is only used to fetch host->clk_mul, and host->clk_mul has been a fixed value at run-time, unless driver unbind. The same as corecfg_clockmultiplier, corecfg_baseclkfreq is used to check the xin_clk at probe time, we don't reference it at run-time. BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works fine. Technically I think this should probably use "pm runtime" and not normal suspend/resume hooks. Any time we end up pm runtime suspended then I think our power will go off (because of genpd?) and we need to restore values. I understand your consideration. BUT genpd is in charge of on/off pd if the corresponding device node has "power-domains" property. RPM is unnecessary for this situation, we will not use autosuspend, right? @shawn, what's your opinion? I'm not sure if this should be done in a generic way where we try to save and restore all values in the "sdhci_arasan_soc_ctl_map" or if we should try to be smarter... -Doug ___ Linux-rockchip mailing list linux-rockc...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
[PATCH v2] serial: 8250_dw: Use an unified new dev variable in probe
Use an unified new dev variable instead of >dev and p->dev in probe function. Reviewed-by: Heikki KrogerusSigned-off-by: Kefeng Wang --- Hi Greg, updated, based on tty-testing branch :) v1->v2: 1) Add Heikki's reviewed-by 2) Rebase on tty-testing branch of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git drivers/tty/serial/8250/8250_dw.c | 45 --- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c index b022f5a..47e6d08 100644 --- a/drivers/tty/serial/8250/8250_dw.c +++ b/drivers/tty/serial/8250/8250_dw.c @@ -360,18 +360,19 @@ static int dw8250_probe(struct platform_device *pdev) struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); int irq = platform_get_irq(pdev, 0); struct uart_port *p = + struct device *dev = >dev; struct dw8250_data *data; int err; u32 val; if (!regs) { - dev_err(>dev, "no registers defined\n"); + dev_err(dev, "no registers defined\n"); return -EINVAL; } if (irq < 0) { if (irq != -EPROBE_DEFER) - dev_err(>dev, "cannot get irq\n"); + dev_err(dev, "cannot get irq\n"); return irq; } @@ -382,16 +383,16 @@ static int dw8250_probe(struct platform_device *pdev) p->pm = dw8250_do_pm; p->type = PORT_8250; p->flags= UPF_SHARE_IRQ | UPF_FIXED_PORT; - p->dev = >dev; + p->dev = dev; p->iotype = UPIO_MEM; p->serial_in= dw8250_serial_in; p->serial_out = dw8250_serial_out; - p->membase = devm_ioremap(>dev, regs->start, resource_size(regs)); + p->membase = devm_ioremap(dev, regs->start, resource_size(regs)); if (!p->membase) return -ENOMEM; - data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL); + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM; @@ -399,57 +400,57 @@ static int dw8250_probe(struct platform_device *pdev) data->usr_reg = DW_UART_USR; p->private_data = data; - data->uart_16550_compatible = device_property_read_bool(p->dev, + data->uart_16550_compatible = device_property_read_bool(dev, "snps,uart-16550-compatible"); - err = device_property_read_u32(p->dev, "reg-shift", ); + err = device_property_read_u32(dev, "reg-shift", ); if (!err) p->regshift = val; - err = device_property_read_u32(p->dev, "reg-io-width", ); + err = device_property_read_u32(dev, "reg-io-width", ); if (!err && val == 4) { p->iotype = UPIO_MEM32; p->serial_in = dw8250_serial_in32; p->serial_out = dw8250_serial_out32; } - if (device_property_read_bool(p->dev, "dcd-override")) { + if (device_property_read_bool(dev, "dcd-override")) { /* Always report DCD as active */ data->msr_mask_on |= UART_MSR_DCD; data->msr_mask_off |= UART_MSR_DDCD; } - if (device_property_read_bool(p->dev, "dsr-override")) { + if (device_property_read_bool(dev, "dsr-override")) { /* Always report DSR as active */ data->msr_mask_on |= UART_MSR_DSR; data->msr_mask_off |= UART_MSR_DDSR; } - if (device_property_read_bool(p->dev, "cts-override")) { + if (device_property_read_bool(dev, "cts-override")) { /* Always report CTS as active */ data->msr_mask_on |= UART_MSR_CTS; data->msr_mask_off |= UART_MSR_DCTS; } - if (device_property_read_bool(p->dev, "ri-override")) { + if (device_property_read_bool(dev, "ri-override")) { /* Always report Ring indicator as inactive */ data->msr_mask_off |= UART_MSR_RI; data->msr_mask_off |= UART_MSR_TERI; } /* Always ask for fixed clock rate from a property. */ - device_property_read_u32(p->dev, "clock-frequency", >uartclk); + device_property_read_u32(dev, "clock-frequency", >uartclk); /* If there is separate baudclk, get the rate from it. */ - data->clk = devm_clk_get(>dev, "baudclk"); + data->clk = devm_clk_get(dev, "baudclk"); if (IS_ERR(data->clk) && PTR_ERR(data->clk) != -EPROBE_DEFER) - data->clk = devm_clk_get(>dev, NULL); + data->clk = devm_clk_get(dev, NULL); if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER) return -EPROBE_DEFER; if (!IS_ERR_OR_NULL(data->clk)) {
[PATCH v2] serial: 8250_dw: Use an unified new dev variable in probe
Use an unified new dev variable instead of >dev and p->dev in probe function. Reviewed-by: Heikki Krogerus Signed-off-by: Kefeng Wang --- Hi Greg, updated, based on tty-testing branch :) v1->v2: 1) Add Heikki's reviewed-by 2) Rebase on tty-testing branch of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git drivers/tty/serial/8250/8250_dw.c | 45 --- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c index b022f5a..47e6d08 100644 --- a/drivers/tty/serial/8250/8250_dw.c +++ b/drivers/tty/serial/8250/8250_dw.c @@ -360,18 +360,19 @@ static int dw8250_probe(struct platform_device *pdev) struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); int irq = platform_get_irq(pdev, 0); struct uart_port *p = + struct device *dev = >dev; struct dw8250_data *data; int err; u32 val; if (!regs) { - dev_err(>dev, "no registers defined\n"); + dev_err(dev, "no registers defined\n"); return -EINVAL; } if (irq < 0) { if (irq != -EPROBE_DEFER) - dev_err(>dev, "cannot get irq\n"); + dev_err(dev, "cannot get irq\n"); return irq; } @@ -382,16 +383,16 @@ static int dw8250_probe(struct platform_device *pdev) p->pm = dw8250_do_pm; p->type = PORT_8250; p->flags= UPF_SHARE_IRQ | UPF_FIXED_PORT; - p->dev = >dev; + p->dev = dev; p->iotype = UPIO_MEM; p->serial_in= dw8250_serial_in; p->serial_out = dw8250_serial_out; - p->membase = devm_ioremap(>dev, regs->start, resource_size(regs)); + p->membase = devm_ioremap(dev, regs->start, resource_size(regs)); if (!p->membase) return -ENOMEM; - data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL); + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM; @@ -399,57 +400,57 @@ static int dw8250_probe(struct platform_device *pdev) data->usr_reg = DW_UART_USR; p->private_data = data; - data->uart_16550_compatible = device_property_read_bool(p->dev, + data->uart_16550_compatible = device_property_read_bool(dev, "snps,uart-16550-compatible"); - err = device_property_read_u32(p->dev, "reg-shift", ); + err = device_property_read_u32(dev, "reg-shift", ); if (!err) p->regshift = val; - err = device_property_read_u32(p->dev, "reg-io-width", ); + err = device_property_read_u32(dev, "reg-io-width", ); if (!err && val == 4) { p->iotype = UPIO_MEM32; p->serial_in = dw8250_serial_in32; p->serial_out = dw8250_serial_out32; } - if (device_property_read_bool(p->dev, "dcd-override")) { + if (device_property_read_bool(dev, "dcd-override")) { /* Always report DCD as active */ data->msr_mask_on |= UART_MSR_DCD; data->msr_mask_off |= UART_MSR_DDCD; } - if (device_property_read_bool(p->dev, "dsr-override")) { + if (device_property_read_bool(dev, "dsr-override")) { /* Always report DSR as active */ data->msr_mask_on |= UART_MSR_DSR; data->msr_mask_off |= UART_MSR_DDSR; } - if (device_property_read_bool(p->dev, "cts-override")) { + if (device_property_read_bool(dev, "cts-override")) { /* Always report CTS as active */ data->msr_mask_on |= UART_MSR_CTS; data->msr_mask_off |= UART_MSR_DCTS; } - if (device_property_read_bool(p->dev, "ri-override")) { + if (device_property_read_bool(dev, "ri-override")) { /* Always report Ring indicator as inactive */ data->msr_mask_off |= UART_MSR_RI; data->msr_mask_off |= UART_MSR_TERI; } /* Always ask for fixed clock rate from a property. */ - device_property_read_u32(p->dev, "clock-frequency", >uartclk); + device_property_read_u32(dev, "clock-frequency", >uartclk); /* If there is separate baudclk, get the rate from it. */ - data->clk = devm_clk_get(>dev, "baudclk"); + data->clk = devm_clk_get(dev, "baudclk"); if (IS_ERR(data->clk) && PTR_ERR(data->clk) != -EPROBE_DEFER) - data->clk = devm_clk_get(>dev, NULL); + data->clk = devm_clk_get(dev, NULL); if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER) return -EPROBE_DEFER; if (!IS_ERR_OR_NULL(data->clk)) { err = clk_prepare_enable(data->clk); if (err) -
Re: [PATCH] serial: 8250_dw: Use an unified new dev variable in probe
Sorry, please ignore, send wrong patch. On 2016/9/1 9:34, Kefeng Wang wrote: > Use an unified new dev variable instead of >dev and p->dev > in probe function. > > Signed-off-by: Kefeng Wang> --- > drivers/tty/serial/8250/8250_dw.c | 45 > --- > 1 file changed, 23 insertions(+), 22 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_dw.c > b/drivers/tty/serial/8250/8250_dw.c > index 3478c2c..225d797 100644 > --- a/drivers/tty/serial/8250/8250_dw.c > +++ b/drivers/tty/serial/8250/8250_dw.c > @@ -363,18 +363,19 @@ static int dw8250_probe(struct platform_device *pdev) > struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); > int irq = platform_get_irq(pdev, 0); > struct uart_port *p = > + struct device *dev = >dev; > struct dw8250_data *data; > int err; > u32 val; > > if (!regs) { > - dev_err(>dev, "no registers defined\n"); > + dev_err(dev, "no registers defined\n"); > return -EINVAL; > } > > if (irq < 0) { > if (irq != -EPROBE_DEFER) > - dev_err(>dev, "cannot get irq\n"); > + dev_err(dev, "cannot get irq\n"); > return irq; > } > > @@ -385,17 +386,17 @@ static int dw8250_probe(struct platform_device *pdev) > p->pm = dw8250_do_pm; > p->type = PORT_8250; > p->flags= UPF_SHARE_IRQ | UPF_FIXED_PORT; > - p->dev = >dev; > + p->dev = dev; > p->iotype = UPIO_MEM; > p->serial_in= dw8250_serial_in; > p->serial_out = dw8250_serial_out; > p->set_termios = dw8250_set_termios; > > - p->membase = devm_ioremap(>dev, regs->start, resource_size(regs)); > + p->membase = devm_ioremap(dev, regs->start, resource_size(regs)); > if (!p->membase) > return -ENOMEM; > > - data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL); > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > if (!data) > return -ENOMEM; > > @@ -403,57 +404,57 @@ static int dw8250_probe(struct platform_device *pdev) > data->usr_reg = DW_UART_USR; > p->private_data = data; > > - data->uart_16550_compatible = device_property_read_bool(p->dev, > + data->uart_16550_compatible = device_property_read_bool(dev, > "snps,uart-16550-compatible"); > > - err = device_property_read_u32(p->dev, "reg-shift", ); > + err = device_property_read_u32(dev, "reg-shift", ); > if (!err) > p->regshift = val; > > - err = device_property_read_u32(p->dev, "reg-io-width", ); > + err = device_property_read_u32(dev, "reg-io-width", ); > if (!err && val == 4) { > p->iotype = UPIO_MEM32; > p->serial_in = dw8250_serial_in32; > p->serial_out = dw8250_serial_out32; > } > > - if (device_property_read_bool(p->dev, "dcd-override")) { > + if (device_property_read_bool(dev, "dcd-override")) { > /* Always report DCD as active */ > data->msr_mask_on |= UART_MSR_DCD; > data->msr_mask_off |= UART_MSR_DDCD; > } > > - if (device_property_read_bool(p->dev, "dsr-override")) { > + if (device_property_read_bool(dev, "dsr-override")) { > /* Always report DSR as active */ > data->msr_mask_on |= UART_MSR_DSR; > data->msr_mask_off |= UART_MSR_DDSR; > } > > - if (device_property_read_bool(p->dev, "cts-override")) { > + if (device_property_read_bool(dev, "cts-override")) { > /* Always report CTS as active */ > data->msr_mask_on |= UART_MSR_CTS; > data->msr_mask_off |= UART_MSR_DCTS; > } > > - if (device_property_read_bool(p->dev, "ri-override")) { > + if (device_property_read_bool(dev, "ri-override")) { > /* Always report Ring indicator as inactive */ > data->msr_mask_off |= UART_MSR_RI; > data->msr_mask_off |= UART_MSR_TERI; > } > > /* Always ask for fixed clock rate from a property. */ > - device_property_read_u32(p->dev, "clock-frequency", >uartclk); > + device_property_read_u32(dev, "clock-frequency", >uartclk); > > /* If there is separate baudclk, get the rate from it. */ > - data->clk = devm_clk_get(>dev, "baudclk"); > + data->clk = devm_clk_get(dev, "baudclk"); > if (IS_ERR(data->clk) && PTR_ERR(data->clk) != -EPROBE_DEFER) > - data->clk = devm_clk_get(>dev, NULL); > + data->clk = devm_clk_get(dev, NULL); > if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER) > return -EPROBE_DEFER; > if (!IS_ERR_OR_NULL(data->clk)) { > err = clk_prepare_enable(data->clk); > if (err)
Re: [PATCH] serial: 8250_dw: Use an unified new dev variable in probe
Sorry, please ignore, send wrong patch. On 2016/9/1 9:34, Kefeng Wang wrote: > Use an unified new dev variable instead of >dev and p->dev > in probe function. > > Signed-off-by: Kefeng Wang > --- > drivers/tty/serial/8250/8250_dw.c | 45 > --- > 1 file changed, 23 insertions(+), 22 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_dw.c > b/drivers/tty/serial/8250/8250_dw.c > index 3478c2c..225d797 100644 > --- a/drivers/tty/serial/8250/8250_dw.c > +++ b/drivers/tty/serial/8250/8250_dw.c > @@ -363,18 +363,19 @@ static int dw8250_probe(struct platform_device *pdev) > struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); > int irq = platform_get_irq(pdev, 0); > struct uart_port *p = > + struct device *dev = >dev; > struct dw8250_data *data; > int err; > u32 val; > > if (!regs) { > - dev_err(>dev, "no registers defined\n"); > + dev_err(dev, "no registers defined\n"); > return -EINVAL; > } > > if (irq < 0) { > if (irq != -EPROBE_DEFER) > - dev_err(>dev, "cannot get irq\n"); > + dev_err(dev, "cannot get irq\n"); > return irq; > } > > @@ -385,17 +386,17 @@ static int dw8250_probe(struct platform_device *pdev) > p->pm = dw8250_do_pm; > p->type = PORT_8250; > p->flags= UPF_SHARE_IRQ | UPF_FIXED_PORT; > - p->dev = >dev; > + p->dev = dev; > p->iotype = UPIO_MEM; > p->serial_in= dw8250_serial_in; > p->serial_out = dw8250_serial_out; > p->set_termios = dw8250_set_termios; > > - p->membase = devm_ioremap(>dev, regs->start, resource_size(regs)); > + p->membase = devm_ioremap(dev, regs->start, resource_size(regs)); > if (!p->membase) > return -ENOMEM; > > - data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL); > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > if (!data) > return -ENOMEM; > > @@ -403,57 +404,57 @@ static int dw8250_probe(struct platform_device *pdev) > data->usr_reg = DW_UART_USR; > p->private_data = data; > > - data->uart_16550_compatible = device_property_read_bool(p->dev, > + data->uart_16550_compatible = device_property_read_bool(dev, > "snps,uart-16550-compatible"); > > - err = device_property_read_u32(p->dev, "reg-shift", ); > + err = device_property_read_u32(dev, "reg-shift", ); > if (!err) > p->regshift = val; > > - err = device_property_read_u32(p->dev, "reg-io-width", ); > + err = device_property_read_u32(dev, "reg-io-width", ); > if (!err && val == 4) { > p->iotype = UPIO_MEM32; > p->serial_in = dw8250_serial_in32; > p->serial_out = dw8250_serial_out32; > } > > - if (device_property_read_bool(p->dev, "dcd-override")) { > + if (device_property_read_bool(dev, "dcd-override")) { > /* Always report DCD as active */ > data->msr_mask_on |= UART_MSR_DCD; > data->msr_mask_off |= UART_MSR_DDCD; > } > > - if (device_property_read_bool(p->dev, "dsr-override")) { > + if (device_property_read_bool(dev, "dsr-override")) { > /* Always report DSR as active */ > data->msr_mask_on |= UART_MSR_DSR; > data->msr_mask_off |= UART_MSR_DDSR; > } > > - if (device_property_read_bool(p->dev, "cts-override")) { > + if (device_property_read_bool(dev, "cts-override")) { > /* Always report CTS as active */ > data->msr_mask_on |= UART_MSR_CTS; > data->msr_mask_off |= UART_MSR_DCTS; > } > > - if (device_property_read_bool(p->dev, "ri-override")) { > + if (device_property_read_bool(dev, "ri-override")) { > /* Always report Ring indicator as inactive */ > data->msr_mask_off |= UART_MSR_RI; > data->msr_mask_off |= UART_MSR_TERI; > } > > /* Always ask for fixed clock rate from a property. */ > - device_property_read_u32(p->dev, "clock-frequency", >uartclk); > + device_property_read_u32(dev, "clock-frequency", >uartclk); > > /* If there is separate baudclk, get the rate from it. */ > - data->clk = devm_clk_get(>dev, "baudclk"); > + data->clk = devm_clk_get(dev, "baudclk"); > if (IS_ERR(data->clk) && PTR_ERR(data->clk) != -EPROBE_DEFER) > - data->clk = devm_clk_get(>dev, NULL); > + data->clk = devm_clk_get(dev, NULL); > if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER) > return -EPROBE_DEFER; > if (!IS_ERR_OR_NULL(data->clk)) { > err = clk_prepare_enable(data->clk); > if (err) > -
Re: [PATCH 2/2] drivers/perf: arm_pmu: Fix NULL pointer dereference during probe
Will Deaconwrites: > On Sat, Aug 27, 2016 at 04:19:50PM +, Stefan Wahren wrote: >> Patch 7f1d642fbb5c ("drivers/perf: arm-pmu: Fix handling of SPI lacking >> interrupt-affinity property") unintended also fixes perf_event support >> for bcm2835 which doesn't have PMU interrupts. Unfortunately this change >> introduce a NULL pointer dereference on bcm2835, because irq_is_percpu >> always expected to be called with a valid IRQ. So fix this regression >> by validating the IRQ before. >> >> Signed-off-by: Stefan Wahren >> Fixes: 7f1d642fbb5c ("drivers/perf: arm-pmu: Fix handling of SPI lacking >> \"interrupt-affinity\" property") >> --- >> drivers/perf/arm_pmu.c |2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Thanks, these two look good to me. I'll queue them up as fixes and hopefully > they'll land in -rc5. FWIW, I tested this on bcm2835-rpi and verified it fixes the boot problem in mainline. Tested-by: Kevin Hilman Kevin
Re: [PATCH 2/2] drivers/perf: arm_pmu: Fix NULL pointer dereference during probe
Will Deacon writes: > On Sat, Aug 27, 2016 at 04:19:50PM +, Stefan Wahren wrote: >> Patch 7f1d642fbb5c ("drivers/perf: arm-pmu: Fix handling of SPI lacking >> interrupt-affinity property") unintended also fixes perf_event support >> for bcm2835 which doesn't have PMU interrupts. Unfortunately this change >> introduce a NULL pointer dereference on bcm2835, because irq_is_percpu >> always expected to be called with a valid IRQ. So fix this regression >> by validating the IRQ before. >> >> Signed-off-by: Stefan Wahren >> Fixes: 7f1d642fbb5c ("drivers/perf: arm-pmu: Fix handling of SPI lacking >> \"interrupt-affinity\" property") >> --- >> drivers/perf/arm_pmu.c |2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Thanks, these two look good to me. I'll queue them up as fixes and hopefully > they'll land in -rc5. FWIW, I tested this on bcm2835-rpi and verified it fixes the boot problem in mainline. Tested-by: Kevin Hilman Kevin
Re: [RFC PATCH V2 3/3] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers
在 2016/8/31 19:48, Arnd Bergmann 写道: On Wednesday, August 31, 2016 7:48:14 PM CEST Dongdong Liu wrote: +static struct hisi_rc_res rc_res[] = { + { + HIP05, + { + DEFINE_RES_MEM(0xb007, SZ_4K), + DEFINE_RES_MEM(0xb008, SZ_4K), + DEFINE_RES_MEM(0xb009, SZ_4K), + DEFINE_RES_MEM(0xb00a, SZ_4K) + } + }, + { + HIP06, + { + DEFINE_RES_MEM(0xa009, SZ_4K), + DEFINE_RES_MEM(0xa020, SZ_4K), + DEFINE_RES_MEM(0xa00a, SZ_4K), + DEFINE_RES_MEM(0xa00b, SZ_4K) + } + }, + { + HIP07, + { + DEFINE_RES_MEM(0xa009, SZ_4K), + DEFINE_RES_MEM(0xa020, SZ_4K), + DEFINE_RES_MEM(0xa00a, SZ_4K), + DEFINE_RES_MEM(0xa00b, SZ_4K), + DEFINE_RES_MEM(0x8a009UL, SZ_4K), + DEFINE_RES_MEM(0x8a020UL, SZ_4K), + DEFINE_RES_MEM(0x8a00aUL, SZ_4K), + DEFINE_RES_MEM(0x8a00bUL, SZ_4K), + DEFINE_RES_MEM(0x600a009UL, SZ_4K), + DEFINE_RES_MEM(0x600a020UL, SZ_4K), + DEFINE_RES_MEM(0x600a00aUL, SZ_4K), + DEFINE_RES_MEM(0x600a00bUL, SZ_4K), + DEFINE_RES_MEM(0x700a009UL, SZ_4K), + DEFINE_RES_MEM(0x700a020UL, SZ_4K), + DEFINE_RES_MEM(0x700a00aUL, SZ_4K), + DEFINE_RES_MEM(0x700a00bUL, SZ_4K) + } + }, I don't know much about ACPI, but I'm pretty sure this is not the normal way to find MMIO resources. Why not read them from the ACPI tables? Hi Arnd Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access. We have not found a comfortable ACPI way to describle RC itself config (not ECAM) resource . Thanks Dongdong Arnd .
Re: [RFC PATCH V2 3/3] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers
在 2016/8/31 19:48, Arnd Bergmann 写道: On Wednesday, August 31, 2016 7:48:14 PM CEST Dongdong Liu wrote: +static struct hisi_rc_res rc_res[] = { + { + HIP05, + { + DEFINE_RES_MEM(0xb007, SZ_4K), + DEFINE_RES_MEM(0xb008, SZ_4K), + DEFINE_RES_MEM(0xb009, SZ_4K), + DEFINE_RES_MEM(0xb00a, SZ_4K) + } + }, + { + HIP06, + { + DEFINE_RES_MEM(0xa009, SZ_4K), + DEFINE_RES_MEM(0xa020, SZ_4K), + DEFINE_RES_MEM(0xa00a, SZ_4K), + DEFINE_RES_MEM(0xa00b, SZ_4K) + } + }, + { + HIP07, + { + DEFINE_RES_MEM(0xa009, SZ_4K), + DEFINE_RES_MEM(0xa020, SZ_4K), + DEFINE_RES_MEM(0xa00a, SZ_4K), + DEFINE_RES_MEM(0xa00b, SZ_4K), + DEFINE_RES_MEM(0x8a009UL, SZ_4K), + DEFINE_RES_MEM(0x8a020UL, SZ_4K), + DEFINE_RES_MEM(0x8a00aUL, SZ_4K), + DEFINE_RES_MEM(0x8a00bUL, SZ_4K), + DEFINE_RES_MEM(0x600a009UL, SZ_4K), + DEFINE_RES_MEM(0x600a020UL, SZ_4K), + DEFINE_RES_MEM(0x600a00aUL, SZ_4K), + DEFINE_RES_MEM(0x600a00bUL, SZ_4K), + DEFINE_RES_MEM(0x700a009UL, SZ_4K), + DEFINE_RES_MEM(0x700a020UL, SZ_4K), + DEFINE_RES_MEM(0x700a00aUL, SZ_4K), + DEFINE_RES_MEM(0x700a00bUL, SZ_4K) + } + }, I don't know much about ACPI, but I'm pretty sure this is not the normal way to find MMIO resources. Why not read them from the ACPI tables? Hi Arnd Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access. We have not found a comfortable ACPI way to describle RC itself config (not ECAM) resource . Thanks Dongdong Arnd .
Re: [PATCH] i2c: rk3x: Restore clock settings at resume time
Hi Doug, Last replied email HTML HEAD error, resend it. 在 2016/8/30 5:22, Douglas Anderson 写道: Depending on a number of factors including: - Which exact Rockchip SoC we're working with - How deep we suspend - Which i2c port we're on We might lose the state of the i2c registers at suspend time. Specifically we've found that on rk3399 the i2c ports that are not in the PMU power domain lose their state with the current suspend depth configured by ARM Tursted Firmware. Note that there are very few actual i2c registers that aren't configured per transfer anyway so all we actually need to re-configure are the clock config registers. We'll just add a call to rk3x_i2c_adapt_div() at resume time and be done with it. NOTE: On rk3399 on ports whose power was lost, I put printouts in at resume time. I saw things like: before: con=0x00010300, div=0x00060006 after: con=0x00010200, div=0x00180025 I do the suspend/resume stress test on my rk3399 & rk3288 board more than 24 hours, the patch works well, it looks nice to me. Reviewed-by: David WuTested-by: David Wu Signed-off-by: Douglas Anderson --- Tested on chromeos-4.4 kernel with backports. drivers/i2c/busses/i2c-rk3x.c | 12 1 file changed, 12 insertions(+) diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c index 3b87afe82f63..0df5ad6bfa31 100644 --- a/drivers/i2c/busses/i2c-rk3x.c +++ b/drivers/i2c/busses/i2c-rk3x.c @@ -,6 +,15 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap, return ret < 0 ? ret : num; } +static __maybe_unused int rk3x_i2c_resume(struct device *dev) +{ + struct rk3x_i2c *i2c = dev_get_drvdata(dev); + + rk3x_i2c_adapt_div(i2c, clk_get_rate(i2c->clk)); + + return 0; +} + static u32 rk3x_i2c_func(struct i2c_adapter *adap) { return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_PROTOCOL_MANGLING; @@ -1332,12 +1341,15 @@ static int rk3x_i2c_remove(struct platform_device *pdev) return 0; } +static const SIMPLE_DEV_PM_OPS(rk3x_i2c_pm_ops, NULL, rk3x_i2c_resume); + static struct platform_driver rk3x_i2c_driver = { .probe = rk3x_i2c_probe, .remove = rk3x_i2c_remove, .driver = { .name = "rk3x-i2c", .of_match_table = rk3x_i2c_match, + .pm = _i2c_pm_ops, }, };
Re: [PATCH] i2c: rk3x: Restore clock settings at resume time
Hi Doug, Last replied email HTML HEAD error, resend it. 在 2016/8/30 5:22, Douglas Anderson 写道: Depending on a number of factors including: - Which exact Rockchip SoC we're working with - How deep we suspend - Which i2c port we're on We might lose the state of the i2c registers at suspend time. Specifically we've found that on rk3399 the i2c ports that are not in the PMU power domain lose their state with the current suspend depth configured by ARM Tursted Firmware. Note that there are very few actual i2c registers that aren't configured per transfer anyway so all we actually need to re-configure are the clock config registers. We'll just add a call to rk3x_i2c_adapt_div() at resume time and be done with it. NOTE: On rk3399 on ports whose power was lost, I put printouts in at resume time. I saw things like: before: con=0x00010300, div=0x00060006 after: con=0x00010200, div=0x00180025 I do the suspend/resume stress test on my rk3399 & rk3288 board more than 24 hours, the patch works well, it looks nice to me. Reviewed-by: David Wu Tested-by: David Wu Signed-off-by: Douglas Anderson --- Tested on chromeos-4.4 kernel with backports. drivers/i2c/busses/i2c-rk3x.c | 12 1 file changed, 12 insertions(+) diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c index 3b87afe82f63..0df5ad6bfa31 100644 --- a/drivers/i2c/busses/i2c-rk3x.c +++ b/drivers/i2c/busses/i2c-rk3x.c @@ -,6 +,15 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap, return ret < 0 ? ret : num; } +static __maybe_unused int rk3x_i2c_resume(struct device *dev) +{ + struct rk3x_i2c *i2c = dev_get_drvdata(dev); + + rk3x_i2c_adapt_div(i2c, clk_get_rate(i2c->clk)); + + return 0; +} + static u32 rk3x_i2c_func(struct i2c_adapter *adap) { return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_PROTOCOL_MANGLING; @@ -1332,12 +1341,15 @@ static int rk3x_i2c_remove(struct platform_device *pdev) return 0; } +static const SIMPLE_DEV_PM_OPS(rk3x_i2c_pm_ops, NULL, rk3x_i2c_resume); + static struct platform_driver rk3x_i2c_driver = { .probe = rk3x_i2c_probe, .remove = rk3x_i2c_remove, .driver = { .name = "rk3x-i2c", .of_match_table = rk3x_i2c_match, + .pm = _i2c_pm_ops, }, };
Re: [PATCH v2 00/11] staging: octeon: multi rx group (queue) support
On 8/31/16 13:57, Aaro Koskinen wrote: > This series implements multiple RX group support that should improve > the networking performance on multi-core OCTEONs. Basically we register > IRQ and NAPI for each group, and ask the HW to select the group for > the incoming packets based on hash. > > Tested on EdgeRouter Lite with a simple forwarding test using two flows > and 16 RX groups distributed between two cores - the routing throughput > is roughly doubled. > > Also tested with EBH5600 (8 cores) and EBB6800 (16 cores) by sending > and receiving traffic in both directions using SGMII interfaces. With this series on 4.4.19, rx works with receive_group_order > 0. Setting receive_group_order=4, I do see 16 Ethernet interrupts. I tried fiddling with various smp_affinity values (e.g. setting them all to , or assigning a different one to each interrupt, or giving a few to some and a few to others), as well as different values for rps_cpus. 10-thread parallel iperf performance varies between 0.5 and 1.5 Gbit/sec total depending on the particular settings. With the SDK kernel I get over 8 Gbit/sec. It seems to be achieving that using just one interrupt (not even a separate one for tx, as far as I can tell) pegged to CPU 0 (the default smp_affinity). I must be missing some other major configuration tweak, perhaps specific to 10G. Can you run a test on the EBB6800 with the interfaces in 10G mode? --Ed
Re: [PATCH v2 00/11] staging: octeon: multi rx group (queue) support
On 8/31/16 13:57, Aaro Koskinen wrote: > This series implements multiple RX group support that should improve > the networking performance on multi-core OCTEONs. Basically we register > IRQ and NAPI for each group, and ask the HW to select the group for > the incoming packets based on hash. > > Tested on EdgeRouter Lite with a simple forwarding test using two flows > and 16 RX groups distributed between two cores - the routing throughput > is roughly doubled. > > Also tested with EBH5600 (8 cores) and EBB6800 (16 cores) by sending > and receiving traffic in both directions using SGMII interfaces. With this series on 4.4.19, rx works with receive_group_order > 0. Setting receive_group_order=4, I do see 16 Ethernet interrupts. I tried fiddling with various smp_affinity values (e.g. setting them all to , or assigning a different one to each interrupt, or giving a few to some and a few to others), as well as different values for rps_cpus. 10-thread parallel iperf performance varies between 0.5 and 1.5 Gbit/sec total depending on the particular settings. With the SDK kernel I get over 8 Gbit/sec. It seems to be achieving that using just one interrupt (not even a separate one for tx, as far as I can tell) pegged to CPU 0 (the default smp_affinity). I must be missing some other major configuration tweak, perhaps specific to 10G. Can you run a test on the EBB6800 with the interfaces in 10G mode? --Ed
Re: [RFC PATCH V2 1/3] PCI: hisi: re-architect Hip05/Hip06 controllers driver to preapare for ACPI
在 2016/8/31 19:45, Arnd Bergmann 写道: On Wednesday, August 31, 2016 7:48:12 PM CEST Dongdong Liu wrote: + +/* HipXX PCIe host only supports 32-bit config access */ +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size, + u32 *val) +{ + u32 reg; + u32 reg_val; + void *walker = _val; + + walker += (where & 0x3); + reg = where & ~0x3; + reg_val = readl(reg_base + reg); + + if (size == 1) + *val = *(u8 __force *) walker; + else if (size == 2) + *val = *(u16 __force *) walker; What is the __force for? Hi Arnd, thanks for replying. __force is used to, well, force a conversion, like casting from or to a bitwise type, else the Sparse checker will throw a warning. + else if (size == 4) + *val = reg_val; + else + return PCIBIOS_BAD_REGISTER_NUMBER; + + return PCIBIOS_SUCCESSFUL; It looks like you are reimplementing pci_generic_config_read32/pci_generic_config_write32 read here, better use them directly. For our host bridge, access RC and EP config space are not the same way. Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access. hisi_pcie_common_cfg_read is used to read RC config space, only supports 32-bit config access. hisi_pcie_common_cfg_read/hisi_pcie_common_cfg_write may change as below will be better. /* HipXX PCIe host only supports 32-bit config access */ int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size, u32 *val) { void __iomem *addr; addr = reg_base + (where & ~0x3); *val = readl(addr); if (size <= 2) *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1); return PCIBIOS_SUCCESSFUL; } /* HipXX PCIe host only supports 32-bit config access */ int hisi_pcie_common_cfg_write(void __iomem *reg_base, int where, int size, u32 val) { void __iomem *addr; u32 mask, tmp; addr = reg_base + (where & ~0x3); if (size == 4) { writel(val, addr); return PCIBIOS_SUCCESSFUL; } else { mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8)); } tmp = readl(addr) & mask; tmp |= val << ((where & 0x3) * 8); writel(tmp, addr); return PCIBIOS_SUCCESSFUL; } Thanks Dongdong Arnd .
Re: [RFC PATCH V2 1/3] PCI: hisi: re-architect Hip05/Hip06 controllers driver to preapare for ACPI
在 2016/8/31 19:45, Arnd Bergmann 写道: On Wednesday, August 31, 2016 7:48:12 PM CEST Dongdong Liu wrote: + +/* HipXX PCIe host only supports 32-bit config access */ +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size, + u32 *val) +{ + u32 reg; + u32 reg_val; + void *walker = _val; + + walker += (where & 0x3); + reg = where & ~0x3; + reg_val = readl(reg_base + reg); + + if (size == 1) + *val = *(u8 __force *) walker; + else if (size == 2) + *val = *(u16 __force *) walker; What is the __force for? Hi Arnd, thanks for replying. __force is used to, well, force a conversion, like casting from or to a bitwise type, else the Sparse checker will throw a warning. + else if (size == 4) + *val = reg_val; + else + return PCIBIOS_BAD_REGISTER_NUMBER; + + return PCIBIOS_SUCCESSFUL; It looks like you are reimplementing pci_generic_config_read32/pci_generic_config_write32 read here, better use them directly. For our host bridge, access RC and EP config space are not the same way. Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access. hisi_pcie_common_cfg_read is used to read RC config space, only supports 32-bit config access. hisi_pcie_common_cfg_read/hisi_pcie_common_cfg_write may change as below will be better. /* HipXX PCIe host only supports 32-bit config access */ int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size, u32 *val) { void __iomem *addr; addr = reg_base + (where & ~0x3); *val = readl(addr); if (size <= 2) *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1); return PCIBIOS_SUCCESSFUL; } /* HipXX PCIe host only supports 32-bit config access */ int hisi_pcie_common_cfg_write(void __iomem *reg_base, int where, int size, u32 val) { void __iomem *addr; u32 mask, tmp; addr = reg_base + (where & ~0x3); if (size == 4) { writel(val, addr); return PCIBIOS_SUCCESSFUL; } else { mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8)); } tmp = readl(addr) & mask; tmp |= val << ((where & 0x3) * 8); writel(tmp, addr); return PCIBIOS_SUCCESSFUL; } Thanks Dongdong Arnd .
Re: [PATCH 2/2] staging: lustre: hide unused variable
> After a code cleanup, we get a harmless warning about a variable > that is unused when CONFIG_FS_POSIX_ACL is disabled: > > drivers/staging/lustre/lustre/llite/xattr.c: In function > 'll_xattr_get_common': > drivers/staging/lustre/lustre/llite/xattr.c:312:24: error: unused variable > 'lli' [-Werror=unused-variable] > > This puts the variable declaration into the same #ifdef. > > Signed-off-by: Arnd Bergmann> Fixes: 1e1f9ff406fd ("staging: lustre: llite: break ll_getxattr_common into 2 > functions") Reviewed-by: James Simmons > --- > drivers/staging/lustre/lustre/llite/xattr.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/staging/lustre/lustre/llite/xattr.c > b/drivers/staging/lustre/lustre/llite/xattr.c > index f252c26ec30f..7b8d4699a71a 100644 > --- a/drivers/staging/lustre/lustre/llite/xattr.c > +++ b/drivers/staging/lustre/lustre/llite/xattr.c > @@ -309,7 +309,9 @@ static int ll_xattr_get_common(const struct xattr_handler > *handler, > { > char fullname[strlen(handler->prefix) + strlen(name) + 1]; > struct ll_sb_info *sbi = ll_i2sbi(inode); > +#ifdef CONFIG_FS_POSIX_ACL > struct ll_inode_info *lli = ll_i2info(inode); > +#endif > int rc; > > CDEBUG(D_VFSTRACE, "VFS Op:inode="DFID"(%p)\n", > -- > 2.9.0 > >
Re: [PATCH 2/2] staging: lustre: hide unused variable
> After a code cleanup, we get a harmless warning about a variable > that is unused when CONFIG_FS_POSIX_ACL is disabled: > > drivers/staging/lustre/lustre/llite/xattr.c: In function > 'll_xattr_get_common': > drivers/staging/lustre/lustre/llite/xattr.c:312:24: error: unused variable > 'lli' [-Werror=unused-variable] > > This puts the variable declaration into the same #ifdef. > > Signed-off-by: Arnd Bergmann > Fixes: 1e1f9ff406fd ("staging: lustre: llite: break ll_getxattr_common into 2 > functions") Reviewed-by: James Simmons > --- > drivers/staging/lustre/lustre/llite/xattr.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/staging/lustre/lustre/llite/xattr.c > b/drivers/staging/lustre/lustre/llite/xattr.c > index f252c26ec30f..7b8d4699a71a 100644 > --- a/drivers/staging/lustre/lustre/llite/xattr.c > +++ b/drivers/staging/lustre/lustre/llite/xattr.c > @@ -309,7 +309,9 @@ static int ll_xattr_get_common(const struct xattr_handler > *handler, > { > char fullname[strlen(handler->prefix) + strlen(name) + 1]; > struct ll_sb_info *sbi = ll_i2sbi(inode); > +#ifdef CONFIG_FS_POSIX_ACL > struct ll_inode_info *lli = ll_i2info(inode); > +#endif > int rc; > > CDEBUG(D_VFSTRACE, "VFS Op:inode="DFID"(%p)\n", > -- > 2.9.0 > >