Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.
On Wed, May 09, 2018 at 12:51:53PM +0800, Shawn Guo wrote: > Hi Bjorn, > > On Tue, May 08, 2018 at 07:56:58AM -0500, Bjorn Helgaas wrote: > ... > > > + return ret; > > > + } > > > + } > > > + > > > + pci->pp.root_bus_nr = -1; > > > > Setting root_bus_nr looks like an unrelated change that should be in a > > separate patch. > > > > But I'm not sure why you need to set root_bus_nr at all, since > > dw_pcie_host_init() always sets it. > > > > Some other callers of dw_pcie_host_init() do set it: > > > > exynos_add_pcie_port() > > imx6_add_pcie_port() > > armada8k_add_pcie_port() > > artpec6_add_pcie_port() > > dw_plat_add_pcie_port() > > histb_pcie_probe() > > qcom_pcie_probe() > > spear13xx_add_pcie_port() > > > > But I don't see *why* any of these need to set it. If they don't need to > > set it, they shouldn't. > > Mostly it's a blind copy of unnecessary code. I tested histb driver > by dropping the line, and did not see anything broken. I will cook up > a series to remove the code from all above drivers, and copy > corresponding driver owner to comment. Thanks, Shawn, I really appreciate that! > > And it would be nice if histb and qcom followed the structure and naming > > conventions of the other drivers, i.e., they should have > > histb_add_pcie_port() and qcom_add_pcie_port(). > > I can create a patch for histb driver, but will leave qcom one to > Stanimir to decide. Sounds good, thanks again!
Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.
Hi, On 9.05.2018 07:51, Shawn Guo wrote: Hi Bjorn, On Tue, May 08, 2018 at 07:56:58AM -0500, Bjorn Helgaas wrote: ... + return ret; + } + } + + pci->pp.root_bus_nr = -1; Setting root_bus_nr looks like an unrelated change that should be in a separate patch. But I'm not sure why you need to set root_bus_nr at all, since dw_pcie_host_init() always sets it. Some other callers of dw_pcie_host_init() do set it: exynos_add_pcie_port() imx6_add_pcie_port() armada8k_add_pcie_port() artpec6_add_pcie_port() dw_plat_add_pcie_port() histb_pcie_probe() qcom_pcie_probe() spear13xx_add_pcie_port() But I don't see *why* any of these need to set it. If they don't need to set it, they shouldn't. Mostly it's a blind copy of unnecessary code. I tested histb driver by dropping the line, and did not see anything broken. I will cook up a series to remove the code from all above drivers, and copy corresponding driver owner to comment. And it would be nice if histb and qcom followed the structure and naming conventions of the other drivers, i.e., they should have histb_add_pcie_port() and qcom_add_pcie_port(). I can create a patch for histb driver, but will leave qcom one to Stanimir to decide. Thanks, will see what can be done. regards, Stan
Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.
On 2018/5/8 20:56, Bjorn Helgaas wrote: > [+cc Stanimir, Jianguo, Shawn for qcom, histb comments below] > > On Tue, May 08, 2018 at 03:03:48PM +0800, Yao Chen wrote: >> This patch adds kirin pcie msi feature. > > Thanks for the patch! Please update your subject and changelog like this: > > PCI: kirin: Add MSI support > > Add support for MSI. > > You can figure out what style to use for the subject by running: > > git log --oneline drivers/pci/dwc/pcie-kirin.c > > Then follow the existing style. > Ok, I will update the style of subject and changelog. >> Signed-off-by: Yao Chen >> --- >> arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 2 ++ >> drivers/pci/dwc/pcie-kirin.c | 38 >> +++ >> 2 files changed, 40 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> index ec3eb8e..4ef684f 100644 >> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> @@ -872,6 +872,8 @@ >>0x0 0x0200>; >> num-lanes = <1>; >> #interrupt-cells = <1>; >> +interrupts = <0 283 4>; >> +interrupts-names = "msi"; >> interrupt-map-mask = <0xf800 0 0 7>; >> interrupt-map = <0x0 0 0 1 >> &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>, >> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c >> index d2970a0..2319c9c 100644 >> --- a/drivers/pci/dwc/pcie-kirin.c >> +++ b/drivers/pci/dwc/pcie-kirin.c >> @@ -426,9 +426,28 @@ static int kirin_pcie_establish_link(struct pcie_port >> *pp) >> return 0; >> } >> >> +static irqreturn_t kirin_pcie_msi_irq_handler(int irq, void *arg) >> +{ >> +struct pcie_port *pp = arg; >> + >> +return dw_handle_msi_irq(pp); >> +} >> + >> +static void kirin_pcie_msi_init(struct pcie_port *pp) >> +{ >> +dw_pcie_msi_init(pp); >> +} >> + >> +static void kirin_pcie_enable_interrupts(struct pcie_port *pp) >> +{ >> +if (IS_ENABLED(CONFIG_PCI_MSI)) >> +kirin_pcie_msi_init(pp); >> +} >> + >> static int kirin_pcie_host_init(struct pcie_port *pp) >> { >> kirin_pcie_establish_link(pp); >> +kirin_pcie_enable_interrupts(pp); >> >> return 0; >> } >> @@ -448,6 +467,25 @@ static int kirin_pcie_host_init(struct pcie_port *pp) >> static int __init kirin_add_pcie_port(struct dw_pcie *pci, >>struct platform_device *pdev) >> { >> +int ret; >> + >> +if (IS_ENABLED(CONFIG_PCI_MSI)) { >> +pci->pp.msi_irq = platform_get_irq(pdev, 0); >> +if (!pci->pp.msi_irq) { >> +dev_err(&pdev->dev, "failed to get msi irq\n"); > > s/msi irq/MSI IRQ/ > >> +return -ENODEV; >> +} >> +ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, >> + kirin_pcie_msi_irq_handler, >> + IRQF_SHARED | IRQF_NO_THREAD, >> + "kirin_pcie_msi", &pci->pp); >> +if (ret) { >> +dev_err(&pdev->dev, "failed to request msi irq\n"); > > s/msi irq/MSI IRQ/ > Also, please include the IRQ number (pci->pp.msi_irq) in the message. > Ok, I will add msi_irq message. >> +return ret; >> +} >> +} >> + >> +pci->pp.root_bus_nr = -1; > > Setting root_bus_nr looks like an unrelated change that should be in a > separate patch. > > But I'm not sure why you need to set root_bus_nr at all, since > dw_pcie_host_init() always sets it. > > Some other callers of dw_pcie_host_init() do set it: > > exynos_add_pcie_port() > imx6_add_pcie_port() > armada8k_add_pcie_port() > artpec6_add_pcie_port() > dw_plat_add_pcie_port() > histb_pcie_probe() > qcom_pcie_probe() > spear13xx_add_pcie_port() > > But I don't see *why* any of these need to set it. If they don't need to > set it, they shouldn't. > > And it would be nice if histb and qcom followed the structure and naming > conventions of the other drivers, i.e., they should have > histb_add_pcie_port() and qcom_add_pcie_port(). > You're right, "pci->pp.root_bus_nr = -1;" is truly unnecessary. I will delete it. >> pci->pp.ops = &kirin_pcie_host_ops; >> >> return dw_pcie_host_init(&pci->pp); >> -- >> 1.9.1 >> >> >> ___ >> linux-arm-kernel mailing list >> linux-arm-ker...@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > . > Thanks, Yao
Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.
Hi Bjorn, On Tue, May 08, 2018 at 07:56:58AM -0500, Bjorn Helgaas wrote: ... > > + return ret; > > + } > > + } > > + > > + pci->pp.root_bus_nr = -1; > > Setting root_bus_nr looks like an unrelated change that should be in a > separate patch. > > But I'm not sure why you need to set root_bus_nr at all, since > dw_pcie_host_init() always sets it. > > Some other callers of dw_pcie_host_init() do set it: > > exynos_add_pcie_port() > imx6_add_pcie_port() > armada8k_add_pcie_port() > artpec6_add_pcie_port() > dw_plat_add_pcie_port() > histb_pcie_probe() > qcom_pcie_probe() > spear13xx_add_pcie_port() > > But I don't see *why* any of these need to set it. If they don't need to > set it, they shouldn't. Mostly it's a blind copy of unnecessary code. I tested histb driver by dropping the line, and did not see anything broken. I will cook up a series to remove the code from all above drivers, and copy corresponding driver owner to comment. > And it would be nice if histb and qcom followed the structure and naming > conventions of the other drivers, i.e., they should have > histb_add_pcie_port() and qcom_add_pcie_port(). I can create a patch for histb driver, but will leave qcom one to Stanimir to decide. Shawn > > > pci->pp.ops = &kirin_pcie_host_ops; > > > > return dw_pcie_host_init(&pci->pp); > > -- > > 1.9.1 > > > > > > ___ > > linux-arm-kernel mailing list > > linux-arm-ker...@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.
On 2018/5/9 5:31, Dmitry Shmidt wrote: > On Tue, May 8, 2018 at 12:03 AM, Yao Chen wrote: >> This patch adds kirin pcie msi feature. >> >> Signed-off-by: Yao Chen >> --- >> arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 2 ++ >> drivers/pci/dwc/pcie-kirin.c | 38 >> +++ >> 2 files changed, 40 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> index ec3eb8e..4ef684f 100644 >> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> @@ -872,6 +872,8 @@ >> 0x0 0x0200>; >> num-lanes = <1>; >> #interrupt-cells = <1>; >> + interrupts = <0 283 4>; >> + interrupts-names = "msi"; > > Possible typo? Should be interrupt-names = "msi"; Yes, you're right. It should be interrupt-names = "msi". I will fix it. Thanks. > >> interrupt-map-mask = <0xf800 0 0 7>; >> interrupt-map = <0x0 0 0 1 >> &gic GIC_SPI 282 >> IRQ_TYPE_LEVEL_HIGH>, >> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c >> index d2970a0..2319c9c 100644 >> --- a/drivers/pci/dwc/pcie-kirin.c >> +++ b/drivers/pci/dwc/pcie-kirin.c >> @@ -426,9 +426,28 @@ static int kirin_pcie_establish_link(struct pcie_port >> *pp) >> return 0; >> } >> >> +static irqreturn_t kirin_pcie_msi_irq_handler(int irq, void *arg) >> +{ >> + struct pcie_port *pp = arg; >> + >> + return dw_handle_msi_irq(pp); >> +} >> + >> +static void kirin_pcie_msi_init(struct pcie_port *pp) >> +{ >> + dw_pcie_msi_init(pp); >> +} >> + >> +static void kirin_pcie_enable_interrupts(struct pcie_port *pp) >> +{ >> + if (IS_ENABLED(CONFIG_PCI_MSI)) >> + kirin_pcie_msi_init(pp); >> +} >> + >> static int kirin_pcie_host_init(struct pcie_port *pp) >> { >> kirin_pcie_establish_link(pp); >> + kirin_pcie_enable_interrupts(pp); >> >> return 0; >> } >> @@ -448,6 +467,25 @@ static int kirin_pcie_host_init(struct pcie_port *pp) >> static int __init kirin_add_pcie_port(struct dw_pcie *pci, >> struct platform_device *pdev) >> { >> + int ret; >> + >> + if (IS_ENABLED(CONFIG_PCI_MSI)) { >> + pci->pp.msi_irq = platform_get_irq(pdev, 0); >> + if (!pci->pp.msi_irq) { >> + dev_err(&pdev->dev, "failed to get msi irq\n"); >> + return -ENODEV; >> + } >> + ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, >> + kirin_pcie_msi_irq_handler, >> + IRQF_SHARED | IRQF_NO_THREAD, >> + "kirin_pcie_msi", &pci->pp); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to request msi irq\n"); >> + return ret; >> + } >> + } >> + >> + pci->pp.root_bus_nr = -1; >> pci->pp.ops = &kirin_pcie_host_ops; >> >> return dw_pcie_host_init(&pci->pp); >> -- >> 1.9.1 >> > > . >
Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.
On Tue, May 8, 2018 at 12:03 AM, Yao Chen wrote: > This patch adds kirin pcie msi feature. > > Signed-off-by: Yao Chen > --- > arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 2 ++ > drivers/pci/dwc/pcie-kirin.c | 38 > +++ > 2 files changed, 40 insertions(+) > > diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > index ec3eb8e..4ef684f 100644 > --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > @@ -872,6 +872,8 @@ > 0x0 0x0200>; > num-lanes = <1>; > #interrupt-cells = <1>; > + interrupts = <0 283 4>; > + interrupts-names = "msi"; Possible typo? Should be interrupt-names = "msi"; > interrupt-map-mask = <0xf800 0 0 7>; > interrupt-map = <0x0 0 0 1 > &gic GIC_SPI 282 > IRQ_TYPE_LEVEL_HIGH>, > diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c > index d2970a0..2319c9c 100644 > --- a/drivers/pci/dwc/pcie-kirin.c > +++ b/drivers/pci/dwc/pcie-kirin.c > @@ -426,9 +426,28 @@ static int kirin_pcie_establish_link(struct pcie_port > *pp) > return 0; > } > > +static irqreturn_t kirin_pcie_msi_irq_handler(int irq, void *arg) > +{ > + struct pcie_port *pp = arg; > + > + return dw_handle_msi_irq(pp); > +} > + > +static void kirin_pcie_msi_init(struct pcie_port *pp) > +{ > + dw_pcie_msi_init(pp); > +} > + > +static void kirin_pcie_enable_interrupts(struct pcie_port *pp) > +{ > + if (IS_ENABLED(CONFIG_PCI_MSI)) > + kirin_pcie_msi_init(pp); > +} > + > static int kirin_pcie_host_init(struct pcie_port *pp) > { > kirin_pcie_establish_link(pp); > + kirin_pcie_enable_interrupts(pp); > > return 0; > } > @@ -448,6 +467,25 @@ static int kirin_pcie_host_init(struct pcie_port *pp) > static int __init kirin_add_pcie_port(struct dw_pcie *pci, > struct platform_device *pdev) > { > + int ret; > + > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > + pci->pp.msi_irq = platform_get_irq(pdev, 0); > + if (!pci->pp.msi_irq) { > + dev_err(&pdev->dev, "failed to get msi irq\n"); > + return -ENODEV; > + } > + ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, > + kirin_pcie_msi_irq_handler, > + IRQF_SHARED | IRQF_NO_THREAD, > + "kirin_pcie_msi", &pci->pp); > + if (ret) { > + dev_err(&pdev->dev, "failed to request msi irq\n"); > + return ret; > + } > + } > + > + pci->pp.root_bus_nr = -1; > pci->pp.ops = &kirin_pcie_host_ops; > > return dw_pcie_host_init(&pci->pp); > -- > 1.9.1 >
Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.
[+cc Stanimir, Jianguo, Shawn for qcom, histb comments below] On Tue, May 08, 2018 at 03:03:48PM +0800, Yao Chen wrote: > This patch adds kirin pcie msi feature. Thanks for the patch! Please update your subject and changelog like this: PCI: kirin: Add MSI support Add support for MSI. You can figure out what style to use for the subject by running: git log --oneline drivers/pci/dwc/pcie-kirin.c Then follow the existing style. > Signed-off-by: Yao Chen > --- > arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 2 ++ > drivers/pci/dwc/pcie-kirin.c | 38 > +++ > 2 files changed, 40 insertions(+) > > diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > index ec3eb8e..4ef684f 100644 > --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > @@ -872,6 +872,8 @@ > 0x0 0x0200>; > num-lanes = <1>; > #interrupt-cells = <1>; > + interrupts = <0 283 4>; > + interrupts-names = "msi"; > interrupt-map-mask = <0xf800 0 0 7>; > interrupt-map = <0x0 0 0 1 >&gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>, > diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c > index d2970a0..2319c9c 100644 > --- a/drivers/pci/dwc/pcie-kirin.c > +++ b/drivers/pci/dwc/pcie-kirin.c > @@ -426,9 +426,28 @@ static int kirin_pcie_establish_link(struct pcie_port > *pp) > return 0; > } > > +static irqreturn_t kirin_pcie_msi_irq_handler(int irq, void *arg) > +{ > + struct pcie_port *pp = arg; > + > + return dw_handle_msi_irq(pp); > +} > + > +static void kirin_pcie_msi_init(struct pcie_port *pp) > +{ > + dw_pcie_msi_init(pp); > +} > + > +static void kirin_pcie_enable_interrupts(struct pcie_port *pp) > +{ > + if (IS_ENABLED(CONFIG_PCI_MSI)) > + kirin_pcie_msi_init(pp); > +} > + > static int kirin_pcie_host_init(struct pcie_port *pp) > { > kirin_pcie_establish_link(pp); > + kirin_pcie_enable_interrupts(pp); > > return 0; > } > @@ -448,6 +467,25 @@ static int kirin_pcie_host_init(struct pcie_port *pp) > static int __init kirin_add_pcie_port(struct dw_pcie *pci, > struct platform_device *pdev) > { > + int ret; > + > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > + pci->pp.msi_irq = platform_get_irq(pdev, 0); > + if (!pci->pp.msi_irq) { > + dev_err(&pdev->dev, "failed to get msi irq\n"); s/msi irq/MSI IRQ/ > + return -ENODEV; > + } > + ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, > +kirin_pcie_msi_irq_handler, > +IRQF_SHARED | IRQF_NO_THREAD, > +"kirin_pcie_msi", &pci->pp); > + if (ret) { > + dev_err(&pdev->dev, "failed to request msi irq\n"); s/msi irq/MSI IRQ/ Also, please include the IRQ number (pci->pp.msi_irq) in the message. > + return ret; > + } > + } > + > + pci->pp.root_bus_nr = -1; Setting root_bus_nr looks like an unrelated change that should be in a separate patch. But I'm not sure why you need to set root_bus_nr at all, since dw_pcie_host_init() always sets it. Some other callers of dw_pcie_host_init() do set it: exynos_add_pcie_port() imx6_add_pcie_port() armada8k_add_pcie_port() artpec6_add_pcie_port() dw_plat_add_pcie_port() histb_pcie_probe() qcom_pcie_probe() spear13xx_add_pcie_port() But I don't see *why* any of these need to set it. If they don't need to set it, they shouldn't. And it would be nice if histb and qcom followed the structure and naming conventions of the other drivers, i.e., they should have histb_add_pcie_port() and qcom_add_pcie_port(). > pci->pp.ops = &kirin_pcie_host_ops; > > return dw_pcie_host_init(&pci->pp); > -- > 1.9.1 > > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[PATCH] Kirin-PCIe: Add kirin pcie msi feature.
This patch adds kirin pcie msi feature. Signed-off-by: Yao Chen --- arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 2 ++ drivers/pci/dwc/pcie-kirin.c | 38 +++ 2 files changed, 40 insertions(+) diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi index ec3eb8e..4ef684f 100644 --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi @@ -872,6 +872,8 @@ 0x0 0x0200>; num-lanes = <1>; #interrupt-cells = <1>; + interrupts = <0 283 4>; + interrupts-names = "msi"; interrupt-map-mask = <0xf800 0 0 7>; interrupt-map = <0x0 0 0 1 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>, diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c index d2970a0..2319c9c 100644 --- a/drivers/pci/dwc/pcie-kirin.c +++ b/drivers/pci/dwc/pcie-kirin.c @@ -426,9 +426,28 @@ static int kirin_pcie_establish_link(struct pcie_port *pp) return 0; } +static irqreturn_t kirin_pcie_msi_irq_handler(int irq, void *arg) +{ + struct pcie_port *pp = arg; + + return dw_handle_msi_irq(pp); +} + +static void kirin_pcie_msi_init(struct pcie_port *pp) +{ + dw_pcie_msi_init(pp); +} + +static void kirin_pcie_enable_interrupts(struct pcie_port *pp) +{ + if (IS_ENABLED(CONFIG_PCI_MSI)) + kirin_pcie_msi_init(pp); +} + static int kirin_pcie_host_init(struct pcie_port *pp) { kirin_pcie_establish_link(pp); + kirin_pcie_enable_interrupts(pp); return 0; } @@ -448,6 +467,25 @@ static int kirin_pcie_host_init(struct pcie_port *pp) static int __init kirin_add_pcie_port(struct dw_pcie *pci, struct platform_device *pdev) { + int ret; + + if (IS_ENABLED(CONFIG_PCI_MSI)) { + pci->pp.msi_irq = platform_get_irq(pdev, 0); + if (!pci->pp.msi_irq) { + dev_err(&pdev->dev, "failed to get msi irq\n"); + return -ENODEV; + } + ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, + kirin_pcie_msi_irq_handler, + IRQF_SHARED | IRQF_NO_THREAD, + "kirin_pcie_msi", &pci->pp); + if (ret) { + dev_err(&pdev->dev, "failed to request msi irq\n"); + return ret; + } + } + + pci->pp.root_bus_nr = -1; pci->pp.ops = &kirin_pcie_host_ops; return dw_pcie_host_init(&pci->pp); -- 1.9.1