Re: [PATCH 1/7] dt-bindings: media: renesas,isp: Add ISP core function block

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

On 2025-03-18 08:27:36 +0100, Krzysztof Kozlowski wrote:
> On 17/03/2025 16:34, Niklas Söderlund wrote:
> > SoCs. I know it's confusing and not logical but that's how they are 
> > made.
> > 
> > - One part is the ISP Channel Selector, this is a function that sits on 
> >   the CIS-2 receiver data bus. It is responsible for selecting which 
> >   CSI-2 Virtual Channel is routed to which DMA capture engine.
> > 
> >   This part is what the rcar-isp.ko driver have always supported and 
> >   every instance of the ISP that is described in tree deals with just 
> >   this one function as this is the one we always had documentation for.
> > 
> >   This block is the one the reg-names and clock-names labels "cs".
> > 
> > - One part that we now wish to add is the ISP Core. This is a 
> >   traditional ISP that act on image data in different ways. This is what 
> >   I try to model with the reg-name and clock-name labeled "core".
> > 
> >   This is new and we have not had documentation for this until recently.  
> >   Unfortunately the "core" and "cs" functions is implemented in the same 
> >   IP block. And to operate the "core" one needs to also deal with "cs".  
> > 
> > To make it more interesting all ISP Channel Selectors (cs) do not have a 
> > companion ISP Core, but most do. The lack of a ISP core is OK, it just 
> > means that video capture path can't manipulate the image as much as 
> > paths that do.
> > 
> > It's not ideal but to support the ISP Core and ISP Channel Selecotr the 
> > rcar-isp.ko module needs both "core" and "cs" clocks and regs. And to 
> > support just the Channel Selector it only needs the "cs" resources.
> > 
> > 
> > Sorry if I have been confusing. A good example of this is patch 4/7 in 
> > this series. It shows the V4M board that have 2 ISP instances, one that 
> > have both cs and core functions, and one that only have cs function.
> 
> Based on this I think the instances with ISP core are not the same
> hardware as instances without. You have there different (new)
> programming model for entirely new part of hardware not present in "old"
> instances.
> 
> Different device means different compatible.
> 
> And judging by the address:
> reg = <0 0xfed00000 0 0x10000>, <0 0xfec00000 0 0x100000>;
> 1. 0xfec0 < 0xfed0
> 2. Huge address range
> 
> that's not "renesas,r8a779h0-isp" at all, but your old "ISP" device is
> actually a part of that 0xfec0_0000.
> 
> Probably the channel selector should have never been called "ISP"
> because it does not process images. :/

There are always room for improvement, but we can only try and create 
bindings that describe the hardware as it is documented.

In the block diagrams the channel selector and the core isp are in the 
same block.  And for better or worse to operate the ISP to process 
images the driver need to poke at the channel selector, even tho I fail 
to see why some of the core registers where put in the cs block.

On Gen3 an equally interesting hardware design can be found, the CSI-2 
channel selector was there placed together with the IP block for image 
capture DMA engines... Luckily that only created a mess in the driver 
and not in the bindings.

Thanks again for your feedback, I really learn a lot!

> 
> > 
> >>
> >> What is this ISP core responsible for inside Renesas ISP? How many ISPs
> >> are inside of SoC?
> > 
> > The ISP core is responsible for image manipulation. ISP Channel Selector 
> > for CSI-2 channel routing. The number of ISP varies between SoCs:
> > 
> > 
> > V3U: Have 4 ISP instances, all 4 have cs and core.
> > V4H: Have 2 ISP instances, both have cs and core.
> > V4M: Have 2 ISP instances, one have cs and core, one have only cs.
> 
> 
> Best regards,
> Krzysztof

-- 
Kind Regards,
Niklas Söderlund