Possible issue at the kirin-pcie driver

Possible issue at the kirin-pcie driver

[Mauro Carvalho Chehab]

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 suspect that it is (1), but, as I don't have any datasheets or
register map, I can't tell for sure.

Could someone with access to the datahseets shed the light?

Thanks,
Mauro

Re: Possible issue at the kirin-pcie driver

[Manivannan Sadhasivam]

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

Re: Possible issue at the kirin-pcie driver

[Mauro Carvalho Chehab]

Em Wed, 7 Jul 2021 16:24:25 +0530
Manivannan Sadhasivam <mani@kernel.org> escreveu:

> 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.

Yeah, it is possible that the register is duplicated on different
regions.

> > 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.

Looking at discuss.96boards.org, I saw some comments that seem
to indicate that the M.2 slot with NVMe devices is not reliable:
it sounds that it works with some models only, but this could
be due to an old PCIe driver, or to some other problem.

Hard to tell without the datasheets. I'll need to wait for a
NVMe card to arrive here in order to test. It will take a couple
of weeks.

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

I'm working right now on moving Hikey 970 to PHY as well. It seems
it is doable, although it is trickier, as both the PHY and the PCIe
driver need to access the APB memory region[1]. I'll probably need to
use a named regmap, in order to allow both to access the same
memory region.

[1] kirin970_pcie_natural_cfg() needs that at the PHY driver, while
    kirin_pcie_read_dbi() and kirin_pcie_write_dbi() needs it at the
    PCIe side.

Thanks,
Mauro