| 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: <pi5ouej6wae2nfjszfmeyctkavvmtycuaf7uxurfpie5x53zae@gz3ymk7laglx>
Date: Mon, 15 Sep 2025 11:19:04 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Hans Zhang <hans.zhang@...tech.com>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kw@...ux.com,
robh@...nel.org, kwilczynski@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
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 v9 07/14] PCI: cadence: Add support for High Perf
Architecture (HPA) controller
On Tue, Sep 09, 2025 at 12:41:33AM GMT, Hans Zhang wrote:
[...]
> Dear Mani,
>
> Thank you very much for your reply. This function is a new one I added
> during my collaboration with Manikandan. Here I'll reply to you. Other
> Manikandan will reply to you.
>
Please trim your replies so that your message appears at the top.
> > rp_bdf, rc_bus_nr
>
> Will change.
>
> >
> > > + u32 ecam_addr_0, region_size_0, request_id_0;
> >
> > So ECAM address is always 32bit? For all Cadence IP implementations?
>
> Sorry, it's my fault here. Since the ECAM distribution of CIX SKY1 is as
> follows:
>
> X8: 0x2c00_0000 ~ 0x3000_0000
> X4: 0x2900_0000 ~ 0x2c00_0000
> X2: 0x2600_0000 ~ 0x2900_0000
> X1_1: 0x2300_0000 ~ 0x2600_0000
> X1_0: 0x2000_0000 ~ 0x2300_0000
>
> This is only the CIX SKY1 designed in the low 32-bit address range. In fact,
> Cadence IP supports 64-bit ECAM addresses. Will change.
>
>
> >
> > > + int busnr = 0, secbus = 0, subbus = 0;
> > > + struct resource_entry *entry;
> > > + resource_size_t size;
> > > + u32 axi_address_low;
> > > + int nbits;
> > > + u64 sz;
> > > +
> > > + entry = resource_list_first_type(&bridge->windows, IORESOURCE_BUS);
> > > + if (entry) {
> > > + busnr = entry->res->start;
> > > + secbus = (busnr < 0xff) ? (busnr + 1) : 0xff;
> > > + subbus = entry->res->end;
> > > + }
> > > + size = resource_size(cfg_res);
> > > + sz = 1ULL << fls64(size - 1);
> > > + nbits = ilog2(sz);
> > > + if (nbits < 8)
> > > + nbits = 8;
> > > +
> > > + root_port_req_id_reg = ((busnr & 0xff) << 8);
> > > + pcie_bus_number_reg = ((subbus & 0xff) << 16) | ((secbus & 0xff) << 8) |
> > > + (busnr & 0xff);
> > > + ecam_addr_0 = cfg_res->start;
> >
> > Doesn't the platform require 'cfg_res->start' to be aligned to 256 MiB? The bus
> > range seem to be 0xff, so the Cadence IP allocates 8 bits for 'bus'. As per the
> > PCIe spec r6.0, sec 7.2.2, says:
> >
> > 'the base address of the range is aligned to a 2(n+20)-byte memory address
> > boundary'
> >
> > So the 'cfg_res->start' should be aligned to 2^28 byte (256 MiB) address.
> >
>
> Just as the ECAM address range mentioned above, our bus is not 0xff.
> Therefore, aligned to 2^28 byte (256 MiB) address is not required.
>
> X8: 0xc0 ~ 0xff
> X4: 0x90 ~ 0xbf
> X2: 0x60 ~ 0x8f
> X1_1: 0x30 ~ 0x5f
> X1_0: 0x00 ~ 0x2f
So n is 6 for your platforma and you need to check for 64MiB alignment. But
since this check is performed in the common code, you need to somehow pass the
'n' bits value from your csky driver.
>
> > > + region_size_0 = nbits - 1;
> > > + request_id_0 = ((busnr & 0xff) << 8);
> > > +
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> > > + CDNS_PCIE_HPA_TAG_MANAGEMENT, 0x200000);
> > > +
> > > + /* Taking slave err as OKAY */
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE, CDNS_PCIE_HPA_SLAVE_RESP,
> > > + 0x0);
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> > > + CDNS_PCIE_HPA_SLAVE_RESP + 0x4, 0x0);
> > > +
> > > + /* Program the register "i_root_port_req_id_reg" with RP's BDF */
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_IP_REG, I_ROOT_PORT_REQ_ID_REG,
> > > + root_port_req_id_reg);
> > > +
> > > + /**
> > > + * Program the register "i_pcie_bus_numbers" with Primary(RP's bus number),
> > > + * secondary and subordinate bus numbers
> > > + */
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_RP, I_PCIE_BUS_NUMBERS,
> > > + pcie_bus_number_reg);
> > > +
> > > + /* Program the register "lm_hal_sbsa_ctrl[0]" to enable the sbsa */
> > > + value = cdns_pcie_hpa_readl(pcie, REG_BANK_IP_REG, LM_HAL_SBSA_CTRL);
> > > + value |= BIT(0);
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_IP_REG, LM_HAL_SBSA_CTRL, value);
> > > +
> > > + /* Program region[0] for ECAM */
> > > + axi_address_low = (ecam_addr_0 & 0xfff00000) | region_size_0;
> >
> > Could you explain what is getting programmed and why?
>
> Cadence IP register description:
>
> Region Base Address [31:8] (24 bits)
> Lower [31:8] bits of the AXI region base address.
>
> Region Size (6 bits)
> The programme value in this field + 1 gives the region size. Minimum value
> to be programmed into this field is 7 as the lower 8 bits of the AXI region
> base address are replaced by zeros by the region select logic. Minumum
> supported region size is 256 bytes.
>
Add some of this info in the comments.
>
> The original intention of ecam_addr_0 & 0xfff00000 is to align with 1MB.
> This is clearly redundant, as one Bus alone requires a 1MB ECAM range.
>
You need to check for alignment at the start as I mentioned above.
> Will delete & 0xfff00000.
>
> Here it refers to CPU address.
>
> >
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> > > + CDNS_PCIE_HPA_AT_OB_REGION_CPU_ADDR0(0),
> > > + axi_address_low);
> > > +
> > > + /* rc0-high-axi-address */
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> > > + CDNS_PCIE_HPA_AT_OB_REGION_CPU_ADDR1(0), 0x0);
> > > + /* Type-1 CFG */
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> > > + CDNS_PCIE_HPA_AT_OB_REGION_DESC0(0), 0x05000000);
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> > > + CDNS_PCIE_HPA_AT_OB_REGION_DESC1(0),
> > > + (request_id_0 << 16));
> > > +
> > > + /* All AXI bits pass through PCIe */
> > > + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> > > + CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0(0), 0x1b);
> > > + /* PCIe address-high */
> >
> > What is this address?
>
> Cadence IP register description:
>
> AXI to PCIe Address Translation
> AXI Slave to PCIe Address Translation registers. Provides bits 31:8 of the
> PCIe address and the number of AXI address bits passed through.
>
> The annotations here will be changed to: /* AXI to PCIe Address Translation
> */
Okay.
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists