| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b42e1ec7-11a0-ab6b-c552-86d204dcb041@quicinc.com>
Date: Wed, 4 Dec 2024 07:45:29 +0530
From: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: <cros-qcom-dts-watchers@...omium.org>,
Bjorn Andersson
<andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, Rob Herring
<robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>, Jingoo Han <jingoohan1@...il.com>,
"Manivannan
Sadhasivam" <manivannan.sadhasivam@...aro.org>,
Lorenzo Pieralisi
<lpieralisi@...nel.org>,
Krzysztof Wilczyński
<kw@...ux.com>,
Bjorn Helgaas <bhelgaas@...gle.com>, <quic_vbadigan@...cinc.com>,
<quic_ramkri@...cinc.com>, <quic_nitegupt@...cinc.com>,
<quic_skananth@...cinc.com>, <quic_vpernami@...cinc.com>,
<quic_mrana@...cinc.com>, <mmareddy@...cinc.com>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>
Subject: Re: [PATCH 2/3] PCI: dwc: Add ECAM support with iATU configuration
On 12/4/2024 12:25 AM, Bjorn Helgaas wrote:
> On Sun, Nov 17, 2024 at 03:30:19AM +0530, Krishna chaitanya chundru wrote:
>> The current implementation requires iATU for every configuration
>> space access which increases latency & cpu utilization.
>>
>> Configuring iATU in config shift mode enables ECAM feature to access the
>> config space, which avoids iATU configuration for every config access.
>>
>> Add "ctrl2" into struct dw_pcie_ob_atu_cfg to enable config shift mode.
>>
>> As DBI comes under config space, this avoids remapping of DBI space
>> separately. Instead, it uses the mapped config space address returned from
>> ECAM initialization. Change the order of dw_pcie_get_resources() execution
>> to acheive this.
>
> s/acheive/achieve/
>
>> Introduce new ecam_init() function op for the clients to configure after
>> ecam window creation has been done.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@...cinc.com>
>> ---
>> drivers/pci/controller/dwc/pcie-designware-host.c | 114 ++++++++++++++++++----
>> drivers/pci/controller/dwc/pcie-designware.c | 2 +-
>> drivers/pci/controller/dwc/pcie-designware.h | 6 ++
>> 3 files changed, 102 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 3e41865c7290..e98cc841a2a9 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -418,6 +418,62 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
>> }
>> }
>>
>> +static int dw_pcie_config_ecam_iatu(struct dw_pcie_rp *pp)
>> +{
>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> + struct dw_pcie_ob_atu_cfg atu = {0};
>> + struct resource_entry *bus;
>> + int ret, bus_range_max;
>> +
>> + bus = resource_list_first_type(&pp->bridge->windows, IORESOURCE_BUS);
>> +
>> + /*
>> + * Bus 1 config space needs type 0 atu configuration
>> + * Remaining buses need type 1 atu configuration
>
> s/atu/ATU/ (initialism, looks like "iATU" might be appropriate here?)
>
> I'm confused about the bus numbering; you refer to "bus 1" and "bus
> 2". Is bus 1 the root bus, i.e., the primary bus of a Root Port?
>
> The root bus number would typically be 0, not 1, and is sometimes
> programmable. I don't know how the DesignWare core works, but since
> you have "bus" here, referring to "bus 1" and "bus 2" here seems
> overly specific.
>
root bus is bus 0 and we don't need any iATU configuration for it as
its config space is accessible from the system memory, for usp port of
the switch or the direct the endpoint i.e bus 1 we need to send
Configuration Type 0 requests and for other buses we need to send
Configuration Type 1 requests this is as per PCIe spec, I will try to
include PCIe spec details in next patch.
>> + */
>> + atu.index = 0;
>> + atu.type = PCIE_ATU_TYPE_CFG0;
>> + atu.cpu_addr = pp->cfg0_base + SZ_1M;
>> + atu.size = SZ_1M;
>> + atu.ctrl2 = PCIE_ATU_CFG_SHIFT_MODE_ENABLE;
>> + ret = dw_pcie_prog_outbound_atu(pci, &atu);
>> + if (ret)
>> + return ret;
>> +
>> + bus_range_max = bus->res->end - bus->res->start + 1;
>> +
>> + /* Configure for bus 2 - bus_range_max in type 1 */
>> + atu.index = 1;
>> + atu.type = PCIE_ATU_TYPE_CFG1;
>> + atu.cpu_addr = pp->cfg0_base + SZ_2M;
>> + atu.size = (SZ_1M * (bus_range_max - 2));
>> + atu.ctrl2 = PCIE_ATU_CFG_SHIFT_MODE_ENABLE;
>> + return dw_pcie_prog_outbound_atu(pci, &atu);
>> +}
>> +
>> +static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *res)
>> +{
>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> + struct device *dev = pci->dev;
>> + struct resource_entry *bus;
>> +
>> + bus = resource_list_first_type(&pp->bridge->windows, IORESOURCE_BUS);
>> + if (!bus)
>> + return -ENODEV;
>> +
>> + pp->cfg = pci_ecam_create(dev, res, bus->res, &pci_generic_ecam_ops);
>> + if (IS_ERR(pp->cfg))
>> + return PTR_ERR(pp->cfg);
>> +
>> + pci->dbi_base = pp->cfg->win;
>> + pci->dbi_phys_addr = res->start;
>> +
>> + if (pp->ops->ecam_init)
>> + pp->ops->ecam_init(pci, pp->cfg);
>
> .ecam_init() is defined to return int, but you ignore the return value.
> If it's practical, I think it would be nicer if you could manage to:
>
> - Drop .enable_ecam.
>
> - Have .ecam_init() return failure if there's not enough ECAM space
> or whatever, i.e., move the qcom_pcie_check_ecam_support() code
> there.
>
> - Handle .ecam_init() failure here by doing whatever we did before.
>
> If there's no useful return value from .ecam_init(), make it void.
>
In controller driver we need to skip few thing if we want to enable
this feature before calling dw_pcie_host_init, better to have this way
>> + return 0;
>> +}
>
>> @@ -454,6 +499,30 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>
>> pp->bridge = bridge;
>>
>> + pp->cfg0_size = resource_size(res);
>> + pp->cfg0_base = res->start;
>> +
>> + if (!pp->enable_ecam) {
>
> If you can't get rid of .enable_ecam, reverse order so this uses
> positive logic:
>
> if (pp->enable_ecam) {
> ...
> } else {
> ...
> }
>
ack.
- Krishna Chaitanya.
>> + pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
>> + if (IS_ERR(pp->va_cfg0_base))
>> + return PTR_ERR(pp->va_cfg0_base);
>> +
>> + /* Set default bus ops */
>> + bridge->ops = &dw_pcie_ops;
>> + bridge->child_ops = &dw_child_pcie_ops;
>> + bridge->sysdata = pp;
>> + } else {
>> + ret = dw_pcie_create_ecam_window(pp, res);
>> + if (ret)
>> + return ret;
>> + bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
>> + pp->bridge->sysdata = pp->cfg;
>> + }
>
Powered by blists - more mailing lists