14:59 <friedy> Hi guys, I'm doing some work with the image sensor processor on the RK3399 soc in Linux. I'm trying to access the control registers for the ISP, so I mapped in the registers with ioremap like normal. But for some reason when I try to read from that region of memory readl() doesn't return and the system hangs. I've never had this happen to me

14:59 <friedy> before, so I don't know why this is happening.

15:00 <friedy> On RK3399 ISP0 is at 0xFF910000. I've mapped this into the kernel's address space, but I still can't read from this region of memory.

15:02 <jakllsch> are the clocks running?

15:05 <friedy> I believe so. I set the clock rates using CRU_CLKSEL_CON53 and enabled the clocks with CRU_CLKGATE_CON12 registers.

15:06 <friedy> Should just be hclk_isp0 and aclk_isp0 correct?

15:07 <robmur01> and you've set all the rates and muxes appropriately all the way up the clock tree? This is why we have clock drivers and properly-abstracted driver APIs ;)

15:08 <robmur01> same for power domains, which is another thing you'll probably need to cover

15:11 <friedy> Do you have an example of this?

15:11 <friedy>     isp0: isp0@ff910000 {

15:11 <friedy>         compatible = "rockchip,rk3399-cif-isp";

15:11 <friedy>         reg = <0x0 0xff910000 0x0 0x4000>;

15:11 <friedy>         interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>;

15:11 <friedy>         clocks = <&cru SCLK_ISP0>,

15:11 <friedy>              <&cru ACLK_ISP0_WRAPPER>,

15:11 <friedy>              <&cru HCLK_ISP0_WRAPPER>;

15:11 <friedy>         clock-names = "isp", "aclk", "hclk";

15:11 <friedy>         iommus = <&isp0_mmu>;

15:11 <friedy>         phys = <&mipi_dphy_rx0>;

15:11 <friedy>         phy-names = "dphy";

15:11 <friedy>         power-domains = <&power RK3399_PD_ISP0>;

15:11 <friedy>         status = "disabled";

15:11 <friedy>         ports {

15:11 <friedy>             #address-cells = <1>;

15:11 <friedy>             #size-cells = <0>;

15:11 <friedy>             port@0 {

15:11 <friedy>                 reg = <0>;

15:11 <CounterPillow> usually you just clk_prepare_enable the right clocks instead of manually writing the registers.

15:11 <CounterPillow> also please don't multiline paste in IRC

15:12 <friedy> I tried that in the beginning but I still had the same problem.

15:12 <friedy> I parsed the device node and use clk_prepare_enable on the three clocks above.

15:13 <robmur01> Er, there's already an upstream driver for that ISP for the last 3 years, so I'm not sure why you'd be starting from scratch anyway :/

15:14 <friedy> I need to make some changes to it. It's for a research project.

15:16 <CounterPillow> well what you pasted at least seems to be the upstream device tree so that's a good start

15:16 <CounterPillow> if it's not clocks then it's power domains

15:16 <CounterPillow> (usually, in my experience)

15:17 <robmur01> that's fair enough, but my point is that all the basic power management stuff is already there and presumably tested, so don't waste your own time trying to reimplement it, just add whatever you want within the framework of the existing driver and give yourself an easier ride

15:20 <friedy> Okay, so if the power is on, the clocks are on, and correct clock rates are set, then I shouldn't have a problem reading from the registers?

15:20 <jakllsch> no guarantees :-)

15:20 <jakllsch> but that's the minimum

15:21 <jakllsch> sometimes there's a clock or reset control in an unusal place

15:22 <CounterPillow> I agree with Robin here, just do your experiments as part of the already working driver, ripping out whatever you don't need. It's working and tested code you can rely on

15:25 <robmur01> for reference, rkisp1_probe() reads an ID register pretty early on; nothing looks unusual about the initial setup before that

15:27 <CounterPillow> pinctrl_pm_select_default_state(dev); is something I don't see often but I'm fairly inexperienced

15:28 <CounterPillow> should only come into play after a suspend/resume cycle though if I understand this correctly

16:04 <robmur01> CounterPillow: yeah, looks like that's probably just to avoid leakage or excess power consumption on the physical camera interface while idle

16:06 <robmur01> (although whether that should really be the phy driver's job is another question...)