On Thu, Jan 09, 2025 at 01:35:58PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 8/1/25 07:00, Xu Yilun wrote:
> > > > > +static void __pci_ide_stream_setup(struct pci_dev *pdev, struct
> > > > > pci_ide *ide)
> > > > > +{
> > > > > + int pos;
> > > > > + u32 val;
> > > > > +
> > > > > + pos = sel_ide_offset(pdev->sel_ide_cap, ide->stream_id,
> > > > > + pdev->nr_ide_mem);
> > > > > +
> > > > > + val = FIELD_PREP(PCI_IDE_SEL_RID_1_LIMIT_MASK, ide->devid_end);
> > > > > + pci_write_config_dword(pdev, pos + PCI_IDE_SEL_RID_1, val);
> > > > > +
> > > > > + val = FIELD_PREP(PCI_IDE_SEL_RID_2_VALID, 1) |
> > > > > + FIELD_PREP(PCI_IDE_SEL_RID_2_BASE_MASK, ide->devid_start) |
> > > > > + FIELD_PREP(PCI_IDE_SEL_RID_2_SEG_MASK, ide->domain);
> > > > > + pci_write_config_dword(pdev, pos + PCI_IDE_SEL_RID_2, val);
> > > > > +
> > > > > + for (int i = 0; i < ide->nr_mem; i++) {
> > > >
> > > >
> > > > This needs to test that (pdev->nr_ide_mem >= ide->nr_mem), easy to miss
> > > > especially when PCI_IDE_SETUP_ROOT_PORT. Thanks,
> >
> > Yes, but nr_ide_mem is limited HW resource and may easily smaller than
> > device memory region number.
>
> My rootport does not have any range (instead, it relies on C-bit in MMIO
It seems strange, then how the RP decide which stream id to use.
> access to set T-bit). The device got just one (which is no use here as I
> understand).
I also have no idea from SPEC how to use the IDE register blocks on EP,
except stream ENABLE bit.
And no matter how I program the RID/ADDR association registers, it
always work...
Call for help.
>
>
> > In this case, maybe we have to merge the
> > memory regions into one big range.
>
> > > >
> > > >
> > > >
> > > > > + val = FIELD_PREP(PCI_IDE_SEL_ADDR_1_VALID, 1) |
> > > > > + FIELD_PREP(PCI_IDE_SEL_ADDR_1_BASE_LOW_MASK,
> > > > > + lower_32_bits(ide->mem[i].start) >>
> > > > > + PCI_IDE_SEL_ADDR_1_BASE_LOW_SHIFT) |
> > > > > + FIELD_PREP(PCI_IDE_SEL_ADDR_1_LIMIT_LOW_MASK,
> > > > > + lower_32_bits(ide->mem[i].end) >>
> > > > > + PCI_IDE_SEL_ADDR_1_LIMIT_LOW_SHIFT);
> > > > > + pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_1(i), val);
> > > > > +
> > > > > + val = upper_32_bits(ide->mem[i].end);
> > > > > + pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_2(i), val);
> > > > > +
> > > > > + val = upper_32_bits(ide->mem[i].start);
> > > > > + pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_3(i), val);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Establish IDE stream parameters in @pdev and, optionally, its
> > > > > root port
> > > > > + */
> > > > > +int pci_ide_stream_setup(struct pci_dev *pdev, struct pci_ide *ide,
> > > > > + enum pci_ide_flags flags)
> > > > > +{
> > > > > + struct pci_host_bridge *hb = pci_find_host_bridge(pdev->bus);
> > > > > + struct pci_dev *rp = pcie_find_root_port(pdev);
> > > > > + int mem = 0, rc;
> > > > > +
> > > > > + if (ide->stream_id < 0 || ide->stream_id > U8_MAX) {
> > > > > + pci_err(pdev, "Setup fail: Invalid stream id: %d\n",
> > > > > ide->stream_id);
> > > > > + return -ENXIO;
> > > > > + }
> > > > > +
> > > > > + if (test_and_set_bit_lock(ide->stream_id, hb->ide_stream_ids)) {
> > > > > + pci_err(pdev, "Setup fail: Busy stream id: %d\n",
> > > > > + ide->stream_id);
> > > > > + return -EBUSY;
> > > > > + }
> > > > > +
> > > > > + ide->name = kasprintf(GFP_KERNEL, "stream%d:%s", ide->stream_id,
> > > > > + dev_name(&pdev->dev));
> > > > > + if (!ide->name) {
> > > > > + rc = -ENOMEM;
> > > > > + goto err_name;
> > > > > + }
> > > > > +
> > > > > + rc = sysfs_create_link(&hb->dev.kobj, &pdev->dev.kobj, ide->name);
> > > > > + if (rc)
> > > > > + goto err_link;
> > > > > +
> > > > > + for (mem = 0; mem < ide->nr_mem; mem++)
> > > > > + if (!__request_region(&hb->ide_stream_res, ide->mem[mem].start,
> > > > > + range_len(&ide->mem[mem]), ide->name,
> > > > > + 0)) {
> > > > > + pci_err(pdev,
> > > > > + "Setup fail: stream%d: address association conflict
> > > > > [%#llx-%#llx]\n",
> > > > > + ide->stream_id, ide->mem[mem].start,
> > > > > + ide->mem[mem].end);
> > > > > +
> > > > > + rc = -EBUSY;
> > > > > + goto err;
> > > > > + }
> > > > > +
> > > > > + __pci_ide_stream_setup(pdev, ide);
> > > > > + if (flags & PCI_IDE_SETUP_ROOT_PORT)
> > > > > + __pci_ide_stream_setup(rp, ide);
> > >
> > > Oh, when we do this, the root port gets the same devid_start/end as the
> > > device which is not correct, what should be there, the rootport bdfn? Need
> >
> > "Indicates the lowest/highest value RID in the range
> > associated with this Stream ID at the IDE *Partner* Port"
> >
> > My understanding is that device should fill the RP bdfn, and the RP
> > should fill the device bdfn for RID association registers. Same for Addr
> > association registers.
>
> Oh. Yeah, this sounds right. So most of the setup needs to be done on the
> root port and not on the device (which only needs to enable the stream),
> which is not what the patch does. Or I got it wrong? Thanks,
I don't get you. This patch does IDE setup for 2 partners:
__pci_ide_stream_setup(pdev, ide); This is the setup on RP
__pci_ide_stream_setup(rp, ide); This is the setup on device
unless AMD setup IDE by firmware, and didn't set the PCI_IDE_SETUP_ROOT_PORT flag.
Thanks,
Yilun
>
> >
> > Thanks,
> > Yilun
> >
> > > to dig that but PCI_IDE_SETUP_ROOT_PORT should detect that it is a root
> > > port. Thanks,
> > >
>
> --
> Alexey
>
