| 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: <c05f3e8b-b205-4d6b-8423-23edbfcc9f2b@cixtech.com>
Date: Thu, 14 Aug 2025 09:29:45 +0800
From: Hans Zhang <hans.zhang@...tech.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, bhelgaas@...gle.com,
lpieralisi@...nel.org, kw@...ux.com, mani@...nel.org, robh@...nel.org,
kwilczynski@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Cc: mpillai@...ence.com, fugang.duan@...tech.com, guoyin.chen@...tech.com,
peter.chen@...tech.com, cix-kernel-upstream@...tech.com,
linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 10/13] PCI: sky1: Add PCIe host support for CIX Sky1
On 2025/8/14 03:16, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
>
> On 13/08/2025 06:23, hans.zhang@...tech.com wrote:
>> +static int sky1_pcie_parse_mem(struct sky1_pcie *pcie)
>> +{
>> + struct device *dev = pcie->dev;
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct resource *res;
>> + void __iomem *base;
>> + int ret = 0;
>> +
>> + base = devm_platform_ioremap_resource_byname(pdev, "reg");
>> + if (IS_ERR(base)) {
>> + dev_err(dev, "Parse \"reg\" resource err\n");
>
> Syntax is return dev_err_probe, and without \" so grepping works
> correctly (see coding style).
Dear Krzysztof,
Thank you very much for your reply. Will change.
>
>> + return PTR_ERR(base);
>> + }
>> + pcie->reg_base = base;
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
>> + if (!res) {
>> + dev_err(dev, "Parse \"cfg\" resource err\n");
>> + return -ENXIO;
>> + }
>> + pcie->cfg_res = res;
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rcsu");
>> + if (!res) {
>> + dev_err(dev, "Parse \"rcsu\" resource err\n");
>> + return -ENXIO;
>> + }
>> + pcie->rcsu_base = devm_ioremap(dev, res->start, resource_size(res));
>
> Why aren't you using wrapper over get_resource and ioremap? Isn't
> devm_platform_ioremap_resource_byname exactly what you want?
>
Thank you for the reminder. Will change.
> And if argument from previous versions was - you need to backport it for
> ancient 3.10 kernel - then it would be a no go.
Ok, will change.
>
>> + if (!pcie->rcsu_base) {
>> + dev_err(dev, "ioremap failed for resource %pR\n", res);
>> + return -ENOMEM;
>> + }
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "msg");
>> + if (!res) {
>> + dev_err(dev, "Parse \"msg\" resource err\n");
>> + return -ENXIO;
>> + }
>> + pcie->msg_res = res;
>> + pcie->msg_base = devm_ioremap(dev, res->start, resource_size(res));
>> + if (!pcie->msg_base) {
>> + dev_err(dev, "ioremap failed for resource %pR\n", res);
>> + return -ENOMEM;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int sky1_pcie_parse_property(struct platform_device *pdev,
>> + struct sky1_pcie *pcie)
>> +{
>> + int ret = 0;
>> +
>> + ret = sky1_pcie_parse_mem(pcie);
>> + if (ret < 0)
>> + return ret;
>> +
>> + sky1_pcie_init_bases(pcie);
>> +
>> + return ret;
>> +}
>> +
>> +static int sky1_pcie_start_link(struct cdns_pcie *cdns_pcie)
>> +{
>> + struct sky1_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
>> +
>> + sky1_pcie_clear_and_set_dword(pcie->strap_base + STRAP_REG(1),
>> + 0, LINK_TRAINING_ENABLE);
>> +
>> + return 0;
>> +}
>> +
>> +static void sky1_pcie_stop_link(struct cdns_pcie *cdns_pcie)
>> +{
>> + struct sky1_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
>> +
>> + sky1_pcie_clear_and_set_dword(pcie->strap_base + STRAP_REG(1),
>> + LINK_TRAINING_ENABLE, 0);
>> +}
>> +
>> +
>> +static bool sky1_pcie_link_up(struct cdns_pcie *cdns_pcie)
>> +{
>> + u32 val;
>> +
>> + val = cdns_pcie_hpa_readl(cdns_pcie, REG_BANK_IP_REG,
>> + IP_REG_I_DBG_STS_0);
>> + return val & LINK_COMPLETE;
>> +}
>> +
>> +static const struct cdns_pcie_ops sky1_pcie_ops = {
>> + .start_link = sky1_pcie_start_link,
>> + .stop_link = sky1_pcie_stop_link,
>> + .link_up = sky1_pcie_link_up,
>> +};
>> +
>> +static int sky1_pcie_probe(struct platform_device *pdev)
>> +{
>> + const struct sky1_pcie_data *data;
>> + struct device *dev = &pdev->dev;
>> + struct pci_host_bridge *bridge;
>> + struct cdns_pcie *cdns_pcie;
>> + struct resource_entry *bus;
>> + struct cdns_pcie_rc *rc;
>> + struct sky1_pcie *pcie;
>> + int ret;
>> +
>> + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>> + if (!pcie)
>> + return -ENOMEM;
>> +
>> + data = of_device_get_match_data(dev);
>> + if (!data)
>> + return -EINVAL;
>> +
>> + pcie->data = data;
>> + pcie->dev = dev;
>> + dev_set_drvdata(dev, pcie);
>> +
>> + bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
>> + if (!bridge)
>> + return -ENOMEM;
>> +
>> + bus = resource_list_first_type(&bridge->windows, IORESOURCE_BUS);
>> + if (!bus)
>> + return -ENODEV;
>> +
>> + ret = sky1_pcie_parse_property(pdev, pcie);
>> + if (ret < 0)
>> + return -ENXIO;
>> +
>> + pcie->cfg = pci_ecam_create(dev, pcie->cfg_res, bus->res,
>> + &pci_generic_ecam_ops);
>> + if (IS_ERR(pcie->cfg))
>> + return PTR_ERR(pcie->cfg);
>> +
>> + bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
>> + rc = pci_host_bridge_priv(bridge);
>> + rc->ecam_support_flag = 1;
>> + rc->cfg_base = pcie->cfg->win;
>> + rc->cfg_res = &pcie->cfg->res;
>> +
>> + cdns_pcie = &rc->pcie;
>> + cdns_pcie->dev = dev;
>> + cdns_pcie->ops = &sky1_pcie_ops;
>> + cdns_pcie->reg_base = pcie->reg_base;
>> + cdns_pcie->msg_res = pcie->msg_res;
>> + cdns_pcie->cdns_pcie_reg_offsets = &data->reg_off;
>> + cdns_pcie->is_rc = data->reg_off.is_rc;
>> +
>> + pcie->cdns_pcie = cdns_pcie;
>> + pcie->cdns_pcie_rc = rc;
>> + pcie->cfg_base = rc->cfg_base;
>> + bridge->sysdata = pcie->cfg;
>> +
>> + if (data->soc_type == CIX_SKY1) {
>
>
> Dead code or rather if (true) code. Don't do it, it's more difficult to
> read.
Will remove.
>
>> + rc->vendor_id = PCI_VENDOR_ID_CIX;
>> + rc->device_id = PCI_DEVICE_ID_CIX_SKY1;
>> + rc->no_inbound_flag = 1;
>> + }
>> +
>> + ret = cdns_pcie_hpa_host_setup(rc);
>> + if (ret < 0) {
>> + pci_ecam_free(pcie->cfg);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct sky1_pcie_data sky1_pcie_rc_data = {
>> + .reg_off = {
>> + .is_rc = true,
>> + .ip_reg_bank_offset = SKY1_IP_REG_BANK_OFFSET,
>> + .ip_cfg_ctrl_reg_offset = SKY1_IP_CFG_CTRL_REG_BANK_OFFSET,
>> + .axi_mstr_common_offset = SKY1_IP_AXI_MASTER_COMMON_OFFSET,
>> + .axi_slave_offset = SKY1_AXI_SLAVE_OFFSET,
>> + .axi_master_offset = SKY1_AXI_MASTER_OFFSET,
>> + .axi_hls_offset = SKY1_AXI_HLS_REGISTERS_OFFSET,
>> + .axi_ras_offset = SKY1_AXI_RAS_REGISTERS_OFFSET,
>> + .axi_dti_offset = SKY1_DTI_REGISTERS_OFFSET,
>> + },
>> + .soc_type = CIX_SKY1,
>
> You have only one device variant, so this entire pcie_data feels redundant.
>
Will remove.
>> +};
>> +
>> +static const struct of_device_id of_sky1_pcie_match[] = {
>> + {
>> + .compatible = "cix,sky1-pcie-host",
>> + .data = &sky1_pcie_rc_data,
>> + },
>> + {},
>
Best regards,
Hans
Powered by blists - more mailing lists