Re: [PATCH v2 1/5] dt-bindings: PCI: Add STM32MP25 PCIe root complex bindings

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Dec 18, 2024 at 09:42:45AM +0100, Christian Bruel wrote:
> 
> 
> On 12/17/24 18:25, Manivannan Sadhasivam wrote:
> > On Tue, Dec 17, 2024 at 04:53:48PM +0100, Christian Bruel wrote:
> > > 
> > > > Makes sense.  What about phys, resets, etc?  I'm pretty sure a PHY
> > > > would be a per-Root Port thing, and some resets and wakeup signals
> > > > also.
> > > > 
> > > > For new drivers, I think we should start adding Root Port stanzas to
> > > > specifically associate those things with the Root Port, e.g.,
> > > > something like this?
> > > > 
> > > >     pcie@48400000 {
> > > >       compatible = "st,stm32mp25-pcie-rc";
> > > > 
> > > >       pcie@0,0 {
> > > >         reg = <0x0000 0 0 0 0>;
> > > >         phys = <&combophy PHY_TYPE_PCIE>;
> > > >         phy-names = "pcie-phy";
> > > >       };
> > > >     };
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml?id=v6.12#n111
> > > > is one binding that does this, others include apple,pcie.yaml,
> > > > brcm,stb-pcie.yaml, hisilicon,kirin-pcie.yaml.
> > > > 
> > > 
> > > On a second thought, moving the PHY to the root-port part would introduce a
> > > discrepancy with the pcie_ep binding, whereas the PHY is required on the
> > > pcie_ep node.
> > > 
> > > Even for the pcie_rc, the PHY is needed to enable the core_clk to access
> > > the PCIe core registers,
> > > 
> > 
> > But why that matters? You can still parse the child nodes, enable PHY and
> > configure PCIe registers. >
> > > So that would make 2 different required PHY locations for RC and EP:
> > > 
> > >      pcie_rc: pcie@48400000 {
> > >        compatible = "st,stm32mp25-pcie-rc";
> > > 
> > >        pcie@0,0 {
> > >          reg = <0x0000 0 0 0 0>;
> > >          phys = <&combophy PHY_TYPE_PCIE>;
> > >          phy-names = "pcie-phy";
> > >        };
> > >      };
> > > 
> > >      pcie_ep pcie@48400000 {
> > >        compatible = "st,stm32mp25-pcie-ep";
> > >        phys = <&combophy PHY_TYPE_PCIE>;
> > >        phy-names = "pcie-phy";
> > >      };
> > > 
> > > Simplest seems to keep the PHY required for the pcie core regardless of the
> > > mode and keep the empty root port to split the design
> > > 
> > 
> > No please. Try to do the right thing from the start itself.
> 
> Parsing the child node to clock the IP seems weird. Note that
> hisilicon,kirin-pcie.yaml also declares the PHY at the controller level.
> 

Nothing is weird here. Almost all multi port controller drivers does the same.
Most of the single port controller instances define port properties in
controller node only, but that's what we want to avoid now.

- Mani

-- 
மணிவண்ணன் சதாசிவம்