Re: [PATCH v3 17/17] PCI: dwc: Add Baikal-T1 PCIe controller support

[Serge Semin]

Bjorn,
Do you have anything to say based on the notes below?

-Sergey

On Wed, Jun 22, 2022 at 08:04:37PM +0300, Serge Semin wrote:
> On Tue, Jun 21, 2022 at 01:29:41PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jun 20, 2022 at 08:13:47PM +0300, Serge Semin wrote:
> > > On Wed, Jun 15, 2022 at 11:48:48AM -0500, Bjorn Helgaas wrote:
> > > > On Fri, Jun 10, 2022 at 11:57:05AM +0300, Serge Semin wrote:
> > > > > Baikal-T1 SoC is equipped with DWC PCIe v4.60a host controller. It can be
> > > > > trained to work up to Gen.3 speed over up to x4 lanes. The host controller
> > > > > is attached to the DW PCIe 3.0 PCS via the PIPE-4 interface, which in its
> > > > > turn is connected to the DWC 10G PHY. The whole system is supposed to be
> > > > > fed up with four clock sources: DBI peripheral clock, AXI application
> > > > > clocks and external PHY/core reference clock generating the 100MHz signal.
> > > > > In addition to that the platform provide a way to reset each part of the
> > > > > controller: sticky/non-sticky bits, host controller core, PIPE interface,
> > > > > PCS/PHY and Hot/Power reset signal. The driver also provides a way to
> > > > > handle the GPIO-based PERST# signal.
> > > > > 
> > > > > Note due to the Baikal-T1 MMIO peculiarity we have to implement the DBI
> > > > > interface accessors which make sure the IO operations are dword-aligned.
> > 
> > > > > +struct dw_pcie_ops bt1_pcie_dw_ops = {
> > > > > +	.read_dbi = bt1_pcie_read_dbi,
> > > > > +	.write_dbi = bt1_pcie_write_dbi,
> > > > > +	.write_dbi2 = bt1_pcie_write_dbi2,
> > > > > +	.start_link = bt1_pcie_start_ltssm,
> > > > > +	.stop_link = bt1_pcie_stop_ltssm,
> > > > > +};
> > 
> > > > Please rename to "dw_pcie_ops" as most
> > > > drivers use. 
> > > 
> > > IMO matching the structure and its instance names is not a good idea.
> > > Other than confusing objects nature, at the very least it forces you to
> > > violate the local namespace convention. Thus in the line of the
> > > dw_pcie->ops initialization it looks like you use some generic
> > > operations while in fact you just refer to the locally defined
> > > DW PCIe ops instance with the generic variable name. Moreover AFAICS
> > > the latest platform drivers mainly use the vendor-specific prefix in
> > > the dw_pcie_ops structure instance including the ones acked by you,
> > > Lorenzo and Gustavo. What makes my code any different from them?
> > 
> 
> > That's fair.  I suggest "bt1_pcie_ops" or "bt1_dw_pcie_ops" to match
> > the other drivers that include the driver name:
> > 
> >   intel_pcie_ops
> >   keembay_pcie_ops
> >   kirin_dw_pcie_ops
> >   tegra_dw_pcie_ops
> 
> +   ks_pcie_dw_pcie_ops
> 
> which is even further from the suggested names.)
> 
> > 
> > They're not 100% consistent, but hopefully we can at least not make
> > things *less* consistent.
> 
> I don't think we can make something less consistent if there is no real
> consistency.) There are at most five ops descriptors can be defined in
> the DW PCIe platform drivers:
> 
> 1. struct dw_pcie_ops - DW PCIe DBI interface accessors,
> +-> dw_pcie_ops
> +-> <vendor>_pcie_ops
> +-> <vendor>_dw_pcie_ops
> 
> 2. struct pci_ops     - own or child PCIe config space accessors,
> +-> dw_pcie_ops !!! in the driver core.
> +-> <vendor>_pci_ops
> +-> <vendor>_pcie_ops
> +-> dw_child_pcie_ops
> +-> <vendor>_child_pcie_ops
> +-> <vendor>_child_pci_ops
> 
> 3. struct dw_pcie_host_ops - DW PCIe Root Port init/de-init operations
> +-> <vendor>_pcie_host_ops
> +-> <vendor>_pcie_dw_host_ops
> 
> 4. struct dw_pcie_ep_ops   - DW PCIe Endpoint init/de-init operations
> +-> pcie_ep_ops
> +-> pci_ep_ops
> +-> <vendor>_pcie_ep_ops
> 
> As you can see each can have different naming approaches used in the
> DW PCIe platform drivers here and there. Some of them have been utilized
> more frequently, some of them - less. As for me what is really consistent
> across all the DW PCIe platform drivers is the local namespace prefix
> of the form "<vendor>_pcie". It is used in all the locally defined
> functions names and more-or-less mainly in the local instances of the
> operation descriptors. So if you want we can pick some approach and
> make sure it is used in all the driver from now on. For instance,
> 
> struct dw_pcie_ops <vendor>_pcie_ops
> struct dw_pcie_host_ops <vendor>_pcie_host_ops
> struct dw_pcie_ep_ops <vendor>_pcie_ep_ops
> struct pci_ops <vendor>_pci_ops // Can be confused with the struct
>                                 // dw_pcie_ops instance, but this what
>                                 // is mainly used in the available drivers.
> struct pci_ops <vendor>_child_pci_ops // less frequent naming
>                                       // approach, but it looks more
>                                       // like the own CFG-space IOs.
> 
> Note the later two cases will violate the local namespace naming
> convention of having "<vendor>_pcie" prefix.
> 
> In my case the names would look like:
> struct dw_pcie_ops bt1_pcie_ops // What you suggest in the comment above
> struct dw_pcie_host_ops bt1_pcie_host_ops
> struct pci_ops bt1_pci_ops // It may look ambiguous with bt1_pcie_ops.
> 
> What do you think?
> 
> > 
> > > > > +static int bt1_pcie_get_res(struct bt1_pcie *btpci)
> > > 
> > > > Can you name this something similar to what other drivers use?  There
> > > > are a couple *_pcie_get_resources() functions (normally called from
> > > > *_pcie_probe()), but no *_get_res() yet.
> > > 
> > > Earlier in this patchset I've introduced a new method to get
> > > the CSRs ranges, PCIe speed, NoF lanes, etc resources. See the patch:
> > > [PATCH v3 14/17] PCI: dwc: Introduce generic resources getter
> > > The method has been named as "dw_pcie_get_res()". So the locally
> > > defined function has been named to refer to that method. If you think
> > > that using the "_resources" suffix is better (IMO there is no
> > > significant difference) then we'll need to change the name there too.
> > > Do you?
> > 
> 
> > Yes.  I don't think there's value in names being gratuitously
> > different.
> 
> Ok.
> 
> > 
> > > > > +	/* AXI-interface is configured with 64-bit address bus width */
> > > > > +	ret = dma_coerce_mask_and_coherent(&btpci->dw.pp.bridge->dev,
> > > > > +					   DMA_BIT_MASK(64));
> > > 
> > > > Just to double-check since this is the first instance of
> > > > dma_coerce_mask_and_coherent() in drivers/pci -- I guess Baikal-T1 is
> > > > unique in needing this?
> > > 
> > > To be honest I've set it here just in case, seeing the dma_mask and
> > > coherent_dma_mask are left uninitialized in the Host bridge device
> > > instance, while it's still participate in the PCI devices hierarchy:
> > > 
> > > 1. platform_device.dev;
> > >                    | (<= devm_pci_alloc_host_bridge(dev))
> > >                    +---+
> > >                       &v
> > > 2. pci_host_bridge.dev.parent
> > >                    | (<= pci_register_host_bridge(bridge) or)
> > >                    | (<= pci_alloc_child_bus()              )
> > >                   &v
> > >            pci_bus.bridge
> > >                    +-------------------+
> > >                    |                   | (<= pci_setup_device())
> > >                    v                   v
> > > 3.     pci_bus.dev.parent  pci_dev.dev.parent
> > >                            pci_dev.dma_mask = 0xffffffff;
> > >                                    | (<= pci_device_add())
> > >                                    +----+
> > >                                        &v
> > >                            pci_dev->dev.dma_mask
> > >                            pci_dev->dev.coherent_dma_mask = 0xffffffffull;
> > > 
> > > So each device detected on the very first PCIe bus gets to have the
> > > PCI host bridge device as a parent. But AFAICS the PCI subsystem core
> > > code doesn't use the PCI host bridge DMA-mask and by default the
> > > dma_mask/coherent_dma_mask fields of each PCIe peripheral device are
> > > unconditionally initialized with DMA_BIT_MASK(32) (they are supposed
> > > to be overridden by the device-driver anyway). So to speak we can
> > > freely drop the dma_coerce_mask_and_coherent() method invocation from
> > > my driver if you say it is required and the PCI host bridge DMA parameter
> > > will never be used. What do you think?
> > 
> 
> > I'd like the usage across drivers to be consistent unless there's a
> > hardware difference that requires something different.  So if you can
> > point to something different in bt1, great.  If not, do it the same as
> > the other drivers.
> 
> Ok. I'll drop it from the driver then.
> 
> > 
> > > > > +static void bt1_pcie_full_stop_bus(struct bt1_pcie *btpci, bool init)
> > > > 
> > > > Can you name this something similar to what other drivers use?
> > > 
> > > For instance? (Please note, the link_stop/link_start callbacks are
> > > defined as separate methods above.) The current names correctly describe
> > > the methods logic. So I wouldn't want to fully change their names.
> > 
> 
> > Do any other drivers contain similar logic?  If so, please use a
> > similar name.
> 
> host_init content is very platform-specific. So each driver has its own
> callback implementation and logical sub-methods split up. My case
> isn't an exception.
> 
> > 
> > > > > +	 * Application reset controls are trigger-based so de-assert the core
> > > > > +	 * resets only.
> > > > > +	 */
> > > > > +	ret = reset_control_bulk_assert(DW_PCIE_NUM_CORE_RSTS, pci->core_rsts);
> > 
> 
> > BTW, the comment says "de-assert" but the code looks like "assert".
> 
> Right. It is supposed to be "assert" in accordance with what is
> actually done.
> 
> > 
> > > > > +	/* Make sure the reset is settled */
> > > > > +	usleep_range(1, 10);
> > > 
> > > > Is this duration related to something in the PCIe spec?  Or the DWC
> > > > spec? 
> > > 
> > > No. These durations are the chip-specific. Partly due to them being
> > > specific for each SoC we can't implement a generic bus reset
> > > procedure.
> > > 
> > > > I'd really like to use named constants when possible, although
> > > > we have a ton of bare magic numbers currently.
> > > > 
> > > > Similar for the poll timeouts and the "state settled" sleep below.
> > > 
> > > I don't really see much need in this parametrization since these
> > > numbers are used only once in the platform driver and their
> > > application is easily inferable from the code context.
> > 
> 
> > Even if they are used only once, it's helpful when constants like this
> > can be connected to the spec or other justification for the specific
> > values.
> 
> Ok. I'll replace the literals with the macros.
> 
> > 
> > > > > +static struct bt1_pcie *bt1_pcie_create_data(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct bt1_pcie *btpci;
> > > > > +
> > > > > +	btpci = devm_kzalloc(&pdev->dev, sizeof(*btpci), GFP_KERNEL);
> > > > > +	if (!btpci)
> > > > > +		return ERR_PTR(-ENOMEM);
> > > > > +
> > > > > +	btpci->pdev = pdev;
> > > > > +
> > > > > +	platform_set_drvdata(pdev, btpci);
> > > > > +
> > > > > +	return btpci;
> > > 
> > > > I don't think it's worth splitting this into a separate function.  I
> > > > think it would be better to use the same structure as other dwc-based
> > > > drivers and keep this in bt1_pcie_probe().
> > > 
> > > Sorry, I disagree in this matter. Generally I don't like the most of
> > > the probe methods designed in the kernel well because after evolving
> > > in time they get to be a mess if incoherent initializations,
> > > allocations, requests, etc. Splitting it up into a set of smaller
> > > coherent methods makes the code much clearer.
> > 
> 
> > There's definitely some tension between making one driver better and
> > making it different from all the others.  I'm all in favor of making
> > all the drivers better and more consistent.  I'm less in favor of
> > one-off improvements because consistency is extremely important for
> > maintenance.
> 
> Well, if there were a consistency in the probe method design it would
> have been another story, but in case of the DW PCIe there is none.
> Some DW PCIe platform drivers perform all the probe actions right in
> the probe method forming a long list of the weakly coherent things,
> some of them have a few sub-functions called but still look the same,
> some of them are mainly split up in the sub-methods, but still perform
> some initialization right in the probe method. So in general there is
> no unification and well defined convention in that regard.
> 
> My approach on the contrary makes the probe method code well unified.
> Should any new additional step is required, the new method can be
> added together with the cleanup antagonist. Similarly the
> sub-methods update patches is easier to review than reading the
> all-in-one probe code update. Moreover such design approach I've
> been using in all the drivers submitted by me to the kernel and no
> questions have been raised so far. Finally the driver is supposed to
> be maintained by its author at the very least. So I definitely won't
> have any problem with it especially after using the same design
> pattern in all my drivers.
> 
> > 
> > > > > +static int bt1_pcie_add_dw_port(struct bt1_pcie *btpci)
> > > 
> > > > All other dwc-based drivers call dw_pcie_host_init() from either
> > > > *_pcie_probe() or *_add_pcie_port().  Please use a similar convention.
> > > 
> > > Not entirely. Tegra is an exception. So as before I don't think there
> > > is a real convention. Most likely it's a result of a lucky coincident.
> > > Moreover I don't really like such naming. Something like
> > > VENDOR_pcie_add_root_port() would be much better.
> > 
> 
> > I stand corrected.  Of the 21 dw_pcie_host_init() calls, 13 are from
> > *_pcie_probe(), 7 are from *_add_pcie_port(), and tegra is from
> > tegra_pcie_init_controller().  I think the *_add_pcie_port() structure
> > is better because it makes room to support multiple root ports.
> 
> See the last comment. It concerns the same methods, but you suggest to
> use the names "*_add_port()" there.
> 
> > 
> > > > > +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> > > > 
> > > > Why do you need this when no other dwc-based drivers do?  Is Baikal-T1
> > > > different in this respect?
> > > 
> > > It's because eDMA engine embedded into the DW PCIe root port. 
> > 
> 
> > Let's add a comment about the fact that this is needed because of the
> > eDMA engine.
> 
> Ok.
> 
> > 
> > > > > +static void bt1_pcie_del_dw_port(struct bt1_pcie *btpci)
> > > 
> > > > Can you call dw_pcie_host_deinit() from the same place as other
> > > > drivers?
> > > > 
> > > >   $ git grep -p dw_pcie_host_deinit drivers/pci/controller/dwc
> > > 
> > > Sorry I'd rather leave it as is. There are only four drivers using
> > > it and one of them don't follow what seems like a convention. I'd
> > > rather have my driver code coherent:
> > > bt1_pcie_add_pcie_port() is used to add the DW PCIe Root Port.
> > > and
> > > bt1_pcie_del_pcie_port() is used to remove the DW PCIe Root Port
> > 
> > I agree with your rationale.  Intel and kirin do:
> > 
> >   *_pcie_probe
> >     dw_pcie_host_init
> > 
> >   *_pcie_remove
> >     dw_pcie_host_deinit
> > 
> > and tegra is similar but from tegra_pcie_init_controller() and
> > tegra_pcie_deinit_controller().  Exynos is the odd one out and calls
> > dw_pcie_host_init() from exynos_add_pcie_port() but
> > dw_pcie_host_deinit() from exynos_pcie_remove().
> > 
> > Your model is better since it removes the "single root port"
> > assumption.  I would like the "bt1_pcie_add_port()" and
> > "bt1_pcie_del_port()" (or "bt1_pcie_remove_port()", which would be
> > slightly more parallel with "add") names to align with other drivers.
> 
> Ok. I'll use bt1_pcie_add_port() and bt1_pcie_del_port() names then.
> * Note the DW PCIe platform drivers mainly use the _pcie_port() suffix
> * in the add-method.
> 
> -Sergey
> 
> > 
> > Bjorn