Re: Possible issue at the kirin-pcie driver

[Manivannan Sadhasivam <mani@kernel.org>]

Hi Mauro,

On Tue, Jul 06, 2021 at 11:35:03AM +0200, Mauro Carvalho Chehab wrote:
> Hi,
> 
> I was asked by Rob Herring to convert the kiring-pcie driver on two parts,
> splitting the PHY logic from it, in order to be able to add PHY support 
> for Hikey 970 at drivers/pci/controller/dwc/pcie-kirin.c.
> 
> While doing so, I noticed something weird issue at the driver, with regards
> to a certain register (PCIE_APB_PHY_STATUS0), as shown below:
> 
> ...
> 
> 	#define PCIE_APB_PHY_STATUS0	0x400
> ...
> 	static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
> 	{
> 		return readl(kirin_pcie->apb_base + reg);
> 	}
> ...
> 	static inline u32 kirin_apb_phy_readl(struct kirin_pcie *kirin_pcie, u32 reg)
> 	{
> 		return readl(kirin_pcie->phy_base + reg);
> 	}
> ...
> 	static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie)
> 	{
> ...
> 		reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
> 		if (reg_val & PIPE_CLK_STABLE) {
>                 	dev_err(dev, "PIPE clk is not stable\n");
> 			return -EINVAL;
> 		}
> 	}
> ...
> 	static int kirin_pcie_link_up(struct dw_pcie *pci)
> 	{
> 		struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
> 		u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
> 	
> 		if ((val & PCIE_LINKUP_ENABLE) == PCIE_LINKUP_ENABLE)
> 			return 1;
> 
> 		return 0;
> 
> 		u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
> 
> 		if ((val & PCIE_LINKUP_ENABLE) == PCIE_LINKUP_ENABLE)
> 			return 1;
> 
> Basically, the code at kirin_pcie_phy_init() use this register as if it is 
> part of the PHY memory region (0xf3f20000 + 0x400), while the code at 
> kirin_pcie_link_up() considers is as belonging to the APB memory
> region (0xff3fe000 + 0x400).
> 
> It sounds to me that there's a mistake somewhere. I mean, either:
> 
> 1. there is a cut-and-paste error, caused it to access the wrong memory
>    region, e.g. at kirin_pcie_link_up() the logic should be:
> 
> 	u32 val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
> 
>    instead of:
> 
> 	u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
> 
>    (or the reverse)
> 
> 2. Both memory regions have a register at address 0x400 with similar
>    names that ended being merged into the same macro;
> 
> 3. the register for APB PHY status0 is duplicated on both regions and,
>    on both, they are at region_base + 0x400.
>

I don't have the datasheet for Kirin970 but...

I think 2 & 3 are the possible ones as I've seen register duplications
across multiple vendors.

> I suspect that it is (1), but, as I don't have any datasheets or
> register map, I can't tell for sure.
> 

If it is 1, then I don't think the driver can work reliably.

Anyway, I think you can still move forward with the splitting provided
that you can access this register in both drivers.

Thanks,
Mani