RE: [PATCH] dt-bindings: imx: update scu resource id headfile

[Aisheng Dong]

> From: Marco Felsch [mailto:m.felsch@xxxxxxxxxxxxxx]
> Sent: Wednesday, February 20, 2019 9:53 PM
> 
> On 19-02-20 13:11, Lucas Stach wrote:
> > Am Mittwoch, den 20.02.2019, 11:21 +0000 schrieb Aisheng Dong:
> > > Hi Marco,
> > >
> > > > From: Marco Felsch [mailto:m.felsch@xxxxxxxxxxxxxx]
> > > > Sent: Wednesday, February 20, 2019 6:53 PM
> > > >
> > > > Hi Aisheng,
> > > >
> > > > On 19-02-20 09:49, Aisheng Dong wrote:
> > > > > > From: Marco Felsch [mailto:m.felsch@xxxxxxxxxxxxxx]
> > > > > > Sent: Wednesday, February 20, 2019 4:17 PM On 19-02-20 03:38,
> > > > > > Aisheng Dong wrote:
> > > > > > > [...]
> > > > > > >
> > > > > > > > > I don't like droping some ID's (e.g.
> > > > > > > > > IMX_SC_R_DC_0_CAPTURE0) by mark them as unused or even
> > > > > > > > > worse give them a other meaning. IMHO the scu-api should
> > > > > > > > > be stable since day 1 and the ID's should only be
> > > > > > extended.
> > > > > > > > > Marking ID's as deprecated is much better than moving them
> around.
> > > > > > > >
> > > > > > > > I agree the SCU APIs should be stable since day 1 and the
> > > > > > > > ID should ONLY be extended, but that is the best cases,
> > > > > > > > the reality is, there are different SoCs/Revision, some
> > > > > > > > revisions may remove the resources ID defined in pre-coded
> > > > > > > > SCU firmware, like the
> > > > > > > > IMX_SC_R_DC_0_CAPTURE0 etc., so SCU APIs removes them
> > > > > > > > after real silicon arrived, now latest SCU firmware marks
> > > > > > > > them as UNUSED, they could be replaced by some other new
> > > > > > > > resources in later new SoC, I am NOT sure, but if it
> > > > > > > > happens, this resource ID table should be updated anyway,
> > > > > > > > leaving the out-of-date resource ID table there will have
> > > > > > issues, since it is NOT sync with SCU firmware.
> > > > > > > > So how to resolve such issue? We hope the SCU firmware
> > > > > > > > should be stable since day 1, but the truth is NOT, could
> > > > > > > > be still some updates but NOT very often. And I believe
> > > > > > > > the updates will NOT break old
> > > > > > kernel version.
> > > > > >
> > > > > > Hi Anson,
> > > > > >
> > > > > > Please don't mix the dt-bindings and the kernel related stuff.
> > > > > > Unfortunately the bindings are within the kernel repo which in
> > > > > > fact is great for us kernel developer but the bindings are
> > > > > > also used in other projects such as barebox or other kernels
> > > > > > (don't know the BSD guys). So you can't ensure that your
> > > > > > change will break something. Please
> > > > keep that in mind.
> > > > > > IMHO solving that issue should be done by the scu firmware. I
> > > > > > tought the scu is a cortex-m4 with a bunch of embedded flash
> > > > > > and ram (I don't know that much about the scu since it is
> closed/black-boxed).
> > > > > > Why do you don't use a translation table within the scu? As I
> > > > > > said earlier I don't like the redefinition of ID's since they
> > > > > > are now part of the
> > > > dt-bindings.
> > > > > > The bindings can store up to 32bit values which is a large
> > > > > > number ;) IMHO wasting some ID's in favour of stability is a better
> solution.
> > > > > >
> > > > >
> > > > > As far as I know, those remove resource IDs are pre-defined and
> > > > > has never been used and won't be used anymore by SC firmware.
> > > > > (Anson can double check it) So I think it's safe to remove them
> > > > > or mark them as
> > > > deprecated.
> > > >
> > > > I think marking them as deprecated by a commentar is better than
> > > > redefining bits to be unused. As I said the bindings not only
> > > > linux related they are used in other projects too.
> > > >
> > >
> > > For other projects, the result is same because SC firmware does not
> > > support those Resource IDs.
> > >
> > > > > Personally I may prefer to remove them as they never worked to
> > > > > avoid confusing, especially at this early stage for mx8.
> > > >
> > > > So why they are there if they never worked? Wouldn't it a better
> > > > approach to start with a basic and working ID file and extend this
> > > > rather than adding id's because maybe the will work..
> > >
> > > Ideally yes, but may be hard in reality.
> > >
> > > SC firmware code usually is developed quite early before silicon comes.
> > > But the real chip may changes, e.g. removed or added something.
> > >
> > > AFAIK those resource IDs removed have never worked in SC firmware,
> > > and they're likely to come out of pre-silicon definition. But chip
> > > changes later.
> > >
> > > > Sorry but this seems wrong to me too.
> > > > I know the approach from driver development, adding a driver
> > > > specific *_reg.h file and later figuring out that those bits won't
> > > > work as aspected. As I said this will be driver and only linux
> > > > related so we can change them as many times as we want. But in
> > > > your case we are talking about dt-bindings and this approach won't work.
> > >
> > > I would agree with you if those resource IDs have worked before.
> > > But they never worked.
> > > Should we keep a thing never worked in device tree just because It
> > > makes us look like keeping compatibility?
> > >
> > > If that, I'd rather removing them to avoid confusing in the future. Life is
> long, right?
> > > I don't want people to bother with those unmeaning things anymore
> > > when they look at it.
> >
> > Removing things is totally fine if they have never worked. Re-using
> > the now "free" IDs is what is problematic. Only ever use previously
> > unused IDs when introducing new stuff into the SCU firmware. This is
> > something you need to communicate quite clearly with the SCU firmware
> developers.
> 
> I'm fine with removing too then there would be a gap. Don't know if it is better
> to have a gap or a comment that those ID isn't supported since fw-revision XYZ
> / silicon version ZYX.
> 
> Just some toughts:
> The gap will then suggest that this ID is free and was never assigned to some
> functionality. In real it was assigned previously and now gets redefined. Also as
> far as I can see there are no gaps currently. Let's wait for Rob's feedback.
> 
> As Lucas mentioned using IMX_SC_R_UNUSED* ID's for new functionality is
> totaly fine but redefine ID's seems wrong to me.
> 

Probably we still keep those removed IDs as IMX_SC_R_UNUSED*?

BTW, I guess Lucas's suggestion is:
" Re-using the now "free" IDs is what is problematic, Only ever use previously
unused IDs when introducing new stuff into the SCU firmware".
Because it's true that reused IDs are new stuff into SCU firmware.
So it's okay to change IMX_SC_R_UNUSED into a new ID, right?

Regards
Dong Aisheng

> Regards,
> Marco
> 
> >
> > Regards,
> > Lucas
> >
> >